fix(agents): tell the truth about inbox-lite's status filter and the in_review checkout contract (BLO-18858)

[allyblockcast]

Split out of #821 per the scope decision on BLO-18858. Docs/contract half — no runtime behaviour change. The runtime half (making unlocked in_review rows atomically checkoutable) is BLO-22666.

Thinking Path

paperclipInboxLite's tool description and OpenAPI summary both claimed the endpoint returned in_review work. The route filters to todo,in_progress,blocked (server/src/routes/agents.ts:2451).

An agent whose only assignments were in_review therefore got a correct empty array from a call that had promised otherwise, read it as a platform failure, and hand-rolled a checkout-lock-blind listIssues sweep to "recover". Two concurrent runs of the same agent then did a full task twice. The empty array was right — the description was the bug, and it is the whole root cause of the incident.

That led to a second question: if agents are told to checkout rather than sweep, is the documented checkout payload itself correct? It was not. Ally's review at b4634b1f caught that documenting in_review in expectedStatuses inverts the behaviour the same page promises. Verified against the service rather than taken on faith:

• issueService.checkout runs its atomic UPDATE first, matching inArray(issues.status, expectedStatuses), and returns early on a match (server/src/services/issues.ts:9187-9235).
• The typed 422 issue_in_review_not_checkoutable is only reachable when that UPDATE matches nothing (:9417-9433).

So documenting in_review made the 422 unreachable and silently flipped an unlocked review/approval wait to in_progress. The old paragraph's justification — that in_review in expectedStatuses is "what makes a locked in_review issue return 409" — was also wrong: the 422 branch is guarded on checkoutRunId == null && executionRunId == null, so a locked row falls through to the generic conflict either way. Omitting in_review preserves both behaviours and makes the documented 422 reachable. The service comment at :9411 already says in_review "is excluded from every caller's expectedStatuses" — the docs had drifted from that stated intent.

What Changed

File	Change
packages/mcp-server/src/tools.ts	paperclipInboxLite description: real status set + what empty means per wake type
server/src/routes/openapi.ts	summary names the status set
skills/paperclip/SKILL.md	Step 3 inbox-lite semantics; Step 4 rewritten to checkout-first; Step 5 lock contract; in_review removed from the documented expectedStatuses and the justification paragraph corrected
issue-triage/SKILL.md	status-scoped in_review read restored; raw sweep banned; checkout failure modes; same expectedStatuses correction
generated/catalog.json	regenerated — only the issue-triage sizeBytes/sha256/contentHash move
agent-inbox-lite-status-contract.test.ts	new — pins the route's status filter
checkout-expected-statuses-doc-contract.test.ts	new — pins the documented checkout payload against in_review drift

The Step 4 rewrite is the substantive part

Master told agents to compare $PAPERCLIP_RUN_ID against executionRunId before working, and to post a comment if they lost. Both are wrong:

• The comparison is TOCTOU-unsafe — the lock can be taken between the read and the first write. Those fields are the lock's storage, not its API.
• Commenting on a lost race mutates an issue another run owns, bumping activity and potentially triggering another wake.

Checkout is the atomic boundary, and it is run-scoped — a concurrent run of your own agent gets a 409 exactly like a foreign agent.

Verification

• General tests (server N/4) runs both contract tests. Locally: 8/8 passed across the two files.
• Load-bearing assertions: expect(filters.status.split(",")).not.toContain("in_review") (route filter) and expect(statuses).not.toContain("in_review") (documented checkout payload).
• Mutation-checked. Reintroducing in_review into skills/paperclip/SKILL.md fails exactly the new doc-contract assertion and nothing else (1 failed | 3 passed), which is what establishes the guard is not vacuous. A separate emptiness assertion guards against the regex passing vacuously if the snippet is reworded or moved.
• The behavioural side of the 422 contract is already pinned by the existing typed-422 test in issues-service.test.ts:8215, which uses exactly the payload now documented — so doc and behaviour are asserted against the same value from both directions.

Risks

• Low. No runtime source is modified: the only non-test, non-doc edits are two description/summary string literals (tools.ts, openapi.ts). No control flow changes.
• The expectedStatuses change is to documentation, not to the service. Agents already receive 409/422 exactly as before; they simply stop being told to send a value that defeats the 422.
• catalog.json is regenerated by its own builder rather than hand-edited; the diff is confined to the one skill actually touched (4 lines), verified before commit.
• Residual: agents holding a cached copy of the old skill text keep sending in_review until they refresh. That is a propagation delay, not a regression — it is the status quo today.

Model Used

claude-opus-5[1m]

