fix: readOnlyViolation cannot see writes to already-dirty paths - #38
fix: readOnlyViolation cannot see writes to already-dirty paths#38webdivs wants to merge 1 commit into
Conversation
`readOnlyViolation` compares git porcelain before and after a run, which cannot
see a write to a path that was already dirty at dispatch: " M src/app.js" before
and " M src/app.js" after are the same line. claude-delegate's own header has
documented this ("a tripwire, not an OS boundary: changes within an already-dirty
file can evade it"), and it bites in the normal case -- asking a read-only
question while you have work in progress is exactly when every dirty path is
invisible to the check.
Alongside the porcelain comparison, a read-only run now fingerprints the contents
of the paths that are ALREADY dirty and compares them again afterwards. Only that
set is covered: a path clean at dispatch surfaces as a new porcelain line anyway,
and fingerprinting a whole repository per run would cost far more than the case it
covers.
The verdict becomes properly three-valued, with proof beating absence of proof:
a detected write wins even when the other signal is unknown; null is reserved for
"coverage was incomplete", never for "nothing happened".
Also fixes a smaller defect in grok-delegate, where
beforeTree !== null && touched !== null && JSON.stringify(...) !== JSON.stringify(...)
evaluates to false when git cannot report -- "could not check" was reported as
"nothing happened". claude-delegate already returned null here. grok's stdout
summary had the matching bug: it tested truthiness, so a null verdict printed as
if the run were clean.
Implementation notes:
- porcelain paths are repository-root-relative, so they are joined against
`git rev-parse --show-toplevel`, not against --cd, which may be a subdirectory
- -z keeps paths containing spaces, quotes, or newlines in one field; -uall
expands untracked directories, since "?? dir/" never moves when a file inside
it changes
- R and C can occupy either status column; the origin field is consumed in both
cases, added to the set for a rename, not for a copy
- the fingerprint covers file type, permission bits, and symlink target, so a
chmod or a retargeted symlink counts
- hashing is streamed in 64KB chunks
- an unreadable path or a submodule marks coverage incomplete, yielding null
Scope is the two relays that already ship this tripwire. The other eight do no
before/after comparison at all, so there is nothing there to correct.
Test: a third read-only scenario next to the existing violation/clean pair. The
harness leaves a file dirty before dispatch and the fake CLI appends to it, so the
porcelain line is byte-identical at both ends. Verified to FAIL against the
unpatched relay before being kept.
WalkthroughChangesRead-only verification
Sequence Diagram(s)sequenceDiagram
participant Relay
participant Git
participant Filesystem
participant DelegatedCLI
Relay->>Git: Resolve repository root and read dirty paths
Relay->>Filesystem: Capture baseline fingerprints
Relay->>DelegatedCLI: Execute read-only delegation
DelegatedCLI->>Filesystem: Modify a pre-existing dirty file
Relay->>Git: Read post-run dirty paths
Relay->>Filesystem: Capture post-run fingerprints
Relay->>Relay: Calculate and write the tri-state verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
skills/claude-delegate/scripts/relay.mjs (2)
712-724: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOne unfingerprintable path discards all fingerprint evidence.
changedDirtyPathsreturnsnullwhen a single baseline path is a submodule or unreadable.readOnlyVerdictthen cannot use the other paths, so a proven content change on a readable path is lost and the verdict becomesnull. A repository that has a dirty submodule at dispatch reportsnullfor every read-only run.Return the partial result plus a coverage flag, so proof still wins over incomplete coverage.
♻️ Proposed partial-coverage result
function changedDirtyPaths(before) { - if (!before || !before.complete) return null; + // Report what IS known plus whether coverage was full, so a proven change still settles the + // verdict when an unrelated path could not be fingerprinted. + if (!before) return { changed: [], complete: false }; const now = fingerprintPaths(before.root, [...before.prints.keys()]); - if (!now.complete) return null; const changed = []; for (const [path, print] of before.prints) { - if (now.prints.get(path) !== print) changed.push(path); + const current = now.prints.get(path); + if (current === FINGERPRINT_UNREADABLE || current === FINGERPRINT_DIRECTORY) continue; + if (print === FINGERPRINT_UNREADABLE || print === FINGERPRINT_DIRECTORY) continue; + if (current !== print) changed.push(path); } - return changed.sort(); + return { changed: changed.sort(), complete: before.complete && now.complete }; }
readOnlyVerdictthen useschanged.length > 0fortrueand!completefornull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/claude-delegate/scripts/relay.mjs` around lines 712 - 724, Update changedDirtyPaths to return the detected changed paths together with a completeness flag instead of returning null when fingerprinting any baseline path is incomplete; retain all readable-path comparisons and mark the result incomplete when coverage is missing. Adjust readOnlyVerdict to use changed.length > 0 for a true verdict, and return null only when no changes are proven and the partial result is incomplete.
644-699: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFingerprinting can hash very large dirty sets twice per run.
dirtyPathsuses-uall. A repository with a large untracked directory that is not ignored (for example a build output tree) enters the dirty set in full.fingerprintPathsthen streams a SHA-256 over every one of those files at dispatch, andchangedDirtyPathsrepeats the work for each result write. In the grok relay the abort path writes results twice, so the cost is paid again.Consider a cheap pre-filter before hashing: compare
sizeandmtimeMsfrom thelstatSyncresult, and hash only when they match the baseline. That keeps detection of same-size, same-mtime edits while skipping most I/O.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/claude-delegate/scripts/relay.mjs` around lines 644 - 699, Optimize the fingerprint flow around pathFingerprint, fingerprintPaths, and changedDirtyPaths by adding a metadata pre-filter using lstatSync size and mtimeMs against the stored baseline before computing SHA-256. Reuse the baseline fingerprint metadata where available, hashing only when size and mtimeMs match so same-size, same-mtime edits remain detected, and preserve existing handling for symlinks, directories, unreadable paths, and absent files.test/relay-smoke.mjs (1)
1328-1334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
nullverdict and for the grok relay.The new scenario covers the
truecase for claude. Two behaviors that this PR changes stay untested:
readOnlyViolation === nullwhen coverage is incomplete. A run in a non-repository work directory is the cheapest trigger, sincegitRepoRootreturnsnull.- The grok relay verdict. The PR states that grok previously reported
nullas clean, and no grok scenario asserts the tri-state result.I can draft both scenarios if you want them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/relay-smoke.mjs` around lines 1328 - 1334, Extend the relay smoke-test scenarios to cover a null verdict using a non-repository work directory where gitRepoRoot returns null, and add a grok relay scenario that asserts its tri-state verdict. Keep the existing claude already-dirty true-case coverage unchanged and verify the expected null outcomes explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/claude-delegate/scripts/relay.mjs`:
- Around line 1082-1084: Exclude relay run artifacts from baseline dirty-path
fingerprints so artifact writes do not produce false changes. In
skills/claude-delegate/scripts/relay.mjs at lines 1082-1084, pass the resolved
output directory to fingerprintDirtyPaths and remove baseline paths whose
relative path from the repository root is inside that directory; apply the same
change in skills/grok-delegate/scripts/relay.mjs at lines 637-639, preserving
the existing readOnly behavior.
In `@skills/grok-delegate/scripts/relay.mjs`:
- Around line 640-645: Update the child process error handler to include the
result of readOnlyFlag(...) when constructing its touchedFiles result, ensuring
spawn-error outcomes carry readOnlyViolation consistently with other results and
trigger the existing read-only warning behavior.
---
Nitpick comments:
In `@skills/claude-delegate/scripts/relay.mjs`:
- Around line 712-724: Update changedDirtyPaths to return the detected changed
paths together with a completeness flag instead of returning null when
fingerprinting any baseline path is incomplete; retain all readable-path
comparisons and mark the result incomplete when coverage is missing. Adjust
readOnlyVerdict to use changed.length > 0 for a true verdict, and return null
only when no changes are proven and the partial result is incomplete.
- Around line 644-699: Optimize the fingerprint flow around pathFingerprint,
fingerprintPaths, and changedDirtyPaths by adding a metadata pre-filter using
lstatSync size and mtimeMs against the stored baseline before computing SHA-256.
Reuse the baseline fingerprint metadata where available, hashing only when size
and mtimeMs match so same-size, same-mtime edits remain detected, and preserve
existing handling for symlinks, directories, unreadable paths, and absent files.
In `@test/relay-smoke.mjs`:
- Around line 1328-1334: Extend the relay smoke-test scenarios to cover a null
verdict using a non-repository work directory where gitRepoRoot returns null,
and add a grok relay scenario that asserts its tri-state verdict. Keep the
existing claude already-dirty true-case coverage unchanged and verify the
expected null outcomes explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5696043-55c9-40ff-b253-f0e5034b67d1
📒 Files selected for processing (3)
skills/claude-delegate/scripts/relay.mjsskills/grok-delegate/scripts/relay.mjstest/relay-smoke.mjs
| // Contents of the paths that are ALREADY dirty. Their porcelain lines will not move if the | ||
| // run edits them, so the line comparison above cannot see those writes on its own. | ||
| const beforeFingerprints = opts.readOnly ? fingerprintDirtyPaths(opts.cd) : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Baseline fingerprints include the relay's own run artifacts. Both relays capture the baseline after prepareRun creates the artifacts. If the output directory sits inside the worktree and is not ignored, dirtyPaths (with -uall) lists those untracked artifacts, the baseline hashes them, and the relay's later writes to the events, final, stderr, and result files change their fingerprints. changedDirtyPaths then reports a change and the verdict becomes true for a run that wrote nothing else. The porcelain comparison tolerated this because the artifact status lines did not move; the fingerprint check does not.
skills/claude-delegate/scripts/relay.mjs#L1082-L1084: pass the run artifact directory tofingerprintDirtyPathsand drop baseline paths that resolve inside it, usingrelative(root, ...)against the resolved out-dir.skills/grok-delegate/scripts/relay.mjs#L637-L639: apply the same exclusion, sincerun.eventsPathis appended during the run at line 678.
📍 Affects 2 files
skills/claude-delegate/scripts/relay.mjs#L1082-L1084(this comment)skills/grok-delegate/scripts/relay.mjs#L637-L639
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/claude-delegate/scripts/relay.mjs` around lines 1082 - 1084, Exclude
relay run artifacts from baseline dirty-path fingerprints so artifact writes do
not produce false changes. In skills/claude-delegate/scripts/relay.mjs at lines
1082-1084, pass the resolved output directory to fingerprintDirtyPaths and
remove baseline paths whose relative path from the repository root is inside
that directory; apply the same change in skills/grok-delegate/scripts/relay.mjs
at lines 637-639, preserving the existing readOnly behavior.
| // every result that reports touchedFiles carries the verdict, aborted runs included - | ||
| // an aborted --read-only review can still have modified the tree | ||
| const readOnlyFlag = (touched) => | ||
| opts.autonomy === "read-only" | ||
| ? { readOnlyViolation: beforeTree !== null && touched !== null && JSON.stringify(beforeTree) !== JSON.stringify(touched) } | ||
| ? { readOnlyViolation: readOnlyVerdict(beforeTree, touched, beforeFingerprints) } | ||
| : {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The spawn-error result carries no verdict.
The comment states that every result reporting touchedFiles carries the verdict. The child.on("error") handler at lines 757-773 reports touchedFiles without spreading readOnlyFlag. For that result readOnlyViolation is absent, so the === null warning at line 835 never prints and the failed read-only run looks unverified in silence. Spread readOnlyFlag(...) in that handler as well, or narrow the comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/grok-delegate/scripts/relay.mjs` around lines 640 - 645, Update the
child process error handler to include the result of readOnlyFlag(...) when
constructing its touchedFiles result, ensuring spawn-error outcomes carry
readOnlyViolation consistently with other results and trigger the existing
read-only warning behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82165af2db
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Re-fingerprint exactly the baseline paths, not whatever happens to be dirty now: a path the | ||
| // run newly dirtied is already reported by the porcelain comparison, and letting an unreadable | ||
| // one of those blind this signal would be a regression, not caution. | ||
| if (!before || !before.complete) return null; |
There was a problem hiding this comment.
When any baseline dirty path is a submodule or unreadable, before.complete is false and this returns before comparing the fingerprints of readable paths. In a repository with a dirty submodule plus an already-dirty regular file, changing the regular file leaves porcelain unchanged and produces null even though its changed hash proves a violation; the identical short-circuit exists in the Grok relay. Compare known fingerprints first, and return null only when none changed and some coverage remains unknown.
Useful? React with 👍 / 👎.
| if (porcelainMoved || (changed !== null && changed.length > 0)) return true; | ||
| if (beforeTree === null || afterTree === null || changed === null) return null; |
There was a problem hiding this comment.
The new verdict makes null include incomplete fingerprints and makes true include changes within already-dirty files, but skills/claude-delegate/SKILL.md:131-135, its dispatch/review references, and the corresponding Grok documentation still define the field solely in terms of porcelain and explicitly say such edits evade detection. Orchestrators following those public contracts will misinterpret the result and miss the central behavior added here, so update every affected skill and reference surface to match the new three-state semantics.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
1 participant
The problem
readOnlyViolationcomparesgit status --porcelainbefore and after a run. That comparisoncannot see a write to a path that was already dirty at dispatch, because such a write usually
does not move the status line:
" M src/app.js"before," M src/app.js"after.This is not a new observation —
claude-delegate/scripts/relay.mjsalready says so:The gap matters because the normal way to use a read-only run is to ask a question while you
have uncommitted work in progress. That is precisely the state in which the tripwire reports
falseno matter what the implementer did to those files.Minimal reproduction
result.jsonreportsreadOnlyViolation: false. The file was modified.A second, smaller defect in
grok-delegateWhen
gitcannot report,beforeTreeisnulland the expression evaluates tofalse—"we could not check" is reported as "nothing happened".
claude-delegatealready gets thisright by returning
null;grok-delegatedoes not. Its stdout summary has the matching bug: ittests
if (result.readOnlyViolation), so anullverdict prints as if the run were clean.The change
The porcelain comparison is kept exactly as it is, and a second signal is added beside it: at
dispatch, a read-only run fingerprints the contents of the paths that are already dirty, and
compares them again afterwards.
Only that set is fingerprinted. A path that is clean at dispatch and gets written surfaces as a
new porcelain line already, and fingerprinting a whole repository on every run would cost far
more than the case it covers.
The verdict becomes properly three-valued, with proof beating absence of proof:
Details that turned out to matter:
directory git ran in (
--porcelainforcesstatus.relativePathsoff). They are joinedagainst
git rev-parse --show-toplevel, so a--cdpointing at a subdirectory still works.-z -uall.-zkeeps a path containing a space, a quote, or a newline in one field.-uallexpands an untracked directory into its files, since a collapsed"?? dir/"linenever changes when a file inside it does.
R/Cin either status column. Such an entry is followed by its origin path as its ownunprefixed field. A rename origin is added to the set (the file moved away from it); a copy
origin is consumed but not added, since a copy source can be perfectly clean.
target, so a
chmod, a retargeted symlink, or a file replaced by a directory all count.memory.
marks coverage incomplete, which yields
nullrather than a clean verdict.Scope
Only
claude-delegateandgrok-delegate— the two relays that already ship areadOnlyViolationtripwire. The other eight do no before/after comparison at all, so there isnothing there to correct; extending them would be a feature, not a fix, and belongs in its own
discussion.
What this does NOT claim
are outside it, and a write followed by a perfect restore before the snapshot is invisible.
falsemeans "no git-visible change was detected", not "the filesystem is unchanged". Thewording in
claude-delegate's header was updated to say so.indistinguishable. The diff remains the authority.
Tests
test/relay-smoke.mjsgains a third read-only scenario alongside the existingviolation/cleanpair:The harness leaves
already-dirty.txtuntracked before dispatch and the fake CLI appends to it,so the porcelain line is identical at both ends. On
masterthis scenario reportsfalse; withthis change it reports
true.Full suite passes locally on macOS.
Provenance
This came out of running a read-only consultation against a repository with uncommitted work and
noticing the answer was reported clean when it should not have been. The implementation was
reviewed across three rounds by an independent model, which found six defects in my own first
cut before this reached you — including the very same
null-reported-as-falsebug I amreporting in
grok-delegate. The two regression tests that cover the subtlest cases (thesubdirectory
--cdand the worktree-column rename) were each verified to fail against thepre-fix logic before being kept, since a test that passes either way proves nothing.
Happy to split this into two PRs (the
grokthree-valued fix is independent and much smaller)if that is easier to review.
Summary by CodeRabbit