fix(skills): surface skill market install failures instead of disabling install - #433
Merged
Merged
Conversation
…ng install The install sheet fetched the preview document and swallowed any failure, leaving the document body null. The confirm button is disabled while the body is null, so a failed fetch left it permanently greyed out behind the word "Loading…" with no error and no retry — the dead end reported in #419, where a Windows user behind a proxy could not install anything. The same report says an error appears as soon as the market page opens, which is the source-level `remoteError` line. Report the failure instead, and make the reason legible: - `PublicNetworkPolicyError` now carries `NETWORK_POLICY_BLOCKED`, a code the IPC wrapper already forwards, so the renderer can tell a policy refusal from an ordinary failure. A refusal means the main-process public-network guard rejected the host from a *local* DNS lookup, which is exactly what a proxy that answers DNS itself (Clash fake-IP in 198.18.0.0/15, a TUN or split resolver) produces for a URL that opens fine in the browser. `assertPublicUrl` and the preview-before-install gate are deliberately untouched (ADR 0243). - The install sheet shows a localized error, the main-process reason, a proxy/DNS hint for policy refusals, and a Retry action that re-runs the preview. Install still requires a previewed document, but the button is never dead without an explanation. - The market list reports why sources failed: `failureKinds` separates a refused source from an unreachable one, the whole-query rejection that vanished into an empty `catch` now shows its reason, and a partial outage is no longer silent. - New strings land in all eight locales, plus `errors.NETWORK_POLICY_BLOCKED` in the registry and the specs (en and zh-CN). Refs #419
There was a problem hiding this comment.
🟡 Changes recommended
Address policy classification coverage, sensitive error-message redaction, and redirect-limit handling/documentation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves skill-market failure diagnostics by surfacing errors, classifying policy/network failures, and adding retry guidance.
Changes:
- Adds stable policy error classification and per-source failure kinds.
- Surfaces localized preview errors, details, proxy guidance, and retry.
- Updates IPC types, documentation, translations, styles, and tests.
File summaries
| File | Summary |
|---|---|
packages/shared/src/public-network.ts |
Adds policy failure classification. |
packages/shared/src/public-network.test.ts |
Tests policy detection. |
packages/shared/src/errors.ts |
Registers the new error code. |
packages/i18n/src/locales/zh-TW/index.ts |
Adds localized messages. |
packages/i18n/src/locales/zh-CN/index.ts |
Adds localized messages. |
packages/i18n/src/locales/tr/index.ts |
Adds localized messages. |
packages/i18n/src/locales/ko/index.ts |
Adds localized messages. |
packages/i18n/src/locales/fr/index.ts |
Adds localized messages. |
packages/i18n/src/locales/es/index.ts |
Adds localized messages. |
packages/i18n/src/locales/en/index.ts |
Adds localized messages. |
packages/i18n/src/locales/de/index.ts |
Adds localized messages. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Updates E2E documentation. |
docs/zh-CN/spec/04-ux/06-settings-ia.md |
Documents updated UX behavior. |
docs/zh-CN/spec/03-runtime/08-error-codes.md |
Documents error codes. |
docs/zh-CN/spec/03-runtime/01-ipc-protocol.md |
Updates IPC documentation. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Updates E2E documentation. |
docs/spec/04-ux/06-settings-ia.md |
Documents updated UX behavior. |
docs/spec/03-runtime/08-error-codes.md |
Documents error codes. |
docs/spec/03-runtime/01-ipc-protocol.md |
Updates IPC documentation. |
apps/desktop/test/skill-market-scan.test.mjs |
Tests source failure classification. |
apps/desktop/test/skill-market-panel.test.mjs |
Tests renderer failure handling. |
apps/desktop/test/skill-market-failure.test.mjs |
Tests failure helpers. |
apps/desktop/test/public-https-fetch.test.mjs |
Tests policy error codes. |
apps/desktop/src/styles/settings.css |
Styles preview errors. |
apps/desktop/src/lib/skill-market-failure.ts |
Classifies market failures. |
apps/desktop/src/lib/api.ts |
Extends the market response type. |
apps/desktop/src/components/settings/SkillMarketPanel.tsx |
Adds visible errors and retry. |
apps/desktop/electron/main/skill-market-scan.ts |
Reports failure kinds. |
apps/desktop/electron/main/public-https-fetch.ts |
Attaches policy error codes. |
Review details
Suppressed comments (1)
docs/spec/03-runtime/08-error-codes.md:69
PublicNetworkPolicyErroris also thrown for the max-redirect case (public-https-fetch.ts:90), not only for URL syntax or local-DNS classification. AssigningNETWORK_POLICY_BLOCKEDto the whole class makes that failure render as an address/policy refusal with the proxy hint, and the spec's claim that retry can only succeed after an address/resolver change is false when the remote redirect chain changes. Please either give redirect-limit failures a distinct code/classification or broaden the documented and localized policy meaning to cover this case.
| `NETWORK_POLICY_BLOCKED` | no | the main-process public-network guard refused a fetch: the URL failed the syntactic public-HTTPS check, or the local DNS lookup could not classify the host as public (ADR 0243). A desktop-only code; retrying cannot succeed until the address or the resolver changes. |
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+30
to
+32
| export function skillMarketFailureDetail(error: unknown): string { | ||
| if (error instanceof Error && error.message) return error.message; | ||
| if (typeof error === "string") return error; |
Comment on lines
+20
to
+21
| const code = (error as { code?: unknown } | null | undefined)?.code; | ||
| return code === ErrorCodes.NETWORK_POLICY_BLOCKED ? "policy" : "network"; |
…arket-install-feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reported in #419: on Windows 0.14.8, behind a network proxy, the skill market shows an error and the install action is unusable ("无法点击安装") while the same URL opens in the user's browser.
This PR fixes the part of that report that is provable from the code, and makes the remaining part diagnosable. It does not claim to lift the underlying network restriction — see "What this does not fix" below.
Root causes found in the code
SkillMarketPanel.openInstallcaught the document fetch, resetdocumentBodytonulland reported nothing. The confirm button isdisabled={installing || documentBody === null || documentTooLarge}, so any failed preview left it permanently greyed out behind the word "Loading…" — no error, no reason, no retry. That is exactly "无法点击安装".searchSkillMarket's.catchreplaced the whole result with{status: "error"}, and the source list only ever carried bare display names. A refusal from the public-network guard was indistinguishable from a host being unreachable, so the user could not tell what to do.PublicNetworkPolicyErrorreached the renderer as a generic failure.Changes
public-https-fetch.ts/packages/shared/src/errors.ts—PublicNetworkPolicyErrornow carriesNETWORK_POLICY_BLOCKED, so the renderer can tell a guard refusal from an ordinary network failure. Registered indocs/spec/03-runtime/08-error-codes.md.packages/shared/src/public-network.ts—PUBLIC_NETWORK_POLICY_ERRORandisPublicNetworkPolicyFailure, so the pure module can classify a refusal without depending onnode:dns.skill-market-scan.ts—search()returnsfailureKinds(policy|network) alongsidefailedSources, with a source name that was both refused and unreachable resolving topolicy, and a hostile display name (__proto__) staying an own key.SkillMarketPanel.tsx+ new puresrc/lib/skill-market-failure.ts— the install sheet shows a localized error, the main-process reason, a proxy/DNS hint for policy refusals, and a Retry that re-runs the preview. The market list distinguishes refused / unreachable / whole-query failure, and now reports a partial outage instead of staying silent.zh-CNmirrors updated.The preview-before-install gate is deliberately unchanged, as is
assertPublicUrl(ADR 0243 chosenet.fetchto keep the proxy stack; the guard's local DNS pre-check is a security boundary).What this does not fix
A user whose failure comes from
assertPublicUrl's local DNS lookup can still be refused. The fetch goes through the proxy (ADR 0177 applies the proxy to Chromium sessions andnet.fetch), but the guard resolves locally withnode:dns; a proxy that answers DNS itself (Clash fake-IP in198.18.0.0/15classifies as benchmark, a TUN/split resolver, or plain resolution failure) makes the guard refuse a host the browser reaches fine. This is inference from code — the report has no logs, so it is not proven.Changing that means moving validation into the proxy stack or replacing the local pre-check with connection-time validation when a proxy is configured. That alters a security boundary and needs an ADR per this repository's rules, so it is not in this PR. With these changes the user gets a specific reason, a proxy hint and a retry (and a transient failure now recovers) instead of a dead button.
Validation
On the integrated local
main(merge commitb1bd0f2b, after merging this branch):pnpm build:jspnpm --filter @pi-desktop/desktop typecheckpnpm lint:biomepnpm -r --if-present testpnpm test:e2e:skill-marketE2E-SKILL-MARKET-NET-BOUNDARYpnpm test:e2e:theme-surfacesfailures: []pnpm test:e2e:layoutpnpm test:e2e:bootThe new tests fail without the fix: reverting the source while keeping the tests gives 7 failures, naming exactly the new behaviour (missing
errorCode, missingsetPreviewFailure, missingfailureKinds, missing locale strings). One caveat stated honestly: the newskill-market-failure.test.mjscovers new pure functions and therefore cannot fail under that mutation by construction.Behavior change to note
PublicNetworkPolicyErrornow reportsNETWORK_POLICY_BLOCKEDinstead of falling through toINTERNALanywhere that guard is used, not only in the skill market. This is a strictly more specific code for the same refusal, registered in spec 08 §3.1.Refs #419