Skip to content

base red: MCP connect reads and writes its card JSON without an index (panic ratchet indexing_slicing 703 > 686) - #34032

Merged
lawrencecchen merged 8 commits into
feat-cmux-nextfrom
base-red/acpmux-panic-mcp-connect
Nov 3, 2026
Merged

lawrencecchen merged 8 commits into
feat-cmux-nextfrom
base-red/acpmux-panic-mcp-connect

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the tip panic-ratchet red acpmux: clippy::indexing_slicing 703 > baseline 686 (STATUS 9357; last green panic tip ab54fff). One commit per culprit:

Culprit Sites This PR's commit
b6e681e (MCP connect card that only the person approves) 10: hub/mcp_connect.rs card["name"] read and four card[..] = writes; secret_mcp.rs the three connect_mcp_server arguments and two result["status"] reads 4b17ef9
29b3781 (connect_mcp_server listed only with the preview flag) 1: secret_mcp.rs t["name"] in the tool-list filter bfd54ec
ccd0852 (after approval the daemon connects once) 16: hub/mcp_connect.rs first_connect reads of the card (transport, url, auth, keyName, keyOptional, name) and two record writes; secret_mcp.rs the result's connect, name, scope reads c3dddb9

Takes back #33811 and #33785: only the form of their JSON reads and writes changes; every value is the same.
Takes back b6e681e, 29b3781 and ccd0852 (direct pushes): only the form of their JSON reads and writes changes; every value and decision is the same.

Fix

Reads go through a small field(value, key) in each file (get, else Null, the value a serde_json index gives for a missing key or a non-object). Writes go through cmux_json::JsonSet::set, as the rest of acpmux does; each target is an object (card_of builds the card with json!({..}), and the record is a copy of a json!({..}) result). No #[allow], no baseline change.

Security core files touched

