Skip to content

ACE-34: stop duplicate App task replies for one entity - #2250

Open
Gandy2025 wants to merge 1 commit into
mainfrom
fix/github-task-reply-cross-run-publication-claim
Open

ACE-34: stop duplicate App task replies for one entity#2250
Gandy2025 wants to merge 1 commit into
mainfrom
fix/github-task-reply-cross-run-publication-claim

Conversation

@Gandy2025

Copy link
Copy Markdown
Contributor

Summary

  • What changed? App-authored GitHub task replies now take a cross-run publication claim before writing to GitHub, and a run that loses the claim reads the hidden first-tree-github-task-reply-run marker back off the entity instead of publishing a duplicate.
  • Why does it matter? On PR fix(cli): verify BYO Context readiness before setup #2207, five accepted webhook events on one unchanged head (32e5d2d4) dispatched five runs, and each published a byte-identical Context Reviewer verdict (comments 5201858908, 5201860954, 5201861440, 5201861803, 5201862152, 07:45:03–07:45:25Z). Publication idempotency lived on the run's own message row (githubTaskReplySubmission), so it made one run idempotent and was blind to a sibling run. The run marker was written into every body and only ever read during same-run reconciliation of an unknown write — never as a publish guard.

How the claim works

The publisher already computes payloadHash = sha256([repository, entityType, entityNumber, body]). That hash is run-independent by construction: it identifies the comment about to be written, not the run writing it. claimGithubTaskReplyPublication claims it through processed_events' existing (event_id, platform) unique index under a dedicated github-task-reply-publication platform, inside the same transaction that claims the run and before the run claim, so a lost publication claim leaves the run untouched and retryable.

  • Winner publishes exactly as before.
  • Loser never POSTs. It lists the App's task replies on the entity and matches on the caller-authored body with the run marker stripped. A match makes the loss terminal (state: failed, code GITHUB_TASK_REPLY_DUPLICATE_PUBLICATION); no match reports the owner as still in flight and leaves the run pending.
  • Definitive GitHub rejection releases the claim so the reply is publishable again. An unknown write deliberately keeps it — the existing fail-closed reconciliation owns that case, and reopening the claim would authorize the duplicate this guards.

A moved head produces a different verdict body, so it hashes differently and still publishes. Ordinary conversational replies are unaffected for the same reason.

Known limitation (deliberate, please read)

The guarantee is one App comment per exact (entity, body), not per (entity, head). A task reply run carries no head SHA — issue_comment payloads do not contain one, and the reply publisher is the generic surface used for conversational answers as well as review verdicts, so a head-scoped suppression would swallow a legitimate second answer at the same head. In the incident the five verdicts were byte-identical, so this collapses them to one; if a future reviewer run renders materially different prose for the same head, that different verdict still publishes.

Also not addressed here: the five runs themselves. resolveGithubAudience mints one automatic provider task per accepted event, and createGithubTaskRun mints a fresh githubTaskRunId per delivery, so the wasted agent runs still happen — this PR only stops them reaching GitHub. Collapsing dispatch needs a product decision about which event kinds may be suppressed while a run is in flight (a human question arriving mid-review must not be swallowed); filed as follow-up.

Validation

  • pnpm check — no new findings (17 warnings / 7 infos, identical to base)
  • pnpm typecheck
  • pnpm test — server 3129/3129, shared 73 files, CLI 114/114. packages/client pi-handler.test.ts fails on this machine with and without these changes (timing-sensitive, pre-existing).

Five new tests in github-task-reply-publisher.test.ts cover: sibling run suppressed with one comment left on the entity, moved head still publishing, two runs for the same exact head publishing concurrently claiming once, claim released on definitive rejection, and claim retained after an unknown write. All five pass; the three suppression tests fail when the claim is stubbed out.

Change Surface

  • apps/cli public CLI or help output
  • tree onboarding / binding / inspection behavior
  • shipped or planned skill topology
  • docs or contributor-facing repository metadata
  • CI / packaging / release plumbing

Notes

  • package or install behavior changes: none. No schema change — the claim reuses the existing processed_events table, so there is no migration.
  • docs or tests updated to match: product tests above, plus a cross-run publication observation added to packages/qa/cases/cross-surface/github-webhook-routing-regression.md.
  • follow-up work: collapse duplicate dispatch for one entity so a repeat event does not bill a full agent run; decide whether a legitimate re-review should update the prior comment rather than append a new one (issue expectation 3). The Context Review PR-review publisher (context-reviewer-publisher.ts) has the same run-scoped-only shape for createPullRequestReview and is untouched here.

ACE-34

Five accepted webhook events on one pull request at one unchanged head
dispatched five runs, and each published its own byte-identical Context
Reviewer verdict. Publication idempotency was scoped to a single run's
message row, so it could not see a sibling run, and the hidden
`first-tree-github-task-reply-run` marker was written but never read back
as a publish guard.

Claim publication on the publisher's existing run-independent payload hash
— repository, entity, body — before the run claims itself, so exactly one
run per exact reply reaches GitHub. A losing run reads the marker back off
the entity: finding its reply already published makes the loss terminal,
and not finding it reports the owner as still in flight and leaves the run
retryable. A definitive GitHub rejection releases the claim; an unknown
write keeps it so nobody republishes behind a comment that may be live.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the cross-run publication claim and loser reconciliation paths. The implementation takes the payload-bound claim before mutating the run, retains it for unknown writes, releases it only for a definitive rejection, and covers sequential, concurrent, rejection, and unknown-write cases. I found no blocking issue.

Database note for human review: this introduces a new persistence semantic, although no schema or migration changes. Each distinct exact reply retains a github-task-reply-publication row in processed_events after success or an unknown write; only a definitive rejection removes it.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: The permanent entity/body claim suppresses distinct future provider tasks that happen to render the same text, not only sibling runs for one logical review.

Risk level: B-low

  • Path baseline: packages/server/** without a core chat/message/session/agent service or schema change -> B-low
  • Semantic lift: none

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: Repeated webhook events on one unchanged pull request can dispatch sibling runs that publish the same App-authored terminal reply several times.
  • Approach: Claim a run-independent hash of repository, entity, and reply body in processed_events; only the winner writes to GitHub, while losers reconcile against existing App comments.
  • Impacted modules: GitHub task-reply publisher and claim service, shared reply error schema, server regression tests, and the cross-surface QA case.

Review findings
❌ 1. The key has no logical task/review generation, and a successful row is never released. After one task replies Done. on an Issue, any later independent supported event on that same Issue whose legitimate terminal answer is also Done. loses this permanent claim, matches the old comment, and is marked failed. That contradicts the current per-event terminal-reply contract and the PR's statement that ordinary conversational replies are unaffected. Scope the claim to the duplicate-dispatch cohort (or to a surface that supplies a stable review generation), and add a regression showing that two independent events on one entity may publish identical bodies while sibling runs for one event still collapse. [R3, R5 / packages/server/src/services/github-task-reply-publication-claim.ts:39]

⚠️ 2. Reusing processed_events also gives the webhook-dedup table a second, permanent retention contract: every successful or unknown exact reply remains there, while only definitive rejection deletes it. The existing system context already tracks crash-stranded processed_events claims as a P1 lifecycle gap; a later cleanup of webhook dedup rows could silently remove publication claims and re-enable duplicates unless this platform is specially preserved. Please give publication claims an explicit independent retention/lifecycle boundary, or encode and document the platform-specific retention requirement where cleanup is owned. [R1, R5 / packages/server/src/services/github-task-reply-publication-claim.ts:32]

Action taken

  • Submitted request changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants