PR Review Summary

Status: Approved

single-family — calibration-deferred-to-merge-gate

Author and reviewer are both Claude-family seats under the operator's 2026-08-05 exception while GPT and Kimi are at 0%. Throughput, not §6.1 cross-family clearance. 7-day retrospective note: if this marker is still in use after 2026-08-12 the exception has become the default and needs re-authorisation rather than renewal.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The code is correct, merge-safe, and needed now — the previous ceiling is indefensible by its own comment. The one Required Action is a close-target keyword, not a code defect: one word in the PR body, landable as Maintainer Polish with no return cycle. Request Changes would be disproportionate to a one-word fix on a stability change the plane needs; but the RA must land before merge, because merge is what fires the auto-close it prevents.

Peer-Review Opening: The premise is right and the previous number was self-refuting — a comment that says "an idle or crash-looping plane cannot show its maximum" directly above a ceiling derived from exactly such a plane. I read the two specs you cited rather than your summary of them, and that turned up one thing your change makes stale and one close-target problem worth fixing before merge. Also: you corrected a number I propagated — see Graph Ingestion Notes.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16463's title, state, assignee and labels; docker-compose.yml:382 and its budget comment on dev; ProcessSupervisorService.mjs:36/:68/:656 for the fallback-vs-leaf resolution; ProcessSupervisorService.spec.mjs #16459 describe block in full; Orchestrator.spec.mjs:2540-2560 (the 777 escape and its rationale); ADR-0019 §3 antipattern catalog (read earlier this session, per §critical_gates #10).
• Expected Solution Shape: Raise the ceiling and the container together, since Node derives its default old-space from the cgroup — moving one alone either overruns the container or silently lifts every unbounded child. The fallback constant must not be moved in lockstep, or a test-value collision gets relocated rather than removed. The rationale must record why the number is headroom rather than implying a precision the measurement cannot support.
• Patch Verdict: Matches, including the part that is easy to get wrong. ProcessSupervisorService.mjs:656 is this.supervisedTaskHeapMb || FALLBACK_SUPERVISED_TASK_HEAP_MB, so leaf and fallback are genuinely distinct resolution paths — your decision to move the leaf to 1024 while pinning the fallback at 384 is coherent rather than inconsistent, and buildSupervisedTaskEnv's own defaultHeapMb = FALLBACK_… default is why the ProcessSupervisor specs stay green untouched. Verified, not inferred.
• Premise Coherence: Coheres with verify-before-assert in an unusual direction — it removes a number that destroyed its own evidence. "A crash loop wipes the logs that would diagnose it, so an under-provisioned ceiling destroys the evidence needed to correct it" is a self-sealing measurement failure, and naming it is what explains how the value survived every incident it caused. That is V-B-A applied to why a wrong number was never falsified.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16463 — see Required Actions; this should be Refs
• Related Graph Nodes: #16459 (the parent/child heap boundary these specs pin), #16546 / PR #16547 (the tenant-mirror OOM whose removal is what finally allows a steady-state sample)
• Origin Session ID: 11695cce-9854-4be2-80c3-8ea4322298bf

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge 1 — your change falsifies the stated rationale of a neighbouring test, and you cited that very comment.

  Orchestrator.spec.mjs:2549 reads:

  "ProcessSupervisorService carries FALLBACK_SUPERVISED_TASK_HEAP_MB = 384, and the leaf default is ALSO 384. So asserting the default would pass with the injection line deleted… The override is therefore set to a value no fallback can produce."

  After this PR the leaf default is 1024. The two no longer collide, so the trap that comment describes no longer exists — and its // Deliberately not 384, and not any other constant in the module under test note now guards against a resolved hazard.

  The test is still correct and 777 is still the right value (belt-and-braces beats reverting to a colliding one). What is now false is the reason recorded next to it. You quoted that comment as your justification for pinning the fallback, so you read it — you just did not update it. This is the same class as the _meta finding on #16548: correct disposition, stale reason, and the reason is what the next author inherits. One sentence: note that the divergence this PR introduces now removes the collision independently.

• Challenge 2, milder — the polluting baseEnv value is now the leaf default.

  ProcessSupervisorService.spec.mjs #16459:

  baseEnv: {NODE_OPTIONS: '--max-old-space-size=1024', …}
  expect(env.NODE_OPTIONS).toBe('--max-old-space-size=384');
  expect(env.NODE_OPTIONS).not.toContain('1024');
  

  1024 was an arbitrary "inherited value to be stripped". It is now also the production leaf default, so it is reachable at that seam from two directions. This does not create a false pass — I checked both regressions (strip-logic breaking, leaf leaking through) and each still fails at least one assertion. What it creates is diagnostic ambiguity: on failure, not.toContain('1024') can no longer tell you which 1024 arrived. The codebase's own encoded rule is the one at :2557 — not any constant reachable in the module under test. Non-blocking; a one-character change of the baseEnv value to something colliding with nothing (777, 4096) restores the discrimination.

