PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: One required action, and it is not follow-up-ticket fuel. This PR ships the file-level authority registry for a repository cut; 12 of its 19 rows carry a field that contradicts their own rationale. Merging a self-contradicting authority row and ticketing the cleanup would leave the ambiguity in the exact artifact whose stated purpose is to remove it. The fix is small and local, so Request Changes is cheaper than Approve+Follow-Up here.

Peer-Review Opening: This is a well-built extension — you derived the population instead of hand-listing it, kept the file action strictly independent of Edge/Cloud custody, and wrote a red arm per mutation class. One contract detail needs settling before merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17699 (Contract Ledger, ACs, Avoided Traps), the changed-file list, current dev source of agentOsExtractionInventory.mjs (1766 lines) and its module JSDoc, the sibling registry JSON, and scoped ai:structure-map --root ai/scripts/diagnostics --files --loc.
• Expected Solution Shape: A workflow-file population derived from the existing workflow-reference rows — never a second hand-maintained list — joined to one explicit registry row per file over a closed move | pin-fetch | retire vocabulary, with action-specific required metadata and loud failure on missing / duplicate / stale-extra / malformed / unknown. Must not hardcode 19 as the population, and must not let an occurrence's Edge/Cloud plane choose a file action. Test isolation: one red arm per mutation class, not one combined arm.
• Patch Verdict: Matches, with one contract drift. The population genuinely derives — buildInventory() feeds reconciled.rows.filter(row => row.surface === SURFACE.workflowReference), and the spec cross-checks the result against an independently rebuilt [...new Set(workflowReferences.map(...))] rather than a literal. 19 appears only as an evidence assertion over real data, so a 20th workflow fails missing-workflow-file-authority exactly as the trap requires. The action is read solely from entry.disposition; occurrence custody survives only as evidence.occurrenceDispositions. The drift is in the per-action metadata contract — see the Contract Completeness Audit.
• Premise Coherence: Coheres — verify-before-assert. The instrument's whole shape is "derive the population from source, then join it to an explicit authority, and refuse to be green when the two disagree." Keeping retire: 0 rather than inventing a retirement row to exercise the branch is the same value applied honestly; the vocabulary is proven by fixture instead.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17699
• Related Graph Nodes: Epic #17500 (non-closing Related:), ADR 0040 §§2.4/2.7/2.8, ADR 0039, #17525, #17533, #17640
• Origin Session ID: f13e43f1-5332-434a-bbae-600b69af2faa

🔬 Depth Floor

Challenge: deriveWorkflowFilePopulation() accepts an occurrence whose evidence.workflowFile is absent by falling back to row.identity.split('::', 1)[0]. That fallback is correct today because collectWorkflowReferences() now always sets the field, so the two agree by construction. The concern is drift: if a future producer emits an occurrence whose identity prefix and evidence.workflowFile disagree, the fold silently prefers evidence and no guard notices the divergence. Not blocking — both writers are in this file and the identity regex constrains the result — but a cheap assertion that the two resolve identically would convert an invariant currently held by convention into one held by the instrument.

Rhetorical-Drift Audit:

• PR description: framing matches the diff — with one exception, flagged as RA-1 (AC-4 is restated as "target + immutable pin authority for pin-fetch" where the ticket AC says "immutable pin authority for pin-fetch").
• Anchor & Echo summaries: precise. WORKFLOW_FILE_DISPOSITION's docblock states the why — "one answers where a referenced target runs, the other answers which repository owns the workflow after the cut" — which is the distinction the whole leaf exists to hold. No metaphor, no snapshot anchors.
• [RETROSPECTIVE] tag: none claimed; none warranted.
• Linked anchors: ADR 0040 §§2.4/2.8 do carry the membership and cutover obligations cited.

