Skip to content

feat: agent-aware oauth login (COR-14189) - #33

Merged
effervescentia merged 4 commits into
masterfrom
ben/cor-14189
Sep 29, 2026
Merged

effervescentia merged 4 commits into
masterfrom
ben/cor-14189

Conversation

@effervescentia

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI balanced review requested due to automatic review settings September 29, 2026 16:22
@linear-code

linear-code Bot commented Sep 29, 2026

Copy link
Copy Markdown

COR-14189

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Remote-agent loopback limitations are not communicated, and failure guidance can incorrectly claim that no credentials were stored.

Review effort: Balanced
Findings: 1 High severity · 2 Low severity

Open (3)
What changed in this PR

Adds agent-aware OAuth login with structured JSON progress and completion events.

Changes:

  • Adds authorization URL callbacks and agent-specific login handling.
  • Supports browserless agent authentication and structured failures.
  • Adds tests and user documentation.
File Description
internal/​oauth/​oauth.go Adds authorization URL callback support.
internal/​oauth/​command.go Routes agent mode to the new flow.
internal/​oauth/​agent.go Emits structured login events.
internal/​oauth/​agent_test.go Tests agent login success and failure.
internal/​cli/​auth.go Enables and documents agent login behavior.
docs/​vf_auth_login.md Updates generated command documentation.
Files not reviewed (1)
  • internal/cli/auth.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/oauth/agent.go
Comment thread docs/vf_auth_login.md Outdated
Comment thread internal/cli/auth.go Outdated

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The new guidance contradicts the README, and one event incorrectly claims that nothing has yet been persisted.

Review effort: Balanced
Findings: None

Resolved since last review (3)
Files not reviewed (1)
  • internal/cli/auth.go: Generated file
Previously missed (2)

In code that hasn't changed since last review

Medium severity Correct first-login hint about persisted OAuth client data

internal/​oauth/​agent.go:59

This hint is literally false for a first-time login: resolveClient dynamically registers and persists oauth-client.json before OnAuthURL emits this event. Clarify that no session credentials/tokens are stored yet, rather than claiming nothing has been written.

Low severity Update README to reflect agent-mode authentication behavior

docs/​vf_auth_login.md:19

The new behavior and this documentation conflict with README.md:267-268, which still says agent mode is unaffected and vf auth login remains blocked there. Update that README section so users do not receive opposite guidance from the two primary documentation surfaces.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Explicit agent-mode flags are not honored, and storage-failure guidance incorrectly identifies locked keychains as a cause.

Review effort: Balanced
Findings: None

Files not reviewed (1)
  • internal/cli/auth.go: Generated file
Previously missed (2)

In code that hasn't changed since last review

Medium severity Agent-mode flag is ignored before command routing

internal/​oauth/​command.go:58

This branch does not honor the documented --agent-mode override in production. Execute calls InitAgentMode before Cobra parses flags (internal/cli/root.go:228-230), which sets agentDetected; the post-parse call then returns at internal/output/agentmode.go:52. Consequently vf --agent-mode auth login takes the human path and may open a browser, while --agent-mode=false cannot disable auto-detected agent behavior. Agent mode must be re-evaluated after flag parsing (or explicit flag values must be allowed to override the cached detection) before routing login.

Low severity Keychain-locked diagnosis misidentifies session save failure

internal/​oauth/​agent.go:102

The keychain-locked diagnosis is misleading: writeSecrets swallows keychain write failures and SaveSession falls back to writing the tokens into oauth.json, so a locked keychain alone cannot produce ErrStoreSession. At this point the returned error comes from creating or writing the session file; directing the agent to fix the keychain can send it down the wrong recovery path.

@effervescentia

Copy link
Copy Markdown
Contributor Author

Re: Copilot review — the two "previously missed" findings

Fixed here: the keychain diagnosis in loginFailureHints (internal/oauth/agent.go).

Copilot is right. SaveSession treats the keychain as best-effort — writeSecrets swallows every failure (unavailable, keyringSet error, refresh-token rollback) and returns false, after which the tokens ride along in oauth.json instead. So a locked keychain can never surface as ErrStoreSession; the only thing that can is writeJSONFile failing on ~/.config/vf. The hint now names the filesystem rather than the keychain, with a comment recording why, plus a test assertion so the keychain claim can't come back.

Not fixed here: --agent-mode being ignored (internal/output/agentmode.go, internal/cli/root.go).

The bug is real — I confirmed it against a built binary, in both directions:

env flag result
clean (none) human API Error (HTTP 404):
clean --agent-mode human — flag ignored
CLAUDECODE=1 (none) agent envelope with error_type
CLAUDECODE=1 --agent-mode=false agent envelope — flag ignored

The mechanism is as described: Execute calls InitAgentMode(rootCmd) at root.go:230 before Cobra parses anything, so GetBoolFlag(...).changed is false and detection falls through to env vars — but the CompareAndSwap at agentmode.go:52 has already latched agentDetected, so the later call from PersistentPreRunE (root.go:68) returns immediately.

It is out of scope for this PR, though. Neither file is in this branch's diff, and the early InitAgentMode(rootCmd) call dates to the initial commit — Copilot's own "in code that hasn't changed since last review" label is accurate. --agent-mode has been inert on master the whole time, for every command, not just login.

What this PR does change is the blast radius: command.go is the first place vf auth login branches on IsAgentMode(), so the dead flag now means you can't force the JSON-event path on an undetected machine or force the browser path on a detected one — while the flag's own help text promises --agent-mode=false works. Worth fixing, but the fix belongs in agentmode.go (let an explicitly-changed flag override the latched detection instead of bailing at the CompareAndSwap), affects every command, and needs its own test. Filing it as a follow-up rather than growing this PR.

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

The agent login flow is coherent, documented, and covered by focused integration tests.

Review effort: Balanced
Findings: None

Files not reviewed (1)
  • internal/cli/auth.go: Generated file

@effervescentia
effervescentia merged commit 355f3c2 into master Sep 29, 2026
4 of 5 checks passed
@effervescentia
effervescentia deleted the ben/cor-14189 branch September 29, 2026 17:04
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.

2 participants