fix(install): gate Claude-only wizard prompts on agent_backend#381
fix(install): gate Claude-only wizard prompts on agent_backend#381
Conversation
The `make config` wizard prompted for `CLAUDE_AUTOCOMPACT_PCT`, `CLAUDE_EFFORT_LEVEL`, and the legacy `CLAUDE_USER` env var unconditionally, even when the operator selected `goose` as the agent backend. None of these are consumed by GooseBackend at runtime, so the prompts asked the operator to set values that would be silently ignored. Three changes in install.py: - Wrap the autocompact prompt in `if agent_backend == "claude":` and initialize `autocompact_pct = "0"` (dataclass default) before the gate so the existing emission check skips correctly for goose. - Same pattern for the effort prompt: initialize to `"high"` and gate. - Extend the existing `if/elif/else` cascade for `claude_user` with a third short-circuit branch `elif agent_backend != "claude":` matching the structure of the two existing short-circuits. The `admin_os_user` prompt at install.py:382 is intentionally NOT gated in this PR. It lives inside `if advanced:`, which fires before `agent_backend` is defined at line 440. Honoring the gate would require either reordering the wizard (move agent_backend prompt earlier) or moving the os_user prompt later. Both touch the wizard's structural UX; deferred until the broader multi-backend rework revisits the wizard end-to-end. Tests: - Extend `_base_inputs` with `agent_backend` (default "claude"), `llm_provider`, `llm_api_key` parameters. When agent_backend is not claude, the helper omits autocompact, effort, and claude_user entries (gated to claude only) and inserts provider + API key entries (the wizard prompts for those on the goose path). - New test `test_goose_backend_skips_claude_only_prompts` asserts the three CLAUDE_* env keys are absent in install.conf when the wizard ran under goose. - New test `test_goose_backend_prunes_existing_claude_only_keys` pre-seeds an install.conf with all three keys set, runs the wizard under goose, and asserts they all disappear. Pins the implicit auto-prune that falls out of the "initialize-before-gate" pattern. - Trim three existing goose fixtures (`test_goose_backend_writes_env`, `test_goose_ollama_no_api_key`, `test_non_claude_backend_drops_stale_extraction_keys`) to remove the three surplus entries each (autocompact, effort, claude_user) that the wizard no longer consumes under goose. - Three pre-existing claude tests (`test_writes_install_conf`, `test_advanced_user_options`, `test_wizard_defaults_do_not_reintroduce_shared_home`) had been passing on borrowed time: they did not provide explicit autocompact / effort / claude_user entries and were relying on the wizard's retry-on-empty-input default fallback to consume downstream entries by mistake. Pytest output capture hid the validation prints. Add the missing entries explicitly so the tests do not depend on absorption side-effects. Tests: 2384 pass (was 2382; +2 new tests).
Review by KaiThe PR is well-structured. Here are my findings: Warning
Suggestion
Suggestion
Everything else looks correct. The "initialize-before-gate" pattern is sound - all three variables are set to their dataclass defaults before the conditional, so the downstream emission checks ( |
- Drop the "set llm_api_key=None for ollama" suggestion from the _base_inputs docstring. The parameter is typed `str` (no None variant), and the wizard skips the API key prompt for ollama anyway, so the default value is harmless and unused. Pyright strict would have flagged the suggested usage. - Replace the memory-file path reference in the test_goose_backend_skips_claude_only_prompts docstring with a plain-language deferral note. A future developer reading the test should not need to know about an internal AI memory file. - Add a guard in _base_inputs that raises ValueError when a caller passes a non-empty effort value with a non-claude backend. The effort prompt does not fire for non-claude backends (gated by this PR), so silently ignoring the parameter would be a confusing failure mode for a future test author.
Review by KaiThe implementation is clean. Here's what I found after reviewing the diff against the prior thread: Prior review items - status All three issues from the prior review are resolved:
New findings Suggestion - # GooseBackend in pool.py:186-198 takes no claude_user kwargLine-number references in cross-file comments are fragile - pool.py can be edited independently and this will silently drift. Drop the Suggestion - # Legacy CLAUDE_USER prompt at install.py:730 is gated on
# agent_backend == "claude" by issue #380 (Change 4).The "(Change 4)" label will be opaque to anyone who doesn't have access to the internal spec. Since the PR description gives the full context, "Change 4 of the spec" should be replaced with just "issue #380" or left out entirely. Minor, but the CLAUDE.md commenting guide implies comments should explain the WHY, not reference private documents. Everything else is correct. The "initialize-before-gate" pattern is applied consistently across all three variables. The |
- Drop the `pool.py:186-198` line-number qualifier from the claude_user comment in install.py. Cross-file line-number references are fragile - pool.py can be edited independently and the comment will silently drift. Reference the function name (GooseBackend) instead, which is stable across refactors. - Drop the "(Change 4)" spec reference from the test comment in _base_inputs. Spec labels are opaque without access to the internal spec doc and violate the project's no-spec-refs rule for public artifacts. The "issue #380" reference is sufficient.
Review by KaiThe diff is clean. One new finding: Suggestion - # Legacy CLAUDE_USER prompt at install.py:730 is gated on
# agent_backend == "claude" by issue #380.
Everything else is correct:
|
Drop the `:730` line-number qualifier from the legacy CLAUDE_USER comment in tests/test_install.py. Same pattern as the round 2 fix for `pool.py:186-198`: cross-file line-number references drift on every edit and produce silently-stale comments. The issue number and variable name are sufficient to locate the gate. Saved memory feedback_no_cross_file_line_numbers_in_comments.md to capture the rule (this PR hit it twice in adjacent rounds).
Review by KaiLooking at this diff against the prior review thread - all three previously-raised issues (pool.py line-number reference, "(Change 4)" label, Suggestion - Note: the admin_os_user prompt at install.py:382 is NOT gated
in this PR.This is a new instance of the exact pattern caught in the last two review rounds - a fragile line-number reference embedded in a comment. Drop the Everything else is correct:
|
Drop the `:382` line-number qualifier from the admin_os_user note in test_goose_backend_skips_claude_only_prompts. Same pattern as the round 2 (pool.py:186-198) and round 3 (install.py:730) fixes. Audited the full diff for any other instances of the file:NNN pattern in added comments before pushing - this was the only remaining one.
Review by KaiLooking at the diff against the prior review thread - the one open finding from the last round ( No new issues found. Specifically:
This PR is clean. |
2 participants
Summary
_cmd_config()previously fired regardless of the operator's selected agent backend, even though the captured values are consumed only byClaudeCodeBackend. Goose-backend operators were asked to set values that would be silently ignored at runtime. This PR adds backend-conditional gating forCLAUDE_AUTOCOMPACT_PCT,CLAUDE_EFFORT_LEVEL, and the legacyCLAUDE_USERprompt at install.py:730.if value != default) correctly skips writing the env key under goose without needing a parallel emission gate. As a free side effect, this implicitly auto-prunes previously-set values frominstall.confon a claude→goose switch — pinned by a regression test.admin_os_userprompt at install.py:382) is intentionally NOT included. That prompt fires insideif advanced:, which runs beforeagent_backendis defined at line 440. Honoring the gate would require structural reordering of the wizard. Deferred to the broader multi-backend rework per operator direction.Fixes #380 (partial — three of four planned gates; admin_os_user gate deferred per above).
Test plan
make check(ruff check + format) — local: passesmake test(full suite, 2384 tests) — local: passestest_goose_backend_skips_claude_only_prompts(new): asserts CLAUDE_AUTOCOMPACT_PCT, CLAUDE_EFFORT_LEVEL, CLAUDE_USER all absent from install.conf when wizard ran under goosetest_goose_backend_prunes_existing_claude_only_keys(new): pre-seeds install.conf with all three keys at non-default values, runs wizard under goose, asserts all three disappear (pins the implicit auto-prune)make configwalk-through under goose backend skips the autocompact, effort, and CLAUDE_USER prompts but otherwise behaves as beforemake configwalk-through under claude backend prompts for all three knobs as before (no behavior regression)Out of scope
admin_os_userprompt at install.py:382 (described above; deferred to broader multi-backend rework).CLAUDE_MAX_SESSION_HOURS/CLAUDE_IDLE_TIMEOUT(Claude-only at the consumer but no wizard prompt today).CLAUDE_*prompts (CLAUDE_TIMEOUT_SECONDS,CLAUDE_MAX_CONTEXT_WINDOW,CLAUDE_MAX_BUDGET_USD→BUDGET_CEILING,CLAUDE_MODEL→DEFAULT_MODEL). All four are consumed by both backends and are correctly NOT gated today.