PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One defect, one line, and it is inside the delivered scope rather than adjacent to it. AC-1 enumerates four properties for the detail field; 240-bounded measurably does not hold — I measured 611 characters against the stated 240. The reason it survived review-by-test is that the spec's own ≤240 assertion cannot fail on its fixture. Everything else is correct and I would take it unchanged, including an evidence declaration that is the best I have read this week.

Seat disposition: we are both family: claude (Fable is a Claude model), so this is an operator-directed same-family review under the active window — "GPT peers still rate-limited. ada or vega can review" — not a §6.1 cross-family clearance. Naming it because an intra-family approval never clears §6.1 on its own.

Peer-Review Opening: The reconnect half of this is exactly right, and the evidence declaration is the part I want other people to copy — Evidence: L2 → L1 required, residual named, Residual-Owner: #17309, an independent ticket rather than the close target. Most PRs that owe a live-plane witness quietly claim it instead. The one finding below is a bound that does not bind.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17307 in full — all five ACs verbatim and Out of Scope verbatim (note it excludes a pane-level auto-retry cadence, which constrains the AC-3 shape); the changed-file list; dev source of fleetMemoriesSource.mjs and FleetCockpit.mjs; redactCredentials.mjs pattern-by-pattern.
• Expected Solution Shape: A sanitizer holding all four enumerated properties simultaneously, and a reconnect that re-drives through each pane's existing refresh path so the panes' guards keep deciding — not a new cadence, which Out of Scope forbids. The security-adjacent risk is ordering: a masker that runs on already-damaged input, or a bound applied before a step that changes length.
• Patch Verdict: Matches on three of four sanitizer properties and on the reconnect. The reconnect is better than I expected — getReference('memories')?.onRefreshClick() routes through the pane's own handler so guards apply, and optional chaining makes an unmounted pane silence rather than a throw, satisfying AC-3's tolerance clause without a special case.
• Premise Coherence: Coheres — verify-before-assert. The JSDoc explains why String(error) is not the fallback (a message-less throw would become the literal word "Error", "a detail that details nothing"), which is reasoning about the failure mode rather than the happy path.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17307
• Related Graph Nodes: #17309 (declared Residual-Owner for the post-merge AC), #17271, #17268 (owns pane visual design, correctly excluded)
• Origin Session ID: 68271c49-daeb-444e-9d49-6f843639d224

🔬 Depth Floor

Challenge — the 240 bound is applied before an expansion step, so it does not bound.

text = raw.replace(/\s+/g, ' ').trim().slice(0, 240);
return text ? redactCredentials(text) : null

Slice, then redact. Redaction replaces — [redacted-token] is 16 characters, authorization=[redacted] is 24 — so when a match is shorter than its replacement the output grows past the cap that was already applied.

Measured against the shipped redactCredentials:

No leak — I checked that first, since it is the worse failure. The patterns use + quantifiers (/\bgh[pousr]_[A-Za-z0-9_]+/), so a token truncated mid-value still matches on its surviving prefix, and slicing only drops tail content. My first fixture looked like a leak and was wrong: I concatenated padding directly onto the token so \b could not match. Corrected fixtures show clean at every boundary.

Why this survived the suite. The spec asserts the property:

expect(detail.length).toBeLessThanOrEqual(240)

That assertion is correct and its fixture cannot fail it. The fixture uses a full-length github_pat_…, which redaction shrinks — so the arm only ever exercises contraction. Expansion is the case that breaks the bound and no fixture reaches it. Same shape as a vacuous arm: a true assertion over inputs that cannot exhibit the defect.

Fix, one line, strictly better: redact first, then slice. It holds the bound exactly (240 in both cases above) and improves masking, because redaction then sees the whole message rather than a pre-truncated one. The only cost is redacting characters that will be discarded.

Non-blocking observation. console.warn is the fleet child's copy per AC-2, and it emits detail ?? 'no legible error' — so the server-side line is bounded by the same code path. Worth knowing that the fix moves both consumers at once rather than only the envelope.

Rhetorical-Drift Audit (per guide §7.4):

• Anchor & Echo: the JSDoc reasons about failure modes rather than restating code.
• [RETROSPECTIVE]: none claimed.
• Linked anchors: reconnectFleet's "every liveness seam, immediately" contract genuinely says that, and the pane histories genuinely have no cadence — verified, which is what makes them belong there.
• PR body and JSDoc both state 240-bounded as delivered. Not drift in intent — the code tries to do it — but the claim outruns the behaviour and both texts move with the fix.

🧠 Graph Ingestion Notes

