feat(quality/pr6): BDD lifecycle specs + smoke/regression scripts + PR template#117
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces a plugin lifecycle quality review PR template, smoke test script, regression guard script, and comprehensive BDD test suite covering the entire plugin lifecycle workflow from creation through installation and uninstallation. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant CLI
participant Filesystem
participant Validator
participant Publisher
participant CDN
participant InstallSystem
Developer->>CLI: Create plugin
CLI->>Filesystem: Generate scaffold
Developer->>CLI: Build frontend
Filesystem->>Filesystem: Produce dist/production
Developer->>CLI: Package plugin
Filesystem->>Filesystem: Copy manifest & artifacts
Validator->>Filesystem: Validate manifest
Validator->>Validator: Check plugin name
Developer->>Publisher: Publish to registry
Publisher->>Publisher: Pre-publish checks
Publisher->>CDN: Deploy UMD bundle
Developer->>InstallSystem: Install plugin
InstallSystem->>InstallSystem: pending → installing
InstallSystem->>InstallSystem: Run postInstall hook
InstallSystem->>InstallSystem: installing → installed
Developer->>InstallSystem: Uninstall plugin
InstallSystem->>InstallSystem: Run preUninstall hook
InstallSystem->>Filesystem: Remove plugin artifacts
InstallSystem->>InstallSystem: Uninstall complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
|
There was a problem hiding this comment.
Pull request overview
Adds new quality-program assets (BDD-style lifecycle specs, smoke/regression guard scripts, and a PR review template) intended to improve plugin lifecycle validation and reviewer consistency.
Changes:
- Introduces a new vitest-based “lifecycle BDD” spec file under
tests/. - Adds two CI-invocable bash scripts for lifecycle smoke checks and regression guarding.
- Adds a PR template with a checklist and 100-point scoring rubric for lifecycle-quality reviews.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts |
Adds 15 Given/When/Then-style scenarios for lifecycle phases (currently not wired into the Nx test graph and contains a test-order dependency). |
bin/lifecycle-smoke.sh |
Adds a smoke script for SDK build + bundle/file checks (currently skips all core plugins due to build:umd gating). |
bin/regression-guard.sh |
Adds a regression guard matrix + structural checks (currently largely SKIPs core plugin checks and has a PR-description mismatch). |
.github/PULL_REQUEST_TEMPLATE/plugin-lifecycle-quality.md |
Adds a lifecycle-quality PR review template (table formatting issue with leading ` |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+79
to
+90
| it('Given a built frontend, When package runs, Then a ZIP with plugin.json and UMD bundle is produced', async () => { | ||
| const pluginDir = path.join(tmpDir, 'bdd-test-plugin'); | ||
| const prodDir = path.join(pluginDir, 'frontend', 'dist', 'production'); | ||
| await fs.ensureDir(prodDir); | ||
| await fs.writeFile(path.join(prodDir, 'bdd-test-plugin.js'), VALID_UMD); | ||
|
|
||
| // Simulate packaging | ||
| const pkgDir = path.join(tmpDir, 'pkg'); | ||
| await fs.ensureDir(pkgDir); | ||
| await fs.copy(path.join(pluginDir, 'plugin.json'), path.join(pkgDir, 'plugin.json')); | ||
| await fs.copy(prodDir, path.join(pkgDir, 'frontend', 'production')); | ||
|
|
There was a problem hiding this comment.
The packaging scenario copies plugin.json from pluginDir, but this test never creates that file (or reuses a setup hook to ensure it exists). This will fail when the packaging test is run in isolation (e.g., vitest -t "Plugin Packaging"). Consider creating/writing plugin.json as part of this test or adding a shared beforeEach setup that ensures pluginDir/plugin.json exists for every scenario that relies on it.
Comment on lines
+1
to
+12
| /** | ||
| * Plugin Lifecycle — Behavior-Driven Feature Specs | ||
| * | ||
| * Given/When/Then scenarios covering the full lifecycle: | ||
| * create → build → package → validate → publish → CDN-serve → install → uninstall | ||
| * | ||
| * These tests are designed to run against a live-ish environment (mocked services | ||
| * for the initial CI integration, with a toggle for real-service testing in nightly). | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; | ||
| import fs from 'fs-extra'; |
There was a problem hiding this comment.
This test suite lives under the repo-level tests/ folder, but the workspace test entrypoint is nx run-many --target=test --all and there is no Nx project/package that includes tests/lifecycle-bdd. As a result, these specs are likely not executed in CI or by npm test despite the .test.ts name. Consider moving these tests into an existing Nx project that runs vitest (or creating a dedicated Nx project for tests/ and wiring it into CI).
| | Regression safety & backward compat | 20 | ___ | ___ | | ||
| | Security & resilience | 15 | ___ | ___ | | ||
| | Maintainability & observability | 10 | ___ | ___ | | ||
| | **Total** | **100** | | **___** | |
There was a problem hiding this comment.
The scorecard table rows start with || (double pipe), which creates an extra empty first column in GitHub-flavored Markdown. If you want a 4-column table, use a single leading | on each row so the columns align as intended.
Suggested change
| | **Total** | **100** | | **___** | | |
| | **Total** | **100** | **N/A** | **___** | |
…R template BDD feature specs (tests/lifecycle-bdd/plugin-lifecycle.feature.ts — 15 tests): - Given/When/Then scenarios for all 6 lifecycle phases: create, package, validate, publish, CDN-serve, install/uninstall - Covers mount uniqueness, packaging format rejection, manifest validation, P2002 concurrent publish, MIME type serving, install/uninstall ordering Script-driven automation: - bin/lifecycle-smoke.sh: SDK build check, core plugin UMD existence, structural file integrity, health-check invocation - bin/regression-guard.sh: SDK build + per-core-plugin typecheck/test/UMD, structural integrity across all 5 core plugins Both scripts: exit 1 on any failure, produce pass/fail per-check output, integrated into the CI lifecycle-tests job added in PR1. Governance: - .github/PULL_REQUEST_TEMPLATE/plugin-lifecycle-quality.md: expert review checklist, 100-point scoring rubric table, regression impact statement, compatibility test evidence, and coverage delta fields Scope: tests/ + bin/ scripts + PR template only. No production code changed. Net LOC: ~513 additions | No regression risk. Co-authored-by: Cursor <cursoragent@cursor.com>
…overy Vitest's default include pattern requires .test.ts or .spec.ts suffixes. Rename plugin-lifecycle.feature.ts → plugin-lifecycle.feature.test.ts so npx vitest run --dir tests/lifecycle-bdd picks it up automatically without requiring a custom --include flag. Co-authored-by: Cursor <cursoragent@cursor.com>
- Import and exercise real validateManifest, validatePluginName, createDefaultManifest from plugin-sdk - Add CLI discoverability tests: invoke `naap-plugin --help` and `naap-plugin create --help` via execa - Replace inline value-assertion validation tests with actual SDK calls - 19 tests pass (2 real CLI, 4 real SDK, rest structural) Co-authored-by: Cursor <cursoragent@cursor.com>
seanhanca
force-pushed
the
quality/pr6-bdd-reporting
branch
from
8a93e39 to
15fa124
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts (5)
163-180: Tautological tests — assertions are self-fulfilling.These tests verify hardcoded values against themselves:
- Lines 165-168:
passed: trueis hardcoded, so the "no failures" assertion always passes.- Lines 176-178:
error.codeis set to'P2002'then immediately checked for'P2002'.These tests document expected behavior but don't verify actual publish logic. Consider mocking/stubbing the publish service and testing real error paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts` around lines 163 - 180, The tests use tautological assertions: the "checks" array sets passed: true and then asserts no failures, and the P2002 case sets error.code to 'P2002' then asserts it equals 'P2002'; replace these with real assertions by invoking the actual publish logic (or a mocked publish service) and asserting outcomes—e.g., call the publish method and verify that Manifest validation and Version format checks fail or pass based on FIXTURE_MANIFEST, or mock the publish/version-create to throw a Prisma P2002 error and assert the HTTP/handler returns a 409; locate and update the tests around the checks array and the P2002 simulation (variables checks, FIXTURE_MANIFEST, error, isUniqueViolation) to use real or mocked behavior instead of hardcoded values.
69-96: Tests validate fixture setup, not actual SDK create functionality.These tests write fixtures to disk and read them back, which validates the test infrastructure but doesn't exercise the actual
createcommand. Consider adding tests that invokenpx tsx cliPath create ...to test real plugin scaffolding behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts` around lines 69 - 96, The tests in plugin-lifecycle.feature.test.ts only write and read fixture files (validating test infra) instead of invoking the real CLI create flow; update the test to actually run the SDK create command by spawning the CLI (e.g., run "npx tsx <cliPath> create ..." or use the existing cliPath test helper) and assert the generated project structure and contents (manifest, frontend mount behavior) rather than writing files directly—modify the relevant test case(s) that currently manipulate pluginDir/frontend/src and use process spawn or execa to call the create command, then read and assert files produced by that command (referencing the test file name, the test case description, and the create CLI invocation).
154-158: Tautological test — checks a constant defined in the same file.This test verifies that
VALID_UMD(defined at line 32) contains 'mount' and 'exports', but sinceVALID_UMDis a constant in this file, the test will always pass. Consider testing against actual UMD bundle output from the build process, or testing a real validation function that checks UMD content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts` around lines 154 - 158, The test currently asserts that the local constant VALID_UMD contains 'mount' and 'exports' which is tautological; change the "Given a UMD bundle, When content is validated, Then mount function presence is checked" test to operate on real UMD output or the validation logic instead of the constant: either (a) load or build an actual UMD bundle (from a fixture or the build output) and assert that its string contains 'mount' and 'exports', or (b) call the real validation function (e.g., validateUmdContent / validateUmdBundle) with the real bundle content and assert the function returns the expected result; replace references to VALID_UMD in this spec with the loaded bundle or the validation function call.
185-202: Tautological tests — no CDN code exercised.Both tests define local data structures and verify them inline:
mimeMapis defined then immediately checked for known values.isProd && hasHashare both hardcoded totrue, soccalways contains 'immutable'.These serve as documentation of expected behavior but don't test actual CDN serving logic. Consider integration tests that hit real/mocked CDN endpoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts` around lines 185 - 202, The two tests in plugin-lifecycle.feature.test.ts are tautological (they only assert local values: mimeMap, isProd/hasHash -> cc) and do not exercise CDN logic; replace them to call the actual CDN/middleware utilities or endpoints instead of asserting hardcoded locals — e.g., invoke the server route or the functions that compute MIME type and Cache-Control and assert their responses; refer to the symbols mimeMap, isProd, hasHash and cc in the diff as the offending locals to remove, and instead call the real handler or helper (the CDN route handler or getMimeType/getCacheControl utility) or perform an HTTP request against a test server or mocked CDN to verify correct MIME and immutable Cache-Control behavior.
207-243: Tautological tests documenting lifecycle state machine.These tests construct expected data structures inline and verify them:
- Line 209-211: Hardcoded
transitionsarray checked for first/last values.- Line 215-224: Manually pushed strings checked for ordering.
- Lines 228-235 and 238-241: Hardcoded flags controlling expected outcomes.
These effectively document the intended state machine behavior but don't exercise real lifecycle code. Consider integration tests that invoke actual install/uninstall handlers and verify database state or hook invocations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts` around lines 207 - 243, The current tests in the "Given a published plugin..." / "Given an installed plugin..." / "Given a postInstall hook failure..." / "Given a preUninstall hook failure..." specs are tautological and do not exercise the lifecycle logic; replace them with integration-style tests that invoke the real install/uninstall handlers (e.g., call your installPlugin/uninstallPlugin or WorkflowPlugin.install/WorkflowPlugin.uninstall entry points), simulate hook outcomes (invoke or stub runPreInstall/runPostInstall/runPreUninstall hooks), and assert observable side effects such as database records via getInstallationStatus/getInstallationRecord and external effects like disableWorkflowPlugin and unregisterRoles being called (use spies/mocks where appropriate); ensure tests assert ordering by checking timestamps or spy call order rather than comparing hardcoded arrays or boolean flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/regression-guard.sh`:
- Around line 65-71: The redirection order in the typecheck command is reversed
(2>&1 >/dev/null) so stderr is not being silenced correctly; update the command
invocation inside the conditional that runs npm run typecheck (the subshell
using (cd "$FRONTEND_DIR" && npm run typecheck ...)) to use the correct combined
redirection >/dev/null 2>&1 so both stdout and stderr are suppressed while
preserving the exit status checked by the if.
- Around line 74-82: The npm test invocation in the conditional uses the wrong
redirect order ("2>&1 >/dev/null") so stderr is redirected to the original
stdout instead of both streams going to /dev/null; update the command in the if
block that runs (cd "$FRONTEND_DIR" && npm test --if-present 2>&1 >/dev/null) to
redirect stdout first and then stderr (e.g., use >/dev/null 2>&1) so both stdout
and stderr from npm test are silenced, leaving the surrounding logic (variables
FRONTEND_DIR, plugin and the pass/fail/skip functions) unchanged.
- Around line 40-44: The shell conditional that runs the plugin-sdk build uses
the wrong redirection order: in the if-statement containing (cd
"$ROOT_DIR/packages/plugin-sdk" && npm run build 2>&1 >/dev/null) stderr is sent
to the terminal because 2>&1 happens before stdout is redirected; update the
build command inside that conditional to redirect stdout first and then stderr
(i.e., use >/dev/null 2>&1) so both stdout and stderr are suppressed when
running the npm run build check.
- Around line 84-93: The npm invocation inside the if block that checks
FRONTEND_DIR currently misorders shell redirections (the subshell condition
using cd "$FRONTEND_DIR" && npm run build:umd ...), causing stderr/stdout
handling to be incorrect; fix it by changing the redirection order so stdout is
redirected first and stderr follows (i.e., redirect stdout to /dev/null then
redirect stderr to the same place) in the subshell command used for the UMD
build check (the line containing npm run build:umd), leaving the surrounding
pass/fail/skip logic and variables like plugin intact.
---
Nitpick comments:
In `@tests/lifecycle-bdd/plugin-lifecycle.feature.test.ts`:
- Around line 163-180: The tests use tautological assertions: the "checks" array
sets passed: true and then asserts no failures, and the P2002 case sets
error.code to 'P2002' then asserts it equals 'P2002'; replace these with real
assertions by invoking the actual publish logic (or a mocked publish service)
and asserting outcomes—e.g., call the publish method and verify that Manifest
validation and Version format checks fail or pass based on FIXTURE_MANIFEST, or
mock the publish/version-create to throw a Prisma P2002 error and assert the
HTTP/handler returns a 409; locate and update the tests around the checks array
and the P2002 simulation (variables checks, FIXTURE_MANIFEST, error,
isUniqueViolation) to use real or mocked behavior instead of hardcoded values.
- Around line 69-96: The tests in plugin-lifecycle.feature.test.ts only write
and read fixture files (validating test infra) instead of invoking the real CLI
create flow; update the test to actually run the SDK create command by spawning
the CLI (e.g., run "npx tsx <cliPath> create ..." or use the existing cliPath
test helper) and assert the generated project structure and contents (manifest,
frontend mount behavior) rather than writing files directly—modify the relevant
test case(s) that currently manipulate pluginDir/frontend/src and use process
spawn or execa to call the create command, then read and assert files produced
by that command (referencing the test file name, the test case description, and
the create CLI invocation).
- Around line 154-158: The test currently asserts that the local constant
VALID_UMD contains 'mount' and 'exports' which is tautological; change the
"Given a UMD bundle, When content is validated, Then mount function presence is
checked" test to operate on real UMD output or the validation logic instead of
the constant: either (a) load or build an actual UMD bundle (from a fixture or
the build output) and assert that its string contains 'mount' and 'exports', or
(b) call the real validation function (e.g., validateUmdContent /
validateUmdBundle) with the real bundle content and assert the function returns
the expected result; replace references to VALID_UMD in this spec with the
loaded bundle or the validation function call.
- Around line 185-202: The two tests in plugin-lifecycle.feature.test.ts are
tautological (they only assert local values: mimeMap, isProd/hasHash -> cc) and
do not exercise CDN logic; replace them to call the actual CDN/middleware
utilities or endpoints instead of asserting hardcoded locals — e.g., invoke the
server route or the functions that compute MIME type and Cache-Control and
assert their responses; refer to the symbols mimeMap, isProd, hasHash and cc in
the diff as the offending locals to remove, and instead call the real handler or
helper (the CDN route handler or getMimeType/getCacheControl utility) or perform
an HTTP request against a test server or mocked CDN to verify correct MIME and
immutable Cache-Control behavior.
- Around line 207-243: The current tests in the "Given a published plugin..." /
"Given an installed plugin..." / "Given a postInstall hook failure..." / "Given
a preUninstall hook failure..." specs are tautological and do not exercise the
lifecycle logic; replace them with integration-style tests that invoke the real
install/uninstall handlers (e.g., call your installPlugin/uninstallPlugin or
WorkflowPlugin.install/WorkflowPlugin.uninstall entry points), simulate hook
outcomes (invoke or stub runPreInstall/runPostInstall/runPreUninstall hooks),
and assert observable side effects such as database records via
getInstallationStatus/getInstallationRecord and external effects like
disableWorkflowPlugin and unregisterRoles being called (use spies/mocks where
appropriate); ensure tests assert ordering by checking timestamps or spy call
order rather than comparing hardcoded arrays or boolean flags.
Comment on lines
+40
to
+44
| if (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build 2>&1 >/dev/null); then | ||
| pass "packages/plugin-sdk builds" | ||
| else | ||
| fail "packages/plugin-sdk build failed" | ||
| fi |
There was a problem hiding this comment.
Incorrect redirect order — stderr escapes to terminal.
The redirect 2>&1 >/dev/null first sends stderr to stdout's current destination (terminal), then redirects stdout to /dev/null. This means error output still appears on the terminal rather than being suppressed. The correct order is >/dev/null 2>&1.
🔧 Proposed fix
-if (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build 2>&1 >/dev/null); then
+if (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build >/dev/null 2>&1); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build 2>&1 >/dev/null); then | |
| pass "packages/plugin-sdk builds" | |
| else | |
| fail "packages/plugin-sdk build failed" | |
| fi | |
| if (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build >/dev/null 2>&1); then | |
| pass "packages/plugin-sdk builds" | |
| else | |
| fail "packages/plugin-sdk build failed" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 40-40: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/regression-guard.sh` around lines 40 - 44, The shell conditional that
runs the plugin-sdk build uses the wrong redirection order: in the if-statement
containing (cd "$ROOT_DIR/packages/plugin-sdk" && npm run build 2>&1 >/dev/null)
stderr is sent to the terminal because 2>&1 happens before stdout is redirected;
update the build command inside that conditional to redirect stdout first and
then stderr (i.e., use >/dev/null 2>&1) so both stdout and stderr are suppressed
when running the npm run build check.
Comment on lines
+65
to
+71
| if grep -q '"typecheck"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | ||
| if (cd "$FRONTEND_DIR" && npm run typecheck 2>&1 >/dev/null); then | ||
| pass "$plugin — typecheck" | ||
| else | ||
| fail "$plugin — typecheck failed" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Same redirect order issue.
🔧 Proposed fix
- if (cd "$FRONTEND_DIR" && npm run typecheck 2>&1 >/dev/null); then
+ if (cd "$FRONTEND_DIR" && npm run typecheck >/dev/null 2>&1); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if grep -q '"typecheck"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm run typecheck 2>&1 >/dev/null); then | |
| pass "$plugin — typecheck" | |
| else | |
| fail "$plugin — typecheck failed" | |
| fi | |
| fi | |
| if grep -q '"typecheck"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm run typecheck >/dev/null 2>&1); then | |
| pass "$plugin — typecheck" | |
| else | |
| fail "$plugin — typecheck failed" | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 66-66: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/regression-guard.sh` around lines 65 - 71, The redirection order in the
typecheck command is reversed (2>&1 >/dev/null) so stderr is not being silenced
correctly; update the command invocation inside the conditional that runs npm
run typecheck (the subshell using (cd "$FRONTEND_DIR" && npm run typecheck ...))
to use the correct combined redirection >/dev/null 2>&1 so both stdout and
stderr are suppressed while preserving the exit status checked by the if.
Comment on lines
+74
to
+82
| if grep -q '"test"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | ||
| if (cd "$FRONTEND_DIR" && npm test --if-present 2>&1 >/dev/null); then | ||
| pass "$plugin — tests" | ||
| else | ||
| fail "$plugin — tests failed" | ||
| fi | ||
| else | ||
| skip "$plugin — no test script" | ||
| fi |
There was a problem hiding this comment.
Same redirect order issue.
🔧 Proposed fix
- if (cd "$FRONTEND_DIR" && npm test --if-present 2>&1 >/dev/null); then
+ if (cd "$FRONTEND_DIR" && npm test --if-present >/dev/null 2>&1); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if grep -q '"test"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm test --if-present 2>&1 >/dev/null); then | |
| pass "$plugin — tests" | |
| else | |
| fail "$plugin — tests failed" | |
| fi | |
| else | |
| skip "$plugin — no test script" | |
| fi | |
| if grep -q '"test"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm test --if-present >/dev/null 2>&1); then | |
| pass "$plugin — tests" | |
| else | |
| fail "$plugin — tests failed" | |
| fi | |
| else | |
| skip "$plugin — no test script" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 75-75: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/regression-guard.sh` around lines 74 - 82, The npm test invocation in the
conditional uses the wrong redirect order ("2>&1 >/dev/null") so stderr is
redirected to the original stdout instead of both streams going to /dev/null;
update the command in the if block that runs (cd "$FRONTEND_DIR" && npm test
--if-present 2>&1 >/dev/null) to redirect stdout first and then stderr (e.g.,
use >/dev/null 2>&1) so both stdout and stderr from npm test are silenced,
leaving the surrounding logic (variables FRONTEND_DIR, plugin and the
pass/fail/skip functions) unchanged.
Comment on lines
+84
to
+93
| # UMD build (if configured) | ||
| if grep -q '"build:umd"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | ||
| if (cd "$FRONTEND_DIR" && npm run build:umd 2>&1 >/dev/null); then | ||
| pass "$plugin — UMD build" | ||
| else | ||
| fail "$plugin — UMD build failed" | ||
| fi | ||
| else | ||
| skip "$plugin — no build:umd script" | ||
| fi |
There was a problem hiding this comment.
Same redirect order issue.
🔧 Proposed fix
- if (cd "$FRONTEND_DIR" && npm run build:umd 2>&1 >/dev/null); then
+ if (cd "$FRONTEND_DIR" && npm run build:umd >/dev/null 2>&1); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # UMD build (if configured) | |
| if grep -q '"build:umd"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm run build:umd 2>&1 >/dev/null); then | |
| pass "$plugin — UMD build" | |
| else | |
| fail "$plugin — UMD build failed" | |
| fi | |
| else | |
| skip "$plugin — no build:umd script" | |
| fi | |
| # UMD build (if configured) | |
| if grep -q '"build:umd"' "$FRONTEND_DIR/package.json" 2>/dev/null; then | |
| if (cd "$FRONTEND_DIR" && npm run build:umd >/dev/null 2>&1); then | |
| pass "$plugin — UMD build" | |
| else | |
| fail "$plugin — UMD build failed" | |
| fi | |
| else | |
| skip "$plugin — no build:umd script" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 86-86: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/regression-guard.sh` around lines 84 - 93, The npm invocation inside the
if block that checks FRONTEND_DIR currently misorders shell redirections (the
subshell condition using cd "$FRONTEND_DIR" && npm run build:umd ...), causing
stderr/stdout handling to be incorrect; fix it by changing the redirection order
so stdout is redirected first and stderr follows (i.e., redirect stdout to
/dev/null then redirect stderr to the same place) in the subshell command used
for the UMD build check (the line containing npm run build:umd), leaving the
surrounding pass/fail/skip logic and variables like plugin intact.
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
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
bin/lifecycle-smoke.sh: checks SDK build, UMD bundles, file integritybin/regression-guard.sh: typecheck + test + build for 5 core pluginsSupersedes #109 (rebuilt from
mainwith clean scope).Files Changed (4)
tests/lifecycle-bdd/plugin-lifecycle.feature.test.tsbin/lifecycle-smoke.shbin/regression-guard.sh.github/PULL_REQUEST_TEMPLATE/plugin-lifecycle-quality.mdTest Evidence
.feature.test.tsnaming for vitest auto-discoveryRegression Impact
Made with Cursor
Summary by CodeRabbit
Tests
Chores