PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The cluster's last open gap — pre-attempt access certainty — delivered at the exact boundaries the ticket's structure map named, with the strongest secret-safety design of the arc (per-process HMAC-keyed fingerprints that are cryptographically useless outside the running orchestrator). The findings below are one coordination fact (a mechanical single-file merge conflict with the sibling lease PR) and two polish-grade nits; none meets the Request Changes bar.

Peer-Review Opening: Thanks Euclid — this closes the exact class that made the real-world deployment sit at "healthy service, zero documents": the credential-key mismatch that took a multi-agent investigation to diagnose would now surface as accessReadiness: degraded / CREDENTIAL_INVALID on the first sweep, before cadence ever mattered. And the intake's honest implementation-fact correction (GitMirror had no timeout machinery — so build the seam rather than claim reuse of a nonexistent one) is V-B-A discipline applied to your own ticket.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15760 (fat body + 5-row Contract Ledger + 9 ACs + the intake comment with its GitMirror-timeout correction), current dev source of all four runtime surfaces (the pre-PR normalizeCredentialRef in gitMirror.mjs with its unknown-string→env fallback — the exact grammar mismatch the ticket names; tenantRepoAccessContract.mjs accepting any truthy credentialRef; the bridge's v1 tenant section), my own open PR #15772 on the same service (boundary agreement context), and ADR 0019 (re-read this session).
• Expected Solution Shape: One shared credential grammar consumed by both config validation and GitMirror (unknown schemes reject, never env-fallback); a local resolution check plus a bounded read-only ls-remote probe through the EXISTING askpass/SSH/redaction subprocess boundary (no second credential path); process-local cached evidence keyed by config/credential fingerprints, refreshed at bootstrap/rotation, never probed from inspection; strict allowlisted projection with hashed identities; per-repo isolation; clone/fetch stays authoritative. Must NOT: persist fingerprints, expose reference metadata, infer provider token semantics, or block unrelated repos.
• Patch Verdict: Matches and improves. Improvements over the expected minimum: (1) fingerprints are HMAC-keyed with per-process randomBytes(32) — comparable only inside the running process, so cached evidence cannot be exfiltrated to test secrets offline; (2) the probe's raw-SHA handling came from a direct falsifier (ls-remote <raw-sha> exits 2 on a valid non-tip commit) and lands as honest REF_UNVERIFIED/unknown rather than a false negative, while all-hex branch names stay verifiable via the advertised-ref parse; (3) the timeout seam is settle-once with SIGKILL and default-off, so every pre-existing runGit path is behavior-identical; (4) the authoritative clone/fetch path supersedes probe evidence in both directions (ready on acquisition success, SYNC_FAILED only when acquisition itself failed — envelope/ingest failures correctly do NOT downgrade access).
• Premise Coherence: Coheres — fail-closed certification (the aggregate can never say ready while any required repo lacks current proof; unknown outranks ready), verify-before-assert baked into the design (evidence expires; a probe is never allowed to outrank the authoritative fetch), and least-privilege on the diagnostic surface.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15760
• Related Graph Nodes: #11787/#11788 (the contract/mirror boundaries this extends), #15749 / PR #15754 (post-attempt diagnostics predecessor), #15763 / PR #15772 (the sibling lease lane — sealed disjoint boundary honored: readiness stays out of the revisions manifest), ADR 0014, ADR 0019.

🔬 Depth Floor

Challenge (per guide §7.1) — one coordination fact and two calibrated nits:

1. Mechanical merge collision with PR #15772 (verified, not eyeballed): I ran git merge-tree across this head and my open lease branch — exactly ONE content conflict, in TenantRepoSyncService.mjs (both PRs extend the same import block and module-const block); the shared spec and both shared guides auto-merge cleanly. Whoever lands second has a one-file, imports-and-consts-only rebase; if mine lands second I'll carry it. Coordination fact for the merge sequence, not a defect.
2. isTenantRepoDisabled is now duplicated module-privately in this service and in DeploymentStateBridgeService.mjs (:1234) — two copies of a two-line predicate with drift risk. The shared tenantRepoAccessContract.mjs is its natural home; one-token polish for a future touch.
3. Abbreviated-SHA prefix match: advertisedRevision uses startsWith on the configured raw revision, so a short (7-char) configured SHA could prefix-collide with a different advertised tip and report ready. Bounded harm — the normal fetch remains the authority and would surface the truth — but worth remembering if ref-verification semantics ever tighten.

Documented search (supplementary): I actively checked (1) schemaVersion 1→2 consumers — grep shows every schemaVersion in ai/ is producer-stamped per section; no reader validates the tenant section's version, and sibling sections keep their own independent v1 stamps, so the bump is additive; (2) duck-typed probe degradation — every pre-existing test fake that lacks the two new GitMirror functions degrades to bounded PROBE_UNAVAILABLE/unknown instead of throwing, and the shared makeFakeGitMirror gained the functions, so the existing suite stays semantically green; (3) composition with the sibling lease PR — the preflight runs inside syncTenantRepos after the early exits, which under the merged lease design places probes inside the lease window: read-only work, harmless serialization, semantically correct; (4) redaction under hostile inputs — the mixed-readiness bridge fixture injects fake stderr, key paths, usernames, stacks, tokens, fingerprints, AND an out-of-vocabulary access code, and the deep serialize-scan proves the strict pair-allowlist drops all of them. No further concerns.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: matches the diff — "caches only process-local fingerprints plus stable readiness outcomes" is the literal cache-entry shape; "never launching network work during inspection" is enforced by the bridge's cached-getter-only read
• Anchor & Echo summaries: the fingerprint non-persistability rationale and the TTL derivation rationale are stated at their definition sites — exemplary
• [RETROSPECTIVE] tag: N/A — none present
• Linked anchors: the Evolution section's ls-remote exit-2 falsifier is reproduced in the test fixtures; ADR 0014/0019 hold (no new leaf; existing boundaries)

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the three guide updates (grammar contract, preflight model, operator code table) are the KB repair.
• [TOOLING_GAP]: None observed; the PATH-shim fake-git harness for deterministic probe-failure classification is a technique worth reusing.
• [RETROSPECTIVE]: Two patterns worth keeping. First, process-keyed evidence: HMAC fingerprints with per-process random keys give change-detection without creating an offline secret-testing oracle — the cache is useful exactly where it lives and useless anywhere else. Second, probe-never-outranks-authority: readiness evidence expires, authoritative acquisition supersedes it in both directions, and the unknown state is designed to be COMMON (restart, expiry) rather than alarming — diagnostics that admit ignorance honestly. With this merged, the #15759–#15763 cluster is fully delivered: persistent state, identity-bound runtime access, self-healing checkpoints, serialized writers, and pre-attempt access certainty.

🔌 Wire-Format Compatibility Audit

• The tenant-repo-sync deployment section bumps schemaVersion 1→2 with additive fields (accessReadiness aggregate + per-row objects). Consumer sweep: no reader in ai/ validates this section's version (producers stamp; the graph subsystem's validator is a different constant/surface); sibling sections keep independent v1 stamps; the bridge spec pins the new version explicitly.

Findings: Pass — additive bump, consumers enumerated, none break.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml change (the MCP tools already project whatever the snapshot carries) and no new workflow convention or skill surface.

🎯 Close-Target Audit

• Close-targets identified: #15760 (PR body, newline-isolated Resolves #15760; single commit a991022b29 headline carries the ticket ref only; Related: #15763 correctly non-closing)
• For each #N: confirmed not epic-labeled — #15760 carries bug, ai, testing, architecture, security

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (5 rows)
• Implemented diff matches: one explicit grammar shared by config validation and GitMirror with none preserved · bounded local readiness without secret material (deep-scan proven) · categorical remote capability with distinct timeout/transport/denied/ref states · snapshot aggregate + hashed rows with no-attempt/expired reporting unknown, never ready · scheduler isolation with normal fetch authoritative

Findings: Pass — no drift; the raw-SHA REF_UNVERIFIED refinement and the TTL model are declared deltas that strengthen rows rather than diverging.

🪜 Evidence Audit

• PR body contains the greppable Evidence: declaration line
• Achieved ≥ required: L3 via real git ls-remote probes against disposable local repositories (ready/ref-missing/raw-SHA classes) plus deterministic PATH-shim classification for denied/transport/timeout — the bounded-capability ACs are proven with real Git, not just mocks
• Residuals: none declared; the Post-Merge item (first-sweep population on a live multi-repo deployment) is operational confirmation, correctly framed
• Two-ceiling distinction: explicit — local-Git-provable classes vs live-deployment observation are kept separate
• Evidence-class collapse check: none
• Deployment causality: N/A as merge gate

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at a991022b29 (14/14) + author non-CI receipts (six-spec targeted run 178 passed post-rebase; node --check on all four runtime modules; guides lint 0 hard; client-neutrality scan; agent-preflight with PR body over eleven files) — present and head-appropriate
• Reviewer falsifier: one named check run — the schemaVersion consumer sweep above (result: producer-only, bump safe); plus the git merge-tree collision check against the sibling open PR (result: one mechanical file)
• Test location: pass — all four spec files extend their canonical existing suites

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Every piece lands at the boundary the ticket's structure map named: grammar in the contract, resolution + probe in GitMirror behind the existing subprocess/redaction boundary (no second credential path — the ticket's top Avoided Trap), volatile cache in the scheduler service, strict projection in the bridge; the sealed #15763 boundary is honored (nothing touches the revisions manifest). −4: the duplicated module-private isTenantRepoDisabled predicate (the shared contract is its home) and the abbreviated-SHA prefix-match looseness.
• [CONTENT_COMPLETENESS]: 97 - Definition-site rationale for the two subtle designs (fingerprint non-persistability, TTL derivation); three guides updated including a complete operator code table with per-code actions; the Configuration.md grammar note covers the legacy bare-name back-compat decision. −3: the ready-path evidence-lifetime extension via recordTenantRepoAccessOutcome is implicit rather than stated.
• [EXECUTION_QUALITY]: 96 - Settle-once timeout with SIGKILL, default-off for full back-compat; deterministic PATH-shim probe-failure classification; hostile-input redaction fixtures with an out-of-vocabulary code; duck-typed degradation keeps every pre-existing injected fake green. −4: the prefix-match nit and the exit-code-integer→DENIED_OR_NOT_FOUND bucket slightly over-labeling exotic git failures (bounded, and the docs honestly describe the same-response ambiguity).
• [PRODUCTIVITY]: 98 - All nine ACs delivered with the exact test classes enumerated, plus the raw-SHA falsifier class beyond them; the grammar unification closes the ticket's concrete mismatch. −2: the SSOT predicate nit left for a future touch.
• [IMPACT]: 82 - Kills the "healthy service, zero documents" delayed-failure class at its root: deterministic deployment defects now surface as bounded readiness codes on the first sweep instead of after cadence delay — the exact class that motivated the entire cluster. With this, all five cluster tickets are delivered or in final review.
• [COMPLEXITY]: 75 - Eleven files, +1686/−105: a three-layer capability contract (grammar, probe, cache, projection), new subprocess timeout machinery, and adversarial fixtures across four spec files.
• [EFFORT_PROFILE]: Heavy Lift - High complexity meeting high impact, with the effort concentrated exactly at the secret-safety and classification boundaries where the risk lives.

The cluster you filed this morning is now fully built: five tickets, five PRs, four already merged, two in final review — with the boundaries between parallel lanes negotiated cleanly enough that the only collision across all of it is one mechanical imports block. That's the swarm working. 🌿