Skip to content

build(deps): refresh Vitest, emnapi, and CodeQL - #1015

Merged
steipete merged 2 commits into
mainfrom
codex/fs-safe-round13
Oct 19, 2026
Merged

steipete merged 2 commits into
mainfrom
codex/fs-safe-round13

Conversation

@steipete

@steipete steipete commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Refresh Vitest and its coverage provider to 5.0.5, emnapi runtime from 2.0.0-beta.1 to beta.2, and the pinned CodeQL action to 4.39.0. All updates have cleared the repository's two-day cooldown; the Node 22 runtime floor stays intact. Consolidates #1012, #1013, and #1014; the lockfile updates originate in those Dependabot PRs.

Vitest 5.0.5 now clears an earlier skip when a later skipIf(false) is applied. Combine the platform and privilege conditions so POSIX permission assertions stay excluded on Windows and for root, while continuing to run for unprivileged POSIX users. The initial Windows Node 26 CI failure reproduced this compatibility issue; no filesystem assertion is weakened.

Validation: the documented full pnpm check passed on Crabbox, including 14,335 tests, build, docs, and installed-package checks. After adapting the skip predicate, 50 focused path-prefix tests passed on unprivileged macOS. Independent Codex reviews found no actionable P0–P2 findings; actionlint and git diff --check pass. The final branch includes the watch fix already merged to main, and exact-head cross-platform CI passed: https://github.com/openclaw/fs-safe/actions/runs/39318704788. Post-merge main CI and the unsigned release dry run also passed: https://github.com/openclaw/fs-safe/actions/runs/39323983897 and https://github.com/openclaw/fs-safe/actions/runs/39324022340.

No package version or release changes.

@steipete
steipete requested a review from a team as a code owner October 19, 2026 19:55
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T20:23:15.909892Z da571d2 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

Updates Vitest and V8 coverage to 5.0.5, emnapi runtime to beta.2, and the pinned CodeQL action to 4.39.0.

Example: Run pnpm test on Windows with the updated lockfile.

  • Before: Vitest 5.0.4 skips the ten POSIX-only path-prefix permission cases.
  • After: Vitest 5.0.5 overrides their platform exclusion and runs them, causing permission and path-separator failures.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) Useful dependency maintenance with one confirmed, narrow Windows validation regression.
Proof confidence 🌊 off-meta tidepool Not applicable: The supplied maintainer-authored classification places this maintenance PR outside the ordinary contributor proof gate; CI establishes the Windows defect, while the reported AWS Crabbox run cbx_c997edd3cd38 could not be inspected through its linked portal.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Product

Kind: Maintenance · Worth it: Yes · Fix scope: Complete
User problem: Contributors depend on compatible test, coverage, and scanning tools to validate filesystem safety.
Reason: The refresh follows established dependency maintenance and the supplied maintainer-authored direction, while preserving runtime requirements, cooldown policy, and public contracts.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep this PR open for repair: the dependency refresh has maintenance value, but the Vitest update introduces a confirmed Windows test-selection regression.

Priority: P2
Reviewed head: a8cf6dc8facf2726b9dc9946a5ae4ff8b5c24241

Before merge

  • Preserve Windows exclusions when upgrading Vitest (P2) - Vitest 5.0.5 makes skipIf(false) explicitly set skip:false. On Windows, itPosix initially skips tests, but process.getuid?.() === 0 evaluates false, so the chained skipIf calls at test/path-prefix.test.ts:255, 277, and 296 re-enable ten POSIX-only cases. All four failing Windows validation jobs report precisely those permission and separator assertions; corresponding base jobs passed. Combine the Windows and root-user predicates into a single exclusion in each declaration so the update preserves platform selection.

Findings

  • [P2] Preserve Windows exclusions when upgrading Vitest — pnpm-lock.yaml:210

Tests

  • Missing end-to-end proof: After repair, verify that Windows skips these ten cases while non-root POSIX runs them and root POSIX retains its permission exclusions.
Agent review details

How this fits together

The development dependencies run filesystem tests and collect coverage, while the CodeQL workflow scans repository code; their outputs are validation results rather than changes to the public filesystem API.

flowchart LR
  A[Package manifest] --> B[pnpm lockfile]
  B --> C[Vitest runner]
  C --> D[Platform test guards]
  D --> E[Filesystem tests and coverage]
  F[CodeQL workflow] --> G[Code scanning results]
Loading

Technical review

Best possible solution:

Keep the dependency refresh and express the combined Windows and root-user exclusion once in each of the three affected path-prefix test declarations.

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

The upstream runner change and existing chained guards explain all ten exact-head Windows failures, while equivalent base checks passed; no target code was executed locally.

Is this the best way to solve the issue?

Updating the matched tooling versions is reasonable, but the existing tests must adapt to Vitest's explicit skip override without weakening their assertions.

Full review comments:

  • [P2] Preserve Windows exclusions when upgrading Vitest — pnpm-lock.yaml:210
    Vitest 5.0.5 makes skipIf(false) explicitly set skip:false. On Windows, itPosix initially skips tests, but process.getuid?.() === 0 evaluates false, so the chained skipIf calls at test/path-prefix.test.ts:255, 277, and 296 re-enable ten POSIX-only cases. All four failing Windows validation jobs report precisely those permission and separator assertions; corresponding base jobs passed. Combine the Windows and root-user predicates into a single exclusion in each declaration so the update preserves platform selection.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5147654024a7.

Provenance checked

Testing

Proof path: unit tests only.

Security

None.

Evidence

Acceptance criteria:

  • On Windows: pnpm test test/path-prefix.test.ts
  • On non-root POSIX: pnpm test test/path-prefix.test.ts
  • On root POSIX: pnpm test test/path-prefix.test.ts
  • pnpm check
  • git diff --check

What I checked:

Likely related people:

  • Vincent Koc: Raw commit d140ddf adds .github/workflows/codeql.yml:46 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: d140ddfdbb33; files: .github/workflows/codeql.yml)

Labels

Label changes:

  • add P2: The update breaks Windows development and CI validation without changing the shipped filesystem API.
  • add merge-risk: 🚨 automation: The changed runner actively selects ten unsupported Windows test cases, causing four validation jobs to fail.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: The update breaks Windows development and CI validation without changing the shipped filesystem API.
  • merge-risk: 🚨 automation: The changed runner actively selects ten unsupported Windows test cases, causing four validation jobs to fail.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 19, 2026, 4:15 PM ET / 20:15 UTC.

steipete and others added 2 commits October 19, 2026 13:21
Consolidate the three pending Dependabot updates while preserving the
Node 22 runtime floor and two-day dependency cooldown.

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant