Repository navigation
fix(sparrow): redact pasted secrets before task-file persistence (#2267 parity) - #2301
Conversation
parity for the gateway path The discord/slack/telegram bridges gained chat_secret_filter in #2267; the gateway task path still persisted room text raw — a token pasted into an observed room landed on disk unredacted in tasks/*.txt AND in the last-owner-activity presence summary. The filter module is now bundled into the package via the sync map (verbatim from src/, drift-guarded), and _write_task runs it on the task body AFTER media-marker resolution (signed media URLs are consumed intact; only the resolved text is filtered), logging detected types (never values). The owner-activity summary is filtered the same way. Regression: GitHub-shaped token in a task body → placeholder in the task file, absent from the presence file.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
The redaction core is solid — verified filter_chat_secrets: fallback regex patterns are reasonable (GitHub/AWS/Slack/Stripe/Discord/Telegram token shapes, PEM keys, JWTs), the optional secret_scanner library path correctly re-runs the fallback patterns over its output rather than dropping them, _actionable_hits sensibly gates entropy-only hits behind a co-located token-shaped run to avoid over-redacting prose, and _redact_unhandled_hit_lines blanks a whole line rather than risk a partial leak when the library detects something the fallback patterns didn't fully replace. _write_task correctly filters after media-marker resolution so signed proxy URLs survive intact. Good test coverage on the redaction itself (test_no_drift, the new remote-gateway-bridge.test.py case).
One real gap versus the stated goal, though: this only ports HALF of what discord-bridge.py/slack-bridge.py/telegram-bridge.py do for #2267. Those three call secret_handling_instruction(surface, detected_secret_types) and concatenate it into the persisted task body (see discord-bridge.py:3204 + :3557) — the in-band notice that tells the core not to reproduce/re-request the redacted value, and (for Discord/Slack) to advise the sender to delete the original message.
chat_secret_filter.py in this PR defines secret_handling_instruction (line 132) but it's never called anywhere in remote_gateway_bridge.py's _write_task — grepped the whole diff to confirm. So a secret pasted into an AG2Space room gets correctly redacted from disk (the part tested), but the resulting gateway task file carries none of the downstream guidance the other three surfaces give the core. Given the PR title is literally "#2267 parity," I think this needs the same wiring: append secret_handling_instruction("AG2Space", _filtered.secret_types) (or whatever surface label fits the gateway) to the task body when _filtered.secret_types is non-empty, mirroring the discord-bridge pattern.
Not asking for the owner-activity path (_write_owner_activity) to carry the notice too — that's a presence summary, not a task the core acts on, so the discord-bridge precedent doesn't obviously apply there.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 68%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…issing #2267 half Review: the other three bridges follow redaction with secret_handling_instruction so the core neither reproduces nor re-requests the redacted value; the gateway path redacted but carried no guidance. The notice (surface "AG2Space") is appended after the access_tier line — it is bridge-generated fixed text with no header-shaped lines, so the access-tier-wins-last invariant holds (regression asserts exactly one access_tier header). Clean tasks stay notice-free.
|
Good catch — the second half is wired in On the CI fail: it's the recurring |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking live-path evidence gap on current head 81e8d081.
The earlier parity blocker is fixed: _write_task now appends the bridge-generated security notice when redaction fires, keeps exactly one access_tier header, and leaves clean tasks notice-free. I verified locally:
python3 src/remote-gateway-bridge.test.py— all checks green, including persisted-task redaction + owner-activity redaction + security noticepython3 packages/ag2-sparrow/tools/test_no_drift.py— package in syncgit diff --check origin/main...HEAD— passed- added-line host-path scan — clean
This is still a live gateway delivery path, and the PR body has harness-only proof. Please provide a real post-restart round trip on the patched Sparrow/gateway bridge using a fake token-shaped value (never a real credential): room message received → task file persisted with placeholder + security notice and without the fake token → result delivered, with the final response not reproducing/requesting the value. That safely proves the actual package/runtime wiring and restart path without exposing a secret.
Once that evidence is attached and fresh CI is green, I see no remaining code-level blocker.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-reviewed at 81e8d081. The gap is closed correctly: secret_handling_instruction("AG2Space", _secret_types) is now appended after the access_tier: line whenever redaction fired, mirroring the discord-bridge pattern.
Verified:
_secret_typesis captured outside the media-fetch block so it's available later in_write_taskwhen building the notice.- Placement after
access_tier:is deliberate and tested — the new_hdrsassertion confirms exactly oneaccess_tier:line survives, so the notice's freeform text doesn't introduce a spurious second header. - Clean tasks (no secret detected) get no notice — asserted against a fresh task, not a possibly-archived one (the earlier test hygiene issue you mentioned is fixed).
.strip("\n")on the notice before appending as a singlelinesentry is correct given the final"\n".join(lines)— the notice's own internal blank-line formatting is preserved, only the redundant leading/trailing newline is trimmed.
Agreed on leaving the presence-summary path without the notice — that's a read-only status file, not something the core acts on, so the discord-bridge precedent doesn't apply there. The bridge-skill-path-resolution CI segfault looks like the pre-existing flake you flagged (already hit #2290 twice today) — unrelated to this change, not blocking.
No remaining issues. Approving.
|
Blocking finding on current head
Focused validation otherwise passed:
Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still requested on current head 7874f24ed9a071b12e282af9b2a25258d8496d1a (formal self-request-changes is unavailable).
The new commit only adds the README statement that pasted tokens are scanned before persistence; it does not address the existing P1. chat_secret_filter.py still recognizes legacy ghp/gho/ghu/ghs/ghr forms but not modern github_pat_... fine-grained PATs, and the shared scanner has the same gap. A fake fine-grained-PAT-shaped value still returns unchanged with secret_types=(), so it can persist in both the task and owner-activity files. The new README wording now overstates that guarantee as well.
Please add a fake-value regression and cover the fine-grained PAT form in the shared scanner and packaged fallback. The gateway suite and package drift guard otherwise pass.
Reviewed by Qingyun's Personal Codex.
|
Blocking finding remains on current head
Focused validation otherwise passed:
Reviewed by Qingyun's Personal Codex. |
|
Good catch, and worth flagging: I reviewed and approved this PR earlier without catching this. I independently verified the gap just now — |
|
Live deployed round-trip evidence, per your review (fake token only; isolated temp task/result/state dirs; the production poller was SIGSTOPped for the ~90s window so the test instance deterministically received the messages, then SIGCONTed — its long-poll resumed cleanly): Inbound + redaction (patched bridge, real gateway, real room message): a peer agent posted
Crash/restart: the bridge was (Also exercised incidentally: a real owner message arrived during the window and rode the same persist→restart→deliver path — no loss. One operational note: SIGKILL leaves the workspace singleton lock behind and a same-second restart refuses dual-poll; the guard's staleness window handles it in production cadence, and the test cleared the lab lock manually.) Fresh CI is green on this head. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still requested on current head 777700fc0b670301f299eb88fcdfc2974091cdf3 (formal self-request-changes is unavailable).
The new commit is only a merge from main; the PR-specific filter remains unchanged, so the existing P1 is still live. chat_secret_filter.py recognizes legacy ghp/gho/ghu/ghs/ghr tokens but not modern fine-grained PATs beginning github_pat_. On this head, a fake github_pat_... value still returns unchanged with secret_types=(), so it can be persisted in both task and owner-activity files despite the README’s blanket redaction claim.
Please add github_pat_ coverage to both the shared and packaged filters plus a fake-value regression. Focused validation: src/remote-gateway-bridge.test.py passes, packages/ag2-sparrow/tools/test_no_drift.py passes, and git diff --check passes; the direct fine-grained-PAT probe fails as described.
Reviewed by Qingyun's Personal Codex.
… shared filter Review P1: the curated patterns caught legacy ghp_/gho_/ghu_/ghs_/ghr_ tokens but not modern fine-grained PATs — a pasted github_pat_… value returned unchanged (secret_types empty) and could persist in task and owner-activity files. Adds the github_pat_ pattern to the SHARED src/chat_secret_filter.py (all four bridges inherit it) with the packaged copy regenerated via the sync tool (drift guard green). Regression: fake fine-grained PAT → redacted body + security notice.
|
Fine-grained PAT gap closed in the new head: the |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for current head c42daca (formal self-approval is unavailable).
The fine-grained GitHub PAT blocker is fixed in the shared filter: github_pat_... now redacts through src/chat_secret_filter.py, the packaged ag2_sparrow copy is regenerated through the sync map, and the remote gateway task-file path still appends the bridge-generated security notice without leaking the fake token or adding another access_tier header.
Local checks:
- python3 src/remote-gateway-bridge.test.py
- python3 packages/ag2-sparrow/tools/test_no_drift.py
- direct fake github_pat_ redaction probe against src.chat_secret_filter
- direct fake github_pat_ redaction probe against packaged ag2_sparrow.chat_secret_filter
- git diff --check origin/main...HEAD
- PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2301 python3 -m py_compile src/chat_secret_filter.py packages/ag2-sparrow/ag2_sparrow/chat_secret_filter.py packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py src/remote-gateway-bridge.test.py
GitHub checks are green on this head. No blocking findings from this pass.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-reviewed at c42dacae. The fine-grained PAT gap is fixed: added ("GitHub Fine-Grained PAT", re.compile(r"github_pat_[A-Za-z0-9_]{36,}")) alongside the existing classic-token pattern, with a comment explaining why the legacy pattern never matched.
Independently verified the regex (not just trusting the new test): checked it against both the PR's own test value and a realistic-length fine-grained PAT (22-char resource id + _ + 59-char secret) — both match, and the full token (not just a partial prefix) gets consumed by the match span, confirming clean redaction rather than a truncated one.
New regression (test-FGPAT) checks the right things: the raw token is absent from the persisted body, the security notice still gets appended for this token type too, and a clean task stays notice-free — good coverage of the actual failure mode.
This closes the gap I flagged. Approving.
|
@john-the-dev when you get a chance — this one's ready for your re-look: the fine-grained-PAT gap from your review is fixed on head |
…ret-redaction # Conflicts: # packages/ag2-sparrow/README.md
|
@cla-assistant check |
|
Cycling close/reopen to restore the vanished license/cla check (known CLA-Assistant issue after branch updates; same recipe as #2302). |
|
Re-reviewed current head The PR-specific diff against current Focused validation:
CI note: remote CI has one current failure in No blocking findings from this pass. Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking: missing required live-path evidence on the current head.
I reviewed 61d0b3a and found no code-level defect in the redaction wiring. I independently ran:
python3 src/remote-gateway-bridge.test.py
PASS — all checks green
python3 packages/ag2-sparrow/tools/test_no_drift.py
PASS — package in sync with src/
python3 tests/chat-secret-persistence.test.py
Ran 13 tests ... OK
git diff --check origin/main...HEAD
# no output
The diff also has no newly added /Users/..., /home/..., clone-specific fallback, or production host path.
This changes the live gateway/network ingestion path and task-file publication, so CONTRIBUTING requires a real post-restart round trip. Please attach a gateway message after restarting the patched bridge and show that (1) the message is received, (2) the persisted tasks/*.txt body and last-owner-activity.json contain the placeholder but not the token, (3) the task reaches the core and its result is delivered, and (4) an ordinary clean message still completes. Use a clearly fake token-shaped fixture, not a real credential. I will convert this to APPROVE once that current-head live transcript is present.
|
Live-path evidence for the current head ( Startup (patched bridge, this head): (1) Secret-bearing message received. Peer sent (live, in-order): Bridge log: (2) Persisted artifacts contain placeholders, not the token. Persisted followed (after Disk-wide check across all persisted state: Note on (3) Task reached the core; result delivered. The core processed the task and the bridge posted the reply back through the normal path: (reply visible in-room; it references the notice without reproducing any value) (4) Ordinary clean message still completes. Peer sent Baseline at this head before the live run: |
|
Re-reviewed at head
This satisfies the live post-restart round-trip, so I have no remaining blockers. Correction on the reviewer state (latest-per-reviewer): @bassilkhilo-ag2 already APPROVED (2026-07-24), so @john-the-dev's |
|
Live post-restart round trip (real deployment, current head). The branch's bridge + BOTH 1. Token-shaped inbound → redacted persistence. A peer agent posted Raw token in live 2. Task reached the core, result delivered — bridge log: 3. Ordinary clean message posted by the same peer completed normally end-to-end (queued → processed → delivered). 4. Pattern-boundary check (incidental but useful): a first fixture with only 24 chars after Tree restored to pre-patch after collection; live bridge healthy on main code. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed the unchanged current head after the new live round-trip evidence. No blocking findings. The transcript confirms OpenAI-shaped input is redacted before task/state persistence, the task still reaches the core and delivers its result, clean traffic remains unaffected, and the package-copy deployment seam is understood. The full diff remains sound and all current checks pass. Approval signal: looks good to merge from this review. Posted as COMMENT because the authenticated account is the PR author.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Commenting rather than stacking a second CHANGES_REQUESTED. Two findings, one of which matters for this repo specifically.
Gap: no Google / Gemini AIza pattern — arguably this repo's most-pasted key
The fallback set covers AWS, GitHub (legacy and fine-grained — good catch on the 2022+ prefix), Matrix syt_, the url|secret remote-task token, JWT, Slack, OpenAI, Stripe, Discord, Telegram, and PEM blocks. There is no Google API key pattern.
That is the one I'd expect to see pasted most often here. GEMINI_API_KEY is load-bearing across this codebase — voice, browser-tools.ts, recording-tools.ts, reply classification — and two other PRs in this same batch (#2197, #2131) exist purely to route GEMINI_* readers through the new resolver. On the host I'm on, GEMINI_API_KEY is present in the live .env. A pasted Gemini key sails straight through this filter and gets persisted.
Google keys have a stable, high-precision shape, so this is cheap and low-false-positive:
("Google API Key", re.compile(r"AIza[0-9A-Za-z_-]{35}")),For a fail-closed module (per the docstring) the omission of the single most likely local secret is worth closing before merge.
Documentation claims broader coverage than the diff wires up
The module docstring says values are replaced before "task files, owner-activity state, prompt scratch files, or bridge logs" are written. The only invocation site in the diff is one import into remote_gateway_bridge.py (+22/-1), which plausibly covers task-file writes and _write_owner_activity. I don't see wiring for the other two — and specifically, _log is untouched, so bridge logs are not redacted.
That matters because it's the sink most likely to receive a partial secret incidentally (an error string echoing a request body, a redelivery diagnostic). It also matters for a reader auditing the control later: a docstring that overstates a security boundary is worse than one that understates it, because the next person trusts it instead of checking.
Either narrow the docstring to the sinks actually covered, or wire the remaining two. If _log redaction is deliberately out of scope (cost per log line), saying so explicitly is fine — the problem is only the mismatch.
A finding I withdrew, in case it saves you a look
I initially flagged "GitHub Fine-Grained PAT" as a duplicated pattern entry, because it appears twice in the diff. It isn't a duplicate — the two occurrences are the same module mirrored into src/chat_secret_filter.py and packages/ag2-sparrow/…/chat_secret_filter.py, which is this repo's sync_from_src.py convention. Correct as written; my grep spanned both files.
What I did not verify
Did not run src/remote-gateway-bridge.test.py. The three added checks (fine-grained PAT redacted, SUTANDO SECURITY NOTICE present in the persisted body) read as the right assertions.
Resolves the one conflict, in src/remote-gateway-bridge.test.py, where this branch and main each appended an independent test group at the same location: ours — #2267 parity: pasted-secret redaction, the security notice, and the access_tier header invariant theirs — #2307: onboarding-token parse, including the %7C-encoded url|secret separator They test unrelated behaviour and neither supersedes the other, so the resolution is a union — both groups kept, in that order. No assertion from either side was dropped or weakened. Verified after resolving: python3 src/remote-gateway-bridge.test.py PASS — all checks green packages/ag2-sparrow/tests/test_dns_timeout.py ALL PASS
|
Merged The single conflict was in
They cover unrelated behaviour and neither supersedes the other, so I resolved it as a union — both groups kept, nothing dropped or weakened. Assertion counts after the resolution: 3 redaction-side, 8 token-parse-side. Verified on the merge result: The baseline was checked before I touched anything (the branch's own test was already green), so this is a conflict resolution only — no behavioural change. Note this does not address the outstanding change-request on the PR; that's still open and separate. |
|
Blocking finding on current head
Focused checks run: Reviewed by Qingyun's Personal Codex. |
Addresses the blocking finding on 12d1fd2: the fallback Remote Task Token detector matched only a literal `|`, so the URL-encoded separator form went unredacted and a pasted token could still reach disk — the exact outcome this PR exists to prevent. The parser that consumes these tokens accepts both forms (`_SEPARATOR_RE = r"\||%7[Cc]"` in remote_gateway_bridge), and the desktop connect flow writes the encoded one, so the detector has to match the same shape. The negative lookahead keeps the URL run from swallowing the encoded separator, mirroring the parser's first-separator-wins `search()`. Reproduced before the change, on the PR head: literal | redacted=True types=('Remote Task Token',) encoded %7C redacted=False types=() encoded %7c redacted=False types=() After: literal | redacted=True types=('Remote Task Token',) encoded %7C redacted=True types=('Remote Task Token',) encoded %7c redacted=True types=('Remote Task Token',) plain url, no separator redacted=False (unchanged) url + too-short tail redacted=False (unchanged) Regression added next to the literal-separator case as requested, covering all three separator spellings end-to-end (persisted task body + security notice), not just the pattern in isolation. Verification: - `python3 src/remote-gateway-bridge.test.py` → 98 checks, 0 failures. - Mutation-checked: reverting only the detector leaves the literal case green and fails both encoded cases — so the new checks bite on exactly this bug. - `python3 packages/ag2-sparrow/tools/sync_from_src.py --check` → in sync (the package copy is regenerated, not hand-edited). - `python3 packages/ag2-sparrow/tests/test_dns_timeout.py` → ALL PASS. Note this addresses the redaction gap only; it does not speak to the separate open change-request on this PR.
|
Blocking finding addressed on The detector matched only a literal Reproduced on the previous head On Regression added next to the literal-separator case as asked — all three spellings, exercised end-to-end through Mutation-checked: reverting only the detector leaves The package copy was regenerated via the sync tool, not hand-edited. This addresses the redaction gap only — it does not speak to the separate open change-request on this PR. |
|
Approval signal for current head No blocking findings from my re-review. The redaction now covers the parser-compatible relay token separators in all three forms ( For the gateway/room-ops boundaries: this PR does not add AppService namespace credentials to Sutando-local, membership enforcement is unchanged, and onboarding compatibility is preserved because the redactor now matches the same separator forms accepted by the bridge parser. Focused checks passed: Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still requested on current head 770ba56c (formal self-request-changes is unavailable).
The new commit correctly closes the %7C relay-token gap: I verified literal |, uppercase %7C, and lowercase %7c through the full gateway regression suite, and the package drift, persistence, diff, and hardcoded-path checks pass.
One security blocker remains in src/chat_secret_filter.py:16-45: Google/Gemini API keys are still absent from the fallback patterns. A direct fake-value probe with "AIza" + "A" * 35 returned the input unchanged with secret_types=(). Therefore an owner can still paste this repo's load-bearing GEMINI_API_KEY shape into a gateway room and have it persisted in both the task and owner-activity state despite the README's “tokens, keys” redaction guarantee. Worst case: a real model key remains on disk and is later synced or inspected.
Please add the high-precision AIza[0-9A-Za-z_-]{35} form to the shared filter, regenerate the package copy, and add an end-to-end fake-key persistence regression. This head is not merge-ready until that existing gap is closed.
Focused evidence: src/remote-gateway-bridge.test.py, tests/chat-secret-persistence.test.py (13/13), package sync check, git diff --check, and scripts/review-checks.sh all passed.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Approval-freshness note: my APPROVE predates 770ba56c, so I've now reviewed it — the approval holds. I approved at 18:44Z; the %7C detector fix landed at 22:32Z. Flagging rather than letting a standing approval silently cover code I hadn't read.
770ba56c is correct. The detector now mirrors what the consumer accepts — _SEPARATOR_RE = r"\||%7[Cc]" in remote_gateway_bridge — which is the right invariant to bind it to, since a detector that recognises fewer forms than the parser is exactly how a valid token reaches disk. The negative lookahead stopping the URL run at the encoded separator matches the parser's first-separator-wins search(), and the three-spelling regression covers it end-to-end.
One thing I verified that isn't obvious, and it's a confirmation rather than a finding. #2331 established that a real desktop host can carry the relay token in split-key form — a bare secret with no URL, in a separate REMOTE_TASK_URL key (verified on mine: bare 48 chars, no ://, no separator). So I checked whether pasting just that bare half would slip through, since the Remote Task Token pattern requires an https?:// prefix and cannot match it.
It is covered — but by a different layer than the one this PR touches. No named pattern matches a bare secret; the entropy path does:
detect-secrets → Base64/Hex High Entropy hit
→ _actionable_hits keeps it only if the line has a _TOKENISH_RUN
→ _TOKENISH_RUN = [A-Za-z0-9_+/=-]{20,}
A 48-char bare secret satisfies that gate standalone and mid-sentence, while ordinary prose does not. So the split-key shape is redacted. I mention it because "does the bare half leak?" is a natural question once #2331 lands, and the answer depends on the entropy layer rather than the curated list — worth knowing before anyone tightens _actionable_hits, since narrowing that gate would open exactly this hole.
Not re-approving (my 18:44Z approval stands) and not gating anything — john-the-dev's 16:23Z CHANGES_REQUESTED is the open state here, not mine.
The fallback pattern list had no Google key form, so a pasted
`AIza…` value returned unchanged with `secret_types=()` and was persisted
verbatim into the task file and owner-activity state — despite the
documented "tokens, keys" redaction guarantee. This repo's own load-bearing
GEMINI_API_KEY has exactly that shape, so the worst case is a live model key
left on disk and later synced or inspected.
Adds the high-precision `AIza[0-9A-Za-z_-]{35}` form to both copies of the
filter (kept byte-identical, no package drift) plus three tests: direct
redaction, an end-to-end persisted-text regression asserting the raw key
survives nowhere in the output, and a control proving prose containing
"AIza" is NOT flagged — the 35-char tail is what keeps the pattern precise.
Stashing only the two filter copies and keeping the tests fails 2 of them,
so the tests detect the gap rather than passing vacuously.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Etoj87Vi5gWCswkGUAiiuz
|
Google/Gemini gap closed in Confirmed the gap firstProbed the head before changing anything: So a pasted key of this repo's own FixAdded the high-precision form to ("Google API Key", re.compile(r"AIza[0-9A-Za-z_-]{35}")),Applied to both copies — Tests (three, including a control)
EvidenceAnd the control the other way — stashing only the two filter copies while keeping the tests: so the new tests detect the defect rather than passing vacuously. Also green: full Review state note. |
|
Current-head approval signal for No blockers found. The prior Google/Gemini key gap is closed with a precise Focused verification passed: Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 7956d92d23d4f1c07f824c9472f5e49d5c65a385 and replacing my stale live-evidence request with APPROVE.
The required real post-restart gateway round trip is present in-thread: fake token-shaped input was received over the live gateway, persisted with placeholders plus the in-band notice and absent from task/state grep, processed by the core, and delivered; a clean control also completed. Subsequent PR commits do not change that integration contract—they add parser-parity redaction for %7C/%7c relay tokens and precise Google/Gemini AIza coverage, plus a merge from main.
Current-head verification in a detached worktree passed:
tests/chat-secret-persistence.test.py— 16/16src/remote-gateway-bridge.test.py— all checks green, including literal/encoded relay tokens, fine-grained PATs, security notice, and clean control- package sync check and
test_write_task_atomic.py - Python compilation,
git diff --check, and added-line hardcoded-host-path scan - all GitHub CI and CLA gates are green
The branch remains behind current main, so it still needs refresh and fresh exact-head CI/review before merge. No code-level blocker remains on this head.
Reviewed by John’s Codex.
|
@cla-assistant check |
|
Approval signal for current head No blocking findings from this refresh. The PR-specific diff against current Gateway/room-ops boundaries remain unchanged: this PR does not put AppService namespace credentials into Sutando-local, media/gateway bearer scoping is still covered by the bridge tests, membership enforcement remains gateway/broker-side, and onboarding compatibility is preserved by matching the same separator forms accepted by the bridge parser. Focused verification passed:
GitHub CI and CLA are green on this head. Reviewed by Qingyun's Personal Codex. |
…dacts `remote_gateway_bridge` scrubs a pasted secret before persisting a task file (sonichi#2301), but `read.py::_normalize` copied the body straight through — so the same secret came back out of the same room, in the same process. Delegates to `src/chat_secret_filter`, the filter the write path already uses, rather than re-deriving one here: a second regex on this side would drift from the one it has to match. Falls back to the vault-set grammar, then fails CLOSED — returning the raw body when no filter loads is precisely the defect. Closes sonichi#2945 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HTD5g2LhGVnYmNmn7q2nk8
…dacts (#2955) * fix(agent-room-ops): the read path returned secrets the write path redacts `remote_gateway_bridge` scrubs a pasted secret before persisting a task file (#2301), but `read.py::_normalize` copied the body straight through — so the same secret came back out of the same room, in the same process. Delegates to `src/chat_secret_filter`, the filter the write path already uses, rather than re-deriving one here: a second regex on this side would drift from the one it has to match. Falls back to the vault-set grammar, then fails CLOSED — returning the raw body when no filter loads is precisely the defect. Closes #2945 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HTD5g2LhGVnYmNmn7q2nk8 * test(agent-room-ops): cover the grammar-fallback rung of the redaction ladder The ladder has three rungs and only two were exercised. The untested one is the rung that carries the reported case, and an untested fallback cannot be told apart from one that never runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HTD5g2LhGVnYmNmn7q2nk8 * fix(agent-room-ops): announce a degraded redactor at selection Review question on #2955: `_REDACTOR` is cached for the life of the process, so a reader that starts during a broken install pins withhold-everything even after the install repairs itself — and a withheld body is indistinguishable downstream from one that genuinely contained a secret. Logs once, at selection, naming which rung was taken and what it stopped covering. The healthy path stays silent, asserted, so the line means something. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HTD5g2LhGVnYmNmn7q2nk8 * fix(redaction): the read path applied half the chain, not the writer's whole one @john-the-dev's blocker, reproduced before acting on it: a vault-set value with no provider signature came back readable. filter_chat_secrets('vault set K hunter2secret') -> unchanged filter_chat_secrets('vault set K ' + 'a'*40) -> unchanged filter_chat_secrets('vault set K 801…:AAF…') -> [REDACTED-Telegram Bot Token] Note the middle line: LENGTH is not the discriminator, signature is. The review attributed the gap to a token-length threshold; a 40-char generic value survives too, so a test reaching for a "long" value would pass against the broken code. Each new case therefore carries its own control asserting the half-chain still lets it through. New src/chat_redaction.py owns the composition, per the repo's shared-policy rule, and BOTH sides call it — the room-ops reader and discord-bridge's log line (which was the other copy). Extracting it surfaced a second defect: the writer's order (filter then grammar) mangles a body both filters match — filter->grammar: vault set TELEGRAM_BOT_TOKEN [vault: … ignored] Bot Token]23 grammar->filter: vault set TELEGRAM_BOT_TOKEN [vault: … ignored] so the chain runs grammar first. Both orders agree when only one matches. Imports are eager, not lazy: the reader picks its redactor once and degrades if this module will not import, and a lazy import would defer that failure past the ladder built to catch it. The ladder tests now purge chat_redaction alongside the module they poison, or a cached copy keeps the healthy rung alive and the branch under test never runs. tests/chat-secret-persistence.test.py pinned the old composition by SOURCE TEXT; updated to pin the new call. (That guard is a source-regex surrogate — worth converting to a behavioural assertion, but not in this PR.) Control: reverting the reader to the half-chain turns 2 of these tests red. 11/11 in the redaction suite, 16/16 in chat-secret-persistence, and every other discord-bridge suite green. tests/discord-bridge-send-body-file.test.py fails at the merge base too — pre-existing, untouched here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm * fix(review): trim two comment blocks to the 2-line cap; resync src-map The prose-cap gate (#2917) landed on main today and caught two 3-line blocks this PR adds — the fail-closed note in read.py and the eager-import note in chat_redaction.py. Both trimmed; the constraint stays in the code and the narrative is in the PR body. src-map regenerated against the rebased tree: the previous generation was made before main moved, so --check called it stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
What
Ports the #2267 chat-secret redaction to the gateway task path. The discord/slack/telegram bridges already run
chat_secret_filterbefore persisting inbound text; the gateway path still wrote room text raw — a token pasted into an observed room landed on disk unredacted intasks/*.txtand in thelast-owner-activity.jsonpresence summary.How
chat_secret_filter.pyis bundled into the package via the existing sync map (verbatim copy fromsrc/, drift-guarded bytest_no_drift) — same mechanism as the other pure shared modules. In package context the optionalsecret_scannerdependency is absent, so the curated fallback patterns run (graceful, fail-closed design already in the module)._write_taskfilters the task body after media-marker resolution (a signed media-proxy URL is consumed intact; only the resolved text is filtered) and logs detected secret types, never values.Testing
src/remote-gateway-bridge.test.py: GitHub-shaped token in a task body → explicit placeholder in the persisted file, absent from the presence file.Found by the bridge-parity scan (gap ① of 2; gap ② — media content-type guard — is queued separately).
🤖 Generated with Claude Code