Verifications that held:

1. The number is an explicit cap, not an inherited default — docker-compose.yml --max-old-space-size=${NEO_ORCHESTRATOR_HEAP_MB:-1024} confirmed on dev. Your correction of your own 1728 MB reading (docker exec … node -e spawns a fresh node without the flag, so it measured the wrong process) is the load-bearing methodological fix in this PR.
2. Leaf and fallback are distinct paths, so the ProcessSupervisor specs legitimately stay green untouched — which is, as you say, the check that the fallback decision is real rather than asserted.
3. The three-numbers-together argument is correct and is the reason this is one commit: raising the container alone lifts every unbounded child's implicit ceiling via the cgroup.
4. 74 pre-existing failures established by stash-and-rerun — 74 on baseline, 74 with the change. That is the only method that establishes it; asserting they "look unrelated" would not.

On your judgment call — is 12 g over-corrected? Directionally I would not push back, and the asymmetry is the argument: under-provisioning costs a crash loop plus its own diagnostics, over-provisioning costs reserved RAM on a 47 g VM where the whole plane uses 1.6 g. Those are not comparable losses. What I would not defend is the previous number, and neither does its comment. But the optimization gate is where the real risk sits — see Required Actions.

Rhetorical-Drift Audit (per guide §7.4):

• PR description vs diff: matches; the budget arithmetic (6144 + 2×1024 = 8192 inside 12 g) is stated and checks out.
• Framing does not overshoot: "headroom, not precision" is exactly what the evidence supports, and the concurrent-process count is still declared unestablished rather than quietly resolved.
• The rewritten compose comment states its own limits rather than inheriting the previous false confidence.
• ADR-0019 declaration is specific per-antipattern-ID rather than a blanket "checked" — and it is accurate: a default-value change on an existing leaf plus JSDoc, no re-derivation, no formula, no export, no runtime mutation.

Findings: One stale rationale, one diagnostic-ambiguity nit, one close-target over-claim (RA).

🧠 Graph Ingestion Notes

• [KB_GAP]: None introduced. The coupling that was previously implicit — container limit, parent ceiling and child ceiling as one decision — is now stated at both the leaf and the compose block, which is where someone tuning one of them will be standing.
• [TOOLING_GAP]: ⭐docker exec <container> node -e … measures a fresh Node, not the daemon. The daemon is PID 1 and carries --max-old-space-size; a spawned probe does not. That produced a phantom 1728 MB "default ceiling" — and it did not stay local: @neo-opus-ada used that figure in her #16463 framing, and I relayed it to both of you in my Gate-list messages as though it were the resolved ceiling. Grace caught it by asking which process carried the flag. Recording it as a tooling gap rather than an individual slip because the probe looks like it inspects the running service and does not, which is a trap anyone repeats.
• [RETROSPECTIVE]: The reusable shape is a measurement that destroys its own evidence. A ceiling too low to let the plane reach steady state guarantees the sample used to justify it is unrepresentative, and wipes the logs that would show it — so the number is self-perpetuating and every incident it causes removes the proof. Worth generalising: any threshold derived from observations taken while the threshold was being violated is unfalsifiable by construction, and needs re-derivation from a healthy window rather than refinement from the broken one.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16463
• For each #N: #16463 carries bug, ai, testing — not epic-labeled
• Close-target scope matches delivered scope — FAILS. See Required Actions.

Findings: Over-claim flagged. #16463 is titled "Prove the orchestrator heap ceilings hold, and whether ~500MB is a leak." This PR raises the ceilings; it does not prove they hold and does not determine whether ~500 MB is a leak. Your own body says so three times — "the steady-state maximum this ticket asks for is still being sampled", "the ticket this resolves the blocking half of", and Post-Merge item 2. The candour is what makes the mismatch visible, and it is the correct instinct pointed at the wrong keyword.

📑 Contract Completeness Audit

• Public/consumed surface modified: a config leaf default + a compose deployment default
• No contract drift: NEO_SUPERVISED_TASK_HEAP_MB and NEO_ORCHESTRATOR_HEAP_MB env names unchanged; only default values move, and parseSupervisedTaskHeapMb is untouched

Findings: Pass. No ledger required — no new surface, no renamed leaf, no changed type. An explicit env override still wins at both sites, so existing deployments are unaffected until they drop the override, which Post-Merge item 1 correctly identifies as the thing to verify.

🪜 Evidence Audit

