Run PR bots as a bare process in CI; remove the GHCR release pipeline (#2866) - #2880
Run PR bots as a bare process in CI; remove the GHCR release pipeline (#2866)#2880jwbron wants to merge 2 commits into
Conversation
…#2866) Phase A of #2866: PR-triggered bots (review, contract-verify, agent-mode design review, review-feedback, check-fixer/autofix, conflict resolver, doc-updater) now run the Agent SDK directly in the GitHub Actions runner instead of spinning up Docker networks + a gateway sidecar + a sandbox container inside the already-ephemeral runner. - action/: action.yml + entrypoint.sh install the Claude CLI + egg_agent and run `python3 -m egg_agent` as a bare process. Deleted gha_exec.py and generate-config.sh (the Docker/gateway orchestration entry points). - Auth: each bot runs as a single GitHub App identity (reviewer App for the review bot, bot App for fixers/docs), minted by the workflow via create-github-app-token and passed to the action as github-token. App-token events retrigger workflows, so the review->fix->re-review chain keeps working with no workflow_run rewiring, and reviewer approve/request-changes is preserved. The capability boundary moves from the gateway allowlist to the App installation's permission scope. - action/bin/gh: slim, gateway-free `gh` shim placed ahead of the real gh on PATH. It re-provides the egg-automated-review marker + verdict handling the workflows depend on (the 1309-line gateway-coupled sandbox/scripts/gh wrapper is bypassed in this path; it stays for k3s pods). - Removed release-images.yml (sole consumer was the action path); cleaned create-release.sh / RELEASING.md / kubernetes-migration.md image sections. - Docs: action/README.md, deployment.md, STRUCTURE.md updated for the bare-process + App-identity model and the security-boundary shift. EGG_AGENT_ROLE role-based write restrictions (tool_interceptor.py) carry over unchanged. k3s pipeline agents keep the gateway. Phase B (delete the orchestrator/sandbox Docker runtime, collapse EGG_RUNTIME, remove the back-compat shims + compose) is a follow-up.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agent-mode design: aligned — approving.
Scoped to agent-mode design alignment only (general correctness/security is the base bot's job). This PR is well-aligned and actually improves alignment in two notable ways:
- Removes a
claude --printinvocation. The deletedsandbox/egg_lib/gha_exec.pybuilt aclaude --printcommand (suppressed with# noqa: EGG100) — explicitly not an approved invocation path perdocs/guides/agent-mode-design.md("Use the Agent SDK, not claude --print"). The newaction/entrypoint.shrunspython3 -m egg_agent, which routes throughegg_agent.client.run_agent()(the Agent SDK). This swaps a flagged anti-pattern for the sanctioned path. action/bin/ghis a legitimate shim, not a post-processing pipeline. It deterministically injects the<!-- egg-automated-review bot= commit= verdict= -->HTML marker, which the review→fix→re-review workflow chain parses — a genuine machine-readable signal for automation (the guide's sanctioned exception), not human-facing structured output. Everything exceptgh pr reviewpasses straight through, and it replaces a 1309-line gateway-coupled wrapper with a ~160-line shim, so the agent uses the realghfreely. No agent output is parsed to re-take an action the agent could take itself.
Other checks: prompts are unchanged (no new pre-fetching), the model default stays an alias (opus, no EGG201 pin), and the agent invocation goes through the SDK (no direct Anthropic API calls / EGG200 concern).
On the security-boundary shift (gateway → App installation token scope): from a design-philosophy standpoint this is handled correctly. The hard, security-critical boundary (no gh pr merge, repo-scoped writes) remains infrastructure-enforced via the App scope + branch protection on main. The one degraded item — the branch-prefix push restriction now being prompt-level ("scope fixers to the PR head branch in the prompt") — is exactly the kind of mitigation the guide warns can be ignored, but the PR documents it as an explicitly accepted risk rather than presenting it as a control, which is the honest path the guide endorses. Whether that residual risk is acceptable is a security-architecture call for the base reviewer/human, not an agent-mode design defect.
No agent-mode design concerns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: PR #2880 — Run PR bots as a bare process in CI; remove the GHCR release pipeline
Verdict: Request changes. One blocking defect makes the new bare-process path non-functional end-to-end, and a test-coverage gap is exactly why it passed CI green. Details below, followed by non-blocking security/operational notes and minor nits. The non-bare-process wiring (token plumbing, removed-input cleanup, gh shim logic) is otherwise sound.
🔴 Blocking — the dependency-install step fails; the agent never runs
action/action.yml:83:
python3 -m pip install --disable-pip-version-check "${{ github.action_path }}/../shared"
egg-shared declares a hard dependency on egg-contracts (shared/pyproject.toml:6), which is a local-only package — not published to PyPI, and not in the install set here. pip resolves it from the index and fails. Reproduced:
$ pip install --dry-run ./shared
Collecting anthropic<1.0,>=0.50 ... # resolves fine — network/PyPI are up
Collecting markdownify>=0.13.1 ... # resolves fine
ERROR: Could not find a version that satisfies the requirement egg-contracts (from egg-shared) (from versions: none)
ERROR: No matching distribution found for egg-contracts
That is a non-zero exit in the composite action's "Install Python dependencies" step, so the "Run egg" step never executes. Every invocation of this action — every PR-bot review via jwbron/egg/action@main (reusable-review.yml:507) — dies before the agent starts. anthropic/markdownify resolving in the same run rules out a network problem; it is specifically egg-contracts.
It does not get better even if that resolution were somehow satisfied:
egg-contractsis not standalone pip-installable as written. Its modules live flat inshared/egg_contracts/andtool.hatch.build.targets.wheelhas no package mapping, sopip install ./shared/egg_contractsfails with hatchling's "Unable to determine which files to ship inside the wheel … no directory that matches the name of your project (egg_contracts)."egg-sharedexcludesegg_contracts*from[tool.setuptools.packages.find].include(shared/pyproject.toml:13), sopip install ./sharedwould never bundle it anyway.- The agent actually needs it at runtime:
egg_agent→egg_restrictions.patterns→from egg_contracts.agent_roles import AgentRole(shared/egg_restrictions/patterns.py:22). The review workflow setsEGG_AGENT_ROLE(reusable-review.yml:521), so the role-enforcement path is live and exercises this import.
Root cause: the sandbox never pip-installs shared/ — it puts it on PYTHONPATH precisely to avoid this (sandbox/Dockerfile:312-314: "Make sandbox and shared modules importable via PYTHONPATH … simpler than pip install and doesn't require pyproject.toml"). The action's comment claims it "mirrors the sandbox image install," but it diverges exactly where the sandbox is deliberately sidestepping pyproject packaging.
Suggested fix (mirror the sandbox): drop pip install ./shared, install only the third-party deps the agent imports, and put shared/ on PYTHONPATH. e.g. in action.yml:
python3 -m pip install --disable-pip-version-check \
'claude-agent-sdk>=0.1.65,<0.2' 'anthropic>=0.50,<1.0' \
'httpx>=0.25.0' 'markdownify>=0.13.1' 'pyyaml>=6.0' 'pydantic>=2.0.0'
echo "PYTHONPATH=${{ github.action_path }}/../shared${PYTHONPATH:+:$PYTHONPATH}" >> "$GITHUB_ENV"
(pydantic is egg_contracts's only dep.) This makes egg_agent, egg_restrictions, egg_logging, and egg_contracts all importable, matching the proven sandbox model. If you'd rather keep pip-install, you must both fix egg-contracts's hatchling packaging and install it before shared — strictly more work than PYTHONPATH.
🟠 Should fix — CI no longer exercises this path, which is why the above is green
test-action.yml replaced the config-generation job with gh-shim unit tests only. Nothing runs the dependency install or python3 -m egg_agent, so the install failure is invisible to CI. Please add a job that runs the action's install steps plus at least a smoke invocation (python3 -m egg_agent --help, or a one-shot trivial prompt) so this class of end-to-end regression is caught. A pure-Bash gh-shim test cannot catch a broken runtime.
Non-blocking — security / operational notes to confirm
- Loss of the branch-prefix push restriction. The gateway previously constrained
git pushtoegg/-prefixed branches (gateway/policy.py). In the bare-process path the only boundary is the App installation scope — correctly documented inaction.yml:18-23andbin/gh:13-15. Please confirm the bot/reviewer Apps'contentspermission combined with branch protection actually prevents pushes tomain/protected branches: the prefix guard is gone, and prompt-injected content in a reviewed PR could otherwise drive a push anywhere the token allows. Bounded by token scope — but the scope is now the entire boundary. - Role enforcement silently fails open under this packaging. Independently of the install failure: with
egg_contractsabsent, theegg_restrictionsimports intool_interceptor.py:66,114hit theirexcept ImportError: return Nonefail-open. Harmless for roleless bots, but the review path setsEGG_AGENT_ROLE, so file-scope restrictions would be silently unenforced for any role-scoped run. The PYTHONPATH fix above resolves this too. - Checkpoint/transcript capture dropped. This path has none of the checkpoint persistence the sandbox provided. Fine if intended for one-shot bots, but flagging the loss of post-hoc debuggability.
Minor / nits
bin/ghvalue-flag parsing (--body/--body-file/--repo, lines 54-61) doesi=$((i+1)); var="${ARGS[$i]}"with no bounds check; a trailing value-expecting flag with nothing after it would tripset -u("unbound variable"). Not reachable through normal agent calls, but a guarded read would be more robust.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed — fixes ready, push currently blockedAll three actionable items are fixed and committed locally as 🔴 Blocking — dependency-install step fails; the agent never runsfixed-in-PR (commit 16aa4e7, pending push). Confirmed the root cause: 🟠 Should fix — CI no longer exercises this pathfixed-in-PR (commit 16aa4e7, pending push). Added a Non-blocking — role enforcement silently fails open under the old packagingfixed-in-PR (commit 16aa4e7, pending push). Resolved by the Minor / nit —
|
Recovery patch (commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…app-*/image-tag inputs, drop generate-config.sh tests)
Conflict Resolution SummaryResolved merge conflicts with
Please review:
Verification:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed delta since 41e320e8: only one new commit (e339248d), a merge of origin/main with conflict resolutions that (a) preserved the PR's single-GitHub-App auth model by dropping the bot-app-* / image-tag / bot-branch-prefix inputs main re-introduced, and (b) dropped main's generate-config.sh tests against a deleted file.
The only PR-authored file change in the delta is .github/workflows/test-integration.yml — disk-pressure mitigation (jlumbroso/free-disk-space pinned by SHA, docker-images: false + tool-cache: false to preserve make build cache + Python toolchain) and the switch from k3s's bundled metrics-server to the hostNetwork variant. Pure CI infra hygiene; no agent-mode surface touched.
No agent-mode design concerns. Prior approval stands.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: merge resolution is clean, but the previously-blocking install bug is still on the branch
Delta since 41e320e8
Only one PR-authored commit landed: e339248d (merge of origin/main). The conflict resolutions are correct end-to-end — I diffed each conflict file against the merge base + each parent:
.github/workflows/reusable-{review,autofix,check-fixer,conflict-resolve}.yml,on-push-doc-updater.yml,on-review-feedback.yml: PR-side drop ofbot-app-{id,private-key,installation-id}+bot-branch-prefix(replaced withgithub-token) is preserved, AND main's drop ofcheckpoint-repois preserved. Verified no stragglers via grep across all seven workflows.action/action.yml: PR-side drop ofimage-tag+INPUT_IMAGE_TAGenv preserved; main's drop ofcheckpoint-repo+INPUT_CHECKPOINT_REPOpreserved..github/workflows/test-action.yml: PR's newgh-shimjob retained; main's threegenerate-config.shtest steps correctly dropped (the script is deleted on this branch).action/generate-config.sh: modify/delete resolved as delete ✔.
.github/workflows/test-integration.yml got new content inherited from main (disk-pressure mitigation via jlumbroso/free-disk-space pinned by SHA, hostNetwork metrics-server). Pure CI hygiene, no agent-mode surface touched.
🔴 BLOCKING — install failure from the previous review is still present
The previous-review reply (commit 16aa4e7) claimed the install bug was fixed but could not be pushed due to a gateway outage. 16aa4e7 is not in the PR. The current head e339248d ships exactly the broken install I flagged before:
# action/action.yml, "Install Python dependencies" step
python3 -m pip install --disable-pip-version-check \
'claude-agent-sdk>=0.1.65,<0.2' pyyaml
python3 -m pip install --disable-pip-version-check "${{ github.action_path }}/../shared"The second line cannot succeed:
shared/pyproject.tomldeclaresdependencies = [..., "egg-contracts"]— a bare name, no PEP 508 direct reference.[tool.setuptools.packages.find].includein the same file excludesegg_contracts*fromegg-shared's wheel.shared/egg_contracts/pyproject.tomlis a separateegg-contractsdistribution that is local-only (not on PyPI; the monorepo wires the two together via[tool.uv]workspace in the top-levelpyproject.toml, whichpip installdoes not consult).- The sandbox image sidesteps this by setting
PYTHONPATH=/opt/egg-runtime/shared(sandbox/Dockerfile:314) and not pip-installingegg-sharedat all.
Concrete consequence: every PR-triggered bot run will fail at the action's "Install Python dependencies" step with Could not find a version that satisfies the requirement egg-contracts. The agent never starts. This is non-functional end-to-end — request-changes.
The fix in the unpushed 16aa4e7 is correct (and I endorsed it in the previous review): drop pip install ./shared, install the third-party deps egg_agent imports (anthropic, httpx, markdownify, pyyaml, pydantic alongside claude-agent-sdk), and put shared/ on PYTHONPATH via $GITHUB_ENV. Please re-apply that commit to the PR head.
🟠 Should fix — CI still doesn't exercise this path
.github/workflows/test-action.yml at e339248d has the gh-shim, shellcheck, prompt-builders, and integration-test jobs. There is no job that runs the action's "Install Python dependencies" step or invokes python3 -m egg_agent (grep -nE "(pip install|python3 -m egg|runtime-deps|claude-agent)" → no matches). This is exactly why the install failure isn't being caught by the PR's own checks. The runtime-deps job in the unpushed 16aa4e7 patch is the right shape — please include it.
While EGG_AGENT_ROLE is set on every review/fixer workflow, the broken pip install ./shared means egg_contracts never imports, so egg_restrictions/tool_interceptor.py:{66,114}'s except ImportError: return None fail-open silently kicks in — role-scoped file restrictions become no-ops. This is operator-facing misconfiguration that produces no signal (a "blocking" pattern per the review rules), but the root cause is the install bug above, not a separate restriction-side bug; the install fix resolves it.
🟡 Minor (nit) — bin/gh value-flag parsing still trips set -u
Still unfixed at action/bin/gh:54-61:
--body | -b)
i=$((i + 1)); body="${ARGS[$i]}" ;;
--body-file | -F)
i=$((i + 1)); body_file="${ARGS[$i]}" ;;
--repo | -R)
i=$((i + 1)); repo="${ARGS[$i]}" ;;A trailing --body / --body-file / --repo with nothing after it reads past the end of ARGS and aborts under set -u. Not reachable through normal agent calls. The take_value helper in 16aa4e7 (bounds-checks i+1, uses printf -v so the index advance persists in the loop) is the right fix and can come along for the ride.
Net
The merge resolution is good and adds no new concerns. The blocking issue is the unpushed-fix gap, not anything introduced by the merge itself. The patch already exists in 16aa4e7; it just needs to land on the PR head. Once that's pushed, this becomes an approve.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Review feedback — fixes ready locally, push is still blocked by the same gateway outageRe-applied the prescribed fix on this run as commit
Per-item disposition: 🔴 Blocking — dependency-install step fails; the agent never runsfixed-in-PR (commit 🟠 Should fix — CI no longer exercises this pathfixed-in-PR (commit Non-blocking — role enforcement silently fails open under the old packagingfixed-in-PR (commit 🟡 Minor (nit) —
|
|
egg feedback addressed. View run logs 8 previous review(s) hidden. |
1 participant
Phase A of #2866. PR-triggered bots now run the Agent SDK directly in the GitHub Actions runner as a bare process, instead of recreating — inside an already-ephemeral, already-credential-scoped runner — the Docker networks + gateway sidecar + sandbox container that k3s needs for untrusted, long-running agents.
What changed
action/action.yml,action/entrypoint.sh): install the Claude CLI +egg_agent, then runpython3 -m egg_agentas a bare process against the checked-out repo. Deletedsandbox/egg_lib/gha_exec.pyandaction/generate-config.sh(the Docker/gateway orchestration entry points) and the staletest_gha_exec.py.create-github-app-tokenand passes it to the action asgithub-token(reviewer App for the review bot, bot App for fixers/doc-updater). The agent runs as that one identity; the App installation's permission scope is the capability boundary. Chosen over the workflowGITHUB_TOKENbecause App-token events retrigger workflows (the special-casedGITHUB_TOKENdoes not), so the review→fix→re-review chain keeps working with noworkflow_runrewiring, and reviewer approve/request-changes is preserved.action/bin/ghshim. The sandboxed agents used a 1309-line gateway-coupledghwrapper (sandbox/scripts/gh) that injected the load-bearing<!-- egg-automated-review bot= commit= verdict= -->marker. Bare-process bots call the realgh, so this slim, gateway-free shim re-provides only that behavior (interceptsgh pr review, passes everything else through). The full wrapper stays for k3s pods.release-images.yml) — verified sole consumer was the action path; k3s builds locally viamake build+make k3s-import. Cleanedcreate-release.sh,RELEASING.md,kubernetes-migration.md.action/README.md,docs/guides/deployment.md,docs/development/STRUCTURE.md.Security boundary
Removing the gateway moves the boundary from a runtime operation allowlist to the App installation permission scope:
gh pr merge/repo delete— keep these off the bot/reviewer App; rely on branch protection onmainas the merge backstop.gateway/policy.py) is lost — an App withcontents: writecan push to any branch. Mitigated by scoping fixers to the PR head branch in the prompt + branch protection.mainprompt-build protection stays, but an injected agent is now bounded only by token scope. Accepted trade-off for PR bots (k3s agents keep the gateway).EGG_AGENT_ROLErole-based write restrictions (tool_interceptor.py) carry over unchanged.Verification
make test→ 17022 passed, 29 skipped, 0 failedmake lint→ green;actionlint→ clean on all workflowsghshim smoke-tested (passthrough + marker injection), covered bytest-action.ymlOut of scope (follow-up)
Phase B — delete the orchestrator/sandbox Docker runtime, collapse the
EGG_RUNTIMEdocker branches, remove thedocker_client/container_spawnershims + compose remnants. Tracked in #2866.