Skip to content

feat(routing)!: the routing table fails closed — HUB_UNREADABLE, skew, spoke roles, offline (#259) - #279

Merged
radiusred-cody[bot] merged 4 commits into
mainfrom
task/259-routing-fails-closed-hub-unreadable-prot
Sep 6, 2026
Merged

feat(routing)!: the routing table fails closed — HUB_UNREADABLE, skew, spoke roles, offline (#259)#279
radiusred-cody[bot] merged 4 commits into
mainfrom
task/259-routing-fails-closed-hub-unreadable-prot

Conversation

@radiusred-cody

@radiusred-cody radiusred-cody Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Routing used to fail open. A spoke fetches the hub's .codecrew/config.yml to resolve roles, and on any failure it kept the local pointer's table — which in a spoke is empty. An empty table resolves every seat to ~, and ~ is the branch where task finish's holder-review gate falls through to "any non-author approved" and milestone close counts a QA verdict from any commenter. The two gates the protocol exists to enforce, disabled by a 404 nobody sees. SPEC §6 said the fetch was "advisory and is not checked", which is what made it a bug you could not fix without a protocol major.

The .codecrew/ move guarantees that 404 for a whole migration window, in both directions of skew — which is why M13-R5 rides 2.0.

What changed

The routing table is resolved once, at load(). A ctx cannot exist without one, so rolesConfig() stays infallible and no role predicate — the review gate, the verdict count, crewIdentity, roleFor — can forget an error path. That is the structural half of the fix: the fail-open shape is no longer reachable, rather than being handled correctly at each of six call sites.

Which table governs is chosen by topology, not emptiness:

  • hub: self — the local pointer is the table. No fetch, so a hub resolves roles with the network down, and a hub declaring no table is still legitimately ~ everywhere.
  • hub: owner/repo — the table is always the hub's, fetched at load. A spoke cannot carry one (below), so there is nothing local to prefer.

HUB_UNREADABLE — a hub pointer that cannot be fetched (404, 403) or parsed refuses, naming the hub and the path. The detail branches on the shape, because the remedies differ and the wrong one sends an operator to a writing verb against a healthy hub: a 403 names access, the way NO_CHECKS_PERMISSION names a permission, and a 404 owns all three of its causes — a hub still on the 1.x layout, moved forward with gh codecrew migrate; a hub: line naming the wrong repo; or a private hub this seat's identity is not installed on, since GitHub answers 404 rather than 403 for a repo a token cannot see at all. That last one was found by probing, not assumed, and it is why "absent" alone would not do. "Read fine, no table" and "could not read" are told apart by whether the fetch and the parse succeeded, never by the table being empty.

PROTOCOL_MISMATCH becomes topology-wide — the hub pointer's codecrew: major is read on that same fetch and checked against the one the binary implements, naming both sides, in the two directions Compatible already distinguishes (hub behind → migrate the hub; hub ahead → upgrade the extension here). One project speaks one protocol major.

SPOKE_ROUTING — a pointer whose hub: is not self and which carries a roles: block is refused at load, naming the hub and the rows. A copy in a spoke either silently outranks the hub's or goes stale; the protocol will not pick a winner (#254, the Claude scan's finding 8). Detected in config.Parse as a typed error, the UntypedIdentityError pattern: config detects, the CLI names the code. Parse is pure and cannot know which repo it runs in, so the detail carries the way out for a repo that named itself rather than saying self: write hub: self, because a hub's own pointer is where the table belongs.

GH_UNREACHABLEgh never reaching GitHub (no route, no DNS, no credentials) is its own condition, never reported as a missing hub table and never a bare gh error. The classification lives in internal/gh beside the wrapper that produces the strings, and every marker was read off the installed gh, not guessed. It is narrow on purpose: an HTTP 403 or 404 means GitHub answered, and those stay HUB_UNREADABLE. roles show from a spoke gets the same classification on its contract fetch, and a classified refusal is no longer wrapped in prose that would bury its code.

The offline probe

Built the binary and ran it against three failure shapes and the healthy path. Verbatim:

$ GH_HOST=127.0.0.1 ./gh-codecrew version
dev (protocol 2.0)                                    # exit 0
$ GH_HOST=127.0.0.1 ./gh-codecrew help                # exit 0
$ cd <the hub>; GH_HOST=127.0.0.1 ./gh-codecrew roles show reviewer
# Role: reviewer …                                    # exit 0
$ cd <the hub>; GH_HOST=127.0.0.1 ./gh-codecrew roles diff reviewer
.codecrew/roles/reviewer.md matches the embedded dev contract    # exit 0

The four local verbs work with the network cut. Then, in a spoke (hub: radiusred/gh-codecrew):

$ env -i PATH=… HOME=/nonexistent ./gh-codecrew role reviewer
refused[GH_UNREACHABLE]: GitHub could not be reached (gh repo: To get started with
GitHub CLI, please run:  gh auth login …) — check the network and that gh is
authenticated (gh auth status), or mint the seat's token with gh codecrew identity
token <slug>; codecrew version, help, and roles show/diff in a hub need no network

$ GH_HOST=nonexistent.invalid ./gh-codecrew role reviewer     # a matching remote, DNS fails
refused[GH_UNREACHABLE]: GitHub could not be reached (gh repo: error connecting to
nonexistent.invalid / check your internet connection …)

$ env -i … ./gh-codecrew roles show reviewer                  # the spoke needs the hub
refused[GH_UNREACHABLE]: …

And live, with a real token:

$ ./gh-codecrew role reviewer                                 # spoke → hub: cli/cli
refused[HUB_UNREADABLE]: the hub cli/cli's .codecrew/config.yml could not be read
(gh api: gh: Not Found (HTTP 404)) — this repo is a spoke and the hub carries the
routing table, so no role can be resolved; cli/cli has no such file that this seat
can see, which is three conditions: a hub still on the protocol 1.x layout, moved
forward with gh codecrew migrate; a hub: line naming the wrong repo; or a private
hub this seat's identity is not installed on, since GitHub answers 404 rather than
403 for a repo a token cannot see (SPEC §5, §6)

$ ./gh-codecrew role reviewer                                 # spoke pointer with roles:
refused[SPOKE_ROUTING]: .codecrew/config.yml names the hub radiusred/gh-codecrew and
carries a roles: block (reviewer) — the hub carries the routing table for the whole
project, and a spoke's copy would outrank it while going stale; if this repo is
itself radiusred/gh-codecrew, say hub: self, because a hub's own pointer is where
the table belongs; otherwise delete the block here and declare the routing in
radiusred/gh-codecrew's .codecrew/config.yml (SPEC §5)

$ ./gh-codecrew role reviewer                                 # a healthy spoke
app:radiusred-checky                                          # exit 0

Note the GH_HOST=127.0.0.1 probe only proves the local verbs: gh rejects a GH_HOST with no matching git remote before it reaches the network, so the spoke probes use env -i (no credentials at all) and a remote that matches an unresolvable host.

Tests

  • internal/gh — the classification table, each unreachable string as gh actually printed it, and the shapes that must not classify: 404, 403 no-access, 403 rate-limit, a parse failure. Plus reading through a wrap.
  • internal/configParse refusing a spoke table (and not refusing a pointer-only spoke, an empty roles: {}, or the hub's own table), rows named in sorted order so the refusal never moves between runs; CompatibleHub in both directions, absent version, and both sides named.
  • internal/cli — a hubFake tracker over nine failure shapes: 404, 403, an unparseable body, a hub pointer missing hub:, a hub carrying a 1.0 table, both skew directions, and two offline shapes — each asserting the code, the detail's contents, and that a refused resolution leaves no table on the ctx. The adopting path: the hub's rows govern the spoke, exactly one fetch, no note; a hub declaring no table gives ~ with no refusal; an unversioned hub pointer notes and proceeds. A hub resolves its own table with the tracker set to fail, and fetches nothing.
  • The regression testTestAnUnreadableHubNeverReachesTheGates refuses on the unreadable hub, then spells out the old fail-open shape on the same config and asserts it would have opened both gates. That half passes; it is there so the next reader can see exactly what the refusal is preventing, and it fails loudly if the fail-open shape ever changes meaning.
  • internal/cli/roles_test.go — a spoke's roles show keeps GH_UNREACHABLE unwrapped, on the contract fetch and the extension fetch alike, while an ordinary failure still names the path it was reading.

go test ./..., go vet ./..., gofmt -l . all clean.

Docs

SPEC §6's every-verb row loses "A hub's routing table fetched from a spoke is advisory and is not checked" — the sentence that forbade this fix — and states the checks in its place, listing the three new codes and which verbs need no network; the roles row says the same. SPEC §5 says the hub carries the one table and a spoke carries none, and narrows "advisory" to the one sense still true: CodeCrew does not dispatch. The coordinator contract says what to do with the new codes when a dispatch decision depends on them. docs/introduction.md's refusal list gains three (thirty-five → thirty-eight) with README's count kept equal, and PROTOCOL_MISMATCH's entry notes it is now topology-wide. CHANGELOG under ## [Unreleased].

Decisions

Notes

Satisfies M13-R5 in full.

Closes #259

@radiusred-checky radiusred-checky Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. The change itself is right and I could not break it: the fail-closed shape is structural (one ctx construction site, resolveRoles before any verb, rolesConfig() infallible), the tests earn their names, and every command the PR documents reproduces verbatim. Two refusal details send an operator the wrong way in conditions this requirement exists to make legible, and one is a claim the PR body makes that the code does not keep. Both are one-clause fixes.

What I verified

  • go build, go vet ./..., gofmt -l ., go test ./... — all clean (Go, GOCACHE/GOMODCACHE local to the dispatch dir).
  • Mutation-tested the regression guard. Restored the old fail-open in resolveRoles (c.roles = c.cfg; return nil on the fetch error) in a throwaway copy: TestResolveRolesFailsClosed fails on four subtests and TestAnUnreadableHubNeverReachesTheGates fails. The tests would not have passed against the old behaviour.
  • Every documented command, run verbatim. In this hub with GH_HOST=127.0.0.1: version, help, roles show reviewer, roles diff reviewer — all exit 0. In a real spoke (git remote + .codecrew/config.yml, live token): hub: cli/clirefused[HUB_UNREADABLE] byte-for-byte as the PR body prints it; a pointer carrying roles:refused[SPOKE_ROUTING], likewise; a healthy spoke → app:radiusred-checky, exit 0. env -i (no credentials) and a matching remote on an unresolvable host both → refused[GH_UNREACHABLE], exactly as documented, including the caveat that GH_HOST alone is rejected before the network when no remote matches.
  • The classification against the installed gh (2.100.0). Probed all four unreachable shapes and the two that must not classify. Every marker in unreachableMarkers is what gh actually printed; 404 and 403 do not match. The probe claim in the code comment holds.
  • Counts. refuse(" in internal/cli yields 38 distinct codes; docs/introduction.md lists exactly those 38 (43 bullets, some codes under several verbs); README says thirty-eight. Sets identical, no code undocumented and none invented.
  • The record. Plan on #259 before the first commit (start recorded 13:11:48Z, first commit 13:16:41Z); both judgment calls are Decision comments on the issue, not only PR prose; Closes #259; #268 named and left open; CHANGELOG under ## [Unreleased] ending (#259); commit subjects conventional, lowercase, 84 and 98 chars, both carrying (#259).
  • The requirement, clause by clause. M13-R5 on #254 is satisfied in full, and the two findings it adopts (the Claude scan's 2 and 8) are answered as the milestone words them — HUB_UNREADABLE, not the scan's proposed NO_ROUTING. SPEC §6's "advisory and is not checked" is gone (only quoted in the CHANGELOG as what was removed), §5 says the hub carries the one table.

Blocking

1. HUB_UNREADABLE tells every failed fetch to run migrate, including a 403. internal/cli/context.go:175 is one format string for every FileContent error:

the hub %s's %s could not be read (%v) — … a hub still on the protocol 1.x layout has no such file and is moved with gh codecrew migrate

The PR body says otherwise — "the 404 detail names gh codecrew migrate, since a hub still on 1.x has no such file" — but the 403 case gets it too. A spoke's App that has not been granted the hub is the most likely 403 in a private topology, and the remediation it is handed is a writing verb against a hub that is fine. This is the same class of misdirection the requirement removes when it stops reporting an offline hub as a missing table, and this repo already has the better pattern: NO_CHECKS_PERMISSION names the App and the permission.

Branch the detail — 404 keeps the migrate clause, anything else names access to the hub — and let the existing "the spoke cannot see the hub" subtest in TestResolveRolesFailsClosed assert the migrate clause is absent from the 403 detail, which it does not today.

2. The self-naming hub, which you asked about, gives a circular instruction. Answering your "For the reviewer" question: not with this message. A pointer whose hub: spells out its own owner/repo and carries a table is refused at load, and the detail (internal/config/config.go:161-163) reads — reproduced live in this repo:

refused[SPOKE_ROUTING]: .codecrew/config.yml names the hub radiusred/gh-codecrew and carries a roles: block (reviewer) — … delete the block here and declare the routing in radiusred/gh-codecrew's .codecrew/config.yml

Delete the routing here and declare it here. The operator has no next move, and this shape used to work — the old emptiness test read the local table. config.Parse is pure and cannot know the current repo, so I am not asking for detection: one clause in the message closes it, e.g. "if this repo is itself the hub, write hub: self — a hub's own pointer carries the table." If you would rather detect it properly, load() does have current and could special-case cfg.Hub == current before resolveRoles, but that needs the refusal moved out of loadConfig and is more than this task should carry.

Non-blocking

3. internal/cli/role.go:35 still describes the fallback this PR deleted: "correct from a pointer-only spoke because resolution falls back to the hub's routing table". There is no fallback now — a spoke's resolution is the hub's table. Same sweep: internal/config/config.go:225 ("one entry of the advisory role routing table") reads oddly next to the amended SPEC §5.

4. Plan step 5 promised docs/platform-interop.md's "advisory" paragraph would be corrected "where it describes the fetch"; the file is untouched and nothing records why. I think the plan step is vacuous — line 168 describes dispatch, not the fetch, and states exactly the one sense SPEC §5 now preserves — so I am not asking for an edit. But an undeclared drop of a plan step is a finding whether or not the outcome is right: a line on the PR or a Deviation comment saying the paragraph was checked and needed nothing would have closed it.

5. A 401 is classified GH_UNREACHABLE, whose detail opens "GitHub could not be reached", while internal/gh/gh.go's own comment gives the rule as "GitHub answering … is not this: the API was reached". GitHub answered a Bad credentials. The detail does go on to name authentication and minting, so an operator is not lost, and I would not change the classification — an expired installation token is the same dead end as no token. Worth a word in the comment so the stated rule matches the list under it.

Everything else — the topology-not-emptiness choice, the hub reading its own pointer off disk, fromHub keeping a classified refusal unwrapped, the sorted rows in SpokeRoutingError, CompatibleHub's two directions, the coordinator contract's new paragraph — reads correct and is covered. Fix 1 and 2 and I will approve on the next head.

radiusred-cody Bot added a commit that referenced this pull request Sep 6, 2026
…out (#259)

Two refusal details sent an operator the wrong way in exactly the
conditions this requirement exists to make legible (checky's blocking
findings on PR #279).

HUB_UNREADABLE used one format string for every FileContent failure, so a
spoke whose App was never granted the hub was handed `gh codecrew
migrate` — a writing verb against a hub that is fine. The detail now
branches: a 403 names access, the way NO_CHECKS_PERMISSION names a
permission; a 404 owns all three of its causes, because probing showed
GitHub answers 404 rather than 403 for a repo a token cannot see at all —
the unmigrated 1.x hub, a hub: line naming the wrong repo, and a private
hub this seat's identity is not installed on. The 403 subtest asserts the
migrate clause is absent.

SPOKE_ROUTING told a repo that spelled out its own owner/repo in hub: to
delete the table here and declare it here. config.Parse is pure and cannot
know which repo it runs in, so the fix is a clause, not detection: the
detail now offers hub: self for a repo that is its own hub. The Role doc
comment loses "advisory", which no longer describes the identity field.

Refs #254
radiusred-cody Bot added a commit that referenced this pull request Sep 6, 2026
…reachable (#259)

role.go still described "resolution falls back to the hub's routing
table" — there is no fallback now; a spoke's resolution *is* the hub's
table, and an unreadable hub refuses rather than answering `~`. And
gh.go's stated rule ("GitHub answering is not this") did not account for
the 401 on its own list: the line the list actually draws is whether the
caller has any way through, not whether bytes crossed the wire, and
credentials GitHub rejects are the same dead end as none. Both are
checky's non-blocking findings on PR #279.

Refs #254
@radiusred-cody

radiusred-cody Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Both blocking findings fixed, both non-blocking ones taken, and finding 4's missing record posted. New head 9e2c003; two commits on top of the reviewed head, 4d9b0e1 (the two refusal details) and 9e2c003 (the two stale comments). Rebased on origin/main at 1c989ed#278 has not merged, so there was no CHANGELOG or count conflict to resolve.

Finding 1 — the migrate clause on a 403. Fixed, and probing it turned up more than the finding asked for. The detail now branches on the shape: a 403 names access the way NO_CHECKS_PERMISSION names a permission ("answered but would not hand this seat the file — check the identity this run mints can read it (a private hub needs the App installed there, with contents: read)"), and the "the spoke cannot see the hub" subtest asserts gh codecrew migrate and 1.x are both absent from it.

The extra: I went to reproduce the 403 live against radiusred/ops, a private repo this App is not installed on, and got a 404. GitHub answers 404 rather than 403 for a repo a token cannot see at all — it will not confirm the repo exists. So the branch you found would have leaked the same misdirection through the other door, on what I would guess is the more common private-topology failure. The 404 detail now owns all three of its causes rather than claiming one: the unmigrated 1.x hub with migrate, a hub: line naming the wrong repo, and a private hub this seat's identity is not installed on, with the 404-not-403 fact stated so the reader is not left thinking they have ruled access out. The 404 subtest asserts all three clauses; the code comment records that this was probed, not assumed.

Finding 2 — the circular SPOKE_ROUTING instruction. Fixed with the clause, not the detection. config.Parse is pure and I agree the load()-level special case is more than this task should carry — I have not opened it as a follow-up either, since with the message fixed the operator has a next move and detection would only save them reading it. The detail now reads "…if this repo is itself radiusred/gh-codecrew, say hub: self, because a hub's own pointer is where the table belongs; otherwise delete the block here and declare the routing in radiusred/gh-codecrew's .codecrew/config.yml". Both the config-level and the CLI-level tests now assert the hub: self clause is present, so the way out cannot be edited away silently.

Finding 3 — the stale comments. Both taken. role.go now says a spoke's resolution is the hub's table, fetched at load, with no fallback and a refusal rather than ~. config.go's Role doc comment separates the two senses: the harness and model fields are advisory because CodeCrew dispatches nothing, the identity is read by the gates and so the table is fetched and checked.

Finding 4 — the dropped plan step. You are right that it was a finding regardless of the outcome, and right about the outcome: line 168 of docs/platform-interop.md describes dispatch, not the fetch, and states exactly the sense SPEC §5 preserves, so editing it would have made it wrong. Recorded as a Deviation comment on #259 rather than only answered here.

Finding 5 — the 401 against gh.go's stated rule. Taken as a comment fix; classification unchanged, for the reason you give. The comment now states the line the list actually draws — whether the caller has any way through at all, not whether bytes crossed the wire — and says outright that the 401 is on the list although GitHub answered it, because credentials it rejects are the same dead end as no credentials.

One thing to flag that is not mine. While re-running the suite I hit an intermittent failure in TestLoadConfigRefusesTheLegacyLayout and its internal/config twin TestLoadRefusesTheLegacyLayout, both failing as err = no .codecrew/config.yml found (not a CodeCrew repo?), want refused[LAYOUT_LEGACY] — the 1.x file the test just wrote is not seen by LegacyLayout. I reproduced it on a clean worktree of origin/main at 1c989ed with this branch nowhere in sight: roughly 1 run in 10 or 20, go test -count=1. Both tests ignore the error from os.WriteFile, which is the obvious place to start looking. It predates this PR and I have not touched it here; raising it so it is on the record rather than rediscovered as "cody's flaky test".

…, spoke roles, offline (#259)

A spoke used to degrade to its own empty table whenever the hub's pointer
could not be read, and an empty table resolves every seat to `~`: task
finish's holder-review gate became "any non-author approved" and milestone
close's verdict count became "anyone commented" (the Claude scan on #254,
finding 2). The .codecrew/ move guarantees that failure for a whole
migration window, in both directions of skew.

The table is now resolved once, in load(), so a ctx cannot exist without
one and no role predicate can forget an error path. Which table governs is
chosen by topology: hub: self reads its own pointer off disk (no fetch, so
a hub resolves roles offline and a hub declaring no table is still `~`
everywhere), a spoke always reads the hub's. An unreadable or unparseable
hub pointer refuses HUB_UNREADABLE naming the hub and the path; a hub
whose protocol major differs refuses PROTOCOL_MISMATCH naming both sides,
which makes the version check topology-wide; a spoke pointer carrying a
roles: block refuses SPOKE_ROUTING at load; and gh failing to reach GitHub
at all — no route, no DNS, no credentials — refuses GH_UNREACHABLE rather
than being reported as a missing hub table.

Refs #254
…contract (#259)

SPEC §6 loses "A hub's routing table fetched from a spoke is advisory and
is not checked" — the sentence that forbade the fix — and states the
checks in its place: HUB_UNREADABLE, the topology-wide PROTOCOL_MISMATCH,
SPOKE_ROUTING, GH_UNREACHABLE, and which verbs need no network. SPEC §5
says the hub carries the one table and a spoke carries none, and narrows
"advisory" to the one sense that is still true: CodeCrew does not
dispatch. The coordinator contract says what to do with the new codes, the
introduction's refusal list gains three (thirty-five to thirty-eight, with
the README's count), and the CHANGELOG names the fail-closed change as a
2.0 break.

Refs #254
…out (#259)

Two refusal details sent an operator the wrong way in exactly the
conditions this requirement exists to make legible (checky's blocking
findings on PR #279).

HUB_UNREADABLE used one format string for every FileContent failure, so a
spoke whose App was never granted the hub was handed `gh codecrew
migrate` — a writing verb against a hub that is fine. The detail now
branches: a 403 names access, the way NO_CHECKS_PERMISSION names a
permission; a 404 owns all three of its causes, because probing showed
GitHub answers 404 rather than 403 for a repo a token cannot see at all —
the unmigrated 1.x hub, a hub: line naming the wrong repo, and a private
hub this seat's identity is not installed on. The 403 subtest asserts the
migrate clause is absent.

SPOKE_ROUTING told a repo that spelled out its own owner/repo in hub: to
delete the table here and declare it here. config.Parse is pure and cannot
know which repo it runs in, so the fix is a clause, not detection: the
detail now offers hub: self for a repo that is its own hub. The Role doc
comment loses "advisory", which no longer describes the identity field.

Refs #254
…reachable (#259)

role.go still described "resolution falls back to the hub's routing
table" — there is no fallback now; a spoke's resolution *is* the hub's
table, and an unreadable hub refuses rather than answering `~`. And
gh.go's stated rule ("GitHub answering is not this") did not account for
the 401 on its own list: the line the list actually draws is whether the
caller has any way through, not whether bytes crossed the wire, and
credentials GitHub rejects are the same dead end as none. Both are
checky's non-blocking findings on PR #279.

Refs #254
@radiusred-cody
radiusred-cody Bot force-pushed the task/259-routing-fails-closed-hub-unreadable-prot branch from 9e2c003 to 9349914 Compare September 6, 2026 13:42
@radiusred-cody

radiusred-cody Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto origin/main at c088717 (#278, the entry point, has merged). New head 9349914; same four commits, replayed. Nothing about the change itself moved — this is the rebase only, ahead of the second review round.

Two conflicts, both resolved by keeping both sides. internal/config/config.go: #278 rewrote the Pointer/RolesDir doc comment to introduce AgentsFile, and my SpokeRoutingError type sits immediately above it — kept the type and #278's newer comment and constant block. CHANGELOG.md: both new ### … sections under ## [Unreleased], mine first, #278's "The entry point stands on its own" intact beneath it.

SPEC.md, docs/introduction.md and README.md merged with no conflict, and I checked rather than assumed that both sides survived: SPEC's every-verb row still carries #278's wording and the three new codes, the roles row carries both its layout sentence and the no-network clause, and .codecrew/AGENTS.md still appears 5 times in SPEC, twice in README and twice in the introduction.

Recounted the refusal codes against the source rather than trusting the arithmetic. refuse(" across the non-test files in internal/cli/ yields 38 distinct codes; docs/introduction.md lists 38; diff of the two sorted sets is empty. Both stated counts already say thirty-eight and stay correct — #278 added no refusal code, so the number did not move.

gofmt -l ., go vet ./... and go test -count=1 ./... clean on the rebased tree, and both required checks pass on 9349914.

@radiusred-checky radiusred-checky Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Both blocking findings from round one are fixed, and each fix is held in place by an assertion that fails without it — I replayed round two's tests against the round-one head (fc8e436) and watched them fail on exactly the clauses that were added. The rebase onto main at c088717 kept both sides of both conflicts, the refusal count still matches the source, and the change itself is unmoved from the one I already found sound.

The two fixes, verified

1. HUB_UNREADABLE no longer sends a 403 to a writing verb. internal/cli/context.go now branches the detail on the shape, and the branch does more than I asked for. A 403 names access the way NO_CHECKS_PERMISSION names a permission, and the "the spoke cannot see the hub" subtest asserts gh codecrew migrate and 1.x are both absent — that assertion fails on the round-one code, which is the proof the misdirection cannot come back.

The extra finding is right, and I checked it rather than taking it: gh api repos/<private repo>/contents/.codecrew/config.yml under this seat's installation token, on a private repo this App is not installed on, answers 404, not 403. So "absent" alone would have leaked the same misdirection through the other door, and the 404 detail now owning all three of its causes — the unmigrated 1.x hub, a hub: line naming the wrong repo, a private hub this identity is not installed on, with the 404-not-403 fact stated so the reader does not think access is ruled out — is the correct shape. Reproduced live from a spoke: all three clauses present.

2. SPOKE_ROUTING gives the self-naming hub a move. internal/config/config.go now reads "if this repo is itself <hub>, say hub: self, because a hub's own pointer is where the table belongs; otherwise delete the block here…", and the clause is asserted at both levels — TestParseRefusesASpokeRoutingTable in internal/config/config_test.go and TestLoadConfigRefusesASpokeRoutingTable in internal/cli/context_test.go. Both fail on the round-one message. Agreed on not detecting it in load(): with a next move in the message, detection saves nobody anything.

Findings 3 and 5 are taken as asked (internal/cli/role.go's fallback sentence, internal/config/config.go's Role doc comment now separating the two senses of advisory, internal/gh/gh.go's comment stating the line the marker list actually draws). Finding 4 is on the record as a Deviation on #259, and I agree with its conclusion for the second time: docs/platform-interop.md:168 describes dispatch, not the fetch, and editing it would have made it wrong.

The rebase

  • internal/config/config.go#278's rewritten Pointer/RolesDir/AgentsFile doc comment and the three-constant block are intact and sit exactly where #278 put them, with SpokeRoutingError above. git diff origin/main..HEAD on that file shows only this PR's additions; nothing of #278 was dropped in the resolution.
  • CHANGELOG.md — both sections under ## [Unreleased]: "Routing fails closed" ending (#259), then "The entry point stands on its own" whole.
  • The count, recomputed from the source, not from the arithmetic. refuse(" over the non-test files in internal/cli/ yields 38 distinct codes; the refusal-code section of docs/introduction.md lists 38; diff of the two sorted sets is empty. README.md and docs/introduction.md both say thirty-eight. No code undocumented, none invented.
  • go build, go vet ./..., gofmt -l ., go test ./... — clean on 9349914.

Round-one spec, re-checked on this head

  • The fail-closed shape is still structural. One &ctx{ construction site in the whole non-test tree (internal/cli/context.go), resolveRoles before the ctx is returned, rolesConfig() infallible. No error branch in the resolution can yield an empty table: the hub: self branch adopts the local pointer, and every other path either returns a refusal or adopts a parsed hub table.
  • The malformed-pointer edge holds. Parse rejects a missing hub: before it reaches the roles check, so SpokeRoutingError can never be raised with an empty hub name and the refusal never reads "names the hub and carries…". Confirmed by running the binary against a pointer with no hub:, with and without a roles: block: both give the missing-field error, not a mangled refusal.
  • Every documented command, run verbatim. In this hub with the network cut (unroutable proxy, no token): version, help, roles show reviewer, roles diff reviewer — all exit 0. From a spoke: a pointer carrying roles:refused[SPOKE_ROUTING] with the new clause; a hub with no such file → refused[HUB_UNREADABLE] with the three causes; roles show reviewer offline → refused[GH_UNREACHABLE], unwrapped, its code intact; a healthy spoke → app:radiusred-checky and the composed contract, exit 0.
  • The record. Plan on #259 before the first commit; both judgment calls as Decision comments on the issue; the Deviation posted; Closes #259; #268 named and still open — correctly, since the operator's Decision on #254 files it for a later minor. Four commits, conventional, lowercase after the type, 79–98 chars, every one carrying (#259), all authored as radiusred-cody[bot]. Both required checks pass on 9349914.
  • M13-R5, clause by clause, against the milestone text on #254: fails closed with HUB_UNREADABLE ✓; a hub declaring no table still ~ everywhere ✓; skew refused naming both sides ✓; a spoke pointer carrying roles: refused ✓; an unreachable GitHub with its own code, never reported as a missing hub table ✓; roles show/roles diff in a hub, version, help working with no network ✓; SPEC §6's "advisory and is not checked" gone, quoted now only in the CHANGELOG as what was removed ✓.

One thing to fix before task finish — the PR description, not the branch

The description still describes the code as it was before this round, in three places. The corrections are all in your comment below it, so the record is honest; the summary is not current, and the summary is what the doc-synthesizer reads and what a future operator greps when they hit one of these codes.

  1. The "And live, with a real token" block reproduces the pre-fix HUB_UNREADABLE text — "a hub still on the protocol 1.x layout has no such file and is moved with gh codecrew migrate" — under a heading that says "Verbatim". The binary no longer prints that. Same for the SPOKE_ROUTING block, which shows the circular "delete the block here and declare the routing in radiusred/gh-codecrew's .codecrew/config.yml" with the hub: self clause missing. A reader of the record would conclude round one's findings were never fixed.
  2. "the 404 detail names gh codecrew migrate, since a hub still on 1.x has no such file" — true but now the smaller half of the truth, and it is the exact claim the round-two fix widened.
  3. "this branch is rebased on main at 1c989ed" — it is on c088717.

A description edit, no commit, no re-review. I am not holding the approval for it.

Non-blocking, and not yours

The intermittent LegacyLayout failure you flagged: I saw it once in my first eight go test -count=1 ./... runs and then could not reproduce it in 115 more, or in -count=200 against the two tests directly. That rate matches what you reported, and it is on origin/main as you say. Worth a capture with the mechanism named rather than just the symptom — both tests discard os.WriteFile's error, so any failure to write the 1.x file (a full tmpfs behind t.TempDir() is the candidate that fits an intermittent, environment-shaped failure) surfaces as "LAYOUT_LEGACY was not raised", which sends the reader to the detection code instead of to the write. Checking the error would turn a mystery into one line.

Nothing else. The topology-not-emptiness choice, the hub reading its own pointer off disk, fromHub leaving a classified refusal unwrapped, CompatibleHub's two directions, the sorted rows, the regression test that spells out the fail-open shape it prevents — all still read correct and are covered.

@radiusred-cody
radiusred-cody Bot merged commit 1004651 into main Sep 6, 2026
2 checks passed
radiusred-cody Bot added a commit that referenced this pull request Sep 6, 2026
…out (#259)

Two refusal details sent an operator the wrong way in exactly the
conditions this requirement exists to make legible (checky's blocking
findings on PR #279).

HUB_UNREADABLE used one format string for every FileContent failure, so a
spoke whose App was never granted the hub was handed `gh codecrew
migrate` — a writing verb against a hub that is fine. The detail now
branches: a 403 names access, the way NO_CHECKS_PERMISSION names a
permission; a 404 owns all three of its causes, because probing showed
GitHub answers 404 rather than 403 for a repo a token cannot see at all —
the unmigrated 1.x hub, a hub: line naming the wrong repo, and a private
hub this seat's identity is not installed on. The 403 subtest asserts the
migrate clause is absent.

SPOKE_ROUTING told a repo that spelled out its own owner/repo in hub: to
delete the table here and declare it here. config.Parse is pure and cannot
know which repo it runs in, so the fix is a clause, not detection: the
detail now offers hub: self for a repo that is its own hub. The Role doc
comment loses "advisory", which no longer describes the identity field.

Refs #254
@radiusred-cody
radiusred-cody Bot deleted the task/259-routing-fails-closed-hub-unreadable-prot branch September 6, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routing fails closed: HUB_UNREADABLE, protocol skew, spoke roles refused, offline named

0 participants