Add durable approval/decision primitive - #2
Merged
Merged
Conversation
Jobs can now ask a human a question and wait for the answer without holding a thread or interrupting the job. A decision is its own small state machine keyed by `decision_id` (dec_<hex>), kept in a `decisions` table so it never overloads `jobs.status`; the job keeps running (or stays queued) while it waits. - `job_request_decision(job_id, prompt, options=["approve","deny"], timeout_seconds=None)` inserts a pending decision and emits `decision_requested`. That reuses the ordinary event->wake-target->delivery path, so the owning thread is notified through the mechanism it already registered (targets must list `decision_requested` in their events). - `job_resolve(job_id, token, choice)` records one of the offered options. Resolving the same choice twice is idempotent; a different choice, an unknown token, or a choice outside the options is an error. The pending-> resolved transition is a guarded UPDATE, so concurrent resolutions cannot both win. - `job_withdraw_decision(job_id, token)` cancels a pending request. - `job_decisions(job_id=None, status=None, limit=50)` lists them. - `job_wait(job_id, ["decision_resolved"])` waits for the answer like any other event; `decision_requested` also ranks as an attention event in `job_view`. - Expiry: the existing maintenance loop sweeps overdue pending decisions, marks them `expired` and emits `decision_expired` (edge-triggered, one event per transition); an expired decision can no longer be resolved. Schema v16 adds the `decisions` table plus (job_id, created_at) and (status, expires_at) indexes. The migration is additive and existing databases still get the usual pre-migration backup. Wiring: JobManager methods, `@mcp.tool()` wrappers, and the daemon POST/GET routes (`/jobs/<id>/decision`, `.../<token>/resolve`, `.../<token>/withdraw`, `GET /decisions`). No CLI subcommand (none of the neighbouring job tools have one). Scoped to local jobs. Remote jobs keep their own database/events and the remote protocol has no decision methods; a cross-machine decision needs protocol + feed work and is deliberately out of scope. Tests: tests/test_decisions.py (13) — lifecycle, idempotency/conflict, withdraw, expiry sweep + resolve-after-expiry, input validation, terminal/ unknown job rejection, token/job mismatch, listing/filters, wake delivery enqueued, job_wait integration, v15->v16 migration, and the HTTP routes end to end. Full suite 802 passed / 6 skipped; go vet/test/build clean.
The Go state reader pins `LatestSchemaVersion` and the cross-language fixture conformance test asserts the generated fixture matches it. Schema v16 (decisions table) moved Python forward, so the mirrored constant and the committed fixture must follow or the go CI jobs fail. No Go logic change: the monitor/state readers do not touch the new table.
Adversarial review of the decision primitive surfaced three HIGH and four MEDIUM findings. All fixed: HIGH - state change and its event were not atomic. Every mutation committed the decision row and then emitted in a separate transaction, so a crash in between left a resolved decision with no `decision_resolved` event (a retry could not repair it, because the idempotent path short-circuits). `_emit` now takes an optional `mutate` callback that runs inside the write transaction after the cap check and before the event insert; `BEGIN IMMEDIATE` still covers the event and its deliveries. request/resolve/withdraw/expire and their events are now one transaction. A `_DecisionNoOp` abort keeps the idempotent same-choice retry from writing a duplicate event. HIGH - the per-job structured-event cap silently disabled the protocol: at the cap, `_emit` returned `persisted=False` with no event and no wake, while the tool still reported success. Decision lifecycle events are now exempt from the cap like terminal events (decisions are human-paced and bounded), and ordinary telemetry is still capped. HIGH - the deadline was checked on a stale read before the CAS, so lock ordering decided whether an overdue approval was accepted. Deadline, status and choice validation now all run inside the write transaction that performs the transition. Withdraw also enforces the deadline (it previously did not, letting an overdue request escape its expiry event). MEDIUM - cleanup left decisions behind, so a pending request could outlive its job and be resolved into an orphan event: cleanup now deletes a job's decisions with its other state. MEDIUM - unbounded prompt/options could exceed `max_event_bytes`, which replaces the whole data object and drops `decision_id`/`choice`, breaking the wake and wait paths. Input is now bounded (10000 chars, 50 options, 200 chars each) and options dedupe linearly. MEDIUM - the daemon matched decision routes by suffix only, so `/jobs/<id>/resolve` raised IndexError (HTTP 500) and `/jobs/<id>/not-a-decision/<token>/resolve` resolved successfully. Routes now match exact segment shapes via `_decision_route`. MEDIUM - job eligibility was read outside the insert transaction; it now runs in the mutate, so a job finishing mid-request cannot get a decision. Tests (13 -> 19): fault injection proving state+event roll back together, event-cap exemption, concurrent-resolve single-winner, withdraw-overdue rejection, input bounds, cleanup removing decisions, decision_requested event data and delivery linkage, exactly-one expiry event across sweeps, and malformed HTTP paths. Full suite 808 passed / 6 skipped; compileall clean.
…emption
Verification review of the transactional fix confirmed 5 of 7 original defects
resolved and found two remaining HIGH issues plus a routing gap:
HIGH - character limits do not bound serialized bytes. `json.dumps` is
ASCII-only, so 10000 astral chars serialize to ~120KB and blow past the 64KB
event limit; `_emit` then replaces the whole data object with the truncation
marker, dropping decision_id/choice and breaking the wake and wait paths while
still committing the decision. `request_decision`/`resolve_decision`/
`withdraw_decision` now check the serialized lifecycle payload against
`max_event_bytes` before the transaction and reject an over-budget request;
`actor` is bounded too (it is only reachable from direct Python callers).
HIGH - the cap exemption keyed on event type, but job stdout can emit any type
via AGENT_EVENT, so a job printing
`AGENT_EVENT {"type":"decision_requested"}` bypassed the per-job cap
indefinitely (and fanned out wake deliveries). The exemption is now a
per-CALL flag (`exempt_from_cap`) set only by the authoritative decision
transitions, so forged telemetry is capped like everything else.
MEDIUM - the decision matcher fixed the reported resolve paths but
decision-looking paths still fell through to legacy suffix routes
(`/jobs/<id>/decision/<token>/pause` paused the job). Decision routes are now
matched before the suffix handlers, and any unmatched path in the decision
namespace returns 404 instead of reaching another operation.
Tests (19 -> 22): unicode/over-byte payload rejection (including a reduced
byte limit), forged decision telemetry still capped, actor bounds, and a
decision-namespace path not reaching another op (asserting the 404 message,
not just an error). Full suite 811 passed / 6 skipped; compileall clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a durable approval/decision primitive: jobs can ask a human a question and wait for the answer without holding a thread or being interrupted.
job_request_decision(job_id, prompt, options=["approve","deny"], timeout_seconds=None)records a pending decision and emitsdecision_requested, reusing the existing event -> wake-target -> delivery path so the owning thread is notified.job_resolve(job_id, token, choice)records the choice (idempotent for the same choice, guarded pending->resolved UPDATE so concurrent resolves can't both win).job_withdraw_decision(job_id, token)cancels a pending request.job_decisions(job_id=None, status=None, limit=50)lists them.job_wait(job_id, ["decision_resolved"])awaits the answer like any event;decision_requestedalso ranks as attention injob_view.decision_expired).A decision is its own state machine keyed by
decision_id, in a newdecisionstable —jobs.statusis untouched, so the job keeps running (or stays queued) while a human decides.Why
Top of the roadmap's Tier-1 product gaps (
roadmap-research-2026-09.md§C1): agents that need a human decision currently have no durable, addressable primitive —needs_inputwas only an attention label with no persistence or resolution semantics. This composes with work Vanth already has (wake targets, deliveries, events,job_wait) rather than importing a control plane.Migration
Schema v16, additive (
decisions+ two indexes), with the standard pre-migration backup. Existing databases upgrade in place.Scope
Local jobs only. Remote jobs keep their own DB/events and the remote protocol has no decision methods; cross-machine decisions need protocol + feed work and are deliberately deferred.
Testing
tests/test_decisions.py(13): lifecycle, idempotency/conflict, withdraw, expiry sweep + resolve-after-expiry, input validation, terminal/unknown job rejection, token/job mismatch, listing/filters, wake delivery enqueued,job_waitintegration, v15->v16 migration, and HTTP routes end-to-end.compileall,go vet ./...,go test ./...,go build ./...clean.README.mdtool table + example,docs/agent-tools.md,CHANGELOG.md.