Skip to content

Conversation

@c-ehrlich
Copy link
Collaborator

  • Show all validation errors instead of the just first one
  • Split the validation into validating and printing, so we can test the important bit without asserting on console output

Copilot AI review requested due to automatic review settings December 1, 2025 17:08
@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 1, 2025

Open in StackBlitz

npm i https://pkg.pr.new/axiomhq/ai/axiom@178

commit: 45e0f35

Copilot finished reviewing on behalf of c-ehrlich December 1, 2025 17:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the flag validation logic to collect and display all validation errors at once, rather than failing on the first error. The implementation splits the validation into two distinct functions: one for collecting errors without side effects (collectFlagValidationErrors) and one for printing errors and exiting (printFlagValidationErrorsAndExit). This separation improves testability by allowing the core validation logic to be tested independently of console output.

Key Changes:

  • Introduced collectFlagValidationErrors function that returns a result object with all validation errors
  • Created printFlagValidationErrorsAndExit function to handle error presentation and process termination
  • Refactored validateFlagOverrides to use the new functions, maintaining backward compatibility

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/ai/src/cli/utils/parse-flag-overrides.ts Adds new validation functions and types; refactors existing validation to collect all errors
packages/ai/test/cli/utils/parse-flag-overrides.test.ts Adds comprehensive tests for collectFlagValidationErrors; minimal tests for validateFlagOverrides
Comments suppressed due to low confidence (1)

packages/ai/test/cli/utils/parse-flag-overrides.test.ts:511

  • Missing test coverage for the case where multiple errors are printed by validateFlagOverrides (e.g., multiple invalid paths). The PR's key feature is showing all validation failures, but there's no test verifying that all errors are actually printed to the console. Consider adding a test that validates multiple console.error calls when there are multiple invalid paths.
  it('prints errors and exits on invalid flags', () => {
    const overrides = {
      'model.unknown': 'value',
    };

    validateFlagOverrides(overrides, testSchema);

    expect(mockConsoleError).toHaveBeenCalledWith('❌ Invalid CLI flags:');
    expect(mockProcessExit).toHaveBeenCalledWith(1);
  });

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant