Skip to content

feat: add runs-on input to the shareable workflows - #67

Merged
hln33 merged 5 commits into
mainfrom
ci/runs-on-input
Oct 9, 2026
Merged

hln33 merged 5 commits into
mainfrom
ci/runs-on-input

Conversation

@wangyb-A

@wangyb-A wangyb-A commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds an optional runs-on input to every shareable workflow so a consuming repository can run the jobs on its own runner, for example the CodeBuild-hosted GitHub Actions runner label codebuild-github-actions-runner-${{ github.run_id }}-${{ github.run_attempt }}.

  • The input is declared on all ten workflow_call workflows (kebab-case, like the existing inputs), defaulting to ubuntu-latest.
  • Each of the 19 runs-on: ubuntu-latest becomes runs-on: ${{ inputs['runs-on'] || 'ubuntu-latest' }}. The || fallback matches how the other inputs are read and keeps the directly triggered forms of these workflows (issue_comment, pull_request_target, ...) working, where the inputs context is empty.
  • The three entry workflows that call other workflows in this repository (ai-pr-review -> claude-review / codex-review; ai-pr-review-address and ai-issue-implementation -> ai-work-item-resolver / codex-issue-worker) forward the input, so a single value from the caller reaches every job.
  • README gains a short "Runner selection" section. ai-pr-review-address-continuation.yml is untouched: it has no workflow_call surface and runs only in this repository.

Behaviour on merge

No behaviour change for any existing caller. Every consumer (aws-durable-execution-sdk-js, -python, -java, -rust, -conformance-tests, -docs) pins these workflows by commit SHA, so none of them sees this change until it bumps the pin, and a caller that bumps without setting runs-on still gets ubuntu-latest.

Runner requirements for callers

Noted in the README so callers can check their runner before opting in:

  • claude-review, codex-review, codex-issue-worker, issue-triage and notify create an unprivileged user and drop into it with sudo, so the runner must run jobs as an account with passwordless sudo and provide adduser and setfacl. The account's name does not matter (see below).
  • Longest timeout-minutes: codex-issue-worker 140 (used by ai-pr-review-address and ai-issue-implementation), codex-review 45, claude-review 30, issue-triage 30. A runner whose own build timeout is lower than these will kill the job.

Removing the runner account assumption

The Codex review on this PR pointed out that the model workflows wrote the invoking account's name as the literal runner, which only holds on GitHub-hosted runners. Measured on the CodeBuild project default (EC2, standard:7.0) in aws/aws-durable-execution-sdk-js#968:

Assumption GitHub-hosted CodeBuild-hosted Effect before this fix
job runs as runner yes root Defaults:runner env_keep never matches, so sudo -u <user> drops the AWS credentials and the model cannot reach Bedrock
runner user/group exists yes no chown runner:<user> and install -o runner fail with invalid user; the setup step exits before the model runs
workspace lives under /home/runner yes /codebuild/output/.../_work/<repo>/<repo>, every ancestor 755 setfacl ... /home/runner fails on a missing path

Changes:

  • The sudoers env_keep rules are removed altogether (see the next section); nothing is written to /etc/sudoers.d any more.
  • chown / install owner and group use $(id -un) / $(id -gn) (prepare_ai_review_user.sh, prepare_codex_implementation_user.sh, codex-review, codex-issue-worker, issue-triage, notify).
  • The /home/runner execute-only ACL becomes a top-down walk over the workspace's ancestors that grants --x exactly where the unprivileged user is blocked. On GitHub-hosted runners this reproduces the previous behaviour (/home/runner is the only blocking ancestor); on CodeBuild it is a no-op.
  • tests/test_prepare_codex_implementation_user.py and tests/test_codex_issue_implementation.py updated for the new strings; full suite passes (301 tests).

The privilege separation itself is unchanged: the model still runs as a freshly created user that is not in sudoers.

Passing the model environment as an explicit allowlist

The follow-up Codex finding on the first fix was that Defaults:<user> env_keep += ... only narrows the environment if the runner image's own sudoers still has env_reset enabled and no broader env_keep. That is the image's choice, not ours, and with a configurable runs-on an image that disables the reset would pass the whole job environment (OIDC request token, CodeBuild credential URI, GH_TOKEN) into the model process.

