Repository navigation
feat(workitems): workitem/v1 store, transition table and idempotency (#349) - #351
Conversation
…349) Step 1 of docs/work-context-contract.md: the internal/workitems package with the single exported transition map, sentinel errors, and a Postgres store for work items, lifecycle events, bounded intents, execution correlations and surface references. No REST surface yet. - Every mutation of one item runs in one transaction under pg_advisory_xact_lock(hashtext('workitem:' || id)); state-changing UPDATEs also carry an state_version guard, so a race resolves to exactly one effect and a 409 for the rest. - Idempotency-Key per (tenant, key) for create and per (item, key) for every other mutation; a key reused with a different request is refused. - external_key dedupes open work through a partial unique index. - Intent text is capped at 8 KiB and passes secretscan before insert; events never copy intent text or surface external ids. - The store records the actor it is handed; it never reads one from a body. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 4m 59s —— View job Review complete
Verdict: changes requested — 0 P1, 3 P2, 5 P3. The core design holds up. The advisory-lock + P2 — should fix before merge
P3 — non-blocking
Findings are posted as inline comments on the PR. I did not modify any files. Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1: Missing test coverage for secret scanning on work item creation
Finding 2: Missing test coverage for secret scanning in intent parameters
Finding 3: Missing test coverage for resuming an execution on an active work item
Finding 4: Missing test coverage for
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| WHERE tenant=$1 AND external_key=$2 AND state IN ('active','waiting')`, in.Tenant, in.ExternalKey)) | ||
| if err == nil { | ||
| out = w | ||
| return nil |
There was a problem hiding this comment.
P2 — the external_key dedupe path returns without recording the idempotency key, so (tenant, key) idempotency is not actually established for this request. Two consequences, both contradicting the PR's own contract table ("a replay returns the original entity"; "the same key with a different request hash returns ErrIdempotencyKeyReuse (never a silent replay)"):
- Silent replay of a different request. Item A already exists open with
external_key=E(opened by a surface that sent no key). Request{key=K, hash=H1, external_key=E}hits this branch and returns A; nothing is persisted for K. A later{key=K, hash=H2, external_key=E}hits the same branch and silently returns A again — noErrIdempotencyKeyReuse, even though the request body differs. - Same key, two entities. After A reaches a terminal state, a retry of
{key=K, hash=H1, external_key=E}no longer matches the dedupe predicate (state IN ('active','waiting')) and nocreate_idempotency_keyrow exists, so it creates a brand-new item B. The same key produced A and then B.
Both are reachable through the normal HTTP retry path once the handlers land. Suggest claiming the key on the dedupe hit — e.g. when the found row has create_idempotency_key IS NULL, UPDATE work_items SET create_idempotency_key=$k, create_request_hash=$h before returning; when it is non-null and differs, decide explicitly (return it anyway vs. reuse error) rather than leaving the key unrecorded.
There was a problem hiding this comment.
Fixed in bbb6cc3. Create keys now live in their own table, work_item_create_requests (tenant, idempotency_key) -> request_hash, work_item_id. They are recorded on the external_key dedupe path as well as on insert. Both consequences are covered by TestCreateRecordsItsKeyEvenWhenExternalKeyDedupes: the same key with a different hash gets ErrIdempotencyKeyReuse, and a retry after the deduped item went terminal returns that item, not a second one. Dropping the dedupe-path record makes the test fail.
| return invalid("resumed_from_execution_id %s is not an execution of %s", from, cur.ID) | ||
| } | ||
| now := s.now() | ||
| exec, err = insertExecution(ctx, tx, cur.ID, in.Engine, in.EngineRef, PhasePending, from, len(execs)+1, now) |
There was a problem hiding this comment.
P2 — this insert can violate UNIQUE (work_item_id, engine, engine_ref) and the error escapes untyped. insertExecution wraps it as workitems: insert execution: %w (line 518) with no sentinel, so the HTTP layer has nothing to match on and will render a 500 for what is a client-input problem.
Reproduction: resume an item with engine_ref equal to one already recorded for that item and no Idempotency-Key. Every other execution path is protected — AttachExecution looks the row up by (engine, engine_ref) first (line 583) and work_item_executions_one_open is pre-checked under the advisory lock — so Resume is the one place a 23505 reaches the caller raw. A plain client retry of a resume hits it.
Map pgerrcode.UniqueViolation here to ErrInvalid (or a ConflictError wrapping ErrExecutionActive), and add a test that resumes twice with the same engine_ref.
There was a problem hiding this comment.
Fixed in bbb6cc3. Under the lock, Resume checks the item's executions for the same (engine, engine_ref) and returns ErrInvalid (400 at the HTTP layer) before the insert. Test: TestResumeRefusesAnEngineRefAlreadyRecorded; removing the check makes it fail.
| out, err = getIntent(ctx, tx, cur.ID, result) | ||
| return err | ||
| } | ||
| if IsTerminal(cur.State) { |
There was a problem hiding this comment.
P2 — missing test coverage for three contract rules in this file, each a one-line mutation away from silently disappearing:
- This terminal guard (
AppendIntent, line 658). No test appends an intent to a completed/archived item; deleting these three lines keeps every test green. - The same guard in
AttachExecution(line 606) —TestExecutionsAttachCorrelateAndStayUniquenever reaches a terminal item. replayed's cross-operation branch (line 323,gotOp != op). The tests only exercise thehash != m.RequestHashhalf; reusing one key for a transition and then an intent is untested, so the operation column could be dropped from the comparison unnoticed.
The PR body lists a mutation-kill table for the other invariants; these three are the gaps in it.
There was a problem hiding this comment.
Fixed in bbb6cc3. TestTerminalItemsRefuseIntentsAndNewExecutions covers both terminal guards, and TestAKeyIsBoundToOneOperation covers the gotOp != op half of the comparison. All three mutants (each guard deleted, the operation dropped from the comparison) now fail a test.
| } | ||
| var out SurfaceRef | ||
| created := false | ||
| err := s.withTx(ctx, "workitem:"+in.WorkItemID, func(tx pgx.Tx) error { |
There was a problem hiding this comment.
P3 — LinkSurface is the only mutation that never calls replayed/remember, so Mutation.IdempotencyKey is silently ignored here. The PR body says idempotency is "per (item, key) for the rest", and SurfaceRefInput embeds Mutation so a handler will happily pass a key. The upsert makes it harmless in practice, but a key reused for a different link request will not raise ErrIdempotencyKeyReuse the way it does everywhere else. Either wire it up or note in the doc comment that the upsert is the idempotency mechanism here.
There was a problem hiding this comment.
Documented in bbb6cc3: the upsert on (item, surface, external_id) is LinkSurface's idempotency, so a key adds nothing there and is not recorded. The handler PR will not pass one for this route.
| return nil | ||
| } | ||
|
|
||
| func lockedItem(ctx context.Context, tx pgx.Tx, id string) (*WorkItem, error) { |
There was a problem hiding this comment.
P3 — lockedItem is a verbatim alias for getItem; it takes no row lock (no FOR UPDATE) and the only serialization is the advisory lock already taken in withTx. The name reads as a row-level lock at all five call sites, which is exactly the kind of thing a later edit could rely on. Either drop the wrapper and call getItem directly, or keep it and say in a comment that it only documents "read under the advisory lock already held".
There was a problem hiding this comment.
Removed in bbb6cc3; the call sites now use getItem directly, and the serialization comment on Store covers the advisory lock.
| if err := checkText("external_key", in.ExternalKey, MaxKeyBytes, false); err != nil { | ||
| return err | ||
| } | ||
| return scanSecrets("title", in.Title) |
There was a problem hiding this comment.
P3 — Create scans only Title. ExternalKey is caller-supplied free text up to 512 bytes and is typically a URL, which is a common place for an embedded token (https://user:ghp_...@host/..., a signed webhook URL with a query-string secret). The same applies to SurfaceRefInput.ExternalID/ActorExternalID (line 255), which are never scanned at all. Cheap to close: run scanSecrets over them too, consistent with the "no secrets reach durable storage" rule this function already enforces for the title.
There was a problem hiding this comment.
Fixed in bbb6cc3: external_key, external_id and actor_external_id all go through scanSecrets. TestSecretsAreRejectedInEveryFreeTextField covers the title, a URL with an embedded token, params and external_id.
| if m.Surface != "" && !surfacePattern.MatchString(m.Surface) { | ||
| return invalid("surface must match %s", surfacePattern) | ||
| } | ||
| if err := checkText("idempotency_key", m.IdempotencyKey, MaxKeyBytes, false); err != nil { |
There was a problem hiding this comment.
P3 — Actor, Surface and RequestID are the only caller-supplied strings with no length bound. IdempotencyKey gets MaxKeyBytes and Surface gets a 32-char regex, but Actor and RequestID go straight into work_item_events.actor_principal / request_id on every mutation with no ceiling. RequestID in particular is usually an echoed X-Request-Id header. A checkText("actor", m.Actor, MaxExternalIDBytes, true) plus the same for request_id would close it and let the existing strings.TrimSpace check fold in.
There was a problem hiding this comment.
Fixed in bbb6cc3: actor and request_id are bounded at 256 bytes through checkText, and the actor check absorbs the old TrimSpace check.
| // only; the HTTP layer decides). Viewer, when set, hides private items owned | ||
| // by anyone else. | ||
| type ListFilter struct { | ||
| Tenants []string |
There was a problem hiding this comment.
P3 — Tenants == nil meaning "every tenant" makes the zero value ListFilter{} a cross-tenant read. The doc comment is explicit that the HTTP layer decides, and the store layer is the right place to defer that, but it is a fail-open default on the one field that bounds tenancy: a handler that forgets to populate Tenants (or populates it from a slice that happens to be nil on an error path) leaks every tenant's open items rather than returning nothing. An explicit AllTenants bool alongside Tenants []string makes the admin case a deliberate statement and the forgotten case empty. Worth settling before the handlers land in the follow-up PR. Compare internal/alerts/store.go:30, which carries a comment about exactly this class of leak.
There was a problem hiding this comment.
Fixed in bbb6cc3. ListFilter has an explicit AllTenants bool; without it, an empty or nil Tenants returns no rows. The test asserts ListFilter{} returns nothing.
| } | ||
| var out *WorkItem | ||
| created := false | ||
| err := s.withTx(ctx, "workitem-create:"+in.Tenant, func(tx pgx.Tx) error { |
There was a problem hiding this comment.
P3 — the lock key is the tenant, so every Create in a tenant is serialized on one advisory lock for the duration of its transaction, including the two lookups and the insert. For a busy tenant with several surfaces opening work that is a single-writer bottleneck. internal/alerts/store.go:118 gets the same open-work dedupe from INSERT ... ON CONFLICT (tenant, fingerprint) WHERE fingerprint <> '' AND status NOT IN (...) DO UPDATE ... RETURNING with no lock at all, leaning on the partial unique index that this schema already declares at line 42. Worth considering, since the index is identical in shape; if the lock is kept, a line on why the ON CONFLICT form was not usable would help the next reader.
There was a problem hiding this comment.
Kept the lock and added a comment explaining why in bbb6cc3. The create decision is three steps (key lookup, external_key lookup, insert plus recording the key), and it has to record the idempotency key against whichever item it resolves to. INSERT ... ON CONFLICT on the partial index can dedupe, but it cannot also write the key row atomically for the conflict case. Per-tenant creates are low-rate human actions, so the serialization is acceptable here.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 3 P2, 5 P3.
The core of this holds up: the advisory-lock + state_version double guard, the transition table as the single enforcement point, the partial unique indexes, and the "events name an intent id, never its text" discipline are all enforced where the description says they are, and the concurrency test is a real test.
P2s:
Create(store.go:254) — theexternal_keydedupe path returns without recording the idempotency key, so the same key can silently replay a different request, and can produce two distinct items once the deduped item goes terminal.Resume(store.go:462) — aUNIQUE (work_item_id, engine, engine_ref)violation escapes untyped and becomes a 500 on an ordinary resume retry;AttachExecutionis protected by its pre-lookup,Resumeis not.- Test gaps (store.go:658) — the terminal guards in
AppendIntentandAttachExecution, andreplayed's cross-operation branch, have no coverage; each survives deletion.
P3s are non-blocking: lockedItem naming, secret scanning not covering external_key/external_id, unbounded Actor/RequestID, the fail-open Tenants == nil default, and the per-tenant create bottleneck vs. the alerts ON CONFLICT shape.
Note: I could not run go build/go vet/go test in this environment, so the review is static only; CI's validate job covers it.
…#349) Review round 1 on #351: - Create idempotency keys live in work_item_create_requests and are recorded on the external_key dedupe path too, so a reused key with a different request is refused and a retry after the item went terminal still returns it instead of opening a second one. - Resume refuses an (engine, engine_ref) already recorded on the item with ErrInvalid instead of surfacing a raw unique violation. - ListFilter fails closed: without AllTenants, an empty Tenants matches nothing. - external_key, external_id and actor_external_id pass secretscan; actor and request_id are bounded; the lockedItem alias is gone. - Tests for terminal intent/attach refusal, cross-operation key reuse, active-item resume, the owner filter and secret rejection per field. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
agy round on 4ad909e (all P3), addressed in bbb6cc3:
Copilot did not review: it reported the requester's quota limit. |
|
Claude finished @mashkovd's task in 4m 21s —— View job Review complete
Verdict: changes requested — 0 P1, 1 P2, 3 P3. Round 1 is cleanly addressed
No deadlock risk: each transaction takes exactly one advisory lock and P2 — should fix before merge
Same line, second consequence: a create asking for P3 — non-blocking
Findings are posted as inline comments. I did not modify any files. Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| } | ||
| if in.ExternalKey != "" { | ||
| w, err := scanItem(tx.QueryRow(ctx, `SELECT `+itemColumns+` FROM work_items | ||
| WHERE tenant=$1 AND external_key=$2 AND state IN ('active','waiting')`, in.Tenant, in.ExternalKey)) |
There was a problem hiding this comment.
P2 — the external_key dedupe returns the found item with no visibility check, so this is a read path onto another principal's visibility: private work item.
docs/work-context-contract.md ("Identity and authorization"): "visibility: private restricts non-admin access to the owning principal." List enforces exactly that with its Viewer clause (store.go:847), and TestListDefaultsToOpenAndHidesOthersPrivateItems asserts it. Create does not, and an external_key is a guessable public string — the contract's own example is a GitHub issue URL.
Failure: alice opens {visibility: private, external_key: https://github.com/mctlhq/mctl-api/issues/7, title: "rotate the prod signing key before the audit"}. bob, an ordinary member of the same tenant, posts a create with that same external_key. He gets back alice's item in full — id, title, owner_principal, state, state_version — plus a durable (tenant, key) row naming it. TestExternalKeyDedupesOnlyOpenWork already exercises this shape (bob dedupes onto alice's item), just with visibility: tenant, so nothing in the suite would notice if the private case regressed.
Second consequence on the same line: a create asking for visibility: private that dedupes onto a visibility: tenant item silently yields a tenant-visible item. The caller is told created=false and gets an item whose visibility is not what they asked for; nothing reports the downgrade.
The handler can re-check out.Visibility/out.OwnerPrincipal after the fact, but this package already decided visibility is partly its own job (ListFilter.Viewer), and the inconsistency is what will make the handler author in the follow-up PR assume Create is safe. Either scope the lookup (AND (visibility = 'tenant' OR owner_principal = $3), with a Viewer on CreateInput) or state in the doc comment that the caller must gate the returned item — and add a test for a private-item dedupe either way.
There was a problem hiding this comment.
Fixed in the latest push. The dedupe now applies only when the found item has the requested visibility and is not another principal's private item. Otherwise Create returns the new ErrExternalKeyInUse with no item, which covers both the private-item read and the silent private→tenant downgrade. TestExternalKeyNeverDedupesOntoWorkTheCallerCouldNotOpen covers the private item, a tenant request landing on a private item, the owner's own dedupe, and a private request landing on a tenant item. Both halves of the condition are mutation-checked. The handler PR (#354) maps the error to 409 external_key_in_use.
| args = append(args, v) | ||
| return fmt.Sprintf("$%d", len(args)) | ||
| } | ||
| if !f.AllTenants { |
There was a problem hiding this comment.
P3 — the fail-closed default is the right call, but AllTenants: true now silently discards a non-empty Tenants. ListFilter{AllTenants: true, Tenants: []string{"acme"}} returns every tenant, not acme.
That is the shape an admin handler will naturally build: AllTenants from user.IsAdmin(), Tenants from a ?tenant= query parameter. The two get set together and the narrowing one is the one that gets dropped — so the admin asking for one tenant gets all of them, which is the same fail-open the AllTenants flag was added to close, one level up. Either AND them (AllTenants only removes the requirement that Tenants be non-empty) or reject the combination with invalid.
There was a problem hiding this comment.
Fixed: named Tenants always narrow, and AllTenants only lifts the empty-means-nothing rule. Asserted with {AllTenants: true, Tenants: [other]}.
| return &ConflictError{Err: ErrExecutionActive, Current: cur} | ||
| } | ||
| } | ||
| if from == "" && len(execs) > 0 { |
There was a problem hiding this comment.
P3 — Resume on an active item that has never had an execution succeeds and records a resumed event with nothing to resume: from stays "", attempt is 1, resumed_from_execution_id is empty, and state_version is bumped for an active -> active move. The doc comment two lines up from the function says "Resume starts a new execution continuing a prior one", and ResumeInput.ResumedFromExecutionID says "Empty means the latest one, if the item has any" — so the zero-execution case is acknowledged but its meaning is never stated.
TestResumeOfAnActiveItemKeepsItActive covers active -> active with a prior execution; nothing covers the no-prior case. If it is meant to double as "start the first run", say so in the doc comment and test it; if not, invalid("%s has no execution to resume", cur.ID) when len(execs) == 0 keeps Resume and AttachExecution from overlapping.
There was a problem hiding this comment.
Fixed: Resume on an item with no executions is ErrInvalid ("attach the first one"), so it no longer overlaps with AttachExecution. Test: TestResumeNeedsAPriorExecution.
There was a problem hiding this comment.
Update: I first rejected this case, but that strands a waiting item (see the round-3 P2), so it is allowed again and now documented and tested as the start-first-run case.
| } | ||
|
|
||
| func checkText(field, value string, max int, required bool) error { | ||
| if required && strings.TrimSpace(value) == "" { |
There was a problem hiding this comment.
P3 — checkText trims for the presence check but the untrimmed value is what gets stored. Actor: " github:alice " passes, and owner_principal / created_by / actor_principal keep the spaces. From then on ListFilter{Owner: "github:alice"} and Viewer: "github:alice" both miss the item — the Viewer miss being the interesting one, since a private item whose owner was stored with stray whitespace becomes invisible to its own owner and stays that way. Tenant has the same shape against Tenants.
One value = strings.TrimSpace(value) returned from a checkText that hands the cleaned string back (or trimming in each validate) removes the class.
There was a problem hiding this comment.
Fixed for the values that are matched exactly: actor and tenant with leading or trailing whitespace are refused, not trimmed, so a caller learns about it (TestPaddedIdentitiesAreRefused). Free text such as intents keeps its whitespace; a trailing newline in an intent is still accepted.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.
Round 1 is cleanly addressed. work_item_create_requests makes the create key a first-class row and rememberCreate runs on both the insert and the external_key dedupe path, so a key now survives the deduped item going terminal; Resume refuses an already-recorded (engine, engine_ref) with ErrInvalid instead of leaking a 23505; the lockedItem alias is gone; external_key, external_id and actor_external_id pass secretscan; actor and request_id are bounded; ListFilter fails closed. The five new tests kill the mutants they were added for, including the cross-operation half of replayed and the terminal guards in AppendIntent/AttachExecution.
Remaining P2 (store.go:262): the external_key dedupe returns the found item with no visibility check, so a tenant member who supplies a guessable external_key gets another principal's visibility: private item back in full — id, title, owner, state. The contract says private restricts non-admin access to the owning principal, and List enforces that with Viewer; Create does not, and the existing dedupe test only covers visibility: tenant. Same line: a create requesting private that dedupes onto a tenant item silently yields a tenant-visible item with no signal.
P3s are non-blocking: AllTenants: true silently discards a non-empty Tenants (the admin-plus-tenant-filter case reopens the fail-open one level up); Resume on an item with no executions records a resumed event with nothing resumed and is untested; checkText trims for presence but stores untrimmed, so a padded Actor makes a private item invisible to its own owner.
Note: go build/go vet/go test are blocked in this environment, so this is static review only; CI's validate job runs the Postgres-backed tests.
…en (#349) Review round 2 on #351: - external_key dedupes only onto an item of the requested visibility that is not another principal's private item; otherwise ErrExternalKeyInUse, so a guessable key never reads someone's private work and a private request is never silently handed a tenant-visible item. - Resume refuses an item with no execution to continue. - Named Tenants always narrow a list; AllTenants only lifts the empty case. - actor and tenant with leading or trailing whitespace are refused. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 3m 44s —— View job Review complete
Verdict: changes requested — 0 P1, 1 P2, 2 P3. Round 2 is addressed
P2 — should fix before merge
So: a surface opens an item ( Round 2 offered "document it as start-the-first-run, or reject it"; rejecting is the option that closes the edge. Allowing it with P3 — non-blocking
Findings are posted as inline comments. I did not modify any files. Caveat: |
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
…349) Follows the store change on #351: a create whose external_key names work the caller could not open answers 409 without disclosing the item. Test covers both a private and a tenant request against another member's private item. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
| if from == "" && len(execs) > 0 { | ||
| from = execs[len(execs)-1].ID | ||
| } | ||
| if len(execs) == 0 { |
There was a problem hiding this comment.
P2 — this new guard makes waiting a one-way door for any item that never had an execution.
Resume is the only path from waiting back to active: Transitions lists waiting + resume -> active, but TransitionInput.validate refuses the action outright (inputs.go:189, "resume is its own operation"), and store.go:501 is the sole Next(cur.State, ActionResume) call site. So this early return is now the gate on the whole edge.
Failure: a surface opens a work item (active, no execution yet — nothing requires one), the item needs an answer before any run starts, so Transition(action=wait, waiting_reason=input) moves it to waiting. The human answers. Resume returns ErrInvalid ("has no execution to resume") because len(execs) == 0, and Transition(action=resume) returns ErrInvalid ("resume is its own operation"). The item can now only be completed, superseded or archived — it can never be worked on again. waiting_reason: input before a first run is exactly the shape the contract describes, and AttachExecution cannot rescue it either: it refuses nothing here, but attaching an execution does not change state, so the item stays waiting regardless.
Round 2 flagged this branch as under-specified and offered two options; rejecting is the one that closes the edge. Suggest instead allowing it when there is nothing to resume — from stays empty, attempt is 1, and the resumed event still records waiting -> active, which is the state change the caller actually needs — and testing that a waiting item with no executions comes back to active.
Two smaller things on the same block: with this check in place the len(execs) > 0 guard on line 479 is dead, and ResumeInput.ResumedFromExecutionID's doc comment still says "Empty means the latest one, if the item has any" (inputs.go:56-57), which no longer describes a reachable case.
There was a problem hiding this comment.
You're right, and it was my round-2 change that caused it. Reverted in the latest push: an item with no execution resumes normally (attempt 1, no resumed_from), so Resume stays the single way out of waiting. TestAWaitingItemWithNoExecutionCanStillResume covers create → wait → resume, and the ResumeInput doc states the case.
| if hash != in.RequestHash { | ||
| return ErrIdempotencyKeyReuse | ||
| } | ||
| w, err := getItem(ctx, tx, id) |
There was a problem hiding this comment.
P3 — the create-key replay path returns the item with no visibility or ownership check, so it is the one remaining way into the hole the external_key branch below just closed.
work_item_create_requests is keyed (tenant, idempotency_key), not (tenant, principal, key), and this branch returns whatever item that row names as soon as request_hash matches. A caller who reuses another member's key with a different body gets ErrIdempotencyKeyReuse (fine), but one whose body hashes the same gets the item itself — including a visibility: private item owned by someone else.
Reachability depends entirely on what the handler feeds RequestHash. If it digests only the request body, the body is largely guessable for the external_key flow (tenant, visibility, origin_surface and the external key are all things the second caller is already supplying; only the title has to match), and a client that derives its Idempotency-Key deterministically from the external key — a common pattern for webhook bridges — makes the key collide by construction rather than by chance. If the handler folds the authenticated principal into the hash, the whole thing is unreachable.
Cheapest fix that does not depend on the handler: add AND the owner/visibility predicate here too, or record owner_principal alongside the key and compare. Worth settling in this PR rather than in #354, since the hash contract is being defined there.
There was a problem hiding this comment.
Fixed by binding keys to the principal. work_item_create_requests and work_item_requests store the actor; the same key from another principal is ErrIdempotencyKeyReuse even with an identical body (TestAKeyBelongsToOnePrincipal, both comparisons mutation-checked). So the replay path can only return an item to the principal whose request created or deduped it.
|
|
||
| // Get returns one work item or ErrNotFound. | ||
| func (s *Store) Get(ctx context.Context, id string) (*WorkItem, error) { | ||
| return getItem(ctx, s.pool, id) |
There was a problem hiding this comment.
P3 — Get takes no viewer and no tenant, while List filters on both and Create now refuses to dedupe onto work the caller could not open. Same for Events (line 798) and Executions (line 591).
That was defensible when the package deferred all authorization to the HTTP layer, but two of the three read paths now enforce visibility themselves, so the remaining one reads as "already checked" rather than as "the caller must check". Work-item ids are opaque wi_<uuid> values, so this is not the guessable-key exposure the external_key branch closes — it only matters once an id leaks (a shared link, a superseded_by field, a surface transcript). Still, superseded_by is returned to any tenant member by List, and following it through Get is the obvious next call.
A Get(ctx, id, viewer string) — or a one-line "the caller gates this; List/Create do not" on all three doc comments — would keep the boundary legible for the handler author in #354.
There was a problem hiding this comment.
Documented on Get: the three reads apply no authorization, and the HTTP layer gates every result with canSeeWorkItem, the same rule List applies through Viewer/Tenants. The handler PR (#354) has the 404-not-403 tests.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.
Round 2 is addressed. The external_key dedupe now applies only when the found item has the requested visibility and is not another principal's private item, returning ErrExternalKeyInUse with no item otherwise — that closes both the private-item read and the silent private -> tenant downgrade, and TestExternalKeyNeverDedupesOntoWorkTheCallerCouldNotOpen covers all four shapes. AllTenants now only lifts the non-empty requirement instead of discarding named tenants, with the admin-narrowed case asserted. checkIdentity rejects padded actor/tenant outright, which is stronger than trimming and keeps the stored value equal to the matched value.
Remaining P2 (store.go:482): the new "no execution to resume" guard closes the waiting -> active edge for any item that never had an execution. Resume is the only path along that edge — TransitionInput.validate refuses ActionResume at inputs.go:189 — so an item that is created, moved to waiting with waiting_reason: input before any run starts, and then answered can never return to active. Attaching an execution does not help, since AttachExecution never changes state. Round 2 offered "document it or reject it"; rejecting is the option that strands the item. Allowing it with from empty and attempt 1 keeps Resume the single owner of that edge.
P3s are non-blocking: the create-key replay path (store.go:252) returns the named item with no visibility or ownership check, so whether the just-closed hole is fully closed depends on the handler folding the principal into RequestHash; and Get/Events/Executions still take no viewer while List and now Create enforce visibility, which reads as "already checked" for the handler author in #354.
Note: go build/go vet/go test are blocked in this environment, so this is static review only; CI's validate job runs the Postgres-backed tests.
…tems resume (#349) Review round 3 on #351: - Create and per-item idempotency records store the actor; the same key from another principal is ErrIdempotencyKeyReuse, never a replay of someone else's write. - Drop the "no execution to resume" guard: Resume is the only way out of waiting, so an item parked for input before any run must still resume (attempt 1, no resumed_from). Documented and tested. - Get/Events/Executions document that authorization is the caller's. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 4m 47s —— View job Review complete
Verdict: changes requested — 0 P1, 1 P2, 3 P3. Round 3 is addressed
P2 — should fix before merge
Neither table has been on
P3 — non-blocking
Findings are posted as inline comments. I did not modify any files. Caveat: |
…cipal (#349) Round on #354: - An inbound X-Request-Id is copied into history truncated to 256 bytes of valid UTF-8, the truncateUA precedent, so a long proxy trace id never turns a write into a 400. - Merges #351's store change binding idempotency keys to the principal; a second principal reusing a key gets 409 idempotency_key_reused (tested at the HTTP layer). Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| tenant TEXT NOT NULL, | ||
| idempotency_key TEXT NOT NULL, | ||
| request_hash TEXT NOT NULL, | ||
| actor TEXT NOT NULL, |
There was a problem hiding this comment.
P2 — actor is added to two tables that a previous commit on this branch already created, and CREATE TABLE IF NOT EXISTS will not add it.
work_item_create_requests and work_item_requests were introduced in bbb6cc3 without this column. Against any database that has already run bbb6cc3 or 443d494, Postgres skips both CREATE TABLE statements with a NOTICE, NewStore returns successfully, and then every statement naming actor fails at request time with 42703 undefined_column:
rememberCreate(line 313) ->workitems: create: record idempotency key: ... column "actor" ... does not exist, so every create carrying anIdempotency-Key500s.remember(line 368) andreplayed(line 350) -> the same for every keyed transition, resume, intent and execution attach.replayedselectsactor, so even the read half fails.
The repo's convention for exactly this is an additive ALTER after the CREATE, used in three other stores:
internal/alerts/store.go:59 ALTER TABLE alerts ADD COLUMN IF NOT EXISTS fingerprint TEXT NOT NULL DEFAULT '';
internal/agentregistry/store.go:208 ALTER TABLE agent_executions ADD COLUMN IF NOT EXISTS definition_version TEXT NOT NULL DEFAULT '';
internal/audit/postgres.go:44 ALTER TABLE audit_events ADD COLUMN IF NOT EXISTS client_ip TEXT;
Blast radius is dev and CI only, since neither table has ever been on main — a fresh deploy is fine. But the branch's own test DB is one of them: newStoreForTest wipes with DELETE FROM work_items, not DROP, so a local Postgres carried over from an earlier commit of this branch fails every keyed test rather than reporting a clean run. Two lines close it, and they are the lines a future column on these tables will need anyway:
ALTER TABLE work_item_create_requests ADD COLUMN IF NOT EXISTS actor TEXT NOT NULL DEFAULT '';
ALTER TABLE work_item_requests ADD COLUMN IF NOT EXISTS actor TEXT NOT NULL DEFAULT '';There was a problem hiding this comment.
Fixed in the latest push with the repo's additive-ALTER convention: ADD COLUMN IF NOT EXISTS actor on both tables. TestSchemaUpgradesAnEarlierRevision drops the columns, reopens the store and does a keyed create, and it fails without the ALTER.
| actor TEXT NOT NULL, | ||
| work_item_id TEXT NOT NULL REFERENCES work_items (id) ON DELETE CASCADE, | ||
| created_at TIMESTAMPTZ NOT NULL, | ||
| PRIMARY KEY (tenant, idempotency_key) |
There was a problem hiding this comment.
P3 — this is the only child table whose FK column is not the leading column of an index, so its ON DELETE CASCADE has nothing to scan with.
Every other table hanging off work_items gets the index for free: work_item_events PK (work_item_id, seq), work_item_surface_refs PK (work_item_id, surface, external_id), work_item_requests PK (work_item_id, idempotency_key), work_item_intents via work_item_intents_item, work_item_executions via UNIQUE (work_item_id, engine, engine_ref). This one is keyed (tenant, idempotency_key), so deleting a work item forces a sequential scan of work_item_create_requests per row deleted.
No production path deletes work items yet, but the contract names a retention sweeper and the tests already delete in bulk (DELETE FROM work_items in newStoreForTest). One line:
CREATE INDEX IF NOT EXISTS work_item_create_requests_item ON work_item_create_requests (work_item_id);There was a problem hiding this comment.
Added work_item_create_requests_item (work_item_id).
| // IdempotencyKey deduplicates the request: per tenant for Create, per | ||
| // work item for everything else, and always bound to Actor. RequestHash is the digest of the | ||
| // request that key was first used with; a replay with the same key and a | ||
| // different hash is ErrIdempotencyKeyReuse, never a silent replay. |
There was a problem hiding this comment.
P3 — the sentence that defines ErrIdempotencyKeyReuse was not updated with the actor binding. It still reads "a replay with the same key and a different hash", but replayed (store.go:358) and Create (store.go:256) now also raise it when the hash matches and only the actor differs — which is the whole point of this commit and the case a reader of this struct most needs told. Suggest "a replay with the same key but a different actor or request hash is ErrIdempotencyKeyReuse, never a silent replay".
Two nits on the same block: line 20 now runs well past the width the rest of this file wraps at, and the new clause ("and always bound to Actor") sits mid-sentence between the two halves of the key description rather than with the error it explains.
There was a problem hiding this comment.
Rewritten: the same key with a different actor or request hash is ErrIdempotencyKeyReuse, never a silent replay. The wrapping is fixed too.
| } | ||
|
|
||
| // Get returns one work item or ErrNotFound. Like Events and Executions it | ||
| // applies no authorization: every HTTP caller gates the result with |
There was a problem hiding this comment.
P3 — canSeeWorkItem does not exist anywhere in the repository. It is presumably the helper #354 will add, but as written this doc comment points a reader at a symbol they cannot find, and nothing here will notice if #354 names it something else.
Also, the note only lives on Get. Executions (line 594) and Events (line 801) are the other two unguarded read paths, and their own doc comments say nothing — a handler author lands on Events directly from the route table, not via Get. Suggest moving the rule to the package comment in types.go (which already states "Who may call it, and as whom, is decided by the HTTP layer"), or repeating one clause on each of the three, and describing the rule rather than naming an unwritten function: "applies no authorization; the caller checks tenant and visibility, as List does through Tenants and Viewer".
There was a problem hiding this comment.
Fixed: the note no longer names a handler symbol, and Get, Events and Executions each say they apply no authorization and that the caller checks tenant and visibility.
| // moves back to active; either way a resumed event is recorded. An item that | ||
| // never had an execution can still be resumed (a request parked for input | ||
| // before any run started): the new execution is attempt 1 with no | ||
| // resumed_from, and Resume stays the only way out of waiting. |
There was a problem hiding this comment.
P3 — the revert is the right call (it restores the only waiting -> active edge), but the doc justifies the zero-execution case only for a waiting item, while the code allows it for an active one too, and only the waiting case is tested.
Resume on an active item with no executions now succeeds: it inserts attempt 1 with an empty resumed_from, bumps state_version, and records an active -> active resumed event. AttachExecution on the same item also inserts attempt 1, but leaves state_version alone and records execution_attached. So there are two ways to start a first run on an active item, differing in event kind and in whether every concurrent holder of the old version gets a 409. A caller picking between them has nothing to go on.
TestAWaitingItemWithNoExecutionCanStillResume covers the waiting path only. Either say in this comment which of the two is intended for "start the first run on an active item", or keep the guard but scope it to cur.State == StateActive so the waiting edge stays open. Worth settling before #354 wires the routes.
Smaller point on the same test: it asserts the returned item and execution but not the event, so the waiting -> active record that the whole revert exists to produce is unchecked. s.Events plus a FromState == StateWaiting && ToState == StateActive assertion, as TestResumeOfAnActiveItemKeepsItActive already does for its own edge, would close that.
There was a problem hiding this comment.
Settled by scoping the guard to active. With no execution, Resume from waiting is allowed (it is the only way out); from active it is ErrInvalid, and AttachExecution starts the first run. The test now also asserts the waiting→active resumed event and the active refusal. Removing the scoped guard makes it fail.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.
Round 3 is addressed, and the two changes are the right ones. Removing the len(execs) == 0 guard restores Resume as the only waiting -> active edge, so an item parked for input before any run started is no longer stranded; TestAWaitingItemWithNoExecutionCanStillResume covers create -> wait -> resume. Binding the key to Actor in both work_item_create_requests and work_item_requests closes the create-key replay path that round 3 flagged: a second principal reusing a key now gets ErrIdempotencyKeyReuse rather than the item it names, and it no longer depends on the handler folding the principal into RequestHash. TestAKeyBelongsToOnePrincipal kills both mutants (drop either actor comparison and a half fails).
Remaining P2 (store.go:103): the actor column is added to two tables that bbb6cc3 already created, with no ALTER TABLE ... ADD COLUMN IF NOT EXISTS. Against a database that ran an earlier commit of this branch, CREATE TABLE IF NOT EXISTS is a no-op, NewStore still succeeds, and then every statement naming actor fails at request time with 42703 — rememberCreate, remember and replayed, so every keyed create, transition, resume, intent and attach. Neither table has been on main, so a fresh deploy is unaffected; the exposure is dev and CI, including this branch's own test DB, since newStoreForTest wipes with DELETE, not DROP. alerts, agentregistry and audit all pair a CREATE TABLE IF NOT EXISTS schema with additive ALTERs for exactly this; two lines here match that.
P3s are non-blocking: Get's new doc names canSeeWorkItem, which exists nowhere in the repo, and Events/Executions carry no note of their own; Mutation.IdempotencyKey's doc still defines the reuse error as "same key and a different hash" and omits the actor half this commit added; work_item_create_requests is the one child table whose FK column leads no index, so its ON DELETE CASCADE sequentially scans; and Resume on an active item with no executions is allowed but undocumented and untested, overlapping AttachExecution with a different event kind and different version semantics.
Note: go build/go vet/go test are blocked in this environment, so this is static review only; CI's validate job runs the Postgres-backed tests.
…e to waiting (#349) Review round 4 on #351: - ALTER TABLE ... ADD COLUMN IF NOT EXISTS actor for both key tables, so a database created by an earlier revision upgrades instead of failing every keyed write with 42703; plus an index on work_item_create_requests (work_item_id) for the cascade. Tested by dropping the columns and reopening the store. - Resume with no prior execution is allowed only from waiting (the sole way out of it); an active item starts its first run with AttachExecution. - Docs: the actor half of ErrIdempotencyKeyReuse; no-authorization notes on Get, Events and Executions without naming a handler symbol. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 5m 8s —— View job Review complete
Verdict: approved — 0 P1, 0 P2, 4 P3. Round 4 is addressed
Re-checked the invariants that carry this package: each mutation takes exactly one advisory lock and P3 — non-blocking
Findings are posted as inline comments. I did not modify any files. Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| } | ||
| if in.ExternalKey != "" { | ||
| w, err := scanItem(tx.QueryRow(ctx, `SELECT `+itemColumns+` FROM work_items | ||
| WHERE tenant=$1 AND external_key=$2 AND state IN ('active','waiting')`, in.Tenant, in.ExternalKey)) |
There was a problem hiding this comment.
P3 — the tenant advisory lock does not cover the item this lookup returns, so the dedupe can hand back an item that is already terminal.
Create serializes on workitem-create:<tenant>; Transition serializes on workitem:<id>. They are different keys, so nothing stops a Transition from completing item A between this SELECT (READ COMMITTED, statement snapshot) and the commit at line 306. The caller then gets A back with created=false and a stale state: active / state_version: 1, and rememberCreate durably binds the key to a work item that is closed. The surface that asked to open work believes it has open work; its next PATCH gets a 409 against a version it was just told, and its next intent gets ErrInvalidTransition.
The window is narrow and everything downstream still fails cleanly, so this is not a merge blocker. But the comment at line 246 ("the key lookup, the external_key lookup and the insert ... must be one decision") claims more than the lock delivers, and one word closes the gap: ... AND state IN ('active','waiting') FOR UPDATE. Transition reaches the row through UPDATE work_items ... WHERE id=$1, so a row lock here actually serializes the two. No deadlock risk: each transaction takes at most one advisory lock and acquires the row lock last.
| if _, err := s.pool.Exec(ctx, `ALTER TABLE work_item_requests DROP COLUMN actor`); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, err := s.pool.Exec(ctx, `ALTER TABLE work_item_create_requests DROP COLUMN actor`); err != nil { |
There was a problem hiding this comment.
P3 — the upgrade test drops columns on the shared test database and relies on the code under test to put them back, and it never exercises the case the backfill actually changes.
Two points:
-
No rows before the upgrade. The table is empty when
actoris dropped, so nothing checks whatDEFAULT ''does to rows written by the earlier revision. Those rows come back withactor = '', andreplayed/Createcompareactor != m.Actor— so a genuine retry of a request recorded before the upgrade now getsErrIdempotencyKeyReuse(a 409) instead of the replay it is entitled to. Fail-closed and branch-only, but it is the one behavioural consequence of this commit and it is neither tested nor written down. Inserting one row with the column dropped, then re-runningNewStore, would pin it either way. -
No restore on failure. If
NewStorereturns an error the testt.Fatals withactorstill dropped from both tables, and every later test in the package fails against a half-dropped schema rather than just this one — exactly the confusing state the ALTER exists to prevent. At.Cleanupthat re-runs the twoADD COLUMN IF NOT EXISTSstatements keeps the blast radius at one test.
Smaller: the test drops the columns but not work_item_create_requests_item, so the new index line is never exercised.
There was a problem hiding this comment.
|
|
||
| // Events returns a work item's lifecycle history in order. No | ||
| // authorization: the caller checks tenant and visibility. | ||
| func (s *Store) Events(ctx context.Context, itemID string) ([]Event, error) { |
There was a problem hiding this comment.
P3 — Events and Executions return an empty slice for a work item id that does not exist, so nothing distinguishes "no history" from "no such item".
Get returns ErrNotFound; these two return []Event{} / []Execution{} and a nil error for wi_does-not-exist. With the doc note that the caller applies authorization, a #354 handler that wires GET /work-items/{id}/events straight to this will answer 200 [] for an unknown id and for another tenant’s id alike — which is the right shape for the second case but the wrong status for the first, and the handler has no signal to tell them apart without a separate Get.
Either return ErrNotFound when the item is absent (an existence check in the same query), or say in these two doc comments that the caller must Get first, since that is the gate that makes the 404 correct.
| return &ConflictError{Err: err, Current: cur} | ||
| } | ||
| if in.Action == ActionSupersede { | ||
| next, err := getItem(ctx, tx, in.SupersededBy) |
There was a problem hiding this comment.
P3 — supersede checks that the target exists and shares the tenant, but not that it is still open, so superseded_by can point at archived or completed work.
Transition(A, supersede, superseded_by: B) succeeds when B is itself archived. A is then terminal, naming a dead successor, and List hands superseded_by to any tenant member as the place to continue — a surface following the pointer lands on work nobody can act on. The contract table (docs/work-context-contract.md:167-168) says only "sets superseded_by", so this is a judgement call rather than a rule violation; a chain also has no cycle guard (A -> B, then B -> A both succeed).
If the intent is that a successor must be live, || IsTerminal(next.State) folds into the existing condition at line 406 and the message already reads correctly. If not, a line here saying the successor may be terminal would stop the next reader from assuming otherwise.
There was a problem hiding this comment.
No P1/P2 findings (4 P3). Good to merge.
Round 4 is addressed, and both fixes are right. The two ALTER TABLE ... ADD COLUMN IF NOT EXISTS actor lines plus work_item_create_requests_item match the alerts / agentregistry / audit convention, and TestSchemaUpgradesAnEarlierRevision pins the no-op CREATE TABLE IF NOT EXISTS case by dropping the columns and re-running NewStore. Scoping the "no execution to resume" refusal to cur.State == StateActive keeps Resume the only waiting -> active edge while sending a first run on an active item to AttachExecution, which removes the overlap round 4 flagged; the waiting case now also asserts the waiting -> active resumed event. The Get doc no longer names a symbol that does not exist, Executions and Events carry their own note, and Mutation.IdempotencyKey states the actor half of the reuse rule.
Re-checked the invariants that matter and they hold: every mutation takes exactly one advisory lock and withTx never nests, so there is no deadlock; state-changing UPDATEs still carry state_version = expected as a second guard; execution attempt is len(execs)+1 under that lock, so neither UNIQUE (work_item_id, attempt) nor UNIQUE (work_item_id, engine, engine_ref) can surface a raw 23505; Create cannot violate the partial external_key index because lookup and insert are serialized per tenant; List builds SQL with positional placeholders behind validState; the INSERT ... VALUES column order in Create matches itemColumns.
P3s, none blocking: the external_key lookup takes no row lock, so a concurrent Transition can make the deduped item terminal between the SELECT and the commit (FOR UPDATE closes it); the upgrade test drops columns on the shared database with no t.Cleanup restore and leaves the pre-existing-row case (actor backfilled to the empty string turns a legitimate retry into ErrIdempotencyKeyReuse) untested; Events and Executions return an empty slice rather than ErrNotFound for an unknown id; and supersede does not require the successor to be non-terminal.
Note: go build / go vet / go test are blocked in this environment, so this is static review only; CI's validate job runs the 27 Postgres-backed tests.
…ede edges Review P3s from #351: - FOR UPDATE on the external_key lookup, so a concurrent transition cannot make the matched item terminal before the create commits. - Events and Executions say an unknown id answers an empty list. - Supersede says the successor may itself be terminal. - The schema-upgrade test restores the dropped columns on cleanup. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
Part of #349. Step 1 of
docs/work-context-contract.md("Implementation status" §1): the store only. The REST handlers, wiring, OpenAPI and README follow in the next PR, and #349 closes there.What
internal/workitems:types.go:workitem/v1resource types;Transitions, the single exported lifecycle map (active ⇄ waiting, → completed/superseded/archived; nothing leaves a terminal state;resumedis only an event kind); sentinel errors;ConflictError, which carries the current item so a 409 can report the state and version.inputs.go: input structs and validation. The contract's bounds are enforced here: intent text ≤ 8 KiB, params must be a JSON object, and title, text and params passsecretscan.Scanbefore insert.store.go: Postgres store:Create,Get,List,Transition,Resume,AttachExecution,AppendIntent,LinkSurface,Events,Executions.Contract points and where they are enforced
pg_advisory_xact_lock(hashtext('workitem:'‖id)); state-changingUPDATEs also requirestate_version = expected, a second layerexpected_state_versionis required on transition and resume; a mismatch returnsErrVersionConflict+ current item, and nothing changesIdempotency-Key(tenant, key)for create, per(item, key)for the rest (work_item_requests); a replay returns the original entity; the same key with a different request hash returnsErrIdempotencyKeyReuse(never a silent replay)external_keyWHERE state IN ('active','waiting'), the shape ofalerts_tenant_fingerprint_openwork_item_executions_one_openattempt = n+1andresumed_from_execution_id(default: the latest; must belong to this item)surface_linkednever copies the external idsMutation.Actoras handed to it by the HTTP layer; there is no body path to itOut of this PR (per the issue scope): context snapshots, the approvals projection,
surface-identities(#350), the retention sweeper.Tests
internal/workitems/store_test.go: 15 tests. They run against Postgres whenTEST_DATABASE_URLis set (CI'svalidatejob has it), following thealerts/domains/humaninputconvention. Local run:go test -race -count=3against postgres:17, all pass;go test -p 1 ./...all pass;golangci-lint0 issues.Mutation proof (each mutant is killed):
TestTransitionsAndVersionConflicts,TestConcurrentTransitionsResolveToExactlyOneTestTransitionReplayAppliesOnceTestIntentsAreBoundedAndNeverCarrySecretsTestResumeContinuesAPriorExecutionTestResumeContinuesAPriorExecutionList→TestListDefaultsToOpenAndHidesOthersPrivateItemsexternal_keyindex non-partial (fresh DB) →TestExternalKeyDedupesOnlyOpenWorkstate_versionguard → the concurrent test (16 racers behind a barrier, 5 rounds) reports 10 winners. Removing either one alone is caught by the other, by design.Consumer note
The mctl-agents client mirror (
orchestrator/work_context/contract.py) was written before this contract was readable. It expectswork_item_id, a different state vocabulary andexecutions[].sequence. It says it will adopt #227's spelling ("a one-line correction here once #227 is readable"), and it fails closed toWORK_ITEM_UNKNOWNon anything unrecognised, behindWORK_CONTEXT_ROLLOUT_MODE=off. This PR follows the accepted contract; aligning that mirror is tracked separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G