refactor(storage): remove the leftover llm-connections.json store - #3117
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughProblem solvedThis PR removes the unused Desktop E2E fixtures and the Computer Use script now use the catalog and credential vault. Existing Source of truthThe PR extends the existing Runtime Policy path. It does not create a parallel connection path. It removes:
The catalog format, credential vault, and Host bootstrap remain unchanged. Scope and complexityThe changes are the smallest coherent solution for removing the obsolete store:
No migration path or parallel abstraction was added. Deletion and simplificationThe deleted implementation, helper, export, and tests can remain removed because their APIs no longer exist. The E2E fixture no longer serializes legacy connections or stores unused metadata. The added fixture test covers catalog seeding, connection slugs, default targets, fetched model metadata, credential status, cleanup, and the absence of legacy connection files. Validation and risksReported validation includes clean Biome checks, successful typechecks, passing Desktop tests, and a storage test suite with one unrelated flaky failure. Required-check status remains unverified without direct check results. Review-relevant risksThe PR removes the public The PR changes Desktop E2E fixtures and Computer Use credential setup. These changes affect test and automation workflows and require independent human review under repository policy. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughThe legacy connection store and public export are removed. Desktop fixtures and computer-use setup now use Runtime Policy storage. E2E scenarios use ChangesConnection catalog migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR removes an unused secondary connection store and updates fixtures and scripts to use the existing catalog and credential vault. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant FixtureOrScript
participant InteractiveStorageRoot
participant RuntimePolicyCatalog
participant CredentialVault
FixtureOrScript->>InteractiveStorageRoot: acquire storage lease
FixtureOrScript->>RuntimePolicyCatalog: create or update catalog entry
FixtureOrScript->>CredentialVault: store API key when required
FixtureOrScript->>RuntimePolicyCatalog: set default target
RuntimePolicyCatalog-->>FixtureOrScript: return commit result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/__tests__/connection-catalog-fixture.test.ts (1)
16-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete two redundant statements.
Line 16 resolves the storage root and discards the capability.
writeConnectionsresolves the root itself, so this call adds no setup. Line 27 is unreachable:assert.okfromnode:assert/strictis typed as an assertion function, soowneris already narrowed after line 26.♻️ Proposed simplification
- await resolveStorageRoot({ path: workspaceRoot, kind: 'interactive' }); await writeConnections(workspaceRoot, Date.now(), 'turn-narrative'); @@ const owner = await tryAcquireInteractiveRootOwner(capability); assert.ok(owner); - if (!owner) return; try {As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8e09ac-910c-4c3f-b324-2715796b4274
📒 Files selected for processing (11)
README.mdREADME.zh-CN.mdapps/desktop/src/main/__tests__/connection-catalog-fixture.test.tsapps/desktop/src/main/e2e-fixture/scenarios-settings.tspackages/cli/src/workspace-root.tspackages/storage/package.jsonpackages/storage/src/__tests__/connection-store.test.tspackages/storage/src/connection-store.tspackages/storage/src/index.tspackages/storage/src/relay-profile-store.tsscripts/computer-use/real-model.mjs
💤 Files with no reviewable changes (5)
- packages/storage/src/relay-profile-store.ts
- packages/storage/src/index.ts
- packages/storage/src/connection-store.ts
- packages/storage/src/tests/connection-store.test.ts
- packages/storage/package.json
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Delete FileConnectionStore, its relay-profile helper, and the package export. Desktop e2e and the Computer Use real-model script now seed connection-catalog.json through the Runtime Policy writer. Existing user llm-connections.json files stay inert on disk. Fixes apache#3107 Generated-by: Grok
Throw if the fixture cannot resolve the zai-live connection id, and drop the unused extra root resolve in the catalog test. Generated-by: Grok
apache#2688 imported Plan Mode locale types from the removed @maka/core root barrel. Other locale catalogs already use @maka/core/ui-locale and @maka/core/plan. The root import breaks desktop main typecheck and cascades through every PR that merges with main. Generated-by: Grok
7a0ca28 to
7de59f4
Compare
|
Fast-path merge This change qualifies for the self-merge fast path: it is low impact and easy to reverse (removes the leftover The latest head was independently reviewed by a read-only subagent (ollama-cloud/deepseek-v4-flash:high) with no open P0/P1/P2 findings. Human contributor @yuhan reviewed the final diff and chose the fast path. |
Summary
Connection catalog authority already lives in Runtime Policy (
connection-catalog.json).FileConnectionStorestill wrote a second file,llm-connections.json, that Host never reads.This PR removes that leftover store:
connection-store.ts,relay-profile-store.ts, their tests, and the./connection-storeexportwriteConnectionsnow creates catalog rows throughopenInteractiveRuntimePolicyStoresForWritescripts/computer-use/real-model.mjscopies or seedsconnection-catalog.json+ the credential vault instead ofllm-connections.jsonconnection-catalog.jsonas the current layoutExisting user
llm-connections.jsonfiles stay on disk as inert data. Maka does not import them.Fixes #3107
Verification
llm-connections.json(archive walkthrough only)npx biome checkon the changed source files — cleannpm --workspace @maka/storage run typecheck— cleannpm --workspace @maka/storage run test:dist— 778 pass / 1 fail / 14 skip. The failure isrejects a second authority for the same storage root in another processinmanaged-dependency-environment-crash. Isolated rerun still flakes; not this changenpm --workspace @maka/desktop run typecheck— cleannpm --workspace @maka/desktop run test:dist— 863/863Not run: full desktop e2e suite; Computer Use live real-model script
AI use
Select exactly one:
Tool(s) and scope:
Grok authored the deletion, fixture/script rewrite, and this PR description.
Checklist
Does this PR entail a change in behavior?