[olafura]

Eleven scenarios in features/mc/orchestration/mcp-thread-tools.feature were still @backlog. Reading the environment did not cut long writing instructions, a credential issued by another environment could reach this one's preferences, launches and attachment sends accepted input past the advertised limits, listing or reading a thread did not say whether it can settle, a thread the user attached as context could not be read from another project, a deleted thread read as merely missing, interrupts did not say what happened, and a retried send or interrupt could act twice.

All of it surfaced first as undefined steps. The missing behaviour is now in the MC's MCP tools — environment preferences and truncation, an environment stamp on credentials, input-limit refusals (by the schema bounds main now checks for every tool), settlement state in list and read, user-attached cross-project reads (agent-written records do not widen them), an honest deleted answer, interrupt statuses, and clientRequestId retries answered once through dispatch_once — the steps drive it through the real MCP server, and the backlog tags are off. The file goes from 71/89 to 89 passed.

The behaviour is what upstream pingdotgg/t3code#2829 did, written anew in Elixir rather than carried over as code, so no upstream author is a co-author of these commits. The pull request stays cited in the feature file's # Sources: and Julius Marminge is already thanked in CREDITS.md.

Done with Muse Code powered by muse-wonder-full-1.0.

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[olafura]

This change moves eleven MCP thread-tool scenarios out of @backlog. It adds settlement state to list and read, lets a thread read a user-attached thread from another project, gives a deleted thread its own answer, adds input limits, reports interrupt statuses, and routes send and interrupt through dispatch_once so a retry with the same clientRequestId acts once. The read, limit and settlement code matches the description. The biggest risk is in the retry keys: the command receipt is keyed only by clientRequestId, not by who called. A second agent that reuses another agent's key on the same thread gets the first agent's receipt back, and its own message or interrupt is silently dropped. I couldn't run the feature file because Hex deps aren't fetched in this checkout. Smaller notes: the environment-stamp check has no path in production. Every credential is minted into this MC's local ETS table, and the environment tools are never forwarded, so only the test's restamping reaches it. That's harmless, but say so in the comment.

apps/server-ex/lib/hal_c2/mcp/tools.ex:811: The command id here is "command:mcp:" <> clientRequestId, and dispatch_once keeps its receipt in the store keyed only by that id. The receipt is checked only against the target thread. The message id two lines up is scoped by sender (message:mcp:#{sender}:#{key}), and so are create_threads ids (thread:mcp:#{caller}:...), but this command id is not. Failure case: agents A and B both send to parent t2 with clientRequestId: "status". B's call gets A's receipt, so no message is dispatched. B is still answered with messageId: message:mcp:B:status, which never existed, and status queued, so B's message is lost while the tool reports success. The interrupt key at line 867 has the same flaw: B's interrupt of a newer run is swallowed by A's receipt and still reported as interrupt_requested. Fix: scope both ids by sender, e.g. "command:mcp:#{sender}:" <> key and "command:mcp:interrupt:#{caller_id}:" <> key. Then add a scenario where two callers use the same key.

"commandId" => "command:mcp:#{sender}:" <> (args["clientRequestId"] || message_id),

Findings (2)

• 🔵 Low: apps/server-ex/lib/hal_c2/mcp/tools.ex:857 — An interrupt retried with the same key doesn't get the first answer back, because the rece…
• 🔵 Low: apps/server-ex/lib/hal_c2/mcp/tools.ex:687 — The comment says this mirrors Settlement.candidate?/2, but it leaves out two of its bloc…

[olafura]

Approved

The new commit scopes MCP retry keys by caller for sends and interrupts. A keyed interrupt now keeps its first answer as a receipt, so a later retry gets that answer back. Settlement's blockers move into Settlement.blocker/2, which automatic settlement and the list/read answers now share. Both earlier findings are fixed: a retry after the run ended now gets the receipt's answer, and a snoozed thread or a message waiting for its run now reports canSettle: false. The reorder in blocker/2 doesn't change candidate?/2. One small gap is left: only an answer that actually interrupted something is kept, so a keyed retry of a no_active_run call can still interrupt a later run. I couldn't run the feature file here because the test deps aren't fetched, so this review is from reading the code.

[olafura]

Approved

No findings.

The only commit since my last review adds HalC2.Orchestration.dispatch_once/1 to the turns proof's covers map, with the same step list as dispatch/1. That fits: dispatch_once/1 either calls dispatch/1 or answers from a stored receipt. A replayed receipt changes no run, so mapping it to the same steps is sound. My earlier low finding is still open and the code is unchanged: in apps/server-ex/lib/hal_c2/mcp/tools.ex at line 942, interrupt saves a receipt only for interrupt_requested. A retried no_active_run call can still interrupt a run that started after the first call. Since nothing here touches it, I am not raising it again.

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix proof proof/hal_c2/turns_proof_test.exs: Mix stopped because the Hex dependencies (ex_maude, exqlite, bandit, …) are not installed in this checkout: "the dependency is not available, run mix deps.get".

Ran 2 checks

• git diff 4a44a497d HEAD: Shows one change: 4 lines in proof/hal_c2/turns_proof_test.exs that add dispatch_once/1 to covers.
• sed -n 155,202p lib/hal_c2/orchestration.ex: Confirmed that dispatch_once/1 either calls dispatch/1 or returns a stored receipt, so the copied step list fits.

[olafura]

Approved

No findings.

Since the last review, one commit landed: a keyed interrupt that finds nothing running now stores its no_active_run answer in the receipt (tools.ex:940-950). A retry with the same clientRequestId gets that stored answer back and won't interrupt a turn that started afterwards. A new scenario covers this, and the step it reuses skips waiting for an interrupted run when there was none. The fix is correct, and the answers it leaves unkeyed are fine: a named run that already finished and a runId that wasn't found give the same answer on retry anyway. The previous review had no findings, so nothing is outstanding. The remaining risk is that I couldn't run the scenarios here (see validation).

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix features mc/orchestration/mcp-thread-tools.feature: (Mix) Can't continue due to errors on dependencies: bandit, exqlite, cucumber and other Hex deps are not available; mix deps.get has not been run in this checkout. I didn't fetch them because that needs network access.

Ran 1 check

• git diff 330fe9c55 HEAD: Read the one new commit (a19553a) together with the interrupt_now, interrupt_receipt and keep_interrupt_receipt code around it.

[olafura]

Approved

1 finding: 1 low.

Since the last review, main was merged in, and 0cbda86 removes the thread tools' own message and attachment checks in favour of HalC2.Mcp.Schema, which validates every tool call before its handler runs. It also deletes the duplicate step definition. That is correct: priv/mcp_tools.json gives launch message maxLength 120000 and attachments maxItems 8, and gives send_attachments maxLength 120000 with attachments minItems 1 and maxItems 8. So all four outline rows are still refused before anything is created. The previous review had no open findings. The only new issue is the leftover comment about limits in threads.ex.

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix features mc/orchestration/mcp-thread-tools.feature mc/orchestration/mcp-queue-project-and-pull-request-tools.feature: Can't continue due to errors on dependencies: Hex deps (cucumber, exqlite, bandit, …) are not fetched in this checkout. I did not run mix deps.get because it needs network access.

Ran 3 checks

• git show 0cbda8671; git log a19553a3a..HEAD: The only branch-side change is 0cbda86. The rest is main merged in with no conflict edits.
• python3 read of priv/mcp_tools.json for hal_c2_thread_launch / hal_c2_thread_send_attachments: launch: message maxLength 120000, attachments maxItems 8. send_attachments: message maxLength 120000, attachments minItems 1 and maxItems 8. These cover all four rows of the input-limit outline.
• grep 'must' lib/hal_c2/mcp/schema.ex: Every bound error reads '... must be ...' or '... must hold ...', which matches the step's new regex. The same step is shared by mcp-queue-project-and-pull-request-tools.feature:96.

[olafura]

Approved

No findings.

The one commit since 0cbda86 (ed551c7) deletes the stale input-limits comment in threads.ex, which fixes my previous finding: @capability_models now sits under no misleading comment. It changes no code, so my earlier approval of the rest of the PR stands, and I found nothing new.

Not run

• mise exec -- mix features orchestration/mcp-thread-tools.feature: The new commit only deletes a comment, so it cannot change how the code runs. The scenarios were already looked at in the previous review.

Ran 2 checks

• git diff 0cbda86712e6eee7e5ace9830ab9e3fb911357ea HEAD: Only the two comment lines above @capability_models in apps/server-ex/lib/hal_c2/mcp/tools/threads.ex were removed.
• git log --oneline 0cbda86712e6eee7e5ace9830ab9e3fb911357ea..HEAD: One commit: ed551c7 chore(mc): drop the stale input-limits comment in the thread tools