fix: harden setup wizard completion and status checks#616
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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 several critical enhancements to the first-run setup wizard, making it more robust and secure. The changes ensure that the system is correctly initialized by enforcing stricter prerequisites for setup completion, such as the presence of a CEO-role user, a company name, and at least one agent. Additionally, it improves the user experience by dynamically providing the minimum password length from the backend to the frontend, preventing hardcoded values and ensuring consistency. 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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #616 +/- ##
==========================================
- Coverage 92.45% 92.42% -0.04%
==========================================
Files 551 551
Lines 27686 27745 +59
Branches 2679 2681 +2
==========================================
+ Hits 25598 25644 +46
- Misses 1635 1650 +15
+ Partials 453 451 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR tightens first-run setup validation and surfaces backend password policy. 🚥 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. 📝 Coding Plan
Comment |
Three setup wizard hardening fixes from PR #584 review: - #588: needs_admin now checks for CEO role explicitly via new count_by_role(HumanRole) method on UserRepository protocol, SQLite implementation, and fake test implementation - #587: complete_setup verifies company name and at least one agent exist (via settings) before marking setup complete, in addition to the existing provider check - #589: SetupStatusResponse now includes min_password_length from backend settings; SetupAdmin.vue reads it from the setup store instead of the hardcoded constant Closes #587 Closes #588 Closes #589 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-reviewed by 4 agents, 2 findings addressed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Derive _DEFAULT_MIN_PASSWORD_LENGTH from AuthConfig (no duplicate constant) - Separate ValueError/SettingNotFoundError/generic exception handling with proper log levels for min_password_length retrieval - Add Field(ge=8) constraint to SetupStatusResponse.min_password_length - Add _check_setup_not_complete guard to complete_setup (consistency) - Add distinct SETUP_STATUS_SETTINGS_DEFAULT_USED event constant - Fix needs_admin docstring to say "CEO" not "admin" - Add debug log for SettingNotFoundError in company check - Add Math.max clamp in frontend minPasswordLength computed - Add SQLite integration tests for count_by_role - Add test for company-missing rejection path in complete_setup - Add test for non-default configured min_password_length - Add try/finally restore for fake state mutation in tests - Replace fragile cast(object()) stub with MagicMock(spec=...) - Add min_password_length to SetupPage.test.ts mock - Add setup store unit tests (minPasswordLength computed) - Update CLAUDE.md persistence + setup endpoint descriptions - Add completion prerequisite note to user_guide.md - Fix test docstrings for accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6d250c9 to
60c017e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/controllers/setup.py`:
- Around line 481-500: When SettingNotFoundError is raised by
settings_svc.get_entry, avoid emitting both logger.debug and the later
logger.warning for the same SETUP_NO_COMPANY event; update the exception handler
around settings_svc.get_entry (the try that sets has_company) to set has_company
= False and call logger.warning(SETUP_NO_COMPANY, reason="setting_not_found")
(or remove the later warning) so only a single warning-level log is emitted
before raising ApiValidationError; reference settings_svc.get_entry,
SettingNotFoundError, has_company, SETUP_NO_COMPANY, logger.debug,
logger.warning, and ApiValidationError when making the change.
- Around line 229-230: Wrap the call to
persistence.users.count_by_role(HumanRole.CEO) in a try/except that catches
QueryError, mirroring the pattern used for the get_entry calls: on success keep
admin_count and compute needs_admin = admin_count == 0; on QueryError set a safe
default (admin_count = 0 so needs_admin becomes True) and log the exception
using the same logger used elsewhere in this module (e.g., process_logger or
logger). Ensure you reference the persistence.users.count_by_role call, the
HumanRole.CEO symbol, and the needs_admin variable when making the change.
In `@tests/unit/api/controllers/test_setup.py`:
- Around line 393-433: The test test_complete_validates_all_prerequisites
mutates settings_repo._store by setting ("company","agents") and does not
restore it; wrap the mutation and subsequent POST checks in a try/finally (or
capture the original value via settings_repo._store.get(("company","agents"))
and restore it after the assertions) so the settings_repo._store key is either
reset to its prior tuple value or removed if it did not exist, ensuring shared
state is cleaned up; reference settings_repo and the ("company","agents") key
when implementing the restore in the finally block.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 671e8699-bbff-424c-be1a-b13524b4b65d
📒 Files selected for processing (17)
CLAUDE.mddocs/design/operations.mddocs/user_guide.mdsrc/synthorg/api/controllers/setup.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/setup.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/user_repo.pytests/unit/api/controllers/test_setup.pytests/unit/api/fakes.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/persistence/test_protocol.pyweb/src/__tests__/stores/setup.test.tsweb/src/__tests__/views/SetupPage.test.tsweb/src/api/types.tsweb/src/components/setup/SetupAdmin.vueweb/src/stores/setup.ts
| admin_count = await persistence.users.count_by_role(HumanRole.CEO) | ||
| needs_admin = admin_count == 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an existing pattern for handling persistence errors in status endpoints
rg -n "count_by_role" --type=py -A5 -B5 | head -60Repository: Aureliolo/synthorg
Length of output: 4471
🏁 Script executed:
#!/bin/bash
# Get the full function context around lines 229-230 in setup.py
sed -n '200,280p' src/synthorg/api/controllers/setup.py | cat -nRepository: Aureliolo/synthorg
Length of output: 3237
🏁 Script executed:
#!/bin/bash
# Check the count_by_role implementation and what exceptions it raises
sed -n '400,420p' src/synthorg/persistence/repositories.py | cat -nRepository: Aureliolo/synthorg
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Search for the actual SQLiteUserRepository implementation of count_by_role
fd count_by_role --type py -x grep -l "class.*UserRepository" | head -5Repository: Aureliolo/synthorg
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Find SQLiteUserRepository implementation file
find . -name "*.py" -type f -exec grep -l "class SQLiteUserRepository" {} \;Repository: Aureliolo/synthorg
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check the actual setup.py to understand line numbering
wc -l src/synthorg/api/controllers/setup.py
# Get a wider context around the target area
sed -n '220,240p' src/synthorg/api/controllers/setup.py | cat -nRepository: Aureliolo/synthorg
Length of output: 925
🏁 Script executed:
#!/bin/bash
# Check for the event constant SETUP_STATUS_SETTINGS_UNAVAILABLE
rg "SETUP_STATUS_SETTINGS_UNAVAILABLE" --type=pyRepository: Aureliolo/synthorg
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Find count_by_role implementation in SQLiteUserRepository
rg -n "async def count_by_role" src/synthorg/persistence/sqlite/user_repo.py -A 15Repository: Aureliolo/synthorg
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Check what's imported at the top of setup.py to see if there's logging setup
head -50 src/synthorg/api/controllers/setup.py | grep -E "^(import|from)"Repository: Aureliolo/synthorg
Length of output: 929
🏁 Script executed:
#!/bin/bash
# Look for any exception types raised in count_by_role or by the database layer
rg "class.*Error" src/synthorg/persistence/ --type=py | head -20Repository: Aureliolo/synthorg
Length of output: 525
🏁 Script executed:
#!/bin/bash
# Get full count_by_role implementation to see what exceptions it catches/raises
sed -n '267,290p' src/synthorg/persistence/sqlite/user_repo.py | cat -nRepository: Aureliolo/synthorg
Length of output: 1005
🏁 Script executed:
#!/bin/bash
# Check if QueryError is subclass of PersistenceError
rg -A 3 "class QueryError" src/synthorg/persistence/errors.pyRepository: Aureliolo/synthorg
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Verify the complete count_by_role implementation including what happens after exception catching
sed -n '267,295p' src/synthorg/persistence/sqlite/user_repo.py | cat -nRepository: Aureliolo/synthorg
Length of output: 1208
🏁 Script executed:
#!/bin/bash
# Check if there's a logger and event constant for persistence errors
rg "PERSISTENCE_USER_COUNT_BY_ROLE_FAILED" src/synthorg/observability/events/Repository: Aureliolo/synthorg
Length of output: 164
Add error handling for count_by_role to match robustness patterns used for other settings queries.
The count_by_role call can raise QueryError if the database is unavailable. The get_entry calls below (lines 233–243, 251–275) already follow an error-handling pattern with try/except and safe defaults; this call should match that pattern to prevent the /status endpoint from returning 500 when the database is temporarily unavailable.
♻️ Suggested error handling
- admin_count = await persistence.users.count_by_role(HumanRole.CEO)
- needs_admin = admin_count == 0
+ try:
+ admin_count = await persistence.users.count_by_role(HumanRole.CEO)
+ needs_admin = admin_count == 0
+ except MemoryError, RecursionError:
+ raise
+ except Exception:
+ logger.warning(
+ SETUP_STATUS_SETTINGS_UNAVAILABLE,
+ setting="admin_count",
+ exc_info=True,
+ )
+ needs_admin = True # Safe default: prompt for admin creation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/setup.py` around lines 229 - 230, Wrap the call
to persistence.users.count_by_role(HumanRole.CEO) in a try/except that catches
QueryError, mirroring the pattern used for the get_entry calls: on success keep
admin_count and compute needs_admin = admin_count == 0; on QueryError set a safe
default (admin_count = 0 so needs_admin becomes True) and log the exception
using the same logger used elsewhere in this module (e.g., process_logger or
logger). Ensure you reference the persistence.users.count_by_role call, the
HumanRole.CEO symbol, and the needs_admin variable when making the change.
| # Verify company has been created. | ||
| has_company = False | ||
| try: | ||
| entry = await settings_svc.get_entry("company", "company_name") | ||
| has_company = bool(entry.value and entry.value.strip()) | ||
| except MemoryError, RecursionError: | ||
| raise | ||
| except SettingNotFoundError: | ||
| logger.debug(SETUP_NO_COMPANY, reason="setting_not_found") | ||
| if not has_company: | ||
| msg = "A company must be created before completing setup" | ||
| logger.warning(SETUP_NO_COMPANY) | ||
| raise ApiValidationError(msg) | ||
|
|
||
| # Verify at least one agent has been created. | ||
| existing_agents = await _get_existing_agents(settings_svc) | ||
| if not existing_agents: | ||
| msg = "At least one agent must be created before completing setup" | ||
| logger.warning(SETUP_NO_AGENTS) | ||
| raise ApiValidationError(msg) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validation logic is correct; minor observability note.
The prerequisite checks follow the correct order per PR objectives: company → agents → providers.
One observation: if SettingNotFoundError is caught (line 488-489), we log at DEBUG, then has_company remains False, triggering the WARNING log at line 492. This results in two log entries for the same event (SETUP_NO_COMPANY) at different levels. While not incorrect (DEBUG for tracing, WARNING for alerting), it could create slight noise.
💡 Optional: Consolidate to single warning with reason
has_company = False
+ reason: str | None = None
try:
entry = await settings_svc.get_entry("company", "company_name")
has_company = bool(entry.value and entry.value.strip())
+ if not has_company:
+ reason = "empty_or_blank"
except MemoryError, RecursionError:
raise
except SettingNotFoundError:
- logger.debug(SETUP_NO_COMPANY, reason="setting_not_found")
+ reason = "setting_not_found"
if not has_company:
msg = "A company must be created before completing setup"
- logger.warning(SETUP_NO_COMPANY)
+ logger.warning(SETUP_NO_COMPANY, reason=reason)
raise ApiValidationError(msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/setup.py` around lines 481 - 500, When
SettingNotFoundError is raised by settings_svc.get_entry, avoid emitting both
logger.debug and the later logger.warning for the same SETUP_NO_COMPANY event;
update the exception handler around settings_svc.get_entry (the try that sets
has_company) to set has_company = False and call
logger.warning(SETUP_NO_COMPANY, reason="setting_not_found") (or remove the
later warning) so only a single warning-level log is emitted before raising
ApiValidationError; reference settings_svc.get_entry, SettingNotFoundError,
has_company, SETUP_NO_COMPANY, logger.debug, logger.warning, and
ApiValidationError when making the change.
| def test_complete_validates_all_prerequisites( | ||
| self, | ||
| test_client: TestClient[Any], | ||
| ) -> None: | ||
| """Completion requires company, agents, and providers. | ||
|
|
||
| The test fixture's root_config provides company_name, so the | ||
| company check passes automatically. This test walks through | ||
| the remaining prerequisite checks: agents, then providers, | ||
| then confirms success once all are satisfied. | ||
| """ | ||
| app_state = test_client.app.state.app_state | ||
| settings_repo = app_state.persistence._settings_repo | ||
| now = datetime.now(UTC).isoformat() | ||
|
|
||
| # 1. No agents -- rejected (company comes from root_config). | ||
| resp = test_client.post("/api/v1/setup/complete") | ||
| assert resp.status_code == 422 | ||
| assert "agent" in resp.json()["error"].lower() | ||
|
|
||
| # 2. Agents set, no providers -- rejected. | ||
| agents_json = json.dumps([{"name": "agent-001", "role": "CEO"}]) | ||
| settings_repo._store[("company", "agents")] = (agents_json, now) | ||
| resp = test_client.post("/api/v1/setup/complete") | ||
| assert resp.status_code == 422 | ||
| assert "provider" in resp.json()["error"].lower() | ||
|
|
||
| # 3. All present -- success. | ||
| stub = MagicMock(spec=BaseCompletionProvider) | ||
| original = app_state._provider_registry | ||
| app_state._provider_registry = ProviderRegistry( | ||
| {"test-provider": stub}, | ||
| ) | ||
| try: | ||
| resp = test_client.post("/api/v1/setup/complete") | ||
| assert resp.status_code == 201 | ||
| body = resp.json() | ||
| assert body["success"] is True | ||
| assert body["data"]["setup_complete"] is True | ||
| finally: | ||
| app_state._provider_registry = original |
There was a problem hiding this comment.
Clean up mutated settings state in prerequisite test.
Line 415 mutates shared settings (("company", "agents")) but this test never restores it. That can make tests order-dependent if fixture scope is broader than function-level.
🔧 Suggested fix
def test_complete_validates_all_prerequisites(
self,
test_client: TestClient[Any],
) -> None:
@@
# 2. Agents set, no providers -- rejected.
agents_json = json.dumps([{"name": "agent-001", "role": "CEO"}])
settings_repo._store[("company", "agents")] = (agents_json, now)
- resp = test_client.post("/api/v1/setup/complete")
- assert resp.status_code == 422
- assert "provider" in resp.json()["error"].lower()
-
- # 3. All present -- success.
- stub = MagicMock(spec=BaseCompletionProvider)
- original = app_state._provider_registry
- app_state._provider_registry = ProviderRegistry(
- {"test-provider": stub},
- )
try:
- resp = test_client.post("/api/v1/setup/complete")
- assert resp.status_code == 201
- body = resp.json()
- assert body["success"] is True
- assert body["data"]["setup_complete"] is True
+ resp = test_client.post("/api/v1/setup/complete")
+ assert resp.status_code == 422
+ assert "provider" in resp.json()["error"].lower()
+
+ # 3. All present -- success.
+ stub = MagicMock(spec=BaseCompletionProvider)
+ original = app_state._provider_registry
+ app_state._provider_registry = ProviderRegistry(
+ {"test-provider": stub},
+ )
+ try:
+ resp = test_client.post("/api/v1/setup/complete")
+ assert resp.status_code == 201
+ body = resp.json()
+ assert body["success"] is True
+ assert body["data"]["setup_complete"] is True
+ finally:
+ app_state._provider_registry = original
finally:
- app_state._provider_registry = original
+ settings_repo._store.pop(("company", "agents"), None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/controllers/test_setup.py` around lines 393 - 433, The test
test_complete_validates_all_prerequisites mutates settings_repo._store by
setting ("company","agents") and does not restore it; wrap the mutation and
subsequent POST checks in a try/finally (or capture the original value via
settings_repo._store.get(("company","agents")) and restore it after the
assertions) so the settings_repo._store key is either reset to its prior tuple
value or removed if it did not exist, ensuring shared state is cleaned up;
reference settings_repo and the ("company","agents") key when implementing the
restore in the finally block.
…store agents state in test
- Wrap count_by_role(HumanRole.CEO) in try/except QueryError with
fail-safe default (needs_admin=True) and warning log
- Remove duplicate SETUP_NO_COMPANY debug log in SettingNotFoundError
handler; single warning emitted before raising ApiValidationError
- Wrap ("company","agents") mutation in test_complete_validates_all_prerequisites
with try/finally to restore settings_repo._store
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.3.8](v0.3.7...v0.3.8) (2026-03-19) ### Features * dynamic settings UI with auto-discovery and basic/advanced toggle ([#600](#600)) ([11b32b7](11b32b7)) ### Bug Fixes * **ci:** remove dst: . from GoReleaser archive config ([#598](#598)) ([c8bf862](c8bf862)) * **engine:** wire compaction_callback and plan_execute_config through auto-selection ([#601](#601)) ([795327f](795327f)) * harden setup wizard completion and status checks ([#616](#616)) ([d99d7b7](d99d7b7)) ### Maintenance * add /codebase-audit skill for deep parallel codebase auditing ([#613](#613)) ([db02320](db02320)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Three setup wizard hardening fixes from PR #584 review:
needs_adminnow checks for CEO role explicitly via newcount_by_role(HumanRole)method onUserRepositoryprotocol, SQLite implementation, and fake test implementation -- instead of just checkinguser_count == 0complete_setupverifies company name and at least one agent exist (via settings) before marking setup complete, in addition to the existing provider check. Check order: company -> agents -> providersSetupStatusResponsenow includesmin_password_lengthfrom backend settings;SetupAdmin.vuereads it from the setup store instead of the hardcodedMIN_PASSWORD_LENGTHconstant, with fallback to the constant if the server value is unavailableChanges
Backend
UserRepositoryprotocol: addedcount_by_role(role: HumanRole) -> intSQLiteUserRepository: parameterizedSELECT COUNT(*) WHERE role = ?implementationsetup.pycontroller:get_setup_statususescount_by_role(CEO)forneeds_admin;complete_setupvalidates company + agents + providers; status response includesmin_password_lengthSETUP_NO_COMPANY,SETUP_NO_AGENTS,PERSISTENCE_USER_COUNTED_BY_ROLE,PERSISTENCE_USER_COUNT_BY_ROLE_FAILEDFrontend
SetupStatusResponsetype: addedmin_password_length: numbersetup.tsstore: addedminPasswordLengthcomputed withMIN_PASSWORD_LENGTHfallbackSetupAdmin.vue: reads password length from store instead of hardcoded constantDocs
CLAUDE.mdanddocs/design/operations.md: clarified completion gate prerequisitesTest plan
test_needs_admin_true_when_only_non_admin_exists-- verifies CEO-specific checktest_needs_admin_false_when_ceo_exists-- verifies default fixture passestest_status_includes_min_password_length-- verifies new field in responsetest_complete_validates_all_prerequisites-- walks through agents -> providers -> success checksPre-reviewed by 4 agents (code-reviewer, frontend+API-contract, docs-consistency, issue-resolution-verifier). 2 doc findings addressed.
Closes #587
Closes #588
Closes #589
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation