feat: add mandatory JWT + API key authentication#287
Conversation
Implement enterprise-grade authentication replacing the insecure X-Human-Role header with real credential verification: - JWT authentication with Argon2id password hashing - API key authentication via SHA-256 hash lookup - First-run admin setup flow (POST /auth/setup) - Login, password change, and /me endpoints - Auth middleware (AbstractAuthenticationMiddleware) with exclude paths - Auto-generated JWT secret persisted to settings table - User and API key repositories (protocol + SQLite implementation) - Schema migration v4 (users, api_keys, settings tables) - Guards now read from authenticated connection.user - Approval controller reads decided_by from auth user role - Comprehensive test coverage (config, service, middleware, controller, repos) Closes #256
Resolve conflicts between audit repository (main) and auth feature (this branch) by keeping both: v4 migration for audit_entries, v5 migration for settings/users/api_keys. Bump SCHEMA_VERSION to 5. Apply 25 findings from 10 review agents: add password length model validators, pass through exception detail messages, add test coverage for auth guards/secret resolution/state management, update docs.
Dependency ReviewThe following issues were found:
License Issuespyproject.toml
OpenSSF Scorecard
Scanned Files
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a complete JWT + API-key authentication subsystem: config, models, service, secret resolution, middleware, controller (/auth), persistence (users, api_keys, settings) with SQLite migrations (v5), AppState and startup wiring, guards/exception updates, observability events, dependency additions, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as ApiAuthMiddleware
participant S as AuthService
participant R as Repository
participant DB as SQLite
rect rgba(220,220,255,0.5)
C->>M: HTTP request w/ Authorization: Bearer <token_or_key>
M->>M: extract bearer token
alt token contains "."
M->>S: decode_token(token)
S-->>M: claims
M->>R: users.get(claims.sub)
R->>DB: SELECT user
DB-->>R: user row
R-->>M: User
M->>M: attach AuthenticatedUser to scope
else API key
M->>S: hash_api_key(raw_key)
S-->>M: key_hash
M->>R: api_keys.get_by_hash(key_hash)
R->>DB: SELECT api_key
DB-->>R: api_key row
R-->>M: ApiKey
M->>R: users.get(api_key.user_id)
R->>DB: SELECT user
DB-->>R: user row
R-->>M: User
M->>M: attach AuthenticatedUser to scope
end
M->>C: forward to handler or return 401 on failure
end
sequenceDiagram
participant C as Client
participant Ctrl as AuthController
participant S as AuthService
participant R as UserRepo
participant DB as SQLite
C->>Ctrl: POST /api/v1/auth/setup {username,password}
Ctrl->>R: count()
R->>DB: SELECT COUNT(*) FROM users
DB-->>R: 0
R-->>Ctrl: 0
Ctrl->>S: hash_password(password)
S-->>Ctrl: password_hash
Ctrl->>R: save(new User)
R->>DB: INSERT/UPDATE user
DB-->>R: OK
R-->>Ctrl: saved
Ctrl->>S: create_token(user)
S-->>Ctrl: token, expiry
Ctrl-->>C: 200 {token, expiry}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and mandatory authentication system for the API, integrating both JWT bearer tokens and SHA-256 hashed API keys. It provides a complete suite of authentication functionalities, including user setup, login, password management, and secure access control. The changes significantly enhance the application's security by ensuring all API interactions are authenticated and authorized, while also updating the underlying persistence layer and documentation to support these new features. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR introduces mandatory JWT + API key authentication across the entire API surface, including Argon2id password hashing, an auth middleware, an auth controller, SQLite migrations for However, there is one blocking issue introduced in this PR:
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant AuthMiddleware
participant Controller
participant AuthService
participant DB
Note over Client,DB: JWT Login Flow
Client->>Controller: POST /auth/login {username, password}
Controller->>DB: get_by_username(username)
DB-->>Controller: User | None
alt user found
Controller->>AuthService: verify_password_async(password, hash)
else user not found
Controller->>AuthService: verify_password_async(password, DUMMY_HASH)
end
AuthService-->>Controller: bool
Controller->>AuthService: create_token(user) → (jwt, expires_in)
Controller-->>Client: {token, expires_in, must_change_password}
Note over Client,DB: Authenticated Request Flow
Client->>AuthMiddleware: GET /api/... Authorization: Bearer <token>
alt token contains "."
AuthMiddleware->>AuthService: decode_token(token) → claims
AuthMiddleware->>DB: users.get(claims["sub"])
DB-->>AuthMiddleware: User
AuthMiddleware->>AuthMiddleware: validate pwd_sig
else no "." in token
AuthMiddleware->>AuthService: hash_api_key(token)
AuthMiddleware->>DB: api_keys.get_by_hash(hash)
DB-->>AuthMiddleware: ApiKey
AuthMiddleware->>DB: users.get(api_key.user_id)
DB-->>AuthMiddleware: User
end
AuthMiddleware->>AuthMiddleware: set scope["user"] = AuthenticatedUser
AuthMiddleware->>Controller: forward request
Controller-->>Client: response
|
There was a problem hiding this comment.
Pull request overview
Adds mandatory API authentication to the Litestar REST layer, replacing the prior self-asserted role header mechanism with JWT + API key auth backed by SQLite persistence and wired into middleware, guards, and controllers.
Changes:
- Introduces
api/auth/subsystem (config, service, middleware, controller, models, secret resolution) and integrates it into app startup and middleware stack. - Extends persistence layer with schema v5 (
settings,users,api_keys), new repository protocols, and SQLite implementations. - Updates guards, exception handling, and unit tests/docs to reflect authenticated access and new endpoints.
Reviewed changes
Copilot reviewed 49 out of 51 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds locked deps for argon2-cffi and pyjwt. |
| pyproject.toml | Declares runtime deps for JWT + Argon2 password hashing. |
| docker/.env.example | Documents AI_COMPANY_JWT_SECRET usage and setup flow. |
| README.md | Updates feature list to include JWT + API key auth. |
| DESIGN_SPEC.md | Documents new /auth endpoints and persistence/auth components. |
| CLAUDE.md | Updates package description to mention auth subsystem. |
| tests/unit/persistence/test_protocol.py | Extends protocol compliance coverage for new repos/settings methods. |
| tests/unit/persistence/test_migrations_v2.py | Updates schema version assertions to v5. |
| tests/unit/persistence/sqlite/test_user_repo.py | Adds SQLite user/api-key repository CRUD tests. |
| tests/unit/persistence/sqlite/test_migrations.py | Adds tests for v5 tables and indexes. |
| tests/unit/config/conftest.py | Includes ApiConfig in root config factory. |
| tests/unit/api/test_state.py | Adds coverage for AppState.auth_service accessor and setter behavior. |
| tests/unit/api/test_guards.py | Refactors guard tests to use JWT auth headers and adds 401 cases. |
| tests/unit/api/test_exception_handlers.py | Adds 401/422 handler tests and updates 4xx message assertions. |
| tests/unit/api/test_app.py | Updates lifecycle test for new _safe_startup() signature and auth init. |
| tests/unit/api/controllers/test_tasks.py | Updates controller tests to use JWT auth headers. |
| tests/unit/api/controllers/test_company.py | Updates headers and expects 401 on invalid auth. |
| tests/unit/api/controllers/test_budget.py | Updates headers and expects 401 on invalid auth. |
| tests/unit/api/controllers/test_autonomy.py | Updates headers and expects 401 on invalid auth. |
| tests/unit/api/controllers/test_approvals.py | Updates decided_by source expectations and auth behavior (401 vs 403). |
| tests/unit/api/controllers/test_analytics.py | Updates headers and expects 401 on invalid auth. |
| tests/unit/api/controllers/test_agents.py | Injects auth service and seeds users for JWT-based access. |
| tests/unit/api/conftest.py | Adds fake user/api-key repos, auth helpers, and default JWT headers for clients. |
| tests/unit/api/auth/test_service.py | Adds unit tests for hashing, JWT creation/validation, API key hashing. |
| tests/unit/api/auth/test_secret.py | Adds unit tests for JWT secret resolution chain behavior. |
| tests/unit/api/auth/test_middleware.py | Adds middleware tests for JWT/API-key auth and excluded paths. |
| tests/unit/api/auth/test_controller.py | Adds endpoint tests for setup/login/change-password/me + password guard. |
| tests/unit/api/auth/test_config.py | Adds AuthConfig validation/immutability tests. |
| tests/unit/api/auth/init.py | Package marker for auth unit tests. |
| src/ai_company/persistence/sqlite/user_repo.py | Implements SQLite repositories for User and ApiKey. |
| src/ai_company/persistence/sqlite/migrations.py | Bumps schema to v5 and adds settings/users/api_keys migration. |
| src/ai_company/persistence/sqlite/backend.py | Wires user/api-key repos and adds get/set settings operations. |
| src/ai_company/persistence/repositories.py | Adds UserRepository / ApiKeyRepository protocols. |
| src/ai_company/persistence/protocol.py | Extends PersistenceBackend protocol with auth repos + settings methods. |
| src/ai_company/observability/events/persistence.py | Adds persistence event constants for users/api-keys/settings. |
| src/ai_company/observability/events/api.py | Adds auth-related API event constants. |
| src/ai_company/api/state.py | Adds auth_service accessor and set_auth_service() for deferred init. |
| src/ai_company/api/guards.py | Switches role extraction to connection.user rather than headers. |
| src/ai_company/api/exception_handlers.py | Adds 401 handler for NotAuthorizedException; adjusts 4xx messaging. |
| src/ai_company/api/errors.py | Adds UnauthorizedError (401). |
| src/ai_company/api/controllers/approvals.py | Populates decided_by from authenticated user role. |
| src/ai_company/api/controllers/init.py | Registers AuthController in exported controller set. |
| src/ai_company/api/config.py | Adds auth: AuthConfig to API config and updates CORS headers list. |
| src/ai_company/api/auth/service.py | Implements Argon2id hashing, JWT encode/decode, API key hashing/gen. |
| src/ai_company/api/auth/secret.py | Implements env→DB→autogen JWT secret resolution. |
| src/ai_company/api/auth/models.py | Adds User, ApiKey, and AuthenticatedUser domain models. |
| src/ai_company/api/auth/middleware.py | Implements JWT-first / API-key fallback authentication middleware. |
| src/ai_company/api/auth/controller.py | Implements /auth/setup, /auth/login, /auth/change-password, /auth/me and password-change guard. |
| src/ai_company/api/auth/config.py | Adds immutable AuthConfig with secret validation and exclude patterns. |
| src/ai_company/api/auth/init.py | Exposes auth public API symbols. |
| src/ai_company/api/app.py | Integrates auth middleware + startup secret resolution into lifecycle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if persistence is not None: | ||
| try: | ||
| await persistence.connect() | ||
| except Exception: | ||
| logger.error( | ||
| logger.exception( |
There was a problem hiding this comment.
_safe_startup() connects the persistence backend but does not run persistence.migrate() before calling resolve_jwt_secret(). With the new settings/users/api_keys tables added in schema v5, a freshly connected SQLite backend that hasn’t been migrated yet will fail when get_setting() queries the settings table. Run migrations right after connect() (and before resolving the JWT secret) to guarantee required tables exist.
src/ai_company/api/auth/secret.py
Outdated
| if stored: | ||
| logger.info( | ||
| API_APP_STARTUP, | ||
| note="JWT secret loaded from persistence", | ||
| ) | ||
| return stored |
There was a problem hiding this comment.
resolve_jwt_secret() returns the persisted secret without stripping whitespace or validating the minimum length. If the stored value is accidentally empty/whitespace or shorter than 32 chars, startup will later fail (or worse, run with a weak secret if validation changes). Consider .strip() + enforcing _MIN_SECRET_LENGTH for the stored secret too; if invalid, regenerate and overwrite (or raise a clear error).
| if stored: | |
| logger.info( | |
| API_APP_STARTUP, | |
| note="JWT secret loaded from persistence", | |
| ) | |
| return stored | |
| if stored is not None: | |
| stored = stored.strip() | |
| if stored: | |
| if len(stored) < _MIN_SECRET_LENGTH: | |
| msg = ( | |
| f"Persisted JWT secret under key '{_SETTING_KEY}' must be at least " | |
| f"{_MIN_SECRET_LENGTH} characters (got {len(stored)}); regenerating." | |
| ) | |
| logger.warning(API_APP_STARTUP, warning=msg) | |
| else: | |
| logger.info( | |
| API_APP_STARTUP, | |
| note="JWT secret loaded from persistence", | |
| ) | |
| return stored |
src/ai_company/api/auth/config.py
Outdated
| ), | ||
| description=( | ||
| "Regex patterns for paths excluded from authentication. " | ||
| "Anchor with ^ and $ to avoid substring matches." |
There was a problem hiding this comment.
The exclude_paths field description says to “Anchor with ^ and $ to avoid substring matches”, but the default includes "^/docs" (no $) which intentionally matches a prefix. Either update the default pattern(s) to be fully anchored, or adjust the description to reflect that prefix regexes are acceptable/expected for docs paths.
| "Anchor with ^ and $ to avoid substring matches." | |
| "Use ^ to anchor at the start of the path and add $ when an exact " | |
| "match (rather than a prefix match) is required." |
| error=str(exc), | ||
| ) | ||
| raise QueryError(msg) from exc | ||
| keys = tuple(_row_to_api_key(row) for row in rows) |
There was a problem hiding this comment.
list_by_user() deserializes rows via tuple(_row_to_api_key(row) for row in rows) without catching ValueError/pydantic.ValidationError. Other methods (get, get_by_hash, and SQLiteUserRepository.list_users) wrap deserialization failures as QueryError and log appropriately. Add consistent error handling here so corrupt rows don’t raise unhandled exceptions.
| keys = tuple(_row_to_api_key(row) for row in rows) | |
| try: | |
| keys = tuple(_row_to_api_key(row) for row in rows) | |
| except (ValueError, ValidationError) as exc: | |
| msg = f"Failed to deserialize API keys for user {user_id!r}" | |
| logger.exception( | |
| PERSISTENCE_API_KEY_LIST_FAILED, | |
| user_id=user_id, | |
| error=str(exc), | |
| ) | |
| raise QueryError(msg) from exc |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed authentication system using JWT and API keys, covering password hashing with Argon2, JWT creation and validation, and API key management. While the implementation significantly improves the API's security posture, two medium-severity issues were identified: the require_password_changed guard is implemented but not enforced, and a race condition in the initial setup endpoint could allow the creation of multiple administrative accounts. Addressing these will ensure the intended security policies are fully effective. Further feedback includes suggestions to improve configurability for security parameters, remove redundant code, and correct some type hints for better clarity and correctness.
| from typing import Self | ||
|
|
||
| from litestar import Controller, Response, get, post | ||
| from litestar.connection import ASGIConnection # noqa: TC002 |
There was a problem hiding this comment.
In Litestar controller methods, the request object is of type litestar.Request, not ASGIConnection. ASGIConnection is a more generic type used in middleware and guards. You should import Request from litestar and use it to type the request parameter in your controller methods (setup, login, change_password, me). This will also allow you to remove the # type: ignore[type-arg] comments and improve type safety.
| def require_password_changed( | ||
| connection: ASGIConnection, # type: ignore[type-arg] | ||
| _: object, | ||
| ) -> None: | ||
| """Guard that blocks users who must change their password. | ||
|
|
||
| Applied to all routes except ``/auth/change-password`` and | ||
| ``/auth/me``. | ||
|
|
||
| Args: | ||
| connection: The incoming connection. | ||
| _: Route handler (unused). | ||
|
|
||
| Raises: | ||
| PermissionDeniedException: If password change is required. | ||
| """ | ||
| user = connection.scope.get("user") | ||
| if not isinstance(user, AuthenticatedUser): | ||
| return | ||
| if user.must_change_password: | ||
| raise PermissionDeniedException(detail="Password change required") | ||
|
|
There was a problem hiding this comment.
The require_password_changed guard is implemented to enforce the security policy that users must change their initial password before accessing other parts of the API. However, this guard is not registered globally in the application or applied to any of the protected controllers or routers in the provided code. Consequently, the must_change_password flag (set to True for the initial admin during setup) is never enforced, allowing users to bypass the mandatory password change requirement.
To remediate this, apply the require_password_changed guard to the application or specific routers. For example, in src/ai_company/api/app.py, you could add it to the api_router or the Litestar instance.
| user_count = await persistence.users.count() | ||
| if user_count > 0: | ||
| msg = "Setup already completed" | ||
| raise ConflictError(msg) | ||
|
|
||
| now = datetime.now(UTC) | ||
| user = User( | ||
| id=str(uuid.uuid4()), | ||
| username=data.username, | ||
| password_hash=auth_service.hash_password(data.password), | ||
| role=HumanRole.CEO, | ||
| must_change_password=True, | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| await persistence.users.save(user) |
There was a problem hiding this comment.
The /auth/setup endpoint checks if any users exist before creating the first admin (CEO). However, the check (user_count > 0) and the user creation (persistence.users.save(user)) are not performed atomically within a single database transaction. If multiple requests are sent to the setup endpoint simultaneously on a fresh installation, this race condition could allow multiple users to be created, all with the CEO role.
To remediate this, wrap the setup logic in a database transaction or use an atomic SQL operation that ensures the user is only created if the table is empty.
src/ai_company/api/app.py
Outdated
| except RuntimeError: | ||
| pass # Already configured (e.g. test-injected) |
There was a problem hiding this comment.
Swallowing a generic RuntimeError can hide other potential issues. A more explicit approach would be to check if the auth service is already configured before trying to set it. For example, you could add a method like is_auth_service_configured() to AppState and use it here. Alternatively, set_auth_service could be made idempotent by logging a warning if called more than once, instead of raising an error that is then caught and ignored.
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| _MIN_PASSWORD_LENGTH = 12 |
There was a problem hiding this comment.
The minimum password length is hardcoded. It would be more flexible to make this configurable, for example, by adding min_password_length to AuthConfig. This would allow for different security policies across deployments.
src/ai_company/api/auth/secret.py
Outdated
|
|
||
| _SETTING_KEY = "jwt_secret" | ||
| _SECRET_LENGTH = 48 # 64 URL-safe base64 chars | ||
| _MIN_SECRET_LENGTH = 32 |
There was a problem hiding this comment.
The constant _MIN_SECRET_LENGTH is also defined in src/ai_company/api/auth/config.py. To avoid duplication and ensure consistency, you could define it in one place (e.g., config.py) and import it here. This improves maintainability.
| _hasher = argon2.PasswordHasher( | ||
| time_cost=3, | ||
| memory_cost=65536, | ||
| parallelism=4, | ||
| hash_len=32, | ||
| salt_len=16, | ||
| ) |
There was a problem hiding this comment.
The Argon2 parameters are hardcoded. For better security and flexibility, consider making these parameters (time_cost, memory_cost, parallelism) configurable, for example, by adding them to AuthConfig. This would allow tuning the password hashing strength based on deployment requirements and hardware.
| auth_user = request.scope.get("user") | ||
| role = auth_user.role.value if auth_user is not None else "unknown" |
There was a problem hiding this comment.
The require_write_access guard on this controller ensures that request.scope['user'] will always be an AuthenticatedUser instance. Therefore, the if auth_user is not None check is redundant, and the else 'unknown' branch is unreachable. You can simplify this to directly access the user's role, which makes the code cleaner and relies on the guarantees provided by the auth middleware and guards.
auth_user = request.scope["user"]
role = auth_user.role.value| auth_user = request.scope.get("user") | ||
| role = auth_user.role.value if auth_user is not None else "unknown" |
There was a problem hiding this comment.
The require_write_access guard on this controller ensures that request.scope['user'] will always be an AuthenticatedUser instance. Therefore, the if auth_user is not None check is redundant, and the else 'unknown' branch is unreachable. You can simplify this to directly access the user's role, which makes the code cleaner and relies on the guarantees provided by the auth middleware and guards.
auth_user = request.scope["user"]
role = auth_user.role.valueThere was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/api/test_guards.py (1)
25-120: 🧹 Nitpick | 🔵 TrivialCollapse the role/status matrix with
@pytest.mark.parametrize.These cases only vary by role and expected status. Parametrizing them would keep the request payload in one place and make the guard matrix much easier to extend without drift.
As per coding guidelines, "Prefer
@pytest.mark.parametrizefor testing similar cases".Also applies to: 125-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/test_guards.py` around lines 25 - 120, Replace the many near-duplicate test functions (e.g., test_allows_ceo, test_allows_manager, test_allows_board_member, test_allows_pair_programmer, test_blocks_observer, test_missing_auth_returns_401, test_invalid_token_returns_401) with a single parametrized test using pytest.mark.parametrize: keep one shared JSON payload and parametrize over (role_or_headers, expected_status) pairs for the role-based cases (use make_auth_headers("role") for roles and None for missing auth, and a literal headers dict for the invalid token case), then inside the single test call client.post("/api/v1/tasks", json=payload, headers=headers_if_any) and assert response.status_code == expected_status; name the new test something like test_task_create_auth_matrix to replace the individual functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DESIGN_SPEC.md`:
- Line 2762: The design enforces mandatory JWT/API-key auth which harms
local-first UX; add a guarded bypass so local single-user runs can skip auth:
introduce a config flag (e.g., AUTH_REQUIRED default true) and an explicit dev
override (e.g., CLI --no-auth or env AI_COMPANY_DEV_MODE=true) and amend the
central auth path (the auth middleware / function used for all requests, e.g.,
authMiddleware or validateAuth) to short-circuit authentication when
AUTH_REQUIRED is false OR when server bind address is localhost/127.0.0.1
(detect via the server.bindAddress or incoming Host header) and log a strong
security warning; keep auth enforced for any non-local bind or when the override
is not set.
In `@src/ai_company/api/app.py`:
- Around line 177-185: The startup currently always calls
resolve_jwt_secret(...) and then app_state.set_auth_service(AuthService(...))
which can generate/persist a secret even when an external auth_service was
injected; change the flow to first check whether an auth service is already
present (e.g. inspect app_state.auth_service or the parameter passed into
create_app) and skip resolve_jwt_secret and set_auth_service when one exists,
and remove the broad except RuntimeError: pass; instead handle
duplicate-configuration explicitly (either by returning early when auth is
supplied or by catching a specific error and logging/re-raising it) so
persistence isn't altered silently by resolve_jwt_secret, and reference
resolve_jwt_secret, app_state.config.api.auth.with_secret,
app_state.set_auth_service, and AuthService when making the change.
In `@src/ai_company/api/auth/config.py`:
- Around line 65-72: The exclude_paths default in AuthConfig is hardcoded to
"/api/v1" routes; remove those hardcoded defaults in AuthConfig (e.g., set Field
default to an empty tuple) and instead derive the auth-exempt regexes at app
wiring time using ApiConfig.api_prefix from ApiConfig (ApiConfig.api_prefix) —
compute patterns for health, docs, base api and auth routes using the configured
prefix and inject/assign that tuple to AuthConfig.exclude_paths when you
instantiate AuthConfig in the app factory/bootstrap so the exclusion list
follows the configured api_prefix.
In `@src/ai_company/api/auth/controller.py`:
- Around line 282-296: In change_password, avoid direct indexing into
request.scope["user"] which can raise KeyError; instead retrieve the user
defensively (e.g., request.scope.get("user")) and if missing or not an
AuthenticatedUser, raise UnauthorizedError with a clear message before
proceeding to call persistence.users.get; update any related assumptions in the
function (auth_user variable usage) so the function fails gracefully when auth
middleware is absent or misconfigured.
- Around line 146-167: The guard require_password_changed currently returns
silently when connection.scope.get("user") is missing or not an instance of
AuthenticatedUser; update it to emit a DEBUG-level log in that early-return path
so missing middleware/configuration is visible during debugging. Specifically,
inside require_password_changed, after fetching user from connection.scope and
before returning when not isinstance(user, AuthenticatedUser), call the module
logger (or create one if missing) to log a concise debug message that includes
that the user scope was not populated and include any identifying info available
from connection.scope (e.g., path or headers) to help trace the
misconfiguration; keep the existing behavior otherwise. Ensure the change
references require_password_changed, AuthenticatedUser, and
connection.scope.get("user").
- Around line 338-354: The me handler accesses request.scope["user"] directly
which can raise KeyError if auth middleware didn't set it; update the me method
to safely retrieve the user (e.g., user = request.scope.get("user")) and
validate its presence/type (ensure it's an AuthenticatedUser) before using its
attributes; if missing or invalid, return an appropriate unauthorized/400
ApiResponse (or raise the existing authentication exception used elsewhere) so
me never assumes request.scope["user"] is present.
In `@src/ai_company/api/auth/models.py`:
- Around line 39-40: Change the timestamp fields created_at and updated_at in
the auth model(s) to use Pydantic's AwareDatetime (e.g., import AwareDatetime
from pydantic or pydantic.types and replace the datetime annotations with
AwareDatetime for created_at and updated_at); ensure the import is added at the
top of src/ai_company/api/auth/models.py and verify other timestamp fields in
the same model use AwareDatetime so comparisons like api_key.expires_at in the
auth middleware (line ~195) are timezone-aware.
In `@src/ai_company/api/auth/secret.py`:
- Around line 48-55: The stored JWT secret loaded via
persistence.get_setting(_SETTING_KEY) is returned without length validation;
update the logic in the function that reads _SETTING_KEY (reference: stored
variable and persistence.get_setting) to validate that stored meets the minimum
length (e.g., >= 32 chars) before returning it; if validation fails, log a
warning via logger (include context "JWT secret from persistence too short") and
treat it as missing so the code continues to generate/derive a new compliant
secret instead of returning the short value.
In `@src/ai_company/api/controllers/approvals.py`:
- Around line 239-240: The code currently writes the caller's role string into
the approval audit field `decided_by`; instead persist the authenticated
principal (e.g., `AuthenticatedUser.user_id` or `AuthenticatedUser.username`)
instead of `role`, and raise/return an error immediately if no authenticated
user is present. Locate the places that set `decided_by` (the approval
handlers/creators referenced around the `decided_by` assignments in this file
and the similar occurrences at the other spots you noted) and replace uses of
the role string with the stable principal identifier from the
`AuthenticatedUser` object, adding a guard that fails fast when
`AuthenticatedUser` is missing (do not fall back to `"unknown"`).
In `@src/ai_company/api/state.py`:
- Around line 101-115: In set_auth_service, before raising RuntimeError on a
second call, add a structured WARNING/ERROR log that includes context (e.g., the
attempted service object or its identifier and the current _auth_service state)
so the double-initialization path is observable; use the module logger (get or
import a logger for this module) to log the message immediately before raising
the RuntimeError in set_auth_service.
In `@src/ai_company/observability/events/persistence.py`:
- Around line 147-148: Add matching success event constants for settings to
mirror the existing failure-only entries: define PERSISTENCE_SETTING_SAVED and
PERSISTENCE_SETTING_FETCHED (similar naming style to
PERSISTENCE_SETTING_SAVE_FAILED and PERSISTENCE_SETTING_FETCH_FAILED) in the
same block so the file has both success and failure pairs for settings; ensure
the string values follow the same pattern ("persistence.setting.saved" and
"persistence.setting.fetched") and export them as Final[str] like the other
event constants.
In `@src/ai_company/persistence/protocol.py`:
- Around line 139-164: Change the public persistence protocol signatures to
require NotBlankStr for identifier-like keys: update the async def
get_setting(self, key: str) -> str | None and async def set_setting(self, key:
str, value: str) -> None to use key: NotBlankStr from core.types, update the
docstrings to reflect non-blank requirement, add the necessary import of
NotBlankStr, and ensure downstream implementations of get_setting and
set_setting accept NotBlankStr (or coerce/validate at the boundary) so all
backends adhere to the tightened contract.
In `@src/ai_company/persistence/sqlite/migrations.py`:
- Around line 200-223: The migration defines UNIQUE constraints inline on
users.username and api_keys.key_hash and then creates duplicate unique indexes
idx_users_username and idx_api_keys_hash; remove the redundant "CREATE UNIQUE
INDEX IF NOT EXISTS idx_users_username ON users(username)" and "CREATE UNIQUE
INDEX IF NOT EXISTS idx_api_keys_hash ON api_keys(key_hash)" entries in
migrations.py so only the implicit unique indexes from "username TEXT NOT NULL
UNIQUE" and "key_hash TEXT NOT NULL UNIQUE" remain, reducing unnecessary write
overhead while preserving uniqueness.
In `@src/ai_company/persistence/sqlite/user_repo.py`:
- Around line 103-145: The repository layer widens typed identifier/name
parameters from NotBlankStr to plain str, losing boundary validation; update the
method signatures in this file (e.g., async def get, async def get_by_username
and the other indicated methods at ~193-197, 260-316, 336-340) to use
NotBlankStr (and Optional[NotBlankStr] or Tuple[NotBlankStr, ...] where
applicable) instead of plain str, add the import for NotBlankStr from
core.types, and keep internal usage the same (no runtime validation changes) so
callers retain the stricter contract and mypy/typing reflect the NotBlankStr
constraint.
- Around line 312-334: list_by_user currently lets _row_to_api_key errors
(ValueError/ValidationError) escape, unlike get()/get_by_hash(); catch those
conversion errors inside list_by_user, log them with the same
PERSISTENCE_API_KEY_LIST_FAILED context and raise a QueryError wrapping the
original exception; specifically wrap the tuple(_row_to_api_key(row) ...)
construction in a try/except that catches ValueError and
pydantic.ValidationError (or ValidationError used elsewhere), call
logger.exception(PERSISTENCE_API_KEY_LIST_FAILED, user_id=user_id,
error=str(exc)) and raise QueryError(f"Failed to list API keys for user
{user_id!r}") from exc so callers see the same QueryError contract.
In `@tests/unit/api/auth/test_controller.py`:
- Around line 38-61: Convert the test_setup_409_when_users_exist test to an
async test function and remove the use of asyncio.get_event_loop(); instead
await the persistence call directly (await
app_state.persistence.users.save(user)). Keep TestClient usage unchanged (its
.post()/.get() calls remain synchronous). This involves changing the test
signature to async def test_setup_409_when_users_exist(...), removing loop =
asyncio.get_event_loop() and loop.run_until_complete(...), and replacing that
with a direct await of app_state.persistence.users.save(user) so the save
operation runs under the pytest async fixture/runtime.
In `@tests/unit/api/auth/test_service.py`:
- Around line 17-34: Move the inline import in the helper function _make_user:
remove "from datetime import UTC, datetime" from inside _make_user and add the
same import at the module level so _make_user uses UTC and datetime via the
top-level imports (leave _make_service(), User creation, and datetime.now(UTC)
unchanged).
In `@tests/unit/api/conftest.py`:
- Around line 583-606: The test fixture test_client (and the other similar
fixture invoking the same helper) must stop driving the asyncio event loop
manually; convert the helper _seed_test_users to an async function and make the
fixture async so you can await it (await _seed_test_users(...)) instead of
calling run_until_complete on the global loop, and update the helper
implementation to await backend.users.save(...) (and any other async backend
calls) directly; remove any run_until_complete or loop.run_* calls and ensure
fixtures use async/await flow so pytest asyncio_mode="auto" can manage the loop.
In `@tests/unit/persistence/sqlite/test_migrations.py`:
- Around line 147-187: Collapse the four nearly identical tests into two
parametrized tests: one parameterizing table names and one parameterizing
expected index names. Replace
test_v5_creates_users_table/test_v5_creates_api_keys_table/test_v5_creates_settings_table
with a single async test decorated with `@pytest.mark.parametrize`("table_name",
["users","api_keys","settings"]) that calls run_migrations(memory_db), queries
sqlite_master for a table with name=table_name using memory_db.execute and
asserts the fetched row is not None; similarly replace
test_v5_creates_user_indexes with an `@pytest.mark.parametrize`("expected_index",
["idx_users_username","idx_api_keys_user_id"]) async test that runs
run_migrations(memory_db), selects index names with the same LIKE query used
before, collects names into a set and asserts expected_index in that set. Use
the existing memory_db fixture and run_migrations call names to locate the code
to edit.
In `@tests/unit/persistence/sqlite/test_user_repo.py`:
- Around line 24-26: Enable SQLite foreign keys on the opened aiosqlite
connection before running migrations: after creating the connection (the
variable `conn` in the test setup) call and await execution of `PRAGMA
foreign_keys = ON` (e.g., `await conn.execute("PRAGMA foreign_keys = ON")` and
`await conn.commit()`), then call `run_migrations(conn)` so the
`api_keys.user_id REFERENCES users(id)` constraint is enforced during the tests.
---
Outside diff comments:
In `@tests/unit/api/test_guards.py`:
- Around line 25-120: Replace the many near-duplicate test functions (e.g.,
test_allows_ceo, test_allows_manager, test_allows_board_member,
test_allows_pair_programmer, test_blocks_observer,
test_missing_auth_returns_401, test_invalid_token_returns_401) with a single
parametrized test using pytest.mark.parametrize: keep one shared JSON payload
and parametrize over (role_or_headers, expected_status) pairs for the role-based
cases (use make_auth_headers("role") for roles and None for missing auth, and a
literal headers dict for the invalid token case), then inside the single test
call client.post("/api/v1/tasks", json=payload, headers=headers_if_any) and
assert response.status_code == expected_status; name the new test something like
test_task_create_auth_matrix to replace the individual functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac1a673c-ef5b-4164-a222-24d66d7cfbf9
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
CLAUDE.mdDESIGN_SPEC.mdREADME.mddocker/.env.examplepyproject.tomlsrc/ai_company/api/app.pysrc/ai_company/api/auth/__init__.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/controller.pysrc/ai_company/api/auth/middleware.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/secret.pysrc/ai_company/api/auth/service.pysrc/ai_company/api/config.pysrc/ai_company/api/controllers/__init__.pysrc/ai_company/api/controllers/approvals.pysrc/ai_company/api/errors.pysrc/ai_company/api/exception_handlers.pysrc/ai_company/api/guards.pysrc/ai_company/api/state.pysrc/ai_company/observability/events/api.pysrc/ai_company/observability/events/persistence.pysrc/ai_company/persistence/protocol.pysrc/ai_company/persistence/repositories.pysrc/ai_company/persistence/sqlite/backend.pysrc/ai_company/persistence/sqlite/migrations.pysrc/ai_company/persistence/sqlite/user_repo.pytests/unit/api/auth/__init__.pytests/unit/api/auth/test_config.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_secret.pytests/unit/api/auth/test_service.pytests/unit/api/conftest.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_autonomy.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_tasks.pytests/unit/api/test_app.pytests/unit/api/test_exception_handlers.pytests/unit/api/test_guards.pytests/unit/api/test_state.pytests/unit/config/conftest.pytests/unit/persistence/sqlite/test_migrations.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/persistence/test_migrations_v2.pytests/unit/persistence/test_protocol.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: Test (Python 3.14)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.14+ with native lazy annotations (PEP 649) — do NOT usefrom __future__ import annotations
Use PEP 758 except syntaxexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions and classes must have type hints; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement of non-Pydantic internal collections
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention and usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves — never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 with@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls) — use structured concurrency over barecreate_task
Maintain maximum line length of 88 characters — enforced by ruff
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly; never silently swallow exceptions
Validate input at system boundaries (user input, external APIs, config files)
Files:
tests/unit/config/conftest.pysrc/ai_company/api/state.pysrc/ai_company/api/guards.pytests/unit/api/auth/test_service.pysrc/ai_company/observability/events/api.pysrc/ai_company/persistence/protocol.pysrc/ai_company/api/auth/__init__.pysrc/ai_company/api/controllers/__init__.pysrc/ai_company/api/config.pytests/unit/api/controllers/test_autonomy.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/exception_handlers.pysrc/ai_company/api/auth/secret.pytests/unit/api/test_exception_handlers.pysrc/ai_company/api/errors.pytests/unit/api/auth/test_config.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/sqlite/backend.pytests/unit/persistence/test_protocol.pysrc/ai_company/api/controllers/approvals.pytests/unit/persistence/test_migrations_v2.pytests/unit/api/controllers/test_tasks.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pysrc/ai_company/persistence/sqlite/user_repo.pytests/unit/api/test_app.pysrc/ai_company/api/auth/middleware.pytests/unit/api/test_state.pysrc/ai_company/api/auth/config.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/api/controllers/test_approvals.pysrc/ai_company/api/app.pytests/unit/persistence/sqlite/test_migrations.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_controller.pytests/unit/api/conftest.pysrc/ai_company/persistence/repositories.pytests/unit/api/controllers/test_budget.pytests/unit/api/auth/test_secret.pysrc/ai_company/api/auth/controller.pytests/unit/api/controllers/test_agents.pytests/unit/api/test_guards.pysrc/ai_company/observability/events/persistence.pysrc/ai_company/persistence/sqlite/migrations.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Enforce 80% minimum code coverage — enforced in CI
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set timeout to 30 seconds per test
Usepytest-xdistvia-n autofor parallel test execution
Prefer@pytest.mark.parametrizefor testing similar cases
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned tests — usetest-provider,test-small-001, etc.
Files:
tests/unit/config/conftest.pytests/unit/api/auth/test_service.pytests/unit/api/controllers/test_autonomy.pytests/unit/api/test_exception_handlers.pytests/unit/api/auth/test_config.pytests/unit/persistence/test_protocol.pytests/unit/persistence/test_migrations_v2.pytests/unit/api/controllers/test_tasks.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/test_app.pytests/unit/api/test_state.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/api/controllers/test_approvals.pytests/unit/persistence/sqlite/test_migrations.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_controller.pytests/unit/api/conftest.pytests/unit/api/controllers/test_budget.pytests/unit/api/auth/test_secret.pytests/unit/api/controllers/test_agents.pytests/unit/api/test_guards.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Every module with business logic must importfrom ai_company.observability import get_loggerand definelogger = get_logger(__name__); never useimport logging,logging.getLogger(), orprint()in application code
Always use variable namelogger(not_logger, notlog) for the logging instance
Use domain-specific event name constants fromai_company.observability.events.<domain>(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget) — import directly:from ai_company.observability.events.<domain> import EVENT_CONSTANT
Use structured logging with kwargs formatlogger.info(EVENT, key=value)— never use format strings likelogger.info("msg %s", val)
All error paths must log at WARNING or ERROR level with context before raising an exception
All state transitions must log at INFO level
Use DEBUG logging level for object creation, internal flow, and entry/exit of key functions
Pure data models, enums, and re-exports do NOT require logging
All provider calls go throughBaseCompletionProviderwhich automatically applies retry + rate limiting — never implement retry logic in driver subclasses or calling code
ConfigureRetryConfigandRateLimiterConfigper-provider inProviderConfig
Retryable errors are:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError(marked withis_retryable=True); non-retryable errors raise immediately without retry
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples — use generic names:example-provider,example-large-001,example-medium-001,example-small-001, orlarge/medium/smallaliases. Vendor names may only appear in: (1) DESIGN_SPEC.md provider list, (2).claude/skill/agent files, (3) third-party import paths. Tests must usetest-provider,test-small-001, etc.
Files:
src/ai_company/api/state.pysrc/ai_company/api/guards.pysrc/ai_company/observability/events/api.pysrc/ai_company/persistence/protocol.pysrc/ai_company/api/auth/__init__.pysrc/ai_company/api/controllers/__init__.pysrc/ai_company/api/config.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/exception_handlers.pysrc/ai_company/api/auth/secret.pysrc/ai_company/api/errors.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/sqlite/backend.pysrc/ai_company/api/controllers/approvals.pysrc/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/api/auth/middleware.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/app.pysrc/ai_company/persistence/repositories.pysrc/ai_company/api/auth/controller.pysrc/ai_company/observability/events/persistence.pysrc/ai_company/persistence/sqlite/migrations.py
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: All Python dependency versions use==(pinned) inpyproject.toml
Dependency groups inpyproject.toml:test(pytest + plugins),dev(includes test + ruff, mypy, pre-commit, commitizen). Install withuv sync(installs everything, dev group is default)
Files:
pyproject.toml
🧠 Learnings (7)
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Configure `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
tests/unit/config/conftest.pysrc/ai_company/api/config.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Use domain-specific event name constants from `ai_company.observability.events.<domain>` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`) — import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/ai_company/observability/events/api.pysrc/ai_company/observability/events/persistence.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to **/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (with `model_copy(update=...)`) for runtime state that evolves — never mix static config fields with mutable runtime fields in one model
Applied to files:
src/ai_company/api/auth/models.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Pure data models, enums, and re-exports do NOT require logging
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: If implementation deviates from `DESIGN_SPEC.md` (better approach found, scope evolved, etc.), alert the user and explain why — user decides whether to proceed or update the spec. Do NOT silently diverge — every deviation needs explicit user approval
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: ALWAYS read `DESIGN_SPEC.md` before implementing any feature or planning any issue — the design spec is the starting point for architecture, data models, and behavior
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to docker/** : Backend image uses 3-stage Docker build (builder → setup → distroless runtime), Chainguard Python, non-root user (UID 65532), CIS-hardened. Web uses `nginxinc/nginx-unprivileged` with SPA routing and API/WebSocket proxy to backend
Applied to files:
DESIGN_SPEC.md
🧬 Code graph analysis (33)
tests/unit/config/conftest.py (1)
src/ai_company/api/config.py (1)
ApiConfig(147-178)
src/ai_company/api/state.py (2)
src/ai_company/api/auth/service.py (1)
AuthService(29-142)src/ai_company/api/errors.py (1)
ServiceUnavailableError(68-74)
tests/unit/api/auth/test_service.py (6)
src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)src/ai_company/api/auth/models.py (1)
User(19-40)src/ai_company/api/auth/service.py (7)
AuthService(29-142)hash_password(39-48)verify_password(50-77)create_token(79-103)decode_token(105-121)hash_api_key(124-133)generate_api_key(136-142)src/ai_company/api/guards.py (1)
HumanRole(18-25)tests/unit/api/auth/test_middleware.py (1)
_make_user(23-33)tests/unit/persistence/sqlite/test_user_repo.py (1)
_make_user(41-56)
src/ai_company/api/auth/__init__.py (3)
src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)src/ai_company/api/auth/models.py (4)
ApiKey(43-66)AuthenticatedUser(69-88)AuthMethod(12-16)User(19-40)src/ai_company/api/auth/service.py (1)
AuthService(29-142)
src/ai_company/api/controllers/__init__.py (1)
src/ai_company/api/auth/controller.py (1)
AuthController(172-354)
src/ai_company/api/config.py (1)
src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)
tests/unit/api/controllers/test_autonomy.py (1)
tests/unit/api/conftest.py (2)
make_auth_headers(514-538)test_client(584-606)
src/ai_company/api/auth/models.py (1)
src/ai_company/api/guards.py (1)
HumanRole(18-25)
src/ai_company/api/exception_handlers.py (2)
src/ai_company/api/dto.py (1)
ApiResponse(31-49)src/ai_company/persistence/errors.py (3)
RecordNotFoundError(20-26)DuplicateRecordError(29-30)PersistenceError(8-9)
src/ai_company/api/auth/secret.py (4)
src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/api/state.py (1)
persistence(82-84)src/ai_company/persistence/protocol.py (3)
PersistenceBackend(27-165)get_setting(139-151)set_setting(153-165)src/ai_company/persistence/sqlite/backend.py (2)
get_setting(357-380)set_setting(382-407)
tests/unit/api/test_exception_handlers.py (1)
src/ai_company/api/errors.py (5)
ApiValidationError(32-38)ConflictError(41-47)ForbiddenError(50-56)NotFoundError(23-29)UnauthorizedError(59-65)
tests/unit/api/auth/test_config.py (1)
src/ai_company/api/auth/config.py (2)
AuthConfig(27-98)with_secret(85-98)
src/ai_company/api/auth/service.py (3)
src/ai_company/api/auth/models.py (1)
User(19-40)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)
tests/unit/persistence/test_protocol.py (2)
src/ai_company/persistence/repositories.py (22)
ApiKeyRepository(407-475)UserRepository(327-403)save(43-52)save(109-118)save(165-175)save(202-211)save(279-289)save(330-339)save(410-419)get(54-66)get(213-225)get(341-353)get(421-433)get_by_username(355-367)list_users(369-378)count(380-389)delete(90-102)delete(255-267)delete(391-403)delete(463-475)get_by_hash(435-447)list_by_user(449-461)src/ai_company/api/auth/models.py (2)
ApiKey(43-66)User(19-40)
tests/unit/persistence/test_migrations_v2.py (2)
src/ai_company/persistence/sqlite/migrations.py (1)
get_user_version(229-233)tests/unit/persistence/sqlite/conftest.py (1)
memory_db(15-22)
tests/unit/api/controllers/test_analytics.py (1)
tests/unit/api/conftest.py (1)
make_auth_headers(514-538)
tests/unit/api/controllers/test_company.py (1)
tests/unit/api/conftest.py (1)
make_auth_headers(514-538)
tests/unit/api/test_app.py (4)
tests/unit/api/conftest.py (2)
root_config(579-580)approval_store(574-575)src/ai_company/api/app.py (1)
_safe_startup(152-219)src/ai_company/api/approval_store.py (1)
ApprovalStore(27-153)src/ai_company/api/state.py (2)
AppState(21-115)persistence(82-84)
tests/unit/api/test_state.py (3)
src/ai_company/api/state.py (1)
auth_service(97-99)tests/unit/api/conftest.py (1)
auth_service(550-551)src/ai_company/api/errors.py (1)
ServiceUnavailableError(68-74)
tests/unit/persistence/sqlite/test_user_repo.py (3)
src/ai_company/api/auth/models.py (2)
ApiKey(43-66)User(19-40)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/persistence/sqlite/user_repo.py (13)
SQLiteApiKeyRepository(213-353)SQLiteUserRepository(58-210)save(68-101)save(223-258)get(103-132)get(260-289)get_by_username(134-160)list_users(162-178)count(180-191)delete(193-210)delete(336-353)get_by_hash(291-310)list_by_user(312-334)
tests/unit/api/controllers/test_approvals.py (1)
tests/unit/api/conftest.py (7)
make_approval(670-698)make_auth_headers(514-538)test_client(584-606)get(51-52)get(214-215)get(285-286)get(313-314)
src/ai_company/api/app.py (4)
src/ai_company/api/auth/middleware.py (1)
create_auth_middleware_class(233-259)src/ai_company/api/auth/secret.py (1)
resolve_jwt_secret(17-64)src/ai_company/api/auth/service.py (1)
AuthService(29-142)src/ai_company/api/state.py (6)
AppState(21-115)persistence(82-84)message_bus(87-89)set_auth_service(101-115)cost_tracker(92-94)auth_service(97-99)
tests/unit/persistence/sqlite/test_migrations.py (3)
tests/unit/persistence/test_migrations_v2.py (1)
memory_db(21-26)tests/unit/persistence/sqlite/conftest.py (1)
memory_db(15-22)src/ai_company/persistence/sqlite/migrations.py (1)
run_migrations(299-367)
tests/unit/api/auth/test_middleware.py (5)
tests/unit/api/conftest.py (5)
FakePersistenceBackend(329-410)auth_config(545-546)connect(346-347)users(399-400)api_keys(403-404)src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)src/ai_company/api/auth/middleware.py (1)
create_auth_middleware_class(233-259)src/ai_company/api/auth/service.py (4)
hash_password(39-48)create_token(79-103)generate_api_key(136-142)hash_api_key(124-133)src/ai_company/api/guards.py (1)
HumanRole(18-25)
tests/unit/api/auth/test_controller.py (4)
src/ai_company/api/guards.py (1)
HumanRole(18-25)tests/unit/api/conftest.py (7)
make_auth_headers(514-538)auth_service(550-551)users(399-400)get(51-52)get(214-215)get(285-286)get(313-314)src/ai_company/api/auth/models.py (3)
User(19-40)AuthenticatedUser(69-88)AuthMethod(12-16)src/ai_company/api/auth/service.py (2)
AuthService(29-142)hash_password(39-48)
tests/unit/api/conftest.py (5)
src/ai_company/api/auth/config.py (1)
AuthConfig(27-98)src/ai_company/api/auth/models.py (2)
ApiKey(43-66)User(19-40)src/ai_company/api/auth/service.py (3)
AuthService(29-142)hash_password(39-48)create_token(79-103)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/persistence/repositories.py (16)
save(43-52)save(109-118)save(165-175)save(202-211)save(279-289)save(330-339)save(410-419)get(54-66)get(213-225)get(341-353)get(421-433)count(380-389)delete(90-102)delete(255-267)delete(391-403)delete(463-475)
src/ai_company/persistence/repositories.py (3)
src/ai_company/api/auth/models.py (2)
ApiKey(43-66)User(19-40)src/ai_company/persistence/sqlite/user_repo.py (8)
get(103-132)get(260-289)list_users(162-178)count(180-191)delete(193-210)delete(336-353)get_by_hash(291-310)list_by_user(312-334)src/ai_company/persistence/sqlite/repositories.py (2)
get(169-189)delete(226-243)
tests/unit/api/controllers/test_budget.py (1)
tests/unit/api/conftest.py (1)
make_auth_headers(514-538)
tests/unit/api/auth/test_secret.py (3)
src/ai_company/api/auth/secret.py (1)
resolve_jwt_secret(17-64)src/ai_company/persistence/sqlite/backend.py (2)
get_setting(357-380)set_setting(382-407)tests/unit/persistence/test_protocol.py (2)
get_setting(267-268)set_setting(270-271)
src/ai_company/api/auth/controller.py (4)
src/ai_company/persistence/sqlite/user_repo.py (6)
get(103-132)get(260-289)count(180-191)save(68-101)save(223-258)get_by_username(134-160)src/ai_company/api/auth/models.py (2)
AuthenticatedUser(69-88)User(19-40)src/ai_company/api/auth/service.py (3)
hash_password(39-48)create_token(79-103)verify_password(50-77)src/ai_company/api/guards.py (1)
HumanRole(18-25)
tests/unit/api/controllers/test_agents.py (2)
tests/unit/api/conftest.py (9)
FakeMessageBus(416-477)FakePersistenceBackend(329-410)make_auth_headers(514-538)_make_test_auth_service(488-490)_seed_test_users(609-635)auth_service(550-551)fake_persistence(555-558)fake_message_bus(562-565)cost_tracker(569-570)src/ai_company/api/auth/service.py (1)
AuthService(29-142)
tests/unit/api/test_guards.py (2)
tests/unit/api/conftest.py (2)
make_auth_headers(514-538)test_client(584-606)tests/unit/api/auth/test_controller.py (1)
bare_client(13-16)
src/ai_company/persistence/sqlite/migrations.py (2)
tests/unit/persistence/sqlite/test_user_repo.py (1)
db(22-28)tests/unit/hr/test_persistence.py (1)
db(29-35)
🪛 ast-grep (0.41.0)
tests/unit/api/auth/test_service.py
[warning] 95-95: Hardcoded JWT secret or private key is used. This is a Insufficiently Protected Credentials weakness: https://cwe.mitre.org/data/definitions/522.html Consider using an appropriate security mechanism to protect the credentials (e.g. keeping secrets in environment variables).
Context: expired_token = jwt.encode(expired_payload, _SECRET, algorithm="HS256")
Note: [CWE-522] Insufficiently Protected Credentials. [REFERENCES]
- https://semgrep.dev/blog/2020/hardcoded-secrets-unverified-tokens-and-other-common-jwt-mistakes/
(jwt-python-hardcoded-secret-python)
🪛 LanguageTool
README.md
[typographical] ~27-~27: To join two clauses or introduce examples, consider using an em dash.
Context: ...ing reports - Litestar REST API (M6) - 15 controllers + WebSocket handler cover...
(DASH_RULE)
[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ...ime feed - Human Approval Queue (M6) - Approval submission, approve/reject with...
(DASH_RULE)
[typographical] ~29-~29: To join two clauses or introduce examples, consider using an em dash.
Context: ...ents - WebSocket Real-Time Feed (M6) - Channel-based subscriptions (tasks, agen...
(DASH_RULE)
[typographical] ~30-~30: To join two clauses or introduce examples, consider using an em dash.
Context: ...ssage-bus bridge - Route Guards (M6) - Role-based read/write access control wit...
(DASH_RULE)
[typographical] ~31-~31: To join two clauses or introduce examples, consider using an em dash.
Context: ... - JWT + API Key Authentication (M7) - Mandatory auth middleware (JWT-first wit...
(DASH_RULE)
[typographical] ~42-~42: To join two clauses or introduce examples, consider using an em dash.
Context: ...only - Security/Approval System (M7) - SecOps agent with rule engine (soft-allo...
(DASH_RULE)
[typographical] ~43-~43: To join two clauses or introduce examples, consider using an em dash.
Context: ...n planned - Advanced Product Surface - web dashboard, external integrations ##...
(DASH_RULE)
🔇 Additional comments (39)
src/ai_company/observability/events/api.py (1)
33-37: Auth event constants look consistent.These additions match the existing event taxonomy and keep the new auth flow observable without introducing ad hoc event names. Based on learnings, use domain-specific event name constants from
ai_company.observability.events.<domain>.src/ai_company/observability/events/persistence.py (1)
127-145: LGTM!The user and API key event constants follow the established
persistence.<entity>.<action>naming convention and provide comprehensive coverage for the new auth persistence layer. The inclusion ofcounted/count_failedfor users aligns well with the admin setup flow requirements.tests/unit/persistence/test_migrations_v2.py (2)
29-32: LGTM!The schema version assertion correctly updated to match the new SCHEMA_VERSION of 5 after the auth tables migration (v5) was added.
53-61: LGTM!The expected user version after
run_migrationsis correctly updated to 5, reflecting the full migration chain (v1 → v2 → v3 → v4 → v5).README.md (2)
27-31: LGTM!The feature documentation accurately describes the new JWT + API key authentication system, including the key implementation details (mandatory middleware, Argon2id hashing, first-run admin setup, SHA-256 API key hashing, regex-based path exclusions).
42-47: LGTM!Status section correctly updated to reflect that JWT + API key authentication is now implemented as part of M7.
src/ai_company/api/auth/controller.py (2)
1-26: LGTM!Module setup follows all coding guidelines: proper logger initialization with
get_logger(__name__), event constants imported from the domain-specificobservability.events.apimodule, and appropriate imports organized.
52-70: LGTM!
SetupRequestDTO is well-designed: frozen Pydantic model, usesNotBlankStrfor identifier fields, and validates password length viamodel_validator. The validation error message is user-friendly.tests/unit/api/controllers/test_analytics.py (2)
9-11: LGTM!Good refactoring to use the centralized
make_auth_headershelper instead of hardcoded headers. This ensures consistent JWT token generation across tests.
28-33: LGTM!Correctly updated to test authentication failure (401) with an invalid Bearer token, reflecting the new JWT-based auth system replacing the previous role-header approach.
tests/unit/api/controllers/test_company.py (2)
8-10: LGTM!Consistent adoption of
make_auth_headershelper matching the pattern used across other controller tests.
29-34: LGTM!Correctly tests the authentication failure case with 401 status for invalid tokens.
tests/unit/api/controllers/test_budget.py (2)
11-13: LGTM!Consistent with other test files in adopting the
make_auth_headershelper for JWT-based authentication.
75-80: LGTM!Correctly updated to expect 401 for invalid authentication token.
CLAUDE.md (1)
68-68: LGTM!Package structure documentation correctly updated to reflect the new JWT + API key authentication capability.
tests/unit/api/test_state.py (2)
60-72: LGTM!Good test coverage for the
auth_serviceaccessor, following the same pattern as existing tests forpersistence,message_bus, andcost_tracker. The test secret meets the minimum length requirement for JWT secrets.
74-92: LGTM!Excellent coverage of the single-assignment semantics for
set_auth_service. Tests both the success case and the guard against double configuration with appropriate error matching.tests/unit/api/test_exception_handlers.py (3)
9-15: LGTM!Import updates correctly include the new
ApiValidationErrorandUnauthorizedErrortypes needed for the new test cases.
82-83: LGTM!Updated assertions correctly verify that 4xx errors now return the actual exception message rather than generic defaults. This aligns with the PR's exception handler changes for detail passthrough.
Also applies to: 95-95, 107-107
132-155: LGTM!New tests properly verify:
UnauthorizedErrormaps to HTTP 401 with the provided messageApiValidationErrormaps to HTTP 422 with the provided messageBoth follow the established test pattern and include appropriate status code and message assertions.
tests/unit/api/auth/test_config.py (1)
1-51: LGTM!Comprehensive test coverage for
AuthConfig:
- Default values and exclude_paths verification
with_secret()method behavior and validation- Frozen model immutability
- Expiry bounds validation (min=1, max=43200)
The broad
Exceptioncatches with noqa comments are acceptable here since Pydantic may raise different validation error types.src/ai_company/api/controllers/__init__.py (1)
5-5: LGTM!
AuthControlleris correctly integrated:
- Imported from the new auth module
- Added to
ALL_CONTROLLERSfor route registration- Exported in
__all__(maintaining alphabetical order)Also applies to: 37-37, 46-46
src/ai_company/api/guards.py (2)
39-53: LGTM!Role extraction now correctly sources from the authenticated user object (
connection.scope["user"]) populated by the auth middleware. The implementation:
- Safely checks for user presence and role attribute
- Validates against
HumanRoleenum with proper error handling- Logs invalid roles before returning
None(denying access)
3-4: LGTM!Documentation updates correctly reflect that role information now comes from
connection.user.role(populated by auth middleware) rather than HTTP headers.Also applies to: 62-63, 89-90
src/ai_company/api/auth/secret.py (1)
17-64: LGTM!Well-structured JWT secret resolution with clear priority chain:
- Environment variable with length validation
- Persistence lookup
- Auto-generate and persist
Logging at appropriate levels (ERROR for validation failures, INFO for successful resolution paths).
tests/unit/api/controllers/test_agents.py (2)
9-13: LGTM on the auth scaffolding integration.The test correctly:
- Imports the new auth helpers (
make_auth_headers,_make_test_auth_service,_seed_test_users)- Creates and passes
auth_servicetocreate_app- Uses
make_auth_headers("observer")instead of raw header assignment(Note: The async issue with
_seed_test_usersis addressed in a separate comment.)Also applies to: 31-33, 52-52, 55-55
45-46: Function_seed_test_usersis not async — no missingawaitneeded.
_seed_test_usersis a regular synchronous function (defined attests/unit/api/conftest.py:609). It internally manages asyncio operations usingasyncio.get_event_loop()andloop.run_until_complete(). The call at line 46 intest_agents.pyis correct.> Likely an incorrect or invalid review comment.src/ai_company/api/auth/service.py (5)
20-26: LGTM!Argon2id hasher configuration uses reasonable security parameters:
time_cost=3: Provides good resistance to brute-forcememory_cost=65536(64MB): Memory-hard to resist GPU attacksparallelism=4: Balances security with server resources
50-77: LGTM on error handling.Comprehensive exception handling for password verification:
VerifyMismatchError: ReturnsFalsesilently (expected case)VerificationError: Logs warning with context, returnsFalseInvalidHashError: Logs warning for corrupted hashes, returnsFalseThis prevents timing attacks and provides appropriate observability without leaking details.
79-121: LGTM!JWT implementation is secure:
- Uses configured algorithm from
AuthConfig(HMAC family only)- Includes standard claims (
sub,iat,exp) and necessary custom claimsdecode_tokenuses an explicitalgorithmslist to prevent algorithm confusion attacks
123-142: LGTM!API key utilities are appropriately implemented:
- SHA-256 is suitable for hashing API keys (not passwords, so no need for slow hash)
secrets.token_urlsafe(32)provides 256 bits of entropy
60-61: Code correctly follows argon2-cffi API.The
PasswordHasher.verify()method signature isverify(hash, password). The code correctly passes(password_hash, password), matching the library's documented API. No issue here.> Likely an incorrect or invalid review comment.tests/unit/api/auth/test_service.py (2)
10-14: LGTM!The hardcoded
_SECRETconstant is appropriate for unit tests - it ensures deterministic, reproducible test behavior. The static analysis warning about hardcoded secrets (line 96) is a false positive in this test context.
37-141: LGTM!Comprehensive test coverage for
AuthService:
- Password hashing: Verifies Argon2id behavior including salt variability
- JWT lifecycle: Tests creation, decoding, expiry handling, and signature validation
- API key utilities: Confirms deterministic hashing and unique key generation
Test techniques are appropriate (e.g., manually crafting expired tokens for expiry tests).
src/ai_company/persistence/repositories.py (1)
11-12: 🛠️ Refactor suggestion | 🟠 MajorKeep persistence abstractions out of the API package.
Importing
UserandApiKeyfromai_company.api.auth.modelsmakes the persistence layer depend on the API namespace. That inverts the dependency direction and makes future auth/persistence cycles much easier to introduce. Please move these shared auth entities into a persistence-neutral/domain module and import them from both sides there.⛔ Skipped due to learnings
Learnt from: CR Repo: Aureliolo/ai-company PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-10T23:12:08.513Z Learning: Applies to src/ai_company/**/*.py : Pure data models, enums, and re-exports do NOT require loggingDESIGN_SPEC.md (4)
3189-3195: LGTM: Well-structured authentication module.The auth module follows a clean separation of concerns:
- config.py — Frozen Pydantic models for auth configuration
- controller.py — REST endpoints (setup, login, change-password, me)
- middleware.py — Request interception (JWT-first, API key fallback)
- models.py — Domain models (User, ApiKey, AuthenticatedUser)
- secret.py — JWT secret resolution chain (env → persistence → generate)
- service.py — Crypto operations (Argon2id, JWT, SHA-256)
The "JWT-first, API key fallback" middleware design is excellent — it prefers short-lived session tokens over long-lived API keys, following security best practices.
2986-2997: LGTM: Persistence layer follows established patterns.The addition of
UserRepositoryandApiKeyRepositoryto the persistence protocol (line 2986) and the corresponding SQLite implementations inuser_repo.py(line 2996) follow the established repository protocol pattern from §7.6.Migration versioning (v4 for audit, v5 for auth) correctly uses SQLite's
user_versionpragma for schema tracking. The PR summary indicates this resolves merge conflicts between two parallel features (audit + auth), which explains the jump from v3 → v5.
3199-3208: LGTM: API layer integration updates are consistent.The updates integrate authentication into the existing API layer:
- Line 3201:
UnauthorizedErroradded to error hierarchy → enables proper 401 HTTP responses- Line 3203: Guards extended for role-based access control → mentioned in PR summary
- Line 3208:
AppStateupdated with "deferred auth init" → suggestsAuthServiceinitialization happens afterPersistenceBackendis ready (correct dependency order)Controller count remains 15 + 1 WebSocket (16 route modules) —
AuthControlleris one of the 15, not an addition.
2762-2762: Implementation correctly uses cryptographically secure random generation for both API keys and JWT secrets — no changes needed.The API key generation uses
secrets.token_urlsafe(32)to produce 43-character high-entropy bearer tokens (src/ai_company/api/auth/service.py:136), which are then SHA-256 hashed for storage. SHA-256 is appropriate for high-entropy generated tokens.The JWT secret generation uses
secrets.token_urlsafe(48)with a 32-character minimum length (src/ai_company/api/auth/secret.py:54), following a three-tier fallback: environment variable, persistence, or auto-generate. The implementation is secure.> Likely an incorrect or invalid review comment.
| | **Docker API** | aiodocker | Async-native Docker API client for `DockerSandbox` backend | | ||
| | **Tool Integration** | MCP SDK (`mcp`) | Industry standard for LLM-to-tool integration | | ||
| | **Agent Comms** | A2A Protocol compatible | Future-proof inter-agent communication | | ||
| | **Authentication** | PyJWT + argon2-cffi | JWT (HMAC HS256/384/512) for session tokens, Argon2id for password hashing, SHA-256 for API key storage | |
There was a problem hiding this comment.
Consider impact of mandatory authentication on "Local First" design principle.
The PR introduces mandatory JWT + API key authentication. While this is excellent for security in networked deployments, it may conflict with the "Local First" design principle (§1.2):
"Local First — Runs locally with option to expose on network or host remotely later"
Mandatory authentication adds friction for local single-user scenarios:
- Users must complete password setup on first run
- No quick "just try it" experience for local evaluation
- Local-only deployments don't inherently need multi-user auth
Industry precedent: Docker Desktop, PostgreSQL, Redis, and many developer tools run locally without authentication by default, only requiring it when exposed on a network.
Possible mitigations:
- Require auth only when binding to non-localhost addresses (bind address detection)
- Add a development mode flag (
--no-authorAI_COMPANY_DEV_MODE=1) with clear security warnings - Auto-login for localhost-only deployments (check if
Hostheader islocalhost/127.0.0.1)
This is a design trade-off between security-by-default and developer experience. Please confirm this trade-off is intentional, or consider adding an escape hatch for local development.
Based on learnings: This implementation changes the UX significantly for local-first usage. If this deviates from the original vision, user approval may be needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DESIGN_SPEC.md` at line 2762, The design enforces mandatory JWT/API-key auth
which harms local-first UX; add a guarded bypass so local single-user runs can
skip auth: introduce a config flag (e.g., AUTH_REQUIRED default true) and an
explicit dev override (e.g., CLI --no-auth or env AI_COMPANY_DEV_MODE=true) and
amend the central auth path (the auth middleware / function used for all
requests, e.g., authMiddleware or validateAuth) to short-circuit authentication
when AUTH_REQUIRED is false OR when server bind address is localhost/127.0.0.1
(detect via the server.bindAddress or incoming Host header) and log a strong
security warning; keep auth enforced for any non-local bind or when the override
is not set.
src/ai_company/api/auth/config.py
Outdated
| exclude_paths: tuple[str, ...] = Field( | ||
| default=( | ||
| "^/api/v1/health$", | ||
| "^/docs", | ||
| "^/api$", | ||
| "^/api/v1/auth/setup$", | ||
| "^/api/v1/auth/login$", | ||
| ), |
There was a problem hiding this comment.
Derive auth-exempt routes from api_prefix.
These patterns hardcode /api/v1, but ApiConfig.api_prefix is configurable in src/ai_company/api/config.py (Lines 175-177). As soon as the prefix changes, /auth/setup, /auth/login, and the health route stop matching the exclusion list and can get trapped behind the auth middleware. Build these defaults from the configured prefix at app wiring time instead of baking the default into AuthConfig.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/auth/config.py` around lines 65 - 72, The exclude_paths
default in AuthConfig is hardcoded to "/api/v1" routes; remove those hardcoded
defaults in AuthConfig (e.g., set Field default to an empty tuple) and instead
derive the auth-exempt regexes at app wiring time using ApiConfig.api_prefix
from ApiConfig (ApiConfig.api_prefix) — compute patterns for health, docs, base
api and auth routes using the configured prefix and inject/assign that tuple to
AuthConfig.exclude_paths when you instantiate AuthConfig in the app
factory/bootstrap so the exclusion list follows the configured api_prefix.
| def require_password_changed( | ||
| connection: ASGIConnection, # type: ignore[type-arg] | ||
| _: object, | ||
| ) -> None: | ||
| """Guard that blocks users who must change their password. | ||
|
|
||
| Applied to all routes except ``/auth/change-password`` and | ||
| ``/auth/me``. | ||
|
|
||
| Args: | ||
| connection: The incoming connection. | ||
| _: Route handler (unused). | ||
|
|
||
| Raises: | ||
| PermissionDeniedException: If password change is required. | ||
| """ | ||
| user = connection.scope.get("user") | ||
| if not isinstance(user, AuthenticatedUser): | ||
| return | ||
| if user.must_change_password: | ||
| raise PermissionDeniedException(detail="Password change required") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defensive handling for missing user scope.
The guard silently returns if user is not an AuthenticatedUser, which works when the middleware hasn't populated the scope. However, this permissive behavior could mask configuration issues where the guard is applied to a route that should require authentication but the middleware isn't running.
Consider logging at DEBUG level when returning early to aid debugging:
💡 Optional: Add debug logging for early return
user = connection.scope.get("user")
if not isinstance(user, AuthenticatedUser):
+ logger.debug(
+ "require_password_changed guard skipped: no AuthenticatedUser in scope"
+ )
return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/auth/controller.py` around lines 146 - 167, The guard
require_password_changed currently returns silently when
connection.scope.get("user") is missing or not an instance of AuthenticatedUser;
update it to emit a DEBUG-level log in that early-return path so missing
middleware/configuration is visible during debugging. Specifically, inside
require_password_changed, after fetching user from connection.scope and before
returning when not isinstance(user, AuthenticatedUser), call the module logger
(or create one if missing) to log a concise debug message that includes that the
user scope was not populated and include any identifying info available from
connection.scope (e.g., path or headers) to help trace the misconfiguration;
keep the existing behavior otherwise. Ensure the change references
require_password_changed, AuthenticatedUser, and connection.scope.get("user").
| def test_setup_409_when_users_exist(self, bare_client: TestClient[Any]) -> None: | ||
| # Re-seed a user so the check fails | ||
| import asyncio | ||
| import uuid | ||
| from datetime import UTC, datetime | ||
|
|
||
| from ai_company.api.auth.models import User | ||
| from ai_company.api.auth.service import AuthService # noqa: TC001 | ||
| from ai_company.api.guards import HumanRole | ||
|
|
||
| app_state = bare_client.app.state["app_state"] | ||
| svc: AuthService = app_state.auth_service | ||
| now = datetime.now(UTC) | ||
| user = User( | ||
| id=str(uuid.uuid4()), | ||
| username="existing", | ||
| password_hash=svc.hash_password("test-password-12chars"), | ||
| role=HumanRole.CEO, | ||
| must_change_password=False, | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| loop = asyncio.get_event_loop() | ||
| loop.run_until_complete(app_state.persistence.users.save(user)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
find . -type f -name "test_controller.py" -path "*/api/auth/*" | head -5Repository: Aureliolo/ai-company
Length of output: 104
🏁 Script executed:
# Check pytest configuration
find . -type f -name "pytest.ini" -o -name "pyproject.toml" -o -name "setup.cfg" | xargs grep -l "asyncio_mode" 2>/dev/null || echo "No asyncio_mode found yet"Repository: Aureliolo/ai-company
Length of output: 80
🏁 Script executed:
# Look for the test file and examine lines around 38-61
if [ -f "tests/unit/api/auth/test_controller.py" ]; then
sed -n '1,70p' "tests/unit/api/auth/test_controller.py" | cat -n
fiRepository: Aureliolo/ai-company
Length of output: 2838
🏁 Script executed:
# Check TestClient and fixture definitions
rg "class TestClient|def bare_client" -A 5 --type pyRepository: Aureliolo/ai-company
Length of output: 884
🏁 Script executed:
# Check pyproject.toml for pytest asyncio configuration
sed -n '/\[tool.pytest/,/^\[/p' ./pyproject.toml | grep -A 20 "asyncio"Repository: Aureliolo/ai-company
Length of output: 708
🏁 Script executed:
# Find the test_client fixture definition
rg "def test_client" -B 2 -A 10 tests/Repository: Aureliolo/ai-company
Length of output: 812
🏁 Script executed:
# Check if there's a conftest.py at the test root level
find tests -name conftest.py -exec echo "File: {}" \; -exec head -50 {} \;Repository: Aureliolo/ai-company
Length of output: 43040
🏁 Script executed:
# Check the Python version requirement
grep -i "python" pyproject.toml | head -20Repository: Aureliolo/ai-company
Length of output: 338
🏁 Script executed:
# Check how other tests handle async database/persistence operations
rg "asyncio.get_event_loop|run_until_complete" tests/ -A 2 -B 2Repository: Aureliolo/ai-company
Length of output: 804
🏁 Script executed:
# Check if there are async test examples in the same file or related files
grep -n "async def test_" tests/unit/api/auth/test_controller.pyRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Search for async test patterns in the test suite
rg "async def test_" tests/unit/ | head -20Repository: Aureliolo/ai-company
Length of output: 1680
🏁 Script executed:
# Check TestClient documentation and usage with async
rg "TestClient.*async|async.*TestClient" tests/ -B 3 -A 3Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Check if bare_client fixture can be used in async tests
grep -A 20 "def bare_client" tests/unit/api/auth/test_controller.pyRepository: Aureliolo/ai-company
Length of output: 779
🏁 Script executed:
# Check if there are any tests that use TestClient in async context
rg "async def test.*TestClient|TestClient.*async" tests/unit/api/ -B 2 -A 5Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Look at the full test method to see what happens after persistence.save
sed -n '38,75p' tests/unit/api/auth/test_controller.py | cat -nRepository: Aureliolo/ai-company
Length of output: 1720
🏁 Script executed:
# Check Python 3.14+ asyncio.get_event_loop() behavior by looking at Python version handling
rg "sys.version|version_info" . --type py | grep -i "3\.14\|3\.15" | head -5Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Verify: search for alternative patterns like asyncio.run() usage
rg "asyncio\.run\(" tests/ -B 2 -A 2Repository: Aureliolo/ai-company
Length of output: 480
🏁 Script executed:
# Check if there's any documentation or comments about Python 3.14 event loop behavior
rg "get_event_loop|3\.14|3\.15" . --type py --type md | grep -i "event\|loop" | head -10Repository: Aureliolo/ai-company
Length of output: 213
🏁 Script executed:
# Look at how persistence is typically initialized/tested
rg "persistence.*save|users.save" tests/unit/api/ -B 3 -A 3 | head -40Repository: Aureliolo/ai-company
Length of output: 2519
🏁 Script executed:
# Check the FakePersistenceBackend setup to understand async requirements
sed -n '1,150p' tests/unit/api/conftest.py | grep -A 50 "class Fake"Repository: Aureliolo/ai-company
Length of output: 3290
🏁 Script executed:
# Verify: can TestClient work with async tests? Search Litestar test patterns
rg "TestClient" tests/unit/api/ | head -15Repository: Aureliolo/ai-company
Length of output: 1473
🏁 Script executed:
# Check if test_client fixture is async or sync
grep -B 5 -A 25 "def test_client" tests/unit/api/conftest.py | head -40Repository: Aureliolo/ai-company
Length of output: 966
🏁 Script executed:
# Check if asyncio.new_event_loop() is used anywhere as alternative pattern
rg "new_event_loop|asyncio.run" tests/ -B 2 -A 2Repository: Aureliolo/ai-company
Length of output: 480
🏁 Script executed:
# Check conftest.py _seed_test_users function completely - it also has the issue
sed -n '40,100p' tests/unit/api/conftest.py | cat -nRepository: Aureliolo/ai-company
Length of output: 2289
🏁 Script executed:
# Verify: can asyncio.get_event_loop() fail in pytest with asyncio_mode=auto?
# Search for pytest-asyncio version/docs
grep -r "pytest-asyncio\|pytest_asyncio" pyproject.toml setup.py setup.cfg 2>/dev/null | head -5Repository: Aureliolo/ai-company
Length of output: 107
🏁 Script executed:
# Check full _seed_test_users function to see if it also has the same pattern
rg "_seed_test_users" -A 30 tests/unit/api/conftest.pyRepository: Aureliolo/ai-company
Length of output: 2469
🏁 Script executed:
# Check if there's a pattern for using asyncio.run() or alternatives elsewhere
rg "asyncio.run\(|asyncio.new_event_loop" tests/ -B 2 -A 2Repository: Aureliolo/ai-company
Length of output: 480
🏁 Script executed:
# Verify: What does pytest-asyncio 1.3.0 with asyncio_mode="auto" do for sync tests?
# Check if there's an event loop running during sync tests
grep -A 5 -B 5 "asyncio_mode" pyproject.tomlRepository: Aureliolo/ai-company
Length of output: 447
🏁 Script executed:
# Check TestClient to see if it's truly synchronous or can be used in async context
rg "class TestClient|def post\(" tests/unit/api/ -A 3 | head -30Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Search for how other tests seed async data without using get_event_loop
rg "def.*test.*persistence|fake_persistence" tests/unit/api/ -B 2 | head -40Repository: Aureliolo/ai-company
Length of output: 2203
🏁 Script executed:
# Check if bare_client can even be used in async test context
# Look at how TestClient is implemented for async compatibility
python3 << 'EOF'
# Check if TestClient can work in async context
# by looking at whether it's actually sync-only
import inspect
try:
from litestar.testing import TestClient
# Check if TestClient methods are async or sync
print("TestClient methods:")
for name in ['post', 'get', 'put', 'delete']:
method = getattr(TestClient, name, None)
if method:
print(f" {name}: async={inspect.iscoroutinefunction(method)}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: Aureliolo/ai-company
Length of output: 97
🏁 Script executed:
# Check the fake_persistence fixture definition more carefully
sed -n '1,300p' tests/unit/api/conftest.py | grep -A 20 "async def fake_persistence"Repository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
# Key question: how does pytest handle sync fixture depending on async fixture?
# Look at test_client fixture definition again
sed -n '170,210p' tests/unit/api/conftest.py | cat -nRepository: Aureliolo/ai-company
Length of output: 1597
🏁 Script executed:
# Check the exact definition of fake_persistence fixture
grep -B 5 -A 15 "^async def fake_persistence" tests/unit/api/conftest.pyRepository: Aureliolo/ai-company
Length of output: 503
🏁 Script executed:
# Verify: How does pytest-asyncio handle sync fixtures depending on async fixtures?
# Check if there are other sync fixtures depending on async fixtures
grep -B 3 "async def" tests/unit/api/conftest.py | grep -A 3 "fixture"Repository: Aureliolo/ai-company
Length of output: 206
🏁 Script executed:
# Final check: Can we make the test async while still using TestClient?
# Search for async test patterns that use fixtures
rg "async def test_.*\(" tests/unit/ -A 5 | grep -B 2 "TestClient\|bare_client\|test_client" | head -30Repository: Aureliolo/ai-company
Length of output: 1837
🏁 Script executed:
# Final verification: Check if there are any tests that show async tests using TestClient
rg "async def test_" tests/unit/api/ -A 10 | grep -B 10 "TestClient\|bare_client\|test_client" || echo "No async tests use TestClient"Repository: Aureliolo/ai-company
Length of output: 15729
🏁 Script executed:
# Check what _seed_test_users is called - only in test_client fixture?
rg "_seed_test_users" tests/ -B 3 -A 1Repository: Aureliolo/ai-company
Length of output: 2058
🏁 Script executed:
# Verify: The core issue - can the test even be made async while using TestClient?
# Check if TestClient can be passed to async tests
sed -n '185,215p' tests/unit/api/conftest.py | cat -nRepository: Aureliolo/ai-company
Length of output: 1225
🏁 Script executed:
# Final key check: verify async test methods CAN be in `@pytest.mark.unit` classes
rg "@pytest.mark.unit" -A 20 tests/unit/api/controllers/test_approvals.py | head -40Repository: Aureliolo/ai-company
Length of output: 1413
🏁 Script executed:
# Check exact pattern from test_approvals.py to see how it handles async tests in marked classes
sed -n '1,50p' tests/unit/api/controllers/test_approvals.py | cat -nRepository: Aureliolo/ai-company
Length of output: 1827
🏁 Script executed:
# Search for any documentation or handling of Python 3.14 asyncio changes
web_search "Python 3.14 asyncio.get_event_loop RuntimeError no event loop"Repository: Aureliolo/ai-company
Length of output: 114
🏁 Script executed:
# Verify: Actually test if asyncio.get_event_loop() fails on newer Python versions
python3 << 'EOF'
import sys
import asyncio
print(f"Python {sys.version}")
try:
loop = asyncio.get_event_loop()
print(f"get_event_loop() succeeded: {loop}")
except RuntimeError as e:
print(f"get_event_loop() raised RuntimeError: {e}")
EOFRepository: Aureliolo/ai-company
Length of output: 212
🌐 Web query:
Python 3.14 asyncio.get_event_loop RuntimeError no event loop
💡 Result:
In Python 3.14, asyncio.get_event_loop() was changed to raise RuntimeError if there is no “current” event loop (it no longer auto-creates one in common cases). (docs.python.org)
What to do instead
1) Top-level (you want to run an async entry point): use asyncio.run()
import asyncio
async def main():
...
asyncio.run(main())This replaces most historical get_event_loop()+run_until_complete() patterns. (kiwix.shawncarter.co.uk)
2) You are already inside async def: use asyncio.get_running_loop()
import asyncio
async def f():
loop = asyncio.get_running_loop()get_running_loop() is the correct API when a loop must already be running. (stackoverflow.com)
3) You are in synchronous code (or a non-main thread) and truly need a loop object: create and set one
import asyncio
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
try:
loop.run_until_complete(main())
finally:
loop.close()This avoids relying on get_event_loop() auto-creation behavior that no longer exists in 3.14. (docs.python.org)
If you paste the smallest snippet that triggers your error (and whether it’s main thread, a worker thread, Jupyter, etc.), I can show the exact minimal change.
Citations:
- 1: https://docs.python.org/3.15/library/asyncio-eventloop.html?utm_source=openai
- 2: https://kiwix.shawncarter.co.uk/content/docs.python.org_en_2025-03/docs.python.org/dev/whatsnew/3.14.html?utm_source=openai
- 3: https://stackoverflow.com/questions/65206110/when-to-use-asyncio-get-running-loop-vs-asyncio-get-event-loop?utm_source=openai
- 4: https://docs.python.org/3.15/library/asyncio-eventloop.html?utm_source=openai
Make the test async to avoid asyncio.get_event_loop() brittle behavior on Python 3.14+.
In Python 3.14, asyncio.get_event_loop() raises RuntimeError when no running loop is set. Convert this test to async and await the persistence call directly; TestClient methods (.post(), .get()) remain synchronous and can be called normally.
Suggested change
- def test_setup_409_when_users_exist(self, bare_client: TestClient[Any]) -> None:
+ async def test_setup_409_when_users_exist(
+ self, bare_client: TestClient[Any]
+ ) -> None:
# Re-seed a user so the check fails
- import asyncio
import uuid
from datetime import UTC, datetime
@@ -55,8 +54,7 @@ class TestSetup:
created_at=now,
updated_at=now,
)
- loop = asyncio.get_event_loop()
- loop.run_until_complete(app_state.persistence.users.save(user))
+ await app_state.persistence.users.save(user)
response = bare_client.post(
"/api/v1/auth/setup",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/auth/test_controller.py` around lines 38 - 61, Convert the
test_setup_409_when_users_exist test to an async test function and remove the
use of asyncio.get_event_loop(); instead await the persistence call directly
(await app_state.persistence.users.save(user)). Keep TestClient usage unchanged
(its .post()/.get() calls remain synchronous). This involves changing the test
signature to async def test_setup_409_when_users_exist(...), removing loop =
asyncio.get_event_loop() and loop.run_until_complete(...), and replacing that
with a direct await of app_state.persistence.users.save(user) so the save
operation runs under the pytest async fixture/runtime.
| @pytest.fixture | ||
| def test_client( | ||
| def test_client( # noqa: PLR0913 | ||
| fake_persistence: FakePersistenceBackend, | ||
| fake_message_bus: FakeMessageBus, | ||
| cost_tracker: CostTracker, | ||
| approval_store: ApprovalStore, | ||
| root_config: RootConfig, | ||
| auth_service: AuthService, | ||
| ) -> TestClient[Any]: | ||
| # Pre-seed users for each role so JWT sub claims resolve | ||
| _seed_test_users(fake_persistence, auth_service) | ||
|
|
||
| app = create_app( | ||
| config=root_config, | ||
| persistence=fake_persistence, | ||
| message_bus=fake_message_bus, | ||
| cost_tracker=cost_tracker, | ||
| approval_store=approval_store, | ||
| auth_service=auth_service, | ||
| ) | ||
| client = TestClient(app) | ||
| client.headers["X-Human-Role"] = "observer" | ||
| # Default: CEO token (most tests need write access) | ||
| client.headers.update(make_auth_headers("ceo")) | ||
| return client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd '^(pyproject\.toml|pytest\.ini|tox\.ini)$' . -x rg -n -C2 'requires-python|target-version|asyncio_mode' {}
rg -n -C2 'get_event_loop\(|run_until_complete\(' tests/unit/api/conftest.py testsRepository: Aureliolo/ai-company
Length of output: 1650
🏁 Script executed:
cat -n tests/unit/api/conftest.py | sed -n '583,636p'Repository: Aureliolo/ai-company
Length of output: 2151
🏁 Script executed:
rg -n 'def create_app' src tests --type py
head -20 src/api/server.py | grep -A 15 'def create_app'Repository: Aureliolo/ai-company
Length of output: 299
🏁 Script executed:
sed -n '254,280p' src/ai_company/api/app.pyRepository: Aureliolo/ai-company
Length of output: 991
🏁 Script executed:
rg -c 'get_event_loop\(|run_until_complete\(' tests --type py
rg -l 'get_event_loop\(|run_until_complete\(' tests --type pyRepository: Aureliolo/ai-company
Length of output: 199
🏁 Script executed:
sed -n '55,65p' tests/unit/api/auth/test_controller.pyRepository: Aureliolo/ai-company
Length of output: 404
Stop driving the event loop manually from this fixture.
_seed_test_users() reaches into the global loop and calls run_until_complete() from synchronous setup. This contradicts the asyncio_mode = "auto" configuration in pytest, which expects fixtures and tests to be async-native. Make the helper and fixture async and await backend.users.save(...) directly.
🐛 Proposed fix
-@pytest.fixture
-def test_client( # noqa: PLR0913
+@pytest.fixture
+async def test_client( # noqa: PLR0913
fake_persistence: FakePersistenceBackend,
fake_message_bus: FakeMessageBus,
cost_tracker: CostTracker,
@@
auth_service: AuthService,
) -> TestClient[Any]:
# Pre-seed users for each role so JWT sub claims resolve
- _seed_test_users(fake_persistence, auth_service)
+ await _seed_test_users(fake_persistence, auth_service)
@@
-def _seed_test_users(
+async def _seed_test_users(
backend: FakePersistenceBackend,
auth_service: AuthService,
) -> None:
@@
- # Save synchronously via event loop
- loop = asyncio.get_event_loop()
- loop.run_until_complete(backend.users.save(user))
+ await backend.users.save(user)Also applies to: 609-635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/conftest.py` around lines 583 - 606, The test fixture
test_client (and the other similar fixture invoking the same helper) must stop
driving the asyncio event loop manually; convert the helper _seed_test_users to
an async function and make the fixture async so you can await it (await
_seed_test_users(...)) instead of calling run_until_complete on the global loop,
and update the helper implementation to await backend.users.save(...) (and any
other async backend calls) directly; remove any run_until_complete or loop.run_*
calls and ensure fixtures use async/await flow so pytest asyncio_mode="auto" can
manage the loop.
| async def test_v5_creates_users_table( | ||
| self, memory_db: aiosqlite.Connection | ||
| ) -> None: | ||
| await run_migrations(memory_db) | ||
| cursor = await memory_db.execute( | ||
| "SELECT name FROM sqlite_master WHERE type='table' AND name='users'" | ||
| ) | ||
| row = await cursor.fetchone() | ||
| assert row is not None | ||
|
|
||
| async def test_v5_creates_api_keys_table( | ||
| self, memory_db: aiosqlite.Connection | ||
| ) -> None: | ||
| await run_migrations(memory_db) | ||
| cursor = await memory_db.execute( | ||
| "SELECT name FROM sqlite_master WHERE type='table' AND name='api_keys'" | ||
| ) | ||
| row = await cursor.fetchone() | ||
| assert row is not None | ||
|
|
||
| async def test_v5_creates_settings_table( | ||
| self, memory_db: aiosqlite.Connection | ||
| ) -> None: | ||
| await run_migrations(memory_db) | ||
| cursor = await memory_db.execute( | ||
| "SELECT name FROM sqlite_master WHERE type='table' AND name='settings'" | ||
| ) | ||
| row = await cursor.fetchone() | ||
| assert row is not None | ||
|
|
||
| async def test_v5_creates_user_indexes( | ||
| self, memory_db: aiosqlite.Connection | ||
| ) -> None: | ||
| await run_migrations(memory_db) | ||
| cursor = await memory_db.execute( | ||
| "SELECT name FROM sqlite_master WHERE type='index' " | ||
| "AND name LIKE 'idx_%' AND name LIKE '%user%' ORDER BY name" | ||
| ) | ||
| indexes = {row[0] for row in await cursor.fetchall()} | ||
| assert "idx_users_username" in indexes | ||
| assert "idx_api_keys_user_id" in indexes |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Collapse the v5 schema checks into parametrized tests.
These four additions all use the same arrange/act/assert shape. A small @pytest.mark.parametrize pass for table names (and another for index names) would cut the duplication and make future schema additions cheaper to cover.
As per coding guidelines, "Prefer @pytest.mark.parametrize for testing similar cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/persistence/sqlite/test_migrations.py` around lines 147 - 187,
Collapse the four nearly identical tests into two parametrized tests: one
parameterizing table names and one parameterizing expected index names. Replace
test_v5_creates_users_table/test_v5_creates_api_keys_table/test_v5_creates_settings_table
with a single async test decorated with `@pytest.mark.parametrize`("table_name",
["users","api_keys","settings"]) that calls run_migrations(memory_db), queries
sqlite_master for a table with name=table_name using memory_db.execute and
asserts the fetched row is not None; similarly replace
test_v5_creates_user_indexes with an `@pytest.mark.parametrize`("expected_index",
["idx_users_username","idx_api_keys_user_id"]) async test that runs
run_migrations(memory_db), selects index names with the same LIKE query used
before, collects names into a set and asserts expected_index in that set. Use
the existing memory_db fixture and run_migrations call names to locate the code
to edit.
| conn = await aiosqlite.connect(":memory:") | ||
| conn.row_factory = aiosqlite.Row | ||
| await run_migrations(conn) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'PRAGMA\s+foreign_keys' src tests
rg -n -C3 'CREATE TABLE api_keys|REFERENCES users' src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 312
🏁 Script executed:
rg -n "def run_migrations|async def run_migrations" src testsRepository: Aureliolo/ai-company
Length of output: 175
🏁 Script executed:
cat -n tests/unit/persistence/sqlite/test_user_repo.py | head -30Repository: Aureliolo/ai-company
Length of output: 1070
🏁 Script executed:
sed -n '299,330p' src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 1101
🏁 Script executed:
sed -n '299,450p' src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 2294
🏁 Script executed:
rg -n "_MIGRATIONS\s*=" src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
sed -n '1,100p' src/ai_company/persistence/sqlite/migrations.py | head -50Repository: Aureliolo/ai-company
Length of output: 1672
🏁 Script executed:
sed -n '200,300p' src/ai_company/persistence/sqlite/migrations.py | grep -n "_MIGRATIONS\|_V"Repository: Aureliolo/ai-company
Length of output: 347
🏁 Script executed:
rg -n "PRAGMA foreign_keys\|foreign_keys.*ON" src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
sed -n '91,98p' src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 327
🏁 Script executed:
rg -n "CREATE TABLE api_keys" -A 10 src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 46
🏁 Script executed:
sed -n '210,230p' src/ai_company/persistence/sqlite/migrations.pyRepository: Aureliolo/ai-company
Length of output: 799
🏁 Script executed:
rg -n "_V5_STATEMENTS" -A 30 src/ai_company/persistence/sqlite/migrations.py | head -40Repository: Aureliolo/ai-company
Length of output: 1484
Enable foreign-key enforcement on this SQLite connection.
The api_keys table schema defines user_id TEXT NOT NULL REFERENCES users(id), but SQLite foreign-key constraints are disabled by default per connection. Neither the fixture nor run_migrations() executes PRAGMA foreign_keys = ON, so tests can pass even when api_keys.user_id holds orphaned values that violate the foreign-key constraint.
Proposed fix
async def db() -> AsyncGenerator[aiosqlite.Connection]:
"""Create an in-memory SQLite DB with schema applied."""
conn = await aiosqlite.connect(":memory:")
+ await conn.execute("PRAGMA foreign_keys = ON")
conn.row_factory = aiosqlite.Row
await run_migrations(conn)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn = await aiosqlite.connect(":memory:") | |
| conn.row_factory = aiosqlite.Row | |
| await run_migrations(conn) | |
| conn = await aiosqlite.connect(":memory:") | |
| await conn.execute("PRAGMA foreign_keys = ON") | |
| conn.row_factory = aiosqlite.Row | |
| await run_migrations(conn) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/persistence/sqlite/test_user_repo.py` around lines 24 - 26, Enable
SQLite foreign keys on the opened aiosqlite connection before running
migrations: after creating the connection (the variable `conn` in the test
setup) call and await execution of `PRAGMA foreign_keys = ON` (e.g., `await
conn.execute("PRAGMA foreign_keys = ON")` and `await conn.commit()`), then call
`run_migrations(conn)` so the `api_keys.user_id REFERENCES users(id)` constraint
is enforced during the tests.
- AuthConfig: make exclude_paths None-default, derived from api_prefix - AuthService: add empty-secret guards, pwd_sig claim, async hashing - Middleware: JWT-only fallthrough for dot-tokens, pwd_sig validation - Controller: TOCTOU race guard on setup, path-based guard exemptions - Models: switch datetime to AwareDatetime, TokenResponse validation - Secret: import MIN_SECRET_LENGTH, validate stored secret length - State: add has_auth_service property, error logging - Protocol: add users/api_keys to docstring, NotBlankStr for settings - SQLite repos: add list_by_user error handling, NotBlankStr params - Events: add PERSISTENCE_SETTING_FETCHED/SAVED constants - DESIGN_SPEC: update implementation snapshot (JWT auth done) - Tests: add corrupted hash, missing sub, bearer edge cases, parametrize guards, short password change test
| # Race guard: undo if another setup completed concurrently | ||
| post_count = await persistence.users.count() | ||
| if post_count > 1: | ||
| await persistence.users.delete(user.id) | ||
| logger.warning(API_AUTH_FAILED, reason="setup_race_detected") | ||
| msg = "Setup already completed" | ||
| raise ConflictError(msg) |
There was a problem hiding this comment.
Race-undo can delete both competing admin accounts
The optimistic post-insert check fires when post_count > 1. If two concurrent POST /auth/setup calls both insert successfully and both then observe count == 2, both will execute persistence.users.delete(user.id) — each deleting its own newly-created record — and both will raise ConflictError. The final user count returns to 0, meaning no admin account survives, even though both callers received a 409.
While the system is technically retryable (count is 0 again), the operator has no indication that retrying will work, and any orchestration layer that interprets a 409 as "already done" will halt incorrectly.
An asymmetric resolution would fix this: after inserting, each request should load all users, determine which user was created first, and only undo if it is the later-created one. This ensures exactly one account persists and exactly one caller receives 201.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/auth/controller.py
Line: 232-238
Comment:
**Race-undo can delete both competing admin accounts**
The optimistic post-insert check fires when `post_count > 1`. If two concurrent `POST /auth/setup` calls both insert successfully and both then observe `count == 2`, **both** will execute `persistence.users.delete(user.id)` — each deleting its own newly-created record — and both will raise `ConflictError`. The final user count returns to 0, meaning no admin account survives, even though both callers received a 409.
While the system is technically retryable (count is 0 again), the operator has no indication that retrying will work, and any orchestration layer that interprets a 409 as "already done" will halt incorrectly.
An asymmetric resolution would fix this: after inserting, each request should load all users, determine which user was created first, and only undo if it is the later-created one. This ensures exactly one account persists and exactly one caller receives 201.
How can I resolve this? If you propose a fix, please make it concise.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 (3)
src/ai_company/api/controllers/approvals.py (1)
230-307: 🛠️ Refactor suggestion | 🟠 MajorExtract the shared decision flow into a helper.
approve()andreject()now duplicate the same lookup, pending-check, audit update, save, publish, and log path, and both exceed the 50-line limit. Pull the shared transition logic into a private helper so future auth/audit fixes only need to land once.♻️ Refactor outline
+ async def _apply_decision( + self, + *, + state: State, + approval_id: str, + request: Request[Any, Any, Any], + target_status: ApprovalStatus, + reason: str | None, + event_type: WsEventType, + log_event: str, + ) -> ApiResponse[ApprovalItem]: + ... + async def approve( self, state: State, approval_id: str, data: ApproveRequest, request: Request[Any, Any, Any], ) -> ApiResponse[ApprovalItem]: - ... - return ApiResponse(data=updated) + return await self._apply_decision( + state=state, + approval_id=approval_id, + request=request, + target_status=ApprovalStatus.APPROVED, + reason=data.comment, + event_type=WsEventType.APPROVAL_APPROVED, + log_event=API_APPROVAL_APPROVED, + )As per coding guidelines, "Keep functions under 50 lines and files under 800 lines".
Also applies to: 314-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/controllers/approvals.py` around lines 230 - 307, Extract the duplicated approve()/reject() transition flow into a private helper (e.g., _transition_approval) that encapsulates: fetching the item via app_state.approval_store.get(approval_id), verify item.status == ApprovalStatus.PENDING (raise ConflictError with the same log API_APPROVAL_CONFLICT), populate audit fields (decided_at, decided_by from request.scope["user"].role.value, decision_reason), save via app_state.approval_store.save (raise NotFoundError with the same API_RESOURCE_NOT_FOUND logging if save returns None), call _publish_approval_event(request, WsEventType..., updated) and write the same logger.info call; have approve() and reject() call this helper with the appropriate ApprovalStatus (APPROVED/REJECTED), WsEventType, and decision_reason (data.comment) so both functions stay under 50 lines and reuse the lookup/persist/publish/log logic.DESIGN_SPEC.md (2)
2985-2997:⚠️ Potential issue | 🟡 MinorThe persistence/auth structure still omits settings storage.
The auth flow now depends on persisting the JWT secret in the
settingstable, but this section only documentsUserRepository/ApiKeyRepositoryand the related SQLite repo file. That leaves the secret-resolution path undocumented and makes the persistence contract incomplete.Based on learnings: If implementation deviates from
DESIGN_SPEC.md, alert the user and explain why — do not silently diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DESIGN_SPEC.md` around lines 2985 - 2997, The DESIGN_SPEC omits persistent settings storage required by the auth flow (JWT secret), so add a SettingsRepository protocol to repositories.py and a concrete SQLiteSettingsRepository in sqlite/repositories.py (or user_repo.py) plus expose it from sqlite/__init__.py; update PersistenceConfig in config.py and the create_backend() factory in factory.py to include the settings backend, and modify sqlite/migrations.py to create a settings table (with a jwt_secret column) and migration steps v1–v5 as needed so the JWT secret resolution path is documented and implemented consistently with UserRepository/ApiKeyRepository.
2565-2578:⚠️ Potential issue | 🟠 MajorDocument the new default auth contract.
Adding
/authand the auth package still leaves the external API contract underspecified. This PR makes authentication mandatory, but the spec does not say that the existing REST routes and/wsare protected by default, which credential types they accept, or how that interacts with the route guards.Based on learnings: If implementation deviates from
DESIGN_SPEC.md, alert the user and explain why — do not silently diverge.Also applies to: 3189-3207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DESIGN_SPEC.md` around lines 2565 - 2578, Update the DESIGN_SPEC.md auth section to define the default authentication contract: state that all existing REST endpoints and the /ws WebSocket are protected by default, list accepted credential types (e.g., Bearer JWT in Authorization header, API-Key header, and optional session cookie), specify token formats and validation rules, and describe how route guards (the auth package and any middleware) enforce protection and error responses (401/403). Also document the WebSocket auth handshake (e.g., initial Authorization header or auth message, token refresh behavior, and connection termination on invalid creds) and note any deliberate deviations from this spec with rationale; ensure the changes also cover the related section referenced (lines 3189-3207).
♻️ Duplicate comments (6)
src/ai_company/api/controllers/approvals.py (1)
239-240:⚠️ Potential issue | 🟠 MajorPersist a stable principal in
decided_by, not the role.
auth_user.role.valuestill collapses every approver with the same role into the same audit identity, and the updated docstrings now document that behavior. Store a stable principal identifier from the authenticated user instead, and fail explicitly if no authenticated user is present rather than letting a missing scope entry surface as a 500.Also applies to: 275-276, 323-324, 359-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/controllers/approvals.py` around lines 239 - 240, The code currently sets the approval audit field decided_by to auth_user.role.value which groups different users by role; change it to persist a stable principal identifier from the authenticated user (e.g., auth_user.id or auth_user.username—use the canonical unique attribute on the auth_user object) wherever decided_by is assigned (replace auth_user.role.value in the create/update approval flows). Also add an explicit check that an authenticated user exists before assigning decided_by and raise a clear authentication/authorization exception (401/403) if absent instead of allowing a missing scope to surface as a 500; update all occurrences that set decided_by (the approval creation/update handlers referenced in the diff) accordingly.tests/unit/api/auth/test_controller.py (1)
38-70:⚠️ Potential issue | 🟠 MajorConvert to async test to avoid
asyncio.get_event_loop()deprecation.The
asyncio.get_event_loop()pattern on lines 60-61 raisesRuntimeErrorin Python 3.14+ when no running loop exists. Sinceasyncio_mode = "auto"is configured, convert this to an async test and await the persistence call directly.Suggested fix
- def test_setup_409_when_users_exist(self, bare_client: TestClient[Any]) -> None: + async def test_setup_409_when_users_exist( + self, bare_client: TestClient[Any] + ) -> None: # Re-seed a user so the check fails - import asyncio import uuid from datetime import UTC, datetime from ai_company.api.auth.models import User from ai_company.api.auth.service import AuthService # noqa: TC001 from ai_company.api.guards import HumanRole app_state = bare_client.app.state["app_state"] svc: AuthService = app_state.auth_service now = datetime.now(UTC) user = User( id=str(uuid.uuid4()), username="existing", password_hash=svc.hash_password("test-password-12chars"), role=HumanRole.CEO, must_change_password=False, created_at=now, updated_at=now, ) - loop = asyncio.get_event_loop() - loop.run_until_complete(app_state.persistence.users.save(user)) + await app_state.persistence.users.save(user) response = bare_client.post(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/auth/test_controller.py` around lines 38 - 70, Convert the synchronous test_setup_409_when_users_exist to an async test function and remove asyncio.get_event_loop()/run_until_complete usage: make the test async def, directly await app_state.persistence.users.save(user) instead of using loop.run_until_complete, and keep the rest of the test logic (using bare_client, creating User via svc.hash_password, and posting to "/api/v1/auth/setup") unchanged so the persistence call executes on the running event loop.tests/unit/api/auth/test_service.py (1)
22-23: 🧹 Nitpick | 🔵 TrivialConsider hoisting
datetimeimports to module level.The inline import pattern works but is unconventional. Moving to module-level would align with standard Python practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/auth/test_service.py` around lines 22 - 23, Hoist the inline import "from datetime import UTC, datetime" to the top-of-file module imports in tests/unit/api/auth/test_service.py so the UTC and datetime symbols are available at module level; remove any duplicate inline imports inside test functions (references to datetime/UTC) and run tests to ensure no import errors.tests/unit/api/conftest.py (1)
633-664:⚠️ Potential issue | 🟠 MajorManual event loop usage contradicts
asyncio_mode="auto".
_seed_test_usersusesasyncio.get_event_loop().run_until_complete()to save users, which conflicts with pytest's async mode configuration. This can cause issues with event loop management.🔧 Proposed fix: Make fixture and helper async
`@pytest.fixture` -def test_client( # noqa: PLR0913 +async def test_client( # noqa: PLR0913 fake_persistence: FakePersistenceBackend, fake_message_bus: FakeMessageBus, cost_tracker: CostTracker, approval_store: ApprovalStore, root_config: RootConfig, auth_service: AuthService, ) -> TestClient[Any]: # Pre-seed users for each role so JWT sub claims resolve - _seed_test_users(fake_persistence, auth_service) + await _seed_test_users(fake_persistence, auth_service) ... -def _seed_test_users( +async def _seed_test_users( backend: FakePersistenceBackend, auth_service: AuthService, ) -> None: ... - import asyncio now = datetime.now(UTC) for role in HumanRole: ... - loop = asyncio.get_event_loop() - loop.run_until_complete(backend.users.save(user)) + await backend.users.save(user)As per coding guidelines, "Use
asyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/conftest.py` around lines 633 - 664, The helper _seed_test_users must not run the event loop manually; make it async (async def _seed_test_users(...)) and replace loop.run_until_complete(backend.users.save(user)) with await backend.users.save(user), updating any fixtures that call _seed_test_users to be async fixtures or await the helper; ensure you still use _get_test_password_hash(...) synchronously or await it if it becomes async so JWT pwd_sig remains correct and tests run under pytest asyncio_mode="auto".src/ai_company/api/auth/controller.py (2)
314-314:⚠️ Potential issue | 🟡 MinorDirect
scope["user"]access risksKeyErrorif middleware is bypassed.Line 314 accesses
request.scope["user"]directly. If the auth middleware is misconfigured or bypassed for this route, this will raise aKeyErrorinstead of a graceful 401.🛡️ Proposed defensive access
- auth_user: AuthenticatedUser = request.scope["user"] + auth_user = request.scope.get("user") + if not isinstance(auth_user, AuthenticatedUser): + raise UnauthorizedError("Authentication required")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/auth/controller.py` at line 314, Replace the direct request.scope["user"] access with a defensive check: use request.scope.get("user") (or request.user if framework provides) to retrieve the principal into auth_user and if it's None raise a 401/Unauthorized HTTPException (or return the same unauthorized response used elsewhere) so the controller (where auth_user is set) fails gracefully when middleware is missing or bypassed; update the code around the auth_user assignment to perform the safe get + conditional unauthorized handling.
377-377:⚠️ Potential issue | 🟡 MinorSame direct
scope["user"]access issue in themeendpoint.Line 377 also accesses
request.scope["user"]directly, which could raiseKeyErrorif the middleware isn't applied.🛡️ Proposed defensive access
- auth_user: AuthenticatedUser = request.scope["user"] + auth_user = request.scope.get("user") + if not isinstance(auth_user, AuthenticatedUser): + raise UnauthorizedError("Authentication required")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/auth/controller.py` at line 377, The me endpoint currently does a direct access auth_user: AuthenticatedUser = request.scope["user"] which can raise KeyError if middleware isn't applied; change this to defensively obtain the user (e.g., user = request.scope.get("user")) and validate it exists, returning an appropriate 401/authorization error or raising HTTPException if user is None, and then cast/assign to AuthenticatedUser (or use a typed check) before proceeding in the me handler; update the symbol auth_user and any downstream usage in the me function to use the guarded value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/persistence/sqlite/user_repo.py`:
- Around line 89-90: Timestamps are being persisted with isoformat() preserving
their original UTC offsets, which can break lexical ordering in SQLite TEXT
columns; update the places writing user.created_at and user.updated_at (e.g., in
the insert/update routines that use user.created_at and user.updated_at) to
normalize datetimes to UTC before serializing (e.g., convert via
astimezone(datetime.timezone.utc) or equivalent) and then call isoformat() so
list_users() and list_by_user() sort chronologically. Ensure the same
UTC-normalization is applied consistently at all write sites referenced in this
file.
---
Outside diff comments:
In `@DESIGN_SPEC.md`:
- Around line 2985-2997: The DESIGN_SPEC omits persistent settings storage
required by the auth flow (JWT secret), so add a SettingsRepository protocol to
repositories.py and a concrete SQLiteSettingsRepository in
sqlite/repositories.py (or user_repo.py) plus expose it from sqlite/__init__.py;
update PersistenceConfig in config.py and the create_backend() factory in
factory.py to include the settings backend, and modify sqlite/migrations.py to
create a settings table (with a jwt_secret column) and migration steps v1–v5 as
needed so the JWT secret resolution path is documented and implemented
consistently with UserRepository/ApiKeyRepository.
- Around line 2565-2578: Update the DESIGN_SPEC.md auth section to define the
default authentication contract: state that all existing REST endpoints and the
/ws WebSocket are protected by default, list accepted credential types (e.g.,
Bearer JWT in Authorization header, API-Key header, and optional session
cookie), specify token formats and validation rules, and describe how route
guards (the auth package and any middleware) enforce protection and error
responses (401/403). Also document the WebSocket auth handshake (e.g., initial
Authorization header or auth message, token refresh behavior, and connection
termination on invalid creds) and note any deliberate deviations from this spec
with rationale; ensure the changes also cover the related section referenced
(lines 3189-3207).
In `@src/ai_company/api/controllers/approvals.py`:
- Around line 230-307: Extract the duplicated approve()/reject() transition flow
into a private helper (e.g., _transition_approval) that encapsulates: fetching
the item via app_state.approval_store.get(approval_id), verify item.status ==
ApprovalStatus.PENDING (raise ConflictError with the same log
API_APPROVAL_CONFLICT), populate audit fields (decided_at, decided_by from
request.scope["user"].role.value, decision_reason), save via
app_state.approval_store.save (raise NotFoundError with the same
API_RESOURCE_NOT_FOUND logging if save returns None), call
_publish_approval_event(request, WsEventType..., updated) and write the same
logger.info call; have approve() and reject() call this helper with the
appropriate ApprovalStatus (APPROVED/REJECTED), WsEventType, and decision_reason
(data.comment) so both functions stay under 50 lines and reuse the
lookup/persist/publish/log logic.
---
Duplicate comments:
In `@src/ai_company/api/auth/controller.py`:
- Line 314: Replace the direct request.scope["user"] access with a defensive
check: use request.scope.get("user") (or request.user if framework provides) to
retrieve the principal into auth_user and if it's None raise a 401/Unauthorized
HTTPException (or return the same unauthorized response used elsewhere) so the
controller (where auth_user is set) fails gracefully when middleware is missing
or bypassed; update the code around the auth_user assignment to perform the safe
get + conditional unauthorized handling.
- Line 377: The me endpoint currently does a direct access auth_user:
AuthenticatedUser = request.scope["user"] which can raise KeyError if middleware
isn't applied; change this to defensively obtain the user (e.g., user =
request.scope.get("user")) and validate it exists, returning an appropriate
401/authorization error or raising HTTPException if user is None, and then
cast/assign to AuthenticatedUser (or use a typed check) before proceeding in the
me handler; update the symbol auth_user and any downstream usage in the me
function to use the guarded value.
In `@src/ai_company/api/controllers/approvals.py`:
- Around line 239-240: The code currently sets the approval audit field
decided_by to auth_user.role.value which groups different users by role; change
it to persist a stable principal identifier from the authenticated user (e.g.,
auth_user.id or auth_user.username—use the canonical unique attribute on the
auth_user object) wherever decided_by is assigned (replace auth_user.role.value
in the create/update approval flows). Also add an explicit check that an
authenticated user exists before assigning decided_by and raise a clear
authentication/authorization exception (401/403) if absent instead of allowing a
missing scope to surface as a 500; update all occurrences that set decided_by
(the approval creation/update handlers referenced in the diff) accordingly.
In `@tests/unit/api/auth/test_controller.py`:
- Around line 38-70: Convert the synchronous test_setup_409_when_users_exist to
an async test function and remove asyncio.get_event_loop()/run_until_complete
usage: make the test async def, directly await
app_state.persistence.users.save(user) instead of using loop.run_until_complete,
and keep the rest of the test logic (using bare_client, creating User via
svc.hash_password, and posting to "/api/v1/auth/setup") unchanged so the
persistence call executes on the running event loop.
In `@tests/unit/api/auth/test_service.py`:
- Around line 22-23: Hoist the inline import "from datetime import UTC,
datetime" to the top-of-file module imports in
tests/unit/api/auth/test_service.py so the UTC and datetime symbols are
available at module level; remove any duplicate inline imports inside test
functions (references to datetime/UTC) and run tests to ensure no import errors.
In `@tests/unit/api/conftest.py`:
- Around line 633-664: The helper _seed_test_users must not run the event loop
manually; make it async (async def _seed_test_users(...)) and replace
loop.run_until_complete(backend.users.save(user)) with await
backend.users.save(user), updating any fixtures that call _seed_test_users to be
async fixtures or await the helper; ensure you still use
_get_test_password_hash(...) synchronously or await it if it becomes async so
JWT pwd_sig remains correct and tests run under pytest asyncio_mode="auto".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 225dc250-79e8-4dff-a724-ece8769309dc
📒 Files selected for processing (19)
DESIGN_SPEC.mdsrc/ai_company/api/app.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/controller.pysrc/ai_company/api/auth/middleware.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/secret.pysrc/ai_company/api/auth/service.pysrc/ai_company/api/config.pysrc/ai_company/api/controllers/approvals.pysrc/ai_company/api/state.pysrc/ai_company/observability/events/persistence.pysrc/ai_company/persistence/protocol.pysrc/ai_company/persistence/sqlite/user_repo.pytests/unit/api/auth/test_config.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_service.pytests/unit/api/conftest.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.14+ with native lazy annotations (PEP 649) — do NOT usefrom __future__ import annotations
Use PEP 758 except syntaxexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions and classes must have type hints; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement of non-Pydantic internal collections
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention and usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves — never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 with@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls) — use structured concurrency over barecreate_task
Maintain maximum line length of 88 characters — enforced by ruff
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly; never silently swallow exceptions
Validate input at system boundaries (user input, external APIs, config files)
Files:
tests/unit/api/auth/test_controller.pysrc/ai_company/api/auth/middleware.pysrc/ai_company/persistence/protocol.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/state.pysrc/ai_company/api/auth/controller.pysrc/ai_company/api/auth/models.pytests/unit/api/auth/test_config.pysrc/ai_company/api/auth/service.pysrc/ai_company/api/auth/secret.pysrc/ai_company/api/controllers/approvals.pytests/unit/api/conftest.pytests/unit/api/auth/test_service.pysrc/ai_company/api/app.pysrc/ai_company/api/config.pytests/unit/api/auth/test_middleware.pysrc/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/observability/events/persistence.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Enforce 80% minimum code coverage — enforced in CI
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set timeout to 30 seconds per test
Usepytest-xdistvia-n autofor parallel test execution
Prefer@pytest.mark.parametrizefor testing similar cases
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned tests — usetest-provider,test-small-001, etc.
Files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_config.pytests/unit/api/conftest.pytests/unit/api/auth/test_service.pytests/unit/api/auth/test_middleware.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Every module with business logic must importfrom ai_company.observability import get_loggerand definelogger = get_logger(__name__); never useimport logging,logging.getLogger(), orprint()in application code
Always use variable namelogger(not_logger, notlog) for the logging instance
Use domain-specific event name constants fromai_company.observability.events.<domain>(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget) — import directly:from ai_company.observability.events.<domain> import EVENT_CONSTANT
Use structured logging with kwargs formatlogger.info(EVENT, key=value)— never use format strings likelogger.info("msg %s", val)
All error paths must log at WARNING or ERROR level with context before raising an exception
All state transitions must log at INFO level
Use DEBUG logging level for object creation, internal flow, and entry/exit of key functions
Pure data models, enums, and re-exports do NOT require logging
All provider calls go throughBaseCompletionProviderwhich automatically applies retry + rate limiting — never implement retry logic in driver subclasses or calling code
ConfigureRetryConfigandRateLimiterConfigper-provider inProviderConfig
Retryable errors are:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError(marked withis_retryable=True); non-retryable errors raise immediately without retry
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples — use generic names:example-provider,example-large-001,example-medium-001,example-small-001, orlarge/medium/smallaliases. Vendor names may only appear in: (1) DESIGN_SPEC.md provider list, (2).claude/skill/agent files, (3) third-party import paths. Tests must usetest-provider,test-small-001, etc.
Files:
src/ai_company/api/auth/middleware.pysrc/ai_company/persistence/protocol.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/state.pysrc/ai_company/api/auth/controller.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/service.pysrc/ai_company/api/auth/secret.pysrc/ai_company/api/controllers/approvals.pysrc/ai_company/api/app.pysrc/ai_company/api/config.pysrc/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/observability/events/persistence.py
🧠 Learnings (10)
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to tests/**/*.py : Use `asyncio_mode = "auto"` in pytest configuration — no manual `pytest.mark.asyncio` needed on async tests
Applied to files:
tests/unit/api/auth/test_controller.pytests/unit/api/conftest.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to **/*.py : Use `NotBlankStr` from `core.types` for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/ai_company/persistence/protocol.pysrc/ai_company/persistence/sqlite/user_repo.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to **/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (with `model_copy(update=...)`) for runtime state that evolves — never mix static config fields with mutable runtime fields in one model
Applied to files:
src/ai_company/api/auth/config.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/config.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : All error paths must log at WARNING or ERROR level with context before raising an exception
Applied to files:
src/ai_company/api/state.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : All state transitions must log at INFO level
Applied to files:
src/ai_company/api/state.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Use DEBUG logging level for object creation, internal flow, and entry/exit of key functions
Applied to files:
src/ai_company/api/state.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: If implementation deviates from `DESIGN_SPEC.md` (better approach found, scope evolved, etc.), alert the user and explain why — user decides whether to proceed or update the spec. Do NOT silently diverge — every deviation needs explicit user approval
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to docker/** : Backend image uses 3-stage Docker build (builder → setup → distroless runtime), Chainguard Python, non-root user (UID 65532), CIS-hardened. Web uses `nginxinc/nginx-unprivileged` with SPA routing and API/WebSocket proxy to backend
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Configure `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`
Applied to files:
src/ai_company/api/config.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to src/ai_company/**/*.py : Use domain-specific event name constants from `ai_company.observability.events.<domain>` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`) — import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/ai_company/observability/events/persistence.py
🧬 Code graph analysis (14)
tests/unit/api/auth/test_controller.py (3)
src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/api/auth/models.py (3)
User(18-39)AuthenticatedUser(68-87)AuthMethod(11-15)src/ai_company/api/auth/controller.py (1)
require_password_changed(148-182)
src/ai_company/api/auth/middleware.py (2)
src/ai_company/api/auth/models.py (2)
AuthenticatedUser(68-87)AuthMethod(11-15)src/ai_company/api/auth/service.py (2)
decode_token(153-174)hash_api_key(177-186)
src/ai_company/persistence/protocol.py (4)
src/ai_company/persistence/repositories.py (2)
ApiKeyRepository(407-475)UserRepository(327-403)tests/unit/api/conftest.py (4)
users(399-400)api_keys(403-404)get_setting(406-407)set_setting(409-410)src/ai_company/persistence/sqlite/backend.py (4)
users(340-346)api_keys(349-355)get_setting(357-380)set_setting(382-407)tests/unit/persistence/test_protocol.py (4)
users(260-261)api_keys(264-265)get_setting(267-268)set_setting(270-271)
src/ai_company/api/state.py (4)
src/ai_company/api/auth/service.py (1)
AuthService(30-195)src/ai_company/api/errors.py (1)
ServiceUnavailableError(68-74)tests/unit/hr/promotion/conftest.py (1)
tracker(104-106)tests/unit/api/conftest.py (1)
auth_service(574-575)
src/ai_company/api/auth/controller.py (8)
src/ai_company/persistence/sqlite/user_repo.py (4)
get(104-133)get(261-290)count(181-192)get_by_username(135-161)src/ai_company/api/auth/models.py (2)
AuthenticatedUser(68-87)User(18-39)src/ai_company/api/dto.py (1)
ApiResponse(31-49)src/ai_company/api/errors.py (2)
ConflictError(41-47)UnauthorizedError(59-65)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/api/state.py (2)
auth_service(98-100)persistence(83-85)src/ai_company/persistence/protocol.py (1)
users(132-134)src/ai_company/persistence/sqlite/backend.py (1)
users(340-346)
src/ai_company/api/auth/models.py (1)
src/ai_company/api/guards.py (1)
HumanRole(18-25)
tests/unit/api/auth/test_config.py (1)
src/ai_company/api/auth/config.py (2)
AuthConfig(28-104)with_secret(91-104)
src/ai_company/api/auth/service.py (3)
src/ai_company/api/auth/models.py (1)
User(18-39)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/api/auth/config.py (1)
AuthConfig(28-104)
src/ai_company/api/auth/secret.py (2)
src/ai_company/persistence/protocol.py (3)
PersistenceBackend(27-167)get_setting(141-153)set_setting(155-167)src/ai_company/persistence/sqlite/backend.py (2)
get_setting(357-380)set_setting(382-407)
tests/unit/api/auth/test_service.py (4)
src/ai_company/api/auth/config.py (1)
AuthConfig(28-104)src/ai_company/api/auth/models.py (1)
User(18-39)src/ai_company/api/auth/service.py (6)
hash_password(40-49)verify_password(51-78)create_token(117-151)decode_token(153-174)hash_api_key(177-186)generate_api_key(189-195)src/ai_company/api/guards.py (1)
HumanRole(18-25)
src/ai_company/api/app.py (6)
src/ai_company/api/auth/controller.py (1)
require_password_changed(148-182)src/ai_company/api/auth/middleware.py (1)
create_auth_middleware_class(252-278)src/ai_company/api/auth/secret.py (1)
resolve_jwt_secret(17-75)src/ai_company/api/state.py (7)
AppState(21-122)persistence(83-85)message_bus(88-90)has_auth_service(103-105)set_auth_service(107-122)cost_tracker(93-95)auth_service(98-100)src/ai_company/persistence/protocol.py (4)
disconnect(58-63)PersistenceBackend(27-167)connect(49-56)migrate(73-79)src/ai_company/api/auth/config.py (1)
with_secret(91-104)
src/ai_company/api/config.py (1)
src/ai_company/api/auth/config.py (1)
AuthConfig(28-104)
tests/unit/api/auth/test_middleware.py (8)
src/ai_company/persistence/sqlite/user_repo.py (6)
get(104-133)get(261-290)save(69-102)save(224-259)delete(194-211)delete(346-363)src/ai_company/api/auth/config.py (1)
AuthConfig(28-104)src/ai_company/api/auth/middleware.py (2)
create_auth_middleware_class(252-278)_extract_bearer_token(101-106)src/ai_company/api/auth/models.py (2)
ApiKey(42-65)User(18-39)src/ai_company/api/auth/service.py (5)
AuthService(30-195)hash_password(40-49)create_token(117-151)generate_api_key(189-195)hash_api_key(177-186)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/api/state.py (2)
auth_service(98-100)persistence(83-85)src/ai_company/persistence/protocol.py (3)
connect(49-56)users(132-134)api_keys(137-139)
src/ai_company/persistence/sqlite/user_repo.py (4)
src/ai_company/api/auth/models.py (2)
ApiKey(42-65)User(18-39)src/ai_company/persistence/errors.py (1)
QueryError(33-34)src/ai_company/persistence/protocol.py (1)
users(132-134)src/ai_company/persistence/sqlite/backend.py (1)
users(340-346)
🪛 ast-grep (0.41.0)
tests/unit/api/auth/test_service.py
[warning] 103-103: Hardcoded JWT secret or private key is used. This is a Insufficiently Protected Credentials weakness: https://cwe.mitre.org/data/definitions/522.html Consider using an appropriate security mechanism to protect the credentials (e.g. keeping secrets in environment variables).
Context: expired_token = jwt.encode(expired_payload, _SECRET, algorithm="HS256")
Note: [CWE-522] Insufficiently Protected Credentials. [REFERENCES]
- https://semgrep.dev/blog/2020/hardcoded-secrets-unverified-tokens-and-other-common-jwt-mistakes/
(jwt-python-hardcoded-secret-python)
🔇 Additional comments (31)
src/ai_company/api/auth/models.py (1)
1-87: LGTM!The authentication domain models are well-structured:
- All models correctly use
ConfigDict(frozen=True)for immutabilityAwareDatetimeis properly used for all timestamp fields, ensuring timezone-aware comparisons in the middlewareNotBlankStris used for all identifier fields as per coding guidelines- Sensitive fields (
password_hash,key_hash) are appropriately hidden from reprtests/unit/api/auth/test_config.py (1)
1-54: LGTM!The tests comprehensively cover
AuthConfigbehavior including defaults, secret validation, immutability, and boundary conditions. The use of@pytest.mark.unitis correct per coding guidelines.src/ai_company/api/config.py (1)
38-41: LGTM!The API config changes properly integrate the new authentication system:
Authorizationheader retained for token-based auth while removing the deprecatedX-Human-RoleheaderAuthConfigis cleanly composed intoApiConfigwith appropriate default factoryAlso applies to: 173-176
src/ai_company/api/auth/middleware.py (3)
1-98: LGTM!The authentication middleware is well-implemented:
- Token routing heuristic (dots → JWT, no dots → API key) correctly distinguishes token types
- Structured logging with proper event constants on all failure paths
- Clean separation between JWT and API key authentication flows
154-164: Good security practice: pwd_sig invalidates tokens after password change.The 16-character SHA-256 prefix of the password hash embedded in the JWT ensures tokens are automatically invalidated when the user changes their password, without requiring token revocation infrastructure.
252-277: LGTM!The dynamic middleware class factory correctly forwards exclude paths to
AbstractAuthenticationMiddleware.__init__, enabling configurable path exclusions without hardcoding.src/ai_company/api/auth/secret.py (2)
48-66: Good: Stored secret validation now implemented.The code now validates that secrets loaded from persistence meet the minimum length requirement, logging a warning and falling through to auto-generation if the stored secret is too short. This provides defense in depth against manually inserted invalid secrets.
1-75: LGTM!The JWT secret resolution chain is well-implemented with proper validation, structured logging at each step, and secure auto-generation using
secrets.token_urlsafe.tests/unit/api/auth/test_service.py (2)
103-104: Static analysis false positive: hardcoded test secret is expected.The static analysis warning about hardcoded JWT secret (CWE-522) is a false positive. Test files necessarily use hardcoded test secrets; this poses no security risk as the secret is only used in isolated unit tests.
37-176: LGTM!Comprehensive test coverage for
AuthService:
- Password hashing with Argon2id verification and edge cases (wrong password, corrupted hash)
- JWT creation/decoding with expiry, signature validation, and claim requirements
- API key hashing determinism and generation uniqueness
tests/unit/api/auth/test_controller.py (1)
223-326: LGTM!The
require_password_changedguard tests comprehensively cover:
- Blocking users with
must_change_password=True- Allowing users without the flag
- Handling missing user in scope
- Path exemptions for
/auth/change-passwordand/auth/me- Rejecting unexpected user types
src/ai_company/api/auth/config.py (2)
73-83: Good:exclude_pathsdefaults toNonefor runtime derivation.The docstring correctly documents that paths are auto-derived from the API prefix at runtime when
None. This addresses the concern about hardcoded/api/v1patterns.
91-104: LGTM!The
with_secretmethod follows the frozen model pattern correctly, usingmodel_copy(update=...)to return a new instance while validating the secret length before copying. Based on learnings: "Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves."src/ai_company/persistence/protocol.py (1)
131-167: LGTM! Protocol extensions are well-defined.The new
usersandapi_keysrepository accessors along withget_setting/set_settingmethods properly useNotBlankStrfor identifier keys. Docstrings follow Google style and include appropriate error documentation.src/ai_company/api/state.py (2)
66-80: LGTM! Centralized service availability check.The
_require_servicehelper properly logs at WARNING level before raisingServiceUnavailableError, providing good observability for missing service configurations.
107-122: LGTM! Double-initialization guard with proper logging.The
set_auth_servicemethod now logs at ERROR level before raisingRuntimeErrorwhen called twice, addressing the previous review feedback about observable error paths.tests/unit/api/auth/test_middleware.py (4)
1-69: LGTM! Well-structured test setup with proper fixtures.The test helpers (
_make_auth_service,_make_user,_build_app) provide clean abstractions for building test scenarios. The_SECRETconstant meets the minimum length requirement.
71-141: LGTM! Comprehensive JWT authentication test coverage.Tests properly cover the happy path and error scenarios: valid JWT, missing header, invalid scheme, invalid JWT, and deleted user. Each test correctly asserts expected HTTP status codes.
143-269: LGTM! Thorough API key authentication tests.Good coverage of API key scenarios including valid keys, revoked keys, expired keys, and orphaned keys (deleted owner). The edge case for deleted owner is particularly valuable for security.
271-313: LGTM! Parametrized bearer token extraction tests.The
@pytest.mark.parametrizedecorator is well-used here to cover various header formats including case variations, empty strings, and wrong schemes.src/ai_company/api/auth/service.py (4)
51-78: LGTM! Robust password verification with appropriate logging.The error handling properly distinguishes between:
VerifyMismatchError: SilentFalsereturn (expected for wrong passwords)VerificationError: WARNING log (indicates potential issues)InvalidHashError: ERROR log (indicates data corruption)This follows the coding guideline to log at WARNING/ERROR before returning on error paths.
117-151: LGTM! JWT creation with password-change invalidation.The
pwd_sigclaim (truncated SHA-256 of password hash) provides automatic token invalidation when passwords change. The empty-secret guard ensures tokens aren't created without proper configuration.
176-195: LGTM! Static utility methods for API key handling.
hash_api_keyuses SHA-256 which is appropriate for API key hashing (fast lookup, not password storage).generate_api_keyusessecrets.token_urlsafe(32)for cryptographically secure key generation.
21-27: Argon2 parameters are compliant with OWASP 2025 recommendations.The configuration (
time_cost=3,memory_cost=65536KiB / 64 MiB,parallelism=4) exceeds OWASP's minimum requirements (memory ≥ 19 MiB,time_cost ≥ 2,parallelism ≥ 1) and aligns with OWASP's equivalent-strength parameter set fort=3while trading additional memory and parallelism for stronger security. No changes needed.src/ai_company/api/auth/controller.py (2)
148-183: LGTM! Guard now handles unexpected user types.The
require_password_changedguard properly logs at WARNING level when encountering an unexpected user type before raisingPermissionDeniedException. This addresses the previous review feedback about defensive handling.
194-258: LGTM! Setup endpoint with race condition protection.The setup flow includes:
- Initial count check to prevent setup after first user exists
- Post-save count check to detect concurrent setup attempts
- Rollback (delete) if race detected
This TOCTOU mitigation is appropriate for the single-admin bootstrap scenario.
tests/unit/api/conftest.py (2)
276-327: LGTM! In-memory user and API key repositories.
FakeUserRepositoryandFakeApiKeyRepositoryproperly implement the repository protocols with dict-based storage. The implementations are straightforward and suitable for testing.
480-563: LGTM! Auth helpers with consistent password hash caching.The password hash caching via
_TEST_PASSWORD_HASHESensures thatmake_auth_headersand_seed_test_usersproduce tokens with matchingpwd_sigclaims, which is essential for middleware user lookups to succeed.src/ai_company/api/app.py (3)
153-199: LGTM! Persistence initialization with auth service resolution.The
_init_persistencefunction properly:
- Connects persistence and runs migrations
- Checks
has_auth_servicebefore resolving JWT secret (addressing previous review feedback)- Uses
logger.exceptionfor proper traceback logging on failures
281-348: LGTM! Application factory with optional auth service injection.The
create_appfunction properly accepts an optionalauth_serviceparameter for testing, stores it inAppState, and passesapp_stateto the lifecycle builder for deferred initialization when not provided.
411-437: LGTM! Middleware stack with authentication first.The auth middleware is correctly placed at the front of the middleware stack (line 434), ensuring authentication runs before other middleware. The default exclude paths cover health, docs, and auth endpoints appropriately.
- Normalize timestamps to UTC before SQLite serialization
- Use defensive scope.get("user") in change_password and me endpoints
- Simplify test fixtures to sync with direct dict assignment
|
|
||
| from pydantic import AwareDatetime # noqa: TC002 | ||
|
|
||
| from ai_company.api.auth.models import ApiKey, User # noqa: TC001 |
There was a problem hiding this comment.
Upward layer dependency: persistence imports from API layer
repositories.py lives in the persistence layer but now imports domain models from ai_company.api.auth.models, which is part of the API layer. This is an inward-to-outward dependency that violates clean architecture: outer layers (API) should depend on inner layers (persistence/domain), never the reverse.
The practical consequences are:
- Any change to the API layer's
User/ApiKeymodels now breaks the persistence layer's protocol definitions. - The persistence layer can no longer be tested or reused without dragging in API-layer dependencies.
Consider relocating User and ApiKey to a shared domain module (e.g., ai_company.core.auth_models or ai_company.persistence.auth_models) so both the API layer and the persistence layer can import from that shared location without creating a circular or upward dependency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/persistence/repositories.py
Line: 11
Comment:
**Upward layer dependency: persistence imports from API layer**
`repositories.py` lives in the persistence layer but now imports domain models from `ai_company.api.auth.models`, which is part of the API layer. This is an inward-to-outward dependency that violates clean architecture: outer layers (API) should depend on inner layers (persistence/domain), never the reverse.
The practical consequences are:
- Any change to the API layer's `User`/`ApiKey` models now breaks the persistence layer's protocol definitions.
- The persistence layer can no longer be tested or reused without dragging in API-layer dependencies.
Consider relocating `User` and `ApiKey` to a shared domain module (e.g., `ai_company.core.auth_models` or `ai_company.persistence.auth_models`) so both the API layer and the persistence layer can import from that shared location without creating a circular or upward dependency.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ai_company/api/auth/controller.py (1)
148-183: 🧹 Nitpick | 🔵 TrivialGuard implementation is sound; consider adding DEBUG log for missing user scope.
The guard correctly handles path exemptions and raises appropriate exceptions. The WARNING log for unexpected user types is a good addition. However, returning silently when
user is None(line 171-172) could mask middleware misconfiguration during debugging.💡 Optional: Add DEBUG log for early return
user = connection.scope.get("user") if user is None: + logger.debug( + "require_password_changed guard skipped: no user in scope", + path=path, + ) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/auth/controller.py` around lines 148 - 183, In require_password_changed, add a debug log when connection.scope.get("user") returns None so middleware misconfiguration is visible during debugging; keep the early return behavior but call logger.debug with context including path (connection.url.path), any relevant scope keys (e.g. list(connection.scope.keys()) or a minimal indicator), and mention _PWD_CHANGE_EXEMPT_SUFFIXES to show the guard was evaluated — do not change the PermissionDeniedException logic or exemption checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/persistence/sqlite/user_repo.py`:
- Line 102: Replace persistence state transition logs that currently call
logger.debug with logger.info: locate the logger.debug(PERSISTENCE_USER_SAVED,
user_id=user.id) occurrence and change it to logger.info; also find the other
logger.debug calls tied to persistence constants (e.g., the lines noted for user
deletes/saves referencing PERSISTENCE_USER_SAVED / PERSISTENCE_USER_DELETED or
similar) and switch those to logger.info so all persisted state transitions are
logged at INFO level (leave message text and structured fields unchanged).
- Around line 66-67: Update the one-line and missing docstrings in this module
to Google-style docstrings for all public classes and functions (e.g., the
UserRepository class and its __init__, plus public methods such as create_user,
get_user, update_user, delete_user, list_users or similarly named methods
referenced in the file); for each docstring include an "Args:" section
(documenting parameter names and types), a "Returns:" section (describe return
type and value), and a "Raises:" section where the method may raise exceptions
(e.g., aiosqlite.Error or ValueError), matching the existing parameter and
return types in the function signatures so ruff D rules pass. Ensure __init__
documents the db parameter and any side effects, and convert all single-line
docstrings into multi-line Google-style blocks for the public API.
---
Duplicate comments:
In `@src/ai_company/api/auth/controller.py`:
- Around line 148-183: In require_password_changed, add a debug log when
connection.scope.get("user") returns None so middleware misconfiguration is
visible during debugging; keep the early return behavior but call logger.debug
with context including path (connection.url.path), any relevant scope keys (e.g.
list(connection.scope.keys()) or a minimal indicator), and mention
_PWD_CHANGE_EXEMPT_SUFFIXES to show the guard was evaluated — do not change the
PermissionDeniedException logic or exemption checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7db22988-813d-4194-bf77-2a1630b030c2
📒 Files selected for processing (4)
src/ai_company/api/auth/controller.pysrc/ai_company/persistence/sqlite/user_repo.pytests/unit/api/auth/test_controller.pytests/unit/api/conftest.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Python 3.14+ with native lazy annotations (PEP 649) — do NOT usefrom __future__ import annotations
Use PEP 758 except syntaxexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions and classes must have type hints; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement of non-Pydantic internal collections
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention and usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves — never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 with@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls) — use structured concurrency over barecreate_task
Maintain maximum line length of 88 characters — enforced by ruff
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly; never silently swallow exceptions
Validate input at system boundaries (user input, external APIs, config files)
Files:
tests/unit/api/conftest.pysrc/ai_company/api/auth/controller.pytests/unit/api/auth/test_controller.pysrc/ai_company/persistence/sqlite/user_repo.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Enforce 80% minimum code coverage — enforced in CI
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set timeout to 30 seconds per test
Usepytest-xdistvia-n autofor parallel test execution
Prefer@pytest.mark.parametrizefor testing similar cases
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned tests — usetest-provider,test-small-001, etc.
Files:
tests/unit/api/conftest.pytests/unit/api/auth/test_controller.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Every module with business logic must importfrom ai_company.observability import get_loggerand definelogger = get_logger(__name__); never useimport logging,logging.getLogger(), orprint()in application code
Always use variable namelogger(not_logger, notlog) for the logging instance
Use domain-specific event name constants fromai_company.observability.events.<domain>(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget) — import directly:from ai_company.observability.events.<domain> import EVENT_CONSTANT
Use structured logging with kwargs formatlogger.info(EVENT, key=value)— never use format strings likelogger.info("msg %s", val)
All error paths must log at WARNING or ERROR level with context before raising an exception
All state transitions must log at INFO level
Use DEBUG logging level for object creation, internal flow, and entry/exit of key functions
Pure data models, enums, and re-exports do NOT require logging
All provider calls go throughBaseCompletionProviderwhich automatically applies retry + rate limiting — never implement retry logic in driver subclasses or calling code
ConfigureRetryConfigandRateLimiterConfigper-provider inProviderConfig
Retryable errors are:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError(marked withis_retryable=True); non-retryable errors raise immediately without retry
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples — use generic names:example-provider,example-large-001,example-medium-001,example-small-001, orlarge/medium/smallaliases. Vendor names may only appear in: (1) DESIGN_SPEC.md provider list, (2).claude/skill/agent files, (3) third-party import paths. Tests must usetest-provider,test-small-001, etc.
Files:
src/ai_company/api/auth/controller.pysrc/ai_company/persistence/sqlite/user_repo.py
🧠 Learnings (2)
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to tests/**/*.py : Use `asyncio_mode = "auto"` in pytest configuration — no manual `pytest.mark.asyncio` needed on async tests
Applied to files:
tests/unit/api/conftest.pytests/unit/api/auth/test_controller.py
📚 Learning: 2026-03-10T23:12:08.513Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:12:08.513Z
Learning: Applies to **/*.py : Use `NotBlankStr` from `core.types` for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/ai_company/persistence/sqlite/user_repo.py
🧬 Code graph analysis (3)
src/ai_company/api/auth/controller.py (7)
src/ai_company/persistence/sqlite/user_repo.py (5)
get(104-133)get(265-294)count(181-192)save(69-102)save(224-263)src/ai_company/api/auth/models.py (2)
AuthenticatedUser(68-87)User(18-39)src/ai_company/api/auth/service.py (4)
AuthService(30-195)hash_password_async(80-93)create_token(117-151)verify_password_async(95-115)src/ai_company/api/dto.py (1)
ApiResponse(31-49)src/ai_company/api/errors.py (2)
ConflictError(41-47)UnauthorizedError(59-65)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/persistence/protocol.py (1)
users(132-134)
tests/unit/api/auth/test_controller.py (5)
src/ai_company/api/guards.py (1)
HumanRole(18-25)tests/unit/api/conftest.py (7)
make_auth_headers(536-562)test_client(608-630)auth_service(574-575)get(51-52)get(214-215)get(285-286)get(313-314)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/ai_company/api/auth/models.py (3)
User(18-39)AuthenticatedUser(68-87)AuthMethod(11-15)src/ai_company/api/auth/controller.py (1)
require_password_changed(148-182)
src/ai_company/persistence/sqlite/user_repo.py (5)
src/ai_company/api/auth/models.py (2)
ApiKey(42-65)User(18-39)src/ai_company/api/guards.py (1)
HumanRole(18-25)src/ai_company/persistence/errors.py (1)
QueryError(33-34)src/ai_company/persistence/repositories.py (7)
save(43-52)save(109-118)save(165-175)save(202-211)save(279-289)save(330-339)count(380-389)src/ai_company/persistence/protocol.py (1)
users(132-134)
🔇 Additional comments (20)
src/ai_company/api/auth/controller.py (6)
1-27: LGTM — Module setup and imports are well-structured.Logger is correctly instantiated using
get_logger(__name__)with theloggervariable name. Imports are appropriate and follow the project conventions.
52-104: LGTM — Request DTOs are well-defined with proper validation.Frozen models with
NotBlankStrfor identifiers, appropriatemax_lengthconstraints, and password length validation viamodel_validator. Clean implementation.
194-258: LGTM — Setup endpoint with reasonable race-condition mitigation.The post-save race guard (lines 232-238) provides reasonable protection against concurrent setup attempts by checking the user count after save and rolling back if needed. Proper logging at INFO level for setup completion.
259-302: LGTM — Login endpoint with secure credential handling.The combined user lookup and password verification (lines 275-277) prevents timing-based user enumeration. Proper structured logging with event constants.
303-370: LGTM — Change password endpoint with defensive scope access.The defensive
request.scope.get("user")pattern with type checking (lines 314-317) properly handles potential middleware misconfiguration, addressing previous review feedback.
371-394: LGTM — Me endpoint with proper defensive access.The defensive scope access pattern (lines 380-383) correctly handles unauthenticated requests.
tests/unit/api/auth/test_controller.py (6)
1-17: LGTM — Test module setup and fixture definition.The
bare_clientfixture properly removes the Authorization header for testing unauthenticated scenarios.
38-72: Test correctly avoids async event loop issues by using direct dict assignment.The direct assignment to
app_state.persistence._users._users[user.id] = user(line 62) is an effective workaround that avoids theasyncio.get_event_loop()pitfalls on Python 3.14+. This addresses the previous review feedback.
84-129: LGTM — Login tests cover success and failure scenarios.Good coverage including valid credentials, wrong password, and nonexistent user cases.
131-204: LGTM — Change password tests cover key scenarios.Good coverage including successful change, wrong current password, missing auth, and validation errors.
206-222: LGTM — Me endpoint tests verify auth requirement and response structure.
224-327: LGTM — Comprehensive guard tests with good parametrization.The
@pytest.mark.parametrizeusage for exempt paths (lines 283-289) follows best practices. Good coverage of all guard branches including the unexpected user type scenario.tests/unit/api/conftest.py (6)
33-38: LGTM — Test auth constants are well-defined.The JWT secret meets the 32-character minimum, and the test user constants are clear.
276-327: LGTM — Fake repositories implement correct async interface.Both
FakeUserRepositoryandFakeApiKeyRepositoryimplement the expected methods with proper async signatures, matching the real repository contracts.
341-411: LGTM — FakePersistenceBackend extended for auth support.The additions for users, API keys, and settings follow the existing pattern in the fake backend.
480-563: LGTM — Auth helpers ensure consistent JWT claims across tests.The password hash caching (
_TEST_PASSWORD_HASHES) and deterministic user ID generation (uuid.uuid5) ensure that tokens produced bymake_auth_headersmatch the seeded users, avoidingpwd_sigmismatches during middleware validation.
633-665: Sync dict assignment is intentional and documented.The
_seed_test_usershelper directly assigns to the fake repository's internal dict (line 664) to avoid async complexities in sync fixtures. This approach is clearly documented in the docstring (lines 645-648) and sidesteps theasyncio.get_event_loop()issues on Python 3.14+. The past review concern is resolved by this design choice.
607-631: LGTM — test_client fixture correctly integrates auth service.The fixture wires up the auth service and seeds test users before creating the app. Default CEO auth headers ensure most tests have write access out of the box.
src/ai_company/persistence/sqlite/user_repo.py (2)
89-90: UTC normalization on write looks correct.Normalizing to UTC before
isoformat()keeps theORDER BY created_atTEXT queries chronologically stable across mixed offsets.Also applies to: 245-249
122-131: Good job preserving the repository error contract.Catching row-deserialization failures and re-raising
QueryErrorkeeps malformed rows from leakingValueError/ValidationErrorthrough read APIs.Also applies to: 152-161, 172-177, 283-292, 310-315, 333-342
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 51 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if persistence is not None: | ||
| try: | ||
| await persistence.connect() | ||
| except Exception: | ||
| logger.error( | ||
| API_APP_STARTUP, | ||
| error="Failed to connect persistence", | ||
| exc_info=True, | ||
| ) | ||
| raise | ||
| await _init_persistence(persistence, app_state) | ||
| started_persistence = True |
There was a problem hiding this comment.
started_persistence is only set to True after _init_persistence() fully succeeds. If connect() succeeds but migrate() or JWT secret resolution fails inside _init_persistence(), _safe_startup() will run cleanup with started_persistence=False, leaving the DB connection open. Consider setting started_persistence=True immediately after a successful connect() (or have _init_persistence() return whether it connected) so failures after connect still trigger disconnect() in _cleanup_on_failure().
| # For 5xx errors return the generic class-level default to avoid | ||
| # leaking internals. For 4xx client errors return the actual | ||
| # exception message — it was set by the controller and is user-safe. | ||
| if exc.status_code >= _SERVER_ERROR_THRESHOLD: | ||
| msg = type(exc).default_message | ||
| else: | ||
| msg = str(exc) or type(exc).default_message | ||
| return Response( |
There was a problem hiding this comment.
handle_api_error() now returns str(exc) for all 4xx ApiErrors. Many controllers raise NotFoundError(f"Task {task_id!r} not found") / similar, so this will expose internal identifiers in responses, which the previous behavior explicitly avoided. Consider only passing through details for a small allow-list (e.g., UnauthorizedError, ApiValidationError) or adding an explicit flag on ApiError (e.g., expose_message) and defaulting to default_message otherwise.
| user_count = await persistence.users.count() | ||
| if user_count > 0: | ||
| logger.warning(API_AUTH_FAILED, reason="setup_already_completed") | ||
| msg = "Setup already completed" | ||
| raise ConflictError(msg) | ||
|
|
||
| now = datetime.now(UTC) | ||
| password_hash = await auth_service.hash_password_async(data.password) | ||
| user = User( | ||
| id=str(uuid.uuid4()), | ||
| username=data.username, | ||
| password_hash=password_hash, | ||
| role=HumanRole.CEO, | ||
| must_change_password=True, | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| await persistence.users.save(user) | ||
|
|
||
| # Race guard: undo if another setup completed concurrently | ||
| post_count = await persistence.users.count() | ||
| if post_count > 1: | ||
| await persistence.users.delete(user.id) | ||
| logger.warning(API_AUTH_FAILED, reason="setup_race_detected") | ||
| msg = "Setup already completed" | ||
| raise ConflictError(msg) |
There was a problem hiding this comment.
The setup “race guard” can delete both concurrently-created users: if two setup requests run in parallel, both can observe count()==0, both insert, both see post_count>1, and both delete their own user, leaving zero users and allowing setup to be repeated. Consider making setup atomic (e.g., BEGIN IMMEDIATE transaction + re-check, or a dedicated singleton/lock row in settings, or an insert constrained by a unique sentinel) so only one request can succeed without risking a full rollback of all created users.
| user_count = await persistence.users.count() | |
| if user_count > 0: | |
| logger.warning(API_AUTH_FAILED, reason="setup_already_completed") | |
| msg = "Setup already completed" | |
| raise ConflictError(msg) | |
| now = datetime.now(UTC) | |
| password_hash = await auth_service.hash_password_async(data.password) | |
| user = User( | |
| id=str(uuid.uuid4()), | |
| username=data.username, | |
| password_hash=password_hash, | |
| role=HumanRole.CEO, | |
| must_change_password=True, | |
| created_at=now, | |
| updated_at=now, | |
| ) | |
| await persistence.users.save(user) | |
| # Race guard: undo if another setup completed concurrently | |
| post_count = await persistence.users.count() | |
| if post_count > 1: | |
| await persistence.users.delete(user.id) | |
| logger.warning(API_AUTH_FAILED, reason="setup_race_detected") | |
| msg = "Setup already completed" | |
| raise ConflictError(msg) | |
| async with persistence.transaction(): | |
| user_count = await persistence.users.count() | |
| if user_count > 0: | |
| logger.warning(API_AUTH_FAILED, reason="setup_already_completed") | |
| msg = "Setup already completed" | |
| raise ConflictError(msg) | |
| now = datetime.now(UTC) | |
| password_hash = await auth_service.hash_password_async(data.password) | |
| user = User( | |
| id=str(uuid.uuid4()), | |
| username=data.username, | |
| password_hash=password_hash, | |
| role=HumanRole.CEO, | |
| must_change_password=True, | |
| created_at=now, | |
| updated_at=now, | |
| ) | |
| await persistence.users.save(user) | |
| # Race guard: if another setup completed concurrently, abort this transaction | |
| post_count = await persistence.users.count() | |
| if post_count > 1: | |
| logger.warning(API_AUTH_FAILED, reason="setup_race_detected") | |
| msg = "Setup already completed" | |
| raise ConflictError(msg) |
| key_hash TEXT NOT NULL UNIQUE, | ||
| name TEXT NOT NULL, | ||
| role TEXT NOT NULL, | ||
| user_id TEXT NOT NULL REFERENCES users(id), |
There was a problem hiding this comment.
api_keys.user_id is declared with REFERENCES users(id), but SQLite does not enforce foreign key constraints unless PRAGMA foreign_keys=ON is enabled per connection. Either enable foreign keys during DB connection setup (recommended) or drop the FK clause to avoid a false sense of integrity.
| user_id TEXT NOT NULL REFERENCES users(id), | |
| user_id TEXT NOT NULL, |
| raise ConflictError(msg) | ||
|
|
||
| role = request.headers.get("x-human-role", "unknown") | ||
| auth_user = request.scope["user"] |
There was a problem hiding this comment.
request.scope["user"] can raise KeyError (or later AttributeError) if authentication middleware is excluded/misconfigured or a different auth backend sets a different user type. Using request.scope.get("user") and validating it’s an AuthenticatedUser (or at least has a role: HumanRole) would avoid turning an auth/config issue into a 500.
| auth_user = request.scope["user"] | |
| auth_user = request.scope.get("user") | |
| if auth_user is None or not hasattr(auth_user, "role"): | |
| msg = "Authenticated user with a role is required to approve items" | |
| logger.error( | |
| API_APPROVAL_CONFLICT, | |
| approval_id=approval_id, | |
| error=msg, | |
| ) | |
| raise RuntimeError(msg) |
| auth_user = request.scope["user"] | ||
| role = auth_user.role.value |
There was a problem hiding this comment.
Same as above: directly indexing request.scope["user"] assumes auth middleware always populated it with the expected type. Prefer get() + type/attribute validation and raise a 401/403 rather than allowing a KeyError/AttributeError to bubble to a 500.
- Mitigate timing side-channel in login (dummy argon2 verification)
- Derive _MIN_PASSWORD_LENGTH from AuthConfig default
- Remove redundant unique indexes (users.username, api_keys.key_hash)
- Defensive scope.get("user") in approvals controller
- Record username (not role) in decided_by field
- Extract _resolve_decision helper in approvals (DRY)
- Fix started_persistence cleanup on post-connect failure
- Enable PRAGMA foreign_keys=ON in SQLite backend
- Log user/api-key saves and deletes at INFO (state transitions)
- Hoist imports in test_service.py
- Parametrize v5 schema tests
- Expand user_repo docstrings to Google style
Summary
AbstractAuthenticationMiddlewaresubclass) supporting both JWT and API key in a single request pipeline/auth/setup,/auth/login,/auth/change-password,/auth/meendpointsAI_COMPANY_JWT_SECRET) → persistence (settings table) → auto-generate + persistrequire_password_changed) blocking operations until initial password is changedaudit_entriestable + indexes; v5 migration addssettings,users,api_keystables + indexes (SCHEMA_VERSION bumped to 5)UnauthorizedError(401),ApiValidationError(422) with detail passthroughauth_serviceaccessor andset_auth_service()for deferred auth initUserRepository,ApiKeyRepositorywith SQLite implementationsTest plan
with_secret()factory (unit)Review coverage
Pre-reviewed by 10 agents, 25 findings addressed:
Closes #256