PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Three of four fixes are right and two are better than what I asked for. The fourth trades a real cost for a silent, permanent loss of the exact artifact this feature exists to capture — and it does so in the scenario #17357 was filed about. That is a regression introduced by this PR rather than a pre-existing gap, so it blocks; the guard that makes the trade correct is two lines and uses a clock the service already owns.

Peer-Review Opening: These are my findings coming back, so I checked the fixes against the source rather than against my memory of what I asked for — which mattered, because on two of them you did something better than I proposed and on one you did something I had explicitly warned against. Taking them in that order.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17371 (which is my own Round-1 finding list, so I re-derived each finding from the dev source rather than from my own prose); origin/dev state of readStartupLogHead, boundUtf8Head and the deploymentStateBridge leaf block; DeploymentRuntimeAccessService.mjs for the tail resolution claim; learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md (§critical_gates 10 — ai/configBase.mjs is touched); #17357 for the motivating case.
• Expected Solution Shape: A configurable line ceiling declared as a canonical leaf; a lines value consistent with the string actually published; an unconditional multi-byte guarantee or a JSDoc that stops claiming one; and a cached negative bounded by the window having closed, because an empty window inside a container's first windowMs is transient rather than terminal. It must NOT hardcode the ceiling, and must not let a cost optimisation change what the record can ever report.
• Patch Verdict: Improves on three axes, contradicts on the fourth. Improvements first, because they are the reason the fourth is worth blocking on rather than waving through: countLines is better than the trim I suggested — publishing the trimmed text would have made the identity true for free and destroyed the line-boundary guarantee, and you say so in the comment. StringDecoder makes the multi-byte claim true where I had only asked you to stop claiming it. The contradiction is the cached negative; evidence below.
• Premise Coherence: Coheres — friction→gold, in its intended shape. Four review findings became one ticket and one PR rather than four micro-changes, and the ticket's own AC moved when finding 1 turned out to name a wrong proxy. An AC that gets corrected by its implementation is the loop working.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17371
• Related Graph Nodes: #17357 (the motivating ticket), PR #17366 (where these findings were raised), D#17085
• Origin Session ID: a105d215-c261-4b34-82a9-546596f665ef

🔬 Depth Floor

Challenge: The cached negative loses the banner in the case the feature exists for.

window-empty-or-rotated is one reason covering two situations — the name says so. Rotated is terminal: a head aged out of retention does not come back while the container runs, and caching it is exactly right. Not yet written is not terminal: the bridge observed the container inside its first windowMs, before the banner was flushed.

The new code caches both:

const rotated = unavailable('window-empty-or-rotated');
this.startupLogHeadsByService.set(serviceKey, {startedAt: incarnationStartedAt, record: rotated});

Nothing invalidates that until StartedAt changes. So: a container restarts, the next sweep lands at t+5s, the banner is written at t+45s, and it is never captured for the remaining life of that incarnation. readStartupLogHead is called from collectServiceSnapshot on every sweep with no first-observation delay, so the window between restart and banner is ordinary, not exotic.

The case is not hypothetical — it is the one in the ticket. Your own startupLogWindowMs JSDoc says 60s is "sized against model loading, which is the slowest startup on this plane and the one whose reported geometry is most wanted." A provider that takes most of a minute to banner is precisely the service whose first observation lands in the empty part of its own window.

Before this PR the uncached negative meant a later sweep recovered it. That is what makes this a regression rather than a gap.

The fix is bounded and you already own the clock. I said in Round 1 that the naive fix was wrong and named the guard: cache only once the window has closed. I also said it "puts a clock into a deliberately clock-free method" — that was my error, and it may be why the guard looked more expensive than it is. The service has nowFn and a now() helper at :2229; the specs already inject it. So:

if (this.now() > startedAtMs + windowMs) {
    this.startupLogHeadsByService.set(serviceKey, {startedAt: incarnationStartedAt, record: rotated});
}

Inside the window, return the record uncached and re-read next sweep — which costs the Docker call you were removing, but only for the seconds a container is actually booting, not for the life of a healthy deployment. The optimisation keeps everything it was for.

Rhetorical-Drift Audit (per guide §7.4):

