fix(desktop): align source_test metadata contract - #7193
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Thanks for the PR! Template looks good ✓ Problem: Observed bug with linked issue (#7192). The shared source config validator ( Direction: Aligned — fixing a real data contract mismatch between Size: 68 additions, 7 deletions across 3 files. No core module paths touched ( Approach: Scope is tight and minimal — three focused changes: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已确认的 bug，有关联 issue (#7192)。共享 source config 校验器 ( 方向： 对齐——修复 规模： 3 个文件，68 行新增、7 行删除。未触及核心模块路径（仅 方案： 范围紧凑——三个聚焦的改动： 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewClean pass. The diff is focused and correct:
Test ResultsTypecheck ( N/A for tmux before/after — this is an internal metadata persistence fix, not a UI change. The test results above are the verification evidence. 中文说明代码审查干净通过。diff 聚焦且正确：
测试结果10 个测试全部通过，37 个 expect() 断言。Typecheck 干净通过。 tmux before/after 不适用——这是内部元数据持久化修复，非 UI 变更。上述测试结果即为验证证据。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — Clean fix for a verified correctness bug. This is a tight, well-evidenced fix. The shared source config validator rejects non-integer The diff is minimal (12 production lines changed, 56 test lines), no scope creep, and typecheck is clean. 10/10 tests pass. Approving. 中文说明信心度: 5/5 — 经过验证的正确性修复。 这是一个紧凑、有充分证据的修复。共享 source config 校验器拒绝非整数 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| if (!connectionResult.hasError) { | ||
| connectionStatus = 'needs_auth'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test verifies that connectionStatus: 'failed' is preserved when both connection fails AND auth has warnings. The guard !connectionResult.hasError is the only thing preventing 'failed' from being overwritten with 'needs_auth'. — Failure scenario: A source with authType: 'oauth', isAuthenticated: false, and a failing MCP stub exercises both hasError = true (→ 'failed') and authResult.hasWarning = true. If this guard were accidentally removed, the status would incorrectly report 'needs_auth' — misleading the user into re-authenticating when the real issue is connectivity.
| if (!connectionResult.hasError) { | |
| connectionStatus = 'needs_auth'; | |
| } | |
| if (!connectionResult.hasError) { | |
| connectionStatus = 'needs_auth'; | |
| } |
Add a test combining stubMcpFail() with isAuthenticated: false + authType: 'oauth', asserting persisted.connectionStatus === 'failed'.
— qwen3.7-max via Qwen Code /review
| } else { | ||
| connectionStatus = 'disconnected'; | ||
| connectionStatus = 'untested'; |
There was a problem hiding this comment.
[Suggestion] No test covers the 'untested' final status — the fallback branch when testConnection returns { success: false, hasError: false }. All existing tests use MCP sources whose stubs produce either success or error. — Concrete cost: If this fallback logic were changed incorrectly (e.g., a refactor that swapped the else branch), no test would catch it, and sources without a connection test handler would silently get the wrong status.
Add a test with a source configuration that causes testConnection to return { success: false, hasError: false }, asserting persisted.connectionStatus === 'untested'.
— qwen3.7-max via Qwen Code /review
| const SHARED_CONNECTION_STATUSES = new Set<NonNullable<SourceConfig['connectionStatus']>>([ | ||
| 'connected', | ||
| 'needs_auth', | ||
| 'failed', | ||
| 'untested', | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] The assertSharedSourceMetadataContract helper hardcodes the valid status values as a set literal, but TypeScript's Set<T> type does not enforce exhaustiveness — if a new value is added to ConnectionStatus, this set silently becomes a subset and the helper will reject the new valid value at test runtime with a misleading "unsupported connectionStatus" error from the test infrastructure. — Concrete cost: A developer adds a new status value and writes handler code that produces it; the test fails with an infrastructure error that looks like a bug in the handler, not the test helper.
| const SHARED_CONNECTION_STATUSES = new Set<NonNullable<SourceConfig['connectionStatus']>>([ | |
| 'connected', | |
| 'needs_auth', | |
| 'failed', | |
| 'untested', | |
| ]); | |
| const ALL_CONNECTION_STATUSES: ConnectionStatus[] = [ | |
| 'connected', | |
| 'needs_auth', | |
| 'failed', | |
| 'untested', | |
| ] as const; | |
| const SHARED_CONNECTION_STATUSES = new Set<ConnectionStatus>(ALL_CONNECTION_STATUSES); |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
Review:
|
source-test.ts state |
result |
|---|---|
| PR | 10 pass, 0 fail |
origin/main |
4 pass, 6 fail |
origin/main + Date.now() only |
8 pass, 2 fail |
Base failures include expect(persisted.enabled).toBe(true) and the missing auto-enabled in config line — so the test is pinned to the real symptom, and the timestamp change and the status-vocabulary change are each independently load-bearing. Good test-first work.
Should change before merge
untested is the wrong mapping for a probe that ran and came back bad — source-test.ts:130
connectionResult returns success: false, hasError: false only from the API paths: HTTP 404, and any non-OK status other than 401/403 (testApiConnectionWithAuth, testApiConnectionBasic). testMcpConnection and testLocalConnection always set either success or hasError, so that else branch is API-only. Confirmed against a local HTTP server:
>>> HTTP 500: isError=false persisted.connectionStatus="untested" lastTestedAt=number enabled=true
>>> HTTP 404: isError=false persisted.connectionStatus="untested" lastTestedAt=number enabled=true
untested is documented in shared as "Connection has not been tested" (sources/types.ts:414) and the UI renders it as a grey "Not Tested" dot. But we did test it — it returned 500. It also keeps the source out of SourceManager's attention filter (needs_auth || failed, agent/core/source-manager.ts:178), which is exactly where a 500'ing API source belongs.
This is not a regression (base wrote disconnected, equally absent from that filter), but of the four legal values this picks the one meaning "never looked":
} else {
- connectionStatus = 'untested';
+ // Probe ran and came back non-OK (404 / unexpected status) — reachable but not healthy.
+ connectionStatus = 'failed';
+ connectionError = connectionResult.error ?? 'Endpoint returned a non-OK status';
}Pre-existing and separate: that path also leaves hasErrors false, so a 500'ing source still reports ✓ Validation passed and gets auto-enabled. Out of scope for this PR, but worth its own issue.
Follow-ups (not blockers)
- The status vocabulary isn't actually enforced at the save boundary. Per the probe above,
'disconnected'and even'totally-made-up'passvalidateSourceConfig—FolderSourceConfigSchemanever declaresconnectionStatus/connectionErrorat all, and zod's default strip-unknown lets them through. So the status half of this PR is a pure consistency fix with no rejection consequence, andassertSharedSourceMetadataContractin the test is stricter than production. The durable fix belongs in shared: addconnectionStatus: z.enum([...])andconnectionError: z.string().optional()toFolderSourceConfigSchema, so the next drift is caught for real. Keep the test helper either way —shareddepends onsession-tools-coreand not the reverse, so the test genuinely cannot import the real validator. A one-line comment saying so would help, since right now it reads like avoidable duplication. - The
catch {}atsource-test.ts:157is the actual root cause of the silence, and it survives this PR. One line prevents a repeat: push⚠ Could not persist test results: ${msg}intolines, and drop/qualify the_Config updated with test results._line on failure. Today a save failure is indistinguishable from success in the tool output — which is precisely how this shipped broken. ConnectionStatusandSourceConnectionStatusare now near-duplicates, with core's copy omittinglocal_disabled. Harmless today (local_disabledis only computed at render time inderiveConnectionStatus, never persisted), but two hand-maintained unions for one on-disk field will drift. A comment pointing atshared/src/sources/types.ts:417as the source of truth would help.- No migration for stale ISO
lastTestedAtvalues on disk. Exposure is genuinely small — base could never persist one through the app, so only hand-edited configs are affected — andsource_testself-heals them since it overwrites the field. But other save paths (markSourceAuthenticated,credential-manager.ts:348) would keep throwing on such a config. Agreed it's out of scope; noting where it would bite.
Conventions, coverage, risk
- Prettier clean on all three files (
npx prettier --check✓). No new dependencies. No security surface — no credential handling or logging changed. The only exported-type change isConnectionStatus, whose sole consumer in the tree issource-test.tsitself. - Note for reviewers:
ci.ymlruns onlycheck:desktop-isolationagainstpackages/desktop; nothing runsbun testthere. This regression test will not run in PR CI, so a future revert won't turn anything red. Not the author's problem, but don't expect CI to guard it. - Scope matches the description, and the
mainmerge on the branch is clean.
Verification recipe
packages/desktop is excluded from the root npm workspaces, so an isolated worktree needs node_modules linked by hand:
git worktree add /tmp/wt7193 pr7193-head
ln -s /path/to/primary/node_modules /tmp/wt7193/node_modules # zod
# gray-matter / croner / beautiful-mermaid are not in the root tree:
mkdir /tmp/gm && cd /tmp/gm && bun add gray-matter@4.0.3 croner beautiful-mermaid
ln -s /tmp/gm/node_modules /tmp/wt7193/packages/desktop/packages/session-tools-core/node_modules
ln -s /tmp/gm/node_modules /tmp/wt7193/packages/desktop/packages/shared/node_modules
# link each packages/desktop/packages/* under /tmp/gm/node_modules/@craft-agent/ too
cd /tmp/wt7193
bun test packages/desktop/packages/session-tools-core/src/handlers/source-test.test.ts
# A/B: git show origin/main:<handler> > <handler> and re-runThe validator probe imports validateSourceConfig directly and feeds it the base vs. PR config shapes; the untested probe stands up a node:http server returning a fixed status and reads back the persisted config.json.
中文说明
结论
修复方向正确，解决的是一个真实且 100% 必然发生的 bug。建议合并前改一行（untested → failed），其余都是可以后续处理的建议。
已验证
直接调用 shared 的真实 validator（不是复述 PR 描述）：
PR 形状（数字时间戳 + failed） -> valid=true
BASE 形状（ISO 字符串 + error）-> valid=false lastTestedAt: Expected number, received string
FolderSourceConfigSchema 要求 lastTestedAt 为非负整数（validators.ts:492），且 saveSourceConfig 校验失败会 throw（storage.ts:118-123）。所以在 main 上，desktop in-process 路径的 source_test 元数据保存每次都失败，不只是"可能被拒绝"。因此还导致：enabled: true 的 auto-enable 永远不落盘（本次会话可用、重启后又关闭）；SourceInfoPage 的"Last tested"永远显示 common.never；而工具输出仍然打印 ℹ Config updated，属于错误信息。
A/B（保持 PR 的测试文件不变，只替换 handler）：PR = 10 pass；origin/main = 4 pass, 6 fail；origin/main + 只改时间戳 = 8 pass, 2 fail。说明时间戳和状态词汇两部分都是各自 load-bearing 的，回归测试钉在了真实症状上。
建议合并前修改
source-test.ts:130 的 else 分支写 untested 语义不对。该分支只有 API 路径能到达（404 或除 401/403 之外的非 OK 状态）；用本地 HTTP server 实测：HTTP 500 与 404 都持久化成 connectionStatus: "untested"。而 shared 里 untested 的含义是"从未测试过"（sources/types.ts:414），并且会让该 source 落在 SourceManager 的 needs_auth || failed 关注列表之外（source-manager.ts:178）。建议改成 failed 并带上 connectionError。
（另外，该分支不会设置 hasErrors，所以返回 500 的 source 仍会显示 ✓ Validation passed 并被 auto-enable —— 这是既有问题，不属于本 PR 范围，建议单独开 issue。）
后续建议（非阻塞）
- 状态词汇其实没有在保存边界被校验：实测
'disconnected'甚至'totally-made-up'都能通过validateSourceConfig，因为FolderSourceConfigSchema根本没声明connectionStatus/connectionError，zod 默认会 strip 未知字段。所以本 PR 的状态部分只是语义统一，没有"被拒绝"的后果；测试里的assertSharedSourceMetadataContract比生产更严格。真正长效的修法是在 shared 的 schema 里加上这两个字段。测试里的重复实现无法避免（依赖方向是 shared → session-tools-core），建议加一行注释说明。 source-test.ts:157的catch {}才是"静默"的根因，本 PR 没有动它。建议在lines里推一行⚠ Could not persist test results: ...，否则下次契约漂移仍然是无声失败。- core 的
ConnectionStatus与 shared 的SourceConnectionStatus现在几乎重复（少一个local_disabled，该值只在渲染时计算、从不落盘，目前无害），建议加注释指向 shared 作为唯一真源。 - 磁盘上已有的 ISO
lastTestedAt没有迁移。影响面很小（base 根本写不进去，只有手改的配置会有），且source_test会覆盖该字段从而自愈；但markSourceAuthenticated/credential-manager.ts:348等保存路径仍会对这种配置抛错。
规范 / 覆盖 / 风险
三个文件 prettier 全部通过；无新增依赖；无安全面变化。提示： ci.yml 对 packages/desktop 只跑 check:desktop-isolation，没有任何 job 跑 bun test，所以这个回归测试不会在 PR CI 里执行，未来若被回退 CI 不会变红。
|
Thanks for the contribution. Closing because the Electron desktop app ( The related contract question in #7192 still stands; the metadata handling now lives in the CLI/ACP layer. If the misalignment reported there is still observable against the current daemon, please follow up on #7192 with a fresh reproduction. |
|
Correction to my earlier close comment: the related contract issue #7192 is also obsolete. The If the metadata behavior still misbehaves in the current daemon/CLI, a new issue against the current code path would be the right place to investigate. |
4 participants
What this PR does
Aligns
source_testmetadata writes with the shared desktop source config contract. The test timestamp is now stored as a millisecond timestamp, and source test outcomes now use the same connection status vocabulary as the shared source state model. Source connection probing, authentication checks, auto-enable behavior, and session activation behavior stay unchanged.Why it's needed
What Problem This Solves
source_testreports source validation results and then tries to persist metadata such as the latest test time, connection state, connection error, and the auto-enabled flag. In the in-process desktop path, that save goes through shared source storage, which validates source configs before writing them. Before this change,source_testwrotelastTestedAtas an ISO string even though the shared source config contract expects a non-negative integer timestamp. When shared storage rejected that config, the handler swallowed the save error, so the user could see a normal-looking test result while the metadata update was silently dropped.The status vocabulary was also out of sync.
source_testcould write states such asunknown,disconnected, anderror, while the shared source state model usesuntested,needs_auth,failed, andconnected. That makes persisted source state harder for downstream desktop/source UI code to interpret consistently.Changes
The handler now initializes untested state as
untested, maps connection failures tofailed, maps successful connection plus missing auth toneeds_auth, and keeps successful connection state asconnected. The session-tools source config type was updated to the same timestamp and status vocabulary so the handler, tests, and shared source state contract describe the same data shape.The test context now rejects metadata that would not fit the shared source metadata contract, then asserts successful, failed, and needs-auth paths actually persist a numeric
lastTestedAtand the expected connection status. This keeps the regression pinned to the save boundary instead of only checking the user-visible text.Evidence
Before this change, the problematic metadata shape looked like this:
That timestamp shape does not satisfy the shared source config contract, which expects
lastTestedAtto be an integer timestamp. The focused test now simulates the shared save boundary by rejecting non-integer timestamps and unsupported status values before writing the config. The updated test covers clean validation,autoEnable: false, failed connection, and missing-auth outcomes.Focused validation passed locally:
The commit hook also ran Prettier and ESLint on the three staged TypeScript files before the commit was created.
Possible call chain / impact
This PR targets the desktop in-process/shared source save path. Contexts without
saveSourceConfigare unchanged, and this does not change source probing, credential refresh, OAuth triggering, MCP validation, or how session activation is requested after auto-enable.Reviewer Test Plan
How to verify
Run the focused source_test handler test and confirm successful, failed, and missing-auth source test paths persist shared-compatible metadata. A reviewer can also inspect that the persisted
lastTestedAtvalues are numeric timestamps and that connection states are limited toconnected,failed,needs_auth, anduntested.Evidence (Before & After)
N/A - this is a non-UI metadata persistence fix. The before/after behavior is covered by the focused unit test and the shared-save-boundary assertions described above.
Tested on
Environment (optional)
Windows local validation used Bun
1.3.14fromD:\ZXY\Dev\bun\bin\bun.exe. WSL has Node available, but no Linuxbunbinary was available in this environment, so Linux was not claimed as tested.Risk & Scope
source_test; it does not change the connection or auth test mechanics themselves.source_testwrites from adding out-of-contract metadata.Linked Issues
Fixes #7192
中文说明
这个 PR 做了什么
这个 PR 将
source_test写入的 source 测试元数据对齐到 desktop shared source config 的契约。测试时间现在写成毫秒级数字时间戳，连接状态也改成 shared source state 使用的connected、failed、needs_auth、untested。连接探测、认证检查、auto-enable 和 session activation 的行为不变。为什么需要
source_test在完成 source 校验后，会尝试保存lastTestedAt、connectionStatus、connectionError以及可能的enabled: true。在 desktop in-process 路径里，这个保存会经过 shared source storage，而 shared storage 写入前会校验 source config。旧实现把lastTestedAt写成 ISO 字符串，但 shared contract 要求它是非负整数时间戳，所以保存可能被 validator 拒绝。由于source_test会吞掉保存错误，用户可能看到正常的测试结果，但元数据和 auto-enable 状态实际上没有落盘。此外，旧的状态值
unknown、disconnected、error和 shared/UI/source manager 期望的状态语义不一致。这个 PR 把新写入的状态统一为 shared source state 的词汇，避免后续消费者看到不一致的状态。证据与验证
本地 focused test 已覆盖 clean run、
autoEnable: false、连接失败、连接成功但缺少认证这几条路径，并在测试 stub 里模拟 shared save boundary：如果写入非整数lastTestedAt或不支持的状态，就拒绝保存。这样旧的 ISO 时间戳写法会被测试捕获。已运行：
影响范围
这个 PR 只影响 desktop in-process/shared source save path 下
source_test新写入的测试元数据。没有改变 source 连接探测、认证刷新、OAuth 触发、MCP validation 或 session activation 逻辑。已有旧配置里的旧状态值不在本 PR 中迁移。