feat: add divisor-entropy Review Council agent with vibe-check init and diff - #26
Conversation
…nd diff 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 <base.json> <pr.json>` 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)
jflowers
left a comment
There was a problem hiding this comment.
Council Verdict: APPROVE
Note: Could not post as APPROVE due to GitHub's self-review prohibition. Posted as COMMENT instead. Original verdict: APPROVE.
Reviewers: Adversary, Architect, Curator, Entropy (proxy), Envoy, Guard, Herald, Scribe, SRE, Testing
Iterations: 1 (unanimous APPROVE on first pass)
Adversary (APPROVE)
- [MEDIUM]
analyze --outputpath accepts arbitrary file paths without validation (defense-in-depth gap; user has shell access, so blast radius is limited). See inline comment. - 3 LOW findings omitted.
Architect (APPROVE — Score: 9/10)
- 4 LOW findings omitted (duplicate fixture helpers,
writeListSectioncoupling,normFloatnaming,main.gopackage doc).
Guard (APPROVE)
- [LOW] CHANGELOG entries missing
Spec:path references. See inline comment. - Systematic spec-to-code alignment verified across all 4 specs — every requirement maps to tested code.
Testing (APPROVE)
- [MEDIUM] Missing test for
tightenThresholdswith multiple simultaneous overrides. See inline comment. - [MEDIUM]
TestRunDiff_PartialBuildSuppressesAddedRemoveduses raw JSON inspection instead of typed struct. See inline comment. - [MEDIUM] No direct test for
normFloatedge case with negative zero. See inline comment. - 2 LOW findings omitted.
SRE (APPROVE)
- 2 LOW findings omitted (package GoDoc, analyze help text).
Curator (APPROVE)
- [MEDIUM] Blog issue filed as #27 during review.
- 1 LOW finding omitted.
Envoy (APPROVE — Score: 8/10)
- [MEDIUM] Unsubstantiated competitive claim persists in
AGENTS.md:9(pre-existing, propagation risk for external comms). - [MEDIUM] README lacks benefit statement in opening paragraph (pre-existing).
- 1 LOW finding omitted.
Herald (APPROVE)
- [LOW] CHANGELOG
divisor-entropyentry describes mechanism, not benefit (advisory for release notes). - Blog post opportunity identified.
Scribe (APPROVE — Score: 9/10)
- [LOW] Missing fenced-code-block language identifiers in fixture README. See inline comment.
- 2 LOW findings omitted.
Entropy Self-Review (APPROVE — Score: 9/10)
- [LOW] Verdict string mismatch:
REQUEST CHANGES(space) vsREQUEST_CHANGES(underscore) in gate description. See inline comment. - Embedded copy byte-identical to dogfooded copy (sha256 confirmed).
Consolidated Findings
main.gopackage doc staleness (Architect + SRE): same file, same fix — stays LOW.- CHANGELOG missing
Spec:paths (Guard + Scribe): same file, same fix — stays LOW.
This review was generated by /review-council (AI-assisted).
| // 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 { |
There was a problem hiding this comment.
[MEDIUM] Defense-in-depth: --output path accepts arbitrary file paths without validation (Adversary)
Unlike the init command (which calls ValidateProjectPath and ensureDir with symlink and traversal checks), the analyze --output path performs no validation before os.WriteFile. The blast radius is limited — the user who runs vibe-check already has shell access — but adding basic path validation (reject .. traversal components) would be consistent with the project's SC-004 convention.
| t.Errorf("verdict: got %v, want %v", result.Verdict, metrics.VerdictComment) | ||
| } | ||
|
|
||
| var raw map[string]json.RawMessage |
There was a problem hiding this comment.
[MEDIUM] Fragile assertion: raw JSON inspection instead of typed struct (Testing)
This test unmarshals into map[string]json.RawMessage to check for the absence of "added" and "removed" keys. This is correct for validating omitempty behavior, but fragile — if the struct tags change from omitempty to always-present empty arrays, this test breaks even though the semantic behavior is preserved. Consider adding a comment explaining that this specifically validates the omitempty serialization contract (not just the empty-list invariant, which is already verified via the typed struct on lines 438–440).
|
|
||
| // --- Task 3.4: tighten-only override enforcement ----------------------------- | ||
|
|
||
| func TestTightenThresholds(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM] Missing combined-override test for tightenThresholds (Testing)
The table tests each override individually but never tests applying multiple tighten overrides simultaneously. Consider adding a case like {name: "all_tighter", instability: float64Ptr(0.10), distance: float64Ptr(0.10), lcom: intPtr(1), want: VerdictThresholds{0.10, 0.10, 1}} to lock down the composition behavior.
| // 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 { |
There was a problem hiding this comment.
[MEDIUM] normFloat lacks a direct unit test (Testing)
This function exists specifically to normalize -0.0 to 0.0 for deterministic table output. While TestRunDiff_Deterministic exercises it indirectly, a direct test like TestNormFloat with math.Copysign(0, -1) would serve as a regression lock and document the intent.
|
|
||
| ### Added | ||
|
|
||
| - `vibe-check diff <base.json> <pr.json>` compares two ModuleGraph JSON |
There was a problem hiding this comment.
[LOW] Missing Spec: path references (Guard + Scribe — consolidated)
CHANGELOG entries should include Spec: references for traceability. The canonical specs for these entries live at:
openspec/changes/add-divisor-entropy-agent/specs/diff-command/spec.mdopenspec/changes/add-divisor-entropy-agent/specs/init-command/spec.mdopenspec/changes/add-divisor-entropy-agent/specs/divisor-entropy-agent/spec.mdopenspec/changes/add-divisor-entropy-agent/specs/analyze-command/spec.md
| - any package's ΔLCOM ≥ 2 → **REQUEST CHANGES**; | ||
| - smaller non-zero shifts that cross no threshold → **COMMENT**; | ||
| - metrics improve or stay stable → **APPROVE**. | ||
|
|
There was a problem hiding this comment.
[LOW] Verdict string mismatch: REQUEST CHANGES vs REQUEST_CHANGES (Entropy self-review)
Lines 142–145 use **REQUEST CHANGES** (space) while the Go constant VerdictRequestChanges and --json output use REQUEST_CHANGES (underscore). The Decision Criteria section (line 209) already uses the correct REQUEST_CHANGES form. Consider aligning for consistency with the wire format.
|
|
||
| Run from this directory (`metrics/testdata/entropy/`): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
[LOW] Missing fenced-code-block language identifiers (Scribe)
Five code blocks in this file use bare triple-backticks without language identifiers. Add bash or console for syntax highlighting and FT-001 compliance.
Summary
Adds the
divisor-entropyReview Council agent — a structural-qualityreviewer that measures the base→PR change in coupling, instability,
abstractness, distance, LCOM, and circular dependencies via
vibe-check analyze+vibe-check diff, enforcing the Boy Scout Rulefor architectural quality.
Key components:
metrics/delta.go,metrics/verdict.go):ComputeDeltacomputes per-module deltas with cycle classification(new/pre-existing/resolved);
DecideVerdictapplies protected gatethresholds (ΔI≥0.15, ΔD≥0.20, ΔLCOM≥2, new cycle → REQUEST_CHANGES;
smaller non-zero → COMMENT; improving/stable → APPROVE; partial-build
→ forced COMMENT, never a false APPROVE)
vibe-check diff <base.json> <pr.json>: CLI computing the entropydelta and verdict with human-table and
--jsonoutput, tighten-onlythreshold overrides, deterministic single-write stdout
vibe-check init [path]: deploys the embeddeddivisor-entropyagent asset to
.opencode/agents/(idempotent skip-existing,--force,--json, symlink-safe path validation)vibe-check analyze --output/-o: writes ModuleGraph JSON to afile instead of stdout (exit 2 on unwritable, no partial stdout)
GOTOOLCHAIN=local: analysis subprocess never downloads atoolchain named by the target module's go.mod (trade-off: trusted
modules needing a newer-than-local toolchain must be analyzed manually)
internal/scaffold/): embeds agent assets via//go:embed, writes to target with deepest-existing-ancestor symlinksafety, 0o755 dirs / 0o644 files
Provenance metadata (producer/version/timestamp) for
--jsonpayloadsis tracked as follow-up: #25.
Documentation issue for the website: unbound-force/website#278.
How to Test
See
metrics/testdata/entropy/README.mdfor the full validationchecklist (comment-band → COMMENT, partial-build → COMMENT).
How to Demo
go run ./cmd/vibe-check init .— observe the deployed agent assetgo run ./cmd/vibe-check diff metrics/testdata/entropy/improvement-base.json metrics/testdata/entropy/improvement-pr.json— observe APPROVE verdict with resolved cyclego run ./cmd/vibe-check diff metrics/testdata/entropy/degradation-base.json metrics/testdata/entropy/degradation-pr.json— observe REQUEST_CHANGES verdict with new cycle reasonKey Files Changed
cmd/vibe-check/internal/goadapter/internal/scaffold/metrics/metrics/testdata/entropy/.opencode/agents/This PR was generated by /uf.finale (AI-assisted).