fix(db): read defaultAssistant from config in createCodebase (fixes #1703)#1746
Conversation
📝 WalkthroughWalkthroughcreateCodebase now selects assistantType in this order: ChangesAssistant Type Precedence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/db/codebases.test.ts (1)
18-20: ⚡ Quick winPrefer
spyOn()overmock.module()for this internal module mock.Line 18 uses
mock.module()for../config/config-loader; switch this tospyOn()+mockRestore()to avoid test isolation issues from module-level mocks.As per coding guidelines, "Use
spyOn()for internal modules that other test files import directly ...mock.module()cleanup does NOT work and should never be relied upon".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/db/codebases.test.ts` around lines 18 - 20, Replace the module-level mock.module call that stubs loadConfig with a Jest spy so the mock is properly scoped and cleaned up: use spyOn(...) to spy on the loadConfig export (e.g., spyOn(require(...), 'loadConfig').mockImplementation(mockLoadConfig)) and then call mockRestore() in an afterEach to restore the original implementation; specifically swap out the mock.module usage for spyOn targeting loadConfig, keep using mockLoadConfig as the implementation, and ensure mockRestore() runs after each test to prevent cross-test pollution.packages/core/src/db/codebases.ts (1)
22-30: ⚡ Quick winAdd explicit observability for config-load fallback.
Line 27 silently catches config-load failures and falls back to
'claude'. Please log a structured_failedevent (and optionally a success_completedevent) so fallback behavior is diagnosable in production.As per coding guidelines, "
**/*.ts: Use structured logging with Pino using event naming convention{domain}.{action}_{state}" and "Apply Fail Fast + Explicit Errors - ... never silently swallow errors ... document intentional fallback behavior with a comment".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/db/codebases.ts` around lines 22 - 30, The config-load fallback for assistantType (variable assistantType set via loadConfig()) currently swallows errors; update the try/catch to log a structured Pino event when loadConfig fails (e.g., using the module's logger instance such as processLogger or logger) with an event name like "config.load_failed" and include error details and context, and optionally emit "config.load_completed" on success; also add a short comment above the catch documenting the intentional fallback to 'claude' so the behavior is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/db/codebases.test.ts`:
- Around line 18-20: Replace the module-level mock.module call that stubs
loadConfig with a Jest spy so the mock is properly scoped and cleaned up: use
spyOn(...) to spy on the loadConfig export (e.g., spyOn(require(...),
'loadConfig').mockImplementation(mockLoadConfig)) and then call mockRestore() in
an afterEach to restore the original implementation; specifically swap out the
mock.module usage for spyOn targeting loadConfig, keep using mockLoadConfig as
the implementation, and ensure mockRestore() runs after each test to prevent
cross-test pollution.
In `@packages/core/src/db/codebases.ts`:
- Around line 22-30: The config-load fallback for assistantType (variable
assistantType set via loadConfig()) currently swallows errors; update the
try/catch to log a structured Pino event when loadConfig fails (e.g., using the
module's logger instance such as processLogger or logger) with an event name
like "config.load_failed" and include error details and context, and optionally
emit "config.load_completed" on success; also add a short comment above the
catch documenting the intentional fallback to 'claude' so the behavior is
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e99c1572-c78a-4b3d-83f6-3ebcad65a5c0
📒 Files selected for processing (2)
packages/core/src/db/codebases.test.tspackages/core/src/db/codebases.ts
…oleam00#1703) createCodebase() hardcoded 'claude' as the fallback when ai_assistant_type was not provided. Now checks process.env.DEFAULT_AI_ASSISTANT first, consistent with how getOrCreateConversation() resolves the default. Falls back to 'claude' only when both the parameter and env var are unset.
kagura-agent
force-pushed
the
fix/codebase-default-assistant
branch
from
1d0c982 to
6a16d37
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/db/codebases.test.ts (1)
81-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden env-var cleanup to prevent cross-test leakage on failure paths.
process.env.DEFAULT_AI_ASSISTANTis mutated in-test and cleaned up only at the tail of the test body. If a test fails early, env state can leak into subsequent tests.Suggested fix
describe('codebases', () => { + let originalDefaultAssistant: string | undefined; beforeEach(() => { mockQuery.mockClear(); + originalDefaultAssistant = process.env.DEFAULT_AI_ASSISTANT; }); + afterEach(() => { + if (originalDefaultAssistant === undefined) { + delete process.env.DEFAULT_AI_ASSISTANT; + } else { + process.env.DEFAULT_AI_ASSISTANT = originalDefaultAssistant; + } + }); @@ test('reads DEFAULT_AI_ASSISTANT env var when ai_assistant_type omitted', async () => { process.env.DEFAULT_AI_ASSISTANT = 'codex'; @@ expect(mockQuery).toHaveBeenCalledWith(expect.any(String), expect.arrayContaining(['codex'])); - delete process.env.DEFAULT_AI_ASSISTANT; }); @@ test('explicit ai_assistant_type takes priority over env var', async () => { process.env.DEFAULT_AI_ASSISTANT = 'codex'; @@ expect(mockQuery).toHaveBeenCalledWith(expect.any(String), expect.arrayContaining(['pi'])); - delete process.env.DEFAULT_AI_ASSISTANT; });As per coding guidelines, "Apply Determinism + Reproducibility - prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic with no flaky timing or network dependence without guardrails".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/db/codebases.test.ts` around lines 81 - 125, The tests mutate process.env.DEFAULT_AI_ASSISTANT and only delete it at the end, risking env leakage on failures; update the tests around createCodebase (the three tests named 'defaults ai_assistant_type to claude...', 'reads DEFAULT_AI_ASSISTANT env var when ai_assistant_type omitted', and 'explicit ai_assistant_type takes priority over env var') to save the original value and restore it in a finally block (or use beforeEach/afterEach) so process.env.DEFAULT_AI_ASSISTANT is always reset even if the test throws; reference process.env.DEFAULT_AI_ASSISTANT and the createCodebase call when implementing the safeguard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/db/codebases.test.ts`:
- Around line 81-125: The tests mutate process.env.DEFAULT_AI_ASSISTANT and only
delete it at the end, risking env leakage on failures; update the tests around
createCodebase (the three tests named 'defaults ai_assistant_type to claude...',
'reads DEFAULT_AI_ASSISTANT env var when ai_assistant_type omitted', and
'explicit ai_assistant_type takes priority over env var') to save the original
value and restore it in a finally block (or use beforeEach/afterEach) so
process.env.DEFAULT_AI_ASSISTANT is always reset even if the test throws;
reference process.env.DEFAULT_AI_ASSISTANT and the createCodebase call when
implementing the safeguard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a12bf020-eb07-4f54-a016-8fb9a3092f57
📒 Files selected for processing (2)
packages/core/src/db/codebases.test.tspackages/core/src/db/codebases.ts
|
@kagura-agent this PR appears to fully address #1703. Consider adding |
|
Thanks for the review! The PR body already includes |
Review SummaryVerdict: ready-to-merge Clean addition of Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage. |
Review SummaryVerdict: minor-fixes-needed This PR adds a Blocking issues(none) Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage. |
|
Thanks for the thorough review @Wirasm! Good catch on the empty-string edge case — I'll add an empty-string guard so |
2 participants
Summary
createCodebase()hardcoded'claude'as the fallback whenai_assistant_typewas not provided, ignoring the user's configureddefaultAssistant.DEFAULT_AI_ASSISTANTenv var or~/.archon/config.yaml) still gotclaudefor every new project registration.createCodebase()now checksprocess.env.DEFAULT_AI_ASSISTANTbefore falling back to'claude', consistent with howgetOrCreateConversation()resolves the default assistant.ai_assistant_typevalues are used directly. No API schema changes. No new imports.UX Journey
Before
After
Architecture Diagram
No architectural changes. Single fallback chain updated:
data.ai_assistant_type ?? process.env.DEFAULT_AI_ASSISTANT ?? 'claude'.Label Snapshot
dbpackages/core/src/db/codebases.tsChange Metadata
Linked Issue
Closes #1703
Validation Evidence
bun test packages/core/src/db/codebases.test.ts→ 31 pass, 0 failbun run test(full suite) → 0 fail across all packageseslint+prettiervia lint-staged → cleanNew tests:
DEFAULT_AI_ASSISTANTenv var whenai_assistant_typeomitted → usescodexclaudewhen no env var setai_assistant_typetakes priority over env varSecurity Impact
None. Reads an existing env var that the server already uses elsewhere.
Compatibility/Migration
Backward compatible. Behavior is identical when
DEFAULT_AI_ASSISTANTis unset (falls back to'claude').Human Verification
bun test(per-file and full suite)Risks and Mitigations
'claude'(same as before)Side Effects/Blast Radius
None. Only affects the default assistant assignment in
createCodebase().Rollback Plan
Revert the single commit.
Summary by CodeRabbit
New Features
Tests