Add bounded Plan analysis status polling

[raghubetina]

Summary

• add firstdraft plan status and a bounded --wait mode for current whole-graph analysis
• validate pinned local state, response identity, generation, status, diagnostics, timestamps, and byte limits before emitting one JSON object
• distinguish retryable read failures from deterministic server-contract mismatches
• share the existing audited state and HTTP response boundaries with plan push without changing its public behavior
• document agent recovery semantics, exercise the packed executable offline, and exercise the status client against a real local endpoint

Behavior

A successful status read exits 0 for every validated domain status. Callers branch on analysis.status. --wait repeats only a validated processing response, pins the first analysis identity and generation, stops on the first failed read, and times out after two minutes with the last validated state.

Status uses only the origin pinned by a successful push. It never reads an API override, sends the private ETag, follows redirects, writes local state, or retries writes.

Verification

• asdf exec npm run check
• 85 tests
• typecheck, ESLint, Prettier, package allowlist, and packed-install smoke all green
• local high-effort review converged with no material findings

Part of firstdraft/firstdraft#133.

[raghubetina]

Technical review

A new command plus a refactor that moves 326 lines out of plan-push.js into shared boundaries. npm run check exits 0 with 85 tests and npm audit reports 0 vulnerabilities, matching the body.

The refactor claim has good evidence behind it

"Share the existing audited state and HTTP response boundaries with plan push without changing its public behavior" is the risky part of this PR. plan push is the command that can lose someone's work, and it just had a third of its implementation extracted.

The evidence is that test/plan-push.test.js is not in the diff at all. Twenty-five tests covering ETag preconditions, conditional create and replace, the recovery envelope, exact-byte transmission, and the error contract all pass unmodified against the new implementation. A behavioral change would have had to survive that suite untouched.

That is a better argument than any amount of reading, and it is worth doing this way round: extract into shared modules, change no tests, let the existing suite adjudicate.

The wait loop is the new risk, and it holds up

Polling loops go wrong in predictable ways. I checked each.

Termination. Without --wait the deadline is null and the first validated response returns immediately. With --wait every path out is bounded: terminal status, deadline, identity change, non-200, or a thrown error. Before sleeping it recomputes remaining and throws if it is not positive, so each iteration makes progress toward the deadline.

A single request cannot outlive the wait. The per-request timeout is clamped:

const requestTimeout =
  deadline === null
    ? REQUEST_TIMEOUT_MS
    : Math.max(1, Math.min(REQUEST_TIMEOUT_MS, deadline - now()));

Without the Math.min, a request starting at 119 seconds could run 30 more. Without the Math.max(1, ...), it could be handed a zero or negative timeout at the boundary.

Identity pinning. This is the one I would most expect to be missing:

current = parsed;
first ??= parsed;
if (
  current.analysis.id !== first.analysis.id ||
  current.analysis.graph_version !== first.analysis.graph_version
) {
  throw new PlanStatusChangedError(current);
}

If a different analysis appears mid-poll, --wait refuses rather than reporting it. Without this, a caller could push, start waiting, have someone else push, and receive a succeeded that describes different bytes. Pinning graph_version as well as id is the right pair.

Timeout carries state. PlanStatusTimeoutError(current) returns the last validated response, and the deadline checks require current !== null, so a timeout is only reported once there is something to report. Before the first successful read a network failure propagates as itself rather than being relabelled.

I also confirmed the origin claim: plan status parses only help and wait, and reads state.api_url, so there is no override path. It cannot be pointed at a different host than the one a successful push pinned.

The retryable distinction is real, and documented where a caller will find it

The two new failure modes are separated rather than merged, and the README says what to do with each:

error	Guidance
status_unavailable	"retry the GET a bounded number of times"
invalid_server_response	"retrying unchanged will not repair the mismatch"

That is the distinction the body promises, and it continues the contract work from #6 and #7 rather than inventing a parallel scheme. The per-command table now spans all four subcommands, so a caller can see which codes are reachable from what it ran.

Worth flagging for the Skill side: firstdraft/skills#8 pins the CLI contract at 6019e29 and enumerates the plan push codes with a fail-closed rule for anything unrecognized. These new codes are status-scoped, so nothing breaks there, and the Skill will want a plan status section and a repin once this merges.

Finding: one verification bullet claims more than was done

The body says the PR will "exercise the packed executable against a real local endpoint." Those are two separate things in this diff, and neither is the conjunction.

scripts/smoke-package.js gained two plan status cases, both offline: an invalid-argument failure and local_input_unreadable. Searching that file for createServer, listen(, 127.0.0.1, localhost, or http:// returns nothing. The packed executable never makes a request.

The real local endpoint is in test/plan-status.test.js, which starts a node:http server on 127.0.0.1:0 and drives 22 tests against it. Those run the source runner in-process, not the packed artifact.

So both halves happened and the coverage is good. The claim just joins them in a way the diff does not support, which matters a little more here than usual because the repository holds itself to describing evidence at its observed boundary. Rewording to something like "exercise the packed executable offline and the status client against a real local endpoint" would match what is there.

The practical exposure is small. check-pack.js asserts the exact tarball contents, so a file missing only from the status code path would fail that check rather than escaping to a user.

[raghubetina]

Lesson: the polling loop you probably wrote wrong

You need to wait for something on a server to finish. Here is the loop almost everyone writes first:

loop do
  result = fetch_status
  break if result.done?
  sleep 1
end

It reads fine. It has at least four bugs, and this PR fixes all of them in about twenty lines of JavaScript. Worth walking through, because you will write this loop again.

Bug one: it never stops

If the job never finishes, the loop never exits. Your script hangs forever, and in CI that means a build that runs until someone notices.

You need a deadline, not a retry count. A count of fifty with a one second sleep is not fifty seconds, because each request also takes time. A deadline says what you actually mean:

const deadline = wait ? now() + WAIT_TIMEOUT_MS : null;
// ...
if (deadline !== null && now() >= deadline) throw new TimeoutError(current);

Bug two: one slow request outlives the deadline

Say your wait is two minutes and each request has a thirty second timeout. A request that starts at 1:59 can run until 2:29.

Clamp the request timeout to whatever is left:

const requestTimeout = Math.max(1, Math.min(REQUEST_TIMEOUT_MS, deadline - now()));

The Math.max(1, ...) matters too. Right at the boundary the remaining time can be zero or negative, and handing that to a timeout API either throws or means "no timeout" depending on the library.

Bug three: you might be watching a different job

This is the subtle one, and the reason I wanted to write this lesson.

You push a change and start waiting. While you wait, a teammate pushes their own change. The server starts a new analysis. Your next poll asks "what is the status?" and gets back succeeded.

Succeeded for whose work? Not yours. But your script reports success and moves on, and now a deploy proceeds on the strength of a check that ran against somebody else's code.

The fix is to remember what you started watching and refuse if it changes:

current = parsed;
first ??= parsed;
if (
  current.analysis.id !== first.analysis.id ||
  current.analysis.graph_version !== first.analysis.graph_version
) {
  throw new PlanStatusChangedError(current);
}

A polling loop must know the identity of the thing it is polling, not just its status. If the endpoint only tells you "the current job," you are asking a question whose answer can silently change meaning underneath you.

This generalizes past polling. Any time you read a value, act on it, and read it again, ask whether "it" is guaranteed to be the same "it."

Bug four: timing out tells you nothing

raise "timed out" throws away everything you learned. The caller knows the wait ended and nothing else. Was it still processing? Had it failed and you missed it?

Carry the last good state out with the error:

throw new PlanStatusTimeoutError(current);

Now the caller can distinguish "still running after two minutes, probably fine, check again" from "it was already failing." Those need different responses.

There is a nice detail in the guard here too: the deadline checks require current !== null, so it only reports a timeout once at least one read succeeded. If nothing ever came back, you get the network error instead, which is the more accurate description of what went wrong.

The version that survives contact with reality

def wait_for_completion(deadline: 2.minutes.from_now)
  first = nil
  loop do
    remaining = deadline - Time.current
    raise TimeoutError.new(last: first) if remaining <= 0

    current = fetch_status(timeout: [remaining, 30.seconds].min)
    first ||= current
    raise ChangedError if current.id != first.id

    return current if current.finished?

    sleep [1, remaining].min
  end
end

Still short. Every line is there because of a failure mode, not because of ceremony.

One more thing, about fixed intervals

This polls every second, which is right for a two minute wait against a local service.

For anything longer or anything you do not own, back off: one second, two, four, capped somewhere sensible. A thousand clients polling a struggling service every second is how a slow recovery becomes an outage. And add jitter, a small random offset, so clients that all started together do not stay synchronized and arrive in waves.

The rule of thumb: poll fast when you are the only caller and the wait is short; back off with jitter otherwise. And whenever the option exists, prefer being told over asking. A webhook or a subscription beats even a well-written polling loop, which is ultimately a way of asking "are we there yet" until somebody says yes.

[raghubetina]

Corrected the PR body to keep the evidence boundaries separate: the packed executable is exercised offline, while the status client is exercised against a real local endpoint. No code change was needed.