Centralized error codes: introduce pkg/errcode with transport adapters#250
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (134)
📝 WalkthroughWalkthroughAdds pkg/errcode core (types, constructors, classification, permanence), errnats/errhttp/errtest adapters, request-id stamping/require helpers, Semgrep rules, and migrates services, workers, frontend, models, tests, and docs to the unified JSON error envelope. ChangesCentralized errcode migration
Estimated code review effort Possibly related PRs
Suggested labels 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: 5
🧹 Nitpick comments (10)
room-service/handler.go (2)
1412-1412: ⚡ Quick winConsider omitting the subject from the client-facing error message.
The subject string is interpolated into the
BadRequestmessage. While not a secret, subject strings contain account/room identifiers. Per the no-leak contract, prefererrcode.BadRequest("invalid mute-toggle subject")without interpolation. If the subject is needed for debugging, useWithMetadata("subject", subj)so it's logged server-side but not sent to the client.🤖 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 `@room-service/handler.go` at line 1412, The BadRequest currently includes the interpolated subject (subj) in the client error; change it to return errcode.BadRequest("invalid mute-toggle subject") without including subj, and attach the subject as server-only metadata using errcode.WithMetadata("subject", subj) (i.e., use errcode.BadRequest(...).WithMetadata(...)) so the subject is logged for debugging but not leaked to clients; update the return at the invalid mute-toggle subject check that currently calls errcode.BadRequest(fmt.Sprintf(...)).
815-815: ⚡ Quick winConsolidate timeout error construction with structured metadata.
Three identical
errcode.Unavailableconstructions interpolateref.RoomIDandref.SiteIDinto the error message. Consider extracting a helper function and usingerrcode.WithMetadata("roomId", ref.RoomID, "siteId", ref.SiteID)to provide structured fields for client-side rendering instead of baking them into the message string.♻️ Example refactor
// In helper.go or at package scope: func channelExpandTimeout(roomID, siteID string) error { return errcode.Unavailable( "timeout listing members of channel", errcode.WithMetadata("roomId", roomID, "siteId", siteID), ) } // At call sites: return nil, nil, channelExpandTimeout(ref.RoomID, ref.SiteID)Also applies to: 826-826, 835-835
🤖 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 `@room-service/handler.go` at line 815, Extract a small helper (e.g., channelExpandTimeout) that returns errcode.Unavailable with a fixed message ("timeout listing members of channel") and adds structured metadata using errcode.WithMetadata("roomId", roomID, "siteId", siteID); then replace the three inline constructions that call errcode.Unavailable(fmt.Sprintf("timeout listing members of channel %s@%s", ref.RoomID, ref.SiteID)) with returns that call channelExpandTimeout(ref.RoomID, ref.SiteID) so the error message is consistent and metadata is attached for client-side rendering.message-gatekeeper/handler_test.go (1)
337-350: ⚡ Quick winTest case label may be stale.
With the
processMessagesignature change (now takes*model.SendMessageRequestinstead of[]byte), this test case labeled "malformed JSON" no longer tests JSON parsing. Line 716 unmarshals the payload and ignores errors, soreqbecomes a zero-value struct, and the test validates zero-value request behavior (missing requestId, empty content, etc.) instead of malformed JSON.Malformed JSON IS tested at the JetStream layer (line 1286:
TestHandleJetStreamMsg_MalformedBody_Acks), so coverage is preserved.Consider either:
- Removing this table test case as redundant, or
- Renaming it to reflect what it actually tests (e.g., "zero-value request").
🤖 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 `@message-gatekeeper/handler_test.go` around lines 337 - 350, The table test entry named "malformed JSON" is stale because processMessage now takes *model.SendMessageRequest (the test's buildData returning "{not valid json}" is unmarshaled into a zero-value req), so either remove this table row or rename it to reflect that it verifies zero-value request handling (e.g., "zero-value request"); if keeping it, update the buildData to return a zero-value/matching request payload (or json.Marshal(&model.SendMessageRequest{})), adjust the test expectations (wantErr/wantInfra) to match zero-value behavior, and update the test label accordingly; reference the processMessage function, model.SendMessageRequest type, the buildData closure, and the table entry named "malformed JSON" when making the change.chat-frontend/src/api/_transport/asyncJob.ts (1)
32-48: 💤 Low valueClarify "7+1" error categories phrasing.
Line 33 says "7+1 generic error categories" but the union type lists 8 categories without a structural distinction. If you meant to group them semantically (e.g., "7 client-facing + 1 internal"), consider making that explicit:
/** * The 8 error categories from `pkg/errcode/category.go`: 7 client-actionable * (bad_request, unauthenticated, forbidden, not_found, conflict, too_many_requests, * unavailable) plus internal for unclassified failures. Mirrors the closed backend set. */Otherwise, "8 error categories" is clearer than "7+1".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@chat-frontend/src/api/_transport/asyncJob.ts` around lines 32 - 48, Update the JSDoc above the ErrorCode type to remove the ambiguous "7+1" phrasing and clearly state the count and grouping; e.g., document that there are eight categories and optionally clarify they are "7 client-actionable categories (bad_request, unauthenticated, forbidden, not_found, conflict, too_many_requests, unavailable) plus one internal category (internal)". Modify the comment that precedes the ErrorCode union so readers immediately understand the grouping and that it mirrors pkg/errcode/category.go.chat-frontend/src/api/_transport/asyncJob.test.js (1)
274-293: 💤 Low valueClarify comment to reflect conditional passthrough behavior.
The comment at lines 275-277 states these kinds "intentionally pass the raw server-supplied message through" but that's only half the story post-migration. With the new errcode system:
- When
reasonis present and inREASON_COPY→ returns humanized copy- When
reasonis absent or unmapped → falls back toerr.messageThe test correctly validates the no-reason case (line 283 creates a plain
Errorwith no reason field), but the comment's phrasing implies unconditional passthrough. Consider updating to:// SyncError/AsyncError fall back to the server-supplied message when no // reason is attached or when the reason isn't in REASON_COPY. All other // kinds (wire-level failures) must produce a branded client-side hint.This clarifies that reason-mapped errors get humanized copy, while unmapped/absent reasons fall back to the raw message.
🤖 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 `@chat-frontend/src/api/_transport/asyncJob.test.js` around lines 274 - 293, Update the test comment near ASYNC_JOB_ERROR_KINDS/RAW_MESSAGE_KINDS to clarify conditional passthrough: explain that SyncError/AsyncError only fall back to the server-supplied message when the error has no reason or the reason isn’t in REASON_COPY, whereas when a reason is present and mapped in REASON_COPY formatAsyncJobError returns humanized copy; all other kinds should produce a branded client-side hint. Reference ASYNC_JOB_ERROR_KINDS, RAW_MESSAGE_KINDS, REASON_COPY, and formatAsyncJobError in the comment to make the behavior explicit.chat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsx (1)
10-10: ⚡ Quick winImport these helpers through
@/api.This test is outside
src/api, so it should consume the barrel export instead of deep-importing the private transport module.♻️ Proposed fix
-import { AsyncJobError, ASYNC_JOB_ERROR_KINDS } from '`@/api/_transport/asyncJob`' +import { AsyncJobError, ASYNC_JOB_ERROR_KINDS } from '`@/api`'Based on learnings:
api/_transport/is internal — only files insideapi/may import from it. Components MUST go throughsrc/api/index.tsbarrel exports.🤖 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 `@chat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsx` at line 10, The test currently deep-imports internal symbols AsyncJobError and ASYNC_JOB_ERROR_KINDS from api/_transport; change the import to consume these helpers from the public barrel by importing them from "`@/api`" instead (update the import statement in LeaveRoomButton.test.jsx to reference "`@/api`" and use the same symbols AsyncJobError and ASYNC_JOB_ERROR_KINDS so the test relies only on the public API).chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx (1)
8-12: ⚡ Quick winAdd a test for the token-invalid redirect path.
This file now mocks
isSSOTokenInvalidErrorandredirectToReloginOnTokenInvalid, but it still doesn't assert the new branch where an invalid SSO token triggers relogin instead of rendering an error.Based on learnings: tests must cover happy path, error paths, edge cases, and invalid input.
🤖 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 `@chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx` around lines 8 - 12, Add a new test case in OidcCallback.test.jsx that simulates the "invalid SSO token" branch: mock isSSOTokenInvalidError to return true and make getOidcManager or the code path throw the same error instance that your component checks; then assert that redirectToReloginOnTokenInvalid was called (and awaited if it returns a promise) and that the error UI/message is not rendered. Reference the existing mocks getOidcManager, isSSOTokenInvalidError, and redirectToReloginOnTokenInvalid and use the same render/act utilities used by other tests to trigger the component lifecycle and perform the assertions.search-service/handler_test.go (1)
175-177: ⚡ Quick winConsider using
errtest.AssertCodefor consistency.Multiple tests manually assert error codes via
errors.As(&rerr) + assert.Equal(errcode.Code*, rerr.Code). Thepkg/errcode/errtestpackage providesAssertCode(t, err, expectedCode)andAssertReason(t, err, expectedReason)helpers for this exact pattern. Using them would improve consistency across the codebase and reduce boilerplate.Example refactor
- var rerr *errcode.Error - require.True(t, errors.As(err, &rerr)) - assert.Equal(t, errcode.CodeBadRequest, rerr.Code) + errtest.AssertCode(t, err, errcode.CodeBadRequest)Apply similar changes to all validation-error assertions in this file (~13 occurrences).
Based on learnings: The branch review doc explicitly flags search-service/handler_test.go for inconsistent errtest adoption at ~25 sites.
Also applies to: 184-186, 279-281, 288-290, 297-300, 307-309, 355-357, 431-433, 444-446, 494-496, 582-584, 596-598, 609-611
🤖 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 `@search-service/handler_test.go` around lines 175 - 177, Replace the manual error-code assertion pattern that uses a typed variable (rerr *errcode.Error), errors.As(err, &rerr) and assert.Equal(t, errcode.CodeBadRequest, rerr.Code) with the errtest helper: call errtest.AssertCode(t, err, errcode.CodeBadRequest) (and errtest.AssertReason where applicable) to reduce boilerplate; update the assertion at the location referencing rerr/errors.As/errcode.CodeBadRequest and apply the same replacement to the other listed occurrences in handler_test.go (~13 sites) so every validation-error check uses errtest.AssertCode/AssertReason consistently.search-service/metrics_test.go (1)
17-35: ⚡ Quick winGive the status-label matrix named subtests.
This table already covers multiple variants of the same behavior, but a failure will not say which case broke. Adding case names with
t.Run(...)will make the signal much clearer.Suggested change
cases := []struct { + name string err error want string }{ - {errcode.BadRequest("x"), "bad_request"}, - {errcode.Unauthenticated("x"), "unauthenticated"}, - {errcode.Forbidden("x"), "forbidden"}, - {errcode.NotFound("x"), "not_found"}, - {errcode.Conflict("x"), "conflict"}, - {errcode.TooManyRequests("x"), "too_many_requests"}, - {errcode.Unavailable("x"), "unavailable"}, - {errcode.Internal("x"), "internal"}, + {"bad_request", errcode.BadRequest("x"), "bad_request"}, + {"unauthenticated", errcode.Unauthenticated("x"), "unauthenticated"}, + {"forbidden", errcode.Forbidden("x"), "forbidden"}, + {"not_found", errcode.NotFound("x"), "not_found"}, + {"conflict", errcode.Conflict("x"), "conflict"}, + {"too_many_requests", errcode.TooManyRequests("x"), "too_many_requests"}, + {"unavailable", errcode.Unavailable("x"), "unavailable"}, + {"internal", errcode.Internal("x"), "internal"}, } for _, tc := range cases { - if got := statusLabel(tc.err); got != tc.want { - t.Errorf("statusLabel(%v) = %q, want %q", tc.err, got, tc.want) - } + t.Run(tc.name, func(t *testing.T) { + if got := statusLabel(tc.err); got != tc.want { + t.Fatalf("statusLabel(%v) = %q, want %q", tc.err, got, tc.want) + } + }) }As per coding guidelines "
**/*_test.go: Prefer table-driven tests when testing multiple input/output variations of the same logic. Each test case must have a descriptive name. Uset.Run(name, func(t *testing.T) { ... })for subtests."🤖 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 `@search-service/metrics_test.go` around lines 17 - 35, The table-driven test TestStatusLabel_CanonicalErrcodePassesThrough should use named subtests so failures show which case failed: add a name field to each case (e.g., "bad_request", "unauthenticated", etc.), iterate over cases and call t.Run(tc.name, func(t *testing.T) { if got := statusLabel(tc.err); got != tc.want { t.Errorf(... ) } }), moving the assertion inside the subtest closure and referencing the existing statusLabel function and the TestStatusLabel_CanonicalErrcodePassesThrough test.history-service/internal/service/messages.go (1)
95-99: ⚡ Quick winAdd
request_idto these direct warning logs.These
slog.Warncalls are emitted directly, so they do not carry the request correlation data added earlier viac.WithLogValues(...). When one of these best-effort paths fails, tracing the warning back to the triggering request gets much harder.Suggested change
- slog.Warn("loading minUserLastSeenAt", "error", rErr, "room_id", roomID) + slog.Warn("loading minUserLastSeenAt", + "error", rErr, + "request_id", natsutil.RequestIDFromContext(c), + "room_id", roomID, + )slog.Warn("canonical marshal failed", - "error", err, "subject", subj, "messageID", evt.Message.ID, "room_id", evt.Message.RoomID) + "error", err, + "request_id", natsutil.RequestIDFromContext(c), + "subject", subj, + "messageID", evt.Message.ID, + "room_id", evt.Message.RoomID)slog.Warn("canonical publish failed", - "error", err, "subject", subj, "messageID", evt.Message.ID, "room_id", evt.Message.RoomID) + "error", err, + "request_id", natsutil.RequestIDFromContext(c), + "subject", subj, + "messageID", evt.Message.ID, + "room_id", evt.Message.RoomID)Also applies to: 453-460
🤖 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 `@history-service/internal/service/messages.go` around lines 95 - 99, The direct slog.Warn calls (e.g., after calling s.rooms.GetMinUserLastSeenAt in messages.go) are missing the request correlation field—extract the current request_id from the request context (the same context key/utility used elsewhere to add c.WithLogValues) and include it as "request_id" in the slog.Warn field list so the warning carries request correlation; apply the same change to the other analogous warn sites (the second occurrence around the 453-460 region) so every direct slog.Warn includes the request_id from gctx before returning.
🤖 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 `@chat-frontend/src/pages/OidcCallback/OidcCallback.jsx`:
- Around line 42-46: The branch that calls redirectToReloginOnTokenInvalid()
must catch rejections so an async rejection doesn't remain unhandled; update the
effect so that when isSSOTokenInvalidError(err) is true you await
redirectToReloginOnTokenInvalid() inside a try/catch and on catch call
setError(formatAsyncJobError(err) || String(err)) (and optionally log) and then
return—this ensures redirect failures are handled and the UI shows the error
instead of hanging on "Completing sign-in...".
In `@docs/superpowers/specs/2026-05-28-centralized-error-codes-design.md`:
- Around line 59-70: The spec's "7 categories" claim conflicts with the table
and code: the table includes too_many_requests (HTTP 429) but the Go constants
omitted CodeTooManyRequests while the implementation defines it; update the spec
and constants to be consistent by either removing too_many_requests from the
table and implementation or adding CodeTooManyRequests to the const list and
updating the documented count to 8; specifically ensure the symbol
CodeTooManyRequests and the category key too_many_requests → 429 are present in
the Go const snippet and in the textual count, or remove both occurrences so the
table, text, and Go consts all match.
In `@message-gatekeeper/handler.go`:
- Around line 295-299: The branch that treats snap == nil as errcode.NotFound is
wrong because a nil snapshot with err == nil indicates a fetcher contract
violation (internal failure) not a missing parent; update the case handling in
the switch that checks snap to detect when snap == nil and err == nil and return
an internal/server error (e.g., errcode.Internal or an ErrInternalf with context
including quotedParentMessageID) instead of errcode.NotFound so callers don't
follow the reply+Ack path; keep the original error semantics when err is
actually a not-found error.
In `@room-service/helper.go`:
- Around line 29-32: The constant errNotRoomMember uses a list-specific message
but is also returned from handleAddMembers; change its human-facing string to a
neutral phrase (e.g., "not a member of the room") while keeping the same
errcode.Forbidden call and errcode.WithReason(errcode.RoomNotMember) so callers
of handleAddMembers and list-members receive the same code/reason but an
appropriate, non-misleading message; update only the message passed to
errcode.Forbidden where errNotRoomMember is defined.
In `@search-service/handler.go`:
- Around line 84-85: Trim req.Query with strings.TrimSpace before validating in
the handler that checks if req.Query == ""; use the trimmed value for the
emptiness check (and assign it back to req.Query if you want downstream code to
see the trimmed term). Update the condition in the function that currently
returns errcode.BadRequest("query is required") to check the trimmed query
(e.g., trimmed := strings.TrimSpace(req.Query); if trimmed == "" { ... }) so
whitespace-only queries are rejected like the other search handlers.
---
Nitpick comments:
In `@chat-frontend/src/api/_transport/asyncJob.test.js`:
- Around line 274-293: Update the test comment near
ASYNC_JOB_ERROR_KINDS/RAW_MESSAGE_KINDS to clarify conditional passthrough:
explain that SyncError/AsyncError only fall back to the server-supplied message
when the error has no reason or the reason isn’t in REASON_COPY, whereas when a
reason is present and mapped in REASON_COPY formatAsyncJobError returns
humanized copy; all other kinds should produce a branded client-side hint.
Reference ASYNC_JOB_ERROR_KINDS, RAW_MESSAGE_KINDS, REASON_COPY, and
formatAsyncJobError in the comment to make the behavior explicit.
In `@chat-frontend/src/api/_transport/asyncJob.ts`:
- Around line 32-48: Update the JSDoc above the ErrorCode type to remove the
ambiguous "7+1" phrasing and clearly state the count and grouping; e.g.,
document that there are eight categories and optionally clarify they are "7
client-actionable categories (bad_request, unauthenticated, forbidden,
not_found, conflict, too_many_requests, unavailable) plus one internal category
(internal)". Modify the comment that precedes the ErrorCode union so readers
immediately understand the grouping and that it mirrors pkg/errcode/category.go.
In
`@chat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsx`:
- Line 10: The test currently deep-imports internal symbols AsyncJobError and
ASYNC_JOB_ERROR_KINDS from api/_transport; change the import to consume these
helpers from the public barrel by importing them from "`@/api`" instead (update
the import statement in LeaveRoomButton.test.jsx to reference "`@/api`" and use
the same symbols AsyncJobError and ASYNC_JOB_ERROR_KINDS so the test relies only
on the public API).
In `@chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx`:
- Around line 8-12: Add a new test case in OidcCallback.test.jsx that simulates
the "invalid SSO token" branch: mock isSSOTokenInvalidError to return true and
make getOidcManager or the code path throw the same error instance that your
component checks; then assert that redirectToReloginOnTokenInvalid was called
(and awaited if it returns a promise) and that the error UI/message is not
rendered. Reference the existing mocks getOidcManager, isSSOTokenInvalidError,
and redirectToReloginOnTokenInvalid and use the same render/act utilities used
by other tests to trigger the component lifecycle and perform the assertions.
In `@history-service/internal/service/messages.go`:
- Around line 95-99: The direct slog.Warn calls (e.g., after calling
s.rooms.GetMinUserLastSeenAt in messages.go) are missing the request correlation
field—extract the current request_id from the request context (the same context
key/utility used elsewhere to add c.WithLogValues) and include it as
"request_id" in the slog.Warn field list so the warning carries request
correlation; apply the same change to the other analogous warn sites (the second
occurrence around the 453-460 region) so every direct slog.Warn includes the
request_id from gctx before returning.
In `@message-gatekeeper/handler_test.go`:
- Around line 337-350: The table test entry named "malformed JSON" is stale
because processMessage now takes *model.SendMessageRequest (the test's buildData
returning "{not valid json}" is unmarshaled into a zero-value req), so either
remove this table row or rename it to reflect that it verifies zero-value
request handling (e.g., "zero-value request"); if keeping it, update the
buildData to return a zero-value/matching request payload (or
json.Marshal(&model.SendMessageRequest{})), adjust the test expectations
(wantErr/wantInfra) to match zero-value behavior, and update the test label
accordingly; reference the processMessage function, model.SendMessageRequest
type, the buildData closure, and the table entry named "malformed JSON" when
making the change.
In `@room-service/handler.go`:
- Line 1412: The BadRequest currently includes the interpolated subject (subj)
in the client error; change it to return errcode.BadRequest("invalid mute-toggle
subject") without including subj, and attach the subject as server-only metadata
using errcode.WithMetadata("subject", subj) (i.e., use
errcode.BadRequest(...).WithMetadata(...)) so the subject is logged for
debugging but not leaked to clients; update the return at the invalid
mute-toggle subject check that currently calls
errcode.BadRequest(fmt.Sprintf(...)).
- Line 815: Extract a small helper (e.g., channelExpandTimeout) that returns
errcode.Unavailable with a fixed message ("timeout listing members of channel")
and adds structured metadata using errcode.WithMetadata("roomId", roomID,
"siteId", siteID); then replace the three inline constructions that call
errcode.Unavailable(fmt.Sprintf("timeout listing members of channel %s@%s",
ref.RoomID, ref.SiteID)) with returns that call channelExpandTimeout(ref.RoomID,
ref.SiteID) so the error message is consistent and metadata is attached for
client-side rendering.
In `@search-service/handler_test.go`:
- Around line 175-177: Replace the manual error-code assertion pattern that uses
a typed variable (rerr *errcode.Error), errors.As(err, &rerr) and
assert.Equal(t, errcode.CodeBadRequest, rerr.Code) with the errtest helper: call
errtest.AssertCode(t, err, errcode.CodeBadRequest) (and errtest.AssertReason
where applicable) to reduce boilerplate; update the assertion at the location
referencing rerr/errors.As/errcode.CodeBadRequest and apply the same replacement
to the other listed occurrences in handler_test.go (~13 sites) so every
validation-error check uses errtest.AssertCode/AssertReason consistently.
In `@search-service/metrics_test.go`:
- Around line 17-35: The table-driven test
TestStatusLabel_CanonicalErrcodePassesThrough should use named subtests so
failures show which case failed: add a name field to each case (e.g.,
"bad_request", "unauthenticated", etc.), iterate over cases and call
t.Run(tc.name, func(t *testing.T) { if got := statusLabel(tc.err); got !=
tc.want { t.Errorf(... ) } }), moving the assertion inside the subtest closure
and referencing the existing statusLabel function and the
TestStatusLabel_CanonicalErrcodePassesThrough test.
🪄 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: 9c1e15b8-a752-4af3-b918-04e4ad351e64
📒 Files selected for processing (126)
.semgrep/errcode.ymlCLAUDE.mdMakefileauth-service/handler.goauth-service/handler_test.gobroadcast-worker/handler.gochat-frontend/CLAUDE.mdchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/api/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.jsxchat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsxchat-frontend/src/components/MainApp/ChatPage/ManageMembersDialog/MemberRoster/MemberRoster.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxchat-frontend/src/components/shared/MessageList/MessageRow/MessageActions/MessageActionMenu/MessageActionMenu.jsxchat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/lib/constants.jschat-frontend/src/lib/constants.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxdocs/client-api.mddocs/error-handling.mddocs/reviews/branch-claude-sharp-hopper-qzm6W-2026-05-31.mddocs/reviews/branch-claude-sharp-hopper-qzm6W-2026-06-01-2.mddocs/reviews/branch-claude-sharp-hopper-qzm6W-2026-06-01.mddocs/superpowers/plans/2026-05-28-centralized-error-codes.mddocs/superpowers/spec.mddocs/superpowers/specs/2026-05-28-centralized-error-codes-design.mdhistory-service/cmd/main.gohistory-service/internal/service/messages.gohistory-service/internal/service/messages_test.gohistory-service/internal/service/room_times.gohistory-service/internal/service/threads.gohistory-service/internal/service/threads_test.gohistory-service/internal/service/utils.goinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/main.gomessage-gatekeeper/fetcher_history.gomessage-gatekeeper/fetcher_history_test.gomessage-gatekeeper/handler.gomessage-gatekeeper/handler_test.gomessage-gatekeeper/store.gomessage-worker/handler.gomessage-worker/store_cassandra.gomock-user-service/handler.gomock-user-service/handler_test.gonotification-worker/handler.gonotification-worker/main.gopkg/errcode/category.gopkg/errcode/category_test.gopkg/errcode/classify.gopkg/errcode/classify_test.gopkg/errcode/codes_auth.gopkg/errcode/codes_message.gopkg/errcode/codes_room.gopkg/errcode/codes_search.gopkg/errcode/codes_test.gopkg/errcode/doc.gopkg/errcode/errhttp/write.gopkg/errcode/errhttp/write_test.gopkg/errcode/errnats/reply.gopkg/errcode/errnats/reply_test.gopkg/errcode/error.gopkg/errcode/error_test.gopkg/errcode/errtest/assert.gopkg/errcode/errtest/assert_test.gopkg/errcode/logctx.gopkg/errcode/logctx_test.gopkg/errcode/match.gopkg/errcode/match_test.gopkg/errcode/options.gopkg/errcode/options_test.gopkg/errcode/parse.gopkg/errcode/parse_test.gopkg/errcode/permanent.gopkg/errcode/permanent_test.gopkg/errcode/reason.gopkg/errcode/reason_test.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_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/params.gopkg/natsrouter/register.gopkg/natsrouter/router.gopkg/natsrouter/router_test.gopkg/natsutil/reply.gopkg/natsutil/reply_test.gopkg/roomcrypto/integration_test.gopkg/testutil/cassandra.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/memberlist_client.goroom-service/memberlist_client_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/main.gosearch-service/handler.gosearch-service/handler_test.gosearch-service/integration_apps_test.gosearch-service/integration_messages_test.gosearch-service/integration_rooms_test.gosearch-service/integration_users_test.gosearch-service/metrics.gosearch-service/metrics_test.gosearch-service/query_rooms.gosearch-service/query_rooms_test.go
💤 Files with no reviewable changes (3)
- pkg/model/error.go
- pkg/natsrouter/errors_test.go
- pkg/natsrouter/errors.go
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
Jun 1, 2026
PR #250 review feedback: 1. chat-frontend/src/pages/OidcCallback/OidcCallback.jsx:42-54 — redirectToReloginOnTokenInvalid() rejections were unhandled. Wrapped in try/catch so a failed redirect surfaces an error message instead of leaving the user on "Completing sign-in..." indefinitely. 2. docs/superpowers/specs/2026-05-28-centralized-error-codes-design.md — spec said "7 categories" but listed 8 in the table. The Go const snippet also omitted CodeTooManyRequests while the shipped implementation defines it. Bumped the count to 8 and added CodeTooManyRequests to the snippet. 3. message-gatekeeper/handler.go:295-300 — snap == nil with err == nil is a fetcher contract violation, not a genuine missing parent. Was returning errcode.NotFound, which would reply+Ack and permanently drop the message. Switched to bare fmt.Errorf so the dispatch classifies it as infra (Nak for redelivery + log). 4. room-service/helper.go:32 — errNotRoomMember is shared between list-members AND add-member channel-source expansion, but the message was list-specific ("only room members can list members"). Neutralized to "only room members can perform this action" so it's accurate for both call sites. Same code + RoomNotMember reason as before; only the user-safe text changed. Updated two test fixtures that pinned the old string. 5. search-service/handler.go:85 — Whitespace-only queries were passing the empty-check and reaching Elasticsearch as effectively empty search terms. Added strings.TrimSpace before validating, matching the other search handlers in this file. Verification: - make fmt / make lint — clean. - All affected Go packages pass `go test -race`. - chat-frontend: typecheck clean, 594/594 unit tests pass.
GITMateuszCharczuk
force-pushed
the
claude/sharp-hopper-qzm6W
branch
3 times, most recently
from
3f4b776 to
7112e17
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
room-service/integration_test.go (1)
1532-1540: ⚡ Quick winAssert the typed non-member error here too.
This is back to
err.Error()matching, so it no longer verifies the sentinel/errcode contract that this PR is standardizing.errors.Is(err, errNotRoomMember)would keep the test stable and contract-focused.As per coding guidelines,
**/*.go: Never compare errors by string — useerrors.Isanderrors.As.🤖 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 `@room-service/integration_test.go` around lines 1532 - 1540, The test currently asserts on the error string from h.handleGetRoomKey; change it to assert the typed sentinel error instead by using errors.Is(err, errNotRoomMember) (e.g. require.True(t, errors.Is(err, errNotRoomMember))). Locate the call to handleGetRoomKey and the existing string assertion (assert.Contains(t, err.Error(), "only room members")) and replace that line with the errors.Is check; ensure the test imports the standard errors package and continues to use subject.RoomKeyGet/RoomKeyGetRequest as before.room-service/handler_test.go (1)
4092-4095: ⚡ Quick winUse typed assertions for
handleGetRoomKeyfailures.These cases now key off
err.Error()substrings, so they can still pass if the handler returns the wrong typed client error but preserves the wording. Prefererrors.Is(err, errNotRoomMember)/errors.As(err, *errcode.Error)per case instead.As per coding guidelines,
**/*.go: Never compare errors by string — useerrors.Isanderrors.As.Also applies to: 4123-4180, 4192-4195
🤖 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 `@room-service/handler_test.go` around lines 4092 - 4095, The tests for handleGetRoomKey should stop asserting on err.Error() substrings and instead use typed error checks: replace string substring checks with errors.Is(err, errNotRoomMember) when expecting that sentinel, and use errors.As(err, &ee) to assert *errcode.Error and examine ee.Code or other fields for cases that expect specific errcode.Error values; update the table-driven test cases (the want struct checks) and the assertion logic in the handleGetRoomKey test to perform these errors.Is/errors.As checks rather than comparing err.Error() text.
🤖 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/errcode/errnats/reply_test.go`:
- Around line 124-180: Tests TestReply_LogsAtErrorLevelOnInternal,
TestReply_UnknownErrorCollapsesToInternal, and
TestReplyQuiet_RespondsButEmitsNoClassifyLine mix require.NoError with manual
if/t.Fatalf checks; replace those manual checks with testify/assert or require
calls for consistency (e.g., use assert.Equal/require.Equal for comparing
expected values, assert.Empty/require.Empty or assert.NotContains for string
checks, and require.NoError remains for unmarshalling). Update the assertions in
the Reply, ReplyQuiet, and related requestAndCaptureReply usages so all failure
checks use testify's assert/require helpers instead of t.Fatalf, referencing the
test function names above to locate each assertion to change.
- Around line 69-84: Replace the plain testing checks in
TestMarshalQuiet_DoesNotLogButStillCollapses with testify assertions: after
calling data := MarshalQuiet(errors.New("mongo down")), use require.NoError(t,
json.Unmarshal(data, &got)) to surface the unmarshal error instead of ignoring
it, then use assert.Equal(t, "internal", got["code"]) and assert.Equal(t,
"internal error", got["error"]) instead of t.Fatalf, and finally use
assert.Empty(t, buf.String(), "MarshalQuiet must not log") to check the buffer;
update imports to include testify's require and assert and remove the direct use
of t.Fatalf and the ignored unmarshal error.
- Around line 98-122: The test TestReply_RespondsWithEnvelopeAndLogsOnce mixes
t.Fatalf with testify; replace the t.Fatalf assertions with testify
require/assert calls for consistency: use require.Equal to assert got["code"],
got["reason"], and got["error"] values instead of the first t.Fatalf, use
require.Len(bufLines, 1) (or require.Len(lines, 1)) to check the log line count
instead of the second t.Fatalf, and use require.Equal("request failed",
line["msg"]) to validate the log message; keep the existing require.NoError
checks for unmarshaling and JSON parsing.
- Around line 48-67: Replace the manual json.Unmarshal error ignores and
t.Fatalf checks in TestMarshal_TypedError and
TestMarshal_UnknownCollapsesToInternal with testify assertions for consistency:
call require.NoError(t, json.Unmarshal(...)) to fail on unmarshal errors, then
use require.Equal/ assert.Equal (or require) to check got["code"], got["reason"]
and got["error"], and use assert.False/require.False or assert.NotContains to
ensure "reason" is absent in TestMarshal_UnknownCollapsesToInternal; locate the
changes around the TestMarshal_TypedError and
TestMarshal_UnknownCollapsesToInternal functions and the Marshal/ctxQuiet usage
to update the assertions.
---
Nitpick comments:
In `@room-service/handler_test.go`:
- Around line 4092-4095: The tests for handleGetRoomKey should stop asserting on
err.Error() substrings and instead use typed error checks: replace string
substring checks with errors.Is(err, errNotRoomMember) when expecting that
sentinel, and use errors.As(err, &ee) to assert *errcode.Error and examine
ee.Code or other fields for cases that expect specific errcode.Error values;
update the table-driven test cases (the want struct checks) and the assertion
logic in the handleGetRoomKey test to perform these errors.Is/errors.As checks
rather than comparing err.Error() text.
In `@room-service/integration_test.go`:
- Around line 1532-1540: The test currently asserts on the error string from
h.handleGetRoomKey; change it to assert the typed sentinel error instead by
using errors.Is(err, errNotRoomMember) (e.g. require.True(t, errors.Is(err,
errNotRoomMember))). Locate the call to handleGetRoomKey and the existing string
assertion (assert.Contains(t, err.Error(), "only room members")) and replace
that line with the errors.Is check; ensure the test imports the standard errors
package and continues to use subject.RoomKeyGet/RoomKeyGetRequest as before.
🪄 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: ea69afc7-2632-470a-8324-ed695cb0fce1
📒 Files selected for processing (132)
.semgrep/errcode.ymlCLAUDE.mdMakefileauth-service/handler.goauth-service/handler_test.goauth-service/middleware.gobroadcast-worker/handler.gobroadcast-worker/main.gochat-frontend/CLAUDE.mdchat-frontend/src/api/_transport/asyncJob.test.jschat-frontend/src/api/_transport/asyncJob.tschat-frontend/src/api/auth/oidcClient.jschat-frontend/src/api/index.tschat-frontend/src/api/types.tschat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.jsxchat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsxchat-frontend/src/components/MainApp/ChatPage/ManageMembersDialog/MemberRoster/MemberRoster.jsxchat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsxchat-frontend/src/components/shared/MessageList/MessageRow/MessageActions/MessageActionMenu/MessageActionMenu.jsxchat-frontend/src/context/NatsContext/NatsContext.jsxchat-frontend/src/lib/constants.jschat-frontend/src/lib/constants.test.jschat-frontend/src/pages/LoginPage/LoginPage.jsxchat-frontend/src/pages/LoginPage/LoginPage.test.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.jsxchat-frontend/src/pages/OidcCallback/OidcCallback.test.jsxdocs/client-api.mddocs/error-handling.mddocs/superpowers/plans/2026-05-28-centralized-error-codes.mddocs/superpowers/spec.mddocs/superpowers/specs/2026-05-28-centralized-error-codes-design.mddocs/superpowers/specs/2026-06-02-unified-worker-error-logging-design.mdhistory-service/cmd/main.gohistory-service/internal/service/messages.gohistory-service/internal/service/messages_test.gohistory-service/internal/service/room_times.gohistory-service/internal/service/threads.gohistory-service/internal/service/threads_test.gohistory-service/internal/service/utils.goinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/main.gomessage-gatekeeper/fetcher_history.gomessage-gatekeeper/fetcher_history_test.gomessage-gatekeeper/handler.gomessage-gatekeeper/handler_test.gomessage-gatekeeper/main.gomessage-gatekeeper/store.gomessage-worker/handler.gomessage-worker/main.gomessage-worker/store_cassandra.gomock-user-service/handler.gomock-user-service/handler_test.gonotification-worker/handler.gonotification-worker/main.gopkg/errcode/category.gopkg/errcode/category_test.gopkg/errcode/classify.gopkg/errcode/classify_test.gopkg/errcode/codes_auth.gopkg/errcode/codes_message.gopkg/errcode/codes_room.gopkg/errcode/codes_search.gopkg/errcode/codes_test.gopkg/errcode/doc.gopkg/errcode/errhttp/write.gopkg/errcode/errhttp/write_test.gopkg/errcode/errnats/reply.gopkg/errcode/errnats/reply_test.gopkg/errcode/error.gopkg/errcode/error_test.gopkg/errcode/errtest/assert.gopkg/errcode/errtest/assert_test.gopkg/errcode/logctx.gopkg/errcode/logctx_test.gopkg/errcode/match.gopkg/errcode/match_test.gopkg/errcode/options.gopkg/errcode/options_test.gopkg/errcode/parse.gopkg/errcode/parse_test.gopkg/errcode/permanent.gopkg/errcode/permanent_test.gopkg/errcode/reason.gopkg/errcode/reason_test.gopkg/idgen/idgen.gopkg/idgen/idgen_test.gopkg/model/error.gopkg/model/event.gopkg/model/member.gopkg/model/model_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/params.gopkg/natsrouter/register.gopkg/natsrouter/router.gopkg/natsrouter/router_test.gopkg/natsutil/reply.gopkg/natsutil/reply_test.gopkg/natsutil/request_id.gopkg/natsutil/request_id_test.gopkg/roomcrypto/integration_test.gopkg/testutil/cassandra.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/memberlist_client.goroom-service/memberlist_client_test.goroom-service/store.goroom-service/store_mongo.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/main.gosearch-service/handler.gosearch-service/handler_test.gosearch-service/integration_apps_test.gosearch-service/integration_messages_test.gosearch-service/integration_rooms_test.gosearch-service/integration_users_test.gosearch-service/metrics.gosearch-service/metrics_test.gosearch-service/query_rooms.gosearch-service/query_rooms_test.go
💤 Files with no reviewable changes (16)
- pkg/natsrouter/errors_test.go
- pkg/model/error.go
- pkg/natsrouter/errors.go
- search-service/integration_messages_test.go
- search-service/query_rooms_test.go
- search-service/integration_users_test.go
- room-worker/main.go
- search-service/query_rooms.go
- search-service/integration_apps_test.go
- search-service/handler_test.go
- search-service/integration_rooms_test.go
- search-service/metrics_test.go
- search-service/metrics.go
- room-worker/handler_test.go
- search-service/handler.go
- room-worker/handler.go
✅ Files skipped from review due to trivial changes (19)
- pkg/errcode/codes_search.go
- pkg/model/member.go
- notification-worker/handler.go
- pkg/errcode/codes_message.go
- pkg/errcode/codes_room.go
- pkg/errcode/error.go
- room-service/store.go
- pkg/errcode/parse_test.go
- pkg/natsrouter/doc.go
- pkg/errcode/doc.go
- docs/superpowers/spec.md
- message-worker/store_cassandra.go
- message-gatekeeper/fetcher_history_test.go
- chat-frontend/CLAUDE.md
- message-worker/handler.go
- broadcast-worker/handler.go
- docs/superpowers/specs/2026-06-02-unified-worker-error-logging-design.md
- docs/error-handling.md
- docs/superpowers/specs/2026-05-28-centralized-error-codes-design.md
🚧 Files skipped from review as they are similar to previous changes (76)
- chat-frontend/src/pages/LoginPage/LoginPage.test.jsx
- chat-frontend/src/components/MainApp/ChatPage/ManageMembersDialog/MemberRoster/MemberRoster.jsx
- pkg/errcode/match.go
- chat-frontend/src/pages/OidcCallback/OidcCallback.test.jsx
- chat-frontend/src/pages/LoginPage/LoginPage.jsx
- chat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.test.jsx
- pkg/errcode/match_test.go
- pkg/model/event.go
- pkg/errcode/codes_test.go
- pkg/errcode/logctx_test.go
- chat-frontend/src/api/index.ts
- message-gatekeeper/store.go
- pkg/errcode/reason_test.go
- pkg/errcode/codes_auth.go
- chat-frontend/src/components/MainApp/Sidebar/CreateRoomDialog/CreateRoomDialog.test.jsx
- pkg/testutil/cassandra.go
- inbox-worker/handler.go
- history-service/cmd/main.go
- mock-user-service/handler.go
- chat-frontend/src/pages/OidcCallback/OidcCallback.jsx
- pkg/errcode/error_test.go
- pkg/errcode/reason.go
- pkg/errcode/category_test.go
- pkg/errcode/category.go
- pkg/roomcrypto/integration_test.go
- pkg/errcode/errtest/assert.go
- chat-frontend/src/lib/constants.test.js
- .semgrep/errcode.yml
- Makefile
- chat-frontend/src/lib/constants.js
- history-service/internal/service/room_times.go
- message-gatekeeper/fetcher_history.go
- pkg/errcode/permanent.go
- pkg/errcode/errhttp/write.go
- pkg/natsrouter/params.go
- pkg/model/model_test.go
- chat-frontend/src/components/MainApp/ChatPage/LeaveRoomButton/LeaveRoomButton.jsx
- chat-frontend/src/api/auth/oidcClient.js
- pkg/errcode/errhttp/write_test.go
- pkg/natsrouter/context_test.go
- pkg/natsrouter/router.go
- pkg/natsrouter/register.go
- room-service/memberlist_client.go
- pkg/natsrouter/context.go
- chat-frontend/src/context/NatsContext/NatsContext.jsx
- pkg/errcode/options_test.go
- chat-frontend/src/api/_transport/asyncJob.test.js
- pkg/natsrouter/middleware.go
- chat-frontend/src/api/types.ts
- room-service/store_mongo.go
- pkg/natsutil/reply.go
- auth-service/handler_test.go
- chat-frontend/src/api/_transport/asyncJob.ts
- room-service/memberlist_client_test.go
- pkg/natsrouter/router_test.go
- pkg/errcode/permanent_test.go
- pkg/errcode/errnats/reply.go
- pkg/errcode/logctx.go
- pkg/errcode/classify.go
- history-service/internal/service/utils.go
- history-service/internal/service/threads_test.go
- pkg/natsrouter/README.md
- history-service/internal/service/threads.go
- pkg/errcode/classify_test.go
- auth-service/handler.go
- pkg/errcode/options.go
- pkg/errcode/errtest/assert_test.go
- pkg/natsutil/reply_test.go
- history-service/internal/service/messages.go
- room-service/helper.go
- docs/client-api.md
- history-service/internal/service/messages_test.go
- room-service/helper_test.go
- message-gatekeeper/handler.go
- room-service/handler.go
- mock-user-service/handler_test.go
GITMateuszCharczuk
force-pushed
the
claude/sharp-hopper-qzm6W
branch
9 times, most recently
from
b5ba553 to
5eb06fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/errcode-nats-talk.md`:
- Around line 14-19: The fenced code block showing the errcode.NotFound /
fmt.Errorf examples is unlabeled; update that triple-backtick fence in
docs/errcode-nats-talk.md to include a language tag (e.g., change ``` to
```text) so markdownlint MD040 is satisfied and the block is explicitly marked
as plain text.
🪄 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: 2f25c685-a7ab-4737-9cb9-6de08f635777
📒 Files selected for processing (16)
CLAUDE.mddocs/errcode-nats-talk.mdmessage-worker/handler.gopkg/errcode/codes_platform.gopkg/errcode/codes_room.gopkg/errcode/codes_test.gopkg/errcode/errnats/reply_test.gopkg/natsrouter/context.gopkg/natsrouter/example_test.gopkg/natsutil/request_id.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.gosearch-service/metrics.go
💤 Files with no reviewable changes (1)
- pkg/natsrouter/context.go
✅ Files skipped from review due to trivial changes (2)
- pkg/errcode/codes_test.go
- pkg/errcode/codes_platform.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/errcode/codes_room.go
- pkg/natsrouter/example_test.go
- search-service/metrics.go
- pkg/natsutil/request_id.go
- room-service/helper.go
- message-worker/handler.go
- pkg/errcode/errnats/reply_test.go
- room-service/integration_test.go
- room-service/helper_test.go
- room-service/handler.go
- room-service/handler_test.go
GITMateuszCharczuk
force-pushed
the
claude/sharp-hopper-qzm6W
branch
7 times, most recently
from
f673342 to
d996671
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reviews/pkg-errcode-readiness-2026-06-03.md (1)
1-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
docs/reviews/*artifact from the PR before merge.This file appears to be an internal review note and should not ship in the branch.
Based on learnings: "Delete every file under
docs/reviews/from the branch just before creating the PR — these reports are working notes for the author, not shippable artifacts."🤖 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 `@docs/reviews/pkg-errcode-readiness-2026-06-03.md` around lines 1 - 386, PR includes an internal review artifact (the long audit markdown added in this change); remove that file from the branch before merging by deleting the artifact and committing the deletion (or amend the PR commit to drop it), verify there are no other files under the reviews artifacts directory left in the branch, and push the updated branch so the merge contains no docs/reviews artifacts; optionally add a one-line note in your PR checklist reminding authors to remove docs/reviews files before creating a public PR.
🤖 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/errcode-nats-talk.md`:
- Line 3: The heading "pkg/errcode — one envelope from handler to frontend" is
using H3 (###) and should be H2 (##) to avoid jumping a level after the H1
title; change the leading hashes on that heading from "###" to "##" so the
markdown structure is valid and linter-compliant.
---
Outside diff comments:
In `@docs/reviews/pkg-errcode-readiness-2026-06-03.md`:
- Around line 1-386: PR includes an internal review artifact (the long audit
markdown added in this change); remove that file from the branch before merging
by deleting the artifact and committing the deletion (or amend the PR commit to
drop it), verify there are no other files under the reviews artifacts directory
left in the branch, and push the updated branch so the merge contains no
docs/reviews artifacts; optionally add a one-line note in your PR checklist
reminding authors to remove docs/reviews files before creating a public PR.
🪄 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: e79ccc1d-f93f-422b-b7fe-7fa92fe1b7f1
📒 Files selected for processing (17)
CLAUDE.mddocs/errcode-nats-talk.mddocs/reviews/pkg-errcode-readiness-2026-06-03.mdmessage-worker/handler.gopkg/errcode/codes_platform.gopkg/errcode/codes_room.gopkg/errcode/codes_test.gopkg/errcode/errnats/reply_test.gopkg/natsrouter/context.gopkg/natsrouter/example_test.gopkg/natsutil/request_id.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.gosearch-service/metrics.go
💤 Files with no reviewable changes (1)
- pkg/natsrouter/context.go
✅ Files skipped from review due to trivial changes (1)
- message-worker/handler.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/errcode/codes_test.go
- pkg/errcode/codes_room.go
- pkg/errcode/codes_platform.go
- pkg/natsrouter/example_test.go
- CLAUDE.md
- search-service/metrics.go
- pkg/errcode/errnats/reply_test.go
- room-service/helper.go
- room-service/integration_test.go
- room-service/helper_test.go
- pkg/natsutil/request_id.go
- room-service/handler.go
GITMateuszCharczuk
pushed a commit
that referenced
this pull request
Jun 3, 2026
main added pin.go using the legacy natsrouter.Err* API that PR #250 removes. Migrate to the same pattern the rest of history-service uses: named errcode constructors for client-facing errors and fmt.Errorf wraps for infra failures (Classify logs once at the boundary, so the local slog.Error calls were redundant). Also fixes a markdownlint H3-after-H1 heading bump in the deck.
…, and CI semgrep rules Adds the centralized error-codes design spec, the migration plan, the repo-wide error-handling guide, refreshes docs/client-api.md with the new error envelope shape and reason catalog, and lands the .semgrep/errcode.yml rules that enforce reason placement, WithCause usage, single-%w wrapping, and named constructors. CLAUDE.md picks up the link to the error-handling guide; Makefile gains the sast targets.
…catalogs Introduces pkg/errcode with 8 categories (bad_request, unauthenticated, forbidden, not_found, conflict, too_many_requests, unavailable, internal), a typed *Error with options pattern (WithReason, WithCause, WithMetadata, WithLogValues), Classify() emitting a single category-aware log line at the boundary, and Permanent/IsPermanent for JetStream Ack-vs-Nak dispatch. Includes the NATS transport adapter (errnats.Reply/Marshal), the Gin HTTP adapter (errhttp.Write), the errtest assertion helper, and per-domain reason catalogs (codes_room, codes_message, codes_search, codes_auth). Classify gates the err.Error() allocation when the cause is the errcode itself.
…or errcode Deletes pkg/model/error.go (the legacy untyped ErrorResponse) — all client-facing errors now flow through the errcode envelope. Updates event/member structs (AsyncJobResult code/reason, CreateRoomStatusExists) to match the new envelope shape and refreshes model_test.go roundtrips.
…leware enriches slog ctx
natsrouter now replies with errcode envelopes natively — RouteError /
ErrorResponse / Err* helpers and pkg/model/error.go are gone. Adds
RequestID() middleware that extracts the X-Request-ID NATS header (or
mints a UUIDv7 via idgen), stores it via natsutil.WithRequestID, and
calls c.WithLogValues("request_id", reqID) so every slog…Context call
inside the handler picks it up automatically. Default() pre-installs
Recovery, RequestID, Logging.
Fixes the chainState sync.Pool use-after-release: releaseContext nils
c.chain, and Next/Abort/IsAborted nil-check with a clear panic message
instead of silently corrupting another request's chain.
pkg/natsutil.Reply gains a marshal-failure path test and adopts the
errcode envelope shape; reply_test.go covers happy + marshal-error
paths against an in-memory NATS server.
…dup paths Consolidates the request-ID rework: chokepoint minting in idgen, the StampRequestID boundary helper, migration of the five workers off reject-on-missing, and the strict RequireRequestID variant that rejects on dedup-critical room paths. Includes the renamed-helper comment cleanup. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
Stamp and forward X-Request-ID on the cross-site list-members path and cover it with an integration test. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
Fix natsGetRoomKey for the errcode envelope shape (rebase conflict resolution) and drop the deleted sanitizeError reference from the integration test. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
…error log Design spec for the deferred unified worker error logging (LogJobError) plus the message-worker process-message error-log request_id one-liner. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
Adds the multi-lens branch review report and applies the curated follow-ups: message-worker slog *Context + request_id correlation, dead ReplyError removal, RequireRequestID reason, and the search-service comment-count fix. Also folds in the CodeRabbit test-quality nits that align with CLAUDE.md: room-service tests assert error identity via require.ErrorIs against the typed sentinels instead of err.Error() string matching, and errnats/reply_test.go uses testify require/assert (require.NoError on the JSON unmarshals) instead of t.Fatalf with ignored unmarshal errors. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
main added pin.go using the legacy natsrouter.Err* API that PR #250 removes. Migrate to the same pattern the rest of history-service uses: named errcode constructors for client-facing errors and fmt.Errorf wraps for infra failures (Classify logs once at the boundary, so the local slog.Error calls were redundant). Also fixes a markdownlint H3-after-H1 heading bump in the deck.
…vability
Three independent follow-ups landing together because the prior CI run
gated on them:
1. Toolchain bump 1.25.10 -> 1.25.11 (Makefile, go.mod, ci.yml)
Clears two stdlib govulncheck advisories that were the only SAST gate
failure (GO-2026-5039 net/textproto, GO-2026-5037 crypto/x509). Neither
trace touches anything this PR added. `make sast` is now green locally.
2. Restore per-request log fields in pin handlers
pin.go's PinMessage/UnpinMessage/ListPinnedMessages now call
c.WithLogValues("account", account, "room_id", roomID) at handler
entry, matching every other handler in messages.go/threads.go. Without
this, the boundary Classify log emits no account/room_id fields for
pin-feature errors -- a silent observability regression that landed
when the pin.go migration dropped its per-error slog.Error calls in
favor of boundary logging.
3. Distinct reason codes for the 5 'forbidden' pin cases
pin.go's 3 unique pin-specific forbidden cases now carry typed
Reason constants so the frontend can render specific copy instead of
falling through to err.message:
- pin_disabled (kill-switch)
- pin_limit_reached (hard cap)
- pin_room_too_large (size gate, non-admin)
The 'not subscribed' case reuses the existing MessageNotSubscribed.
pkg/errcode/codes_message.go + codes_test.go (allReasons) get the new
constants; chat-frontend/.../asyncJob.ts REASON_COPY map gets the
user-facing english. docs/client-api.md pin/unpin/list error tables
gain a Reason column and the master \xa76 reason catalog gets the new
entries; also fixes the stale 'one of 7 generic categories' line to
'8'.
GITMateuszCharczuk
force-pushed
the
claude/sharp-hopper-qzm6W
branch
from
79733e6 to
36b5a9f
Compare
pkg/atrest does not import errcode; its sentinels (ErrAuthFailed, ErrPayloadMalformed) are plain errors.New strings. The multi-%w idiom there joins a sentinel with the underlying crypto/json error so errors.Is works for both — there is no errcode error in the chain, so the rule's invariant cannot be violated. Same shape as the existing pkg/errcode/** exclusion. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
nats-subject-naming.md was missing several request/reply entries from
recently-merged PRs:
- msg.{get,thread,edit,delete,pin,unpin,pinned.list} (PR #205 pin/unpin,
plus pre-existing edit/delete/get/thread that were never documented)
- room.{roomID}.{siteID}.key.get (PR #259, client-callable room key
fetch RPC)
- search.{siteID}.{messages,rooms,apps,users} (PR #232, siteID scoping)
Added matching builder + wildcard entries so the Subject Builders
catalog matches pkg/subject.
CLAUDE.md had no mention of at-rest message encryption (PR #155). Added
a "Project-Specific Patterns" subsection covering the envelope-encrypt
contract (what's in EncryptedFields vs. what stays plaintext), which
services own which piece (message-worker writes / lazy DEK,
history-service decrypts, room-worker eager DEK on sync DM), and the
config surface (ATREST_ENABLED + Vault settings) so future service
authors know to take an atrest.Cipher rather than touching plaintext
columns directly.
https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
This reverts commit c599866.
Working notes from the branch_review skill, not shippable artifacts. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
The previous test-integration (history-service) run failed with "Get https://registry-1.docker.io/v2/: context deadline exceeded" when testcontainers tried to pull Cassandra/MongoDB images. Re-run. https://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
mliu33
reviewed
Jun 3, 2026
| h.sendReply(ctx, accountFromSubject(msg.Subject()), msg.Data(), natsutil.MarshalError("invalid message subject")) | ||
| // Best-effort error reply so the client doesn't hang; sendReply no-ops | ||
| // when account or requestId is unusable. Ack — malformed is not retryable. | ||
| h.sendReply(ctx, accountFromSubject(msg.Subject()), &req, errnats.Marshal(ctx, errcode.BadRequest("invalid message subject"))) |
Collaborator
There was a problem hiding this comment.
"account" is already obtained from line 70 "subject.ParseUserRoomSiteSubject"
mliu33
approved these changes
Jun 3, 2026
Collaborator
mliu33
left a comment
mliu33
left a comment
There was a problem hiding this comment.
Outstanding work! Thanks!!
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
Squashed 22 commits for clean rebase onto post-#250 main.
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
Squashed 22 commits for clean rebase onto post-#250 main.
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
Squashed 22 commits for clean rebase onto post-#250 main.
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
…errcode) Two read-only NATS request/reply RPCs gated by room-membership OR platform-admin: GetRoomAppTabs and GetRoomAppCommandMenu. Squash-rebased onto latest main and migrated to the centralized error handling introduced by #250: sentinels are now *errcode.Error (errAppAccessDenied -> Forbidden+RoomNotMember, errResponseTooLarge -> Internal); the NATS wrappers use wrappedCtx's (ctx, err) form + errnats.Reply; marshalBounded returns ([]byte, error); sanitizeError removed. Invalid-subject parse failures return errcode.BadRequest. Includes pkg/model types, pkg/subject builders, three RoomStore methods + compound indexes, SITE_URL config, and client-api docs. The 38 prior PR commits are collapsed here because the cross-cutting errcode migration made a per-commit replay impractical. https://claude.ai/code/session_016e4UVPcE4kQQoHrYa2H2yB
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
…errcode) Two read-only NATS request/reply RPCs gated by room-membership OR platform-admin: GetRoomAppTabs and GetRoomAppCommandMenu. Squash-rebased onto latest main and migrated to the centralized error handling introduced by #250: sentinels are now *errcode.Error (errAppAccessDenied -> Forbidden+RoomNotMember, errResponseTooLarge -> Internal); the NATS wrappers use wrappedCtx's (ctx, err) form + errnats.Reply; marshalBounded returns ([]byte, error); sanitizeError removed. Invalid-subject parse failures return errcode.BadRequest. Includes pkg/model types, pkg/subject builders, three RoomStore methods + compound indexes, SITE_URL config, and client-api docs. The 38 prior PR commits are collapsed here because the cross-cutting errcode migration made a per-commit replay impractical. https://claude.ai/code/session_016e4UVPcE4kQQoHrYa2H2yB
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 3, 2026
…errcode) Two read-only NATS request/reply RPCs gated by room-membership OR platform-admin: GetRoomAppTabs and GetRoomAppCommandMenu. Squash-rebased onto latest main and migrated to the centralized error handling introduced by #250: sentinels are now *errcode.Error (errAppAccessDenied -> Forbidden+RoomNotMember, errResponseTooLarge -> Internal); the NATS wrappers use wrappedCtx's (ctx, err) form + errnats.Reply; marshalBounded returns ([]byte, error); sanitizeError removed. Invalid-subject parse failures return errcode.BadRequest. Includes pkg/model types, pkg/subject builders, three RoomStore methods + compound indexes, SITE_URL config, and client-api docs. The 38 prior PR commits are collapsed here because the cross-cutting errcode migration made a per-commit replay impractical. https://claude.ai/code/session_016e4UVPcE4kQQoHrYa2H2yB
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
Introduces a unified error-handling infrastructure (
pkg/errcode) that replaces four incompatible error-reply patterns across the codebase. All client-facing errors — over NATS sync replies, JetStream async results, and HTTP — now marshal to a single envelope:{ "error": <message>, "code": <category>, "reason"?: <domain-code>, "metadata"?: {…} }.The migration decouples
pkg/natsrouterfrom error semantics, retirespkg/model.ErrorResponseand four legacypkg/natsutilhelpers, and adds thetoo_many_requests(429) category. All 9 services (auth-service, history-service, inbox-worker, message-gatekeeper, mock-user-service, notification-worker, room-service, room-worker, search-service) are migrated to typederrcodeenvelopes.Key Changes
New
pkg/errcodepackage with core types:*Error— canonical client-facing error type (unexportedcausefield prevents leaks)Code— closed set of 8 generic categories (bad_request, unauthenticated, forbidden, not_found, conflict, unavailable, too_many_requests, internal)Reason— open set of domain-specific machine codes (e.g.,RoomNotMember,AuthTokenExpired)BadRequest(),NotFound(), etc.) andOptionpattern forWithCause(),WithReason(),WithMetadata()Classify()— converts any error to client-safe*Errorand logs once server-sideErrPermanentsentinel for non-retryable job failures (replaces ad-hocerrors.New("permanent"))Transport adapters:
pkg/errcode/errnats— NATS request/reply adapter;Reply()marshals*Errorto JSON and sends as replypkg/errcode/errhttp— Gin HTTP adapter;Write()sets status code and JSON response bodypkg/errcode/errtest— test assertions for wire envelopesService migrations:
*errcode.Erroror wrap errors withfmt.ErrorfforClassify()to handleerrInvalidRole) are now typed as*errcode.Errorso they classify directly at reply boundariesmodel.AsyncJobResult) extended withCodeandReasonfieldsinfraErrorwrapper types; transient vs. permanent distinction now viaErrPermanentsentinelRemoved legacy patterns:
pkg/natsrouter/errors.go—RouteError,Err*constructors,Code*string constspkg/natsutilhelpers —MarshalError,MarshalErrorWithCode,ReplyError,TryParseErrorpkg/model.ErrorResponsestructauth-servicead-hocgin.H{"error": ...}repliesDocumentation & tooling:
docs/error-handling.md— developer guide for producing client-facing errorsdocs/client-api.md— updated with new envelope format and behavior changes.semgrep/errcode.yml— rules to enforceReasoncodes are declared as typed constantsClassify(), wire marshaling, and transport adaptersFrontend updates:
chat-frontend/src/api/_transport/asyncJob.ts— updated to handle new error envelope shapechat-frontend/src/lib/constants.js— predicate for detecting error envelopes (supports both old and new shapes during rollout)reasoncodes (e.g., `isSSOhttps://claude.ai/code/session_01L8doEHgS2fgje7Gh6nZQpa
Summary by CodeRabbit
New Features
Bug Fixes
Documentation