feat(advisor): resolve --repo by name via the connected-repos API#806
Conversation
`actual advisor --repo <value>` now accepts a connected repository's name
in addition to a UUID. A value that parses as a UUID is used directly, as
before. Any other value is resolved to a repo id by listing the
organization's connected repositories (GET /v1/connected-repos) and
matching on name, or on `owner/name` to disambiguate a name shared across
owners. An unrecognized or ambiguous name fails with a clear message that
lists the repositories to choose from.
- api: add `ActualApiClient::list_connected_repos` plus the
`ConnectedRepository` / `GetConnectedReposResponse` types. This endpoint
returns a flat `{error, message}` error body, so it gets its own error
mapper, distinct from the advisor routes' nested `{error:{code,message}}`.
- error: add `RepoNotFound`, carrying the listed-repositories message and
an actionable fix hint.
- advisor: resolve `--repo` before building the query. A 403 during the
lookup gets the same cross-org guidance as the query itself.
Per-file line coverage stays at 100%.
Generated by the operator's software factory.
City: factory-main · Agent: local-core.builder-1
On behalf of: @benw5483
Co-Authored-By: <operator-factory-bot> <factory-bot@<operator-domain>.invalid>
backlineint
left a comment
backlineint
left a comment
There was a problem hiding this comment.
Nice, tightly-scoped change with unusually thorough error-path coverage. Three suggestions, none blocking:
-
Case-sensitive name matching (
resolve_repo_name, src/cli/commands/advisor.rs). GitHub owner/repo names are case-insensitive in practice, but the match uses==. A user typing--repo Actual-CLIfor a repo namedactual-cligets a confusing "no match" error that then lists the very repo they typed. Suggested fix: compare witheq_ignore_ascii_case(still surfacing ambiguity if two repos differ only by case). -
Required-but-unused
urlfield (ConnectedRepository, src/api/types.rs).urlis never read (onlyname,external_owner, andrepo_unique_idare used), yet it's a required field — a server response that omits or renames it fails the entire deserialization and breaks name resolution. Either drop the field or give it#[serde(default)]. -
Static hint reads wrong for an empty org (
RepoNotFoundhint, src/error.rs). The hint always says "Pass one of the connected repository names to --repo", which is contradictory when the message just said the organization has no connected repositories. Minor; could vary the hint or reword it to cover both cases.
GitHub owner/repo names are case-insensitive in practice, so match connected repositories with eq_ignore_ascii_case instead of ==. A value like --repo ACTUAL-CLI now resolves to actual-cli; a bare name shared across owners still surfaces as ambiguous. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
url is never read by name resolution, yet it was a required field, so a server response that omits or renames it failed the whole deserialization and broke --repo resolution. Mark it #[serde(default)], matching the tolerant sibling fields in the same module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The RepoNotFound hint always said "Pass one of the connected repository names", which contradicts the message when the organization has none connected. Reword to "Pass a connected repository name to --repo, or omit --repo to query the whole organization" so it reads correctly in both the wrong-name and empty-org cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review @backlineint — addressed all three suggestions:
Each has a test; CI is green. Re-requesting your review. |
2 participants
What
actual advisor --repo <value>now accepts a connected repository's name, not only its UUID. A UUID still works exactly as before. A name is resolved to a repository id via the connected-repos API, matching on the repo name (orowner/nameto disambiguate a name shared by two owners). An unrecognized or ambiguous name fails with a clear message listing the repositories you can pick from.Quality
-D warningsclean;cargo fmtclean; full test suite green.list_connected_reposmethod plus response types), a newRepoNotFounderror, the advisor command wiring, and the--repohelp text.Draft for review.