fix: stop ledger tests accumulating temporary files - #1294
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper review The body now includes exact-head full-suite CI success and the executed before/after fixture-cleanup and failure-path proof. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
The requested native review is blocked before provider invocation by the mandatory agent-input scan in run 33262270348. The PR remains unchanged at A local reproduction using the native scanner's flags and isolated environment identified the same existing synthetic malformed-URL fixture in both versions of
No credential-shaped value is reproduced here. The native scanner deliberately scans both complete base and head blobs for changed files ( This PR is left unmerged pending resolution of the synthetic-fixture classification by the scan owner. No scanner policy, source scope, timeout, or test assertion has been weakened, and no further review request is being made while this blocker remains. |
|
@clawsweeper re-review The scan-owner prerequisite is merged in #1296 ( This PR's code remains unchanged at |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. Reviewed August 29, 2026, 3:51 PM ET / 19:51 UTC. ClawSweeper reviewWhat this changesThis PR makes action-ledger test fixtures remove their temporary directories and ensures a failed projection-fairness assertion releases held mock requests before later tests run. Merge readinessKeep open: current main still has the pre-cleanup test fixtures, while this narrow test-only repair has credible exact-head filesystem and failure-path proof with no actionable patch defect found. Priority: P3 Review scores
Verification
How this fits togetherAction-ledger tests create temporary filesystem roots to exercise durable event and shard handling. A separate projection-fairness test holds mocked requests in a shared admission pool, so its cleanup affects isolation for later tests. flowchart LR
A[Action-ledger tests] --> B[Temporary fixture roots]
B --> C[Node test lifecycle hook]
C --> D[Fixture directory cleanup]
A --> E[Projection fairness test]
E --> F[Finally releases held requests]
D --> G[Clean later test runs]
F --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the per-test temp-root cleanup and fairness-pool finalization while keeping the broader runner-lifecycle work in #1099 separately scoped. Do we have a high-confidence way to reproduce the issue? Yes — the PR body records exact-head before/after filesystem counts and an injected fairness failure; current main retains the base versions of both affected test files. Is this the best way to solve the issue? Yes — registering cleanup at fixture creation and draining held fairness requests in finally is the narrowest fix for the demonstrated test-lifetime failures. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0a46047ef6a6. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix: stop ledger tests accumulating temporary files This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
1 participant
Related: #1099
What Problem This Solves
Resolves a problem where repeated action-ledger tests accumulate temporary files on long-lived development and validation hosts, even when the tests pass. One existing aggregate-limit case leaves nine 2 MiB shards per run. A fairness-test assertion failure can also leave mock requests occupying every projection slot and cause subsequent tests to fail or hang.
Why This Change Was Made
Each test fixture now registers cleanup as soon as it creates its root, before path canonicalization can fail. Cleanup belongs to the active Node test and removes only that fixture's directory. The fairness fixture releases held requests and drains their work in
finally, including callbacks arriving after cleanup starts. The existing timeouts and fairness assertions are unchanged.This complements #1099's broader test-runner isolation and stale-run cleanup. These helpers must also clean up during direct
node --testinvocations and between individual tests; the changes do not overlap that PR.User Impact
Repeated narrow ledger tests no longer leave their temporary data behind after normal completion or assertion failure. A failed fairness assertion no longer strands the following tests. There is no production behavior, configuration, schema, or dependency change. Hard process termination can still bypass Node test hooks.
OpenClaw Bay Impact
None. Only local test-fixture lifetime changes; production queues, action history, publication, and dashboard data are unchanged.
Documentation Impact
Reviewed
CONTRIBUTING.mdanddocs/action-ledger.md. Their validation and ledger contracts remain accurate; no documentation change is needed for this test-only repair.Evidence
Eight existing focused cases pass: aggregate byte limits, idempotent writes, exported projection concurrency, both import-race child cleanup cases, symlink defenses, and parent-chain swap detection. A fresh temporary directory contains zero retained fixture roots afterward. Formatting,
git diff --check, and pre-commit and committed-branch Codex autoreviews pass. Production LOC delta is zero; test LOC is net +8, primarily cleanup scope andtry/finallystructure.Real Behavior Proof
Claim and exercised surface: action-ledger test fixtures release their real filesystem directories and held projection requests when their owning test finishes or fails.
Environment: macOS, Node v24.20.0; isolated source at base
db14db010bf3044be85f25fe40a587a5ca77523a. The reviewed patch is commit0efc0e7d2ada1f665e5b2c00ad41c5c3d078c65a. Required runtime modules were emitted from that source with Node's built-in TypeScript stripping. Each comparison used an isolated temporary directory; no live channel or remote projection endpoint was contacted.Scenario and command: execute the existing disk-producing tests before and after the patch, then count directories and actual retained file bytes under the isolated
TMPDIR:node --test --test-name-pattern='state shard imports enforce aggregate byte limits before publication|action event writes are create-only and replay-idempotent' test/action-ledger-runtime.test.ts test/action-ledger.test.tsObserved filesystem trace:
0 !== 4; process remained pendingLimits: the attempted full two-file macOS run exceeded the fairness fixture's unchanged 60-second request timeout while writing its 68 durability-backed events. That run was stopped and is not claimed green. The exact-head Linux
pnpm checksuite subsequently passed, along with Windows launcher, sparse-build, and CodeQL checks (CI run). No timeout was raised or assertion weakened. All temporary data from the deliberate failure and stopped proof runs was removed.Native Review Admission
The pre-provider scanner blocker was repaired at its owner in #1296, merged as
0a46047ef6a6be7182babb6e33ed6d08df374d0a. That change retains complete scanning and classifies only the exact existing synthetic fixture; it includes native admission/refusal proof and passed the normal review and CI gates. This cleanup PR remains unchanged at0efc0e7d2ada1f665e5b2c00ad41c5c3d078c65a, so the cleanup proof above remains applicable. A fresh committed-branch Codex autoreview also passed. The current native re-review can now use the repaired owner on main.