fix: inherit agent model in Skill Forge LLM calls - #208
Open
guix4ever wants to merge 1 commit into
Open
Conversation
guix4ever
marked this pull request as ready for review
July 24, 2026 06:14
Contributor
|
Heads-up on a naming change that will collide with this branch. A test-debt cleanup (#223) renames Sorry for the churn. Nothing to do until the rename merges. |
6 tasks
Author
|
Got it, thanks for the heads-up. I'll rebase onto the latest main and move my test changes to |
0xKT
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary A read-only audit of all 298 test files found the suite in good shape -- exactly one genuinely fake test -- with the real debt sitting in file names and a scattering of assertions that passed for the wrong reason. This clears that, and fixes the bugs found while clearing it. **Three dead tests dealt with.** Two removed because neither could ever assert anything: - `test_chat_e2e.py` was a module-level skip over three `NotImplementedError` stubs. The wire-up it waited for has shipped (`tui_commands` instantiates `SubscriptionEmitter`) and the proposal doc it cited is gone. - `test_tui_exit_e2e.py` asserted that building the agent loop leaves lancedb's thread live. It does not: the loop is constructed but never started, and the memory plugin talks to everos over HTTP, so lancedb is not even imported. The assertion could not hold in any configuration. The third, `slashParity.test.ts`, shelled out to python for a `raven_cli.commands.COMMAND_REGISTRY` that no longer exists, and skipped all three of its cases when the import failed -- three skips on every run. The parity it wanted needs no python: `createSlashHandler.ts:45-51` routes a slash locally when `findSlashCommand` resolves it and otherwise hands it to `slash.exec`, a CLI subprocess a mutation cannot reach the live session from. It now asserts that predicate directly. `NATIVE_MUTATING_COMMANDS` and the three-way `classifyRoute` went with it: all six names in that set are also in the local registry, so the native branch could not change any outcome. **19 files renamed** off phase, ticket and bug codes (AGENTS.md section 5.1), including one reversed CLI name and two integration files moved onto a legal `<kind>`. Reference updates ride along: `.pre-commit-config.yaml` names one of these files in a `detect-private-key` exclude, and three docs pages point at them. **Assertions that passed for the wrong reason, tightened**: eight over-broad `pytest.raises(Exception)` narrowed to the type each site actually raises; an `except` clause that swallowed the `AssertionError` its own fakes use as a tripwire; a `hasattr` probe replaced by binding a broker and checking it stuck; and a recall assertion that read as a pass on an empty result set. **Dead weight removed**: ten unused locals, three dead code blocks, six `importorskip` guards on hard dependencies, two hardcoded `/tmp` paths, and ticket codes left in docstrings after the renames. **Two coverage gaps the deletions exposed are now closed**: - `tests/test_cli_exit.py` pins the hard-exit guard, which had no tests at all. - The TUI chat e2e gains a multi-turn case. It reads the persisted session rather than the screen, because a prompt is echoed into the transcript the moment it is typed -- screen-scraping for a planted word passes even when history is broken. **Three timing bugs fixed in the TUI e2e tests.** They keyed off the banner, which paints about seven seconds before the app accepts input, and submitted the moment text was typed. Dropped prompts made the pipeline look dead in one test, and made a negative assertion (`assert not leak_detected`) pass vacuously in another -- no prompt means no streaming means no leak. **Gaps in the two features that landed during review**, small enough to close here: the `session.py` wiring that fills `update_available` / `update_command` (removing it left the suite green), `_upgrade_command_works` (a fixture stubbed it to `True` everywhere), an `episodeSummary` case with an empty test body, and `clipToWidth` / `hasMeaningfulReasoning`, which had no coverage at all. The larger items from that audit are tracked in #229. **Upstream review attributions dropped from ui-tui comments.** 36 mentions of another project's review thread (#19835 as "Copilot round-N review"), plus #18994, #19194 and #14045 -- none resolve to anything here, and AGENTS.md section 1.1 rules out comments pointing at information only visible elsewhere. The rationale each carried is kept; four `describe()` titles lose a trailing `(#18994)`. Three went further and documented a python counterpart that does not exist: `platform.ts` credited key-spelling normalization to `raven_cli/voice.py` (no such module -- `raven/tui_rpc/methods/_stubs.py` answers `voice.toggle` with "voice not supported in Raven v0.1"), and `useConfigSync.ts` plus its test credited an `interrupt` default to `raven_cli/config.py` and `tui_gateway/server.py::_load_busy_input_mode` (`busy_input_mode` appears nowhere in python; neither module exists). **One flaky test fixed, caught by this PR's own CI run.** `test_stdio_no_executor_does_not_raise` set `command="true"` and let `connect_mcp_servers` really spawn it, so the result depended on how a live child process raced the MCP handshake -- it failed once on CI in the full-suite run while passing 30 out of 30 standalone runs locally. It also asserted only that whatever came back was not a `SandboxInitError`. The transport now raises instead of running (the idiom its sibling test already uses) and the test asserts the stdio branch was reached. Review follow-ups, folded in: the hard-exit test now skips rather than asserting no lancedb thread exists process-wide, `tests/conftest.py` no longer credits the pytest segfault to a lancedb thread the suite never starts (raven imports lancedb nowhere), and the status-bar patterns the two TUI e2e tests wait on live in one module instead of a hand-mirrored copy per file. Every new or changed assertion was checked by mutating the code under test and confirming the assertion fails. ## Type - [x] Others Test files, test configuration, and comments. The only non-test files touched are ui-tui comments and four `describe()` titles -- no behaviour change. ## Verification ``` uv run --all-extras pytest -q # 4654 passed, 25 deselected cd ui-tui && npx vitest run # 936 passed, no skips uv run --all-extras pytest tests/tui/autotest -m e2e -q # passes locally with tui-use installed and ui-tui built bash .claude/scripts/preflight_ci.sh # ALL PREFLIGHT CHECKS PASSED (including the TUI lane) ``` Mutation checks, each confirming the new assertion fails when the code under test is broken: the `session.py` nudge wiring removed; `clipToWidth` replaced with identity; `hasMeaningfulReasoning` forced to `true`; a never-sent prompt added to the multi-turn session assertion. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes No behaviour changes: the non-test diff is comments only. The renames touch git history for 19 test files; #208 is the only open PR affected and has been notified. Two known items are deliberately left alone: `test_dogfood_whitelist.py` is flaky when the whole file runs (tracked in #228, unchanged here), and the `lancedb_finalization_hazard` gate in `commands.py` never fires today -- its test docstring says so explicitly, so the coverage does not read as an argument for keeping it. ## Related Issues Closes #223 --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
guix4ever
force-pushed
the
fix/skill_forge_model_inheritance
branch
from
July 30, 2026 07:20
6e2f1fb to
f98b71f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
llm_gate_modelis unsetRoot cause
The context-engine factory did not pass the active agent model into either Skill Forge LLM stage.
QueryRewriterhad no model parameter, whileLLMGateFilterreceivedNonewhen no gate-specific override was configured. Both calls could therefore fall back to a provider-level default that differed fromagents.defaults.model, including models unavailable in the user's region.Impact
The rewriter and gate now use the same configured model as the agent by default. Deployments that set
skillForge.llmGateModelcontinue to use that explicit model for gate calls.Follow-up consideration
A future change could add an optional
rewrite_modeloverride, withagents.defaults.modelas its fallback. That would let operators pin a cheaper or lower-latency model for query rewriting without relying on an implicit provider default. This PR intentionally does not add a new configuration field so the fix remains focused on model inheritance.Validation
uv run pytest tests/test_skill_forge_rewriter.py tests/test_skill_forge_gate.py tests/test_phase_a_default_engine.py -q— 33 passeduv run --extra dev ruff checkon all changed Python filesuv run --extra dev ruff format --checkon all changed Python filesgit diff --checkCloses #155.