From 2b876fda801101b741d151e2ad8c8bebd7f700a9 Mon Sep 17 00:00:00 2001 From: Jacob Wan Date: Sat, 25 Jul 2026 11:58:03 -0700 Subject: [PATCH 1/3] docs(rules): document the dependency-bump procedure where it is read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .claude/rules/ci.md | 4 ++++ CLAUDE.md | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.claude/rules/ci.md b/.claude/rules/ci.md index 99755df..734fc17 100644 --- a/.claude/rules/ci.md +++ b/.claude/rules/ci.md @@ -22,6 +22,10 @@ It needs generated proto code, so run `just proto` first. `govulncheck` fails only on advisories it can **statically reach** from finch's own code. A scan reports a silent tail of advisories in imported packages and required modules that never surface, and static reachability is defeated by reflection and interface dispatch. So a green scan means "no reachable advisory," not "no known-vulnerable dependency." The broader question — is any dependency in the graph known-vulnerable at all — is answered by Dependabot alerts, which cover the full transitive closure but cannot tell you whether the code is reachable. The two are complements; neither alone is coverage. +So when scoping a dependency bump, read both surfaces rather than whichever one raised the alarm. `govulncheck -C -scan module` lists every advisory affecting a module in the graph with no reachability filtering, which is what surfaces the two blind spots. Note the flag shape: module mode accepts no package pattern, so a trailing `./...` is rejected. + +Each surface can be the only one that sees a given advisory. A Dependabot alert names **one** advisory, and its stated fix version clears that advisory rather than the package — an unalerted sibling in the same package can need a higher version. Going the other way, an advisory with no Go vulnerability database entry is invisible to `govulncheck` in every mode, and Dependabot is the only place it appears. A bump scoped from one surface alone closes some of what it looks like it closed. + The scan covers Go modules only. The Qt app's C++ dependencies (gRPC and protobuf from apt, Qt from the install action) are covered by neither mechanism. ### When an advisory has no available fix diff --git a/CLAUDE.md b/CLAUDE.md index 7248be0..1a710de 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,7 +42,9 @@ just test-mcp # MCP server only The daemon and mcp modules depend on core via `replace` directives pointing to `../core`. Because they resolve core's full dependency graph through those directives, a dependency change that shifts core's transitive versions (e.g. a Dependabot bump) leaves their `go.sum` stale. Run `just tidy` to reconcile all three modules after any such change. -CI verifies this rather than trusting it: `test-and-lint` runs `go mod tidy -diff` per module, so a missed reconciliation fails the build instead of sitting latent. +Run `just proto` before `just tidy`. Generated code under `daemon/gen/` is not committed, so on a clean tree it does not exist yet — and without it `go mod tidy` prunes the gRPC and protobuf requirements. On a dependency change that means the reconciliation step quietly deletes the requirement being changed. + +CI verifies both of these rather than trusting them: `test-and-lint` runs `go mod tidy -diff` per module, and does so *after* `buf generate` for the same reason. A missed reconciliation fails the build instead of sitting latent, and a tidy run that preceded generation is caught the same way — the committed `go.mod` arrives with the requirement stripped, so the diff reports it as missing. ## CI Notes From 849cbac413cd0647319cffa03d9ccbbb69b2e088 Mon Sep 17 00:00:00 2001 From: Jacob Wan Date: Sat, 25 Jul 2026 12:32:56 -0700 Subject: [PATCH 2/3] build(dx): make generated code a declared prerequisite instead of a warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .claude/rules/build-and-migrations.md | 4 +++- .claude/rules/ci.md | 4 ++-- .github/workflows/ci-go.yml | 7 ++++--- CLAUDE.md | 4 ++-- justfile | 21 ++++++++++----------- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/.claude/rules/build-and-migrations.md b/.claude/rules/build-and-migrations.md index a6d2a30..5dd362e 100644 --- a/.claude/rules/build-and-migrations.md +++ b/.claude/rules/build-and-migrations.md @@ -6,7 +6,9 @@ ## Just Targets -`just proto` MUST run before `just build-*`. The `just all` target runs the full dependency chain: proto → build-daemon → build-mcp. Per-module targets (`test-core`, `lint-daemon`, etc.) exist for focused development. +Generated code is a prerequisite of every target that loads a daemon or mcp package — building, testing, linting, scanning, and `just tidy` alike, since production and test files in both modules import `daemon/gen/`. Each of those recipes declares `proto` as a dependency rather than relying on the caller to remember, and `just` runs it once per invocation however many legs ask for it. The `core` recipes deliberately do not, so `just test-core` and its siblings need no protobuf toolchain. Per-module targets (`test-core`, `lint-daemon`, etc.) exist for focused development. + +A recipe that loads Go packages without generated code present does not degrade quietly — it fails to load, naming the missing `daemon/gen/finch/v1` import. Any new recipe touching daemon or mcp wants the same `proto` dependency. ## Embedded Migrations diff --git a/.claude/rules/ci.md b/.claude/rules/ci.md index 734fc17..b4f8912 100644 --- a/.claude/rules/ci.md +++ b/.claude/rules/ci.md @@ -16,13 +16,13 @@ This list is the single source of truth for the supplies below — they referenc `just vuln` scans all three Go modules with `govulncheck`. It is deliberately **not** in the list above: the scan takes minutes, and that list feeds the pre-merge and readiness supplies, so including it would impose the cost at every merge on top of CI already running it. It is also not in the pre-push hook, for the same reason. Run it when changing dependencies or when you want the answer before pushing; otherwise let CI be the enforcing surface. -It needs generated proto code, so run `just proto` first. +Its daemon and mcp legs regenerate protobuf code first, declared as a recipe prerequisite, so the scan needs no manual setup. ### What the gate guarantees `govulncheck` fails only on advisories it can **statically reach** from finch's own code. A scan reports a silent tail of advisories in imported packages and required modules that never surface, and static reachability is defeated by reflection and interface dispatch. So a green scan means "no reachable advisory," not "no known-vulnerable dependency." The broader question — is any dependency in the graph known-vulnerable at all — is answered by Dependabot alerts, which cover the full transitive closure but cannot tell you whether the code is reachable. The two are complements; neither alone is coverage. -So when scoping a dependency bump, read both surfaces rather than whichever one raised the alarm. `govulncheck -C -scan module` lists every advisory affecting a module in the graph with no reachability filtering, which is what surfaces the two blind spots. Note the flag shape: module mode accepts no package pattern, so a trailing `./...` is rejected. +So when scoping a dependency bump, read both surfaces rather than whichever one raised the alarm. For the scanner, `govulncheck -C -scan module` lists every advisory affecting a module in the graph with no reachability filtering — it lifts the reachability limits described above, not the database-coverage limit described below. Two notes on the form: module mode takes no package pattern, so a trailing `./...` is rejected, and it still loads packages, so run `just proto` first — this is a raw invocation rather than a recipe, so nothing declares that prerequisite on your behalf. For the alerts, `gh api repos/{owner}/{repo}/dependabot/alerts` — `.github/dependabot.yml` configures version updates rather than advisories, so it cannot answer this. Each surface can be the only one that sees a given advisory. A Dependabot alert names **one** advisory, and its stated fix version clears that advisory rather than the package — an unalerted sibling in the same package can need a higher version. Going the other way, an advisory with no Go vulnerability database entry is invisible to `govulncheck` in every mode, and Dependabot is the only place it appears. A bump scoped from one surface alone closes some of what it looks like it closed. diff --git a/.github/workflows/ci-go.yml b/.github/workflows/ci-go.yml index 99696e5..34350c9 100644 --- a/.github/workflows/ci-go.yml +++ b/.github/workflows/ci-go.yml @@ -130,9 +130,10 @@ jobs: - name: Generate protobuf code run: buf generate - # Enforces the go.sum reconciliation CLAUDE.md documents as a manual `just tidy` - # step. Must run after buf generate: without daemon/gen, tidy prunes the gRPC and - # protobuf requirements and reports a spurious diff. Cheap, so it fails fast. + # Enforces the go.sum reconciliation the `just tidy` recipe performs locally. + # Must run after buf generate: without daemon/gen, tidy cannot load the packages + # importing it and fails outright — it looks for the missing package as an external + # module rather than pruning anything. Cheap, so it fails fast. - name: Verify go.mod/go.sum are tidy run: | go -C core mod tidy -diff diff --git a/CLAUDE.md b/CLAUDE.md index 1a710de..3d7d0ba 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,9 +42,9 @@ just test-mcp # MCP server only The daemon and mcp modules depend on core via `replace` directives pointing to `../core`. Because they resolve core's full dependency graph through those directives, a dependency change that shifts core's transitive versions (e.g. a Dependabot bump) leaves their `go.sum` stale. Run `just tidy` to reconcile all three modules after any such change. -Run `just proto` before `just tidy`. Generated code under `daemon/gen/` is not committed, so on a clean tree it does not exist yet — and without it `go mod tidy` prunes the gRPC and protobuf requirements. On a dependency change that means the reconciliation step quietly deletes the requirement being changed. +`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 both of these rather than trusting them: `test-and-lint` runs `go mod tidy -diff` per module, and does so *after* `buf generate` for the same reason. A missed reconciliation fails the build instead of sitting latent, and a tidy run that preceded generation is caught the same way — the committed `go.mod` arrives with the requirement stripped, so the diff reports it as missing. +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. ## CI Notes diff --git a/justfile b/justfile index 1524082..b79dc08 100644 --- a/justfile +++ b/justfile @@ -7,11 +7,11 @@ proto: buf generate # Build the daemon binary -build-daemon: +build-daemon: proto cd daemon && go build -o finch-daemon . # Build the MCP server binary -build-mcp: +build-mcp: proto cd mcp && go build -o finch-mcp . # Build the Qt app @@ -34,11 +34,11 @@ test-core: cd core && go test ./... # Run daemon tests -test-daemon: +test-daemon: proto cd daemon && go test ./... # Run MCP server tests -test-mcp: +test-mcp: proto cd mcp && go test ./... # Run golangci-lint on all Go modules @@ -47,25 +47,24 @@ lint: lint-core lint-daemon lint-mcp lint-core: cd core && golangci-lint run ./... -lint-daemon: +lint-daemon: proto cd daemon && golangci-lint run ./... -lint-mcp: +lint-mcp: proto cd mcp && golangci-lint run ./... # Scan all Go modules for known vulnerabilities. -# Needs generated proto code — run `just proto` first, or the daemon and MCP scans -# fail to load packages (both import the gitignored daemon/gen). # Reports only advisories reachable from finch's own code; CI enforces the same check. +# For the complementary enumerating scan, see `.claude/rules/ci.md`. vuln: vuln-core vuln-daemon vuln-mcp vuln-core: cd core && govulncheck ./... -vuln-daemon: +vuln-daemon: proto cd daemon && govulncheck ./... -vuln-mcp: +vuln-mcp: proto cd mcp && govulncheck ./... # Format all Go code @@ -75,7 +74,7 @@ fmt: cd mcp && go fmt ./... # Reconcile go.mod/go.sum across all Go modules (run after a dependency change) -tidy: +tidy: proto cd core && go mod tidy cd daemon && go mod tidy cd mcp && go mod tidy From 1969122a57fa85d705386c31627d9d8517832d56 Mon Sep 17 00:00:00 2001 From: Jacob Wan Date: Sat, 25 Jul 2026 12:53:12 -0700 Subject: [PATCH 3/3] docs(dx): drop the manual proto step from the quick-reference lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3d7d0ba..086878d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ just all # Build everything just test # Run all tests just lint # Run linters just proto # Regenerate protobuf code -just vuln # Scan Go modules for known vulnerabilities (needs `just proto` first) +just vuln # Scan Go modules for known vulnerabilities ``` ## Architecture diff --git a/README.md b/README.md index 974298f..e7d2f88 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ just build-app # Build the Qt desktop app just test # Run all Go tests just test-app # Build and run the Qt app's Qt Quick Test suite just lint # Run golangci-lint on all modules -just vuln # Scan Go modules for known vulnerabilities (run just proto first) +just vuln # Scan Go modules for known vulnerabilities ``` ## Running Locally