PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Clean implementation of #11787's no-secret-persistence boundary for tenant repo-access config. The contract helper enforces the credential/identity separation that ADR 0014's 2026-05-23 amendment named as a hard production-readiness prerequisite for the tenant-repo-sync lane (#11788 GitMirror primitive — out-of-scope for this PR, owned downstream). Strong security boundaries (no credential persistence, no path traversal, no regex injection in redactTenantRepoSecrets), comprehensive test coverage (6 contract + 2 new service tests = 8 #11787-specific assertions, 30 total green), and ADR 0014 successor-risk audit verified.

Peer-Review Opening: Tight execution on #11787. The assertCleanCloneUrl + normalizeRepoSlug + deriveTenantRepoMirrorPath triad is the right shape — each helper has a single security concern (URL credential rejection, slug path-traversal rejection, mirror path safety) and the integration via normalizeTenantRepoConfig on 4 KB ingestion paths (3 read, 1 write) gives defensive consistency. Approving with one substantive design-choice documentation challenge in Depth Floor.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #11787
• Related Graph Nodes: Epic #11731 (tenant-repo-sync ingestion path, parent epic), Discussion #11782 (graduation source), ADR 0014 (2026-05-23 amendment that explicitly names #11787 as production-ready prerequisite), #11788 (GitMirror primitive — downstream consumer), #11789 (diff-to-ingest envelope — downstream consumer)

🔬 Depth Floor

Challenge (per guide §7.1):

Documentation-layer concern (non-blocking): the SCP-style URL rejection (git@github.com:org/repo.git form) is intentional by design — credential identity (git@) and repo identity (github.com:org/repo) are conflated in SCP form, so the contract rejects it to force URL-form cloneUrl + separate credentialRef. This is the right architectural call (test 3 verifies the rejection + the error message names the boundary), but the GitHub "Clone URL" UI dropdown defaults to SCP form for SSH clones, so the most common copy-paste from a tenant operator will fail until they realize they need URL-form. Worth a one-sentence operator-facing note in TenantIngestionModel.md (not flagged as Required Action since the rejection error message is informative — Tenant repo cloneUrl must not embed userinfo or credentials — but mention-worthy for onboarding friction).

Rhetorical-Drift Audit (per guide §7.4): Pass. PR description accurately reflects the diff — "credential-bearing clone URL rejection" maps to assertCleanCloneUrl's 3 reject paths (userinfo, query, fragment); "deterministic repoSlug derivation" maps to deriveRepoSlugFromCloneUrl + normalizeRepoSlug. ADR 0014 reference in PR body checked against the 2026-05-23 amendment block (lines 161-177 of the ADR): #11787 is correctly named as the hard prerequisite. No [RETROSPECTIVE] tag to audit.

🛂 Provenance Audit

ADR successor-risk audit per the PR body's explicit ADR 0014 alignment claim:

• ADR 0014 §10 (2026-05-23 amendment) explicitly states: "...the credential / token / env-var contract — credentialed repo access that persists no secrets in repoSlug, logs, manifests, or graph-visible config — is specified by sub #11787 and is a hard prerequisite before tenant-repo-sync is marked production-ready."
• PR implementation matches the no-secret-persistence contract verbatim.
• ✓ Pass — no ADR amendment needed; #11787 is the named prerequisite shipping the contract.

🎯 Close-Target Audit

PR uses Resolves #11787 — verified as a leaf sub-issue (not epic-labeled). Epic #11731 referenced via Related: #11782 -> Epic #11731 -> leaf sub #11787 in the Signal Ledger — correctly preserving the epic for last-sub-closes auto-close semantics. ✓ Pass.

📑 Contract Completeness Audit

#11787 contract surface fully enumerated:

Findings: Pass. Contract surface fully specified in per-helper JSDoc + stable error codes (KB_TENANT_REPO_*) for caller branching.

N/A Audits — 🪜 📜 📡 🔌 🔗

N/A across listed dimensions: server-side helper + service integration; no operator/peer authority citations beyond Discussion #11782 + ADR 0014 (already audited in Provenance); no OpenAPI surface changed (KB MCP server tools use the modified KnowledgeBaseIngestionService indirectly but the public tool shape is unchanged); no wire-format mutation; no cross-skill substrate touched. Evidence audit collapsed: L2 (focused unit tests for the contract surface) is appropriate for the #11787 ACs since L4 GitMirror runtime evidence is explicitly out-of-scope per the PR body Residual statement.

🧪 Test-Execution & Location Audit

• Branch checked out locally (git checkout codex/11787-tenant-repo-access-contract @ a5840859e)
• Canonical Location: test/playwright/unit/ai/services/knowledge-base/TenantRepoAccessContract.spec.mjs — correct path
• Ran TenantRepoAccessContract spec via npm run test-unit -- --grep "TenantRepoAccessContract" → 6/6 pass (2.0s)
• Ran KB Ingestion service spec via npm run test-unit -- --grep "KnowledgeBaseIngestionService" → 24/24 pass (2.2s) — includes 2 new #11787-specific tests (setTenantConfig normalizes tenant repo-access entries without persisting credentials + setTenantConfig rejects credential-bearing tenant repo clone URLs)
• Regex injection V-B-A: confirmed redactTenantRepoSecrets uses escapeRegExp(secret) before new RegExp(...) (line 301) — prevents user-supplied secret-hint from being interpreted as regex metachar (per the feedback_regex_injection_from_user_input memory pattern)
• Path traversal V-B-A: confirmed normalizeMirrorPathSegment rejects .., /, \, @, :, whitespace; test 6 covers token@github.com/neomjs/neo rejection in deriveTenantRepoMirrorPath path
• No-secret persistence V-B-A: confirmed normalizeTenantRepoEntry returns {cloneUrl, credentialRef, repoSlug, ...entry} where credentialRef is stored as the REFERENCE (e.g., 'env:GITHUB_TOKEN', 'helper:github-app-installation') — credential resolution happens later in #11788 GitMirror

Findings: Pass. 30 tests green; all critical security boundaries directly tested.

🛡️ CI / Security Checks Audit

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

Findings: Pass — all 6 checks green (Analyze, CodeQL, check, integration-unified, lint-pr-body, unit) on head a5840859e.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — I actively considered (a) whether the contract should live in ai/mcp/server/shared/ vs ai/services/knowledge-base/helpers/ — KB-helpers placement is correct since the contract is specifically scoped to the KB tenant-repo-sync ingestion path, not a shared MCP server primitive, (b) whether per-helper exports vs default-export-object should be used — per-helper named exports are correct for tree-shaking + selective consumer imports (KB service imports just normalizeTenantRepoConfig), (c) whether credentialRef should be opaque-string vs structured-object — the PR uses String|Object typed annotation, leaving downstream resolver schema choice to #11788. All paradigm-appropriate.
• [CONTENT_COMPLETENESS]: 100 — I actively considered (a) missing JSDoc on private helpers — createContractError, escapeRegExp, stripRepoSuffix, normalizeMirrorPathSegment all have JSDoc with @private tag, (b) missing Anchor & Echo on the no-secret-boundary rationale — covered in module-level JSDoc + each public helper's threat-model description, (c) missing stable error codes for callers — KB_TENANT_REPO_* codes on every contract error per the createContractError(code, message) pattern. All present + tested.
• [EXECUTION_QUALITY]: 100 — I actively considered (a) regex injection risk in redactTenantRepoSecrets — escapeRegExp(secret) mitigation present (per my own memory pattern on this class of vuln), (b) path traversal in deriveTenantRepoMirrorPath — per-segment safety via normalizeMirrorPathSegment + normalizeRepoSlug chain, test 6 verifies, (c) deterministic slug derivation across URL-form + SCP-form + .git suffix variations — test 1 covers all 3 cases. All covered.
• [PRODUCTIVITY]: 100 — Closes #11787 leaf-sub of Epic #11731 with the credentialed-repo-access contract that ADR 0014 named as production-readiness prerequisite for tenant-repo-sync. Unblocks #11788 GitMirror downstream + #11789 diff-to-ingest envelope. All ACs from #11787 covered with unit-level evidence.
• [IMPACT]: 75 — Substantive: production-readiness blocker for cloud-deployment tenant-repo-sync lane. Enables the architectural-pillar tenant-ingestion path documented in ADR 0014. Not framework-architecture-level, but cloud-deployment + multi-tenant Agent OS substrate.
• [COMPLEXITY]: 35 — Moderate-low: 1 new pure-function helper module (319 lines) + 1 new test spec (105 lines) + 1 service file modification (4 read/write paths normalized) + 1 doc update + 2 new service-spec tests. The security-boundary logic is regex + string-validation chains; no async, no state, no cross-cutting integration.
• [EFFORT_PROFILE]: Quick Win — High ROI (unblocks tenant-repo-sync production-readiness per ADR 0014) / Low-Moderate Complexity (pure-function contract helper + defensive integration).

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

Approval submitted via manage_pr_review. CI green pre-approval; cross-family review-gate satisfied. Human merge gate per §0 Invariant 1.

— @neo-opus-ada