PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code is right and I am asking for no change to it. Both required actions are documentation-of-record: the ticket's Contract Ledger row 2 drifted on name, unit and mechanism, and §5.4 makes ledger sync a stated merge gate rather than reviewer discretion. Approve+Follow-Up would be the wrong shape here — this is not debt to schedule, it is a two-minute correction to the artifact that defines the contract, and deferring it means the ledger stays wrong for exactly as long as anyone might read it.

Peer-Review Opening: This is the strongest test authorship I have reviewed this window, and I want to name why before the findings: you asserted the request rather than its shadow, you wrote the call-site test that an isolated corpus cannot substitute for, and you reported the fixture-shaped-like-the-bug yourself, unprompted. The two required actions below are both on the ticket, not the diff.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17357 in full (problem, Contract Ledger, all five ACs, Out of Scope) and its correction comment; the changed-file list; learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md (§critical_gates 10 — the diff touches ai/configBase.mjs); origin/dev source of DeploymentStateBridgeService.mjs, deploymentStateBridgeStore.mjs and the deploymentStateBridge leaf block; the sibling boundUtf8Tail as precedent; ContainerHealthDiagnosisService.mjs as the downstream consumer. Prior-art sweep run over the bridge/startup/byte-bounding decision space.
• Expected Solution Shape: A logs.startup envelope carrying a bounded head of the current incarnation, keyed so a restart invalidates it structurally rather than on a clock, with the bound declared as a canonical leaf(default, env, type) and read at the use site. It must NOT hardcode the read bounds, must NOT let an absent head serialize as an empty string, and must isolate in test by construction — a config seam, never a mutation of the shared AiConfig singleton.
• Patch Verdict: Improves on the expected shape. I expected retention with a replacement arm, because that is what the ticket prescribed; the shipped re-derivation removes the entire stale-record class instead of implementing it correctly, which is the better answer. Specific evidence that moved me: readStartupLogHead holds no record it must expire — the cache key is StartedAt, so Map.set on a restart is a replacement by construction, and there is no arm to get wrong. The one place the diff falls short of its own standard is tail: 10_000 (see Depth Floor).
• Premise Coherence: Coheres — verify-before-assert, and unusually literally. The ticket exists because a 23 GB attribution rested on a fitted formula when the engine's own numbers had been available and were discarded by retention; this PR makes the measurement reachable so the next attribution can be read rather than modelled. The unavailableReason taxonomy is the same value applied to the projection itself: it refuses to let absence-of-evidence serialize as evidence-of-absence.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17357
• Related Graph Nodes: #17356 (sibling from the same 2026-08-18 incident), PR #17362, D#17085 (the mechanically-guarded form axis this PR's parity lint is a live specimen of), #17070 (the silent-truncation class from the same plane)
• Origin Session ID: a105d215-c261-4b34-82a9-546596f665ef

🔬 Depth Floor

Challenge: Four, none blocking.

1 — The unavailable path is never cached, and the case that pays for it is the one you named as expected.

Every unavailable(...) return sits above the startupLogHeadsByService.set(...), so only successes are cached. For unreadable that is correct — a transient Docker failure must retry. For window-empty-or-rotated it is not: rotation does not un-rotate, and StartedAt does not move until a restart, so that service pays a Docker --since/--until scan on every sweep, forever, and can never succeed. Your own JSDoc argues this harder than it argues for the success-path cache: "re-reading it every collection would pay a Docker call per service per sweep to receive the same bytes" — here it receives none.

The reason I am not making it a required action is that the obvious fix is wrong, and the correct one needs a decision you should make rather than me: inside the first windowMs of a container's life an empty window is legitimately transient (the banner may not have been written yet), so the negative may only be cached once nowFn() > startedAtMs + windowMs. That is a real design choice with a clock in it, and this PR is otherwise clock-free by design.

2 — tail: 10_000 is a hidden bound sitting two lines from a leaf.

logTail is leaf(120, 'NEO_DEPLOYMENT_STATE_BRIDGE_LOG_TAIL', 'number'). Its sibling bound on the head read is an inline literal with a comment explaining why it is generous. Both are read bounds on the same Docker operation in the same service; one is configurable and env-overridable, the other is not reachable without a code change. ADR-0019's position is that resolution belongs to the leaf, and "the window is the bound that matters" explains why 10_000 is safe, not why it should be invisible. Non-blocking because it is a saturation guard rather than a tuning knob — but it is the one spot where this diff does not hold itself to the standard it applies everywhere else.

3 — lines and text disagree by one whenever the head ends with a newline.

text  = bounded.text.trim();
lines : text.split('\n').length,   // counted on the TRIMMED text
text  : bounded.text,              // published UNTRIMMED

boundUtf8Head returns sliced.slice(0, lastNewline + 1) on truncation — trailing newline included — and returns text verbatim when under cap, which for container logs almost always ends in \n. So a consumer recomputing record.text.split('\n').length gets record.lines + 1. lines is the more correct number, which is exactly why the mismatch is worth a line of thought: this record's whole purpose is that a reader can trust what it says about itself.

4 — The multibyte claim in boundUtf8Head's JSDoc is true on one branch of two.

"Trimming to a line boundary also removes the split-multi-byte-character case for free."

It does, on the lastNewline > 0 path. The fallback path returns sliced — a raw buffer.subarray(0, maxBytes).toString('utf8') — which yields U+FFFD when the cap lands mid-character, exactly as boundUtf8Tail does at its own edge. The spec covers the over-long-single-line case with 'xxxxxxxxxx', which is ASCII and cannot expose it. Since you invited a seat at the twin asymmetry specifically: the asymmetry is right, and I could not falsify the design reason for it. What I could falsify is the parenthetical — the twins differ in which end survives, not in whether a degenerate cut can split a character.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates. The claim I checked hardest was "the ticket's own retention machinery turned out to be unnecessary" — readTargetLogs does bound server-side when both since and until are present, so the re-derivation premise holds and the retention removal is earned, not asserted.
• Anchor & Echo summaries: precise, and the readStartupLogHead JSDoc's "Nothing is stored, and that is the design" is a durable statement of intent rather than a snapshot of this cycle.
• [RETROSPECTIVE]: n/a — none claimed.
• Linked anchors: restartChurn and memoryPressure.receipt genuinely establish the survive-your-observation-window pattern the ticket cites them for; I read both.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: Two things here are worth keeping past this PR. First, the fixture that matched the bug: your isolated arms returned a bare payload while the reader consumed the response without unwrapping .data, so all eight passed and only the published-record test failed. You reported it in the PR body without being asked. A fixture built from the author's reconstruction of a contract can only confirm that reconstruction — the real {data, proof} envelope is the fix, and the general rule is that the fixture's shape must come from the producer, never from the reader. Second, asserting the request and not only the response: expect(calls[0].since) / expect(calls[0].until) tests the mechanism claim — that the window is bounded server-side — where asserting only result.text would have passed identically against a client-side trim. Most tests of a "we bound it upstream" claim never touch the call site at all.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17357 (PR body, newline-isolated, line 1). No Closes / Fixes in body or in the commit range.
• #17357 carries enhancement, ai, agent-os — not epic. Valid leaf target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (two rows).
• Implemented PR diff matches the Contract Ledger — it does not. Row 2 has drifted on all three axes.

Your correction comment states "The Contract Ledger row still holds" — singular, and true of row 1: services[].logs.startup is published exactly as promised, envelope and all. Row 2 is not addressed there, and it is the row the retention removal invalidated. logStartupMaxBytes is a leaf that does not exist, so a reader going to the ticket for the config surface gets a name they cannot grep and a mechanism that was deliberately removed.

The row-1 Evidence cell needs the same pass: it promises "a fixture asserting a restart replaces rather than appends." The shipped spec asserts the replacement — calls length 2, new incarnationStartedAt, fresh text — but not the not-appends half. I agree AC-2's mechanism is genuinely superseded; what survives supersession is the output property, because "two incarnations' startup facts read as one and the older values look current" is a statement about the record, not about how it got there.

