fix(test): keep CodeQL fixtures data-only - #1291
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 29, 2026, 7:40 AM ET / 11:40 UTC. ClawSweeper reviewWhat this changesThis PR rewrites two test fixtures so variable clock and path values are stored as data while Node inherits an already-open checkout directory descriptor without a shell wrapper. Merge readinessThe test-only refactor appears correct, and the current PR body supplies exact-head Crabbox proof. This MEMBER-authored draft should remain open for explicit maintainer acceptance rather than automated cleanup. Priority: P3 Review scores
Verification
How this fits togetherThe affected tests drive ClawSweeper’s apply-decision deadline handling and terminal-proof cleanup. Controlled clock state and a helper process feed the built CLI and terminal driver, which produce runtime-yield and cleanup assertions. flowchart LR
A[Fixture state data] --> B[Fixed Node preload]
B --> C[Apply decision test]
C --> D[Deadline yield assertions]
E[Open checkout descriptor] --> F[Node helper process]
F --> G[Terminal cleanup test]
G --> H[Cleanup assertions]
Decision needed
Why: Repository policy keeps MEMBER-authored items open for explicit maintainer judgment even when the patch and evidence are satisfactory. Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the fixed-preload and direct-descriptor fixture patterns, then accept the current-head test-only cleanup once a maintainer is satisfied with the supplied proof. Do we have a high-confidence way to reproduce the issue? Yes, at source level: the two changed fixtures directly exercise the affected runtime-yield and directory-descriptor cleanup paths. The supplied exact-head Crabbox run reports the focused suite passing; this read-only review did not rerun it. Is this the best way to solve the issue? Yes. Moving fixture inputs into JSON and passing the directory descriptor directly removes variable-generated executable and shell construction while retaining the established test scenarios. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e1eefe217304. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles) |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
62c9ef0 to
6007142
Compare
What Problem This Solves
Resolves test fixtures that CodeQL identifies as shell-command injection and dynamically generated executable code:
The alerts remain open and undismissed on
mainate1eefe217304f0f510acb3890db1155cef61682e.Why This Change Was Made
The runtime-budget fixtures now use one fixed preload program and put timestamps and paths in a sibling JSON state file, including the unflagged sibling fixture using the same pattern.
After rebasing over #1293, its wrapper-death synchronization test is preserved completely. The following, separate terminal-cleanup fixture now passes an already-open checkout directory descriptor directly to Node as inherited fd 9 without a shell.
The change is test-only. It does not alter production, configuration, workflows, dependencies, generated files, or documentation.
User Impact
There is no user-visible runtime change. Maintainers keep the same process-cleanup and runtime-budget coverage without executable test source being assembled from fixture data.
OpenClaw Bay Impact
OpenClaw Bay is unaffected. This changes only local test fixture construction and does not alter lifecycle, review publication, queues, status, telemetry, or dashboard contracts.
Documentation Impact
No documentation update is needed because no product, operator, API, configuration, workflow, or integration contract changes.
Evidence
6007142e97bde18214d5a024c43433ed09df54d8e1eefe217304f0f510acb3890db1155cef61682e6714fca9b61cd14e35983515af58b186f2f7a92d- pass the checkout descriptor without a shell6007142e97bde18214d5a024c43433ed09df54d8- load runtime clock data from fixture state0; tests+54/-46(net+8)HOMEbehaviorbuild:allequivalent using the linked toolchain: passedcheckequivalent: static checks, formatting, builds, lint, and coverage thresholds passed; aggregate tests ended at 4,017 passed, 3 unrelated existing macOS/toolchain failures, and 9 skipped. The failures were Bash 3 lackingmapfile, login-shellHOMEbehavior, and repair target-validation commit plumbing.git diff --check: passedoxfmt --check: passed across 770 filesgpt-5.6-solhigh review: no findingsgpt-5.6-solhigh review: no findingspnpm check, Windows Codex launcher, sparse repair build smoke, and both CodeQL analysesReal Behavior Proof
Claim: The changed fixtures preserve their observable behavior while removing shell construction and dynamic executable-code generation.
Exercised surface: The built ClawSweeper runtime exercised by
test/apply-runtime-budget.test.tsandtest/live-proof-review-environment.test.ts.Scenario / fixture: Runtime-budget retry, pre-close, and post-close deadline cases; #1293's wrapper-death synchronization case; terminal cleanup with an unrelated process holding checkout directory fd 9; the complete two-file focused suite.
Command and environment: Fresh AWS Crabbox checkout of PR #1291 at exact head
6007142e97bde18214d5a024c43433ed09df54d8, followed by a frozen install,pnpm run build:all, and:Observed result: Build passed. All 34 tests passed, including all runtime clock call sites, wrapper-death synchronization, and the unrelated directory-descriptor cleanup case.
Artifact / trace: AWS lease
cbx_42ef63cefe10; runrun_b718d30f61b3; https://crabbox.openclaw.ai/portal/runs/run_b718d30f61b3Cleanup: The owned AWS lease was released immediately after the successful run.
Limits: This is test-only Linux proof and performs no live GitHub mutation beyond this PR. Post-merge default-branch alert closure requires merge approval and remains outside this draft executor pass.