refactor(mcp): move git tool handlers behind one tool context - #1142
Merged
ScriptedAlchemy merged 13 commits intoSep 9, 2026
Merged
Conversation
The git tool family (affected, diff/commit/changelog/PR context, branch list/search/diff, its shell reads and PR-context cursor) plus the dependency-hint adapters and the retrieval-cursor decoder now live in `tracedecay-mcp`. They no longer take a `TraceDecay` handle or a per-handler pile of authority parameters: every daemon-owned input crosses one boundary, `McpToolContext`, which carries the admitted worktree route and active branch, the caller's deadline and cancellation, the registered project session store that authenticates PR-context cursors, and the code-index search/branch-diff executors with the authorization proved for them. An authority the daemon did not admit stays a typed `None` in the context, so a standalone server reports each handler's own unavailable state instead of minting a substitute. The composition root keeps only context construction and dispatch, and the moved code carries no compatibility reexport in either direction. Promotes `gix` to a workspace dependency so both crates share the pinned version and feature set.
…plan-reopened' into sol/mcp-handlers-1073
McpToolContext accepted the project root, session store, code-index executors and search authority as independent public arguments, so a caller could bind a store from one project alongside executors admitted for another and nothing would notice. Replace that surface with a single McpToolBinding that carries the admitted project store and code index together with their resolved scopes, and validate the whole set in McpToolContext::bind: the root must be absolute, the store and index scopes must agree with the request scope, and an admitted index must carry both its authority and its executors. Mismatches return a typed McpToolBindingError instead of silently dropping an authority. Graph queries only materialize inside a handler, so verify_graph_scope checks a query's checkouts against the admitted scope at use time, and dependency hints now take the bound context rather than parallel deadline and cancellation parameters.
The pr_context cursor hashed Path::to_string_lossy() for its root identity, so two distinct roots whose non-UTF8 bytes lossily encode to the same string produced interchangeable cursors, and decoding asserted ValidatedAuthorization::Authorized locally instead of taking the authorization the request was admitted under. PrContextCursorBinding now carries the canonical resolved project, repository and worktree identity alongside the root's native OS bytes from tracedecay_runtime_core::os_str_bytes, and the cursor authority comes from the bound tool context's authorized project session db. The binding hashes into two digests so a foreign project, store or root is refused as denied while a moved diff comparison is refused as invalid.
…plan-reopened' into sol/mcp-handlers-1073
…plan-reopened' into sol/mcp-handlers-1073
McpToolContext::bind trusted the labels its caller attached: a store lease and code-index executors each arrived with a scope of their own, so any public caller could present one project's authority beside another project's scope, and authorized_project_session_db() minted Authorized for whatever lease it held. A request that resolved no checkout also waved a verified graph through, because absence of an admitted scope was read as nothing to isolate against. The binding now carries exactly one scope — the checkout project open resolved for the serving route, published from the daemon through server construction into tool dispatch. Scoped authorities are admitted under it instead of relabelling it: AdmittedProjectStore and AdmittedCodeIndex have private fields and fallible constructors, a store lease is checked against the logical shard the registry opened it for, and a code-index admission must carry both the read admission envelope it authenticates and at least one executor. Authorization is the root's verdict, carried through untouched, so an unauthorized store denies at the PR-context cursor authority rather than reporting a missing capability. A graph query with no admitted scope is refused, and the dead standalone constructor is gone. PR-context cursor identity drops the reference-sensitive scope digest for the three fields identifies_same_checkout compares, plus the signing store's own registered shard, so pagination survives an ordinary branch switch while a foreign checkout, root, or store stays denied.
…plan-reopened' into sol/mcp-handlers-1073
The admitted store lease was checked with StoreShardScopeV1::project_id(), which reports Project, ProjectSessions, and Code shards alike. A lease on this very project's project store or one of its code stores therefore passed as project-session authority: the project ids matched, and the family never entered the comparison. Those are separate stores with their own tables and retention. The family is now matched exactly, and exhaustively, so a shard family added later must be classified rather than inherit an answer. A lease that is not the admitted project's session shard is refused as mcp_tool_binding_store_not_session_shard, which also subsumes the profile and remote-node shards that carry no project at all. The PR-context cursor's bound store identity had the same flattening: it reduced the signing shard to the project it mentioned, so one project's session, project, and code shards produced a single store identity and their cursors verified interchangeably. It now carries the shard's logical scope whole through the canonical serialization, which keeps a foreign store denied rather than merely stale.
…plan-reopened' into sol/mcp-handlers-1073
|
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
tracedecay/crates/tracedecay-mcp/src/handlers/git/context.rs
Lines 109 to 110 in 707d451
Cancel blocking Git workers after dispatch timeouts
When tracedecay_changelog or tracedecay_commit_context exceeds its carried or universal dispatch deadline, tokio::time::timeout drops this future, but dropping a spawn_blocking join handle does not stop a task that has already started. The tree diff, status scan, or rev-walk therefore continues detached; repeated timed-out requests against a large or pathological repository can accumulate filesystem work and exhaust the blocking pool even though every caller has already received a timeout. Route these operations through a cancellation-aware worker like the controlled PR-context path and cover the timeout/drop case.
AGENTS.md reference: AGENTS.md:L154-L156
ℹ️ 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".
Contributor
Performance Comparison
|
ScriptedAlchemy
merged commit Sep 9, 2026
434b511
into
codex/tracedecay-total-redesign-plan-reopened
11 of 13 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Child of #707 for #1073 (composition root).
Moves the git MCP tool handlers out of the root crate into
tracedecay-mcpbehind a singleMcpToolContextthat binds all admitted authorities in one step. Review of that binding surfaced three authority-proof defects, fixed here with RED/GREEN tests:McpToolContext::bindnow proves authority coherence for real (project, worktree, store lease) instead of accepting matching-looking inputs; the dead shippedMcpToolContext::standaloneis removed.StoreShardScopeV1::ProjectSessionsfamily exactly and exhaustively. PreviouslyStoreShardScopeV1::project_id()reportsProject,ProjectSessions, andCodealike, so a lease on the admitted project's own project store or a code store passed as session authority. Refusal ismcp_tool_binding_store_not_session_shard; the old "not project scoped" variant is gone.scope_digestis no longer part of the identity.RED evidence (guards reverted):
only_the_project_sessions_family_carries_session_authority—Codeshard returnsSome(project.admitted)whereNoneis required;a_cursor_cannot_travel_between_shard_families_of_one_project— session and project shards hash to the same digest. GREEN: 254/254.Verification:
tracedecay-mcp --lib254 passed; rootmcp::380 passed; clippy-D warningson both crates--all-targets; fmt; commitlint. Merged #707 atd56a8a1d2.Not in scope, flagged for its owner (
fable/reset-idempotency-1130):production_codex_hook_ingest_survives_message_search_reopenfails in observation ingest becausecrates/tracedecay-rusqlite-runtime/src/repository/observation/authority.rswraps a provenance collision intorusqlite::Error::InvalidParameterName, surfacing a real collision as retryable infrastructure failure.