[sonichi]

What changed, and why

The connections docs, help text and docstrings used "code", "connect code" and "agent portal" for what is the agent's relay token (<relay_url>|<bearer>). This PR:

• adds a short glossary to docs/connections.md that defines only the relay token (taken by add / update_token; the CLI reads it on stdin with -);
• rewords docs, docstrings and the refusal messages in src/connections.py from "code" to "token" (for example "the token is for agent …", "holds the NEW token", "relaunched on the unchanged token"); error codes, behaviour and identifiers are unchanged;
• points a refused move at the Sutando desktop app instead of the agent portal / add '<code>': the move fallback in docs/connections.md, the scripts/connections.py move hint, the sspace-migration health-check message and the last line of try_reuse_move's docstring now say "in the Sutando desktop app, open Homeserver connections and choose Update code… (Reconnect with a code… if it is paused)". This wording is under review and on hold: Update code… replaces the token of the existing ag2.space row, which a token for s.space cannot recover (see the new test below). The final wording is pending the owner's answer;
• adds tests/connections-move-recovery.test.py, an end-to-end regression over agent-api's real HTTP connections routes: with every row on ag2.space and move refused, replacing the ag2.space row's token with an s.space token is refused (legacy_row_unmanaged for the legacy row, token_server_mismatch for a named row), while adding the s.space token as a new connection (POST /api/v1/connections, confirm, expected_server: s.space) and promoting it with park leaves an s.space row that is the sole primary, with the ag2.space row parked.

Look first

docs/connections.md (glossary, the move fallback), scripts/connections.py (move hint), src/health-check.py (check_sspace_migration), tests/connections-move-recovery.test.py.

Behavior proven

• Recovery to an s.space primary through Sutando's own connections route: tests/connections-move-recovery.test.py (2 tests; a mutation that reconnects instead of promoting fails both).
• Reworded strings: tests/connections-cli.test.py, tests/health-check-sspace-migration.test.py, tests/connections.test.py assert the new text.

Feature/documentation consistency

docs/catalog.json last_verified not bumped (wording and a test; no feature change).

Evidence / tests

Run locally under env -i, nice -n 19:

• tests/health-check-sspace-migration.test.py 7 OK; tests/connections-cli.test.py 21 OK; tests/connections.test.py 361 OK (2 skipped); tests/dotspace-literal-guard.test.py 7 OK; tests/connections-move-recovery.test.py 2 OK
• bash scripts/review-checks.sh --diff <full PR diff>: PASS; ruff check on the touched Python: clean

Not touched (on purpose)

• The Codex and Claude Code adapter READMEs and the connect-code flow there: a separate PR covers them.
• docs/codex-core.md's MCP agent_link link code: a different flow from the relay token.

Live path: N/A. Written by Claude for Chi Wang, who is accountable for it.

🤖 Generated with Claude Code

[qingyun-wu]

Reviewed at 31ec7e2.

Scope check: the diff touches only prose, docstrings and comments; no executable line or identifier changes. But one of those doc lines is pinned by a test, and CI is red because of it.

Blocker: the doc and the health check must name the same command.

• docs/connections.md:329 changes add '<code>' to add '<connect string>'. tests/health-check-sspace-migration.test.py:101 (test_doc_section_names_the_same_commands) asserts the doc section contains python3 scripts/connections.py add '<code>', the same text the health check prints (asserted at line 58) and that scripts/connections.py:178 prints (asserted in tests/connections-cli.test.py:91,510). That is what fails python standalone tests (13/15), and the tsc + tests (clean install) aggregator fails only because of it.
• Fix: keep '<code>' in that step for now, or change the placeholder everywhere in one go (health-check text, the CLI move hint and the three asserts). The PR body already defers the CLI string to a follow-up; this doc line has to go with it.

Wording: "relay token" now names two different things.

• docs/connections.md:9-11 and the Codex README (packages/ag2-sparrow/ag2_sparrow/adapters/codex/README.md:45-46) define the relay token as what connect --code - yields, the bearer kept in the seat's token, and set it apart from a legacy url|secret connect string.
• docs/connections.md:328 then says "relay token (the legacy url|secret connect string)", and the unchanged docstring at src/connections.py:2230 says "The relay token (url|secret)". Pick one meaning and use it everywhere. If add takes either, step 1 could say "this agent's relay token or legacy connect string".

Wording: where the credential comes from.

• In the glossary, the Add agent flow creates a new agent and its one-time connect code. But docs/connections.md:150 ("new token from the Add agent flow rotates that credential"), :328 and src/connections.py:3556 send the reader to the Add agent flow to get an existing agent's token. Rotating an existing agent's token is "Get new token" in that agent's settings, as docs/connections.md:396 already says. Please point those three lines there, or explain how Add agent applies to an existing agent.

No private names, IDs or local paths found. The hardcoded-path and root-artifact scans are clean.

[keweichen]

Reviewed current head 4c360463f52ee2a2f022817a3ea9d4fbfd0e4df4 against base d2e2acf39afb0a96b963c598a88aead750acf5dd. The second commit changes metadata only; its tree is identical to 31ec7e2.

Changes requested; this head is not merge-ready.

1. The migration fallback command now disagrees with the shipped guidance and its contract test. docs/connections.md:329 says add <connect string>, while src/health-check.py:7015-7016 and scripts/connections.py:177-178 still tell users add <code>. On this exact head, python3 -B tests/health-check-sspace-migration.test.py ran 7 tests and failed test_doc_section_names_the_same_commands; the current exact-head CI shard 13 reports the same file failing. Please align the docs, runtime guidance, CLI hint, and tests together. Updating only the assertion would leave contradictory user instructions.

2. The glossary still gives one credential two incompatible meanings. connect --code writes <relay_url>|<bearer> to the seat token file (gateway_credential_sources.seat_credential_files), and preview_connection, which backs add and update_token, rejects a value without a URL-bearing url|secret shape. Presenting “that relay token” and “a legacy url|secret connect string” as alternative inputs conflicts with docs/connections.md:328 and src/connections.py:2230, which call url|secret the relay token. Define the one-time opaque acp.… code and the long-lived URL-bearing credential once, then use that mapping consistently. The same sweep should fix docs/dotspace.md:89 and the config-schema description, where an opaque connect code is still said to name a gateway host.

3. The existing-agent acquisition path is contradictory. docs/connections.md:150,328 and the try_reuse_move docstring send an existing agent to Add agent, while the new glossary says Add agent creates a new agent and docs/connections.md:396 names Get new token/Revoke for rotation. The worst case is creating another agent and then hitting token_agent_mismatch. Point migration and renewal to the actual Reconnect/Get new token flow.

4. Do not put this long-lived credential in argv. The new fallback example passes it as a literal command argument, exposing it to shell history and the process list. scripts/connections.py add - already reads stdin; document a safe stdin flow, including the required expected-server/confirmation handling, or make the CLI provide one.

5. Fix the current task metadata before merge. The metadata-only commit adds a named Stand: trailer, but public task trailers are IDs only, and the actual change commit still lacks its required Sutando-Task: trailer. Rewrite the two commits so each task commit carries the required ID trailer and the named trailer is gone.

The experimental-gate lines remain outside this review as scoped to #6259. review-checks.sh, git diff --check, Python compilation, shell syntax, and the 21-test connections CLI suite pass.

(agent-assisted, Codex for Kewei’s seat)

[qingyun-wu]

Reviewed at 4c36046. The distinction is the right one to draw (a one-time acp. code that connect --code redeems, versus the long-lived url|secret credential the bridge runs on), and the src/connections.py, start-cli.sh, codex-core.md and Codex README edits read correctly against the code: POST /api/v1/connections/<instance>/token takes {"token"}, and connect_common.CODE_PREFIX = "acp.". Two things block it.

Blocking

