Make approved agent mutations replay-safe - #61
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Trustabl scan
Readiness goes from Readiness now 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 Projected if all findings resolved 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 Findings by severity
Projected headroom — estimate, not a re-scan
Projected by re-applying trustabl's own scoring with the listed findings resolved (nothing new introduced). Treat as guidance, not a guarantee.
✅ Passed scanning |
![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: fc7f686dcc
ℹ️ 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".
| payload = await store.run_once_async( | ||
| scope=ctx.idempotency_scope(spec), | ||
| idempotency_key=call.id, | ||
| arguments={"tool": spec.id, "arguments": call.arguments or {}}, |
There was a problem hiding this comment.
When a REACT_TEXT run invokes the same mutating tool twice, ReactTextAdapter._to_turn() assigns both calls the constant ID react_0, while this ledger scopes keys only by run, target, and tool. Consequently, a second separately approved call with the same arguments replays the first result without executing, and one with different arguments is rejected as an idempotency conflict; use a call ID that incorporates the turn/iteration before treating it as the approval key.
Useful? React with 👍 / 👎.
| def create_issue( | ||
| title: str, | ||
| idempotency_key: str, | ||
| body: str = "", |
There was a problem hiding this comment.
In the default legacy CrewAI path, _build_issue_agent() passes ISSUE_TOOLS directly to CrewAI and no wrapper injects an approval/request ID, so this required parameter becomes model-facing even though the model is never given such an ID. Mutating calls will either fail argument validation when the model omits it or use a fabricated value that cannot remain stable across retries, defeating the new guarantee; bind the runtime-generated key in a wrapper instead of adding it to the exposed tool signature.
Useful? React with 👍 / 👎.
| async def _approved_operation() -> Dict[str, Any]: | ||
| result = await _invoke_handler() | ||
| return result.to_idempotency_payload() |
There was a problem hiding this comment.
When an externally mutating handler catches a lost-response exception and returns ToolResult.failure—as _handler_for() does for every MCP exception and the remote filesystem and Git push handlers do for network failures—this callback returns normally, so run_once_async() records the key as completed. The caller then receives an ordinary failure without requires_reconciliation, even though the external action may have committed, and a newly approved retry can repeat it; propagate or explicitly mark ambiguous external failures as indeterminate rather than serializing every ToolResult as completed.
Useful? React with 👍 / 👎.
| first = asyncio.run(registry.execute(call, ctx)) | ||
| second = asyncio.run(registry.execute(call, ctx)) | ||
|
|
||
| assert not first.ok and first.error == "TimeoutError" | ||
| assert not second.ok and second.error == "idempotency_guard" |
There was a problem hiding this comment.
On every supported Python version (3.11–3.12), built-in TimeoutError is an alias of asyncio.TimeoutError, so the exception raised by this test is caught by ToolRegistry.execute() and returned as error="timeout", not "TimeoutError"; running the targeted suite on Python 3.12 therefore fails this assertion consistently (and the message also says timed out after Nones). Adjust the timeout handling or the expected result so the committed suite is green.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Summary
This hardens GitPilot's human-in-the-loop mutation path around a production invariant:
The final design keeps the generic toolkit predictable, adds durable replay protection only at the real agent-runtime boundary, preserves legacy compatibility, and keeps read/search performance on the zero-ledger fast path.
Production runtime boundary
ToolRegistrykeeps its existing SDK/test/direct-call semantics.RuntimeToolRegistryadds replay protection only when a call belongs to a real run/session.ToolSpec.effects(WRITES_FS,GIT_LOCAL,GIT_REMOTE,FORGE_WRITE,EXTERNAL_WRITE).ToolCall.id— also used as the approval request id — becomes the runtime idempotency key automatically. The model does not invent or manage keys.ToolResultwithout invoking the handler again.Durable mutation ledger
Adds a small standard-library SQLite/WAL execution ledger (
~/.gitpilot/idempotency.sqlite3, configurable withGITPILOT_IDEMPOTENCY_DB):BEGIN IMMEDIATE;WALjournal mode and full durability sync;indeterminatestate after a lost/ambiguous response.No Redis, Kafka, Temporal, database service, or other runtime infrastructure is introduced.
Human approval UX
Legacy CrewAI compatibility
The compatibility surface still exposes
idempotency_keywhere Trustabl expects mutation replay protection, but existing caller ergonomics are preserved where possible:RuntimeToolRegistry;This removes Trustabl
CREW-006findings at the implementation level rather than suppressing the scanner.Reliability and test hardening
Two unrelated-but-real CI assumptions were also made deterministic while validating the change:
user.name/user.email, so commit tests do not depend on a developer or CI runner having global Git identity configured.Validation
Final head:
f8719e6f5cd409f46443fe74e38ec9997fc35dde34703430520: 100/100 readiness, 0 critical / 0 high / 0 medium / 0 low, native exit 0, gate passed.34703430537: 3,874 passed, 4 skipped, no failures.The PR is green end-to-end on its final head.