You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
src/skilify/skilify-worker.ts reimplements its own query() function (~30 lines: fetch + retry on 5xx + AbortSignal timeout, after the fix in this PR) instead of using the canonical DeeplakeApi client at src/deeplake-api.ts. The two implementations are now functionally equivalent on the happy path but maintain two parallel retry/timeout codepaths that have already drifted once.
Why this is worth a follow-up
The skilify worker was introduced in PR #98 and inlined a stripped-down query() for self-containment (the worker is a fully bundled, detached subprocess; importing the full DeeplakeApi would have pulled in a much larger bundle than necessary on initial design). This was a reasonable trade-off in the original PR.
Then claude[bot]'s review on PR #98 flagged that the inlined query() was missing the AbortSignal.timeout(...) + try/catch-around-fetch + retry-on-rejection path that DeeplakeApi.query() already had. We added them inline (commit <HEAD>), restoring parity on the happy path.
What's left is the structural duplication. Two real risks:
Drift again: any future improvement to DeeplakeApi.query() (e.g. a new HTTP status to retry, a backoff tuning, an auth-token refresh hook) will silently NOT apply to the skilify worker. We'll find out in production when the worker misbehaves on a corner case the API client already handles.
Test surface duplication: src/deeplake-api.ts already has retry tests in claude-code/tests/deeplake-api.test.ts. The skilify worker's inlined query() has no equivalent unit tests because it's harder to reach (the worker is a subprocess entry point — see the vitest.config.ts:exclude rationale). Sharing the API client gives the worker the same test coverage by construction.
Proposed approach
Replace the inlined query() in src/skilify/skilify-worker.ts with an instance of DeeplakeApi:
Then thread api through the existing call sites (listCandidateSessions, fetchSessionRows, the INSERT INTO skills path).
Bundle-size impact
The worker bundle today is ~30 KB. DeeplakeApi brings in ~/.deeplake/credentials.json reads (for fallback creds), index-marker-store, embedding-related sql helpers, client-header. esbuild tree-shakes most of it because the worker only uses query() + the constructor. Estimated bundle growth: 5-10 KB. Acceptable.
Acceptance criteria
src/skilify/skilify-worker.ts no longer defines its own query() function
Coverage thresholds in vitest.config.ts for src/skilify/skilify-worker.ts still pass
Optional: add a unit test that mocks DeeplakeApi and asserts the worker calls api.query with the expected SQL shape (currently the worker has no direct unit test surface because it's the subprocess entry)
Background
Surfaced during PR #98 reviews. The inline-fetch pattern was the right call at original-PR time but the maintenance cost of the duplication outweighs the original bundle-size argument now that the worker has been hardened to match the API client.
Summary
src/skilify/skilify-worker.tsreimplements its ownquery()function (~30 lines: fetch + retry on 5xx + AbortSignal timeout, after the fix in this PR) instead of using the canonicalDeeplakeApiclient atsrc/deeplake-api.ts. The two implementations are now functionally equivalent on the happy path but maintain two parallel retry/timeout codepaths that have already drifted once.Why this is worth a follow-up
The skilify worker was introduced in PR #98 and inlined a stripped-down
query()for self-containment (the worker is a fully bundled, detached subprocess; importing the fullDeeplakeApiwould have pulled in a much larger bundle than necessary on initial design). This was a reasonable trade-off in the original PR.Then claude[bot]'s review on PR #98 flagged that the inlined
query()was missing theAbortSignal.timeout(...)+ try/catch-around-fetch + retry-on-rejection path thatDeeplakeApi.query()already had. We added them inline (commit<HEAD>), restoring parity on the happy path.What's left is the structural duplication. Two real risks:
Drift again: any future improvement to
DeeplakeApi.query()(e.g. a new HTTP status to retry, a backoff tuning, an auth-token refresh hook) will silently NOT apply to the skilify worker. We'll find out in production when the worker misbehaves on a corner case the API client already handles.Test surface duplication:
src/deeplake-api.tsalready has retry tests inclaude-code/tests/deeplake-api.test.ts. The skilify worker's inlinedquery()has no equivalent unit tests because it's harder to reach (the worker is a subprocess entry point — see thevitest.config.ts:excluderationale). Sharing the API client gives the worker the same test coverage by construction.Proposed approach
Replace the inlined
query()insrc/skilify/skilify-worker.tswith an instance ofDeeplakeApi:Then thread
apithrough the existing call sites (listCandidateSessions,fetchSessionRows, theINSERT INTO skillspath).Bundle-size impact
The worker bundle today is ~30 KB.
DeeplakeApibrings in~/.deeplake/credentials.jsonreads (for fallback creds),index-marker-store,embedding-related sql helpers,client-header. esbuild tree-shakes most of it because the worker only usesquery()+ the constructor. Estimated bundle growth: 5-10 KB. Acceptable.Acceptance criteria
src/skilify/skilify-worker.tsno longer defines its ownquery()function38f823efor the e2e scripts at/tmp/skilify-pi-real-e2e.mjsand/tmp/skilify-pull-e2e.mjs)src/skilify/skilify-worker.tsstill passDeeplakeApiand asserts the worker callsapi.querywith the expected SQL shape (currently the worker has no direct unit test surface because it's the subprocess entry)Background
Surfaced during PR #98 reviews. The inline-fetch pattern was the right call at original-PR time but the maintenance cost of the duplication outweighs the original bundle-size argument now that the worker has been hardened to match the API client.