Conversation
preview_write's warning read: "This preview does not execute. execute_approved_write is destructive and requires the matching approval token plus confirm=true." It never mentions validate_write, which is the only step that registers the approval (register_write_approval). Anyone following the warning goes straight from preview_write to execute_approved_write and gets "approval token has not been validated in this server session or has expired" — an error that reads like an infrastructure fault rather than a missing step. In our case it was first reported as a suspected session-affinity problem, and the misleading warning was only found after ruling that out: write_approvals lives on the per-lifespan AppContext, so it survives across requests for as long as the process does. The warning now names the missing step. The error it warns about states the TTL and, for approvals staged from a local file, warns against re-sending the sha256: fingerprint echoed back by an earlier call in place of the original *_from_path argument — that would write the fingerprint string to the binary field. It deliberately does not accuse the caller of skipping validation, since an expired approval is reported through the same branch once it has been swept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
preview_writereturns this warning:It never mentions
validate_write, which is the only step that registers the approval (register_write_approval). A caller who follows the warning goes straight frompreview_writetoexecute_approved_writeand gets:That reads like an infrastructure fault rather than a missing step. In our case it was first reported as a suspected session-affinity problem, and the misleading warning was only found after ruling that out —
write_approvalslives on the per-lifespanAppContext, so an approval survives across requests for as long as the process does.User-facing behavior change: wording only. The warning now names
validate_writeas the next call. The error it warns about now states the TTL and, for approvals staged from a local file, warns against re-sending thesha256:...fingerprint echoed back by an earlier call in place of the original*_from_pathargument — doing so would write the fingerprint string into the binary field.The message deliberately keeps "has not been validated ... or has expired" as one branch rather than splitting it. Once
_sweep_expired_write_approvalshas run, an expired approval is indistinguishable from one that was never validated, and telling a caller who did validate that they skipped the step would be worse than saying nothing.Coverage
tests/test_agent_tools.pyasserts the preview warning namesvalidate_writeand names it beforeexecute_approved_write, so the ordering that caused the confusion cannot silently come back. The existingexecute_approved_writeerror-path tests intests/test_server.pystill pass against the new wording unchanged.Verification
preview_write→validate_write→execute_approved_writeround-trip, plus the failure path).The deselected test is
test_from_path_rejects_symlink_escape_within_upload_root, which needs the Windows symlink privilege and fails on this machine both before and after the change.Checklist
docs/troubleshooting.md;README.md's Safe Write Model already describes the three steps correctly — the defect was only in the tool's own warning text)CHANGELOG.mdunder## UnreleasedRelation to #61
#61 moves this error into a new
_write_execution_state_errorhelper and carries the old wording forward verbatim, so the two will conflict textually intools_write.py. Whichever lands second, the resolution is to keep this wording inside that helper. Happy to rebase on top of #61 if you prefer to merge it first.