Structure local command failures

[raghubetina]

Summary

• emit the established invalid_arguments JSON envelope for handled plan init and plan subject-id syntax failures
• add a narrowly scoped local_initialization_failed envelope for filesystem failures that may leave .firstdraft incomplete
• exercise exact stderr framing, exit classes, privacy, and unchanged success output through the runner and installed tarball
• document the complete subcommand contract and the intentionally unchanged plain-text command-group boundary

Contract

Handled failures from plan init, plan subject-id, and plan push now write exactly one JSON object to stderr. Agents branch on the stable error field rather than detail; unexpected defects and failures before subcommand dispatch remain uncaught.

This slice does not change plan push, root-level usage output, success stdout, or exit-status classes.

Verification

• npm run check on pinned Node.js 24.18.0
• 63 tests passing
• typecheck, lint, formatting, package allowlist, and offline installed-tarball smoke passing
• npm audit --audit-level=low reports zero vulnerabilities

[raghubetina]

Technical review

The follow-up slice noted on #6, where plan init and plan subject-id still emitted prose while plan push had a contract. Small and consistent with what is already there.

npm run check on the branch exits 0 with 63 tests passing and npm audit reports 0 vulnerabilities, matching the body.

The change

Four call sites move to the existing writeJson envelope: two invalid_arguments in runPlanInit (argument parsing and the application key/name validation), one in runPlanSubjectId, and the new local_initialization_failed for a filesystem failure during init. Exit classes are untouched, 2 for syntax and 1 for a failed write.

Reusing invalid_arguments across all three subcommands rather than minting per-command variants is the right call. The situation is identical from a caller's perspective, and the detail already names which subcommand was invoked.

The README table gained the dimension it needed

The contract table is now keyed by command as well as code:

Commands	error
plan init, plan subject-id, plan push	invalid_arguments
plan init	local_initialization_failed
plan push	the other five

That matters more than it looks. A caller needs to know which codes are reachable from the command it actually ran, and a flat list of seven would have implied that any of them can come from anywhere.

The privacy line is not decorative

The guarantee was extended to cover "runtime paths" and "raw filesystem" errors, which is exactly the risk local_initialization_failed introduces. A filesystem error naturally carries an absolute path, and interpolating one into the envelope would leak the user's directory layout into an agent's context and any log that captures it.

I checked the detail string rather than taking the claim on faith:

const PLAN_INIT_FAILED_DETAIL =
  "Could not initialize .firstdraft. The directory may be incomplete; no existing files were overwritten.";

No path, and the caught error is not interpolated anywhere. The installed-tarball smoke asserts exact stderr framing, empty stdout, the exit status, and the absence of the canary secret for all three new envelopes, so this is pinned through the published artifact rather than only the in-process runner.

The stated boundary holds up

The body calls the plan command-group level intentionally unchanged, and the README says so plainly:

Root-level and plan command-group usage failures remain human-readable text on standard error with exit 2. A failure before a subcommand can begin, such as an unavailable working directory, also remains uncaught, as do unexpected programming defects.

Worth testing that reasoning rather than accepting it, since the obvious objection is that an agent invoking a subcommand the installed CLI does not have would hit Unknown command. as prose. In practice the consuming Skill discovers capability by running firstdraft plan --help and requiring the subcommands it needs to be listed, rather than by invoking one and interpreting the failure. So the prose boundary sits outside the path an agent actually takes, and drawing the line there is defensible.

Leaving unexpected defects uncaught is also right. A programming error dressed up in the same envelope as a handled failure would tell a caller it had hit a known condition when it had not.

Cross-repo note

firstdraft/skills#7 pins the CLI contract at d588647 and enumerates six push codes, with a fail-closed rule that treats any unrecognized error value as an unknown outcome. This PR adds a seventh code scoped to a different subcommand, so nothing there breaks and the Skill's rule handles it safely if it arrives unexpectedly.

Still, the Skill currently has no branch for local_initialization_failed, and its init handling detects damage with test -f and test -r checks instead. Once this merges, updating that Skill to consume the init envelope and repin the baseline is the natural next step on the other side.

[raghubetina]

Lesson: say where your error handling stops

Most advice about error handling is about what to catch. This PR is interesting for the opposite reason. It is careful about what it does not catch, and it writes that down.

The README ends its list of error codes like this:

Root-level and plan command-group usage failures remain human-readable text on standard error with exit 2. A failure before a subcommand can begin, such as an unavailable working directory, also remains uncaught, as do unexpected programming defects.

Three things declared out of scope, on purpose, in the same document that promises the contract. That sentence is doing real work, and the habit is worth copying.

The two ways this normally goes wrong

Catching too little is the one everybody worries about. A failure escapes, the user sees a stack trace, nothing is handled.

Catching too much gets far less attention and is worse, because it is silent. You write this:

def import(file)
  parse_and_save(file)
rescue => e
  Rails.logger.error(e)
  { status: "failed" }
end

Now every failure is a "failed import." A malformed file, sure. But also a typo in a method name, a missing migration, a nil where you expected a record. All reported to the caller as though you understood what went wrong and handled it.

The caller does the reasonable thing with {status: "failed"}. It tells the user their file was bad. The user fixes a file that was never the problem, and your actual bug is now invisible, buried in a log nobody greps.

A bare rescue converts your bugs into your users' problems. That is why this codebase lets unexpected defects crash rather than wrapping them in the same envelope as handled failures. A caller seeing local_initialization_failed learns something true. A caller seeing that code because of a typo learns something false.

Where to draw the line

The useful question is not "could this raise?" but "do I know what this means, and can the caller do something specific about it?"

Look at the shape the CLI uses:

} catch (error) {
  if (!isFileSystemError(error)) throw error;

  writeJson(stderr, {
    error: "local_initialization_failed",
    detail: PLAN_INIT_FAILED_DETAIL,
  });
  return 1;
}

The guard clause is the whole idea. A filesystem error during init is understood: the directory may be half-written, and the caller should stop and look. Anything else gets rethrown, because the code has no idea what it is and pretending otherwise would be a lie.

In Rails the same discipline usually means rescuing the specific class:

rescue ActiveRecord::RecordNotFound
  head :not_found

and letting everything else reach your error tracker, which is what it is for.

The part people skip

Deciding the boundary is half of it. The other half is telling whoever depends on you where it is.

An undocumented boundary gets discovered in production. Someone assumed every failure came back as a clean error object, wrote code on that assumption, and it held right up until the day it did not. Nothing was broken. They just could not see where your promise ended.

So write it down. In a README, in the API docs, in a comment above the rescue. One sentence covering:

• what you handle and how the caller can tell
• what you deliberately let through
• what happens to the caller when it does

That last one matters most. "Unexpected defects are uncaught" tells a caller they need a crash path, not just an error path. Without it they will assume the error path is enough.

The uncomfortable version

A promise you cannot keep is worse than no promise. If your API says every failure returns a structured error, and one path escapes with a raw exception, you have not merely failed to handle a case. You have taught every caller to skip defensive handling that would have caught it.

Narrow, honest, documented beats broad and aspirational. "I handle these four things, everything else crashes" is a contract someone can build on. "I handle errors" is not.

[raghubetina]

Thanks for checking the boundary against the actual Skill discovery path. Agreed on the cross-repository follow-up: the Skill should consume local_initialization_failed and repin its CLI baseline now that this contract is on main; that remains separate from this deliberately narrow CLI slice.