fix(e2e): remove scenario advisor yaml dependency#4081
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces the ChangesYAML Parsing Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/e2e-advisor/scenarios.mts (1)
300-346: ⚡ Quick winValidate regex indentation expectations against
scenarios.yaml
parseScenarioSection’s indentation-specific regexes line up with the actual YAML intest/e2e/nemoclaw_scenarios/scenarios.yaml:
- No tabs found (parser’s “spaces-only” assumption holds for this file)
- Scenario headings match the expected
^ <id>:$shapesuites:matches^ suites:and list items match^ - <item>- No YAML anchors/aliases detected on
suites/runner_requirementsfieldsThe parser will still fail silently if future YAML deviates from this exact formatting (spacing/tabs/other YAML constructs), so adding validation/error reporting (or using a real YAML parser) would make it more robust.
🤖 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 `@tools/e2e-advisor/scenarios.mts` around lines 300 - 346, parseScenarioSection assumes strict space-based indentation and exact line formats, which will silently mis-parse if scenarios.yaml deviates; update parseScenarioSection to validate and surface errors (or switch to a YAML parser): either (1) replace the hand-rolled line/regex logic in parseScenarioSection with a proper YAML parse of the extracted section (using a YAML library) to populate scenarios[currentId].suites and .runner_requirements, or (2) add explicit validation checks after parsing (e.g., detect unexpected indentation, missing currentId before list items, or unrecognized lines) and throw/report a clear error referencing the section name and currentId so formatting issues are visible; reference the parseScenarioSection function and the scenarios/currentId/suites/runner_requirements symbols when making changes.
🤖 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.
Inline comments:
In `@tools/e2e-advisor/scenarios.mts`:
- Around line 348-366: parseSuiteScripts currently uses regexes to parse the
"suites" section line-by-line so YAML aliases like "steps: *id" are ignored
(causing 6 suites to appear empty); update parseSuiteScripts to use a proper
YAML parser that resolves aliases (or at minimum detect a
"steps:\s*\*[A-Za-z0-9_-]+" pattern and follow the alias lookup) so script lists
are populated. Specifically, replace the indentation/regex parsing in
parseSuiteScripts (and its dependency extractTopLevelSection if needed) with a
YAML parse of the suites block (e.g., parse into objects and read suiteId ->
steps/script arrays) or add logic to resolve alias references from the same YAML
content before returning suites.
---
Nitpick comments:
In `@tools/e2e-advisor/scenarios.mts`:
- Around line 300-346: parseScenarioSection assumes strict space-based
indentation and exact line formats, which will silently mis-parse if
scenarios.yaml deviates; update parseScenarioSection to validate and surface
errors (or switch to a YAML parser): either (1) replace the hand-rolled
line/regex logic in parseScenarioSection with a proper YAML parse of the
extracted section (using a YAML library) to populate scenarios[currentId].suites
and .runner_requirements, or (2) add explicit validation checks after parsing
(e.g., detect unexpected indentation, missing currentId before list items, or
unrecognized lines) and throw/report a clear error referencing the section name
and currentId so formatting issues are visible; reference the
parseScenarioSection function and the
scenarios/currentId/suites/runner_requirements symbols when making changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d5a699c-d3f9-4dd4-9aa8-65aba3370e37
📒 Files selected for processing (1)
tools/e2e-advisor/scenarios.mts
Comment on lines
+348
to
+366
| function parseSuiteScripts(text: string): Record<string, string[]> { | ||
| const section = extractTopLevelSection(text, "suites"); | ||
| const suites: Record<string, string[]> = {}; | ||
| let currentId: string | undefined; | ||
|
|
||
| for (const line of section.split(/\r?\n/)) { | ||
| const suiteMatch = line.match(/^ ([A-Za-z0-9_.-]+):\s*$/); | ||
| if (suiteMatch) { | ||
| currentId = suiteMatch[1]; | ||
| suites[currentId] = []; | ||
| continue; | ||
| } | ||
| if (!currentId) continue; | ||
| const scriptMatch = line.match(/^ script:\s*([A-Za-z0-9_./-]+)\s*$/); | ||
| if (scriptMatch) suites[currentId].push(scriptMatch[1]); | ||
| } | ||
|
|
||
| return suites; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify custom parser correctly extracts suite scripts
SUITES_FILE="test/e2e/validation_suites/suites.yaml"
if [ ! -f "$SUITES_FILE" ]; then
echo "ERROR: $SUITES_FILE not found"
exit 1
fi
echo "=== Checking for tabs ==="
if grep -P '^\t' "$SUITES_FILE"; then
echo "ERROR: Found tabs. Parser expects spaces only."
exit 1
fi
echo "✓ No tabs found"
echo ""
echo "=== Sample suite entries (should match pattern: 2 spaces + name + colon) ==="
grep -P '^ [A-Za-z0-9_.-]+:\s*$' "$SUITES_FILE" | head -5
echo ""
echo "=== Sample script entries (should match pattern: 6 spaces + 'script:' + path) ==="
grep -P '^ script:\s*[A-Za-z0-9_./-]+\s*$' "$SUITES_FILE" | head -10
echo ""
echo "=== Checking for YAML anchors on steps (parser won't follow aliases) ==="
if grep -P '^ steps:.*[&*]' "$SUITES_FILE"; then
echo "WARNING: Found anchors/aliases on steps. Parser won't follow alias references."
fi
echo ""
echo "=== Count of suite definitions vs script extractions ==="
SUITE_COUNT=$(grep -cP '^ [A-Za-z0-9_.-]+:\s*$' "$SUITES_FILE")
SCRIPT_COUNT=$(grep -cP '^ script:\s*[A-Za-z0-9_./-]+\s*$' "$SUITES_FILE")
echo "Suites: $SUITE_COUNT"
echo "Scripts: $SCRIPT_COUNT"
echo "✓ Verification complete"Repository: NVIDIA/NemoClaw
Length of output: 1293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SUITES_FILE="test/e2e/validation_suites/suites.yaml"
python3 - <<'PY'
import re, sys, collections
path="test/e2e/validation_suites/suites.yaml"
text=open(path,"r",encoding="utf-8").read()
# Extract top-level 'suites:' section (best-effort)
m=re.search(r'(?ms)^suites:\s*\n(.*)$', text)
if not m:
print("Could not find 'suites:' top-level key")
sys.exit(1)
section=m.group(1)
suite_re=re.compile(r'^ ([A-Za-z0-9_.-]+):\s*$')
script_re=re.compile(r'^ script:\s*([A-Za-z0-9_./-]+)\s*$')
steps_alias_re=re.compile(r'^ steps:\s*\*')
suites_scripts=collections.defaultdict(list)
suites_has_steps_alias=set()
current=None
for line in section.splitlines():
sm=suite_re.match(line)
if sm:
current=sm.group(1)
continue
if current is None:
continue
if steps_alias_re.match(line):
suites_has_steps_alias.add(current)
sm2=script_re.match(line)
if sm2:
suites_scripts[current].append(sm2.group(1))
# Summarize
all_suites=sorted(set(suite_re.findall(section)))
zero=[s for s in all_suites if len(suites_scripts.get(s,[]))==0]
alias_only=[s for s in sorted(suites_has_steps_alias) if len(suites_scripts.get(s,[]))==0]
print("Total suites in section:", len(all_suites))
print("Suites with 0 extracted scripts:", len(zero))
if zero:
print("First 15 zero-script suites:", zero[:15])
print("Suites that use steps aliases and have 0 extracted scripts:", len(alias_only))
if alias_only:
print("First 15 alias-only suites:", alias_only[:15])
# Show a few non-zero samples for sanity
nonzero=[s for s in all_suites if len(suites_scripts.get(s,[]))>0]
print("Sample non-zero suites (up to 10):")
for s in nonzero[:10]:
scripts=suites_scripts[s]
print(f"- {s}: {len(scripts)} scripts; first: {scripts[0]}")
PYRepository: NVIDIA/NemoClaw
Length of output: 366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test/e2e/validation_suites/suites.yaml"
# Show the suite blocks for a few suites that were "alias-only" (0 extracted scripts by the indentation parser)
for suite in cloud-inference diagnostics docs-validation gateway-health openai-compatible-inference sandbox-shell; do
echo "================ ${suite} ================"
# Print from the suite key line until the next top-level suite key (2 spaces + name + colon)
awk -v s="$suite" '
$0 ~ "^ " s ":\\s*$" {inblock=1}
inblock {print}
inblock && $0 ~ "^ [A-Za-z0-9_.-]+:\\s*$" && $0 !~ "^ " s ":\\s*$" {inblock=0}
' "$FILE" | head -n 120
done
# Also confirm what "script:" lines exist (and their indentation) within these suites
echo "================ script lines (with leading whitespace counts) ================"
python3 - <<'PY'
import re
file="test/e2e/validation_suites/suites.yaml"
suites=["cloud-inference","diagnostics","docs-validation","gateway-health","openai-compatible-inference","sandbox-shell"]
lines=open(file,'r',encoding='utf-8').read().splitlines()
suite_re=re.compile(r'^ ([A-Za-z0-9_.-]+):\s*$')
script_re=re.compile(r'^(\s*)script:\s*(.+?)\s*$')
current=None
wanted=set(suites)
out={s:[] for s in suites}
for line in lines:
m=suite_re.match(line)
if m:
current=m.group(1)
if current in wanted:
sm=script_re.match(line)
if sm:
indent=len(sm.group(1).replace('\t',' '))
out[current].append((indent, sm.group(2)))
for s in suites:
print(s, "script lines:", len(out[s]))
if out[s]:
print(" indent samples:", out[s][:5])
PYRepository: NVIDIA/NemoClaw
Length of output: 1157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/e2e-advisor/scenarios.mts"
# 1) Locate parseSuiteScripts + extractTopLevelSection implementation
rg -n "function parseSuiteScripts|extractTopLevelSection" "$FILE"
# Show surrounding code (avoid huge output by limiting lines around matches)
python3 - <<'PY'
import subprocess, re
file="tools/e2e-advisor/scenarios.mts"
# get line numbers for parseSuiteScripts and extractTopLevelSection
text=open(file,'r',encoding='utf-8').read().splitlines()
targets=[]
for i,l in enumerate(text,1):
if "function parseSuiteScripts" in l or "extractTopLevelSection" in l:
targets.append(i)
for t in sorted(set(targets)):
start=max(1,t-20); end=min(len(text),t+60)
print("\n### Snippet around line",t,"###\n")
for j in range(start,end+1):
print(f"{j}:{text[j-1]}")
PY
# 2) Re-run the parser logic in Python using the same regex expectations:
# - take the "suites" top-level section the way extractTopLevelSection likely does (between "suites:" and next top-level key)
python3 - <<'PY'
import re, sys, collections
text=open("test/e2e/validation_suites/suites.yaml","r",encoding="utf-8").read()
# Best-effort mimic: extract from "^suites:" to next "^\\S" (top-level key)
m=re.search(r'(?ms)^suites:\s*\n(.*?)(?:\n^[^ \t].*?:\s*$|\Z)', text)
if not m:
# fallback: from suites: to EOF
m=re.search(r'(?ms)^suites:\s*\n(.*)$', text)
if not m:
print("Could not locate suites: section")
sys.exit(1)
section=m.group(1)
suite_re=re.compile(r'^ ([A-Za-z0-9_.-]+):\s*$')
script_re=re.compile(r'^ script:\s*([A-Za-z0-9_./-]+)\s*$')
suites=collections.defaultdict(list)
current=None
for line in section.splitlines():
sm=suite_re.match(line)
if sm:
current=sm.group(1)
suites[current]=[]
continue
if not current:
continue
sm2=script_re.match(line)
if sm2:
suites[current].append(sm2.group(1))
all_suites=sorted(suites.keys())
zero=[s for s in all_suites if len(suites[s])==0]
print("Parsed suites:", len(all_suites))
print("Suites with 0 extracted scripts (would fail to map scripts for alias-based suites):", len(zero))
print("First 30 zero-script suites:", zero[:30])
# Count how many script lines exist in file under exact indentation (for sanity)
file_script_re=re.compile(r'^ script:\s*[A-Za-z0-9_./-]+\s*$', re.M)
print("Total inline 'script:' lines matching exact indentation:", len(file_script_re.findall(text)))
PYRepository: NVIDIA/NemoClaw
Length of output: 12268
Fix parseSuiteScripts to handle YAML aliases (it currently misses 6 suites)
- In
tools/e2e-advisor/scenarios.mts:348-366, the regexes correctly match inline suite/script entries intest/e2e/validation_suites/suites.yaml(<suite>:andscript:). - However, 6 suites define
steps: *id...without any inlinescript:keys (cloud-inference,diagnostics,docs-validation,gateway-health,openai-compatible-inference,sandbox-shell), soparseSuiteScriptsreturns an empty list for them. - Downstream mapping that relies on this will treat those suites as having no scripts—use a YAML parser that resolves aliases (instead of indentation-based regex parsing) or explicitly handle/skip alias-based suites.
🤖 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 `@tools/e2e-advisor/scenarios.mts` around lines 348 - 366, parseSuiteScripts
currently uses regexes to parse the "suites" section line-by-line so YAML
aliases like "steps: *id" are ignored (causing 6 suites to appear empty); update
parseSuiteScripts to use a proper YAML parser that resolves aliases (or at
minimum detect a "steps:\s*\*[A-Za-z0-9_-]+" pattern and follow the alias
lookup) so script lists are populated. Specifically, replace the
indentation/regex parsing in parseSuiteScripts (and its dependency
extractTopLevelSection if needed) with a YAML parse of the suites block (e.g.,
parse into objects and read suiteId -> steps/script arrays) or add logic to
resolve alias references from the same YAML content before returning suites.
cv
approved these changes
May 22, 2026
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used read-only inspection and trusted collected metadata; no scripts, tests, npm commands, or package-manager commands were executed by this advisor.; GitHub review-thread state was not fully available; CodeRabbit was observed pending/in-progress in collected metadata.; No linked issues were present in the trusted context, so acceptance mapping is based on PR body/comment clauses rather than linked issue clauses.; The provided diff was truncated after the changed parser area, so final judgment also used repository file reads for tools/e2e-advisor/scenarios.mts. Full advisor summaryPR Review AdvisorBase: The code change is narrowly scoped and appears to remove the js-yaml runtime dependency as intended, but the PR is not merge-ready while GitHub reports mergeStateStatus=BLOCKED and review-thread state is unavailable. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
This was referenced May 27, 2026
jyaunches
added a commit
that referenced
this pull request
May 27, 2026
…ite from source-shape scanner Two batched fixes for CI on the scenario-suite isolation PR: - Update test/e2e-scenario-advisor.test.ts to expect the registry scenario id ubuntu-repo-cloud-openclaw-telegram. The advisor switched to listScenarios() in #4081 so the legacy YAML id ubuntu-repo-docker__cloud-nvidia-openclaw-telegram is no longer produced. - Treat the isolated test/e2e-scenario/ tree as test fixtures in scripts/find-source-shape-tests.ts. The migration-inventory-lock test reads scenario manifests, expected-states, and validation_suites YAML; those are declarative test assets, not product source. Exempting test/e2e-scenario/ brings source_shape_cases back to 0 without losing scanner coverage of product source under src/, agents/, nemoclaw/, etc. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
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
3 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
js-yamlRoot cause
The E2E advisor job only installs the Pi SDK under the trusted advisor checkout, not the repo's npm dependencies.
scenarios.mtsrequiredjs-yaml, so the deterministic scenario advisor step failed before writing its summary artifact; the scenario comment step then skipped because the summary was missing.Test plan
Summary by CodeRabbit