Every model process is now started with sudo -u <user> -- env -i and an explicit allowlist, the pattern codex-issue-worker already used:

  • codex-review, issue-triage, notify: the five AWS variables, AWS_EC2_METADATA_DISABLED=true, CODEX_HOME, and the variables sudo's reset used to keep (HOME, PATH, LANG, USER, LOGNAME, SHELL). The preflight that checked whether sudo had preserved the credentials now checks the job environment directly.
  • claude-review via scripts/run_claude_isolated.sh: the same AWS set plus the 8 ANTHROPIC_* / CLAUDE_CODE_* / DETAILED_PERMISSION_MESSAGES settings the old env_keep rules named; unset optional variables are skipped rather than passed as empty strings.
  • The four /etc/sudoers.d writes and their visudo checks are deleted, as is the sudoers block in prepare_ai_summary_user.sh.

On GitHub-hosted runners the model sees the same environment as before (the allowlist is the old env_keep set plus what env_reset kept). Everywhere else it is independent of sudo policy, which is also what keeps the CodeBuild project role out of reach: the aws/aws-durable-execution-sdk-js#968 probe confirmed the unprivileged user inherits no AWS_CONTAINER_* variables and cannot read the root process environment, and with env -i that no longer depends on the image.

Context

Part of moving Durable Execution SDK CI onto CodeBuild-hosted runners (aws/aws-durable-execution-sdk-js#937, aws/aws-durable-execution-sdk-python#747, aws/aws-durable-execution-sdk-java#751). Those PRs migrate each repository's own workflows; the thin shells that call this repository were left on ubuntu-latest because their runs-on lives here. Follow-up PRs in the SDK repositories will bump the pin and pass the label. Companion change for the OpenTelemetry conformance orchestrator: aws/aws-durable-execution-conformance-tests#132.

Verification

  • All 11 workflow files parse as YAML; the 10 workflow_call workflows expose runs-on with the ubuntu-latest default.
  • grep ubuntu-latest over the workflows now matches only the input defaults and the || fallbacks; grep for Defaults:runner, -o runner, runner: and /home/runner (outside comments) over scripts and workflows matches nothing.
  • python3 -m unittest discover -s tests: 301 tests, OK (test_serve_aws_credentials is intermittently flaky on a loopback-socket race, unrelated to this change). bash -n on the three edited scripts. Ancestor enumeration checked against both workspace shapes (/home/runner/work/<repo>/<repo> and /codebuild/output/<id>/src/actions-runner/_work/<repo>/<repo>).
  • GitHub-hosted path: this repository's own Claude and Codex review runs execute the changed prepare_ai_review_user.sh, run_claude_isolated.sh and env -i launch paths on ubuntu-latest (where id -un is runner and the ACL walk reduces to the previous /home/runner grant). On 5123354 the Codex Generate and Post jobs passed; Claude started, authenticated to Bedrock and ran until the pre-existing 20-turn cap (the same failure occurs on main-based runs), so credential delivery is confirmed for both.
  • CodeBuild path, scripts (job account root, EC2 standard:7.0): aws-durable-execution-sdk-js run 37539805626 checks out this branch at d901b2f and runs prepare_ai_review_user.sh (claude-review and codex-review), prepare_ai_summary_user.sh and prepare_codex_implementation_user.sh as the job account, then mirrors each workflow's env -i launch with a stub binary. All four pass: users are created, the ACL walk adds nothing (every ancestor is 755), the review users read the workspace and cannot read /root, the summary user cannot read the workspace, the implementation user can write the worktree but not .git, and the stub receives exactly the allowlisted variables (no GITHUB_*, no AWS_CONTAINER_*, no unlisted job variables).
  • CodeBuild path, real review: aws-durable-execution-sdk-js run 37542509461 calls codex-review.yml from this branch with runs-on set to the CodeBuild label. The Generate job ran on codebuild-github-actions-runner-37542509461-1, created codex-review, assumed the Bedrock role through OIDC, launched Codex through the env -i allowlist, reviewed the pull request on Amazon Bedrock and the Post job published the review. The caller is push-triggered because the ai-pr-review-runtime environment's branch policy cannot match a refs/pull/N/merge ref.

Every reusable workflow accepts an optional runs-on input (default ubuntu-latest) and threads it into the workflows it calls, so a consuming repository can run the jobs on its own runner such as a CodeBuild-hosted GitHub Actions runner. Callers that do not set the input are unchanged.
@wangyb-A
wangyb-A marked this pull request as ready for review October 5, 2026 21:45
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 5, 2026 21:46 — with GitHub Actions Active
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 5, 2026 21:59 — with GitHub Actions Error
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 5, 2026 21:59 — with GitHub Actions Error
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 5, 2026 22:09 — with GitHub Actions Active
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 5, 2026 22:09 — with GitHub Actions Active
Comment thread README.md Resolved
@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Claude AI review

Reviewed the full diff (10 shareable workflow files + README) against the base revision. The change mechanically adds an optional runs-on string input (default ubuntu-latest) to every workflow_call surface and rewires each job-level runs-on: ubuntu-latest to runs-on: ${{ inputs['runs-on'] || 'ubuntu-latest' }}, matching the existing ||-fallback idiom already used for other optional inputs (environment-name, no-pr-label, etc.) so directly-triggered forms (issue_comment, pull_request_target, workflow_dispatch, …) keep working where the inputs context is empty.

Verified completeness and consistency:

  • All 19 pre-existing runs-on: ubuntu-latest job fields were converted (counted and matched against the files: ai-pr-review-address.yml ×2, ai-pr-review.yml ×2, ai-work-item-resolver.yml ×1, claude-review.yml ×2, codex-issue-worker.yml ×2, codex-review.yml ×2, issue-triage.yml ×2, notify.yml ×5, stale-issue-closer.yml ×1); none were missed.
  • All 6 cross-workflow uses:/with: call sites that needed to forward the value (ai-pr-review → claude-review/codex-review; ai-pr-review-address → ai-work-item-resolver/codex-issue-worker; ai-issue-implementation → ai-work-item-resolver/codex-issue-worker) correctly add runs-on: ${{ inputs['runs-on'] || 'ubuntu-latest' }} to the with: block (jobs that uses: a reusable workflow cannot carry their own runs-on: field, so this is the only valid mechanism).
  • inputs['runs-on'] bracket syntax is required and used consistently for the hyphenated key, matching the convention for other hyphenated inputs in this repo.
  • ai-issue-implementation.yml's own jobs have no direct runs-on: fields (they only call reusable workflows), so no conversion was needed there, which matches the diff.
  • README's new "Runner selection" section is accurate: notify and issue-triage are top-level workflows that themselves spawn an unprivileged sudo user, while ai-pr-review/ai-pr-review-address/ai-issue-implementation transitively require root because they call claude-review/codex-review/codex-issue-worker, which do. The code fence is well-formed.
  • Added/removed line counts reconcile exactly with pr.json (94 additions / 19 deletions / 11 files), so no unrelated changes are hidden in the diff.

No functional, typing, or lifecycle defects were found; this PR does not touch execution-history matching, placeholder/variable substitution, async/polling logic, or OpenTelemetry semantics.

Residual test risk: this is a declarative GitHub Actions change with no in-repo harness to execute workflows; verification here is limited to YAML validity and the grep check noted in the PR description. The actual runtime behavior (a caller passing a non-default runs-on value and successfully running every downstream job on that runner, including the privileged/sudo steps) will only be exercised when a consumer repository bumps its pinned SHA and supplies the input on a self-hosted/CodeBuild runner, as the author themselves acknowledges.

Reviewed commit 402841a0fe2eeaae62686912eab9a7fbc84a85a6. Workflow run

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 5, 2026 23:15 — with GitHub Actions Active
@zhongkechen
zhongkechen had a problem deploying to ai-pr-review-runtime October 5, 2026 23:15 — with GitHub Actions Failure
Comment thread README.md Resolved
@github-actions

This comment has been minimized.

The model workflows drop privileges with sudo and wrote the invoking
account's name as the literal `runner`: in sudoers `Defaults:runner
env_keep` rules, in `chown runner:<user>` / `install -o runner` calls,
and in a traversal ACL on /home/runner. GitHub-hosted runners match all
three; CodeBuild-hosted runners run as root with no `runner` account, no
/home/runner, and a workspace under /codebuild, so the setup step failed
before the model ran, and a sudoers rule that did not match the invoker
silently dropped the AWS credentials across sudo.

Resolve the account with `id -un` / `id -gn` at runtime, and replace the
/home/runner ACL with a top-down walk over the workspace's ancestors
that grants execute-only traversal exactly where the unprivileged user
is blocked. On GitHub-hosted runners this reproduces the previous
behaviour; on CodeBuild it is a no-op because /codebuild is
world-traversable.

Document the actual runner requirements and the invariant that
AWS_CONTAINER_CREDENTIALS_RELATIVE_URI must never be added to env_keep,
since sudo's environment reset is what keeps the CodeBuild project role
out of the model's reach.
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 6, 2026 00:59 — with GitHub Actions Failure
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 6, 2026 00:59 — with GitHub Actions Active
Comment thread .github/workflows/codex-review.yml Resolved
@github-actions

This comment has been minimized.

The model jobs preserved their Bedrock credentials across sudo with
`Defaults:<user> env_keep += ...` rules. That only restricts the
environment if the runner image's sudoers still has `env_reset` on and
no broader `env_keep`, which is the image's choice, not ours; with the
new `runs-on` input any image can be selected, and one that disables
the reset would hand the whole job environment (OIDC request token,
CodeBuild credential URI, GH_TOKEN) to the model process.

Start every model process with `sudo -u <user> -- env -i` and an
explicit allowlist instead, as codex-issue-worker already does. The
allowlist is the same set the env_keep rules named plus the variables
sudo's reset used to keep (HOME, PATH, LANG, USER, LOGNAME, SHELL), so
the model sees the same environment on GitHub-hosted runners and is
independent of sudo policy everywhere else. AWS_EC2_METADATA_DISABLED
is set so the SDK never falls back to instance or container metadata.

With that, the four sudoers.d files, their visudo checks, and the
"never add X to env_keep" caveat are gone; the preflight that checked
whether sudo preserved the credentials now checks the job environment
directly. In run_claude_isolated.sh unset optional variables
(AWS_BEARER_TOKEN_BEDROCK, CLAUDE_CODE_*) are skipped rather than passed
as empty strings.
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 6, 2026 01:40 — with GitHub Actions Failure
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 6, 2026 01:40 — with GitHub Actions Active
Comment thread README.md Resolved
Comment thread README.md Resolved
@github-actions

This comment has been minimized.

…nuation

Two gaps surfaced by review of the runs-on input:

- The caller templates in docs/ai-pr-review-address.md did not carry runs-on,
  so a repository copying them would leave the workflow_run continuation (and
  the intake/address jobs) on ubuntu-latest while the rest of its workflows
  moved to a hosted runner. All three calls now show the input, with a note
  that the value must be identical across them.

- The prepare scripts create the unprivileged user and its home
  unconditionally and never clean up, which only works on a runner that
  discards its environment after each job. The README now states that
  requirement explicitly instead of implying any sudo-capable Linux runner
  will do.
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime October 6, 2026 22:27 — with GitHub Actions Active
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 6, 2026 22:27 — with GitHub Actions Failure
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings. Residual risk remains around custom-runner layouts because the new runner propagation and environment-isolation paths lack dedicated automated coverage.

Reviewed commit d901b2f1a50731f46bb791b7cae6cbe7f56ae917. Workflow run

@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 9, 2026 17:10 — with GitHub Actions Failure
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime October 9, 2026 18:32 — with GitHub Actions Failure
@hln33

hln33 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

The failing Claude review / Generate Claude review check (run 37400320408) is a false positive in claude-code-action, not a problem with this PR.

What happened

  1. The review ran with --max-turns 20 (claude-review.yml L217 on main; the job is pull_request_target, so it used main's workflow and wrapper, not this branch's).
  2. Claude completed the review: subtype: "success", is_error: false, num_turns: 51. The CLI never reported error_max_turns, so its own round cap was not hit.
  3. The action then threw Claude reported a successful result after 51 turns, exceeding the configured maximum of 20 (base-action/src/run-claude-sdk.ts L241–250, pinned v1.0.236). That skipped "Save Claude review" and "Upload Claude review", so the finished review was discarded.

Why

--max-turns caps API rounds. num_turns counts user-role messages: the prompt plus one per tool_result. The review prompt is restricted to Read/Grep/Glob and issues several tool calls per round, so num_turns grows 2–3x faster than rounds. A larger diff like this one means more reads, so it crosses 20 more easily. Upstream tracks this as anthropics/claude-code-action#1795 (open, bug p2); the reporter there saw 15 of 16 failures across 100 runs be this exact mismatch.

This PR's changes to claude-review.yml and run_claude_isolated.sh (the runs-on input and the env -i allowlist) do not touch turn handling; the wrapper still forwards "$@" unchanged.

Suggested fix (separate PR against main, since this check runs main's workflow)

claude-review.yml is the only workflow using claude-code-action and the only one with --max-turns; the Codex workflows have no equivalent check. Raise --max-turns there to ~60 with a comment pointing at #1795, and revert once upstream compares rounds instead of num_turns. timeout-minutes: 30 remains the hard budget. Re-running this job may also pass if Claude batches fewer tool calls, but that is nondeterministic.

@hln33

hln33 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Tracking the --max-turns false positive in #70.

@hln33
hln33 merged commit 288e431 into main Oct 9, 2026
22 of 25 checks passed
@hln33
hln33 deleted the ci/runs-on-input branch October 9, 2026 18:56
zhongkechen added a commit to aws/aws-durable-execution-sdk-js that referenced this pull request Oct 14, 2026
## Summary

The AI PR review, issue triage, Slack notification, and stale-issue
closer workflows still run on the shared `ubuntu-latest` pool. Pass the
JavaScript testing account's CodeBuild runner label to these four
reusable workflow callers, completing the shared CI portion of #937.

Pin all four callers to `8ea82196db65b5b0dd67f0b72bb03fa2fa9cf65b`,
which is four commits after `288e4317948ca778ebf01acca9342d8b898a1dc2`
(the merged commit from aws/aws-durable-execution-ci#67 that introduced
the `runs-on` input). The pinned revision retains that input in all four
reusable workflows and both nested AI review workflows, and includes
subsequent dependency, model-default, and Claude turn-limit fixes.

Merged `main` into the branch, preserving its updated stale-issue
schedule and OTel workflow. OTel's CodeBuild migration, newer
conformance orchestrator pin, and tested Node 22 setup helper are
already on `main`; the resulting PR diff contains only the four CI
workflow callers.

## Validation

- Parsed the four callers at `6f6b7b86` and all six referenced shared
workflows at `8ea82196` with duplicate-key detection; verified
supplied/required inputs, explicit secrets, and CodeBuild runner
propagation through both nested AI review workflows.
- `actionlint` 1.7.12 passed for all four modified callers.
- Parsed all 16 workflow YAML files and checked for duplicate keys.
- Validated supplied inputs, required inputs, explicit secrets, and
permissions against the exact pinned reusable workflow contracts,
including nested AI review and OTel workflows.
- Node 22 setup tests: all three offline fixtures passed. Shell syntax
and `git diff --check` passed.
- Previous CodeBuild runtime verification: [reusable review
run](https://github.com/aws/aws-durable-execution-sdk-js/actions/runs/37542509461)
and [model-user scripts running as
root](https://github.com/aws/aws-durable-execution-sdk-js/actions/runs/37539805626).
These four callers use `main`'s workflow definitions, so their first
event after merge exercises the updated callers.

No linked issue: follow-up to #937.

---------

Co-authored-by: Alex Wang <[email protected]>
Co-authored-by: Frank Chen <[email protected]>
Co-authored-by: Harry Nguyen <[email protected]>

This branch had an error being deployed

1 failed deployment
ai-pr-review-runtime — d901b2f1 Deployed Oct 9, 2026 by wangyb-A via Claude review / Generate Claude review #188
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