1. The diff breaks a test, and CI is red because of it. tests/health-check-sspace-migration.test.py:101 pins the fallback command in the "Moving a terminal-run core to s.space" section: assertIn("python3 scripts/connections.py add '<code>'", section). docs/connections.md:329 now says add '<connect string>', so python standalone tests (13/15) fails with test_doc_section_names_the_same_commands, and tsc + tests (clean install) fails as the aggregate. The test exists to keep this doc section and the health-check hint naming the same commands, so update the test, the doc and the CLI hint together (see 3). (Shard 4/15's failure, tests/gateway-agent-activity-hooks.test.py, is a timing test this docs-only diff can't reach. It looks unrelated, but rerun it to be sure.)

2. Fallback step 1 and the re-adoption paragraph now name the wrong source, and contradict the PR's own glossary. The glossary (docs/connections.md:9-11) says the Add agent flow creates the one-time acp.… code. Step 1 (docs/connections.md:328) then says to get the url|secret relay token "from the app's Add agent flow", and :150 says "getting a new token from the Add agent flow rotates that credential". As this doc describes it at :397, an existing agent's url|secret token comes from its settings (the access-token card, Get new token / Revoke), not from Add agent. Add agent creates a new agent. The same mislabel is in the try_reuse_move docstring (src/connections.py:3556, "the app's Add agent flow"). Please name the agent's settings / Get new token as the source in these three places, or, if Add agent really does hand out the url|secret string, correct the glossary to say so.

Non-blocking

3. scripts/connections.py:177-178 still prints get a connect code at <server>'s agent portal, then run ... add '<code>'. After this PR that is exactly the wording the doc calls wrong, so a user following the CLI hint is told to fetch a "connect code" and paste it into add, which (per the glossary) takes a token, not an acp. code. The PR body defers it because a test pins the string, but item 1 already needs a test edit, so changing both here keeps the doc, the hint and the tests consistent in one place.
4. Glossary nit: "legacy url|secret connect string" and "relay token" are the same bytes (the relay token is <relay_url>|<secret>). As written, "takes that token, or a legacy url|secret connect string" reads as two formats. One clause saying they are the same credential under its older name would help.

CI otherwise: 44 pass, 4 skipped, 3 fail (the two above plus the aggregate gate). review-checks.sh --diff: hardcoded-paths and root-artifacts clean; prose-cap skipped (no post-image locally).

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• scripts/connections.py (100%)
• src/connections.py (100%)

Summary

• Total: 9 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Thanks @keweichen for the detailed review (and @qingyun-wu, whose two reviews raised the same points). Fixed in 2375a21, point by point, each re-checked against the code first:

1. One fallback command everywhere. The doc section, the sspace-migration health-check hint and the move refusal hint in scripts/connections.py now all name python3 scripts/connections.py add - --yes --expected-server s.space (the CLI hint fills in the server from the refusal). The three asserts move with them: test_doc_section_names_the_same_commands and the hint assert in tests/health-check-sspace-migration.test.py, and the two in tests/connections-cli.test.py. Both files pass locally (7 and 21 tests).

2. One glossary. docs/connections.md now defines both terms once: a connect code is the one-time opaque acp.… code Add agent shows for a new agent, redeemed by connect --code -, which writes the agent's relay token to the seat's token file; a relay token is the long-lived <relay_url>|<bearer> credential (seat_credential_files writes exactly that), the bytes older text calls the url|secret connect string. add and update_token take only a relay token, because preview_connection needs the URL inside it. The rest of connections.md now says "token" wherever it meant this credential (refusal table, re-adoption, update_token, surfaces). The src/connections.py docstrings (module, primary_token_from_channel_file, try_reuse_move) use the same mapping.

3. Existing agent → Get new token, never Add agent. The re-adoption paragraph, fallback step 1 and the try_reuse_move docstring now point to that agent's settings, Get new token (the flow the update_token section already names). The glossary says Add agent creates another agent, whose token update_token refuses with token_agent_mismatch. The health-check and CLI hints say "Get new token, not Add agent".

4. No credential in argv. The fallback uses the stdin form cmd_add already supports: add - --yes --expected-server s.space, paste, Ctrl-D. The doc explains why --yes is needed: - uses up stdin, so the y/N prompt can't be answered, and --yes needs --expected-server. It also says what is checked: the row is saved only when the token's gateway answers as that server and accepts the token. Another server ("token belongs to server …") or a rejected token (token_rejected) is refused and nothing is saved. The Surfaces list now names the add - form next to update-token -.

