Conversation
AI CostNo spend detected for branch Updated 2026-09-25T17:08Z · last 30 days · data from Requesty |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
lavicosa
changed the title
7546a3a to
cdf61a9
Compare
|
|
||
| // Options configures Login. Only APIBaseURL is required; the rest exist so | ||
| // tests can stand in for the browser, the terminal, and the network. | ||
| type Options struct { |
There was a problem hiding this comment.
Is there a reason we have so many options right now?
As a first step, maybe we can simplify this a bit by keeping the options, but failing if they are not specified, limiting the number of different permutations that we need to test and care about?
As in, bottom line, remove all the default values from inside Login, and just have the caller always specify those.
There was a problem hiding this comment.
It was to facilitate headless vs tui mode as well as testing but agree it could be nicer. I've simpified this to a validate function to fail if not specified. All of the fields are required except OnAuthorizeURL.
| // The redirect URI must use the 127.0.0.1 literal, not localhost, so that is | ||
| // what we bind to. | ||
| func listenCallback(state string) (*callbackServer, error) { | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") |
There was a problem hiding this comment.
Can we add a comment here saying that port 0 generates a random port number, and we create the redirectUri after we already know it
| // check comes first so that a response meant for another attempt, or forged | ||
| // by a page that guessed the port, is never acted on. | ||
| func (s *callbackServer) resolve(query url.Values) callbackResult { | ||
| if subtle.ConstantTimeCompare([]byte(query.Get("state")), []byte(s.state)) != 1 { |
There was a problem hiding this comment.
why ConstantTimeCompare is important here?
There was a problem hiding this comment.
Good catch, dropped to be a !=.
| return callbackResult{err: errors.New("state mismatch: the response did not belong to this sign-in attempt")} | ||
| } | ||
|
|
||
| if code := query.Get("error"); code != "" { |
There was a problem hiding this comment.
err := query.Get("error") ?
| } | ||
|
|
||
| if code := query.Get("error"); code != "" { | ||
| return callbackResult{err: &Error{Code: code, Description: query.Get("error_description")}} |
There was a problem hiding this comment.
How do we decide when to return an Error and when to return an errors.New ?
There was a problem hiding this comment.
We return an Error for failures from the authorization server and errors.New when it's a problem the CLI detects.
It's like that to capture the auth server error code given in a typed struct and facilitate tests.
|
|
||
| // newHarnessCommands returns the `requesty <harness>` commands, each of which | ||
| // starts a harness with Requesty injected for that run. | ||
| func newHarnessCommands(env environment) []*cobra.Command { |
There was a problem hiding this comment.
Can't we build something that automatically generates a command per Harness in our internal harnesses package?
Probably just a bit of massaging of the DefaultConfigDirClaudeCode to be a method that accepts the harness name and a factory that does the same to create the actual client.
There was a problem hiding this comment.
I would push back and say that the harness launch commands belong in cmd. However, I have simplified it so that the definitions used here now come from LaunchDefinition and harnesses.LaunchDefinitions().
I also removed DefaultConfigDirClaudeCode and the other harness specific functions and moved it to be inside the harness constructor, the right place for it.
3c3c4fe to
a6adee1
Compare
2 participants
requesty <harness>commands that launch a harness through Requestyoauthpackage for authenticatingonboardingpackage which onboards a user using OAuth and provisions an API key