Add the Dispatcher pull-request updater task - #24822
Conversation
Consumes the UpdatePRComment snapshots the gatherer already emits and projects them onto one pull-request comment that never regresses. - pr_comment.py renders DispatcherProgress as HTML-rich markdown, modelled on the Datadog CI Visibility comment. An unfinished run is signalled four redundant ways so it cannot be mistaken for a final one. - task_pull_request_updater.py finds or creates the marked comment, holds its lock across revision validation and the write, and only advances the rendered revision once the write lands. - Two GitHub client endpoints the updater needs: update_issue_comment and list_issue_comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Preview only. This is one comment, edited three times, to show what Dispatcher renders as a When the run starts🔄 Dispatcher tests · in progressNote Tests are still running. 3 of 3 batches have not finished yet. 12 of 12 jobs have not reported.
Batches
Part way through, with a batch retrying🔄 Dispatcher tests · in progressNote Tests are still running. 1 of 3 batches has not finished yet.
Batches
❌ Failures⏳ This comment updates automatically as each batch finishes. Finished, with failures❌ Dispatcher tests · failedCaution Dispatcher tests failed. See the failures below.
Batches
❌ Failures🔁 Retried jobs
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 62f5c70 | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
Pull request overview
Adds Dispatcher progress reporting through a single continuously updated pull-request comment.
Changes:
- Adds comment rendering, truncation, failure details, and progress summaries.
- Adds a PR updater with revision ordering, retries, and comment reuse.
- Extends the async GitHub client and test infrastructure for issue comments.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ddev/src/ddev/cli/ci/tests/pr_comment.py |
Renders Dispatcher progress comments. |
ddev/src/ddev/cli/ci/tests/task_pull_request_updater.py |
Creates and updates the PR comment. |
ddev/src/ddev/utils/github_async/client.py |
Adds list and update comment endpoints. |
ddev/tests/cli/ci/tests/test_pr_comment.py |
Tests comment rendering behavior. |
ddev/tests/cli/ci/tests/test_task_pull_request_updater.py |
Tests updater ordering and failures. |
ddev/tests/cli/ci/tests/test_task_test_gatherer.py |
Tests gatherer-to-updater integration. |
ddev/tests/cli/ci/tests/preview_pr_comment.py |
Generates manual rendering previews. |
ddev/tests/helpers/github_async.py |
Extends the fake GitHub client. |
ddev/tests/test_fake_github_async.py |
Tests fake comment operations. |
ddev/tests/utils/github_async/test_endpoints.py |
Tests new GitHub endpoints. |
ddev/changelog.d/24822.added |
Records the new updater feature. |
Suppressed comments (3)
ddev/src/ddev/cli/ci/tests/task_pull_request_updater.py:148
- A PR author can place this public marker in their own comment. The next run will select that comment, but GitHub does not allow the Dispatcher token to edit another user's comment, so every update fails instead of finding or creating the bot-owned comment. Matching the marker does not prove authorship; also verify the expected bot/app author (and preferably require the marker at the start) before caching the ID.
async for page in self._client.list_issue_comments(self._options.owner, self._options.repo, pr_number):
for comment in page.data:
# Only Dispatcher writes the marker, so matching it is enough to prove authorship.
if COMMENT_MARKER in comment.body:
self._comment_id = comment.id
ddev/src/ddev/cli/ci/tests/task_pull_request_updater.py:98
- This catches the client's actionable
GitHubAuthenticationError(a subclass ofHTTPStatusError) and retries it as though it were transient. On the final snapshot it is then replaced by the generic “Could not write” error, hiding the token/permission guidance; on intermediate snapshots it is swallowed entirely. Authentication failures should stop immediately while preserving their actionable message.
except httpx.HTTPError as error:
if _is_body_too_long(error):
# Not transient: the same body would be rejected again.
return await self._write_minimal(pr_number, message, log_extra)
ddev/src/ddev/utils/github_async/client.py:515
- This new public endpoint is absent from
ENDPOINT_CALLS, so it misses the suite's shared HTTP-error, authentication-error, and response-header coverage.ddev/tests/utils/github_async/AGENTS.md:31-36requires every public endpoint to be registered; add a first-page list-comments case to that registry.
async def list_issue_comments(
self,
owner: str,
repo: str,
issue_number: int,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment content: - Stop rendering the message revision: internal ordering metadata the PR author cannot act on. - Render a retrying batch as plain "in progress"; the retry is reported per job, where it is actionable. - Put failing test and step names in <code> rather than Markdown code spans. html.escape leaves backticks alone, and a pytest parametrised id can contain one, which closed the span early. Updater robustness: - Match the marker only at the start of a body, and treat a 403/404 on edit as "not our comment": forget it, remember it as unusable so the lookup cannot re-adopt it, and create one we own. Quoting the bot's comment copies the marker, which previously wedged the run entirely. - Let GitHubAuthenticationError propagate instead of retrying it and replacing its actionable message with a generic write failure. Test infrastructure: - Register update_issue_comment and list_issue_comments in ENDPOINT_CALLS, as tests/utils/github_async/AGENTS.md requires, so they get the shared error and header coverage. - Fake client: an empty comment list is one empty page, matching the real client, not zero pages. - Fix the preview script's documented invocation, which could not resolve its imports as written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Measured the encoding margin rather than assuming it: a full 240-job comment is 59,892 characters and 59,976 bytes — 1.0014x — because emoji appear once per batch row and per heading, so their share falls as the comment fills with ASCII test names and URLs. The remaining ~5.5k of headroom is for GitHub's undocumented accounting, not for UTF-8. Two tests hold it: the worst-case body must encode to under 65,536 bytes, and the budget must keep a flat 5,000-character reserve below that limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 408bd27667
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return progress.failed > 0 or any( | ||
| batch.status is Status.FAILURE or batch.error is not None for batch in progress.batches |
There was a problem hiding this comment.
When a workflow job succeeds but its artifacts are missing, TaskTestGatherer records attempt.error = NO_ARTIFACTS while leaving the job and batch status as SUCCESS. _unavailable() will still render the warning for that job, but this predicate returns false, so the final comment heading/footer can say the Dispatcher tests passed even though the result was explicitly unavailable. Include latest job-attempt errors in this failure check so unavailable results cannot be presented as a passing run.
Useful? React with 👍 / 👎.
evalya-impact-summaryevalya impact analysis |
The client maps every non-rate-limit 403 to GitHubAuthenticationError, and that class subclasses httpx.HTTPStatusError. Because _write checked the authentication clause first, a 403 meaning "this comment is not yours to edit" was re-raised as a credentials failure and never reached _forget_unusable_comment, so only a 404 exercised the recovery that UNUSABLE_COMMENT_STATUSES advertises for both. The authentication clause now offers the error to that recovery before treating it as a token problem. A 401, or a 403 with no comment to forget, still propagates immediately with the client's own message. _write_minimal gains the same recovery, bounded to two passes: the second write targets a comment we just created, so it cannot be refused for not being ours twice. Losing the fallback is the worst place to give up, since it is the run's last chance to report anything. Its give-up policy moves to a module-level helper rather than being repeated at each exit. The tests that should have caught this were injecting a bare HTTPStatusError for a 403, a shape the real client never produces, so they passed against the broken path. They now build the error the client actually raises, and cover the minimal-retry recovery plus two guards that a genuinely rejected token is not swallowed into a silent comment rewrite. Also replaces an unguarded re.search(...).group(1) in the gatherer suite with the guarded helper the updater suite already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A job can conclude success while its artifacts never arrive: the gatherer derives the job's status from the workflow conclusion and sets NO_ARTIFACTS from the missing artifact directory, two independent sources with nothing coupling them. _has_failure only honoured batch-level errors, so such a run rendered "✅ Dispatcher tests · passed" and an "⚠️ Unavailable results" section at the same time, contradicting _unavailable's own contract that batches and jobs whose result could not be established are never rendered as success. Counting attempt errors as failures would have swapped one contradiction for another: the failures section only lists jobs whose status is FAILURE, so the comment would have said "failed" with a CAUTION pointing at a section that was never rendered, next to "❌ 0 failed" in the totals. So a run whose results could not be established now reads as neither passed nor failed but as a third state, with the heading, the alert and the footer all agreeing. A failure still outranks it, since it is the more actionable of the two. Batch errors move out of _has_failure for the same reason. That also fixes the case where a workflow concluded successfully but reported no jobs: it used to render as a failure with an empty failures section. A timeout is unaffected and still reads as a failure, because it fails every job in the batch. The count in the alert and the bullets in the section now come from one list, so they cannot disagree with each other. Counters are untouched: a missing-artifacts job still counts as passed, which progress.py establishes deliberately — the error qualifies the execution, it does not erase it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation ReportAll 21 validations passed. Show details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62f5c70109
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def _is_body_too_long(error: httpx.HTTPError) -> bool: | ||
| """Whether *error* is GitHub rejecting the comment body for exceeding its length limit.""" | ||
| return isinstance(error, httpx.HTTPStatusError) and error.response.status_code == BODY_TOO_LONG_STATUS |
There was a problem hiding this comment.
When GitHub returns 422 for a comment validation/spam response instead of the specific over-long-body validation (GitHub documents 422 for these endpoints as validation failed or spammed), this predicate classifies it as BODY_TOO_LONG_STATUS, so _write() immediately resubmits a minimal comment and can hide the real failure instead of surfacing or retrying it correctly. Gate the fallback on the response body/error code for the “body is too long” validation, not just the status code.
Useful? React with 👍 / 👎.
Preview: the all-passing final commentThe preview above covers starting, mid-run with a batch retrying, and finished with failures. This is the remaining case — a run where everything passed — which is what "successful detail is deliberately thin" means in practice: no Failures section, no Unavailable results, no Retried jobs, because there is nothing a reader has to act on. Produced by ✅ Dispatcher tests · passed
Batches
|
Preview: a run whose results could not be establishedSame twelve jobs as the passing preview above, with one difference: Two things are worth reading carefully, because they look like a contradiction and are not:
Before the fix this exact snapshot rendered The marker is stripped here for the same reason as the previous preview.
|
| Batch | State | Jobs | Workflow |
|---|---|---|---|
batch-01 | ✅ passed | 4/4 | run 121 |
batch-02 | ✅ passed | 4/4 | run 122 |
batch-03 | ✅ passed | 4/4 | run 123 |
⚠️ Unavailable results
vault / py3.12 / linux— no artifacts were downloaded for this job
What does this PR do?
Gives Dispatcher a voice on the pull request. Until now it ran the tests, gathered every result, and told nobody — the progress snapshots it produces had no consumer.
Dispatcher now maintains exactly one comment per pull request, posted as soon as the run starts and then edited in place, over and over, as each batch finishes. It never posts a second comment — not on later updates, and not on a later run against the same pull request, because it finds its previous comment by a hidden marker and edits that one. Within a run it never goes backwards: an update that arrives late is discarded rather than allowed to overwrite fresher results. (One active Dispatcher execution per PR is a design precondition, enforced by workflow concurrency.)
The comment answers, at a glance:
Successful detail is deliberately thin. A run where everything passes is a few lines; the space goes to what a reader has to act on.
What it looks like
One preview comment on this PR shows the real rendering at three points in a run — starting, mid-run with a batch retrying, and finished with failures. It is a single comment edited three times, which is exactly how the real thing behaves.
The starting state, in full:
and once it has finished, with something to report:
Notes for reviewers
An in-progress comment can never be mistaken for a final one. That is stated four ways — the heading, the callout, a bar that refuses to fill, and the footer — because a run that reads as finished when it is not is the one failure mode that actively misleads. A test asserts all four agree with each other.
A batch can fail even when every one of its jobs passed. That is a workflow-level failure — a setup, upload or cleanup step — and the comment reports it as a failure rather than quietly summing up the jobs and calling it green.
Runs with no pull request still work. Pushes to
master, the nightly schedule and merge-queue runs have nothing to comment on, so the summary goes to the logs and no API call is made. The same set-up runs in every context.If GitHub rejects the comment as too long, it is re-posted without the failure detail rather than dropped. A short comment beats no comment.
The marker identifies the comment but does not prove we own it. Quoting the bot's comment copies the marker, so an edit GitHub refuses (403/404) is treated as "not our comment": Dispatcher forgets it and writes one it does own, rather than retrying an edit that can never succeed. A rejected token, by contrast, propagates immediately with its own message instead of being retried.
Retry reporting is written but not yet reachable — it lights up when the retry work lands. Wiring Dispatcher to actually create this updater is the entry-point task (AI-6474 / AI-6484) and is not part of this PR.
Motivation
AI-6480. Dispatcher has been producing complete result snapshots since #24774 with nothing consuming them, so it currently gives no feedback to the person whose PR it is testing.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged