fix(dep-check): an image built here is not a dependency a registry answers for #41

Merged
julian merged 4 commits from fix/issue-25-dep-check-local-image into main 2026-08-21 23:20:23 +02:00
Owner

Closes #25.

The noise

$ deno run --allow-read --allow-net --allow-env tools/dep-check/main.ts --dry-run
checking 8 dependencies
  ? opsdeck rig-base: registry answered HTTP 401
  = actions/checkout v7.0.1: up to date
  ...

docker/dev-rig/Dockerfile:10 is FROM opsdeck:rig-base, and no registry has that image — docker/dev-rig/up.sh:57 builds it out of docker/Dockerfile and tags it locally. Discovery walks the tree rather than reading a hardcoded list of paths, deliberately, so it finds the line, imageDependency splits it like any other reference, and the unqualified name resolves against Docker Hub where library/opsdeck does not exist and anonymous auth answers 401. The tag carries no version either, so a lookup that succeeded would still have nothing to compare.

It never made the run red — the exit rule is failed || unwatched.length || unreachable === reports.length — so this was never breakage. What it cost is the property the tool is built around: a run that finds nothing writes nothing, so anything it does print is worth reading. A line that prints forever and can never be resolved is a line that gets skipped, and a genuine registry outage on a real dependency prints the same shape.

Which option

The issue lists four and prescribes none. This is option 1, the marker.

  • 2, an ignore list — rejected in the issue itself: an entry that outlives its Dockerfile silences a real dependency later.
  • 3, report as unwatched — reads correctly, but unwatched.length takes the run red, so it trades permanent noise for permanent failure unless the bucket is first split into "cannot be watched, deliberately" and "should be watched, isn't". Bigger change to the exit-code rule than this line is worth.
  • 4, a build arg — one line, but it works by side effect of a rule written for something else ($ means operator indirection) and makes the base implicit to whoever reads the Dockerfile.

There is also a fifth the issue does not list: never ask a registry about a tag parseVersion cannot read — the 401 disappears with no marker at all, and debian:bookworm-slim stops paying a wasted round trip every run. It is not a drop-in: settled (main.ts in survey) needs the complete tag list to close an issue whose tag was later changed to an unversioned one (issues.ts:118-127, and resolvedComment says so in as many words), so the lookup cannot be skipped unconditionally. Named here so it does not read as overlooked.

Option 1 is closest to the existing convention: discovery already reads a trailing # v7.0.1 comment to keep a SHA-pinned action answerable.

The marker

# dep-check: local — up.sh builds this, no registry has it
FROM opsdeck:rig-base

Read above a FROM or a COPY --from=; that line then stops being a declaration at all — not reported, not counted, not red.

Its own line, never trailing. Docker has no inline comments — # inside an instruction is an argument, so FROM x # dep-check: local fails the build with FROM requires either one or three arguments. The parser deliberately does not read the marker in that position either, so the form it documents is the form that builds. (Confirmed word-for-word by review, on a machine that has docker.)

It reaches exactly one instruction — at any number of comment lines' distance. A blank line or any other instruction ends it; further comment lines do not, which is the shape the real file has (marker, then the sentence explaining it, then the FROM). That reach is wider than "the line below", and a skip used to leave no trace anywhere — the drift the review flagged: the day opsdeck:rig-base becomes a published image, whoever edits the FROM also has to spot a paragraph several comment lines up, and missing it would unwatch a real dependency silently. So discovery now carries every ref the marker silences out in a third bucket, skipped, and a dry run prints each one:

  - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked

Not counted, never red — and not by-hand-only. An earlier revision of this description (and two sentences in the diff) claimed the line was "silent in CI"; that was false, and review caught it. deps.yml runs --dry-run on every same-repository pull request, so the pull request that edits a marked FROM is exactly the run whose CI log shows this line move or go missing — including the drift case above, and a marker quietly un-marked by an ARG slipped above a parameterised FROM, the likeliest real edit to defeat it. The weekly and on-demand runs both take the write path, without --dry-run, so neither ever prints it — said that way around deliberately, with no claim about which runs are silent, because a fork's pull request skips the job entirely and prints nothing too. (A previous revision claimed the weekly run was "the one that stays silent"; review caught the exclusivity.) The comment in main.ts and the paragraph in docs/dependencies.md now say exactly that.

A mistyped marker leaves the line watched. The safe direction: a typo costs a noisy line, the other direction silences a real dependency. The inverse edge is documented too: a mid-sentence mention of the marker in a comment is not read, but a comment that begins with the marker text is a marker — the LOCAL_MARKER docstring now says a Dockerfile comment documenting the convention must not open with it.

Marking a line local orphans any issue already open about it. resolve only fires for dependencies still being discovered, and a marked line is no longer discovered — so an open issue about it would sit asserting an upgrade nobody watches for. No live impact here (rig-base always came back with error set, so no issue was ever filed), but the docs now say: close such an issue by hand, the checker never will.

Stage names still register. The marker is read below the AS name, so a marked FROM x AS build still declares build and a later COPY --from=build is not mistaken for an image.

Dockerfiles only. Neither workflow reader looks for it: workflowImagesFrom does not, so a container: image: cannot be marked local, and actionsFrom does not, so neither can a uses: line. Nothing needs it today; the docstring now names both readers, and a test holds the container: half true.

After

$ deno run --allow-read --allow-net --allow-env tools/dep-check/main.ts --dry-run
  - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked
checking 7 dependencies
  = actions/checkout v7.0.1: up to date
  = debian bookworm-slim: no version in that tag, nothing to compare
  = denoland/deno 2.9.5: up to date
  = denoland/deno bin-2.9.5: up to date
  = docker/build-push-action v7.3.0: up to date
  = docker/login-action v4.6.0: up to date
  + docker/setup-buildx-action v4.2.0: v4.3.0 available, filing an issue
dry run: nothing was written

Exit 0, nothing unreachable. (The + on setup-buildx-action is real upstream movement since the first run of this branch, not this diff — review saw the same.)

Tests

Seven marker tests in tools/dep-check/discover_test.ts, with skipped assertions threaded through: the marked FROM and COPY --from= land in skipped with their file:line and nowhere counted; a marked line still names its stage; a marked untagged FROM is skipped, not reported; the marker reaches exactly one instruction (blank line, other instruction, and a following comment line that must not end it) with the ARG-between case additionally asserting skipped stays empty — the watched direction; a mistyped marker leaves the line watched; the inline form is not read; and workflowImagesFrom never reads the marker at all.

Of the original six, four fail with the source change stashed — the review measured four where this description previously claimed three. The mistyped-marker and inline tests pass either way; they are guards against a future loosening of LOCAL_MARKER, not detectors of this change, and are kept for that. The skipped assertions do not run against the old source at all — there is no skipped field to read.

Gates, re-run after the fourth (comment- and doc-only) commit: deno task check, deno lint tools/dep-check/, deno fmt --check, deno test --allow-read tools/dep-check/ (62 passed), and the --dry-run above against the real tree.

docs/dependencies.md gains the marker under "What it will not tell you", including the dry-run skip line, what its movement or absence means, and which runs print it — plus a bullet for # syntax=docker/dockerfile:1, an image reference on a mutable tag living on a comment line that discovery never reads, and the orphaned-issue clause above. The sample fence now carries the output's real two-space indent; the checking 7 dependencies line sits inside the fence because deno fmt strips a fence's common leading indent, so the flush line is what anchors the relative one.

