Make MCP responses bounded and agent-native - #233
Conversation
|
@codex review please |
|
Warning Review limit reached
Next review available in: 44 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 ignored due to path filters (5)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds MCP capability discovery, structured output schemas, compact/full receipts, summary/full diagnostic responses, semantic transition evidence tracking, response validation before writes, and CLI v1 compatibility projections. It also updates integration, unit, and playback coverage for these contracts and flows. ChangesMCP contracts and output schemas
Diagnostics and repository observations
Compatibility and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 5a5a4c22ab
ℹ️ 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".
Review repair activity summary
The exact inline thread has been answered and resolved. The backlog-card retirement was also followed by generated DAG repair @codex review please — please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1f5f14d38
ℹ️ 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".
| }, | ||
| { | ||
| family: "diagnostic", | ||
| openingCall: "doctor", |
There was a problem hiding this comment.
In an unbound daemon session, capabilities is callable before workspace binding, but this advertises doctor as the diagnostic openingCall; server-tool-access.ts only allows the daemonAlwaysAvailableTools set through before binding and then throws WorkspaceBindingRequiredError when !isBound, and doctor is not in that set. An agent following the advertised opener from daemon capability discovery will immediately fail until it binds a workspace, so use an always-available diagnostic opener such as causal_status/explain, or make summary doctor callable while unbound.
Useful? React with 👍 / 👎.
| const inputSchema = { ...(def.schema ?? {}), ...COMMON_TOOL_INPUT_SCHEMA }; | ||
| const zodSchema = toolInputSchema(def.schema); |
There was a problem hiding this comment.
When an MCP client omits arguments for tools whose only inputs are optional (stats, workspace_status, capabilities, or default doctor), these lines still register a non-optional object schema because every tool now gets the optional receipt control. MCP's schema reference allows CallToolRequest.params.arguments to be omitted, and that undefined is rejected against the registered object schema before the handler runs; previously tools without schemas used the zero-arg registration path and worked. Normalize missing args to {} or avoid adding an input schema for truly no-input tools.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/integration/mcp/server.test.ts (1)
95-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated structured-content/schema-validation helper across two integration test files. Both files reimplement the same "parse
structuredContent, assert it matches the text payload, validate againstMCP_OUTPUT_SCHEMAS[tool]" logic; the root cause is the absence of one shared, exported helper.
test/integration/mcp/server.test.ts#L95-L109: keep (or move) the genericstructuredPayload(result, tool: McpToolName)here as the canonical implementation, e.g. relocated intotest/helpers/mcp.tsalongsideextractTextso both integration suites can import it.test/integration/mcp/daemon-bridge.test.ts#L18-L36: replacestructuredSafeReadandstructuredCapabilitieswith calls to the shared generic helper (structuredPayload(result, "safe_read")/structuredPayload(result, "capabilities")) instead of maintaining two hardcoded copies.🤖 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 `@test/integration/mcp/server.test.ts` around lines 95 - 109, The structured-content and schema-validation logic is duplicated across the MCP integration tests. In test/integration/mcp/server.test.ts lines 95-109, move or retain structuredPayload(result, tool) as the shared exported helper, preferably alongside extractText in test/helpers/mcp.ts; in test/integration/mcp/daemon-bridge.test.ts lines 18-36, remove structuredSafeRead and structuredCapabilities and replace their uses with structuredPayload(result, "safe_read") and structuredPayload(result, "capabilities").src/cli/peer-command.ts (1)
115-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded detail-mode tool list is a future sync hazard.
requiresFullDetailhardcodes"doctor"/"activity_view"as the only tools needingdetail: "full"for CLI v1 compatibility. Correct today, but if another diagnostic tool later gains a bounded summary/full split (matching the "diagnostic" family inmcp-capability-discovery.ts), it's easy to forget updating this list, silently degrading that tool's CLI peer output to summary-only data.Consider deriving this list from a single shared, exported constant (colocated with the tools that implement the detail split) rather than duplicating tool names here.
🤖 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 `@src/cli/peer-command.ts` around lines 115 - 120, Replace the locally hardcoded requiresFullDetail tool-name check in the peer command with a shared exported constant defined alongside the diagnostic tools’ detail-mode implementations, such as the source in mcp-capability-discovery.ts. Use that constant to determine whether to send detail: "full", preserving the existing receipt behavior and CLI compatibility.src/contracts/output-schemas.ts (1)
223-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated contract definitions risk silent drift. Receipts, doctor, activity-view, semantic-transition, and
mcpOutputBodySchemasare defined here and again insrc/contracts/output-schema-fragments.ts/src/contracts/output-schema-mcp.ts. Thecode_finddivergence flagged above is a concrete instance of this class of drift. Consider having one module import from the other (or a shared fragment) rather than re-declaring, so a change can't land in one copy only.🤖 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 `@src/contracts/output-schemas.ts` around lines 223 - 268, Consolidate the receipt schema definitions in output-schemas.ts with their duplicates in output-schema-fragments.ts and output-schema-mcp.ts so there is a single source of truth. Update the affected imports and consumers around fullReceiptSchema, compactReceiptSchema, receiptSchema, and legacyCliReceiptSchema to reuse shared definitions, preserving the existing validation behavior and public exports.src/mcp/server-tool-access.ts (1)
120-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFall back to text-block parsing when
structuredContentis present but invalid.If
result.structuredContentis defined buttryParseJsonObjectfails to validate it (e.g. non-object structured content from some other MCP caller), the function returnsnullimmediately instead of falling back to parsing thecontenttext block, which may still contain valid, parseable JSON. This is a narrow regression versus the prior behavior which always attempted the text-block parse.♻️ Proposed fix
export function parseToolPayload(result: McpToolResult): JsonObject | null { - if (result.structuredContent !== undefined) { - return tryParseJsonObject(result.structuredContent); - } + if (result.structuredContent !== undefined) { + const structured = tryParseJsonObject(result.structuredContent); + if (structured !== null) { + return structured; + } + } const textBlock = result.content.find((entry) => entry.type === "text"); if (textBlock === undefined) { return null; } try { return parseJsonTextObject(textBlock.text, "MCP tool result"); } catch { return null; } }🤖 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 `@src/mcp/server-tool-access.ts` around lines 120 - 133, Update parseToolPayload so an invalid structuredContent result from tryParseJsonObject does not return immediately; when validation fails, continue to the existing text-block lookup and parseJsonTextObject fallback, preserving the null result when no valid text-block JSON is available.
🤖 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 `@src/contracts/output-schemas.ts`:
- Around line 1278-1296: Update the code_find schema in output-schema-mcp.ts to
match the union defined in output-schemas.ts, including the
structuralRefusalSchema extension with projection set to "refused" alongside the
existing strict object shape. Keep both validation paths synchronized so refused
code_find bodies are accepted consistently.
In `@src/mcp/persisted-local-history.ts`:
- Line 757: Update readTransitionEventFromGraph() so legacy transition nodes
missing observationBasis receive the schema-compatible default or migration
value before transitionEventSchema.safeParse() runs. Preserve the existing value
for nodes that provide observationBasis, ensuring older persisted events remain
in state.transitionEvents.
In `@src/mcp/tools/doctor.ts`:
- Around line 94-124: Update the summary health calculation in the detail ===
"summary" branch so the permanently seeded
"structural_history_readiness_unknown" evidence remains an informational
degradedReasons entry but does not force health to "degraded"; derive health
from the other actionable degradation reasons while preserving the existing
response shape and deduplication.
---
Nitpick comments:
In `@src/cli/peer-command.ts`:
- Around line 115-120: Replace the locally hardcoded requiresFullDetail
tool-name check in the peer command with a shared exported constant defined
alongside the diagnostic tools’ detail-mode implementations, such as the source
in mcp-capability-discovery.ts. Use that constant to determine whether to send
detail: "full", preserving the existing receipt behavior and CLI compatibility.
In `@src/contracts/output-schemas.ts`:
- Around line 223-268: Consolidate the receipt schema definitions in
output-schemas.ts with their duplicates in output-schema-fragments.ts and
output-schema-mcp.ts so there is a single source of truth. Update the affected
imports and consumers around fullReceiptSchema, compactReceiptSchema,
receiptSchema, and legacyCliReceiptSchema to reuse shared definitions,
preserving the existing validation behavior and public exports.
In `@src/mcp/server-tool-access.ts`:
- Around line 120-133: Update parseToolPayload so an invalid structuredContent
result from tryParseJsonObject does not return immediately; when validation
fails, continue to the existing text-block lookup and parseJsonTextObject
fallback, preserving the null result when no valid text-block JSON is available.
In `@test/integration/mcp/server.test.ts`:
- Around line 95-109: The structured-content and schema-validation logic is
duplicated across the MCP integration tests. In
test/integration/mcp/server.test.ts lines 95-109, move or retain
structuredPayload(result, tool) as the shared exported helper, preferably
alongside extractText in test/helpers/mcp.ts; in
test/integration/mcp/daemon-bridge.test.ts lines 18-36, remove
structuredSafeRead and structuredCapabilities and replace their uses with
structuredPayload(result, "safe_read") and structuredPayload(result,
"capabilities").
🪄 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
Run ID: 52a03186-8bf0-4be1-88a9-2a1a0ca57d9f
⛔ Files ignored due to path filters (21)
CHANGELOG.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mddocs/MCP.mdis excluded by!**/*.mddocs/SETUP.mdis excluded by!**/*.mddocs/TECHNICAL_TEARDOWN.mdis excluded by!**/*.mddocs/design/SURFACE_agent-working-set-control-plane.mdis excluded by!**/*.mddocs/design/WARP_local-causal-history-graph-schema.mdis excluded by!**/*.mddocs/design/versioned-json-output-schemas.mdis excluded by!**/*.mddocs/feedback/2026-07-14-codex-agent-native-campaigns.mdis excluded by!**/*.mddocs/invariants/versioned-output-schemas.mdis excluded by!**/*.mddocs/method/backlog/bad-code/CLEAN_mcp-output-schema-authority-duplicated-and-drifted.mdis excluded by!**/*.mddocs/method/backlog/bad-code/CLEAN_method-drift-can-silently-pass-with-zero-playback-questions.mdis excluded by!**/*.mddocs/method/backlog/bad-code/CLEAN_mutating-tools-need-prepared-response-contract.mdis excluded by!**/*.mddocs/method/backlog/cool-ideas/SURFACE_git-graft-enhance-expanded-git-subcommands.mdis excluded by!**/*.mddocs/method/backlog/cool-ideas/SURFACE_on-demand-exact-mcp-output-contracts.mdis excluded by!**/*.mddocs/method/backlog/dependency-dag.dotis excluded by!**/*.dotdocs/method/backlog/dependency-dag.svgis excluded by!**/*.svgdocs/method/retro/SURFACE_agent-working-set-control-plane/SURFACE_agent-working-set-control-plane.mdis excluded by!**/*.mddocs/method/retro/SURFACE_agent-working-set-control-plane/witness/verification.mdis excluded by!**/*.mddocs/plans/graft-source-architecture-and-managed-workspace-convergence.mdis excluded by!**/*.mddocs/three-surface-capability-matrix.mdis excluded by!**/*.md
📒 Files selected for processing (80)
src/api/tool-bridge.tssrc/cli/daemon-status.tssrc/cli/peer-command.tssrc/contracts/capabilities.tssrc/contracts/causal-ontology.tssrc/contracts/diagnostic-evidence-gap.tssrc/contracts/diagnostic-summary-bounds.tssrc/contracts/mcp-capability-discovery.tssrc/contracts/mcp-discovery-output-schemas.tssrc/contracts/output-schema-cli.tssrc/contracts/output-schema-fragments.tssrc/contracts/output-schema-mcp.tssrc/contracts/output-schema-meta.tssrc/contracts/output-schemas.tssrc/mcp/burden.tssrc/mcp/context.tssrc/mcp/persisted-local-history-graph.tssrc/mcp/persisted-local-history-policy.tssrc/mcp/persisted-local-history.tssrc/mcp/receipt.tssrc/mcp/repo-state-git.tssrc/mcp/repo-state-observation.tssrc/mcp/repo-state-transition.tssrc/mcp/repo-state-types.tssrc/mcp/repo-state.tssrc/mcp/repo-tool-job.tssrc/mcp/repo-tool-worker-context.tssrc/mcp/runtime-observability.tssrc/mcp/semantic-transition-summary.tssrc/mcp/server-context.tssrc/mcp/server-invocation.tssrc/mcp/server-tool-access.tssrc/mcp/server.tssrc/mcp/tool-input-controls.tssrc/mcp/tool-registry.tssrc/mcp/tools/activity-view.tssrc/mcp/tools/capabilities.tssrc/mcp/tools/diagnostic-detail.tssrc/mcp/tools/diagnostic-models.tssrc/mcp/tools/doctor.tssrc/mcp/tools/graft-edit.tstest/helpers/mcp.tstest/integration/mcp/daemon-bridge.test.tstest/integration/mcp/daemon-server.test.tstest/integration/mcp/server.test.tstest/unit/api/tool-bridge.test.tstest/unit/cli/daemon-status.test.tstest/unit/cli/peer-command.test.tstest/unit/contracts/capabilities.test.tstest/unit/contracts/causal-ontology.test.tstest/unit/contracts/mcp-capability-discovery.test.tstest/unit/contracts/mcp-discovery-output-schemas.test.tstest/unit/contracts/output-schemas.test.tstest/unit/library/index.test.tstest/unit/mcp/capabilities.test.tstest/unit/mcp/changed.test.tstest/unit/mcp/context-guard.test.tstest/unit/mcp/daemon-worker-pool.test.tstest/unit/mcp/diagnostic-summary.test.tstest/unit/mcp/graft-edit.test.tstest/unit/mcp/layered-worldline.test.tstest/unit/mcp/per-call-workspace-route.test.tstest/unit/mcp/persisted-local-history.test.tstest/unit/mcp/project-root-resolution.test.tstest/unit/mcp/receipt-builder.test.tstest/unit/mcp/receipt.test.tstest/unit/mcp/repo-state-observation.test.tstest/unit/mcp/repo-state-transition.test.tstest/unit/mcp/runtime-observability.test.tstest/unit/mcp/semantic-transition-guidance.test.tstest/unit/mcp/server-tool-access.test.tstest/unit/mcp/tools.test.tstests/playback/0058-system-wide-resource-pressure-and-fairness.test.tstests/playback/0078-three-surface-capability-baseline-and-parity-matrix.test.tstests/playback/0088-target-repo-git-hook-bootstrap.test.tstests/playback/CORE_migrate-path-ops-to-port.test.tstests/playback/CORE_v060-bad-code-burndown.test.tstests/playback/SURFACE_agent-dx-governed-edit.test.tstests/playback/SURFACE_capability-matrix-truth.test.tstests/playback/SURFACE_governed-write-tools.test.ts
| code_find: z.union([ | ||
| z.object({ | ||
| query: z.string(), | ||
| kind: z.string().nullable(), | ||
| matches: z.array(precisionSymbolMatchSchema).optional(), | ||
| total: z.number().int().nonnegative().optional(), | ||
| path: z.string().optional(), | ||
| projection: z.literal("refused").optional(), | ||
| reason: z.string().optional(), | ||
| reasonDetail: z.string().optional(), | ||
| next: z.array(z.string()).optional(), | ||
| actual: actualSchema.optional(), | ||
| source: z.enum(["warp", "live"]), | ||
| layer: worldlineLayerSchema, | ||
| }).strict(), | ||
| structuralRefusalSchema.extend({ | ||
| projection: z.literal("refused"), | ||
| }).strict(), | ||
| ]), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the two code_find definitions and locate all mcpOutputBodySchemas definitions/imports.
rg -nP -C2 '\bcode_find\s*:' src/contracts/output-schemas.ts src/contracts/output-schema-mcp.ts
echo '--- definitions ---'
rg -nP 'mcpOutputBodySchemas\s*(:|=)' src/contracts
echo '--- importers ---'
rg -nP "import[^;]*mcpOutputBodySchemas" srcRepository: flyingrobots/graft
Length of output: 2339
🏁 Script executed:
sed -n '1,220p' src/contracts/output-schema-cli.ts
printf '\n---\n'
sed -n '1120,1650p' src/contracts/output-schemas.ts
printf '\n---\n'
sed -n '120,310p' src/contracts/output-schema-mcp.tsRepository: flyingrobots/graft
Length of output: 33941
code_find schema drift breaks CLI validation — src/contracts/output-schemas.ts accepts the refused variant, but src/contracts/output-schema-mcp.ts still defines code_find as the plain object shape. That leaves symbol_find on the CLI path rejecting refused code_find bodies while the other validation path accepts them. Keep both definitions in sync.
🤖 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 `@src/contracts/output-schemas.ts` around lines 1278 - 1296, Update the
code_find schema in output-schema-mcp.ts to match the union defined in
output-schemas.ts, including the structuralRefusalSchema extension with
projection set to "refused" alongside the existing strict object shape. Keep
both validation paths synchronized so refused code_find bodies are accepted
consistently.
| payload: { | ||
| semanticKind: asString(node.props["semanticKind"]), | ||
| authority: asString(node.props["authority"]), | ||
| observationBasis: asString(node.props["observationBasis"]), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect transitionEventSchema's observationBasis field and asString's undefined handling.
rg -nP -A5 'transitionEventSchema\s*=' src/mcp/persisted-local-history.ts
rg -nP -A8 'function asString\(' src/mcp/persisted-local-history.tsRepository: flyingrobots/graft
Length of output: 469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '700,820p' src/mcp/persisted-local-history.ts | cat -nRepository: flyingrobots/graft
Length of output: 6085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all references to observationBasis and the transition event schema.
rg -n -A4 -B4 'observationBasis|transitionEventSchema|eventKind:\s*"transition"' src/mcp/persisted-local-history.tsRepository: flyingrobots/graft
Length of output: 4152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the transition event schema and any import/migration logic around observationBasis.
sed -n '340,430p' src/mcp/persisted-local-history.ts | cat -n
printf '\n---\n'
sed -n '1030,1075p' src/mcp/persisted-local-history.ts | cat -n
printf '\n---\n'
sed -n '1488,1510p' src/mcp/persisted-local-history.ts | cat -nRepository: flyingrobots/graft
Length of output: 5990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the transition event schema definition and the write path that populates observationBasis.
rg -n -A20 -B10 'const transitionEventSchema|transitionEventSchema\s*=' src/mcp/persisted-local-history.ts
printf '\n---\n'
rg -n -A20 -B10 'createTransitionEvent\(|observationBasis' src/mcp/persisted-local-history.tsRepository: flyingrobots/graft
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,170p' src/mcp/persisted-local-history.ts | cat -n
printf '\n---\n'
sed -n '1180,1265p' src/mcp/persisted-local-history.ts | cat -n
printf '\n---\n'
rg -n -A8 -B8 'createTransitionEvent|observationBasis' src/mcp/persisted-local-history.tsRepository: flyingrobots/graft
Length of output: 10631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the transition event schema and the createTransitionEvent implementation.
rg -n -A25 -B12 'const transitionEventSchema|export type TransitionEvent|createTransitionEvent\(' src/mcp/persisted-local-history.ts src/mcp/persisted-local-history-policy.tsRepository: flyingrobots/graft
Length of output: 5961
Make observationBasis backward-compatible for legacy transition nodes. readTransitionEventFromGraph() passes asString(node.props["observationBasis"]) into transitionEventSchema.safeParse(), so older persisted nodes without this property are rejected and their transition events are dropped from state.transitionEvents. Add a fallback/migration if pre-existing graph data is still supported.
🤖 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 `@src/mcp/persisted-local-history.ts` at line 757, Update
readTransitionEventFromGraph() so legacy transition nodes missing
observationBasis receive the schema-compatible default or migration value before
transitionEventSchema.safeParse() runs. Preserve the existing value for nodes
that provide observationBasis, ensuring older persisted events remain in
state.transitionEvents.
| if (detail === "summary") { | ||
| const localHistory = localHistorySummary(status, persistedLocalHistory); | ||
| const repoConcurrencyGap = concurrencyGap(repoConcurrency); | ||
| const degradedReasons: DiagnosticEvidenceGap[] = [ | ||
| "structural_history_readiness_unknown", | ||
| ...(status.bindState === "unbound" ? ["workspace_unbound" as const] : []), | ||
| ...(localHistory.readiness === "unavailable" | ||
| ? ["local_history_unavailable" as const] | ||
| : localHistory.readiness === "degraded" | ||
| ? ["local_history_inactive" as const] | ||
| : []), | ||
| ...(workspaceOverlayFooting === null | ||
| ? [] | ||
| : [workspaceOverlayFooting.degradedReason]), | ||
| ...(repoConcurrencyGap === null | ||
| ? [] | ||
| : [repoConcurrencyGap]), | ||
| ]; | ||
| const response: DoctorSummaryResponse = { | ||
| health: degradedReasons.length === 0 ? "healthy" : "degraded", | ||
| workspace: workspaceSummary(status), | ||
| history: { | ||
| structural: { | ||
| readiness: "unknown", | ||
| reason: "not_observed", | ||
| }, | ||
| local: localHistory, | ||
| }, | ||
| degradedReasons: [...new Set(degradedReasons)], | ||
| recommendedNextAction, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
health can never be "healthy" — a permanent degraded reason is always included.
degradedReasons unconditionally seeds "structural_history_readiness_unknown" (Line 98) regardless of actual state, and DoctorSummaryResponse.history.structural.readiness is typed as the literal "unknown" only, confirming structural evidence is never tracked today. Since health is derived purely from degradedReasons.length === 0, every summary response will report health: "degraded", making the "healthy" branch of the type unreachable in practice. This undermines the bounded, at-a-glance health signal that this new summary mode is meant to provide.
Consider excluding structural-history-unknown from the health computation (treat it as an informational caveat, not a health-degrading condition) until structural readiness tracking actually exists.
🩺 Proposed fix
- const degradedReasons: DiagnosticEvidenceGap[] = [
- "structural_history_readiness_unknown",
- ...(status.bindState === "unbound" ? ["workspace_unbound" as const] : []),
+ const informationalGaps: DiagnosticEvidenceGap[] = ["structural_history_readiness_unknown"];
+ const degradedReasons: DiagnosticEvidenceGap[] = [
+ ...(status.bindState === "unbound" ? ["workspace_unbound" as const] : []),
...(localHistory.readiness === "unavailable"
? ["local_history_unavailable" as const]
: localHistory.readiness === "degraded"
? ["local_history_inactive" as const]
: []),
...(workspaceOverlayFooting === null
? []
: [workspaceOverlayFooting.degradedReason]),
...(repoConcurrencyGap === null
? []
: [repoConcurrencyGap]),
];
const response: DoctorSummaryResponse = {
health: degradedReasons.length === 0 ? "healthy" : "degraded",
workspace: workspaceSummary(status),
history: { ... },
- degradedReasons: [...new Set(degradedReasons)],
+ degradedReasons: [...new Set([...informationalGaps, ...degradedReasons])],
recommendedNextAction,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (detail === "summary") { | |
| const localHistory = localHistorySummary(status, persistedLocalHistory); | |
| const repoConcurrencyGap = concurrencyGap(repoConcurrency); | |
| const degradedReasons: DiagnosticEvidenceGap[] = [ | |
| "structural_history_readiness_unknown", | |
| ...(status.bindState === "unbound" ? ["workspace_unbound" as const] : []), | |
| ...(localHistory.readiness === "unavailable" | |
| ? ["local_history_unavailable" as const] | |
| : localHistory.readiness === "degraded" | |
| ? ["local_history_inactive" as const] | |
| : []), | |
| ...(workspaceOverlayFooting === null | |
| ? [] | |
| : [workspaceOverlayFooting.degradedReason]), | |
| ...(repoConcurrencyGap === null | |
| ? [] | |
| : [repoConcurrencyGap]), | |
| ]; | |
| const response: DoctorSummaryResponse = { | |
| health: degradedReasons.length === 0 ? "healthy" : "degraded", | |
| workspace: workspaceSummary(status), | |
| history: { | |
| structural: { | |
| readiness: "unknown", | |
| reason: "not_observed", | |
| }, | |
| local: localHistory, | |
| }, | |
| degradedReasons: [...new Set(degradedReasons)], | |
| recommendedNextAction, | |
| }; | |
| if (detail === "summary") { | |
| const localHistory = localHistorySummary(status, persistedLocalHistory); | |
| const repoConcurrencyGap = concurrencyGap(repoConcurrency); | |
| const informationalGaps: DiagnosticEvidenceGap[] = ["structural_history_readiness_unknown"]; | |
| const degradedReasons: DiagnosticEvidenceGap[] = [ | |
| ...(status.bindState === "unbound" ? ["workspace_unbound" as const] : []), | |
| ...(localHistory.readiness === "unavailable" | |
| ? ["local_history_unavailable" as const] | |
| : localHistory.readiness === "degraded" | |
| ? ["local_history_inactive" as const] | |
| : []), | |
| ...(workspaceOverlayFooting === null | |
| ? [] | |
| : [workspaceOverlayFooting.degradedReason]), | |
| ...(repoConcurrencyGap === null | |
| ? [] | |
| : [repoConcurrencyGap]), | |
| ]; | |
| const response: DoctorSummaryResponse = { | |
| health: degradedReasons.length === 0 ? "healthy" : "degraded", | |
| workspace: workspaceSummary(status), | |
| history: { | |
| structural: { | |
| readiness: "unknown", | |
| reason: "not_observed", | |
| }, | |
| local: localHistory, | |
| }, | |
| degradedReasons: [...new Set([...informationalGaps, ...degradedReasons])], | |
| recommendedNextAction, | |
| }; |
🤖 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 `@src/mcp/tools/doctor.ts` around lines 94 - 124, Update the summary health
calculation in the detail === "summary" branch so the permanently seeded
"structural_history_readiness_unknown" evidence remains an informational
degradedReasons entry but does not force health to "degraded"; derive health
from the other actionable degradation reasons while preserving the existing
response shape and deduplication.
Records the first Graft-domain Edict action (graft.warp@1 / recordSymbolChange) proven to compile to real Core IR and lower to real Echo Target IR (echo.dpo@1) via Edict's actual compiler, plus the negative-control evidence confirming the custom compiler-context facts genuinely participate in resolution. Also captures a real dependency-drift finding: this experiment's harness does not build against Echo's currently pinned Edict revision, because of the TargetIrArtifact intents->actions rename in the very next Edict commit. Not yet checked against Echo's own build/test suite directly. Explicitly not validated: stale-basis enforcement, bundle assembly, WASM execution, or persistence semantics. Those are tracked as next steps.
Extends the warp-lawpack-v0 spike per explicit instruction to hold Echo's pin rather than regenerate its digest-locked provider package. Mechanically derives an intent-keyword variant of the canonical action-keyword source (single-token transform, checked to change nothing else), compiles it under Edict c75c3f5 (Echo's actual pinned revision, via a local worktree), and drives it through real invocation of Echo's checked, unmodified lowerer.echo-dpo.component.wasm. Result: the WASM component executes for real and returns a genuine typed refusal (UnsupportedSemantics), not a crash or validation failure -- the native oracle lowerer accepts this action shape but the real, narrower checked component does not. Cross-version comparison confirms the intent->action rename is a pure terminology migration with no semantic drift in Core/Target IR for this action shape. Bundle assembly, full runtime execution, persistence semantics, and stale-basis enforcement remain unproven and out of scope for this rung. The Echo compiler-epoch migration (rewriting the checked fixture, regenerating the digest-locked package via echo-wesley-gen) remains a separate, not-yet-started, explicitly scoped Echo-side change.
…shape Answers the question raised before committing to the Echo intent->action migration: is the checked lowerer component's UnsupportedSemantics refusal about the vocabulary epoch, action shape, or something else? Three variants under the identical pinned epoch and pipeline: the exact original fixture (positive control), a single-field Graft action matching the original's exact shape but with different names, and the four-field recordSymbolChange from the prior rung. Result: the original fixture is accepted; both the shape-matched and shape-different Graft actions are refused identically. The checked component currently accepts exactly one action identity -- the original fixture it was built and reviewed against -- not a general class of target.replace-shaped actions, regardless of intent/action vocabulary. Also records a harness bug found and fixed mid-experiment (a hardcoded core-coordinate label caused a false negative on the positive control) rather than silently discarding it. Reframes the recommended next step: the Echo epoch migration alone would not unblock new-action execution against this component. The real gating question is what it takes to teach the checked component a second action identity at all, which likely needs the same echo-wesley-gen regeneration machinery either way.
… costs Research-only, no Echo changes. Traces the UnsupportedSemantics refusal to its exact source: echo-edict-provider-lowerer/-verifier hardcode the original fixture's coordinate, type shapes, action name, profile, and budget as literal Rust constants and equality checks -- not a data-driven allowlist, real control flow compiled into the WASM component. Documents the two paths to change it (hand-written second recognizer branch vs. genuinely generalizing into a real interpreter, the latter complicated by the lowerer crate deliberately not depending on edict-syntax) and the downstream cost either way: WASM recompile via the existing xtask pipeline, echo-wesley-gen regeneration, re- verification, and updating Echo's own digest-pinned tests. Recommends neither path yet -- establishes cost, not a decision.
…ation Full re-verification pass on docs/design/warp-lawpack-v0/: recomputed every SHA-256 claim, diffed the two source variants against the mechanical-derivation claim, cross-checked every quoted refusal/ diagnostic against its raw evidence file, re-read the hardcoded constants directly from Echo's current source, and independently rebuilt and reran all three harnesses end-to-end -- all three reproduced byte-for-byte identical output to what's checked in. Found and fixed: - All three harness*.rs files had incorrect include_str! paths left over from their original scratch-directory layout (one referenced a nonexistent filename, two referenced a relative path that doesn't exist in this repo at all). Fixed to reference the actual checked-in record-symbol-change.edict, and reverified by rebuilding against the corrected paths. - teaching-a-new-action-research.md understated the xtask packaging tool's size as "2,100+ lines" when it's actually 2,868. - README's file table was missing two real files (record-symbol-change.c75-pinned.edict, teaching-a-new-action- research.md). - Added a "Reproducing these results" section documenting exactly what a rebuild requires, since no Cargo.toml is checked in. Everything else verified accurate: all SHA-256 hashes, the mechanical action->intent diff, all quoted compiler diagnostics and refusals, and all hardcoded-constant claims against Echo's live source.
1 participant
Summary
Compatibility
Pre-PR Code Lawyer repairs
The complete branch review found three P1, three P2, and one P4 issue. This head repairs the concrete defects: transition occurrence identity, stale Git evidence, reflog baseline freshness, CLI-v1 shape preservation, legacy WARP response validation, pre-write graft_edit body validation, and agent-onboarding documentation consistency. The design packet and retro contain the RED/GREEN evidence for each repair.
Broader two-phase plan/validate/commit semantics for daemon control-plane mutations are explicitly filed as follow-up debt; this PR does not claim that future architecture as shipped.
Validation
Evidence
Summary by CodeRabbit