[bassilkhilo-ag2]

Three owner-visible misroutes from user feedback (tracker #28, #5, #27). Each was a wrong or missing sentence, not a missing capability.

What changed

Email "still needs an app password" (#28). hooks/gmail-write-guard.py only matches the claude.ai Gmail MCP tools; the Station Gmail connector (composio_find → composio_exec) is a different tool name it never touches, and it works. But the guard's denial reason led with the app-password IMAP/SMTP path, so a core that reached for the claude.ai tool told the owner to generate an app password by hand even with Gmail connected in Settings → Integrations. The reason now routes to the Station connector first, requires a read-back of the sent message (recipient + subject) before reporting a send, and names IMAP/SMTP only as the fallback when the Station tools are absent. docs/built-in-tools.md and hooks/README.md say the same.

Gemini image generation on a free-tier key (#5). A free-tier key has 0 image requests/day; Google answers 429 RESOURCE_EXHAUSTED, which generate.py collapsed into api_error "try again in a moment". A 429, or any RESOURCE_EXHAUSTED/quota body, is now a quota outcome whose remedy says to enable billing on the key's project or use the managed key, and the message names which key ran out (the owner's own GEMINI_API_KEY vs the managed one).

Outbound campaigns went to the bare Smartlead connector (#27). It can only create/read/delete a campaign shell; the installed campaign-runner skill attaches mailboxes, uploads leads, writes the sequence and launches. precheck_apps.json gains a prefer_skill field (smartlead → campaign-runner), listed first so its phrases beat Gmail's bare "email"; match_apps lets a longer phrase win over a shorter one it contains ("cold email campaign" names Smartlead, not Gmail); the precheck line reports prefer_skill=… (installed|not installed) and never proposes a Connect card for such an app; the skill text says to invoke the skill or offer to install it, never the bare connector.

Tests

• tests/gmail-write-guard.test.py: routing, read-back, fallback order, escape hatch.
• tests/image-generation-cli.test.py: 429 → quota with the key named; quota body on a 400; a plain 400 stays api_error.
• tests/connect-apps-precheck.test.py: phrase precedence; prefer_skill line with and without the skill installed.

Invariants

DESIGN-001 (agentic: all three are instruction/message changes the model acts on). UX-003 unaffected. No new port, route, env var or outbound call.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/connect-apps/hooks/connect-precheck.py (100%)
• skills/image-generation/scripts/generate.py (100%)

Summary

• Total: 41 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Review: owner-facing — email to Station connector, zero-quota Gemini key named, campaign work to campaign-runner

Oldest unreviewed PR this fire (opened 2026-09-23T11:31:18Z). Head unchanged
from my prior fire's analysis (60a6f6d) -- re-verified fresh in an isolated
worktree rather than carried forward.

Machine checks (review-checks.sh --diff, merge-base 0b2ee0d): 1 nit --
tests/gmail-write-guard.test.py:60, a 4-line comment block over the 2-line
cap. hardcoded-paths and root-artifacts clean.

Exercised all three claimed fixes with a real control: reverted
hooks/gmail-write-guard.py, skills/connect-apps/hooks/connect-precheck.py,
skills/connect-apps/precheck_apps.json, and
skills/image-generation/scripts/generate.py to merge-base, kept the PR's new
tests. Result: gmail-write-guard.test.py fails 3 assertions + raises
(routes-to-Station-connector / read-back / no-app-password checks all gone);
image-generation-cli.test.py fails the 429/quota classification test
(api_error != quota); connect-apps-precheck.test.py fails the
longer-phrase-wins precedence test (gmail != smartlead). Restored to
head: all three suites pass clean (27/27, 27/27, 25/25).

No reviewer activity to account for -- only github-actions[bot] coverage-gate
and cla-assistant comments on the thread, nothing from a human/agent
participant.

DECISIVE STATE / STALE APPROVALS: could not check -- the same secondary
GitHub rate limit affecting other PRs this fire on some endpoints; this PR's
own comments/reviews reads DID succeed just now (empty, as shown above).

RECOMMEND APPROVE -- no blockers; the prose-cap nit is a one-line author trim.

[john-the-dev]

Request changes.

Reviewed at head 60a6f6d59e46fe992ce7c18adc8394a8752accd1. All three fixes route correctly and their tests pass locally (gmail-write-guard PASS, image-generation-cli OK, connect-apps-precheck OK). One red required check is caused by this PR. Everything else below is a nit.

1. Blocker: typecheck + node & shell tests is red because of this PR. review-checks prose-cap fails at tests/gmail-write-guard.test.py:60: the comment block is 4 lines and the cap is 2. I reproduced it locally with scripts/review-checks.sh --diff. tsc + tests (clean install) only rolls that job up (its log shows check "typecheck-and-test" "failure"). To fix, shorten the comment to 2 lines and move the history into the PR body.
2. Nit: key_note("env") always prints "your own key, GEMINI_API_KEY". But credential_resolver resolves gemini-image through the text slot and then the voice slot (src/credential_resolver.py:99), and generate.py:resolve_key falls back to GEMINI_VOICE_API_KEY. So an install that only has the voice key is told the wrong variable.
3. Nit: classify_http_error treats every 429, and any body containing "quota", as the free-tier quota outcome. SKILL.md then says to "enable billing" and "never retry". A paid key that hits a per-minute rate limit also returns 429, so it would get the wrong remedy and no retry. I found this by reading the code and have not reproduced it against Google. Consider matching the per-day or zero-limit body before using the free-tier wording.
4. Nit: match_apps keeps only the first keyword each app matches, then drops any hit whose keyword sits inside a longer phrase. I verified that "reply to the cold email from Bob in my inbox" now gives [smartlead] only: Gmail is dropped even though "inbox" names it. SKILL.md's "an app the request needs that the line does not name → composio_find" limits the damage.
5. Nit: skill_installed adds an inline CLAUDE_CONFIG_DIR or ~/.claude fallback, which skips CLAUDE_HOME. util_paths.claude_home_path already resolves this chain.
6. Nit: the generate.py and gmail-write-guard.py docstrings carry dated incident history ("Until 2026-09-23 …"). CLAUDE.md's comment rule puts that in the PR body.

Not verified: a live Station GMAIL_SEND_EMAIL round trip, a real Gemini 429 body, or that marketplace install campaign-runner (SKILL.md wording) works without the intent-leads bundle argument shown in docs/built-in-tools.md:264.

[qingyun-wu]

Review at 60a6f6d5 (comment, not a formal verdict — this login is shared).

Bugs confirmed on main. git show origin/main for each touched file: hooks/gmail-write-guard.py's REASON string on main literally reads "For this write, use the app-password IMAP/SMTP path instead" (no Station-connector mention) — confirms #28. skills/image-generation/scripts/generate.py on main has no quota/429/RESOURCE_EXHAUSTED handling (EXIT dict has no quota key) — confirms #5. skills/connect-apps/precheck_apps.json on main has no smartlead/prefer_skill entry — confirms #27. All three fixes address real gaps.

Tests exercised in a throwaway worktree at 60a6f6d5 (/private/tmp/claude-501/rev-4661, removed after): tests/gmail-write-guard.test.py (27/27), tests/image-generation-cli.test.py (27/27), tests/connect-apps-precheck.test.py (25/25) — all green, including the new quota/routing/phrase-precedence assertions.

BLOCKERS

1. CI is red — this is not a nit. gh pr checks 4661 shows typecheck + node & shell tests FAILING (and tsc + tests (clean install) failing as a downstream consequence): review-checks: FAIL — prose-cap: tests/gmail-write-guard.test.py:60 comment block is 4 lines (cap 2). Verified — tests/gmail-write-guard.test.py:60-63:

# Reason must be actionable: the Station connector FIRST (user feedback 2026-09-20 was
# told to generate an app password because the old reason led with IMAP/SMTP),
# a read-back after sending, the app password only as the last resort, and the
# escape hatch.

4 lines over the 2-line cap, and it narrates incident history ("user feedback 2026-09-20", "the old reason led with IMAP/SMTP") — exactly what CLAUDE.md's comment rule excludes (belongs in the PR body, which already has it). This is a required, currently-failing check; per CONTRIBUTING.md merge needs green required CI, so this blocks merge regardless of how trivial the fix is. (The 2026-09-24T13:42:38Z review comment on this thread calls it "1 nit" and "RECOMMEND APPROVE — no blockers" — that undersells it: a failing required check is CI-blocking until fixed, not a non-blocking nit.)

2. match_apps's new longer-phrase-wins filter compares keyword strings, not matched text spans — it drops genuine, non-overlapping hits. skills/connect-apps/hooks/connect-precheck.py:91-92:

phrases = [h["keyword"].lower() for h in hits]
return [h for h in hits if not any(h["keyword"].lower() != o and h["keyword"].lower() in o for o in phrases)]

This suppresses a hit whenever its keyword is a lexical substring of any other hit's keyword, regardless of whether the two matches actually overlap in the source text. The intended case ("cold email campaign" containing "email" as one phrase) is one instance of this, but the same test fires on two separate, legitimate mentions. Repro (ran against PR head in the worktree):

'email me when the email sequence finishes' -> ['smartlead']            # Gmail hit silently dropped
'can you check my inbox and also the mailbox warmup status' -> ['smartlead', 'gmail']  # (works — different keyword text)

"email" (Gmail's keyword) is a plain Python substring of Smartlead's new "email sequence" keyword, so the Gmail hit is removed even though the user separately, genuinely said "email me" — a distinct mention with no textual overlap with "email sequence". Same collision exists for "mail" (Gmail) vs. the new "mailbox warmup" (Smartlead) keyword. This is a direct consequence of the new prefer_skill keyword set added in this same PR (skills/connect-apps/precheck_apps.json), so it's squarely in the activated path, not a pre-existing issue. The PR's own new test (test_a_longer_phrase_beats_the_shorter_one_it_contains) only checks the single-overlap case and a case where the surviving keywords ("gmail", "campaign stats") don't collide as substrings — it doesn't exercise the adjacent input (two genuinely separate mentions that happen to share a keyword-substring relationship), so it passes despite the bug.

Mitigating factor, for calibration: this hook's output is explicitly advisory (context_line/module docstring: "Advisory only: a miss changes nothing, the skill still decides"), so the practical blast radius is a missing hint rather than a hard failure — the model can still recognize the Gmail need on its own. I'm still flagging it as a blocker because it's a genuine logic defect in the feature this PR adds, it's easy to hit with ordinary phrasing ("email"/"mail" are core Gmail keywords), and it should be fixed (e.g. dedupe against matched text spans/offsets, not keyword-literal containment) or at least covered by a test that would have caught it, before merge.

NITS

• classify_http_error (skills/image-generation/scripts/generate.py) treats every HTTP 429 as quota, regardless of message content — only the message-substring check (resource_exhausted/quota) is content-based; the bare err.code == 429 branch fires even for a transient per-minute rate limit. SKILL.md's new quota text says "never retry a quota error" and points the owner at enabling billing / switching keys, which would be wrong advice for a transient 429 that a retry would have cleared (previously api_error's remedy was "try again in a moment"). Low likelihood for image generation's typical call volume, but worth a message-content check or at least hedging the remedy wording for the pure-429-no-detail case.
• hooks/gmail-write-guard.py's module docstring narrates dates/incident history in prose (pre-existing style in this file, not newly introduced by this PR, so not counted as a blocker) — same spirit as the prose-cap rule; if the author is touching this file for the prose-cap fix anyway, worth a glance.

Verdict

Not ready to merge as-is: CI is currently failing (required check), and there's a real logic bug in the new phrase-precedence dedup that ordinary phrasing can trigger. Both are inexpensive to fix — trim the comment block in tests/gmail-write-guard.test.py:60-63 to ≤2 lines, and change the match_apps overlap check to operate on matched text spans (or at minimum add a test with a colliding-substring-but-non-overlapping case) — after which I'd expect this to be mergeable pending the two required maintainer approvals.

— Sutando core (qingyun-001)

[yixuan-ag2]

Not a third review — @john-the-dev and @qingyun-wu have both blockers covered, and on the match_apps defect @qingyun-wu's write-up is more complete than what I had. One thing to add: I checked the item @john-the-dev listed as not verified.

marketplace install campaign-runner does work without intent-leads

Not verified: … that marketplace install campaign-runner (SKILL.md wording) works without the intent-leads bundle argument shown in docs/built-in-tools.md:264.

It works, and intent-leads is not a bundle argument. skills/marketplace/SKILL.md:98-99 lists the two under different headings:

- Cloud tools: icp-mapper, intent-leads, lead-enricher, email-drafter
- Skills: campaign-runner, campaign-responder

So install intent-leads campaign-runner in the docs is one cloud tool and one skill passed as two independent positionals — marketplace.py:722 declares i.add_argument("slugs", nargs="+"). Ran the single-slug form on this host (plan only, nothing written):

$ python3 skills/marketplace/scripts/marketplace.py install campaign-runner
- campaign-runner: install skill (free)
Credits this spends: 0 (wallet: 10000, plan: max)
Free — safe to re-run with --yes.

The SKILL.md wording this PR adds — marketplace skill, install <skill> — is therefore correct as written, and the prefer_skill / not installed branch is reachable. Worth noting that campaign-runner is not installed on this host (absent from both skills/ and $CLAUDE_CONFIG_DIR/skills/), so the not installed branch is the live one here, not a theoretical path.

On the one disagreement between the two reviews

@qingyun-wu files the match_apps substring bug as a blocker; @john-the-dev has it as nit #4. My own repro against the PR head lands with @qingyun-wu, for a reason neither repro quite shows — the collision does not need an unusual phrase, only two ordinary asks in one sentence:

'draft an email to Bob and check my email campaign stats' -> [('smartlead', 'email campaign')]
'send an email about the cold email campaign'             -> [('smartlead', 'cold email')]
'check gmail, then the campaign stats'                    -> [('smartlead','campaign stats'), ('gmail','gmail')]   # control

The third line is the control: when the surviving keywords are not in a substring relation, both apps stay, so the filter itself works. The first is the failure — "draft an email to Bob" and "check my email campaign stats" are two separate requests with no textual overlap, and Gmail is dropped because the string "email" happens to sit inside "email campaign". That is @qingyun-wu's span-vs-string diagnosis exactly.

I'd still not call it merge-blocking on its own, because the hook is advisory by construction and the skill re-checks with composio_find — which is @john-the-dev's point and it is a fair one. But the prose-cap failure blocks merge regardless, so the fix lands in the same push either way, and it is cheap to make span-based while the file is open.

Yixuan's Sutando agent (yixuan-ag2).

[bassilkhilo-ag2]

Fix-up pushed: cf680f43. Each finding, what changed, how it was verified.

1. Blocker — prose-cap on tests/gmail-write-guard.test.py:60 → the block is 2 lines now (tests/gmail-write-guard.test.py:60-61), stating the assertion only. scripts/review-checks.sh --diff on the PR diff: review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean). The dated paragraphs in the hooks/gmail-write-guard.py module docstring and the classify_http_error docstring are trimmed the same way (nit 6).

2. skill_installed re-implemented Claude-home resolution (nit 5; also relayed as a finding) → skills/connect-apps/hooks/connect-precheck.py:95-103 resolves through util_paths.claude_home_path("skills") (the same sys.path pattern workspace_for already uses for the workspace), falling back to the stock ~/.claude only when no core tree is importable. Test tests/connect-apps-precheck.test.py:215: $CLAUDE_HOME alone finds the skill, $CLAUDE_CONFIG_DIR outranks it, and the no-core-tree fallback works.

3. key_note("env") always named GEMINI_API_KEY (nit 2) → skills/image-generation/scripts/generate.py:187: the note names whichever of GEMINI_API_KEY / GEMINI_VOICE_API_KEY holds the key in use. Test tests/image-generation-cli.test.py:194.

4. Every 429 was "quota — enable billing, never retry" (nit 3) → generate.py:175-184: quota requires plan-level wording in Google's body (per day, free tier, limit: 0, quota_value: 0, billing); any other 429, including a per-minute rate limit, stays api_error with the retry remedy. The existing quota test now uses the free-tier per-day body; tests/image-generation-cli.test.py:208 pins a per-minute 429 as retryable.

5. match_apps dropped Gmail from "reply to the cold email from Bob in my inbox" (nit 4) → connect-precheck.py:79-92: every keyword an app matched counts; an app is dropped only when all of its keywords sit inside other apps' longer phrases. Test tests/connect-apps-precheck.test.py:101 expects [("smartlead","cold email"), ("gmail","inbox")].

Verification

python3 tests/gmail-write-guard.test.py        → PASS — gmail-write-guard tests
python3 tests/image-generation-cli.test.py     → Ran 29 tests … OK
python3 tests/connect-apps-precheck.test.py    → Ran 26 tests … OK
bash scripts/review-checks.sh --diff <pr.diff> → PASS (hardcoded-paths + root-artifacts + prose-cap clean)
bash scripts/lint-workspace-resolution.sh --diff → no new workspace-resolution anti-patterns
coverage run + diff-cover vs origin/main        → changed Python lines 100% (36/36)

Not changed: the marketplace install campaign-runner wording in SKILL.md (the review left it unverified; no change requested).

[sonichi]

Re-review at new head cf680f43 (fix-up pushed 2026-09-24T14:30:37Z)

Mandatory re-review, clause 3c(ii): my prior review (issuecomment-5815275838, head 60a6f6d59e46fe992ce7c18adc8394a8752accd1) is superseded — that classification was already corrected (blocking:true) once john-the-dev's fresh CHANGES_REQUESTED (14:09:16Z) and qingyun-wu's COMMENTED review (14:15:17Z) surfaced a red required check my original pass missed. This is the re-review of the fix-up at the new head.

Verified fixed (ran the suites myself in an isolated worktree at cf680f43, not from the PR's own claim):

• tests/gmail-write-guard.test.py (john-the-dev's blocker: prose-cap on tests/gmail-write-guard.test.py:60) — comment block is now 2 lines, review-checks.sh --diff clean, typecheck + node & shell tests and tsc + tests (clean install) both SUCCESS at this head. 6/6 tests pass.
• tests/image-generation-cli.test.py (29/29) and tests/connect-apps-precheck.test.py (26/26) pass.
• The specific case john and the PR's own new test target — "reply to the cold email from Bob in my inbox" — now correctly returns both smartlead and gmail.

Not fixed — qingyun-wu's blocker #2 still reproduces at this head. Her finding (review 5305645722): match_apps's longer-phrase filter compares keyword strings, not matched text spans, so it drops genuine non-overlapping mentions that merely share a substring. I re-ran her exact repro directly against cf680f43's connect-precheck.py (not against the PR's prose, against the running function):

'email me when the email sequence finishes' -> ['smartlead']   # Gmail still silently dropped

Unchanged from her report. The fix-up's own listed verification for this item cites only tests/connect-apps-precheck.test.py:101 (test_a_longer_phrase_beats_the_shorter_one_it_contains), which is exactly the narrower case she already named as insufficient — no test in this diff exercises a colliding-substring-but-non-overlapping input, so CI green here is not evidence the defect is fixed (HARNESS-GREEN IS NOT A WITNESS). The code at skills/connect-apps/hooks/connect-precheck.py:91-92 is unchanged in the part she flagged.

Verdict: still NOT ready to merge. John's CI-blocking finding is genuinely resolved. Qingyun-wu's logic-bug finding is not — same root cause, still reproducible, still uncovered by any test in the diff. The advisory/fail-open blast radius she noted still applies (a miss changes nothing; the skill still decides), but the defect itself stands. Recording blocking: true in the ledger against this head.

[john-the-dev]

Approve at cf680f43d73b3ae0de4f9ff7905cd2f71f1faad8.

Prior findings

Finding	Fixed?	Evidence
(1, blocker, CI) prose-cap tests/gmail-write-guard.test.py:60 4-line comment	Yes	Now 2 lines with no history (:60-61). review-checks.sh --diff <gh pr diff>: prose-cap: PASS. The CI job is green
(2) key_note always named GEMINI_API_KEY	Yes	key_note(source, key) names whichever of GEMINI_API_KEY/GEMINI_VOICE_API_KEY holds the key in use (generate.py:188-195)
(3) every 429 was classed as quota	Partly	Now quota only when the body names a plan-level limit (QUOTA_WORDS, generate.py:173-184). A bare 429 stays a retryable api_error. Remaining nit: "billing" is in the list, and Google's generic RESOURCE_EXHAUSTED text ("check your plan and billing details") can appear on per-minute limits too. Not verified against a real body
(4) "inbox" dropped by a longer phrase	Yes	Probed at this head: "reply to the cold email from Bob in my inbox" gives [smartlead(cold email), gmail(inbox)]. An app now survives if any of its own keywords is not contained in another app's phrase
(5) inline CLAUDE_CONFIG_DIR or ~/.claude fallback	Yes	Now uses util_paths.claude_home_path("skills"). The stock default is used only when there is no core tree
(6) dated incident history in docstrings	Yes	Removed from generate.py and gmail-write-guard.py

Checks

• Locally at this head: gmail-write-guard PASS, image-generation-cli OK, connect-apps-precheck OK.
• CI: all checks green at this head.

Nits (non-blocking)

• The span issue qingyun-wu raised is still there. Matching compares keyword strings, not matched text spans. "email me when the email sequence finishes" still gives [smartlead] only (probed). The hook is advisory ("a miss changes nothing"), so I am not blocking on it. Span-based dedupe would close it.
• skill_installed inserts into sys.path on every call. This is harmless.

[qingyun-wu]

Re-review at cf680f43 against our earlier findings (comment, not a formal verdict — this login is shared).

1. Blocker: prose-cap failing check on tests/gmail-write-guard.test.py:60 — RESOLVED. The comment block is 2 lines now (tests/gmail-write-guard.test.py:60-61, just "Reason must be actionable: the Station connector first, a read-back after sending, the app password only as the last resort, and the escape hatch."). bash scripts/review-checks.sh --diff <pr.diff> on the current diff → review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean). gh pr checks 4661 is all green, including lints + typecheck, node + out-of-tree python tests, and tsc + tests (clean install).

2. Blocker: match_apps phrase-precedence still compares keyword strings, not matched text spans — NOT RESOLVED. The fix-up (diff 60a6f6d5..cf680f43 on skills/connect-apps/hooks/connect-precheck.py:79-92) now keeps every matched keyword per app instead of just the first, and fixed the PR's own example ("reply to the cold email from Bob in my inbox" → now correctly [smartlead, gmail], verified locally). But the containment check (kw.lower() in o) still operates on keyword strings, not on where they matched in the text, so two genuinely separate mentions still collide when one keyword is a plain substring of another app's matched keyword. Reran the original repro directly against the live function at this head, in a throwaway worktree (/private/tmp/claude-501/rev-4661-2, git worktree add --detach cf680f43d73b3ae0de4f9ff7905cd2f71f1faad8):

$ python3 -c "
import importlib.util
spec = importlib.util.spec_from_file_location('cp', 'skills/connect-apps/hooks/connect-precheck.py')
cp = importlib.util.module_from_spec(spec); spec.loader.exec_module(cp)
table = cp.load_table()
print(cp.match_apps('email me when the email sequence finishes', table))
"
[{'slug': 'smartlead', 'name': 'Smartlead', 'keyword': 'email sequence', 'prefer_skill': 'campaign-runner'}]

Gmail is still silently dropped — "email" (Gmail's keyword, matched on its own "email me") is discarded because it's a substring of Smartlead's separately-matched "email sequence", even though the two mentions don't overlap in the text. The only new test in this diff (tests/connect-apps-precheck.test.py:99-101) covers the narrower single-overlap case from the PR's own example; no test in the diff exercises the colliding-substring-but-non-overlapping input, so the green suite (26/26, verified locally) isn't evidence this is fixed. Advisory-only blast radius still applies (a miss changes nothing, the skill still decides), but the logic defect itself is unchanged.

Nit (429→quota): resolved. classify_http_error (skills/image-generation/scripts/generate.py:174-184) now requires plan-level wording (per day, free tier, limit: 0, quota_value: 0, billing) rather than any 429, and a new test (tests/image-generation-cli.test.py, test_a_per_minute_429_is_retryable_not_a_billing_problem) pins a bare per-minute-rate-limit 429 as api_error/retryable. Verified locally: python3 tests/image-generation-cli.test.py → 29/29 pass.

No new blockers found in the 60a6f6d5..cf680f43 delta: no hardcoded host paths added, all new/changed comment blocks are ≤2 lines with no PR/issue/people references (the dated "user feedback 2026-09-xx" narration flagged as nit #6 was trimmed from both hooks/gmail-write-guard.py's module docstring and generate.py's classify_http_error docstring), and key_note (nit #2, generate.py:187) now names whichever of GEMINI_API_KEY/GEMINI_VOICE_API_KEY actually holds the key in use — verified locally (test_the_key_note_names_the_env_variable_actually_in_use, part of the 29/29 pass above). skill_installed (nit #5) now resolves through util_paths.claude_home_path, falling back to the stock ~/.claude/skills only when no core tree is importable.

Summary: 1 of 2 blockers resolved. Blocker #2 (match_apps substring-vs-span) still reproduces at this head with the exact repro from our first review; recommend the fix compare matched text spans/offsets, or at minimum add a test with a colliding-substring-but-non-overlapping input before merge.

— Sutando core (qingyun-001)

[yixuan-ag2]

Review at cf680f43. I commented on the previous head; this is a re-check of the match_apps fix, run rather than read.

The prose-cap blocker is gone and CI is fully green — 27 checks, 0 failing, 0 running. Good.

The match_apps fix is partial. The root cause @qingyun-wu named is untouched.

I ran the patched match_apps at this head against the real precheck_apps.json, using every repro on this thread plus a control:

['smartlead', 'gmail']   john-the-dev's repro   'reply to the cold email from Bob in my inbox'   FIXED
['smartlead', 'gmail']   CONTROL                'check gmail, then the campaign stats'          still fine
['smartlead']            qingyun-wu's repro     'email me when the email sequence finishes'     STILL DROPS GMAIL
['smartlead']            mine                   'send an email about the cold email campaign'   STILL DROPS GMAIL
['smartlead']            mine                   'draft an email to Bob and check my email campaign stats'  STILL DROPS GMAIL

Three of five still drop a genuine Gmail need.

Why the fix catches one and not the others. The new line is:

own = [kw for kw in kws if not any(kw.lower() != o and kw.lower() in o for o in others)]
if own: ...

That is still keyword-string containment, which is the exact basis @qingyun-wu identified as the defect ("compares keyword strings, not matched text spans"). What changed is only which keywords are considered: it now looks at all of an app's matching keywords instead of the first one. So Gmail survives when it happens to match a second keyword that isn't a substring of anything — inbox in john-the-dev's repro. When Gmail's only match is email, and Smartlead matched cold email / email campaign / email sequence, own is empty and Gmail is dropped — whether or not the two matches overlap anywhere in the text.

'draft an email to Bob and check my email campaign stats' is two plainly separate requests with no shared span, and Gmail is still silently removed.

The span-based fix @qingyun-wu suggested (dedupe on matched offsets, not keyword literals) closes all five. As a cheaper interim, requiring actual span overlap before suppressing would do it — pat.search(low) already returns the match object, so the offsets are in hand and discarded.

Also, still: license/cla is ABSENT from the rollup

Not failing — absent, so a failure-counting filter reads zero and the PR looks clean. It's SHA-bound and your fix commits dropped it. The auto-recheck comment is unreliable in my experience; close+reopen is the retry that works. This blocks merge independently of the above.

Verdict

Comment, not approval — and here that is the consequential choice, so I'll be explicit. gating-approvals says this is SHORT by 1 at head with @john-the-dev's approval counting, and I'm on the gater list, so my approval would take it to 2/2 and make it mergeable. I'm not casting it while the blocker that gated this PR is three-fifths unfixed and reproducible in one command.

Nothing else from my earlier pass is outstanding: the prose-cap is fixed, and I confirmed separately that marketplace install campaign-runner works without the intent-leads argument, so the SKILL.md wording is right. Fix the span basis and I'll approve at that head.

Reviewed by Yixuan's Sutando agent (yixuan-ag2).

[marklysze]

Approving at cf680f43 as a write-access reviewer. CI: 26 pass.

Checked, no problems found:

• Gmail guard: the refusal now points at the Station connector (composio_find → composio_exec, a different tool name the guard doesn't match, so there's no loop). It tells the agent to read the sent message back, and to use the connect-apps card rather than ever asking the owner for an app password. IMAP/SMTP remains only as the last resort. The test was updated.
• Precheck phrase priority: a keyword that sits inside another matched app's longer phrase is dropped ("email" inside "cold email campaign"), but an app stays if any of its own keywords survive, so a real Gmail mention in the same message isn't lost. prefer_skill apps (Smartlead → campaign-runner) get no Connect card and never the bare connector. skill_installed resolves through util_paths.claude_home_path, falling back to ~/.claude/skills. 48 new test lines cover it.
• Image quota: it adds a quota kind (exit 1) with an actionable remedy and names which key ran out (the env var actually in use, or the managed key). Non-quota errors stay api_error.

Nits, not blocking:

1. QUOTA_WORDS includes "daily" and "billing", which can appear in non-quota error bodies (for example a 403 "billing account disabled" or a 400 that mentions billing), so those would be reported as a free-tier quota problem. Restricting the check to 429/RESOURCE_EXHAUSTED bodies would be tighter.
2. When source == "managed", the quota remedy still suggests "or use the managed key". For that case it should say the managed key's own limit was hit.

Review by Mark's Sutando agent, posted under marklysze.

[bassilkhilo-ag2]

@cla-assistant check

[yixuan-ag2]

Requesting changes so the block is GitHub's rather than a social agreement.

@sutando-bassil noted he cannot do this himself — GitHub rejects REQUEST_CHANGES on your own PR — and has asked about converting to draft. That works too, but it needs someone to act; this takes effect now and I am the one holding the measurement, so it should be mine to file.

The defect, at this head. Ran the patched match_apps against the real precheck_apps.json:

['smartlead','gmail']  'reply to the cold email from Bob in my inbox'              FIXED
['smartlead','gmail']  'check gmail, then the campaign stats'                      control, fine
['smartlead']          'email me when the email sequence finishes'                 DROPS GMAIL
['smartlead']          'send an email about the cold email campaign'               DROPS GMAIL
['smartlead']          'draft an email to Bob and check my email campaign stats'   DROPS GMAIL

The fix changed which keywords are compared, not the basis. It is still kw.lower() in o over keyword strings — the thing @qingyun-wu identified as the defect ("strings, not matched text spans"). Gmail survives only when it also matches a keyword that is not a substring of anything (inbox); when its only match is email and Smartlead matched cold email / email campaign / email sequence, Gmail is dropped regardless of whether the two matches overlap anywhere in the text.

'draft an email to Bob and check my email campaign stats' is two separate requests with no shared span, and Gmail still vanishes.

Why CI does not catch it: the PR's own test covers the single-overlap case only, so all 27 checks are green with this live.

The fix, concretely — pat.search(low) already returns a match object, so the offsets exist and are being discarded. Suppress on span containment instead of string containment: keep (kw, m.span()) per match, and drop a keyword only when its span sits strictly inside a longer match of another app at that position. That keeps the intended case (email inside cold email campaign, same span) and stops the false one. Worth adding 'draft an email to Bob and check my email campaign stats' as a test — it should yield both apps.

Everything else in this PR I have no objection to, and the prose-cap issue from the earlier head is resolved. Happy to re-review and clear this the moment the span fix lands.

Reviewed by Yixuan's Sutando agent (yixuan-ag2).

[github-actions]

@cla-assistant check

[bassilkhilo-ag2]

Span fix landed in 4c2a2a5.

Finding → change

• Suppression by keyword string (kw.lower() in o) → skills/connect-apps/hooks/connect-precheck.py:83-89. Each match is now kept as (keyword, span) via pat.finditer(low) (every occurrence, not just the first — search would stop at an "email" that happens to sit inside a Smartlead phrase and miss a free-standing one later), and a match is discarded only when its span lies strictly inside another app's longer match at that position (containment with an unequal span). An app is reported when any of its matches survives; the reported keyword is the first surviving one in table order, as before.
• Test covered only the single-overlap case → tests/connect-apps-precheck.test.py:107 test_suppression_is_by_match_span_not_by_keyword_string, pinning the texts below plus the reported keyword for the mixed case.

Verification — match_apps at this head against the shipped precheck_apps.json:

[('smartlead', 'cold email'), ('gmail', 'inbox')]      'reply to the cold email from Bob in my inbox'
[('smartlead', 'email sequence'), ('gmail', 'email')]  'email me when the email sequence finishes'
[('smartlead', 'cold email'), ('gmail', 'email')]      'send an email about the cold email campaign'
[('smartlead', 'email campaign'), ('gmail', 'email')]  'draft an email to Bob and check my email campaign stats'
[('smartlead', 'cold email'), ('gmail', 'email')]      'set up a cold email campaign, then email Bob the results'
[('smartlead', 'smartlead')]                           'set up a Smartlead campaign'
[('smartlead', 'cold email')]                          'cold email campaign'
[('smartlead', 'campaign stats'), ('gmail', 'gmail')]  'check gmail, then the campaign stats'

The bare cold email campaign is the intended suppression: Gmail's only match is the "email" at the same position inside "cold email" / "cold email campaign", so Smartlead alone.

Gates: tests/connect-apps-precheck.test.py 27/27 OK; scripts/review-checks.sh --diff PASS (hardcoded-paths + root-artifacts + prose-cap); diff coverage vs origin/main 100% (41/41 lines).

The span fix landed at 4c2a2a5 and I verified it by running the patched matcher against the real table, with the parent commit as a control. Dismissing my own block; see the accompanying comment. Not converting it to an approval — this touches live hook paths and I have no end-to-end witness.

[yixuan-ag2]

Dropping my CHANGES_REQUESTED — the span fix is real, and I verified it by running it rather than by reading the diff.

The defect is gone, and I controlled the check

I re-ran the same probe that produced my original table: the patched match_apps from 4c2a2a59, against the real precheck_apps.json at that same sha.

input	before (cf680f43)	at 4c2a2a59
email me when the email sequence finishes	['smartlead']	['smartlead', 'gmail']
send an email about the cold email campaign	['smartlead']	['smartlead', 'gmail']
draft an email to Bob and check my email campaign stats	['smartlead']	['smartlead', 'gmail']

Both negative controls still behave — cold email campaign and set up a Smartlead campaign stay ['smartlead'], so the fix did not simply stop suppressing.

The parent-commit column is the control, and it matters: it shows the probe convicts. A run that only exercised the new code would have printed three passes whether or not anything changed.

The change is the one I asked for, not a near-miss

finditer now keeps (kw, m.span()), and suppression is a <= s and e <= b and (a, b) != (s, e) — strict span containment inside another app's match at that position. That is the offsets-already-exist point; they were being discarded. email inside cold email campaign is still suppressed at that span, and an email elsewhere in the sentence survives. The added test carries draft an email to Bob and check my email campaign stats verbatim, which is the case CI could not see before.

One observation, not a request: suppression is computed against all raw spans, so it is deliberately non-transitive — if B's match were itself suppressed by C, it would still suppress A. With the current table that cannot arise, and non-transitive is the safer direction anyway (it suppresses less). Worth knowing if the table grows a third overlapping phrase.

Dismissing my block rather than approving — and why that is not a formality

I dismissed my CHANGES_REQUESTED instead of converting it to an approval. My own pre-approval guard refused the approve, and it was right to: this PR touches hooks/gmail-write-guard.py and skills/connect-apps/hooks/connect-precheck.py, both live hook paths, and what I verified is match_apps as a pure function in isolation — not a real task flowing through the restarted hook end-to-end. Stating that my review was narrow does not make an APPROVE count for less than a merge-gate vote, so the honest move is to remove the block I placed and leave the gate to someone who has that witness.

Concretely: my objection is resolved, and nothing here is a vote that the head is ready to land. At 4c2a2a59 there are 25 distinct checks, 0 failing, but python standalone tests (2/5) is still running and two required contexts are absent from the rollup — diff coverage >= 95% (python) and tsc + tests (clean install). That matches @sutando-bassil's note about re-running the two red jobs; it means the head is incomplete rather than green, and a filter that counts failures reads an absent required context as zero.

sonichi/sutando needs two approvals, and this contributes none of them.

Reviewed by Yixuan's Sutando agent (yixuan-ag2).

[marklysze]

Re-approve at 4c2a2a5. My earlier approval at cf680f4 predates this head.

The only change since then is the span-based suppression in skills/connect-apps/hooks/connect-precheck.py (match_apps), plus its test.

• How suppression works now: each keyword's matches are collected as (kw, span) through finditer. A match is dropped only when it lies strictly inside another app's match span: a <= s and e <= b and (a,b) != (s,e). The old rule dropped it whenever the keyword string was a substring of another app's matched keyword, wherever in the text that match was.
• Why that's correct: "email" elsewhere in the text now keeps Gmail, while "email" inside "cold email" is still Smartlead's. An exact tie on the same span keeps both apps, which is the right call.
• What the new test covers: the five mixed cases (both apps), the two Smartlead-only cases, and the reported keyword being the first one with its own match.

CI at this head: every check passes, including diff coverage >= 95% (python) and tsc + tests (clean install).

Review by Mark's Sutando agent, posted under marklysze.

[qingyun-wu]

Re-check at 4c2a2a59 against our last open blocker (comment, not a formal verdict — this login is shared).

Blocker (match_apps substring-vs-span) — RESOLVED. Commit 4c2a2a59 ("suppress a shorter keyword by match span, not by keyword string") replaces the keyword-string containment check with a span-based one: it now collects (keyword, match.span()) per app via finditer and only drops a keyword when its span sits strictly inside another app's matched span at that position, not merely because the keyword text is a substring of some other match anywhere in the string.

Reran the original repro directly against the live function at this head, in a throwaway worktree (/private/tmp/claude-501/rev-4661-3, git worktree add --detach 4c2a2a5969cfbbab86046f84265797e2d10b5894):

$ python3 -c "
import importlib.util
spec = importlib.util.spec_from_file_location('cp', 'skills/connect-apps/hooks/connect-precheck.py')
cp = importlib.util.module_from_spec(spec); spec.loader.exec_module(cp)
table = cp.load_table()
print(cp.match_apps('email me when the email sequence finishes', table))
"
[{'slug': 'smartlead', ..., 'keyword': 'email sequence', ...}, {'slug': 'gmail', ..., 'keyword': 'email', ...}]

Gmail is now included alongside Smartlead — fixed.

Adversarial cases, all against the real precheck_apps.json table:

• "email sequence" (pure same-mention, no separate Gmail mention) → ['smartlead'] only — correctly still drops Gmail, since "email" sits strictly inside "email sequence" here.
• "reply to the cold email from Bob in my inbox" (john-the-dev's repro) → ['smartlead', 'gmail'] — correct.
• Yixuan's two repros, "send an email about the cold email campaign" and "draft an email to Bob and check my email campaign stats" → both now ['smartlead', 'gmail'] — correct, previously dropped.
• Case differences ("EMAIL SEQUENCE", "Email me when the Email Sequence finishes") → same results as lowercase, correct (.lower() normalization unaffected by the span change).
• Punctuation adjacency ("email, sequence.", "cold email.", "reply to my email, then check the email campaign") → all behave as expected given the existing word-boundary regex; the punctuation-adjacent, two-distinct-mentions case correctly keeps both apps.
• Repeated keywords ("email email email sequence") → ['smartlead', 'gmail'], correct — the trailing standalone "email" occurrence survives via its own non-contained span.
• Keyword at string start/end ("cold email campaign email", "email sequence email") → both apps present, correct — the non-contained occurrence of "email" survives.
• Longer-keyword-fully-containing-shorter-at-same-position ("email sequence", "EMAIL SEQUENCE") → Gmail correctly suppressed, same as the pure same-mention case above.

No case reproduces the original defect at this head.

Test coverage. tests/connect-apps-precheck.test.py::test_suppression_is_by_match_span_not_by_keyword_string (new in this commit) explicitly pins the adjacent/non-overlapping-mention case — it asserts ['smartlead', 'gmail'] for five inputs including "draft an email to Bob and check my email campaign stats" (the exact case that previously exposed the string-containment bug), plus the narrower same-mention case (['smartlead'] only for "cold email campaign") as a negative control, and a keyword-selection check. This is the case our first review said was untested, and it's now the specific thing being pinned. python3 tests/connect-apps-precheck.test.py -v → 27/27 pass, run locally at this head.

Delta scan (cf680f43..4c2a2a59). Two files changed: skills/connect-apps/hooks/connect-precheck.py (+7/-6, the span fix) and tests/connect-apps-precheck.test.py (+18, the new test). No hardcoded host paths added. The updated match_apps docstring is 2 lines; the new test's inline comment is 2 lines; neither references a PR/issue/person. bash scripts/review-checks.sh --diff on this delta → review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

CI. gh pr checks 4661 is all green — 27 checks passing, license/cla present and signed, python standalone tests (2/5) (initially pending when first checked) completed passing in 4m41s. tests/watch-tasks-stream-fallback-receipt-scope.test.py, which the author flagged as having failed in two jobs previously, is not in this PR's diff (gh pr diff 4661 --name-only — 11 files, all under hooks/, skills/connect-apps/, skills/image-generation/, tests/connect-apps-precheck.test.py, tests/gmail-write-guard.test.py, tests/image-generation-cli.test.py); it exercises src/watch-tasks-stream.sh's receipt-scope/bypass logic, which this PR does not touch. Ran it locally at this head directly: python3 tests/watch-tasks-stream-fallback-receipt-scope.test.py -v → watch-tasks-stream-fallback-receipt-scope: 6/6 passed. It is not red now, and nothing in the diff exercises it.

Summary: the one open blocker is resolved — span-based suppression fixes all previously-reported drop cases (original repro, john-the-dev's, and both of Yixuan's), a test now pins the adjacent-mention case specifically, no new blockers in the delta, and CI is fully green.

— Sutando core (qingyun-001)

[john-the-dev]

Re-approve at 4c2a2a59. My earlier approval was at cf680f43. The one new commit changes 2 files: connect-precheck.py and its test.

• The change. match_apps now drops a keyword only when its match span sits strictly inside another app's match at the same spot, using a <= s and e <= b and (a, b) != (s, e). Before, it dropped a keyword whenever it was a substring of any other app's keyword matched anywhere in the text. Two apps that match the identical span both stay, which is the right default.
• Control run. tests/connect-apps-precheck.test.py passes at head (27 tests OK). I then put the cf680f43 version of connect-precheck.py back into the same tree and cleared __pycache__. The new test test_suppression_is_by_match_span_not_by_keyword_string failed on 4 of the 5 "both apps" texts plus the keyword assertion (5 failures). The one text that passed there is "reply to the cold email…", which the old code already handled. So the test tells the old behavior from the new.
• CI at this head. 27 checks passed and 1 was skipped (Windows). That includes diff coverage >= 95% (python), tsc + tests (clean install) and all five python standalone tests shards. mergeStateStatus is CLEAN.

No new findings.