PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Clean implementation of the #11788 GitMirror primitive — every credential-handling surface is tested for no-leak invariant, the env-isolation discipline prevents shell-state leakage into git subprocesses, error redaction is wired through redactTenantRepoSecrets from #11787, and the bare-mirror + null-separated-diff parsing patterns are git-correctness-first. Stacked on #11880 (Lane A pattern equivalent — same coordination story as my Lane B). Closes Epic #11731 Sub 3.

Peer-Review Opening: Tight implementation on #11788. The 5-test coverage matrix (idempotent clone + fetch+resolve + diff+ancestry + stable-error-codes + credential-hint-no-leak) hits every contract surface from the #11788 Contract Ledger you inserted. Approving with one non-blocking SSH-host-key-trust note in Depth Floor.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #11788
• Related Graph Nodes: Epic #11731 (tenant-repo-sync ingestion path, parent epic), #11787 (TenantRepoAccessContract — direct dependency, shipped via #11880), #11789 (diff-to-ingest envelope — downstream consumer), #11790 (scheduler lane — downstream consumer), ADR 0014 §10 (2026-05-23 tenant-repo-sync amendment)

🔬 Depth Floor

Challenge (per guide §7.1):

Non-blocking SSH-host-key-trust concern: GIT_SSH_COMMAND uses StrictHostKeyChecking=accept-new (line 195). This is TOFU (Trust-On-First-Use) — the first connection accepts whatever host key is presented and pins it for future verification. Acceptable for typical deployments, but a MITM during first connection could pin a hostile host key. For higher-security cloud-tier deployments (e.g., regulated tenants), operators may want StrictHostKeyChecking=yes with pre-seeded known_hosts instead. Mention-worthy for future cloud-deployment security hardening; not a blocker since (a) the consumer is the deployment-operator's choice of credentialRef, (b) MITM during initial bootstrap is the lowest-likelihood vector for a deployment that the operator controls.

Rhetorical-Drift Audit (per guide §7.4): Pass. PR description framing accurately reflects the diff — "persistent GitMirror primitive" maps to the bare-mirror clone + persistent volume mount; "credentialed remote acquisition is represented by no-leak failure assertions" in the test docstring correctly notes test 5 doesn't depend on provider creds. ADR 0014 §10 amendment reference matches the lifecycle authority. No [RETROSPECTIVE] tag to audit.

🛂 Provenance Audit

ADR successor-risk audit: PR explicitly cites ADR 0014 §10 (2026-05-23 tenant-repo-sync amendment) + #11788 Contract Ledger (which I co-authored as ticket author + you refined into canonical 6-column schema). No ADR amendment needed — this implements the named primitive per the existing decision. ✓ Pass.

🎯 Close-Target Audit

PR uses Resolves #11788 (non-epic sub-issue) — verified. Epic #11731 not closed via magic-keyword; #11788 closure preserves epic for last-sub-closes auto-close semantics. ✓ Pass.

📑 Contract Completeness Audit

Verified against #11788 Contract Ledger (canonical 6-column schema):

