Repository navigation
fix(gateway-credentials): the owner checks, writes and removes the bridge declaration; sparrowd reads it through the owner - #5815
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/sparrow_agent_bridge.pyLines 211-219 211 homes = sorted(p for p in agents.iterdir() if p.name.endswith(SUFFIX)) if agents.is_dir() else []
212 for home in homes:
213 path = home / adapter.DECLARATION
214 if _stat(path) is None:
! 215 continue
216 try:
217 decl, why = load(path, agents)
218 except ScanUnavailable as e:
219 decl, why = None, str(e) |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 8056be46b17d30f0a006e313b48e469574e5c0dc. REQUEST_CHANGES — one verified structural-guard blocker.
_names_only() in tests/gateway-credential-sources.test.py:1387-1402 proves only that the accessor call starts in a dict assigned to one name inside a scope named wire_bridge; it never checks how that assigned record is subsequently used. This ordinary staged read therefore passes the guard:
def wire_bridge(cfg):
body = {"token_env": _sources.token_env_name()}
return os.getenv(body["token_env"])Exact-head result:
credential_access(source, "adapters/claude_code/cli.py") => []
The same false negative occurs with os.environ.get(body["token_env"]) and os.environ[body["token_env"]]. Expected is a non-empty finding: the accessor now feeds a credential reader, contrary to its “record name only, never read the variable with it” contract. The new dict test covers the inline expression but not this two-statement form.
The allowlist identity is also wider than the reviewed top-level function: nested functions, class methods, and async functions named wire_bridge in the same module all receive the exemption. Please constrain the record/field's downstream use to audited data-only sinks and bind NAME_ONLY_SITES to the unique canonical top-level function; add the staged-read case as a negative control.
This is not evidence of a current credential leak—the shipped wire_bridge still serializes the record safely, and the diff is test-only. The risk is that the claimed enforcement remains false-negative and a future edit can read the real host token while the guard stays green.
Verified on this head: gateway credential suite 127/127; hermetic sentinels 20/20; git diff --check and review-checks.sh clean. Restoring the parent detector while retaining HEAD tests makes 4 of 12 Guard tests fail, so the intended keyword/inline-dict/alias fixes do discriminate. Required CI and CLA are green. Fixing the staged-use and canonical-scope gaps would clear my blocker.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Blocking: the new audited-sink exception is still name-based and lets the credential record escape.
At exact head ddb0c8a67440c2e9c313cbf7898ca0b8e1ad68a2, line 1427 trusts any callee whose AST spelling is json.dumps; the next line also trusts any bare return body. In a complete synthetic module, wire_bridge(json) built the token_env_name() record and called a caller-supplied json.dumps(body): credential_access(...) returned [], while executing it under env -i returned the synthetic secret. The same false negative occurs with the genuine stdlib function plus a caller-supplied cls encoder. Because the serialized output and returned record are untracked, dump→load→getenv and return→caller-unwrapping probes also pass cleanly. No real credentials were used.
There is a second exact-head false negative in the same record proof: a nonconstant dict key is stored as {None} at line 1414, so {k: token_env_name()} followed by body["alias"] passes when k = "alias". The equivalent literal-key control is correctly rejected.
Please bind the allowed sink to the canonical stdlib import and constrain it to the terminal shipped use (including arguments that can replace serialization behavior), or track the resulting value/caller; remove or track the bare-return escape; and fail closed on noncanonical keys. Add kill controls for these paths. The shipped adapter is currently safe—it imports stdlib json and writes the serialization directly—so this is a future enforcement hole, not a current live leak.
I verified that all nine bypasses claimed in this update are fixed: HEAD flags all nine, while 7779e2d0 misses seven and already flags two. The gateway suite passes 132/132, the hermetic sentinels pass 20/20, the prior-guard kill control fails the new test, diff/review checks pass, and exact-head CI plus CLA are green. This remains not merge-ready until the structural guard rejects the synthetic escaping cases above.
sonichi
changed the title
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head c87f6e2e85e80ef11dcfbc182668d257035fb4b7. REQUEST_CHANGES — not merge-ready. The shipped adapter is safe and I found no current credential leak, but the claimed structural enforcement still has reproducible false negatives.
-
Blocking: the package-wide owner-key-tuple rule misses the exact string/getattr form the PR says it covers. At lines 1407-1410,
KEY_TUPLESis checked only againstname; unlikeOWNER_ONLY, it is never checked against constanttext. On this head:_sources.TOKEN_KEYS[0] -> finding getattr(_sources, "TOKEN_KEYS")[0] -> []Planting the latter in a new package helper also made the real
package_findings(...)scan return[]; against the real owner it selectsREMOTE_TASK_TOKENand reached a synthetic value underenv -i. The CLI-only word-set assertion catches this spelling only when it lives directly incli.py; it does not make the package-wide rule true. Please check tuple-name string constants too and add the directgetattrpositive control. -
Blocking: the opaque owner→adapter handoff and the general K4 claim are not enforced. The CLI pin proves only that forbidden words are absent and
bridge_declaration_textappears somewhere. This exact adapter mutant names none of those words:declaration = _sources.bridge_declaration_text(...) record = json.loads(declaration) selector = next(value for key, value in record.items() if key.endswith("_env")) os.getenv(selector) cm._write(path, declaration, 0o644)
It produced
credential_access(...) == [], selectedREMOTE_TASK_TOKEN, reached only a synthetic secret, and still passed all 17Guardtests plus all 5ConnectHostSupervisedtests. Ajson.loads→json.dumps(..., indent=1, sort_keys=False)rebuild likewise stays byte-identical and passes both layers. Thus K4 catches the old literal rebuild, not byte-identical rebuilds generally. Keep the simple no-reference rule, but add a structural delegation pin that the owner's returned text reachescm._writedirectly and unchanged, with these two kill controls. -
Blocking merge evidence is still missing/red.
- The required diff-coverage job ran normally and failed at 88%, missing canonical owner lines 46 and 48. The new behavior test calls only the vendored
owner, notOWNERS["src"]; byte-equal sync is useful but does not execute the canonical file. Drive the assertion over the canonical owner too and clear the required gate. - This now changes the bridge-install/live path, but the PR body and comments contain no real post-restart round trip. The worst case is a declaration the supervisor rejects or cannot launch, cutting task/result delivery. The harness and byte-parity proof reduce that risk but do not replace the live witness required by REVIEW lesson 15.
- The required diff-coverage job ran normally and failed at 88%, missing canonical owner lines 46 and 48. The new behavior test calls only the vendored
Verified passing on this head: gateway credentials 132/132 (base 124, all eight new tests collected), hermetic sentinels 20/20, Claude adapter 239/239 (base 238), vendored sync, git diff --check, and review-checks.sh. Exact-path exclusions and their positive control work, and the repo-wide out-of-package references are accurately disclosed.
One evidence correction is also needed: an isolated replay of K4 produced two Guard failures, not the three pasted in the PR body; test_the_exclusions_are_exactly_the_owner_modules passed. Fixing the two enforcement gaps, required coverage, live witness, and pasted K4 result would clear my blockers.
sonichi
changed the title
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 7c7145391a74754ae383dbd7b94cc2db3db133bf. REQUEST_CHANGES — not merge-ready. The prior string/getattr and 88% coverage blockers are fixed: required CI, CLA, and diff coverage now pass, and the shipped paths appear safe. Two claimed structural boundaries still have reproducible false negatives.
-
Blocking: the adapter still receives the existing declaration text, so the read/parse/environment-read class is not gone.
owned_declaration()returns the complete text, andwire_bridge()binds it before calling the owner writer. The new delegation assertion checks only the_sources.write_bridge_declarationcall/result; it never constrains that pre-read text.This exact-head mutant after the
problemcheck names no owner-private symbol or record key:if text: record = json.loads(text) selector = next(value for key, value in record.items() if key.endswith("_env")) os.getenv(selector)
It produced
credential_access(...) == [], passed all 21Guardtests and all 5ConnectHostSupervisedtests, and a controlled install→wire_bridgereplay observedREMOTE_TASK_TOKENresolving to the suite's synthetic token; no real credential was used. The disclosure that the file contains only the variable name does not mitigate this—the name is the selector for the token already present in the adapter process environment. K3 kills holding the owner'sNonereturn, but not this shipped pre-read route. Please move the ownership/foreign-declaration check behind the record owner boundary so the adapter never receives declaration bytes, pin that delegation, and add this existing-text mutant as a kill control. -
Blocking: the sparrowd reader's second owner delegation is not pinned. Current code safely calls
bridge_token_environ(), but replacing that call withtoken_env = {"REMOTE_TASK_TOKEN": token}passed all 21Guardtests and all 12sparrowd-agent-bridgestests;owner_only_findings(..., reader=True)also returned[]. The reader guard requires onlybridge_declaration_fieldsto remain, notbridge_token_environor its call shape. Pin both accessors so the direct token-selector copy cannot return while the guard stays green.
The required live post-restart round trip is also still absent, as the PR body states. REVIEW lesson 15 makes that a separate approval blocker. Worst case: a future green-guard adapter edit can select and read the host token, or reader/writer drift can prevent the bridge from launching and interrupt task/result delivery.
Verified on this head: gateway 136/136, hermetic 20/20, Claude adapter 239/239, sparrowd 12/12 + rescan 32/32, local bridge 19/19, wrapper PASS, vendored sync, git diff --check, and review-checks.sh. The coverage artifact independently resolves to 68/68 changed executable lines, including 26/26 in the canonical owner.
sonichi
changed the title
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 2c06c889e874c949cf1655fc52cbf812d437efec. REQUEST_CHANGES — not merge-ready.
The exact round-7 mutants are fixed: the adapter no longer receives declaration bytes, the reader delegates both field and token handling to the owner, all required CI and CLA checks pass, and exact changed-line coverage is 130/130. The shipped adapter and reader are currently safe. Two claimed structural boundaries still have executable false negatives.
-
Blocking: the declaration-path guard still permits ordinary local-helper refactors to read and interpret the declaration outside the owner.
declaration_reads()recognizes the path only when its assignment directly contains a marker. Moving that same expression behind a local helper and then usingpath.exists()/path.read_text()produceddeclaration_reads=[]andcredential_access=[], passed all 23Guardtests and all 5ConnectHostSupervisedtests, and a controlledenv -ireplay selected only the suite's syntheticREMOTE_TASK_TOKEN. Constant-equivalent filenames such as"worker" + ".json"also produce no finding. Separately, line 1469 exempts any receiver whose method is spelledwrite_bridge_declarationorremove_bridge_declaration; it does not establish that the receiver is_sources, so a local helper with either name can receive and read the path while the guard remains green.This is an adjacent-input failure in the enforcement itself, not a leak in the current implementation. Please bind owner-call exemptions to the exact owner receiver and close the helper/constant-equivalent path flows (or replace the heuristic with a closed-world structural rule), with kill controls for each route.
-
Blocking: the reader guard does not enforce its stated “may set no entry itself” rule. After the current whole merge in
bridge_environ(), adding:env.update(REMOTE_TASK_TOKEN=token)
yielded
token_injection_findings=[]and passed all 23Guardtests plus all 12sparrowd-agent-bridgestests.token_injection_findings()checks string nodes andSubscript(Store), but anast.keyword.argis neither. A deadif False: env.update(token_env)also satisfies the whole-merge test, while a dynamically constructed key can perform the live self-write. Please pin an effective merge into the returned environment and reject every direct mapping-entry write; add these exact forms as kill controls.
The required REVIEW §15 post-restart round trip is still absent. Since code findings remain, do not spend that witness yet: close both structural holes, rerun the exact-head gates, freeze the final head, then run and paste the live witness before re-requesting review.
Verification on this head: gateway 138/138, Claude adapter 239/239, hermetic 20/20, sparrowd 12/12 + rescan 32/32, local bridge 19/19, wrapper PASS, vendored sync, git diff --check, review-checks.sh, all non-skipped CI, CLA, and 130/130 changed executable lines. Worst case if these guards remain permissive: a future green-guard adapter edit can again select the host token, or a reader edit can bypass owner policy and disrupt bridge task/result delivery.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Request changes at 2c06c88. I concur with Kewei's review at this head; I re-checked his reader finding myself.
Blocking (false claim in the guard; independently reproduced):
- The PR body, guard point 4, says that in the reader
bridge_environ"stores no item itself". At this head I added one line afterenv.update(token_env)insrc/sparrow_agent_bridge.py:135:env.update(REMOTE_TASK_TOKEN=token). All 138 tests intests/gateway-credential-sources.test.pystill pass.token_injection_findings()inspects string nodes andSubscript(Store), and a keyword argument (ast.keyword.arg) is neither. So the guard does not enforce the property the body claims. File restored afterwards. - I did not re-run Kewei's finding 1 (helper-wrapped and constant-equivalent declaration paths, and owner-call exemptions matched by method name alone). I have no reason to doubt it, and it is the same class of gap.
Asks, matching Kewei's:
- Reject every direct mapping-entry write in the reader, whether keyword,
updatewith a dict,setdefault, or a dynamic key. Pin that the owner's environ is effectively merged into the returned env, with each form as a kill control. - Bind the owner-call exemptions to the exact owner module receiver, and close the helper and constant-equivalent path routes, or replace the heuristic with a closed-world rule. Add a kill control for each route.
- After that, freeze the head and post the REVIEW §15 post-restart round trip.
What is right: the design of removing the data flow, so the adapter gets only an opaque status and the reader goes through the owner, is the correct shape. The shipped adapter and reader are currently safe; the gap is in the enforcement.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head a5288fcb93bb90051e6474a78fac710146ed1c8f. REQUEST_CHANGES — not merge-ready.
The three routes reported at 2c06c889 are fixed: the helper/constant path cases and owner-receiver controls now discriminate, the prior keyword/setdefault/putenv/dict-merge reader writes are caught, all 27 clean Guard tests pass, and the shipped adapter/reader paths remain safe. Two false negatives still contradict this head's stated structural boundaries.
-
Blocking: an f-string is treated as a message even when it is the path passed to a file reader.
marked()skips everyJoinedStr, rather than only a path formatted into a diagnostic. This exact-head mutant returns bothdeclaration_reads=[]andcredential_access=[]:path = bridge_home(root) / DECLARATION text = open(f"{path}").read()
The adjacent controls discriminate:
path.read_text()is flagged, whileprint(f"declared: {path}")remains allowed. This is a direct declaration read outside the owner under the opaque-path rule, not an arbitrary environment-reader rewrite. Please qualify the f-string exemption by its use context and add this read plus the legitimate message as a two-way kill control. -
Blocking: the reader's claimed closed-world mapping-write rule misses mapping union update. The test says the module's only mapping writes are the four pinned operations, but the scanner only rejects
Subscriptstores and calls whose attribute is inMAPPING_WRITES. Adding this after the canonicalos.environ.update(environ):os.environ |= {next(iter(environ)): token}
produced
token_injection_findings=[]andowner_only_findings=[], passed all 27Guardtests and all 12sparrowd-agent-bridgestests, and executed throughmain()with return code 0. A synthetic-only probe observed the synthetic token under one unrelated environment binding in addition toREMOTE_TASK_TOKEN. This is an ordinary extra mapping write inside the explicit closed-world claim. Pinningmain's environment mutation structurally, or coveringAugAssign(BitOr)and__ior__, with this exact mutant as a kill control would close it.
Exact-head local verification: gateway credential sources 142/142, sparrowd bridge 12/12, git diff --check, and review-checks.sh pass; CLA is green and the published diff-coverage result is 130/130. Required CI is currently red: python standalone tests (7/9) timed out this changed guard suite after 240 seconds, which also failed the aggregate job. That may be a runtime/flakiness issue, but the required gate must be rerun green.
No REVIEW §15 post-restart witness was present in the PR or review thread when this review was submitted. Because either code fix moves the head, run and paste that witness on the final head. Worst case without these fixes: a future green-guard edit can read the declaration outside its owner or place the host token under an unrelated process variable, exposing it and/or breaking bridge startup and task delivery.
REVIEW.md §15 live witness at da4e1aeHead Which token each step used
(1) install writes the declaration through the owner — PASSThe full Call path: (2) the head's sparrowd runs the declared bridge with the token only via
|
There was a problem hiding this comment.
Reviewing exact head da4e1ae76d37006e667a511e8bec0f18a50ad386.
REQUEST_CHANGES — this head is not merge-ready.
The two requested regressions are fixed: the prior open(f"{path}").read() case is now reported, and the prior os.environ |= {...} case is now reported. The memoization is effective, the shipped adapter/reader paths still look safe, and the exact-head gates are green: local gateway 143/143, sparrowd 12/12, adapter 239/239, ConnectHostSupervised 5/5, git diff --check, and the repository review checks all pass; GitHub CI and CLA pass, with 130/130 changed executable lines reported covered.
Two executable false negatives remain inside the claims this test now makes:
-
The f-string exemption still trusts every callee.
message()classifies everyJoinedStras a message, and the call check exempts that argument without checking the sink. This direct consumer copies the declaration bytes outside the owner whiledeclaration_reads(),credential_access(), and the package finding all remain empty:path = bridge_home(root) / DECLARATION shutil.copyfile(f"{path}", dst)
Executing the mutant copied synthetic declaration bytes, while all 28
Guardtests passed. I also confirmed the same miss with an ordinary helper that callsPath(p).read_text(): 28/28Guardand 5/5ConnectHostSupervisedstill passed, and the helper read the synthetic declaration. This is not an arbitrary environment-read rewrite; it is exactly a call being handed the declaration path, including the helper case named indeclaration_reads()'s contract. Please restrict the message exemption to audited message sinks/argument positions, or conservatively track path arguments through unknown consumers, and add a copying/reading kill control plus a legitimate-print control. -
The reader's closed world permits a stored process-environment writer.
READER_ENVIRON_USESallows theos.environ.updateattribute expression, while the write check recognizes only calls whose callee still has a listed attribute name. Appending this after the pinned update produces notoken_injection_findings()finding:write = os.environ.update write({next(iter(environ)): token})
All 28
Guardtests and all 12 sparrowd tests passed. A synthetic execution then put the token under an unrelated process-environment key (LANG) in addition to the owner's key. Please bind allowed environment operations to the exact pinned call sites rather than allowing retrieval/storage of a bound mutator, and add this alias as a kill control. An independent probe also found the same closed-world weakness through functional mapping writers such asoperator.setitem(environ, ..., token).
§15 correction after inspecting the newly posted witness: the exact-head witness was posted seconds before this review but had not yet appeared in my room context. It exercises the real gateway and shipped scratch-tree sparrowd at this SHA, restarts sparrowd, delivers one real round trip through DeliveryCore, verifies owner-mediated declaration write/removal and token handoff, checks zero scratch-file token hits, and records teardown. Its stated limitation is the unchanged launchd/tmux portion of full install/uninstall. This satisfies REVIEW.md §15. The live-path gate is therefore PASS; the two code-enforcement blockers above are unchanged and still prevent approval.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewing exact head da4e1ae76d37006e667a511e8bec0f18a50ad386.
I accept the owner's stated stopping rule. Under that scope, my prior f-string sink and stored-os.environ.update findings are guard-completeness follow-ups in #5820, not blockers. The exact-head live witness satisfies REVIEW.md §15, and the focused suites and GitHub gates remain green.
REQUEST_CHANGES — one current shipped-path credential leak remains.
The owner recognizes two live token source names: TOKEN_KEYS = ("REMOTE_TASK_TOKEN", "AG2_REMOTE_TOKEN"). But bridge_token_environ() returns only the first name, and bridge_environ() strips only HOST_ONLY plus the keys in that one-entry mapping. HOST_ONLY itself contains neither token name. Therefore a sparrowd host credential supplied under the supported legacy alias AG2_REMOTE_TOKEN survives into the agent bridge. main() then installs that environment and runs the bridge module.
I reproduced this through the real wrapper path with synthetic values: the declaration was written by cli.wire_bridge, the wrapper fetched agent-token from its mocked vault reader, and the launched module saw:
rc 0
{'REMOTE_TASK_TOKEN': 'agent-token', 'AG2_REMOTE_TOKEN': 'host-legacy', 'KEEP_ME': 'yes'}
The current tests/sparrowd-agent-bridges.test.py 12/12 and tests/gateway-credential-sources.test.py 143/143 both pass despite that result. This is execution on the shipped reader/wrapper path, not another bypass form of the static guard. It exposes the host/core token to an agent bridge alongside the bridge's own per-agent token, so I cannot approve this SHA under the stated rule.
Please have the owner expose the full set of host-only token names, strip every supported token source name, and inject the per-agent token only under the canonical bridge name. Pin the behavior with both host variables present: REMOTE_TASK_TOKEN must be replaced by the vault token and AG2_REMOTE_TOKEN must be absent. The same finding is now recorded in #5820; because it affects the shipped path, it belongs in this PR under the stopping rule. Once that is fixed, I have no remaining blocker within the agreed scope.
…reviewed record The guard accepted the accessor beneath any keyword and any dict value, and did not model import aliases, so os.getenv(key=...), a read through an inline dict, and an aliased import all passed. A call is now allowed only as a value of a dict literal assigned whole to one name inside a listed (module, function) site; aliasing the accessor on import is flagged. One negative test per form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e one top-level site A record that started at the reviewed site could still be read back in the next statement, and any def named wire_bridge (nested, method, async, or a second one) counted as the site. The site is now the unique module-level def, the record bound there may only be passed whole, returned, or read at a key other than the one holding the name, and no module may read the record key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed sink Treating the record as whole whenever it was a positional argument to any call let dict(body).popitem()[1], d.update(body), list(body) and similar launder the name. The record may now go only to a listed sink (json.dumps, as its first positional argument, the one shipped use), be returned, or be read at a fixed key that does not hold the name. Any other use is flagged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dapter names the token key Four review rounds showed that statically following the record's data flow through an adapter cannot be made complete: a shadowed or aliased json, an encoder passed as cls=, or laundering through another call all slipped past. The owner now builds the declaration's JSON text (bridge_declaration_text), wire_bridge writes what it returns, and the guard reduces to one rule: no package module outside the owner names token_env_name, a key tuple, or the "token_env" key. The scan excludes the two owner modules by exact path, and a positive control plants a reference in the adapter. The declaration bytes and the printed worker name are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… reads it for sparrowd The adapter could still hold the owner's text and parse a selector out of it, and the guard checked key-tuple names but not their strings. The owner now writes the declaration itself (write_bridge_declaration, tmp + os.replace) and returns nothing, so the adapter hands it a path and never holds the record. The declaration's reader, sparrow_agent_bridge, takes the fixed fields and the token's variable from owner accessors instead of naming the field or its own constant. The guard flags any identifier or string naming an owner key tuple, an owner-private name or the record's token key, keeps the reader's accessors to the reader, scans the reader too, and pins that the adapter's one owner call is a discarded statement. Both owner copies are exercised. The declaration bytes, the printed worker name and sparrowd's checks are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eader's token comes from the owner The adapter still read an existing declaration to check it wrote it, so it held the bytes it could parse a selector out of, and the reader's token injection was not pinned to the owner's accessor. The owner now does the ownership check, the write and the removal (write_bridge_declaration returns '' or why not; remove_bridge_declaration returns removed and why), with the same messages. The guard treats a path built from the declaration's file name as opaque outside the owner: it may be formatted or handed to those two owner calls, nothing else. The reader's bridge_environ must merge bridge_token_environ(...) whole, store no item itself, and name no owner token variable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…alls to the owner's import, close the reader The guard read raw string literals where it detected a file name, a credential path or a systemctl probe, so "worker" + ".json" and similar folded forms went unseen; every detection now reads _constant (shape checks keep requiring a literal). The owner-call exemption trusted the method name: it now needs a direct call on the module's one import of the owner module, bound once and never rebound, and any other route to an owner entry point is flagged. A path returned by a helper, a lambda or a module global is the declaration too. The reader's bridge_environ is pinned to its exact body, and its mapping writes and main's environment to an exact allowlist, so a keyword write, a dead merge or a dynamic key after the merge is flagged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pin every environ reach; scan each module once
The previous head stopped the declaration walk at every f-string, so
open(f"{path}").read() went unseen. marked() is pure reachability again (it
stops only at an owner call), and a message is exempt only where it is the
argument itself: an f-string, a name bound from one, or a call to a function
returning only those, together with not/compare and and/or/if over them.
The reader allowed os.environ |= {...}: it now has no in-place or | merge,
every reach of a process environment is one of three pinned uses, and
__ior__/__or__ count as mapping writes.
The guard re-scanned the whole package in several tests, which timed out
under CI instrumentation. Each pure scan is memoized per (source, module),
the def listing is cached per module, and the taint loops stop at their
fixpoint: the file runs in ~8s instead of ~33s under coverage locally.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… must not inherit `bridge_environ` stripped only the name `bridge_token_environ` hands the agent's token under (`TOKEN_KEYS[0]`), so a host holding `AG2_REMOTE_TOKEN` or `REMOTE_MEDIA_HS_TOKEN` passed both to the agent bridge, where `discover()` reads either. The owner now exposes `bridge_strip_keys()`, every `TOKEN_KEYS` entry plus `MEDIA_TOKEN_KEY`, and the reader filters on that set before merging the agent's token. The guard pins the new body, admits the accessor to the reader-only set, and flags a reader that narrows the set again; the sparrowd suite builds the bridge's environment with a synthetic host value under each name and checks each is gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
da4e1ae to
7529fd4
Compare
…f the gateway token `bridge_strip_keys()` named `TOKEN_KEYS` and the media token, so a host's `GATEWAY_TOKEN` or `RELAY_TOKEN` still reached the agent bridge. The bridge reads neither, but room-ops and bee-actions rank both above `REMOTE_TASK_TOKEN`, so a child the bridge spawns would act as the host. The set is now the union of `TOKEN_KEYS`, the contract's `TOKEN_ALIAS_PRECEDENCE` (imported, not copied) and `MEDIA_TOKEN_KEY`, each once; the guard asserts the tuple against the contract, and the sparrowd suite gains one per-key test for each alias. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 36beb75aeee96bdfc233b2cdd444710b8f4a3700. REQUEST_CHANGES — not merge-ready.
The direct environment-key fix is now sound. I reproduced the shipped cli.wire_bridge -> sparrow_agent_bridge.main path with synthetic host values under REMOTE_TASK_TOKEN, AG2_REMOTE_TOKEN, GATEWAY_TOKEN, RELAY_TOKEN, and REMOTE_MEDIA_HS_TOKEN: the child retained only the injected agent REMOTE_TASK_TOKEN plus an unrelated sentinel. The parent 7529fd40 still leaked GATEWAY_TOKEN and RELAY_TOKEN, so the new control discriminates. Credential-source tests pass 143/143, bridge tests pass 18/18, canonical/vendored sync passes, and review checks pass.
Two blocking findings remain:
-
P1 — the per-agent bridge still reads the host credential file through inherited
CLAUDE_CONFIG_DIR. The launchd job supplies that host config root (plist L87-L88);bridge_environ()preserves it (reader L127-L135); and the child resolves$CLAUDE_CONFIG_DIR/channels/ag2space/.env(source L220-L227) and registers every discovered credential candidate (bridge L1815-L1820). An exact-head synthetic witness produced:winner_url https://agent.example/relay winner_token_is_agent True host_file_token_was_read True candidate_paths ['<worktree>/.review-host/channels/ag2space/.env']The agent token wins, but the host token bytes still enter the per-agent bridge and its secret registry. That violates the credential-ownership boundary through an indirect shipped path, and the new direct-key regression does not cover it. Please add a shipped-path sentinel for this route and separate the agent bridge's credential discovery from the host
.env. Blindly strippingCLAUDE_CONFIG_DIRis not necessarily safe because this bridge also uses it foraccess.jsonandREMOTE_PROACTIVE_TRUST_OK; preserve required noncredential policy through an explicit audited path. -
P2 — exact-head required CI is red.
tests/sparrowd-agent-bridges.test.py:366usesseen.update(os.environ), which the hermetic credential-test gate correctly reports as a whole-environment copy. The local exact-head run failed both its normal and Python-3.9 fallback variants withtests/sparrowd-agent-bridges.test.py:366: copy, and GitHub'spython standalone tests (7/9)job is now failed. Capture only the asserted keys (or use the repository's hermetic helper).
The final-head live gate is also still outstanding. The only posted REVIEW.md §15 post-restart witness is for da4e1ae7; both 7529fd40 and 36beb75a changed the shipped bridge environment afterward, and the PR body says the new witness has not been run. After the two blockers above are fixed and the head is frozen, please record the real post-restart round trip for that final SHA and get required CI green.
No separate nits.
…e wrapper carries the host's trust setting The per-agent bridge inherited the host's CLAUDE_CONFIG_DIR (needed for access.json), and at import `_channel_env_candidates()` opened `$CLAUDE_CONFIG_DIR/channels/ag2space/.env`, so `discover()` registered the host's file token before precedence picked the agent's. The owner now marks a handed-in token (`bridge_token_environ` sets GATEWAY_CREDENTIALS_INJECTED) and `env_files()` returns nothing under that mark, so no caller in the child, the config fallback included, reaches the file. The one host setting the child took from that file, REMOTE_PROACTIVE_TRUST_OK, is resolved by the wrapper through `bridge_carried_config()` (environment, else the host's files, credentials staying inside the owner) and handed in as a plain value. The guard pins the new reader body and the carried set; the sparrowd suite runs the shipped path with the reader and open instrumented and a canary token in the host's .env. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head dd5b0d5f690fab23b01042ab7f0c7122bf24d753 against base 7940f25ff965f51df3273d94f009cb1b771119c0.
The three blockers from my review at d094bf0a are resolved: marked readers no longer reopen host file/vault credentials, carried trust comes only from the owner, and the hermetic structural gate passes. The new wake-child identity decision is also explicit: it acts as the declared agent. I am replacing that stale review state with the current-head finding below.
Blocker
[P1] Bind the declared-agent bearer only to an agent-owned endpoint.
bridge_environ() strips token aliases but not URL aliases. The bridge's production discovery then prefers REMOTE_TASK_URL / AG2_REMOTE_URL from its inherited environment over the URL embedded in the agent token. Finally, dd5's new wake path passes that already-selected URL beside _RAW into wake_environ.
I exercised the shipped real-subprocess chain (wrapper -> bridge import/discovery -> queued task -> _fire_on_task -> wake subprocess -> room ops) four times. Both supported inherited aliases fail:
REMOTE_TASK_URL: host_token_loaded=false host_url_loaded=true agent_token=true agent_url=false
AG2_REMOTE_URL: host_token_loaded=false host_url_loaded=true agent_token=true agent_url=false
GATEWAY_URL: host_token_loaded=false host_url_loaded=false agent_token=true agent_url=true
RELAY_URL: host_token_loaded=false host_url_loaded=false agent_token=true agent_url=true
FAILING ALIASES: ['REMOTE_TASK_URL', 'AG2_REMOTE_URL']
The existing sentinel seeds only GATEWAY_URL, which this discovery path does not read, so its green assertion does not cover either actual URL_KEYS alias. Worst case, the agent bearer is sent to an unintended/stale host gateway; even without interception, declared-agent task, result, and room operations are routed to the wrong deployment.
Please resolve the endpoint under the shared declared-agent credential owner before it becomes bridge state: exclude both supported host URL aliases from the generic base, then derive or explicitly carry an agent-owned endpoint. Add both REMOTE_TASK_URL and AG2_REMOTE_URL cases to the real-chain sentinel. Fixing only the final wake environment is insufficient because the bridge itself has already selected the wrong endpoint. For precision, bridge-level URL contamination exists at the base commit; dd5 newly propagates that contaminated endpoint with the agent bearer into the wake child and claims the changed-path invariant that the child gets the agent's URL.
Required live evidence
The PR body still says the REVIEW.md section 15 post-restart witness has not run. After the code fix and a frozen final head, please provide the real restarted-service declared-agent round trip; the hermetic subprocess sentinel is valuable but does not replace that merge gate.
Verification
- Independent production-path alias repro: both actual aliases fail as shown above; both controls pass.
tests/declared-bridge-wake-child-credentials.test.py: 9/9 pass stock.tests/sparrowd-agent-bridges.test.py: 21/21 pass.tests/gateway-credential-sources.test.py: 144/144 pass.tests/hermetic-env-structural.test.py: 26/26 pass.- Adapter suite: 239/239 pass; vendor sync and review scans pass.
- Exact-head GitHub checks: 35 success, 3 skipped, none failing/pending;
license/clais successful.
Requesting changes for the credential/endpoint pairing above and the still-pending exact-final-head live witness.
…m the agent's token bridge_environ stripped the host's token aliases but kept its URL names, and discover() ranks an environment URL above the one inside the token. An inherited REMOTE_TASK_URL or AG2_REMOTE_URL therefore pointed a declared bridge at the host's gateway with the agent's bearer, and the wake child inherited that endpoint. The owner's bridge_strip_keys now also lists each URL name and alias (URL_KEYS and the contract's URL_ALIAS_PRECEDENCE), so bridge_environ drops them from the generic base and the bridge's endpoint is the one inside the agent's token. wake_environ drops the same names and sets the bridge's URL beside the agent's token. Tests: the real-chain sentinel runs once per host URL name (REMOTE_TASK_URL, AG2_REMOTE_URL, GATEWAY_URL, RELAY_URL) and requires both the bridge's own URL and the wake child's room-ops URL to be the agent's. A bridge_environ test covers all four names, and the pinned strip list is updated. Without the URL names in the owner, all four sentinel cases fail. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen @liususan091219 the URL finding from both reviews at dd5b0d5 (Kewei 5636966255, Susan 5636948182) is fixed at the owner in 989fa2b. CI is green there: 35 checks pass, the required ones included. Two unrelated tests,
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 989fa2b5. The URL finding from my review at dd5b0d5f (5636948182) is fixed. I checked it in the code and re-ran it myself. Not approving: there is no approval from anyone else at this head, and license/cla is absent.
Where this account's change request stands. Its CHANGES_REQUESTED (at 2c06c889) stays on the record until an approval replaces it. Every blocking ask from this account is now cleared: asks 1 and 2 (cleared at 0da379e2), the wake-child identity question (answered at dd5b0d5f), and the URL finding (fixed here). The only open item is ask 3, the §15 post-restart round trip at the frozen final head, which is named, not blocking.
The strip now covers every URL name discover() can read. Verified in code.
URL_KEYSis("REMOTE_TASK_URL", "AG2_REMOTE_URL"), anddiscover()reads the environment URL only throughfirst_env(env, URL_KEYS). Its other URL tiers are the URL inside the winning token, and the file tier, which under the mark gets no files.- The contract's
URL_ALIAS_PRECEDENCEisGATEWAY_URL, RELAY_URL, REMOTE_TASK_URL, AG2_REMOTE_URL.bridge_strip_keys()now returns all four plus bothURL_KEYS, once each, in both byte-identical copies (sync_from_src.py --check: in sync). bridge_environstripsbridge_strip_keys().wake_environnow strips the same set, which is exactly the names it stripped by hand atdd5b0d5f, then sets the bridge's URL beside the agent's token.remote_gateway_bridge.pytakes its endpoint only from_FOUND.url. It reads no other URL-type environment name.
Tests, run at 989fa2b5 in a scratch worktree under Python 3.12
declared-bridge-wake-child-credentials9/9 (the sentinel runs once per URL name),sparrowd-agent-bridges22/22,gateway-credential-sources144/144,hermetic-env-structural26/26,test_on_task_wakeall pass, claude-code adapter 239/239,test_vault_token_tier7/7,credential-contract-golden37/37, and thechannel-env-*andchannel-token-*suites all pass.- The new tests fail on the old code. With the three source files at
dd5b0d5fand the new tests kept, the sentinel fails forREMOTE_TASK_URLandAG2_REMOTE_URLon the bridge's own endpoint ('https://host-gw.example/relay' != 'https://agent-gw.example/relay').GATEWAY_URLandRELAY_URLpass there, as they should, becausedd5b0d5f's wake child already stripped them. The newbridge_environtest and the pinned strip tuple also fail. - The author's mutation claim holds. At this head, with only the two URL terms removed from
bridge_strip_keys()in both copies, all four sentinel cases fail. The first two fail on the bridge's endpoint, the last two on "room ops resolved the host's gateway URL". Files restored afterwards. - Probe.
bridge_environwith all four host URL names and a host token set: ahttps://agent/relay|<secret>token makesdiscover()return the agent's URL. A bare secret makes it return'', and no host value survives either way.
The fail-closed consequence. I looked for a current deployment that this would break, and found none. The documented install path says vault set AG2_RELAY_TOKEN_<worker> <the agent's relay token>. The relay token AG2 Space issues for an agent is the full <relay_url>|<secret> string, so the URL comes with it. A bare secret only fails if someone pasted one by hand. On main that would have borrowed the host's endpoint. Two small notes, neither one an ask:
- The FATAL hint that a declared bridge then prints, "set REMOTE_TASK_URL", cannot help it, because that name is stripped. A declared-bridge-specific line would be clearer: "the vault token must be the combined
https://…|<secret>string". - The PR body's 989fa2b bullet does not state this consequence. Your comment does, and the body is the record.
Gates at this head: CI 35 success, 3 skipped, none failing. license/cla is absent: the status endpoint returns 0 contexts at 989fa2b5. It was success at dd5b0d5f. 0 unresolved threads. Mergeable, but BLOCKED.
What it still needs
- An approval at this head. Kewei's latest review is a CHANGES_REQUESTED at
dd5b0d5ffor this same URL finding plus the §15 witness. Nobody else has reviewed989fa2b5. license/clareported at this head.- Still owed (named, not blocking): the §15 post-restart round trip at the frozen final head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 989fa2b56c3017a0d19624b69a6561174708130d (direct parent dd5b0d5f690fab23b01042ab7f0c7122bf24d753, base 7940f25ff965f51df3273d94f009cb1b771119c0).
The endpoint-binding blocker from my review at dd5b0d5f is fixed. The shared owner now strips all four URL aliases before bridge discovery, the real wrapper → bridge → queue → wake subprocess → room-ops chain selects the agent endpoint for REMOTE_TASK_URL, AG2_REMOTE_URL, GATEWAY_URL, and RELAY_URL, and a parent-owner mutation makes all four cases fail. The prior token/file/vault/trust isolation remains green.
I am requesting changes for one compatibility blocker plus the required live gate.
Blocker
[P1] Preserve or explicitly migrate the supported declared-agent split credential form.
The security boundary in bridge_strip_keys() is correct, but bridge_environ() now removes the only endpoint available to a declared bridge whose vault entry is a bare secret. That is a previously accepted persistent configuration: docs/remote-gateway-protocol.md says the split URL + token form is recommended and calls the combined token a legacy/bootstrap shortcut; the managed adapter instructions say to put <token> in the derived vault key and define no agent-owned URL field or combined-only requirement.
I reproduced the shipped declaration/wrapper/bridge startup with a synthetic bare vault secret and inherited REMOTE_TASK_URL, stopping before any network call:
dd5b0d5f: URL=https://host-gw.example/relay, agent bearer selected, startup validation passed
989fa2b5: URL='', agent bearer selected, FATAL: no gateway URL
The fail-closed direction is right; restoring an unbound host endpoint is not the remedy. The rollout is not mitigated, though: an existing declared split-form agent stops processing after its worker restarts. The generic fatal also says to set REMOTE_TASK_URL, but this wrapper deliberately strips that setting, so one of its two recovery instructions cannot work for this mode.
Please keep the endpoint/credential binding and provide either a trusted agent-owned endpoint source or an explicit managed-mode migration/preflight to combined tokens, with accurate persistent-configuration docs and an actionable diagnostic. Add a regression for the chosen transition. An owner-accepted compatibility break would still need a concrete pre-upgrade migration for existing declared agents; one live test identity using a combined token does not establish that transition.
Required live evidence
The REVIEW.md §15 exact-head post-restart round trip is still explicitly outstanding. The four-alias subprocess sentinel is good harness evidence, but approval requires the authorized real restarted-service round trip at this frozen SHA.
Verification / gates
declared-bridge-wake-child-credentials: 9/9;sparrowd-agent-bridges: 22/22; credential owner: 144/144; adapter: 239/239; hermetic structural: 26/26.- Vendored sync,
git diff --check, and cumulative + child-only review scans pass. - Exact-head GitHub Actions: 35 success, 3 skipped, none failing or pending.
license/clais absent at this SHA (statuses=[], positive control: success at parentdd5b0d5f), so the complete merge gate is not green.
Verdict: changes requested; this head is not merge-ready. Resolve the declared split-form transition, provide the exact-head §15 witness, and restore the CLA status.
…token or the owner's vault URL key With every host URL name stripped, a declared agent on the split credential form (a bare secret in AG2_RELAY_TOKEN_<worker>) had no endpoint and died with "no gateway URL", and that FATAL told it to set REMOTE_TASK_URL, which the wrapper strips. The agent now gets an endpoint from a source the owner writes. sparrow_agent_bridge.endpoint() reads AG2_RELAY_URL_<worker>, a vault key derived from the segment like the token key, only when the token embeds no URL; an embedded URL wins. gateway_credential_sources.bridge_token_environ(token, url) sets REMOTE_TASK_URL beside a bare token, and gateway_url_problem refuses anything but an http(s) URL with a host. The declaration still chooses nothing: a URL the agent's own session writes into worker.json is never read. A bare secret without the URL key does not start, and the wrapper names the one `vault set` that fixes it. `sparrow_agent_bridge.py --preflight` lists every declared agent and whether it will start, never printing a value. The bridge's FATAL for a declared bridge now names the vault keys and the preflight. Install prints the URL key's `vault set` line, and the managed-mode README and the protocol doc describe the rule. Tests: the split form with the URL key works, and without it fails with the diagnostic; a combined token ignores, and never reads, the URL key; a host URL under any of the four names never reaches the bridge; a URL written into worker.json is never used; an unusable vault URL is refused by name; preflight reports each case. The real-chain sentinel runs the combined and split forms for every host URL name, and the pinned reader shapes are updated. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es and the preflight's failure paths Diff coverage measures the src/ owner copy, which the new behavior was exercised only through the package copy. The both-copies owner test now covers bridge_token_environ with and without an embedded URL, token_embeds_url, gateway_url_problem and wake_environ. The preflight tests add a missing token, an unreadable declaration, and the default vault reader. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@keweichen your split-form P1 from review 5637489086 is fixed at 49d5005 (commits 8804cf5, 49d5005). The endpoint now has an agent-owned source, and nothing is inherited from the host. CI is green there: 36 checks pass, the 15 required ones included, and
The §15 live round trip at the frozen final head is still owed. It runs once the owner gives his word for this sha. |
…d, not as a bare secret A token that starts with a gateway URL but carries no url|secret separator (an encoded or lost '|') was reported to a declared bridge as a bare secret, and the wrapper would have looked for its URL key instead. The owner's token_malformed() classifies it. The wrapper refuses it with "a malformed combined token: store it again as https://<gateway>|<secret> under AG2_RELAY_TOKEN_<worker>" without reading the URL key, and the bridge's FATAL for a declared bridge says the same. Tests: the both-copies owner test covers token_malformed; the wrapper refuses a malformed token by name and never reads its URL key; the declared bridge's FATAL names it as malformed. With the classification removed, all three fail. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… refuse a token with no secret The host's own bridge still decided its missing-separator hint with its own `TOKEN[:4] == "http"` test, which also hinted for a bare secret such as `httpfoo...`. It now asks the owner's token_malformed(_RAW). A token with an empty secret (`https://gw|`) passed --preflight and then exited FATAL in the bridge. The owner's token_secret_empty() now lets the wrapper and the preflight refuse it by name first. Tests: the host's bridge hints only for a malformed combined token, not for `httpfoo...`; the wrapper and the preflight refuse a token with no secret; the both-copies owner test covers token_secret_empty. With either fix reverted, its regression fails. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed current exact head 43d1c5ac1d868930131d4362774233c05c97c7c1 against base 7940f25ff965f51df3273d94f009cb1b771119c0, including the last-minute 43d1c5ac delta.
The previous endpoint-source design is directionally fixed: the split URL key is derived from the agent segment, worker.json cannot choose it, every host URL alias is stripped, and combined tokens win without reading the URL key. The new head also correctly makes an empty-secret combined token fail preflight and centralizes the host bridge's missing-separator hint. I am still requesting changes for three exact-head findings plus the required live gate.
Blockers
-
[P1] Preserve the canonical URL-shaped opaque bearer in the split form.
token_malformed()classifies every scheme-prefixed value without a separator as a broken combined token, andendpoint()refuses it before reading the derived URL key. That contradicts the canonical parser's explicit rule that a scheme-prefixed/no-separator value remains an opaque bare secret (shared contract lines 74–94) and its existing pinned valid rowURL_SHAPED_BEARER + REMOTE_TASK_URL(test lines 68–97). At this head:endpoint(URL_SHAPED_BEARER, owner_url_key) -> malformed combined token discover(REMOTE_TASK_TOKEN=URL_SHAPED_BEARER, REMOTE_TASK_URL=owner_url) -> the same token + owner_urlSo the earlier split-form compatibility blocker is not fully resolved: a previously supported opaque bearer still stops after this upgrade even though its owner-written URL exists. Do not infer a lost separator solely from the secret's shape; honor the canonical opaque-bearer case when the derived URL is present, or deliberately revise and migrate the shared contract. Add the existing
URL_SHAPED_BEARERsentinel to the declared-agent endpoint tests. -
[P1] Redact the newly admitted vault URL before the startup log persists it.
The new path reads
AG2_RELAY_URL_<worker>, injects it asREMOTE_TASK_URL, and reaches the unchanged raw interpolation atremote_gateway_bridge.py:7554. The validator permits URL userinfo, query, and fragment; this module already registers those pieces as secrets and owns_redact_url(), but the startup log does not call it. A synthetic production-main run, stopped atrefresh_routingbefore the first network request, printed and persisted:[remote-gateway-bridge] starting — gateway=https://url-user-5815:url-pass-5815@gw.example/relay?api_key=query-secret-5815#frag-secret-5815 ... registry_contains_query_secret=True redacted_form=https://gw.example/relayThe raw sink existed at the parent, but this delta newly gives a declared bare-token bridge an owner-vault URL path to it; at
989fa2b5that split URL could not reach the process. Log the redacted form and add a split-vault URL sentinel with synthetic userinfo/query/fragment values. -
[P2] Reject hostless URLs in both credential forms before preflight says
ok.The new empty-secret check is correct. The adjacent URL decision remains incomplete:
gateway_url_problem()treats text before the first slash as a host instead of parsing the authority, whileendpoint()bypasses that validator entirely for combined tokens. At this head, split URL keyshttps://?missing-host,https://#missing-host, andhttps://:443/relayall yieldgateway_url_problem == ''and preflight exit 0; the corresponding combined forms with|secretalso pass.urllib.parse.urlsplit(...).hostnameisNonefor every case, and the production request path cannot use them.Since this command is the advertised pre-restart transition gate, a false
okdefeats its purpose. Validate a parsed hostname for both forms and pin the hostless adjacent cases.
Required live evidence
The PR body still says the REVIEW.md §15 post-restart round trip has not run at this frozen head. The 8-case subprocess sentinel is useful harness evidence, but approval still requires the author's real restarted-service declared-agent round trip on the final SHA.
Verification / gates
- Exact-head focused suites: declared wake chain 12/12, wrapper 33/33, credential owner 145/145; the prior-head hermetic structural 26/26 and adapter/install subsets passed, and this delta does not touch those paths.
- Mutation control: disabling split-URL propagation makes exactly the four split-form wake cases fail while the four combined cases pass, so the new real-chain coverage discriminates.
- Review checks (hardcoded paths, root artifacts, prose cap) pass.
- Exact-head GitHub checks are still running with no failure reported at review time;
license/clais successful.
Worst case on upgrade: a supported declared agent stops because its opaque bearer resembles a URL; a credential-bearing vault URL is copied into persistent logs; or the safety preflight blesses a structurally unusable endpoint. Verdict: changes requested; this head is not merge-ready. Re-review after these findings are fixed and the exact-final-head §15 witness is posted.
…e gateway in logs, require a parsed host Three findings from review at 43d1c5a: - The shared contract reads a scheme-prefixed token with no separator as an opaque bare secret. The wrapper refused it as a malformed combined token before reading AG2_RELAY_URL_<worker>. It now reads the URL key first; only with no gateway does it name both remedies (re-store a combined token that lost its '|', or set the URL key for an opaque bearer). The owner's helper is now token_url_shaped(), documented as never a reason to refuse a token whose gateway is known. - The vault URL reaches the bridge as REMOTE_TASK_URL, and the startup log printed URL raw, userinfo, query and fragment included. The startup line and the token-file gateway-change line now go through the module's _redact_url(). - gateway_url_problem() parses the URL and requires a hostname and a valid port, so https://?x, https://#x and https://:443/relay are refused. The wrapper and the preflight validate a combined token's embedded gateway with the same check. Tests: a URL-shaped bearer (the contract's URL_SHAPED_BEARER) runs with its URL key and, without one, is refused naming both remedies; the declared bridge's FATAL names both; the startup log carries no userinfo, query or fragment of a vault URL; hostless gateways are refused in both forms by the wrapper and the preflight; the both-copies owner test covers the parsed host check. With each fix reverted, its regressions fail. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
|
@keweichen all three findings from review 5637862457 at 43d1c5a are fixed at 97e312b. CI is green there: 36 checks pass, the 15 required ones included, and
Mutation checks, each fix reverted alone:
Local runs (load-gated, The PR body's "After review" section covers these commits. The §15 live round trip at the final head is still owed, pending the owner's word for this sha. |
REVIEW.md §15 live witness at 97e312bHead Which token each part used
(1) install writes the declaration through the owner — PASS (synthetic)The full Synthetic sentinel: the real sparrowd → wrapper → bridge → wake child chain — PASSSetup. The run was the head's
An audit hook (
(2) the head's sparrowd runs the declared bridge with the real token, restart, one round trip — PASSThe head's Restart. SIGTERM went to the sparrowd PID after One round trip after the restart. The owner's agent posted one message to the test agent in a test room. The witness wrote the result file atomically. Token absence. These are in-process counts only; the value was never printed.
(3) uninstall removes the declaration through the owner; sparrowd stops the worker — PASSSynthetic: Teardown. sparrowd was stopped by verified PID and no scratch process was left. The owner's agent deleted the vault copy and restored the test identity's relay. What this proves at 97e312b:
What it does not prove:
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 97e312b1. Kewei's three findings from review 5637862457 (at 43d1c5ac) are fixed. I checked each one in the code and ran the tests myself. The §15 witness at this head ran the production path. I am not approving yet, because nobody else has approved at this head. Kewei's CHANGES_REQUESTED at 43d1c5ac is still his latest verdict.
Where this account stands. Its CHANGES_REQUESTED at 2c06c889 stays on the record until an approval replaces it. All of its blocking asks were cleared by 989fa2b5. Ask 3, the §15 round trip, is now supplied (see below). From this account's side the code is ready to approve.
1. URL-shaped opaque bearer: fixed. endpoint() in src/sparrow_agent_bridge.py no longer refuses a token that starts with a scheme but has no separator. A combined token still takes its URL from the token. Any other token, URL_SHAPED_BEARER included, reads AG2_RELAY_URL_<worker>. It is refused only when that key is empty, and then the message names both remedies. token_malformed is gone, replaced by token_url_shaped, and its only remaining uses are hints shown when no gateway was found.
2. Vault URL in logs: fixed for logs. The persisted record is a fair follow-up.
- The startup line (
remote_gateway_bridge.py:7555) and both URLs in the token-file gateway-change line (:2742–2743) now go through_redact_url.gateway-status.json(:3651) already did. - I grepped every
URLuse inremote_gateway_bridge.py, plusevent_channel.py,sparrow_agent_bridge.pyand the owner. Every other use is a request target (_req,EventChannel,CardPoster), a comparison (_same_origin), or the wake child's environment.--preflightprints no value. - One raw sink is left:
refresh_routing()writes"gateway": URLtostate/owner-routing*.json(:1910). That write is unchanged from base7940f25f. On base, a combined token whose URL carried userinfo already reached it, so this PR adds one more source rather than a new sink. AG2 Space issueshttps://<gateway>/relay|<secret>, with no userinfo or query. #5845 tracks it, so I accept it as a follow-up. A cheap way to close it for the newly admitted path: havegateway_url_problemrefuse userinfo, query and fragment, which no gateway needs. Note that_redact_url's own docstring saysstate/vault-syncs, so #5845 should be fixed and not left open indefinitely. Not an ask for this PR.
3. Hostless URLs: fixed. gateway_url_problem() now parses with urlsplit and requires a hostname and a parseable .port. endpoint() runs the same check on a combined token's embedded gateway, so both forms share one validator. I probed it directly:
- Refused:
https://?missing,https://#x,https://:443/relay,https:///relay,gw.example/relay,ftp://h/r,https://h:99999/r,https://h:abc/r,https://[::1/r. - Accepted:
https://h/r,https://h:8443/r,http://h/r(http was allowed before too), and alsohttps://u:p@h/relayand…?api_key=x. Userinfo and query are accepted and then redacted in logs, which is why the suggestion under 2 is relevant.
Tests, run at 97e312b1 in a scratch worktree, Python 3.12.15, scratch HOME
gateway-credential-sources145/145,sparrowd-agent-bridges35/35,declared-bridge-wake-child-credentials13/13.sync_from_src.py --check: in sync, and the two owner copies are byte-identical.- The new tests fail on the old code. I put
src/andpackages/back to43d1c5acand kept the new tests:- the hostless test fails for all three cases in both forms;
- both
URL_SHAPED_BEARERendpoint tests fail; - the startup-log redaction test and the both-remedies FATAL test fail;
- the owner test errors on
token_url_shaped/token_gatewayin both copies.
So 8 failures, 2 failures and 2 errors across the three suites. The tests check behaviour, not source text. The redaction test runsmain()up torefresh_routing, so it does not cover the routing record, which matches the follow-up scope.
§15 witness (comment 6386036603): it ran the production path.
- The head's
sparrowd.pyransparrow_agent_bridge.main→endpoint()→bridge_environ→ the real bridge. - It used a real test identity's token against the real gateway. It did a SIGTERM restart checked by PID, then delivered exactly one result after the restart.
- After that it removed the declaration through the owner, and sparrowd stopped the worker.
- The only stand-ins were a scratch HOME/workspace and a
securityshim that answered that one keychain key, which I accept for a witness that must not touch the installed service. - Its limits are stated: the live run used a combined token, so the split form was covered only by the synthetic sentinel. The launchd/tmux parts of
installwere not exercised, and this PR does not change them.
Gates at this head: 36 checks pass (license/cla among them, success, 1 status context), 3 skipped, none failing. 0 unresolved threads. Mergeable. reviewDecision CHANGES_REQUESTED.
What it still needs: an approval at 97e312b1 from another reviewer, ideally Kewei re-reviewing against his three findings and the witness. This account will approve once one exists at this head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 97e312b1a02dc178e4ea7d1c1e6f602185aecaf8 against merge-base 7940f25ff965f51df3273d94f009cb1b771119c0.
APPROVE — the three blockers from my review of 43d1c5ac are fixed, the exact-head live gate is satisfied, and I found no scoped code blocker.
Verified:
- A URL-shaped opaque bearer works in split form: the token remains opaque and the gateway is loaded from the derived vault URL key. Combined
URL|tokenstill takes precedence. - Gateway URL parsing/host/port validation is centralized in
gateway_url_problem()and both endpoint resolution and--preflightdelegate to it. Hostless, malformed-port, and malformed-IPv6 inputs fail closed. - The startup and valid token-file gateway-change log paths call
_redact_url(); the lost-separator diagnostic now distinguishes malformed combined credentials from an intentional URL-shaped bearer. - The PR body gives the required transition order: pull, run
--preflight, then restart. - The exact-head §15 witness in issue comment
6386036603documents a post-restart task/result round trip through the real declared bridge and DeliveryCore, exactly-once archive behavior, token non-leak checks, uninstall shutdown, and cleanup. I treated this as published live evidence rather than independently rerunning the owner's real-token path.
Local evidence at this head:
tests/gateway-credential-sources.test.py: 145/145 passtests/sparrowd-agent-bridges.test.py: 35/35 passtests/declared-bridge-wake-child-credentials.test.py: 13/13 passpackages/ag2-sparrow/tests/test_gateway_status.py: passpackages/ag2-sparrow/tools/sync_from_src.py --check: passgit diff --checkandscripts/review-checks.sh: pass
Worst-case transition behavior is deliberate and documented: a legacy bare token without a URL vault key now stops rather than guessing, with --preflight required before restart.
Non-blocking residual: _redact_url() still returns its input on parse failure. I reproduced _reload_rotated_token() logging userinfo/query/fragment from a malformed token-file gateway URL. That behavior is also present at the merge-base and the new shared validator prevents such a declared endpoint from starting, so it is not a regression or a reason to hold this PR. Please track a fail-closed redactor follow-up and narrow the PR-body claim that credentials are never logged; issue #5845 already tracks the separate persisted raw routing-URL sink.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approving at 97e312b1. This replaces my change request from 2c06c889 and my comments at 989fa2b5 and 97e312b1.
The bar is met: keweichen (a collaborator with write access) approved this exact head (review 5638861472), and the code is verified.
Evidence, from my review at this head (#5815 (review)):
- Every one of my earlier asks is cleared: the injected-mark gate on the file and vault readers, owner-exclusive
REMOTE_PROACTIVE_TRUST_OK, the hermetic lane, the wake child acting as the agent, and URL names stripped inbridge_environ/wake_environ. - Kewei's three findings at
43d1c5acare fixed: the URL-shaped bearer,_redact_urlon every gateway log line, and host/port validation in the singlegateway_url_problem. With the sources put back to43d1c5ac, the new tests fail. - The §15 post-restart round trip at this head ran the production sparrowd → bridge path (comment 6386036603).
- CI: 36 pass, 3 skipped.
license/clasuccess (1 context). 0 unresolved threads.
Follow-up, not blocking: #5845, the raw gateway URL in the persisted routing record. Having gateway_url_problem refuse userinfo, query and fragment would close it for the new path.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 97e312b (suites run locally: declared-bridge-wake-child-credentials 13 OK, sparrowd-agent-bridges 35 OK, claude_code_adapter 239 passed, gateway-credential-sources 145 OK; vendored copy identical; review-checks clean). The code holds: worker.json (0644) carries only names, paths and the vault key name; the token is read only through vault_intercept.get_vault_key; the gateway URL comes only from the token or the AG2_RELAY_URL_<worker> vault key; --preflight prints names never values (asserted); write_bridge_declaration is temp-file + chmod + os.replace with a failed write leaving nothing behind; foreign file, invalid JSON, list, directory, dangling symlink and unreadable file are each refused untouched; startup/rotation log lines pass URLs through _redact_url.
Requesting changes on one gate only, REVIEW.md lesson 15: the body says the live post-restart witness has not been run, and this changes how sparrowd starts bridges and what the wake child's environment holds — a live path, not approvable on harness proof alone (the same bar my hold on #4196 applies). Run one restart round trip on an affected host that completes a task and I approve at that head.
Should-fix alongside: (1) the body's "the vault, which only the owner writes" is not enforced — the agent session runs as the same user and can write AG2_RELAY_URL_<worker> with security, which is exactly the threat the body gives for keeping the URL out of worker.json; correct the threat model (pre-existing exposure, not a regression). (2) GATEWAY_CREDENTIALS_INJECTED makes channel_token.token_from_vault/token_from_env_file return '' for every variable (src/channel_token.py:~121,~146), so Slack/Telegram/Discord tokens are also closed in a declared bridge's tree — safe, but say so. (3) The upgrade disruption (a declared agent with a bare-secret token and no AG2_RELAY_URL_<worker> stops starting until --preflight) belongs in the release notes. (4) _declaration_owned reads with no size cap while the reader caps at 64 KiB; preflight tracebacks when agents/ cannot be listed.
|
@qingyun-wu thanks for the review at 97e312b (5638933083). This is a PR body change only: the head is still The §15 gate. The live post-restart round trip ran at this exact head: #5815 (comment). The PR body's old "has not been run" line is replaced by a "Live post-restart witness" section pointing there, with its findings and stated limits.
Should-fixes:
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at 97e312b (unchanged head; body-only update). The REVIEW.md lesson 15 gate is met by the live witness in issuecomment-6386036603: the head's sparrowd ran the declared bridge for a test identity with its real relay token handed in only as REMOTE_TASK_TOKEN and marked injected, no host token or URL name reached it, the bridge connected to the real gateway, was restarted by verified PID, served one live message with exactly one delivered line, removing the declaration stopped the worker, and the token appeared 0 times in 6,127 scratch files and in no process's exec-time environment; the synthetic sentinel covers the split-token form with all four host URL names inherited. Stated limits (one identity, one host, combined token; install/uninstall launchd and tmux parts not run) are the ones I expected. The threat-model paragraph now says what the vault does and does not enforce (same-user writes to AG2_RELAY_URL_<worker> are a pre-existing exposure, no worse than the token key, not reaching host credentials); the body states that every channel token is closed under GATEWAY_CREDENTIALS_INJECTED and the environment-tier limit; the release notes carry the upgrade step (--preflight before restarting sparrowd); the _declaration_owned cap and the preflight traceback are in #5845. Code approved earlier as sound; approval at this head.
Follow-up to #5698, closing the guard gap reported in #5698 (comment), with every review round on this PR folded in.
Thanks to the reviewers whose findings shaped each round: the reviews on this PR (most recently 5620661505 and 5620696233 at 2c06c88, and 5621130345 at a5288fc), and an independent agent review that found the folded-literal gap fixed here.
Design: remove the data flow, don't analyse it
A static guard can't resolve every alias. Each time the AST check was tightened, review found another way through. So the record, and the file that holds it, no longer reach any module outside the owner.
src/gateway_credential_sources.py, vendored byte-identical bytools/sync_from_src.py):write_bridge_declaration(path, *, written_by, ...)checks any file already atpath. A directory, a dangling symlink, an unreadable file, or a filewritten_bydidn't write each leave the file alone and return the same message the adapter printed before. Otherwise it writes the declaration (tmp +os.replace) and returns''.remove_bridge_declaration(path, *, written_by)returns(removed, why).wire_bridgeandunwire_bridgeinadapters/claude_code/cli.pypass a path and their own values and print what they printed before. The adapter never opens the declaration or holds its bytes.src/sparrow_agent_bridge.py(sparrowd) takes the fixed field frombridge_declaration_fields(). It hands the bridge its token throughbridge_token_environ(token), whichbridge_environmerges whole.What the guard proves
The guard proves delegation: the adapter and the declaration's reader route through the owner, and they don't name or open what only the owner should. It can't prove that a module has no arbitrary new code reading environment variables; that boundary belongs to review. Concretely, it checks five things:
src/sparrow_agent_bridge.py, names an owner key tuple, an owner-private name, or the record's"token_env"key. That covers identifiers, imports, attributes and strings. Every string the guard detects is folded by the one helper,_constant, so concatenations, f-strings of constants,%and.formatcount. Call-shape checks still require real literals.write_bridge_declaration,remove_bridge_declaration,bridge_declaration_fieldsandbridge_token_environare reachable only as<alias>.<name>(...).<alias>must be the name bound by the module's single top-level import of the owner module, bound once and never rebound. A same-named local function or method, an aliased reference, agetattr, a second import under that name, or a parameter shadowing it is flagged. The reader's two accessors are the reader's alone.DECLARATION, from a string folding toworker.json, from a name or module global bound from one, or from a function or method that returns one. Reachability follows the path everywhere, including inside f-strings; only what an owner call returns is opaque. A message about the path is exempt only where it is the argument itself: an f-string, a name bound from one, a call to a function returning only those, or anot, comparison orand/or/ifover them. Used any other way (a receiver, a nested call, a read), it is the path, soopen(f"{path}").read()is flagged andprint(f"declared: {path}")is not.**spread.bridge_environmust be exactly the pinned body (docstring aside), merging the owner's mapping whole. The module's mapping writes must be exactly four pinned calls, with__ior__/__or__counted as writes. It may contain no in-place assignment and no|merge. Every reach of a process environment must be one of three pinned uses, so an alias,from os import environorgetattr(os, "environ")is flagged. Any**merge must be of the owner's fields,main's environment must bebridge_environ(decl, token)alone, and no string may be one of the owner's token variable names. Any edit to those lines is an edit to this test, which review sees.The four points
token_env_name,TOKEN_KEYSor thetoken_envfield. That covers the package and the record's one reader.skills/schedule-crons/ensure-cron-room.pyandskills/ag2space-voice/navigate.tsname the same gateway-token variables, but for their own lookup of the host's gateway credentials. They are not different secrets, but neither reads nor writes the bridge declaration. That separate, older duplication is left alone here.'', a reason, or(removed, reason)). It never holds the record or the file's bytes.installproduces the same declaration and printed line as ddb0c8a, and the owner's write, check and remove are run against both owner copies. A rebuild with identical bytes can't be seen in the bytes; the guard catches it instead.Before/after: this round's findings, a5288fc's guard vs this head's
Declaration rows run
declaration_readson the snippet. Reader rows run the reader checks on the realsrc/sparrow_agent_bridge.pywith one edit applied.The last two reader rows were already caught at a5288fc (a direct item delete, and an update outside the pinned calls); they stay as regression pins.
The previous round's table (2c06c88 vs a5288fc) still holds at this head; the earlier-round kill controls below rerun it.
Runtime of the guard suite
Required CI timed out on
tests/gateway-credential-sources.test.py(over 240s under instrumentation) at a5288fc. The profile showed the cost was rescanning: six tests each rancredential_accessover every package module, and the systemd rule reparsed the package eleven times. Each pure scan is now memoized per (source, module), the per-module def listing is cached, and the taint loops stop at their fixpoint. The timeout is unchanged. Wall time locally, both undercoverage run:The heaviest remaining frames under cProfile at this head (the first full-package scan, done once):
Kill controls
This round and the last: each mechanism turned off in this head's guard (then restored; sha256 matched)
Earlier rounds, rerun at this head
Guardistests/gateway-credential-sources.test.py;ConnectHostSupervisedispackages/ag2-sparrow/tests/test_claude_code_adapter.py.Coverage
coverage7.16 was run locally over the credential-sources and both sparrowd suites, and the changed lines were counted againstorigin/main:Suites at this head
Only
tests/gateway-credential-sources.test.pychanged since a5288fc. Withenv -i HOME=$(mktemp -d) PATH=/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin nice -n 19 python3 <file>, started at a 1-minute load under 4:The adapter (239/239), sparrowd rescan (32/32), local bridge (19/19) and wrapper (PASS) suites, and the byte-identical declaration check, last ran at a5288fc. Their code is unchanged since. Changed-line coverage of
src/is unchanged too (owner 56/56, sparrowd 6/6).git diff --checkis clean.scripts/review-checks.sh --diffover the PR diff returnsPARTIAL: the hardcoded-path and root-artifact checks are clean, but prose-cap was skipped because the script was not run from inside the checkout. A manual scan of the added lines finds no comment run longer than 2 lines.2026-10-23: every host token name is stripped, not only the first
An independent review at da4e1ae (also recorded as the comment on #5820) found that
bridge_token_environreturns onlyTOKEN_KEYS[0], sobridge_environstrippedREMOTE_TASK_TOKENalone: a host holdingAG2_REMOTE_TOKEN, orREMOTE_MEDIA_HS_TOKEN(MEDIA_TOKEN_KEY), passed each to the agent bridge, wherediscover()reads either. Main had the same gap; this PR owns the credential name set, so it is fixed here, in the owner.bridge_strip_keys()insrc/gateway_credential_sources.pyreturnsTOKEN_KEYS + (MEDIA_TOKEN_KEY,):REMOTE_TASK_TOKEN,AG2_REMOTE_TOKEN,REMOTE_MEDIA_HS_TOKEN. Vendored byte-identical (sync_from_src.py --checkin sync).bridge_environfilters sparrowd's environment onHOST_ONLYand that set, then mergesbridge_token_environ(token)whole as before. No adapter changed.bridge_strip_keysjoinsREADER_ONLY(named only by the reader, only as a call on the owner's import); the pinnedbridge_environbody is the new one; three reader mutants that narrow the set again (strip = token_env,HOST_ONLY + tuple(token_env),bridge_strip_keys()[:1]) are flagged; the owner behaviour test asserts the tuple, that it equalsset(TOKEN_KEYS) | {MEDIA_TOKEN_KEY}, and that it covers the name the agent's token goes under.tests/sparrowd-agent-bridges.test.py, classWrapper, synthetic values):bridge.mainruns the bridge with a host value under all three names; one test per key asserts the host'sREMOTE_TASK_TOKENis replaced by the agent's, andAG2_REMOTE_TOKENandREMOTE_MEDIA_HS_TOKENare absent from the environment the bridge module runs under (key and value). A fourth drivesbridge_environdirectly overbridge_strip_keys()so a name added to the owner's set later is covered without editing the test.The
AG2_REMOTE_TOKENitem listed in the comment on #5820 is fixed here; the issue's two guard-completeness gaps (shutil.copyfile, aliasedenviron.update) remain its own.Before/after, the new sparrowd tests with
src/(owner, reader and the vendored owner) checked out at da4e1ae and then at this head,env -i … nice -n 19 python3 <file>:The branch was rebased onto main (7940f25) in the same push; the rebase applied clean.
Same day, second finding from the same reviewer (7529fd4): the set stopped at
TOKEN_KEYS+ the media token, so a host'sGATEWAY_TOKENorRELAY_TOKENstill passed through. The bridge reads neither, but room-ops and bee-actions rank both aboveREMOTE_TASK_TOKEN, so a child of the bridge (theREMOTE_TASK_ON_TASKwake, say) would act as the host.bridge_strip_keys()is now the union ofTOKEN_KEYS, the contract'sTOKEN_ALIAS_PRECEDENCE(imported fromgateway_credentials, the owner's existingcontractimport; not copied) andMEDIA_TOKEN_KEY, each once:REMOTE_TASK_TOKEN,AG2_REMOTE_TOKEN,GATEWAY_TOKEN,RELAY_TOKEN,REMOTE_MEDIA_HS_TOKEN.The guard asserts that tuple and its equality with the three sources; the sparrowd suite gains a per-key test for
GATEWAY_TOKENandRELAY_TOKEN(synthetic host values); the table test overbridge_strip_keys()is kept. Before/after, with the owner (src and vendored) at 7529fd4 and then at this head:Same day, third finding (36beb75, P1, traced through the launchd job): sparrowd's wrapper left the host's
CLAUDE_CONFIG_DIRin the bridge child (it must:channels/<channel>/access.jsonis resolved from it), and at import the child's_channel_env_candidates()opened$CLAUDE_CONFIG_DIR/channels/ag2space/.env, sodiscover()registered the host's file token in_SECRETSbefore precedence chose the agent's. The config fallback had the same path:_config_from_channel_env("REMOTE_PROACTIVE_TRUST_OK")opens that same file, so "the child never opens channels/.env" and "the child keeps the file fallback" could not both hold.Design: discovery is gated in the owner, and the one host setting the child took from the file is resolved by the parent and handed in as a value.
bridge_token_environ(token)now also sets a mark,INJECTED_KEY(GATEWAY_CREDENTIALS_INJECTED=1);credentials_injected(env)reads it;env_files()returns an emptyEnvFilesunder the mark without calling the reader. Every channel-.env reach in the bridge (_channel_env_candidates, hence_config_from_channel_env,_token_from_ag2space_envanddiscover's file tier) goes throughenv_files, so under an injected token the child has no path to the file.BRIDGE_CARRIED_CONFIG = ("REMOTE_PROACTIVE_TRUST_OK",)is the audited set of host config a supervisor carries;bridge_carried_config(env, read_text)resolves each from the host's environment, else its channel files through the caller's bounded reader, and returns only those values (a key set nowhere is absent; the files' credentials never leave the call).bridge_environbinds the host mapping once, filters it as before, merges the derived env, then_sources.bridge_carried_config(host, read_regular), then the token environ (mark included).CLAUDE_CONFIG_DIRstays. The wrapper runs as the host, so it is the one allowed to read the host's file.READER_ENVIRONandREADER_WRITESpin the new body;bridge_carried_configjoinsREADER_ONLYandOWNER_READERS(its reader argument is checked likeenv_files'); mutants that resolve the setting in the reader, drop the carried merge, or strip the mark are flagged; an owner test proves an injected env opens nothing, registers nothing and resolves nothing itself, while the host resolves the setting (env wins, file else, absent when set nowhere).tests/sparrowd-agent-bridges.test.py,HostCredentialFiles): a fakeCLAUDE_CONFIG_DIRwhosechannels/ag2space/.envholds a canary token andREMOTE_PROACTIVE_TRUST_OK=1, plus anaccess.json;builtins.open,read_regularandread_regular_textare wrapped to record every path;bridge.mainruns the shipped wrapper with an injected agent token and the bridge module imported where the child would run. Asserts: zero opens of the.envby the child (the wrapper's own open is recorded separately and expected), the canary nowhere in_SECRETS, the child's environment or its stderr,TOKENis the agent's,_ag2space_access_path()is the fakeaccess.jsonand_load_tier_map()opened it,PROACTIVE_TRUST_OKis True from the carried value; a second case sets the flag0in the host environment and shows neither process opens the file.Before/after (owner, vendored owner and reader at 36beb75 with the new tests, then at this head),
env -i … nice -n 19, under the shared test lock:After review (6dcb862, 6caff4d, 0da379e, dd5b0d5, 989fa2b, 8804cf5, 49d5005, 63404c8, 43d1c5a, 97e312b)
6dcb862 (tests only).
tests/sparrowd-agent-bridges.test.pyno longer copiesos.environ, which the hermetic structural gate refuses. It records whatbridge_environreturns and checks key by key that the bridge runs under exactly that.6caff4d.
src/channel_token.py's file and vault readers return''without reading in a process tree that carries the injected mark. Room ops' channel.envand vault tiers andscripts/channel-env.shall go through them, so a declared bridge's wake child cannot reload the host's credential.bridge_environalso dropsBRIDGE_CARRIED_CONFIGkeys from the generic base, so a carried key comes only frombridge_carried_config. Newtests/declared-bridge-wake-child-credentials.test.pyis a real-subprocess sentinel from the wrapper through_fire_on_taskto room ops, with an unmarked control.0da379e. The
shared/path line inchannel_token.pycarries the workspace-resolution lint's line pragmaallow-repo-root: it walks to the repo root, not a workspace.dd5b0d5. A declared agent's wake child acts as the agent.
gateway_credential_sources.wake_environ(the owner) drops every gateway token and URL alias from the wake environment under the mark, then sets the agent's own token as the bridge holds it, plus its gateway URL. The host's credential never reaches it. Room ops there resolves exactly that env token and fails closed without one. The host's own unmarked bridge's wake child is unchanged and still gets no credential.989fa2b. A declared bridge's endpoint comes only from the agent's token.
bridge_strip_keys()also lists every gateway URL name and alias, sobridge_environdrops an inheritedREMOTE_TASK_URL,AG2_REMOTE_URL,GATEWAY_URLorRELAY_URL. Discovery would otherwise rank it above the token's own URL and send the agent's bearer to the host's gateway. The wake child gets the same agent URL. The sentinel covers all four names.8804cf5 and 49d5005. A declared agent's gateway comes from its token, or from a vault key the owner writes. A URL inside the agent's token (
AG2_RELAY_TOKEN_<worker>, the combinedhttps://<gateway>|<secret>form) wins. For a bare secret,sparrow_agent_bridge.endpoint()readsAG2_RELAY_URL_<worker>, a vault key derived from the segment like the token key.bridge_token_environ(token, url)setsREMOTE_TASK_URLbeside a bare token only, andgateway_url_problemrefuses anything but an http(s) URL with a host. Install prints that key'svault setline. The managed-mode README anddocs/remote-gateway-protocol.mdstate the rule.63404c8, 43d1c5a, 97e312b. Token and URL shapes.
https://gw|) is refused by the wrapper and the preflight (token_secret_empty()).AG2_RELAY_URL_<worker>. Only with no gateway do the wrapper and the declared bridge's FATAL name both remedies: re-store a combined token that lost its|, or set the URL key. The host's own bridge's lost-separator hint uses the same owner,token_url_shaped().gateway_url_problem()parses the URL and requires a hostname and a valid port, for the vault URL and for a combined token's embedded gateway alike._redact_url(), so a well-formed vault URL's userinfo, query and fragment are not logged. A URL_redact_url()cannot parse is still printed raw in the rotation-warning path; making it fail closed is tracked in persisted routing record stores the gateway URL raw (userinfo) #5845.Fail-closed consequence and transition. A declared agent never takes its endpoint from the host's environment:
REMOTE_TASK_URL,AG2_REMOTE_URL,GATEWAY_URLandRELAY_URLare all stripped. So a declared agent whose vault token is a bare secret, with noAG2_RELAY_URL_<worker>, does not start. Before this PR it would have sent its bearer to the host's gateway. It now fails closed: the wrapper names the one fix,vault set AG2_RELAY_URL_<worker> <the agent's gateway URL>(or store the combined token), and the bridge's FATAL for a declared bridge names the same vault keys instead of the strippedREMOTE_TASK_URL. On a host with declared agents, runpython3 src/sparrow_agent_bridge.py --preflightafter pulling this change and before restarting sparrowd: the flag ships with this change, so it does not exist on the old checkout. It prints one line per declared agent,ok(gateway from its token, or from its URL key) orWILL NOT STARTwith thatvault setline, and never prints a value. Installs whose token AG2 Space issued (the combined form) need nothing.Why the URL is not in the declaration.
worker.jsonsits beside the agent's root and is writable by the agent's own session, which runs as the same user, so the declaration chooses nothing: every field the bridge uses is derived from its directory's segment and checked against the file. A session-chosen URL there would let that session point its own bridge at an endpoint it controls. The wrapper stamps every task it queues with the host workspace's task-envelope key, so the session could feed itself owner-tier tasks carrying a valid stamp. The URL therefore lives in the vault, under a key derived like the token's, and a URL field written intoworker.jsonis never read (pinned bytest_a_url_written_into_the_declaration_is_never_used).What the vault does and does not enforce.
AG2_RELAY_URL_<worker>is owner-written by convention: install prints thevault setline for the owner, and nothing in the bridge writes it. It is not enforced. The vault is the user's login Keychain, and the agent's session runs as the same user, so it could write that key withsecurity add-generic-password, just as it could rewrite the agent's own token keyAG2_RELAY_TOKEN_<worker>. Both exposures predate this PR: the token key has had this property since declared bridges shipped. What this PR changes is that the declaration file, which the session writes in the ordinary course of its work, no longer carries anything the bridge acts on. The residual risk is a same-user process deliberately writing the owner's Keychain entry for its own agent. That is no worse than the token key itself, and it does not reach the host's credentials, which stay stripped from the bridge and closed to its process tree.Every channel token is closed in a declared bridge's tree, not only the gateway's. Under
GATEWAY_CREDENTIALS_INJECTED,channel_token.token_from_env_fileandchannel_token.token_from_vault(src/channel_token.pyL123 and L148 at 97e312b) return''for every variable, not just the gateway's. So the host's Slack, Telegram and Discord tokens are also unreadable from a declared bridge and its wake child, through.envfiles and the vault alike. This is intended: a declared agent's tree reads none of the host's credential files or vault entries. The environment tier still reads. Apart from the stripped gateway token and URL names, a declared bridge inherits sparrowd's environment, so a host channel token exported into sparrowd's own environment would still be visible there. The closed tiers are the.envfiles and the vault.Release notes
Declared agent bridges under sparrowd: endpoint source changed. After this change, a declared agent never takes its gateway URL from the host's environment. Its URL comes from its vault token (the combined
https://<gateway>|<secret>form AG2 Space issues) or from the new owner-written vault keyAG2_RELAY_URL_<worker>.AG2_RELAY_URL_<worker>, stops starting after sparrowd restarts.python3 src/sparrow_agent_bridge.py --preflight. For every agent it marksWILL NOT START, run thevault set AG2_RELAY_URL_<worker> <the agent's gateway URL>line it prints, or store the combined token instead..envfiles or vault.Live post-restart witness (REVIEW lesson 15)
Run at this frozen head: #5815 (comment)
REMOTE_TASK_TOKENand marked injected. No host token or URL name reached it.deliveredlog line)..env.install/uninstalllaunchd and tmux parts were not run; this PR does not change them.Follow-ups
#5845 tracks three code items left out of this frozen head:
_declaration_owned()reading an existing declaration with no size cap (the reader caps at 64 KiB)preflight()raising a traceback whenagents/cannot be listed🤖 Generated with Claude Code