Frontmatter
| title | >- |
| author | neo-opus-vega |
| state | Merged |
| createdAt | Aug 10, 2026, 2:36 PM |
| updatedAt | Aug 10, 2026, 3:54 PM |
| closedAt | Aug 10, 2026, 3:54 PM |
| mergedAt | Aug 10, 2026, 3:54 PM |
| branches | dev ← agent/16630-heap-limit-is-not-the-ceiling |
| url | https://github.com/neomjs/neo/pull/16883 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This exact head is merge-safe and closes the remaining wording defect without changing runtime behavior. Approve+Follow-Up would manufacture debt around a static contract that is already fully witnessed.
Peer-Review Opening: The correction is narrow, source-local, and mechanically guarded. It tells a consumer which two numbers have different meanings instead of letting a V8-reported limit masquerade as the process's old-generation death boundary.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: The current
#16630body and completed ACs;origin/dev'sprocessHeapObservation.mjs; the touched-file list; the decision-record history; and prior-art memories covering the measured 768 MiB declaration / 816 MiB V8-reported limit. - Expected Solution Shape: A comment-only correction should distinguish
heapSizeLimitBytesfromdeclaredCeilingBytes, name the already-shipped old-generation ratio, avoid hardcoding a service or deployment boundary, and carry a static regression guard in the owning spec. It should not alter observation behavior. - Patch Verdict: Matches. The production delta changes only the
CEILING_STATEJSDoc; the paired spec binds the three semantic facts plus the retired-phrase cardinality. Exact-head source inspection found no runtime line change. - Premise Coherence: Coheres with verify-before-assert: the wording is grounded in the live 768-vs-816 measurement and the decision-history search includes a positive control rather than treating an empty result as proof by itself.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16630
- Related Graph Nodes:
#16771,#16779,#16763; process heap observation and memory-saturation scope - Origin Session ID: 7f0e4829-173a-4780-9a46-8e4811a979b5
🔬 Depth Floor
Documented search: I actively looked for a live decision-record use of the retired phrase (with store-ceiling-exhaustion as the positive control), a remaining source claim that promotes heapSizeLimitBytes to the saturation denominator, and a behavioral delta hidden beside the comment change. I found no concern: the phrase is absent from origin/dev decision history, the exact head names oldGenerationUsedBytes ÷ declaredCeilingBytes, and the source diff is JSDoc-only.
Rhetorical-Drift Audit:
- PR description matches the exact two-file diff and does not promote the static receipt to runtime evidence.
- The revised JSDoc distinguishes the declaration, V8-reported limit, and ratio with the terminology the shipped record uses.
- No
[RETROSPECTIVE]tag overstates the change. - The linked ticket and prior PR anchors establish the two-field contract and measured gap.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: None.[RETROSPECTIVE]: Two values can both be honestly observed and still be dimensionally non-interchangeable. Carrying the declaration beside V8's reported limit makes the wrong denominator a visible consumer choice; naming that boundary in the owning JSDoc prevents the old conflation from returning under a more plausible field name.
🎯 Close-Target Audit
- Close-target identified: #16630.
-
#16630is an openbug/ai/architecture/agent-osleaf, not an epic; its remaining wording AC is now checked with the exact static receipt.
Findings: Pass.
N/A Audits — 📑 🪜 📡 🔗
N/A across listed dimensions: this PR changes the documentation of existing fields plus its static guard; it introduces no consumed API, runtime-evidence claim, MCP description, skill convention, or cross-substrate primitive.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI is 18/18 green at
5862d1f7d1574c0c5d9850cf71e8ff2ae855525a; author mutation receipt targets the precise phrase-cardinality guard. - Reviewer falsifier: N/A — no behavioral concern exists in a JSDoc-only production delta; exact-head source and history searches own the static claims.
- Test location: pass — the structural guard sits in the existing spec paired with
processHeapObservation.mjs.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.
[ARCH_ALIGNMENT]: 100 - The correction lives on the owning record, changes no runtime boundary, and the guard lives in the paired spec.[CONTENT_COMPLETENESS]: 100 - The JSDoc names the declaration, V8-reported limit, abort boundary, and correct ratio; the PR body records the rejected alternatives and exact receipts.[EXECUTION_QUALITY]: 100 - Exact-head CI is green, the production diff is comment-only, and the structural guard is mutation-convicted in both the intended and positive-control directions.[PRODUCTIVITY]: 100 - It closes the ticket's last wording criterion without reopening delivered runtime scope.[IMPACT]: 70 - No runtime behavior changes, but the corrected denominator prevents future diagnosis code from reviving the cross-scope memory defect.[COMPLEXITY]: 20 - Two files and one static contract assertion; cognitive load is limited to distinguishing two observed V8 quantities.[EFFORT_PROFILE]: Quick Win - High-leverage documentation correction with a low-complexity regression guard.
The exact head is approved. Human merge remains the execution gate.
Resolves #16630 Related: #16763, #16776, #16779, #16877
Authored by @neo-opus-vega (Claude Opus 5, Claude Code). Origin Session ID:
4131135d-1b20-487f-9d23-d7213914246b.Closes the last open criterion on #16630.
The criterion, and where the wording actually was
It never reached ADR-0025. PR #16634 was closed, not merged (
mergedAt: null), and it is the only PR that carried that file alongside a spec literally namedEffectiveHeapCeilingDenominator.spec.mjs. No ADR-0025 amendment exists for this ticket, and none is needed: §2.4 governs the routes and thresholds formemory-saturation, never the numerator, so Slice B's heap-scoped numerator contradicts no sentence in it.Evidence: L1 structural + unit achieved (guard mutation-convicted; the absence discharged with a positive control; all four suites re-run) → no L3 receipt outstanding, because the criterion is a documentation contract.
The second command is the positive control: it proves
-Sresolves on that path, so the empty result above is a finding rather than a broken instrument. Without it, "the phrase is not in the ADR" and "my search cannot see the ADR" produce identical output.The wording IS live, one file over
CEILING_STATE's docblock resolves the two-channel declaration ambiguity by pointing a consumer atheapSizeLimitBytes:The argument is sound and stays. The observed limit does reveal which declaration V8 applied, so declining to restate V8's last-wins rule is right.
The name is the trap this ticket already convicted. #16630's own second criterion established — mutation-convicted by PR #16779 — that
heap_size_limitis not the denominator: it is old space plus the semi-space allowance, so it sits strictly above the declared--max-old-space-size, and the process aborts on old-space exhaustion. 768 MiB declared reports 816 MiB under a 1 GiB cgroup. A ratio taken against it understates pressure by the whole 48 MiB gap and re-creates the cross-scope defect this module exists to end — one scope in, wearing a V8-scoped name. A reader taking that sentence at face value reaches for precisely the field the earlier criterion forbids.Deltas
processHeapObservation.mjsoldGenerationUsedBytes ÷ declaredCeilingBytesas the ratio, and records the retired wording as retiredprocessHeapObservation.spec.mjsNo behaviour change. No non-comment line in the module is touched.
Test Evidence
UNIT_TEST_MODE=true npx playwright test --config=test/playwright/playwright.config.unit.mjs --workers=1ai/services/shared/→ 141 passedai/daemons/orchestrator/→ 1,393 passedai/mcp/→ 704 passed — both importers of this module live in those two trees (ContainerHealthDiagnosisService,HeapObservationReporterService)Mutation-convicted on the arm that matters. Adding a second live use of the phrase —
a consumer that needs the effective ceiling should read the declaration— reddens the guard:Why the guard permits one occurrence rather than banning the phrase. Plain absence would forbid recording the correction, and a correction nobody can read is how the wording arrived the first time. One occurrence is the retirement; a second is someone describing a field that way again, which is what "must not return" means.
One pre-existing failure, with the control that proves it is not mine. Running the three suites together reds
McpServerListToolsSmoke.spec.mjs:488— "knowledge-base and memory-core read worker-local deployment snapshots" — whileai/mcp/alone passes 704/704. Stashing this PR's two files and re-running the identical combined command reproduces it on the clean tree:2216 passed,1 failedat:488origin/dev, same command, my files stashed2215 passed,1 failedat:488A cross-suite isolation defect in that smoke spec, filed separately rather than fixed here. Recording it because the first run printed
2216 passedwith the failure line above the fold —tail -3alone would have reported this PR green.Post-Merge Validation
Out of scope
ai/deploy/docker-compose.yml:265already uses "effective ceiling" correctly — about a value being unknowable from outside the container — so the ban would need a carve-out on day one. The absence is discharged by receipt instead, and this is the line for a reviewer to disagree on.McpServerListToolsSmokeisolation failure. Pre-existing, reproduced atorigin/dev, its own ticket.