fix(mise): unblock check in fresh worktrees, plus domain-architecture doc refresh - #514
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents proto-only tooling (buf + api-linter) from blocking the repo’s standard dev/test workflow by moving those tools from mise.dev.toml’s env-level [tools] into task-scoped tools = { ... } entries on the proto:* tasks that actually use them, with pinned versions. It also adjusts shell-script discovery to ignore agent worktrees and updates relevant Copilot skills documentation.
Changes:
- Move
buf/api-linterout ofmise.dev.toml’s[tools]and ontoproto:*tasks as task-scoped tools (pinned tobuf 1.70.0,api-linter 2.3.1). - Exclude
.claude/worktrees/**(and nested.mise/**) from shell script format/check file discovery. - Update skills docs to reflect the new mise tool-scoping behavior (plus a broader domain-architecture doc rewrite).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
mise.dev.toml |
Moves proto tooling to task-scoped tools so non-proto tasks aren’t blocked by proto tool provisioning failures. |
mise-tasks/shell/format.sh |
Adjusts find exclusions to avoid formatting nested worktree/toolchain shell scripts. |
mise-tasks/shell/check.sh |
Mirrors the find exclusions for shellcheck to avoid worktree/toolchain shell scripts. |
docs/code/domain-architecture.md |
Large documentation rewrite of domain architecture (appears out of scope for the PR title/description). |
.claude/skills/build-and-env/SKILL.md |
Updates the env/tooling documentation to match the new scoping approach (and retains the 403 trap history). |
.claude/skills/api-contract/SKILL.md |
Updates proto-tooling/403 guidance to reflect task-scoped tools and pinned versions. |
| # 2. Task-scoped tools degrade to a warning when provisioning fails, where an | ||
| # [tools] entry is a hard error. The task body still runs. |
| didn't need those tools. Reproduced live: | ||
|
|
||
| ``` | ||
| mise buf@1.70.0 [1/3] download buf-Linux-x86_64.tar.gz |
There was a problem hiding this comment.
Skipping this one — the inconsistency is pre-existing and intentional, and my diff doesn't touch those lines (git diff origin/main -- .claude/skills/build-and-env/SKILL.md returns no buf@ changes; I only edited the prose above the block).
It's also not actually inconsistent. That's a verbatim captured log, and mise legitimately prints the resolved version in progress lines (buf@1.70.0) and the requested spec in the error summary (aqua:bufbuild/buf@latest) — because latest resolved to 1.70.0 before the download failed. Changing the first line to buf@latest would make the snippet internally tidier but would no longer be a real mise transcript, which defeats the point of quoting it.
Generated by Claude Code
| ↓ | ||
| ┌─────────────────────────────────────────────┐ | ||
| │ State Management (BeerProvider) │ | ||
| │ • Orchestrates domain services │ | ||
| │ • Composes domain controllers │ | ||
| │ • Manages UI state (loading, errors) │ | ||
| │ • Handles persistence, analytics, notify │ | ||
| │ • Uses repositories for data access │ | ||
| └───────────┬────────────────┬────────────────┘ |
There was a problem hiding this comment.
Fair catch — the doc rewrite is genuinely unrelated to the tooling fix. It's here because the branch bundles three issues (#509, #510, #511), not because they're coupled.
I've rewritten the PR description to cover all three and call the mixed scope out explicitly. The three commits are clean and separable (5d83960 tooling, 8985795 shell discovery, 239505f docs), so splitting into three PRs is cheap if the maintainer prefers independent review and rollback — flagged there for a decision.
Generated by Claude Code
api-linter sat in mise.dev.toml's [tools], so it was provisioned for every dev-env task. Its install fails behind a proxy, which aborted format, analyze, test and check — none of which need proto tooling. On Claude Code Web .miserc.toml auto-selects the dev env, so this broke plain `./bin/mise run check` outright. Declare buf and api-linter as task-scoped tools on the proto:* tasks that invoke them. Note what this does NOT fix, since the issue proposes it: pinning. Both tools were already pinned and locked — mise.dev.lock carried buf 1.70.0 and api-linter 2.3.1 with checksums and asset URLs, and the failure happened anyway. Verified on mise 2026.5.8 that a lockfile entry does not suppress api-linter's SLSA provenance call: download and checksum both succeed, only the provenance lookup 403s, and MISE_SLSA_VERIFY=0 does not disable it. mise had recorded github_attestations = "unavailable" and still attempted verification. What scoping buys is blast radius, which is the actual complaint in #510. Also set lockfile = true so mise maintains mise.lock/mise.dev.lock on install rather than only when regenerated by hand (#454). Known trade-off: task-scoped tools are not in a config's [tools] set, so `mise lock` drops them from mise.dev.lock and they re-resolve on a fresh worktree. Moving them to a MISE_ENV=dev,proto env file would keep both properties, at the cost of changing the proto:* invocation. Verified: `./bin/mise run check` completes (1313 tests pass) where it previously died during tool provisioning. Fixes #510
The find in shell/check.sh and shell/format.sh excluded "./.mise/*", anchored at the repo root, so it did not match a *nested* .mise inside an agent worktree. Each worktree under .claude/worktrees/ carries its own extracted Flutter SDK — thousands of vendored .sh files, some of which shfmt rejects — so check aborted before analyze or test ran, pointing at vendored SDK code rather than anything the developer changed. This broke the pre-commit gate for the repo's own documented parallel-agent workflow, whose whole mechanism is agent worktrees. Make the exclusions unanchored and skip worktrees explicitly. Verified against a synthetic worktree tree: the old pattern picks up its scripts, the new one excludes them, and shell:check passes with it present. Fixes #509
Every symbol the filter sections named was removed when filter state moved into DrinkFilterController, and the drift widened with #505/#506. The worked example did not compile against the current code. - applyAllFilters -> filterDrinks, with its real named parameters - BeerProvider._applyFiltersAndSort() -> DrinkFilterController.recompute() as the single recompute path - category: String? -> categories: Set; hideUnavailable: bool -> visibilityFilters: Set - add the missing facet-scoping rule and its two invariants (an active filter is never hidden; allergens must be non-zero-present), plus the deliberate exclusion of free-text search from facet scoping Also correct references to FavoritesService/RatingsService, removed in #391/#395 in favour of UserDataStore, and add the controllers layer to the architecture diagram. Leaving those would have shipped a doc fix that still pointed at deleted classes. Fixes #511
1689138 to
239505f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-ship-issues-509-511-y.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
The comment said a task-scoped tool that fails to provision degrades to a warning and "the task body still runs", which reads as though the task succeeds. It doesn't: proto:* will run its body and fail on `buf: command not found` unless the binary is already on PATH. Spell out that the win is check/test no longer being collateral damage, not that proto linting works without buf. Raised in review on #514.
richardthe3rd
changed the title
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-ship-issues-509-511-y.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Bundles three related issues. They share a branch because the session was pinned to a single branch, not because they're coupled — see "Scope" below if you'd prefer them split.
Fixes #510
Fixes #509
Fixes #511
#510 — proto tooling blocked every mise task
api-lintersat inmise.dev.toml's[tools], so it was provisioned for every dev-env task. Its install fails behind a proxy, which abortedformat,analyze,testandcheck— none of which need proto tooling. On Claude Code Web.miserc.tomlauto-selects the dev env, so this broke plain./bin/mise run checkoutright.bufandapi-linterare now task-scoped to theproto:*tasks that invoke them, andlockfile = trueis set so mise maintainsmise.lock/mise.dev.lockon install rather than only when regenerated by hand (#454).What this does not fix, since the issue proposes it: pinning. Both tools were already pinned and locked —
mise.dev.lockcarriedbuf1.70.0 andapi-linter2.3.1 with checksums and asset URLs, and the failure happened anyway. Verified on mise 2026.5.8 that a lockfile entry does not suppressapi-linter's SLSA provenance call: download and checksum both succeed, only the provenance lookup againstapi.github.com403s, andMISE_SLSA_VERIFY=0does not disable it. mise had even recordedgithub_attestations = "unavailable"in the lock and still attempted verification — that looks like a mise bug worth reporting upstream.So what scoping buys is blast radius, which is the actual complaint in the issue. It contains the failure rather than resolving it: a
proto:*task will still fail onbuf: command not foundif the binary isn't already on PATH. The win is thatcheckandtestare no longer collateral damage.Known trade-off: task-scoped tools aren't part of a config's
[tools]set, somise lockdrops them frommise.dev.lock— those two lose lock coverage and re-resolve on a fresh worktree. Moving them to aMISE_ENV=dev,protoenv file would keep both properties (off the critical path and locked) at the cost of changing theproto:*invocation. Not done here; happy to switch if preferred.#509 —
checkfailed whenever an agent worktree existedfindinmise-tasks/shell/{check,format}.shexcluded./.mise/*, anchored at the repo root, so it didn't match a nested.miseinside an agent worktree — each of which carries its own extracted Flutter SDK. Exclusions are now unanchored, with worktrees skipped explicitly.Verified against a synthetic worktree: the old pattern picks up its scripts, the new one excludes them, and
shell:checkpasses with one present.#511 — domain-architecture.md documented a removed filter API
Rewrote the filter sections against current code:
applyAllFilters→filterDrinkswith its real named parameters,BeerProvider._applyFiltersAndSort()→DrinkFilterController.recompute()as the single recompute path, andcategory: String?/hideUnavailable: bool→ theSet-based fields. Added the facet-scoping rule and its two invariants (an active filter is never hidden; allergens must be non-zero-present) plus the deliberate exclusion of free-text search — previously documented only in the controller's doc comment.Also corrected
FavoritesService/RatingsServicereferences (removed in #391/#395 in favour ofUserDataStore) and added the controllers layer to the diagram.Scope
Copilot flagged that the doc rewrite is unrelated to the tooling fix, which is fair. The three commits are clean and separable, so this can be split into three PRs if you'd rather review and roll them back independently — say the word.
Verification
./bin/mise run checkcompletes (1313 tests pass) in an environment where it previously died during tool provisioning. Note that this could not have been verified before #510's fix —checkwas unrunnable here, which forced the implementation order.