fix(e2e): run full repo scenario cli build#4002
Conversation
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used trusted gathered GitHub metadata and read-only repository inspection; no scripts, tests, package-manager commands, or E2E jobs were executed by this advisor.; No linked issue text or comments were available, so acceptance mapping is limited to untrusted PR body clauses and diff/test evidence.; CI and E2E results were still in progress at review time, so final merge readiness cannot be determined from completed checks.; Open PR overlap information was available only from trusted metadata; the overlapping PR diffs were not reviewed in detail. Full advisor summaryPR Review AdvisorBase: The patch targets existing E2E install/test files and appears directionally correct, but merge is blocked by pending CI/mergeState and pending E2E-adjacent checks for an install-path change. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
📝 WalkthroughWalkthroughThe install script for ChangesCLI Build Migration to npm run build:cli
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/scenario-framework-tests/e2e-lib-helpers.test.ts (1)
824-825: ⚡ Quick winConsider adding explicit assertion for build:cli script existence.
The current code uses optional chaining with a fallback to empty string. If
package.jsonis missing thebuild:cliscript, line 825's assertion will fail with a generic message. Adding an explicit check would improve test diagnostics.🧪 Suggested improvement for clearer test failure messages
const buildScript = JSON.parse(fs.readFileSync(path.join(REPO_ROOT, "package.json"), "utf8")).scripts?.["build:cli"] ?? ""; +expect(buildScript, "package.json should define scripts.build:cli").not.toBe(""); expect(buildScript).toContain("generate-oclif-metadata-manifest.js");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/scenario-framework-tests/e2e-lib-helpers.test.ts` around lines 824 - 825, Test currently falls back to empty string so failure message is generic; add an explicit assertion that package.json contains a scripts object with a "build:cli" key before asserting its contents. Locate the code that reads package.json (the buildScript variable and JSON.parse call) and add a prior assertion like checking packageJson.scripts exists and Object.prototype.hasOwnProperty.call(packageJson.scripts, "build:cli") (or expect(packageJson.scripts).toHaveProperty("build:cli")) so the test fails with a clear message, then keep the existing expect(buildScript).toContain(...) content check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/scenario-framework-tests/e2e-lib-helpers.test.ts`:
- Around line 824-825: Test currently falls back to empty string so failure
message is generic; add an explicit assertion that package.json contains a
scripts object with a "build:cli" key before asserting its contents. Locate the
code that reads package.json (the buildScript variable and JSON.parse call) and
add a prior assertion like checking packageJson.scripts exists and
Object.prototype.hasOwnProperty.call(packageJson.scripts, "build:cli") (or
expect(packageJson.scripts).toHaveProperty("build:cli")) so the test fails with
a clear message, then keep the existing expect(buildScript).toContain(...)
content check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c250d466-1fc6-462c-b326-7bfb8da9f530
📒 Files selected for processing (2)
test/e2e/nemoclaw_scenarios/install/repo-current.shtest/e2e/scenario-framework-tests/e2e-lib-helpers.test.ts
Summary
npm run build:clifor repo-current scenario installs instead of rawtsc.Context
The
E2E / Scenario Runner / Allrun on main failed repo-based scenarios becauserepo-current.shcompiled TypeScript directly and skippedgenerate-oclif-metadata-manifest.js, leavingdist/lib/cli/oclif-command-metadata.generated.jsonabsent at first CLI invocation.Verification
bash -n test/e2e/nemoclaw_scenarios/install/repo-current.shnpm run build:clinpm test -- --run test/e2e/scenario-framework-tests/e2e-lib-helpers.test.ts -t repo_current_installnpm test -- --run test/e2e/scenario-framework-tests/e2e-scenario-first-migration.test.ts test/e2e/scenario-framework-tests/e2e-scenario-resolver.test.ts test/e2e/scenario-framework-tests/e2e-scenarios-workflow.test.tsNote: full
e2e-lib-helpers.test.tsstill has two pre-existing unrelated failures in this worktree (security_policy_credentials_helper_should_fail_on_missing_policy_preset,scenario_dry_run_should_trace_helper_sequence_in_order).Summary by CodeRabbit
Tests
Chores