Skip to content

feat(cli): add orca supervisor print and orca supervisor doctor - #17853

Open
klaidliadon wants to merge 6 commits into
stablyai:mainfrom
klaidliadon:feat/orca-supervisor-command
Open

klaidliadon wants to merge 6 commits into
stablyai:mainfrom
klaidliadon:feat/orca-supervisor-command

Conversation

@klaidliadon

Copy link
Copy Markdown

ELI5

orcad can already generate and audit its own service definition. This adds the
same two commands to the orca CLI, so you can ask the question without first
finding the orcad bundle on disk.

What Changed

  • orca supervisor print — generates a supervisor service definition.
  • orca supervisor doctor — audits an installed one.

Both are thin front doors over the generator and audit orcad --print-service /
--doctor already expose. New src/cli/handlers/supervisor.ts and
src/cli/specs/supervisor.ts, registered in the handler-group manifest and the
spec index.

Why

Three deliberate choices worth flagging:

1. orca supervisor print requires --orcad <path>, and orcad's own flag does not

Under the CLI, process.argv[1] is the CLI's entry — not orcad.js. Inferring it
produced a unit pinning ExecStart=<node> .../cli/index.js, which is a service
that cannot start.

So the flag is required rather than guessed, and the spec note says why at the
point someone reads the usage:

--orcad is required here: this process is the orca CLI, so it cannot name which
orcad.js the service should run.

A wrong ExecStart fails at service-start time, far from the command that wrote
it, which is exactly the kind of error worth refusing up front.

2. Named supervisor, not daemon or service

daemon already means the terminal daemon everywhere in this codebase. service
is two characters from serve — and one starts a server while the other writes
system files, which is the kind of neighbouring pair typo recovery must never
steer between. supervisor is the word docs/reference/orcad-operations.md
already uses.

3. Registered local-only

Via shouldIgnoreRemoteSelection. These inspect this machine's service files,
which is normally not the host a paired CLI targets — remote selection would
silently answer about the wrong machine.

Build wiring

orcad/orcad-service-command gets an electron.vite.config.ts entry because the
CLI now imports that path. Without one, the build that runs after build:cli
cleans it out of out/main and the command dies at require time.
main-module-bundle-parity.test.ts exists to catch exactly this.

The modules added to tsconfig.cli.json are all leaves — node builtins,
src/shared and each other — so the daemon itself stays out of the CLI's import
graph. That is the whole reason the path consolidation in the base PR was worth
doing.

Linked Issue

Refs #15956 — same rationale as the generator PR it builds on: this gives
operators a correct unit and an audit, rather than restructuring the daemon's
lifetime.

Visual Proof

N/A — CLI commands, terminal output only.

Testing

handler-group-manifest.test.ts verifies each group's keys against its real
exports, so a drifted registration fails CI rather than dispatch — the new
supervisor group is covered by it automatically.

main-module-bundle-parity.test.ts covers the bundle entry.

Field-tested on a Synology NAS: orca supervisor print --orcad <path> produces a
unit whose ExecStart names the real orcad bundle, and orca supervisor doctor
reports the same findings as orcad --doctor against the same installed unit.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Not run by me in this environment: pnpm lint, pnpm typecheck, pnpm test
and pnpm build — this host cannot install the full toolchain. Reported clean from
a separate run; CI is the authority.

AI Disclosure

Claude Opus 5 wrote the commits and this description. I reviewed them and ran the
NAS verification.

Review

  • Cross-platform: delegates to the same generator, so systemd on Linux and
    launchd on macOS behave as they do under orcad. No new platform branch.
  • SSH/remote/local: explicitly local-only, and the spec notes say so in the
    user-facing text so the scope is not a surprise. This is the main compatibility
    consideration in the PR and it is handled deliberately rather than by default.
  • Agents/integrations: none.
  • Performance: lazy-loaded handler group, consistent with every other group —
    nothing added to CLI startup.
  • UI: none.
  • Security: print writes to stdout only; the operator places the file. No
    elevated privilege is taken or implied. doctor reads service files and reports
    findings.
  • Backwards compatibility: purely additive. New command path, new spec entries;
    no existing command's behaviour changes.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Top of a three-PR stack. Depends on #17852 (feat/orcad-print-service-and-doctor),
