activeloopai / activeloopai/hivemind
Replace inlined skilify-worker query() with shared DeeplakeApi client (eliminate retry/timeout duplication)
- 主要言語
- TypeScript
- スター
- 1.6k
- フォーク
- 107
- 平均マージ
- 17時間 30分
- マージ済み PR(30日)
- 6
説明
## 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.
コントリビューションガイド
このリポジトリのコントリビューションガイドは索引されていません
評価
この issue はまだ評価されていません。