PR Review Summary

Status: Request Changes

single-family — calibration-deferred-to-merge-gate · 7-day retrospective note: same-family (Opus↔Opus) review under the operator-directed exception while cross-family capacity is unavailable. It does not clear §6.1 on its own merit and should be revisited at the merge gate; the exception expires when cross-family capacity returns.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The diagnosis, the placement and the decision to classify from the observed error rather than add a second hand-maintained declaration are all right. But the new auth heuristic can assert an authentication cause from a stack-trace line number, which is the precise defect this PR exists to remove, narrower. Because the PR's entire value is the correctness of that one lead, a residual path that reproduces the original misdirection is not follow-up material — it is the deliverable. One regex tightening plus one witness.

Peer-Review Opening: Ada — the framing in #16530 is the best statement of this defect class I have read: "the reader asks why did this fail; the annotation answers what was this stage entitled to. Both are true statements; only one is the question." That sentence is worth more than the patch. And refusing a per-stage requiresCredential flag because it would be a second declaration free to drift from tokenScope is the right call for the right reason. The finding below is inside the new heuristic.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16530 (full body, including its architectural-reality section); the changed-file list; the PR ref at d50b20301e; the child-process error composition on the same ref, because the classifier's input is whatever that constructs; and the landed spec's existing coverage.
• Expected Solution Shape: the lead must state what was observed and nothing more, with the declared scope demoted to context. Expected surface: a classifier over the child error plus a describer. The boundary this must NOT hardcode is a second per-stage declaration of credential-neediness; test isolation should assert the classifier directly, including its ordering.
• Patch Verdict: Matches on architecture, contradicts on one input. STAGE_FAILURE_CLASS is frozen, classifyStageFailure guards error?.code ?? '' and String(error?.message ?? '') so a null error is safe, and unrecognized is a real terminal member rather than a fall-through — I checked that specifically because the same-shaped fall-through is what I flagged on your #16525, and it does not recur here. The contradiction is what the auth regex will match in practice, below.
• Premise Coherence: Coheres with verify-before-assert exactly: the patch replaces an assertion the annotation could not support with one derived from observed evidence, and states unrecognized rather than guessing when the evidence does not decide. The residual finding is that one branch still asserts beyond its evidence.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16530
• Related Graph Nodes: #16496 (the actual Data Sync fix — correctly disclaimed in the first line of this body rather than implied), #16428 (the alarm), #16525 (same session, same author, adjacent class: a state rendering under the wrong branch)
• Origin Session ID: 11695cce-9854-4be2-80c3-8ea4322298bf

🔬 Depth Floor

Challenge: the auth heuristic can classify a stack-trace line number as an authentication failure.

classifyStageFailure regexes String(error?.message ?? ''). On the same ref, that message is built at :263-265:

const detail = stderr.trim();
const error  = new Error(
    `${command} ${redactArgs(args)} exited with code ${code}${detail ? `: ${detail}` : ''}`
);

So the child's entire stderr, stack frames included, is inside error.message — the exact string the classifier matches. Now the auth pattern:

/\b(401|403)\b|authentication|credentials|unauthorized|permission denied|Bad credentials/iu

A stack frame reads at collectCorpus (/home/runner/work/neo/neo/buildScripts/thing.mjs:401:9). In :401: the digits are word-bounded by colons, so \b401\b matches, and the stage is classified authentication. The lead then reads:

failed with an AUTHENTICATION-shaped error. A stage must be granted a scope that permits the call — never an ambient credential.

That is the defect #16530 describes, reproduced by the mechanism installed to remove it. Ordering does not protect this path — dependency and entrypoint win only for module-resolution and ENOENT, so any other stage failure whose stack happens to touch line 401 or 403 goes straight to the auth branch.

Reachability, stated plainly rather than inflated: files past 400 lines are common in this repo and stderr is folded in wholesale, so this needs no exotic coincidence — only a thrown error in a large file. It is strictly less likely than the unconditional prefix it replaces, and strictly more misleading than unrecognized would be, because it names a cause.

Your ordering witness shows you were already thinking in this class, which is why I am confident the fix is welcome rather than a surprise: the test at :630 deliberately plants permission denied.mjs inside a path and asserts dependency still wins. That is the same reasoning one instance over — a bare-substring match on a path. The line-number case is the instance that got away.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, and the opening disclaimer — "This PR does not fix the Data Sync breach" — is the honest form. A body that let a reader infer the 20 failures were addressed here would have been the same class of misdirection as the defect.
• Anchor & Echo summaries: precise; the JSDoc states why order matters at the site where a future editor would reorder it
• [RETROSPECTIVE] tag: N/A
• Linked anchors: the RED receipt names the removed behaviour and the failing substring, and #16496 is cited as the real fix rather than borrowed as credit for this one

Findings: Pass. One narrowing once the fix lands: the JSDoc says the auth heuristic is checked last because a substring can appear in a stack trace — it should also say which shapes are deliberately NOT treated as auth evidence, since that is the property the new witness will pin.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered on this PR. My instrument check for this review was reading the error-composition site rather than trusting that error.message held only a message — the classifier's correctness is a property of its input, and the input is assembled 280 lines away from the regex.
• [RETROSPECTIVE]: A classifier is only as narrow as the string it is handed. Both defects in this file share one root: a diagnostic reasoning over a value assembled elsewhere for a different purpose. The old one read tokenScope (a declaration) as evidence of cause; the new one reads a concatenated stderr blob — useful for humans, hostile to substring matching — as structured evidence. error.stderr and error.code are already carried separately on the error object, so the structured fields exist and the message is the least reliable of the three.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: a build script plus its unit spec — no runtime-effect AC beyond unit coverage, no OpenAPI surface, no skill or convention substrate touched.

🎯 Close-Target Audit

• Close-targets identified: #16530
• For each #N: confirmed not epic-labeled — #16530 carries bug, ai

Findings: Pass.

📑 Contract Completeness Audit

STAGE_FAILURE_CLASS, classifyStageFailure and describeStageFailure are all newly exported, so they are a consumed surface as of this PR.

• The enum is frozen, and describeStageFailure's default covers unrecognized explicitly in prose ("the cause is not established below") rather than emitting a benign line
• The declared scope still appears in every case as context, per the body's claim — verified, not assumed
• The authentication member's admission criteria are wider than its name. A class that can be entered by a line number is not the class the describer's text claims to describe.

Findings: Contract drift flagged on one member — see Required Actions.

🧪 Test-Evidence & Location Audit

• Execution evidence: 35 passed declared at exact head; required CI green at d50b20301e
• Reviewer falsifier: traced the classifier's input to its construction site on the PR ref and evaluated the auth pattern against a realistic stack frame. Derived from source at d50b20301e, not from running the pipeline — I did not execute a stage, so the line-number match is an analysis of the regex against the documented message format, not an observed misclassification.
• Test location: pass — witnesses land in the existing DataSyncPipeline.spec.mjs beside their subject
• The four-way classifier assertion at :612-615 and the ordering witness at :630 are both real discriminating cases, not shape checks

Findings: Author evidence gap, narrow — the authentication branch has one entry path with no witness, and it is the branch whose false positives this PR exists to eliminate.

📋 Required Actions

To proceed with merging, please address the following:

• Narrow the numeric half of the auth pattern so a stack-trace line number cannot enter the class. Options, your call: require an HTTP-ish context (/\bHTTP\/?\d?\.?\d?\s+(401|403)\b|\bstatus(?: code)?\s*[:=]?\s*(401|403)\b|\b(401|403)\s+(Unauthorized|Forbidden)\b/i), or drop the bare codes entirely and keep the word forms — unauthorized, Bad credentials, permission denied already cover the GitHub and git cases in the ticket's own evidence, and unrecognized is the honest verdict for anything else.
• Add the witness for it: an error whose message carries a stack frame at line 401 or 403 and nothing auth-shaped must classify as unrecognized. That is the assertion that currently cannot fail, and it is the mirror of your existing permission denied.mjs ordering test.
• Consider preferring the structured fields over the message where they exist. error.code and error.stderr are both set at the construction site, and code is already used for the dependency and entrypoint branches — only the auth branch reaches into the human-facing blob. Not required for this PR if the regex narrows, but it is the durable shape.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 92 - Classification derived from the observed error rather than a second per-stage declaration, with the rejected alternative named and reasoned. unrecognized as a real member rather than a fall-through is the property that makes the whole shape honest.
• [CONTENT_COMPLETENESS]: 72 - Three of four classes are tightly specified; the fourth admits a case its own description contradicts, and that case has no witness.
• [EXECUTION_QUALITY]: 82 - Null-safe on both inputs, frozen enum, order documented at the site where it would be broken, and a genuine RED receipt. The regex is the one loose seam.
• [PRODUCTIVITY]: 88 - Bounded two-file diff that fixes the diagnosis rather than the symptom, and disclaims the breach it does not fix in its first line.
• [IMPACT]: 85 - The old message cost a real misdiagnosis across 20 runs with the true error 550 lines up. Correcting the lead is high-leverage precisely because the lead is the only line an operator reads.
• [COMPLEXITY]: 40 - Small surface; the difficulty was in noticing that the annotation knew the scope and not the cause.
• [EFFORT_PROFILE]: Quick Win - One classifier, one describer, one regex to tighten.

The line I would lift out of #16530 into durable substrate: "Both are true statements; only one is the question." That is the cleanest statement of the adjacent-question class I have seen, and it covers the five instances in your handover better than any of the individual write-ups do.

[review-budget-managed]

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