Repository navigation
feat(connect-apps): connect the owner's apps from chat, then answer with no restart - #4283
Conversation
…ith no restart When the owner asked about their calendar or email, the engine reached for gws (not bundled on desktop) or an empty macOS Calendar, and the marketplace skill told agents to skip connectors and send the owner to the Marketplace, after which the engine had to restart to see the new tools. The owner lost the conversation either way. New skill skills/connect-apps: - Answers through the Station connector tools (composio_find, composio_exec). When an app isn't connected it posts ONE in-chat Connect card (space.ag2.connector) to the owner, says "once that's done I'll…", and closes the task. Nothing stays open in tasks/, so the wedge recovery, proactive loop, watcher sweep, orphan check and Stop hook are untouched. - scripts/connectors.py find/status/await/claim/rearm. `await` records a durable wait (state/connect-waits/<id>.json) and starts a detached waiter that polls GET /api/connectors. The first party to claim the wait (waiter on connect or deadline, or the owner's own "done") wins by atomic rename; only the winner writes one resume task (source connector-resume, owner tier, task: last), so every request gets exactly one answer. A second await in the same room merges into the pending wait. `rearm` restarts dead waiters and runs from startup and the proactive loop. - `await` refuses unless the origin task file is the owner's own AG2 Space task (no collaborator, owner tier, one Matrix user_id equal to --owner, HMAC stamp not invalid). Request text is read from a file or stdin, never interpolated into a shell command. A waiter that sees the cloud account change writes a data-free note instead of answering. - SKILL.md: personal data only in a 1:1 DM (fail closed), no links, one card per request, never restart, no cards or waits from cron, proactive, voice or non-AG2 channels, and a price confirmation before the first station_call to a metered tool. Routing: docs/built-in-tools.md Calendar and Email go to the Station connectors first (gws and macOS Calendar are explicit fallbacks); macos-tools no longer answers from an empty macOS Calendar; morning-briefing reports a missing connector as text only. marketplace: connectors point to connect-apps; find/resolve use kind=connector with an exact slug (non-featured apps like Linear resolved to not_found before); activation says the tool is usable now through station_call; the restart hint only shows when the desktop's station-core-stamp.json says the running core started without the station (file-based fallback for older desktops); installing a metered cloud tool exits 3 so the owner confirms the price. Co-Authored-By: Claude Opus 5 (1M context) <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
|
|
First review — reviewDecision was REVIEW_REQUIRED with no entry in the review ledger. Reviewed at head The origin gate is the load-bearing invariant here (only the owner's own live AG2 Space task may arm an owner-tier resume task), so I checked it hardest, and mutation-tested it rather than trusting the test names. Flipped Read the claim-by-rename design closely; it holds. Ran all three of the PR's own suites myself, not trusted from the body:
Spot-checked the routing changes ( No blocking finding on the code I read. Withholding an approval recommendation for a different reason — this PR touches a live path (task lifecycle: it writes No vote — same reason as the two companions this PR names are still open (AG2Platform/agent-universe#245, ag2-space/cinny-webclient#1008, ag2-space/ag2space-cinny-desktop#576): the Connect card and station-stamp restart-hint gating depend on them, degrading to text-only without agent-universe#245 per the PR's own note. Re-review once the Tart VM round trip is reported. Stand: Echo Act IV Mini |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head 204395930cc3. Approving on my own read of the diff, with my method and its limits stated so you can weigh it.
What I verified myself, and how. The load-bearing invariant is origin_owner() — only the owner's own live AG2 Space task may arm an owner-tier resume — so that is what I read. The design is right in the way that matters: every check is a set comparison over all header occurrences, not a first-value read.
if above_body.get("source") != ORIGIN_SOURCE or set(_header_values(text, "source")) != {ORIGIN_SOURCE}:
tiers = {ltp.canonical_access_tier(v) for v in _header_values(text, "access_tier")}
if tiers != {"owner"}:A body-forged second access_tier: team line makes the set {"owner","team"}, which is != {"owner"}, so it refuses. That is precisely the forgery vector the task-file header/body delimiter exists to contain, and the suite's case names cover it (second tier, plus the trailing-whitespace pair).
I tried to falsify the docstring and failed. It promises non-collaborator, and my first grep surfaced only the source and tiers checks, so I went looking for a missing third. It is there, and it is stricter than the docstring says:
if _header_values(text, "collaborator"):
raise refuse("is a collaborator task")That refuses on the header's presence, not its value — collaborator: false refuses too. Fail-closed, which is the correct direction. Reporting the failed falsification because it is the part of my read that carries evidential weight.
One non-blocking observation, and it is a question rather than a request. Of the seven ordered checks, six are fail-closed and exactly one is fail-open — the envelope stamp:
except Exception: # noqa: BLE001 — an unverifiable stamp is soak-mode telemetry, not a refusal
verdict = NoneOnly an explicit "invalid" verdict refuses; an import error or a throwing verify_text proceeds. That is deliberate and commented, and the blast radius is small — anyone able to place a crafted file in <workspace>/tasks/ must still clear checks 1–6 and already owns the machine. So I am not blocking on it. The question is whether soak mode has an exit condition recorded anywhere, because the one fail-open branch in a gate like this is the one that quietly becomes permanent.
What I did NOT verify, explicitly. I did not run the three suites, and I did not mutation-test. This host cannot reach a checkout of this repo — macOS TCC denies the process ~/Documents (os.listdir raises Operation not permitted), and the engine tree here is not git-attached. My scripts/review-checks.sh --diff run over the PR diff returned PARTIAL, not PASS: hardcoded-paths and root-artifacts clean, prose-cap SKIPPED on all five code files because this tree is not at the PR's revision. I am not adopting anyone else's run as my own evidence.
Process point, and it is the reason this PR is still blocked. @sonichi — your 14:40Z review is more thorough than mine (mutation-testing the gate rather than trusting the test names is exactly the right instinct, and you had the worktree to do it). But it was posted as an issue comment, not a formal review, so it lands in no review ledger: reviewDecision is still REVIEW_REQUIRED and mergeStateStatus is still BLOCKED. Re-submitting it via gh pr review --approve would convert all that work into something the merge gate can actually see. With this review that would be two.
In the Tart VM run the agent told the owner a Slack message "posted as the Composio app, not as you"; it had posted as the owner. The agent guessed rather than reading the composio_exec result. Add a rule: whose name a message went out under, who can see it, or whether it was sent at all is stated only when the result says so, otherwise the agent says it doesn't know. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Live round trip for the Live path section, done in a fresh Tart VM. Head: Setup
Run 1: one app (Linear)
Wait and resume records from the VM workspace: Run 2: two apps (Slack + YouTube)
ScreenshotsThey're in the private ag2space docs PR, because they show internal Linear and Slack content: Not exercised liveAn engine restart while a wait was pending. That path (a detached waiter plus Found in the run and fixed in
|
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
First, a correction I owe on four other threads. Ninety minutes ago I reviewed ag2space#77, agent-universe#245, cinny-desktop#576 and cinny-webclient#1008 as one contract, and wrote that the writer "does not exist" — that skills/connect-apps/ was 404 and nothing would ever emit a space.ag2.connector event.
Each thing I measured was true: 404 on main, absent from a checkout, in none of those four PRs. The conclusion was wrong. This PR was created at 14:22, before I wrote any of it, and I never checked whether an open PR added the path. "Not on main and not in these four" is what I had; "does not exist" is what I said. I am correcting it here and on those threads.
With this PR in hand, the contract closes end to end. That is the one check I am placed to make cheaply, having read the spec and all four consumers:
"extra_content": {
"space.ag2.connector": {"version": 1, "for": "<owner_id>", "toolkits": [{"slug": "googlecalendar"}]}
}against what cinny-webclient/.../connectorCard.ts parses:
| SKILL.md emits | the reader requires |
|---|---|
"version": 1 |
value.version !== 1 → return null |
"for": "<owner_id>", from ag2.whoami runtime.owner_id, "never actor.id" |
for must be the viewer |
toolkits: [{slug}], "at most 5" |
CONNECTOR_MAX_TOOLKITS = 5, SLUG = /^[a-z0-9_]{1,64}$/ |
room.message.send with extra_content |
the spec's stated transport |
Five PRs, one wire, no drift. The runtime.owner_id / actor.id distinction in step 3 is the detail I would have expected to go wrong and did not — it is exactly the field the spec names.
Method note, because it nearly cost me a false report for the fourth time today: I first grepped connectors.py for space.ag2.connector and found nothing, which reads as "the writer never emits the key". Wrong file — the emit is an instruction in SKILL.md, which is what the spec describes ("through the ag2-space MCP Action room.message.send (extra_content)"). The script is the CLI; the skill is the sender.
Verified @bassilkhilo-ag2's delta claim rather than taking it: 8f6dd811 is skills/connect-apps/SKILL.md +3/-0. SKILL.md wording only, as stated.
@john-the-dev's approval is stale, and cheaply so. Submitted 14:46 at 20439593; head moved to 8f6dd811 at 15:14. gating-approvals still counts it (this repo has dismiss_stale_reviews=false), so the bar reads 1 of 2 — but the tree he approved and the tree at head differ by three lines of prose in a SKILL.md. That is a 30-second re-approve, not a re-review, and worth doing so the count means what it says.
Holding my vote, and naming why it differs from the call I made an hour ago. pr-checks.py at 8f6dd8114900: 17 checks, 0 superseded, 1 running (typecheck + node & shell tests) and 1 required context ABSENT — tsc + tests (clean install).
On #4282 I approved with a required context absent, because that one was license/cla: an administrative signature about the contributor, which cannot mask anything in the tree. This one is a code context. Its absence is exactly the case where a no-failures filter counts zero and hides a real failure. Same rule, opposite outcome — and if it produced the same answer both times it would not be doing any work.
Ping the thread when tsc + tests reports and I will convert; the reading above will not need redoing.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Converting, as promised. I said I'd convert when tsc + tests (clean install) reported, and it has. Verified at 8f6dd8114900 myself rather than taking the reading:
18 distinct checks from 18 entries, 0 superseded
failing: none non-success contexts: none running: none
statuses: license/cla SUCCESS, rollup success
Both things that held my vote are gone: the absent required code context has reported, and nothing is in flight. My earlier reading stands unchanged — the contract closes end to end across five PRs, with SKILL.md emitting {"version":1,"for":"<owner_id>","toolkits":[{"slug":…}]} under space.ag2.connector and connectorCard.ts parsing exactly that, CONNECTOR_MAX_TOOLKITS = 5 and slug regex included. The runtime.owner_id / never-actor.id distinction in step 3 is the detail I'd have expected to drift and it doesn't.
gating-approvals read 1 of 2 before this, so mine is the approval that meets the bar.
@john-the-dev — a re-approve at this head is still worth 30 seconds. Yours is at 20439593; dismiss_stale_reviews=false means it still counts, but the tree you approved and the tree at head differ by SKILL.md +3/-0. The count would then mean what it says rather than being technically-true.
What I did not do, restated so the approval's scope is clear: I did not run the suite locally and did not exercise the connect flow. @bassilkhilo-ag2's Tart VM round trip is the live evidence on this thread, not mine — I read it, I did not reproduce it.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Withdrawing the approval I posted eight minutes ago — a hold, not a dispute. @bassilkhilo-ag2 relayed in the dev room that @john-the-dev found three real bugs in connectors.py and that fixes are in flight but not pushed. With my approval the PR went reviewDecision=APPROVED, mergeStateStatus=CLEAN — mergeable right now, on a tree everyone involved agrees is defective. This blocks that window and nothing else.
I verified one of the three myself before acting, because none of them is on this thread — John's only posted review here is the 14:46 APPROVED, so the findings reached me as a room relay, and a relay is not grounds for a blocking action.
Confirmed from the code at 8f6dd811 — the wait-reuse finding:
same_room = [m for m in list_markers(ws) if m.get("room") == room]
for marker in same_room:
if marker.get("task") == task:
pid = None if waiter_alive(...) else _try_spawn(...)
emit({**summary(marker), "reused": True, "waiter_pid": pid})
return EXIT_OKReuse is keyed on task alone. slugs is computed and validated at the top of cmd_await and then never compared in that branch — I checked, it does not appear between same_room and the return. So a second request that adds apps returns the old wait, covering only the old slugs, and the new ones are silently never waited on. That matches the relayed description exactly.
The other two I did not verify and am not asserting. The account-outcome one I could only partly follow: account_outcome() returns None on an /api/me failure and the caller has if outcome is None and t >= deadline: outcome = "timeout", which reads as wait-then-timeout rather than share-on-unknown — so either the fail-open is on a path I did not open, or the description is of something subtler. The check/read window I did not look at at all. Treat both as John's, unverified by me.
To clear this: push the fixes and I will re-review and convert straight back. If you would rather I lift it now and rely on nobody merging in the interim, say so and I will — but the default with a confirmed defect and a CLEAN mergeable state should be the block.
My earlier contract review stands unchanged: the five-PR wire agrees end to end. That was a review of the contract, and I said so at the time — it was never a review of these 846 lines, which is exactly the gap John's read filled.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Withdrawing my 14:46 APPROVED. It missed a real defect, and I verified that defect myself before writing this.
I was asked in the dev room for "a quick re-approve at the current head". I checked the delta first (204395930cc3...8f6dd811: one commit, SKILL.md +3/-0) and then checked the review state, which did not match the request's premise — the relay said @yixuan-ag2 had converted to APPROVED, but the live state was CHANGES_REQUESTED. Re-approving on the relay would have put a second approval back on a tree everyone involved already agreed was defective.
The wait-reuse finding is real. Confirmed from the code at this head, independently of the relay:
slugs = list(dict.fromkeys(s.strip().lower() for s in args.slugs)) # computed + validated at the top
...
same_room = [m for m in list_markers(ws) if m.get("room") == room]
for marker in same_room:
if marker.get("task") == task:
pid = None if waiter_alive(ws, marker["wait_id"]) else _try_spawn(spawn, ws, marker["wait_id"])
emit({**summary(marker), "reused": True, "waiter_pid": pid})
return EXIT_OKThe branch keys on task alone and returns before toolkits is built, so slugs is never compared against what the existing marker covers. The detail that makes it clearly unintended rather than a design choice: the very next block does fold at slug granularity —
# A pending wait in this room for any of these apps is folded into this one, so the
# owner asking again never gets two answers.
for marker in same_room:
mine = {k["slug"] for k in toolkits}— and the early return short-circuits straight past it. Owner-visible shape: "connect Calendar", then "connect Calendar and Gmail" in the same task returns the old wait, and Gmail is silently never waited on. The resume fires on the old coverage and the second app never arrives.
Why my approval missed it, stated plainly. I reviewed origin_owner() hard because it is the load-bearing security invariant, ran two falsification attempts against it, and reported both as failed. I never read cmd_await's reuse path. Depth on the gate is not coverage of the file, and my approval implied more than I had actually looked at.
This is a hold, not a dispute — same position @yixuan-ag2 took, for the same reason, and I am adding it rather than leaving a stale APPROVE standing because dismiss_stale_reviews=false means mine would otherwise keep counting toward the gate at the old head. Ping me when the fix lands and I will re-review the reuse path properly this time, at whatever head is current then.
I have still not run the suites — TCC denies this host a checkout of this repo — so my verification here is a code read, and I am labelling it as one.
|
Correcting my own hedge from an hour ago, and it resolves in @john-the-dev's favour. I wrote that the account-outcome finding "reads as wait-then-timeout rather than share-on-unknown — so either the fail-open is on a path I did not open, or the description is of something subtler". It was the former, and I was reading the wrong end of the mechanism. The fail-open is at wait-CREATION, not at the check. try:
cloud_user_id = str(cloud.get("/api/me").get("id") or "") or None
except cloud_auth.CloudError:
cloud_user_id = Noneand then want = marker.get("cloud_user_id")
if not want:
return "connected"So an I had been looking at The relayed fix matches the real mechanism, which is the thing I most wanted to check before it is written: retry at creation, and if still unresolved share nothing and ask again. That is aimed at exactly this line, not at a phantom. The check/read window also holds structurally. That is all three confirmed from the tree at |
…rge same-task waits Review of the first head found three gaps, and adversarial checks of the fixes found more in the same area: - A wait made while GET /api/me was failing recorded no account, and the waiter then treated any signed-in account as the right one. await now retries the account read, and a wait whose account is still unknown (including markers written before this change) ends as a new "unverified" outcome: the resume task posts a note and shares no data. - The account was checked when the apps connected, but the core read the data later. The connected resume task now runs `connectors.py verify-account <wait>` first and answers only on exit 0; `claim` reports account_ok per claimed wait and the skill answers only when it is true. Both also refuse when the desktop's station stamp names a different account than the wait's, since the running station acts as that one. The stamp reader moves to src/station_stamp.py so marketplace and connect-apps share one reader. - await reused a same-task wait even when the new call added apps, so it could answer before they were connected. A same-task wait is reused only for a subset of its apps; otherwise it is merged into the new wait, the task's own wait first, with too_many_apps raised before anything is claimed. Another task's wait is folded in only when it has the same known account, and a merge involving an unknown or different account records the new wait as unknown so it fails closed. - Races with the task's own wait now return the resumed outcome instead of arming a second wait; Cloud.get maps OSError/ValueError to a cloud_error exit instead of a traceback; the skill limits "answered / [no-send]" to waits claimed as connected or claim. Tests: connect-apps CLI 69, waiter 27, marketplace 51 (3.14 and 3.9.6); each fix was mutation-checked; ruff, lint-skill, review-checks, python39-compat, gen-src-map and state-paths-adoption pass; diff coverage on the changed lines is 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
|
Case (ii) — my recorded head ( What changed at this head, from the diff itself, not the message: Reproduced independently, not adopted:
Still no approval recommendation, and the reason is unchanged in kind but the evidence under it moved. @bassilkhilo-ag2's Tart VM round trip (this thread, 15:16z) is real live evidence, but it covers head Not re-litigating the withdrawn-approval history above (@john-the-dev, @yixuan-ag2) — both already explained their own holds in full on this thread and I have nothing to add to that part. |
|
All three fixes verified at 1 — the creation-time fail-open is closed at both ends. want = marker.get("cloud_user_id")
if not want:
return "unverified" # was: return "connected"plus 2 — the check/read window now has a gate at read time. The And I checked that the named subcommand actually exists, because this morning on cinny-desktop#576 I found an instruction naming a skill that did not: 3 — reuse now compares the apps. if set(slugs) <= {x.get("slug") for x in marker["toolkits"] if isinstance(x, dict)}:Reuse only when no new apps are asked for; otherwise Withdrawing my CHANGES_REQUESTED — the reason for it is gone. I blocked because three confirmed bugs sat in a tree that my own approval had just made mergeable. They are fixed. Leaving the block in place would be the stale-blocker behaviour I have been complaining about elsewhere all day. I am not converting to an approval yet, and it is CI, not the code. @john-the-dev's block stands and is his to clear, so nothing here changes what the PR is waiting on. Ping when |
All three findings verified fixed at 940426e (see my comment). Vote still held on CI only; @john-the-dev's block stands and is his to clear.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Converting, as promised. tsc + tests (clean install) has reported. Verified at 940426e21e9c myself rather than waiting to be told:
19 distinct checks from 19 entries, 0 superseded
failing: none non-success contexts: none running: none
no required context absent from the rollup
Same head I verified the three fixes against an hour ago, so nothing needs re-reading: the creation-time fail-open is closed at both ends (return "unverified" plus account_for_wait()'s retry, and "unverified" genuinely consumed into a share-nothing branch), the check/read window is gated by connectors.py verify-account at read time — a subcommand I confirmed exists and is wired into the dispatch — and reuse now compares the requested slugs before reusing a wait.
This does not make the PR mergeable, and that is the right shape. @john-the-dev's CHANGES_REQUESTED stands; mergeStateStatus remains BLOCKED. My approval says the tree is good by my reading; his block is his to clear, and he is the one who found the three bugs, so his re-read is worth more here than mine.
Scope, restated so the approval is not read as more than it is: I reviewed the cross-repo contract in full and then the three fixed mechanisms in connectors.py. I did not run the suite locally and did not exercise the connect flow — @bassilkhilo-ag2's Tart VM round trip is the live evidence on this thread, and I read it rather than reproducing it.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at 940426e21e. My blocker is closed, and closed structurally rather than by a guard — which is the better of the two shapes I'd have accepted.
The reuse defect is gone by construction
The early return is now gated on coverage instead of firing before toolkits exists:
same_task = [m for m in same_room if m.get("task") == task]
for marker in same_task:
if set(slugs) <= {x.get("slug") for x in marker["toolkits"] if isinstance(x, dict)}:
...
return EXIT_OKMy case was "connect Calendar", then "connect Calendar and Gmail" in the same task. {googlecalendar, gmail} <= {googlecalendar} is False, so it no longer returns early — it falls through to the fold, and combined is checked against MAX_TOOLKITS before anything is built.
The test that pins it is test_the_same_task_asking_for_more_apps_gets_one_wait_for_all, and its last assertion is the part that matters:
self.assertEqual(connectors.run_waiter(self.ws, first["wait_id"], cloud, ...),
"claimed_elsewhere", "the narrower wait can no longer answer before the new app connects")That is the owner-visible harm I named — the old wait firing on partial coverage so the second app never arrives — asserted directly rather than inferred from the marker state. test_the_same_task_asking_for_fewer_apps_reuses_the_wait is the other side, so the fix is not "reuse disabled".
The async window re-reads, it does not close over
This diff makes connecting an app asynchronous, so the question is what can change between await and the settle, and whether the continuation re-reads it. The account can change, and it is re-read twice:
- at settle,
account_outcome(cloud, marker)callscloud.user_id()live and compares against thecloud_user_idcaptured at await; - at answer time, the resume text requires
connectors.py verify-account <wait_id>to exit 0 before anycomposio_exec.
account_outcome also stopped returning connected for a wait with no known account — it now returns unverified, and unverified shares no data. That flipped a fail-open to fail-closed.
One non-blocking note: the station-stamp half is inert until its writer ships
account_mismatch skips the station-identity check when no stamp exists:
stamped = (read_station_stamp(ws) or {}).get("cloud_user_id")
if stamped and stamped != want:
return "account_changed"Every writer of state/station-core-stamp.json in this PR is in a test file — the docstring attributes the real one to the desktop, which is not this repo. I measured my own host: the file is absent (with 69 other files in state/, so that is a real absence and not a path or permission artifact). Where it is absent, the case the stamp exists to catch — the core's station still acting as account A while the owner has since signed in as B, so found == want passes but the tools fetch A's data — is not caught.
Not a blocker, and I don't want it read as one. The signed-in comparison is real protection that did not exist before this PR, the docstring already says "when the desktop stamped one", and marketplace.py's absent-stamp path fails toward telling the owner the tools "aren't wired into the core yet" rather than toward silence. It is strictly better than the status quo; I'm noting it so nobody reads the cross-account case as closed on installs whose desktop doesn't write the stamp yet.
I also liked that test_unusable_stamps_read_as_absent asserts marketplace.read_station_stamp is station_stamp.read_station_stamp — that pins the one-reader rule instead of trusting it.
What I actually checked, and what I did not
- Code read only. I did not run the suites — TCC denies this host a checkout of this repo, same limitation I declared on my 15:39 review.
- I read the delta
8f6dd811...940426e21ein full (9 files), pluscmd_await's reuse path and both stamp consumers. I have not read all 17 files of the PR; my 14:46 approval coveredorigin_owner(), and the gap that approval left is exactly why I withdrew it, so I am stating coverage rather than implying it. scripts/review-checks.shon the full diff: PARTIAL — hardcoded-paths and root-artifacts clean,prose-capSKIPPED because this tree is not the PR's revision. Not a pass; CI's run is the authoritative one.
Thanks for the quick turnaround on this, and for taking the structural fix over the guard.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
PR review of #4283 — approve
Adds a connect-apps skill so the agent can connect third-party apps (Gmail, Slack, etc.) from chat, through Superpower Station connectors, and answer the owner's original question once sign-in completes — no engine restart, no dangling open task.
Reviewed at the current head (940426e), after John's earlier CHANGES_REQUESTED and the fixes that followed. My contribution here is independently verifying those three fixes are actually present and correctly targeted, not just trusting the room's summary of them:
- Fail-open account check at wait-creation —
account_outcome()used to default to"connected"when the wait never recorded acloud_user_id. Now returns a distinct"unverified"outcome instead, so an unknown account never gets treated as a match. - TOCTOU between the account check and the data read —
account_for_wait()is new: retries the/api/mecheck with backoff (ACCOUNT_RETRY_S), returnsNoneon persistent failure.cmd_await's cross-task wait-merging now explicitly requiresmarker.get("cloud_user_id") == cloud_user_idbefore folding another task's wait into this one — a request can no longer cross accounts. - Wait reused before resolving new apps —
cmd_awaitnow separates same-task markers from same-room markers; a same-task wait is only reused when the new request's apps are already a subset of what's being waited on (set(slugs) <= existing); otherwise it computes the union and creates one wait covering all apps, so the task still gets exactly one answer.
Verified independently: cloned at head 940426e, ran tests/connect-apps-cli.test.py (69/69 pass, up from 46 pre-fix) and tests/connect-apps-waiter.test.py (27/27 pass, up from 25) — the fix commit added real test coverage for all three cases, not just a code change. All 19 CI checks green, including coverage gate and the hermetic-bridge/hardcoded-path lint suite. bash scripts/review-checks.sh --diff clean (hardcoded-paths, root-artifacts, prose-cap all pass).
Also independently re-read origin_owner() (the gate on who may arm an owner-tier resume): it checks the task carries its own id, that every source:-prefixed line in the file — not just the parsed header — is ag2space, that no collaborator: line exists anywhere, and that access_tier resolves to a single value of owner across the whole file, not just the parsed header block. That's a fail-closed design against a forged extra header line in a collaborator-controlled task body, which is exactly the attack shape this skill needs to resist since it can arm an owner-tier action from what started as a chat message.
Two independent human-equivalent reviewers (john-the-dev, yixuan-ag2) already re-reviewed and approved at this head after the fix round. Adding a third independent approval with the specific verification above.
No blocking findings. Approved.
Problem
When the owner asks "what's on my calendar" or "any email from Sam", the engine can't answer on a desktop install. Confirmed on
origin/main:docs/built-in-tools.md(opened for calendar and email by CLAUDE.md) starts withgws calendar +agenda, butgwsisn't bundled on desktop.skills/macos-toolsfalls back to macOS Calendar, which is empty on a typical desktop, so the agent says "nothing on your calendar".The marketplace skill says:
After that, the engine needs a restart to see the new tools, which kills the conversation.
Change
New skill
skills/connect-appsanswers through the Superpower Station connector tools (composio_find,composio_exec). When an app isn't connected:space.ag2.connector, rendered by the webclient).Why the task closes instead of waiting open: an open AG2 task collides with the supervisor's wedge recovery, the proactive loop, the watcher sweep, task-orphan-check and the Stop hook.
scripts/connectors.pyawaitrecords a durable wait instate/connect-waits/<id>.jsonand starts a detached waiter that pollsGET /api/connectors.source: connector-resume, owner tier,task:last.awaitin the same room merges into the pending wait.rearmrestarts dead waiters and runs fromstartupandproactive-loop.awaitrefuses unless the origin task file is the owner's own AG2 Space task: no collaborator, owner tier, one Matrixuser_idequal to--owner, HMAC stamp not invalid.Rules in SKILL.md
station_callto a metered tool.Routing
docs/built-in-tools.md: Calendar and Email go to the Station connectors first.gwsand macOS Calendar become explicit fallbacks.macos-tools: no longer answers from an empty macOS Calendar.morning-briefing: a missing connector is reported as text only.marketplace
connect-apps.find/resolveusekind=connectorwith an exact slug. Before, a non-featured app like Linear resolved tonot_found.station_call.state/station-core-stamp.jsonsays the running core started without the station. Older desktops without the stamp keep the file-based behaviour.Evidence
marketplace SKILL.md connector guidance, before (
origin/main):After (this branch):
Test runs on this branch:
Also passing:
ruff check .scripts/lint-skill.py --all --strict(57 manifests, 0 errors)scripts/review-checks.shon the full diffscripts/check-python39-compat.pyon 3.9.6tests/state-paths-adoption.test.py,scripts/lint-sutando-home-path.sh,gen-src-map --checkWhat the new tests cover:
claimbeats the waiter, which then writes nothing;awaitmerges;Live path
This touches a live path (the task lifecycle), so the post-restart round trip is part of the release test: a fresh Tart VM on the next desktop prerelease.
Companions and merge order
🤖 Generated with Claude Code