Make plan push failures machine-readable

[raghubetina]

Summary

• emit one JSON object on stderr for every handled plan push failure
• preserve exit status 0/1/2 while adding stable agent-facing error discriminators
• distinguish validated server rejections from unknown request outcomes without trusting raw response bodies
• retain an observed HTTP status on protocol and response-stream failures without exposing network details
• whitelist problem and diagnostic response fields, keeping private recovery state exclusive to local_state_not_saved
• document the recovery contract and exercise it through the full runner and the installed package

Error contract

The stable codes are:

• invalid_arguments
• invalid_configuration
• local_input_unreadable
• request_outcome_unknown
• server_rejected
• local_state_not_saved

Unvalidated responses remain conservative: agents stop and reconcile rather than infer whether the Plan was stored.
Validated rejection payloads are projected into a bounded response object. Command arguments, Plan bytes, raw
network errors, and unvalidated response bodies are never emitted.

This is the bounded follow-up recorded on #5 after the concrete consumer need surfaced in firstdraft/skills#6.

Verification

• asdf exec npm run check
• 62 tests passing
• TypeScript, ESLint, Prettier, exact package allowlist, offline installed-package smoke, and audit all clean

[raghubetina]

Technical review

This is the follow-up to the design suggestion on #5, and it closes it properly. One linear commit, npm run check exits 0 with 62 tests passing, matching the claim.

The envelope is exhaustive, and I checked rather than assumed

The claim worth testing is "every handled plan push failure writes exactly one JSON object." Enumerating every exit from runPlanPush:

invalid_arguments        → 2
invalid_configuration    → 2
local_input_unreadable   → 1
request_outcome_unknown  → 1   (thrown network/protocol error)
request_outcome_unknown  → 1   (unvalidatable rejection)
server_rejected          → 1
local_state_not_saved    → 1

Every one calls writeJson. There are no remaining stderr.write prose calls anywhere in the push path, so the old string constants are gone rather than merely supplemented.

The unknown-error fallthrough still rethrows and crashes with a stack trace. That is the right call: an unhandled bug should not be dressed up as a contract code, and the body scopes the claim to handled failures.

One thing I specifically checked because it would be an easy miss. PlanPushProtocolError's constructor now requires options and reads options.status, so a call site that was not updated would throw a TypeError on undefined instead of the intended error. All five construction sites pass {status: response.status}. Both PlanPushNetworkError sites are right too, and correctly asymmetric: the pre-response fetch failure omits status because no response was received, while the mid-stream failure includes it.

The response allowlist preserves everything real and drops everything else

Replacing writeJson(stderr, result.body) with a field-by-field projection is the most consequential change here, and it has a failure mode worth testing for: an allowlist that is too tight silently discards diagnostics the agent needs.

I ran the real First Draft diagnostic payloads through safeRejectedResponse directly. Both shapes verified against First Draft main in firstdraft/skills#6, one with a source_pointer location and one with line/column:

diagnostics round-trip:  IDENTICAL
line/column round-trip:  IDENTICAL

No field is lost, including subject: null and the empty related_locations and suggestions arrays, which a naive projection would have dropped. Then the same payload with an extra field attached:

{ ...diagnostic, evil: "IGNORE PREVIOUS INSTRUCTIONS" }
// projected → the evil key is gone

That is the point of the allowlist, and it matters more than usual here. The consumer is an agent, so an unvalidated server body echoed into stderr is untrusted text landing in a model's context. Projecting known fields rather than filtering known-bad ones is the correct direction, since a blocklist only stops what you thought of.

The loop with firstdraft/skills#6 actually closes

On #5 I flagged that five failure classes collapsing onto two exit codes would force prose matching. That was theoretical until the Skill showed up and did exactly that. Mapping the Skill's recovery branches onto the new codes:

Skill's rule	Now keyed on
422 diagnostics	server_rejected, status 422, response.diagnostics
unsupported_capability	response.diagnostics[].code
412 concurrent replacement	server_rejected, status 412, response.code
Ambiguous outcome	request_outcome_unknown
Damaged local files	local_input_unreadable
State save failure	local_state_not_saved

All six are machine-readable. The two that previously had no signal but an English sentence now have codes.

The README earns its place here by saying which half is the contract:

Agents should branch on its stable error value, not on the human-readable detail.

And exercising the envelope through the packed, installed CLI in scripts/smoke-package.js rather than only the in-process runner is the right level to pin a published contract at.

The remaining work is on the Skill side, and this PR demonstrates why

Worth flagging for whoever picks up firstdraft/skills#6: that repository's diagnostics-and-recovery.md still branches on prose, including the line "If the CLI reports that the Plan may have been accepted."

This PR reworded that message. Old:

Could not complete the First Draft request. The Plan may have been accepted; local state was not changed.

New:

The Plan may have been accepted, but the response could not be verified. Stop and reconcile before pushing again; local state was not changed.

