fix(start): drop slow-mode polling on late allowlisted scope upgrades#5387
Open
laitingsheng wants to merge 9 commits into
Open
fix(start): drop slow-mode polling on late allowlisted scope upgrades#5387laitingsheng wants to merge 9 commits into
laitingsheng wants to merge 9 commits into
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a bounded "fast-reentry" polling window to the auto-pair watcher that briefly forces 1-second polling after allowlisted device approvals are attempted, enabling capture of late CLI arrivals. The nightly E2E workflow and tests are updated to use unified CLI scope-upgrade naming instead of issue-4462 prefixes. ChangesFast-reentry scope-upgrade polling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 44%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27463690336
|
![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: 2
🤖 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 `@scripts/nemoclaw-start.sh`:
- Around line 2050-2061: The fast-reentry counter is being reinitialized every
loop when attempted_approval is true, allowing a single failing pending request
to keep fast mode indefinitely; change the logic in the watcher loop so
FAST_REENTRY_REMAINING is only reset once per approval lifecycle (e.g., on the
rising edge of attempted_approval or when FAST_REENTRY_REMAINING == 0) instead
of every loop iteration: add a small persistent flag like
prev_attempted_approval or check FAST_REENTRY_REMAINING to detect transitions
and only set FAST_REENTRY_REMAINING = FAST_REENTRY_POLLS and print the log when
the transition occurs (leave APPROVED and SLOW_MODE usage as-is).
In `@test/nemoclaw-start.test.ts`:
- Around line 1746-1859: The test added ("drops slow-mode polling back to fast
cadence when a late scope upgrade arrives") exceeds the file line budget;
extract the duplicated late-cli fixture and setup into a small reusable helper
(e.g. create a function like createLateCliFixture that returns { tmpDir,
fakeOpenclaw, stateFile, approveLog } and sets up the script and permissions)
and call that helper from this test (and the adjacent similar test) instead of
inlining the long bash string and env setup, or alternatively fold the
fast-reentry assertions into the existing late-upgrade test to avoid duplicating
the entire fixture; locate references to fakeOpenclaw, stateFile, approveLog,
and buildAutoPairScript() to update call sites to use the helper and remove the
duplicated lines from the test body.
🪄 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: 5b40365e-a235-4bb4-bb63-2de367836ecb
📒 Files selected for processing (5)
.github/workflows/nightly-e2e.yamlscripts/nemoclaw-start.shtest/e2e-script-workflow.test.tstest/e2e/test-cli-scope-upgrade-approval.shtest/nemoclaw-start.test.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27463736935
|
laitingsheng
added
integration: openclaw
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/test-cli-scope-upgrade-approval.sh`:
- Around line 1082-1086: The grep check for '[auto-pair] fast-reentry bumped'
using auto_pair_log_snapshot should be made informational: in the else branch
replace the fail call with a non-fatal log/warn (or call a function like warn or
echo) so the script doesn't hard-fail when the watcher didn't log the marker;
keep the pass branch unchanged. Update the block that currently uses grep -F ...
and calls fail to instead emit a warning/message while allowing the test to
continue.
🪄 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: 5a348f44-52fd-43f1-a690-23bae8b5c178
📒 Files selected for processing (4)
.github/workflows/nightly-e2e.yamlscripts/nemoclaw-start.shtest/e2e/test-cli-scope-upgrade-approval.shtest/nemoclaw-start.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/nemoclaw-start.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27464785119
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27465290873
|
…failures Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…approve failures" This reverts commit bba211b.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/nemoclaw-start.test.ts`:
- Around line 2248-2250: The test's assertion currently only matches
"fast-reentry bumped polls=3 approved=0 mode=fast" and can miss alternate bump
lines; update the marker and expectation used in test variable markerRe (and the
subsequent expect using run.stdout.match(...).length) to match any "fast-reentry
bumped" line (ignore the particular field values) and assert the total number of
such occurrences is exactly 1 so any extra bump log (e.g., with approved=1) will
fail the test.
🪄 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: 97da2f1c-484a-4ce2-a416-670cedbb0123
📒 Files selected for processing (1)
test/nemoclaw-start.test.ts
…ng-edge assertion Signed-off-by: Tinson Lai <tinsonl@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
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.
Summary
The in-sandbox auto-pair watcher transitions to slow-mode keepalive after the first device pairs and a handful of quiet polls. A scope upgrade requested after that point — fresh
openclaw tui,openclaw agent, or any other allowlisted CLI client — waits up to one slow-poll interval before the watcher revisits the pending list, which exceeds the OpenClaw client's tolerance forscope upgrade pending approvaland forces an embedded-mode fallback. Drop the slow-mode default to 5s and arm a bounded fast-reentry counter on the rising edge of each fresh allowlisted approval attempt so the override floors polling at 1s for the next few iterations. The counter is keyed byrequestId(garbage-collected against the live pending list) so a sticky failing request bumps once and then yields back to the slow cadence; it is also floored by the caller's existing default so fast-reentry never increases latency on a tight retry pass.Related Issue
Fixes #5343
Refs #5324
#5324is partially addressed by the polling-cadence fix when the failing non-TUI client requests stay within the allowlisted scopes (operator.pairing/read/write). The bug's other symptoms —Unknown command: openclaw device, the dashboardAuth did not matchrate-limit, and any subcommand whose pairing genuinely needsoperator.admin— sit outside the auto-pair policy boundary and remain open for a separate operator-driven approval surface.Changes
scripts/nemoclaw-start.sh: declareFAST_REENTRY_POLLS/FAST_REENTRY_INTERVAL/FAST_REENTRY_REMAINING/FAST_REENTRY_BUMPED_REQUEST_IDS, add asleep_for_next_poll(default)helper that floors the override by the caller's default, wire it into every watcher sleep call-site, and bump the counter once on the rising edge of each fresh allowlisted approval attempt — emitting a single[auto-pair] fast-reentry bumped polls=N approved=N mode=slow|fastmarker. LowerSLOW_INTERVALdefault from 30s to 5s so the worst-case latency on the first late upgrade sits below typical OpenClaw client wait windows.test/nemoclaw-start.test.ts: extract a shared late-CLI fixture (setupLateCliFixture) so the existing convergence test and the new fast-reentry assertions share their fakeopenclawstub, and fold the fast-reentry marker + ordering + single-rising-edge assertions into the existing late-upgrade test to stay inside the file size budget.test/e2e/test-cli-scope-upgrade-approval.sh(renamed fromtest-issue-4462-scope-upgrade-approval.sh): dropAUTO_PAIR_SLOW_INTERVAL_DEFAULTfrom600to5so the in-sandbox watcher is observably exercised by the test, tolerate watcher-wins in Phase 3 viaallow_already_approved=1, and make Phase 6 strict — it now fails the test if the watcher did not record a slow-mode transition or did not log the fast-reentry marker.test/e2e-script-workflow.test.ts,.github/workflows/nightly-e2e.yaml: update the legacy + nightly script allowlists and CI job script paths to the renamed file, rename the two affected nightly E2E jobs to drop the issue-id prefix (cli-scope-upgrade-approval-e2e,cli-scope-upgrade-legacy-repro-e2e), and sync the approval-job'sNEMOCLAW_AUTO_PAIR_SLOW_INTERVAL_SECSfrom600to5.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Tests