Closes #25. ## The noise ``` $ deno run --allow-read --allow-net --allow-env tools/dep-check/main.ts --dry-run checking 8 dependencies ? opsdeck rig-base: registry answered HTTP 401 = actions/checkout v7.0.1: up to date ... ``` `docker/dev-rig/Dockerfile:10` is `FROM opsdeck:rig-base`, and no registry has that image — `docker/dev-rig/up.sh:57` builds it out of `docker/Dockerfile` and tags it locally. Discovery walks the tree rather than reading a hardcoded list of paths, deliberately, so it finds the line, `imageDependency` splits it like any other reference, and the unqualified name resolves against Docker Hub where `library/opsdeck` does not exist and anonymous auth answers 401. The tag carries no version either, so a lookup that *succeeded* would still have nothing to compare. It never made the run red — the exit rule is `failed || unwatched.length || unreachable === reports.length` — so this was never breakage. What it cost is the property the tool is built around: a run that finds nothing writes nothing, so anything it does print is worth reading. A line that prints forever and can never be resolved is a line that gets skipped, and a genuine registry outage on a real dependency prints the same shape. ## Which option The issue lists four and prescribes none. This is **option 1**, the marker. - **2, an ignore list** — rejected in the issue itself: an entry that outlives its Dockerfile silences a real dependency later. - **3, report as `unwatched`** — reads correctly, but `unwatched.length` takes the run red, so it trades permanent noise for permanent failure unless the bucket is first split into "cannot be watched, deliberately" and "should be watched, isn't". Bigger change to the exit-code rule than this line is worth. - **4, a build arg** — one line, but it works by side effect of a rule written for something else (`$` means operator indirection) and makes the base implicit to whoever reads the Dockerfile. There is also a **fifth** the issue does not list: never ask a registry about a tag `parseVersion` cannot read — the 401 disappears with no marker at all, and `debian:bookworm-slim` stops paying a wasted round trip every run. It is not a drop-in: `settled` (`main.ts` in `survey`) needs the complete tag list to close an issue whose tag was later changed to an unversioned one (`issues.ts:118-127`, and `resolvedComment` says so in as many words), so the lookup cannot be skipped unconditionally. Named here so it does not read as overlooked. Option 1 is closest to the existing convention: discovery already reads a trailing `# v7.0.1` comment to keep a SHA-pinned action answerable. ## The marker ``` # dep-check: local — up.sh builds this, no registry has it FROM opsdeck:rig-base ``` Read above a `FROM` or a `COPY --from=`; that line then stops being a declaration at all — not reported, not counted, not red. **Its own line, never trailing.** Docker has no inline comments — `#` inside an instruction is an argument, so `FROM x # dep-check: local` fails the build with `FROM requires either one or three arguments`. The parser deliberately does not read the marker in that position either, so the form it documents is the form that builds. (Confirmed word-for-word by review, on a machine that has docker.) **It reaches exactly one instruction — at any number of comment lines' distance.** A blank line or any other instruction ends it; further comment lines do not, which is the shape the real file has (marker, then the sentence explaining it, then the `FROM`). That reach is wider than "the line below", and a skip used to leave no trace anywhere — the drift the review flagged: the day `opsdeck:rig-base` becomes a published image, whoever edits the `FROM` also has to spot a paragraph several comment lines up, and missing it would unwatch a real dependency silently. So discovery now carries every ref the marker silences out in a third bucket, `skipped`, and a dry run prints each one: ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked ``` Not counted, never red — and **not by-hand-only**. An earlier revision of this description (and two sentences in the diff) claimed the line was "silent in CI"; that was false, and review caught it. `deps.yml` runs `--dry-run` on every same-repository pull request, so the pull request that edits a marked `FROM` is exactly the run whose CI log shows this line move or go missing — including the drift case above, and a marker quietly un-marked by an `ARG` slipped above a parameterised `FROM`, the likeliest real edit to defeat it. The weekly and on-demand runs both take the write path, without `--dry-run`, so neither ever prints it — said that way around deliberately, with no claim about which runs are silent, because a fork's pull request skips the job entirely and prints nothing too. (A previous revision claimed the weekly run was "the one that stays silent"; review caught the exclusivity.) The comment in `main.ts` and the paragraph in `docs/dependencies.md` now say exactly that. **A mistyped marker leaves the line watched.** The safe direction: a typo costs a noisy line, the other direction silences a real dependency. The inverse edge is documented too: a mid-sentence mention of the marker in a comment is not read, but a comment that *begins* with the marker text is a marker — the `LOCAL_MARKER` docstring now says a Dockerfile comment documenting the convention must not open with it. **Marking a line local orphans any issue already open about it.** `resolve` only fires for dependencies still being discovered, and a marked line is no longer discovered — so an open issue about it would sit asserting an upgrade nobody watches for. No live impact here (rig-base always came back with `error` set, so no issue was ever filed), but the docs now say: close such an issue by hand, the checker never will. **Stage names still register.** The marker is read *below* the `AS` name, so a marked `FROM x AS build` still declares `build` and a later `COPY --from=build` is not mistaken for an image. **Dockerfiles only.** Neither workflow reader looks for it: `workflowImagesFrom` does not, so a `container: image:` cannot be marked local, and `actionsFrom` does not, so neither can a `uses:` line. Nothing needs it today; the docstring now names both readers, and a test holds the `container:` half true. ## After ``` $ deno run --allow-read --allow-net --allow-env tools/dep-check/main.ts --dry-run - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked checking 7 dependencies = actions/checkout v7.0.1: up to date = debian bookworm-slim: no version in that tag, nothing to compare = denoland/deno 2.9.5: up to date = denoland/deno bin-2.9.5: up to date = docker/build-push-action v7.3.0: up to date = docker/login-action v4.6.0: up to date + docker/setup-buildx-action v4.2.0: v4.3.0 available, filing an issue dry run: nothing was written ``` Exit 0, nothing unreachable. (The `+` on `setup-buildx-action` is real upstream movement since the first run of this branch, not this diff — review saw the same.) ## Tests Seven marker tests in `tools/dep-check/discover_test.ts`, with `skipped` assertions threaded through: the marked `FROM` and `COPY --from=` land in `skipped` with their file:line and nowhere counted; a marked line still names its stage; a marked untagged `FROM` is skipped, not reported; the marker reaches exactly one instruction (blank line, other instruction, and a following comment line that must *not* end it) with the `ARG`-between case additionally asserting `skipped` stays empty — the watched direction; a mistyped marker leaves the line watched; the inline form is not read; and `workflowImagesFrom` never reads the marker at all. Of the original six, **four** fail with the source change stashed — the review measured four where this description previously claimed three. The mistyped-marker and inline tests pass either way; they are guards against a future loosening of `LOCAL_MARKER`, not detectors of this change, and are kept for that. The `skipped` assertions do not run against the old source at all — there is no `skipped` field to read. Gates, re-run after the fourth (comment- and doc-only) commit: `deno task check`, `deno lint tools/dep-check/`, `deno fmt --check`, `deno test --allow-read tools/dep-check/` (62 passed), and the `--dry-run` above against the real tree. `docs/dependencies.md` gains the marker under "What it will not tell you", including the dry-run skip line, what its movement or absence means, and which runs print it — plus a bullet for `# syntax=docker/dockerfile:1`, an image reference on a mutable tag living on a comment line that discovery never reads, and the orphaned-issue clause above. The sample fence now carries the output's real two-space indent; the `checking 7 dependencies` line sits inside the fence because `deno fmt` strips a fence's *common* leading indent, so the flush line is what anchors the relative one.
fix(dep-check): an image built here is not a dependency a registry answers for
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m45s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m0s
469bfd85de
`docker/dev-rig/Dockerfile` is `FROM opsdeck:rig-base`, and that image comes
from no registry — `docker/dev-rig/up.sh` builds it out of `docker/Dockerfile`
and tags it locally. Discovery walks the tree rather than reading a list of
paths, deliberately, so it finds the line, resolves the unqualified name
against Docker Hub, and prints

    ? opsdeck rig-base: registry answered HTTP 401

on every run. It never made the run red, and it can never be resolved either.
What it cost is the property the tool is built around: a run that finds
nothing writes nothing, so anything it does print is worth reading. A line
that prints forever is a line that gets skipped, and a real registry outage
prints the same shape.

Nothing in `FROM opsdeck:rig-base` distinguishes a locally built image from a
registry one, so the line has to say so itself:

    # dep-check: local — up.sh builds this, no registry has it
    FROM opsdeck:rig-base

The marker is read above a `FROM` or a `COPY --from=`, and that line then
stops being a declaration at all — not reported, not counted toward
`unwatched`, which would have traded permanent noise for permanent red.

Its own line, never trailing: docker has no inline comments, so
`FROM x  # dep-check: local` would fail the build with "FROM requires either
one or three arguments".

It reaches exactly one instruction — a blank line or any other instruction
ends it, further comment lines do not — because an entry that outlives what it
was written for is the ignore-list failure this form exists to avoid. A
mistyped marker leaves the line watched, which is the safe direction.

`--dry-run` now reports 7 dependencies with nothing unreachable.

Closes #25

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thisilike requested changes 2026-08-16 22:29:58 +02:00
Dismissed
thisilike left a comment

Re-reviewed the whole diff, re-ran every gate rather than taking the description's word for them, and reproduced the one claim it flags as unverified. The parser is correct — I tried to break it and could not. Two things to fix before merge, both small, and one of them is a sentence this PR adds that is not true.

