[bassilkhilo-ag2]

Priority

Level: medium
Reason: s.space-only desktop installs (rc.18 onward) get no gateway health verdict; read-side half of F20 in the ag2.space removal plan.

Closes

F20 (onboarding removal plan tracker), read side. s.space stays a secondary lane: PRIMARY_LANE_DOMAINS in the desktop is unchanged.

Summary

On the desktop, s.space signs in as a secondary gateway lane, so its bridge writes only state/gateway-status.<lane>.json (for example gateway-status.s_dspace.json). health-check, core-input-watch and services_status read only the unsuffixed state/gateway-status.json, so on an s.space-only install they had no opinion about the lane that is actually serving. gateway_serving (the shared owner of this record) now resolves the desktop's active lane: active_lane() reads the first line of $SUTANDO_APP_SUPPORT/channels/active-gateway-lane (the selector the desktop writes; ignored unless it is a valid instance slug other than all/primary), and status_path(state_dir) returns that lane's suffixed file when it exists, else the unsuffixed one. The three readers resolve their default path through it; health-check keeps status_read_path's legacy workspace-root fallback for the unsuffixed file.

The "AG2 Space re-link recovery" text that tells the agent which status file to read lives in the desktop repo (engine/AGENTS.md / engine/CLAUDE.md of ag2-space/ag2space-cinny-desktop), not in this repo, so it is updated in the companion desktop PR ag2-space/ag2space-cinny-desktop#1005.

Checklist

• Confirmed no other open PR closes the same issue
• Git author email matches my CLA-signed email (bassil@ag2.ai)
• Single concern per PR
• Confirmed bug exists on upstream/main (the new test fails there, below)
• Test added: tests/gateway-status-active-lane.test.py
• Before/after evidence pasted below
• Every claim is verifiable from the diff or the pasted output
• Live path? Read-only status readers; no bridge, delivery loop or startup change. The live case (an s.space lane writing gateway-status.s_dspace.json with channels/active-gateway-lane = s_dspace) is the one the rc.18 L2 journey run recorded (connect_op committed lane=s_dspace, gateway window gateway-s_dspace).
• Stacked PR? N/A
• No host-specific paths; fixtures are temp dirs
• No CI workflow change (the new file is picked up by find tests -name '*.test.py')
• Uses resolve_workspace / status_read_path; the app-support dir comes from $SUTANDO_APP_SUPPORT, as channel_env_containment and cloud_auth read it

Before / after evidence

Before (src reverted to upstream/main 9826add, new test kept):

$ python3 tests/gateway-status-active-lane.test.py
AttributeError: module 'gateway_serving' has no attribute 'status_path'

After (this head):

$ python3 tests/gateway-status-active-lane.test.py
PASS: gateway-status readers resolve the active lane, legacy file still read
$ python3 tests/gateway-serving-shared-owner.test.py           # PASS
$ python3 tests/health-check-gateway-bridge.test.py            # PASS
$ python3 tests/gateway-connected-requires-last-ok.test.py     # PASS
$ python3 tests/gateway-age-helpers-overflow-safe.test.py      # PASS
$ python3 tests/core-input-watch-gateway-probe.test.py         # PASS
$ python3 tests/core-input-watch.test.py                       # PASS
$ python3 tests/services-status-liveness-patterns.test.py      # PASS
$ python3 tests/services-status.test.py                        # PASS

The new test covers: a workspace with only gateway-status.s_dspace.json and active lane s_dspace reads as serving in health-check (_gateway_serving, _gateway_last_ok_age_h), core-input-watch (_gateway_status) and services_status (registry probe path and probe_gateway); selectors absent, empty, all, primary, ../etc and S_DSPACE fall back to gateway-status.json; an active lane whose file is missing falls back; the legacy single file still works.

🤖 Generated with Claude Code

[yixuan-ag2]

Changes requested. There is one blocker, a red required check caused by the new test file. Everything else below is non-blocking. Reviewed at head 563d0198, merge-base 9826add5.

Blocker

tests/hermetic-env-structural.test.py fails on the new file with tests/gateway-status-active-lane.test.py:26: misplaced-isolation and :36: unisolated. That failure is the red python standalone tests (2/9), and it fails the required tsc + tests (clean install).

• Reproduced at head: FAILED (failures=2), and both failures name this file.
• Control: the same suite at the merge-base without the file gives Ran 27 tests ... OK.
• Cause: lines 23-25 (tempfile.mkdtemp() / .mkdir()) run before isolate_os_environ(...) at :26. That placement voids the isolation, so the loader at :36 counts as unisolated.
• Fix: call the isolation first, then create the two dirs and set the env vars. That should clear both findings.

The other red jobs look unrelated. (6/9) is whatsapp-guided-connect, the known flake in #5784. (7/9) is ack-claim-lock-cleanup, which passes locally at head and imports none of the touched modules. A rerun should confirm.

Verified

• Discrimination of the new test:
  • At head: PASS.
  • Control 1, merge-base plus the test only: fails with AttributeError on status_path.
  • Control 2, merge-base plus head's gateway_serving.py only (readers unwired): the 4 reader checks fail (health-check _gateway_serving and last-ok age, core-input-watch, services_status probe path). The reader assertions discriminate, not just the resolver ones.
  • Mutation, exists() fallback dropped: both "missing lane file" checks fail.
• Existing suites at head, all rc 0:
  • gateway-serving-shared-owner, health-check-gateway-bridge, gateway-connected-requires-last-ok, gateway-age-helpers-overflow-safe
  • core-input-watch-gateway-probe, core-input-watch (195 tests)
  • services-status-liveness-patterns, services-status
  • health-check-gateway-lane-verdict (12), health-check-gateway-stale-lane (29), health-check-gateway-outage-duration (7)
  • gateway-runtime-identity, health-snapshot (93)
  • review-checks.sh --diff: PASS.
• Bug exists on main, read-only on a live s.space-only desktop install. The install has selector s_dspace, a fresh gateway-status.s_dspace.json and a parked primary gateway-status.json about 4.8h stale.
  • At the merge-base, health-check _gateway_serving() returns None and _gateway_status_stale_age_s() is about 17359s. check_gateway_bridge therefore takes the stale-sidecar warn branch before it reaches _gateway_lane_verdicts, even though that function does report s_dspace serving. core-input-watch _gateway_status returns None.
  • At head, both return True.
  • The fixture has no primary file at all. With an absent primary, main already falls through to the lane verdicts, so the stale-primary shape is the one that actually warns on main. Worth adding as a case.
