refactor: rework app logging, dependency injection and cancellation#844
Merged
steveiliop56 merged 22 commits intomainfrom May 10, 2026
Merged
refactor: rework app logging, dependency injection and cancellation#844steveiliop56 merged 22 commits intomainfrom
steveiliop56 merged 22 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigrates logging to a new internal logger, introduces RuntimeConfig and sentinel errors, refactors bootstrap/services/controllers/middleware to dependency-injected constructors with context/WaitGroup lifecycles, updates CLI logging calls, and standardizes tests with shared helpers and require assertions. ChangesLogger, runtime, and contracts
Services
Controllers
Middleware
Bootstrap
CLI
Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
dosubot
Bot
added
the
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/bootstrap/db_bootstrap.go (1)
24-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
*sql.DBis leaked on every error path aftersql.Open.If
iofs.New,sqlite3.WithInstance,migrate.NewWithInstance, ormigrator.Upfails, the function returns without callingdb.Close(). The connection pool (and underlying file handle in modernc/sqlite) is not released, and on retry/restart loops this can accumulate. Defer-close on the *sql.DB until success, then transfer ownership toapp.db.🛡️ Proposed fix
db, err := sql.Open("sqlite", app.config.Database.Path) - if err != nil { return fmt.Errorf("failed to open database: %w", err) } + // Ensure the connection is released on any error path below. + success := false + defer func() { + if !success { + _ = db.Close() + } + }() + // Limit to 1 connection to sequence writes, this may need to be revisited in the future // if the sqlite connection starts being a bottleneck db.SetMaxOpenConns(1) migrations, err := iofs.New(assets.Migrations, "migrations") - if err != nil { return fmt.Errorf("failed to create migrations: %w", err) } target, err := sqlite3.WithInstance(db, &sqlite3.Config{}) - if err != nil { return fmt.Errorf("failed to create sqlite3 instance: %w", err) } migrator, err := migrate.NewWithInstance("iofs", migrations, "sqlite3", target) - if err != nil { return fmt.Errorf("failed to create migrator: %w", err) } - if err := migrator.Up(); err != nil && err != migrate.ErrNoChange { + if err := migrator.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { return fmt.Errorf("failed to migrate database: %w", err) } app.db = db + success = true return nil }(Also switched the
migrate.ErrNoChangecheck toerrors.Isfor wrap-safety — minor.)🤖 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 `@internal/bootstrap/db_bootstrap.go` around lines 24 - 58, The *sql.DB opened by sql.Open must be closed on all error paths until we successfully attach it to the app; modify the db_bootstrap logic so that immediately after db, err := sql.Open(...) you schedule a deferred close (e.g., defer func() { if !success { db.Close() } }()) or similar, set a local success flag only after all steps (iofs.New, sqlite3.WithInstance, migrate.NewWithInstance, migrator.Up) succeed, then assign app.db = db and flip the flag to avoid closing the live DB; also replace the direct equality check for migrate.ErrNoChange with errors.Is(err, migrate.ErrNoChange) when evaluating migrator.Up() errors to preserve wrapped errors.internal/controller/oauth_controller.go (1)
134-277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
oauth_controllerredirects to useruntime.AppURLfor consistency.
oauth_controller.gousescontroller.config.AppURLfor all/error,/login,/unauthorized,/authorize, and/continueredirects, whileproxy_controller.go,context_controller.go, and tests consistently usecontroller.runtime.AppURL. SinceAppURLexists in bothConfigandRuntimeConfig, and the broader codebase standardizes onruntime.AppURL, migrate all 11+ redirect calls in this method tocontroller.runtime.AppURLto maintain a single source of truth.🤖 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 `@internal/controller/oauth_controller.go` around lines 134 - 277, The redirect URLs in the OAuth callback flow use controller.config.AppURL but must use the canonical controller.runtime.AppURL; update every redirect call in the OAuth handler (references around oauthPendingSession, the state/checks, CreateSession and the final redirects) to replace controller.config.AppURL with controller.runtime.AppURL for all paths (/error, /unauthorized, /authorize, /continue, and the base AppURL) so the controller uses the runtime single source of truth.internal/controller/oidc_controller.go (2)
469-513:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
authorizeErrorstill assumescontroller.oidcis non-nil.
Authorizenow calls this helper for the "OIDC not configured" path with an empty callback, but the fallback branch still usescontroller.oidc.GetIssuer(). That turns the new guard into a nil-pointer panic instead of an error response.🤖 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 `@internal/controller/oidc_controller.go` around lines 469 - 513, authorizeError assumes controller.oidc is non-nil and calls controller.oidc.GetIssuer() in the fallback branch, which can panic when authorize calls this helper for the "OIDC not configured" path; update authorizeError to nil-check controller.oidc before calling GetIssuer and use a safe fallback (e.g. a predefined error URL or an empty string) or return an appropriate error response when controller.oidc is nil; locate the logic in authorizeError and change the fallback that builds the redirect_uri (where it creates ErrorScreen, encodes queries and uses controller.oidc.GetIssuer()) to first verify controller.oidc != nil and handle the nil case without dereferencing it.
77-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
GetClientInfowhen OIDC is disabled.This route is still registered even when
oidcServiceis nil, andcontroller.oidc.GetClient(...)will panic in that case. Please short-circuit with the same disabled/not-found response used inTokenandUserinfo.🤖 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 `@internal/controller/oidc_controller.go` around lines 77 - 90, GetClientInfo must guard against a nil OIDC service to avoid panics: in the OIDCController.GetClientInfo method, check if controller.oidc == nil (the same guard used in Token and Userinfo) and return the same disabled/not-found JSON response before calling controller.oidc.GetClient(req.ClientID); update the method to short-circuit when oidc is nil so controller.oidc.GetClient(...) is never invoked on a nil receiver.
🧹 Nitpick comments (6)
internal/model/context_test.go (1)
234-242: 💤 Low valuePrefer
errors.Isfor sentinel error assertion.Comparing
err.Error()againstmodel.ErrUserContextNotFound.Error()works for now, but it bypasses the actual benefit of a sentinel (wrap-safe identity checks viaerrors.Is). Consider adding a dedicated assertion path for this case, or asserting witherrors.Isdirectly inside therunclosure and returning a bool.Example sketch (separate from the table-based cases)
t.Run("NewFromGin returns ErrUserContextNotFound when context value is missing", func(t *testing.T) { var c model.UserContext _, err := c.NewFromGin(newGinCtx(nil, false)) require.ErrorIs(t, err, model.ErrUserContextNotFound) })🤖 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 `@internal/model/context_test.go` around lines 234 - 242, The test case currently compares error strings; change it to use errors.Is (or require.ErrorIs) so sentinel comparisons are wrap-safe: inside the table case for the NewFromGin scenario (or in a dedicated t.Run), have the run closure call c.NewFromGin(newGinCtx(nil, false)), check err with errors.Is(err, model.ErrUserContextNotFound) (or use require.ErrorIs) and return a bool (or make the test use require.ErrorIs directly) instead of returning err.Error(); reference model.UserContext.NewFromGin and model.ErrUserContextNotFound when updating the test.internal/utils/logger/logger.go (1)
123-155:CallerSkipFrame(1)is appropriate for current audit method usage patterns.All audit method calls (
AuditLoginSuccess,AuditLoginFailure,AuditLogout) are direct invocations from handler methods in controllers. The concern about future wrapping (e.g., middleware, service helpers) is architecturally valid; if audit logging is later extracted into shared helpers, the caller frame offset will need adjustment. Consider documenting this assumption or revisiting the implementation strategy if the codebase introduces audit logging wrappers.🤖 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 `@internal/utils/logger/logger.go` around lines 123 - 155, The audit methods AuditLoginSuccess, AuditLoginFailure and AuditLogout rely on CallerSkipFrame(1) assuming they are called directly from controller handlers; add a short inline comment/TODO above these methods documenting this assumption and that the CallerSkipFrame value must be increased if audit logging is moved into middleware/wrappers or helper functions, so future refactors adjust CallerSkipFrame accordingly (or make the skip configurable at that time).internal/service/ldap_service.go (1)
253-278: 💤 Low valuePass
ldap.contexttobackoff.Retryso reconnect honors shutdown.
backoff.Retry(context.TODO(), ...)ignores the service context, so an in-flight reconnect (up to 3 tries × multiplier 1.5 starting at 500 ms) keeps running afterctxis cancelled, delaying clean shutdown. The reconnect routine is the only path here, and it's already invoked from the heartbeat goroutine that ownsldap.context.♻️ Proposed change
- _, err := backoff.Retry(context.TODO(), operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3)) + _, err := backoff.Retry(ldap.context, operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3))🤖 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 `@internal/service/ldap_service.go` around lines 253 - 278, The reconnect function currently calls backoff.Retry with context.TODO(), which prevents the retry loop from honoring shutdown; update the call in LdapService.reconnect to pass the service context (ldap.context) instead of context.TODO() so retries are cancelled when the service context is done; locate the backoff.Retry invocation in reconnect and replace the context argument with ldap.context (no other behavioral changes required).internal/service/access_controls_service.go (1)
14-65: ⚡ Quick winDrop the pointer-to-interface for
labelProvider.
*LabelProvideris a pointer to an interface, which is an antipattern in Go: interface values are already nil-able and carry their own indirection, so wrapping them in*Tadds unnecessary overhead and forces awkward(*acls.labelProvider).GetLabels(domain)call sites. Removing the pointer wrapper allows the bootstrap code to pass the interface value directly instead of&labelProvider, and the nil check remains unchanged.♻️ Proposed change
type AccessControlsService struct { log *logger.Logger - labelProvider *LabelProvider + labelProvider LabelProvider static map[string]model.App } func NewAccessControlsService( log *logger.Logger, - labelProvider *LabelProvider, + labelProvider LabelProvider, static map[string]model.App) *AccessControlsService { return &AccessControlsService{ log: log, labelProvider: labelProvider, static: static, } } // If we have a label provider configured, try to get ACLs from it if acls.labelProvider != nil { - return (*acls.labelProvider).GetLabels(domain) + return acls.labelProvider.GetLabels(domain) }Update the bootstrap call site at
internal/bootstrap/service_bootstrap.go:48from&labelProvidertolabelProvider.🤖 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 `@internal/service/access_controls_service.go` around lines 14 - 65, The AccessControlsService currently stores labelProvider as a pointer-to-interface (*LabelProvider); change the field type in AccessControlsService to LabelProvider (remove the *), update NewAccessControlsService signature to accept labelProvider LabelProvider, and update all uses: in GetAccessControls call labelProvider.GetLabels(domain) (remove the (*...).GetLabels dereference) and keep the nil check as before (if acls.labelProvider != nil). Also update the bootstrap/constructor call site where NewAccessControlsService is invoked to pass labelProvider (not &labelProvider).internal/controller/user_controller_test.go (1)
29-30: ⚡ Quick winFinish the runtime-driven migration for cookie-domain assertions.
This test now builds
runtime, but several cookie checks later in the table still hardcode"example.com". Switching those expectations toruntime.CookieDomainwill keep the suite aligned withcreateTestConfigsinstead of the helper’s current default.🤖 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 `@internal/controller/user_controller_test.go` around lines 29 - 30, The tests in user_controller_test.go build a runtime via createTestConfigs(t) but still assert cookie domain strings using the hardcoded "example.com"; update those assertions to use runtime.CookieDomain instead so expectations follow the test runtime configuration. Locate assertions in the test table that compare or expect cookie domains (e.g., header/cookie checks) and replace the literal "example.com" with runtime.CookieDomain, ensuring any related helper calls or expected values reference runtime.CookieDomain consistently.internal/controller/proxy_controller_test.go (1)
24-25: ⚡ Quick winUse
runtime.AppURLfor expected redirect URLs.This test is now config-driven, but the expected login redirects still embed
https://tinyauth.example.com. Deriving those expectations fromruntime.AppURLwill keep the assertions synchronized withcreateTestConfigsand avoid unrelated breakage when the helper changes.🤖 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 `@internal/controller/proxy_controller_test.go` around lines 24 - 25, The test currently asserts hardcoded redirect URLs ("https://tinyauth.example.com...") even though createTestConfigs(t) returns a runtime with the authoritative AppURL; update the assertions to derive expected redirect URLs from runtime.AppURL (returned by createTestConfigs) instead of the literal string. Locate the test uses around cfg, runtime := createTestConfigs(t) and replace occurrences of the hardcoded base URL with runtime.AppURL (e.g., build expected strings with runtime.AppURL + path or fmt.Sprintf("%s%s", runtime.AppURL, "/login") ) so the expected redirects stay in sync with the test config.
🤖 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 `@internal/bootstrap/app_bootstrap.go`:
- Around line 278-290: When a listener error is received from errChan, initiate
the normal shutdown before returning the error: invoke the application's cancel
function (call the app's cancel method/func used to cancel app.ctx), wait for
background goroutines to finish (app.wg.Wait()), perform the DB close/logging
path (app.db.Close() and the existing log messages), and only then return
fmt.Errorf("server error: %w", err); replace the current immediate return inside
the case err := <-errChan block with this shutdown sequence using the same
symbols (app.ctx, app.cancel(), app.wg.Wait(), app.db.Close(), errChan).
- Around line 305-308: The shutdown goroutines currently call server.Close(),
which drops active connections; replace those calls in serveHTTP() and
serveUnix() with server.Shutdown(ctx) using a cancellable context (e.g.,
context.WithTimeout(app.ctx, <reasonable timeout>)) so in-flight requests drain
gracefully, handle and log any Shutdown error via app.log.App, and ensure you
cancel the timeout context after Shutdown returns; update the goroutine bodies
that reference server.Close() to create the timeout context, call
server.Shutdown(timeoutCtx), log errors, and defer timeout cancel().
In `@internal/controller/controller_test.go`:
- Around line 14-106: Extract the duplicated createTestConfigs helper into a
shared test package (e.g., package testutil) and export it as
CreateTestConfigs(t *testing.T) (model.Config, model.RuntimeConfig) plus export
the TOTP secret as TestingTOTPSecret; move the full implementation (including
bcrypt.GenerateFromPassword, model.Config, model.RuntimeConfig, OIDC client
conversion, CookieDomain/AppURL, etc.) into internal/testutil/config.go, update
internal/controller/controller_test.go and
internal/middleware/middleware_test.go to call testutil.CreateTestConfigs(t) and
replace testingTOTPSecret with testutil.TestingTOTPSecret, and add the new
import for the testutil package in both test files.
In `@internal/controller/oauth_controller.go`:
- Around line 166-172: GetOAuthUserinfo's error is ignored and user may be nil,
causing a panic when accessing user.Email; change the handler around
controller.auth.GetOAuthUserinfo(sessionIdCookie) to check err (and that user !=
nil) immediately after the call, log the error with controller.log.App (use
Warn/Error) and perform the same redirect to fmt.Sprintf("%s/error",
controller.config.AppURL) and return if there is an error or nil user before
accessing user.Email; update any existing controller.log.App.Warn("OAuth
provider did not return an email") usage to only run when user is non-nil but
Email is empty.
In `@internal/controller/well_known_controller_test.go`:
- Around line 101-102: The test calls service.NewOIDCService(log, cfg, runtime,
queries, ctx, wg) and continues even if it returns an error; immediately after
the call (where oidcService, err := service.NewOIDCService(...) is assigned) add
a require.NoError(t, err) to fail fast on setup errors so the test stops before
registering routes or exercising handlers when oidcService construction failed.
In `@internal/service/docker_service.go`:
- Line 53: The call wg.Go(service.watchAndClose) is invalid because
sync.WaitGroup has no Go method; update the caller and function: either change
the parameter type from *sync.WaitGroup to *errgroup.Group (import
golang.org/x/sync/errgroup), change service.watchAndClose signature to return
error and keep wg.Go(service.watchAndClose), or keep sync.WaitGroup and replace
the call with the standard pattern wg.Add(1); go func() { defer wg.Done(); _ =
service.watchAndClose() } (or handle its returned error inside the goroutine if
you alter its signature to return error); reference the symbols wg and
service.watchAndClose when making the change.
In `@internal/service/oidc_service_test.go`:
- Around line 65-76: The test builds a RuntimeConfig without any OIDCClients so
NewOIDCService returns (nil, nil) and the subsequent CompileUserinfo call runs
on a nil receiver; populate runtime.OIDCClients with at least one valid client
entry (or otherwise provide cfg values that cause NewOIDCService to construct a
non-nil service) before calling service.NewOIDCService, then assert svc is
non-nil (e.g., require.NotNil(t, svc)) so CompileUserinfo is invoked on a real
OIDCService instance; refer to RuntimeConfig.OIDCClients, NewOIDCService, and
CompileUserinfo to locate and fix the test setup.
In `@internal/service/oidc_service.go`:
- Around line 789-800: The loop over expiredCodes uses GetOidcTokenBySub and
logs non-sql.ErrNoRows errors but then continues using the zero-value token,
which can trigger DeleteOldSession incorrectly; update the error handling inside
the loop (the block around GetOidcTokenBySub in the same function) so that after
logging any non-sql.ErrNoRows error you skip to the next expiredCode (e.g.,
continue the loop) instead of proceeding to check token.TokenExpiresAt and
calling DeleteOldSession for that subject.
In `@internal/utils/logger/logger_test.go`:
- Line 165: The test calls assert.NotContains with arguments reversed; change
the call to use the haystack first and needle second so that
assert.NotContains(t, buf.String(), "test_nop") is used instead of
assert.NotContains(t, "test_nop", buf.String()); update the assertion in
logger_test.go to pass buf.String() as the second parameter and "test_nop" as
the third.
In `@internal/utils/logger/logger.go`:
- Line 36: Update the comment in internal/utils/logger/logger.go that currently
reads "No reason to enabled audit by default since it will be suppressed by the
log level" to correct the grammar by replacing "enabled" with "enable" so it
reads "No reason to enable audit by default since it will be suppressed by the
log level"; locate the comment near the audit-related logging logic in the
logger implementation to ensure the change is applied to the right comment.
---
Outside diff comments:
In `@internal/bootstrap/db_bootstrap.go`:
- Around line 24-58: The *sql.DB opened by sql.Open must be closed on all error
paths until we successfully attach it to the app; modify the db_bootstrap logic
so that immediately after db, err := sql.Open(...) you schedule a deferred close
(e.g., defer func() { if !success { db.Close() } }()) or similar, set a local
success flag only after all steps (iofs.New, sqlite3.WithInstance,
migrate.NewWithInstance, migrator.Up) succeed, then assign app.db = db and flip
the flag to avoid closing the live DB; also replace the direct equality check
for migrate.ErrNoChange with errors.Is(err, migrate.ErrNoChange) when evaluating
migrator.Up() errors to preserve wrapped errors.
In `@internal/controller/oauth_controller.go`:
- Around line 134-277: The redirect URLs in the OAuth callback flow use
controller.config.AppURL but must use the canonical controller.runtime.AppURL;
update every redirect call in the OAuth handler (references around
oauthPendingSession, the state/checks, CreateSession and the final redirects) to
replace controller.config.AppURL with controller.runtime.AppURL for all paths
(/error, /unauthorized, /authorize, /continue, and the base AppURL) so the
controller uses the runtime single source of truth.
In `@internal/controller/oidc_controller.go`:
- Around line 469-513: authorizeError assumes controller.oidc is non-nil and
calls controller.oidc.GetIssuer() in the fallback branch, which can panic when
authorize calls this helper for the "OIDC not configured" path; update
authorizeError to nil-check controller.oidc before calling GetIssuer and use a
safe fallback (e.g. a predefined error URL or an empty string) or return an
appropriate error response when controller.oidc is nil; locate the logic in
authorizeError and change the fallback that builds the redirect_uri (where it
creates ErrorScreen, encodes queries and uses controller.oidc.GetIssuer()) to
first verify controller.oidc != nil and handle the nil case without
dereferencing it.
- Around line 77-90: GetClientInfo must guard against a nil OIDC service to
avoid panics: in the OIDCController.GetClientInfo method, check if
controller.oidc == nil (the same guard used in Token and Userinfo) and return
the same disabled/not-found JSON response before calling
controller.oidc.GetClient(req.ClientID); update the method to short-circuit when
oidc is nil so controller.oidc.GetClient(...) is never invoked on a nil
receiver.
---
Nitpick comments:
In `@internal/controller/proxy_controller_test.go`:
- Around line 24-25: The test currently asserts hardcoded redirect URLs
("https://tinyauth.example.com...") even though createTestConfigs(t) returns a
runtime with the authoritative AppURL; update the assertions to derive expected
redirect URLs from runtime.AppURL (returned by createTestConfigs) instead of the
literal string. Locate the test uses around cfg, runtime := createTestConfigs(t)
and replace occurrences of the hardcoded base URL with runtime.AppURL (e.g.,
build expected strings with runtime.AppURL + path or fmt.Sprintf("%s%s",
runtime.AppURL, "/login") ) so the expected redirects stay in sync with the test
config.
In `@internal/controller/user_controller_test.go`:
- Around line 29-30: The tests in user_controller_test.go build a runtime via
createTestConfigs(t) but still assert cookie domain strings using the hardcoded
"example.com"; update those assertions to use runtime.CookieDomain instead so
expectations follow the test runtime configuration. Locate assertions in the
test table that compare or expect cookie domains (e.g., header/cookie checks)
and replace the literal "example.com" with runtime.CookieDomain, ensuring any
related helper calls or expected values reference runtime.CookieDomain
consistently.
In `@internal/model/context_test.go`:
- Around line 234-242: The test case currently compares error strings; change it
to use errors.Is (or require.ErrorIs) so sentinel comparisons are wrap-safe:
inside the table case for the NewFromGin scenario (or in a dedicated t.Run),
have the run closure call c.NewFromGin(newGinCtx(nil, false)), check err with
errors.Is(err, model.ErrUserContextNotFound) (or use require.ErrorIs) and return
a bool (or make the test use require.ErrorIs directly) instead of returning
err.Error(); reference model.UserContext.NewFromGin and
model.ErrUserContextNotFound when updating the test.
In `@internal/service/access_controls_service.go`:
- Around line 14-65: The AccessControlsService currently stores labelProvider as
a pointer-to-interface (*LabelProvider); change the field type in
AccessControlsService to LabelProvider (remove the *), update
NewAccessControlsService signature to accept labelProvider LabelProvider, and
update all uses: in GetAccessControls call labelProvider.GetLabels(domain)
(remove the (*...).GetLabels dereference) and keep the nil check as before (if
acls.labelProvider != nil). Also update the bootstrap/constructor call site
where NewAccessControlsService is invoked to pass labelProvider (not
&labelProvider).
In `@internal/service/ldap_service.go`:
- Around line 253-278: The reconnect function currently calls backoff.Retry with
context.TODO(), which prevents the retry loop from honoring shutdown; update the
call in LdapService.reconnect to pass the service context (ldap.context) instead
of context.TODO() so retries are cancelled when the service context is done;
locate the backoff.Retry invocation in reconnect and replace the context
argument with ldap.context (no other behavioral changes required).
In `@internal/utils/logger/logger.go`:
- Around line 123-155: The audit methods AuditLoginSuccess, AuditLoginFailure
and AuditLogout rely on CallerSkipFrame(1) assuming they are called directly
from controller handlers; add a short inline comment/TODO above these methods
documenting this assumption and that the CallerSkipFrame value must be increased
if audit logging is moved into middleware/wrappers or helper functions, so
future refactors adjust CallerSkipFrame accordingly (or make the skip
configurable at that time).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24420aa6-3528-4ecd-8954-61373dfb1aab
📒 Files selected for processing (51)
cmd/tinyauth/create_user.gocmd/tinyauth/generate_totp.gocmd/tinyauth/healthcheck.gocmd/tinyauth/tinyauth.gocmd/tinyauth/verify_user.gointernal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/context_controller.gointernal/controller/context_controller_test.gointernal/controller/controller_test.gointernal/controller/health_controller.gointernal/controller/health_controller_test.gointernal/controller/oauth_controller.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller.gointernal/controller/proxy_controller_test.gointernal/controller/resources_controller.gointernal/controller/resources_controller_test.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/middleware/context_middleware_test.gointernal/middleware/middleware_test.gointernal/middleware/ui_middleware.gointernal/middleware/zerolog_middleware.gointernal/model/config.gointernal/model/context.gointernal/model/context_test.gointernal/model/runtime.gointernal/service/access_controls_service.gointernal/service/auth_service.gointernal/service/docker_service.gointernal/service/kubernetes_service.gointernal/service/kubernetes_service_test.gointernal/service/ldap_service.gointernal/service/oauth_broker_service.gointernal/service/oauth_presets.gointernal/service/oauth_service.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/utils/app_utils.gointernal/utils/logger/logger.gointernal/utils/logger/logger_test.gointernal/utils/tlog/log_audit.gointernal/utils/tlog/log_wrapper.gointernal/utils/tlog/log_wrapper_test.go
💤 Files with no reviewable changes (5)
- internal/utils/tlog/log_wrapper_test.go
- internal/utils/app_utils.go
- internal/utils/tlog/log_audit.go
- cmd/tinyauth/tinyauth.go
- internal/utils/tlog/log_wrapper.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/oidc_controller.go (1)
146-150: ⚡ Quick winPass a proper error instead of nil to
authorizeError.At line 149,
erris from theBindJSONcall at line 140, which succeeded (otherwise the function would have returned at line 143). Passingnilor an unrelated error toauthorizeErrorreduces log quality.🔧 Suggested fix
client, ok := controller.oidc.GetClient(req.ClientID) if !ok { - controller.authorizeError(c, err, "Client not found", "The client ID is invalid", "", "", "") + controller.authorizeError(c, errors.New("client_not_found"), "Client not found", "The client ID is invalid", "", "", "") return }🤖 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 `@internal/controller/oidc_controller.go` around lines 146 - 150, When GetClient returns ok == false, don't pass the unrelated/previous err to authorizeError; create and pass a new descriptive error (e.g., errors.New("client not found") or fmt.Errorf with the req.ClientID) so logs show the real cause. Update the call in the block after controller.oidc.GetClient(req.ClientID) to construct a proper error value and pass it into controller.authorizeError along with the existing message parameters to improve diagnostics.
🤖 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 `@internal/test/test.go`:
- Around line 36-37: Replace uses of path.Join with filepath.Join for filesystem
paths created from t.TempDir(); specifically update the assignments to
PrivateKeyPath and PublicKeyPath (and the other occurrences noted) so they use
filepath.Join(tempDir, "...") instead of path.Join to ensure correct path
separators on Windows and other platforms; import "path/filepath" if not already
present and remove or keep "path" only if still needed for non-filesystem joins.
- Around line 95-102: The OIDCClients slice is built by ranging over
config.OIDC.Clients (a map) which is non-deterministic; change the
implementation in the OIDCClients anonymous constructor to collect and sort the
map keys (e.g., into a []string), then iterate the sorted keys to set client.ID
and append model.OIDCClientConfig entries so the resulting slice order is
stable; update the code that references OIDCClients to rely on this
deterministic ordering if needed.
---
Nitpick comments:
In `@internal/controller/oidc_controller.go`:
- Around line 146-150: When GetClient returns ok == false, don't pass the
unrelated/previous err to authorizeError; create and pass a new descriptive
error (e.g., errors.New("client not found") or fmt.Errorf with the req.ClientID)
so logs show the real cause. Update the call in the block after
controller.oidc.GetClient(req.ClientID) to construct a proper error value and
pass it into controller.authorizeError along with the existing message
parameters to improve diagnostics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ae39a4-d99c-4ed7-b665-543530ef2213
📒 Files selected for processing (16)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/db_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/controller/context_controller_test.gointernal/controller/oauth_controller.gointernal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/resources_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware_test.gointernal/service/oidc_service.gointernal/test/test.gointernal/utils/logger/logger.gointernal/utils/logger/logger_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/controller/resources_controller_test.go
- internal/utils/logger/logger_test.go
- internal/controller/well_known_controller_test.go
- internal/bootstrap/db_bootstrap.go
- internal/bootstrap/router_bootstrap.go
- internal/controller/proxy_controller_test.go
- internal/controller/oauth_controller.go
- internal/controller/context_controller_test.go
- internal/middleware/context_middleware_test.go
- internal/controller/oidc_controller_test.go
- internal/utils/logger/logger.go
- internal/controller/user_controller_test.go
- internal/service/oidc_service.go
- internal/bootstrap/app_bootstrap.go
Rycochet
previously approved these changes
May 9, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/test/test.go (1)
95-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
OIDCClientsconstruction deterministic (Line 97).Ranging over a map produces non-deterministic order, so this can introduce test flakiness when
runtime.OIDCClientsis compared or serialized. Build the slice from sorted keys.Suggested patch
import ( + "sort" "path/filepath" "testing" @@ OIDCClients: func() []model.OIDCClientConfig { - var clients []model.OIDCClientConfig - for id, client := range config.OIDC.Clients { + keys := make([]string, 0, len(config.OIDC.Clients)) + for id := range config.OIDC.Clients { + keys = append(keys, id) + } + sort.Strings(keys) + + clients := make([]model.OIDCClientConfig, 0, len(keys)) + for _, id := range keys { + client := config.OIDC.Clients[id] client.ID = id clients = append(clients, client) } return clients }(),🤖 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 `@internal/test/test.go` around lines 95 - 102, The OIDCClients slice is built by ranging over the map config.OIDC.Clients which yields non-deterministic order; to fix, collect the map keys into a []string, sort them (e.g. sort.Strings), then iterate the sorted keys to populate client.ID and append to the []model.OIDCClientConfig so OIDCClients is produced deterministically (references: OIDCClients, config.OIDC.Clients, model.OIDCClientConfig).
🤖 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.
Duplicate comments:
In `@internal/test/test.go`:
- Around line 95-102: The OIDCClients slice is built by ranging over the map
config.OIDC.Clients which yields non-deterministic order; to fix, collect the
map keys into a []string, sort them (e.g. sort.Strings), then iterate the sorted
keys to populate client.ID and append to the []model.OIDCClientConfig so
OIDCClients is produced deterministically (references: OIDCClients,
config.OIDC.Clients, model.OIDCClientConfig).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df1dd065-ebc7-4c6e-8a8b-6ec487bd60bb
📒 Files selected for processing (1)
internal/test/test.go
scottmckendry
previously approved these changes
May 9, 2026
Member
scottmckendry
left a comment
scottmckendry
left a comment
There was a problem hiding this comment.
tiny work - couple minor things
Rycochet
approved these changes
May 10, 2026
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.
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Refactor