Doctor: expose auth profile findings#97125
Conversation
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 4:21 PM ET / 20:21 UTC. Summary Reproducibility: not applicable. as a bug reproduction; current main has the legacy Doctor note path, while this PR adds a new explicit structured lint surface for the same auth-profile findings. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused five-file shape after an auth/Doctor maintainer accepts the explicit Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction; current main has the legacy Doctor note path, while this PR adds a new explicit structured lint surface for the same auth-profile findings. Is this the best way to solve the issue? Yes, if maintainers want this feature: the implementation reuses existing auth-health classification, keeps detection read-only/default-disabled, and leaves mutating repair in the legacy Doctor path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 640258d7b31d. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
b98f68b to
506aa69
Compare
|
Addressed ClawSweeper's P2 canonical auth-store path finding in Change made:
Fresh proof:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
506aa69 to
c906573
Compare
|
@clawsweeper re-review Current head Prior addressed findings remain in place:
Validation before rebase remains applicable to the unchanged diff:
Post-rebase hosted checks are in progress on the new exact head. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
galiniliev
left a comment
There was a problem hiding this comment.
This adds an opt-in structured Doctor lint check for auth-profile findings, but the PR head is behind current main on the same doctor auth surface. The result is not mergeable as-is and, more importantly, the checked-out PR head drops a current-main auth diagnostic.
LOC: +377/-13 (5 files)
Findings:
- [P1] Rebase without dropping malformed API-key auth diagnostics. Current
origin/mainclassifies malformed API-key profile values as missing auth withreasonCode: "malformed_api_key", surfaces the tailored "Paste the API key value" doctor hint, and hassrc/commands/doctor-auth.profile-health.test.tscoverage for a stored API-key profile whose key is anopenclaw onboard --auth-choice ...command. This PR head removes that test, loses themalformed_api_keyhint branch inresolveAuthIssueHint, and restores the olderfindIssuesfilter that ignoresapi_keymissing profiles. The new structured collector atsrc/commands/doctor-auth.ts:378also copies that narrower filter, so after rebase it should include the same API-key missing/malformed cases as the existing doctor path. Please rebase onto currentmainand preserve the current malformed API-key diagnostic behavior in both the legacy doctor note path and the new structuredcore/doctor/auth-profilesfindings.
Best-fix verdict: acceptable mitigation once rebased and parity with current Doctor auth diagnostics is preserved.
Alternatives considered: keeping the structured check narrower was rejected because the PR states it exposes missing credentials and it is wired through the existing auth-profile health surface; moving this into a separate check was rejected because the existing doctor:auth-profiles contribution already owns these profile findings.
Code read: src/commands/doctor-auth.ts, src/commands/doctor-auth.profile-health.test.ts, src/agents/auth-health.ts, src/agents/auth-profiles/credential-state.ts, src/flows/doctor-health-contributions.ts, src/flows/doctor-lint-flow.ts, src/flows/health-check-adapter.ts.
Remaining uncertainty: I did not run a full post-rebase suite because the live PR is currently DIRTY against main; focused tests on the checked-out head passed via node scripts/run-vitest.mjs run src/commands/doctor-auth.profile-health.test.ts src/commands/doctor-auth.hints.test.ts src/flows/doctor-health-contributions.test.ts --reporter=verbose.
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
3809b19 to
c6a4fb1
Compare
|
@galiniliev follow-up on your requested changes. Addressed your P1 on preserving malformed API-key auth diagnostics. Current head: What changed:
Proof on the current head:
This remains default-disabled ( |
|
Merged #97125. Landed source head: Final validation before merge:
The landed change keeps |
What Problem This Solves
Doctor has a mature auth-profile health path, but its findings were not available through the structured lint surface. That left profile expiry, missing credentials, cooldown/disabled state, and legacy Codex provider overrides outside the
doctor --lint --only/--allworkflow.Why This Change Was Made
This adds an opt-in structured health check,
core/doctor/auth-profiles, wired through the existingdoctor:auth-profilescontribution. Detection is read-only and mirrors existing auth-profile health behavior; the existing mutating repair flow remains inrunAuthProfileHealthand the legacy repair helpers.The check is
defaultEnabled: false, so it is not part of default Doctor lint. It is discoverable through explicit lint selection such as--only core/doctor/auth-profilesand--all.After ClawSweeper's P2 path finding, structured auth-profile findings now report the canonical SQLite auth store display path from
resolveAuthStorePathForDisplay(...), not the legacyauth-profiles.jsonpath.User Impact
doctor --lintbehavior is unchanged for auth profiles.openclaw-agent.sqliteauth store path.Evidence
Focused tests:
Result after the path fix: 2 Vitest shards passed, 68 tests total.
ClawSweeper-requested path resolver proof:
Result: 1 Vitest shard passed, 9 tests total.
Formatting/lint/type/API checks:
All passed.
Source CLI proof with isolated temp state/config and a seeded expired OAuth profile in the canonical SQLite auth store:
Observed exit code
1and one structured warning forcore/doctor/auth-profileswith:Legacy override source CLI proof was also run with isolated temp state/config and a legacy
openai-codexoverride. It produced one structured warning atmodels.providers.openai-codex, as expected for the config override finding.Default lint was also probed with isolated state. The worktree was noisy from unrelated fresh-checkout missing gateway/skills/UI state, but the auth-profile check was not included by default, matching
defaultEnabled: falseand the contribution tests.Codex review after the path fix:
Result: no accepted/actionable findings; the review reported that the change consistently switches auth profile health findings from the legacy JSON path to the SQLite auth store display path.
Known validation note: a broader test-source
tsgoprobe reached unrelated missing optional dependency/type errors in ACP/MCP/fs-safe test files. The local test-source type issue introduced by this branch was fixed;pnpm tsgo:coreand focused tests passed.Commit note: this linked worktree repeatedly lost
node_modulesafter source CLI/build helper paths. The commit was amended with--no-verifyafter the explicit validation above passed.Post-rebase note: rebased cleanly after #97075 merged to head
c90657345e546f14b925e4856400ac07bab5221d; diff unchanged at 5 files, +377/-13. Post-rebase hosted checks are green/skipped, including Real behavior proof.