Repository navigation
feat(runtime): persist trusted Write/Edit evidence before dispatch - #6089
jackeyfaker77 wants to merge 11 commits into
Conversation
Refs apache#4319 Generated-by: OpenAI Codex
Add Edit preparation and guarded execution, versioned mutation evidence, conversation-copy rebinding, and regression coverage for both tools. Refs apache#4319 Generated-by: OpenAI Codex
Keep package and recovery triggers aligned with their source closures. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the complete 20-file change at 04d2b7d6c3f324e43f25dca3466ab6fa87f07375: versioned file evidence and decoding, Write/Edit preparation and execution, worker permissions/protocol, the existing T1 transaction, cancellation/locking and conversation-copy rebinding. One P2 remains: the new unconditional cleanup at packages/runtime/src/tool-runtime.ts:1922 breaks the existing successful Client Capability settlement contract and causes this PR's CI failure. The inline comment identifies the failing assertion and comparison.
The pre-dispatch evidence premise is observable, not just an issue proposal. In 18 independent child-process controls, terminating the actual Runtime process after committed T1 or before T2 left the operation prepared after SQLite reopen/rebuild. The file respectively remained unchanged or contained the expected new content. This head retained the bound before/expected-after fingerprints; removing preparation from the same builtin tool retained those file effects and operation states but no trusted file evidence. Seventy-two rehashed cross-identity substitutions were rejected, and historical events without the optional field remained readable. These observations demonstrate evidence persistence, not successful automatic recovery.
Scope and cost need a maintainer decision: the recovery registry, observer/reconciler, scan and Host recovery lifecycle are still absent. Worker-backed writes add a preparation process before execution, plus bounded hashing and result construction; production performance was not measured. Write/Edit retain never_auto_retry. The first contract covers regular UTF-8 files with before/after sizes up to 1 MiB and result JSON up to 256 KiB; unsupported observations retain ordinary execution without recovery eligibility. The in-process lock and descriptor checks are not cross-process CAS. The immediate change is therefore evidence production and guarded normal execution, not complete production file recovery.
The producer, execution guard, T1 binding and copy rebinding form one coherent contract: separating them would either persist a prediction that execution does not check or make copied events fail their identity validation. Keeping future recovery registration, reconciliation and lifecycle work separate is appropriate. Write and Edit could be stacked separately for review size, but I did not find an unrelated feature that must be split out of this patch. Whether to land this foundation before its recovery consumer remains a product/design choice, not approval implied by passing tests.
Validation: fresh dependency installation and root clean build; 307 passing tests across 19 complete Runtime/Core files covering file operations, durable boundaries, crash windows, copy and permissions. The complete settlement test file adds 13 passes and one failure, reproducing CI's extra cancel after successful admission; substituting the exact main ToolRuntime module in memory makes the same unchanged file pass all 14 tests. The other dependencies remain at this PR head, so that comparison isolates the changed module rather than claiming a full main-suite run. Core/Runtime/Host typechecks, 16-file Biome check, diff check and protocol guard passed. The independent crash probe used actual SIGKILL and the real local bypass executor/SQLite authority, not a simulated exception. Existing tests also exercised real one-shot workers and copy rebinding. Native Linux sandbox execution could not run locally because bubblewrap is absent; native macOS/Windows, the complete monorepo suite and a recovery consumer were not locally validated.
The internal filesystem worker protocol moves from 11 to 12; Runtime Host epoch remains 220 with no Host protocol change. The merge with current main 6c63fa05 is clean. Exact-head Windows packaging, recovery and W0 evidence checks and dependency audit succeeded, but CI 38395346636 failed on the settlement assertion above. This is a PR-caused failure, not one of the documented Windows baseline cleanup failures. Neither passing checks nor this review imply feature approval.
| }); | ||
| return sandboxError ? { error: msg, sandbox: sandboxError } : this.errorReturn(msg); | ||
| } finally { | ||
| await preparedExecution?.cancel(); |
There was a problem hiding this comment.
[P2] Close the settlement-contract regression introduced by cleanup
This unconditional cancel() now runs after a prepared Client Capability has executed successfully, not only when admission/T1 fails. Exact-head CI run 38395346636 fails the unchanged prepares Bypass Client Capability work before T1 and admits only after T1 test: actual order is ['prepare', 'T1', 'admit', 'cancel'], while its established success contract expects ['prepare', 'T1', 'admit'].
The complete test file reproduces this locally (13 pass, 1 fail); loading the exact 6c63fa05 ToolRuntime module in memory with the same test and other dependencies makes all 14 pass. This isolates the PR-caused CI failure. The production broker treats cancellation of an already-retired invocation as a no-op, so this evidence does not establish that completed production invocations are actually cancelled. Filesystem preparation needs reliable resource release, but its new cleanup must either be scoped appropriately or explicitly reconciled with the existing prepared-tool success contract and tests before this head is ready.
Cancel only preparations that never started execution. Once execute takes ownership, leave success and failure cleanup to the prepared executor. Preserve the existing Client Capability success contract and cover failures before execution and inside execution. Generated-by: OpenAI Codex
Retry validation after the isolated CodeMode timing test failure; no source changes. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At a0f156ff9b3a65dfeec33f30df955c1c3515d0bb, the previous P2 settlement-order finding is fixed. No additional P0–P3 issue was established in this increment.
The change transfers cleanup ownership to the prepared executor immediately before calling its execute method (packages/runtime/src/tool-runtime.ts:1648–1658). Runtime therefore cancels only preparations that never entered execution. The unchanged Client Capability success test again observes prepare → T1 → admit, without an extra cancel. Loading the exact 04d2b7d6 ToolRuntime into the same test still reproduces the old extra-cancel failure; the rest of the dependencies remain at the new head in this comparison.
The failure and cancellation paths also retain cleanup:
- A preparation cancelled after T1 publication fails, or when T1 itself fails, does not execute.
- Once execution starts, its executor releases its own resources on success, execution failure or abort; Runtime does not cancel that completed/executing object again.
- Twelve independent real Write/Edit checks cover success, T1 failure, publication failure, execution failure, abort before execution and abort during execution. Each check uses the actual filesystem preparation and SQLite commit paths, verifies the expected file contents and durable success/error/aborted outcome, then performs another prepared Write on the same target to prove that the real lock was released. The failure and abort timing are controlled test seams, not live provider incidents.
The original 18-process crash/reopen checks also pass: owned child processes are killed after committed T1 or before T2, and SQLite rebuild retains the expected evidence and operation state. The preparation-removal controls retain the same file effects but lack trusted evidence; 72 rehashed cross-identity substitutions are rejected and legacy events remain readable. These checks still do not exercise a Host recovery reconciler, native mount sandbox or cross-process compare-and-swap.
The increment contains two commits: the cleanup change and a validation-rerun commit with no source changes. The net diff affects Runtime and its settlement tests (+75/−4), with no rebase, new evidence format, or additional feature scope. A fresh dependency installation and normal clean build succeeded. Eight complete Runtime/Host test files passed 145 tests with no skips, including all 16 settlement tests and the existing Host broker/coordinator tests. Runtime and Host type checks, scoped Biome, whitespace checks and the protocol guard passed. This is not a full-monorepo, native-platform or live-provider validation.
Epoch 220 and the existing worker protocol 12 are unchanged from the preceding head; no storage-schema change was added. The head merges cleanly with fetched main 6c63fa057ed97916b77db3ff002a13636d6a564a; that synthetic merge was not separately built. At the final check, CI and audit were queued, Windows packaging was running, and the recovery and sandbox evidence checks had succeeded. This does not mean all checks are complete. The earlier feature-scope caveats remain: this PR produces evidence and guards execution, but does not implement recovery reconciliation; feature adoption and merging remain human decisions.
Preserve upstream chat image reads while advancing the prepared Write/Edit worker contract to v13. Add rejection coverage for upstream v12 workers. Generated-by: OpenAI Codex
The upstream chat-image parser requires marked@18.0.11 in production. Record the single package instance reported by Linux CI without changing the dependency-closure guard. Generated-by: OpenAI Codex
Upstream simplified the navigation-state initializer but left its nonTriviaTokens count stale. Record the measured 7280 tokens so strict base checks reflect the unchanged source tree. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At 2b3eff5a3931289f128c3e7555fdc45cea3f48d8, I found no new P0–P2 issue in the changes since a0f156ff. The earlier cleanup-ownership fix remains intact.
The merge with main 331340ab preserves all five original author commits. Its only textual conflict was the filesystem-worker version: both parents independently used v12 for different contracts. packages/runtime/src/filesystem-worker/protocol.ts:30 now uses v13 for the combined prepared Write/Edit and chat-image contract. Request and response schemas require that version, and the real client rejects a v12 response before accepting its result (client.ts:752). The separate Host compatibility epoch is 222, inherited from main; its guard reports no Host protocol change against the merge parent.
I verified the built worker through 16 actual subprocesses. Four v12 mutation requests were rejected without changing the file. For both Write and Edit, v13 preparation left the file unchanged, matching guarded execution succeeded, and an externally changed file caused execution to fail without overwriting those external bytes. Chat image reads preserved the exact bytes of a 2560×1 PNG; ordinary reads still refused that image, and oversized/invalid images retained their typed errors.
The cleanup transfer in tool-runtime.ts:1655 is unchanged. Twelve real Write/Edit scenarios again passed through filesystem preparation and SQLite persistence, covering success, T1 failure, publication failure, executor failure and cancellation before/during execution. Each case then acquired the same target's lock for another prepared Write. These use controlled failure timing; they do not test native OS sandbox enforcement or a recovery reconciler.
The two subsequent commits only add marked@18.0.11 to the packaged dependency ledger and correct the AppShell token count from 7285 to 7280. Both ledger files are byte-identical to their counterparts in #6129. These two lines duplicate/overlap that PR; rebase after #6129 lands to reconcile them.
Validation: the normal clean root build and Host type check passed before those ledger-only commits; production sources, tests, dependencies and the lockfile are unchanged by them. On the final head, all 151 focused tests and both subprocess/filesystem probes passed again. Full formatting, whitespace, strict renderer architecture (168 checker tests), artifact budget (7 tests; 242 package instances), Windows skip inventory and the epoch guard passed. This is not a full-monorepo, native Windows/macOS/Linux-sandbox or live-provider validation, and I did not rerun the earlier crash/reopen campaign.
At the final snapshot, CI, Windows packaging and sandbox evidence were still running; audit and Windows recovery had succeeded. The current head is therefore not yet all green. This remains an evidence-producing feature, not recovery reconciliation; feature adoption and merging remain human decisions.
Merge main after apache#6129 and retain its latest renderer ledger. Both CI ledgers now have no net diff against upstream; preserve the Write/Edit evidence changes. Validation: root build/typecheck/lint/format, Desktop/UI Knip, artifact budget, strict renderer architecture, 69 core regressions, ASF and protocol checks passed. Independent review found no issues. Renderer checker fixtures: 166 passed, 1 POSIX skip, 1 unchanged Windows absolute-path fixture failure. Generated-by: OpenAI Codex
Replace the fixed immediate-turn budget with bounded wall-clock polling while keeping the injected observation clock unchanged. Add a delayed-work regression that fails with the old helper. Validation: all 251 event-wait tests passed; the two CI failures passed 10 independent runs; Host build/typecheck, Biome, ASF and independent review passed. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.Incremental review:
2b3eff5a..50565abd, a merge of main atd14cec57plus one test-only commit. I found no P0–P2 issue, and CItestpasses on this head.
- Merge: the only hand resolution is the AppShell
nonTriviaTokensvalue inrenderer-architecture.json, which takes main's 7276. That correctly drops the CI-ledger lines this PR had duplicated from #6129. The Write/Edit evidence code is unchanged. 50565abd:eventually()inevent-wait-manager.test.tsnow polls on a 5 s wall-clock deadline with 5 ms sleeps, instead of 100 rounds ofsetImmediate. The injected observation clock stays fixed, so the timeout and Retry-After assertions stay exact. The new self-test checks that delayed host work completes without advancing that clock or firing its timers. This is a reasonable fix for a flaky test.
P3 (scope): event-wait-manager.test.ts belongs to the event-wait feature from #6113, not to Write/Edit evidence. If this is a flake on main, a small separate PR would let it land independently of this feature.
|
Thanks @Astro-Han for the review. I’ll move the event-wait test fix into a separate PR against main, run the tests and an independent agent review, then remove it from #6089 so this PR stays focused on Write/Edit evidence. I’ll link the separate PR here once it is ready. |
Remove the event-wait test-only change from the Write/Edit evidence PR now that it is proposed independently in apache#6140. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.Incremental review:
50565abd..c28bda56. The event-wait test change is reverted here and moved to its own PR, #6140. This resolves my scope P3. No other change. I found no P0–P2 issue, and CItestpasses.
The event-wait tests can exhaust their fixed `setImmediate` polling budget before asynchronous Host work reaches the expected checkpoint. In CI this caused the late-authorization and slow-cooldown-persistence tests to fail before their third authorization observation. Give the test helper a 5-second monotonic wall-clock deadline with 5 ms sleeps. The injected observation clock remains fixed, so existing timeout, Retry-After and cancellation assertions retain their exact semantics. A regression test verifies that delayed Host work completes without advancing that clock or firing its timers. Production code is unchanged. Extracted from #6089 following its scope review, so this test fix can land independently of Write/Edit evidence. Refs #6113. Failure evidence: https://github.com/apache/maka/actions/runs/39031806684/job/117393589335 Generated-by: OpenAI Codex
|
With #6130 now merged, I’d like to clarify the intended direction for this PR before making further changes. #6089 persists versioned Write/Edit pre-state and expected post-state evidence at T1, and checks the prepared preconditions during execution. It does not implement a recovery consumer; both tools remain #6130 retires the previous reconciliation roadmap while explicitly leaving #6089 unchanged. Would maintainers still consider landing this evidence-producing foundation independently, despite the absence of a current recovery consumer and the additional preparation/hashing cost? Alternatively, would you prefer that I narrow this PR to execution-time file-content checks without persisted recovery evidence, or defer it until a concrete consumer is agreed upon? That narrower option would still need a separate cost/benefit assessment. I’ll leave the implementation unchanged pending guidance. |
Summary
Write/Edit dispatches currently lack trusted file-state evidence for later reconciliation. This adds versioned UTF-8 mutation contracts, captures bounded pre-state and expected post-state inside the filesystem executor, and binds that evidence to the dispatch identity before the existing atomic T1 persistence boundary. Guarded execution checks the prepared preconditions; Edit uses the same matching transform for preparation and execution.
Conversation copies rebind evidence to their new identities, and historical dispatches without evidence remain readable. Both tools retain
never_auto_retry. This implements the evidence-producing checklist item; the Host-owned recovery registry, reconciliation, and recovery lifecycle remain separate work.Refs #4319
Verification
Scope follow-up: the event-wait polling fix was extracted to test(runtime-host): wait for event observation I/O by deadline #6140 and removed from this PR in
c28bda560. The resulting tree exactly matches the parent before that test-only fix; the event-wait test file matches main. Host build and all 69 settlement, file-mutation-evidence and durable-boundary tests passed after removal. Independent Codex review confirmed that only this fix was removed and found no P0-P2 issues. Until test(runtime-host): wait for event observation I/O by deadline #6140 lands, the original upstream event-wait flake may still affect CI; this PR no longer includes its fix.Latest cleanup fix: Runtime build, two-file Biome check, and all 69 tests in the complete settlement, file-mutation-evidence, and durable-boundary suites passed. The existing Client Capability success assertion is unchanged; two new tests cover publication failure before execution and execution throwing after cleanup ownership transfers. Independent Codex review passed. Host invocation-broker tests passed 2/2; the separate admission integration test fails while deleting its SQLite fixture on Windows (
EBUSY).Passed: full build, typecheck, lint, format, Knip for desktop/UI, ASF headers, Windows-test inventory, protocol-epoch check, and diff checks.
Passed: 180 focused tests covering Write/Edit T1 failures, simulated crash windows and SQLite reopen/rebuild, changed files, cancellation, evidence tampering, worker preparation/permissions, Edit matching parity, and conversation-copy rebinding. One Linux-only test was skipped on Windows.
Full conversation-copy suite: 16 passed / 13 failed; the same 13 Windows SQLite
EBUSYcleanup failures reproduce on unmodified upstream (14 passed / 13 failed). Other broader filesystem failures reproduce on the baseline and involve Windows symlink permissions or POSIX/Linux assumptions.Passed: 43 Windows source-closure and CI-planner checks after synchronizing the workflow path filters. The broader workflow-policy run passed 45/46 tests; its remaining failure is Windows
EPERMwhile removing a temporary Git fixture.Native Linux/macOS sandbox execution was not validated locally and requires CI. An independent Codex review found no unresolved actionable issues; it does not replace the required human review.
AI use
Tool(s) and scope: OpenAI Codex assisted with implementation, tests, documentation, review, and this PR description. Affected commits include
Generated-bytrailers.Checklist
The second checkbox remains unchecked because the broader Windows suites have the baseline failures described above.
Does this PR entail a change in behavior?