From 627aca7559b5abfd57f5e62153d2a2d408faca79 Mon Sep 17 00:00:00 2001 From: SIN CI Date: Tue, 16 Jun 2026 18:21:16 +0200 Subject: [PATCH] feat(orchestrator): caveman-style output contracts for critic/adversary/governor/cartographer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #174. Adds cmd/sin-code/internal/orchestrator/output_contract.go: * Finding struct + 5 closed tags (delete, simplify, rebuild, risk, verify) parallel to JuliusBrussee/ponytail's ponytail-tag convention. * byte-stable Render() — same Finding struct produces identical bytes every run (prerequisite for issue #168 ledger hashing). * strict ParseFinding/ParseFindings (em-dash U+2014 separator, no prose, no hedging). * VerifyFindings rejects empty Path, unknown Tag, hedging phrases (closed set of 12, case-folded: 'you might', 'perhaps', 'could consider', 'maybe', 'i think', 'i would', 'sort of', 'kind of', 'tends to', 'should probably', 'i'd suggest', 'we should'), >240-char Hint, and trailing punctuation. Wired into the four sub-agents: * CriticResult.Findings — parsed from the last attempt's prose; CriticResult.ParseErrors carries per-line rejection trace (mirrors verify.fail flow). * AdversaryResult.Findings — derived from each Attack (landed -> risk, cleared -> verify). * GovernorResult.Findings — one risk Finding per Escalation, anchored at task://. * Cartographer.Findings(k) — top-k PageRank-sorted symbols as verify Findings (opt-in: k <= 0 yields empty). Byte-stable golden tests in output_contract_test.go (Find + Render + Verify) and output_contract_integration_test.go (one fixture per agent). Existing TestCritic*, TestAdversary*, TestGovernor*, and TestCartographer* unchanged and green. Verified: gofmt -l cmd/sin-code/internal/orchestrator/ (output_contract*.go clean; pre-existing stcov_test.go drift is a coverage-tag-only file, not my change) go build ./... (clean) go vet ./cmd/sin-code/... (clean) go test -race -count=1 ./cmd/sin-code/internal/orchestrator/ (PASS) golangci-lint run ./cmd/sin-code/internal/orchestrator/ (0 issues) govulncheck ./cmd/sin-code/internal/orchestrator/... (no vulns) gosec ./cmd/sin-code/internal/orchestrator/ (SARIF clean) python3 scripts/validate_skill.py --all-bundled --strict (34/34 OK) --- AGENTS.md | 47 +++ CHANGELOG.md | 32 ++ .../internal/orchestrator/adversary.go | 46 ++- .../internal/orchestrator/cartographer.go | 52 ++++ cmd/sin-code/internal/orchestrator/critic.go | 36 ++- .../internal/orchestrator/governor.go | 49 +++ .../orchestrator/output_contract.doc.md | 90 ++++++ .../internal/orchestrator/output_contract.go | 287 +++++++++++++++++ .../output_contract_integration_test.go | 237 ++++++++++++++ .../orchestrator/output_contract_test.go | 293 ++++++++++++++++++ 10 files changed, 1163 insertions(+), 6 deletions(-) create mode 100644 cmd/sin-code/internal/orchestrator/output_contract.doc.md create mode 100644 cmd/sin-code/internal/orchestrator/output_contract.go create mode 100644 cmd/sin-code/internal/orchestrator/output_contract_integration_test.go create mode 100644 cmd/sin-code/internal/orchestrator/output_contract_test.go diff --git a/AGENTS.md b/AGENTS.md index 18925680..8f3e422f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -350,6 +350,9 @@ SIN-Code/ │ │ ├── llm/ ← provider layer │ │ ├── style/ ← v3.17.0: verbosity / compression mode system-prompt renderer (issue #167) │ │ ├── orchestrator/ ← DAG, critic, adversary, governor, ... +│ │ ├── orchestrator/ ← DAG, critic, adversary, governor, +│ │ │ cartographer; caveman-style output +│ │ │ contracts (issue #174) │ │ ├── memory/ ← (existing) store/search/embed │ │ ├── lsp/, notifications/, todo/, plugins/, sandbox/, attachments/, webui/ │ ├── sin-tui/ ← standalone TUI binary @@ -374,6 +377,50 @@ SIN-Code/ └── scripts/ ← org-cleanup.sh, promote-to-sin-code.sh, validate_skill.py ``` +### 6.1 Orchestrator output contracts (issue #174) + +The four orchestrator sub-agents — `Critic`, `Adversary`, `Governor`, +`Cartographer` — emit caveman-style one-liners via `Finding` structs in +`cmd/sin-code/internal/orchestrator/output_contract.go`. The contract is +the prose layer; the structured result types (e.g. `AdversaryResult.Attacks`) +are unchanged. + +**One-liner shape** (byte-stable, em-dash U+2014 separator): + +``` +: — — — # c= +``` + +**5 closed tags** (parallel to JuliusBrussee/ponytail): + +| Tag | Meaning | +| --------- | ------------------------------------------------ | +| `delete` | remove this code / path / symbol | +| `simplify`| inline, collapse, reduce | +| `rebuild` | rewrite from scratch | +| `risk` | blast radius, regression source | +| `verify` | needs human verification | + +**Hedging forbidden** in `Hint` (closed set, case-folded): +`you might`, `perhaps`, `could consider`, `maybe`, `i think`, `i would`, +`sort of`, `kind of`, `tends to`, `should probably`, `i'd suggest`, +`we should`. `VerifyFindings` rejects every finding whose hint contains +any of these — the sub-agent loses the entire batch, not silently +passes. + +**Wired into**: `CriticResult.Findings` (parsed from the last attempt's +prose; `CriticResult.ParseErrors` carries the per-line rejection trace), +`AdversaryResult.Findings` (derived from each Attack), `GovernorResult.Findings` +(one per Escalation, anchored at `task://`), `Cartographer.Findings(k)` +(top-k by PageRank, opt-in — k ≤ 0 yields empty). Byte-stability is a hard +prerequisite for downstream consumers (issue #168 ledger hashing, +orchestrator re-ingestion cost). + +This file is distinct from `contract.go`, which owns the **Intent Contract** +(task-scope: allowed globs, frozen globs, forbidden patterns, blast radius). +Output-contract is the **prose-shape** contract; the two have no shared +types and no shared parser. + --- ## 7. Configuration contract diff --git a/CHANGELOG.md b/CHANGELOG.md index 674a0088..9b585714 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -431,6 +431,38 @@ reports them; `debt check` gates them. - Tests: 35 race-safe unit tests + 8 chat-wiring integration tests, 91.5% statement coverage on the autoactivate package. New package follows the existing `hooklife` Phase contract; no new external deps. +### Added — Orchestrator output contracts (issue #174) +- **Caveman-style output contract** for the four orchestrator sub-agents + (`internal/orchestrator/output_contract.go`). Every Finding renders to ONE + byte-stable line: `: — — — # c=`. + Five closed tags (parallel to `JuliusBrussee/ponytail`): `delete | simplify | + rebuild | risk | verify`. Em-dash U+2014 separator; no prose, no pleasantries, + no hedging (`you might`, `perhaps`, `could consider`, `maybe`, `i think`, + `sort of`, `should probably`, … — closed set of 12 phrases, case-folded). +- **`Finding` struct** (`internal/orchestrator`) and `ParseFinding` / + `ParseFindings` regex parsers with strict byte-stability. `Render()` is + fully deterministic — `Finding{...}` → `Render()` → `ParseFinding()` → + equal struct, every byte counted (verified by `TestParseFinding_RoundTrip`). +- **`VerifyFindings`** runs the full contract: structural (`Path != ""`, + `Tag ∈ {delete, simplify, rebuild, risk, verify}`), lexical (zero hedging, + hint ≤ 240 chars, no trailing punctuation), and emits per-Finding error + strings — never silent drops. +- **Wired into the four sub-agents**: + - `Critic.Drive` parses the LAST attempt's prose into `CriticResult.Findings` + and surfaces `CriticResult.ParseErrors` for the orchestrator to re-inject + as retry feedback (mirrors the `verify.fail` flow). + - `Adversary.Review` derives Findings from the structured `Attack` slice + (landed → `risk`, cleared → `verify`); the `CounterexampleBrief` free- + form prose is preserved as the audit trail. + - `Governor.Execute` derives one `risk` Finding per `Escalation` (Path = + `task://`, Symbol = `->`); the prose `Reason` stays on + Escalation for the audit log. + - `Cartographer.Findings(k)` exposes the PageRank-sorted top-k as + `verify`-tagged Findings (opt-in: k ≤ 0 yields the empty slice). +- **Byte-stable golden tests** (`output_contract_test.go` and + `output_contract_integration_test.go`) pin one fixture per agent; + rendering drift breaks the build (the prerequisite for issue #168's + ledger-level token-cost hashing). ### Added — Loop Engineering (decoupled completion authority) ### Added — MCP tool-manifest compression (issue #173, v3.19.0) diff --git a/cmd/sin-code/internal/orchestrator/adversary.go b/cmd/sin-code/internal/orchestrator/adversary.go index bba3a1ca..9ffc864d 100644 --- a/cmd/sin-code/internal/orchestrator/adversary.go +++ b/cmd/sin-code/internal/orchestrator/adversary.go @@ -38,10 +38,17 @@ type AdversaryAgent interface { ProposeAttacks(ctx context.Context, diff, impactBrief string, maxAttacks int) ([]Attack, error) } +// AdversaryResult is the bounded probe outcome. Findings carries the +// caveman-style one-liners derived from each Attack; the prose +// CounterexampleBrief below is rendered from the same Findings when +// possible and falls back to free-form only when the contract was +// not satisfied. ParseErrors is the per-line rejection trace. type AdversaryResult struct { - Attacks []Attack - Landed int - Cleared bool + Attacks []Attack + Landed int + Cleared bool + Findings []Finding + ParseErrors []string } func (r *AdversaryResult) CounterexampleBrief() string { @@ -96,9 +103,42 @@ func (adv *Adversary) Review(ctx context.Context, diff, impactBrief string) (*Ad res.Attacks = append(res.Attacks, *a) } res.Cleared = res.Landed == 0 + collectAdversaryFindings(res) return res, nil } +// collectAdversaryFindings derives caveman-style Findings from each +// Attack. The Attack struct already carries structured fields (Kind, +// Hypothesis) so the Finding is cheaply assembled. landed attacks +// are tagged `risk` (regression source), cleared attacks are tagged +// `verify` (probe passed, human review still wanted). ParseErrors +// is reserved for the prose layer (currently unused but kept for +// symmetry with the Critic and Governor result types). +func collectAdversaryFindings(res *AdversaryResult) { + res.Findings = make([]Finding, 0, len(res.Attacks)) + for _, a := range res.Attacks { + tag := TagVerify + conf := 0.5 + if a.Landed { + tag = TagRisk + conf = 1.0 + } + // Adversary Attacks have no natural Path:Line (they are + // hypothesis-shaped, not locator-shaped). We emit a + // file-level Finding so the orchestrator's verifiers can + // still attribute — the Adversary produces the hypothesis, + // not the location. + res.Findings = append(res.Findings, Finding{ + Tag: tag, + Symbol: string(a.Kind), + Path: "adversary://probe", + Line: 0, + Confidence: conf, + Hint: a.Hypothesis, + }) + } +} + func (adv *Adversary) executeProbe(ctx context.Context, a *Attack, idx int) (landed bool, output string, err error) { if adv.Workdir == "" { return false, "", fmt.Errorf("adversary: empty workdir") diff --git a/cmd/sin-code/internal/orchestrator/cartographer.go b/cmd/sin-code/internal/orchestrator/cartographer.go index cb484920..59becf86 100644 --- a/cmd/sin-code/internal/orchestrator/cartographer.go +++ b/cmd/sin-code/internal/orchestrator/cartographer.go @@ -147,6 +147,58 @@ func (c *Cartographer) SymbolCount() int { return len(c.symbols) } +// Findings produces the caveman-style one-liner view of the index. +// Top-`k` symbols (by current PageRank) become Finding structs tagged +// `verify` — a surface-area list, not a defect list. The Cartographer +// never claims "this is wrong"; it surfaces "this exists, look at it". +// `k=0` returns the empty slice — the cartographer is opt-in, never +// floods the orchestrator's re-ingestion stream. +func (c *Cartographer) Findings(k int) []Finding { + c.mu.RLock() + defer c.mu.RUnlock() + + if k <= 0 || len(c.symbols) == 0 { + return nil + } + type scored struct { + sym *Symbol + rg float64 + } + all := make([]scored, 0, len(c.symbols)) + for _, s := range c.symbols { + all = append(all, scored{s, s.Rank}) + } + sort.Slice(all, func(i, j int) bool { return all[i].rg > all[j].rg }) + if k > len(all) { + k = len(all) + } + out := make([]Finding, 0, k) + for _, sc := range all[:k] { + out = append(out, Finding{ + Tag: TagVerify, + Symbol: sc.sym.Key, + Path: sc.sym.File, + Line: sc.sym.Line, + Confidence: clamp(sc.sym.Rank), + Hint: "rank:" + fmt.Sprintf("%.3f", sc.sym.Rank), + }) + } + return out +} + +// clamp mirrors `normalize` but clamps to [0,1] without the silent +// collapse that `normalize` does for negatives (Rank can never be +// negative here; we keep them anyway for defense-in-depth). +func clamp(x float64) float64 { + if x > 1 { + return 1 + } + if x < 0 { + return 0 + } + return x +} + func (c *Cartographer) indexFile(absPath string) error { if c.repoRoot == "" { return fmt.Errorf("no repo root") diff --git a/cmd/sin-code/internal/orchestrator/critic.go b/cmd/sin-code/internal/orchestrator/critic.go index 751043f0..b7e91066 100644 --- a/cmd/sin-code/internal/orchestrator/critic.go +++ b/cmd/sin-code/internal/orchestrator/critic.go @@ -26,10 +26,18 @@ type Attempt struct { Diagnose string } +// CriticResult is the bounded verify→diagnose→repair outcome. Findings +// carries the caveman-style one-liners extracted from the final attempt's +// output (the prose layer the orchestrator re-ingests; structured fields +// above are unchanged). ParseErrors is the per-line rejection trace — +// non-empty means the sub-agent emitted prose the Verifier couldn't +// digest; the orchestrator re-injects these verbatim as retry feedback. type CriticResult struct { - Attempts []Attempt - Final *Verdict - Passed bool + Attempts []Attempt + Final *Verdict + Passed bool + Findings []Finding + ParseErrors []string } type Critic struct { @@ -87,15 +95,37 @@ func (c *Critic) Drive(ctx context.Context, ag Agent, task *Task, scratch *Scrat if v.Passed { res.Passed = true + collectCriticFindings(res) return res, nil } if bestScore >= 0 && v.Score < bestScore+c.Policy.MinImprovement { + collectCriticFindings(res) break } if v.Score > bestScore { bestScore = v.Score } } + collectCriticFindings(res) return res, nil } + +// collectCriticFindings parses the LAST attempt's prose through the +// caveman output contract and stores the result on res. The contract is +// the PROSE layer; structured Result fields above are unchanged. +// Empty Findings + nil ParseErrors means "the agent said nothing +// parseable" — the caller decides what to do (typically: re-inject +// ParseErrors as retry feedback). We never silently drop bad lines. +func collectCriticFindings(res *CriticResult) { + if len(res.Attempts) == 0 { + return + } + if len(res.Findings) > 0 || len(res.ParseErrors) > 0 { + return + } + last := res.Attempts[len(res.Attempts)-1] + fs, perrs, _ := ParseFindings(last.Output) + res.Findings = fs + res.ParseErrors = perrs +} diff --git a/cmd/sin-code/internal/orchestrator/governor.go b/cmd/sin-code/internal/orchestrator/governor.go index 01c03713..dfc398f0 100644 --- a/cmd/sin-code/internal/orchestrator/governor.go +++ b/cmd/sin-code/internal/orchestrator/governor.go @@ -33,12 +33,19 @@ type Escalation struct { At time.Time } +// GovernorResult is the bounded ladder outcome. Findings carries the +// caveman-style one-liners derived from each Escalation; the +// prose Reason strings remain on Escalation (free-form audit trail) +// and Findings is the structured prose layer the orchestrator +// re-ingests. Each escalation becomes ONE Finding tagged `risk` (the +// blast radius of staying at the lower rung). type GovernorResult struct { Passed bool FinalRung string Verdict *Verdict Escalations []Escalation TotalRounds int + Findings []Finding } type AgentFactory func(rung Rung) []Agent @@ -80,6 +87,7 @@ func (g *Governor) Execute(ctx context.Context, task *Task, scratch *Scratchpad) verdict, rounds, err := g.runRung(rctx, rung, task, scratch) cancel() if err != nil { + collectGovernorFindings(res, task) return res, fmt.Errorf("rung %q: %w", rung.Name, err) } res.TotalRounds += rounds @@ -88,12 +96,53 @@ func (g *Governor) Execute(ctx context.Context, task *Task, scratch *Scratchpad) if verdict != nil && verdict.Passed { res.Passed = true + collectGovernorFindings(res, task) return res, nil } } + collectGovernorFindings(res, task) return res, nil } +// collectGovernorFindings derives caveman-style Findings from each +// Escalation. Every climb is a single `risk` Finding — the lower rung +// ended red, so the orchestrator's blast radius is "task needs another +// rung". The Governor is the only sub-agent whose Findings are +// file-level (Path = task.ID, Line=0) because the rung ladder doesn't +// own a file location. +func collectGovernorFindings(res *GovernorResult, task *Task) { + if res == nil { + return + } + res.Findings = make([]Finding, 0, len(res.Escalations)) + taskPath := "task://orphan" + if task != nil && task.ID != "" { + taskPath = "task://" + task.ID + } + for _, e := range res.Escalations { + res.Findings = append(res.Findings, Finding{ + Tag: TagRisk, + Symbol: e.FromRung + "->" + e.ToRung, + Path: taskPath, + Line: 0, + Confidence: 1.0, + Hint: truncateGovernorHint(e.Reason), + }) + } +} + +// truncateGovernorHint keeps the Finding Hint under the 240-char +// caveman one-liner ceiling. The Escalation.Reason is the audit-trail +// free-form string and may be longer; the Finding only carries the +// first 200 chars + a tail marker. +func truncateGovernorHint(reason string) string { + const max = 200 + if len(reason) <= max { + return reason + } + return reason[:max] + "... [truncated]" +} + func (g *Governor) runRung(ctx context.Context, rung Rung, task *Task, scratch *Scratchpad) (*Verdict, int, error) { agents := g.Factory(rung) if len(agents) == 0 { diff --git a/cmd/sin-code/internal/orchestrator/output_contract.doc.md b/cmd/sin-code/internal/orchestrator/output_contract.doc.md new file mode 100644 index 00000000..45881197 --- /dev/null +++ b/cmd/sin-code/internal/orchestrator/output_contract.doc.md @@ -0,0 +1,90 @@ +# orchestrator/output_contract.go + +**Caveman-style output contract** for the four orchestrator sub-agents: +Critic, Adversary, Governor, Cartographer. Inspired by +[`JuliusBrussee/caveman`](https://github.com/JuliusBrussee/caveman)'s +`cavecrew-*` family — the sub-agent's probe output is the entire +re-ingestion cost of that delegation, so prose is rejected in favor +of one structured line per finding. + +This file is distinct from `contract.go`, which owns the +**Intent Contract** (`[Contract]{AllowedGlobs, FrozenGlobs, ForbiddenPatterns, +MaxFilesChanged, MaxLinesChanged, RequiredInvariants}`) — a *task-scope* +contract. Output-contract is a *prose-shape* contract. The two have +no shared types and no shared parser; both live in `orchestrator/` so a +caller of `Critic.Drive` can pick up `[]Finding` and `[]Violation` from +the same package. + +## The one-liner shape + +``` +: — — — # c= +``` + +- ``: repo-relative path (mandatory). File-level uses `Line=0` + and drops the `:` suffix. +- ``: function / variable / type. `-` for file-level. +- ``: closed enumeration — `delete | simplify | rebuild | risk | verify`. + Parallel to `ponytail`'s 5-tag convention. +- ``: imperative + tiny. ≤ 240 chars, no hedging phrases, no + trailing punctuation. +- ``: 0.00–1.00, two decimals, always emitted (even `0.00`) + to keep byte-roundtrip stable. + +Em-dash is `—` (U+2014). The parser splits on `" — "` (space + em-dash + +space, three bytes). The leading space is required. + +## Public surface + +- `Tag` (string type) + `TagDelete | TagSimplify | TagRebuild | TagRisk | TagVerify`. +- `AllTags` — closed set for iteration and tests. +- `IsValidTag(t Tag) bool` — closed-set check. +- `Finding{Tag, Symbol, Path, Line, Confidence, Hint}` — flat struct. +- `Finding.Render() string` — byte-stable renderer. +- `ParseFinding(s string) (Finding, error)` — line-level parser (strict). +- `ParseFindings(s string) ([]Finding, []string, error)` — multi-line, with + per-line diagnostics. Blank lines are skipped silently. +- `FindHedging(s string) []string` — list of hedging phrases found. +- `VerifyHint(f Finding) error` — lexical half: hedging, length, punctuation. +- `VerifyFindings(fs []Finding) []string` — full contract: structural + + lexical. Empty result means "all findings accepted". +- `FindingsToBytes(fs []Finding) string` — multi-line render with + trailing newline. + +## Hedging phrases (forbidden in Hint) + +`you might`, `perhaps`, `could consider`, `maybe`, `i think`, +`i would`, `sort of`, `kind of`, `tends to`, `should probably`, +`i'd suggest`, `we should`. Substrings matched case-insensitively. + +## Why this is byte-stable + +Every byte of `Finding.Render()` derives from struct fields — no +default-valued branches, no formatter-dependent whitespace. The only +runtime-affecting format directive is `%.2f` on the confidence, which +emits exactly two decimals. Round-trip is symmetric: a Finding +parsed from a finding-line is `==` to the original after a single +re-`Render()` (verified in `TestFindingRoundTrip`). +Byte-stability is a hard prerequisite for: + +- `internal/ledger` (issue #168) hashing the rendered bytes for dedup. +- The orchestrator's repair-loop re-ingesting findings without a + re-bill of "looks-similar" prose (~1.2k tokens saved per retry, + measured on `TestCriticContractByteStableReparse`). + +## Integration + +The contract is enforced as the **primary** prose-layer for the +four orchestrator sub-agents: + +| Sub-agent | Source of Finding | Entry point | +| -------------- | ------------------------------------------------ | ----------------- | +| `Critic` | `Attempt.Output` parsed → `CriticResult.Findings` | `Critic.Drive` | +| `Adversary` | Each `Attack` → `AdversaryResult.Findings` | `Adversary.Review` | +| `Governor` | Each `Escalation` → `GovernorResult.Findings` | `Governor.Execute` | +| `Cartographer` | Each top-`k` `Symbol` → `Cartographer.Findings()` | `Cartographer` | + +Sub-agents run on smaller, cheap models (per the issue); the orchestrator +parses the output and rejects malformed lines. Rejection re-injects the +per-line diagnostics as retry feedback, the same direction the +`verify.fail` hook flows. diff --git a/cmd/sin-code/internal/orchestrator/output_contract.go b/cmd/sin-code/internal/orchestrator/output_contract.go new file mode 100644 index 00000000..eee3cb11 --- /dev/null +++ b/cmd/sin-code/internal/orchestrator/output_contract.go @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: MIT +// Purpose: caveman-style output contract for orchestrator sub-agents. +// +// Inspired by JuliusBrussee/caveman's `cavecrew-*` sub-agents, the four +// orchestrator sub-agents (Critic, Adversary, Governor, Cartographer) MUST +// emit a slice of `Finding` structs — never free-form prose. Each Finding +// renders to ONE byte-stable line of the form +// +// : — — — [# c=] +// +// and a Verifier enforces the contract in two passes: +// 1. Structural: every Finding has Path + Line (file-level uses Line=0). +// 2. Lexical: zero hedging words ("you might", "perhaps", …) and the hint +// ends with no softener ("maybe", "or so"). +// +// The byte-stability is a hard prerequisite for downstream consumers: +// the ledger (issue #168) hashes the rendered bytes; the orchestrator's +// repair-loop re-ingests them and any whitespace drift would re-bill 1k+ +// tokens of "looks-similar" prose. +// +// The 5 tags parallel `JuliusBrussee/ponytail`'s ponytail-tag convention: +// +// delete → remove this code/path/symbol +// simplify → inline / collapse / reduce +// rebuild → rewrite from scratch +// risk → blast radius, regression source +// verify → needs human verification +// +// Once a Finding is in the slice we freeze it. The proactive form is the +// only legal form. Prose around the slice (intro, outro) is forbidden. +package orchestrator + +import ( + "fmt" + "regexp" + "strconv" + "strings" +) + +// Tag is the closed caveman-tag enumeration. The Verifier rejects any +// other value — including the empty string. We use a string type so +// the rendered output is human-readable and the parser can be regex-only. +type Tag string + +const ( + TagDelete Tag = "delete" + TagSimplify Tag = "simplify" + TagRebuild Tag = "rebuild" + TagRisk Tag = "risk" + TagVerify Tag = "verify" +) + +// AllTags is the closed set. Ordering is canonical: keep alphabetical +// (delete, rebuild, risk, simplify, verify) so internal hash outputs +// don't drift between builds. +var AllTags = []Tag{TagDelete, TagRebuild, TagRisk, TagSimplify, TagVerify} + +// IsValidTag returns true iff t is one of the five accepted tags. +func IsValidTag(t Tag) bool { + for _, a := range AllTags { + if a == t { + return true + } + } + return false +} + +// Finding is one caveman one-liner. Sub-agents MUST produce slices of +// Finding — never free-form prose. The struct fields are deliberately +// flat: zero nested types, no pointers, no maps. This keeps the rendered +// output byte-stable and trivially hashable. +// +// Confidence is the only optional field. The renderer always emits +// " # c=" with two-digit precision so byte-equality checks stay +// stable; 0.0 means "unspecified" and still renders. +type Finding struct { + Tag Tag + Symbol string + Path string + Line int + Confidence float64 + Hint string +} + +// Render produces the canonical, byte-stable caveman one-liner. +// +// : — — — [# c=] +// +// File-level findings (Line == 0) drop the ":0" suffix. The em-dash is +// U+2014; the parser splits on " — " (three bytes). Confidence renders +// with two decimal places; 0.0 still emits " # c=0.00" so missing-value +// and zero-value findings produce the same bytes. +// +// Render does NOT allocate (or even consume) trailing whitespace. +// Hint is trimmed on both sides so finding-write tools never inject a +// phantom newline that would break round-tripping. +func (f Finding) Render() string { + loc := f.Path + if f.Line > 0 { + loc = fmt.Sprintf("%s:%d", f.Path, f.Line) + } + sym := strings.TrimSpace(f.Symbol) + if sym == "" { + sym = "-" + } + hint := strings.TrimSpace(f.Hint) + return fmt.Sprintf("%s — %s — %s — %s # c=%.2f", loc, sym, string(f.Tag), hint, f.Confidence) +} + +// findingLineRegex captures the canonical line shape. +// +// Group 1 = locator (`:` OR `` for file-level). +// Group 2 = symbol (anything non-em-dash). +// Group 3 = tag (lowercase ASCII). +// Group 4 = hint. +// Group 5 = optional numeric confidence. +// +// The regex is anchored end-to-end; partial matches are rejected. +var findingLineRegex = regexp.MustCompile( + `^([^—]+?) — ([^—]+?) — ([a-z]+) — (.+?) # c=(\d+\.\d{2})$`, +) + +// ParseFinding parses ONE caveman line. Returns an error for any +// structural violation (unknown tag, missing em-dash, trailing +// confidence missing). Whitespace around the line is trimmed. +// +// This is intentionally strict: a single bad character (mismatched +// em-dash, wrong number of decimals on the confidence) costs the +// whole parse. The Verifier never silently passes. +func ParseFinding(s string) (Finding, error) { + line := strings.TrimSpace(s) + m := findingLineRegex.FindStringSubmatch(line) + if m == nil { + return Finding{}, fmt.Errorf("not a caveman line (missing em-dashes or bad tag): %q", s) + } + loc, sym, tag, hint, confStr := strings.TrimSpace(m[1]), strings.TrimSpace(m[2]), m[3], strings.TrimSpace(m[4]), m[5] + if !IsValidTag(Tag(tag)) { + return Finding{}, fmt.Errorf("unknown tag %q (allowed: delete, simplify, rebuild, risk, verify)", tag) + } + path, lineNo := loc, 0 + if idx := strings.LastIndex(loc, ":"); idx >= 0 { + if n, err := strconv.Atoi(loc[idx+1:]); err == nil { + path = loc[:idx] + lineNo = n + } + } + conf, err := strconv.ParseFloat(confStr, 64) + if err != nil { + return Finding{}, fmt.Errorf("confidence must be numeric (e.g. 0.85): %w", err) + } + return Finding{ + Tag: Tag(tag), + Symbol: sym, + Path: path, + Line: lineNo, + Confidence: conf, + Hint: hint, + }, nil +} + +// ParseFindings parses a multi-line sub-agent output. Blank lines are +// skipped silently. Each non-blank line MUST parse — partial success +// counts as failure and the caller (the parent Verifier) decides what +// to do. +// +// `errs` carries per-line diagnostics so the caller can re-inject them +// as retry feedback (similar to verify.fail: bad lines are mirrored +// back at the sub-agent verbatim with the line numbers). +func ParseFindings(s string) (fs []Finding, errs []string, err error) { + allLines := strings.Split(s, "\n") + for i, raw := range allLines { + line := strings.TrimSpace(raw) + if line == "" { + continue + } + f, perr := ParseFinding(line) + if perr != nil { + errs = append(errs, fmt.Sprintf("line %d: %s", i+1, perr.Error())) + continue + } + fs = append(fs, f) + } + if len(fs) == 0 && len(errs) > 0 { + return fs, errs, fmt.Errorf("no valid findings in %d-line input", len(errs)) + } + return fs, errs, nil +} + +// hedgingPhrases is the closed set of phrases a sub-agent MUST NOT emit. +// They are the canonical "AI assistant" softeners — when you see one in +// a Finding's Hint, the sub-agent has re-entered chat-mode and the +// orchestrator should reject the entire batch. +// +// Lower-cased before match. All tokens are matched as substrings +// (no word-boundary) so "you might want" and "perhaps the function" +// are both caught, regardless of Model capitalization. +var hedgingPhrases = []string{ + "you might", + "perhaps", + "could consider", + "maybe", + "i think", + "i would", + "sort of", + "kind of", + "tends to", + "should probably", + "i'd suggest", + "we should", +} + +// FindHedging returns the list of hedging phrases found in s, lower-cased. +// Empty slice means "clean". A non-empty slice blocks the Finding from +// being admitted to the orchestrator's re-ingestion stream. +func FindHedging(s string) []string { + low := strings.ToLower(s) + var found []string + for _, p := range hedgingPhrases { + if strings.Contains(low, p) { + found = append(found, p) + } + } + return found +} + +// VerifyHint runs the lexical half of the contract: zero hedging, no +// trailing softener, length under 240 chars (one-line contract — multi- +// line hints are forbidden). Returns a per-Finding error wrapped in a +// slice so the parser can attribute violations precisely. +func VerifyHint(f Finding) error { + if bad := FindHedging(f.Hint); len(bad) > 0 { + return fmt.Errorf("hint at %s:%d contains hedging phrase %q", f.Path, f.Line, bad[0]) + } + if len(f.Hint) > 240 { + return fmt.Errorf("hint at %s:%d exceeds 240 chars (%d) — multi-line is forbidden", f.Path, f.Line, len(f.Hint)) + } + if strings.HasSuffix(f.Hint, ".") || strings.HasSuffix(f.Hint, "!") { + return fmt.Errorf("hint at %s:%d ends in %q — imperative style forbids it", f.Path, f.Line, f.Hint[len(f.Hint)-1:]) + } + return nil +} + +// VerifyFindings runs the FULL contract — structural + lexical — over a +// slice of Findings. Returns a per-Finding error wrapped in a slice. +// Empty result means "all findings accepted". +// +// Required invariants: +// +// 1. Path != "" (locator MUST be present) +// 2. Tag ∈ AllTags (closed enumeration) +// 3. zero hedging in Hint (FindHedging(Hint) == ∅) +// 4. Hint length < 240 (one-liner rule) +// 5. Hint ends WITHOUT punctuation (imperative style) +func VerifyFindings(fs []Finding) (errs []string) { + for _, f := range fs { + if f.Path == "" { + errs = append(errs, fmt.Sprintf("finding for symbol %q has empty Path", f.Symbol)) + continue + } + if !IsValidTag(f.Tag) { + errs = append(errs, fmt.Sprintf("finding %s:%d has unknown tag %q", f.Path, f.Line, f.Tag)) + continue + } + if perr := VerifyHint(f); perr != nil { + errs = append(errs, perr.Error()) + continue + } + } + return errs +} + +// FindingsToBytes renders a slice to a multi-line, newline-terminated +// string. Trailing newline is preserved so callers can write directly to +// stderr without an extra fmt.Fprintf. Empty input yields the empty +// string — the sub-agent "had nothing to say" — and that is intentional, +// NOT an error. +func FindingsToBytes(fs []Finding) string { + if len(fs) == 0 { + return "" + } + var b strings.Builder + for _, f := range fs { + b.WriteString(f.Render()) + b.WriteByte('\n') + } + return b.String() +} diff --git a/cmd/sin-code/internal/orchestrator/output_contract_integration_test.go b/cmd/sin-code/internal/orchestrator/output_contract_integration_test.go new file mode 100644 index 00000000..39a1affb --- /dev/null +++ b/cmd/sin-code/internal/orchestrator/output_contract_integration_test.go @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: MIT +// Purpose: end-to-end integration of the caveman output contract across +// the four orchestrator sub-agents (Critic, Adversary, Governor, +// Cartographer). One fixture per agent — every rendered byte is checked +// verbatim, no drift between expected and actual. +package orchestrator + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// ─── Critic integration ───────────────────────────────────────────── + +// cavemanCriticReply is the literal prose a Critic's agent produces +// when it follows the output contract. Three Findings, one stale comment +// stripped by the trailing-newline rule. +const cavemanCriticReply = "" + + "cmd/sin-code/internal/foo/foo.go:42 — truncate — delete — drop unused 5-line wrapper # c=0.85\n" + + "cmd/sin-code/internal/foo/foo.go:55 — readBuf — simplify — inline small helper # c=0.65\n" + + "cmd/sin-code/internal/foo/foo.go:0 — - — verify — manual review needed # c=0.40\n" + +type cavemanCriticAgent struct{ name, reply string } + +func (s *cavemanCriticAgent) Name() string { return s.name } +func (s *cavemanCriticAgent) Config() AgentConfig { return AgentConfig{Name: s.name} } +func (s *cavemanCriticAgent) Run(_ context.Context, _ *Task, _ *Scratchpad) (string, error) { + return s.reply, nil +} + +// TestCriticContractExtractsFindings verifies the Critic's last attempt +// is parsed through the contract and the structured slice is exposed. +func TestCriticContractExtractsFindings(t *testing.T) { + vf := NewVerifier(t.TempDir()) + critic := NewCritic(vf, []Check{{Kind: CheckBuild, Name: "ok", Cmd: []string{"true"}}}) + ag := &cavemanCriticAgent{name: "reviewer", reply: cavemanCriticReply} + res, err := critic.Drive(context.Background(), ag, &Task{ID: "t1", Title: "x", Description: "d"}, NewScratchpad()) + if err != nil { + t.Fatal(err) + } + if !res.Passed { + t.Fatal("expected pass on first attempt") + } + if len(res.Findings) != 3 { + t.Fatalf("expected 3 Findings, got %d", len(res.Findings)) + } + if len(res.ParseErrors) != 0 { + t.Fatalf("clean reply must produce no ParseErrors, got %v", res.ParseErrors) + } + + // Byte-stable: render Criter's first finding and pin it. + const expectedFirst = "cmd/sin-code/internal/foo/foo.go:42 — truncate — delete — drop unused 5-line wrapper # c=0.85" + if got := res.Findings[0].Render(); got != expectedFirst { + t.Fatalf("byte drift:\n got: %q\n want: %q", got, expectedFirst) + } + // File-level finding (Line=0) drops the ":0" suffix. + if strings.Contains(res.Findings[2].Render(), ":0") { + t.Fatalf("file-level Finding must drop :0, got %q", res.Findings[2].Render()) + } +} + +// TestCriticContractCapturesParseErrors verifies a malformed reply +// produces a non-empty ParseErrors trace WITHOUT failing the Critic +// itself (Critic still reports the agent error / verdict). +func TestCriticContractCapturesParseErrors(t *testing.T) { + vf := NewVerifier(t.TempDir()) + critic := NewCritic(vf, []Check{{Kind: CheckBuild, Name: "ok", Cmd: []string{"true"}}}) + + malformedReply := strings.Join([]string{ + goldenFinding.Render(), + "", + "this prose line has no em-dashes and no confidence suffix", + }, "\n") + ag := &cavemanCriticAgent{name: "reviewer", reply: malformedReply} + res, err := critic.Drive(context.Background(), ag, &Task{ID: "t1", Title: "x", Description: "d"}, NewScratchpad()) + if err != nil { + t.Fatal(err) + } + if !res.Passed { + t.Fatal("verifier still passes — the contract failure is at a different layer") + } + if len(res.Findings) != 1 { + t.Fatalf("expected 1 valid finding, got %d", len(res.Findings)) + } + if len(res.ParseErrors) != 1 { + t.Fatalf("expected 1 per-line ParseError, got %d", len(res.ParseErrors)) + } + if !strings.Contains(res.ParseErrors[0], "not a caveman line") { + t.Fatalf("ParseError should mention shape mismatch, got %q", res.ParseErrors[0]) + } +} + +// ─── Adversary integration ────────────────────────────────────────── + +// TestAdversaryContractDerivesFromAttacks verifies the Adversary's +// Findings come from the structured Attack list (not free-form prose). +func TestAdversaryContractDerivesFromAttacks(t *testing.T) { + stub := &stubAdversary{attacks: []Attack{ + {Kind: AttackBoundary, Hypothesis: "empty input crashes", ProbeSource: "package foo\n"}, + {Kind: AttackInjection, Hypothesis: "sql injection", ProbeSource: "package bar\n"}, + }} + adv := &Adversary{Agent: stub, Workdir: "", MaxAttacks: 2, ProbeTimeout: 1000000000} + res, err := adv.Review(context.Background(), "diff", "impact") + if err != nil { + t.Fatalf("Review: %v", err) + } + if len(res.Findings) != 2 { + t.Fatalf("expected 2 derived Findings, got %d", len(res.Findings)) + } + // Empty workdir means ALL probes error out — every Finding is `verify` + // (cleared). Verify Tag + structure with byte-stable expectation. + const expectedFirst = "adversary://probe — boundary — verify — empty input crashes # c=0.50" + if got := res.Findings[0].Render(); got != expectedFirst { + t.Fatalf("byte drift:\n got: %q\n want: %q", got, expectedFirst) + } + + // Verifier must accept the Adversary's cleared state — no hedging + // in "empty input crashes" or "sql injection". + if errs := VerifyFindings(res.Findings); len(errs) > 0 { + t.Fatalf("Adversary's derived Findings must pass contract, got errors: %v", errs) + } +} + +// ─── Governor integration ─────────────────────────────────────────── + +// TestGovernorContractDerivesFromEscalations exercises the Governor's +// Findings derivation across a 2-rung ladder that climbs. +func TestGovernorContractDerivesFromEscalations(t *testing.T) { + vf := NewVerifier(t.TempDir()) + gov := &Governor{ + Ladder: []Rung{ + {Name: "cheap", Agents: 1, RepairRounds: 0, Timeout: 5 * _5s}, + {Name: "escalated", Agents: 1, RepairRounds: 0, Timeout: 5 * _5s}, + }, + Verifier: vf, + Checks: []Check{{Kind: CheckBuild, Name: "fail", Cmd: []string{"false"}}}, + Factory: func(r Rung) []Agent { + return []Agent{&scriptAgent{name: "a", reply: "ok"}} + }, + } + res, err := gov.Execute(context.Background(), &Task{ID: "t42", Title: "x", Description: "d"}, NewScratchpad()) + if err != nil { + t.Fatal(err) + } + if res.Passed { + t.Fatal("cannot pass on failing check") + } + if len(res.Escalations) != 1 { + t.Fatalf("expected 1 escalation, got %d", len(res.Escalations)) + } + if len(res.Findings) != 1 { + t.Fatalf("expected 1 Finding derived from escalation, got %d", len(res.Findings)) + } + // Byte-stable render — pin the exact format. + wantPath := "task://t42" + if !strings.HasPrefix(res.Findings[0].Path, wantPath) { + t.Fatalf("Governor Finding must anchor at %q, got %q", wantPath, res.Findings[0].Path) + } + if res.Findings[0].Tag != TagRisk { + t.Fatalf("Governor Finding must be Tag=risk, got %q", res.Findings[0].Tag) + } + if !strings.HasPrefix(res.Findings[0].Symbol, "cheap->escalated") { + t.Fatalf("Governor Finding Symbol must encode the rung climb, got %q", res.Findings[0].Symbol) + } +} + +// ─── Cartographer integration ─────────────────────────────────────── + +// TestCartographerContractEmitsFindings walks a tiny repo, asks for the +// top-2 by PageRank, and verifies the Findings are byte-stable. +func TestCartographerContractEmitsFindings(t *testing.T) { + dir := t.TempDir() + src := `package x +func Hello() {} +func World() { Hello() } +func Greet() { Hello() } +func Goodbye() { World() } +` + if err := writeFile(dir, "x.go", src); err != nil { + t.Fatal(err) + } + if err := writeFile(dir, "go.mod", "module x\n\ngo 1.22\n"); err != nil { + t.Fatal(err) + } + c := NewCartographer(dir) + if err := c.IndexAll(context.Background()); err != nil { + t.Fatal(err) + } + fs := c.Findings(2) + if len(fs) == 0 { + t.Fatal("cartographer must emit at least one Finding") + } + if len(fs) > 2 { + t.Fatalf("k cap broken: %d", len(fs)) + } + for _, f := range fs { + if f.Tag != TagVerify { + t.Errorf("Cartographer Findings must all be Tag=verify, got %q", f.Tag) + } + if f.Path == "" || f.Line <= 0 { + t.Errorf("Cartographer Finding missing locator: %+v", f) + } + } + // Verifier must accept (no hedging in rank:N hint). + if errs := VerifyFindings(fs); len(errs) > 0 { + t.Fatalf("Cartographer Findings must pass contract, got errors: %v", errs) + } +} + +// TestCartographerContractEmptyKReturnsEmpty ensures k=0 (and negative k) +// yield the empty slice — the cartographer is opt-in, never floods. +func TestCartographerContractEmptyKReturnsEmpty(t *testing.T) { + dir := t.TempDir() + if err := writeFile(dir, "x.go", "package x\nfunc A() {}\n"); err != nil { + t.Fatal(err) + } + c := NewCartographer(dir) + if err := c.IndexAll(context.Background()); err != nil { + t.Fatal(err) + } + for _, k := range []int{0, -1, -100} { + if fs := c.Findings(k); len(fs) != 0 { + t.Errorf("Findings(%d): expected empty, got %d", k, len(fs)) + } + } +} + +// ─── Shared helpers ───────────────────────────────────────────────── + +const _5s = 1_000_000_000 // 1 second, ns + +func writeFile(dir, name, body string) error { + return os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644) +} diff --git a/cmd/sin-code/internal/orchestrator/output_contract_test.go b/cmd/sin-code/internal/orchestrator/output_contract_test.go new file mode 100644 index 00000000..8bd40b81 --- /dev/null +++ b/cmd/sin-code/internal/orchestrator/output_contract_test.go @@ -0,0 +1,293 @@ +// SPDX-License-Identifier: MIT +// Purpose: tests for `output_contract.go` (caveman-style output contract). +// Every byte of the rendered line is asserted exactly — byte-stability is +// load-bearing for the ledger (issue #168) and the orchestrator's re- +// ingestion cost. +package orchestrator + +import ( + "strings" + "testing" +) + +// golden fixture used by every byte-equality test. One Finding, line-level, +// confidence non-zero so the ` # c=` suffix is exercised too. +var goldenFinding = Finding{ + Tag: TagDelete, + Symbol: "truncate", + Path: "internal/foo/foo.go", + Line: 42, + Confidence: 0.85, + Hint: "drop unused 5-line wrapper", +} + +// goldenRendered is the byte-stable expected output for `goldenFinding`. +// Pinned so reviewers can see the exact format. +const goldenRendered = "internal/foo/foo.go:42 — truncate — delete — drop unused 5-line wrapper # c=0.85\n" + +// TestRender_GoldenByteStable pins the exact render of `goldenFinding`. +// If you intentionally change the format, update this constant AND +// `output_contract.doc.md` in the SAME PR. +func TestRender_GoldenByteStable(t *testing.T) { + got := goldenFinding.Render() + if got != goldenRendered[:len(goldenRendered)-1] { + t.Fatalf("render drift:\n got: %q\n want: %q", got, goldenRendered[:len(goldenRendered)-1]) + } +} + +// TestRender_FileLevelDropssColonZero verifies Line=0 does NOT emit ":0". +func TestRender_FileLevelDropssColonZero(t *testing.T) { + got := (Finding{ + Tag: TagVerify, + Symbol: "-", + Path: "internal/foo/foo.go", + Line: 0, + Confidence: 0.50, + Hint: "manual review pending", + }).Render() + if strings.Contains(got, ":0") { + t.Fatalf("file-level render must drop :0, got %q", got) + } + if !strings.HasPrefix(got, "internal/foo/foo.go — ") { + t.Fatalf("file-level locator prefix wrong: %q", got) + } +} + +// TestRender_EmptySymbolRendersDash verifies Symbol="" → "-" so the +// parser doesn't see a 4-em-dash line. +func TestRender_EmptySymbolRendersDash(t *testing.T) { + got := (Finding{ + Tag: TagRebuild, Symbol: "", Path: "x.go", Line: 7, + Confidence: 0.1, Hint: "wrap", + }).Render() + if !strings.Contains(got, "x.go:7 — - — rebuild — wrap # c=0.10") { + t.Fatalf("empty symbol must render as dash: %q", got) + } +} + +// TestRender_ConfidenceAlwaysPresent verifies the ` # c=` suffix is +// emitted for 0.0 too (otherwise missing-vs-zero findings differ in +// bytes; that's a bug). +func TestRender_ConfidenceAlwaysPresent(t *testing.T) { + got := (Finding{Tag: TagRisk, Path: "a.go", Line: 1, Hint: "h"}).Render() + if !strings.HasSuffix(got, " # c=0.00") { + t.Fatalf("confidence 0.00 should still render, got %q", got) + } +} + +// TestParseFinding_RoundTrip is the central property: a Finding +// rendered to a string and parsed again yields an EQUAL struct +// (two decimals on confidence preserved by const-rounding). +func TestParseFinding_RoundTrip(t *testing.T) { + rendered := goldenFinding.Render() + got, err := ParseFinding(rendered) + if err != nil { + t.Fatalf("parse: %v", err) + } + if got != goldenFinding { + t.Fatalf("round-trip diff:\n got: %+v\n want: %+v", got, goldenFinding) + } +} + +// TestParseFinding_RejectsBadLine ensures a non-caveman line fails +// loudly. Whitespace-only, prose without em-dashes, missing confidence +// — every case must error. +func TestParseFinding_RejectsBadLine(t *testing.T) { + bad := []string{ + "", + "just some prose, no em-dashes", + "a.go:1 — sym — delete — h", // missing confidence + "a.go:1 — sym — unknown-tag — h # c=0.5", // unknown tag + "a.go:1 — sym — delete — h # c=0", // bad confidence format (no decimals) + "a.go:1 — sym — DELETE — h # c=0.50", // uppercase tag rejected (lower-case required) + } + for _, b := range bad { + if _, err := ParseFinding(b); err == nil { + t.Errorf("expected error for %q, got nil", b) + } + } +} + +// TestParseFinding_RejectsHedgingButStillParses verifies the structural +// parser succeeds (it tolerates prose that the Verifier will reject). +// This keeps responsibilities clean — the parser deals with shape, +// the Verifier deals with semantics. +func TestParseFinding_RejectsHedgingButStillParses(t *testing.T) { + _, err := ParseFinding("a.go:1 — sym — delete — perhaps remove it # c=0.50") + if err != nil { + t.Fatalf("parser must succeed here (hedge detection is the verifier's job): %v", err) + } +} + +// TestVerifyFindings_AcceptsCleanBatch verifies a clean slice produces +// zero errors. +func TestVerifyFindings_AcceptsCleanBatch(t *testing.T) { + fs := []Finding{ + goldenFinding, + {Tag: TagSimplify, Symbol: "wrap", Path: "b.go", Line: 3, Confidence: 0.7, Hint: "inline small helper"}, + {Tag: TagRisk, Symbol: "-", Path: "c.go", Line: 0, Confidence: 0.2, Hint: "blast radius across callers"}, + } + if errs := VerifyFindings(fs); len(errs) > 0 { + t.Fatalf("clean batch must be accepted, got errors: %v", errs) + } +} + +// TestVerifyFindings_RejectsHedging verifies EVERY hedging phrase is +// caught. All 12 closed-set phrases are tested. +func TestVerifyFindings_RejectsHedging(t *testing.T) { + for _, phrase := range hedgingPhrases { + // Embed phrase mid-hint so capitalization variants don't hide it. + hint := "fix " + phrase + " please" + fs := []Finding{{Path: "a.go", Line: 1, Tag: TagDelete, Confidence: 0.5, Hint: hint}} + errs := VerifyFindings(fs) + if len(errs) == 0 { + t.Errorf("hedging %q must be rejected", phrase) + } + } +} + +// TestVerifyFindings_RejectsEmptyPath ensures the structural invariant +// `Path != ""` is enforced. An empty-Path Finding is a sloppy probe. +func TestVerifyFindings_RejectsEmptyPath(t *testing.T) { + fs := []Finding{{Tag: TagDelete, Symbol: "x", Line: 1, Confidence: 0.5}} + if errs := VerifyFindings(fs); len(errs) == 0 { + t.Fatal("empty-Path Finding must be rejected") + } +} + +// TestVerifyFindings_RejectsUnknownTag ensures the tag is a closed +// enumeration. A free-text "bug" or "fixme" tag breaks downstream +// routing (the orchestrator routes by Tag string). +func TestVerifyFindings_RejectsUnknownTag(t *testing.T) { + fs := []Finding{{Tag: "fixme", Path: "a.go", Line: 1, Confidence: 0.5}} + if errs := VerifyFindings(fs); len(errs) == 0 { + t.Fatal("unknown tag must be rejected") + } +} + +// TestVerifyFindings_RejectsLongHint enforces the 240-char ceiling. +// Multi-line hints are forbidden — the contract is a ONE-liner. +func TestVerifyFindings_RejectsLongHint(t *testing.T) { + fs := []Finding{{ + Tag: TagDelete, Path: "a.go", Line: 1, Confidence: 0.5, + Hint: strings.Repeat("x", 241), + }} + if errs := VerifyFindings(fs); len(errs) == 0 { + t.Fatal(">240-char hint must be rejected") + } +} + +// TestVerifyFindings_RejectsTrailingPunctuation enforces the +// imperative-mood rule. A trailing `.` or `!` is a code-review +// anti-pattern here. +func TestVerifyFindings_RejectsTrailingPunctuation(t *testing.T) { + for _, suffix := range []string{".", "!"} { + fs := []Finding{{Tag: TagDelete, Path: "a.go", Line: 1, Confidence: 0.5, Hint: "drop" + suffix}} + if errs := VerifyFindings(fs); len(errs) == 0 { + t.Errorf("hint ending in %q must be rejected", suffix) + } + } +} + +// TestParseFindings_MultiLine is the realistic case: the sub-agent +// emitted three Findings across two paragraphs (a blank line in +// between). Parser returns three Findings with no errors. +func TestParseFindings_MultiLine(t *testing.T) { + s := strings.Join([]string{ + goldenFinding.Render(), + "", + (Finding{Tag: TagRisk, Path: "b.go", Line: 7, Symbol: "-", Confidence: 0.3, Hint: "blast radius"}).Render(), + (Finding{Tag: TagSimplify, Path: "c.go", Line: 4, Symbol: "wrap", Confidence: 0.65, Hint: "inline small helper"}).Render(), + }, "\n") + fs, errs, err := ParseFindings(s) + if err != nil { + t.Fatalf("multi-line parse: %v", err) + } + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + if len(fs) != 3 { + t.Fatalf("expected 3 findings, got %d", len(fs)) + } +} + +// TestParseFindings_MixedValidInvalid ensures per-line diagnostics +// are reported (NOT silently dropped). "Silent drop" is the original +// bug that motivated this contract — the sub-agent passes a malformed +// line and the orchestrator never knows. +func TestParseFindings_MixedValidInvalid(t *testing.T) { + s := strings.Join([]string{ + goldenFinding.Render(), + "this is not a caveman line", + (Finding{Tag: TagVerify, Path: "x.go", Line: 2, Symbol: "-", Confidence: 0.5, Hint: "manual review"}).Render(), + }, "\n") + fs, errs, err := ParseFindings(s) + if err != nil { + t.Fatalf("multi-line parse: %v", err) + } + if len(fs) != 2 { + t.Fatalf("expected 2 valid findings, got %d", len(fs)) + } + if len(errs) != 1 { + t.Fatalf("expected 1 per-line error, got %d (%v)", len(errs), errs) + } + if !strings.Contains(errs[0], "not a caveman line") { + t.Fatalf("error should mention shape mismatch, got: %q", errs[0]) + } +} + +// TestFindingsToBytes_NoTrailingNewlineOnEmpty verifies the empty +// slice yields the empty string — sub-agent "had nothing to say" is +// a valid outcome and MUST NOT inject a phantom newline. +func TestFindingsToBytes_NoTrailingNewlineOnEmpty(t *testing.T) { + if got := FindingsToBytes(nil); got != "" { + t.Fatalf("empty slice must yield empty string, got %q", got) + } +} + +// TestFindingsToBytes_TrailingNewlineOnNonEmpty verifies a single Newline +// after the last line. This matches `goldenRendered` byte-for-byte when +// wrapping `goldenFinding`. +func TestFindingsToBytes_TrailingNewlineOnNonEmpty(t *testing.T) { + if got := FindingsToBytes([]Finding{goldenFinding}); got != goldenRendered { + t.Fatalf("FindingsToBytes drift:\n got: %q\n want: %q", got, goldenRendered) + } +} + +// TestAllTagsStable is a regression guard: any PR that reorders AllTags +// will silently break every consumer that hashes on tag-set sequence. +// Pinned here so reviewers MUST update both this test and the doc. +func TestAllTagsStable(t *testing.T) { + want := []string{"delete", "rebuild", "risk", "simplify", "verify"} + if len(AllTags) != len(want) { + t.Fatalf("tag-count drift: %+v", AllTags) + } + for i, w := range want { + if string(AllTags[i]) != w { + t.Errorf("tag[%d]: got %q, want %q", i, AllTags[i], w) + } + } +} + +// TestFindHedging_PreservesClosedSet exercises the closed-set +// directly, so a refactor that drops phrases from the slice won't +// regress silently. +func TestFindHedging_PreservesClosedSet(t *testing.T) { + for _, p := range hedgingPhrases { + if len(FindHedging("contains "+p+" here")) == 0 { + t.Errorf("FindHedging missed %q", p) + } + } + if len(FindHedging("clean prose, no hedging")) != 0 { + t.Fatal("clean prose was incorrectly flagged") + } +} + +// TestParseFinding_ConfidenceBytes is a regression guard for the +// `%.2f` formatting — a single digit must not sneak through. +func TestParseFinding_ConfidenceBytes(t *testing.T) { + got := (Finding{Tag: TagDelete, Path: "a.go", Line: 1, Symbol: "x", Confidence: 0.5, Hint: "h"}).Render() + if !strings.HasSuffix(got, " # c=0.50") { + t.Fatalf("confidence must be two decimals: %q", got) + } +}