From 71d92df2a901c6d6bc0dcda7f05cba364cc3d44d Mon Sep 17 00:00:00 2001 From: Brandon Chatham Date: Thu, 30 Jul 2026 15:29:48 -0700 Subject: [PATCH 1/3] fix(xreview): provide the driver interpreter instead of trusting the 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 --- .github/seidroid/xreview/README.md | 11 +++++++++-- .github/workflows/seidroid-xreview.yml | 10 +++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/.github/seidroid/xreview/README.md b/.github/seidroid/xreview/README.md index 5213aea..45bc11f 100644 --- a/.github/seidroid/xreview/README.md +++ b/.github/seidroid/xreview/README.md @@ -145,8 +145,15 @@ until the items above hold. The driver has been exercised end-to-end against live PRs: mint, session create, sandbox launch, the agent reading the PR through the credential bridge, a structured verdict, and -teardown. The **reusable workflow and its thin caller** are being wired into their first repo -now; the first `seidroid xreview` comment there is the comment-trigger path's first real test. +teardown. The **reusable workflow and its thin caller** have carried a real +`seidroid xreview` comment as far as the in-cluster runner: the trusted-commenter guard +admitted it, the `xreview` job scheduled on `uci-default`, and the driver checked out at the +caller's `uci-ref`. The steps from the bearer mint onward have not yet run on that runner, +so treat the in-cluster half of the path as unproven until one run posts a verdict. + +Expect the gaps that remain to be runner-image ones rather than logic ones. The in-cluster +image is not the hosted image the driver was developed against, and the mint and post steps +assume `curl` and `gh` are on it, which is not yet confirmed the way the interpreter now is. Known gaps for a follow-up: the verdict currently posts as `github-actions[bot]` rather than `seidroid[bot]` (posting via the seidroid app token is a later change); and the diff --git a/.github/workflows/seidroid-xreview.yml b/.github/workflows/seidroid-xreview.yml index dc94e93..0032ccb 100644 --- a/.github/workflows/seidroid-xreview.yml +++ b/.github/workflows/seidroid-xreview.yml @@ -104,13 +104,21 @@ jobs: sparse-checkout-cone-mode: false persist-credentials: false + # The interpreter comes from here rather than the runner image because an image is + # free to ship python3 without the venv module: the in-cluster ARC image does, and + # the venv below cannot bootstrap without ensurepip. This also fixes the version + # the driver runs under instead of inheriting whatever the image happens to carry. + - name: Set up Python + uses: actions/setup-python@v7 + with: + python-version: '3.12' + - name: Set up driver runtime id: runtime shell: bash run: | set -euo pipefail # A pinned venv keeps the httpx version deterministic regardless of the image. - command -v python3 >/dev/null 2>&1 || { echo "python3 not found on runner" >&2; exit 1; } venv="$RUNNER_TEMP/seidroid-venv" python3 -m venv "$venv" "$venv/bin/python" -m pip install --disable-pip-version-check --no-input --quiet "httpx==0.27.2" From ac0c688a3f177ae1138e0451870f8e90d8b7237a Mon Sep 17 00:00:00 2001 From: Brandon Chatham Date: Thu, 30 Jul 2026 15:33:48 -0700 Subject: [PATCH 2/3] fix(xreview): post the verdict without the gh CLI 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 --- .github/seidroid/xreview/README.md | 8 +++-- .github/workflows/seidroid-xreview.yml | 49 ++++++++++++++++---------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/.github/seidroid/xreview/README.md b/.github/seidroid/xreview/README.md index 45bc11f..edafa85 100644 --- a/.github/seidroid/xreview/README.md +++ b/.github/seidroid/xreview/README.md @@ -151,9 +151,11 @@ admitted it, the `xreview` job scheduled on `uci-default`, and the driver checke caller's `uci-ref`. The steps from the bearer mint onward have not yet run on that runner, so treat the in-cluster half of the path as unproven until one run posts a verdict. -Expect the gaps that remain to be runner-image ones rather than logic ones. The in-cluster -image is not the hosted image the driver was developed against, and the mint and post steps -assume `curl` and `gh` are on it, which is not yet confirmed the way the interpreter now is. +The gaps that surfaced there were runner-image ones rather than logic ones, because the +in-cluster image is a minimal runner rather than the hosted image the driver was developed +against. It carries `curl`, `jq` and `git`, and a `python3` with no venv module; it does not +carry the `gh` CLI. So the workflow now brings its own interpreter and posts through the +API client `github-script` provides, and depends on the image only for `curl`. Known gaps for a follow-up: the verdict currently posts as `github-actions[bot]` rather than `seidroid[bot]` (posting via the seidroid app token is a later change); and the diff --git a/.github/workflows/seidroid-xreview.yml b/.github/workflows/seidroid-xreview.yml index 0032ccb..8cb579e 100644 --- a/.github/workflows/seidroid-xreview.yml +++ b/.github/workflows/seidroid-xreview.yml @@ -168,9 +168,8 @@ jobs: set -euo pipefail # Create exactly ONE managed sei-droid session, drive it, write the verdict # to verdict.md ONLY on a real verdict (no placeholder), and DELETE the - # session on exit AND on SIGTERM/SIGINT. Fail-closed: anything other than the - # exact opt-in 'false' runs dry. --trigger-id scopes the run key to this - # comment so a re-fire adopts rather than resurrects. + # session on exit AND on SIGTERM/SIGINT. --trigger-id scopes the run key to + # this comment so a re-fire adopts rather than resurrects. args=("$REPO" "$PR" --out verdict.md) if [ -n "${TRIGGER_ID:-}" ]; then args+=(--trigger-id "$TRIGGER_ID"); fi set +e @@ -196,22 +195,36 @@ jobs: # Post only when a real verdict was produced. Keying on verdict_produced, not # the exit code, so a teardown-only failure still posts a valid verdict and a # no-verdict run never upserts a placeholder. + # + # Posted through github-script rather than the gh CLI because the in-cluster + # runner image does not carry gh, and because the API client it hands the script + # is the same token this job already holds. The repo comes from the run context + # and the pr number from the guard's output, so neither is interpolated into a + # shell command. if: ${{ steps.drive.outputs.verdict_produced == 'true' }} - shell: bash + uses: actions/github-script@v9 env: - GH_TOKEN: ${{ github.token }} MARKER: "" - REPO: ${{ github.repository }} PR: ${{ needs.guard.outputs.pr_number }} - run: | - set -euo pipefail - body="$MARKER"$'\n'"$(cat verdict.md)" - # One bot comment per PR: find by marker -> PATCH, else POST. repo/pr from env, - # not template-interpolated into the script. - id="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \ - --jq "map(select(.body | startswith(\"$MARKER\"))) | .[0].id // empty")" - if [ -n "$id" ]; then - gh api -X PATCH "repos/$REPO/issues/comments/$id" -f body="$body" >/dev/null - else - gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null - fi + with: + script: | + const fs = require('fs'); + const marker = process.env.MARKER; + const issue_number = Number(process.env.PR); + const body = `${marker}\n${fs.readFileSync('verdict.md', 'utf8')}`; + // One bot comment per PR: find this bot's marker and edit it, else create. + const comments = await github.paginate(github.rest.issues.listComments, { + ...context.repo, + issue_number, + per_page: 100, + }); + const existing = comments.find((c) => c.body?.startsWith(marker)); + if (existing) { + await github.rest.issues.updateComment({ + ...context.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ ...context.repo, issue_number, body }); + } From 2adb1b2660731701b131d4d4cb61a5b3cf46d527 Mon Sep 17 00:00:00 2001 From: Brandon Chatham Date: Fri, 31 Jul 2026 08:31:53 -0700 Subject: [PATCH 3/3] fix(xreview): harden the verdict post and bound the job 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 --- .github/seidroid/xreview/README.md | 11 ++++-- .github/workflows/seidroid-xreview.yml | 47 ++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/.github/seidroid/xreview/README.md b/.github/seidroid/xreview/README.md index edafa85..f9e8822 100644 --- a/.github/seidroid/xreview/README.md +++ b/.github/seidroid/xreview/README.md @@ -153,9 +153,14 @@ so treat the in-cluster half of the path as unproven until one run posts a verdi The gaps that surfaced there were runner-image ones rather than logic ones, because the in-cluster image is a minimal runner rather than the hosted image the driver was developed -against. It carries `curl`, `jq` and `git`, and a `python3` with no venv module; it does not -carry the `gh` CLI. So the workflow now brings its own interpreter and posts through the -API client `github-script` provides, and depends on the image only for `curl`. +against. It carries `curl` and `git`, and a `python3` with no venv module; it does not carry +the `gh` CLI. So the workflow now brings its own interpreter and posts through the API +client `github-script` provides. + +What it still expects from the image is `curl`, for the bearer mint, `git`, for the driver +checkout, and `bash` plus `tar` and the usual coreutils, which the interpreter setup needs +to unpack and install what it downloads. The node the three actions run on comes from the +runner rather than the image. Known gaps for a follow-up: the verdict currently posts as `github-actions[bot]` rather than `seidroid[bot]` (posting via the seidroid app token is a later change); and the diff --git a/.github/workflows/seidroid-xreview.yml b/.github/workflows/seidroid-xreview.yml index 8cb579e..187f009 100644 --- a/.github/workflows/seidroid-xreview.yml +++ b/.github/workflows/seidroid-xreview.yml @@ -84,6 +84,10 @@ jobs: # The reviewed repo's runner label. Org ARC scale set by default, in-cluster, so it # reaches omnigent over the ClusterIP Service and mints its bearer there. runs-on: ${{ inputs.runs-on }} + # A stalled fetch, download, pip resolve, or mint would otherwise hold a runner (and a + # live credential) until the six-hour default. The driver bounds its own run; this + # bounds everything around it. + timeout-minutes: 30 permissions: pull-requests: write # upsert the one sticky verdict comment contents: read # read PR metadata @@ -109,6 +113,7 @@ jobs: # the venv below cannot bootstrap without ensurepip. This also fixes the version # the driver runs under instead of inheriting whatever the image happens to carry. - name: Set up Python + id: python uses: actions/setup-python@v7 with: python-version: '3.12' @@ -116,11 +121,14 @@ jobs: - name: Set up driver runtime id: runtime shell: bash + env: + SETUP_PYTHON: ${{ steps.python.outputs.python-path }} run: | set -euo pipefail # A pinned venv keeps the httpx version deterministic regardless of the image. venv="$RUNNER_TEMP/seidroid-venv" - python3 -m venv "$venv" + # The interpreter the setup step resolved, not whatever PATH offers. + "$SETUP_PYTHON" -m venv "$venv" "$venv/bin/python" -m pip install --disable-pip-version-check --no-input --quiet "httpx==0.27.2" echo "python=$venv/bin/python" >> "$GITHUB_OUTPUT" @@ -136,6 +144,7 @@ jobs: client_id="${OMNIGENT_M2M_CLIENT_ID:-sei-droid}" : "${OMNIGENT_M2M_CLIENT_SECRET:?OMNIGENT_M2M_CLIENT_SECRET not provided by the caller}" resp="$(curl -sS -w $'\n%{http_code}' \ + --connect-timeout 5 --max-time 30 \ --config <(printf 'user = "%s:%s"\n' "$client_id" "$OMNIGENT_M2M_CLIENT_SECRET") \ -d grant_type=client_credentials \ "$BASE_URL/oauth/token")" || { echo "omnigent /oauth/token request failed" >&2; exit 1; } @@ -180,7 +189,10 @@ jobs: # Downstream gates on whether a verdict was PRODUCED (verdict.md non-empty), # not on the exit code: a teardown-only failure still publishes, and a # no-verdict run never posts a placeholder. - if [ -s verdict.md ]; then + # Non-whitespace rather than non-empty: a final agent message with no text + # block writes a lone newline, which -s admits and which would post a comment + # carrying nothing but the marker. + if grep -q '[^[:space:]]' verdict.md 2>/dev/null; then echo "verdict_produced=true" >> "$GITHUB_OUTPUT" if [ "$rc" -ne 0 ]; then echo "::warning::driver exited $rc but produced a verdict (e.g. a teardown leak); see logs" @@ -207,24 +219,47 @@ jobs: MARKER: "" PR: ${{ needs.guard.outputs.pr_number }} with: + # A 429 or 5xx on this call would otherwise discard a completed review; the + # retry set excludes 4xx other than 429, so a permission or validation error + # still fails fast. + retries: 3 script: | const fs = require('fs'); + const path = require('path'); const marker = process.env.MARKER; const issue_number = Number(process.env.PR); - const body = `${marker}\n${fs.readFileSync('verdict.md', 'utf8')}`; - // One bot comment per PR: find this bot's marker and edit it, else create. + // Anchored to the workspace rather than the process cwd, which a later + // working-directory default or a step reorder could move out from under this. + const verdict = path.join(process.env.GITHUB_WORKSPACE, 'verdict.md'); + const body = `${marker}\n${fs.readFileSync(verdict, 'utf8')}`; + const comments = await github.paginate(github.rest.issues.listComments, { ...context.repo, issue_number, per_page: 100, }); - const existing = comments.find((c) => c.body?.startsWith(marker)); + // One bot comment per PR: edit the marked one, else create it. The author has + // to be a bot as well as the marker matching. Anyone who can comment on the PR + // can write the 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. Matching on the marker alone would let such a comment + // either capture the verdict slot, since this token can edit it, or fail the + // step and suppress the review. + const existing = comments.find( + (c) => c.body?.startsWith(marker) && c.user?.type === 'Bot', + ); if (existing) { await github.rest.issues.updateComment({ ...context.repo, comment_id: existing.id, body, }); + core.info(`updated verdict comment ${existing.id} by ${existing.user?.login}`); } else { - await github.rest.issues.createComment({ ...context.repo, issue_number, body }); + const created = await github.rest.issues.createComment({ + ...context.repo, + issue_number, + body, + }); + core.info(`created verdict comment ${created.data.id}`); }