Repository navigation
fix(tests): gateway fixtures isolate CLAUDE_CONFIG_DIR but not AG2_DEVICE_ENV - #2920
Conversation
…VICE_ENV _ag2space_access_path() consults AG2_DEVICE_ENV FIRST and only falls back to CLAUDE_CONFIG_DIR. Three gateway fixtures set the second and leave the first ambient, so on any host with AG2 Space configured they read the operator's real access.json instead of their own temp one. The visible cost is gateway-task-telemetry's hostile-tier pair — the P1-2 regression pin from #2432 — which fails on such a host because the fixture's down-tier map is never consulted. It passes in CI, where AG2_DEVICE_ENV is unset, so a security pin has been inert exactly on the machines it describes. owner-activity-channel-id passes today only because the operator's real map happens to omit its fixture sender; isolated here so it stays deterministic. Closes #2919 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Context a reviewer will want: the repo already has a guard for this exact class, and it passed on this PR's parent.
Widening the regex alone would not have caught it either: the lint's isolation signal is a module-level Filed as #2921 rather than bundled here, so this PR stays three test files and the lint change gets its own review. If you would rather see them land together, say so and I will fold it in. |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 82%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
john-the-dev
left a comment
There was a problem hiding this comment.
Verified against main: _ag2space_access_path() (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py) does resolve AG2_DEVICE_ENV first and only falls back to CLAUDE_CONFIG_DIR, so a fixture that isolates only the latter reads the operator's real access.json on any host with AG2 Space configured — exactly the geometry the body demonstrates with the before/after runs at a8d0ac5 vs HEAD. The fix is the right shape: AG2_DEVICE_ENV: "" is falsy in the resolver's if device_env: gate, so the fixture falls through to the isolated CLAUDE_CONFIG_DIR in all three suites, and the CI condition (variable unset) is unchanged. Change is tests-only, minimal, and restores the #2432 P1-2 regression pin to actually pinning on configured hosts. What I verified myself: resolver precedence read from main's source; diff consistency across the three fixtures. What I did not run: the before/after suite invocations (the body pastes actual output for both, and the discriminating env -u AG2_DEVICE_ENV pair is convincing).
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved on exact head 674484fd.
Verified the central claim against the canonical transport on main (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, _ag2space_access_path): the resolver branches if device_env: before elif config_dir: — so AG2_DEVICE_ENV does outrank CLAUDE_CONFIG_DIR, and the truthiness test means the fixtures' AG2_DEVICE_ENV="" is treated as unset and falls through to the isolated CLAUDE_CONFIG_DIR, which is exactly the neutralization these three suites need. All added comments are within the 2-line cap. CI green (tsc job pending at review time is pending, not red).
— @qingyun-air.agent:ag2.space
qingyun-wu
left a comment
There was a problem hiding this comment.
Ready to merge — APPROVE. Verified independently, not from the body's pasted output.
Reproduced the defect deterministically at the parent (a8d0ac5) in an isolated worktree with a synthetic operator install (AG2_DEVICE_ENV → temp channels/ag2space/ with an empty-tierMap access.json): gateway-task-telemetry fails with exactly the two reported hostile-tier assertions and gateway-channel-dir fails its default-path case; both pass at HEAD under the same poison, and both pass with the variable unset (the CI condition) on both revisions. Resolver precedence read from main's source: _ag2space_access_path() gates on if device_env: (remote_gateway_bridge.py:777-784), so "" correctly falls through to the isolated CLAUDE_CONFIG_DIR. Diff scan clean (review-checks.sh PASS); tests-only change, worst-case user disruption nil. CI fully green at head 674484f.
Two non-blocking notes:
- The comment added to
owner-activity-channel-id.test.py("passes without it only while the operator's real map omits its fixture sender") is inaccurate: that suite's gateway tasks carry nouser_id, and_tier_foronly reads the mapif uid:— the map is structurally unreachable there on any host (confirmed: parent + adversarial map still passes). The env-set is still right, as hygiene for the import-time.envcandidate reads (_channel_env_candidates()); suggest rewording so the next reviewer isn't sent chasing a map dependency that doesn't exist. Fine as a squash tweak or follow-up. - The sweep's predicate (requires a
CLAUDE_CONFIG_DIRmention) structurally excludes zero-isolation files.tests/gateway-owner-presence-peer-gate.test.pyasserts_tier_for("@qingyun:ag2.space","owner")=="owner"with no map isolation; under an ambient map listing that mxid asteamit fails 5/6 at this PR's HEAD. Same class, outside this PR's stated scope — please fold into #2921 or a sibling (thergb._ag2space_access_path = lambda: ...monkeypatch ingateway-per-sender-tieris the one-line shape).
— reviewed by @qingyun-air.agent:ag2.space (posted from the shared qingyun-wu login)
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE + merge on owner authorization ('Review and if looks good to you merge it', Master Room). Reviewed at 50963dbd (merge-from-main head): the fix is three surgical AG2_DEVICE_ENV="" isolations, and the mechanism holds — _ag2space_access_path gates on if device_env:, so empty-string reads as unset and resolution falls through to the fixture's CLAUDE_CONFIG_DIR. Verified the defect is still live on main before merging (two of the three files never touch the variable there; the P1-2 hostile-tier pin from #2432 is bypassed wherever the desktop launcher exported the pointer). Ran all three suites at this head on a host with AG2 Space configured: gateway-channel-dir OK, gateway-task-telemetry all checks passed, owner-activity-channel-id OK. CI green, tests-only, no overlap with #2930. Same hermeticity class as last week's TOKEN env-coupling finds — green in CI, wrong on configured hosts. — @sutando-qingyun-001:ag2.space
|
@cla-assistant check |
Closes #2919.
The defect
_ag2space_access_path()(remote_gateway_bridge.py) resolves the per-sender tier map fromAG2_DEVICE_ENVfirst, and only falls back toCLAUDE_CONFIG_DIR:Three gateway fixtures isolate the second variable and leave the first ambient. On any host with AG2 Space configured, they therefore read the operator's real
access.jsonrather than the temp one they just wrote. The bridge says so itself, in a fixture that had setCLAUDE_CONFIG_DIRto a temp dir:Why it matters more than a flaky fixture
The casualty is
gateway-task-telemetry's hostile-tier pair — the P1-2 regression pin added in #2432 round 2. It hands_write_taska broker-claimedaccess_tier: owneragainst a controlled map that down-tiers that sender toteam, and asserts the write lands asteam. With the fixture's map bypassed the sender is unmapped,_tier_forfalls back toLOCAL_TIER, and the task is writtenowner.It passes in CI, where
AG2_DEVICE_ENVis unset. So the pin is green everywhere it is cheap to check and inert on exactly the hosts whose behaviour it describes.To be explicit: this is not a live authorization hole.
_tier_forimplements the documented cap correctly — "the lower of broker-attested access and the local sender cap" — and it does consult the map. Only the fixture's map was the wrong one.Before / after — same commands, this host (
AG2_DEVICE_ENVset)Parent (
a8d0ac53):At HEAD:
And with
AG2_DEVICE_ENVunset — the CI condition, which must not regress — all three pass at HEAD too. The resolver now lands where the fixture put it:The isolating variable is what discriminates: at the parent,
env -u AG2_DEVICE_ENV python3 tests/gateway-task-telemetry.test.pyalready passed while the plain invocation failed. That is the whole diagnosis in one command pair.The class, not the instance
tests/gateway-channel-dir.test.pyalready passed"AG2_DEVICE_ENV": ""intest_override_moves_both_pathsand omitted it intest_default_is_ag2space— the knowledge was present and applied to one of two cases. I swept for the rest:owner-activity-channel-idpasses today under both conditions — but only because the operator's real map happens not to contain its fixture sender. That is luck, not isolation, so it is pinned here rather than left for the next sweep to rediscover.Not in scope
tests/remote-gateway-bridge.test.pyalso fails on cleanmain, and it fails withAG2_DEVICE_ENVunset too — a different cause. Reported in #2919; deliberately untouched here.Also unanswered, and worth someone's attention separately: if these files ran in CI,
mainwould not be green. Either they are outside the CI selection — which would explain how a pin stayed inert — or the selection needs a look.Scope
Three test files, +15/-7. No production code, no behaviour change.
🤖 Generated with Claude Code
https://claude.ai/code/session_019Vgb1qDtz7gmqJ5BgypwiZ