Skip to content

feat(filename): suffix Windows reserved names - #83

Merged
steipete merged 1 commit into
mainfrom
feat/windows-reserved-filenames
Aug 2, 2026
Merged

feat(filename): suffix Windows reserved names#83
steipete merged 1 commit into
mainfrom
feat/windows-reserved-filenames

Conversation

@steipete

@steipete steipete commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • suffix Windows reserved basenames with _ while preserving original case and extensions on every platform
  • share the exported WINDOWS_RESERVED_DEVICE_NAMES set between device-path checks and filename sanitization
  • cover every shared reserved name, mixed case, extensions, dollar names, superscript variants, truncation ordering, fallback behavior, and idempotence
  • update the filename contract documentation and credit @SebTardif's original idea from fix(path): reject Windows reserved basenames in sanitizeUntrustedFileName #67

Approved contract

Examples:

  • CONCON_
  • nul.txtnul_.txt
  • aux.caux_.c
  • conin$conin$_

Detection is case-insensitive, transformation preserves case, the suffix is inserted before the first extension, and the result is then capped at 200 characters. Empty, ., and .. results retain the existing fallback behavior.

Proof

  • Focused: pnpm exec vitest run test/filename.test.ts test/device-path.test.ts — 47 passed
  • Full gate: pnpm check — 645 passed, 25 skipped; packaged-consumer check passed
  • Security: pnpm test:security — 62 passed
  • Docs: pnpm docs:site — built successfully
  • Built output: { CON: 'CON_', 'nul.txt': 'nul_.txt', 'aux.c': 'aux_.c' }
  • Autoreview: clean, no accepted/actionable findings

Release note

This portability-contract addition makes the next release minor: 0.6.0. No release is authorized until Peter gives the release word.

@steipete
steipete requested a review from a team as a code owner August 2, 2026 03:07
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 2, 2026
@clawsweeper

clawsweeper Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 1, 2026, 11:11 PM ET / August 2, 2026, 03:11 UTC.

ClawSweeper review

What this changes

The PR changes sanitizeUntrustedFileName() to append _ to Windows-reserved device basenames before their extension, using the shared reserved-name set and documenting/testing the new portability contract.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep this PR open for normal maintainer review. Current main still returns Windows reserved basenames unchanged, while this focused patch adds the documented portability behavior, shares the existing reserved-name source of truth, and supplies broad regression coverage plus built-package output.

Priority: P2
Reviewed head: 5ca756d26b080bacf0f27311d9db55a96d1501da

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, documented portability-contract change with broad derived test coverage and credible built-package proof.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.
Evidence reviewed 6 items Current-main behavior remains unchanged: Current main sanitizes path syntax, control characters, and invalid Windows characters, then returns the resulting basename without checking Windows reserved device names; the requested behavior is therefore not already implemented.
Focused proposed implementation: The branch imports the existing Windows reserved-name set, checks the name before its first extension case-insensitively, inserts _, and then applies the existing 200-character cap.
Regression coverage and documentation align: The patch tests every shared reserved name, mixed case, extensions, dollar names, superscript variants, truncation ordering, and idempotence, while documenting the same ordered transformation.
Findings None None.
Security None None.

How this fits together

The filename sanitizer reduces attacker-controlled input to one portable filename segment before external-output and sibling-temp helpers use it in filesystem operations. The device-path module already defines Windows device names for guarded reads; this PR reuses that data to prevent sanitized names from resolving to Windows devices.

flowchart LR
  A[Untrusted filename] --> B[Filename sanitizer]
  B --> C[Character and path cleanup]
  C --> D[Windows reserved-name check]
  D --> E[Portable filename segment]
  E --> F[Output and temp-file helpers]
  E --> G[Filesystem write target]
Loading

Before merge

  • Resolve merge risk (P1) - Merging intentionally changes a public sanitizer result on every platform: callers that previously received CON or nul.txt will instead receive CON_ or nul_.txt; the compatibility change is documented and should remain a conscious release-note decision.
  • Complete next step (P2) - No discrete repair is needed; this is a focused feature PR awaiting normal maintainer merge review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 5 files affected; 79 additions, 12 deletions The focused change updates implementation, shared data visibility, regression tests, documentation, and the unreleased release note.
Reserved-name coverage All 31 shared names plus mixed-case and extension variants The tests derive cases from the same set used by guarded device-path handling, reducing list drift.

Merge-risk options

Maintainer options:

  1. Accept the documented filename change (recommended)
    Merge the suffixing contract as the intended portability behavior, with the release note clearly calling out changed results for Windows-reserved basenames.

Technical review

Best possible solution:

Land the shared reserved-name suffix rule with its focused tests and documentation so untrusted filename inputs remain usable while avoiding Windows device basenames across platforms.

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

Not applicable: this PR adds an explicitly described filename-portability behavior rather than repairing a current-main reproduction.

Is this the best way to solve the issue?

Yes. Reusing the device-path reserved-name set avoids divergent Windows-name lists, and suffixing preserves a usable filename segment while preventing a reserved basename from reaching later filesystem helpers.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a bounded public portability improvement to filename sanitization with limited but real downstream behavior change.
  • add merge-risk: 🚨 compatibility: Existing callers may observe a different sanitized filename for Windows-reserved basenames after upgrade.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.

Label justifications:

  • P2: This is a bounded public portability improvement to filename sanitization with limited but real downstream behavior change.
  • merge-risk: 🚨 compatibility: Existing callers may observe a different sanitized filename for Windows-reserved basenames after upgrade.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix built-package output for representative reserved names plus focused, full-check, security, documentation-build, and packaged-consumer results.

Evidence

What I checked:

  • Current-main behavior remains unchanged: Current main sanitizes path syntax, control characters, and invalid Windows characters, then returns the resulting basename without checking Windows reserved device names; the requested behavior is therefore not already implemented. (src/filename.ts:5, 0965b89390ad)
  • Focused proposed implementation: The branch imports the existing Windows reserved-name set, checks the name before its first extension case-insensitively, inserts _, and then applies the existing 200-character cap. (src/filename.ts:5, 5ca756d26b08)
  • Regression coverage and documentation align: The patch tests every shared reserved name, mixed case, extensions, dollar names, superscript variants, truncation ordering, and idempotence, while documenting the same ordered transformation. (test/filename.test.ts:29, 5ca756d26b08)
  • Feature-history provenance: Blame attributes the current filename sanitizer and reserved-name set to the v0.5.1 release commit, and the same current-area author proposed this follow-up contract. (src/device-path.ts:32, 16e1bd489ae8)
  • Not implemented or released yet: The PR head is not an ancestor of current main; the checked-out latest release is v0.5.1 at 16e1bd4, so this feature has neither landed on main nor shipped. (CHANGELOG.md:4, 0965b89390ad)
  • Related prior proposal: The PR explicitly credits the closed, unmerged related proposal at fix(path): reject Windows reserved basenames in sanitizeUntrustedFileName #67. That proposal used fallback output; this PR deliberately chooses a suffix-preserving portability contract instead. (CHANGELOG.md:5, 5ca756d26b08)

Likely related people:

  • steipete: Current-main blame attributes the filename sanitizer and Windows reserved-name set to Peter Steinberger’s v0.5.1 release commit; he also authored the focused follow-up branch. (role: current implementation author and recent area contributor; confidence: high; commits: 16e1bd489ae8, 5ca756d26b08; files: src/filename.ts, src/device-path.ts, test/filename.test.ts)
  • SebTardif: The related closed proposal at fix(path): reject Windows reserved basenames in sanitizeUntrustedFileName #67 identified the same reserved-name portability problem and is credited in this PR’s release note. (role: related prior proposal author; confidence: medium; commits: c47b4e59718f; files: src/filename.ts, test/filename.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.

@steipete
steipete merged commit 2477f56 into main Aug 2, 2026
29 checks passed
@steipete
steipete deleted the feat/windows-reserved-filenames branch August 2, 2026 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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