which depends on #17851 (fix/daemon-socket-path-budget). Only the CLI-layer commit is new
here; please land the two beneath it first.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

klaidliadon and others added 2 commits September 1, 2026 00:39
A data root long enough to push the daemon's endpoint past `sun_path` killed the
daemon at startup — but orcad's own listener name is about 5 bytes shorter, so
there is a band where orcad came up, reported ready, and served clients with
terminal survival silently off. Every terminal then died on the next restart,
which is the one thing forking a detached daemon exists to prevent. Found on
DSM: a root of 86 characters reproduced it, 84 was fine.

orcad now refuses at startup with `ORCAD_EXIT_CONFIGURATION` (78). This is not
one of the unenumerable runtime reasons `startOrcadDaemon` fail-opens for: it is
known before anything is attempted, permanent until an operator changes a value
they supplied, and pure arithmetic. That puts it with the bind address and the
instance lock, which already refuse, and `RestartPreventExitStatus=78` keeps a
supervisor from restart-spinning on it. The check runs ahead of the instance
lock deliberately — it reads nothing and creates nothing, so refusing a root
should not first take a lock on it.

**The limit is verified against the kernel rather than asserted**, because past
it there is no single failure to detect: on Linux a 109-110 byte path fails
EADDRINUSE against a free name, and 111+ reports a successful `listen` with no
directory entry at all — which looks healthy to everything except the dev+ino
reads daemon endpoint ownership is built on. The test binds at the limit and one
byte past it and asserts a stat-able entry, so a wrong constant fails it.

Supporting changes, all in service of that:

- **Consolidate the daemon's endpoint path builders into one leaf module**
  (`daemon-runtime-paths.ts`), so the budget measures the real names.
  `daemon-spawner` re-exports `getDaemonSocketPath` / `getDaemonTokenPath` /
  `getDaemonPidPath` from it rather than moving them, so every existing caller
  imports the same name from the same place and only the definition moved.
  `daemon-endpoint-ownership` likewise re-exports `getDaemonSocketBindPath`.
- **Measure bytes, not characters.** A non-ASCII root spends more of the budget
  than its length suggests. `ssh-control-socket` now shares the constant and the
  byte measurement instead of keeping its own copy of the 104/108 split.

Boundary verified end to end against the built orcad: 85 starts with a live
daemon socket, 86 exits 78.

Refs stablyai#10726

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
Two flags for the supervised deployment orcad exists to serve:

- `orcad --print-service` generates a systemd unit (and a launchd plist) for a
  given data root, bind and port.
- `orcad --doctor` audits an installed one, then probes the live host — the unit
  as written, the daemon socket, the data root — and reports what it actually
  measured rather than what it expected.

**`KillMode=process`, not `mixed`.** Measured on systemd 219: under `mixed` a
`systemctl restart` destroyed the daemon, its shell, the session and its
scrollback; the identical unit under `process` kept all four. `mixed` SIGTERMs
the main process and then SIGKILLs everything still in the control group, and
`setsid()` escapes the process group, not the cgroup — so the detached daemon
dies with it. The generator emits `process` and the doctor reports `mixed` as
critical, including in units orcad itself generated before this was settled.

**systemd values are quoted, and the readers parse quotes.** Both halves matter
together: a data root containing a space would otherwise round-trip into a unit
that silently pins a different root, and an audit that could not parse its own
output would call every quoted unit malformed.

**An unreadable service definition is reported as unreadable, not absent.**
Those are different operator actions — one is a permissions fix, the other an
install — and collapsing them sent people to reinstall a unit that was already
there.

The doctor also answers whether the data root leaves room for the daemon's
socket, reusing the budget from the preceding commit. It is critical when a unit
file pins the root and a warning — naming whose root it measured — when it falls
back to the invoking shell's, because those are different claims.

`ORCAD_ENTRY_FILENAME` is named once in `orcad-artifacts` because a generated
unit execs it. `OrcadArtifact.optional` goes with it: nothing set it, the filter
was a no-op, and the one artifact that would want it cannot be an entry there at
all since its filename is derived per platform-arch.

Field-tested on a Synology NAS (DSM, systemd 219) and on a root runner; several
commits' worth of defects found that way are folded in.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Greptile Summary

This stack adds service-definition generation and auditing for orcad, exposes both operations through local-only orca supervisor commands, and consolidates daemon socket-path calculations.

  • Adds supervisor print and supervisor doctor CLI specs, handlers, registration, and build entries.
  • Adds systemd/launchd rendering, discovery, static auditing, live host probes, and operator diagnostics.
  • Adds daemon socket-path budgeting and shared endpoint-path helpers.

Confidence Score: 2/5

The PR should not merge until launchd output is valid and the generation and explicit-path audit workflows behave as advertised.

macOS generation always emits malformed XML, sudo-based generation selects an account the renderer rejects, and an explicit service path is mixed with conventional definitions, producing misleading findings and disabling live checks.

Files Needing Attention: src/shared/supervisor-service-render.ts; src/main/orcad/orcad-service-command.ts

Important Files Changed

Filename Overview
src/shared/supervisor-service-render.ts Adds systemd and launchd generation, but the launchd output contains an XML-invalid comment.
src/main/orcad/orcad-service-command.ts Coordinates generation and auditing, but root generation defaults to a rejected account and explicit audit paths are conflated with conventional definitions.
src/cli/handlers/supervisor.ts Adds thin local CLI adapters with text and JSON output handling; no independent defect was identified.
src/shared/supervisor-service-audit.ts Adds static checks for lifecycle, data-root, account, duplicate-service, and executable invariants.
src/main/daemon/daemon-runtime-paths.ts Consolidates daemon endpoint naming and adds Unix socket-path budget calculations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  CLI[orca supervisor print/doctor] --> CMD[orcad-service-command]
  CMD --> RENDER[Service renderer]
  CMD --> DISCOVERY[Installed-service discovery]
  DISCOVERY --> AUDIT[Static service audit]
  CMD --> PROBES[Host and supervisor probes]
  AUDIT --> OUTPUT[Text or JSON findings]
  PROBES --> OUTPUT
Loading

Reviews (1): Last reviewed commit: "feat(cli): add `orca supervisor print` a..." | Re-trigger Greptile

Comment on lines +199 to +207
const scopeNote =
config.scope === 'system'
? ' LaunchDaemon: runs at boot, independent of any login session.'
: ' LaunchAgent: runs ONLY while this user is logged in. A headless Mac needs\n --scope system instead.'
return `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<!-- Generated by orcad. Regenerate rather than hand-edit:
orcad --print-service --scope ${config.scope}
${scopeNote} -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Launchd output contains invalid comments

When either print command generates a launchd service, the XML comment contains the forbidden -- sequence in option names, producing a malformed plist that strict plist validation and launchd can reject.

Suggested change
const scopeNote =
config.scope === 'system'
? ' LaunchDaemon: runs at boot, independent of any login session.'
: ' LaunchAgent: runs ONLY while this user is logged in. A headless Mac needs\n --scope system instead.'
return `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<!-- Generated by orcad. Regenerate rather than hand-edit:
orcad --print-service --scope ${config.scope}
${scopeNote} -->
const scopeNote =
config.scope === 'system'
? ' LaunchDaemon: runs at boot, independent of any login session.'
: ' LaunchAgent: runs ONLY while this user is logged in. A headless Mac needs\n system scope instead.'
return `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<!-- Generated by orcad. Regenerate rather than hand-edit.
Selected scope: ${config.scope}.
${scopeNote} -->

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and all three were already fixed one PR down the stack — this branch was built on #17852's earlier head and had not picked them up.

Rebased onto #17852's current head (d64dc49). The plist comment now names flags without their leading dashes, with a comment saying why XML forbids -- in a comment body; root generation under sudo is handled separately from refusing to run as root; and --service-path now selects rather than appending to conventional discovery.

This PR's own diff is now just the CLI layer — 12 files under src/cli/ plus the two build-config entries. Sorry for the noise: the stack drifted while #17852 was absorbing review, and I should have re-synced before you looked.

Comment on lines +190 to +192
// Generating as root is normal (sudo, a container); running orcad as root is not, so
// the account is a flag rather than an inheritance.
user: options.user ?? userInfo().username,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Sudo generation selects root

When service generation runs through sudo without the optional --user flag, userInfo().username selects root, which the renderer rejects, so the normal system-service workflow throws instead of printing a definition.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and all three were already fixed one PR down the stack — this branch was built on #17852's earlier head and had not picked them up.

Rebased onto #17852's current head (d64dc49). The plist comment now names flags without their leading dashes, with a comment saying why XML forbids -- in a comment body; root generation under sudo is handled separately from refusing to run as root; and --service-path now selects rather than appending to conventional discovery.

This PR's own diff is now just the CLI layer — 12 files under src/cli/ plus the two build-config entries. Sorry for the noise: the stack drifted while #17852 was absorbing review, and I should have re-synced before you looked.

Comment on lines +264 to +267
const { files, unreadable } = collectServiceFiles(
platform,
options.servicePath ? [options.servicePath] : []
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Explicit audit path is conflated

When --service-path is used on a host with a conventional orcad definition, the requested path is appended to conventional discovery rather than selected independently, causing unrelated duplicate-service findings and disabling live probes for the requested definition.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and all three were already fixed one PR down the stack — this branch was built on #17852's earlier head and had not picked them up.

Rebased onto #17852's current head (d64dc49). The plist comment now names flags without their leading dashes, with a comment saying why XML forbids -- in a comment body; root generation under sudo is handled separately from refusing to run as root; and --service-path now selects rather than appending to conventional discovery.

This PR's own diff is now just the CLI layer — 12 files under src/cli/ plus the two build-config entries. Sorry for the noise: the stack drifted while #17852 was absorbing review, and I should have re-synced before you looked.

…agent log

Three defects the review found in the generator/reader pair, each one a case of
the two halves not agreeing on the same encoding or the same identity.

`readPlistString` returned the XML-escaped text. The generator writes `&` as
`&amp;`, so the doctor compared `/srv/a&amp;b` against the caller's `/srv/a&b`
and reported the plist it had just produced as pinning a different data root --
and stat-ed an ExecStart path that was never on disk. Decoded on one pass, so a
path literally containing `&lt;` survives as itself rather than decoding twice.
This is the plist half of the rule the systemd splitter already followed.

`assertNotRoot` compared against the name only. systemd's `User=` takes a
numeric uid too, so `--user 0` rendered exactly the unit the guard exists to
refuse, and the audit read it back as an ordinary account and reported
`Runs as 0.` as healthy. Guard and audit now share `isRootAccount`.

A LaunchAgent got `StandardOutPath=/var/log/orcad.log`. That path is root-owned
on macOS and the agent runs unprivileged, so it cannot open its own log, and
launchd reports the failure against the file rather than the job -- it reads as
a broken install for a reason the plist never states. The renderer does no I/O
and so cannot resolve a home directory, so it now emits the keys only when the
caller resolved a path; `printService` supplies `~/Library/Logs/orcad.log` for
an agent and `/var/log/orcad.log` for a daemon.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
@klaidliadon

Copy link
Copy Markdown
Author

Thanks — three useful P1s. All of them land on files this PR does not add: they come from the commit underneath it, #17852 (feat(orcad): add --print-service and --doctor). Dispositions below, each checked against the tree rather than against the summary.

1. "Launchd output contains invalid comments" — correct, and stronger than "can reject"

src/shared/supervisor-service-render.ts:205-207 opens an XML comment and then emits orcad --print-service --scope ${config.scope}; in user scope scopeNote adds --scope system instead. inside the same comment. XML forbids -- anywhere inside a comment body, so the generated plist is not well-formed for either scope. Verified with a real parser rather than by eye:

xml.parsers.expat.ExpatError: not well-formed (invalid token): line 3, column 15

Column 15 is the -- of --print-service. plutil -lint fails on the same token, and plutil is the first thing an operator runs on a generated plist.

The systemd path at line 123-124 carries the same text but as # comments, where -- is harmless — which is why this only shows up on darwin.

Worth pairing the fix with a test: supervisor-service-render.test.ts asserts AbandonProcessGroup, entity escaping and the logout warning, but never parses its own output, so nothing there fails today.

2. "Sudo generation selects root" — declining

The finding assumes system-scope generation normally runs under sudo. It does not. orca supervisor print writes to stdout and changes nothing on disk, and supervisorInstallHint puts sudo only on launchctl bootstrap and systemctl daemon-reload|enable|start — never on generation itself. The documented flow is orca supervisor print --scope system --user orca redirected (or piped to sudo tee), which is also this PR's own spec example.

When it is run under sudo, the throw is the intended outcome, not a failure to handle one. assertNotRoot refuses with "Pass the account orcad should run as." The alternative is rendering UserName=root, which makes orcad create a root-owned data root and every later user-scope run fail with orcad_data_root_wrong_owner — a state orcad deliberately will not repair. Inferring SUDO_USER instead would silently pick an account nobody named, in a file whose entire purpose is to pin one. A loud refusal that names the flag is the correct behaviour here.

3. "Explicit audit path is conflated" — correct, and there is a sharper repro

collectServiceFiles appends --service-path to candidateServicePaths(platform) rather than replacing them (src/main/orcad/supervisor-service-discovery.ts:76-79). The consequence is worse than the ambiguity described: point --service-path at the definition conventional discovery already finds, and the same file is collected twice. files.length === 2, so auditDuplicates sees two paths sharing one data root and emits multiple_services_one_root at severity critical with exit 1 — while probing goes false and every live check is silently skipped. A false critical produced entirely by naming the file you wanted audited.

The natural shape is for an explicit path to select the definition rather than extend the candidate list.


Fixes for 1 and 3 belong in #17852, not here — landing them on top of the stack would leave #17852 shipping the bug if it merges first. Raising them there. No change to this PR's own layer, which the review also read as clean.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

supervisor is registered in the handler manifest, the spec index, and shouldIgnoreRemoteSelection, but not in isCommandGroup — so orca supervisor --help (and orca help supervisor) prints Unknown command: supervisor plus the full root help and exits 1, instead of listing the two subcommands.

Reviewed changes

This PR adds orca supervisor print and orca supervisor doctor as a local-only CLI front door over the orcad --print-service / --doctor generator and audit added by the dependency PRs beneath it. The review concentrates on the CLI-layer commit (02fe8fe), the only commit new to this PR.

  • Handlers — src/cli/handlers/supervisor.ts maps CLI flags back onto the orcad-side argv, translates SupervisorServiceUnsupportedError into a RuntimeClientError, and wires --json refusal plus exit codes.
  • Specs — src/cli/specs/supervisor.ts documents both commands, their flags, and the local-only scope.
  • Registration — the group is added to handler-group-manifest.ts, specs/index.ts, and shouldIgnoreRemoteSelection in index.ts.
  • Build wiring — electron.vite.config.ts gains an orcad/orcad-service-command entry, and tsconfig.cli.json lists the leaf modules the CLI now imports so build:cli doesn't strip them from out/main.

⚠️ supervisor is missing from isCommandGroup

orca supervisor --help and orca help supervisor resolve to the group path, then fall through printHelp because isCommandGroup does not list supervisor: the operator gets Unknown command: supervisor plus the entire root help, and the process exits 1. Every other command group — including vm, the closest analog — is registered there, so this is an omission, not a policy.

Technical details
# Register `supervisor` in isCommandGroup

## Affected sites
- src/cli/args.ts:201 — the `isCommandGroup` array lacks `'supervisor'`. This file is not in the PR diff, hence the body section rather than an inline comment.

## Required outcome
- `orca supervisor --help` and `orca help supervisor` print the `formatGroupHelp` output for `supervisor` (both subcommands) and exit 0.

## Suggested approach
- Add `'supervisor'` to the `isCommandGroup` array in `src/cli/args.ts`, alongside `'vm'`.

ℹ️ Nitpicks

  • no-probe is not registered in BOOLEAN_FLAGS (src/cli/args.ts), unlike every other --no-* boolean flag. It parses correctly today only because it is always last or followed by another flag; add it for consistency.
  • The hand-curated root help (root-help-text-primary.ts) does not mention supervisor print or supervisor doctor, so orca with no arguments omits the new commands.
  • No unit test exercises src/cli/handlers/supervisor.ts itself — the flag→argv mapping, the --json refusal, and the exit-code wiring are only covered indirectly by the manifest and bundle-parity guards.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

…essing a log path

Every generated plist was malformed XML, in both scopes. The header comment
carried the regeneration command verbatim, and XML forbids the string `--`
anywhere inside a comment body -- so `--print-service` made the document
unparseable. expat rejects it, `plutil -lint` rejects it, and launchctl will not
load the job: the entire darwin path was dead on arrival. The flags are now
named rather than written, with the constraint stated in the file so it is not
"fixed" back. renderSystemd carries the same sentence safely because `#`
comments have no such rule, which is why this was darwin-only.

Nothing caught it because no test parsed the output it asserted against. There
is now a well-formedness assertion over both scopes, plus one proving a `--` in
a generated path cannot reach a comment.

`--service-path` extended conventional discovery instead of selecting. Naming
the definition discovery already finds -- the obvious way to ask for a report on
your own install -- collected that file twice, so `auditDuplicates` reported
`multiple_services_one_root` as CRITICAL with exit 1 while every live probe was
skipped as ambiguous. A false critical produced by naming the file you wanted
audited. An explicit path now selects.

The launchd log path guessed an account's home. `UserName` honours `--user`
while the path used the generating user's home, and `/var/log` is root-owned
while the job runs unprivileged -- so both scopes could emit a log the job
cannot open, which launchd reports against the path rather than the job. No path
is defaulted now; generation warns and says what to add, since none can be shown
writable by the run-as account from here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
@klaidliadon

klaidliadon commented Sep 1, 2026 •

Copy link
Copy Markdown
Author

Good catch on isCommandGroup — that one was real and is fixed in c85d0bb.

supervisor missing from isCommandGroup — fixed

Confirmed exactly as described: orca supervisor --help and orca help supervisor reach printHelp, miss both findCommandSpec and isCommandGroup, print Unknown command: supervisor followed by the whole root help, and src/cli/index.ts:78-83 then sets exit 1. Every subcommand's own --help kept working, which is why nothing in the specs revealed it.

Worth adding: supervisor was the only multi-segment command prefix in the CLI missing from that list, and the list has no entries without a matching spec — so it is an exhaustive invariant, not a curated one. index.test.ts now asserts that directly, plus the behaviour including the exit code, since an unregistered group still prints a screenful of help and only the 1 says it went wrong.

Root help — fixed

Agreed, and for the reason the command exists: a front door added so an operator does not have to find the orcad bundle first is not much of a front door if orca --help never mentions it. vm recipe doctor is the precedent for a narrow operational command earning its own heading. Summaries trimmed to that file's width, as vm recipe doctor's already is.

No handler test — fixed

src/cli/handlers/supervisor.test.ts covers the argv rebuild (pinning that flag order is the handler's, not the caller's Map insertion order), the --json refusal on print, the audit exit code out of both output paths, the unsupported-platform translation, and that every other failure keeps its own message — the missing---orcad refusal is a plain Error whose text is the only thing naming the flag, so translating it would lose that.

no-probe in BOOLEAN_FLAGS — declining

The premise doesn't hold. BOOLEAN_FLAGS is not a registry of every boolean flag, and --no-* is not the pattern that decides membership — three of the four other --no-* flags in the CLI are absent from it:

flag in BOOLEAN_FLAGS
no-pairing yes
no-screenshot (10 computer specs) no
no-parent (worktree create, worktree set) no
no-ua-spoof (tab profile create) no

