fix: bump Agent Canvas after Automation releases - #240
Conversation
Dispatch a rerunnable downstream bump only after PyPI publication succeeds. Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands Do a /codereview on this PR. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
🟡 Acceptable — the release ordering and stale-run handling are thoughtful, but the claimed privilege boundary is incomplete.
[CRITICAL ISSUES]
- [
.github/workflows/bump-agent-canvas.yml, line 289] Artifact trust boundary: the fresh PAT-bearing runner accepts the downloaded patch after only checking that Git can apply it. It never independently enforces the six allowed paths or verifies the requested Automation/SDK versions. A compromised preparation runner or action can therefore replace the artifact with any valid Agent Canvas patch; this job will commit and push it usingOPENHANDS_BOT_GITHUB_PAT_PUBLIC. Re-run the path allowlist and semantic checks on the staged diff in this fresh job before committing (using NUL-safe path handling), and reject non-regular-file/mode changes. Ideally expose the PAT only after those checks in a step that does not execute target-repository code.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This adds a release-triggered, cross-repository write path backed by a public-repository PAT. The downstream PR is draft and the preparation job is unprivileged, which are good controls, but artifact contents cross into the privileged job without independent validation. That makes compromise of the preparation path capable of writing attacker-selected changes to a trusted same-repository branch in Agent Canvas.
Recommendation: Do not auto-merge. Request review from a human security/CI maintainer after the privileged job independently validates the staged patch.
VERDICT:
❌ Needs rework: The privilege split must validate data at the trust boundary before using the PAT.
KEY INSIGHT:
A fresh runner is not a security boundary if it treats an artifact produced outside that boundary as trusted code changes.
Validation performed: repository pre-commit passed; all seven embedded shell blocks passed bash -n; I reproduced that the current privileged commands accept and stage an arbitrary valid patch touching README.md.
This review was created by an AI agent (OpenHands) on behalf of the requester.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
|
||
| if [ -s /tmp/agent-canvas.patch ]; then | ||
| git apply --check /tmp/agent-canvas.patch | ||
| git apply --index /tmp/agent-canvas.patch |
There was a problem hiding this comment.
Artifact trust boundary: git apply --check proves only that the artifact is syntactically applicable; it does not prove that it contains the six changes validated in the unprivileged job. This fresh job never repeats the allowed-path check or verifies config/defaults.json against AUTOMATION_VERSION / SDK_VERSION, so a substituted artifact can modify any Agent Canvas file and then be committed and pushed with the PAT. Please validate the staged diff independently here before git commit: enforce the exact path allowlist with NUL-safe handling, reject mode/symlink changes, verify the exact requested versions, and repeat the old-version/JSON checks. Keep the PAT unavailable until those validations pass.
|
Reviewed PR #240 and posted the review: #240 (review) Verdict: Needs rework (🔴 high risk). Blocking finding: the PAT-bearing job downloads a patch and only runs Validation completed:
GitHub would not permit |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds a new bump-agent-canvas.yml workflow that opens a dependency-bump PR in OpenHands/agent-canvas after openhands-automation is published to PyPI, plus a dispatch step in pypi-release.yml to trigger it automatically on release.
Assessment
No material findings. The design is sound and the implementation is thorough.
Security
The two-job architecture provides good isolation:
- The
prepare-agent-canvas-updatejob has onlycontents: readpermissions, no secrets, and clones the public agent-canvas repo to produce a git patch artifact. - The
publish-agent-canvas-updatejob is the only job that holdsOPENHANDS_BOT_GITHUB_PAT_PUBLIC, and it receives the patch via artifact — it does not execute untrusted code.git applyonly applies file changes, so even a compromised prepare job could not achieve code execution in the privileged job. - Version strings (
$AUTOMATION_VERSION,$SDK_VERSION) are validated against a strict `^[0-9]+.[0-9]+.[0-9]+$ regex before use in PR titles, commit messages, or branch names, preventing injection.
Correctness
- Idempotency: Stale-version detection (via
sort -Vcomparison against both main and the existing branch) correctly skips no-op or downgrade scenarios. Theskipoutput gates the publish job. - Race conditions: The
expected_shahandoff between jobs, re-verified viagit ls-remotein the publish job, plus--force-with-leaseon push, handles concurrent branch updates safely. - SDK pin extraction: The
sedregex matchesopenhands-sdk==X.Y.Zandopenhands-workspace==X.Y.Zfrom PyPIrequires_dist, consistent with the==pins inpyproject.toml. The consistency check (sdk_version == workspace_version) fails safely if they diverge. - PR body preservation: The Python script that merges the human-owned
HUMAN:section from an existing PR body into the generated body correctly handles the section boundary case and falls back to preserving the entire existing body if the markers are missing or malformed. - Heredoc indentation: The PR body heredoc content is at the same YAML indentation as the
catcommand, so after block-scalar dedentation it lands at column 0 — the body will not have spurious leading whitespace.
Risk
Low. The changes are purely additive (a new workflow file and a non-breaking extension to the release workflow). The dispatch step in pypi-release.yml runs only after the release job succeeds and dispatches on main, which is correct since the workflow file will be on main by the time the next release ships. The actions/upload-artifact@v7 and actions/download-artifact@v8 versions are not referenced elsewhere in the repo but are consistent with the repo practice of tracking recent action versions.
Summary
OPENHANDS_BOT_GITHUB_PAT_PUBLICWhy
Agent Canvas pins its Automation and Agent Server SDK versions together, but SDK releases currently stop after opening the Automation dependency bump. This completes the release chain after the corresponding Automation package is actually published.
Validation
uv run pre-commit run --files .github/workflows/pypi-release.yml .github/workflows/bump-agent-canvas.ymlbash -non all seven embedded shell blocksopenhands-automation==1.1.5No release workflow was dispatched during validation.
This PR was created by an AI agent (OpenHands) on behalf of the requester.
@enyst can click here to continue refining the PR