silent verify script#282
Open
jasonmorais wants to merge 17 commits into
Open
Conversation
Contributor
Reviewer's GuideIntroduces a new @cellix/local-dev package that provides silent command runners and a fluent verification sequence API, rewires the root verify workflow to use a new Node-based script built on these utilities, and adjusts the agent formatter to suppress output on success while still logging issue counts on failure, alongside minor export/import tidy-ups and docs/tests for the new package. Sequence diagram for the new silent verify workflowsequenceDiagram
actor Dev
participant VerifyScript as verify.ts
participant VerifySeq as VerificationSequence
participant SeqRunner as runSilentCommandSequence
participant CmdRunner as runSilentCommand
Dev->>VerifyScript: runVerifyCommand()
VerifyScript->>VerifySeq: run()
VerifySeq->>SeqRunner: runSilentCommandSequence(options)
loop steps in sequence
SeqRunner->>CmdRunner: runSilentCommand(step)
alt [command succeeds]
CmdRunner-->>SeqRunner: SilentCommandResult(status=0)
else [command fails]
CmdRunner-->>SeqRunner: SilentCommandResult(status!=0)
SeqRunner-->>VerifySeq: SilentCommandSequenceResult(step, status!=0)
VerifySeq-->>VerifyScript: result
VerifyScript-->>Dev: process.exitCode = status
end
end
SeqRunner-->>VerifySeq: SilentCommandSequenceResult(status=0)
VerifySeq-->>VerifyScript: result(status=0)
VerifyScript->>Dev: write verify passed
VerifyScript->>Dev: process.exitCode = 0
Sequence diagram for updated agent formatter output logicsequenceDiagram
participant Formatter as AgentFormatter
participant Runner as CucumberRunner
Runner->>Formatter: onTestRunFinished(testRunFinished)
alt [issueCount === 0]
Formatter-->>Runner: return (no log)
else [issueCount > 0]
Formatter->>Formatter: log("\n--- (Agent) Results ---\n")
Formatter->>Formatter: log(statusCounts)
Formatter->>Formatter: log(duration)
Formatter->>Formatter: log("Issues: " + issueCount)
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.
# 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
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 it into the repo’s verify workflow while keeping existing checks intact.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation: