Skip to content

feat(api): scope pull_task / GET /work/next to a specific run_id#79

Merged
viktor-shcherb merged 4 commits intomainfrom
dev/75-pull-task-run-id
Apr 30, 2026
Merged

feat(api): scope pull_task / GET /work/next to a specific run_id#79
viktor-shcherb merged 4 commits intomainfrom
dev/75-pull-task-run-id

Conversation

@viktor-shcherb
Copy link
Copy Markdown
Member

Summary

Adds an optional run_id filter to both GET /work/next and the pull_task MCP tool so an agent can drive a single Murmur run end-to-end without picking up unrelated stale or concurrent work that's also in the queue. Closes #75.

Without the parameter, the legacy global-FIFO behaviour is preserved verbatim — backwards compatible.

Files changed

  • src/api/agent/sql.ts — new CLAIM_BY_RUN_SQL (4-bind variant of CLAIM_SQL with AND run_id = ? in the inner SELECT).
  • src/api/agent/work.ts — prepare claimByRunStmt; GET /next reads c.req.query("run_id") and dispatches to the new statement when set.
  • src/mcp/tools.ts — new PULL_TASK_INPUT_SHAPE = { run_id?: string }; register pull_task with inputSchema; defensive (args, extra) vs (extra) callback-signature handling for in-process SDK quirk; PULL_TASK_DESCRIPTION updated.
  • src/api/agent/agent.test.ts — 4 new tests covering legacy / scoped / unknown-run / cross-pipeline cases.
  • src/mcp/server.test.ts — 2 new MCP tests for run-scoped pull_task; the existing 3 pull_task-callsite tests now pass arguments: {} (the SDK requires arguments to be an object once an inputSchema is set, even with all-optional fields; {} is semantically identical to omitted args).
  • DESIGN.md §3.3 / §3.4 — documents the new optional run_id parameter.

Verification (from the issue)

  • Unit test: GET /work/next with no run_id claims oldest ready row globally (legacy)
  • Unit test: GET /work/next?run_id=r_X claims oldest ready row whose run_id == r_X, ignoring older ready rows from other runs
  • Unit test: GET /work/next?run_id=r_unknown returns {ok:true, data:null}
  • MCP test: pull_task with no args still works (legacy) — exercised by 3 existing tests via arguments: {}
  • MCP test: pull_task({run_id: 'r_X'}) claims only that run's work
  • All 11 existing tests in src/mcp/server.test.ts still pass

Manual checks run locally

  • pnpm typecheck clean
  • pnpm exec eslint <changed files> clean (the pre-existing .claude/worktrees/agent-*/ leftover dirs surface unrelated errors in pnpm lint; out of scope for this PR)
  • pnpm test 294 + 34 = 328 tests pass
  • pnpm grep:all all four gates pass
  • pnpm test:unit src/mcp/server.test.ts run repeatedly — stable, 13/13 green

Notes for reviewer

  • The pull_task callback now uses a defensive (args, extra) vs (extra) discriminator. The current SDK always calls (args, extra) once inputSchema is set, but the orchestrator's heads-up flagged a historical/test-harness quirk where the callback could be invoked with a single extra argument. The discriminator checks for the RequestHandlerExtra marker slots (requestId, sendNotification); cheap and contained.
  • CLAIM_BY_RUN_SQL keeps the same composite-index plan: SQLite's planner uses idx_subtask_instances_ready for the leaf scan and evaluates AND run_id = ? row-by-row off the leaf; verified by reading the prepared-statement plan in dev console.
  • Empty-string run_id= is treated as missing (defensive against ?run_id= URL slips), matching the wire convention that c.req.query returns undefined for missing fields.

- src/api/agent/sql.ts: add CLAIM_BY_RUN_SQL (4-bind variant of CLAIM_SQL with AND run_id = ?)
- src/api/agent/work.ts: prepare claimByRunStmt; GET /next reads c.req.query('run_id') and dispatches to the new statement when set, else legacy global FIFO
- src/mcp/tools.ts: add PULL_TASK_INPUT_SHAPE { run_id?: string }; register pull_task with inputSchema; defensive (args, extra) vs (extra) signature handling for in-process SDK quirk; update PULL_TASK_DESCRIPTION to mention the new param

