new-website: close two holes in the shared push classifier - #74
Merged
Conversation
Found by Codex reviewing the same two functions after they had been copied into apreet-site — the fourth repo carrying them. Both reproduced against v0.20. 1. GitHub's actual permission error is "ERROR: Permission to <repo>.git denied to <user>" — which contains neither "Permission denied" nor anything network-ish. It fell through the auth list, matched the generic "Could not read from remote repository" in the network test, and was RETRIED as a blip. Same shape as the host-key hole closed in #70, sitting right beside it. 2. Mid-transfer disconnects that don't say "hung up" — "error: RPC failed", "send-pack: unexpected disconnect" — matched nothing at all. Codex proposed putting (2) in the retry arm. Taking the finding but not the fix: these are MID-TRANSFER, so on the push path the upload may already have landed and a blind retry is the one thing that must not happen. They join the arm, which refuses to guess and tells the owner to re-run — self-diagnosing, since an already-landed push returns "Everything up-to-date". They stay in the network function for the FETCH path, where a retry is free because nothing has been published. 28 scenarios; the two new ones fail against v0.20 (permission-denied retried twice as "network", RPC drop unrecognised). Still to propagate: GenAI_site, m-squad-website and apreet-site all carry these two functions verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…esson Every failure message in the publish path already says two things — what went wrong, and what state you are in now — but nothing said that was the rule, so the next arm added would only have to remember it. The stronger half came from the sibling deploy script for apreet.com, which needs a local merge and therefore CAN leave a half-finished state behind. Its review concluded that describing that state was the wrong answer: it now restores the branch to its pre-merge tip instead, because a state that no longer exists needs no explanation and cannot trip the next run. ship.sh gets that property for free by pushing main:production with nothing checked out. Stating the rule anyway, since the reasoning is what transfers: prefer undoing to documenting, and document only what cannot be undone — which is exactly why the dropped arm refuses to guess rather than asserting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The marker's own instruction is to bump it when ship.sh changes, and this branch changes ship.sh. Left at 0.20 it would tell every site grepping it that they were current when they were not — which is the one job the marker has. Caught by checking the change against the rest of the suite rather than by reviewing the diff: nothing in the diff itself looks wrong, and no reviewer reading it would see a stale number in an unchanged line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex and ollama-cloud on the branch's own diff. Two real regex holes, both mine,
both in patterns that get copied into other repos.
"error: RPC failed" is an UMBRELLA, not a transport diagnosis — HTTP 401/403/413
wear it too, and none of those is transient. Now requires curl's own error number
after the semicolon, which the HTTP forms don't have. Isolated by a 413 fixture
rather than a 403: a 403 also prints "Authentication failed", which the auth arm
catches anyway, so it would not have proved anything.
"fatal: protocol error" covered persistent configuration faults ("bad line length
character"), and on the push path it sat in the network arm only — so it was
RETRIED, contradicting this file's own stated mid-transfer policy. Removed.
Both reviewers independently flagged the auth wildcard: "Permission to .* denied"
would match arbitrary remote-hook prose containing those words. Anchored to
GitHub's shape with [^[:space:]]+.
Codex also challenged the mid-transfer no-retry REASONING, and was right. A
repeated non-force push of the same ref is idempotent — it lands, says "Everything
up-to-date", or is rejected — so a retry would be SAFE. The real reason is cost:
the pre-push gate has already run by then, so a retry spends another full build
and test run on a state nobody can describe yet. The comment now says that
instead of implying a safety argument that does not hold.
Fixtures: the mid-transfer case tested both alternatives in one input, so either
could have broken silently — split. Added negative cases (RPC-wearing-HTTP-413,
persistent protocol fault) and one for the credential wording that had none,
which guards its arm rather than proving it: "Authentication failed" on the next
line already matched.
Trimmed the volatile repo count and the sibling project's name from the comments,
per the review — this code is copied across repos and that detail goes stale.
32 scenarios green; rpc413 and protocolfault both fail against the previous
commit on this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by Codex reviewing these same two functions after they had been copied into
apreet-site— the fourth repo carrying them. Both reproduced against v0.20.1. GitHub's actual permission error was retried as a network blip. The real wording is
ERROR: Permission to <repo>.git denied to <user>— which contains neitherPermission deniednor anything network-ish. It fell through the auth list, matched the genericCould not read from remote repositoryin the network test, and got retried. Same shape as the host-key hole closed in #70, sitting immediately beside it.2. Mid-transfer disconnects that don't say "hung up" —
error: RPC failed,send-pack: unexpected disconnect— matched nothing at all.Taking the finding, not the fix
Codex proposed putting (2) in the retry arm. These are mid-transfer: on the push path the upload may already have landed, so a blind retry is the one thing that must not happen. They join the
droppedarm, which refuses to guess and tells the owner to re-run — self-diagnosing, since an already-landed push returns "Everything up-to-date". They stay in the network function for the fetch path, where a retry is free because nothing has been published.Verification
28 scenarios green. The two new ones fail against v0.20: permission-denied retried twice as "network", RPC drop unrecognised.
Propagation
GenAI_site,m-squad-websiteandapreet-siteall carry these two functions verbatim and need the same change. That is the cost of copying rather than sharing — worth stating plainly rather than discovering later.🤖 Generated with Claude Code