feat(validate): add command to validate JSON AST against Concerto met… by hussein-saad · Pull Request #69 · accordproject/concerto-cli

hussein-saad · 2025-03-13T13:18:38Z

Closes #55

Changes

New Command Addition:

Added a new validate-ast command to the CLI, which validates a JSON syntax tree against the Concerto metamodel.

Error Handling Improvements:

Improved error handling in the print method to catch and report JSON parsing errors and invalid AST structures. (lib/commands.js)
Added comprehensive error messages for various validation failures in the validateAST method. (lib/commands.js)

Validation Enhancements:

Introduced a new validateAST method in the Commands class to validate a Concerto JSON AST. This method checks for the correct structure, required properties. (lib/commands.js)

Test Coverage Expansion:

Added tests for the new validateAST method to cover various scenarios, including valid and invalid models, missing properties, and error handling. (test/cli.js)
Enhanced existing tests to handle invalid JSON content, file not found errors, and invalid AST validation. (test/cli.js)

Related Issues

Issue Add validateAST command and/or validate on print #55

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to main from fork:branchname

…amodel and enhance print function with AST validation Add validateAST method to validate Concerto metamodel JSON structures Enhance print function to validate AST structure before converting to CTO Provide detailed error messages for invalid AST structur Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

hussein-saad · 2025-03-15T23:59:11Z

hi @DianaLease, could you please review this PR?

DianaLease · 2025-03-26T01:40:37Z

@dselman @mttrbrts Could you please take a look at this one? thanks!

mttrbrts

Thanks for the contribution! Let's reuse the validation logic that already exists in the MetamodelUtil module, rather than redefining it partially here.

lib/commands.js

Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

mttrbrts

Thanks for your contribution. Just a few small comments from me.

test/cli.js

lib/commands.js

… AST validation Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

hussein-saad · 2025-04-13T06:25:42Z

hey @mttrbrts, could you take a look at the changes when you have time?

Copilot

Pull request overview

Adds CLI support for validating Concerto JSON metamodel ASTs, and strengthens print by validating AST input before converting it to CTO, addressing #55.

Changes:

Added a new validate-ast CLI command for validating a JSON AST against the Concerto metamodel.
Added Commands.validateAST and integrated AST validation into Commands.print.
Expanded test coverage for validation and error-handling scenarios.

Reviewed changes

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

File	Description
`index.js`	Adds the new `validate-ast` CLI command and its argument parsing/handler logic.
`lib/commands.js`	Implements `validateAST` and validates JSON ASTs in `print` prior to printing.
`test/cli.js`	Adds unit tests for `validateAST` and new `print` error-handling cases.

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

Copilot · 2026-03-01T19:35:16Z

index.js

+        yargs.option('strict', {
+            describe: 'perform strict validation',
+            type: 'boolean',
+            default: true
+        });


The strict option is defined for the validate-ast command but never used (it isn't passed to Commands.validateAST, and validateAST doesn't accept options). This is confusing for CLI users; either wire it through (e.g., add an options parameter) or remove the flag until it’s supported.

Suggested change

yargs.option('strict', {

describe: 'perform strict validation',

type: 'boolean',

default: true

});

Copilot · 2026-03-01T19:35:17Z

index.js

+            Commands.validateAST(json);
+        } catch (err) {
+            Logger.error('Error validating AST');


The validate-ast handler swallows the underlying exception details by always logging only "Error validating AST" and exiting. This makes it hard to diagnose whether the failure was due to file I/O, JSON parsing, or metamodel validation; log err.message (and/or the full error) and consider printing the success message returned by Commands.validateAST on success.

Suggested change

Commands.validateAST(json);

} catch (err) {

Logger.error('Error validating AST');

const result = Commands.validateAST(json);

if (result) {

Logger.info(result);

}

} catch (err) {

Logger.error('Error validating AST');

Logger.error(err && err.message ? err.message : err);

Copilot · 2026-03-01T19:35:17Z

lib/commands.js

+        let json;
+        try {
+            json = JSON.parse(inputString);
+            // Validate the AST before printing
+            this.validateAST(json);
+        } catch (error) {
+            throw new Error(error.message);
+        }


print catches JSON/AST validation errors and rethrows new Error(error.message), which discards the original error type and stack trace. Prefer rethrowing the original error (or wrapping while preserving the original error as the cause) so callers and users can debug failures more easily.

Copilot · 2026-03-01T19:35:17Z

lib/commands.js

+     * Validates a Concerto JSON Abstract Syntax Tree (AST)
+     *
+     * @param {object} ast - The AST to validate
+     * @returns {string} Success message
+     * @throws {Error} If validation fails or file cannot be read/parsed.


The JSDoc for validateAST says it throws if the "file cannot be read/parsed", but this method only validates an already-parsed AST object and never reads/parses a file. Please update the docstring to match the actual behavior (or move file parsing responsibilities into this method if that’s the intent).

Suggested change

* Validates a Concerto JSON Abstract Syntax Tree (AST)

*

* @param {object} ast - The AST to validate

* @returns {string} Success message

* @throws {Error} If validation fails or file cannot be read/parsed.

* Validates a Concerto JSON Abstract Syntax Tree (AST).

*

* @param {object} ast - The AST to validate.

* @returns {string} Success message.

* @throws {Error} If validation fails.

Copilot · 2026-03-01T19:35:17Z

lib/commands.js

+
+        try {
+            // deserialize the user's AST using the metamodel's rules.
+            // This will validates the structure, types, and constraints ($class, required fields etc.)


Minor grammar in the inline comment: "This will validates" should be "This will validate". Fixing this keeps comments clear and professional.

Suggested change

// This will validates the structure, types, and constraints ($class, required fields etc.)

// This will validate the structure, types, and constraints ($class, required fields etc.)

Copilot · 2026-03-01T19:35:18Z

test/cli.js

+            try {
+                await Commands.print('/path/to/nonexistent/file.json');
+                expect.fail('Expected error was not thrown');
+            } catch (err) {
+                err.should.be.an.instanceOf(Error);


This test uses a hard-coded POSIX path (/path/to/nonexistent/file.json) to trigger ENOENT. Since CI runs on Windows/macOS/Linux, it’s safer to generate a guaranteed-nonexistent path using tmp/os.tmpdir() + a random filename so the test is platform-independent and can’t accidentally exist.

Suggested change

try {

await Commands.print('/path/to/nonexistent/file.json');

expect.fail('Expected error was not thrown');

} catch (err) {

err.should.be.an.instanceOf(Error);

const tempDir = await tmp.dir({ unsafeCleanup: true });

const nonExistentPath = path.join(tempDir.path, `nonexistent-${Date.now()}.json`);

try {

try {

await Commands.print(nonExistentPath);

expect.fail('Expected error was not thrown');

} catch (err) {

err.should.be.an.instanceOf(Error);

}

} finally {

tempDir.cleanup();

ekarademir · 2026-03-14T21:47:39Z

@hussein-saad will you be able to address the comments?

DianaLease requested review from a team, dselman and mttrbrts March 18, 2025 22:22

mttrbrts requested changes Mar 26, 2025

View reviewed changes

lib/commands.js Outdated Show resolved Hide resolved

lib/commands.js Outdated Show resolved Hide resolved

lib/commands.js Outdated Show resolved Hide resolved

refactor(validation): simplify AST validation process

dcebc42

Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

hussein-saad requested a review from mttrbrts April 2, 2025 23:22

mttrbrts reviewed Apr 5, 2025

View reviewed changes

test/cli.js Outdated Show resolved Hide resolved

lib/commands.js Show resolved Hide resolved

lib/commands.js Outdated Show resolved Hide resolved

hussein-saad added 3 commits April 6, 2025 01:19

fix(commands): enable automatic addition of the Concerto metamodel in…

cf870a6

… AST validation Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

refactor(commands): Refactor error handling in print method

f7baab8

Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

fix(test): Fix the tests for AST validation

460d757

Signed-off-by: Hussein Saad <hussein.saad2021@gmail.com>

hussein-saad requested a review from mttrbrts April 5, 2025 23:56

Merge branch 'main' into hussein-saad/i55/validate-ast

6f645be

mttrbrts requested a review from Copilot March 1, 2026 19:32

Copilot started reviewing on behalf of mttrbrts March 1, 2026 19:32 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

vermarjun mentioned this pull request Mar 7, 2026

Add validateAST command and/or validate on print #55

Open

2 tasks

	// This will validates the structure, types, and constraints ($class, required fields etc.)
	// This will validate the structure, types, and constraints ($class, required fields etc.)

Conversation

hussein-saad commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #55

Changes

New Command Addition:

Error Handling Improvements:

Validation Enhancements:

Test Coverage Expansion:

Related Issues

Author Checklist

Uh oh!

hussein-saad commented Mar 15, 2025

Uh oh!

DianaLease commented Mar 26, 2025

Uh oh!

mttrbrts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mttrbrts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hussein-saad commented Apr 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

ekarademir commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hussein-saad commented Mar 13, 2025 •

edited

Loading