Repository navigation
fix(mcp): preserve terminal action uncertainty on disconnect - #165
Merged
Merged
Conversation
link1345
marked this pull request as ready for review
October 2, 2026 06:23
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. |
link1345
force-pushed
the
fix/153-terminal-disconnect
branch
from
October 2, 2026 06:49
26f4d93 to
a5edab8
Compare
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
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.
Fixes #153.
An accepted terminal click could lose its completion when the runtime shut down. MCP then returned only a generic closed/failed connection error, dropping the host request ID and whether the action had already been sent.
Semantic action transport failures now report
not_sentorcompletion_unconfirmed, the request stage, send evidence, and available bridge/host IDs. Completion polling and legacy post-action observation stay on the original connection. Neither disconnect nor process exit is treated as operation success, and uncertain actions are not retried. Successful correlated completion, explicit bridge/host failures, and legacy null-receipt success remain compatible. Diagnostics do not include action payloads.Validation (original head
26f4d93, final heada5edab8):989163781a0e5ce6ac629e99a7d140a47e0db1ad, sample187117c03f2c4dc710c75a6657aac8bdf2e3ed8f, freshly built Debug addon with the pinned godot-cpp revision, official Windows x64 Godot 4.75b4e0cb0f, Bun 1.4.0.get_tree().quit()toget_tree().quit(23).isError: true,completion_unconfirmed,awaiting_completion, host ID 6 and bridge command ID. No final action replay. Received completion and process exit status are separate evidence.gua_auditorreviewed the cumulative change and found no actionable defects. It independently ran the initial 32-test suite and MCP typecheck; two final compatibility cases were then validated by the parent.input-metadata.test.tstimes out atget_versionidentically with frozen baseline and fixed sources on Windows (the full suite passes on Bun 1.4.0 and in PR TypeScript CI). No unrelated metadata workaround is included.Direct connections without the logging proxy also reproduced the ambiguity on baseline in both modes (normal exit 0 and nonzero fixture exit 23, one each). Three direct fixed runs retain
completion_unconfirmed,awaiting_completion, and request ID 6. The rendered normal fixed run received the correlated successful completion (request ID 6, epoch 1, frame 301, revision 7), returnedisError: false, and exited 0. This confirms that a genuine completion remains successful and that the terminal result depends on timing.Rebased cleanly onto latest main
b49ed96214fff59b31f6362687f6443fd6e1da91after packaging PR #163 merged.git range-diffconfirms an identical #153 patch; packaging does not change MCP, GDScript addon, core, runtime or bridge source. New heada5edab8fbfe169965487cb90dd2ec821cd7716dcagain passes 34 MCP tests and all workspace typechecks. The original automated review completed with no findings and a positive reaction; final-head CI passed all 17 jobs and automated review completed without findings, with a positive reaction and no unresolved threads. The superseded original CI run had 16 passing checks and one still compiling when canceled.Latest-main direct smoke: normal and nonzero-exit fixtures in both modes (four runs) all retain
completion_unconfirmed/awaiting_completionand host ID 6. The previously verified native addon is reused because the affected native and GDScript runtime source is unchanged by the packaging base update.Contract validation (all tests in
packages/mcp/test/action-outcome.test.ts):Execution: final head a5edab8, Windows Godot4.7, Bun1.4.0,
bun test packages/mcp/test34pass/121assertions,bun run checkall workspace checks pass; new13 tests also pass pinned Bun1.3.14. CI17/17pass and automated review completed at this exact head with positive reaction and no unresolved threads. Engine matrix: issue evidence; local JSON/traffic/process logs inevidence/{baseline-verified,baseline-direct,fixed,fixed-direct,latest-main-direct}, archived inissue-153-evidence.zip.Latest main3ab2617 changes only AGENTS.md from tested baseb49ed96; runtime/test source remains identical. Contract mapping and evidence handoff are applied within the existing audit gate. Unverified: Linux/macOS real-engine behavior, native completion generation timing, published-package consumers. Unit transport fault injection and repository packaging CI do not prove those paths. Exit status never confirms operation success.
Selected violation-detection proof:
validate-contract-mutation.pyexports the unchanged final head to a disposable directory. Removing structured diagnostic serialization causes both accepted-disconnect tests to fail exactly at line66 (missing outcome/stage/send/ID); removing completion-ID validation causes the mismatched-completion test to fail at line141 (incorrect success with ID8). Logs:contract-mutation.logandcontract-correlation-mutation.log. Mutations are outside the submitted checkout and restored. Startup attempts missing disposable dependency links are excluded; the final attempts reached these precise assertions.The existing read-only gua_auditor completed the new contract-evidence handoff without actionable gaps. It independently inspected latest main's AGENTS.md, mapping, PR body, mutation script and both logs, evidence README, test source and authoritative JSON summaries/sample traces. It confirmed both deliberate violations reached the exact intended assertions, disposable source was restored byte-identically, and the submitted checkout remained clean. It inspected engine artifacts rather than repeating engine runs or mutations. Its prior independently executed validation was 32 MCP tests and typecheck; the final 34 tests / 17 CI results were supplied by the parent.
Final merge gate verified unchanged head a5edab8, current main 3ab2617, 17 successful CI checks, no unresolved review threads and positive completed automated review. PR #165 was squash-merged as 095d101 at 2026-10-02T09:25:33Z. GitHub confirms issue #153 CLOSED. No releases or deployments performed.