Repository navigation
feat(runtime-cli): support idempotency keys in task submit - #5595
Conversation
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at dacc048. Verified myself:
- The server-side idempotency machinery (
src/runtime-api/dispatcher.py,request_store.py,tasks_view.py.submit_idempotent) already exists and already handles key storage, fingerprint mismatch on key reuse with different task text, and recovery after a lost response or daemon restart.protocol.pyalready documentstask.submit {task, priority?, idempotencyKey?}. This PR is exactly what it says: a thin CLI flag (--idempotency-key) that forwardsidempotencyKeyinto the existing RPC call — no new server logic, 15 lines insutando-runtime.py. - Ran
tests/runtime-cli-task-idempotency.test.pymyself: 10/10 pass, including real spawned-daemon scenarios (test_lost_response_retry_after_daemon_restart,test_lost_response_retry_same_daemon) — not just mocks. scripts/review-checks.shon the diff: clean.- Server-side validation already rejects an empty/whitespace key (
tasks_view.py:105), so the CLI doesn't need to duplicate that check.
No blockers. Approving.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at head dacc048154311f5324837092cd60e1e72b41d777 (one commit on f7cb1131; diff is src/runtime-cli/sutando-runtime.py +11/−4 and a new tests/runtime-cli-task-idempotency.test.py). Fresh worktree.
What the diff does, checked against the tree. The CLI gains --idempotency-key on task submit and forwards it as idempotencyKey only when given (sutando-runtime.py:1009-1012); an unkeyed submit sends exactly the old params. Nothing else changes. The policy is where the architecture rules put it: server.py is transport (its own comment at :139), dispatcher.py:303-306 routes a keyed submit to TasksView.submit_idempotent, and the receipt is a hard link under tasks/.runtime-api-idempotency/<sha256(key)>.task with the task id derived from the key (tasks_view.py:97-130). That backend predates this PR (#2542 lineage) and keeps its own tests; the PR adds no dedupe machinery of its own.
My own probe, real server.py + real CLI over a Unix socket in an isolated workspace (same fixture env as the PR test):
same key x2 → same taskId, state pending both times, queue 1
different key → second taskId, queue 2
same key, other text → exit 1 {"error": "-32602: idempotency key is already bound to a different task submission"}, queue unchanged
same key, other prio → same refusal, queue unchanged
unkeyed x2 → two taskIds, queue 4
daemon terminated and restarted, same key → original taskId, state pending, queue unchanged
task moved to tasks/archive, same key → original taskId, state done, nothing requeued (queue 3)
So the key dedupes, a different body under the same key is refused rather than replayed, and the receipt survives the dispatcher/runtime boundary and a daemon restart.
Suites at head: runtime-cli-task-idempotency 10/10, runtime-api-tasks-view 73/73, and every other runtime-api-* / runtime-cli-* suite (26 files) green; scripts/review-checks.sh --diff PASS.
REVIEW.md §15. This is a live path (CLI → Unix socket → runtime-api submit). The body's witness is a real one: the daemon run as a subprocess, a transport-only relay dropping the successful response, then the identical command recovering the original id both on the same daemon and after replacing the daemon PID (test_lost_response_retry_after_daemon_restart is that scenario, and the body's monitor run repeats it by hand). I reproduced the restart case above. It is on an isolated candidate, not the installed service — for a flag that only forwards a parameter to a backend that already shipped with its own round trip, that is sufficient; noting it so the owner knows what was and was not exercised.
Nits, non-blocking: the body's checklist boxes are all unticked although the text below them answers each; and --idempotency-key help could say the key is scoped to one workspace's tasks/ (the receipt lives there), since an operator retrying against a different host gets a new task.
APPROVE at dacc0481. Merge is the owner's.
|
Follow-up to my approval: the "diff coverage >= 95% (python)" gate is now red — missed line 1011 in Root cause, confirmed by reading the actual CI step: Fix, same pattern already used by PYBASE = [sys.executable]
if os.environ.get("SUTANDO_TEST_SUBPROCESS_COVERAGE") == "1":
PYBASE += ["-m", "coverage", "run", f"--rcfile={ROOT / '.coveragerc'}"]then use Not touching your PR myself — flagging per your own instruction ("if blocked, reply here with the issue"). Review: #5595 (review) (or see the PR's review list for mine, approved before this gate result came in — approval still stands, this is a CI mechanics gap, not a code concern). |
|
@cla-assistant check |
|
Addressed review feedback in
Running-product observations:
/bin/zsh -c "python3 - <<'PY'
import os,json,pathlib,subprocess,shutil,datetime,time,xml.etree.ElementTree as ET
R=pathlib.Path.cwd(); B=pathlib.Path('[local path]'); D=B/'diagnostics'; C=D/'coverage'; E=os.environ.copy(); E.update(json.loads((B/'environment.json').read_text())); E.update(PYTHONDONTWRITEBYTECODE='1',TMPDIR=str(B/'tmp'),SUTANDO_TELEMETRY='0',COVERAGE_GATE_PRECOMPUTED='1',BASE_REF='f7cb113121e840bd1564232f00b2c8d8e42ee5a5')
E.pop('COVERAGE_GATE_FAIL_UNDER',None); E.pop('COVERAGE_FILE',None)
created=['.coverage','coverage.xml','diff-cover.md','coverage-summary.md']
assert all(not (R/name).exists() for name in created),'would replace existing coverage artifact'
try:
shutil.copy2(C/'.coverage',R/'.coverage'); shutil.copy2(C/'coverage.xml',R/'coverage.xml')
started=datetime.datetime.now(datetime.timezone.utc).isoformat(); begin=time.monotonic(); argv=['bash','scripts/coverage-gate.sh']
p=subprocess.run(argv,cwd=R,env=E,text=True,capture_output=True,timeout=90)
(C/'product-gate.log').write_text(p.stdout+p.stderr)
print(json.dumps({'argv':argv,'env':{'BASE_REF':E['BASE_REF'],'COVERAGE_GATE_PRECOMPUTED':'1'},'at':started,'exit':p.returncode,'seconds':round(time.monotonic()-begin,3)})); print(p.stdout); print(p.stderr)
(C/'product-gate-receipt.json').write_text(json.dumps({'argv':argv,'base':E['BASE_REF'],'at':started,'exit':p.returncode}))
for name in ('diff-cover.md','coverage-summary.md'):
if (R/name).exists(): shutil.copy2(R/name,C/name)
assert p.returncode==0
assert 'Total: 5 lines' in p.stdout and 'Missing: 0 lines' in p.stdout and 'Coverage: 100%' in p.stdout
xml=ET.parse(C/'coverage.xml'); observed={}
for cls in xml.findall('.//class'):
if cls.get('filename')=='src/runtime-cli/sutando-runtime.py':
observed={int(line.get('number')):int(line.get('hits')) for line in cls.findall('lines/line') if int(line.get('number')) in (878,1009,1010,1011,1012)}
print('changed-line-hits',observed); assert len(observed)==5 and all(observed.values())
(C/'changed-line-hits.json').write_text(json.dumps(observed))
finally:
for name in created: (R/name).unlink(missing_ok=True)
for argv in (['git','rev-parse','HEAD'],['git','status','--porcelain'],['git','diff','--exit-code'],['git','diff','--cached','--exit-code']):
p=subprocess.run(argv,cwd=R,env=E,text=True,capture_output=True,timeout=10); print(json.dumps({'argv':argv,'exit':p.returncode,'stdout':p.stdout,'stderr':p.stderr})); assert p.returncode==0
PY"Exit: 1 /bin/zsh -c "python3 - <<'PY'
from pathlib import Path
import json,xml.etree.ElementTree as ET,subprocess,hashlib,datetime
b=Path('[local path]'); d=b/'diagnostics'; c=d/'coverage'; root=ET.parse(c/'coverage.xml'); rows=[]
for cls in root.findall('.//class'):
if cls.get('filename','').endswith('runtime-cli/sutando-runtime.py'):
hits={int(x.get('number')):int(x.get('hits')) for x in cls.findall('lines/line') if int(x.get('number')) in (878,1009,1010,1011,1012)}; rows.append({'xml_filename':cls.get('filename'),'hits':hits}); assert len(hits)==5 and all(hits.values())
assert len(rows)==1; print(json.dumps(rows)); (c/'changed-line-hits.json').write_text(json.dumps(rows,indent=2))
records=[]
for argv in (['git','rev-parse','HEAD'],['git','status','--porcelain'],['git','diff','--exit-code'],['git','diff','--cached','--exit-code'],['git','diff','dacc048154311f5324837092cd60e1e72b41d777','HEAD','--name-only'],['git','diff','f7cb113121e840bd1564232f00b2c8d8e42ee5a5','HEAD','--','.coveragerc','scripts/coverage-gate.sh']):
p=subprocess.run(argv,text=True,capture_output=True,timeout=10); r={'argv':argv,'exit':p.returncode,'stdout':p.stdout,'stderr':p.stderr}; records.append(r); print(json.dumps(r)); assert p.returncode==0
if argv[1] in ('status',) or argv[-1]=='scripts/coverage-gate.sh': assert not p.stdout
(d/'final-checkout.json').write_text(json.dumps(records,indent=2))
for name in ['cli-task-idempotency','instrumented-cli-task-idempotency']:
events=[]
for line in (d/(name+'.log')).read_text().splitlines():
if line.startswith('{'):
try: events.append(json.loads(line))
except ValueError: pass
started={e['pid'] for e in events if e.get('event')=='daemon-ready'}; reaped={e['pid'] for e in events if e.get('event')=='daemon-reaped' and e.get('returncode')==0}
r={'log':name,'daemon_ready':len(started),'daemon_reaped':len(reaped),'all_reaped':started==reaped}; print(json.dumps(r)); assert started==reaped and len(started)==13
(d/'artifact-hashes.json').write_text(json.dumps({str(p.relative_to(d)):hashlib.sha256(p.read_bytes()).hexdigest() for p in d.rglob('*') if p.is_file() and p.name"'!='"'artifact-hashes.json'},indent=2))
PY"Exit: 0 Regression checks:
/bin/zsh -c "python3 - <<'PY'
import os,json,subprocess,time
from pathlib import Path
base=Path('[local path]'); diag=base/'diagnostics'; cov=diag/'coverage'; cov.mkdir(exist_ok=True)
env=os.environ.copy();env.update(json.loads((base/'environment.json').read_text()));env.update(TMPDIR=str(base/'tmp'),PYTHONDONTWRITEBYTECODE='1',SUTANDO_TELEMETRY='0',SUTANDO_TEST_SUBPROCESS_COVERAGE='1',COVERAGE_FILE=str(cov/'.coverage'))
commands=[['python3','-m','coverage','--version'],['diff-cover','--version'],['python3','-m','coverage','run','--rcfile=.coveragerc','tests/runtime-cli-task-idempotency.test.py'],['python3','-m','coverage','combine','--rcfile=.coveragerc'],['python3','-m','coverage','xml','--rcfile=.coveragerc','-o',str(cov/'coverage.xml')],['diff-cover',str(cov/'coverage.xml'),'--compare-branch=f7cb113121e840bd1564232f00b2c8d8e42ee5a5','--fail-under=95','--markdown-report',str(cov/'diff-cover.md')]]
receipts=[]
for i,argv in enumerate(commands):
start=time.time();log=cov/f'command-{i+1}.log'
with log.open('w') as out:r=subprocess.run(argv,env=env,stdout=out,stderr=subprocess.STDOUT,timeout=180)
receipt=dict(argv=argv,start=start,end=time.time(),exit=r.returncode,log=str(log));receipts.append(receipt)
print(json.dumps(receipt),flush=True);print(log.read_text()[-2500:],flush=True)
(cov/'receipts.json').write_text(json.dumps(receipts,indent=2))
if r.returncode:break
PY"Exit: 0 /bin/zsh -c "python3 - <<'PY'
import json,os,shutil,subprocess,time
from pathlib import Path
root=Path.cwd();base=Path('[local path]');cov=base/'diagnostics/coverage'
env=os.environ.copy();env.update(json.loads((base/'environment.json').read_text()));env.update(TMPDIR=str(base/'tmp'),PYTHONDONTWRITEBYTECODE='1',BASE_REF='f7cb113121e840bd1564232f00b2c8d8e42ee5a5',COVERAGE_GATE_PRECOMPUTED='1',SUTANDO_TELEMETRY='0');env.pop('COVERAGE_FILE',None);env.pop('COVERAGE_GATE_FAIL_UNDER',None)
artifacts=[root/name for name in ('.coverage','coverage.xml','diff-cover.md','coverage-summary.md','coverage-gate-report.md')]
assert not any(p.exists() for p in artifacts), 'Refusing to overwrite existing artifacts'
tracked=set(subprocess.check_output(['git','ls-files'],text=True).splitlines());assert not any(p.name in tracked for p in artifacts)
try:
for name in ('.coverage','coverage.xml'):shutil.copyfile(cov/name,root/name)
argv=['bash','scripts/coverage-gate.sh'];start=time.time()
with (cov/'product-gate.log').open('w') as out:r=subprocess.run(argv,env=env,stdout=out,stderr=subprocess.STDOUT,timeout=90)
print((cov/'product-gate.log').read_text())
receipt={'argv':argv,'BASE_REF':env['BASE_REF'],'COVERAGE_GATE_PRECOMPUTED':'1','threshold':'default 95 unchanged','data':'10 passing real-transport CLI tests, subprocess coverage enabled','start':start,'end':time.time(),'exit':r.returncode}
print(json.dumps(receipt));(cov/'product-gate-receipt.json').write_text(json.dumps(receipt,indent=2))
for name in ('diff-cover.md','coverage-summary.md','coverage-gate-report.md'):
if (root/name).exists():shutil.copyfile(root/name,cov/('product-'+name))
finally:
for p in artifacts:p.unlink(missing_ok=True)
PY"Exit: 0 Independent verification, code review and product checks passed on candidate |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-reviewed at bfd7cf7. The fix is exactly the two-line SUTANDO_TEST_SUBPROCESS_COVERAGE wrapper I flagged — verified it myself rather than trusting the diff: ran the real test suite under coverage run with the wrapper active (10/10 still pass, functional behavior unchanged), then ran diff-cover against origin/main the same way CI does: 100% diff coverage on sutando-runtime.py (was 80%). scripts/review-checks.sh clean.
No remaining blockers from me. Approving at this head (my prior approval was stale, at dacc048).
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-review at head bfd7cf71896a2d5b9cc17eb3da6f0a64d631a971 (one commit over dacc0481). Fresh worktree.
The delta is test-only: tests/runtime-cli-task-idempotency.test.py +6/−2 — a PYBASE prefix that runs the daemon and CLI subprocesses under python -m coverage run --rcfile=.coveragerc when SUTANDO_TEST_SUBPROCESS_COVERAGE=1, otherwise plain sys.executable as before. src/ is unchanged from the head I approved (git diff dacc0481 bfd7cf71 -- src/ is empty), so every finding in that review stands.
The keyed branch is exercised and the test is load-bearing: at head the suite passes 10/10; with the two forwarding lines (sutando-runtime.py:1010-1011, if args.idempotency_key is not None: params["idempotencyKey"] = …) removed, six tests fail (test_archived_retry_does_not_requeue, test_completed_retry_does_not_requeue, four test_invalid_keys_rejected_by_backend_without_publication cases). The file is picked up by scripts/discover-python-tests.sh.
Coverage gate at this head: diff coverage >= 95% (python) is SUCCESS, gate comment "Total: 5 lines, Missing: 0 lines, Coverage: 100%" — the previously uncovered line 1011 is now hit. scripts/review-checks.sh --diff PASS on the full diff.
My earlier review had no inline threads, so nothing to resolve. APPROVE at bfd7cf71. Merge is the owner's.
Priority
Level: medium
Reason: Owner-approved reliability improvement.
Closes
N/A — owner-approved work; no public issue is claimed as closed.
Summary
feat(runtime-cli): support idempotency keys in task submit
Review first:
src/runtime-cli/sutando-runtime.py,tests/runtime-cli-task-idempotency.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 feat(runtime-cli): support idempotency keys in task submit, "idempotencyKey". Related results for reviewer comparison: 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(runtime-api): support idempotency keys in task.submit #5490, feat(codex): redeem earned reset near weekly exhaustion #4829, fix(runtime-api): tolerate task claims during listing #5538, feat(runtime-api): local approval/elicitation/capability RPC — daemon + CLI + durable store (v0) #2325, feat(runtime): expose bounded provider reads to local apps #2753 Returned PRs were compared against current files and overlapping diffs: feat(runtime-api): Sutando Server v0 — local runtime daemon, canonical CLI, instance registry, base views #3303: Related files, different patch; current-source reproduction and independent review remain required.; feat(sparrow): B1 identity contract — canonical constructors, vectors, ratchets (B slice 2) #3315: Different changed files.; feat(runtime-api): support idempotency keys in task.submit #5490: Different changed files.; feat(codex): redeem earned reset near weekly exhaustion #4829: Different changed files.; fix(runtime-api): tolerate task claims during listing #5538: Different changed files.; feat(runtime-api): local approval/elicitation/capability RPC — daemon + CLI + durable store (v0) #2325: Related files, different patch; current-source reproduction and independent review remain required.; feat(runtime): expose bounded provider reads to local apps #2753: Related files, different patch; current-source reproduction and independent review remain required.. Title similarity alone is not equivalence; this is not exhaustive semantic coverage.
Current upstream:
f7cb113121e840bd1564232f00b2c8d8e42ee5a5. Current main equals the tested source base; required GitHub checks still run on the PR.Diff review: - review-correctness: Inspected the complete diff and relevant callers. src/runtime-cli/sutando-runtime.py:878 defines the optional argument; lines 1009–1012 forward its supplied value unchanged only when non-None. Empty values therefore reach backend validation, while omitted options preserve legacy submission. Reviewed dispatcher.py:295 and tasks_view.py:97–145 for validation, durable receipts, conflicts and lifecycle recovery. Existing RPC timeout, error handling and output remain unchanged. Fresh real-socket tests confirmed these interactions.
review-scope: HEAD is exactly 674ecea6d90e3bb8778858413b9b2e77f754c33e; its parent is f7cb113. The reconstructed diff SHA-256 exactly matches supplied e7329cb78014eb3d5064c512aacac1ed5ddd72a7e0b68efe85a21c6569bef564. Only the two allowed paths changed. No automatic retries, interactive-chat changes, reconnect changes or new deduplication machinery were added. Final git status was empty.
review-maintainability: The production change is limited to argument declaration, help and conditional RPC parameter construction. Validation and persistence retain their existing owners. Reviewed all 341 test lines: fixture helpers exercise real CLI/server subprocesses, the relay only forwards transport frames, lifecycle fixtures are explicitly identified, and cleanup is registered before startup. No added hardcoded host paths were found; git diff --check passed.
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 python3 [local path] exit 0, at 2026-10-15T19:24:30–19:24:33Z. It launched python3 src/runtime-api/server.py from the candidate; CLI task list returned {"tasks":[]} after 0.320s. Executed python3 src/runtime-cli/sutando-runtime.py task submit 'Summarize synthetic notes for the retry demo' --priority urgent --idempotency-key monitor-notes-20261015 through a transport-only Unix-socket relay. The relay captured a successful publication and discarded its response; CLI exited 1. After restarting the daemon, the identical command returned task-rtapi-efcf610c8e6b613598e0c52da86e7827071b7f9d82c28b8f1cf1cc6e215465c3, state pending, with exactly one task. Full commands, outputs and file snapshots: monitor-journey2.jsonl. An earlier observer-only assertion expected an untrimmed result; it was corrected to the documented trimming contract, with both attempts preserved.
monitor-repeatability: Repeated python3 src/runtime-cli/sutando-runtime.py task submit 'Summarize synthetic notes for the retry demo' --priority urgent --idempotency-key monitor-notes-20261015 twice after replacing daemon PID 20581 with PID 20594. Both exited 0 and returned the original taskId, state pending. Queue count remained 1; task and receipt SHA-256, inode and modification time were unchanged. After moving the synthetic task into tasks/archive, the same command returned that taskId with state done and an empty queue. Commands and outputs: monitor-journey2.jsonl, retry-recovered and archive-retry events.
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:
Exit: 0
After
acceptance-1: The real-socket CLI integration test confirmed separate invocations with identical key, text and priority return the same taskId with exactly one queued task and unchanged persisted files. Transport capture confirmed verbatim key forwarding, including Unicode and surrounding whitespace.
acceptance-2: A transport-only relay discarded the successful daemon response after publication. Explicit CLI retries recovered the captured taskId with one unchanged publication, both without restart and after replacing daemon PID 92301 with PID 92303. Both lost-response-recovered receipts are preserved in verify-cli-task-idempotency.json.
acceptance-3: Completed and archived submission tests returned the original taskId with state done after daemon restart. Neither recreated a queue entry or changed persisted task/result files. Lifecycle states were synthetic filesystem fixtures; no task consumer was run.
acceptance-4: Two identical unkeyed submissions and two submissions with different keys produced four distinct taskIds and four task files. Transport capture confirmed unkeyed CLI requests omit idempotencyKey.
acceptance-5: Reusing a key with changed text or priority returned backend conflict errors without changing the original task or receipt. Empty, whitespace-only, 1025-byte ASCII and 1026-byte Unicode keys failed without publication. A 1024-byte Unicode key succeeded.
acceptance-6: Help assertions verified the option, explicit retry semantics, completion/restart behavior and absence of automatic retries. Tests inherited the supplied scratch environment and PATH, used isolated state and real Unix sockets beneath the private runtime, and bounded execution. All 13 daemon processes exited successfully, were reaped and were confirmed absent; relay threads joined and temporary fixtures were removed. Startup logs, readiness responses and cleanup evidence are retained in verify-cli-task-idempotency.json and verify-identity.json.
check-cli-task-idempotency: python3 tests/runtime-cli-task-idempotency.test.py ran on the pinned candidate from 2026-10-15T19:17:04.121652Z to 19:17:09.461976Z with a 90-second timeout; exit 0. Output: 'Ran 10 tests in 5.119s', 'OK'. Full output: [local path]
check-existing-task-contract: python3 tests/runtime-api-tasks-view.test.py ran on the pinned candidate from 2026-10-15T19:17:04.121749Z to 19:17:05.675355Z with a 90-second timeout; exit 0. Output: 'Ran 73 tests in 1.325s', 'OK'. Full output: [local path]
Test plan
Observed: PASS; exit 0.
Observed: PASS; exit 0.
Canonical review scanner:
Verified candidate:
674ecea6d90e3bb8778858413b9b2e77f754c33e. Publication commit:dacc048154311f5324837092cd60e1e72b41d777. Both have tree90c5105ace3339c2c7b4342bc2a90aac50016da1; only factory-authored commit metadata changed. Acceptance evidence refers to the verified candidate; the scanner ran on the publication tree.