This is the MCP connect consent path. The PR changes only cmux-tui/crates/acpmux/src/hub/mcp_connect.rs (the card, the person's answer, the first connect) and cmux-tui/crates/acpmux/src/secret_mcp.rs (the agent-side tool and the text it reads). It does not change who may answer the card (hub/person.rs), the resolver and URL/command rules (cmux-mcp-connect), or mcp_probe. The key still goes only into the server's own header: key_header is built from the same three values. risk_tier.py grades these files normal; the review was asked to check each consent decision against the base.

Red at the base, green at the head

Command (fleet): cmux-ci run --class light --script scripts/ci/cmux-tui-rust-check.sh --arg panic --os linux --ref SHA

  • Red at 201216c (tip watcher, STATUS 9357): acpmux: clippy::indexing_slicing 703 > baseline 686. The current base 6eb610f runs as step 4fe3906df73561ad36e3b597 (result in a comment).
  • Green at this head c3dddb9: step 4521d2536cd175ac20b07131, exit 0 (rust-panic-ratchet: 86 crates checked, 36 at deny, no row over its baseline).

Later culprits on the base (the count rose to 714 while this PR was open)

Culprit Sites This PR's commit
63381ba (#33811, workflow: dispatch durable agents to paired machines) 15: hub/workflow_activity/remote.rs four request[..] = writes; server/requests/workflow_dispatch.rs brief, title, from and briefId reads; server/workflow_activity.rs the owner call's hello["_meta"] writes 7cb3b32
a965813 (#33785, workflow: expose daemon runs through the CLI) 1: hub/workflow_runs.rs spec["name"] a54af9d

Reads use field() or get; writes use JsonSet::set and set_path (the hello has no _meta, so set_path builds the same {_meta:{acpmux:{callerSession:..}}}). Green at the head a54af9d merged with the tip 649cebc: panic step a555ef647ddc57ef2d522e56 exit 0; clippy step b03745b31215f89b2c6b87f1 exit 0; test workflow step ee6eeac74b809ad9f1957b64 122 of 122 passed.

Changelog

none

Observed / Not run

Observed on the fleet (Linux) at this head: panic step green (above); clippy step ad64ec83d77ad1926bb18c6c exit 0; cmux-tui-rust-check.sh test mcp step ccff63d97be06ad6bfb0b8e2: 342 of 342 passed. rustfmt and prepush-gate.py passed.
Not run: no local cargo (rule); the live end-to-end test (CMUX_MCP_CONNECT_LIVE=1) needs the network servers.


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Replaces indexed JSON reads and writes in the MCP connect and workflow dispatch paths to satisfy the panic-ratchet baseline without changing behavior.

  • Missing fields still resolve to Null; existing request, consent, caller-proof, and receipt-token values are preserved.
  • Uses JsonSet::set and set_path for JSON writes, with no baseline changes or lint suppressions.
  • Extends the coverage to workflow dispatch: remote request writes, durable brief reads, the owner call's hello _meta, and the workflow-spec name read.

Written for commit a54af9d. Summary will update on new commits.

View guided diff Turn on auto-fix

… (panic ratchet, from b6e681e)

b6e681e (acpmux: MCP connect card that only the person approves) added
serde_json index reads and writes in hub/mcp_connect.rs (card["name"], four
card[..] = writes) and secret_mcp.rs (the connect_mcp_server arguments, two
result["status"] reads). Reads go through field() (get, else Null, the value
an index gives); writes through JsonSet::set on the card, which card_of always
builds as an object. Same values; the card and its consent path do not change.
… index (panic ratchet, from 29b3781)

29b3781 hides connect_mcp_server from the tool list unless
CMUX_MCP_CONNECT_PREVIEW=1 with t["name"]; it reads field(t, "name") now.
Same filter.
… text without an index (panic ratchet, from ccd0852)

ccd0852 (after approval the daemon connects once) reads the card's
transport, url, auth, keyName and keyOptional and writes the result record by
serde_json index in hub/mcp_connect.rs, and secret_mcp.rs reads the result's
connect, name and scope by index. Reads go through field(), the record writes
through JsonSet::set (the record is a copy of a json! object). Same decisions:
the key still goes only into the server's own header, keyNeeded and
signInNeeded are answered as before.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@cursor

cursor Bot commented Nov 3, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review a54af9d: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr34032 -f ref=base-red/acpmux-panic-mcp-connect -f platform=macos -f backend_mode=local

web and size checks: green

Shipped in:

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at c3dddb9

I read the full diff against the merge base 74c166b and the three commits one by one. No cargo was run; the fleet panic, clippy and test mcp steps in the description are the build evidence.

Reads (same value at each site)

  • serde_json Index<&str> for a read is index.index_into(self).unwrap_or(&NULL). Value::get(key) uses the same index_into. So field(v, k) gives &Null for a missing key and for a value that is not an object, the same as v[k].
  • *field(..) == "s" and != "s" use impl PartialEq<&str> for Value, the same impl that v[k] == "s" used. *field(card, "keyOptional") != json!(true) is still Value against Value.
  • In json!({"keyName": field(card, "keyName")}), "auth": field(card, "auth") and the server/scope/reason arguments, json! calls to_value(&expr). &&Value serializes the same as &Value, so the produced Value is the same.
  • field(card, "name").clone() gives the same value as card["name"].clone(), Null included.

Writes

  • JsonSet::set (cmux-json) turns null into an object, inserts into an object, and returns false only for any other type (where IndexMut would panic).
  • All three card_of arms that return Some are json!({..}) objects, so the four card writes always insert.
  • record is a clone of result, and every arm of first_connect (keyNeeded, connected, signInNeeded, failed, notStarted) is a json!({..}) object, so both record writes always insert.
  • The ignored bool from set is not #[must_use], and the rest of acpmux ignores it the same way.

Consent and the key

  • key_header is still Some only for (Some("key"), Some(keyName), Some(key)).
  • keyNeeded is still auth == "key", keyOptional not true, and no header.
  • signInNeeded still copies only auth.
  • The decision, status and scope logic in request_mcp_connect did not change.
  • The key goes only into mcp_probe::probe. The appended mcp_connect_result record and the agent text hold no key, as at the base.
  • The two status reads in secret_mcp.rs and the preview filter (*field(t, "name") != "connect_mcp_server") choose the same branches as before, so the tool stays hidden without CMUX_MCP_CONNECT_PREVIEW=1.

Lint and compile

  • Each private field helper is used outside tests in its own file (in mcp_connect.rs there is no test module that it could shadow).
  • use cmux_json::JsonSet as _; is new in mcp_connect.rs, and acpmux already depends on cmux-json.workspace. secret_mcp.rs already imported it.
  • field(&arguments, ..) gets arguments: Value, and field(t, ..) gets &Value from retain.

Commit mapping

  • I matched every index site that each culprit adds to the files: b6e681e has 10 sites, which are 4b17ef9; 29b3781 has 1, which is bfd54ec; ccd0852 has 16, which are c3dddb9. These match the table in the description.
  • The remaining result["outcome"], result["declined"] and arguments["name"/"secretRef"/..] sites were there before these culprits, and the PR does not touch them.
  • feat-cmux-next has not changed either file since the merge base.

No findings.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Panic step at the base 6eb610f (step 4fe3906df73561ad36e3b597): rust-panic-ratchet RED, acpmux: clippy::indexing_slicing 703 > baseline 686. At this head (step 4521d2536cd175ac20b07131): exit 0.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
… an index (panic ratchet, from #33811)

#33811 (63381ba, workflow: dispatch durable agents to paired machines)
added serde_json index reads and writes in hub/workflow_activity/remote.rs
(four request[..] writes), server/requests/workflow_dispatch.rs (the brief,
title, from and briefId reads) and server/workflow_activity.rs (the owner
call's hello _meta). Reads go through field() (get, else Null); writes
through JsonSet::set and set_path (every target is a json! object; hello has
no _meta, so set_path builds the same {_meta:{acpmux:{CALLER:..}}}). Same
values; the receipt token and the caller proof go where they went.
@cursor

cursor Bot commented Nov 3, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@lawrencecchen lawrencecchen removed the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@blacksmith-sh

blacksmith-sh Bot commented Nov 3, 2026

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmplbnwxe3t/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmpss9jburm/summary-missing.json (live):/summary-missing.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

… ratchet, from #33785)

#33785 (a965813) reads each spec's name with spec["name"]; it reads
spec.get("name").and_then(Value::as_str) now. Same names.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at a54af9d

Scope: the two commits added after the earlier pass at c3dddb9, 7cb3b32 (workflow dispatch) and a54af9d (workflow runs). Read-only review; no Rust build or test run by this reviewer.

What I checked:

  1. hub/workflow_activity/remote.rs, four request[..] = writes to request.set(..).
    • remote_request: request is a json!({...}) object, so set inserts exactly like IndexMut. field is a &str from the array; set takes impl Into<String>, so the key is the same.
    • Re-attach (receiptToken): request comes from remote_request (an object). Same value.
    • workflow_cancel_remote: request is the persisted remoteDispatch value. Before the writes, request.get("host").and_then(Value::as_str) must return Some, which is only possible for an object, so set writes action and wait exactly as before. (For a non-object, the old code returned early at the same host check, so the panic case could not occur there either.)
    • JsonSet scope: remote.rs has use super::* from hub/workflow_activity.rs, which has use super::* from hub/mod.rs, which has use cmux_json::JsonSet as _; (line 282). Glob imports bring in underscore-imported traits, and the base already relies on this pattern (for example hub/inbox.rs and hub/queue_approve.rs call .set with only use super::*). The compile also passed at this head: the build jobs and acpmux tests (lib, hub_integration) in run 41687623434 (headSha a54af9d) are green.
  2. server/workflow_activity.rs, the owner call's hello. hello is json!({"protocolVersion":1,"clientInfo":{...}}) and has no _meta. Map::set_path inserts _meta as Null, makes it an object, does the same for acpmux, and inserts CALLER_FIELD there. The result is {_meta:{acpmux:{CALLER_FIELD:{sessionId,token}}}}, the same as the old two writes, with the same key insertion order. The caller token goes in the same place. use cmux_json::JsonSet as _; is added to this file.
  3. server/requests/workflow_dispatch.rs, reads through the new field(). value.get(key).unwrap_or(&Value::Null) gives the same value as Index<&str> (Null for a missing key or a non-object). Inside json!, a &Value serializes the same as the indexed value. field(..).as_str() for briefId and from gives the same results. Name clash: fn field is defined only at line 114 in this module, and a local item shadows any glob-imported field, so it is not ambiguous. The file has no other field identifier. JsonSet was already in scope here (row.set, prompt.set_path existed before).
  4. hub/workflow_runs.rs: spec.get("name").and_then(Value::as_str) is the same as spec["name"].as_str() (None for a missing key, a non-string or a non-object spec).

No behavior change found. The receipt token and the caller proof go where they went before.

Not caused by these commits: cmux-next daemon tests (same-tree) is red at this head on SnapshotSessionBarrierTests.snapshotDoesNotAcknowledgeAHeldSidebarBatch (timed out on test-await-agents after 2.0 seconds). That is a Swift live-daemon sidebar barrier test, not workflow dispatch JSON. I did not check whether it is red on the base too.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen
lawrencecchen merged commit a25b4d9 into feat-cmux-next Nov 3, 2026
243 of 254 checks passed
@lawrencecchen
lawrencecchen deleted the base-red/acpmux-panic-mcp-connect branch November 3, 2026 19:41

This branch was previously deployed

3 inactive deployments
ci-routing-agent — a54af9dd Deployed Nov 3, 2026 by github-actions[bot] via cmux-next macOS placement #66882
artifacts — a54af9dd Deployed Nov 3, 2026 by lawrencecchen via publish tree-addressed cmux-tui #107100
sentry-dsyms — a54af9dd Deployed Nov 3, 2026 by lawrencecchen via upload cmux-tui dSYMs to Sentry #107100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant