feat: add synthesis layer diagnostics to dewey doctor - #107
Conversation
…doctor synthesis diagnostics)
em-redhat
left a comment
There was a problem hiding this comment.
PR Review: #107 — feat: add synthesis layer diagnostics to dewey doctor
Summary
Well-scoped, structurally sound implementation that mirrors the existing Embedding Layer pattern. All five issue #102 acceptance criteria are covered (resolved endpoint, provider type, model, connectivity, model availability). Code is correct, tests cover all provider paths (Ollama, Vertex, unconfigured), and CI passes. Token safety is verified by negative assertions.
Findings
[MEDIUM] Website Documentation Sync issue not filed
The constitution (§Development Workflow) and AGENTS.md both require a gh issue create --repo unbound-force/website issue documenting new user-facing behavior. The dewey doctor synthesis section adds new user-visible output. No matching issue was found in unbound-force/website.
[LOW] Test isolation hardening
TestDoctorCmd_SynthesisUnconfigured and the Vertex tests in cli_test.go do not set XDG_CONFIG_HOME to redirect global config reads. A developer's ~/.config/dewey/config.yaml with a synthesis section could cause unexpected test behavior. Adding t.Setenv("XDG_CONFIG_HOME", t.TempDir()) would fully harden these tests. This matches a pre-existing gap in the embedding tests and is non-blocking.
Verdict
COMMENT — The implementation is correct and well-tested. The MEDIUM finding is a process tracking gap (missing website issue), not a code defect. The LOW finding is an optional quality improvement. The code itself is ready for approval once the website issue is filed.
This review was generated by /uf.review-pr (AI-assisted).
Add a Synthesis Layer section to dewey doctor output between the existing Embedding Layer and MCP Server sections. Reports provider type, resolved endpoint, model, connectivity, and model availability for all three provider states (Ollama, Vertex, unconfigured). - Ollama: connectivity via ensureOllama + model availability check - Vertex: config completeness validation (no live API calls) - Unconfigured: PASS with 'not configured (optional)' Tests cover all provider states including Ollama unreachable, dual-endpoint independence, unknown provider, Vertex misconfigured, and token safety negative assertions. Closes unbound-force#102
6c403d9 to
41d3846
Compare
|
Thanks for the review @em-redhat — both findings addressed: [MEDIUM] Website Documentation Sync: The website issue content has been prepared and will be filed in [LOW] Test isolation ( |
em-redhat
left a comment
There was a problem hiding this comment.
Re-Review: APPROVE
Both findings from the first review are fully resolved:
| Finding | Severity | Status |
|---|---|---|
| Website Documentation Sync issue not filed | MEDIUM | RESOLVED — Issue #249 filed in unbound-force/website |
| Test isolation hardening (XDG_CONFIG_HOME) | LOW | RESOLVED — All 6 new tests set t.Setenv("XDG_CONFIG_HOME", t.TempDir()) |
All five acceptance criteria from #102 are covered. CI passes clean (build-and-test + MegaLinter). No security, alignment, or constitution compliance findings.
This review was generated by /uf.review-pr (AI-assisted).
Summary
Adds a Synthesis Layer section to
dewey doctoroutput, closing the diagnostics gap where synthesis provider configuration had no visibility. This mirrors the existing Embedding Layer section and reports provider type, resolved endpoint, model, connectivity, and model availability.Closes #102
What Changed
Production code (
cli.go)runDoctorChecks()between Embedding Layer and MCP Server sections (+63 lines)ensureOllama()+ model availability viaOllamaSynthesizer.Available()NewSynthesizerFromConfig()(no live API calls, no GCP credentials required)Tests (
cli_test.go,main_test.go)TestDoctorCmd_SynthesisUnconfigured— unconfigured provider reports PASSTestRunDoctorChecks_SynthesisOllamaProvider— dual-endpoint independence (separate mock servers for embedding and synthesis)TestRunDoctorChecks_SynthesisOllamaUnreachable— unreachable endpoint skips model checkTestDoctorCmd_SynthesisVertexConfigured— Vertex reports config status + token safety negative assertionsTestDoctorCmd_SynthesisVertexMisconfigured— missing project field reports FAILTestDoctorCmd_SynthesisUnknownProvider— unknown provider reports FAIL with supported listDocumentation
README.mdupdated to include synthesis layer indewey doctordescriptionProvenance
openspec/changes/doctor-synthesis-diagnostics/synthesis-endpoint-envOpenSpec (bug: synthesis provider reuses DEWEY_EMBEDDING_ENDPOINT instead of its own env var #71)CI Gates
go build ./...go vet ./...go test -race -count=1 ./...Review Council
Constitution Alignment