activeloopai / activeloopai/hivemind

Replace inlined skilify-worker query() with shared DeeplakeApi client (eliminate retry/timeout duplication)

オープン
#102 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
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 はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。