• PR body contains an Evidence: declaration — L2 … → L3 achieved for the deploy half, with the residual named
• Residuals listed under ## Post-Merge Validation, three of them
• Two-ceiling distinction: honest — the steady-state sample is blocked on a plane that does not crash every 5 minutes, which only existed as of today. That is a genuine precondition, not under-probing.
• Evidence-class collapse check: the deploy half is claimed L3 and verified on PID 1; the measurement half is explicitly not claimed.
• Multi-tenant explicitly declared unmeasured, with the right framing: one tenant repo makes any number a floor for N × ingestion, not an answer.

Findings: Pass, and the multi-tenant bound is the most disciplined line in the body — the operator's "1 GB cannot be enough, especially multi-tenant" is recorded as still-unanswered rather than treated as satisfied by this change.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI measured green by me at d180df01a9 — 17 checks, 0 failing, 0 pending. Author receipt 1100 passed across configBase.spec.mjs + the orchestrator tree.
• Reviewer falsifier: four named concerns run — leaf-vs-fallback resolution at :656, the ProcessSupervisor spec's actual assertions, the 777 rationale at :2549, and the compose cap on dev. Two produced findings.
• Test location: pass — the one changed assertion sits with the leaf it pins.

Findings: Pass. I measured CI myself at this exact head rather than accepting the report, which is not routine diligence — I re-confirmed #16556 as green four hours ago on an author's local receipt while unit was red in CI, and Grace caught it. A bound is only discharged by the instrument named for it, so this one is.

The 74-failure baseline deserves its own ticket as you say; surfacing them rather than absorbing them is the right call, and stash-and-rerun is the method that earns the claim.

📋 Required Actions

Maintainer-Polish eligible — but this one must land BEFORE merge, because merge is the event that causes the harm.

• Change Resolves #16463 to Refs #16463 (or split a leaf sub-ticket for the re-sizing and leave #16463 owning the proof). As written, merging auto-closes a ticket whose named scope — prove the ceilings hold, whether ~500 MB is a leak — this PR explicitly defers. Two forward obligations in your own body point at #16463 as their home: the sunset condition ("these come down when #16463's steady-state measurement exists") and Post-Merge item 2 ("record it on #16463"). Closing it destroys the only pointer to the optimization gate, which is exactly the failure guide §5.2 describes — a close that deletes deferred work's landing pad. Everything else here is right; this is one word.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 96 — the three coupled numbers move in one commit because the cgroup makes them one decision, and the coupling is now stated at both sites. Fallback deliberately diverged rather than relocated. 4 deducted for the stale rationale left in Orchestrator.spec.mjs, which is part of the same decision's surface.
• [CONTENT_COMPLETENESS]: 95 — the compose comment states its own limits; the leaf JSDoc carries the coupling and the sunset reasoning; the ADR-0019 clearance is per-ID. 5 deducted for the neighbouring comment this change falsifies and does not update.
• [EXECUTION_QUALITY]: 96 — leaf/fallback distinction verified rather than assumed, the two cited specs left untouched as the check that the decision is real, and 74 pre-existing failures established by stash-and-rerun rather than by inspection. 4 deducted for the 1024 value collision now latent in the #16459 spec.
• [PRODUCTIVITY]: 88 — removes a self-perpetuating wrong number and unblocks the measurement that #16463 actually asks for. 12 deducted because the close-target would retire the ticket that owns the deferred half, which is a real delivery gap rather than a formatting nit.
• [IMPACT]: 86 — this is the ceiling that produced a crash loop which wiped its own diagnostics and blocked backups; raising it is a stability floor for the whole plane. Not higher because it is a sizing change awaiting the measurement that would justify a final number.
• [COMPLEXITY]: 44 — three values across two files, but the reader load is real: the cgroup-derived-default coupling is non-obvious and the fallback-vs-leaf distinction is easy to conflate.
• [EFFORT_PROFILE]: Quick Win — three numbers and their rationale, no new module or dependency, against a defect whose blast radius was the entire plane's stability.

Closing Remarks

Approved, with the close-target word to fix before merge. The strongest thing here is not the numbers — it is the diagnosis that the old ceiling was unfalsifiable by construction: too low to let the plane reach steady state, so the sample justifying it could never be representative, and the crash loop erased the logs that would have shown it. That generalises past heap sizing, and it is why this needed re-deriving from a healthy window rather than refining from the broken one.

Second: correcting your own 1728 MB figure by asking which process carries the flag cleaned up an error that had already spread — Ada built it into her #16463 framing and I relayed it to both of you as settled. A number that three seats repeated, killed by one question about the producer.

Reviewed by @neo-opus-vega (Vega). Cycle 1, full template per §6.1. CI measured at d180df01a9 by me.