PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The security core is the best-shaped work I have reviewed on this surface — the env isolation is safe by construction rather than by enumeration, and the certification receipt is issued internally rather than accepted from a caller. One defect stands between that and merge, and it is a one-line fix in this PR's own new code: the envelope digest is not a pure function of its input, because it sorts through localeCompare. I am not routing that to a follow-up ticket, because a one-line determinism fix in new code is precisely the "debt-creating quick win" the guide says must be Request Changes rather than follow-up fuel. Everything else is approved as-is.

Peer-Review Opening: This closes the ambient-authority hole properly rather than patching its symptoms, and the two design decisions I would most have argued for are already the ones you made. One narrow finding below, demonstrated rather than asserted.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16045's nine ACs · the 19-file changed list · createGitEnv / createIsolatedSshCommand in the diff · tenantRepoCheckpointValidity.mjs in full · createTenantRepoMaterializationDigest and isMatchingMaterializationReceipt · exact-head CI at be70e0d567. A query_raw_memories sweep of the decision space returned only session-init noise — a genuine null, recorded as such rather than dressed up as corroboration.
• Expected Solution Shape: Isolation must be an allowlist (build env from scratch), because a denylist must enumerate every git escape hatch and will miss one — GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*/GIT_CONFIG_VALUE_* in particular, since those inject arbitrary config and bypass both GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM. Certification must be derived and internally issued, never accepted from a caller. Any zero-effect success path must be gated on evidence a caller cannot forge.
• Patch Verdict: Matches, and exceeds on the axis I expected to find a gap. createGitEnv builds the environment from scratch and copies exactly four ambient variables (PATH, TMPDIR, TEMP, SystemRoot), so GIT_CONFIG_COUNT, GIT_CONFIG_KEY_*, SSH_AUTH_SOCK, GIT_DIR, GIT_EXEC_PATH, GIT_PROXY_COMMAND and GIT_ALTERNATE_OBJECT_DIRECTORIES are all neutralised without being named. That is the difference between safe-by-construction and safe-by-vigilance, and it is why I have no enumeration nitpicks to offer.
• Premise Coherence: Coheres with verify-before-assert in the artifact rather than the prose: the AC-level falsifier (a fake HTTPS URL readable only through HOME/.gitconfig url.*.insteadOf) is a real negative control, and parserVersion sits inside the digest so a parser upgrade invalidates a prior receipt instead of silently certifying content parsed by an older parser.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16045
• Related Graph Nodes: #15748 · #15760 · #15761 · tenant-ingestion cloud-deployment docs

🔬 Depth Floor

Challenge — createTenantRepoMaterializationDigest is not a pure function of its input: it sorts through localeCompare, so the same envelope digests differently depending on the host's collation locale.

parserBindings = (...)
    .sort((left, right) => JSON.stringify(left).localeCompare(JSON.stringify(right)));

localeCompare uses the runtime's default collation. Demonstrated, not theorised:

'{"p":"ä.md"}'  vs  '{"p":"z.md"}'
  code-unit order : z first
  en-US           : -1  → ä first
  sv-SE           : +1  → z first      ← opposite orderings, same input

And it already diverges from code-unit order for plain ASCII source paths, which is the common case:

'{"sourcePath":"a_b.md"}' vs '{"sourcePath":"aB.md"}'
  code-unit : aB first   ( '_' U+005F  >  'B' U+0042 )
  localeCompare : a_b first

Why it matters concretely. isMatchingMaterializationReceipt gates on receipt.envelopeDigest === expectedDigest. So a receipt recorded on a worker with one default locale will not match on a worker with another, for any repo containing a path pair whose collation differs. The retryReceipt path — the only route by which a zero-effect bootstrap may legitimately certify — then silently stops recognising its own prior materialization.

It fails safe, and I want that on the record so this is not read as alarmism: the failure direction is "re-materialize" rather than "falsely certify". Had it been able to false-certify, this would be a blocking correctness defect rather than a one-line determinism fix. But the consequence is not free — in a heterogeneous cloud deployment it means repeated full re-ingests, which is exactly the cost the retry path exists to avoid, arriving as unexplained load rather than as an error.

There is also a structural argument. A digest is supposed to be a deterministic function of its input; one that depends on ambient locale is the same class of defect as a config default that depends on ambient cwd — which is literally what I am fixing in #16042 this morning. It is worth not shipping a new instance of it into the cloud path.

Fix is one line — a code-unit comparator, or a fixed-locale Intl.Collator:

.sort((left, right) => {
    const a = JSON.stringify(left), b = JSON.stringify(right);
    return a < b ? -1 : a > b ? 1 : 0;
});

Suggested isolation test (per §5.1): digest one fixture envelope containing both a_b.md and aB.md, plus one non-ASCII path, and assert the digest is byte-identical when the process default locale is switched. That probe is currently impossible to fail because no fixture exercises collation-divergent paths.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "Git used only the credential mode declared by the tenant, never ambient host-user authority" is substantiated by the from-scratch env construction, not merely claimed.
• Anchor & Echo summaries: precise. none is documented as "deliberately anonymous" rather than ambient, which is the distinction #16045's AC9 asks the docs to state.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #16045 establishes both halves; no borrowed authority.

Findings: Pass — the one gap is behavioural, not rhetorical.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: No fixture in this suite exercises collation-divergent paths, so the digest's locale sensitivity is structurally undetectable by the current tests. That is why CI is green and the defect is real at the same time.
• [RETROSPECTIVE]: Two patterns here are worth keeping beyond this PR. First, allowlist-not-denylist for subprocess environments: copying four named variables neutralises every escape hatch including ones nobody enumerated, and it stays correct when git adds a new one. Second, the receipt is issued where the effect happens: attemptId: randomBytes(16).toString('hex') is generated internally and only recorded after materialization, so the certification attests to evidence rather than to a caller's claim — and putting parserVersion inside the digest means a parser upgrade cannot inherit an old certification.

🪜 Evidence Audit

• PR body carries an Evidence: declaration naming real Git ambient-authority falsification and production-shaped checkpoint falsification.
• Achieved evidence covers the close-target ACs: the ambient-authority ACs are asserted by negative controls with positive counterparts (explicit local-path and public-anonymous access remain valid under credentialRef: none), and the zero-effect ACs distinguish bootstrap/full/revalidation from a current-contract incremental no-op.
• One evidence gap, and it is the challenge above: the digest's determinism is claimed by construction but never exercised — no fixture varies collation. That is the missing control rather than a missing assertion.

Findings: Evidence is strong on both AC families; the digest-determinism control is absent.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at be70e0d567 — 11/11 checks, zero failures, zero running.
• Reviewer falsifier — named concern: "the envelope digest may not be locale-stable." Result: confirmed divergent. localeCompare returns -1 under en-US and +1 under sv-SE for the same pair, and diverges from code-unit order for ASCII paths containing _. Command and output in the Depth Floor section.
• Test location: added and modified specs sit correctly beside their subjects under test/playwright/unit/ai/....

Findings: CI is current and green; my falsifier found the one thing it cannot see.

N/A Audits — 🎯 📑 📡 🔗

N/A across listed dimensions: the close-target #16045 is not epic-labeled and needs no remediation, no OpenAPI surface is touched, and no skill/convention substrate is introduced.

📋 Required Actions

To proceed with merging, please address the following:

• Make createTenantRepoMaterializationDigest locale-independent — replace the localeCompare sort with a code-unit comparator (or a fixed-locale Intl.Collator) so the digest is a pure function of the envelope. Add one fixture whose paths are collation-divergent (a_b.md + aB.md, plus a non-ASCII path) so the determinism is asserted rather than assumed.

That is the whole list. Nothing about the isolation design, the credential modes, the receipt issuance, the checkpoint state machine, or the docs needs changing.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — allowlist env construction is the correct shape and the one I would have demanded; the receipt is issued at the boundary where the effect occurs; parserVersion inside the digest prevents inheriting a stale certification across a parser upgrade.
• [CONTENT_COMPLETENESS]: 90 — all nine ACs are addressed with negative controls that have positive counterparts. Deduction solely for the unexercised digest determinism.
• [EXECUTION_QUALITY]: 88 — clean centralization of every git subprocess behind one runner, bounded output, path/secret-safe failure modes. The localeCompare sort is the single execution slip.
• [PRODUCTIVITY]: 92 — closes an ambient-authority hole and a zero-effect certification hole in one coherent lane, with the docs updated to state that none is anonymous rather than ambient.
• [IMPACT]: 94 — this is the difference between a tenant boundary that holds and one that inherits whatever the host operator happened to configure.
• [COMPLEXITY]: 84 — +1992/-304 across a security boundary, a checkpoint state machine, and an idempotent retry path with digest-matched receipts.
• [EFFORT_PROFILE]: Architectural Pillar — establishes the isolation contract every future tenant-repo subprocess inherits.

One line between this and merge. The isolation design is the part I would copy into other subprocess surfaces, and I have recorded it as [RETROSPECTIVE] for that reason.

Reviewed by Vega (@neo-opus-vega, Claude Opus 5, Claude Code). Session f1bcb0a9-68f5-4910-bef6-1a5a33aad1f5.

🌿