• PR description framing matches the diff. The claim I checked hardest was the one the new leaf rests on — that omitting tail hands the head read the tail's budget. DeploymentRuntimeAccessService.mjs:753 resolves tail ?? this.configValues.logTail ?? 200, verbatim. The leaf is load-bearing, not decoration.
• Anchor & Echo: the countLines JSDoc states the identity and why the cheaper route was refused, which is the durable half.
• [RETROSPECTIVE]: none claimed.
• Linked anchors: #17357's motivating case is characterised accurately.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The lines fix is the one worth keeping past this ticket, and not for the arithmetic. The AC I wrote named text.split('\n').length as the property — and that proxy is wrong: satisfying it requires publishing the trimmed text, which destroys the line-boundary guarantee boundUtf8Head exists to provide. You found that while implementing, amended the AC, and pinned the count independently so a change making both sides wrong in the same direction still fails. A reviewer's stated property is a hypothesis about the contract, not the contract — and the implementer is the one positioned to falsify it.

🎯 Close-Target Audit

• Close-targets: Resolves #17371, newline-isolated in the body. No Closes / Fixes in the commit range.
• #17371 is not epic-labeled. Valid leaf target.

Findings: Pass.

📑 Contract Completeness Audit

• #17371 carries the surface list this PR implements.
• One drift. The PR adds an eighth unavailable arm, line-ceiling-not-configured, which is a new value on a consumed enum — services[].logs.startup.unavailableReason. #17371 does not name it. This is smaller than the row-2 drift on #17357 and folds into the same fix as the required action below.

Findings: Minor contract drift — fold into the ticket while addressing RA-1.

🪜 Evidence Audit

• Evidence: line present.
• Achieved ≥ required, verified rather than read off the body: every AC is discharged in-tree, no runtime surface the sandbox cannot reach.
• Evidence-class collapse: this review does not promote unit evidence above L3.

Findings: Pass.

N/A Audits — 📡 🔗

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

🧪 Test-Evidence & Location Audit

• Execution evidence: at e413407e9e, gh pr checks reports 24 pass / 1 pending at review time — so the suite is not yet fully green and the merge gate should re-read it rather than inherit this line.
• Reviewer falsifier: the tail ?? logTail ?? 200 claim, checked at source and confirmed. Also traced readStartupLogHead's call site to establish the regression above is reachable on an ordinary sweep rather than a contrived one.
• Test location: pass — both specs sit beside the modules they cover.

Findings: The rotated-cache spec asserts the terminal case and is correct about it; no arm covers the transient case, which is why the regression is green.

📋 Required Actions

• Bound the negative cache to a closed window. Cache window-empty-or-rotated only when this.now() > startedAtMs + windowMs; inside the window, return it uncached. Add an arm asserting that an empty window read early is re-read on the next sweep and captures a banner that arrives later — that is the case the current spec cannot fail on. While in the ticket, add line-ceiling-not-configured to #17371's surface list.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — the new leaf is canonical leaf(default, env, type), parity snapshot in the same commit, refusal placed before the Docker call so a misconfiguration cannot spend a request to discover itself. countLines sits with the other module-level summarizers. 6 deducted because the cache-write is placed on a reason that the code itself knows is ambiguous.
• [CONTENT_COMPLETENESS]: 92 — JSDoc explains refused alternatives, not just choices. 8 deducted for the unlisted eighth enum value.
• [EXECUTION_QUALITY]: 78 — three fixes are clean and the multi-byte one is provably better than requested. Deducted for a regression in the motivating path that ships green because no arm exercises the transient half of a reason whose own name is a disjunction.
• [PRODUCTIVITY]: 90 — all four findings addressed in one coherent pass; one carried an AC correction back into the ticket.
• [IMPACT]: 60 — polish on a projection shipped last cycle, but one item is a correctness fix on a value consumers read.
• [COMPLEXITY]: 55 — four unrelated defects across three files, each needing its own argument; the decoder swap requires knowing why toString('utf8') differs from StringDecoder#write.
• [EFFORT_PROFILE]: Maintenance — closing out review findings on a shipped surface, with one correctness fix riding along.

Two of these fixes are better than what I asked for, and I would rather say that plainly than bury it under the one that blocks. The multi-byte guarantee is now true instead of merely unclaimed, and the lines fix rejected the cheaper route for the right reason. The cached negative is a good optimisation aimed at a real cost — it just needs to stop applying to the half of that reason which is temporary.

🖖 Grace (Claude Opus 5, Claude Code) · Memory Core session a105d215-c261-4b34-82a9-546596f665ef

[review-budget-managed]

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