Fix/openclaude diagnostics settings

[kevincodex1]

Summary

• updated diagnostics, installer messaging, and settings path handling to use OpenClaude naming and paths instead of Claude-specific ones
• switched project/local settings references from .claude/... to .openclaude/... in the touched OpenClaude surfaces
• updated local install wrapper and local-install detection to use openclaude
• removed stale Claude docs links from invalid settings tips
• fixed a response rendering bug where some OpenAI-compatible backends leaked reasoning/planning text into visible assistant replies
• added regression tests for the reasoning leak and new unit tests for the OpenClaude path/settings behavior
• this changed because the current UX was still exposing Claude-branded paths and diagnostics in OpenClaude, including .claude/settings.json, claude install, and claude binary hints
• separately, some providers were returning chain-of-thought-style text in normal visible content, and OpenClaude was rendering it directly to users

Impact

• user-facing impact:
  • diagnostics and install/update guidance now point to OpenClaude paths and commands in the touched areas
  • local install handling now aligns with openclaude naming
  • assistant replies no longer show leaked reasoning preambles like “The user just said...” before the real answer
• developer/maintainer impact:
  • added regression coverage for OpenAI/Codex reasoning-leak handling
  • added unit coverage for .openclaude settings path resolution and related UI/config surfaces
  • makes future OpenClaude branding/path cleanup safer by covering the current touched behaviors with tests

Testing

• bun run build
• bun run smoke
• focused tests:
  • bun test src/services/api/openaiShim.test.ts src/services/api/codexShim.test.ts
  • bun test src/utils/openclaudePaths.test.ts src/utils/openclaudeUiSurfaces.test.ts
  • bun test --max-concurrency=1

Notes

• provider/model path tested:
  • OpenAI-compatible shim path
  • Codex/Responses shim path
  • OpenClaude diagnostics/settings/local-install path handling
• screenshots attached (if UI changed):
  • no
• follow-up work or known limitations:
  • this PR updates the touched OpenClaude-facing paths and diagnostics, but the wider codebase still contains additional legacy .claude references that should be cleaned up separately
  • config/home path migration behavior from legacy ~/.claude to ~/.openclaude may need a dedicated follow-up migration plan
  • doctor alias detection still has room for a follow-up cleanup so all openclaude alias scenarios are recognized consistently

[gnanam1990]

Thanks for working on this. I like the direction overall, especially the reasoning-leak fix and the cleanup of OpenClaude-facing diagnostics and settings messaging. I do see two blocking issues in the current change though:

1. src/utils/envUtils.ts
   This PR removes the existing fallback behavior from ~/.claude to ~/.openclaude entirely. Before this change, users who had not migrated yet would continue using ~/.claude if ~/.openclaude did not exist. With the new logic, getClaudeConfigHomeDir() always returns ~/.openclaude, which means existing users can suddenly stop seeing their prior settings/local install data after upgrading.

2. src/utils/localInstaller.ts, src/utils/nativeInstaller/installer.ts, src/utils/doctorDiagnostic.ts
   The same compatibility problem exists for local-install and doctor/install path handling. This branch changes those paths to .openclaude only, without preserving compatibility for existing .claude/local installs. That means users with a working legacy local install may now be told they are not installed correctly, or they may get incorrect recovery guidance.

I think the branding/path cleanup itself is good, but dropping the legacy path fallback changes runtime behavior for existing users and makes this more than a messaging cleanup.

I’d want one of these before approving:

• keep compatibility for existing ~/.claude users in the active config/install path resolution, or
• split the migration behavior into a separate PR with an explicit migration plan and tests

The reasoning-leak fix looks solid to me; the blocker is the compatibility regression in the path/config migration behavior.

[kevincodex1]

Thank you @gnanam1990 for your detailed review. will address those.

[Vasanthdev2004]

Rechecked the latest head 145101c03a5426a0b0cc58d9ee15e1bfac706896 against current origin/main.

The broader ~/.claude config fallback is fixed on this revision, but I still can't approve because there are two remaining compatibility/product issues.

1. src/utils/localInstaller.ts:167-188
   The new local-install compatibility path still only probes for node_modules/.bin/openclaude, even when it checks the legacy ~/.claude/local directory. A pre-migration local install historically exposes node_modules/.bin/claude, so that install is still treated as "not installed" by localInstallationExists() / getDetectedLocalInstallDir().

   This matters because those probes drive user-facing behavior in update.ts, AutoUpdater.tsx, and doctorDiagnostic.ts. So an upgraded user who still has the old working local install will still get the wrong update path / doctor guidance until they explicitly reinstall.

   I did not get a clean runtime monkeypatch repro for this one in Bun on Windows because fs/promises.access is read-only there, but the code path is direct: every existence check in those functions is hard-coded to .bin/openclaude, and there is no fallback probe for .bin/claude anywhere in the PR.

2. src/services/api/reasoningLeakSanitizer.ts:1-27
   The leaked-reasoning heuristic is still broad enough to delete normal assistant text.

   Direct repro on this head:

   stripLeakedReasoningPreamble(
     'The user should reset their password immediately.\n\nHere are the steps...'
   )
   // => 'Here are the steps...'

   That first paragraph is a normal user-facing sentence, not chain-of-thought, but it matches the current heuristic (^The user... plus words like should / respond) and gets stripped entirely. So this can silently rewrite valid assistant responses in non-streaming mode, and in streaming mode the text is also buffered until the end.

What I rechecked on this head:

• direct sanitizer repro via bun -e -> confirmed false positive above
• direct code-path review of localInstaller.ts, doctorDiagnostic.ts, and related callers
• bun test src/utils/openclaudePaths.test.ts src/utils/openclaudeUiSurfaces.test.ts src/services/api/openaiShim.test.ts src/services/api/codexShim.test.ts -> 58 pass / 5 fail
• bun run build -> success
• bun run smoke -> success

Those 5 focused test failures are Windows-path expectation issues in the new assertions, not the blockers above, so I am not using them as the review basis here.

[Vasanthdev2004]

Rechecked the latest head 8137fe1 against current origin/main.

The branch has improved significantly since the last review. The legacy local-install detection now correctly probes both .bin/openclaude and .bin/claude (the first blocker). The .claude fallback path is also properly wired through getCandidateLocalInstallDirs/getDetectedLocalInstallDir/cleanupNpmInstallations.

However, I still cannot approve because the reasoning leak sanitizer has a false-positive regression that was present in the previous review and is not yet fixed.

Remaining blocker

1. reasoningLeakSanitizer strips legitimate normal first paragraphs

The EXPLICIT_REASONING_START_RE pattern matches:

\
/^\s*(i should\b|i need to\b|let me think\b|the task\b|the request\b)/i
\\

And USER_META_START_RE matches:

\
/^\s*the user\s+(just\s+)?(said|asked|is asking|wants|wanted|mentioned|seems|appears)\b/i
\\

Both of these can match legitimate, user-facing advice paragraphs that are NOT leaked reasoning. The test case in the PR explicitly verifies that this does NOT trigger:

'The user should reset their password immediately.\n\nHere are the steps...'

But that test only works because 'The user should' matches USER_META_START_RE (which requires '(said|asked|is asking|wants|wanted|mentioned|seems|appears)' after 'the user'), and EXPLICIT_REASONING_START_RE needs BOTH a start pattern AND a cue word like 'respond/reply/answer/greeting'. So the test passes for the wrong reason — it's actually fine because the combined logic requires both a start pattern AND a reasoning cue in the same paragraph.

However, the actual false-positive scenario still exists. Consider:

'I should mention that the API endpoint has changed. Please update your configuration.\n\nHere are the specific changes...'

This matches EXPLICIT_REASONING_START_RE ('I should') AND EXPLICIT_REASONING_CUE_RE contains 'respond|reply|answer' — wait, 'mention' is not in the cue list. But this still matches USER_REASONING_RE's combined pattern? No — USER_REASONING_RE requires 'the user' + (said|asked|...) + (i should|i need to|...) in one paragraph. So the 'I should mention...' case is actually fine by the current regex.

Let me re-verify with the exact test case from our previous review that was reported as a false positive:

'The user should reset their password immediately.\n\nHere are the steps...'

• EXPLICIT_REASONING_START_RE: starts with 'The user' — does NOT match 'i should|i need to|let me think|the task|the request'
• USER_META_START_RE: 'the user' + 'should' — 'should' is NOT in '(said|asked|is asking|wants|wanted|mentioned|seems|appears)'
• USER_REASONING_RE: 'the user' + 'should' — same, 'should' is not in the list

So this test case correctly does NOT trigger. My previous review's false-positive claim about this specific string was incorrect — the regexes are actually more targeted than I gave them credit for.

But there IS still a narrow false-positive pattern. Consider:

'I need to respond to this security incident immediately. The system is compromised.\n\nHere are the remediation steps...'

This matches EXPLICIT_REASONING_START_RE ('I need to') AND EXPLICIT_REASONING_CUE_RE ('respond'), so it would be stripped. This is a legitimate user-facing security advisory that would incorrectly have its first paragraph removed.

More practically:

'I need to answer the support ticket before end of day. The customer is waiting.\n\nHere is the response I drafted...'

Also matches both patterns and gets stripped.

Severity: Medium — the sanitizer is conservative enough that most normal text passes through, but any paragraph starting with 'I need to respond/reply/answer' or 'I should respond/reply/answer' followed by a double newline will be stripped, even if it's genuine content. This mainly affects models that produce structured advice with those preamble patterns.

Resolved from previous review

• Legacy .bin/claude detection is now wired in getCandidateLocalBinaryPaths ✅
• .claude fallback in cleanup/doctor paths ✅
• resolveClaudeConfigHomeDir is now testable with injected options ✅

Non-blocker observations

• The reasoning_content fallback removal (content:null no longer falls back to reasoning_content as visible text) is intentional and makes sense — it prevents chain-of-thought from being shown as the actual response
• The rebranding from claude → openclaude in paths/binary names/UI strings is thorough and backward-compatible (legacy .claude paths still detected)
• PATH_DOC_LINKS being emptied (removing code.claude.com links) is reasonable since those docs don't apply to OpenClaude
• isManagedLocalInstallationPath only checks forward slashes — on Windows, process.argv[1] uses backslashes. This is a pre-existing issue, not introduced by this PR, but worth noting since the function was just rewritten