From 94d8f7ea6d84ceea1cbf9eddc7c92518daf98c23 Mon Sep 17 00:00:00 2001 From: Vortex Dispatch Date: Thu, 16 Jul 2026 03:55:11 +0200 Subject: [PATCH] feat(config+preflight): warn that non-default models.qa LLM bindings are inert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QA stage is command-based (lint/build/test, engine/qa.go) — an LLM bound to the qa role is never called. Operators setting models.qa (e.g. codex/gpt-5.5) expected a review pass that never happened, and cost estimates looked wrong. The config still accepts the binding, but is no longer silent about it. - config.Config.Warnings(): non-fatal advisories; warns when models.qa differs from the shipped default (an explicit operator statement of intent), pointing at models.reviewer as the real LLM review binding. The default binding stays silent — warning every install is noise. - vxd config validate prints warnings before the PASSED line. - New qa_model preflight check (CheckQAModelInert / testable CheckQAModelInertWith), WARNING tier; AllChecks 18 → 19. - Tests: TestConfigValidate_QAModelInertWarning, TestPreflight_QAModelInertCheck; pinned counts updated (checks_test.go, TestAudit_PreflightCheckCounts). - Docs: README/CLAUDE.md/AGENTS.md check counts + Codex caveat note. Closes WEAKNESSES.md P0-02. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 2 +- CLAUDE.md | 6 +++--- README.md | 4 ++-- internal/cli/config_cmd.go | 4 ++++ internal/config/config.go | 21 +++++++++++++++++++ internal/config/config_test.go | 34 +++++++++++++++++++++++++++++++ internal/preflight/checks.go | 31 ++++++++++++++++++++++++++++ internal/preflight/checks_test.go | 31 ++++++++++++++++++++++++++-- test/audit_structural_test.go | 9 ++++---- 9 files changed, 130 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 193f481..46207d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -118,7 +118,7 @@ billing: | `vxd metrics` | Success rates, timing, escalations, SLA breaches per requirement | | `vxd estimate "req"` | Cost estimation with `--quick`, `--json`, `--rate` | | `vxd report ` | Client delivery report (`--html`, `--internal`) | -| `vxd preflight` | Run 18 pre-flight checks before dispatch | +| `vxd preflight` | Run 19 pre-flight checks before dispatch | | `vxd approve/reject` | Human review gates for PRs | | `vxd approve-plan` | Approve story plan before dispatch | | `vxd reject-plan` | Reject story plan | diff --git a/CLAUDE.md b/CLAUDE.md index da413bc..a28a03d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -179,7 +179,7 @@ dashboard: | `vxd metrics` | Success rates, timing, escalations, SLA breaches per requirement | | `vxd estimate "req"` | Cost estimation with `--quick`, `--json`, `--rate` | | `vxd report ` | Client delivery report (`--html`, `--internal`) | -| `vxd preflight` | Run 18 pre-flight checks before dispatch | +| `vxd preflight` | Run 19 pre-flight checks before dispatch | | `vxd approve ` | Approve a story PR for merge (`--all ` for batch) | | `vxd approve-plan` | Approve story plan before dispatch | | `vxd reject-plan` | Reject a plan with feedback | @@ -481,7 +481,7 @@ Requirements can reference figma.com design URLs; vxd builds the UI to match the An independent LLM audit (static analysis + test runs at `ebfa34c`) produced `docs/audit-findings-2026-07-05.md`; every finding was fixed or substantiated the same day — the full disposition table is in that file's **Resolution log** (§12), and `test/audit_structural_test.go` pins the closed gaps in CI. Highlights: - **Notifications wired (W-02/W-03):** `notify.FilteredNotifier` gates by EventType; a configured Slack webhook always sends `PIPELINE_STALLED`, `notify_on_sla` gates SLA breaches, `notify_on_complete` gates `REQ_COMPLETED`/`REQ_BLOCKED` (sent synchronously with a 10 s bound from `emitRequirementOutcome` — terminal event, a goroutine would die with the process). Allowlist built by `notificationAllowlist` (resume.go). - **Resume-path ownership restored (E-05+):** `rebuildDAG` was dropping `OwnedFiles`/`WaveHint`, silently disabling dispatcher overlap filtering + sequential serialization on every resume. Both carried through now; corrupt `owned_files` JSON sets `Story.OwnedFilesCorrupt` (transient) → forced `sequential` (unknown ownership must run alone). Tests: `sqlite_ownedfiles_test.go`, `rebuild_dag_ownership_test.go`. -- **Preflight grew to 18 checks (O-01/O-06):** `disk_space` (statfs on the state-dir filesystem, warn <1 GiB — event-log appends fail mid-requirement otherwise) and `tmux_server` (probes REAL session creation, catching stale sockets/TMUX_TMPDIR breakage that binary presence can't). Counts: AllChecks 18, DispatchChecks 11 — pinned by `TestAllChecks_Count`/`TestDispatchChecks_Count` + `TestAudit_PreflightCheckCounts`; update README/CLAUDE.md/AGENTS.md together when these change. +- **Preflight grew to 18 checks (O-01/O-06):** `disk_space` (statfs on the state-dir filesystem, warn <1 GiB — event-log appends fail mid-requirement otherwise) and `tmux_server` (probes REAL session creation, catching stale sockets/TMUX_TMPDIR breakage that binary presence can't). Counts: AllChecks 19 (18 + the `qa_model` inert-binding check), DispatchChecks 11 — pinned by `TestAllChecks_Count`/`TestDispatchChecks_Count` + `TestAudit_PreflightCheckCounts`; update README/CLAUDE.md/AGENTS.md together when these change. - **`security.require_scanners` (W-05):** opt-in strict mode — a story whose applicable scanners were skipped/failed BLOCKS (pause) instead of passing with reduced coverage. Default remains graceful-degrade (environment faults shouldn't halt the factory; loss is surfaced by the `security_scanners` preflight WARNING + gate logs). - **`make verify` reordered (E-01):** build → vet → test → doc-coverage → lint; `make lint` fails with install hints when `golangci-lint` is missing, so a bare dev box still gets full test signal before the gate fails. - **Autoresearch:** `evolve` CLI wired end-to-end to `ProgramMDEvolver` (human-gated PR; `TestWiring_AutoresearchEvolveCmd_Wired`), v1 baseline moved to neutral 0.5 (`TestAudit_AutoresearchBaseline`). @@ -502,7 +502,7 @@ An independent LLM audit (static analysis + test runs at `ebfa34c`) produced `do - **Automatic fallback:** the provider is wired as `CodexWithFallback` — on any Codex failure (CLI missing, rate limit, rejected model) it falls back to the Claude CLI on `claude-opus-4-7` (constant `codexFallbackModel` in `req.go`). The fallback substitutes the Anthropic model ID, since Codex and Anthropic use different IDs. - **Setup:** `npm i -g @openai/codex` then `codex login` (ChatGPT subscription). Verify: `codex exec -m gpt-5.5 --skip-git-repo-check -s read-only --ignore-user-config -o /tmp/x "say OK"`. - **Dedicated `reviewer` binding.** `models.reviewer` (optional `ModelConfig`, falls back to Senior when unset) lets the post-execution code reviewer use a provider distinct from Senior. This is the safe place to put `codex`: the reviewer is a post-execution LLM call, never spawned as a coding agent, whereas Senior IS spawned via the runtime (`executor.runtimeForRole` maps `anthropic→claude`, `google→gemini`, `openai→codex`; bare `codex` has no agent runtime). `resolveReviewerClient` (resume.go) builds it. Set `reviewer: {provider: codex, model: gpt-5.5}` to run review on GPT-5.5 while Senior stays on Anthropic. -- **Caveat — QA is command-based.** VXD's QA stage (`qa.go`) runs lint/build/test, not an LLM, so a `codex` binding on the `qa` role is inert until an LLM-QA path exists. The reviewer (`models.reviewer`/Senior) and manager (Manager model) are the LLM verification/diagnosis roles. +- **Caveat — QA is command-based.** VXD's QA stage (`qa.go`) runs lint/build/test, not an LLM, so a `codex` binding on the `qa` role is inert until an LLM-QA path exists. Non-default `models.qa` bindings now trigger a WARNING from `vxd config validate` (`Config.Warnings()`) and the `qa_model` preflight check (`CheckQAModelInert`), pointing at `models.reviewer`. The reviewer (`models.reviewer`/Senior) and manager (Manager model) are the LLM verification/diagnosis roles. ### Debugging Checklist (Pipeline Issues) 1. **Stories stuck in draft after escalation** → Check if SLA breach is killing them on resume. Reset `escalation_tier` in SQLite if needed. diff --git a/README.md b/README.md index 71198dd..2f285c5 100644 --- a/README.md +++ b/README.md @@ -236,7 +236,7 @@ vhs docs/demo.tape | `vxd dashboard status` | Show whether the always-on dashboard daemon is running (PID, port, URL). | | `vxd dashboard stop` | SIGTERM the always-on dashboard daemon and remove its pidfile (idempotent). | | `vxd watch [req-id]` | Terminal-friendly always-on status: tails events for one requirement (defaults to the newest in the current repo) until terminal status or Ctrl+C. | -| `vxd preflight` | Run pre-flight environment checks (18 checks, 3 severity tiers) | +| `vxd preflight` | Run pre-flight environment checks (19 checks, 3 severity tiers) | | `vxd estimate ` | Estimate cost (`--quick`, `--json`, `--rate`, `--save`) | | `vxd report ` | Generate client delivery report (`--html`, `--internal`, `--output`) | | `vxd figma auth` | One-time interactive session: store a Figma personal access token (validated, 0600) | @@ -521,7 +521,7 @@ internal/ improve/ Self-improvement engine (research, analysis, repo learning, revenue) llm/ LLM clients (Anthropic, OpenAI, Google AI, Claude CLI, Fallback) memory/ Memory dashboard + MemPalace integration - preflight/ Pre-flight validation (18 checks, 3 severity tiers) + preflight/ Pre-flight validation (19 checks, 3 severity tiers) repolearn/ 3-pass repo learning (static, git history, LLM deep) runtime/ Adapter/Runner pattern (tmux, Docker, SSH) scratchboard/ Shared memory across parallel agents diff --git a/internal/cli/config_cmd.go b/internal/cli/config_cmd.go index b0d0efb..5457c59 100644 --- a/internal/cli/config_cmd.go +++ b/internal/cli/config_cmd.go @@ -75,6 +75,10 @@ func runConfigValidate(cmd *cobra.Command, _ []string) error { return err } + for _, w := range cfg.Warnings() { + fmt.Fprintf(out, "WARNING: %s\n", w) + } + fmt.Fprintf(out, "Config validation PASSED.\n") return nil } diff --git a/internal/config/config.go b/internal/config/config.go index 3becf50..a3049c3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -448,6 +448,27 @@ type DevDBDockerConfig struct { Host string `yaml:"host"` // default "localhost"; override for Colima/VM setups (e.g. "192.168.64.3") } +// Warnings returns non-fatal configuration advisories: settings that are +// accepted but do not do what an operator plausibly expects. Surfaced by +// `vxd config validate` and the qa_model preflight check. +func (c Config) Warnings() []string { + var warnings []string + + // models.qa is inert: the QA stage runs lint/build/test commands + // (engine/qa.go), not an LLM. An operator who explicitly binds the qa + // role (any binding different from the shipped default) expects a + // review pass that never happens — and cost estimates look wrong. + def := DefaultConfig().Models.QA + qa := c.Models.QA + if qa.Provider != "" && (qa.Provider != def.Provider || qa.Model != def.Model) { + warnings = append(warnings, fmt.Sprintf( + "models.qa is bound to %s/%s, but the QA stage is command-based (lint/build/test) and never calls an LLM — this binding is inert. For an LLM review pass, bind models.reviewer instead (see README: Configuration).", + qa.Provider, qa.Model)) + } + + return warnings +} + // Validate checks that all configuration values are within allowed ranges. // It returns an error describing the first invalid value found. func (c Config) Validate() error { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0435943..0abd8d0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -566,3 +566,37 @@ sla: t.Errorf("bare int key: expected [5]=240, got %d", cfg2.SLA.MaxMinutesPerComplexity[5]) } } + +// TestConfigValidate_QAModelInertWarning pins the models.qa advisory: an +// operator-set (non-default) QA binding produces a warning that the QA stage +// is command-based, while the shipped default binding stays silent. +func TestConfigValidate_QAModelInertWarning(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Models.QA = config.ModelConfig{Provider: "codex", Model: "gpt-5.5"} + + warnings := cfg.Warnings() + found := false + for _, w := range warnings { + if strings.Contains(w, "models.qa") && strings.Contains(w, "inert") { + found = true + if !strings.Contains(w, "models.reviewer") { + t.Errorf("warning should point to models.reviewer as the LLM review path: %q", w) + } + } + } + if !found { + t.Fatalf("non-default models.qa binding must produce an inert-binding warning, got %v", warnings) + } + + // Default config: no warnings (the shipped default binding is not an + // operator statement of intent — warning every install is noise). + if w := config.DefaultConfig().Warnings(); len(w) != 0 { + t.Errorf("config.DefaultConfig().Warnings() = %v, want none", w) + } + + // Empty provider (role unset entirely) also stays silent. + cfg.Models.QA = config.ModelConfig{} + if w := cfg.Warnings(); len(w) != 0 { + t.Errorf("empty models.qa must not warn, got %v", w) + } +} diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index de0dc65..f1aec42 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -360,6 +360,36 @@ func CheckSecurityScanners(lookPath func(string) (string, error)) Result { strings.Join(missing, ", "), strings.Join(hints, " && "))} } +// CheckQAModelInert warns when models.qa is bound to a non-default LLM: the +// QA stage is command-based (lint/build/test, engine/qa.go) and never calls +// an LLM, so the binding is inert — the operator expects a review pass that +// won't happen. Delegates to CheckQAModelInertWith after loading the active +// config the same way CheckBillingConfig does. +func CheckQAModelInert() Result { + cfg := config.DefaultConfig() + if c, err := config.LoadFromFile("vxd.yaml"); err == nil { + cfg = c + } else if home := os.Getenv("HOME"); home != "" { + if c, err := config.LoadFromFile(filepath.Join(home, ".vxd", "config.yaml")); err == nil { + cfg = c + } + } + return CheckQAModelInertWith(cfg) +} + +// CheckQAModelInertWith is the config-injected core of CheckQAModelInert, +// split out so it can be unit-tested without touching the filesystem. +func CheckQAModelInertWith(cfg config.Config) Result { + for _, w := range cfg.Warnings() { + if strings.Contains(w, "models.qa") { + return Result{Name: "qa_model", Severity: SeverityWarning, Passed: false, + Message: w + " Docs: README.md Configuration table (models.reviewer)."} + } + } + return Result{Name: "qa_model", Severity: SeverityWarning, Passed: true, + Message: "models.qa binding OK (QA stage is command-based; no inert LLM binding)"} +} + // --- Check sets --- // DispatchChecks returns the checks run before every dispatch operation. @@ -383,6 +413,7 @@ func AllChecks() []Check { return append(DispatchChecks(), binaryCheck, scannerCheck, CheckConfig, CheckProject, CheckStateDir, CheckBillingConfig, CheckOllama, + CheckQAModelInert, ) } diff --git a/internal/preflight/checks_test.go b/internal/preflight/checks_test.go index 991e0e3..d435773 100644 --- a/internal/preflight/checks_test.go +++ b/internal/preflight/checks_test.go @@ -199,8 +199,8 @@ func TestDispatchChecks_Count(t *testing.T) { func TestAllChecks_Count(t *testing.T) { checks := preflight.AllChecks() - if len(checks) != 18 { - t.Fatalf("expected 18 total checks, got %d", len(checks)) + if len(checks) != 19 { + t.Fatalf("expected 19 total checks, got %d", len(checks)) } } @@ -324,3 +324,30 @@ func TestAllChecksWithConfig_AddsTwoDevDBChecks(t *testing.T) { t.Errorf("AllChecksWithConfig should add 2 checks, got %d vs base %d", full, base) } } + +// TestPreflight_QAModelInertCheck pins the qa_model preflight check: a +// non-default models.qa binding fails at WARNING tier with the inert-binding +// message, the default binding passes. +func TestPreflight_QAModelInertCheck(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Models.QA = config.ModelConfig{Provider: "codex", Model: "gpt-5.5"} + + r := preflight.CheckQAModelInertWith(cfg) + if r.Name != "qa_model" { + t.Fatalf("name = %q, want qa_model", r.Name) + } + if r.Severity != preflight.SeverityWarning { + t.Errorf("severity = %v, want warning", r.Severity) + } + if r.Passed { + t.Error("non-default models.qa binding must fail the check") + } + if !strings.Contains(r.Message, "inert") || !strings.Contains(r.Message, "models.reviewer") { + t.Errorf("message should explain inertness and point to models.reviewer: %q", r.Message) + } + + ok := preflight.CheckQAModelInertWith(config.DefaultConfig()) + if !ok.Passed { + t.Errorf("default config must pass, got: %s", ok.Message) + } +} diff --git a/test/audit_structural_test.go b/test/audit_structural_test.go index d5977f5..9696a9b 100644 --- a/test/audit_structural_test.go +++ b/test/audit_structural_test.go @@ -34,11 +34,12 @@ func readRepoFile(t *testing.T, rel string) string { // TestAudit_PreflightCheckCounts pins the preflight check inventory so doc // drift (the audit found AGENTS.md "12", README "15" vs code 16) is caught by // CI. Counts moved 16→18 / 9→11 when the disk_space and tmux_server checks -// landed (audit findings O-01/O-06) — README, CLAUDE.md, and AGENTS.md must -// state these numbers. +// landed (audit findings O-01/O-06), then 18→19 with the qa_model inert-binding +// check (WEAKNESSES.md P0-02) — README, CLAUDE.md, and AGENTS.md must state +// these numbers. func TestAudit_PreflightCheckCounts(t *testing.T) { - if got := len(preflight.AllChecks()); got != 18 { - t.Errorf("AllChecks() = %d, want 18 (README CLI table)", got) + if got := len(preflight.AllChecks()); got != 19 { + t.Errorf("AllChecks() = %d, want 19 (README CLI table)", got) } if got := len(preflight.DispatchChecks()); got != 11 { t.Errorf("DispatchChecks() = %d, want 11", got)