ci: pin the whole deploy path, and watch it weekly #11
Loading…
Reference in a new issue
No description provided.
Delete branch "ci/dependency-update-issues"
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?
Comes out of the pinning thread on #10, and replaces the part of it that was going to be a policy document nobody reads.
Three commits, one mechanism: pin everything the deploy path trusts, and run something that stops a hard pin from going stale. Either half alone is worse than useless — a checker over movable tags watches nothing, and a pin with nothing watching it rots into a two-year-old checkout action.
The problem, restated
Deno's dependencies are watched:
deno.json, a lockfile,deno outdated, anddeno install --frozenin the image build. Nothing underneath that was..forgejo/workflows/build.ymlbuilds the image and then SSHes to the production host, so everything it runs holdsPACKAGE_TOKENandDEPLOY_SSH_KEYwhile it runs — and every input to that job was named by something movable:actions/checkout@v6was whatever that repository decided it meant on the morning it ran;ssh-keyscanon every run, which trusted whatever answered on that address at that moment, one line before handing it the credential.Pinning fixes all three and creates one new problem: pins go stale silently, which is the actual reason nobody pins.
e56579a— the watchertools/dep-checkreads.forgejo/workflows/*.ymlanddocker/Dockerfile, asks each registry or forge which tags exist, and files an issue per dependency that is behind..forgejo/workflows/deps.ymlruns it Mondays at 06:00 UTC, plusworkflow_dispatch.Built to survive being ignored, because a weekly job that files a fresh issue every run is a job people mute, and a muted job is worse than no job — it looks like coverage:
Matching is shape first, order second. The non-numeric parts of a tag are a deployment choice and not a version, so
bin-2.5.6is never offered as an upgrade to2.5.6, a prerelease is only offered to something already on one, andbookworm-slimis reported as having no version to compare rather than as up to date — the distinction is the difference between a checker and a checker that lies.Within one shape a missing component counts as zero, so
v6is behindv6.0.1. That is the opposite of the rule the docker module wants for image pins, and the reason is written next to it: these are releases,v6andv6.0.0are the same one.06cd02f— the version pins.forgejo/workflows/build.ymlactions/checkout,docker/login-action,docker/setup-buildx-action,docker/build-push-action→ full commit SHAs.forgejo/workflows/deps.ymlactions/checkout→ SHA, and thedenoland/denocontainer it runs in → digestdocker/Dockerfiledenoland/deno(build),debian:bookworm-slim(runtime),denoland/deno:bin-…(theCOPY --from) → index digestsPinned at the versions already declared, not the newest. Moving
checkoutfrom v6 to v7 is a decision; the point of the weekly check is that it arrives as an issue rather than as a tag that moved on its own. The first run says so about six of the seven, which makes this PR self-demonstrating.Both hosts agree.
code.forgejo.organdgithub.comreturned identical SHAs for all four action tags, so the pin does not depend on which one the instance resolves from.The trailing
# v6comments are load-bearing. A SHA says nothing about which release it is, sodiscover.tsreads the comment — that is what keeps the checker answering after its own advice is taken. A pin whose comment lies is worse than no comment, which is why re-pinning moves both in one commit.One thing the pinning turned up:
COPY --from=denoland/deno:bin-2.5.6is a base image that appears in noFROMline, so nothing listed it anddiscover.tsdid not see it. It would have stayed unpinned and unwatched. Both fixed here, with a test.856d4b7— the host keyssh-keyscan -H 192.168.0.3 >> known_hostsis gone. Trust-on-first-use where every use is a first use is not trust; it is a formality that produces aknown_hostsfile. The key is pinned in.forgejo/known_hosts, and the deploy statesStrictHostKeyChecking=yesrather than relying on a default a runner image could have changed. A substituted host now fails verification instead of collecting the credential.In the repository rather than in a secret: a host public key is public by design, so committing it makes it reviewable and makes a change to it show up in a diff. Only the ed25519 key is listed — it is what the client prefers, and the RSA and ECDSA keys the host also offers would each be another way for verification to pass after this one was replaced.
Confirmed twice before being written down: it matches the entry already in the operator's
known_hosts, and it matches what the host reports for itself over an already-trusted session. Then verified with nothing else on the path:(The key's comment field still reads
root@rx200s6— a stale label from an earlier machine, not a mismatch. The key itself is what the live host presents.)A rebuild or a rekey now fails the deploy closed, which is the right outcome. Both the file and
docs/security.mdsay how to re-pin.Verification
Local; there is no CI on this branch, because
on: pull_requestis added by #10 anddeps.ymldeliberately has no PR trigger.And the loop closes — the checker still finds everything through the pins:
Six real findings on the first run. That is the backlog this repository already had and could not see.
--dry-rundoes every read and no write, and needs no token.Scope
Deliberately narrow.
tools/dep-checkdoes not watchdeno.json:deno outdatedalready answers for that, and a second opinion on the same question is a second thing to keep correct. Adding a kind is a small change if that turns out to be wanted.Notes for review
deno.jsongains"@std/assert": "jsr:@std/assert@^1"in the import map, becausedeno lintrefuses an inlinejsr:specifier and the new tests need assertions. #10 adds the identical line in the identical place, so the two should merge without a conflict — the one line here worth watching when they meet.Forge.label()looks thedependencieslabel up and files without it when absent. It does not create labels: a token that may only write issues should not fail a whole run over one, and a label nobody made is a label nobody filters on.uses: owner/repo@refresolves is an instance setting, so hosts are tried in order —code.forgejo.orgthengithub.com, overridable withDEP_CHECK_ACTION_HOSTS. Whichever answered is recorded in the issue body.docs/dependencies.mdcarries what is pinned and the re-pinning procedure with exact commands;docs/security.mdgains a section on the build and deploy path. Owner: whoever merges the dependency issue.ci: ask every week whether anything we pin has movedto ci: pin every action and base image, and watch them weekly`ssh-keyscan -H 192.168.0.3 >> known_hosts` ran on every deploy, which means the job trusted whatever answered on that address at that moment — one line before handing it DEPLOY_SSH_KEY. Trust-on-first-use where every use is a first use is not trust; it is a formality that produces a known_hosts file. The host key is pinned in .forgejo/known_hosts now, and the deploy states StrictHostKeyChecking=yes rather than relying on a default a runner image could have changed. A substituted host fails verification instead of collecting the credential. In the repository rather than in a secret: a host public key is public by design, so committing it makes it reviewable and makes a change to it show up in a diff. Only the ed25519 key is listed — it is what the client prefers, and the RSA and ECDSA keys the host also offers would each be another way for verification to pass after this one was replaced. The key was confirmed twice before being written down: it matches the entry already in the operator's known_hosts, and it matches what the host reports for itself over an already-trusted session. Verified against the live host with nothing else on the path: ssh -o StrictHostKeyChecking=yes -o UserKnownHostsFile=.forgejo/known_hosts \ -o BatchMode=yes jul14n@192.168.0.3 hostname A rebuild or a rekey now fails the deploy closed, which is the right outcome; both the file and docs/security.md say how to re-pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>ci: pin every action and base image, and watch them weeklyto ci: pin the whole deploy path, and watch it weeklyReviewed at
06cd02fagainst base1ae4844, in a clean worktree. Every inline comment below sits on a line this pull request adds — nothing here is a complaint about code that was already there. Ran the lot:deno fmt --check/deno lint/deno check tools/dep-check/main.tsclean, 26 tests pass, and the dry run reproduces the output in the description exactly.The pinning half is correct, and I verified it against the registries rather than taking the description's word for it. All four action SHAs resolve to the tag in their trailing comment on both
github.comandcode.forgejo.org, byte-identical; all three image digests match the current manifest for their tag:The shape-first version matching in
version.tsis genuinely careful —sha256-…rejected as a prefix,bin-never offered against plain, prerelease gated on the current tag. No complaints there.The checker half has three bugs that fire on this repository, not on a hypothetical one.
Blockers
1. Issue identity is the name alone, and two dependencies here share a name
The marker holds
name+latest;main.tsmatches onname=<dep.name>. ButDependencyidentity is kind + name + version, and the dry run finds twodenoland/denoentries —2.5.6andbin-2.5.6. After the first run both issues carry<!-- dep-check name=denoland/deno … -->, so each dependency matches both issues anddecidetakes whichever open one comes first.Reproduced against the real
decide, with two issues as the first run would leave them:The bin issue gets rewritten with the plain image's title and body; the plain issue is orphaned and never touched again. Next Monday the marker says
2.9.5, so it flips back. It ping-pongs one issue between two dependencies indefinitely, and the second issue is never closed even after the upgrade lands.The same collision runs through
resolve: upgrading one variant closes the other variant's issue, and that close is then treated as a decision.Fix is small — put the declared version in the marker (
name=… from=… latest=…) and match on name + version.2.
latest === nullmeans two different things, and one of them closes issuesdecidereads a nulllatestas "the repository caught up". It is also what you get from a registry that answers200 {"tags": []}, from a truncated page, and from a trailing comment the regex read as junk (# renovate: v6yieldsrenovate:). Reproduced:That path comments "no newer tag of that shape exists" and closes the issue. Combined with the closed-issue rule — a decision, never reopened — a single bad answer permanently silences that dependency for that version. Nothing writes to the log to say it happened.
resolveneeds positive evidence: a non-empty tag list that contains the declared tag. Absence of a result is not the same as absence of a newer version, and this tool's own argument is that the distinction is what separates a checker from a checker that lies.3. The 1000-tag cap is real and silent
Measured against the live registry:
Both cap out. Docker Hub sorts lexically, so the numeric and
bin-tags happen to land before the cutoff and today's answers are right by accident. Nothing warns when the cap is hit, so the first tag that sorts past it turns "up to date" into a guess with the same confident output.Cheapest correct fix: when the loop exits with
urlstill set, return anerror—mainalready skips those and reports them.Serious
Actions never paginate at all (
remote.ts) — one request,linkignored. Counts today: checkout 68, login-action 43, build-push-action 74. Under the cap, but the same silent class as #3, and GitHub's/tagsordering is not a documented sort.No error handling around any forge write (
main.ts) — a single non-2xx fromcreate/update/comment/closeescapesmain()and kills the run, leaving the remaining dependencies unprocessed. That directly contradicts the rule stated fifteen lines above it: "one unreachable host must not cost the other twelve their issue." The rule is right; it just is not applied to the write side.The entire mutating half is untested and has never executed.
issues_test.tscoversdecide;Forgehas no test.deps.ymlhas nopull_requestorpushtrigger and Forgejo only schedules from the default branch, so nothing on this branch has exercised the workflow, the container, the token, orcheckoutinside a Deno image. The first run ofcreate/update/comment/closewill be unattended, against the live tracker — and it hits #1 on its second execution.DEP_CHECK_FORGE_URL/DEP_CHECK_REPOSITORYexist precisely so this can be rehearsed against a scratch repository; worth doing before merge, or adding aForgetest with an injectedfetch.docs/dependencies.mdopens by stating something that is not true of this repository — that CI installs with--frozen. Nothing here passes it; the Dockerfile's install step is plaindeno install, which resolves past a stale lock rather than failing on it. The description repeats the claim. In a document whose entire job is to be accurate about what is watched and what is not, this is the one line that is not. Adding--frozento the Dockerfile is probably the better of the two fixes — same argument as the rest of this change, one layer up — but dropping the sentence also works.deps.ymlhas nopermissions:block. A pull request about narrowing supply-chain blast radius gives the job the runner's default full-repository token in order to file issues.contents: read+issues: writeis one line, and it is the argument the rest of the change makes.Moderate
updatefires on a change tolatest.tagand never on a change todep.version. Bumpv6→v6.5whilev7is still out and the issue keeps saying "declared at v6 and v7 exists", with the titlev6 → v7, indefinitely. Reproduced.A SHA-pinned
uses:with no trailing comment is dropped with no output at all — the exact failure this change exists to prevent, met with silence..forgejo/workflowsanddocker/Dockerfileare hardcoded single paths. TheCOPY --fromhole this pull request just fixed was the same class of bug: a place nobody had listed.Forge.issues()caps at 10 × 50 = 500 withstate=alland stops silently; past that the tool stops finding its own issues and refiles duplicates.existingis a single snapshot taken before the loop, so a create in one iteration is invisible to the next. Compounds #1.Minor
workflowImagesFrommatchesimage:at any nesting, including awith:input.FROMregex anchors on$, soFROM x:1 # commentis silently skipped.SHAis 40 hex only; git's sha256 object format is 64.reports.sortkeys ondep.namealone, so the twodenoland/denorows swap order between runs.deno task checkstill only walkspackages/server/main.ts, so a type error in the newtools/dep-checkfails neither the image build nor the push to main — only the Monday job. That is what the "must fail here, not on Monday" comment indeps.ymlis reaching for.Verdict
Worth splitting.
06cd02f— the pins — is correct, independently verified, and good to merge on its own.e56579a— the checker — should not be pointed at the live tracker until #1, #2 and #3 are fixed and the write path has been run once against a scratch repository.Everything above is reproducible; happy to hand over the probe scripts for #1, #2 and #3.
@ -0,0 +8,4 @@# Monday morning, before the week's work starts. Forgejo only runs scheduled# workflows from the default branch, so changes here take effect once merged.on:schedule:No
pull_requestorpushtrigger, and Forgejo only runs scheduled workflows from the default branch — so this file, the container image, the token wiring, andactions/checkoutrunning inside a Deno image (nogit, so it takes the REST tarball path) have all never executed.Everything verified in the description is the pure logic. The half that talks to the tracker is the half nothing has run.
@ -0,0 +15,4 @@workflow_dispatch:jobs:dependencies:No
permissions:block, so this job runs with the runner's default token scope in order to file issues.A change about narrowing supply-chain blast radius handing out a full-repository write token is the one place the argument is not applied to itself.
permissions: {contents: read, issues: write}is one line.@ -0,0 +26,4 @@# the checker is part of the repository, so it is held to the same bar# as the rest of it — a broken checker must fail here, not on Monday- name: TypecheckThe comment says "a broken checker must fail here, not on Monday" — but this workflow only runs on Monday (or on manual dispatch), so "here" is Monday.
The check that would make the comment true is
deno task checkcoveringtools/dep-check, since that is what runs on every push to main via the image build. Right now a type error in this tool passes CI and surfaces a week later.@ -0,0 +4,4 @@- **Deno packages** (`deno.json`, `deno.lock`) — `deno outdated` answers forthese, the lockfile keeps a resolve reproducible, and CI installs with`--frozen` so a stale lock fails the build instead of being resolved past.This is not true of this repository. Nothing here passes
--frozen; the Dockerfile's install step is a plaindeno install, which resolves past a stale lock rather than failing on it. The pull request description repeats the claim.In a document whose only job is to be accurate about which layers are watched and which are not, this is the one line that is not. Adding
--frozento the Dockerfile is probably the better fix — it is the same argument as the rest of this change, one layer up — but dropping the sentence works too.@ -0,0 +49,4 @@const name = ref.slice(0, at);const rev = ref.slice(at + 1);const version = SHA.test(rev) ? comment : rev;if (!version) return;A SHA-pinned
uses:with no trailing comment is dropped here with no output whatsoever.That is the precise failure this change exists to prevent — a hard pin with nothing watching it — and the tool's response is silence. It is also not a hypothetical: it is one forgotten comment away, in a repository that just pinned five actions.
Collect these and print them (
? actions/checkout: pinned by SHA with no version comment), and count them in the summary.@ -0,0 +63,4 @@export function workflowImagesFrom(text: string, file: string): Dependency[] {const out: Dependency[] = [];text.split("\n").forEach((line, i) => {const m = /^\s*image:\s*["']?([^"'\s]+)["']?\s*$/.exec(line);This matches any line whose key is
image:, at any nesting depth — awith:input to a scanning or build action would be picked up as a base image the repository depends on. Worth at least requiring the indentation of acontainer:child, or checking the preceding line.@ -0,0 +91,4 @@stages.has(ref.toLowerCase()) || /^\d+$/.test(ref);text.split("\n").forEach((line, i) => {const where = `${file}:${i + 1}`;const from = /^\s*FROM\s+((?:--\S+\s+)*)(\S+)(?:\s+AS\s+(\S+))?\s*$/i.exec(The
$anchor meansFROM x:1 # commentdoes not match and is skipped silently. Same failure mode as the uncommented SHA pin above — a dependency that exists and is not watched, with nothing said about it.@ -0,0 +154,4 @@/** every dependency declared under `.forgejo/workflows` and in the Dockerfile */export async function discover(root: string): Promise<Dependency[]> {const found: Dependency[] = [];const workflows = `${root}/.forgejo/workflows`;.forgejo/workflowsanddocker/Dockerfileare hardcoded single paths.The
COPY --from=denoland/deno:bin-2.5.6hole this pull request just found and fixed was exactly this class of bug: a place nobody had listed. A second Dockerfile added later is unwatched in the same way, and the tool will not say so.@ -0,0 +43,4 @@assertEquals(actionsFrom(pinned, "build.yml")[0].version, "v6");});Deno.test("a pinned action with no comment says nothing rather than guessing", () => {This test locks in the silence rather than the behaviour that is wanted. "Says nothing rather than guessing" is right about not guessing a version, but an unwatched pin is the failure the whole change is built around — it should be reported, not merely not-guessed.
Assert on a warning or a second return channel, not on
[].@ -0,0 +72,4 @@*/async issues(): Promise<ExistingIssue[]> {const out: ExistingIssue[] = [];for (let page = 1; page <= 10; page++) {Caps at 10 × 50 = 500 issues,
state=all, and stops silently. Once this repository passes 500 issues the tool stops finding its own and starts refiling duplicates — the exact outcome the marker scheme exists to prevent.Same silent-cap pattern as
MAX_PAGESinremote.ts; worth fixing both the same way.@ -0,0 +101,4 @@return labels.find((l) => l.name === name)?.id ?? null;}async create(This class is the entire mutating half of the tool and it has no test.
issues_test.tscoversdecide, which is the pure part.Combined with
deps.ymlhaving nopull_requestorpushtrigger, and Forgejo only running schedules from the default branch, nothing anywhere has executedcreate/update/comment/close. The first execution will be unattended, on a Monday, against the live tracker — and by then the name-collision blocker will already be in play.DEP_CHECK_FORGE_URL/DEP_CHECK_REPOSITORYmake a rehearsal against a scratch repository cheap. AForgetest with an injectedfetchwould be cheaper still.@ -0,0 +36,4 @@}export function marker(name: string, latest: string): string {return `<!-- dep-check name=${name} latest=${latest} -->`;The marker is the identity, and it is missing the half that disambiguates.
name=denoland/deno latest=2.9.5andname=denoland/deno latest=bin-2.9.5are the same dependency as far asmain.tsis concerned — see the blocker there.marker(name, from, latest)withfrom=<dep.version>fixes both this and the cross-variantresolve.@ -0,0 +91,4 @@);const open = mine.find((x) => x.issue.state === "open");if (!latest) {Blocker. A null
latestis read as "the repository caught up", but it is also what a registry answering200 {"tags": []}produces, and what a truncated tag list produces, and what a trailing comment the regex misread produces (# renovate: v6yieldsrenovate:).That comments "no newer tag of that shape exists" and closes the issue — and a closed issue is a decision that is never reopened for that version. One bad answer silences the dependency permanently, with nothing in the log to say so.
resolveshould require positive evidence: a non-empty tag list containing the declared tag. Absence of a result is not absence of a newer version — which is this tool's own argument for whybookworm-slimis reported as "nothing to compare" rather than "up to date".@ -0,0 +96,4 @@? { do: "resolve", issue: open.issue.number }: { do: "skip", why: "up to date" };}if (open) {updatetriggers on a change tolatest.tagand never on a change todep.version. Bumpv6→v6.5whilev7is still out and the issue keeps its original text forever:After a partial bump the issue states the wrong current version indefinitely.
@ -0,0 +98,4 @@);}}const existing: ExistingIssue[] = forge ? await forge.issues() : [];One snapshot, taken before the loop, then never refreshed. An issue created in iteration N is invisible in iteration N+1 — which is part of why the two
denoland/denodependencies both file on the first run and then start fighting over one issue on the second.@ -0,0 +104,4 @@for (const { dep, latest, source, error } of reports) {if (error) continue;const what = `${dep.name} ${dep.version}`;const mine = existing.filter((i) => i.body.includes(`name=${dep.name} `));Blocker. Identity is the name alone, but
Dependencyidentity is kind + name + version — anddiscoverfinds twodenoland/denoentries in this repository (2.5.6andbin-2.5.6). Both issues carryname=denoland/deno, so this filter returns both for each dependency anddecidepicks whichever open one is first.Reproduced with two issues as the first run would leave them:
The bin issue is rewritten with the plain image's title and body; the plain issue is orphaned and never closed. Next week it flips back — one issue ping-ponging between two dependencies indefinitely.
Fix: carry the declared version in the marker and match on name + version.
@ -0,0 +141,4 @@}console.log(` + ${what}: ${latest!.tag} available, filing an issue`);if (forge) {const number = await forge.create(title, body, labels);None of the forge writes in this loop are guarded. Any non-2xx from
create/update/comment/closethrows out ofmain(), so one failure halfway through leaves every remaining dependency unprocessed and the process exits on an unhandled rejection.That is the rule stated at line 76 — "one unreachable host must not cost the other twelve their issue" — applied to reads but not to writes. Catch per dependency, count the failures, and return non-zero at the end.
@ -0,0 +89,4 @@let token: string | null = null;const tags: string[] = [];try {for (let page = 0; page < MAX_PAGES && url; page++) {Blocker. This loop stops after 10 pages with no signal that it stopped early. Measured against the live registry:
Both cap out. Docker Hub sorts lexically, so the numeric and
bin-tags land before the cutoff and today's answers are correct by luck. The first tag that sorts past it turns "up to date" into a guess delivered in the same confident tone.When the loop exits with
urlstill set, returnfail(source, …)—mainalready reports and skips errors rather than acting on them.@ -0,0 +134,4 @@: `https://${host}/api/v1/repos/${name}/tags?limit=100`;}export async function listActionTags(Actions are not paginated at all — one request at
?per_page=100/?limit=100, and thelinkheader thatnextLinkalready knows how to read is ignored here.Counts today:
actions/checkout68,docker/login-action43,docker/build-push-action74. Under the cap, but the same silent-truncation class as the image path, and GitHub's/tagsordering is not a documented sort — so the newest tag being in the first page is not something the code is entitled to assume.Review found three ways this filed, rewrote or closed issues on the strength of something it had not actually established. All three fire on this repository rather than on a hypothetical one. **Issue identity was the name alone.** `denoland/deno:2.5.6` and `denoland/deno:bin-2.5.6` are two dependencies sharing a name, so after the first run each matched both issues and `decide` took whichever came first: one issue rewritten with the other's text, the second orphaned, the pair flipping every week. Identity is now kind + name + tag SHAPE, which is stable across both versions moving — the declared version cannot be the key either, or bumping v6 to v6.5 would orphan the issue and file a duplicate. **A null `latest` meant two different things and one of them closed issues.** It is "nothing newer exists", and it is also what an empty tag list, a truncated page and a misread version comment produce. Closing on that posts "no newer tag of that shape exists" and shuts the issue, and a closed issue is a decision that is never reopened — so one bad answer silenced a dependency permanently. Resolving now requires positive evidence: a complete tag list that contains the declared tag. **Tag lists were being truncated in silence**, which turns "up to date" into a guess delivered in the same confident tone. Measured: the ten-page ceiling cut `library/debian` at 1000 of 2976 tags and `denoland/deno` at 1000 of 1751, and the action lookup did not paginate at all, reading 50 of `actions/checkout`'s 68 because Forgejo caps a requested limit=100 at 50. The ceiling is now 60 pages, actions follow `link` like images already did, and reaching the end without exhausting the list is an ERROR rather than a short answer. All four now read complete: 68, 74, 2976, 1751. Also from the review: - Forge writes are wrapped per dependency. The read side already said "one unreachable host must not cost the other twelve their issue"; the write side was the one place that rule was not applied, and any non-2xx killed the run mid-way. - `Forge` has tests, with an injected `fetch`. It is the half that mutates a real tracker unattended, and it had never executed. - `deps.yml` runs the whole thing with `--dry-run` on every pull request, so the workflow, container, checkout and entry point are exercised on the way in rather than for the first time on a Monday. - `permissions: {contents: read, issues: write}`. A change about narrowing what a job can reach was handing this one the default full-repository token. - A pin nothing can compare — `uses: …@<sha>` with no `# tag` comment, an untagged `FROM` — is reported and reddens the run instead of being dropped. That is the exact failure this tool exists to catch; meeting it with silence made the tool complicit in it. - Issues are discovered by walking the tree for Dockerfiles and workflow directories rather than from two hardcoded paths. The `COPY --from` hole found yesterday was that same class of bug. - `Forge.issues()` pages to the end instead of stopping at 500 and quietly refiling duplicates; the issue snapshot is kept current as the loop writes; an issue is rewritten when the DECLARED version moves, not only the available one; `image:` only counts under `container:`; `FROM x:1 # c` is no longer skipped by the `$` anchor; a 64-hex SHA is a SHA. - `docker/Dockerfile` gains `deno install --frozen`, which `docs/dependencies.md` already claimed CI did. The claim is now true rather than deleted — same argument as the rest of this change, one layer up. - `deno task check` covers `tools/dep-check`, so a type error here fails the image build rather than surfacing a week later in the Monday job. Verified under the pinned CI toolchain (`denoland/deno:2.5.6`, via the digest this repository pins) rather than the local Deno: fmt and lint clean, 44 tests pass, `deno check` clean on both entrypoints, dry run reports all seven dependencies with both deno variants now distinct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Pushed
b2ade0a. All thirteen numbered items plus the minors are addressed; nothing declined. The three blockers were real and I reproduced each before fixing it.Thank you for measuring rather than reasoning about #3 — that one was wrong in a way I could not have argued my way to.
Blockers
1. Name-only identity. Correct, and worse than a collision: the two
denoland/denoentries would have rewritten each other's issue every week and orphaned one permanently. Your suggestion wasname + from, but that trades this bug for #9's — bumpv6tov6.5and the key changes, orphaning the issue and filing a duplicate. What is actually stable is the tag shape, whichversion.tsalready computes for the matching rule: identity is nowkind:name:shape, soimage:denoland/deno:~andimage:denoland/deno:bin-~are distinct and neither moves when a number does.fromis in the marker as well, but as data (see #9), not as identity.2.
latest === nullconflated. Correct, and the consequence you named — one bad answer permanently silencing a dependency, because the close is then treated as a decision — is the worst failure mode in the tool.decidenow takessettled, which is the caller's answer to "do we hold a complete tag list containing the declared tag". Only that closes an issue. Everything else skips with a reason.3. The 1000-tag cap. Real, and my fix is different from the one you proposed because the measurement pointed elsewhere. Erroring at the cap would have made
debiananddenoland/denopermanently unanswerable, since both exceed it. Following the pagination to the end instead:Both registries do serve the whole list; the ceiling was just too low. It is 60 pages now, actions follow
linkexactly as images already did, and reaching the ceiling is an error rather than a short answer — which is the part of your comment that mattered most. Your "correct by accident" reading was exactly right: lexical sort was doing the work.Serious
4. Actions paginate. Same
nextLinkthe image path used.5. Every forge write is wrapped per dependency, counted, and the run exits non-zero. You were right that the rule was stated fifteen lines above the code that broke it.
6. Two of the three things you asked for, and one I cannot do from here honestly:
forge_test.ts— nine tests against an injectedfetch: auth header, URL shape, request bodies for all four writes, non-2xx surfacing as an error, pagination to the first short page, and the refusal to decide from a truncated issue list.deps.ymlnow has apull_requesttrigger running--dry-run, so the workflow, the container,checkoutinside a Deno image and the entry point all execute on the way in. This PR is the first exercise of it — if it goes red, that is the review working.DEP_CHECK_FORGE_URL/DEP_CHECK_REPOSITORYare there for it.7. Fixed by making the claim true:
docker/Dockerfilenow runsdeno install --frozen. Same argument as the rest of the change, one layer up — and it is byte-identical to the line #10 adds, so the two merge cleanly.8.
permissions: {contents: read, issues: write}. One caveat written into the file rather than glossed: enforcement depends on the runner honouringpermissions, which I have not verified on this instance. Stating it costs nothing and is the correct declaration either way, but I am not claiming a guarantee I have not tested.Moderate and minor
9. An issue is rewritten when the declared version moves, not only the available one — both halves of the sentence it makes have to stay true. Test covers the partial bump.
10. A SHA pin with no comment now comes back in an
unwatchedchannel, is printed, and reddens the run. So does an untaggedFROM. You were right that the test locked in the silence; it now asserts the report.11. Discovery walks the tree for
Dockerfile*and both.forgejo/workflowsand.github/workflows.12.
Forge.issues()pages to the first short page with no round-number ceiling, and throws rather than deciding from a partial list.13. The snapshot is kept current as the loop writes.
Minors:
image:only counts undercontainer:(inline form handled too); theFROMregex allows a trailing comment; 64-hex SHAs are SHAs; sort breaks ties on version.deno task checknow coverstools/dep-check/main.ts, so a type error here fails the image build rather than waiting for Monday. That also makes the comment indeps.ymltrue, which it was not.Verification
Run under the pinned CI toolchain —
denoland/deno:2.5.6by the digest this PR pins — rather than my local Deno, which is 2.9.4:Worth flagging since it bit me:
deno task checkis red onmainunder Deno 2.9.4 — twoTimeout-vs-numbererrors inpackages/serverthat do not occur under 2.5.6. Not mine, and #10 fixes them properly. I mention it only because adding an entrypoint to that task makes it look like this PR broke it.On splitting
You are right that
06cd02fstands alone ande56579adid not. I would rather not split now — the checker is the thing that makes the pins maintainable, and landing pins without it recreates the staleness problem that stopped anyone pinning in the first place. If you still want them apart after this pass, say so and I will reorder into two PRs with the pins first.CI is green at
bd1c4c1— run #428, 1m09s,pull_request. That is the first time this workflow has executed at all, and it took two failures to get there, both of which were mine and both worth writing down:container: denoland/deno:2.5.6cannot runactions/checkout. The image ships nonode(norgit, norcurl), so the job died atexec: "node": executable file not found in $PATHbefore step one.docker run -v "$PWD":/wmounted an empty directory. The runner executes steps inside its own container, so$PWDthere is not a path the host daemon knows — the mount succeeds against nothing, surfacing asImport 'file:///w/tools/dep-check' failed, not found.Both are now resolved in .forgejo/deno.sh: the job runs on the runner, each command enters the digest-pinned image, and the workspace is copied in with
docker cp. Your #6 was right for a better reason than either of us gave — the workflow had never run, and it did not work.I cannot mark these threads resolved; Forgejo's API has no endpoint for it. Here is the state of every item instead, so the thread can be closed out by eye.
Fixed as asked
remote.ts—tagsFromHostfollowslink; checkout now reads 68 tags, not 50main.ts— try/catch per dependency, counted, run exits non-zero--frozenclaim untruedocker/Dockerfile— claim made true rather than deletedupdateignoresdep.versionissues.ts—open.mark.from === dep.versionis part of the skip conditiondiscover.ts—unwatchedchannel, printed, reddens the rundiscover.ts— walks forDockerfile*, reads both workflow directoriesforge.ts— pages to the first short page, throws rather than deciding from a partial listmain.ts—existingupdated as the loop writesimage:only undercontainer:;FROM x:1 # cno longer skipped; 64-hex SHAs; sort breaks ties on version;deno task checkcoverstools/dep-checkFixed, but not the way you proposed
1, identity. You suggested
name + from. That trades this bug for #9 — bumpv6tov6.5and the key moves, orphaning the issue. Keyed onkind:name:shapeinstead, which is stable while either version moves.fromis in the marker as data, not as identity.3, the tag cap. Erroring at the cap would have made
debian(2976 tags) anddenoland/deno(1751) permanently unanswerable, since both exceed it. Raised the ceiling to 60 pages and made reaching it an error — the second half is the part your comment was actually about.Cannot be done as asked
8,
permissions:. The runner answered this one directly: "Job dependencies or its workflow has a permissions field, which is not supported in Forgejo and will be ignored. Use Authorized Integrations to grant capabilities to this job instead." I had added the block with a caveat that I had not verified enforcement; the honest resolution is that it is gone. A declared restriction that is not in force is worse than an absent one. The lever that does exist is documented: an Authorized Integration scoped to issues, supplied asDEP_CHECK_TOKEN. Until that secret exists this job runs with the runner's default token — stated indeps.ymlanddocs/dependencies.mdrather than implied otherwise.Still open
6, the write path has never touched a real tracker. Two thirds done:
forge_test.tscovers it against an injectedfetch, and the PR dry run now exercises the workflow, checkout and entry point for real. The live rehearsal against a scratch repository is the honest remaining gap — it means creating and deleting a repository on this instance, so I would rather @julian say when.DEP_CHECK_FORGE_URL/DEP_CHECK_REPOSITORYare there for it.Unrelated to this PR but found while reading the runs: #10's
verifyjob has the same container/checkout defect and has never passed. Runs 175–186 are twelve consecutivepull_requestfailures; every push-to-main run before them is green. Its stated purpose — "review has something to go on before main does" — is not being delivered, and every verification claim on that PR came from local runs. Flagged there rather than fixed, since it is your branch.Second pass, at
bd1c4c1against base1ae4844. Every inline comment is on a line this pull request adds.Reproduced independently rather than read: 44 tests pass, dry run exits 0 with both
denoland/denovariants distinct,deno lintclean over 10 files.forge_test.tsis the real thing — injectedfetch, auth header, request bodies for all four writes, non-2xx rejecting, pagination stopping at the first short page, and the refusal to decide from an unbounded list. Thesettledflag, the 60-page ceiling that errors instead of truncating,linkpagination for actions, per-dependency try/catch, the liveexistingsnapshot,--frozen, and thefromhalf of the marker all check out. Removingpermissions:and quoting the runner's actual warning was the right call over keeping a declaration that is not enforced.Three of the original thirteen are genuinely closed in a way I could verify by running the code. Two new problems arrived with the fixes, and one of them is worse than what it replaced.
Blockers
1. The shape key collides across declaration sites, and now it closes the issue
depKeyiskind:name:shapeKey(version), andshapeKeydeliberately erases the numbers. So two declarations of the same thing at different versions share one key:actions/checkoutis declared in bothbuild.yml:30anddeps.yml:34. It is also the subject of the first issue this tool will file. So the most likely next event in this repository is: someone acts on that issue, bumpsbuild.yml, missesdeps.yml. Then:The run comments "no newer tag of that shape exists" and closes the issue on the strength of the site that was upgraded, while the other site is still on v6. And a closed issue is a decision that is never reopened for that version — so
deps.ymlstays onv6, permanently unwatched, with the tracker asserting it is current.That is the same failure mode as blocker #2 last round (closing on an answer that does not mean what it is taken to mean), reached by a different route, and it now triggers on the ordinary success path rather than on a registry hiccup.
Keying on
name + fromwas wrong for the reason you gave. But the fix is not a different key — it is that one key must not have two versions. Either fold same-key declarations into a single dependency whosewherelists both sites and whose version is the oldest, or make a same-key/different-version pair a reported finding in its own right. The second is arguably the most useful sentence this tool could say: "actions/checkoutis declared at v6 in one place and v7.0.1 in another." Nothing else in this repository is watching for that, which is the whole premise.2.
.forgejo/deno.shpins a digest that nothing watchesIMAGE="denoland/deno:2.5.6@sha256:3ea71953…". Ran discovery against the tree:It is the only digest pin in the repository that discovery does not read, and it is in a file this pull request adds. This is
COPY --from=denoland/deno:bin-2.5.6verbatim — "a base image that appears in noFROMline, so nothing listed it and it would have stayed unpinned and unwatched" — which the description rightly presents as the thing pinning turned up. The fix for CI reintroduced it one directory over.Worse than the original, because the tool now reports
denoland/deno 2.5.6as fully covered while one of its three declarations is invisible. Bump the Dockerfile anddeno.shsilently stays behind, on a digest, forever.Either teach discovery to read
IMAGE=-shaped pins out of.forgejo/*.sh, or move the pin somewhere discovery already looks and have the script read it from there.Security — confirm before merging
3.
pull_requeston a public repository, a self-hosted runner, and the host Docker socketAdding the trigger was right, and #6 is better for it. But look at what the trigger now reaches:
deps.ymlruns oncth-ubuntu-latestand every step shells out to.forgejo/deno.sh, which runsdocker create/docker cp/docker startagainst the host daemon. This repository is public (private: false).A pull request from a fork supplies its own
.forgejo/deno.sh, and the runner executes it. Access to the host Docker daemon is host root, on the same runner thatbuild.ymluses withPACKAGE_TOKENandDEPLOY_SSH_KEY. Forge secrets are withheld from fork pull requests, so the immediate reach is the runner rather than the secrets — but the runner is the thing that holds them on the next push to main.I am flagging this rather than asserting it, because it turns entirely on whether this instance lets anyone fork. Worth confirming that before merge. If forks are possible, the usual gate is running the job only for same-repository pull requests:
Note this is not new in kind — #10 adds
on: pull_requesttobuild.yml— but it is the first one that hands a pull-request-controlled script the Docker socket.Moderate
docker cp . "$id:/w"works by accident. With a directory source and an existing destination directory,docker cpcopies the directory into the destination; with a non-existent destination it copies the contents. It does the second only becausedenoland/denohas no/w. A base image that ships one, or a change indocker cp's handling, nests the tree one level deeper and surfaces as exactly theImport 'file:///w/…' failed, not foundthe comment above it says was already solved once.docker cp ./. "$id:/w"states the intent instead of relying on the destination's absence.forge.label()is outside the guard thatcreate/update/closenow have. A token that may write issues but not read labels aborts the whole run before a single issue is filed — the failure mode #5 was about, in the one call that did not get wrapped. (issues()throwing is deliberate and correct;label()is not the same case, and the doc comment above it says as much: "should not fail the whole run on a label".)unwatched.lengthmakes the run red. Right for the pull-request check. On the weekly run it means "somebody forgot a# v6comment" and "the checker is broken" arrive as the same signal. Worth separating eventually; not worth blocking on.issues_test.tscovers two variants of one image having different identities, but not two versions of one variant. That is the hole blocker #1 lives in — the test that would have caught it is one line from one that exists.Minor, and two things that are not your fault
docs/modules.md(+256 non-whitespace lines) anddocs/push-notifications.md(+82) are puredeno fmtreflow. Checked word by word: nothing changed but rewrapping,*em*→_em_, table-separator padding, and reformatting of fenced code samples. 380 lines of unrelated diff a reviewer has to scan to establish they are noise. Worth its ownchore(docs): deno fmtcommit.Not yours, but relevant to that:
deno fmt --checkon this head under Deno 2.9.4 reportsFound 81 not formatted files in 227. The formatter's output is toolchain-dependent, so the reflow above churns straight back the first time anyone formats on a newer Deno. Something to know before committing to formatted docs.Also not yours, and I want it on the record so it is not mistaken for a regression:
deno task checkis red under Deno 2.9.4 with the twoTimeout-vs-numbererrors you flagged. I confirmed they are pre-existing —heartbeatTimer: number | nullis unchanged at1ae4844— so addingtools/dep-check/main.tsto the task does not regress anything, and the image build runs under 2.5.6 where it passes. Your note about it was accurate.The host key.
.forgejo/known_hostsparses as a well-formed ed25519 entry,SHA256:CBJYIn5cdGyK+GC3PFYeRpFya4Oz9vUfGwIsYyG06ZQ. Whether it is the right host is not something a reviewer can establish from the diff, and I am not going to open a connection to your production host to find out — so that verification stands on your two local checks, which is the correct place for it to stand. The change itself is right: replacing per-runssh-keyscanwith a pinned key,StrictHostKeyChecking=yesstated rather than defaulted, and one key rather than three. The-o UserKnownHostsFile=~/.ssh/known_hostsis redundant with the default but harmless.Verdict
The three original blockers are properly dead, and #6 in particular is better than what I asked for — the workflow failing twice on the way in is worth more than the tests. But #1 and #2 are both regressions of the specific arguments this change is built on: acting on an answer that does not mean what it is taken to mean, and a pin with nothing watching it. Neither is hard to fix.
On splitting: I withdraw it. The checker is close enough that separating them would cost more than it buys.
Reproductions for #1 and #2 are two short scripts against your own exports; say the word and I will paste them.
@ -0,0 +18,4 @@# Usage: .forgejo/deno.sh test --allow-read tools/dep-check/set -euIMAGE="denoland/deno:2.5.6@sha256:3ea71953ff50e3ff15c377ead1a8521f624e2f43d27713675a8bed7b33f166aa"Blocker. This digest is pinned and nothing watches it. Ran discovery against the tree:
It is the only digest pin in the repository discovery does not read.
This is
COPY --from=denoland/deno:bin-2.5.6again — "a base image that appears in no FROM line, so nothing listed it and it would have stayed unpinned and unwatched", which the description presents as the find that justified the whole exercise. The CI fix reintroduced it one directory over.Worse than the original: the tool now reports
denoland/deno 2.5.6as covered while one of its three declarations is invisible. Bump the Dockerfile and this stays behind, on a digest, indefinitely.Either read
IMAGE=-shaped pins out of.forgejo/*.sh, or move the pin to somewhere discovery already looks and have the script read it from there.@ -0,0 +31,4 @@id=$(docker create -w /w $env_args "$IMAGE" deno "$@")trap 'docker rm -f "$id" >/dev/null 2>&1 || true' EXITdocker cp . "$id:/w" >/dev/nullThis works by accident.
docker cpcopies a source directory into an existing destination directory, and copies its contents only when the destination does not exist. It does the second here purely becausedenoland/denohas no/w.A base image that ships one — or a change in how
docker cpresolves.— nests the workspace a level deeper and surfaces as exactly theImport 'file:///w/…' failed, not foundthe comment above says was already solved once.docker cp ./. "$id:/w"states the intent rather than depending on the destination's absence.@ -0,0 +20,4 @@## checked against the key the host reports for itself over an already-trusted# session (`ssh <host> cat /etc/ssh/ssh_host_ed25519_key.pub`).192.168.0.3 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAICgU0tgg/3hqPSKtwmnTyG+6DEQSVxygoMj5wXlJS998Parses as a well-formed ed25519 entry,
SHA256:CBJYIn5cdGyK+GC3PFYeRpFya4Oz9vUfGwIsYyG06ZQ.Whether it is the right host is not establishable from a diff, and I am not going to open a connection to your production host to find out — so that rests on your two local checks, which is where it belongs. Worth saying explicitly rather than implying the pin was verified end to end by review.
The change itself is right: per-run
ssh-keyscanreplaced by a pinned key,StrictHostKeyChecking=yesstated rather than defaulted, and one key instead of three. The reasoning for ed25519-only is correct — extra host key types are extra ways for verification to pass after this one is replaced.@ -0,0 +17,4 @@# Without this the workflow, the checkout and the tool's own entry point# would first execute unattended on a Monday, against the live tracker —# the one place where "it has never run" is least acceptable.pull_request:Security — worth confirming before this merges. Adding the trigger was right and #6 is better for it. But consider what it now reaches: this job runs on
cth-ubuntu-latestand every step shells out to.forgejo/deno.sh, which drives the host Docker daemon (docker create/cp/start). This repository is public.A fork's pull request supplies its own
.forgejo/deno.shand the runner executes it. Docker socket access is host root — on the same runnerbuild.ymluses withPACKAGE_TOKENandDEPLOY_SSH_KEY. Fork pull requests do not get secrets, so the immediate reach is the runner rather than the credentials; the runner is what holds them on the next push to main.This turns on whether your instance lets anyone fork, which I cannot tell from here. If it does, the usual gate:
Not new in kind — #10 adds
on: pull_requesttobuild.yml— but this is the first one handing a pull-request-controlled script the Docker socket.@ -20,3 +20,3 @@"build": "deno task build:shell && deno task build:shared && deno task build:modules","start": "deno run --allow-read --allow-write=data --allow-env --allow-net --allow-ffi --allow-run packages/server/main.ts","check": "deno check packages/server/main.ts","check": "deno check packages/server/main.ts tools/dep-check/main.ts",Correct fix for the "fails on Monday" problem, and I want the consequence on the record so it is not mistaken for a regression from this change:
deno task checkis red under Deno 2.9.4 with the twoTimeout-vs-numbererrors you flagged. I confirmed they pre-date this branch —heartbeatTimer: number | nullis unchanged at1ae4844— so adding this entry point does not regress anything, and the image build runs under 2.5.6 where it passes. Your note about it was accurate.@ -50,3 +50,3 @@// admin-only mutationctx.router.post("/things", ctx.auth.requireRole("admin"), (c) => c.json({ ok: true }));ctx.router.post(This file and
docs/push-notifications.mdare puredeno fmtreflow — I checked word by word, and nothing changed but rewrapping,*em*→_em_, table-separator padding, and reformatting inside fenced code samples. 256 and 82 non-whitespace lines respectively.No objection to the formatting; it is 380 lines of unrelated diff a reviewer has to read to establish they are noise. Own commit —
chore(docs): deno fmt— keeps the pinning change reviewable.Worth knowing before committing to this:
deno fmt --checkon this head under Deno 2.9.4 reportsFound 81 not formatted files in 227. The formatter's output is toolchain-dependent, so this reflow churns straight back the first time anyone formats on a newer Deno.@ -0,0 +225,4 @@".claude",]);const WORKFLOW_DIRS = [".forgejo/workflows", ".github/workflows"];Discovery reads workflow YAML and
Dockerfile*..forgejo/deno.shnow pins a digest too, and this list is why it goes unseen — see the blocker on that file.The walk itself is a real improvement over the two hardcoded paths, and skipping
.claudeis a good catch given worktrees live there.@ -0,0 +40,4 @@* its own. Not the declared version either: bumping `v6` to `v6.5` would* orphan the issue and file a duplicate. The shape is what stays put.*/export const depKey = (dep: Dependency): string =>Blocker — this is where the close comes from.
shapeKeyerases the numbers on purpose, so two declarations of the same thing at different versions share one key:actions/checkoutis declared in bothbuild.yml:30anddeps.yml:34, and it is the subject of the first issue this tool will file. So the likely next event is a partial bump —build.ymlmoved,deps.ymlmissed:The run comments "no newer tag of that shape exists" and closes the issue, while a site is still on v6 — and a closed issue is never reopened for that version.
deps.ymlis then stale, unwatched, with the tracker asserting it is current.name + fromwas wrong for the reason you gave. The fix is not a different key: one key must not hold two versions. Fold same-key declarations into one dependency (oldest version, both sites inwhere), or report the pair as a finding — "declared at v6 here and v7.0.1 there" is arguably the most valuable thing this tool could say, and nothing else watches for it.@ -0,0 +46,4 @@assertEquals(issueTitle(DEP, V7), "deps(action): actions/checkout v6 → v7");});Deno.test("two variants of one image have different identities", () => {This covers two variants of one image having different identities, which is the bug from last round and is properly dead.
The hole blocker #1 lives in is one line away: two versions of one variant —
actions/checkoutatv6indeps.ymlandv7.0.1inbuild.yml— which share a key and make the run close the issue while a site is still stale.@ -0,0 +112,4 @@return 2;}forge = new Forge(cfg);const id = await forge.label(LABEL);label()is the one forge call left outside the guard thatcreate/update/comment/closenow have. A token that may write issues but not read labels aborts the run here, before anything is filed — which is the failure mode #5 was about.The doc comment on
Forge.labelalready states the intent: "a token that may only write issues should not fail the whole run on a label". It just is not what the code does. Catching around this and falling through tolabels = []finishes the thought.(
issues()throwing is deliberate and correct — deciding from a partial issue list is the thing that refiles duplicates.)@ -0,0 +193,4 @@// Red when something was actually wrong: a write that failed, a dependency// nothing can watch, or every single lookup failing (one is noise, all of// them is a broken checker).return failed || unwatched.length || unreachable === reports.length ? 1 : 0;unwatched.lengthreddening the run is right for the pull-request check — a pin nobody can compare should stop the merge.On the weekly run it means "somebody forgot a
# v6comment" and "the checker itself is broken" arrive as the same red. Worth separating eventually; not worth blocking on.@ -0,0 +64,4 @@** An unversioned tag has no shape to speak of, so it stands for itself.*/export function shapeKey(version: string): string {The doc comment argues the shape is what stays put while the numbers move, and that is right for one declaration site. It stops being true with two:
2.5.6and2.9.5are the same shape and must not be the same issue.Whichever way #1 is resolved, this function is fine — the collision belongs to
depKey's use of it, not to the definition of a shape.julian referenced this pull request2026-08-10 14:06:55 +02:00
Pushed six commits at
ab305fc. Both blockers fixed, the security question answered with a check rather than a guess, and the three moderates that were fixable are fixed. One is deferred, with the reason; nothing is disputed.You were right about the shape of #1 in a way I want to name before the fix: both blockers are the same mistake I said this change existed to prevent, made one layer out from where I was looking. Acting on an answer that does not mean what it is taken to mean, and a pin with nothing watching it.
1. One key must not hold two versions —
c069a7cFixed by folding, not by re-keying.
dedupe()is nowfold(), and it folds on the issue identity rather than onkind+name+version, producing oneDependencythat carries every declaration insitesand reports the oldest version among them.That makes the partial bump you described a non-event. The issue is about the site still on
v6, so it stays open — and correct — until every site has moved:Which does mean the title reads
v6 → v7.0.1for as long as one site lags, while another already sits onv7.0.1. That is the intended reading: the issue is not "a newer version exists", it is "this repository is not on it yet", and it should not stop saying so while that is true.I took your second suggestion as well, because you were right that it is the most useful sentence available. When sites disagree the issue says which version is where —
— and the run logs it, so a
--dry-runsurfaces it too. Nothing else here watches for that.Two definitions had drifted apart and are now one:
depKeylives indiscover.tsnext to the fold that needs it and is re-exported fromissues.ts. The marker key and the fold key being the same key is load-bearing; two definitions of it was how this got in.Declarationis what one line says,Dependencyis what one issue is about.Your inline note on
shapeKeywas right too — I left it alone, the collision wasdepKey's use of it.2.
.forgejo/deno.sh—563ecf3Removed the pin rather than teaching discovery to read shell scripts. The image comes out of the Dockerfile's build-stage
FROM, which is already watched:One declaration instead of two, and it is one the checker reads. It also closes a gap I had only asserted: the script's own comment says the toolchain must be the same image the Dockerfile builds with, and now that is enforced rather than maintained by hand. Empty
IMAGEexits 1 with a message.3. Fork pull requests and the docker socket —
74c32b6Checked instead of assumed, and you were right to flag it:
So the trigger is reachable by anyone with an account. Gated to same-repository pull requests; scheduled and dispatched runs are unaffected.
build.ymlgets the same treatment when #10 adds its trigger — that belongs in that pull request, not this one.4.
docker cp . "$id:/w"—563ecf3Real, and it reproduces exactly as you described. Against the pinned image with a destination that does exist:
Now
docker cp ./., which asks for the contents whether or not the destination exists.5.
forge.label()outside the guard —8590294Fixed. It warns and files without the label.
issues()stays fatal, for the reason you gave.6.
unwatched.lengthreddening the weekly run — deferred, deliberatelyAgreed, and not changed. The separation I would want is not a second exit code: an unwatched declaration is a finding, and this tool's whole argument is that findings belong in the tracker rather than in a run's exit status. So the right version files an issue for it like everything else, and then the weekly run only goes red when the checker is actually broken. That is a bigger change than this pull request should carry, and the current behaviour is wrong only in being coarse — it does not close anything or say anything false. Left as a follow-up.
7. The test that was one line away —
c069a7cAdded, in both files:
foldon two versions of one variant, anddecidethrough the full sequence — file, partial bump, complete bump — asserting it never resolves on the strength of the upgraded site. Also that an unversioned tag folds only with the identical string, sinceshapeKeyreturns the tag itself there.48 tests,
deno lintanddeno fmt --checkclean,deno check tools/dep-check/main.tsclean, dry run exits 0 and still reports all seven.8, 9. The docs reflow —
4533047Reverted to base rather than split into its own commit. Your #9 is why: nothing checks formatting in CI, and if
deno fmtoutput moves between toolchains then committing 380 reflowed lines here just guarantees they churn back. Formatting the docs is a decision worth taking on purpose, on a chosen Deno version, in a pull request about that.docs/dependencies.mdanddocs/security.mdkeep their real content.10, 11. On the record
Both noted, nothing changed. Thank you for confirming the
deno task checkerrors pre-date the branch — that was worth having from someone other than me.And you drew the line in the right place on the host key. It rests on the two local checks, and it should; a reviewer cannot establish that from a diff, and I would not want one opening a connection to production to try.
Every reproduction you offered, I could reproduce from the description alone, so I did not need the scripts — but the offer is what made them quick to confirm. The
docker cpone in particular I would have argued with if you had not given the exact rule.Third pass, at
ab305fcagainst base1ae4844. Every inline comment is on a line this pull request adds.Both blockers are properly dead, and I checked by running rather than reading: 48 tests pass,
deno lintclean over 10 files,deno fmt --checkclean over 14, dry run exits 0 with all seven dependencies. Dropping to a comment — nothing left here blocks a merge.Two of your fixes are better than what I proposed, and it is worth saying which and why:
The fold. I suggested folding or reporting the disagreement; you did both, and folding at the oldest version is the part that makes it correct rather than merely different. The issue now stays true while the repository catches up, which is the property the whole closed-issue rule depends on. Reporting the newest would have been the obvious reading of "one issue per thing" and would have kept the bug.
The toolchain pin. I offered two options — teach discovery to read shell scripts, or move the pin somewhere watched — and you took a third that is better than either: delete the second declaration entirely and derive it from the
FROMthat already exists. Verified:One declaration, and it is one the checker reads. It also turns "CI must use the same toolchain the image is built with" from a comment into something enforced.
The fork check was the right way to answer #3 —
forks_disabled: falseand an open registration form is a fact, and I had only been able to offer a conditional. And I think you are right to defer #6: an unwatched declaration is a finding, findings belong in the tracker, and a second exit code would be the wrong shape of fix. Better reasoning than my original comment.Everything below is minor. Two of them are one fix.
1 and 2 —
split()andfold()disagree withversion.tsabout what a version isversion.tsstates it outright, in the comment oncompare: "these are RELEASES,v6andv6.0.0are the same one."split()decides with a stringSetinstead, andfold()'s tie-break inherits the same gap:So the new disagreement warning — the most valuable sentence in the tool — fires on a distinction the tool elsewhere denies, and when it does, which version lands in the title, in the marker's
from, and insettleddepends on directory read order.No such pair exists in this repository today, so this is latent rather than live. One fix covers both: compare parsed
numsand fall back to string equality only whenparseVersionreturns null — which is the same rulebehind()already uses two functions up.3 —
Dependency.whereis now deadInherited from
Declaration, maintained byfold(at.where = d.where), and read by nothing:main.tsandissues.tsboth go throughsites. It is a field that looks authoritative and holds only the oldest site's line. Either drop it, or stop havingDependencyextendDeclarationso the type says what it means.4 —
deno.shis now coupled to Dockerfile syntax, and that is worth knowingFROM --platform=$BUILDPLATFORM denoland/deno:…, or a registry-qualified name, does not match thesedand the script exits 1 with a clear message. Failing loud is right and I am not asking for it to change. But a Dockerfile refactor now breaks CI from a file the Dockerfile never mentions, and the only thing that says so is a comment in the shell script. One line indocs/dependencies.mdunder the re-pinning procedure would put it where someone editing thatFROMwill see it.5 — confirm the
if:gate on this pull request's own runIf
github.event.pull_request.head.repo.full_namedoes not populate on this Forgejo, the expression is false and the job skips on every pull request. That fails safe, but it silently removes the check you just added — and a skipped job does not look like a problem.This pull request is same-repository, so its own run answers it: green means the gate works, skipped means it does not. Worth glancing at before merging rather than after.
Still open
The write path has not touched a real tracker. You have asked twice for a go-ahead on the scratch-repository rehearsal, and that is the right person to ask — it means creating and deleting a repository on the instance. Flagging it here so it does not get lost between rounds, not to press it.
Verdict
Approve after 1 and 2, which are one change. Everything else is optional and none of it blocks.
Two things I want on the record because they were yours: catching that
container: denoland/denocannot run a Node action, and that a bind mount from inside the runner's own container mounts nothing, are both findings that only exist because you ran the thing. My #6 asked for a rehearsal and got something better — a workflow that failed twice in public and is now known to work.@ -0,0 +25,4 @@# file, so a digest written here would go stale in silence. The build stage's# FROM is the one declaration; it is already watched, and reading it from# there is also what guarantees CI runs the toolchain the image is built with.DOCKERFILE=docker/DockerfileBetter than either option I offered. Deleting the second declaration beats teaching discovery to read shell scripts and beats moving the pin somewhere watched, because it leaves one declaration rather than two that have to agree. Verified:
One note worth putting somewhere a reader will hit it: this couples CI to the Dockerfile's
FROMsyntax.FROM --platform=$BUILDPLATFORM denoland/deno:…, or a registry-qualified name, does not match and the script exits 1 — failing loud, which is right. But a Dockerfile refactor then breaks CI from a file the Dockerfile never mentions, and only this comment says so. A line under the re-pinning procedure indocs/dependencies.mdwould put it in front of whoever edits thatFROM.@ -0,0 +37,4 @@# forks enabled and open registration, so the trigger is reachable by# anyone; secrets are withheld from fork runs, but the runner that holds# them on the next push to main is not.if: >-Right answer, and checking
forks_disabledand the registration form rather than reasoning about it is what settled it — I could only offer a conditional.One thing to confirm rather than assume, in the other direction: if
github.event.pull_request.head.repo.full_namedoes not populate on this Forgejo, this expression is false and the job skips on every pull request. That fails safe, but it silently removes the check you just added, and a skipped job does not look like a problem.This pull request is same-repository, so its own run answers it — green means the gate works, skipped means it does not. Worth a glance before merging rather than after.
@ -0,0 +41,4 @@* asserting it is current. Reporting the oldest keeps the issue open until* every site has moved, which is when it is actually true.*/export interface Dependency extends Declaration {whereis inherited here, maintained byfold(at.where = d.where), and read by nothing —main.tsandissues.tsboth go throughsites.A field that looks authoritative and actually holds only the oldest site's line. Either drop it, or stop extending
Declarationso the type says what aDependencynow is: a set of sites plus the oldest version among them.The
Declaration/Dependencysplit itself is a real improvement — the old type was doing both jobs and that is part of how the two-versions bug hid.@ -0,0 +240,4 @@`${dep.kind}:${dep.name}:${shapeKey(dep.version)}`;/** strictly behind, among tags of one shape */function behind(a: string, b: string): boolean {behindis strictly-less-than, which is right — but it meansfold's tie-break is decided by input order whenevercomparereturns 0:Which version lands in the issue title, in the marker's
from, and insettledthen depends on the orderDeno.readDirhappens to yield. Latent — no such pair exists in this repository today — but it is nondeterminism sitting next to the identity.Same fix as
split(): once equal-by-numscounts as equal, there is no tie to break arbitrarily, because the two are genuinely the same release.@ -0,0 +258,4 @@* `Dependency`. One key holding two versions is what let a run close an issue* because one of two sites had been upgraded.*/export function fold(decls: Declaration[]): Dependency[] {This is the fix, and folding at the oldest is the part that makes it right rather than merely different.
I suggested folding or reporting the disagreement and you did both, but the oldest-version choice is yours and it is what makes the issue stay true while the repository catches up — which is what the closed-issue rule depends on. Folding at the newest would have been the obvious reading of "one issue per thing" and would have preserved the bug in a form that was harder to see.
Verified the sequence: partial bump keeps the issue open, complete bump resolves it.
@ -0,0 +276,4 @@}/** the versions this dependency is declared at, when its sites disagree */export function split(dep: Dependency): Declaration[] | null {Minor, and the same fix as the one on
behindbelow. This decides with a stringSet, butversion.tssays oncompare: "these are RELEASES,v6andv6.0.0are the same one."So the disagreement warning — which is the most valuable sentence this tool produces, and the reason I suggested it — fires on a distinction the tool elsewhere denies.
Compare parsed
nums, falling back to string equality only whenparseVersionreturns null. That is exactly whatbehind()already does two functions up; the rule just did not make it into here.@ -0,0 +88,4 @@// one of these issues and bumping only one of the places. Said out loud// here as well as in the issue, because a dry run is where it gets read.for (const dep of deps) {const sites = split(dep);Putting the disagreement in the run log as well as the issue is the right call — a dry run is where this actually gets read, and it is the one thing here nothing else in the repository watches for.
It is also the code that surfaces the
split()bug: until that compares numerically, this warns aboutv6versusv6.0.0, whichcomparesays is the same release.@ -0,0 +139,4 @@// read labels is an ordinary configuration, and aborting on it would// lose the whole run to a cosmetic field.console.warn(` ! could not read labels (${(e as Error).message}); filing without`,This is the one I most expected to be argued with rather than fixed, since the failure is hypothetical. Warning and filing without the label is exactly the rule the rest of the loop follows, and the doc comment on
Forge.labelnow describes what the code does instead of what it intended.Approving at
ab305fc.Bookkeeping first, since it is my fault you were looking for this: the third pass went up as a
COMMENTreview (#issuecomment-247), and Forgejo renders those as loose comments with no verdict — so it neither showed as a review nor cleared the re-request you filed at 14:26. The detail is all in that one; this is the verdict that was missing from it.Approved on the substance, not on fatigue. Both blockers from the second pass are dead, verified by running rather than reading: 48 tests pass,
deno lintclean over 10 files,deno fmt --checkclean over 14, dry run exits 0 with all seven dependencies, andgrep -rln "sha256:" .forgejo dockernow returnsdocker/Dockerfilealone.Two of the fixes are better than what I asked for:
Answering #3 with
forks_disabledand the registration form, instead of arguing about it, is what settled that one. And your reasoning for deferring #6 is better than the comment it answers — an unwatched declaration is a finding, findings belong in the tracker, and a second exit code is the wrong shape.One thing to do, not blocking.
split()compares versions as strings whileversion.tssays oncomparethat "v6andv6.0.0are the same one", so the disagreement warning fires on a distinction the tool elsewhere denies — andfold's tie-break then falls toreadDirorder. One change fixes both: compare parsednums, fall back to strings only whenparseVersionreturns null, exactly asbehind()already does. Latent today, since no such pair exists in this repository. Follow-up is fine.Two to glance at rather than fix. Confirm this pull request's own run went green rather than skipped — if
github.event.pull_request.head.repo.full_namedoes not populate here, the newif:gate fails safe by skipping every pull request, which silently removes the check. AndDependency.whereis now dead: maintained byfold, read by nothing.Still yours to call: the write path has not touched a real tracker. You have asked twice about the scratch-repository rehearsal; that decision is with @julian, not with the review.
Good change. The pins were right from the first commit and the checker took three rounds to become worth trusting — which is the correct order for those two things to have happened in.