fix(xreview): bring the driver's tooling instead of reading it off the runner image - #52
Open
bdchatham wants to merge 3 commits into
Open
fix(xreview): bring the driver's tooling instead of reading it off the runner image#52bdchatham wants to merge 3 commits into
bdchatham wants to merge 3 commits into
Conversation
…runner image The Review job runs on the in-cluster uci-default ARC scale set, whose image ships python3 without the venv module. `python3 -m venv` therefore failed with "ensurepip is not available" on the first real run, before the bearer mint, so the review produced no verdict and posted nothing. The step had only ever run on a hosted image, which bundles venv. Take the interpreter from actions/setup-python rather than the image, which also fixes the version the driver runs under. The image-probe guard goes away with it, since the interpreter is now provided rather than discovered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR SummaryMedium Risk Overview The review job adds Operational hardening: 30-minute job timeout, curl connect/max time on omnigent token mint, and non-whitespace check before treating Reviewed by Cursor Bugbot for commit 2adb1b2. Bugbot is set up for automated code reviews on this repo. Configure here. |
The in-cluster runner image does not carry gh. Its base installs curl, jq and git and nothing else relevant, and the layer over that adds only build-essential and ca-certificates, so the sticky-upsert step would have failed the same way the interpreter did once a run got that far. Post through github-script instead, which runs on the node the runner already has and is handed an API client using this job's token. The upsert keeps the same marker, so a comment posted by an earlier revision is still the one that gets edited. Also drop a stale sentence describing the dry-run gate, which no longer exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bdchatham
changed the title
The sticky-comment match keyed on the marker alone, and anyone who can comment on a reviewed PR can write that marker, including the untrusted PR author, whom the trigger guard does not cover because it gates who may run the bot rather than whose code is reviewed. Such a comment either captured the verdict slot, since this job's token can edit it, or failed the step and suppressed the review. Require a bot author as well. Also bound what the run can hold: a job timeout, connect and total timeouts on the bearer mint, and retries on the one API call that would otherwise discard a finished review. The verdict gate now tests for non-whitespace rather than a non-empty file, since a final agent message carrying no text writes a lone newline and would post a marker-only comment. The interpreter comes from the setup step's own output rather than PATH, the verdict path is anchored to the workspace, and the post logs which comment it wrote. Corrects the README's image-dependency list, which claimed curl was the only one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7 tasks
Contributor
Author
|
Superseded by #54, which removes xreview while the team reconsiders its approach. This PR exists because the driver's interpreter had to be supplied rather than read off the runner image, which is one of the concrete design questions #54 records for the next attempt. Worth closing rather than merging: the fix is real, but it lands on code we are withdrawing, and the constraint it works around is better addressed by deciding where the driver should live. The change stays in history if the next approach keeps a Python driver in |
bdchatham
added a commit
that referenced
this pull request
Aug 3, 2026
## What this does Removes `xreview` from `.github/seidroid/`, along with its reusable workflow. 2,064 lines deleted across 16 files. `ai-review` is untouched. ## Why now, given it works xreview reached working end to end. It drove the `sei-droid` agent inside a managed omnigent Kubernetes sandbox with a real `git`/`gh` toolchain, so a review could build, test and inspect a tree rather than only read its diff, and it came back with one structured verdict. The reusable workflow, the session driver, idempotency, policy and the verdict post all landed and ran. Having the whole shape running is exactly what made the design questions legible, and those questions are cheaper to answer on a clean base than by amending a working implementation nobody has committed to yet. Better to iterate on the approach before it becomes something other repos depend on. The implementation is not lost. It stays in history at this PR's merge-base, and the callers below pin it by SHA, so it remains runnable while we decide. ## Nothing breaks Two repos call the reusable workflow: | Caller | Pin | |---|---| | `sei-protocol/sei-chain` | `…@6590124` (uci v0.0.15), `uci-ref` same commit | | `sei-protocol/platform` | `…@6590124` (uci v0.0.15), `uci-ref` same commit | Both pin a specific commit for **both** `uses:` and `uci-ref`, so they resolve this workflow from git history and keep working unchanged after this merge. I verified both files rather than assuming. **But they should not be left as they are.** Each still advertises a `seidroid xreview` PR-comment trigger for a feature no longer maintained on `main`, so a contributor could invoke it and get a result from code we have stopped iterating on. I would rather remove those two callers in follow-ups than leave them pinned to an abandoned path. Happy to open both, or leave them to whoever owns those repos, whichever the team prefers. ## Supersedes #52 (`fix/xreview-setup-python`) is an in-flight xreview fix and becomes moot. It should be closed rather than merged. ## What I would want the discussion to cover Recording these so the next attempt starts from the questions rather than rediscovering them: - **Where the driver belongs.** A Python driver living in `.github/` is awkward: it needs its own `pyproject.toml`, it is invisible to the repo's normal test and lint gates, and its interpreter has to be supplied rather than assumed. #52 exists because of that last point specifically. - **Trigger shape.** A `seidroid xreview` comment is discoverable but unbounded. Any PR can start a credentialed sandbox session, and nothing sizes the cost against the change. - **Where the verdict lands.** A PR comment reads well for a human and is awkward to gate on. A check run gates well and is worse to read. - **The pinning contract.** Callers pin `uses:` and `uci-ref` to the same commit by hand. That is the thing which keeps this PR from breaking anyone, and it is also a step that is easy to get wrong in a way nothing catches. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] UI / frontend change - [x] Refactor / chore - [ ] Docs - [ ] Test / CI - [ ] Breaking change Not marked breaking: both callers pin by SHA and are unaffected on merge. ## Test Plan No behaviour to test; this is a deletion. What I verified instead: - Every tracked path matching `xreview` is gone: the 14-file `xreview/` tree plus `.github/workflows/seidroid-xreview.yml`. - No dangling references remain. `git grep -i xreview` returns one hit, the deliberate note in `.github/seidroid/README.md` explaining where the feature went. - `git grep seidroid-xreview` returns nothing. - `.github/seidroid/` retains `ai-review/` and `README.md`; `.github/workflows/` retains its other nine workflows. - Both external callers read and confirmed pinned by SHA. - The README's closing paragraph existed only to contrast the two review paths, so it is rewritten rather than left describing a path that no longer exists. Signed-off-by: bdchatham <bdchatham@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
The first real
seidroid xreviewcomment reached the in-cluster runner and failed before it could mint a bearer. The Review job runs on theuci-defaultARC scale set, whose image shipspython3without the venv module, so building the driver's virtualenv exited withensurepip is not available. Everything downstream was skipped, which is why the run produced no verdict and correctly posted nothing.The step had only ever run on a GitHub-hosted image before, where venv is bundled. The underlying problem is that the workflow was reading tooling off the runner rather than bringing it, and that image is a minimal runner rather than the hosted one.
So this fixes both places it does that, not just the one that failed. Reading the image confirms what is actually there: the base installs
curl,jqandgit, the layer over it addsbuild-essentialandca-certificates, andpython3arrives only as a dependency ofsoftware-properties-common, which is why it has no venv. There is nogh. The verdict post would therefore have failed the same way as soon as a run got that far.Changes
The interpreter now comes from
actions/setup-pythonrather than from whatever the image happens to carry, which also fixes the version the driver runs under instead of inheriting it. Thecommand -v python3probe goes away with that, since the interpreter is provided rather than discovered.The verdict post moves from the
ghCLI togithub-script, which runs on the node the runner already has and is handed an API client using the token this job already holds. The marker is unchanged, so a comment posted by an earlier revision is still the one that gets edited rather than duplicated. Both actions are pinned at the major tag, matching how this repo pinscheckout,setup-goandgithub-scriptelsewhere.The workflow now depends on the image only for
curl, in the mint step, which is confirmed present.A stale sentence describing the dry-run gate is also removed, since that gate no longer exists.
Verification
Confined to the reusable workflow, so the real check is a live run rather than anything CI here can assert. The workflow parses and the step order is unchanged apart from the insertion.
setup-python's download path on this runner is not a guess: the same scale set already runsactions/setup-goagainst an effectively empty tool cache in sei-chain's test workflow, so the tool-download path is exercised there today.The remaining unknown is the part no static check reaches, which is whether the driver completes a review from inside the cluster. That is what the next run answers.
Rollout
Consumers pin both
uses:anduci-refto a commit, so this needs a release and a matching bump in the sei-chain and platform callers before it changes anything.🤖 Generated with Claude Code