docs-seo-audit: never post the Slack summary twice in one run - #654
docs-seo-audit: never post the Slack summary twice in one run#654warp-agent-staging[bot] wants to merge 2 commits into
Conversation
Harden the Slack notification section against double-posting: mandate exactly one post per run, check the channel for an existing same-day SEO Audit message before posting, and replace the freeform curl example with a one-shot Python poster (notify_seo_slack.py) that performs the dedupe check and post together so there is no window to re-send after revising the wording. Fixes JAS-3: an agent revised its summary mid-run and posted twice (~11s apart) for the same audit run. Co-Authored-By: Warp <agent@warp.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@warp-agent-staging[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds a one-shot Slack poster and updates the docs SEO audit skill to avoid duplicate same-day Slack summaries. The direction matches the reported failure, but the helper still has paths that can miss an existing same-day post and send another message.
Concerns
- The history lookup only inspects the latest 50 channel messages, so a busy channel can push an earlier same-day SEO Audit summary out of the dedupe window.
- The helper posts anyway when
conversations.historyfails, which bypasses the dedupe check on exactly the ambiguous retry path this PR is meant to make safe.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| return False | ||
|
|
||
|
|
||
| def fetch_recent_messages(token: str, channel: str, limit: int = 50) -> list: |
There was a problem hiding this comment.
--date with pagination or another same-day bound instead of relying on a fixed recent-message window.
| try: | ||
| recent = fetch_recent_messages(token, args.channel) | ||
| except (urllib.error.URLError, RuntimeError) as exc: | ||
| print(f"warning: could not check channel history ({exc}); posting anyway", file=sys.stderr) |
There was a problem hiding this comment.
conversations.history fails bypasses the dedupe guarantee: a retry after an ambiguous earlier success can still send a second summary. Treat an unverified history check as a skipped or failed notification and write the message to run output rather than calling chat.postMessage.
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR updates the SEO audit skill to mandate a one-shot Python Slack poster and adds direct regression coverage for top-level same-day message recognition. CI is green, the reviewer request read-back includes rachaelrenk, and the helper test is meaningful; however, the notification path remains fail-open and only reads 50 messages, so it can still post a duplicate summary. Found: 0 critical, 2 important, 0 suggestions, 0 nits. Request changes.
Required changes
- Paginate Slack history through the target date before deciding no matching top-level summary exists.
- Fail closed when the history request cannot be verified; do not call
chat.postMessage.
Verification
CI: all 8 checks passed. Local: python3 -m py_compile and the 7-case dedupe regression test passed. ./script/presubmit is not present in this repository, and trunk is not installed in this sandbox. The current reviewRequests read-back includes rachaelrenk.
Verdict
Found: 0 critical, 2 important, 0 suggestions, 0 nits
Request changes
| return False | ||
|
|
||
|
|
||
| def fetch_recent_messages(token: str, channel: str, limit: int = 50) -> list: |
There was a problem hiding this comment.
conversations.history query does not satisfy the same-day dedupe requirement in a busy channel: an existing SEO Audit post can fall outside the newest 50 messages, and this helper will then post a duplicate. Paginate history and stop only after reaching messages older than the target date (with regression coverage for a matching post on a later page).
| try: | ||
| recent = fetch_recent_messages(token, args.channel) | ||
| except (urllib.error.URLError, RuntimeError) as exc: | ||
| print(f"warning: could not check channel history ({exc}); posting anyway", file=sys.stderr) |
There was a problem hiding this comment.
conversations.history fails after an earlier ambiguous or successful attempt, this exception handler treats history as empty and calls chat.postMessage, bypassing the no-duplicate contract. Fail closed: report the history-check failure and return non-zero or a skipped result without posting.
Rework for JAS-3 review: the same-day dedupe check now paginates conversations.history through the target date instead of relying on a fixed 50-message window, so an earlier same-day summary in a busy channel is still found. A failed or unverifiable history check now fails closed (skips the post, non-zero exit) instead of treating history as empty and posting anyway. Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Overview
This rework paginates Slack history and fails closed on lookup errors. It cannot be accepted because the pagination request corrupts valid opaque Slack cursor values.
Verdict
Checks: CI ✅ · Python compile and 11 targeted regression cases ✅ · docs build unavailable locally (Node 25 is outside the repository’s supported engines; CI build ✅)
Found: 0 critical, 1 important, 0 suggestions
Prior concerns: pagination traversal and history-failure handling are addressed; cursor encoding remains outstanding.
Request changes
Review run
https://oz.staging.warp.dev/runs/01a0489e-91f4-76cb-aea4-13017d8e7179
| cursor = None | ||
| while True: | ||
| url = ( | ||
| f"{SLACK_API}/conversations.history?channel={channel}" |
There was a problem hiding this comment.
next_cursor value (and assert the encoded follow-up URL in the pagination regression test). Slack cursors can contain query-reserved characters: a + becomes a space and an & starts a new parameter here, so the next request can use an invalid cursor, miss later same-day messages, and allow a duplicate summary. Build the query with urllib.parse.urlencode instead of interpolating the cursor.
Summary
An SEO audit run posted two Slack summaries ~11s apart for the same run (PR #652): a rough first draft, then a "cleaned up" re-send. Root cause: the
docs-seo-auditskill's Slack notification section had no "post at most once" contract — just a barecurlexample with no success check — so nothing stopped an agent from revising the wording and re-sending. Not a duplicate schedule and not two agent runs; only one SEO schedule exists and both messages came from the same Oz run.Changes
.agents/skills/docs-seo-audit/SKILL.md
ok: true) post.*SEO Audit — <date>*message before posting, and skipping if one exists.curlexample with the one-shot poster script below, matching theaeo_crosslink_auditpattern..agents/skills/docs-seo-audit/scripts/notify_seo_slack.py (new)
Small helper that performs the same-day dedupe check and the post in a single invocation, so an agent can't casually re-invoke
curlto "fix" wording after a successful post. The dedupe check paginates Slack history through the start of the target date rather than a fixed recent-message window, and fails closed (skips the post, non-zero exit) when the history check itself can't be verified. Exits0on post/skip, non-zero on a post failure or an unverified history check..agents/skills/docs-seo-audit/test_notify_seo_slack.py (new)
Regression test for the dedupe logic (
find_existing_post/is_top_level/fetch_messages_for_date): same-day top-level post detected regardless of exact wording, a match on a later pagination page is still found, pagination stops when there's no next cursor, different date not matched, thread replies not mistaken for a top-level post, thread-parent still counts, empty history doesn't block the first post, and a failed history check (Slack API error or network error) fails closed without callingchat.postMessage.Rework changes
Addressed both
[IMPORTANT]findings from the code-review pass:fetch_recent_messages(singleconversations.historycall,limit=50) is replaced withfetch_messages_for_date, which paginates the API witholdestset to the start of the target date and followsresponse_metadata.next_cursoruntil Slack reports no more pages. Addedcheck_pagination_finds_later_page_match(a same-day post that only appears on a simulated second page is still detected) andcheck_fetch_stops_pagination_when_no_next_cursorregression coverage.main()previously caught the history-fetch exception, logged a warning, and fell through to treat history as empty (posting proceeds). It now returns a non-zero exit and skips the post on anyconversations.historyfailure — a Slack API error (RuntimeError) or a network error (URLError) — instead of risking a duplicate on the exact ambiguous-retry path this script exists to prevent. Addedcheck_main_fails_closed_on_history_errorandcheck_main_fails_closed_on_url_errorregression coverage (both assert a non-zero exit and thatpost_messageis never called).Also updated the "Sending the notification" section of
SKILL.mdto describe the new fail-closed exit behavior.Verification
python3 .agents/skills/docs-seo-audit/test_notify_seo_slack.py— all 11 cases pass (7 original + 4 new: later-page pagination match, pagination-stop, and two fail-closed cases).python3 -m py_compileon both scripts — no syntax errors.notify_seo_slack.py --help— CLI parses as expected.name:/description:present) per the docs repo's skill-authoring conventions.trunk check/trunk fmtare not runnable in this sandbox (Trunk CLI isn't vendored in the repo, percreate_pr's note); no other repo-documented checks apply to.agents/skills/content.SKILL.mdprose change itself isskill-doc-only(testing-exempt perfactory-verification) — the dedupe/fail-closed logic is genuinely testable code, so it got regression tests instead of relying on the exemption.Originating thread: https://warpdev.slack.com/archives/C09BVK0PL3Y/p1787900556500469