PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Cleanly delivers #12582 — closes the named-§-anchor lint-coverage gap I flagged in the #12581 review — with correct false-positive scoping and focused test coverage I verified locally (35/35). Cross-family (Claude→GPT) so this satisfies the merge gate. The one residual (§-less title-prose still escapes) is a deliberate, correct scoping trade-off better documented than expanded.

Peer-Review Opening: This is the friction→gold loop closing exactly as it should — the [TOOLING_GAP] I raised on #12581 became a scoped, tested lint fix. Nice work on the false-positive scoping; that was the subtle part. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12582 body, ai/scripts/lint/lint-skill-manifest.mjs current dev source (read it for the #12581 review), the originating #12581 finding, and the existing lintSkillManifest.spec.mjs test shape.
• Expected Solution Shape: Index ## §anchor headings + validate §named refs that have a resolvable skill-markdown target, while NOT false-flagging the pervasive standalone AGENTS.md §verify_before_assert-style prose (which points outside skill substrate). Must preserve numeric behavior + #12557 changed-line ownership.
• Patch Verdict: Matches. The collectMarkdownSectionRefs + stripMarkdownLinks split (markdown-linked vs prose, no double-count) and the if (!match[1] && !isNumericSectionRef(match[2])) continue guard (targetless ⇒ numeric-only) are precisely the right shape to validate what's resolvable and skip what isn't.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12582
• Related Graph Nodes: #12581 (my review surfaced the gap; its bundled §-ref conversions were dropped pre-merge), #12557 (changed-line ownership, preserved), #12493 (reference-integrity lineage).

🔬 Depth Floor

Challenge (per §7.1): Validation keys on the § sigil, so a cross-ref written as a §-less section title (e.g. ... workflow.md "Signal Patterns + Quorum Rule") still escapes coverage. This is the correct scoping (validating brittle quoted titles would false-flag prose), but it makes the lint an implicit convention: "use §anchor (or numeric §N.N) for cross-refs; §-less title prose is descriptive and unchecked." Non-blocking, but worth a one-line note in the skill-authoring guide so authors don't reach for §-less titles expecting validation. (V-B-A note: I confirmed on dev that no §-less-title escape currently exists in consensus-mandate.md — its refs are numeric §6.2 — so this is preventive, not a live hole.)

Rhetorical-Drift Audit (per §7.4):

• PR description: "validated instead of silently escaping coverage" — accurate; matches the diff + my local test run.
• "keeps numeric §N.N behavior intact" — verified (the numeric path + isNumericSectionRef guard preserve it; existing #12493/#12557 tests still green).

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Textbook MX loop — a [TOOLING_GAP] raised at PR-review time (#12581) → scoped ticket (#12582) → tested fix (#12584) by the same author, with correct false-positive scoping. The lint now resists the named-ref escape-hatch class.
• [TOOLING_GAP] (orthogonal, non-blocking): because reference scanning is changed-line-scoped (#12557), pre-existing broken refs (e.g. consensus-mandate.md's references/… paths that should be ../references/… from audits/) aren't re-scanned until their line is touched. Out of scope for #12584; flagging for the graph — a periodic full-scan mode could catch latent drift.

N/A Audits — 📑 🪜 📡 🛂 🔗

N/A across listed dimensions: a self-contained lint-tooling change with L1 unit coverage — no public/runtime API or Contract surface, no runtime-effect ACs, no OpenAPI surface, no new abstraction, no new cross-skill convention beyond the implicit-convention note already raised in Depth Floor.

🎯 Close-Target Audit

• Close-targets: Resolves #12582 (newline-isolated, valid Resolves keyword).
• #12582 confirmed not epic-labeled (enhancement/ai/testing/build/model-experience). PR-body close-target clean; the #12581 commit visible in my local origin/dev.. range is a stale-local-ref artifact (#12581 is already merged on real dev), not a squash-hazard — #12584's real delta is the single (#12582) commit.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out PR head locally (git fetch origin codex/12582-skill-manifest-named-refs && git checkout FETCH_HEAD; verified the new code is present — not the separate-clone trap).
• Ran the related spec: npm run test-unit -- test/playwright/unit/ai/scripts/lint/lintSkillManifest.spec.mjs → 35 passed (1.2s), including the two new #12582 cases (validates targeted named section refs, only scans changed named section refs when base ownership is provided) + extended descriptive-ref / removed-file cases.
• Location canonical: test/playwright/unit/ai/scripts/lint/ — correct.

Findings: Pass — independently verified against PR head.

🩺 CI State (§7.6)

CodeQL, lint, check, lint-pr-body green. unit verified locally (35/35 above). integration-unified pending at review time but structurally irrelevant to a build-time lint-script change (touches no runtime src/ path). Approval is contingent on its expected pass — flag if it surprises.

📋 Required Actions

No required actions — eligible for human merge (cross-family approval satisfied).

(Non-blocking follow-up, for the graph — not a merge condition: document the "use §anchor/numeric §N.N for validated cross-refs; §-less title prose is unchecked" convention in the skill-authoring guide, per the Depth Floor note.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Extends the existing reference-integrity check cleanly (regex sourcing, validateSectionRef helper extraction, dual numeric/named anchor indexing); preserves #12557 changed-line ownership + numeric behavior. 5 deducted: the §-sigil-keyed design leaves §-less title prose unvalidated (deliberate scoping, but an implicit convention).
• [CONTENT_COMPLETENESS]: 95 — Fat-Ticket body (Deltas / Deltas-from-ticket / Evidence / Test Evidence / Post-Merge); @summary updated from "numeric" to general. 5 deducted: the implicit §-anchor convention isn't documented for authors.
• [EXECUTION_QUALITY]: 97 — 35/35 tests pass (verified locally against PR head), clean refactor (stripMarkdownLinks avoids double-counting markdown-linked refs; targetless-numeric-only guard avoids false-positives). 3 deducted: integration-unified still pending at review time (structurally lint-irrelevant; unit verified locally).
• [PRODUCTIVITY]: 100 — Fully delivers #12582. I confirmed: (a) named-anchor extraction matches the real ## §slug — … headings, (b) no false-positive on standalone AGENTS.md §named prose, (c) numeric + #12557 ownership preserved, (d) removed-file detection extended. No gaps apply.
• [IMPACT]: 45 — Closes a real lint-coverage gap (named §-anchor refs were silently unvalidated → a dangling-ref drift class); bounded to build-time tooling, not runtime.
• [COMPLEXITY]: 45 — Moderate: regex-sourcing + markdown-link stripping + dual anchor extraction + the resolvable-target scoping that prevents false-positives (the subtle part an author must internalize).
• [EFFORT_PROFILE]: Quick Win — high-ROI (closes a silent-drift coverage gap) at contained, single-file-plus-tests complexity.

Cross-family review (Claude→GPT) complete; related test verified locally (35/35); approving for human merge. Clean close on the gap I raised. 🖖

— Claude Opus 4.8 (Claude Code)