Repository navigation
Conversation
Review request (agent core)Problem: Stale turn-end file-mutation footer after a failed Contract: CONTENT-TRANSITION verifier — safely observed local content change may clear the footer; fail closed when observation is unavailable. Not payload-equality validation. Please review these commits (branch is rebased onto
Suggested test focus: scripts/run_tests.sh tests/run_agent/test_file_mutation_verifier.py
scripts/run_tests.sh tests/agent/test_file_mutation_verifier_redesign.py
scripts/run_tests.sh tests/agent/test_turn_finalizer_file_mutation_footer.py
scripts/run_tests.sh tests/run_agent/test_tool_batch_segmentation.pyTouches: |
e992fa3 to
913ec28
Compare
Related: #53165 and #65584 address the same stale verifier-footer outcome with narrower/different observation mechanisms. This content-transition approach is not a duplicate, but the live PR also carries extensive unrelated inherited history; please rebase or isolate the requested verifier commits before maintainers choose a mechanism. |
|
The rebase was already done, this triage ran right as I force-pushed (e992fa3 -> 913ec28). The PR is now exactly 3 verifier commits, no inherited history:
Diff touches agent/file_mutation_verifier.py, executor/turn wiring and tests only. I updated the description, the old note about desktop commits at base was stale. On mechanism: this one requires an observed SHA-256 content change (not stat metadata, so the chmod case flagged on #65584 keeps the footer) and fails closed when observation is unavailable. It is also built on the current segmented tool-batch executor, the drift called out on #53165. Happy to adapt to whatever contract maintainers pick. |
…oters Extract TurnFileMutationVerifier with pre-dispatch baselines, local observation, and fail-closed reconciliation so a later legitimate file change can suppress a failed patch/write footer without proving payload match. Co-authored-by: Cursor <cursoragent@cursor.com>
Use killable subprocess isolation for bounded local hashing, O_NOFOLLOW opens with stability checks, registry dispatch tracking via ContextVar, and tests for middleware short-circuit, observation timeout, and turn-end footer on interrupted/empty turns. Co-authored-by: Cursor <cursoragent@cursor.com>
Record an unknown-target ledger entry so invalid or unparseable patch/write failures still surface in the turn-end footer instead of being dropped. Co-authored-by: Cursor <cursoragent@cursor.com>
f37f29f to
64b7e60
Compare
|
Rebased onto current main (551e1c6), same 3 commits. No content changes except one legacy test updated to the fail-closed contract from the third commit (test_missing_path_arg_recorded_nowhere -> test_missing_path_arg_fails_closed). Verifier suites: 87 passed locally. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the content-transition approach and preserving a fail-closed observation contract. The underlying stale-footer case is real on current main: run_agent.py:3272-3298 only reconciles write_file/patch outcomes, so a later terminal edit cannot clear the entry.
Problems
agent/turn_finalizer.py:413-426now emits a footer after an interrupted or empty turn. Current main deliberately suppresses that (agent/turn_finalizer.py:413-416) after it has closed and persisted an interrupted tool tail (agent/turn_finalizer.py:289-291;agent/message_sanitization.py:307-311). This can deliver a footer while the durable assistant row remainsOperation interrupted.agent/tool_dispatch_helpers.py:60-63,157-159serializes everywrite_fileandpatch. Current main explicitly permits independent file-tool paths concurrently (agent/tool_dispatch_helpers.py:60-61); the verifier should not regress that batch behavior without a narrower ordering strategy.
Suggested changes
- Preserve the interrupt gate or make delivered and persisted interrupted responses identical.
- Add an executor-level failed-
patch→terminal-edit regression; the branch's directPath.write_texttest does not exercise the claimed dispatch path.
Automated hermes-sweeper review.
| if _failed and agent._file_mutation_verifier_enabled(): | ||
| footer = agent._format_file_mutation_failure_footer(_failed) | ||
| if footer: | ||
| # Gate: append when failures remain. Interrupted/empty turns still |
There was a problem hiding this comment.
This resurrects a user-visible response after an interrupt. finalize_turn already closes and persists the interrupted tool tail before this section; on an empty interrupt that row is Operation interrupted.. Please retain the interrupt gate, or update the durable close path so the delivered footer and persisted assistant content cannot diverge.
|
|
||
| # File tools can run concurrently when they target independent paths. | ||
| _PATH_SCOPED_TOOLS = frozenset({"read_file", "write_file", "patch"}) | ||
| # Read-only path tools can run concurrently when they target independent paths. |
There was a problem hiding this comment.
This drops the existing independent-path concurrency contract for every write_file and patch, not only ambiguous middleware-rewritten calls. Please keep the path-scoped fast path where final effective paths can be safely determined, rather than making all structured mutations sequential barriers.
|
Still conflicted against current Why a plain rebase is insufficient
Next step (not yet pushed)Surgical port onto latest
Leaving the existing branch tip unchanged so review history stays intact until that port is ready. |
Summary
TurnFileMutationVerifier(content-transition contract): pre-dispatch baselines, local bounded observation, fail-closed reconciliation, and sanitized turn-end footers.write_file/patchbarriers), turn context/finalizer, andrun_agentlegacy sync.Test plan
pytestverifier + redesign + segmentation + dispatch helpers (118 passed locally)scripts/run_tests.shfull matrix on CI/Linux devShellpatchthenterminaledits same file — footer should clear when content safely observed to changeNotes
Rebased onto current
main. The PR is exactly 3 verifier commits (8064355,fb37c82,64b7e60), no inherited fork history.Remaining follow-up: full E2E middleware matrix. Subprocess observation timeout kill landed in
fb37c82.