• No reader left behind. The other gateway-status readers in src/ either glob every lane (health_snapshot.py, connections.py, health-check's lane helpers) or are per-instance bridge code. services_status.GATEWAY_STATUS_PATH is now unused in src/; only tests/services-status.test.py:389 uses it.

Fallback semantics and worst case (non-blocking)

• Selector absent, empty, all, primary or invalid: the unsuffixed file is read, the same as main.
• Selector names a lane whose file is missing: the unsuffixed file is read. On a desktop that file is the parked lane's leftover.
  • A fresh, connected leftover (just after a lane switch) reads as serving until its freshness window runs out: 180s in health-check and services_status, 90s in core-input-watch. After that, health-check gives a stale warn and the other two have no opinion and fall back to pgrep.
  • So this fails toward a short false-OK, or a short false not-serving if the leftover recorded connected:false. It cannot turn a connected legacy record into "disconnected".
  • These reads match main, so this is not a regression.
• Divergence from the desktop's lane policy:
  • active_lane() reads the raw selector. The desktop's launcher guard (_effective_active_lane) and its own status readers (effective_active_gateway_lane, documented there as "Status readers use this, never the raw file") ignore the selector in three cases: it does not serve the signed-in homeserver, it has no launch key, or it relays via loopback. In those cases every lane runs.
  • In that state this PR moves the three readers off the primary onto the named lane. If that lane is not signed in, core-input-watch could pin gateway-down where main read the serving primary.
  • The desktop writes the selector and the homeserver record together, with rollback, so this should be rare. It is still a third reading of a policy the desktop keeps in two parity-tested copies. Having the desktop publish the effective lane would remove it. That can be a follow-up.

Nits

• src/gateway_serving.py:26: the comment calls this the bridge's GATEWAY_INSTANCE shape, but the bridge's shape is [A-Za-z0-9_-]{1,32} (gateway_instance.INSTANCE_RE, src/connections.py:101). Lowercase-only matches what the desktop writes, so the behavior is fine; the comment is wrong.
• Selector validation is barely pinned. With the regex and the all/primary exclusion both removed, only the S_DSPACE check fails locally, and only because macOS is case-insensitive. On a case-sensitive FS the fallback should hide it too. Asserting gs.active_lane() is None for each rejected selector would pin it.
• (F20) in the gateway_serving.py:45 docstring and the services_status.py:284 comment is a tracker reference (CLAUDE.md comment rule).
• The "KNOWINGLY PRIMARY-ONLY" header at services_status.py:278 now contradicts its last line.
• There is a stray if True: at test line 81.

What would change the verdict: a green hermetic-env-structural. With that fixed, I'd approve.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/core-input-watch.py (100%)
• src/gateway_serving.py (100%)
• src/health-check.py (100%)
• src/services_status.py (100%)

Summary

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

[yixuan-ag2]

Approve. Reviewed at head 77aa083e. The blocker from my last review (at 563d0198) is fixed. The notes at the end are non-blocking.

Blocker fixed

• isolate_os_environ() now runs at test line 10, before any project import. The temp dirs are created at lines 20-22, after it.
• I ran the structural scanner's violations() on the new test file:
  • At 563d0198: :26 misplaced-isolation, :36 unisolated.
  • At 77aa083e: [].
• The full tests/hermetic-env-structural.test.py at head gives Ran 27 tests ... OK.
• CI at head: 36 of 37 check runs succeed and PowerShell + Windows runtime is skipped. Every python standalone tests (n/9) shard passes, and so does tsc + tests (clean install). The earlier (6/9) and (7/9) reds did not come back.

Other checks

• F20 references are gone. No added line in the diff against main contains F20, and git grep F20 over the five touched files at head finds nothing.
• The new test still discriminates. It passes at head. I ran two controls:
  • Merge-base 9826add5 src with the head test fails with AttributeError ... status_path.
  • Merge-base src plus only head's gateway_serving.py fails all four reader checks: health-check serving, health-check last-ok age, core-input-watch, and the services_status probe path.
• Dropping SUTANDO_WORKSPACE_DIR from the test leaks nothing. I ran it with an empty <repo>/workspace/state present, and it wrote nothing there. The readers get WS and STATE assigned or passed explicitly.
• Related suites pass at head:
  • hermetic-workspace-guard, lint-hermetic-bridge-tests, lint-hermetic-discovery-floor, hermetic-env-sentinels
  • gateway-serving-shared-owner, health-check-gateway-bridge, core-input-watch-gateway-probe, services-status
  • health-check-gateway-lane-verdict, health-check-gateway-stale-lane
• scripts/review-checks.sh --diff on the PR diff: PASS.

Still open from last review (non-blocking)

• src/gateway_serving.py:26 says the regex is the bridge's GATEWAY_INSTANCE shape. The bridge's shape is [A-Za-z0-9_-]{1,32}.
• The rejected selectors are pinned only through the path fallback. Asserting gs.active_lane() is None for each one would pin the regex itself.
• The fixture still has no stale-primary case. That is the shape that actually warns on main.
• The KNOWINGLY PRIMARY-ONLY header at src/services_status.py:278 contradicts its last line.
• There is still an if True: at test line 76.
• The divergence between the raw selector and the desktop's effective_active_gateway_lane remains a follow-up.

[liususan091219]

Approve. Reviewed at head 77aa083e. The bar is met by the expert route: @yixuan-ag2's approval at this head is substantive, and the required verification exists. They reproduced the bug read-only on a live s.space-only desktop install. On main, _gateway_serving() returned None against a 4.8h-stale primary; at head it returns True. They also ran controls showing the reader assertions discriminate, and confirmed the hermetic-env blocker is fixed.

Independently checked

• Tests use the real code. tests/gateway-status-active-lane.test.py imports the real gateway_serving and loads the three readers via spec_from_file_location. It does not grep the source text or copy the logic into the test. It passes locally at head.
• No reader is left behind. I searched src/ scripts/ skills/ packages/ for gateway-status. The single-file readers core-input-watch.py:529, health-check.py:6975 and services_status.py:286 now go through gateway_serving.status_path. The rest either read every lane's file (health_snapshot.py:379, connections.py:1578, connect_bridge.py:148) or are code inside one bridge instance.
• The lane policy has one owner. It lives in gateway_serving.py:30-51.
• Status. CI is green and license/cla passed. There are no unresolved threads.

Non-blocking

1. gateway_serving.py:32: active_lane() reads only $SUTANDO_APP_SUPPORT. sutando_config.desktop_runtime_selection_path also falls back to the engine tree's parent directory, so jobs started without that variable quietly read only the primary file.
2. gateway_serving.py:26: the comment calls this the bridge's instance shape. The bridge's shape is [A-Za-z0-9_-]{1,32} (connections.py:101).
3. Asserting gs.active_lane() is None for each rejected selector would pin the rule itself. There is also a leftover if True: in the test.
4. services_status.py:278: the "KNOWINGLY PRIMARY-ONLY" header contradicts line 284.

— Sutando (susans-MacBook-Pro)

[sonichi]

Code-owner approve at 05e333f (agent-assisted, Blue for Chi's seat).

• One owner for the lane choice. gateway_serving.status_path resolves it, and core-input-watch, health-check and services_status delegate to it instead of each calling primary_sidecar_name. That follows the shared-adapter-policy rule.
• Safe selector. active_lane() reads the first line of $SUTANDO_APP_SUPPORT/channels/active-gateway-lane and accepts only the bridge's instance shape (_LANE_RE admits no / and no ..), excluding all/primary. A bad selector can't point the read outside state/. The lane file is preferred only when it exists; otherwise it's the primary sidecar as before, so nothing changes for installs without the selector.
• health-check keeps status_read_path's legacy root fallback for the chosen name.
• Non-blocking: active_lane() depends on SUTANDO_APP_SUPPORT. A process started outside the desktop's environment (a manual health-check.py from a shell) falls back to the primary sidecar and reports the s.space lane as unknown again. Worth a line in the docstring, or a fallback to the app's default support dir.
• Since Yixuan's and Susan's approvals, this head only merges main. CI was still running when I approved.

[john-the-dev]

Approve at exact head 05e333f37ffbc1ade656f444e3f18196aaf76399 (independent agent-assisted review). No blocking findings.

The shared gateway_serving.status_path owns the lane choice, and all three single-file readers delegate to it. This fits the shared-adapter-policy boundary. Existing freshness/reconnect decisions remain with their readers; health-check retains its legacy path fallback. No status schema or writer changes are introduced.

Independent validation in isolated temporary state:

• The active-lane integration test passes. Keeping the new resolver but restoring the three readers from base 369597667abe602b82d146c072351c1e46e60ef8 fails four reader assertions, so this verifies the actual wiring rather than only the new helper's existence.
• Additional real-reader scenarios: stale primary + serving active lane returns (True, True, running) at head versus (None, None, offline) with base readers. A fresh healthy primary cannot mask a down active lane (False, False, offline) or malformed active record (None, None, offline).
• Shared serving-contract, health-check gateway, core-input gateway-probe, services-status (36 tests), and hermetic structural (27 tests) suites pass. The earlier test-isolation blocker is resolved.
• The exact-tree review scanner passes.

Non-blocking limits remain as already discussed: selection depends on SUTANDO_APP_SUPPORT; missing active sidecars fall back to the primary, and the raw selector can differ from the desktop's effective selection. Worst case, a missing active sidecar can temporarily report the primary’s health; existing identity-blind process fallbacks also remain. This is retained fallback behavior, and the change does not start or stop gateway services. The regex comment should describe the desktop selector shape, not claim parity with the bridge's broader instance validator.

This review used synthetic sidecars and real reader code; no live service restart or round trip was performed. Listed checks pass/skip, but the structured readiness collector returned UNKNOWN, so this code approval is not a merge-readiness claim.

[keweichen]

CHANGES REQUESTED at 05e333f37ffbc1ade656f444e3f18196aaf76399.

Blocking: src/gateway_serving.py:26-50 introduces a private, unbounded lane matcher that drifts from the production GATEWAY_INSTANCE contract (src/connections.py:103-104 and packages/ag2-sparrow/ag2_sparrow/gateway_instance.py:26, both [A-Za-z0-9_-]{1,32}).

Exact repro on macOS with Python 3.13.5: write 256 lowercase a characters to $SUTANDO_APP_SUPPORT/channels/active-gateway-lane. At this head, active_lane() returns all 256 characters and _LANE_RE accepts 33 and 256 characters, while both production validators reject them. gateway_serving.status_path(), health-check._gateway_serving(), and services_status.service_registry() then raise OSError: [Errno 63] File name too long; core-input-watch._gateway_status() catches the exception and silently degrades to None. This defeats the stated invalid-selector fallback and can stop health/status output on one malformed desktop-owned state file. The green test covers traversal and uppercase but not the production length bound.

Please reuse or extract one dependency-light bounded instance validator, make invalid or unreadable selector values fall back without raising, and add behavioral controls for values over 32 characters, including a 256-character selector. That would change this verdict. The focused suites and CI otherwise pass, and the intended active-lane delegation is sound.

[sonichi]

Re-reviewed at ae0ee29 (agent-assisted, Blue for Chi's seat). Still approve.

• Lane rule. The selector is now checked against the bridge's own connections.INSTANCE_RE (^[A-Za-z0-9_-]{1,32}$) instead of a private regex, so there's one owner for what a lane is.
• Invalid selectors. ../etc, dotted names and anything over 32 characters are never a lane.
• Uppercase. An uppercase selector now passes the shape check, but it only looks up gateway-status.<lane>.json, which won't exist, and falls back. That's harmless.
• No raise. p.exists() is wrapped in try/except OSError, so an over-long path can't raise.
• Tests. They now assert both active_lane() is None and the fallback for each bad selector.
  Merge once CI is green (3 pass / 15 pending when I looked).

[john-the-dev]

Re-reviewed exact head ae0ee29fa5aa0cf5e83cbfa919b44dd8c13d9b97. The overlong-selector failure raised by @keweichen is fixed; no remaining blocking code finding in this delta.

The previous private matcher admitted unbounded values. The helper now delegates to connections.INSTANCE_RE.fullmatch, enforcing the production 32-character bound, and treats an OSError from the file existence probe as fallback. The updated tests assert selector rejection directly and exercise all reader paths for 33- and 256-character values.

Independent evidence:

• Updated active-lane test passes at this head. Keeping the updated test but restoring gateway_serving.py from 05e333f3 fails with the reported OSError: [Errno 63] File name too long on the 256-character selector.
• Shared serving-contract suite and services-status suite (36 tests) pass.
• Three independent reader scenarios still pass: stale primary/healthy active, healthy primary/down active, and healthy primary/malformed active.
• Exact-tree review scanner passes.

My earlier review did not catch the missing selector length bound; this recheck reproduces that failure and verifies the fix. The separate changes-requested review still needs its reviewer’s update; this code recommendation does not clear their verdict. CI was still running, and the structured readiness receipt remains UNKNOWN. No live runtime/configuration changes or post-restart round trip were performed.

[github-actions]

@cla-assistant check

[sonichi]

Re-reviewed at 5b12d94 (agent-assisted, Blue for Chi's seat). The one new commit is test-only. It covers the shell-run path with no SUTANDO_APP_SUPPORT (no lane). It also covers the lane file's exists() raising: status_path, health-check and core-input-watch all fall back to the primary file, and the patch is restored in finally. Still approve; merge once CI is green.

[john-the-dev]

Approve at exact head 5b12d940d76c1eb611730947f2e53476d5295b69 after reviewing the test-only delta from ae0ee29. No new production changes or blocking findings.

The added checks exercise the existing empty-app-support and existence-probe-error branches. The Path.exists patch is limited to the active lane filename and restored in finally; the test then confirms normal lane selection resumes.

Independent isolated validation:

• Updated active-lane reader suite passes.
• Making the production existence-error handler re-raise makes the new check fail with exists() raising propagated OSError(63, ...).
• Removing the empty-root guard, with a valid selector under an isolated working directory, makes the new no-app-support assertion fail. This demonstrates it prevents an unintended relative-path selector read, rather than passing merely because no file exists.
• Exact-tree review scanner passes.

The prior production review and overlong-selector fix verification still apply. This refresh adds branch-test evidence; it does not claim a live round trip or green CI. CI remains in progress and the structured readiness collector returns UNKNOWN. No merge or runtime changes performed.

[keweichen]

Re-reviewed exact current head 5b12d940d76c1eb611730947f2e53476d5295b69. APPROVE; ready to merge. This replaces my prior changes-requested review because its blocker is fully fixed.

• active_lane() now delegates to connections.INSTANCE_RE.fullmatch, so the reader uses the production [A-Za-z0-9_-]{1,32} bound instead of a private unbounded matcher. The 33- and 256-character selectors are rejected before path construction.
• status_path() catches OSError from the selected lane file probe and falls back to the primary sidecar. The 5b12d940 delta is test-only and directly forces that branch, restores Path.exists in finally, and pins the no-SUTANDO_APP_SUPPORT path.
• Exact-head local validation passed: active-lane integration, shared serving-owner, health-check primary-connection (22 tests), core-input gateway probe, services-status (36 tests), hermetic-env structural checks, and the cumulative review scanner. Keeping the current test against 05e333f3 source fails with the original 256-character ENAMETOOLONG, so the regression control discriminates.
• Current CI and CLA are green; Windows is intentionally skipped. Worst case for a malformed selector or unreadable lane probe is now the established primary-sidecar fallback, not a health/status crash. This is a read-only status-selection path, so no bridge restart witness is required.

[liususan091219]

Approve at 5b12d940. This replaces my earlier approval at 77aa083e, which the later commits made stale.

Bar met (expert route, and also ≥3 known approvals): at this exact head there are substantive approvals from Chi's seat (sonichi), @john-the-dev and @keweichen. @keweichen's approval replaces their own change request at 05e333f3, whose blocker (a private, unbounded lane matcher) is fixed: gateway_serving.active_lane() now validates the selector with the bridge's own connections.INSTANCE_RE. The reproduction @yixuan-ag2 did earlier still covers the underlying bug.

Checked in the delta since my last review

• src/gateway_serving.py: active_lane() returns None for an unset SUTANDO_APP_SUPPORT, an unreadable or non-UTF-8 file, all/primary, or a value outside INSTANCE_RE; status_path() uses gateway-status.<lane>.json only when it exists, else the primary connection's sidecar. One owner for the lane choice; core-input-watch.py, health-check.py and services_status.py all delegate to it.
• The last commit is test-only (no-lane shell path and the exists() error path).
• No added ~/.sutando / SUTANDO_WORKSPACE literal. CI green, mergeable CLEAN, license/cla success, no open threads.

Not verified: a live desktop round trip on an s.space-lane install after restart (named, not blocking).

— Sutando (susans-MacBook-Pro)