refactor(mcp): run affected tests and the dashboard through the owner - #2310
Conversation
The graph-tool owner's catalog gains a typed side-effect entry kind. The tool-catalog operation's OwnerSideEffectEntryV1 names what the call does (EffectClass::SpawnsProcess or BindsServer), its dispatch ceiling (the long-running ten-minute ceiling for a test run, the interactive ceiling for the dashboard), and that identical concurrent calls each run. The capability manifest takes its effect class, cancellation, and deadline from that entry, settles within the call (operation receipt, no idempotency key), and the MCP dispatch contract, the dispatch ceiling, and the identical-read coalescer all read it. The tools migrated earlier stay read entries. tracedecay_run_affected_tests and tracedecay_dashboard are owner-served operations with typed requests and results. An unknown key, a mistyped changed path, an unknown profile, or an out-of-range port is refused instead of being ignored or silently defaulted. The dashboard's daemon-owned readers, writers, and session authorities reach it through the owner's existing tool context. The session-workflow dispatch group, its binding row, and the name-keyed effect, cancellation, and ceiling rules for both tools are deleted.
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbbc8aea59
ℹ️ 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".
| | ApplicationSurfaceOperation::RunAffectedTests | ||
| | ApplicationSurfaceOperation::Dashboard => match value { |
There was a problem hiding this comment.
Preserve authoritative settlement for the migrated side effects
When either newly routed operation is cancelled or reaches its deadline after dispatch to the daemon, invoke_application_surface consults application_surface_cancellation_policy, but that match has no arm for RunAffectedTests or Dashboard and therefore classifies both as ReadOnly. The controlled client consequently returns a DuringRead cancellation immediately instead of waiting for effect settlement; a dashboard bind can complete after the caller was told it was cancelled, and an affected-test run can lose its terminal receipt. Classify these operations as AuthoritativeEffect (preferably from their catalog effect metadata) so cancellation cannot abandon an in-flight side effect.
AGENTS.md reference: AGENTS.md:L172-L177
Useful? React with 👍 / 👎.
tracedecay_run_affected_testsandtracedecay_dashboardnow run through the project's graph-tool owner, on MCP,tracedecay tool, and thetracedecay dashboardCLI (which reaches the daemon over MCP). To support that, the owner's catalog gains a typed entry kind for operations that act beyond reading. It follows #2296 and #2284.The side-effect entry kind (
tracedecay-tool-catalog, the single authority)EffectClassgainsspawns_processandbinds_server.MutatesProjectwas left out because no migrated operation needs it.owner_side_effect_spec(op). The spec takes its effect class, deadline, cancellation (spawns_process: cooperativeBeforeAdmission+EffectInFlight;binds_server: not cancellable) and stateless lifecycle from the entry. The read specs are unchanged.effectisspawns_process/binds_serverwithread_only: false;tool_dispatch_ceiling;tool_allows_identical_read_coalescing, which returnsfalseforRunEach.LONG_RUNNING_TOOL_DISPATCH_CEILINGis now defined from the catalog constant.Migration
Typed requests/results: in
crates/tracedecay-contracts/src/retrieval/owner_effect_surface.rs. They serialize to the JSON these tools already emitted.Now refused instead of ignored or defaulted:
changed_paths(now a required field);profile;port(previously it silently became 7341);action.Kept in-band:
max_tests/timeout_secsbounds violations and "no tests cover" remain in-band typed results, as before.Owner dispatch: the root
compute_graph_tool_for_ownerruns both under the owner's admitted authorities and the catalog ceiling.Dashboard dependencies: the dashboard's daemon-owned readers, writers and session authorities now reach it through the owner's existing
ToolCallRegistryOptions, not a second context type:All of them already lived on the daemon's
McpServer; before this change they were passed only on the in-client dispatch path.Deleted: the in-client paths are gone, as are the
SessionWorkflowbinding row and dispatch group, and the name-keyed effect, verified-journey, cancellation and long-running-ceiling rules for both tools. The compat symbols themselves are untouched.Fail-before / pass-after (production MCP,
harness.call_tool)These test files were run on unchanged
origin/master(697755b):With this change, all pass:
run_affected_tests_reports_the_cargo_result_for_the_changed_manifest. The realcargo testrun returns the literal outcome:exit_code0,passed1,dispatched_tests["tests::greeting_is_hello_world"],results[{"test": …, "passed": true, "covers_source_ids": […]}]. It also covers the failing and truncated runs, plus typed refusals forfilter,profile: "bench",changed_paths: "src/lib.rs"and[…, 7].tracedecay_dashboard_tool_starts_and_returns_url_and_serves_capabilities: the result is exactly{"status":"started","url":"http://127.0.0.1:{port}/","host":"127.0.0.1","port":port}, and a GET of/api/capabilitiesreturns 200tracedecay-dashboard.identical_concurrent_test_runs_each_run_while_identical_reads_still_ride: two identical concurrentrun_affected_testscalls return two differentterminal.operation_ids, and the identical-read coalescer's leaders+followers counters do not move. Two identical concurrenttracedecay_filesreads add exactly 2 to those counters. This test also passes on master, where both tools were alreadyadministrative. It guards the rule rather than failing before.Runtime journey (debug
tracedecayfrom this branch, isolated HOME/profile, one daemon undersystemd-run --user --scope -p MemoryMax=6G -p MemorySwapMax=1G)Run before the final rebase. After rebasing onto 21552e3, refusals render as
isErrorproblem results instead of JSON-RPC errors; the tests assert that newer shape.Checks (local)
mcp_suite(before the final rebases): 558 passed, 24 failed. The 24 split as follows:diff_context_large_response…, which timed out under host load; it passed on the rerun.mcp_suite -- affected dashboard schema_test protocol_test graph_query_test mcp_cli_serve_test: 113 passed.--lib -- mcp::: 183 passed.tracedecay-mcp389 (one session-workflow test deleted with its group),tracedecay-tool-catalog6 +tool_catalog_suite22, mcp-catalog 29, contracts suite 277, daemon-protocol 65.mcp_suite -- affected dashboard: 9 of this change's tests pass. The one failure isautomation_run_list_refuses_a_non_directory_dashboard_root, which is red on master because 21552e3 changed how owner refusals render.schema_test::schema_required_arguments_match_representative_handler_parsersis red on master for the same reason.cargo check --all-targets(both crates,test-transport): clean.-D warningsovertracedecay tracedecay-mcp tracedecay-contracts tracedecay-daemon-protocol tracedecay-api tracedecay-mcp-catalog tracedecay-tool-catalog tracedecay-cli --all-targets, with and withouttest-transport, was clean on the pre-rebase head. On current master every crate tripsclippy::result_large_err, 298 fn and 71 closure sites, becauseTraceDecayErrorgrew in 21552e3. That also reproduces on an unmodified master checkout; filed as fix(domain): TraceDecayError too large for clippy since 21552e3764 #2308. None of those sites is code this change added.cargo fmt --all -- --check: clean.pnpm run contracts:generatethencontracts:check: up to date. The SDK and the dashboardEffectClassenum were regenerated.pnpm run typecheck(dashboard): clean.Cargo.lockstill pins1.0.0-beta.55against thebeta.56manifest. I left it alone.