Skip to content

fix: map EACCES/EPERM to permission denied in pinned writes - #76

Open
NanoRisk6 wants to merge 2 commits into
openclaw:mainfrom
NanoRisk6:cat/pinned-write-permission-errors-115920
Open

fix: map EACCES/EPERM to permission denied in pinned writes#76
NanoRisk6 wants to merge 2 commits into
openclaw:mainfrom
NanoRisk6:cat/pinned-write-permission-errors-115920

Conversation

@NanoRisk6

@NanoRisk6 NanoRisk6 commented Jul 30, 2026

Copy link
Copy Markdown

What Problem This Solves

Pinned writes that fail with EACCES / EPERM were reported as “path is not a regular file under root”. Agents and operators then chase path/symlink bugs instead of fixing permissions. Fixes openclaw/openclaw#115920 (fs-safe error mapping).

Why This Change Was Made

normalizePinnedWriteError now maps common permission errno values to a clear “permission denied” style message while leaving real path/root failures on their existing messages.

User Impact

Permission failures surface as permission failures. Agents stop misdiagnosing ordinary ACL/ownership problems as “not a regular file”.

Evidence

  • Unit coverage for EACCES / EPERM mapping on in-root regular files.
  • Unchanged messages for true path/root validation failures.
  • Focused tests for the pinned-write error normalizer.

Real behavior proof (after-fix)

Local macOS Node v26 — in-root pinned write against a non-writable workspace directory (mode 0555). Paths redacted.

proof: platform darwin node v26.3.1
proof: root path redacted as <workspace>
proof: root open ok
proof: attempting pinned write to notes.txt (dir mode 0555)
--- after-fix error ---
name: FsSafeError
code: denied-path
message: permission denied
cause.code: EACCES
cause.message: EACCES: permission denied, open '<path>'
PROOF_OK: permission failures surface as permission denied

Harness (repro):

PROOF_ROOT=$(mktemp -d /tmp/fs-safe-proof.XXXXXX)/workspace
mkdir -p "$PROOF_ROOT" && echo before > "$PROOF_ROOT/notes.txt" && chmod 555 "$PROOF_ROOT"
# pnpm build && PROOF_ROOT=$PROOF_ROOT node --input-type=module <<'JS'
# import { root } from './dist/index.js';
# const fs = await root(process.env.PROOF_ROOT);
# await fs.write('notes.txt', 'after\n'); // throws FsSafeError denied-path / permission denied
# JS

@clawsweeper re-review

Follow-up fix (semantics)

ClawSweeper correctly flagged that denied-path is policy (denyMutations), not OS permissions.

Head now maps:

  • EACCES / EPERMFsSafeError("helper-failed", "permission denied") · category: operational
  • EROFSFsSafeError("helper-failed", "read-only filesystem")
  • ENOSPC unchanged → helper-failed

Still not "path is not a regular file under root".

Real behavior proof (after semantics fix)

Local macOS Node — in-root pinned write, workspace dir mode 0555. Paths redacted.

code: helper-failed
message: permission denied
category: operational
cause.code: EACCES
PROOF_OK

Unit: pnpm exec vitest run test/edge-coverage.test.ts — pass.

@clawsweeper re-review

