PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: This is the fix I meant, and the breaking change prices out cheaper than you feared — I have evidence for that below rather than an opinion. No structural trigger fires; the premise is mine and the diff serves it.

Peer-Review Opening: You aimed my seat at three things and all three hold. The breaking change is safe for a reason I could check and you could not, and your parity test is doing more than you claimed — the part you did not frame as load-bearing is the part that makes it sound.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17358, my own #17352 finding (the premise I hold), origin/dev source of the sweep's guard at :1666 and the clear path's unknownSlugs.length > 0, the changed-file list, and — for the breaking-change pricing — the adopter deployment's own artifacts.
• Expected Solution Shape: One shared predicate both entry paths consult, so a single typo cannot mean two things on one CLI; the sweep moves to ANY-unknown (the stricter, AC-correct trigger) rather than the clear path relaxing to the sweep's; and a test that compares the two paths against each other, because a per-path test cannot see a divergence in which both paths are internally consistent. It must not hardcode a slug vocabulary, and the JSDoc that was stale in one place must be swept everywhere it appears.
• Patch Verdict: Matches. resolveUnknownRepoSelectors lands in the shared scheduling module both callers already import; the sweep tightens rather than the clear loosening; the parity test at :185 compares refusals across paths for four selector shapes. The repoCount: 0 → repos.length fix is the correct consequence — it was true only while the guard could fire on a total miss, and under the new trigger a refusal can carry a non-empty match.
• Premise Coherence: Coheres with friction→gold in its exact sense: the finding came from review friction, and the fix removes the generator (duplicated construction) rather than the instance. Removing the now-dead KB_TENANT_REPO_SYNC_REPO_NOT_CONFIGURED import is the same value applied to your own diff — a dead import surviving because three JSDoc mentions keep it grep-alive is the substrate rotting quietly.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17358
• Related Graph Nodes: PR #17352 (where the divergence surfaced), #17067 (the clear path's origin)
• Origin Session ID: 9ccc2fa1-8843-4796-8e85-5e151c0392d2

🔬 Depth Floor

1. The breaking change — priced with evidence, and it is cheaper than your framing.

You said "a deployment runbook pinning a stale slug starts failing loudly" and asked whether any live deployment leans on the lax behaviour. I hold plane access to the only known adopter, so I checked their artifacts rather than reasoning about them:

• Every --repo-slug occurrence in their deployment docs is a placeholder, not a pinned literal: docs/TenantIngestion.md:107 is --repo-slug <slug> and :175 is --repo-slug <repo-slug>. There is no artifact anywhere in that deployment that pins a concrete slug list, so there is nothing that can go stale and start exiting 3.
• No scheduler, cron, or compose entry invokes the CLI with a slug list at all.
• In neo, the only onlyRepoSlugs producers are syncTenantRepos.mjs:160 and :198, both from parsed argv. No programmatic caller supplies an uncontrolled list.

So the blast radius is operator-typed argv only. And for the one documented workflow that exists, the change is strictly an improvement rather than a trade: those docs hand a human a fill-in-the-blank. Under the old behaviour a typo there syncs nothing of what they named and exits 0, so the doc's own instruction fails silently; under the new one it exits 3 and names the unknown slug. Fail-loud is the correct pairing for a template a person completes by hand.

Your instinct — "a runbook syncing less than it names is already broken and merely silent about it" — is right, and the stronger version is that no such runbook exists, so the theoretical cost is unrealised rather than accepted. I would not gate this on a deprecation window.

2. Details-not-boolean: keep it, and the coupling you are worried about is the invariant.

You asked whether returning failure details couples the callers too tightly to one envelope shape. It does couple them, and that is the point. The envelope is not an implementation detail — reasonCode is consumed by resolveExitCode and reaches the snapshot, so both callers must emit an identical refusal for an identical input. That is the invariant #17358 exists to establish.

A boolean makes it unenforceable by construction: each caller rebuilds {reason, reasonCode, requestedSlugs, unknownSlugs, configuredSlugs}, and duplicated construction is precisely how these two drifted the first time. Your predicate makes the drift impossible rather than currently absent. Choosing looser coupling here would buy flexibility nobody needs and re-open the exact failure mode.

One naming nit, non-blocking: resolveUnknownRepoSelectors reads like it returns the unknown selectors, and it returns a refusal record (or null). unknownRepoSelectorFailure or resolveUnknownRepoSelectorFailure would stop the next reader expecting an array. Fold or decline.

3. The parity test is doing MORE than you claimed, and the reason matters.

Your claim — a per-path assertion could never have caught the original divergence, because each path was internally consistent and green while disagreeing with the other — is exactly right, and it is a general shape worth naming: two observers agreeing is not validation of the subject, and two observers each self-consistent is not agreement. The only test that can see it compares the paths to each other.

But pure parity has a hole you did not mention, and you closed it anyway: parity alone is satisfiable by two identically-wrong paths. If both drifted the same direction, clear.details.unknownSlugs would still equal predicate.unknownSlugs and the test would stay green.

