fix(openclaw): isolate managed heartbeat sessions - #11487
Conversation
Run explicitly configured managed heartbeats in OpenClaw's dedicated heartbeat session. Validate the isolated setting, configuration hash, and startup interval. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
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. |
|
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 (8)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughConfigured OpenClaw heartbeats now use isolated sessions. Managed-image fixtures and direct and OpenShell checks validate cadence, isolation, startup behavior, configuration hashes, and protected error handling. ChangesOpenClaw heartbeat configuration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Managed OpenClaw heartbeats now use dedicated sessions and the updated validation paths cover the configured cadence and generated configuration integrity. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-11487.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit df172e7 in the TypeScript / code-coverage/cliThe overall line coverage in commit df172e7 in the Show a line coverage summary of the most impacted files.
Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Run explicitly configured managed heartbeats in OpenClaw's dedicated heartbeat session. Validate the isolated setting, configuration hash, and startup interval. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
<!-- markdownlint-disable MD041 --> ## 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> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
PR Review Advisor finished for commit |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
## Outcome The launch supervisor timeout test now lets the host timeout terminate the command before the fixture's internal deadline expires. ## Reason The fixture's two-second session budget could expire before the 2.5-second host timeout. This caused `fixture.result.timedOut` to be false. The same assertion failed on [main CI](https://github.com/NVIDIA/NemoClaw/actions/runs/34602042635/job/103271974108) and blocked [PR #11487](#11487). ## Changes Give `supervisor-timeout` the existing ten-second session budget used by `restored-canonical-timeout`. Keep the host timeout and all descendant-cleanup assertions unchanged. No production code, retry policy, or new mechanism changes. ## Verification - Linux, Node 22.23.2 container with `--init`: `npx vitest run --project e2e-support test/e2e/support/launch-agent-turn.test.ts -t "host command times out" --reporter=verbose` — passed; 1 test passed, 43 deselected. This ran before rebase; the test file is unchanged by rebase. - macOS: `npx vitest run --project e2e-support test/e2e/support/launch-agent-turn.test.ts` — 6 passed, 38 Linux-only tests skipped. - `npm --prefix nemoclaw run build` and `npm run build:cli` — passed. - `NODE_OPTIONS=--max-old-space-size=5120 npm run validate:pr` — passed for `b1aeedef5b80ae061745a502eb5938b755526f43`, with canonical validation base `41c5625e8b831ed213cd5c381385973adc58659c`. - Diff review found no secrets, API keys, or credentials. ## Review notes Local Advisor sandbox creation is unavailable in this shepherd session. Alternative review completed for the one-line diff: correctness, unchanged cleanup assertions, sibling timeout budgets, and security boundaries were checked. This is self-review, not independent Advisor clearance. Hosted checks and review remain pending. An initial Linux container without an init process retained zombie descendants; the corrected container passed the affected test. The full Linux suite did not complete, so no full-suite pass is claimed. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated the supervisor timeout test scenario to use a 10-second launch session budget, aligning it with the restored canonical timeout scenario. * Other timeout modes and default budgets remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
rsliter
left a comment
rsliter
left a comment
There was a problem hiding this comment.
Reviewed commit df172e7 against accepted issue #10262. The shared generator emits the isolated heartbeat for configured cadences, including 0m; managed startup and image checks validate cadence, isolation, startup evidence, and config integrity. Shell inputs are quoted, failed log reads omit captured output, focused validation passed, the current CodeRabbit full review has no actionable findings, and the bounded cross-issue sweep found no adjacent conflict.
|
The earlier MCP-discovery setup failure is cleared: managed-image workflow 34641125743 attempt 2 passed, including both MCP passes and all-agent activation. CI and the full CodeRabbit review also passed for df172e7 (no actionable review findings). We are now waiting on the selected protected-runtime E2E, including multi-architecture managed-image startup: https://github.com/NVIDIA/NemoClaw/actions/runs/34666235561 The run is in progress, not yet passing evidence. It targets PR head df172e7, base ffc1116, with trusted workflow 37261ef. Selector: managed-image-protected-runtime; no Launchable or Jetson opt-ins. Human review and merge remain separate from this automated qualification. |
|
The protected E2E failure is also present on main: both native architectures fail while building Hermes because the protected builder selects root but Hermes requires sandbox. Main comparison: https://github.com/NVIDIA/NemoClaw/actions/runs/34670833866. Fix PR #11613 changes the default to sandbox and passes 37 focused tests plus publication validation. Its CI, automated review, and selected E2E are next. This PR remains waiting for that dependency; five-minute follow-up is active. No merge has been performed. |
4 participants
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
isolatedSession: truewith explicitly configured heartbeat cadences, including0m, without adding a public setting.2m, its generated configuration hash, and the120000ms startup interval. The sandbox probe verifies the built image beyond generator-only assertions.PATHand quote shell inputs literally, including paths that contain quotes and shell metacharacters.Verification
npm run validate:prpassed against canonical mainfa97e069a0b1ae8922f112e7260a9cbc9e6d7c7f, including CLI type-checking.df172e751692babe739d9b38d936fe0c0903b81b.Review notes
Candidate:
df172e751692babe739d9b38d936fe0c0903b81binNVIDIA/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-startupandmanaged-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
Documentation
Bug Fixes