Verified independently, not taken on trust

  • deno test --allow-read tools/dep-check/ → 61 passed. deno lint tools/dep-check/, deno fmt --check, deno check tools/dep-check/ → clean.
  • Discovery against the real tree: 7 deps, 0 unwatched, and opsdeck rig-base is the only thing lost — actions/checkout v7.0.1 (3 sites), docker/login-action, docker/setup-buildx-action, docker/build-push-action, denoland/deno 2.9.5, debian bookworm-slim, denoland/deno bin-2.9.5 are all still found at the same lines.
  • The inline claim is exactly right, and this machine had docker to prove it. FROM debian:bookworm-slim # dep-check: local:
    ERROR: failed to build: failed to solve: dockerfile parse error on line 1: FROM requires either one or three arguments
    
    Word for word what the docstring says. That "worth a second pair of eyes" is settled — the form documented is the form that builds.
  • With discover.ts reverted to HEAD~1, four of the six new tests fail, not three.
  • Stressed the state machine well past what the tests cover; all correct: FROM --platform=… (marker still reaches it), CRLF, tabs inside the marker, FROM scratch consuming it, marker at EOF, a heredoc body (RUN <<EOF / # dep-check: local / EOF — the EOF line consumes it, so the next FROM stays watched), a RUN … \ continuation the same way, # dep-check: locally (the \b holds), a prose comment mentioning the marker mid-sentence (# we dropped the dep-check: local marker here — not read, because the regex is effectively anchored to the #), and a marked FROM followed by a real COPY --from=denoland/deno:bin-2.9.5, which is still watched.
  • Nothing else reads these FROM lines: .forgejo/deno.sh:31 and tools/ci_pins_test.ts:38 both anchor on ^FROM *denoland/deno: in docker/Dockerfile, so a comment above dev-rig's FROM reaches neither.

1. "it cannot outlive the line it was written for" is not true (blocking — one sentence, one line of output)

tools/dep-check/discover.ts:181 and docs/dependencies.md:134 both make that claim, and the whole argument for this form over option 2 rests on it. But comment lines do not end the marker's reach — deliberate, with a test enshrining it — so the real bound is "one instruction, at any number of comment lines' distance". Ran it:

# dep-check: local
# a
# b
# c
# d
FROM debian:bookworm-slim

deps: [].

The real Dockerfile is the shape that makes this reachable: the marker is a two-line paragraph at the tail of a nine-line block about something else entirely. The day opsdeck:rig-base becomes a published image — dev-rig pulling a real base rather than building one — whoever edits line 12 also has to notice and delete a paragraph six lines up that reads as part of the isolation comment. Miss it, and a versioned, published dependency is unwatched forever, silently. That is the exact failure this tool exists to catch, and right now the marker is the only thing in the repository that can cause it.

The reach rule should stay as it is — the two-line marker paragraph in the real file needs it. What is missing is that the skip leaves no trace anywhere: checking 7 dependencies and nothing else. There is no output, under any flag, that says a line was skipped, so this drift can never be noticed from a run.

Cheapest fix that keeps the property the whole design rests on ("a run that finds nothing writes nothing, so anything it prints is worth reading"): carry the skipped refs out of dockerfileImagesFrom in a third bucket and print them only under --dry-run, where a human is already reading:

  - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked

Not counted, not red, invisible in CI. It answers "did my marker land where I meant it", and it makes §2 self-correcting. Plus: reword the two sentences to say "one instruction" rather than "the line it was written for".

2. An ARG under the marker silently un-marks it (low — same fix)

# dep-check: local
ARG X=1
FROM opsdeck:rig-base

deps: ["opsdeck:rig-base"]. The 401 is back. Correct per the documented rule and it fails in the safe direction, but ARG-before-FROM is the idiom for a parameterised base, so this is the likeliest way the marker quietly stops working in practice. discover_test.ts:221 already constructs this exact case; it just is not surfaced anywhere a person would see it. The --dry-run line above is what makes it noticeable.

3. Two of the six tests do not detect this change (nit)

discover_test.ts:234 (mistyped) and :248 (inline) pass identically against HEAD~1. Both are real guards against a future loosening — if LOCAL_MARKER were ever tested against non-comment lines, the inline one catches it — so keep them. The description should just not imply all six are change-detecting.

4. workflowImagesFrom has no marker (nit)

container: image: opsdeck:rig-base in a workflow would produce the same permanent 401 with no way out, and a # line above it is not read. Nothing needs it today. One clause in the LOCAL_MARKER docstring saying "Dockerfiles only" stops the next reader assuming otherwise.

On the option list — no action, but name it

There is a fifth option the description does not consider: do not ask a registry about a tag parseVersion cannot read. opsdeck:rig-base is unversioned, pickLatest returns null, and the answer is discarded — the 401 is paid for nothing, and debian:bookworm-slim pays the same wasted round trip on every run.

It is not a drop-in replacement, and I checked that before raising it: settled (main.ts:64) needs the tag list to close an issue whose tag was later changed to an unversioned one (issues.ts:118-127, and resolvedComment says so in as many words), so the lookup cannot be skipped unconditionally. Worth one line in the description so the next reader does not re-derive that and conclude it was overlooked.

Everything else holds: the marker read below the AS name, unwatched correctly not inheriting the skip, the exit rule unaffected (unreachable === reports.length behaved the same before and after, since rig-base was always one of the unreachable), deno fmt fence style matching the rest of the doc, one clean conventional-commit, and no drift on main in any touched path.

Re-reviewed the whole diff, re-ran every gate rather than taking the description's word for them, and reproduced the one claim it flags as unverified. The parser is correct — I tried to break it and could not. Two things to fix before merge, both small, and one of them is a sentence this PR adds that is not true. ## Verified independently, not taken on trust - `deno test --allow-read tools/dep-check/` → 61 passed. `deno lint tools/dep-check/`, `deno fmt --check`, `deno check tools/dep-check/` → clean. - Discovery against the real tree: 7 deps, 0 unwatched, and `opsdeck rig-base` is the *only* thing lost — `actions/checkout v7.0.1` (3 sites), `docker/login-action`, `docker/setup-buildx-action`, `docker/build-push-action`, `denoland/deno 2.9.5`, `debian bookworm-slim`, `denoland/deno bin-2.9.5` are all still found at the same lines. - **The inline claim is exactly right, and this machine had docker to prove it.** `FROM debian:bookworm-slim # dep-check: local`: ``` ERROR: failed to build: failed to solve: dockerfile parse error on line 1: FROM requires either one or three arguments ``` Word for word what the docstring says. That "worth a second pair of eyes" is settled — the form documented is the form that builds. - With `discover.ts` reverted to `HEAD~1`, **four** of the six new tests fail, not three. - Stressed the state machine well past what the tests cover; all correct: `FROM --platform=… ` (marker still reaches it), CRLF, tabs inside the marker, `FROM scratch` consuming it, marker at EOF, a heredoc body (`RUN <<EOF` / `# dep-check: local` / `EOF` — the `EOF` line consumes it, so the next `FROM` stays watched), a `RUN … \` continuation the same way, `# dep-check: locally` (the `\b` holds), a prose comment mentioning the marker mid-sentence (`# we dropped the dep-check: local marker here` — not read, because the regex is effectively anchored to the `#`), and a marked `FROM` followed by a real `COPY --from=denoland/deno:bin-2.9.5`, which is still watched. - Nothing else reads these `FROM` lines: `.forgejo/deno.sh:31` and `tools/ci_pins_test.ts:38` both anchor on `^FROM *denoland/deno:` in `docker/Dockerfile`, so a comment above dev-rig's `FROM` reaches neither. ## 1. "it cannot outlive the line it was written for" is not true (blocking — one sentence, one line of output) `tools/dep-check/discover.ts:181` and `docs/dependencies.md:134` both make that claim, and the whole argument for this form over option 2 rests on it. But comment lines do not end the marker's reach — deliberate, with a test enshrining it — so the real bound is "one **instruction**, at any number of comment lines' distance". Ran it: ``` # dep-check: local # a # b # c # d FROM debian:bookworm-slim ``` → `deps: []`. The real Dockerfile is the shape that makes this reachable: the marker is a two-line paragraph at the tail of a nine-line block about something else entirely. The day `opsdeck:rig-base` becomes a published image — dev-rig pulling a real base rather than building one — whoever edits line 12 also has to notice and delete a paragraph six lines up that reads as part of the isolation comment. Miss it, and a versioned, published dependency is unwatched forever, silently. That is the exact failure this tool exists to catch, and right now the marker is the only thing in the repository that can cause it. The reach rule should stay as it is — the two-line marker paragraph in the real file needs it. What is missing is that the skip leaves **no trace anywhere**: `checking 7 dependencies` and nothing else. There is no output, under any flag, that says a line was skipped, so this drift can never be noticed from a run. Cheapest fix that keeps the property the whole design rests on ("a run that finds nothing writes nothing, so anything it prints is worth reading"): carry the skipped refs out of `dockerfileImagesFrom` in a third bucket and print them **only under `--dry-run`**, where a human is already reading: ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked ``` Not counted, not red, invisible in CI. It answers "did my marker land where I meant it", and it makes §2 self-correcting. Plus: reword the two sentences to say "one instruction" rather than "the line it was written for". ## 2. An `ARG` under the marker silently un-marks it (low — same fix) ``` # dep-check: local ARG X=1 FROM opsdeck:rig-base ``` → `deps: ["opsdeck:rig-base"]`. The 401 is back. Correct per the documented rule and it fails in the safe direction, but `ARG`-before-`FROM` is *the* idiom for a parameterised base, so this is the likeliest way the marker quietly stops working in practice. `discover_test.ts:221` already constructs this exact case; it just is not surfaced anywhere a person would see it. The `--dry-run` line above is what makes it noticeable. ## 3. Two of the six tests do not detect this change (nit) `discover_test.ts:234` (mistyped) and `:248` (inline) pass identically against `HEAD~1`. Both are real guards against a future loosening — if `LOCAL_MARKER` were ever tested against non-comment lines, the inline one catches it — so keep them. The description should just not imply all six are change-detecting. ## 4. `workflowImagesFrom` has no marker (nit) `container: image: opsdeck:rig-base` in a workflow would produce the same permanent 401 with no way out, and a `#` line above it is not read. Nothing needs it today. One clause in the `LOCAL_MARKER` docstring saying "Dockerfiles only" stops the next reader assuming otherwise. ## On the option list — no action, but name it There is a fifth option the description does not consider: do not ask a registry about a tag `parseVersion` cannot read. `opsdeck:rig-base` is unversioned, `pickLatest` returns null, and the answer is discarded — the 401 is paid for nothing, and `debian:bookworm-slim` pays the same wasted round trip on every run. It is **not** a drop-in replacement, and I checked that before raising it: `settled` (`main.ts:64`) needs the tag list to close an issue whose tag was later changed to an unversioned one (`issues.ts:118-127`, and `resolvedComment` says so in as many words), so the lookup cannot be skipped unconditionally. Worth one line in the description so the next reader does not re-derive that and conclude it was overlooked. Everything else holds: the marker read below the `AS` name, `unwatched` correctly not inheriting the skip, the exit rule unaffected (`unreachable === reports.length` behaved the same before and after, since rig-base was always one of the unreachable), `deno fmt` fence style matching the rest of the doc, one clean conventional-commit, and no drift on main in any touched path.
@ -116,0 +131,4 @@
`FROM x # dep-check: local` fails the build with
`FROM requires either one or three arguments`. It reaches exactly one
instruction — a blank line or any other instruction ends it — because an entry
that outlives what it was written for is the ignore-list failure this form
Owner

Same overclaim as discover.ts:181: "outlives what it was written for" is only true at instruction granularity. The paragraph already says "a blank line or any other instruction ends it" one clause earlier, which is correct — it is this clause that promises more than the code delivers. Say "one instruction" and drop the "cannot outlive" framing, or state plainly that comment lines do not end it (which the reader needs anyway, since the shipped marker relies on exactly that).

Same overclaim as `discover.ts:181`: "outlives what it was written for" is only true at instruction granularity. The paragraph already says "a blank line or any other instruction ends it" one clause earlier, which is correct — it is this clause that promises more than the code delivers. Say "one instruction" and drop the "cannot outlive" framing, or state plainly that comment lines do not end it (which the reader needs anyway, since the shipped marker relies on exactly that).
@ -167,0 +178,4 @@
*
* A comment directly above the line rather than an ignore list somewhere
* else, for the same reason an action's tag lives next to its SHA: the next
* reader of the Dockerfile sees it, and it cannot outlive the line it was
Owner

This is the sentence the whole form rests on, and it is not accurate. A comment line does not end the marker's reach (line 219 below, and there is a test asserting it), so the bound is "one instruction", not "the line it was written for" — and any number of comment lines can sit between.

Reproduced: marker, four unrelated comment lines, then FROM debian:bookworm-slimdeps: []. In the real docker/dev-rig/Dockerfile the marker is a two-line paragraph at the tail of a nine-line block about the nested engine, so the day that base becomes a published image, the person editing the FROM has to also spot a paragraph six lines up. Miss it and a real dependency is unwatched forever, with nothing in the output saying so.

Reword to "one instruction", and see the review body for the --dry-run line that makes the drift visible.

This is the sentence the whole form rests on, and it is not accurate. A comment line does not end the marker's reach (line 219 below, and there is a test asserting it), so the bound is "one **instruction**", not "the line it was written for" — and any number of comment lines can sit between. Reproduced: marker, four unrelated comment lines, then `FROM debian:bookworm-slim` → `deps: []`. In the real `docker/dev-rig/Dockerfile` the marker is a two-line paragraph at the tail of a nine-line block about the nested engine, so the day that base becomes a published image, the person editing the `FROM` has to also spot a paragraph six lines up. Miss it and a real dependency is unwatched forever, with nothing in the output saying so. Reword to "one instruction", and see the review body for the `--dry-run` line that makes the drift visible.
@ -184,0 +216,4 @@
marked = false;
return;
}
if (/^\s*#/.test(line)) {
Owner

The blank-line and instruction resets are what the comment above claims, but this branch is the third case and it is the one that is unbounded: comments neither set nor clear the marker unless they are the marker, so the reach is arbitrarily long across a comment block.

That is the right behaviour for the real file's two-line marker paragraph — the problem is only that a skip is invisible afterwards. Suggest returning the skipped refs in a third bucket on Scan so main.ts can print them under --dry-run:

  - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked

Not counted, not red, silent in CI.

The blank-line and instruction resets are what the comment above claims, but this branch is the third case and it is the one that is unbounded: comments neither set nor clear the marker unless they *are* the marker, so the reach is arbitrarily long across a comment block. That is the right behaviour for the real file's two-line marker paragraph — the problem is only that a skip is invisible afterwards. Suggest returning the skipped refs in a third bucket on `Scan` so `main.ts` can print them under `--dry-run`: ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked ``` Not counted, not red, silent in CI.
@ -139,0 +218,4 @@
["debian"],
);
// and an instruction that is not a FROM consumes it too
Owner

This case is worth more than a line in a combined test. ARG-before-FROM is the standard idiom for a parameterised base, so this is the likeliest way the marker stops working in a real edit:

# dep-check: local
ARG X=1
FROM opsdeck:rig-base

deps: ["opsdeck:rig-base"], the 401 back with no indication why the marker stopped applying. Fails in the safe direction, agreed — it is the diagnosability that is missing, which the --dry-run skip line in the review body fixes.

This case is worth more than a line in a combined test. `ARG`-before-`FROM` is the standard idiom for a parameterised base, so this is the likeliest way the marker stops working in a real edit: ``` # dep-check: local ARG X=1 FROM opsdeck:rig-base ``` → `deps: ["opsdeck:rig-base"]`, the 401 back with no indication why the marker stopped applying. Fails in the safe direction, agreed — it is the *diagnosability* that is missing, which the `--dry-run` skip line in the review body fixes.
fix(dep-check): a skip the marker makes now leaves a trace a dry run prints
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m33s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 56s
0ae2301d11
Review follow-ups for #41. The marker's reach is one instruction at any
number of comment lines' distance — wider than "the line it was written
for", which the prose claimed and the code never delivered — and a skip
left no output anywhere, so a marker that drifted onto the wrong
instruction, or one an ARG slipped above the FROM quietly un-marked,
could never be noticed from a run.

- discover: every ref the marker silences is carried out of
  dockerfileImagesFrom in a third bucket, `skipped`, with its file:line.
  Not a dependency, not unwatched: never counted, never red.
- main: printed only under --dry-run, where a human is already reading:
    - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked
  CI output and the exit rule are unchanged.
- discover.ts and docs/dependencies.md stop claiming the marker "cannot
  outlive the line it was written for"; the stated bound is now the next
  instruction, with comment lines explicitly not ending it.
- LOCAL_MARKER documented as a Dockerfile convention only, and a test
  holds that workflowImagesFrom never reads it. The ARG-between case now
  also asserts skipped stays empty — the watched direction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

All four points addressed in 0ae2301, plus the description rewritten where it overclaimed. Point by point:

1. The invisible skip (blocking). Taken exactly as suggested: dockerfileImagesFrom carries every ref the marker silences out in a third bucket, skipped (ref + file:line), merged through discover(), and main.ts prints each one only under --dry-run:

  - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked

Not counted in checking N dependencies, no effect on the exit rule, nothing printed in CI mode. Verified against the real tree — the line above is the actual output, exit 0. Both overclaiming sentences are gone: the LOCAL_MARKER docstring and docs/dependencies.md now state the bound as one instruction at any number of comment lines' distance, say explicitly that comment lines do not end the reach, and point at the dry-run line as the place drift becomes visible.

2. ARG un-marking. Covered by the same dry-run line, as you said — the skip line goes missing and the 401 returns, which is now a visible diff between two dry runs instead of nothing. The test for that case (discover_test.ts, the reach test) now also asserts skipped stays empty, with a comment naming it the likeliest real edit to defeat the marker.

3. Test claims. The description now says four of the original six fail with the source change stashed (your measurement was right, mine was wrong), and names the mistyped and inline tests as guards against a future loosening of LOCAL_MARKER rather than change detectors.

4. Marker scope. LOCAL_MARKER docstring gained the clause: read in Dockerfiles only, workflowImagesFrom does not look for it. A new test holds that sentence true — a # dep-check: local above a container: image: changes nothing and produces no skip entry.

The fifth option is now named in the description, with the reason it is not a drop-in (settled needs the complete tag list for the unversioned-tag close path) so nobody re-derives it.

Gates re-run after the change: deno task check, deno lint tools/dep-check/, deno fmt --check, deno test --allow-read tools/dep-check/ (62 passed), and the --dry-run against the real tree quoted in the description.

All four points addressed in 0ae2301, plus the description rewritten where it overclaimed. Point by point: **1. The invisible skip (blocking).** Taken exactly as suggested: `dockerfileImagesFrom` carries every ref the marker silences out in a third bucket, `skipped` (ref + file:line), merged through `discover()`, and `main.ts` prints each one only under `--dry-run`: ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked ``` Not counted in `checking N dependencies`, no effect on the exit rule, nothing printed in CI mode. Verified against the real tree — the line above is the actual output, exit 0. Both overclaiming sentences are gone: the `LOCAL_MARKER` docstring and `docs/dependencies.md` now state the bound as one **instruction** at any number of comment lines' distance, say explicitly that comment lines do not end the reach, and point at the dry-run line as the place drift becomes visible. **2. `ARG` un-marking.** Covered by the same dry-run line, as you said — the skip line goes missing and the 401 returns, which is now a visible diff between two dry runs instead of nothing. The test for that case (`discover_test.ts`, the reach test) now also asserts `skipped` stays empty, with a comment naming it the likeliest real edit to defeat the marker. **3. Test claims.** The description now says four of the original six fail with the source change stashed (your measurement was right, mine was wrong), and names the mistyped and inline tests as guards against a future loosening of `LOCAL_MARKER` rather than change detectors. **4. Marker scope.** `LOCAL_MARKER` docstring gained the clause: read in Dockerfiles only, `workflowImagesFrom` does not look for it. A new test holds that sentence true — a `# dep-check: local` above a `container: image:` changes nothing and produces no skip entry. **The fifth option** is now named in the description, with the reason it is not a drop-in (`settled` needs the complete tag list for the unversioned-tag close path) so nobody re-derives it. Gates re-run after the change: `deno task check`, `deno lint tools/dep-check/`, `deno fmt --check`, `deno test --allow-read tools/dep-check/` (62 passed), and the `--dry-run` against the real tree quoted in the description.
thisilike requested changes 2026-08-21 16:52:19 +02:00
Dismissed
thisilike left a comment

Re-reviewed the second commit and re-read the whole diff from scratch, taking nothing from the description or from my own previous pass on trust. Every gate re-run, every number re-measured, the parser attacked again with cases the tests do not cover, and the one claim the last round called "worth a second pair of eyes" re-proven on a machine that has docker.

§1 and §2 from the last review are genuinely fixed, and the fix is the right shape: the reach rule stayed, the skip now leaves a trace, and the ARG case is the one the test suite names explicitly. One thing is blocking, and it is the same class of defect as last time — two sentences this commit adds that are not true, one of which contradicts a sentence already in the same file.

Re-measured, not taken on trust

  • deno test --allow-read tools/dep-check/62 passed, as claimed. deno lint tools/dep-check/ (11 files), deno fmt --check (314 files), deno check tools/dep-check/main.ts, and the full deno task check → all clean.
  • Real tree, real network: --dry-run exits 0 with the skip line exactly as described and no 401 anywhere.
      - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked
    checking 7 dependencies
    
    docker/dev-rig/Dockerfile:12 is correct after the two added comment lines. All seven other deps still found. (docker/setup-buildx-action now reports v4.3.0 available — real upstream movement since you ran it, nothing to do with this diff.)
  • "Four of the original six fail with the source stashed" is right. I reverted discover.ts to fb29e2c and ran with --no-check: the four you name fail, mistyped and inline pass. Of the seven at HEAD, five fail — the new workflow test fails too, but only because found.skipped is undefined rather than [], which is an artifact of the missing field, not detection of the behaviour. Your framing is accurate.
  • The inline claim, re-proven here. FROM debian:bookworm-slim # dep-check: localparse error on line 1: FROM requires either one or three arguments. The documented form builds clean (sha256:7cd206d2…). Word for word what the docstring says.
  • up.sh:57 does build and tag opsdeck:rig-base out of docker/Dockerfile, and :71 saves it — the doc's account of why no registry has it is exact.
  • Every Scan in the file is constructed through empty(), so no literal was missed when skipped became required; main.ts:78 is the only consumer of discover() in the tree.
  • Branch is behind main (merge base fb29e2c, main at 2844b43) but nothing on main has touched tools/dep-check, docs/dependencies.md or docker/dev-rig since. No drift.
  • Attacked the state machine again, past the last round's set. All correct, all in the safe direction: ## dep-check: local, tabs inside the marker, CRLF, marker at EOF, marker over FROM scratch, marker over a stage-name FROM, marker over COPY with no --from= (the next real COPY --from= stays watched), marker over COPY --from=<stage>, FROM x:1 # c # dep-check: local (not read), marker → blank → comment → FROM (ends at the blank, as documented), and a marked FROM … AS rig followed by COPY --from=rig (stage still registered). I could not construct a case that silences a dependency the author did not mark.

1. "CI stays silent" is false — CI is the one place this line always prints (blocking, two sentences)

docs/dependencies.md:148: "under --dry-run only, where a human is already reading; CI stays silent and the run stays green."
tools/dep-check/main.ts:104: "Dry run only: this is the answer to 'did my marker land where I meant it', and CI has nobody asking."

.forgejo/workflows/deps.yml:55-59:

      - name: Check dependencies (dry run)
        if: github.event_name == 'pull_request'
        run: >
          sh .forgejo/deno.sh run --allow-read --allow-net --allow-env
          tools/dep-check/main.ts --dry-run

Every same-repository pull request runs --dry-run. docs/dependencies.md:50-51 — a hundred lines above the paragraph this PR adds, already on main — says so in as many words: "Every pull request runs exactly that, so the workflow, the container, discovery and every registry read are exercised on the way in." So the file now contradicts itself, and the comment that justifies the dryRun gate justifies it with the inverse of what the gate actually does.

The behaviour is better than advertised, which is why this is text-only: the drift scenario is somebody editing that FROM in a pull request, and the pull request's own CI log is precisely where the skip line lands. The weekly scheduled run is the silent one. But "CI stays silent" tells the next reader not to look in the one place it is guaranteed to appear, and "CI has nobody asking" is a premise somebody could reasonably act on — by dropping the gate, or by adding the line to the scheduled run to fix a problem that does not exist. Reword both to say what happens: printed on every pull-request run, never on the weekly one, never counted, never red.

2. Marking something local orphans any issue already open about it (low, one sentence of docs)

main.ts:163 iterates reports, which comes from survey(deps), which comes from discover().deps. A dependency that disappears from discovery is never reached by decide, so resolve never fires for it and an open issue about it stays open forever, asserting an upgrade that nobody is watching for any more.

No live impact today, and I checked before raising it: opsdeck rig-base always came back with error set, so main.ts:164 skipped it and no issue was ever filed. But this marker is the first mechanism in the tool that deliberately removes a discovered dependency while its declaration stays in the tree — previously a dep only vanished by being deleted or bumped. Worth one clause under the marker in docs/dependencies.md: if the line you are marking already has an open dependency issue, close it yourself, because the checker will not.

3. The sample output lost its indentation (nit)

docs/dependencies.md:142. Real output is - opsdeck:rig-base …; the fence holds - opsdeck:rig-base … (the two spaces went to the list indent). Every other sample in the file is verbatim, and rendered without them this one reads as a markdown bullet that wandered into a code block.

4. A Dockerfile comment that starts with the marker text silences the next instruction (nit, informational)

Verified both directions, because the last review's phrasing here was loose:

  • # we removed the dep-check: local markerdeps: ["debian"]. Safe — the regex needs the # immediately before the phrase, so a mid-sentence mention is not read. That conclusion was right.
  • # dep-check: local is the convention heredeps: [], skipped: ["debian:1"]. Read as a marker.

Needs someone to document the convention inside a Dockerfile, starting the sentence with the marker itself. Not worth code, and \b already holds the useful line (locale no, local-only yes). Recording it so the next reader does not have to re-derive it.

5. Out of scope, but adjacent: # syntax= is an unwatched image (nit)

docker/Dockerfile:1 is # syntax=docker/dockerfile:1 — a mutable-tag image reference that discovery never reads, because it lives on a comment line. Predates this PR and the new code steps over it correctly (comment branch, does not set marked). Mentioning it only because this diff is the one that adds a "what it will not tell you" entry about images, and this is another one.

Everything else holds

Marker read below the AS name so a marked stage still registers; isStage and scratch checked before the skip so neither lands in skipped; unwatched correctly not inheriting the skip; - does not collide with the existing ! ? = v + symbols; console.log rather than console.warn, correctly, since a skip is not a warning; the exit rule untouched and verified exit 0 against the real tree; issues.ts:118-127 and main.ts:64 are exactly where the description's fifth-option argument says they are, and that argument is sound — the lookup genuinely cannot be skipped unconditionally; two clean conventional commits; .forgejo/deno.sh:31 and tools/ci_pins_test.ts:38 both anchor on docker/Dockerfile, so the dev-rig comment reaches neither.

Fix §1 and merge. §2 is one sentence and §3 is two spaces; I would take them in the same push, but neither is what is holding this.

Re-reviewed the second commit and re-read the whole diff from scratch, taking nothing from the description or from my own previous pass on trust. Every gate re-run, every number re-measured, the parser attacked again with cases the tests do not cover, and the one claim the last round called "worth a second pair of eyes" re-proven on a machine that has docker. §1 and §2 from the last review are genuinely fixed, and the fix is the right shape: the reach rule stayed, the skip now leaves a trace, and the `ARG` case is the one the test suite names explicitly. One thing is blocking, and it is the same class of defect as last time — two sentences this commit adds that are not true, one of which contradicts a sentence already in the same file. ## Re-measured, not taken on trust - `deno test --allow-read tools/dep-check/` → **62 passed**, as claimed. `deno lint tools/dep-check/` (11 files), `deno fmt --check` (314 files), `deno check tools/dep-check/main.ts`, and the **full `deno task check`** → all clean. - Real tree, real network: `--dry-run` exits **0** with the skip line exactly as described and no 401 anywhere. ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked checking 7 dependencies ``` `docker/dev-rig/Dockerfile:12` is correct after the two added comment lines. All seven other deps still found. (`docker/setup-buildx-action` now reports `v4.3.0 available` — real upstream movement since you ran it, nothing to do with this diff.) - **"Four of the original six fail with the source stashed" is right.** I reverted `discover.ts` to `fb29e2c` and ran with `--no-check`: the four you name fail, mistyped and inline pass. Of the *seven* at HEAD, five fail — the new workflow test fails too, but only because `found.skipped` is `undefined` rather than `[]`, which is an artifact of the missing field, not detection of the behaviour. Your framing is accurate. - **The inline claim, re-proven here.** `FROM debian:bookworm-slim # dep-check: local` → `parse error on line 1: FROM requires either one or three arguments`. The documented form builds clean (`sha256:7cd206d2…`). Word for word what the docstring says. - `up.sh:57` does build and tag `opsdeck:rig-base` out of `docker/Dockerfile`, and `:71` saves it — the doc's account of why no registry has it is exact. - Every `Scan` in the file is constructed through `empty()`, so no literal was missed when `skipped` became required; `main.ts:78` is the only consumer of `discover()` in the tree. - Branch is behind main (merge base `fb29e2c`, main at `2844b43`) but nothing on main has touched `tools/dep-check`, `docs/dependencies.md` or `docker/dev-rig` since. No drift. - Attacked the state machine again, past the last round's set. All correct, all in the safe direction: `## dep-check: local`, tabs inside the marker, CRLF, marker at EOF, marker over `FROM scratch`, marker over a stage-name `FROM`, marker over `COPY` with no `--from=` (the *next* real `COPY --from=` stays watched), marker over `COPY --from=<stage>`, `FROM x:1 # c # dep-check: local` (not read), marker → blank → comment → `FROM` (ends at the blank, as documented), and a marked `FROM … AS rig` followed by `COPY --from=rig` (stage still registered). I could not construct a case that silences a dependency the author did not mark. ## 1. "CI stays silent" is false — CI is the one place this line always prints (blocking, two sentences) `docs/dependencies.md:148`: "under `--dry-run` only, where a human is already reading; CI stays silent and the run stays green." `tools/dep-check/main.ts:104`: "Dry run only: this is the answer to 'did my marker land where I meant it', and **CI has nobody asking**." `.forgejo/workflows/deps.yml:55-59`: ```yaml - name: Check dependencies (dry run) if: github.event_name == 'pull_request' run: > sh .forgejo/deno.sh run --allow-read --allow-net --allow-env tools/dep-check/main.ts --dry-run ``` Every same-repository pull request runs `--dry-run`. `docs/dependencies.md:50-51` — a hundred lines above the paragraph this PR adds, already on main — says so in as many words: "Every pull request runs exactly that, so the workflow, the container, discovery and every registry read are exercised on the way in." So the file now contradicts itself, and the comment that justifies the `dryRun` gate justifies it with the inverse of what the gate actually does. The *behaviour* is better than advertised, which is why this is text-only: the drift scenario is somebody editing that `FROM` in a pull request, and the pull request's own CI log is precisely where the skip line lands. The weekly scheduled run is the silent one. But "CI stays silent" tells the next reader not to look in the one place it is guaranteed to appear, and "CI has nobody asking" is a premise somebody could reasonably act on — by dropping the gate, or by adding the line to the scheduled run to fix a problem that does not exist. Reword both to say what happens: printed on every pull-request run, never on the weekly one, never counted, never red. ## 2. Marking something local orphans any issue already open about it (low, one sentence of docs) `main.ts:163` iterates `reports`, which comes from `survey(deps)`, which comes from `discover().deps`. A dependency that disappears from discovery is never reached by `decide`, so `resolve` never fires for it and an open issue about it stays open forever, asserting an upgrade that nobody is watching for any more. No live impact today, and I checked before raising it: `opsdeck rig-base` always came back with `error` set, so `main.ts:164` skipped it and no issue was ever filed. But this marker is the first mechanism in the tool that deliberately removes a *discovered* dependency while its declaration stays in the tree — previously a dep only vanished by being deleted or bumped. Worth one clause under the marker in `docs/dependencies.md`: if the line you are marking already has an open dependency issue, close it yourself, because the checker will not. ## 3. The sample output lost its indentation (nit) `docs/dependencies.md:142`. Real output is ` - opsdeck:rig-base …`; the fence holds `- opsdeck:rig-base …` (the two spaces went to the list indent). Every other sample in the file is verbatim, and rendered without them this one reads as a markdown bullet that wandered into a code block. ## 4. A Dockerfile comment that *starts* with the marker text silences the next instruction (nit, informational) Verified both directions, because the last review's phrasing here was loose: - `# we removed the dep-check: local marker` → `deps: ["debian"]`. Safe — the regex needs the `#` immediately before the phrase, so a mid-sentence mention is not read. That conclusion was right. - `# dep-check: local is the convention here` → `deps: []`, `skipped: ["debian:1"]`. Read as a marker. Needs someone to document the convention inside a Dockerfile, starting the sentence with the marker itself. Not worth code, and `\b` already holds the useful line (`locale` no, `local-only` yes). Recording it so the next reader does not have to re-derive it. ## 5. Out of scope, but adjacent: `# syntax=` is an unwatched image (nit) `docker/Dockerfile:1` is `# syntax=docker/dockerfile:1` — a mutable-tag image reference that discovery never reads, because it lives on a comment line. Predates this PR and the new code steps over it correctly (comment branch, does not set `marked`). Mentioning it only because this diff is the one that adds a "what it will not tell you" entry about images, and this is another one. ## Everything else holds Marker read below the `AS` name so a marked stage still registers; `isStage` and `scratch` checked before the skip so neither lands in `skipped`; `unwatched` correctly not inheriting the skip; `-` does not collide with the existing `!` `?` `=` `v` `+` symbols; `console.log` rather than `console.warn`, correctly, since a skip is not a warning; the exit rule untouched and verified exit 0 against the real tree; `issues.ts:118-127` and `main.ts:64` are exactly where the description's fifth-option argument says they are, and that argument is sound — the lookup genuinely cannot be skipped unconditionally; two clean conventional commits; `.forgejo/deno.sh:31` and `tools/ci_pins_test.ts:38` both anchor on `docker/Dockerfile`, so the dev-rig comment reaches neither. Fix §1 and merge. §2 is one sentence and §3 is two spaces; I would take them in the same push, but neither is what is holding this.
@ -116,0 +145,4 @@
That is the answer to "did my marker land where I meant it". A marker that
drifted onto the wrong instruction, or one that an `ARG` slipped in above the
`FROM` quietly un-marked, shows up as this line moving or going missing —
under `--dry-run` only, where a human is already reading; CI stays silent and
Owner

Blocking, §1. .forgejo/workflows/deps.yml:55-59 runs main.ts --dry-run on every same-repository pull request, so CI does not stay silent — it is the one place this line always prints. And line 50-51 of this same file already says so: "Every pull request runs exactly that."

The run does stay green; that half is right. Reword to: printed on every pull-request run, never on the weekly one, never counted, never red.

Blocking, §1. `.forgejo/workflows/deps.yml:55-59` runs `main.ts --dry-run` on every same-repository pull request, so CI does not stay silent — it is the one place this line always prints. And line 50-51 of this same file already says so: "Every pull request runs exactly that." The run does stay green; that half is right. Reword to: printed on every pull-request run, never on the weekly one, never counted, never red.
@ -99,0 +101,4 @@
// anywhere, a marker that drifted onto the wrong instruction, or one an
// ARG un-marked, could never be noticed from a run. Dry run only: this is
// the answer to "did my marker land where I meant it", and CI has nobody
// asking.
Owner

Blocking, §1. "CI has nobody asking" is the inverse of what happens: deps.yml:56 gates the --dry-run invocation on github.event_name == 'pull_request', so this loop runs on every same-repo PR and never on the Monday schedule.

The gate is still the right call — this is exactly where a reviewer of a FROM edit is looking — but the reason written here is wrong, and it is the kind of premise the next person acts on by removing the gate.

Blocking, §1. "CI has nobody asking" is the inverse of what happens: `deps.yml:56` gates the `--dry-run` invocation on `github.event_name == 'pull_request'`, so this loop runs on every same-repo PR and never on the Monday schedule. The gate is still the right call — this is exactly where a reviewer of a `FROM` edit is looking — but the reason written here is wrong, and it is the kind of premise the next person acts on by removing the gate.
docs(dep-check): the skip line prints on every pull request, and now the text says so
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m21s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
796c3d4ef3
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Reworked in 796c3d4 — doc/comment-only, no behavior change. Point by point:

