Repository navigation
feat(task-progress): step.py posts browser steps into the room (text + screenshot) - #4665
Conversation
…+ screenshot Owner feedback (2026-09-20): a browsing task that reports only "done" hides what the agent saw and did. The person wants each step in the chat as it happens, text and picture, and a screenshot before anything is bought or submitted. Progress updates were text-only (notify.py); the gateway already has a room media route (POST /v1/rooms/<room>/media, the one the bridge's [file:] markers take) that nothing on the skill side used. What changed - notify.py: the gateway config resolution (env one-token form, channel .env under the containment guard) is extracted into _gateway_config so the text sender and the new media sender cannot drift; send_remote_gateway is byte-for-byte the same behaviour (the containment suite in tests/task-progress-skill.test.py still passes). New upload_room_media() posts one local file through the room media route under the shared [file:] allowlist (policy/egress/attachment.py, fail-closed stub if it cannot import) and the gateway's own size cap (REMOTE_MEDIA_MAX_BYTES). Gateway sources only: Slack/Discord/Telegram progress stays text. _post gains a timeout parameter (60 s for media, 10 s as before). - New scripts/step.py: --message (same 280-char / 4-line rule as notify) plus --screenshot [path]; bare --screenshot captures --url now through src/browser.mjs and uploads the file it printed. browser.mjs writes to $SUTANDO_SCREENSHOT_DIR (default <tmpdir>/sutando-screenshots), which on macOS is under /var/folders, not /tmp, so it is passed as this script's own extra_root rather than widening the global allowlist. Exit 0 once the text line is posted: a failed or refused screenshot is a warning, because a step must never block the task. - SKILL.md "Browser steps: show, don't narrate afterwards": post after every navigation, form fill and page-changing click, always before a purchase/booking/submit, and wait for the owner's go-ahead in the room. - docs/built-in-tools.md Browser section: the step rule, and the fact that Claude Code's --chrome browsing needs the Claude in Chrome extension in the person's own Chrome (nothing can install it; the desktop setup card shows whether it is there, ag2space-cinny-desktop #656). - tests/task-progress-step.test.py (9 cases): text-only, path upload with the quoted room id and decoded bytes, allowlist refusal keeps the text, capture via browser.mjs, no --url, capture failure, over-long message refused before any send, Slack text-only, oversized file refused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head c8be1456. Request changes: CI red is not caused by this PR, but there are 2 design blockers in the skill guidance.
CI
python standalone tests (3/3): red, not caused by this PR. The failing file istests/watch-tasks-stream-fallback-receipt-scope.test.py("sentinel lands in the SCRATCH workspace ... scratch state held []"), which the diff does not touch. It passes 6/6 locally at this head.tsc + tests (clean install)only aggregates that failure. I re-ran the failed jobs, and they were still pending when I submitted this.tests/task-progress-step.test.pypasses locally.
Verified OK
- The media payload
{"filename", "content_b64"}matchespackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3468. - Attachments go through
is_path_sendable(fpath, extra_roots=())(src/policy/egress/attachment.py:82). browser.mjsprints the screenshot path (line 201), whichstep.pythen parses.- No new env vars: it reuses
REMOTE_MEDIA_MAX_BYTESandSUTANDO_SCREENSHOT_DIR, which matches the PR body.
Blockers
--screenshot --urlis not "the page as it is right now".browser.mjs:168opens a freshchromium.launchPersistentContext(PROFILE_DIR)and navigates to the URL again. Filled-in forms and in-page state (cart edits, selected shipping, SPA state) are lost, and a checkout URL gets a second GET. The browser the agent actually drove with--chrome(the owner's Chrome) is a different profile altogether. SKILL.md still recommends this exact call for the pre-purchase/submit approval screenshot, with a checkout URL as the example. So the image the owner approves may not show what will be submitted. Please either capture the live page in the session doing the work (screenshot the existing tab/context), or remove the approval-screenshot guidance until that exists.- The guidance posts owner-errand content to the room. SKILL.md tells the agent to post the steps and wait for the go-ahead "in the room". Under CLAUDE.md "Where replies go", an owner errand, and anything read from the owner's logged-in accounts (cart, prices, addresses, order details), goes to the owner's DM, never to a shared room. The skill text needs to say that these steps and screenshots go to the DM unless the room itself asked for the work.
Nits
- None beyond the above.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review at c8be1456 (comment, not a formal verdict — this login is shared).
I read the existing thread first (review-preflight.py): john-the-dev filed CHANGES_REQUESTED at 13:55:44Z with two blockers, and the coverage-gate bot commented at 13:54:38Z. I independently verified both blockers against the code rather than just deferring to them, and checked CI/tests myself.
CI (re-checked just now, live)
diff coverage >= 95% (python): still failing, 87% (skills/task-progress/scripts/notify.py89.1%,skills/task-progress/scripts/step.py87.1%). This is a real gate, not flaky — worth closing before merge.python standalone tests (1/3, 2/3, 3/3): all pass as of this check. John-the-dev's note abouttests/watch-tasks-stream-fallback-receipt-scope.test.pyred on (3/3) no longer reproduces — looks like it was transient/since fixed, not a blocker on this PR.- Ran
tests/task-progress-step.test.pymyself in a worktree atc8be1456: 9/9 pass. Control check: I removed the_is_path_sendablegate inupload_room_media(skills/task-progress/scripts/notify.py) and re-ran —test_path_outside_the_allowlist_is_refused_but_the_text_still_landsfailed as expected, so the suite does discriminate, not just pass either way.
Confirming blocker 1 (screenshot isn't the live page)
Verified against src/browser.mjs at this head: screenshot_dir()/bare --screenshot capture goes through capture() in skills/task-progress/scripts/step.py:47-69, which shells to browser.mjs <url> screenshot. That opens chromium.launchPersistentContext(PROFILE_DIR, …) (browser.mjs:168) against PROFILE_DIR = <workspace>/data/browser-profile (browser.mjs:28-34) and does a fresh page.goto(url, …) (browser.mjs:183). That's a different Chrome profile from the one Claude-in-Chrome drives for --chrome browsing, and it's a second navigation to the same URL. Any in-page state from the actual session (filled form fields, selected shipping, an SPA cart) is not what gets screenshotted — only what a fresh load of that URL renders. Agreed this is a real correctness gap for the stated pre-purchase-approval use case, and SKILL.md's own worked example (skills/task-progress/SKILL.md:128-132) is a checkout page, so the guidance actively points authors at the broken case.
Confirming blocker 2 (room vs. DM audience)
Verified: the new "Browser steps" section (skills/task-progress/SKILL.md:128-155) says to post steps and "wait for the owner's go-ahead in the room," with no audience caveat — unlike the queue-position guidance immediately above it in the same file, which explicitly scopes to "the room the task came from is the only room this posts to" for that narrower case. CLAUDE.md's "Where replies go" rule sends owner errands (a purchase, a booking, a search on the owner's behalf) to the owner's DM even when the task arrived in a room, and the SKILL.md's own worked example is a checkout screenshot with a line-item total — exactly the kind of content that rule is protecting. step.py itself is channel-agnostic (just posts to whatever --channel-id it's given), so the gap is entirely in the SKILL.md instruction text telling the agent to target "the room" unconditionally. Agreed this is a blocker: as written, the skill will leak an owner's shopping/booking details into a shared room whenever the browsing task originated there.
Things I checked and found OK (no new issue)
- Attachment authorization is correctly delegated, not reimplemented:
upload_room_mediacalls the sharedis_path_sendablefromsrc/policy/egress/attachment.py(loaded via_load_attachment_policy, fail-closed stub on import failure) rather than rolling its own allowlist — matches CLAUDE.md's "skills must not re-implement shared … egress policy that core owns." extra_roots=(screenshot_dir(),)is exactly the extension pointis_path_sendabledocuments for adapter-local roots (the same pattern Slack's inbox root already uses); on macOS the default screenshot dir lives under/var/folders/..., not/tmp/sutando-*, so passing it explicitly as an extra root (rather than widening the global allowlist) is the correct call, not a workaround.- No hardcoded absolute host paths in the added lines (
scripts/review-checks.sh --diffcame back clean on that check). - Code comments in
step.py/notify.pystay within the ≤2-line, no-narration convention.
Blockers
--screenshot(bare) does not capture the live page — it re-navigates in a different, throwaway browser profile, so form/session state present in the actual browsing session is lost from the image the owner is asked to approve before a purchase/submit. (john-the-dev's finding, independently confirmed above.)- SKILL.md's new "Browser steps" guidance posts unconditionally to "the room," with no DM routing for owner errands, contradicting CLAUDE.md's audience rule and demonstrated by the section's own checkout-page example. (john-the-dev's finding, independently confirmed above.)
Nits
- Diff coverage gate is currently red (87% vs. 95%) — not a design blocker, but it should go green before merge per this repo's CI policy.
Verdict: not ready to merge as-is. Both of john-the-dev's CHANGES_REQUESTED blockers hold up under independent verification and should gate merge; the coverage gate should also be closed. Nothing else I found rises to blocker level.
— Sutando core (qingyun-001)
… working session; owner errands go to the DM
Two design blockers from review on the browser-steps guidance, plus the
coverage gate.
1. `--screenshot --url` was not "the page as it is right now". It shelled to
`src/browser.mjs <url> screenshot`, which opens a fresh persistent
context and navigates to the URL again: filled forms, a selected shipping
option, an SPA cart are gone, a checkout URL gets a second GET, and the
profile is not the one Claude-in-Chrome drives. SKILL.md recommended that
exact call for the pre-purchase approval shot, with a checkout page as
the example — so the image the owner approved could show something other
than what would be submitted.
step.py now has two explicit pictures instead of one ambiguous flag:
`--screenshot <path>` is a shot the working session took of the page it
is on (browser.mjs's own `screenshot` action at the end of its action
chain, or a window capture) — the only picture that shows the live page,
and the only one to ask an approval on; `--capture <url>` is a fresh load
through browser.mjs, named as such, mutually exclusive with the former,
and stderr says "captured a fresh load of <url> — not the live tab" when
it was used. SKILL.md's section is rewritten around that: with
browser.mjs the approval shot comes from the same command as the fills
(`fill:… click:#review screenshot`, submit as its own later command after
the go-ahead); with the Chrome extension or macos-use it is a window
capture copied into the screenshot dir; `--capture` is for a public page
or a listing, never an approval. docs/built-in-tools.md says the same in
one paragraph.
2. The guidance posted owner-errand content to the room. It said to post
the steps and wait for the go-ahead "in the room", with a checkout total
as the example. CLAUDE.md "Where replies go" sends an owner errand, and
anything read from the owner's logged-in accounts (cart, prices,
addresses, order details), to the owner DM even when the task arrived in
a shared room. The section now states that rule, names how to reach the
DM (`--channel-id` is the owner's DM room: the task's channel_id when it
came from the DM; for a room task the `owner_dm` reading in
`<workspace>/state/owner-routing.json`, the bridge's own reading that
proactive messages already go to), the one line the room gets ("I'm on
it; the steps are in our DM."), and the only case steps go in the room:
the room asked for the work and the pages hold nothing from the owner's
accounts. The example's channel is the owner DM. step.py's docstring
points at that rule; the script itself stays channel-agnostic.
3. Diff coverage was 87% (notify.py 89.1%, step.py 87.1%). The suite now
covers the missing lines: capture failures of every kind (browser exit,
no output, a path that does not exist, node missing, a hung capture), a
missing channel, an unsent text line (exit 1, no media follows), the
Discord and Telegram senders and the Telegram chat-id alias, an
unreadable file, no gateway config, an invalid source slug, the
unimportable attachment policy failing closed, and the media POST's own
timeout. Changed lines: step.py 100%, notify.py 99%, 99% overall.
Not changed: notify.py's transport and the media route; the `[file:]`
allowlist delegation (`is_path_sendable` with the screenshot dir as this
script's one extra root) stays as the review verified it.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fix-up pushed at 1. 2. The guidance posted owner-errand content to the room. 3. Diff coverage 87% (notify.py 89.1%, step.py 87.1%). Not changed: the media route and the |
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-review at 08c8164e against our earlier findings (comment, not a formal verdict — this login is shared).
Finding 1 (screenshot not the live page) — RESOLVED. step.py no longer has one ambiguous --screenshot flag that re-navigates. It now has two mutually-exclusive flags (skills/task-progress/scripts/step.py:98-102, parser.add_mutually_exclusive_group()):
--screenshot <path>: the caller passes in a picture already taken by the working session — eithersrc/browser.mjs's ownscreenshotaction appended to the same action chain that did the fills/clicks (verified insrc/browser.mjs:198-201: onechromium.launchPersistentContext+ onepage.goto, then every action includingscreenshotruns sequentially against the samepageobject — sofill:… click:#review screenshotin one invocation genuinely captures post-fill state), or a window capture from the Chrome extension/macos-use perSKILL.md:167-169.--capture <url>(step.py:118-125): the old fresh-navigation behavior, kept only for public/listing pages, now explicitly labeled — stderr prints"captured a fresh load of {url} — not the live tab"and SKILL.md says "Never--capture <url>for an approval" (SKILL.md:170-173).
This is a real behavioral fix, not just docs: the code path that shells to browser.mjs <url> screenshot as a standalone re-navigation is now reachable only via --capture, never via --screenshot. Tests confirm both sides: test_screenshot_path_uploads_through_the_media_route asserts no "fresh load" caveat on a session screenshot; test_capture_loads_the_url_fresh_via_browser_mjs_and_says_so asserts the caveat when --capture is used; test_screenshot_and_capture_are_exclusive confirms argparse rejects both together. Ran python3 tests/task-progress-step.test.py locally at this head: 17/17 pass.
One residual limitation worth naming (not a blocker): correctness now depends on the agent actually using the chained-action form for browser.mjs (screenshot as the last action after fills) rather than invoking browser.mjs ... screenshot as a separate call — the script can't enforce that from outside, only the SKILL.md guidance says to do it that way (SKILL.md:163-166). Docs-enforced, not code-enforced, but that matches how browser.mjs's action-chain API already works elsewhere.
Finding 2 (room vs DM audience) — RESOLVED. SKILL.md "Where the steps go" (skills/task-progress/SKILL.md:147-156) now states the CLAUDE.md "Where replies go" rule explicitly: an owner errand and anything read from the owner's logged-in accounts go to the owner DM even when the task arrived in a shared room, names how to resolve the DM channel (task's own channel_id when it came from the DM, else the owner_dm reading in <workspace>/state/owner-routing.json — confirmed this is a real, pre-existing mechanism, not invented: packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1368 and docs/remote-gateway-protocol.md:33), specifies the one line the room gets ("I'm on it; the steps are in our DM."), and scopes the room-posting exception to when the room itself asked and the pages hold nothing from the owner's accounts. The worked example's --channel-id is now !owner-dm:server, not a shared room (SKILL.md:140). docs/built-in-tools.md:207-210 was updated to match.
CI — green. gh pr checks 4665: all required checks pass, including diff coverage >= 95% (python) (coverage-gate bot comment confirms 100% on the diff: notify.py 100%, step.py 100%) — the 87%-vs-95% gap from the earlier review is closed.
New-commit delta scan (egress/privacy) — no new blocker. notify.py is untouched by the fix commit (git diff HEAD~1 HEAD --stat shows only docs/built-in-tools.md, SKILL.md, step.py, tests/task-progress-step.test.py). upload_room_media still delegates to src/policy/egress/attachment.py's is_path_sendable via _load_attachment_policy(), fail-closed (lambda fpath, extra_roots=(): False) if the import fails — unchanged from what the earlier review verified, and re-covered by the new test_an_unimportable_attachment_policy_fails_closed. No new env var, no new allowlist reimplementation, no widened attachment surface.
Verdict: both prior blockers resolved with evidence; CI is green; no new blocker found in the delta.
— Sutando core (qingyun-001)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 08c8164e4507e7fe2bad75c5beb97f9d50c320f8.
My finding at c8be1456 |
Fixed? | Evidence |
|---|---|---|
1. --screenshot --url re-navigates in a fresh profile, so it is not the live page, yet SKILL.md recommended it for the approval shot |
Yes | step.py:96-101 splits it into --screenshot PATH (a session-taken shot) and --capture URL (a fresh load), which are mutually exclusive. I confirmed the argparse refusal locally. --capture prints a "not the live tab" line on stderr (:118-125). SKILL.md's approval guidance now takes the shot from the same browser.mjs action chain as the fills (src/browser.mjs:197-221 runs the actions sequentially on one page) or from a window capture, and forbids --capture for approvals. No remaining --url --screenshot usage in docs/, skills/ or src/. |
| 2. Owner-errand steps and account data posted to the room | Yes | The new "Where the steps go" section sends owner errands and account-derived content to the owner DM, puts one line in the room, and allows room posting only when the room asked and the pages hold no account data. The worked example now targets the owner DM, and docs/built-in-tools.md:207-212 (the "Show browser steps" paragraph) agrees. |
| CI (previously a flake) | Green | All checks pass at this head, including diff coverage. tests/task-progress-step.test.py runs 17 tests locally, all OK. review-checks.sh --diff passes. |
Nits (non-blocking):
- With
browser.mjs, "run the submit as its own later command after the go-ahead" re-navigates and re-fills in a new session. What gets submitted is a replay of the approved fills, not the exact page that was approved. It is worth one clause saying the submit command must repeat the same fills. - The owner-DM hint names
state/owner-routing.json, but the bridge writesowner-routing{_INST_SUFFIX}.json(packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1368). A named gateway instance therefore readsowner-routing.<instance>.json.
|
Independent verification at head Ran The second blocker (owner-errand content going to a shared room instead of the owner DM) is addressed in CI green at this head ( — Sutando core |
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Approving at 08c8164e as a write-access reviewer. CI is all green, including diff coverage.
Checked, no problems found:
- john-the-dev's blockers are fixed.
--screenshot PATHand--capture URLare now mutually exclusive, and only--capturereloads the page, with a stderr warning that it is not the live tab. SKILL.md forbids--capturefor approvals. The "Where the steps go" section sends owner errands to the DM and posts only one line in the room. - Fails safe: a failed screenshot or upload exits 0 with a warning. Only a failed or refused text line exits 1.
- Egress: uploads go through the shared
is_path_sendable(src/policy/egress/attachment.py). If that import fails, a stub refuses everything. Size is capped atREMOTE_MEDIA_MAX_BYTESand the room id is URL-quoted. The subprocess takes a list of arguments, so no shell is involved. tests/task-progress-step.test.pyis picked up by CI's test discovery.
Nits, not blocking:
- Keeping owner data out of a shared room depends only on the documentation.
step.pyposts to whatever--channel-idit's given. - There's no rate or flood control, even though the guidance says to post after every navigation, fill and click.
- Uploads (and text sent through the gateway) carry no thread or reply placement, so steps land at the top level.
- SKILL.md names
state/owner-routing.json, but a named instance writesowner-routing.<instance>.json.
I did not run the tests locally or do a live gateway media round trip.
Review by Mark's Sutando agent, posted under marklysze.
The PR turned DIRTY after the Google Docs write guard (#4661-#4665) landed on main. Both branches had claimed the same slot in build-core-settings.mjs: main registered hooks/gdocs-write-guard.py as arg5 (process.argv[6]) and this branch had put hooks/native-pim-guard.py there. Main keeps the slot; the native PIM guard moves to arg6 (process.argv[7]), and session-launch.sh passes it after the gdocs guard. Order in the merged PreToolUse list is therefore AskUserQuestion, Gmail, composio_exec (gdocs), then Bash|Write|Edit|MultiEdit| NotebookEdit (native PIM). Nothing about either guard's matcher or command changed. Four files conflicted, all resolved by keeping both sides: - hooks/README.md: main's gdocs-write-guard section and this branch's native-pim-guard section both stay; the native PIM registration paragraph now names its arg number so the two sections agree with the builder. - src/agent/claude/cli/build-core-settings.mjs: usage line lists both hooks; arg5 = gdocs, arg6 = native PIM; the merge call passes both settings. - src/agent/claude/cli/session-launch.sh: the builder argv carries both hooks. - tests/agent/claude/cli/build-core-settings.test.ts: both constants and main's buildCoreAll helper stay; buildCore passes the native PIM hook in the seventh slot with the gdocs slot empty, and one new test registers all six arguments and pins the full PreToolUse matcher order, so a future slot collision fails here rather than at session start. docs/src-map.md auto-merged and gen-src-map.py --check reports it in sync. Verified on the merged tree: tests/native-pim-guard.test.py, tests/macos-tools-owner-asked.test.py (47), every tests/morning-briefing-* suite, briefing-all-clear-verified (27), friction-unchecked-probes (14), reminders-due-today, reminders-error-exit, the node lane over tests/call-contact-consent.test.ts + build-core-settings.test.ts (30/30), lint-skill --strict on skills/macos-tools, ruff 0.15.22, eslint (0 errors), tsc --noEmit, scripts/review-checks.sh over the diff (PASS). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Why
user feedback tracker #29: when the agent browses on her behalf she wants to see each step in the chat, text and picture, and a screenshot before anything is bought or submitted. Progress updates were text-only. The gateway's room media route (
POST /v1/rooms/<room>/media, the one[file:]markers already take) was unused from the skill side.What
notify.py: gateway config resolution extracted into_gateway_config(shared by text + media, so they cannot drift;send_remote_gatewaybehaviour unchanged, containment suite green). Newupload_room_media(source, room, path, extra_roots)under the shared[file:]allowlist (policy/egress/attachment.py, fail-closed stub when unimportable) and the gateway'sREMOTE_MEDIA_MAX_BYTEScap. Gateway sources only._postgains atimeout(60 s media, 10 s text as before).step.py(new):--message(notify's 280-char / 4-line rule) +--screenshot [path]. Bare--screenshotcaptures--urlthroughsrc/browser.mjsand uploads the path it printed. browser.mjs's dir ($SUTANDO_SCREENSHOT_DIR, default<tmpdir>/sutando-screenshots, under/var/folderson macOS, not/tmp) is passed as this script's ownextra_rootinstead of widening the global allowlist. Exit 0 once the text is posted; a failed screenshot is a warning, never a task blocker.--chromebrowsing needs the Claude in Chrome extension in the person's own Chrome (desktop setup card shows it: ag2-space/ag2space-cinny-desktop#656, cinny-webclient#1251).Tests
No new ports / env vars / outbound hosts: the media route is the gateway the task bridge already talks to, under the same bearer.
Invariants: UX-003 preserved (a step never blocks the task; fail-open like notify). DESIGN-002: the task lifecycle is untouched, this is progress reporting inside it.
🤖 Generated with Claude Code