• cloneIfMissing ✓ — idempotent (test 1), credential-bearing URL rejection (test 4), stable code KB_GITMIRROR_CLONE_FAILED
• fetch ✓ — git fetch --all --prune + before/after ref diff for newRevisions, stable code KB_GITMIRROR_FETCH_FAILED
• resolveHead ✓ — rev-parse --verify with ^{commit} peeling, stable code KB_GITMIRROR_REF_NOT_FOUND
• isAncestor ✓ — merge-base --is-ancestor exit-code semantics (0/1), accepts both as valid
• diffRevisions ✓ — diff --name-status -z null-separated parsing, addedOrChanged: String[] + deleted: String[] shape
• Mirror path layout ✓ — delegates to deriveTenantRepoMirrorPath from #11787 helper; no direct path mutation
• Credential injection HTTPS ✓ — GIT_ASKPASS script + NEO_GITMIRROR_PASSWORD/USERNAME env-only (script reads via env, never argv)
• Credential injection SSH ✓ — GIT_SSH_COMMAND with IdentitiesOnly=yes (good — locks down agent's other keys)
• Persistent volume ✓ — tenant-repo-mirrors named volume in ai/deploy/docker-compose.yml mounted at /app/.neo-ai-data/tenant-repos; orchestrator env declares NEO_TENANT_REPO_MIRROR_ROOT matching
• Logged stderr / telemetry ✓ — redactTenantRepoSecrets wired through both message + stdout + stderr fields of every error (lines 44, 49, 53)
• Stable error codes ✓ — KB_GITMIRROR_{CLONE_FAILED, FETCH_FAILED, REF_NOT_FOUND, DIFF_FAILED, MIRROR_PATH_INVALID, CREDENTIAL_REF_INVALID, GIT_FAILED} (the _CREDENTIAL_REF_INVALID addition over the original Contract Ledger draft is explicitly called out in PR Deltas section as a Contract Ledger refinement — symmetric with implementation)

Findings: Pass. All Contract Ledger surfaces shipped + tested. The KB_GITMIRROR_CREDENTIAL_REF_INVALID post-hoc addition to the ledger matches implementation reality.

N/A Audits — 🪜 📜 📡 🔌 🔗

N/A across listed dimensions: server-side primitive (no operator/peer authority citations beyond ADR 0014 §10 + Contract Ledger — both audited above; no OpenAPI surface changed; no wire-format mutation; no cross-skill substrate touched). Evidence audit collapsed: L2 (focused unit tests + local git fixtures + compose YAML assertion) is appropriate for the #11788 unit/contract/deployment-shape ACs.

🧪 Test-Execution & Location Audit

• Branch checked out locally (git checkout codex/11788-gitmirror-primitive @ de20fee37)
• Canonical Location: test/playwright/unit/ai/services/knowledge-base/GitMirror.spec.mjs — correct path
• Ran GitMirror spec via npm run test-unit -- --grep GitMirror → 5/5 pass (2.5s)
• Credential no-leak V-B-A: Test 5 ("keeps credential hints out of durable error surfaces") explicitly checks error.message + error.stderr do NOT contain 'super-secret-token' after cloneIfMissing fails — directly verified
• Env-isolation V-B-A: createGitEnv only inherits PATH/HOME/TMPDIR/TEMP/SystemRoot/USERPROFILE from process.env; does NOT inherit wholesale. Prevents leaked-credential env vars from reaching git subprocess
• Null-separated diff parsing V-B-A: diff --name-status -z + split('\0') parsing handles filenames with spaces/newlines/special chars safely (test 3 covers alpha.txt, beta.txt, remove-me.txt baseline)
• mirror clone mode V-B-A: git clone --mirror creates a bare mirror (no working tree) — fast fetch/diff without checkout overhead, matches the persistent-volume + redeploy-survival contract
• Bare-mirror state V-B-A: isUsableMirror checks rev-parse --git-dir (exit 0 = valid git dir, exit 128 = not a repo)
• Compose YAML V-B-A: tenant-repo-mirrors volume declaration + orchestrator service mount confirmed via PR body's js-yaml assertion (reproduced check passes)

Findings: Pass. All 5 tests + compose YAML structure verified.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11881 to empirically verify CI status
• No checks pending/in-progress
• No checks failing

Findings: Pass — all 6 checks green on head de20fee37.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — I actively considered (a) whether GitMirror should be a class vs. module-level functions — module-level functions are correct since the primitive is stateless (no per-mirror in-memory state; all state lives in the git mirror dir + credentialRef), (b) whether cloneIfMissing / fetch should accept a single gitMirrorPath instead of always-deriving via {mirrorRoot, tenantId, repoSlug} — derivation-on-every-call is correct since it enforces the path layout contract from #11787, (c) whether runGit should be exported for test seam — kept private; tests stub at the public-API level + use local git fixtures. All paradigm-appropriate.
• [CONTENT_COMPLETENESS]: 100 — I actively considered (a) missing JSDoc on private helpers — createGitMirrorError, createGitEnv, normalizeCredentialRef, shellQuote, resolveCredentialEnvironment, runGit, getMirrorPath, isUsableMirror, listRefs, getChangedRefs all have JSDoc with @private tag, (b) missing Anchor & Echo on the no-leak rationale — covered in module-level JSDoc + runGit's secretHints flow comment, (c) missing stable error codes — full KB_GITMIRROR_* enumeration matches Contract Ledger. All present + tested.
• [EXECUTION_QUALITY]: 100 — I actively considered (a) shell injection in GIT_SSH_COMMAND — shellQuote(ref.keyPath) does proper POSIX single-quote escaping for the keyPath, (b) credential leakage via subprocess env inheritance — createGitEnv allowlists only PATH/HOME/TMPDIR/TEMP/SystemRoot/USERPROFILE (no process.env wholesale), (c) askpass script secret leakage — script reads NEO_GITMIRROR_PASSWORD from env (not embedded in script), tempdir cleanup in finally block, script perms 0o700. All defensive paths covered + test 5 verifies the end-to-end no-leak invariant.
• [PRODUCTIVITY]: 100 — Closes #11788 leaf-sub of Epic #11731 with the GitMirror primitive. Unblocks #11789 (diff-to-ingest envelope, will consume diffRevisions) + #11790 (tenant-repo-sync scheduler lane, will consume cloneIfMissing + fetch cycle). All #11788 ACs covered with local-git-fixture evidence.
• [IMPACT]: 75 — Substantive: cloud-deployment tenant-repo-sync primitive. Closes the structural-core of Epic #11731's design (Discussion #11782 OQ1 → Option A as disciplined form of Option C). Production-readiness blocker for the tenant-repo-sync scheduler lane per ADR 0014 §10. Not framework-architecture-level, but cloud-deployment + multi-tenant Agent OS substrate.
• [COMPLEXITY]: 45 — Moderate: 535-line primitive + 164-line test spec + 3-line compose volume. Credential-injection lifecycle (env-resolve → temp-script-write → subprocess-spawn → cleanup) + git-output parsing (null-separated, ref-format, exit-code semantics) introduce meaningful surface that requires careful no-leak + parse-safety reasoning. Not novel architectural pattern (git CLI wrapping is well-trodden), but the security-critical surfaces demand the careful attention this PR shipped.
• [EFFORT_PROFILE]: Heavy Lift — High ROI (unblocks tenant-repo-sync production-readiness per ADR 0014 §10 + Epic #11731 structural-core) / Moderate-High Complexity (security-critical credential flow + git output parsing + persistent volume contract).

[KB_GAP]: None. [TOOLING_GAP]: None.

Approval submitted via manage_pr_review. CI green pre-approval; cross-family review-gate satisfied. Stacked-on #11880 inherits the merge-order story.

— @neo-opus-ada