diff --git a/docs/SURFACE.md b/docs/SURFACE.md index b5ed97a75..d05071355 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -309,6 +309,14 @@ No process runs between events: the handler wakes, executes to its next await, p deterministic-command preflight gap.โ€ Consequently, `cli_missing` applies to declared `llm` and `agent` CLIs, not deterministic command words. + For YAML/JSON specs, `flows check` summarises `unprovable_effects` warnings + affecting two or more steps in one trailing warning line. The denominator + counts every step in the spec, including `llm` and `agent` steps. + Use `--explain-warnings` (also with `--watch`) to list the original per-step + warnings; `--json` always retains every diagnostic in its stdout report. + Other diagnostic kinds remain individual. `flows run`, `flows resume`, and + `flows deploy` still print individual warnings. + **Project-config discovery:** starting in the flow file's directory, `flows check` walks parent directories through the filesystem root and selects the first readable `flows.json`. That nearest file is the whole project config; it is not merged with outer files. Its schema is `{ "cli"?: , "executors"?: [], "models"?: [], "mcp"?: }`; unknown keys, malformed model entries, and duplicates fail closed as `config_invalid`. A nearer config therefore defines a self-contained nested project boundary and prevents accidental inheritance of outer credentials, executors, or model approvals. The selected path is printed with project-level resolutions and named in refusals; if it declares no `cli` or models, outer configs remain shadowed. At gate 1, a trigger executor is considered registered only when its name is present in this author-written `executors` array; `flows check` does not yet contact a registry, broker, or RelayCron, and absence is `no_executor`. Implementation status for issue #132: this named-agent contract currently @@ -1047,7 +1055,7 @@ verbs: `observer`, which mints an observer link without contacting the daemon, and `status`, which reads a run's journal without one: ```text -flows check [--watch] [--json] +flows check [--watch] [--json] [--explain-warnings] flows run [--json] [--no-spawn] [--data-dir ] flows run [--json] [--no-spawn] [--data-dir ] --input flows resume [--json] [--no-spawn] [--data-dir ] diff --git a/ops/BACKLOG.md b/ops/BACKLOG.md index e70f11b6b..98a4b465b 100644 --- a/ops/BACKLOG.md +++ b/ops/BACKLOG.md @@ -3,6 +3,11 @@ Items the Lead should weigh in assess after ops/DIRECTIVES.md and the current gate's needs. Not commitments; ordering is the Lead's call with evidence. +- **Extend unprovable-effects warning summaries beyond `check`.** + `flows check` folds repeated warnings and offers `--explain-warnings`. + `run`/`resume`/`deploy` retain individual warnings; consider the same rendering + with an appropriate way to request details (run reports have no step total). + - **`timeoutMs` is enforced LATE, not never โ€” a step ran 2.3x past its limit.** Run 6d9bc35c's `verify-1` declares `timeoutMs: 1200000` (20 min) in workflows/drive-cloud.yaml. It ran about 46 minutes โ€” log frozen at 19628 diff --git a/packages/sdk/src/cli-commands.ts b/packages/sdk/src/cli-commands.ts index 402f25d6e..0c5835f45 100644 --- a/packages/sdk/src/cli-commands.ts +++ b/packages/sdk/src/cli-commands.ts @@ -148,7 +148,8 @@ export const CLI_VERBS = [ name: 'check', description: 'Compile and preflight a flow without running it, or opening a daemon socket', args: [{ name: 'source', description: 'flow.ts, flow.yaml, or spec.json', required: true }], - options: [JSON_OPTION, { flags: '--watch', description: 'Re-check on every change to the flow and its imports' }], + options: [JSON_OPTION, { flags: '--watch', description: 'Re-check on every change to the flow and its imports' }, + { flags: '--explain-warnings', description: 'List every unprovable-effects warning per step instead of one summary line' }], variants: ['check'], }, { diff --git a/packages/sdk/src/cli-watch.ts b/packages/sdk/src/cli-watch.ts index 2fe2a6dbb..b126d0026 100644 --- a/packages/sdk/src/cli-watch.ts +++ b/packages/sdk/src/cli-watch.ts @@ -11,7 +11,7 @@ type ExitCode = 0 | 1 | 2 | 3; /** A runner around the ordinary CLI, with a fresh module cache for every check. */ export async function watchCheck( path: string, - json: boolean, + options: { json: boolean; explainWarnings: boolean }, io: CliIo, /** Cancellation, owned by the caller; this function installs no signal handler. */ signal: AbortSignal, @@ -20,8 +20,8 @@ export async function watchCheck( return await watchChecks({ path, signal, - check: () => checkOnce(path, json, io), - clear: () => { if (!json) io.stdout('\x1b[2J\x1b[H'); }, + check: () => checkOnce(path, options, io), + clear: () => { if (!options.json) io.stdout('\x1b[2J\x1b[H'); }, }); } catch (error) { io.stderr(`REFUSED [input_unreadable] Could not watch "${path}": ${error instanceof Error ? error.message : String(error)}`); @@ -140,13 +140,13 @@ function discoverWatchPaths(path: string, imports: Map): Set { +function checkOnce(path: string, options: { json: boolean; explainWarnings: boolean }, io: CliIo): Promise { // Importing an authored TS file in this process again would return its stale // cached definition (including transitive imports). A fresh CLI process uses // precisely the same check pipeline and reporting as a one-shot invocation. const entry = fileURLToPath(new URL(import.meta.url.endsWith('.ts') ? './cli.ts' : './cli.js', import.meta.url)); return new Promise((resolveCode, reject) => { - const child = spawn(process.execPath, [...process.execArgv, entry, 'check', ...(json ? ['--json'] : []), path], { + const child = spawn(process.execPath, [...process.execArgv, entry, 'check', ...(options.json ? ['--json'] : []), ...(options.explainWarnings ? ['--explain-warnings'] : []), path], { stdio: ['ignore', 'pipe', 'pipe'], // Ctrl-C belongs to the watcher; let an active check finish so its report // and exit status remain a pair, then release all resources. diff --git a/packages/sdk/src/cli.ts b/packages/sdk/src/cli.ts index db6b7e000..086b07093 100644 --- a/packages/sdk/src/cli.ts +++ b/packages/sdk/src/cli.ts @@ -1,4 +1,5 @@ #!/usr/bin/env node +import { foldUnprovableEffects, type FoldOptions } from './cli/diagnostic-fold.js'; import { startDetachedRun, emitDetachedHandle } from './cli/detached-run.js'; import { takeDetachedReceipt, type DetachedReceipt } from './cli/detached-record.js'; import { withSubscriptionMetadata } from './cli/subscription-report.js'; @@ -105,7 +106,7 @@ export type ParsedArgs = | CloudScheduleArgs | { command: 'schedules'; json: boolean } | { command: 'unschedule'; scheduleId: string; json: boolean } - | { command: 'check'; json: boolean; watch: boolean; value: string } + | { command: 'check'; json: boolean; watch: boolean; explainWarnings: boolean; value: string } | { command: 'run'; detach?: boolean; bucket: string | undefined; reuseFromRunId: string | undefined; localAgent: boolean; agentCapacity: number | undefined; dataDir: string; input: string | undefined; json: boolean; spawn: boolean; noObserverLink: boolean; cloudMirror: boolean; allowHumanInfluenced: boolean; value: string } | { command: 'resume'; detach?: boolean; localAgent: boolean; agentCapacity: number | undefined; dataDir: string; json: boolean; spawn: boolean; noObserverLink: boolean; cloudMirror: boolean; allowHumanInfluenced: boolean; value: string } | { command: 'answer'; dataDir: string; json: boolean; spawn: boolean; note: string | undefined; by: string | undefined; runId: string; waitId: string; answer: boolean } @@ -138,7 +139,7 @@ const USAGE = [ 'flows unschedule [--json] ', 'flows deploy @sha256: --to ', 'flows run @sha256: [--bucket ] [--data-dir ] [--json] [--detach]', - 'flows check [--watch] [--json] ', + 'flows check [--watch] [--json] [--explain-warnings] ', 'flows serve-webhook --data-dir --port

[--allow [,]]', 'flows run [--json] [--no-spawn] [--no-observer-link] [--cloud-mirror] [--detach] [--data-dir

] [--local-agent [--agent-capacity ]] [--reuse-from ] ', 'flows run --cloud [--json] [--wait] [--sync-code] [--no-connect] ', @@ -310,7 +311,7 @@ export async function runCli( if (parsed.command === 'check') { if (parsed.watch) { - return withInterrupt(options.signal, (signal) => watchCheck(parsed.value, parsed.json, io, signal)); + return withInterrupt(options.signal, (signal) => watchCheck(parsed.value, { json: parsed.json, explainWarnings: parsed.explainWarnings }, io, signal)); } // Deliberately daemon-free (kernel/DAEMON-LIFECYCLE.md ยง4). `checkFlow` is // a compile-and-preflight that opens no daemon socket, and the parser @@ -324,7 +325,7 @@ export async function runCli( const checked = /\.(?:[cm]?[jt]s)$/.test(parsed.value) ? await checkAuthoredFlowComposed(parsed.value) : checkFlow(parsed.value, { warnUnresolvedAgentWorker: true }); - emitCheckReport(checked.report, parsed.json, io); + emitCheckReport(checked.report, parsed.json, io, parsed.explainWarnings); return checked.report.ok ? 0 : 2; } @@ -693,6 +694,7 @@ function parseArgs(args: readonly string[]): ParsedArgs | undefined { let json = false; let watch = false; + let explainWarnings = false; let cloud = false; let wait = false; let syncCode = false; @@ -745,6 +747,11 @@ function parseArgs(args: readonly string[]): ParsedArgs | undefined { if (!isAgentCapacity(agentCapacity)) return undefined; continue; } + if (argument === '--explain-warnings') { + if (command !== 'check' || explainWarnings) return undefined; + explainWarnings = true; + continue; + } if (argument === '--watch') { if (command !== 'check' || watch) return undefined; watch = true; @@ -830,7 +837,7 @@ function parseArgs(args: readonly string[]): ParsedArgs | undefined { if (command === 'run' && input !== undefined && !isAuthoredFlowPath(positionals[0]!)) return undefined; return command === 'check' - ? { command, json, watch, value: positionals[0]! } + ? { command, json, watch, explainWarnings, value: positionals[0]! } : command === 'run' ? { command, ...(detach ? { detach } : {}), bucket, reuseFromRunId, localAgent, agentCapacity, dataDir, input, json, spawn, noObserverLink, cloudMirror, allowHumanInfluenced, value: positionals[0]! } : { command, ...(detach ? { detach } : {}), localAgent, agentCapacity, dataDir, json, spawn, noObserverLink, cloudMirror, allowHumanInfluenced, value: positionals[0]! }; @@ -1094,14 +1101,18 @@ function modelProvenance(source: CliModelSource | undefined): string { return source === undefined ? '' : ` from ${MODEL_PROVENANCE[source]}`; } -function emitCheckReport(report: CheckReport, json: boolean, io: CliIo): void { +function emitCheckReport(report: CheckReport, json: boolean, io: CliIo, explainWarnings = false): void { + const foldOptions: FoldOptions = { + fold: true, explain: explainWarnings, + ...(report.gates.length > 0 ? { totalSteps: report.gates.length } : {}), + }; if (json) { - emitDiagnostics(report.diagnostics, io); + emitDiagnostics(report.diagnostics, io, foldOptions); io.stdout(JSON.stringify(report)); return; } const deferred = report.diagnostics.filter(isWorkerSurfaceWarning); - emitDiagnostics(report.diagnostics.filter((diagnostic) => !isWorkerSurfaceWarning(diagnostic)), io); + emitDiagnostics(report.diagnostics.filter((diagnostic) => !isWorkerSurfaceWarning(diagnostic)), io, foldOptions); for (const gate of report.gates) { // A gate that accepts every output is legal, but it must not read like a // gate that judges something. @@ -1147,7 +1158,7 @@ function emitCheckReport(report: CheckReport, json: boolean, io: CliIo): void { // the hosted verbs check the same list against Cloud before submitting. const requires = report.requirements === undefined ? '' : describeFlowRequirements(report.requirements); if (requires) io.stdout(`REQUIRES ${requires}`); - emitDiagnostics(deferred, io); + emitDiagnostics(deferred, io, foldOptions); if (report.ok) io.stdout(`CHECK PASSED ${report.path ?? ''}`.trimEnd()); } @@ -1211,8 +1222,9 @@ function emitRunReport( function emitDiagnostics( diagnostics: CheckReport['diagnostics'] | RunReport['diagnostics'], io: CliIo, + options: FoldOptions = {}, ): void { - for (const diagnostic of diagnostics) { + for (const diagnostic of foldUnprovableEffects(diagnostics, options)) { io.stderr(`${diagnosticLabel(diagnostic.severity)} [${diagnostic.kind}] ${diagnostic.message}`); } } diff --git a/packages/sdk/src/cli/diagnostic-fold.ts b/packages/sdk/src/cli/diagnostic-fold.ts new file mode 100644 index 000000000..d27de6614 --- /dev/null +++ b/packages/sdk/src/cli/diagnostic-fold.ts @@ -0,0 +1,28 @@ +export interface FoldOptions { + /** Collapse repeated warnings only when the caller opts in. */ + fold?: boolean; + explain?: boolean; + /** Every step in the spec, including llm and agent steps. */ + totalSteps?: number; +} + +/** Presentation only: retain the original diagnostics for machine consumers. */ +export function foldUnprovableEffects(diagnostics: readonly D[], options: FoldOptions = {}): readonly (D | { + severity: 'warning'; kind: 'unprovable_effects'; message: string; +})[] { + if (options.fold !== true || options.explain) return diagnostics; + const warnings = diagnostics.filter(d => d.kind === 'unprovable_effects'); + const count = new Set(warnings.flatMap(d => d.stepId === undefined ? [] : [d.stepId])).size + + warnings.filter(d => d.stepId === undefined).length; + if (count < 2) return diagnostics; + const denominator = options.totalSteps === undefined ? '' : ` of ${options.totalSteps}`; + return [ + ...diagnostics.filter(d => d.kind !== 'unprovable_effects'), + { + severity: 'warning', kind: 'unprovable_effects', + message: `${count}${denominator} steps have effects that cannot be proven before execution (rerun with --explain-warnings to list them).`, + }, + ]; +} diff --git a/packages/sdk/tests/check-warning-summary.test.ts b/packages/sdk/tests/check-warning-summary.test.ts new file mode 100644 index 000000000..42ed291a2 --- /dev/null +++ b/packages/sdk/tests/check-warning-summary.test.ts @@ -0,0 +1,100 @@ +import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { runCli } from '../src/cli.js'; +import { checkFlow } from '../src/cli/check.js'; +import { foldUnprovableEffects } from '../src/cli/diagnostic-fold.js'; + +const fixture = fileURLToPath(new URL('../../../testdata/preflight/unprovable-many.flow.yaml', import.meta.url)); +const warning = (stepId?: string) => ({ severity: 'warning', kind: 'unprovable_effects', message: `effects ${stepId}`, ...(stepId === undefined ? {} : { stepId }) }); +const three = [warning('a'), warning('b'), warning('c')]; +const summary = 'WARNING [unprovable_effects] 3 of 4 steps have effects that cannot be proven before execution (rerun with --explain-warnings to list them).'; +const unresolved = 'WARNING [command_unresolved] Step "suspicious" command "definitely-not-a-real-binary" does not resolve as an executable; it runs only if the shell supplies it.'; +const details = [ + 'WARNING [unprovable_effects] Step "first" command "printf" resolves, but its effects cannot be proven before execution.', + 'WARNING [unprovable_effects] Step "second" command "printf" resolves, but its effects cannot be proven before execution.', + 'WARNING [unprovable_effects] Step "conditional" starts with the shell reserved word "if", whose effects cannot be proven before execution.', +]; +async function invoke(args: string[]) { + const stdout: string[] = [], stderr: string[] = []; + const code = await runCli(args, { stdout: line => stdout.push(line), stderr: line => stderr.push(line) }); + return { stdout, stderr, code }; +} + +describe('warning folding', () => { + it('is opt-in and explain restores the original array', () => { + expect(foldUnprovableEffects(three)).toBe(three); + expect(foldUnprovableEffects(three, {})).toBe(three); + expect(foldUnprovableEffects(three, { fold: true, explain: true })).toBe(three); + }); + it('keeps zero, single, and duplicate-step warnings verbatim', () => { + for (const input of [[], [warning('a')], [warning('a'), warning('a')]]) { + expect(foldUnprovableEffects(input, { fold: true })).toBe(input); + } + }); + it('counts distinct steps and each warning without a step, without mutating input', () => { + const other = { severity: 'warning', kind: 'command_unresolved', message: 'missing' }; + const input = Object.freeze([warning('a'), other, warning('a'), warning(), warning()]); + expect(foldUnprovableEffects(input, { fold: true, totalSteps: 58 })).toEqual([ + other, { severity: 'warning', kind: 'unprovable_effects', message: '3 of 58 steps have effects that cannot be proven before execution (rerun with --explain-warnings to list them).' }, + ]); + expect(foldUnprovableEffects(three, { fold: true })[0]?.message).toBe('3 steps have effects that cannot be proven before execution (rerun with --explain-warnings to list them).'); + }); +}); + +describe('flows check warning presentation', () => { + it('puts the suspicious command ahead of one summary', async () => { + const result = await invoke(['check', fixture]); + expect(result.code).toBe(0); + expect(result.stderr).toEqual([unresolved, summary]); + }); + it('explains each step in original order', async () => { + const result = await invoke(['check', '--explain-warnings', fixture]); + expect(result.code).toBe(0); + expect(result.stderr).toEqual([...details, unresolved]); + }); + it('keeps JSON byte-identical to the original report, with either text mode', async () => { + const expected = JSON.stringify(checkFlow(fixture, { warnUnresolvedAgentWorker: true }).report); + for (const flags of [[], ['--explain-warnings']]) { + const result = await invoke(['check', '--json', ...flags, fixture]); + expect(result.code).toBe(0); + expect(result.stdout).toEqual([expected]); + expect(result.stderr).toEqual(flags.length ? [...details, unresolved] : [unresolved, summary]); + expect(JSON.parse(result.stdout[0]!).diagnostics.filter((d: { kind: string }) => d.kind === 'unprovable_effects')).toHaveLength(3); + } + }); + it.each([ + ['run', '--explain-warnings', fixture], + ['resume', '--explain-warnings', 'run-id'], + ['check', '--explain-warnings', '--explain-warnings', fixture], + ])('refuses invalid flag use: %j', async (...args) => { + const result = await invoke(args); + expect(result.code).toBe(2); + expect(result.stderr[0]).toContain('REFUSED [invalid_invocation]'); + }); + it('retains every run warning through the run renderer without a daemon', async () => { + const directory = mkdtempSync(join(tmpdir(), 'flows-warning-run-')); + try { + const result = await invoke(['run', '--no-spawn', '--no-observer-link', '--data-dir', directory, fixture]); + expect(result.code).toBe(2); + expect(result.stderr.filter(line => line.startsWith('WARNING [unprovable_effects]'))).toEqual(details); + expect(result.stderr.some(line => line.includes('daemon_unreachable'))).toBe(true); + } finally { rmSync(directory, { recursive: true, force: true }); } + }); + it.each([false, true])('forwards explain=%s to the watch child', async (explain) => { + const entry = fileURLToPath(new URL('../dist/cli.js', import.meta.url)); + const child = spawn(process.execPath, [entry, 'check', '--watch', ...(explain ? ['--explain-warnings'] : []), fixture]); + const closed = once(child, 'close'); + let stdout = '', stderr = ''; + child.stdout.on('data', chunk => { stdout += chunk; }); + child.stderr.on('data', chunk => { stderr += chunk; }); + try { + await expect.poll(() => stdout.includes('CHECK PASSED'), { timeout: 5000 }).toBe(true); + } finally { child.kill('SIGINT'); await closed; } + expect(stderr.trim().split('\n')).toEqual(explain ? [...details, unresolved] : [unresolved, summary]); + }); +}); diff --git a/packages/sdk/tests/relay-cli-surface.test.ts b/packages/sdk/tests/relay-cli-surface.test.ts index d0b3ce60d..9eefa8ac1 100644 --- a/packages/sdk/tests/relay-cli-surface.test.ts +++ b/packages/sdk/tests/relay-cli-surface.test.ts @@ -77,6 +77,7 @@ const INVOCATIONS: readonly { verb: string; argv: readonly string[]; variant: Pa { verb: 'build', argv: ['build', '--verify', '--json', BUNDLE_DIR], variant: 'build' }, { verb: 'build', argv: ['build', BUNDLE_DIR, '--verify'], variant: 'build' }, { verb: 'check', argv: ['check', 'flow.yaml'], variant: 'check' }, + { verb: 'check', argv: ['check', '--explain-warnings', 'flow.yaml'], variant: 'check' }, { verb: 'check', argv: ['check', '--watch', '--json', 'flow.yaml'], variant: 'check' }, // Both `deploy` forms: the positional decides which variant the verb produces. { verb: 'deploy', argv: ['deploy', DIGEST, '--to', 'file:///tmp/bucket'], variant: 'deploy' }, diff --git a/testdata/preflight/unprovable-many.flow.yaml b/testdata/preflight/unprovable-many.flow.yaml new file mode 100644 index 000000000..76b20ed0c --- /dev/null +++ b/testdata/preflight/unprovable-many.flow.yaml @@ -0,0 +1,16 @@ +# yaml-language-server: $schema=https://schema.relayflows.dev/v0.1/flows.schema.json +version: '0.1.0' +name: unprovable-many +steps: + - id: first + type: deterministic + command: "printf first" + - id: second + type: deterministic + command: "printf second" + - id: conditional + type: deterministic + command: "if [ -f x ]; then printf found; fi" + - id: suspicious + type: deterministic + command: definitely-not-a-real-binary