Repository navigation
mcpx: one durable task store across MCP and /v1 — Phase 1 of #1554 (#1555) - #1575
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 32 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d659653966
ℹ️ 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".
| // MCPServer() (issue #1555). Set here, where the daemon server | ||
| // exists in this process; commands that never build one leave it | ||
| // nil and their MCP servers keep private stores. | ||
| a.daemonStore = srv.TaskStore() |
There was a problem hiding this comment.
During takeover, this opens the task database before srv.TakeOver() transfers ownership and drains the predecessor's HTTP requests. tasks.Open() snapshots the rows and rewrites nonterminal tasks as failed, so a task completed by the predecessor between this initialization and handover remains incorrectly failed in the successor; tasks created during that interval are missing from its store entirely. An ordinary duplicate daemon startup also modifies the database before Listen() rejects it. Defer loading the store until the predecessor has stopped updating it.
Useful? React with 👍 / 👎.
| if s.TaskStore != nil { | ||
| // Injected: the daemon's one store, already configured | ||
| // (PollInterval included) by whoever built it. | ||
| s.taskStore = s.TaskStore |
There was a problem hiding this comment.
After a daemon restart, the injected store restores MCP tasks but the server's taskOwners map is empty. visible() treats a missing owner as permission to access the task by ID, so a legacy task previously restricted to its originating connection becomes readable through tasks/get and tasks/result by any other connection that knows its ID. This bypasses the connection isolation explicitly enforced in requirements_test.go's another-connection-cannot-list-or-read-it test. Persist the ownership restriction alongside durable tasks, or otherwise deny foreign connections access to restored connection-owned tasks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f29b78f6d0
ℹ️ 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".
| if a.daemonStore != nil { | ||
| srv.TaskStore = a.daemonStore |
There was a problem hiding this comment.
On the daemon's MCP path, this assignment is discarded before the server is returned: App.MCPServer() appends a.v1OpTools() and then calls srv.WithExtras(extras), whose new Server literal in internal/mcpserver/server.go does not copy TaskStore. Consequently, the daemon-served MCP server still creates a private in-memory store, so MCP tasks remain invisible through /v1 and disappear on restart. Preserve TaskStore in WithExtras, or perform the injection after that decoration; the new tests inject directly into an undecorated server and therefore miss this production path.
Useful? React with 👍 / 👎.
…1555) Phase 1 of #1554 (triage: queue task 1609). mcpserver.Server gains an exported TaskStore field, set in App.MCPServer() alongside Notify/Passthrough/Timing when the App runs the daemon (root.go wires daemon.Server.TaskStore(), a new exported accessor for the existing sync.Once store). When injected, tasks() uses that store and never creates its own; standalone servers (adapter, openapi, stdio shim) keep the lazy in-memory behaviour. PollInterval: the shared store keeps the daemon construction value from protoTasks.pollInterval -- the same setting Timing.TaskPoll reads -- so neither surface cadence changes. Retires conformance gap ext-tasks-durable-before-response. Tests: injected store is the MCP surface's store (cross-surface both directions, ownerless /v1 tasks unlisted by MCP), MCP tasks survive store reopen with working-task reaping, daemon accessor identity.
…s/list in the shared-store test, conformance coverage for the retired gap treefmt-check found gofmt drift in cli/commands.go (import order) and mcpserver/server.go (field alignment). The shared-store test listed via the 2026-07-28 extension, which removed tasks/list; it now lists through the legacy handler with a legacy-started task on a second server sharing the store (legacy list requires an owner match; modern-started tasks are ownerless by design). Retiring the ext-tasks-durable-before-response gap left its matrix cell uncovered; coverage now names TestInjectedStoreKeepsMCPTasksAcrossReopen@2026-07-28 and the matrix doc is regenerated. mcpserver + conformance + cli + daemon packages pass on the laptop.
d05af42 to
516df39
Compare
Phase 1 of #1554 for #1555, implemented per the triage verdict in queue task 1609 (this PR is queue task 1642).
What
One
tasks.Storeinstance now serves both surfaces in the daemon:mcpserver.Servergains an exportedTaskStorefield — the wiring style this codebase already uses (Notify,Passthrough,Timingare exported fields set post-construction inApp.MCPServer(); there are no setters). When set,tasks()uses it and never lazily creates its own; when nil, today's lazy in-memory behaviour is byte-for-byte unchanged.daemon.Servergains an exportedTaskStore()accessor returning the existingsync.OncetaskStore()instance (SQLite at<state>/tasks.db, in-memory fallback) — exactly one store per daemon;/v1handlers keep using the unexported path.internal/cli/root.go, where the daemon server andlazyMCPare built in one process, setsa.daemonStore = srv.TaskStore();App.MCPServer()injects it into the daemon-served MCP server alongsideNotify/Passthrough/Timing.ext-tasks-durable-before-responseretired (internal/conformance/coverage_stateful.go) — its reason ("the task store is an in-memory map; a task does not survive the daemon") is no longer true for the daemon's MCP surface.PollInterval — the one named source
protoTasks.pollIntervalwon, and neither surface's cadence changes. It was already the single named source for both: the daemon store reads it directly at construction (ops.go), andApp.MCPServer()already fed the MCP side'sTiming.TaskPollfrom the same key. The shared store is configured once, at daemon construction, fromprotoTasks.pollInterval;Timing.resolved().TaskPollremains only on the standalone lazy-creation path.Standalone disposition (triage's open plumbing question)
Injection happens only on the daemon-served
/mcppath.CmdServe's stdio shim,adapter.goandopenapi.goconstructions never havea.daemonStoreset (root.go's daemon-run path is the only place it is assigned), so they keep a private in-memory store with unchanged behaviour — there is no second surface to share with in those processes.Tests (new)
mcpserver/tasks_shared_store_test.go: the injected store is the store the MCP surface uses — an MCP-started task isGet-visible in the shared instance under the same id (the/v1read path); an ownerless store-started (/v1) task is not listed by any MCP connection; an MCP-started terminal task survives store close/reopen from the sametasks.db, and a task working at close reloads asFailed("daemon restarted before task completion").daemon/taskstore_shared_test.go:TaskStore()returns the one instance.Verification is CI per fleet policy (
cd pkgs/mcpx && go test ./...runs in theidworkflow).Refs #1555, #1554. Origin: dezren39/darwin-nix#209 TASK-27/TASK-20.
Need help on this PR? Tag
@codesmith-botwith what you need.