feat(contracts): carry typed problem detail beside the message - #2259
Conversation
The application problem carried only a code and a message, so producers flattened structured facts into the message: the park cause and remedy, the stale refresh frontier, and a writer lock's missed deadline. ApplicationProblemDetailV1 is now the one authority for those facts. A problem built from a detail derives its kind, code, retry, legal actions and message from it, and the record serializes the detail on every surface: MCP structured content, CLI JSON and labelled lines, and HTTP. A source-edit lock deadline is now retryable saturation with its lock class, not a permanent execution failure that leaked the lock path.
|
![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: 48807b1c8b
ℹ️ 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 let Some(detail) = self.detail() | ||
| && *self != Self::from_detail(detail.clone()) |
There was a problem hiding this comment.
When detail is None, this validation skips detail identity enforcement entirely. As a result, a generic Unavailable, Stale, or Saturated problem can use a reserved code such as application.code-index.parked with detail: null and still pass both construction and wire deserialization, silently dropping the typed facts clients rely on. Reject detail-owned codes unless their matching detail is present.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
| (problem.detail !== null && | ||
| !(typeof problem.kind === "string" && detailKinds.includes(problem.kind))) || |
There was a problem hiding this comment.
The SDK only checks that any non-null detail accompanies one of three broad problem kinds, so it accepts combinations Rust rejects, such as a lock_deadline detail on an unavailable problem or a parked detail on stale; it likewise never verifies the detail-derived code, message, retry, or legal actions. Such malformed HTTP responses are exposed as valid typed problems instead of raising TraceDecayMalformedResponseError. Validate each detail variant against its full canonical problem tuple.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
| let response = tracedecay_daemon_protocol::DaemonInvocationResponse::application_problem( | ||
| &request.request_id, | ||
| self.problem.clone(), | ||
| ); |
There was a problem hiding this comment.
This executor returns a preconstructed ApplicationProblem directly, so the new test never exercises the production DaemonSessionRefreshService → SessionRefreshOutcome::StaleFrontier → begin_result/effect_error path that is supposed to preserve requested, committed, and active. A regression in that wiring therefore remains green; create the stale store/service state and invoke the production journey instead of injecting its expected result.
AGENTS.md reference: AGENTS.md:L172-L177
Useful? React with 👍 / 👎.
| if let Some(detail) = &problem.detail { | ||
| for (label, value) in detail.labelled_fields() { | ||
| self.text(label, value); |
There was a problem hiding this comment.
When a parked cause or remedy contains control characters, for example from an error mentioning a repository path, message() folds them but labelled_fields() returns the original raw strings. Passing those strings directly to the human view emits controls such as ESC in CLI/MCP text because the Markdown renderer only replaces CR/LF and Markdown punctuation, allowing terminal-control injection and bypassing the safety guaranteed by SafeDiagnostic; fold or validate these values before rendering.
Useful? React with 👍 / 👎.
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. |
…m-typed-detail # Conflicts: # crates/tracedecay-contracts/src/lib.rs
1 participant
What was wrong
The problem envelope the daemon sends to clients (
ApplicationProblem, and theApplicationProblemRecordevery surface serializes) had only a code and a message. So producers wrote their facts into the message string:"The code index for this worktree is parked; remedy: …; cause: …""The refresh window no longer contains the committed projection frontier 80; …""sync lock: source edit writer lock at /abs/path stayed busy …". On the source-edit path this also landed as a permanentexecution_failed/ contact administrator, even though the lock doc calls it a retryable lock-deadline error, and it exposed the lock's filesystem path.A CLI, MCP, HTTP, or SDK client that wanted the cause had to parse English.
What changed
The shape is unreleased (the typed envelope shipped this week), so it changes in place. There is no V2 and no alias.
ApplicationProblemDetailV1(schemars, intracedecay-contracts) is a tagged enum and the only authority for these facts:parked { cause, remedy, retries_on_wake }stale_refresh_frontier { requested, committed, active }lock_deadline { resource, deadline_ms }ApplicationProblem::from_detail(detail)is the only constructor for a detailed problem. The detail determines the kind, code, retry, legal actions, and message:detail.message()is the only message rendering.validate()rejects any problem whose fields differ fromfrom_detail(detail), so a hand-formatted message or a detail grafted onto another kind fails closed.detail.labelled_fields()supplies the labelled lines for MCP and CLI text output.problem.detailis always serialized, and isnullwhen the problem has no detail, following the same required-nullable rule ascommitted_receipt. It reaches MCPstructuredContent.problem, the CLI--jsonenvelope, the HTTP problem body, and the TS SDK record, which validates it.OwnedPrimitiveRuntime::parked_refusal)TraceDecayError::LockDeadline { resource, deadline_ms }replacesSyncLockat the two deadline sites (source-edit writer lock, response-handle writer lock). Source-edit and retainedmap_execution_errornow map it to the detailed saturated problem. The handle-store busy classification (MCP truncation status, LSPfeedback-cycle-handle-store-busy) keys on the typed variant.tracedecay_search's unavailable output carries the samedetailfor a terminal park and renders its failure message from it. The old hand-formatted"parked: …; remedy: …"string is gone.dashboard/src/contractsand the TS SDK types;contracts:checkis green.Per-surface literals (parked, through the shipped daemon)
crates/tracedecay/tests/daemon_suite/code_index_park_test.rsmakes a committed file unreadable, waits for the park, then asserts{"kind":"parked","cause":CAUSE,"remedy":REMEDY,"retries_on_wake":false}on each surface, plus the unchanged message text:tracedecay_searchJSONdetailproblem.detailandstructuredContent.problem.detailtool code_symbol_search --jsonproblem.detailandmessage- Parked cause: …and- Retries on wake: falsePOST /projects/{id}/application/code/code_symbol_search: status 503, andvalue.problem.detail/messageFails on master: I ran the same test binary with
TRACEDECAY_TEST_BINpointing at anorigin/master(08a6807) daemon build. It fails at the first detail assertion withleft: Nulland passes against this branch.Other problems:
retained_timeout_dispatch_tests::a_stale_refresh_frontier_reaches_mcp_as_typed_detailsends the daemon's problem through a serde round trip of the daemon protocol, then through the production MCP retained adapter. It asserts the literal{kind: stale_refresh_frontier, requested: 120, committed: 40, active: 80}, the unchanged message, and the markdown labelled lines.session-runtimea_stale_begin_frontier_is_a_stale_problem_naming_the_committed_frontierpins the same detail on the begin projection.invocation::source_edit::tests::a_missed_writer_lock_deadline_is_retryable_capacity_with_typed_detailasserts the daemon wire:saturated/after_delay/["retry"], the rendered message, and{kind: lock_deadline, resource: "source-edit writer lock", deadline_ms: 30000}. On master the same error wasexecution_failedwith the path in the message.a_detailed_problem_carries_its_facts_beside_the_rendered_messagecovers the round trip, a restated detail rejected against its message, an omitteddetailrejected, and a detail grafted ontoinvalid_requestrejected.surfaces a problem's typed detail and refuses one on a kind without detail.Runtime journey (debug CLI build from this branch)
Isolated
HOME/TRACEDECAY_DATA_DIR; one daemon undersystemd-run --user --scope -p MemoryMax=6G -p MemorySwapMax=1G;src/model.rsset to mode 000 and committed.Local verification
tracedecay-contracts420tracedecay-daemon-protocol65tracedecay-dashboard-api173 (2 ignored)tracedecay-mcp391tracedecay-application476tracedecay-session-memory303tracedecay-session-runtime122tracedecay-source-edit88tracedecay-domain222tracedecay-api53tracedecay-daemon-service320 passed, 1 failed. The failure isadoption_observation::…census…, which also fails on master and is filed as test(daemon-service): adoption census expects 34 retrieval capabilities, catalog has 41 #2246.tracedecaylibdaemon::+mcp::tools::handlers::: 624/624. After the final rebase, 620 passed in the parallel run and the other 4 were load flakes that passed sequentially.daemon_suitepark test: 1 passedsdk_suite: 21 passedcore_cli_suite: 139/139. The first run had 134 pass. Of the 5 failures, one was a missing example fixture and four were load-timing flakes, and all 5 passed when rerun sequentially under the cap.mcp_suite: 136 passed, covering every module whose fixtures changed plus every test that failed in the first attempt. The first pass had 105 pass; its 31 failures all timed out in the harness's 20s code-index readiness wait under load, and all 31 passed sequentially. The full 569-test suite was OOM-killed twice by the 6G scope cap, at about 139 tests each time, so it has not completed under the cap.typecheck;contracts:checktypecheckcargo clippy --all-targets -D warnings: clean with and withouttest-transportcargo fmt --all -- --check: cleanLeft open
GithubSourceUnavailable { state, remedy }(fix(configuration): provision the GitHub origin source binding #2221) is not added. No problem producer flattens it: the GitHub source state and remedy only travel as the typedgithub_sourcestatus field. A variant with no producer would be unreachable.IndexFreshness, already reads cause and remedy from the typedCodeIndexConvergenceParkedV1status, and no dashboard route receives a parked application problem. The change there is regenerated contracts plusdetail: nullin the hand-built record fixtures.ProjectRouteerrors, so their MCP and CLI output carries the rendered message but notdetailorlegal_actions: source-edit and graph-tool adapters drop the typed problem record #2255.