feat: add system user for CLI-to-backend authentication#710
Conversation
The CLI (synthorg backup, synthorg wipe) calls the backend's admin backup API with a JWT, but the backend's auth middleware requires the JWT subject to map to a real database user. Previously the CLI used sub="synthorg-cli" which didn't exist, causing all CLI backup operations to fail with "Authentication required". Add a persistent "system" user bootstrapped at app startup: - New HumanRole.SYSTEM with write+read access - Random Argon2id password hash (nobody knows the plaintext) - Cannot log in, change password, or appear in user lists - CLI JWT pwd_sig validation skipped (random hash, never changes) - Setup endpoint uses count_by_role(CEO) to ignore system user - CLI JWT sub changed from "synthorg-cli" to "system" - Removed aud claim from CLI JWT (backend doesn't validate it, PyJWT rejects tokens with unvalidated audience) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace private _hasher import with AuthService.hash_password - Wrap ensure_system_user in try/except in lifecycle startup - Add iss=synthorg-cli validation for SYSTEM role in middleware - Add missing WARNING logs on change_password and me auth failures - Add SYSTEM role to Human Roles table in operations design page - Document system user / CLI token flow in docs/security.md - Add regression test: non-system user without pwd_sig still rejected - Add Go test assertion that aud claim is absent - Update system_user.py docstring for upsert-on-race semantics Pre-reviewed by 11 agents, 11 findings addressed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
WalkthroughAdds a persistent internal "system" user (id/username "system") bootstrapped at startup with an Argon2id-hashed random password. Introduces SYSTEM role and constants; middleware bypasses Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 'system' user to facilitate secure CLI-to-backend authentication. It addresses a previous authentication failure by creating a dedicated user with restricted privileges and adjusting the JWT configuration. The changes enhance the system's security posture and ensure proper authentication for CLI operations. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/api/auth/test_controller.py`:
- Around line 423-459: test_change_password_rejects_system_user assumes the
system user exists which can make the test flaky; before building the JWT seed
the system user into the test DB (similar to test_login_rejects_system_user /
test_setup_succeeds_with_system_user_present) so the auth middleware accepts the
token and the controller returns 403. In practice, in
test_change_password_rejects_system_user use bare_client.app.state["app_state"]
(app_state) and the same user creation helper used by the other tests to insert
synthorg.api.auth.system_user.SYSTEM_USER_ID (or call the
app_state.user_service/system user create method) prior to creating the JWT and
posting to /api/v1/auth/change-password.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc44726b-9047-4899-8f15-28f977d0e3b9
📒 Files selected for processing (16)
cli/cmd/backup.gocli/cmd/backup_test.godocs/design/operations.mddocs/security.mdsrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/system_user.pysrc/synthorg/api/guards.pysrc/synthorg/api/lifecycle.pysrc/synthorg/observability/events/api.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/user_repo.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_system_user.pytests/unit/api/fakes.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). (7)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: CLI Test (macos-latest)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotationsin Python code -- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling on Python 3.14 -- enforced by ruff
Line length must be 88 characters -- enforced by ruff
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/lifecycle.pytests/unit/api/auth/test_middleware.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/repositories.pytests/unit/api/fakes.pysrc/synthorg/api/guards.pytests/unit/api/auth/test_controller.pysrc/synthorg/api/auth/middleware.pytests/unit/api/auth/test_system_user.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints and Google-style docstrings -- enforced by ruff D rules and mypy strict mode
Use immutability patterns: create new objects instead of mutating existing ones. For non-Pydantic collections, usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldinstead of storing redundant fields; useNotBlankStrfor all identifier/name fields instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Functions must be < 50 lines, files < 800 lines
All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO
Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport loggingorprint()in application code
Use event name constants fromsynthorg.observability.events.<domain>modules instead of string literals (e.g.,API_REQUEST_STARTEDfromevents.api)
Always uselogger.info(EVENT, key=value)for structured logging -- never uselogger.info('msg %s', val)format strings
Validate input at system boundaries (user input, external APIs, config files) rather than throughout application code
Handle errors explicitly and never silently swallow them
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples -- use generic names likeexample-provider,example-large-001,test-provider,test-small-001
Usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization) for Pydantic models with dict/list fields
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/lifecycle.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/repositories.pysrc/synthorg/api/guards.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Cobra framework for CLI commands and charmbracelet libraries (huh, lipgloss) for interactive UI
Files:
cli/cmd/backup.gocli/cmd/backup_test.go
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use native
testing.Ffuzz functions for Go fuzzing (e.g.,FuzzYamlStr)
Files:
cli/cmd/backup_test.go
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All public API responses must follow RFC 9457 error format
Files:
src/synthorg/api/lifecycle.pysrc/synthorg/api/guards.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowmarkers for tests
Use Hypothesis for property-based testing in Python with@given+@settingsdecorators. HYPOTHESIS_PROFILE env var controls examples (ci=50, dev=1000)
Never skip, dismiss, or ignore flaky tests -- always fix them fully. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()instead of widening margins. Useasyncio.Event().wait()for indefinite blocking instead ofasyncio.sleep(large_number)
Prefer@pytest.mark.parametrizefor testing similar cases instead of writing multiple test functions
Files:
tests/unit/api/auth/test_middleware.pytests/unit/api/fakes.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_system_user.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
📚 Learning: 2026-03-22T08:19:13.388Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T08:19:13.388Z
Learning: Applies to src/**/*.py : Use event name constants from `synthorg.observability.events.<domain>` modules instead of string literals (e.g., `API_REQUEST_STARTED` from `events.api`)
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under ai_company.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.) — import directly
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/lifecycle.pytests/unit/api/auth/test_middleware.pydocs/security.mdsrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/security/**/*.py : Security package (security/): SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume)
Applied to files:
src/synthorg/api/guards.pydocs/security.md
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/auth/middleware.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
src/synthorg/api/auth/system_user.py
🧬 Code graph analysis (8)
src/synthorg/api/lifecycle.py (3)
src/synthorg/api/auth/system_user.py (1)
ensure_system_user(41-88)src/synthorg/api/state.py (2)
persistence(165-167)auth_service(185-187)tests/unit/providers/management/conftest.py (1)
app_state(68-83)
tests/unit/api/auth/test_middleware.py (5)
tests/unit/api/fakes.py (10)
FakePersistenceBackend(412-515)connect(435-436)users(488-489)get(42-43)get(212-213)get(286-287)get(317-318)get(378-379)get(401-402)get(524-525)tests/unit/persistence/test_protocol.py (2)
connect(289-290)users(342-343)src/synthorg/api/auth/models.py (1)
User(19-40)src/synthorg/api/guards.py (1)
HumanRole(18-26)src/synthorg/api/auth/middleware.py (1)
_resolve_jwt_user(171-244)
src/synthorg/persistence/sqlite/user_repo.py (2)
src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-51)
src/synthorg/persistence/repositories.py (3)
src/synthorg/persistence/sqlite/user_repo.py (1)
count(253-274)tests/unit/api/fakes.py (1)
count(298-299)tests/unit/persistence/test_protocol.py (1)
count(194-195)
tests/unit/api/fakes.py (3)
src/synthorg/persistence/repositories.py (1)
count(389-398)src/synthorg/persistence/sqlite/user_repo.py (1)
count(253-274)tests/unit/persistence/test_protocol.py (1)
count(194-195)
tests/unit/api/auth/test_controller.py (5)
tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/auth/models.py (1)
User(19-40)tests/unit/api/conftest.py (1)
auth_service(161-162)src/synthorg/api/state.py (2)
auth_service(185-187)persistence(165-167)src/synthorg/api/guards.py (1)
HumanRole(18-26)
tests/unit/api/auth/test_system_user.py (4)
src/synthorg/api/auth/system_user.py (2)
ensure_system_user(41-88)is_system_user(29-38)src/synthorg/api/guards.py (1)
HumanRole(18-26)tests/unit/api/fakes.py (12)
FakePersistenceBackend(412-515)connect(435-436)users(488-489)get(42-43)get(212-213)get(286-287)get(317-318)get(378-379)get(401-402)get(524-525)count(298-299)list_users(295-296)tests/unit/api/auth/test_middleware.py (1)
_make_auth_service(18-19)
src/synthorg/api/auth/controller.py (4)
src/synthorg/api/auth/system_user.py (1)
is_system_user(29-38)src/synthorg/persistence/sqlite/user_repo.py (1)
count_by_role(276-308)src/synthorg/api/guards.py (1)
HumanRole(18-26)src/synthorg/api/errors.py (1)
UnauthorizedError(211-219)
🔇 Additional comments (20)
docs/security.md (1)
70-77: LGTM!The security documentation accurately captures the new system user identity and its authentication behavior. The description of JWT claims (
sub: "system",iss: "synthorg-cli"), thepwd_sigbypass, and the operational restrictions (cannot log in, change password, or be modified) aligns with the implementation changes across the codebase.docs/design/operations.md (1)
1156-1156: LGTM!The Human Roles table update correctly documents the new System role with appropriate access level (write, internal only) and operational constraints (cannot log in, be deleted, or be modified). This aligns with the implementation in
guards.pywhereHumanRole.SYSTEMis added to_WRITE_ROLES.src/synthorg/api/guards.py (1)
26-36: LGTM!The
HumanRole.SYSTEMenum value and its inclusion in_WRITE_ROLEScorrectly grants write access to the system user for CLI-to-backend operations. Since_READ_ROLESis derived from_WRITE_ROLES, the system user automatically gets read access as well. The docstring update on line 65 maintains consistency with the implementation.src/synthorg/observability/events/api.py (1)
44-44: LGTM!The new event constant follows the established
"<domain>.<noun>.<verb>"naming convention and is correctly typed asFinal[str]. This event will be used for structured logging when the system user is bootstrapped during application startup.src/synthorg/persistence/repositories.py (1)
378-398: LGTM!The protocol docstrings correctly document that
list_users()andcount()operate on human users only, excluding the system user. This aligns with the concrete implementations inSQLiteUserRepositoryandFakeUserRepository. Theget()method remains unchanged, allowing retrieval of the system user by ID when needed.tests/unit/api/fakes.py (1)
295-302: LGTM!The fake repository correctly mirrors the production
SQLiteUserRepositorybehavior by excludingHumanRole.SYSTEMusers fromlist_users()andcount(). Thecount_by_role()method appropriately remains unchanged, allowing explicit role-specific counts (includingSYSTEMwhen requested). This ensures test fakes accurately reflect production semantics.src/synthorg/persistence/sqlite/user_repo.py (2)
222-251: LGTM!The
list_users()implementation correctly excludes the system user via parameterized SQL query while preserving thecreated_atordering. The docstring accurately documents the behavior and provides guidance on how to retrieve the system user when needed (viaget()with the system user ID).
253-274: LGTM!The
count()implementation correctly excludes the system user using a parameterized query. This ensurescount()returns the number of human users, which is the expected behavior for user management operations.cli/cmd/backup.go (1)
172-186: LGTM!The JWT payload correctly uses
sub: "system"to match the system user identity in the database, whileiss: "synthorg-cli"allows the backend middleware to verify the token originated from the CLI. The removal of theaudclaim (mentioned in PR objectives) avoidsInvalidAudienceErrorfrom PyJWT. The 60-second expiry and HMAC-SHA256 signing provide appropriate security for local CLI-to-backend authentication.cli/cmd/backup_test.go (1)
192-200: LGTM!The test correctly validates the updated JWT claim structure:
sub:"system"instead of the old"synthorg-cli", presence ofiss:"synthorg-cli", and explicitly asserts the absence of theaudclaim. The negative assertion on line 198-200 is good defensive testing to prevent regressions.src/synthorg/api/lifecycle.py (1)
203-210: LGTM!The system user bootstrap is correctly positioned after
_init_persistence(which ensuresauth_serviceis available) and follows the established error-handling pattern: log with exception context, then re-raise to trigger reverse cleanup. The existing_cleanup_on_failuremechanism will properly disconnect persistence if this step fails.tests/unit/api/auth/test_middleware.py (1)
389-513: LGTM!Comprehensive test coverage for the system user JWT authentication:
test_system_user_jwt_without_pwd_sig_authenticates: Validates CLI-style JWTs withsub=systemandiss=synthorg-cliare accepted withoutpwd_sig.test_system_user_resolves_to_system_role: Directly tests_resolve_jwt_userto confirm correct role assignment.test_non_system_user_without_pwd_sig_returns_401: Critical regression test ensuring regular users cannot bypasspwd_sigvalidation.src/synthorg/api/auth/middleware.py (1)
200-229: LGTM!The conditional validation logic is correctly structured:
- User existence is verified first (lines 190-198), preventing bypass attempts with non-existent system user IDs.
- For
HumanRole.SYSTEM, theiss="synthorg-cli"check provides defense-in-depth—even a valid JWT withsub=systemrequires the correct issuer claim.- Regular users continue to require
pwd_sigvalidation, maintaining the existing security posture.The
issclaim is protected by the JWT signature, so it cannot be forged.tests/unit/api/auth/test_controller.py (1)
461-496: LGTM!This test correctly validates that the
/auth/setupendpoint succeeds when only the system user exists, confirming thecount_by_role(CEO)change in the controller. The explicit user clearing and system user seeding makes this test self-contained.tests/unit/api/auth/test_system_user.py (1)
1-116: LGTM!Comprehensive test coverage for the system user module:
TestIsSystemUser: Validates the simple identity check with positive, negative, and empty cases.TestEnsureSystemUser: Covers creation, role assignment, Argon2id hash format,must_change_password=False, idempotency (same hash on repeated calls), and exclusion fromcount()andlist_users().The tests effectively validate both the
system_user.pyimplementation and the corresponding fake repository filtering behavior.src/synthorg/api/auth/controller.py (3)
282-283: LGTM!Switching from
count()tocount_by_role(HumanRole.CEO)correctly ensures the system user (which has roleSYSTEM, notCEO) doesn't block the first-run setup. The same fix in the race guard maintains consistency.Also applies to: 302-303
345-358: LGTM!System user login rejection is implemented with constant-time protection: the
_DUMMY_ARGON2_HASHverification runs even for system users, preventing timing-based enumeration. The explicitpassword_valid = Falseensures the system user can never authenticate via/auth/login.
400-415: LGTM!The system user password change block is correctly implemented:
- Logging before rejection (lines 408-412) satisfies the "log before raise" guideline.
PermissionDeniedException(403) is appropriate since the user is authenticated but lacks permission for this action.- The
is_system_user(auth_user.user_id)check uses the trusted user ID from the validated JWT.src/synthorg/api/auth/system_user.py (2)
41-88: LGTM!The implementation is sound:
- The random password (
os.urandom(64).hex()) provides 256 bits of entropy, making brute-force infeasible.- The check-then-save pattern has a benign race condition (as documented in lines 47-51): concurrent startups may regenerate the hash, but this is harmless since CLI tokens skip
pwd_sigvalidation.- Structured logging with event constants follows project conventions.
One minor note: line 71 uses the synchronous
hash_passwordmethod. If this causes noticeable startup latency, consider usinghash_password_async, though the impact is likely negligible for a single hash operation.
29-38: LGTM!Simple, focused helper function with proper type hints and Google-style docstring.
There was a problem hiding this comment.
Code Review
This pull request introduces a 'system' user role for internal CLI-to-backend authentication. The system user is bootstrapped at startup, cannot be logged into, deleted, or modified, and its JWTs skip password-change detection. Changes include modifications to JWT handling, user authentication, and database queries to exclude the system user from standard user lists and counts. New tests were added to verify the system user's behavior.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
- Coverage 92.34% 92.32% -0.02%
==========================================
Files 571 572 +1
Lines 29406 29475 +69
Branches 2845 2855 +10
==========================================
+ Hits 27156 27214 +58
- Misses 1777 1786 +9
- Partials 473 475 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Security:
- Remove SYSTEM from _WRITE_ROLES (least privilege); add dedicated
require_system_or_write_access guard scoped to backup controller
- Re-add aud claim to CLI JWT with backend validation in middleware
- Add delete guard for system user at repository layer
- Block system user from requesting WebSocket tickets
- Reject reserved "system" username in setup endpoint
Correctness:
- Use async hash_password_async in ensure_system_user (was blocking
event loop with sync Argon2id)
- Extract SYSTEM_ISSUER and SYSTEM_AUDIENCE constants (cross-language
with Go CLI); add cross-reference comments
- Fix middleware docstring ("never changes" was incorrect)
- Fix ensure_system_user docstring TOCTOU description
- Fix User.id docstring (said "UUID" but system user uses "system")
- Add index on users.role column (used by count_by_role)
- Disable verify_aud in PyJWT decode (validated manually per-role)
Frontend:
- Add 'system' variant to HumanRole TypeScript type
Tests (9 new):
- Wrong issuer, wrong audience, pwd_sig present (middleware)
- SQLite list_users/count exclusion, delete rejection (user_repo)
- Setup with non-CEO humans, reserved username rejection (controller)
- ensure_system_user error propagation (system_user)
- Refactor test_system_user.py to use fixtures
- Use _TEST_JWT_SECRET constant in controller tests
Docs:
- Update operations.md SYSTEM role description to "backup/wipe only"
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- test_ws: SYSTEM excluded from _READ_ROLES (scoped to backup only) - test_migrations: add idx_users_role to expected indexes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/api/auth/service.py (1)
194-213: 🧹 Nitpick | 🔵 TrivialConsider documenting that audience validation is deferred to middleware.
The
verify_aud=Falseoption disables PyJWT's audience verification here, with validation now handled per-role in the auth middleware. The docstring doesn't mention this delegation, which could be helpful for future maintainers.📝 Suggested docstring addition
def decode_token(self, token: str) -> dict[str, Any]: """Decode and validate a JWT. + + Note: Audience (``aud``) is not validated here; the auth + middleware validates audience per-role (system vs human). Args: token: Encoded JWT string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/auth/service.py` around lines 194 - 213, The decode_token docstring should note that audience (aud) verification is intentionally disabled here (verify_aud=False) because audience validation is performed later in the per-role auth middleware; update the docstring for decode_token to explicitly state that audience validation is deferred to the auth middleware and not performed by jwt.decode, referencing verify_aud=False and the auth middleware's per-role checks so future maintainers understand the delegation.src/synthorg/api/controllers/backup.py (1)
1-4: 🧹 Nitpick | 🔵 TrivialUpdate docstrings to reflect system role access.
The module and class docstrings state "All endpoints require write access," but the guard now also permits the
SYSTEMrole (which is not a human write role). Consider updating for accuracy.📝 Suggested docstring updates
-"""Backup controller -- admin endpoints for backup/restore operations. - -All endpoints require write access. -""" +"""Backup controller -- admin endpoints for backup/restore operations. + +All endpoints require write access or system role (CLI). +"""class BackupController(Controller): """Admin endpoints for backup and restore operations. - All endpoints require write access. + All endpoints require write access or system role (CLI). """Also applies to: 43-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/backup.py` around lines 1 - 4, The module-level docstring and the BackupController class docstring still state "All endpoints require write access" but the guard now also permits the SYSTEM role; update both docstrings (module top-level and the BackupController class docstring) to accurately reflect allowed access (e.g., "All endpoints require write access or the SYSTEM role") and adjust any wording to clarify that SYSTEM is a non-human system role rather than a human write role.
🤖 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/synthorg/api/auth/system_user.py`:
- Around line 78-84: The debug log for the "already_exists" branch omits the
user_id; update the logger.debug call in the block that checks existing = await
persistence.users.get(SYSTEM_USER_ID) to include the user_id (e.g.,
user_id=SYSTEM_USER_ID) alongside action="already_exists" when calling
logger.debug(API_AUTH_SYSTEM_USER_ENSURED, ...), matching the info log used on
creation.
In `@tests/unit/api/auth/test_system_user.py`:
- Around line 35-44: The three nearly identical tests in TestIsSystemUser should
be consolidated into one parametrized test: replace the three methods
(test_true_for_system_id, test_false_for_other_id, test_false_for_empty) with a
single test method (e.g., test_is_system_user) decorated with
`@pytest.mark.parametrize` that iterates over tuples of (input_id, expected_bool)
including (SYSTEM_USER_ID, True), ("some-other-id", False), and ("", False), and
assert is_system_user(input_id) == expected_bool; this keeps the test class and
uses the existing symbols is_system_user and SYSTEM_USER_ID for clarity.
- Around line 49-84: Multiple tests repeatedly call ensure_system_user and fetch
the same user; consolidate into a single test that calls ensure_system_user
once, retrieves the user via fake_persistence.users.get(SYSTEM_USER_ID), and
asserts all attributes (user.id == SYSTEM_USER_ID, user.username ==
SYSTEM_USERNAME, user.role == HumanRole.SYSTEM,
user.password_hash.startswith("$argon2id$"), and user.must_change_password is
False) to reduce runtime; update or remove the four existing tests
(test_creates_user_on_empty_db, test_system_user_has_system_role,
test_system_user_has_argon2id_hash, test_system_user_must_change_password_false)
and replace with one combined test that references ensure_system_user,
fake_persistence.users.get, SYSTEM_USER_ID, SYSTEM_USERNAME, and
HumanRole.SYSTEM.
In `@tests/unit/api/fakes.py`:
- Around line 304-308: The fake repository's delete method checks user_id ==
"system" directly; update tests/unit/api/fakes.py in async def delete(self,
user_id: str) to use the same production check by calling
is_system_user(user_id) (or comparing to the shared SYSTEM_USER_ID constant)
instead of the hardcoded string so behavior stays consistent with
SQLiteUserRepository.delete and won't diverge if the system ID changes.
---
Outside diff comments:
In `@src/synthorg/api/auth/service.py`:
- Around line 194-213: The decode_token docstring should note that audience
(aud) verification is intentionally disabled here (verify_aud=False) because
audience validation is performed later in the per-role auth middleware; update
the docstring for decode_token to explicitly state that audience validation is
deferred to the auth middleware and not performed by jwt.decode, referencing
verify_aud=False and the auth middleware's per-role checks so future maintainers
understand the delegation.
In `@src/synthorg/api/controllers/backup.py`:
- Around line 1-4: The module-level docstring and the BackupController class
docstring still state "All endpoints require write access" but the guard now
also permits the SYSTEM role; update both docstrings (module top-level and the
BackupController class docstring) to accurately reflect allowed access (e.g.,
"All endpoints require write access or the SYSTEM role") and adjust any wording
to clarify that SYSTEM is a non-human system role rather than a human write
role.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b7dda06-ae5e-409c-9070-3b0334481dc7
📒 Files selected for processing (20)
cli/cmd/backup.gocli/cmd/backup_test.godocs/design/operations.mdsrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/auth/system_user.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/guards.pysrc/synthorg/persistence/sqlite/schema.sqlsrc/synthorg/persistence/sqlite/user_repo.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_system_user.pytests/unit/api/controllers/test_ws.pytests/unit/api/fakes.pytests/unit/persistence/sqlite/test_migrations.pytests/unit/persistence/sqlite/test_user_repo.pyweb/src/api/types.ts
📜 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). (6)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotationsin Python code -- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling on Python 3.14 -- enforced by ruff
Line length must be 88 characters -- enforced by ruff
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/auth/service.pytests/unit/persistence/sqlite/test_migrations.pysrc/synthorg/api/controllers/backup.pytests/unit/api/controllers/test_ws.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/api/fakes.pysrc/synthorg/api/auth/middleware.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pysrc/synthorg/api/guards.pysrc/synthorg/persistence/sqlite/user_repo.pytests/unit/api/auth/test_system_user.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints and Google-style docstrings -- enforced by ruff D rules and mypy strict mode
Use immutability patterns: create new objects instead of mutating existing ones. For non-Pydantic collections, usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldinstead of storing redundant fields; useNotBlankStrfor all identifier/name fields instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Functions must be < 50 lines, files < 800 lines
All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO
Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport loggingorprint()in application code
Use event name constants fromsynthorg.observability.events.<domain>modules instead of string literals (e.g.,API_REQUEST_STARTEDfromevents.api)
Always uselogger.info(EVENT, key=value)for structured logging -- never uselogger.info('msg %s', val)format strings
Validate input at system boundaries (user input, external APIs, config files) rather than throughout application code
Handle errors explicitly and never silently swallow them
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples -- use generic names likeexample-provider,example-large-001,test-provider,test-small-001
Usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization) for Pydantic models with dict/list fields
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/guards.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All public API responses must follow RFC 9457 error format
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/guards.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/system_user.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowmarkers for tests
Use Hypothesis for property-based testing in Python with@given+@settingsdecorators. HYPOTHESIS_PROFILE env var controls examples (ci=50, dev=1000)
Never skip, dismiss, or ignore flaky tests -- always fix them fully. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()instead of widening margins. Useasyncio.Event().wait()for indefinite blocking instead ofasyncio.sleep(large_number)
Prefer@pytest.mark.parametrizefor testing similar cases instead of writing multiple test functions
Files:
tests/unit/persistence/sqlite/test_migrations.pytests/unit/api/controllers/test_ws.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/api/fakes.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_system_user.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Cobra framework for CLI commands and charmbracelet libraries (huh, lipgloss) for interactive UI
Files:
cli/cmd/backup.gocli/cmd/backup_test.go
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use native
testing.Ffuzz functions for Go fuzzing (e.g.,FuzzYamlStr)
Files:
cli/cmd/backup_test.go
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Vue 3 components must use TypeScript with type safety in the dashboard
Files:
web/src/api/types.ts
web/src/**/*.{ts,tsx,vue,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for Vue linting in the dashboard and run lint checks via
npm --prefix web run lint
Files:
web/src/api/types.ts
🧠 Learnings (10)
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Applied to files:
src/synthorg/api/controllers/backup.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/api/**/*.py : Use Litestar for REST + WebSocket API. Controllers, guards, channels, JWT + API key + WS ticket auth, RFC 9457 structured errors.
Applied to files:
src/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/auth/middleware.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_middleware.pysrc/synthorg/api/guards.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/security/**/*.py : Security package (security/): SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume)
Applied to files:
src/synthorg/api/auth/middleware.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/security/**/*.py : Security module includes SecOps agent, rule engine (soft-allow/hard-deny), audit log, output scanner, risk classifier, autonomy levels (4 strategies), timeout policies.
Applied to files:
src/synthorg/api/auth/middleware.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Security: SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies: disabled/weighted/per-category/milestone), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume).
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/persistence/sqlite/user_repo.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
src/synthorg/api/auth/system_user.py
🧬 Code graph analysis (11)
src/synthorg/api/controllers/backup.py (1)
src/synthorg/api/guards.py (1)
require_system_or_write_access(94-118)
tests/unit/api/controllers/test_ws.py (1)
web/src/api/types.ts (1)
HumanRole(46-52)
tests/unit/persistence/sqlite/test_user_repo.py (2)
src/synthorg/persistence/sqlite/user_repo.py (5)
SQLiteUserRepository(87-350)list_users(223-252)count(254-275)delete(311-350)delete(531-558)src/synthorg/persistence/repositories.py (2)
list_users(378-387)count(389-398)
tests/unit/api/fakes.py (3)
src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-52)src/synthorg/persistence/sqlite/user_repo.py (4)
count(254-275)count_by_role(277-309)delete(311-350)delete(531-558)
src/synthorg/api/auth/middleware.py (2)
src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-52)
tests/unit/api/auth/test_controller.py (7)
tests/unit/api/conftest.py (2)
make_auth_headers(123-149)auth_service(161-162)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/auth/models.py (1)
User(19-40)src/synthorg/api/state.py (2)
auth_service(185-187)persistence(165-167)src/synthorg/api/auth/service.py (1)
hash_password(69-78)src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-52)
tests/unit/api/auth/test_middleware.py (5)
tests/unit/api/auth/test_system_user.py (1)
_make_auth_service(19-20)tests/unit/api/fakes.py (10)
FakePersistenceBackend(415-518)connect(438-439)users(491-492)get(42-43)get(212-213)get(286-287)get(320-321)get(381-382)get(404-405)get(527-528)src/synthorg/api/auth/service.py (1)
hash_password(69-78)src/synthorg/api/guards.py (1)
HumanRole(18-26)src/synthorg/api/auth/middleware.py (1)
_resolve_jwt_user(172-254)
src/synthorg/persistence/sqlite/user_repo.py (3)
src/synthorg/api/auth/system_user.py (1)
is_system_user(42-51)src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-52)
tests/unit/api/auth/test_system_user.py (3)
src/synthorg/api/auth/system_user.py (2)
ensure_system_user(54-105)is_system_user(42-51)src/synthorg/api/guards.py (1)
HumanRole(18-26)tests/unit/api/fakes.py (25)
FakePersistenceBackend(415-518)connect(438-439)users(491-492)get(42-43)get(212-213)get(286-287)get(320-321)get(381-382)get(404-405)get(527-528)count(298-299)list_users(295-296)save(39-40)save(71-72)save(103-107)save(134-135)save(160-161)save(186-187)save(209-210)save(234-238)save(283-284)save(317-318)save(342-343)save(378-379)save(401-402)
src/synthorg/api/auth/controller.py (7)
src/synthorg/api/auth/system_user.py (1)
is_system_user(42-51)src/synthorg/api/errors.py (2)
ConflictError(189-197)UnauthorizedError(211-219)src/synthorg/persistence/sqlite/user_repo.py (1)
count_by_role(277-309)src/synthorg/persistence/repositories.py (1)
count_by_role(400-412)src/synthorg/api/guards.py (1)
HumanRole(18-26)web/src/api/types.ts (1)
HumanRole(46-52)src/synthorg/api/auth/service.py (1)
verify_password_async(130-150)
src/synthorg/api/auth/system_user.py (6)
src/synthorg/api/auth/models.py (1)
User(19-40)src/synthorg/api/guards.py (1)
HumanRole(18-26)src/synthorg/api/auth/service.py (2)
AuthService(36-245)hash_password_async(115-128)src/synthorg/persistence/protocol.py (1)
PersistenceBackend(31-195)tests/unit/api/fakes.py (21)
users(491-492)get(42-43)get(212-213)get(286-287)get(320-321)get(381-382)get(404-405)get(527-528)save(39-40)save(71-72)save(103-107)save(134-135)save(160-161)save(186-187)save(209-210)save(234-238)save(283-284)save(317-318)save(342-343)save(378-379)save(401-402)src/synthorg/persistence/sqlite/user_repo.py (4)
get(144-183)get(416-455)save(102-142)save(368-414)
🔇 Additional comments (27)
web/src/api/types.ts (1)
46-53: HumanRole update is correct and aligned with backend contract.Adding
'system'to theHumanRoleunion keeps frontend DTO typing in sync with the new backend role while preserving strict compile-time type safety.src/synthorg/api/auth/models.py (1)
23-23: LGTM!The docstring update correctly reflects that user IDs are no longer exclusively UUIDs, accommodating the fixed "system" identifier for the system user.
tests/unit/persistence/sqlite/test_migrations.py (1)
53-53: LGTM!The expected index set correctly includes
idx_users_roleto match the schema addition.cli/cmd/backup.go (1)
178-180: LGTM!The JWT
subclaim is correctly changed to"system"to map to the backend's system user, whileissremains"synthorg-cli"for middleware identification of CLI-originated tokens.src/synthorg/persistence/sqlite/schema.sql (1)
185-186: LGTM!The index on
users(role)appropriately supports role-based queries likecount_by_role(CEO)and system user exclusion filters introduced in this PR.tests/unit/api/controllers/test_ws.py (1)
273-283: LGTM!The test comprehensively verifies that
HumanRole.SYSTEMis excluded from WebSocket access while all other roles remain permitted. The docstring clearly explains the security rationale.cli/cmd/backup_test.go (1)
187-220: LGTM!The test improvements are excellent:
- JSON parsing is more robust than substring matching
- All expected claims (
sub,iss,aud,iat,exp) are properly validated- The 60-second expiry window check ensures token lifetime is correct
- Cross-language sync comment aids maintainability
tests/unit/persistence/sqlite/test_user_repo.py (1)
97-125: LGTM! Good coverage of system user exclusion semantics.The tests properly verify that:
list_users()filters outHumanRole.SYSTEMuserscount()excludes system users from the totaldelete()rejects deletion of the system user with the expected error messageMinor observation: Line 123 uses
pytest.raises(Exception, ...)which works but could useQueryErrordirectly for stronger type documentation (matching the implementation inuser_repo.pyline 334). This is non-blocking since thematch=pattern validates the behavior.src/synthorg/api/auth/middleware.py (1)
201-239: LGTM! Sound security design for system user JWT handling.The implementation correctly:
- Skips
pwd_sigvalidation for system users (since the random hash is never known)- Requires both
iss=SYSTEM_ISSUERandaud=SYSTEM_AUDIENCEto constrain which tokens may bypasspwd_sig- Maintains the existing
pwd_sigvalidation for all other roles- Logs appropriate warnings with context on rejection paths
This provides defense-in-depth by ensuring only CLI-originated tokens (with the correct issuer/audience) can authenticate as the system user without password signature verification.
docs/design/operations.md (1)
1156-1156: LGTM! Documentation accurately reflects implementation.The System role entry correctly documents:
- Scoped write access to backup/restore/wipe endpoints (aligns with
require_system_or_write_accessguard)- Cannot log in, be deleted, or be modified (aligns with controller and repository guards)
- Bootstrapped at startup (aligns with
ensure_system_userbehavior)tests/unit/api/auth/test_middleware.py (1)
389-645: LGTM! Comprehensive test coverage for system user JWT authentication.The test suite thoroughly covers:
- Positive path: System user JWT without
pwd_sigauthenticates whenissandaudare correct- Role resolution: Confirms middleware resolves to
HumanRole.SYSTEM- Issuer validation: Wrong
issrejected with 401- Audience validation: Wrong
audrejected with 401- Ignored pwd_sig: Stale
pwd_sigin system token doesn't cause rejection- Non-system regression: Regular users without
pwd_sigstill rejectedThis provides strong confidence in the authentication bypass constraints.
tests/unit/api/auth/test_controller.py (1)
382-561: LGTM! Well-structured tests for system user blocking behavior.The test suite covers critical security behaviors:
- Login rejection: System user login returns 401 (cannot authenticate via password)
- Password change rejection: System user gets 403 on
/change-password- Setup bypass: Setup endpoint allows first-run when only system user exists
- Reserved username: Setup rejects "system" username with 409
- CEO-only check: Setup succeeds with non-CEO users (OBSERVER) present
The explicit seeding of the system user in each test ensures isolation and addresses the previous review feedback about test fragility.
src/synthorg/api/guards.py (1)
26-26: LGTM! Well-designed least-privilege guard system.The implementation correctly:
- Adds
HumanRole.SYSTEMto the role enum- Creates
_SYSTEM_WRITE_ROLESas a superset of_WRITE_ROLESfor scoped access- Excludes
SYSTEMfrom general_WRITE_ROLESto enforce least privilege- Provides
require_system_or_write_accessfor endpoints the CLI needs (backup/wipe)- Updates
require_write_accessdocstring to direct callers to the appropriate guardThis ensures the system user can only access explicitly authorized endpoints while preventing accidental privilege escalation to other write endpoints.
Also applies to: 39-44, 94-119
src/synthorg/persistence/sqlite/user_repo.py (3)
223-252: LGTM! Proper exclusion of system user from user listings.The query correctly filters with
WHERE role != ?bound toHumanRole.SYSTEM.value, ensuring the internal CLI identity is not exposed in user management APIs. The docstring clearly documents this behavior and directs callers to useget()with the system user ID if needed.
254-275: LGTM! Count exclusion aligns with list behavior.The
count()method uses the sameWHERE role != ?filter aslist_users(), ensuring consistency between the two methods and preventing the system user from affecting user count metrics.
311-350: LGTM! Robust deletion guard for system user.The implementation:
- Uses
is_system_user()for the check (consistent with the helper function)- Logs at WARNING level before raising (per coding guidelines)
- Raises
QueryErrorwith a clear message- Performs the check before any database operation (fail-fast)
tests/unit/api/auth/test_system_user.py (1)
117-131: LGTM -- error propagation test is correct.The test validates that persistence errors propagate properly. The manual cleanup on line 131 is technically not needed since fixtures create a fresh
FakePersistenceBackendper test, but keeping it for clarity is fine.src/synthorg/api/auth/controller.py (6)
282-295: LGTM -- reserved username guard and CEO-based check are correct.The reserved username check prevents collision with the system user, and switching to
count_by_role(HumanRole.CEO)ensures the system user's presence doesn't block first-run setup. Logging at WARNING before raising follows guidelines.
310-316: LGTM -- race guard correctly uses CEO-only count.The race guard is now consistent with the pre-check, ensuring system user presence doesn't trigger false positives.
354-367: LGTM -- constant-time rejection for system user login.The system user login block correctly uses the dummy hash verification to prevent timing-based enumeration, matching the behavior for non-existent users.
416-424: LGTM -- system user modification correctly blocked.The change-password endpoint properly denies system users with appropriate logging before raising
PermissionDeniedException.
488-495: LGTM -- improved observability for auth failures.The added warning log with path context aids debugging.
533-541: LGTM -- WebSocket ticket request correctly blocked for system user.Blocking system users from obtaining WebSocket tickets is the right security posture since CLI operations don't need WebSocket access.
src/synthorg/api/auth/system_user.py (4)
1-22: LGTM -- module setup follows guidelines.The module docstring clearly explains the system user's purpose and constraints. Logger setup and TYPE_CHECKING pattern are correct.
42-51: LGTM -- simple and correct implementation.The function has proper type hints and a Google-style docstring.
86-105: LGTM -- secure password generation and proper async handling.Using
os.urandom(64).hex()provides 512 bits of entropy. The async password hashing correctly avoids blocking the event loop. Error logging is handled at the persistence layer, so no duplication is needed here.
24-39: LGTM -- constants are well-documented with verified cross-language sync to Go CLI.The
Final[str]typing ensures immutability. Cross-language synchronization to Go CLI is confirmed:cli/cmd/backup.go:179constructs the JWT payload with matching values (sub="system",iss="synthorg-cli",aud="synthorg-backend"), validated by tests inbackup_test.go.
- system_user.py: add user_id to debug log in already_exists branch - test_system_user.py: parametrize TestIsSystemUser, consolidate 4 attribute tests into one combined test - fakes.py: use is_system_user() instead of hardcoded "system" string - service.py: document verify_aud=False delegation to auth middleware - backup.py: update module and class docstrings for SYSTEM role access Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/synthorg/api/auth/system_user.py`:
- Around line 54-106: The function ensure_system_user is too long and lacks
explicit failure-path logging and should be split: extract the User construction
into a helper (e.g., build_system_user or create_system_user_obj) that returns
the prepared User given password_hash and timestamp, and shorten
ensure_system_user to orchestrate existence check, password generation/hash,
call the helper, save, and logging; also add an explicit except block around
persistence.users.save (and around hash_password_async if desired) that logs a
WARNING or ERROR with context (use API_AUTH_SYSTEM_USER_ENSURED,
action="create_failed", user_id=SYSTEM_USER_ID and include exception detail)
before re-raising so all error paths are logged and state transitions remain at
INFO.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdf26a31-d6c1-4d53-8175-6ba87cbba956
📒 Files selected for processing (5)
src/synthorg/api/auth/service.pysrc/synthorg/api/auth/system_user.pysrc/synthorg/api/controllers/backup.pytests/unit/api/auth/test_system_user.pytests/unit/api/fakes.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). (7)
- GitHub Check: Test (Python 3.14)
- GitHub Check: CLI Test (macos-latest)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotationsin Python code -- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling on Python 3.14 -- enforced by ruff
Line length must be 88 characters -- enforced by ruff
Files:
src/synthorg/api/auth/service.pysrc/synthorg/api/controllers/backup.pytests/unit/api/fakes.pytests/unit/api/auth/test_system_user.pysrc/synthorg/api/auth/system_user.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints and Google-style docstrings -- enforced by ruff D rules and mypy strict mode
Use immutability patterns: create new objects instead of mutating existing ones. For non-Pydantic collections, usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldinstead of storing redundant fields; useNotBlankStrfor all identifier/name fields instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Functions must be < 50 lines, files < 800 lines
All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO
Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport loggingorprint()in application code
Use event name constants fromsynthorg.observability.events.<domain>modules instead of string literals (e.g.,API_REQUEST_STARTEDfromevents.api)
Always uselogger.info(EVENT, key=value)for structured logging -- never uselogger.info('msg %s', val)format strings
Validate input at system boundaries (user input, external APIs, config files) rather than throughout application code
Handle errors explicitly and never silently swallow them
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples -- use generic names likeexample-provider,example-large-001,test-provider,test-small-001
Usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization) for Pydantic models with dict/list fields
Files:
src/synthorg/api/auth/service.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/system_user.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All public API responses must follow RFC 9457 error format
Files:
src/synthorg/api/auth/service.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/auth/system_user.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowmarkers for tests
Use Hypothesis for property-based testing in Python with@given+@settingsdecorators. HYPOTHESIS_PROFILE env var controls examples (ci=50, dev=1000)
Never skip, dismiss, or ignore flaky tests -- always fix them fully. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()instead of widening margins. Useasyncio.Event().wait()for indefinite blocking instead ofasyncio.sleep(large_number)
Prefer@pytest.mark.parametrizefor testing similar cases instead of writing multiple test functions
Files:
tests/unit/api/fakes.pytests/unit/api/auth/test_system_user.py
🧠 Learnings (9)
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/backup.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Applied to files:
src/synthorg/api/controllers/backup.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/backup.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/api/**/*.py : Use Litestar for REST + WebSocket API. Controllers, guards, channels, JWT + API key + WS ticket auth, RFC 9457 structured errors.
Applied to files:
src/synthorg/api/controllers/backup.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
tests/unit/api/fakes.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/fakes.pysrc/synthorg/api/auth/system_user.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/auth/test_system_user.py
📚 Learning: 2026-03-22T08:19:13.388Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T08:19:13.388Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases instead of writing multiple test functions
Applied to files:
tests/unit/api/auth/test_system_user.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/auth/test_system_user.py
🧬 Code graph analysis (3)
tests/unit/api/fakes.py (3)
src/synthorg/api/auth/system_user.py (1)
is_system_user(42-51)src/synthorg/api/guards.py (1)
HumanRole(18-26)src/synthorg/persistence/sqlite/user_repo.py (3)
count(254-275)delete(311-350)delete(531-558)
tests/unit/api/auth/test_system_user.py (4)
src/synthorg/api/auth/service.py (1)
AuthService(36-251)src/synthorg/api/auth/system_user.py (2)
ensure_system_user(54-106)is_system_user(42-51)src/synthorg/api/guards.py (1)
HumanRole(18-26)tests/unit/api/fakes.py (24)
FakePersistenceBackend(416-519)users(492-493)get(43-44)get(213-214)get(287-288)get(321-322)get(382-383)get(405-406)get(528-529)count(299-300)list_users(296-297)save(40-41)save(72-73)save(104-108)save(135-136)save(161-162)save(187-188)save(210-211)save(235-239)save(284-285)save(318-319)save(343-344)save(379-380)save(402-403)
src/synthorg/api/auth/system_user.py (3)
src/synthorg/api/auth/service.py (2)
AuthService(36-251)hash_password_async(115-128)src/synthorg/persistence/protocol.py (1)
PersistenceBackend(31-195)src/synthorg/persistence/sqlite/user_repo.py (4)
get(144-183)get(416-455)save(102-142)save(368-414)
🔇 Additional comments (5)
src/synthorg/api/auth/service.py (1)
197-201: Audience-check delegation is implemented consistently.Docstring and decode options are aligned, and the change cleanly centralizes audience enforcement outside the decode helper.
Also applies to: 218-218
src/synthorg/api/controllers/backup.py (1)
17-17: Guard update is coherent with the SYSTEM-role auth flow.Switching to
require_system_or_write_accessis a clean, targeted authorization change for CLI-backed operations.Also applies to: 54-54
tests/unit/api/fakes.py (1)
8-8: Fake repository behavior now matches system-user semantics.Excluding SYSTEM users from human-facing list/count paths and protecting delete via
is_system_user()is the right alignment for test fidelity.Also applies to: 297-300, 306-308
tests/unit/api/auth/test_system_user.py (2)
37-46: Good use of parametrization for identity checks.The table-driven
is_system_userassertions are concise and complete for the core truth table.
51-109: System-user bootstrap tests are comprehensive and pragmatic.This set covers creation, invariants, idempotency, repository-visibility rules, and failure propagation with clear scope.
| async def ensure_system_user( | ||
| persistence: PersistenceBackend, | ||
| auth_service: AuthService, | ||
| ) -> None: | ||
| """Create the system user if it does not already exist. | ||
|
|
||
| Safe to call on every startup. If the user already exists, | ||
| returns immediately. Under concurrent startup a narrow TOCTOU | ||
| window exists between the existence check and the save; the | ||
| underlying persistence backend's upsert semantics ensure this | ||
| is harmless -- the last writer wins, regenerating the password | ||
| hash. This is safe because CLI tokens authenticate via the | ||
| shared JWT HMAC signature and skip ``pwd_sig`` validation | ||
| (enforced in ``_resolve_jwt_user``). | ||
|
|
||
| The system user receives a random Argon2id password hash | ||
| generated from a 128-character hex string derived from 64 | ||
| bytes of ``os.urandom``. Nobody knows the plaintext, | ||
| preventing login via ``/auth/login``. | ||
|
|
||
| Args: | ||
| persistence: Connected persistence backend. | ||
| auth_service: Authentication service for password hashing. | ||
| """ | ||
| existing = await persistence.users.get(SYSTEM_USER_ID) | ||
| if existing is not None: | ||
| logger.debug( | ||
| API_AUTH_SYSTEM_USER_ENSURED, | ||
| action="already_exists", | ||
| user_id=SYSTEM_USER_ID, | ||
| ) | ||
| return | ||
|
|
||
| # Generate a random password nobody will ever know. | ||
| random_password = os.urandom(64).hex() | ||
| password_hash = await auth_service.hash_password_async(random_password) | ||
|
|
||
| now = datetime.now(UTC) | ||
| user = User( | ||
| id=SYSTEM_USER_ID, | ||
| username=SYSTEM_USERNAME, | ||
| password_hash=password_hash, | ||
| role=HumanRole.SYSTEM, | ||
| must_change_password=False, | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| await persistence.users.save(user) | ||
| logger.info( | ||
| API_AUTH_SYSTEM_USER_ENSURED, | ||
| action="created", | ||
| user_id=SYSTEM_USER_ID, | ||
| ) |
There was a problem hiding this comment.
Add explicit failure-path logging and split ensure_system_user into smaller units.
ensure_system_user currently exceeds the function-length rule and propagates bootstrap failures without module-level WARNING/ERROR context. Please add a logged failure branch and extract user construction into a helper.
♻️ Proposed refactor
-from synthorg.observability.events.api import API_AUTH_SYSTEM_USER_ENSURED
+from synthorg.observability.events.api import (
+ API_AUTH_FAILED,
+ API_AUTH_SYSTEM_USER_ENSURED,
+)
@@
+def _build_system_user(*, password_hash: str, now: datetime) -> User:
+ """Build the persisted system user model."""
+ return User(
+ id=SYSTEM_USER_ID,
+ username=SYSTEM_USERNAME,
+ password_hash=password_hash,
+ role=HumanRole.SYSTEM,
+ must_change_password=False,
+ created_at=now,
+ updated_at=now,
+ )
+
+
async def ensure_system_user(
@@
- # Generate a random password nobody will ever know.
- random_password = os.urandom(64).hex()
- password_hash = await auth_service.hash_password_async(random_password)
-
- now = datetime.now(UTC)
- user = User(
- id=SYSTEM_USER_ID,
- username=SYSTEM_USERNAME,
- password_hash=password_hash,
- role=HumanRole.SYSTEM,
- must_change_password=False,
- created_at=now,
- updated_at=now,
- )
- await persistence.users.save(user)
+ try:
+ random_password = os.urandom(64).hex()
+ password_hash = await auth_service.hash_password_async(random_password)
+ now = datetime.now(UTC)
+ user = _build_system_user(password_hash=password_hash, now=now)
+ await persistence.users.save(user)
+ except Exception:
+ logger.error(
+ API_AUTH_FAILED,
+ reason="system_user_bootstrap_failed",
+ user_id=SYSTEM_USER_ID,
+ exc_info=True,
+ )
+ raise
logger.info(
API_AUTH_SYSTEM_USER_ENSURED,
action="created",
user_id=SYSTEM_USER_ID,
)As per coding guidelines: "Functions must be < 50 lines, files < 800 lines" and "All error paths must log at WARNING or ERROR with context before raising; all state transitions must log at INFO".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/auth/system_user.py` around lines 54 - 106, The function
ensure_system_user is too long and lacks explicit failure-path logging and
should be split: extract the User construction into a helper (e.g.,
build_system_user or create_system_user_obj) that returns the prepared User
given password_hash and timestamp, and shorten ensure_system_user to orchestrate
existence check, password generation/hash, call the helper, save, and logging;
also add an explicit except block around persistence.users.save (and around
hash_password_async if desired) that logs a WARNING or ERROR with context (use
API_AUTH_SYSTEM_USER_ENSURED, action="create_failed", user_id=SYSTEM_USER_ID and
include exception detail) before re-raising so all error paths are logged and
state transitions remain at INFO.
🤖 I have created a release *beep* *boop* --- ## [0.4.7](v0.4.6...v0.4.7) (2026-03-22) ### Features * add system user for CLI-to-backend authentication ([#710](#710)) ([dc6bd3f](dc6bd3f)) * dev channel builds with incremental pre-releases between stable releases ([#715](#715)) ([0e8a714](0e8a714)) * replace hardcoded name pools with Faker multi-locale name generation ([#714](#714)) ([5edc6ec](5edc6ec)) ### Bug Fixes * dev-release tag creation, dependabot coverage, go -C cli convention ([#730](#730)) ([7634843](7634843)) * improve name generation step UX and fix sentinel expansion bug ([#739](#739)) ([f03fd05](f03fd05)) * settings page UX polish -- toggle bug, source badges, form improvements ([#712](#712)) ([d16a0ac](d16a0ac)) * switch dev tags to semver and use same release pipeline as stable ([#729](#729)) ([4df6b9b](4df6b9b)), closes [#713](#713) * unify CLI image discovery and standardize Go tooling ([#738](#738)) ([712a785](712a785)) * use PAT in dev-release workflow to trigger downstream pipelines ([#716](#716)) ([d767aa3](d767aa3)) ### CI/CD * bump astral-sh/setup-uv from 7.4.0 to 7.6.0 in /.github/actions/setup-python-uv in the minor-and-patch group ([#731](#731)) ([7887257](7887257)) * bump the minor-and-patch group with 3 updates ([#735](#735)) ([7cd253a](7cd253a)) * bump wrangler from 4.75.0 to 4.76.0 in /.github in the minor-and-patch group ([#732](#732)) ([a6cafc7](a6cafc7)) * clean up all dev releases and tags on stable release ([#737](#737)) ([8d90f5c](8d90f5c)) ### Maintenance * bump the minor-and-patch group across 2 directories with 2 updates ([#733](#733)) ([2b60069](2b60069)) * bump the minor-and-patch group with 3 updates ([#734](#734)) ([859bc25](859bc25)) * fix dependabot labels and add scope tags ([#736](#736)) ([677eb15](677eb15)) * remove redundant pytest.mark.timeout(30) markers ([#740](#740)) ([9ec2163](9ec2163)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
subchanged from"synthorg-cli"to"system"(maps to real DB user)audclaim from CLI JWT (backend'sjwt.decodedoesn't validate audience, causing PyJWTInvalidAudienceError)count_by_role(CEO)instead ofcount()so system user doesn't block first-run setuppwd_sigfor SYSTEM role (validatesiss=synthorg-cliinstead)Why
synthorg backupandsynthorg wipewere broken -- the CLI generated JWTs withsub: "synthorg-cli"but the backend's auth middleware requiressubto map to a real database user. All CLI backup operations failed with "Authentication required".Test plan
🤖 Generated with Claude Code