feat(pkg): natsrouter improvements + mongoutil bulk API + new minioutil#157
Merged
feat(pkg): natsrouter improvements + mongoutil bulk API + new minioutil#157
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (6)
📝 WalkthroughWalkthroughThis PR integrates three major feature areas: (1) extraction of generic MongoDB utilities into a shared ChangesMongoutil Package Extraction & Bulk-Write APIs
Minioutil MinIO JSON Bucket Wrapper
Natsrouter Admission Control & Timeouts
Supporting Infrastructure & Model Updates
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md (3)
191-196: 💤 Low valueAdd language identifier to fenced code block.
The code block showing the layering pattern (lines 192-196) is missing a language identifier, which triggers a markdownlint warning. Since this is a conceptual diagram rather than executable code, use the
textidentifier:-``` +```text BulkUpsertByID(items, idFn) → BulkUpsert(items, func(x) any { return bson.M{"_id": idFn(x)} }) → BulkWrite(buildModels(items, filterFn))<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md
around lines 191 - 196, The fenced code block showing the layering pattern lacks
a language identifier; update the block that contains BulkUpsertByID,
BulkUpsert, BulkWrite, buildModels and filterFn to use thetextlanguage tag
(i.e., change the openingtotext) so markdownlint stops warning while
keeping the conceptual diagram unchanged.</details> --- `122-122`: _⚡ Quick win_ **Nil pointer risk with empty-input `(nil, nil)` contract.** The documented contract states that `BulkWrite`, `BulkUpsert`, and `BulkUpsertByID` return `(nil, nil)` on empty input. This creates a nil pointer dereference risk if callers access fields like `res.Upserted` without first checking `res != nil`. While line 526-527 notes this was flagged by reviewers and kept as-is, consider documenting a clear usage pattern in the godoc, such as: ```go // Safe usage pattern: res, err := col.BulkUpsert(ctx, items, filterFn) if err != nil { return err } if res != nil { log.Infof("upserted %d items", res.Upserted) } ``` Alternatively, return `&BulkResult{}` (zero-valued struct) instead of `nil` to eliminate the nil-check requirement while maintaining the same semantic meaning (zero operations performed). Also applies to: 209-210 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md` at line 122, The current godoc contract for BulkWrite, BulkUpsert, and BulkUpsertByID returns (nil, nil) on empty input which forces callers to nil-check res before accessing fields like res.Upserted; instead, change the empty-input behavior to return a zero-valued &BulkResult{} (not nil) so callers can safely inspect fields without a nil pointer check, and update the godoc for BulkWrite/BulkUpsert/BulkUpsertByID to show the safe usage pattern (or note that callers need not check res for nil when empty input returns a zero BulkResult); ensure references to BulkResult and examples mentioning res.Upserted are updated accordingly. ``` </details> --- `446-452`: _💤 Low value_ **Optional: Minor style improvement for readability.** The out-of-scope list has multiple consecutive bullets starting with "MinIO". Consider varying the sentence structure for better readability: ```diff - MinIO presigned URLs (`PresignPut` / `PresignGet`). - MinIO multipart upload helpers. - MinIO object metadata / tagging / versioning / SSE. - MinIO event notifications. - MinIO custom TLS (mTLS, custom CA) — callers needing it construct `*minio.Client` themselves. + MinIO presigned URLs (`PresignPut` / `PresignGet`). + Multipart upload helpers. + Object metadata / tagging / versioning / SSE. + Event notifications. + Custom TLS (mTLS, custom CA) — callers needing it construct `*minio.Client` themselves. ``` Since all items in this section relate to MinIO, the repeated prefix is redundant. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md` around lines 446 - 452, Rewrite the consecutive bullets that all begin with "MinIO" to remove the repetitive prefix and improve readability: e.g., change items mentioning "MinIO presigned URLs (`PresignPut` / `PresignGet`)", "MinIO multipart upload helpers", "MinIO object metadata / tagging / versioning / SSE", "MinIO event notifications", "MinIO custom TLS (mTLS, custom CA)", "MinIO region option", and "MinIO bucket creation" into varied phrasing such as "Presigned URLs (`PresignPut` / `PresignGet`)", "Multipart upload helpers", "Object metadata/tagging/versioning and SSE", "Event notifications", "Custom TLS (mTLS, custom CA) — callers needing it construct `*minio.Client` themselves", "Region option (note: `minio-go` defaults to `us-east-1`)", and "Bucket creation — provisioned by ops; `NewBucket` only verifies existence" so the list is concise and avoids repeated "MinIO" prefixes. ``` </details> </blockquote></details> <details> <summary>pkg/natsrouter/integration_test.go (1)</summary><blockquote> `280-312`: _💤 Low value_ **LGTM — but consider nudging the second `Register` ahead of the first request for clarity.** The test relies on the spawn-site backstop releasing the semaphore slot and the WaitGroup decrement so the follow-up `ok.{id}` request succeeds. That's the right thing to verify. One small readability tweak: `Register(r, "ok.{id}", ...)` is currently registered *after* the panic round-trip (lines 302-305), which is fine on `*otelnats.Conn` (the inbox round-trip in `nc.Request` flushes pending SUBs), but it reads as if the subscription might not be live yet. Registering both routes up front would avoid the question without changing test semantics. <details> <summary>♻️ Optional reorder</summary> ```diff r := natsrouter.New(nc, "integration-panic-backstop", natsrouter.WithMaxConcurrency(2)) natsrouter.Register(r, "boom.{id}", func(c *natsrouter.Context, req echoReq) (*echoResp, error) { panic("intentional handler panic") }) + natsrouter.Register(r, "ok.{id}", + func(c *natsrouter.Context, req echoReq) (*echoResp, error) { + return &echoResp{Seq: req.Seq}, nil + }) // Panicking request must receive a reply (not time out) and the // reply must indicate an error. data, _ := json.Marshal(echoReq{Seq: 1}) resp, err := nc.Request(context.Background(), "boom.1", data, 5*time.Second) require.NoError(t, err, "panicking handler should still produce a reply via backstop") var payload map[string]any require.NoError(t, json.Unmarshal(resp.Data, &payload)) assert.Equal(t, "internal error", payload["error"], "expected internal error reply from backstop") // Process survived: a follow-up normal request must succeed. - natsrouter.Register(r, "ok.{id}", - func(c *natsrouter.Context, req echoReq) (*echoResp, error) { - return &echoResp{Seq: req.Seq}, nil - }) data, _ = json.Marshal(echoReq{Seq: 2}) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/natsrouter/integration_test.go` around lines 280 - 312, Move the follow-up subscription so both routes are registered before sending the first request: in TestIntegration_SpawnSitePanicBackstop register the "ok.{id}" handler (the natsrouter.Register call that returns echoResp) prior to performing the nc.Request to "boom.1"; keep the panic handler registration for "boom.{id}" as-is and ensure the order change only reorders the Register(r, "ok.{id}", ...) call (no other logic changes). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@pkg/minioutil/minio_integration_test.go:
- Around line 192-206: The test TestIntegration_List_DefaultCap currently seeds
only 5 objects so it never triggers the fallback cap; update the test to insert
defaultListCap+1 objects (use the same Put loop but iterate to defaultListCap+1)
before calling b.List(ctx, "k-", 0) and then assert that the returned keys
length equals defaultListCap to verify the List fallback cap behavior; reference
NewBucket, b.Put, b.List and the defaultListCap constant when making the change.In
@pkg/minioutil/minio.go:
- Around line 194-198: The startup probe in Connect uses client.ListBuckets
which requires account-wide s3:ListAllMyBuckets and is too broad; instead
perform a bucket-scoped existence/permission check (the same used by NewBucket)
against the configured bucket name so least-privilege credentials succeed.
Replace the ListBuckets probe with a bucket-scoped check such as calling the
existing BucketExists (or equivalent Head/Stat on the configured bucket) using
the probeCtx and the configured bucket variable, and return the wrapped error
only if that bucket-scoped check fails; remove the ListBuckets call so Connect
no longer demands ListAllMyBuckets permission.
Nitpick comments:
In@docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md:
- Around line 191-196: The fenced code block showing the layering pattern lacks
a language identifier; update the block that contains BulkUpsertByID,
BulkUpsert, BulkWrite, buildModels and filterFn to use thetextlanguage tag
(i.e., change the openingtotext) so markdownlint stops warning while
keeping the conceptual diagram unchanged.- Line 122: The current godoc contract for BulkWrite, BulkUpsert, and
BulkUpsertByID returns (nil, nil) on empty input which forces callers to
nil-check res before accessing fields like res.Upserted; instead, change the
empty-input behavior to return a zero-valued &BulkResult{} (not nil) so callers
can safely inspect fields without a nil pointer check, and update the godoc for
BulkWrite/BulkUpsert/BulkUpsertByID to show the safe usage pattern (or note that
callers need not check res for nil when empty input returns a zero BulkResult);
ensure references to BulkResult and examples mentioning res.Upserted are updated
accordingly.- Around line 446-452: Rewrite the consecutive bullets that all begin with
"MinIO" to remove the repetitive prefix and improve readability: e.g., change
items mentioning "MinIO presigned URLs (PresignPut/PresignGet)", "MinIO
multipart upload helpers", "MinIO object metadata / tagging / versioning / SSE",
"MinIO event notifications", "MinIO custom TLS (mTLS, custom CA)", "MinIO region
option", and "MinIO bucket creation" into varied phrasing such as "Presigned
URLs (PresignPut/PresignGet)", "Multipart upload helpers", "Object
metadata/tagging/versioning and SSE", "Event notifications", "Custom TLS (mTLS,
custom CA) — callers needing it construct*minio.Clientthemselves", "Region
option (note:minio-godefaults tous-east-1)", and "Bucket creation —
provisioned by ops;NewBucketonly verifies existence" so the list is concise
and avoids repeated "MinIO" prefixes.In
@pkg/natsrouter/integration_test.go:
- Around line 280-312: Move the follow-up subscription so both routes are
registered before sending the first request: in
TestIntegration_SpawnSitePanicBackstop register the "ok.{id}" handler (the
natsrouter.Register call that returns echoResp) prior to performing the
nc.Request to "boom.1"; keep the panic handler registration for "boom.{id}"
as-is and ensure the order change only reorders the Register(r, "ok.{id}", ...)
call (no other logic changes).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `e7be9d74-7494-47fc-a9fc-5754bbd4f149` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 394cacdd4e7e085bce5ca73a46a95dd0646e84f1 and b4ebfbd05c5295b499003e869b23ad0177ec7e6c. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (42)</summary> * `docs/superpowers/plans/2026-04-30-natsrouter-improvements.md` * `docs/superpowers/plans/2026-05-06-mongoutil-extension-and-minioutil.md` * `docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.md` * `go.mod` * `history-service/internal/mongorepo/collection.go` * `history-service/internal/mongorepo/setup_test.go` * `history-service/internal/mongorepo/subscription.go` * `history-service/internal/mongorepo/subscription_test.go` * `history-service/internal/mongorepo/threadroom.go` * `history-service/internal/mongorepo/threadroom_test.go` * `history-service/internal/service/mocks/mock_repository.go` * `history-service/internal/service/service.go` * `history-service/internal/service/threads.go` * `history-service/internal/service/threads_test.go` * `pkg/minioutil/minio.go` * `pkg/minioutil/minio_integration_test.go` * `pkg/minioutil/minio_test.go` * `pkg/mongoutil/bulk.go` * `pkg/mongoutil/bulk_integration_test.go` * `pkg/mongoutil/bulk_test.go` * `pkg/mongoutil/collection.go` * `pkg/mongoutil/collection_integration_test.go` * `pkg/mongoutil/empty_input_test.go` * `pkg/mongoutil/options.go` * `pkg/mongoutil/options_test.go` * `pkg/mongoutil/pagination.go` * `pkg/mongoutil/pagination_test.go` * `pkg/natsrouter/README.md` * `pkg/natsrouter/context.go` * `pkg/natsrouter/context_test.go` * `pkg/natsrouter/doc.go` * `pkg/natsrouter/errors.go` * `pkg/natsrouter/errors_test.go` * `pkg/natsrouter/example_test.go` * `pkg/natsrouter/integration_test.go` * `pkg/natsrouter/middleware.go` * `pkg/natsrouter/middleware_test.go` * `pkg/natsrouter/router.go` * `pkg/natsrouter/router_test.go` * `pkg/testutil/minio.go` * `pkg/testutil/testimages/testimages.go` * `search-service/main.go` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * history-service/internal/mongorepo/collection.go * history-service/internal/mongorepo/subscription_test.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 7, 2026
Per CodeRabbit review on PR #157: (MAJOR) Connect's ListBuckets probe required s3:ListAllMyBuckets (an account-wide IAM permission). Real production deployments scope credentials to the one bucket the service uses (s3:ListBucket on that bucket only), so the probe would force callers to grant broader IAM than they actually need. NewBucket already performs a bucket-scoped probe via client.BucketExists (which only requires s3:ListBucket on that bucket), so removing the ListBuckets probe is a strict improvement: less IAM required, and the fail-fast-on-misconfig contract is preserved -- it just lives in NewBucket where it's bucket-scoped. (MINOR) Renamed TestIntegration_List_DefaultCap to TestIntegration_List_ZeroMaxKeysReturnsAll. The original name implied the test exercised the 1000-key cap engaging, but it only seeded 5 keys. Renamed + added a comment explaining why we don't seed 1001 keys (~30s of CI Put traffic for marginal coverage; the cap-engages behavior is exercised at small N by TestIntegration_List_MaxKeysCap). (NIT) Added 'text' language identifier to the layering code block in the spec to silence markdownlint.
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 7, 2026
… fixups Updates the spec to reflect what actually shipped after CodeRabbit review and code-quality fixups landed on PR #157: - Connect: dropped ListBuckets probe entirely. Spec now documents the construct-only behavior and explains the IAM rationale (ListBuckets requires s3:ListAllMyBuckets account-wide; NewBucket's BucketExists is bucket-scoped and sufficient). Resolution log entries (Q8 and the earlier bullet) updated to point at the new "Post-merge amendments" section. - Public API: Bucket[T] now lists Raw() and Name() in the main API section (previously only mentioned in the round-2 resolution log). - New "Post-merge amendments" section documents five changes that landed after the round-2 log was finalized: Connect probe removal, List MaxKeys plumbing, goleak.IgnoreCurrent baseline, Get's "two round trips" godoc correction, bsonSetWithoutID move + error wrapping. Each entry explains the rationale and cites the commit. No code change.
GITMateuszCharczuk
force-pushed
the
claude/review-history-service-KWjho
branch
from
d528378 to
3126cd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/testutil/minio.go (1)
39-40: ⚡ Quick winDon’t silently discard
Terminateerrors in init failure paths.Both cleanup calls ignore errors without comment, which hides container cleanup failures. Please either join/wrap terminate errors into
minioInitError add an explicit comment that cleanup is best-effort.♻️ Suggested adjustment
- _ = container.Terminate(ctx) - minioInitErr = fmt.Errorf("get minio endpoint: %w", err) + if termErr := container.Terminate(ctx); termErr != nil { + minioInitErr = fmt.Errorf("get minio endpoint: %w (terminate minio: %v)", err, termErr) + return + } + minioInitErr = fmt.Errorf("get minio endpoint: %w", err) return ... - _ = container.Terminate(ctx) - minioInitErr = fmt.Errorf("connect minio: %w", err) + if termErr := container.Terminate(ctx); termErr != nil { + minioInitErr = fmt.Errorf("connect minio: %w (terminate minio: %v)", err, termErr) + return + } + minioInitErr = fmt.Errorf("connect minio: %w", err) returnAs per coding guidelines: "Never ignore errors silently — comment if intentionally discarded."
Also applies to: 48-49
🤖 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 `@pkg/testutil/minio.go` around lines 39 - 40, When init fails we currently call container.Terminate(ctx) and discard its error; update the failure path so Terminate's error is not silently dropped: capture the error returned by container.Terminate(ctx) and either wrap it into the existing minioInitErr (e.g. combine with fmt.Errorf or errors.Join) or append a short explicit comment stating the call is best-effort and its error may be ignored; apply the same change for the other cleanup call that currently ignores its return (the second container.Terminate usage) so both Terminate errors are handled or explicitly documented.pkg/natsrouter/integration_test.go (1)
280-312: ⚡ Quick winMake the panic-backstop test actually prove cleanup.
With
WithMaxConcurrency(2), a leaked semaphore slot still leaves enough capacity for the follow-up"ok"request, and this test never observesShutdownto provewg.Done()ran. Using a cap of1, or assertingShutdownreturns after the panic path, would make the regression match what the comment claims.🤖 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 `@pkg/natsrouter/integration_test.go` around lines 280 - 312, The test TestIntegration_SpawnSitePanicBackstop currently uses natsrouter.New(..., natsrouter.WithMaxConcurrency(2)) so a leaked semaphore slot can still allow the follow-up "ok" request to succeed; change the concurrency cap to 1 (use natsrouter.WithMaxConcurrency(1)) so a leaked slot blocks the second request and the test actually verifies cleanup, or alternatively add an explicit check that r.Shutdown(...) completes after the panic path to assert wg.Done() was called; update the invocation in the test to use WithMaxConcurrency(1) (or call r.Shutdown and assert it returns) and adjust assertions accordingly.
🤖 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 `@pkg/natsrouter/router.go`:
- Around line 258-285: The closeLoop can break on ctx.Done() while some
subscriptions are still draining, so we must not start r.wg.Wait() unless we
know no more r.wg.Add(1) calls are possible; change the code around the
closeLoop/WaitGroup section to record whether the closeLoop completed normally
(e.g. set a flag when the for loop finishes without hitting the ctx.Done() case)
and only create the wgDone goroutine and call r.wg.Wait() when that flag is
true; if the loop broke due to ctx.Done(), skip the wg.Wait() path and just
append the ctx error (as already done) so we avoid the Add/Wait race on r.wg
(referencing closed, subs, closeLoop and r.wg).
---
Nitpick comments:
In `@pkg/natsrouter/integration_test.go`:
- Around line 280-312: The test TestIntegration_SpawnSitePanicBackstop currently
uses natsrouter.New(..., natsrouter.WithMaxConcurrency(2)) so a leaked semaphore
slot can still allow the follow-up "ok" request to succeed; change the
concurrency cap to 1 (use natsrouter.WithMaxConcurrency(1)) so a leaked slot
blocks the second request and the test actually verifies cleanup, or
alternatively add an explicit check that r.Shutdown(...) completes after the
panic path to assert wg.Done() was called; update the invocation in the test to
use WithMaxConcurrency(1) (or call r.Shutdown and assert it returns) and adjust
assertions accordingly.
In `@pkg/testutil/minio.go`:
- Around line 39-40: When init fails we currently call container.Terminate(ctx)
and discard its error; update the failure path so Terminate's error is not
silently dropped: capture the error returned by container.Terminate(ctx) and
either wrap it into the existing minioInitErr (e.g. combine with fmt.Errorf or
errors.Join) or append a short explicit comment stating the call is best-effort
and its error may be ignored; apply the same change for the other cleanup call
that currently ignores its return (the second container.Terminate usage) so both
Terminate errors are handled or explicitly documented.
🪄 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: dd904f0e-03c7-4b83-8d5b-63ca6567d2ba
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
docs/superpowers/plans/2026-04-30-natsrouter-improvements.mddocs/superpowers/plans/2026-05-06-mongoutil-extension-and-minioutil.mddocs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.mdgo.modhistory-service/internal/mongorepo/collection.gohistory-service/internal/mongorepo/setup_test.gohistory-service/internal/mongorepo/subscription.gohistory-service/internal/mongorepo/subscription_test.gohistory-service/internal/mongorepo/threadroom.gohistory-service/internal/mongorepo/threadroom_test.gohistory-service/internal/service/mocks/mock_repository.gohistory-service/internal/service/service.gohistory-service/internal/service/threads.gohistory-service/internal/service/threads_test.gopkg/minioutil/minio.gopkg/minioutil/minio_integration_test.gopkg/minioutil/minio_test.gopkg/mongoutil/bulk.gopkg/mongoutil/bulk_integration_test.gopkg/mongoutil/bulk_test.gopkg/mongoutil/collection.gopkg/mongoutil/collection_integration_test.gopkg/mongoutil/empty_input_test.gopkg/mongoutil/options.gopkg/mongoutil/options_test.gopkg/mongoutil/pagination.gopkg/mongoutil/pagination_test.gopkg/natsrouter/README.mdpkg/natsrouter/context.gopkg/natsrouter/context_test.gopkg/natsrouter/doc.gopkg/natsrouter/errors.gopkg/natsrouter/errors_test.gopkg/natsrouter/example_test.gopkg/natsrouter/integration_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/natsrouter/router.gopkg/natsrouter/router_test.gopkg/testutil/minio.gopkg/testutil/testimages/testimages.gosearch-service/main.go
💤 Files with no reviewable changes (2)
- history-service/internal/mongorepo/subscription_test.go
- history-service/internal/mongorepo/collection.go
✅ Files skipped from review due to trivial changes (10)
- pkg/mongoutil/pagination.go
- pkg/mongoutil/options.go
- pkg/minioutil/minio_test.go
- pkg/mongoutil/pagination_test.go
- pkg/natsrouter/errors_test.go
- go.mod
- pkg/minioutil/minio_integration_test.go
- pkg/mongoutil/empty_input_test.go
- pkg/natsrouter/README.md
- pkg/mongoutil/bulk_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/testutil/testimages/testimages.go
- history-service/internal/mongorepo/setup_test.go
- pkg/mongoutil/options_test.go
- pkg/natsrouter/context.go
- pkg/natsrouter/example_test.go
- search-service/main.go
- history-service/internal/service/service.go
- history-service/internal/mongorepo/threadroom_test.go
- history-service/internal/service/threads_test.go
- history-service/internal/service/mocks/mock_repository.go
- pkg/natsrouter/router_test.go
- pkg/mongoutil/bulk.go
- pkg/mongoutil/collection_integration_test.go
- history-service/internal/mongorepo/threadroom.go
- pkg/minioutil/minio.go
- history-service/internal/mongorepo/subscription.go
- pkg/natsrouter/errors.go
- pkg/natsrouter/middleware.go
- pkg/mongoutil/collection.go
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 7, 2026
… test fidelity; document best-effort cleanup CodeRabbit review on PR #157 surfaced three items on the squashed branch: (MAJOR) pkg/natsrouter/router.go — Shutdown's wg.Wait() phase could race with Add(1) calls from still-draining subscriptions when the closeLoop broke early on ctx.Done(). Per sync.WaitGroup docs, Add concurrent with Wait is undefined when the counter is zero (panic risk). Fix: track allClosed; only enter the wg.Wait block when every subscription confirmed close. If ctx expired before all subs closed, surface the error and let the caller's deadline take precedence -- remaining handler goroutines continue in the background until process exit. Comment block rewritten to explain the invariant. (NIT) pkg/natsrouter/integration_test.go — TestIntegration_SpawnSite- PanicBackstop used WithMaxConcurrency(2). With cap=2, a leaked semaphore slot would still leave capacity for the follow-up "ok" request, masking a cleanup regression. Switched to cap=1 so a leaked slot blocks the second request and the test actually observes slot release. Added a comment explaining why cap=1 is load-bearing. (NIT) pkg/testutil/minio.go — replaced two `_ = container.Terminate(ctx)` calls with explicit "best-effort cleanup" comments per CLAUDE.md "never ignore errors silently — comment if intentionally discarded". The discarded errors are intentional (init already failed; Docker reaps the container on test-process exit either way), now documented.
GITMateuszCharczuk
force-pushed
the
claude/review-history-service-KWjho
branch
from
7069b84 to
c3dce7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/mongoutil/bulk.go (1)
22-28: 💤 Low valueReturn concrete types instead of
mongo.WriteModelinterfaceThe guideline "Accept interfaces, return structs" recommends returning concrete types rather than interfaces. Both
UpsertModelandDeleteModelshould return*mongo.UpdateOneModeland*mongo.DeleteOneModelrespectively. Callers collecting these into[]mongo.WriteModelwill implicitly convert the concrete types.Proposed change
-func UpsertModel(filter, update any) mongo.WriteModel { +func UpsertModel(filter, update any) *mongo.UpdateOneModel { return mongo.NewUpdateOneModel().SetFilter(filter).SetUpdate(update).SetUpsert(true) } -func DeleteModel(filter any) mongo.WriteModel { +func DeleteModel(filter any) *mongo.DeleteOneModel { return mongo.NewDeleteOneModel().SetFilter(filter) }🤖 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 `@pkg/mongoutil/bulk.go` around lines 22 - 28, Change the function signatures to return concrete pointer types instead of the interface: have UpsertModel return *mongo.UpdateOneModel and DeleteModel return *mongo.DeleteOneModel; keep the body as-is (mongo.NewUpdateOneModel()... and mongo.NewDeleteOneModel()...) so callers that collect into []mongo.WriteModel will implicitly convert the concrete pointers to the interface. Update the function declarations for UpsertModel and DeleteModel to use the concrete return types and adjust any local usage if any static typing errors appear.pkg/natsrouter/integration_test.go (1)
303-305: ⚡ Quick winUse a typed error payload instead of
map[string]anyAt Line 303, decoding into
map[string]anyweakens contract checks for NATS JSON payloads in this test path.Proposed refactor
- var payload map[string]any - require.NoError(t, json.Unmarshal(resp.Data, &payload)) - assert.Equal(t, "internal error", payload["error"], "expected internal error reply from backstop") + type panicErrResp struct { + Error string `json:"error"` + } + var payload panicErrResp + require.NoError(t, json.Unmarshal(resp.Data, &payload)) + assert.Equal(t, "internal error", payload.Error, "expected internal error reply from backstop")As per coding guidelines: “All NATS payloads must be JSON — use encoding/json with typed structs … never
map[string]interface{}”.🤖 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 `@pkg/natsrouter/integration_test.go` around lines 303 - 305, Replace the untyped map decode with a concrete struct for the error payload: define a local type (e.g., ErrorPayload with field Error string `json:"error"`), unmarshal resp.Data into that struct (replacing the variable payload map[string]any and the json.Unmarshal call), keep the require.NoError assertion, and then assert on the ErrorPayload.Error value (instead of payload["error"]) to enforce the JSON contract for NATS replies in the test where resp.Data is validated.
🤖 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 `@pkg/natsrouter/integration_test.go`:
- Around line 350-356: The test spawns inflight goroutines that call nc.Request
but currently ignores returned errors and never waits for completion; update the
block around inflight, the anonymous goroutine, and the nc.Request call to
collect each request outcome and join the goroutines (e.g., use a sync.WaitGroup
and an error channel or a results slice) so any error from nc.Request (or
marshal) is recorded and asserted in the test instead of being discarded; ensure
you close/collect the channel and fail the test if any request returned an error
so no tail goroutines remain past the assertion boundary.
---
Nitpick comments:
In `@pkg/mongoutil/bulk.go`:
- Around line 22-28: Change the function signatures to return concrete pointer
types instead of the interface: have UpsertModel return *mongo.UpdateOneModel
and DeleteModel return *mongo.DeleteOneModel; keep the body as-is
(mongo.NewUpdateOneModel()... and mongo.NewDeleteOneModel()...) so callers that
collect into []mongo.WriteModel will implicitly convert the concrete pointers to
the interface. Update the function declarations for UpsertModel and DeleteModel
to use the concrete return types and adjust any local usage if any static typing
errors appear.
In `@pkg/natsrouter/integration_test.go`:
- Around line 303-305: Replace the untyped map decode with a concrete struct for
the error payload: define a local type (e.g., ErrorPayload with field Error
string `json:"error"`), unmarshal resp.Data into that struct (replacing the
variable payload map[string]any and the json.Unmarshal call), keep the
require.NoError assertion, and then assert on the ErrorPayload.Error value
(instead of payload["error"]) to enforce the JSON contract for NATS replies in
the test where resp.Data is validated.
🪄 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: 7c7bc55e-d3ca-4f1c-bb99-f5eb48f65b28
📒 Files selected for processing (8)
pkg/minioutil/minio.gopkg/model/employee.gopkg/mongoutil/bulk.gopkg/mongoutil/collection.gopkg/mongoutil/options.gopkg/natsrouter/integration_test.gopkg/natsrouter/router.gopkg/testutil/minio.go
✅ Files skipped from review due to trivial changes (1)
- pkg/mongoutil/options.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/testutil/minio.go
- pkg/minioutil/minio.go
- pkg/mongoutil/collection.go
- pkg/natsrouter/router.go
4 tasks
…n + HandlerTimeout Five improvements to pkg/natsrouter, designed and reviewed via docs/superpowers/plans/2026-04-30-natsrouter-improvements.md: - WithMaxConcurrency option + ErrUnavailable error: bounds in-flight handler goroutines with a configurable cap (default unbounded / gin-style; set to a positive int to throttle). Excess requests get a fast ErrUnavailable reply rather than blocking the consumer. - Spawn-site panic backstop: a recover() wrapper around the spawned handler goroutine releases the semaphore slot and decrements the in-flight WaitGroup so a panicking handler can't deadlock the router or leak permits. The backstop also publishes a "internal error" reply so the requester sees a response rather than a timeout. - Shutdown waits for in-flight handlers: r.Shutdown(ctx) drains the in-flight WaitGroup before returning, with the supplied ctx bounding the wait. Prevents the long-standing race where nc.Drain() returns before handler goroutines have finished writing their replies. - HandlerTimeout middleware: per-handler deadline that returns ErrTimeout when the inner handler runs past its budget. Composes cleanly with the spawn-site backstop. - Default() constructor + doc.go example + README rewrite: newcomers can now read pkg/natsrouter/README.md end-to-end and build a working service without bouncing through the older spec docs. Reviewed across multiple rounds by bug, architecture, and NATS-expert agents; consolidated errata applied. Integration tests exercise the admission cap, the panic backstop (process survives + follow-up requests succeed), and the shutdown wait.
Two small fixes to search-service/main.go surfaced during the natsrouter shutdown work: - Shutdown now waits for the natsrouter's in-flight handlers before draining nc, closing the previous race where requests arriving during shutdown could be dropped after the consumer drained but before handler goroutines finished. - Rename Header() -> GetHeader() to match the *otelnats.Conn idiom the rest of the codebase uses.
… API
Two changes that share the same package boundary:
1. PROMOTION (refactor). Move the generic Mongo helpers out of
history-service/internal/mongorepo/ into the shared pkg/mongoutil:
- Collection[T] generic wrapper (FindOne / FindByID / FindMany
with (nil, nil) not-found semantics; Aggregate; AggregatePaged
with $facet pagination + 16 MB caveat documented; Raw escape
hatch).
- OffsetPageRequest / OffsetPage[T] / EmptyPage[T] /
NewOffsetPageRequest pagination types.
- QueryOption + WithProjection / WithSort / WithLimit / WithSkip
functional options.
history-service/internal/mongorepo retains domain-specific repos
(subscription, threadroom, pipelines) and flips imports to
mongoutil. The TestCollection_* integration tests extract from
subscription_test.go into pkg/mongoutil/collection_integration_test.go;
subscription_test.go keeps only TestSubscriptionRepo_*. New
mongorepo/setup_test.go is a thin testutil.MongoDB wrapper used by
the remaining domain tests. Mock file regenerated to use the new
imports. No logic changes to the domain code -- pure relocation
plus reference flips.
2. BULK-WRITE API (new). Three layers mirroring FindOne/FindByID:
- BulkWrite (foundation): wraps mongo-driver's BulkWrite with
explicit SetOrdered(false), short-circuits on empty input to
(nil, nil), preserves the partial-success *BulkResult on
partial failure so callers can inspect WriteErrors via
errors.As(err, &mongo.BulkWriteException{}).
- BulkUpsert (typed convenience): builds N UpsertModels from
items + filter mapper. \$set MERGE semantics (preserves stored
fields not in T). bsonSetWithoutID strips _id from the marshal
payload because MongoDB rejects updates to the immutable _id;
_id is set on insert from the upsert filter.
- BulkUpsertByID (ergonomic): pure pass-through with built-in
bson.M{"_id": idFn(item)} filter. Cheapest possible bulk-upsert
pattern (always-indexed _id, single B-tree lookup).
- InsertMany (sibling to bulk-upsert): write-only batch with
SetOrdered(false), returns (int64, error) so partial-failure
callers see how many items got through.
BulkResult mirrors mongo.BulkWriteResult fields the wrapper
exposes; UpsertModel + DeleteModel are stateless write-model
constructors; fromDriverResult is the driver-to-wrapper mapper.
Empty-input contract is (nil, nil) for the bulk methods and
(0, nil) for InsertMany; both are documented at the type level.
Designed via docs/superpowers/specs/2026-05-06-mongoutil-extension-
and-miniout-design.md (spec) and executed via the matching plan.
Spec was reviewed across 2 rounds by bug, spec-consistency, senior
architecture, and Mongo-expert agents before any code landed; each
plan task was implemented via TDD and reviewed by spec-compliance
plus code-quality agents (with Mongo expert on tasks involving
substantive driver interaction). The "Post-merge amendments"
section in the spec captures changes that landed after the
resolution log was finalized (BulkUpsert _id strip, bsonSetWithoutID
location + error wrapping, BulkUpsertByID always-indexed note).
New pkg/minioutil follows the pkg/<provider>util convention used by cassutil/mongoutil/natsutil/valkeyutil. Surface: - Connect(ctx, endpoint, useSSL, accessKey, secretKey)(*minio.Client, error) Construct-only. Does NOT issue a connectivity probe -- a probe via ListBuckets requires s3:ListAllMyBuckets (account-wide IAM); real production deployments scope credentials to one bucket via s3:ListBucket on that bucket's ARN, so probing here would force broader IAM than the package needs. NewBucket carries the actual fail-fast probe (bucket-scoped BucketExists). The ctx parameter is retained for signature symmetry with valkeyutil.Connect. - Bucket[T any] + NewBucket[T](ctx, client, bucketName) (*Bucket[T], error) Typed wrapper binding a *minio.Client to a single bucket and a JSON-marshalable payload type T. NewBucket calls client.BucketExists at construction so a misconfigured MINIO_BUCKET env var fails the service at startup rather than failing every Get/Put silently. Does NOT create the bucket -- provisioning is owned by ops/IaC. - Bucket[T].Put(ctx, key, v) Marshals v as JSON, uploads with explicit Content-Type: application/json; charset=utf-8 so downstream tools (S3 CLI, browsers, other languages) can identify the payload format without out-of-band knowledge. - Bucket[T].Get(ctx, key) (*T, error) Returns (nil, nil) on missing key, matching Collection.FindOne's not-found semantics. The NoSuchKey response surfaces from obj.Stat() (minio-go's GetObject is lazy); errors.As against minio.ErrorResponse with Code=="NoSuchKey" is the discriminator. Two HTTP round trips per Get (HEAD via Stat, then GET via Decode); acceptable for the small-JSON-blob workload. - Bucket[T].List(ctx, prefix, maxKeys) ([]string, error) Lists object keys within the bucket (NOT bucket names). maxKeys=0 falls back to defaultListCap (1000 -- matches S3's per-page cap) to prevent accidental unbounded scans on misuse. context.WithCancel + defer cancel() is load-bearing: minio-go's ListObjects spawns a goroutine that fills the returned channel, and breaking out of the range loop without cancelling leaks that goroutine. MaxKeys is plumbed through to ListObjectsOptions so the server returns only min(maxKeys, 1000) per page rather than the full default. - Bucket[T].Delete(ctx, key) error Idempotent on non-versioned buckets -- relies on S3 native semantics (DELETE returns 204 regardless of prior existence). - Bucket[T].Raw() *minio.Client and Bucket[T].Name() string Escape hatches mirroring Collection[T].Raw() so callers needing features the wrapper does not surface (presigned URLs, multipart, conditional Put, tagging, versioning) can reach the underlying client without giving up the bucket binding. Test infrastructure: - pkg/testutil.MinIO(t, prefix) mirrors testutil.MongoDB: sync.Once- shared MinIO container across the test process plus a per-test bucket name derived from a stable fnv hash of t.Name(). Bucket names are S3-valid (lowercase letters, digits, hyphens; capped at 63 chars). Best-effort cleanup with a 30s timeout. - pkg/testutil/testimages adds the MinIO image pin so all minio-touching integration tests track the same version. - pkg/minioutil/minio_test.go has only the trivial Bucket[T].Raw / Name accessor unit test. Hand-rolled S3 stub-server unit tests were considered and rejected during round-2 review -- emulating S3's wire format is fragile against minio-go upgrades and provides false confidence; testcontainers MinIO is fast enough (~1m for the full suite) to be the sole behavioral backstop. - The List integration test uses goleak.IgnoreCurrent() snapshotted before the List call to verify the channel-cleanup pattern without false-positives from minio-go's HTTP keepalive goroutines (IdleConnTimeout=60s, longer than goleak's drain budget; goleak's defaults do not cover net/http.persistConn). New deps (all Apache-2.0): - github.com/minio/minio-go/v7 -- MinIO Go SDK - github.com/testcontainers/testcontainers-go/modules/minio -- test only - go.uber.org/goleak -- test only, for the List leak guard Designed via docs/superpowers/specs/2026-05-06-mongoutil-extension- and-miniout-design.md (the same design doc that covers the mongoutil extension; see prior commit). 7 implementation tasks (Tasks 9-15), each TDD + reviewed by spec-compliance, code-quality, and S3-expert agents on the load-bearing tasks (Connect, NewBucket, Get NoSuchKey, List goroutine cleanup).
… test fidelity; document best-effort cleanup CodeRabbit review on PR #157 surfaced three items on the squashed branch: (MAJOR) pkg/natsrouter/router.go — Shutdown's wg.Wait() phase could race with Add(1) calls from still-draining subscriptions when the closeLoop broke early on ctx.Done(). Per sync.WaitGroup docs, Add concurrent with Wait is undefined when the counter is zero (panic risk). Fix: track allClosed; only enter the wg.Wait block when every subscription confirmed close. If ctx expired before all subs closed, surface the error and let the caller's deadline take precedence -- remaining handler goroutines continue in the background until process exit. Comment block rewritten to explain the invariant. (NIT) pkg/natsrouter/integration_test.go — TestIntegration_SpawnSite- PanicBackstop used WithMaxConcurrency(2). With cap=2, a leaked semaphore slot would still leave capacity for the follow-up "ok" request, masking a cleanup regression. Switched to cap=1 so a leaked slot blocks the second request and the test actually observes slot release. Added a comment explaining why cap=1 is load-bearing. (NIT) pkg/testutil/minio.go — replaced two `_ = container.Terminate(ctx)` calls with explicit "best-effort cleanup" comments per CLAUDE.md "never ignore errors silently — comment if intentionally discarded". The discarded errors are intentional (init already failed; Docker reaps the container on test-process exit either way), now documented.
…Type Replaces the 3-field Employee placeholder (AccountName, Name, EngName) with the full schema looked up from the employee MongoDB collection. New fields cover: - Identity: ID (_id), EmployeeID, EmployeeCategory, EmployeeRoom, Status, RosterCode, SiteID, Company. - Organisational hierarchy: 8 levels x 4 attrs each (description / id / name / tcName) for Department, Department1, Division, Division1, Section, Section1, Function, Function1; plus DivTCName, ManagedOrgIDs, Orgs []Org, and OrgCode (computed; bson:"-"). - Contact: Mail, Phone, Phones, JOSLocationURL, Location, LocationURL. - Shift: ShiftCode, ShiftCodeDescEn/Zh, ShiftStartTime, ShiftEndTime. - Supervisor: SupervisorAccountName, SupervisorID, SupervisorName, SupervisorEngName, SupervisorPhone, SupervisorPhones. Renames the Go field EngName -> EnglishName (bson tag stays "engName" so wire format is unchanged). Zero callers of model.Employee in the codebase today, so the rename has no breakage. Adds two supporting types: - OrgType (string enum: section1, section, department1, department, division1, division, function1, function). - Org (id / description / name / tcName / type). Every field has both bson and json tags per CLAUDE.md "All model structs get both json and bson tags". Empty/optional fields use omitempty consistently.
Per project rule: max ~2 lines of comment, only when WHY is non-obvious.
Cuts roughly 250 lines of doc that restated MinIO/Mongo SDK behavior,
behavior already implied by function names + signatures, or
multi-paragraph rationales better captured in commit history.
What's kept:
- One-line WHY notes for non-obvious behavior (e.g. (nil, nil)
not-found contract, FindMany returns []T{} not nil for JSON, $set
is MERGE not REPLACE, _id stripping is required because Mongo
rejects updates to immutable _id, defer cancel() is load-bearing
to avoid a goroutine leak).
- One-line caveats with real footgun risk (omitempty caveat on
BulkUpsert, 16 MB BSON limit on AggregatePaged $facet).
- A brief explanation of why minioutil.Connect doesn't probe at
startup (least-privilege IAM rationale).
What's gone:
- Multi-paragraph "Implementation note" sections that restated
what the code clearly shows.
- Field-by-field BulkResult godoc; replaced with a one-line summary
+ the empty-input contract on the type itself.
- Long pitfall lists where 1 keyword would do.
- "Standard wiring" code-example blocks in godoc.
- Restatements of S3 / Mongo native behavior callers can find in
upstream docs.
Net: ~250 fewer comment lines across 5 files, no logic change.
CodeRabbit (3 findings):
- pkg/mongoutil/bulk.go: UpsertModel/DeleteModel return concrete
*mongo.UpdateOneModel/*mongo.DeleteOneModel instead of the
interface (Go idiom + CLAUDE.md "accept interfaces, return
structs"). bulk_test.go drops the now-redundant type assertions.
- pkg/natsrouter/integration_test.go panic-backstop test: typed
payload struct instead of map[string]any (CLAUDE.md "never
map[string]interface{} for NATS payloads").
- pkg/natsrouter/integration_test.go shutdown-wait test: client
goroutines now collected via sync.WaitGroup; nc.Request errors
drain through an error channel and are asserted, no more silent
drops or tail goroutines past the assertion boundary.
Comment trim (every modified file outside pkg/natsrouter):
Max 2 lines per comment, mostly 1-liners. Cuts the BulkUpsert
godoc from 5 lines to 2; InsertMany 3->1; minioutil.Connect 3->1;
testutil.MinIO 11->2; search-service shutdown 5->1; plus several
2->1 collapses. Zero logic change.
New pkg/restyutil produces a *resty.Client with codebase defaults
wired in: OTel transport (so trace context propagates on every
outbound call), slog response logging at debug level, 30s timeout.
Options compose on top: WithTimeout / WithHeader / WithBearerToken /
WithRetries.
Replaces the raw net/http boilerplate seen in pkg/oidc and
pkg/searchengine — services adopting this can write
client.R().SetContext(ctx).SetBody(req).SetResult(&resp).Post("/path")
instead of building requests, checking statuses, and hand-rolling
JSON decode at every call site.
Adds github.com/go-resty/resty/v2 (Apache-2.0) as the canonical
HTTP client per CLAUDE.md (the rule was previously aspirational --
nothing in the repo used Resty before this).
Tests cover defaults, every Option, end-to-end round-trip via
httptest, and retry-on-5xx behaviour.
…ror + WithTransport, log hygiene) Three reviewers (bug / code-quality / senior-engineer) consolidated the same actionable items: - Drop WithRetries entirely. Resty's default retry condition only fires on transport errors, not 5xx, so the option was misleading without an accompanying retry-condition helper. YAGNI-applied: no current call site needs retries; bring it back when a real caller has a concrete retry policy in mind. - Rename WithRetries' max parameter -> moot (option dropped). The shadow of the Go 1.21 builtin `max` is no longer present. - Add OnError hook so transport errors (connect refused, DNS, ctx deadline, TLS) log too. Without it the helper REGRESSED observability vs. the raw net/http boilerplate it replaces. - Add WithTransport(http.RoundTripper) Option. Wraps the supplied transport with otelhttp so OTel propagation is preserved by construction, even with custom TLS / proxy / dialer. Required before pkg/oidc (which needs InsecureSkipVerify in dev) can migrate. - Strip query string from URL in slog log fields. URLs can carry ?token=... or ?api_key=... in the wild; CLAUDE.md "Never log tokens, passwords, or full message bodies". - Propagate request_id from ctx into the slog line via natsutil.RequestIDFromContext. CLAUDE.md "include in all log lines". Tests cover OnError firing on transport error, WithTransport preserving the custom round-tripper, request_id propagation, and query-string stripping.
GITMateuszCharczuk
force-pushed
the
claude/review-history-service-KWjho
branch
from
10b9828 to
1f905ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/restyutil/restyutil_test.go (1)
99-99: ⚡ Quick win
slog.NewTextHandlerin tests violates the project's JSON-format logger guideline.Both logging tests mutate the global default logger using
slog.NewTextHandler, and the subsequentContainsassertions are written against text-format output (key=value). The coding guideline explicitly forbids text-format loggers in all**/*.gofiles — test files included.Switch to
slog.NewJSONHandlerand update the assertion patterns to match JSON-encoded fields:♻️ Proposed fix for `TestLog_PropagatesRequestID` and `TestLog_FiresOnTransportError`
-slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) +slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))In
TestLog_PropagatesRequestID, update assertions accordingly:-assert.Contains(t, out, "request_id=req-abc-123") +assert.Contains(t, out, `"request_id":"req-abc-123"`) -assert.NotContains(t, out, "secret", "query string must be stripped from logs") +assert.NotContains(t, out, "secret", "query string must be stripped from logs")In
TestLog_FiresOnTransportError:-assert.Contains(t, buf.String(), "http error") +assert.Contains(t, buf.String(), `"msg":"http error"`)As per coding guidelines: "Always use
log/slogwith JSON format — never usefmt.Println,log.Println, or text-format loggers."Also applies to: 121-121
🤖 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 `@pkg/restyutil/restyutil_test.go` at line 99, Tests currently replace the global logger with slog.NewTextHandler (via slog.SetDefault(slog.NewTextHandler(&buf, ...))) which violates the JSON-only logger guideline; change those setups in TestLog_PropagatesRequestID and TestLog_FiresOnTransportError to use slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}) and update the assertions that inspect buf to match JSON-encoded fields (e.g., look for JSON keys/values like "request_id" or the JSON "level"/"msg" fields instead of key=value text patterns) so the tests validate JSON output while still using the same buffer and global logger replacement points.pkg/natsrouter/router.go (1)
18-29: ⚡ Quick winKeep
Registrarout of the producer package’s public API.This new exported interface is defined on the implementer side and seems to exist only to support
Register*composition insidepkg/natsrouter. That permanently expands the package surface and makes future router changes harder to evolve. Prefer an unexported internal interface or keep accepting*Routeruntil a real external consumer needs an abstraction.As per coding guidelines: “Define interfaces in the consumer, not the implementer”.
🤖 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 `@pkg/natsrouter/router.go` around lines 18 - 29, Registrar is exported but only used internally; change it to an unexported interface or remove it and accept *Router directly to avoid expanding the public API. Specifically, rename the interface symbol Registrar to registrar (or delete it) and update any references (addRoute, HandlerFunc usages, and places that type-assert or declare Registrar) inside pkg/natsrouter to use the unexported registrar or *Router instead so the interface remains internal and the package surface doesn't change.
🤖 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 `@docs/superpowers/plans/2026-04-30-natsrouter-improvements.md`:
- Around line 5-8: Update the plan doc to reflect the implemented behavior:
change the described default semaphore cap from "100" to "unbounded (no default
cap)" and replace mentions of a per-route "WithConcurrency" bulkhead with the
actual implemented router-level "WithMaxConcurrency" option; keep architecture
notes about the single dispatcher goroutine and the semaphore-guarded
spawn-per-message model, retain the `HandlerTimeout(d)` middleware description,
and explicitly mark any superseded design ideas (per-route `WithConcurrency` and
default-100 cap) as deprecated so the doc matches the shipped `pkg/natsrouter`
behavior.
In `@docs/superpowers/plans/2026-05-06-mongoutil-extension-and-minioutil.md`:
- Around line 1791-1806: The Connect function currently performs a ListBuckets
probe that was removed from the spec; remove the probe call and its timeout
context (the probeCtx, cancel, and client.ListBuckets usage) from Connect
(function Connect) so it no longer requires account-wide s3:ListAllMyBuckets,
drop the now-unused "time" import, and update the Connect godoc to state that
the ctx parameter is retained for signature symmetry (and is unused) to match
NewBucket's BucketExists behavior and the post-merge amendments.
- Around line 2569-2587: In TestIntegration_List_MaxKeysCap replace the stale
goleak pattern by updating the teardown call: locate the defer
goleak.VerifyNone(t) at the top of the TestIntegration_List_MaxKeysCap function
and change it to call VerifyNone with the IgnoreCurrent baseline option (use
goleak.IgnoreCurrent()) so the test uses the current-goroutine baseline when
verifying leaks.
In `@pkg/natsrouter/router.go`:
- Around line 176-212: The dispatch path (the goroutine started after
r.admit/release and before r.wg.Add/r.wg.Done) can still start work after
Shutdown returns because Shutdown's timeout path bypasses the wait and does not
prevent late callbacks from calling r.admit/r.wg.Add; add a "router stopping"
gate that Shutdown sets (e.g., an atomic or mutex-protected boolean like
r.stopping) and check it in the dispatch path immediately after admittance and
before r.wg.Add (and also before spawning the goroutine and before calling
acquireContext), rejecting requests via r.replyBusy when stopping is true;
ensure Shutdown sets this flag before changing allClosed and after signaling
drains so no new handlers can increment r.wg, and apply the same guard to the
alternate dispatch block around lines 258-293.
---
Nitpick comments:
In `@pkg/natsrouter/router.go`:
- Around line 18-29: Registrar is exported but only used internally; change it
to an unexported interface or remove it and accept *Router directly to avoid
expanding the public API. Specifically, rename the interface symbol Registrar to
registrar (or delete it) and update any references (addRoute, HandlerFunc
usages, and places that type-assert or declare Registrar) inside pkg/natsrouter
to use the unexported registrar or *Router instead so the interface remains
internal and the package surface doesn't change.
In `@pkg/restyutil/restyutil_test.go`:
- Line 99: Tests currently replace the global logger with slog.NewTextHandler
(via slog.SetDefault(slog.NewTextHandler(&buf, ...))) which violates the
JSON-only logger guideline; change those setups in TestLog_PropagatesRequestID
and TestLog_FiresOnTransportError to use slog.NewJSONHandler(&buf,
&slog.HandlerOptions{Level: slog.LevelDebug}) and update the assertions that
inspect buf to match JSON-encoded fields (e.g., look for JSON keys/values like
"request_id" or the JSON "level"/"msg" fields instead of key=value text
patterns) so the tests validate JSON output while still using the same buffer
and global logger replacement points.
🪄 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: 1018e473-71f5-4546-bfaa-2ff8ba101ef6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
docs/superpowers/plans/2026-04-30-natsrouter-improvements.mddocs/superpowers/plans/2026-05-06-mongoutil-extension-and-minioutil.mddocs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.mdgo.modhistory-service/internal/mongorepo/setup_test.gohistory-service/internal/mongorepo/subscription.gohistory-service/internal/mongorepo/subscription_test.gohistory-service/internal/mongorepo/threadroom.gohistory-service/internal/mongorepo/threadroom_test.gohistory-service/internal/service/mocks/mock_repository.gohistory-service/internal/service/service.gohistory-service/internal/service/threads.gohistory-service/internal/service/threads_test.gopkg/minioutil/minio.gopkg/minioutil/minio_integration_test.gopkg/minioutil/minio_test.gopkg/model/employee.gopkg/mongoutil/bulk.gopkg/mongoutil/bulk_integration_test.gopkg/mongoutil/bulk_test.gopkg/mongoutil/collection.gopkg/mongoutil/collection_integration_test.gopkg/mongoutil/empty_input_test.gopkg/mongoutil/options.gopkg/mongoutil/options_test.gopkg/mongoutil/pagination.gopkg/mongoutil/pagination_test.gopkg/natsrouter/README.mdpkg/natsrouter/context.gopkg/natsrouter/context_test.gopkg/natsrouter/doc.gopkg/natsrouter/errors.gopkg/natsrouter/errors_test.gopkg/natsrouter/example_test.gopkg/natsrouter/integration_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/natsrouter/router.gopkg/natsrouter/router_test.gopkg/restyutil/restyutil.gopkg/restyutil/restyutil_test.gopkg/testutil/minio.gopkg/testutil/testimages/testimages.gosearch-service/main.go
💤 Files with no reviewable changes (1)
- history-service/internal/mongorepo/subscription_test.go
✅ Files skipped from review due to trivial changes (12)
- pkg/mongoutil/pagination_test.go
- pkg/natsrouter/errors_test.go
- pkg/mongoutil/pagination.go
- pkg/mongoutil/options.go
- pkg/natsrouter/middleware_test.go
- history-service/internal/service/service.go
- pkg/mongoutil/bulk_test.go
- pkg/natsrouter/errors.go
- pkg/restyutil/restyutil.go
- pkg/mongoutil/empty_input_test.go
- pkg/minioutil/minio.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/minioutil/minio_test.go
- pkg/testutil/testimages/testimages.go
- pkg/natsrouter/context.go
- pkg/natsrouter/context_test.go
- history-service/internal/mongorepo/threadroom_test.go
- history-service/internal/mongorepo/setup_test.go
- search-service/main.go
- pkg/mongoutil/options_test.go
- pkg/natsrouter/example_test.go
- pkg/natsrouter/doc.go
- pkg/mongoutil/bulk.go
- pkg/mongoutil/collection_integration_test.go
- history-service/internal/service/threads_test.go
- pkg/minioutil/minio_integration_test.go
- pkg/model/employee.go
- history-service/internal/service/mocks/mock_repository.go
- pkg/mongoutil/collection.go
- pkg/natsrouter/README.md
…lan docs CodeRabbit pass on the rebased branch surfaced 1 real bug + 5 cleanups. (MAJOR) router.go: Shutdown previously had a window where late NATS callbacks (subscription mid-drain or fired after ctx expiry) could still call admit() + wg.Add(1) + spawn a handler goroutine AFTER Shutdown returned. That goroutine could then race teardown of caller- owned dependencies (DB closed, NATS drained). Add a stopping atomic.Bool gate that Shutdown sets BEFORE any drain step; natsHandler checks it first and replies busy instead of admitting. Existing in-flight handlers still drain through the wg.Wait path; only NEW dispatches post-Shutdown-start are rejected. (NIT) router.go: removed the exported Registrar interface — used only by Register/RegisterNoBody/RegisterVoid in this package, no external mocks, no alternative implementation. Their first parameter is now *Router directly. Pure simplification: existing callers (which pass *Router today) keep working. CLAUDE.md "interfaces in the consumer". (NIT) restyutil_test.go: switched the two log-capture tests from slog.NewTextHandler to NewJSONHandler; CLAUDE.md "always JSON, never text-format". Updated the matched assertions. (DOC) 2026-04-30-natsrouter-improvements.md: marked the original "default 100" + per-route WithConcurrency wording as superseded with a post-implementation note. Shipped behavior: unbounded default, single router-level WithMaxConcurrency. (DOC) 2026-05-06-mongoutil-extension-and-minioutil.md Task 9: dropped the ListBuckets startup probe from the Connect code block; added a post-implementation note pointing at the spec's amendments section. Future agents executing this plan get the IAM-compliant code. (DOC) Same plan, Task 14: replaced the stale defer goleak.VerifyNone(t) pattern with goleak.IgnoreCurrent() baseline; updated the comment to explain why minio-go's HTTP keepalive goroutines (IdleConnTimeout=60s) require it.
mliu33
approved these changes
May 7, 2026
| // If multiple WithMaxConcurrency options are supplied, the last call | ||
| // with a positive n takes effect. Saturation triggers a 503-style | ||
| // ErrUnavailable reply. | ||
| func WithMaxConcurrency(n int) Option { |
| } | ||
|
|
||
| func logError(req *resty.Request, err error) { | ||
| slog.Debug("http error", logFields(req, 0, time.Since(req.Time), err)...) |
Collaborator
There was a problem hiding this comment.
maybe it's better to use slog.Error for log error ?
| return nil | ||
| } | ||
|
|
||
| func logError(req *resty.Request, err error) { |
Collaborator
There was a problem hiding this comment.
maybe it's better to include resp.StatusCode when logging error response ?
| return | ||
| } | ||
| r.wg.Add(1) | ||
| go func() { |
5 tasks
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 8, 2026
…l/Employee Two changes: 1. logError uses slog.Error instead of slog.Debug, matching the codebase convention (mongoutil.Disconnect, natsutil.Reply/Ack, natsrouter handler-error all use Error). @mliu33 PR #157 review. 2. Delete pkg/model/employee.go entirely. The struct had zero callers in the repo. No replacement; bring back when a real user shows up. logResponse stays at slog.Debug -- status code was already in the log fields (logFields includes resp.StatusCode), no level change needed beyond what @mliu33 actually requested.
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
May 8, 2026
…l/Employee Two changes: 1. logError uses slog.Error instead of slog.Debug, matching the codebase convention (mongoutil.Disconnect, natsutil.Reply/Ack, natsrouter handler-error all use Error). @mliu33 PR #157 review. 2. Delete pkg/model/employee.go entirely. The struct had zero callers in the repo. No replacement; bring back when a real user shows up. logResponse stays at slog.Debug -- status code was already in the log fields (logFields includes resp.StatusCode), no level change needed beyond what @mliu33 actually requested.
Joey0538
pushed a commit
that referenced
this pull request
May 8, 2026
…l/Employee (#163) Two changes: 1. logError uses slog.Error instead of slog.Debug, matching the codebase convention (mongoutil.Disconnect, natsutil.Reply/Ack, natsrouter handler-error all use Error). @mliu33 PR #157 review. 2. Delete pkg/model/employee.go entirely. The struct had zero callers in the repo. No replacement; bring back when a real user shows up. logResponse stays at slog.Debug -- status code was already in the log fields (logFields includes resp.StatusCode), no level change needed beyond what @mliu33 actually requested. Co-authored-by: Claude <noreply@anthropic.com>
4 tasks
Joey0538
added a commit
that referenced
this pull request
May 8, 2026
ECK and most managed ES clusters require credentials. Add SEARCH_USERNAME and SEARCH_PASSWORD env vars on search-service and search-sync-worker (both default to empty so dev clusters without auth still work). Also refactor searchengine.New to take a Config struct rather than growing positional args (now five connection knobs: Backend, URL, Username, Password, TLSSkipVerify) — labeled fields at every call site and easy to extend. Test call sites (9) updated to the struct form. Drive-by: fix history-service/internal/mongorepo/room.go and service.go to import the Collection / NewCollection / WithProjection helpers from pkg/mongoutil (they were moved out of the local package in #157 but room.go and the compile-time check were left referencing the old package-local names, breaking make lint on main). https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Three independent additions bundled in one branch (per request):
pkg/natsrouter— admission control, panic safety, graceful shutdown,HandlerTimeout, doc.go, README rewrite.pkg/mongoutilextension — promotes generic Mongo helpers (Collection[T],OffsetPage,QueryOption) fromhistory-service/internal/mongorepo/into sharedpkg/mongoutil. Adds a three-layer bulk-write API:BulkWrite(foundation,SetOrdered(false), partial-failure result preservation)BulkUpsert(typed convenience,$setMERGE semantics,_id-stripping helper)BulkUpsertByID(ergonomic,bson.M{"_id": idFn(item)}filter)InsertManyreturns(int64, error)for partial-failure visibility.pkg/minioutil(new) — typedBucket[T]JSON-blob wrapper aroundminio-go/v7. CRUD:Put/Get(NoSuchKey →(nil, nil)) /List(channel-cancellation pattern,goleakverified) /Delete(idempotent).NewBucketfail-fasts on missing bucket;Raw()+Name()escape hatches.Highlights
BulkUpsert$setwould have rejected every existing-doc update because of immutable_id(fixed viabsonSetWithoutID).goleak.VerifyNone(t)with default ignores would have false-positively flaggednet/http.persistConnkeepalive goroutines (switched toIgnoreCurrent()baseline).Getgodoc claim about HTTP round-trip count was inaccurate (Statissues a separate HEAD; corrected).pkg/testutil.MinIO(t, prefix)mirrors the existingtestutil.MongoDBpattern (sync.Once shared container + per-test fnv-hashed bucket).Spec & plan
docs/superpowers/specs/2026-05-06-mongoutil-extension-and-miniout-design.mddocs/superpowers/plans/2026-05-06-mongoutil-extension-and-minioutil.md15 tasks, each implemented + reviewed serially.
Test plan
make lint— 0 issuesmake test— all unit tests passmake test-integration— Mongo + MinIO testcontainers tests pass (NOTE: this was not runnable in the implementation sandbox due to a Docker socket access quirk; CI must verify)history-serviceintegration tests still pass after themongorepo→mongoutilmigrationapplication/json; charset=utf-8content-typegoleak.IgnoreCurrent()confirms no leak on early breakhttps://claude.ai/code/session_01Ngkd45NdAxeqX4FU4t6w9X
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests