diff --git a/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md b/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md index 731c8df3..c6e768e9 100644 --- a/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md +++ b/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md @@ -250,7 +250,59 @@ The "project memory is team memory" payoff (557dd39's Part IV addition) is the n ## §5 Provider expansion -(filled in by Phase 6 — covers `bdfa2a6`, `5075f6e`, `2edc7ee`) +**Source:** nagent `bdfa2a6`, `5075f6e`, `2edc7ee` (`bin/helpers/nagent_llm.py:13-19` + `:27-31` + `:37-42` + `:54-77` + `:123-130` + `:198-279` + `:315-336` + `:381-400` + `:582-625` + `:739-770` + `:357-391`, `bin/nagent:1075-1081`, `config.example.json:7`, `README.md:82-90` + `:956-967` + `:991-995`, `tests/test_nagent.py:1010-1042` + `:2734-2797`, `context/data-oriented-design.md`). +**One-liner:** Together is added as a sixth provider (OpenAI-wire-compatible, always streamed). Per-model context windows become a verified table; rebuild now fires on whichever trips first — byte ceiling or 0.85 of the model's window. The claude-code provider blanks inherited `ANTHROPIC_API_KEY` so its billing stays on its own login; the spinner names the provider/model. +**Pattern(s) vs v2.3:** UPDATE. v2.3 had 5 providers (openai, anthropic, google, cursor, claude-code); v3 has 6 (adds together). The v2.3 review noted v2.3 had 5 providers per the project's tech-stack.md — Manual Slop has 8 (per the qwen_llama_grok track); the count is independent of the abstraction. The token-cap awareness is NEW (v2.3 had byte-only rebuild triggers). v2.3 §5 ("the loop") is extended with a per-model token cap as a second rebuild trigger. +**Manual Slop implications:** Manual Slop's `src/ai_client.py` already has per-provider history locks (per `docs/guide_ai_client.md`) but does not have a per-model context-window table; the rebuild/compaction is currently driven by heuristic token estimates. The pattern "verify the window, don't guess; only assert what you've tested" maps to Manual Slop's `provider_state` architecture (per `docs/guide_ai_client.md`). The claude-code billing quirk (`env={"ANTHROPIC_API_KEY": ""}`) is a specific gotcha worth documenting — Manual Slop's claude-code integration (per tech-stack.md) may benefit from the same discipline. +**Decision candidate:** NEW Candidate 21 (MEDIUM). "Per-model token-cap awareness for Manual Slop `ai_client`": add `MODEL_CONTEXT_WINDOWS` table; rebuild fires on byte ceiling OR 0.85 of window; "don't guess" — omit rather than estimate. See `decisions.md` Candidate 21. +**Cross-refs:** §2 Conversation safety net (rebuild trigger gets a second condition); §3 Hooks (per-turn status can include `current model / window / usage`). +**Source-read citations:** +- `bin/helpers/nagent_llm.py:13-19` — `PROVIDERS` extended + `TOGETHER_BASE_URL` (bdfa2a6) +- `bin/helpers/nagent_llm.py:27-31` — `DEFAULT_MODELS["together"]` (bdfa2a6) +- `bin/helpers/nagent_llm.py:37-42` — `CREDENTIAL_ENV["together"]` = `("TOGETHER_API_KEY",)` (bdfa2a6) +- `bin/helpers/nagent_llm.py:54-77` — `MODEL_CONTEXT_WINDOWS` table (10 verified models) (bdfa2a6) +- `bin/helpers/nagent_llm.py:123-130` — `model_context_window(model)` returns `None` for unknown (bdfa2a6) +- `bin/helpers/nagent_llm.py:198-279` — Together client + `_together_chat` (always streamed) (bdfa2a6) +- `bin/helpers/nagent_llm.py:315-336` — `list_models("together")` — direct fetch because Together returns a bare JSON array (bdfa2a6) +- `bin/helpers/nagent_llm.py:381-400` — `list_providers()` — static catalog, no network (bdfa2a6) +- `bin/helpers/nagent_llm.py:582-625` — Together in `generate_text_with_usage` + `generate_with_upload_usage` (bdfa2a6) +- `bin/helpers/nagent_llm.py:739-770` — `_together_upload` — image-upload only, base64 data URL (bdfa2a6) +- `bin/helpers/nagent_llm.py:357-391` — `env={"ANTHROPIC_API_KEY": ""}` + error-result-survives-stream-exception + synthetic-error-text-skip (5075f6e) +- `bin/nagent:1075-1081` — `target = f"{llm.provider}/{llm.model}" if llm.model else llm.provider` (2edc7ee) +- `config.example.json:7` — `"context_window_tokens": 0` (bdfa2a6) +- `README.md:82-90` — providers table extension (bdfa2a6) +- `README.md:956-967` — "Conversation rebuilt (compacted...) when **either** trigger fires first" (bdfa2a6) +- `README.md:991-995` — `--list-providers` CLI example (bdfa2a6) +- `tests/test_nagent.py:1010-1042` — `test_call_llm_wait_spinner_names_provider_and_model` (2edc7ee) +- `tests/test_nagent.py:2734-2797` — 4 new claude-code tests (5075f6e) +**Honest gaps in this cluster:** +- `MODEL_CONTEXT_WINDOWS` is verified against the Together API only on 2026-06-17. Other providers' models are intentionally omitted. A future track should add more verifications. +- The `env={"ANTHROPIC_API_KEY": ""}` blanking assumes subprocess env takes precedence over inherited env. Correct on POSIX; Windows env handling could differ. Unverified. +- The Together `/v1/models` direct fetch at `bin/helpers/nagent_llm.py:315-336` is a vendor-specific workaround. If Together changes the response shape, the parser silently returns fewer models. A defensive check (count returned models, warn if zero) could harden this. + +**Pattern deep-dive.** The provider-expansion abstraction is a four-piece composition: **register**, **window**, **trigger**, **bill**. Register: a provider is one tuple in `PROVIDERS` + one entry in `DEFAULT_MODELS` + one tuple in `CREDENTIAL_ENV` + one entry in `PACKAGE_HINTS`. The 5-tuple is enough to surface a provider in `--list-providers` and route a `generate_text_with_usage` call. Window: `MODEL_CONTEXT_WINDOWS` is a verified table, not an estimate. "Omit rather than guessed" (per `bin/helpers/nagent_llm.py:60-62`) is the discipline — the table at `bin/helpers/nagent_llm.py:54-77` lists exactly the models whose windows were verified by API error or by direct lookup, and the function `model_context_window` returns `None` for unknowns (the caller falls back to byte-only behavior). Trigger: rebuild fires on whichever trips first, the byte ceiling OR 0.85 of the model's window (per `README.md:956-967`). The 0.85 safety fraction is the data-oriented response to "model capability degrades under high context utilization, not just at the limit" (per the issues/0004 spec). Bill: the claude-code billing quirk (`env={"ANTHROPIC_API_KEY": ""}`) is the discipline "API-key billing stays the anthropic provider's job" (per `bin/helpers/nagent_llm.py:361-364`) — billing is data; the provider that owns the billing owns the env. + +The token-cap awareness is the load-bearing change. A byte-only rebuild trigger is a proxy for token utilization, and the proxy fails on small-window models — `rebuild_at_kb: 384` is far too high to fire on a 8192-token model. The per-model window table is the data-grounded alternative. The `context_window_tokens` config key (per `config.example.json:7`) is the extension point: a user who wants a new model's window can add it without code change. The "unknown returns None" behavior at `bin/helpers/nagent_llm.py:123-130` is the discipline — a missing entry is not a default to a guess; it's a signal to fall back to the byte-only behavior, which is correct for large-window models and merely late for small-window models (the failure is visible, not silent). + +The `bdfa2a6` commit message is explicit about the verification process: "DeepSeek-V4-Pro confirmed by a context_length_exceeded error ('maximum context length is 512000 tokens'). Qwen3.7-Plus/Max advertise context_length=1000000, but an oversized request is rejected with 'Range of input length should be [1, 983616]' — so the enforced input cap is 983616, with ~16384 of the 1M reserved for output." The distinction between "advertised total context_length" and "enforced input cap" is load-bearing — the table records the enforced cap, not the advertisement. This is the same data discipline as the project's `conductor/code_styleguides/cache_friendly_context.md`: stable data (verified numbers) vs volatile data (advertised numbers). + +A code-shape sketch using survey grammar: + +``` +providers := { name: string, default_model: string, + credentials: [env-var], package: string, + context_window: int | nil } // [M] mutable aggregate +provider { name, model, env } :: LlmResult {ssdl} [B] // boundary + // SDK call; failures surface text + exit code + +rebuild-trigger { conversation_chars, model, settings } :: fire? {ssdl} [I] + byte_trip := conversation_chars > settings.rebuild_at_kb * 1024 + window_trip := model_context_window(model) + and tokens > window * CONTEXT_WINDOW_SAFETY_FRACTION + byte_trip or window_trip +``` + +The `{ssdl}` markers note the abstractions: the provider call is a boundary (B) where SDK errors become LlmResult errors; the rebuild trigger is an inspectable invariant (I) computed from data on disk. ## §6 Delegation rewrite