Findings: Contract drift flagged — see Required Actions.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved ≥ required: L3 → L3 required, Residual: none. Verified rather than read off the body — every AC on #17357 is discharged in-tree (the read, the bound, the cache and the publication are all in-process), so there is no runtime surface the sandbox cannot reach and no residual to own.
• Two-ceiling distinction: n/a — nothing shipped below its required class.
• Evidence-class collapse: this review does not promote the unit evidence above L3.
• Deployment causality: n/a — no external receipt used as a merge gate.

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml surface is touched, and the PR introduces no skill, convention, or AGENTS*.md change that another substrate would need to fire.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at d1a2fb103b9f5aaea20966feab0fc6810dbb2e8a — gh pr checks exit 0, 20 checks including unit, integration-unified, integration-parity, components, lint-pr-body. Author receipt of 316 passing across changed basenames and their importers is consistent with it. The config-leaf-parity.json snapshot for orchestrator.deploymentStateBridge.startupLogWindowMs is in the same commit as the leaf, per the lint's own instruction.
• Reviewer falsifier: n/a — no named behavioural concern survived source reading. The one I expected to find did not hold up: I went looking for a consumer that treats a truthy logs as "we have tail data", since summarizeLogs now returns {startup} where it previously returned null. ContainerHealthDiagnosisService.mjs:1708 guards on typeof logs.text !== 'string', not on truthiness, so the head-without-tail shape degrades to logs-unavailable correctly and cannot reach the attribution path at :1732.
• Test location: pass — both specs sit beside the modules they cover under the mirrored test/playwright/unit/ai/... path.

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following. Both are on #17357; neither asks for a change to the diff.

• Sync Contract Ledger row 2 to shipped reality. Replace bridgeConfig.logStartupMaxBytes / "bound on the retained head" with startupLogWindowMs / "server-side read window in ms, byte-bounded by the existing logMaxBytes". §5.4 blocks approval on ledger drift, and this row names a leaf that does not exist.
• Discharge the row-1 Evidence cell, either way. Add expect(third.text).not.toContain(STARTED) to the incarnation-cache spec, or amend the cell to state the property that is actually asserted — a restart produces a fresh read keyed on the new incarnationStartedAt. Your call which; I have no preference between them, only that the table and the spec agree.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — the head helper sits beside its twin in the store, the reader sits in the service that owns the collection, and publication inside logs is argued rather than assumed. The leaf is canonical leaf(default, env, type) with no A4 env-ternary, no A5 hasEnvValue, no A7 formula re-derivation, and the config default-parameter is a B4-avoiding injection seam rather than a singleton mutation. 8 deducted for tail: 10_000 as a hidden bound beside a declared one.
• [CONTENT_COMPLETENESS]: 85 — JSDoc is above the bar on both new surfaces, including the negative space (why not retention, why not a bigger logTail, why not an empty string). 15 deducted for the stale Contract Ledger row and its Evidence cell, which are part of the contract-of-record this PR ships against.
• [EXECUTION_QUALITY]: 88 — seven unavailable arms each asserted for reason, text and lines; the twins asserted against each other on one input; the published-record path covered rather than only the reader. 12 deducted across the uncached negative on the expected path, the lines/text off-by-one, and the unasserted not-appends property.
• [PRODUCTIVITY]: 95 — all five ACs reached, AC-2 declared superseded on the record with the mechanism named rather than quietly re-interpreted. AC-5, the motivating case, is the one that matters and it is answerable from the snapshot alone.
• [IMPACT]: 75 — closes a diagnostic blind spot that already cost a 23 GB misattribution on the adopter plane, and generalises past one engine's banner to every process that reports its resolved geometry exactly once. Bounded to Agent OS observability rather than a consumed public contract.
• [COMPLEXITY]: 60 — one new method, one new helper, one leaf and one call-site rewire across four production files; the reader load is concentrated in a single method with seven exit arms, each of which has to be understood as distinct from the others.
• [EFFORT_PROFILE]: Quick Win — 164 production lines close a class of unreadable-startup-facts across every observed service, and the 248 spec lines are what makes that claim checkable rather than asserted.

The retention removal is the part I would keep from this PR if I could keep only one thing. The ticket asked for a stale-record arm and you came back having established that there is nothing to make stale — that is a better outcome than implementing the prescription correctly, and you recorded the supersession instead of letting the AC quietly mean something new. Fix the table it left behind and this is merge-eligible.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z