Skip to content

fix(file-search): type binary install failures - #222

Open
tt-a1i wants to merge 1 commit into
mainfrom
codex/issue-170-effect-errors
Open

fix(file-search): type binary install failures#222
tt-a1i wants to merge 1 commit into
mainfrom
codex/issue-170-effect-errors

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Problem

The file-search binary installer represented expected download, validation, and extraction failures with global Error values. This erased the Effect failure-channel discriminant, while the live installer also chained multiple Effect.provide calls that could separate Layer lifecycle ownership.

Closes #170.

Value

The installer now keeps expected failures type-safe internally and preserves deterministic Layer construction without changing the public installation error contract. This removes the 11 globalErrorInEffectFailure diagnostics and the multipleEffectProvide diagnostic reported by the Effect Language Service.

Approach

  • add one bounded internal BinaryInstallError for expected download, redirect, checksum, and extraction failures;
  • keep InstallError as the public BinaryEnv.install failure and retain the typed internal error as its cause;
  • compose the manual-redirect Fetch client with Node services into one Layer, then provide that Layer once;
  • assert both bounded-response failures and the live install error mapping retain the typed discriminant.

Validation

  • bun run check — passed with the 12 reported Effect diagnostics removed
  • bun run test — passed: 934 Node tests and 31 Vitest tests
  • git diff --check — passed

Impact

  • User-visible behavior: none; existing install error messages are preserved.
  • Model-visible context/tools: none.
  • Runtime/lifecycle: binary installation now uses one composed Layer and typed expected failures.
  • Persisted config/data: none.
  • Compatibility/risk: low; the public InstallError contract and conservative failure behavior are unchanged.

@agnitum2009 agnitum2009 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Second review — independent verification against ab2b3c2; all claims confirmed.

Local verification (fetched the branch and ran the stated test plan):

  • bun run check -> exit 0, the 12 Effect diagnostics in binaries.ts are gone, no new diagnostics
  • bun run test -> 934 Node tests + 31 Vitest tests pass

Behavioral verification of the Layer composition (adversarial tests with local servers):

  • The Layer.provide(Layer.succeed(FetchHttpClient.RequestInit)({ redirect: "manual" })) binding works: layerMergedContext captures the build-time context and merges it into the request context, so the fetch client still sees redirect: "manual". Verified with a redirect matrix (old vs new composition both return the 302 verbatim instead of following) and an end-to-end live-install test where an https->http downgrade redirect is correctly refused with refusing non-HTTPS download URL and nothing is materialized on disk.
  • Tampered payloads still fail with SHA-256 mismatch, so the integrity contract holds.

All #170 acceptance criteria hold.

Non-blocking hardening suggestions:

  1. The repo has no redirect-behavior regression test, and the manual binding now depends on the subtle build-time-capture semantics of layerMergedContext — a future refactor that moves Layer.succeed(RequestInit) out of Layer.provide (or swaps layerFetch for a layer not built via layerMergedContext) would silently lose the manual-redirect policy with no test catching it. A client-level assertion (302 returned verbatim, not followed) would pin this cheaply.
  2. Cosmetic: unify Effect.fail(new BinaryInstallError(...)) vs yield* new BinaryInstallError(...) to one style.
  3. A one-line comment at installLayer noting RequestInit is bound at build time would help.

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.

file-search: 修复 Effect failure channel 与多次 provide 诊断

2 participants