Skip to content

test(WriteTool.write): File permissions respect user's umask#21233

Open
HaleTom wants to merge 2 commits intoanomalyco:devfrom
HaleTom:write-test-umask
Open

test(WriteTool.write): File permissions respect user's umask#21233
HaleTom wants to merge 2 commits intoanomalyco:devfrom
HaleTom:write-test-umask

Conversation

@HaleTom
Copy link
Copy Markdown

@HaleTom HaleTom commented Apr 6, 2026

Issue for this PR

Fixes #19076 while respecting user's umask

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

The write tool test sets file permissions when writing sensitive data fails on systems with a non-default umask (e.g. 0027). The test hardcodes 0o644 as the expected permission, but Filesystem.write() respects the system umask when no explicit mode is set.

This wraps the permission test in process.umask(0o022) / finally { process.umask(prevUmask) } so the assertion is deterministic regardless of the host environment.

Why force umask in the test, not in production code?

PR #19077 attempts to fix this by forcing 0o644 on all files via a post-write chmod to bypass umask filtering. This is a security regression — a user running umask 077 has intentionally restricted their environment so new files are 0600 (owner-only). Forcing 0o644 programmatically loosens their security boundary and makes all AI-written files world-readable, including .env files, credentials, or private keys.

You should never programmatically loosen a user's umask — it's a deliberate security boundary. The correct approach is to respect it, or chmod to something more restrictive (e.g., 0600 for sensitive files), never less.

With #14853 closed by its author, there's no remaining reason to override or loosen a user's umask in the write tool. I'd gently suggest closing both #19076 and #19077.

How did you verify your code works?

  • bun test test/tool/write.test.ts — all 13 tests pass
  • bun typecheck — no errors
  • Verified with umask 077 and umask 022 — both pass

Screenshots / recordings

N/A — no UI changes.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Copilot AI review requested due to automatic review settings April 6, 2026 19:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The following comment was made by an LLM, it may be inaccurate:

Potential Duplicate Found:

These two PRs are complementary—PR #21233 stabilizes the test, and PR #19077 fixes the production code. Check if both are needed or if there's overlap in scope.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Makes the tool.write permission-related test deterministic across environments by controlling the process umask during the assertion.

Changes:

  • Wraps the sensitive-file permission assertion in a process.umask(0o022) / restore block to avoid failures under non-default host umasks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@HaleTom HaleTom changed the title test(write): set umask to 0o022 for deterministic file permissions test(WriteTool.write): File permissions respect user's umask Apr 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

Hey! Your PR title test(WriteTool.write): File permissions respect user's umask doesn't follow conventional commit format.

Please update it to start with one of:

  • feat: or feat(scope): new feature
  • fix: or fix(scope): bug fix
  • docs: or docs(scope): documentation changes
  • chore: or chore(scope): maintenance tasks
  • refactor: or refactor(scope): code refactoring
  • test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

@HaleTom HaleTom force-pushed the write-test-umask branch 2 times, most recently from 9719a21 to 3fcdc4a Compare April 9, 2026 09:25
@HaleTom HaleTom force-pushed the write-test-umask branch from 3fcdc4a to 90f7ec2 Compare April 9, 2026 14:51
@github-actions github-actions bot mentioned this pull request Apr 9, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tool.write should enforce 0644 file mode despite umask

2 participants