build(dx): enforce the proto prerequisite, document advisory scoping - #112
Merged
Conversation
Both facts a dependency bump needs were already written down, and neither was anywhere an agent looks before planning one. A recent bump was consequently scoped at two advisories when the real figure was nine, and ordered its steps so that the reconciliation would have stripped the requirement being patched. The CI rule already said Dependabot and govulncheck are complements and that neither alone is coverage. That describes a relationship; it prescribes no action, which is how the plan managed to quote it while still scoping from a single source. It now says to read both surfaces when scoping a bump, names the module-mode command that enumerates rather than filters, and states the asymmetry in both directions: a Dependabot alert's fix version clears the advisory it names rather than the package, so an unalerted sibling can need a higher version — and an advisory with no Go vulnerability database entry is invisible to govulncheck entirely, leaving Dependabot as its only surface. The generate-before-tidy ordering was documented only in a workflow step comment and a justfile comment. That workflow comment points at CLAUDE.md as the home for the manual step, so the loop was already assumed closed from the other end; it just wasn't. CLAUDE.md now carries the precondition next to the `just tidy` instruction it belongs to. The flag shape is stated because it is not the obvious one — module mode rejects a package pattern — and both commands were run to confirm the text describes what they actually do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…arning Amends the documentation-only change this branch started with. An adversarial pass established that the mechanism it described was wrong, and that the rule it stated in prose was already stated in two other files and enforced in none. What the mechanism actually is, established by removing daemon/gen and running the commands rather than by reading the comment that asserted it: `go mod tidy` does not prune the gRPC and protobuf requirements. It looks for the missing package as an external module and fails to load, naming `github.com/jakewan/finch` and a version. The gRPC requirement could not have been pruned in any case — non-generated main.go in both modules imports it directly. The workflow comment asserting the pruning behavior is corrected here too, since it was the source the prose paraphrased and would otherwise contradict it. The scope also turned out to be nine recipes rather than one. Generated code is imported by production and test files in both daemon and mcp, so building, testing, linting, scanning, and reconciling all require it — and only `just all` chained proto. Each of those nine now declares proto as a dependency; `just` runs it once per invocation however many legs ask. The core recipes deliberately do not, so `just test-core` still needs no protobuf toolchain, matching the existing reason `test-app` is kept out of `test`. That inverts what the documentation should say. Prose warning a reader to remember an ordering is replaced by prose explaining why a recipe has a prerequisite, and the two manual "run just proto first" instructions this made obsolete are retired. The build rule now describes the general requirement rather than naming build targets only, and states that a recipe missing it fails loudly rather than degrading. Two deliberate deviations worth naming: the concrete advisory numbers the plan specified for the CI rule were dropped in favour of generic wording, because version-specific examples in a durable rule rot; and the sentence documenting `just all`'s dependency chain was removed as redundant once each leg carries its own prerequisite. Verified end to end: with daemon/gen deleted, `just tidy` regenerates and then reconciles cleanly, which is the failure this change makes unreachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR formalizes the protobuf codegen prerequisite across local just recipes and aligns CI/docs to explain why generated daemon/gen/ must exist before Go package-loading steps (build/test/lint/vuln/tidy), plus adds guidance on properly scoping vulnerability advisories using both Dependabot alerts and govulncheck module-mode.
Changes:
- Make daemon/mcp build/test/lint/vuln/tidy recipes depend on
protosobuf generateruns automatically when needed. - Update CI workflow commentary to correctly describe why
go mod tidy -diffmust run after code generation (load failure vs. “pruning”). - Expand contributor-facing documentation to clarify the proto prerequisite and how to scope dependency-bump advisories.
PR description checks (per repo guidelines):
- Overview section: Pass
- Scope cohesion: Pass
- No private tooling references: Pass
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
justfile |
Adds proto prerequisites to daemon/mcp recipes (build/test/lint/vuln/tidy) to enforce codegen ordering automatically. |
CLAUDE.md |
Documents just tidy’s proto prerequisite and the CI ordering rationale. |
.github/workflows/ci-go.yml |
Corrects workflow comments about why tidy must run after generation (load failure, not dependency pruning). |
.claude/rules/ci.md |
Updates CI guidance to reflect the new automatic proto prerequisite and documents advisory scoping across Dependabot + govulncheck module scan. |
.claude/rules/build-and-migrations.md |
Replaces the manual “run proto first” instruction with the enforced prerequisite model and its failure mode. |
Comment on lines
+45
to
+47
| `just tidy` regenerates protobuf code first, as a prerequisite declared on the recipe rather than left to the caller. Generated code under `daemon/gen/` is not committed — absent on a fresh clone and after `just clean` — and `go mod tidy` cannot load the packages importing it, so without it the reconciliation fails outright rather than producing anything. See `.claude/rules/build-and-migrations.md` for the rule covering every target with that requirement. | ||
|
|
||
| CI verifies this rather than trusting it: `test-and-lint` runs `go mod tidy -diff` per module — after `buf generate`, for the same reason — so a missed reconciliation fails the build instead of sitting latent. |
Owner
Author
There was a problem hiding this comment.
Fixed in 1969122. An occurrence scan found the same claim in README.md's quick reference too, so both lines are corrected.
The recipe prerequisites added in this branch made two quick-start lines assert a setup step that no longer exists: both CLAUDE.md and README.md told the reader `just vuln` needs `just proto` run first, which its daemon and mcp legs now handle themselves. README.md was not otherwise part of this change. It is included because this branch is what made it false — leaving a consumer-facing doc asserting a prerequisite the tooling retired is a contract this change fractured rather than adjacent work. Enumerating the eight `just proto` references across the docs and rules found two more that are now redundant without being wrong: the install sequence in README.md's build-from-source section (`install-*` chains through `build-*` to `proto`), and the stale-PR smoke test's "just proto then just all". Both are left alone — neither misinforms, and the install-sequence comment still earns its place explaining to a first-time builder why generated code is absent from a fresh clone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
The plan behind #111 made two errors from one cause: the procedure for bumping a Go dependency was documented, but not where an agent reads while planning. It scoped nine advisories as two, and ordered its steps so the reconciliation would have run before the code generation it depends on.
Writing that procedure down surfaced that half of it should not be documentation at all. Generated code under
daemon/gen/is not committed, and production and test files in both daemon and mcp import it — so nine recipes require it and onlyjust allchained it. Each of those nine now declaresprotoas a dependency;justruns it once per invocation however many legs ask for it. Thecorerecipes deliberately do not, sojust test-corestill needs no protobuf toolchain, matching the reasontest-appis kept out oftest.The mechanism every existing description gave for that hazard was also wrong.
go mod tidywithout generated code does not prune the gRPC and protobuf requirements: it looks for the missing package as an external module and fails to load. gRPC could not be pruned in any case, since non-generatedmain.goimports it directly in both modules. The workflow comment asserting the pruning behaviour is corrected here too — it was the source the prose paraphrased, and would otherwise contradict it.What remains as guidance is the half that cannot be enforced: scoping a dependency bump means reading both Dependabot alerts and a module-mode
govulncheckscan, because each can be the only surface that sees a given advisory.How it works
The documentation's job inverted. Prose warning a reader to remember an ordering became prose explaining why a recipe carries a prerequisite, and the two manual "run
just protofirst" instructions it made obsolete are retired. The build rule now states the general requirement rather than naming build targets only, and says that a recipe missing it fails loudly rather than degrading."Read both surfaces" names an execution path for each. Naming only the scanner would leave the instruction half-unfollowable, because
.github/dependabot.ymlconfigures version updates rather than advisories — an agent grepping the repo for the alerts surface lands on the one file that cannot answer it. The module-mode caveats are stated for the same reason: it takes no package pattern, and it loads packages despite the name, so it needs generated code like everything else.Verified end to end rather than by reading: with
daemon/gendeleted,just tidyregenerates and then reconciles cleanly, which is the failure the prerequisite makes unreachable.