Closes part of #75 (interfaces only — verification tests follow).
- src/api/agent/agent.test.ts: 4 new tests
  * legacy global-FIFO behaviour with no run_id
  * run_id filter skips older rows from other runs
  * unknown run_id returns null even with non-empty global queue
  * filter is independent of pipeline_id (cross-pipeline scope check)
- src/mcp/server.test.ts: 2 new MCP tests
  * pull_task({ run_id }) claims only that run's work
  * pull_task({ run_id: 'r_unknown' }) returns null
- Existing 11 server.test.ts tests updated to pass arguments: {} for the
  3 pull_task callsites — the SDK now requires arguments to be an object
  even when all schema fields are optional. Semantically identical to no
  args (no run_id supplied).
…al run_id

Documents the run-scope filter added in issue #75. Both the HTTP route
and the MCP tool gain an optional run_id; without it, the legacy global
FIFO behaviour is preserved.
Copy link
Copy Markdown
Member Author

@viktor-shcherb viktor-shcherb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: APPROVE

(Submitted as a comment because the GH API blocks self-approval; the PR is the same author as this reviewer agent's account.)

All issue verification items met:

  • Unit: GET /work/next (no run_id) claims oldest ready globally — agent.test.ts L772-797.
  • Unit: GET /work/next?run_id=run-NEW skips older run-OLD's row — L799-829, also asserts run-OLD-first stays ready.
  • Unit: GET /work/next?run_id=r_unknown returns {ok:true,data:null} and does not consume the global queue — L831-852.
  • Unit: cross-pipeline run_id scope check — L854-873.
  • MCP: legacy pull_task (no args) works via arguments: {} in 3 existing tests.
  • MCP: pull_task({run_id: 'run-NEW'}) claims only that run — server.test.ts L417-449.
  • MCP: pull_task({run_id: 'r_unknown'}) returns null — L451-465.

Local gates run on the PR worktree:

  • pnpm typecheck — clean.
  • pnpm test — 294 + 34 passing.
  • pnpm grep:all — all four gates pass.
  • pnpm lint on the changed files (src/api/agent/{sql,work}.ts, src/api/agent/agent.test.ts, src/mcp/{tools,server.test}.ts) — clean. Pre-existing .claude/worktrees/... leftover dirs surface unrelated lint errors as the PR description notes.

Index-use confirmation: I ran EXPLAIN QUERY PLAN against both CLAIM_SQL and CLAIM_BY_RUN_SQL on a fresh in-memory DB. Both produce SEARCH subtask_instances USING INDEX idx_subtask_instances_ready (status=?) — the planner keeps the index for the (status='ready', created_at) leaf and evaluates AND run_id = ? row-by-row off the leaf. The SQL header comment is accurate.

CI: green (quality success, build skipped per branch policy).

Process:

  • Claim posted 11:52:55Z; sketch 11:53:22Z; first commit 11:56:55Z — sketch BEFORE code, good.
  • Commits ordered interfaces:tests:docs:. The interfaces: commit bundles the actual implementation (not just stubs with throw not implemented); a strict reading of AGENTS.md §4-§6 expects skeleton-then-tests-then-impl. Mild slip, consistent with how earlier feature commits in this repo (e.g., feat(api): atomic claim pickup with CAS submit) were authored. Not blocking. Worth flagging in the next orchestrator round so the developer adopts the strict 3-commit rhythm going forward.

Nits (non-blocking, address if you'd like):

  • src/mcp/tools.ts L324-346: the defensive (args, extra) vs (extra) discriminator is contained and cheap, but the SDK contract since adding inputSchema is now stably (args, extra) — the discriminator is pure paranoia. Fine to keep; if you ever want to simplify, drop it and rely on the schema-set callback signature (covered by the new MCP tests that exercise both single-arg and double-arg call paths in-process).
  • CLAIM_BY_RUN_SQL JSDoc could mention the EXPLAIN QUERY PLAN result inline rather than asserting "tolerates the extra filter" — cheap reader trust win.

LGTM. Orchestrator: ready to merge.

@viktor-shcherb viktor-shcherb merged commit 253fd6e into main Apr 30, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(api): scope pull_task / GET /work/next to a specific run_id

1 participant