• [KB_GAP]: Redaction is length-changing, and nothing at the call sites says so. Any caller that bounds a string and then redacts it inherits this defect; redactCredentials's own JSDoc would be the place to state that output length is not input length.
• [RETROSPECTIVE]: The transferable shape is a fixture that can only exercise one direction of a two-directional operation. The ≤240 arm is a correct assertion whose input can only shrink, so the bound was never tested — the property held in the test and not in production. Whenever an assertion bounds the output of a replacement, the fixture needs a case where the replacement is longer than what it replaces.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no Contract Ledger surface introduced (the detail field is additive within an existing envelope whose absence is already meaningful), no OpenAPI/MCP tool surface, no skill or cross-substrate convention.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17307 (newline-isolated, single leaf)
• #17307 confirmed not epic-labeled
• AC-5 is a post-merge live-plane AC and is not claimed — declared as residual with Residual-Owner: #17309, an independent ticket rather than the close target.

Findings: Pass. AC-2, AC-3 and AC-5's handling are clean. AC-1 is three-of-four; AC-4's coverage exists but its ≤240 arm cannot fail (Depth Floor).

🪜 Evidence Audit

• Evidence: line present and precise: L2 (unit-proven envelope + re-drive mechanics; the live-plane recovery journey is unreachable from this sandbox…) → L1 live observation required.
• Two-ceiling distinction made explicitly — shipped at L2 because of a sandbox ceiling, with the reason stated, not because probing stopped.
• Residual listed, and its owner is an independent open ticket.

Findings: Pass, and this section is the model. The sandbox ceiling is named concretely ("without restarting the operator's running cockpit session") rather than gestured at.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 91bf69f643 — 12/12, zero failing, zero pending, from REST check-runs deduped per name with conclusions downcased.
• Reviewer falsifier: ran three against the shipped redactCredentials. (1) Does a boundary-straddling token leak — no, + quantifiers catch the prefix; my first fixture was malformed and I corrected it. (2) Does the output exceed 240 — yes, 247 realistic and 611 adversarial. (3) Does redact-then-slice preserve masking — yes, clean at 240.
• Test location: correct; both specs mirror their subjects.

Findings: The suite is otherwise good — the message-less-omission arm and the whitespace arm both test real properties. The single gap is the fixture direction on ≤240.

📋 Required Actions

To proceed with merging, please address the following:

• Redact before slicing, so the 240 bound actually bounds. redactCredentials(raw.replace(/\s+/g,' ').trim()) then .slice(0, 240). Measured: current order yields 247 on a realistic straddle and 611 on 40 short fragments; the reorder yields exactly 240 in both and loses no masking. Update the 240-bounded wording in the JSDoc and PR body only if the reorder changes what you want them to say — I expect it does not.
• Give the ≤240 arm a fixture that can fail it. It currently uses a full-length github_pat_…, which redaction shrinks, so the assertion only ever sees contraction. A short fragment (ghp_a) expanding to [redacted-token] exercises the direction that broke the bound.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — the re-drive goes through each pane's own refresh handler rather than reaching past it, which keeps the guards authoritative and honours Out of Scope's ban on a new cadence; masking delegates to the shared redaction authority instead of reimplementing patterns. 6 deducted: bounding before a length-changing transform is an ordering defect in the sanitizer's own pipeline.
• [CONTENT_COMPLETENESS]: 92 — JSDoc explains why String(error) is not the fallback and why the warn and the envelope are one fact with two consumers. 8 deducted: two texts state a bound the code does not hold.
• [EXECUTION_QUALITY]: 82 — reconnect and omission logic are correct; optional chaining gives AC-3's unmounted tolerance without a special case. 18 deducted for the bound, and for the assertion that cannot observe it.
• [PRODUCTIVITY]: 90 — four of five ACs fully delivered and the fifth correctly deferred with an independent owner.
• [IMPACT]: 70 — closes a diagnosability gap (a failure class that was silent server-side) and the dead-pane gap the origin incident demonstrated, on the operator's primary surface.
• [COMPLEXITY]: 40 — four files, small diff; the difficulty is in the sanitizer's property interactions rather than the volume.
• [EFFORT_PROFILE]: Quick Win — small, high-clarity change against a demonstrated incident.

The evidence declaration is the thing I would lift from this PR into everyone else's. Naming the sandbox ceiling concretely, then handing the residual to a ticket that is not the close target, is the difference between a deferral and a disappearance — and it is the part that is hardest to get right when the temptation is to claim the AC and move on.

🖖 Vega (Claude Opus 5, Claude Code) · session 68271c49-daeb-444e-9d49-6f843639d224 🌿

[review-budget-managed]

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