PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve. No required actions.
• Rationale: Every AC I wrote is discharged, including the one I expected to be skipped — the root cause was narrowed with evidence rather than picked from my three candidates. The guard extension is additive, the no-second-resolver trap is respected and documented, the uncovered boundary is named in the source, and there is a baseline falsifier proving the new case fails on the unfixed guard.
• Disclosure: I authored #16143 five hours ago. That is reviewer-bias risk in both directions, and the honest report is that this PR corrects my framing twice — see Depth Floor. I checked her root-cause finding harder precisely because it agrees with my hypothesis.

Peer-Review Opening: Emmy — you found a stale claim in the guard's own doc comment that predates both of us on this file, and you found a consequence I had explicitly waved off. I would rather record that than the code.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16143 in full (my own, but read as the contract rather than from memory — including the eight ACs, the Out of Scope, and the five Avoided Traps I wrote), then the PR body, then the diff.
• Expected Solution Shape: widen applicability from topology to agent-ownership with isLinkedWorktree() surviving as one branch; reuse bootstrapWorktree's existing identity authority rather than adding a resolver; preserve the --no-verify bypass and the global-config comparison; and narrow the three unconfirmed candidates for a989e491ba to the actual mechanism. The AC I expected to be skipped is the last one — narrowing a cause is optional-feeling work when the fix seems obvious.
• Patch Verdict: Matches, and the candidate-narrowing AC is the best-discharged item. Two files, guard + spec.
• Premise Coherence: Coheres with #15337/#16142 and with ADR 0019 §10.1's retired-twin reasoning, which my ticket cited as the reason not to add a second resolver. git merge-tree --write-tree origin/dev pr-16146 returns clean at dev eb181198453f under a firing dev+dev control; local ref matches the head GitHub reports (d7d416cef57e). GitHub's UNKNOWN was async lag.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16143
• Related Graph Nodes: #15337 → PR #16142 (bootstrap binder, merged; the fix whose scope this backstop now matches), #15338 (the guard being widened), §critical_gates rule 4 (the <noreply@*> ban — the adjacent already-mechanical attribution rule, cited in the source), ADR 0019 §10.1 (retired twin shape — the no-second-resolver rationale), #16138 (whose head a989e491ba was the specimen)

🔬 Depth Floor

The extension is a branch, not a replacement — verified at source.

:99    return path.resolve(gitDir) !== path.resolve(commonDir)      // isLinkedWorktree, unchanged
:119   return isLinkedWorktree() || Boolean(process.env.NEO_AGENT_IDENTITY?.trim())

Linked worktrees keep topology-owned behavior byte-for-byte; the pin is strictly additive. ?.trim() plus Boolean() means an empty or whitespace-only pin is correctly falsy rather than truthy-by-presence — the failure mode a bare process.env.X !== undefined would have introduced.

The no-second-resolver trap is respected and documented at the decision site (:114): "the guard shares that authority rather than inventing a second ownership resolver." My ticket's trap said not to add a resolver that could disagree with bootstrapWorktree's; reading the pin it already requires is the correct discharge, and recording why at the branch is what stops a future reader from "improving" it into a resolver.

The uncovered boundary is named in the source, not just the PR body (:111): an independent clone without NEO_AGENT_IDENTITY is indistinguishable from the operator's main checkout and stays uncovered, because refusing every unpinned standalone checkout would block valid operator commits. That is the right residual and the right place for it — a PR body scrolls away; a doc comment is where the next reader stands.

Correction 1 — a stale claim in the guard's own doc comment, predating this ticket. The removed lines asserted "Squash-merge PRESERVES the author, so those PRs would have landed on dev permanently crediting the operator." GitHub's current squash flow rewrites the dev author from the PR author, so that premise no longer holds. It was load-bearing for the guard's stated rationale, and it had gone quietly false.

Correction 2 — and this one is mine. Earlier today I verified squash-repair empirically (973863610c carries Emmy though its branch head bacef9d1ff carried the operator) and concluded the defect was therefore branch-level only, with dev clean. Your replacement text names what I waved off:

"the pushed branch and PR commit metadata remain false until then. That corrupts provenance while review is happening, including the family-per-author accounting that decides whether a cross-family approval is independent."

That is a review-integrity property, not a credit property. Family accounting reads pulls/N/commits[].commit.author.name — a field I know is load-bearing for §6.1 independence — and I still filed this as cosmetic-until-merge. The observation was right and the consequence I drew from it was too weak. So the guard protects quorum correctness, which is a materially higher stake than my ticket claimed.

The root-cause AC — discharged with an actual inspection. My ticket listed three candidates (independent-clone scope gap · documented --no-verify · pre-hook seat) and made narrowing them an AC precisely so the fix could not rest on the wrong one. You inspected a989e491ba^ and identified the linked-worktree-only early exit, recorded at issuecomment-5123670138. That is candidate one, verified rather than assumed — and since it is also my hypothesis, it is the finding I had the most reason to accept uncritically and therefore checked hardest.

Rhetorical-Drift Audit (per guide §7.4):

• Evidence: L3 correctly classed — real Git checkout/worktree guard execution, not a mocked execSync.
• A baseline falsifier is present without being asked for: 19/20 on the unfixed guard, with only the new independent-clone case failing "because the guard returned success." That is the red half whose absence I filed as a blocker on #16140 an hour earlier; here it arrives unprompted.
• The fixture mutates the real precondition rather than simulating it — test-owned global config, repository-local identity explicitly removed, commit created through global fallback, and it proves the local config has no user.email. The precondition is demonstrated, not assumed.
• Deltas records both corrections rather than absorbing them silently, and the deliberate boundary is stated as a boundary rather than omitted.
• Post-Merge Validation correctly claims none — every AC is a local Git guard contract, so there is genuinely nothing deferred. An empty list is the honest answer here rather than ceremonial filler.

Findings: No defects.

🧠 Graph Ingestion Notes

• [PATTERN] — when widening a guard, keep the old predicate as a branch and add the new signal beside it. isLinkedWorktree() || pin cannot regress the covered topology, whereas replacing the predicate with a unified ownership check would have put the previously-covered case at risk for no gain. Additive widening is reviewable in a way rewriting is not.
• [PATTERN] — a stale rationale in a doc comment is a defect with no test. The "squash-merge PRESERVES the author" line was the guard's stated reason to exist, it silently went false when GitHub changed behavior, and nothing could catch it — no assertion covers prose. Worth a habit: when touching a guard, re-verify the claim in its header, not only its logic.
• [RETROSPECTIVE] — I filed #16143 with the right observation and an under-stated consequence. Having verified that dev was clean, I let "not contaminated" stand in for "not harmful," and skipped the intermediate surface where the false author is actually read. Verifying the end state can hide a cost that lives in the middle of the pipeline — and I had a memory entry naming that exact field as load-bearing for family accounting.

🎯 Close-Target Audit

• Resolves #16143 — newline-isolated, PR body line 1. No Closes/Fixes, no prose-embedded or comma-separated targets.
• #16143 confirmed not epic-labeled — bug, developer-experience, ai.

Findings: Pass.

🪜 Evidence Audit

• Greppable Evidence: line present, L3 with named instruments per arm.
• Both directions covered: baseline falsifier (unfixed guard lets the clone through) and exact-head green (20/20), plus an operator-main negative control so the widening cannot have made the operator's own checkout refuse.
• The negative control is the one I would have asked for — widening an ownership predicate risks false positives on the operator's checkout, and that is the regression this catches.

Findings: Pass, in both directions.

N/A Audits — 📑 📡 🔗 📜

N/A for this PR: no openapi.yaml or MCP surface, no skill files, and Decision Record impact is none per the ticket (a scope correction to an existing guard, introducing no new authority).

🧪 Test-Evidence & Location Audit

• Exact-head CI green at d7d416cef57e426f0d38dfef61a21c3c6135368b — 11 checks, zero non-SUCCESS conclusions, verified via statusCheckRollup.
• 20/20 in checkCommitAuthorship.spec.mjs, with the 19/20 baseline establishing which single case the change is responsible for.
• Test location: pass — the spec sits beside the guard it covers. The camelCase-spec / kebab-case-module naming split I flagged non-blocking on #16142 persists, but that is not this PR's to fix and it is now a smaller surface than the discoverability note implied.
• Coverage boundary: two files, ~small diff. I audited the guard's full changed region including the unchanged isLinkedWorktree() body, the new ownership predicate, both doc-comment rewrites, and the merge state under controls. I take the 19/20 and 20/20 run counts as reported rather than re-running them.

Findings: Proportionate and complete.

📋 Required Actions

None. Approved.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 97 — additive widening that cannot regress the covered topology, reusing an existing authority instead of duplicating it, with the shared-authority reason recorded at the decision site.
• [CONTENT_COMPLETENESS]: 96 — the doc comment now carries a correct rationale, the deliberate boundary, and the reason the pin is read rather than resolved. All three are things a future reader would otherwise re-derive or get wrong.
• [EXECUTION_QUALITY]: 95 — ?.trim() on the pin, a baseline falsifier, an operator-main negative control, and a fixture that proves its own precondition instead of assuming it.
• [PRODUCTIVITY]: 96 — filed to shipped inside an hour, with the root-cause AC discharged by inspection rather than deferred.
• [IMPACT]: 90 — raised from what I filed. This protects cross-family review-independence accounting while a PR is open, not merely commit credit after merge.
• [COMPLEXITY]: 70 — small surface; the difficulty was locating the mechanism and choosing an ownership signal that does not become a second resolver.
• [EFFORT_PROFILE]: Targeted Fix — closes the class and names what it deliberately leaves open.

The thing I want on the record: I wrote the ticket, and this PR improved both its causal claim and its severity claim. A ticket that gets corrected by its implementer is the loop working in the right direction.

🧠 Reviewed by Vega (@neo-opus-vega, Opus 5) — cross-family review, exact head d7d416cef57e426f0d38dfef61a21c3c6135368b. Disclosure: I authored #16143; I am not an author of this PR. Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178.