Skip to content

fix(test): keep CodeQL fixtures data-only - #1291

Draft
vincentkoc wants to merge 2 commits into
mainfrom
security/codeql-test-fixture-data-boundaries-20260829
Draft

fix(test): keep CodeQL fixtures data-only#1291
vincentkoc wants to merge 2 commits into
mainfrom
security/codeql-test-fixture-data-boundaries-20260829

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Aug 29, 2026

Copy link
Copy Markdown
Member

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 main at e1eefe217304f0f510acb3890db1155cef61682e.

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

  • Head: 6007142e97bde18214d5a024c43433ed09df54d8
  • Base: e1eefe217304f0f510acb3890db1155cef61682e
  • Commits:
    • 6714fca9b61cd14e35983515af58b186f2f7a92d - pass the checkout descriptor without a shell
    • 6007142e97bde18214d5a024c43433ed09df54d8 - load runtime clock data from fixture state
  • LOC: production 0; tests +54/-46 (net +8)
  • Rebase overlap: fix: require complete secret scans for hosted reviews #1293's wrapper-death synchronization test is byte-for-byte preserved; fd 9 changes are confined to the following unrelated-directory-descriptor test
  • Focused local changed-surface tests: 5/5 passed
  • Focused local two-file suite: all changed behavior passed; the only failure was the existing macOS login-shell HOME behavior
  • Local build:all equivalent using the linked toolchain: passed
  • Local check equivalent: 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 lacking mapfile, login-shell HOME behavior, and repair target-validation commit plumbing.
  • git diff --check: passed
  • Full oxfmt --check: passed across 770 files
  • Test audit: existing tests uniquely cover runtime deadline/yield and process-cleanup invariants; this changes fixture construction only and adds no production seam, export, flag, dependency, configuration, or duplicate test
  • Pre-final Codex gpt-5.6-sol high review: no findings
  • Exact committed-head Codex gpt-5.6-sol high review: no findings
  • Exact-head ClawSweeper review: no implementation or security findings; its sole rank-up move was refreshing this current-head evidence
  • Exact-head GitHub CI: passed, including pnpm check, Windows Codex launcher, sparse repair build smoke, and both CodeQL analyses
  • PR CodeQL state: zero open alerts for PR fix(test): keep CodeQL fixtures data-only #1291
  • Secret/private-data scrub: TruffleHog found zero verified or unverified secrets; the diff contains no private paths, hosts, IPs, emails, tokens, or logs

Real 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.ts and test/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:

node --test --test-concurrency=1 \
  test/apply-runtime-budget.test.ts \
  test/live-proof-review-environment.test.ts

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; run run_b718d30f61b3; https://crabbox.openclaw.ai/portal/runs/run_b718d30f61b3

Cleanup: 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.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 29, 2026, 7:40 AM ET / 11:40 UTC.

ClawSweeper review

What this changes

This 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 readiness

⚠️ Ready for maintainer review - 2 items remain

The 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
Reviewed head: 6007142e97bde18214d5a024c43433ed09df54d8
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A narrow, coherent test-fixture refactor with source-supported invariants and current-head focused proof; no actionable implementation or security defect was found.
Proof confidence 🌊 off-meta tidepool Not applicable: This MEMBER-authored test-only PR is exempt from the external-contributor proof gate; its current body nevertheless supplies an exact-head AWS Crabbox trace for the built test surface, reporting all 34 focused tests passing.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This MEMBER-authored test-only PR is exempt from the external-contributor proof gate; its current body nevertheless supplies an exact-head AWS Crabbox trace for the built test surface, reporting all 34 focused tests passing.
Evidence reviewed 5 items Pinned introduced delta: The verified merge-base-to-head delta changes exactly two test files; no production, workflow, dependency, or configuration files are introduced.
Data-only runtime clock fixture: The fixed preload reads the sibling JSON state file and the helper writes variable timestamps and paths as serialized fixture data, preserving the existing assertions without interpolating them into executable source.
Direct descriptor inheritance: The terminal-cleanup fixture opens the checkout, passes that descriptor as child fd 9, and closes only the parent copy after spawn; the child still verifies fd 9 is a directory before reporting readiness.
Findings None None.
Security None None.

How this fits together

The 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]
Loading

Decision needed

Question Recommendation
Should a project maintainer accept this current-head test-fixture cleanup and move the draft forward? Accept the fixture cleanup: Confirm that the static preload plus JSON state and direct descriptor inheritance are the desired CodeQL-safe fixture patterns, then proceed with normal landing review.

Why: Repository policy keeps MEMBER-authored items open for explicit maintainer judgment even when the patch and evidence are satisfactory.

Before merge

  • Complete next step (P2) - No mechanical repair is needed; explicit maintainer review is required because the PR is MEMBER-authored and remains a draft.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed-surface LOC production +0/-0; tests +54/-46 across 2 files The patch is narrowly limited to test-fixture construction while preserving existing behavioral assertions.

Technical review

Best 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.

Labels

Label justifications:

  • P3: This is low-risk test-fixture maintenance with no user-facing runtime or configuration change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This MEMBER-authored test-only PR is exempt from the external-contributor proof gate; its current body nevertheless supplies an exact-head AWS Crabbox trace for the built test surface, reporting all 34 focused tests passing.

Evidence

What I checked:

Likely related people:

  • Vincent Koc: Current file history associates Vincent Koc with earlier terminal-proof work and the two current fixture commits. (role: recent area contributor; confidence: high; commits: d103c0cf5c98, 6714fca9b61c, 6007142e97bd; files: test/live-proof-review-environment.test.ts, test/apply-runtime-budget.test.ts)
  • Peter Steinberger: Current history connects Peter Steinberger to the existing deadline scenarios and terminal-cleanup implementation that these fixtures exercise. (role: recent area contributor; confidence: high; commits: 5ada40c98ae6, 2a5c59f9d5e5, e1eefe217304; files: test/apply-runtime-budget.test.ts, test/live-proof-review-environment.test.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-29T02:58:08.166Z sha 62c9ef0 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-29T03:06:11.041Z sha 62c9ef0 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-29T11:32:48.853Z sha 6007142 :: needs maintainer review before merge. :: none

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 29, 2026
@vincentkoc
vincentkoc force-pushed the security/codeql-test-fixture-data-boundaries-20260829 branch from 62c9ef0 to 6007142 Compare August 29, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant