feat: complete monorepo migration — rewire crew to workspace packages#2
feat: complete monorepo migration — rewire crew to workspace packages#2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab2fab100
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Re-export from @modular/harness (migrated from local copy). | ||
| * @see packages/harness/src/backends/in-process.ts | ||
| */ | ||
| export { InProcessBackend } from '@modular/harness'; |
There was a problem hiding this comment.
InProcessBackend is re-exported from @modular/harness, but the harness public entrypoint does not export that class (see packages/harness/src/index.ts, which only re-exports backend types). This makes backend imports fail when ./backends/inProcessBackend.js is loaded (and registry.ts imports it unconditionally), so in-process execution cannot start.
Useful? React with 👍 / 👎.
| export { DepthLevel, DEPTH_TOKEN_RATIOS } from '@modular/core'; | ||
| export type { DepthLevel } from '@modular/core'; |
There was a problem hiding this comment.
This file now re-exports the same symbol as both value and type in separate statements (e.g. DepthLevel here), which TypeScript reports as duplicate identifiers (TS2300) and blocks compilation; the same pattern repeats for ContextSpec, Condition, InlineAgent, AgentRef, and Budget in this header block. Use a single combined export form or only one export per symbol name.
Useful? React with 👍 / 👎.
| export { InlineAgent, AgentRef } from '@modular/core'; | ||
| export type { InlineAgent, AgentRef } from '@modular/core'; | ||
| export type { ResolvedAgent, AgentRunEvent } from '@modular/core'; | ||
| export { Budget, estimateCost } from '@modular/core'; |
There was a problem hiding this comment.
Switching to estimateCost from @modular/core changes cost semantics for crew runs: core falls back to Claude pricing for unknown models, while this module still defines crew-specific pricing entries (including gpt-4.1 and gpt-4.1-mini) that are now ignored. Budget checks in the compiler can therefore overestimate cost and stop runs early for models not in core’s table.
Useful? React with 👍 / 👎.
VictorGjn
left a comment
VictorGjn
left a comment
There was a problem hiding this comment.
Ultra Review — 9 findings (5 P0, 2 P1, 2 P2) — all blocking items fixed
What's good
- Shim strategy is correct — preserves all internal import paths, zero consumer changes needed
- Commit granularity is excellent (one per shim)
- Types.ts split is clean (shared →
@modular/core, crew-specific stays local) - Net -860 lines of deduplication
P0 — Build-breaking / Runtime crash (all fixed ✅)
| # | Issue | Commit |
|---|---|---|
| 1 | InProcessBackend not exported from @modular/harness index.ts — import fails |
ba9a020 |
| 2 | Duplicate export + export type for 6 Zod symbols (DepthLevel, ContextSpec, Condition, InlineAgent, AgentRef, Budget) → TS2300 |
803f044 |
| 3 | import type { estimateCost } in harness budget.ts — erased at runtime → TypeError: estimateCost is not a function |
38c618d |
| 4 | in-process.ts imports SwarmBackend, AgentResult, AgentHandle from @modular/core (doesn't export them), wrong ./mailbox.js path, wrong StudioProvider source |
b364ad7 |
| 5 | backends/types.ts imports from ./mailbox.js (doesn't exist) instead of ../mailbox.js |
3d8fe98 |
P1 — Semantic regression (all fixed ✅)
| # | Issue | Commit |
|---|---|---|
| 6 | estimateCost from core missing gpt-4.1 and gpt-4.1-mini pricing → falls back to Claude Sonnet ($3/$15), overestimates GPT-4.1 cost by ~50-87% |
7074b6e |
| 7 | MockProvider silently removed from crew's public API (index.ts diff drops the export) |
873dc86 |
P2 — Hygiene (not fixed, non-blocking)
| # | Issue |
|---|---|
| 8 | MODEL_PRICING in crew types.ts is now dead code — estimateCost comes from core |
| 9 | AgentResult (harness) vs AgentRunResult (core) — two near-identical types with different names/status enums. Not new but the migration makes it more visible. |
Codex found 2/9 issues (P0-1, P0-2, P1-6). This review found 7 additional issues including 3 P0s that Codex missed (P0-3: import type crash, P0-4: wrong import paths, P0-5: wrong mailbox path).
All 5 P0 and 2 P1 issues have been fixed in 7 commits pushed to this branch. The PR is now mergeable.
…tCalls, isAvailable, tool_call, rich packContext)
Major changes from v1: - New Phase 0: Ship Crew to npm + landing page before anything else - CI promoted from priority #5 to #2 (non-negotiable for monorepo) - Design system extraction moved before Studio features (not after) - Accessibility added as cross-cutting concern (WCAG AA) - Realistic scope: ~20 extractable primitives, not 44+ - Typed error hierarchy + FactBus concurrency guard added - Crew-first strategy (adoption wedge) with Studio as enterprise upgrade - Honest moat assessment: 2 real moats out of 8 claimed - Exit thesis revised: target Anthropic/Sourcegraph/JetBrains, not OSS projects - Key metrics defined: 500 npm installs, 10 design partners in 90 days Reviewed by: Backend, Frontend, UI/UX, Design Systems, CPO, VC agents. https://claude.ai/code/session_01E77cyQoeLLUkXT5xr6TYC3
1 participant
What
Complete the monorepo migration by rewiring
apps/crewto consume workspace packages instead of local duplicate files.Changes
11 crew files replaced with re-export shims:
facts/fact-bus.ts@modular/harnessfacts/mailbox.ts@modular/harnesshooks/hookRunner.ts@modular/harnessbackends/types.ts@modular/harnessbackends/inProcessBackend.ts@modular/harnessbackground/backgroundRunner.ts@modular/harnessorchestrator/budgetGuard.ts@modular/harnesstrace/eventStream.ts@modular/harnesstrace/summarizer.ts@modular/harnesspresets/index.ts@modular/harnessworktree/worktreeManager.ts@modular/worktreecrew/src/types.tsrefactored:@modular/core@modular/providerscrew/src/index.tsupdated:@modular/harnessinstead of local files@modular/worktreeREADME migration checklist: All 10 core items now checked.
Why shims instead of direct deletion
The shim approach (
export { X } from '@modular/harness') preserves all existing internal import paths. No consumer files needed updating. Tests, compiler, orchestrator, and store modules continue to import from../facts/fact-bus.jsetc. as before.Not included (follow-up)
ds/→ @modular/ui@modular/*imports directlymock.ts@modular/providersMockProvider (8.4KB vs 3.4KB)