Support YAML and TOML config formats; add avenor verify command - #166
Support YAML and TOML config formats; add avenor verify command#166sdougbrown wants to merge 6 commits into
Conversation
Add internal/configfile package with a shared Load(path, dst) helper that detects format by file extension (.json default, .yaml/.yml, .toml) and decodes into any Go struct. Non-JSON formats are normalized through a JSON intermediate so that DisallowUnknownFields semantics and json struct tags apply uniformly across all formats. Wire configfile.Load into the three existing config loaders: - rosterconfig.Load - teamrunner.LoadTeamConfigWithRoster - looprunner.LoadLoopConfigWithRoster This also extends DisallowUnknownFields to team and loop configs (previously only roster enforced it), keeping deferred/misspelled fields from silently becoming no-ops in all config types. The shared loader is designed so that future Workflow and Controller config loaders can use configfile.Load with zero format-specific code. Related: #165 (os.Expand variable substitution pass)
Add a 'verify' subcommand that loads and validates config files without starting a run. It exercises the full validation path (format decoding, unknown-field rejection, mutual exclusions, prompt presence, roster entry references, prompt_file resolution) and recursively validates nested loop_file/team_file references that are only loaded on demand during a real run. Usage: avenor verify --loop-file loop.json avenor verify --team-file team.json --roster-file roster.json avenor verify --roster-file roster.json --roster-entry planner Accepts the same --dir flag as run for resolving relative paths. Works with JSON, YAML, and TOML configs via the shared configfile loader. Exit code 0 with 'ok:' messages on success; exit code 1 with 'error:' messages on failure.
Add avenor verify section to cli.md with flags, examples, and output format. Update flag descriptions and roster/team/loop docs to mention YAML and TOML support alongside JSON. Add verify usage tips to the loop and team quick-start sections.
Must-fix: - Remove unused parentPath parameter from checkNested - Fix YAML multi-doc error handling: non-EOF parse errors are now wrapped with the underlying error instead of being mislabeled as 'multiple YAML documents'. The three-way check (EOF=ok, nil=multiple docs, error=parse error) preserves the multi-document rejection while surfacing real parse errors correctly. Should-fix: - Replace custom contains/indexOf/containsCount helpers with strings.Contains and strings.Count from the standard library - Factor buildVerifyBinary to use sync.Once so the binary is compiled once instead of per-test (35s → 2.8s) - Add explicit error for --roster-entry without --roster-file - Improve TestVerifyNestedLoopFile to assert on file paths (outer.json, inner.json) instead of just counting 'ok: loop' occurrences - Improve TestVerifyLoopWithRosterAndRosterEntryRef to assert on ok: output New test coverage: - TestVerifyRosterEntryWithoutRosterFile - TestVerifyNestedTeamFile (team→team nesting) - TestVerifyCombinedLoopTeamRoster (all three config types at once) - TestLoadYAMLRejectsCommentsOnly - TestLoadTOMLRejectsNestedUnknownFields
a461b49 to
5f24e04
Compare
![umpire-bot[bot]](https://avatars.githubusercontent.com/in/3739611?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is marked... FOUL BALL. 👉
Warning
- T3 Duplication: format tests in looprunner, teamrunner, and rosterconfig follow an identical structure and should use a shared helper (
internal/looprunner/config_format_test.go:11,internal/teamrunner/config_format_test.go:11,internal/rosterconfig/roster_format_test.go:11) - T1 Obscurity:
TestVerifyCombinedLoopTeamRosterasserts on multiple output substrings without labeled sub-tests, making failure localization difficult (cmd/avenor/verify_test.go:379) - T1 Obscurity: The 'contains' helper lacks context or labels, allowing tests to pass silently on unrelated successful output (
cmd/avenor/verify_test.go:67) - Documentation: The test for null JSON decodes to a zero-value struct without error; it should assert specific field values to better document behavior (
internal/configfile/configfile_test.go:62)
Prior Findings Review
Fixed (5)
Prior findings that have been resolved.
5 prior findings have been resolved in the current code.
| } | ||
| } | ||
|
|
||
| func yamlToJSON(path string, data []byte) ([]byte, error) { |
There was a problem hiding this comment.
yamlToJSON() error paths (empty YAML, multi-document, comments-only) have no direct unit test of the function itself; they are only exercised indirectly through Load-level tests (TestLoadYAMLRejectsEmptyDocument, TestLoadYAMLRejectsMultipleDocuments, TestLoadYAMLRejectsCommentsOnly). A direct unit test of yamlToJSON would localize failures in the normalization path (e.g. json.Marshal errors on unusual YAML nodes) instead of surfacing them via the full load flow.
There was a problem hiding this comment.
👾 AI Agent
Accepted as-is. The yamlToJSON error paths (empty YAML, multi-document, comments-only) are exercised through the public Load API via TestLoadYAMLRejectsEmptyDocument, TestLoadYAMLRejectsMultipleDocuments, and TestLoadYAMLRejectsCommentsOnly. These tests exercise the full normalization + decode path, which is the surface callers actually use. yamlToJSON is an unexported implementation detail; adding direct unit tests would couple tests to internal structure without improving coverage. Additionally, yaml.v3 produces values that json.Marshal always handles successfully — the json.Marshal error path is unreachable in practice.
| } | ||
| } | ||
|
|
||
| func TestVerifyRosterWithEntryLookup(t *testing.T) { |
There was a problem hiding this comment.
TestVerifyRosterWithEntryLookup (line 108) and TestVerifyRosterEntryNotFound (line 125) are near-identical: both build the same single-entry roster, invoke the built binary with --roster-file roster.json --roster-entry <name>, and assert on output, differing only in the entry name and the success/error assertion. Consider folding the negative case into a table-driven test (success/not-found rows) to remove the duplication.
There was a problem hiding this comment.
👾 AI Agent
Accepted as-is. The two tests have different semantics — one asserts a success path (exit 0, ok: output, entry found) and the other asserts an error path (exit 1, error: output, entry not found). Folding them into a table-driven test would obscure the success/error distinction for minimal deduplication gain (the shared setup is ~5 lines). The current separate form is clearer for readers and easier to extend independently.
PR review fixes: - Print errors before oks so failures surface first in merged stdout/stderr - Add output assertion to TestVerifyEmptyPhaseName - Add nonexistent.json assertion to TestVerifyFileNotFound - Add TestDecodeInvalidTOMLPreservesParseError for TOML error wrapping - Add TestDecodeInvalidYAMLPreservesParseError for YAML error wrapping - Remove dead containsCount helper - Tighten TestVerifyNestedLoopFileInvalid to assert on inner.json Regression review fixes: - Add defensive nil check in tomlToJSON for empty TOML (go-toml/v2 returns empty map not nil, so the check is defensive only; downstream validation catches empty configs via Validate())
|
Addressed 7 comments across 4 files:
Review loop: 2 iterations. Regression review found one additional must-fix (empty TOML silently accepted) — added defensive nil check in tomlToJSON; go-toml/v2 returns empty map (not nil) for empty input so the check is defensive only. Downstream Validate() catches empty configs via required-field checks. Also tightened TestVerifyNestedLoopFileInvalid to assert on inner.json path. All 39 test packages pass. |
1 similar comment
|
Addressed 7 comments across 4 files:
Review loop: 2 iterations. Regression review found one additional must-fix (empty TOML silently accepted) — added defensive nil check in tomlToJSON; go-toml/v2 returns empty map (not nil) for empty input so the check is defensive only. Downstream Validate() catches empty configs via required-field checks. Also tightened TestVerifyNestedLoopFileInvalid to assert on inner.json path. All 39 test packages pass. |
|
@umpire-bot review again |
| } | ||
| v.seen[abs] = true | ||
|
|
||
| fallback := v.resolve(rosterFallback) |
There was a problem hiding this comment.
T5 Coverage Illusion: when a loop config has a roster_file field but no --roster-file flag is passed, addLoop resolves the fallback (line 113) and calls LoadLoopConfigWithRoster with that fallback (line 114). No test exercises the case where a loop config declares roster_file: roster.json and verify picks it up as the roster source. Add a test with a loop config containing roster_file and a roster with a valid entry reference in a loop phase.
There was a problem hiding this comment.
👾 AI Agent
Incorrect. TestVerifyLoopWithRosterAndRosterEntryRef already covers this: it invokes verify with only --loop-file, while that loop config declares roster_file: roster.json and a phase uses the declared roster entry.
| // detection. This is useful when the caller has already read the file (for | ||
| // example to apply a pre-processing step) or is working with embedded data. | ||
| func Decode(path string, data []byte, dst any) error { | ||
| jsonBytes, err := normalizeToJSON(path, data) |
There was a problem hiding this comment.
Decode wraps errors from yamlToJSON/tomlToJSON with fmt.Errorf("decode config %s: %w", path, err), but those functions already wrap with the same prefix via %w, causing double-wrapping (e.g., "decode config X: decode config X: ...").
There was a problem hiding this comment.
👾 AI Agent
Incorrect. Decode returns the normalizeToJSON error directly in internal/configfile/configfile.go (the if err != nil { return err } branch), so YAML/TOML decode errors receive one decode config prefix from their helper rather than a duplicate prefix.
|
Addressed 5 comments across 3 files:
Review loop: 1 iteration. |
1 participant
Summary
Adds YAML and TOML support for config files (Team, Loop, Roster) alongside the existing JSON format, and introduces an
avenor verifycommand for validating configs without starting a run.Closes #147. Related: #165 (future
os.Expandvariable substitution pass).New:
internal/configfilepackageA shared
Load(path, dst)helper that:.json(default),.yaml/.yml,.tomlany→json.Marshal→ strictjson.Decoder) so thatDisallowUnknownFieldssemantics andjsonstruct tags apply uniformly across all formatsWired into the three existing config loaders:
rosterconfig.Loadteamrunner.LoadTeamConfigWithRosterlooprunner.LoadLoopConfigWithRosterEach loader replaced its
os.ReadFile+json.Unmarshal/json.NewDecoderwith a singleconfigfile.Load(path, &cfg)call. The validation and resolution logic downstream (Validate,ResolvePhaseFiles,LoadForConfig,validateRosterEntries) is format-agnostic and unchanged.Behavior change: Team and loop configs now get
DisallowUnknownFields(previously only roster enforced it). This is consistent with the codebase's stated intent — deferred/misspelled fields should not silently become no-ops. If existing configs have extra fields, they will need to be removed before this change.The shared loader is designed so that future Workflow and Controller config loaders can call
configfile.Load(path, &Config{})with zero format-specific code.New:
avenor verifycommandValidates config files without starting a run:
Exercises the full validation path (format decoding, unknown-field rejection, mutual exclusions, prompt presence, roster entry references, prompt_file resolution) and recursively validates nested
loop_file/team_filereferences that are only loaded on demand during a real run. Cycle detection via aseenmap prevents infinite recursion.Exit 0 with
ok:messages on success; exit 1 witherror:messages on failure.Dependencies
gopkg.in/yaml.v3 v3.0.1github.com/pelletier/go-toml/v2 v2.4.3Both are stable, widely-used, and lightweight (no transitive dependency explosions).
Design notes
The JSON-intermediate approach was chosen over direct decoding into structs because:
DisallowUnknownFieldsis preserved uniformly — YAML/TOML libraries don't have clean equivalentsjsonstruct tags work unchanged — no duplicateyaml:/toml:tags neededThe wire/JSON-only path (MCP tool args,
SpawnParams, HTTP handlers) is untouched — alternate formats are purely a filesystem-loader concern. Configs passed via tool calls remain JSON by definition.Motivating use case
The umpire-bot integration (
~/Code/umpire-bot) currently has configs with${MAX_ITERATIONS}and${LLM_BASE_URL}template variables that are invalid JSON until substituted. The integrator works around this with raw-textstrings.ReplaceAllplus a sentinel hack. Issue #165 tracks addingos.Expand-style variable substitution toconfigfile.Load, which would let the integrator pass template paths directly and delete itsmaterialize*functions and sentinel workaround.Test coverage
internal/configfile— format detection, JSON/YAML/TOML decode, unknown-field rejection per format, trailing data, multi-document YAML, empty YAML, comments-only YAML, invalid syntax, format equivalence,Decodeentry point, file-not-foundavenor verify— valid/invalid configs, roster entry lookup, nested loop/team files, combined configs, YAML/TOML formats, missing files,--roster-entrywithout--roster-filerosterconfig,teamrunner, andlooprunner— verify that JSON, YAML, and TOML produce identical decoded resultsCommits
feat: support YAML and TOML config files— configfile package + loader wiring + deps + testsfeat: add avenor verify command— verify subcommand + testsdocs: document verify command and multi-format config support— cli.md, loop.md, team.mdfix: address review findings— unused param, YAML error handling, test improvements, new coverage