Findings: One drift, carried as RA-1.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: Full npm run --silent ai:structure-map -- --files --loc remains unusable for this surface — it fails with Cannot create a string longer than 0x1fffffe8 characters. Both the author and I fell back to --root ai/scripts/diagnostics, which succeeded. The mandatory pre-verdict step is currently satisfiable only by scoping, so the guide's unscoped instruction cannot be followed as written at this repo size.
• [RETROSPECTIVE]: The load-bearing decision here is refusing to let edge/cloud custody imply a file action. Those two vocabularies answer different questions — where a referenced script executes versus which repository owns the workflow — and collapsing them would have produced a plausible, fully-green registry that silently mis-routes workflows at the cut. Keeping them orthogonal, with occurrence dispositions demoted to evidence, is why byDisposition can be trusted.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: close-target ACs are repository-local and fully covered by the focused unit guard plus the committed SHA-bound receipt (no runtime surface beyond CI reach); no ai/mcp/server/*/openapi.yaml surface is touched; no skill file, convention, or architectural primitive is introduced.

🎯 Close-Target Audit

• Close-targets identified: #17699 (newline-isolated Resolves #17699 in the PR body)
• #17699 confirmed not epic-labeled — it is leaf 7 of Epic #17500, and the epic is correctly carried as a non-closing Related: reference

Commit 2a28f17ef2 ends (#17699) and carries no magic close keyword, so the body is the single close authority.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger — drift found

The ledger and AC-4 both map metadata to actions 1:1: "Action-specific metadata is enforced: target repository for move, immutable pin authority for pin-fetch, retirement/successor evidence for retire." The reconciler instead requires targetRepository === 'neomjs/neo-agent-brain' for both move and pin-fetch. Carried as RA-1.

Findings: Contract drift flagged.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 2a28f17ef2 (integration-parity success; mergeStateStatus: CLEAN), plus the author's committed clean-tree receipt at schema v5, ok:true, 19/69, zero residue.
• Reviewer falsifier: no named behavioral concern — I verified the population-derivation claim by source read rather than execution, since the assertion is structural.
• Test location: correct — the spec extends the existing focused sibling at test/playwright/unit/ai/scripts/diagnostics/.

The mutation coverage is the strong part, and it is per-class rather than one combined arm: missing-workflow-file-authority; duplicate-workflow-file-authority; invalid-workflow-file-disposition (the hostile copy fixture, which is the trap the Epic explicitly names); stale-workflow-file-authority; unexpected-workflow-file-metadata; and a dedicated arm proving each action's terminal metadata is required. The mixed-custody arm is the one I care most about — a single file carrying both edge and cloud occurrences resolves to one pin-fetch action with occurrenceDispositions: ['cloud','edge'] retained as evidence, which convicts any future attempt to infer the action from custody.

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — pin-fetch rows assert a targetRepository that contradicts their own rationale. All 12 pin-fetch rows carry "targetRepository": "neomjs/neo-agent-brain", identical to the 7 move rows, while their rationale states the opposite — "the Engine retains learn/agentos decisions, so its ADR seam gate stays here" and "Engine pull requests retain their admission gate". A pin-fetch workflow by definition does not go to neo-agent-brain; it stays in Engine and fetches from a pin. So one field name answers two opposite questions depending on the row's action — destination for move, fetch-source for pin-fetch — with nothing in the JSDoc or the registry recording that. Concretely: anyone asking "which workflows move to neo-agent-brain?" and filtering this authority by targetRepository gets 19 instead of 7, in the registry whose stated purpose is preventing a workflow from being "broken in Engine, duplicated across repositories". Note the code already establishes the right precedent one branch below — retire pushes targetRepository onto unexpected precisely because it does not apply. Either apply that same treatment to pin-fetch (restoring the ledger's 1:1 mapping; pinAuthority already names the pinned runtime), or keep the field and disambiguate it per action by name and docblock. Your call which — both discharge RA-1. If you keep it, AC-4's restatement in the PR body should say so explicitly rather than widening the ticket's wording in passing.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 85 — Placement is right and deliberately so: extends the two existing diagnostic siblings, adds no .mjs file, creates no second registry, and composes with the per-occurrence layer instead of replacing it (scoped structure-map confirms the inventory script + JSON as the owning siblings among 39 files). 15 deducted because the metadata contract carries a field whose meaning varies by action with nothing recording it.
• [CONTENT_COMPLETENESS]: 80 — Both new exported functions carry @summary + typed params, and WORKFLOW_FILE_DISPOSITION documents the custody-vs-cut distinction rather than restating the values. 20 deducted for the undocumented per-action semantics of targetRepository — the exact gap RA-1 names.
• [EXECUTION_QUALITY]: 85 — Bidirectional residue, deterministic sort on rows and errors, identity-shape validation, non-array guards on both registry and population, and unexpected cross-contamination detection. 15 deducted because the pin-fetch target requirement encodes the contradiction into the guard itself, so the instrument now enforces the ambiguity.
• [PRODUCTIVITY]: 90 — Seven of eight ACs are cleanly met with committed evidence; AC-4 is implemented broader than specified. Every named Avoided Trap is genuinely avoided, including the two easiest to fake (hardcoding 19, and deriving the action from plane custody).
• [IMPACT]: 80 — This is the file-level authority governing whether 19 workflows move, stay-and-pin, or retire at a repository cut. Wrong rows here surface as broken or duplicated CI after relocation, not as a failing test.
• [COMPLEXITY]: 70 — Two new exported functions, a fold-then-join with residue in both directions, and wiring into an already 1189-LoC composite report whose ok is now an eight-term conjunction.
• [EFFORT_PROFILE]: Heavy Lift — moderate diff size against high downstream consequence; the cost is in getting the authority vocabulary exactly right, not in the line count.

Genuinely good instincts on the two things that would have been easy to get wrong and hard to catch later — deriving the population instead of listing it, and keeping the cut action orthogonal to execution custody. RA-1 is the same discipline applied one field further: targetRepository is currently answering a different question for pin-fetch rows than for move rows, and only the rationale prose says so.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code

[review-budget-managed]

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