normalizePinnedWriteError previously reported every unrecognized write
error as "path is not a regular file under root", which misdiagnosed
ordinary permission failures (openclaw/openclaw#115920).

Map EACCES/EPERM/EROFS/ENOSPC/EISDIR/ENOENT to accurate FsSafeError kinds,
and append errno codes on remaining fallbacks.
@NanoRisk6
NanoRisk6 requested a review from a team as a code owner July 30, 2026 02: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. 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 Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed July 30, 2026, 9:37 AM ET / 13:37 UTC.

ClawSweeper review

What this changes

The branch maps pinned-write permission, read-only filesystem, space, directory, and missing-file OS errors to specific public FsSafeError results instead of the generic invalid-path result.

Merge readiness

⚠️ Needs maintainer review before merge - 2 items remain

This PR addresses a real pinned-write diagnostic bug and includes credible after-fix local proof, but it still changes documented public error classifications without the required Unreleased compatibility note. Keep it open for that narrow repair before merge.

Priority: P2
Reviewed head: 15951cf91aff7fb34131e150d21f27f8b69a1262

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The behavioral evidence is strong and the semantic correction is focused, but the missing required compatibility note prevents a merge-ready rating.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR provides a redacted after-fix macOS terminal transcript for a real in-root EACCES pinned write, showing the corrected helper-failed operational result; the reporter also confirmed it matches the production failure.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR provides a redacted after-fix macOS terminal transcript for a real in-root EACCES pinned write, showing the corrected helper-failed operational result; the reporter also confirmed it matches the production failure.
Evidence reviewed 5 items Changed public contract: The PR changes several externally observable pinned-write outcomes from invalid-path to helper-failed, not-file, and not-found; repository policy treats exported error codes and categories as compatibility surfaces.
Focused coverage: The accompanying test additions verify EACCES, EPERM, EROFS, ENOSPC, and an unmapped EIO fallback, including the corrected permission message.
Repository policy: The fully read target AGENTS.md requires an Unreleased CHANGELOG.md entry for public, compatibility, package, or operational changes and identifies error codes as public compatibility surfaces.
Findings 1 actionable finding [P2] Document the changed public error contract
Security None None.

How this fits together

Pinned writes operate on files whose location and identity were checked under a capability-style filesystem root. When the final operating-system write fails, the error normalizer selects the public error code and message that agents and operators use to diagnose the failure.

flowchart LR
  A[Caller pinned write] --> B[Root-confined file operation]
  B --> C[Operating system write result]
  C --> D[Error normalization]
  D --> E[Public FsSafeError]
  E --> F[Agent or operator diagnosis]
Loading

Before merge

  • Document the changed public error contract (P2) - Add an Unreleased CHANGELOG.md compatibility note for the changed pinned-write FsSafeError classifications. This branch changes externally visible outcomes from invalid-path to helper-failed, not-file, and not-found; the repository policy explicitly treats error codes and categories as public compatibility surfaces. This is the still-unfixed finding from the prior review at the same head SHA.
  • Resolve merge risk (P1) - Merging without an Unreleased compatibility note leaves downstream callers unaware that pinned writes can now emit different public error codes and categories for the same underlying OS failures.

Findings

  • [P2] Document the changed public error contract — src/root-errors.ts:33-48
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 files affected; 68 added, 2 removed A small code-and-test patch changes multiple public error outcomes, so release communication matters more than diff size.
Mapped outcomes 5 explicit errno cases plus fallback The normalizer now distinguishes permission, read-only, space, directory, missing-file, and unknown write failures.

Merge-risk options

Maintainer options:

  1. Document the public error change (recommended)
    Add an Unreleased compatibility note that names the affected pinned-write error codes and messages, then rerun the focused test and repository checks.
  2. Accept an undocumented contract change
    Merge the corrected behavior without a release note only if maintainers explicitly accept that downstream error-code consumers receive no upgrade notice.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Add an Unreleased CHANGELOG compatibility note for pinned-write error reclassification, then run the focused test, `pnpm check`, and `git diff --check`.

Technical review

Best possible solution:

Retain the corrected operational mappings, add a concise Unreleased note describing the changed pinned-write error classifications, and keep the existing focused coverage and real filesystem proof as the regression record.

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

Yes, at source level and with credible real-run evidence: a pinned write to an in-root file under a non-writable directory produces EACCES/EPERM and exercises this normalizer. The PR includes a redacted macOS transcript and a reproducible harness for the corrected result.

Is this the best way to solve the issue?

Mostly yes: distinguishing operational permission failures from path-validation failures is the narrow maintainable fix, and preserving denied-path for denyMutations keeps the existing policy contract intact. The remaining required improvement is documenting the public reclassification for upgrades.

Full review comments:

  • [P2] Document the changed public error contract — src/root-errors.ts:33-48
    Add an Unreleased CHANGELOG.md compatibility note for the changed pinned-write FsSafeError classifications. This branch changes externally visible outcomes from invalid-path to helper-failed, not-file, and not-found; the repository policy explicitly treats error codes and categories as public compatibility surfaces. This is the still-unfixed finding from the prior review at the same head SHA.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a bounded correction to user-visible error reporting with a concrete compatibility follow-up.
  • merge-risk: 🚨 compatibility: The patch changes public FsSafeError codes and categories that downstream callers may inspect.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR provides a redacted after-fix macOS terminal transcript for a real in-root EACCES pinned write, showing the corrected helper-failed operational result; the reporter also confirmed it matches the production failure.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR provides a redacted after-fix macOS terminal transcript for a real in-root EACCES pinned write, showing the corrected helper-failed operational result; the reporter also confirmed it matches the production failure.

Evidence

Acceptance criteria:

  • [P1] pnpm exec vitest run test/edge-coverage.test.ts.
  • [P1] pnpm check.
  • [P1] git diff --check.

What I checked:

  • Changed public contract: The PR changes several externally observable pinned-write outcomes from invalid-path to helper-failed, not-file, and not-found; repository policy treats exported error codes and categories as compatibility surfaces. (src/root-errors.ts:33, 15951cf91aff)
  • Focused coverage: The accompanying test additions verify EACCES, EPERM, EROFS, ENOSPC, and an unmapped EIO fallback, including the corrected permission message. (test/edge-coverage.test.ts:62, 15951cf91aff)
  • Repository policy: The fully read target AGENTS.md requires an Unreleased CHANGELOG.md entry for public, compatibility, package, or operational changes and identifies error codes as public compatibility surfaces. (AGENTS.md:30, ab933820c089)
  • Prior review continuity: The latest completed ClawSweeper review at the same head SHA raised the missing Unreleased compatibility note; the current PR file list still contains only the implementation and test files. (CHANGELOG.md:1, 15951cf91aff)
  • Real behavior confirmation: The PR body and follow-up comment provide a redacted macOS pinned-write transcript showing an in-root EACCES now returns helper-failed with permission denied; the production reporter confirmed that this matches the observed ownership-drift failure. (15951cf91aff)

Likely related people:

  • NanoRisk6: Authored both commits on the reviewed head and corrected the error-code semantics after the earlier review identified that denied-path is reserved for mutation policy. (role: current patch contributor; confidence: medium; commits: 2df41adc22cc, 15951cf91aff; files: src/root-errors.ts, test/edge-coverage.test.ts)
  • 609NFT: Provided the concrete production confirmation that an ordinary in-root EACCES was misclassified, making this person a useful validation contact for the observable behavior. (role: production-behavior reporter; confidence: medium)

Rank-up moves

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

  • Add an Unreleased CHANGELOG.md note describing the changed pinned-write error classifications.
  • Run the focused test, pnpm check, and git diff --check after the documentation update.

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 (6 earlier review cycles)
  • reviewed 2026-07-30T02:35:04.465Z sha 2df41ad :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-30T02:56:27.883Z sha 2df41ad :: needs real behavior proof before merge. :: [P1] Keep raw errno out of unrelated FsSafeError codes
  • reviewed 2026-07-30T03:24:43.411Z sha 15951cf :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T03:35:36.551Z sha 15951cf :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T11:51:54.952Z sha 15951cf :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T12:21:48.081Z sha 15951cf :: needs changes before merge. :: [P2] Document the changed public error contract

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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. labels Jul 30, 2026
@NanoRisk6

Copy link
Copy Markdown
Author

After-fix real behavior proof

Pinned write to an in-root file under a non-writable workspace dir (chmod 555) now surfaces as permission failure (not “path is not a regular file under root”).

proof: platform darwin node v26.3.1
proof: root path redacted as <workspace>
proof: root open ok
proof: attempting pinned write to notes.txt (dir mode 0555)
--- after-fix error ---
name: FsSafeError
code: denied-path
message: permission denied
cause.code: EACCES
cause.message: EACCES: permission denied, open '<path>'
PROOF_OK: permission failures surface as permission denied

PR body updated with the same transcript + repro harness.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
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 proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 30, 2026
denied-path is reserved for denyMutations policy (docs/errors.md).
EACCES/EPERM/EROFS are operational I/O failures — same family as ENOSPC.
Messages stay clear; codes stay honest.
@NanoRisk6

Copy link
Copy Markdown
Author

Semantics fix + new proof

Code contract correction (per prior ClawSweeper review):

errno before (wrong) now
EACCES/EPERM denied-path helper-failed (operational)
EROFS denied-path helper-failed
(policy denyMutations) denied-path unchanged

denied-path stays reserved for denyMutations only.

After-fix real pinned write

code: helper-failed
message: permission denied
category: operational
cause.code: EACCES
PROOF_OK

Workspace dir mode 0555, in-root notes.txt write via root().write. Paths redacted.

Tests: vitest run test/edge-coverage.test.ts

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
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. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 30, 2026
@609NFT

609NFT commented Jul 30, 2026

Copy link
Copy Markdown

Reporter of openclaw/openclaw#115920 here. This matches the failure we hit in production: an EACCES on an ordinary, in-root regular file (workspace ownership drift) was surfaced as "path is not a regular file under root", which sent our debugging in exactly the wrong direction for a while.

The mapping table covers every case we have actually seen (EACCES/EPERM and EROFS in containerized workspaces), and appending the errno to the invalid-path fallback means even an unmapped code stays diagnosable. LGTM from the reporting side.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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. labels Jul 30, 2026
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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

[Bug]: fs-safe reports EACCES as "path is not a regular file under root" (normalizePinnedWriteError catch-all, cf #72362)

2 participants