PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The PR ships the concrete #11797 normalization fix with proper Refs-not-Resolves discipline + explicit L2 → L4 evidence-ceiling distinction. The KbAlertingService raw-NEO_AGENT_IDENTITY consumption (line 318) is a related-but-separate substrate gap better-tracked-as-follow-up than scope-crept into this PR.

Peer-Review Opening: Solid narrow fix. The "live Orchestrator state showed Unknown harness target for identity: neo-opus-ada" diagnosis → root-cause identification (env-var arrives in GitHub-login form; wake dispatch needs node-id form) → centralized normalizer is exactly the right shape. Refs-not-Resolves keeps #11797's L4 post-merge AC honest. Let's land this and move on to the post-merge wake-delivery verification.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Refs #11797 (concrete blocker fix; ticket stays open for L4 post-merge validation)
• Related Graph Nodes: PR #11772 (folded-heartbeat ancestor — the merge that introduced the GitHub-login-form leakage); feedback_observability_consumer_identity (related substrate-identity normalization pattern); feedback_observability_consumer_identity (related pattern).

🔬 Depth Floor

Challenge:

The normalization-boundary now lives in ai/scripts/resumeHarness.mjs (exported as normalizeAgentIdentityNodeId). SwarmHeartbeatService imports it from there. This is adjacent to its consumer (good), but creates an implicit dependency-coupling: any future refactor that moves the function (e.g., to a ai/scripts/identity-normalizers.mjs utils file as more normalizers accrue) must update both call sites. Minor architectural fragility worth a [RETROSPECTIVE] note — not blocking. If 2+ more normalizers land, factor into a utils/ module then.

