Add Canon loop scaffolding and reporter/cli sample modules - #16
Conversation
Summary of ChangesHello @andreidita22, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for a robust and verifiable module development process by introducing the "Canon loop." It defines a strict contract for modules through a new lock file, provides concrete examples with "reporter" and "cli" modules, and integrates a verification script to enforce these standards. The changes aim to streamline development, improve consistency, and ensure that all modules meet predefined architectural and functional requirements before deployment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the "Canon loop" concept, adding significant scaffolding, documentation, and two new sample modules: reporter and cli. The changes are extensive and well-documented, establishing a clear process for module implementation and verification. The new canon-verify.mjs script is a great addition for ensuring conformance. My review focuses on improving the robustness and efficiency of the new module implementations and their test runners. I've identified a few areas for improvement in argument parsing, file I/O efficiency, and test cleanup logic to prevent race conditions.
| if (tokens.length && tokens[0].includes('node')) { | ||
| tokens.shift(); | ||
| } | ||
| if (tokens.length && tokens[0].endsWith('.js')) { | ||
| tokens.shift(); | ||
| } |
There was a problem hiding this comment.
The logic for stripping the node executable and script name from argv is fragile. It relies on includes('node') and endsWith('.js'), which can lead to incorrect behavior with arguments that happen to match these patterns (e.g., a file argument named node.js). This could cause arguments to be silently dropped.
A more robust approach would be to expect the caller to provide a clean argument array (e.g., by passing process.argv.slice(2)), which is a common and robust pattern for reusable CLI modules. This makes the module's behavior more predictable.
| if (tokens.length && tokens[0].includes('node')) { | |
| tokens.shift(); | |
| } | |
| if (tokens.length && tokens[0].endsWith('.js')) { | |
| tokens.shift(); | |
| } | |
| // The cliParse function expects that `argv` has been pre-processed | |
| // to remove the node executable and script name. | |
| const tokens = argv.slice(); |
| for (let i = 0; i < tokens.length; i += 1) { | ||
| const token = tokens[i]; | ||
| if (token.startsWith('--')) { | ||
| const [rawKey, rawValue] = token.slice(2).split('=', 2); | ||
| if (!rawKey) { | ||
| result.errors.push('Invalid flag syntax.'); | ||
| continue; | ||
| } | ||
| let value = rawValue; | ||
| if (value === undefined) { | ||
| const next = tokens[i + 1]; | ||
| if (next && !next.startsWith('-')) { | ||
| value = next; | ||
| i += 1; | ||
| } else { | ||
| value = true; | ||
| } | ||
| } | ||
| result.options[rawKey] = value; | ||
| continue; | ||
| } | ||
| if (token.startsWith('-')) { | ||
| const flags = token.slice(1).split(''); | ||
| for (const flag of flags) { | ||
| result.options[flag] = true; | ||
| } | ||
| continue; | ||
| } | ||
| result.positionals.push(token); | ||
| } |
There was a problem hiding this comment.
This for loop, which manually increments the index i both in the loop definition and within the loop body, can be hard to follow and is prone to off-by-one errors. Refactoring this to a while loop that consumes tokens from the array using tokens.shift() would make the logic clearer and less error-prone, as you would not need to manage the index manually.
Here's a conceptual example:
const tokens = ...; // remaining tokens
while (tokens.length > 0) {
const token = tokens.shift();
if (token.startsWith('--')) {
// ... logic for long options
if (value === undefined) {
const next = tokens[0]; // peek
if (next && !next.startsWith('-')) {
value = tokens.shift(); // consume value
}
}
} else {
// ...
}
}| if (!alreadyLast) { | ||
| prior.push(trimmed); | ||
| await fs.writeFile(logFile, `${prior.join('\n')}\n`, 'utf8'); | ||
| } |
There was a problem hiding this comment.
The current implementation reads the entire log file into memory, modifies the array, and then writes the entire contents back to disk on every call. This can become inefficient if the log file grows very large, leading to high memory usage and slow I/O.
Using fs.appendFile is more efficient for this use case as it avoids rewriting the entire file. The in-memory prior array can still be updated after a successful append to ensure the correct lines are returned.
| if (!alreadyLast) { | |
| prior.push(trimmed); | |
| await fs.writeFile(logFile, `${prior.join('\n')}\n`, 'utf8'); | |
| } | |
| if (!alreadyLast) { | |
| await fs.appendFile(logFile, `${trimmed}\n`, 'utf8'); | |
| prior.push(trimmed); | |
| } |
| } | ||
| } finally { | ||
| await fs.rm(logDir, { recursive: true, force: true }); | ||
| await fs.rm(path.join(moduleRoot, '.tmp'), { recursive: true, force: true }); |
There was a problem hiding this comment.
This line, which removes the parent .tmp directory, can introduce a race condition if tests are run in parallel. Another test might be using a different scratch directory under .tmp, and this would delete it prematurely, causing the other test to fail. The cleanup should be limited to the scratch directory created for this specific test case (logDir). The parent .tmp directory should be cleaned up in a global test suite teardown, not within each test case.
| } | ||
|
|
||
| const lockPath = args.lock; | ||
| const modulesRoot = args['modules-root'] || args.modulesRoot; |
There was a problem hiding this comment.
The expression args['modules-root'] || args.modulesRoot is redundant. The parseArgs function creates object keys from arguments like --modules-root as modules-root. Therefore, args.modulesRoot will always be undefined. While this doesn't cause a bug, it's confusing and can be simplified.
| const modulesRoot = args['modules-root'] || args.modulesRoot; | |
| const modulesRoot = args['modules-root']; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tokens = argv.slice(); | ||
| if (tokens.length && tokens[0].includes('node')) { | ||
| tokens.shift(); | ||
| } | ||
| if (tokens.length && tokens[0].endsWith('.js')) { | ||
| tokens.shift(); |
There was a problem hiding this comment.
The parser trims a node binary and then only removes the next token when it ends with .js. When the CLI is executed as an ES module (e.g. node cli.mjs report …), the .mjs script path remains in tokens, so the parser treats the path itself as the command and flags every invocation as Unknown command. This repo already ships scripts with .mjs extensions, so the CLI will misbehave in normal usage until the code recognises module file extensions (or more generally strips any second argument that looks like a script file).
Useful? React with 👍 / 👎.
…li-modules-for-canon-loop Canon: enforce reporter idempotency and record PASS evidence
1 participant
Summary