Skip to content

Qi/code 622 - #480

Open
adminliu-main wants to merge 19 commits into
masterfrom
qi/code-622
Open

Qi/code 622#480
adminliu-main wants to merge 19 commits into
masterfrom
qi/code-622

Conversation

@adminliu-main

Copy link
Copy Markdown

Summary

新增 LinkCode 插件市场:桌面端现在可以浏览官方插件目录、安装插件,并通过
manifest 驱动的设置表单配置插件。关联 CODE-622

  • Daemon —— 新增 marketplace/ 服务(目录拉取与刷新)和
    plugin-store/(暂存式安装、失败自动回滚;插件设置项中的 secret 值写入
    vault),两者均接入控制通道。
  • 客户端 —— 控制通道新增市场相关操作(浏览 / 安装 / 配置),SDK 增加
    对应 operations,workbench 提供 SWR hooks。
  • 设置界面 —— 插件设置新增 LinkCode 页签:目录卡片展示安装状态(含
    "本地已装更新版本"识别);单插件配置弹窗(secret 字段不回显折叠默认值;
    与默认值相等的设置项按删除持久化)。
  • Wire 协议 —— 纯新增帧,WIRE_PROTOCOL_VERSION 78 → 80,
    MIN_COMPATIBLE_WIRE_VERSION 不变。
  • mail 插件移出本仓库 —— 改由官方插件市场仓库分发
    (arcboxlabs/linkcode-plugins-official#1),本 PR 不再内置任何插件;
    本地调试用 scripts/dev-marketplace.mts fixture。

Verification

  • pnpm check:ci —— 通过(format / lint / typecheck,0 错误)
  • pnpm test —— 通过(仅有的 3 个失败是 release-artifact 的已知环境问题,
    master 上同样失败,与本改动无关)
  • apps/daemon/e2e/plugin-marketplace.e2e.ts —— 通过:基于 dev marketplace
    fixture 端到端跑通目录拉取、暂存安装、设置写入(secret 进 vault)与回滚
  • 渲染层由组件/视图测试覆盖(linkcode-config-dialoglinkcode-config
    viewplugin-market);设置界面可用 scripts/dev-marketplace.mts
    在本地实际驱动
  • 三轮 review 意见已在 6373d0cd05b49deb 中处理完毕

Checklist

  • pnpm check:cipnpm test 均通过(无 Rust 改动)
  • 已运行受影响的界面/服务并确认改动生效(daemon e2e + dev marketplace
    fixture)
  • wire 消息有变更:WIRE_PROTOCOL_VERSION 已提升(78 → 80,纯新增,
    兼容地板不变)
  • 新增代码与资产为本人工作成果,或已在上方注明来源与许可兼容性
  • 行为变更处的文档与注释已更新(新增环境变量已写入
    docs/ENVIRONMENT.md

@linear-code

linear-code Bot commented Aug 25, 2026

Copy link
Copy Markdown

CODE-622

@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds an end-to-end LinkCode plugin marketplace, including catalog refresh, verified staged installation, manifest-driven configuration, vault-backed secrets, client operations, and settings UI.

  • Adds daemon marketplace and plugin-store services with rollback and startup reconciliation.
  • Adds shared plugin, marketplace, configuration, and wire contracts.
  • Adds client SDK operations, workbench hooks, catalog/configuration UI, fixtures, and coverage.
  • Restricts MCP component names to provider-compatible injection keys.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/daemon/src/plugin-store/store.ts Implements serialized staged installs, replacement rollback, uninstall tombstones, registry persistence, startup reconciliation, and vault-backed plugin settings; the previously reported replacement and rollback defects are no longer established at HEAD.
packages/foundation/schema/src/model/linkcode-plugin.ts Defines marketplace plugin manifests, settings, artifacts, installed records, and MCP components while enforcing provider-compatible MCP server names.
apps/daemon/src/marketplace/service.ts Implements marketplace refresh, validation, caching, conditional requests, and release resolution.
packages/host/engine/src/session/start-options-resolver.ts Projects installed plugin MCP components into session startup using the validated shared manifest contract.
packages/client/workbench/src/settings/plugins/linkcode-config-dialog.tsx Provides the manifest-driven plugin configuration dialog with masked secret handling.
packages/foundation/schema/src/wire/payload.ts Integrates the additive marketplace and plugin-configuration message families into the shared wire contract.

Sequence Diagram

sequenceDiagram
  participant UI as Plugin Settings UI
  participant Client as Client SDK
  participant Daemon as Daemon Marketplace
  participant Store as Plugin Store
  participant Vault as Secret Vault
  participant Index as Marketplace Index
  UI->>Client: Browse or install plugin
  Client->>Daemon: Marketplace wire request
  Daemon->>Index: Refresh catalog
  Index-->>Daemon: Manifest and artifact metadata
  Daemon->>Store: Install selected release
  Store->>Store: Download, verify, and stage archive
  Store->>Store: Publish package and registry record
  Store-->>Client: Installed plugin
  UI->>Client: Save manifest-driven settings
  Client->>Store: Configuration patch
  Store->>Vault: Persist secret values
  Store->>Store: Persist non-secret values
  Store-->>UI: Masked configuration view
Loading

Reviews (8): Last reviewed commit: "fix(plugin-store): throw on failed pendi..." | Re-trigger Greptile

Comment thread packages/foundation/schema/src/model/linkcode-plugin.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

One reachable data-loss bug (dotted setting ids silently drop from every save), plus two small hardening items and a trust-model question worth answering before this ships wider.

Substantial, well-tested change. I read the full diff end-to-end. Things I checked and found genuinely correct, so nobody has to re-derive them: the wire floor is right (78 → 80 is additive-only, MIN_COMPATIBLE_WIRE_VERSION correctly unmoved); maskValues() fails closed when settings is undefined, unreadable manifests drop the plugin from list() entirely rather than leaking, secret defaults are deliberately not folded in, and the rollback/error paths carry no secret values; node-tar 7.5.22 blocks .., absolute paths, and symlink-redirect by default so there is no extraction escape; the marketplace cache/validator writes are all atomic-rename, so the worst concurrency outcome is one self-correcting extra round-trip, not a stuck catalog; and the en / zh-cn locale additions are key-for-key parallel. The tests are real tests — concrete equality assertions, negative leak assertions (not.toContain('secret upstream response')), deliberately corrupt fixtures — not absorbing snapshots.

Four things below.


Test coverage points at a shape that cannot exist. Both workbench config fixtures key on maxBodyChars. That id has uppercase characters, so isSafeIdSegment (linkcode-plugin.ts:4) would reject it — no plugin can ever declare it. It typechecks only because LinkCodePluginSettings is z.infer of a z.record whose key .refine doesn't narrow the TS key type. So the suite exercises an impossible id while missing the reachable hazard (see the inline comments). Worth switching those fixtures to something like api.key, which would have caught the bug on its own.

Dead forward-compat shim. linkcode-tab.tsx's merge defends against "pre-wire-80 daemons that still return an empty 304 payload." But master is at wire 78, and both 79 and 80 originate inside this PR — 79 never shipped, so no such daemon exists in the wild. The current.releases merge is unreachable defensiveness; I'd drop it and let the 304 response stand on its own.

Trust model — a question, not a finding. Installing a plugin executes whatever command/args/env the on-disk manifest declares, and the catalog UI renders none of those fields. The SRI hash pins the tarball bytes to what the index published, so it protects against a tampering CDN but by construction not against a hostile index. That means adding a marketplace is a grant of arbitrary local code execution, which is a defensible design (it's the npm-registry bargain) — but right now it's only reachable by hand-editing config.json, since there's no add-marketplace wire kind or UI. Before an "add marketplace" affordance ships, it'd be good to have the intended model written down somewhere durable, and to decide whether that UI needs a consent step. Flagging now because the sequencing decision is easier to make while the surface is still config-only.

Files reviewed: 68 · Commits: 4 · Base master · Head qi/code-622 (05b49de) · Prior pullfrog review: none

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/client/workbench/src/settings/plugins/linkcode-config-dialog.tsx Outdated
Comment thread packages/client/workbench/src/settings/plugins/linkcode-config.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts Outdated
Comment thread apps/daemon/src/plugin-store/paths.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes — the 12 files a02615f touched, re-read against the full PR diff.

  • Confirmed all four prior review threads are substantively addressed and resolved them: makePluginTmpDir now mkdirs only the parent and a constructor boot sweep reconciles .tmp-* / .tmp-retired-* siblings; the tar extract runs strict: true; pluginConfigFormKey is applied symmetrically across register / Controller / Field and the pure module.
  • Verified the .$ escape against react-hook-form's installed source rather than memory: node_modules/react-hook-form/dist/index.esm.mjs:91 is const FIELD_PATH_RE = /[.[\]'"]/; and stringToPath splits on exactly that set. $ is not a path separator, and ID_SEGMENT_RE forbids $ in setting ids, so the mapping is injective. The fix is correct.
  • Traced the dropped 304 merge in linkcode-tab.tsx to no regression: usePluginMarketCatalog registers refreshPluginMarketplace as its fetcher, so mutate() still forces the daemon's conditional GET.
  • Walked the new crash-consistency machinery in plugin-store/store.ts: single production construction site (apps/daemon/src/index.ts:190), per-id serialized installs, and one daemon per channel × profile, so there is no sweep-versus-in-flight-install race. Same-version reinstall cannot self-delete (if (previous.path === targetDir) continue;), .tmp-<pid>- can never be misread as .tmp-retired-, and registry.json sits above the depth-2 walk.
  • Checked whether the new MCP_SERVER_NAME_RE refine, which reaches LinkCodePluginManifestReaderSchema and therefore readManifest(), could retro-invalidate installed manifests — it cannot, because the plugin store itself ships in this PR, and install-time strict validation rejects a dotted server name up front.
  • Confirmed the new export * from './system-proxy' in packages/host/assets/src/index.ts is load-bearing: apps/daemon/src/marketplace/service.ts:14 imports fetchWithSystemProxy through it.
  • Read the new engine-side handlers and their 15 tests: refresh and install both gate marketplace-present → configured → enabled, and neither leaks cause detail into request.failed (asserted directly).
  • Read withPluginMcpServers and its three new resolver tests, the widened McpWarningReasonSchema contract tests, the new wire variants and their round-trip tests, the i18n additions, the catalog/installed presentation, and scripts/dev-marketplace.mts.

🧭 Unanswered from the last review

The marketplace trust-model question from review 1 is still open, and a02615f does not address it. Adding a marketplace is effectively a grant of arbitrary local code execution: the on-disk manifest's command / args / env drive the spawned stdio MCP server, and the catalog UI renders none of those fields. Today the only way in is hand-editing config.json or setting LINKCODE_MARKETPLACE_URL, which is a reasonable gate — but there is no wire kind or UI for adding one, so the trust boundary has never been stated. Worth writing down (in apps/daemon/AGENTS.md or the marketplace module's own doc comment) before an "add marketplace" surface ships, since that surface is what turns this from an expert-only escape hatch into a click.

ℹ️ Nitpicks

Five small cleanups, all inline. None of them block.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/daemon/src/plugin-store/paths.ts Outdated
Comment thread apps/daemon/src/plugin-store/paths.ts Outdated
Comment thread packages/client/core/src/client.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts
Comment thread apps/daemon/src/__tests__/plugin-store.test.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues found in the incremental range. One prior nit is left open below.

Re-reviewed 444224b + d5edb11 over a02615f (15 files, +305/−207 — no master merge, so the range-diff is authoritative). Four of the five review-2 threads are addressed and now resolved.

Traced and clean:

  • serialize() per-plugin-id chain (plugin-store/store.ts) — install/uninstall share one key space, .catch(noop) keeps a failed op from poisoning the chain, and settle only evicts when the map still holds that run. void run.catch(noop) suppresses the unhandled rejection while the caller still sees the real error.
  • rollbackPublishedPluginPackage — both branches end consistent. Registry-write failure after publish with retiredDir defined (same-version reinstall) restores the backup while the registry still points at the old version; with retiredDir undefined (new-version install) the target is removed and the registry never held the record. The rmSync(stagingDir) at the top of the catch is a force: true no-op post-rename.
  • plugins-settings.tsx ?. removals — safe, not just type-safe. tayori's trigger is typed Promise<SdkData<SdkMethod>> and implemented with throwOnError: true + responseStyle: 'fields', so it resolves with data or throws; it does not have useSWRMutation's optional-return semantics.
  • lifecyclePendingRef / savePendingRef (linkcode-tab.tsx) — useRef is the right gate here, since isMutating is a useTransition pending flag that can be deferred past a fast double-click. The single shared lifecycle ref matches the already-global busy = install.isMutating || uninstall.isMutating, so gating and rendering agree.
  • String.raw in scripts/dev-marketplace.mts — checked the whole template: String.raw disables escapes but not ${}, so '${VERSION}' still interpolates from the in-scope const VERSION, and the two '\n' sites emit bytes identical to the previous cooked '\\n'.
  • setSettings' field === undefinedfieldId in settings, the index.ts async-IIFE refactor, and the start-options-mcp.test.ts hoist + Promise.all are all semantics-preserving.

Thanks for writing down the trust model in apps/daemon/AGENTS.md — "marketplace sources are trusted code origins, SRI pins bytes but does not make a hostile index safe, adding sources stays config/env-only until an explicit consent design is approved" is exactly the answer the earlier scope question was asking for. That closes it from my side.

Left open (nitpick, not blocking): plugin-store.test.ts:293. The new retired element (truefalse) is genuine signal and the test is stronger for it, but the fixture still only calls writeRegistry([legacy, live]) and never writePackage(legacy, …) — so legacy.path never exists on disk and its existsSync element stays trivially false in both the pre- and post-state arrays. Creating legacy.path in the fixture would make that assertion prove the legacy dir was actually swept.

Not approving only because that one thread is still open; nothing here warrants blocking the merge.

Files reviewed: 69 · Commits reviewed: 7 · Base master · Head qi/code-622 (d5edb11) · Prior Pullfrog review: a02615f

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/daemon/src/plugin-store/store.ts

@lucas77778 lucas77778 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall implementation and tests are solid, and all existing CI checks pass. However, this version still has reachable paths that can report a successful but non-functional install, break registry/package consistency, or leave secrets behind after uninstall. The blocking issues are called out inline and should be addressed before merge; the Switch accessible-name note is explicitly non-blocking.

Comment thread packages/host/engine/src/session/start-options-resolver.ts
Comment thread apps/daemon/src/plugin-store/store.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts
Comment thread packages/foundation/schema/src/model/linkcode-plugin.ts
Comment thread apps/daemon/src/plugin-store/store.ts
Comment thread packages/client/workbench/src/settings/plugins/linkcode-config.ts Outdated
Comment thread packages/host/engine/src/session/start-options-resolver.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The four new commits land all eight of the previous review's threads — I re-verified each fix in source rather than trusting the diff. Two new issues came out of the rewritten lifecycle code: a reinstall can silently inherit a previous install's secrets, and the configuredSecrets wire field is optional for a compatibility case that cannot exist.

Reviewed d4b9136..debfb28 (26 files, +1161/−92 — no master merge in the range).

🐛 Reinstall bypasses a pending uninstall cleanup

The retire-first + tombstone scheme is right for the crash cases it was written for, but the reinstall path is not wired into it. When uninstall()'s settings cleanup throws, it logs and returns at store.ts:238, leaving .uninstall-<id>.json on disk for the boot sweep to retry — correct so far. The registry entry, however, was already removed at store.ts:205. So a reinstall of the same id:

  1. computes previousRecords from the registry (empty), so the previousRecords.length > 0 gate at store.ts:356 skips reconcileSettingsForManifest entirely;
  2. never looks for or consumes the existing tombstone;
  3. leaves getSettings() reading the previous install's config.json values and vault secrets as though they were its own.

Then at next boot sweepUninstallTombstones() sees registered.has(pluginId) is true and discards the marker unread (store.ts:621-623) — the pending cleanup is abandoned, not retried. Net effect: a user uninstalls a plugin, the vault prune fails, they reinstall, and the old credentials are live again with nothing surfaced to them. Narrow trigger (the cleanup has to fail), but that is precisely the failure this machinery exists to cover.

Worth having installExclusive consume a tombstone for the id it is about to install — run the cleanup, drop the marker, then install — so a reinstall honors the pending uninstall instead of inheriting through it.

🔒 A secret → non-secret manifest flip declassifies the stored value

reconcileSettingsForManifest handles the protective direction deliberately (the comment at store.ts:644-646 calls out that a non-secret → secret flip must not leave plaintext behind). The opposite direction promotes: the vault value is coerced back to its declared type, written into config.json, and deleted from the vault (store.ts:675-680). A value the user typed into a masked secret field now sits in plaintext and is served to clients unmasked by maskedView, because maskValues only skips fields whose current manifest entry is secret.

Given apps/daemon/AGENTS.md treats marketplace sources as trusted code origins, this isn't an escalation — a hostile publisher already has better options. It is still a confidentiality change driven entirely by remote manifest content, with no user consent, and the safe alternative costs nothing: drop the value and let the user re-enter it. The upgrade test pins the promotion as intended behavior, so if that's the deliberate call it's fine — I'd just want it to be a decision rather than a side effect of "move a usable value instead of dropping it."

ℹ️ Nitpicks

  • validatePluginConfigPatch rejects '' for a required secret field but not a required non-secret one: '' satisfies isValidPluginSettingValue, lands in effective, and passes the bare fieldId in effective required check. Not reachable from the UI (buildPluginConfigPatch turns a blank non-secret field into a remove), but it's a gap in the authority the daemon now claims.
  • Both withCustomMcpServers (start-options-resolver.ts:138) and withPluginMcpServers (:207) independently call enabledMcpServerNames('codex', …), and that call spins a Codex discovery server and issues a plugin/list plus per-plugin detail reads. Two full round trips per session start when a user has both custom MCP servers and plugins enabled; the result could be resolved once and threaded through.

Verified clean in this delta, for the record: wire 80 → 81 is additive with the floor correctly unmoved; the default-vs-type refinement and its seven counterexamples; isProjectablePluginRelease applied symmetrically at the catalog filter, resolveRelease, and the dev mock host; readRegistryStrict + hasExpectedPackagePath fail closed; retireForUninstall's restore-on-throw; configuredSecrets carrying ids only, never values, in every transition state; PluginConfigValidationErrorinvalid_request; InMemoryLinkCodePluginStore.setSettings running the real validator; the aria-labelledby accessible-name fix. The new tests are genuinely falsifiable — the escaped-path, registry-rollback, and manifest-violation cases each fail against the old code, and the e2e asserts the vault/config split end to end (secret absent from config.json, absent from both masked reads).

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/daemon/src/plugin-store/store.ts
Comment thread apps/daemon/src/plugin-store/store.ts Outdated
Comment thread packages/foundation/schema/src/wire/plugin-config.ts Outdated
Comment thread packages/client/workbench/src/settings/plugins/linkcode-config.ts Outdated
Comment thread packages/host/engine/src/plugin/linkcode-store.ts Outdated
Comment thread packages/host/engine/src/session/start-options-resolver.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

All six threads from the last review are landed, verified in source, and resolved. The commit that closed the duplicate-preflight nitpick overshot: every Codex session start now pays a discovery round trip that previously only ran when something could consume it.

Reviewed changesdebfb28..f12c51a (15 files, +259/−188; no master merge). The range-diff is authoritative except for start-options-resolver.ts, where the Effect-to-synchronous restructure garbles it — I read the current file and git show debfb282:… instead.

  • Verified each of review 5's six threads in source and resolved them: installExclusive now consumes the .uninstall-<id>.json tombstone and purges before installing; the secret → non-secret flip drops the vault value instead of promoting it into plaintext config.json; configuredSecrets is required on both plugin-config.listed and plugin-config.updated, with the = true default gone from validatePluginConfigField and the ?? view.configuredSecrets fallback gone with it; '' is now rejected for every field, not just secrets.
  • Confirmed making configuredSecrets required strands no producer — every construction site supplies a concrete string[], and the flipped contract test now asserts that a reply without the presence bits fails loudly instead of silently reading as "already configured."
  • Traced the reconcile/purge rewrite: the tombstone branch and previousRecords.length > 0 are mutually exclusive by construction (a tombstone can only exist once uninstall()'s registry write committed, which is exactly what empties previousRecords); the config→vault promotion cannot collide with the vault-side prune loop, since secretPatch is applied after both have been built; and the purge keys on the plugin id alone, so a reinstall at a different version still starts clean.
  • Re-ran the eight test files this delta touches: 96 tests pass.

🐛 Every Codex session start now pays a discovery round trip

f12c51a de-duplicates the preflight correctly, but it hoists the call above the guards that used to make it conditional. providerMcpServerNames runs at start-options-resolver.ts:73 for every kind === 'codex' start, while withCustomMcpServers still early-returns on enabled.length === 0 (:146) and withPluginMcpServers on entries.length === 0 (:203) — both now after the round trip has already happened. The old code resolved the names inside each helper, below those guards, so a user with nothing to inject paid nothing.

The guard wasn't moved, it was dropped: engine.ts:110 always constructs PluginService, so the && this.plugins half of the condition never short-circuits in production. And the cost is the one this repo already measured in codex.ts:175 — "measured on 0.144.1: list 3.3s, one read ~160ms" — plus a spawned app-server, sitting on the session-start path.

Net trade: one round trip saved for users who have both custom MCP servers and plugins enabled, one round trip added for every user who has neither. The second group is currently everyone, since the plugin marketplace ships in this PR.

I verified this rather than reading it off the diff — asserting listEnabledMcpServerNames is never called for resolve({ kind: 'codex', cwd: '/repo' }) with no CustomMcpServerService and an empty InMemoryLinkCodePluginStore passes at debfb282 and fails at f12c51a9 ("expected vi.fn() to not be called at all, but actually been called 1 times"). Resolving lazily, or gating the call on whether either fold has work to do, keeps the dedup and restores the zero-work case. Nothing in the suite covers this in either direction, which is why the hoist read as a pure win.

ℹ️ Nitpicks

Two, both inline and neither blocking: the install-time purge sits after upsertRegistry, leaving a narrow crash window that reinstates the inheritance bug the block exists to fix; and const names = nativeMcpNames is a dead alias in both folds.

Files reviewed: 71 · Commits reviewed: 16 · Base master · Head qi/code-622 (f12c51a) · Prior Pullfrog review: debfb28

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/host/engine/src/session/start-options-resolver.ts
Comment thread packages/host/engine/src/session/start-options-resolver.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Both of the last review's items land, and the preflight gate is exactly right. The reordered tombstone purge closes the crash window it was aimed at, but its error path still resurrects the previous install's vault secrets — and the test that names the purge passes with the whole block deleted.

Reviewed changesf12c51a..7733052 (2 commits, 3 files, +92/−35; no master merge, confirmed with git diff --stat).

  • Gated the Codex discovery round trip on hasWork (c16e3ea) — this is the fix I was asking for. I checked the gate is neither narrower nor broader than the guards it replaces: (this.customMcp?.listEnabled().length ?? 0) > 0 mirrors withCustomMcpServers' enabled.length === 0, and this.linkCodePluginStore?.list().some((entry) => entry.installed.enabled) mirrors withPluginMcpServers' entries = store.list().filter((entry) => entry.installed.enabled). So there is no case where discovery is skipped while a fold still injects, which is what would have silently cost name-conflict detection. resolve has exactly two consumers of nativeMcpNames, both covered.
  • Removed both const names = nativeMcpNames aliases — folds read the parameter directly.
  • Moved the install-time purge above the staging and registry writes (7733052) — a hard kill in that span now leaves the id unregistered, so the boot sweep genuinely retries instead of discarding the marker.
  • Added two preflight tests. not.toHaveBeenCalled() for the zero-work case is the one that would have caught the regression; toHaveBeenCalledTimes(1) guards against re-duplication. Both halves of the gate are covered once you count the pre-existing skips a LinkCode plugin whose MCP name collides with a native Codex plugin (no custom-MCP service, one seeded enabled plugin, expects name-conflict) — that test only passes if discovery actually ran for the plugin half. 46 tests pass across the two touched suites.

I also dispatched a crash-consistency specialist at the reordered purge specifically to try to break it, and could not: the only state where a tombstone coexists with a registry record is a hard kill between writeUninstallTombstone (store.ts:204) and writeRegistry (:206), and the constructor's sweepUninstallTombstones discards that marker before any listener binds, so installExclusive can never observe both. Purging and then failing the download is also correct — the uninstall had already committed, so those values were meant to be gone.

ℹ️ Nitpicks

Two, both inline and neither blocking: the invariant comment at store.ts:377 states the wrong reason for a claim that does hold, and the test comment at plugin-store.test.ts:768 describes something that did not happen.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/daemon/src/plugin-store/store.ts
Comment thread apps/daemon/src/__tests__/plugin-store.test.ts Outdated
Comment thread apps/daemon/src/plugin-store/store.ts Outdated
…nstall

The install-time pending-uninstall purge must fail the install when the
vault write fails, rather than logging and proceeding. A failed purge
followed by a committed install would leave the new install inheriting
the uninstalled plugin's vault secrets, since the tombstone is discarded
once the id is re-registered.

Throw from the catch block instead of logging. At that point no staging,
download, or registry write has happened, so the install simply fails,
the marker stays, and the id remains unregistered — exactly the state
the boot sweep retries.

Also correct two comment inaccuracies:
- The invariant comment at store.ts:377 now states the correct reason
  why tombstone and previousRecords can never coexist (constructor sweep
  discards markers for registered ids before any listener binds).
- The test comment at plugin-store.test.ts:769 now reflects that the
  constructor sweep, not the install-time purge, performs the cleanup
  when a new store instance is constructed.

Add a test that exercises the same-instance reinstall path (production
shape) where the vault remains broken across uninstall and reinstall,
verifying the install throws and leaves the state for boot-sweep retry.
@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes7733052..60867e4 (1 commit, 2 files, +61/−8; no master merge, confirmed with git diff --stat). All three of the last review's items land, and the one that mattered lands with real coverage behind it.

  • Failed the reinstall on a failed pending-uninstall purgestore.ts:298-302 now throws instead of logging. I checked the abort is actually free at that point: makePluginTmpDir (:275) only names the directory and mkdirSync(stagingDir) is at :307, so nothing is staged, no download has run, and the registry is untouched. The install fails with the id unregistered and the marker intact, which is precisely the state sweepUninstallTombstones retries (:647-650 runs the cleanup when the id is absent from the registry, rather than hitting the registered.has discard at :644). The log line that promised a retry which could not happen is gone with it.
  • Corrected the invariant comment at store.ts:374-377 to name the constructor's sweepUninstallTombstones as what rules out a tombstone coexisting with previousRecords.
  • Closed the coverage gap the last review found by neutering. The new test at plugin-store.test.ts:776 reuses a single store instance across uninstall → install — the production shape (apps/daemon/src/index.ts:190) — with an always-throwing replaceAll on the plugin namespace, and asserts the id stays unregistered, the marker survives, and the stale secret is still there for the sweep. I re-ran the same experiment that exposed the gap: replacing the throw with a logger.warn makes exactly one test fail, and it is this one. 26/26 pass with the fix in place, so the purge block can no longer be deleted in silence.
  • Fixed the stale comment on the older reinstall test at :769. Keeping that test on a fresh store is the right call — it now honestly documents the fresh-process path while the new test covers the same-process one.

Worth recording so it does not come back as a question: a host whose vault write keeps failing can no longer reinstall that plugin id at all. That is the intended trade — the boot sweep retries on every daemon start and clears the marker as soon as the vault recovers, and the alternative was a committed install running on the uninstalled plugin's credentials. It also takes a genuine I/O failure to reach, since a missing or untrustworthy keyring degrades the vault to plaintext rather than throwing.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants