silent verify script#282
Open
jasonmorais wants to merge 9 commits into
Open
Conversation
Contributor
Reviewer's GuideIntroduces a new @cellix/local-dev workspace package that provides silent command runners and typed wrappers around pnpm/knip/audit/Snyk/Sonar tooling, wires the root verify workflow through a new TypeScript verify script built on these runners, silences agent formatter output when there are no issues, and performs minor dependency/workspace/docs updates. Sequence diagram for TypeScript-based verify workflow using silent runnerssequenceDiagram
actor Developer
participant NpmVerify as npm_verify_script
participant VerifyTs as verify_ts
participant VerificationSequence
participant RunSequence as runSilentCommandSequence
participant RunCommand as runSilentCommand
participant Tool as External_tool
Developer->>NpmVerify: pnpm run verify
NpmVerify->>VerifyTs: node build-pipeline/scripts/verify.ts
VerifyTs->>VerificationSequence: verificationSequence.addStep(...)+
VerifyTs->>VerificationSequence: VerificationSequence.run()
VerificationSequence->>RunSequence: runSilentCommandSequence(steps)
loop for each step
RunSequence->>RunCommand: runSilentCommand(SilentCommandOptions)
RunCommand->>Tool: spawnSync(command, args)
Tool-->>RunCommand: status, stdout, stderr
alt status == 0
RunCommand-->>RunSequence: SilentCommandResult (no output replayed)
else status != 0
RunCommand-->>RunSequence: SilentCommandResult
RunCommand->>Developer: replayFailureOutput(stdout, stderr)
break loop
RunSequence-->>VerificationSequence: SilentCommandSequenceResult (failing step)
VerificationSequence-->>VerifyTs: SilentCommandSequenceResult
VerifyTs->>Developer: process.exitCode = status
end
end
end
alt all steps succeed
RunSequence-->>VerificationSequence: SilentCommandSequenceResult (status 0)
VerificationSequence-->>VerifyTs: SilentCommandSequenceResult
VerifyTs->>Developer: write verify passed
VerifyTs->>Developer: process.exitCode = 0
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AgentFormatter.onTestRunFinishedyou now return early whenissueCount === 0, which removes the previous "All scenarios passed" messaging entirely; if that feedback is still desired, consider keeping a minimal success summary instead of returning silently. - The
verify.tsscript prints a custom failure line afterrunSilentCommandSequencehas already replayed the failing step's stdout/stderr; consider including the step exit status in that replay or adjusting the extra message to avoid redundant-but-slightly-different failure context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AgentFormatter.onTestRunFinished` you now return early when `issueCount === 0`, which removes the previous "All scenarios passed" messaging entirely; if that feedback is still desired, consider keeping a minimal success summary instead of returning silently.
- The `verify.ts` script prints a custom failure line after `runSilentCommandSequence` has already replayed the failing step's stdout/stderr; consider including the step exit status in that replay or adjusting the extra message to avoid redundant-but-slightly-different failure context.
## Individual Comments
### Comment 1
<location path="build-pipeline/scripts/verify.ts" line_range="29-38" />
<code_context>
+ .addStep(sonarPullRequestAnalysis())
+ .addStep(sonarQualityGate());
+
+const isRunningAsCommand = import.meta.main;
+
+function runVerifyCommand(): void {
+ const result = cellixVerify.run();
+ if (result.status !== 0) {
+ process.stderr.write(`\nverify failed at step "${result.step.name}" (exit ${result.status})\n`);
+ } else {
+ process.stdout.write('verify passed\n');
+ }
+ process.exitCode = result.status;
+}
+
+if (isRunningAsCommand) {
+ runVerifyCommand();
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `import.meta.main` will never trigger in Node.js and prevents the script from running when executed directly.
In Node.js, `import.meta.main` is always `undefined`, so `runVerifyCommand()` never runs, even when this file is invoked as the entry script (e.g. via `pnpm run verify`). Use a Node-compatible check instead, such as comparing `import.meta.url` to `pathToFileURL(process.argv[1]).href`, or invoke `runVerifyCommand()` from a dedicated bootstrap file that always calls it.
</issue_to_address>
### Comment 2
<location path="package.json" line_range="52" />
<code_context>
"sonar:pr-windows": "for /f %i in ('node build-pipeline/scripts/get-pr-number.cjs') do set PR_NUMBER=%i && sonar-scanner -Dsonar.pullrequest.key=%PR_NUMBER% -Dsonar.pullrequest.branch=%BRANCH_NAME% -Dsonar.pullrequest.base=main",
"check-sonar": "node build-pipeline/scripts/check-sonar-quality-gate.cjs",
- "verify": "pnpm run format:check && pnpm run test:arch && pnpm run test:coverage:merge && pnpm run test:e2e && pnpm run knip && pnpm run audit && pnpm run snyk && pnpm run sonar:pr && pnpm run check-sonar",
+ "verify": "node build-pipeline/scripts/verify.ts",
"knip": "knip",
"snyk": "pnpm run snyk:test && pnpm run snyk:code",
</code_context>
<issue_to_address>
**issue (bug_risk):** Running a `.ts` file directly with `node` will fail without a custom loader or precompiled JS.
This script now calls `node build-pipeline/scripts/verify.ts`, but Node can’t run TypeScript directly. Unless there’s an existing loader configured for this command (e.g. `tsx`, `ts-node`, or a custom `--loader`), it will fail at runtime. Consider either targeting the compiled `.js` file or running it via the configured TS runner instead of plain `node`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
|
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="build-pipeline/scripts/verify.ts" line_range="29-38" />
<code_context>
+ .addStep(sonarPullRequestAnalysis())
+ .addStep(sonarQualityGate());
+
+const isRunningAsCommand = import.meta.main;
+
+function runVerifyCommand(): void {
+ const result = cellixVerify.run();
+ if (result.status !== 0) {
+ process.stderr.write(`\nverify failed at step "${result.step.name}" (exit ${result.status})\n`);
+ } else {
+ process.stdout.write('verify passed\n');
+ }
+ process.exitCode = result.status;
+}
+
+if (isRunningAsCommand) {
+ runVerifyCommand();
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `import.meta.main` is likely non-portable and may prevent the verify script from running when invoked directly.
In Node’s current ESM implementation `import.meta.main` is not available, so `isRunningAsCommand` will be `undefined` and `runVerifyCommand()` will never run, even when you call `node build-pipeline/scripts/verify.ts` directly. To ensure the script actually executes as a CLI, switch to the common pattern of comparing `import.meta.url` with `pathToFileURL(process.argv[1]).href` (or another supported entry-point check).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
verify.tsscript printsverify passedon success, which slightly conflicts with the goal of keeping successful verification runs silent; consider making this success message optional (e.g., behind a flag) or removing it to keep the workflow fully quiet when healthy. - The
runSilentCommandhelper hardcodes a 64MBmaxBuffer; if you expect some tools to produce larger output, consider exposingmaxBuffer(or a similar option) onSilentCommandOptionsso callers can tune this per command. - The construction of the
cellixVerifysequence inverify.tsis fairly specific and could be reused elsewhere; consider extracting this builder into a small shared helper in@cellix/local-dev(or a nearby module) so local/CI verification flows can share the same sequence definition without duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `verify.ts` script prints `verify passed` on success, which slightly conflicts with the goal of keeping successful verification runs silent; consider making this success message optional (e.g., behind a flag) or removing it to keep the workflow fully quiet when healthy.
- The `runSilentCommand` helper hardcodes a 64MB `maxBuffer`; if you expect some tools to produce larger output, consider exposing `maxBuffer` (or a similar option) on `SilentCommandOptions` so callers can tune this per command.
- The construction of the `cellixVerify` sequence in `verify.ts` is fairly specific and could be reused elsewhere; consider extracting this builder into a small shared helper in `@cellix/local-dev` (or a nearby module) so local/CI verification flows can share the same sequence definition without duplication.
## Individual Comments
### Comment 1
<location path="packages/cellix/local-dev/src/silent-runners/index.ts" line_range="177-186" />
<code_context>
+ * @param options - Sequence steps and optional process context.
+ * @returns The final successful step result or the first failing step result.
+ */
+export function runSilentCommandSequence(options: SilentCommandSequenceOptions): SilentCommandSequenceResult {
+ const { cwd, env, spawn, steps, streams } = options;
+ if (steps.length === 0) {
+ throw new Error('runSilentCommandSequence requires at least one step');
+ }
+
+ let lastResult: SilentCommandSequenceResult | undefined;
+
+ for (const step of steps) {
+ const context: CommandSequenceContext = { cwd, env, spawn, streams };
+ const result = step.output === 'inherit' ? runInheritedCommand(buildCommandOptions(step, context)) : runSilentCommand(buildCommandOptions(step, context));
+ lastResult = {
+ ...result,
+ step,
+ };
+
+ if (result.status !== 0) {
+ return lastResult;
+ }
+ }
+
+ if (!lastResult) {
+ throw new Error('runSilentCommandSequence requires at least one step');
+ }
</code_context>
<issue_to_address>
**nitpick:** The `runSilentCommandSequence` empty-steps guard is duplicated and could be simplified.
The function throws `runSilentCommandSequence requires at least one step` both on the initial `steps.length === 0` check and again when `!lastResult` after the loop. Because of the initial check, the post-loop guard is unreachable and can be removed, or you can drop the initial check and rely solely on the `lastResult` check to cover the empty-steps case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
# Conflicts: # pnpm-lock.yaml # pnpm-workspace.yaml
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new verify runner currently writes
verify passedon success, which slightly conflicts with the "silent on success" contract of the silent runners; consider omitting this message so local runs stay fully quiet when healthy. - In
replayFailureOutput, the stderr replay concatenates the failure banner and the tool stderr without ensuring a trailing newline from the tool output; consider always terminating the combined message with a newline to avoid malformed prompts when the tool's stderr is not newline-terminated. - The Snyk helper
snykIacScantakestargetsas the first argument whereas the other Snyk helpers accept only an options object; consider making its signature consistent (e.g.,snykIacScan({ targets, args, name })) to keep the public API uniform.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new verify runner currently writes `verify passed` on success, which slightly conflicts with the "silent on success" contract of the silent runners; consider omitting this message so local runs stay fully quiet when healthy.
- In `replayFailureOutput`, the stderr replay concatenates the failure banner and the tool stderr without ensuring a trailing newline from the tool output; consider always terminating the combined message with a newline to avoid malformed prompts when the tool's stderr is not newline-terminated.
- The Snyk helper `snykIacScan` takes `targets` as the first argument whereas the other Snyk helpers accept only an options object; consider making its signature consistent (e.g., `snykIacScan({ targets, args, name })`) to keep the public API uniform.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a reusable silent verification runner package and wire the root verify workflow through it while keeping successful runs quiet.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: