[radenkovic]

Why this PR exists

These are the last review fixes from the TW-6922 CLI OAuth work. The commit (5ce9a94, 2026-10-08) was made on tw-6922-cli-oauth after #123 had been squash-merged, so it never reached main. GitHub had already auto-deleted the remote branch, which left the local branch as its only copy. This PR cherry-picks it onto current main unchanged; it applied with no conflicts.

What it changes

• TW-5024: fix(auth): update config file when switching default grant #14: isOAuthSession returns (bool, error). A keyring read error is now reported instead of being treated as "not an OAuth session". So refresh no longer calls account.Refresh on an OAuth session, a failed read no longer counts as "no dashboard login session", and orgs switch reports the error.
• TW-5023: fix: correct Nylas dashboard URL typo #15: dpop.New reports a malformed or wrong-length stored seed instead of silently replacing it. oauth logout checks the dashboard session's origin before building the DPoP service, so it never creates a key when there is no OAuth dashboard session.
• TW-5020: feat(audit): add audit logging with invoker identity tracking #21: the oauth package no longer wires the dashboard hooks from init(). main.go calls oauth.WireDashboard() explicitly.
• TW-5019: Nylas Chat #22: oauth status, login and logout honour --json, --format yaml and --quiet through common.GetOutputWriter. Structured output never carries a token, and in structured mode the browser prompt goes to stderr.
• feat(mcp): native MCP server + Codex assistant support #24: ARCHITECTURE.md lists the new ports (oauthas, lock, MCP credentials) and adapters.
• N11: the remaining review nit, plus new tests (DPoP seed, kept login session, OAuth logout DPoP, session origin, structured output).

Verification

On this branch rebased onto main: go build, go vet, make lint (0 issues) and go test -race ./... all pass.

⚠️ Conflicts with #125 (TW-7617, device sign-in)

There's one small hunk in internal/cli/oauth/login.go, around the pre-login prompt. Whichever PR merges second needs a rebase. When resolving it, apply #22's rule to the device prompt too: in structured output (--json / --format yaml), the device code and verification URL go to stderr so stdout carries only the report, and --quiet must not hide the code, since the user can't sign in without it.

🤖 Generated with Claude Code

[qasim-nylas]

Review of PR #126 (Claude + Codex)

Two reviewers examined this PR: Claude and Codex. We did a check of each finding against the code on 1f95115. All findings below are confirmed, except where we write "not reproduced".

Result: changes are necessary before merge. Two items break make ci or make ci-full. One item makes --json output not valid.

Summary of the claims

Item	Status	Note
#14 isOAuthSession returns (bool, error)	Correct	All callers handle the error. A keyring error does not stop oauth logout or config reset.
#15 dpop.New reports a bad seed	Correct	The new behavior has no recovery path. Refer to finding 3.
#21 oauth.WireDashboard() from main.go	Correct	cmd/nylas/main.go is the only entrypoint. No test used the old init().
#22 structured output	Not complete	The device-code prompt does not obey the rule. Refer to finding 1.
#24 ARCHITECTURE.md	Correct
N11 and test file splits	Correct	We compared each declaration in the moved test files. All are byte-identical. No test is lost.

---

Findings that block the merge

1. [Major] The device-code prompt always goes to stdout. (Claude + Codex)

• File: internal/cli/oauth/login.go:102-112
• OnDeviceAuthorization writes the verification URL and the user code to cmd.OutOrStdout(). It does not examine the output mode.
• Result: nylas oauth login --device --json writes text and then JSON to stdout. jq cannot parse it.
• An SSH session selects device mode automatically. Thus --json over SSH fails also, without --device.
• The PR description gives this rule for the merge with [TW-7617] Add device-code sign-in for remote CLI login and signup #125: "the device code and verification URL go to stderr". The code does not apply it.
• No test finds this problem. The fake Login does not call opts.OnDeviceAuthorization.
• Fix:
  1. In structured mode, write the prompt to cmd.ErrOrStderr().
  2. Do not hide the prompt with --quiet. The user cannot sign in without the code.
  3. Add a test. Make the fake call OnDeviceAuthorization with --json and with --quiet. Make sure that stdout is pure JSON or empty, and that stderr contains the code.

2. [Major] login_kept_session_test.go breaks the integration build. (Codex)

• File: internal/cli/oauth/login_kept_session_test.go:1
• The file has no //go:build !integration tag. It uses fakeService and withService from oauth_test.go. That file has the !integration tag.
• Result: go vet -tags integration ./internal/cli/oauth/ fails with undefined: fakeService. Thus make test-integration and make ci-full fail.
• Fix: add //go:build !integration as the first line, as in the other unit test files in this package.

3. [Major] A new test fails when SSH environment variables are set. (Claude + Codex)

• File: internal/cli/oauth/output_test.go:154
• isSSHSession() selects device mode. Then stderr contains "Using device sign-in", but the test expects "Opening your browser".
• We reproduced it: TestLoginCmd_JSONReportsSessionAndDashboard fails in an SSH shell. It passes when SSH_CONNECTION, SSH_CLIENT and SSH_TTY are not set.
• Result: make ci fails for each developer who works over SSH. The statement "go test -race ./... all pass" is true only without SSH.
• Fix: set the three variables to empty with t.Setenv in this test, or in withService. device_test.go:27-29 already does this.

---

Findings that we recommend you fix

4. [Major] A bad DPoP seed now stops all dashboard commands, and the error gives no help. (Claude)

• Files: internal/adapters/dpop/dpop.go:46-52, through internal/cli/dashboard/helpers.go:53-55
• Before this PR, the CLI replaced a bad seed. Now each command that builds the DPoP service fails. This includes nylas dashboard login and nylas dashboard logout.
• The error is "DPoP proof generation failed: the stored key is N bytes, want 32". It does not tell the user what to do.
• Only nylas config reset removes dashboard_dpop_key. That command also removes the API credentials, the grants and the config.
• We agree that the CLI must not replace the key silently. But the user must have a recovery path.
• Fix:
  1. Add a recovery instruction to the error message.
  2. Or, let dashboard logout clear the local tokens and the key without the DPoP service.
  3. Change the error prefix. The key load failed, not "proof generation".

5. [Minor] oauth logout keeps the dashboard session when the seed is bad. (Claude)

• File: internal/cli/dashboard/helpers.go:167-169
• If loadOrCreateDPoPKey fails and an OAuth dashboard session is stored, the function returns early. It does not revoke the dashboard tokens. It does not clear them locally.
• Then the then step runs without the lock.
• Fix: clear the local dashboard tokens under the lock, also when the DPoP service fails. Only the server revocation needs DPoP.

6. [Minor] dashboard logout can skip the server revocation without a message. (Claude, code reading only, not reproduced)

• Files: internal/app/dashboard/session_store.go:98-99, internal/app/dashboard/auth_service.go:141-159
• This applies to a legacy session that has no recorded server. If the read of the origin fails, loadDashboardTokens returns an error. Then logout() does not revoke the session. It clears the session locally and returns nil.
• Before this PR, the CLI revoked the session in this case.
• This needs one keyring read to fail while other reads succeed. Thus it is rare.
• Fix: return the error, or try the revocation.

7. [Minor] Some new branches have no tests. (Claude)

• orgs switch when the CLI cannot read the origin.
• A read error in checkLegacyOAuthSessionServer.
• thenUnlessOAuthSession when a concurrent login stores a session (ran=false, helpers.go:187-189).
• --format yaml for status, login and logout.
• The device prompt in structured mode (finding 1).

8. [Minor] Some documents are not correct or not complete. (Claude)

• docs/COMMANDS.md:189 says "--quiet prints nothing" for login. In device mode this is not true, and it must not be true.
• docs/security/overview.md does not tell about the DPoP seed change. The doc rules mark a security change as critical.
• No document tells how to recover from a bad DPoP key.

---

Nits (optional)

• structuredOutput (output.go:141) only calls common.IsStructuredOutput. You can call the common function directly.
• quietOutput (output.go:146) reads the flags again and also examines common.IsQuiet(). structuredOutput does not examine common.IsQuiet(). Thus the two functions can give different answers.
• statusReport.LoggedIn is always true. When there is no session, the command returns an error, not a report.
• The text of ErrDashboardLoginSessionKept (oauth_session.go:32) is difficult to read. We recommend: "Run nylas dashboard logout, then run this command again. Do this to use this login, or if that session has expired."
• loadOrCreateDPoPKey uses common.DashboardSessionLock, not the lock given to clearOAuthSessionThen. Thus the tests and production use different locks on this path.
• TestWireDashboard_SetsTheDashboardHooks is in output_test.go. It is not an output test.
• thenUnlessOAuthSession ignores the unlock error at line 188.

---

Build and test results (1f95115)

Check	Result
go build ./...	Pass
go vet ./...	Pass
go vet -tags integration ./internal/cli/oauth/	Fail (finding 2)
golangci-lint on the changed packages	0 issues
go test on the changed packages, SSH variables set	Fail (finding 3)
go test -race -count=1 on the changed packages, SSH variables not set	Pass

The test file splits are correct. We compared all top-level declarations: adapters/dashboard 28/28, adapters/mcp 126/126, app/auth 69/69, app/dashboard 84/84 (plus 5 new ones).

🤖 Generated with Claude Code

[qasim-nylas]

Second review of PR #126 (Claude + Codex), commit 222a571

Two reviewers examined the fix commit 222a571: Claude and Codex. We did a check of each item from the first review. Then we examined the new code. We compared the two reviews and did a check of each finding against the code.

Result: no item blocks the merge. The fix commit corrects all three blockers from the first review. It also adds a recovery path for a bad DPoP key. The recovery path has some small problems. We recommend that you fix items 1 to 3 below before merge.

Status of the first review's findings

Item	Status	Evidence
1. Device prompt goes to stdout	Fixed	login.go:104-117 writes the prompt to stderr in structured mode and with --quiet. This applies to --device, --no-browser and the SSH path. The fake now calls OnDeviceAuthorization (oauth_test.go:46-52). We removed the stderr switch, and the new test (output_test.go:212-245) failed. Thus the test finds the problem.
2. Build tag on login_kept_session_test.go	Fixed	go vet -tags integration ./... passes.
3. Test fails when SSH variables are set	Fixed	clearSSHEnv (oauth_test.go:515). The tests pass with and without the SSH variables.
4. Bad DPoP seed stops all dashboard commands	Fixed, with a new small problem	The error now gives a recovery instruction (dpop.go:31). ErrDashboardDPoPKeyInvalid and ErrDashboardDPoPKeyUnreadable are different errors (errors.go:73-78). dashboard logout removes the bad key. Refer to new finding 1.
5. oauth logout keeps the dashboard session when the seed is bad	Fixed	helpers.go:167-173 calls ForgetIfOAuthThen. This removes the session under the lock. The message is not correct. Refer to new finding 2.
6. dashboard logout skips the revocation without a message	Fixed	auth_service.go:148-154 returns "cleared but not revoked". Test: forget_session_test.go:57-74.
7. New branches have no tests	Partly fixed	Now tested: the device prompt with --json and --quiet, and the read error of the legacy server. Not tested: orgs switch when the origin read fails, thenUnlessOAuthSession with ran=false, --format yaml, and the new recovery branch (new finding 4).
8. Documents	Fixed	COMMANDS.md:189-194,291-297 and security/overview.md:28 agree with the code. One message does not agree. Refer to new finding 2.
Nits	Not fixed	All seven nits from the first review stay. They are optional.

We also did a check of these points:

• A transient keyring read error does not delete the key. Only a value that the CLI read correctly, and that does not decode, is "invalid". A read failure is "unreadable", and the CLI deletes nothing.
• errors.Is finds both new errors through all callers. The %w and errors.Join wrapping is correct.
• dashboard logout with a bad key removes the legacy session or the OAuth-origin session. It keeps the nylas oauth login tokens. This is correct, because both sessions use the bad key.

---

New findings

1. [Minor] The recovery can delete a valid key and an active session. (Claude + Codex, reproduced by Codex)

• Files: internal/cli/dashboard/logout.go:20-23, internal/app/dashboard/oauth_session.go:256-265
• The CLI examines the key without the lock (helpers.go:54-55). Then ForgetSession gets the lock and deletes the key. It does not examine the key again.
• Failure sequence:
  1. Process A finds the bad key.
  2. Before A gets the lock, a different process removes the key, and a new login stores a valid key and session.
  3. Process A gets the lock. It deletes the valid key and the new session.
• Result: the new session stays active on the server, and nothing can revoke it. TW-5023: fix: correct Nylas dashboard URL typo #15 must prevent this type of orphaned session. The time window is small, but the result is permanent.
• Codex reproduced this with a scratch test.
• Fix:
  1. In ForgetSession, read the key again under the lock.
  2. Delete the key only if it is still not valid.
  3. If the key is now valid, use the usual logout with revocation.
  4. If the read fails, keep the key and return the error.

2. [Minor] The oauth logout message is not correct when the DPoP key cannot load. (Claude, code reading)

• Files: internal/cli/oauth/logout.go:41-44, internal/cli/dashboard/helpers.go:172
• The user sees: "Could not end the dashboard session: the dashboard session was not revoked on the server: ...".
• But the CLI removed the session locally. Thus "Could not end" is not correct. Also, the docs say that the CLI removes the session.
• The CLI also shows "not revoked" when it finds no OAuth session under the lock. In this case, it removed nothing.
• Fix:
  1. Change the message to "The dashboard session was removed locally, but not revoked on the server".
  2. Show this message only when the CLI removed a session.

3. [Minor] The CLI replaces an empty stored seed without a message. (Codex, reproduced)

• Files: internal/adapters/dpop/dpop.go:46, internal/cli/dashboard/helpers.go:54-55
• Condition: the store contains dashboard_dpop_key with an empty value, and Get returns no error.
• Then seedB64 != "" is false, and New makes and stores a new key. This is the silent replacement that TW-5023: fix: correct Nylas dashboard URL typo #15 removes.
• Also, loadOrCreateDPoPKey thinks that a key is present. Thus it does not get the creation lock.
• Fix:
  1. Make a new key only when errors.Is(err, domain.ErrSecretNotFound).
  2. Return ErrDashboardDPoPKeyInvalid for an empty value that the CLI read correctly.

4. [Minor] The recovery branch of dashboard logout has no test. (Claude)

• File: internal/cli/dashboard/logout.go:21-23,47-60
• If a person removes the ErrDashboardDPoPKeyInvalid branch, all tests still pass. This branch is the only recovery path in the docs.
• Fix: add two tests.
  1. An invalid key: make sure that the key and the session are removed, and that the OAuth tokens stay.
  2. An unreadable key: make sure that the CLI deletes nothing.

---

Nits (optional)

• The recovery messages ignore the output flags. logoutWithInvalidKey (logout.go:57-58) writes the warning to stdout with common.Yellow.Println. Thus --json and --format yaml give text, and --quiet does not hide the success message. (Codex, reproduced.) The usual dashboard logout path has the same behavior before this PR. We recommend that you use common.PrintWarningStderr for the warning, at minimum.
• The recovery message can be incorrect. It says "the session was removed" also when no session was stored. (Claude, reproduced.)
• A revocation can stop without a good reason. auth_service.go:148 also skips the revocation when only the write of the session server fails (session_store.go:79-81). In this case, the server check passed before. (Claude, code reading only. This is rare.)

---

Build and test results (222a571)

Check	Result
go build ./...	Pass
go vet ./...	Pass
go vet -tags integration ./...	Pass (failed on 1f95115)
golangci-lint run ./internal/... ./cmd/...	0 issues
go test -race -count=1 on the changed packages, SSH variables set	Pass (failed on 1f95115)
go test -race -count=1 on the changed packages, SSH variables not set	Pass
File sizes	All changed Go files have fewer than 600 lines. The largest is account_client_test.go with 576 lines. docs/COMMANDS.md has 1,026 lines, and it had 1,006 lines on main before this PR.

Both reviewers used scratch tests for the reproductions. We deleted the scratch tests after use.

🤖 Generated with Claude Code

[qasim-nylas]

LGTM 👍