Branch Skill recovery on stable CLI codes

[raghubetina]

Summary

• branch every handled plan push recovery path on the merged CLI JSON error discriminator rather than mutable prose
• preserve one-request-per-candidate behavior, stop-only handling for unknown outcomes, and private recovery_state
• pin the merged CLI contract at d588647 and model 422 diagnostics through the real server_rejected envelope
• cover all six stable codes in the always-loaded Skill, reference table, behavioral evals, and repository tests

Boundary

This remains an experimental authoring and bounded-diagnostics Skill. It does not add capabilities, publish the Skill or CLI, or claim Publish, Compilation, generation, deployment, or client support.

Verification

• sh script/check — 11 tests pass
• npm audit --audit-level=low — 0 vulnerabilities
• gh skill publish --dry-run — passes with the existing recommended-license and tag-protection advisories
• merged CLI npm run check — 62 tests plus typecheck, lint, formatting, package allowlist, and packed-artifact smoke pass
• bundled Foundation Plan schema remains byte-identical to First Draft

[raghubetina]

Technical review

This closes the coupling I raised on #6, where two recovery branches were keyed on English sentences the CLI happened to print. One linear commit. sh script/check passes with 11 tests and npm audit reports 0 vulnerabilities, matching the body.

The pin is real and the fixtures match what the CLI actually produces

firstdraft/cli#6 merged as d588647044e64333d14bf467f4eb7d43728305db, which is currently the tip of that repository's main. So the cited baseline is a genuine merged commit rather than a branch SHA that will move.

The claim worth testing hardest is that the fixtures "model 422 diagnostics through the real server_rejected envelope." A hand-written fixture that merely resembles the CLI's output would reintroduce the same drift problem in a new place.

I extracted the projection function from the merged CLI at that commit and ran the fixtures' own diagnostics through it:

unsupported-field-capabilities-diagnostics.json → FIXTURE MATCHES merged-CLI projection exactly
malformed-json-diagnostics.json                 → FIXTURE MATCHES merged-CLI projection exactly

Exact, including key order. The detail string also matches the CLI's actual constant rather than a paraphrase:

const PLAN_PUSH_SERVER_REJECTED_DETAIL = "First Draft rejected the Plan.";

The underlying diagnostics are still the ones I verified against First Draft main during #6, so the whole chain holds: server emits it, CLI projects it, fixture records it.

The vendored schema is also unchanged, still 5994c41f65eab52f92020fa24437e76b6957b7016ccf231dce06e8097f0b34b5 and still byte-identical to First Draft main.

The prose branches are genuinely gone

Both rules I flagged on #6 now key on codes. Searching the recovery reference for the old detection phrasing returns only a table row that describes what request_outcome_unknown means, not a rule that detects it by matching text. The instruction is now explicit:

Parse that object and branch on its stable error value. Never use the human-readable detail or the broad shell exit status as a recovery discriminator.

All six codes appear in both the always-loaded SKILL.md and the reference, and the repository tests assert that: the SKILL.md push section must contain a branch for every code in planPushErrorCodes, each eval case must carry both a "error":"<code>" prompt and a matching "Branches on <code>" expectation, and there is a doesNotMatch guarding against the prose rules coming back.

The best thing here is the rule for output it does not recognize

This is the part I would keep if the rest were thrown away:

If a failed command does not produce one parseable JSON object with one of these six error values, its outcome is also unknown. Stop, preserve the local files, and report the failure without exposing private state.

That covers two gaps at once. The CLI rethrows unhandled errors with a stack trace rather than an envelope, which I noted on cli#6 as correctly scoped but uncovered on the consumer side. It also covers a future CLI that adds a seventh code or renames one.

That second case matters more than it looks, and it changes how worried anyone should be about the remaining manual coupling below. If the CLI's contract drifts, the Skill sees an error value it does not know, falls into this rule, and stops. The failure mode is safe and unhelpful rather than confidently wrong, which is the right way for a contract mismatch to fail.

Note: the cross-repo binding is still manual, and that is now mostly fine

The test file hand-maintains both halves of the contract:

const foundationPlanCliBaseline = "d588647044e64333d14bf467f4eb7d43728305db";
const planPushErrorCodes = ["invalid_arguments", ..., "local_state_not_saved"];

Nothing fetches or hashes anything from the CLI. The baseline is asserted only to appear as a string in the reference document. So if the CLI renames a code tomorrow, every test in this repository still passes.

Worth contrasting with how this repository already handles the same problem for the schema, which is vendored and then digest-verified in the suite:

assert.equal(createHash("sha256").update(schemaSource).digest("hex"), "5994c41f...");

The same pattern would work here. Vendor the six codes as a small fixture derived from the pinned CLI commit, digest it, and a rename becomes a CI failure with an obvious remedy instead of a silent divergence discovered at runtime.

I would not treat this as blocking, for the reason in the previous section. Six documented codes plus a recorded baseline commit plus a fail-closed default is a large improvement over matching sentences, and the unknown-code path already prevents the dangerous outcome. This is the difference between "drift stops the agent safely" and "drift tells you at CI time", which is worth having eventually but not worth holding a PR for.

One small observation: the test count stayed at 11 while the suite gained 134 lines, so the new assertions strengthened existing tests rather than adding cases. That is fine and arguably the right structure here, just worth noting since an unchanged count can read as an unchanged suite.

[raghubetina]

Lesson: depending on something you do not control

Your code calls something maintained by other people. A gem, an internal service another team owns, a CLI, a payment API. You cannot stop them from changing it, and you will not get a heads-up.

The whole discipline is about making their changes fail in a way you notice, instead of a way you do not.

This PR is a small, complete example. A Skill in one repository consumes a CLI in another. A previous version detected one failure by matching the sentence the CLI printed. That worked until the CLI reworded the sentence, at which point it kept working purely by accident, because the phrase it matched happened to survive the rewrite.

Three ways to hold a dependency, worst to best

Match on prose. if stderr.include?("could not connect"). Free to write, breaks on a typo fix. This is what got replaced here.

Branch on a stable identifier. The CLI now emits {"error": "request_outcome_unknown"} and documents that the code is the contract and the message is not. Much better, because now the other team knows which part they promised not to change.

Bind it mechanically. Best, when you can get it. Something in your test suite fails when their side moves.

Most codebases stop at the second. That is often the right call, and worth being deliberate about rather than lazy about.

What "bind it mechanically" looks like

The same repository already does this for a different dependency. It vendors a copy of a JSON Schema owned by another project, and the test suite hashes the copy:

assert.equal(
  createHash("sha256").update(schemaSource).digest("hex"),
  "5994c41f65eab52f92020fa24437e76b6957b7016ccf231dce06e8097f0b34b5",
);

Anyone editing that vendored file gets a red test. The digest turns "we think these match" into something the machine checks.

You already use weaker versions of this everywhere. Gemfile.lock records exact versions so bundle install is reproducible. VCR cassettes record real API responses so your tests break when you change the request shape. Contract testing tools like Pact exist entirely for this problem between services.

The error codes in this PR do not get that treatment. The test file holds a hand-copied list of six strings and a hand-copied commit SHA. If the CLI renames a code, every test here still passes. I pointed that out in the review as a natural next step, since the pattern is already in the repository.

The part that makes it acceptable anyway

Here is the design decision worth stealing. The Skill says this about output it does not recognize:

If a failed command does not produce one parseable JSON object with one of these six error values, its outcome is also unknown. Stop, preserve the local files, and report the failure without exposing private state.

Think about what that buys. The CLI adds a seventh code next month. Nothing in this repository notices. The agent hits that code at runtime, does not recognize it, and stops safely instead of guessing.

Drift becomes safe-but-unhelpful rather than confidently wrong. That is the difference between a system that inconveniences you and one that corrupts something.

Compare the shapes:

# Confidently wrong: an unknown status silently means "not paid"
def paid?
  status == "paid"
end

# Safe: an unknown status is an error, not a false
def paid?
  case status
  when "paid" then true
  when "pending", "failed" then false
  else raise UnknownPaymentStatus, status
  end
end

The first is shorter and reads fine. Then the payment provider adds "paid_out_of_band", and you quietly stop shipping orders that were actually paid. Nothing errors. The bug surfaces as a customer complaint weeks later.

The second is noisier and correct. An unrecognized value means you have learned something you did not know, and the honest response is to stop rather than to fold it into whichever branch happens to be the default.

Rails leans this way too. enum raises on an unknown value rather than storing nil. Strong parameters drop unpermitted keys rather than guessing at intent.

The habit

When you consume something you do not own, ask two questions.

What exactly am I depending on, and is it the part they promised? A code, a status, a documented field. Not the wording, not the key order, not whatever the response happened to include last Tuesday.

When their side changes, where does it surface? In your CI, if you managed to bind it. At runtime otherwise, in which case the unknown case had better stop rather than guess.

Getting the second one right matters more than getting the first one perfect. Plenty of dependencies are not practical to bind mechanically. None of them are improved by a default branch that treats "I have never seen this" as "it is fine."

[raghubetina]

Thank you. I agree with the review and am leaving the cross-repository mechanical binding as a deliberate follow-up rather than adding a hand-vendored manifest here.

A local digest would prove that this repository did not accidentally edit its copied six-code list, but it would not detect the CLI changing after the pinned commit; the copied fixture and digest would remain green together. A genuinely mechanical binding should start with a machine-readable contract artifact owned by the CLI, then have this repository consume or verify that artifact at an explicit baseline. Until that distribution boundary exists, the merged SHA, tests around every known branch, packed-CLI verification, and fail-closed unknown-code rule provide the intended safe boundary.

No code changes are needed from the hosted review.