§1 (blocking) — "CI stays silent" was false. Both sentences rewritten to say what actually happens. docs/dependencies.md now reads: every same-repository pull request runs --dry-run, so the pull request that edits a marked FROM is exactly the run whose log shows the skip line move or go missing; the weekly scheduled run never prints it, and no run counts it or goes red over it. The dryRun-gate comment in main.ts now justifies the gate with the same truth — dry run is not a by-hand-only mode, deps.yml runs it on every same-repo PR, and the scheduled run is the silent one. The self-contradiction with the paragraph at docs/dependencies.md:49-51 is gone; that paragraph was the accurate one and stands unchanged.

§2 — orphaned issues. Added under the marker in docs/dependencies.md: issues are resolved only for dependencies still being discovered, and a marked line is no longer discovered — so an open issue about the line being marked must be closed by hand, the checker never will. Framed as "what the marker does not touch" so it reads as a property of the mechanism, not trivia.

§3 — fence indentation. Restored, with a wrinkle worth knowing: deno fmt (dprint) strips a fence's common leading indent inside a list item, so a fence holding only the skip line loses the two spaces on the next format pass — which is presumably how they vanished in the first place. The fence now also holds the checking 7 dependencies line, flush like the real output; that anchors the relative indent and fmt leaves it alone (verified by running deno fmt on the file and on isolated test cases). Sample is now more faithful anyway — skip line, then the count it was excluded from.

§4 — marker-prefix comments. Documented in the LOCAL_MARKER docstring: a mid-sentence mention is not read (the # must stand directly before the phrase), but a comment that begins with the marker text is a marker, so a Dockerfile comment documenting the convention must not open with it. Your two test cases are the ones the sentence encodes.

§5 — # syntax=. Took it despite being out of scope — it is exactly the class the new doc section exists for. New bullet under "What it will not tell you": docker/Dockerfile opens with # syntax=docker/dockerfile:1, an image reference on a mutable tag living on a comment line, which discovery never reads.

Description updated: the "silent in CI" claim replaced (and owned as previously false), the After block refreshed to today's real output — including + docker/setup-buildx-action v4.2.0: v4.3.0 available, the upstream movement you also saw, and the dry run: nothing was written trailer — plus the §2/§4/§5 additions and the fmt-indent mechanics.

Verification, re-run at 796c3d4 on this machine: deno test --allow-read tools/dep-check/ 62 passed; deno lint tools/dep-check/ clean (11 files); deno fmt --check clean (314 files); full deno task check exit 0; real-tree --dry-run exit 0 with the skip line at docker/dev-rig/Dockerfile:12 and no 401 anywhere.

Reworked in 796c3d4 — doc/comment-only, no behavior change. Point by point: **§1 (blocking) — "CI stays silent" was false.** Both sentences rewritten to say what actually happens. `docs/dependencies.md` now reads: every same-repository pull request runs `--dry-run`, so the pull request that edits a marked `FROM` is exactly the run whose log shows the skip line move or go missing; the weekly scheduled run never prints it, and no run counts it or goes red over it. The `dryRun`-gate comment in `main.ts` now justifies the gate with the same truth — dry run is not a by-hand-only mode, `deps.yml` runs it on every same-repo PR, and the scheduled run is the silent one. The self-contradiction with the paragraph at `docs/dependencies.md:49-51` is gone; that paragraph was the accurate one and stands unchanged. **§2 — orphaned issues.** Added under the marker in `docs/dependencies.md`: issues are resolved only for dependencies still being discovered, and a marked line is no longer discovered — so an open issue about the line being marked must be closed by hand, the checker never will. Framed as "what the marker does not touch" so it reads as a property of the mechanism, not trivia. **§3 — fence indentation.** Restored, with a wrinkle worth knowing: `deno fmt` (dprint) strips a fence's *common* leading indent inside a list item, so a fence holding only the skip line loses the two spaces on the next format pass — which is presumably how they vanished in the first place. The fence now also holds the `checking 7 dependencies` line, flush like the real output; that anchors the relative indent and fmt leaves it alone (verified by running `deno fmt` on the file and on isolated test cases). Sample is now more faithful anyway — skip line, then the count it was excluded from. **§4 — marker-prefix comments.** Documented in the `LOCAL_MARKER` docstring: a mid-sentence mention is not read (the `#` must stand directly before the phrase), but a comment that *begins* with the marker text is a marker, so a Dockerfile comment documenting the convention must not open with it. Your two test cases are the ones the sentence encodes. **§5 — `# syntax=`.** Took it despite being out of scope — it is exactly the class the new doc section exists for. New bullet under "What it will not tell you": `docker/Dockerfile` opens with `# syntax=docker/dockerfile:1`, an image reference on a mutable tag living on a comment line, which discovery never reads. **Description** updated: the "silent in CI" claim replaced (and owned as previously false), the After block refreshed to today's real output — including `+ docker/setup-buildx-action v4.2.0: v4.3.0 available`, the upstream movement you also saw, and the `dry run: nothing was written` trailer — plus the §2/§4/§5 additions and the fmt-indent mechanics. **Verification, re-run at 796c3d4 on this machine:** `deno test --allow-read tools/dep-check/` 62 passed; `deno lint tools/dep-check/` clean (11 files); `deno fmt --check` clean (314 files); full `deno task check` exit 0; real-tree `--dry-run` exit 0 with the skip line at `docker/dev-rig/Dockerfile:12` and no 401 anywhere.
thisilike requested changes 2026-08-21 20:31:14 +02:00
Dismissed
thisilike left a comment

Third pass, from a detached worktree at 796c3d4, taking nothing from the description or from my own two previous reviews on trust. Every gate re-run, every number in the description re-measured, the real --dry-run re-executed against the live network, the stash test reproduced, and the parser attacked with twenty-five cases neither previous round tried. I could not break it.

Both blockers from the last round are properly fixed, and fixed in the right places rather than papered over. What is left is one sentence this commit adds that is false in the same way the last two rounds' blockers were false — an exclusivity claim where there are two cases — and it is a one-word fix.

Re-measured, not taken on trust

  • deno test --allow-read tools/dep-check/62 passed, exactly as claimed. deno lint tools/dep-check/ (11 files), deno fmt --check (314 files), deno check tools/dep-check/main.ts, and the full deno task check → all clean.
  • Real tree, real network. Byte-for-byte what the description's "After" block shows, exit 0:
      - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked
    checking 7 dependencies
      = actions/checkout v7.0.1: up to date
      = debian bookworm-slim: no version in that tag, nothing to compare
      = denoland/deno 2.9.5: up to date
      = denoland/deno bin-2.9.5: up to date
      = docker/build-push-action v7.3.0: up to date
      = docker/login-action v4.6.0: up to date
      + docker/setup-buildx-action v4.2.0: v4.3.0 available, filing an issue
    dry run: nothing was written
    
    No 401 anywhere. docker/dev-rig/Dockerfile:12 is correct after the two added comment lines. The + is real upstream movement, not this diff.
  • "Four of the original six fail with the source stashed" is right. I reverted discover.ts to fb29e2c and ran --no-check: of the seven at HEAD, five fail — the four you name, plus the workflow test, which fails only on found.skipped being undefined rather than []. That is the missing field, not detection of behaviour. Your framing is accurate to the letter.
  • §1 from last round is genuinely fixed, not reworded around. grep for "CI stays silent", "nobody asking", "cannot outlive", "silent in CI" over tools/ and docs/ → nothing left. docs/dependencies.md:152-153 and main.ts:102-104 now both say --dry-run runs on every same-repository pull request, and both are correct: deps.yml:55-59 gates the step on github.event_name == 'pull_request' and deps.yml:40-42 gates the job on head.repo.full_name == github.repository. The "same-repository" qualifier is load-bearing and you got it right — a fork's pull request skips the job entirely and prints nothing.
  • §2 is fixed — the orphaned-issue clause at docs/dependencies.md:156-159, and I re-verified the mechanism: main.ts:165 iterates reportssurvey(deps)discover().deps, so a marked line is never reached by decide and resolve can never fire for it.
  • §3 is fixed, at the byte level. cat -A on the fence: the skip line carries four leading spaces and checking 7 dependencies carries two, inside a two-space list item — so rendered it is - opsdeck:… and a flush checking 7 dependencies, which is the real output verbatim. deno fmt --check is happy with it.
  • §4 is fixed and the docstring is accurate in both directions. Re-proved both: # we dropped the dep-check: local marker here → watched; # dep-check: local is the convention here → skipped. discover.ts:212-215 says exactly that.
  • §5 is fixed — the # syntax= bullet at docs/dependencies.md:116-118.
  • 796c3d4 is comment-only in the two .ts files it touches, so "the final (doc-only) commit" is fair.
  • Branch is behind main (merge base fb29e2c, main now cc65b8c), but git log fb29e2c..origin/main -- tools/dep-check docs/dependencies.md docker/dev-rig .forgejo/workflows/deps.yml is empty. No drift.
  • empty() is still the only construction of a Scan in the tree (discover.ts:95, used at :444), so nothing was missed when skipped became required. main.ts:78 is the only consumer of discover() anywhere in the workspace.
  • Only two Dockerfiles exist (docker/Dockerfile, docker/dev-rig/Dockerfile) and opsdeck:rig-base is declared exactly once. entrypoint.sh:284 re-tags it from a shell script, which discovery never reads. tools/ci_pins_test.ts:37 and .forgejo/deno.sh both anchor on docker/Dockerfile, so the dev-rig comment reaches neither. No other file in the tree names a locally-built image on a FROM.

Twenty-five new attacks on the state machine — all in the safe direction

Beyond the two previous rounds' sets: # dep-check : local (space before the colon → watched, safe), #dep-check:local (read), ## dep-check: local (read), FROM --platform=linux/amd64 (marker still reaches it), marker over a FROM with a trailing # note (skipped), ONBUILD FROM (unchanged from before the diff — never watched either way), a malformed FROM x AS a b (dropped, as it was before), marker over RUN then FROM (watched), marker over COPY with no --from= then a real COPY --from= (watched), COPY --from=0 (nothing, nothing was watched), marker over FROM scratch (next FROM watched), a bare # line not ending the reach, indented marker over indented FROM, marker at EOF with no trailing newline, marker over a digest-bearing COPY --from=, two stacked markers, a marker below a FROM, CRLF, marker → ARG → blank → FROM (watched, nothing in skipped), locally (not read), local-only (read), and a marker over a FROM <stage> reference. Not one case silences a declaration the author did not mark.

1. main.ts:105-106 — "the one that stays silent" is false (blocking, one word)

// this line move or go missing. The weekly scheduled run is the one that
// stays silent.

deps.yml:10-20 has three triggers, not two: schedule, workflow_dispatch, pull_request. The dry-run step is gated on github.event_name == 'pull_request' and the write step on !=, so workflow_dispatch is silent too — and it is the write path, against the live tracker. There are two silent runs, and this sentence says there is one.

Small in itself, and docs/dependencies.md:153-154 is merely narrow rather than false ("the weekly scheduled run never prints it" is true, just not exclusive). But docs/dependencies.md:17 — already on main, a hundred lines above — says the workflow runs "every Monday at 06:00 UTC, and on demand from the Actions tab", so the file names the second trigger and this comment does not. The premise somebody could act on: "the scheduled run is the silent one" reads as "an on-demand run would show me whether my marker landed", and it will not — it will file issues instead. docs/dependencies.md:40-44 does point at the local dry-run as the by-hand check, which limits the damage, which is why this is one clause and not a redesign.

the weekly and on-demand runs are the ones that stay silent — and the same clause in the doc while you are in there. That is the whole fix.

I am blocking on one word deliberately, and it is the last thing: this is the third consecutive round whose only blocker is a sentence the diff adds that is not quite true, and in a tree where the comments are the design record, a false exclusivity claim in main.ts is a defect in the same terms the last two were. Push it and this merges on sight.

2. "Dockerfiles only" names one of the two readers (nit)

discover.ts:210-211 says workflowImagesFrom does not look for the marker, so a container: image: cannot be marked local. True, and the test at discover_test.ts:268 holds it — but actionsFrom cannot read it either, so a uses: line is equally unmarkable. "Read in Dockerfiles only" already governs; one clause naming both readers would stop the next person testing it against a uses:.

3. A marker inside a \ continuation is read (nit, informational)

RUN echo a \
# dep-check: local
FROM debian:1

skipped: ["debian:1"]. It needs the author to type the marker, so it silences nothing unmarked, and real docker folds that FROM into the RUN anyway — so the parser was already wrong about that line before this diff. Recording it so the next reader does not re-derive it. No code.

On the noise property — observation, no action

The description opens with the right argument: "a line that prints forever and can never be resolved is a line that gets skipped." The skip line now prints on every same-repository pull request, forever, and can never be resolved either. It is a much weaker version of the problem — green, - rather than ?, and it is the answer to a real question — but it is traded rather than restored, and the last round is the one that asked for it. Worth knowing which property was bought with which.

Everything else holds

Marker read below the AS name so a marked stage still registers, with the COPY --from=rig test proving it; isStage and scratch checked before the skip so neither lands in skipped; unwatched correctly not inheriting the skip, and the untagged-marked-FROM test closing the door on the marker trading a 401 for a red run; marked scoped per file, so no leak between Dockerfiles; the exit rule untouched and exit 0 verified against the real tree; - colliding with none of ! ? = v ^ + x; console.log not console.warn, correctly, since a skip is not a warning; the fifth-option argument sound — main.ts:64 and issues.ts really do need the tag list for settled, so the lookup cannot be skipped unconditionally; three clean conventional commits; and no drift on main in any touched path.

Third pass, from a detached worktree at `796c3d4`, taking nothing from the description or from my own two previous reviews on trust. Every gate re-run, every number in the description re-measured, the real `--dry-run` re-executed against the live network, the stash test reproduced, and the parser attacked with twenty-five cases neither previous round tried. I could not break it. Both blockers from the last round are properly fixed, and fixed in the right places rather than papered over. What is left is one sentence this commit adds that is false in the same way the last two rounds' blockers were false — an exclusivity claim where there are two cases — and it is a one-word fix. ## Re-measured, not taken on trust - `deno test --allow-read tools/dep-check/` → **62 passed**, exactly as claimed. `deno lint tools/dep-check/` (11 files), `deno fmt --check` (**314 files**), `deno check tools/dep-check/main.ts`, and the **full `deno task check`** → all clean. - Real tree, real network. Byte-for-byte what the description's "After" block shows, **exit 0**: ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked checking 7 dependencies = actions/checkout v7.0.1: up to date = debian bookworm-slim: no version in that tag, nothing to compare = denoland/deno 2.9.5: up to date = denoland/deno bin-2.9.5: up to date = docker/build-push-action v7.3.0: up to date = docker/login-action v4.6.0: up to date + docker/setup-buildx-action v4.2.0: v4.3.0 available, filing an issue dry run: nothing was written ``` No 401 anywhere. `docker/dev-rig/Dockerfile:12` is correct after the two added comment lines. The `+` is real upstream movement, not this diff. - **"Four of the original six fail with the source stashed" is right.** I reverted `discover.ts` to `fb29e2c` and ran `--no-check`: of the seven at HEAD, **five** fail — the four you name, plus the workflow test, which fails only on `found.skipped` being `undefined` rather than `[]`. That is the missing field, not detection of behaviour. Your framing is accurate to the letter. - **§1 from last round is genuinely fixed, not reworded around.** `grep` for "CI stays silent", "nobody asking", "cannot outlive", "silent in CI" over `tools/` and `docs/` → nothing left. `docs/dependencies.md:152-153` and `main.ts:102-104` now both say `--dry-run` runs on every same-repository pull request, and both are correct: `deps.yml:55-59` gates the step on `github.event_name == 'pull_request'` and `deps.yml:40-42` gates the job on `head.repo.full_name == github.repository`. The "same-repository" qualifier is load-bearing and you got it right — a fork's pull request skips the job entirely and prints nothing. - **§2 is fixed** — the orphaned-issue clause at `docs/dependencies.md:156-159`, and I re-verified the mechanism: `main.ts:165` iterates `reports` ← `survey(deps)` ← `discover().deps`, so a marked line is never reached by `decide` and `resolve` can never fire for it. - **§3 is fixed, at the byte level.** `cat -A` on the fence: the skip line carries four leading spaces and `checking 7 dependencies` carries two, inside a two-space list item — so rendered it is ` - opsdeck:…` and a flush `checking 7 dependencies`, which is the real output verbatim. `deno fmt --check` is happy with it. - **§4 is fixed and the docstring is accurate in both directions.** Re-proved both: `# we dropped the dep-check: local marker here` → watched; `# dep-check: local is the convention here` → skipped. `discover.ts:212-215` says exactly that. - **§5 is fixed** — the `# syntax=` bullet at `docs/dependencies.md:116-118`. - `796c3d4` is comment-only in the two `.ts` files it touches, so "the final (doc-only) commit" is fair. - Branch is behind main (merge base `fb29e2c`, main now `cc65b8c`), but `git log fb29e2c..origin/main -- tools/dep-check docs/dependencies.md docker/dev-rig .forgejo/workflows/deps.yml` is **empty**. No drift. - `empty()` is still the only construction of a `Scan` in the tree (`discover.ts:95`, used at `:444`), so nothing was missed when `skipped` became required. `main.ts:78` is the only consumer of `discover()` anywhere in the workspace. - Only two Dockerfiles exist (`docker/Dockerfile`, `docker/dev-rig/Dockerfile`) and `opsdeck:rig-base` is declared exactly once. `entrypoint.sh:284` re-tags it from a shell script, which discovery never reads. `tools/ci_pins_test.ts:37` and `.forgejo/deno.sh` both anchor on `docker/Dockerfile`, so the dev-rig comment reaches neither. No other file in the tree names a locally-built image on a `FROM`. ## Twenty-five new attacks on the state machine — all in the safe direction Beyond the two previous rounds' sets: `# dep-check : local` (space before the colon → **watched**, safe), `#dep-check:local` (read), `## dep-check: local` (read), `FROM --platform=linux/amd64` (marker still reaches it), marker over a `FROM` with a trailing `# note` (skipped), `ONBUILD FROM` (unchanged from before the diff — never watched either way), a malformed `FROM x AS a b` (dropped, as it was before), marker over `RUN` then `FROM` (watched), marker over `COPY` with no `--from=` then a real `COPY --from=` (watched), `COPY --from=0` (nothing, nothing was watched), marker over `FROM scratch` (next `FROM` watched), a bare `#` line not ending the reach, indented marker over indented `FROM`, marker at EOF with no trailing newline, marker over a digest-bearing `COPY --from=`, two stacked markers, a marker *below* a `FROM`, CRLF, marker → `ARG` → blank → `FROM` (watched, nothing in `skipped`), `locally` (not read), `local-only` (read), and a marker over a `FROM <stage>` reference. **Not one case silences a declaration the author did not mark.** ## 1. `main.ts:105-106` — "the one that stays silent" is false (blocking, one word) > `// this line move or go missing. The weekly scheduled run is the one that` > `// stays silent.` `deps.yml:10-20` has three triggers, not two: `schedule`, `workflow_dispatch`, `pull_request`. The dry-run step is gated on `github.event_name == 'pull_request'` and the write step on `!=`, so **`workflow_dispatch` is silent too** — and it is the *write* path, against the live tracker. There are two silent runs, and this sentence says there is one. Small in itself, and `docs/dependencies.md:153-154` is merely narrow rather than false ("the weekly scheduled run never prints it" is true, just not exclusive). But `docs/dependencies.md:17` — already on main, a hundred lines above — says the workflow runs "every Monday at 06:00 UTC, **and on demand from the Actions tab**", so the file names the second trigger and this comment does not. The premise somebody could act on: "the scheduled run is the silent one" reads as "an on-demand run would show me whether my marker landed", and it will not — it will file issues instead. `docs/dependencies.md:40-44` does point at the local dry-run as the by-hand check, which limits the damage, which is why this is one clause and not a redesign. `the weekly and on-demand runs are the ones that stay silent` — and the same clause in the doc while you are in there. That is the whole fix. I am blocking on one word deliberately, and it is the last thing: this is the third consecutive round whose only blocker is a sentence the diff adds that is not quite true, and in a tree where the comments are the design record, a false exclusivity claim in `main.ts` is a defect in the same terms the last two were. Push it and this merges on sight. ## 2. "Dockerfiles only" names one of the two readers (nit) `discover.ts:210-211` says `workflowImagesFrom` does not look for the marker, so a `container: image:` cannot be marked local. True, and the test at `discover_test.ts:268` holds it — but `actionsFrom` cannot read it either, so a `uses:` line is equally unmarkable. "Read in Dockerfiles only" already governs; one clause naming both readers would stop the next person testing it against a `uses:`. ## 3. A marker inside a `\` continuation is read (nit, informational) ``` RUN echo a \ # dep-check: local FROM debian:1 ``` → `skipped: ["debian:1"]`. It needs the author to type the marker, so it silences nothing unmarked, and real docker folds that `FROM` into the `RUN` anyway — so the parser was already wrong about that line before this diff. Recording it so the next reader does not re-derive it. No code. ## On the noise property — observation, no action The description opens with the right argument: "a line that prints forever and can never be resolved is a line that gets skipped." The skip line now prints on every same-repository pull request, forever, and can never be resolved either. It is a much weaker version of the problem — green, `-` rather than `?`, and it is the answer to a real question — but it is traded rather than restored, and the last round is the one that asked for it. Worth knowing which property was bought with which. ## Everything else holds Marker read below the `AS` name so a marked stage still registers, with the `COPY --from=rig` test proving it; `isStage` and `scratch` checked before the skip so neither lands in `skipped`; `unwatched` correctly not inheriting the skip, and the untagged-marked-`FROM` test closing the door on the marker trading a 401 for a red run; `marked` scoped per file, so no leak between Dockerfiles; the exit rule untouched and exit 0 verified against the real tree; `-` colliding with none of `! ? = v ^ + x`; `console.log` not `console.warn`, correctly, since a skip is not a warning; the fifth-option argument sound — `main.ts:64` and `issues.ts` really do need the tag list for `settled`, so the lookup cannot be skipped unconditionally; three clean conventional commits; and no drift on main in any touched path.
docs(dep-check): the on-demand run is as silent as the weekly one, and the text says so
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m50s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 56s
309494d481
Review follow-ups for #41, round three. Comment- and doc-only.

- main.ts claimed the weekly scheduled run is "the one that stays
  silent" — false by exclusivity: deps.yml has three triggers, and
  workflow_dispatch takes the write path exactly like schedule does, so
  an on-demand run never prints the skip line either. The comment and
  docs/dependencies.md now say both runs take the write path without
  --dry-run, with no claim about being the only silent ones (a fork's
  pull request skips the job entirely and prints nothing too).
- The LOCAL_MARKER docstring said "Dockerfiles only" but named only
  workflowImagesFrom; actionsFrom cannot read the marker either, so a
  uses: line is equally unmarkable. Both readers are now named, in the
  docstring and in the doc.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Round-three items addressed in 309494d — comment- and doc-only, as the review anticipated.

§1 (blocking, the one word): main.ts no longer claims the weekly run is "the one that stays silent". It now says the weekly and on-demand runs both take the write path, without --dry-run, so neither ever prints the skip line — phrased as a positive statement about those two runs rather than an exclusivity claim, since a fork's pull request skips the job entirely and prints nothing either, and that would have been the same defect a third time. docs/dependencies.md got the matching clause: "the weekly scheduled run and an on-demand run from the Actions tab never print it — both take the write path".

§2 (nit): the LOCAL_MARKER docstring now names both workflow readers: workflowImagesFrom does not look for it, so a container: image: cannot be marked local, and actionsFrom does not, so neither can a uses: line. The parallel sentence in docs/dependencies.md says "container: image: or uses:" now too. The existing test still covers only the container: half, and the description says so rather than implying both are held.

§3 and the noise-property observation: taken as recorded — no code, per the review. The continuation-line edge and the traded property now live in the review thread, which is part of the record too.

Gates re-run at 309494d: deno task check, deno lint tools/dep-check/ (11 files), deno fmt --check (314 files), deno test --allow-read tools/dep-check/ (62 passed), and the real --dry-run — exit 0, output byte-identical to the description's "After" block. PR description updated in the same pass (the silent-runs sentence, the Dockerfiles-only paragraph, and the gates line now saying "fourth commit").

Round-three items addressed in `309494d` — comment- and doc-only, as the review anticipated. **§1 (blocking, the one word):** `main.ts` no longer claims the weekly run is "the one that stays silent". It now says the weekly and on-demand runs both take the write path, without `--dry-run`, so neither ever prints the skip line — phrased as a positive statement about those two runs rather than an exclusivity claim, since a fork's pull request skips the job entirely and prints nothing either, and that would have been the same defect a third time. `docs/dependencies.md` got the matching clause: "the weekly scheduled run and an on-demand run from the Actions tab never print it — both take the write path". **§2 (nit):** the `LOCAL_MARKER` docstring now names both workflow readers: `workflowImagesFrom` does not look for it, so a `container: image:` cannot be marked local, and `actionsFrom` does not, so neither can a `uses:` line. The parallel sentence in `docs/dependencies.md` says "`container: image:` or `uses:`" now too. The existing test still covers only the `container:` half, and the description says so rather than implying both are held. **§3 and the noise-property observation:** taken as recorded — no code, per the review. The continuation-line edge and the traded property now live in the review thread, which is part of the record too. Gates re-run at `309494d`: `deno task check`, `deno lint tools/dep-check/` (11 files), `deno fmt --check` (314 files), `deno test --allow-read tools/dep-check/` (62 passed), and the real `--dry-run` — exit 0, output byte-identical to the description's "After" block. PR description updated in the same pass (the silent-runs sentence, the Dockerfiles-only paragraph, and the gates line now saying "fourth commit").
thisilike approved these changes 2026-08-21 23:05:00 +02:00
thisilike left a comment

Fourth pass, detached worktree at 309494d. Both findings from round three are fixed, 309494d is comment- and doc-only as the message claims, and every gate re-run clean. Approving.

The two findings

§1 — the exclusivity claim is gone, and what replaced it is true. main.ts:105-106 now reads "The weekly and on-demand runs both take the write path, without --dry-run, so neither ever prints it." Checked against deps.yml rather than against the description: three triggers (schedule, workflow_dispatch, pull_request), the dry-run step gated == 'pull_request' and the write step != 'pull_request', so schedule and workflow_dispatch both land on the write path. The sentence names both and claims nothing about being the only silent runs — which is the right way around, since a fork's pull request fails the job's if: and prints nothing either. docs/dependencies.md:153-155 says the same thing in the same shape.

§2 — both readers are named, and the new half is true. discover.ts:210-212: "Neither workflow reader looks for it — workflowImagesFrom does not, so a container: image: cannot be marked local, and actionsFrom does not, so neither can a uses: line." Verified against the function rather than the docstring: actionsFrom never references LOCAL_MARKER, and I ran it — a # dep-check: local line above a uses: changes nothing, the action stays watched at v7.0.1.

One thing worth recording while you are at that clause, informational only. A marker written trailing a uses: is not ignored — actionsFrom's (?:#\s*(\S+))? reads a trailing comment as the tag a SHA is meant to be, so uses: actions/checkout@<sha> # dep-check: local yields a dependency at version dep-check:. That is the pre-existing trailing-comment rule, not this diff, and it fails in the safe direction — a nonsense lookup that gets reported, never a silenced dependency. "Cannot be marked local" is exactly the right description of it. No change wanted; recording it so the next person does not discover it by trying.

§3 was informational and needed nothing.

Re-run, not taken from the description

  • deno test --allow-read tools/dep-check/62 passed. deno lint tools/dep-check/ (11 files), deno fmt --check (314 files), and the full deno task check → clean.
  • --dry-run against the real tree and the real network: identical output to round three, exit 0, no 401.
      - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked
    checking 7 dependencies
    
  • 309494d really is comment- and doc-only: the two .ts hunks touch nothing but a docstring and a comment block, and re-running the round-three probes confirms behaviour is bit-identical — marked FROM skipped, ARG un-marks it back to watched, a mistyped marker leaves the line watched.
  • Still no drift: git log fb29e2c..origin/main over tools/dep-check, docs/dependencies.md, docker/dev-rig and .forgejo/workflows/deps.yml is empty; main is at cc65b8c.
  • Four clean conventional commits.

Everything verified across the three previous rounds still holds: the parser survived twenty-five adversarial cases in round three and none of them silences a declaration the author did not mark; the marker is read below the AS name so a marked stage still registers; isStage and scratch are checked before the skip; unwatched does not inherit the skip; the exit rule is untouched; the orphaned-issue clause and the # syntax= bullet are in the doc; and empty() is still the sole Scan constructor with main.ts the sole discover() consumer.

Ship it.

Fourth pass, detached worktree at `309494d`. Both findings from round three are fixed, `309494d` is comment- and doc-only as the message claims, and every gate re-run clean. Approving. ## The two findings **§1 — the exclusivity claim is gone, and what replaced it is true.** `main.ts:105-106` now reads "The weekly and on-demand runs both take the write path, without `--dry-run`, so neither ever prints it." Checked against `deps.yml` rather than against the description: three triggers (`schedule`, `workflow_dispatch`, `pull_request`), the dry-run step gated `== 'pull_request'` and the write step `!= 'pull_request'`, so `schedule` and `workflow_dispatch` both land on the write path. The sentence names both and claims nothing about being the only silent runs — which is the right way around, since a fork's pull request fails the job's `if:` and prints nothing either. `docs/dependencies.md:153-155` says the same thing in the same shape. **§2 — both readers are named, and the new half is true.** `discover.ts:210-212`: "Neither workflow reader looks for it — `workflowImagesFrom` does not, so a `container: image:` cannot be marked local, and `actionsFrom` does not, so neither can a `uses:` line." Verified against the function rather than the docstring: `actionsFrom` never references `LOCAL_MARKER`, and I ran it — a `# dep-check: local` line above a `uses:` changes nothing, the action stays watched at `v7.0.1`. One thing worth recording while you are at that clause, informational only. A marker written *trailing* a `uses:` is not ignored — `actionsFrom`'s `(?:#\s*(\S+))?` reads a trailing comment as the tag a SHA is meant to be, so `uses: actions/checkout@<sha> # dep-check: local` yields a dependency at version `dep-check:`. That is the pre-existing trailing-comment rule, not this diff, and it fails in the safe direction — a nonsense lookup that gets reported, never a silenced dependency. "Cannot be marked local" is exactly the right description of it. No change wanted; recording it so the next person does not discover it by trying. **§3** was informational and needed nothing. ## Re-run, not taken from the description - `deno test --allow-read tools/dep-check/` → **62 passed**. `deno lint tools/dep-check/` (11 files), `deno fmt --check` (314 files), and the **full `deno task check`** → clean. - `--dry-run` against the real tree and the real network: identical output to round three, **exit 0**, no 401. ``` - opsdeck:rig-base (docker/dev-rig/Dockerfile:12): built here, not checked checking 7 dependencies ``` - `309494d` really is comment- and doc-only: the two `.ts` hunks touch nothing but a docstring and a comment block, and re-running the round-three probes confirms behaviour is bit-identical — marked `FROM` skipped, `ARG` un-marks it back to watched, a mistyped marker leaves the line watched. - Still no drift: `git log fb29e2c..origin/main` over `tools/dep-check`, `docs/dependencies.md`, `docker/dev-rig` and `.forgejo/workflows/deps.yml` is empty; main is at `cc65b8c`. - Four clean conventional commits. Everything verified across the three previous rounds still holds: the parser survived twenty-five adversarial cases in round three and none of them silences a declaration the author did not mark; the marker is read below the `AS` name so a marked stage still registers; `isStage` and `scratch` are checked before the skip; `unwatched` does not inherit the skip; the exit rule is untouched; the orphaned-issue clause and the `# syntax=` bullet are in the doc; and `empty()` is still the sole `Scan` constructor with `main.ts` the sole `discover()` consumer. Ship it.
julian merged commit 18eb508b16 into main 2026-08-21 23:20:23 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/core!41
No description provided.