Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/controller/src/lib/openclaw-config-compiler.ts`:
- Around line 263-275: The code currently treats provider.apiKey === null as
“OAuth connected” and rewrites openai → openai-codex; change this to check a
persisted OAuth connection flag instead. In the block that uses
OAUTH_PROVIDER_MAP, oauthTarget and provider (from config.providers.find by
providerId), replace the apiKey === null gate with a check for a persisted
connection indicator on the provider (for example provider.oauthConnected ===
true or provider.connection?.profileId != null or provider.profile?.id != null
depending on your model for persisted OAuth state). Ensure you only return
`${oauthTarget}/${modelSuffix}` when provider.enabled is true AND the provider
has that persisted OAuth connection flag set (not merely apiKey === null).

In `@apps/controller/src/routes/provider-oauth-routes.ts`:
- Around line 67-81: Don't call container.openclawAuthService.consumeCompleted()
before persisting the provider and syncing; consumeCompleted() clears the
in-memory completion state too early. Change the flow in the completed branch so
you first compute models (using completed.models or OPENAI_CODEX_KNOWN_MODELS),
then call container.modelProviderService.upsertProvider(providerId, ...) and
await container.openclawSyncService.syncAll(), and only after both succeed call
container.openclawAuthService.consumeCompleted() and return c.json({
...flowStatus, models }, 200); alternatively implement a transactional
consumeCompleted() variant that only clears state after upsertProvider and
syncAll complete successfully.

In `@apps/controller/src/runtime/openclaw-auth-profiles-writer.ts`:
- Around line 19-45: readExistingProfiles currently swallows all read/parse
errors which can turn transient partial reads into full clobbers; change
readExistingProfiles to only return null for ENOENT and to rethrow other errors
(including JSON.parse failures) so callers can decide how to proceed. Add a
shared per-file lock/single-writer helper (e.g., withFileLock or a Mutex keyed
by "auth-profiles.json") and use it from both writeForAgents and
OpenClawAuthService.mergeOAuthProfile so the read-modify-write cycle for
auth-profiles.json is serialized; move the read->modify->write logic inside the
locked callback and ensure mergeOAuthProfile and writeForAgents call that
helper.

In `@apps/controller/src/services/openclaw-auth-service.ts`:
- Around line 473-526: findAgentAuthProfilesPath currently returns the
auth-profiles.json for the "first directory wins" agent which makes
readAuthProfiles and writeAuthProfiles nondeterministic in multi-agent setups;
change the logic so the target agent is resolved explicitly (preferred) or
updates are fanned out to every agent directory: update
findAgentAuthProfilesPath to accept an agent identifier (or provide a new
findAllAgentAuthProfilesPaths helper) and have readAuthProfiles/readAuthProfiles
use that explicit path(s); alternatively implement a loop in writeAuthProfiles
that writes the same JSON to every path returned by a new
findAllAgentAuthProfilesPaths, and make readAuthProfiles deterministic by
reading a specified agent path or merging/validating all agent files
consistently.
- Around line 528-543: mergeOAuthProfile is doing an unlocked read-modify-write
(readAuthProfiles -> writeAuthProfiles) which races with the new sync writer;
serialize updates by acquiring the same global/shared writer lock or delegating
all mutations to the single-writer helper in openclaw-auth-profiles-writer
(instead of calling writeAuthProfiles directly). Wrap the entire
read/merge/write sequence in the shared lock API provided by
apps/controller/src/runtime/openclaw-auth-profiles-writer.ts (or call its single
writer method) so mergeOAuthProfile (function name) uses that lock/entrypoint
and prevents concurrent clobbering of auth-profiles.json.
- Around line 95-101: Validate the incoming state before processing the error
branch in the OAuth callback: in the callback handler, check that the request's
state matches the stored/expected state and reject or ignore the request (return
400) if it does not, before reading error_description; additionally,
sanitize/escape any user-provided text passed into errorHtml by HTML-escaping
characters (use a small utility that replaces &, <, >, ", ' with entities) and
pass the escaped string into errorHtml instead of raw error_description; apply
the same state-first validation and escaping to the other error-handling branch
in this file as well.
- Around line 242-251: When removing the profileKey ("openai-codex:default")
from existing.profiles in disconnect logic, also clear any stale lastGood
pointer that references that profile: check existing.lastGood and if it equals
profileKey (or if lastGood points to a profile key that no longer exists in
remainingProfiles) remove or set updated.lastGood to undefined/null so
AuthProfilesData stays consistent; update the same updated object created after
computing remainingProfiles (references: profileKey, existing,
remainingProfiles, updated, lastGood).

In `@apps/web/src/pages/models.tsx`:
- Around line 1285-1356: The queries and mutations currently coerce missing
backend responses into harmless defaults, which hides failures and can leave
oauthPending spinning; update the oauthProviderStatus and oauthFlowStatus
queryFn functions to treat missing or error responses as failures by throwing an
error when res.data is undefined or indicates an error (instead of returning
{connected:false} or {status:"idle"}), and update startOAuthMutation.mutationFn
and disconnectOAuthMutation.mutationFn to validate res.data and throw when the
response is missing or contains an error so React Query routes to onError rather
than onSuccess; keep the existing enabled/refetchInterval logic and ensure the
useEffect still reads oauthFlowStatus.data after failures are surfaced via
react-query errors.

In `@docs/plans/2026-03-24-oauth-model-persistence.md`:
- Line 15: Several task headings (for example "Task 1: Add `models` field to
OAuth status response schema" and the other task headings noted in the review)
are using level-3 headings (###) directly under the document's top-level #
header, causing a heading-level jump; change those task headings from ### to
level-2 (##) so the structure is # then ## for tasks, and scan the file for the
other task headings mentioned (the ones at the other commented locations) and
update them similarly to maintain consistent hierarchical markdown structure.

In `@docs/plans/2026-03-24-openai-oauth-test-plan.md`:
- Around line 35-36: Update the test plan so callback behavior no longer assumes
model-fetch during the OAuth callback: change table rows for cases 11 and 12 to
remove "Mock `fetch` for token + models endpoints" and instead assert that on a
valid callback the status becomes "completed" and the user's profile is
populated while models are NOT populated at callback (models are populated later
via the known-model persistence flow), and for the state-mismatch case assert
status "failed" with a CSRF error and no model-fetch attempt; apply the same
adjustments to the corresponding section covering lines 59-67 so all
expectations reflect the no-fetch callback flow.

---

Nitpick comments:
In `@apps/controller/src/services/openclaw-sync-service.ts`:
- Around line 59-61: OAUTH_PROVIDER_PREFIXES is duplicated and must be the
single source of truth; extract the mapping into a shared constant (e.g., export
const OAUTH_PROVIDER_PREFIXES or OAUTH_PROVIDER_MAP in a new shared module like
openclaw-oauth-constants) and replace the local definitions in both
openclaw-sync-service.ts (const OAUTH_PROVIDER_PREFIXES) and
openclaw-config-compiler.ts (OAUTH_PROVIDER_MAP) to import and use that shared
export so runtime and compile-time model resolution stay in sync; ensure the
exported name is descriptive and update all references in both files to use the
imported symbol.

In `@docs/plans/2026-03-24-oauth-model-persistence.md`:
- Line 11: Replace the machine-specific absolute worktree path on line 11 with a
repo-relative reference; change the hardcoded
"/Users/alche/Documents/digit-sutando/nexu/.worktrees/openai-oauth" to a
relative path such as ".worktrees/openai-oauth" or "./.worktrees/openai-oauth"
and keep the branch name `feat/openai-codex-oauth` intact so contributors can
use the same worktree reference across machines.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/controller/openapi.json`:
- Around line 4848-4850: The OpenAPI 3.1 schema for the apiKey property still
uses the removed "nullable": true; update the apiKey schema (the "apiKey"
property in the OpenAPI document) to express nullability using a union type by
replacing "type": "string" and "nullable": true with a JSON Schema
3.1-compatible null union such as "type": ["string","null"] so SDKs will honor
sending null to clear the saved key.

