fix(e2e): gate scenario fan-out by onboarding+secret support contract#4978
Closed
jyaunches wants to merge 1 commit into
Closed
fix(e2e): gate scenario fan-out by onboarding+secret support contract#4978jyaunches wants to merge 1 commit into
jyaunches wants to merge 1 commit into
Conversation
Addresses two PR Review Advisor 🛠️ Needs-attention findings on HEAD 70df051: 1. Live fan-out includes scenarios without onboarding and secret plumbing (test/e2e-scenario/nemoclaw_scenarios/onboard/dispatch.sh:32): The typed registry declares 12 scenarios whose onboarding profile has no case in dispatch.sh and/or whose requiredSecrets are not declared in the scenario workflows. Live fan-out via e2e-scenarios-all.yaml would either fail at onboarding dispatch ('unsupported onboarding profile') or run without the credential boundary the registry declares. Path chosen (per advisor): filter the generated matrix to fully supported scenarios + add a static contract test. Implementation: - New module test/e2e-scenario/scenarios/runtime-support.ts exporting SUPPORTED_ONBOARDING_IDS (mirrors dispatch.sh case statement), WORKFLOW_AVAILABLE_SECRETS (mirrors workflow secrets:), and isScenarioFullyWired(scenario) which combines them with the compiler's docker-missing onboarding rewrite logic. - buildScenarioMatrix() in run.ts now filters via isScenarioFullyWired and writes a structured per-scenario explanation of skipped scenarios to stderr. The matrix went from 23 entries to 11; the 12 skipped scenarios stay in the registry as roadmap items. - New framework test e2e-scenario-fully-wired.test.ts locks the contract: parses dispatch.sh and the two scenario workflows, asserts SUPPORTED_ONBOARDING_IDS == dispatcher case set, WORKFLOW_AVAILABLE_SECRETS == workflow secrets, every emitted matrix entry is fully wired, every filtered scenario has at least one structured reason. - Updated e2e-scenario-matrix.test.ts to assert matrix == fully-wired subset (was: matrix == every registered scenario). 2. No-docker negative onboarding truncates the framework-seeded context (test/e2e-scenario/nemoclaw_scenarios/onboard/cloud-openclaw-no-docker.sh:35): The TS runner seeds context.env before phase actions, and dispatch-action.sh explicitly forbids workers from calling e2e_context_init because it opens with ': > ctx' and wipes seeded keys. The no-docker negative worker still called e2e_context_init, wiping E2E_SCENARIO/E2E_SANDBOX_NAME/E2E_GATEWAY_URL before state-validation probes (gateway-absent, sandbox-absent) need them. Removed the call. Added a comment block at the same site explaining why workers must not call e2e_context_init, mirroring the contract already documented in dispatch-action.sh. Verified: vitest test/e2e-scenario/ 374/374 (8 new contract tests); typecheck:cli clean; npx tsx scenarios/run.ts --emit-matrix produces 11 fully-wired entries with the 12 filtered scenarios reported on stderr; bash -n + shfmt clean on edited shell file.
Contributor
📝 WalkthroughWalkthroughThis PR introduces a typed "fully-wired" contract for E2E scenarios that validates which scenarios can be executed given the supported onboarding profiles and workflow secrets, applies filtering to the scenario matrix, and enforces invariants bidirectionally across TypeScript constants, bash dispatcher logic, and GitHub workflow configurations. ChangesE2E Scenario Fully-Wired Contract
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 2 worth checking, 1 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-scenario/framework-tests/e2e-scenario-fully-wired.test.ts (1)
78-103: 💤 Low valueConsider clarifying blank-line handling in YAML parser.
The comment at line 92 states "De-dent or blank line ends the block," but the code only sets
inSecrets = falsefor non-blank dedented lines (lines 93-95). Blank lines within a secrets block are skipped but don't terminate parsing. While this works for the documented "stable structure" use case, the implementation doesn't match the comment.♻️ Consider clarifying the logic
Option 1: Update the comment to match the actual behavior:
- // De-dent or blank line ends the block. + // Skip blank lines; dedent ends the block.Option 2: Make blank lines actually end the block if that's the intent:
if (line.trim() === "" || line.search(/\S/) <= secretsIndent) { - // De-dent or blank line ends the block. - if (line.search(/\S/) >= 0 && line.search(/\S/) <= secretsIndent) { - inSecrets = false; - } + // Blank line or dedent ends the secrets block. + inSecrets = false; continue; }Given the stable YAML structure, the current implementation is acceptable.
🤖 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-scenario-fully-wired.test.ts` around lines 78 - 103, The comment in workflowSecretKeys describing "De-dent or blank line ends the block" is inaccurate because the implementation only ends the secrets block on de-dent, while blank lines are skipped; update the comment to accurately describe the current behavior (i.e., blank lines inside the secrets block are ignored and only a dedented non-blank line terminates the block) so the code and comment match; locate the logic in function workflowSecretKeys around the inSecrets/secretsIndent handling and adjust the inline comment accordingly.
🤖 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-scenario-fully-wired.test.ts`:
- Around line 78-103: The comment in workflowSecretKeys describing "De-dent or
blank line ends the block" is inaccurate because the implementation only ends
the secrets block on de-dent, while blank lines are skipped; update the comment
to accurately describe the current behavior (i.e., blank lines inside the
secrets block are ignored and only a dedented non-blank line terminates the
block) so the code and comment match; locate the logic in function
workflowSecretKeys around the inSecrets/secretsIndent handling and adjust the
inline comment accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5fbd4f8f-4a04-48be-acdc-fca844059644
📒 Files selected for processing (5)
test/e2e-scenario/framework-tests/e2e-scenario-fully-wired.test.tstest/e2e-scenario/framework-tests/e2e-scenario-matrix.test.tstest/e2e-scenario/nemoclaw_scenarios/onboard/cloud-openclaw-no-docker.shtest/e2e-scenario/scenarios/run.tstest/e2e-scenario/scenarios/runtime-support.ts
8 tasks
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.
Why
Follow-up to #4380 addressing the two PR Review Advisor 🛠️ Needs-attention findings that were posted after the lgtm[] format fix landed but before the merge could re-fetch the advisor signal:
"Live fan-out includes scenarios without onboarding and secret plumbing" — the typed scenario registry declares 12 scenarios whose onboarding profile has no case in
nemoclaw_scenarios/onboard/dispatch.shand/or whose required secrets are not declared in the scenario workflows. Live fan-out viae2e-scenarios-all.yamlwould either fail at onboarding dispatch withunsupported onboarding profile(Mode A) or run without the credential boundary the registry declares."No-docker negative onboarding truncates the framework-seeded context" — the no-docker negative worker called
e2e_context_init, which opens with: > ctxand wipes seeded keys (E2E_SCENARIO,E2E_SANDBOX_NAME,E2E_GATEWAY_URL) before the state-validation phase's gateway-absent / sandbox-absent probes run. Mirrors a contract already documented indispatch-action.sh.What changed
Finding 1: scenario fan-out support gate
Path chosen (per advisor): filter the generated matrix to fully supported scenarios and add a static contract test.
test/e2e-scenario/scenarios/runtime-support.tsexporting:SUPPORTED_ONBOARDING_IDS— mirrors the bash dispatcher case statement.WORKFLOW_AVAILABLE_SECRETS— mirrorse2e-scenarios.yamlande2e-scenarios-all.yamlsecrets:blocks.isScenarioFullyWired(scenario)— combines the two with the compiler'sdocker-missingonboarding-rewrite logic.buildScenarioMatrix()inrun.tsnow filters viaisScenarioFullyWiredand writes a structured per-scenario explanation of skipped scenarios to stderr. The matrix went from 23 entries to 11; the 12 skipped scenarios stay in the registry as roadmap items.e2e-scenario-fully-wired.test.tslocks the contract:dispatch.shand the two scenario workflows.SUPPORTED_ONBOARDING_IDS== dispatcher case set.WORKFLOW_AVAILABLE_SECRETS== workflow secrets.e2e-scenario-matrix.test.tsto assert matrix == fully-wired subset (was: matrix == every registered scenario).Finding 2: no-docker context init removal
Removed the
e2e_context_initcall incloud-openclaw-no-docker.sh. Added a comment block at the same site explaining why workers must not calle2e_context_init, mirroring the contract already documented indispatch-action.sh.Verification
vitest test/e2e-scenario/— 374/374 (8 new contract tests added)typecheck:cli— cleannpx tsx scenarios/run.ts --emit-matrix— produces 11 fully-wired entries with the 12 filtered scenarios reported on stderrbash -n+shfmtclean on edited shell fileOut of scope
The 12 filtered scenarios are not removed from the typed registry — they stay as roadmap items. Adding their dispatcher cases + secret plumbing belongs to follow-up work scoped per integration (slack, discord, telegram, brave, openai-compatible, etc.). The framework test will fail-loud the moment any of them lands in the dispatcher without also being added to
SUPPORTED_ONBOARDING_IDS.Refs #4380.
Summary by CodeRabbit
Tests
Bug Fixes