Review and Fix Repository View Authority

I reviewed trail #1392 at head aedf0ac2, which is 27 commits touching 17 files against main. CI is green, the trail has no agent findings, and the only failing gate is approvals. I found four things worth fixing before you approve, and one is a real bug. I didn't edit anything or post any findings.

1. repo view --json replaces a field the server already sends (bug)

nativeRepoViewJSON (repo_native_mirror.go) starts from the server's repo record and adds the view's own keys. Its comment says "Nothing here overwrites a server value", but the record already has a placements field, a list of entries with id, cell, clusterSlug, mirror and status. So the output depends on the data:

• When the view finds placements, it replaces the server's list with its own entries, which have cluster, clusterSlug, role, status and cloneUrl.
• When it finds none, the key is left alone and the server's list comes through as-is.

A script reading .placements[].cluster gets a host on one repo and nothing on another. The test "placements is omitted when nothing holds the repo" only passes because its fixture has no placements. Fix: build the output from the row shape and copy only the record fields you want to keep (e.g. capabilities), or give the view's list a key the server doesn't use. Then add a test whose fixture includes a server placements list.

2. The comment about Ctrl-C covers a case the code doesn't handle

In runNativeRepoView, the ctx.Err() != nil branch's comment says it fixes the case where the readiness read succeeds just as the user interrupts. The switch checks aerr == nil first, so that case still prints the table and exits 0. TestRepoView_AnInterruptedReadinessReadIsNotSwallowed only covers a read that fails. Fix: check ctx.Err() before the switch, or remove that paragraph from the comment.

3. Code and docs still mention the --project flag repo view dropped

repo view no longer defines --project, so it's now an unknown-flag error. But:

• warnFlagsGitHubViewIgnores still loops over projectFlagName, and that check can never fire. Its doc comment and cli-conventions.md both say /gh/ refs warn about an ignored --project.
• The comment on warnRedundantProjectFlag still says repo view is the one command bound to that flag that already fetches the repo.
• The comment above the name fallback in runNativeRepoView says "repo view also takes a ULID and a bare name" and "A ULID ref looked nothing up". The function's header comment, a few lines up, says neither form is accepted.

Fix: drop projectFlagName from the loop and update those three comments and the doc paragraph.

4. The recovery hint after repo create can name a command that fails

When the create response has no path, repoViewRef falls back to the bare repo name. The printed hint entire repo view web --authoritative is then refused, because repo view only takes /et/… or /gh/…. The comment admits "it is not a working ref". It only happens briefly after a create, but it's the hint we show exactly when readiness wasn't confirmed. Consider printing only the ID for support in that case, rather than a command we know will fail.

Smaller points

• reportNativeMirrorNotes: the else fmt.Fprintln(w, reason) branch can't run. The outer check already requires a cluster slug, and placementCluster never returns "" for one.
• Every repo view of a native repo now makes two repo reads, and without --authoritative it prints a stderr warning whenever the server can't answer readiness. That's intended, but it's new noise for scripts that used to get a silent dash.

What I checked and found fine

• The authority check against the raw text in parseEntireCloneURL. The user@evil.com and uppercase-scheme cases are tested.
• .git handling: dropped from /gh/ refs, kept as part of /et/ names.
• Lowercasing of /gh/ refs, same as the old parser.
• validateClusterFilter, including a cluster with no usable public host.
• Placement sorting now matches between the native and GitHub views.
• Visibility has three states: when the server doesn't say, private is left out rather than shown as public.
• The repo mirror get and repo view <ULID> removals, which match your no-backcompat rule for control-plane renames.

Want me to post these as trail findings (entire trail finding add)?

I sent the repo-view session the four findings from the review plus the unreachable else branch in reportNativeMirrorNotes. Each one names the file, the fix, and the test to add where one is missing. I also told it to run mise run check before committing, mise run lint before pushing, and to check the trail's findings after the push.

The message is queued there. It hasn't started working on it yet: that session was idle, and a delivery notice will show up here if it needs its user's approval first or refuses the message. I didn't subscribe to a notice for when it finishes; I can if you want one.

I reviewed the new commit, 02f5a30b, which went through the repo-view session. All five items from my last review are fixed, but the trail is still blocked: a new high-severity finding is open, and CI was still running (12 of 16 checks passed, 4 still going).

Checked fixes:

1. placements in --json: the view's list now replaces the server's; when the view has none, the key is removed. Two new tests use a fixture that includes the server's own list, which covers the gap I flagged.
2. Ctrl-C: the cancellation check now runs before the switch, and a new test covers a read that succeeds just as the cancel lands.
3. Dropped --project: only --authoritative is checked now; the three stale comments and the doc paragraph are rewritten. The doc also now describes --json correctly: the server's record plus the view's own keys.
4. Hint after repo create: with no path, the repo view lines are dropped and only the repository ID is printed. This one has no test. Nothing checks the no-path output of either the wait-failed hint or the --no-wait hint, so it's worth asking for one.
5. The unreachable else branch is gone.

New open finding 01M3PWZYTGD4 (high): it says parseEntireCloneURL checks the authority text as typed but returns u.Host, which comes from a separate parse. I ran 15 hostile inputs through the parser in a throwaway worktree (userinfo, ?, #, %40, backslash, uppercase scheme, IPv6, empty host). In every accepted input the returned host equals the authority that was checked, and every @evil.com form is rejected. So it can't be exploited today. Still, returning authority is a one-line change that makes the code obviously correct, and it's cheaper than defending the current form. Once that lands, resolve the finding with -m.

The probe also showed entire://c.entire.io:/gh/a/b is accepted with an empty port, returning host c.entire.io:. That comes from the older host check, validateClusterHost, not from this PR.

Should I send repo-view the one-line authority fix and the missing no-path test?

I sent both follow-ups to the repo-view session:

1. Finding 01M3PWZYTGD4: return the checked authority instead of u.Host, then resolve the finding with a -m note citing the probe results.
2. Missing test for the hint after repo create: cover a create response with no path, both when readiness wasn't confirmed and with --no-wait. The test should check that stderr has no repo view line and does include the repository ID.

The message is queued; that session hasn't picked it up yet. I asked to be told when it next goes idle, and I'll re-review the new commits and the trail's findings then.

The repo-view session says both follow-ups are done and its pre-commit check (mise run check) is running. Nothing has been pushed yet: the trail head is still 02f5a30b, and finding 01M3PWZYTGD4 is still open. CI passed on the current head, 17 of 17 checks.

I've asked to be told when it next goes idle, and I'll re-review once the new commit and the finding resolution land.

Nothing is pushed yet. The repo-view session says lint is clean and it's running the full pre-commit check (mise run check) again before it commits. I've asked to be told when it's next idle, and I'll re-review once the new commit is on the trail.

That notice repeated the previous one, and repo-view still hasn't pushed; the branch is still at 02f5a30b. I've asked to be told when it's next idle, and I'll re-review once the commit lands.

That's the same 17:45 notice for the third time, so repo-view hasn't moved since then. I checked the branch after the previous one and it was still at 02f5a30b. I haven't subscribed again, because each new subscription just replays that same notice.

Tell me when repo-view says it has pushed, or just say "review again", and I'll check the new commit, the added test, and whether finding 01M3PWZYTGD4 is resolved.

Both follow-ups are written but not pushed. They're staged, uncommitted, in this same worktree; the repo-view session works on this checkout too. The trail head is still 02f5a30b, and I didn't commit or change anything.

Staged change, checked:

• Finding 01M3PWZYTGD4: parseEntireCloneURL now returns the checked authority, with a comment noting that behaviour is unchanged. My earlier probe backs that up.
• Missing test: TestReportRepoCreationWithoutAPath covers a create response with no path in both cases, readiness not confirmed and --no-wait with a non-active state. It checks that no entire repo view line is printed, the repository ID is, and the exit code is right. A third case checks that the hint is printed when a path exists, so the test fails if someone removes the hint altogether.
• One nit: in that test, the NotContains "repo view web" assertion can't fail on its own, because the line before it already rules out any entire repo view. It's harmless.

I have nothing else to raise, so once this is committed and pushed the review is clear.

Still to do before this trail can merge:

1. repo-view commits and pushes; per its last notice it was re-running mise run check.
2. Finding 01M3PWZYTGD4 gets resolved with -m once the fix is on the trail.
3. Someone approves the trail; approvals is the one gate that has never passed.

I couldn't read the trail's gates or findings this time because your Entire login for us.auth.entire.io has expired. Run ! entire login and I'll confirm the finding is cleared after the push.

I re-reviewed the push, 388672b0, and it's clear. The only gate still failing is approvals.

• Commit: it's the change I reviewed while it was staged: parseEntireCloneURL returns the checked authority, and there's the new TestReportRepoCreationWithoutAPath. It touches the same 2 files (+70/−1), nothing else changed, and nothing is left staged in the worktree.
• Trail gates: CI passed (17 of 17), and the findings gate passed with no open findings, so 01M3PWZYTGD4 is cleared. Approvals is the one still failing; no reviewer has approved yet.

Every item from my three passes is fixed, and I have nothing new to raise. What's left is a human approval, which has to come from a reviewer other than you, since you authored the trail.

Mostly a merge from main. Next: isolating the branch's own changes.

I re-reviewed the trail at head 7f5218e7, which adds two branch commits plus a merge from main since my last pass. The merge has no conflict resolutions, and the trail shows no open findings. I found one bug and that the newest commit's four fixes have no tests. I didn't edit anything or post any findings.

90d42101: repo list now prints a name you can act on

• What's good: NAME is now the /et/<project>/<repo> path, so every cell can be pasted into repo view again. The columns and colours match repo mirror list. The new resolveProjectRefNamed adds a GetProject call only when --project is a ULID, and the test covers that case.
• Dead code: styleTableWith (corecmd.go:527) now has no production caller, and its own comment says it is "reached only by its own tests". Delete it and its tests, rather than keeping it on the chance it's needed later.
• Stale comment: repo.go:271 still says "(see preStyleTable)", but this commit deleted that function.
• Scope: changing repo list's columns, and the header in mirror list, is a second objective in this trail. It follows from the repo view change, so I'd leave it in, but call it out in the trail description, which is still empty.

7f5218e7: the table now names one cluster per row

The reasoning is sound: the CLUSTER cell and the clone URL now both read one hostOf, with the repo record as the primary's fallback, which matches what repo clone does. The --authoritative=false fix and the reworded hint are correct.

Bug: the stderr note still names a different cluster than the table. In reportNativeMirrorNotes, the failure-reason line for the primary still uses placementCluster(hostBySlug, …), which has no record fallback. So a failed repo whose cluster is missing from /clusters shows aws-us-east-2.entire.io in the table and us-east: max retries exhausted on stderr. That's the same mismatch this commit fixed in the table. The primary's line should use the same hostOf/clusterCell logic.

None of the four fixes is tested. In a throwaway worktree I reverted each fix on its own and ran the repo-view, mirror and readiness tests; they still passed every time:

The only new test covers the --authoritative note on GitHub repos. The existing TestRepoView_AuthoritativeRefusesAStatelessAnswer checks the flagless case for a zero exit only, not for the new warning.

Still to do

The stderr cluster name, tests for the four fixes, removing styleTableWith, and the stale comment. After that, only approvals is left; CI passes (17 of 17) and the trail shows no findings.

Want me to send these to repo-view?

I sent repo-view these five items:

1. Bug: the stderr failure-reason line should name the primary's cluster the same way the table does now.
2. Test gap: a test for each of the four fixes that passed with the fix reverted, each one failing when its fix is removed.
3. Cleanup: delete styleTableWith and its tests.
4. Cleanup: fix the stale "(see preStyleTable)" comment at repo.go:271.
5. Trail description: fill it in, and call out the repo list column change.

It's queued there; I didn't ask to be notified when it's done, since those notices kept repeating last time. Say "review again" once it has pushed.

The new commit, 960fbd44, fixes everything from my last review and I have nothing new to raise. CI was still running when I checked (8 of 15 passed, none failed), and the agent review of this head hadn't finished.

• Bug fixed: reportNativeMirrorNotes now reads cluster names from the row the table was built from. The stderr reason line can no longer name a different cluster than the table.
• Tests: in a throwaway worktree I reverted each fix on its own and ran the tests. All five reverts now fail:

My first run showed four of these passing, but only because my test filter didn't match the new test's name. Rerun with the right filter, all five fail.

• Cleanup: styleTableWith and its tests are deleted, and the stale preStyleTable comment is gone.
• Trail description: it's now filled in, with the repo list column change listed as a second user-visible change.

One optional nit: placements in the row now carry lastError, so reportNativeMirrorNotes could loop over row.Placements and drop its mirrors parameter. That isn't worth blocking on.

Once CI and the agent review pass, the only gate left is approval from a reviewer other than you.

I sent repo-view the one remaining nit. reportNativeMirrorNotes should take everything it prints from the row, which already carries each placement's cluster name and lastError. That means dropping its mirrors parameter and the name map it builds. The output must stay exactly the same, and the existing tests should pass without edits.

It's queued there. Say "review again" once it has pushed.

I reviewed the conflict resolution; it's staged in this worktree and the merge isn't committed yet. The resolution is correct, and I found one stale doc line plus one optional nit.

85af74fa (the nit from last time): reportNativeMirrorNotes now takes everything it prints from the row, and the mirrors parameter and the name map are gone. The only change in output is that the notes now come out in the table's order, which the commit documents and a test covers.

Merge with main (#2630, case-insensitive .git handling):

• cmd/entire/cli/repo_mirror.go: it keeps the branch's parseEntireCloneURL and drops main's edit to the old parseMirrorCloneURL, which this branch deleted. It also removes the gitremote import that only main's edit used. Nothing is lost: the branch's parser passes the path to the shared ref parsers, so main's case-insensitive .git rule applies to clone URLs too.
• cmd/entire/cli/repo_mirror_test.go: it adopts main's .GIT and .Git cases and adds the matching /et/ cases.
• repo view --help and the conventions doc already describe .git as dropped on both forges.
• Verified on the staged tree: go build ./..., go vet, and go test ./cmd/entire/cli/ ./docs/... all pass. That's my own run, not the full mise run check.

Fix before committing:

• docs/development/cli-conventions.md:494: main's new paragraph lists parseMirrorCloneURL among the parsers that drop .git, but this branch deleted that function. Change it to parseEntireCloneURL. The local-doc-link test doesn't check names inside code spans, so nothing caught this.

Optional nit:

• cmd/entire/cli/repo_mirror_test.go:2273: the comment calls repo view "the verb that replaced mirror get". That describes history rather than what the test checks; "the grammar of repo view" says the same thing.

After the merge is committed and pushed, the trail's "conflicting" status should clear. Approval would then be the only gate left.

Want me to send the doc fix, and the nit, to repo-view?

I sent repo-view both items:

• Doc fix: cli-conventions.md:494 should name parseEntireCloneURL instead of the deleted parseMirrorCloneURL.
• Test comment: the comment at repo_mirror_test.go:2273 should describe repo view's grammar, not that it replaced mirror get.

Either can go into the merge commit or a follow-up. The message is queued there. Say "review again" after the merge is pushed.

I re-reviewed the trail at head c9d1df71. Both items from my last review are fixed, the conflict with main is gone, and one new low-severity finding is being fixed but isn't committed yet. CI and the agent review were still running when I checked (5 of 15 checks passed, none failed).

• Merge commit 20e08968: it changes the same two files by the same line counts (−18, and +18/−20) as the staged resolution I reviewed, so it's the resolution I checked.
• c9d1df71: the conventions doc now names parseEntireCloneURL, and the test comment says "the grammar of repo view".

New trail finding 01M4666Q5FST (low): the comment at repo.go:468 says a bare pair like acme/web "is answered with BOTH spellings". That reads as if the pair is accepted, but it's actually refused, with both /et/ and /gh/ forms suggested. There's an uncommitted fix for it in this worktree, presumably from repo-view, rewording it to "a bare acme/web is REFUSED — with both spellings suggested". That reads correctly.

• The same wording appears in a second place: the test comment c9d1df71 just rewrote, at repo_mirror_test.go:2274, still says "a bare pair is answered with both spellings". It's worth changing in the same commit so the finding's point isn't left half-fixed.

Once that commit lands and the finding is resolved with -m, only approval is left.

You're right. The open medium finding 01M466P8CAQE is real: a semantic conflict with main that unit tests can't catch.

What's broken: main's af42b1be ("make a leaked native mirror recoverable") added sweepNativeMirrors in e2e/controlplane/sweep_test.go. It calls repo mirror get <path> --json, which this trail deletes, and it keeps only placements with role "native_mirror", which this trail renames to "mirror". After this merges:

• The mirror get call fails with an unknown-subcommand error, and sweepList treats that as a skip, so no mirrors are cleaned up.
• Even if the call worked, the role check would match nothing.

Either way, a leaked native mirror stays put, and the server won't delete its repo, project or org until it's gone. Per the sweep's own comment, that permanently takes one of the test account's three org slots. CI doesn't catch it: the file only builds with the e2e tag, and the verb is spelled as separate arguments, so the guard that bans the old command string (deprecated_strings_test) never sees it.

The fix is small: the sweep gets repo.Path from repo list --json, which is the /et/<project>/<repo> form repo view accepts.

• sweep_test.go:95: "repo", "mirror", "get", repoPath, "--json" → "repo", "view", repoPath, "--json"
• sweep_test.go:99: p.Role != "native_mirror" → p.Role != "mirror"
• The clone-URL host lookup can stay. p.Cluster is now the host too, but it falls back to the slug for clusters with no usable host, so the clone URL is the safer source.
• To check it, run go vet -tags e2e ./e2e/controlplane/. Running the sweep for real needs the control-plane e2e suite, which you'd have to ask for explicitly.

Also done: the low finding is fixed in 95c420fe, and both comments now say a bare pair is refused. Once it no longer shows as open, the medium finding is the only thing blocking the findings gate. CI was still running when I checked (12 of 16 passed, none failed).

Want me to send this fix to repo-view?

I sent repo-view the fix for finding 01M466P8CAQE:

• Point the cleanup sweep at repo view instead of the deleted repo mirror get.
• Match the renamed role "mirror" instead of "native_mirror".
• Grep e2e/ for any other old spellings that came in with the merge from main.
• Check it with go vet -tags e2e ./e2e/controlplane/, without running the real control-plane suite.
• Resolve the finding with a -m note.

It's queued there. Say "review again" once it has pushed.