Make MCP task authoring reliable - #176
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesMCP task workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant ProposalBundleWorkflow
participant PrismaTransaction
participant TaskRecords
MCPClient->>ProposalBundleWorkflow: submit reviewed candidate references
ProposalBundleWorkflow->>ProposalBundleWorkflow: validate aliases, visibility, and dependency closure
ProposalBundleWorkflow->>PrismaTransaction: persist accepted tasks and related records
PrismaTransaction->>TaskRecords: create tasks, parents, blockers, impacts, and endpoints
TaskRecords-->>MCPClient: return reference map and persisted task IDs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb88756ae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR hardens the MCP task-authoring contract in @optimitron/web so agents can reliably (a) enumerate task inventories without silent truncation, (b) reference tasks by stable IDs/taskKeys (plus bundle-local refs), and (c) persist complete proposal bundles atomically with correct OAuth boundary enforcement.
Changes:
- Added opt-in cursor pagination to
listTasksandsearchTasks, including bounded authorized windows with explicitRESULT_WINDOW_EXCEEDEDerrors. - Expanded task reference handling across create/update/dependency/bundle flows to accept exact task IDs or exact
taskKeyvalues, plus bundle-localrefaliases and a returnedreferenceMap. - Made proposal bundle persistence atomic (tasks + edges + endpoints + impact + provenance) and tightened OAuth boundary checks; updated docs, tests, and the personal task-engine smoke script accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/lib/mcp-server.ts | Implements pagination cursors/windows, exact-reference resolution, bundle alias/referenceMap support, transactional bundle writes, and stricter boundary validation. |
| packages/web/src/lib/mcp-instructions.ts | Updates agent-facing operational guidance for pagination, references, bundles, and completion flows. |
| packages/web/src/lib/tests/mcp-server.test.ts | Adds coverage for pagination behavior, reference resolution, bundle atomicity/alias rules, and dependency updates/outcomes. |
| packages/web/scripts/mcp-personal-task-engine-smoke.ts | Updates smoke script to use personal roots, exact references, completion operations, and paginated listing. |
| docs/MCP_SERVER.md | Documents pagination envelopes, bounded windows, reference rules, and the bundle/referenceMap workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/lib/mcp-server.ts (1)
12658-12712: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound the existing-task lookup for
proposeTaskBundle.
prisma.task.findManyhas notakeand no reference filter atpackages/web/src/lib/mcp-server.ts:12658. Admin callers can read each non-admin branch owner's tasks without the non-adminOR, and the select loads livesourceArtifactsfor every result. Collect the candidate and dependency aliases first, then filter this query on those aliases and add a bound before comparing againstexistingTasks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/mcp-server.ts` around lines 12658 - 12712, Update the existing-task lookup inside proposeTaskBundle to collect the candidate and dependency aliases before querying, then constrain prisma.task.findMany to those aliases for every caller, including admins. Add the appropriate result bound and avoid loading unrelated live sourceArtifacts; preserve the subsequent existingTasks comparison using the bounded, alias-filtered results.
🧹 Nitpick comments (3)
packages/web/src/lib/mcp-server.ts (2)
14789-14825: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRun the edge probe and both writes in one transaction.
taskEdge.findFirst,taskEdge.updateMany, andtaskEdge.createManyexecute as three independent statements. Two concurrentaddDependencycalls for the same pair can both readexistingEdge == nulland both reportoutcome: "created".skipDuplicatesprotects the row, so only the reported outcome is wrong. Wrapping the three statements inprisma.$transactionmakes the reported outcome match the write that actually happened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/mcp-server.ts` around lines 14789 - 14825, Wrap the taskEdge.findFirst probe, updateMany, and createMany operations in a single prisma.$transaction within addDependency, and derive the returned created/outcome values from the transaction-scoped existingEdge result. Preserve the existing edge metadata, duplicate handling, and response shape while ensuring concurrent calls report the outcome of the serialized write.
13066-13256: 🗄️ Data Integrity & Integration | 🔵 TrivialBound the work performed inside the bundle transaction.
The transaction now creates every accepted draft, then loops over each created draft to update the parent, create each blocker edge, upsert the communication endpoint, create the impact estimate, and upsert the source artifact.
attachProposalImpactEstimateandattachProposalSourceArtifactalso perform a dynamicawait import(...)inside the transaction. The work is sequential and scales with the candidate count, so a large bundle holds row locks for the fulltimeout: 60_000window and can abort after doing all the work.Consider capping the accepted candidate count per bundle, and moving the dynamic imports to before
prisma.$transactionso no module load happens while the transaction is open.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/mcp-server.ts` around lines 13066 - 13256, The bundle transaction performs unbounded sequential work and dynamically imports modules while holding locks. In the flow surrounding prisma.$transaction, cap the accepted/promotable candidate count per bundle using the established bundle limit behavior, and resolve the modules required by attachProposalImpactEstimate and attachProposalSourceArtifact before entering the transaction; reuse those imports inside the transaction while preserving all existing task, dependency, endpoint, impact, and artifact operations.packages/web/src/lib/__tests__/mcp-server.test.ts (1)
4211-4242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe transaction mock cannot prove rollback.
mocks.transactioninvokes the callback withtransactionClientand performs no rollback, so this test proves ordering only: the impact failure aborts beforesourceArtifactUpsertruns. It does not prove that the created draft rows are discarded.
mocks.taskCreateis also shared between the base client andtransactionClient, so a regression that movedtask.createoff the transaction client would still pass. Consider giving the transaction client a distincttask.createspy, or asserting the call order so the test fails if draft creation leaves the transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/__tests__/mcp-server.test.ts` around lines 4211 - 4242, Strengthen the “keeps proposal attachments inside the draft transaction” test by making transactionClient.task.create distinct from the base client’s task.create and asserting draft creation uses the transaction client, ideally with call-order verification before createDirectTaskImpactInTransaction. Update the transaction mock or test setup so a rollback-related regression that moves task creation outside the transaction cannot still pass; preserve the existing failure and attachment assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/MCP_SERVER.md`:
- Line 64: Update the guidance sentence near the blockerTaskRefs documentation
to recommend blockerTaskRefs instead of the legacy depends_on field, while
leaving the legacy-alias clarification unchanged.
- Around line 104-120: Label the first JSON block as the paginated request and
the second as the response in the listTasks/searchTasks documentation. Add
concise labels immediately above each code block, preserving both JSON examples
unchanged.
In `@packages/web/scripts/mcp-personal-task-engine-smoke.ts`:
- Around line 146-160: Update listAllTasks so it does not request an unbounded
public task window: narrow listTasks with a query-level filter such as
parentTaskId or status, or handle RESULT_WINDOW_EXCEEDED as a skip in the
before/after count assertion while preserving the existing pagination behavior.
In `@packages/web/src/lib/mcp-server.ts`:
- Around line 13173-13230: Update the blocker-edge creation loop to deduplicate
by resolved blockerTaskId rather than blockerRef. Track task IDs already wired
for the current task, skip references resolving to an existing ID, and preserve
the existing validation and tx.taskEdge.create behavior for the first
occurrence.
---
Outside diff comments:
In `@packages/web/src/lib/mcp-server.ts`:
- Around line 12658-12712: Update the existing-task lookup inside
proposeTaskBundle to collect the candidate and dependency aliases before
querying, then constrain prisma.task.findMany to those aliases for every caller,
including admins. Add the appropriate result bound and avoid loading unrelated
live sourceArtifacts; preserve the subsequent existingTasks comparison using the
bounded, alias-filtered results.
---
Nitpick comments:
In `@packages/web/src/lib/__tests__/mcp-server.test.ts`:
- Around line 4211-4242: Strengthen the “keeps proposal attachments inside the
draft transaction” test by making transactionClient.task.create distinct from
the base client’s task.create and asserting draft creation uses the transaction
client, ideally with call-order verification before
createDirectTaskImpactInTransaction. Update the transaction mock or test setup
so a rollback-related regression that moves task creation outside the
transaction cannot still pass; preserve the existing failure and attachment
assertions.
In `@packages/web/src/lib/mcp-server.ts`:
- Around line 14789-14825: Wrap the taskEdge.findFirst probe, updateMany, and
createMany operations in a single prisma.$transaction within addDependency, and
derive the returned created/outcome values from the transaction-scoped
existingEdge result. Preserve the existing edge metadata, duplicate handling,
and response shape while ensuring concurrent calls report the outcome of the
serialized write.
- Around line 13066-13256: The bundle transaction performs unbounded sequential
work and dynamically imports modules while holding locks. In the flow
surrounding prisma.$transaction, cap the accepted/promotable candidate count per
bundle using the established bundle limit behavior, and resolve the modules
required by attachProposalImpactEstimate and attachProposalSourceArtifact before
entering the transaction; reuse those imports inside the transaction while
preserving all existing task, dependency, endpoint, impact, and artifact
operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e96246ac-04c2-438f-8b4c-f10f0fa0df98
📒 Files selected for processing (5)
docs/MCP_SERVER.mdpackages/web/scripts/mcp-personal-task-engine-smoke.tspackages/web/src/lib/__tests__/mcp-server.test.tspackages/web/src/lib/mcp-instructions.tspackages/web/src/lib/mcp-server.ts
PR review packetStart here
Review checklist
Changed files considered
Updated automatically when this PR's preview or visual review reruns. |
Code reviewOne high-confidence bug found (bug agents disagreed on the exact mechanism; verified directly against the PR's actual code before reporting).
|
|
Code review Reviewed for CLAUDE.md/AGENTS.md compliance (two independent passes) — no violations found; the PR stays within its stated scope. Two confirmed bugs in packages/web/src/lib/mcp-server.ts, both independently verified against the PR head commit: 1. listTasks hard-fails legacy (non-paginated) calls that use extended filters optimitron/packages/web/src/lib/mcp-server.ts Lines 9646 to 9677 in 906ceb3 needsCompleteAuthorizedWindow is wantsPagination || needsExtendedFiltering — not gated on wantsPagination alone — and the RESULT_WINDOW_EXCEEDED error (L9668-9676) is returned before the !wantsPagination legacy-response branch (L9719). So a plain call like listTasks({ executionMode: "AGENT_ONLY", limit: 5 }) (no paginated, no cursor) now returns a hard error with zero results whenever the caller's authorized task set exceeds 5000 rows. Before this PR the same call fetched a bounded window and returned best-effort matches. This contradicts the PR's own new doc text in docs/MCP_SERVER.md: "Calls with neither paginated: true nor cursor retain the legacy one-page array response." It also makes the error message's remedy unactionable in this path: it says "Narrow the query-level filters," but executionMode (and requiredTags, compensationKind, engagementKind, remotePolicy, applicationPolicy, ownerOrganizationId) are applied in-memory after this check (L9678+), not pushed into the Prisma where clause — there is no query-level filter to narrow. Contrast with the searchTasks handler, which correctly returns the legacy response before checking the window. Suggested fix: gate the window check on wantsPagination only, or move the !wantsPagination legacy return above the window check (as searchTasks does). 2. searchTasks pagination window guard is off-by-one — both false-rejects and can silently omit matches optimitron/packages/web/src/lib/mcp-server.ts Lines 10129 to 10146 in 906ceb3 tasks.searchTasks clamps its DB candidateLimit to Math.min(Math.max(limit * 4, 64), 500), so limit: 500 (passed here when paginating) always yields DB take: 500 — no sentinel row past the window (contrast listTasks, which correctly fetches 5001 and guards on greater than 5000). The guard here checks results.length >= 500, the same value as the fetch cap rather than one past it:
Note: fixing this isn't a one-line change in this file alone — candidateLimit is hard-clamped to 500 in tasks.server.ts, so raising the mcp-server fetch limit to 501 requires also raising that clamp (or having searchTasks return an explicit truncation signal). No other significant or high-confidence issues found. |
Goal
Make MCP task authoring reliable enough that an agent can create a useful task graph in one pass, save the returned references, enumerate the whole queue, and complete work through the correct workflow without guessing generated IDs or status fields.
What changed
listTasksandsearchTasks, with bounded result windows and explicit errors instead of silent omissiontaskKeyvalues across task creation, updates, dependencies, and bundles; reject ambiguous or conflicting referencesidandtaskIdfromcreateTask, and return areferenceMapfrom bundle proposals so follow-up calls can reuse persisted IDscompleteTask,completeTaskClaim, and formal execution/verification flowsCompatibility and scope
listTasksandsearchTasksresponse arrays remain unchanged until callers opt in withpaginated: trueor a cursor*Refnames are documented@optimitron/dbtype changesValidation
pnpm --filter @optimitron/web exec vitest run src/lib/__tests__/mcp-server.test.ts --maxWorkers=1 --minWorkers=1(247/247)pnpm --filter @optimitron/web run typecheck:fastscripts/mcp-personal-task-engine-smoke.tsgit diff --checkThe broad local parallel Vitest run exposed a pre-existing collision between task-funding test cleanup prefixes; the untouched affected test files pass serially. CI remains the clean full-suite check for this PR.
Summary by CodeRabbit