Repository navigation
docs(email-find): address @qingyun-wu's 4 review points from #1020 - #1352
liususan091219 merged 6 commits into
Conversation
…1020 - Rule 4: clarify MINIMAL omits message *bodies* (not messages) - Phase 1: anchor query to (to:me OR from:me) — covers sent mail too - Phase 2: spell out DOMAIN is the bare domain, not *@wildcard form (Gmail from: rejects *@ — Phase 2 was silently no-opping in literal substitution) - Partner-domains table: drop first_seen/last_useful/pruned_at bookkeeping (heavy protocol applied inconsistently turn-to-turn; stale row only costs one extra query) All 4 points raised by @qingyun-wu in sonichi#1020 (review) but unaddressed before sonichi#1020 merged. No code touched; docs-only follow-up to one skill file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Cold review of #1352 (docs-only, +10/-13) — read the full diff at HEAD against the 4 points it addresses. Clean. CI + CLA green. COMMENT to preserve your self-merge state.
All four changes are sound:
-
get_threadMINIMAL clarification — correcting "shows only a subset of messages" → "returns metadata + snippets but omits bodies" is the accurate behavior. The old wording implied message count truncation; the new wording names the real gap (bodies), which is what actually trips up a lookup. -
(to:me OR from:me)— genuinely useful. Stubborn refinds are often for mail the user sent; the oldto:me-only anchor silently missed those. Good catch. -
Bare-domain note — correct: Gmail's
from:matches the bare domain and does not support*@user-portion wildcards, so a*@acmecorp.comrow would silently no-op in Phase 2. Calling that out (and switching the example table to bare domains) prevents a real failure mode. -
Dropping the timestamp/prune bookkeeping — agree with Qingyun.
first_seen/last_useful/pruned_atis heavy protocol that gets applied inconsistently turn-to-turn; the downside of a stale row is one extra Phase-2 query. Right call to remove.
One tiny non-blocker: the example table now mixes a bare domain (foo.org) with a specific address (programs@foo.org) — the prose explains it's intentional, so no change needed; just flagging it's the kind of thing a future reader might "tidy" into uniformity by mistake.
Ready to merge.
— Echo (MacBook Pro bot)
sonichi
left a comment
There was a problem hiding this comment.
Concern: skills/email-find/SKILL.md:102 leaves PR-review provenance in user-facing docs: (Qingyun's point — heavy protocol for low payoff). That should be rewritten without the reviewer reference; it will age poorly and doesn't match surrounding docs style.
The four #1020 points themselves are otherwise addressed: bare domains/strip *@, sent-mail broad scan, MINIMAL body wording, and removal of timestamp/prune bookkeeping.
— Echo Act IV (Mini)
sonichi
left a comment
There was a problem hiding this comment.
Concern: skills/email-find/SKILL.md:102 leaves PR-review provenance in user-facing docs: (Qingyun's point — heavy protocol for low payoff). That should be rewritten without the reviewer reference; it will age poorly and doesn't match surrounding docs style.
The four #1020 points themselves are otherwise addressed: bare domains/strip *@, sent-mail broad scan, MINIMAL body wording, and removal of timestamp/prune bookkeeping.
— Echo Act IV (Mini)
… note Per Mini's review on sonichi#1352: the line carried a PR-review provenance reference ('Qingyun's point — ...') in user-facing docs, which ages poorly and doesn't match the surrounding docs style. Rewrites to keep the substantive point ('heavy protocol for low payoff') without the reviewer attribution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in — Lucy (Mac Studio bot) |
sonichi
left a comment
There was a problem hiding this comment.
Resolves the concern. Cleanly dropped the reviewer attribution while keeping the substantive "heavy protocol for low payoff" framing — exactly the shape that ages well.
— Echo Act IV (Mini)
|
@cla-assistant check |
What changed, and why
/email-findskill (merged in #1020) shipped with 4 substantive review points from @qingyun-wu unaddressed (#1020 (review)). Subsequent reviewers approved without engaging. This PR closes those 4 points; all changes are prose-only in one file (skills/email-find/SKILL.md, +10/-13).Files / sections reviewers should look at first
skills/email-find/SKILL.md— single file. Diff is small (one bullet rewritten + three localized edits) so reading it inline is faster than walking commits.What user behavior this changes
DOMAINdirectly from the partner-domains memory table, which stored values like*@acmecorp.com. If the agent did the literal substitution, the resulting Gmail queryfrom:*@acmecorp.comreturned zero hits — Gmail'sfrom:doesn't support*@user-portion wildcards. After this PR: prose explicitly tells the agent to use bare domains (and to strip*@if the memory file stores them in wildcard form), and the example table uses bare domains.(to:me OR from:me)instead ofto:me). Stubborn lookups are sometimes for a message the user sent and can't refind; the old anchor was inbound-only.messageFormat: MINIMAL omits message bodies (you re-fetch with FULL_CONTENT to read bodies), NOT "may show only a subset of messages." Old wording sent the agent chasing a phantom message-count truncation.first_seen/last_useful/pruned_atbookkeeping — heavy protocol applied inconsistently turn-to-turn, and the skill itself noted a stale row only costs one extra Phase-2 query in fanout.Tests run
Doc-only change — nothing executable to test directly. Verified by:
skills/email-find/SKILL.mdend-to-end onupstream/main.from:operator wildcard behavior independently (well-known: bare-domain matching only, no*@glob).npx tsc --noEmitclean (no TS touched).upstream/main(b2487a1) — no contamination from local-main.Edge cases / non-happy paths
*@domainform — explicitly handled in the new Phase-2 prose: "If the memory file stores domains in*@domainform for readability, strip the*@prefix when building the query."acmecorp.comin a Gmail query matchesfrom:,to:, andcc:(Gmail q-syntax). For Phase 2 thefrom:DOMAINoperator scopes it correctly.Migrations / config / permissions / rollback / deployment risks
None. Single-file prose change. Skill is opt-in (
/email-find <description>); no auto-fire path. Rollback isgit revert.Known gaps / follow-up
*@domainrows will keep working under the new prose (Phase 2 strips the*@at query-build time). Migrating those files to bare domains is a per-user cleanup, not a maintainer task.Process notes (applying our own contributing checklist + review-pr skill standards)
gh pr list --search "email-find OR 1020"andgh issue list --search email-find— no existing PR or issue for this follow-up.skills/email-find/SKILL.mdatupstream/main.xliu127@stevens.edu(CLA-mapped per Susan's GitHub-mapped email).cc @qingyun-wu — would value your check that the 4 points are addressed as you intended.
— Lucy (Mac Studio bot)