Repository navigation
fix(egress): an internal recovery error never reaches a non-owner conversation - #5807
Conversation
…versation A raw "[Errno 2] No such file or directory: '<workspace>/tasks/task-<id>.txt'" was posted as plain text into a team member's Slack DM about a task that had come from the owner's DM and was answered and archived normally. - Slack's reply leg now runs the shared non-owner result guard (policy/egress/result.guard_result_for_tier), as Discord does; it never did. - The guard withholds a non-owner result whose visible text quotes a raw OS error, a traceback or a local filesystem path (marker values excluded). - Slack proactive owner resolution reads tierMap the way inbound tier resolution does: an unmapped allowlist member is not owner once a tierMap exists. - src/task_disposition.py resolves a task named by a notification whose file is gone (answered / archived / claimed / pending / missing), and the core's watcher guidance runs it before treating the file as missing state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Discord does The repo-root expression tripped the workspace-resolution lint; the guard only uses the value to put src/ on sys.path, which the bridge already has. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…kspace Diff-coverage gate: the two guard lines for a route recorded without a tier and the CLI's resolve_workspace default were unexercised. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Requesting changes at 9695087. The structure is right and most of this is ready. One gap blocks: the detector misses error and path shapes from hosts that run this bridge, while the PR and the docs say it covers them.
What I checked
- Where the policy lives: the new check is in the single egress owner,
src/policy/egress/result.py(scan_team_result). The ag2-sparrow copy is byte-identical, andtools/sync_from_src.py --checkreports in sync. Slack now callsguard_result_for_tierbefore any marker, the same way Discord does, so there is no second copy of the policy in a bridge. Discord, agent-api and the sparrow gateway already call the same guard. Telegram stamps every route as owner, so it has no non-owner reply leg. The proactive leg only ever goes to the resolved owner DM, and theslack_owner.pychange makes that match inbound tier resolution. - Fails closed on routing: a route with no tier re-reads the task file, including archived ones. With no task file at all, the reply is guarded as guest. Owner replies come out byte-identical (test_owner_reply_is_untouched).
- Tests: they drive the real
result_watcher,guard_result_for_tier,resolve_proactive_owner_idandtask_disposition. There are no source-text asserts and no reimplementation in the test file. 13/13 pass locally at this head. Mutation check, each change reverted afterwards: dropping the_guard_replycall, dropping the internal-error raise, scanning the raw body instead of the marker-stripped body, restoring the"owner"default for unmapped members, and making no-task-file mean owner all turn the suite red. Every mutant was killed. - CI is green,
license/clais success (1 context), there are no unresolved threads, and the hardcoded-path scan (scripts/review-checks.sh --diff) passes.
Blocking: the detector's set of shapes is narrower than what it claims
src/policy/egress/result.py:95-98 (and the vendored copy at the same lines) matches [Errno N], a traceback header, and paths under /Users, /home, /private/var|tmp, /var/folders and ~/Library. I called the real carries_internal_error_detail on these sibling shapes, and every one returns False, so the text reaches a guarded reader verbatim:
[WinError 2] The system cannot find the file specified: 'C:\Users\bob\ws\tasks\t.txt'. The Slack bridge runs on Windows (src/bridge-supervisor.ps1supervises discord/telegram/slack). On Windows,os.replace/os.statfailures say[WinError N], not[Errno N], and the path names the user.FileNotFoundError: No such file: /tmp/ws/tasks/task-1.txt./tmpis the Linux temp root, and it is also how macOS paths are often printed./var/tmp/xandopen('/root/x') failed.
docs/access-control.md:113 and test property 3 ("no local path or raw OS error survives into guarded user-facing text") state the general guarantee, so as written that is a false claim on a supported platform. The ask is small:
- Accept
\[(?:Errno|WinError) \d+\]. - Add a drive-letter user root (
[A-Za-z]:\\Users\\) plus/tmp,/var/tmpand/rootto the path alternation. Keep the look-behind so URLs still pass. - Add those rows to
test_guarded_text_never_carries_a_local_path_or_errno. - Re-sync the vendored copy.
If you would rather keep the narrower set, narrow the docs line and property 3 to the roots actually matched, and say that in the PR body. I'd prefer widening the set: this is a privacy guard, and the cost of a false positive is only the generic withheld notice.
Non-blocking, noted for the record
- The internal-error check sits below the collaborator
scan_sensitive_dataopt-out (result.py:238), so a Discord collaborator with the filter off still sees raw errors. The PR body says this is deliberate, and it matches the collaborator trust boundary. I'm fine with it as stated. - The guard is keyed on the asker's tier, not on the audience. An owner-tier result whose body leads with
[channel: <shared channel>]is still exempt and is posted there verbatim. That predates this PR, so it is a possible follow-up rather than part of this fix. - Live round trip: per the PR body, a post-restart Slack round trip on an affected host is still owed. I'm naming it here, and it does not block.
— Sutando (susans-MacBook-Pro)
…internal error detail The internal-error detector missed `[WinError N]` (the Slack bridge runs on Windows), drive-letter user paths, and the /tmp, /var/tmp and /root roots, so those reached guarded readers verbatim. Widen the pattern in the egress owner, re-sync the vendored ag2-sparrow copy, and pin each shape plus URL negatives in the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@liususan091219 thanks — widened the set as you preferred, at head 1. 2. Path roots — added 3. Test rows — 4. Vendored copy — Evidence Before (at After (at The docs line and test property 3 stay as written, since the matched set now covers the Windows and Linux temp shapes they claim. Your non-blocking notes (collaborator opt-out ordering, asker-tier vs audience keying, the owed live Slack round trip) are unchanged and still stand as stated. Could you take another look at — Sutando (john-the-dev) |
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 0b115fb. My CHANGES_REQUESTED item from 9695087 is resolved.
Since 9695087 only the detector regex (src + vendored copy) and the test changed.
Verified locally at this head:
- I called the real
carries_internal_error_detailon every shape from my review.[WinError 2] … 'C:\Users\bob\…',/tmp/ws/tasks/task-1.txt,/var/tmp/x,open('/root/x'), andc:/Users/bob/xnow all return True. The negatives stay False:https://example.com/tmp/x,https://github.com/root/repo, "the /tmp directory",D:\Data\x. - 13/13 tests pass.
- Mutation check: with the regex reverted to the 9695087 set, the suite fails (1 failure). The file was restored afterwards.
- The vendored copy is byte-identical, and
packages/ag2-sparrow/tools/sync_from_src.py --checkreports in sync.
Why COMMENT and not APPROVE: there are no approvals at this head yet, so the approve bar (one expert approval at the head) is not met. CI is also still pending at this sha; license/cla is success. My CR from 9695087 still shows until I approve. I will approve once an expert approves this head and CI is green. The live Slack round trip is still owed (named, not blocking).
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 0b115fb. Each fix sits where its policy lives, and I found no code blockers:
- Slack reply leg.
_guard_replyrunsguard_result_for_tierbefore any marker parsing, as Discord does. A route with no tier re-reads it from the task file, and a missing file guards as guest. A suppression-only body ([no-send]) is still honored first inclassify_result_for_tier, beforescan_team_result, so a guarded[no-send]that names a path stays silent and doesn't become a withheld notice to the non-owner. - Detection.
carries_internal_error_detailscans only the marker-stripped body, so an admitted[file:]path still delivers. The path roots are anchored ((?<![\w.~/-])), so URLs and repo-relative paths pass. The owner tier is exempt. The vendoredteam_result_guard.pyis byte-identical (sync check). slack_owner. An unmapped member is owner only in a legacy file with notierMapkey, which now matchesresolve_access_tier. That closes the wrong-recipient owner notice.
Approval is held for two things:
- REVIEW.md §15.
slack-bridge.pyresult_watcheris the live Slack delivery loop, and the evidence is the stubbed-SDK test only. I need one post-restart round trip on a real Slack bridge: an owner DM reply delivered unchanged, and a team DM reply carrying an[Errno …]path withheld with the notice. - @liususan091219's CHANGES_REQUESTED at 9695087 is still the active state. Please re-review the point-by-point answers at this head.
Non-blocking: the withhold is all-or-nothing. A guarded reply that legitimately quotes /tmp/... or ~/Library/..., for example a collaborator debugging their own setup, is withheld in full with the generic notice. That's the right default for a P0 privacy fix. A redact-in-place variant could follow later if it turns out to cost real answers.
sonichi
left a comment
There was a problem hiding this comment.
Comment: not approvable yet at 0b115fb.
Verified
- The vendored
team_result_guard.pyis byte-identical tosrc/policy/egress/result.pyand covered by the drift test. - Owner tier is untouched (
is_guarded_tier, result.py:79), and the owner reply is delivered byte-identical. - The new test passes 13/13.
- Kill controls: dropping
slack-bridge.py:1748fails 3 tests, droppingresult.py:241fails 4, and the baseslack_owner.pyfails 1.
Blocking
- A second copy of the tier fallback, already drifted.
slack-bridge.py:591-594re-implementsdiscord-bridge.py:4992-4994(unknown, then task file, then guest) but addsfind_archived_task. Move it intopolicy/egress/result.pyand point both bridges at it. - Only the reply leg is covered. The guard keys on the asker's tier, so proactive bodies still reach any room unguarded:
- Slack sends proactives as
access_tier="owner"(slack-bridge.py:1841), and_send_replyhonors[channel:](:1508). - AG2's
_proactive_routesends[channel: !room](remote_gateway_bridge.py:5553).
Guard by destination, or narrow the PR's claim and file the follow-up.
- Slack sends proactives as
Non-blocking
- The detector withholds ordinary answers ("mkdir -p /tmp/build", "/home/ubuntu/app") but misses relative-path errors ("FileNotFoundError: tasks/task-1.txt").
- With an AG2 collaborator's filter off (remote_gateway_bridge.py:1526), the check is skipped in shared rooms.
- The
task_disposition.pydocstring allowsmissingto go to "the owner's DM", but CLAUDE.md says log only. - The tests cover Slack only, not Discord or AG2.
task_disposition used existence-only find_result, so an empty or whitespace-only result file (live or archived) read as `answered` and masked a pending or missing task. Reuse delivery.task_dispatch's find_ready_result, which walks every candidate until a ready body. Regressions: empty live result, empty archived result, and a ready archived result behind an empty live placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed the P2 on Defect: Fix: it now calls the shared Regressions (
Evidence:
|
Slack and Discord each restated "unknown tier -> task file -> guest", and the copies had drifted: only Slack also looked in the task archive. Move the policy into resolve_route_tier in policy/egress/result.py (vendored to ag2-sparrow) and point both reply legs at it, so Discord now honours an archived task's tier instead of reading it as guest. Tests pin the helper's contract (carried tier, live/claimed/archived task file, guest when absent) and each bridge's delegation: a spy on resolve_route_tier fails if either reply leg stops calling it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed review 5608947343 at 28f37b1. 1. Duplicated tier fallback. There is now a single helper:
2. Proactive destination scope. I narrowed the claim rather than adding destination guarding here. The PR body now says the fix covers the task reply leg, plus the proactive leg only where it resolves to the owner's DM. Channel-redirected proactives (Slack The non-blocking notes are untouched in this push. |
…ridge Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at 7c5a37e. The delta is right:
resolve_route_tierinsrc/policy/egress/result.pyis now the one fallback: route tier, then the live, claimed or archived task file, then guest. Slack's_guard_replyand Discord'spoll_resultsboth call it, and the inline copies are gone. Discord now finds archived tasks too, andtests/discord-bridge-route-tier-delegation.test.pypins that delegation. The vendoredteam_result_guard.pyimports.task_archiveand.local_task_protocol, and both exist inag2_sparrow, so the packaged copy still imports.task_dispositionnow usesfind_ready_result, so an empty or not-yet-published result no longer counts as answered. That's the Codex-core P2.- Proactive scope is narrowed to the owner-DM leg, with the channel-redirect case linked to #4971 and #5341. That's a fair boundary.
Still a COMMENT. Gate 2 is gone, as agreed: Susan approves after me. What remains is REVIEW.md §15, the real-Slack post-restart round trip on this head. Post it and I'll approve.
|
@cla-assistant check |
|
The latest review from @qingyun-wu at 7c5a37e confirms that the code changes are correct and the test suite is passing. The only remaining step before approval is a manual, live round-trip test on a Slack bridge to verify the fix in a real environment. The PR appears ready for that final validation step. |
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 7c5a37e. The delta since my 0b115fb review (3 commits, linear) is correct.
Verified locally at this head:
- One tier fallback.
resolve_route_tier(src/policy/egress/result.py) is now the only place that resolves a reply's tier: the route's tier, else the live or archived task file, else guest. Slack_guard_replyand Discordpoll_resultsboth call it, and their inline copies are gone. Discord previously did not look in the archive; now it does. The vendored copy is byte-identical,sync_from_src.py --checkreports in sync, andag2_sparrowships bothtask_archive.pyandlocal_task_protocol.py. task_dispositionusesfind_ready_result. An empty or unpublished result no longer counts as answered.- Tests. recovery-error-never-to-non-owner, discord-bridge-route-tier-delegation and team-result-guard all pass. I made two deliberate mutations, each reverted afterwards, and both were caught. Dropping the archived lookup from
resolve_route_tierfails both the Discord delegation test and the recovery suite ("an archived owner task was guarded as non-owner"). Restoring "any result file counts" intask_dispositionfails 2 recovery tests. - CI. The one red job is
tests/skills/worker-pool/pool-restart.test.py, a 60-second timeout waiting for disposable runtime state. This PR does not touch that path, and it is the same flake seen on #5805. license/cla is success (1 context).
Why COMMENT and not APPROVE: there is no expert approval at this head yet. Qingyun is holding hers for the REVIEW.md §15 real-Slack post-restart round trip. My CHANGES_REQUESTED from 9695087 still stands until I approve, and I will approve once she approves this head.
— Sutando (susans-MacBook-Pro)
|
Live Slack round trip on the PR build, at head 7c5a37e, on a real install with a real Slack workspace. Setup: the installed engine is exactly this PR's base (5e632bf). I put this PR's head versions of the four changed runtime files over it (src/slack-bridge.py, src/slack_owner.py, src/policy/egress/result.py, and src/task_disposition.py, which is new), and ran the real Slack bridge (Socket Mode) from that tree against the live workspace. Leg 1, owner (pass): the owner sent the bot a real Slack DM. The bridge created an owner-tier task, and the plain-text result was delivered unchanged. Bridge log: No Leg 2, team tier (pass): a route marked The withheld notice was posted in place of the path. Limits:
|
|
@qingyun-wu the live Slack round trip you asked for is in the comment above, and both legs pass at 7c5a37e. @liususan091219 your CHANGES_REQUESTED from 9695087 still blocks the merge. The wider error/path patterns you asked for are in this head (2b59c75). Could you re-review or dismiss it? |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at 7c5a37e. The REVIEW.md §15 gate is met by the live Slack round trip (issuecomment-6342587669). It was the real Socket Mode bridge on a real install, with this head's four runtime files over the PR's base:
- Owner leg. A real owner DM was delivered unchanged, with no
[team-guard]line. - Team leg. A route marked
access_tier: teamwent through the persisted pending-replies, the bridge restart reload,result_watcherand_guard_reply. Its body carried an[Errno 2] … '<absolute path>', which was withheld asinternal error detail, and the generic notice was posted instead.
The team-tier route was synthesized rather than coming from a real non-owner sender, which is a fair limit. This PR doesn't change inbound tier resolution, and resolve_route_tier plus the proactive-owner fix are pinned by unit tests. Code review as in my two earlier reviews.
sonichi
left a comment
There was a problem hiding this comment.
Approved at 7c5a37e on the owner's word. The tier fallback now has one owner (resolve_route_tier in src/policy/egress/result.py), shared by the Slack and Discord bridges. Proactive and [channel:] bodies are explicitly out of scope, with the destination-tier guard tracked in #4971. The red shard is the pool-restart flake, outside this diff.
Master DB row rmvir6xy8xkiqa0 (P0).
Bug
On an install at engine de81346 (desktop 0.7.16), the Slack bot posted this into a team member's Slack DM:
The task it names came from the owner's DM; it was answered about a minute later and archived normally. Two defects: a task that had already been claimed/archived was treated as fatal missing state, and the raw error (a local path plus a task id) reached a non-owner as plain text.
Cause
The sentence is not in any shipped code:
git log --all -Sover sonichi/sutando (main, de81346, every branch) and a byte search of the desktop app bundle and the installed engine both return nothing. It was composed by the core model. What let it leak is in code:src/slack-bridge.pyresult_watchersendsresults/<id>.txtto the asker's channel with no tier check. Discord (src/discord-bridge.py,guard_result_for_tierbefore any marker) and the other bridges do run it. A non-owner Slack reply went out verbatim, markers and all.policy/egress/result.scan_team_resultonly checked delivery markers and secrets, so even a guarded bridge would have delivered a raw[Errno …]naming a local path.src/slack_owner.pyread an allowlist member missing fromtierMapas owner (tier_map.get(uid, "owner")), whileresolve_access_tierin the bridge reads the same member asother. With notofuOwnerand that member listed first, a proactive owner notice went to them.Fix (where each policy lives)
src/slack-bridge.py: new_guard_replyrunsguard_result_for_tieron the reply leg before any marker is parsed (a route without a tier re-reads it from the task file; no task file → guest), the same as Discord.src/policy/egress/result.py(vendored intoag2_sparrow/team_result_guard.pyviasync_from_src.py):carries_internal_error_detailwithholds a guarded result whose visible text quotes[Errno N], a traceback, or a path under a per-user/temp root. Marker values are excluded (parse_markers(...).body), so an admitted[file:]stays deliverable. Owner tier is exempt; the collaborator filter-off opt-out still applies. The guarded reader gets the existing generic withheld notice, and the log line names the reason.src/slack_owner.py: unmapped members are owner only in a legacy file with notierMapkey, matching_ensure_tier_map_seeded/resolve_access_tier.src/task_disposition.py(new):task_disposition(workspace, id)→answered | claimed | pending | archived | missing, built from the existing lookups (find_resultcovers live, flat, epoch-suffixed andarchive/YYYY-MM/results;find_task_filecovers claimed variants;find_archived_taskcoverstasks/archive/and its month partitions). CLAUDE.md/AGENTS.md watcher line: a missing named file is resolved first; onlymissingis an error, and it goes to the log, never as a result or into a conversation.docs/access-control.md+ cataloglast_verified: states the guard covers Slack and internal error detail.Look first at:
src/slack-bridge.py_guard_reply, thensrc/policy/egress/result.pycarries_internal_error_detail.Evidence
New
tests/recovery-error-never-to-non-owner.test.pydrives the real Slackresult_watcher(SDK stubbed) with the incident-shaped body.Before (main, with
src/task_disposition.pycopied in so the import resolves):On main without the new module the file fails to import (
task_disposition.pydoes not exist).After (this head):
Covered: an archived (
tasks/archive/YYYY-MM/) task →archived; an epoch-suffixedresults/archive/YYYY-MM/result →answered; a claimed variant →claimed; an owner reply is delivered byte-identical; a[file:]marker path is not prose; URLs and repo-relative paths pass; legacy no-tierMapaccess files keep the grandfathered owner.Suites run locally:
tests/*slack*,*team*,*guard*,*withheld*,*collaborator*,*dedup*,src-map,agents-md-sync, …): 0 failures.packages/ag2-sparrowadapter tests that import the vendored guard (pytest): 360 passed.sync_from_src.py --check: in sync.scripts/review-checks.sh --diff <(git diff origin/main...HEAD): PASS.One regression found and fixed in development:
collaborator-attachment-gatecaught the first detector flagging an admitted[file: /var/folders/…]. It now scans the marker-stripped body.Risks / gaps
slack_owner.resolve_proactive_owner_idno longer picks an unmapped member).[channel:]redirect (Slack_send_reply) or to a room (the AG2 gateway's_proactive_route) is not guarded by destination. Follow-up: guard proactive delivery by destination. The duplicate gate flagged open issues on the same path, so it is tracked there rather than in a new issue: slack-bridge ignores the channel-redirect marker on proactive result files #4971 (Slack proactive[channel:]handling) and Other delivery bridges (Discord/Slack/Telegram/dm-result) still parse_markers() without skip_after_channel — same #5331 symptom #5341 (bridges' proactive marker parsing).🤖 Generated with Claude Code