feat: self-update wrappers on update + pre-commit and CI from prior PR - #8
Merged
Conversation
added 3 commits
May 21, 2026 19:23
- workflows/ci.yml: drop push trigger and branch filters so CI runs only on open PRs (any target branch); simplify test-linux and test-devcontainers fork-protection conditions since the push branch is now unreachable - docs/storage-layout.md: add instructions.md and AGENTS.md to the list of Codex config files synced into /scodex-config (scodex:400 copies these in addition to config.toml) - CONTRIBUTING.md: list cleanup.sh and PLAN.md in the project structure - .gitignore: exclude *.pcap captures from the worktree
…g validation - .pre-commit-config.yaml: actionlint, shellcheck, bash/zsh syntax checks, Conventional Commits validation, and AI-attribution stripping on the commit-msg stage - rename .githooks/commit-msg to .githooks/strip-ai-attribution.sh so pre-commit's own commit-msg dispatcher does not collide with the attribution-stripping script when installed under .githooks - CONTRIBUTING.md: document pre-commit install steps, including the core.hooksPath unset for clones that previously used .githooks
`sclaude update` and `scodex update` previously rebuilt the shared image but left the wrapper script itself untouched. A pre-`sagent-sandbox`-rename install would keep computing an old image hash, fail to find a tagged image, and loop on "image not found" / rebuild every invocation. `update` now: - downloads the latest release of both wrappers (sclaude + scodex) when the running script is out of date, installs them in place (using sudo if the destination is not writable), and re-execs with the new wrapper so the image rebuild runs on current logic (skip with `SAGENT_SKIP_SELF_UPDATE=1`) - adds `--pull` alongside `--no-cache` so the Ubuntu base image refreshes on every update, not just the layer cache - verifies the image tag exists after the build and prints the resulting Claude / Codex CLI version so a successful update is observable Refs BUGS.md #48, #49.
e6qu
commented
May 21, 2026
e6qu
left a comment
e6qu
left a comment
Owner
Author
There was a problem hiding this comment.
Automated review by codex exec (codex-cli 0.132.0). Summary: The self-update path needs stronger asset verification, atomic installs, explicit failure handling, and protection against partial wrapper updates/downgrades. CI also lost the trusted post-merge E2E coverage for fork contributions.
added 4 commits
May 21, 2026 19:45
Addresses the codex review on #8. Six concrete fixes to the self-update flow in both wrappers: 1. download_release_asset now also verifies the asset declares the expected SCRIPT_NAME and WRAPPER_VERSION before installing, not just a bash shebang. A wrong asset uploaded to the wrong release name no longer slips through. 2. install_wrapper_file stages the new content into a temp file in the destination directory and renames it over the old wrapper, so an interrupted install can no longer leave a truncated script. 3. New version_gt helper uses `sort -V`; self_update_wrapper now only replaces the wrapper when the latest tag is strictly newer, so a dev checkout that has been bumped past the last published release no longer downgrades itself. 4. self_update_wrapper now returns non-zero on any failed download or install. The `update` dispatch aborts before rebuilding the image when the wrapper update failed, so a stale wrapper plus a fresh image never looks like a successful update again. 5. Restructured into a two-phase flow: download both assets to staging first, only install once both downloads succeeded. A partial install (self replaced but paired failed) is surfaced explicitly with manual install instructions for the paired wrapper. 6. verify_image_built no longer falls back to "unknown" when the inner tool's --version fails. A non-runnable CLI in the rebuilt image is now a hard failure with the underlying error captured. Refs the codex review threads on PR #8.
Restores the trusted-context post-merge safety net flagged in codex review thread on PR #8. Lint jobs remain PR-only as before; the two heavy E2E jobs now run on: - pull_request from same repo (fork PRs still skipped by the head.repo.full_name guard, unchanged) - push to main Dropped `needs: lint` from the two test jobs so they can run on push events where the lint job is filtered out by the PR-only `if:` guard. The lint job still gates merges via branch protection on PRs. Refs codex review thread on PR #8 (.github/workflows/ci.yml:4).
`pull_request_target` runs in the base-repo context with secrets while checking out PR-supplied refs — a well-known exfiltration vector. Add a pygrep pre-commit hook that fails any commit introducing the token under `.github/workflows/`, and document the policy in CONTRIBUTING.md so the intent is discoverable without reading the hook list. No workflow currently uses the trigger; this is purely a guardrail to keep it that way.
`sclaude update` and `scodex update` must always work in all cases, per explicit direction. Two changes to honor that: - WRAPPER_VERSION mismatch on a downloaded release asset is now a warning, not a hard reject. Release-please cannot be relied on to bump the constant in the published asset (v2.3.0 shipped with WRAPPER_VERSION="2.2.0"), and blocking self-update on that release-time bug would defeat the point of self-update. The SCRIPT_NAME check stays strict — that's the load-bearing "wrong asset uploaded to wrong name" guard. - T10 now copies the under-test wrapper into a tmpdir and runs `update` there, so the full self-update + image-rebuild flow is exercised (instead of being skipped via SAGENT_SKIP_SELF_UPDATE). The tmpdir copy keeps the in-place wrapper replacement from clobbering the script subsequent tests share. The release-please version marker added in the previous commit will keep WRAPPER_VERSION in sync on future releases, so the warning path should rarely fire once v2.4.0+ is out.
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.
Why
After #6 merged,
sclaude update/scodex updatestill left the running wrapper script alone — so any install that predates thesclaude-sandbox→sagent-sandboximage-repo rename keeps recomputing an old hash, can't find the matching image, and loops on rebuild forever. This PR makesupdatetruly update the wrapper itself.Changes
feat: self-update on
update(10ecf72)sclaudeandscodex, install them in place (usingsudoif the destination isn't writable), and re-exec with the new wrapper so the image rebuild runs against current logic.SAGENT_SKIP_SELF_UPDATE=1disables the wrapper download.--no-cachebuilds now also pass--pullso the Ubuntu base image refreshes, not just the layer cache.claude --version/codex --versionso a successful update is observable.chore: add pre-commit hooks (
5bb0054, cherry-picked from #6 — never made it to origin before the merge).pre-commit-config.yamlwires up actionlint, shellcheck, bash/zsh syntax checks, Conventional Commits validation, and AI-attribution stripping on thecommit-msgstage..githooks/commit-msg→.githooks/strip-ai-attribution.shso pre-commit's owncommit-msgdispatcher can coexist when installed under.githooks.core.hooksPathunset for anyone migrating from the old setup.ci: scope CI to PRs and fix doc drift (
edede2e, cherry-picked from #6 — also never reached origin)docs/storage-layout.mdlistsinstructions.mdandAGENTS.mdalongsideauth.jsonandconfig.tomlin the Codex sync list.cleanup.shandPLAN.md..gitignoreexcludes*.pcap.Testing
pre-commit run --all-files(actionlint, shellcheck, bash-n, zsh-n all green)shellcheck sclaude scodex test_e2e.sh test_devcontainers.sh cleanup.shbash -n/zsh -non both wrappersI did not run a live
sclaude updatesince that requires a published release newer than the running wrapper — the round trip can be verified after this lands and the next release is cut.