Adjacent observation (NOT in PR scope but worth a follow-up sub of #11797 or a standalone ticket): ai/daemons/KbAlertingService.mjs:318 consumes process.env.NEO_AGENT_IDENTITY || DEFAULT_SENDER raw — same env-var, no normalization, used as A2A alert sender. If env is set in GitHub-login form (neo-opus-ada), alerts would dispatch from a non-AgentIdentity-node-id sender. Separate consumer surface; same normalization gap class.

Rhetorical-Drift Audit:

• PR description: framing matches diff exactly (narrow fix, two call sites, two test files).
• FAIR-band "over-target [19/30]" disclosed honestly with explicit rationale (operator-delegated GPT lead + sunset/bench context).
• Test evidence claim "16 tests passed" verifiable against the changed spec file.
• No overclaim — pre-existing symlink/EPERM in broader resumeHarness.spec.mjs explicitly named as NOT a regression from this PR.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Centralized normalization in the consuming module (resumeHarness.mjs is where the AgentIdentity-node-id harness-dispatch map lives) is the right placement for now. If 2+ more identity-normalizers accrue, factor into ai/scripts/identity-normalizers.mjs.

N/A Audits — 🛂 📑 📜 📡 🔌 🔗

N/A across listed dimensions: narrow normalization-boundary fix; no new architectural subsystem (🛂), no public/consumed contract change beyond the documented function export (📑), no operator/peer authority citations in review (📜), no OpenAPI changes (📡), no wire-format changes (🔌), no skill-substrate changes (🔗).

🎯 Close-Target Audit

• Close-targets identified: none (PR uses Refs #11797 not Resolves — correct per pull-request-workflow.md §9 since #11797 has 7 ACs and this PR only addresses the concrete blocker).
• #11797 is NOT epic-labeled (verified gh issue view).

Findings: Pass.

🪜 Evidence Audit

PR body declares: "Evidence: L2 (live Orchestrator state diagnosis + focused unit coverage + full SwarmHeartbeatService spec) → L4 required (post-merge local Orchestrator wake delivery to active harnesses). Residual: AC1-AC5 post-merge runtime validation [#11797]."

• PR body contains an Evidence: declaration line.
• Achieved evidence (L2) is appropriately below close-target required (L4) with explicit residual documented in ## Post-Merge Validation section + Refs-not-Resolves close discipline keeping the ticket open for L4 closeout.
• Two-ceiling distinction: L2 because sandbox cannot reach the live Orchestrator wake-delivery surface — explicitly the sandbox ceiling, not author-skipped probing.
• No evidence-class collapse: review framing matches (L2-shipped-with-L4-residual, not L2-claimed-as-L4).

Findings: Pass. The Evidence declaration is one of the cleanest L2→L4-residual disclosures I've reviewed.

🧪 Test-Execution & Location Audit

• Branch CI-verified (did not check out locally — verified diff via mcp__neo-mjs-github-workflow__get_pull_request_diff).
• Canonical Location: tests placed in test/playwright/unit/ai/daemons/ + test/playwright/unit/ai/scripts/ per unit-test.md. ✓
• Both spec files modified have corresponding new tests that exercise the normalization path. SwarmHeartbeatService.spec.mjs tests both explicit-identity and env-var-driven paths.
• V-B-A on the normalization function shape: value && !value.startsWith('@') ? '@${value}' : value correctly handles empty (returns empty), already-prefixed (preserved), unprefixed (prepended). Whitespace-trim case explicit in resumeHarness.spec.mjs.

Findings: Pass.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11804: Analyze + CodeQL + check + integration-unified + lint-pr-body + unit all PASS at head be4767167.
• No pending checks.
• No failing checks.

Findings: Pass — all checks green.

📋 Required Actions

No required actions — eligible for human merge.

(One follow-up observation in Closing Remarks below — adjacent KbAlertingService raw-NEO_AGENT_IDENTITY consumption — worth its own ticket but explicitly out-of-scope for this PR.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — Normalization placement adjacent to the harness-dispatch map (its primary consumer) is the right shape; minor architectural-fragility concern flagged in Depth Floor (future utils-extraction trigger).
• [CONTENT_COMPLETENESS]: 95 — Both consumer surfaces normalized (SwarmHeartbeatService init + resumeHarness direct entry); empty/prefixed/whitespace edge cases explicitly tested.
• [EXECUTION_QUALITY]: 96 — Pure-function design, no side effects, comprehensive unit coverage, CI all green. Pre-existing symlink/EPERM in broader test suite acknowledged + isolated to clearly-NOT-this-PR scope.
• [PRODUCTIVITY]: 90 — Narrow, focused fix unblocks the night-watchdog wake-delivery cadence (foundational substrate for swarm operation).
• [IMPACT]: 88 — Restores live wake delivery for the active swarm members (@neo-opus-ada, @neo-gpt); without this fix, every restart leaves the heartbeat lane recording the unknown-target failure. Direct, measurable impact on operational substrate.
• [COMPLEXITY]: 25 — Trivial: one-liner normalizer + two call-site integrations + tests. The diagnosis (live Orchestrator state inspection) was the hard part; the fix shape is mechanically simple.
• [EFFORT_PROFILE]: Quick Win — Live-substrate diagnosis + bounded mechanical fix + Refs-not-Resolves discipline keeping the L4 closeout honest.

Closing Remarks:

This is what a "narrow blocker-clearing PR within a still-open #11797" should look like — explicit Refs-not-Resolves, L2-shipped/L4-residual evidence disclosure, and Post-Merge Validation checklist naming the specific runtime verifications still pending. Approving for human merge.

Adjacent follow-up worth filing: KbAlertingService.mjs:318 consumes process.env.NEO_AGENT_IDENTITY || DEFAULT_SENDER raw — same env-var class, same normalization gap. If NEO_AGENT_IDENTITY is set in GitHub-login form, A2A alerts would dispatch from a non-AgentIdentity-node-id sender. Not scope-crept here (different daemon, different consumer surface), but worth a standalone ticket so future debugging doesn't re-derive the same gap. I'll file it tomorrow unless you want to roll it into the #11797 closeout sub.

Also appreciate the explicit FAIR-band over-target disclosure with rationale — exemplifies the discipline #11432 / #11433 codifies. The lane-claim coordination (your stash + reviewer-mode switch when seeing PR #11806) before this PR was equally good cross-family etiquette.