Skip to content

refactor(test): deduplicate filesystem fixtures - #121

Merged
steipete merged 2 commits into
mainfrom
refactor/test-suite-dedup
Aug 4, 2026
Merged

refactor(test): deduplicate filesystem fixtures#121
steipete merged 2 commits into
mainfrom
refactor/test-suite-dedup

Conversation

@steipete

@steipete steipete commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Reduces duplicated test infrastructure accumulated during the recent filesystem stress work. Three separate TAR encoders and repeated native, temporary-directory, open-handle, and matrix setup made the suite harder to review without adding distinct behavioral proof.

Why This Change Was Made

One shared TAR fixture now owns archive construction, existing temporary-directory fixtures own cleanup, and repeated mode and error cases are data-driven. The repair on current main deliberately preserves the distinct FileStore serializer-failure, private-file mode, durable-queue mode, and readable-by-others permission regressions identified during review.

The final branch changes tests only. There is no public API or runtime behavior change.

User Impact

No user-visible behavior changes. The patch removes 377 lines and adds 194 across nine test files while retaining the security, durability, native, archive, lock, and concurrency behavior checks.

Evidence

  • pnpm check: 100 files passed, 1,033 tests passed, 61 platform or optional skips; build, docs, package, filesystem-boundary, and file-size checks passed.

  • git diff --check passed.

  • Codex autoreview of the repair and complete branch: TruffleHog clean; no accepted or actionable findings (0.99).

  • Exact-head CI: full platform, native, Cargo, package-smoke, and workflow checks passed.

  • CodeQL and benchmarks passed.

  • Native-enabled coverage passed on retry. The first attempt hit a single 5-second timeout in native-publish-equivalence.test.ts; the unchanged exact head passed on rerun.

  • Distinct regression coverage retained

  • Security and compatibility impact considered

  • Changelog intentionally unchanged because behavior is unchanged

  • No credentials, private paths, private hosts, or sensitive contents included

@steipete
steipete requested a review from a team as a code owner August 3, 2026 16:31
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Aug 3, 2026
@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 4, 2026, 6:22 PM ET / 22:22 UTC.

ClawSweeper review

What this changes

Consolidates duplicated TAR, temporary-directory, permission, lock, and path test fixtures while preserving filesystem safety regression cases.

Merge readiness

Blocked until real behavior proof from a real setup is added - 3 items remain

Keep open: the refactor appears to retain the previously flagged serializer-failure regression coverage, but it still needs inspectable real-checkout proof and resolution of the failing Node 22 coverage check before merge.

Priority: P2
Reviewed head: 0a5da7567a42236ea1dfe42ca98269386b204ca1

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch is focused and appears to preserve the previously flagged coverage, but real behavior proof remains a merge gate.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body lists automated checks but has no inspectable after-fix transcript, live output, artifact, recording, screenshot, or redacted log from a real checkout; add redacted proof and request re-review if needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body lists automated checks but has no inspectable after-fix transcript, live output, artifact, recording, screenshot, or redacted log from a real checkout; add redacted proof and request re-review if needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Focused test-only diff: The final branch changes nine test and test-helper files; the shared TAR builder centralizes fixture construction without changing package, workflow, or runtime source files.
Earlier regression finding is retained: The final branch keeps the queue serializer-failure assertion that verifies a circular value does not replace the last complete entry, so the previous review finding is not present at this head.
Feature history: The current archive-fuzz helper originated with the recent path-and-archive hardening work, making its author the clearest history-based routing candidate for this consolidation.
Findings None None.
Security None None.

How this fits together

The test suite exercises fs-safe’s guarded archive, file-store, lock, permission, and path primitives across JavaScript and native backends. Its fixtures create adversarial filesystem inputs and assert that the public APIs fail safely.

flowchart LR
  A[Archive and filesystem scenarios] --> B[Shared test fixtures]
  B --> C[Guarded filesystem primitives]
  C --> D[JavaScript backend]
  C --> E[Native backend]
  D --> F[Safety regression assertions]
  E --> F
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body lists automated checks but has no inspectable after-fix transcript, live output, artifact, recording, screenshot, or redacted log from a real checkout; add redacted proof and request re-review if needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Consolidating security-boundary fixtures can silently weaken regression coverage; current evidence is limited to claimed automated results, while the Node 22 coverage job is failing.
  • Complete next step (P2) - Contributor-provided real behavior proof and a resolved coverage result are needed; no narrow code repair is established.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Test-suite delta 9 test files; +194, -377 lines The net reduction consolidates fixtures across archive, permission, lock, native, and store safety regression coverage.

Merge-risk options

Maintainer options:

  1. Prove the final test matrix (recommended)
    Attach a redacted terminal transcript or live output from the final head showing the refactored checks running, and resolve the Node 22 coverage failure before merge.

Technical review

Best possible solution:

Keep one shared fixture layer, then provide redacted execution proof from the final branch and resolve the coverage failure before landing.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a test-fixture refactor rather than a report of broken user behavior; the relevant verification is execution of the refactored safety suite.

Is this the best way to solve the issue?

Unclear: the shared fixtures are a maintainable direction and the prior serializer-coverage concern is retained, but final-head execution proof and the failing coverage job must establish that coverage remains equivalent.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fe009ad3422a.

Labels

Label justifications:

  • P2: This is a bounded test-infrastructure change on security-sensitive filesystem coverage, with no demonstrated user-facing regression.
  • merge-risk: 🚨 security-boundary: The refactor changes tests that protect path, archive, permission, lock, and store safety contracts, so coverage equivalence needs demonstrated execution.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists automated checks but has no inspectable after-fix transcript, live output, artifact, recording, screenshot, or redacted log from a real checkout; add redacted proof and request re-review if needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Focused test-only diff: The final branch changes nine test and test-helper files; the shared TAR builder centralizes fixture construction without changing package, workflow, or runtime source files. (test/helpers/archive-fuzz.ts:12, 0a5da7567a42)
  • Earlier regression finding is retained: The final branch keeps the queue serializer-failure assertion that verifies a circular value does not replace the last complete entry, so the previous review finding is not present at this head. (test/store-stress.test.ts:104, 0a5da7567a42)
  • Feature history: The current archive-fuzz helper originated with the recent path-and-archive hardening work, making its author the clearest history-based routing candidate for this consolidation. (test/helpers/archive-fuzz.ts:12, ed06666a28eb)
  • Execution evidence remains insufficient: The PR body reports automated checks but provides no inspectable terminal transcript, live output, artifact, recording, or redacted runtime log; the supplied check state also shows Node 22 coverage failing on this head. (0a5da7567a42)

Likely related people:

  • steipete: Current helper and archive regression coverage trace to the path-and-archive hardening commit, and this person also authored the current refactor branch. (role: introduced archive regression harness and recent area contributor; confidence: high; commits: ed06666a28eb; files: test/helpers/archive-fuzz.ts, test/archive-read-boundaries.test.ts, test/native-archive-equivalence.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach redacted terminal or live output from the final branch showing the relevant checks running.
  • Resolve the failing Node 22 coverage check and include its result in the proof.

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-03T16:35:35.271Z sha 2b6b100 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T16:40:09.938Z sha 2b6b100 :: needs real behavior proof before merge. :: [P2] Preserve FileStore serializer-failure coverage
  • reviewed 2026-08-03T20:22:54.010Z sha 2b6b100 :: needs real behavior proof before merge. :: [P2] Preserve FileStore serializer-failure coverage

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Aug 3, 2026
@steipete
steipete force-pushed the refactor/test-suite-dedup branch from 2b6b100 to 0a5da75 Compare August 4, 2026 22:19
@steipete
steipete merged commit d463414 into main Aug 4, 2026
25 of 26 checks passed
@steipete
steipete deleted the refactor/test-suite-dedup branch August 4, 2026 22:25
@steipete

steipete commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Landed as d4634144acc37370d6730a6b43e9004fe26207c6.

Verification performed on exact head 0a5da7567a42236ea1dfe42ca98269386b204ca1:

  • Local pnpm check: 100 test files passed, 1,033 tests passed, 61 platform or optional skips; build, docs, package, filesystem-boundary, and file-size checks passed.
  • Local git diff --check passed.
  • Codex autoreview ran on both the focused repair and complete branch; TruffleHog was clean and neither run reported an accepted or actionable finding.
  • Full CI passed across Node 22 and 24 on Linux, macOS, and Windows; native checks passed on Linux, macOS, Windows, and musl; Cargo audit/clippy, workflow lint, benchmarks, CodeQL, and bundled-package smoke tests passed: https://github.com/openclaw/fs-safe/actions/runs/30955950691
  • Native-enabled coverage passed: https://github.com/openclaw/fs-safe/actions/runs/30955950715

The first coverage attempt hit one 5-second timeout in native-publish-equivalence.test.ts; the unchanged exact head passed when that failed job was rerun. No production source or public behavior changed.

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

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant