feat(auth): login self-heal, logout, status, OAuth best practices

[sVIKs]

What

Self-diagnosing, self-healing auth for the simulator MCP server, plus OAuth 2.1 / RFC 8414 / RFC 8252 alignment. Sibling PR on the corezoid plugin: sVIKs/corezoid-ai-plugin PR #69 (shared .env semantics are coordinated).

login self-heals ACCOUNT_URL

The .env keys are shared with the corezoid plugin; a sibling tool can overwrite ACCOUNT_URL with a host that serves no OAuth endpoints (field incident: the browser opened the admin dashboard and login died silently). login now asks the chosen gateway's public config for its declared account service (saUrl — works for cloud and on-prem) before opening a browser, corrects .env on mismatch, and reports what was fixed. Failures explain what to check and show the consent URL that was used.

New tools

• logout — removes ACCESS_TOKEN/REFRESH_TOKEN with a .env.bak backup first (backup failure aborts), warns about the shared .env.
• status — no-side-effect diagnosis: environment, token state, .env path; probe=true verifies ACCOUNT_URL against the gateway.

OAuth hardening (verified against the live account service)

• Endpoint discovery via /.well-known/oauth-authorization-server (the service publishes it — verified) with a conventional-path fallback for older on-prem installs.
• Silent renewal via the refresh_token grant before any browser round-trip. The service currently answers that grant with a session-scoped token papi rejects (verified live), so a refreshed token is trusted only after one authenticated papi call succeeds; otherwise the flow falls back to the browser, respecting refresh-token rotation and dropping a useless stored token.
• ACCOUNT_URL scheme validation (https, or http for loopback only) before the browser opens; ctx-aware, timeout-bounded token exchange honouring --insecure; token-endpoint bodies never echoed into errors (field names only).

Test hygiene

TestMain pins SIMULATOR_WORK_DIR to a sandbox for the whole suite — a test that forgets t.Setenv can no longer touch a real .env.

Testing

• go build ./..., go vet ./..., go test -race ./... green; +20 tests (self-heal, discovery, refresh rotation/validation, redaction, logout roundtrip, ctx cancel).
• Live: status MISMATCH detection against a poisoned .env on the real mw gateway; full OAuth login through the real consent flow; logout backup/idempotence; refresh grant + papi rejection verified with real tokens.

🤖 Generated with Claude Code

[gh-corezoid]

AI Review

Adds OAuth 2.1/RFC 8414 hardening to the simulator MCP server: login now self-heals ACCOUNT_URL from the gateway's public config before opening a browser, plus two new tools (logout with .env.bak backup and status for no-side-effect diagnosis), silent refresh-token renewal, PKCE flow context-cancellation, and a TestMain sandbox in both test packages.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch	❌ fail
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning
S1 — No manual edits to public/	✅ skip
S2 — API path parameter names match openapi	✅ skip
S3 — New tools have eval scenarios	⚠️ warning
S4 — Discovery artifacts committed if source changed	✅ pass
S5 — All six manifest files version-synced	✅ skip
S6 — README and ARCHITECTURE.md §4 updated for new tools	⚠️ warning

Issues found

[error] U4 — Wrong base branch
The PR targets main but simulator-ai-plugin requires base branch develop. This is a feature PR (feat(auth)) with new tools, not a docs-only or chore change, so the mismatch is hard — CI guards on simulator-ai-plugin reject non-develop bases.
Fix: close this PR and reopen (or retarget) against develop.

---

[warning] U6 — validateSimToken duplicates TLS transport logic from authHTTPClient
build.go's validateSimToken() (the papi-probe used to validate a refresh-minted token) hand-builds its own http.Transport with InsecureSkipVerify logic:

tr := &http.Transport{}
if insecure {
    tr.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
}
resp, err := (&http.Client{Timeout: 15 * time.Second, Transport: tr}).Do(req)

This duplicates auth.authHTTPClient(timeout) in oauth.go, which was introduced in this same PR for exactly this purpose. If transport behavior needs updating (proxy settings, retry policy, cipher list), there are now two places to change. Concrete failure scenario: --insecure support for the token-exchange path gets fixed in authHTTPClient but the papi probe keeps the old behavior silently.
Fix: export authHTTPClient (or add a thin auth.NewHTTPClient(insecure, timeout) constructor) and use it in validateSimToken.

---

[warning] S3 — logout and status missing from eval-scenarios.json and from knownToolNames()
build.go now registers logout and status, but neither appears in testdata/eval-scenarios.json. Per S3, new tools should have at least one evaluation scenario so the corpus stays representative of the real tool surface.
Additionally, eval_test.go's knownToolNames() function explicitly lists auth helpers ("login", "set-workspace") but was not updated to include "logout" and "status". Any future scenario that references these tools will fail TestEvalScenariosReferenceRealTools with an "unknown tool" error, which will block whoever adds the scenario rather than the PR author.
Fix: add "logout": true and "status": true to knownToolNames(), and add at least one scenario each to eval-scenarios.json.

---

[warning] S6 — docs/ARCHITECTURE.md §4 not updated
The Setup row in §4 still reads:

| Setup | set-environment ... login getWorkspaces set-workspace |

logout and status appear in README.md (correctly updated) but not in ARCHITECTURE.md §4.
Fix: add logout and status to the Setup row in docs/ARCHITECTURE.md §4 with their one-line descriptions.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[bazyk]

