Skip to content

Add resumable offline delivery replay - #232

Open
yihanzhu wants to merge 10 commits into
mainfrom
codex/delivery-replay-v1
Open

Add resumable offline delivery replay#232
yihanzhu wants to merge 10 commits into
mainfrom
codex/delivery-replay-v1

Conversation

@yihanzhu

@yihanzhu yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner

One inactive offline delivery-loop slice under the accepted Roadmap construction program.

Uses the existing local Git materializer, verifies one candidate blob, and journals restartable materialize/verify/review-wait/publish-wait phases. Review/publisher records are offline observations only; authority remains none.

The driver binds loaded code to source and captures actual native dependency executables, rejecting script launchers. Candidate commit/tree/parent and blob are revalidated before accepting observations. Cancellation preserves resumable materialization/verification state; identity types are checked before journaling. Caller candidates are not cleaned or overwritten during recovery.

Targeted proof on 1fa6e09: 28/28 checks, ShellCheck 0.11.0, Bash syntax, Python AST and diff check passed. Safe ablation removed unnecessary execution indirection, duplicate validation and a redundant assertion; final tests run on the reduced candidate. Exact required CI and independent review remain the publication gates.

Scope: replay, focused tests, restore manifest, README/RESTORE and same-PR construction plan. No candidate-code execution, model invocation, real targets, credentials, network, installation, activation, release, deployment or actual publication. The current-session operator ruling on ordinary revision rounds is recorded in comment 5551631795; no shipped operating policy is changed.

@cloudflare-workers-and-pages

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

Copy link
Copy Markdown

Deploying ystack with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1fa6e09
Status: ✅  Deploy successful!
Preview URL: https://f612c67f.fabrica-6yx.pages.dev
Branch Preview URL: https://codex-delivery-replay-v1.fabrica-6yx.pages.dev

View logs

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 6537715
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The replay cannot recover from a real crash in a critical window after materialization, despite presenting itself as resumable. It also has an uncaught malformed-input path and a timing-dependent interruption regression test.

Full review comments:

  • [P2] Reconcile materialization before rerunning after a crash — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tX6KbaG6PK/delivery/v1/replay.py:309-312
    If the process is killed or the host fails after the materializer creates repository.git but before the verifying phase is journaled, the persisted phase remains materializing. A retry invokes the materializer again against the now-nonempty candidate directory, which fails its empty-directory check and permanently marks this replay failed instead of resuming. Reconcile an already-created candidate or otherwise make this side effect and phase transition recoverable.

  • [P2] Handle invalid UTF-8 as a replay error — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tX6KbaG6PK/delivery/v1/replay.py:55-59
    When an input, observation, or persisted journal contains invalid UTF-8, json.loads(bytes) raises UnicodeDecodeError, not JSONDecodeError. That exception bypasses main()'s controlled error handling and emits a traceback instead of the expected replay diagnostic, so decoding failures should be converted to ReplayError here.

  • [P2] Synchronize the interruption test to the asserted phase — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tX6KbaG6PK/scripts/test/delivery-replay.test.sh:182-182
    The presence of run.json only proves that the initial materializing state was written; it is created before run_materializer() starts. On a fast or differently scheduled host, materialization and verification can finish between this poll and the signal, causing kill to fail under set -e or leaving the journal at review-wait rather than the asserted verifying phase. Use a controlled blocker or a phase-specific synchronization point to make this test deterministic.

@yihanzhu

yihanzhu commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 0462ad2
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The new recovery path cannot reconcile any interrupted no-change materialization, and structurally malformed persisted state can escape the CLI's controlled error handling. Both affect the replay's advertised resumability.

Full review comments:

  • [P2] Handle no-change commits during reconciliation — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.p2BR4JoVZn/delivery/v1/replay.py:223-224
    When a no-change materialization is interrupted after repository.git is installed but before the journal advances, retry reaches this loop. The adapter points candidate at the source commit and records that source commit as parent_commit_id, while candidate^ resolves the source commit's actual parent—or fails for a root commit—so the valid candidate can never equal the recomputed receipt and recovery fails.

  • [P2] Reject malformed journal shapes before resuming — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.p2BR4JoVZn/delivery/v1/replay.py:322-322
    When run.json is valid JSON but has a non-object identity, this chained .get raises an uncaught AttributeError; similarly, a journal retaining the correct run key but missing phase later raises an uncaught KeyError. Since the state directory is caller-owned and main() only catches OSError and ReplayError, structurally malformed journals produce tracebacks instead of the intended controlled error, so the full journal schema should be validated before use.

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 4c29c1a
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The replay can resume under an incompletely bound materializer identity, accepts malformed actor identities, and does not safely validate candidate identity in recovered journals. Its documented focused test is also broken on supported hosts without a separate system jq.

Full review comments:

  • [P2] Bind the materializer's full package identity — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6A66gfB6Ny/delivery/v1/replay.py:151-151
    When a replay resumes from materializing after the checkout changes, hashing only materialize.sh leaves the run key unchanged even though that script executes protocol.jq, scripts/core-contract.sh, the generation registry, and generation modules. An interruption before candidate creation can therefore resume under different materialization behavior without becoming stale; bind the complete executable package/dependency identity.

  • [P2] Require candidate IDs after materialization — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6A66gfB6Ny/delivery/v1/replay.py:316-318
    For verifying and later journals, candidate IDs are only validated when present and are never compared with the materialization record. Deleting identity.candidate_tree_id from an interrupted journal passes validation and later raises an uncaught KeyError; supplying different valid IDs instead produces a state inconsistent with its saved receipt. Require both IDs and equality with materialization in post-materialization phases.

  • [P2] Reject non-string actor IDs — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6A66gfB6Ny/delivery/v1/replay.py:291-292
    When an offline observation supplies a numeric actor_id, such as 123, converting it with str() makes it pass the identifier regex, but the original numeric value is then stored in the journal. This violates the repository's string ID contract and emits malformed state; check that actor_id is a string before matching it, including in state validation.

  • [P2] Use the pinned jq binary in the test — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6A66gfB6Ny/scripts/test/delivery-replay.test.sh:109-109
    On the explicitly supported Darwin hosts, a system jq is not guaranteed to exist. The test locates and wraps the pinned jq successfully but then invokes bare jq for its assertions without adding the runtime directory to PATH, so the documented standalone restore check fails with command not found; invoke the wrapper or prepend its directory to PATH.

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: e02c634
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The replay has malformed-input paths that escape its controlled error handling, and its recorded execution and verification identities can diverge from what was actually used. These issues undermine the fail-closed and exact-identity properties of the new replay journal.

Full review comments:

  • [P2] Reject non-string source object IDs before regex matching — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.adOR10VZi3/delivery/v1/replay.py:175-178
    When a syntactically valid input supplies a numeric or otherwise non-string source tree object_id, OID.fullmatch(source_tree_id) raises TypeError, which main() does not catch, so malformed input produces a traceback instead of a controlled replay failure. Check that the value is a string before applying the regex, as is already done for request_sha.

  • [P2] Catch integer-limit failures from JSON decoding — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.adOR10VZi3/delivery/v1/replay.py:71-75
    On Python 3.11+, JSON containing an integer longer than the configured digit limit causes json.loads to raise ValueError, not JSONDecodeError. Such an input, observation, or journal therefore escapes ReplayError handling and prints a traceback; catch this decoder failure and report it as invalid JSON.

  • [P2] Bind recorded hashes to the bytes actually executed — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.adOR10VZi3/delivery/v1/replay.py:196-200
    If the replay package, jq binary, or closure helper is replaced after these path-based hashes are computed but before the materializer consumes them, the run executes the new bytes while journaling the old digests and run_key. The state lock does not cover these files, so a concurrent checkout or dependency replacement defeats the exact executable identity; execute immutable snapshots or revalidate every consumed file before recording success.

  • [P2] Relate saved verification evidence to the run identity — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.adOR10VZi3/delivery/v1/replay.py:389-394
    When a persisted review-wait, publish-wait, or completed journal contains a different but well-formed verification ID, path, or SHA-256, this validation accepts it. Resume rechecks the blob using identity["verifier"] but never replaces the saved verification record, so the completed receipt can claim evidence for a verifier, path, or digest that was not used; require these fields to equal the current verifier identity and expected digest.

@yihanzhu yihanzhu added needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure round-3 Review-loop counter: revision 3 (cap) labels Sep 5, 2026
@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Review cap reached. CI run 33934673501/check 101220163015 (app 15368) passed at head e02c634 / base a637451; final independent review 5548482129 remains not clean. No merge or further automatic revision.

Proposed one final revision ONLY: (1) reject non-string source object IDs with controlled errors; (2) handle JSON integer-limit decoder ValueError; (3) bind execution digests to bytes actually consumed using immutable snapshots or complete revalidation; (4) require saved verification id/path/digest to equal the run verifier. Preserve all existing behavior, tests, scope and boundaries.

Operator action requested: authorize this exact four-item final revision, followed by fresh CI and independent review. Existing candidate and history are preserved.

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

The operator directly authorized one final revision in the current coordinator session on 2026-09-05. Scope is limited to the four findings in review 5548482129: source object ID type validation, JSON integer-limit decoding failures, consumed executable byte identity, and saved verification binding. Resume the preserved clean head e02c634 against base a637451 on this same branch. Add regression tests, perform scoped ablation, then require fresh exact CI and one full independent final review. No finding is waived and no additional revision loop is authorized.

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

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Authorized final revision is now a7de157 against a637451. Only replay.py and its focused test changed. The source-tree type guard already existed at e02c634; numeric/list/null regressions confirm controlled rejection, so that finding needed no code change. Other fixes cover JSON decoder ValueError, private execution snapshots with owner-only cleanup, and exact saved-verifier binding. Post-ablation focused matrix: 21/21; syntax, ShellCheck 0.11.0, Python AST, and diff checks pass. Removed a special execution branch and redundant suffix condition, then reran the matrix. Fresh CI and final independent review remain required; this comment is not review evidence.

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Final revision a7de157 passed required ci (run 33961833994, check 101294932870, app 15368) against base a637451. The independent Codex review invocation was rejected by the tool approval layer before execution because it transmits PR content to the Codex review service and posts the review comment. No review ran, no review verdict exists, and no merge was attempted. Preserving this exact clean candidate while requesting explicit approval for that review operation; no further code revision is requested.

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

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

The operator explicitly approved sending PR #232 code and diff to the OpenAI Codex review service and publishing the original review result on this PR, after disclosure in the current coordinator conversation on 2026-09-05. The prior tool rejection happened before execution, so no final review was consumed. Exact head a7de157 and base a637451 remain unchanged; required ci check 101294932870 from app15368 is successful. Proceeding with the one authorized final independent review; no code revision or gate waiver.

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

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: a7de157
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The replay can bypass its execution snapshot, can accept observations after the materialized candidate ref changes, and does not consistently honor cancellation. These break the identity and resumability guarantees central to the change.

