Add dev speed tooling: caching, scoped tests, hot-reload, and scaffolder#223
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR introduces developer speed improvements by adding filesystem-backed build caching, scoped pre-commit testing based on staged Go files, and automated hot-reload development via Air. The changes include a cache library, pre-commit hook enhancements, Makefile tooling integration, and supporting infrastructure. ChangesDev Speed and Iteration Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
tools/scaffold/templates/handler_test.go.tmpl (1)
9-13: ⚡ Quick winUse
require.NotNilbefore dereferencing handler fields.
assert.NotNilallows continuation, soh.siteIDcan still be evaluated after a nil failure. Userequire.NotNilfirst, thenassert.Equal.Proposed diff
import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewHandler(t *testing.T) { h := NewHandler(noopStore{}, "site-test") - assert.NotNil(t, h) + require.NotNil(t, h) assert.Equal(t, "site-test", h.siteID) }As per coding guidelines "
**/*_test.go: Use standardtestingpackage withgithub.com/stretchr/testify/assertandtestify/requirefor assertions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/scaffold/templates/handler_test.go.tmpl` around lines 9 - 13, The test TestNewHandler must stop execution if NewHandler returns nil before accessing h.siteID; replace the initial assert.NotNil with require.NotNil for the handler variable (call require.NotNil(t, h) before any dereference), then keep the assert.Equal(t, "site-test", h.siteID) to check the siteID—update the TestNewHandler function to use require.NotNil from testify/require and ensure the import is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/hooks/pre-commit.sh:
- Line 38: The cache key generation expands $STAGED_GO with
word-splitting/globbing causing incorrect keys for paths with spaces or special
chars; update the pre-commit hook so buildcache_key is called with a
NUL/newline-safe array of staged files instead of an unquoted expansion: collect
staged paths into an array variable (e.g., STAGED_GO_ARRAY), iterate or pass the
array elements quoted (or use a NUL-delimited stream) into buildcache_key, and
ensure KEY is assigned from the function call that receives the safe array so
buildcache_key gets exact file names without word-splitting; reference the
existing KEY assignment and the buildcache_key invocation in
.claude/hooks/pre-commit.sh.
In `@docs/superpowers/plans/2026-05-25-dev-speed-suggestions.md`:
- Line 152: The markdown contains multiple fenced code blocks using only ```
which trigger markdownlint MD040; update each flagged fence start (the literal
``` markers) to include an explicit language identifier (e.g., ```text, ```go,
```yaml, ```dockerfile) appropriate to the block contents so the linter stops
flagging MD040.
- Line 27: The plan references two different air template names (.air.toml.tmpl
vs air-template.toml) which will break scaffolding; pick one canonical filename
and make the text and Task 4 consistent — either rename the artifact created in
Task 4 (referenced as air-template.toml) to .air.toml.tmpl, or change the plan
text and all later references (the repeated block around Task 4 and the
duplicate section later) to use air-template.toml; update every mention in the
plan so the template name is identical and include a short note in Task 4 that
the file is created under the agreed canonical name.
In `@Makefile`:
- Around line 185-190: The SAST cache key generation (the files list used by
buildcache_key and checked by buildcache_hit) excludes ./tools/* while the
sast-vuln target invokes govulncheck ./... (which includes tools), causing false
cache hits; fix by making the scopes match: either add ./tools/* back into the
files passed to buildcache_key/buildcache_hit (so the key reflects tool changes)
or restrict the sast-vuln govulncheck invocation to the same excluded paths (so
scans match the key). Update the Makefile entries that build the files variable
and the sast-vuln target (references: buildcache_key, buildcache_hit, sast-vuln,
govulncheck) accordingly.
In `@tools/scaffold/main_test.go`:
- Around line 10-99: Replace manual testing checks in
TestGenerate_CreatesAllExpectedFiles, TestGenerate_SubstitutesServiceName,
TestGenerate_RefusesOverwriteUnlessForce, and
TestGenerate_RejectsInvalidServiceName to use
github.com/stretchr/testify/require for fatal conditions (e.g. prepare/first
generate errors, readFile failures, and initial generate calls) and
github.com/stretchr/testify/assert for non-fatal checks (e.g. file existence,
string contains, second-generate failure expectations); update imports to
include "github.com/stretchr/testify/assert" and
"github.com/stretchr/testify/require", swap t.Fatalf/t.Fatalf-equivalents for
require.NoError/require.FailNow or require.Error, and t.Errorf checks for
assert.True/assert.Contains/assert.Error as appropriate, keeping helper readFile
and the generate(generateOpts{...}) calls unchanged.
In `@tools/scaffold/main.go`:
- Around line 95-103: The current code creates the output file with os.Create
(variable out), defers out.Close() and executes tmpl.Execute, which ignores any
error returned from Close; change the defer to capture and propagate close
errors: replace defer out.Close() with a deferred function that checks the
Close() result and, if a non-nil error occurs, wraps/combines it with the
current err (the error from tmpl.Execute or a new error) and returns it (e.g.,
using a named return error variable or assigning to err in the deferred
closure); ensure errors from tmpl.Execute and out.Close() are both returned
(referencing variables out, tmpl.Execute, and Close).
In `@tools/scaffold/templates/azure-pipelines.yml.tmpl`:
- Around line 15-17: The GoTool@0 task is pinned to a patch-precision version
string ('version: '1.25.10'') which can fail if that exact patch isn’t
available; update the GoTool@0 task's inputs to use a minor-precision release
(e.g., change the version value to '1.25') or set it to the actual published
patch (e.g., '1.25.<published>') so the runner will resolve a valid Go release.
In `@tools/scaffold/templates/handler.go.tmpl`:
- Around line 19-23: In Handler.HandleJetStreamMsg, include the
correlation/request_id in all log fields: extract the ID from context (e.g., a
request-id helper like requestid.FromContext(ctx) or ctx.Value("request_id"))
and fallback to msg.Header().Get("request_id") if missing, then add that value
as a "request_id" key to both the slog.InfoContext and slog.ErrorContext calls
so every message log includes the traceable correlation ID.
In `@tools/scaffold/templates/main.go.tmpl`:
- Around line 34-38: After parsing cfg (cfg := env.ParseAs[config]()), validate
cfg.MAX_WORKERS is > 0 before it is used to create the semaphore or set pull
limits (the variables/functions that build sem capacity and pullLimit). If
cfg.MAX_WORKERS is not > 0, return/exit with a clear error via slog.Error and
non-zero exit code, or replace it with a sane default; ensure both the semaphore
creation (sem) and any pull limit calculations use the validated/normalized
MAX_WORKERS value.
- Around line 88-90: The consume loop currently swallows errors from iter.Next()
(msgCtx, msg, err := iter.Next()) by just returning; change this to surface the
error instead of silently exiting: when iter.Next() returns a non-nil err, log
the error with context (including msgCtx if available) and either break the loop
or propagate the error to the caller (return the err) so the failure is visible
and handled; update the surrounding function to return an error if you choose
propagation, or ensure the logger (e.g., processLogger or fmt) records the
failure before exiting the loop.
---
Nitpick comments:
In `@tools/scaffold/templates/handler_test.go.tmpl`:
- Around line 9-13: The test TestNewHandler must stop execution if NewHandler
returns nil before accessing h.siteID; replace the initial assert.NotNil with
require.NotNil for the handler variable (call require.NotNil(t, h) before any
dereference), then keep the assert.Equal(t, "site-test", h.siteID) to check the
siteID—update the TestNewHandler function to use require.NotNil from
testify/require and ensure the import is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a708571a-4b24-4cad-8e2f-78c7d2baf71c
📒 Files selected for processing (18)
.claude/hooks/pre-commit.sh.gitignoreMakefiledocs/superpowers/plans/2026-05-25-dev-speed-suggestions.mdtools/buildcache/buildcache.shtools/buildcache/buildcache_test.shtools/dev/air-template.tomltools/dev/dev.shtools/scaffold/main.gotools/scaffold/main_test.gotools/scaffold/templates/Dockerfile.tmpltools/scaffold/templates/azure-pipelines.yml.tmpltools/scaffold/templates/bootstrap.go.tmpltools/scaffold/templates/docker-compose.yml.tmpltools/scaffold/templates/handler.go.tmpltools/scaffold/templates/handler_test.go.tmpltools/scaffold/templates/main.go.tmpltools/scaffold/templates/store.go.tmpl
| if [ "${PRECOMMIT_NO_CACHE:-0}" != "1" ] && [ -f tools/buildcache/buildcache.sh ]; then | ||
| # shellcheck disable=SC1091 | ||
| source tools/buildcache/buildcache.sh | ||
| KEY=$(buildcache_key $STAGED_GO) |
There was a problem hiding this comment.
Quote-safe staged file handling is broken for cache key generation.
Line 38 expands $STAGED_GO with word splitting/globbing, so paths with spaces/special chars produce incorrect keys and unreliable cache hits/misses.
Use an array (NUL/newline-safe) and pass quoted elements.
Suggested fix
-STAGED_GO=$(git diff --cached --name-only --diff-filter=ACMR -- '*.go' 'go.mod' 'go.sum')
+mapfile -t STAGED_GO_FILES < <(git diff --cached --name-only --diff-filter=ACMR -- '*.go' 'go.mod' 'go.sum')
+STAGED_GO=$(printf '%s\n' "${STAGED_GO_FILES[@]}")
...
- KEY=$(buildcache_key $STAGED_GO)
+ KEY=$(buildcache_key "${STAGED_GO_FILES[@]}")🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/hooks/pre-commit.sh at line 38, The cache key generation expands
$STAGED_GO with word-splitting/globbing causing incorrect keys for paths with
spaces or special chars; update the pre-commit hook so buildcache_key is called
with a NUL/newline-safe array of staged files instead of an unquoted expansion:
collect staged paths into an array variable (e.g., STAGED_GO_ARRAY), iterate or
pass the array elements quoted (or use a NUL-delimited stream) into
buildcache_key, and ensure KEY is assigned from the function call that receives
the safe array so buildcache_key gets exact file names without word-splitting;
reference the existing KEY assignment and the buildcache_key invocation in
.claude/hooks/pre-commit.sh.
Comment on lines
+185
to
+190
| files=$$(find . -name '*.go' \ | ||
| -not -path './tools/*' -not -path './chat-frontend/*' \ | ||
| -not -path './testdata/*' -not -path './docs/*' \ | ||
| -not -path './.cache/*' -not -path './tmp/*' | sort); \ | ||
| key=$$(buildcache_key go.sum $$files); \ | ||
| if buildcache_hit "$(SAST_CACHE_DIR)" "$(SAST_CACHE_BUCKET)" "$$key"; then \ |
There was a problem hiding this comment.
SAST cache key omits files that govulncheck still scans.
Line 185–188 excludes ./tools/* from the cache key, but Line 219 runs govulncheck ./..., which includes tools packages. This can return a false cache hit and skip required scans after tool-package changes.
Use matching scope for key + scans (either include tools in key inputs, or scope sast-vuln to the same excluded paths).
Suggested alignment (example: keep tools excluded consistently)
sast-vuln:
`@test` -x "$(GOVULNCHECK)" || { echo "govulncheck not installed — run 'make tools'"; exit 1; }
- GOTOOLCHAIN=$(TOOLS_GO_TOOLCHAIN) $(GOVULNCHECK) ./...
+ GOTOOLCHAIN=$(TOOLS_GO_TOOLCHAIN) $(GOVULNCHECK) ./... 2>/dev/null- key=$$(buildcache_key go.sum $$files); \
+ key=$$(buildcache_key go.sum $$files); \(If you prefer scanning tools, keep sast-vuln ./... and remove -not -path './tools/*' from the key input set.)
Also applies to: 217-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 185 - 190, The SAST cache key generation (the files
list used by buildcache_key and checked by buildcache_hit) excludes ./tools/*
while the sast-vuln target invokes govulncheck ./... (which includes tools),
causing false cache hits; fix by making the scopes match: either add ./tools/*
back into the files passed to buildcache_key/buildcache_hit (so the key reflects
tool changes) or restrict the sast-vuln govulncheck invocation to the same
excluded paths (so scans match the key). Update the Makefile entries that build
the files variable and the sast-vuln target (references: buildcache_key,
buildcache_hit, sast-vuln, govulncheck) accordingly.
Comment on lines
+10
to
+99
| func TestGenerate_CreatesAllExpectedFiles(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| if err := generate(generateOpts{ | ||
| ServiceName: "foo-worker", | ||
| StreamName: "MESSAGES_CANONICAL", | ||
| RepoRoot: tmp, | ||
| }); err != nil { | ||
| t.Fatalf("generate: %v", err) | ||
| } | ||
| want := []string{ | ||
| "foo-worker/main.go", | ||
| "foo-worker/bootstrap.go", | ||
| "foo-worker/store.go", | ||
| "foo-worker/handler.go", | ||
| "foo-worker/handler_test.go", | ||
| "foo-worker/deploy/Dockerfile", | ||
| "foo-worker/deploy/docker-compose.yml", | ||
| "foo-worker/deploy/azure-pipelines.yml", | ||
| } | ||
| for _, rel := range want { | ||
| path := filepath.Join(tmp, rel) | ||
| if _, err := os.Stat(path); err != nil { | ||
| t.Errorf("missing file %s: %v", rel, err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestGenerate_SubstitutesServiceName(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| if err := generate(generateOpts{ | ||
| ServiceName: "my-thing", | ||
| StreamName: "MESSAGES_CANONICAL", | ||
| RepoRoot: tmp, | ||
| }); err != nil { | ||
| t.Fatalf("generate: %v", err) | ||
| } | ||
| main := readFile(t, filepath.Join(tmp, "my-thing", "main.go")) | ||
| if !strings.Contains(main, "my-thing") { | ||
| t.Errorf("main.go missing service name; got:\n%s", main) | ||
| } | ||
| dockerfile := readFile(t, filepath.Join(tmp, "my-thing", "deploy", "Dockerfile")) | ||
| if !strings.Contains(dockerfile, "my-thing/") { | ||
| t.Errorf("Dockerfile missing service name path; got:\n%s", dockerfile) | ||
| } | ||
| } | ||
|
|
||
| func TestGenerate_RefusesOverwriteUnlessForce(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| opts := generateOpts{ | ||
| ServiceName: "dup", | ||
| StreamName: "MESSAGES_CANONICAL", | ||
| RepoRoot: tmp, | ||
| } | ||
| if err := generate(opts); err != nil { | ||
| t.Fatalf("first generate: %v", err) | ||
| } | ||
| if err := generate(opts); err == nil { | ||
| t.Errorf("second generate without Force should fail") | ||
| } | ||
| opts.Force = true | ||
| if err := generate(opts); err != nil { | ||
| t.Errorf("generate with Force should succeed: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestGenerate_RejectsInvalidServiceName(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| cases := []string{"", "Foo", "foo_bar", "foo bar", "../escape"} | ||
| for _, name := range cases { | ||
| t.Run(name, func(t *testing.T) { | ||
| err := generate(generateOpts{ | ||
| ServiceName: name, | ||
| StreamName: "MESSAGES_CANONICAL", | ||
| RepoRoot: tmp, | ||
| }) | ||
| if err == nil { | ||
| t.Errorf("expected error for invalid name %q", name) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func readFile(t *testing.T, path string) string { | ||
| t.Helper() | ||
| b, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("read %s: %v", path, err) | ||
| } | ||
| return string(b) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use testify/assert and testify/require for assertions in these tests.
Please replace manual assertion patterns (if ... t.Errorf/t.Fatalf) with assert/require to align with project test standards.
As per coding guidelines, "**/*_test.go: Use standard testing package with github.com/stretchr/testify/assert and testify/require for assertions".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/main_test.go` around lines 10 - 99, Replace manual testing
checks in TestGenerate_CreatesAllExpectedFiles,
TestGenerate_SubstitutesServiceName, TestGenerate_RefusesOverwriteUnlessForce,
and TestGenerate_RejectsInvalidServiceName to use
github.com/stretchr/testify/require for fatal conditions (e.g. prepare/first
generate errors, readFile failures, and initial generate calls) and
github.com/stretchr/testify/assert for non-fatal checks (e.g. file existence,
string contains, second-generate failure expectations); update imports to
include "github.com/stretchr/testify/assert" and
"github.com/stretchr/testify/require", swap t.Fatalf/t.Fatalf-equivalents for
require.NoError/require.FailNow or require.Error, and t.Errorf checks for
assert.True/assert.Contains/assert.Error as appropriate, keeping helper readFile
and the generate(generateOpts{...}) calls unchanged.
Comment on lines
+95
to
+103
| out, err := os.Create(outPath) | ||
| if err != nil { | ||
| return fmt.Errorf("create output: %w", err) | ||
| } | ||
| defer out.Close() | ||
| if err := tmpl.Execute(out, data); err != nil { | ||
| return fmt.Errorf("execute template: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Propagate Close errors when writing generated files.
defer out.Close() drops close-time I/O failures, so generation can report success with incomplete output.
Proposed fix
-func renderTemplate(tmplName, outPath string, data templateData) error {
+func renderTemplate(tmplName, outPath string, data templateData) (retErr error) {
src, err := fs.ReadFile(templatesFS, "templates/"+tmplName)
if err != nil {
return fmt.Errorf("read embedded template: %w", err)
}
@@
out, err := os.Create(outPath)
if err != nil {
return fmt.Errorf("create output: %w", err)
}
- defer out.Close()
+ defer func() {
+ if cerr := out.Close(); retErr == nil && cerr != nil {
+ retErr = fmt.Errorf("close output: %w", cerr)
+ }
+ }()
if err := tmpl.Execute(out, data); err != nil {
return fmt.Errorf("execute template: %w", err)
}
return nil
}As per coding guidelines, "Never ignore errors silently — comment if intentionally discarded".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/main.go` around lines 95 - 103, The current code creates the
output file with os.Create (variable out), defers out.Close() and executes
tmpl.Execute, which ignores any error returned from Close; change the defer to
capture and propagate close errors: replace defer out.Close() with a deferred
function that checks the Close() result and, if a non-nil error occurs,
wraps/combines it with the current err (the error from tmpl.Execute or a new
error) and returns it (e.g., using a named return error variable or assigning to
err in the deferred closure); ensure errors from tmpl.Execute and out.Close()
are both returned (referencing variables out, tmpl.Execute, and Close).
Comment on lines
+15
to
+17
| - task: GoTool@0 | ||
| inputs: | ||
| version: '1.25.10' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Azure Pipelines GoTool@0support installing exact version1.25.10? If not, what version format should be used (e.g., 1.25.x)?
💡 Result:
No—based on the Azure Pipelines GoTool@0 task’s documented version input, you should not rely on a patch-precision string like 1.25.10 being accepted as an exact Go release/version specifier. The task’s version is treated as a Go version string such as 1.9.3, but the practical supported pattern for pinning/reproducibility with this task is to specify the full released Go version (major.minor or major.minor.patch as published by Go). The official task reference only gives an example with a patch release format like 1.9.3 and does not document a 1.25.10-style 2-digit patch expectation for this input [1]. What to use instead: - If you want a specific published Go release, use the exact published patch version (for Go 1.25 that would be 1.25.<published patch> once known/released; e.g., 1.25.2 is shown in Azure-pipelines-tasks tests/docs) [2]. - If you want the latest patch for the 1.25 line, use the minor-only pin such as 1.25 (task behavior supports this style; Azure-pipelines-tasks discussion mentions downloading/caching the latest patch for Go 1.25 when version: '1.25') [2]. So for your question: prefer version: '1.25' (latest patch in Go 1.25) or version: '1.25.<exact patch release>' (e.g., 1.25.2 style) rather than 1.25.10. [1][2]
Citations:
- 1: https://learn.microsoft.com/en-us/azure/devops/pipelines/tasks/reference/go-tool-v0?view=azure-pipelines
- 2: Configure gotools task to pass custom url from customer microsoft/azure-pipelines-tasks#21285
Fix GoTool@0 version pin (version: '1.25.10') to a valid Go release.
GoTool@0’s version input is a Go version string, and relying on a patch-precision format like 1.25.10 is not documented/guaranteed; if that exact Go patch release doesn’t exist or isn’t recognized for download, the tool install will fail and stop the pipeline before build/test. Use version: '1.25' to follow the latest patch in the 1.25 line, or pin to the actual published patch version for Go 1.25 (e.g., 1.25.<published>).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/templates/azure-pipelines.yml.tmpl` around lines 15 - 17, The
GoTool@0 task is pinned to a patch-precision version string ('version:
'1.25.10'') which can fail if that exact patch isn’t available; update the
GoTool@0 task's inputs to use a minor-precision release (e.g., change the
version value to '1.25') or set it to the actual published patch (e.g.,
'1.25.<published>') so the runner will resolve a valid Go release.
Comment on lines
+19
to
+23
| func (h *Handler) HandleJetStreamMsg(ctx context.Context, msg jetstream.Msg) { | ||
| slog.InfoContext(ctx, "{{.ServiceName}} received message", "subject", msg.Subject()) | ||
| if err := msg.Ack(); err != nil { | ||
| slog.ErrorContext(ctx, "ack failed", "error", err) | ||
| } |
There was a problem hiding this comment.
Include request_id in handler log fields.
The current log lines only include subject/error; add correlation ID from context (or headers fallback) so each message is traceable across services.
As per coding guidelines, "Generate or extract a unique request/correlation ID at the entry point ... include in all log lines".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/templates/handler.go.tmpl` around lines 19 - 23, In
Handler.HandleJetStreamMsg, include the correlation/request_id in all log
fields: extract the ID from context (e.g., a request-id helper like
requestid.FromContext(ctx) or ctx.Value("request_id")) and fallback to
msg.Header().Get("request_id") if missing, then add that value as a "request_id"
key to both the slog.InfoContext and slog.ErrorContext calls so every message
log includes the traceable correlation ID.
Comment on lines
+34
to
+38
| cfg, err := env.ParseAs[config]() | ||
| if err != nil { | ||
| slog.Error("parse config", "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Validate MAX_WORKERS is greater than zero before using it.
A non-positive value can produce a broken worker setup (sem capacity and pull limits), leading to stalled consumption.
Proposed fix
cfg, err := env.ParseAs[config]()
if err != nil {
slog.Error("parse config", "error", err)
os.Exit(1)
}
+ if cfg.MaxWorkers <= 0 {
+ slog.Error("invalid MAX_WORKERS", "max_workers", cfg.MaxWorkers)
+ os.Exit(1)
+ }Also applies to: 83-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/templates/main.go.tmpl` around lines 34 - 38, After parsing
cfg (cfg := env.ParseAs[config]()), validate cfg.MAX_WORKERS is > 0 before it is
used to create the semaphore or set pull limits (the variables/functions that
build sem capacity and pullLimit). If cfg.MAX_WORKERS is not > 0, return/exit
with a clear error via slog.Error and non-zero exit code, or replace it with a
sane default; ensure both the semaphore creation (sem) and any pull limit
calculations use the validated/normalized MAX_WORKERS value.
Comment on lines
+88
to
+90
| msgCtx, msg, err := iter.Next() | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
Don’t silently exit the consume loop on iterator errors.
The loop currently returns without any signal/log on iter.Next() failure, which can leave the process running but no longer consuming.
Proposed fix
msgCtx, msg, err := iter.Next()
if err != nil {
+ slog.Error("iterator next failed", "error", err)
return
}As per coding guidelines, "Never ignore errors silently — comment if intentionally discarded".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scaffold/templates/main.go.tmpl` around lines 88 - 90, The consume loop
currently swallows errors from iter.Next() (msgCtx, msg, err := iter.Next()) by
just returning; change this to surface the error instead of silently exiting:
when iter.Next() returns a non-nil err, log the error with context (including
msgCtx if available) and either break the loop or propagate the error to the
caller (return the err) so the failure is visible and handled; update the
surrounding function to return an error if you choose propagation, or ensure the
logger (e.g., processLogger or fmt) records the failure before exiting the loop.
hmchangw
force-pushed
the
claude/dev-speed-suggestions-zpt2h
branch
from
bb8c9e4 to
b9e5f2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Makefile (2)
150-150: ⚡ Quick winCentralize Air version pin like other tooling versions.
Line 150 hardcodes
v1.62.0; defining anAIR_VERSIONvariable keeps version management consistent and safer to update.Proposed change
+AIR_VERSION := v1.62.0 @@ - GOTOOLCHAIN=$(TOOLS_GO_TOOLCHAIN) go install github.com/air-verse/air@v1.62.0 + GOTOOLCHAIN=$(TOOLS_GO_TOOLCHAIN) go install github.com/air-verse/air@$(AIR_VERSION)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 150, Add a new AIR_VERSION variable at the top of the Makefile and replace the hardcoded version in the install command: instead of "go install github.com/air-verse/air@v1.62.0" use the AIR_VERSION variable (e.g., github.com/air-verse/air@$(AIR_VERSION)), so update the line that contains the GOTOOLCHAIN ... go install github.com/air-verse/air@v1.62.0 to reference AIR_VERSION; ensure AIR_VERSION is documented near other tool version variables for consistency.
129-130: ⚡ Quick winAvoid mutating file permissions during
make dev.Running
chmod +xon every dev run can dirty the working tree unnecessarily. Prefer invoking viabash(or set executable bit once in git).Proposed change
- `@chmod` +x tools/dev/dev.sh - ./tools/dev/dev.sh $(SERVICE) + bash tools/dev/dev.sh $(SERVICE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 129 - 130, Remove the chmod +x invocation from the Makefile dev target and call the script via an interpreter instead: replace the chmod +x tools/dev/dev.sh and ./tools/dev/dev.sh $(SERVICE) sequence with a direct invocation like bash tools/dev/dev.sh $(SERVICE) (or sh tools/dev/dev.sh $(SERVICE)); alternatively, set the executable bit for tools/dev/dev.sh once in the repository and keep the existing ./tools/dev/dev.sh $(SERVICE) invocation. Ensure the change targets the Makefile entry that runs tools/dev/dev.sh.tools/buildcache/buildcache_test.sh (1)
16-23: ⚡ Quick winAdd regression checks for order-independence and path sensitivity.
Current smoke coverage misses two high-value guarantees: reordered inputs should keep the same key, and different paths with identical contents should not collide.
Proposed test additions
k1=$(buildcache_key "$tmp/a/x.go" "$tmp/a/y.go") k2=$(buildcache_key "$tmp/a/x.go" "$tmp/a/y.go") [ "$k1" = "$k2" ] || { echo "FAIL: key not stable"; exit 1; } + +k1_rev=$(buildcache_key "$tmp/a/y.go" "$tmp/a/x.go") +[ "$k1" = "$k1_rev" ] || { echo "FAIL: key depends on input order"; exit 1; } + +mkdir -p "$tmp/b" +cp "$tmp/a/x.go" "$tmp/b/x.go" +kp=$(buildcache_key "$tmp/a/x.go") +kq=$(buildcache_key "$tmp/b/x.go") +[ "$kp" != "$kq" ] || { echo "FAIL: key ignores file path"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/buildcache/buildcache_test.sh` around lines 16 - 23, Add two regression checks to tools/buildcache/buildcache_test.sh: (1) verify order-independence by calling buildcache_key with the same file set in different orders (e.g., buildcache_key "$tmp/a/y.go" "$tmp/a/x.go") and assert the key equals the original k1; (2) verify path sensitivity by copying file contents to a different path (e.g., cp "$tmp/a/x.go" "$tmp/b/x.go") then call buildcache_key on the new path set and assert the produced key is different from k1. Use the existing test pattern and variable names (k1, k2, k3) and same failure-reporting style to keep consistency with the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/hooks/pre-commit.sh:
- Around line 35-43: The cache path currently can abort the pre-commit due to
set -e; make cache usage best-effort by guarding failures rather than letting
them bubble up: when sourcing tools/buildcache/buildcache.sh and when calling
buildcache_key and buildcache_hit (and the other cache block around lines
79-80), run them in safe checks so any error only logs a warning and continues
(e.g., wrap source and function calls in conditional expressions or use || true
to avoid exiting, capture failures and echo a warning mentioning
PRECOMMIT_NO_CACHE and the cache KEY but do not exit with non-zero); update the
branches that call buildcache_key, buildcache_hit, and the source so they never
cause the hook to fail.
In `@tools/buildcache/buildcache.sh`:
- Around line 17-24: The cache key currently appends only the content hash to
the variable sums in the for loop (for f in "$@"), which can cause collisions
when different file paths have identical content; update that loop so each entry
includes both the file path and its SHA-256 content hash (e.g., "path hash" or
"hash path") before adding to sums (preserve the existing "missing:$f" behavior
for absent files), then continue to printf, LC_ALL=C sort and sha256sum as
before; reference the for f in "$@" loop and the sums variable to locate where
to change the entry format.
In `@tools/dev/dev.sh`:
- Around line 8-15: Validate the SERVICE variable's format before using it in
paths or sourcing: add a check right after SERVICE="${1:?usage: dev.sh
<service-name>}" to ensure SERVICE matches an allowlist regex (e.g.,
^[a-z0-9-]+$), rejecting values containing slashes or dots (../ or /) and
printing an error + exit if the check fails; apply the same validation before
any later uses (such as the CFG path and sourcing of .env.dev around the section
that currently handles lines ~27-33) so only repo-root service names composed of
lowercase letters, digits and dashes are allowed.
---
Nitpick comments:
In `@Makefile`:
- Line 150: Add a new AIR_VERSION variable at the top of the Makefile and
replace the hardcoded version in the install command: instead of "go install
github.com/air-verse/air@v1.62.0" use the AIR_VERSION variable (e.g.,
github.com/air-verse/air@$(AIR_VERSION)), so update the line that contains the
GOTOOLCHAIN ... go install github.com/air-verse/air@v1.62.0 to reference
AIR_VERSION; ensure AIR_VERSION is documented near other tool version variables
for consistency.
- Around line 129-130: Remove the chmod +x invocation from the Makefile dev
target and call the script via an interpreter instead: replace the chmod +x
tools/dev/dev.sh and ./tools/dev/dev.sh $(SERVICE) sequence with a direct
invocation like bash tools/dev/dev.sh $(SERVICE) (or sh tools/dev/dev.sh
$(SERVICE)); alternatively, set the executable bit for tools/dev/dev.sh once in
the repository and keep the existing ./tools/dev/dev.sh $(SERVICE) invocation.
Ensure the change targets the Makefile entry that runs tools/dev/dev.sh.
In `@tools/buildcache/buildcache_test.sh`:
- Around line 16-23: Add two regression checks to
tools/buildcache/buildcache_test.sh: (1) verify order-independence by calling
buildcache_key with the same file set in different orders (e.g., buildcache_key
"$tmp/a/y.go" "$tmp/a/x.go") and assert the key equals the original k1; (2)
verify path sensitivity by copying file contents to a different path (e.g., cp
"$tmp/a/x.go" "$tmp/b/x.go") then call buildcache_key on the new path set and
assert the produced key is different from k1. Use the existing test pattern and
variable names (k1, k2, k3) and same failure-reporting style to keep consistency
with the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e774d794-f225-4a7c-b666-7945372929a9
📒 Files selected for processing (8)
.claude/hooks/pre-commit.sh.gitignoreMakefiledocs/superpowers/plans/2026-05-25-dev-speed-suggestions.mdtools/buildcache/buildcache.shtools/buildcache/buildcache_test.shtools/dev/air-template.tomltools/dev/dev.sh
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- tools/dev/air-template.toml
- docs/superpowers/plans/2026-05-25-dev-speed-suggestions.md
Comment on lines
+35
to
+43
| if [ "${PRECOMMIT_NO_CACHE:-0}" != "1" ] && [ -f tools/buildcache/buildcache.sh ]; then | ||
| # shellcheck disable=SC1091 | ||
| source tools/buildcache/buildcache.sh | ||
| KEY=$(buildcache_key $STAGED_GO) | ||
| if buildcache_hit .cache precommit "$KEY"; then | ||
| echo "Pre-commit: cached (key $KEY); bypass with PRECOMMIT_NO_CACHE=1." >&2 | ||
| exit 0 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Make cache best-effort so optional acceleration never blocks commits.
This path is an optimization, but with set -e any cache computation/mark failure can abort the hook and block a commit even when lint/tests would otherwise pass. Treat cache failures as warnings and continue uncached.
Proposed hardening
if [ "${PRECOMMIT_NO_CACHE:-0}" != "1" ] && [ -f tools/buildcache/buildcache.sh ]; then
# shellcheck disable=SC1091
source tools/buildcache/buildcache.sh
- KEY=$(buildcache_key $STAGED_GO)
- if buildcache_hit .cache precommit "$KEY"; then
- echo "Pre-commit: cached (key $KEY); bypass with PRECOMMIT_NO_CACHE=1." >&2
- exit 0
+ if KEY=$(buildcache_key $STAGED_GO 2>/dev/null); then
+ if buildcache_hit .cache precommit "$KEY"; then
+ echo "Pre-commit: cached (key $KEY); bypass with PRECOMMIT_NO_CACHE=1." >&2
+ exit 0
+ fi
+ else
+ echo "Pre-commit: cache unavailable, continuing without cache." >&2
+ KEY=""
fi
fi
@@
if [ -n "$KEY" ] && declare -F buildcache_mark >/dev/null; then
- buildcache_mark .cache precommit "$KEY"
+ buildcache_mark .cache precommit "$KEY" || \
+ echo "Pre-commit: could not write cache marker; continuing." >&2
fiAlso applies to: 79-80
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/hooks/pre-commit.sh around lines 35 - 43, The cache path currently
can abort the pre-commit due to set -e; make cache usage best-effort by guarding
failures rather than letting them bubble up: when sourcing
tools/buildcache/buildcache.sh and when calling buildcache_key and
buildcache_hit (and the other cache block around lines 79-80), run them in safe
checks so any error only logs a warning and continues (e.g., wrap source and
function calls in conditional expressions or use || true to avoid exiting,
capture failures and echo a warning mentioning PRECOMMIT_NO_CACHE and the cache
KEY but do not exit with non-zero); update the branches that call
buildcache_key, buildcache_hit, and the source so they never cause the hook to
fail.
Comment on lines
+17
to
+24
| for f in "$@"; do | ||
| if [ -f "$f" ]; then | ||
| sums+=$(sha256sum "$f" | awk '{print $1}')$'\n' | ||
| else | ||
| sums+="missing:$f"$'\n' | ||
| fi | ||
| done | ||
| printf '%s' "$sums" | LC_ALL=C sort | sha256sum | awk '{print $1}' |
There was a problem hiding this comment.
Include file paths in key material to prevent false cache hits.
The cache key for existing files currently depends on content hashes only, so different staged file sets with identical content can collide and incorrectly skip verification. Include both path and content hash per entry before sorting/hashing.
Proposed fix
buildcache_key() {
local f
local sums=""
+ local h=""
for f in "$@"; do
if [ -f "$f" ]; then
- sums+=$(sha256sum "$f" | awk '{print $1}')$'\n'
+ h=$(sha256sum "$f" | awk '{print $1}')
+ sums+="${f}:${h}"$'\n'
else
- sums+="missing:$f"$'\n'
+ sums+="${f}:missing"$'\n'
fi
done
printf '%s' "$sums" | LC_ALL=C sort | sha256sum | awk '{print $1}'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for f in "$@"; do | |
| if [ -f "$f" ]; then | |
| sums+=$(sha256sum "$f" | awk '{print $1}')$'\n' | |
| else | |
| sums+="missing:$f"$'\n' | |
| fi | |
| done | |
| printf '%s' "$sums" | LC_ALL=C sort | sha256sum | awk '{print $1}' | |
| buildcache_key() { | |
| local f | |
| local sums="" | |
| local h="" | |
| for f in "$@"; do | |
| if [ -f "$f" ]; then | |
| h=$(sha256sum "$f" | awk '{print $1}') | |
| sums+="${f}:${h}"$'\n' | |
| else | |
| sums+="${f}:missing"$'\n' | |
| fi | |
| done | |
| printf '%s' "$sums" | LC_ALL=C sort | sha256sum | awk '{print $1}' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/buildcache/buildcache.sh` around lines 17 - 24, The cache key currently
appends only the content hash to the variable sums in the for loop (for f in
"$@"), which can cause collisions when different file paths have identical
content; update that loop so each entry includes both the file path and its
SHA-256 content hash (e.g., "path hash" or "hash path") before adding to sums
(preserve the existing "missing:$f" behavior for absent files), then continue to
printf, LC_ALL=C sort and sha256sum as before; reference the for f in "$@" loop
and the sums variable to locate where to change the entry format.
Comment on lines
+8
to
+15
| SERVICE="${1:?usage: dev.sh <service-name>}" | ||
| REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
| cd "$REPO_ROOT" | ||
|
|
||
| if [ ! -d "$SERVICE" ]; then | ||
| echo "dev: service '$SERVICE' not found at repo root" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Validate SERVICE format before using it in paths and source.
Directory-existence alone is insufficient. A value like ../foo can escape the repo-relative service scope and affect CFG path and .env.dev sourcing. Add an allowlist pattern for service names first (e.g., lowercase letters, digits, dashes).
Proposed hardening
SERVICE="${1:?usage: dev.sh <service-name>}"
REPO_ROOT="$(git rev-parse --show-toplevel)"
cd "$REPO_ROOT"
+
+if [[ ! "$SERVICE" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then
+ echo "dev: invalid service name '$SERVICE' (expected: lowercase letters, digits, dashes)" >&2
+ exit 1
+fi
if [ ! -d "$SERVICE" ]; then
echo "dev: service '$SERVICE' not found at repo root" >&2
exit 1
fiAlso applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/dev/dev.sh` around lines 8 - 15, Validate the SERVICE variable's format
before using it in paths or sourcing: add a check right after
SERVICE="${1:?usage: dev.sh <service-name>}" to ensure SERVICE matches an
allowlist regex (e.g., ^[a-z0-9-]+$), rejecting values containing slashes or
dots (../ or /) and printing an error + exit if the check fails; apply the same
validation before any later uses (such as the CFG path and sourcing of .env.dev
around the section that currently handles lines ~27-33) so only repo-root
service names composed of lowercase letters, digits and dashes are allowed.
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
3 participants
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.
Summary
This PR implements four independent developer experience improvements to reduce iteration time on the chat monorepo:
make sastnow skips re-running security scans when the same set of Go source files has already been verified./...make dev SERVICE=<name>target usesairfor live-reload development against the shared deps stackmake scaffold SERVICE=<name>command generates a complete worker service skeleton with all boilerplate filesAll features are opt-in and backward-compatible; existing workflows continue to work unchanged.
Key Changes
Task 1: Cache infrastructure (
tools/buildcache/)tools/buildcache/buildcache.sh— Shared library providingbuildcache_key(),buildcache_hit(), andbuildcache_mark()functions for filesystem-based verification cachingtools/buildcache/buildcache_test.sh— Smoke tests verifying cache key stability and hit/miss behavior.gitignore— Added.cache/,tmp/, and.air.*.tomlto ignore local artifactsTask 2: SAST caching
Makefile— Wrappedsast:target with cache logic: computes a hash of all Go source files +go.sum, checks for a cached result, and marks the key on success. Bypass withNO_CACHE=1Task 3: Scoped pre-commit hook
.claude/hooks/pre-commit.sh— Rewrote to detect affected services from staged file paths and scopemake testaccordingly:go.mod/go.sumorpkg/files are staged → runs full./...(blast radius safety)PRECOMMIT_NO_CACHE=1Task 4: Hot-reload dev loop
tools/dev/air-template.toml— Air configuration template with service name substitutiontools/dev/dev.sh— Launcher script that substitutes the service name into the air config, sources optional.env.dev, and execs airMakefile— Addeddevtarget requiringSERVICE=<name>, installsairin thetoolstarget, and setSHELL := /usr/bin/env bashto support buildcache array operationsTask 5 & 6: Worker service scaffolder
tools/scaffold/main.go— Generator command that validates service names (lowercase, hyphen-separated), creates the service directory tree, and renders all templates with substitutionstools/scaffold/main_test.go— Comprehensive tests covering file creation, name substitution, overwrite protection, and invalid name rejectiontools/scaffold/templates/*.tmpl— Eight templates for a complete worker service:main.go.tmpl— Service entry point with config parsing, NATS/JetStream setup, graceful shutdownbootstrap.go.tmpl— Stream bootstrap logicstore.go.tmpl— Store interface definition with mockgen directivehandler.go.tmpl— Message handler skeletonhandler_test.go.tmpl— Unit test skeletonDockerfile.tmpl— Multi-stage builddocker-compose.yml.tmpl— Local dev compose fileazure-pipelines.yml.tmpl— CI/CD pipeline templateMakefile— Addedscaffoldtarget wrapping the generatorImplementation Details
https://claude.ai/code/session_01CT5YBdEaHXDHbjFmPTPU8R
Summary by CodeRabbit
New Features
make dev SERVICE=<name>for hot-reload development with automatic service recompilation.Documentation
Chores