Repository navigation
feat(runtime-api): support idempotency keys in task.submit - #5490
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Requesting changes at 526c639 — not on the code, which I verified independently and found correct, but on the PR body's own evidence.
The pasted before/after evidence is anchored to the wrong commit. The body's "review-scope" line states "HEAD is exactly e7c88e95f5ab3e70afffc6a58eb3b3fec60e86dc" and gives a diff SHA-256 computed against that commit — but the PR's actual head (confirmed via gh pr view and github-decision.py) is 526c639cfe7d0d5e2bd5a8e8f3169857dcbe47a0, a different commit. The "monitor-user-journey"/"monitor-repeatability" script output and the computed diff hash in the body are therefore not evidence for what's actually on this PR right now — they can't be checked against this diff, which is this repo's own #1 reason to bounce a PR (per CONTRIBUTING.md). Most likely a stale evidence block from before a later push, not a code problem, but it needs to be re-generated against the real head before I'd call it mergeable evidence.
What I verified myself, independent of the PR body's claims:
- Cloned at the exact head (526c639) and ran
tests/runtime-api-tasks-view.test.py: 65/65 pass, including the 3 new idempotency-submit tests. - Reproduced the before/after: copied just the new test file onto the parent commit (88945c2) — 13 failures, including the two
test_submit_idempotency_*cases, confirming the dispatcher genuinely didn't expose this path before. dispatcher.py's 4-line addition delegates entirely to the already-shipped, already-testedTasksView.submit_idempotent(from #4488, not new in this PR) and reuses the existing genericValueError → ProtocolError(-32602)translation already in_task()— no new validation logic, no duplicated policy.tests/runtime-api-dispatcher.test.pyalso passes at head (no regressions), andprotocol.py's one-line doc update matches the new optional param.- CI: all required checks pass (I watched
diff coverageandtsc + testsfinish green), CLA signed.
Small, well-scoped, correctly delegated change — re-point the evidence section to the real head (or re-run and re-paste) and I'd expect to approve on the next pass.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at 526c639.
Verified myself, in an isolated worktree at this head:
python3 tests/runtime-api-tasks-view.test.pypasses in full.- The bug exists on main, shown as behavior, not as missing tests. I put main's
dispatcher.pyunder the PR's three new tests. They fail on the behavior itself: a keyed retry gets a differenttaskId({'taskId': 'task-rtapi-c0ed…'} != {'taskId': 'task-rtapi-1db4…'}, completion and archive cases), and'',' 'andNonekeys raise noProtocolError. With the head's dispatcher they pass. The "before" block in the PR body only showsAttributeError: no attributefrom the tests not existing yet. That proves nothing about behavior, so please replace it with output like the above (non-blocking for me, since I reproduced it). - Whole path:
TasksView.submit_idempotentalready validates the key (non-string, blank, non-UTF-8, over 1024 bytes) and the field-level collision, raisingValueError. The existingexcept ValueErrorin_taskmaps that to-32602. The four added lines only route to that owner, so no policy is duplicated.
Worst case for existing users: none that I can find. idempotencyKey is opt-in, and an absent key takes the unchanged submit path with the same priority default. test_submit_without_key_remains_non_idempotent pins that. Explicit null counts as "present" and is refused with -32602 rather than silently ignored, which is the safer reading.
Exact-head evidence receipt: stable, all 15 required checks pass, including license/cla.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Approve at 526c639. Ready to merge on the code; this replaces this account's earlier CHANGES_REQUESTED at the same head (reason below).
The earlier CHANGES_REQUESTED from this login said the body's evidence was tied to e7c88e95…, not the head. The code hasn't changed. Its concern was that the pasted output could not be checked against this diff, so I re-ran the body's commands at this head and at the parent. The output matches what the body pasted. With that, I no longer see a blocker. e7c88e95 is not reachable on GitHub (git fetch gives "not our ref", and the commits API returns 404/422), so I could not check the body's claim that the two commits share a tree. My reruns make that claim unnecessary.
What I ran (fresh clone, focused tests only)
- Parent
88945c26, the body's acceptance script:same keyed RPC IDs equal: False,published files after keyed retry: 2,blank key RPC state: pending. That matches the body's Before section. - Head
526c639c, same script:True,1, and the blank key raisesProtocolError -32602 "idempotency key is required". - The body's named test command:
Ran 20 tests … FAILED (errors=3)at the parent (the new tests don't exist there). At head:Ran 20 tests … OK. The full file goes from 62 to 65 tests, OK. - Discrimination control: I put the head's test file on the parent's
dispatcher.py.test_submit_idempotency_retry_after_restart_and_completionfails (completion and archive).test_submit_idempotency_rejects_invalid_keys_and_collisionsfails on all 9 bad keys and both collision cases withProtocolError not raised.test_submit_without_key_remains_non_idempotentpasses, which is correct because it pins unchanged behavior. So the tests check behavior, not just that the tests exist.
Whole path
src/runtime-api/dispatcher.py:303-306only routes toTasksView.submit_idempotent(src/runtime-api/tasks_view.py:97). That function validates the key: it must be a non-blank string, valid UTF-8, and at most 1024 bytes. It also rejects a field collision. In both cases it raisesValueError, and the existingexcept ValueErroratdispatcher.py:335maps that to-32602. No policy is duplicated.src/runtime-api/protocol.py:31: the doc line matches the new behavior.
Worst case for existing users: none found. The parameter is opt-in. No in-repo task.submit caller sends idempotencyKey (both calls in src/runtime-cli/sutando-runtime.py send {"task": …} only), so current traffic takes the unchanged submit path. The change is dispatch-only behind an existing owner-only RPC, with no bridge, delivery loop or startup change. I judge that a direct RuntimeDispatcher.handle exercise is enough and a post-restart witness isn't needed.
Added-line path scan: clean. A grep over + lines for /Users/, /home/, ~/., /private/, /usr/bin/, expanduser and Path.home found nothing. As a control, the same pattern fires on a known /Users/ line.
CI / CLA: github-evidence.py --expect-head 526c639c returned stable, checks_passed, with all 15 required checks passing, including license/cla. The coverage gate shows 100% of the diff covered.
Nits (non-blocking)
- The body's Before evidence shows
AttributeError: no attribute(the tests don't exist yet), which says nothing about behavior. The discrimination run above is the better before-evidence. Consider pasting it. - The task id is
sha256(key)alone, so keys share one namespace across every client of the daemon. Two local clients that reuse a short key likerequest-1with different text get-32602instead of two tasks. The socket is owner-only, so this is acceptable, but a line inprotocol.pytelling callers to use unique keys (for example UUIDs) would help. - The body points at the unreachable
e7c88e95for its diff hash. Future bodies should cite the published head.
keweichen
changed the title
Priority
Level: medium
Reason: Owner-approved reliability improvement.
Closes
N/A — owner-approved work; no public issue is claimed as closed.
Summary
Expose existing retry-safe submission through task.submit
Review first:
src/runtime-api/dispatcher.py,src/runtime-api/protocol.py,tests/runtime-api-tasks-view.test.py.Checklist
gh pr list --repo sonichi/sutando --search "closes #N")git log -1 --format='%ae'shows a GH-mapped email, not*.localornoreply@anthropic.com)upstream/main(or feature isn't already covered)branches:filter can silently exclude a stacked PR)resolve_workspace/resolveWorkspace/claude_home_pathhelpers (seeCLAUDE.md"Workspace contract"; enforced by thelint-workspace-resolution,lint-claude-home-path, andworkspace-leak-checkCI checks)Contribution observations (unchecked items remain reviewer decisions):
Duplicate search: Searched PRs (all states) and open issues using Expose existing retry-safe submission through task.submit, "task.submit", "idempotencyKey". Related results for reviewer comparison: docs(scp): v0 is not local-only — describe the shipped network surface accurately #3565, feat(runtime-api): add idempotent task submission #4488, feat(runtime-api): Sutando Server v0 — local runtime daemon, canonical CLI, instance registry, base views #3303, feat(sparrow): B1 identity contract — canonical constructors, vectors, ratchets (B slice 2) #3315, feat(codex): redeem earned reset near weekly exhaustion #4829, feat(runtime-api): local approval/elicitation/capability RPC — daemon + CLI + durable store (v0) #2325, feat(runtime): expose bounded provider reads to local apps #2753 No close title duplicate was detected; this is not a claim of exhaustive semantic coverage.
Current upstream:
88945c26f75a1df390414efb2af1374975f2bd47. Current main equals the tested source base; required GitHub checks still run on the PR.Diff review: - review-correctness: Inspected dispatcher.py:300-309 and the existing TasksView writer. Parameter membership selects submit_idempotent even for explicit null or blank values, forwarding keys unchanged into writer validation. The existing ValueError handler maps validation and collisions to ProtocolError -32602. Absent keys retain the original submit path and priority default. Full task-view test file passed: 65 tests, exit 0.
review-scope: HEAD is exactly e7c88e95f5ab3e70afffc6a58eb3b3fec60e86dc. Base-to-HEAD changes only the three allowed paths; computed diff SHA-256 is db72604eace9d10105dfe11d440171cb7a2d7027d09bc5cb34b90afe27c6b8be, matching supplied evidence. Git status was clean before and after checks. No durable-writer, CLI, chat retry or cancellation changes.
review-maintainability: The four-line dispatcher addition delegates durable policy to its existing owner without duplication. Protocol documentation names the optional parameter. Tests use real temporary-directory TasksView instances and fresh dispatchers, checking publication count, original bytes, error codes and lifecycle outcomes. git diff --check passed; no added hardcoded host paths.
Documentation: No standalone documentation files changed. The reviewer should assess the code and contract comments for documentation completeness.
Live-path verification: - monitor-user-journey: Ran PYTHONDONTWRITEBYTECODE=1 python3 - <<'PY' with direct RuntimeDispatcher.handle calls over a TemporaryDirectory and real TasksView. Submitted 'Prepare local review notes' with priority low and idempotencyKey monitor-request-1; task.submit and task.status returned the same taskId with state pending. After creating a local result and removing the queue file, task.get_result returned 'Review notes complete'. Script exited 0.
monitor-repeatability: The independent Python journey constructed a fresh dispatcher/view for every call. Output: 'fresh-dispatcher retries: 3; identical ID; all file hashes unchanged'; 'completion retries: 3; done; no requeue; file hashes unchanged'; and 'archive retry: done; file hashes unchanged'. Assertions checked every persisted file's SHA-256 before and after retries.
These observations used the isolated candidate; no production deployment was performed.
Rollback: revert this PR. Production behavior after deployment remains to be observed.
keweichen; CLA status must be observed on the PR.Before / after evidence
Before
Recorded command/output on the pinned source:
/bin/zsh -lc 'python3 tests/runtime-api-tasks-view.test.py DispatchTests.test_submit_idempotency_retry_after_restart_and_completion DispatchTests.test_submit_idempotency_rejects_invalid_keys_and_collisions DispatchTests.test_submit_without_key_remains_non_idempotent TasksViewIdempotencyTests'Exit: 1
Exit: 0
After
acceptance-1: The named dispatcher test passed: fresh dispatcher instances return the same task ID and preserve one publication; completion and archival retries return done without requeueing.
acceptance-2: The named test passed: blank, invalid-type and oversized keys produce ProtocolError -32602 without publication; changed text or priority produces the same error and preserves the original task.
acceptance-3: The named test passed: two identical unkeyed submissions produce distinct task IDs and two files.
acceptance-4: All 17 existing TasksViewIdempotencyTests passed, including concurrency, crash recovery, completion and archive cases.
check-rpc-submission-idempotency: Executed the exact frozen argv independently: exit 0, 20 tests passed in 0.136 seconds, within the 60-second limit.
scope-and-identity: HEAD matches the supplied candidate commit. Base-to-HEAD diff changes only the allowed paths; git diff --check passed and git status --porcelain was empty before and after execution. Dispatcher delegates keyed submissions to the existing writer and retains the existing ValueError-to-parameter-error mapping.
Recorded candidate output:
Observed: PASS; exit 0.
Test plan
Observed: PASS; exit 0.
Canonical review scanner:
Verified candidate:
e7c88e95f5ab3e70afffc6a58eb3b3fec60e86dc. Publication commit:526c639cfe7d0d5e2bd5a8e8f3169857dcbe47a0. Both have tree248169e2b93e35f89e3b0902aa93142283849a9e; only factory-authored commit metadata changed. Acceptance evidence refers to the verified candidate; the scanner ran on the publication tree.