silent verify script#282
Open
jasonmorais wants to merge 19 commits into
Open
Conversation
Contributor
Reviewer's GuideIntroduce a reusable "silent verification" infrastructure in @cellix/local-dev (including command runners, pnpm/Snyk/Sonar wrappers, and a fluent verification sequence) and adopt it in the root verify pipeline, while tightening AgentFormatter logging behavior, exposing writeJsonFile, and updating exports, docs, and dependency overrides. 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.
# Conflicts: # package.json # packages/cellix/serenity-framework/src/formatters/agent-formatter.ts
# Conflicts: # package.json
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:
Fixed security issues:
-
@opentelemetry/exporter-prometheus (link)
-
@vitest/browser (link)
-
form-data (link)
-
protobufjs (link)
-
vite (link)
-
ws (link)
-
Some of the step-definition and helper changes collapse previously multi-line logical conditions and
actor.attemptsTocalls into single very long lines, which hurts readability; consider keeping these broken across lines to preserve clarity and easier diffs. -
For
runSilentCommandSequencefailures you currently only show the raw command line inreplayFailureOutput; consider also including theCommandSequenceStep.nameso failures are easier to correlate with the higher-level verify steps (e.g.snyk:code,audit:prod).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Some of the step-definition and helper changes collapse previously multi-line logical conditions and `actor.attemptsTo` calls into single very long lines, which hurts readability; consider keeping these broken across lines to preserve clarity and easier diffs.
- For `runSilentCommandSequence` failures you currently only show the raw command line in `replayFailureOutput`; consider also including the `CommandSequenceStep.name` so failures are easier to correlate with the higher-level verify steps (e.g. `snyk:code`, `audit:prod`).
## Individual Comments
### Comment 1
<location path="packages/cellix/local-dev/src/silent-runners/index.ts" line_range="283-293" />
<code_context>
+ ...(step.args ? { args: step.args } : {}),
+ ...(options.cwd ? { cwd: options.cwd } : {}),
+ ...(options.env ? { env: options.env } : {}),
+ ...(step.maxBuffer !== undefined ? { maxBuffer: step.maxBuffer } : options.maxBuffer !== undefined ? { maxBuffer: options.maxBuffer } : {}),
+ ...(options.spawn ? { spawn: options.spawn } : {}),
+ ...(options.streams ? { streams: options.streams } : {}),
</code_context>
<issue_to_address>
**suggestion:** Nested ternary for resolving maxBuffer hurts readability and is easy to misinterpret.
Consider replacing this nested ternary with clearer logic, e.g. compute `const maxBuffer = step.maxBuffer ?? options.maxBuffer;` and then conditionally spread `{ maxBuffer }`, or use straightforward `if` branches. This makes the precedence explicit and reduces the chance of mistakes if more options are added later.
```suggestion
function buildCommandOptions(step: CommandSequenceStep, options: CommandSequenceContext): SilentCommandOptions {
const maxBuffer = step.maxBuffer ?? options.maxBuffer;
return {
...(step.args ? { args: step.args } : {}),
...(options.cwd ? { cwd: options.cwd } : {}),
...(options.env ? { env: options.env } : {}),
...(maxBuffer !== undefined ? { maxBuffer } : {}),
...(options.spawn ? { spawn: options.spawn } : {}),
...(options.streams ? { streams: options.streams } : {}),
command: step.command,
};
}
```
</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
# Conflicts: # packages/cellix/local-dev/README.md # packages/cellix/local-dev/manifest.md # packages/cellix/local-dev/package.json # packages/cellix/local-dev/src/index.test.ts # packages/cellix/local-dev/src/index.ts # packages/cellix/local-dev/tsconfig.vitest.json # packages/cellix/local-dev/vitest.config.ts # pnpm-lock.yaml # pnpm-workspace.yaml
noce-nick
approved these changes
Jul 10, 2026
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Fixed security issues:
- @opentelemetry/exporter-prometheus (link)
- @vitest/browser (link)
- form-data (link)
- protobufjs (link)
- vite (link)
- ws (link)
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
Fixed security issues:
- @opentelemetry/exporter-prometheus (link)
- @vitest/browser (link)
- form-data (link)
- protobufjs (link)
- vite (link)
- ws (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/cellix/local-dev/src/silent-runners/index.ts" line_range="297-301" />
<code_context>
+ };
+}
+
+function toSilentCommandResult(result: SpawnSyncReturns<string>): SilentCommandResult {
+ return {
+ ...(result.error ? { error: result.error } : {}),
+ signal: result.signal,
+ status: result.status ?? 1,
+ stderr: result.stderr ?? '',
+ stdout: result.stdout ?? '',
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify handling of signal-terminated processes so status and messaging stay consistent.
For signal-terminated children, `result.status` is `null` and `result.signal` is set. Forcing `status` to `1` here will cause `replayFailureOutput` to print `exit 1`, losing the signal context and giving a misleading reason for failure. Consider either mapping the signal to a conventional status while still surfacing the signal in the failure output, or leaving `status` null and special‑casing the message (e.g. `Command failed (signal SIGTERM)`).
Suggested implementation:
```typescript
function toSilentCommandResult(result: SpawnSyncReturns<string>): SilentCommandResult {
const status =
result.status !== null && result.status !== undefined
? result.status
: result.signal
? null
: 1;
return {
...(result.error ? { error: result.error } : {}),
signal: result.signal,
status,
stderr: result.stderr ?? '',
stdout: result.stdout ?? '',
};
}
```
To fully implement the suggestion:
1. Update the `SilentCommandResult` type (where it is defined) so that `status` can be `number | null` instead of just `number`, if it is currently restricted to `number`.
2. In the code that prints or logs failures (e.g. the `replayFailureOutput` helper referenced in your comment), special‑case the messaging:
- If `result.signal` is set and `result.status` is `null`, render something like: `Command failed (signal SIGTERM)` instead of `exit 1`.
- Otherwise, keep the existing `exit <status>` message for non‑signal failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+297
to
+301
| function toSilentCommandResult(result: SpawnSyncReturns<string>): SilentCommandResult { | ||
| return { | ||
| ...(result.error ? { error: result.error } : {}), | ||
| signal: result.signal, | ||
| status: result.status ?? 1, |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Clarify handling of signal-terminated processes so status and messaging stay consistent.
For signal-terminated children, result.status is null and result.signal is set. Forcing status to 1 here will cause replayFailureOutput to print exit 1, losing the signal context and giving a misleading reason for failure. Consider either mapping the signal to a conventional status while still surfacing the signal in the failure output, or leaving status null and special‑casing the message (e.g. Command failed (signal SIGTERM)).
Suggested implementation:
function toSilentCommandResult(result: SpawnSyncReturns<string>): SilentCommandResult {
const status =
result.status !== null && result.status !== undefined
? result.status
: result.signal
? null
: 1;
return {
...(result.error ? { error: result.error } : {}),
signal: result.signal,
status,
stderr: result.stderr ?? '',
stdout: result.stdout ?? '',
};
}To fully implement the suggestion:
- Update the
SilentCommandResulttype (where it is defined) so thatstatuscan benumber | nullinstead of justnumber, if it is currently restricted tonumber. - In the code that prints or logs failures (e.g. the
replayFailureOutputhelper referenced in your comment), special‑case the messaging:- If
result.signalis set andresult.statusisnull, render something like:Command failed (signal SIGTERM)instead ofexit 1. - Otherwise, keep the existing
exit <status>message for non‑signal failures.
- If
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
2 participants
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 reusable silent verification command runners and integrate them into the repository-wide verify workflow.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: