feat: add voice report schema preflight CLI#807
Conversation
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge; the new command is purely local and additive, with no effect on existing commands or API calls. The implementation and schemas are solid. Two tests reach outside the CLI module via relative paths into backend testdata and docs/, making them fragile under directory restructuring or isolated test runs. The structured JSON output (--json) also never emits valid: false — failures produce only a non-zero exit code, which can surprise automated consumers expecting parseable output in all cases. cli/cmd/artifact_voice_report_test.go — the two fixture-dependent tests warrant a second look for cross-module path stability.
|
| Filename | Overview |
|---|---|
| cli/cmd/artifact_voice_report.go | New command implementation that reads a JSON voice report, auto-detects or accepts an explicit schema, and validates it via the google/jsonschema-go library. Logic is sound; the main concern is that valid: false is never emitted in structured JSON output on failure. |
| cli/cmd/artifact_voice_report_test.go | Four tests covering auto-detect, explicit schema, invalid payload, and unknown type. Two tests use relative paths reaching outside the CLI module into backend testdata and docs/, creating a fragile cross-module dependency. |
| cli/cmd/voice_schemas/voice-live-continuity-report.schema.json | Embedded JSON Schema for live-continuity reports. Uses oneOf to enforce status/passed/evidence_status consistency. Required fields and enum constraints look correct. |
| cli/cmd/voice_schemas/voice-source-separation-report.schema.json | Embedded schema for source-separation reports. oneOf enforces that status=passed ↔ passed=true and status∈{failed,degraded} ↔ passed=false. No "warn" status, consistent with the top-level enum. |
| cli/cmd/voice_schemas/voice-video-sync-report.schema.json | Embedded schema for video-sync reports. type field is optional (omitted-type reports are supported via --schema). allOf conditional enforcement for paired vs missing_translation segments looks correct. Matches the docs/schemas copy exactly. |
| cli/go.mod | Promotes github.com/google/jsonschema-go to a direct dependency and github.com/spf13/pflag from indirect to direct. Both moves are correct given the new code's direct use. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[agentclash artifact validate-voice-report file] --> B{--schema flag?}
B -- yes --> C[readJSONSchema from disk path]
B -- no --> D[readJSONDocument]
D --> E{report.type?}
E -- live_continuity --> F[voice-live-continuity-report.schema.json]
E -- video_sync --> G[voice-video-sync-report.schema.json]
E -- source_separation --> H[voice-source-separation-report.schema.json]
E -- empty --> X1[error: pass --schema]
E -- unknown --> X2[error: unsupported type]
F --> I[readJSONSchema from embed.FS]
G --> I
H --> I
I --> J[schema.Resolve nil]
C --> J
J --> K[schema.Validate report]
K -- pass --> L[print valid:true result]
K -- fail --> M[return error: voice report schema validation failed]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
cli/cmd/artifact_voice_report_test.go:11-46
**Cross-module test fixture dependency**
`TestArtifactValidateVoiceReportAutoDetectsLiveContinuity` and `TestArtifactValidateVoiceReportAcceptsExplicitSchemaForOmittedType` both reach outside the CLI module with relative paths (`../../backend/internal/voiceartifacts/testdata/` and `../../docs/schemas/`). These tests will silently break if the backend testdata is reorganised, if the CLI module is extracted to a separate repo, or if `go test` is invoked from a working directory other than `cli/cmd`. The two pure-failure tests avoid this by writing temp files in-process; the same approach would make the happy-path tests self-contained.
### Issue 2 of 2
cli/cmd/artifact_voice_report.go:93-100
**`valid: false` is never emitted in structured JSON output**
When schema validation fails, `validateVoiceReportFile` returns an error and the zero-value `voiceReportValidationResult{}` (which has `Valid: false`) is discarded. The command's `RunE` returns the error directly, so with `--json` a caller receives no output at all on failure — only a non-zero exit code. A script that runs `agentclash artifact validate-voice-report --json report.json` and tries to parse stdout will get an empty buffer on the failing path. Emitting `{"valid": false, "errors": [...]}` on failure, or at minimum consistently emitting the result struct before returning a non-zero exit, would make the structured mode reliable for automation.
Reviews (1): Last reviewed commit: "feat: add voice report schema preflight" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func TestArtifactValidateVoiceReportAutoDetectsLiveContinuity(t *testing.T) { | ||
| stdout := captureStdout(t) | ||
| err := executeCommand(t, []string{ | ||
| "--json", | ||
| "artifact", | ||
| "validate-voice-report", | ||
| filepath.Join("..", "..", "backend", "internal", "voiceartifacts", "testdata", "voicey_live_continuity_report.json"), | ||
| }, "http://unused") | ||
| if err != nil { | ||
| t.Fatalf("validate voice report error: %v", err) | ||
| } | ||
|
|
||
| var payload map[string]any | ||
| if err := json.Unmarshal([]byte(stdout.finish()), &payload); err != nil { | ||
| t.Fatalf("output is not JSON: %v", err) | ||
| } | ||
| if payload["valid"] != true { | ||
| t.Fatalf("valid = %v, want true", payload["valid"]) | ||
| } | ||
| if !strings.Contains(str(payload["schema"]), voiceSchemaLiveContinuityFile) { | ||
| t.Fatalf("schema = %v, want live continuity schema", payload["schema"]) | ||
| } | ||
| } | ||
|
|
||
| func TestArtifactValidateVoiceReportAcceptsExplicitSchemaForOmittedType(t *testing.T) { | ||
| err := executeCommand(t, []string{ | ||
| "artifact", | ||
| "validate-voice-report", | ||
| filepath.Join("..", "..", "backend", "internal", "voiceartifacts", "testdata", "voicey_video_sync_report.json"), | ||
| "--schema", | ||
| filepath.Join("..", "..", "docs", "schemas", voiceSchemaVideoSyncFile), | ||
| }, "http://unused") | ||
| if err != nil { | ||
| t.Fatalf("validate voice report with explicit schema error: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestArtifactValidateVoiceReportAutoDetectsLiveContinuity and TestArtifactValidateVoiceReportAcceptsExplicitSchemaForOmittedType both reach outside the CLI module with relative paths (../../backend/internal/voiceartifacts/testdata/ and ../../docs/schemas/). These tests will silently break if the backend testdata is reorganised, if the CLI module is extracted to a separate repo, or if go test is invoked from a working directory other than cli/cmd. The two pure-failure tests avoid this by writing temp files in-process; the same approach would make the happy-path tests self-contained.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/artifact_voice_report_test.go
Line: 11-46
Comment:
**Cross-module test fixture dependency**
`TestArtifactValidateVoiceReportAutoDetectsLiveContinuity` and `TestArtifactValidateVoiceReportAcceptsExplicitSchemaForOmittedType` both reach outside the CLI module with relative paths (`../../backend/internal/voiceartifacts/testdata/` and `../../docs/schemas/`). These tests will silently break if the backend testdata is reorganised, if the CLI module is extracted to a separate repo, or if `go test` is invoked from a working directory other than `cli/cmd`. The two pure-failure tests avoid this by writing temp files in-process; the same approach would make the happy-path tests self-contained.
How can I resolve this? If you propose a fix, please make it concise.
| return "", fmt.Errorf("voice report must be a JSON object") | ||
| } | ||
| reportType, _ := object["type"].(string) | ||
| reportType = strings.TrimSpace(reportType) | ||
|
|
||
| var schemaFile string | ||
| switch reportType { | ||
| case "agentclash.voice.live_continuity_eval.v1", "voicey.live_continuity_eval": |
There was a problem hiding this comment.
valid: false is never emitted in structured JSON output
When schema validation fails, validateVoiceReportFile returns an error and the zero-value voiceReportValidationResult{} (which has Valid: false) is discarded. The command's RunE returns the error directly, so with --json a caller receives no output at all on failure — only a non-zero exit code. A script that runs agentclash artifact validate-voice-report --json report.json and tries to parse stdout will get an empty buffer on the failing path. Emitting {"valid": false, "errors": [...]} on failure, or at minimum consistently emitting the result struct before returning a non-zero exit, would make the structured mode reliable for automation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/artifact_voice_report.go
Line: 93-100
Comment:
**`valid: false` is never emitted in structured JSON output**
When schema validation fails, `validateVoiceReportFile` returns an error and the zero-value `voiceReportValidationResult{}` (which has `Valid: false`) is discarded. The command's `RunE` returns the error directly, so with `--json` a caller receives no output at all on failure — only a non-zero exit code. A script that runs `agentclash artifact validate-voice-report --json report.json` and tries to parse stdout will get an empty buffer on the failing path. Emitting `{"valid": false, "errors": [...]}` on failure, or at minimum consistently emitting the result struct before returning a non-zero exit, would make the structured mode reliable for automation.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Closes #806
Summary
agentclash artifact validate-voice-report <file>for local voice report schema preflight--schema <path>for explicit validation, including omitted-type video-sync reportsTests
Review-checkpoint: testing/codex-voice-schema-cli.md