feat(cli): fountain oauth-client (#1125) - #1884
Conversation
BinaryBourbon
left a comment
There was a problem hiding this comment.
Clean. The oauthClient struct carries a comment pointing at oauth_client_json.ex as the thing it mirrors, which is what stops it drifting silently; id vs client_id being called out in that same comment is worth its weight, since taking the wrong one is the mistake every user of these commands will make once.
docs/cli.md has the fenced bash block with all four subcommands, so cli/internal/cmd/docs_test.go is satisfied, and docs/cli/commands.md is regenerated rather than hand-written. The prose reads as Simplified Technical English without fighting it.
One nit worth acting on:
Flags().StringSlice splits values on commas. Cobra's StringSlice parses each occurrence as CSV, so
fountain oauth-client create Notes --redirect-uri 'https://notes.test/cb?ids=1,2'
arrives at the server as two redirect URIs, https://notes.test/cb?ids=1 and 2, and the second is rejected with an error that points nowhere near the cause. StringArray appends each occurrence verbatim and is the right type for anything URL-shaped. It is a one-word change in two places (create and update) and the strings help text (--redirect-uri strings) stays the same, so docs/cli/commands.md does not move.
Smaller:
origins:inprintOAuthClientinherits the loopback under-reporting from the server'soriginsfield (see my note on #1882) — it will printhttp://localhost:5173for a client CORS admits on any port.oauthClientCreateandoauthClientListfall through toFatal(err.Error())whereupdateanddeletetranslate a 404 into "app not found: %s". Not wrong — neither takes an id — just noting the asymmetry is deliberate.
Approving.
89a5b9f to
fbf9fb7
Compare
0f08acd to
0ed9c05
Compare
|
Review feedback applied; force-pushed (amended commit).
|
BinaryBourbon
left a comment
There was a problem hiding this comment.
StringArray in both places, with the reason in a comment at the declaration rather than at one of the three read sites. All four GetStringSlice reads moved with it — I checked, there is no half-converted pair left.
I was wrong about the help text. I said the strings rendering stays the same so docs/cli/commands.md would not move. Cobra prints the flag's type name, so it became --redirect-uri stringArray and the option blocks realigned. You regenerated them, which is why cli/internal/cmd/docs_test.go is green — I ran go test ./internal/cmd/ at the stack tip and the package passes, so the regeneration is consistent rather than hand-patched.
The comment is worth the three lines it takes:
StringSlice, not StringArray: StringSlice parses each occurrence as CSV, so a redirect URI carrying a comma in its query string would arrive as two URIs, the second of them nonsense.
StringArray looks like the less obvious choice of the two, so without that a future contributor "fixes" it back.
Two things for after this stack, neither touching this PR:
The same type is used for three other repeatable flags, and one of them has the same trap:
| Flag | Risk |
|---|---|
fountain apply --var KEY=VAL |
real — --var 'HOSTS=a,b' becomes HOSTS=a and b, and b is not a KEY=VAL pair |
fountain conv prompt --image |
low — a comma in a filename is legal but rare |
fountain webhooks create --event |
none — event names carry no commas, and CSV is arguably wanted there |
The --var one is worth an issue on its own; it is the same failure with a worse error message, since a bare b fails the pair parse rather than the URI validation.
origins: in printOAuthClient still prints the bare join. #1882 answered this nit by putting "admitted on any port, not only the port shown here" in the OpenAPI description and #1883 put it on the console card, but neither reaches a terminal. One conditional suffix in printOAuthClient, the way origins_line/1 does it, would finish the set. Not blocking — the nit was an either/or and one side of it landed.
No test file for oauthclient.go, which matches agent.go, env.go, vault.go and sandbox.go — the standard here is the docs diff plus the server-side tests, not per-command Go tests, and I would rather this met that standard than introduced a new one.
Approving.
|
Both follow-ups from my review are filed, so they do not go with the stack:
Neither blocks this PR. |
fbf9fb7 to
153e791
Compare
`list`, `create`, `update` and `delete` against `/api/oauth/clients`, so an app being built inside a sandbox can be registered from the same terminal it is being built in. `create` prints the generated `client_id`, which is the thing that goes in the app, and the `id` these commands take, which is the record. `delete` says what deleting does and does not do: new sign-ins stop, and keys already issued stay valid until `fountain keys revoke`. `docs/cli.md` gains the section `cli/internal/cmd/docs_test.go` requires for a new command, and `docs/cli/commands.md` is regenerated from the Cobra tree with `go test . -update-cli-docs`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
0ed9c05 to
e4c54c3
Compare
Part 8 of 9 for #1125 (re-cut of #1489). Based on #1883.
list,create,updateanddeleteagainst/api/oauth/clients, so an app being built inside a sandbox can be registered from the same terminal it is being built in.createprints the generatedclient_id, which is the thing that goes in the app, and theidthese commands take, which is the record.deletesays what deleting does and does not do: new sign-ins stop, and keys already issued stay valid untilfountain keys revoke.docs/cli.mdgains the sectioncli/internal/cmd/docs_test.gorequires for a new command, in a fencedbashblock.docs/cli/commands.mdis regenerated from the real Cobra tree withgo test . -update-cli-docs, not hand-written.Re-derived, not carried: #1489's version of this file imported
github.com/BinaryBourbon/fountain/cli/internal/api. The module isgithub.com/managoat/fountain/clinow, andapileftinternal/, so it would not have compiled.Left to the next PR: ADR 0021, the manual and the SDK bump.
🤖 Generated with Claude Code
https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
Part of #1125