Repository navigation
fix(gateway): a held serve's saved copy is redacted before it is written - #6135
Conversation
_save_reserve kept the raw broker task, so a token or vault set value in its body, wake answer or room question was written to relay-reserves before any redaction. The copy is now redacted (vault grammar, then the signature scanner; nothing stored; media markers and ids kept) and marked, so its later render does not intercept the placeholder again. A broker field never carries the mark.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.pyLines 6336-6344 6336 digest = _digest(body)
6337 text = json.dumps({"format": _RESERVE_FORMAT, "owner": {"task": wire_tid, "candidate_run": run, "lease": lease},
6338 "sha256": digest, "cleaned": cleaned, "task": body}, sort_keys=True)
6339 if task_attempts.write_if_absent(path, text, mode=0o600) is None:
! 6340 return kept()
6341 except (OSError, ValueError, _ReserveLost) as e:
6342 _log_backoff((wire_tid, "reserve-unsaved"), f"task {wire_tid}: its held serve could not be saved "
6343 f"({diagnostics.describe(e)})")
6344 return NoneLines 6410-6419 6410 return True
6411 except _ReserveLost:
6412 new = _save_reserve(t.wire, run, lease, task)
6413 return new is not None and bool(t.put(pending_serve={**pending, "reserve": new}))
! 6414 except OSError:
! 6415 return False
6416
6417
6418 def _hold_reserve(t, first: str, task: dict, lease: "str | None") -> "tuple[str, None, bool] | None":
6419 """A new lease for a run that may already be executing: the run keeps its generation and nothing |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Agent-assisted review by Sutando, an AI agent working for this account, at 438a39a. Requesting changes for one leak path. The single-writer shape is right: _save_reserve is the only writer of relay-reserves*/v1-*.json, and redaction runs before the write, with nothing written if it raises.
Blocking
-
Text inside a media-marker-shaped bracket is never redacted (
remote_gateway_bridge.py~6299–6303,_cleaned_for_reserve). The loop keeps every[remote-media: …]match whole, but only the first one is fetched, and anyone can type that bracket. Probe: a real marker followed by[remote-media: ghp_… vault set K hunter2secret]ends with both the token and the vault value in the saved copy. The task file stays clean, becausefilter_chat_secretsruns over the whole body there; only the saved copy leaks. Fix: keep only the first marker's URL/attributes and redact the rest, or redact the marker's attribute text too. -
The cleaning policy now exists twice.
_clean_text/_clean_strings(~6267–6292) repeatclean()inside_redacted_question(~5035), and they already differ in four ways:- the vendored grammar vs
_vault_intercept_fns()[1] or _local_redact_vault_set; - dict keys cleaned vs not;
- an exempt-id list vs none;
- inline
question_id/expected_revisionvs_QUESTION_IDENTITY.
One shared cleaner, please.
- the vendored grammar vs
Should fix
3. test_a_broker_field_never_marks_a_fresh_task_as_already_redacted doesn't discriminate. It still passes with the task.pop(RESERVE_CLEANED, None) at ~5944 disabled. It only sends a token, and filter_chat_secrets runs regardless. What the pop actually protects is a vault set value: with the mark present, vault redaction is skipped. Send vault set K <value> with a forged reserve_cleaned: [].
4. The mark is trusted from the body (~6044). A copy saved by #6108's code, or an old task-wide copy, holds the raw broker task, so a broker-sent reserve_cleaned there would skip vault redaction on publish. Binding trust to the envelope (e.g. a format bump) closes it.
Low
5. _identifier (~6288) exempts every *_id key plus answered_by/outcome/kind, at any depth, and dict keys aren't cleaned. A probe kept a token under answer.form_id and as a dict key. Neither is user text today.
Checked:
- With flags off,
_save_reserveisn't reached; the only change is that a brokerreserve_cleanedis dropped. - With V2 on and turn context off, there's no regression of the #6128 kind. The V2-only case has no direct test.
- CHANGELOG-PENDING has the entry, and review-checks pass.
tests/gateway-agent-activity-hooks.test.py: 228 OK, both with and without the lifecycle flags.- Revert check: saving the raw task fails the redaction test.
- CI is green.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head 438a39a3b8ff33ab5e54cbe963141bfa728adfe6. This adds a concrete real-path failure to the standing cleaning-policy blocker; the PR is not merge-ready.
Blocking: the real room-question path is not idempotent.
_write_task first cleans room_question at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:5948-5951, then _cleaned_for_reserve cleans it again at :6305-6312, and owed publication calls _write_task for a third pass. At this head:
raw option: vault set K hunter2secret
first _redacted_question: vault set K [vault: non-owner tier — ignored]
saved reserve: vault set K [redacted before saving] non-owner tier — ignored]
publish prepass: vault set K [vault: non-owner tier — ignored] before saving] non-owner tier — ignored]
The first pass detects GitHub Token and vault-set command; the saved reserve_cleaned and publish pass retain only vault-set command. Thus the restarted run gets corrupted quoted question text and an incomplete secret notice. tests/gateway-agent-activity-hooks.test.py:236-261 checks only that raw secrets are absent and the answer rule remains, so it stays green while content is mangled. The new direct _cleaned_for_reserve test bypasses the first production prepass. Please use one idempotent cleaning/metadata owner and pin the exact held-save/restart round trip, including unchanged sanitized text and all detected types.
The existing media-marker blocker is independently reproduced. Lines 6299-6304 preserve every marker-shaped bracket, although runtime fetch consumes only the first. A real marker followed by [ag2space-media: ghp_… vault set K hunter2secret] leaves both secrets in the saved reserve. This defeats the security purpose for sync/backup-visible state.
Live gate: the body explicitly says the live round trip was not run. This changes bridge persistence and restart publication, so REVIEW criterion 15 requires an exact-head post-restart held re-serve before approval; deferring that witness to #6124 after merge is not sufficient.
Verified: the ordinary-body leak exists at merge-base aa42eb682 and is fixed here; hooks 228/228, lifecycle 9/9, attempts 90/90, agent-turn 54/54, and the bridge suite pass. All three new tests were collected; against merge-base source they produce one failure and two expected missing-helper errors. Review checks, positive control, diff check, CI (45 success/3 expected skips), CLA, and 100% diff coverage are clean.
Worst case is flag-limited but material: an opted-in host syncs raw secrets from marker text, then publishes a mangled held question after restart. Fix the two code blockers, make the forged-mark test discriminate with a low-entropy vault set value, attach the live witness, and re-request review.
…the mark the envelope's A typed bracket shaped like the media marker was kept whole in the saved copy, so text inside it was never redacted: the whole body is redacted now, and a marker with nothing secret in it is unchanged. The question and the saved copy share one redactor, dict keys and ids included. The redacted mark moves from the task into the envelope (format 2), so an older copy or a broker field never carries it.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 438a39a3b8ff33ab5e54cbe963141bfa728adfe6. Changes requested. The head has not moved since the two CHANGES_REQUESTED at this sha (sonichi, keweichen), and I confirmed the two code blockers against the source.
Blocking: incomplete fix. Marker-shaped text is saved unredacted.
MEDIA_MARKER_REis\[<tag>:([^\]]*)\](remote_gateway_bridge.py:2843), so it matches any bracketed text after the tag._cleaned_for_reserve(:6299-6303) keeps every match whole. Only the first marker is fetched, and anyone can type the bracket.- So
[<tag>: ghp_… vault set K value]in a body reachesrelay-reserves*/v1-*.jsonraw. That is the leak this PR exists to close. - Ask: keep only the real (first) marker, or redact marker attribute text too. Add a test with a forged second marker.
Blocking: fragmentation. The cleaning policy now exists twice and already differs.
_clean_text/_clean_strings(:6267-6292) re-implementclean()inside_redacted_question(:5035-5060). They differ on:- the vault redactor: vendored grammar vs
_vault_intercept_fns()[1] or _local_redact_vault_set; - dict keys: not cleaned vs cleaned;
- identifier exemptions:
_identifiervs_QUESTION_IDENTITY.
- the vault redactor: vendored grammar vs
- keweichen's repro shows the two compose non-idempotently on the real
_write_task→ save → publish path. The question text is mangled ([redacted before saving] non-owner tier — ignored]) and the GitHub-token type drops out of the notice. - Ask: one shared cleaner owns both paths, plus a held-save → restart → publish round-trip test that asserts the sanitized text is unchanged and every detected type survives.
Should fix:
test_a_broker_field_never_marks_a_fresh_task_as_already_redacteddoesn't discriminate. A token is redacted byfilter_chat_secretsregardless, so the test passes with thepopremoved. Use a forgedreserve_cleaned: []with avault set K <low-entropy value>.
Named, not blocking:
- No live held re-serve yet. The body defers it to #6124.
Verified:
_save_reserveis the only writer, and redaction runs before the write.- The flags-off path is unchanged.
- CI is green,
license/clais success, and there are 0 unresolved threads. - Hardcoded-path scan: none.
— Sutando (susans-MacBook-Pro)
…eps what it found The question was redacted as the task came in, again when its held serve was saved, and again when that copy was published: the vault grammar re-read its own placeholder as a value, and the later passes lost the types the first found. It is now redacted once; the saved copy keeps that text and carries its types in the envelope's cleaned, and a redacted copy's question is not redacted again.
|
sonichi's and keweichen's reviews at
Against the bridge at At this head, with Full Python list and package publish gate: no new failures. #6092's merged question path behaves the same: it uses the same vault redactor and scanner as before, now through the shared function. keweichen: the question redacted once. Your schedule (intake, then save, then publish, each redacting again) is gone. The question is redacted once, at the top of
Live (REVIEW 15): the coordinator runs a held re-serve on the rig at this head, with a crafted body, before approval. The bridge is restarted before the ack lands. Checked: no value in |
|
Live witness for #6135 at exact head Body sent: Sender: the rig's second agent, not the owner, because an owner-tier
Not covered live:
Reserve, task and notice detailRaw run |
|
liususan091219: your review at |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head b3dde43ee5947a33faee43e8c0411896882bb2b2.
Changes requested — this head is not merge-ready.
Blocking: the new format-2 reserve is not safe across a one-version rollback. This head writes redacted body text plus the trusted cleaned mark outside the body, but keeps the old v1-* reserve reference/name. The immediate parent reader accepts that envelope, ignores the format-2 mark, and sends the already-redacted owner text through vault interception again.
Cross-version new-writer → old-reader repro:
new_writer_saved= vault set API_KEY [vault: non-owner tier — ignored]
old_reader_format2_mark= None
old_owner_intercept_calls= ['vault set API_KEY [vault: non-owner tier — ignored]']
The real old interceptor turns that into a second, mangled “NOT STORED … resend” instruction. This violates the single-interception contract after an ordinary rollback and can mislead an owner about whether a value was stored. Please make new envelopes fail closed to old readers (for example, a versioned reference/name the old v1-* grammar rejects), or provide another rollback-safe representation. Add a new-writer → old-reader regression and name the rollback worst case in the PR's Risks section.
The prior findings are otherwise fixed: the original parent leak reproduces, the four named regressions and full 229-test hooks suite pass, the bridge suite and review checks pass, and exact-head CI/CLA/diff coverage are green. The cited exact-head real-broker run satisfies the post-restart live-path gate, with its owner-tier/question/mid-flight limits properly disclosed.
Fix the rolling-upgrade/rollback contract and this is ready for another review.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Agent-assisted re-review by Sutando, an AI agent working for this account, at b3dde43 (fixes in 7911c3e, cd8e7d9). Approving; this replaces my changes-requested at 438a39a.
- Typed bracket kept raw. The whole body is now redacted, markers included (
_cleaned_for_reserve,remote_gateway_bridge.py:6273-6281). Checked with detect-secrets loaded: real-looking markers survivefilter_chat_secretsunchanged (relay hex and UUID ids, Matrix 24-char ids, a signed URL). - Duplicated cleaner. There's one redactor,
_redact_quoted(:5035), and_redacted_questionuses it (:5058)._clean_text,_clean_strings,_identifierand the placeholder are gone. - Forge test. It now uses a
vault setvalue with a forgedreserve_cleaned: []. - Mark trusted from the body. Envelope format 2 carries
cleaned._read_reservestrips any body mark and restores it only from a format-2 envelope (:6330,:6340-6342). The broker's copy is dropped on a fresh serve (:5945-5946). Format-1 envelopes on main are unredacted, so full vault handling of them is correct. _identifierexemption is removed, and dict keys are redacted too.- The question is redacted once at intake (
:5950-5954)._hold_reserveruns after that, inside_write_task.
- Revert checks. Each of five reverts fails a new test: keeping markers whole; trusting the body mark in
_read_reserve; not popping the broker mark; redacting the question again on publish; ignoring the intake findings. - Tests:
tests/gateway-agent-activity-hooks.test.py229 OK. CI is green.
Nit. A format-2 copy published after REMOTE_TASK_TURN_CONTEXT is turned off keeps its (already redacted) question rather than dropping it to None.
The live witness (6568156434) is non-owner tier only, so the owner-tier "not intercepted twice" path and restart-before-ack aren't witnessed live; a unit test covers the restart.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at b3dde43ee5947a33faee43e8c0411896882bb2b2 (fixes in 7911c3ed4, cd8e7d993). My change request at 438a39a3b is resolved. I'm not approving yet, because keweichen's rollback blocker at this head holds up against the code.
My findings, checked against the code:
- Marker-shaped text.
_cleaned_for_reserve(remote_gateway_bridge.py:6273-6281) now redacts the whole body, markers included, so a forged second marker no longer survives. - One cleaner.
_redact_quoted(:5035) is now the only redactor for quoted data, and_redacted_questioncalls it._clean_text,_clean_stringsand_identifierare gone. The question is redacted once, at intake (:5946-5954). - Forge test.
test_a_broker_field_never_marks_a_task_as_already_redactednow discriminates. With thetask.pop(RESERVE_CLEANED, None)disabled it fails, andhunter2secretshows up in the task file. - Tests at this head. The hooks suite passes 229/229, and so do the envelope golden, durable-ack and bridge tests. With the bridge put back to
438a39a3b, the reworked tests fail (3 failures, 1 error), including the leakedghp_…. With it put back to7911c3ed4, the restart round-trip test fails.
Still open: keweichen's rollback finding (confirmed by reading). The merge-base _read_reserve checks the owner, sha256 and the v1-* id, but never format or cleaned. So after a one-version rollback it would accept a format-2 copy as unredacted, and send owner text that is already redacted through vault interception a second time. I did not run the old-reader harness myself.
What's missing for my approval: make new envelopes fail closed for an older reader, or show a rollback-safe representation, and add a new-writer → old-reader regression. sonichi's approval is at this head and the held re-serve witness (6568156434) is exact-head, so the bar is otherwise met. Owner tier and restart-before-ack are named as not witnessed live; that is not blocking.
CI is green (44 success, 3 skipped). license/cla is success, with 1 context.
— Sutando (susans-MacBook-Pro)
…e refuses it An earlier bridge read the format-2 copy under its v1- name, ignored its redaction record and intercepted the redacted body again. A redacted copy is now v2-<hash>.json: the earlier reader's id grammar refuses it and fails that generation, and its sweep leaves it. A v2 copy without its redaction record is lost, never read raw; v1 copies publish as before.
|
keweichen's review at A redacted copy fails closed for an earlier bridge.
New-writer → old-reader, run against Tests at this head:
Risks (added to the body): after a rollback to an earlier bridge, a generation whose held serve was saved as |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 43c1a24c8d975d972f78898ecd4c14429898f913 (fix in 43c1a24c8). Changes requested: one new upgrade-window regression from the v2- rename, with a failing test below. Everything raised earlier is fixed.
Earlier points, checked at this head
- Mine at
438a39a3b(marker-shaped text, duplicated cleaner, non-discriminating forge test): resolved, as in myb3dde43eereview. Unchanged by this commit. - keweichen's rollback blocker at
b3dde43ee: fixed. A redacted copy is nowv2-<hash>(remote_gateway_bridge.py:6317-6319). The earlier reader'sv1-[0-9a-f]{32}grammar refuses its reference, and the earlier sweep skips it. Av2-copy without format 2 andcleanedis_ReserveLost(:6342-6345).
Blocking: a held serve saved as v1- by the earlier bridge is never accepted again under the same lease
_hold_reserve's same-lease branch (:6375) checks_save_reserve(...) == pending["reserve"]._save_reservenow always names its copyv2-(:6291, defaultversion=2). So on a ledger whose pending reserve is av1-ref written before the upgrade, the ids never match.- Result:
_write_taskreturnsNoneand logs "its held serve's saved copy is not this serve — left unacked for a re-serve", on every same-lease redelivery, until the broker re-serves under a new lease. It also writes av2-copy that no record names. - At
b3dde43eethis case worked: the name stayedv1-,write_if_absentfound the existing copy, and the ref with its sha was returned. So this is a regression from this commit, in exactly the rolling-upgrade window (REVIEW lesson 5). The body says "v1 copies publish as before", and that's true for the ack path but not for this redelivery path. - Repro, added to the
Hooksclass intests/gateway-agent-activity-hooks.test.py. It fails at this head and passes with the bridge atb3dde43ee(control):
def test_a_same_lease_redelivery_of_an_earlier_bridges_v1_held_serve_is_accepted(self):
tid = "task-probe-v1-upgrade"
self.addCleanup(self.forget, tid)
self.served_acked(tid)
task = _task(tid, lease_id="L2", source_event_id="$e", task="hello")
self.assertEqual(gw._write_task(task), (tid, True))
pend = dict(gw._ATTEMPTS.get(tid)["pending_serve"])
run, new_ref = pend["run"], pend["reserve"]
body = {**task, "id": tid} # what an earlier bridge saved: a raw format-1 v1- copy
env = {"format": 1, "owner": {"task": tid, "candidate_run": run, "lease": "L2"},
"sha256": gw._digest(body), "task": body}
v1 = gw._reserve_id(tid, run, "L2", 1)
(gw._reserve_dir() / f"{v1}.json").write_text(json.dumps(env, sort_keys=True))
(gw._reserve_dir() / f"{new_ref['id']}.json").unlink()
with gw._ATTEMPTS.transition(tid) as t:
self.assertTrue(t.put(pending_serve={**pend, "reserve": {"id": v1, "run": run, "lease": "L2",
"sha256": env["sha256"]}}))
self.assertEqual(gw._write_task(task), (tid, True)) # head: None- Fix suggestion: in the same-lease branch, when
pending["reserve"]is av1-ref, verify it with_read_reserve(or save under version 1) instead of comparing it to a freshv2-save. That keeps a rawv1-copy on full vault handling, as now. Add the test above.
Worst case: on an opted-in host that upgrades while a held serve is pending, its same-lease redeliveries go unacked until a new lease comes. It's a delay, not a leak or a double interception. It is flag-gated (REMOTE_TASK_LIFECYCLE_V2), so default installs are unaffected.
Verified at this head
- With both flags on:
tests/gateway-agent-activity-hooks.test.py230 OK, lifecycle 9 OK, attempts 90 OK, andsrc/remote-gateway-bridge.test.pypasses. license/clais success (1 context). Check runs were still in progress when I submitted (27 success, 3 skipped, 15 in progress, none failed), so CI is not green yet.- The
b3dde43eelive witness doesn't cover thev2-name. Named, not blocking.
Not merge-ready until the same-lease v1- case is fixed and pinned. That one change would make this an approve from me.
— Sutando (susans-MacBook-Pro)
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-approving at e399889. Since 60ab543 there's a main merge, plus 10f046c: an owner's vault command is now stored once per task across a render and its held copy, not once per copy. When the first render handles an owner's vault set, it records vault_handled on the task's attempt. _save_reserve reads that record and, if it's set, only redacts (store=False). When the held copy stores it first, it sets the same record. If the record can't be written, the copy falls back to storing, so a value can be stored twice but is never left raw. The new test runs a render followed by a held copy and checks that there is one store and no plaintext saved. Measured from main, the rest of the diff is unchanged.
The earlier non-blocking note still applies: a failed store is redacted and only logged, so the owner never learns to resend it.
(agent-assisted, Blue for Chi's seat)
|
Post-restart round trip for #6135 at exact head Limits: sender is the rig's second agent (tier Body:
Raw output |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e3998897d3d138061fb65233fe1fce9de1e9420a. Since my approval at f88507b38: a26e2a6e4 (owner vault set handled when a held copy is first saved), 10f046c28 (the double-store fix), and main merges.
Approve.
The double-store bug. After a26e2a6e4, _cleaned_for_reserve always called _vault_handled with storing on. So on an ordinary schedule, where L1 renders (owner vault set stored) and then L2 is held, the command was stored a second time. That happened before the ack chose a generation, and it could overwrite a key the owner had changed since (keweichen at 60ab54396).
The fix is right. 10f046c28 gives the side effect one durable owner: vault_handled on the task's attempt record.
- The render records it after an owner interception changes the text (
remote_gateway_bridge.py:6099-6102via_note_vault_handled, 5064-5074). _save_reservestores only when the record does not say so, and records it when it stores (:6346-6355)._vault_handled(..., store=False)always redacts (:5050), so either branch leaves the copy redacted.Transition.putis a merging leaf commit, andswitch_runspreads the record, so the flag survives a generation switch.- The attempt key is right:
broker_tidin_write_taskis the wire id_serve_attemptuses. - Every failure mode (record not written, a crash between the Keychain write and the note,
write_if_absentfailing after the store) can only repeat a store. None skips the store or leaves plaintext on disk.
Verified in a scratch clone at this head:
- hooks 234, lifecycle 9, task-attempts, durable-ack and
remote-gateway-bridge.test.pyall pass. - With only the bridge half of
10f046c28reverted, both new tests fail. With thevault_handledasserts also removed, the stores are counted:2 != 1 : the held copy does not store it again. The test discriminates on the side effect itself, not only on the new field. - v1-/v2- upgrade still holds: forcing
_same_held_copyback to the plain_save_reserve(...) == refcomparison fails bothlost=False/Truesubcases oftest_a_same_lease_redelivery_of_an_earlier_bridges_v1_held_serve_is_accepted; restored, they pass. - Secrets: every path from the body to the copy is redacted or stored-and-replaced. The record holds only a boolean. The live round trip at this head read the saved copy directly: the placeholder appears once in the copy and once in the task file, with 0 plaintext across
state/,tasks/and the log. - CI: 45 checks pass, 3 skipped (release / Windows jobs that don't apply), coverage ≥95% passes. The commit status endpoint has 1 context (
license/cla: success), not zero.review-checks.shon the PR diff passes.
Non-blocking:
- Owed live run: the owner-path run (an owner
vault setacross render → held re-serve → restart → publish) has not been done live. The owner store and the double-store fix rest on the harness, because the rig has no owner tier. It should be run before or soon after merge, with Mark's OK for a dev app. - Prior note: the earlier note stands. A failed or raising interception is redacted and only logged, and now also recorded as handled, so the owner is never told to resend.
- Edge case: the
"vault-set command" in cleanedgate at:6354would also mark the task handled when only the wake carried the command. That is harmless: nothing is stored either way. - PR body: still owed its update for owner handling and the completed live runs.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head e3998897d3d138061fb65233fe1fce9de1e9420a.
CHANGES REQUESTED — the ordinary live-record L1-render → L2-hold duplicate store is fixed, and I accept the exact-head Team-tier restart witness. The owner-side “once per task” contract is still broken in adjacent supported schedules.
1. Blocking: retirement drops the marker, so a finished-task re-serve overwrites a newer vault value
vault_handled exists only on the active task record. TaskAttempts.finish() copies lease/generation/delivery into run history at task_attempts.py:1329-1334, then removes the task record at :1348. revive_finished() reconstructs only run/lease/generation/finished at :776-797; it cannot restore the marker. This is not a hypothetical unsupported transition: the protocol explicitly requires a new-lease serve after an earlier run has finished to be held for its ack at docs/remote-gateway-protocol.md:457-459.
Exact-head hermetic production-writer probe:
calls_after_render=1 vault=old-value marker=True
record_after_finish={} run_history_keys=[delivered, gen, item, lease, published, seq, task]
external_update=newer-value
held_after_finish=('task-review-finished-vault', True)
calls_after_reserve=2 vault=old-value marker=True
The marker shown last is written only after the second interception. Thus the old task value overwrote the newer credential. The new test at tests/gateway-agent-activity-hooks.test.py:621-639 keeps the active record alive, so it cannot expose this path. Please make the sanitized vault disposition task-durable across finish → revive_finished → hold, and pin that exact sequence.
2. Blocking: the boolean marker loses the owner’s actual store outcome
When the marker is present, _save_reserve() passes store=False. _vault_handled() then uses redact_fn at remote_gateway_bridge.py:5048-5051, but that production helper is specifically the non-owner redactor. The held copy therefore replaces both a successful [STORED-IN-KEYCHAIN] and a failed [VAULT-STORE-FAILED] outcome with [vault: non-owner tier — ignored]. Generation 2 publishes that false disposition unchanged.
Success probe: one interception, but both the saved copy and published owner task read:
task: please vault set K [vault: non-owner tier — ignored]
Failure probe:
first_failure_visible=True
marker_after_failed_store=True
saved_task=please vault set K [vault: non-owner tier — ignored]
published_failure_visible=False
intercept_calls=1
The failure indication designed to tell the owner that storage failed disappears. A task-wide boolean cannot reconstruct per-command stored/failed/refused outcomes; persist/reuse the sanitized disposition, or publish another truthful durable outcome. Pin generation-2 publication for both success and failure.
3. Blocking: the crash window still permits the same stale overwrite
The external store precedes the durable marker on both paths: render at :6099-6101, and reserve handling at :6317 before the copy/marker writes at :6352-6355. Injecting a crash immediately after the render-side store produced:
after_crash_calls=1 vault=old-value marker=None task_file_exists=False
after_restart_L2_calls=2 vault=old-value marker=True
I changed the simulated vault to newer-value between those lines; the L2 reserve overwrote it with the old task value. The PR comment acknowledges repeat storage if the record write fails, but repeat storage is not harmless here—it recreates the stale-credential overwrite that this round is meant to close. Please use a durable pre-side-effect/ambiguous state (with an explicit retry/verify outcome) or an idempotency contract, and pin both render-side and reserve-side crash boundaries.
Worst case: limited to Lifecycle-V2 owner tasks, a delayed re-serve or bridge crash restores an old credential over a newer one; a failed original store can also be re-rendered as a non-owner ignore. Flags-off installs are unaffected.
Validation at this head: the two new named tests pass; attempts 90/90, agent-turn 56/56, the full bridge suite, diff check, and review checks pass. Exact-head CI is green (44 success, 3 skipped), CLA is green, and the head is mergeable. The real-broker witness at comment 6572901476 satisfies the generic restart gate—restart before L2 ack, epoch 184→185, one generation-2 post, and directly inspected redacted v2- bytes—but it correctly discloses that Team tier does not exercise any owner behavior above.
This is not ready to merge. After these owner schedules are fixed and pinned, rerun the required exact-head restart witness and update the PR body, whose live section still stops at the earlier head.
(agent-assisted, Sutando for Kewei's seat)
Storing from a held copy could put an old value over a newer one (after the task finished, or across a crash before its marker), and its redaction replaced a store or failure outcome with a non-owner note. A held copy now never stores: the owner's command is replaced with a note that it was not stored from this re-sent copy and should be resent if the key is not stored. The vault_handled marker goes; only a serve's first render stores.
|
keweichen's review at A held copy never stores an owner's
Against your three findings:
The Pinned:
At At this head, with both flags on: hooks 233 OK; lifecycle 9, attempts 90, durable-ack 39, golden 3, bridge.test all OK. Full Python list: no new failures. Publish gate passes. Live: the coordinator reruns the non-owner held re-serve → restart → publish at this head. The owner path stays harness-only (the rig can't produce owner tier, and a real owner store would write Mark's Keychain). The PR body's owner handling and live sections are updated. |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-approving at 68b0249. Since e399889 there's a main merge plus 2df9480, which simplifies the vault rule: a held copy never stores an owner's vault set. It replaces the value with "[vault: not stored from this re-sent copy; send the command again if the key is not stored]", and anyone else's command is redacted as before. Only the first render of a serve stores, so the vault_handled bookkeeping is gone. That's sound: no plaintext reaches disk, a stale re-sent value can never overwrite a newer key, and the owner is told to resend, which also closes the silent-loss note I left earlier. Measured from main, the rest of the diff is unchanged.
(agent-assisted, Blue for Chi's seat)
|
Post-restart round trip for #6135 at exact head Limits: sender is the rig's second agent (tier Body:
Raw output |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 68b0249dda055b01398ea44466e4c7178efbf52f. Since my approval at e3998897d: 2df948098 (a held copy never stores an owner's vault set) and main merges.
Approve.
The change is right. It removes the second store instead of trying to make it safe, which addresses all three of keweichen's findings at e3998897d (finish → revive → hold, a store/fail outcome rewritten as a non-owner ignore, and the crash windows). It takes the "explicit retry" outcome that review offered.
_cleaned_for_reserve(remote_gateway_bridge.py:6294-6309): for an owner, the body goes throughredact_vault_commands(..., placeholder=_RESENT_VAULT), which only redacts and never stores. Everyone else is redacted as before. Thenfilter_chat_secretsruns.- The vendored grammar (
ag2_sparrow/vault_set_grammar.py) is byte-identical tosrc/vault_set_grammar.py, andsrc/vault_intercept.pyimports itsVAULT_SET_RE. So the redactor matches every command the interceptor would have stored, and no value is left raw. - A v2 copy is published without interception (
:6080-6084), so the note is never fed to the interceptor. vault_handled,_note_vault_handledand thestore/tparameters are gone, with no references left.
The resend note. It says vault set K [vault: not stored from this re-sent copy; send the command again if the key is not stored]. It keeps the key name and drops the value. It goes into the copy and into the generation-2 task file, so the owner hears it through the agent's reply, the same way the stored/failed markers reach them. The copy's cleaned still names vault-set command, so the task file also carries the security notice. That notice tells the agent to ask for vault set if the value wasn't supplied, which agrees with the note. Neither text carries the secret.
Stores. The first render still stores normally through _vault_handled(_fetched, sender_tier) (:6086). I found no path where the value is neither stored nor a resend asked for:
- If the first render ran, it stored.
- If it didn't, every held copy, including one re-saved after
_ReserveLost, carries the note. - A v1 copy from an earlier bridge is published as before and stores at render, so nothing is lost on the upgrade path.
Verified in a scratch clone at this head (both flags on):
- hooks 233, lifecycle 9, durable-ack 39, task-attempts 90 and
remote-gateway-bridge.test.pyall pass. - With
2df948098's bridge half reverted (only_RESENT_VAULTkept so the test runs), all three subtests oftest_a_held_copy_never_stores_an_owners_vault_command_and_tells_them_to_resendfail: no note, and the copy holds[STORED-IN-KEYCHAIN]. - With the note kept but a store added inside the held copy, all three fail on the count:
1 != 0,2 != 1,2 != 1. The test checks the side effect itself, and therendered == 1assert pins the normal first-run store. - v1-/v2- still holds: forcing
_same_held_copyback to the plain_save_reserve(...) == reffails bothlost=False/Truesubcases oftest_a_same_lease_redelivery_of_an_earlier_bridges_v1_held_serve_is_accepted. review-checks.shon the PR diff passes.- CI: 46 pass, 3 skipped. The commit status endpoint has 1 context (
license/cla: success), not zero.
Non-blocking:
- Owed live run: the owner path (an owner
vault setacross render or crash → held re-serve → restart → publish showing the resend note) hasn't been run live. The latest round trip at this head is Team tier, so the note and the no-store rule rest on the harness. It should be run before or soon after merge on a rig with an owner tier and a throwaway Keychain. - PR body: the "On:" bullet in the risk section says the tests haven't found a schedule where the first generation never rendered, but the new "crash before render" subtest is that schedule. And the live section stops at
e3998897d. Worth updating both.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 68b0249dda055b01398ea44466e4c7178efbf52f.
CHANGES REQUESTED — the new owner resend-note policy fixes the three previously reported held-copy schedules, and I accept the exact-head Team-tier restart witness. The stated one-version rollback path still recreates the raw reserve and can repeat the owner store.
Blocking: v2 → previous-bridge rollback writes and adopts a plaintext v1 copy before it can fail closed
I ran a shared-state cross-version probe with this head as the writer and its exact base 558e221b6782cd281d1be1e7a44e15bb742d63b8 as the rollback reader. The first process rendered/stored L1 once and saved the fresh L2 hold as v2:
HEAD L2: ref=v2-f026… raw_in_copy=false resend_note=true
After restarting the exact previous bridge, I re-served the task under L3—the fresh lease required by the protocol. It did not fail the generation. It wrote the broker body into a new v1 reserve and replaced the pending v2 reference:
BASE L3: result=(task, true) intercepts=0
raw_counts={v1-0c816…: 1, v2-f026…: 0}
pending_lease=L3 pending_ref=v1-0c816…
Once L3's generation-2 ack was accepted, owed publication read that raw v1 and invoked the owner interceptor again (stores=1 in the rollback process), after L1 had already stored once at HEAD. Thus rollback both persists the credential in sync/backup-visible state and can restore the old task value over a newer credential.
The cause is in the previous reader's production path: _hold_reserve() calls its raw _save_reserve() before comparing references on the same lease, and on a fresh lease installs that newly written v1 reference outright. Its _save_reserve() serializes the broker task verbatim before write_if_absent(). The current test at tests/gateway-agent-activity-hooks.test.py:541-543 only proves that a v2- name does not match the old v1- regex; it never executes the old writer against new-format state.
This contradicts both docs/remote-gateway-protocol.md:454 and the PR's Risks claim that an earlier bridge refuses the v2 generation. Please make downgrade fail before the prior writer can persist or replace any reserve (for example through a downgrade/startup fence or another representation the previous binary fails closed on), then pin current writer → exact previous reader → fresh-lease re-serve and assert: no raw v1, the v2 reference is not replaced, and no second interception.
Worst case: limited to Lifecycle-V2 hosts that roll back one version, but material: a credential the new bridge kept out of durable state is written there by the rollback and then stored a second time, potentially overwriting a newer value.
The latest held-copy test otherwise covers the requested resend-note outcome, and the exact-head real-broker witness at comment 6573977799 satisfies the generic restart gate with its owner-tier limitation correctly disclosed. Exact-head validation passes: hooks 233/233, lifecycle 9/9, attempts 90/90, durable-ack 39/39, agent-turn 56/56, golden 3/3, full bridge suite, git diff --check, review checks, CI and CLA. Please also refresh the stale test counts and rollback/risk prose in the PR body with the fix.
This is not ready to merge.
(agent-assisted, Sutando for Kewei's seat)
… instead of saving a raw copy On a rollback, the previous bridge read the format-4 ledger, saved the broker task raw as a v1- held copy on a fresh lease, replaced the v2- reference and later re-intercepted the owner's vault set. The ledger now becomes format 5 on its first write: a format-4 reader refuses it and holds every task, writing nothing, until the ledger is set aside; formats 3 and 4 are read and upgraded.
|
keweichen's review at The previous bridge now fails before it can persist or replace anything. The attempt ledger becomes format 5 on its first write (
Scope: the ledger exists only with Current writer → exact previous reader → fresh-lease re-serve, run as a real cross-version probe:
Before the fix (this head with After: So there's no raw In-repo pin:
The CHANGELOG and the protocol doc state the one-way step. The earlier At this head, with both flags on: hooks 233 OK; attempts 90, lifecycle 9, agent-turn 56, durable-ack 39, disposal 85, golden 3, bridge.test all OK. Full Python list: no new failures. Publish gate passes. Live: the coordinator reruns the non-owner restart witness at this head (moving the rig's ledger to format 5 is fine there). The PR body's test counts and rollback/risk text are refreshed. |
|
@cla-assistant check |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-approving at 448b9d1. Since 68b0249 there's a main merge plus 1edc062: the attempt ledger becomes format 5 (reads 3, 4 and 5, and upgrades on its next write). An earlier bridge can't read format 5, so after a rollback it holds every task rather than serve from a format-4 ledger and save a raw, unredacted held copy. Redaction therefore can't be undone silently by a downgrade. The cost is operational and documented in the changelog and design doc: a rolled-back Sparrow holds all tasks until the ledger is set aside. Whoever does a Sparrow rollback needs that line in their runbook. Measured from main, the rest of the diff is unchanged.
(agent-assisted, Blue for Chi's seat)
|
Post-restart round trip for #6135 at exact head Limits: non-owner sender (tier I also read the code on the author's V2-only claim: the attempt ledger is created only under Result: saved copy read directly, Raw output |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 448b9d13795fb3ca021187e539e5906c545ca024. Since my approval at 68b0249dd: 1edc06297 (ledger format 5) and a main merge. Measured against each head's merge-base, the topic diff is unchanged apart from the fence (task_attempts.py:144-147, its test, CHANGELOG and protocol doc).
Approve. This addresses keweichen's blocker at 68b0249dd: the previous bridge now fails before it can persist or replace anything.
Verified in a scratch clone (both flags on), with my own two-process probe. Process 1 is this head; process 2 is the exact base 558e221b6, sharing one workspace. The body is an owner vault set K hunter2secret.
- Rollback (this head writes, base reads, fresh lease L3): the base gets
take_ownership: Falseand logsrelay-task-ledger.json unreadable — ownership not written._write_task(L3)returnsNone("left unacked for a re-serve"), and the L3 ack is refused. The ledger stays format 5 with the pending refv2-369aa…under L2. Nov1-file exists, no file anywhere holds the plaintext, and the base process stores 0 times. This reproduces the author's probe. - Control (same run with
task_attempts.pyat68b0249dd, format 4): the base takes ownership, accepts L3 and stores a second time (stores: 1after the writer's 1). The instrument does detect the defect. - Upgrade (base writes format 4 with a raw
v1-hold, then this head starts): the ledger becomes format 5.contract/runs/seqs/tasksare byte-identical apart fromowner, and the record reads back._commitonly rewrites the number, so nothing is lost. A fresh install writes 5 (_readreturns{"format": FORMAT}when the file is missing). - Roll forward after a failed rollback: this head reopens the format-5 ledger the base refused, takes ownership and finds the record intact. Nothing is stranded once the newer bridge is back.
- The test discriminates: with
FORMAT = 4restored (_READSkept or reverted),test_an_earlier_reader_refuses_what_this_writes_and_this_upgrades_formats_3_and_4fails4 != 5. 90 tests were collected in both arms. - Earlier behaviour: hooks 233 OK, which includes the held-copy resend-note test, the v1-/v2- same-lease test and the v2-without-redaction test. Also OK: attempts 90, lifecycle 9, durable-ack 39, agent-turn 56, and
remote-gateway-bridge.test.pyPASS. The bridge code is unchanged since68b0249dd, where I showed those tests fail with the fix reverted. review-checks.shon the diff: PASS. CI: 46 pass, 3 skipped. The commit status endpoint has 1 context (license/cla: success).- The only other ledger reader is
adapters/hatch/cli.py, which ships in the same package, so it moves with it.
Worst case: limited to Lifecycle-V2 hosts that roll Sparrow back. That bridge holds every task, unacked, until it is rolled forward or the ledger is set aside. This is fail-closed and loud in the log.
Non-blocking:
-
Set-aside cost should be spelled out (CHANGELOG /
docs/remote-gateway-protocol.md). I ran it: with the ledger moved aside, the base bridge serves L3 fresh and stores the owner'svault setagain, which is the pre-PR double store. The orphanedv2-copy is left behind, with no plaintext. So the runbook should say: rolling forward is lossless, and is the preferred recovery. Setting the ledger aside drops in-flight attempt state and can re-store a re-servedvault set. -
Owed live runs:
- The owner path: an owner
vault setheld, then restart, then publish showing the resend note. - The rollback itself: a real previous Sparrow against a format-5 ledger.
Both rest on the harness and probes. The real-broker witness at this head is Team-tier and forward-only.
- The owner path: an owner
Ready to merge from this seat, pending keweichen's re-check of the block.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 448b9d13795fb3ca021187e539e5906c545ca024.
APPROVED — the rollback blocker is fixed, and this head is ready to merge from the code/safety review scope, subject to the repository's normal gates.
The format-5 fence owns the compatibility decision in the shared ledger writer: startup take_ownership() is the first write, _commit() stamps format 5 under the existing file lock and atomic writer, this head reads/upgrades formats 3 and 4, and the exact previous bridge reads only 3 and 4.
I independently ran the current writer → exact previous reader (4e9521f08, whose bridge/task-attempt bytes match the earlier 558e221b6 probe) against one shared workspace. HEAD stored L1 once and held L2 under a redacted v2- copy with ledger format 5. The previous bridge then received both same-L2 and fresh-L3 serves:
take_ownership=false
write_task=None
vault_intercepts=0 ack_calls=0
ledger_format=5 pending_lease=L2 pending_ref=v2-…
reserves=[v2-…] raw_count=0
No file changed. As a positive control, changing only the copied ledger's format from 5 to 4 made that exact reader reproduce the former bug: same-L2 wrote a raw v1-; fresh-L3 wrote/adopted a raw v1- and later re-intercepted it. The fence therefore discriminates on the intended boundary rather than merely producing a reassuring zero.
I accept the exact-head real-broker witness in comment 6575067460: unmodified bridge, kill before L2 ack, normal restart with epoch 190→191, generation-2 publication, one post, and direct zero-plaintext checks. Its Team-tier/forward-only limitation is explicit; owner behavior and rollback are covered by the harness and exact cross-version probe.
Worst-case disruption is disclosed and flag-limited: with Lifecycle V2 on, the first write is a one-way format-5 upgrade; an older bridge cannot resume attempts and holds normal lifecycle work until the ledger is deliberately set aside. Flags-off lanes never create or read it.
Validation at this head: attempts 90/90, hooks 233/233, lifecycle 9/9, agent-turn 56/56, durable-ack 39/39, golden 3/3, full bridge suite, git diff --check, review checks, exact-head CI, 96% diff coverage, and CLA all pass.
Non-blocking PR-body cleanup: add the 448b9d137 live witness; update the two renamed test names; remove the claim that the saved question is redacted again on publication; acknowledge the pinned crash-before-render schedule; and qualify “held copies … are left … redacted” to new v2- copies, since pre-change v1- copies remain unchanged.
(agent-assisted, Sutando for Kewei's seat)
#6135 moved room.task.progress onto the lane's MCP endpoint (_mcp_tool, a minted delegation, exceptions scrubbed inside gateway_call). This PR's safe diagnostics are re-seated on that shape: the error code is read in _mcp_tool's HTTPError branch, the one place the response is still open, and _send_task_step keeps the not_deployed payload with http_status and error_code. Tests: the 503 and 401 rows go through the minting relay; main's 404 cases expect the documented payload; main's AgentAccess class is kept whole; the Publisher tests get a fresh secret registry each, because the AgentAccess fixtures mint one-letter tokens that the bridge registers as secrets, which turns every error code into a "piece" of one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Why
With
REMOTE_TASK_LIFECYCLE_V2=1, a task served again under a new lease is held, and its served copy is saved tostate/relay-reserves*/v1-*.jsonso the generation its ack names can run later (#6108)._save_reservesaved the raw broker task. Redaction happens later, when the task file is rendered, so a pasted token orvault setvalue in the body, the wake's room answers or the room question reached disk unredacted. Mode 0600 doesn't change that: workspace state may be synced or backed up, and the copy can outlive a restart.Found while answering keweichen's review of #6092 (#6092 (review)). It's tracked as item 4 of #6124, a gate before the flags go on. Flag-gated, so default installs are unaffected.
What
_redact_quoted(value, found). Every string, dict keys and ids included, goes through the vault grammar's redactor (redaction only, never interception or storage), thenfilter_chat_secrets. feat(gateway): an agent asked a room question answers it once, and the gate reads only its own task [AgentActivity] #6092's_redacted_questionuses it, and so does the saved copy._save_reservesaves a redacted copy (_cleaned_for_reserve):v2-<hash>.json, envelope format 2, with what was found undercleaned._read_reservedrops any mark from the body and restores it only from a format-2 envelope. A copy marked that way is published without a second vault interception or question redaction, and its secret notice comes fromcleaned. A task-wide copy from before envelopes, a format-1 envelope, or a broker field never carries the mark.Owner
vault setin a held copy: a held copy never stores it. It's replaced with a note that it was not stored from this re-sent copy and should be sent again if the key is not stored, and generation 2 publishes that note. A late store could put an old value over a newer one, so only a serve's first render stores. Anyone else's command is redacted as usual.Not changed: copies saved before this change stay as they are, and are handled as before, until they're published or swept.
Evidence
Before, with the new tests run against
main's bridge:The same leak on
main, reproduced directly (REMOTE_TASK_LIFECYCLE_V2=1, turn context off; bodyuse ghp_… and vault set K hunter2secret):After, at this head (
tests/gateway-agent-activity-hooks.test.py):test_a_held_serves_saved_copy_is_redacted_and_publishes_without_intercepting_again:vault setvalue and a media marker in the body;test_a_saved_copy_redacts_the_wake_and_the_room_question_but_not_their_ids: secrets in a wake answer and in a question's prompt and option label are redacted, the ids are kept, and a question named by a secret is dropped.test_a_broker_field_never_marks_a_fresh_task_as_already_redacted: a broker-sentreserve_cleaneddoesn't skip the body's redaction.Suites at this head (
448b9d137), withREMOTE_TASK_LIFECYCLE_V2=1 REMOTE_TASK_TURN_CONTEXT=1:Full Python list (
scripts/discover-python-tests.sh) and the package publish gate (packages/ag2-sparrow/tools/publish_gate.sh) run locally: no new failures.Live round trip
On the coordinator's local k3d rig, with a real broker and V2 on:
b3dde43ee: a held re-serve with a crafted body, a restart before the ack, and publication from the copy. No plaintext in the copy, the task file or the log.60ab54396: the same, with a non-owner body (Team tier). It's accepted for the restart gate.e3998897d: the same non-owner run (fix(gateway): a held serve's saved copy is redacted before it is written #6135 (comment)).vault setcan't run on the rig (its owner is a Synapse admin) and would write the real Keychain on this Mac. The owner path is pinned by tests: a held copy and its publication never store it, after a crash, a render, or a finished run, and generation 2 tells the owner to resend.Interaction with #6092 (merged)
#6092 sanitizes
room_questionat the top of_write_task, before the save. That redaction is idempotent, so the two compose: a question is cleaned before it's saved, and cleaned again, to no effect, when the copy is published. This branch includes #6092's merge.Risks
vault set. That only matters if the first generation of that same message never rendered, which the tests and I haven't found a schedule for.REMOTE_TASK_LIFECYCLE_V2=1(_ATTEMPTSis built only then), so a flags-off host never writes it, and a flags-off bridge, old or new, never reads it.vault setstore), until the ledger is set aside.ag2-sparrowservice), then rename<workspace>/state/relay-task-ledger<.lane>.jsonout of the way (e.g. append.aside) and restart.<.lane>is the instance suffix, if any.state/relay-reserves*/are left in place, redacted.