Acceptance criteria

• paperclipInboxLite description and OpenAPI summary name the true status set (todo, in_progress, blocked) and state that in_review is excluded by design
• Both skills state that an empty inbox is an answer, not an error, and forbid the raw-sweep "recovery"
• The heartbeat contract says checkout — not a field read — is the ownership decision, and does not instruct a losing run to mutate the issue
• issue-triage retains a status-scoped discovery path for the in_review work it advertises triaging
• The documented checkout payload does not contain in_review, and a test prevents that drift recurring
• No runtime source file is modified (server/src/services/**, server/src/routes/** except the OpenAPI summary string)

Deliberately excluded

Carried in #821's branch but not here, because each depends on the runtime change or is unrelated:

1. paperclipCheckoutIssue description rewrite + expectedStatuses default gaining in_review — asserts behaviour master does not have.
2. server/src/services/issues.ts + its two large lock test files — the runtime change, tracked as BLO-22666, which still carries an open Critical at server/src/routes/issues.ts:5096.
3. ui/src/pages/apps/ReviewQueueCard.tsx — unrelated drive-by.
4. A revert of paperclipCreateIssue's duplicateCandidates text and its test — fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821's branch deletes both, which master has. Almost certainly a bad merge resolution; carrying it would have silently reverted someone else's work.

Related PRs (dedup search)

Searched the GitHub PR list (open + recently closed) for inbox-lite, BLO-18858, and expectedStatuses. Three artifacts exist for this one issue — disclosed here rather than left for a reviewer to discover:

PR	Author	State	Relationship
#821	app/allyblockcast	open, draft	Original. Converted to draft 2026-08-06 because it read green while carrying an open Critical at a head nobody had approved. This PR is its docs/contract half.
#1117	kkroo	open, CONFLICTING	Human re-authoring of #821's full tree (opened 22:36Z, one minute after this PR) so an independent author could carry the Ally approval. Overlaps this PR and partly contradicts it — see below.
#1126	app/allyblockcast	open	Different defect (BLO-22574); shares only the issue thread.

The overlap with #1117 is substantive, not cosmetic

#1117 carries the runtime change (server/src/services/issues.ts, +39/−37) that removes the unlocked-in_review checkout rejection. This PR documents that the typed 422 issue_in_review_not_checkoutable exists and that in_review must therefore stay out of expectedStatuses. Both are defensible; they are not simultaneously true. Landing them in either order without a decision leaves the skills describing behaviour the service does not have — which is this issue's original defect, reintroduced.

They also collide textually: both touch skills/paperclip/SKILL.md, issue-triage/SKILL.md, generated/catalog.json, tools.ts, openapi.ts, and both add agent-inbox-lite-status-contract.test.ts.

Sequencing is a call for #1117's author; raised on that PR and on BLO-18858 rather than resolved unilaterally here.

Checklist

• I searched the GitHub PR list (open + recently closed) for similar or duplicate PRs — found fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821, fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 and fix(monitor): two-surface PR review gate check (BLO-22574) #1126, all linked above

[allyblockcast]

🔗 Paperclip issue: BLO-18858

[allyblockcast]

🔗 Paperclip issue: BLO-18858

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• PR is titled docs: but includes source code changes (packages/mcp-server/src/tools.ts, server/src/routes/openapi.ts). Please retitle as fix:, feat:, or refactor: so the right gates run, or remove the source code changes if this is genuinely a docs: PR.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b4634b1

Critical Issues (0)

Important Issues (1)

• [gstack/review] skills/paperclip/SKILL.md:90 — Including in_review in the documented checkout request makes the new typed-422 guidance at line 99 unreachable for an unlocked review issue. issueService.checkout performs its atomic update first for every status in expectedStatuses (server/src/services/issues.ts:9185-9212), setting the row to in_progress; the existing issue_in_review_not_checkoutable test gets 422 only because it omits in_review from expectedStatuses (server/src/__tests__/issues-service.test.ts:8215-8220). Following this skill therefore silently moves an unlocked review/approval wait back into active execution instead of preserving review state as promised.
  • Remove in_review from the documented expectedStatuses list (including the matching issue-triage request), or change checkout to reject in_review before the initial update. Add a service test using the exact documented payload so this contract cannot drift again.

Suggestions (1)

• [tests] server/src/__tests__/agent-inbox-lite-status-contract.test.ts:104 — The empty-result test only reasserts a mocked empty list; consider relying on the status-filter assertion or replacing this case with coverage that distinguishes an excluded in_review assignment from a general empty service response.

Strengths

• The MCP description, OpenAPI summary, and route test consistently identify the actual todo,in_progress,blocked inbox filter.
• The guidance correctly rejects read-then-write lock inspection as a substitute for atomic checkout and avoids side-effecting comments after a lost checkout race.

Recommended Action

1. Correct the in_review checkout contract before merge.
2. Add exact-payload regression coverage for the documented behavior.

This PR is authored by app/allyblockcast; the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• PR is titled docs: but includes source code changes (packages/mcp-server/src/tools.ts, server/src/routes/openapi.ts). Please retitle as fix:, feat:, or refactor: so the right gates run, or remove the source code changes if this is genuinely a docs: PR.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please re-review at head 257274923.

Addresses your Important finding from b4634b1f (skills/paperclip/SKILL.md:90 — documenting in_review in expectedStatuses makes the typed 422 unreachable).

I verified the mechanism against the service rather than taking it on faith, and you are right:

• checkout runs its atomic UPDATE first on inArray(issues.status, expectedStatuses) and returns early on a match (issues.ts:9187-9235); the 422 is only reached when that UPDATE matches nothing (:9417-9433). So documenting in_review silently flipped an unlocked review wait to in_progress.
• The paragraph's stated justification was also wrong — it claimed in_review in expectedStatuses is what yields 409 on a locked row. It is not: the 422 branch is guarded on checkoutRunId == null && executionRunId == null, so a locked row falls through to the generic conflict either way. Removing in_review therefore preserves both behaviours.
• issues.ts:9411 already stated in_review "is excluded from every caller's expectedStatuses" — the docs had drifted from that intent.

Changes: in_review removed from the documented payload in both skills/paperclip/SKILL.md and issue-triage/SKILL.md; the justification paragraph rewritten to explain the ordering hazard; catalog.json regenerated (diff confined to the issue-triage entry, 15 skills intact).

On your requested test: the existing typed-422 service test (issues-service.test.ts:8215) already uses exactly the payload now documented, so the behavioural side is pinned. I added checkout-expected-statuses-doc-contract.test.ts to pin the documentation side, which is where the drift actually happened. Mutation-checked: reintroducing in_review fails exactly that assertion and nothing else (1 failed | 3 passed); a separate emptiness assertion stops the regex passing vacuously if the snippet moves.

Your Suggestion on agent-inbox-lite-status-contract.test.ts:104 (the empty-result case only reasserting a mocked empty list) I have not acted on — it is fair, but it is a test-quality improvement on a case that is not load-bearing, and I would rather not expand this diff further while it is waiting to merge. Say the word if you want it in this PR rather than a follow-up.

Also in this push: PR retitled docs: → fix: and the body given the required sections, both of which the commitperclip quality gate was failing on — unrelated to your review, but it was the actual reason the review check was red.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2572749

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The inbox-lite contract is now consistent across the MCP tool, OpenAPI summary, skills, route behavior, and regression coverage.
• The checkout guidance correctly makes the atomic checkout operation, rather than a lock-field read, the ownership boundary.
• The expected-status documentation preserves the services typed unlocked-in_review response instead of implicitly moving review work to in_progress.

Recommended Action

1. No blocking changes requested.

[allyblockcast]

Closing as superseded by #1117 (kkroo), which was opened one minute after this PR and targets the same issue, BLO-18858.

Not a judgement call about quality — the diff makes it mechanical:

• 6 of this PR's 7 files are also in fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117, and fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 carries the Step 3 inbox-lite prose from here essentially verbatim, plus the matching Critical Rules entries.
• fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 also fixes the behaviour, where this PR only documents it: fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 removes the unlocked-in_review checkout rejection so an assigned in_review row becomes atomically checkoutable. That addresses BLO-18858's root cause rather than describing it.
• This PR's one unique file — server/src/__tests__/checkout-expected-statuses-doc-contract.test.ts — becomes wrong under fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117. It hard-asserts the skills must never document in_review in expectedStatuses, which is only correct while the 422 issue_in_review_not_checkoutable exists. fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 deletes that 422. Landing both, in either order, leaves the docs describing a service that does not exist — the very defect BLO-18858 was filed about.

The one thing here that #1117 does not have is the Step 4 rewrite (removing the racy "compare $PAPERCLIP_RUN_ID against executionRunId and cede" instruction, which #1117's own new Critical Rule forbids). I've offered that hunk on #1117 directly rather than landing a redundant PR to carry it: #1117 (comment)

Branch pr1116 is not deleted and this PR is reopenable — if #1117 stalls or you'd rather take the docs-only half first, say so on BLO-18858 and I'll reopen.