fix(pi): stop the edit-write policy blocking atomic, and decide by recoverability - #2735
Merged
Merged
Conversation
modules/home/ai/pi/default.nix writes edit-write-policy.ts straight into ~/.pi/agent/extensions/, and atomic loaded it too. getAgentDirs (packages/coding-agent/src/config.ts:376) returns [~/.atomic/agent, ~/.pi/agent] with no filesystem test, and addAutoDiscoveredResources (core/package-manager-auto-resources.ts:188) scans both roots' extension directories additively, so a populated ~/.atomic/agent does not suppress the pi root and every .ts file placed there runs under both agents whether or not either settings file names it. The policy reads a top-level path off the tool input, which is pi's edit and write schema. atomic's write also has path, but its edit takes a single hashline input string under additionalProperties: false, so no path can reach the policy from atomic and every atomic edit was refused as malformed input. Writes at the root of an ordinary git repository failed separately. Both are fixed in the following commit; this one stops the extension reaching an agent it was never written for. piOnlyExtensions names the files only pi may load, and extensionsForAtomic renders them into atomic's top-level extensions key. That key is a different override channel from packagesForAtomic: a pattern in a packages entry matches against the package source directory, while this one matches against each scanned configuration root, so an entry needs its extensions/ segment. The bare -edit-write-policy.ts spelling is a silent no-op, because the force-exclude arm compares only the root-relative or absolute path and never the basename; the structural check asserts both the relationship and the spelling, since nothing else in this repository observed the coupling.
The policy refused on every condition it recognized and on every condition it failed to parse. In its recorded operational history that produced fourteen refusals and no true positive: nine in pi, all from a join-emptiness gate deleted in 4fa2a0f as a defect, and five in atomic from the two faults below. In the session that prompted this change the model routed around a refusal with a bash heredoc and sed -i inside one turn and completed the mutation anyway, so the refusal did not prevent the write; it moved the write off a tool surface that renders a diff and queues per file onto one that does neither. Decisions are now classified by recoverability. A mutation version control can undo is announced and permitted: a protected git branch, a protected jj bookmark, a conflicted or divergent @, and every diamond topology violation. A mutation nothing would bring back is still refused: a target the identified repository does not contain, and a target in no repository at all. Announcement rather than confirmation is deliberate. Pi has no permission system, so a dialog on an autonomous session can have nobody to answer it, which is the stall pattern ce9c780 and its parent removed from the Claude Code gates; notify is fire-and-forget in every mode and cannot deadlock. The channel is ctx.ui.notify, which appends to the chat scrollback in Pi's TUI and emits a protocol frame under RPC. It is a no-op when the host has no dialog-capable UI, so a print-mode run announces nothing; that is the accepted cost of never stalling. Every indeterminate class now permits instead of refusing. A probe this policy cannot parse says nothing about whether a mutation is safe, and both faults that made this arm fire were environmental rather than diagnostic. The repository classification still carries its diagnostic and a new classification case kind asserts it, so the probe parsers stay oracled while the decision fails open. Three faults fixed alongside the reclassification: The jj outside-repository diagnostic was anchored at the end of stderr. jujutsu 0.43.0 appends Hint lines whenever the probed directory holds a .git directory, which is exactly the case that has to fall through to the Git branch, so the whole git arm including the protected-branch check was unreachable at the root of every ordinary Git repository. Both that pattern and the Git analogue now match a prefix; the exit-code and empty-stdout conjuncts are kept, since those are what distinguish the diagnostic from a probe that failed some other way. The regression row carries jujutsu 0.43.0's verbatim output rather than a synthetic line. Path normalization diverged from Pi's. Pi resolves through resolveToCwd, which folds unicode spaces, strips an @ prefix, expands a leading tilde and converts a file URL; the policy applied only the @ strip and then resolved against cwd, so a write to ~/secret was judged as a path inside the repository and permitted while Pi wrote it to the home directory. normalizeToolPath mirrors Pi's order and is asserted against it row by row. The immutable-root refusal is dropped. /nix/store is root-owned and the kernel already refuses those writes, so the check restated a filesystem permission rather than adding containment. The malformed-tool-input refusal stays. It is drawn from an unreadable request rather than an unrecoverable target: a call whose path cannot be read is one the policy cannot reason about at all. Spec deltas accompany each of these; Fail-closed policy becomes Fail-open policy, and the diamond and default-branch requirements move to announcement. New cases were replayed against the pre-change policy: eight of nine discriminate. The ninth, atomic's hashline edit shape, refuses before and after by construction and pins no behavior change; it is kept as a forward guard, so that teaching the adapter a second tool grammar has to be a deliberate decision rather than a quiet one.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
modules/home/ai/pi/default.nixwritesedit-write-policy.tsinto~/.pi/agent/extensions/, for pi. atomic loaded it too:getAgentDirs(config.ts:376at0.9.13) returns[~/.atomic/agent, ~/.pi/agent]with no filesystem test, andaddAutoDiscoveredResources(core/package-manager-auto-resources.ts:188) scans both roots' extension directories additively. A populated~/.atomic/agentdoes not suppress the pi root, so every.tsfile placed there runs under both agents whether or not either settings file names it.Under atomic that produced two failures, both reproduced from the session transcript:
edit. atomic'seditschema is{ input: string }withadditionalProperties: false; there is nopathfield, and a model that emitted one would be rejected by argument validation before the hook runs. The policy reads a top-levelpath, so every atomic edit was refused as malformed input without the decision core ever running.Hint:lines to the outside-repository diagnostic whenever the probed directory holds a.gitdirectory. The policy anchored that diagnostic at the end of stderr, so the characterization failed and inspection returned indeterminate — which the old policy refused. That is exactly the case that must fall through to the Git branch, so the entirecase "git"arm, including the protected-branch check, was unreachable at the root of every ordinary Git repository.Neither was visible to the oracle: every repository and adapter case is driven by injected fake capabilities, no real
jjorgitprocess ever touches the policy, and the adapter rows feed pi's schema. A case at:1651did construct a trailingHint:line — and asserted that it should block, whichFail-closed policyin the spec required.Reclassified arms, and the principle
Decisions are now classified by recoverability, not severity: block only what cannot be undone, announce everything else.
main/master@@@parents, join conflicted/parents)/nix/storeAnnouncement rather than confirmation is deliberate: pi has no permission system, so a dialog on an autonomous session can have nobody to answer it — the stall pattern removed from the Claude Code
git pushandnix rungates.notifyis fire-and-forget in every mode and cannot deadlock.Two entries deserve calling out because they are judgement calls rather than transcriptions:
prompt, andpromptno longer exists. It falls in the "not protected by version control" class alongside containment, so it blocks. On an autonomous session this is not a regression — the old code already blocked it, viainteraction unavailable for required mutation confirmation. On an interactive session it is stricter than before. Easy to flip if that reads wrong./nix/storeisdrwxrwxr-t root nixbld;touch /nix/store/probereturnsPermission denied. The check restated a filesystem permission rather than adding containment.Notification channel, and what it costs
ctx.ui.notify(message, "warning")on pi'sExtensionUIContext. In the TUI it reachesshowWarning, which appends tochatContainer— persistent in the scrollback, not a transient toast. Under RPC it emits anotifyprotocol frame. It is a no-op when the host has no dialog-capable UI (noOpUIContext.notifyincore/extensions/runner.ts), so a print-mode run announces nothing.That hole is the accepted cost of never stalling, and it is reported rather than papered over.
ntfy, which thegate-dangerous-commandsNOTICE path uses, would close it, but it means a network side effect inside a synchronous tool-call gate and a new runtime dependency — a design decision beyond this change's scope. Flagging it rather than deciding it.The check asserts delivery: a notify row that announces nothing fails, because a notify decision that discards its signal is a bare allow, which is a different decision from the one the policy declares.
atomic deliberately retains no path-based edit/write gate
After this change atomic has no path-based edit or write policy at all. What remains for atomic is
permission-gate's argv classification (registered for both agents) and filesystem permissions. Nothing stops atomic editing onmain, or in a repository whose jj join is broken.This is deliberate, not an oversight. The alternative is teaching the adapter atomic's hashline grammar — and one
editcall can carry several[PATH#TAG]sections (grammar.lark:1file_patch+;assertUniquePreparedPathsrejects only duplicate paths), so a first-header check would authorize a call that also mutates every other named file unchecked.extractFirstHeaderPathin atomic's ownedit.tsis a TUI title-line renderer, not an authorization primitive. Doing it correctly means maintaining a second implementation of another project's parser inside a security check, and the evidence did not justify it: the policy's whole recorded history is fourteen refusals and zero true positives, and in the motivating session the model routed around a refusal with abashheredoc andsed -iwithin one turn and completed the mutation anyway.The
adapter cannot read atomic hashline edit inputcase is kept as a forward guard, so that teaching the adapter a second tool grammar has to be a deliberate decision rather than a quiet one.Verification
Severity. New and reclassified assertions were replayed against the pre-change policy (
ce9c7800); 8 of 9 discriminate:The inert row is inert by construction and is reported as such rather than counted.
The structural assertion was checked the same way: replacing the force-exclude with the bare-basename spelling flips
everyDeclaredExcludedandnoBareBasenameSpellingtofalseand fails the check. That spelling is a silent no-op in atomic — the-arm compares only the root-relative or absolute path, never the basename — so the assertion pins the one detail most likely to be got wrong.Not done
The stale evidence anchors in
openspec/changes/archive/2026-08-15-configure-pi-agent-environment/verify.mdare left alone. They point at a 3406-line version of the check module that no longer exists, but that file records what was verified at the time; re-anchoring it to today's line numbers would make it claim a verification it never performed, and this change moves those lines again.No
~/.atomic/or~/.pi/state was touched, so atomic stays broken for editing until this lands.