Review: content looks solid and server-verified — but not mergeable as-is

I verified the server-facing assumptions against pong-server and they hold:

• /config → data.saUrl is correct: getConfigReq returns saUrl: config('auth').url (the SA/account service), sent as { data }. So resolveAccountURL reads the right field.
• OAuth endpoints: pong-server itself talks to the separate SA service at ${saUrl}/oauth2/authorize and ${saUrl}/oauth2/token (getSingleOAuth2Obj.js). There is no /.well-known/oauth-authorization-server on pong — but that's fine, because your discovery probes the SA host and cleanly falls back to exactly those conventional /oauth2/* paths. So the fallback is correct even where metadata is absent.
• All referenced symbols exist (FetchPublicConfig, SaveAccountURL, engines.ResetAuth), and the Gate (build / vet / test / discovery) check passes.

Blockers before merge:

1. Wrong base branch. This PR targets main, so the Guard base branch check fails (base must be develop / release / hotfix) and the PR is BLOCKED. Please retarget to develop.
2. No version bump. CHANGELOG.md carries an ## [Unreleased] section only. Finalize a version — and coordinate with the in-flight 2.4.1 (fix(smart-forms): pushSmartForm Windows path-separator bug when creating a new page #71) / 2.4.2 (fix(kiro): self-healing MCP path resolution in dev checkouts, bump to 2.4.1 #72) so numbers don't collide (likely 2.4.3).
3. Given this touches auth / token storage / refresh rotation, it warrants a careful human review before merge — flagging rather than auto-merging.

Once retargeted to develop and versioned, this should be good to go.

[gh-corezoid]

AI Review

Re-review after synchronize: squashed to a single commit (feat(auth): self-heal, logout, status, OAuth best practices, bumped to 2.4.3) retargeted at develop — all four previously flagged issues are resolved and no new issues were found.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (develop)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	✅ pass
S1 — No manual edits to public/	⏭️ skip (no public/ changes)
S2 — API path parameter names match openapi	⏭️ skip (no new papi paths)
S3 — New tools have eval scenarios with correct names	✅ pass
S4 — Discovery artifacts committed if source changed	⏭️ skip (logout/status registered outside allOps(); no discovery impact)
S5 — All six manifest files version-synced	✅ pass (all at 2.4.3)
S6 — README and ARCHITECTURE.md §4 updated for new tools	✅ pass

Issues found

No issues found.

Fixed since last review ✅

• U4 (error) — Base branch — PR retargeted from main to develop. CI guard will now pass. ✅
• U6 (warning) — Duplicated TLS transport logic — validateSimToken now calls the exported auth.NewHTTPClient(insecure, timeout) constructor introduced in the same PR. Single place for transport policy. ✅
• S3 (warning) — Missing eval scenarios and knownToolNames() entries — "logout" and "status" added to knownToolNames() in eval_test.go; both tools now have liveOnly: true scenarios in eval-scenarios.json. ✅
• S6 (warning) — docs/ARCHITECTURE.md §4 not updated — Setup row now lists logout and status with their one-line descriptions. ✅

Build note (U5): go build ./... and go test ./... run clean on the PR branch; all packages that have tests pass. Base (develop) also green — no regressions introduced.

Version bump (S5): All six manifest files updated to 2.4.3 (addresses the human reviewer's request for a concrete version before merge).

---

This review was generated automatically. A human maintainer should still make the merge decision.

[bazyk]

Design question on the ACCOUNT_URL self-heal — is it treating the symptom rather than the cause?

The self-heal (resolveAccountURL + SaveAccountURL + the "🔧 Self-healed…" notes) exists only because the OAuth flow trusts a persisted ACCOUNT_URL in a .env that is shared with the corezoid plugin, which can overwrite it. But the account URL isn't ours to store an opinion about — the source of truth is what the gateway returns from /config (data.saUrl), which we already fetch via FetchPublicConfig (the same call set-environment uses).

If saUrl comes from the API, then the robust design is simply:

on login, always derive the account URL from /config and pass it straight to PKCEFlow; the stored ACCOUNT_URL is not authoritative.

That removes the whole problem class instead of patching it: if nothing trusts the stored value, there is nothing for a sibling plugin to "poison," and no compare-heal-persist-and-explain protocol is needed. It's a strictly smaller change than the current machinery, and it reuses the FetchPublicConfig we already have.

The only residual reason to keep a stored value at all is the narrow edges:

• /config unreachable (gateway down while the separate SA service is up) → fall back to the last known URL;
• a manual override for a non-standard on-prem where /config reports the wrong thing.

Both are a couple of fallback lines, not a self-heal subsystem.

Suggestion: replace the self-heal with "always derive saUrl from /config; treat ACCOUNT_URL only as an optional offline fallback / manual override." Keep the genuinely useful parts of this PR (status, logout, token-body redaction in errors) as-is.

Separately — two parts of the OAuth hardening are currently inert against the live backend and add most of the risk in this auth path:

• refresh_token silent renewal: per the PR's own note, the account service currently mints a session-scoped token papi rejects, so this path always falls back to the browser after an extra round-trip.
• RFC 8414 discovery (/.well-known/oauth-authorization-server): pong's SA service doesn't serve it, so discovery always falls back to the conventional /oauth2/authorize + /oauth2/token paths.

Consider splitting those two into a follow-up "when the SA service supports it" PR so only code that has an effect today lands in develop, keeping the auth surface minimal.