From 10e352e43ef1e922cfce59df3d9ef2ec10477262 Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Thu, 14 May 2026 09:08:58 -0700 Subject: [PATCH 1/3] fix(onboard,uninstall): replace misleading recovery messages (#3456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the two output threads in #3456 left after the core dead-loop fix landed via #3459 + #3434: Sub-bug #3 — `src/lib/onboard.ts` printed `nemoclaw destroy --yes && nemoclaw onboard --gpu` with a literal `` placeholder, and assumed at least one sandbox was registered. When the GPU-passthrough mismatch hit on the State B re-run path with an empty registry (the dead-loop case), the hint was not actionable. Replace with a registry-aware helper at `src/lib/onboard/gpu-recovery.ts` that renders the right shape: - empty registry → suggest `nemoclaw uninstall && nemoclaw onboard --gpu` - one sandbox → suggest destroy --yes --cleanup-gateway for that name - multiple sandboxes → list each, only the last gets --cleanup-gateway Sub-bug #4 — `src/lib/actions/uninstall/run-plan.ts` printed `Destroyed gateway 'nemoclaw' skipped` when the openshell destroy no-op'd (gateway already gone) — the "Destroyed … skipped" wording was self-contradictory. Extend `runOptional` with an `onSkip` option; route the gateway destroy to emit `Gateway 'nemoclaw' already removed or unreachable` on no-op. Tests: - `src/lib/onboard/gpu-recovery.test.ts` (6 tests): forbid literal `` placeholder anywhere in the output; cover empty / single / multi-sandbox cases; defensive filter on whitespace names so a `nemoclaw destroy` rendering can never happen. - `src/lib/actions/uninstall/run-plan.test.ts`: assert the new "already removed or unreachable" wording and the absence of the "Destroyed gateway 'nemoclaw' skipped" string. The core dead loop itself (sub-bugs #1, #2 and State B GPU mismatch) is already addressed by #3459 + #3434 + #3483; #3456 will close once this lands. See the #3456 status comment for the full mapping. Refs #3456. Mirrors (and tightens) the approach in the closed PR #3464, which left the literal `` placeholder in tests per CodeRabbit feedback that was never addressed. Signed-off-by: Charan Jagwani --- src/lib/actions/uninstall/run-plan.test.ts | 37 +++++++++++ src/lib/actions/uninstall/run-plan.ts | 34 +++++++--- src/lib/onboard.ts | 19 +++++- src/lib/onboard/gpu-recovery.test.ts | 74 ++++++++++++++++++++++ src/lib/onboard/gpu-recovery.ts | 60 ++++++++++++++++++ 5 files changed, 212 insertions(+), 12 deletions(-) create mode 100644 src/lib/onboard/gpu-recovery.test.ts create mode 100644 src/lib/onboard/gpu-recovery.ts diff --git a/src/lib/actions/uninstall/run-plan.test.ts b/src/lib/actions/uninstall/run-plan.test.ts index 72b3d2fd92f..03091864b16 100644 --- a/src/lib/actions/uninstall/run-plan.test.ts +++ b/src/lib/actions/uninstall/run-plan.test.ts @@ -582,4 +582,41 @@ describe("uninstall run plan", () => { expect(warnings).toContain("Failed to disable /swapfile; skipping swap cleanup."); expect(logs).not.toContain("Swap file removed"); }); + + it("#3456 sub-bug #4: gateway destroy no-op uses the 'already removed' wording, not 'Destroyed ... skipped'", () => { + // When `openshell gateway destroy -g nemoclaw` returns non-zero (gateway + // already gone), the previous code printed `Destroyed gateway 'nemoclaw' + // skipped` — self-contradictory. The fix routes this branch to an onSkip + // message that describes the actual state. + const warnings: string[] = []; + const logs: string[] = []; + const result = runUninstallPlan( + { assumeYes: true, deleteModels: false, keepOpenShell: true }, + { + commandExists: (command) => command !== "docker" && command !== "pgrep", + env: { HOME: "/home/test", TMPDIR: "/tmp/test" } as NodeJS.ProcessEnv, + error: (line) => warnings.push(line), + existsSync: () => false, + isTty: false, + log: (line) => logs.push(line), + rmSync: vi.fn(), + run: (command, args) => { + // The openshell gateway destroy command no-ops when the gateway is + // already gone — return non-zero to exercise the onSkip branch. + if (command === "openshell" && args[0] === "gateway" && args[1] === "destroy") { + return notFound(); + } + if (args[0] === "-c") return ok("/fake/bin/tool\n"); + return ok(); + }, + runDocker: () => ok(""), + }, + ); + + expect(result.exitCode).toBe(0); + expect(warnings.join("\n")).toContain("Gateway 'nemoclaw' already removed or unreachable"); + expect(`${warnings.join("\n")}\n${logs.join("\n")}`).not.toContain( + "Destroyed gateway 'nemoclaw' skipped", + ); + }); }); diff --git a/src/lib/actions/uninstall/run-plan.ts b/src/lib/actions/uninstall/run-plan.ts index b3d71ea1fc3..c99289bbebe 100644 --- a/src/lib/actions/uninstall/run-plan.ts +++ b/src/lib/actions/uninstall/run-plan.ts @@ -207,10 +207,24 @@ function confirm(options: UninstallRunOptions, runtime: UninstallRuntime): boole return false; } -function runOptional(runtime: UninstallRuntime, description: string, command: string, args: string[]): void { +function runOptional( + runtime: UninstallRuntime, + description: string, + command: string, + args: string[], + opts: { onSkip?: string } = {}, +): void { const result = runtime.run(command, args, { env: runtime.env, stdio: "ignore" }); - if (result.status === 0) runtime.log(description); - else runtime.warn(`${description} skipped`); + if (result.status === 0) { + runtime.log(description); + return; + } + // #3456 sub-bug #4: when the destroy/delete call no-ops (target already + // gone), printing ` skipped` was self-contradictory — e.g. + // "Destroyed gateway 'nemoclaw' skipped" suggested the gateway was both + // destroyed AND skipped. Callers that care can pass a `onSkip` message + // describing the actual state (target absent or unreachable). + runtime.warn(opts.onSkip ?? `${description} skipped`); } function stopHelperServices(paths: UninstallPaths, runtime: UninstallRuntime): void { @@ -390,12 +404,14 @@ function removeOpenShellResources(options: UninstallRunOptions, runtime: Uninsta for (const provider of NEMOCLAW_PROVIDERS) { runOptional(runtime, `Deleted provider '${provider}'`, "openshell", ["provider", "delete", provider]); } - runOptional(runtime, `Destroyed gateway '${options.gatewayName || "nemoclaw"}'`, "openshell", [ - "gateway", - "destroy", - "-g", - options.gatewayName || "nemoclaw", - ]); + const gatewayLabel = options.gatewayName || "nemoclaw"; + runOptional( + runtime, + `Destroyed gateway '${gatewayLabel}'`, + "openshell", + ["gateway", "destroy", "-g", gatewayLabel], + { onSkip: `Gateway '${gatewayLabel}' already removed or unreachable` }, + ); } function removeAliases(paths: UninstallPaths, runtime: UninstallRuntime): void { diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 2fca8bfeac7..acb1c7a41f9 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -340,6 +340,7 @@ import type { } from "./onboard/types"; import { listChannels } from "./sandbox/channels"; import { streamGatewayStart } from "./onboard/gateway"; +import { gpuPassthroughRecoveryLines } from "./onboard/gpu-recovery"; import type { StreamSandboxCreateResult } from "./sandbox/create-stream"; import type { SandboxEntry } from "./state/registry"; import type { BackupResult } from "./state/sandbox"; @@ -10385,9 +10386,21 @@ async function onboard(opts: OnboardOptions = {}): Promise { const gpuOutput = String(gpuCheck.stdout || "").trim(); const gatewayHasGpu = gpuCheck.status === 0 && gpuOutput !== "null" && gpuOutput !== "[]"; if (!gatewayHasGpu) { - console.error(" Existing gateway was started without GPU passthrough."); - console.error(" To enable GPU, destroy the existing sandbox and gateway, then re-onboard:"); - console.error(` nemoclaw destroy --yes && nemoclaw onboard --gpu`); + // #3456 sub-bug #3: emit registry-aware recovery hint instead of the + // previous hard-coded `` placeholder, which was not actionable + // when no sandbox was registered (the dead-loop State B path). + let registeredNames: string[] = []; + try { + registeredNames = registry + .listSandboxes() + .sandboxes.map((s) => s.name) + .filter(Boolean); + } catch { + /* registry unreadable — fall through to empty-list hint */ + } + for (const line of gpuPassthroughRecoveryLines(registeredNames)) { + console.error(line); + } process.exit(1); } } diff --git a/src/lib/onboard/gpu-recovery.test.ts b/src/lib/onboard/gpu-recovery.test.ts new file mode 100644 index 00000000000..1eda000dca0 --- /dev/null +++ b/src/lib/onboard/gpu-recovery.test.ts @@ -0,0 +1,74 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Tests for the GPU-passthrough mismatch recovery hint (#3456 sub-bug #3). + * + * The hint replaces a hard-coded line that printed a literal `` + * placeholder and assumed at least one sandbox was registered — which broke + * the install-loop recovery flow when the registry was empty (the State A / + * State B dead loop the reporter hit on six Linux hosts). + */ + +import { describe, expect, it } from "vitest"; +import { gpuPassthroughRecoveryLines } from "./gpu-recovery"; + +describe("gpuPassthroughRecoveryLines", () => { + it("never emits a literal `` placeholder for any input", () => { + for (const names of [null, [], ["alpha"], ["alpha", "beta"], ["alpha", "beta", "gamma"]]) { + const lines = gpuPassthroughRecoveryLines(names); + expect(lines.join("\n")).not.toMatch(//); + } + }); + + it("suggests `nemoclaw uninstall` when no sandboxes are registered (null input)", () => { + const lines = gpuPassthroughRecoveryLines(null); + const joined = lines.join("\n"); + expect(joined).toContain("Existing gateway was started without GPU passthrough"); + expect(joined).toContain("nemoclaw uninstall"); + expect(joined).toContain("nemoclaw onboard --gpu"); + // Must NOT suggest the destroy form — there is nothing to destroy. + expect(joined).not.toMatch(/nemoclaw [a-z-]+ destroy/); + }); + + it("suggests `nemoclaw uninstall` when no sandboxes are registered (empty array)", () => { + const lines = gpuPassthroughRecoveryLines([]); + expect(lines.join("\n")).toContain("nemoclaw uninstall"); + expect(lines.join("\n")).not.toMatch(/nemoclaw [a-z-]+ destroy/); + }); + + it("suggests destroy for a single registered sandbox with --cleanup-gateway", () => { + const lines = gpuPassthroughRecoveryLines(["my-assistant"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw my-assistant destroy --yes --cleanup-gateway"); + expect(joined).toContain("nemoclaw onboard --gpu"); + // The single-sandbox form must not suggest uninstall — destroy is enough. + expect(joined).not.toContain("nemoclaw uninstall"); + }); + + it("lists every registered sandbox and only appends --cleanup-gateway to the last", () => { + const lines = gpuPassthroughRecoveryLines(["alpha", "beta", "gamma"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw alpha destroy --yes"); + expect(joined).toContain("nemoclaw beta destroy --yes"); + expect(joined).toContain("nemoclaw gamma destroy --yes --cleanup-gateway"); + // Only one --cleanup-gateway across all rows. + expect(joined.match(/--cleanup-gateway/g) ?? []).toHaveLength(1); + // alpha/beta lines must NOT have --cleanup-gateway. + const alphaLine = lines.find((line) => line.includes("nemoclaw alpha destroy")); + const betaLine = lines.find((line) => line.includes("nemoclaw beta destroy")); + expect(alphaLine).not.toContain("--cleanup-gateway"); + expect(betaLine).not.toContain("--cleanup-gateway"); + }); + + it("filters out empty/whitespace names defensively", () => { + // Belt-and-suspenders: if registry.listSandboxes() ever returns a row with + // an empty name, we shouldn't render `nemoclaw destroy --yes` (the very + // bug shape this fix exists to prevent). + const lines = gpuPassthroughRecoveryLines(["", " ", "real"]); + const joined = lines.join("\n"); + expect(joined).toContain("nemoclaw real destroy --yes --cleanup-gateway"); + // No double-spaced "nemoclaw destroy" rendering. + expect(joined).not.toMatch(/nemoclaw\s{2,}destroy/); + }); +}); diff --git a/src/lib/onboard/gpu-recovery.ts b/src/lib/onboard/gpu-recovery.ts new file mode 100644 index 00000000000..52848a90d3a --- /dev/null +++ b/src/lib/onboard/gpu-recovery.ts @@ -0,0 +1,60 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Recovery hint emitted when an onboard run finds the reusable gateway was + * started without GPU passthrough but the current run requested it. + * + * Before #3456 this was a hard-coded `nemoclaw destroy --yes` line + * with a literal `` placeholder — not actionable when the registry was + * empty (the State A / State B dead loop the reporter hit on six Linux + * hosts). This helper renders the right shape based on what's actually + * registered. + */ + +/** + * Returns the multi-line recovery hint for the GPU-passthrough mismatch + * branch in onboard. Caller is expected to emit each line on its own line + * via `console.error` / `runtime.log`. + * + * Empty / null input means no sandboxes are registered locally; we suggest + * `nemoclaw uninstall` because there is nothing for `nemoclaw + * destroy` to act on. A single registered sandbox gets one destroy line + * with `--cleanup-gateway` so the gateway also goes away (otherwise destroy + * preserves the shared gateway by default — see v0.0.39 release notes). + * Multiple sandboxes get one destroy line each; only the last carries + * `--cleanup-gateway` so the gateway lives until every sandbox is gone. + */ +export function gpuPassthroughRecoveryLines(names: readonly string[] | null): string[] { + const cleanNames = (names ?? []).map((n) => n.trim()).filter((n) => n.length > 0); + + if (cleanNames.length === 0) { + return [ + " Existing gateway was started without GPU passthrough.", + " No sandboxes are registered, so there is nothing for `nemoclaw destroy` to act on.", + " Clear the stale gateway state and re-onboard with GPU enabled:", + " nemoclaw uninstall && nemoclaw onboard --gpu", + ]; + } + + if (cleanNames.length === 1) { + return [ + " Existing gateway was started without GPU passthrough.", + " To enable GPU, destroy the existing sandbox and gateway, then re-onboard:", + ` nemoclaw ${cleanNames[0]} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu`, + ]; + } + + const lastIdx = cleanNames.length - 1; + const destroyLines = cleanNames.map((name, idx) => + idx === lastIdx + ? ` nemoclaw ${name} destroy --yes --cleanup-gateway && nemoclaw onboard --gpu` + : ` nemoclaw ${name} destroy --yes`, + ); + + return [ + " Existing gateway was started without GPU passthrough.", + " To enable GPU, destroy each registered sandbox and the gateway, then re-onboard:", + ...destroyLines, + ]; +} From afe54274ed7d1ee1180f1b5f171277810309abec Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Thu, 14 May 2026 09:21:20 -0700 Subject: [PATCH 2/3] =?UTF-8?q?fix(onboard):=20pass=20onboard-entrypoint-b?= =?UTF-8?q?udget=20=E2=80=94=20move=20registry=20lookup=20into=20gpu-recov?= =?UTF-8?q?ery=20(#3456)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first commit grew `src/lib/onboard.ts` by +13 lines, which trips the `onboard-entrypoint-budget` policy on `main` (entrypoint must be net-neutral or smaller; new logic belongs under `src/lib/onboard/**`). Extract the inline registry-lookup + emit loop into two new exports in `src/lib/onboard/gpu-recovery.ts`: - `getRegisteredSandboxNamesForGpuRecovery()` — registry read with a graceful empty-list fallback. - `reportGpuPassthroughRecovery(emit, loadNames?)` — emits the hint lines via `emit`. `loadNames` defaults to the registry reader; tests inject their own list. The onboard.ts callsite collapses from 14 lines to 1: `reportGpuPassthroughRecovery(console.error);` Net change in `src/lib/onboard.ts`: -1 line. Budget passes. Adds two tests for the new wrapper covering empty-registry and multi-sandbox cases. 24 tests pass (was 22). Signed-off-by: Charan Jagwani --- .../SKILL.md | 345 ++++++++++++++++++ src/lib/onboard.ts | 18 +- src/lib/onboard/gpu-recovery.test.ts | 22 +- src/lib/onboard/gpu-recovery.ts | 33 +- 4 files changed, 399 insertions(+), 19 deletions(-) create mode 100644 .agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md diff --git a/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md b/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md new file mode 100644 index 00000000000..36907c5035e --- /dev/null +++ b/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md @@ -0,0 +1,345 @@ +--- +name: nemoclaw-maintainer-issue-autopilot +description: Ships a minimum-scope PR end-to-end for the simplest in-scope NemoClaw issue. Runs nine stages with user gates — local-branch precheck, selection, scope check against repo docs, reproduce-or-refute, test-first implementation, PR open, batch self-review, CI watch, CodeRabbit fix loop, perfect-match acceptance gate. Resumable state via /tmp/issue-autopilot-.state.json survives conversation breaks; identity check rejects 'Test User' fallbacks. Use when there's a clear in-scope ticket to ship, when the maintainer wants minimum-scope discipline enforced, or when resuming an interrupted issue→PR pipeline. Local-only — confirms at every externally-visible step. +--- + +# Issue Autopilot + +Autonomous "issue → merge-ready PR" pipeline for NemoClaw. The goal is the absolute minimum work that closes one ticket cleanly, fully tested, with a PR body any onlooker can understand, then waits. **Local-only — exclude via `.git/info/exclude`.** All draft content lives in the conversation; user must confirm before any destructive or externally-visible action (PR open, force-push, label change, close). + +## Invocation + +```text +/nemoclaw-maintainer-issue-autopilot +``` + +Flags: + +| Flag | Default | Meaning | +|------|---------|---------| +| `--top N` | `8` | Candidates to surface in selection | +| `--max-files N` | `5` | Auto-halt if implementation touches more files | +| `--max-lines N` | `300` | Auto-halt if added lines exceed this | +| `--watch-ci` | `on` | Stay in CI watch loop after PR opens | +| `--cr-fix-loop` | `on` | Fix CodeRabbit comments automatically (re-prompts on each cycle) | +| `--dry-run` | `off` | Stop after presenting verdict; don't open PR | +| `--resume ` | `off` | Resume an interrupted run from its last checkpointed stage. Skips Stage 1 (selection) entirely and rehydrates from the state file. | + +## Resumable state (survives conversation breaks) + +A 9-stage pipeline can span hours. If the conversation gets compacted, the runtime crashes, or the user pauses overnight, the skill MUST be able to resume from the last completed stage without restarting Stage 1. + +**State file:** `/tmp/issue-autopilot-.state.json` + +**Schema:** + +```json +{ + "schema_version": 1, + "issue_number": 3259, + "repo": "NVIDIA/NemoClaw", + "started_at": "2026-05-14T17:00:00Z", + "last_updated": "2026-05-14T18:42:00Z", + "last_completed_stage": 6, + "stages": { + "1_selection": { "completed_at": "...", "picked_issue": 3259, "score": 4.3 }, + "2_scope": { "completed_at": "...", "verdict": "in-scope", "docs_anchor": "docs/manage-sandboxes/runtime-controls.md" }, + "3_repro": { "completed_at": "...", "method": "synthetic-docker", "evidence_path": "/tmp/repro-3259.log" }, + "4_implementation": { "completed_at": "...", "files_touched": ["src/lib/..."], "lines_added": 47, "lines_removed": 3, "branch": "fix/3259-..." }, + "5_pr": { "completed_at": "...", "pr_number": 3499, "pr_url": "https://..." }, + "6_self_review": { "completed_at": "...", "acceptance_map_path": "/tmp/3259-acceptance.md", "gaps_fixed": [] }, + "7_ci": { "completed_at": null, "last_check": "...", "failing_checks": [], "pre_existing_flakes": [] }, + "8_cr_fix": { "completed_at": null, "rounds": [], "open_critical": 0 }, + "9_ready": { "completed_at": null } + }, + "halts": [ + { "stage": 4, "at": "...", "reason": "max-files breach", "user_resolution": "approve-override" } + ] +} +``` + +**Write protocol:** + +- At every stage transition, OVERWRITE the state file atomically: `cat > /tmp/issue-autopilot-.state.json.tmp && mv ... .state.json` +- Never partial-write. Atomic rename is the only safe pattern. +- Include the conversation-derivable fields (verdicts, file lists, paths) — NOT raw diff content (that's reconstructible from `git diff`). + +**Read protocol on resume:** + +1. If `--resume ` is passed AND `/tmp/issue-autopilot-.state.json` exists, load it. +2. Print a summary table of completed-vs-pending stages, ask the user to confirm: "Resume from Stage ?" +3. On confirm, rehydrate any inferred state from disk (`git status` for branch, `gh pr view ` for CI/CR state). +4. Re-run any incomplete in-flight stage from the beginning of that stage (not from mid-stage) — stages must be idempotent per-execution. +5. If `--resume ` is passed but the state file is missing, halt and tell the user "no checkpoint found for #". + +**Cleanup:** when Stage 9 completes (READY FOR HUMAN REVIEW), do NOT delete the state file. Keep it around for postmortem/audit. The maintainer can `rm /tmp/issue-autopilot-*.state.json` periodically. + +**Validation pass on resume:** After rehydrating, verify the world hasn't moved underneath the checkpoint: + +- Branch in state file still exists locally (`git rev-parse --verify `) — if missing, halt +- PR in state file still open (`gh pr view --json state`) — if merged/closed, halt and ask user how to proceed +- Issue in state file still open (`gh issue view --json state`) — if closed, surface and ask whether to discard the run + +## Hard rules (these never bend) + +1. **One ticket per run.** No "while we're here" scope creep — extras go in `PROACTIVE-LOG.md`. +2. **Read scope before scoring.** `CLAUDE.md` (Project Overview + Architecture), `docs/` index, `.agents/skills/` audience buckets. Reject issues outside the documented surface. +3. **Reproduce or refute before fixing.** If you can't reproduce in <10 min (synthetic or actual), surface that and halt — bug reports with stale paths or impossible repros (#2757 was a case) get a comment, not a PR. +4. **Tests-first.** Acceptance criteria → test case → implementation → loop until green. Per Karpathy "goal-driven execution." +5. **Stop at every externally-visible step.** Open PR, push, label, close issue, post comment — confirm with user first. +6. **Identity check before commit.** Verify `git var GIT_AUTHOR_IDENT` matches the maintainer running the skill (not a stub like `Test User ` from a leftover local `.git/config` override) AND commit signing is configured (`%G?` = `G` after a test commit). Halt if either fails. Recovery: `git config --local --unset user.name && git config --local --unset user.email` so the global identity takes over; verify with `git var GIT_AUTHOR_IDENT`. + +## Workflow stages (execute in order, halt on block) + +> **Checkpoint reminder:** at the end of every stage, write the updated state file (`/tmp/issue-autopilot-.state.json`) before moving to the next stage. Atomic rename only. See the "Resumable state" section above for the full schema. This is a hard rule — without checkpoints, the resume flag is useless. + +### Stage 0 — Local-branch precheck (run before everything else) + +Before pulling any candidate, scan local git state for in-flight work the maintainer may have forgotten about: + +```bash +# Any branch with the candidate issue number in its name? +for issue in $CANDIDATE_ISSUES; do + hits=$(git branch -a --list "*${issue}*" 2>/dev/null) + if [ -n "$hits" ]; then + echo " ⚠ existing branch(es) for #${issue}:" + echo "$hits" | sed 's/^/ /' + fi +done + +# Any stash referencing the issue? +git stash list 2>/dev/null | grep -E "#?[0-9]+" | while read -r line; do + echo " ⚠ stash references issue(s): $line" +done + +# Any uncommitted changes? Surface as a courtesy — the autopilot won't proceed +# from a dirty tree. +[ -n "$(git status --porcelain)" ] && echo " ⚠ working tree dirty — commit or stash before Stage 4" +``` + +For any candidate that has matching local artifacts, surface them in Stage 1's selection table as a `local_artifacts` column. Two right actions when it fires: + +1. **Resume that local work** — the maintainer may already be halfway through. Instead of starting fresh, invoke `--resume ` if a state file exists, or check out the existing branch and continue manually. +2. **Discard the local artifacts** — only after confirming with the maintainer. The skill never auto-deletes branches or stashes. + +If a candidate has unresolved local artifacts and the maintainer doesn't confirm one of the two right actions, the skill **deprioritizes that candidate in the rank** (push to the bottom of the table). Don't auto-block — the maintainer might explicitly want to start fresh. + +This catches the failure mode where you start a new run on #N, get to Stage 4, and discover you already have a `fix/N-half-done` branch from three days ago. + +### Stage 1 — Selection + +Fetch open issues with lightweight fields only: + +```bash +gh issue list --repo NVIDIA/NemoClaw --state open --limit 200 \ + --json number,title,labels,createdAt,updatedAt,comments +``` + +Score candidates by **ease × impact × scope-fit × pr-state**, and surface **assigned-to-me** issues as a distinct top tier: + +- **Ease (1-5):** small file count guess (from title/body), clear acceptance criteria, no `status: blocked` / `needs-info` / `wontfix` / `enhancement: feature` labels. +- **Impact (1-5):** `priority: high` (+3), `priority: medium` (+1), `security` (+2), `bug` (+1), `NV QA` (+1), high comment count on recent activity (+1). +- **Scope-fit (0-3):** must map to a clear path in `CLAUDE.md`'s Architecture table OR docs/. Out-of-scope = 0 and disqualifies. +- **PR-state (-3 to +1):** open PR exists and APPROVED/ready-to-merge → −3 (skip); open PR stale or red CI → 0 (still a candidate, escalate in Stage 2); no PR → +1. +- **Assignee (+5):** issue is assigned to the running user (resolve dynamically via `gh api user --jq .login` at skill start). Always surface in its own pinned section at the top of the table — it's explicitly someone's workload, ignoring it is wrong. + +**Assigned-to-me discovery:** + +```bash +gh issue list --repo NVIDIA/NemoClaw --state open --assignee @me --limit 200 \ + --json number,title,labels,createdAt,updatedAt,comments +``` + +Merge that result with the general candidate pool and label each row with `assignee=` or blank. + +For each candidate, also surface in the table: `pr_state` column (`NO_PR` / `OPEN_` with state hint) and `assignee` column. User picks with that info visible — they may explicitly want to take over an `existing-pr-needs-work` case OR take an assigned-to-me issue first. + +Surface top-8 in a table with scope-fit + pr_state + assignee rationale per row, then **wait for user pick**. + +### Stage 2 — Scope validation (deep) + +For the picked issue, run these checks before any code work: + +0. **Read every comment, not just the body.** Run `gh api repos///issues//comments --paginate` and parse each comment. Comments often contain: + - Additional sub-bugs the reporter or others discovered after filing (e.g. #3456 comment 1 added "uninstall leaves residuals" as a 4th sub-bug not in the body) + - Workarounds that hint at the real root cause + - "Already-fixed in PR #N" notes (e.g. #3418 had a "fixed in #3367" comment that the autopilot missed by only reading the body) + - Reproductions on more platforms that widen the test matrix + The Stage 1 lightweight scan only reads the body — Stage 2 MUST enumerate sub-bugs from body + every comment together. If the combined sub-bug count or scope spans multiple subsystems, halt and ask the user to scope the picked work to ONE sub-bug. +1. **File paths cited still exist.** Grep the source for the file/line refs in the issue body. (Issue #3265 cited stale paths — caught via `find src/lib -name "local-inference*"`.) +2. **Repo policy match.** Does the fix area appear in `CLAUDE.md` § Architecture? In `docs/` somewhere? If neither, the issue is asking for a NEW surface — escalate. +3. **Already-fixed detection.** Three signals to check, any one triggers close-as-resolved: + - **Label**: issue has `fixed-on-latest`, `done`, `status: resolved`, or `status: superseded`. (Caught #3115 in autopilot run 3 — the issue still listed `priority` and `bug` but the maintainer team had marked `fixed-on-latest` before closing.) + - **Code grep**: search current `main` for the symptom or proposed fix. If the symptom no longer reproduces (e.g. #3418 claimed `nemoclaw/package.json` lacks a `test` script — but the script is already present on main), it's fixed. + - **Recent merged PR titles**: `gh pr list --state merged --search ""` — multiple merged PRs in the last 30 days referencing the issue strongly suggests it's been addressed in pieces (e.g. #3280 had 5+ merged commits before the autopilot would have picked it). + + Right action for any of these: **close-as-resolved** with a comment, not open a PR. +4. **Existing-PR triage.** Run `gh pr list --repo NVIDIA/NemoClaw --state open --search " in:body"` AND title-substring search. For each hit, classify: + - **READY_TO_MERGE** — PR diff covers every acceptance clause, CI green, has at least one approval (or `reviewDecision == "APPROVED"`). Action: **skip this issue**, don't duplicate effort. Surface the PR URL to the user as "already in flight". + - **NEEDS_REBASE_OR_FINISH** — PR addresses the issue but has CI red, unaddressed CR comments, or has been stale >14 days. Action: ask user whether to (a) rebase and finish that PR, (b) leave it alone and pick a different issue. + - **WRONG_DIRECTION** — PR is open but the diff doesn't actually solve the acceptance criteria (or solves the wrong sub-problem). Action: surface verdict to user; if they confirm, open a competing PR or comment on the existing one with the gap analysis. + - **NO_PR** — clean, proceed. + + Do this BEFORE coding. The previous Stage 1 filter "exclude any issue referenced in an open PR" was too coarse — it killed valid cases where the existing PR is stalled. +5. **Acceptance criteria are testable.** If the issue says "should feel snappier" → halt, ask for measurable criteria. + +Output a one-paragraph scope verdict (`in-scope` / `out-of-scope` / `needs-clarification` / `already-fixed` / `existing-pr-ready` / `existing-pr-needs-work`) and **wait for user confirmation** before coding. + +### Stage 3 — Reproduce or refute + +Cheap repro first — synthetic Docker container, a unit-test harness, or a one-line bash that demonstrates the bad behavior. ≤10 min budget. If you can't reproduce: + +- The bug report may have stale paths or wrong root cause (#2757 case). Draft a "request more info" comment and **halt** — don't proceed to fix. +- If repro is fundamentally not possible without prod infrastructure, flag that and halt. + +### Stage 4 — Test-first implementation + +1. Write the failing test(s) that map to the acceptance criteria. ONE test per criterion. Mock external dependencies (curl, openshell exec, fs). +2. Implement the minimum code to make them pass. +3. **Hard halt** if `git diff --stat` exceeds `--max-files` or `--max-lines` — present the diff, ask the user whether to (a) trim scope, (b) approve overrun, or (c) abandon. +4. Run typecheck (`npm run typecheck:cli`) and relevant unit tests on touched files. Loop until green. +5. Never `--no-verify` or `SKIP=` hooks unless the user explicitly approves — known pre-existing flake patterns (5s testTimeout in unrelated files) are the only documented exception. + +### Stage 5 — PR open (gated) + +Draft the PR body with these sections — present to user, get OK before `gh pr create`: + +- **Summary** — 1-2 lines of what changed. +- **Acceptance criteria mapping** — table: issue requirement → evidence (file:line / test name). +- **Behavior matrix** — for state-machine-like fixes, table of input → output. +- **Test plan** — exact commands to verify locally + manual repro steps. +- **Notes for reviewers** — anything not obvious from the diff. + +**PR body style — read the team's house style.** Different teams have different preferences: + +- Some teams want the PR body strictly technical (acceptance map, behavior matrix, test plan, reviewer notes) — plain-English analogies stay in conversation only, never in the public PR body. +- Other teams welcome plain-English / "explain this to a non-engineer" sections in the body to make the change accessible to non-developers reading the PR. + +If the repo has a `CONTRIBUTING.md` PR template or the team has a documented house style, follow that. If not, default to **technical-only** — it's the lower-risk choice for cross-team review. Always confirm with the maintainer before opening the PR if you're not sure which mode applies. + +Commit message: Conventional Commits, ends with the issue's `Closes #N`, signed-off-by, Co-Authored-By Claude. + +After user OK, open PR + apply labels matching the issue's labels (intersect with repo's available labels). **Confirm label list with user before applying.** + +### Stage 6 — Batch self-review + +Apply the `nemoclaw-maintainer-quick-wins` lens to your own PR: + +- **Two-lens judgment chain** (Scope/Coverage Lens + Sequencing Lens) — see `quick-wins/JUDGMENT-CHAIN.md`. Fail-fast. +- **Karpathy lens** — see `quick-wins/KARPATHY-LENS.md`. Simplicity, surgical, goal-driven. +- **Acceptance criteria 1:1 map** — every clause in the issue's "Expected" / "Acceptance" / "Proposed change" section must trace to a line in the PR diff OR an explicit "intentionally skipped because…" note. + +Report findings inline. If self-review surfaces a gap, fix it and add another commit BEFORE proceeding to CI watch. + +### Stage 7 — CI watch + flake triage + +```bash +gh pr view --repo NVIDIA/NemoClaw --json statusCheckRollup,reviewDecision,mergeStateStatus +``` + +For each failing check: + +- **Pre-existing flake on `main`?** Verify with `gh run list --workflow= --branch=main --limit=5`. If yes, note in conversation, do NOT attempt to fix. +- **Caused by this PR?** Drop into fix-then-recommit. + +Do not poll faster than every 60s. Use `Monitor` for "tell me when CI settles" if supported. + +### Stage 8 — CodeRabbit fix loop + +Fetch CR comments: + +```bash +gh api repos/NVIDIA/NemoClaw/pulls//comments --paginate +``` + +For each comment severity: + +| Severity | Action | +|---|---| +| Critical / Major | Auto-fix, present diff for user OK, push as `fix(scope): address CodeRabbit feedback on #NNNN` | +| Minor / Nit | Batch into one comment fix-up commit at the end | +| Question / Suggestion (no `Potential issue` flag) | Draft a reply for user to optionally post; do NOT auto-fix | + +After each round, re-run Stage 6 (batch self-review) on the updated PR to confirm acceptance still maps and nothing regressed. + +### Stage 9 — Acceptance perfect-match gate + Wait + +Before reporting READY FOR REVIEW, run an explicit **perfect-match audit** — no extra, no missing. + +**A. Extract acceptance clauses — LITERAL, not paraphrased.** Parse the issue body for every clause under "Expected", "Acceptance", "Proposed change", "Test strategy", "Steps to Reproduce" (final-state expectations), and any numbered requirement. **For lists of items the issue calls out by name** (e.g. "for each commonly changed item (model, provider, policy preset, openclaw.json keys, agents.list, channel tokens, dashboard port, GPU passthrough, sandbox name, shields posture)"), each named item is its own clause. Use the verbatim name as the row title — do not paraphrase or group, because paraphrasing hides gaps (the #3501 audit shipped 17/18 because "openclaw.json keys" was matched against keyword "openclaw" instead of the literal phrase, missing it). + +Each clause becomes a row in this table: + +| # | Clause (verbatim from issue) | Evidence (file:line / test name / CI step) | Status | +|---|---|---|---| + +`Status` ∈ `MET` / `MISSING` / `INTENTIONALLY_SKIPPED` (and justification if skipped). + +**B. Scan the diff for surplus.** Run `git diff --name-only origin/main..HEAD` and for every changed file, confirm at least one acceptance clause traces to changes in that file. Any file whose changes don't map to a clause is **surplus** — either revert it or document why it's required (e.g. test infrastructure). + +**C. Halt if either side fails:** + +- Any `MISSING` clause → **do not report ready.** Fix or escalate. +- Any unjustified surplus → **revert** the surplus changes; if user confirms it's needed, document the "intentional extra" in the PR body. + +**D. Final gate checklist (all must be ✅ to ship the READY message):** + +- [ ] Every acceptance clause maps to evidence (MET or INTENTIONALLY_SKIPPED with note) +- [ ] No surplus files / lines that don't trace to a clause +- [ ] CI green OR only pre-existing flakes (verified against `main` runs) +- [ ] CodeRabbit has no open `Potential issue` flags +- [ ] Every commit's `%G?` = `G` and `git log --pretty=format:'%an <%ae>'` matches the real maintainer +- [ ] `npm run typecheck:cli` clean on the final state +- [ ] Targeted unit tests pass (the ones that map to acceptance clauses) +- [ ] PR labels intersect the issue's labels (confirmed with user) + +**E. If all pass:** post **READY FOR HUMAN REVIEW** with the perfect-match table inline + a one-line RFR draft. **Stop.** Don't merge, don't request review, don't ping reviewers. + +**Why this gate matters:** the #3265 → #3498 dry-run shipped two intermediate states (one missing the rename, one with token-store extraction that wasn't required) before settling on the perfect match. The user caught both by asking "does this match acceptance, nothing more nothing less?" — that question is now this gate's job to answer before claiming done. + +## Halt conditions (these are the ones that aren't obvious) + +- **Three consecutive CR comments on the same file** — strong signal Stage 2's scope was wrong; abort and re-scope rather than thrash through fixups. +- **CodeRabbit flags a `Critical` requiring architectural rethink** — stop. Architectural rethink in Stage 8 means Stage 4 missed something fundamental; reopen scope. +- **CI red on a check this PR caused AND the fix isn't obvious in one commit** — same logic. One-commit fix = continue; n-commit fix loop = the PR is wrong. + +Generic halts (user says stop / can't reproduce / breach `--max-files` / identity check fails) are assumed. + +## Hard nos + +- No human-review bypass. No rebase / force-push outside the maintainer's explicit per-invocation request. No scope expansion ("extras → PROACTIVE-LOG.md, separate ticket"). No fixing pre-existing flakes inside this run. + +## JSON sidecar output + +In addition to the resumable state file documented above, the skill writes a final-result sidecar at run completion: `/tmp/nemoclaw-skill-output-issue-autopilot-.json`. + +**Envelope:** shared maintainer-skill schema (see `find-already-fixed/SKILL.md`). + +**Per-result shape (single object — one run, one issue):** + +```json +{ + "issue": 3259, + "issue_url": "https://...", + "pr": 3499, + "pr_url": "https://...", + "branch": "fix/3259-...", + "stages_completed": [1, 2, 3, 4, 5, 6, 7, 8, 9], + "halts": [], + "scope_verdict": "in-scope", + "acceptance_audit_path": "/tmp/nemoclaw-skill-output-acceptance-audit-.json", + "ci_final_state": "success" | "flake-noted" | "red", + "cr_rounds": 2, + "state_file": "/tmp/issue-autopilot-3259.state.json", + "ready_for_review_at": "" +} +``` + +Sub-skills invoked during the run (`quick-wins`, `acceptance-audit`, `ci-flake-triage`) write their own sidecars; this skill records pointers to them in the per-stage fields above. + +## Trust-but-verify (the non-obvious ones) + +- **"Test passes locally" ≠ "CI will pass."** Always rebuild `dist/` before running vitest against compiled output. Local stale `dist/` masks regressions. +- **"Issue body says line N" ≠ "line N is still there."** Refactors move things. Grep before assuming. +- **"CodeRabbit says X is broken" ≠ "X is broken."** CR agents hallucinate deletions that never existed (seen on PR #3295 — claimed ~120 LoC of GPU helpers deleted; never existed on main). Always verify CR claims against `git show main:`. +- **"DCO passed" ≠ "author identity is right."** DCO checks the `Signed-off-by` trailer string match, not name. Check `git log --pretty=format:'%h %an <%ae>'` before push — the `Test User` failure mode that produced Stage 0's identity check passed DCO and still shipped 4 wrong-author commits. diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index acb1c7a41f9..23783a15758 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -340,7 +340,7 @@ import type { } from "./onboard/types"; import { listChannels } from "./sandbox/channels"; import { streamGatewayStart } from "./onboard/gateway"; -import { gpuPassthroughRecoveryLines } from "./onboard/gpu-recovery"; +import { reportGpuPassthroughRecovery } from "./onboard/gpu-recovery"; import type { StreamSandboxCreateResult } from "./sandbox/create-stream"; import type { SandboxEntry } from "./state/registry"; import type { BackupResult } from "./state/sandbox"; @@ -10386,21 +10386,7 @@ async function onboard(opts: OnboardOptions = {}): Promise { const gpuOutput = String(gpuCheck.stdout || "").trim(); const gatewayHasGpu = gpuCheck.status === 0 && gpuOutput !== "null" && gpuOutput !== "[]"; if (!gatewayHasGpu) { - // #3456 sub-bug #3: emit registry-aware recovery hint instead of the - // previous hard-coded `` placeholder, which was not actionable - // when no sandbox was registered (the dead-loop State B path). - let registeredNames: string[] = []; - try { - registeredNames = registry - .listSandboxes() - .sandboxes.map((s) => s.name) - .filter(Boolean); - } catch { - /* registry unreadable — fall through to empty-list hint */ - } - for (const line of gpuPassthroughRecoveryLines(registeredNames)) { - console.error(line); - } + reportGpuPassthroughRecovery(console.error); process.exit(1); } } diff --git a/src/lib/onboard/gpu-recovery.test.ts b/src/lib/onboard/gpu-recovery.test.ts index 1eda000dca0..172501e63cc 100644 --- a/src/lib/onboard/gpu-recovery.test.ts +++ b/src/lib/onboard/gpu-recovery.test.ts @@ -10,8 +10,8 @@ * State B dead loop the reporter hit on six Linux hosts). */ -import { describe, expect, it } from "vitest"; -import { gpuPassthroughRecoveryLines } from "./gpu-recovery"; +import { describe, expect, it, vi } from "vitest"; +import { gpuPassthroughRecoveryLines, reportGpuPassthroughRecovery } from "./gpu-recovery"; describe("gpuPassthroughRecoveryLines", () => { it("never emits a literal `` placeholder for any input", () => { @@ -72,3 +72,21 @@ describe("gpuPassthroughRecoveryLines", () => { expect(joined).not.toMatch(/nemoclaw\s{2,}destroy/); }); }); + +describe("reportGpuPassthroughRecovery", () => { + it("emits the empty-registry path when loadNames returns no names", () => { + const emit = vi.fn(); + reportGpuPassthroughRecovery(emit, () => []); + const joined = emit.mock.calls.map((c) => c[0]).join("\n"); + expect(joined).toContain("nemoclaw uninstall"); + expect(joined).not.toMatch(//); + }); + + it("emits the multi-sandbox path when loadNames returns several names", () => { + const emit = vi.fn(); + reportGpuPassthroughRecovery(emit, () => ["alpha", "beta"]); + const joined = emit.mock.calls.map((c) => c[0]).join("\n"); + expect(joined).toContain("nemoclaw alpha destroy --yes"); + expect(joined).toContain("nemoclaw beta destroy --yes --cleanup-gateway"); + }); +}); diff --git a/src/lib/onboard/gpu-recovery.ts b/src/lib/onboard/gpu-recovery.ts index 52848a90d3a..51c770adf4b 100644 --- a/src/lib/onboard/gpu-recovery.ts +++ b/src/lib/onboard/gpu-recovery.ts @@ -9,9 +9,12 @@ * with a literal `` placeholder — not actionable when the registry was * empty (the State A / State B dead loop the reporter hit on six Linux * hosts). This helper renders the right shape based on what's actually - * registered. + * registered AND owns the registry lookup, so the onboard.ts callsite stays + * a single call (also keeps onboard.ts inside its size budget). */ +import * as registry from "../state/registry"; + /** * Returns the multi-line recovery hint for the GPU-passthrough mismatch * branch in onboard. Caller is expected to emit each line on its own line @@ -58,3 +61,31 @@ export function gpuPassthroughRecoveryLines(names: readonly string[] | null): st ...destroyLines, ]; } + +/** + * Read registered sandbox names with a graceful empty-list fallback when the + * registry can't be opened. Extracted so the onboard callsite stays a single + * line and so unit tests can inject their own list. + */ +export function getRegisteredSandboxNamesForGpuRecovery(): string[] { + try { + return registry + .listSandboxes() + .sandboxes.map((s) => s.name) + .filter(Boolean); + } catch { + return []; + } +} + +/** + * Emit the GPU-passthrough mismatch recovery hint to `emit` (typically + * `console.error`). `loadNames` is injectable for tests; the production + * default reads the on-disk sandbox registry. + */ +export function reportGpuPassthroughRecovery( + emit: (line: string) => void, + loadNames: () => string[] = getRegisteredSandboxNamesForGpuRecovery, +): void { + for (const line of gpuPassthroughRecoveryLines(loadNames())) emit(line); +} From b592bda1dac20c2c4fa659b7c4879b503016dd60 Mon Sep 17 00:00:00 2001 From: Charan Jagwani Date: Thu, 14 May 2026 09:27:37 -0700 Subject: [PATCH 3/3] chore: remove unrelated local-only skill file from PR (#3456) `.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md` was accidentally included in the previous commit (afe54274e). It's a local-only maintainer skill, not relevant to the #3456 fix. Drop it from the PR so the Stage 9 perfect-match audit shows no surplus. Signed-off-by: Charan Jagwani --- .../SKILL.md | 345 ------------------ 1 file changed, 345 deletions(-) delete mode 100644 .agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md diff --git a/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md b/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md deleted file mode 100644 index 36907c5035e..00000000000 --- a/.agents/skills/nemoclaw-maintainer-issue-autopilot/SKILL.md +++ /dev/null @@ -1,345 +0,0 @@ ---- -name: nemoclaw-maintainer-issue-autopilot -description: Ships a minimum-scope PR end-to-end for the simplest in-scope NemoClaw issue. Runs nine stages with user gates — local-branch precheck, selection, scope check against repo docs, reproduce-or-refute, test-first implementation, PR open, batch self-review, CI watch, CodeRabbit fix loop, perfect-match acceptance gate. Resumable state via /tmp/issue-autopilot-.state.json survives conversation breaks; identity check rejects 'Test User' fallbacks. Use when there's a clear in-scope ticket to ship, when the maintainer wants minimum-scope discipline enforced, or when resuming an interrupted issue→PR pipeline. Local-only — confirms at every externally-visible step. ---- - -# Issue Autopilot - -Autonomous "issue → merge-ready PR" pipeline for NemoClaw. The goal is the absolute minimum work that closes one ticket cleanly, fully tested, with a PR body any onlooker can understand, then waits. **Local-only — exclude via `.git/info/exclude`.** All draft content lives in the conversation; user must confirm before any destructive or externally-visible action (PR open, force-push, label change, close). - -## Invocation - -```text -/nemoclaw-maintainer-issue-autopilot -``` - -Flags: - -| Flag | Default | Meaning | -|------|---------|---------| -| `--top N` | `8` | Candidates to surface in selection | -| `--max-files N` | `5` | Auto-halt if implementation touches more files | -| `--max-lines N` | `300` | Auto-halt if added lines exceed this | -| `--watch-ci` | `on` | Stay in CI watch loop after PR opens | -| `--cr-fix-loop` | `on` | Fix CodeRabbit comments automatically (re-prompts on each cycle) | -| `--dry-run` | `off` | Stop after presenting verdict; don't open PR | -| `--resume ` | `off` | Resume an interrupted run from its last checkpointed stage. Skips Stage 1 (selection) entirely and rehydrates from the state file. | - -## Resumable state (survives conversation breaks) - -A 9-stage pipeline can span hours. If the conversation gets compacted, the runtime crashes, or the user pauses overnight, the skill MUST be able to resume from the last completed stage without restarting Stage 1. - -**State file:** `/tmp/issue-autopilot-.state.json` - -**Schema:** - -```json -{ - "schema_version": 1, - "issue_number": 3259, - "repo": "NVIDIA/NemoClaw", - "started_at": "2026-05-14T17:00:00Z", - "last_updated": "2026-05-14T18:42:00Z", - "last_completed_stage": 6, - "stages": { - "1_selection": { "completed_at": "...", "picked_issue": 3259, "score": 4.3 }, - "2_scope": { "completed_at": "...", "verdict": "in-scope", "docs_anchor": "docs/manage-sandboxes/runtime-controls.md" }, - "3_repro": { "completed_at": "...", "method": "synthetic-docker", "evidence_path": "/tmp/repro-3259.log" }, - "4_implementation": { "completed_at": "...", "files_touched": ["src/lib/..."], "lines_added": 47, "lines_removed": 3, "branch": "fix/3259-..." }, - "5_pr": { "completed_at": "...", "pr_number": 3499, "pr_url": "https://..." }, - "6_self_review": { "completed_at": "...", "acceptance_map_path": "/tmp/3259-acceptance.md", "gaps_fixed": [] }, - "7_ci": { "completed_at": null, "last_check": "...", "failing_checks": [], "pre_existing_flakes": [] }, - "8_cr_fix": { "completed_at": null, "rounds": [], "open_critical": 0 }, - "9_ready": { "completed_at": null } - }, - "halts": [ - { "stage": 4, "at": "...", "reason": "max-files breach", "user_resolution": "approve-override" } - ] -} -``` - -**Write protocol:** - -- At every stage transition, OVERWRITE the state file atomically: `cat > /tmp/issue-autopilot-.state.json.tmp && mv ... .state.json` -- Never partial-write. Atomic rename is the only safe pattern. -- Include the conversation-derivable fields (verdicts, file lists, paths) — NOT raw diff content (that's reconstructible from `git diff`). - -**Read protocol on resume:** - -1. If `--resume ` is passed AND `/tmp/issue-autopilot-.state.json` exists, load it. -2. Print a summary table of completed-vs-pending stages, ask the user to confirm: "Resume from Stage ?" -3. On confirm, rehydrate any inferred state from disk (`git status` for branch, `gh pr view ` for CI/CR state). -4. Re-run any incomplete in-flight stage from the beginning of that stage (not from mid-stage) — stages must be idempotent per-execution. -5. If `--resume ` is passed but the state file is missing, halt and tell the user "no checkpoint found for #". - -**Cleanup:** when Stage 9 completes (READY FOR HUMAN REVIEW), do NOT delete the state file. Keep it around for postmortem/audit. The maintainer can `rm /tmp/issue-autopilot-*.state.json` periodically. - -**Validation pass on resume:** After rehydrating, verify the world hasn't moved underneath the checkpoint: - -- Branch in state file still exists locally (`git rev-parse --verify `) — if missing, halt -- PR in state file still open (`gh pr view --json state`) — if merged/closed, halt and ask user how to proceed -- Issue in state file still open (`gh issue view --json state`) — if closed, surface and ask whether to discard the run - -## Hard rules (these never bend) - -1. **One ticket per run.** No "while we're here" scope creep — extras go in `PROACTIVE-LOG.md`. -2. **Read scope before scoring.** `CLAUDE.md` (Project Overview + Architecture), `docs/` index, `.agents/skills/` audience buckets. Reject issues outside the documented surface. -3. **Reproduce or refute before fixing.** If you can't reproduce in <10 min (synthetic or actual), surface that and halt — bug reports with stale paths or impossible repros (#2757 was a case) get a comment, not a PR. -4. **Tests-first.** Acceptance criteria → test case → implementation → loop until green. Per Karpathy "goal-driven execution." -5. **Stop at every externally-visible step.** Open PR, push, label, close issue, post comment — confirm with user first. -6. **Identity check before commit.** Verify `git var GIT_AUTHOR_IDENT` matches the maintainer running the skill (not a stub like `Test User ` from a leftover local `.git/config` override) AND commit signing is configured (`%G?` = `G` after a test commit). Halt if either fails. Recovery: `git config --local --unset user.name && git config --local --unset user.email` so the global identity takes over; verify with `git var GIT_AUTHOR_IDENT`. - -## Workflow stages (execute in order, halt on block) - -> **Checkpoint reminder:** at the end of every stage, write the updated state file (`/tmp/issue-autopilot-.state.json`) before moving to the next stage. Atomic rename only. See the "Resumable state" section above for the full schema. This is a hard rule — without checkpoints, the resume flag is useless. - -### Stage 0 — Local-branch precheck (run before everything else) - -Before pulling any candidate, scan local git state for in-flight work the maintainer may have forgotten about: - -```bash -# Any branch with the candidate issue number in its name? -for issue in $CANDIDATE_ISSUES; do - hits=$(git branch -a --list "*${issue}*" 2>/dev/null) - if [ -n "$hits" ]; then - echo " ⚠ existing branch(es) for #${issue}:" - echo "$hits" | sed 's/^/ /' - fi -done - -# Any stash referencing the issue? -git stash list 2>/dev/null | grep -E "#?[0-9]+" | while read -r line; do - echo " ⚠ stash references issue(s): $line" -done - -# Any uncommitted changes? Surface as a courtesy — the autopilot won't proceed -# from a dirty tree. -[ -n "$(git status --porcelain)" ] && echo " ⚠ working tree dirty — commit or stash before Stage 4" -``` - -For any candidate that has matching local artifacts, surface them in Stage 1's selection table as a `local_artifacts` column. Two right actions when it fires: - -1. **Resume that local work** — the maintainer may already be halfway through. Instead of starting fresh, invoke `--resume ` if a state file exists, or check out the existing branch and continue manually. -2. **Discard the local artifacts** — only after confirming with the maintainer. The skill never auto-deletes branches or stashes. - -If a candidate has unresolved local artifacts and the maintainer doesn't confirm one of the two right actions, the skill **deprioritizes that candidate in the rank** (push to the bottom of the table). Don't auto-block — the maintainer might explicitly want to start fresh. - -This catches the failure mode where you start a new run on #N, get to Stage 4, and discover you already have a `fix/N-half-done` branch from three days ago. - -### Stage 1 — Selection - -Fetch open issues with lightweight fields only: - -```bash -gh issue list --repo NVIDIA/NemoClaw --state open --limit 200 \ - --json number,title,labels,createdAt,updatedAt,comments -``` - -Score candidates by **ease × impact × scope-fit × pr-state**, and surface **assigned-to-me** issues as a distinct top tier: - -- **Ease (1-5):** small file count guess (from title/body), clear acceptance criteria, no `status: blocked` / `needs-info` / `wontfix` / `enhancement: feature` labels. -- **Impact (1-5):** `priority: high` (+3), `priority: medium` (+1), `security` (+2), `bug` (+1), `NV QA` (+1), high comment count on recent activity (+1). -- **Scope-fit (0-3):** must map to a clear path in `CLAUDE.md`'s Architecture table OR docs/. Out-of-scope = 0 and disqualifies. -- **PR-state (-3 to +1):** open PR exists and APPROVED/ready-to-merge → −3 (skip); open PR stale or red CI → 0 (still a candidate, escalate in Stage 2); no PR → +1. -- **Assignee (+5):** issue is assigned to the running user (resolve dynamically via `gh api user --jq .login` at skill start). Always surface in its own pinned section at the top of the table — it's explicitly someone's workload, ignoring it is wrong. - -**Assigned-to-me discovery:** - -```bash -gh issue list --repo NVIDIA/NemoClaw --state open --assignee @me --limit 200 \ - --json number,title,labels,createdAt,updatedAt,comments -``` - -Merge that result with the general candidate pool and label each row with `assignee=` or blank. - -For each candidate, also surface in the table: `pr_state` column (`NO_PR` / `OPEN_` with state hint) and `assignee` column. User picks with that info visible — they may explicitly want to take over an `existing-pr-needs-work` case OR take an assigned-to-me issue first. - -Surface top-8 in a table with scope-fit + pr_state + assignee rationale per row, then **wait for user pick**. - -### Stage 2 — Scope validation (deep) - -For the picked issue, run these checks before any code work: - -0. **Read every comment, not just the body.** Run `gh api repos///issues//comments --paginate` and parse each comment. Comments often contain: - - Additional sub-bugs the reporter or others discovered after filing (e.g. #3456 comment 1 added "uninstall leaves residuals" as a 4th sub-bug not in the body) - - Workarounds that hint at the real root cause - - "Already-fixed in PR #N" notes (e.g. #3418 had a "fixed in #3367" comment that the autopilot missed by only reading the body) - - Reproductions on more platforms that widen the test matrix - The Stage 1 lightweight scan only reads the body — Stage 2 MUST enumerate sub-bugs from body + every comment together. If the combined sub-bug count or scope spans multiple subsystems, halt and ask the user to scope the picked work to ONE sub-bug. -1. **File paths cited still exist.** Grep the source for the file/line refs in the issue body. (Issue #3265 cited stale paths — caught via `find src/lib -name "local-inference*"`.) -2. **Repo policy match.** Does the fix area appear in `CLAUDE.md` § Architecture? In `docs/` somewhere? If neither, the issue is asking for a NEW surface — escalate. -3. **Already-fixed detection.** Three signals to check, any one triggers close-as-resolved: - - **Label**: issue has `fixed-on-latest`, `done`, `status: resolved`, or `status: superseded`. (Caught #3115 in autopilot run 3 — the issue still listed `priority` and `bug` but the maintainer team had marked `fixed-on-latest` before closing.) - - **Code grep**: search current `main` for the symptom or proposed fix. If the symptom no longer reproduces (e.g. #3418 claimed `nemoclaw/package.json` lacks a `test` script — but the script is already present on main), it's fixed. - - **Recent merged PR titles**: `gh pr list --state merged --search ""` — multiple merged PRs in the last 30 days referencing the issue strongly suggests it's been addressed in pieces (e.g. #3280 had 5+ merged commits before the autopilot would have picked it). - - Right action for any of these: **close-as-resolved** with a comment, not open a PR. -4. **Existing-PR triage.** Run `gh pr list --repo NVIDIA/NemoClaw --state open --search " in:body"` AND title-substring search. For each hit, classify: - - **READY_TO_MERGE** — PR diff covers every acceptance clause, CI green, has at least one approval (or `reviewDecision == "APPROVED"`). Action: **skip this issue**, don't duplicate effort. Surface the PR URL to the user as "already in flight". - - **NEEDS_REBASE_OR_FINISH** — PR addresses the issue but has CI red, unaddressed CR comments, or has been stale >14 days. Action: ask user whether to (a) rebase and finish that PR, (b) leave it alone and pick a different issue. - - **WRONG_DIRECTION** — PR is open but the diff doesn't actually solve the acceptance criteria (or solves the wrong sub-problem). Action: surface verdict to user; if they confirm, open a competing PR or comment on the existing one with the gap analysis. - - **NO_PR** — clean, proceed. - - Do this BEFORE coding. The previous Stage 1 filter "exclude any issue referenced in an open PR" was too coarse — it killed valid cases where the existing PR is stalled. -5. **Acceptance criteria are testable.** If the issue says "should feel snappier" → halt, ask for measurable criteria. - -Output a one-paragraph scope verdict (`in-scope` / `out-of-scope` / `needs-clarification` / `already-fixed` / `existing-pr-ready` / `existing-pr-needs-work`) and **wait for user confirmation** before coding. - -### Stage 3 — Reproduce or refute - -Cheap repro first — synthetic Docker container, a unit-test harness, or a one-line bash that demonstrates the bad behavior. ≤10 min budget. If you can't reproduce: - -- The bug report may have stale paths or wrong root cause (#2757 case). Draft a "request more info" comment and **halt** — don't proceed to fix. -- If repro is fundamentally not possible without prod infrastructure, flag that and halt. - -### Stage 4 — Test-first implementation - -1. Write the failing test(s) that map to the acceptance criteria. ONE test per criterion. Mock external dependencies (curl, openshell exec, fs). -2. Implement the minimum code to make them pass. -3. **Hard halt** if `git diff --stat` exceeds `--max-files` or `--max-lines` — present the diff, ask the user whether to (a) trim scope, (b) approve overrun, or (c) abandon. -4. Run typecheck (`npm run typecheck:cli`) and relevant unit tests on touched files. Loop until green. -5. Never `--no-verify` or `SKIP=` hooks unless the user explicitly approves — known pre-existing flake patterns (5s testTimeout in unrelated files) are the only documented exception. - -### Stage 5 — PR open (gated) - -Draft the PR body with these sections — present to user, get OK before `gh pr create`: - -- **Summary** — 1-2 lines of what changed. -- **Acceptance criteria mapping** — table: issue requirement → evidence (file:line / test name). -- **Behavior matrix** — for state-machine-like fixes, table of input → output. -- **Test plan** — exact commands to verify locally + manual repro steps. -- **Notes for reviewers** — anything not obvious from the diff. - -**PR body style — read the team's house style.** Different teams have different preferences: - -- Some teams want the PR body strictly technical (acceptance map, behavior matrix, test plan, reviewer notes) — plain-English analogies stay in conversation only, never in the public PR body. -- Other teams welcome plain-English / "explain this to a non-engineer" sections in the body to make the change accessible to non-developers reading the PR. - -If the repo has a `CONTRIBUTING.md` PR template or the team has a documented house style, follow that. If not, default to **technical-only** — it's the lower-risk choice for cross-team review. Always confirm with the maintainer before opening the PR if you're not sure which mode applies. - -Commit message: Conventional Commits, ends with the issue's `Closes #N`, signed-off-by, Co-Authored-By Claude. - -After user OK, open PR + apply labels matching the issue's labels (intersect with repo's available labels). **Confirm label list with user before applying.** - -### Stage 6 — Batch self-review - -Apply the `nemoclaw-maintainer-quick-wins` lens to your own PR: - -- **Two-lens judgment chain** (Scope/Coverage Lens + Sequencing Lens) — see `quick-wins/JUDGMENT-CHAIN.md`. Fail-fast. -- **Karpathy lens** — see `quick-wins/KARPATHY-LENS.md`. Simplicity, surgical, goal-driven. -- **Acceptance criteria 1:1 map** — every clause in the issue's "Expected" / "Acceptance" / "Proposed change" section must trace to a line in the PR diff OR an explicit "intentionally skipped because…" note. - -Report findings inline. If self-review surfaces a gap, fix it and add another commit BEFORE proceeding to CI watch. - -### Stage 7 — CI watch + flake triage - -```bash -gh pr view --repo NVIDIA/NemoClaw --json statusCheckRollup,reviewDecision,mergeStateStatus -``` - -For each failing check: - -- **Pre-existing flake on `main`?** Verify with `gh run list --workflow= --branch=main --limit=5`. If yes, note in conversation, do NOT attempt to fix. -- **Caused by this PR?** Drop into fix-then-recommit. - -Do not poll faster than every 60s. Use `Monitor` for "tell me when CI settles" if supported. - -### Stage 8 — CodeRabbit fix loop - -Fetch CR comments: - -```bash -gh api repos/NVIDIA/NemoClaw/pulls//comments --paginate -``` - -For each comment severity: - -| Severity | Action | -|---|---| -| Critical / Major | Auto-fix, present diff for user OK, push as `fix(scope): address CodeRabbit feedback on #NNNN` | -| Minor / Nit | Batch into one comment fix-up commit at the end | -| Question / Suggestion (no `Potential issue` flag) | Draft a reply for user to optionally post; do NOT auto-fix | - -After each round, re-run Stage 6 (batch self-review) on the updated PR to confirm acceptance still maps and nothing regressed. - -### Stage 9 — Acceptance perfect-match gate + Wait - -Before reporting READY FOR REVIEW, run an explicit **perfect-match audit** — no extra, no missing. - -**A. Extract acceptance clauses — LITERAL, not paraphrased.** Parse the issue body for every clause under "Expected", "Acceptance", "Proposed change", "Test strategy", "Steps to Reproduce" (final-state expectations), and any numbered requirement. **For lists of items the issue calls out by name** (e.g. "for each commonly changed item (model, provider, policy preset, openclaw.json keys, agents.list, channel tokens, dashboard port, GPU passthrough, sandbox name, shields posture)"), each named item is its own clause. Use the verbatim name as the row title — do not paraphrase or group, because paraphrasing hides gaps (the #3501 audit shipped 17/18 because "openclaw.json keys" was matched against keyword "openclaw" instead of the literal phrase, missing it). - -Each clause becomes a row in this table: - -| # | Clause (verbatim from issue) | Evidence (file:line / test name / CI step) | Status | -|---|---|---|---| - -`Status` ∈ `MET` / `MISSING` / `INTENTIONALLY_SKIPPED` (and justification if skipped). - -**B. Scan the diff for surplus.** Run `git diff --name-only origin/main..HEAD` and for every changed file, confirm at least one acceptance clause traces to changes in that file. Any file whose changes don't map to a clause is **surplus** — either revert it or document why it's required (e.g. test infrastructure). - -**C. Halt if either side fails:** - -- Any `MISSING` clause → **do not report ready.** Fix or escalate. -- Any unjustified surplus → **revert** the surplus changes; if user confirms it's needed, document the "intentional extra" in the PR body. - -**D. Final gate checklist (all must be ✅ to ship the READY message):** - -- [ ] Every acceptance clause maps to evidence (MET or INTENTIONALLY_SKIPPED with note) -- [ ] No surplus files / lines that don't trace to a clause -- [ ] CI green OR only pre-existing flakes (verified against `main` runs) -- [ ] CodeRabbit has no open `Potential issue` flags -- [ ] Every commit's `%G?` = `G` and `git log --pretty=format:'%an <%ae>'` matches the real maintainer -- [ ] `npm run typecheck:cli` clean on the final state -- [ ] Targeted unit tests pass (the ones that map to acceptance clauses) -- [ ] PR labels intersect the issue's labels (confirmed with user) - -**E. If all pass:** post **READY FOR HUMAN REVIEW** with the perfect-match table inline + a one-line RFR draft. **Stop.** Don't merge, don't request review, don't ping reviewers. - -**Why this gate matters:** the #3265 → #3498 dry-run shipped two intermediate states (one missing the rename, one with token-store extraction that wasn't required) before settling on the perfect match. The user caught both by asking "does this match acceptance, nothing more nothing less?" — that question is now this gate's job to answer before claiming done. - -## Halt conditions (these are the ones that aren't obvious) - -- **Three consecutive CR comments on the same file** — strong signal Stage 2's scope was wrong; abort and re-scope rather than thrash through fixups. -- **CodeRabbit flags a `Critical` requiring architectural rethink** — stop. Architectural rethink in Stage 8 means Stage 4 missed something fundamental; reopen scope. -- **CI red on a check this PR caused AND the fix isn't obvious in one commit** — same logic. One-commit fix = continue; n-commit fix loop = the PR is wrong. - -Generic halts (user says stop / can't reproduce / breach `--max-files` / identity check fails) are assumed. - -## Hard nos - -- No human-review bypass. No rebase / force-push outside the maintainer's explicit per-invocation request. No scope expansion ("extras → PROACTIVE-LOG.md, separate ticket"). No fixing pre-existing flakes inside this run. - -## JSON sidecar output - -In addition to the resumable state file documented above, the skill writes a final-result sidecar at run completion: `/tmp/nemoclaw-skill-output-issue-autopilot-.json`. - -**Envelope:** shared maintainer-skill schema (see `find-already-fixed/SKILL.md`). - -**Per-result shape (single object — one run, one issue):** - -```json -{ - "issue": 3259, - "issue_url": "https://...", - "pr": 3499, - "pr_url": "https://...", - "branch": "fix/3259-...", - "stages_completed": [1, 2, 3, 4, 5, 6, 7, 8, 9], - "halts": [], - "scope_verdict": "in-scope", - "acceptance_audit_path": "/tmp/nemoclaw-skill-output-acceptance-audit-.json", - "ci_final_state": "success" | "flake-noted" | "red", - "cr_rounds": 2, - "state_file": "/tmp/issue-autopilot-3259.state.json", - "ready_for_review_at": "" -} -``` - -Sub-skills invoked during the run (`quick-wins`, `acceptance-audit`, `ci-flake-triage`) write their own sidecars; this skill records pointers to them in the per-stage fields above. - -## Trust-but-verify (the non-obvious ones) - -- **"Test passes locally" ≠ "CI will pass."** Always rebuild `dist/` before running vitest against compiled output. Local stale `dist/` masks regressions. -- **"Issue body says line N" ≠ "line N is still there."** Refactors move things. Grep before assuming. -- **"CodeRabbit says X is broken" ≠ "X is broken."** CR agents hallucinate deletions that never existed (seen on PR #3295 — claimed ~120 LoC of GPU helpers deleted; never existed on main). Always verify CR claims against `git show main:`. -- **"DCO passed" ≠ "author identity is right."** DCO checks the `Signed-off-by` trailer string match, not name. Check `git log --pretty=format:'%h %an <%ae>'` before push — the `Test User` failure mode that produced Stage 0's identity check passed DCO and still shipped 4 wrong-author commits.