Repository navigation
fix(runtime-api): cancel mirrored human requirements - #5741
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed head 0da838c against base 8e6ebd5.
This is a COMMENT review because the authenticated GitHub login, keweichen, is also the PR author; it cannot count as an approval or a formal change request.
Changes requested before merge:
- Blocking (src/runtime-api/dispatcher.py:573-574): this adds a second production copy of the mirrored human-request type set already used by recover at line 156. Repository architecture explicitly treats duplicated policy as a defect because a later request type can drift between recovery and cancellation. Please define one named constant or helper for the mirrored request types and use it in both branches.
Verification:
- Base reproduction: request.cancel left approval, elicitation, and human_action mirrors pending and active while their runtime rows became cancelled. The same probe at this head cancelled both records for all three types.
- Independent exact-head QA passed 313 focused tests and assertions. The two changed behavioral suites failed against base production code as expected: the integration suite had six failures and the adapter suite had nine errors.
- The real daemon, Unix socket, CLI, SQLite store, HITL writer, restart, stale-action rejection, projection edit, permission failure, and explicit retry paths passed. Terminal winners and unrelated requirements remained unchanged.
- review-checks and git diff --check passed. CI run 39330519245 completed successfully, and license/cla is green.
Worst-case disruption: the runtime DB and HITL file store cannot commit atomically. A mirror-write failure can leave a cancelled runtime row with an active card until request.cancel is retried. The command returns an error instead of false success, and retry repair is verified both before and after restart.
Merge readiness: not ready at this head. The behavior is otherwise sound, but the duplicated policy should be centralized, then the PR still needs two independent maintainer approvals. I did not merge.
|
@cla-assistant check |
|
Addressed review feedback in
Running-product observations:
/bin/zsh -lc "rg -n 'def |CREATE TABLE|INSERT|runtime_requests' src/runtime-api/request_store.py
rg -n 'fail-under|diff-cover|unmeasured|BASE_REF' scripts/coverage-gate.sh
sed -n '1,180p' scripts/coverage_unmeasured.py"Exit: 0 /bin/zsh -lc "python3 - <<'PY'
import ast,json,subprocess,os,time
from pathlib import Path
root=Path.cwd(); base=Path('[local path]'); diag=base/'diagnostics'
env=os.environ.copy(); env.update(json.loads((base/'environment.json').read_text())); env.update(PYTHONDONTWRITEBYTECODE='1',COVERAGE_FILE=str(diag/'.coverage-monitor'),SUTANDO_TELEMETRY='0')
tree=ast.parse(Path('src/runtime-api/dispatcher.py').read_text())
defs=[n for n in tree.body if isinstance(n,ast.Assign) and any(isinstance(t,ast.Name) and t.id=='MIRRORED_REQUEST_TYPES' for t in n.targets)]
assert len(defs)==1
values={n.value for n in ast.walk(defs[0]) if isinstance(n,ast.Constant)}
assert values=={'approval','elicitation','human_action'}
consumers={n.name:[c.lineno for c in ast.walk(n) if isinstance(c,ast.Name) and c.id=='MIRRORED_REQUEST_TYPES'] for n in ast.walk(tree) if isinstance(n,ast.FunctionDef) and n.name in ('recover','_cancel')}
assert all(consumers.get(name) for name in ('recover','_cancel'))
sets=[n.lineno for n in ast.walk(tree) if isinstance(n,(ast.Set,ast.Tuple,ast.List)) and len(n.elts)==3 and all(isinstance(e,ast.Constant) for e in n.elts) and {e.value for e in n.elts}==values]
assert len(sets)==1
feedback={'definition_line':defs[0].lineno,'types':sorted(values),'consumers':consumers,'literal_occurrences':sets}
(diag/'monitor-feedback-1.json').write_text(json.dumps(feedback,indent=2)); print(json.dumps(feedback))
commands=[['python3','-m','coverage','run','--rcfile=.coveragerc','tests/runtime-api-dispatcher.test.py'],['python3','-m','coverage','run','--rcfile=.coveragerc','tests/runtime-api-human-action.test.py'],['python3','-m','coverage','combine','--rcfile=.coveragerc'],['python3','-m','coverage','xml','--rcfile=.coveragerc','-o',str(diag/'monitor-coverage.xml')],['diff-cover',str(diag/'monitor-coverage.xml'),'--compare-branch=8e6ebd5f857a99044f0a0b2d1b3dd4cc95b2a8fe','--fail-under=95','--markdown-report',str(diag/'monitor-diff-coverage.md')],['python3','scripts/coverage_unmeasured.py','8e6ebd5f857a99044f0a0b2d1b3dd4cc95b2a8fe',str(diag/'monitor-coverage.xml')]]
results=[]
for i,argv in enumerate(commands):
start=time.monotonic(); p=subprocess.run(argv,env=env,cwd=root,text=True,capture_output=True,timeout=60)
(diag/('monitor-coverage-%d.log'%i)).write_text(p.stdout+p.stderr)
row={'argv':argv,'exit_code':p.returncode,'elapsed_seconds':round(time.monotonic()-start,3),'tail':(p.stdout+p.stderr)[-600:]}; results.append(row); print(json.dumps(row))
if p.returncode: break
(diag/'monitor-coverage-commands.json').write_text(json.dumps(results,indent=2))
assert len(results)==len(commands) and all(r['exit_code']==0 for r in results)
PY"Exit: 0 Regression checks:
/bin/zsh -c "python3 - <<'PY'
import json,os,subprocess,time,datetime
from pathlib import Path
p=Path('[local path]'); d=p/'diagnostics'
env=os.environ.copy(); env.update(json.loads((p/'environment.json').read_text())); env.update(PYTHONDONTWRITEBYTECODE='1',COVERAGE_FILE=str(d/'.verify-coverage'))
commands=[['python3','-m','coverage','run','--rcfile=.coveragerc','tests/runtime-api-dispatcher.test.py'],['python3','-m','coverage','run','--rcfile=.coveragerc','tests/runtime-api-human-action.test.py'],['python3','-m','coverage','combine','--rcfile=.coveragerc'],['python3','-m','coverage','xml','--rcfile=.coveragerc','-o',str(d/'verify-coverage.xml')],['diff-cover',str(d/'verify-coverage.xml'),'--compare-branch=8e6ebd5f857a99044f0a0b2d1b3dd4cc95b2a8fe','--fail-under=95','--markdown-report',str(d/'verify-diff-coverage.md')],['python3','scripts/coverage_unmeasured.py','8e6ebd5f857a99044f0a0b2d1b3dd4cc95b2a8fe',str(d/'verify-coverage.xml')]]
receipts=[]
for i,argv in enumerate(commands):
start=datetime.datetime.now(datetime.timezone.utc).isoformat()
r=subprocess.run(argv,env=env,text=True,capture_output=True,timeout=90)
log=d/f'verify-coverage-{i}.log'; log.write_text(r.stdout+r.stderr)
rec={'argv':argv,'start':start,'stop':datetime.datetime.now(datetime.timezone.utc).isoformat(),'exit':r.returncode,'log':str(log)}
receipts.append(rec); print(json.dumps(rec),flush=True)
print((r.stdout+r.stderr)[-1600:],flush=True)
if r.returncode: break
(d/'verify-coverage-commands.json').write_text(json.dumps(receipts,indent=2))
raise SystemExit(r.returncode)
PY"Exit: 0 /bin/zsh -c "python3 - <<'PY'
import ast,json,hashlib
from pathlib import Path
root=Path.cwd(); p=root/'src/runtime-api/dispatcher.py'; tree=ast.parse(p.read_text())
types={'approval','elicitation','human_action'}
defs=[n for n in tree.body if isinstance(n,ast.Assign) and any(isinstance(t,ast.Name) and t.id=='MIRRORED_REQUEST_TYPES' for t in n.targets)]
assert len(defs)==1
assert isinstance(defs[0].value,ast.Call) and defs[0].value.func.id=='frozenset'
assert set(ast.literal_eval(defs[0].value.args[0]))==types
branches={}
for f in ast.walk(tree):
if isinstance(f,ast.FunctionDef) and f.name in {'recover','_cancel'}:
uses=[n.lineno for n in ast.walk(f) if isinstance(n,ast.Name) and n.id=='MIRRORED_REQUEST_TYPES']
assert len(uses)==1,(f.name,uses)
branches[f.name]=uses
copies=[]
for n in ast.walk(tree):
if isinstance(n,(ast.Set,ast.Tuple,ast.List)):
try: value=ast.literal_eval(n)
except (ValueError,TypeError): continue
if all(isinstance(v,str) for v in value) and set(value)==types: copies.append(n.lineno)
assert len(copies)==1
receipt={'constant_definition':defs[0].lineno,'types':sorted(types),'shared_uses':branches,'literal_set_copies':copies,'dispatcher_sha256':hashlib.sha256(p.read_bytes()).hexdigest()}
out=Path('[local path]'); out.write_text(json.dumps(receipt,indent=2)); print(json.dumps(receipt))
log=Path('[local path]').read_text()
events=[]
for line in log.splitlines():
i=line.find('{\"event\":')
if i>=0: events.append(json.loads(line[i:]))
starts=[r for r in events if r['event']=='daemon-start']; ready=[r for r in events if r['event']=='ready']; stops=[r for r in events if r['event']=='daemon-stop']
assert len(starts)==len(stops)==len(ready)==7
assert {r['pid'] for r in starts}=={r['pid'] for r in stops}
assert all(r['exit_code']==0 and r['reaped'] for r in stops)
assert all(Path(r['socket']).is_relative_to('[local path]') for r in starts)
cli=[r for r in events if r['event']=='cli']; failures=[r for r in cli if r['exit_code']"'!=0]
summary={'"'processes':[{'pid':s['pid'],'argv':s['argv'],'socket':s['socket'],'start':s['at'],'ready':next(r['at'] for r in ready if r['pid']==s['pid']),'stop':next(r['at'] for r in stops if r['pid']==s['pid']),'reaped':True} for s in starts],'cli_count':len(cli),'errors':failures,'cancelled_pairs':[r for r in events if r['event']=='cancelled-pair'],'projection_count':sum(r['event']=='local-projection' for r in events)}
Path('[local path]').write_text(json.dumps(events,indent=2))
Path('[local path]').write_text(json.dumps(summary,indent=2))
print(json.dumps(summary,indent=2))
PY"Exit: 0 Independent verification, code review and product checks passed on candidate |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-owner approve at 949505d, CI green. MIRRORED_REQUEST_TYPES is now the single source in recover() and _cancel(). _cancel only cancels the mirror when the store's CAS transition actually landed 'cancelled', so a request that has already resolved, expired or been cancelled skips it. HitlManager._terminate is a no-op on unknown or terminal ids, so a double cancel is safe. Red-first: with dispatcher.py and ha_adapter.py reverted to base, there are 6 cancellation failures and 9 adapter errors. Merge-order note: #5742 is not stacked on this PR and still hardcodes the ('approval','elicitation','human_action') tuple in _settle. Whichever merges second should rebase and use MIRRORED_REQUEST_TYPES instead of adding a second copy.
Priority
Level: medium
Reason: Owner-approved reliability improvement.
Closes
N/A — owner-approved work; no public issue is claimed as closed.
Summary
fix(runtime-api): cancel mirrored human requirements
Review first:
src/runtime-api/dispatcher.py,src/runtime-api/ha_adapter.py,tests/runtime-api-dispatcher.test.py,tests/runtime-api-human-action.test.py,tests/runtime-api-request-cancellation.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 fix(runtime-api): cancel mirrored human requirements, "request.cancel", "task.cancel". Related results for reviewer comparison: docs(scp): v0 is not local-only — describe the shipped network surface accurately #3565, fix(runtime-api): emit recognized cancellation instructions #5535, feat(runtime-api): Sutando Server v0 — local runtime daemon, canonical CLI, instance registry, base views #3303 Returned PRs were compared against current files and overlapping diffs: docs(scp): v0 is not local-only — describe the shipped network surface accurately #3565: Different changed files.; fix(runtime-api): emit recognized cancellation instructions #5535: Different changed files.; 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.. Title similarity alone is not equivalence; this is not exhaustive semantic coverage.
Upstream observed during evidence preparation:
8e6ebd5f857a99044f0a0b2d1b3dd4cc95b2a8fe. Main matched the tested source base at preparation; PR checks and review remain required.Diff review: - review-correctness: Inspected the full change against original base 8e6ebd5 and traced dispatcher, adapter, stores, task status, projection and RPC error handling. Cancellation re-reads durable status, reconciles previously cancelled human requests and uses the deterministic mirror ID after restart. Existing terminal guards preserve outcomes. Mirror-write errors propagate instead of returning success.
review-scope: Only the five allowed paths changed. Regenerated patch SHA-256 matches 672286b833a55a7412b3556a8eecb5e96378c5e510240d84501fbdef49f60212. The repair relative to HEAD's parent adds one blank line between imports. Task cancellation, worker recovery, capability execution semantics, historical cleanup, transport, card wording and desktop changes remain excluded.
review-maintainability: Dispatcher orchestration delegates through HumanActionAdapter.cancel to HitlManager.cancel. Locking, terminal-state protection and atomic persistence remain in the existing shared writer. No duplicate persistence policy, schema migration or new configuration was introduced. Tests cover behavior and delegation.
Documentation: No standalone documentation files changed. The reviewer should assess the code and contract comments for documentation completeness.
Live-path verification: - monitor-user-journey: Executed python3 [local path] at 2026-10-19T21:47:19Z–21:47:26Z: 60 real bin/sutando calls against this checkout's server.py on the supplied private socket. Initial daemon PID 60309 reached CLI readiness in 0.381 seconds. Commands included approval request, elicitation request, human-action request and request cancel. For example, bin/sutando request cancel approval-dfe1c4cd74e6 returned status cancelled; both durable records were cancelled at mirror revision 2, the old action was rejected, the local projection edited the existing card to cancelled, and its task returned to pending. All three types passed. Transcript: monitor-journey.jsonl.
monitor-repeatability: Repeated bin/sutando request cancel for all three cancelled requests before and after daemon restart; snapshots, revisions and record inventory remained unchanged, and another projection drive emitted nothing. A controlled write failure made bin/sutando request cancel human_action-e6fa5293eb9a exit 1 with -32000 and Permission denied. Restart alone preserved the cancelled runtime row and pending mirror. Retrying the same command cancelled that mirror, advancing revision 1 to 2 exactly once; another retry changed nothing. Four journey daemons exited 0 and were reaped. Commands, outputs and timestamps: monitor-journey.jsonl.
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
Exit: 0
After
acceptance-1: Real server.py and bin/sutando journeys passed for approval, elicitation and human_action. Both durable records became cancelled; requirements left active(); previous actions were rejected as stale; local projections edited existing cards to cancelled. Associated tasks returned to pending with unchanged task files and no duplicates.
acceptance-2: Unrelated requirements remained actionable and resolved normally. Approved, denied, resolved, completed and declined outcomes and answers remained unchanged. Unknown IDs returned exit 1 with -32602. Unmirrored cancellation created no requirement. An independent socket witness confirmed another pending request keeps its shared task waiting.
acceptance-3: Repeated cancellation and restart preserved record inventories, snapshots and revisions. Explicit retry after restart repaired a cancelled runtime row with an active mirror; restart alone did not reconcile it. An independent witness also confirmed retry preserved an archived cancelled requirement without recreating its live file.
acceptance-4: Directory-permission failures exercised the production mirror writer and returned CLI exit 1 with -32000 and Permission denied. Restoring permissions and retrying reconciled the same records, including after restart. A mirror resolved before retry retained its terminal outcome and answer. The independent approval witness confirmed exactly one mirror revision increment.
acceptance-5: The new integration suite and both extended focused suites passed using the supplied scratch environment and private sockets. Startup, CLI and shutdown waits were bounded; fixtures were synthetic and local. All 17 daemon launches across frozen, coverage and independent boundary runs reached readiness and were reaped. Scoped ps was unavailable during the boundary run; direct Popen PID, CLI readiness and wait receipts established lifecycle.
check-request-cancel-integration: python3 tests/runtime-api-request-cancellation.test.py exited 0: four tests passed, 64 CLI calls and seven successfully reaped daemons. Ran 2026-10-19T21:40:10.949Z–21:40:18.891Z in 7.942 seconds, within 120 seconds.
check-request-cancel-dispatcher: python3 tests/runtime-api-dispatcher.test.py exited 0 in 1.275 seconds, within 60 seconds. Delegation, terminal preservation, durable-state race and restart reconciliation assertions passed.
check-request-cancel-adapter: python3 tests/runtime-api-human-action.test.py exited 0: 17 tests passed in 0.215 seconds, within 60 seconds.
repair-review-comment-cap: git diff 8e6ebd5 HEAD | bash scripts/review-checks.sh exited 0. The unchanged scanner reported hardcoded-paths, root-artifacts and prose-cap clean.
coverage: Reran all three suites with python3 -m coverage run --rcfile=.coveragerc, combined results and exported XML. diff-cover against original base 8e6ebd5 with --fail-under=95 exited 0: six changed executable lines, zero missing, 100% coverage. Dispatcher coverage was 71%; adapter coverage was 91%. Commands and measurements are in coverage/commands.json.
checkout-integrity: Final HEAD matches the pinned candidate; git status --porcelain=v1 --untracked-files=all is empty. Working-tree and base-to-candidate diff checks passed. Only the five approved paths changed. Regenerated patch SHA-256 matches 672286b833a55a7412b3556a8eecb5e96378c5e510240d84501fbdef49f60212. Scanners, coverage configuration and CI workflow remain unchanged.
Test plan
Observed: PASS; exit 0.
Observed: PASS; exit 0.
Observed: PASS; exit 0.
Canonical review scanner:
Verified candidate:
a4a2a4581cf0285ba9d8e0e78c0ef4e3dcb84c6d. Publication commit:0da838c81b615f1cbdf65f51af5d52da01b61f79. Both have tree14d82d215be195362dda2a1c6f890b33ba318653; only factory-authored commit metadata changed. Acceptance evidence refers to the verified candidate; the scanner ran on the publication tree.