diff --git a/.github/workflows/managed-images.yaml b/.github/workflows/managed-images.yaml index fd72bb1bf2d..5322b77f188 100644 --- a/.github/workflows/managed-images.yaml +++ b/.github/workflows/managed-images.yaml @@ -111,8 +111,8 @@ jobs: CANDIDATE_SHA: ${{ github.event.pull_request.head.sha }} # Retains the reviewed discovery-permission repair and the current # managed-image security inventory. The previous staging source pinned - # Vim 9.2.0782, which cannot satisfy the candidate's 9.2.0858 contract. - STAGING_QA_SOURCE_SHA: af2a73f0d6ce8f08a2975560f376470387c535d0 + # libssh2 nemoclaw1, which cannot satisfy the candidate's nemoclaw2 contract. + STAGING_QA_SOURCE_SHA: ce96811ddb418ad01c040521a1fe912b5bcb405e STAGING_QA_BASE_IMAGE: nemoclaw-deepagents-code-base:staging-31396519688 STAGING_QA_FINAL_IMAGE: nemoclaw-managed-pr/langchain-deepagents-code-staging-qa steps: diff --git a/scripts/checks/no-defaulted-dependent-flags.mts b/scripts/checks/no-defaulted-dependent-flags.mts new file mode 100644 index 00000000000..9b5d49fc8da --- /dev/null +++ b/scripts/checks/no-defaulted-dependent-flags.mts @@ -0,0 +1,152 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Guard oclif flag definitions against combining `default` with `dependsOn`. + * + * oclif validates dependsOn whenever the flag has a value. A parser default + * always supplies one, so oclif rejects every invocation that omits the + * dependency. Apply defaults in the action layer instead, as channels status + * does for --timeout (#8883). + * + * The scan covers direct `Flags.({...})` object literals. Options + * passed through `Flags.custom` factories, spread composition, or aliased + * imports are out of scope; no flag under `src` or `nemoclaw/src` combines + * them with dependsOn today. + */ + +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import ts from "typescript"; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../.."); +const SCAN_ROOTS = ["src", "nemoclaw/src"]; +const SKIP_DIRS = new Set([".git", "coverage", "dist", "node_modules"]); + +export interface DefaultedDependentFlagViolation { + filePath: string; + line: number; + flagName: string; +} + +function flagObjectPropertyNames(node: ts.ObjectLiteralExpression): string[] { + return node.properties.flatMap((property) => + (ts.isPropertyAssignment(property) || + ts.isShorthandPropertyAssignment(property) || + ts.isMethodDeclaration(property)) && + (ts.isIdentifier(property.name) || ts.isStringLiteral(property.name)) + ? [property.name.text] + : [], + ); +} + +function flagNameFor(callExpression: ts.CallExpression): string { + const parent = callExpression.parent; + return ts.isPropertyAssignment(parent) && + (ts.isIdentifier(parent.name) || ts.isStringLiteral(parent.name)) + ? parent.name.text + : "(unnamed flag)"; +} + +export function findDefaultedDependentFlags( + sourceText: string, + filePath: string, +): DefaultedDependentFlagViolation[] { + if (!sourceText.includes("dependsOn")) return []; + const sourceFile = ts.createSourceFile(filePath, sourceText, ts.ScriptTarget.Latest, true); + const violations: DefaultedDependentFlagViolation[] = []; + const visit = (node: ts.Node): void => { + if ( + ts.isCallExpression(node) && + ts.isPropertyAccessExpression(node.expression) && + ts.isIdentifier(node.expression.expression) && + node.expression.expression.text === "Flags" && + node.arguments.length > 0 && + ts.isObjectLiteralExpression(node.arguments[0]) + ) { + const names = flagObjectPropertyNames(node.arguments[0]); + if (names.includes("dependsOn") && names.includes("default")) { + const { line } = sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)); + violations.push({ filePath, line: line + 1, flagName: flagNameFor(node) }); + } + } + ts.forEachChild(node, visit); + }; + visit(sourceFile); + return violations; +} + +export function checkFiles(filePaths: readonly string[]): DefaultedDependentFlagViolation[] { + return filePaths.flatMap((filePath) => { + const absolutePath = path.resolve(REPO_ROOT, filePath); + return findDefaultedDependentFlags( + fs.readFileSync(absolutePath, "utf-8"), + path.relative(REPO_ROOT, absolutePath).split(path.sep).join("/"), + ); + }); +} + +export function formatViolations( + violations: readonly DefaultedDependentFlagViolation[], +): string { + return [ + "oclif flags must not combine a parser default with dependsOn.", + "The default gives the flag a value on every parse, so oclif applies", + "dependsOn validation and rejects each invocation that omits the", + "dependency (#8883). Apply the default in the action layer instead.", + "", + ...violations.map( + (violation) => `${violation.filePath}:${violation.line} ${violation.flagName}`, + ), + ].join("\n"); +} + +export function isScannedSourcePath(filePath: string): boolean { + return ( + SCAN_ROOTS.some((root) => filePath.startsWith(`${root}/`)) && + filePath.endsWith(".ts") && + !filePath.endsWith(".test.ts") && + !filePath.endsWith(".test-helpers.ts") && + !filePath.endsWith(".d.ts") + ); +} + +function sourceFiles(): string[] { + return SCAN_ROOTS.flatMap((root) => [...walkSourceFiles(path.join(REPO_ROOT, root))]) + .map((filePath) => path.relative(REPO_ROOT, filePath).split(path.sep).join("/")) + .filter(isScannedSourcePath); +} + +function* walkSourceFiles(dir: string): Generator { + if (!fs.existsSync(dir)) return; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (!SKIP_DIRS.has(entry.name)) yield* walkSourceFiles(fullPath); + continue; + } + if (entry.isFile() && entry.name.endsWith(".ts")) yield fullPath; + } +} + +function normalizeCliPaths(args: readonly string[]): string[] { + return args + .filter((arg) => arg !== "--") + .map((arg) => path.relative(REPO_ROOT, path.resolve(arg)).split(path.sep).join("/")) + .filter(isScannedSourcePath); +} + +function main(): void { + const cliPaths = normalizeCliPaths(process.argv.slice(2)); + const filePaths = cliPaths.length > 0 ? cliPaths : sourceFiles(); + const violations = checkFiles(filePaths); + if (violations.length > 0) { + console.error(formatViolations(violations)); + process.exitCode = 1; + } +} + +if (fileURLToPath(import.meta.url) === path.resolve(process.argv[1] ?? "")) { + main(); +} diff --git a/scripts/checks/run.mts b/scripts/checks/run.mts index fdef63a22c1..f1d1b6282ca 100644 --- a/scripts/checks/run.mts +++ b/scripts/checks/run.mts @@ -53,6 +53,11 @@ export const CHECKS: readonly CheckCommand[] = [ command: TSX, args: ["scripts/checks/dependency-pins.mts"], }, + { + name: "no-defaulted-dependent-flags", + command: TSX, + args: ["scripts/checks/no-defaulted-dependent-flags.mts"], + }, { name: "no-coverage-ignore", command: TSX, diff --git a/src/commands/sandbox/channels/status.test.ts b/src/commands/sandbox/channels/status.test.ts index f849af7ba33..29bbaf3cc7a 100644 --- a/src/commands/sandbox/channels/status.test.ts +++ b/src/commands/sandbox/channels/status.test.ts @@ -50,11 +50,27 @@ describe("SandboxChannelsStatusCommand readiness flags", () => { rootDir, ); + expect(showSandboxChannelStatusMock).toHaveBeenCalledWith("alpha", { + channel: "slack", + asJson: true, + quietJson: true, + wait: true, + timeoutSeconds: undefined, + }); + expect(process.exitCode).toBe(1); + }); + + it.each([ + [["alpha"], undefined], + [["alpha", "--channel", "slack"], "slack"], + ] as const)("accepts the documented no-wait invocation %j (#8883)", async (argv, channel) => { + await SandboxChannelsStatusCommand.run([...argv], rootDir); + + expect(showSandboxChannelStatusMock).toHaveBeenCalledTimes(1); expect(showSandboxChannelStatusMock).toHaveBeenCalledWith( "alpha", - expect.objectContaining({ timeoutSeconds: 180 }), + expect.objectContaining({ channel, wait: undefined, timeoutSeconds: undefined }), ); - expect(process.exitCode).toBe(1); }); it.each([ diff --git a/src/commands/sandbox/channels/status.ts b/src/commands/sandbox/channels/status.ts index 2decc38be86..7a495823e75 100644 --- a/src/commands/sandbox/channels/status.ts +++ b/src/commands/sandbox/channels/status.ts @@ -34,8 +34,11 @@ export default class SandboxChannelsStatusCommand extends NemoClawCommand { }), timeout: Flags.integer({ dependsOn: ["wait"], - description: "Readiness timeout in seconds", - default: 180, + // No parser default: oclif validates dependsOn whenever the flag has a + // value, so a default makes oclif reject every invocation that omits + // --wait (#8883). showSandboxChannelStatus applies the 180-second + // budget documented in docs/reference/commands.mdx. + description: "Readiness timeout in seconds (default: 180)", min: 1, }), }; diff --git a/src/lib/actions/sandbox/channel-status.test.ts b/src/lib/actions/sandbox/channel-status.test.ts index ad9333e3e27..f92f4fabd6c 100644 --- a/src/lib/actions/sandbox/channel-status.test.ts +++ b/src/lib/actions/sandbox/channel-status.test.ts @@ -573,6 +573,28 @@ describe("showSandboxChannelStatus Slack readiness wait", () => { expect(configRead.mock.calls.map(([timeoutMs]) => timeoutMs)).toEqual([1_150]); expect(sleep).toHaveBeenCalledWith(500); }); + + it("applies the documented 180-second budget when the caller omits timeoutSeconds (#8883)", async () => { + const { deps, gatewayPolicy } = slackWaitHarness([{ connected: false }]); + + const result = await showSandboxChannelStatus("alpha", { + deps, + channel: "slack", + wait: true, + timeoutSeconds: undefined, + pollIntervalMs: 60_000, + asJson: true, + quietJson: true, + }); + + expect(result && "readiness" in result ? result.readiness : null).toMatchObject({ + state: "timeout", + category: "timeout", + reason: "timeout", + elapsedMs: 180_000, + }); + expect(gatewayPolicy.mock.calls[0]?.[1]).toBe(180_000); + }); }); describe("showSandboxChannelStatus unsupported readiness wait", () => { diff --git a/test/checks-runner.test.ts b/test/checks-runner.test.ts index 66718c3950f..4e0c5bb0f30 100644 --- a/test/checks-runner.test.ts +++ b/test/checks-runner.test.ts @@ -33,6 +33,14 @@ describe("checks runner", () => { }); }); + it("registers the defaulted dependent flag check (#8883)", () => { + expect(CHECKS).toContainEqual({ + name: "no-defaulted-dependent-flags", + command: process.platform === "win32" ? "tsx.cmd" : "tsx", + args: ["scripts/checks/no-defaulted-dependent-flags.mts"], + }); + }); + it("runs Windows command shims through cmd.exe", () => { expect( buildCheckSpawnInvocation(sampleCheck, "win32", { diff --git a/test/managed-image-publication-workflow.test.ts b/test/managed-image-publication-workflow.test.ts index 6fe551bcfbe..024b008a20a 100644 --- a/test/managed-image-publication-workflow.test.ts +++ b/test/managed-image-publication-workflow.test.ts @@ -725,7 +725,7 @@ describe("complete managed-image publication workflow", () => { expect(qaBuilder.permissions).toEqual({ contents: "read" }); expect(qaBuilder.env).toMatchObject({ CANDIDATE_SHA: "${{ github.event.pull_request.head.sha }}", - STAGING_QA_SOURCE_SHA: "af2a73f0d6ce8f08a2975560f376470387c535d0", + STAGING_QA_SOURCE_SHA: "ce96811ddb418ad01c040521a1fe912b5bcb405e", STAGING_QA_BASE_IMAGE: "nemoclaw-deepagents-code-base:staging-31396519688", }); expect(qaBuilder.env).not.toHaveProperty("STAGING_PRODUCER_SHA"); diff --git a/test/no-defaulted-dependent-flags.test.ts b/test/no-defaulted-dependent-flags.test.ts new file mode 100644 index 00000000000..5940da05247 --- /dev/null +++ b/test/no-defaulted-dependent-flags.test.ts @@ -0,0 +1,64 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; +import { + findDefaultedDependentFlags, + isScannedSourcePath, +} from "../scripts/checks/no-defaulted-dependent-flags.mts"; + +describe("defaulted dependent flag guard", () => { + it("reports a Flags.integer definition that combines default with dependsOn (#8883)", () => { + const source = [ + "const flags = {", + " timeout: Flags.integer({", + ' dependsOn: ["wait"],', + " default: 180,", + " }),", + "};", + ].join("\n"); + + expect(findDefaultedDependentFlags(source, "src/example.ts")).toEqual([ + { filePath: "src/example.ts", line: 2, flagName: "timeout" }, + ]); + }); + + it("flags a function-valued default, which oclif also resolves on every parse", () => { + const source = 'const f = Flags.string({ dependsOn: ["wait"], default: () => "x" });'; + + expect(findDefaultedDependentFlags(source, "src/example.ts")).toMatchObject([ + { line: 1, flagName: "(unnamed flag)" }, + ]); + }); + + it("allows dependsOn without a default and a default without dependsOn", () => { + const source = [ + 'const a = Flags.integer({ dependsOn: ["wait"], min: 1 });', + "const b = Flags.integer({ default: 180 });", + ].join("\n"); + + expect(findDefaultedDependentFlags(source, "src/example.ts")).toEqual([]); + }); + + it("ignores non-Flags calls that combine the same option names", () => { + const source = 'options({ dependsOn: ["wait"], default: 180 });'; + + expect(findDefaultedDependentFlags(source, "src/example.ts")).toEqual([]); + }); +}); + +describe("scanned source path selection", () => { + it("scans source TypeScript under src and nemoclaw/src", () => { + expect(isScannedSourcePath("src/commands/sandbox/channels/status.ts")).toBe(true); + expect(isScannedSourcePath("nemoclaw/src/commands/example.ts")).toBe(true); + }); + + it("excludes tests, declarations, and paths outside the scan roots", () => { + expect(isScannedSourcePath("src/commands/sandbox/channels/status.test.ts")).toBe(false); + expect(isScannedSourcePath("src/lib/actions/sandbox/channel-status.test-helpers.ts")).toBe( + false, + ); + expect(isScannedSourcePath("src/lib/example.d.ts")).toBe(false); + expect(isScannedSourcePath("scripts/checks/run.mts")).toBe(false); + }); +});