fix(advisor): complete local review on macOS by jyaunches · Pull Request #11500 · NVIDIA/NemoClaw

jyaunches · 2026-09-10T23:55:25Z

Outcome

Local PR review now starts reliably from macOS temporary directories, finds Homebrew-installed tools, and removes its owned temporary root after success or failure. Previously, the trusted bootstrap could exit silently through a symlinked TMPDIR, omit rg from its constrained path, or leave immutable Advisor input behind.

Reason

PR #11487 cannot complete its required local review because the Advisor bootstrap and cleanup fail on a standard macOS checkout. These failures also reproduce on canonical main.

Related issues

Relates to #11487.

Changes

Resolve the trusted Advisor implementation to its canonical path before starting Node. This is required because macOS exposes /var through /private/var; the bootstrap is the consumer, and string comparison alone cannot establish direct execution through that alias. The bootstrap-isolation regression test uses a synthetic symlinked temporary root.
Add /opt/homebrew/bin to the constrained host tool path so the Advisor can invoke its required rg executable on Apple Silicon Homebrew installations. Keeping the fixed allowlist preserves the existing trust boundary.
Restore write permission only on directories inside the Advisor-owned unpredictable temporary root before removal. Direct unlinking is insufficient because specialist input is intentionally read-only; the cleanup regression test also proves that a symlinked external directory is not modified.

Verification

npx vitest run --project integration test/automation/pull-requests/pr-review-advisor-local.test.ts -t 'installs trusted dependencies|removes its temporary root' --reporter=dot — 3 passed, 15 skipped.
npm run checks:repository — passed.
npm run build:cli && npm --prefix nemoclaw run build — passed after the final base integration.
NODE_OPTIONS=--max-old-space-size=5120 npm run validate:pr — passed against canonical main 79c09d969ab4d3c2d5446c8b985fc0e513c9054b after the final base integration.
Manual reproduction on canonical main — confirmed the silent canonical-path exit, missing Homebrew rg, and immutable-directory cleanup failure.
Diff inspection — no secrets, API keys, or credentials are present.

Review notes

Sensitive paths: tools/pr-review-advisor/local-review.mts and tools/pr-review-advisor/local-review-implementation.mts. Repository NVIDIA/NemoClaw, commit 64b54ea1ee7269f2f488c3eaefcdca5ac681be0f, reviewed locally through focused regression tests, repository checks, canonical builds, full PR validation, and manual diff/security-boundary inspection; no candidate-owned findings remain. Independent local Advisor clearance is unavailable because the trusted bootstrap deliberately loads its implementation from canonical main, which is the defect this PR repairs. The PR remains draft for independent review and hosted checks.

Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

Bug Fixes
- Improved cleanup of temporary review data, including read-only directories and symbolic links, without altering files outside the managed temporary area.
- Temporary directories created by the review process are now made writable when necessary, ensuring reliable cleanup under restrictive permissions.
- Improved execution reliability on systems using Homebrew-installed tools.
- Updated startup behavior to consistently run the intended checked-out implementation.

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

copy-pr-bot · 2026-09-10T23:55:28Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

coderabbitai · 2026-09-10T23:55:31Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3b90ec32-4284-4c4d-b8a9-8eb2cd5f8970

📥 Commits

Reviewing files that changed from the base of the PR and between 79c09d9 and 64b54ea.

📒 Files selected for processing (3)

test/automation/pull-requests/pr-review-advisor-local.test.ts
tools/pr-review-advisor/local-review-implementation.mts
tools/pr-review-advisor/local-review.mts

📝 Walkthrough

Walkthrough

The local review bootstrap now uses trusted executable paths and resolved implementation paths. Temporary-directory cleanup adjusts permissions only for internally owned roots. Tests cover symlinked entrypoints, TMPDIR, read-only directories, and external symlink targets.

Changes

Local review hardening

Layer / File(s)	Summary
Canonical bootstrap and temporary-path handling `tools/pr-review-advisor/local-review.mts`, `test/automation/pull-requests/pr-review-advisor-local.test.ts`	The trusted `PATH` includes `/opt/homebrew/bin`. The bootstrap launches the checked-out implementation through its resolved real path. Tests cover direct entrypoint execution through a temporary symlink and `TMPDIR` handling.
Owned temporary-directory cleanup `tools/pr-review-advisor/local-review-implementation.mts`, `test/automation/pull-requests/pr-review-advisor-local.test.ts`	Cleanup makes directories under an internally owned temporary root writable before removal. Tests cover read-only contents and verify that external symlink targets keep their permissions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: cv, apurvvkumaria

✨ 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/fix-local-review-canonical-entrypoint

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

github-code-quality · 2026-09-10T23:56:36Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 64b54ea in the codex/fix-local-revi... branch remains at 96%, unchanged from commit 79c09d9 in the main branch.

_{Updated September 11, 2026 00:09 UTC}

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

## Outcome Each locally owned Advisor gateway now uses a process-local in-memory database. Repeated reviews can register the `advisor` provider without colliding with a previous review or a host database. ## Reason Local review of #11495 still failed after #11500 merged. On trusted `main` commit `05796cf4e47b717aa80b2d9c603946e0ec5aad0b`, the gateway became healthy, then `openshell provider create` failed with `provider already exists`. The gateway had inherited the default persistent database while the runner assumed a fresh provider namespace. ### Related issues Relates to #11495. Follows #11500. ## Changes - Set `OPENSHELL_DB_URL=sqlite::memory:?cache=shared` only for an owned gateway process. The existing SQLite backend provides isolation without adding a database-directory lifecycle or deleting host state. - Keep non-owned gateway configuration unchanged. The merge-conflict-fixer regression test protects this sibling path. - Extend the Advisor credential-boundary test to reject an inherited database URL and preserve the caller environment. - Document local gateway database lifetime. ## Verification - `TMPDIR=/private/tmp npx vitest run --project integration test/automation/pull-requests/pr-review-advisor-openshell.test.ts test/automation/pull-requests/pr-merge-conflict-fixer.test.ts --maxWorkers 1` — 61 passed. - Two consecutive real owned-gateway lifecycles using OpenShell 0.0.106, Docker Desktop 28.3.2, a temporary HOME, and an inert provider credential — both registered `advisor`, configured an unverified inert model, and stopped successfully. No model requests or sandboxes were created. The temporary diagnostic files were removed and neither gateway listener remained. - Normal pre-commit and commit-message hooks — passed. - `NODE_OPTIONS=--max-old-space-size=5120 npm run validate:pr` — passed against canonical `main` `05796cf4e47b717aa80b2d9c603946e0ec5aad0b`; validation and hook execution code were unchanged from that base. - Diff inspection — no secrets, API keys, credentials, or unrelated changes are included. ## Review notes Repository: NVIDIA/NemoClaw. Reviewed commit: `8a0d8d7d0a0ef20e3ef3a4843c05909ca9f5bc71`. Self-review covered the runtime credential boundary, process ownership, inherited configuration, gateway failure cleanup, and the owning Advisor README. Provider data is disposable only for locally owned gateways; existing databases and non-owned behavior remain unchanged. Full local Advisor clearance is not claimed: its trusted implementation comes from `main`, where the reproduced provider collision remains. This draft is for independent review and CI, not approval or merge. The initial review evidence consists of the focused tests, real process diagnostic, and direct diff review. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>  ## Summary by CodeRabbit - **New Features** - Locally owned inference gateways now use an isolated in-memory database. - Database records created during a local gateway session are discarded when the gateway stops. - Existing database configuration is ignored for locally owned gateways, preventing unintended use of another gateway’s database. - Externally managed gateways continue using their existing environment configuration. - **Documentation** - Local-run guidance now explains gateway database isolation, lifecycle, and configuration behavior.  Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

## Outcome An explicitly configured NemoClaw-managed OpenClaw heartbeat runs in a dedicated heartbeat session. Managed-image checks reject a main-session heartbeat, a different cadence, or a stale configuration hash. ## Reason NemoClaw previously set only `agents.defaults.heartbeat.every`, leaving heartbeat turns in the ordinary main session. Main-session activity could make liveness appear current without a scheduled heartbeat turn. ### Related issues Fixes #10262. Relates to #10244. Advisor dependency #11500 and main test repair #11503 have merged and are incorporated. ## Changes - Set `isolatedSession: true` with explicitly configured heartbeat cadences, including `0m`, without adding a public setting. - Qualify the managed-image fixture with `2m`, its generated configuration hash, and the `120000` ms startup interval. The sandbox probe verifies the built image beyond generator-only assertions. - Exercise the actual probe with valid, main-session, wrong-cadence, and stale-hash fixtures. Resolve the test hash executable through `PATH` and quote shell inputs literally, including paths that contain quotes and shell metacharacters. - Document dedicated heartbeat sessions and accepted duration syntax. The restore-merger defect in #10244 remains separate scope. ## Verification - Generator and managed-runtime contract suites: 197 tests passed. - Managed-startup environment suite: 41 tests passed. - Final probe contract suite with shell metacharacters in the temporary path: 48 tests passed. - Maintainer focused review rerun: 238 tests passed across the generator, managed-startup mapping, and protected runtime contracts. - CLI and plugin builds passed. - `npm run validate:pr` passed against canonical main `fa97e069a0b1ae8922f112e7260a9cbc9e6d7c7f`, including CLI type-checking. - The incorporated main test repair passes all 88 live export tests. - Required GitHub checks pass on candidate `df172e751692babe739d9b38d936fe0c0903b81b`. - CodeRabbit completed a full review of the candidate with no actionable findings and minimal merge risk. - Repository checks and signed-commit hooks passed. The diff contains no secrets, API keys, or credentials. - Documentation validation passed on the candidate. ## Review notes Candidate: `df172e751692babe739d9b38d936fe0c0903b81b` in `NVIDIA/NemoClaw`. Sensitive paths are the config generator and managed-image qualification scripts. Full diff review covered correctness, shell quoting, credential-safe diagnostics, test coverage, and deterministic risk selection. The maintainer review approved this candidate. The bounded cross-issue sweep found no adjacent fix or contradiction. The complete PR Review Advisor report remains unavailable because the Advisor service is failing before specialist execution, so merge remains blocked on that evidence. Both OpenClaw MCP discovery passes now succeed on the candidate. The second pass succeeded when rerun after its external Cloudflare tunnel process exited before readiness on all three bounded attempts. The deterministic risk plan requires `managed-image-multiarch-startup` and `managed-image-protected-runtime`. These manual qualifications have not been dispatched. Merge remains separate from automated evaluation. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>  ## Summary by CodeRabbit * **New Features** * Agent heartbeats now run in dedicated, isolated sessions. * Configured heartbeat intervals automatically enable isolated-session behavior. * Managed OpenClaw environments use a validated two-minute heartbeat interval. * **Documentation** * Updated heartbeat documentation to explain periodic isolated-session turns. * **Bug Fixes** * Improved validation of heartbeat settings, startup behavior, and configuration integrity in managed environments.  --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

fix(advisor): complete local review on macOS

578380d

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

jyaunches self-assigned this Sep 10, 2026

merge: integrate main into advisor repair

64b54ea

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

jyaunches mentioned this pull request Sep 11, 2026

feat(ci): publish review queue E2E evidence #11495

Merged

jyaunches marked this pull request as ready for review September 11, 2026 00:15

jyaunches merged commit 05796cf into main Sep 11, 2026
60 of 65 checks passed

jyaunches deleted the codex/fix-local-review-canonical-entrypoint branch September 11, 2026 00:16

github-actions Bot added the v0.0.123 label Sep 11, 2026

jyaunches mentioned this pull request Sep 11, 2026

fix(advisor): isolate owned gateway provider state #11501

Merged

jyaunches mentioned this pull request Sep 11, 2026

fix(openclaw): isolate managed heartbeat sessions #11487

Merged

Navigation Menu

Uh oh!

fix(advisor): complete local review on macOS - #11500

fix(advisor): complete local review on macOS#11500
jyaunches merged 2 commits into
mainfrom
codex/fix-local-review-canonical-entrypoint

jyaunches commented Sep 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

copy-pr-bot Bot commented Sep 10, 2026

Uh oh!

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Uh oh!

github-code-quality Bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jyaunches commented Sep 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Outcome

Reason

Related issues

Changes

Verification

Review notes

Summary by CodeRabbit

Uh oh!

copy-pr-bot Bot commented Sep 10, 2026

Uh oh!

coderabbitai Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Uh oh!

github-code-quality Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jyaunches commented Sep 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading

github-code-quality Bot commented Sep 10, 2026 •

edited

Loading