Skip to content

fix(pairing): clear consecutive scope approvals - #9264

Merged
cv merged 2 commits into
mainfrom
codex/cover-paired-scope-approval
Aug 16, 2026
Merged

fix(pairing): clear consecutive scope approvals#9264
cv merged 2 commits into
mainfrom
codex/cover-paired-scope-approval

Conversation

@cv

@cv cv commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fresh OpenClaw onboarding can produce an initial CLI pairing request followed by a write-scope upgrade. This change lets the bounded approval pass clear both allowlisted transitions instead of leaving the upgrade pending.

E2E root cause

Changes

  • Raise the shared connect approval cap from one to two allowlisted requests.
  • Preserve the 10-second outer timing margin for two approval commands.
  • Model the initial pairing, write upgrade, and a third distinct allowlisted request so the test protects the two-request ceiling.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: restores the intended connect-time pairing sequence without changing commands, configuration, errors, or a documented product contract
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent Codex Desktop review confirmed that unknown clients and operator.admin remain denied and the distinct third request protects the two-approval ceiling
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: The change restores the intended connect-time pairing sequence without changing commands, configuration, errors, or a documented product contract.
  • Agent: Codex Desktop

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run test/sandbox-connect-inference/auto-pair-approval.test.ts src/lib/actions/sandbox/connect-autopair-budget.test.ts passed 13 tests; the later test correction passed 9 tests in the changed file
  • Applicable broad gate passed — not applicable because this changes one bounded constant and its focused regression test
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • New Features

    • Increased the number of approvals available during automatic connection pairing from one to two.
    • Extended the connection operation timeout to better accommodate pairing and permission upgrades.
  • Tests

    • Updated approval-flow coverage for multiple pairing approvals and capped subsequent upgrade requests.

cv added 2 commits August 16, 2026 15:45
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: dbcfc776-c600-4d5d-a9ac-1c425200f610

📥 Commits

Reviewing files that changed from the base of the PR and between 1feaf2b and 96a15dd.

📒 Files selected for processing (2)
  • src/lib/actions/sandbox/connect-autopair-budget.ts
  • test/sandbox-connect-inference/auto-pair-approval.test.ts

Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The auto-pair approval budget increases from one to two approvals. The sandbox outer spawn timeout increases from 25 to 35 seconds. Tests cover two successful CLI approvals and a capped webchat upgrade.

Changes

Auto-pair budget

Layer / File(s) Summary
Update approval and timeout limits
src/lib/actions/sandbox/connect-autopair-budget.ts
The approval limit increases to two transitions. The outer spawn timeout increases to 35 seconds.
Validate two-approval behavior
test/sandbox-connect-inference/auto-pair-approval.test.ts
The test covers initial CLI pairing, a CLI write-scope upgrade, and a capped webchat upgrade.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 96a15

The change allows one additional bounded pairing approval and adds regression coverage for the two-request limit; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

  • NVIDIA/NemoClaw#9219: Directly related to auto-pair approval behavior and the /tmp/auto-pair.log startup path.

Suggested reviewers: brandonpelfrey

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: allowing consecutive pairing scope approvals.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/cover-paired-scope-approval

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 96a15dd in the codex/cover-paired-s... branch remains at 96%, unchanged from commit 1feaf2b in the main branch.

@cv
cv merged commit 6f28b97 into main Aug 16, 2026
65 of 66 checks passed
@cv
cv deleted the codex/cover-paired-scope-approval branch August 16, 2026 22:54
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections match; severity counts match.
5 terminology differences from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • Fresh OpenClaw finalization at src/lib/actions/sandbox/connect-autopair-budget.ts:9: primary classified it as justified; the second opinion classified it as define.
  • initial CLI pairing at src/lib/actions/sandbox/connect-autopair-budget.ts:9: selected only by the second-opinion lane as established.
  • operator.pairing at test/sandbox-connect-inference/auto-pair-approval.test.ts:146: selected only by the second-opinion lane as established.
  • operator.write upgrade at src/lib/actions/sandbox/connect-autopair-budget.ts:10: selected only by the second-opinion lane as define.
  • cli-write-upgrade at test/sandbox-connect-inference/auto-pair-approval.test.ts:149: selected only by the second-opinion lane as established.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • established — write-scope upgrade at test/sandbox-connect-inference/auto-pair-approval.test.ts:120: Keep "write-scope upgrade" for the operator.write request after CLI pairing.
  • justified — Fresh OpenClaw finalization at src/lib/actions/sandbox/connect-autopair-budget.ts:9: Keep the modifier because it distinguishes fresh onboarding finalization from restored-clone behavior.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: onboard-repair, onboard-resume
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

1 optional E2E recommendation
  • issue-4462-scope-upgrade-approval

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@cv

cv commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

CI follow-up:

  • All 12 CLI shards passed after retry. The original shard failure was isolated to nondeterministic PTY assertions in test/e2e/support/launch-agent-turn.test.ts; the third attempt passed.
  • The aggregate coverage gate remains red because src/lib/security/credential-filter.ts is below its existing ratchet. PR fix(cli): restore full E2E lifecycle convergence #9262 already owns that test correction and leaves the production credential filter unchanged.
  • The coverage upload also returns HTTP 400 for this commit.

I did not duplicate #9262 or weaken the coverage threshold in this one-root-cause PR. After #9262 lands, this branch can incorporate main if the coverage fix is still required for a passing rerun.

@wscurran wscurran added the bug-fix PR fixes a bug or regression label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants