Repository navigation
feat(credentials): one owner for gateway credential precedence and masking (prerequisite for #5665) - #5698
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.pyLines 420-429 420 r = _systemctl(("--user", "show", "ag2-sparrow.service", "--property=ActiveState",
421 "--property=MainPID", "--property=ExecMainStartTimestampMonotonic"))
422 try:
423 return _parse_unit_props(r.stdout) if _ok(r) else None
! 424 except Exception: # noqa: BLE001 - any surprise in the answer is an unknown state
! 425 return None
426
427
428 def _parse_unit_props(stdout: str) -> "dict | None":
429 if not isinstance(stdout, str):Lines 521-529 521 exec_start = _find_console_script(SPARROW_COMMAND)
522 if not exec_start:
523 note(f"{SPARROW_COMMAND} was not found beside {sys.executable} or on PATH; no service "
524 "was set up. Reinstall ag2-sparrow for this Python and run install again")
! 525 return False
526 device_env = device_env_path()
527 try:
528 st = device_env.stat()
529 except OSError:Lines 585-596 585 f"systemctl --user show {SERVICE_NAME} then did not give a well-formed state; it stays "
586 f"stopped until you run: systemctl --user start {SERVICE_NAME}", file=sys.stderr)
587 return True
588 if props is None:
! 589 note(f"systemctl --user show {SERVICE_NAME} did not give a well-formed state, so whether it runs "
590 "is unknown; it was neither enabled, started nor restarted; run ag2-sparrow-hatch install "
591 "again once the user manager answers")
! 592 return False
593 state = props["ActiveState"]
594 active = state in ("active", "reloading")
595 main_pid = props["MainPID"]
596 ran = props["ExecMainStartTimestampMonotonic"] != 0Lines 604-614 604 _systemctl(("--user", "enable", "ag2-sparrow.service"))
605 if not _ok(r):
606 note(f"systemctl --user enable {SERVICE_NAME} failed ({_why(r)}); fix that and run "
607 f"systemctl --user enable --now {SERVICE_NAME}")
! 608 return False
609 if active and changed:
! 610 r = _systemctl(("--user", "restart", "ag2-sparrow.service"))
611 print(f"restarted: {SERVICE_NAME} with the new unit" if _ok(r) else
612 f"NOTE service: restart failed ({_why(r)}); run systemctl --user restart "
613 f"{SERVICE_NAME}")
614 print(f"enabled: {SERVICE_NAME}" + (" (running)" if start or active else " (not started)"))packages/ag2-sparrow/ag2_sparrow/bounded_file_read.pyLines 24-34 24 fd = os.open(path, os.O_RDONLY | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0))
25 except OSError as e:
26 if e.errno in _ABSENT:
27 return None, ""
! 28 return None, f"unreadable ({errno.errorcode.get(e.errno, 'error')})"
! 29 except (ValueError, TypeError):
! 30 return None, "not a usable path"
31 try:
32 info = os.fstat(fd)
33 if not stat.S_ISREG(info.st_mode):
34 return None, "not a regular file"Lines 41-54 41 break
42 chunks.append(chunk)
43 size += len(chunk)
44 if size > limit:
! 45 return None, f"over {limit} bytes" # grew past the bound after the fstat
46 return b"".join(chunks).decode("utf-8").replace("\r\n", "\n").replace("\r", "\n"), ""
47 except UnicodeDecodeError:
48 return None, "not UTF-8"
! 49 except OSError as e:
! 50 return None, f"unreadable ({errno.errorcode.get(e.errno, 'error')})"
51 finally:
52 os.close(fd)
53 packages/ag2-sparrow/ag2_sparrow/gateway_credential_io.pyLines 29-37 29 if os.path.isfile(os.path.join(cur, "src", marker)):
30 return os.path.join(cur, "src")
31 parent = os.path.dirname(cur)
32 if parent == cur:
! 33 return ""
34 cur = parent
35
36
37 def vault_lookup(src: str, vault_get=None) -> "Optional[Callable[[str], str]]":Lines 38-56 38 """The vault read for the ag2space token through the shared core `channel_token` in `src`
39 (monorepo_src's answer): total-failure-safe, never surfacing the value; None without one."""
40 try:
41 if not src:
! 42 return None
43 if src not in sys.path:
44 sys.path.insert(0, src)
45 from channel_token import token_from_vault
! 46 except Exception:
! 47 return None
48 return lambda key: token_from_vault(key, vault_get=vault_get)
49
50
51 def _vault() -> "Optional[Callable[[str], str]]":
! 52 return vault_lookup(monorepo_src("channel_token.py"))
53
54
55 def register_candidates(env: Mapping[str, str], registry: "gateway_credentials.SecretRegistry") -> None:
56 """Register every gateway credential candidate `env` leads to (the environment, the channelpackages/ag2-sparrow/ag2_sparrow/gateway_credential_sources.pyLines 20-29 20 from typing import Callable, Mapping, Optional, Sequence
21
22 try:
23 from . import gateway_credentials as contract # the package's generated copy
! 24 except ImportError:
! 25 import ag2_gateway_credentials as contract # the canonical shared/ module, when on the path
26
27 TOKEN_KEYS = ("REMOTE_TASK_TOKEN", "AG2_REMOTE_TOKEN")
28 URL_KEYS = ("REMOTE_TASK_URL", "AG2_REMOTE_URL")
29 TOKEN_FILE_KEY = "REMOTE_TASK_TOKEN_FILE"packages/ag2-sparrow/ag2_sparrow/gateway_credentials.pyLines 183-192 183 if len(e) > 1 and e[0] in "xuU":
184 return chr(int(e[1:], 16))
185 if len(e) == 3:
186 return chr(int(e, 8))
! 187 except (ValueError, OverflowError):
! 188 return m.group(0)
189 return _SYSTEMD_SIMPLE.get(e, e)
190
191
192 def systemd_unescape(text: str) -> str:Lines 450-458 450 url, _secret = parse_onboarding_token(value)
451 if credential_name(key) or url or (registry is not None and registry.contains(value)):
452 return WITHHELD
453 if "://" in value:
! 454 return redact_url(value)
455 return value
456
457
458 def _is_comment(line: str) -> bool:Lines 488-496 488 continue
489 groups.append(current)
490 current = []
491 if current:
! 492 groups.append(current)
493 return groups
494
495
496 def _join(group: "list[str]") -> str:packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.pyLines 556-567 556
557 def _token_from_vault_ag2space(vault_get=None):
558 """The vault tier (gateway_credential_sources.token_from_vault): the current name, then the
559 legacy AG2_REMOTE_TOKEN alias; '' on any failure."""
! 560 lookup = _vault_lookup(vault_get)
! 561 tok = gateway_credential_sources.token_from_vault(lookup)[0] if lookup else ""
562 if tok:
! 563 _announce_vault_token()
564 return tok
565
566
567 def _team_guard_fns():packages/ag2-sparrow/tests/test_hatch_adapter.pyLines 2074-2082 2074 real_proc = keepalive._proc
2075
2076 def read(pid, name):
2077 if name == "cmdline" and not Path("/proc").is_dir() and pid == proc.pid:
! 2078 return b"\0".join(a.encode() for a in self.old_argv) + b"\0"
2079 return real_proc(pid, name)
2080 return mock.patch.object(keepalive, "_proc", side_effect=read)
2081
2082 def wait_for(self, path):Lines 2455-2463 2455 def systemd(cmd):
2456 if cmd[1:] == ["--user", "show-environment"] and not next(answers, True):
2457 refused.append(cmd)
2458 return subprocess.CompletedProcess(cmd, 1, "", "Failed to connect to bus")
! 2459 return fake(cmd)
2460 real_which, stopped, spawned = shutil.which, [], []
2461 with mock.patch.object(hatch_cli.shutil, "which", side_effect=lambda name, *a, **k:
2462 "/usr/bin/systemctl" if name == "systemctl" else real_which(name, *a, **k)), \
2463 mock.patch.object(sm, "_run", side_effect=systemd_seam(systemd)), \skills/agent-room-ops/gateway_credentials.pyLines 182-192 182 try:
183 if len(e) > 1 and e[0] in "xuU":
184 return chr(int(e[1:], 16))
185 if len(e) == 3:
! 186 return chr(int(e, 8))
! 187 except (ValueError, OverflowError):
! 188 return m.group(0)
189 return _SYSTEMD_SIMPLE.get(e, e)
190
191
192 def systemd_unescape(text: str) -> str:Lines 450-458 450 url, _secret = parse_onboarding_token(value)
451 if credential_name(key) or url or (registry is not None and registry.contains(value)):
452 return WITHHELD
453 if "://" in value:
! 454 return redact_url(value)
455 return value
456
457
458 def _is_comment(line: str) -> bool:Lines 488-496 488 continue
489 groups.append(current)
490 current = []
491 if current:
! 492 groups.append(current)
493 return groups
494
495
496 def _join(group: "list[str]") -> str:src/bounded_file_read.pyLines 24-34 24 fd = os.open(path, os.O_RDONLY | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0))
25 except OSError as e:
26 if e.errno in _ABSENT:
27 return None, ""
! 28 return None, f"unreadable ({errno.errorcode.get(e.errno, 'error')})"
! 29 except (ValueError, TypeError):
! 30 return None, "not a usable path"
31 try:
32 info = os.fstat(fd)
33 if not stat.S_ISREG(info.st_mode):
34 return None, "not a regular file"Lines 41-54 41 break
42 chunks.append(chunk)
43 size += len(chunk)
44 if size > limit:
! 45 return None, f"over {limit} bytes" # grew past the bound after the fstat
46 return b"".join(chunks).decode("utf-8").replace("\r\n", "\n").replace("\r", "\n"), ""
47 except UnicodeDecodeError:
48 return None, "not UTF-8"
! 49 except OSError as e:
! 50 return None, f"unreadable ({errno.errorcode.get(e.errno, 'error')})"
51 finally:
52 os.close(fd)
53 Lines 53-58 53
54
55 def read_regular_text(path: str, limit: int = MAX_BYTES) -> Optional[str]:
56 """`path`'s UTF-8 text, or None when it is missing or refused (read_regular says which)."""
! 57 return read_regular(path, limit)[0]src/gateway_credential_sources.pyLines 34-42 34
35 def token_env_name() -> str:
36 """The token's environment variable by name, for a record that says where the token goes; another
37 module may name it only through this call, never read the variable with it."""
! 38 return TOKEN_KEYS[0]
39
40 # The precedence, stated once; the first source carrying a value wins (see discover() for each name:
41 # "file" is the first listed .env file carrying a token, "token" the URL inside the winning one).
42 SOURCES = (
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at 1c20ee4
Verdict: REQUEST_CHANGES. The split is right. The pure contract stays I/O-free, the owner opens nothing, and condition 2 (drift checks) is met. The finding-1 class is fixed, and a test fails when only the winner is registered. Not ready to merge, for four reasons:
- condition 1 (the structural guard) is not met;
- condition 3 is met only at the bridge;
- the live bearer's tail leaks through a systemd continuation line;
- the bridge's startup path has harness proof only.
The changes that would flip this to APPROVE are B1–B4 below.
Method: I verified the head with gh pr view 5698 --json headRefOid. The merge-base is the current origin/main tip, 5e632bf. I worked in a scratch worktree, now removed, with synthetic fixtures only and no .env, keychain or vault reads. review-preflight ran: no decisive review is on record, and all CI is green, including license/cla. The CI log shows tests/gateway-credential-sources.test.py was collected, in shard 5/9.
Blockers
B1. Condition 1 is not met: the guard matches literal key names, and its self-test cannot fail
Guard.test_no_other_package_module_reads_a_credential_source flags a .get/getenv/pop/setdefault call or a subscript only when the first argument is a string constant equal to a credential key. That is a name match done through the AST. It catches aliases of os.environ only because it ignores the receiver.
I dropped each probe below into ag2_sparrow/_rr_probe.py and ran the guard:
| Probe | Result |
|---|---|
os.environ.get("REMOTE_TASK_TOKEN") (control) |
CAUGHT |
e = os.environ; e.get("REMOTE_TASK_TOKEN") |
CAUGHT |
from os import environ; environ["REMOTE_TASK_TOKEN"] |
CAUGHT |
getattr(os, "environ")["REMOTE_TASK_TOKEN"] |
CAUGHT |
K = "REMOTE_TASK_TOKEN"; os.environ.get(K) |
MISSED |
os.environ.get("REMOTE_TASK_" + "TOKEN") / f-string key |
MISSED |
os.environ.get(g.TOKEN_KEYS[0]) (the owner's own constant) |
MISSED |
[v for k, v in os.environ.items() if k.endswith("TASK_TOKEN")] |
MISSED |
d = dict(os.environ); d[k] |
MISSED |
os.environ.__getitem__("REMOTE_TASK_TOKEN") |
MISSED |
Path("~/.config/ag2-sparrow/relay.env").expanduser().read_text() |
MISSED |
open(os.environ.get("AG2_DEVICE_ENV")).read() |
MISSED |
subprocess.run(["security", "find-generic-password", ...]) |
MISSED |
from channel_token import token_from_vault; token_from_vault("REMOTE_TASK_TOKEN") |
MISSED |
So the file reads and the keychain/vault calls that condition 1 named are not covered at all.
The self-test is vacuous. test_the_guard_sees_each_read_shape parses four snippets and asserts that each one's key is in SOURCE_KEYS. It never runs the detector on them. Mutation M1 replaced the detector's condition with if False:, and the suite still reported 35/35 OK. That is lesson 9: the instrument was never shown able to fire.
Ask:
- Factor the detector into a function, and have the self-test assert CAUGHT for each bypass row above.
- Flag every string constant equal to a credential key outside the owner, wherever it appears (including inside
BinOpandJoinedStr), and every use of the owner's key tuples outside the owner. - Allowlist by (module, function) the places that may do credential I/O:
open,read_textandread_byteson credential paths,import channel_token, andsecuritysubprocess calls. Today those are the bridge's_channel_env_candidates,_read_textand_vault_lookup, and Hatch's_read_text.
The design keeps I/O at the adapter edge, so the guard should pin where that I/O may happen, not ban it.
B2. The live bearer's tail leaks through a systemd continuation line
systemd joins a line that ends in \ with the next line. mask_env_assignments masks per physical line. The test used a registered (live) synthetic token Synth9Tok-en-Q7zZ-live:
'Environment=REMOTE_TASK_TOKEN=Synth9Tok-\\\nen-Q7zZ-live\n'
-> 'Environment=<withheld>\nen-Q7zZ-live\n'
The first line is withheld because shlex fails on it. The second line holds only a piece of the token, so registry.contains misses it, and 12 of the 22 characters print in the install diff. The same happens with an unregistered (retired) token whose "REMOTE_TASK_TOKEN=…" word sits on the continuation line: it prints whole.
Ask: fold \-continued lines into one logical line before masking, withhold every physical line of a withheld logical line, and add both cases to test_install_diff_masking_….
B3. Condition 3 is met only at the bridge, and SOURCES drives nothing
The same ROWS drive both real sinks, which is good. But the Hatch test asserts only that candidates are withheld.
_credential_registry()callsdiscover()and throws the result away, so no test shows that Hatch resolves a row to the same winner as the bridge.- Hatch also builds its own file list. It calls
channel_env_paths(env)with the default"ag2space"channel dir, while the bridge passesCHANNEL_DIR, which honoursREMOTE_TASK_CHANNEL_DIR. So the two sinks' source sets can already differ. That is the drift condition 3 exists to stop.
SOURCES is a data pin, not the precedence. discover() hard-codes the order.
- Mutation M2 reordered
SOURCESto("vault", "env", "file")and changed nothing that runs. Onlytest_the_table_is_stated_once(the literal) and the drift check failed. - Mutation M7 put the file tier before env, and the
ROWStests caught it. - So the effective spec is
ROWSplus the body ofdiscover(), andSOURCEScan go stale beside them. That is two statements of the precedence, not one.
Ask:
- Make
discover()iterateSOURCES, or derive each row's expected winner fromSOURCESin the test. - Have Hatch build its files through one owner helper that takes an injected reader and the channel dir, so both sinks share one candidate set.
- Assert in the Hatch sink test that Hatch's discovery equals each row's expectation, as the bridge test does.
"Same masked status" cannot be checked yet, because the bridge's own sinks are not routed through the contract here (the narrowed claim). Say in the body that this half of condition 3 lands in #5665.
B4. Live path: startup has harness proof only
This rewrites the bridge's import-time credential resolution: startup, auth header and token file. The child-process import test is good harness proof, but REVIEW.md lesson 15 and CONTRIBUTING require a real post-restart round trip. Please paste one: restart a Sparrow on a file-tier token, plus one on an env token, and show a task delivered end to end.
Checked and fine
-
Condition 2 (drift): met.
cmp src/gateway_credential_sources.py packages/.../gateway_credential_sources.pyshows the two are identical, and the test asserts it.- The package and skill contract copies differ from
shared/only by the 3-line GENERATED header. tools/sync_gateway_credentials.py --checkreturned "ok: 2 generated copies match canonical", andsync_from_src.py --checkreturned "in sync".- Mutation M6 (one character changed in the skill copy) fails the drift test.
-
Finding 1 class: fixed, and the test discriminates.
- Main's
adapters/hatch/cli.pydropped into the head tree fails the Hatch sink test on 6 of 6 rows, becauseenv-token-…,device-token-…,channel-token-…andretired-token-…print in the diff. - Mutation M3 (register only the winner) fails 16 tests, including both real-sink tests.
- Mutation M4 (Hatch registers files only, not env) fails 3.
- The wake-context half is #5665's, as the body says.
- Main's
-
A losing source's secret is masked: a losing env, device, channel or token-file token is registered and withheld (rows 1, 2, 5 and 6). The vault is the exception: it is read only when it wins, so a losing vault value is never held. That is acceptable, but see N6.
-
Purity: the contract and the owner have no
environ,open,read_text,getenv,subprocess,printorglobal. The module-level assignments are constants, regexes and the_EAItable built fromsocketconstants. The registry is per instance and lock-guarded. -
Masking that works. These fuzz cases all came out withheld:
- case variants, percent-encoding in upper and lower hex, and double encoding;
reprandunicode_escaperenderings;- systemd
Environment="A=x" "REMOTE_TASK_TOKEN=…" B=y(shown asA=x REMOTE_TASK_TOKEN=<withheld> B=y) and the single-quoted form; - URL userinfo, query and fragment, both under a key and inside
ExecStart; https://gw|tokand%7Conboarding strings under a neutral key.
Mutation M5b (words after the first shown raw) and mutation M8 (credential-name masking dropped) are caught.
-
Bridge strings: "token not in env; loaded from {path}", "token not in env or .env; loaded from vault" and the deprecation line are byte-identical to main, and print in the same order.
-
Architecture: the policy is in
src/, vendored, with I/O injected, and the adapters keep their reads. This matches "Shared adapter policy". -
Tests at head:
Suite Result tests/gateway-credential-sources.test.py35/35 test_hatch_adapter176 passed, 2 skipped test_env_token_quotes15 test_vault_token_tier7 test_gateway_status4 test_wheel_contents+test_pool_advertisement_push31 channel-env-order-contract6 gateway-channel-dir2 gateway-auto-reenroll39 credential-contract-golden37/37 gateway-proactive-room-from-channel-env11 gateway-channel-skip-ownerrc 0 src/remote-gateway-bridge.test.pyFAILED (8) on this host, but the same 8 fail on origin/main(the proactive/nudge delivery cases), and CI is green -
review-checks.shpasses on the diff; the control/Users/someone/foois flagged./home/x/relay.envis a fixture placeholder. Every new comment is at most 2 lines.
Nits (non-blocking)
-
N1. Masking gaps that need an unregistered (retired) secret, or an unusual escape.
KEY = vwith spaces is accepted by the owner's ownparse_env_text, so the parser and the masker disagree on what counts as an assignment.Environment = "REMOTE_TASK_TOKEN=…"(systemd accepts spaces around the=).- Credential names the regex misses:
PRIVATE_KEY,DB_PASS,BEARER,SESSION_COOKIE. - JSON and YAML lines,
--token <v>/--token=<v>inExecStart, and scheme-lessu:<secret>@host. - A registered token rendered with a JSON-escaped
\/or/also leaks.
-
N2. Undeclared bridge behaviour changes. The body says nothing changes beyond finding 1, but three things do:
_read_token_filenow swallowsUnicodeDecodeError. On main it propagated out of_reload_rotated_token; now that read means no rotation.REMOTE_TASK_TOKEN_FILEis now read at import, to register its contents.- With
REMOTE_TASK_TOKENunset andAG2_REMOTE_TOKEN='""', main printed the deprecation warning and this head does not.
All three are defensible; name them in the body.
-
N3. The guard covers the package only.
src/still resolvesREMOTE_TASK_TOKENitself insrc/health-check.py:4091(its ownREMOTE_TASK_TOKEN or AG2_REMOTE_TOKENprecedence),src/runtime-health.py:236andsrc/runtime-api/dispatcher.py:54. List them as follow-ups on this owner. -
N4. The purity test checks one file three times.
test_the_contract_is_purealways parsesshared/, whateverself.cis. It is safe because of the drift check, but it reads as covering three copies. -
N5. The kept wrappers test a path production no longer takes.
_token_from_ag2space_envand_token_from_vault_ag2spaceare kept for the legacy tests, but the import path now callsdiscover()directly. That is fine, as long as the real-sink test stays the production pin. -
N6. Hatch never registers the vault. In the "vault is last" row, the vault token under a neutral key still prints in the Hatch diff, and the test skips that case deliberately. This is acceptable where the vault is macOS-only; say so.
-
N7. A comment references an issue. The
_vault_lookupdocstring carries "(#2638)", which CLAUDE.md's comment rule says belongs in the commit message. It was carried over from main.
Worst-case disruption
The bridge's precedence is unchanged:
- all six
ROWShold through the real bridge; - the legacy env, vault, quote and channel-order suites pass;
- the only behaviour shifts are the ones listed in N2.
Worst case: a Sparrow starts with a different token than before, if some input falls outside ROWS. B4's live round trip is the mitigation.
Hatch now reads the installer's environment and device file during a diff. It only reads them, and a failed read gives "".
Evidence scripts are in the scratchpad: rr5698-fuzz.py, rr5698-mut.sh, rr5698-bridge-{head,main}.out.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at ef5ad0d
Verdict: REQUEST_CHANGES, narrowed. The code is close to approvable. All three of my conditions are now substantially met, and every blocker from the 1c20ee4 review moved except one: the live witness has not been run. REVIEW.md lesson 15 does not let me approve a startup-path rewrite on harness proof alone. I also found one small guard hole that I would like closed in the same push.
Two changes would flip this to APPROVE:
- R1: run the §15 witness at this head, with the plan amended as described below.
- R2: two one-line guard fixes.
Method:
- I verified the head with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf (theorigin/maintip). The two new commits are 76c3885 and ef5ad0d. - I used a scratch worktree (now removed) and synthetic fixtures only. Nothing read
.env, the keychain or the vault. - CI: 36 checks pass and 3 are skipped.
review-checks.shpasses on the diff. - The thread already holds one
CHANGES_REQUESTEDreview from this shared login at 1c20ee4. When this goes to APPROVE, replace that review rather than stacking a second one.
Blockers
R1. The §15 witness is planned but not run, and the plan needs three amendments
The body's plan is one restart per tier (env, file, vault, token file), plus a new PID and no token in the output. The shape is right, but it is marked "to be scheduled, not yet run". Lesson 15 asks for the run itself at this head before approval. Amend the plan so each run proves what it claims:
- Every tier needs a delivered task. As written, only tier 1 (env) delivers a task end to end. Tiers 2 to 4 only "authenticate". The file tier through
AG2_DEVICE_ENVis the desktop-spawned case the old code comment called "the case that matters". It needs a task flowing through the restarted bridge, not just a 200 on the first request. - The env tier has no startup line to name. The bridge prints a source line only for the file and vault tiers, so "the bridge names the env tier" cannot be observed. Say instead that no "loaded from" line is printed and the task is delivered. Do not add a new string to the bridge.
- Name the build. The plan says "at this PR's head … from the merged wheel". Say it is a wheel built from ef5ad0d, or from the final head if it moves. Record that sha with each run.
R2. The guard still misses a key built from a partly dynamic expression, and a reader handed to the owner as a lambda
I re-ran every bypass from my last review, plus some new shapes. I put each one in ag2_sparrow/_rr_probe.py and ran Guard.test_no_other_package_module_touches_a_credential_source.
Bypasses from the last review:
| Probe | Result |
|---|---|
a key held in a variable (K = "REMOTE_TASK_TOKEN"; os.environ.get(K)) |
CAUGHT |
a key built with + from constants |
CAUGHT |
g.TOKEN_KEYS[0] |
CAUGHT |
iterating os.environ.items() with an endswith("TASK_TOKEN") filter |
CAUGHT |
d = dict(os.environ); d[k] |
CAUGHT |
os.environ.__getitem__(...) |
CAUGHT |
Path("…/relay.env").read_text() |
CAUGHT |
open(os.environ.get("AG2_DEVICE_ENV")) |
CAUGHT |
a security find-generic-password subprocess, including when security is built in a variable |
CAUGHT |
from channel_token import token_from_vault |
CAUGHT |
the f-string from my last list: n = "TOKEN"; os.environ.get(f"REMOTE_TASK_{n}") |
MISSED |
The author's self-test covers f"REMOTE_TASK_{'TOKEN'}", which is an all-constant f-string. My shape interpolates a variable, and that is the one still missed.
The same root cause misses these too:
"REMOTE_TASK_" + n;"REMOTE_TASK_" + os.environ.get("SUFFIX", "TOKEN").
Cause: credential_access skips any constant whose parent is a BinOp, JoinedStr or FormattedValue. It then relies on _constant() to fold the whole expression, and _constant() returns None as soon as one part is not a constant. So the 12-character piece "REMOTE_TASK_" is never checked on its own.
Fix: skip a child only when its parent folds to a string, for example _constant(parent) is not None.
Also missed: a lambda reader handed to the owner. For example: g.discover(os.environ, [], read_token_file=lambda p: open(p).read()).
- The reader check only looks at
Namenodes starting with_read. - The scope check never marks
open(p)here, because the lambda's argumentpcarries no credential marker. - That is exactly the seam the injected-I/O design creates.
Fix: flag any Lambda passed as read_token_file or vault_lookup (or as env_files's reader) whose body is not a call to an allowlisted name. The bridge's own vault_lookup=lambda key: (_vault_lookup() or …)(key) stays allowed.
Add both shapes to test_the_detector_catches_every_bypass.
Still missed, and acceptable for an AST guard: I record these but do not ask for them.
- Keys built as
"_".join([...]),b"…".decode(),"%s" %formatting or[::-1]. io.FileIO.- The
keyringmodule,importlib.import_module("channel_" + "token")andosascript. Path(p).read_text()on an untainted argument.
None of these appears in the package today.
The other checks
The if False: detector mutant is now caught. Three disabling mutants each fail Guard.test_the_detector_catches_every_bypass:
- the key check set to
if False:; credential_accessreturning[]straight away;- the I/O check set to
if False:.
The vacuous self-test from the last review is fixed.
Reordering SOURCES changes the winner at both sinks.
- On row 1 (env, device file and channel file), the unmodified table gives
env-token-AAAA1111from env at both the bridge and Hatch. - With the package
SOURCEStoken tier reordered to("file", "env", "vault"), both givedevice-token-BBBB2222from the file tier. - The full suite under that mutant fails 12 tests, including both real-sink tests and the row test.
discover()now walksSOURCES, so the table is the precedence. Script:rr5698-reorder.py.
The continuation leak (B2) is gone. Each of these is now fully withheld:
- the split credential-named value
Environment=REMOTE_TASK_TOKEN=Synth9Tok-\+en-Q7zZ-live, which printsREMOTE_TASK_TOKEN=<withheld> <withheld>; - a credential word on the continuation line, registered or retired;
- the unregistered split value.
Mutation M9 (folding disabled) fails the masking test. One residual is left (N1).
Other mutants (rr5698-mut.sh; the guard mutants were run by hand):
| Mutant | Tests failing |
|---|---|
| register only the winner | 23 |
| Hatch registers files only | 6 |
only the first Environment= word masked (shared) |
caught by the drift check |
| one character changed in the skill copy | 1 (drift) |
| credential-name masking dropped | 10 |
| whole-value registration dropped | 23, including test_a_url_shaped_bearer_is_registered_whole |
env_files ignores the channel directory |
3, at both sinks |
Fuzz (rr5698-fuzz.py). These leaked at 1c20ee4 and are now masked:
KEY = v;Environment = …;- both continuation forms;
PRIVATE_KEY,DB_PASS,BEARERandSESSION_COOKIE.
The remaining leaks are listed under N2.
Conditions 2 and 3.
- Drift: the owner copies are byte-identical (
cmp), both--checktools pass, and a one-character change to the skill copy is caught. - Hatch now builds its files through the shared
env_files/channel_dir. - The Hatch sink test asserts the same row resolution as the bridge, except where the vault wins (Hatch has no vault, which the body documents).
- The masked-status half of condition 3 is explicitly moved to #5665 in the body. I accept that narrowing.
Finding 1 is still fixed, and the test still discriminates (23 failures when only the winner is registered).
Purity still holds. test_the_contract_is_pure now parses each copy's own file (Path(self.c.__file__)), which fixes the old N4.
Bridge behaviour.
- A file value is unquoted once again; the 1c20ee4 double unquote is fixed and pinned by
test_a_file_value_is_unquoted_once. - The deprecation warning prints whenever the old name is set and the new one is not, which matches main's
_env_compat. - The startup strings are unchanged.
- The body now lists the three behaviour changes: a non-UTF-8 token file means no rotation, the token file is read at import, and the warning keeps main's behaviour. The non-UTF-8 case has a real-bridge test.
Test runs at this head:
| Suite | Result |
|---|---|
tests/gateway-credential-sources.test.py |
42/42 |
test_hatch_adapter |
176 passed, 2 skipped |
| env-quotes, vault-tier, gateway-status, wheel-contents, pool-advertisement | 57 |
channel-env-order-contract |
6 |
gateway-channel-dir |
2 |
gateway-auto-reenroll |
39 |
credential-contract-golden |
37/37 |
gateway-proactive-room-from-channel-env |
11 |
gateway-channel-skip-owner |
rc 0 |
census-d1-anchors |
4 |
src/remote-gateway-bridge.test.py |
FAILED (8) on this host |
The 8 bridge-test failures are the same 8 that fail on origin/main (diffed), and CI is green.
Nits from the last review, rechecked:
| Nit | Status |
|---|---|
| N1 masking gaps | mostly fixed; residual in N2 |
| N2 undeclared behaviour changes | fixed (listed in the body) |
N3 src/ readers |
listed as a linked follow-up |
| N4 purity test reads one copy | fixed |
| N5 legacy wrappers | unchanged, fine |
| N6 Hatch and the vault | documented |
N7 (#2638) in a docstring |
removed |
All added comments are within 2 lines (the prose-cap check passes).
Nits (non-blocking)
N1. A fragment of a registered token survives on a continued line whose key is neutral or which is not an assignment.
| Input | Output |
|---|---|
Environment=X=Synth9Tok-\ + en-Q7zZ-live |
Environment=X=Synth9Tok- <withheld> |
ExecStart=/x Synth9Tok-\ + en-Q7zZ-live |
printed raw: the joined text has a space between the pieces, so contains misses it |
The same happens with CRLF line endings, a # comment inside the continuation, and an empty continued line.
systemd replaces the backslash with a space, so in every one of these cases the effective configuration does not hold the full token. That is why this is a nit and not a blocker.
Cheap fix: also test the joined logical line with all whitespace removed against the registry, and withhold the whole logical line on a hit.
N2. Masking leaks that remain (from my last list), for a retired, unregistered secret:
- JSON or YAML lines;
--token v/--token=vinExecStart;- a scheme-less
u:<secret>@host; - a credential assignment inside a quoted value (
Environment="NOTE=see REMOTE_TASK_TOKEN=…"); - a percent-encoded key name.
For a registered secret: \/ and / renderings, and a secret split across two separate (non-continued) lines.
An install diff over a unit or .env file rarely contains these. Fine as follow-ups.
Worst-case disruption
The precedence is unchanged on every row, now including REMOTE_TASK_CHANNEL_DIR and a URL-shaped bearer. The double unquote that 1c20ee4 introduced is gone.
The remaining risk is an input outside the rows that starts a Sparrow with a different token. The R1 witness is the mitigation. Hatch's new reads are read-only and fail to an empty value.
Evidence (scratchpad): rr5698-fuzz.py, rr5698-mut.sh, rr5698-guard-probes.py, rr5698-reorder.py, rr5698-v2-bridge-fails.out (versus rr5698-bridge-main.out).
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at e92c073
Verdict: COMMENT. The code is approvable at this head. The APPROVE follows the per-tier §15 witness, run at this exact head (REVIEW.md lesson 15). No code blocker remains. Nothing is posted.
When the APPROVE is posted, it replaces the two CHANGES_REQUESTED reviews from this shared login, at 1c20ee4 and ef5ad0d. It does not stack on top of them.
Method
- Head. I checked it with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf. - What changed. One commit since ef5ad0d (e92c073), and it touches only
tests/gateway-credential-sources.test.py.git diff --quiet ef5ad0d7 HEAD -- src shared skills packagesis clean, so my ef5ad0d findings on the code still stand. - Checks.
- CI: 36 checks pass and 3 are skipped.
review-checks.shpasses.- Both
--checksync tools pass. - The suite passes, 42/42.
- Data. A scratch worktree, now removed. Synthetic fixtures only; no
.env, keychain or vault reads.
The guard (R2 from v2): fixed
Both of the new rules work:
- A constant inside a
+or f-string is skipped only when the whole expression folds to a constant. - A reader handed to
discover()orenv_files(), including a lambda's body, may name only allowlisted readers or its own parameters.
The self-test now runs 18 bypass shapes.
Old probes (rr5698-guard-probes.py) are all caught now. These include:
- the partly dynamic f-string
n = "TOKEN"; os.environ.get(f"REMOTE_TASK_{n}"), which is the one still missed at ef5ad0d; "REMOTE_TASK_" + n;"REMOTE_TASK_" + os.environ.get("SUFFIX", "TOKEN");- the lambda reader
discover(..., read_token_file=lambda p: open(p).read()); - every other probe from the 1c20ee4 list.
New reader variants (rr5698-guard-probes-v3.py), each placed as a package module:
| Variant | Result |
|---|---|
a lambda wrapping an allowlisted reader with extra I/O (lambda p: _read_text(p) or open(p).read()) |
CAUGHT |
the same lambda with a subprocess.run side effect |
CAUGHT |
functools.partial around a lambda that opens the file |
CAUGHT |
functools.partial(_read_text, x=1) |
CAUGHT |
a nested def rd(p) passed by name |
CAUGHT |
a nested def _read_text that shadows the allowlisted name, inside an allowlisted module |
CAUGHT (by its enclosing function's I/O scope) |
| a reader bound to a variable first | CAUGHT |
self._read_text method reader |
CAUGHT |
env_files(env, read_text=lambda p: open(p).read()) (the reader passed as a keyword) |
MISSED |
from .gateway_credential_sources import discover as d; d(..., read_token_file=lambda p: open(p).read()) |
MISSED |
discover(env, [], **{"read_token_file": lambda p: open(p).read()}) |
MISSED |
The guard's own mutants. Each mutant below fails Guard.test_the_detector_catches_every_bypass:
- G1: the key check set to
if False:; - G2: the detector returning
[]straight away; - G3: the I/O check set to
if False:; - G4: the fold rule reverted;
- G5: the reader rule reverted to
_read*names only; - G6: the reader rule disabled.
Code mutants still fail as at ef5ad0d:
| Mutant | Tests failing |
|---|---|
SOURCES reordered |
12, and the winner changes at both the bridge and Hatch |
| registering only the winner | 23 |
| continuation joining disabled | 2 |
| one character changed in the skill copy | 1 (the drift check) |
Fuzz. rr5698-fuzz.py gives the same 11 residual leaks as at ef5ad0d, all of them nits listed in v2. The code is unchanged since then, and the continuation fix still holds.
Before the APPROVE: the §15 witness at e92c073
The body's plan is still marked "to be scheduled, not yet run". It is also unchanged from ef5ad0d, so the three amendments from v2 still apply:
- Every tier delivers a task end to end, not only env. The file tier through
AG2_DEVICE_ENV(the desktop-spawned case), the non-defaultREMOTE_TASK_CHANNEL_DIRrun, the vault and the token-file rotation each show a task flowing through the restarted bridge. - The env tier is shown by its absence. No "loaded from" line prints, and the task is delivered. The bridge prints no env-tier line, and none should be added.
- The build is named. Use a wheel built from e92c073, or from the final head if it moves, with the sha recorded per run and a new PID per restart. Also show that the token appears in no startup or status output.
If the head moves before the witness, the witness moves with it.
Nits (non-blocking, follow-up is fine)
N1. Three dispatch shapes slip past the reader rule (the three MISSED rows above):
env_fileswith its reader as a keyword.handedreads onlyargs[1:2]forenv_files. Also readcall.keywordswitharg == "read_text".- The owner imported under an alias. The callee name is matched literally. Resolve
ImportFromaliases ofdiscoverandenv_files. **kwargs. Flag any**argument to the owner.
No module does any of these today.
N2. The insides of an allowlisted reader are not inspected. An allowlisted _read_text that also calls the security command, or opens os.environ["AG2_DEVICE_ENV"] directly, passes. Allowlisting by (module, function) means exactly this, so it is acceptable. Keep those readers to one line, as they are now.
N3. Obfuscated keys still pass. "_".join([...]), b"…".decode(), % formatting, [::-1], io.FileIO, keyring, importlib.import_module("channel_" + "token") and osascript are not caught. That is fine for an AST guard.
N4. v2's N1 and N2 still stand.
- N1: a fragment of a registered token can print on a continued line whose key is neutral, or which is not an assignment. systemd never sees the full token there.
- N2: these renderings still leak:
- JSON or YAML lines;
--tokenflags;u:<secret>@hostwith no scheme;- an assignment inside a quoted value;
- a percent-encoded key name;
- a registered token written with
\/or/, or split across two ordinary lines.
Worst-case disruption
Unchanged from v2. The precedence is pinned by eight rows at both real sinks, and the non-default channel directory is covered. The remaining risk is an input outside those rows that starts a Sparrow with a different token. The per-tier witness above is the mitigation, and it gates the APPROVE.
Evidence (scratchpad): rr5698-guard-probes.py, rr5698-guard-probes-v3.py, rr5698-mut.sh, rr5698-fuzz.py, rr5698-reorder.py.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Review of exact head e92c073cb5934dbbaaedf2394e663cf2d7f7e8f5 (merge-base 5e632bf7267338db1c47a080879f99d5ae57e741).
Verdict: REQUEST_CHANGES. CI and CLA are green, and the intended precedence/unquoting/non-UTF-8 improvements largely work, but the credential-masking contract is still incomplete and the startup rewrite still lacks the required live witness.
Merge blockers
-
A systemd comment block inside a continued credential assignment exposes the suffix.
shared/ag2_gateway_credentials.py:374-383ends the logical group at every physical line that does not itself end in\. That does not match systemd: comments following a continuation are ignored and continuation resumes at the next non-comment line (systemd.syntax). The generated copies inherit the same defect.Synthetic input (the first line ends in a literal backslash):
Environment="REMOTE_TASK_TOKEN=retired-first-9382\ # comment retired-last-8372"Exact-head output from
mask_env_assignments, also reproduced through Hatch's real_install_filediff path:Environment=<withheld> retired-last-8372"The suffix must not survive. Preserve the continuation across intervening comment blocks, regenerate both copies, and pin the real sink with a discriminating regression.
-
Explicit token-file discovery does not register every materialized candidate.
src/gateway_credential_sources.py:117-145,209-214reduces token-file assignments to the winning alias (and last duplicate) before registration. With:REMOTE_TASK_TOKEN=current-file-9821 AG2_REMOTE_TOKEN=legacy-file-7364and an environment token winning overall, exact-head output is:
registry.contains("current-file-9821") -> True registry.contains("legacy-file-7364") -> False mask_env_assignments("Environment=NOTE=legacy-file-7364\n", registry) -> "Environment=NOTE=legacy-file-7364\n"Thus a discarded credential that was actually read can still print in the Hatch diff. This contradicts the PR's “register every materialized candidate before it picks the winners” contract. Register all parsed token/URL assignments, including losing aliases and overwritten duplicates, before selecting winners.
-
The R2 structural guard still misses valid injected-reader forms.
tests/gateway-credential-sources.test.py:513-522does not collectenv_files(..., read_text=...), and it exempts every lambda parameter name even when a default binds that name to forbidden I/O. Both probes return[]:env_files(env, read_text=lambda p: open(p).read()) discover(env, files, read_token_file=lambda p, open=open, read=None: open(p).read())
The self-test is non-vacuous, but it does not exercise either form. Cover the keyword reader and make lambda binding analysis scope-aware so the guard enforces the ownership boundary it claims.
-
The new eager read can hang bridge startup on a non-regular credential path.
remote_gateway_bridge.py:1659-1671uses unboundedPath.read_text()during import, even when a valid environment token already wins. WithREMOTE_TASK_TOKEN_FILEnaming a synthetic FIFO with no writer and a valid env token/URL, the same sanitized import probe produced:base: completed, rc=0, stdout="imported" head: timed out importing bridge after 1.0sEager registration of regular files is the declared behavior; blocking forever on a FIFO/device is not. Reject or safely bound non-regular/blocking sources before the read and add an import regression.
-
The required real post-restart witness is still absent.
The PR body still says “to be scheduled, not yet run.” Before approval, run it at the final exact head/build: each env, device/channel file (including non-default channel dir), vault, and token-file rotation tier must deliver a task end to end after a real restart; record the build SHA and new PID; for the env tier prove delivery plus the expected absence of a “loaded from” line; and show no credential appears in startup/status output. Harness results do not substitute for this live-path evidence under
REVIEW.mdlesson 15.
Verified
python3 tests/gateway-credential-sources.test.py: 42/42 pass.- Remote bridge, credential golden, vault, env-quote, and Hatch suites pass in a sanitized synthetic environment (Hatch: 176 pass, 2 skip).
- Both sync checks,
review-checks.sh, andgit diff --checkpass; the worktree stayed clean. - Whole URL-shaped bearer registration, SOURCES-driven precedence, exactly-once unquoting, and non-UTF-8 no-rotation behavior pass independent probes.
- Current GitHub CI and CLA are green.
Worst case is a live or retired credential printed in an install diff, or every bridge restart hanging on an existing non-regular path. Fixing the four code/test blockers and supplying the exact-final-head live witness would change my verdict. All review probes used synthetic values; I did not restart a service or touch live credentials.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at 7811176
Review action: COMMENT (the §15 witness still waits for the owner's go). Merge-readiness: not approvable at this head. Two code blockers are open.
All four claimed fixes work, and each fails its test when reverted. The FIFO hang is gone. But the two things I was asked to probe both turned out to be real:
- B1: the "known gap" about a repeated
.envkey is a leak. - B2: a refused credential file silently changes which credential wins.
Method
- Head. I checked it with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf. The only commit since e92c073 is 7811176. - Workspace. A scratch worktree, now removed. Synthetic fixtures only; no
.env, keychain or vault reads. - Checks.
- CI: 36 checks pass and 3 are skipped.
review-checks.sh: pass.- Both
--checksync tools: pass. src/bounded_file_read.pymatches its vendored copy byte for byte.
- Prior reviews on the thread.
Blockers
B1. A key repeated inside one channel .env leaves its earlier value unregistered, and the Hatch install diff prints it
parse_env_text builds a dict, so the last assignment wins. discover() registers only values.get(key) from each file. The earlier value was read from the host's own credential file, yet it is never registered.
I wrote a device .env holding:
REMOTE_TASK_TOKEN=early-dup-token-1111
REMOTE_TASK_URL=…
REMOTE_TASK_TOKEN=late-dup-token-2222
The installed unit carried both tokens under neutral keys. The real _install_file diff showed:
-Environment=NOTE=early-dup-token-1111 <- printed
-Environment=NOTE2=<withheld>
-ExecStart=<withheld>
| File holding the duplicate | Early value in the diff | Late value in the diff | Bridge registry (early / late) |
|---|---|---|---|
device .env |
printed | withheld | False / True |
channel .env |
printed | withheld | False / True |
This is the same defect class this PR fixed for the token file (fix 2) and for finding 1: a credential the host read but never registered. The install diff is the one production sink so far.
The other sinks:
safe_status,code_field,redact_textandscrubhave no production callers at this head.describenever renders exception text.- So the install diff is the only exposure today. Once #5665 routes the bridge's sinks through
_SECRETS, those sinks inherit the same hole.
So this is a leak, not a gap.
Fix: register every assignment of a credential key in each channel .env, the same way _assignment_pairs now does for the token file. Then add the duplicate-key fixture to both real-sink tests.
The fix could have env_files carry the duplicate-preserving pairs, or the raw text, next to values. Precedence is unchanged by this: the last value still wins.
Probe: rr5698-dupkey.py.
B2. A refused credential file is treated as absent without a word, so the winner changes silently
read_regular_text returns None for every outcome:
- missing;
- not a regular file;
- over 1 MiB;
- not UTF-8;
- an OSError.
env_files then skips the file, and discover() moves on to the next tier.
I wrote a device file holding device-refused-tok-3131 and a channel .env holding channel-fallback-tok-4242, then ran the real bridge in a child process:
| Device file | Winner | Does stderr name the refused file? | Hatch diff prints the device token? |
|---|---|---|---|
| over 1 MiB | channel token | no | yes |
| not UTF-8 | channel token | no | yes |
| FIFO | channel token | no | (not run) |
In each case the only line is token not in env; loaded from <channel .env>.
Compared with main:
- Over 1 MiB: main read the file, so the device token won. This head silently connects with a different token, which may be a different identity. That is the wrong-identity hazard the bridge's own
AG2_DEVICE_ENVcomment warns about. - FIFO: main hung. Not hanging is right, but falling through without a word is not.
- Not UTF-8: main also skipped silently. So this one is pre-existing, but it is the same path and should get the same report.
- Hatch: a refused file's tokens are never registered, so the install diff prints them. The table above shows this for the over-1 MiB and not-UTF-8 cases.
Fix:
- Have the reader report why it refused (for example, return a reason next to
None, or raise a typed refusal). - In the bridge, print one stderr line naming the path and the reason class. Never print the content.
- In Hatch, print a matching
NOTE.
Recommended (REVIEW.md lesson 4, fail closed):
- When the refused file is the explicitly named
AG2_DEVICE_ENV, do not fall through to another file's token. Stop the file tier with that message. - At minimum, mention the refusal next to the fallback line, so "loaded from X" is never the only clue.
Add a real-bridge test that asserts the message and the chosen behaviour for the over-1 MiB and FIFO cases.
Probe: rr5698-refused.py.
The four claimed fixes: verified
| Fix | Test(s) that fail when it is reverted |
|---|---|
| 1. A comment line inside a continuation does not end it | Reverted in the package contract: test_a_comment_inside_a_continuation_does_not_end_it, test_the_hatch_install_diff_masks_past_a_comment_in_a_continuation, plus the drift check (3 failures). Reverted in shared/: the shared copy's test plus the drift check. |
| 2. Every token-file assignment is registered | test_every_token_file_assignment_is_registered, test_the_hatch_install_diff_withholds_a_losing_token_file_alias, plus the drift check |
3a. The guard catches read_text= |
Guard.test_the_detector_catches_every_bypass |
| 3b. The guard catches I/O bound in lambda defaults | Guard.test_the_detector_catches_every_bypass |
| 4. One bounded, non-blocking reader | Fix reverted (open without O_NONBLOCK): test_a_fifo_credential_path_never_hangs_the_bridge_import errors on both REMOTE_TASK_TOKEN_FILE and AG2_DEVICE_ENV, and BoundedRead fails |
FIFO at head: read_regular_text on a FIFO with no writer returns None in 0.001 s.
The reader's newline handling matches the old text-mode reads. \r\n and \r become \n, which is what Path.read_text and open() already did. No value changes.
Probes and mutants re-run
-
The suite passes, 51/51.
-
rr5698-guard-probes.py: all 15 targeted probes are caught, including every one from the 1c20ee4 list, the partly dynamic f-string and concatenation, and the lambda reader. The other 11 misses are the obfuscation and residual shapes already accepted in v3. -
rr5698-guard-probes-v3.py:env_files(env, read_text=lambda …)is now caught. Still missed:discoverimported under an alias;- a reader passed through
**kwargs. - These are v3 N1 and remain nits.
-
Unchanged from v3:
- the guard and code mutants (detector disabled, SOURCES reordered at both sinks, only-winner registration, continuation join, skill-copy drift);
- the fuzz, which still shows the same 11 residual nits.
-
Existing suites:
Suite Result test_hatch_adapter+ env-quotes + vault-tier + gateway-status + wheel-contents + pool-advertisement233 passed, 2 skipped channel-env-order 6 gateway-channel-dir 2 auto-reenroll 39 credential-contract-golden 37/37 proactive-room-from-channel-env 11 channel-skip-owner rc 0 census-d1-anchors 4 src/remote-gateway-bridge.test.pythe same 8 failures as origin/main(diffed); CI is green
Nits (non-blocking)
These carry over from v3:
- N1: an aliased
discoverimport, and readers passed through**kwargs. - N2: the insides of an allowlisted reader are not inspected.
- N3: obfuscated keys still pass.
- N4: fuzz residuals, such as JSON or YAML lines and
--tokenflags.
Before the APPROVE
- Fix B1 and B2, with the tests above.
- Run the per-tier §15 witness at the final head, with the three amendments from v2:
- every tier delivers a task end to end;
- the env tier is shown by the absence of a "loaded from" line;
- the wheel is built from the sha under review, with a new PID per restart.
- Add one witness row: an oversized or FIFO
AG2_DEVICE_ENVwith a channel.envpresent, showing the B2 message and behaviour.
Worst-case disruption
B2 is now the worst case: an oversized or otherwise refused AG2_DEVICE_ENV makes a Sparrow connect, without a word, using another file's token. That could be the wrong identity, the failure this code path has guarded against since the bare-home rule.
B1 exposes an old token in the installer's diff output.
Both need small fixes, and every other behaviour is pinned.
Evidence (scratchpad): rr5698-dupkey.py, rr5698-refused.py, rr5698-blockread.py, rr5698-guard-probes.py, rr5698-guard-probes-v3.py, rr5698-mut.sh, rr5698-fuzz.py, rr5698-v4-bridge-fails.out.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at da7a847
Verdict: REQUEST_CHANGES, for one blocker. Both of my v4 blockers are fixed, and each fix has tests that fail when it is reverted. But the fail-closed change breaks an existing pinned test on main's contract, and CI is red at this head. With no blocker this would have been a COMMENT (code approvable, APPROVE after the §15 witness). Once B1 is resolved it goes back to that.
Method
- Head. I checked it with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf. The only commit since 7811176 is da7a847. - Workspace. A scratch worktree, now removed. Synthetic fixtures only; no
.env, keychain or vault reads. - CI: 2 checks fail, 33 pass, 4 are skipped.
python standalone tests (5/9)failed ontests/gateway-proactive-room-from-channel-env.test.py.tsc + tests (clean install)fails only as the aggregate of that failure.
- Other checks.
review-checks.shpasses, and both--checksync tools pass.
Blocker
B1. Failing closed on a refused AG2_DEVICE_ENV breaks a contract main pins, and CI is red
The failing test is test_an_undecodable_candidate_does_not_shadow_the_next in tests/gateway-proactive-room-from-channel-env.test.py.
- It passed at 7811176: 11/11 in my v4 run.
- Its setup: a non-UTF-8
AG2_DEVICE_ENVfile, and a readable channel.envholdingREMOTE_PROACTIVE_ROOM. - What it asserts: the bridge still takes
PROACTIVE_ROOMfrom the channel.env. - At this head:
''!='!ownerdm:example.org'.
Cause:
env_files()nowbreaks on a refused device file, so the channel.envis never read.- That file feeds the config reads (
_config_from_channel_env:REMOTE_PROACTIVE_ROOM,REMOTE_PROACTIVE_TRUST_OKand the rest) as well as the token tier.
The body is also incomplete here. It says "On main a directory there was skipped". But main also skipped a non-UTF-8 device file, and this test pins that.
Pick one and own it:
- (a) Keep the fail-closed contract. Update this test deliberately: a refused named device file now means
FATALand no config from later files. Say in the body that main's "undecodable device file is skipped" contract is replaced. - (b) Narrow fail-closed to the credential tiers. Still list the later files for config, but stop the file and vault token tiers, and keep
fatal.
Either way, CI must be green at the head that is reviewed.
v4 blockers: both fixed
B1 (a repeated key leaks). rr5698-dupkey.py at this head:
| File holding the duplicate | Early value in the Hatch diff | Late value in the Hatch diff | Bridge registry (early / late) |
|---|---|---|---|
device .env |
withheld | withheld | True / True |
channel .env |
withheld | withheld | True / True |
The winner is still the last value, late-dup-token-2222. Repeats and export lines both register, through parse_env_pairs and _assignment_pairs.
B2 (refusal is silent). rr5698-refused.py and rr5698-fatal.py:
| Scenario | Bridge | Hatch |
|---|---|---|
refused AG2_DEVICE_ENV (over 1 MiB, not UTF-8 or FIFO), no env token |
winner none; stderr names the path and reason; main() exits FATAL: AG2_DEVICE_ENV names <path>, which was refused (…); no "loaded from"; no token in the output |
credentials_usable() False, NOTE credentials: …, REFUSED: …; _install_file prints (diff withheld: …); the device token is not printed |
refused AG2_DEVICE_ENV, env token set |
the env token is resolved, but main() still exits FATAL (declared: "even when an env token is set") |
install refused, diff withheld |
refused default channel .env (FIFO), device token present |
device token wins; the refusal is named on stderr | NOTE |
The token file: what happens today, and a recommendation
rr5698-fatal.py, scenarios C to E:
| Scenario | Startup winner | Stderr |
|---|---|---|
C. readable REMOTE_TASK_TOKEN_FILE, token in the device file |
device token (source=file) |
nothing |
| D. the same token file refused (not UTF-8) | the same device token | credential file <token> was refused (not UTF-8); nothing in it was read |
| E. refused token file, env token set | the env token, as with a readable file | the same note |
In D and E, TOKEN_FILE stays set to the refused path, and Hatch prints only a NOTE.
At startup, a refused token file does not change which token wins. The token file is not a startup tier: the token comes from env, then the file tier, then the vault. The file is the rotation channel only.
The cost comes later.
_reload_rotated_token()re-reads the file throughread_regular_text(...) or "". A refused file reads as"", so it does not rotate, with no further message.- After a 401,
_recover_authsleeps in its recheck loop, waiting for a rotation it can never read. - The only clue is the one line printed at import, possibly hours earlier.
- If the file becomes refused after startup (rewritten over 1 MiB, or replaced by a FIFO), there is no clue at all.
Recommendation: yes, fail closed. I agree with the coordinator, for a reason that is slightly different from the precedence argument. REMOTE_TASK_TOKEN_FILE is explicitly named, like AG2_DEVICE_ENV, and a refused one silently disables the operator's recovery path.
- At startup, a refused
REMOTE_TASK_TOKEN_FILEgivesDiscovery.fatal: the bridge exits FATAL and Hatch refuses the install, the same asAG2_DEVICE_ENV. - On reload, use
read_regularand name a refusal (path and reason, no content) in the auth-recheck log. Rate-limit it to one line per change of reason. Never treat it as "no rotation yet". - Pin both with a real-bridge test, and add the case to the Behaviour changes section of the body.
If the author would rather keep it a warning, (2) alone is the minimum, so a refused rotation file is never silent at the moment it matters.
Probes and mutants re-run
The suite passes, 63/63.
Mutants added in this round. Each fails tests when reverted:
| Mutant | Tests failing |
|---|---|
| R1: repeated assignments not registered | 4 (including the real Hatch diff) |
| R2: refused device file does not block | 8 |
R3: discover ignores blocked |
6 |
R4: the bridge's main() ignores fatal |
4 |
| R5: the bridge prints no notes | 5 |
R6: the Hatch diff is not withheld on fatal |
2 |
R7: Hatch install ignores credentials_usable |
2 |
| R8: the reader reports a refusal as absence | 5 |
| R9: a token-file refusal is not noted | 2 |
This matches the author's report that 9 revert mutants fail.
Earlier mutants, all still caught:
- the detector key check turned off: the self-test fails;
SOURCESreordered: 13 tests;- the continuation join turned off: 4;
- skill-copy drift: 1.
Probes:
rr5698-guard-probes.py: all 15 targeted probes are caught.rr5698-guard-probes-v3.py: only the aliaseddiscoverimport and**kwargsslip through (the v3 nit).rr5698-fuzz.py: the same 11 residual nits.
Existing suites:
| Suite | Result |
|---|---|
test_hatch_adapter and the package suites |
233 passed, 2 skipped |
| channel-env-order | 6 |
| gateway-channel-dir | 2 |
| auto-reenroll | 39 |
| credential-contract-golden | 37/37 |
| channel-skip-owner | rc 0 |
| census-d1-anchors | 4 |
src/remote-gateway-bridge.test.py |
the same 8 failures as origin/main |
| gateway-proactive-room-from-channel-env | 10/11 (B1) |
Nits (non-blocking)
- N1. The env-token fail-closed case has no test. Refused
AG2_DEVICE_ENVplus an env token exits FATAL, as declared, but the real-bridge test covers only the no-env-token row. Add the env-token row.- This is a new hard failure for desktop-style launchers that set both, so it is worth a line in the release notes.
- N2. A refused default channel
.envfalls through to the vault. That is documented and named, and the vault can hold another identity's token. It is acceptable because the path was not explicitly named, but it is the same shape as B2. The witness row covers it. - N3. Carried from v3 and v4: the aliased
discoverimport and**kwargsreaders, the insides of allowlisted readers, obfuscated keys, and the fuzz residuals.
Before the APPROVE
- Resolve B1, with CI green at the head under review.
- Decide on the token-file recommendation above.
- Run the per-tier §15 witness at that head, with the v2 amendments:
- every tier delivers a task end to end;
- the env tier is shown by the absence of a "loaded from" line;
- the wheel is built from the sha under review, with a new PID per restart.
- Add v4's refused-file witness row: an oversized or FIFO
AG2_DEVICE_ENV, showingFATAL.
Prior reviews from this shared login are on record at 1c20ee4, ef5ad0d, e92c073 and 7811176. Each new verdict should replace those, not stack on them.
Evidence (scratchpad): rr5698-dupkey.py, rr5698-refused.py, rr5698-fatal.py, rr5698-mut.sh, rr5698-guard-probes.py, rr5698-guard-probes-v3.py, rr5698-fuzz.py, rr5698-v5-bridge-fails.out.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Review of exact head da7a847f16971f88bf5336602481455c08fa9789 (including the delta from requested head 781117669f5ea57f684746e0d62ee874b39901a8).
Verdict: REQUEST_CHANGES. The latest commit fixes repeated channel-.env registration and the refused AG2_DEVICE_ENV fallback/diff case. Three credential-safety gaps remain, the current head has a real CI regression, and the required final-head live witness is still pending.
Merge blockers
1. Required CI is red on a regression introduced by the new fail-closed walk
src/gateway_credential_sources.py:104-115 stops walking all channel files after a refused AG2_DEVICE_ENV. That also prevents the later channel .env from supplying non-credential configuration. The existing contract test now fails:
python3 tests/gateway-proactive-room-from-channel-env.test.py
FAIL: test_an_undecodable_candidate_does_not_shadow_the_next
AssertionError: '' != '!ownerdm:example.org'
Ran 11 tests ... FAILED (failures=1)
GitHub's python standalone tests (5/9) and aggregate tsc + tests (clean install) checks fail on the same test. The prior 78111766 head passes it. Keep credential selection fail-closed without making the credential-specific refusal policy silently redefine the shared channel-config walk, or explicitly reconcile the contract and all consumers; the current code/test disagreement cannot merge.
2. Systemd logical-line masking still exposes credentials on two valid adjacent inputs
shared/ag2_gateway_credentials.py:383-391 begins a continuation whenever a physical line merely endswith("\\"). Actual systemd skips top-level comments before continuation processing and tracks escape parity, so only an odd trailing run continues. The primary parser shows both rules: systemd conf-parser.c at 7197de99, lines 357-396.
Both cases still print synthetic credentials through the real Hatch _install_file diff at this head:
# harmless comment \
Environment=REMOTE_TASK_TOKEN=synth-after-comment-5698
Environment=SAFE=C:\\
Environment=REMOTE_TASK_TOKEN=synth-after-even-slashes-5698
The first comment incorrectly starts a logical group; the even pair in the second case incorrectly consumes the next assignment. The reported comment-inside-an-active-continuation fix passes, but these adjacent branches remain unsafe. Skip standalone comments before continuation state, use trailing-backslash parity, regenerate all copies, and test both through each copy and the Hatch sink.
3. Refused channel/token files remain unmaskable but Hatch still prints the diff
The new cli.py:166-170 withholds a diff only when found.fatal is set. gateway_credential_sources.py:263-266 records a refused default channel .env or explicit REMOTE_TASK_TOKEN_FILE only in found.notes, so Hatch continues with a registry that cannot contain that file's credentials.
I reproduced both with an over-1-MiB file beginning with a synthetic token and an installed unit containing that token under Environment=NOTE=. In each real _install_file run:
fatal=''
notes=('credential file ... was refused (over 1048576 bytes); nothing in it was read',)
secret_visible=True
-Environment=NOTE=<synthetic refused-file token printed raw>
The latest push correctly makes a refused named AG2_DEVICE_ENV fatal and withholds its diff. Apply the same safe-sink rule whenever discovery has any unread/refused credential candidate: fallback may remain a separately documented runtime choice, but Hatch must not show an unredacted diff when masking discovery is incomplete. Pin default-channel and token-file refusals through the real Hatch sink.
4. Hatch still discovers the installer environment, not the installed service environment
adapters/hatch/cli.py:143-149 resolves from installer os.environ, while the unit rendered at :530-542 carries only selected variables and omits CLAUDE_CONFIG_DIR, REMOTE_TASK_CHANNEL_DIR, token/URL aliases, and the token-file setting. Bridge startup resolves from the eventual service process environment at remote_gateway_bridge.py:1662-1664.
A systemd user service inherits the manager's environment, not automatically the invoking systemctl client's environment (systemd.exec, “Environment Variables in Spawned Processes”). The installer calls show-environment only as an availability probe and discards its output.
The exact-head model using the real resolver, unit renderer, bridge import, and Hatch diff still shows both directions:
installer-only custom config/channel:
Hatch token=file; unit carries neither selector; clean-manager bridge token=none
manager-only custom config/channel:
Hatch token=none; bridge token=file; registry contains manager token=False
real Hatch diff prints that manager-only token raw
Model the effective service environment separately from the installer environment and register every candidate the service can see. Do not put bearer secrets into the mode-0644 unit. Add divergent installer/manager-environment rows to the real-sink tests.
5. The required exact-final-head live witness remains absent
The PR body still says the §15 run is pending. After the code and CI blockers are fixed, run the real post-restart round trip for each source tier at the final built SHA, with a new PID and sanitized output. Harness proof does not replace this live-path gate.
Verified at da7a847f
- Repeated keys in device/channel
.envfiles now preserve last-value-wins selection while registering both values; neither appears in the real Hatch diff. - A refused explicit
AG2_DEVICE_ENVnow names the refusal, selects no fallback token, exits fatally, and withholds the Hatch diff. - The earlier explicit token-file assignment, guard keyword/default-lambda, and FIFO fixes still pass.
tests/gateway-credential-sources.test.py: 63/63 pass.tests/ag2-sparrow-hatch-adapter.test.py: 176 pass, 2 skip with its documented syntheticUSERprecondition.- Credential golden tests: 37/37 pass; both sync checks,
review-checks.sh, andgit diff --checkpass; CLA is green.
Non-blocking cleanup remains: the bounded-reader module's “never block” wording overclaims regular-file behavior on NFS/FUSE, the structural guard still misses aliases/**kwargs, and the PR body still says 42 tests rather than 63.
Worst case remains a live or retired credential printed in an install diff, or a restarted service silently using a different identity than Hatch assessed. All probes used synthetic values in isolated temporary environments; I did not touch a live service, host credential file, vault, or Keychain.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at 38f4b68
Verdict: REQUEST_CHANGES, for two blockers. Both come from the new systemd-manager read. The credential work itself is clean:
- option (b) is in, and the protected proactive-room test passes unedited;
- a refused token file is FATAL at startup and named again on reload;
- the odd-backslash rule matches systemd's conf-parser and does not leak;
- every earlier probe and mutant holds.
Without B1 and B2 this would be the COMMENT you expected (code approvable, APPROVE after the §15 witness).
Method
- Head. I checked it with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf. The only commit since da7a847 is 38f4b68. - Workspace. A scratch worktree, now removed. Synthetic fixtures only, with
systemctlmocked. No.env, keychain or vault reads. - CI: 36 checks pass, 3 are skipped.
review-checks.shpasses, and both--checksync tools pass.
Blockers
B1. install --no-service now hides an existing managed service from the rest of install
install() now caches _MANAGER_PROBE["env"] = None if args.no_service else _manager_env(). Inside install, systemd_user_available() has become _manager_env() is not None, so with --no-service it reports False even when systemd is reachable.
That answer reaches more than the credential analysis. keepalive._unit_owner() drives ensure_sparrow, restart_listener and unmanaged_health_problem.
Probe rr5698-noservice.py: systemd reachable (mocked), and an installed unit carrying SERVICE_MARKER:
| Where | _unit_owner() |
systemd_user_available() |
|---|---|---|
| outside install | ours |
(true) |
inside install --no-service |
None |
False |
inside install |
ours |
True |
On main, install --no-service on a host that already runs the managed unit:
ensure_sparrowsawowner == "ours";- it deferred to the unit's process, and
restart_listenerusedsystemctl --user restart.
At this head, the same run goes down the unmanaged branch, with two bad outcomes:
- If the service lacks
REMOTE_HEALTH_COMMAND:replace_sparrowstops the service's Sparrow and spawns a detached one. The unit hasRestart=alwaysandRestartSec=10, so systemd brings its own back 10 s later, and the two contend for the lock. - Otherwise:
restart_listenerspawns a detached Sparrow next to the running service. It exits 75 ("another Sparrow holds the lock") and install reportserror: Sparrow did not start.
This is a service-management behaviour change that a credential-analysis decision leaked into (REVIEW.md lesson 5). test_hatch_adapter (233 pass) has no case that combines --no-service with an existing managed unit, so nothing caught it.
Fix: keep "which environment do I analyse?" apart from "is systemd available?". For example, pass the analysis environment into _credential_discovery, or cache a separate _ANALYSIS_ENV. systemd_user_available() must keep answering the real question. Add the case above to test_hatch_adapter.
B2. The new credential-source read escapes the guard
_manager_env() shells out to systemctl --user show-environment. The result is the environment the service inherits, which can carry REMOTE_TASK_TOKEN and its aliases, and it feeds discover(). That is credential I/O at a new site. But:
IO_ALLOWEDdoes not list it;credential_access()returns[]for the whole ofadapters/hatch/cli.py;- the same read placed in any other package module is also
[], including one that just prints the manager environment (probes in the v6 evidence).
The guard's job, under condition 1, is to pin where credential I/O happens. This site is outside the pin.
Fix:
- Add
("adapters/hatch/cli.py", "_manager_env")toIO_ALLOWED. - Teach the detector that a
show-environmentconstant (orsystemctl … show-environment) is a credential-store read, the same waysecurityandfind-generic-passwordalready are. - Add the probe to
test_the_detector_catches_every_bypass.
The systemd-manager read: everything else checks out
- The shell-out.
_systemd_run(["systemctl", "--user", "show-environment"])becomessystemctl --no-ask-password --user show-environment. It runs withstdin=DEVNULL,capture_output,timeout=SYSTEMD_TIMEOUT_S(30), and everyOSErrororSubprocessErrorgivesNone. It is bounded, cannot prompt, and runs at most once per install (cached, and cleared infinally). - No secret from the manager environment is printed.
- The manager environment is only an input to
discover(). Notes andfatalcarry paths, never values, and the diff renders unit, script and config files only. - In the probe, a manager-only
REMOTE_TASK_TOKENand an installer-only one were both withheld when they appeared in the installed unit under neutral keys. Both environments go into one registry. - A non-credential manager variable printed nowhere.
- The manager environment is only an input to
- The fallback is named where it matters.
- With
--no-servicethe fallback is the documented intent, and install printsskipped: the ag2-sparrow user service (--no-service). - When no manager answers,
install_serviceprintssystemctl --user is not available here, so no service was set up. - The diff itself does not say which environment it analysed (N1).
- With
Credential behaviour at this head
rr5698-fatal.py:
| Scenario | Bridge | Hatch |
|---|---|---|
A. refused AG2_DEVICE_ENV, no env token |
FATAL, named on stderr | usable False, diff withheld |
B. refused AG2_DEVICE_ENV, env token set |
FATAL (as declared) | usable False, diff withheld |
| C. readable token file | device token, no stderr | usable, normal diff |
| D. refused token file, device token present | FATAL: REMOTE_TASK_TOKEN_FILE names <path>, which was refused (not UTF-8); token rotation cannot read it… |
usable False, diff withheld |
| E. refused token file, env token set | FATAL | usable False, diff withheld |
F. refused default channel .env |
device token wins; named on stderr | NOTE, and the diff is withheld because masking is incomplete |
This resolves the v5 token-file recommendation. A token file refused at reload is named with ; no rotation (mutant N4 below).
Config still reaches later files after a refused device file: gateway-proactive-room-from-channel-env passes 11/11. Re-adding the da7a847 break (mutant N8) fails test_general_config_still_reaches_a_later_file_after_a_refused_one.
rr5698-dupkey.py: the early and late duplicates are withheld for both the device and the channel file. The registry holds both, and the last value wins.
rr5698-refused.py: for an oversized, non-UTF-8 or FIFO device file, the winner is none, the file is named, and no token leaks.
The odd-backslash rule (contract probe):
| Case | Result |
|---|---|
an even run (2 or 4) before a credential line, an Environment= line, or free text holding a registered token |
not continued; each line masked on its own (REMOTE_TASK_TOKEN=<withheld>, or <text withheld…>) |
| an odd run (1 or 3) | continues; the credential word on the next line and a split registered token are withheld |
a comment line ending in \ |
does not continue (skipped first) |
| a comment inside a continuation | skipped without ending it |
| an odd run at end of file | the line is withheld |
One residual: an unregistered token split across two lines with an even run of backslashes prints its tail (N2). systemd does not join those lines, so the effective configuration does not hold that token.
Mutants. Each fails tests when reverted.
New in this round:
| Mutant | Tests failing |
|---|---|
| N1: any trailing backslash continues | 3 |
| N2: comments not skipped first | 3 |
| N3: token-file refusal not fatal | 5 |
| N4: reload refusal not named | 1 |
| N5: diff withheld only on fatal | 3 |
| N6: service env = installer env | 3 |
| N7: installer env not also registered | 1 |
N8: the break re-added |
2 |
Earlier mutants, still caught:
| Mutant | Tests failing |
|---|---|
| repeated assignments not registered | 4 |
the bridge's main() ignores fatal |
7 |
| detector key check off | the self-test fails |
SOURCES reordered |
16 |
Probes and fuzz:
rr5698-guard-probes.py: 15/15 targeted probes caught.rr5698-guard-probes-v3.py: only the aliaseddiscoverimport and**kwargsslip through (the earlier nit).rr5698-fuzz.py: the same 11 residuals.
Tests:
| Suite | Result |
|---|---|
| the new file | 74/74 |
test_hatch_adapter + the package suites |
233 passed, 2 skipped |
| channel-env-order | 6 |
| gateway-channel-dir | 2 |
| auto-reenroll | 39 |
| credential-contract-golden | 37/37 |
| proactive-room-from-channel-env | 11/11 |
| channel-skip-owner | rc 0 |
| census-d1-anchors | 4 |
src/remote-gateway-bridge.test.py |
the same 8 failures as origin/main |
Nits (non-blocking)
- N1. The diff does not say which environment it analysed. Add one line, such as "analysed: the user manager's environment" or "this shell's environment".
- N2. The masked output can read differently from the file.
- An even-run line is re-rendered through shlex, so
SAFE=a\\shows asSAFE=a\, which reads like a continuation. - The tail of an unregistered token split across an even-run boundary prints. systemd does not see it as the token.
- An even-run line is re-rendered through shlex, so
- N3. The
$'…'parsing in_manager_envhas two rough edges.- A malformed escape,
X=$'bad\', raisesUnicodeDecodeErrorout ofinstall. Catch it and fall back to the raw value. - UTF-8 bytes shown as
\xc3\xa9decode as mojibake (café), so a non-ASCII secret would be registered in the wrong form. Decode the escapes to bytes, then UTF-8.
- A malformed escape,
- N4. Carried over: aliased or
**kwargsreaders, the insides of allowlisted readers, obfuscated keys, and the fuzz residuals.
Before the APPROVE
- Fix B1 and B2, with tests. Keep CI green at that head.
- Rerun the §15 witness on Sudoo's VM at that head, with the v2 amendments:
- every tier delivers a task end to end;
- the env tier is shown by the absence of a "loaded from" line;
- the wheel is built from that sha, with a new PID per restart.
- Add these witness rows:
- a refused
AG2_DEVICE_ENVgives FATAL; - a refused
REMOTE_TASK_TOKEN_FILEgives FATAL; - on the Hatch VM,
install --no-serviceon a host with the managed unit already running leaves that unit's Sparrow in charge.
- a refused
Shared-login reviews are on record at 1c20ee4, ef5ad0d, e92c073 and 7811176. Each new verdict should replace those, not stack on them.
Evidence (scratchpad): rr5698-noservice.py, rr5698-fatal.py, rr5698-dupkey.py, rr5698-refused.py, rr5698-mut.sh, rr5698-guard-probes.py, rr5698-guard-probes-v3.py, rr5698-fuzz.py, rr5698-v6-bridge-fails.out.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-review of exact head 38f4b6897be529ca48189386c6563e6875994496.
Verdict: REQUEST_CHANGES. The regressions called out on da7e595 are fixed and current CI/CLA are green, but three credential/runtime blockers remain. The final-head live witness is also still pending.
Merge blockers
1. Valid systemd escapes bypass credential-name masking
shared/ag2_gateway_credentials.py:411-429 parses Environment= with shlex.split(), rather than systemd's C-style unescaping. A real _diff() repro at this head produced:
input: Environment=REMOTE_TASK_\x54OKEN=synth-hex-key-5698
output: -Environment=REMOTE_TASK_x54OKEN=synth-hex-key-5698
The \u0054 and octal \124 spellings also printed the synthetic value. systemd decodes each spelling into REMOTE_TASK_TOKEN, but the masker classifies the undecoded key and misses it. Environment= uses systemd.syntax unquoting, whose escape table includes hex, octal, and Unicode escapes.
Please parse this syntax compatibly before classifying names/values (or fail closed on unsupported escapes) and cover these variants through the real Hatch diff sink and synced copies.
2. Per-unit credential sources are absent from discovery and same-diff duplicates leak
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.py:145-188 models manager-global show-environment plus only the prospective AG2_DEVICE_ENV. It does not incorporate an existing unit's Environment=, drop-ins, EnvironmentFile=, or UnsetEnvironment=. This matters because systemctl show-environment explicitly excludes per-unit variables, while systemd adds those variables later when it forks the unit.
The retained hand-written-unit path at cli.py:584-594 reproduces a direct leak:
old:
Environment=REMOTE_TASK_TOKEN=synth-per-unit-duplicate-5698
Environment=NOTE=synth-per-unit-duplicate-5698
diff:
-Environment=REMOTE_TASK_TOKEN=<withheld>
-Environment=NOTE=synth-per-unit-duplicate-5698
_diff() masks each line independently, so it never registers the credential assignment before rendering the duplicate. A drop-in/EnvironmentFile= probe likewise showed the effective token survived installation but was not registered, and the duplicate NOTE= line printed raw.
Please account for effective per-unit sources (or conservatively withhold what cannot be resolved), and make diff rendering two-pass: collect credential assignments from all displayed old/new text before emitting any line. Add retained-unit and surviving-drop-in coverage.
3. Hatch selects the credential environment before it knows which runner will actually start
Credential validation happens at cli.py:705. install_service() is best-effort and returns early on setup failure (:543-581), _install() does not propagate that outcome, and ensure_sparrow() then falls back to detached startup (:726-738). That detached process inherits the installer environment in keepalive.py:208-246, not the manager environment already analyzed.
The real-path hermetic probe showed:
analysis CLAUDE_CONFIG_DIR: /tmp/manager-config
spawn CLAUDE_CONFIG_DIR: /tmp/installer-config
analysis matches spawn: False
A second probe made service setup fail and put a refused token file only in the installer environment. Manager-based validation demoted it to a note, then the detached environment selected it and production discovery classified it fatal:
service_setup_failed= True
detached_uses_installer_token_file= True
detached_fatal= True
The adjacent --no-service path also forces the manager cache to None without stopping an already-running unit, so validation may again describe a different identity from the process that remains live.
Please determine/propagate the actual runner mode before credential validation and validate the environment that runner will use. Cover service-install failure followed by detached fallback and --no-service with an existing unit.
4. Required final-head live witness is still pending
This changes startup/service behavior. The PR body still marks the real Linux post-restart inbound-task → reply round trip as not run. Per REVIEW.md lesson 15, this remains a merge gate after the code blockers above are fixed.
Verified at this head
- Refused
AG2_DEVICE_ENVremains fatal for credential tiers while later files may still provide noncredential config. - Refused token/channel files withhold Hatch diff values.
- Top-level systemd comments and odd/even continuation parity behave correctly.
- Stable manager-vs-installer selection tests and proactive-room config regression pass.
tests/gateway-credential-sources.test.py: 74/74 passed.tests/ag2-sparrow-hatch-adapter.test.py: 178 passed, 2 skipped.- Proactive-room suite: 11/11 passed.
- Vendored sync checks, review scanner,
git diff --check, all current GitHub checks, and CLA passed.
Nonblocking: the PR body still reports 42 credential-source tests, while the suite now collects 74.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed the requested option-(a) revision at dcc978a62833c975619d53ade51336120b778805. The head moved during review, so I also inspected and reran both failures at the current exact head, ad3cbb1369903c5606948913bb218ae0ca33d33e.
Verdict: REQUEST_CHANGES. The broad direction is improved, but two boundary failures remain, and the required final-head live witness is still pending.
1. [P1] Register the complete systemd assignment value before the real diff sink renders it
register_assignments() still splits on every quote/whitespace. At the real Hatch _diff() sink, this exact-head hermetic input:
Environment=REMOTE_TASK_TOKEN='abcdefgh'
ExecStart=/bin/echo --token='abcdefgh' --x
still renders:
-Environment=<withheld>
-ExecStart=/bin/echo --token='abcdefgh' --xThe same parser gap exposes a short value from Environment="REMOTE_TASK_\x54OKEN=abc" on a neutral line and the decoded value from Environment="REMOTE_TASK_TOKEN=alpha\somega" as NOTE=alpha omega.
The new register_candidates() seeding at ad3cbb13 masks a value only while a matching candidate is still discoverable from the active env/file/vault sources. It does not repair two-text registration for a retired or drifted unit value, which is exactly what an install diff may be displaying. Worst case, a retired bearer is printed to terminal or automation logs.
Please parse/register the complete systemd item (including supported quoting/escapes), or fail closed when that cannot be done, and add these shapes through the real Hatch diff sink plus the synced-copy contract.
2. [P1] --no-service must not replace a service-owned process when manager reachability is uncertain
_unit_owner() collapses “the owned unit exists, but the user-manager probe is temporarily unavailable” into “no unit.” install --no-service therefore misses its deferral branch, and ensure_sparrow() treats the live service-held process as unmanaged.
Hermetic full-install repro at this head, with an owned unit and a live synthetic Sparrow holding the production-format lock while reachability returns false,false,false,true:
install_rc=0
old_service_holder_exit=-15
detached_spawn_count=1
owner_after_probe_recovery="ours"
Output included replacing: Sparrow ... and started: spawned detached. With the unit's Restart=always, the terminated service can restart beside the detached process, causing duplicate/lock contention and potentially start-limit disruption.
Please distinguish “unit absent” from “manager temporarily unreachable.” Under --no-service, an installed unit or service-owned live holder must not authorize a stop/detached replacement merely because reachability is unknown. Add the transient-unavailable unit + live-holder regression.
3. Required live witness remains outstanding
This changes bridge/startup/service behavior, and the PR body still marks the final-SHA Linux restart plus inbound-task → delivered-reply witness as owner-gated and pending. That witness is required before approval; I did not touch live services, configuration, or credentials.
Verified at ad3cbb13: credential suite 78/78; Hatch adapter 180/180 with 2 platform skips; both sync checks, review checks, and git diff --check pass. Whole environment-shaped lines and continuations are withheld, Hatch does not consume manager output or decide credential validity, refused named files fail at bridge startup, and the normal active-service/no-unit --no-service paths pass. Those positives do not cover the two cases above.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at ad3cbb1
Verdict: REQUEST_CHANGES, for three blockers. All three are small and need no new I/O. With them fixed, this becomes a COMMENT (code approvable, APPROVE after the witness on Sudoo's VM).
What ad3cbb1 does. Hatch's _diff now seeds its masking registry from the owner's discover(): env, device and channel files, token file, and the vault as the last tier. Winners, notes and fatal results are discarded.
What it fixes. The v7 trade-off (D1) is fixed when the installer's shell names AG2_DEVICE_ENV or holds the token.
What it does not fix.
- It misses Hatch's own default device file (B3, new).
- It does not address v7 blocker 1. The masking is by registry value only, not by shape (B1).
- It leaves v7 blocker 2 open (B2).
Method
- Head and delta. I checked the head with
gh pr view 5698 --json headRefOid. The delta from dcc978a is one commit:adapters/hatch/cli.py+4 −2, a new package-onlygateway_credential_io.py(+60),remote_gateway_bridge.py+3 −19, and two test files.shared/,src/andskills/are unchanged. - CI: 35 checks pass, 3 are skipped.
- Workspace. Scratch worktrees, now removed. Synthetic tokens only; the vault is mocked through
gateway_credential_io._vault. No.env, keychain or vault reads.
Blockers
B1 (carried from v7). Masking is by registry value only: credential shapes on non-environment lines still print
At this head:
- An
ExecStartargument or a hookAuthorization:header is withheld only when its value is in the registry. That covers the new tests' case: a token that discovery found. register_assignmentsstill registers only line-leadingKEY=values whose KEY is credential-named._mask_linestill judges only the line's own leading key.
So a token that discovery did not find prints. rr5698-v7-fuzz.py at this head, with the vault mocked empty, prints all of these raw:
| Line | Shown |
|---|---|
ExecStart=/opt/bin/sparrow --token <tok> / --token=<tok> / lower-case execstart=… --token <tok> |
raw |
ExecStart=/usr/bin/env -i REMOTE_TASK_TOKEN=<tok> … and … env X=<tok> … |
raw |
ExecStartPre=/bin/sh -c 'export REMOTE_TASK_TOKEN=<tok>' |
raw |
curl -H "Authorization: Bearer <tok>" … |
raw |
Description=Sparrow <tok>, NOTE=<tok>, a JSON line |
raw |
Such a token can be a retired token, a token from another account, or a vault-only token the owner never reaches.
This also closes the author's vault-only gap without a Keychain read. The author asks whether to always read the vault for masking. I recommend not doing that: it adds a possible Keychain prompt to install. Add a shape rule instead, applied whatever the registry holds. Withhold any line that contains:
- a
KEY=anywhere in the line whose KEY matches_CREDENTIAL_NAME_RE(soenv -i …,sh -c 'export …'and--opt KEY=vare covered); - a flag matching
--?[\w-]*(token|secret|password|passwd|api[-_]?key|bearer)[\w-]*, in the--flag valueor--flag=valueform; - an
Authorization:header, or the wordBearerfollowed by a value.
Then register the values found that way, at any length, so the same value elsewhere is withheld too.
What then remains is a vault-only token, or a token found nowhere, placed bare in an arbitrary position (--flag <tok>, NOTE=<tok>, free text). State that as the limit in the body. Add the table rows above to the contract test and to the real-Hatch test.
B2 (carried from v7). The guard still does not pin that the show-environment probe discards its output
This is unchanged in the delta. IO_ALLOWED names systemd_user_available by name only. Each of these variants still passes credential_access:
- it returns
r.stdout; - it prints
r.stdout; - it stashes the output in a module global;
- another function parses its return value.
Fix: a behavioural pin. Give a mocked _systemd_run a stdout holding a synthetic REMOTE_TASK_TOKEN=…. Assert the function returns exactly True (a bool), prints nothing, and leaves no copy on the module.
B3 (new). The seeding reads the installer's raw environment, so Hatch's own default device file is never read
_diff calls register_candidates(os.environ, …). The unit Hatch installs is given AG2_DEVICE_ENV=<device_env_path()>, and 38f4b68 overlaid exactly that value. This head does not.
So in the default Hatch install, where the token sits in ~/.config/ag2-sparrow/relay.env and the installer's shell does not export AG2_DEVICE_ENV, the live token is never registered. rr5698-v8-live.py:
| Live token held in | ExecStart=/x --flag <live> |
NOTE=<live> |
vault consulted |
|---|---|---|---|
AG2_DEVICE_ENV, set in the installer's env |
withheld | withheld | no |
Hatch's default device file, AG2_DEVICE_ENV unset |
printed | printed | yes (10 calls for 5 diffs) |
| the installer's env | withheld | withheld | no |
The middle row is the common case, so the fix covers the less common ones. It also means install reaches the vault tier on a normal host. In a monorepo checkout on macOS, that is a security find-generic-password call, which is the Keychain cost the author wanted to avoid.
Fix (one line): register_candidates(dict(os.environ, AG2_DEVICE_ENV=str(device_env_path())), registry), or pass that env from Hatch. Add a real-Hatch test with AG2_DEVICE_ENV unset and the token in the default path.
The delta's other questions
-
Does
gateway_credential_io.pyreintroduce I/O the guard should flag?- It does reintroduce credential reads into Hatch: env, the device and channel files, the token file and the vault, for masking only.
- That stretches the body's option (a) wording ("Hatch no longer reads … decides nothing"). The decision half still holds: winners, notes and fatal results are discarded, and
installgates nothing on them. The read half no longer holds, so update the option (a) section and the finding-1 line to say so. - The guard pins the new site.
IO_ALLOWEDadds("gateway_credential_io.py", "read_file"),("gateway_credential_io.py", "vault_lookup")and("gateway_credential_io.py", "_vault").credential_accessreturns[]for the module as shipped, but flags an addeddump()that opensrelay.env(gateway_credential_io.py:dump credential I/O outside the adapters' edge). So the allowlist is by function and not a blanket module pass. register_candidatesitself is not allowlisted, and does not need to be: it only hands allowlisted readers todiscover.- The module is package-local, not vendored from
src/. That is consistent with "adapter I/O stays at the edge".
-
Does the bridge's −19 change any FATAL path? No.
_vault_lookupand_monorepo_srcnow delegate togateway_credential_io.vault_lookupandmonorepo_src. The bodies are the same code, andmonorepo_src(marker, __file__)walks up from the same package directory.rr5698-fatal-v7.pyat this head matches dcc978a row for row:- A, B, I and J (a refused
AG2_DEVICE_ENV: over 1 MiB, with an env token, with a vault token, or a directory) are FATAL. - D and E (a refused token file) are FATAL.
- C, F, G and H (readable token file, refused default channel
.env, env only, vault only) run.
- A, B, I and J (a refused
test_vault_token_tierpasses 7/7, andsrc/remote-gateway-bridge.test.pyshows the same 8 failures asorigin/main.
-
Mutant. Removing the
register_candidatescall fails 12 tests, matching the author's report. -
Suites:
Suite Result tests/gateway-credential-sources.test.py78/78 test_hatch_adapter+ the package suites235 passed, 2 skipped (CI's file order) rr5698-guard-probes.py15/15 One harness note: running
test_vault_token_tier.pybeforetest_env_token_quotes.py/test_hatch_adapter.pyin a single pytest process gives 2 failures. That happens at dcc978a too, so it is pre-existing and not from this delta. CI runs each file on its own.
Unchanged since v7 (still fine)
These were checked at dcc978a, and the delta touches none of them:
- environment-shaped lines are withheld whole (
EnvironmentFile=,PassEnvironment=,UnsetEnvironment=, escaped, lower-case or indented spellings, continuations), and so are the*Credential*=directives; --no-servicedefers to a managed unit;- no dead masker paths are left.
Before the APPROVE
- Fix B1, B2 and B3, with tests, and keep CI green.
- Update the body:
- option (a) now means Hatch reads for masking but decides nothing;
- finding 1 for Hatch is covered by the registry plus the B1 shape rule;
- the stated limit is a vault-only or unknown token placed bare in an arbitrary position.
- Run the witness on Sudoo's VM at that head, per tier, with the v2 amendments:
- every tier delivers a task end to end;
- the env tier is shown by the absence of a "loaded from" line;
- the wheel is built from that sha, with a new PID per restart.
- Include the rows already planned, plus one new row: a default-path Hatch install with a hand-written unit holding the live token in
ExecStart, which must show it withheld.
Reviews on record: 9 in total. This shared login has 6 (CHANGES_REQUESTED at 1c20ee4, ef5ad0d, 7811176, da7a847 and 38f4b68, plus a COMMENTED at e92c073). Kewei has CHANGES_REQUESTED at e92c073, da7a847 and 38f4b68. Each new verdict should replace the ones before it, not stack on them.
Evidence (scratchpad): rr5698-v8-live.py, rr5698-v7-live.py, rr5698-v7-fuzz.py, rr5698-fatal-v7.py, rr5698-mut.sh, rr5698-guard-probes.py, rr5698-v8-bridge-fails.out.
d1a1adc to
6f8de04
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login), at 6f8de04
Verdict: COMMENT. The code is approvable at this head. The APPROVE follows the per-tier §15 witness on Sudoo's VM at this exact head (REVIEW.md lesson 15).
All five blockers (Q1–Q3 from my v7 and v8 drafts, K1–K2 from Kewei) are fixed. Each has a test that fails when its fix is reverted. What remains are non-blocking nits: a few shape gaps adjacent to the stated limit (N1), and over-withholding that can hide harmless lines (N2). N2 is worth fixing in the same push.
When the APPROVE is posted, it replaces this login's earlier reviews: CHANGES_REQUESTED at 1c20ee4, ef5ad0d, 7811176, da7a847 and 38f4b68, and COMMENTED at e92c073. It does not stack on them.
Method
- Head and CI. I checked the head with
gh pr view 5698 --json headRefOid. The merge-base is still 5e632bf. CI: 36 checks pass and 3 are skipped. The delta from ad3cbb1 is one commit. - Checks.
- Both
--checksync tools pass, andreview-checks.shpasses. - The new test file passes 88/88.
test_hatch_adapterand the package suites pass 239, with 2 skipped (CI's file order).- The other suites (channel-env-order, credential-contract-golden, proactive-room) pass.
src/remote-gateway-bridge.test.pyshows the same 8 failures asorigin/main.
- Both
- Workspace. A scratch worktree, now removed. Synthetic tokens only; the vault and
systemctlare mocked. No.env, keychain or vault reads.
The five blockers: verified
| Blocker | Evidence at this head | Fails when reverted |
|---|---|---|
| Q1. Mask by shape. | rr5698-v7-fuzz.py: every shaped row is withheld (--token, --token=, lower-case execstart --token, env -i REMOTE_TASK_TOKEN=, sh -c 'export REMOTE_TASK_TOKEN=…', Authorization: Bearer, the JSON key). The 5 rows still printing are all inside the stated limit: no shape, and a token no source holds. Those are env X=<tok>, Description=…<tok>, NOTE=<tok>, and two unheld "live" tokens. |
shape rule off: 27 |
| Q2. Pin the probe's discard. | The shipped probe passes. The author's 8 variants are flagged, and so are 3 of mine: _ok(r) then return r, a logging lambda inside _ok(...), and _ok(x[0]) on a list holding the run. |
(the guard's own self-test) |
| Q3. Seed from the default device file. | rr5698-v8-live.py, in all three cases (token in an exported AG2_DEVICE_ENV, in Hatch's default ~/.config/ag2-sparrow/relay.env with AG2_DEVICE_ENV unset, or in the installer's env): ExecStart=/x --flag <live>, NOTE=<live>, env REMOTE_TASK_TOKEN=<retired>, --token <retired> and Environment=… are all withheld, and the vault is reached 0 times (it was 10 at ad3cbb1). rr5698-v7-live.py: all 4 rows withheld. |
overlay dropped: 1 (test_the_hatch_install_diff_seeds_from_the_default_device_file) |
| K1. Split systemd values as systemd does. | Values are split with quotes and escapes decoded, and registered raw, decoded and quote-stripped. | systemd_words dropped: 3 |
K2. --no-service with an unanswering manager. |
rr5698-k2.py runs the real install --no-service, with systemctl present but answering None or rc 1, a unit present (ours or hand-made) and a live lock holder (pid 4242). No _stop/SIGTERM, no replace_sparrow, no _spawn_sparrow, no os.kill, no install_service; rc 0. It prints skipped: … is installed but systemctl --user did not answer, so whether that service runs Sparrow is unknown; no Sparrow is stopped and no detached one is started beside it and NOTE sparrow: … install neither stops nor starts a Sparrow. restart_listener returns False with the reason, and unmanaged_health_problem returns (None, None) for UNIT_UNREACHABLE, so keepalive does not act either. The body declares all of this. |
(the author's 2 named tests) |
Other checks:
--no-servicewith a reachable manager (rr5698-noservice-v7.py): with a managed unit,ensure_sparrowseesoursand defers. With no unit, one detached Sparrow. No manager secret is printed.- The bridge's FATAL paths (
rr5698-fatal-v7.py) match the previous head row for row:- A, B, I and J (a refused
AG2_DEVICE_ENV: over 1 MiB, with an env token, with a vault token, or a directory) are FATAL; - D and E (a refused token file) are FATAL;
- C, F, G and H (readable token file, refused default channel
.env, env only, vault only) run.
- A, B, I and J (a refused
New shape bypasses I tried (rr5698-v9-bypass.py):
| Withheld | Still printed |
|---|---|
--Token, --TOKEN= |
-t <tok>, -k <tok> |
?access_token= and &token= in a URL |
--key, --access-key, --credentials, --pat |
a URL path token, curl -u user:pass |
X-Api-Key: <tok>, X-Auth-Token: <tok> headers |
wget --password, Authorization:Bearer<tok> |
YAML token: <tok> |
Cookie: session=…, Authorization: Basic <b64> |
sh -c 'T=<tok> /x' (neutral key) |
lower-case token=, JSON token / apiKey / accessToken |
—token (an em dash, not --) |
a %-encoded value under REMOTE_TASK_TOKEN, an escaped key (REMOTE_TASK_\x54OKEN=) |
a bare base64 value, and a comment line holding a token |
Nits (non-blocking)
N1. Shape gaps next to the stated limit. The body's limit is "no credential shape on its line". These are credential shapes by any ordinary reading, yet they print:
--key,--access-key,--credential(s),--pat;- any
<Name>: <value>HTTP header whose name holdskey,token,authorsecret(X-Api-Key,X-Auth-Token); - YAML
token:/secret:keys.
They are cheap to add to _SHAPE_NAME and _HEADER_RE, for example [\w-]*(?:key|credential|pat)… for flags and \b[\w-]*(?:key|token|auth|secret)[\w-]*\s*:\s*value for headers. Short flags (-t, -k), neutral keys, base64 and Unicode dashes can stay in the stated limit. Name them there.
N2. Over-withholding: a short shaped value poisons unrelated lines (rr5698-v9-over.py). A shaped value is registered at min_len=1, and every shown line is then matched by substring:
ExecStart=/x --auth=1registers1.RestartSec=10andDescription=Sparrow 1.0are then withheld.--auth-mode=basichidesDescription=A basic relay.--token-file /home/u/…/tokenhides anExecStartPrethat merely lists that path.--author qingyunandX-AUTHOR=are withheld (anauthshape), and so isDescription=by qingyun.--tokenizer,--oauth-client-idandDescription=Bearer of tasksare withheld as shapes.- An
Environment=path value of 8 or more characters hides anExecStartthat uses the same path.
This fails closed, so nothing leaks. But a diff of all <withheld> defeats the diff's purpose, which is to show what --force would discard.
Fix:
- The shaped line is already withheld by shape, so register a shaped value only at
BARE_SECRET_MINor above. - Exclude plain filesystem paths, values starting with
/, from shape-value registration.
Keep the harmless-lines control: a stock unit (Description, After, ExecStart=/usr/bin/python3 -m ag2_sparrow, Restart, RestartSec, WantedBy) prints untouched today. Add one test with --auth=1 next to RestartSec=10.
N3. K2 starts no Sparrow and still returns rc 0. A host with a leftover unit file and no user manager (WSL, or a container with the systemctl binary but no systemd) now gets no Sparrow at all: not from install, and not from keepalive. Install exits 0 with only a NOTE. It is declared, and it is the safe choice, but consider a non-zero rc or a "Sparrow is not running" line, so automation does not read it as a success. Add a witness row for this.
N4. Carried over: the aliased discover import and **kwargs readers, the insides of allowlisted readers, and obfuscated keys.
Before the APPROVE
- Optional in the same push: N1 and N2.
- Run the §15 witness on Sudoo's VM at the head that will merge, per tier, with the v2 amendments:
- every tier delivers a task end to end;
- the env tier is shown by the absence of a "loaded from" line;
- the wheel is built from that sha, with a new PID per restart.
- Include the planned rows:
- a refused
AG2_DEVICE_ENVgives FATAL; - a refused token file gives FATAL;
install --no-servicewith the managed unit running keeps the same PID;- a hand-written unit shows
Environment=<withheld>; - a default-path install with the live token in a hand-written
ExecStartshows it withheld.
- a refused
- Add one more row:
install --no-servicewhile the user manager is stopped. It must print the "did not answer" line, and the service's Sparrow PID must be unchanged.
If the head moves before the witness (for N1 and N2), the witness moves with it, and this review should be re-checked at the new sha.
Evidence (scratchpad): rr5698-v9-bypass.py, rr5698-v9-over.py, rr5698-k2.py, rr5698-v7-fuzz.py, rr5698-v7-live.py, rr5698-v8-live.py, rr5698-noservice-v7.py, rr5698-fatal-v7.py, rr5698-mut.sh, rr5698-v9-bridge-fails.out.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 6f8de0444c6396b5100e668a1daeba5b9db7c770 — REQUEST_CHANGES.
The five previously reported paths are materially improved and their focused regressions pass, but three fail-closed gaps remain.
Blockers
-
The real Hatch diff can still disclose the tail of an
Environment=credential line.shared/ag2_gateway_credentials.py:448-466callssplitlines(True), so Python treats U+2028, U+2029, and U+0085 as physical line boundaries even though the surrounding code strips/preserves only CR/LF. Throughadapters/hatch/cli.py:_diff, this synthetic input:Environment="REMOTE_TASK_TOKEN=qa-first<U+2028>qa-tail-9988"produced:
-Environment=<withheld>qa-tail-9988"I reproduced the same
tail_visible=Trueresult for U+2028, U+2029, and U+0085. Restrict physical-line grouping to the line endings the unit text actually uses (CR/LF), and pin all three through the real_diffsink. -
A failed unit-property query still authorizes killing a service-owned Sparrow.
In a synthetic full
install --no-servicerun,show-environmentsucceeded, the followingsystemctl --user show ... --property=...failed, and the live process named by the production-format lock exited on SIGTERM (-15).cli.py:425-430collapses that provider failure to{};keepalive.py:381-405then reads the unknownActiveState/MainPIDas inactive/no PID, printsreplacing:, and calls_stop(holder).An ambiguous manager/property response must remain an explicit unknown state and fail closed before
_stop()or restart. Add this adjacent failure case to the full install regression. The original “show-environmentitself does not answer” case now passes, but it does not cover the next manager call failing. -
The Q2 structural guard misses constant-constructed
show-environmentcommands.tests/gateway-credential-sources.test.py:992-1013gates on an exactast.Constant("show-environment"); the store check at:1062-1063does the same. The exact-literal leak is caught, but otherwise identical retaining functions returned no findings when the command was built with each of:"show-" + "environment" "".join(("show-", "environment")) f"show-{'environment'}" "show-{}".format("environment") "show-%s" % "environment"
The first form is compiler-constant and invokes the same command at runtime, yet
credential_access(...) == []. Fold/recognize these constant strings and add negative-instrument controls. The shipped probe currently discards its output; the blocker is that the claimed regression guard does not enforce that invariant.
Verified at this head
- The prior K1 quote/escape/tokenization repros are masked; the parent controls disclose and this head does not.
- The direct K2 manager-unreachable case now causes zero SIGTERM and zero detached spawn.
- Q1 credential-shaped assignments, flags, headers, and repeated-value sinks are withheld.
- Q2's shipped
systemd_user_available()returns a boolean and does not retain manager output for the covered literal path. - Q3 seeds the default device file, resolves the vault reader at most once per install, and clears the per-install mask on success and failure.
tests/gateway-credential-sources.test.py: 88 passed.packages/ag2-sparrow/tests/test_hatch_adapter.py: 184 passed, 2 skipped in a clean synthetic environment.- Vault tier: 7 passed; env quotes: 15 passed; credential goldens: 37/37 passed.
- Both sync checks,
git diff --check, andscripts/review-checks.shpassed. Current GitHub checks were 36 passing, 3 skipped, including green CLA.
The real Linux post-restart/delivery witness remains pending as the PR body says. I did not touch live services, configuration, credentials, .env files, or the vault.
Rule 1: one scanner (_names_and_values) hands credential_name the complete key before every `=` or `:` (inside its quotes with any characters, and as the unquoted run) and every flag name. Regexes only locate positions; the verdict is credential_name alone. A JSON key such as "pa/ss" no longer prints through the Hatch diff. A test-owned stem/separator oracle sweeps every stem split by /, ., -, space, +, :, _, an escaped quote and unicode punctuation through the contract copies and the real _diff sink. Rule 2: _unit_props parses and bounds every property before returning (MainPID at most 10 digits and PID_MAX_LIMIT, the start timestamp at most 20 digits and a u64); any exception or out-of-range value is an unknown state. install_service now does nothing at all on an unknown state: no stop, unit write, daemon-reload or enable. Rule 3: every systemctl invocation goes through _systemctl, called with a literal tuple. The guard flags any "systemctl" literal outside that helper, any non-direct or non-literal use of it or of _systemd_run, and show-environment outside the reachability probe. No dataflow analysis. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…; failed install says so - Name scanner: before each `=` or `:` (not `://`), credential_name judges every suffix of the line back to its start (the whole span, plus each suffix a SEGMENT_STEMS word can start), and the quoted key; for each flag, the run to a blank or `=` and the shell word with its quotes removed (also when the quote opens before the dashes). `pa ss: v`, `--"pa ss" v`, `"--pa ss" v` and `--pa" "ss v` are withheld through the real Hatch diff. - Systemd guard: runs over every package module, the credential owners included; pins exactly one top-level `_systemctl` and `_systemd_run`, in cli.py; flags an import alias, a rebinding, a parameter or keyword of that name, a redefinition, and either name in a string (getattr). - Install: when it stopped the unit and the next state query is malformed, it prints an error with `systemctl --user start ag2-sparrow.service`, skips the Sparrow check and exits 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st, not the host env The real-sink harnesses copied the host environment minus a denylist, so a host REMOTE_TASK_TOKEN_FILE, AG2_DEVICE_ENV or channel-dir selector reached the masking seed and its file was read. Every harness that feeds the real sinks or the bridge now starts from PATH, LANG, TMPDIR, a temp HOME and USER, plus the synthetic values it sets. Regression: test_no_harness_reads_a_credential_path_the_outer_environment_names puts sentinel selectors in the outer environment and asserts the recorded reads never open them and the bridge child's env never carries them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y builds its env from one allowlist
The shared bridge-import helper overlaid its values onto the outer environment
and kept REMOTE_TASK_TOKEN_FILE and REMOTE_TASK_CHANNEL_DIR, so the real bridge
import opened the token file and channel .env they named. The same overlay,
host copy or denylist was repeated across the suites that import or run the
bridge, Hatch, room_ops' gateway client or the credential owner.
- tests/_helpers/hermetic_env.py is the one builder. hermetic_env(**extra)
carries only PATH, LANG, TMPDIR (plus the runner's coverage controls, and
the process essentials on Windows), a temp HOME and a fixed USER.
isolate_os_environ() swaps this process's environment for it through
patch.dict(clear=True); isolated_child_env() hands an isolated process's
own environment to a child.
- hermetic_gateway and gateway_relay use it, as do 143 test files: module-level
isolation before any of those modules runs, and every child environment from
the helper. test_wheel_contents drops its host denylist, Hatch's py_env sets
a fixed USER, and the outage-recovery and no-ceiling children drop their
host copies.
- tests/hermetic-env-structural.test.py refuses, in any test file naming those
modules: os.environ.copy(), dict(os.environ, {**os.environ, f(**os.environ),
a comprehension over os.environ, env=None or env=os.environ to a subprocess,
a None-default env parameter handed to a spawn, and any subprocess, loader
or import of those modules in a file that never isolates (or above where it
does). Its self-test fires every rule on a flagged sample.
- tests/hermetic-env-sentinels.test.py runs the hermetic import, three room_ops
entry points, the relay harness, the Hatch suite and the credential-owner
suite under an audit hook, with sentinel token-file, device-env, channel-dir
and config-dir selectors plus marked token, URL and instance/agent identity
values outside. No sentinel file may be opened and no marked value may
reach a child's environment or the harness's own at a gateway import or
at exit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s from the shared helper RealSinks.bridge() built its own child environment and, under the coverage runner, appended coverage's whole install directory to PYTHONPATH: a private copy of the bootstrap that let a sitecustomize.py beside coverage run in every bridge child. It now builds the env with hermetic_env(), which exposes only the coverage package. A synthetic coverage install with a sitecustomize beside it never runs in the real bridge child; a killed control shows the same plant runs once that directory is on the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…redential owner main added agent_identity with its own unquote/env-file parsing while this branch moved the bridge's parsing into gateway_credential_sources. After the rebase the bridge takes _unquote_env from the owner, so agent_identity now delegates unquote_env, file_value and per-file parsing there too: one parser for both the identity and the credential tiers, and the bridge's `_unquote_env is agent_identity.unquote_env` pin still holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9fa27dd to
c5a3c66
Compare
…every adapter connect_common (main's claude-local supervisor) carried its own systemctl runner beside Hatch's, so the package had two implementations of the policy the credential guard pins to one. Both now call service_manager: one bounded runner (`_run`, the one seam tests patch), `_systemctl` with a literal argument tuple plus a unit name it checks against UNIT_RE at runtime, and the one reachability probe. - The probe drops Hatch's `which` precheck: without systemctl on PATH the run raises, `_run` answers None, and the probe is False either way. - connect_common's argv takes Hatch's flag order (`--no-ask-password --user`); verbs, arguments, unit names and bounds are unchanged. - Both adapters' tests patch service_manager._run; Hatch's fakes read the argv through a shim that asserts --no-ask-password is present. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r into service_manager The helper, `_systemd_run` and the probe moved, so SYSTEMCTL_HELPER, SYSTEMD_PROBE and the probe's IO_ALLOWED entry name service_manager.py. A helper call may now pass `unit=` (checked against UNIT_RE at runtime) and `timeout=` beside the literal tuple and locate=True; any other keyword, positional or non-literal tuple element is still flagged. New controls: a raw systemctl run appended to connect_common, either CLI or keepalive is flagged; a second `_systemctl` def in connect_common is flagged; a unit outside UNIT_RE raises ValueError and runs nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…variable The claude-local bridge declaration names the env var the host supervisor puts the vault token in. It spelled the key as a literal, which the package guard flags. The owner now exports token_env_name(), and the declaration calls it. The guard accepts that call only bare, as a dict or keyword value. Reading the environment with it, binding it to a name, aliasing the function, using it as a dict key, or redefining it elsewhere is flagged. So are the owner's key tuples and the literal key, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Review at e7fb22c719e2dbbbceaeceda902461a70ff323b8 (base main; merge-base b95a3f86, 2 commits behind main, none touching packages/ag2-sparrow or the credential modules).
Verdict: COMMENT. No change-request condition fires at this head, and I found no blocker in the rebase delta. It is not approvable yet: no review of any kind exists at e7fb22c7, so no maintainer approval is on this head, and the REVIEW.md #15 live witness is still owed.
The new shared owner, service_manager.py (commit 4b84d6ee)
- One
_run(service_manager.py:16), one_systemd_run(:24), one_systemctl(:34), one probesystemd_user_available(:46). Hatch (adapters/hatch/cli.py:33) andconnect_common.py(:37,:405,:465,:488-516,:530,:899) both call it. The old private runners are deleted from both. - Argv: connect_common moves from
systemctl --user --no-ask-password <verb>tosystemctl --no-ask-password --user <verb>. Both are options before the verb, and systemctl parses options in any order, so this is harmless. Hatch already used this order on main. - Bounds match the old call sites exactly:
TIMEOUT_S = 30equals both the oldRUN_TIMEOUT_S(connect_common:44) and Hatch's oldSYSTEMD_TIMEOUT_S. daemon-reload / enable / restart / stop still takeshape.manager_timeout_s; disable, the show calls and uninstall's daemon-reload still take the default. Verbs, property lists and unit names are unchanged;loginctl ... enable-lingerand thelaunchctlcalls go through_runwith the same arguments and bounds. - The probe drops Hatch's
whichprecheck, but keepalive's_unit_owner(keepalive.py:344) still doeslocate=Truefirst, so "no systemctl" (None) and "manager did not answer" (UNIT_UNREACHABLE) stay distinct. - Unit check:
UNIT_RE.fullmatch(service_manager.py:13,:41) raises before anything runs. It applies to theunit=path, which is every connect_common call (ag2-sparrow-claude-local.service,ag2-sparrow-claude-local-bridge.serviceboth match). Hatch and the probe never passunit=: Hatch writes"ag2-sparrow.service"inside the literal tuple, which the regex does not see, and that is pinned bytest_every_shipped_service_name_literal_is_the_service_nameinstead. That is acceptable for literals, but "validated on every path" is true only for computed unit names. - Tree grep: no raw
systemctlorloginctlinvocation remains outsideservice_manager.pyinpackages/,src/,skills/,shared/orscripts/(the remaining hits are user-facing message strings). claude_code/cli.py has none.
Guards
- Behavioral:
test_the_helper_runs_only_an_ag2_sparrow_unitdrives the real_systemctlwith_runpatched. Mutation: replacing theUNIT_REcheck withif False:makes it fail on all 8 bad units (evil.service,show-environment,ag2-sparrow.service --all, ...). Restored: pass. - Structural (REVIEW.md #14 exception: duplication and delegation, paired with the behavioral test above). Mutation: replacing
connect_common.py:899with_sm._run(["systemctl", "--user", "daemon-reload"])makestest_the_systemd_rule_covers_every_package_moduleFAIL ("connect_common.py:899 a systemctl literal outside _systemctl"); the same for a raw call appended toadapters/claude_code/cli.py. Restored: pass. - Non-blocking gap: the literal check does not evaluate constants.
subprocess.run(["system" + "ctl", "--user", "show-" + "environment"], capture_output=True).stdoutappended toadapters/claude_code/cli.pypasses the whole credential suite (124/124). That is deliberate obfuscation, not something ordinary code does, so I am not treating it as a finding. The negative controls cover every literal-program spelling. token_env_name()(gateway_credential_sources.py:35, mirrored insrc/) returnsTOKEN_KEYS[0];claude_code/cli.py:238uses it only as a record value. The bypass list flagsos.environ.get(_sources.token_env_name()),os.getenv(token_env_name()), an aliased call and a second definition, and accepts only the dict-value and keyword forms.
Kewei's change request at 4b0568ed
- Private coverage bootstrap: resolved.
tests/gateway-credential-sources.test.py:636builds the child env with the sharedhermetic_env(); nocoverage.__file__path is added.test_a_sitecustomize_beside_coverage_never_runs_in_the_bridge_child(:668) with its live control (:678). I mutation-checked this at934e6ed9; the lines are unchanged here. - Parent instruments false-green: resolved upstream. #5703 merged 2026-10-18, #5704 merged 2026-10-21, both on main under this branch. At this head
tests/_helpers/hermetic_env.py:121setsPYTHONNOUSERSITE=1;sentinel_audit.py:25coverschannel_env_resolve;hermetic-env-sentinels.test.py:137is its case;hermetic-env-structural.test.py:648-655flagsfrom subprocess import run. - lesson 15 witness: still owed.
Hisb3d5c3f0items:PYTHONUSERBASEdropped (hermetic_env.py:16,121); bee-actions isolated (tests/bee-actions.test.py:184,hermetic_env(**e), clear=True).
Tests (offline, env -i, temp HOME, synthetic values only)
- gateway-credential-sources 124/124 OK.
- test_hatch_adapter 195 OK (2 skipped).
- test_claude_code_adapter 238 OK (it needs
tmuxon PATH; without it, 24 tmux cases fail the same way on main). - hermetic-env-sentinels 19/20:
test_a_suite_that_stops_all_patches_reads_no_outer_selectorfails here because a live heartbeat writer is running on this host. It fails identically on main9ad5126c, so it is not this PR. CI passes it. - CI at this head: 36 pass, 3 skipped, 0 failing.
license/cla: success (1 context). Unresolved review threads: 0.
Still missing before an APPROVE
- A maintainer approval at
e7fb22c7. The deciding reviews are still qingyun-wu's CHANGES_REQUESTED atad3cbb13(her later reviews are COMMENTs, which do not clear it) and keweichen's at4b0568ed. Both need a re-review at this head. - The REVIEW.md #15 witness: a real per-tier, post-restart round trip at the final head. This PR changes how the bridge and Hatch find credentials and how both adapters run the service manager. I name it; it is not a blocker in this review.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review at e7fb22c (rebased onto main, merge-base b95a3f8). COMMENT. I found no code blocker in the delta since 934e6ed. This is not an approval: the REVIEW.md §15 post-restart, per-tier witness at this head is still owed. The rebase changed live supervisor code (connect_common.py:404, 465, 489–513, the new service_manager.py), and the PR body says the witness is pending.
Delta. git range-diff c621a051..934e6ed9 b95a3f86..e7fb22c7:
- 22 commits carry over. Commits 1, 2, 9, 10, 13 and 20 differ only in rebase context.
- Four commits are new: c5a3c66, 4b84d6e, c35f8ec, e7fb22c.
service_manager. I patched subprocess.run at b95a3f8 and at e7fb22c, then drove unit_state, manager_detail, start_supervisor, stop_supervisor, linger, agent_loaded and the Hatch probe for both claude-local shapes (27 calls per side).
- The only diff is
--user --no-ask-password→--no-ask-password --user, which is semantically identical for systemctl'sgetopt_long. - Verbs, arguments, units, timeouts (30 and 240) and kwargs are unchanged. No
env=is passed on either side. - Hatch argv is unchanged by construction.
- All three shipped units match
UNIT_RE.
Mutants killed by tests/gateway-credential-sources.test.py:
UNIT_REas.*\.service: 4 failmatchinstead offullmatch: 2 fail- Hatch-only regex: 1 error
- unit check off: 9 fail
- token file-before-env: 17 fail
- url file-before-token: 2 fail
- AG2_DEVICE_ENV tier dropped: 16 fail, 4 errors
token_env_namereturns the legacy key: 1 fail
Other new commits.
token_env_name()returns the same literal as before. It is used only as a record value and reads or logs no value.agent_identitynow delegates parsing to the owner. 17 .env edge cases give identical output at the base and the head.
Tests (offline, env -i, temp HOME, synthetic values only):
- gateway-credential-sources: 124/124
- hermetic-env-sentinels: 20/20
- hermetic-env-structural: 26/26
- test_hatch_adapter: 195 pass, 2 skipped
- test_claude_code_adapter: 238/238
CI. Newest run per name: 36 pass and 3 are skipped. "tsc + tests (clean install)" is green. CLA is green, and review-checks.sh --diff passes.
Earlier findings.
- Kewei's 4b0568e blockers 1 and 2 are resolved: 4e389c4, and #5703/#5704 merged.
- My own 4b0568e follow-ups are resolved: the
CIallowlist, and the sentinels docstring at:5.
Still open:
- The §15 witness at this sha.
- The PR body predates the rebase. It still names
test/hermetic-env-ruleas the base and does not describeservice_manager,token_env_nameor theagent_identitydelegation. Please add before/after evidence for those four commits. - A maintainer approval at this head.
Nits:
connect_common.lingerspells--no-ask-passworditself instead of using_systemd_run._oknow exists in three copies.- The
service_managerdocstring says it "cannot print the manager's environment", but its own probe runsshow-environment(the output is discarded).
REVIEW.md §15 live witness at e7fb22cOwner's go. A scratch gateway bridge, CI at e7fb22c: 36 pass, 3 skipped, 0 fail. Credential resolution through the new ownerI called the PR's Bridge start, restart, round tripThe owner posted "witness 5698 task a: reply PONG and the current UTC time" in the test room. Bridge #2 wrote it to the scratch inbox, and the reply was published there: The token never appearsI loaded the token value in-process from the vault and printed counts only. I checked the full onboarding string and its secret part.
Other new surfaces, read-only (scratch HOME)
Observations (not blocking)
Teardown. TERM to bridge #2 (verified). The private tmux server was killed through its own socket, the scratch credential file deleted, no scratch process is left, and the test relay is restored. Result: PASS. After a restart under this head the bridge resolves its credential through the new owner, round-trips a real owner message, and the token appears in no log, process listing or produced file. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at e7fb22c. This replaces my CHANGES_REQUESTED at ad3cbb1. The code review is as in my COMMENT at this head: no code blocker, and the earlier findings are resolved.
REVIEW.md §15 is now satisfied by the witness at this sha (issuecomment-6330181049).
- Credential: a scratch bridge from this head's package resolved its credential through the new owner (
token_source=file, masked). - Restart: it reconnected after a TERM restart, with the PID verified.
- Round trip: a real owner turn was delivered exactly once.
- Token: 0 hits in
ps -E(before and after the turn) and across 3054 scratch files.
One gap the witness can't cover on macOS: the systemd path in service_manager. For that I'm relying on the argv harness from my COMMENT. With subprocess.run patched at b95a3f8 and at e7fb22c, I drove 27 calls each, and the only difference is --no-ask-password --user (order-insensitive for systemctl). Verbs, units, timeouts and kwargs are identical, and all shipped units match UNIT_RE. A Linux post-merge check of ag2-sparrow start/stop would close it.
CI on this head is green (no failing or pending checks), and CLA is green.
Still non-blocking: the PR body predates the rebase, and the three nits from my COMMENT stand.
Dismissed on the owner's word (merge over the open CR per his stop rule). The remaining findings are fixed (the sitecustomize leak via hermetic_env, with a regression test) or minor (parent test-tooling gaps, fixed in #5703/#5704). The §15 live witness passed at e7fb22c, and Qingyun approved at this head.
Per-task ack isolation so one failed ack no longer dead-letters a batch (#5734), the claude-local connector (#5695) with its fixes (#5767, #5769), owner-DM nudge holds (#5737, #5768), read-only collaborators and the collaborator timeout (#5687, #5690), and one owner for gateway credentials (#5698). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #5698 (one owner for gateway credential precedence and masking). Conflicts: the bridge takes its credentials from gateway_credential_sources and keeps the scrubbing hooks, gateway_call wrapping and diagnostics.describe sinks; the Hatch install diff is gateway_credentials.mask_env_assignments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#5698 owner - Hatch's wake-context registers candidates through gateway_credential_io.register_candidates, not its own key list and token-file parser; the install diff is gateway_credentials.mask_env_assignments. - gateway_instance.SinkRegistry: the owner's registry, whose values the log and status sinks withhold too (a URL by its credential parts, so the bare gateway still renders). - The owner registers every value of a token file (each assignment and each raw line) and the token file a channel .env names, which is the rotation file whenever that file wins. - Sinks main added since the branch point take diagnostics.describe(); a room_upgraded replacement is kept only as a room id holding no secret; the poll-anomaly line is logged from our own fields, not from the exception. - Tests follow: the install-diff assertions take the owner's whole-line masking; the long-poll cases expect the exception type, not its text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…._run after the rebase Main moved connect_common's manager runner into service_manager (#5698); the codex-local suite now patches the runner where it lives, as the claude-local suite already does. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Post-merge re-review at exact head
os.getenv(key=_sources.token_env_name())
os.environ.get({"token_env": _sources.token_env_name()}["token_env"])
from .gateway_credential_sources import token_env_name as key_name
os.getenv(key_name())Evidence from The first positive control fires, and The PR merged while this requested re-review was running, so a formal — Sutando (susans-MacBook-Pro) |
…r under the shared supervised-session lifecycle (#5761) * feat(ag2-sparrow): `ag2-sparrow-codex-local` hosts the Codex connector under the shared supervised-session lifecycle - adapters/codex/connect_cli.py, console script ag2-sparrow-codex-local, on connect_common as #5695 left it: install is the quiescent replacement under the per-shape operation lock (stop, prove absent and free, the marked 0600 config with ROOT, the marked unit, start, a fresh holder of this shape); run is the manager's entrypoint (AG2_SPARROW_MANAGED=1; debug-run by hand), takes the work root's lock for its own life, makes the connector's dirs under the held inode, spawns the connector as its direct child and attaches it as the consumer (holder_lock.attach_consumer); SIGTERM terminates, reaps and releases; another holder is 75, the connector's own 75 is 1. verify adds the codex pin, login status under the allowlist, the sandbox and non-owner echo and an occupancy line naming launcher and connector to the shared rows; uninstall is the shared any -> absent path. - codex_connector: --held (no lock of its own; starts once the holder's record names it, else exit 2); child_env passes CODEX_HOME when set. - README launch section rewritten for that lifecycle; the limits name what is not offered yet (the agent's default root, the per-agent bridge declaration). - tests/test_codex_local_connect.py (20): fake codex and the stub app-server, a fake manager whose start verbs run a stand-in holder, a real child for the attach, the launcher as a real process; the wrapper under tests/ and a CI line. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ag2-sparrow): the Codex README keeps the pinned upgrade-paths statement The holder-lock README pin requires the exact sentence naming the Hatch installer and the keepawake self-upgrade as the supported paths and the manual stop for everything else; the installer is stated beside it as keeping that order, not as a third path. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ag2-sparrow): codex-local runs the checked executable, serves held only as a verified consumer, and shares the holder sequence - connect_common owns the launcher-side policy both shapes ran as copies: `hold_and_supervise` (take the root, refuse a tasks symlink, the shape's prepare and spawn hooks, stop-before-start, attach the child as the consumer, reap on any failure, the release line), `supervise_child`, `kill_and_reap`, `holder_words` and a `labels` parameter on `occupancy_line` (held names the holder and its live consumer; unobserved never reads free), and the launchd entry shim (`launchd_exit`, `run_launchd`, `run_child_argv`). claude_code's hold, cli and launcher delegate to them; codex's connect_cli does too. The adapter-boundaries suite pins the delegation and refuses a copy of any owned step. - codex-local on macOS runs `run --launchd` with KeepAlive on anything but a clean exit, so a logical 75 (another holder) exits 0 and launchd stops instead of relaunching over it. - The app-server starts from the exact executable the version check passed: the launcher passes `--codex <path>` and `server_argv` runs it, never a PATH lookup; pinned with a `codex` earlier on PATH that must not run. - `--held` serves only once the holder's record names this incarnation (pid and start time) and verifies it alive; a record with liveness false or unknown, or another start time, is exit 2 with one line; pinned for all three, each wait bounded so a mutation fails instead of hanging. - Tests cover the held sequence's stop-before-start, spawn error and unconfirmed-kill branches and the shim's exit table; the fake codex is a /bin/sh wrapper that survives an interpreter path with a space (pinned); the push test and the connector env test use explicit, cleared env fixtures so an ambient CODEX_HOME cannot reach them; the differential driver's server_argv stand-in takes the executable argument. - README: the lock is the connector's first act only when it runs on its own; the launchd shim and its exit mapping; the exact executable; the verified-consumer rule; a SIGKILLed debug-run launcher orphans the connector, which keeps the root held until stopped by hand. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ag2-sparrow): the held connector lives in the root inode, debug-run takes the operation lock, and the suite leaks nothing - The launcher passes the held root's descriptor to the connector (`--root-fd`, pass_fds); under `--held` the connector fchdirs into it and names tasks/, results/ and state/ relative to it, so a work root renamed and recreated under the same pathname after startup is never read or written (the owner thread's cwd, a pathname by protocol, is the inode's current one per turn). `--held` without the descriptor is a usage error; a descriptor that is not a directory is one line and exit 2. Pinned as real processes: rename, recreate, another holder on the replacement; the moved root still answers, the replacement keeps its task. - `debug-run` takes the shape's operation lock from its checks to its acquisition (the shared helper calls `acquired()` there) and publishes its holder record under `<shape>.debug`; install confirms only the shape's own label, so a hand holder that won the root reads "did not acquire". Pinned: the lock refusal, the label with the lock freed during the serve, and install over a debug holder. - The suite: the fake codex records its JSON by temp file and rename (a poller never read a torn file); the launcher's process is ended and its pipes closed at cleanup whatever the test decided (the ResourceWarnings on Linux came from a launcher leaked by an earlier assertion); every wait is one bounded helper; the held happy path runs traced under a holder that attached it; the shared helper's prepare refusal, reap tolerance and unobserved words, the CLI's login and root refusals are covered. Diff coverage measured the gate's way: 99%. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ag2-sparrow): every Codex turn names its own cwd, so a warmed owner channel follows the moved root turn/start now carries the turn's working directory (the 0.160.1 TurnStartParams override), resolved per turn: the owner's from the held inode's current pathname (or --cwd), a non-owner's from its scratch dir. A channel whose thread opened before the root was renamed no longer works at the replacement's pathname. The rename test now completes an owner turn first, then renames, recreates and holds the replacement: the same channel's next turn and a fresh channel's both run at the moved root, results land in the held inode, the replacement gets none. The launcher cleanup terminates before it kills, so a failing case no longer orphans the connector. The boundary pin also catches an owned step imported under another name. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): codex-local's manager fake patches service_manager._run after the rebase Main moved connect_common's manager runner into service_manager (#5698); the codex-local suite now patches the runner where it lives, as the claude-local suite already does. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): codex-local connect test isolates its env before gateway imports and spawns its launcher with a hermetic child env Answers review 5618033999: isolate_os_environ() now runs at module top, before the ag2_sparrow imports, and the launcher child's env comes from isolated_child_env() instead of dict(os.environ, ...). fcntl moves below the isolation because the scanner's Python 3.9 stdlib fallback omits it. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
… with a built-in fallback (#5665) * feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback and self-upgrade Sparrow's heartbeat carries package_version and keeps the reply's setup_revision, min_version and target_version for adapters. The Hatch hook prompt becomes a stub that sends the worker to `ag2-sparrow-hatch wake-context`, which prints the served prompt and worker note from the setup cache, else the built-in copies (today's wording verbatim); the package's tier notes are always printed and served tier_addenda follow them. Served tunables are clamped to package bounds. The keepawake run refreshes the cache through one fetch boundary (no transport yet, so it returns None) and starts one detached, hash-pinned upgrade per target version, rolling back on any failure and reporting upgrade-failed; a changed hook definition wakes the agent once instead of being applied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): write the heartbeat's setup hint through the bridge's durable writer The bridge allows file moves only in its known movers, so the hint goes through _durable_write instead of its own temp-and-replace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): run the served-setup suite under repository coverage Adds the tests/ wrapper the coverage job discovers, tests for each rollback step, restart, spawn, the detached entry and the fetch boundary, and writes the setup cache through the adapter's existing atomic writer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(ag2-sparrow): the wake context tells the worker to reach its owner only in the owner's DM A canonical owner note, printed whatever the served text says, with the owner's DM room when Sparrow has read it from AG2 Space (its owner-routing state file). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): self-upgrade stops, installs, then starts, under a lock, with an offline rollback - Order: snapshot, stop Sparrow (SIGTERM, then wait), pip install, pip check, install re-run, start, verify. A reply queued while Sparrow is stopped stays in the results dir and its outbox and is delivered after the start; keepawake leaves Sparrow alone while an upgrade holds the lock. - Lock: run_upgrade and the attempt record are taken under a non-blocking flock on STATE_DIR/upgrade.lock; a second run logs "upgrade already running" and skips. - Rollback: every file the install recorded is copied to STATE_DIR/rollback/<version>/ before the install and restored on failure, with no index and no network. Editable and URL installs (direct_url.json) are never upgraded. - The package's tier and owner notes come last, after "These rules override anything above:". Every decision is logged on the keepawake line, AUTO_UPGRADE=0 included. A started attempt older than 30 minutes counts as failed. pip check rolls back only on a new problem naming ag2-sparrow. pip runs with --only-binary=:all:. Versions parse ASCII digits only, at most 64 characters, trailing zeros ignored. heartbeat_cadence_s bounds are [30, 90]. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): served setup fails closed to the built-in text; self-upgrade moves to its own PR - A cached setup older than the setup_revision Sparrow last heard announced is never used: wake-context and the health row fall back to the built-in text and defaults, and a failed fetch never brings the stale cache back. A fetched setup that does not validate is never cached, so a corrected one at the same revision is still fetched. - wake-context shows owner_dm_room only while the owner-routing record is bound to the identity and gateway the running Sparrow reports, as the bridge requires; the gateway URL redaction moves to gateway_instance.redact_url so both read it one way. - Served commands must be the reply command with plain words: no shell syntax. - A served keepawake interval shorter than the installed task's cannot flag a normal gap. - self_upgrade, AUTO_UPGRADE and the hook's upgrade notice wake leave this PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): wake-context binds the owner DM to the full gateway URL, not its redacted form The bridge's owner-routing writer now stores a sha256 of the agent identity and the full gateway URL, query included, and the gateway status it already writes carries the same digest for the running Sparrow. wake-context shows owner_dm_room only when the record's identity and full URL digest equal the running Sparrow's. The redacted URL stays for display, so two enrollments that differ only in the query string no longer match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): the owner-DM binding is an HMAC under a per-install key, never the URL 3a55fd8 persisted the raw gateway URL in owner-routing.json (as the bridge already did) and a plain sha256 of identity + full URL there and in gateway-status.json, both in the state dir, which may be synced. Now the bridge's existing routing writer stores the agent id, the redacted gateway URL and binding = HMAC-SHA256(per-install key, identity + full URL); the status file carries the same binding for the running Sparrow. The key lives in ~/.config/ag2-sparrow/binding.key (0600, outside the state dir), created by the bridge only. The bridge's reload and wake-context compare bindings through the one helper, gateway_instance.binding_digest; a record from before this change still reloads by its stored full URL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): only the owner-routing writer creates the binding key, under the config home The bridge suite pinned the routing record's old shape (raw gateway URL); it now pins the redacted URL plus the HMAC binding, with its key under a temporary config home. The key path follows XDG_CONFIG_HOME, else ~/.config, and only the owner-routing writer creates it, so the status writer and the reload never write outside the state dir. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): migrate legacy routing records, key under the passwd home, publish the key atomically - A pre-binding owner-routing record (full gateway URL on disk) is rewritten on load through the bridge writer as the redacted URL plus the keyed binding, atomically and once; the bridge creates the key on that path, so a cold upgrade keeps the owner DM. A record that cannot be bound is never written, and an unmigrated reading is not marked persisted, so the writer retries. - The binding key lives under the effective uid's passwd home, never $XDG_CONFIG_HOME/$HOME, so a service unit and the Hatch hook resolve one key. - The key is written to a 0600 temp file, fsynced and os.link'ed into place; on EEXIST the existing key is used. An empty, short, malformed, non-0600 or foreign-owned key is refused, and replaced only once older than a 5 s grace. - redact_url fails closed to a placeholder; the startup and token-rotation logs name only the redacted gateway. Digests compare with hmac.compare_digest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): tier notes come from task_io; the key home is holder_lock's - The built-in runtime text's tier note is task_io.non_owner_note(), the one owner of the non-owner policy the hook already reads; the JSON copy is gone, and the test pins the hook's read and the built-in text to it. - binding_key_path() uses holder_lock._home() (effective uid's passwd home); the local copy of that lookup is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): single-winner key recovery, URL-safe error text, stale routing temps swept - Creating or recovering the binding key runs under an exclusive lock on the key dir (file_lock), and every writer re-reads the key under it before binding, so two writers recovering a bad key end up with one key. - Every bridge log line and the gateway-status error pass through gateway_instance.redact_text: the full gateway URL becomes its redacted form, and text still carrying userinfo, the query, a query value or the fragment is withheld whole (an InvalidURL from a URL with a space in its query). - On load the bridge removes the pre-binding writer's crash leftovers (.owner-routing<sfx>.json.<pid>.tmp): regular files in the state dir only, this uid's, past a 5 s grace, whose pid is dead; symlinks are not followed. - owner_dm_room compares bindings with hmac.compare_digest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): the key home goes through task_io; the routing-temp sweep is a known mover - task_io stays the one module that imports holder_lock: it gains user_home() (holder_lock._home(), None without a passwd entry), and binding_key_path() uses it instead of importing holder_lock. - The disposal guard lists _sweep_routing_temps as a known mover: it unlinks only dead writers' owner-routing temps in the state dir, never a result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): fail-closed URL redaction in every rendering; routing migration has its own owner - redact_text withholds text when any credential-bearing URL part survives in any rendering: raw, repr/unicode_escape-escaped (a tab, LF or DEL in the query, as http.client's InvalidURL renders it) or percent-encoded/decoded. Parts are matched with the delimiter the URL puts before them (`=value`, `k=v`, `?query`, `userinfo@`, `#fragment`), so a short value is caught with no length guard. Parts are split by hand as well as by urlsplit, which drops tab/CR/LF, and an unparseable URL is covered by the hand split. - The owner-routing migration (legacy record rewrite, old-writer temp sweep) moves to routing_migrations: idempotent, never raises, and retried at each load until it finishes; the bridge's _load_routing runs it first, before the reading is prepared. A temp past 1 h is stale whatever its pid says. - A key with a trailing newline is refused; binding comparisons use bytes, so a non-ASCII binding no longer raises. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): a temp-sweep error no longer blocks the routing rewrite; pin three redaction/compare paths - routing_migrations: an OSError on one swept entry skips that entry and leaves the migration unfinished (retried at the next load), but the legacy record rewrite still runs; a temp that vanishes mid-sweep is not an error. - Tests pin the userinfo parts (u:SECRETPW@, token@), the bare-key query parts (?flag, &flag, ;flag) and the bytes compare_digest in wake-context and in the bridge's reload (a non-ASCII binding is refused, never raises). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): gateway-call errors carry only their type; tri-state routing migration; age-only sweep - Redaction by construction: every call that receives the gateway URL (_req, the reenroll claim) runs inside _gateway_call, which re-raises what it raises as a subclass of the same type whose text is only "<type> contacting <redacted gateway>" plus the HTTP status; text and bytes attributes are dropped, the status code, errno and an HTTPError's body stay. The event channel and the card poster, which hold the URL too, keep only the exception type. redact_text stays as a second net on every log line and status error. - routing_migrations returns done (rewritten and read back), not_needed, or failed (unknown: a read error, an unbindable, unwritten or unverified record, or an unfinished sweep), retried at the next load. After a rewrite the bridge reads the record from disk again before using it. - The legacy temp sweep is age-only: older than 1 h is stale whatever pid its name carries, this process's own included. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): every gateway-URL call is wrapped, guarded by a static test; scrubbing excepthooks - gateway_instance.gateway_call is the one wrapper; the bridge's _gateway_call delegates to it. Every call that receives the gateway URL or a URL derived from it now runs inside it: _req, the reenroll claim, the media fetch, the event channel's stream open and the card poster's post. - The event channel catches any exception around its open and keeps only the type, so a control character's InvalidURL no longer kills its thread. - A scrubbed exception exposes no __context__, and nested exceptions in its attributes (URLError.reason) are scrubbed too. - The bridge installs scrubbing sys.excepthook and threading.excepthook: an uncaught exception prints as its type and redact_text'd text only. - A static guard finds every Request/urlopen/build_opener/HTTP(S)Connection call in the package and fails unless it sits inside gateway_call (the routine adapter, which talks to a different endpoint, is the one exemption). - redact_text compares percent escapes case-insensitively. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): log and status sinks take structured safe fields only The sinks are the boundary. A log line or a status field takes an exception's type (with an OSError's errno name and an HTTP status), the redacted gateway identity and text the code wrote, never an exception's text, a response body or gateway-supplied free text. Behaviour code still reads HTTPError bodies for control flow. - diagnostics.py owns the policy: describe(exc) for any exception, code_field for a gateway-supplied token (an error name, an approval code: a short token absent from every form of the URL), and safe_status, which nets every nested string of the status payload through redact_text and withholds non-JSON values. - Every exception interpolated into a log or a status field in the bridge, the event consumer, the default observer and the Hatch keepalive goes through describe (the vendored local-file core modules are outside it). - Reenroll: the refusal body is never logged; refused_error and the approval code are kept only as code_field tokens. - Static guards: no exception object or response body reaches a sink except through describe/type/.code; the status file is written only through safe_status; gateway_call is matched by exact name, with aliases, openers and bare references resolved. The production _reenroll_claim() is driven with credential forms and stdout, stderr, the log file, the status file and the whole test home are scanned together. - Smaller: a dedented per-row assertion loop, a nested-reason scrub test, and the excepthooks report the innermost file:line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): owner-routing migration without a latch; the reading is rebuilt from disk on every load - routing_migrations runs on every load and reports what disk still holds: done (it migrated and nothing is left), not_needed, pending (a legacy temp still younger than the 1 h bound is left for a later load), or failed. No state is latched, so a young temp is swept by the same process once it ages past the bound, and a failed read-back after a committed rewrite can no longer mark anything complete. - _load_routing has no loaded/migrated latch: it migrates, then rebuilds the in-memory reading from the verified disk record each time. A gateway reading the writer has not committed yet is kept until its write commits. A re-enrolment is detected against the reading's identity and gateway. - Tests: a young temp ageing through _load_routing in one process; a failed read-back after a committed rewrite still yields the owner DM; the bridge source carries no latch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): the nested-reason case uses ECONNREFUSED by name, not a macOS errno Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): in-process cases for the excepthooks, the event channel's type-only status and the migration's read-back and sweep errors Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): diagnostics.describe names the reason, errno and raising line describe() now reads `URLError(ConnectionRefusedError/ECONNREFUSED) contacting <redacted gateway> at <file>:<line>`: the reason's type and errno name (an EAI_* name for a resolver error), so refused, DNS and timeout read differently, and the innermost frame's file name and line. A KeyError gives its key only when the failing code wrote it as a string literal; otherwise just its type. The poll loop's catch-all, the event channel and the card poster log through it, so every location comes from one place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): one secret registry for the sinks, a taint-following sink guard, foreign routing clears - diagnostics keeps one registry of the process's secrets: every credential part of the gateway URL, the bearer token, the media token and any token read from a file. Logs, status fields and the excepthooks withhold text holding any of them in any rendering, and code_field refuses a value that holds one or is a piece of one, so an echoed bearer is neither a 403 error name nor an approval code. A bare secret shorter than 8 characters is not matched alone; delimited URL parts are, at any length. - The archive-reconcile note and the room-gate log now go through describe, with the health-snapshot error and a logged proactive response. They slipped past the sink guard because it checked sink arguments only; it now follows exception text and response bodies through local text assignments and into a same-module helper's parameter, in line order. - A foreign owner-routing record clears a reading that came from disk. - describe trusts a gateway attribute only on a scrubbed exception class; safe_status withholds non-finite numbers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): a call handed a sink function is itself a sink; two report_once sites describe - The settle-ledger read and the claim-recovery failure handed raw exception text to disposal.report_once(..., _log, ...); both now go through diagnostics.describe. The sink guard missed them because _log was passed as a value: any call handed a sink function as an argument is now a sink for its other arguments. The revert test covers both sites. - A test runs a registered secret through the bridge's _log and status writer and through safe_status, so dropping the registry from either fails. - Matching: percent-escape hex in any case, the bytes of a secret exactly (code_field no longer lower-cases them), and redact_text matches the URL's scheme and host in any case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): tokens match at any length; URL username registered; hook record scrubbed; owner DM only while connected - The bearer, media and file-read tokens are registered at any length: the 8-character floor now applies only to bare URL query values, where a short one would withhold ordinary words. A short bearer echoed as a 403 error or an approval code reaches no sink. - The URL's username is registered on its own, beside the whole userinfo and the password. - The excepthooks scrub every piece of the record (thread name, file name, type, text) and withhold the whole record if a secret spans the pieces. - owner_dm_room hands out the owner DM only while Sparrow's status says it is connected, with a matching binding. - safe_status runs the registry on values; keys, the payload's own field names, go through the URL net only. - The sink guard treats a call handed a sink function (by name) as a sink; a variable that happens to be called log or error is not one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): close the red-team findings: setup hint, owner DM, case-folded echoes, every hook - F1: the heartbeat reply's setup_revision is kept only as a code_field token and its versions only as PEP 440 (diagnostics.version_field); the hint then runs through safe_status and is written with NaN refused. - F2: gateway_instance.owner_room is the one check for an owner DM: a strict Matrix room id on the agent's own homeserver, holding no registered secret. The bridge applies it to /v1/agents, the routing record (writer and reader), the withheld-review cache and a created room; wake-context applies it again before printing, with the device env's bearer and URL registered in its process. - F3: secret matching is case-folded on both sides everywhere (the registry, code_field, redact_text's credential parts), so an echo in another case is withheld. - A scrubbing sys.unraisablehook, which an exception in an atexit callback also reaches. - _guarded_result_body reports describe(exc), and the Hatch install diff withholds credential-named values and registered secrets. - Tests kill the four surviving mutants: the spread-across-pieces hook check, the bare password, the bare fragment, and delimited parts at any length. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): an owner DM that is a piece of a secret is refused; behavioural review-failure test - diagnostics.holds_or_is_piece is code_field's rule, now shared: owner_room also refuses a room whose localpart (6 characters or more) is a piece of a registered secret in any case, so "!<part of the bearer>:ag2.space" reaches neither the routing files nor the prompt. - The private-review test is behavioural: _route_withheld_review raises an OSError whose path holds a canary, and neither _guarded_result_body's reason nor the caller's log carries it. - Tests pin that _routing_record never stores an unchecked owner DM and drive the vendored result_disposal lines through the bridge's _log with the bearer in the path, exact and upper case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): withhold every credential candidate, mask install diffs, keep refused revisions refused - Hatch registers the env and device-file credentials (both aliases, the media token, every line of a rotated token file) each on its own, so the one precedence discards is still withheld from wake-context and the diff. - The install diff withholds every value on an Environment= line and any credential-named value; other URLs keep only scheme, host and path. - A heartbeat whose setup_revision is refused records setup_revision_withheld, and runtime_text treats it as a mismatch: no stale served text revives. - /v1/agents.owner is admitted only through gateway_instance.owner_mxid (Matrix user id, no secret, no piece of one) before it is cached or invited. - team_result_guard load failures reach the log as diagnostics.describe(exc). - diagnostics.forms is total over lone surrogates, so a non-UTF-8 token in a bytes environment no longer stops the bridge importing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): Hatch and the bridge register credentials through the #5698 owner - Hatch's wake-context registers candidates through gateway_credential_io.register_candidates, not its own key list and token-file parser; the install diff is gateway_credentials.mask_env_assignments. - gateway_instance.SinkRegistry: the owner's registry, whose values the log and status sinks withhold too (a URL by its credential parts, so the bare gateway still renders). - The owner registers every value of a token file (each assignment and each raw line) and the token file a channel .env names, which is the rotation file whenever that file wins. - Sinks main added since the branch point take diagnostics.describe(); a room_upgraded replacement is kept only as a room id holding no secret; the poll-anomaly line is logged from our own fields, not from the exception. - Tests follow: the install-diff assertions take the owner's whole-line masking; the long-poll cases expect the exception type, not its text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): owner_mxid pins the owner to the agent's homeserver owner_mxid checked for a piece of a registered secret only in the localpart, so an owner whose server part carried a piece of the bearer (in any case, or with a suffix) was kept, written to the withheld-review-dm cache and invited. It now applies owner_room's whole rule: the owner's server must be the server part of the agent's identity, so no server part can carry a piece. A piece check on the server part would instead refuse the real homeserver. Tests: the two server-part repros go through _gateway_owner and _owner_review_dm; the unit cases add server-part pieces, a foreign server, a missing identity, and the ROOM_PIECE_MIN boundary on the localpart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): the install diff withholds five more valid systemd shapes test_install_withholds_every_environment_assignment_... now also passes an Environment line with whitespace around `=` (both spacings), a continued Environment group whose second line holds the token, an `env` assignment on ExecStart, and a `--token` argument. mask_env_assignments withholds each one; the per-line masker it replaced printed all five verbatim. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(ag2-sparrow): build every gateway-reaching test environment with the hermetic helper tests/hermetic-env-structural.test.py flagged six sites: two child environments in test_hatch_served_setup.py copied os.environ minus a denylist, and four files imported a module that now reaches the credential contract with no module-top isolation. The two child environments are now hermetic_env(home, ...) with the same overrides, and each file isolates os.environ before its first project import. test_event_consumer.py and taskify-redacts-room-bodies.test.py come into scope once diagnostics delegates to gateway_credentials, so they are isolated here too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(gateway-credentials): a URL key's plain URL registers by its parts; bound the token file's raw line The discovery owner registered every candidate whole, so a plain gateway URL read under a URL key withheld its own redacted form from any sink that shares the registry. register_pairs() now registers such a URL by its credential parts (register_gateway_url), keeps every other value whole (a token key's URL-shaped bearer included), and env_files carries (key, value) pairs so the rule can see the key. This moves the routing the Hatch SinkRegistry did into the owner. token_file_values registered every '='-free line of a token file at any length, so a stray "ok" blanked log lines. token_file_pairs keeps each assignment and adds only the raw line token_file_token would read once no assignment is left; beside a token assignment it counts only at BARE_SECRET_MIN or longer. Tests (both the src owner and its vendored copy): the bare gateway renders after discovery while a URL's credential parts, a URL-shaped bearer and the file token stay withheld; the raw line is registered from an env-named and a channel-named token file; a short line beside an assignment and any later raw line stay visible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(ag2-sparrow): the log and status sinks call the credential owner's masking; delete the private copies diagnostics.py and gateway_instance.py held a second copy of the masking contract gateway_credentials owns (fold, forms, _views, secret_parts, redact_url, redact_text, describe, code_field, safe_status), and it had drifted: with no systemd-unescape view, an escaped echo of a registered token such as `synth\x2ddrift-...` passed diagnostics.contains_secret while the owner's registry caught it. SinkRegistry only fed one registry from the other. Now the owner is the one implementation and the package keeps one registry: - shared/ag2_gateway_credentials.py (regenerated into the package and the room-ops skill) gains what only the copy had: scrub_exception, describe's " contacting <gateway>" for an exception it made (scrubbed through an optional registry), version_field, safe_status's optional registry on values only, and the surrogatepass percent form beside the escaped one. - diagnostics.py holds REGISTRY, the process's one SecretRegistry, and each function delegates to the owner with it. - gateway_instance.py re-exports the owner's redact_url, redact_text, REDACTED_* and scrub_exception; SinkRegistry, register_gateway_secrets' body and the URL helpers are gone. owner_room and owner_mxid share one helper, so their checks cannot drift apart. - The bridge's _SECRETS is diagnostics.REGISTRY; its log, status and _redact_url call the owner. Hatch registers device candidates into the same registry. Where the owner is stricter (systemd-unescaped and unquoted views in contains and redact_text), the sinks now are too. Tests: an escaped echo of the bearer (\x2d, \x66, \055) through the bridge's _log and _emit_gateway_status is withheld (fails at 4a5d2eb, where the log carries all three). A guard in gateway-credential-sources fails if a package module defines a masking-contract name that does not delegate to the owner, repeats an owner function's body under another name, subclasses SecretRegistry, or holds a module-level registry outside diagnostics; at 4a5d2eb it reports 18 sites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ag2-sparrow): the saved AGENT_MXID reaches the launched Sparrow, and a revision has one spelling Install saves AGENT_MXID in its config, but neither the user service's environment nor the detached Sparrow the keepawake run starts carried it. Without it the bridge has no identity, so no gateway_binding and no owner_dm_room, and the owner-DM wording never activated on a normal install. `install_service` and `_spawn_sparrow` now read the saved value through `cli.saved_mxid`; with none saved (or not an MXID) they set nothing and print one NOTE line. A served setup's `revision` was accepted as an integer and cached as one, while the bridge's hint kept an announced `setup_revision` only as a `code_field` string token, so an integer revision was withheld on one side and never equal on the other. `gateway_credentials.revision_field` is the one normaliser (an integer that is not a bool is its decimal text, then code_field's rules); the bridge's hint writer and runtime_text's setup_problem, refresh_cache and current_cache all call it through diagnostics. The generated copies are regenerated from the canonical source. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ag2-sparrow): a launched Sparrow runs only as the saved identity, and every cached revision is read normalised The detached launch built the child's environment from its own, so with no saved AGENT_MXID a caller's AGENT_MXID or AGENT_ID passed through and the bridge, whose resolver reads the process env first, ran as that identity. `_spawn_sparrow` now removes every key in `agent_identity.ENV_KEYS` in every branch and sets only the validated saved MXID. A user unit inherits the user manager's whole environment, so the unit writer gains `UnsetEnvironment=` for the identity keys it does not set; it is applied after `Environment=`, so the saved identity is never also unset. `current_cache` normalised a cached revision only against an announcement and otherwise returned the raw cache, so a no-hint cache with a bool, float, missing, malformed or secret revision stayed usable and a legacy integer stayed an integer. `runtime_text.cache_revision` is now the one reader, with the registry and the redacted gateway URL Sparrow reports: `current_cache` normalises before the comparison and returns a copy whose revision is text, and a revision that does not normalise reads as no cache. `keepalive main` registers the device secrets before any cache is read, as wake-context does. An end-to-end test runs the shipped install and the real spawn with a stand-in Sparrow that is the real bridge module (only the gateway call is faked), from an environment carrying a foreign identity in both resolver keys: the launched bridge binds as the installed identity and wake-context names the owner DM; with no saved identity it binds as nobody and wake-context names no room. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ag2-sparrow): a launch pins the bridge's identity at every resolver tier, and a legacy cached revision is written back as text The Hatch launchers removed only the process-env identity keys, so a Sparrow launched with no saved AGENT_MXID still resolved an identity from a later tier: an AGENT_MXID / AGENT_ID / AG2SPACE_USER_ID alias in the device env file or the inherited channel .env, or the durable state/auth/ag2space.json binding. The policy now lives in the resolver. agent_identity.resolve honours a launch pin (AG2_IDENTITY_PINNED): a pinned environment answers from its own AGENT_MXID alone, so a pinned bridge runs as the saved identity or as none at every tier. launch_environ / pinned_environ are the one way a launcher builds that environment; the detached spawn and the systemd unit both use them. Channel config (CLAUDE_CONFIG_DIR, AG2_DEVICE_ENV) is unchanged: only identity is pinned. The re-enroll log no longer tells a pinned bridge to write its identity into a channel .env, which it would now ignore. Tests pin each tier for saved and no-saved launches through the bridge's own resolver: every FILE_KEYS alias in both file candidates and a durable record, under the detached child's real final environment and the rendered unit's final environment; the token-only end-to-end test plants all of them and waits for the bridge to report connected instead of a fixed settle. A cache an older build wrote with an integer revision is now rewritten with the text revision. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Priority
Level: high
Reason: a prerequisite for #5665. Its credential handling was split between the live bridge and the Hatch adapter, so each fix missed a site; this gives that policy one owner first.
Closes
N/A. This is the prerequisite agreed on #5665 (option A): #5665 (comment)
Stacked PR
Merge order: A → B → C.
test/hermetic-env, based onmain).test/hermetic-env-rule, based ontest/hermetic-env).test/hermetic-env-rule.After A and B merge, this PR retargets
main. #5665 depends on this PR. After this merges, #5665 rebases onto it, removes its private copies of this policy (the bridge's and the Hatch adapter's), fixes its own findings 3 and 6, and then runs the final-SHA tests and the §15 witness.Split again at 4b0568e: the sweep is two stacked PRs
At 355 files, #5703 was itself past the 300-file limit of
gh pr diff, so the sweep is now two PRs. There was no force-push.test/hermetic-env) keeps the helpers and the first 174 conversions. Staging commit 78ce9eb takes out the rest, and the branch passes onmainby itself.test/hermetic-env-rule) is a single commit reverting 78ce9eb. It brings back the other 174 conversions, the structural rule and the sentinel audit, and its tree equals the full sweep.test/hermetic-env-rule(merge commit 4b0568e, tree unchanged from 28b2423), and its base is nowtest/hermetic-env-rule. Its diff is still the same 18 files.Split at 28b2423: the test-hermeticity sweep moved to #5703
At 5949fcf this PR changed 371 files, past the 300-file limit of CI's
gh pr diff. Most of them were the repo-wide test-hermeticity sweep.test/hermetic-env, and its own PR, test: one hermetic environment for every credential-touching test #5703, based onmain. It carriestests/_helpers/hermetic_env.py,credential_scope.py,sentinel_audit.py, the structural and sentinel tests, and 348 test conversions that do not depend on this PR's code.tests/skill-symlink-ephemeral-target.test.pyno longer derives its durable fixtures fromPath.home(), which is now a temp directory.test/hermetic-envwas merged into this branch (merge commit 28b2423), and the base was changed totest/hermetic-env.bounded_file_read.tests/gateway-credential-sources.test.py, and the sentinel case that audits it.--no-serviceand systemd behaviour that only this PR adds.skill-symlink-ephemeral-targetfix.env -iwith a temp HOME and USER:gateway-credential-sources: 120/120;skill-symlink-ephemeral-target: 9/9;At 5949fcf: no host user site in any child; scope derived from the code that reads credential names (reviews 5564207460, 5564171259)
Blocker 1: the host user site defeated the sentinel audit.
hermetic_envcarriedPYTHONUSERBASE, so a host.pthran beforesentinel_auditinstalled its hook. A real sentinel read and "0 leaks" looked the same.PYTHONNOUSERSITE=1, andPYTHONUSERBASEis not forwarded. The user site is never turned back on.SUTANDO_TEST_SUBPROCESS_COVERAGE=1orCOVERAGE_*), the parent importscoverageand adds its install directory toPYTHONPATH. APYTHONPATHdirectory runs no.pthfile.tests/ag2-sparrow-drift.test.pylaunchespython -m coverage runchildren. Underenv -iwith only the CI-shaped variables (SUTANDO_TEST_SUBPROCESS_COVERAGE=1 PYTHONFAULTHANDLER=1, parent undercoverage run), it passes. With the bootstrap removed, the child fails withNo module named coverage.UserSitecases intests/hermetic-env-sentinels.test.pyplant a synthetic user-site.pththat copies the sentinel token file:PYTHONUSERBASE, it must not run, andhermetic_envmust not carry the variable;Blocker 2: bee-actions, and the scope behind it.
tests/bee-actions.test.pynow isolates at module level._run_room()overlays its fixture withpatch.dict(os.environ, hermetic_env(**e), clear=True), so an outerGATEWAY_*/RELAY_*value can no longer outrank the fixture'sREMOTE_TASK_TOKEN.test_outer_gateway_selectors_never_reach_a_room_request. SentinelGATEWAY_TOKEN/RELAY_TOKENandGATEWAY_URL/RELAY_URLare set, with the URLs aimed at the localhost stub. Forpost-roomandregister-room, the stub must see onlyBearer tok-gwand no sentinel path.tests/bee-actions.test.py.tests/_helpers/credential_scope.pyscanssrc/,skills/andpackages/for modules whose code (not comments or docstrings) holds aGATEWAY_/RELAY_/REMOTE_TASK_/AG2_REMOTE_name as a string constant. Today that is 16 modules. A test that names one by file, import or dotted path is in scope. This brought in 205 test files, all converted: each isolates at module level, and its child envs come fromisolated_child_env(). The modules behind them arehealth-check.py(most), task-progressnotify.py,runtime-health.py, runtime-apidispatcher.py,channel_token,ensure-cron-room.py,bee_actions.pyand room_ops_gateway.py.Non-blocking, included.
VALUESis now the fixed identity names plus every token, URL and identity name the same scan finds. A case pins that the scan is non-empty and covered.PYTHONUSERBASEfollow-up from 5564171259 is closed by blocker 1.Revert checks (each fails as expected, then passes again when restored).
hermetic_envrestored to b3d5c3f: both user-site cases FAIL ("a host .pth read the sentinel token file before the audit hook"). This is the reviewed repro.PYTHONUSERBASEforwarded again, withPYTHONNOUSERSITEkept: FAIL (PYTHONUSERBASEpresent in the child env).PYTHONNOUSERSITEdropped: FAIL._run_roomback topatch.dict(os.environ, e): FAIL on both verbs.bee-actionsand one converted health-check file restored to b3d5c3f: the structural tree test FAILS (unisolated,copy).test_scope_is_derived_from_the_modules_that_read_credential_names).Counts. All runs used
env -iwith a temp HOME and USER, one file at a time.skill-symlink-ephemeral-target,start-cli-model-pin); both are green in CI at b3d5c3f.git diff --checkclean, review-checks PASS.The 205 newly in-scope test files
tests/a-fallback-hits-probe.test.pytests/agents-md-sync.test.pytests/bee-actions.test.pytests/bridge-skill-path-resolution.test.pytests/briefing-all-clear-verified.test.pytests/channel-token-relay-quoting.test.pytests/channel-token-vault-fallback.test.pytests/claude-hooks-autofix.test.pytests/cli-auth-failure-needs-login.test.pytests/cli-wedge-health-probe.test.pytests/codex-scheduler-import-without-plistlib.test.pytests/core-health-verdict.test.pytests/core-input-watch.test.pytests/core-model-pin-probe.test.pytests/core-restart-heartbeat-and-cron-hold.test.pytests/core-supervisor-relay.test.pytests/cron-notify.test.pytests/daily-punctuality-completion-sentinels.test.pytests/discord-post-gate-wiring.test.pytests/discord-readers-429.test.pytests/discord-token-delegation.test.pytests/engine-conflict-resolve.test.pytests/gateway-age-helpers-overflow-safe.test.pytests/gateway-connected-requires-last-ok.test.pytests/gateway-serving-shared-owner.test.pytests/git-binary-resolution.test.pytests/github-status-authority.test.pytests/health-check-alert-history-legacy.test.pytests/health-check-alert-history-prune.test.pytests/health-check-artifact-rebuilt-under-process.test.pytests/health-check-bodhi-dist.test.pytests/health-check-bridge-launcher-dupe.test.pytests/health-check-bridge-log-content.test.pytests/health-check-bridge-log-freshness.test.pytests/health-check-bridge-probe-coverage.test.pytests/health-check-bridge-skip.test.pytests/health-check-carrier-set-enforced.test.pytests/health-check-claude-hook-registration.test.pytests/health-check-claude-task-notifier.test.pytests/health-check-codex-presence.test.pytests/health-check-codex-task-notifier.test.pytests/health-check-comm-sweep-freshness.test.pytests/health-check-commits-behind-shallow.test.pytests/health-check-community-support.test.pytests/health-check-compiled-artifact-stale.test.pytests/health-check-context-read-budget.test.pytests/health-check-conversation-server-pin-composes.test.pytests/health-check-core-quota.test.pytests/health-check-core-request-rejections.test.pytests/health-check-core-supervisor.test.pytests/health-check-credential-proxy-drivability.test.pytests/health-check-credential-proxy-staleness.test.pytests/health-check-cron-runner.test.pytests/health-check-cron-schedule-liveness.test.pytests/health-check-daily-punctuality.test.pytests/health-check-dangling-symlinks.test.pytests/health-check-disk-space.test.pytests/health-check-dynamic-loop-freshness.test.pytests/health-check-emit-task.test.pytests/health-check-engine-revision-drift.test.pytests/health-check-fix-launchd-label.test.pytests/health-check-gateway-lane-verdict.test.pytests/health-check-gateway-outage-duration.test.pytests/health-check-gateway-probe-unknown.test.pytests/health-check-gateway-stale-lane.test.pytests/health-check-gateway-transient-outage.test.pytests/health-check-git-stdin-no-pipe.test.pytests/health-check-git-stdin-pipe-pool.test.pytests/health-check-held-no-consumer.test.pytests/health-check-hitl-archive.test.pytests/health-check-host-subtrees-self.test.pytests/health-check-import-without-plistlib.test.pytests/health-check-index-trend-call-site.test.pytests/health-check-index-trend-unavailable.test.pytests/health-check-legacy-notes-divergence.test.pytests/health-check-live-tree-drift.test.pytests/health-check-lstart-blank-line.test.pytests/health-check-memory-dir-override.test.pytests/health-check-memory-dir-siblings.test.pytests/health-check-memory-freepct.test.pytests/health-check-memory-index-trend.test.pytests/health-check-memory-pressure.test.pytests/health-check-memory-sync-vault.test.pytests/health-check-memory-sync.test.pytests/health-check-migrate-reader-contract.test.pytests/health-check-names-workspace.test.pytests/health-check-notes-split-brain.test.pytests/health-check-notify-gateway.test.pytests/health-check-notify-slack.test.pytests/health-check-optional-service-pin-composes.test.pytests/health-check-orphaned-results-skip-markers.test.pytests/health-check-orphaned-results.test.pytests/health-check-outbox-parked.test.pytests/health-check-pane-key-fence.test.pytests/health-check-per-host-config-backup.test.pytests/health-check-phone-server-restart-safety.test.pytests/health-check-pin-covers-every-prescription.test.pytests/health-check-pin-orphan-surfaces.test.pytests/health-check-pin-veto-survives-later-checks.test.pytests/health-check-pin-vetoes-voice-kickstart.test.pytests/health-check-pinned-proxy-stays-live.test.pytests/health-check-pool-advertisement.test.pytests/health-check-pool-suspended.test.pytests/health-check-pool-watchers-all-tracked.test.pytests/health-check-probe-drain.test.pytests/health-check-probe-names-its-subject.test.pytests/health-check-proxy-pin-withdraws-restart-advice.test.pytests/health-check-punctuality-completed-unpublished.test.pytests/health-check-punctuality-dispatch.test.pytests/health-check-punctuality-scores-dispatch.test.pytests/health-check-quarantine-skip-markers.test.pytests/health-check-quota-account-identity.test.pytests/health-check-quota-telemetry.test.pytests/health-check-recover-core.test.pytests/health-check-recover-cron.test.pytests/health-check-recovery-telemetry.test.pytests/health-check-root-tidy-personal-assets.test.pytests/health-check-secret-scanner-mode.test.pytests/health-check-session-cron-stamp.test.pytests/health-check-severe-status-glyph.test.pytests/health-check-skill-junctions.test.pytests/health-check-skill-symlinks-live.test.pytests/health-check-skill-symlinks.test.pytests/health-check-skills-driver-drift.test.pytests/health-check-slack-unfurl.test.pytests/health-check-stale-proactive-backlog.test.pytests/health-check-stuck-loop.test.pytests/health-check-summary-warnings.test.pytests/health-check-supervised-bridge-restart.test.pytests/health-check-supervised-watcher-not-orphan.test.pytests/health-check-sutando-app-absent.test.pytests/health-check-sutando-app-electron.test.pytests/health-check-sutando-app-hotkeys.test.pytests/health-check-symlink-fix-reachable.test.pytests/health-check-sync-conflicts-unmerged.test.pytests/health-check-task-claim-age.test.pytests/health-check-task-queue-pool-aware.test.pytests/health-check-task-queue-vanish.test.pytests/health-check-task-watcher-standby-only.test.pytests/health-check-task-watcher.test.pytests/health-check-telegram-poll-conflict.test.pytests/health-check-twilio-gate.test.pytests/health-check-unread-watcher.test.pytests/health-check-user-defined-checks.test.pytests/health-check-vault-manifest.test.pytests/health-check-vendored-resolver-env.test.pytests/health-check-voice-config.test.pytests/health-check-voice-dep-detail.test.pytests/health-check-voice-guarded-fix.test.pytests/health-check-voice-pin-suppression.test.pytests/health-check-warn-suppression.test.pytests/health-check-web-client-port.test.pytests/health-check-workspace-root-tidy.test.pytests/health-check-workspace-wiring.test.pytests/health-slack-remedy-tofu.test.pytests/health-sutando-app-warn-names-watchdog.test.pytests/health-task-queue-inflight.test.pytests/heartbeat-freshness-bounds.test.pytests/lint-hermetic-bridge-tests.test.pytests/lint-resolver-stub-arity.test.pytests/live-tree-drift-reference.test.pytests/local-core-freshness-bounds.test.pytests/local-task-protocol-write.test.pytests/memory-index-integrity.test.pytests/morning-briefing-src-dir-wiring.test.pytests/node-runtime-resolve.test.pytests/onboarding-status-check.test.pytests/pending-questions-stamp-under-state.test.pytests/pool-suspension-shared-owner.test.pytests/proactive-destination.test.pytests/proactive-loop-memory-index-budget.test.pytests/process-pins.test.pytests/punctuality-dead-job-not-green.test.pytests/punctuality-stale-lane-does-not-shadow.test.pytests/recovery-issues-http.test.pytests/review-checks-py.test.pytests/runtime-api-agents-view.test.pytests/runtime-api-dispatcher.test.pytests/runtime-api-human-action.test.pytests/runtime-api-identity-view.test.pytests/runtime-api-runtime-view.test.pytests/runtime-api-server.test.pytests/runtime-api-task-binding-cli.test.pytests/runtime-api-tasks-view.test.pytests/runtime-health-ag2space-station.test.pytests/runtime-health-atomic-writes.test.pytests/runtime-health.test.pytests/service-sources-covers-launchd.test.pytests/session-cron-config-drift.test.pytests/session-runtime.test.pytests/skill-hooks-discovery.test.pytests/skill-symlink-ephemeral-target.test.pytests/slack-bridge-heartbeat-wedge.test.pytests/slack-bridge-write-task.test.pytests/slack-context-first-ungated.test.pytests/start-cli-model-pin.test.pytests/startup-ceremony-gate.test.pytests/sutando-config-python-resolution.test.pytests/task-progress-skill.test.pytests/task-progress-step.test.pytests/task-workstreams.test.pytests/vault-intercept.test.pytests/voice-agent-kill-path-gate.test.pytests/watcher-identity.test.pytests/watcher-sentinel-naming.test.pyAt e69284d: one allowlist for every test that reaches credential discovery (reviews 5563848156, 5563980203)
Blocker 1, and the class behind it.
tests/_helpers/hermetic_gateway.pyoverlaid its values onto the outer environment, soREMOTE_TASK_TOKEN_FILEandREMOTE_TASK_CHANNEL_DIRreached the real bridge import. That was the second instance of one pattern: a test that imports or runs the bridge, Hatch, room_ops' gateway client or the credential owner, and builds its environment by overlaying onto the outer one, copying it, or copying it minus a denylist. One commit removes the pattern repo-wide.Shared helper:
tests/_helpers/hermetic_env.py. It is the only place such an environment is built.hermetic_env(home=None, **extra)starts from nothing. It carries PATH, LANG, TMPDIR and the suite runner's coverage controls, plus the process essentials (SYSTEMROOT and similar) on Windows only. It adds a temp HOME, a fixedUSER=hermetic-test, and then the synthetic values.isolate_os_environ(**extra)swaps this process's environment for that one throughpatch.dict(os.environ, …, clear=True).isolated_child_env(**extra)gives an already isolated process's own environment to a child. It refuses to run before isolation.Converted: 145 files.
hermetic_gateway(isolate, then import) andgateway_relay(its bridge children gethermetic_env).tests/,packages/ag2-sparrow/tests,src/remote-gateway-bridge.test.pyandskills/agent-room-ops/test_room_ops.py. Each isolates at module level before any of those modules runs.dict(os.environ…),{**os.environ…}and denylist comprehension is nowhermetic_env(...). This includes the three sites in 5563980203:test_wheel_contents.pyused a host denylist;gateway-outage-recovery(:618) copied the host environment;gateway-relay-result-no-ceiling(:1240) copied the host environment.setUpsnapshot/restore pairs becamepatch.dict(clear=True). Hatch'spy_env()sets a fixed USER.gateway-archive-the-sent-generation's checker self-test now names the host home ashermetic_gateway.HOST_HOME, sincePath.home()is the temp one.Structural check:
tests/hermetic-env-structural.test.py.tests/,packages/*/tests,src/*.test.pyandskills/**/test_*.py. The helper and the audit instrument are exempt.os.environ.copy(),dict(os.environ…),{**os.environ…},f(**os.environ), or a dict comprehension overos.environ;env=Noneorenv=os.environpassed to a subprocess;envparameter that defaults to None and is handed to a spawn without being rebuilt;envcomes from the helper counts as covered, and so does code after an isolation call in the same function.Regression:
tests/hermetic-env-sentinels.test.py.tests/_helpers/sentinel_audit.py).REMOTE_TASK_TOKEN_FILE,AG2_DEVICE_ENV,REMOTE_TASK_CHANNEL_DIR, and aCLAUDE_CONFIG_DIRholding channel.envfiles;REMOTE_TASK_TOKEN,AG2_REMOTE_TOKEN,REMOTE_MEDIA_HS_TOKEN,GATEWAY_TOKEN,RELAY_TOKEN,MATRIX_AS_TOKEN, the four URL keys,GATEWAY_INSTANCE,AGENT_MXID,AGENT_ID,SUTANDO_INSTANCE_ID,SUTANDO_WORKER_ID,SUTANDO_WORKER_SEATandSPARROW_HA_OWNER.hermetic_gateway.py.envare opened (path-only run); 15 of the 21 sentinels (the two path selectors it kept and 13 values) are present at the bridge importgateway-voice-outcome-contract)hermetic_gateway.py.env302 times and handed both path selectors to 54 bridge childrentest_hatch_adapter.pygateway-credential-sources.test.py.envare opened; all 21 sentinels in child envsagent-room-ops-context-rename,room-ops-say-extra-content,turn-ledger-stop-gate, each aloneReverting only
gateway_relay.pydoes not fail the relay case, because its parent test has already isolated the environment it copies.room_ops_gateway_channel_envandroom_ops_gateway_vaultset every selector themselves, so they are not the room_ops cases.Measured across the class. Every executing in-scope test file was run under the same audit, with the path selectors only. At 28130f9, 104 of 141 opened a sentinel file or handed one to a child. At this head, 0 of 141 do. With the value and identity sentinels added, 139 of 141 are clean. In
sci-notify-reviewersandtest_signal_exit_logthe marked values stay in the test process's own environment, though every child they start getshermetic_envand neither imports gateway code in-process. Both are in the follow-up below.Counts.
env -i, with a temp HOME and USER set. That covers the 169 touched or candidate test files.agent-room-ops-read-redactionandimport-claude-context-digest-parts(norequestsmodule for detect-secrets);health-check-fix-down-bridgescase f (a live slack-bridge on the host).sparrowd-c0overflows the AF_UNIX path limit under the long scratch TMPDIR. With a short TMPDIR it passes at both heads.gateway-credential-sources120test_hatch_adapterandag2-sparrow-hatch-adapter195 each, 2 skippedcredential-contract-golden37/37room_ops_gateway_vault9,room_ops_gateway_channel_env16,gateway-proactive-room-from-channel-env11remote-gateway-bridgeall greenhatch-reply-real-drain6,test_vault_token_tier7,test_env_token_quotes15,test_gateway_statusALL PASStest_adapter_boundaries8,test_room_ops173,gateway-outage-recovery76test_wheel_contents4 PASS lineshermetic-env-structural7,hermetic-env-sentinels5test_no_drift,gen-src-map --check,scripts/review-checks.sh(PR diff piped in),git diff --check,ruff check .andlint-hermetic-bridge-testspass.CI at e69284d. 33 pass and 4 skip.
python standalone tests (5/9)fails ontests/ag2-sparrow-drift.test.pyalone, and thetsc + testsaggregate fails because of it.python -m coverage. coverage is installed in the runner's user site-packages, which the temp HOME hides, so the child fails withNo module named coverage. I reproduced this locally at e69284d.hermetic_envkeeps the host'sPYTHONUSERBASE(an interpreter package path, not a credential selector). With it, the drift test passes under the coverage child.tests/_helperslibraries, so a test file must isolate its own process.Blocker 2, the §15 exact-head witness, is owner-gated and still pending.
At d485bce: every candidate name, a package-wide guard, and a failed install that says so (reviews 5563658019, 5563643767)
One commit fixes 5563658019's two blockers and 5563643767's N1 and N2.
Closed-world rules, each with its single enforcement point (current)
Credential names:
credential_name()is the only name verdict._name_positions()hands it every candidate span. Before each=or:that does not start://: every suffix of the text back to the start of the line, and the quoted key (any characters, escaped quotes included). It judges the whole span, whose folded text holds every shorter suffix's, plus each suffix that starts where apw/patword can (the only way a suffix judges differently; pinned against a brute-force check of every suffix). For each flag: the run to a blank or=, and the shell word with its quotes removed, also when the quote opens before the dashes. If any candidate is accepted, the line is withheld. The regexes only locate positions._HEADER_RE(anAuthorization:orbearervalue) and_ENVIRONMENT_DOUBT_RE(environment directives) also decide, and both only add withholding.Unit state:
_unit_props()is the only parser. This is unchanged from 7706cb0.Guard:
_systemctl()is the only place systemctl is named. The rule runs over every package module,gateway_credentials.pyandgateway_credential_sources.pyincluded. Exactly one_systemctland one_systemd_runexist in the package, each a top-level def inadapters/hatch/cli.py; only that_systemctlmay hold the"systemctl"literal. The guard flags:from .cli import _systemctl as run,_systemd_run as run);run = _systemctl,run = cli._systemctl);getattr(_cli, "_systemctl"),"_system" + "ctl");_systemctlwith one literal tuple,_systemd_runon a literal program.No dataflow is followed.
Findings map (5563658019, 5563643767)
pa ss: synth-only-valueandrun --"pa ss" synth-only-valueprinted through the real_difftest_every_key_position_is_judged_whole_by_credential_name(3 copies; unquoted assignment, colon and flag forms for every separator, space included);test_the_real_hatch_diff_judges_every_complete_key(real_diff, empty registry injected; the two exact repro lines plus"--pa ss"and--pa" "sspinned to<text withheld…>);test_a_name_position_judges_every_suffix_of_its_line(3 copies)_systemd_findings()test_the_systemd_rule_covers_every_package_module: shipped package[]; a syntheticsubprocess.run(["systemctl", *rest])appended togateway_credentials.py,gateway_credential_sources.py,remote_gateway_bridge.pyorkeepalive.pyin a copied package is flagged in that modulefrom .cli import _systemctl as run/_systemd_run as rununflagged; any_systemctldef incli.pyexempttest_every_non_literal_systemctl_argument_is_flagged(11 new repros); the package test (a second_systemctlinkeepalive.py, a_systemctlmethod incli.py)HELPERGuard.appended(); the helper alone incli.pyis pinned to[]showleft it stopped silentlyerror: service: this install stopped ag2-sparrow.service and did not start it again, …; it stays stopped until you run: systemctl --user start ag2-sparrow.service, skips the Sparrow check and exits 1test_install_that_stopped_the_service_and_then_lost_its_state_fails_with_the_start_command(onlystopanddaemon-reloadreach the manager; rc ≠ 0; the line and command on stderr)credential_shapesdocstringKnown limit (unit state). If install has already stopped the unit and the second state query then comes back malformed, install does not restart it on an unverified state. It prints
error: service: this install stopped ag2-sparrow.service and did not start it again …, followed by the commandsystemctl --user start ag2-sparrow.service, skips the Sparrow check and exits 1.Stated limits.
run --pa ss v) is two shell words, so--pais judged alone and prints. A bare token that no source holds and no credential name precedes still prints.getattr(_cli, a + b)) is outside what an AST guard sees.Cost of rule 1. Over-withholding is accepted. In the shipped hook script, the prose value of
REMAINING_WORK_NOTE=is now<withheld>; the same 2 of 176 lines are withheld whole as at 7706cb0. A 6,192-character line with 400=masks in 0.08 s.At 28130f9
Review 5563772408 blocker 1: the real-sink harnesses no longer copy the host environment minus a denylist.
RealSinks.hatch_diff(),install_service()and the detached-install case build theirs withhermetic_env()(PATH, LANG, TMPDIR, a temp HOME, USER, plus the synthetic values each sets);test_hatch_adapter.py'spy_env()is an allowlist too, and the three in-process cases that merged into the host environment now clear it. The bridge child already ran on an explicit environment.test_no_harness_reads_a_credential_path_the_outer_environment_names. SentinelREMOTE_TASK_TOKEN_FILE,AG2_DEVICE_ENVandCLAUDE_CONFIG_DIR+REMOTE_TASK_CHANNEL_DIRfiles in the outer environment; reads are recorded throughgateway_credential_io.read_file. Fails on revert ofinstall_service()(all three sentinel paths read).gateway-credential-sources120 (119);test_hatch_adapter195, 2 skipped (unchanged). Both sync checks,scripts/review-checks.sh(PR diff piped in) andgit diff --checkpass. Each file run on its own with python3 underenv -i, a temp HOME and USER set.Pins and counts at d485bce
k=v,k: v,--k=v,--"k" vand'--k' v.--k vis added for every separator except ASCII space. That is 5,930 lines per copy, 17,790 in all (4,215 per copy at 7706cb0). Real_diffsink: 4,605 (665 at 7706cb0).credential_nameaccepts must be withheld.[], and 6 package-wide traversal cases.gateway-credential-sources: 119 (115).test_hatch_adapter/ag2-sparrow-hatch-adapter: 195, 2 skipped (194).credential-contract-golden37/37,room_ops_gateway_vault9,room_ops_gateway_channel_env16,gateway-proactive-room-from-channel-env11.remote-gateway-bridgePASS (326 checks each).hatch-reply-real-drain6,test_vault_token_tier7,test_env_token_quotes15,test_gateway_statusALL PASS, both adapter-boundary suites 8 each.gen-src-map --check,scripts/review-checks.sh(PR diff piped in) andgit diff --checkpass.Revert checks at d485bce
_difflines, and the every-suffix oracle in each copy_diff, and the oracle (3 copies)_diff_systemd_findings)gateway_credentials.pyandgateway_credential_sources.pycasestest_install_that_stopped_the_service_and_then_lost_its_state_fails_with_the_start_command: install exits 0 with no lineAt 7706cb0: one enforcement point per rule (review 5563400596)
Each 5563400596 blocker was a narrower pre-filter in front of the rule's real decision. This commit removes the pre-filter, so each rule has exactly one place that decides, and pins that place.
Closed-world rules, each with its single enforcement point
(Refined at d485bce above: rule 1 judges every suffix and the dequoted flag word; rule 3 runs package-wide and pins one helper.)
credential_name()is the only name verdict._names_and_values()hands it the complete key before every=or:that does not start://(inside its quotes with any characters, escaped quotes included, and as the unquoted run back to a blank or quote), and every flag name (to a blank or=, and to,or a quote); the regexes only locate positions, and any accepted name withholds its line._unit_props()is the only parser. It returns fully parsed values (ActiveStatea documented state;MainPID≤ 10 ASCII digits and ≤ 4,194,304, Linux PID_MAX_LIMIT;ExecMainStartTimestampMonotonic≤ 20 ASCII digits and ≤ 2^64−1, as ints), and any exception or out-of-range value isNone; onNone,install_service,ensure_sparrowandrestart_listenerstop, write, reload, enable, reset, restart, start and signal nothing._systemctl()is the only place systemctl is named. Every systemctl run (and the PATH lookup, vialocate=True) goes through it with one literal tuple of literal strings; the guard flags any"systemctl"program literal outside that helper's definition, any use of_systemctlor_systemd_runthat is not a direct call with a literal argv, andshow-environmentin any string literal outsidesystemd_user_available. No constant is evaluated and no dataflow is followed.Findings map (5563400596)
{"pa/ss":…}(andpa+ss,pa:ss) printed through the real Hatch_diffcredential_nametest_every_key_position_is_judged_whole_by_credential_name(3 copies),test_the_real_hatch_diff_judges_every_complete_key(real_diff, empty registry injected; the exact{"pa/ss":"synth-only-value"}line is pinned to<text withheld…>)ORACLE_STEMSandORACLE_SEPARATORSapikeyis an equivalent mutant, sincekeyalready covers it)MainPID→show → stop → daemon-reload → show→ValueError_unit_props; unknown does nothingtest_a_number_its_consumer_cannot_take_stops_reloads_and_starts_nothing(full install: onlyshow-environmentand oneshowreach the manager, no unit is written, no traceback);test_unit_props_accepts_only_a_positively_verified_state(oversized and edge table)show, install still ranenabletest_install_neither_stops_nor_starts_the_service_when_the_unit_state_query_fails(noenable,daemon-reload,stop,start,restart,reset-failed)["systemctl"] + rest, stagedcmd += rest, starred, and the four earlier verb forms appended"systemctl"literal outside_systemctltest_every_non_literal_systemctl_argument_is_flaggedSide-effect audit.
install_servicecalls_unit_props()before the stop and before the unit write;ensure_sparrowbefore_stopandrestart_listener;restart_listenerbeforereset-failed/restart. Each consumer now reads only the parsed ints (main_pid = props["MainPID"],holder != props["MainPID"], timestamp!= 0), so no conversion is left after a side effect.Behaviour change. With the user manager's state unknown, install no longer writes the unit, reloads or enables it; it prints
…did not give a well-formed state, so nothing was stopped, written, reloaded or enabled; run ag2-sparrow-hatch install again once the user manager answers. When no unit existed yet, the keepalive then treats the host as having no service (the same path as a host withoutsystemctl --user).Stated limits (unchanged in kind). Rule 1: a bare token that no source holds and no credential name precedes still prints. Rule 3: a program name never spelled as a
"systemctl"literal (assembled at runtime, from the environment, as bytes, or inside a shell string) is outside what an AST guard sees; the package has noshell=Truecall. Prose naming systemctl (note(f"run systemctl --user …")) is not argv and is not flagged.Pins and counts at 7706cb0
/ . - space + : _ \" • · → U+3000) × every split position = 660 keys + 5 controls = 665. Contract: 3 quoted shapes for every key, plus 4 unquoted shapes (k=v,k: v,--k v,--k=v) for non-blank separators = 4,215 subtests per copy, 12,645 in all. Real_diffsink: 665._unit_props; the 5,000-digit repro + the 7 oversized as full installs (run inServiceandInstallOrderTest).cli.py.gateway-credential-sources: 115 (111 at 251781e).test_hatch_adapter/ag2-sparrow-hatch-adapter: 194, 2 skipped (192).credential-contract-golden37/37,room_ops_gateway_vault9,room_ops_gateway_channel_env16,gateway-proactive-room-from-channel-env11, bothremote-gateway-bridgePASS (326 checks each),hatch-reply-real-drain6,test_vault_token_tier7,test_env_token_quotes15,test_gateway_statusALL PASS, both adapter-boundary suites 8 each. Both sync checks,gen-src-map --check,scripts/review-checks.sh(PR diff piped in) andgit diff --checkpass. Run one at a time with python3, a cleared environment, a temp HOME and USER set.Revert checks at 7706cb0
_diffkeyssig: 387,cred: 579,session: 1,161, …);apikeyis equivalent (keycovers it)cli.py+keepalive.py)test_unit_props_accepts_only_a_positively_verified_state, and the no-enable assertiontest_every_non_literal_systemctl_argument_is_flaggedand the shipped-package guard; the old detector misses 134 of the 211 bypass sources, including 19 of 20 staged/concatenated/starred literal-"systemctl"formsAt 251781e: closed-world rules (review 5563208499)
Each review round found an adjacent case of an enumerated rule. This commit replaces each enumerated rule with a closed-world rule that fails safe by construction, so reviewers can judge the rule rather than a list of cases.
Closed-world rules
(Refined at 7706cb0 above: each rule now names its single enforcement point.)
pass pwd secret token key auth cred bearer jwt session cookie priv sig otp apikey, or has or ends in apworpatsegment; any such name in a name position (a flag, or a name followed by=or a:that does not start://) withholds its line whatever its value or length.ActiveState,MainPIDandExecMainStartTimestampMonotoniceach exactly once, a documentedActiveState, ASCII-decimal numbers), and with the manager's state unknown it reports success only for a lock holder proven byis_sparrow_pidto run Sparrow; anything malformed, missing, unexpected or unproven does nothing and says why.show-environment: any string literal (f-string,%and.formattemplate parts included) holding it outsidesystemd_user_available, and any systemctl invocation (a list holding the literal"systemctl", any_systemd_runcall, or"systemctl"anywhere butshutil.which) whose arguments are not a plain literal list of literal strings, is flagged; the probe must pass a literal list straight to_ok.This reverses an earlier loosening. At f42fe3f the name rule became whole-word so that
--author,AUTH_MODE,--tokenizer-style names printed. Rule 1 withholds--author,AUTH_MODE,Author:,OAuthClientIdandoauth-client-id,COMPASS,--bypass-proxy,KillSignalandBearer of …again: over-withholding is the accepted price.patis not in the requested stem list; it stays a credential (as before) under the segment-or-suffix rule, because as a substring it would withhold everypath. The 8-character minimum and the path exclusion now apply only to registering a value for reuse on other lines, never to withholding the shaped line itself (the old 8-character gate onBearervalues is gone).(Superseded at 7706cb0 by the
_systemctlhelper.) To satisfy rule 3, the shipped systemctl calls incli.pyandkeepalive.pypass literal lists ("ag2-sparrow.service"written out; a guard test pins every such literal equal toSERVICE_NAME),_unit_props()always queries the same three properties, andenable/enable --noware two literal calls.Stated limit (unchanged in kind). A bare token that no source holds, with no credential name in a name position on its line and no recurrence on a shaped line, still prints (
Description=Sparrow <tok>). For rule 3: a systemctl command assembled at runtime from non-literal pieces that never spell the literal"systemctl"(for example a shell string or a program name from the environment), or one passed as bytes, is outside what an AST guard can see.Findings map (5563208499)
DB_PASSCODEprinted (regression from 6f8de04)test_adjacent_shapes_from_review_are_withheld,test_the_hatch_install_diff_withholds_the_adjacent_review_shapes(real Hatch_diff)args: [--token,…],args: [--access-key=…],separates a flag value){"auth.token":…},access.token: …COMPASS/--bypass-proxywithheld,OAuthClientIdinconsistenttest_a_credential_name_holds_a_stem_after_foldingActiveState=not-a-systemd-state/bogus/activeactedtest_a_malformed_unit_answer_stops_signals_and_starts_nothing,test_unit_props_accepts_only_a_positively_verified_stateMainPID=²raised after the stop;MainPID=٠read as 0is_sparrow_pidbefore success)test_an_unproven_fresh_lock_holder_is_no_success_while_the_manager_is_silentrelay.envunit_state_unknown; revert row below shows the SIGTERMrestart_listenerreset/restarted without a verified statetest_keepawake_restarts_nothing_on_a_malformed_unit_answer.format, mapping%, tuple-concatjointest_every_non_literal_systemctl_argument_is_flagged(19 spellings × 5 placements, plus aliases, starred, concatenated, shell and message strings)Pins and counts at 251781e
_,-,.,__) × 4 casings × 13 shapes (KEY=,export,--f v,--f=v,-f v,[--f,v],[--f=v], double- and single-quoted JSON, YAML, YAML list, header,env -i) = 4,420 subtests per contract copy, 13,260 in all.=, empty answer, no stdout) × 2 install modes = 48 full-install runs; plus all 8 documented states accepted.gateway-credential-sources: 111 (102 at 3cbfb5b).test_hatch_adapter/ag2-sparrow-hatch-adapter: 192, 2 skipped (187).credential-contract-golden37/37,room_ops_gateway_vault9,room_ops_gateway_channel_env16,gateway-proactive-room-from-channel-env11, bothremote-gateway-bridgePASS,hatch-reply-real-drain6,test_vault_token_tier7,test_env_token_quotes15,test_gateway_statusALL PASS, both adapter-boundary suites 8 each. Both sync checks,gen-src-map --check,scripts/review-checks.sh(diff piped in) andgit diff --checkpass. Run one at a time with python3, a cleared environment and a temp HOME.Revert checks at 251781e
test_a_credential_name_holds_a_stem_after_folding(3 copies); both real-sink tests (DB_PASSCODEand the other 5 lines,--author)cli.py+keepalive.py)MainPID=²raisesValueErrormid-install);_unit_propstable 24 of 24 malformed accepted; unproven holder returns 0 andis_sparrow_pidis never called; keepawake resets and restarts on a malformed answertest_every_non_literal_systemctl_argument_is_flagged: 26 subtests, including all four 5563208499 formsRe-scope at dcc978a: option (a)
Both reviews at 38f4b68 (5562704937 and 5562717028) converged on option (a): credential truth lives in the process that runs (the bridge); Hatch fails closed at presentation and stops emulating systemd.
fatalare discarded)._manager_env, the analysisshow-environmentread,_service_envand the manager/installer baseline are gone.systemd_user_available()is main's reachability probe again (its output is discarded), so it no longer depends on--no-serviceor on any analysis choice._credential_discoveryandcredentials_usableare gone, withDiscovery.withheld, which only they used. Each bridge validates the environment it actually started with, inmain(), before it polls or publishes anything.At f42fe3f: the re-review of 6f8de04 (5563096096, 5563059400)
One commit fixes 5563096096's three blockers and 5563059400's three nits; nothing else changes.
_groupsand the Hatch_diffsplit at LF only (physical_lines; a CRLF stays whole), as systemd reads a unit.str.splitlinesalso broke at U+2028, U+2029, U+0085 and others, soEnvironment="REMOTE_TASK_TOKEN=qa-first<U+2028>qa-tail-9988"rendered-Environment=<withheld>qa-tail-9988". Now that line is one group and printsEnvironment=<withheld>alone. A directive is also judged in every piece any line break starts (CR, NUL and everystr.splitlinesbreak), and any other shown line holding such a break is withheld whole, so no renderer that breaks there shows an unmasked piece.systemd_wordssplits on systemd's whitespace (space, tab, CR, LF) only._unit_propsreturnsNonewhensystemctl --user showfails, cannot run, or answers without anActiveStateor with a non-numericMainPID. Every decision on it fails safe:ensure_sparrowtreatsNonelike an unanswering manager (no SIGTERM, no detached spawn, no start; it sayssystemctl --user show ag2-sparrow.service did not give its state), andinstall_servicedoes not stop the unit first, then onlyenables it (never--now, neverrestart) and says so. Audited:_unit_owner(unit file +show-environment),restart_listener,unmanaged_health_problem,keepawakeandinstall_serviceare the only deciders; none reads a missing property as inactive now.error: Sparrow is not running: <unit> is installed but systemctl --user did not answer, so whether that service runs Sparrow is unknown; install neither stops nor starts a Sparrow. Start your user manager (systemctl --user) and run install again, or remove <unit> if it is left over. When a Sparrow does hold the lock, it exits 0 and printsNOTE sparrow: … Sparrow (pid N) was left as it is._constantfolds+, implicit concatenation, f-strings,sep.join((…))/[…],.format(…)and%; the probe gate and the store check use it, so"show-" + "environment"and the other four forms 5563096096 listed are caught.--key,--access-key,--credentials,--patflags;Name: valueheaders and YAML keys whose name is a credential word (X-Api-Key:,X-Auth-Token:,token:,api_key:,password:).credential_name()decides every key, flag and header name: split on separators, case changes and digits; a word counts when it is a credential word (token,secret,password,passwd,pass,credential(s),key,apikey,auth,authorization,bearer,cookie,session,pat, …) or ends in a glued one (MYTOKEN,DBPASSWORD).--author,--tokenizer,X-AUTHOR=and--oauth-client-idno longer count;Bearerneeds a value of 8+ characters, soDescription=Bearer of tasksprints. A shaped line is still withheld; its value is registered for reuse elsewhere only at 8+ characters and when it is not a plain path (/…), so--auth=1no longer hidesRestartSec=10,--auth-mode=basicno longer hidesDescription=A basic relay, and a--token-filepath no longer hides a line that lists it.At 6f8de04: the re-review of ad3cbb1 (5562940921, 5562941475)
One commit fixes the five code blockers; nothing else from option (a) changes.
register_assignmentssplits anEnvironment=value as systemd does (quotes anywhere in a word, C escapes such as\x54and\sdecoded, an escaped key judged decoded) as well as as written, and registers each value raw, unescaped and quote-stripped. Every rendered line is matched by raw substring in four views: as written, percent-decoded, systemd-unescaped and with its quotes off. No tokenization of the shown line decides whether a registered value is masked.systemctl --userprobe fails isUNIT_UNREACHABLE, not "no unit" (nosystemctlbinary at all is still "no unit").install --no-servicedefers and prints why;ensure_sparrowandrestart_listenerneither SIGTERM the lock holder nor spawn a detached Sparrow, and say so.KEY=anywhere in the line (env -i KEY=…,sh -c 'export KEY=…', an argument), a quoted JSON credential key, a--token/--token=/--secret/--password/--api-key/--bearer/--auth…flag, anAuthorization:header or aBearervalue, also in the unescaped and unquoted views. An assignment line shows only its key (ExecStart=<withheld>); any other line prints<text withheld: it may contain a credential>. The value of each shape is registered at any length, so the same token on a neutral line elsewhere in the diff is withheld too. This covers a vault-only token without reading the vault.show-environmentunless the run's result goes straight into_ok(...)(a bool) and the scope neither reads.stdout/.stderr, prints, writes, appends nor assigns, walruses, yields or declares a global. A behavioural test feeds the probe a syntheticREMOTE_TASK_TOKEN=on stdout and stderr: it returns exactlyTrue, prints nothing, and leaves no copy on the module.dict(os.environ, AG2_DEVICE_ENV=<device_env_path()>), the value the unit and a detached Sparrow are given, so Hatch's defaultrelay.envis registered when the installer's shell does not name it. The seeded registry is built once perinstall(on its first diff) and reused, and the vault reader is resolved once and only when that tier is reached: each vault key is looked up at most once per install. No new I/O and no Keychain read is added: the vault is still reached only when no earlier tier holds a token, now at most once instead of per diff.Stated limit. A token that no source holds (retired, another account's) or that only the vault holds while the vault tier is not reached, placed bare in an arbitrary position with no credential shape on its line and not recurring on a shaped line in the same diff (
Description=Sparrow <tok>,NOTE=<tok>, free text), still prints. So does such a token after a short flag (-t,-k), under a neutral key (env X=<tok>,sh -c 'T=<tok> /x'), after a Unicode dash (—token), as a bare base64 value, in a comment line, or afterBearerwhen shorter than 8 characters. A shaped value shorter than 8 characters or a plain path is withheld on its own line but not registered for other lines. Names are matched as whole credential words, soAUTHKEY,PRIVKEY,SESSIONID,JWT,--pwand--pwdare not shapes (none is the gateway bearer), and an environment line's whole-line withholding still registers its value, soEnvironment=AUTH_MODE=1can hide an unrelatedRestartSec=10. Both are candidates for a follow-up. Closing that would need reading the vault on every install (a possible Keychain prompt), which this PR does not do. Pinned by the limit assertions intest_a_credential_shape_is_withheld_whatever_the_registry_holdsandtest_the_hatch_install_diff_withholds_every_credential_shape.Soundness limits of the name rule. A credential name with no stem in it is not caught: abbreviations (
TKN), stemless names (CERT,PEM,DSN), lookalike letters from other scripts, leetspeak, and names assembled across variables. Addingcrt/pem/dsn/tknas stems is a candidate follow-up.Unchanged from the reviewed head: the
SOURCESprecedence table, register-every-candidate, the bounded reader, option (b)'s general-config fall-through, both bridge FATAL paths, the structural guard (less the entry for the removed Hatch reader) and the sync and drift checks.Summary
The gateway credential policy now has two owners, one pure and one for precedence.
1. Pure contract:
shared/ag2_gateway_credentials.py. It keeps the existing ruling: no I/O, no module-global state. It gains the masking contract:SecretRegistry, an object its caller owns. A secret is matched in every rendering: any letter case, percent-encoding in either hex case, double encoding,reprandunicode_escapeescapes, and pieces of it. Rendering is total over anystr, including a non-UTF-8 environment byte.code_field,redact_url,redact_text,safe_status(non-finite numbers withheld) anddescribe(exception type, errno or EAI name, HTTP status, aKeyErrorkey only when the code wrote it, and the raisingfile:line; never the exception's text).mask_env_assignmentsandregister_assignments, for showing a unit, drop-in,.envor script (the Hatch install diff).Environment=<withheld>: no key, value or path. That coversEnvironment=,EnvironmentFile=,UnsetEnvironment=andPassEnvironment=, and any directive in doubt: the word in any letter case, a backslash (a hex, octal or Unicode escape systemd might decode) or a non-ASCII character. Nothing is decoded; doubt withholds.physical_lines), as systemd reads a unit; any other line break inside a line puts it in doubt (an environment directive in any piece withholds the group, and any other such line is withheld whole). Continuations are grouped at least as widely as systemd joins them: any trailing backslash continues (comment lines too), and a comment inside a continuation does not end it. A group with any environment line is withheld whole.register_assignmentsruns over both texts before anything prints: each value on an environment line, split as systemd splits it and as written (at 8+ characters), each credential-named value (any length, the key judged systemd-unescaped), and each credential shape's value (8+ characters, not a plain path); every value raw, systemd-unescaped and quote-stripped. A duplicate under a neutral key or on another line is then withheld too.credential_shapes: a credential-namedKEY=anywhere, a quoted JSON credential key, a credentialName: valueheader or YAML key, a credential flag,Authorization:or aBearervalue of 8+ characters; names judged bycredential_nameas whole words) is withheld whatever the registry holds.KEY=valuelines keep credential-name masking (credential_name: closed-world rule 1 since 251781e; before it, a whole word such asTOKEN,SECRET,PASSWORD,PASSWD,PASS,CREDENTIAL,KEY,AUTH,BEARER,COOKIE,SESSIONorPAT, or a word ending in a glued one such asMYTOKEN), onboarding strings and credential-bearing URLs.tools/sync_gateway_credentials.pyinto the package and the room-ops skill, as before.2. Discovery and precedence owner:
src/gateway_credential_sources.py. It is vendored into ag2-sparrow bytools/sync_from_src.py.SOURCESstates the precedence once, anddiscover()walks it: the table is the code path, not a description of one. Reordering a tier list reorders the winner (tested).REMOTE_TASK_TOKEN, thenAG2_REMOTE_TOKEN) → file (AG2_DEVICE_ENV, then$CLAUDE_CONFIG_DIR/channels/<dir>/.env) → vaultREMOTE_TASK_TOKEN_FILE→ the file that supplied the tokendiscover()takes the environment, the.envfiles and token file the caller read, and a vault read the caller injects. It registers every materialized candidate before it picks the winners, so a sink can withhold a credential that precedence discarded. Each value is registered whole as well as by its parts, so a URL-shaped opaque bearer is a secret too.It opens nothing itself. The AG2-specific file, env and vault I/O stays at the adapter edge.
channel_dir(env)andenv_files(env, read_text)are the one answer to which channel.envfiles are read and in what order, includingREMOTE_TASK_CHANNEL_DIR. The caller injects the reader.3. Delegation.
AG2_DEVICE_ENVset to Hatch's device file) from the owner's register-every-candidate path (gateway_credential_io.register_candidates: the environment, the device and channel.envfiles, the token file, and the vault when no earlier tier holds a token, read through the same readers the bridge hands the owner). That is for masking only: the winners, notes andfatalare dropped, and a refused file yields no values. It then registers the values assigned in the two texts (register_assignments) and renders throughmask_env_assignments.packages/ag2-sparrow/ag2_sparrow/gateway_credential_io.pyholds the reads the adapters hand the owner (the bounded file reader, the monorepo walk and the vault lookup). The bridge's_vault_lookupand_monorepo_srcdelegate to it, so there is one implementation. A Sparrow that Hatch starts detached (no unit, or a service setup that failed) validates its own environment at startup, like any other bridge.What this fixes
The finding numbers are from the #5665 review at 11e0504.
wake-contextgets the owner's fix when it rebases onto it.Not in this PR
/v1/agents.owneradmission) and finding 5 (theteam_result_guard unavailable: {exc}branch). Both are pre-existing on main.src/health-check.py:4091,src/runtime-health.py:236andsrc/runtime-api/dispatcher.py:54read gateway credential keys directly. They move onto the owner in a linked follow-up._redact_urland its log and status sinks are not yet routed through the contract here. feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665 does that when it removes its private copies. This PR claims only these: discovery, precedence and startup validation for the bridge, fail-closed presentation for the Hatch install diff, and the pure functions.Behaviour changes
Precedence and value parsing are unchanged for every launcher: a file value is unquoted exactly once, as on main, so a padded or quoted-blank token resolves the same (tested). Fail-closed covers the credential (token/URL) tiers only; general config keeps main's behaviour.
Bridge (the one place credentials are decided):
main(), before it polls or publishes anything. Messages carry source names, paths and reasons, never values.REMOTE_TASK_TOKEN_FILEis now read once at import, so its token and URL are registered as secrets before anything is shown. On main it was first read at rotation..envis skipped with that warning.AG2_DEVICE_ENVis FATAL:main()exits withFATAL:(even when an env token is set), and no later file or the vault stands in for the token. General config such asREMOTE_PROACTIVE_ROOMkeeps falling through to the later channel.env, as on main. A missing file is still skipped silently.REMOTE_TASK_TOKEN_FILEis FATAL at startup (path and reason, no content), since it would silently disable rotation. If it becomes unreadable later, each rotation reload names the refusal on stderr and does not rotate. On main a non-UTF-8 token file raised from the rotation read.exportlines, losing aliases in the token file); the last value still wins.Hatch install (presentation only):
Environment=<withheld>. On main those lines printed raw. The cost: the diff no longer shows which environment line changed, only that one did.ExecStart=<withheld>).ExecStart=argument, a hook script'sAuthorization: Bearerheader) is withheld when discovery reaches it from the environment, Hatch's own device file (named or default), the channel file, the token file or the vault. At dcc978a such a token printed in full, because only values assigned inside the two texts were registered.install --no-serviceon a host where the user service is installed: as on main, the service stays the one Sparrow and no detached Sparrow is started beside it. Install now also printsskipped: the ag2-sparrow user service (--no-service); <unit> is already installed, so that service runs Sparrow and its environment is the live identity; no detached Sparrow is started beside it.install --no-servicewhere the unit is installed butsystemctl --userdoes not answer: install printsskipped: … <unit> is installed but systemctl --user did not answer, so whether that service runs Sparrow is unknown; no Sparrow is stopped and no detached one is started beside it, and the Sparrow step says it neither stops nor starts one. If a Sparrow holds the lock, that is aNOTE sparrow:naming its pid and install exits 0; if none runs (a leftover unit with no user manager), it iserror: Sparrow is not running: …and install exits 1.keepawakein that state does not spawn a detached Sparrow either; it reports the restart as failed with the reason.install(with or without--no-service) where the manager answersshow-environmentbut the followingsystemctl --user showfails or gives no usable state: the same as above (no SIGTERM, no spawn, no start or restart; the reason namessystemctl --user show ag2-sparrow.service did not give its state). With a service install, the unit is written andenabled but not stopped first and not started or restarted.install --no-servicewith no unit, or a service setup that failed: as on main, Sparrow starts detached from the installer's environment, and that bridge's own startup check decides (a refused named file is FATAL there).Files to read first
src/gateway_credential_sources.py, the masking section ofshared/ag2_gateway_credentials.py(sets_environment,register_assignments,mask_env_assignments),packages/ag2-sparrow/ag2_sparrow/gateway_credential_io.py, then the credential block ofremote_gateway_bridge.py(the_FOUND = discover(...)block,main()'s FATAL check) and_diff/installinadapters/hatch/cli.py.Docs
docs/src-map.mdis regenerated for the newsrc/module. The D1 identity census row for the ag2space credentials owner is re-anchored to the owner'sdiscover()(scripts/census-d1.py, and the regenerateddocs/census/d1-identity-census.md). No other docs change: precedence and behaviour are unchanged for every launcher.Before / after evidence
On
main,ag2-sparrow-hatch installover an existing unit holdingEnvironment="SAFE=x" "REMOTE_TASK_TOKEN=<token>"printed the token in its diff. At this head that line printsEnvironment=<withheld>, as does\x45nvironment=NOTE=<token>,REMOTE_TASK_\x54OKEN=<token>, anEnvironmentFile=path, and a neutral-key duplicate of the token elsewhere in the unit.Test plan
tests/gateway-credential-sources.test.py: 102 tests (88 at 6f8de04, 78 at ad3cbb1, 76 at dcc978a). Hermetic: temp HOME and files, a faked vault, fakesystemctlrunners, no network. Contract cases run against every copy (the package copy,shared/and the skill copy); owner cases against both owner copies.code_field, URL parts,redact_text,safe_status,describe, purity by AST; and the presentation rules:test_every_environment_line_is_withheld_whole,test_an_escaped_or_doubtful_directive_is_withheld,test_a_continued_environment_line_is_withheld_with_every_line_it_joins,test_a_value_on_an_environment_line_is_withheld_wherever_it_recurs,test_edge_paths_never_raise(malformed escapes included).SOURCESchanges the winner; refused named files are fatal and named; config still reaches a later file.test_every_assignment_of_a_repeated_key_is_registered_by_the_bridge).test_a_token_only_outside_an_assignment_is_masked_from_the_env_or_the_device_fileandtest_a_token_only_outside_an_assignment_is_masked_from_the_token_file_or_the_vault(a fake vault reader; a unit'sExecStart=and a hook script'sAuthorizationheader, each holding the token outside any assignment),test_the_hatch_install_diff_withholds_every_candidate_of_every_row(each candidate now only inExecStartPre=arguments, no longer also on anEnvironment=line; the vault candidate is checked in the row where the vault tier is reached, as in the bridge),..._masks_past_a_comment_in_a_continuation,..._follows_systemd_continuations,..._withholds_escaped_directive_spellings,test_an_owned_or_retained_unit_prints_no_environment_content,test_a_drop_in_is_never_read_and_its_lines_would_be_withheld, andtest_after_a_failed_service_install_the_detached_bridge_checks_its_own_environment(two routes: a service setup that failed, and--no-servicewith no unit; the environment Hatch hands the detached Sparrow is fed to the real bridge, which exits FATAL on the refused token file, while Hatch printed no refusal).test_a_systemd_value_is_registered_whole_whatever_its_quotes_or_escapes(5562940921's exactREMOTE_TASK_TOKEN='abcdefgh'+--token='abcdefgh'pair, then'abcdefgh',REMOTE_TASK_\x54OKEN=abcandalpha\somegaeach recurring on neutral, quoted and escaped lines) andtest_a_credential_shape_is_withheld_whatever_the_registry_holds(5562941475's rows plus--secret,--password=,--api-key,--bearer,Bearer\s…, a JSON key and--auth-token, with and without a registry; a shaped value recurring underenv X=,Description=,NOTE=and a JSON line; the bare-token limit).test_the_hatch_install_diff_registers_a_systemd_value_whole(the same repros through_install_file→_diff),test_the_hatch_install_diff_withholds_every_credential_shape(5562941475's sixExecStart*/curlforms with an empty vault, the recurrence rows, and the limit),test_the_hatch_install_diff_seeds_from_the_default_device_file(AG2_DEVICE_ENVunset, the token only in$HOME/.config/ag2-sparrow/relay.env:ExecStart=/x --flag <live>andNOTE=<live>withheld, and the vault fake records no lookup), andGuard.test_the_reachability_probe_discards_what_show_environment_prints(the shipped form passes; eight variants that return the run, return.stdout, print, stash in a global, append, walrus, write to stderr or yield are flagged).test_only_an_lf_ends_a_line_and_no_other_break_carries_a_tail(U+2028, U+2029, U+0085, VT, FF, FS, GS, RS, CR and NUL:physical_lines, 5563096096'sEnvironment=repro, a directive after the break, a shown line and an assignment holding one),test_flag_header_and_yaml_credential_shapes(N1's flags, headers and YAML keys),test_a_credential_name_is_a_whole_word(MYTOKEN,DBPASSWORD,apiKeycount;AUTHOR,tokenizer,oauth-client-id,keyboard,patchdo not;--author,--tokenizer,X-AUTHOR=,--oauth-client-id,Bearer of tasksandAuthor:lines print unchanged) andtest_a_short_or_path_shaped_value_hides_no_other_line(--auth=1next toRestartSec=10andDescription=Sparrow 1.0,--auth-mode=basic, a--token-filepath; an 8+ value is still reused).test_the_hatch_install_diff_shows_no_tail_past_a_unicode_line_break(5563096096's repro for U+2028, U+2029 and U+0085 through_install_file→_diff: the body is exactly[Service],-Environment=<withheld>,-ExecStart=/x,+ExecStart=/y) andtest_the_hatch_install_diff_keeps_harmless_lines_and_names(--auth=1withheld,RestartSec=10and--author <name>shown, anX-Api-Key:header withheld).Guard.test_the_reachability_probe_discards_what_show_environment_printsnow also builds the command eight ways (+, implicit concatenation then+,"".join(…),"-".join([…]), f-string,.format,%with one and two args): each is flagged when returned via.stdoutor stored in a local, flagged as a store read in an unlisted scope, and passes when only_ok(...)tests it.show-environmentreads; the allowlist names Hatch's reachability probe (systemd_user_available), drops the removed Hatch reader, and names the edge readers ingateway_credential_io(read_file,vault_lookup,_vault, and_vault_once, the per-install memo of_vault). Hatch itself hands the owner no reader and opens no credential source in its own code.packages/ag2-sparrow/tests/test_hatch_adapter.py: 187 tests, 2 skipped (184 at 6f8de04, 180 at ad3cbb1). New at f42fe3f:test_no_service_never_stops_a_service_sparrow_when_the_unit_state_query_fails(5563096096's sequence with fakes: an owned active unit, a live synthetic Sparrow holding the production-format lock without the health command,show-environmentanswering andshowfailing, not running, answering noActiveState, or an emptyMainPID: the old holder alive, no spawn, no start/restart/stop/enable/reset-failed, rc 0, the reason printed),test_install_neither_stops_nor_starts_the_service_when_the_unit_state_query_fails(the same through a full service install: no stop, start or restart, a plainenableonly) andtest_a_leftover_unit_with_no_user_manager_exits_non_zero_and_starts_nothing(with and without--no-service: rc 1,error: Sparrow is not running: …, nothing spawned). New at 6f8de04:test_no_service_never_replaces_a_service_sparrow_while_the_manager_does_not_answer(5562940921's sequence with fakes: an owned unit, a live synthetic Sparrow holding the production-format lock without the health command, reachabilityfalse,false,false,true: no SIGTERM, no detached spawn, the old holder alive, both ownership checks met the failing probe, the reason printed),test_an_unanswering_manager_gets_no_detached_sparrow_beside_its_unit(restart_listener),ReachabilityProbe.test_the_probe_discards_what_show_environment_printsandInstallMasking.test_the_vault_is_consulted_at_most_once_per_install(two diffs in oneinstall --force:_vaultresolved once, each key looked up once). Earlier, with 180 (its base case now fakes the vault reader, so no install diff reaches the host's vault), addingtest_no_service_defers_to_an_installed_running_service(fake systemd, an active managed unit whose Sparrow holds the lock: nothing stopped, nothing spawned, no start/restart/stop/enable, the live-identity line printed) andtest_no_service_with_no_unit_runs_one_detached_sparrow.Revert checks at this head. Each mutant fails the named tests:
test_an_escaped_or_doubtful_directive_is_withheld,test_every_environment_line_is_withheld_whole,test_the_hatch_install_diff_withholds_escaped_directive_spellingstest_a_continued_environment_line_...,test_the_hatch_install_diff_masks_past_a_comment_in_a_continuationtest_a_continued_environment_line_...test_a_value_on_an_environment_line_...,test_an_owned_or_retained_unit_...,test_the_hatch_install_diff_withholds_every_candidate_of_every_rowshow-environmenttest_the_detector_catches_every_bypasstest_a_token_only_outside_an_assignment_...tests (env, device file, token file, vault) and all eight rows oftest_the_hatch_install_diff_withholds_every_candidate_of_every_row_manager_envput back in Hatchtest_no_other_package_module_touches_a_credential_sourcetest_after_a_failed_service_install_...,test_no_other_package_module_...--no-servicehides systemd (the 38f4b68 behaviour)test_no_service_defers_to_an_installed_running_service(the service's Sparrow is stopped and a detached one spawned)test_no_service_defers_to_an_installed_running_servicetest_a_systemd_value_is_registered_whole_...(all three repros, in each of the 3 copies) andtest_the_hatch_install_diff_registers_a_systemd_value_whole(all three): 12 subtest failurestest_no_service_never_replaces_a_service_sparrow_while_the_manager_does_not_answer,test_an_unanswering_manager_gets_no_detached_sparrow_beside_its_unittest_a_credential_shape_is_withheld_...(every form, every copy) andtest_the_hatch_install_diff_withholds_every_credential_shape: 87 subtest failurestest_the_reachability_probe_discards_what_show_environment_prints(all eight variants)test_no_other_package_module_touches_a_credential_source,ReachabilityProbe.test_the_probe_discards_what_show_environment_printstest_the_hatch_install_diff_seeds_from_the_default_device_filetest_the_vault_is_consulted_at_most_once_per_installstr.splitlines(shared copy)test_only_an_lf_ends_a_line_...: 9 subtest failuresstr.splitlinestest_the_hatch_install_diff_shows_no_tail_past_a_unicode_line_break: all 3 characterstest_only_an_lf_ends_a_line_...: 10test_only_an_lf_ends_a_line_...: 10showreads as inactive again ({})ActiveStateor a blankMainPIDacceptedtest_no_service_never_stops_...: 2test_a_leftover_unit_with_no_user_manager_...+and f-stringstest_the_reachability_probe_discards_...Name: value/ YAML shapetest_flag_header_and_yaml_credential_shapes: 6key,pat,credential(s)wordstest_flag_header_and_yaml_credential_shapes: 6test_a_credential_name_is_a_whole_wordtest_a_short_or_path_shaped_value_hides_no_other_line: 3The owned-unit half of
test_an_owned_or_retained_unit_...pins that an owned unit is replaced without rendering any of it; it also passed before, because the owned path never printed a diff.Run at f42fe3f one at a time with python3, cleared environment and a temp HOME, all passing:
gateway-credential-sources(102),gateway-proactive-room-from-channel-env(11),credential-contract-golden(37/37),room_ops_gateway_vault(9),room_ops_gateway_channel_env(16),tests/remote-gateway-bridge.test.pyandsrc/remote-gateway-bridge.test.py(all checks green),ag2-sparrow-hatch-adapter(187, 2 skipped),hatch-reply-real-drain(6),test_env_token_quotes(15),test_vault_token_tier(7),test_gateway_status(all pass), both adapter-boundaries suites (8 each).sync_from_src --check,sync_gateway_credentials --check,gen-src-map --check,scripts/review-checks.shon the diff andgit diff --checkpass.Latest findings, and how option (a) resolves each
install --no-servicehid an installed managed service (systemd availability conflated with the analysis choice)systemd_user_available()is main's probe, independent of--no-service. With a unit installed, install defers to it, starts nothing detached and prints that the service is the live identity.test_no_service_defers_to_an_installed_running_service,test_no_service_with_no_unit_runs_one_detached_sparrowshow-environmentcredential read escaped the guardshow-environmentas a credential-store read; the only allowlisted site is the reachability probe, which discards the output.test_the_detector_catches_every_bypass(two new probes);test_no_other_package_module_touches_a_credential_sourcefails if_manager_envreturnsKEY=line whose key holds an escape.test_an_escaped_or_doubtful_directive_is_withheld,test_the_hatch_install_diff_withholds_escaped_directive_spellingsEnvironmentFile=,UnsetEnvironment=andPassEnvironment=are withheld whole; values from both texts are registered before rendering (two-pass); drop-ins are never read.test_an_owned_or_retained_unit_prints_no_environment_content,test_a_drop_in_is_never_read_and_its_lines_would_be_withheld,test_a_value_on_an_environment_line_is_withheld_wherever_it_recurs--no-service) runs the bridge's own startup check on its own environment.test_after_a_failed_service_install_the_detached_bridge_checks_its_own_environmenttest_a_systemd_value_is_registered_whole_whatever_its_quotes_or_escapes,test_the_hatch_install_diff_registers_a_systemd_value_whole--no-servicereplaced a service-owned Sparrow while the manager probe was briefly downUNIT_UNREACHABLE: no stop, no detached spawn, the reason printed.test_no_service_never_replaces_a_service_sparrow_while_the_manager_does_not_answer,test_an_unanswering_manager_gets_no_detached_sparrow_beside_its_unittest_a_credential_shape_is_withheld_whatever_the_registry_holds,test_the_hatch_install_diff_withholds_every_credential_shapetest_the_reachability_probe_discards_what_show_environment_prints(Guard),ReachabilityProbe.test_the_probe_discards_what_show_environment_printstest_the_hatch_install_diff_seeds_from_the_default_device_file,test_the_vault_is_consulted_at_most_once_per_installsplitlines(True)broke at U+2028/U+2029/U+0085, so the real_diffshowed a credential's tailtest_only_an_lf_ends_a_line_and_no_other_break_carries_a_tail,test_the_hatch_install_diff_shows_no_tail_past_a_unicode_line_breakshow-environmentSIGTERMed the service-owned Sparrow_unit_propsreturnsNonefor a failed or unusable answer; every decider treats it as unknown: no SIGTERM, spawn, start or restart, with the reason.test_no_service_never_stops_a_service_sparrow_when_the_unit_state_query_fails,test_install_neither_stops_nor_starts_the_service_when_the_unit_state_query_failsshow-environment+, join, f-string,.format,%) in the probe gate and the store check.test_the_reachability_probe_discards_what_show_environment_prints(eight built forms, flagged and discarded controls)--key/--access-key/--credentials/--pat,X-Api-Key/X-Auth-Token, YAMLtoken:printedtest_flag_header_and_yaml_credential_shapes,test_the_hatch_install_diff_keeps_harmless_lines_and_names--author,--tokenizermatchedcredential_name; shaped values registered only at 8+ and not as a plain path;Bearerneeds 8+.test_a_credential_name_is_a_whole_word,test_a_short_or_path_shaped_value_hides_no_other_line,test_the_hatch_install_diff_keeps_harmless_lines_and_nameserror: Sparrow is not running: …; exits 0 only when a Sparrow holds the lock.test_a_leftover_unit_with_no_user_manager_exits_non_zero_and_starts_nothingdiscoverimport,**kwargsreaders, allowlisted readers' insides, obfuscated keysNits N1 (which environment the diff analysed) and N3 (
$'…'parsing in_manager_env) no longer apply: there is no analysis environment and no manager parse. 5562717028 #4 (the final-head live witness) stays open below.§15 live witness plan (pending the owner; not run)
At this PR's head, restart one live bridge from the merged wheel, once per source tier, with the other tiers absent. For each, confirm from the bridge's own startup line and a delivered task:
env:
REMOTE_TASK_TOKENset in the launcher environment. The bridge names the env tier, authenticates, and delivers a test task end to end.file: the token only in
AG2_DEVICE_ENV(then again only in the channel.envunder a non-defaultREMOTE_TASK_CHANNEL_DIR). The bridge names that file and authenticates.vault (macOS): the token only in the Keychain. The bridge names the vault tier and authenticates.
token file:
REMOTE_TASK_TOKEN_FILEset. Rewrite the file to a new token; the bridge rotates without a restart and keeps authenticating.Fail-closed rows: a refused
AG2_DEVICE_ENVexitsFATAL:; a refusedREMOTE_TASK_TOKEN_FILEexitsFATAL:.Hatch VM:
install --no-serviceon a host whose managed unit already runs leaves that unit's Sparrow in charge (same PID, no detached Sparrow); install over a hand-written unit prints onlyEnvironment=<withheld>for its environment lines; a default-path install (noAG2_DEVICE_ENVexported) over a hand-written unit holding the live token inExecStartshows it withheld.Hatch VM, manager stopped:
install --no-servicewhile the user manager is stopped prints the "did not answer" line, and the service's Sparrow PID is unchanged (with no Sparrow running, install exits 1 witherror: Sparrow is not running).For each restart, record the new process id (it is a new process, not the old one) and that no startup or status output contains the token. Evidence is by function only (a test room, the owner's agent); nothing identifying is posted.
Hardcoded paths
None. Test fixtures use temp directories and placeholder paths.
🤖 Generated with Claude Code
Deferred follow-ups (non-blocking, filed as one issue at merge)
Masking residuals (stated limits in #5698)
-t,-k), a bare base64 value, or a value under a name with no credential stem (TKN,CERT,PEM,DSN, lookalike letters, names assembled across variables) still prints in the Hatch install preview. Considercrt/pem/dsn/tknas stems.--pa ss vis two shell words, so the value prints.AUTH_MODE=1can hide an unrelated line likeRestartSec=10.Guard coverage
systemctl show --property=Environment,/proc/<pid>/environ.discoverimport,**kwargsreaders, the internals of allowlisted readers, and a helper name assembled at runtime.Review 5563772408 (non-blocking)
run ""--pa" "ss value) is one--pa ssword but stays visible with an empty registry.p...p=valueline (16k characters: about 3.6 s)._systemctl, or_systemctl(...)on an arbitrary receiver.Docs
credential_name()is "the only name verdict" everywhere it is described.Review 5564171259 (non-blocking)
E = os.environ; dict(E)) is not flagged.PYTHONUSERBASEpass-through re-enables the host user site (.pth,usercustomize) in children: pass it only under coverage, or setPYTHONNOUSERSITE=1.Update at 934e6ed (review 5564617995; merges #5704 at c621a05)
RealSinks.bridge()no longer copies the coverage bootstrap privately. It builds the child env withhermetic_env(), which exposes only the coverage package.sitecustomize.pybeside it never runs in the real bridge child. In the killed control, the same plant runs once its directory is onPYTHONPATH, and restoring the removed lines fails the test.env -iwith a temp HOME and USER:gateway-credential-sources122/122, structural 13/13, sentinels 14/14 (the credential-owner case included).🤖 Generated with Claude Code