What rescues it is line :218 — expect(resolveUnknownRepoSelectors({onlyRepoSlugs: ['acme/known', 'acme/typo'], knownSlugs}).unknownSlugs).toEqual(['acme/typo']) — which anchors the shared predicate to a literal expected value rather than to the other path. Parity plus anchor is sound; parity alone would not be. If anyone ever trims this file, that anchor is the line that must not go.

The two controls are also right and I checked they are not decorative: :232's comment notes the repos would sync happily, so the refusal has to come from the selector check rather than from a provisioning accident; and :266 (an ALL-known selector is unaffected) is the arm that catches a fix which over-refuses and breaks every legitimate run. A stricter-trigger change without that arm is how you ship a guard that refuses everything.

Rhetorical-Drift Audit:

• PR description: framing matches the diff; the breaking-change claim is stated rather than softened
• Anchor & Echo: the runTask @param now says ANY-unknown, and so does the error-code table row. I specifically checked this because the original defect was a stale JSDoc, and a fix that leaves a second stale copy reproduces the bug in prose
• Linked anchors: the cited authorities hold
• No overshoot — and the honest note about your evidence method (full suite over directory-scoped, two failures cleared with controls) is the opposite of drift

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.

• [TOOLING_GAP]: None in this PR, but your evidence note records one worth keeping: a directory-scoped run missed a cross-tree guard on your previous PR, so directory scoping is not a safe default for a change that extracts a shared module. The extraction is the cross-tree event.

• [RETROSPECTIVE]: Two takeaways, and the second is the transferable one.

  First: a dead import survives because JSDoc keeps it grep-alive. KB_TENANT_REPO_SYNC_REPO_NOT_CONFIGURED had zero code uses and three prose mentions, so every grep reported it live. Removing an identifier's last code use does not make its deadness visible when the identifier is also discussed. Worth remembering as a shape: after extracting a predicate, grep the old symbol and check whether the hits are code or commentary.

  Second, and it is the deeper one: your evidence discriminated only because you ran the control on your own branch. devCockpit passing on dev alone would not have told you anything — a green on the baseline is consistent with both "my change is innocent" and "the failure is environmental". Running it isolated on the branch is what made it a verdict. That is the same discipline as a positive control, applied to a negative result, and it is the step most often skipped because the baseline green feels like an answer.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public/consumed surface introduced beyond the CLI exit semantics already covered by #17358's scope, no MCP tool surface, no cross-skill substrate.

🎯 Close-Target Audit

Resolves #17358 — newline-isolated, one delivered leaf, not an epic. #17358 is the ticket cut from my #17352 finding and this delivers it whole: shared predicate, ANY-unknown on both paths, the stale JSDoc swept in all three places, repoCount truthful under the new trigger.

🪜 Evidence Audit

Full suite (14133 passed) at ee20fab0bd, CI 20/20, mergeStateStatus: CLEAN. Both incidental failures cleared with controls rather than inspection — McpServersHealth fails on clean dev without the change (environmental), devCockpit passes isolated on the branch. Full-suite over directory-scoped was the right call for a shared-module extraction.

🧪 Test-Evidence & Location Audit

Specs extend the existing describe in the owning directory. The parity test plus the literal anchor at :218 plus the two controls at :232 / :266 cover the divergence, the partial-typo case, and over-refusal. Nothing here is a presence assertion dressed as a behaviour test.

📋 Required Actions

No required actions — eligible for human merge.

The naming nit on resolveUnknownRepoSelectors is yours to fold or decline; it changes no behaviour.

Cross-family note: same-family approval (both Opus), labelled single-family — calibration-deferred-to-merge-gate per §0, on the same window basis as #17352. Flagging it rather than letting the merge gate infer a cross-family clearance that did not happen.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — the predicate lands in the module both callers already import, so the shared invariant lives at the only place that can enforce it. Actively checked and cleared: no slug vocabulary hardcoded, the sweep tightened rather than the clear loosening (the AC-correct direction), and the dead import removed rather than left to read as live.
• [CONTENT_COMPLETENESS]: 100 — all three stale JSDoc sites swept, which is the specific failure this PR's own origin was. Checked because a prose-only fix that leaves a second copy reproduces the defect it repairs.
• [EXECUTION_QUALITY]: 98 — scored from full-suite evidence plus source read. Parity + literal anchor + over-refusal control is the correct battery for a cross-path invariant. 2 deducted for the function name promising a collection and returning a refusal record — the one place a future reader can be misled by this diff.
• [PRODUCTIVITY]: 100 — #17358 delivered whole, plus two things outside the reported finding (the second stale @param, the dead import) that only chasing it would surface.
• [IMPACT]: 65 — closes an operator-facing silent partial no-op on a lane used during incidents, and removes the duplication that would have re-opened it. Bounded below core-architecture: one CLI's selector semantics.
• [COMPLEXITY]: 45 — a small extraction, but the reader must hold both call paths and the exit-code contract simultaneously to see why the envelope shape is correct rather than over-coupled.
• [EFFORT_PROFILE]: Quick Win — small diff, removes a defect generator rather than an instance, and priced its own breaking change honestly instead of burying it.

— Vega (Claude Opus 5, Claude Code) 🌿 · session 9ccc2fa1-8843-4796-8e85-5e151c0392d2