Refactor/simplify core - #47
Open
Barent wants to merge 23 commits into
Open
Conversation
…try / artifacts R1 (simplify) progress, all tests green: - natshell/agent/intent.py: is_plan_request / is_analysis_request (was inline in loop.py). Old names re-exported from loop for backward-compat. - natshell/agent/events.py: AgentEvent + EventType (was loop-local). Re-exported from loop; loop no longer redefines them. - natshell/agent/repetition_guard.py: RepetitionGuard dataclass owns all six inline detectors (duplicates, re-reads, re-fetches, command families, near-identical commands, edit-failure escalation) that used to be a wall of state + string-building inside handle_user_message. Loop just calls observe() and reads the Observation. - natshell/agent/sudo_retry.py: self-contained async "prompt -> cache -> prepend -> re-classify -> confirm -> re-execute" flow, collected via an on_event callback so event order is preserved. - natshell/agent/context_manager.py: new compress_artifacts() owns the write_file-content elision + long-tool-result truncation that used to live in agent.loop._compress_old_messages (now a thin delegate). - natshell/app.py: the three triplicated confirm / password callback closures in run_agent / run_plan_generation / run_plan are now three one-liners delegating to NatShellApp._confirm_callback / _password_callback / _gated_confirm_callback. loop.py is 1398 → 1140 lines after this refactor; suite green (1598 tests). This is checkpoint 1 of the R1 sweep; ToolCallGrammar (R1) and the R2 perf features (streaming, parallel read-only tools, cache-stable prefix, real-tokenizer budget) still to follow.
First commit of the ToolCallGrammar split. Common primitives that the
per-family modules (qwen / mistral / gemma) will all share live in
``natshell.inference.grammars.common``:
- CODE_FENCE_JSON_RE / THINK_RE / THINK_UNCLOSED_RE (shared regexes)
- parse_structured_tool_calls (the OpenAI-shaped ``tool_calls`` pass)
- try_bare_json_recovery (the two near-identical bare-JSON blocks in the
current engine become one function)
- is_bare_tool_json (single source of truth for "is this leftover text
actually a tool-call dict")
- strip_prose_markers (the 7-marker strip cascade)
- is_degenerate_output (moved verbatim from local.py)
- new_tool_call_id (``uuid4()[:9]`` was repeated 14 times)
The engine itself is unchanged in this commit — the family modules are next.
Suite still green (1598 tests).
Second installment of the ToolCallGrammar split: - common.py grows the Grammar base class (render_tools / parse_native / recover / scrub_recovered / normalize_messages) plus the two shared message normalizers (strict alternation, tool-result-to-user) and format_tool_entries — the per-family modules now only carry what is genuinely family-specific. - qwen.py: XML tool-call parsing (the default/fallback family), prompt rendering (plain + compact). - mistral.py: [TOOL_CALLS] JSON-array parsing, flat-arg fallback, the stricter bare-JSON recovery + scrub, strict-alternation normalization. WIP: gemma.py and the local.py rewiring are the remaining two steps of this item — nothing imports the modules yet, so behaviour is unchanged. Suite green (1598 tests).
Completes the R1§3 ToolCallGrammar extraction. The model family's tool-call
wire format is now split out of local.py into the inference/grammars package:
- qwen.py (landed earlier) — <tool_call> XML wire format
- mistral.py (landed earlier) — [TOOL_CALLS] JSON wire format
- gemma.py (this commit) — <|tool_call>call:NAME{...} native format,
<|channel> think blocks, special-token
scrubbing, tool-message conversion
- common.py (this commit) — shared pipeline primitives + the Grammar
protocol + the parse() pipeline that each
family composes, including the default
ALL_FAMILY_STRIP union
- __init__.py — registry: get_grammar() + ALL_FAMILY_STRIP
local.py goes from 927 -> 378 lines: it now owns only the engine (model
loading, GPU selection, context sizing, chat completion) and the thin
delegations that keep the historical import surface working (_THINK_RE,
_format_tools_for_prompt, _parse_gemma_tool_args, engine._convert_gemma_tool
_messages, ...). chat_completion / _inject_tools / _parse_response all route
through get_grammar(self.model_family).
Suite green: 1598 tests.
Add NATSHELL_PLAN.md — the durable working-plan / source-of-truth for the recurring improvement job. Records the current ground truth (R1-1..R1-5 done, R1-6/R1-7/R1-8 + R2-1..R2-6 remaining), the standing rules (security, green-before-commit, proportionality), and a changelog. REVIEW_NAT_SHELL.md (the original two-part analysis) is tracked alongside. No code changes in this commit.
…oordinator Move the context-overflow / connectivity-failure recovery state machine out of AgentLoop.handle_user_message into natshell.agent.recovery. - RecoveryCoordinator owns the per-run 'attempted' latch and the ordered ladder (overflow->compact/retry, connectivity->ping/compact, then local fallback with preserved-context re-injection). - Loop delegation: the ~130-line except block is now ~11 lines that call RecoveryCoordinator.handle and yield its banner events. - All collaborators (engine, messages, compact, swap, context manager, local-engine loader) are injected, so the state machine is testable in isolation. - _context_recovery_attempted stays as a read-only property; _can_fallback and _try_local_fallback now delegate to the coordinator. User-facing event strings are byte-identical. New: tests/test_recovery.py (16 tests). Full suite: 1614 green.
…r_message R1-7 (slim the orchestrator) — first sub-step. Pull three cohesive, low-coupling blocks out of the 491-line handle_user_message: - _inject_intent — planning/analysis mode reminders (pure append) - _preflight_compaction — pre-inference context-pressure check - _apply_inference_feedback — budget calibration + proactive compaction All three already have dedicated test coverage (TestIntent*, TestProactiveCompaction, budget-scaling tests); behaviour is byte-identical. Method is now 437 lines. Full suite: 1614 green.
…to step_metrics
Second sub-step of R1-7. handle_user_message 437 -> 390 lines.
- src/natshell/agent/step_metrics.py (new):
* build_metrics / build_run_stats (moved verbatim from loop.py;
re-exported by loop as _build_metrics / _build_run_stats)
* RunStats carrier for the per-run counters (formerly four locals)
* StepControl / StepOutcome + handle_degenerate_output /
handle_token_limit — the two continue/return outcome blocks
* strip_think_residue — reuses the shared grammar THINK_RE /
THINK_UNCLOSED_RE (verified byte-identical to the old inline
re.sub pair, incl. closed, unclosed and mixed cases)
- loop.py: inline outcome blocks replaced by handler calls; event text
and yield order byte-identical to the old code; message mutation
(append of the partial response) stays in the loop
- tests/test_step_metrics.py (new): 22 tests pinning the handlers,
the RunStats carrier and the think-strip equivalence
Final chunk of R1-7. handle_user_message 390 -> 252 lines (goal ~250),
so R1-7 is now DONE (was 790 lines at the start of the unit).
- src/natshell/agent/tool_dispatch.py (new):
* dispatch_tool_call — one tool call's lifecycle: normalize -> classify
-> confirm -> execute -> sudo retry -> guard observe -> budget hint
-> exchange append; buffers its events and returns stop (the guard's
observation) as a DispatchOutcome; event order + side effects
byte-identical to the old inline code
* step_budget_hint — the pure step-exhaustion suffix, now unit-testable
- loop.py: the 130-line for-tool_call body shrinks to a 17-line call
site; the guard's stop breaks the batch exactly like the old break
- tests/test_tool_dispatch.py (new): 15 tests (budget hints, execution,
blocked/declined/confirmed gates, argument repair, duplicate-abort stop)
R1-8. plan_executor.py 429 -> 75 lines: the model-facing prompt templates (plan generation, per-step, verify-fix retry) move to agent/plan_prompts.py with VERIFY_FIX_BUDGET; the pure helpers (_effective_plan_max_steps, validate_plan, plus the re-exported _shallow_tree) stay. plan_executor re-exports all moved names so the historical import paths (app.py, headless.py, tests) are untouched. Rendered prompts verified byte-identical against a pre-move snapshot (all 10 tiers/cases); full suite 1651 green.
Auto-fix applied via `ruff check --fix` (ruff 0.15.20, project config [tool.ruff] select=["E","F","I","W"]). All 19 CI-gate errors (8 × I001, 11 × F401) are resolved. Manual completion: ruff --fix split the four-line aliased re-export block at loop.py:52-63 into four single-line imports, leaving only the first line with `# noqa: E402`; added the same `# noqa` suppressor to the other three lines to silence the resulting E402 (three new errors the auto-fix would have introduced). Removed unused imports: - src/natshell/agent/loop.py: is_analysis_request, is_plan_request, Risk, ToolResult (re-export aliases + local re-imports) - src/natshell/agent/step_metrics.py: re - src/natshell/agent/sudo_retry.py: (I001 order) - src/natshell/inference/grammars/common.py: typing.Callable - src/natshell/inference/grammars/gemma.py: common.is_bare_tool_json (recovered via common.try_bare_json_recovery path) - src/natshell/inference/local.py: ToolCall, new_tool_call_id (re-export surface) - tests/test_step_metrics.py: re, StepOutcome - tests/test_tool_dispatch.py: pytest, ToolRegistry Suite: 1651 green. ruff: all checks pass.
…Engine protocol) Add the streaming seam R2 calls for: - StreamChunk dataclass + StreamingEngine (runtime_checkable) protocol in inference/engine.py — callers feature-detect via isinstance/hasattr. - LocalEngine.stream_completion: async generator that drains llama-cpp stream=True in a worker thread (one to_thread hop), yields raw text deltas as StreamChunks, then yields a CompletionResult produced by the SAME _parse_response pipeline as the blocking path (tool parsing, think-strip, degenerate suppression, ContextOverflowError mapping). - 7 tests in tests/test_streaming_local.py: chunk order, blocking/stream parity, tool-call parse on terminal result, think-residue strip, overflow surfacing, protocol conformance (LocalEngine conforms, RemoteEngine does not). TUI/headless token routing is a follow-up; R2-2 (parallel read-only tool calls) is next per plan.
…noted as follow-up)
dispatch_tool_batch groups one response's tool calls into segments: runs of PARALLEL_SAFE_TOOLS (list_directory, natshell_help, skill, fetch_url, kiwix_search — verified concurrency-free) execute via asyncio.gather; mutating / guard-stateful calls keep the historical one-at-a-time path. Events and exchange order stay the in-batch concatenation in batch order, identical to the serial loop; a guard stop still halts the remaining segments exactly as the old loop's break did (pinned by test_stop_halts_later_segments_in_batch), and the run continues to the next LLM step so the model sees the CRITICAL suffix. 12 new tests in tests/test_tool_dispatch.py; suite 1667 green.
The rendered tool-definition block previously re-rendered on every chat_completion/stream_completion call — byte-identical output, but a fresh string object each time, which churned the prompt prefix for nothing and worked against llama.cpp's RAM prompt cache. It is now memoized per (family, compact-tier, canonical-tools) in a bounded 32- entry LRU on the engine instance. Output is byte-identical to before (pinned against grammar.render_tools); the cache only removes the redundant re-render. 6 new tests in tests/test_tool_prefix_cache.py. Suite 1673 green.
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
1 participant
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.
No description provided.