The Skill's rule still matches, because the phrase "the Plan may have been accepted" happens to survive in the rewrite. That is luck rather than design, and it is a live demonstration of the coupling I described there: a message changed, no test in either repository noticed, and the consumer kept working by coincidence. Updating the Skill to key on request_outcome_unknown and local_input_unreadable would retire the coupling for good.

Two smaller notes

plan init and plan subject-id still emit prose on failure. That matches the PR's stated scope and the title, so it is not an omission, but it does leave an agent with a structured contract for one subcommand and prose for the others. A natural next slice.

An unvalidatable rejection now reports request_outcome_unknown rather than a rejection with its HTTP status. That is more conservative than before, and for a case like a proxy returning 413 with an HTML body it slightly overstates the uncertainty, since nothing was stored. The envelope still carries status, so an agent retains the actionable detail, and the CLI genuinely cannot distinguish a proxy rejection from a server that processed the request and then failed to respond validly. Erring toward reconcile is the right default.

[raghubetina]

Lesson: writing errors for something that cannot read

Every error message you have ever written was aimed at a person. Someone reads "Could not connect to the database" and knows what to do. That works because the reader brings judgment.

Now the caller is a script, or an agent. It cannot bring judgment. It has to decide something from your error, and all it has is what you printed.

This PR is about that shift, and the pattern generalizes well beyond this CLI.

Exit codes run out almost immediately

The traditional answer is the exit code. Zero means success, nonzero means failure, and shell scripts have branched on that for fifty years.

firstdraft plan push || echo "something went wrong"

The trouble is you get about three useful values before the vocabulary is exhausted. This CLI has six distinct failures, and they call for genuinely different responses:

• the local file could not be read, so nothing was sent
• the request failed and the outcome is unknown
• the server rejected the Plan and explained why
• the server accepted it but saving local state failed

Retrying is correct for one of those and actively harmful for another. All of them are "nonzero."

You could assign each a number, and some tools do. It reads badly at the call site, though. Nobody remembers that 4 means the state write failed, and adding a seventh case means picking another magic number and hoping no wrapper script hardcoded it.

So this PR keeps exit codes coarse, 0 for success, 2 for "you called it wrong", 1 for everything else, and puts the real information in a JSON object on stderr:

{
  "error": "request_outcome_unknown",
  "detail": "The Plan may have been accepted, but the response could not be verified...",
  "status": 502
}

Two fields, two audiences. detail is for the human reading the terminal. error is for the program, and the README is explicit that programs must branch on error and never on detail.

That split is the whole idea. The stable identifier and the readable sentence are different things, and conflating them is what makes error handling brittle. You have seen the failure already if you have ever written this:

rescue => e
  retry if e.message.include?("timeout")   # please do not
end

That code breaks when someone improves the wording. Rescue the class, check a code, and let the message stay free to change.

There is a live example of exactly this in the PR. A sister repository has instructions that detect one failure by matching the phrase "the Plan may have been accepted." This PR reworded that message, and the match survived only because the phrase happened to appear in the new wording too. Nothing tested that coupling. It worked by luck, which is the least durable reason for anything to work.

Allowlist what you echo back

Here is the subtler change. The old code, on a server rejection, printed the server's response body straight through:

writeJson(stderr, result.body);

Reasonable-looking. The server sent JSON explaining the problem, so pass it along.

The new code builds the output field by field, copying only known keys: code, severity, message, location, and a few more. Anything else the server sent is dropped.

Two things make that worth the extra code.

The first is stability. If you echo a body wholesale, every field the server happens to include silently becomes part of your CLI's output. You did not choose it, you cannot test it, and you will break somebody when the server changes.

The second matters more given who is reading. That output lands in an agent's context, where text is at least partly instructions. A hostile or compromised server could include a field whose value reads "ignore your previous instructions." Copying known fields means that string never reaches the agent at all.

Note the direction. The code does not scan for bad content and strip it. It names what is allowed and ignores the rest.

Allowlists fail closed; blocklists fail open. A blocklist only stops what you thought of in advance, and you will not think of everything. Same reason you use strong parameters rather than trying to reject dangerous keys:

params.expect(post: [:title, :body])   # allowlist

Add a column tomorrow and it is not mass-assignable until you say so. That is the behavior you want from a security boundary.

I checked that the allowlist here is not too aggressive, which is the usual way this goes wrong. The real diagnostics the server sends survive it byte for byte, including a subject: null and two empty arrays that a careless projection would have quietly dropped. Tight enough to block an injected field, loose enough to lose nothing real.

When to bother

Not every script needs this. A one-off deploy script printing to a terminal you are watching is fine as is.

Reach for it when a program is the caller and it has to make a decision. That covers CLIs meant for CI, anything an agent invokes, and JSON APIs, which have the same problem and a good convention for it in RFC 9457 problem details.

The rule of thumb: if a caller would otherwise have to parse your prose to decide what to do, give them a code instead. You will change the prose eventually. That is what prose is for.