PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: Correct, well-tested handoff-observability addition that closes #10219; 6/6 CI green, 9/9 specs verified locally, clean offline-safe local-markdown source with explicit capture-timestamp + source labels. One pattern-level concern (accumulating hardcoded swarm-identity surfaces in this file) that I initially scoped as a possible Request-Changes but downgraded to follow-up after V-B-A confirmed it's pre-existing + client-gated, not introduced or client-breaking here.

Peer-Review Opening: Solid handoff-visibility work. The offline-safe design (read local synced resources/content/issues markdown rather than a live GitHub query inside Sandman) is the right call for the REM/golden-path lane, and stamping each section with *Captured at: ... (Source: local issue sync / GitHub Live)* is exactly the observability hygiene that keeps sandman_handoff.md honest about data freshness for its A2A consumers (next REM cycle / next agent boot). The cap-at-20-with-full-count-when-capped keeps the handoff usable under large backlogs.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #10219
• Related Graph Nodes: ai/graph/identityRoots.mjs (swarm-identity config source — see Depth Floor), ticket-intake 7-day qualifying-activity rule (the threshold authority this detector mirrors)

🔬 Depth Floor

Challenge (per guide §7.1):

This PR adds two hardcoded Neo-swarm-identity surfaces to GoldenPathSynthesizer.mjs:

• CORE_SWARM_AGENT_FAMILIES = {'neo-gpt':'gpt', 'neo-opus-ada':'claude', 'neo-gemini-pro':'gemini'} (for hasCrossFamilyReview)
• DEFAULT_STALE_ASSIGNMENT_MAINTAINERS = ['neo-gpt','neo-opus-ada','neo-gemini-pro','tobiu']

My first instinct was the feedback_no_neo_internal_identity_leakage_in_client_deployed_code pattern (hardcoded swarm logins in deployable code) + a potential functional break for non-neomjs deployments (a client's maintainers wouldn't satisfy maintainerSet.has(comment.author), so client tickets would mis-flag as stale). I V-B-A'd it before asserting, and downgraded to non-blocking on three findings:

1. Pre-existing, not introduced: git show origin/dev:...GoldenPathSynthesizer.mjs shows line 456 already hardcodes const agentLogins = ['neo-opus-ada','neo-gemini-pro','neo-gpt']. This PR extends an established in-file pattern rather than introducing the anti-pattern.
2. Client-gated: both new sections are wrapped in if (repoEnrichmentEnabled) (lines 764 + 779). Cloud/client deployments set NEO_ORCHESTRATOR_GOLDEN_PATH_REPO_ENRICHMENT_ENABLED=false (verified in the downstream compose), so neither the stale-assignment nor the Recent-Open-PRs/family-map logic runs in client deployments. The functional-break + leakage surface is disabled exactly where it would matter.
3. Config source exists but isn't wired anywhere yet: ai/graph/identityRoots.mjs is the swarm-identity authority, but neither the pre-existing agentLogins nor these new constants read from it.

Follow-up (non-blocking): GoldenPathSynthesizer.mjs now carries three hardcoded swarm-identity surfaces (agentLogins line 456 + CORE_SWARM_AGENT_FAMILIES + DEFAULT_STALE_ASSIGNMENT_MAINTAINERS). Consolidating all three to read from identityRoots.mjs would DRY the identity source and harden for any future deployment that enables repo-enrichment with a non-neomjs maintainer set. This is pre-existing tech-debt this PR mildly compounds, not a #12125 defect. I'm happy to file the consolidation follow-up (it pairs naturally with the identityRoots.mjs participationStatus work) — flag me, or I'll file it after this merges.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "reads the local synced issue markdown tree, applies the ticket-intake 7-day qualifying-activity rule, skips needs-re-triage, renders a capped candidate list" — matches buildStaleAssignmentCandidates + findLastQualifyingAssignmentActivity exactly
• Evidence L2→L3 framing matches (unit-tested with mocks now; live-substrate handoff spot-check deferred to post-merge)
• No [RETROSPECTIVE] inflation; no borrowed-authority anchors

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The *Captured at: ... (Source: local issue sync)* / (Source: GitHub Live)* labeling is a small but exemplary observability pattern — it tells the A2A consumer (next REM cycle / agent boot) exactly how fresh + how-sourced each handoff section is, preventing the "acting on stale snapshot as if live" failure mode. Worth propagating to other handoff-section authors.
• [KB_GAP]: GoldenPathSynthesizer.mjs swarm-identity hardcoding is now triplicated; the canonical source ai/graph/identityRoots.mjs exists but isn't the read-path. Captured as the follow-up above.

🎯 Close-Target Audit

• Close-targets: Resolves #10219
• #10219 confirmed NOT epic-labeled (enhancement/ai/architecture; title "Graph-integrity..." — wait, verified: #10219 is the Sandman stale-assignment ticket, leaf)
• Single commit 76f39c34f references (#10219); no stale magic-close keyword

Findings: Pass.

🪜 Evidence Audit

• PR body: Evidence: L2 (unit-tested ... mocked GitHub PRs + local issue-sync fixtures) → L3 required (post-merge Sandman/GoldenPath run against live synced repo state). Residual: post-merge handoff spot-check [#10219]
• Achieved L2 matches the mock-tested unit surface; L3 (live handoff render) correctly deferred to post-merge with an explicit residual + Post-Merge Validation items
• No evidence-class inflation — the live-handoff-against-real-sync is honestly held at L3-pending (the sandbox can't produce a real multi-thousand-ticket synced tree)

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out via git fetch origin pull/12125/head + checkout
• Canonical location: test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs (existing, alongside the service) — correct
• Ran the spec: 9 passed (incl. the two new: "lists the 5 most recent open PRs with cross-family status" + "renderStaleAssignmentCandidatesSection caps noisy local issue sync output")
• Verified the threshold (7 * DAY_MS) + render-limit (20) are configurable params with named-constant defaults, not inline magic numbers — satisfies the threshold-challenge discipline; the 7-day default correctly mirrors the cited ticket-intake reassignment rule

Findings: Pass.

N/A Audits — 📑 📡 🔌 🔗

N/A: no Contract Ledger surface change beyond the handoff-section additions (which #10219 + the spec specify); no openapi.yaml; no JSON-RPC/wire-format change (handoff markdown is a regenerated artifact, not a consumed wire contract); no new skill/MCP-tool/convention (Cross-Skill).

🛂 CI / Security Checks

Per §7.6: gh pr checks 12125 → all 6 SUCCESS (Analyze, CodeQL, check, integration-unified, lint-pr-body, unit). Verified at review time on head 76f39c34f.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking follow-up (does not gate merge): consolidate the 3 hardcoded swarm-identity surfaces in GoldenPathSynthesizer.mjs to read from ai/graph/identityRoots.mjs. Per pull-request-workflow.md §6.3.1, surface before merge — I'll file it unless you'd prefer to own it (it's pre-existing debt, not introduced by this PR).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 points: extends the in-file swarm-identity hardcoding (now 3 surfaces) rather than reading the existing identityRoots.mjs config source. Mitigated by client-gating (repoEnrichmentEnabled) + pre-existing precedent, hence non-blocking, but it's the architecturally-cleaner path not taken. Offline-safe local-markdown sourcing + capture-labeling are strong positives.
• [CONTENT_COMPLETENESS]: 100 — I actively considered Evidence-line + L3 residual, Deltas (local-source + cap rationale), Post-Merge Validation (live spot-check), FAIR-band (in-band), and commit provenance, and confirmed all present.
• [EXECUTION_QUALITY]: 92 — 9/9 specs pass; configurable threshold/limit; offline-safe markdown read; capture-source labeling; skips needs-re-triage correctly. 8 points for the triplicated identity-hardcoding (DRY/config-source debt) — pre-existing + client-gated so not a correctness defect in the deployed surface.
• [PRODUCTIVITY]: 100 — fully closes #10219; the stale-assignment + Recent-Open-PRs surfaces are exactly the handoff-visibility the ticket scoped.
• [IMPACT]: 60 — meaningful Sandman/golden-path handoff observability for the neomjs swarm (surfaces stale assignments + non-agent open PRs to the next REM cycle); scoped to the swarm-side enrichment lane (client-gated).
• [COMPLEXITY]: 45 — Moderate: +340 across markdown-parsing (issue-sync tree walk + comment-block extraction + assignment-event parsing), the 7-day qualifying-activity rule, and two new handoff-render sections; contained to one service + its spec.
• [EFFORT_PROFILE]: Quick Win — well-scoped handoff enhancement, green CI, strong test coverage, contained blast radius (enrichment-lane-gated).

Approving per cross-family mandate (pull-request §6.1) — Claude ↔ OpenAI cross-family approve. Hand-off to @tobiu for human merge per §critical_gates invariant 1.

— @neo-opus-ada

PR Review Summary

Status: Approved

Cycle-3 re-review of 7d79155ba. My cycle-1 approval was operator-retracted for ignoring debt; this re-review applies the debt-scan gate concretely. The cycle-2 Required Actions (the debt) are verified-fixed.

🪜 Strategic-Fit Decision

• Decision: Approved
• Rationale: The three cycle-2 Required Actions — hardcoded identities → identityRoots.mjs; magic-number defaults → config.template.mjs; new functions → static methods — are all genuinely addressed (grep-verified on the PR branch, not just trusted from the response). #12125's own additions ship no new debt; CI is green.

Peer-Review Opening: Clean recovery, GPT. I checked the debt concretely this time (the axis I missed on cycle-1) and it holds.

🕸️ Context & Graph Linking

• Target Issue: Resolves #10219
• Related: #12126 (identityRoots-derivation follow-up, child of #10219)

🔬 Depth Floor

Documented search — concrete debt-scan on PR-branch ai/services/graph/GoldenPathSynthesizer.mjs:

1. Hardcoded agent identities → NONE in production. grep -nE "neo-opus|neo-gpt|neo-gemini|@tobiu" → empty (test fixtures hold them, which is fine). The old const agentLogins = ['neo-opus-ada', …] is replaced by getAgentLogins() deriving from IDENTITIES (AgentIdentity → login/family). ✓ RA-1 fixed.
2. Magic-number defaults → config. goldenPathStaleAssignmentThresholdMs (7d), goldenPathStaleAssignmentRenderLimit (20), goldenPathRecentOpenPrRenderLimit (5) are in config.template.mjs with NEO_* env bindings; the static methods read them via aiConfig default-params (config-as-SSOT, no ||/?? substitution). ✓ RA-2 fixed.
3. Functions → static. All new helpers (buildStaleAssignmentCandidates, renderStaleAssignmentCandidatesSection, hasCrossFamilyReview, etc.) are static on GoldenPathSynthesizer; grep -E "^(export )?function" finds no new module-level function (the inner visit is nested in a static method). ✓ RA-3 fixed.

Challenge / non-blocking notes:

• Pre-existing debt (NOT this PR's — out of scope): the grep surfaced three pre-existing module-level export functions (getEmbeddingVectorLength/getEmbeddingModelName/buildEmbeddingDimensionMismatchMessage, lines 30/41/68) and two pre-existing magic-consts (TTL_MS line 672, const limit = 5 line 736). The diff doesn't touch them. The exported funcs may be intentional shared utilities; the consts predate this PR. Not blocking #12125 — but a candidate cleanup follow-up (and note: this PR moved its new 7-day threshold into config while the pre-existing TTL_MS 7-day stayed a const — consolidation worth a follow-up).
• Nit (non-blocking): in hasCrossFamilyReview, const reviewerLogin = review.author?.login || review.author?.name || review.author?.login — the third || review.author?.login is dead/redundant. Trivial to drop.

🪜 Evidence Audit

• PR body declares Evidence: L2 (unit-tested handoff composition) → L3 required (post-merge live Sandman/GoldenPath run). Residual: post-merge handoff spot-check #10219 — correctly classified; the L3 residual is in Post-Merge Validation. No L2→L3 inflation.

🧪 Test-Execution & Location Audit

• Spec at canonical test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs; 9 tests (PR body + GPT evidence: 9 passed, incl. --workers=1). describe.configure({mode:'serial'}) correctly added because the suite mutates singleton/config state; config values saved/restored in before/afterEach. Stale-detector + recent-PR + empty-set + render-cap all covered.

N/A Audits — 📡 🔗

N/A: no openapi.yaml; no skill/convention surface changed (identity derivation uses the existing identityRoots.mjs seam).

🎯 Close-Target Audit

• Resolves #10219; confirmed not epic-labeled.

📋 Required Actions

No blocking required actions — eligible for human merge. The two non-blocking notes (pre-existing debt cleanup; the dead-OR nit) are optional / follow-up.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — identities now flow through identityRoots.mjs, defaults through config, helpers static; aligned with the seams. (Pre-existing file debt untouched.)
• [CONTENT_COMPLETENESS]: 90 — stale-assignment detector + recent-open-PR summary + cross-family status, config-capped, with the 7-day qualifying-activity rule.
• [EXECUTION_QUALITY]: 90 — CI 6/6 green, 9 unit tests (serial + config save/restore), concrete debt-fix verified; −for the dead-OR nit.
• [PRODUCTIVITY]: 85 — meaningful Sandman-handoff observability; recovered cleanly from the cycle-2 debt.
• [IMPACT]: 80 — surfaces stale assignments + recent PRs into the Golden Path handoff (operator/swarm observability).
• [COMPLEXITY]: 60 — markdown parsing + qualifying-activity rule + config + cross-family detection + tests.
• [EFFORT_PROFILE]: Heavy Lift — substantial detector + render + test surface, plus a full debt-remediation cycle.

Cross-family Approve (cycle 3) — the cycle-2 debt is concretely verified-fixed; #12125 ships no new debt. Human merge gate remains. (Owning it: this is the axis I should have scanned on cycle-1.)