Repository navigation
Conversation
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 (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughInteractive PTY rules now support optional output settling delays. The driver waits for the configured idle period after the latest output before responding. The Pi qualification test uses a 2-second delay, and end-to-end coverage verifies renewed settling after later output. ChangesInteractive PTY settling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant InteractiveCommandRule
participant PTYDriver
participant PythonPTYDriver
InteractiveCommandRule->>PTYDriver: Provide optional settleMs
PTYDriver->>PTYDriver: Record latest output time
PTYDriver->>PythonPTYDriver: Serialize settleMs
PTYDriver->>PTYDriver: Wait until output is idle
Merge Risk: ⚪ Minimal · up to The optional settling delay preserves immediate existing responses and correctly resets after later terminal output; no concrete current-head failure remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 11a9efc in the TypeScript / code-coverage/cliThe overall line coverage in commit 11a9efc in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
|
Alternative review completed for The full three-file diff review found no actionable correctness, security, scope, or regression-coverage finding. All nine hosted Advisor specialists reported clear results for this commit. CodeRabbit also reported no actionable comments. Its docstring-coverage warning is advisory and identifies no behavioral defect. Local validation passed:
All four PR commits have valid GitHub verification. Required hosted CI passed. The working tree is clean, and review found no secrets in the diff. Local Advisor execution was unavailable because automatic approval review rejected repository-content transmission to its inference service. The authorized alternative used full-diff inspection, focused tests, publication validation, and independent hosted review. This does not claim local Advisor clearance. The two-second quiet period remains a timing heuristic; live Pi qualification must verify session persistence. The approved E2E run selects The PR is non-draft and labeled |
|
The first selected E2E run failed before live test execution because I supplied a descriptive correlation ID instead of the required lowercase UUIDv4. Every selected test invocation reported The corrected run uses correlation |
|
Selected E2E results for The image-source fetch and parity check passed. Pi pre-cleanup then ran A focused diagnostic against current main PR #10355 already introduces |
Consolidate the PTY settling fix and its regression test from PR #11853. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Outcome Pi rebuild preserves recorded context-window, output-token, and reasoning settings. JSON automation excludes project context files. Pi qualification checks the context-free task after fresh onboarding, rebuild, and recovery, and verifies interactive session persistence. Pi remains unavailable to ordinary installations pending #8818. ## Reason Rebuild reconstruction dropped Pi tuning. Automation examples accepted project context, and the qualification flow lacked a fresh-onboarding proof for the context-free task. ### Related issues Resolves #7929. Refs #7928 and the accepted Pi decision in #7926. ## Changes - Replay recorded Pi tuning through the existing startup-profile builder; test explicit false, upper bounds, omitted values, and conflicting ambient inputs. - Document Pi model limits, candidate-image authority, lifecycle operations, recovery, and trust boundaries. Add the model-limits route to both documentation-routing copies. - Seed hostile context files and run `pi-read-v2` with `--no-context-files`. Retain the independent read-tool oracle for fresh onboarding, rebuild, and recovery. - Preserve the PTY settling repair from #11853. Require the interactive turn to add session state and require rebuild to preserve the session inventory. - Use the canonical verified pre-onboarding cleanup owner from main; remove the obsolete best-effort helper. Force termination of the deterministic seed harness on timeout. - Isolate vLLM selection tests from real host-readiness probes. Include the merged SDK delivery dependency from #11921 for credential-free installation during qualification. ## Verification - Current candidate: `b205695cc6f7fa4e9a50d57ff306fd644c1dc01f`, including canonical main `052c19e9a7f65d0ea3dd2e6e6202aa27fe7ef476`. Fresh hosted CI, automated review, and Pi qualification are pending; the successful run below qualifies the earlier commit only. - Current integration checks: signed-commit hooks passed; Pi rebuild and vLLM selection suites passed 46 tests. Pi oracle and PTY support suites passed; the client suite had one unchanged five-second timeout whose two ForwardTcp cases passed in isolation. - Repair commit: `3127f1b62897ceeeb5f938be5f9938b97abefedf`. - Pi task/oracle support suite: 22 tests passed. Published-route suite: 63 tests passed, including exact source mappings for Pi-only onboarding and commands. - Semantic phase validation, E2E assertion ratchet, repository checks, source-shape and growth checks, secret scan, and normal signed-commit hooks passed. - Publication validation, CLI/plugin builds, CLI/plugin TypeScript, and JavaScript configuration checks passed. The guarded fast-forward push and branch/PR readback match the repair commit; all PR commits are GitHub Verified. - The inspected diff and secret scan contain no secrets or credentials. - [Hosted CI](https://github.com/NVIDIA/NemoClaw/actions/runs/35173912638) and the complete [managed-image workflow](https://github.com/NVIDIA/NemoClaw/actions/runs/35173912251) passed, including native AMD64 and ARM64 Pi builds. - [Live AMD64 Pi qualification passed](https://github.com/NVIDIA/NemoClaw/actions/runs/35177167273/job/105061800208) on `3127f1b62897ceeeb5f938be5f9938b97abefedf`. The retained [evidence artifact](https://github.com/NVIDIA/NemoClaw/actions/runs/35177167273/artifacts/10479695594) contains `pi-agent-qualification.json`, `evidence-manifest.json`, all three independent `pi-read-v2` task proofs, session-persistence evidence, policy and credential-boundary checks, and successful cleanup. All seven phases passed; all three read tasks passed on their first attempts. `Relevant E2E` also passed. This is focused Pi coverage, not full-suite release qualification. - Flakiness retained: the [first run of the same candidate](https://github.com/NVIDIA/NemoClaw/actions/runs/35176205131/job/105058784992) failed on an inference invocation HTTP 503 during stop/start after onboarding and rebuild passed. The [exact-base comparison](https://github.com/NVIDIA/NemoClaw/actions/runs/35176719363) failed earlier on the absent-gateway pre-cleanup bug this PR fixes, so that comparison is inconclusive. One bounded unchanged-candidate rerun passed. The HTTP 503 cause remains unclassified; no assertion or failure handling was weakened. ## Review notes The ARM64 lifecycle finding is excluded under the [accepted September 3 amendment](#7926 (comment)), as recorded in the [PR disposition](#10355 (comment)). Full lifecycle qualification runs on AMD64. ARM64 still requires native managed-image build, startup, publication, and matching receipt evidence. This does not waive CI or authorize public Pi activation. The lifecycle artifact above and both platform image jobs in the linked managed-image workflow provide the retained qualification evidence. They do not resolve the inherited review findings or authorize merge. All nine hosted Advisor specialist reports for `0eb1437` were collected. The fresh-onboarding finding and CodeRabbit seed-timeout finding are repaired. CodeRabbit completed review of `71f352f`; its additional Pi source-mapping finding is repaired. The merge retains the current canonical receipt pair and matching authority. CodeRabbit completed review of `3127f1b` with six findings in files inherited unchanged from canonical `main` commit `9c36cd9cd72e07194f6593928a63038e47f11678` (merged #11906). Their file blobs match that canonical commit exactly, including the WeChat sibling of the synchronous-process finding. These are retained as inherited findings outside the Pi repair: [review and six findings](#10355 (review)). The scope warning also comes from those merged base changes; the net Pi delta against the current canonical integration base is 17 files. Self-review of the repair preserves the independent task oracle, cleanup failure propagation, and interactive persistence checks. <!-- nemoclaw-docs-review:start --> - Documentation review: `docs-updated` - Documentation evidence: Verified Pi model-limit bounds, rebuild tuning replay, models.json generation, nproc/nofile limits, setpriv privilege drop, corporate-CA merge, writable paths, and all documented commands and flags against checked-in source; changed documentation reviewed against WRITING.md and STYLE.md; verdict PASS with no blocking findings. - Documentation agent: independent read-only review subagent <!-- docs-review-head-sha: e4ff783 --> <!-- docs-review-agents-blob-sha: 43145d5 --> <!-- nemoclaw-docs-review:end --> --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Kao Félix <me@kaofelix.dev> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added Pi guidance for configuring context windows, output limits, and reasoning settings, including validation and unsupported options. - Documented resumable onboarding, recovery, sandbox replacement, rebuilds, model changes, and update workflows. - Added headless operation guidance for excluding project context files. - Clarified sandbox resource limits, security checks, certificate handling, and qualification evidence. - Updated Pi navigation and reference links to make model-limit guidance easier to find. - **Bug Fixes** - Rebuilds now preserve configured Pi model settings, including explicit and maximum supported values. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Kao Félix <me@kaofelix.dev> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kao Félix <me@kaofelix.dev> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
1 participant
Outcome
The interactive PTY driver can wait for terminal output to become idle before it sends a scripted response. Pi qualification now waits after the expected answer appears before it sends Ctrl-D, which lets Pi finish and persist the session.
Reason
Pi can include the expected answer in streamed reasoning. The previous rule sent Ctrl-D on that first match, so Pi exited before it wrote the session that the rebuild phase must verify.
Related issues
Refs #10355
Changes
settleMsguard to interactive PTY rules. Existing consumers keep immediate responses.Verification
npm exec -- vitest run --project e2e-support test/e2e/support/onboard-interactive-pty.test.ts— passed, 5 tests.npm run test:changed— passed.npm run e2e:assertions:check— passed with the existing assertion budget.npm run typecheck:cli— passed.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Improvements
Tests