feat(openshell): route session reads over authenticated gRPC - #6793
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # src/lib/adapters/openshell/grpc-sandbox-control.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Make the maintainer merge gate evaluate only current, complete GitHub Actions evidence for the exact PR diff. Before this change, stale retry jobs and later all-skipped body-edit runs could block an otherwise-green PR; after it, the gate selects the latest complete attempt while failing closed when GitHub evidence is incomplete. ## Changes - Group duplicate check runs by authoritative workflow identity; require exact-diff evidence for required checks and exact-head evidence for supported non-PR Actions events. - Resolve rerun attempts through every page of the GitHub Actions jobs API, then recheck the run attempt to reject races without hiding matrix-job failures. - Require every latest-attempt job for the check name to appear in the PR rollup and bind every required name to parseable Actions evidence. - Reject legacy status contexts that reuse a required check name without exact-diff Actions provenance. - Ignore all-skipped workflow runs only when GitHub metadata proves a complete skipped run and another meaningful run for the same PR diff and workflow. - Normalize GitHub Actions statuses and conclusions against their documented value sets so unknown metadata cannot count as meaningful evidence. - Bind Actions evidence to the immutable run `head_sha` as well as PR association metadata so live PR fields cannot make a stale run look current. - Bind substantive `pr.yaml` evidence to its canonical immutable PR/head/base title, require `checks` and `changes` from the same `gate true` run, and fail closed on malformed metadata-only `gate false` shapes. - Reject `check-hash` evidence created at or before the latest paginated `base_ref_changed` event, preventing a retargeted PR from inheriting a base-trusted result from its prior base. - Compare the PR base snapshot with the live base branch tip so review-blocked PRs cannot mask stale exact-diff evidence. - Require exactly one successful head/base-bound E2E coordination check from the GitHub Actions app, including the legacy check-name migration path. - Require required merge-gate checks to conclude `SUCCESS`; optional checks may still conclude `NEUTRAL` or `SKIPPED`. - Rerun trusted installer-hash verification after PR base retargets so required evidence cannot be inherited from the prior base. - Keep the Dependabot DCO exemption identity-bound while producing a successful required job and preserving mandatory commit verification. - Recognize only the exact all-skipped `pr.yaml` metadata-edit job shape, preserving earlier substantive matrix evidence while rejecting malformed lookalikes. - Keep malformed URLs, invalid timestamps, stale heads, mixed workflow identities, incomplete conclusions, and ambiguous evidence fail-closed. - Add regression coverage for stale attempts, pagination, rerun races, missing jobs, skipped body-edit runs, malformed identities and timestamps, singleton and tied runs, matrix jobs, status contexts, and custom checks. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes an internal maintainer skill and does not alter NemoClaw user-facing behavior. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Three independent read-only security/correctness reviews found no blocker; the full 133-test affected suite and repository hooks passed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/skills/check-gates-compliance.test.ts test/skills/check-gates-evidence.test.ts` (133 passed); live checker passes #7024 and rejects #6793 only for its stale pre-retarget `check-hash` evidence - [x] Applicable broad gate passed — `npm run check:diff` - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Tightened maintainer merge-gate CI evaluation to use exact-diff evidence and added final-revision re-verification for conflicts if PR revision/merge state changes. * Extended contributor compliance to support Dependabot-style DCO-body bypass and surfaced bypass status in gate output. * **Bug Fixes** * Improved CI gate check deduplication by selecting the latest attempt per check context and failing closed when evidence is incomplete. * Updated conflicts gate logic to consider both mergeability and merge state (including clearer “behind” handling). * **Tests** * Refactored and expanded check-gates compliance/evidence coverage using shared fixtures, including exact-diff coordination, pagination, and malformed evidence cases. * **Documentation** * Updated merge-gate workflow guidance to reflect the stricter exact-diff and merge-readiness rules. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com>
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking correctness/security findings on exact head 8fd2064:
-
execSandboxReadOnlyWithGrpcFallbackwraps both client construction andawait grpc.exec(...)in the same broadcatch. A rejected execution promise is therefore labeled a configuration failure and replayed through the CLI, even though the outcome may be post-dispatch. This contradicts the PR's no-replay contract. Only an explicitOpenShellGrpcPreDispatchErrorshould be eligible for fallback; an untyped rejection must be returned as the execution failure, with coverage for both rejected post-dispatch and rejected typed pre-dispatch cases. -
The fallback control is a process-global, unscoped CLI client. When the sandbox is bound to a named gateway but a different gateway is active, a configuration/lookup fallback can execute against the active gateway instead of
gatewayName—and a same-named sandbox can return another gateway's session data. Construct the CLI fallback for the requested gateway (and reject ambient endpoint overrides that can bypass that binding), and test the exact--gateway <name>invocation. -
When the local registry row is absent,
resolveSandboxGatewayName(undefined)silently chooses the default gateway even thoughensureLiveSandboxOrExitmay just have validated the sandbox on another active gateway. Preserve the legacy active-gateway CLI capture path for unregistered sandboxes, or fail before direct dispatch; do not direct gRPC to a guessed gateway.
I can address these with a narrow append-only maintainer commit while preserving the original author's credit.
Co-authored-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
There was a problem hiding this comment.
Verified on exact head 55e7d3f: post-dispatch rejections fail closed without CLI replay; typed pre-dispatch failures use a gateway-scoped fallback that rejects endpoint overrides; unregistered sandboxes preserve the active-gateway compatibility path. Focused tests, CLI type-checking, structural guards, formatting, commit hooks, DCO, and GitHub signature verification passed. CI and advisors are now running on this head.
Summary
Routes OpenClaw session-list reads through an authenticated direct gRPC client for the sandbox's named OpenShell gateway. The compatibility path retries through supported OpenShell CLI execution only when configuration or sandbox lookup fails before dispatch, so completed commands are never replayed.
This is PR 4 of 26 in the OpenShell gRPC migration stack.
Changes
grpc-gateway-config.test.ts,grpc-sandbox-control.test.ts,sandbox-control-routing.test.ts, andpassthrough.test.ts; a direct unconditional switch is insufficient because named gateways can require different auth material and staged compatibility is safe only before dispatch.Stack
feat/openshell-grpc-client/aeto see only this slice.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run typecheck:clipassednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — targeted and changed-graph validation were used; required CI supplies the broad repository gatesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com