Teach Skill to recover from init errors

[raghubetina]

Summary

• repin the Skill to merged CLI baseline 6019e2935079f4a844611443558176b44b770f81
• make plan init branch on invalid_arguments and local_initialization_failed, preserve incomplete local state, and fail closed on unknown output
• retain project-relative file checks as evidence without exposing paths, raw errors, Plan bytes, or private state
• add behavioral evals for invalid arguments, partial initialization, and unparseable output
• verify the exact merged CLI through both its source runner and a freshly packed and installed executable

Boundaries

• The existing six-code plan push table and recovery semantics are unchanged.
• This does not expand Foundation Plan authoring or server capabilities.
• This does not publish the Skill or CLI.

Verification

• sh script/check — 12 tests pass
• node script/check-cli-contract.mjs /Users/sandbox2/code/firstdraft/cli — exact CLI runner and packed artifact pass
• npm audit --audit-level=low — 0 vulnerabilities
• gh skill publish --dry-run . — passes with the existing license and tag-protection advisories
• bundled schema remains byte-identical to current First Draft, SHA-256 5994c41f65eab52f92020fa24437e76b6957b7016ccf231dce06e8097f0b34b5

[raghubetina]

Technical review

This closes the manual-binding gap I raised on #7, and it closes it harder than I suggested there. I proposed vendoring the codes and digesting them. This runs the actual CLI instead.

Verification reproduced

sh script/check                              → 12 tests pass
node script/check-cli-contract.mjs <cli>     → exit 0
npm audit --audit-level=low                  → 0 vulnerabilities

All matching the body. The pinned baseline 6019e2935079f4a844611443558176b44b770f81 is cli#7's merge commit and is currently the tip of that repository's main, so it is a real merged revision rather than a branch SHA that will move.

The binding is genuine, and I checked that it bites

A cross-repo check is only worth having if it fails when the two sides diverge. The wiring makes that possible: CI checks out firstdraft/cli at the exact pinned ref into tmp/firstdraft-cli, then runs the contract script against it.

The script refuses to be pointed at the wrong thing:

const revision = run("git", ["rev-parse", "HEAD"], cliDirectory);
assert.equal(revision.stdout.trim(), cliBaseline);

So it cannot pass against a CLI that is merely nearby. I tested the failure cases rather than assuming them, pointing it at the previous merged CLI commit and at nothing at all:

correct commit (6019e29)  → exit 0
previous commit (d588647) → exit 1
no argument               → exit 1

That is the property that was missing on #7. A rename or a dropped code on the CLI side now fails this repository's CI instead of being discovered by an agent at runtime.

It also guards the properties that make the CLI safe to invoke at all, asserting the packaged bin entry point, no runtime dependencies, and no prepack build step. Those are exactly the things that would quietly stop being true if someone added a bundler.

The init boundary mirrors the push one

The new section has the same shape as the existing push contract: a table keyed on error, an instruction to branch on the code and never on detail or exit status, and a fail-closed rule. That rule is more thorough than its predecessor:

If output has an unknown code, is absent or malformed, mixes JSON with other text, or contains more than one value, fail closed: treat .firstdraft/ as possibly incomplete, preserve it, and stop.

"Mixes JSON with other text" is a good addition. The CLI writes one object, but stderr is a shared stream, and a naive JSON.parse on whatever arrives would throw on any stray line. Naming that case means the agent stops rather than falls through an unhandled parse error.

The demotion of the filesystem checks is the other thing worth calling out. The Skill previously detected damaged state primarily with test -f and test -r. Those checks remain, correctly reframed:

These checks are evidence about local state, not a substitute for the command's error code.

That is the right relationship. The error code says what the command did; the file checks say what is on disk. Using the second to infer the first is how you end up reinitializing over a directory that a failed init left half-written.

Two small notes

script/check changed from node --test to node --test test/*.test.mjs. That glob is not recursive, so a future test/some/nested.test.mjs would silently stop running. Only test/repository.test.mjs exists today, so nothing is lost now. test/**/*.test.mjs would keep the narrowing without the trap.

The contract check is a separate CI step rather than part of script/check, which is reasonable since it needs a CLI checkout that a contributor may not have. It does mean "the checks pass locally" now covers less than it used to. Worth a line in the README's contributor section if anyone is relying on script/check alone before pushing, since the thing most likely to break here is the cross-repo contract.

[raghubetina]

Lesson: testing against the real thing, pinned

Your code calls something you did not write. To test it, you have to decide what stands in for that thing.

There is a spectrum here, and most developers only ever use one end of it.

The spectrum

A stub you wrote. Fast, deterministic, and it tests your understanding of the dependency rather than the dependency.

allow(PaymentGateway).to receive(:charge).and_return(success: true)

Green forever. Including after the gateway starts returning {"status": "succeeded"} instead of {"success": true}, at which point your tests are a monument to how the API worked when you wrote them.

A recorded response. VCR captures a real interaction once and replays it. Better, because the shape was real at least once. It still ages, and a cassette recorded two years ago proves nothing about today.

The live dependency. Accurate and unusable. Slow, needs network and credentials, breaks your build when someone else's service has a bad afternoon, and gives different answers on different days.

The real dependency, pinned. This is the one people forget exists, and it is what this PR does.

What pinned means here

CI checks out the other repository at an exact commit and runs the checks against it:

- uses: actions/checkout@...
  with:
    repository: firstdraft/cli
    ref: 6019e2935079f4a844611443558176b44b770f81
    path: tmp/firstdraft-cli
- run: node script/check-cli-contract.mjs tmp/firstdraft-cli

Then the check refuses to run against anything else:

const revision = run("git", ["rev-parse", "HEAD"], cliDirectory);
assert.equal(revision.stdout.trim(), cliBaseline);

You get the accuracy of the real thing and the determinism of a stub. It is the real CLI, actually executed. It is also the same bytes every run, so a red build means somebody changed something rather than the internet having weather.

The tradeoff is that upgrades become deliberate. Nothing tells you the dependency moved; you find out when you bump the pin. That is the same bargain as Gemfile.lock, and most teams already accept it happily.

Where this beats a mock: a mock encodes your belief about the dependency. A pinned real dependency encodes the dependency. When those two disagree, and eventually they do, only one of them tells you.

The habit worth stealing

Here is the part I want you to take away, and it applies to every test you write.

I did not trust that this check worked. I made it fail.

correct commit  → exit 0
previous commit → exit 1
no argument     → exit 1

Three runs. The middle one is the whole point: I pointed the check at the previous version of the CLI and confirmed it noticed.

A test you have never seen fail is a hypothesis, not a test. Consider how many ways a guard like this passes for the wrong reason. The script silently skips when the directory is missing. An assert sits inside a callback that never runs. Someone wraps it in a try and swallows the error. A typo in the file glob means the whole file is never collected. In every one of those cases you get a green check and no protection whatsoever, and green looks identical either way.

The fix costs about a minute. Break the thing on purpose and watch the test go red.

# You just wrote this. Do you know it works?
test "rejects a negative price" do
  assert_not Product.new(price: -1).valid?
end

Delete the validation and rerun it. If it still passes, your test was checking something else, probably that a different required attribute was missing. Put the validation back and move on, now actually knowing.

This is the manual version of mutation testing, which automates exactly this idea: change the code, and any test that does not notice was not testing that code. You do not need the tooling to get most of the value. You need the instinct to ask, once, "what would make this fail?" and then go make it fail.

Where to spend the effort

Not everything deserves a pinned real dependency. The rough ordering:

Stub the things that are slow, expensive, or hard to reach, and accept that you are testing your own assumptions. Record real interactions when the shape is complicated and reasonably stable. Pin and run the real thing when a contract between two codebases is the thing most likely to break, which is exactly the case here, since the two repositories ship separately and drift silently.

And whichever you pick, break it once to confirm it can fail. That step is free, and it is the only evidence you will ever have that the check is doing anything at all.

[raghubetina]

Addressed the recursive-discovery note in 1b341a1: script/check now passes the quoted test/**/*.test.mjs pattern to Node, so it remains scoped to this repository while admitting future nested tests. I verified all 12 tests on both Node 22.0.0 and 24.18.0.

The second note was already covered in the Development section: it documents the separate pinned-CLI command, explains that it exercises the source runner and packed artifact, and records that CI runs it after the repository-only suite.