---

Nitpick comments:
In `@apps/controller/tests/provider-oauth-routes.test.ts`:
- Around line 8-55: The createEnv helper is duplicated across tests; extract it
into a single shared test fixture (e.g., export function createEnv(rootDir:
string): ControllerEnv from a new tests/fixtures/test-env module), export the
ControllerEnv type if needed, and replace the duplicate implementations in
provider-oauth-routes.test and openclaw-auth-service.test with an import of the
shared createEnv function; ensure both tests update their imports to reference
the new fixture and that the function signature and returned keys remain
unchanged.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/pages/models.tsx`:
- Around line 1273-1279: The onSuccess callback currently calls
window.open(data.browserUrl, "_blank") in the OAuth flow; update that call to
pass the third feature string "noopener,noreferrer" (i.e.,
window.open(data.browserUrl, "_blank", "noopener,noreferrer")) to prevent the
opened page from accessing window.opener, leaving the rest of the logic
(setOauthPending(true) and toast.error(data.error)) unchanged; locate this
change in the onSuccess handler where window.open, setOauthPending, and
toast.error are referenced.
- Around line 1307-1326: The saveCodingPlanMutation currently has no error
handler so failures from saveProvider leave the UI silent; add an onError
handler on saveCodingPlanMutation that captures the error from saveProvider and
surfaces user feedback (e.g., set an error state or call a toast/notification)
and ensures UI state cleanup (e.g., do not clear codingPlanKey or call
markSetupComplete/onAutoSelectModel on failure). Locate saveCodingPlanMutation
and implement onError to accept the error, log/report it, update any local error
state or show a notification, and avoid running the success-side effects
(setCodingPlanKey, markSetupComplete, onAutoSelectModel) when an error occurs.

---

Nitpick comments:
In `@apps/web/src/pages/models.tsx`:
- Around line 1328-1337: The reset useEffect that calls setApiKey, setBaseUrl,
setIsEditingApiKey, setVerifiedModels, setOauthPending, setCodingPlanKey, and
setCodingPlanRegion currently depends on [dbProvider, meta.defaultProxyUrl];
update the dependency array to also include providerId so the effect explicitly
re-runs when the selected provider changes (keeps the same reset logic in the
useEffect tied to dbProvider, meta.defaultProxyUrl, and providerId).

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/controller/src/runtime/openclaw-auth-profiles-store.ts`:
- Around line 99-112: readAuthProfiles currently only swallows missing-file
errors but lets parse errors from parseAuthProfilesData propagate; update
readAuthProfiles to catch JSON/parse errors and, when options?.missingOk is
true, return null instead of rethrowing. Specifically, wrap the readFile +
parseAuthProfilesData call in a try/catch, keep the existing isMissingFileError
check, and add a check for parse failures (e.g., any non-missing error from
parseAuthProfilesData) to return null when options?.missingOk is true; otherwise
rethrow. This targets the readAuthProfiles function and prevents uncaught parse
errors for callers (e.g., openclaw-auth-service.ts) that rely on missingOk.

---

Nitpick comments:
In `@apps/controller/src/services/openclaw-sync-service.ts`:
- Around line 62-78: Replace the hard-coded OAUTH_PROVIDER_PREFIXES in
openclaw-sync-service.ts with a derived list from the canonical
OAUTH_PROVIDER_MAP exported from openclaw-config-compiler.ts: export
OAUTH_PROVIDER_MAP (if not already exported) from openclaw-config-compiler.ts,
import it into openclaw-sync-service.ts, and compute OAUTH_PROVIDER_PREFIXES as
Object.values(OAUTH_PROVIDER_MAP).map(p => `${p}/`) so
resolveAvailableRuntimeModel uses the authoritative prefixes instead of a
duplicated array.

In `@apps/controller/tests/openclaw-config-compiler.test.ts`:
- Around line 223-248: Tests around compileOpenClawConfig and createConfig
duplicate nearly identical OpenAI config payloads (bots, runtime.defaultModelId,
providers.models, etc.); extract a shared fixture/helper (e.g.,
createOpenAIConfigFixture or openaiConfigPayload) that builds the common config
and accept small overrides for bot/model/provider differences, then replace the
duplicated inline objects in the tests with calls to that helper to reduce
duplication and future drift.