ci: use shared Forgejo review writer #7
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/public-shared-review-writer-canary"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Installs the public shared review-writer action as the public production canary.
The generated posting workflow pins the action to immutable merge commit ac19a5f2f8009f178b572faab54d66087e4607a0, removes all fourteen retired local writer files, and removes only the stale Knip writer entry.
This installation PR cannot exercise its own pull_request_target base workflow. After merge, a separate harmless follow-up PR will validate cold and warm shared-action resolution.
Approach review: The approach looks good. This PR is a clean deletion-and-replacement migration: it removes the 14-file local
.forgejo/actions/post-review/implementation and substitutes a single URL-pinned reference to the sharedforgejo-review-writeraction. The chosen design is the correct, idiomatic way to consume a shared composite action across Forgejo repositories.The immutable 40-character commit SHA pin (
ac19a5f2…07a0) is preferable to a tag pin for security-critical CI: a mutable tag could be re-pointed, whereas the SHA is fixed for the lifetime of the action. The now-redundantactions/checkoutstep is correctly removed (the action is no longer local), whileactions/setup-nodeis retained because the remote action executes its TypeScript at runtime. Thewith:block passes all nine inputs that the deletedaction.ymldeclared, preserving the existing contract, andknip.jsondrops only the now-defunct local entry while keeping the workflow glob.No materially better alternative exists — the migration is already in its minimal, safe form.
Approach review by Approach Review 3 (OpenCode Wafer) (GLM-5.2)
Approach review: The approach is sound. Replacing the vendored composite action with the shared writer is a straightforward lift-and-shift: the nine
with:inputs match the deletedaction.ymlone-for-one, the full-URLuses:form already has precedent in this repo (https://data.forgejo.org/forgejo/{upload,download}-artifact@v4), and the action is pinned to an immutable commit rather than a movable tag — the right mitigation for a step that runs withgithub.tokenand OIDC onmain. Dropping the checkout follows correctly since nothing in the job reads the repository any more, and the removal is clean: notsconfig/package.json/workspace references survive, and the only leftover is the now-empty.forgejo/actions/**glob inknip.json, which is harmless and keeps future local actions covered.One maintenance coupling is worth a thought (inline). Separately,
timeout-minutes: 3is unchanged and its comment now omits checkout, but the job gained a cold clone of the action repository in its place — the follow-up validation PR described in the description is the natural place to confirm the budget still holds on a cold resolve.Approach review by Claude Code Opus (opus)
@ -62,3 +55,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0The consumer workflow still pins the runtime (
actions/setup-node@v6,26.5.0) that the shared action depends on. That was coherent whenpost-review.tslived in this repo, but the Node requirement is now an implementation detail offorgejo-review-writer(native TS execution needs 22.18+/24+), and nothing links the two pins: bumping the action's SHA in each installed repo can silently pair it with a Node version it no longer expects, and any future runtime change means editing every consumer workflow in lockstep.Worth considering for the shared action instead: either declare it as a JS action (
runs: using: node24) so it runs on the runner's bundled runtime, or keep it composite and let it run its ownactions/setup-nodestep. Consumers would then reduce to the singleuses:line, and the runtime requirement would travel with the pinned SHA. Not blocking for the canary — it works as written — but it is cheaper to settle before the action is installed across more repositories.This is an approach alternative with two materially different designs (JS node24 action versus composite-owned setup-node). It requires user direction before any change; awaiting that decision.
Approach review: The shared-action migration is the materially simpler maintenance model: the workflow preserves the existing input contract, pins the shared action to an immutable commit, and removes the now-redundant local implementation and Knip entry. No clearly better approach is evident.
Approach review by Codex GPT-5.6 SOL (gpt-5.6-sol)
Summary: No actionable defects found. One informational note.
I verified the migration is behavior-preserving rather than taking it on faith:
action.ymlatac19a5f2is byte-identical to the deleted.forgejo/actions/post-review/action.yml, including all nine input names and thenode "${{ github.action_path }}/post-review.ts"composite step, so everywith:key in the workflow still resolves.HEAD^:.forgejo/actions/post-review/*:post-review.ts,-capability,-comments,-diff,-envelope,-http,-pending,-pr,-receipt,-resultare identical.forgejo-post-review-common.tsdiffers only by an added explanatory comment — the pinnedAXRECIPE_URL/AXRECIPE_AUDIENCEconstants enforced bypinnedEnvironment()are unchanged, so the anti-OIDC-redirect control survives.package.jsongains only devDependencies (still"type": "module", no runtime deps), andtsconfig.jsononly widensincludefor tests. Nothing the action needs at runtime requires an install.actions/setup-node, the composite action, which runs fromgithub.action_path) needs repo contents.ac19a5f2f8009f178b572faab54d66087e4607a0is the current tip ofmainin the publicj4k-oss/forgejo-review-writerrepo and is anonymously readable, so the runner's action fetch can resolve it cold; it stays reachable frommainas later commits land../.forgejo/actions/post-review, andpr-review.yml's dispatch assertion (info.jobs[0] !== "post-review") still matches the retained job id.Code review by Claude Code Opus (opus)
@ -6,3 +6,2 @@"github-actions": {"config": [".forgejo/workflows/*.{yml,yaml}", ".forgejo/actions/**/action.{yml,yaml}"],"entry": [".forgejo/actions/post-review/post-review.ts"]"config": [".forgejo/workflows/*.{yml,yaml}", ".forgejo/actions/**/action.{yml,yaml}"]ℹ️ Info:
.forgejo/actions/**/action.{yml,yaml}now matches nothing — this PR deletes the entire.forgejo/actions/tree. It is harmless (this repo runs plainknip, not--treat-config-hints-as-errorslike the writer repo does), so CI is unaffected; worth dropping only if you don't expect to add a local action back.Fixed in
3d4280b: removed the stale local-action config glob.Summary: No correctness or security defects found; 1 low cleanup item and 1 informational note.
I verified the migration end to end against the pinned commit rather than assuming it:
code.j4k.dev/j4k-oss/forgejo-review-writeratac19a5f2f8009f178b572faab54d66087e4607a0. It is the current default-branch tip and itsaction.ymlis byte-identical to the deleted.forgejo/actions/post-review/action.yml, so all nine input names (axrecipe-url…source-generator-attempt) and thenode "${{ github.action_path }}/post-review.ts"invocation still line up with thewith:block.post-review.tsand everyforgejo-post-review-*.tsare identical except one added comment inforgejo-post-review-common.ts; onlypackage.json/tsconfig.jsondiffer, and the sharedpackage.jsonkeeps"type": "module", which is what the type-stripped ESM entry needs. The pinnedAXRECIPE_URL/AXRECIPE_AUDIENCEconstants are unchanged, sovars.AXRECIPE_URLstill has to match the compiled value or the writer fails closed.node:builtins or relative./*.ts, so dropping the checkout without adding an install step is safe;actions/setup-node@v6with an explicitnode-versionneeds no workspace either, and no other step in the job touched the repo tree.pr-review.ymlstill dispatches job idpost-review(asserted atpr-review.yml:3406), and its slot allowlist (pr-review.yml:351and repeats) matches the writer'sREVIEW_SLOTSset exactly.forgejo-review-toolsis an artifact name, not a slot, so it is correctly absent from the writer allowlist..forgejo/actions/post-reviewanywhere (no tsconfig, oxfmt, oxlint, pnpm-workspace, or docs reference), so the fourteen deletions leave nothing dangling.Code review by Claude Code Opus (opus)
@ -62,3 +55,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0ℹ️ Info: Verified the pin resolves:
ac19a5f2f8009f178b572faab54d66087e4607a0is the default-branch tip of the publicj4k-oss/forgejo-review-writerrepo and itsaction.ymlis byte-identical to the local one this PR deletes, so the ninewith:inputs below all match.Worth recording for future bumps: this job passes
github.tokenand, via workflow-levelenable-openid-connect: true, exposesACTIONS_ID_TOKEN_REQUEST_*to the action's steps. The full-SHA pin is the only thing keeping that code immutable now that it lives outside this repo — a later change to a tag or branch would silently hand review-write and OIDC-minting capability to whatever that ref points at. A side effect of the move is that the writer sources no longer pass through this repo'spnpm lint/format:check/knip; that coverage now depends entirely on the shared repo's own CI.@ -6,3 +6,2 @@"github-actions": {"config": [".forgejo/workflows/*.{yml,yaml}", ".forgejo/actions/**/action.{yml,yaml}"],"entry": [".forgejo/actions/post-review/post-review.ts"]"config": [".forgejo/workflows/*.{yml,yaml}", ".forgejo/actions/**/action.{yml,yaml}"]🟢 Low: With
.forgejo/actions/fully deleted, the second glob.forgejo/actions/**/action.{yml,yaml}now matches nothing and can be dropped alongside theentryline this PR removed, leaving"config": [".forgejo/workflows/*.{yml,yaml}"].No functional impact: I ran
knip@6against this config and it reports no configuration hint for the unmatched pattern, sopnpm knipinchecks.ymlis unaffected either way — this is just the last stale writer reference left behind.Fixed in
3d4280b: removed the stale local-action config glob.Approach looks good.
Approach looks good.
Awaiting user direction on the inline runtime-ownership alternative.
Summary: Verified the installation end-to-end; found 1 low-severity documentation issue and 1 informational note.
Verification performed against the pinned ref:
j4k-oss/forgejo-review-writeris publicly readable andac19a5f2f8009f178b572faab54d66087e4607a0exists and is the current head ofmain(repo has no tags), so the SHA pin resolves and is immutable.action.ymldeclares exactly the nine inputs the workflow passes —axrecipe-url,axrecipe-audience,forgejo-token,review-slot,review-repository,review-number,review-head-sha,source-workflow-run-id,source-generator-attempt— with no new or dropped input, and is byte-identical to the deleted localaction.yml..tssources are byte-identical to their counterparts at the pinned commit exceptforgejo-post-review-common.ts, which differs only by an added two-line comment above theAXRECIPE_URL/AXRECIPE_AUDIENCEconstants. ThepinnedEnvironmentchecks still reject anyvars.AXRECIPE_*value that does not match the compiled constants, so the OIDC-redirect guard is preserved.node:builtins and relative.tsimports) and touches no filesystem or subprocess: nonode:fs, nochild_process, noGITHUB_WORKSPACEread. The PR diff is fetched over the Forgejo API, not from a work tree. Droppingactions/checkoutis therefore safe, andactions/setup-node@v6needs no lockfile becausecacheis not set..forgejo/actions/post-reviewremain in workflows,knip.json, tsconfigs,.oxfmtrc.json,fta.json, or the hooks. The deleted actionpackage.jsonwas not a pnpm workspace member (pnpm-workspace.yamldeclares nopackages), sopnpm install --frozen-lockfileis unaffected.REVIEW_SLOTSallowlist at the pinned commit matches the five slot names the generator workflow dispatches.Code review by Claude Code Opus (opus)
@ -45,3 +45,1 @@# This job includes checkout, a cold Node setup, and Forgejo/Axrecipe API# calls. Its timeout caps scheduled execution but excludes runner queue# time; the writer independently rejects results past Axrecipe's expiry.# This job includes a cold Node setup and Forgejo/Axrecipe API calls. Its🟢 Low: This comment enumerates what the 3-minute budget has to cover, and the edit drops
checkoutwithout adding the step that replaced it. The job now fetches and resolves a remote action fromcode.j4k.devbefore the composite step runs — a network cost that did not exist when the action came from the workspace, and the one the PR body itself flags for cold/warm validation. As written the comment understates the budget's contents for the next person tuningtimeout-minutes.Suggested wording:
This job includes a cold Node setup, a cold shared-action fetch, and Forgejo/Axrecipe API calls.Fixed in
84deaca: documented the cold shared-action fetch in the timeout budget.@ -62,3 +55,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0ℹ️ Info: No action needed — recording what I verified about this pin, since it moves privileged code (task token plus OIDC minting) out of the repo.
The commit resolves, is the current head of
main, and the repo carries no tags. A commit SHA cannot be repointed the way a tag can, so the pin is sound; the only way it stops resolving is a history rewrite that GCs the object, which fails the job closed rather than silently running different code. The action'saction.ymland all twelve TypeScript modules at this SHA are byte-identical to the files deleted here, apart from a two-line explanatory comment inforgejo-post-review-common.ts, so this installation is behaviour-preserving.Summary: No actionable issues found.
What I checked on the changed files:
https://code.j4k.dev/j4k-oss/forgejo-review-writerat the pinned commitac19a5f2f8009f178b572faab54d66087e4607a0and diffed all fourteen files against the deleted local copies (git show 2494766^:...). Every runtime file is byte-identical, includingaction.yml. The only differences are a comment above theAXRECIPE_URL/AXRECIPE_AUDIENCEconstants, the action repo's ownpackage.jsonmetadata/devDeps, andtsconfig.jsonaddingtest/**/*.tstoinclude— none reachable at run time.with:keys inpr-review-post.ymlmatch the sharedaction.ymlinputs exactly; no input added, renamed, or dropped.vars.AXRECIPE_URL/AXRECIPE_AUDIENCEagainst compiled-in constants so a repository-scoped Actions variable could not redirect a live OIDC JWT. The shared action at this commit keepspinnedEnvironment("AXRECIPE_URL", AXRECIPE_URL)with the samehttps://recipe.axkit.devconstants, so passingvars.*remains safe. The action is pinned to a 40-hex immutable commit rather than a mutable tag.actions/checkout. The writer reads only the Forgejo and Axrecipe APIs — it never touched the workspace, andgithub.action_pathresolves inside the runner's action download for a remote action.observe-review-writersinpr-review.ymlis an existing checkout-freesetup-nodejob in this same pipeline, so the pattern is already exercised here.actions/setup-node@v6'spackage-manager-cachedefault only auto-enables for an npmpackageManagerfield, so the now-absentpackage.json/lockfile causes no cache lookup or failure..forgejo/actions/(release.ymlonly reuses./.forgejo/workflows/checks.yml), and theknip.jsonedit removes exactly the two entries whose targets were deleted while keeping the workflow glob.pr-review.yml's dispatch assertion still matches the singlepost-reviewjob name.One thing I could not verify from here, consistent with the PR body's own canary plan: whether the runner resolves a
uses:URL pinned to a commit SHA on cold and warm fetches. I did not assert a limitation because I had no way to test it.Code review by Claude Code Opus (opus)
Summary: No actionable issues found.
Verified the swap from the in-repo composite action to the pinned shared writer:
action.ymlatac19a5f2declares exactly the same nine required inputs (axrecipe-url,axrecipe-audience,forgejo-token,review-slot,review-repository,review-number,review-head-sha,source-workflow-run-id,source-generator-attempt) and the samenode "${{ github.action_path }}/post-review.ts"entry point, so everywith:key in the workflow still binds.forgejo-post-review-common.tsand test-only changes topackage.json/tsconfig.json.REVIEW_SLOTS, thepinnedEnvironmentchecks onAXRECIPE_URL/AXRECIPE_AUDIENCE, and the trusted-head/capability logic are unchanged, so no behavioral regression. The slot allowlist still covers every slotpr-review.ymldispatches.node:processand its siblings; it never touchesnode:fs,process.cwd(), orGITHUB_WORKSPACE, and it resolves its own files throughgithub.action_path. Nothing else in the reduced job needs a working tree, andactions/setup-noderuns with an explicitnode-versionand nocache, so it needs no lockfile.ac19a5f2f8009f178b572faab54d66087e4607a0is the current head ofmainin the publicj4k-oss/forgejo-review-writerrepo, and pinning by SHA is immutable. Full-URLuses:already has precedent inpr-review.yml(https://data.forgejo.org/forgejo/...), so runner resolution is exercised..forgejo/actions/**— no tsconfig include, vitest glob, oxlint/oxfmt path, or other workflow.pr-review.yml's dispatch validation asserts the job idpost-review, which the workflow still uses. Theknip.jsontrim removes exactly the now-danglingentryand action-config glob while leaving the workflow glob intact.Code review by Claude Code Opus (opus)
Summary: No actionable issues found; 1 informational note.
I verified the migration end to end against the pinned commit:
action.ymlatac19a5fis byte-identical to the deleted local.forgejo/actions/post-review/action.yml— same nine required inputs, same env mapping, samenode "${{ github.action_path }}/post-review.ts"composite step. Every input the workflow still passes is declared.forgejo-post-review-common.ts; the pinnedAXRECIPE_URL/AXRECIPE_AUDIENCEconstants and thepinnedEnvironmentcheck are intact, and the slot allowlist (forgejo-review-{approach-smart-1,approach-smart-2,approach-3,code-smart-1,code-smart-2}) still matches the five slotspr-review.ymldispatches.node:fs, noGITHUB_WORKSPACE); it only consumes the nine action inputs plus runner-providedGITHUB_SERVER_URLand the OIDC request env. The old checkout existed solely to materialize the local action.actions/setup-node@v6is invoked withoutcache, so it needs no workspace either.uses:URL and commit-SHA refs, and recommends SHAs over tags. The referenced repository is public (anonymous read succeeds) andac19a5f2f8009f178b572faab54d66087e4607a0is the current tip of itsmain, so the ref resolves..forgejo/actions; the generator's dispatch validation still expects exactly one job namedpost-review, which is unchanged. Theknip.jsontrim removes only globs whose targets no longer exist, and the rootentry/projectscopes are untouched.Code review by Claude Code Opus (opus)
@ -62,3 +56,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0ℹ️ Info: Verified against the pinned commit: the shared
action.ymldeclares exactly the nine inputs passed here, and the writer sources atac19a5fdiffer from the deleted local copies only by an added comment — no runtime behavior change. The commit is the current tip of the writer repo'smain, so it is reachable today; if that branch is ever rewritten and the commit becomes unreferenced, this step would start failing to fetch. A tag or release ref pointing at the same commit would make the pin durable without weakening it, since the runner still resolves it to the same tree.Summary: No actionable issues found. 1 informational note.
The PR replaces the 14-file local
post-reviewcomposite action with a SHA-pinned shared action and drops the now-unneeded checkout step. I verified the migration end to end against the pinned commit rather than assuming it:ac19a5f2f8009f178b572faab54d66087e4607a0resolves on the public repoj4k-oss/forgejo-review-writer(private: false) and is the currentmainhead, so the runner can fetch and check it out.action.ymldeclares exactly the nine inputs the workflow passes (axrecipe-url,axrecipe-audience,forgejo-token,review-slot,review-repository,review-number,review-head-sha,source-workflow-run-id,source-generator-attempt) and maps them to the same env names; no input-name drift.forgejo-post-review-common.tsdiffers only by an added two-line comment, andaction.ymlonly by a leading---. The compiledAXRECIPE_URL/AXRECIPE_AUDIENCEpins are unchanged, so the existingvars.AXRECIPE_URL/vars.AXRECIPE_AUDIENCEstill satisfypinnedEnvironment.REVIEW_SLOTSallowlist matches the five slotspr-review.ymldispatches.node:processenv plus HTTP), and the dispatcher's response check still expects job idpost-review, which the job keeps.knip.jsonno longer points at deleted paths; the removedentryand.forgejo/actions/**config glob would otherwise be unresolved. The deleted.forgejo/actions/post-review/package.jsonwas not a pnpm workspace importer (pnpm-workspace.yamldeclares nopackages), so--frozen-lockfileis unaffected, and no other workflow or config references.forgejo/actions.Code review by Claude Code Opus (opus)
@ -62,3 +55,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0ℹ️ Info: Verified against the pinned commit: the action exists on the public
j4k-oss/forgejo-review-writerrepo atac19a5f…(currentmainhead), its declared inputs match thiswith:block exactly, itsREVIEW_SLOTSallowlist covers all five slotspr-review.ymldispatches, and its sources are byte-identical to the removed local files (including the compiledAXRECIPE_URL/AXRECIPE_AUDIENCEpins). The writer touches no repository files, so dropping the checkout step is safe. One consequence worth being aware of, not a defect: review publication now depends on the runner being able to fetchcode.j4k.devat job time, and a fetch failure inside the 3-minute job timeout fails the writer run the same way a checkout failure did before.Summary: No actionable issues found.
Verification performed against the pinned shared action (
ac19a5f, fetched anonymously fromcode.j4k.dev):action.ymldeclares exactly the nine inputs the workflow passes (axrecipe-url,axrecipe-audience,forgejo-token,review-slot,review-repository,review-number,review-head-sha,source-workflow-run-id,source-generator-attempt) and is otherwise byte-identical to the deleted localaction.yml, including the compositenode "${{ github.action_path }}/post-review.ts"step.forgejo-post-review-common.tsand repo packaging (package.json,tsconfig.jsoninclude)."type": "module"is retained, and the code stays dependency-free, so the un-installeddevDependenciesin the sharedpackage.jsondo not matter at runtime.actions/checkoutstep is safe:post-review.tsreads only environment variables plusGITHUB_SERVER_URLand talks to the Forgejo/Axrecipe HTTP APIs; nothing in the writer touches the workspace.actions/setup-nodeis still needed for the compositenodeinvocation and is pinned to an explicit version, so it does not need repository files either..forgejo/actions/post-reviewremain (pr-review.ymlonly matches thepost-reviewjob name in its dispatch check, which is unchanged), and theknip.jsontrim drops exactly the globs whose targets were deleted.Code review by Claude Code Opus (opus)
@ -62,3 +55,3 @@- name: Post exact finalized reviewuses: ./.forgejo/actions/post-reviewuses: https://code.j4k.dev/j4k-oss/forgejo-review-writer@ac19a5f2f8009f178b572faab54d66087e4607a0ℹ️ Info: Verified
ac19a5f2f8009f178b572faab54d66087e4607a0resolves inj4k-oss/forgejo-review-writerand itsaction.ymlinput set matches everywith:key below, so the swap is interface-compatible.One operational note, no change requested here: that commit is currently the tip of
mainand the repo has no tags. A commit SHA pin is only immutable while the object stays reachable — a force-push ofmainfollowed by GC on the Forgejo side would make the fetch fail. It fails closed (the writer job errors rather than running unpinned code), but the symptom would be reviews silently not being posted until the pin is refreshed, so keeping a protected tag or branch on the referenced commit in the writer repo is worth doing.Acknowledged: the exact-head review found no actionable defect; its fetch-failure note describes the intended fail-closed behavior at
8f46262.Acknowledged: the exact-head review found no actionable defect at
8f46262.