Skip to content

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

Open
yihanzhu wants to merge 8 commits into
mainfrom
codex/local-git-materializer-v1
Open

Roadmap item 4: Add inactive local Git materializer#229
yihanzhu wants to merge 8 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: 62b39cc
Status: ✅  Deploy successful!
Preview URL: https://b13e1b6f.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.

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