Repository navigation
feat(work-items): workitem/v1 REST routes, wiring and OpenAPI (#349) - #354
Conversation
Steps 2 and 4 of docs/work-context-contract.md over the internal/workitems store: open, list, read, transition, intents, executions, resume, surface-refs and events under /api/v1/work-items; mutations share the 20/min write budget. - The acting principal comes only from authentication, rendered as github:<login>, service:<id> or oidc:<id>. A body naming an actor (actor, owner_principal, created_by, on_behalf_of, ...) is refused with 400 actor_not_accepted; actor_external_id on a surface reference is correlation only. Delegation waits for mctl-api#350. - Authorization is re-evaluated per request: admin, or tenant access plus visibility; an item the caller may not see is 404. - Typed error codes for 404/409/400; 409s carry the current state and state_version. - WORK_ITEMS_DB_URL (falls back to AUDIT_DB_URL) and WORK_ITEMS_DISABLED; nil store answers 503. - OpenAPI paths and schemas, README, .env.example, and the contract's implementation status. Also quotes a description in openapi.yaml that made the served spec unparseable, with a test that parses it. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 8m 30s —— View job Review posted — changes requested (0 P1, 2 P2, 3 P3)
Five inline comments are on the diff; the review verdict is submitted against What holds upThe authorization model matches P2 — should fix before merge1. 2. Idempotency hash is not bound to the principal ( P3
The unrelated |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit FindingsFinding 1
Finding 2
Finding 3
|
…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
|
Claude finished @mashkovd's task in 7m 12s —— View job Review complete
Reviewed What holds upThe identity model is the strong part of this change. P2
P3
I could not run |
agy P3s on #354: the executions list (empty array, ordering) and superseded_by pointing at a missing or invisible item. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
agy round on f62f3a4 (all P3), addressed in aa50f0f:
Copilot did not review: it reported the requester's quota limit. |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
|
Claude finished @mashkovd's task in 5m 22s —— View job Review posted
Verdict: changes requested — 0 P1, 2 P2, 4 P3. Findings are posted as inline comments on the diff. The identity model is the strong part of this PR: P21.
2. Unexpected store errors are swallowed — The P3
Not reproducedThe earlier Antigravity finding about I could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| m := workitems.Mutation{Actor: principalOf(user), Surface: surface, IdempotencyKey: key} | ||
| if meta, ok := ClientMetaFromContext(r.Context()); ok { | ||
| m.RequestID = meta.RequestID |
There was a problem hiding this comment.
P2 — a client-supplied X-Request-Id header longer than 256 bytes makes every mutating work-item route return 400.
ClientMeta.RequestID comes from middleware.GetReqID, and chi's middleware.RequestID (internal/api/router.go:203) uses the inbound X-Request-Id header verbatim when present, only generating an id when the header is absent. It is unbounded and unvalidated. Here it is copied straight into Mutation.RequestID, and Mutation.validate() caps it:
// internal/workitems/inputs.go:127
if err := checkText("request_id", m.RequestID, MaxExternalIDBytes /* 256 */, false); err != nil {so the store returns ErrInvalid, and writeWorkItemError maps that to 400 invalid_request with the message request_id exceeds 256 bytes — blaming a field the caller never put in the body. checkText also rejects non-UTF-8, which a raw header value can be.
All six mutating routes are affected, since CreateInput / TransitionInput / IntentInput / ExecutionInput / ResumeInput / SurfaceRefInput all call Mutation.validate() first. Reads are unaffected.
Repro: POST /api/v1/work-items with X-Request-Id set to 300 as and an otherwise valid body returns 400 instead of 201. An ingress or client library that injects a long opaque trace id breaks work-item writes for everything behind it.
This package already has the right precedent for caller-controlled metadata of exactly this shape — truncateUA / maxUserAgentLen in internal/api/clientmeta.go. Truncate (or drop) the request id here rather than letting it fail the write:
if meta, ok := ClientMetaFromContext(r.Context()); ok {
m.RequestID = truncateRequestID(meta.RequestID) // cap at workitems.MaxExternalIDBytes
}Worth a test that a 300-byte X-Request-Id still yields 201.
There was a problem hiding this comment.
Fixed in the latest push. truncateRequestID (following the truncateUA precedent) keeps the request id to 256 bytes of valid UTF-8 before it reaches the store. TestWorkItems_LongRequestIDNeverFailsAWrite sends a 300-byte id with an invalid byte and gets 201; without the truncation it gets 400.
| writeError(w, http.StatusInternalServerError, "hash request") | ||
| return workitems.Mutation{}, false | ||
| } | ||
| sum := sha256.Sum256(append([]byte(op+"\x00"+itemID+"\x00"), canonical...)) |
There was a problem hiding this comment.
P2 — the idempotency request hash does not bind a key to the principal that used it, so one caller can replay another caller's key and be told their write succeeded.
The preimage is op + item/tenant + the decoded body. Mutation.Actor is deliberately kept out of the body (that is what forbiddenIdentityFields enforces), so it never reaches this hash. The key namespaces in the store are (tenant, idempotency_key) for create and (work_item_id, idempotency_key) for everything else — both cross-principal.
Consequence, with alice and bob both members of the tenant:
- alice:
PATCH /work-items/{id}{"action":"archive","expected_state_version":1}withIdempotency-Key: k-1→ archived, event attributed togithub:alice. - bob: byte-identical request with
Idempotency-Key: k-1→replayed()matches on op+hash and returns200 OKwith the item. Nothing is recorded for bob, and bob is told the transition is his.
Same on create: bob gets 200 and alice's work item back; and if that item is visibility: private, the handler answers 409 external_key_in_use (line 308) even though no external_key was involved — a misleading code for what is really a key collision.
This is the accountability property this file's header states ("a caller that believes it acted as someone else must find out it did not"), and the store's own comment says a reused key is ErrIdempotencyKeyReuse, "never a silent replay". Folding the principal into the preimage restores that in one line — a genuine retry from another pod of the same service still hashes identically, while a different principal now gets the typed 409:
sum := sha256.Sum256(append([]byte(op+"\x00"+itemID+"\x00"+m.Actor+"\x00"), canonical...))(Needs the m := workitems.Mutation{...} assignment moved above this block.) If the cross-principal sharing is intentional, it deserves a comment here and a test pinning it — as written it reads as an oversight.
There was a problem hiding this comment.
Fixed at the store level, so the binding does not depend on the handler (see #351 964f2a8). Both key tables record the actor, and a different principal with the same key and body gets ErrIdempotencyKeyReuse. At the HTTP layer, TestWorkItems_AnotherPrincipalCannotReplayYourKey returns 409 idempotency_key_reused for bob; the misleading external_key_in_use path can no longer be reached that way.
| writeErrorCode(w, http.StatusBadRequest, wiCodeInvalid, "limit must be a positive integer", nil) | ||
| return | ||
| } | ||
| f.Limit = n |
There was a problem hiding this comment.
P3 — limit above the store maximum is silently clamped, contradicting the OpenAPI contract added in this PR.
The handler rejects non-numeric and <= 0, but passes anything larger straight through; Store.List then clamps to maxListLimit (500) without telling the caller. Meanwhile openapi.yaml declares limit: {minimum: 1, maximum: 500}, which a generated client reads as "values above 500 are rejected".
A caller asking for limit=1000 and receiving exactly 500 items has no way to distinguish "that is all of them" from "you were truncated" — the usual len(items) < limit ⇒ last page heuristic then silently drops work. Rejecting > workitems.MaxListLimit with 400 invalid_request (the same shape as the <= 0 case just above) matches the spec and makes truncation impossible to miss. maxListLimit would need exporting.
No test covers the limit parameter at all today — neither the <= 0 rejection nor the clamp.
| if !decodeWorkItemBody(w, r, &body) { | ||
| return | ||
| } | ||
| if body.Tenant == "" || !user.HasTenantAccess(body.Tenant) { |
There was a problem hiding this comment.
P3 — a missing/empty tenant answers 403 tenant_forbidden, not 400 invalid_request.
user.HasTenantAccess("") is false for a non-admin and true for an admin, so the body.Tenant == "" arm is what actually catches it — and it produces 403 with the message no access to tenant "". The OpenAPI added in this PR marks tenant as required, so a generated client expects 400 here; and "no access to tenant """ reads as an authorization decision about a tenant the caller never named.
Splitting the two cases keeps the typed codes meaningful:
if body.Tenant == "" {
writeErrorCode(w, http.StatusBadRequest, wiCodeInvalid, "tenant is required", nil)
return
}
if !user.HasTenantAccess(body.Tenant) { ... 403 ... }
| writeWorkItemError(w, err) | ||
| return | ||
| } | ||
| h.auditWorkItem(r, user, "work_item.execution", item.ID, item.Tenant, |
There was a problem hiding this comment.
P3 — this is the one handler that audits unconditionally; a pure idempotent replay writes a duplicate audit entry.
Every other mutating handler here gates on created (CreateWorkItem:311, AppendWorkItemIntent:451, ResumeWorkItem:553, LinkWorkItemSurface:605). AttachExecution returns created == false in two quite different situations: an existing execution whose phase was just advanced (a real change, worth auditing), and a replayed() hit on the same Idempotency-Key where nothing at all happened. As written, a client retrying an attach N times produces N work_item.execution audit rows for one event.
created alone is not the right gate either, since the phase-advance case should still be audited. Either have the handler compare the returned exec.Phase against the requested one, or let the store report "nothing changed" alongside created.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.
The authorization model, the forged-actor rejection and the 404-not-403 visibility handling all hold up under review, and the tests genuinely kill the mutants the description lists. Two things should be settled before merge:
- P2
mutationForcopies the caller-controlledX-Request-Idheader intoMutation.RequestIDunbounded; the store capsrequest_idat 256 bytes, so a long header turns every one of the six mutating routes into400 invalid_request.truncateUAinclientmeta.gois the existing precedent. - P2 The idempotency request hash omits
Mutation.Actor, and both key namespaces are cross-principal, so a second principal replaying another's key gets200 OKfor a write recorded against someone else — the opposite of the accountability rule this file opens with.
P3s: limit > 500 silently clamped against a spec that declares maximum: 500; empty tenant answers 403 rather than 400; AttachWorkItemExecution audits on a pure replay where the other five handlers gate on created.
The unrelated openapi.yaml quoting fix plus embed_test.go is a good catch and worth landing regardless.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit FindingsFinding 1
Finding 2
Finding 3
|
| writeError(w, http.StatusInternalServerError, "hash request") | ||
| return workitems.Mutation{}, false | ||
| } | ||
| sum := sha256.Sum256(append([]byte(op+"\x00"+itemID+"\x00"), canonical...)) |
There was a problem hiding this comment.
P2 — the create idempotency hash omits the acting principal, so two principals in the same tenant collide on one key.
work_item_create_requests is keyed (tenant, idempotency_key) (internal/workitems/store.go:253), and the only tiebreaker is request_hash. That hash is built here from op + tenant + json(createWorkItemBody) — and createWorkItemBody has no actor field, because identity deliberately never reaches the body. So the actor never enters the preimage.
Failure scenario: alice POST /api/v1/work-items with Idempotency-Key: ci-run-1 and {"tenant":"acme","title":"ship it"} → 201, item owned by github:alice. Bob, also in acme, sends the byte-identical body with the same key. The store finds the recorded row, hash == in.RequestHash, and returns alice's item. Bob gets 200 with owner_principal: github:alice and no item of his own was ever created — a silent no-op, not a typed error. If alice's item was visibility: private, CreateWorkItem falls into the !canSeeWorkItem branch at line 307 and answers 409 external_key_in_use / "external_key already names open work you cannot see" on a request that carried no external_key at all.
Keys like ci-run-1, issue-42 or a date are exactly the shapes a shared automation surface generates, so this is not only a UUID-collision hypothetical.
Handler-local fix: put the principal in the preimage, so a second caller reusing the key gets the honest 409 idempotency_key_reused instead of someone else's item:
sum := sha256.Sum256(append([]byte(op+"\x00"+itemID+"\x00"+principalOf(user)+"\x00"), canonical...))A same-caller retry still hashes identically, so replay behaviour is unchanged. Worth a test that two users in one tenant sending the same key+body do not share an item.
| case errors.Is(err, workitems.ErrInvalid): | ||
| writeErrorCode(w, http.StatusBadRequest, wiCodeInvalid, err.Error(), nil) | ||
| default: | ||
| writeError(w, http.StatusInternalServerError, "work-items store error") |
There was a problem hiding this comment.
P2 (error handling): the default branch discards the store error entirely. Any failure this switch does not recognise -- pool exhaustion, a context deadline, a scan or column error, an unmapped pgx error -- becomes a bare 500 {"error":"work-items store error"} with nothing written to the log, on all ten work-item routes.
Elsewhere in this package an unexpected store error is logged before the response (handlers_lifecycle.go:199, handlers_usage.go:245, handlers_human_input.go:336), and CLAUDE.md asks for structured slog. As written a work-items outage is invisible in the API logs; the only signal is a 500 rate.
Suggested: slog.Error("work-items store error", "error", err) before the writeError call, so the discarded context reaches the operator.
| list := e.do(bob, "GET", "/api/v1/work-items?tenant="+e.tenant, nil) | ||
| if items, _ := list.body["items"].([]any); len(items) != 1 { | ||
| t.Errorf("bob lists %s", list.raw) | ||
| } |
There was a problem hiding this comment.
P2 (test coverage): every list call in this file passes ?tenant=, so the tenancy default in ListWorkItems (handlers_work_items.go:349-360) is never exercised -- neither case user.IsAdmin(): f.AllTenants = true nor default: f.Tenants = user.Groups.
That switch is the cross-tenant boundary for the list route. Swapping the default arm to f.AllTenants = true would hand every tenant open work to any authenticated caller, and the whole suite still passes. It is the one authorization branch in this PR without a named test, while canSeeWorkItem, the forbidden-field scan and the external_key oracle each have one.
Two assertions close it: GET /api/v1/work-items with no tenant query, as a member of a single tenant, returns only that tenant items (open one in a second tenant the caller is not in); and the same call as an admin sees both.
| case errors.Is(err, workitems.ErrInvalid): | ||
| writeErrorCode(w, http.StatusBadRequest, wiCodeInvalid, err.Error(), nil) | ||
| default: | ||
| writeError(w, http.StatusInternalServerError, "work-items store error") |
There was a problem hiding this comment.
P2 — the default branch swallows every unexpected store error: no slog line, no error text, nothing. handlers_work_items.go does not import log/slog at all, so a dropped connection, a pgx scan failure, a schema drift or a context deadline inside internal/workitems produces a bare {"error":"work-items store error"} and leaves zero trace on the server side.
Failure scenario: Postgres restarts mid-request. Store.Transition returns workitems: transition: %w wrapping a pgx connection error. It matches none of the sentinels above, so the caller sees 500 "work-items store error" and the operator has an untraceable 500 on the route the mctl-agents client polls — no way to tell a connection reset from a scan bug without adding logging and redeploying.
Every other store-backed handler in this package logs the unexpected case (handlers_agent_platform.go:180, handlers_human_input.go:336, handlers_human_input_response.go:202), which is also what CLAUDE.md's "structured logging with slog" asks for. Suggest:
default:
slog.Error("work_items store error", "error", err)
writeError(w, http.StatusInternalServerError, "work-items store error")Passing the request through would let you add request_id/path too, but the bare line already closes the gap.
| writeError(w, http.StatusInternalServerError, "hash request") | ||
| return workitems.Mutation{}, false | ||
| } | ||
| sum := sha256.Sum256(append([]byte(op+"\x00"+itemID+"\x00"), canonical...)) |
There was a problem hiding this comment.
P3: the request digest is op + itemID + json(body), which leaves two seams.
-
It does not include the acting principal. Two callers in the same tenant who send byte-identical creates under the same
Idempotency-Keyhash the same, so the second gets 200 with the first caller work item --owner_principalandcreated_bynaming someone else -- instead of opening its own. The store scopes the create key per tenant, not per actor, so the HTTP layer is the only place that can be separated. FoldingprincipalOf(user)into the digest turns the collision intoidempotency_key_reusedrather than a silently shared item. -
It depends on where the key was carried.
idempotency_keysits in each body struct withomitempty, so a first attempt with theIdempotency-Keyheader and a retry with the body field -- presented as equivalent in the OpenAPI text -- marshal differently, and the genuine retry is refused 409idempotency_key_reused.TestWorkItems_IdempotentOpencovers the header/body mismatch case, not this one. Hashing the body with the key field cleared, or always set to the resolvedkey, makes the two spellings agree.
| writeWorkItemError(w, err) | ||
| return | ||
| } | ||
| h.auditWorkItem(r, user, "work_item.execution", item.ID, item.Tenant, |
There was a problem hiding this comment.
P3 — this is the only mutating route that audits unconditionally; CreateWorkItem (313), AppendWorkItemIntent (453), ResumeWorkItem (555) and LinkWorkItemSurface (607) all gate on created.
AttachExecution returns created == false both for a pure idempotency replay and for a correlate call whose phase already matches, so a client that retries POST .../executions with the same Idempotency-Key writes a fresh work_item.execution audit entry on every attempt even though the store did nothing. A phase change is a real event worth auditing, so the condition is not simply if created — if created || exec.Phase != previousPhase needs the store to report it, or gate on created and accept losing the phase-change entry.
| if !decodeWorkItemBody(w, r, &body) { | ||
| return | ||
| } | ||
| if body.Tenant == "" || !user.HasTenantAccess(body.Tenant) { |
There was a problem hiding this comment.
P3: a missing tenant is answered 403 tenant_forbidden with the message no access to tenant "". The OpenAPI marks tenant required on this body, and HasTenantAccess("") is false only incidentally -- for an admin it returns true, so an admin omitting tenant falls through to the store and gets 400 invalid_request instead, i.e. the same malformed request yields a different status depending on who sends it. Splitting the empty case out as 400 invalid_request makes it uniform and matches the documented shape.
| if !decodeWorkItemBody(w, r, &body) { | ||
| return | ||
| } | ||
| if body.Tenant == "" || !user.HasTenantAccess(body.Tenant) { |
There was a problem hiding this comment.
P3 — an omitted tenant is answered as 403 tenant_forbidden with no access to tenant "", not 400 invalid_request. The OpenAPI marks tenant required, so a client that forgets it gets a permission error for a validation mistake — confusing to debug and it makes 403 rates unusable as an authorization signal. Splitting the empty case out costs two lines:
if body.Tenant == "" {
writeErrorCode(w, http.StatusBadRequest, wiCodeInvalid, "tenant is required", nil)
return
}
if !user.HasTenantAccess(body.Tenant) { ... 403 ... }
There was a problem hiding this comment.
No P1/P2 findings (4 P3). Good to merge.
Both P2s from the previous round are closed, each pinned by a test that fails without the fix: writeWorkItemError now logs the unmapped store error before the 500, and TestWorkItems_ListWithoutTenantStaysInsideTheCallersTenants exercises both arms of the list tenancy switch (a member sees only its own tenant with no ?tenant=, an admin sees both). The earlier P3s also landed: limit is rejected above 500 rather than silently clamped (TestWorkItems_LimitAndTenantAreValidated), an empty tenant is 400 not 403, the request digest drops idempotency_key so header and body spellings agree (TestWorkItems_KeyInHeaderOrBodyIsTheSameRequest), ResumeWorkItem builds its response from what Resume returned instead of re-reading, WORK_ITEMS_DISABLED goes through strconv.ParseBool, and surface-refs is in the forged-actor coverage.
The identity and authorization model holds up: principalOf keyed on how the caller authenticated, the forbidden-field scan before any decode, canSeeWorkItem re-evaluated per request, 404 rather than 403 for an invisible item, superseded_by gated on the caller's own visibility, and Store.List failing closed when Tenants is empty and AllTenants is unset. Reads sit outside the 20/min write group and mutations inside it, as the contract specifies.
Remaining P3s (none blocking, all inline): AttachWorkItemExecution and TransitionWorkItem audit on a pure idempotent replay where the other four handlers gate on created; the surface-refs 400 on an Idempotency-Key header is absent from the OpenAPI that declares that header on every other work-item POST; the request digest canonicalises only the top level, so a nested params object re-serialised in a different key order refuses a genuine retry; and a literal null body passes the body-must-be-an-object guard (harmless today, every route still 400s on a required field).
Note: go build and go test were denied in this sandbox, so this is static review; CI supplies TEST_DATABASE_URL, so the Postgres-backed handler tests run rather than skipping.
…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
|
Claude finished @mashkovd's task in 3m 39s —— View job Review posted — approved (0 P1, 0 P2, 4 P3)
Two inline comments are on the diff; the verdict is submitted against New in this commitBoth changes are real improvements:
The comments added to What holds upEverything from the previous round survives re-reading: P3 (none blocking)
One caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P2)
Finding 2 (P2)
|
| writeWorkItemError(w, err) | ||
| return | ||
| } | ||
| h.auditWorkItem(r, user, "work_item.execution", item.ID, item.Tenant, |
There was a problem hiding this comment.
P3 (carried over, still open) — AttachWorkItemExecution and TransitionWorkItem (:444) are the two mutating handlers that audit unconditionally; CreateWorkItem (:340), AppendWorkItemIntent (:480), ResumeWorkItem (:582) and LinkWorkItemSurface (:635) all gate on created.
A client retrying POST .../executions or PATCH /{id} with the same Idempotency-Key gets a pure store-level replay — replayed() returns early, nothing is written, no event is appended — yet each attempt still emits a work_item.execution / work_item.transition audit row. N retries behind a flaky proxy become N audit entries for one event, so the audit log stops being a usable answer to "how many times did this actually change".
created alone is not the right gate for attach (a genuine phase advance also returns created == false and is worth auditing) and Transition returns no such flag at all, so closing it properly means the store reporting "nothing changed" alongside its current returns. Follow-up rather than a reshuffle here.
There was a problem hiding this comment.
Agreed it needs the store to report a replay; tracked as #355 rather than reshuffled here.
| Records a surface-native conversation for reply routing, or refreshes | ||
| its last_seen_at. `actor_external_id` is correlation metadata only: it | ||
| is never used as, or resolved to, the acting principal (that needs | ||
| the surface-identity binding, mctl-api#350). |
There was a problem hiding this comment.
P3 (carried over, still open) — the published contract for this route does not mention Idempotency-Key, but LinkWorkItemSurface (internal/api/handlers_work_items.go:619) answers 400 invalid_request when the header is present.
Idempotency-Key is declared as a header parameter on every other work-item POST (/work-items, /intents, /executions, /resume), so a generated client or an HTTP layer that stamps it uniformly on all POSTs — a common SDK default, and the pattern the other four routes teach — gets a hard failure on this one route with nothing in the spec to explain it. TestWorkItems_SurfaceRefsRefuseAnIdempotencyKeyAndAForgedActor pins the behaviour; only the contract is silent.
One line in the description above (the handler's message text is already the right wording) makes it discoverable before a client hits it in production.
There was a problem hiding this comment.
Fixed in 609f3f3: the surface-refs description now says the route takes no Idempotency-Key and answers 400 to one.
There was a problem hiding this comment.
No P1/P2 findings (4 P3). Good to merge.
New in 60dfd69, and both are real improvements:
Store.Createnow takesFOR UPDATEon theexternal_keylookup. The per-tenant advisory lock already serialises concurrent creates, but not a concurrentTransitionon the matched item: without the row lock, a create could dedupe onto an item that a committing transition had just taken terminal, handing the caller closed work in answer to an "open work" request. Under READ COMMITTED the locked re-read re-evaluatesstate IN (active,waiting), so that item correctly drops out and a fresh one is opened — which the partial unique indexwork_items_tenant_external_key_openpermits, since it only covers open states. No deadlock is introduced:Createholdsworkitem-create:<tenant>and waits on a row lock, whileTransition/Resume/AttachExecutionholdworkitem:<id>and never wait on the tenant lock, so there is no cycle.TestSchemaUpgradesAnEarlierRevisionnow restores the droppedactorcolumns int.Cleanup(matching theADD COLUMN IF NOT EXISTS ... TEXT NOT NULL DEFAULT ...migration verbatim, and registered afternewStoreForTestso LIFO runs it while the pool is still open). A failure there stays one failure instead of leaving a half-dropped schema for every later test in the shared database.
The comments added to Transition (supersede may point at a terminal item) and to Executions/Events (an unknown id answers an empty list, not ErrNotFound, so the caller must Get first) document exactly the two edges a reader would otherwise have to infer; the handlers do Get first in both cases.
Everything checked in the previous round still holds: the principal comes only from auth.UserFromContext and is keyed on how the caller authenticated; the forbidden-field scan runs before any decode on every mutating route; canSeeWorkItem is re-evaluated per request and an invisible item answers 404, not 403; superseded_by is gated on the caller own visibility; Store.List fails closed when Tenants is empty and AllTenants is unset, and TestWorkItems_ListWithoutTenantStaysInsideTheCallersTenants exercises both arms of that switch. The authorization model matches docs/work-context-contract.md "Identity and authorization" as written. Mutations sit inside the 20/min write group and reads outside it, as the contract specifies.
Remaining P3s, none blocking:
AttachWorkItemExecution(:541) andTransitionWorkItem(:444) audit unconditionally; a purereplayed()hit writes a duplicatework_item.execution/work_item.transitionrow where the other four handlers gate oncreated. Closing it properly needs the store to report "nothing changed" — follow-up. (inline)- The
/surface-refs400 on anIdempotency-Keyheader is absent fromopenapi.yaml, which declares that header on every other work-item POST. (inline) requestDigestInputcanonicalises only the top level;intentBody.Paramsis ajson.RawMessagespliced in as sent, so the same params re-serialised in a different key order refuses a genuine retry with 409. Narrow in practice; the comment at:206overclaims either way.- A literal
nullbody passes the "body must be a JSON object" guard (:160) —json.Unmarshalinto a map accepts it. Harmless today since every route then 400s on a required field;|| keys == nilmakes the guard mean what it says.
Caveat: go build and go test were denied in this sandbox, so this is static review. CI supplies TEST_DATABASE_URL (validate.yml:53) and runs -p 1, so the Postgres-backed handler tests run rather than silently skipping.
…e-refs key - The kill switch turns work items off for any value except an explicit false/0/no/off: a boolean parse made "yes" or "disabled" silently leave them on, contrary to the README and .env.example. - OpenAPI: surface-refs takes no Idempotency-Key and answers 400 to one. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
agy P2 on |
|
Claude finished @mashkovd's task in 2m 51s —— View job Review posted — approved (0 P1, 0 P2, 4 P3)
One inline comment is on the diff; the verdict is submitted against New in this commit
Not reproducedThe agy P2 on self-supersede is not reachable. What holds upThe identity and authorization model survives re-reading: P3 (none blocking)
One caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| // "false" keeps them on while "yes" or "disabled" never silently fails open. | ||
| func workItemsDisabled(v string) bool { | ||
| switch strings.ToLower(strings.TrimSpace(v)) { | ||
| case "", "false", "0", "no", "off": |
There was a problem hiding this comment.
P3 — the inversion is the right call for a kill switch, but two values that strconv.ParseBool (the previous implementation, one commit ago) read as false now read as disabled: f and F. A WORK_ITEMS_DISABLED=f that meant "off" under 60dfd69 silently takes every /api/v1/work-items route to 503 here, with only a slog.Warn to show for it — the exact failure mode this commit set out to remove, just in the other direction.
Unlikely in practice, and erring toward off is the safer half of the trade, so this is a nit. Adding "f" to the false arm costs nothing and makes the set a superset of both spellings the flag has had:
case "", "false", "f", "0", "no", "off":Separately, TestWorkItemsDisabledParsesABoolean no longer describes what it tests — the function deliberately stopped parsing a boolean. The table itself is good (yes, on, disabled all pin the new behaviour); only the name is stale.
There was a problem hiding this comment.
Fixed in 03012b8: f is in the false arm again, and the test is renamed and turned into a case table (which also clears the gocritic whitespace-key lint failure).
There was a problem hiding this comment.
No P1/P2 findings (4 P3). Good to merge.
New in this commit: the WORK_ITEMS_DISABLED kill switch now errs toward off (any value except false/0/no/off), which matches what README.md and .env.example say and closes the agy P2 — a boolean parse left "yes" or "disabled" silently enabled. The table test pins yes/on/disabled as disabling and ""/false/0/no/off/" FALSE " as keeping it on. The /surface-refs description now documents that the route takes no Idempotency-Key and answers 400 to one, which was the other carried-over P3. strings is still needed in main.go and strconv is still used at :1095, so no unused import.
Checked the agy P2 on self-supersede: not reachable. TransitionInput.validate (internal/workitems/inputs.go:208) already rejects SupersededBy == WorkItemID before the store runs, so superseded_by cannot point at its own id.
The identity and authorization model holds up on re-reading: principalOf keyed on how the caller authenticated (service:/github:/oidc:), the forbidden-field scan before any decode on every mutating route, canSeeWorkItem re-evaluated per request, an invisible item answered 404 rather than 403, superseded_by gated on the caller's own visibility, and Store.List failing closed when Tenants is empty and AllTenants is unset. Reads are inside the authenticated group and outside the 20/min write budget; mutations are inside it, as the contract specifies. The openapi.yaml quoting fix plus TestSpecParses remains a real catch — that file did not parse before.
P3 (none blocking):
- cmd/api/main.go:1186 — "f"/"F" flipped meaning versus the previous ParseBool implementation, so WORK_ITEMS_DISABLED=f now disables; TestWorkItemsDisabledParsesABoolean no longer parses a boolean.
- handlers_work_items.go:541 and :444 — AttachWorkItemExecution and TransitionWorkItem audit unconditionally; a pure replayed() hit writes a duplicate work_item.execution / work_item.transition row where the other four handlers gate on created. Needs the store to report "nothing changed"; follow-up.
- handlers_work_items.go:228 — requestDigestInput canonicalises only the top level; intentBody.Params is a json.RawMessage spliced in as sent, so the same params re-serialised in a different key order refuses a genuine retry with 409.
- handlers_work_items.go:160 — a literal null body passes the "body must be a JSON object" guard; harmless today since every route then 400s on a required field.
Caveat: go build and go test were denied in this sandbox, so this is static review — CI supplies TEST_DATABASE_URL (validate.yml:53) and runs -p 1, so the Postgres-backed handler tests run rather than silently skipping.
gocritic flagged a whitespace map key. "f" keeps work items on, as it did under the boolean parse one commit ago. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 1m 56s —— View job Review posted — approved (0 P1, 0 P2, 5 P3)
One inline comment is on the diff; the verdict is submitted against New in this commit
What holds upRe-reading the full range, everything from the previous rounds survives: P3 (none blocking)
One caveat: |
…o range copy Also accept "f" as false in the kill switch, matching #354. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| | `AUDIT_DB_URL` | PostgreSQL connection string (falls back to in-memory). `sslmode=disable` is upgraded to `require`, or `verify-full` when a CNPG CA is mounted. | — | No | | ||
| | `HUMAN_INPUT_DB_URL` | PostgreSQL for the human-input delivery ledger (`POST /api/v1/human-input/{request_id}/response`). Falls back to `AUDIT_DB_URL`; with neither, that endpoint answers 503 (there is no in-memory fallback). The raw answer is kept only until its delivery resolves, or until the request expires. | — | No | | ||
| | `WORK_ITEMS_DB_URL` | PostgreSQL for the `workitem/v1` store (`/api/v1/work-items*`). Falls back to `AUDIT_DB_URL`; with neither, every work-items route answers 503. | — | No | | ||
| | `WORK_ITEMS_DISABLED` | Kill switch: any value except `false`/`0`/`no`/`off` leaves the work-items store off (routes answer 503) without touching `AUDIT_DB_URL` for other stores. | unset | No | |
There was a problem hiding this comment.
P3 — the two operator-facing docs now enumerate a set the code no longer matches. workItemsDisabled (cmd/api/main.go:1185) treats f/F as false, but this row and .env.example:23 both say "any value except false/0/no/off" — so by the documented rule WORK_ITEMS_DISABLED=f turns work items off, while the code leaves them on.
Harmless in direction (the divergence errs toward keeping the routes up, not toward a kill switch that silently fails open), and f is an unlikely thing to write by hand. But it is the same doc-vs-code gap the previous round closed in the other direction, so it is worth keeping the two in step: add `f` to both lists, or drop f from the code arm and keep the docs as the single spelling of the rule.
The function comment at cmd/api/main.go:1181 already lists "false"/"f"/"0"/"no"/"off" correctly; only the docs an operator actually reads are behind.
There was a problem hiding this comment.
No P1/P2 findings (5 P3). Good to merge.
New in 03012b8: workItemsDisabled accepts "f"/"F" as false again, so the set is now a superset of both spellings the flag has had — a WORK_ITEMS_DISABLED=f that meant "off" under the previous ParseBool implementation no longer flips to disabling every /api/v1/work-items route. TestWorkItemsDisabledErrsTowardOff is renamed to describe what it actually tests and converted to a case table (which also removes the gocritic whitespace-key issue); it pins ""/false/f/F/0/no/off/" FALSE\t" as keeping work items on and true/1/TRUE/yes/on/disabled as turning them off, so both halves of the kill-switch rule fail if either arm regresses.
Everything checked in earlier rounds still holds on re-reading: principalOf keyed on how the caller authenticated (service:/github:/oidc:), the forbidden-field scan before any decode on every mutating route, canSeeWorkItem re-evaluated per request, an invisible item answered 404 rather than 403, superseded_by gated on the caller's own visibility, and Store.List failing closed when Tenants is empty and AllTenants is unset (with both arms of the tenancy switch exercised). Mutations sit inside the 20/min write group and reads outside it, as the contract specifies. Store.Create's FOR UPDATE on the external_key match and the schema-upgrade test's column restore (registered after newStoreForTest, so LIFO runs it while the pool is open, and matching the migration's ADD COLUMN IF NOT EXISTS ... DEFAULT '' verbatim) are both still correct.
Remaining P3s, none blocking:
- README.md:138 and .env.example:23 say "any value except false/0/no/off", which no longer matches the code now that f/F is false. The divergence errs toward keeping routes up, not toward a switch that fails open. (inline)
- AttachWorkItemExecution (:541) and TransitionWorkItem (:444) audit unconditionally; a pure replayed() hit writes a duplicate work_item.execution / work_item.transition row where the other four handlers gate on created. Tracked as #355.
- requestDigestInput canonicalises only the top level; intentBody.Params is a json.RawMessage spliced in as sent, so the same params re-serialised in a different key order refuses a genuine retry with 409.
- A literal null body passes the "body must be a JSON object" guard (:160) — json.Unmarshal into a map accepts it. Harmless today since every route then 400s on a required field; || keys == nil makes the guard mean what it says.
- ListWorkItems does not validate the state query parameter at the HTTP layer; the store rejects an unknown one with ErrInvalid, so the outcome is a correct 400 — noted only because limit and tenant are validated in the handler and state is not.
Caveat: go build and go test were denied in this sandbox, so this is static review. CI supplies TEST_DATABASE_URL (validate.yml:53) and runs -p 1, so the Postgres-backed handler and store tests run rather than silently skipping.
Closes #349. Stacked on #351 (the store); I'll retarget to
mainonce #351 merges.Steps 2 and 4 of
docs/work-context-contract.mdon top of theinternal/workitemsstore.Routes
POST /api/v1/work-itemsIdempotency-Key; dedupes open work onexternal_keyGET /api/v1/work-itemsopen;tenant,state,owner,limitGET /api/v1/work-items/{id}{schema_version, work_item, state_version, latest_execution}. This is the route the mctl-agents client calls (orchestrator/work_context/client.pyROUTES["get_work_item"])PATCH /api/v1/work-items/{id}expected_state_versionrequiredPOST .../intentsGET|POST .../executionsPOST .../resumePOST .../surface-refsGET .../eventsMutations join the shared 20/min write group; reads stay outside it, as the contract specifies. Not in this PR, each with its own issue: snapshots (#352), approvals + the retention sweeper (#353),
surface-identities(#350). The contract's "Implementation status" section now says so.Acceptance criteria → evidence
router.go;TestSpecParsespins every routed method in the OpenAPI; routes not built are listed above with their issuesauth.UserFromContext; a caller actor is never authoritativeprincipalOf(github:/service:/oidc:by how the caller authenticated).decodeWorkItemBodyrefusesactor,actor_principal,owner(_principal),created_by,on_behalf_of,subject,delegated_actor,user(_id)with 400actor_not_acceptedon every mutating route.TestWorkItems_ForgedActorIsRejectedEverywhere(incl."actor":"tg:123") also asserts nothing changedactor_external_idis stored as correlation and never becomes an actor:TestWorkItems_SurfaceActorIsCorrelationNeverIdentitychecks that the service principal acts as itself, the subject staysgithub:alice, and history carries notg:/chat idsTestWorkItems_OpenReadAndPersist,TestWorkItems_IdempotentOpen,TestWorkItems_ResumeStartsTheNextExecution,TestWorkItems_StaleVersionIs409WithCurrentState; store-level race test in #351work_item_not_found; 409state_version_conflict/invalid_transition/execution_activewithdetails.state/state_versionTestWorkItems_NoTranscriptInHistoryOrAuditchecks the 8 KiB cap, secret rejection, and that neither events nor audit entries carry intent textTEST_DATABASE_URLTestWorkItems_UnconfiguredStoreAnswers503runs without a DBAuthorization: admin; otherwise
HasTenantAccessplus visibility (private→ owner only), re-evaluated per request. An invisible item is 404 (TestWorkItems_VisibilityAnswers404AndTenantAccess403). Anexternal_keythat resolves to someone else's private item returns 409external_key_in_usewithout disclosing it.superseded_bymust be visible to the caller.Wiring:
WORK_ITEMS_DB_URL→AUDIT_DB_URLfallback;WORK_ITEMS_DISABLEDkill switch; a nil store makes every route answer 503 with a startup warning.Unrelated fix:
internal/openapi/openapi.yamlonmaindoes not parse. An unquoted:in a human-input description (line 2341) broke/openapi.yamlfor every client generator. It is quoted here, andinternal/openapi/embed_test.gonow parses the spec: that test fails onmain's file and passes here.Mutation proof (each mutant is killed)
ForgedActorIsRejectedEverywherecanSeeWorkItem: drop the tenant check / drop the private-owner check →VisibilityAnswers404AndTenantAccess403principalOf: drop thegithub:/service:branch →OpenReadAndPersist/SurfaceActorIsCorrelationNeverIdentityIdempotency-Keymismatch →IdempotentOpengo test -p 1 ./...against postgres:17: all pass.golangci-lint: only the existing gosec G710 on/docs(router.go:235), which this PR does not touch.Consumer alignment
The mctl-agents mirror (
orchestrator/work_context/contract.py) expects pre-contract spellings (work_item_id,open/in-progress/...,executions[].sequence). It fails closed toWORK_ITEM_UNKNOWNbehindWORK_CONTEXT_ROLLOUT_MODE=off, and it says it will adopt #227's names once they are readable. That alignment is a separate mctl-agents change.🤖 Generated with Claude Code
https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G