From d893e4bfa9cf9d391112da2dcd334f3c07518131 Mon Sep 17 00:00:00 2001 From: xizhuomengcontin Date: Tue, 15 Sep 2026 17:21:22 +0800 Subject: [PATCH] fix(cli): let push take the --fs the help offers and the code reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pushCommand` reads `args.bool('fs')` and `orca push --help` lists `--fs`, but push's own entry in BY_COMMAND listed only `gateway` and `force`, so `assertKnownFlags` refused the flag before the implementation ever saw it. It did not degrade to ignoring the flag. `orca push --fs` failed outright, and the error named only the two flags that were listed, so there was no way to ship the workspace snapshots at all. Every run on a gateway therefore came back with `blob_count: 0`, and forking a pulled run died inside git: git read-tree failed (128): fatal: failed to unpack tree object f7aa6aa5… which is the console's documented "fork it locally" path — `orca pull ` then `orca compare last --from N`. The union check in flags.test.ts could not see it: it flattens every command's flags into one set, and `record` and `replay` both allow `fs`. `sync.test.ts` could not see it either — it calls `pushCommand` with args it builds itself, which never passes through the gate. So the new test holds the allowlist to the help text per command, which is the contract the user actually reads. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/flags.ts | 10 ++++++- packages/cli/test/flags.test.ts | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/flags.ts b/packages/cli/src/flags.ts index 93a61427..d3dac28f 100644 --- a/packages/cli/src/flags.ts +++ b/packages/cli/src/flags.ts @@ -31,7 +31,15 @@ export const BY_COMMAND: Record = { quickstart: ['dir', 'full'], // push/pull reach a gateway the user names. `--force` means two different deliberate overrides: // on push, store despite a secret-scan finding; on pull, replace a local run that already exists. - push: ['gateway', 'force'], + // + // `--fs` belongs here because `pushCommand` reads it (`args.bool('fs')`) and the help text + // offers it. Leaving it out did not make push ignore the flag — it made push REFUSE it, before + // the implementation ever saw it, and the refusal named only the two flags that were listed. So + // the workspace snapshots could not be shipped by any invocation, every run on the gateway came + // back with `blob_count: 0`, and forking a pulled run died in `git read-tree` on a tree object + // that had never been sent. This list is meant to be counted from the `args.*` call sites; that + // one was missed. + push: ['gateway', 'force', 'fs'], pull: ['gateway', 'force'], record: ['fs', 'shell', 'agent-spans', 'mcp-config', ...RETRIEVAL, ...TLS, ...UPSTREAM], attach: [ diff --git a/packages/cli/test/flags.test.ts b/packages/cli/test/flags.test.ts index 8bfa67ad..f9a83e93 100644 --- a/packages/cli/test/flags.test.ts +++ b/packages/cli/test/flags.test.ts @@ -106,6 +106,52 @@ describe('the allowlist against the source it mirrors', () => { expect(unreachable).toEqual([]); }); + /** + * The one above flattens every command's flags into one set and asks whether SOMEONE allows the + * name. That is too weak by exactly one level, and the gap shipped: `push` reads `args.bool('fs')` + * and the help text offers `--fs`, but `push`'s own entry listed only `gateway` and `force`. + * Because `record` and `replay` both allow `fs`, the union check was satisfied and said nothing. + * + * What the user got was not a flag quietly ignored — it was `orca push --fs` REFUSED, with an + * error naming only the two flags that were listed. The snapshots could therefore not be shipped + * by any invocation, so every run on a gateway carried `blob_count: 0`, and forking a pulled run + * died inside `git read-tree` on a tree object that had never left the machine. + * + * The help text is the contract the user reads, so hold the allowlist to it per command rather + * than in aggregate. `sync.test.ts` could not catch this: it calls `pushCommand` with args it + * built itself, which never passes through the gate this file guards. + */ + it('accepts, for each command, every flag the help text offers it', () => { + const help = readFileSync(join(SRC, 'main.ts'), 'utf8').split(/\r?\n/); + const documented = new Map>(); + let command: string | undefined; + for (const line of help) { + // ` orca push [run] send a run to the gateway` — two spaces, then the command. + const heading = line.match(/^ {2}orca ([a-z-]+)\b(.*)$/); + if (heading) { + command = heading[1]!; + const flags = documented.get(command) ?? new Set(); + // A usage line can carry a flag itself: `orca replay [run] --from N`. + for (const m of heading[2]!.matchAll(/--([a-z][a-z0-9-]*)/g)) flags.add(m[1]!); + documented.set(command, flags); + continue; + } + // ` --fs include the workspace snapshots` — eight spaces, under the last heading. + const flag = command === undefined ? null : line.match(/^ {8}--([a-z][a-z0-9-]*)/); + if (flag) documented.get(command!)!.add(flag[1]!); + } + // The parse itself has to keep working, or the check passes by finding nothing. + expect(documented.get('push'), 'the help block stopped parsing').toBeDefined(); + expect([...documented.get('push')!].sort()).toContain('fs'); + + const refused: string[] = []; + for (const [cmd, flags] of documented) { + const allowed = new Set([...(BY_COMMAND[cmd] ?? []), ...GLOBAL]); + for (const name of flags) if (!allowed.has(name)) refused.push(`orca ${cmd} --${name}`); + } + expect(refused.sort()).toEqual([]); + }); + /** * A flag the code reads as a boolean has to be declared valueless, or the parser hands it the * next word and the positional it belonged to disappears.