Full review comments:

  • [P1] Reject externally selected execution roots — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.U2RMFE0Zdb/delivery/v1/replay.py:457-459
    A caller can invoke a copy at <root>/delivery/v1/replay.py --execution-root <root> and satisfy this equality check. If that root contains .dependencies, this bypasses create_execution_snapshot(), allowing package or dependency files to change after input_identity() hashes them but before run_materializer() executes them, so the journal can record the wrong execution identity. Authenticate the parent-created snapshot rather than relying only on path equality.

  • [P2] Revalidate the candidate ref before accepting review — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.U2RMFE0Zdb/delivery/v1/replay.py:547-550
    After a run reaches review-wait, changing refs/heads/candidate to another commit does not block completion as long as the old recorded tree object remains in the repository. This check verifies a blob from the saved tree but never confirms that the candidate ref still matches the recorded commit, tree, and parent, so review and publisher observations can complete for a candidate different from the repository's selected candidate.

  • [P2] Stop wait phases after receiving an interrupt — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.U2RMFE0Zdb/delivery/v1/replay.py:562-570
    When SIGTERM or SIGINT arrives during a resumed review-wait or publish-wait run, the handler only sets interrupted; these branches never inspect it. For example, a signal received while waiting for the lock or reading an observation can still be followed by recording review, publisher, and completed-offline, so cancellation is silently ignored after verification.

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

The authorized final review completed and posted raw comment 5551563469 on exact head a7de157/base a637451. The coordinator read it in full and checked the affected code. It is NOT clean: execution-root selection can bypass snapshot creation; wait phases do not recheck the selected candidate ref against the recorded commit/tree/parent; wait phases can continue after cancellation. Required ci is green, but no merge was attempted. This authorized final revision/review is exhausted; preserving the clean branch and stopping further edits. Proposed next disposition, not yet authorized: fix only these three correctness gaps and their regressions in the existing replay implementation/test, perform safe ablation, then fresh exact CI and independent review. No new process infrastructure or product scope. The operator already explicitly approved Codex review data transmission and posting the result; that permission is not the present blocker.

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

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Direct operator ruling received in the current coordinator conversation on 2026-09-05: authorize the three findings from 5551563469 and continue the already-agreed inactive Roadmap construction work without stopping solely for ordinary revision-round limits. This is a current-session construction ruling, not an activation or reusable target policy change. It does not waive findings, exact required CI, independent review, bounded scope, publisher checks, or inactivity. Continue same PR/branch from clean a7de157 against a637451; retain round-3 as history, not reset the counter. Fix and independently review candidates until clean, then publish through the existing exact-head squash gate. No new process infrastructure; no real targets, credentials, installation, activation or deployment. Codex review payload/comment permission remains explicitly granted in 5551475271.

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

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 6d53d29
Reviewed-base: a637451
reviewer: gpt-5.6-sol @ high

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

The replay can execute dependency bytes not represented by its recorded identity, mishandles cancellation during materialization, and can persist a journal that its own validator rejects. These defects undermine the new replay's execution-integrity and resumability guarantees.

Full review comments:

  • [P2] Snapshot the executable behind dependency launchers — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.P1T6hoc9el/delivery/v1/replay.py:167-168
    When --jq-bin is a launcher script—as the supported Darwin/arm64 test creates—the snapshot and jq_sha256 cover only that script, while it executes an absolute cached binary outside the snapshot. Replacing that binary after snapshotting or between resumed runs changes the executed code without changing the run identity, defeating the claimed immutable execution record; snapshot and bind the actual executable or reject such launchers.

  • [P2] Keep interrupted materializations resumable — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.P1T6hoc9el/delivery/v1/replay.py:550-552
    When SIGINT or SIGTERM reaches the foreground process group while materialization is running, the shell materializer exits with 130/143 and run_materializer raises ReplayError; this handler then journals a terminal failed phase even though the interruption flag is set. Every later invocation exits immediately from that failed state, so ordinary cancellation cannot resume as the signal-handling design intends; handle the interruption before converting the child exit into a failed replay.

  • [P2] Reject non-string commit identities before journaling — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.P1T6hoc9el/delivery/v1/replay.py:221-222
    When malformed input supplies commit_id as a 40-digit JSON number, converting it with str(...) lets it pass the OID check and stores an integer in the journal. The materializer then rejects the input, but the next invocation rejects the replay's own saved journal because validate_state requires this field to be a string, making the failure state inconsistent and unusable; require a string before applying the OID regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

round-3 Review-loop counter: revision 3 (cap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant