fix: refine codex health check dialog flow#70
Conversation
|
@coderabbitai re-review |
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds config and remote model health checks, provider chat validation, improved network probing with IPv4 retry and POST support, a web UI health-check dialog and methods, new CLI health-reporting core, updated model/network utilities, expanded tests and fixtures, and related UI/templates/styles. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as Health-Check Modal
participant App as Web UI App
participant API as Backend API
participant ProbeUtils as Probe Utils
participant Provider as Provider Endpoint
User->>Modal: enter prompt & send
Modal->>App: sendHealthCheckDialogMessage()
App->>API: api('provider-chat-check', { name, prompt })
API->>ProbeUtils: buildModelConversationSpecs()
ProbeUtils-->>API: candidate endpoints
API->>Provider: probeJsonPost(candidate, body)
Provider-->>API: HTTP response
API->>ProbeUtils: extractModelResponseText(response)
ProbeUtils-->>API: { ok, status, reply, durationMs }
API-->>App: result payload
App->>Modal: update messages & lastResult
Modal->>User: display reply and metadata
sequenceDiagram
participant Config as Local Config
participant Health as buildConfigHealthReport
participant ProbeUtils as Model Probe Utils
participant Network as Probe Executor
participant Provider as Provider Server
Config->>Health: buildConfigHealthReport({ remote: true })
Health->>Health: validate provider/model/base_url
Health->>ProbeUtils: buildModelProbeSpecs(provider, model)
ProbeUtils-->>Health: candidate specs
Health->>Network: runRemoteHealthCheck(...)
Network->>Provider: probeJsonPost(candidate1)
Provider-->>Network: response
Network->>Network: parse/extract text or error
Network-->>Health: check result (ok/status/duration/issues)
Health-->>Config: aggregated report { ok, issues, remote }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai re-review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 7406-7415: The code blocks new providers because
readCurrentModels()/provider-current-models.json lacks an entry for a freshly
added provider; update addProviderToConfig to seed that map so the "missing
current model" branch won't trigger. Specifically, when addProviderToConfig adds
a providerName, read and update the same store used by readCurrentModels() to
set currentModels[providerName] to config.model if available, otherwise to a
sensible default (e.g., the provider's default or the first model from that
provider's model list), then write the file back; reference addProviderToConfig,
readCurrentModels, provider-current-models.json and providerName/model variables
to locate and implement the change.
- Around line 7337-7345: The probe is using config.model (currentModel) which
may belong to a different provider; replace that lookup with the per-provider
model resolution used by resolveProviderChatTarget so the probe uses the
provider-specific model. Concretely, call resolveProviderChatTarget(...) (the
same call/site used elsewhere) to get the model for this provider, fall back to
'' if absent, and pass that resolved model into buildModelProbeSpec(provider,
resolvedModel, provider.base_url) instead of using config.model; keep the
existing probeSpec/url/body/apiKey handling unchanged.
In `@tests/e2e/test-health-speed.js`:
- Around line 117-127: The routed-provider block overwrites configPath with
routedConfig but never restores originalConfig, making later tests that reuse
tmpHome order-dependent; fix by saving the originalConfig (existing content of
configPath) before writing routedConfig and wrap the write in a try/finally
where the finally block restores originalConfig back to configPath (use the
existing originalConfig, configPath, and routedConfig variables) so tmpHome is
returned to its prior state after the test.
- Around line 166-176: The test fails to reset the request log before asserting
for the provider-chat-check route, so routedProviderRequests may contain
previous speed-test entries; clear or reinitialize the routedProviderRequests
array (e.g., set routedProviderRequests.length = 0 or call its clear helper)
immediately before calling api('provider-chat-check', ...) and then run the
three asserts (checking conversationResult.ok, conversationResult.reply, and
routedProviderRequests.includes('/project/ym/responses')) to ensure the
assertion only reflects requests made by provider-chat-check.
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 354-360: The test's allowlist order is unsorted and can mismatch
the sorted assertion path; update the array allowedExtraCurrentMethodKeys so its
entries are sorted alphabetically to match extraCurrentMethodKeys (which is
sorted on line 352) — locate allowedExtraCurrentMethodKeys in the test and
reorder the strings (e.g., ensure "sendHealthCheckDialogMessage" comes
before/after "scheduleTouchNavIntentReset" according to alphabetical order) so
the strict equality assertion passes.
In `@web-ui/modules/app.methods.codex-config.mjs`:
- Around line 392-426: The health-check currently treats any non-error payload
as success; tighten the guard in the provider-chat-check handling (around
api('provider-chat-check'), hasResponseError, getResponseMessage) to require an
explicit success shape before pushing a success message: only treat as success
when res && res.ok === true and a valid response body exists (e.g., typeof
res.reply === 'string' && res.reply.trim().length > 0 or a defined success
flag/model/preview as your API guarantees); otherwise, treat as failure by
composing an error message with getResponseMessage(res, '健康检测失败'), push the
failure entry to this.healthCheckDialogMessages and call this.showMessage(...,
'error'). Ensure you also set sensible defaults for status, durationMs, model,
rawPreview using the same Number.isFinite/typeof checks already in place so
unexpected objects (e.g., {}) do not get treated as success.
In `@web-ui/partials/index/modal-health-check.html`:
- Around line 15-24: The labels for the provider select and the dialog textarea
are not associated with their controls; add explicit id attributes to the select
and textarea (e.g., "health-check-provider-select" and
"health-check-message-textarea") and update the corresponding <label> elements
to use for="..." so they reference those ids; ensure you keep the existing
v-model bindings (healthCheckDialogSelectedProvider and the textarea's v-model)
and any disabled bindings (healthCheckDialogLockedProvider /
healthCheckDialogSending) intact, and use unique ids if the modal can be
instantiated multiple times.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1209c97-c78c-4f16-83c2-712d3852ae9b
📒 Files selected for processing (23)
cli.jscli/config-health.jslib/cli-models-utils.jslib/cli-network-utils.jspackage.jsontests/e2e/helpers.jstests/e2e/recent-health.e2e.jstests/e2e/run.jstests/e2e/test-health-speed.jstests/unit/agents-modal-guards.test.mjstests/unit/cli-network-utils.test.mjstests/unit/config-health-module.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/provider-chat-utils.test.mjstests/unit/run.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/app.methods.codex-config.mjsweb-ui/partials/index/modal-health-check.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles.cssweb-ui/styles/health-check-dialog.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 GitHub Actions: ci
tests/unit/web-ui-behavior-parity.test.mjs
[error] 344-344: web-ui behavior parity test failed: captured bundled app skeleton exposed unexpected extra data keys versus parity baseline commit e5d3b2a. Unexpected extra keys: 'healthCheckDialogLastResult', 'healthCheckDialogLockedProvider', 'healthCheckDialogMessages', 'healthCheckDialogPrompt', 'healthCheckDialogSelectedProvider', 'healthCheckDialogSending', 'showHealthCheckDialog'; missing from baseline: 'claudeModelsRequestSeq', 'codexModelsRequestSeq', 'providerSwitchDisplayTarget', 'sessionPathRequestSeqMap'.
🪛 HTMLHint (1.9.2)
web-ui/partials/index/modal-health-check.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (26)
package.json (1)
9-20: LGTM!Adding
"cli/"to thefilesarray correctly ensures the newcli/config-health.jsmodule is included in the published npm package.lib/cli-network-utils.js (3)
4-9: LGTM!The retry heuristic correctly identifies network-level failures (timeout, unreachable, DNS issues) where IPv4 fallback may succeed on dual-stack systems.
55-103: LGTM!The probing logic with IPv4 retry fallback is well-structured. Early error returns for invalid URLs/protocols appropriately omit
durationMssince no network request occurs, and consumers handle this withNumber.isFinitechecks.
105-159: LGTM!The POST probing implementation correctly mirrors the GET logic with proper Content-Length calculation and IPv4 retry support.
lib/cli-models-utils.js (3)
85-122: LGTM!The URL candidate generation logic correctly handles various base URL patterns, preferring versioned endpoints for root paths and direct paths for URLs already containing version segments.
168-205: LGTM!The conversation spec builder properly adapts request bodies per
wire_apitype and includes thewireApifield for response handling downstream.
207-281: LGTM!The text extraction utilities handle multiple response payload shapes robustly, with de-duplication preventing repeated content from parallel extraction paths.
cli/config-health.js (3)
55-97: LGTM!The early-exit validation with structured issue reporting provides clear feedback when remote probing cannot proceed.
109-132: LGTM!The probe iteration correctly tries multiple URL candidates, continuing on failure/404 until finding a working endpoint or exhausting candidates. The
Number.isFiniteguards handle missingdurationMsfrom early-exit error paths inprobeJsonPost.
189-229: LGTM!The dependency injection pattern enables clean testing, and the early-exit handling for config parse/read failures prevents cascading errors.
web-ui/styles.css (1)
10-10: LGTM!The new health-check dialog stylesheet is appropriately imported after the core modal styles.
web-ui/index.html (1)
23-23: LGTM!The health-check modal partial is correctly included alongside other modal templates.
tests/unit/run.mjs (1)
25-26: LGTM!The new unit test modules for network utilities and config health are correctly integrated into the test runner.
tests/e2e/run.js (2)
58-91: LGTM!The routed provider mock correctly configures custom paths (
/project/ym/modelsand/project/ym/responses) to test URL routing behavior without automatic/v1injection. TheroutedProviderRequestsexposure enables e2e tests to assert actual request paths.
164-164: LGTM!The routed provider server is properly cleaned up in the finally block alongside other mock servers.
tests/unit/provider-chat-utils.test.mjs (1)
13-24: Good targeted coverage for provider chat utility paths.These tests validate the exact routing and parsing behaviors introduced by this PR without changing API expectations.
Also applies to: 26-43
tests/unit/config-tabs-ui.test.mjs (1)
12-12: UI regression assertions are well aligned with the new health-check dialog flow.The updated expectations and negative guards provide strong protection for the copy, action wiring, and modal partial integration.
Also applies to: 35-35, 47-47, 64-65, 177-177, 180-180, 216-219
web-ui/app.js (1)
63-63: State additions are consistent with the new dialog lifecycle.The newly introduced keys align with modal bindings and method mutations, with sane initial values.
Also applies to: 196-201
tests/unit/web-ui-behavior-parity.test.mjs (1)
325-334: Parity data-key allowlist update looks correct.This aligns the test with the newly introduced health-check dialog state and prevents false drift failures.
tests/e2e/recent-health.e2e.js (1)
7-11: E2E updates are consistent with dynamic porting and new remote health payload shape.The new assertions are coherent with the provider health-check flow introduced in this PR.
Also applies to: 20-20, 80-80, 87-87, 128-140
tests/unit/cli-network-utils.test.mjs (1)
14-63: Retry-path test is solid and properly isolated.This is a good verification of the timeout-triggered IPv4 fallback path, with clean restoration of the stubbed transport.
tests/unit/agents-modal-guards.test.mjs (1)
183-230: New guard tests cover the critical dialog behaviors well.These additions strengthen protection around remote payload handling, provider lock state, and transcript send flow.
Also applies to: 232-260, 262-308
web-ui/styles/health-check-dialog.css (1)
1-144: Styles are coherent with the new dialog markup and states.Class coverage for controls, message variants, and result badges is complete and consistent.
cli.js (3)
45-53: Nice extraction boundary.Pulling the health/network helpers into dedicated modules while keeping the wrapper surface unchanged makes this path easier to reason about.
Also applies to: 68-68, 2824-2827
7365-7382: Good defensive payload parsing.Handling both
error: "..."anderror: { message/code }covers the common provider error shapes.
7514-7540: Result normalization and route wiring look consistent.Returning the same
{ ok, status, durationMs, error }shape for GET and POST probes keeps the handler simple, and the new API action matches the fields consumed by the dialog UI.Also applies to: 10398-10403
| const currentModel = typeof config.model === 'string' ? config.model.trim() : ''; | ||
| const probeSpec = buildModelProbeSpec(provider, currentModel, provider.base_url); | ||
| if (probeSpec && probeSpec.url) { | ||
| return { | ||
| method: 'POST', | ||
| url: probeSpec.url, | ||
| body: probeSpec.body, | ||
| apiKey: provider.preferred_auth_method || '' | ||
| }; |
There was a problem hiding this comment.
Probe the selected provider with its own model.
config.model belongs to the active provider. When params.name points at a different provider, this POST probe can send the wrong model and report a healthy provider as failed. resolveProviderChatTarget() below already does the per-provider model lookup correctly.
💡 Minimal fix
- const currentModel = typeof config.model === 'string' ? config.model.trim() : '';
- const probeSpec = buildModelProbeSpec(provider, currentModel, provider.base_url);
+ const currentModels = readCurrentModels();
+ const savedModel = currentModels && typeof currentModels[params.name] === 'string'
+ ? currentModels[params.name].trim()
+ : '';
+ const activeProvider = typeof config.model_provider === 'string' ? config.model_provider.trim() : '';
+ const activeModel = typeof config.model === 'string' ? config.model.trim() : '';
+ const probeModel = savedModel || (activeProvider === params.name ? activeModel : '');
+ const probeSpec = buildModelProbeSpec(provider, probeModel, provider.base_url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 7337 - 7345, The probe is using config.model
(currentModel) which may belong to a different provider; replace that lookup
with the per-provider model resolution used by resolveProviderChatTarget so the
probe uses the provider-specific model. Concretely, call
resolveProviderChatTarget(...) (the same call/site used elsewhere) to get the
model for this provider, fall back to '' if absent, and pass that resolved model
into buildModelProbeSpec(provider, resolvedModel, provider.base_url) instead of
using config.model; keep the existing probeSpec/url/body/apiKey handling
unchanged.
| const currentModels = readCurrentModels(); | ||
| const savedModel = currentModels && typeof currentModels[providerName] === 'string' | ||
| ? currentModels[providerName].trim() | ||
| : ''; | ||
| const activeProvider = typeof config.model_provider === 'string' ? config.model_provider.trim() : ''; | ||
| const activeModel = typeof config.model === 'string' ? config.model.trim() : ''; | ||
| const model = savedModel || (activeProvider === providerName ? activeModel : ''); | ||
| if (!model) { | ||
| return { error: `Provider ${providerName} missing current model` }; | ||
| } |
There was a problem hiding this comment.
Newly added providers can get stuck behind missing current model.
This branch requires every non-active provider to already have an entry in provider-current-models.json. The dialog caller only sends { name, prompt }, and addProviderToConfig() in this file does not seed that map, so a provider added through the Web UI cannot be chat-checked until the user manually switches to it first.
💡 Suggested follow-up near addProviderToConfig()
try {
writeConfig(newContent);
} catch (e) {
return { error: `写入配置失败: ${e.message}` };
}
+
+ const currentModels = readCurrentModels();
+ if (!currentModels[name]) {
+ currentModels[name] = readModels()[0] || '';
+ writeCurrentModels(currentModels);
+ }
return { success: true };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 7406 - 7415, The code blocks new providers because
readCurrentModels()/provider-current-models.json lacks an entry for a freshly
added provider; update addProviderToConfig to seed that map so the "missing
current model" branch won't trigger. Specifically, when addProviderToConfig adds
a providerName, read and update the same store used by readCurrentModels() to
set currentModels[providerName] to config.model if available, otherwise to a
sensible default (e.g., the provider's default or the first model from that
provider's model list), then write the file back; reference addProviderToConfig,
readCurrentModels, provider-current-models.json and providerName/model variables
to locate and implement the change.
| const routedConfig = [ | ||
| 'model_provider = "routed"', | ||
| 'model = "e2e-model"', | ||
| '', | ||
| '[model_providers.routed]', | ||
| `base_url = "${routedProviderUrl}"`, | ||
| 'wire_api = "responses"', | ||
| 'preferred_auth_method = "sk-routed"', | ||
| '' | ||
| ].join('\n'); | ||
| require('fs').writeFileSync(configPath, routedConfig, 'utf-8'); |
There was a problem hiding this comment.
Restore config.toml after the routed-provider block.
This write never restores originalConfig, so any later e2e case that reuses tmpHome inherits the routed provider config and becomes order-dependent. The earlier invalid-config section already uses the right try/finally pattern; this block needs the same cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-health-speed.js` around lines 117 - 127, The routed-provider
block overwrites configPath with routedConfig but never restores originalConfig,
making later tests that reuse tmpHome order-dependent; fix by saving the
originalConfig (existing content of configPath) before writing routedConfig and
wrap the write in a try/finally where the finally block restores originalConfig
back to configPath (use the existing originalConfig, configPath, and
routedConfig variables) so tmpHome is returned to its prior state after the
test.
| const conversationResult = await api('provider-chat-check', { | ||
| name: 'routed', | ||
| prompt: '请回复连接正常' | ||
| }, 5000); | ||
| assert(conversationResult.ok === true, 'provider-chat-check should succeed'); | ||
| assert(conversationResult.reply === 'routed provider is healthy', 'provider-chat-check should parse assistant text'); | ||
| assert( | ||
| routedProviderRequests.includes('/project/ym/responses'), | ||
| 'provider-chat-check should hit the direct routed responses endpoint' | ||
| ); | ||
|
|
There was a problem hiding this comment.
Reset the request log before asserting the chat-check route.
routedProviderRequests still contains the speed-test hit from the previous block, so the /project/ym/responses assertion can pass even if provider-chat-check never used that route.
🔍 Tighten the route assertion
- const conversationResult = await api('provider-chat-check', {
+ routedProviderRequests.length = 0;
+ const conversationResult = await api('provider-chat-check', {
name: 'routed',
prompt: '请回复连接正常'
}, 5000);
assert(conversationResult.ok === true, 'provider-chat-check should succeed');
assert(conversationResult.reply === 'routed provider is healthy', 'provider-chat-check should parse assistant text');
assert(
routedProviderRequests.includes('/project/ym/responses'),
'provider-chat-check should hit the direct routed responses endpoint'
);
+ assert(
+ !routedProviderRequests.includes('/project/ym/v1/responses'),
+ 'provider-chat-check should not inject /v1 before direct routed responses'
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-health-speed.js` around lines 166 - 176, The test fails to
reset the request log before asserting for the provider-chat-check route, so
routedProviderRequests may contain previous speed-test entries; clear or
reinitialize the routedProviderRequests array (e.g., set
routedProviderRequests.length = 0 or call its clear helper) immediately before
calling api('provider-chat-check', ...) and then run the three asserts (checking
conversationResult.ok, conversationResult.reply, and
routedProviderRequests.includes('/project/ym/responses')) to ensure the
assertion only reflects requests made by provider-chat-check.
| const res = await api('provider-chat-check', { | ||
| name: provider, | ||
| prompt | ||
| }); | ||
| this.healthCheckDialogLastResult = res; | ||
|
|
||
| if (hasResponseError(res) || res.ok === false) { | ||
| const message = getResponseMessage(res, '健康检测失败'); | ||
| this.healthCheckDialogMessages.push({ | ||
| id: `assistant-${Date.now()}`, | ||
| role: 'assistant', | ||
| text: message, | ||
| ok: false, | ||
| status: Number.isFinite(res && res.status) ? res.status : 0, | ||
| durationMs: Number.isFinite(res && res.durationMs) ? res.durationMs : 0, | ||
| model: typeof (res && res.model) === 'string' ? res.model : '', | ||
| rawPreview: typeof (res && res.rawPreview) === 'string' ? res.rawPreview : '' | ||
| }); | ||
| this.showMessage(message, 'error'); | ||
| return; | ||
| } | ||
|
|
||
| const reply = typeof res.reply === 'string' && res.reply.trim() | ||
| ? res.reply.trim() | ||
| : '已收到响应，但未解析到可展示文本。'; | ||
| this.healthCheckDialogMessages.push({ | ||
| id: `assistant-${Date.now()}`, | ||
| role: 'assistant', | ||
| text: reply, | ||
| ok: true, | ||
| status: Number.isFinite(res.status) ? res.status : 0, | ||
| durationMs: Number.isFinite(res.durationMs) ? res.durationMs : 0, | ||
| model: typeof res.model === 'string' ? res.model : '', | ||
| rawPreview: typeof res.rawPreview === 'string' ? res.rawPreview : '' | ||
| }); |
There was a problem hiding this comment.
Require an explicit success shape before showing a provider reply.
Only explicit error payloads and ok === false count as failure here. {} or another unexpected truthy payload can still fall through to the success branch and make the dialog report a healthy reply.
🛡️ Tighten the success guard
const res = await api('provider-chat-check', {
name: provider,
prompt
});
this.healthCheckDialogLastResult = res;
- if (hasResponseError(res) || res.ok === false) {
+ if (!res || typeof res !== 'object' || hasResponseError(res) || res.ok !== true) {
const message = getResponseMessage(res, '健康检测失败');
this.healthCheckDialogMessages.push({
id: `assistant-${Date.now()}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await api('provider-chat-check', { | |
| name: provider, | |
| prompt | |
| }); | |
| this.healthCheckDialogLastResult = res; | |
| if (hasResponseError(res) || res.ok === false) { | |
| const message = getResponseMessage(res, '健康检测失败'); | |
| this.healthCheckDialogMessages.push({ | |
| id: `assistant-${Date.now()}`, | |
| role: 'assistant', | |
| text: message, | |
| ok: false, | |
| status: Number.isFinite(res && res.status) ? res.status : 0, | |
| durationMs: Number.isFinite(res && res.durationMs) ? res.durationMs : 0, | |
| model: typeof (res && res.model) === 'string' ? res.model : '', | |
| rawPreview: typeof (res && res.rawPreview) === 'string' ? res.rawPreview : '' | |
| }); | |
| this.showMessage(message, 'error'); | |
| return; | |
| } | |
| const reply = typeof res.reply === 'string' && res.reply.trim() | |
| ? res.reply.trim() | |
| : '已收到响应，但未解析到可展示文本。'; | |
| this.healthCheckDialogMessages.push({ | |
| id: `assistant-${Date.now()}`, | |
| role: 'assistant', | |
| text: reply, | |
| ok: true, | |
| status: Number.isFinite(res.status) ? res.status : 0, | |
| durationMs: Number.isFinite(res.durationMs) ? res.durationMs : 0, | |
| model: typeof res.model === 'string' ? res.model : '', | |
| rawPreview: typeof res.rawPreview === 'string' ? res.rawPreview : '' | |
| }); | |
| const res = await api('provider-chat-check', { | |
| name: provider, | |
| prompt | |
| }); | |
| this.healthCheckDialogLastResult = res; | |
| if (!res || typeof res !== 'object' || hasResponseError(res) || res.ok !== true) { | |
| const message = getResponseMessage(res, '健康检测失败'); | |
| this.healthCheckDialogMessages.push({ | |
| id: `assistant-${Date.now()}`, | |
| role: 'assistant', | |
| text: message, | |
| ok: false, | |
| status: Number.isFinite(res && res.status) ? res.status : 0, | |
| durationMs: Number.isFinite(res && res.durationMs) ? res.durationMs : 0, | |
| model: typeof (res && res.model) === 'string' ? res.model : '', | |
| rawPreview: typeof (res && res.rawPreview) === 'string' ? res.rawPreview : '' | |
| }); | |
| this.showMessage(message, 'error'); | |
| return; | |
| } | |
| const reply = typeof res.reply === 'string' && res.reply.trim() | |
| ? res.reply.trim() | |
| : '已收到响应，但未解析到可展示文本。'; | |
| this.healthCheckDialogMessages.push({ | |
| id: `assistant-${Date.now()}`, | |
| role: 'assistant', | |
| text: reply, | |
| ok: true, | |
| status: Number.isFinite(res.status) ? res.status : 0, | |
| durationMs: Number.isFinite(res.durationMs) ? res.durationMs : 0, | |
| model: typeof res.model === 'string' ? res.model : '', | |
| rawPreview: typeof res.rawPreview === 'string' ? res.rawPreview : '' | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.codex-config.mjs` around lines 392 - 426, The
health-check currently treats any non-error payload as success; tighten the
guard in the provider-chat-check handling (around api('provider-chat-check'),
hasResponseError, getResponseMessage) to require an explicit success shape
before pushing a success message: only treat as success when res && res.ok ===
true and a valid response body exists (e.g., typeof res.reply === 'string' &&
res.reply.trim().length > 0 or a defined success flag/model/preview as your API
guarantees); otherwise, treat as failure by composing an error message with
getResponseMessage(res, '健康检测失败'), push the failure entry to
this.healthCheckDialogMessages and call this.showMessage(..., 'error'). Ensure
you also set sensible defaults for status, durationMs, model, rawPreview using
the same Number.isFinite/typeof checks already in place so unexpected objects
(e.g., {}) do not get treated as success.
| <label class="form-label">提供商</label> | ||
| <select | ||
| v-model="healthCheckDialogSelectedProvider" | ||
| class="form-select" | ||
| :disabled="!!healthCheckDialogLockedProvider || healthCheckDialogSending"> | ||
| <option value="" disabled>请选择提供商</option> | ||
| <option v-for="provider in displayProvidersList" :key="'health-check-provider-' + provider.name" :value="provider.name"> | ||
| {{ provider.name }} | ||
| </option> | ||
| </select> |
There was a problem hiding this comment.
Associate these labels with their controls.
Both labels are detached from the <select> / <textarea>, so assistive tech will not expose a programmatic name for either field inside the dialog.
♿ Minimal fix
- <label class="form-label">提供商</label>
+ <label class="form-label" for="health-check-dialog-provider">提供商</label>
<select
+ id="health-check-dialog-provider"
v-model="healthCheckDialogSelectedProvider"
class="form-select"
:disabled="!!healthCheckDialogLockedProvider || healthCheckDialogSending">
...
- <label class="form-label">消息</label>
+ <label class="form-label" for="health-check-dialog-prompt">消息</label>
<textarea
+ id="health-check-dialog-prompt"
v-model="healthCheckDialogPrompt"
class="form-input health-check-dialog-textarea"Also applies to: 54-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/partials/index/modal-health-check.html` around lines 15 - 24, The
labels for the provider select and the dialog textarea are not associated with
their controls; add explicit id attributes to the select and textarea (e.g.,
"health-check-provider-select" and "health-check-message-textarea") and update
the corresponding <label> elements to use for="..." so they reference those ids;
ensure you keep the existing v-model bindings (healthCheckDialogSelectedProvider
and the textarea's v-model) and any disabled bindings
(healthCheckDialogLockedProvider / healthCheckDialogSending) intact, and use
unique ids if the modal can be instantiated multiple times.
|
@coderabbitai re-review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the actual code changes for concrete regression risks. 🏁 Script executed: Length of output: 24877 Now let me check 🏁 Script executed: Length of output: 3006 🏁 Script executed: Length of output: 5292 🏁 Script executed: Length of output: 868 🏁 Script executed: Length of output: 4246
Regression AssessmentNo unintended regressions found. The specific behavioral changes are all intentional and covered by the new unit and e2e tests. Details below:
One Minor Correctness Note (non-blocking)In size += chunk.length;
if (size <= options.maxBytes) {
chunks.push(chunk);
}
// response is never res.destroy()'dFor responses exceeding 256 KB this wastes bandwidth and could yield a truncated body that fails JSON parsing. For health-check probe payloads (model API responses) this threshold is effectively unreachable in practice, so it won't cause a live regression — but it is worth noting as a latent issue. Bottom line: Safe to merge. No regressions are introduced. 🧠 Learnings used |
|
@coderabbitai re-review |
|
If you merge this PR directly, it will not introduce any regressions. There are no impacted issues to report. All behavioral changes are intentional and covered by the new unit and e2e tests added in this PR. The one latent issue previously noted ( 🧠 Learnings used✅ Actions performedReview triggered.
|
SurviveM
dismissed
coderabbitai[bot]’s stale review
Force-skipped requested changes for this PR per maintainer decision.
ymkiux
left a comment
ymkiux
left a comment
There was a problem hiding this comment.
Approved to satisfy the required review gate for this PR.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
UI
Tests
Chores