feat(#1313): accept positional args for validate input#3382
Conversation
|
🤖 Finished Review · ✅ Success · Started 9:02 AM UTC · Completed 9:13 AM UTC |
Review — commentThe PR correctly implements positional argument support for Two issues are worth addressing before merge: Findings1.
|
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 2:33 PM UTC · Completed 2:55 PM UTC |
- Move file-path merge and validation from RunE to PreRunE for earlier error reporting before expensive policy initialization - Add deduplication of file paths when both --file flag and positional arguments provide the same file - Update all 7 acceptance test scenarios to use positional arguments instead of --file flag, preventing CI failures from deprecation warning on stderr - Regenerate auto-generated documentation (ec_validate_input.adoc) - Remove unnecessary SetTestRekorPublicKey from no-file-provided test Addresses review feedback on #3382
🔧 Fix agent — iteration 1 (human-triggered)Addressed 4 of 5 review findings. Fixed the high-severity acceptance test breakage by converting all scenarios to positional args. Regenerated auto-generated docs. Moved validation to PreRunE for earlier error reporting. Added file path deduplication. Disagreed with converting existing unit tests (out of scope enhancement). Fixed (4):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Review · ❌ Terminated · Started 2:59 PM UTC · Ended 3:13 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
🤖 Finished Review · ✅ Success · Started 2:59 PM UTC · Completed 3:13 PM UTC |
|
A few things from the previous review round are still open. The fix agent addressed the biggest items (PreRunE timing, dedup, acceptance tests, docs regen), but these were left untouched:
|
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 12:12 PM UTC · Completed 12:35 PM UTC |
- Replace cobra.ArbitraryArgs with custom validator rejecting flag-like args - Remove log.Warn deprecation warning to match image.go silent pattern - Use errors.Join for empty-files check (error accumulation consistency) - Add deprecated --file multi-file patterns to examples/docs - Fix Use field ordering to show files before flags - Consolidate 3 positional arg tests into table-driven test with require - Add dedup test case and flag-like arg rejection test - Regenerate ec_validate_input.adoc via go generate Addresses review feedback on #3382
|
🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:42 PM UTC |
|
/fs-fix address all the unresolved inline comments |
|
/fs-fix address the three remaining unresolved comments (args validator, missing acceptance test, missing unit test cases) |
|
🤖 Finished Fix · ✅ Success · Started 8:12 AM UTC · Completed 8:39 AM UTC |
- Remove redundant custom Args validator: Cobra already rejects unknown flags during flag parsing; the custom validator also broke valid filenames starting with '-' after the '--' end-of-flags marker - Add acceptance test for deprecated --file flag backward compatibility - Add unit test cases for empty string and non-existent file path as positional arguments Addresses review feedback on #3382
|
🤖 Finished Review · ✅ Success · Started 8:40 AM UTC · Completed 8:46 AM UTC |
|
/fs-fix address the last outstanding comment about the empty positional arg, then squash all commits into one |
|
🤖 Finished Fix · ✅ Success · Started 8:49 AM UTC · Completed 9:02 AM UTC |
Change ec validate input to accept input file paths as positional arguments instead of requiring the --file flag. This enables natural shell glob usage like ec validate input *.yaml. The --file flag is hidden but still accepted for backward compatibility. If both positional arguments and --file are provided, the values are combined with deduplication. Empty and whitespace-only entries are stripped before validation. Changes: - cmd/validate/input.go: add cobra.ArbitraryArgs, merge positional args into filePaths, deduplicate and strip empty entries, replace MarkFlagRequired with MarkHidden, add manual validation for at least one input file, update usage/examples - cmd/validate/input_test.go: add tests for positional args, multiple positional args, mixed positional and flag usage, invalid positional args including empty string handling - docs: update ec_validate_input.adoc for new usage pattern - features: update acceptance tests for positional arg usage Closes #1313
6050c23 to
73f9024
Compare
|
🤖 Finished Review · ✅ Success · Started 9:03 AM UTC · Completed 9:10 AM UTC |
|
🤖 Finished Retro · ✅ Success · Started 9:35 AM UTC · Completed 9:45 AM UTC |
|
PR #3382 on conforma/cli required 5 fix agent iterations and 7 review runs over 2.5 days. The primary rework driver was the fix agent ignoring human reviewer inline comments on its first pass — it addressed only the review agent's findings, causing 4 extra fix iterations. The review agent re-raised the same MarkDeprecated finding across all 7 runs even after the human explicitly resolved it citing an existing codebase convention. The review agent also gave contradictory advice on argument validation (first suggesting a custom validator, then calling it redundant), wasting a fix cycle. Proposals filed
|
Change ec validate input to accept input file paths as positional arguments instead of requiring the --file flag. This enables natural shell glob usage like ec validate input *.yaml.
The --file flag is deprecated but still accepted for backward compatibility. If both positional arguments and --file are provided, the values are combined.
Changes:
positional args into filePaths, replace MarkFlagRequired
with MarkHidden, add manual validation for at least one
input file, update usage/examples
multiple positional args, mixed positional and flag usage,
update no-file-provided test for new error message
Closes #1313
Post-script verification
agent/1313-positional-args-input)d134e495b18cf8493cfea8791e310b58f0320797..HEAD)