Skip to content

Roadmap item 4: Add inactive local Git materializer - #229

Merged
yihanzhu merged 23 commits into
mainfrom
codex/local-git-materializer-v1
Sep 4, 2026
Merged

Roadmap item 4: Add inactive local Git materializer#229
yihanzhu merged 23 commits into
mainfrom
codex/local-git-materializer-v1

Conversation

@yihanzhu

@yihanzhu yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add the inactive runtime for core.forge.materialize-candidate.v2.
  • Import an exact sanitized bare source into a caller-disposable bare candidate repository.
  • Apply one contract-bound text patch through a scratch-only index.
  • Return the reviewed protocol's verified receipt and core-valid stage result.

Scope

Tracks Roadmap item 4 without closing an intake issue. This is the runtime unit after protocol PR #228. The accepted main protocol is unchanged byte-for-byte; this PR adds only runtime code, its focused test, restore entries, and synchronized docs. Default-profile assembly follows after this runtime has a durable main identity.

Safety

The runtime is inactive and local-only. It rejects worktrees, alternates, shallow or partial repositories, replace/graft state, hooks, filters, remotes, unsafe paths, binary patches, symlinks, submodules, and overlapping or non-private boundaries. It receives no credential or network access and cannot push, publish, merge, install, activate a profile, contact a provider, or use a real target during construction.

Targeted proof

  • runtime materialization matrix: 27/27 across SHA-1/SHA-256 and adversarial repository/path cases
  • accepted protocol file is byte-identical to main
  • Bash syntax, ShellCheck 0.11.0, rename, and diff checks pass

The preserved runtime branch was adopted through the operator-approved merge topology. Relative to current main the PR contains exactly five runtime concern paths; it was not rebased or force-pushed.

ci added 2 commits September 2, 2026 15:56
…cal-git-materializer-v1

# Conflicts:
#	README.md
#	RESTORE.md
#	adapters/local-git-materializer/v1/protocol.jq
#	ci/required-files.txt
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: fc92725
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-45095/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The main flow is coherent, but failure atomicity is broken when response delivery fails, and two documented source-sanitization states can bypass the current filesystem checks.

Full review comments:

  • [P2] Mark success only after writing the response — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.x3x67QLAVX/adapters/local-git-materializer/v1/materialize.sh:393-394
    If stdout becomes unwritable, such as when the caller closes a pipe early, cat returns nonzero and the adapter exits as failed, but success is already 1 so cleanup preserves repository.git. A retry with the same candidate root then fails E_CANDIDATE_ROOT, leaving a candidate without a complete receipt; mark success only after the response write succeeds.

  • [P2] Reject replacement refs stored in packed-refs — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.x3x67QLAVX/adapters/local-git-materializer/v1/materialize.sh:235-235
    When a refs/replace/* entry exists only in packed-refs, the loose refs/replace directory can be absent and this check accepts the repository despite the documented replacement-state rejection. GIT_NO_REPLACE_OBJECTS keeps the ref inert during these commands, but the source still violates the adapter's sanitization invariant; inspect logical replacement refs or packed-refs as well.

  • [P2] Reject linked-worktree metadata in bare common directories — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.x3x67QLAVX/adapters/local-git-materializer/v1/materialize.sh:230-232
    A bare repository with attached linked worktrees stores their metadata under $GIT_DIR/worktrees/; its common directory has no commondir file and still reports itself as bare, so it passes these checks. This contradicts the advertised worktree rejection, and the current negative test covers only an ordinary non-bare repository rather than this case.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 3, 2026

Copy link
Copy Markdown

Deploying ystack with  Cloudflare Pages  Cloudflare Pages

Latest commit: da15ae0
Status: ✅  Deploy successful!
Preview URL: https://c0b1cb1c.fabrica-6yx.pages.dev
Branch Preview URL: https://codex-local-git-materializer-f7ju.fabrica-6yx.pages.dev

View logs

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 2bab371
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-91621/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer violates its scratch-write boundary and contains a newline-based allowed-path bypass that can produce a successful but false receipt. It also processes unbounded raw input before applying schema limits.

Full review comments:

  • [P1] Route core validation through the supplied scratch root — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Z4AK86TQ2l/adapters/local-git-materializer/v1/materialize.sh:158-160
    On every invocation, these unaccounted core-contract.sh calls create validation state under /tmp/ystack-portable-core-ingress.*, and the later stage-result validation does the same. This writes outside the caller-provided scratch boundary and makes otherwise valid materializations fail in sandboxes where only scratch_root is writable; use the core's accounted-validation mode with storage under the supplied scratch root.

  • [P1] Reject newline bytes before line-based path accounting — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Z4AK86TQ2l/adapters/local-git-materializer/v1/materialize.sh:259-261
    When a source filename contains an LF, grep '[[:cntrl:]]' does not see the record-separating newline, so this check accepts it. A patch changing foo\nbar is subsequently recorded as two paths, foo and bar; if both appear in allowed_paths, the scope check passes and the receipt reports the wrong path count and digest even though neither allowed path is the file that changed.

  • [P2] Bound the raw input before copying and parsing it — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Z4AK86TQ2l/adapters/local-git-materializer/v1/materialize.sh:120-125
    When input_path is a very large malformed regular file, the adapter copies it and asks jq to parse and sort the entire file before any protocol payload limits are enforced. A caller-controlled input can therefore exhaust scratch space or jq memory/CPU despite the bounded schema; reject inputs above a conservative outer-envelope byte limit before copying or parsing them.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 52fa387
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-19488/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer rejects some inputs accepted by its protocol and does not fully enforce its documented repository-path safety boundary. Both issues affect valid or adversarial repository inputs.

Full review comments:

  • [P2] Support all contract-valid commit timestamps — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.g3E0gjADYI/adapters/local-git-materializer/v1/materialize.sh:356-356
    When a changed request uses a valid attempt.started_at outside Git's accepted date range, such as year 2100, the protocol accepts it because schema::time_ok allows years 0000–9999, but git commit-tree rejects GIT_AUTHOR_DATE and returns E_CANDIDATE_GIT. The materializer should use a deterministic Git-compatible timestamp or reject unsupported dates during contract validation.

  • [P2] Reject non-ASCII control characters in tree paths — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.g3E0gjADYI/adapters/local-git-materializer/v1/materialize.sh:276-276
    Because the script forces LC_ALL=C, [[:cntrl:]] only catches ASCII control characters; UTF-8 C1 controls such as U+0080 and malformed non-ASCII byte sequences pass this check. A source tree containing such a filename is therefore copied into the candidate despite the protocol's path_ok policy and documentation claiming unsafe paths are rejected.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: c143e07
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-47552/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer leaves its largest untrusted write unbounded and allows host Git templates to contaminate the supposedly sanitized candidate repository. Both issues undermine the adapter's safety guarantees.

Full review comments:

  • [P1] Bound the reachable-object import — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nj2vBf6K19/adapters/local-git-materializer/v1/materialize.sh:312-316
    When the selected commit has a large reachable history, even a tiny valid patch causes pack-objects to stream the entire history into scratch and index-pack to duplicate it in the candidate repository without any byte quota. An untrusted or simply very large source can therefore exhaust disk or stall the worker despite the input and core validations being bounded; enforce a caller-provided or fixed budget while importing objects.

  • [P2] Initialize from an explicit empty Git template — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nj2vBf6K19/adapters/local-git-materializer/v1/materialize.sh:309-310
    On a host whose compiled default Git template directory contains custom hooks, refs, alternates, or other files, git init copies them into the staging repository. The temporary core.hooksPath override only prevents hooks during this invocation and is not persisted, so the returned candidate can contain active hooks or extra state despite the adapter's sanitization guarantees; initialize with a known empty template.

@yihanzhu yihanzhu added the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Construction review cap reached on exact head c143e07145a2d2d0eccdad99899a49b2a5b3860e / base 150568f05853b834311a61596e82dfb91c043584. The final independent review found two unresolved Important issues: (1) reachable-object import has no byte quota; (2) git init can inherit host template files. No code changed after that verdict.

Operator ruling requested: authorize one strictly bounded final revision on this same PR to add a fixed reachable-object import budget with an over-budget regression test, and initialize from an explicit empty scratch template with a host-template contamination regression test. The PR remains limited to its existing five runtime paths; no authority, activation, credential, network, release, install, or deployment scope changes. After authorization, the same branch will rerun targeted tests, exact CI, and one final independent review before any squash merge.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Operator ruling received directly in the active coordinator session on 2026-09-03: authorize the exact two-item bounded revision proposed in comment 5529174878. This ruling covers only a fixed reachable-object import budget plus regression proof, and an explicit empty Git template plus regression proof, on the same branch and existing five-path PR scope. It does not waive targeted tests, fresh exact CI, final independent review, or publisher receipt.

@yihanzhu yihanzhu removed the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: e1f9335
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-76706/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The implementation makes the protocol's no-change outcome unavailable for the natural empty-patch case. Its path validation also permits severe process amplification for repositories that remain within the advertised import budgets.

Full review comments:

  • [P1] Preserve the supported no-change path — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.OvWFC7Ex8D/adapters/local-git-materializer/v1/materialize.sh:201-201
    When a producer supplies an empty text/x-diff payload to represent no changes, this check rejects it before the existing equal-tree logic can emit the protocol's explicit no-change result. The protocol permits zero-length payload data, and ordinary nonempty Git patches change the tree, so the intended no-change path is effectively unavailable; accept an empty patch and bypass Git apply, or invoke it with empty-patch support.

  • [P2] Validate repository paths without per-component processes — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.OvWFC7Ex8D/adapters/local-git-materializer/v1/materialize.sh:287-287
    For repositories containing many files or deeply nested paths, this starts an external tr process for every path component, in addition to several processes per path, and the entire source and candidate trees are both scanned. The object-count limit does not bound tree entries because many entries can reference the same blob, so a source within the documented object and byte budgets can still launch hundreds of thousands or millions of processes; validate the path stream in one bounded process or cap entries/components before this loop.

@yihanzhu yihanzhu added the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

The operator-authorized final review completed on exact head e1f93354433bc420aad22bb89f67bf33eb07930b / base 150568f05853b834311a61596e82dfb91c043584, but it is not clean. Review comment 5530330241 found two new unresolved Important issues: (1) zero-length producer patches are rejected, making the core-supported no-change result unavailable; (2) path validation launches external processes per path component, allowing severe process amplification within the current object/byte budget. No code changed after this verdict.

Operator ruling requested: authorize one further strictly bounded correction on this same PR to accept an empty patch by skipping Git apply and prove a canonical no-change result, and replace per-component external path checks with a single bounded validation pass (with amplification regression proof). Keep the existing five-path scope and all inactivity/authority restrictions. Fresh targeted tests, exact CI, and a final independent review remain required before merge.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Operator ruling received directly in the active coordinator session on 2026-09-03: authorize the exact two-item correction proposed in comment 5530340949. This covers only empty-patch no-change support and bounded path validation without per-component external processes, with regression proof, on the same branch and existing five-path scope. It does not waive fresh targeted tests, exact CI, final independent review, or publisher receipt.

@yihanzhu yihanzhu removed the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 8eae999
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-35785/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The hidden worker command bypasses the adapter's environment-cleaning boundary. The patch also asserts proof for resource-limit behavior that its focused test never exercises.

Full review comments:

  • [P2] Reject direct entry to the cleaned worker mode — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.zDPCgxBYVg/adapters/local-git-materializer/v1/materialize.sh:13-17
    When the executable is invoked with __materialize_clean, it skips the env -i re-exec and proceeds with the caller's full environment. Exported Bash functions can then override unqualified commands such as find, id, or uname; for example, overriding find to return no output bypasses the empty-directory and source-symlink checks. Treat this internal mode as inaccessible from the public command line or otherwise guarantee the same environment sanitization.

  • [P2] Exercise the advertised resource-limit branches — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.zDPCgxBYVg/RESTORE.md:523-526
    The test does not prove two limits claimed here: the oversized-blob fixture exceeds the byte budget but has only a few reachable objects, while the 65,537-path fixture reuses one blob and only exercises the tree-entry limit. No fixture exceeds 65,536 reachable objects, and none stays within that count while producing a listing over 64 MiB, so those new enforcement branches can regress while CI remains green. Add focused fixtures before claiming this restore proof.

@yihanzhu yihanzhu added the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

The final independent review on exact head 8eae999a27ba907223a49e55c16d84fc1b2815f9 / base 150568f05853b834311a61596e82dfb91c043584 is not clean. Review comment 5531358564 found two new unresolved Important issues: (1) direct __materialize_clean entry bypasses the public environment-cleaning re-exec; (2) focused tests do not exercise the advertised 65,536 reachable-object limit or 64 MiB tree-listing limit. No code changed after this verdict.

Operator ruling requested: authorize one strictly bounded correction on this same PR to guarantee environment sanitization for every worker entry with a hostile exported-function regression, and add focused fixtures that cross the reachable-object-count and tree-listing-byte limits (or narrow any proof wording that cannot be exercised honestly). Keep the existing five-path scope and all inactivity/authority restrictions. Fresh targeted tests, exact CI, and a final independent review remain required before merge.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Standing operator ruling received directly in the active coordinator session on 2026-09-03. For PR #229, the coordinator may resolve all further independent-review Important findings without repeated approval only while the work remains the same local Git materializer concern, stays within the existing five PR paths, and does not expand architecture, authority, permissions, credentials, network, activation, release, install, deployment, or external-write scope. Every new head still requires targeted tests, fresh exact app-15368 CI, and a complete independent exact-head/base review with no unresolved Important before publisher squash merge. Any scope or safety-model change, genuine review disagreement, identity mismatch, or evidence gap restores needs-human.

@yihanzhu yihanzhu removed the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 3, 2026
@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 62b39cc
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-11329/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer's stated input bounds do not bound candidate data written during patch application. A compact, contract-valid input can therefore cause severe disk amplification before the post-apply checks run.

Review comment:

  • [P1] Enforce candidate size limits before applying patches — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Tt4HTTznQx/adapters/local-git-materializer/v1/materialize.sh:387-391
    A source tree can reference the same near-256 MiB blob from many paths, so it passes the unique-object import budget, while a small patch makes distinct edits to each path. These calls then write a separate near-256 MiB blob for every edit before max_changed_paths and scope are checked, allowing an accepted input to create tens of gigabytes and exhaust the filesystem. Preflight the affected paths and cumulative output size, or enforce a hard candidate-repository quota before the mutating apply.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: f25170e
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-86563/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer can exceed its candidate-write resource bound through copy fan-out and can silently lose valid empty tree entries outside the materialization contract. Both violate core safety and correctness guarantees of the new adapter.

Full review comments:

  • [P1] Preserve copy multiplicity in the mutation budget — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.J4eojVhf7B/adapters/local-git-materializer/v1/materialize.sh:392-392
    When a patch copies one large source file to multiple destinations and slightly modifies each copy, sort -u collapses the repeated source path before blob sizes are summed. The preflight therefore counts the source blob once even though git apply writes one similarly sized blob per destination, allowing a contract-valid patch to expand far beyond the stated 256 MiB candidate budget; account for each copy destination's source bytes before applying.

  • [P1] Reject empty subtrees before index round-tripping — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.J4eojVhf7B/adapters/local-git-materializer/v1/materialize.sh:310-311
    For a valid bare source containing an explicit empty subtree, ls-tree -r emits no record for that subtree, so this scan accepts it. The subsequent index round-trip drops the empty tree, while recursive diff-tree also omits that directory; with an otherwise allowed file patch, materialization can therefore succeed after silently changing the tree outside allowed_paths, and an empty patch fails to produce the promised no-change result.

@yihanzhu

yihanzhu commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: f085b18
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-64409/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

Several resource limits are enforced only after unbounded reads or parsing have already occurred. A malicious or concurrently modified local input can therefore exhaust memory, CPU, or scratch space despite the adapter's documented bounds.

Full review comments:

  • [P1] Reject oversized Git objects before walking them — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uVaLVZE2te/adapters/local-git-materializer/v1/materialize.sh:365-366
    When an untrusted source contains a highly compressed oversized commit or tree, scan_tree invokes git ls-tree before the 256 MiB object-size accounting below. Git inflates and parses the tree before head can limit stdout, so such an object can exhaust memory before being rejected. Preflight object sizes and walk trees through a bounded mechanism before parsing them.

  • [P1] Bound source config before parsing it — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uVaLVZE2te/adapters/local-git-materializer/v1/materialize.sh:276-278
    When the supplied bare repository has a very large but valid config, such as millions of repeated allowlisted keys or one huge value, this command parses all of it and can write an unbounded source-config file. The filesystem inventory limits only encoded path names, not file contents, so a malicious source can exhaust scratch space or CPU before rejection; cap the config input and generated listing before processing them.

  • [P2] Bound the input snapshot copy itself — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uVaLVZE2te/adapters/local-git-materializer/v1/materialize.sh:137-138
    If the regular input file is appended to after the initial wc check, cp continues copying past the validated 8 MiB limit and the oversized snapshot is then handed to jq. A concurrent producer can therefore bypass the input resource bound and consume unbounded scratch space; copy through a limit-enforcing stream and reject a limit-plus-one result.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ab26997
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-32246/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer rejects a valid class of text patches that replace a directory with a regular file, despite the contract not excluding such transitions.

Review comment:

  • [P2] Allow directory-to-file patch replacements — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Td9Qmfn51I/adapters/local-git-materializer/v1/materialize.sh:477-480
    When a valid patch deletes a subtree such as dir/file and replaces it with a regular file at dir, git apply --numstat includes dir, and this lookup resolves the existing tree object. Rejecting every non-blob therefore returns E_PATCH_PATH before Git can apply the otherwise contract-valid directory/file transition. Treat an existing tree at the replacement path as zero source-blob bytes; the deleted descendant paths account for their own blobs.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3885f5b
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-84074/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer enforces several limits only after Git has already parsed source-controlled data. Oversized historical objects and packed refs can therefore cause resource exhaustion despite the documented bounds.

Full review comments:

  • [P1] Bound ancestor objects before recursive traversal — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.CszFF0zSaj/adapters/local-git-materializer/v1/materialize.sh:417-420
    For a source whose selected tip and tree are small but whose parent commit or historical tree expands beyond the 256 MiB limit, rev-list --objects recursively loads that oversized object before the later cat-file size accounting runs. A compressed ancestor can therefore exhaust memory or CPU before returning E_SOURCE_LIMIT; walk commits and trees non-recursively with per-object size checks before parsing them.

  • [P2] Cap packed refs before enumerating replacements — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.CszFF0zSaj/adapters/local-git-materializer/v1/materialize.sh:314-315
    For a bare source with a very large or unsorted packed-refs containing unrelated refs, the filesystem inventory counts only one filename while for-each-ref may parse the entire unbounded file to find replacement refs. This bypasses the stated source resource bounds even when the selected commit has tiny reachable history; snapshot and size-cap packed-refs before this check or use a bounded replacement-ref check.

@yihanzhu yihanzhu added the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 4, 2026
@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Standing ruling boundary reached. Review comment 5534548633 requires proving every historical commit/tree size before recursive Git traversal. The current five-file shell runtime cannot do that safely: rev-list parses ancestors before the limit, while implementing Git loose/pack/delta parsing in shell would create a new unsafe parser. The already-started packed-refs fix remains preserved locally and uncommitted; no branch head changed after the review.

Operator decision requested: authorize the smallest architecture expansion for a separately named bounded object-closure helper, with its own regression tests, durable exception rationale, fixed removal/re-evaluation condition, and manifest/docs updates; or direct us to defer the local materializer runtime and continue the Roadmap with an inactive unsupported forge binding. Until that decision, #229 stays needs-human and will not merge.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Operator authorization received directly in the active coordinator session on 2026-09-04: choose the helper-expansion option from comment 5535343923. Scope expands only by adapters/local-git-materializer/v1/object-closure.c plus its entry in the existing restore manifest/docs/test paths. The helper is private to the materializer and may only enumerate the selected Git object closure with fixed count/byte/object-size bounds before recursive Git parsing. It exposes no reusable API, authority, credential, network, activation, release, install, deploy, or external-write capability.

Exception lifecycle: the external invariant is that supported Git CLI traversal parses ancestor commits/trees before the caller can enforce per-object resource limits. Re-evaluate and remove the helper when the supported Git interface provides pre-parse per-object bounds or an accepted sandbox boundary supplies equivalent hard memory/CPU/disk enforcement. The helper must not be copied elsewhere without a separate decision. Existing uncommitted packed-refs work remains part of this same authorized correction. Fresh focused tests, exact CI, independent review, and publisher receipt remain mandatory.

@yihanzhu yihanzhu removed the needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure label Sep 4, 2026
@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 2df4dd2
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-70638/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer's packed replacement-ref check can be bypassed by valid uppercase hexadecimal object IDs, violating its fail-closed source validation.

Review comment:

  • [P2] Reject uppercase packed replacement OIDs — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.0HMFS2SlNf/adapters/local-git-materializer/v1/materialize.sh:329-330
    When packed-refs represents a replacement ref with uppercase A-F in its object ID, Git still accepts the ref, but this lowercase-only regex misses it. Such a source therefore passes the promised replace-state preflight; make the hexadecimal match case-insensitive or include uppercase digits.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 8459a0a
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-28186/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The helper can either fail under a stricter execution boundary or relax its inherited resource controls. This violates the adapter's sandbox assumptions while it processes untrusted Git data.

Review comment:

  • [P1] Preserve inherited resource ceilings — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.C7NwtEYUyK/adapters/local-git-materializer/v1/object-closure.c:40-46
    When an execution boundary starts this helper with stricter resource limits, these assignments overwrite both the soft and hard limits with fixed values. If the inherited hard limit is lower, setrlimit fails and every materialization exits with status 126; if only the soft limit is lower, the helper raises it and weakens the caller's sandbox while processing an untrusted repository. Read the inherited limits and only lower them when they exceed the adapter's ceilings.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 6b6664d
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-83027/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The path-overlap guard fails for a root source directory, allowing writes within the source boundary before rejection. This breaks an explicit isolation guarantee.

Review comment:

  • [P2] Reject descendants when the source directory is root — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.ELKIAu3QVT/adapters/local-git-materializer/v1/materialize.sh:104-106
    When source_git_dir is /, appending / produces //, so neither pattern matches candidate or scratch directories such as /candidate. The adapter then creates scratch state inside the source repository before a later inventory check fails, violating the source/candidate/scratch separation and source-unchanged guarantees. Handle / explicitly or use an ancestry check that works for root paths.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 7239500
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-38455/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer's sanitization boundary still trusts and forwards an inherited executable search path. In affected environments this permits arbitrary command execution and invalidates the adapter's safety checks.

Review comment:

  • [P1] Replace the inherited PATH before resolving tools — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tgGNODxr71/adapters/local-git-materializer/v1/materialize.sh:25-27
    When PATH contains a target- or user-writable directory, the clean worker preserves it and later executes jq, uname, id, find, and wc through that path. A fake jq can spoof the version check, while fake utility commands can bypass boundary and resource checks, allowing arbitrary code execution under the materializer principal despite the environment scrub. Use trusted absolute executables or an explicitly pinned dependency path instead of forwarding the caller's PATH.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: c2d2d4a
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-99881/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer mishandles an otherwise accepted absolute jq path when it contains shell-splitting characters. This causes a valid invocation to fail and can select the wrong executable.

Review comment:

  • [P2] Quote the jq executable in the registry check — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jAslcyv7gR/adapters/local-git-materializer/v1/materialize.sh:64-64
    When the supplied jq executable has whitespace or glob characters in its absolute path, this unquoted expansion is word-split or pathname-expanded even though the earlier validation accepts such paths. For example, /tmp/runtime tools/jq passes the version check but is invoked here as /tmp/runtime, causing valid materializations to fail or invoking an unintended executable; quote $jq_bin as in the other calls.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: c829634
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-56488/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer's fail-closed tree validation can silently succeed after scratch write failures, potentially skipping security-relevant subtree checks. This should be corrected before the adapter is considered reliable.

Review comment:

  • [P1] Fail when the tree-scan queue cannot be written — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.8gJe6yLUzR/adapters/local-git-materializer/v1/materialize.sh:433-433
    When the scratch filesystem returns ENOSPC or another write error here, the failed printf is ignored: scan_tree is invoked on the left side of ||, so Bash disables errexit throughout the function, and the final successful rm can make it return success. The omitted subtree is then never checked for unsafe paths, symlinks, or submodules, allowing a materialization receipt to be emitted without the promised validation; explicitly check every queue/output write and return failure.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: d214301
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-12789/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The materializer's expanded path listing is not covered by its stated 16 MiB tree-scan bound, allowing valid bounded trees to consume substantially more scratch space than promised.

Review comment:

  • [P2] Cap expanded tree-path output — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Vfklt3Ldac/adapters/local-git-materializer/v1/materialize.sh:428-433
    When tens of thousands of files share a near-4096-byte directory prefix, the raw ls-tree data can remain below 16 MiB while this write repeats the full prefix for every file, producing roughly 256 MiB in source-paths or candidate-paths. Because total_bytes accounts only for raw per-tree output, this bypasses the documented tree-scan byte limit and can exhaust caller scratch space; track and cap bytes written to the expanded output as well.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 6bb997c
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-68435/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The tree scanner's new expanded-path bound covers blob output but not directory paths appended to its queue. A crafted bounded source tree can therefore cause substantially larger scratch writes than the stated ceiling.

Review comment:

  • [P1] Bound expanded directory paths before queueing — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.zdCbEeidOf/adapters/local-git-materializer/v1/materialize.sh:439-440
    When a tree beneath a near-4096-byte prefix contains many child trees, this branch appends every expanded path to $queue without charging expanded_bytes. A 65,536-entry tree can therefore write roughly 256 MiB before the 1,024-tree check rejects it, bypassing the documented 16 MiB expanded-path ceiling and potentially exhausting constrained scratch storage; charge queued directory paths against the same byte budget before writing them.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: da15ae0
Reviewed-base: 150568f
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/229-24556/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

No discrete, actionable correctness defects were identified in the changes relative to the specified merge base.

@yihanzhu
yihanzhu merged commit a637451 into main Sep 4, 2026
2 checks passed
@yihanzhu
yihanzhu deleted the codex/local-git-materializer-v1 branch September 4, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant