Gate JetStream stream creation behind BOOTSTRAP_STREAMS across services#130
Gate JetStream stream creation behind BOOTSTRAP_STREAMS across services#130
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughMakes JetStream stream creation opt-in per service via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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: 4
🧹 Nitpick comments (6)
room-worker/bootstrap_test.go (2)
50-65: Useerrors.Isto verify the underlying error is wrapped, not just substring-matched.The substring assertion confirms the wrapping prefix, but it doesn't verify that
tc.failErris preserved in the error chain. Addingassert.ErrorIs(t, err, tc.failErr)complements the substring check and aligns with the project guideline to avoid string comparisons for error semantics.♻️ Proposed refinement
if tc.wantErrSub != "" { require.Error(t, err) assert.Contains(t, err.Error(), tc.wantErrSub) + assert.ErrorIs(t, err, tc.failErr) return }As per coding guidelines: "Never compare errors by string — use
errors.Isanderrors.As".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/bootstrap_test.go` around lines 50 - 65, The test currently checks the wrapped error by substring only; update the assertion after calling bootstrapStreams to also verify the underlying error is preserved using errors.Is by adding assert.ErrorIs(t, err, tc.failErr) (in the table-driven testcase where tc.wantErrSub != ""), referencing the fakeStreamCreator failure value (tc.failErr) and the bootstrapStreams call so the test ensures the original error is in the error chain rather than relying only on string matching.
22-28: LGTM with one nit on the fake's record ordering.The fake correctly returns
(nil, f.failErr)on the configured failing stream and otherwise records the requested name. Note that on a failure case, the failing stream name is intentionally not appended tocreated— that's fine, but combined with the early-returnreturnon Line 64 the test never asserts the post-failure state ofcreated(e.g., that no other streams were attempted). For room-worker that's a non-issue today (only one stream), but if/when this fake is reused for services bootstrapping multiple streams (e.g., message-gatekeeper's MESSAGES + MESSAGES_CANONICAL), consider assertingfake.createdin the error case too to lock in fail-fast ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/bootstrap_test.go` around lines 22 - 28, The fake CreateOrUpdateStream currently returns early on a configured failure (using f.failOn) without recording the attempted stream name in f.created, which prevents tests from asserting post-failure ordering; either update the fakeStreamCreator.CreateOrUpdateStream to append cfg.Name to f.created before returning the error when cfg.Name == f.failOn, or keep the fake as-is and add assertions in the failing test to explicitly check f.created after the error to ensure no further streams were attempted (referencing fakeStreamCreator.CreateOrUpdateStream, f.failOn and f.created).CLAUDE.md (1)
218-218: Convention is well-specified.The added bullet captures the contract clearly: opt-in via
Bootstrap bootstrapConfigwithenvPrefix:"BOOTSTRAP_"andenv:"STREAMS", no-op when disabled,bootstrapStreams(ctx, js, siteID, enabled) errorhelper, andBOOTSTRAP_STREAMS=trueindeploy/docker-compose.ymlfor local dev. Production manifests omitting the var to fail-fast on missing streams is the right default.One small consistency note: the existing Section 6 still lists "Use
js.CreateOrUpdateStreamat startup — it's idempotent" at line 197 without qualification. Consider tightening that bullet to cross-reference the opt-in rule (e.g., "...at startup when bootstrap is enabled") so the two rules don't read as contradictory to a first-time reader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 218, Update the Section 6 bullet that currently reads "Use js.CreateOrUpdateStream at startup — it's idempotent" to qualify it with the opt-in bootstrap rule: state that js.CreateOrUpdateStream (or equivalent stream creation) should only be invoked at startup when bootstrap is enabled via the Bootstrap bootstrapConfig / BOOTSTRAP_STREAMS flag, and reference the bootstrapStreams(ctx, js, siteID, enabled) helper as the place that must guard calls to js.CreateOrUpdateStream so the two rules are not contradictory.notification-worker/main.go (1)
26-34: Bootstrap wiring is correct; same connection-string default concern asroom-worker(optional).The new
Bootstrapfield correctly threadsBOOTSTRAP_STREAMSinto the gating helper.Optional follow-up (these lines appear to be only realigned, not semantically modified):
NATS_URLandMONGO_URIshould be,requiredrather than have a localhostenvDefaultso a misconfigured pod fails fast in production instead of silently pointing at the wrong endpoint.message-gatekeeper/main.goalready uses the correct form and is a good template.As per coding guidelines "never default secrets or connection strings — mark them required".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-worker/main.go` around lines 26 - 34, The NatsURL and MongoURI struct tags in the config struct currently provide localhost envDefault values which can hide misconfiguration; update the tags for the fields NatsURL and MongoURI in the config type to remove the envDefault and mark them required (use `env:"NATS_URL,required"` and `env:"MONGO_URI,required"` style), leaving other fields unchanged so the process fails fast when those connection strings are not provided.room-worker/main.go (1)
23-31: Field addition looks good; consider tightening connection-string defaults while you're here (optional).The new
Bootstrap bootstrapConfig \envPrefix:"BOOTSTRAP_"`field correctly wiresBOOTSTRAP_STREAMS` into the gating helper.Optional, since these lines are pre-existing and only realigned: per the
**/main.goguideline "never default secrets or connection strings — mark them required",NATS_URLandMONGO_URIshould be,requiredrather thanenvDefault:"…localhost…". Defaulting them risks a misconfigured pod silently connecting to localhost in production. Worth a follow-up PR if you don't want to expand scope here.As per coding guidelines "Always provide
envDefaultfor non-critical config (port, database name, log level); never default secrets or connection strings — mark them required".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/main.go` around lines 23 - 31, The config struct defaults for connection strings should not silently fallback to localhost; update the struct field tags for NatsURL and MongoURI in the config type so they are required (replace the envDefault values with the `required` tag), i.e. modify the `config` struct fields `NatsURL` and `MongoURI` to use `env:"NATS_URL,required"` and `env:"MONGO_URI,required"` (leave other fields and the new `Bootstrap bootstrapConfig` unchanged).docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md (1)
526-527: The expected grep output is over-specified and brittle.Hard-coding “8 lines total … plus search-sync-worker/main.go” may fail on harmless edits. Prefer validating allowed file paths/patterns instead of exact line counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md` around lines 526 - 527, The test's grep expectation is brittle because it asserts exact line counts; update the validation to assert allowed file paths/patterns instead: ensure only helper bodies under <service>/bootstrap.go (e.g., message-gatekeeper/bootstrap.go and other services' bootstrap.go) and search-sync-worker/main.go are present and that no other top-level main.go files appear. Replace the hard-coded "8 lines total" check with pattern-based assertions that whitelist the accepted file paths (message-gatekeeper bootstrap helper calls, each service's bootstrap.go helper, and search-sync-worker/main.go) and reject any main.go outside search-sync-worker; locate the validation logic that constructs the grep/expected output check and change it to match these path patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.md`:
- Around line 63-69: The plan's test/helper snippets declare
CreateOrUpdateStream (fakeStreamCreator) and streamCreator to return
jetstream.Stream but the real instrumented client in message-worker/bootstrap.go
uses oteljetstream.Stream; update the example snippets (functions
CreateOrUpdateStream and the streamCreator type/usages in Task 2/3/4) to import
github.com/Marz32onE/instrumentation-go/otel-nats/oteljetstream and change
return types and variables from jetstream.Stream to oteljetstream.Stream (or
alternatively add a short note in the plan explaining that production services
use an OTEL-wrapped JetStream client so agentic code must target
oteljetstream.Stream).
In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md`:
- Around line 203-204: The inbox bootstrap test currently only verifies stream
Name but must enforce the "Name-only" contract: update the test scaffold that
fakes/records CreateOrUpdateStream calls so it captures the full
jetstream.StreamConfig object (not just Name), then add an assertion in the
inbox bootstrap test case that the captured StreamConfig.Subjects is nil or
empty; specifically modify the fake CreateOrUpdateStream recorder used by the
inbox-worker tests to store the entire jetstream.StreamConfig and assert
Subjects == nil/len==0 in the test (apply the same change to the other
inbox-related test snippets referenced around the same test block).
- Line 9: Update the plan's Tech Stack entry that currently reads "Go 1.25" to
the exact toolchain version "Go 1.25.8" so it matches go.mod and Dockerfiles;
search for the string "Tech Stack: Go 1.25" (or any occurrences of "Go 1.25") in
this document and replace them with "Go 1.25.8" to avoid version
mismatch/confusion.
In `@docs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.md`:
- Line 134: The spec incorrectly instructs adding unit tests in main_test.go but
the PR adds bootstrap_test.go for each service (correct because the helper is
defined in bootstrap.go); update the spec text to reference bootstrap_test.go
instead of main_test.go and mention that each service should add a table-driven
unit test file named bootstrap_test.go that tests the new helper in
bootstrap.go.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 218: Update the Section 6 bullet that currently reads "Use
js.CreateOrUpdateStream at startup — it's idempotent" to qualify it with the
opt-in bootstrap rule: state that js.CreateOrUpdateStream (or equivalent stream
creation) should only be invoked at startup when bootstrap is enabled via the
Bootstrap bootstrapConfig / BOOTSTRAP_STREAMS flag, and reference the
bootstrapStreams(ctx, js, siteID, enabled) helper as the place that must guard
calls to js.CreateOrUpdateStream so the two rules are not contradictory.
In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md`:
- Around line 526-527: The test's grep expectation is brittle because it asserts
exact line counts; update the validation to assert allowed file paths/patterns
instead: ensure only helper bodies under <service>/bootstrap.go (e.g.,
message-gatekeeper/bootstrap.go and other services' bootstrap.go) and
search-sync-worker/main.go are present and that no other top-level main.go files
appear. Replace the hard-coded "8 lines total" check with pattern-based
assertions that whitelist the accepted file paths (message-gatekeeper bootstrap
helper calls, each service's bootstrap.go helper, and
search-sync-worker/main.go) and reject any main.go outside search-sync-worker;
locate the validation logic that constructs the grep/expected output check and
change it to match these path patterns.
In `@notification-worker/main.go`:
- Around line 26-34: The NatsURL and MongoURI struct tags in the config struct
currently provide localhost envDefault values which can hide misconfiguration;
update the tags for the fields NatsURL and MongoURI in the config type to remove
the envDefault and mark them required (use `env:"NATS_URL,required"` and
`env:"MONGO_URI,required"` style), leaving other fields unchanged so the process
fails fast when those connection strings are not provided.
In `@room-worker/bootstrap_test.go`:
- Around line 50-65: The test currently checks the wrapped error by substring
only; update the assertion after calling bootstrapStreams to also verify the
underlying error is preserved using errors.Is by adding assert.ErrorIs(t, err,
tc.failErr) (in the table-driven testcase where tc.wantErrSub != ""),
referencing the fakeStreamCreator failure value (tc.failErr) and the
bootstrapStreams call so the test ensures the original error is in the error
chain rather than relying only on string matching.
- Around line 22-28: The fake CreateOrUpdateStream currently returns early on a
configured failure (using f.failOn) without recording the attempted stream name
in f.created, which prevents tests from asserting post-failure ordering; either
update the fakeStreamCreator.CreateOrUpdateStream to append cfg.Name to
f.created before returning the error when cfg.Name == f.failOn, or keep the fake
as-is and add assertions in the failing test to explicitly check f.created after
the error to ensure no further streams were attempted (referencing
fakeStreamCreator.CreateOrUpdateStream, f.failOn and f.created).
In `@room-worker/main.go`:
- Around line 23-31: The config struct defaults for connection strings should
not silently fallback to localhost; update the struct field tags for NatsURL and
MongoURI in the config type so they are required (replace the envDefault values
with the `required` tag), i.e. modify the `config` struct fields `NatsURL` and
`MongoURI` to use `env:"NATS_URL,required"` and `env:"MONGO_URI,required"`
(leave other fields and the new `Bootstrap bootstrapConfig` unchanged).
🪄 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: b376b2f2-6a39-436f-9547-1adfc0d32317
📒 Files selected for processing (33)
CLAUDE.mdbroadcast-worker/bootstrap.gobroadcast-worker/bootstrap_test.gobroadcast-worker/deploy/docker-compose.ymlbroadcast-worker/main.godocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.mddocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.mddocs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.mdinbox-worker/bootstrap.goinbox-worker/bootstrap_test.goinbox-worker/deploy/docker-compose.ymlinbox-worker/main.gomessage-gatekeeper/bootstrap.gomessage-gatekeeper/bootstrap_test.gomessage-gatekeeper/deploy/docker-compose.ymlmessage-gatekeeper/main.gomessage-worker/bootstrap.gomessage-worker/bootstrap_test.gomessage-worker/deploy/docker-compose.ymlmessage-worker/main.gonotification-worker/bootstrap.gonotification-worker/bootstrap_test.gonotification-worker/deploy/docker-compose.ymlnotification-worker/main.goroom-service/bootstrap.goroom-service/bootstrap_test.goroom-service/deploy/docker-compose.ymlroom-service/main.goroom-worker/bootstrap.goroom-worker/bootstrap_test.goroom-worker/deploy/docker-compose.ymlroom-worker/main.gosearch-sync-worker/deploy/docker-compose.yml
Joey0538
force-pushed
the
claude/audit-message-streams-uvS7l
branch
from
533c403 to
48838e0
Compare
|
Triaged the CodeRabbit review and pushed Addressed:
Deferred (not in scope):
Tests + lint green on the amended commit. Generated by Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
message-gatekeeper/bootstrap_test.go (1)
30-78: LGTM — covers ordering and both per-stream wrap messages.The success case pins both creation order (
MESSAGESthenMESSAGES_CANONICAL) and naming viaassert.Equalon the slice, and the two failure cases lock in distinct wrap prefixes pluserrors.Ison the cause. Coverage is appropriate for the helper's behavior.Optional (deferred per PR notes): for the
MESSAGES_CANONICALfailure case you could additionally assertfake.created == ["MESSAGES_test"]to make the "first stream succeeded before second failed" sequencing explicit, but it's redundant given the success case already pins order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@message-gatekeeper/bootstrap_test.go` around lines 30 - 78, No change required: the TestBootstrapStreams table-driven tests correctly validate ordering and error wrapping for bootstrapStreams using fakeStreamCreator; keep the existing assertions (including assert.Equal on fake.created and errors.Is checks) and leave the optional extra assertion for MESSAGES_CANONICAL failure as deferred per PR notes.inbox-worker/main.go (1)
149-156: LGTM — bootstrap gating with fail-fast consumer creation.When
Bootstrap.Enabled=false(production),bootstrapStreamsno-ops and the subsequentCreateOrUpdateConsumerwill fail loudly if the stream is missing — matching the PR's intended fail-fast behavior in prod.Minor nit (optional):
inboxCfgis now only used for.Name; you could inlinestream.Inbox(cfg.SiteID).Namedirectly into the consumer call to avoid the temporary, but it's fine either way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inbox-worker/main.go` around lines 149 - 156, The temporary variable inboxCfg is only used for its Name field; remove the inboxCfg assignment and inline stream.Inbox(cfg.SiteID).Name directly into the js.CreateOrUpdateConsumer call (replace inboxCfg.Name with stream.Inbox(cfg.SiteID).Name) to simplify the code while keeping the same fail-fast behavior around bootstrapStreams and CreateOrUpdateConsumer.docs/superpowers/plans/2026-04-27-inbox-stream-ownership.md (1)
1-507: Plan doc looks fine for a point-in-time record.Per the PR objectives, doc-only tweaks were intentionally deferred to keep noise down; the implementation matches the convention described here. One small thing if you ever revise: Step 2.2's "wait, that's wrong → restructure as" narrative reads as in-progress reasoning rather than final guidance — collapsing it to just the correct version would make the plan easier to follow on a second read. Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-27-inbox-stream-ownership.md` around lines 1 - 507, The plan doc's Step 2.2 contains an in-progress reasoning sentence ("But notice ... This is wrong") that should be removed so the plan reads as final guidance; edit the Step 2.2 section to delete the explanatory aside and leave only the correct bootstrap loop replacement (the version that gates stream creation with `if cfg.Bootstrap.Enabled && streamCfg.Name != inboxName { ... }`) along with the concise comment that INBOX is skipped because `inbox-worker` owns it; ensure references to `inboxBootstrapStreamConfig` are removed and the section only describes using `streamCfg` directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/superpowers/plans/2026-04-27-inbox-stream-ownership.md`:
- Around line 1-507: The plan doc's Step 2.2 contains an in-progress reasoning
sentence ("But notice ... This is wrong") that should be removed so the plan
reads as final guidance; edit the Step 2.2 section to delete the explanatory
aside and leave only the correct bootstrap loop replacement (the version that
gates stream creation with `if cfg.Bootstrap.Enabled && streamCfg.Name !=
inboxName { ... }`) along with the concise comment that INBOX is skipped because
`inbox-worker` owns it; ensure references to `inboxBootstrapStreamConfig` are
removed and the section only describes using `streamCfg` directly.
In `@inbox-worker/main.go`:
- Around line 149-156: The temporary variable inboxCfg is only used for its Name
field; remove the inboxCfg assignment and inline stream.Inbox(cfg.SiteID).Name
directly into the js.CreateOrUpdateConsumer call (replace inboxCfg.Name with
stream.Inbox(cfg.SiteID).Name) to simplify the code while keeping the same
fail-fast behavior around bootstrapStreams and CreateOrUpdateConsumer.
In `@message-gatekeeper/bootstrap_test.go`:
- Around line 30-78: No change required: the TestBootstrapStreams table-driven
tests correctly validate ordering and error wrapping for bootstrapStreams using
fakeStreamCreator; keep the existing assertions (including assert.Equal on
fake.created and errors.Is checks) and leave the optional extra assertion for
MESSAGES_CANONICAL failure as deferred per PR notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e021437-2fe3-4f05-b1cd-6464b07c471d
📒 Files selected for processing (35)
CLAUDE.mdbroadcast-worker/bootstrap.gobroadcast-worker/bootstrap_test.gobroadcast-worker/deploy/docker-compose.ymlbroadcast-worker/main.godocs/superpowers/plans/2026-04-27-inbox-stream-ownership.mddocs/superpowers/specs/2026-04-27-inbox-stream-ownership-design.mdinbox-worker/bootstrap.goinbox-worker/bootstrap_test.goinbox-worker/deploy/docker-compose.ymlinbox-worker/main.gomessage-gatekeeper/bootstrap.gomessage-gatekeeper/bootstrap_test.gomessage-gatekeeper/deploy/docker-compose.ymlmessage-gatekeeper/main.gomessage-worker/bootstrap.gomessage-worker/bootstrap_test.gomessage-worker/deploy/docker-compose.ymlmessage-worker/main.gonotification-worker/bootstrap.gonotification-worker/bootstrap_test.gonotification-worker/deploy/docker-compose.ymlnotification-worker/main.goroom-service/bootstrap.goroom-service/bootstrap_test.goroom-service/deploy/docker-compose.ymlroom-service/main.goroom-worker/bootstrap.goroom-worker/bootstrap_test.goroom-worker/deploy/docker-compose.ymlroom-worker/main.gosearch-sync-worker/deploy/docker-compose.ymlsearch-sync-worker/inbox_stream.gosearch-sync-worker/inbox_stream_test.gosearch-sync-worker/main.go
💤 Files with no reviewable changes (1)
- search-sync-worker/inbox_stream_test.go
✅ Files skipped from review due to trivial changes (8)
- room-worker/deploy/docker-compose.yml
- broadcast-worker/deploy/docker-compose.yml
- room-service/deploy/docker-compose.yml
- notification-worker/deploy/docker-compose.yml
- inbox-worker/deploy/docker-compose.yml
- search-sync-worker/deploy/docker-compose.yml
- docs/superpowers/specs/2026-04-27-inbox-stream-ownership-design.md
- room-service/main.go
🚧 Files skipped from review as they are similar to previous changes (11)
- message-gatekeeper/deploy/docker-compose.yml
- message-worker/deploy/docker-compose.yml
- room-worker/bootstrap.go
- broadcast-worker/bootstrap.go
- room-service/bootstrap_test.go
- message-worker/bootstrap_test.go
- message-worker/bootstrap.go
- broadcast-worker/bootstrap_test.go
- notification-worker/bootstrap.go
- message-gatekeeper/main.go
- message-worker/main.go
Joey0538
force-pushed
the
claude/audit-message-streams-uvS7l
branch
from
bfbe901 to
5eeb42b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
notification-worker/main.go (1)
95-100: Optional: avoid recomputingcanonicalCfgafter bootstrap.
stream.MessagesCanonical(cfg.SiteID)is already called insidebootstrapStreams, and here only.Nameis needed forCreateOrUpdateConsumer. You could inlinestream.MessagesCanonical(cfg.SiteID).Namein the consumer call (or havebootstrapStreamsreturn the resolved name) to remove the redundant derivation. Same nit applies tobroadcast-worker/main.go(lines 89‑96).Not blocking —
MessagesCanonicalis pure and cheap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-worker/main.go` around lines 95 - 100, The code recomputes canonicalCfg after calling bootstrapStreams even though bootstrapStreams already calls stream.MessagesCanonical(cfg.SiteID); to remove this redundancy either (A) inline stream.MessagesCanonical(cfg.SiteID).Name directly in the call to CreateOrUpdateConsumer where canonicalCfg.Name is used, or (B) change bootstrapStreams to return the resolved name (string) and use that return value instead of calling stream.MessagesCanonical again; update references to canonicalCfg.Name in CreateOrUpdateConsumer accordingly and apply the same minor cleanup in broadcast-worker where the same pattern appears.docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md (1)
84-87: Consider updating the error test pattern to show errors.Is verification.The test snippet shows only
assert.Contains(err.Error(), tc.wantErrSub)to verify wrapped errors. Per the PR objectives, the actual implementation already useserrors.Is+ message assertion to properly verify error wrapping, which aligns with CLAUDE.md guidance to avoid string-only comparisons.For future readers using this plan as a reference for similar tasks, consider updating the test scaffold to demonstrate the full pattern:
if tc.wantErrSub != "" { require.Error(t, err) assert.Contains(t, err.Error(), tc.wantErrSub) assert.ErrorIs(t, err, tc.creatorErr) // Verify error wrapping return }This would require adding a
failErr errorfield to the test struct and setting it in the error case. Since the implementation already follows the correct pattern, this is purely a documentation enhancement.Note: This also applies to the inbox-worker test pattern at lines 270-273 and any other similar test scaffolds in the plan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md` around lines 84 - 87, Update the failing-test branches to verify wrapped errors with errors.Is rather than only checking the message: where the code currently does require.Error(t, err) and assert.Contains(t, err.Error(), tc.wantErrSub) update to also call assert.ErrorIs(t, err, tc.failErr) (or assert.ErrorIs(t, err, tc.creatorErr) if that name is already used) and add a failErr error field to the test case struct (e.g., tc.failErr) so callers can set the expected wrapped error; apply the same change to the inbox-worker test scaffold and any other similar test patterns in this plan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md`:
- Around line 84-87: Update the failing-test branches to verify wrapped errors
with errors.Is rather than only checking the message: where the code currently
does require.Error(t, err) and assert.Contains(t, err.Error(), tc.wantErrSub)
update to also call assert.ErrorIs(t, err, tc.failErr) (or assert.ErrorIs(t,
err, tc.creatorErr) if that name is already used) and add a failErr error field
to the test case struct (e.g., tc.failErr) so callers can set the expected
wrapped error; apply the same change to the inbox-worker test scaffold and any
other similar test patterns in this plan.
In `@notification-worker/main.go`:
- Around line 95-100: The code recomputes canonicalCfg after calling
bootstrapStreams even though bootstrapStreams already calls
stream.MessagesCanonical(cfg.SiteID); to remove this redundancy either (A)
inline stream.MessagesCanonical(cfg.SiteID).Name directly in the call to
CreateOrUpdateConsumer where canonicalCfg.Name is used, or (B) change
bootstrapStreams to return the resolved name (string) and use that return value
instead of calling stream.MessagesCanonical again; update references to
canonicalCfg.Name in CreateOrUpdateConsumer accordingly and apply the same minor
cleanup in broadcast-worker where the same pattern appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7fc8d37-b5b1-4263-8a53-99930762e277
📒 Files selected for processing (38)
CLAUDE.mdbroadcast-worker/bootstrap.gobroadcast-worker/bootstrap_test.gobroadcast-worker/deploy/docker-compose.ymlbroadcast-worker/main.godocs/superpowers/plans/2026-04-27-inbox-stream-ownership.mddocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.mddocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.mddocs/superpowers/specs/2026-04-27-inbox-stream-ownership-design.mddocs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.mdinbox-worker/bootstrap.goinbox-worker/bootstrap_test.goinbox-worker/deploy/docker-compose.ymlinbox-worker/main.gomessage-gatekeeper/bootstrap.gomessage-gatekeeper/bootstrap_test.gomessage-gatekeeper/deploy/docker-compose.ymlmessage-gatekeeper/main.gomessage-worker/bootstrap.gomessage-worker/bootstrap_test.gomessage-worker/deploy/docker-compose.ymlmessage-worker/main.gonotification-worker/bootstrap.gonotification-worker/bootstrap_test.gonotification-worker/deploy/docker-compose.ymlnotification-worker/main.goroom-service/bootstrap.goroom-service/bootstrap_test.goroom-service/deploy/docker-compose.ymlroom-service/main.goroom-worker/bootstrap.goroom-worker/bootstrap_test.goroom-worker/deploy/docker-compose.ymlroom-worker/main.gosearch-sync-worker/deploy/docker-compose.ymlsearch-sync-worker/inbox_stream.gosearch-sync-worker/inbox_stream_test.gosearch-sync-worker/main.go
💤 Files with no reviewable changes (1)
- search-sync-worker/inbox_stream_test.go
✅ Files skipped from review due to trivial changes (12)
- broadcast-worker/deploy/docker-compose.yml
- search-sync-worker/deploy/docker-compose.yml
- room-service/deploy/docker-compose.yml
- inbox-worker/deploy/docker-compose.yml
- room-worker/deploy/docker-compose.yml
- message-worker/deploy/docker-compose.yml
- room-worker/bootstrap.go
- message-gatekeeper/deploy/docker-compose.yml
- message-worker/bootstrap_test.go
- docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.md
- CLAUDE.md
- inbox-worker/main.go
🚧 Files skipped from review as they are similar to previous changes (8)
- notification-worker/deploy/docker-compose.yml
- room-service/bootstrap_test.go
- notification-worker/bootstrap_test.go
- message-worker/bootstrap.go
- message-gatekeeper/bootstrap_test.go
- message-worker/main.go
- room-worker/main.go
- broadcast-worker/bootstrap.go
Seven services unconditionally call CreateOrUpdateStream at startup; in prod, streams are pre-provisioned by ops/IaC. This spec extends the search-sync-worker BOOTSTRAP_STREAMS convention to those services so prod no longer touches stream config and dev still works standalone. https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Splits the rollout across the seven services into two parts: - Part 1: message-gatekeeper, broadcast-worker, message-worker, notification-worker - Part 2: room-worker, inbox-worker, room-service + CLAUDE.md update Each task follows TDD (Red-Green-Refactor) with bite-sized steps and a single commit per service. https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Streams in production are pre-provisioned by ops/IaC. Services should
only create their own consumers, not the streams themselves. Extend the
existing search-sync-worker BOOTSTRAP_STREAMS convention to every
service that calls CreateOrUpdateStream at startup.
Each affected service grows a service-local bootstrap.go with:
- bootstrapConfig{ Enabled bool } env:"STREAMS" envDefault:"false"
- streamCreator interface (one method, oteljetstream.Stream return)
- bootstrapStreams(ctx, js, siteID, enabled) error helper
main.go wraps the existing CreateOrUpdateStream call(s) inside the helper
and gates them on cfg.Bootstrap.Enabled. Consumer creation is unchanged
and stays unconditional. Local deploy/docker-compose.yml files set
BOOTSTRAP_STREAMS=true so any service can stand up alone in dev against
a fresh NATS; production manifests omit the var and inherit the default
false.
Services updated:
- message-gatekeeper (MESSAGES + MESSAGES_CANONICAL)
- broadcast-worker, message-worker, notification-worker (MESSAGES_CANONICAL)
- room-worker, room-service (ROOMS)
- inbox-worker (INBOX, Name-only — Subjects owned by ops/IaC sourcing)
- search-sync-worker (compose flag added for local-dev parity)
CLAUDE.md is updated under "JetStream Streams" so any new service that
interacts with JetStream follows the same opt-in convention.
https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
inbox-worker takes over the INBOX stream's schema bootstrap so it has a single owner. search-sync-worker's inboxBootstrapStreamConfig was always flagged as temporary scaffolding; this spec retires it. Federation (Sources + SubjectTransforms) leaves app code entirely and belongs to ops/IaC in production. https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Five tasks: widen inbox-worker helper to Name+Subjects, skip INBOX in search-sync-worker bootstrap loop, delete inboxBootstrapStreamConfig scaffolding + its tests, document the ownership rule in CLAUDE.md, final integration verification. https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
…-worker scaffolding
inbox-worker is now the sole owner of INBOX_{siteID} schema bootstrap.
Its bootstrapStreams helper passes Name + Subjects from pkg/stream.Inbox,
matching the canonical schema. Federation (Sources + SubjectTransforms)
remains owned by ops/IaC and never appears in app code.
search-sync-worker stops bootstrapping INBOX. The dead scaffolding
inboxBootstrapStreamConfig (originally added with PR #109 and flagged
in its own doc comment as a temporary stand-in until inbox-worker
migrated) is deleted along with its dedicated unit-test file. The
RemoteSiteIDs config field is removed since its only consumer was the
deleted function. The service's bootstrap loop now skips INBOX while
continuing to create consumers for INBOX-based collections (spotlight,
user-room).
CLAUDE.md gains an explicit ownership rule under "JetStream Streams"
documenting that app code owns Name + Subjects and ops/IaC owns
Sources + SubjectTransforms.
Tests: inbox-worker bootstrap test now asserts Subjects matches
pkg/stream.Inbox(siteID).Subjects and Sources is empty (regression
guard against re-introducing federation in app code).
https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Joey0538
force-pushed
the
claude/audit-message-streams-uvS7l
branch
from
5eeb42b to
153eaa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.md`:
- Around line 63-89: Update the doc example so the local interface and test fake
match real usage: change the streamCreator method signature used by
bootstrapStreams to return oteljetstream.Stream instead of jetstream.Stream, and
update the fakeStreamCreator used in tests to record the full
jetstream.StreamConfig (e.g. created []jetstream.StreamConfig) rather than only
names; have the fake append the incoming cfg in CreateOrUpdateStream and update
the test assertions to compare the full cfgs (Subjects, Name, etc.) against
wantConfigs to ensure Subjects and other fields are validated.
In `@message-gatekeeper/main.go`:
- Around line 79-84: The startup currently gates creation/check of the canonical
stream behind bootstrapStreams and cfg.Bootstrap.Enabled which delays detection
of a missing MESSAGES_CANONICAL until first publish; update the startup logic so
that the canonical stream is validated at process start: call a dedicated check
or ensure bootstrapStreams always verifies the existence of MESSAGES_CANONICAL
(even if cfg.Bootstrap.Enabled is false) before proceeding to construct
messagesCfg via stream.Messages; if the canonical stream is missing or
inaccessible, log the error via slog.Error and exit(1) to restore fail-fast
behavior.
In `@room-service/main.go`:
- Around line 22-35: The struct currently defaults critical connection strings
and secrets; update the env tags so connection strings and credentials are
required: remove envDefault values for NatsURL and MongoURI and change their
tags to include ",required" (NatsURL, MongoURI), and remove empty defaults for
credential fields (NatsCredsFile, MongoUsername, MongoPassword, ValkeyPassword)
and mark them required as appropriate; keep non-critical defaults (e.g.,
MaxRoomSize, MaxBatchSize) as-is and retain already-required fields like
ValkeyAddr and ValkeyGracePeriod. Ensure you only modify the struct tags for
NatsURL, NatsCredsFile, MongoURI, MongoUsername, MongoPassword, and
ValkeyPassword so missing production config fails fast.
- Around line 82-85: The service must still verify the ROOMS stream at startup
even when cfg.Bootstrap.Enabled (BOOTSTRAP_STREAMS) is false; update the startup
logic so that after calling bootstrapStreams(ctx, js, cfg.SiteID,
cfg.Bootstrap.Enabled) you perform an explicit existence check for the ROOMS
stream (e.g., call a new or existing check like ensureStreamExists/checkStream
with "ROOMS" or the ROOMS constant against the JetStream client `js`) and if the
stream is missing log a clear error and exit (slog.Error + os.Exit(1)). Keep
bootstrapStreams behavior unchanged for creating streams, but always run the
ROOMS existence verification regardless of cfg.Bootstrap.Enabled to restore
fail-fast behavior before any js.Publish calls.
🪄 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: 0580eded-effa-49d0-852f-6b878e1b8f32
📒 Files selected for processing (38)
CLAUDE.mdbroadcast-worker/bootstrap.gobroadcast-worker/bootstrap_test.gobroadcast-worker/deploy/docker-compose.ymlbroadcast-worker/main.godocs/superpowers/plans/2026-04-27-inbox-stream-ownership.mddocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.mddocs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.mddocs/superpowers/specs/2026-04-27-inbox-stream-ownership-design.mddocs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.mdinbox-worker/bootstrap.goinbox-worker/bootstrap_test.goinbox-worker/deploy/docker-compose.ymlinbox-worker/main.gomessage-gatekeeper/bootstrap.gomessage-gatekeeper/bootstrap_test.gomessage-gatekeeper/deploy/docker-compose.ymlmessage-gatekeeper/main.gomessage-worker/bootstrap.gomessage-worker/bootstrap_test.gomessage-worker/deploy/docker-compose.ymlmessage-worker/main.gonotification-worker/bootstrap.gonotification-worker/bootstrap_test.gonotification-worker/deploy/docker-compose.ymlnotification-worker/main.goroom-service/bootstrap.goroom-service/bootstrap_test.goroom-service/deploy/docker-compose.ymlroom-service/main.goroom-worker/bootstrap.goroom-worker/bootstrap_test.goroom-worker/deploy/docker-compose.ymlroom-worker/main.gosearch-sync-worker/deploy/docker-compose.ymlsearch-sync-worker/inbox_stream.gosearch-sync-worker/inbox_stream_test.gosearch-sync-worker/main.go
💤 Files with no reviewable changes (1)
- search-sync-worker/inbox_stream_test.go
✅ Files skipped from review due to trivial changes (12)
- notification-worker/deploy/docker-compose.yml
- broadcast-worker/deploy/docker-compose.yml
- room-worker/deploy/docker-compose.yml
- search-sync-worker/deploy/docker-compose.yml
- message-gatekeeper/deploy/docker-compose.yml
- inbox-worker/deploy/docker-compose.yml
- room-service/deploy/docker-compose.yml
- inbox-worker/bootstrap.go
- inbox-worker/bootstrap_test.go
- message-worker/main.go
- docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.md
- docs/superpowers/specs/2026-04-27-inbox-stream-ownership-design.md
🚧 Files skipped from review as they are similar to previous changes (11)
- message-worker/deploy/docker-compose.yml
- room-worker/bootstrap_test.go
- notification-worker/bootstrap_test.go
- room-worker/bootstrap.go
- message-worker/bootstrap_test.go
- room-service/bootstrap.go
- broadcast-worker/bootstrap_test.go
- room-service/bootstrap_test.go
- message-gatekeeper/bootstrap.go
- docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part2.md
- broadcast-worker/main.go
bootstrapStreams now restores fail-fast for publisher services. When BOOTSTRAP_STREAMS=false (production path), the helper calls js.Stream() to verify each stream exists. A missing stream returns an error that main.go converts to a clean exit with a log line, so a misprovisioned deploy surfaces at startup instead of at first publish. The streamCreator interface is widened to streamManager (adds Stream method). The fake test double is renamed to fakeStreamManager and gains an existing map so tests can simulate "stream not found" cleanly. Each service's TestBootstrapStreams gains a "disabled - verifies existing stream" case (replacing the old "skips creation" case) and a "disabled - fails when stream missing" case asserting both the wrap message and errors.Is(err, jetstream.ErrStreamNotFound). https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
- room-service/main.go: NATS_URL and MONGO_URI now require env values
(no more localhost defaults). Aligns with CLAUDE.md §6 ("never default
secrets or connection strings — mark them required") and matches the
pattern already used by message-gatekeeper / message-worker. Local dev
is unaffected — docker-compose sets both explicitly.
- docs/superpowers/specs/.../optional-stream-bootstrap-design.md: update
the spec's example helper + test snippet to match what actually landed:
streamManager interface (two methods, returns oteljetstream.Stream),
fakeStreamManager test fake records full jetstream.StreamConfig, the
disabled branch verifies via Stream(), tests assert errors.Is on the
wrapped sentinel. Resolves the doc-vs-code drift CodeRabbit flagged.
https://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
|
Addressed the latest CodeRabbit review (commits Fixed (Major):
Deferred (with reasoning):
Tests + lint green on Generated by Claude Code |
3 participants
Summary
In production, JetStream streams are pre-provisioned by ops/IaC. Today every consumer service unconditionally calls
js.CreateOrUpdateStreamat startup, which can clobber ops-managed stream config. This PR extends the existingsearch-sync-workerBOOTSTRAP_STREAMSconvention (defaultfalse) to every other service that creates a stream:message-gatekeeper(MESSAGES + MESSAGES_CANONICAL)broadcast-worker,message-worker,notification-worker(MESSAGES_CANONICAL)room-worker,room-service(ROOMS)inbox-worker(INBOX,Name-only —Subjectsare owned by ops/IaC cross-site sourcing)Each affected service grows a service-local
bootstrap.gowith abootstrapConfig{ Enabled bool }struct (env prefixBOOTSTRAP_, defaultfalse), a one-methodstreamCreatorinterface, and abootstrapStreams(ctx, js, siteID, enabled) errorhelper.main.gocalls the helper instead of inliningCreateOrUpdateStream. Consumer creation stays unconditional.Local
deploy/docker-compose.ymlfor all eight services (the seven gated above plussearch-sync-worker) setsBOOTSTRAP_STREAMS=trueso any service can stand up alone in dev against a fresh NATS. Production manifests omit the var and inherit the defaultfalse—CreateOrUpdateConsumerwill fail fast at startup if ops/IaC hasn't provisioned the stream yet, which is the desired behavior.CLAUDE.mdis updated under "JetStream Streams" so any new service follows the convention.Test plan
bootstrap_test.go) cover the gate:Enabled=falseno-ops;Enabled=truecreates the expected stream(s); creator error wraps withfmt.Errorf("create <NAME> stream: %w", err).make test— green across the monorepo.make lint— 0 issues.grep envDefault:"false"confirms every service ships the production-safe default.grep BOOTSTRAP_STREAMS=trueconfirms every local docker-compose enables bootstrap for dev.grep CreateOrUpdateStreamconfirms no inline calls remain outside the gatedbootstrap.gohelpers andsearch-sync-worker/main.go(which already gated itself).make uplocally to confirm the stack still bootstraps cleanly with the new env vars.Spec:
docs/superpowers/specs/2026-04-27-optional-stream-bootstrap-design.mdPlan:
docs/superpowers/plans/2026-04-27-optional-stream-bootstrap-part1.md,…-part2.mdhttps://claude.ai/code/session_015Cu3UPeWDU4DaJwP7JZtvc
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests