Better error messages#104
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request implements improved error messages and debugging capabilities for the yapi CLI, addressing items from WISHLIST.md. The changes add bare variable syntax detection, verbose mode for chain execution, and example files demonstrating the new debugging features.
Changes:
- Added bare
$word.wordvariable syntax detection to warn users when they forget the required${...}braces - Implemented
--verboseflag foryapi runto show resolved request details and response bodies during chain execution - Created four example files in
examples/debugging/demonstrating error messages and debugging workflows
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/internal/vars/vars.go | Adds BareChainRef regex and FindBareRefs() function to detect bare variable references |
| cli/internal/validation/analyzer.go | Adds warnBareChainRefs() to emit warnings for bare variables during validation |
| cli/internal/runner/runner.go | Implements verbose logging for resolved requests and responses, adds runtime bare variable warnings |
| cli/cmd/yapi/run.go | Threads verbose flag from CLI to runner options |
| examples/debugging/*.yapi.yml | Four example files demonstrating debugging features and error messages |
| PLAN.md | Updates plan document to reflect completed work on error messages |
| WISHLIST.md | Adds wishlist documenting pain points that motivated these improvements |
| ## 3. Show resolved request details in verbose chain execution (WISHLIST #2) | ||
|
|
||
| `wait_for` runs first. Once `until` conditions pass, `expect` runs on the final response: | ||
| The problem: When a chain step fails, you can't see what values were actually sent because variable substitution is invisible. | ||
|
|
||
| ```yaml | ||
| wait_for: | ||
| until: | ||
| - .status != "pending" # Wait until not pending | ||
| period: 1s | ||
| timeout: 30s | ||
| **Changes:** | ||
|
|
||
| expect: | ||
| status: 200 | ||
| assert: | ||
| - .status == "completed" # Then verify it's completed (not failed) | ||
| ``` | ||
| - **`cli/internal/runner/runner.go`**: Add `Verbose bool` field to `runner.Options`. | ||
| - **`cli/internal/runner/runner.go`**: In `RunChain()`, after `interpolateConfig()` succeeds and before executing, if `opts.Verbose` is true, print the resolved config to stderr: | ||
| - Resolved URL (with method) | ||
| - Resolved headers | ||
| - Resolved body (JSON-serialized if map, or raw if string) | ||
| - Uses `fmt.Fprintf(os.Stderr, ...)` with `[VERBOSE]` prefix, consistent with the existing Logger pattern. | ||
| - **`cli/cmd/yapi/run.go`**: Set `opts.Verbose = ctx.verbose` when building runner.Options in `executeRunE()`. | ||
|
|
||
| ### With `timeout` | ||
|
|
||
| The existing `timeout` field is per-request. `wait_for.timeout` is total polling time: | ||
|
|
||
| ```yaml | ||
| timeout: 5s # Each poll attempt times out after 5s | ||
| --- | ||
|
|
||
| wait_for: | ||
| until: | ||
| - .ready == true | ||
| period: 2s | ||
| timeout: 60s # Total polling time limit | ||
| ``` | ||
| ## 4. Print step responses in verbose chain mode (WISHLIST #4) | ||
|
|
||
| ### With `delay` | ||
| The problem: In chain execution, you only see the final failing step's output, not intermediate step responses. | ||
|
|
||
| `delay` happens before `wait_for` starts: | ||
| **Changes:** | ||
|
|
||
| ```yaml | ||
| delay: 5s # Wait 5s before starting to poll | ||
| - **`cli/internal/runner/runner.go`**: In `RunChain()`, after each step executes, if `opts.Verbose` is true, print the step's response details to stderr: | ||
| - Status code | ||
| - Response body (truncated at 1000 chars for readability) | ||
| - Duration | ||
|
|
||
| wait_for: | ||
| until: | ||
| - .status == "done" | ||
| period: 2s | ||
| timeout: 30s | ||
| ``` | ||
| This replaces the need for a per-step `debug: true` field -- verbose mode shows everything, which is simpler and avoids new config surface area. | ||
|
|
||
| --- | ||
|
|
||
| ## Output During Polling | ||
| ## 5. Example files: `examples/debugging/` | ||
|
|
||
| When running with verbose/default output: | ||
| Create example `.yapi.yml` files that demonstrate the improved debugging experience: | ||
|
|
||
| ``` | ||
| [POLL] Attempt 1 - conditions not met, retrying in 2s... | ||
| [POLL] Attempt 2 - conditions not met, retrying in 2s... | ||
| [POLL] Attempt 3 - request failed (503), retrying in 2s... | ||
| [POLL] Attempt 4 - conditions met! | ||
| ``` | ||
| - **`bare-variable-warning.yapi.yml`**: A chain that uses `$step.field` (bare) to trigger the new warning. | ||
| - **`chain-verbose-demo.yapi.yml`**: A multi-step chain against jsonplaceholder with variables that shows how `--verbose` reveals resolved values. | ||
| - **`assertion-failure-demo.yapi.yml`**: A request with an `expect:` block that will fail, showing the detailed assertion error output. | ||
| - **`missing-key-demo.yapi.yml`**: A chain that references a nonexistent JSON key, showing the precise error path. | ||
|
|
||
| --- | ||
|
|
||
| ## Config Schema | ||
|
|
||
| ```go | ||
| type Backoff struct { | ||
| Seed string `yaml:"seed"` // Initial wait, e.g., "1s" | ||
| Multiplier float64 `yaml:"multiplier"` // e.g., 2 | ||
| } | ||
|
|
||
| type WaitFor struct { | ||
| Until []string `yaml:"until"` // Required: JQ assertions | ||
| Period string `yaml:"period,omitempty"` // Fixed interval, e.g., "2s" | ||
| Backoff *Backoff `yaml:"backoff,omitempty"` // Exponential backoff | ||
| Timeout string `yaml:"timeout"` // Required: total time limit | ||
| } | ||
| ``` | ||
|
|
||
| Added to `ConfigV1`: | ||
| ```go | ||
| type ConfigV1 struct { | ||
| // ... existing fields ... | ||
| WaitFor *WaitFor `yaml:"wait_for,omitempty"` | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
| ## Files changed | ||
|
|
||
| ## Validation Rules | ||
| | File | Change | | ||
| |------|--------| | ||
| | `WISHLIST.md` | Remove item #3 | | ||
| | `cli/internal/vars/vars.go` | Add `BareChainRef` regex + `FindBareRefs()` function | | ||
| | `cli/internal/validation/analyzer.go` | Add `warnBareChainRefs()`, call from `analyzeParsed()` | | ||
| | `cli/internal/runner/runner.go` | Add `Verbose` to `Options`, add verbose logging in `RunChain()` | | ||
| | `cli/cmd/yapi/run.go` | Thread `verbose` into `runner.Options` | | ||
| | `examples/debugging/*.yapi.yml` | 4 new example files | | ||
|
|
||
| 1. `until` is required and must have at least one assertion | ||
| 2. `timeout` is required and must be valid Go duration | ||
| 3. Exactly one of `period` OR `backoff` must be specified (mutually exclusive) | ||
| 4. If `period`: must be valid Go duration | ||
| 5. If `backoff`: `seed` must be valid Go duration, `multiplier` must be > 1 | ||
| 6. All `until` expressions must be valid JQ | ||
| **No new dependencies. No config schema changes. No breaking changes.** |
There was a problem hiding this comment.
Per the custom coding guideline (ID: 1000000), when adding new CLI features, documentation should be updated. The --verbose flag for yapi run is a new user-facing feature but isn't documented in README.md or SKILL.md. While the flag was already defined in the CLI, this PR implements its actual functionality for chain execution.
Consider adding a section in README.md or SKILL.md that explains yapi run --verbose for debugging chain execution, or at least mentioning it where chains are documented. The example file examples/debugging/chain-verbose-demo.yapi.yml provides good documentation through example, but users should be able to discover this feature through the main documentation as well.
| // warnBareChainRefs scans YAML text for bare $word.word patterns that look like | ||
| // chain variable references but aren't wrapped in ${...}, which means they won't | ||
| // be substituted. Emits a warning diagnostic for each occurrence. | ||
| func warnBareChainRefs(text string) []Diagnostic { | ||
| var diags []Diagnostic | ||
| lines := strings.Split(text, "\n") | ||
|
|
||
| for lineNum, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| // Skip comments and graphql blocks (which use $var syntax legitimately) | ||
| if strings.HasPrefix(trimmed, "#") { | ||
| continue | ||
| } | ||
|
|
||
| bareRefs := vars.FindBareRefs(line) | ||
| for _, ref := range bareRefs { | ||
| col := strings.Index(line, ref) | ||
| diags = append(diags, Diagnostic{ | ||
| Severity: SeverityWarning, | ||
| Field: "", | ||
| Message: fmt.Sprintf("possible bare variable reference '%s' -- did you mean '${%s}'? Only the ${...} form is substituted", ref, ref[1:]), | ||
| Line: lineNum, | ||
| Col: col, | ||
| }) | ||
| } | ||
| } | ||
| return diags | ||
| } |
There was a problem hiding this comment.
The warnBareChainRefs function will produce false positive warnings for GraphQL queries. GraphQL legitimately uses $variable syntax for query variables (e.g., query GetUser($id: ID!)). The comment on line 537 acknowledges this ("Skip comments and graphql blocks"), but the implementation only skips comment lines.
The existing FindEnvVarRefs function (lines 578-676) correctly handles GraphQL blocks by tracking inGraphQLBlock state and skipping lines within GraphQL query/mutation blocks. The same logic should be applied here to avoid warning users about valid GraphQL variable syntax.
| # yapi run bare-variable-warning.yapi.yml | ||
| # | ||
| # Expected output: | ||
| # [WARN] line 18: possible bare variable reference '$get_todo.userId' |
There was a problem hiding this comment.
The expected output in the comment states the warning will appear on "line 18", but the actual bare variable reference $get_todo.userId appears on line 25. The expected output should indicate line 25.
| # [WARN] line 18: possible bare variable reference '$get_todo.userId' | |
| # [WARN] line 25: possible bare variable reference '$get_todo.userId' |
| if len(body) > 1000 { | ||
| body = body[:1000] + fmt.Sprintf("... (%d bytes total)", len(body)) | ||
| } |
There was a problem hiding this comment.
The truncation logic uses byte-based slicing (body[:1000]) which could split multi-byte UTF-8 characters, resulting in invalid UTF-8 output. While this is a verbose/debug output and may not be critical, it could cause display issues or errors when the truncated string is printed. Consider using rune-based truncation or validating the truncation point to avoid splitting UTF-8 sequences.
| if len(body) > 1000 { | ||
| body = body[:1000] + fmt.Sprintf("... (%d bytes total)", len(body)) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] Body: %s\n", body) | ||
| } | ||
| } | ||
| if cfg.JSON != "" { | ||
| j := cfg.JSON | ||
| if len(j) > 1000 { | ||
| j = j[:1000] + fmt.Sprintf("... (%d bytes total)", len(j)) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] JSON: %s\n", j) | ||
| } | ||
| if cfg.Data != "" { | ||
| d := cfg.Data | ||
| if len(d) > 1000 { | ||
| d = d[:1000] + fmt.Sprintf("... (%d bytes total)", len(d)) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] Data: %s\n", d) | ||
| } | ||
| } | ||
|
|
||
| // logStepResponse prints response details to stderr for debugging. | ||
| func logStepResponse(stepNum int, stepName string, result *Result) { | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] Step %d (%s) response:\n", stepNum, stepName) | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] Status: %d\n", result.StatusCode) | ||
| fmt.Fprintf(os.Stderr, "[VERBOSE] Duration: %s\n", result.Duration) | ||
| body := result.Body | ||
| if len(body) > 1000 { | ||
| body = body[:1000] + fmt.Sprintf("... (%d bytes total)", len(body)) |
There was a problem hiding this comment.
The truncation logic is duplicated in lines 451-452, 458-459, and 471-472. This violates the DRY principle from the custom coding guidelines. Consider extracting this into a helper function like truncateString(s string, maxLen int) string to reduce duplication and make the code more maintainable.
| // warnBareChainRefs scans YAML text for bare $word.word patterns that look like | ||
| // chain variable references but aren't wrapped in ${...}, which means they won't | ||
| // be substituted. Emits a warning diagnostic for each occurrence. | ||
| func warnBareChainRefs(text string) []Diagnostic { | ||
| var diags []Diagnostic | ||
| lines := strings.Split(text, "\n") | ||
|
|
||
| for lineNum, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| // Skip comments and graphql blocks (which use $var syntax legitimately) | ||
| if strings.HasPrefix(trimmed, "#") { | ||
| continue | ||
| } | ||
|
|
||
| bareRefs := vars.FindBareRefs(line) | ||
| for _, ref := range bareRefs { | ||
| col := strings.Index(line, ref) | ||
| diags = append(diags, Diagnostic{ | ||
| Severity: SeverityWarning, | ||
| Field: "", | ||
| Message: fmt.Sprintf("possible bare variable reference '%s' -- did you mean '${%s}'? Only the ${...} form is substituted", ref, ref[1:]), | ||
| Line: lineNum, | ||
| Col: col, | ||
| }) | ||
| } | ||
| } | ||
| return diags | ||
| } |
There was a problem hiding this comment.
The warnBareChainRefs function lacks test coverage. Given that this is a new validation diagnostic that will be shown to users and has edge cases around GraphQL blocks and comment detection, it should have tests to verify it correctly identifies bare references while avoiding false positives.
| } | ||
| for k, v := range cfg.Query { | ||
| check(fmt.Sprintf("query '%s'", k), v) | ||
| } |
There was a problem hiding this comment.
The warnBareChainRefsInConfig function doesn't check the cfg.Graphql field for bare variable references. While GraphQL queries legitimately use $variable syntax for GraphQL variables and should NOT trigger warnings, the function should still check this field but skip GraphQL-specific $variable patterns. However, since GraphQL content uses $var legitimately, this field should probably be excluded from bare reference checking entirely.
| } | |
| } | |
| // Intentionally skip cfg.Graphql from bare reference checking: | |
| // GraphQL queries legitimately use $variable syntax, which should not trigger warnings. | |
| _ = cfg.Graphql |
| // BareChainRef matches bare $word.word patterns (without braces) that look like | ||
| // chain variable references. Negative lookbehind for { ensures we don't match ${...}. | ||
| // We use a two-step approach: find all $word.word patterns, then exclude those inside ${}. | ||
| var BareChainRef = regexp.MustCompile(`\$([A-Za-z_][A-Za-z0-9_]*\.[A-Za-z0-9_.]+)`) | ||
|
|
||
| // FindBareRefs returns bare chain-style references ($step.field) that are NOT | ||
| // wrapped in ${...}. These are likely user mistakes since only ${...} is substituted. | ||
| func FindBareRefs(s string) []string { | ||
| var results []string | ||
| for _, loc := range BareChainRef.FindAllStringIndex(s, -1) { | ||
| start := loc[0] | ||
| // Check that this is not inside ${...} by looking for a preceding { | ||
| if start > 0 && s[start-1] == '{' { | ||
| continue | ||
| } | ||
| results = append(results, s[start:loc[1]]) | ||
| } | ||
| return results | ||
| } |
There was a problem hiding this comment.
The new FindBareRefs function and BareChainRef regex lack test coverage. The repository has comprehensive test files (e.g., vars_test.go with 120 lines), but no tests were added for this new functionality. Given the complexity of the regex and the edge cases around GraphQL variables and proper ${} detection, this should have corresponding tests.
| if len(body) > 1000 { | ||
| body = body[:1000] + fmt.Sprintf("... (%d bytes total)", len(body)) |
There was a problem hiding this comment.
The byte count reported in the truncation message is incorrect. The code reports the length of the already-truncated string len(body) after setting body = body[:1000] + ..., which will be greater than 1000 due to the appended message. It should capture the original length before truncation. For example:
origLen := len(body)
body = body[:1000] + fmt.Sprintf("... (%d bytes total)", origLen)
This same issue appears in lines 451-452, 458-459, and 471-472.
… tests, fix example line number - DRY up duplicated truncation logic into a UTF-8-safe truncateStr helper - Add table-driven tests for FindBareRefs covering bare refs, wrapped refs, edge cases - Fix incorrect line number in bare-variable-warning example comment (18 → 25) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #104 +/- ##
==========================================
- Coverage 53.01% 52.56% -0.46%
==========================================
Files 35 35
Lines 3550 3628 +78
==========================================
+ Hits 1882 1907 +25
- Misses 1489 1535 +46
- Partials 179 186 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
No description provided.