PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both findings from my #17199 review are closed, verified with my own probes rather than by reading the new tests. The one deviation from the ticket — classifying the odd shapes instead of merely reporting them — is strictly stronger than what I asked for and the residual class is still reported. Not Approve+Follow-Up: nothing is deferred.

Peer-Review Opening: I filed these two as measured-latent and explicitly non-blocking, so the honest thing to say first is that you did not just patch the two shapes I named — you generalised past them, and the generalisation catches a case neither of us wrote down. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: My own #17199 review (where both findings originated and where I measured them latent-zero); ticket #17202 as filed; the guard and spec at head 2e6ee8b104 read via the contents API; the exact-head check set.
• Expected Solution Shape: Two narrow repairs — reject a specifier that statSync reports as a directory, and stop dropping entries the extraction regex cannot match. The fix must NOT hardcode the two shapes I happened to name (missing ./, compound commands), because those were symptoms of a general property: the extractor excluded rather than reported. Test isolation: synthetic fixtures, since the real tree measures zero of both.
• Patch Verdict: Improves on the expected shape. The extraction is global and tolerant, so the named shapes are now checked rather than flagged, and the residual — a command that references ai/scripts yet yields no entry — is what gets reported. That is the right inversion: flagging a checkable entry would have been a worse outcome than checking it.
• Premise Coherence: Coheres: friction→gold. The two items were dispositioned non-blocking in review, which is the easiest category to lose. They became a ticket with red-proof ACs and then a PR that widened the fix beyond the report. The ratchet ran end to end without anyone re-deciding whether it was worth it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17202
• Related Graph Nodes: #17182 / PR #17199 (the guard this hardens, and the review that produced both findings) · #16929 (the closure walker that surfaced the original specimen) · #17171 (the required-status-context gap this workflow still inherits)
• Origin Session ID: 00348bc3-c011-4035-90a3-f0eb62b8c95c

🔬 Depth Floor

Documented search: I actively checked (1) whether the widened regex can double-count or skip on adjacent matches, (2) whether stat(candidate) can be reached with a path that does not exist — including through the config.template.mjs fallback branch — and (3) whether the unclassifiable list is merely printed or actually gated. Found no concerns.

On (3), because it is the one that decides whether my finding is really closed: a console.warn note in a workflow log would not have closed it — a log line nobody reads is the same silence in a different colour. It is gated by expect(unclassifiable).toEqual([]) in the real-tree pin, so a future escape fails the unit suite, which is a required check, rather than printing into a log. Placing the gate there rather than in the lint's exit code is also the correct call: an ai:* command that mentions ai/scripts without running one is not a broken entrypoint, so failing the guard on it would have manufactured a false verdict.

Reviewer falsifier — I re-ran my own probes against your extraction rather than trusting the new specs, since I am the one who claimed these were the shapes:

The ../ai/scripts/… row is the one worth naming: neither the ticket nor my review mentioned it, and it lands in unclassifiable rather than being silently excluded. That is the property my finding was actually about, reached by fixing the class instead of the two instances.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — the "classification over flagging" delta is declared rather than slipped in
• Anchor & Echo summaries: the JSDoc states the coverage-count invariant as the reason, not the mechanism
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: #17199 / #17202 check out

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The reusable shape is where the gate was placed. The guard prints what it could not classify and the spec fails on it — because the two questions are different: "is this entrypoint broken" belongs to the lint's exit code, "did our coverage silently shrink" belongs to a pinned invariant. Collapsing both into the lint would have produced either a false failure or a note nobody reads. Worth copying wherever a guard reports a count that reads as coverage.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed surface (the changed extractEntrypoints signature has exactly two consumers, both in this PR), no OpenAPI, no cross-skill convention, and every AC is decidable by unit test with no runtime effect the sandbox cannot reach.

🎯 Close-Target Audit

• Close-targets identified: #17202
• For each #N: confirmed not epic-labeled — #17202 carries enhancement / ai

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2e6ee8b104 — 21 checks pass, 0 failing, 0 pending, mergeStateStatus: CLEAN. Author receipts specific and current: 10/10 new spec arms, 332/332 lint-spec dir, live guard run 63/63.
• Reviewer falsifier: my seven-case extraction probe above, run against your code at head. All seven land correctly.
• Test location: pass — test/playwright/unit/ai/scripts/lint/ mirrors ai/scripts/lint/.

Findings: Pass. The directory arm is red-proved with a real mkdirSync fixture rather than a stubbed stat, which is the right choice: a stub would have proved the branch runs, not that a directory reaches it.

One thing I checked and found sound rather than assumed: stat(candidate) cannot be reached with a non-existent path. Every assignment to candidate is guarded by an exists() check, including the config.template.mjs fallback, and fs.existsSync follows symlinks so a broken link returns false rather than passing a stat-able-but-absent path through.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - The gate placement is the standout: exit code for "broken entrypoint", pinned spec invariant for "coverage shrank". Separating those is what keeps the guard from manufacturing a false verdict on a mention-only command.
• [CONTENT_COMPLETENESS]: 94 - The signature change is declared with its consumer count, the deviation from the ticket's literal wording is argued rather than slipped in, and the JSDoc carries the invariant instead of restating the code.
• [EXECUTION_QUALITY]: 93 - Global extraction with correct lastIndex handling, normalisation to a single form, injectable stat paired with the existing injectable exists, and a real-directory fixture.
• [PRODUCTIVITY]: 92 - Two review findings to filed ticket to widened fix, same day, without either being renegotiated.
• [IMPACT]: 80 - Hardens a guard rather than closing a live defect — both shapes measured zero on today's tree. Its value is that the coverage number stops being able to lie, which is what the original specimen taught.
• [COMPLEXITY]: 45 - Small diff over subtle ground: regex statefulness and a resolver boundary that already had one carve-out.
• [EFFORT_PROFILE]: Quick Win - Bounded, self-pinning, and it pays forward on every future ai:* entry.

You closed the two I named and the one I did not. Approving.

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