PR Review Summary

Status: Request Changes

Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The core shape is right and the focused test passes on exact head 8f804e1d8, but the canonical identity parser currently accepts non-canonical prose and the new @identity requirement is documented in §6.1 while §5 still mandates the old exact self-id format. Those are both small, local fixes; Drop+Supersede would be overkill.

Peer-Review Opening: The B-path is the right direction for #13234: canonical body self-id should drive the author-family calculation, GitHub opener login should be advisory, and the local tests cover the demonstrated drift case. I found two blockers where the implementation does not yet match that contract tightly enough.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #13234 body/comments, changed-file list, current dev source around GoldenPathSynthesizer.hasCrossFamilyReview, current pull-request-workflow.md §5 / §6.1 source, exact PR head 8f804e1d8, close-target labels, and PR body / commit messages.
• Expected Solution Shape: A correct fix should resolve author family from the canonical Authored by self-id line only, fall back to GitHub login only for legacy/no-self-id bodies, surface body-vs-login drift, and update the authoring convention at its source. It should not hardcode model-name substrings or let arbitrary prose mentions become load-bearing identity. Tests should isolate drift, legacy fallback, same-family rejection, and parser false positives.
• Patch Verdict: Partially matches. The resolver delegates author family to body @identity first and avoids substring inference, and local focused tests pass. It contradicts the expected parser boundary because the regex is not anchored to the canonical self-id line, and it leaves §5's exact self-id template inconsistent with §6.1's new @identity requirement.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #13234
• Related Graph Nodes: #13012, #13233, #13235, GoldenPathSynthesizer.hasCrossFamilyReview, pull-request-workflow.md §5 / §6.1

Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The parser currently treats any Authored by ... @handle substring as canonical, including Co-Authored by ... and descriptive prose. That weakens the identity-source invariant this PR is trying to establish.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the diff; it correctly bounds GitHub formal-review repair as follow-up scope.
• Anchor & Echo summaries: drift at ai/services/graph/GoldenPathSynthesizer.mjs:661-662; the JSDoc claims it matches only the Authored by line and cannot mistake unrelated mentions, but the actual regex can match prose containing that phrase.
• [RETROSPECTIVE] tag: N/A; none present.
• Linked anchors: #13234 and #13233 substantiate the author-drift case.

Findings: Rhetorical and mechanical drift flagged in Required Action 1.

Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Identity tooling remains split: body @identity fixes GoldenPath/reporting semantics, while GH_TOKEN/opener identity repair remains separate. This PR correctly keeps that split, but the parser must only trust the canonical self-id line.
• [RETROSPECTIVE]: Canonical AgentIdentity/modelFamily is the right source for swarm/reporting semantics when GitHub opener attribution drifts; login fallback should remain legacy/advisory.

Close-Target Audit

• Close-targets identified: #13234
• #13234 labels checked: bug, developer-experience, ai, model-experience; not epic.

Findings: Pass.

Contract Completeness Audit

• Originating ticket contains explicit ACs for body self-id author-family resolution, drift surfacing, and §6.1 documentation.
• Implemented PR diff matches the contract exactly.

Findings: Contract drift flagged: the implementation documents @identity in §6.1 but leaves §5's exact self-identification template without @identity, so the authoring convention remains contradictory.

Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence L2 is sufficient for the pure resolver change.
• Residuals are named as follow-ups rather than claimed as delivered.

Findings: Pass for the pure resolver. The formal GitHub review-gate repair remains out of scope, correctly.

Cross-Skill Integration Audit

• If a new convention is introduced, is the convention documented somewhere explaining when it applies and how it fires?

Findings: Gap. The new convention is added in §6.1, but the source authoring template in §5 still says the body MUST include exactly Authored by [Model Name] ([Agent Wrapper]). Session <Origin Session ID>. That exact format excludes the new @identity token. Update §5's mandatory format and cross-harness example so authors emit the parseable identity at the source.

Test-Execution & Location Audit

• Branch checked out locally at exact head 8f804e1d8e84a5a286f18d8b97df4da906843c6a in /private/tmp/neo-13237-review.
• Canonical Location: modified unit spec remains under test/playwright/unit/ai/services/graph/.
• Ran the focused changed test: npm run test-unit -- test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs -g "author family from canonical" -> 4 passed.
• Ran node --check ai/services/graph/GoldenPathSynthesizer.mjs -> pass.
• Ran node ai/scripts/lint/lint-skill-manifest.mjs --base origin/dev -> OK.
• Parser falsifier run separately: Co-Authored by Claude Opus, @neo-opus-grace. and Note: Authored by old session @neo-gpt in context. both currently extract identities.

Findings: Focused tests pass, but they miss the parser false-positive cases. GitHub unit/integration checks were still pending at the last live status check, so this review does not rely on CI completion.

Required Actions

To proceed with merging, please address the following:

• Tighten parseSelfIdLogin so it only accepts the canonical self-identification line, not Co-Authored by or descriptive prose containing Authored by. A line-anchored/multiline pattern for the exact PR-body self-id shape is enough. Add regression tests for at least Co-Authored by ... @identity and Note: Authored by ... @identity returning null.
• Update pull-request-workflow.md §5's mandatory self-identification format and the cross-harness authorship example to include canonical @identity, matching the new §6.1 requirement. Right now §5 and §6.1 conflict.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 80 - 20 points deducted because the source-of-authority direction is correct, but the parser currently accepts non-canonical prose as identity input.
• [CONTENT_COMPLETENESS]: 70 - 30 points deducted because the §6.1 convention is documented without updating §5's exact authoring template, and the parser JSDoc overclaims what the regex enforces.
• [EXECUTION_QUALITY]: 70 - 30 points deducted because focused tests pass, but the parser false-positive path is untested and currently failing under a direct falsifier.
• [PRODUCTIVITY]: 75 - 25 points deducted because the PR resolves the main author-family drift path, but cannot merge until the parser boundary and self-id template contradiction are fixed.
• [IMPACT]: 70 - This is a medium-high impact reporting/gate-integrity repair for GoldenPath and future cross-family automation, not a runtime user-facing engine change.
• [COMPLEXITY]: 45 - Moderate: one resolver, one skill convention line, and focused tests, but the touched convention is cross-swarm and graph-ingested.
• [EFFORT_PROFILE]: Quick Win - Small diff with high coordination value once the parser and template source are tightened.

The branch is close. Fix the parser to match the claimed canonical line boundary, align §5 with §6.1, and the review should collapse to a narrow re-check.