Skip to content

Commit e919c97

Browse files
khaliqgantkhaliqgant
authored
[factory] p5: consume @agent-relay/factory from npm + tear down in-Pear daemon (#373)
* [factory] p5: consume @agent-relay/factory from npm + tear down in-Pear daemon Factory issue p5. Autonomous build via relayflows squad loop. * chore(factory p5): regenerate electron-builder.mcp-resources.yml after factory-sdk removal * fix(factory p5): complete daemon teardown in main/index, ipc-handlers, preload (remove factory-manager import, FactoryEvent, start/stop) — typecheck:node clean
1 parent dd0ed75 commit e919c97

117 files changed

Lines changed: 1483 additions & 27927 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.workflow-artifacts/factory-p5-pear-teardown/claude-fix.md‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
# Fix report — factory p5 (pear teardown)
2+
3+
All four review findings were valid and have been fixed. No findings were skipped.
4+
5+
## Finding 1 [Medium] — Zero test coverage for new cloud-status + config-parsing logic → FIXED
6+
7+
Added two new `describe` blocks to `src/main/ipc-handlers.test.ts`, driving the
8+
module-private functions through the registered IPC handlers (the existing
9+
`ipcMain.handle` mock captures them).
10+
11+
`./auth` is now mocked with `resolveCloudAuth` / `getAccountWorkspaceId` /
12+
`accountWorkspaceReadyRetryOptions`, and `fetch` is stubbed per-case via
13+
`vi.stubGlobal`. Config round-trips use a real temp dir (`mkdtempSync`) since the
14+
handlers use real `fs/promises` + `existsSync`.
15+
16+
`registerIpcHandlers factory:read-config / factory:save-config`:
17+
- empty NodeConfig when the file does not exist
18+
- save→file→re-read round-trip reproduces the draft; asserts the on-disk shape
19+
- reads NodeConfig fields that live under `repos.*` via fallback
20+
- promotes edited fields to the top level, leaving `repos.*` untouched (Finding 3 pin)
21+
- returns validation errors without writing on an invalid draft
22+
23+
`registerIpcHandlers factory:status`:
24+
- (a) all endpoints 404 → `state:'empty'` (asserts the 3-endpoint fallback walk)
25+
- (b) 401 → `state:'unauthenticated'`, `connected:false`
26+
- non-OK 500 → `state:'unavailable'`, message contains the status code
27+
- (c) 200 payload with agents/issues/counters → `state:'online'` normalized output
28+
- (d) `resolveCloudAuth` → null → `state:'unauthenticated'`, no workspace lookup
29+
- duplicate concurrent `factory:status` → `resolveCloudAuth` consulted **once**
30+
(covers the `factoryStatusInFlight` coalescer). Note: the IPC handler is `async`
31+
so it wraps the shared promise in a fresh one each call; the coalescing is
32+
therefore asserted via the single underlying `resolveCloudAuth` call (and equal
33+
resolved values), not via `===` promise identity.
34+
35+
## Finding 2 [Low] — Dead `factory:event` push path → FIXED (option b: delete dead plumbing)
36+
37+
The teardown's spec is a read-only manual snapshot (no main-side emitter), so the
38+
dead subscription plumbing was removed rather than wiring up polling:
39+
- `src/shared/types/ipc.ts`: removed the `FactoryEvent` type and `factory.onEvent`
40+
from the `PearAPI` factory surface.
41+
- `src/preload/index.ts`: removed the two `FactoryEvent` type imports and the
42+
`onEvent`/`subscribe('factory:event')` binding.
43+
- `src/renderer/src/lib/ipc-mock.ts`: removed the `onEvent` mock entry.
44+
- `src/renderer/src/components/factory/FactoryPage.tsx`: the mount effect no longer
45+
subscribes; added a comment documenting that status is a read-only snapshot
46+
fetched on mount and via the manual Refresh button (daemon removed in p5).
47+
48+
Grep confirms no `FactoryEvent` / `factory:event` / `factory.onEvent` references
49+
remain (only the explanatory comment in `FactoryPage.tsx`).
50+
51+
## Finding 3 [Low] — read/save asymmetry for `cloneRoot`/`clonePaths` → FIXED
52+
53+
Added a comment above `extractNodeConfig` in `src/main/ipc-handlers.ts` documenting
54+
that NodeConfig fields live at the **top level** (where `NodeConfigSchema` picks
55+
them), distinct from the workspace-level `repos.*` block; read falls back into
56+
`repos.*`, save always promotes to the top level. The asymmetry is now pinned by
57+
the "promotes edited NodeConfig fields to the top level, leaving repos.* untouched"
58+
test, which fails loudly if the loader location ever changes.
59+
60+
## Finding 4 [Nit] — duplicate React keys from issue buckets → FIXED
61+
62+
`normalizeFactoryCloudStatus` now dedups issues by `key` (keep first occurrence)
63+
across the `issues` + `inFlight`/`inflight` + `queued` buckets via a `Set`, with a
64+
comment referencing the AGENTS.md "treat duplicate delivery as normal" guidance.
65+
Covered by the 200-payload test (AR-1 appears in two buckets → `issues.length === 2`).
66+
67+
## Commands run (all from the worktree root)
68+
69+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run src/main/ipc-handlers.test.ts` → 21/21 pass
70+
- `npm run typecheck:node` → PASS
71+
- `npm run typecheck:web` → PASS
72+
- `npx eslint <all changed files>` → clean (no output)
73+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run` → **31 files, 438 tests, all pass**
74+
75+
## Files changed
76+
77+
- `src/main/ipc-handlers.ts` (dedup + doc comment)
78+
- `src/main/ipc-handlers.test.ts` (new factory config + status test suites)
79+
- `src/shared/types/ipc.ts` (removed `FactoryEvent` + `factory.onEvent`)
80+
- `src/preload/index.ts` (removed `FactoryEvent` imports + `onEvent` binding)
81+
- `src/renderer/src/lib/ipc-mock.ts` (removed `onEvent` mock)
82+
- `src/renderer/src/components/factory/FactoryPage.tsx` (removed dead subscription, added comment)

‎.workflow-artifacts/factory-p5-pear-teardown/claude-review.md‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# Fresh-eyes Review — factory p5 (pear teardown)
2+
3+
**Verdict: APPROVE WITH MINOR FINDINGS.** The teardown is clean and the core claims verify. No
4+
hard blockers. Findings below are test-coverage gaps and dead/asymmetric surface, all actionable.
5+
6+
## What I verified (independently re-run, not trusted from self-reflection)
7+
8+
- `npm run typecheck:node` — PASS
9+
- `npm run typecheck:web` — PASS (FactoryPage rewrite typechecks against the new `FactoryStatus`/`FactoryNodeConfig`)
10+
- `npx vitest run src/main/ipc-handlers.test.ts` — PASS (10/10); the existing IPC test does **not** reference the deleted `factory-manager`, so the deletion broke nothing.
11+
- Proof greps over `src bin scripts`: no `factory-sdk`, `factoryManager`, `FactoryManager`, `factory:start`, `factory:stop`, `FactoryLogLine`, `/tmp/factory-run`, `packages/factory-sdk`. Clean.
12+
- No dangling callers of `pear.factory.start/stop` in the renderer.
13+
- `bin/pear.mjs`: all imports still used after the `pear factory` branch removal — no dead imports.
14+
- `@agent-relay/factory@0.1.1` is installed and exports `FactoryConfig`; the `Pick<FactoryConfig, 'capabilities'|'clonePaths'|'dryRun'> & Partial<Pick<…,'workspaceId'|'cloneRoot'>>` type resolves (those fields exist at the FactoryConfig top level).
15+
- `auth.resolveCloudAuth` / `getAccountWorkspaceId` / `accountWorkspaceReadyRetryOptions` exist with the shape used (`CloudAuth` has `apiUrl` + `accessToken`); `fetch` in the Electron-42 main process is an established pattern (cloud-agent/integrations/broker all use it).
16+
- `factoryStatusInFlight` in-flight coalescing aligns with the AGENTS.md "coalesce concurrent starts/attaches with keyed in-flight promises" rule.
17+
- `electron-builder.mcp-resources.yml` diff is a benign regenerated artifact: the removed nested `@agent-relay/harnesses/node_modules/@agent-relay/broker-*` globs are deduped — the top-level `@agent-relay/broker-*` entries remain, so the broker binaries are still bundled. `@agent-relay/fleet/**` + `jiti/**` were added as factory transitive deps. No packaging regression.
18+
19+
## Findings
20+
21+
### 1. [Medium] New factory cloud-status + config-parsing logic has ZERO test coverage
22+
**File:** `src/main/ipc-handlers.ts` (lines ~82–430)
23+
24+
The teardown added a substantial block of non-trivial, branch-heavy logic with no tests:
25+
`normalizeNodeConfigInput`, `extractNodeConfig`, `readFactoryNodeConfig`, `saveFactoryNodeConfig`,
26+
`normalizeIssue`, `normalizeAgent`, `normalizeFactoryCloudStatus`, `readFactoryCloudStatus`
27+
(endpoint fallback on 404/405/501, 401/403 → `unauthenticated`, non-OK → `unavailable`, payload
28+
normalization, config-shape detection on save). The existing `ipc-handlers.test.ts` does not touch
29+
factory, and `npm test` (the `src/main/__tests__/*.test.ts` glob) does not even run the vitest file
30+
that would.
31+
32+
**Required fix:** Add a vitest spec exercising this surface. The functions are module-private, so test
33+
through the registered IPC handlers — the existing `ipc-handlers.test.ts` mock already captures
34+
handlers via the `ipcMain.handle` mock. Required cases:
35+
- `factory:read-config` / `factory:save-config` round-trip against a temp `factory.config.json` (assert the saved file shape and that a re-read reproduces the draft).
36+
- `factory:status` with a mocked global `fetch`: (a) all endpoints 404 → `state: 'empty'`; (b) 401 → `state: 'unauthenticated'`, `connected: false`; (c) a 200 payload with `agents`/`issues`/`counters` → normalized output with `state: 'online'`; (d) unauthenticated (`resolveCloudAuth` → null) → `state: 'unauthenticated'`.
37+
- A duplicate `factory:status` call while one is in flight returns the **same** promise (covers the `factoryStatusInFlight` coalescing — this is the AGENTS.md replay-hardening path and deserves an explicit regression test).
38+
39+
### 2. [Low] Dead `factory:event` push path — status never updates live
40+
**Files:** `src/preload/index.ts:309`, `src/shared/types/ipc.ts:459` (`FactoryEvent`), `src/renderer/src/components/factory/FactoryPage.tsx:96-98`
41+
42+
Main no longer emits `factory:event` (the emitter was removed with `factoryManager`), but the channel
43+
is still plumbed through preload, the `FactoryEvent` type, the mock, and `FactoryPage` subscribes via
44+
`pear.factory.onEvent`. That subscription can never fire. Cloud status is therefore fetched only on
45+
mount and on the manual Refresh button — there is no polling/auto-refresh.
46+
47+
**Required fix:** Pick one and make it intentional:
48+
- (a) If a refreshing view is intended, add a polling interval in the `FactoryPage` effect (guard against overlap with the existing in-flight coalescer) — and add a test for it; **or**
49+
- (b) If a manual read-only snapshot is the spec, delete the now-dead `onEvent`/`FactoryEvent`/`factory:event` plumbing (preload + type + mock + FactoryPage subscription), or leave a one-line comment marking it reserved for Phase 2 so it isn't read as live.
50+
51+
**Required test:** whichever path is chosen, assert the refresh behavior (mock returns a new status on the second `pear.factory.status()` and the UI reflects it after refresh/poll).
52+
53+
### 3. [Low] `read` vs `save` asymmetry for `cloneRoot` / `clonePaths`
54+
**File:** `src/main/ipc-handlers.ts` — `extractNodeConfig` (reads `source.cloneRoot ?? repos?.cloneRoot`) vs `saveFactoryNodeConfig` (else branch writes top-level `{...raw, ...parsed.config}`)
55+
56+
This repo's `factory.config.json` is the compact single-source shape (operative values under `repos.cloneRoot`/`repos.clonePaths`, no `nodeConfig`/`factoryConfig` wrapper). On read, the editor
57+
falls back into `repos.*`; on save it writes a **top-level** `cloneRoot`/`clonePaths` and leaves
58+
`repos.*` untouched. Net: after editing "Clone root" and saving, the file carries both a top-level
59+
`cloneRoot` (edited) and a stale `repos.cloneRoot`, which can silently diverge. This is *probably*
60+
correct for the published loader (NodeConfigSchema picks the top-level fields), but the duplication is
61+
a foot-gun and the read-fallback-then-promote behavior is undocumented.
62+
63+
**Required fix:** Add a brief comment documenting that NodeConfig fields live at the top level,
64+
distinct from the workspace-level `repos.*`. **Required test:** the round-trip test from Finding 1
65+
should assert the saved file shape explicitly against the `repos`-based input so this asymmetry is
66+
pinned and any future loader-location change fails loudly.
67+
68+
### 4. [Nit] Issue list can emit duplicate React keys
69+
**File:** `src/main/ipc-handlers.ts` — `normalizeFactoryCloudStatus`; `FactoryPage.tsx:187` (`key={issue.key}`)
70+
71+
`issues` is built by concatenating the `issues` + `inFlight`/`inflight` + `queued` buckets with no
72+
dedup by `key`. If a speculative cloud payload lists the same issue in two buckets, React warns on
73+
duplicate keys.
74+
75+
**Required fix:** dedup by `key` (keep first occurrence) before returning `issues` — also the more
76+
correct behavior and consistent with the AGENTS.md "treat duplicate delivery as normal" guidance.
77+
**Required test:** covered by adding a duplicate-key issue to the mocked payload in the Finding 1
78+
`factory:status` test and asserting the returned `issues` length.
79+
80+
## Summary
81+
82+
Spec coverage is met: the in-Pear daemon model is fully removed (no `FactoryManager`, no spawn, no
83+
heartbeat/reaper/registry, no `pear factory` passthrough, no `factory:start`/`stop` IPC), config IPC is
84+
NodeConfig-only, and `FactoryPage` is a read-only cloud view + local NodeConfig editor. Typechecks and
85+
the existing vitest pass; nothing dangling. The gaps are: (1) untested new parsing/status logic incl.
86+
the coalescer, (2) a dead `factory:event` path with no auto-refresh, (3) an undocumented read/write
87+
location asymmetry for `cloneRoot`/`clonePaths`, and (4) a duplicate-React-key nit. All are
88+
test-and-polish items, not blockers.

‎.workflow-artifacts/factory-p5-pear-teardown/codex-fix.md‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# Codex Fix - factory p5 pear teardown
2+
3+
## Result
4+
5+
Fixed the remaining valid review finding.
6+
7+
## Finding Addressed
8+
9+
### Split factory configs could lose inherited checkout mappings on save
10+
11+
`factory:read-config` now matches the published factory loader's split-config inheritance behavior for `workspaceConfig` / `nodeConfig` files:
12+
13+
- `nodeConfig.workspaceId` falls back to `workspaceConfig.workspaceId` when omitted.
14+
- `nodeConfig.cloneRoot` falls back to `workspaceConfig.repos.cloneRoot` when omitted.
15+
- `nodeConfig.clonePaths` falls back to `workspaceConfig.repos.clonePaths` when omitted.
16+
17+
`factory:save-config` now writes split configs without materializing inherited checkout fields as node-level overrides when those fields were absent from the existing `nodeConfig`. This prevents a no-op read/save from turning inherited workspace checkout mappings into explicit `nodeConfig` overrides. Existing explicit `nodeConfig.clonePaths: {}` overrides are preserved.
18+
19+
## Files Changed
20+
21+
- `src/main/ipc-handlers.ts`
22+
- `src/main/ipc-handlers.test.ts`
23+
24+
## Tests / Proofs Added
25+
26+
- Added a split-config regression test that reads and saves a config with inherited `workspaceConfig.repos.cloneRoot` and `workspaceConfig.repos.clonePaths`, then verifies the effective `nodeConfig` with the published factory loader implementation.
27+
- Added a regression test that preserves an explicit split-config empty `nodeConfig.clonePaths` override.
28+
29+
## Verification
30+
31+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run src/main/ipc-handlers.test.ts` - PASS, 23 tests.
32+
- `npm run typecheck:node` - PASS.
33+
- `npm run typecheck:web` - PASS.
34+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run` - PASS, 31 files / 440 tests.
35+
- `git diff --check` - PASS.
36+
- `npm test` - PASS, 122 tests.
37+
38+
## Notes
39+
40+
`npm test` prints expected diagnostic warnings from burn-spawn-hook tests for simulated fallback cases (`ledger locked` and missing `@relayburn/sdk`), but the command exits successfully with all tests passing.

‎.workflow-artifacts/factory-p5-pear-teardown/codex-review.md‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Codex Review - factory p5 pear teardown post-fix
2+
3+
**Verdict: CHANGES REQUESTED.** The Claude fix resolves the first-pass findings in the normal compact config path, removes the dead `factory:event` plumbing from tracked source, and adds useful IPC regression coverage. I found one remaining split-config regression in the new NodeConfig read/save compatibility path.
4+
5+
## Findings
6+
7+
### 1. [Medium] Split factory configs can lose inherited checkout mappings on save
8+
9+
**Files:** `src/main/ipc-handlers.ts:191`, `src/main/ipc-handlers.ts:245`
10+
11+
`saveFactoryNodeConfig` explicitly handles files that contain `workspaceConfig` or `nodeConfig`, but `extractNodeConfig` only reads `raw.nodeConfig` when it exists and does not fall back into `raw.workspaceConfig.repos`. That diverges from the published factory loader's split-config semantics: `combineSplitConfigInput` inherits `cloneRoot` and `clonePaths` from `workspaceConfig.repos` when `nodeConfig` omits them.
12+
13+
The save path then makes this worse because `normalizeNodeConfigInput` always materializes `clonePaths: {}`. For a valid split config such as:
14+
15+
```json
16+
{
17+
"workspaceConfig": {
18+
"repos": {
19+
"cloneRoot": "/work",
20+
"clonePaths": { "AgentWorkforce/pear": "/custom/pear" }
21+
}
22+
},
23+
"nodeConfig": {
24+
"capabilities": ["spawn:claude"]
25+
}
26+
}
27+
```
28+
29+
Pear's editor reads an empty `clonePaths`, and a no-op save rewrites `nodeConfig` with `clonePaths: {}`. The factory loader treats that empty object as an explicit node override, so `workspaceConfig.repos.clonePaths` is no longer inherited. That can silently change the effective checkout map.
30+
31+
**Required fix:** Either remove the split-config branch if Pear only supports compact/root and `factoryConfig` shapes, or make split read/save match `@agent-relay/factory`'s loader semantics. The safest implementation is to add a test with a split `workspaceConfig`/`nodeConfig` fixture and assert that a no-op read/save preserves the effective `cloneRoot`/`clonePaths`. Then update extraction to use `workspaceConfig.repos` as fallback for split configs and avoid writing an empty `nodeConfig.clonePaths` that shadows inherited workspace mappings unless the user actually set an explicit empty override.
32+
33+
## Verification
34+
35+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run src/main/ipc-handlers.test.ts` - PASS, 21 tests.
36+
- `npm run typecheck:node` - PASS.
37+
- `npm run typecheck:web` - PASS.
38+
- `git diff --check` - PASS.
39+
- `env -u AGENT_RELAY_BROKER_PID npx vitest run` - PASS, 31 files / 438 tests.
40+
- Source grep is clean for removed tracked APIs: no `factory:start`, `factory:stop`, `FactoryEvent`, `factoryManager`, `FactoryManager`, `pear factory`, `factory-sdk`, `packages/factory-sdk`, or `/tmp/factory-run` under tracked source areas. The only remaining `factory:event` text in source is an explanatory comment in `FactoryPage.tsx`.
41+
42+
## Summary
43+
44+
I reviewed the changed source, diff, repo instructions, prior review/fix artifacts, installed `@agent-relay/factory` loader behavior, and regression tests. Artifact produced: `.workflow-artifacts/factory-p5-pear-teardown/codex-review.md`.

0 commit comments

Comments
 (0)