fix(#240): use configured clarification model for pm#243
fix(#240): use configured clarification model for pm#243ouroboros-agent[bot] wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
b80249c| Triggered by: PR opened
Branch: issue-240/ouroboros-pm-hardcodes-claude-model-inst | 2 files, +45/-2 | CI: passing
Issue #240 Requirements
| Requirement | Status |
|---|---|
| ouroboros pm hardcodes Claude model instead of using configured clarification model | CHECKED IN REVIEW — see findings and changed files. |
Previous Review Follow-up
First review — no previous findings.
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/pm.py:86 | High | High | Omitting --model no longer uses the previous built-in default (claude-sonnet-4-20250514); it now resolves through get_clarification_model(), which falls back to claude-opus-4-6 for normal backends. That is a breaking default change for every unconfigured ouroboros pm run, with materially different cost/latency characteristics. |
| 2 | src/ouroboros/cli/commands/pm.py:86 | Medium | Medium | --resume now re-resolves the model from current config/env on every invocation instead of using a stable default. If OUROBOROS_CLARIFICATION_MODEL or config.yaml changes between runs, the same interview session can silently switch models mid-stream, which makes resumed behavior nondeterministic. |
| 3 | src/ouroboros/cli/commands/pm.py:86 | Medium | High | This command now depends on config loading before it can even print the banner. get_clarification_model() only recovers from ConfigError; load_config() still lets raw file I/O errors from config_path.open() escape, so an unreadable config file can now crash bare ouroboros pm at startup. The old hardcoded default did not have this failure mode. |
| 4 | src/ouroboros/cli/commands/pm.py:52 | Low | High | Changing the option default from a concrete string to None removes the CLI’s explicit default value from Typer’s public interface. Users can no longer learn the effective PM model from --help, and because the value now comes from config/env, the command surface is less discoverable and harder to reason about. |
| 5 | tests/unit/cli/test_pm.py:12 | Medium | High | The new tests only cover the happy path for default resolution and explicit override. They do not cover the PR’s risky cases: config-load failure, fallback behavior when config is absent/unreadable, or resume-time model stability. As a result, the regressions above would ship without test detection. |
Test Coverage
Missing regression coverage in tests/unit/cli/test_pm.py for get_clarification_model() failure paths, especially unreadable ~/.ouroboros/config.yaml and other non-ConfigError startup failures introduced by src/ouroboros/cli/commands/pm.py.
Missing coverage for resume behavior in src/ouroboros/cli/commands/pm.py: there is no test proving a resumed PM session keeps a stable model instead of re-reading mutable config/env state.
Missing CLI-surface coverage for the changed option contract at src/ouroboros/cli/commands/pm.py: no test checks pm --help or otherwise verifies what default model is exposed/documented after switching the option default to None.
Design Notes
Centralizing model selection through config is the right direction, but this implementation couples PM session behavior to mutable global config at invocation time. For a stateful workflow like pm --resume, the model should be persisted with session metadata or derived from saved session state so resumed interviews are reproducible. If the CLI does resolve from config, it should also keep the default visible and handle config I/O failures defensively so the command remains usable without perfect local setup.
Follow-up Recommendations
- Re-run review after author addresses blocking findings.
- Consider adding targeted regression tests for the modified behavior.
Files Reviewed
src/ouroboros/cli/commands/pm.pytests/unit/cli/test_pm.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Summary
Test Plan
Closes #240