What the set is actually for is switches whose following token could be read as a value — the !hasNext || next.startsWith('--') fallback in parseArgs handles everything else. supervisor doctor takes no positionals, so --no-probe can only ever be followed by another flag or by nothing, which is why it already parses correctly in every shape.

You're right that this is invisible from the set, though, so the answer is a test rather than a set entry: args.test.ts now pins --no-probe parsing as a switch bare, before --json, and before --service-path <path>. Adding it to the set instead would assert a convention the tree does not actually follow.


Separately, the three Greptile P1s on this thread all landed on src/shared/supervisor-service-render.ts and src/main/orcad/orcad-service-command.ts, which come from #17852 underneath this PR — dispositions and repros in this comment, raised on #17852 rather than fixed here so that PR does not merge with the bugs still in it.

Gates: pnpm lint, typecheck, test and build were not run on this change, and to be clear about what that means — CI has not run them either. PR Checks, PR test LoC and Computer-use e2e are all sitting at action_required on both commits, awaiting a maintainer's approval for a fork PR; the only workflows that have executed are Track Community PRs, Greptile and pullfrog. So nothing has type-checked or run the three test files added here. Approving those runs is the check.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Since the prior pullfrog review (02fe8fe), commit c85d0bb landed to fix the group-registration gap and add the missing coverage.

  • Registered supervisor in isCommandGroup — src/cli/args.ts now lists supervisor alongside vm, so orca supervisor --help / orca help supervisor resolve to the group help and exit 0 instead of printing Unknown command: supervisor.
  • Added a regression guard — src/cli/index.test.ts asserts every multi-segment command prefix in COMMAND_SPECS is registered as a command group, so a future group can't ship with the same omission.
  • Added root-help entries — src/cli/root-help-text-primary.ts lists supervisor print / supervisor doctor under a new Supervisor: heading.
  • Added handler tests — src/cli/handlers/supervisor.test.ts pins the flag→argv rebuild, the bare --no-probe emission, the --json refusal, and the exit-code/host-name wiring.
  • Pinned the --no-probe decision — src/cli/args.test.ts documents why no-probe stays out of BOOLEAN_FLAGS and locks the parsing behavior that the reasoning relies on.

The prior [!IMPORTANT] finding and all three nitpicks are addressed. Local run of args.test.ts, handlers/supervisor.test.ts, and index.test.ts passes (72 tests).

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

klaidliadon and others added 2 commits September 1, 2026 13:45
A front door on the `orca` CLI over the generator and audit `orcad
--print-service` / `--doctor` already expose, so an operator setting up a
supervised host does not have to find the orcad bundle first to ask a question
about it.

**`orca supervisor print` requires `--orcad <path>`, and orcad's own flag does
not.** Under the CLI, `process.argv[1]` is the CLI's entry, not `orcad.js`, so
inferring it produced a unit pinning `ExecStart=<node> .../cli/index.js` — a
service that cannot start. The flag is required rather than guessed, and the
spec note says why at the point someone reads the usage.

Named `supervisor` rather than `daemon` or `service`: `daemon` already means the
terminal daemon everywhere in this codebase, and `service` is two characters
from `serve` while one starts a server and the other writes system files — the
kind of neighbouring pair typo recovery must never steer between. It is also the
word `docs/reference/orcad-operations.md` already uses.

Registered local-only via `shouldIgnoreRemoteSelection`: these inspect THIS
machine's service files, which is normally not the host a paired CLI targets, so
remote selection would silently answer about the wrong machine.

`orcad/orcad-service-command` gets an `electron.vite.config.ts` entry because the
CLI now imports that path — without one, the build that runs after `build:cli`
cleans it out of `out/main` and the command dies at require time.
`main-module-bundle-parity.test.ts` exists to catch exactly this.

The modules added to `tsconfig.cli.json` are all leaves — node builtins,
`src/shared` and each other — so the daemon itself stays out of the CLI's import
graph.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
`orca supervisor --help` and `orca help supervisor` printed `Unknown command:
supervisor`, then the entire root help, then exited 1. The group was registered
in the handler manifest, the spec index and `shouldIgnoreRemoteSelection` — but
not in `isCommandGroup`, which is what `printHelp` consults before falling
through to the unknown-command path. Each subcommand's own `--help` kept
working, so nothing about the specs revealed the gap.

`supervisor` was the only multi-segment command prefix in the CLI missing from
that list, and the list carries no entries without a matching spec — so the
registration is an exhaustive invariant rather than a curated one, and
`index.test.ts` now asserts it directly. The behaviour is pinned too, exit code
included: an unregistered group still prints a screenful of help, and only the 1
says it went wrong.

Both commands are now listed under `orca --help`. A front door added so an
operator does not have to find the orcad bundle first is not much of a front
door if the root help never mentions it; `vm recipe doctor` is the precedent for
a narrow operational command earning its own heading. Summaries are trimmed to
the width that file uses, as `vm recipe doctor`'s already is.

`supervisor.test.ts` covers the adapter itself, which had no direct test: the
argv it rebuilds for the orcad-side parser (flag order is the handler's, not the
caller's), the `--json` refusal on `print`, the audit exit code carried out of
both output paths, and the unsupported-platform translation — plus the fact that
every other failure keeps its own message, since the missing-`--orcad` refusal
is a plain Error whose text is the only thing naming the flag.

`--no-probe` is deliberately left out of `BOOLEAN_FLAGS`. That set is for
switches whose following token could be mistaken for a value; `supervisor
doctor` takes no positionals, so every shape it can be written in already parses
as a switch — and three of the four other `--no-*` flags in the CLI
(`no-screenshot`, `no-parent`, `no-ua-spoof`) are absent from it for the same
reason. `args.test.ts` pins the parse instead, because the set alone reads as
though it should be there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UacRCfD4XPe29bD3ZWNc57
@klaidliadon
klaidliadon force-pushed the feat/orca-supervisor-command branch from c85d0bb to d64dc49 Compare September 1, 2026 11:45

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ One minor wording issue inline — the delta itself is clean, well-tested, and correctly reasoned.

Reviewed changes

Since the prior pullfrog review (c85d0bb), the branch gained two orcad-layer fix commits (9a059419, 125bd546); the two CLI-layer commits were rebased with identical content.

  • Made --service-path select rather than extend — collectServiceFiles now reads only the explicitly named definition instead of unioning it with the conventional locations, eliminating the false multiple_services_one_root CRITICAL and the probe skip that followed double-collection.
  • Decoded plist entities on read — readPlistString/readPlistStrings undo the generator's escaping so &/</> round-trip, and the doctor no longer accuses its own plist of a data-root mismatch.
  • Refused uid 0 as root — isRootAccount, shared between the generator guard and the audit, flags User=0/00; the numeric spelling can no longer walk between them.
  • Emitted a parseable plist — the launchd header comment no longer contains -- (forbidden in XML comment bodies), with a well-formedness test over both scopes.
  • Stopped guessing a launchd log path — the renderer omits StandardOutPath/StandardErrorPath unless the caller resolves one, and print-service warns via launchdLogDestinationWarning instead.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

): ServiceFileDiscovery {
const candidates =
explicitPaths.length > 0
? explicitPaths.map((path) => ({ path, scope: inferScopeFromPath(path) }))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When --service-path names a file that does not exist, this branch makes the named path the only thing searched, so collectDoctorFindings ends up with zero files and auditSupervisorServices answers no_service_found — No orcad service definition found in the conventional locations., with remedy orcad --print-service. That wording is wrong for an explicitly named path: the operator never asked about conventional locations, and the remedy suggests regenerating rather than checking the path they typed.

Technical details
# Report the named path, not conventional locations

## Affected sites
- src/main/orcad/supervisor-service-discovery.ts:87 — SELECT branch makes an explicit path the only thing searched.
- src/shared/supervisor-service-audit.ts:296 — `no_service_found` hardcodes "conventional locations".

## Required outcome
- When `--service-path` was provided and nothing was found, the finding should name the path the operator supplied rather than claim conventional locations were searched.

## Suggested approach (optional)
- Thread an `explicit` flag (or the requested path) from `collectDoctorFindings` into the audit input, and emit a distinct message/remedy for it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant