Unify Compile and publication journey - #18
Conversation
Make plan compile the single terminal command for validated GitHub publication from one explicitly authorized action. Keep compilation status and artifact download as read-only recovery tools with retained-Head provenance checks.
|
Stack note update: this PR was originally described as 2 of 2. It is now the second feature chunk, followed by #19 for retained Compilation status, provenance, and download. The product Compile behavior here remains the prerequisite for that follow-on. |
ReviewThe journey unification is clean and the concurrency pinning is well designed. One finding that resolves an open question I left on firstdraft#301: the service and CLI changes are a hard flag day in both directions, not just one. Verification
Matches the description. Neither deployment order is safeOn firstdraft#301 I flagged that the released CLI rejects the expanded compilation response, and left open whether the new CLI would tolerate a service that does not yet send Both sides use exact set equality. Released CLI, 13 keys. This branch, 14: const COMPILATION_KEYS = [
"id",
"analysis_run_id",
"graph_version",
"head_source_sha256",
...
];gated at function hasExactKeySet(value, keys) {
return (
isRecord(value) && arraysEqual(Object.keys(value).sort(), [...keys].sort())
);
}All four combinations:
Only the matched pairs work. So:
There is no ordering that avoids a broken window, which makes this a flag day rather than a sequencing question. The description does name the service dependency, which is the right instinct, but "requires #301" understates it: the two must become visible to users simultaneously, or one release has to tolerate both shapes. The cheapest fix is one transitional release where Worth weighing against how many users exist right now. At The ETag design is the best partThe description says the final Publication mutation is pinned to both the accepted ETag and exact Plan bytes. Those turn out to be one mechanism: const HEAD_ETAG_PATTERN = /^"sha256:([0-9a-f]{64})"$/;The ETag is a digest of the Plan bytes, so: headers: {
"If-Match": etag,
},gives optimistic concurrency and byte identity in a single conditional request. A concurrent writer who advanced the Head between analysis and creation changes the bytes, which changes the digest, which changes the ETag, and the That is a genuinely tidy use of a mechanism most codebases only use for caching. Worth noting in the design docs if it is not there already, because a reader who knows ETags only as a cache header will not see why this closes the race. The same idea appears on the download path at response.headers.get("etag") !== `"sha256:${metadata.sha256}"` ||The artifact's ETag is checked against the digest the manifest declared, so a transport that served the wrong bytes is caught before materialization rather than after. Recovery handlingPreserving phase-specific recovery when either the push or the Publication mutation is ambiguous is the right shape, and it matches how the service models the same states. One thing worth confirming against the service side: the ambiguous-Publication case on the CLI needs to agree with the service's The stated limitation is honest
That is a real constraint stated plainly, including what it can and cannot do, which is more useful than a general "publication is limited." A user who compiles, edits their Plan, and compiles again will meet it, so it belongs in the README rather than only in the PR description if it is not there yet. Removals
|
The header you thought was for cachingYou have probably seen The raceCompiling a Foundation Plan takes three steps:
Between step 2 and step 3 there is a gap. Analysis said "this Plan is valid," and then you make a separate request asking for it to be compiled. Now suppose something else pushes a new Plan in that gap. Another terminal, a teammate, an agent doing work in parallel. Step 3 arrives and compiles a Plan that nothing ever validated. Analysis approved version 4; version 5 got compiled. This is a time-of-check to time-of-use bug, and it is one of the most common shapes of concurrency bug there is. You verified something, then acted on it, and the thing changed in between. You will meet it with permission checks, balance checks, inventory checks, and status checks. The fix that does not workThe instinct is to check again: const current = await readPlanStatus();
if (current.etag !== validatedEtag) throw new Error("changed");
await requestCompilation();Look closely and you have moved the gap rather than closed it. Now it sits between the re-check and The fix that does workThe check has to happen inside the operation, on the server, as part of the same request. HTTP has a header for exactly this: response = await sendRequest(fetchFunction, endpoint, {
method: "PUT",
headers: {
Accept: "application/json, application/problem+json",
"If-Match": etag,
},
...
});
Now there is no gap. The comparison and the mutation are one atomic operation on the server, which is the only place they can be atomic. This is optimistic concurrency control. You do not lock anything. You proceed on the assumption nothing changed, and you carry enough information for the server to reject you if you were wrong. Compare pessimistic locking, where you take a lock up front and everybody else waits: optimistic is cheaper when conflicts are rare, and conflicts here are rare. If you have used class Plan < ApplicationRecord
# a lock_version column makes Rails raise
# ActiveRecord::StaleObjectError on a conflicting update
endSame bargain. Carry a version, let the write fail if it moved. The part that makes this elegantHere is where this design is better than a plain version counter: const HEAD_ETAG_PATTERN = /^"sha256:([0-9a-f]{64})"$/;The ETag is not a counter or a timestamp. It is a SHA-256 of the Plan's bytes. That has a consequence worth thinking about. With a version counter, The second is a stronger statement, and it is stronger in a useful direction. Consider somebody pushing a change and then pushing it back:
You get identity rather than sequence. The description mentions being pinned "to both the accepted ETag and exact Plan bytes," and once you see that the ETag is the byte digest, those are one thing said twice. The same idea reappears on the download path: response.headers.get("etag") !== `"sha256:${metadata.sha256}"` ||The artifact's ETag is checked against the digest its manifest declared. A proxy that served a stale or wrong body is caught before anything is written to disk. Weak versus strong ETags, while we are here. An ETag prefixed with Doing this in RailsRails gives you both halves: # generating the ETag
def show
@plan = Plan.find(params[:id])
fresh_when etag: Digest::SHA256.hexdigest(@plan.source)
end
# enforcing the condition
def update
@plan = Plan.find(params[:id])
if request.headers["If-Match"] != %("sha256:#{digest_of(@plan)}")
return head :precondition_failed
end
...
end
The other thing in this PR, and it is a warningThe same file has a key check: function hasExactKeySet(value, keys) {
return (
isRecord(value) && arraysEqual(Object.keys(value).sort(), [...keys].sort())
);
}The response must have exactly these keys. Not fewer, not more. That client is being paired with a service change that adds one field. I checked all four combinations, and only the matched pairs work:
So there is no order in which you can ship these two without a window where somebody is broken. Deploy the service first and existing users break. Release the client first and early upgraders break. That is called a flag day, and it is the thing strict validation on both ends buys you. The escape is one transitional release where the client accepts both shapes:
Four releases to add one field. Worth it when the strictness is load bearing, and worth knowing you have signed up for it. The general lesson pairs with the one above. Strict checks inside a single request, like |
Describe how Compile binds current local bytes to the accepted Head before its conditional Publication request. This makes the concurrency boundary reproducible from the public workflow.
Record why normalization alone cannot preserve these letters before the ASCII identifier filter. This keeps the explicit map from looking arbitrary.
|
Addressed the documentation follow-up in The PR description now also records the intentional coordinated service/CLI rollout. With no published compatible package or users, I am keeping the mixed-version compatibility shim rejected rather than adding transitional cruft. |
Preserve the reviewed Compile journey while inheriting the landed generators and advisory repair required by every hosted check.
Stack
CLI stack 2 of 3, based on #16. The focused provenance correction follows in #19.
Service dependency: firstdraft/firstdraft#301 exposes immutable compilation.head_source_sha256 for historical download provenance. The audit job also requires the independent lockfile repair in #17 to land on main.
The service response and CLI package are a coordinated release pair because both sides validate the exact response shape. The CLI is unpublished and there are no users, so this stack intentionally adds no transitional parser for mixed versions.
Summary
The final Publication mutation is pinned to both the accepted ETag and exact Plan bytes so a concurrent writer cannot advance the candidate between analysis and creation.
Verification
Current limitation
A Project has one retained Publication in this release. Re-running Compile can safely replay that singleton, but cannot repoint it to a later Head. No CLI package was published and no live service or GitHub account was mutated.