Skip to content

fix(write): name validate_write as the step that registers an approval - #63

Open
glazperle wants to merge 1 commit into
erpipe-org:mainfrom
glazperle:upstream-pr/write-approval-guidance
Open

glazperle wants to merge 1 commit into
erpipe-org:mainfrom
glazperle:upstream-pr/write-approval-guidance

Conversation

@glazperle

Copy link
Copy Markdown

Summary

preview_write returns this warning:

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). A caller who follows 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

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 an approval survives across requests for as long as the process does.

User-facing behavior change: wording only. The warning now names validate_write as the next call. The error it warns about now 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 — 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_approvals has 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.py asserts the preview warning names validate_write and names it before execute_approved_write, so the ordering that caused the confusion cannot silently come back. The existing execute_approved_write error-path tests in tests/test_server.py still pass against the new wording unchanged.

Verification

  • Odoo versions: 17.0, confirmed against a live instance (preview_writevalidate_writeexecute_approved_write round-trip, plus the failure path).
  • Odoo transport: XML-RPC. MCP transport: Streamable HTTP.
  • Exact commands:
uv run python -m ruff check .                  # All checks passed!
uv run python -m mypy src                      # Success: no issues found in 34 source files
uv run python -m pytest                        # 915 passed, 1 deselected
uv run python -m pytest tests/test_server.py   # 212 passed

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

  • Updated documentation (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)
  • Updated CHANGELOG.md under ## Unreleased
  • No credentials or production data in the change
  • No gate is loosened: this changes message strings and adds no new path

Relation to #61

#61 moves this error into a new _write_execution_state_error helper and carries the old wording forward verbatim, so the two will conflict textually in tools_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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant