From 57e89895915a63accb5a99bea2560bc96c3372b8 Mon Sep 17 00:00:00 2001 From: Jay Flowers Date: Mon, 31 Aug 2026 23:29:27 -0400 Subject: [PATCH] feat: add divisor-entropy Review Council agent with vibe-check init and diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a structural-entropy divisor that measures base↔PR design-quality delta (coupling, instability, abstractness, distance, LCOM, circular dependencies) via vibe-check analyze + diff in an isolated git worktree, enforcing the Boy Scout Rule across PRs. - metrics/delta.go, metrics/verdict.go: base↔PR entropy delta engine (ComputeDelta) and deterministic verdict engine (DecideVerdict) with protected gate thresholds (ΔI≥0.15, ΔD≥0.20, ΔLCOM≥2, new cycle) - cmd/vibe-check/diff.go: `vibe-check diff ` CLI with tighten-only threshold overrides and human/JSON output - internal/scaffold/: embedded agent-asset deployment package with symlink-safe path validation and skip/force semantics - cmd/vibe-check/init.go: `vibe-check init [path]` CLI deploying embedded Review Council agents to .opencode/agents/ - cmd/vibe-check/analyze.go: add --output/-o flag, force GOTOOLCHAIN=local for hermetic analysis - Comprehensive test suite (88-92% coverage across packages) - Schema-valid ModuleGraph diff fixtures + validation checklist - Documentation: AGENTS.md, CHANGELOG.md, README.md updates Assisted-by: claude-opus Generated with AI assistance (claude-opus) --- .opencode/agents/divisor-entropy.md | 261 +++++ ...tropy-agent-20260901T023412-jay-flowers.md | 10 + ...tropy-agent-20260901T023423-jay-flowers.md | 10 + ...tropy-agent-20260901T023437-jay-flowers.md | 10 + ...tropy-agent-20260831T182255-jay-flowers.md | 10 + ...tropy-agent-20260831T190914-jay-flowers.md | 10 + ...tropy-agent-20260831T192522-jay-flowers.md | 10 + ...tropy-agent-20260831T210347-jay-flowers.md | 23 + ...tropy-agent-20260831T212924-jay-flowers.md | 25 + ...tropy-agent-20260831T231711-jay-flowers.md | 10 + .../openspec-20260831T231732-jay-flowers.md | 10 + AGENTS.md | 40 +- CHANGELOG.md | 37 + README.md | 43 +- cmd/vibe-check/analyze.go | 16 +- cmd/vibe-check/analyze_test.go | 134 +++ cmd/vibe-check/diff.go | 404 ++++++++ cmd/vibe-check/diff_test.go | 937 ++++++++++++++++++ cmd/vibe-check/init.go | 244 +++++ cmd/vibe-check/init_test.go | 375 +++++++ cmd/vibe-check/root.go | 2 + internal/goadapter/env_test.go | 36 + internal/goadapter/resolve.go | 14 +- .../scaffold/assets/agents/divisor-entropy.md | 261 +++++ internal/scaffold/doc.go | 25 + internal/scaffold/embed.go | 12 + internal/scaffold/scaffold.go | 226 +++++ internal/scaffold/scaffold_test.go | 507 ++++++++++ metrics/delta.go | 296 ++++++ metrics/delta_test.go | 455 +++++++++ metrics/testdata/entropy/README.md | 134 +++ .../testdata/entropy/comment-band-base.json | 22 + metrics/testdata/entropy/comment-band-pr.json | 22 + .../testdata/entropy/degradation-base.json | 35 + metrics/testdata/entropy/degradation-pr.json | 40 + .../testdata/entropy/improvement-base.json | 40 + metrics/testdata/entropy/improvement-pr.json | 35 + .../testdata/entropy/partial-build-base.json | 22 + .../testdata/entropy/partial-build-pr.json | 28 + metrics/verdict.go | 167 ++++ metrics/verdict_test.go | 399 ++++++++ .../add-divisor-entropy-agent/.openspec.yaml | 2 + .../add-divisor-entropy-agent/design.md | 498 ++++++++++ .../add-divisor-entropy-agent/proposal.md | 119 +++ .../specs/analyze-command/spec.md | 40 + .../specs/diff-command/spec.md | 166 ++++ .../specs/divisor-entropy-agent/spec.md | 248 +++++ .../specs/init-command/spec.md | 134 +++ .../add-divisor-entropy-agent/tasks.md | 369 +++++++ 49 files changed, 6966 insertions(+), 7 deletions(-) create mode 100644 .opencode/agents/divisor-entropy.md create mode 100644 .uf/dewey/learnings/add-divisor-entropy-agent-20260901T023412-jay-flowers.md create mode 100644 .uf/dewey/learnings/add-divisor-entropy-agent-20260901T023423-jay-flowers.md create mode 100644 .uf/dewey/learnings/add-divisor-entropy-agent-20260901T023437-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T182255-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T190914-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T192522-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T210347-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T212924-jay-flowers.md create mode 100644 .uf/dewey/learnings/divisor-entropy-agent-20260831T231711-jay-flowers.md create mode 100644 .uf/dewey/learnings/openspec-20260831T231732-jay-flowers.md create mode 100644 cmd/vibe-check/diff.go create mode 100644 cmd/vibe-check/diff_test.go create mode 100644 cmd/vibe-check/init.go create mode 100644 cmd/vibe-check/init_test.go create mode 100644 internal/goadapter/env_test.go create mode 100644 internal/scaffold/assets/agents/divisor-entropy.md create mode 100644 internal/scaffold/doc.go create mode 100644 internal/scaffold/embed.go create mode 100644 internal/scaffold/scaffold.go create mode 100644 internal/scaffold/scaffold_test.go create mode 100644 metrics/delta.go create mode 100644 metrics/delta_test.go create mode 100644 metrics/testdata/entropy/README.md create mode 100644 metrics/testdata/entropy/comment-band-base.json create mode 100644 metrics/testdata/entropy/comment-band-pr.json create mode 100644 metrics/testdata/entropy/degradation-base.json create mode 100644 metrics/testdata/entropy/degradation-pr.json create mode 100644 metrics/testdata/entropy/improvement-base.json create mode 100644 metrics/testdata/entropy/improvement-pr.json create mode 100644 metrics/testdata/entropy/partial-build-base.json create mode 100644 metrics/testdata/entropy/partial-build-pr.json create mode 100644 metrics/verdict.go create mode 100644 metrics/verdict_test.go create mode 100644 openspec/changes/add-divisor-entropy-agent/.openspec.yaml create mode 100644 openspec/changes/add-divisor-entropy-agent/design.md create mode 100644 openspec/changes/add-divisor-entropy-agent/proposal.md create mode 100644 openspec/changes/add-divisor-entropy-agent/specs/analyze-command/spec.md create mode 100644 openspec/changes/add-divisor-entropy-agent/specs/diff-command/spec.md create mode 100644 openspec/changes/add-divisor-entropy-agent/specs/divisor-entropy-agent/spec.md create mode 100644 openspec/changes/add-divisor-entropy-agent/specs/init-command/spec.md create mode 100644 openspec/changes/add-divisor-entropy-agent/tasks.md diff --git a/.opencode/agents/divisor-entropy.md b/.opencode/agents/divisor-entropy.md new file mode 100644 index 0000000..6421f28 --- /dev/null +++ b/.opencode/agents/divisor-entropy.md @@ -0,0 +1,261 @@ +--- +description: "Structural-entropy divisor — measures the base→PR change in design-quality metrics (coupling, instability, abstractness, distance, LCOM, circular dependencies) via vibe-check and reports a verdict." +mode: subagent +temperature: 0.1 +permission: + edit: deny + webfetch: deny + bash: + "*": "deny" + "git merge-base *": "allow" + "git rev-parse *": "allow" + "git worktree add *": "allow" + "git worktree remove *": "allow" + "git worktree prune": "allow" + "git fetch origin *": "allow" + "git check-ref-format *": "allow" + "vibe-check analyze *": "allow" + "vibe-check diff *": "allow" +--- + + +# Role: The Entropy Divisor + +You are the structural-entropy reviewer for this project. Your exclusive domain +is **architectural drift** — whether a pull request degrades the design quality +of the codebase relative to its base. You measure the base→PR change in the +Martin design-quality metrics — afferent coupling (Ca), efferent coupling (Ce), +instability (I), abstractness (A), distance from the main sequence (D), cohesion +(LCOM4), and circular dependencies — and report a verdict backed by an auditable +metric-delta table. You enforce the Boy Scout Rule: a PR should not leave the +architecture measurably worse than it found it. + +You do NOT compute metric arithmetic in-prompt. The deltas and the verdict are +computed for you by the tested Go `vibe-check diff` command; you orchestrate the +measurement, then report and explain its output. This keeps the verdict +deterministic and its thresholds protected in tested code, not in fallible prompt +arithmetic. + +You operate in **Code Review Mode**: the caller asks you to review the changes on +a PR branch against its base. + +--- + +## Step 0: Prior Learnings (optional) + +If Dewey MCP tools are available (`dewey_semantic_search`): + +1. Query for prior learnings about structural entropy, coupling, and cohesion + regressions: + `dewey_semantic_search({ query: "structural entropy coupling instability cohesion regression" })` +2. Query for learnings related to the packages touched by the diff: + `dewey_semantic_search({ query: "" })` +3. Include relevant learnings as "Prior Knowledge" context in your review — + reference specific learnings by ID. + +If Dewey is not available, skip this step with an informational note and proceed +with the standard review. + +--- + +## Source Documents + +Before reviewing, read: + +1. `AGENTS.md` -- Project overview, architecture, and coding conventions +2. `.specify/memory/constitution.md` -- Constitution principles (if present) +3. `.opencode/uf/packs/severity.md` -- Shared severity definitions (MUST load for + consistent severity classification across the council) +4. Any other `*.md` files under `.opencode/uf/packs/` that apply to the change. + If no pack files are found, note this and proceed with universal checks only. + +--- + +## Code Review Mode + +This is the default mode. You compute the structural delta between the base ref +and the PR ref, then report the Go-computed verdict. + +### Ref validation (run these IN ORDER, before a ref reaches any command) + +The base ref is overridable, so it is untrusted input. Validate it in exactly +this order before it is ever interpolated into a command: + +1. **Regex gate FIRST.** Reject any base ref that does not match + `^[A-Za-z0-9._/-]+$`. This is the PRIMARY metacharacter defense and is applied + before the ref reaches ANY command. It rejects spaces, `;`, `&&`, `||`, `|`, + `$(...)`, backticks, redirection, and `=`, so a ref can neither smuggle shell + metacharacters nor form a dangerous option (which would require `=` or a + space). +2. **`git check-ref-format` THEN.** Run `git check-ref-format ""` for + ref-semantic validation. This rejects `..`, `@{`, a leading `-`, and a + trailing `.lock` — cases the regex alone permits (the regex allows `.` and + `/`, and therefore `..`). The regex and `check-ref-format` are complementary, + not equivalent. +3. **Resolve to a SHA THEN.** Resolve the validated ref with + `git rev-parse --verify --end-of-options "^{commit}"` so it can never be + parsed as a flag or a chained command. Use ONLY the resulting SHA thereafter; + never re-interpolate the untrusted branch string. + +### Delta workflow + +1. **Determine the base ref.** Default the base to + `git merge-base HEAD origin/main` (matching the council's three-dot + `git diff main...HEAD` scope). If an explicit base ref is supplied, run the + ordered ref-validation steps above on it first. +2. **Fetch only if needed, only after the regex.** In a shallow CI clone the + merge-base may be unavailable. If the base ref must be fetched, run + `git fetch origin ` ONLY after the `^[A-Za-z0-9._/-]+$` regex has + validated it. `git fetch` accepts no `--end-of-options` terminator, so for + this command the regex is the load-bearing pre-fetch guard. +3. **Create an isolated worktree.** Run `git worktree prune` first to clear any + stale entries, then `git worktree add` the resolved base SHA into a UNIQUE + temp directory OUTSIDE the repo tree (e.g. under the system temp dir), so the + PR working tree and index are never mutated and no tool scanning the repo + picks up the worktree as part of the module. +4. **Analyze both refs with the SAME binary.** Run + `vibe-check analyze --output ` in the base worktree and + `vibe-check analyze --output ` on the PR checkout, each with a + bounded `--timeout`. Use `vibe-check` — NOT `gaze` or `goda` — and use the + `--output` flag to materialize each JSON graph to a file (the allowlist + forbids shell redirection). Using the same binary for both makes the delta + reflect source changes, not tool-version changes. +5. **Diff the two graphs.** Run `vibe-check diff ` to + compute the per-package deltas, classify cycles, and render the verdict. +6. **Remove the worktree.** Run `git worktree remove --force` on the temp + worktree — ALWAYS, including when analysis failed — so no residual state + remains. + +### The verdict is computed BY `vibe-check diff` + +The verdict is produced by the tested Go `metrics.DecideVerdict` gates inside +`vibe-check diff`, NOT by in-prompt arithmetic. Report and explain it; do not +recompute thresholds yourself. The deterministic gates are: + +- a new circular dependency (present in the PR graph, absent from the base) → + **REQUEST CHANGES**; +- any existing package's ΔInstability ≥ 0.15 → **REQUEST CHANGES**; +- any package's ΔDistance ≥ 0.20 → **REQUEST CHANGES**; +- any package's ΔLCOM ≥ 2 → **REQUEST CHANGES**; +- smaller non-zero shifts that cross no threshold → **COMMENT**; +- metrics improve or stay stable → **APPROVE**. + +Pre-existing cycles that are unchanged do NOT, on their own, trigger REQUEST +CHANGES. Added and removed packages are reported for information only and never +trigger a gate. Float deltas are rounded to 4 decimal places before comparison +and the gates are inclusive (`≥`); the exact rules are the single source of truth +inside `vibe-check diff`. + +--- + +## Out of Scope + +These dimensions are owned by other Divisor personas — do NOT produce findings +for them: + +- **Security / credentials / injection** → The Adversary +- **General structure, patterns, conventions, DRY** → The Architect +- **Test coverage depth / assertion quality** → The Tester +- **Plan alignment / intent drift / zero-waste / constitution** → The Guard +- **Operational readiness / deployment / performance** → The SRE +- **Documentation & content pipeline** → The Curator + +Your lane is strictly the base→PR structural-metric *delta*. Absolute, +single-snapshot metric ceilings are enforced by `vibe-check analyze --max-*` +flags, not by this divisor — do not re-litigate a package's standing coupling if +the PR did not change it. + +--- + +## Output Format + +Report, in this order: + +1. The **base ref SHA** and **PR ref SHA** compared. +2. A **per-package delta table** with one row per changed package and the columns + `Ca`, `Ce`, `Instability`, `Abstractness`, `Distance`, `LCOM`, each shown as + `base → PR (Δ)`, sourced from the `vibe-check diff` JSON output. +3. The **list of newly introduced cycles** (if any), sourced from the diff JSON. +4. The overall **entropy direction** (improving / stable / degrading), sourced + from the diff JSON. +5. Findings in the standard divisor block format: + +``` +### [SEVERITY] Finding Title + +**File**: `path/to/package` +**Constraint**: Structural entropy (name the metric that regressed) +**Description**: What degraded, by how much (base → PR, Δ), and why it matters +**Recommendation**: How to reduce the regression +``` + +Severity levels: CRITICAL, HIGH, MEDIUM, LOW (per `.opencode/uf/packs/severity.md`). + +6. A domain **Score (1–10)**: + - 9-10: metrics improve or hold; no regression + - 7-8: negligible drift within rounding noise + - 5-6: COMMENT-band regressions worth discussion + - 3-4: at least one REQUEST CHANGES threshold crossed + - 1-2: multiple gates crossed and/or a new cycle introduced +7. A final **verdict line** — `APPROVE`, `REQUEST CHANGES`, or `COMMENT` — which + MUST be the Go-computed verdict reported by `vibe-check diff`. + +--- + +## Decision Criteria + +- **APPROVE** when metrics improve or remain stable and no gate fired + (`vibe-check diff` reports `APPROVE`). +- **REQUEST CHANGES** when `vibe-check diff` reports `REQUEST_CHANGES`: a new + cycle, or ΔInstability ≥ 0.15, ΔDistance ≥ 0.20, or ΔLCOM ≥ 2 on any package. +- **COMMENT** for smaller material shifts below every threshold, and whenever the + measurement is unreliable (see Graceful Degradation). + +End your review with a clear verdict line, the domain Score, and a summary of +findings. The verdict MUST be the one reported by `vibe-check diff` — you report +and explain it, you do not override it. + +### Graceful Degradation + +Return **COMMENT** — never a false APPROVE, and never a crash — whenever you +cannot obtain a reliable measurement, including when: + +- the `vibe-check` binary is not on `PATH`; +- the base ref cannot be resolved or analyzed (for example a shallow clone whose + merge-base cannot be fetched, or a base that does not build — a broken base + cannot serve as a baseline); +- the PR ref fails to analyze (for example the PR does not build) — clearly + distinguish "PR does not build" from a clean measurement; +- `git worktree` creation fails; +- either analysis returns a partial build (`Status: "partial"`, or load-error + warnings, with zeroed type metrics) — treat this as a degraded measurement, + not a clean one; `vibe-check diff` marks such a delta unreliable and forces + COMMENT; +- `vibe-check diff` cannot produce a verdict. + +In every degraded case, report the limitation clearly so the reviewer +understands why full delta data is unavailable, and still remove the temporary +worktree. + +--- + +## Security / Operating Constraints + +`vibe-check analyze` loads the target module with `go/packages` type-checking, +which **executes the target's own build tooling** — compilation and any cgo — to +resolve types. Analyzing a ref therefore runs code from that ref on the host: a +residual **code-execution surface**. + +`GOTOOLCHAIN=local` (forced by the `vibe-check analyze` binary inside its +sanitized subprocess environment) does **NOT** close this surface. It closes only +the `go.mod` `toolchain`-directive vector — it prevents an untrusted `go.mod` from +downloading and executing a different toolchain from a proxy. It does NOT prevent +cgo or other build-time code from running during analysis. + +Consequently this divisor **MUST only run on refs the CI context already trusts** +(same-repo PRs or already-built branches) and **MUST NOT be wired into CI that +analyzes untrusted fork pull requests**. Privileged CI SHOULD additionally export +`GOTOOLCHAIN=local` ambiently as defense-in-depth. Do not remove or widen the +`bash` allowlist in this agent's frontmatter to work around these constraints — +the allowlist, the ref-sanitization regex, and `git rev-parse --verify +--end-of-options` are the load-bearing controls that keep this reviewer safe. diff --git a/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023412-jay-flowers.md b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023412-jay-flowers.md new file mode 100644 index 0000000..6cc8c41 --- /dev/null +++ b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023412-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: add-divisor-entropy-agent +author: jay-flowers +category: gotcha +created_at: 2026-09-01T02:34:12Z +identity: add-divisor-entropy-agent-20260901T023412-jay-flowers +tier: draft +--- + +On 2026-08-31, while implementing the divisor-entropy change on branch add-divisor-entropy-agent in the vibe-check repo, delegating work to subagents proved unreliable under an autonomous /uf.unleash run: the cobalt-crush-dev subagent returned an empty result and created NO file for an implementation task (internal/scaffold/scaffold.go did not exist after the Task reported 'completed'), and later a gaze-reporter review subagent was cancelled under context pressure. The durable lesson (category: gotcha): never assume a 'completed' delegation actually produced its artifact — immediately verify by reading the expected output file (or checking git status), and be ready to self-implement or self-review as a sanctioned fallback. This matters most during long autonomous pipelines where a silent no-op would otherwise cascade into a broken build gate several steps later. diff --git a/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023423-jay-flowers.md b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023423-jay-flowers.md new file mode 100644 index 0000000..1fb3208 --- /dev/null +++ b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023423-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: add-divisor-entropy-agent +author: jay-flowers +category: pattern +created_at: 2026-09-01T02:34:23Z +identity: add-divisor-entropy-agent-20260901T023423-jay-flowers +tier: draft +--- + +On 2026-08-31 in the vibe-check repo (branch add-divisor-entropy-agent), the three CLI subcommands analyze, diff, and init were all built on the same testable architecture pattern (category: pattern): a RunXxx(ctx context.Context, opts XxxOptions) (*XxxResult, error) function holds ALL logic — flag validation, calling the domain layer, exit-code mapping (0 success, 1 policy violation for analyze, 2 tool/IO failure), and a single buffered stdout write guarded by ctx.Err() so no partial output is emitted on cancellation — while the cobra RunE is only a thin wrapper that wires cmd.OutOrStdout()/cmd.ErrOrStderr(), binds flags, sets up signal.NotifyContext(SIGINT/SIGTERM), and returns an exitCodeError{code,err} consumed by main() for the process exit code. Crucially, an unexported injectable seam — writeFile func(path string, data []byte, perm fs.FileMode) error on the options struct, defaulting to os.WriteFile when nil — lets tests deterministically exercise the I/O-failure exit-2 branch without needing root or a read-only filesystem. init reuses diff's writeListSection helper (DRY), and embed.go mirrors metrics/schema.go's //go:embed var + exported accessor so staticcheck's unused rule stays green. diff --git a/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023437-jay-flowers.md b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023437-jay-flowers.md new file mode 100644 index 0000000..0f1d3f0 --- /dev/null +++ b/.uf/dewey/learnings/add-divisor-entropy-agent-20260901T023437-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: add-divisor-entropy-agent +author: jay-flowers +category: pattern +created_at: 2026-09-01T02:34:37Z +identity: add-divisor-entropy-agent-20260901T023437-jay-flowers +tier: draft +--- + +On 2026-08-31 in the vibe-check repo (branch add-divisor-entropy-agent), two reusable design patterns landed (category: pattern). First, the metrics base-to-PR entropy engine (metrics/delta.go + metrics/verdict.go): ComputeDelta matches modules by Module.Path and classifies cycles by their SORTED MEMBER SET so identity is rotation-invariant; DecideVerdict applies INCLUSIVE gates (ΔInstability≥0.15, ΔDistance≥0.20, ΔLCOM≥2, or any new circular dependency → REQUEST_CHANGES; smaller non-zero shifts → COMMENT; improving/stable → APPROVE), rounds floats to 4 decimals round-half-away-from-zero via math.Round(x*1e4)/1e4, and forces COMMENT (never a false APPROVE) whenever input is unreliable — a nil graph, Status != complete, or a load-error warning (partial build). This deliberately contrasts with the analyze command's threshold checks which use strict > comparison. Second, internal/scaffold/scaffold.go is a symlink-safe embedded-asset writer: Run delegates to an unexported run(assets fs.FS, opts) seam (so fstest.MapFS drives sort-order tests), validates the target with metrics.ValidateProjectPath, then does a deepest-existing-ancestor walk that Lstat-checks each path component (rejecting symlink or non-directory components) and verifies containment via filepath.EvalSymlinks + filepath.Rel (rejecting '..' escapes), writing dirs 0o755 / files 0o644 and normalizing mode with O_TRUNC+Chmod on force-overwrite; Result slices are asset basenames, stable-sorted. Finally, dogfooding works: running `go run ./cmd/vibe-check init .` regenerates .opencode/agents/divisor-entropy.md byte-identical to the embedded source of truth, so the deployed Review Council agent and the embedded asset never drift. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T182255-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T182255-jay-flowers.md new file mode 100644 index 0000000..28836a0 --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T182255-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: decision +created_at: 2026-08-31T18:22:55Z +identity: divisor-entropy-agent-20260831T182255-jay-flowers +tier: draft +--- + +Guard council review (Spec Review Mode) of vibe-check OpenSpec change `add-divisor-entropy-agent` (issue #3: divisor-entropy Review Council agent computing base↔PR structural-quality delta via `vibe-check analyze`, plus `vibe-check init` embedded-asset scaffolder). VERDICT: APPROVE WITH ADVISORIES; domain score 8/10. Plan alignment is exemplary — all 7 issue ACs map bidirectionally to spec requirements + tasks, no orphan tasks, deferred P2 `vibe-check diff`/provenance/CI-hard-gate explicitly listed as Non-Goals and tracked. Zero-waste verified against source (no new Go deps; `metrics.ValidateProjectPath` exists at metrics/security.go:61; `//go:embed`+defensive-copy pattern exists at metrics/schema.go:5-6; cobra already present). Coverage Strategy IS defined (design.md:255-286, ≥80% targets) so NO Constitution-IV CRITICAL. Gatekeeping clean: new delta thresholds (new cycle, ΔI≥0.15, ΔD≥0.20, ΔLCOM≥2) are orthogonal NEW gates that don't weaken the existing absolute-threshold CLI flags; D5 declares them protected once ratified. Two MEDIUM accuracy defects: (1) proposal.md:25-28/86 + design.md:139-141/310-314 falsely claim divisor-entropy is 'the only divisor that enables bash' / 'all existing divisors use bash: deny' — FALSE: .opencode/agents/divisor-curator.md:8-13 already uses the identical `"*":"deny"`+specific-`allow` granular bash pattern (temp 0.2, and uses "ask" for its one mutating cmd gh issue create); the design misses curator as de-risking prior art. (2) proposal.md:87 Principle VI row claims unqualified 'preserves determinism' but design D2 (line 85) + R1 (305-308) disclose the base↔PR delta is LLM-computed and NON-deterministic — table should read 'PASS with disclosed tension' matching the honest PARTIAL already on Principle III. Two LOW: 'read-only git subcommands' mislabels fetch/worktree-add (they mutate local state) in the Principle V row (proposal.md:86, spec.md:112-114); design.md:23 over-generalizes 'existing divisors use temperature 0.1' (curator 0.2, herald 0.4, envoy 0.5; 0.1 only for adversary/architect/guard/sre/testing/scribe). Repo context: .golangci.yml exists but NO .github/workflows/ directory in-tree despite CHANGELOG.md:40-42 claiming ci.yml — pre-existing, out of scope, but means no existing coverage-threshold gate exists. Recurring Guard pattern for this project (matches prior go-analyze/universal-coupling Envoy reviews): verify Constitution Alignment table rows against the design body's own disclosed tensions (require 'PASS with deferral/tension' wording), and verify 'only/all/every existing X' novelty claims against actual sibling-agent frontmatter before accepting them. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T190914-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T190914-jay-flowers.md new file mode 100644 index 0000000..bc1ed57 --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T190914-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: decision +created_at: 2026-08-31T19:09:14Z +identity: divisor-entropy-agent-20260831T190914-jay-flowers +tier: draft +--- + +Adversary council review (Spec Review Mode, iteration 2) of vibe-check OpenSpec change `add-divisor-entropy-agent` (divisor-entropy Review Council agent + `vibe-check diff` delta/verdict engine + `vibe-check init` scaffolder). VERDICT: REQUEST CHANGES; Security & Resilience score 7/10 (up from 6/10 in iter 1). Iteration-1 findings resolved: H1 bash allowlist LARGELY resolved (narrowed to `git worktree add/remove/prune`+`git fetch origin *`; last-matching-rule precedence now VERIFIABLE — confirmed via customize-opencode skill: 'opencode evaluates the LAST matching rule, broad rules first, narrow last'; ref sanitization `^[A-Za-z0-9._/-]+$` + `git rev-parse --verify --end-of-options`; contract test set-equality). H2 threat model resolved at doc level (design D2 lines 100-109 GOTOOLCHAIN=local + trusted-refs-only; proposal Constitution V downgraded to PARTIAL). M1/M2/M3/L1 fully resolved. THREE residual/new MEDIUMs remain: (1) GOTOOLCHAIN=local mitigation is UNENFORCEABLE AS SPECIFIED — resolve.go packageEnvAllowlist (GOPATH/GOROOT/GOMODCACHE/GOPROXY/GONOSUMCHECK/GOMOD) omits GOTOOLCHAIN, so the analyze subprocess runs effective GOTOOLCHAIN=auto (honors go.mod toolchain directive → download+exec from GOPROXY); AND the D4 bash allowlist `vibe-check analyze *` would DENY an env-prefixed `GOTOOLCHAIN=local vibe-check analyze ...` (doesn't match the anchored pattern). Fix: force GOTOOLCHAIN=local inside `vibe-check analyze` (SanitizeEnvironment) + add task/AC. (2) D4 asserts 'metacharacter/compound commands denied by catch-all' as fact, but this is NOT in authoritative OpenCode docs (skill only documents precedence, not compound-command parsing); the set-equality contract test only checks STATIC frontmatter, not runtime enforcement — residual verification gap (compensated by ref regex excluding metachars). (3) NEW resilience gap from the added diff command: partial builds (resolve.go lines 94-114 emit load-error warnings + ZEROED type metrics for failed packages, analyze still exits 0 with a valid graph) are not accounted for by ComputeDelta/DecideVerdict or the agent degradation clause — a PR that partially builds yields spurious deltas over zeroed metrics and can produce the FALSE APPROVE that D10 explicitly forbids (or a false REQUEST_CHANGES). ModuleGraph carries Warnings but diff-command spec never consumes them. Recurring Adversary pattern for this project: cross-reference threat-model MUSTs against the actual reused code (security.go/resolve.go) and the agent's own bash allowlist to catch mitigations that are stated but not wired; and verify asserted OpenCode runtime-permission behavior against the customize-opencode skill/schema rather than accepting design prose. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T192522-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T192522-jay-flowers.md new file mode 100644 index 0000000..66adaed --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T192522-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: decision +created_at: 2026-08-31T19:25:22Z +identity: divisor-entropy-agent-20260831T192522-jay-flowers +tier: draft +--- + +Architect council review (Spec Review Mode, iteration 2) of vibe-check OpenSpec change `add-divisor-entropy-agent` (divisor-entropy Review Council agent + new `vibe-check diff` delta/verdict engine [metrics/delta.go ComputeDelta/GraphDelta + metrics/verdict.go DecideVerdict/Verdict/VerdictThresholds] + `vibe-check init` scaffolder). VERDICT: REQUEST CHANGES; Architectural Alignment 6/10. All 5 iteration-1 findings RESOLVED: (1) "only divisor that enables bash" reframed as "second after divisor-curator" mirroring its `"*":"deny"`+allowlist shape, temps corrected curator=0.2/architect=0.1 (verified vs frontmatter); (2) scaffold.Scaffold→scaffold.Run(opts Options)(*Result,error) (AP-002 no-stutter); (3) AP-006 marker added+contract-tested; (4) Result gained Forced; (5) RunInit honors ctx.Err, root-must-exist precondition explicit matching security.go:61 ValidateProjectPath, drift check uses --force, contract test asserts description. Architecture proper is 8-caliber: correct Layer-1 metrics/ (pure ComputeDelta/DecideVerdict) vs Layer-3 cmd/ (thin RunDiff) split mirroring compute.go+analyze.go; AP-001/002/003/004/007 conformant; scaffold.Run vs cmd RunAnalyze/RunInit/RunDiff correctly applies AP-002 per-package; diff-command set-equality cycle identity (spec:24-28) is EXACT because metrics.Cycle is the sorted member set of a Tarjan SCC (cycle.go/cycles.go) — SCCs uniquely identified by member set. HEADLINE HIGH (compound, 3 facets — the delta/verdict data model is narrower than both the ModuleGraph it consumes AND the agent spec it serves, and H4 made `vibe-check diff` the SOLE verdict authority so anything DecideVerdict can't express is unreachable): (a) diff-command spec:7,12 requires abstractness(A) delta but Delta struct (tasks 2.1) omits A → diff's own scenario can't pass; (b) divisor-entropy-agent spec:80-82 + design D5:184,189-190 require "new package in zone of pain/uselessness → REQUEST_CHANGES / new-but-healthy → COMMENT" but diff-command spec:49 + DecideVerdict implement only new-cycle/ΔI/ΔD/ΔLCOM, give added packages NO delta, and GraphDelta carries no Zone → new-package regressions silently APPROVE (litmus: no deterministic test writable from signature); (c) ComputeDelta/DecideVerdict never consume ModuleGraph.Warnings (graph.go:22-24) so partial builds (analyze exits 0 with zeroed metrics per resolve.go) yield spurious deltas — D10 degradation only covers TOTAL analyze failure. MEDIUM: design Non-Goal:54 "Changes to metrics APIs" contradicts proposal:78-82 new metrics surface (stale post-H4) — matches known repo pattern "non-goal needs refinement not spec". MEDIUM: diff uses INCLUSIVE ≥ boundaries (diff spec:61-79) vs analyze's established STRICT > "value==threshold passes" (analyze-command spec:253-256, analyze.go:105) — defensible (delta-trigger vs absolute-ceiling) but UNACKNOWLEDGED in design (contrast: exit-code divergence {0,2} vs {0,1,2} IS justified at diff spec:101). LOWs: entropy-direction is a required output with no struct home; init symlink-escape check EvalSymlinks a not-yet-existing dest (breaks primary create-when-missing scenario, validate deepest existing ancestor); AP-006 literal `v{version}` ships unsubstituted given copy-only Run+drift-check and mislabels repo's actual `uf vdev` marker; metrics/ now carries verdict/policy (cohesion nit); diff threshold flags "mirroring analyze" conflate absolute-ceiling names with delta semantics; new specs use WHEN/THEN vs repo's established GIVEN/WHEN/THEN (DR-003). Recurring Architect pattern for this project: when an agent prompt is refactored to delegate a decision to a tested Go function (H4-style), verify the function's INPUT data model (Delta/GraphDelta) can actually express every MUST-clause the agent spec still asserts and consumes every field of the entity it differences (ModuleGraph.Warnings/Zone) — prose gates that outrun the data model become unreachable; and cross-check numeric-gate boundary conventions (≥ vs >) against sibling commands. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T210347-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T210347-jay-flowers.md new file mode 100644 index 0000000..e416e1f --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T210347-jay-flowers.md @@ -0,0 +1,23 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: context +created_at: 2026-08-31T21:03:47Z +identity: divisor-entropy-agent-20260831T210347-jay-flowers +tier: draft +--- + +Handoff state for OpenSpec change `add-divisor-entropy-agent` (GitHub issue #3, branch opsx/add-divisor-entropy-agent), a divisor-entropy Review Council agent + supporting CLI for vibe-check. STATUS at handoff: all six spec artifacts are revised and `openspec validate add-divisor-entropy-agent` passes; the `` marker is NOT yet written; implementation (code) has NOT started. This is /uf.unleash Step 6 (spec review) rework — spec review reached iteration 2 (REQUEST CHANGES, all findings addressed) and the artifacts were revised; a fresh council pass is the next step. + +ARTIFACTS under openspec/changes/add-divisor-entropy-agent/: proposal.md; design.md (Decisions D1-D10 + Coverage Strategy + Risks + Open Questions); tasks.md (11 task groups); specs/divisor-entropy-agent/spec.md (8 requirements); specs/diff-command/spec.md (6 requirements incl Partial-Build Degraded Verdict); specs/init-command/spec.md (7 requirements); specs/analyze-command/spec.md (Hermetic Toolchain requirement). + +KEY DESIGN DECISIONS already settled across two review iterations (do NOT re-litigate — they are deliberate): +1. Delta + verdict are computed by a PURE, table-tested Go function in metrics/ (ComputeDelta, GraphDelta, DecideVerdict, Verdict, VerdictThresholds) exposed via a new `vibe-check diff ` command; the divisor-entropy agent runs `vibe-check analyze` on both refs then `vibe-check diff` — it does NOT compute deltas in-prompt (resolves Constitution III/VI + testability). +2. Verdict gates EXACTLY: any new cycle | ΔInstability≥0.15 | ΔDistance≥0.20 | ΔLCOM≥2 → REQUEST_CHANGES; smaller positive shifts → COMMENT; improve/stable → APPROVE. Each float delta is rounded to 4 decimals (1e-4) before the ≥ comparison. Inclusive ≥ (vs analyze's strict >) is intentional. These defaults are PROTECTED gates. +3. The zone-of-pain/uselessness gate for NEW packages was DROPPED (absolute new-package quality is already enforced by `vibe-check analyze --max-distance`; the entropy divisor is strictly a delta/regression tool). Added/removed packages are reported but never trigger delta gates. +4. `vibe-check analyze` FORCES GOTOOLCHAIN=local in its sanitized analysis subprocess env (forced value, not host passthrough) so an untrusted target go.mod toolchain directive cannot trigger a toolchain download+exec. The agent cannot set env inline (its bash allowlist only permits bare `vibe-check analyze *`). Privileged CI SHOULD also export GOTOOLCHAIN=local ambiently. +5. Partial-build safety: `vibe-check diff` inspects ModuleGraph.Warnings and Status!='complete' on either input; unreliable measurement → COMMENT (never a false APPROVE), annotate affected packages, suppress added/removed noise (Partial-Build Degraded Verdict requirement). +6. divisor-entropy is the SECOND divisor to enable bash (after divisor-curator, which it mirrors: `"*": "deny"` catch-all + allowlist). temperature: 0.1 matches divisor-architect (divisor-curator uses 0.2). Bash allowlist narrowed to: git merge-base *, git rev-parse *, git worktree add *, git worktree remove *, git worktree prune, git fetch origin *, vibe-check analyze *, vibe-check diff *. Ref sanitization (regex ^[A-Za-z0-9._/-]+$ plus `git rev-parse --verify --end-of-options ^{commit}`) is the PRIMARY metacharacter defense; OpenCode catch-all denial of compound/metachar commands is framed as defense-in-depth (assumed, not guaranteed). +7. Constitution alignment: III PARTIAL (analyze/diff/init --json payloads lack provenance metadata), V PARTIAL (analyze executes target build tooling; mitigated by GOTOOLCHAIN=local + trusted-refs-only), VI PASS (deltas/verdict are exact, deterministic, table-tested Go). Full P2 longitudinal architectural-drift tracking remains future work; this change delivers only the point-in-time base↔PR primitive. + +RESUME INSTRUCTIONS: start a fresh session, run /uf.unleash on branch opsx/add-divisor-entropy-agent. It resumes at Step 6 (artifacts exist + validate clean), re-runs the 4-divisor council (Adversary/Architect/Guard/Tester) in spec-review mode; if all APPROVE (Guard may APPROVE WITH ADVISORIES) it writes the spec-review-passed marker and proceeds to Step 7 Implement per tasks.md (metrics/delta.go, metrics/verdict.go, internal/scaffold/, cmd/vibe-check/init.go + diff.go, embedded divisor-entropy.md asset, tests) with a per-phase CI hard gate (go build ./..., go test -race -count=1 ./..., go vet ./..., golangci-lint run ./...). Constraint until spec review passes: spec-only edits under openspec/changes/add-divisor-entropy-agent/; no implementation code; no commits. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T212924-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T212924-jay-flowers.md new file mode 100644 index 0000000..20c21de --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T212924-jay-flowers.md @@ -0,0 +1,25 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: context +created_at: 2026-08-31T21:29:24Z +identity: divisor-entropy-agent-20260831T212924-jay-flowers +tier: draft +--- + +RESUME BREADCRUMB for /uf.unleash on OpenSpec change `add-divisor-entropy-agent` (branch opsx/add-divisor-entropy-agent, GitHub issue #3, repo vibe-check), dated 2026-08-31. This supersedes learning/divisor-entropy-agent-20260831T210347-jay-flowers. STATUS: /uf.unleash is PAUSED at Step 6 (Spec Review). A fresh 4-divisor council (Adversary/Architect/Guard/Testing, spawned directly as parallel subagents in Spec Review Mode, OpenSpec tier) ran spec-review iteration 1 of this resume and returned COUNCIL VERDICT = REQUEST CHANGES. Scores: Adversary 5/10, Architect Alignment 6/10 + Domain 7/10, Guard 7/10, Testing 6/10. Both Guard and Architect stated: resolve the HIGH findings (and ideally the MEDIUMs) and this is a clean APPROVE. openspec validate --strict still passes. tasks.md still has ALL checkboxes [ ] and NO marker (correct; must NOT be written until council APPROVES). No implementation code written, no commits made (phase boundary respected). + +FIVE HIGH FINDINGS BLOCK (each is report-only per hybrid policy; all require human/design revision of spec artifacts under openspec/changes/add-divisor-entropy-agent/): +HIGH-1 (TRIPLE CONSENSUS Architect+Guard+Testing, safety-critical, compound-escalates toward CRITICAL): The 'Partial-Build Degraded Verdict' requirement (specs/diff-command/spec.md:113-127; depended on by specs/divisor-entropy-agent/spec.md:208-213) has NO implementing task, NO test, NO fixture. tasks.md only defines the unreliable flag (2.1) and vaguely 'sets' it (2.2) but never specifies the SOURCE (inspect both ModuleGraphs' Warnings + Status!='complete'), the CONSEQUENCE (force COMMENT / annotate affected packages / suppress added-removed signal), WHERE it lives (spec says RunDiff; DecideVerdict(GraphDelta,...) is the testable home), and no test in 2.4/3.5/9.2 plus omitted from design.md:350-361 Coverage Strategy. Failure mode = FALSE APPROVE on degradation (partial build zeroes type metrics -> looks like improvement), the inverse of AC#7's 'never a false APPROVE'. This is the unresolved THIRD LEG of the prior compound finding (Warnings unconsumed). FIX: add tasks (a) ComputeDelta sets unreliable flag from Warnings/Status; (b) DecideVerdict/RunDiff forces COMMENT+annotate+suppress + flag in --json; (c) partial-build unit case 2.4, CLI case 3.5, fixture 9.2; (d) add to design.md Coverage Strategy; cite 'Partial-Build Degraded Verdict' in Satisfies. +HIGH-2 (Adversary): Least-privilege bash allowlist cannot materialize analyze output into the files diff needs. `vibe-check analyze` writes JSON to STDOUT ONLY (no --output flag; verified analyze.go:100) and `vibe-check diff` takes FILE-PATH positionals (integrated-ref mode was rejected in D2). Agent has no permitted way to persist stdout (redirection `>` denied by catch-all, edit denied). FIX: add `vibe-check analyze --output ` (-o) flag (already covered by the `vibe-check analyze *` allow entry), specify in analyze-command/spec.md + tasks, update agent workflow task 4.3. Do NOT fix by permitting redirection/edit (would defeat metachar denial). Genuine design addition to the analyze surface. +HIGH-3 (Adversary): `git check-ref-format` is mandated MUST (specs/divisor-entropy-agent/spec.md:137-149) but is NOT one of the exactly-8 allowlisted bash prefixes, so the catch-all '*':'deny' denies it at runtime AND the set-equality contract test (task 6.4) forbids adding it. Compounding: the 'primary metachar defense' regex ^[A-Za-z0-9._/-]+$ PERMITS '.' and '/' hence '..' (e.g. ../../x passes), and check-ref-format is exactly the control that rejects ..,@{,leading -,.lock. FIX (needs human decision, touches PROTECTED D6 allowlist): either (a) add `git check-ref-format *` -> 9 entries and update EVERY enumeration + the 6.4 set-equality test (design D4 + coverage strategy, agent spec x2, tasks 4.1/6.4); or (b) drop the MUST-check-ref-format requirement and state regex + `git rev-parse --verify --end-of-options ^{commit}` are the complete controls. Reconcile 'exactly N' everywhere; clarify regex does not stop ../slash. +HIGH-4 (Adversary): 'Trusted-refs-only' — the SOLE mitigation for residual cgo/build-execution RCE (GOTOOLCHAIN=local closes only the toolchain-directive vector; cgo still compiles/executes attacker C) — lives ONLY in design.md:103-117 (D2). It is NOT a normative requirement in divisor-entropy-agent/spec.md and NO task requires the SHIPPED divisor-entropy.md (deployed to consumers via init) to document it; analyze-command/spec.md:11 references a 'trusted-refs-only operating constraint documented for the agent' that no artifact actually produces. FIX: add normative requirement to divisor-entropy-agent/spec.md ('MUST only run on trusted refs; MUST NOT be wired into untrusted-fork-PR CI'); require a 'Security/Operating Constraints' section in the embedded asset; add a 6.4 contract-test assertion for that section. +HIGH-5 (Testing; Adversary LOW): Provenance-marker version injection unspecified and contradicts the contract test. Task 4.2 requires the AP-006 marker carry the REAL binary version resolved at scaffold time, but task 6.4 asserts the marker present by parsing the EMBEDDED (pre-build) asset which cannot hold a real version; no substitution mechanism exists (scaffold.Options has no Version field, internal/scaffold cannot import cmd/vibe-check's version var), and dogfood 9.1 `go run` bakes version='dev', making the D8/R7 regenerate-and-`git diff --exit-code` drift check non-deterministic. Existing convention (uf.init.md:864) matches marker PREFIX only. FIX: pick one and make tasks consistent — (A) keep STABLE marker in embedded asset, 6.4 asserts PREFIX `` is appended to tasks.md and the pipeline proceeds to Step 7 Implement. diff --git a/.uf/dewey/learnings/divisor-entropy-agent-20260831T231711-jay-flowers.md b/.uf/dewey/learnings/divisor-entropy-agent-20260831T231711-jay-flowers.md new file mode 100644 index 0000000..d8f553f --- /dev/null +++ b/.uf/dewey/learnings/divisor-entropy-agent-20260831T231711-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: divisor-entropy-agent +author: jay-flowers +category: context +created_at: 2026-08-31T23:17:11Z +identity: divisor-entropy-agent-20260831T231711-jay-flowers +tier: draft +--- + +Resume breadcrumb for /uf.unleash on OpenSpec change `add-divisor-entropy-agent` (GitHub issue #3, branch opsx/add-divisor-entropy-agent, project /Users/jflowers/Projects/github/zero-dot-force/vibe-check, FEATURE_DIR=openspec/changes/add-divisor-entropy-agent/). STATUS: Step 6 Spec Review is COMPLETE. All 6 OpenSpec artifacts (proposal.md, design.md, tasks.md, and specs/analyze-command|diff-command|divisor-entropy-agent|init-command/spec.md) were revised across two review-council iterations. Iteration 1 returned REQUEST CHANGES with 5 HIGH findings (HIGH-1 Partial-Build Degraded Verdict had no task/test/fixture; HIGH-2 least-privilege bash allowlist could not materialize analyze output to files; HIGH-3 git check-ref-format was mandated but blocked by the exactly-8 allowlist; HIGH-4 trusted-refs-only mitigation was not normative nor in the shipped asset; HIGH-5 provenance version marker was unspecified and contradicted the contract test) plus several MEDIUM/LOW — ALL remediated. Iteration 2 returned all-APPROVE (Adversary 9/10, Architect Alignment 9/10 + Domain 9/10, Guard 8/10 APPROVE-WITH-ADVISORIES, Testing 9/10) with zero HIGH/CRITICAL, and its F1-F7 LOW/MEDIUM advisories were then auto-fixed. `openspec validate add-divisor-entropy-agent --strict` PASSES (exit 0). The `` marker is written at the very end of tasks.md; there is NO `` marker yet; ALL task checkboxes in tasks.md are still `- [ ]` (implementation NOT started). Key settled design decisions carried through the revisions: the structural delta + verdict engine is pure table-tested Go in metrics/ (ComputeDelta/GraphDelta/DecideVerdict/Verdict/VerdictThresholds) exposed via a new `vibe-check diff `; verdict gates (PROTECTED) are any new cycle OR ΔInstability≥0.15 OR ΔDistance≥0.20 OR ΔLCOM≥2 → REQUEST_CHANGES, smaller positive → COMMENT, improve/stable → APPROVE, floats rounded 4dp round-half-away-from-zero before the inclusive ≥; the divisor-entropy agent has a NINE-entry bash allowlist (git merge-base *, git rev-parse *, git worktree add *, git worktree remove *, git worktree prune, git fetch origin *, git check-ref-format *, vibe-check analyze *, vibe-check diff *); `vibe-check analyze` forces GOTOOLCHAIN=local and gains an --output/-o flag; the embedded asset carries a stable version-less marker `` and a `## Security / Operating Constraints` section; nil ComputeDelta inputs set the unreliable flag → COMMENT (never false APPROVE). TO RESUME: simply re-run /uf.unleash — the session-resume guard will detect the `` marker and the all-unchecked task boxes and resume at Step 7 Implement. Step 7 procedure: load the `pre-flight` skill to derive exact CI/build/test commands from .github/workflows/ (do NOT hardcode; AGENTS.md known commands are go build ./..., go test -race -count=1 ./..., go vet ./..., golangci-lint run ./...), then execute tasks.md's 11 task groups phase-by-phase (sequential non-[P] tasks first via cobalt-crush-dev, then [P] parallel tasks via replicator_forge_worktree_create/spawn_subtask/merge/cleanup with ≤4 workers/batch, marking each task [x] immediately), running the pre-flight skill in hard-gate mode as a checkpoint after each phase; then Step 8 Code Review (review council in Code Review Mode, up to 3 iterations, write `` on all-APPROVE), Step 9 Retrospective (store learnings), Step 10 Demo (verbatim output format). This handoff was made at the Step 6→Step 7 boundary because the session hit a sustained hard context limit; pausing at the durable spec-review marker is safer than starting the large implementation phase without context headroom. Supersedes the earlier resume breadcrumbs learning/divisor-entropy-agent-20260831T210347-jay-flowers and learning/divisor-entropy-agent-20260831T212924-jay-flowers. diff --git a/.uf/dewey/learnings/openspec-20260831T231732-jay-flowers.md b/.uf/dewey/learnings/openspec-20260831T231732-jay-flowers.md new file mode 100644 index 0000000..84806ea --- /dev/null +++ b/.uf/dewey/learnings/openspec-20260831T231732-jay-flowers.md @@ -0,0 +1,10 @@ +--- +tag: openspec +author: jay-flowers +category: gotcha +created_at: 2026-08-31T23:17:32Z +identity: openspec-20260831T231732-jay-flowers +tier: draft +--- + +OpenSpec validation gotcha (vibe-check / openspec CLI): `openspec validate --strict` requires that each ADDED requirement's STATEMENT — the prose between the `### Requirement: ` header and its first `#### Scenario:` — LEAD with a normative RFC-2119 clause containing SHALL or MUST. A requirement whose statement opens with a rationale sentence (e.g. "Because vibe-check analyze executes the target's build tooling...") FAILS strict validation with an error like "requirement '' must contain SHALL or MUST", even when MUST/SHALL keywords appear later in the same paragraph. FIX: reorder the paragraph so the first sentence is the normative statement (e.g. "The divisor-entropy agent SHALL only run on refs the CI context already trusts and MUST NOT be wired into CI that analyzes untrusted fork pull requests."), then follow with the rationale. Encountered while authoring the 'Trusted-Refs-Only Operating Constraint' requirement in specs/divisor-entropy-agent/spec.md for the add-divisor-entropy-agent change. diff --git a/AGENTS.md b/AGENTS.md index ef6f636..39135c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -241,6 +241,20 @@ go build ./... # Build the CLI binary go build ./cmd/vibe-check +# Deploy embedded Review Council agent assets into .opencode/agents/ of a project +go run ./cmd/vibe-check init . # --force to overwrite, --json for machine output + +# Analyze a module and write ModuleGraph JSON to a file (default: stdout) +go run ./cmd/vibe-check analyze -o graph.json ./... # --output is the long form + +# Compare two ModuleGraph snapshots (base vs PR): entropy delta + verdict +go run ./cmd/vibe-check diff base.json pr.json # --json for machine output + +# Note: `vibe-check analyze` forces GOTOOLCHAIN=local for its go/packages +# subprocess, so analysis never downloads a toolchain named by a target +# module's go.mod. Trade-off: a trusted module whose go.mod `toolchain` +# directive requires a newer-than-local Go must be built/analyzed manually. + # Test (with race detection) go test -race -count=1 ./... @@ -260,7 +274,9 @@ golangci-lint run ./... cmd/vibe-check/ # CLI entry point (Layer 3) main.go # Binary entry point with ldflags version embedding root.go # Cobra root command with --version flag - analyze.go # analyze subcommand with threshold flags + analyze.go # analyze subcommand with threshold flags (incl --output/-o) + diff.go # diff subcommand (base vs PR entropy delta + verdict) + init.go # init subcommand (deploys embedded agent assets) internal/goadapter/ # Go language adapter (Layer 2) adapter.go # Adapter struct implementing metrics.Adapter resolve.go # Package loading via go/packages @@ -270,10 +286,18 @@ internal/goadapter/ # Go language adapter (Layer 2) extensions.go # go.interfaceWidth and go.interfaceProximity extensions doc.go # Package-level GoDoc testdata/ # Test fixtures (coupling, types, lcom, extensions, partial) +internal/scaffold/ # Embedded agent-asset deployment for `vibe-check init` + doc.go # Package-level GoDoc + embed.go # //go:embed assets/agents/*.md (embedded source of truth) + scaffold.go # Symlink-safe asset writer (skip/force; 0o755 dirs, 0o644 files) + scaffold_test.go # Writer + embedded-asset contract tests + assets/agents/ # Embedded Review Council agent assets + divisor-entropy.md # Structural-entropy divisor agent (source of truth) metrics/ # Universal coupling metrics model (Layer 1) adapter.go # Adapter interface and Capability type compute.go # Metric computation functions cycle.go # Cycle type for circular dependency representation + delta.go # GraphDelta + ComputeDelta (base vs PR deltas, entropy direction) doc.go # Package-level GoDoc external.go # ExternalAdapter (JSON-RPC subprocess) graph.go # ModuleGraph and ModuleResult types (with Extensions) @@ -285,8 +309,10 @@ metrics/ # Universal coupling metrics model (Layer 1) security.go # Path validation and environment sanitization validate.go # JSON schema validation (accepts v1.0 and v1.1) values.go # Named metric types (Instability, Abstractness, etc.) + verdict.go # Verdict + DecideVerdict (protected entropy gate thresholds) warning.go # Warning type for analysis caveats zone.go # Zone and Status types + testdata/entropy/ # ModuleGraph diff fixtures + validation README (entropy divisor) openspec/ # OpenSpec change artifacts (proposals, specs, tasks) changes/ # Individual change directories schemas/ # Spec validation schemas @@ -300,7 +326,10 @@ The architecture follows a three-layer design per the RFC phasing: - **Layer 1** (`metrics/`): Language-agnostic universal model — Ca, Ce, Instability, Abstractness, Distance from Main Sequence, LCOM4, circular dependency detection, JSON schema validation, adapter - interface, and security primitives. + interface, security primitives, and the base↔PR entropy delta engine + (`ComputeDelta`) plus verdict engine (`DecideVerdict`) with protected + gate thresholds (ΔInstability ≥ 0.15, ΔDistance ≥ 0.20, ΔLCOM ≥ 2, or a + new circular dependency). - **Layer 2** (`internal/goadapter/`): Go language adapter implementing `metrics.Adapter`. Uses `golang.org/x/tools/go/packages` for type-aware dependency resolution, AST-based type classification, @@ -309,8 +338,11 @@ The architecture follows a three-layer design per the RFC phasing: (`go.interfaceWidth`, `go.interfaceProximity`). - **Layer 3** (`cmd/vibe-check/`): CLI entry point using cobra. Provides `vibe-check analyze` with threshold flags (`--max-instability`, - `--max-distance`, `--max-lcom`, `--no-circular-deps`, `--timeout`) - and JSON output. + `--max-distance`, `--max-lcom`, `--no-circular-deps`, `--timeout`, + `--output`/`-o`) and JSON output; `vibe-check diff ` + computing the entropy delta and verdict (with tighten-only threshold + overrides); and `vibe-check init [path]` deploying the embedded Review + Council agent assets into `.opencode/agents/`. RFC phasing status: diff --git a/CHANGELOG.md b/CHANGELOG.md index 89e1c3a..bd47eca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,43 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `vibe-check diff ` compares two ModuleGraph JSON + snapshots and reports the structural-entropy delta (per-module Ca, Ce, + instability, abstractness, distance, and LCOM deltas), new and resolved + circular dependencies, an entropy direction (improving/stable/ + degrading), and a verdict (APPROVE/COMMENT/REQUEST_CHANGES). Protected + gate thresholds — ΔInstability ≥ 0.15, ΔDistance ≥ 0.20, ΔLCOM ≥ 2, or a + new circular dependency — yield REQUEST_CHANGES; smaller non-zero shifts + yield COMMENT; improving or stable yields APPROVE. Exit code is 0 + whenever both inputs are valid (the verdict travels in the payload, not + the exit code); exit code 2 is reserved for missing, unreadable, or + schema-invalid input and for a looser-than-default (non-tightening) + threshold override. `--json` emits a machine-readable payload; the + `--max-instability-delta`, `--max-distance-delta`, and + `--max-lcom-delta` overrides are tighten-only. +- `vibe-check init [path]` deploys the embedded Review Council agent + assets into `.opencode/agents/` — skipping existing files by default, + overwriting with `--force`, and printing a machine-readable summary of + written/skipped/forced files with `--json`. Path defaults to `.`. +- `divisor-entropy` Review Council agent (embedded source of truth, + deployed by `vibe-check init`): measures the base↔PR design-quality + delta via `vibe-check analyze` and `vibe-check diff` inside an isolated + git worktree and reports a verdict. Runs only on trusted refs. +- `vibe-check analyze --output ` (`-o`) writes the ModuleGraph JSON + to a file instead of stdout (stdout remains the default; a failed write + exits with code 2 and a stderr diagnostic without emitting partial + stdout). + +### Changed + +- `vibe-check analyze` now forces `GOTOOLCHAIN=local` for its + `go/packages` subprocess, so analysis never downloads a toolchain named + by a target module's `go.mod`. Trade-off: a trusted module whose + `go.mod` `toolchain` directive requires a newer-than-local Go must be + built/analyzed manually. + ## [0.1.0] - 2026-08-31 Initial release: package-level design-quality and architectural metrics diff --git a/README.md b/README.md index 42ae5c8..20c7fe9 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,10 @@ vibe-check analyze [path] ``` `analyze` takes a single optional path to a Go module directory and defaults to the -current directory (`.`). It writes the analysis as JSON to stdout. +current directory (`.`). It writes the analysis as JSON to stdout, or to a file with +`--output`/`-o`. Analysis forces `GOTOOLCHAIN=local` for its internal `go/packages` +subprocess, so it never downloads a Go toolchain named by the target module's `go.mod` +(a trusted module that requires a newer-than-local toolchain must be built manually). ```bash # Analyze the current module @@ -49,6 +52,7 @@ vibe-check analyze ./myproject --max-instability 0.8 --no-circular-deps | `--max-lcom` | int | unset (no check) | Fail if any module's LCOM4 exceeds this value. Must be `>= 1`. | | `--no-circular-deps` | bool | `false` | Treat any detected circular dependency as a violation. | | `--timeout` | duration | none | Bound total analysis time (e.g., `30s`, `2m`). No timeout by default. | +| `--output`, `-o` | string | stdout | Write the ModuleGraph JSON to a file instead of stdout. Exits `2` (with no partial stdout) if the file cannot be written. | | `--version` | — | — | Print version, commit, and build date, then exit. Use on the root command: `vibe-check --version`. | Threshold comparisons use strict greater-than: a metric exactly equal to the threshold @@ -114,6 +118,43 @@ analysis a `module`. Throughout vibe-check's output — every `modules[]` entry and each `VIOLATION: module ...` line on stderr — one `module` corresponds to one Go package. +## Comparing snapshots: `vibe-check diff` + +`diff` compares two `analyze` JSON snapshots — a base and a PR — and reports the change in +design-quality metrics plus a verdict: + +```bash +vibe-check analyze --output base.json ./... # on the base revision +vibe-check analyze --output pr.json ./... # on the PR revision +vibe-check diff base.json pr.json # add --json for machine-readable output +``` + +`diff` computes per-module deltas (Ca, Ce, instability, abstractness, distance, LCOM), new +and resolved cycles, an entropy direction (`improving`, `stable`, or `degrading`), and a +verdict — `APPROVE`, `COMMENT`, or `REQUEST_CHANGES`. The protected gates are +Δinstability ≥ 0.15, Δdistance ≥ 0.20, ΔLCOM ≥ 2, or a new circular dependency (any of +which yields `REQUEST_CHANGES`); smaller non-zero shifts yield `COMMENT`; an improving or +flat delta yields `APPROVE`. A partial/unreliable input is always downgraded to `COMMENT`. + +It exits `0` whenever both inputs are valid — the verdict is data in the payload, so a +`REQUEST_CHANGES` verdict still exits `0` — and `2` when an input is missing, unreadable, or +schema-invalid, or when a `--max-instability-delta`, `--max-distance-delta`, or +`--max-lcom-delta` override is looser than the protected default (overrides may only tighten). + +## Deploying agents: `vibe-check init` + +`init` deploys the embedded Review Council agent assets into a project's `.opencode/agents/` +directory: + +```bash +vibe-check init [path] # path defaults to "."; --force to overwrite, --json for machine output +``` + +It writes the bundled `divisor-entropy` agent — a structural-entropy reviewer that runs +`analyze` + `diff` across a base↔PR pair in an isolated worktree — and skips files that +already exist unless `--force` is given. It exits `0` on success (including when every asset +is skipped) and `2` on an invalid target path or I/O failure. + ## Known limitations - `analyze` accepts a single path argument (defaulting to `.`); it does not take multiple diff --git a/cmd/vibe-check/analyze.go b/cmd/vibe-check/analyze.go index 1d15690..4a6b8a0 100644 --- a/cmd/vibe-check/analyze.go +++ b/cmd/vibe-check/analyze.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "os" "os/signal" "syscall" "time" @@ -24,6 +25,9 @@ type AnalyzeOptions struct { Stderr io.Writer // Path is the project directory to analyze. Path string + // OutputPath, when non-empty, names a file to which the ModuleGraph JSON is + // written (mode 0o644) instead of Stdout. Empty (the default) writes to Stdout. + OutputPath string // MaxInstability is the threshold for instability violations. // nil means not set (no threshold check). Must be in [0.0, 1.0]. @@ -97,7 +101,14 @@ func RunAnalyze(ctx context.Context, opts AnalyzeOptions) (*AnalyzeResult, error return &AnalyzeResult{ExitCode: 2}, fmt.Errorf("analyze: %w", ctx.Err()) } - if _, err := fmt.Fprintln(opts.Stdout, string(data)); err != nil { + // Step 5b: Emit the graph. With --output set, write to the file and NOTHING + // to stdout (not even a partial write on error); otherwise preserve the + // stdout default. Threshold checking still runs after a successful write. + if opts.OutputPath != "" { + if err := os.WriteFile(opts.OutputPath, data, 0o644); err != nil { + return &AnalyzeResult{ExitCode: 2}, fmt.Errorf("write output file %s: %w", opts.OutputPath, err) + } + } else if _, err := fmt.Fprintln(opts.Stdout, string(data)); err != nil { return &AnalyzeResult{ExitCode: 2}, fmt.Errorf("write output: %w", err) } @@ -201,6 +212,7 @@ func analyzeCmd() *cobra.Command { maxLCOM int noCircularDeps bool timeout time.Duration + output string ) cmd := &cobra.Command{ @@ -235,6 +247,7 @@ JSON output is always written to stdout, even when violations are detected.`, Stdout: cmd.OutOrStdout(), Stderr: cmd.ErrOrStderr(), Path: path, + OutputPath: output, NoCircularDeps: noCircularDeps, Timeout: timeout, } @@ -278,6 +291,7 @@ JSON output is always written to stdout, even when violations are detected.`, cmd.Flags().IntVar(&maxLCOM, "max-lcom", 0, "Maximum allowed LCOM value (>= 1)") cmd.Flags().BoolVar(&noCircularDeps, "no-circular-deps", false, "Treat circular dependencies as violations") cmd.Flags().DurationVar(&timeout, "timeout", 0, "Analysis timeout (e.g., 30s, 2m)") + cmd.Flags().StringVarP(&output, "output", "o", "", "Write ModuleGraph JSON to file instead of stdout") return cmd } diff --git a/cmd/vibe-check/analyze_test.go b/cmd/vibe-check/analyze_test.go index 97216e9..89b5345 100644 --- a/cmd/vibe-check/analyze_test.go +++ b/cmd/vibe-check/analyze_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "os" "path/filepath" "runtime" "strings" @@ -1025,3 +1026,136 @@ func TestCheckThresholds(t *testing.T) { }) } } + +// --- Task 2.6: --output / -o flag --- + +func TestRunAnalyze_OutputFileWritesGraphNotStdout(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("skipping integration test") + } + + outPath := filepath.Join(t.TempDir(), "graph.json") + var stdout, stderr bytes.Buffer + opts := AnalyzeOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: couplingFixtureDir(t), + OutputPath: outPath, + } + + result, err := RunAnalyze(context.Background(), opts) + if err != nil { + t.Fatalf("RunAnalyze returned error: %v", err) + } + if result.ExitCode != 0 { + t.Errorf("ExitCode: got %d, want 0", result.ExitCode) + } + + // Nothing must be echoed to stdout when writing to a file. + if stdout.Len() != 0 { + t.Errorf("stdout should be empty when --output is set, got %d bytes", stdout.Len()) + } + + // The file must contain schema-valid ModuleGraph JSON. + data, err := os.ReadFile(outPath) + if err != nil { + t.Fatalf("read output file: %v", err) + } + if len(data) == 0 { + t.Fatal("output file is empty, expected JSON") + } + if err := metrics.Validate(data); err != nil { + t.Errorf("output file failed schema validation: %v", err) + } +} + +func TestRunAnalyze_NoOutputFlagWritesStdout(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("skipping integration test") + } + + var stdout, stderr bytes.Buffer + opts := AnalyzeOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: couplingFixtureDir(t), + } + + result, err := RunAnalyze(context.Background(), opts) + if err != nil { + t.Fatalf("RunAnalyze returned error: %v", err) + } + if result.ExitCode != 0 { + t.Errorf("ExitCode: got %d, want 0", result.ExitCode) + } + if stdout.Len() == 0 { + t.Fatal("stdout is empty, expected JSON output when --output is absent") + } + if err := metrics.Validate(stdout.Bytes()); err != nil { + t.Errorf("stdout JSON failed schema validation: %v", err) + } +} + +func TestRunAnalyze_UnwritableOutputPathExitsTwo(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("skipping integration test") + } + + // A path under a nonexistent subdirectory cannot be created by os.WriteFile, + // which does not create parent directories. + outPath := filepath.Join(t.TempDir(), "nonexistent-subdir", "out.json") + var stdout, stderr bytes.Buffer + opts := AnalyzeOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: couplingFixtureDir(t), + OutputPath: outPath, + } + + result, err := RunAnalyze(context.Background(), opts) + if err == nil { + t.Fatal("expected error for unwritable output path, got nil") + } + if result.ExitCode != 2 { + t.Errorf("ExitCode: got %d, want 2", result.ExitCode) + } + // No partial graph must be emitted to stdout on output-file failure. + if stdout.Len() != 0 { + t.Errorf("stdout should be empty on output-file failure, got %d bytes", stdout.Len()) + } +} + +// TestAnalyzeCommand_OutputFlagWiring drives the -o short flag through the cobra +// command (Execute) to cover the StringVarP registration and the OutputPath +// wiring, asserting the file is written and stdout stays empty. +func TestAnalyzeCommand_OutputFlagWiring(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("skipping integration test") + } + + outPath := filepath.Join(t.TempDir(), "graph.json") + cmd := rootCmd() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"analyze", "-o", outPath, couplingFixtureDir(t)}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: unexpected error: %v\nstderr: %s", err, errOut.String()) + } + if out.Len() != 0 { + t.Errorf("stdout should be empty when -o is set, got %d bytes", out.Len()) + } + + data, err := os.ReadFile(outPath) + if err != nil { + t.Fatalf("read output file: %v", err) + } + if err := metrics.Validate(data); err != nil { + t.Errorf("output file failed schema validation: %v", err) + } +} diff --git a/cmd/vibe-check/diff.go b/cmd/vibe-check/diff.go new file mode 100644 index 0000000..ea90f5b --- /dev/null +++ b/cmd/vibe-check/diff.go @@ -0,0 +1,404 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os" + "os/signal" + "strings" + "syscall" + "text/tabwriter" + + "github.com/spf13/cobra" + + "github.com/zero-dot-force/vibe-check/metrics" +) + +// DiffOptions contains the configuration for the diff command. +// It follows the AP-001 Options struct pattern for testable CLI commands. +type DiffOptions struct { + // Stdout is the writer for the delta report (table or JSON). Required. + Stdout io.Writer + // Stderr is the writer for diagnostics and errors. Required. + Stderr io.Writer + // BasePath is the filesystem path to the base ModuleGraph JSON document. + BasePath string + // PRPath is the filesystem path to the PR ModuleGraph JSON document. + PRPath string + // Thresholds holds the verdict gates applied by DecideVerdict. Callers build + // this from metrics.DefaultVerdictThresholds, optionally tightened by the + // command layer's tighten-only override flags. + Thresholds metrics.VerdictThresholds + // JSON selects machine-readable JSON output when true; otherwise a + // human-readable table is written. + JSON bool +} + +// DiffResult contains the outcome of a diff comparison. +// It follows the AP-001 Result struct pattern. +type DiffResult struct { + // Delta is the computed structural delta between the base and PR graphs. + Delta metrics.GraphDelta + // Verdict is the entropy verdict rendered from the delta and thresholds. + Verdict metrics.Verdict + // Reasons lists the machine-readable reasons each gate fired. It is empty for + // an APPROVE verdict. + Reasons []string + // ExitCode is the process exit code: 0 when both inputs are valid and a + // verdict was computed, 2 on a tool failure (unreadable or schema-invalid + // input). A REQUEST_CHANGES verdict still exits 0 — the verdict travels in + // the payload, not the exit code. + ExitCode int +} + +// diffJSON is the machine-readable diff payload emitted under --json. Field +// declaration order determines JSON key order and every key uses camelCase. The +// added/removed fields use omitempty so they are absent when there is nothing to +// report; because ComputeDelta suppresses them (leaves them empty) whenever the +// measurement is unreliable, an unreliable payload never carries added/removed +// signal. +type diffJSON struct { + Verdict metrics.Verdict `json:"verdict"` + Reasons []string `json:"reasons"` + EntropyDirection metrics.EntropyDirection `json:"entropyDirection"` + Unreliable bool `json:"unreliable"` + Modules []metrics.Delta `json:"modules"` + Added []string `json:"added,omitempty"` + Removed []string `json:"removed,omitempty"` + NewCycles []metrics.Cycle `json:"newCycles"` + ResolvedCycles []metrics.Cycle `json:"resolvedCycles"` +} + +// RunDiff reads two ModuleGraph JSON documents, computes their structural delta, +// renders an entropy verdict, and writes a report to opts.Stdout. It is the +// testable entry point per AP-002/AP-003: all business logic lives here, not in +// the cobra command layer. +// +// Exit code semantics (also mirrored in the returned DiffResult.ExitCode): +// - 0: both inputs were read and validated, the delta and verdict were +// computed, and the report was written. A REQUEST_CHANGES verdict still +// returns 0 — diff is a reporting tool and conveys the verdict in its +// payload, not in the process exit code. +// - 2: either input is missing/unreadable or fails ModuleGraph schema +// validation. In that case nothing is written to opts.Stdout and the +// returned error describes the failure for the command layer to report. +// +// RunDiff never writes a partial payload to opts.Stdout on any error path. +func RunDiff(ctx context.Context, opts DiffOptions) (*DiffResult, error) { + // Step 1: Read both inputs. A missing or unreadable file is a tool failure + // (exit 2) with no stdout output. + baseData, err := os.ReadFile(opts.BasePath) + if err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("read base file %s: %w", opts.BasePath, err) + } + prData, err := os.ReadFile(opts.PRPath) + if err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("read pr file %s: %w", opts.PRPath, err) + } + + // Step 2: Validate both documents against the ModuleGraph schema before + // computing anything. A schema-invalid input is a tool failure (exit 2). + if err := metrics.Validate(baseData); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("validate base file %s: %w", opts.BasePath, err) + } + if err := metrics.Validate(prData); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("validate pr file %s: %w", opts.PRPath, err) + } + + // Step 3: Unmarshal into typed graphs. Validation already parsed the JSON, so + // a failure here indicates a value that cannot map onto the typed model. + var base, pr metrics.ModuleGraph + if err := json.Unmarshal(baseData, &base); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("parse base file %s: %w", opts.BasePath, err) + } + if err := json.Unmarshal(prData, &pr); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("parse pr file %s: %w", opts.PRPath, err) + } + + // Step 4: Compute the delta and verdict. Both are pure and deterministic. + delta := metrics.ComputeDelta(&base, &pr) + verdict, reasons := metrics.DecideVerdict(delta, opts.Thresholds) + + // Step 5: Render the report into a buffer so a single write reaches Stdout. + // Buffering keeps the output atomic (no partial payload on a write error) and + // deterministic. + var buf bytes.Buffer + if opts.JSON { + if err := writeDiffJSON(&buf, delta, verdict, reasons); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("encode diff json: %w", err) + } + } else { + writeDiffTable(&buf, delta, verdict, reasons) + } + + // Final context check before writing — prevent partial output on cancellation. + if err := ctx.Err(); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("diff: %w", err) + } + + if _, err := opts.Stdout.Write(buf.Bytes()); err != nil { + return &DiffResult{ExitCode: 2}, fmt.Errorf("write diff output: %w", err) + } + + return &DiffResult{ + Delta: delta, + Verdict: verdict, + Reasons: reasons, + ExitCode: 0, + }, nil +} + +// writeDiffJSON renders the diff as a single indented JSON object to w. A nil +// reasons slice is normalized to an empty slice so the reasons key is always a +// JSON array, never null. +func writeDiffJSON(w io.Writer, delta metrics.GraphDelta, verdict metrics.Verdict, reasons []string) error { + if reasons == nil { + reasons = []string{} + } + payload := diffJSON{ + Verdict: verdict, + Reasons: reasons, + EntropyDirection: delta.Direction, + Unreliable: delta.Unreliable, + Modules: delta.Modules, + Added: delta.Added, + Removed: delta.Removed, + NewCycles: delta.NewCycles, + ResolvedCycles: delta.ResolvedCycles, + } + data, err := json.MarshalIndent(payload, "", " ") + if err != nil { + return fmt.Errorf("marshal diff payload: %w", err) + } + if _, err := fmt.Fprintln(w, string(data)); err != nil { + return fmt.Errorf("write diff payload: %w", err) + } + return nil +} + +// writeDiffTable renders the diff as a human-readable report to w. When the +// measurement is unreliable it leads with a partial-build annotation and omits +// the added/removed sections whose signal ComputeDelta has suppressed. It then +// prints the per-module delta table, the new and resolved cycle lists, the +// entropy direction, the verdict, and the verdict reasons. All collections are +// consumed in the stable, sorted order that ComputeDelta and DecideVerdict +// guarantee, so output is byte-stable across runs. Writes target an in-memory +// buffer that cannot fail, so intermediate write errors are intentionally +// discarded. +func writeDiffTable(w io.Writer, delta metrics.GraphDelta, verdict metrics.Verdict, reasons []string) { + if delta.Unreliable { + _, _ = fmt.Fprintln(w, "partial build — measurement unreliable") + _, _ = fmt.Fprintln(w) + } + + _, _ = fmt.Fprintln(w, "Per-module deltas (PR minus base):") + tw := tabwriter.NewWriter(w, 0, 4, 2, ' ', 0) + _, _ = fmt.Fprintln(tw, "PATH\tCa\tCe\tInstability\tAbstractness\tDistance\tLCOM") + if len(delta.Modules) == 0 { + _, _ = fmt.Fprintln(tw, "(no shared modules)") + } + for _, m := range delta.Modules { + _, _ = fmt.Fprintf(tw, "%s\t%d\t%d\t%.4f\t%.4f\t%.4f\t%d\n", + m.Path, m.Ca, m.Ce, + normFloat(m.Instability), normFloat(m.Abstractness), normFloat(m.Distance), + m.LCOM) + } + _ = tw.Flush() + + writeCycleSection(w, "New cycles:", delta.NewCycles) + writeCycleSection(w, "Resolved cycles:", delta.ResolvedCycles) + + if !delta.Unreliable { + writeListSection(w, "Added packages:", delta.Added) + writeListSection(w, "Removed packages:", delta.Removed) + } + + _, _ = fmt.Fprintf(w, "\nEntropy direction: %s\n", delta.Direction) + _, _ = fmt.Fprintf(w, "Verdict: %s\n", verdict) + if len(reasons) == 0 { + _, _ = fmt.Fprintln(w, "Reasons: (none)") + return + } + _, _ = fmt.Fprintln(w, "Reasons:") + for _, r := range reasons { + _, _ = fmt.Fprintf(w, " - %s\n", r) + } +} + +// writeCycleSection writes a titled list of cycles to w, one member-joined cycle +// per line, or "(none)" when the list is empty. +func writeCycleSection(w io.Writer, title string, cycles []metrics.Cycle) { + _, _ = fmt.Fprintln(w, title) + if len(cycles) == 0 { + _, _ = fmt.Fprintln(w, " (none)") + return + } + for _, c := range cycles { + _, _ = fmt.Fprintf(w, " - %s\n", strings.Join([]string(c), " ")) + } +} + +// writeListSection writes a titled list of strings to w, one item per line, or +// "(none)" when the list is empty. +func writeListSection(w io.Writer, title string, items []string) { + _, _ = fmt.Fprintln(w, title) + if len(items) == 0 { + _, _ = fmt.Fprintln(w, " (none)") + return + } + for _, it := range items { + _, _ = fmt.Fprintf(w, " - %s\n", it) + } +} + +// normFloat maps negative zero to positive zero so a delta of exactly zero +// always renders as "0.0000" rather than "-0.0000", keeping the table output +// stable. +func normFloat(f float64) float64 { + if f == 0 { + return 0 + } + return f +} + +// tightenThresholds applies tighten-only overrides to the protected default +// gates. Each override argument is nil when its flag was not set. An override is +// accepted only when it does not exceed the default: a smaller value tightens +// the gate and an equal value is a no-op, so neither weakens it. A looser +// (strictly greater) override returns an error naming the offending flag, +// enforcing the AGENTS.md gatekeeping mandate that protected quality gates may +// be tightened but never weakened. +func tightenThresholds(def metrics.VerdictThresholds, instability, distance *float64, lcom *int) (metrics.VerdictThresholds, error) { + out := def + if instability != nil { + if *instability > def.MaxInstabilityDelta { + return def, fmt.Errorf("--max-instability-delta %.4f is looser than the protected default %.4f: overrides may only tighten (lower) the gate", *instability, def.MaxInstabilityDelta) + } + out.MaxInstabilityDelta = *instability + } + if distance != nil { + if *distance > def.MaxDistanceDelta { + return def, fmt.Errorf("--max-distance-delta %.4f is looser than the protected default %.4f: overrides may only tighten (lower) the gate", *distance, def.MaxDistanceDelta) + } + out.MaxDistanceDelta = *distance + } + if lcom != nil { + if *lcom > def.MaxLCOMDelta { + return def, fmt.Errorf("--max-lcom-delta %d is looser than the protected default %d: overrides may only tighten (lower) the gate", *lcom, def.MaxLCOMDelta) + } + out.MaxLCOMDelta = *lcom + } + return out, nil +} + +// diffCmd creates the cobra command for the diff subcommand. It wires flag +// parsing, tighten-only threshold validation, and signal handling, then +// delegates to RunDiff per AP-002 (no business logic in the command layer). +func diffCmd() *cobra.Command { + var ( + jsonOut bool + maxInstabilityDelta float64 + maxDistanceDelta float64 + maxLCOMDelta int + ) + + defaults := metrics.DefaultVerdictThresholds() + + cmd := &cobra.Command{ + Use: "diff ", + Short: "Compare two ModuleGraph JSON files and report an entropy verdict", + Long: `Diff compares a base ModuleGraph against a PR ModuleGraph, computes the +per-package structural-quality delta (Ca, Ce, instability, abstractness, +distance from main sequence, LCOM), classifies new and resolved circular +dependencies, and renders a deterministic entropy verdict (APPROVE, COMMENT, or +REQUEST_CHANGES). + +Both inputs must be JSON documents conforming to the ModuleGraph schema, as +produced by 'vibe-check analyze'. Output is a human-readable table by default or +a JSON object with --json. + +The verdict is reported in the output payload, not the exit code: diff exits 0 +whenever both inputs are valid (even for a REQUEST_CHANGES verdict) and exits 2 +only when an input is missing, unreadable, or schema-invalid. + +The --max-*-delta override flags are TIGHTEN-ONLY: a value looser than the +protected default (instability 0.15, distance 0.20, LCOM 2) is rejected with +exit code 2.`, + Args: cobra.ExactArgs(2), + // SilenceUsage prevents cobra from printing usage on RunE errors; we + // report errors ourselves. + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + // Resolve tighten-only threshold overrides BEFORE reading any file or + // computing a verdict, converting only explicitly-set flags to + // pointers. A looser-than-default override is rejected with exit 2 and + // no stdout payload. + var ( + instOverride *float64 + distOverride *float64 + lcomOverride *int + ) + if cmd.Flags().Changed("max-instability-delta") { + instOverride = &maxInstabilityDelta + } + if cmd.Flags().Changed("max-distance-delta") { + distOverride = &maxDistanceDelta + } + if cmd.Flags().Changed("max-lcom-delta") { + lcomOverride = &maxLCOMDelta + } + + thresholds, err := tightenThresholds(defaults, instOverride, distOverride, lcomOverride) + if err != nil { + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "Error:", err) + return &exitCodeError{code: 2, err: err} + } + + // Signal handling: intercept SIGINT and SIGTERM. + ctx, stop := signal.NotifyContext(cmd.Context(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + opts := DiffOptions{ + Stdout: cmd.OutOrStdout(), + Stderr: cmd.ErrOrStderr(), + BasePath: args[0], + PRPath: args[1], + Thresholds: thresholds, + JSON: jsonOut, + } + + result, err := RunDiff(ctx, opts) + if err != nil { + // Print error to stderr; cobra will not print usage because + // SilenceUsage is true. + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "Error:", err) + code := 2 + if result != nil && result.ExitCode != 0 { + code = result.ExitCode + } + return &exitCodeError{code: code, err: err} + } + + if result.ExitCode != 0 { + // Defensive: RunDiff couples every non-zero exit code with a + // non-nil error handled above, so this path is not expected. + return &exitCodeError{ + code: result.ExitCode, + err: fmt.Errorf("diff failed with exit code %d", result.ExitCode), + } + } + + return nil + }, + } + + cmd.Flags().BoolVar(&jsonOut, "json", false, "Emit a machine-readable JSON payload instead of a table") + cmd.Flags().Float64Var(&maxInstabilityDelta, "max-instability-delta", defaults.MaxInstabilityDelta, "Tighten-only instability-increase gate (must be <= 0.15)") + cmd.Flags().Float64Var(&maxDistanceDelta, "max-distance-delta", defaults.MaxDistanceDelta, "Tighten-only distance-increase gate (must be <= 0.20)") + cmd.Flags().IntVar(&maxLCOMDelta, "max-lcom-delta", defaults.MaxLCOMDelta, "Tighten-only LCOM-increase gate (must be <= 2)") + + return cmd +} diff --git a/cmd/vibe-check/diff_test.go b/cmd/vibe-check/diff_test.go new file mode 100644 index 0000000..49d3bd3 --- /dev/null +++ b/cmd/vibe-check/diff_test.go @@ -0,0 +1,937 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "math" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/zero-dot-force/vibe-check/metrics" +) + +// --- Fixture helpers --------------------------------------------------------- + +// mkModule builds a ModuleResult with the given stored metric values. The raw +// exportedTypes/abstractTypes counts are fixed placeholders: these fixtures set +// the stored instability/abstractness/distance/lcom values directly to exercise +// specific deltas, and ComputeDelta operates on those stored values rather than +// recomputing the Martin formulas, so internal metric-formula consistency is not +// required here. +func mkModule(path string, ca, ce int, inst, abst, dist float64, lcom int) metrics.ModuleResult { + return metrics.ModuleResult{ + Module: metrics.Module{ + Path: path, + Name: path, + Ca: ca, + Ce: ce, + ExportedTypes: 2, + AbstractTypes: 1, + }, + Instability: metrics.Instability(inst), + Abstractness: metrics.Abstractness(abst), + Distance: metrics.Distance(dist), + LCOM: metrics.LCOM(lcom), + Zone: metrics.ZoneNormal, + } +} + +// mkGraph builds a schema-valid ModuleGraph. Nil slices are normalized to empty +// (non-nil) slices so the marshaled document carries JSON arrays (never null), +// which metrics.Validate requires. +func mkGraph(status metrics.Status, mods []metrics.ModuleResult, cycles []metrics.Cycle, warnings []metrics.Warning) metrics.ModuleGraph { + if mods == nil { + mods = []metrics.ModuleResult{} + } + if cycles == nil { + cycles = []metrics.Cycle{} + } + if warnings == nil { + warnings = []metrics.Warning{} + } + return metrics.ModuleGraph{ + SchemaVersion: metrics.SchemaVersionCurrent, + Language: "go", + Modules: mods, + Cycles: cycles, + Warnings: warnings, + Status: status, + } +} + +// improvementFixtures returns a base/PR pair where ex/a improves (ΔCe -1, +// ΔInstability -0.20, ΔDistance -0.20, ΔLCOM -2) and a base cycle is resolved. +// Expected verdict APPROVE, direction improving. +func improvementFixtures() (base, pr metrics.ModuleGraph) { + base = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{ + mkModule("ex/a", 1, 2, 0.50, 0.10, 0.40, 3), + mkModule("ex/b", 2, 0, 0.00, 0.20, 0.80, 1), + }, + []metrics.Cycle{{"ex/b", "ex/a"}}, // deliberately unsorted; ComputeDelta normalizes + nil) + pr = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{ + mkModule("ex/a", 1, 1, 0.30, 0.10, 0.20, 1), + mkModule("ex/b", 2, 0, 0.00, 0.20, 0.80, 1), + }, + nil, nil) + return base, pr +} + +// degradeCycleFixtures returns a base/PR pair with identical module metrics but +// a cycle newly introduced in the PR. Expected verdict REQUEST_CHANGES, +// direction degrading, with a new-cycle reason. +func degradeCycleFixtures() (base, pr metrics.ModuleGraph) { + mods := []metrics.ModuleResult{ + mkModule("ex/a", 1, 1, 0.50, 0.10, 0.40, 1), + mkModule("ex/b", 2, 0, 0.00, 0.20, 0.80, 1), + } + base = mkGraph(metrics.StatusComplete, mods, nil, nil) + pr = mkGraph(metrics.StatusComplete, mods, []metrics.Cycle{{"ex/a", "ex/b"}}, nil) + return base, pr +} + +// commentBandFixtures returns a base/PR pair where ex/a instability rises by +// 0.05 — a material shift below every REQUEST_CHANGES gate. Expected verdict +// COMMENT, direction stable. +func commentBandFixtures() (base, pr metrics.ModuleGraph) { + base = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{mkModule("ex/a", 1, 1, 0.50, 0.10, 0.30, 1)}, nil, nil) + pr = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{mkModule("ex/a", 1, 1, 0.55, 0.10, 0.30, 1)}, nil, nil) + return base, pr +} + +// partialBuildFixtures returns a base/PR pair where the PR is a partial build +// (Status partial plus a load-error warning) and differs structurally (ex/b +// removed, ex/c added). Expected verdict COMMENT, unreliable flag true, with the +// added/removed signal suppressed. +func partialBuildFixtures() (base, pr metrics.ModuleGraph) { + base = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{ + mkModule("ex/a", 1, 1, 0.50, 0.10, 0.40, 1), + mkModule("ex/b", 2, 0, 0.00, 0.20, 0.80, 1), + }, nil, nil) + pr = mkGraph(metrics.StatusPartial, + []metrics.ModuleResult{ + mkModule("ex/a", 1, 1, 0.50, 0.10, 0.40, 1), + mkModule("ex/c", 0, 1, 1.00, 0.00, 0.00, 1), + }, + nil, + []metrics.Warning{{Code: "load-error", Message: "failed to load ex/c"}}) + return base, pr +} + +// structuralOnlyFixtures returns a base/PR pair with no shared modules (ex/a +// removed, ex/b added), no metric regression, and no cycle change. Expected +// verdict APPROVE, direction stable, with non-empty added/removed (reliable +// measurement). +func structuralOnlyFixtures() (base, pr metrics.ModuleGraph) { + base = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{mkModule("ex/a", 1, 1, 0.50, 0.10, 0.40, 1)}, nil, nil) + pr = mkGraph(metrics.StatusComplete, + []metrics.ModuleResult{mkModule("ex/b", 1, 1, 0.50, 0.10, 0.40, 1)}, nil, nil) + return base, pr +} + +// writeGraphFile marshals g to indented JSON, asserts it is schema-valid via +// metrics.Validate, writes it into dir under name, and returns the path. +func writeGraphFile(t *testing.T, dir, name string, g metrics.ModuleGraph) string { + t.Helper() + data, err := json.MarshalIndent(g, "", " ") + if err != nil { + t.Fatalf("marshal fixture %s: %v", name, err) + } + if err := metrics.Validate(data); err != nil { + t.Fatalf("fixture %s is not schema-valid: %v", name, err) + } + path := filepath.Join(dir, name) + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write fixture %s: %v", name, err) + } + return path +} + +// writeGraphPair writes a base/PR fixture pair into a fresh temp dir and returns +// their paths. +func writeGraphPair(t *testing.T, base, pr metrics.ModuleGraph) (basePath, prPath string) { + t.Helper() + dir := t.TempDir() + return writeGraphFile(t, dir, "base.json", base), writeGraphFile(t, dir, "pr.json", pr) +} + +// decodedDiff mirrors the --json diff payload for test assertions. +type decodedDiff struct { + Verdict string `json:"verdict"` + Reasons []string `json:"reasons"` + EntropyDirection string `json:"entropyDirection"` + Unreliable bool `json:"unreliable"` + Modules []metrics.Delta `json:"modules"` + Added []string `json:"added"` + Removed []string `json:"removed"` + NewCycles []metrics.Cycle `json:"newCycles"` + ResolvedCycles []metrics.Cycle `json:"resolvedCycles"` +} + +// approxEqual reports whether two float64 deltas are equal within 1e-9, +// tolerating IEEE-754 subtraction noise in fixture deltas. +func approxEqual(got, want float64) bool { + return math.Abs(got-want) <= 1e-9 +} + +// runDiffJSONViaCommand executes the diff subcommand through rootCmd and decodes +// the emitted JSON payload. +func runDiffJSONViaCommand(t *testing.T, args []string) decodedDiff { + t.Helper() + cmd := rootCmd() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs(args) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute(%v) error: %v\nstderr: %s", args, err, errOut.String()) + } + var decoded decodedDiff + if err := json.Unmarshal(out.Bytes(), &decoded); err != nil { + t.Fatalf("unmarshal diff json: %v\npayload: %s", err, out.String()) + } + return decoded +} + +// --- Task 3.5: JSON payload scenarios --------------------------------------- + +func TestRunDiff_JSONScenarios(t *testing.T) { + t.Parallel() + + type want struct { + verdict string + direction string + unreliable bool + newCycles int + resolvedCycles int + reasonSubstr string // empty means expect zero reasons + } + tests := []struct { + name string + build func() (metrics.ModuleGraph, metrics.ModuleGraph) + want want + }{ + { + name: "improvement_approve", + build: improvementFixtures, + want: want{ + verdict: "APPROVE", direction: "improving", + newCycles: 0, resolvedCycles: 1, reasonSubstr: "", + }, + }, + { + name: "degradation_request_changes", + build: degradeCycleFixtures, + want: want{ + verdict: "REQUEST_CHANGES", direction: "degrading", + newCycles: 1, resolvedCycles: 0, reasonSubstr: "new-cycle", + }, + }, + { + name: "comment_band_stable", + build: commentBandFixtures, + want: want{ + verdict: "COMMENT", direction: "stable", + newCycles: 0, resolvedCycles: 0, reasonSubstr: "materiality", + }, + }, + { + name: "partial_build_unreliable", + build: partialBuildFixtures, + want: want{ + verdict: "COMMENT", direction: "stable", unreliable: true, + newCycles: 0, resolvedCycles: 0, reasonSubstr: "partial-build", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + base, pr := tt.build() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, + Stderr: &stderr, + BasePath: basePath, + PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), + JSON: true, + }) + if err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + if result.ExitCode != 0 { + t.Errorf("ExitCode: got %d, want 0", result.ExitCode) + } + + var out decodedDiff + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("unmarshal diff json: %v\npayload: %s", err, stdout.String()) + } + + if out.Verdict != tt.want.verdict { + t.Errorf("verdict: got %v, want %v", out.Verdict, tt.want.verdict) + } + if string(result.Verdict) != tt.want.verdict { + t.Errorf("result.Verdict: got %v, want %v", result.Verdict, tt.want.verdict) + } + if out.EntropyDirection != tt.want.direction { + t.Errorf("entropyDirection: got %v, want %v", out.EntropyDirection, tt.want.direction) + } + if out.Unreliable != tt.want.unreliable { + t.Errorf("unreliable: got %v, want %v", out.Unreliable, tt.want.unreliable) + } + if len(out.NewCycles) != tt.want.newCycles { + t.Errorf("newCycles count: got %v, want %v", len(out.NewCycles), tt.want.newCycles) + } + if len(out.ResolvedCycles) != tt.want.resolvedCycles { + t.Errorf("resolvedCycles count: got %v, want %v", len(out.ResolvedCycles), tt.want.resolvedCycles) + } + + if tt.want.reasonSubstr == "" { + if len(out.Reasons) != 0 { + t.Errorf("reasons: got %v, want none", out.Reasons) + } + } else { + found := false + for _, r := range out.Reasons { + if strings.Contains(r, tt.want.reasonSubstr) { + found = true + break + } + } + if !found { + t.Errorf("reasons %v do not contain %q", out.Reasons, tt.want.reasonSubstr) + } + } + + // Per-module rows MUST be sorted ascending by Path. + for i := 1; i < len(out.Modules); i++ { + if out.Modules[i-1].Path > out.Modules[i].Path { + t.Errorf("modules not sorted by path: %q before %q", out.Modules[i-1].Path, out.Modules[i].Path) + } + } + }) + } +} + +func TestRunDiff_JSONImprovementDeltaValues(t *testing.T) { + t.Parallel() + base, pr := improvementFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + if _, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }); err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + + var out decodedDiff + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("unmarshal diff json: %v", err) + } + + if len(out.Modules) != 2 { + t.Fatalf("modules count: got %d, want 2", len(out.Modules)) + } + a := out.Modules[0] + if a.Path != "ex/a" { + t.Errorf("modules[0].path: got %q, want %q", a.Path, "ex/a") + } + if a.Ca != 0 { + t.Errorf("ex/a ca delta: got %d, want 0", a.Ca) + } + if a.Ce != -1 { + t.Errorf("ex/a ce delta: got %d, want -1", a.Ce) + } + if !approxEqual(a.Instability, -0.20) { + t.Errorf("ex/a instability delta: got %v, want -0.20", a.Instability) + } + if !approxEqual(a.Distance, -0.20) { + t.Errorf("ex/a distance delta: got %v, want -0.20", a.Distance) + } + if a.LCOM != -2 { + t.Errorf("ex/a lcom delta: got %d, want -2", a.LCOM) + } + + if len(out.ResolvedCycles) != 1 { + t.Fatalf("resolvedCycles count: got %d, want 1", len(out.ResolvedCycles)) + } + if got := strings.Join([]string(out.ResolvedCycles[0]), " "); got != "ex/a ex/b" { + t.Errorf("resolved cycle members: got %q, want %q", got, "ex/a ex/b") + } + if len(out.Reasons) != 0 { + t.Errorf("reasons: got %v, want none for APPROVE", out.Reasons) + } +} + +func TestRunDiff_JSONDegradationReasons(t *testing.T) { + t.Parallel() + base, pr := degradeCycleFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }) + if err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + if result.Verdict != metrics.VerdictRequestChanges { + t.Errorf("verdict: got %v, want %v", result.Verdict, metrics.VerdictRequestChanges) + } + + var out decodedDiff + if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { + t.Fatalf("unmarshal diff json: %v", err) + } + if len(out.NewCycles) != 1 { + t.Fatalf("newCycles count: got %d, want 1", len(out.NewCycles)) + } + if got := strings.Join([]string(out.NewCycles[0]), " "); got != "ex/a ex/b" { + t.Errorf("new cycle members: got %q, want %q", got, "ex/a ex/b") + } + found := false + for _, r := range out.Reasons { + if strings.Contains(r, "new-cycle: ex/a ex/b") { + found = true + break + } + } + if !found { + t.Errorf("reasons %v do not contain the new-cycle reason", out.Reasons) + } +} + +func TestRunDiff_PartialBuildSuppressesAddedRemoved(t *testing.T) { + t.Parallel() + base, pr := partialBuildFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }) + if err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + if result.Verdict != metrics.VerdictComment { + t.Errorf("verdict: got %v, want %v", result.Verdict, metrics.VerdictComment) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(stdout.Bytes(), &raw); err != nil { + t.Fatalf("unmarshal diff json: %v", err) + } + + unreliableRaw, ok := raw["unreliable"] + if !ok { + t.Fatal("payload missing unreliable key") + } + var unreliable bool + if err := json.Unmarshal(unreliableRaw, &unreliable); err != nil { + t.Fatalf("unmarshal unreliable: %v", err) + } + if !unreliable { + t.Error("unreliable: got false, want true") + } + if _, ok := raw["added"]; ok { + t.Error("payload must not include added when unreliable") + } + if _, ok := raw["removed"]; ok { + t.Error("payload must not include removed when unreliable") + } +} + +func TestRunDiff_PartialBuildTableAnnotation(t *testing.T) { + t.Parallel() + base, pr := partialBuildFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + if _, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: false, + }); err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + out := stdout.String() + if !strings.Contains(out, "partial build") || !strings.Contains(out, "measurement unreliable") { + t.Errorf("table missing partial-build annotation:\n%s", out) + } + if strings.Contains(out, "Added packages:") { + t.Errorf("table must not include Added section when unreliable:\n%s", out) + } + if strings.Contains(out, "Removed packages:") { + t.Errorf("table must not include Removed section when unreliable:\n%s", out) + } + if !strings.Contains(out, "Verdict: COMMENT") { + t.Errorf("table missing COMMENT verdict:\n%s", out) + } +} + +func TestRunDiff_StructuralAddedRemovedReported(t *testing.T) { + t.Parallel() + base, pr := structuralOnlyFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + // JSON: added/removed present and correct; verdict APPROVE; empty module set. + var jsonOut, stderr bytes.Buffer + if _, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &jsonOut, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }); err != nil { + t.Fatalf("RunDiff(json) returned error: %v", err) + } + var out decodedDiff + if err := json.Unmarshal(jsonOut.Bytes(), &out); err != nil { + t.Fatalf("unmarshal diff json: %v", err) + } + if out.Verdict != "APPROVE" { + t.Errorf("verdict: got %v, want APPROVE", out.Verdict) + } + if len(out.Modules) != 0 { + t.Errorf("modules: got %d, want 0 (no shared modules)", len(out.Modules)) + } + if len(out.Added) != 1 || out.Added[0] != "ex/b" { + t.Errorf("added: got %v, want [ex/b]", out.Added) + } + if len(out.Removed) != 1 || out.Removed[0] != "ex/a" { + t.Errorf("removed: got %v, want [ex/a]", out.Removed) + } + + // Table: shows the "(no shared modules)" row and both list sections. + var tableOut, stderr2 bytes.Buffer + if _, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &tableOut, Stderr: &stderr2, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: false, + }); err != nil { + t.Fatalf("RunDiff(table) returned error: %v", err) + } + table := tableOut.String() + for _, want := range []string{"(no shared modules)", "Added packages:", "ex/b", "Removed packages:", "ex/a"} { + if !strings.Contains(table, want) { + t.Errorf("table missing %q:\n%s", want, table) + } + } +} + +// --- Task 3.5: exit codes ---------------------------------------------------- + +func TestRunDiff_ExitTwoOnBadInput(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T) (basePath, prPath string) + wantSub string + }{ + { + name: "missing_base", + setup: func(t *testing.T) (string, string) { + base, pr := improvementFixtures() + _, prPath := writeGraphPair(t, base, pr) + return filepath.Join(t.TempDir(), "missing-base.json"), prPath + }, + wantSub: "read base file", + }, + { + name: "missing_pr", + setup: func(t *testing.T) (string, string) { + base, pr := improvementFixtures() + basePath, _ := writeGraphPair(t, base, pr) + return basePath, filepath.Join(t.TempDir(), "missing-pr.json") + }, + wantSub: "read pr file", + }, + { + name: "invalid_base", + setup: func(t *testing.T) (string, string) { + dir := t.TempDir() + _, pr := improvementFixtures() + prPath := writeGraphFile(t, dir, "pr.json", pr) + badPath := filepath.Join(dir, "bad-base.json") + // Missing required fields (modules/cycles/warnings/status). + if err := os.WriteFile(badPath, []byte(`{"schemaVersion":"1.1","language":"go"}`), 0o644); err != nil { + t.Fatalf("write bad base: %v", err) + } + return badPath, prPath + }, + wantSub: "validate base file", + }, + { + name: "invalid_pr", + setup: func(t *testing.T) (string, string) { + dir := t.TempDir() + base, _ := improvementFixtures() + basePath := writeGraphFile(t, dir, "base.json", base) + badPath := filepath.Join(dir, "bad-pr.json") + if err := os.WriteFile(badPath, []byte(`not valid json`), 0o644); err != nil { + t.Fatalf("write bad pr: %v", err) + } + return basePath, badPath + }, + wantSub: "validate pr file", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + basePath, prPath := tt.setup(t) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }) + if err == nil { + t.Fatal("expected error, got nil") + } + if result.ExitCode != 2 { + t.Errorf("ExitCode: got %d, want 2", result.ExitCode) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on error (no payload), got %d bytes: %s", stdout.Len(), stdout.String()) + } + if !strings.Contains(err.Error(), tt.wantSub) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantSub) + } + }) + } +} + +// TestRunDiff_ParseErrorAfterValidation covers the defensive unmarshal branch: a +// document that passes schema validation (ca is a non-negative number) but cannot +// be unmarshaled into the typed graph (ca is not an integer). +func TestRunDiff_ParseErrorAfterValidation(t *testing.T) { + t.Parallel() + dir := t.TempDir() + badModule := `{"schemaVersion":"1.1","language":"go","modules":[{"path":"ex/a","name":"a","ca":1.5,"ce":0,"instability":0.5,"abstractness":0.1,"distance":0.4,"lcom":1,"exportedTypes":2,"abstractTypes":1,"zone":"normal"}],"cycles":[],"warnings":[],"status":"complete"}` + if err := metrics.Validate([]byte(badModule)); err != nil { + t.Fatalf("precondition: base must pass Validate, got: %v", err) + } + basePath := filepath.Join(dir, "base.json") + if err := os.WriteFile(basePath, []byte(badModule), 0o644); err != nil { + t.Fatalf("write base: %v", err) + } + _, pr := improvementFixtures() + prPath := writeGraphFile(t, dir, "pr.json", pr) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }) + if err == nil { + t.Fatal("expected parse error, got nil") + } + if result.ExitCode != 2 { + t.Errorf("ExitCode: got %d, want 2", result.ExitCode) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on parse error, got: %s", stdout.String()) + } + if !strings.Contains(err.Error(), "parse base file") { + t.Errorf("error %q does not contain 'parse base file'", err.Error()) + } +} + +func TestRunDiff_ContextCancelled(t *testing.T) { + t.Parallel() + base, pr := improvementFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + var stdout, stderr bytes.Buffer + result, err := RunDiff(ctx, DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: true, + }) + if err == nil { + t.Fatal("expected error for cancelled context, got nil") + } + if result.ExitCode != 2 { + t.Errorf("ExitCode: got %d, want 2", result.ExitCode) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on cancellation, got %d bytes", stdout.Len()) + } +} + +// --- Task 3.5: default human table ------------------------------------------- + +func TestRunDiff_HumanTableDefault(t *testing.T) { + t.Parallel() + base, pr := improvementFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + var stdout, stderr bytes.Buffer + result, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: false, + }) + if err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + if result.ExitCode != 0 { + t.Errorf("ExitCode: got %d, want 0", result.ExitCode) + } + out := stdout.String() + if strings.HasPrefix(strings.TrimSpace(out), "{") { + t.Errorf("default output looks like JSON, want a table:\n%s", out) + } + for _, want := range []string{ + "Per-module deltas", "Instability", "Distance", "LCOM", + "ex/a", "Verdict: APPROVE", "Entropy direction: improving", + } { + if !strings.Contains(out, want) { + t.Errorf("table missing %q:\n%s", want, out) + } + } +} + +// --- Task 3.5: determinism --------------------------------------------------- + +func TestRunDiff_Deterministic(t *testing.T) { + t.Parallel() + + for _, jsonMode := range []bool{true, false} { + name := "table" + if jsonMode { + name = "json" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + base, pr := degradeCycleFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + run := func() string { + var stdout, stderr bytes.Buffer + if _, err := RunDiff(context.Background(), DiffOptions{ + Stdout: &stdout, Stderr: &stderr, + BasePath: basePath, PRPath: prPath, + Thresholds: metrics.DefaultVerdictThresholds(), JSON: jsonMode, + }); err != nil { + t.Fatalf("RunDiff returned error: %v", err) + } + return stdout.String() + } + first := run() + second := run() + if first != second { + t.Errorf("output not deterministic:\nfirst:\n%s\nsecond:\n%s", first, second) + } + }) + } +} + +// --- Task 3.4: tighten-only override enforcement ----------------------------- + +func TestTightenThresholds(t *testing.T) { + t.Parallel() + def := metrics.DefaultVerdictThresholds() + + tests := []struct { + name string + instability *float64 + distance *float64 + lcom *int + wantErr bool + want metrics.VerdictThresholds + }{ + {name: "no_overrides", want: def}, + {name: "tighter_instability", instability: float64Ptr(0.05), want: metrics.VerdictThresholds{MaxInstabilityDelta: 0.05, MaxDistanceDelta: 0.20, MaxLCOMDelta: 2}}, + {name: "equal_instability_noop", instability: float64Ptr(0.15), want: def}, + {name: "looser_instability", instability: float64Ptr(0.16), wantErr: true}, + {name: "tighter_distance", distance: float64Ptr(0.10), want: metrics.VerdictThresholds{MaxInstabilityDelta: 0.15, MaxDistanceDelta: 0.10, MaxLCOMDelta: 2}}, + {name: "equal_distance_noop", distance: float64Ptr(0.20), want: def}, + {name: "looser_distance", distance: float64Ptr(0.21), wantErr: true}, + {name: "tighter_lcom", lcom: intPtr(1), want: metrics.VerdictThresholds{MaxInstabilityDelta: 0.15, MaxDistanceDelta: 0.20, MaxLCOMDelta: 1}}, + {name: "equal_lcom_noop", lcom: intPtr(2), want: def}, + {name: "looser_lcom", lcom: intPtr(3), wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := tightenThresholds(def, tt.instability, tt.distance, tt.lcom) + if tt.wantErr { + if err == nil { + t.Fatal("expected error for looser override, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("thresholds: got %+v, want %+v", got, tt.want) + } + }) + } +} + +func TestDiffCommand_TightenOnlyRejectsLooser(t *testing.T) { + t.Parallel() + base, pr := improvementFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + tests := []struct { + name string + flag string + val string + }{ + {"instability", "--max-instability-delta", "0.30"}, + {"distance", "--max-distance-delta", "0.50"}, + {"lcom", "--max-lcom-delta", "5"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cmd := rootCmd() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"diff", tt.flag, tt.val, basePath, prPath}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected error for looser override, got nil") + } + var ece *exitCodeError + if !errors.As(err, &ece) { + t.Fatalf("error is not *exitCodeError: %T (%v)", err, err) + } + if ece.code != 2 { + t.Errorf("exit code: got %d, want 2", ece.code) + } + // No verdict payload may be emitted when a looser override is rejected; + // because the inputs are valid, an empty stdout proves the tighten check + // ran BEFORE any file read or verdict computation. + if out.Len() != 0 { + t.Errorf("stdout must be empty (no payload), got %d bytes: %s", out.Len(), out.String()) + } + if !strings.Contains(errOut.String(), "looser than the protected default") { + t.Errorf("stderr missing tighten diagnostic: %s", errOut.String()) + } + }) + } +} + +func TestDiffCommand_TightenAppliesStricter(t *testing.T) { + t.Parallel() + base, pr := commentBandFixtures() + basePath, prPath := writeGraphPair(t, base, pr) + + // Baseline: default thresholds yield COMMENT for the 0.05 instability shift. + baseline := runDiffJSONViaCommand(t, []string{"diff", "--json", basePath, prPath}) + if baseline.Verdict != "COMMENT" { + t.Fatalf("baseline verdict: got %v, want COMMENT", baseline.Verdict) + } + + // Tightened: --max-instability-delta 0.05 (< 0.15) makes the 0.05 shift fire + // the inclusive gate, flipping the verdict to REQUEST_CHANGES. + tightened := runDiffJSONViaCommand(t, []string{"diff", "--json", "--max-instability-delta", "0.05", basePath, prPath}) + if tightened.Verdict != "REQUEST_CHANGES" { + t.Errorf("tightened verdict: got %v, want REQUEST_CHANGES", tightened.Verdict) + } +} + +// --- Task 3.3/3.4: command-level exit codes ---------------------------------- + +func TestDiffCommand_ExitCodes(t *testing.T) { + t.Parallel() + base, pr := degradeCycleFixtures() // valid inputs; verdict REQUEST_CHANGES + basePath, prPath := writeGraphPair(t, base, pr) + + t.Run("valid_inputs_exit_zero_even_for_request_changes", func(t *testing.T) { + t.Parallel() + cmd := rootCmd() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"diff", "--json", basePath, prPath}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: unexpected error: %v\nstderr: %s", err, errOut.String()) + } + var decoded decodedDiff + if err := json.Unmarshal(out.Bytes(), &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if decoded.Verdict != "REQUEST_CHANGES" { + t.Errorf("verdict: got %v, want REQUEST_CHANGES (exit still 0)", decoded.Verdict) + } + }) + + t.Run("missing_file_exit_two", func(t *testing.T) { + t.Parallel() + cmd := rootCmd() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"diff", filepath.Join(t.TempDir(), "nope.json"), prPath}) + err := cmd.Execute() + if err == nil { + t.Fatal("expected error, got nil") + } + var ece *exitCodeError + if !errors.As(err, &ece) { + t.Fatalf("error is not *exitCodeError: %T", err) + } + if ece.code != 2 { + t.Errorf("exit code: got %d, want 2", ece.code) + } + if out.Len() != 0 { + t.Errorf("stdout must be empty on tool failure, got: %s", out.String()) + } + }) +} + +func TestDiffCommand_Help(t *testing.T) { + t.Parallel() + cmd := rootCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs([]string{"diff", "--help"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + output := out.String() + for _, want := range []string{ + "diff", "--json", "--max-instability-delta", "--max-distance-delta", "--max-lcom-delta", + } { + if !strings.Contains(output, want) { + t.Errorf("help output missing %q", want) + } + } +} diff --git a/cmd/vibe-check/init.go b/cmd/vibe-check/init.go new file mode 100644 index 0000000..852f977 --- /dev/null +++ b/cmd/vibe-check/init.go @@ -0,0 +1,244 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "io/fs" + "os/signal" + "path/filepath" + "syscall" + + "github.com/spf13/cobra" + + "github.com/zero-dot-force/vibe-check/internal/scaffold" +) + +// InitOptions contains the configuration for the init command. +// It follows the AP-001 Options struct pattern for testable CLI commands. +type InitOptions struct { + // Stdout is the writer for the deployment summary (table or JSON). Required. + Stdout io.Writer + // Stderr is the writer for diagnostics and errors. Required. + Stderr io.Writer + // Path is the target project directory into which agent assets are deployed. + // Empty defaults to the current directory ("."). + Path string + // Force overwrites existing agent asset files instead of skipping them. + Force bool + // JSON selects machine-readable JSON output when true; otherwise a + // human-readable summary is written. + JSON bool + + // writeFile is an optional filesystem seam forwarded to scaffold.Run so the + // I/O-failure exit path can be exercised deterministically in tests. When + // nil, scaffold.Run defaults to os.WriteFile. It is unexported because it is + // a test seam, not part of the public command contract. + writeFile func(path string, data []byte, perm fs.FileMode) error +} + +// InitResult contains the outcome of an init deployment. +// It follows the AP-001 Result struct pattern. +type InitResult struct { + // Written lists the asset filenames newly created. + Written []string + // Skipped lists the asset filenames left untouched because they already + // existed and Force was not set. + Skipped []string + // Forced lists the asset filenames overwritten because Force was set. + Forced []string + // ExitCode is the process exit code: 0 on success (including an all-skipped + // run), 2 on an invalid target path or an I/O failure. + ExitCode int +} + +// initJSON is the machine-readable init payload emitted under --json. Field +// declaration order determines JSON key order and every key uses lowercase. All +// three slices are normalized to empty (never null) so each key is always a JSON +// array. +type initJSON struct { + Written []string `json:"written"` + Skipped []string `json:"skipped"` + Forced []string `json:"forced"` +} + +// RunInit deploys the embedded Review Council agent assets into the target +// project's .opencode/agents/ directory and writes a summary to opts.Stdout. It +// is the testable entry point per AP-002/AP-003: all business logic lives here, +// not in the cobra command layer. +// +// Exit code semantics (also mirrored in the returned InitResult.ExitCode): +// - 0: assets were deployed (or all skipped) and the summary was written. +// - 2: the target path is invalid (missing, not a directory, or traversal) or +// an I/O failure occurred while writing an asset. In that case nothing is +// written to opts.Stdout and the returned error describes the failure. +// +// RunInit never writes a partial summary to opts.Stdout on any error path. +func RunInit(ctx context.Context, opts InitOptions) (*InitResult, error) { + // Default the target path to the current directory. + path := opts.Path + if path == "" { + path = "." + } + + // Honor cancellation before doing any filesystem work. + if err := ctx.Err(); err != nil { + return &InitResult{ExitCode: 2}, fmt.Errorf("init: %w", err) + } + + // Deploy the embedded assets. scaffold.Run validates the target path and + // performs the writes; any error (invalid path or I/O failure) is a tool + // failure (exit 2) with no stdout output. + res, err := scaffold.Run(scaffold.Options{ + TargetDir: path, + Force: opts.Force, + WriteFile: opts.writeFile, + }) + if err != nil { + return &InitResult{ExitCode: 2}, fmt.Errorf("init: %w", err) + } + + // Honor cancellation after the writes but before emitting output. + if err := ctx.Err(); err != nil { + return &InitResult{ExitCode: 2}, fmt.Errorf("init: %w", err) + } + + // Render the summary into a buffer so a single write reaches Stdout, keeping + // output atomic (no partial payload on a write error) and deterministic. + var buf bytes.Buffer + if opts.JSON { + if err := writeInitJSON(&buf, res); err != nil { + return &InitResult{ExitCode: 2}, fmt.Errorf("encode init json: %w", err) + } + } else { + writeInitSummary(&buf, path, res) + } + + if _, err := opts.Stdout.Write(buf.Bytes()); err != nil { + return &InitResult{ExitCode: 2}, fmt.Errorf("write init output: %w", err) + } + + return &InitResult{ + Written: res.Written, + Skipped: res.Skipped, + Forced: res.Forced, + ExitCode: 0, + }, nil +} + +// writeInitJSON renders the deployment result as a single indented JSON object +// to w. Each slice is normalized so its key is always a JSON array, never null. +func writeInitJSON(w io.Writer, res *scaffold.Result) error { + payload := initJSON{ + Written: normStringSlice(res.Written), + Skipped: normStringSlice(res.Skipped), + Forced: normStringSlice(res.Forced), + } + data, err := json.MarshalIndent(payload, "", " ") + if err != nil { + return fmt.Errorf("marshal init payload: %w", err) + } + if _, err := fmt.Fprintln(w, string(data)); err != nil { + return fmt.Errorf("write init payload: %w", err) + } + return nil +} + +// writeInitSummary renders the deployment result as a human-readable summary to +// w. It names the target agents directory, then lists the written, skipped, and +// forced assets. The lists are consumed in the stable, sorted order scaffold.Run +// guarantees, so output is byte-stable across runs. +func writeInitSummary(w io.Writer, targetDir string, res *scaffold.Result) { + agentsDir := filepath.Join(targetDir, ".opencode", "agents") + _, _ = fmt.Fprintf(w, "Deployed agent assets under %s:\n", agentsDir) + writeListSection(w, "Written:", res.Written) + writeListSection(w, "Skipped:", res.Skipped) + writeListSection(w, "Forced:", res.Forced) +} + +// normStringSlice returns s unchanged, or an empty (non-nil) slice when s is +// nil, so JSON marshaling always yields an array rather than null. +func normStringSlice(s []string) []string { + if s == nil { + return []string{} + } + return s +} + +// initCmd creates the cobra command for the init subcommand. It wires flag +// parsing and signal handling, then delegates to RunInit per AP-002 (no +// business logic in the command layer). +func initCmd() *cobra.Command { + var ( + force bool + jsonOut bool + ) + + cmd := &cobra.Command{ + Use: "init [path]", + Short: "Deploy vibe-check Review Council agent assets into a project", + Long: `Init deploys the embedded vibe-check Review Council agent assets (such as the +divisor-entropy structural-entropy reviewer) into a target project's +.opencode/agents/ directory. + +Existing files are skipped by default; use --force to overwrite them. The target +path defaults to the current directory. Output is a human-readable summary by +default or a JSON object with --json. + +Exit code is 0 on success (including an all-skipped run) and 2 when the target +path is invalid or an asset cannot be written.`, + Args: cobra.MaximumNArgs(1), + // SilenceUsage prevents cobra from printing usage on RunE errors; we + // report errors ourselves. + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + // Determine target path: argument or current directory. + path := "." + if len(args) > 0 { + path = args[0] + } + + // Signal handling: intercept SIGINT and SIGTERM. + ctx, stop := signal.NotifyContext(cmd.Context(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + opts := InitOptions{ + Stdout: cmd.OutOrStdout(), + Stderr: cmd.ErrOrStderr(), + Path: path, + Force: force, + JSON: jsonOut, + } + + result, err := RunInit(ctx, opts) + if err != nil { + // Print error to stderr; cobra will not print usage because + // SilenceUsage is true. + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), "Error:", err) + code := 2 + if result != nil && result.ExitCode != 0 { + code = result.ExitCode + } + return &exitCodeError{code: code, err: err} + } + + if result.ExitCode != 0 { + // Defensive: RunInit couples every non-zero exit code with a + // non-nil error handled above, so this path is not expected. + return &exitCodeError{ + code: result.ExitCode, + err: fmt.Errorf("init failed with exit code %d", result.ExitCode), + } + } + + return nil + }, + } + + cmd.Flags().BoolVar(&force, "force", false, "Overwrite existing agent asset files instead of skipping them") + cmd.Flags().BoolVar(&jsonOut, "json", false, "Emit a machine-readable JSON payload instead of a summary") + + return cmd +} diff --git a/cmd/vibe-check/init_test.go b/cmd/vibe-check/init_test.go new file mode 100644 index 0000000..7d6abd3 --- /dev/null +++ b/cmd/vibe-check/init_test.go @@ -0,0 +1,375 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +// initAssetName is the single embedded Review Council asset that init deploys. +// The scaffold writer reports assets by basename, so this is the value that +// appears in InitResult slices and in the --json payload. +const initAssetName = "divisor-entropy.md" + +// deployedInitAssetPath returns the on-disk location of the deployed asset for a +// given project root. +func deployedInitAssetPath(root string) string { + return filepath.Join(root, ".opencode", "agents", initAssetName) +} + +// initFailingWriter is an io.Writer whose Write always fails, used to exercise +// the stdout-write error path of RunInit. +type initFailingWriter struct{ err error } + +func (w initFailingWriter) Write([]byte) (int, error) { return 0, w.err } + +// TestRunInit_HumanSummaryLifecycle covers task 8.1: the human-readable summary +// lists written files on the first run, reports skips on a second run, and +// reports forced overwrites under Force. +func TestRunInit_HumanSummaryLifecycle(t *testing.T) { + t.Parallel() + dir := t.TempDir() + var stdout, stderr bytes.Buffer + + // First run: the asset is written. + res, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: dir}) + if err != nil { + t.Fatalf("first RunInit returned error: %v", err) + } + if res.ExitCode != 0 { + t.Fatalf("first run ExitCode: got %d, want 0", res.ExitCode) + } + if got, want := res.Written, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("first run Written: got %v, want %v", got, want) + } + if len(res.Skipped) != 0 || len(res.Forced) != 0 { + t.Errorf("first run should have no skips/forced: skipped=%v forced=%v", res.Skipped, res.Forced) + } + out := stdout.String() + if !strings.Contains(out, "Written:") { + t.Errorf("summary missing Written section; got:\n%s", out) + } + if !strings.Contains(out, initAssetName) { + t.Errorf("summary missing asset name; got:\n%s", out) + } + if _, statErr := os.Stat(deployedInitAssetPath(dir)); statErr != nil { + t.Errorf("expected deployed asset on disk: %v", statErr) + } + + // Second run: the existing asset is skipped. + stdout.Reset() + res2, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: dir}) + if err != nil { + t.Fatalf("second RunInit returned error: %v", err) + } + if got, want := res2.Skipped, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("second run Skipped: got %v, want %v", got, want) + } + if len(res2.Written) != 0 { + t.Errorf("second run Written should be empty: got %v", res2.Written) + } + if !strings.Contains(stdout.String(), "Skipped:") { + t.Errorf("second run summary missing Skipped section; got:\n%s", stdout.String()) + } + + // Third run with Force: the asset is overwritten. + stdout.Reset() + res3, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: dir, Force: true}) + if err != nil { + t.Fatalf("force RunInit returned error: %v", err) + } + if got, want := res3.Forced, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("force run Forced: got %v, want %v", got, want) + } + if len(res3.Written) != 0 || len(res3.Skipped) != 0 { + t.Errorf("force run should only report forced: written=%v skipped=%v", res3.Written, res3.Skipped) + } + if !strings.Contains(stdout.String(), "Forced:") { + t.Errorf("force run summary missing Forced section; got:\n%s", stdout.String()) + } +} + +// TestRunInit_JSONLifecycle covers task 8.2: the --json payload unmarshals and +// carries the expected per-stage values, with every key present as an array. +func TestRunInit_JSONLifecycle(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + type initPayload struct { + Written []string `json:"written"` + Skipped []string `json:"skipped"` + Forced []string `json:"forced"` + } + + run := func(force bool) initPayload { + t.Helper() + var stdout, stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: dir, + Force: force, + JSON: true, + }) + if err != nil { + t.Fatalf("RunInit(force=%v) returned error: %v", force, err) + } + if res.ExitCode != 0 { + t.Fatalf("RunInit(force=%v) ExitCode: got %d, want 0", force, res.ExitCode) + } + var p initPayload + if err := json.Unmarshal(stdout.Bytes(), &p); err != nil { + t.Fatalf("json.Unmarshal(%q): %v", stdout.String(), err) + } + return p + } + + // First run: only written is populated; skipped and forced are empty arrays. + first := run(false) + if got, want := first.Written, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("first run written: got %v, want %v", got, want) + } + if len(first.Skipped) != 0 { + t.Errorf("first run skipped: got %v, want empty", first.Skipped) + } + if len(first.Forced) != 0 { + t.Errorf("first run forced: got %v, want empty", first.Forced) + } + + // Second run: the asset moves to skipped. + second := run(false) + if got, want := second.Skipped, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("second run skipped: got %v, want %v", got, want) + } + if len(second.Written) != 0 { + t.Errorf("second run written: got %v, want empty", second.Written) + } + + // Force run: the asset moves to forced. + forced := run(true) + if got, want := forced.Forced, []string{initAssetName}; !slices.Equal(got, want) { + t.Errorf("force run forced: got %v, want %v", got, want) + } + if len(forced.Written) != 0 || len(forced.Skipped) != 0 { + t.Errorf("force run should only populate forced: written=%v skipped=%v", forced.Written, forced.Skipped) + } +} + +// TestRunInit_JSONArraysAreNeverNull verifies the --json payload emits empty +// arrays rather than null for the unused categories on a first run. +func TestRunInit_JSONArraysAreNeverNull(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + if _, err := RunInit(context.Background(), InitOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: t.TempDir(), + JSON: true, + }); err != nil { + t.Fatalf("RunInit returned error: %v", err) + } + if strings.Contains(stdout.String(), "null") { + t.Errorf("JSON payload must not contain null arrays; got:\n%s", stdout.String()) + } +} + +// TestRunInit_SuccessExitCode covers task 8.3: a valid deployment exits 0. +func TestRunInit_SuccessExitCode(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: t.TempDir()}) + if err != nil { + t.Fatalf("RunInit returned error: %v", err) + } + if res.ExitCode != 0 { + t.Errorf("ExitCode: got %d, want 0", res.ExitCode) + } +} + +// TestRunInit_NonexistentPathExitsTwo covers task 8.3: an invalid (nonexistent) +// target path exits 2, writes nothing to stdout, and creates no files. +func TestRunInit_NonexistentPathExitsTwo(t *testing.T) { + t.Parallel() + missing := filepath.Join(t.TempDir(), "does-not-exist") + var stdout, stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: missing}) + if err == nil { + t.Fatal("expected error for nonexistent path, got nil") + } + if res == nil || res.ExitCode != 2 { + t.Fatalf("ExitCode: got %v, want 2", res) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on error, got: %q", stdout.String()) + } + if _, statErr := os.Stat(missing); !os.IsNotExist(statErr) { + t.Errorf("nonexistent path must not be created: stat err = %v", statErr) + } +} + +// TestRunInit_TraversalPathExitsTwo covers task 8.3: a path-traversal target is +// rejected with exit 2 and nothing is written. +func TestRunInit_TraversalPathExitsTwo(t *testing.T) { + t.Parallel() + base := t.TempDir() + traversal := base + string(os.PathSeparator) + ".." + string(os.PathSeparator) + "escape" + var stdout, stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{Stdout: &stdout, Stderr: &stderr, Path: traversal}) + if err == nil { + t.Fatal("expected error for traversal path, got nil") + } + if res == nil || res.ExitCode != 2 { + t.Fatalf("ExitCode: got %v, want 2", res) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on error, got: %q", stdout.String()) + } + if _, statErr := os.Stat(filepath.Join(filepath.Dir(base), "escape", ".opencode")); !os.IsNotExist(statErr) { + t.Errorf(".opencode must not be created via traversal: stat err = %v", statErr) + } +} + +// TestRunInit_WriteFileErrorExitsTwo covers task 8.3: an I/O failure surfaced +// through the injected writeFile seam exits 2, wraps the underlying error, and +// writes nothing to stdout. +func TestRunInit_WriteFileErrorExitsTwo(t *testing.T) { + t.Parallel() + sentinel := errors.New("simulated disk failure") + var stdout, stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{ + Stdout: &stdout, + Stderr: &stderr, + Path: t.TempDir(), + writeFile: func(string, []byte, fs.FileMode) error { return sentinel }, + }) + if err == nil { + t.Fatal("expected error from failing writeFile, got nil") + } + if !errors.Is(err, sentinel) { + t.Errorf("error chain: got %v, want to wrap %v", err, sentinel) + } + if res == nil || res.ExitCode != 2 { + t.Fatalf("ExitCode: got %v, want 2", res) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on error, got: %q", stdout.String()) + } +} + +// TestRunInit_StdoutWriteErrorExitsTwo exercises the atomic-output error path: +// when the summary cannot be written to stdout, RunInit exits 2. +func TestRunInit_StdoutWriteErrorExitsTwo(t *testing.T) { + t.Parallel() + sentinel := errors.New("broken pipe") + var stderr bytes.Buffer + res, err := RunInit(context.Background(), InitOptions{ + Stdout: initFailingWriter{err: sentinel}, + Stderr: &stderr, + Path: t.TempDir(), + }) + if err == nil { + t.Fatal("expected error from failing stdout writer, got nil") + } + if !errors.Is(err, sentinel) { + t.Errorf("error chain: got %v, want to wrap %v", err, sentinel) + } + if res == nil || res.ExitCode != 2 { + t.Fatalf("ExitCode: got %v, want 2", res) + } +} + +// TestRunInit_CancelledContextExitsTwo verifies RunInit honors context +// cancellation before doing filesystem work. +func TestRunInit_CancelledContextExitsTwo(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + var stdout, stderr bytes.Buffer + res, err := RunInit(ctx, InitOptions{Stdout: &stdout, Stderr: &stderr, Path: t.TempDir()}) + if err == nil { + t.Fatal("expected error for cancelled context, got nil") + } + if res == nil || res.ExitCode != 2 { + t.Fatalf("ExitCode: got %v, want 2", res) + } + if stdout.Len() != 0 { + t.Errorf("stdout must be empty on error, got: %q", stdout.String()) + } +} + +// TestInitCmd_Execute verifies the cobra command layer wires flags and args +// through to a successful deployment. +func TestInitCmd_Execute(t *testing.T) { + t.Parallel() + dir := t.TempDir() + cmd := initCmd() + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{dir}) + if err := cmd.Execute(); err != nil { + t.Fatalf("initCmd Execute returned error: %v", err) + } + if !strings.Contains(stdout.String(), initAssetName) { + t.Errorf("expected asset name in output; got:\n%s", stdout.String()) + } + if _, statErr := os.Stat(deployedInitAssetPath(dir)); statErr != nil { + t.Errorf("expected deployed asset on disk: %v", statErr) + } +} + +// TestInitCmd_JSONFlag verifies the --json flag produces a machine-readable +// payload through the command layer. +func TestInitCmd_JSONFlag(t *testing.T) { + t.Parallel() + dir := t.TempDir() + cmd := initCmd() + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"--json", dir}) + if err := cmd.Execute(); err != nil { + t.Fatalf("initCmd --json Execute returned error: %v", err) + } + var p struct { + Written []string `json:"written"` + } + if err := json.Unmarshal(stdout.Bytes(), &p); err != nil { + t.Fatalf("json.Unmarshal(%q): %v", stdout.String(), err) + } + if !slices.Contains(p.Written, initAssetName) { + t.Errorf("expected %s in written; got %v", initAssetName, p.Written) + } +} + +// TestInitCmd_InvalidPathReturnsExitCodeError verifies the command layer maps a +// RunInit failure to an *exitCodeError carrying exit code 2. +func TestInitCmd_InvalidPathReturnsExitCodeError(t *testing.T) { + t.Parallel() + cmd := initCmd() + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{filepath.Join(t.TempDir(), "missing")}) + err := cmd.Execute() + if err == nil { + t.Fatal("expected error for invalid path, got nil") + } + var ece *exitCodeError + if !errors.As(err, &ece) { + t.Fatalf("error type: got %T, want *exitCodeError", err) + } + if ece.code != 2 { + t.Errorf("exit code: got %d, want 2", ece.code) + } + if !strings.Contains(stderr.String(), "Error:") { + t.Errorf("expected diagnostic on stderr; got: %q", stderr.String()) + } +} diff --git a/cmd/vibe-check/root.go b/cmd/vibe-check/root.go index 7f1114c..5939d7a 100644 --- a/cmd/vibe-check/root.go +++ b/cmd/vibe-check/root.go @@ -19,6 +19,8 @@ and circular dependency detection for Go codebases.`, } cmd.AddCommand(analyzeCmd()) + cmd.AddCommand(diffCmd()) + cmd.AddCommand(initCmd()) return cmd } diff --git a/internal/goadapter/env_test.go b/internal/goadapter/env_test.go new file mode 100644 index 0000000..349764f --- /dev/null +++ b/internal/goadapter/env_test.go @@ -0,0 +1,36 @@ +package goadapter + +import ( + "slices" + "testing" +) + +// TestPackageEnv_ForcesLocalToolchain asserts the constructed subprocess +// environment always contains GOTOOLCHAIN=local, hardening analyze so a target +// module's go.mod "toolchain" directive cannot trigger a toolchain download. +func TestPackageEnv_ForcesLocalToolchain(t *testing.T) { + t.Parallel() + + env := packageEnv() + if !slices.Contains(env, "GOTOOLCHAIN=local") { + t.Errorf("packageEnv() must contain %q, got %v", "GOTOOLCHAIN=local", env) + } +} + +// TestPackageEnv_HostValueDoesNotLeak asserts the forced value wins even when the +// host process sets GOTOOLCHAIN. Because GOTOOLCHAIN is not on the allowlist the +// host value is filtered out by SanitizeEnvironment, and the appended +// GOTOOLCHAIN=local is authoritative (exec resolves duplicate keys last-wins +// regardless). t.Setenv precludes t.Parallel() here. +func TestPackageEnv_HostValueDoesNotLeak(t *testing.T) { + t.Setenv("GOTOOLCHAIN", "auto") + + env := packageEnv() + if !slices.Contains(env, "GOTOOLCHAIN=local") { + t.Errorf("packageEnv() must contain %q even when the host sets GOTOOLCHAIN=auto, got %v", + "GOTOOLCHAIN=local", env) + } + if slices.Contains(env, "GOTOOLCHAIN=auto") { + t.Errorf("packageEnv() must NOT contain the host value %q, got %v", "GOTOOLCHAIN=auto", env) + } +} diff --git a/internal/goadapter/resolve.go b/internal/goadapter/resolve.go index ee13002..4419f23 100644 --- a/internal/goadapter/resolve.go +++ b/internal/goadapter/resolve.go @@ -39,6 +39,18 @@ var packageEnvAllowlist = []string{ "GOMOD", } +// packageEnv builds the environment for the go/packages subprocess. It starts +// from the sanitized allowlist ([metrics.SanitizeEnvironment]) and appends +// "GOTOOLCHAIN=local" as the LAST entry so it wins: on exec a duplicated key is +// resolved last-wins, forcing the local toolchain regardless of the host +// environment. This blocks a target module's go.mod "toolchain" directive from +// triggering a Go toolchain download and execution during subprocess package +// loading. GOTOOLCHAIN is intentionally NOT in packageEnvAllowlist, so no host +// value ever passes through — the appended value is authoritative. +func packageEnv() []string { + return append(metrics.SanitizeEnvironment(packageEnvAllowlist), "GOTOOLCHAIN=local") +} + // resolvePackages loads Go packages from projectPath using go/packages and // builds the module-internal import adjacency map used to compute Ca/Ce. It // filters results to module-internal packages and records warnings for any @@ -59,7 +71,7 @@ func resolvePackages(ctx context.Context, projectPath string) ( Mode: loadFlags, Dir: projectPath, Context: ctx, - Env: metrics.SanitizeEnvironment(packageEnvAllowlist), + Env: packageEnv(), Tests: false, } diff --git a/internal/scaffold/assets/agents/divisor-entropy.md b/internal/scaffold/assets/agents/divisor-entropy.md new file mode 100644 index 0000000..6421f28 --- /dev/null +++ b/internal/scaffold/assets/agents/divisor-entropy.md @@ -0,0 +1,261 @@ +--- +description: "Structural-entropy divisor — measures the base→PR change in design-quality metrics (coupling, instability, abstractness, distance, LCOM, circular dependencies) via vibe-check and reports a verdict." +mode: subagent +temperature: 0.1 +permission: + edit: deny + webfetch: deny + bash: + "*": "deny" + "git merge-base *": "allow" + "git rev-parse *": "allow" + "git worktree add *": "allow" + "git worktree remove *": "allow" + "git worktree prune": "allow" + "git fetch origin *": "allow" + "git check-ref-format *": "allow" + "vibe-check analyze *": "allow" + "vibe-check diff *": "allow" +--- + + +# Role: The Entropy Divisor + +You are the structural-entropy reviewer for this project. Your exclusive domain +is **architectural drift** — whether a pull request degrades the design quality +of the codebase relative to its base. You measure the base→PR change in the +Martin design-quality metrics — afferent coupling (Ca), efferent coupling (Ce), +instability (I), abstractness (A), distance from the main sequence (D), cohesion +(LCOM4), and circular dependencies — and report a verdict backed by an auditable +metric-delta table. You enforce the Boy Scout Rule: a PR should not leave the +architecture measurably worse than it found it. + +You do NOT compute metric arithmetic in-prompt. The deltas and the verdict are +computed for you by the tested Go `vibe-check diff` command; you orchestrate the +measurement, then report and explain its output. This keeps the verdict +deterministic and its thresholds protected in tested code, not in fallible prompt +arithmetic. + +You operate in **Code Review Mode**: the caller asks you to review the changes on +a PR branch against its base. + +--- + +## Step 0: Prior Learnings (optional) + +If Dewey MCP tools are available (`dewey_semantic_search`): + +1. Query for prior learnings about structural entropy, coupling, and cohesion + regressions: + `dewey_semantic_search({ query: "structural entropy coupling instability cohesion regression" })` +2. Query for learnings related to the packages touched by the diff: + `dewey_semantic_search({ query: "" })` +3. Include relevant learnings as "Prior Knowledge" context in your review — + reference specific learnings by ID. + +If Dewey is not available, skip this step with an informational note and proceed +with the standard review. + +--- + +## Source Documents + +Before reviewing, read: + +1. `AGENTS.md` -- Project overview, architecture, and coding conventions +2. `.specify/memory/constitution.md` -- Constitution principles (if present) +3. `.opencode/uf/packs/severity.md` -- Shared severity definitions (MUST load for + consistent severity classification across the council) +4. Any other `*.md` files under `.opencode/uf/packs/` that apply to the change. + If no pack files are found, note this and proceed with universal checks only. + +--- + +## Code Review Mode + +This is the default mode. You compute the structural delta between the base ref +and the PR ref, then report the Go-computed verdict. + +### Ref validation (run these IN ORDER, before a ref reaches any command) + +The base ref is overridable, so it is untrusted input. Validate it in exactly +this order before it is ever interpolated into a command: + +1. **Regex gate FIRST.** Reject any base ref that does not match + `^[A-Za-z0-9._/-]+$`. This is the PRIMARY metacharacter defense and is applied + before the ref reaches ANY command. It rejects spaces, `;`, `&&`, `||`, `|`, + `$(...)`, backticks, redirection, and `=`, so a ref can neither smuggle shell + metacharacters nor form a dangerous option (which would require `=` or a + space). +2. **`git check-ref-format` THEN.** Run `git check-ref-format ""` for + ref-semantic validation. This rejects `..`, `@{`, a leading `-`, and a + trailing `.lock` — cases the regex alone permits (the regex allows `.` and + `/`, and therefore `..`). The regex and `check-ref-format` are complementary, + not equivalent. +3. **Resolve to a SHA THEN.** Resolve the validated ref with + `git rev-parse --verify --end-of-options "^{commit}"` so it can never be + parsed as a flag or a chained command. Use ONLY the resulting SHA thereafter; + never re-interpolate the untrusted branch string. + +### Delta workflow + +1. **Determine the base ref.** Default the base to + `git merge-base HEAD origin/main` (matching the council's three-dot + `git diff main...HEAD` scope). If an explicit base ref is supplied, run the + ordered ref-validation steps above on it first. +2. **Fetch only if needed, only after the regex.** In a shallow CI clone the + merge-base may be unavailable. If the base ref must be fetched, run + `git fetch origin ` ONLY after the `^[A-Za-z0-9._/-]+$` regex has + validated it. `git fetch` accepts no `--end-of-options` terminator, so for + this command the regex is the load-bearing pre-fetch guard. +3. **Create an isolated worktree.** Run `git worktree prune` first to clear any + stale entries, then `git worktree add` the resolved base SHA into a UNIQUE + temp directory OUTSIDE the repo tree (e.g. under the system temp dir), so the + PR working tree and index are never mutated and no tool scanning the repo + picks up the worktree as part of the module. +4. **Analyze both refs with the SAME binary.** Run + `vibe-check analyze --output ` in the base worktree and + `vibe-check analyze --output ` on the PR checkout, each with a + bounded `--timeout`. Use `vibe-check` — NOT `gaze` or `goda` — and use the + `--output` flag to materialize each JSON graph to a file (the allowlist + forbids shell redirection). Using the same binary for both makes the delta + reflect source changes, not tool-version changes. +5. **Diff the two graphs.** Run `vibe-check diff ` to + compute the per-package deltas, classify cycles, and render the verdict. +6. **Remove the worktree.** Run `git worktree remove --force` on the temp + worktree — ALWAYS, including when analysis failed — so no residual state + remains. + +### The verdict is computed BY `vibe-check diff` + +The verdict is produced by the tested Go `metrics.DecideVerdict` gates inside +`vibe-check diff`, NOT by in-prompt arithmetic. Report and explain it; do not +recompute thresholds yourself. The deterministic gates are: + +- a new circular dependency (present in the PR graph, absent from the base) → + **REQUEST CHANGES**; +- any existing package's ΔInstability ≥ 0.15 → **REQUEST CHANGES**; +- any package's ΔDistance ≥ 0.20 → **REQUEST CHANGES**; +- any package's ΔLCOM ≥ 2 → **REQUEST CHANGES**; +- smaller non-zero shifts that cross no threshold → **COMMENT**; +- metrics improve or stay stable → **APPROVE**. + +Pre-existing cycles that are unchanged do NOT, on their own, trigger REQUEST +CHANGES. Added and removed packages are reported for information only and never +trigger a gate. Float deltas are rounded to 4 decimal places before comparison +and the gates are inclusive (`≥`); the exact rules are the single source of truth +inside `vibe-check diff`. + +--- + +## Out of Scope + +These dimensions are owned by other Divisor personas — do NOT produce findings +for them: + +- **Security / credentials / injection** → The Adversary +- **General structure, patterns, conventions, DRY** → The Architect +- **Test coverage depth / assertion quality** → The Tester +- **Plan alignment / intent drift / zero-waste / constitution** → The Guard +- **Operational readiness / deployment / performance** → The SRE +- **Documentation & content pipeline** → The Curator + +Your lane is strictly the base→PR structural-metric *delta*. Absolute, +single-snapshot metric ceilings are enforced by `vibe-check analyze --max-*` +flags, not by this divisor — do not re-litigate a package's standing coupling if +the PR did not change it. + +--- + +## Output Format + +Report, in this order: + +1. The **base ref SHA** and **PR ref SHA** compared. +2. A **per-package delta table** with one row per changed package and the columns + `Ca`, `Ce`, `Instability`, `Abstractness`, `Distance`, `LCOM`, each shown as + `base → PR (Δ)`, sourced from the `vibe-check diff` JSON output. +3. The **list of newly introduced cycles** (if any), sourced from the diff JSON. +4. The overall **entropy direction** (improving / stable / degrading), sourced + from the diff JSON. +5. Findings in the standard divisor block format: + +``` +### [SEVERITY] Finding Title + +**File**: `path/to/package` +**Constraint**: Structural entropy (name the metric that regressed) +**Description**: What degraded, by how much (base → PR, Δ), and why it matters +**Recommendation**: How to reduce the regression +``` + +Severity levels: CRITICAL, HIGH, MEDIUM, LOW (per `.opencode/uf/packs/severity.md`). + +6. A domain **Score (1–10)**: + - 9-10: metrics improve or hold; no regression + - 7-8: negligible drift within rounding noise + - 5-6: COMMENT-band regressions worth discussion + - 3-4: at least one REQUEST CHANGES threshold crossed + - 1-2: multiple gates crossed and/or a new cycle introduced +7. A final **verdict line** — `APPROVE`, `REQUEST CHANGES`, or `COMMENT` — which + MUST be the Go-computed verdict reported by `vibe-check diff`. + +--- + +## Decision Criteria + +- **APPROVE** when metrics improve or remain stable and no gate fired + (`vibe-check diff` reports `APPROVE`). +- **REQUEST CHANGES** when `vibe-check diff` reports `REQUEST_CHANGES`: a new + cycle, or ΔInstability ≥ 0.15, ΔDistance ≥ 0.20, or ΔLCOM ≥ 2 on any package. +- **COMMENT** for smaller material shifts below every threshold, and whenever the + measurement is unreliable (see Graceful Degradation). + +End your review with a clear verdict line, the domain Score, and a summary of +findings. The verdict MUST be the one reported by `vibe-check diff` — you report +and explain it, you do not override it. + +### Graceful Degradation + +Return **COMMENT** — never a false APPROVE, and never a crash — whenever you +cannot obtain a reliable measurement, including when: + +- the `vibe-check` binary is not on `PATH`; +- the base ref cannot be resolved or analyzed (for example a shallow clone whose + merge-base cannot be fetched, or a base that does not build — a broken base + cannot serve as a baseline); +- the PR ref fails to analyze (for example the PR does not build) — clearly + distinguish "PR does not build" from a clean measurement; +- `git worktree` creation fails; +- either analysis returns a partial build (`Status: "partial"`, or load-error + warnings, with zeroed type metrics) — treat this as a degraded measurement, + not a clean one; `vibe-check diff` marks such a delta unreliable and forces + COMMENT; +- `vibe-check diff` cannot produce a verdict. + +In every degraded case, report the limitation clearly so the reviewer +understands why full delta data is unavailable, and still remove the temporary +worktree. + +--- + +## Security / Operating Constraints + +`vibe-check analyze` loads the target module with `go/packages` type-checking, +which **executes the target's own build tooling** — compilation and any cgo — to +resolve types. Analyzing a ref therefore runs code from that ref on the host: a +residual **code-execution surface**. + +`GOTOOLCHAIN=local` (forced by the `vibe-check analyze` binary inside its +sanitized subprocess environment) does **NOT** close this surface. It closes only +the `go.mod` `toolchain`-directive vector — it prevents an untrusted `go.mod` from +downloading and executing a different toolchain from a proxy. It does NOT prevent +cgo or other build-time code from running during analysis. + +Consequently this divisor **MUST only run on refs the CI context already trusts** +(same-repo PRs or already-built branches) and **MUST NOT be wired into CI that +analyzes untrusted fork pull requests**. Privileged CI SHOULD additionally export +`GOTOOLCHAIN=local` ambiently as defense-in-depth. Do not remove or widen the +`bash` allowlist in this agent's frontmatter to work around these constraints — +the allowlist, the ref-sanitization regex, and `git rev-parse --verify +--end-of-options` are the load-bearing controls that keep this reviewer safe. diff --git a/internal/scaffold/doc.go b/internal/scaffold/doc.go new file mode 100644 index 0000000..1dd62c1 --- /dev/null +++ b/internal/scaffold/doc.go @@ -0,0 +1,25 @@ +// Package scaffold embeds the vibe-check agent asset templates and deploys them +// into a target repository's .opencode/agents/ directory via `vibe-check init`. +// +// # Role +// +// scaffold is the implementation backing the `vibe-check init` command. It +// carries the canonical, version-controlled agent definitions (currently the +// divisor-entropy Review Council agent) as files embedded into the binary, then +// writes them into a consuming project so the Review Council can auto-discover +// them. Shipping the assets inside the same binary that runs `vibe-check +// analyze` keeps the deployed agent aligned with the metrics engine it drives. +// +// # Layout +// +// - embed.go embeds assets/agents/*.md into an [embed.FS] exposed by [Assets]. +// - assets/agents/ holds the Markdown agent definitions that are the single +// source of truth for what `vibe-check init` deploys. +// +// The embedded asset is the single source of truth: the copy deployed into a +// repository is generated from it rather than hand-authored, which prevents the +// deployed agent from drifting away from the version shipped in the binary. +// +// This package lives under internal/ because scaffolding is a CLI implementation +// detail that external modules must not import. +package scaffold diff --git a/internal/scaffold/embed.go b/internal/scaffold/embed.go new file mode 100644 index 0000000..91b455d --- /dev/null +++ b/internal/scaffold/embed.go @@ -0,0 +1,12 @@ +package scaffold + +import "embed" + +//go:embed assets/agents/*.md +var assetsFS embed.FS + +// Assets returns the embedded filesystem containing the agent asset +// templates that vibe-check init deploys into a target repository. +func Assets() embed.FS { + return assetsFS +} diff --git a/internal/scaffold/scaffold.go b/internal/scaffold/scaffold.go new file mode 100644 index 0000000..1d2795d --- /dev/null +++ b/internal/scaffold/scaffold.go @@ -0,0 +1,226 @@ +package scaffold + +import ( + "fmt" + "io/fs" + "os" + "path" + "path/filepath" + "sort" + "strings" + + "github.com/zero-dot-force/vibe-check/metrics" +) + +const ( + // assetSourceDir is the directory within the embedded filesystem that holds + // the agent asset templates. The embed glob is assetSourceDir + "/*.md". + assetSourceDir = "assets/agents" + + // targetSubdir is the directory, relative to the target repository root, + // into which agent assets are deployed. + targetSubdir = ".opencode/agents" + + // dirPerm is the mode applied to directories created during deployment. + dirPerm fs.FileMode = 0o755 + + // filePerm is the mode applied to asset files written during deployment. + filePerm fs.FileMode = 0o644 +) + +// Options configures a scaffold Run. +type Options struct { + // TargetDir is the root directory of the repository into which agent assets + // are deployed. It MUST be an existing directory; traversal components are + // rejected. + TargetDir string + + // Force, when true, overwrites existing destination files. When false + // (the default), pre-existing files are left unchanged and reported as + // skipped. + Force bool + + // WriteFile is an injectable seam for writing a destination file. When nil, + // os.WriteFile is used. It mirrors the os.WriteFile signature so tests can + // substitute a stub to exercise the I/O-failure path without special + // privileges. + WriteFile func(path string, data []byte, perm fs.FileMode) error +} + +// Result reports the outcome of a scaffold Run. Each slice holds the base +// filenames of the affected assets in stable, ascending lexicographic order. +type Result struct { + // Written lists assets that were newly created. + Written []string + + // Skipped lists assets that already existed and were left unchanged because + // Force was false. + Skipped []string + + // Forced lists assets that already existed and were overwritten because + // Force was true. + Forced []string +} + +// Run deploys the embedded agent assets into opts.TargetDir. It validates the +// target directory, creates the .opencode/agents tree if necessary, and writes +// each embedded asset, skipping or overwriting existing files according to +// opts.Force. It returns a Result describing which assets were written, +// skipped, or forced. +func Run(opts Options) (*Result, error) { + return run(assetsFS, opts) +} + +// run is the core scaffold routine parameterized over the source filesystem so +// tests can inject a synthetic fs.FS. Run calls it with the embedded assetsFS. +func run(assets fs.FS, opts Options) (*Result, error) { + if err := metrics.ValidateProjectPath(opts.TargetDir); err != nil { + return nil, fmt.Errorf("scaffold: validate target directory: %w", err) + } + + // Canonicalize the validated root so containment checks compare against the + // resolved path. ValidateProjectPath already confirmed it exists and is a + // directory. + root, err := filepath.EvalSymlinks(opts.TargetDir) + if err != nil { + return nil, fmt.Errorf("scaffold: resolve target directory %q: %w", opts.TargetDir, err) + } + + writeFile := opts.WriteFile + if writeFile == nil { + writeFile = os.WriteFile + } + + entries, err := fs.Glob(assets, assetSourceDir+"/*.md") + if err != nil { + return nil, fmt.Errorf("scaffold: enumerate embedded assets: %w", err) + } + + destDir, err := ensureDir(root, targetSubdir) + if err != nil { + return nil, err + } + + result := &Result{} + for _, entry := range entries { + name := path.Base(entry) + + data, err := fs.ReadFile(assets, entry) + if err != nil { + return nil, fmt.Errorf("scaffold: read embedded asset %q: %w", entry, err) + } + + destPath := filepath.Join(destDir, name) + + existed, err := regularFileExists(destPath) + if err != nil { + return nil, err + } + if existed && !opts.Force { + result.Skipped = append(result.Skipped, name) + continue + } + + if err := writeFile(destPath, data, filePerm); err != nil { + return nil, fmt.Errorf("scaffold: write asset %q: %w", destPath, err) + } + // os.WriteFile preserves an existing file's mode on overwrite and is + // subject to umask on create, so normalize the mode explicitly to keep + // deployment deterministic. + if err := os.Chmod(destPath, filePerm); err != nil { + return nil, fmt.Errorf("scaffold: set mode on %q: %w", destPath, err) + } + + if existed { + result.Forced = append(result.Forced, name) + } else { + result.Written = append(result.Written, name) + } + } + + sort.Strings(result.Written) + sort.Strings(result.Skipped) + sort.Strings(result.Forced) + + return result, nil +} + +// ensureDir creates rel (a slash-separated path relative to root) one component +// at a time, verifying after each step that the component is not a symlink and +// still resolves inside root. This prevents a symlink planted at an +// intermediate component (for example .opencode or .opencode/agents) from +// redirecting writes outside the validated root. It returns the absolute path +// of the deepest directory. +func ensureDir(root, rel string) (string, error) { + current := root + for _, part := range strings.Split(rel, "/") { + if part == "" { + continue + } + current = filepath.Join(current, part) + + info, err := os.Lstat(current) + switch { + case err == nil: + if info.Mode()&fs.ModeSymlink != 0 { + return "", fmt.Errorf("scaffold: refusing to follow symlink in deploy path: %s", current) + } + if !info.IsDir() { + return "", fmt.Errorf("scaffold: deploy path component is not a directory: %s", current) + } + case os.IsNotExist(err): + if mkErr := os.Mkdir(current, dirPerm); mkErr != nil { + return "", fmt.Errorf("scaffold: create directory %q: %w", current, mkErr) + } + // Mkdir is subject to umask; normalize to the intended mode. + if chErr := os.Chmod(current, dirPerm); chErr != nil { + return "", fmt.Errorf("scaffold: set mode on directory %q: %w", current, chErr) + } + default: + return "", fmt.Errorf("scaffold: inspect %q: %w", current, err) + } + + if err := verifyContained(root, current); err != nil { + return "", err + } + } + + return current, nil +} + +// verifyContained resolves target and confirms it lies within root. root MUST +// already be a symlink-resolved absolute path. +func verifyContained(root, target string) error { + resolved, err := filepath.EvalSymlinks(target) + if err != nil { + return fmt.Errorf("scaffold: resolve %q: %w", target, err) + } + + rel, err := filepath.Rel(root, resolved) + if err != nil { + return fmt.Errorf("scaffold: compute relative path for %q: %w", resolved, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return fmt.Errorf("scaffold: deploy path escapes target root: %s", target) + } + + return nil +} + +// regularFileExists reports whether p exists as a regular file. It uses Lstat so +// a symlink at the destination is detected rather than followed, and returns an +// error if the destination is a symlink. +func regularFileExists(p string) (bool, error) { + info, err := os.Lstat(p) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("scaffold: inspect %q: %w", p, err) + } + if info.Mode()&fs.ModeSymlink != 0 { + return false, fmt.Errorf("scaffold: refusing to overwrite symlink: %s", p) + } + + return true, nil +} diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go new file mode 100644 index 0000000..0937f9d --- /dev/null +++ b/internal/scaffold/scaffold_test.go @@ -0,0 +1,507 @@ +package scaffold + +import ( + "bytes" + "errors" + "io/fs" + "os" + "path" + "path/filepath" + "runtime" + "slices" + "strings" + "testing" + "testing/fstest" +) + +const ( + embeddedAssetPath = "assets/agents/divisor-entropy.md" + deployedAssetName = "divisor-entropy.md" +) + +// TestRun_DeploysEmbeddedAsset covers task 6.1: assets land in +// .opencode/agents/ with a 0o755 directory tree and 0o644 files, and the +// deployed bytes match the embedded source. +func TestRun_DeploysEmbeddedAsset(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + res, err := Run(Options{TargetDir: dir}) + if err != nil { + t.Fatalf("Run returned error: %v", err) + } + if !slices.Equal(res.Written, []string{deployedAssetName}) { + t.Errorf("Written: got %v, want %v", res.Written, []string{deployedAssetName}) + } + if len(res.Skipped) != 0 { + t.Errorf("Skipped: got %v, want empty", res.Skipped) + } + if len(res.Forced) != 0 { + t.Errorf("Forced: got %v, want empty", res.Forced) + } + + openCodeDir := filepath.Join(dir, ".opencode") + if oi, err := os.Stat(openCodeDir); err != nil || !oi.IsDir() { + t.Fatalf(".opencode dir missing or not a directory: %v", err) + } + + agentsDir := filepath.Join(openCodeDir, "agents") + di, err := os.Stat(agentsDir) + if err != nil { + t.Fatalf("stat agents dir: %v", err) + } + if !di.IsDir() { + t.Fatalf("%s is not a directory", agentsDir) + } + if got := di.Mode().Perm(); got != 0o755 { + t.Errorf("agents dir perm: got %o, want %o", got, 0o755) + } + + assetPath := filepath.Join(agentsDir, deployedAssetName) + fi, err := os.Stat(assetPath) + if err != nil { + t.Fatalf("stat deployed asset: %v", err) + } + if got := fi.Mode().Perm(); got != 0o644 { + t.Errorf("asset perm: got %o, want %o", got, 0o644) + } + + got, err := os.ReadFile(assetPath) + if err != nil { + t.Fatalf("read deployed asset: %v", err) + } + want, err := fs.ReadFile(assetsFS, embeddedAssetPath) + if err != nil { + t.Fatalf("read embedded asset: %v", err) + } + if !bytes.Equal(got, want) { + t.Errorf("deployed asset content does not match embedded source") + } +} + +// TestRun_SkipsExistingByDefault covers task 6.1: a second run without Force +// reports the existing asset as skipped and writes nothing. +func TestRun_SkipsExistingByDefault(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + if _, err := Run(Options{TargetDir: dir}); err != nil { + t.Fatalf("first Run: %v", err) + } + res, err := Run(Options{TargetDir: dir}) + if err != nil { + t.Fatalf("second Run: %v", err) + } + if !slices.Equal(res.Skipped, []string{deployedAssetName}) { + t.Errorf("Skipped: got %v, want %v", res.Skipped, []string{deployedAssetName}) + } + if len(res.Written) != 0 || len(res.Forced) != 0 { + t.Errorf("expected only skips; got Written=%v Forced=%v", res.Written, res.Forced) + } +} + +// TestRun_ForceOverwrites covers task 6.1: Force overwrites an existing asset +// and reports it as forced. +func TestRun_ForceOverwrites(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + if _, err := Run(Options{TargetDir: dir}); err != nil { + t.Fatalf("first Run: %v", err) + } + res, err := Run(Options{TargetDir: dir, Force: true}) + if err != nil { + t.Fatalf("force Run: %v", err) + } + if !slices.Equal(res.Forced, []string{deployedAssetName}) { + t.Errorf("Forced: got %v, want %v", res.Forced, []string{deployedAssetName}) + } + if len(res.Written) != 0 || len(res.Skipped) != 0 { + t.Errorf("expected only forced; got Written=%v Skipped=%v", res.Written, res.Skipped) + } +} + +// TestRun_ForceNormalizesPermissions covers task 6.2: a pre-existing file with +// a loose mode is normalized to 0o644 on a forced overwrite. +func TestRun_ForceNormalizesPermissions(t *testing.T) { + t.Parallel() + dir := t.TempDir() + agentsDir := filepath.Join(dir, ".opencode", "agents") + if err := os.MkdirAll(agentsDir, 0o755); err != nil { + t.Fatalf("mkdir agents dir: %v", err) + } + assetPath := filepath.Join(agentsDir, deployedAssetName) + if err := os.WriteFile(assetPath, []byte("stale"), 0o666); err != nil { + t.Fatalf("pre-create asset: %v", err) + } + // WriteFile is subject to umask; force the loose mode explicitly. + if err := os.Chmod(assetPath, 0o666); err != nil { + t.Fatalf("chmod pre-create asset: %v", err) + } + + res, err := Run(Options{TargetDir: dir, Force: true}) + if err != nil { + t.Fatalf("force Run: %v", err) + } + if !slices.Equal(res.Forced, []string{deployedAssetName}) { + t.Errorf("Forced: got %v, want %v", res.Forced, []string{deployedAssetName}) + } + fi, err := os.Stat(assetPath) + if err != nil { + t.Fatalf("stat asset: %v", err) + } + if got := fi.Mode().Perm(); got != 0o644 { + t.Errorf("perm after forced overwrite: got %o, want %o", got, 0o644) + } +} + +// TestRun_RejectsNonexistentRoot covers task 6.3: a nonexistent TargetDir is +// rejected and nothing is written. +func TestRun_RejectsNonexistentRoot(t *testing.T) { + t.Parallel() + parent := t.TempDir() + missing := filepath.Join(parent, "does-not-exist") + + res, err := Run(Options{TargetDir: missing}) + if err == nil { + t.Fatalf("expected error for nonexistent root, got nil (res=%v)", res) + } + if _, statErr := os.Stat(filepath.Join(missing, ".opencode")); !os.IsNotExist(statErr) { + t.Errorf("expected nothing written under a nonexistent root") + } +} + +// TestRun_RejectsTraversalPath covers task 6.3: a TargetDir containing a ".." +// segment is rejected before any write. +func TestRun_RejectsTraversalPath(t *testing.T) { + t.Parallel() + dir := t.TempDir() + sep := string(filepath.Separator) + traversal := dir + sep + ".." + sep + "escape" + + if _, err := Run(Options{TargetDir: traversal}); err == nil { + t.Fatalf("expected error for traversal path %q, got nil", traversal) + } +} + +// TestRun_RejectsSymlinkedAgentsDir covers task 6.3: a symlinked +// .opencode/agents that resolves outside the validated root is rejected and no +// asset leaks to the symlink target. +func TestRun_RejectsSymlinkedAgentsDir(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("symlink semantics are unreliable on Windows") + } + root := t.TempDir() + outside := t.TempDir() + + if err := os.Mkdir(filepath.Join(root, ".opencode"), 0o755); err != nil { + t.Fatalf("mkdir .opencode: %v", err) + } + if err := os.Symlink(outside, filepath.Join(root, ".opencode", "agents")); err != nil { + t.Fatalf("create symlink: %v", err) + } + + if _, err := Run(Options{TargetDir: root}); err == nil { + t.Fatalf("expected error when .opencode/agents is a symlink, got nil") + } + if _, err := os.Stat(filepath.Join(outside, deployedAssetName)); !os.IsNotExist(err) { + t.Errorf("asset leaked outside the root via symlink: %v", err) + } +} + +// TestRun_WriteFileError covers the injectable WriteFile seam (task 5.2): an +// I/O failure surfaces as a wrapped error from Run. +func TestRun_WriteFileError(t *testing.T) { + t.Parallel() + dir := t.TempDir() + sentinel := errors.New("disk full") + + _, err := Run(Options{ + TargetDir: dir, + WriteFile: func(string, []byte, fs.FileMode) error { return sentinel }, + }) + if err == nil { + t.Fatalf("expected error from failing WriteFile, got nil") + } + if !errors.Is(err, sentinel) { + t.Errorf("error chain: got %v, want wrapped %v", err, sentinel) + } +} + +// orderedGlobFS wraps an fs.FS and returns a fixed, caller-controlled Glob +// ordering so tests can prove run sorts its results independent of walk order. +type orderedGlobFS struct { + fs.FS + globResult []string +} + +func (o orderedGlobFS) Glob(string) ([]string, error) { + return o.globResult, nil +} + +// TestRun_ResultsAreSorted covers task 6.5: over a synthetic multi-entry fs.FS +// presented in non-alphabetical order, run returns Written/Skipped/Forced in +// stable ascending order. +func TestRun_ResultsAreSorted(t *testing.T) { + t.Parallel() + base := fstest.MapFS{ + "assets/agents/charlie.md": {Data: []byte("charlie")}, + "assets/agents/alpha.md": {Data: []byte("alpha")}, + "assets/agents/bravo.md": {Data: []byte("bravo")}, + } + assets := orderedGlobFS{ + FS: base, + globResult: []string{ + "assets/agents/charlie.md", + "assets/agents/bravo.md", + "assets/agents/alpha.md", + }, + } + want := []string{"alpha.md", "bravo.md", "charlie.md"} + dir := t.TempDir() + + res, err := run(assets, Options{TargetDir: dir}) + if err != nil { + t.Fatalf("run (write): %v", err) + } + if !slices.Equal(res.Written, want) { + t.Errorf("Written not sorted: got %v, want %v", res.Written, want) + } + + res2, err := run(assets, Options{TargetDir: dir}) + if err != nil { + t.Fatalf("run (skip): %v", err) + } + if !slices.Equal(res2.Skipped, want) { + t.Errorf("Skipped not sorted: got %v, want %v", res2.Skipped, want) + } + + res3, err := run(assets, Options{TargetDir: dir, Force: true}) + if err != nil { + t.Fatalf("run (force): %v", err) + } + if !slices.Equal(res3.Forced, want) { + t.Errorf("Forced not sorted: got %v, want %v", res3.Forced, want) + } +} + +// TestEmbeddedAsset_Contract covers task 6.4: the embedded divisor-entropy.md +// carries the required frontmatter, provenance marker, sections, and a bash +// allowlist that is exactly the nine permitted commands. +func TestEmbeddedAsset_Contract(t *testing.T) { + t.Parallel() + data, err := fs.ReadFile(assetsFS, embeddedAssetPath) + if err != nil { + t.Fatalf("read embedded asset: %v", err) + } + content := string(data) + + if ok, _ := path.Match("divisor-*.md", deployedAssetName); !ok { + t.Errorf("asset name %q does not match divisor-*.md glob", deployedAssetName) + } + matches, err := fs.Glob(assetsFS, "assets/agents/divisor-*.md") + if err != nil { + t.Fatalf("glob embedded assets: %v", err) + } + if !slices.Contains(matches, embeddedAssetPath) { + t.Errorf("embedded assets missing %q; got %v", embeddedAssetPath, matches) + } + + for _, needle := range []string{ + "mode: subagent", + "temperature: 0.1", + "edit: deny", + "webfetch: deny", + `"*": "deny"`, + } { + if !strings.Contains(content, needle) { + t.Errorf("frontmatter missing %q", needle) + } + } + + if frontmatterDescription(content) == "" { + t.Errorf("frontmatter description is empty or missing") + } + + if !strings.Contains(content, "`), so a regenerated asset is byte-identical to +the committed copy and the drift check is deterministic across binary versions — +the marker carries no version to normalize, and `scaffold.Options` has no +`Version` field and performs no version substitution. + +### D9: Agent output contract + +**Decision**: In Code Review Mode the agent MUST output: +- the base ref (merge-base SHA) and PR ref (HEAD SHA) compared; +- a per-package delta table (Ca, Ce, Instability, Abstractness, Distance, LCOM: base → PR, Δ), + sourced from the `vibe-check diff` JSON output; +- the list of newly introduced cycles (if any), sourced from the `vibe-check diff` + JSON output; +- an overall entropy direction (improving / stable / degrading), sourced from the + `vibe-check diff` JSON output; +- findings in the standard divisor block + (`### [SEVERITY] Title`, **File**, **Description**, **Recommendation**; + severity per `.opencode/uf/packs/severity.md`); +- a domain Score (1–10) and a final verdict line + (`APPROVE` / `REQUEST CHANGES` / `COMMENT`), where the verdict is the Go-computed + result reported by `vibe-check diff`. + +**Rationale**: Matches the other divisors' output format and makes the delta +sourced from Go-computed, auditable output (Constitution III observability, VI fidelity). + +### D10: Graceful degradation + +**Decision**: If the `vibe-check` binary is not on `PATH`, the base ref cannot be +resolved or analyzed, `git worktree` fails, or EITHER the base OR the PR ref fails +to analyze (e.g., the PR does not build), the agent MUST report the limitation and +return **COMMENT** — never a false APPROVE and never a hard crash. The agent MUST +distinguish "PR does not build" from a clean measurement so reviewers understand +why full delta data is unavailable. + +**Rationale**: Constitution II (composability): the reviewer must not block a PR +because of its own tooling gap, and must not silently approve when it could not +measure. + +## Coverage Strategy + +**Unit tests — `internal/scaffold/` (target ≥ 80% line coverage)**: Using +`t.TempDir()`, verify: assets are written to `.opencode/agents/` with mode +`0o644` and created dirs with mode `0o755`; nested directory creation; default +skip-existing behavior (unchanged file, reported as skipped); `--force` +overwrites; embedded `divisor-entropy.md` is non-empty; `metrics.ValidateProjectPath` +rejects traversal (`../`) targets. `Result` array ordering MUST be asserted +deterministic (stable, sorted). + +**Embedded-asset contract test**: Parse the embedded `divisor-entropy.md` and +assert: +- `description` is non-empty; +- frontmatter contains `mode: subagent`, `temperature: 0.1`, `edit: deny`, + `webfetch: deny`; +- a granular `bash` block whose catch-all `"*"` is `deny`; +- the allowlist is **exactly** (set-equality — failing on any missing OR extra + entry): `git merge-base *`, `git rev-parse *`, `git worktree add *`, + `git worktree remove *`, `git worktree prune`, `git fetch origin *`, + `git check-ref-format *`, `vibe-check analyze *`, and `vibe-check diff *`; +- the AP-006 provenance marker prefix `` (a stable, version-less marker) + immediately after the frontmatter, then write the agent + body: `# Role`, an optional `## Step 0: Prior Learnings` (Dewey-optional), + and `## Source Documents` (AGENTS.md, constitution, + `.opencode/uf/packs/severity.md`, other packs) matching the existing divisor + structure. +- [x] 4.3 Write `## Code Review Mode` describing the delta workflow, ordering + ref validation so the regex gate runs FIRST: reject any base ref that does + not match `^[A-Za-z0-9._/-]+$` (the primary metacharacter defense, applied + before the ref reaches any command), THEN run `git check-ref-format` for + ref-semantic validation (rejecting `..`, `@{`, leading `-`, `.lock`), THEN + resolve it with `git rev-parse --verify --end-of-options "^{commit}"` + (so a ref can never be parsed as a flag or chained command). Default the base + to `git merge-base HEAD origin/main`; if the base ref must be fetched, run + `git fetch origin ` only after that same regex has validated the ref + (note `git fetch` takes no `--end-of-options`, so the regex is the + load-bearing pre-fetch guard). Create an isolated `git worktree` at the + resolved SHA in a unique temp directory outside the repo (running + `git worktree prune` first to clear any stale entry), run + `vibe-check analyze --output ` (NOT gaze/goda) with a bounded + `--timeout` to materialize a JSON graph for both the base worktree and the PR + checkout using the same binary (using the `--output` flag rather than shell + redirection, which the allowlist forbids), run `vibe-check diff` on the two + resulting JSON graph files, then remove the worktree with + `git worktree remove --force`. Satisfies "Structural Delta Computation" + and "Base Branch Isolation". +- [x] 4.4 Document that the deterministic verdict is computed by + `vibe-check diff` (the tested Go `DecideVerdict` gates: new cycle → REQUEST + CHANGES; ΔI ≥ 0.15 → REQUEST CHANGES; ΔD ≥ 0.20 → REQUEST CHANGES; + ΔLCOM ≥ 2 → REQUEST CHANGES; smaller shifts → COMMENT; improve/stable → + APPROVE) — the agent reports and explains the tool's verdict rather than + computing thresholds in-prompt. Satisfies "New Circular Dependency + Detection" and "Deterministic Verdict Thresholds". +- [x] 4.5 Write `## Out of Scope` (delegating other personas' domains) and the + `## Output Format` requiring base+PR SHAs, the per-package delta table + (Ca, Ce, Instability, Abstractness, Distance, LCOM: base → PR, Δ) sourced + from the `vibe-check diff` JSON, the new-cycle list, entropy direction, + standard `### [SEVERITY]` finding blocks (per `severity.md`), a domain Score + (1–10), and a final verdict line. Satisfies "Auditable Delta Report". +- [x] 4.6 Write `## Decision Criteria` and a graceful-degradation clause: + return COMMENT (never a false APPROVE, never a crash) when the binary is + missing, the base ref cannot be resolved/analyzed, the PR ref fails to + analyze, worktree creation fails, or `vibe-check diff` cannot produce a + verdict. Satisfies "Graceful Degradation". +- [x] 4.7 Write a `## Security / Operating Constraints` section in the asset + documenting that `vibe-check analyze` executes the target's build tooling + (compilation and cgo) — a residual code-execution surface that + `GOTOOLCHAIN=local` does NOT close (it only closes the toolchain-directive + download vector) — and that the divisor therefore MUST only run on refs the + CI context already trusts and MUST NOT be wired into untrusted-fork-PR CI. + Satisfies "Trusted-Refs-Only Operating Constraint". + +## 5. Scaffold Writer Logic + +- [x] 5.1 In `internal/scaffold/scaffold.go`, define `Options{ TargetDir + string; Force bool }` and `Result{ Written []string; Skipped []string; + Forced []string }` with GoDoc on every exported symbol. +- [x] 5.2 Implement `Run(opts Options) (*Result, error)` (named `Run`, not + `Scaffold`, to avoid the package/function stutter and follow the AP-002 + canonical entry-point name): validate `TargetDir` using + `metrics.ValidateProjectPath` (which requires an existing directory and + rejects traversal); resolve symlinks using a deepest-existing-ancestor + strategy — starting from the already-validated root, walk toward the + destination finding the deepest directory that actually exists, then create + each new component incrementally and verify containment after creation (or + use `O_NOFOLLOW` semantics) so a symlink placed at `.opencode/` or + `.opencode/agents/` cannot redirect writes outside the validated root; add an + injectable write/filesystem seam to `Options` (e.g. `WriteFile func(path + string, data []byte, perm fs.FileMode) error`) so the I/O-failure exit-2 + branch is testable unconditionally without requiring root access; walk + `assetsFS`; create the `.opencode/agents/` tree with mode `0o755`; write + each asset with mode `0o644`. Satisfies init-command "Deploy Embedded Agent + Assets", "Target Path Validation", and "Safe File Permissions". +- [x] 5.3 Implement skip-existing default (record in `Skipped`) and `Force` + overwrite (record in `Forced`); on overwrite, normalize the destination file + mode to `0o644` (e.g. `O_TRUNC` write followed by `Chmod`) so a pre-existing + loose mode cannot persist; wrap I/O errors with + `fmt.Errorf("context: %w", err)` naming the operation and path. `Result` + slices MUST be returned in stable, sorted order. Satisfies "Idempotent + Skip-Existing Behavior" and "Force Overwrite". + +## 6. Scaffold Package Tests + +- [x] 6.1 [P] Add `internal/scaffold/scaffold_test.go` using `t.TempDir()`: + assert assets land in `.opencode/agents/`, directory mode is `0o755`, file + mode is `0o644`, nested dirs are created, default run skips an existing file + (reported in `Skipped`), and `Force` overwrites (reported in `Forced`). +- [x] 6.2 [P] Add a `Force`-overwrite permission test: pre-create the target + file with a loose mode (e.g. `0o666`), run with `Force`, and assert the + resulting file mode is normalized to `0o644`. +- [x] 6.3 [P] Add a path-traversal / nonexistent-root rejection test (a + `..`-escaping or nonexistent `TargetDir` returns an error and writes + nothing) and a symlink-escape test (a `.opencode/agents` resolving outside + the root is rejected), guarding the symlink case with a `runtime.GOOS` check + / `t.Skip` on platforms without reliable symlink support. +- [x] 6.4 [P] Add an embedded-asset contract test: parse the embedded + `divisor-entropy.md` frontmatter and assert `description` is non-empty, + `mode: subagent`, `temperature: 0.1`, `edit: deny`, `webfetch: deny`, and a + granular `bash` block whose catch-all is `deny` and whose allowlist is + **exactly** (set-equality — failing on any missing *or* extra entry) the nine + entries `git merge-base *`, `git rev-parse *`, `git worktree add *`, + `git worktree remove *`, `git worktree prune`, `git fetch origin *`, + `git check-ref-format *`, `vibe-check analyze *`, and `vibe-check diff *`; + assert the AP-006 provenance marker prefix ` +