5. Host-naming terminology. docs/dotspace.md and the allow_local description in docs/sutando-config.schema.json said an opaque connect code "may name a single-label host". They now say a relay token's gateway URL may.

6. Trailers. The new commit carries Sutando-Task: task-39c1095ab7d7b05d2b and no Stand: trailer. Existing commits were not rewritten. The earlier metadata-only commit (4c36046, the one with the Stand: trailer) can only be removed by a history rewrite (force-push), and that needs the owner's word, so it stays for now.

Checks run locally on the new head: the two test files above, plus config-known-keys-parity, sutando-config-twin-contract, dotspace-literal-guard, discord-post-gate-schema and health-check-live-checkout-branch. Also review-checks.sh --diff (hardcoded-paths, root-artifacts, prose-cap: PASS), ruff and git diff --check.

CI on the new head: 48 pass, 3 skipped, 0 fail.

[qingyun-wu]

Re-review at 2375a21. Both blockers from the previous review are fixed:

1. tests/health-check-sspace-migration.test.py:101 now asserts add - --yes --expected-server s.space, which docs/connections.md:332 (the s.space migration section) contains. tests/connections-cli.test.py:91 and :509 and src/health-check.py:7014 use the same command. CI is green at this head (48 pass, 3 skipping, 0 failed/cancelled), standalone shard 13/15 included.
2. Where the relay token comes from is now consistent. The glossary at docs/connections.md:9-14 says an existing agent gets a new relay token from its settings (Get new token) and never from Add agent. :153, :331 and src/connections.py:3556 now say the same, and so does :405.

The earlier non-blocking points are also handled. The scripts/connections.py:177-180 hint now says relay token and uses the stdin form. The glossary now says the relay token and the legacy url|secret connect string are the same bytes. The add - / --yes / --expected-server flow it recommends matches the CLI: scripts/connections.py:109 reads stdin and :111-113 refuses --yes without --expected-server.

Non-blocking nit: the update-token help still calls the token a "code", at scripts/connections.py:38-40 ("a new code ... reads the code from stdin ... a literal code") and :362 ("reads the new code from stdin"). The glossary reserves "code" for the one-time acp. connect code, so "relay token" would be more accurate there. Fine as a follow-up.

[qingyun-wu]

Re-approved at 63aac60. The only change since my last approval at 2375a21 is the button name: "Get new token" becomes "Get token" in docs/connections.md, scripts/connections.py, src/connections.py, src/health-check.py and the matching assertion in tests/connections-cli.test.py. That matches the web client, whose Identity & Access panel offers "Get token". CI: 48 pass, 3 skipped.

[sonichi]

60a3a60 reverts the core-agent token path added in 0d980a7. Checked against origin/main: the only reference to /api/v1/connections is the route itself (src/agent-api.py:207 connections_http, POST dispatch at src/agent-api.py:1575), and src/Sutando/ (RestartCoordinator.swift, SutandoConfig.swift, main.swift) contains no sign-in or token code. Nothing in this repository shows a desktop sign-in handing the relay token to the local agent API, so the move fallback, its CLI hint, the sspace-migration health check, the try_reuse_move docstring and their tests are back to main's text, and the glossary no longer has that sentence. The token wording and the "code" to "token" message rewording stay.

[qingyun-wu]

Reviewed only the delta from 63aac60 to this head.

[P2] Keep the recovery credential off argv: docs/connections.md:328-332 and scripts/connections.py:177-178 restore add '<code>', replacing the stdin recipe in the approved head. The value contains the long-lived relay bearer, so following this recovery instruction records it in shell history and exposes it in process arguments. The same instruction is restored in src/health-check.py:7014-7016. Keep the relay-token terminology and stdin form (add - --yes --expected-server ...) in all three surfaces. Removing an unavailable Get token action for the core's own agent does not require reverting the safe input path.

The new glossary correctly removes the unsupported claim that Add agent exposes an acp. code. The recovery text still calls the credential a connect code from an agent portal rather than matching the web client's Get token label for eligible external agents. Distinguish that flow from the core's desktop handoff; do not direct core users to a button they do not have.

Validation: 21 connections CLI tests pass; review-checks passes. CI at review time: 46 successful, one skipped, none pending or failing. The request is for the recovery-instruction regression above, not a CI failure.

[qingyun-wu]

Re-reviewed the complete delta from 60a3a60 to 9be2780. The previous blocker is fixed.

The recovery instructions in docs/connections.md:328-329, scripts/connections.py:177-178 and src/health-check.py:7011-7015 now point to Homeserver connections → Update code… / Reconnect with a code…. The delta introduces no bearer-bearing argv, shell-history recipe, or credential interpolation; the remaining move command contains no token. The obsolete agent-portal/add-command guidance is removed, including the related function docstring. The named actions exist in the web client's desktop-only HomeserverConnections component.

The tests exercise the new text: the CLI refusal test asserts the complete output string, the health-check test checks the exact desktop suffix and rejects agent-portal/add/promote text, and the documentation test checks the same instructions. All 21 CLI tests and seven health-check migration tests pass locally; review-checks passes. CI is green: 46 successful checks and one skipped, none pending, cancelled, timed out or failed. Approving to clear my prior request for changes. No merge or push performed.

[liususan091219]

Commenting at 9be27800. No change-request condition fires from my side, but this is not an approval yet. @keweichen's CHANGES_REQUESTED at 4c360463 is still the standing review from that account, and one of its points is still open.

@keweichen's five points, checked against this head:

1. Fallback command vs. its contract test. Fixed. The doc, scripts/connections.py:177-178 and src/health-check.py all give the same "Homeserver connections → Update code… (Reconnect with a code… if it is paused)" text. tests/health-check-sspace-migration.test.py asserts that suffix and assertNotIn("connections.py add '<code>'"). CI is green, including the shard that failed at 4c360463.
2. One credential with two meanings. Fixed. The docs/connections.md glossary defines one relay token, <relay_url>|<bearer>, and names url|secret as the same bytes. docs/dotspace.md:89 and the allow_local schema description now say "a relay token's gateway URL", not a connect code naming a host.
3. Existing agent sent to Add agent. Fixed. The glossary says Add agent creates another agent, and update_token refuses that token (token_agent_mismatch). The move fallback no longer points to Add agent.
4. Credential in argv. Fixed. The fallback no longer has a credential-bearing command, and the documented add - --yes --expected-server / update-token INSTANCE - forms read the token from stdin.
5. Commit trailers. Still open. 4c360463 still carries a named Stand: trailer, and 31ec7e21 has no Sutando-Task: trailer. The repo is squash-only, so this can be fixed in the squash message at merge time rather than by rewriting the branch. Please say so on the thread, so @keweichen can clear the request.

The PR body is stale. It says the change "pointed at the Add agent flow" and "added ... one defining sentence in the Codex adapter README". The head points to the desktop app's Homeserver connections, and no README is in the diff (commit 0d980a7e says the Codex README is left to its own PR). Please update the body so it matches what will be squashed.

Approve bar: @qingyun-wu approved at this head with a full delta re-review. This is a docs/wording change, so no live test is required. Once @keweichen re-reviews, or the CR is otherwise cleared, it meets the bar.

Checked: CI green (all checks pass), license/cla success (1 context), no unresolved threads, review-checks hardcoded-paths and root-artifacts clean.

— Sutando (susans-MacBook-Pro)

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed only the delta from approved 9be2780 to ae33e0c: it adds tests/connections-move-recovery.test.py, with no production, documentation or command-string change. Synthetic credentials travel in HTTP request bodies, not subprocess argv or shell recipes. The current web-client labels still match Homeserver connections, Update code… and Reconnect with a code….

Both new HTTP recovery tests pass. They verify rejected token updates, new connection confirmation, promotion, exactly one primary, the old row parked, and the correct vault value. review-checks passes. CI is complete: 46 successful checks, one skipped; none pending/cancelled/timed out/failed.

Non-blocking for this test-only delta, but the new witness exposes an existing guidance inconsistency: tests/connections-move-recovery.test.py:167-178 proves an update of the legacy or named old-server row fails with legacy_row_unmanaged/token_server_mismatch; recover() instead adds the new-server connection and promotes it. docs/connections.md:328-329 still directs refused moves to Update code…, whose desktop handler calls updateConnectionToken on the existing row. The labels match, but that action is not the cross-server recovery proven here. Align the prose/CLI/health-check guidance with the tested add-and-promote flow while retaining desktop token entry. This inconsistency predates the requested delta; it is not a regression introduced by these tests.

COMMENT records this follow-up and successful delta verification without reversing the prior approval on unchanged production behavior. No merge or push performed.

[liususan091219]

Reviewed at ae33e0c9 (the delta from 9be27800 is tests/connections-move-recovery.test.py plus the PR body). Requesting changes: the new test is evidence that the recovery instruction this PR ships does not recover.

Blocking: the fallback after a refused move points at an action the PR's own test proves is refused (incomplete fix).

• docs/connections.md:328-329, scripts/connections.py:177-178 (cmd_move), src/health-check.py:7014-7015 (check_sspace_migration) and the try_reuse_move docstring at src/connections.py:3556-3557 all now say: after a refused move, "open Homeserver connections and choose Update code… (Reconnect with a code… if it is paused)". The add + promote recipe those four places used to give is removed, and the tests that pinned it (tests/health-check-sspace-migration.test.py:58-62,79,102-105, tests/connections-cli.test.py:91,509-510) now assert the desktop wording instead.
• tests/connections-move-recovery.test.py:163-178 (added at this head) proves, over the real HTTP connections routes, that POST /api/v1/connections/<row>/token with an s.space token against the ag2.space row is refused: 409 legacy_row_unmanaged for the legacy row, 409 token_server_mismatch for a named row. Recovery in that test is POST /api/v1/connections (confirm, expected_server: s.space) and then promote with park — add a new connection, never update the old row's token.
• The PR body says the same: "Update code… replaces the token of the existing ag2.space row, which a token for s.space cannot recover", and marks the wording "under review and on hold … pending the owner's answer".

So a user who follows the sspace-migration warning, the move hint or the doc section at this head is sent to a dead end, and the working CLI path they had before this PR is gone from all three surfaces. At 9be27800 this was not yet shown; the test at this head shows it, which is why this review differs from the earlier comment.

Asks

1. Make the four surfaces name the recovery the test proves: add the s.space relay token as a new connection, then promote it (desktop: whatever control performs the add-and-promote; CLI: the stdin form add - --yes --expected-server s.space followed by promote s-space --repo . --logs-dir …, which 2375a212/63aac606 had and which keeps the bearer out of argv). If "Update code…" is to stay in the text, say for which case it applies (same server, rotated token) and do not offer it as the cross-server recovery.
2. Keep the three asserting tests pinned to that text, so the doc, the CLI hint and the health-check cannot drift apart again (that is what test_doc_section_names_the_same_commands and test_commands_parse_with_the_real_cli exist for).
3. Settle the on-hold wording before asking for merge; while it is on hold, this is not mergeable as-is.

Not blocking / status

• The new test is behavioural (real HTTP server, real connections module, fakes only at the vault, gateway probe, resolver and spawn); it is the right shape. The SUTANDO_WORKSPACE= in its isolate_os_environ call is the established test-isolation pattern, not a live default.
• @keweichen's CHANGES_REQUESTED at 4c360463 still stands from that account; points 1-4 are fixed at this head, point 5 (commit trailers: a named Stand: trailer on 4c360463, no Sutando-Task: on 31ec7e21) is still open and can be fixed in the squash message. @qingyun-wu's approval stands from 9be27800 with a delta verification at this head.
• Checked: CI 46 pass / 1 skip, license/cla success (1 context), no unresolved threads, hardcoded-path scan over the added lines clean.

— Sutando (susans-MacBook-Pro)