ci: run update_pins.py weekly and open a PR - #32
Conversation
7c989a6 to
307c8b3
Compare
dtump
left a comment
There was a problem hiding this comment.
NEEDS WORK · 0 block, 4 fix, 1 later
I agree with the functionality and with most of the design calls (single long-lived branch, bootstrapping from pins/uv.env, dispatch inputs through env). I walked the run path step by step rather than just reading it — notes on what I verified are at the bottom. Four things to fix before merge.
Fix before merge
1. The OpenSpec change is missing, and this repo pre-registered it
openspec/changes/automate-version-pins/tasks.md:51, under "## 8. Deferred to dedicated follow-up changes":
- 8.2 Weekly automated refresh (CI/cron) opening a PR, so freshness doesn't depend on someone remembering to run the script — the fragments + script were built to support it
That is this PR, verbatim. It ships with no change directory, no delta, and the box still unticked. CONTRIBUTING.md:11 asks for a change proposal for non-trivial behaviour changes, and the plan for this specific work named a dedicated follow-up change.
I did consider whether CI plumbing is exempt — none of the eleven prior commits touching .github/workflows/ carried OpenSpec artifacts, and that precedent is fine for linting and action bumps. This is different: it adds a second, unattended actor to the version-pin-refresh capability plus a new operator-configured secret, and every requirement in that capability is currently phrased "the operator runs the refresh script".
What I'd like: one ADDED Requirement: Scheduled unattended refresh with scenarios for the changed-pins and unchanged-pins paths, and 8.2 ticked pointing at it. Sequencing note — openspec/specs/version-pin-refresh/ does not exist yet (automate-version-pins is 26/28 tasks and unarchived), so a standalone change has no synced capability to delta against. Either archive automate-version-pins first and add the follow-up change on top, or fold the requirement into that change and tick 8.2 there. Your call which, but the requirement needs to exist somewhere before the archive/sync happens, otherwise the synced spec will describe pin refresh as a manual, operator-only flow on the day the automation goes live.
2. The fallback token path is the one that will actually run, and it produces an unmergeable PR
.github/workflows/pins-updater.yml:111 and README.md:280 describe the GITHUB_TOKEN fallback as leaving CI idle. Two things sharpen that:
GET /repos/schubergphilis/claude-docker/actions/secretsreturns{"total_count":0}—PINS_UPDATER_TOKENdoes not exist, so the fallback is what ships on merge.- The
main protectionruleset requires theValidateandDocker build (validate, no push)contexts. On a PR opened withGITHUB_TOKENthose never report at all, so the merge button is blocked on "Expected — waiting for status to be reported".
So the weekly PR is not merely uninformative, it is unmergeable until a human pushes an empty commit or closes and reopens it. Please create the secret before merging (a fine-grained PAT scoped to this repo with contents: write + pull_requests: write, rather than a classic PAT with full repo), and reword the README note from "won't run" to "can't be merged until".
3. The link in the generated PR body 404s
.github/workflows/pins-updater.yml:132 emits a repo-relative markdown link. GitHub only rewrites relative links in files rendered from the repo, not in issue or PR bodies — POST /markdown with mode=gfm and context set leaves it untouched:
see <a href=".github/workflows/pins-updater.yml">x</a> here
On a PR page the browser resolves that against /<org>/<repo>/pull/, giving https://github.com/schubergphilis/claude-docker/pull/.github/workflows/pins-updater.yml — a 404 in every weekly PR body. Use ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/blob/main/.github/workflows/pins-updater.yml. (The README.md:278 link is fine — that one is rendered from a repo file and does get rewritten.)
4. The uv bootstrap step buys nothing
.github/workflows/pins-updater.yml:53-66 re-implements Dockerfile:159-171 — curl, sha256sum -c, tar, install. The comment justifies it against a third-party setup action, but that isn't the alternative on the table: update_pins.py declares requires-python = ">=3.11" and dependencies = [], the runner ships python3.12, and ci.yml:36 and ci.yml:105 already invoke the very same script as python3 update_pins.py. Dropping the step removes ten lines, removes the only step that can fail before any work happens, and removes a hard-coded uv-x86_64-unknown-linux-gnu/ archive path that now has to be kept in step with the Dockerfile's $(uname -m)-derived one.
If you want the uv bootstrap anyway for parity with the documented operator command, say that in the comment instead — the current rationale argues against an option nobody was proposing.
Later, not blocking
.github/workflows/pins-updater.yml:127 — the force-push is unconditional, so a week in which nothing new clears the soak still produces a fresh commit SHA over a byte-identical tree. The main protection ruleset sets dismiss_stale_reviews_on_push: true, so an approved-but-unmerged pins PR silently loses its approval every Monday and re-runs the required build. Cheap guard if you ever want it: fetch the remote branch and skip push + edit when HEAD^{tree} matches. Not blocking — most weeks the tree genuinely differs (claude-code alone moves often enough), and when it does, dismissing the review is the correct behaviour.
Strengths
Things I checked that hold up, so they don't get "fixed" later by someone who didn't:
installinto/usr/local/binwithout sudo works. I went looking for this one expecting a failure. The runner image build runssudo chmod -R 777 /usr/local/binininstall-nodejs.sh, so the step is fine as written.- Dispatch inputs go through
envand are never interpolated into a script body — andinputs.soak || '7'correctly covers both the schedule trigger (no inputs) and a dispatcher who clears the field. zizmor is clean, and it would have caught the other spelling. persist-credentials: falseplus an explicit push URL is the right pairing given the token may be a PAT rather than the job token, and the job-levelpermissionsblock is scoped rather than inherited.- The automated PR passes this repo's own PR gates. The commit body is 117 characters, which would trip
body-max-line-lengthunder stock config-conventional —mcvs-general-action'scommitlint.config.mjsdisables that rule, so it passes.lint-git's behind-main check passes because the branch is rebuilt frommainevery run, and the generated body is non-empty formcvs-pr-validation-action. - Failure modes are non-catastrophic.
update_pins.pyreturns non-zero only on real resolve/download/integrity failures, sopipefailmeans a bad refresh can never reach the commit step;print_reminders,ubuntu_current_digestandgo_latest_stableare all best-effort and cannot fail the job; every request carriestimeout=30; and passingGITHUB_TOKENto the script is safe because its redirect handler stripsAuthorizationwhen a redirect crosses hosts. - The rationale comments are worth keeping. The
concurrencynote, the single-branch justification, and thetee-for-both-log-and-body note all answer the question a reader would actually have.
|
Correction to my review: |
1597a78 to
2b336fb
Compare
Drop the uv bootstrap step: update_pins.py is stdlib-only with an empty dependency set and the runner's python3.12 already satisfies its requires-python, so uv had nothing to resolve. ci.yml already invokes the script as python3. Removes the only step that could fail before any work started, plus a hard-coded archive path that had to track the Dockerfile's $(uname -m)-derived one. Build the workflow link in the generated PR body from GITHUB_SERVER_URL and GITHUB_REPOSITORY. GitHub rewrites repo-relative markdown links only in files rendered from the repo, so the relative one resolved against the pull request URL and 404'd in every weekly body. State the real cost of the GITHUB_TOKEN fallback in both the workflow comment and README: main's ruleset requires the Validate and Docker build contexts, which never report on a PR opened with GITHUB_TOKEN, so the PR is unmergeable rather than merely unvalidated until a human intervenes. Name the fine-grained PAT scope PINS_UPDATER_TOKEN should carry. Add the OpenSpec change automate-version-pins deferred as task 8.2, and tick it. version-pin-refresh gains a Scheduled unattended refresh requirement covering the changed-pins and unchanged-pins paths, PR replacement, on-demand overrides, and that the run never rewrites a manual pin — without it the capability would sync describing pin refresh as operator-only on the day the automation goes live.
2b336fb to
c8c069f
Compare
|
Thanks — all four addressed in c8c069f, and the branch is rebased onto current 1. OpenSpec change. Taking the standalone route your correction unblocked: I left it unarchived deliberately. Tasks 4.5–4.7 are the secret, a live dispatch run, and the unchanged-pins path — none of which can honestly be ticked yet, and archiving would sync a requirement whose verification hasn't happened. It sits alongside 2. Token. Reworded in both places from "CI won't run" to the actual consequence — the required The secret itself I can't create — flagging it as a merge prerequisite rather than a follow-up. 3. Link. Now 4. uv bootstrap. Dropped, and you're right that the comment was arguing against an option nobody proposed. Later item. Left out, per your own reasoning — but written down in One drift I picked up while in there: the generated PR body listed the manual pins as "nodejs, go, ubuntu base". On the strengths section — thanks for writing down what you checked, particularly the Still not run live. Verified offline: |
Adds .github/workflows/pins-updater.yml: every Monday (and on demand via workflow_dispatch, with optional --soak / --block-major-bumps overrides) it runs
uv run update_pins.pyand, if any pin moved, force-pushes the bump/pins branch and opens or refreshes a single PR carrying the script's full report.One long-lived branch rather than a dated branch per run: a stale pins PR proposes versions the next refresh has already superseded, so the newest run should replace the open PR instead of stacking another one beside it.
uv is bootstrapped from pins/uv.env — the same URL and sha256 the Dockerfile installs, verified with sha256sum -c — instead of a third-party setup action, so the workflow leans on the repo's own pinned artifact and there is one less action to keep pinned.
The PR is opened with secrets.PINS_UPDATER_TOKEN when that secret exists, falling back to GITHUB_TOKEN. The fallback works but GitHub does not trigger pull_request workflows for PRs opened with GITHUB_TOKEN, so the docker build and smoke matrix that actually validate a pin bump stay idle until someone pushes an empty commit or reopens the PR. Both paths are documented in the workflow and in README.
Not run live yet. YAML parses, yamllint is clean apart from the SHA-comment spacing warning every existing workflow here shares, and all four run blocks pass bash -n. shellcheck was unavailable in the authoring environment; CI's shellcheck job scans files on disk, not YAML run blocks, so it would not cover them either.