activeloopai / activeloopai/hivemind
Replace inlined skilify-worker query() with shared DeeplakeApi client (eliminate retry/timeout duplication)
- Dominant language
- TypeScript
- Stars
- 1.6k
- Forks
- 107
- Avg merge
- 17h 30m
- Merged PRs (30d)
- 6
Description
## Summary
`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 ``), restoring parity on the happy path.
What's left is the structural duplication. Two real risks:
1. **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.
2. **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`:
```ts
import { DeeplakeApi } from "../deeplake-api.js";
const api = new DeeplakeApi(cfg.token, cfg.apiUrl, cfg.orgId, cfg.workspaceId, cfg.sessionsTable);
async function query(sql: string): Promise[]> {
return api.query(sql);
}
```
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
- Worker still passes the existing pull e2e + Pi real-runtime e2e (links: PR #98 commit `38f823e` for the e2e scripts at `/tmp/skilify-pi-real-e2e.mjs` and `/tmp/skilify-pull-e2e.mjs`)
- Worker bundle size growth < 15 KB
- 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.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.