Skip to content

feat: Phase 3 — Emisso Factory company, agent skills, PR fixes - #2

Merged
cdotte merged 4 commits into
masterfrom
feat/phase3-factory-seed-skills
Mar 21, 2026
Merged

feat: Phase 3 — Emisso Factory company, agent skills, PR fixes#2
cdotte merged 4 commits into
masterfrom
feat/phase3-factory-seed-skills

Conversation

@cdotte

@cdotte cdotte commented Mar 21, 2026

Copy link
Copy Markdown

Summary

  • Seed data: Replace Paperclip Demo Co with Emisso Factory — 3 agents (Engineer, SDR, Marketing) using emisso_sandbox adapter, project + workspace, goal, 3 sample issues, 2 budget policies ($1,000/mo company, $500/mo engineer)
  • SDR skill: Port the production folder-native CRM skill from emisso-hq (prospecting, enrichment, email drafting, pipeline management)
  • Marketing skill: New content creation skill with brand voice, blog/social guidelines, audience definitions
  • Phase 2 PR fixes: Address all 5 Greptile review comments — 2 P1 bugs (dead process launch, missing stdin arg), shared type violation, MCP textarea stale state, session token entropy

Test plan

  • pnpm typecheck clean
  • pnpm build passes
  • pnpm dev → run seed → verify company + 3 agents + project + workspace + issues + budget policies created
  • In UI: Emisso Factory → Agents → Engineer, SDR, Marketing visible with emisso_sandbox adapter
  • In UI: manually wake Engineer → verify sandbox creates, clones emisso-hq, runs Claude
  • Verify budget policies show under company settings

🤖 Generated with Claude Code

@cdotte

cdotte commented Mar 21, 2026

Copy link
Copy Markdown
Author

@claude review edge cases.

@github-actions

github-actions Bot commented Mar 21, 2026

Copy link
Copy Markdown

Claude finished @cdotte's task in 2m 42s —— View job


Edge Case Review

  • Gather context
  • Review emisso-sandbox adapter (execute, helpers, types)
  • Review seed data logic
  • Review UI adapter components
  • Review auth bridge change
  • Post findings

Findings

🔴 P1 — buildRunnerScript: No error handler on proc.stdin

server/src/adapters/emisso-sandbox/execute.ts:430-431

proc.stdin.write(prompt);
proc.stdin.end();

If Claude exits before stdin is fully written (e.g. claude binary not found on $PATH, or crash on startup), Node.js emits an EPIPE / ERR_STREAM_DESTROYED error on proc.stdin. Without an error listener, this is an unhandled exception that will crash the runner.mjs inside the sandbox. The sandbox then exits non-zero but with no useful error message, making this very hard to debug.

Fix: add proc.stdin.on('error', () => {}) or proc.on('error', (err) => { process.stderr.write(err.message); process.exit(1); }) before writing.

Fix this →


🔴 P1 — embedGitCredentials: SSH URLs silently lose credentials

server/src/adapters/emisso-sandbox/helpers.ts:111-123

} catch {
  return url;  // returns original SSH URL — no credentials embedded
}

If repoUrl is an SSH-style URL (git@github.com:org/repo.git), new URL(url) throws (SSH URLs aren't valid WHATWG URLs). The catch silently returns the original URL without credentials. The git clone then fails with exit 128 (authentication error), but no warning is logged to onLog — the error only surfaces as a generic "Git clone failed". The user has no indication why.

Also affects the primary repo URL — if a user sets an SSH URL in adapter config, they'll get silent auth failures with no useful error.

Fix: detect SSH URLs before attempting new URL() and either warn early (fail-fast in execute) or log a clear warning before the clone attempt.

Fix this →


🟡 P2 — AbortError misclassified as timeout in outer catch

server/src/adapters/emisso-sandbox/execute.ts:365

const isTimeout = message.includes("AbortError") || ...

The outer catch block catches all errors — including AbortError from the cloneController (triggered by the 60s clone timeout) or installController (120s install timeout). These are logged as timedOut: true with message "sandbox execution timed out after Xs", but the timeout actually came from git clone or npm install, not the overall Claude execution. This is misleading in the UI — it shows the wrong timeout duration and hides the root cause.

Mitigation: use distinct error types/codes for clone vs install vs overall timeouts, or at minimum keep a phase variable and include it in the errorMessage.


🟡 P2 — Edit-mode MCP servers stored under wrong key

ui/src/adapters/emisso-sandbox/config-fields.tsx:168

onChange={(e) => {
  mark("adapterConfig", "mcpServersJson", e.target.value);
}}

In create mode, buildEmissoSandboxConfig correctly converts mcpServersJson (string) → mcpServers (object) before saving. But in edit mode, mark("adapterConfig", "mcpServersJson", ...) patches adapterConfig.mcpServersJson directly — the server receives a raw JSON string under a key it doesn't recognize instead of a parsed mcpServers object. This means MCP server edits on existing agents are silently discarded at the adapter level (the server reads config.mcpServers, not config.mcpServersJson).

Fix this →


🟡 P2 — Seed is non-idempotent and unwrapped in a transaction

packages/db/src/seed.ts:1-205

Two issues:

  1. No ON CONFLICT / idempotency guard — running pnpm seed twice creates duplicate companies, agents, issues, and budget policies without error.
  2. No transaction wrapping — if the projectWorkspaces, issues, or budgetPolicies inserts fail, the earlier rows (company, agents, goal, project) are permanently committed in an inconsistent state.

🟡 P2 — parse-stdout.ts result entry token counts hardcoded to 0

ui/src/adapters/emisso-sandbox/parse-stdout.ts:69-71

inputTokens: 0,
outputTokens: 0,
cachedTokens: 0,

The result transcript entry always displays 0 tokens in the UI even when Claude reports real usage. The server-side helpers.ts correctly extracts usage, but the UI-side parser doesn't plumb the token counts through. Users will see blank/zero token usage in the sandbox run transcript.

The result event carries total_cost_usd (correctly read at line 71) and modelUsage — the same aggregation logic from helpers.ts:59-68 should be applied here.

Fix this →


🔵 P3 — Parallel clone error-handling loses non-primary errors

server/src/adapters/emisso-sandbox/execute.ts:224-243

Promise.all rejects with the first error. When multiple clones fail simultaneously, only the first error's message is preserved. The others silently contribute to cloneController.abort(). In practice (most likely a token issue or network issue) all repos would fail for the same reason — but if additionalRepos have different access issues, the error message could be misleading.


🔵 P3 — MAX_STDOUT_BYTES cap allows single-chunk overshoot

server/src/adapters/emisso-sandbox/execute.ts:308

if (log.stream === "stdout" && fullStdout.length < MAX_STDOUT_BYTES) {
  fullStdout += log.data;
}

If log.data is a large chunk (e.g. Claude printing a big file), fullStdout can grow beyond 5 MB by up to one chunk. Low risk, but fullStdout.length + log.data.length <= MAX_STDOUT_BYTES is more precise.


🔵 P3 — supabase-bridge.ts: no invalidation of old sessions on re-auth

server/src/auth/supabase-bridge.ts:136-150

Each call to /auth/bridge creates a new session without invalidating previous ones for that user. A user who re-authenticates repeatedly accumulates live sessions indefinitely. The existing setInterval cleanup only runs on stale rateLimitStore entries, not sessions.

Low risk in the short term, but could become a security surface. Consider deleting the user's existing sessions (or the oldest Nth ones) before inserting the new one.


🔵 P3 — EmissoCreateConfigValues is missing maxTurns and model

ui/src/adapters/emisso-sandbox/build-config.ts:3-8

export interface EmissoCreateConfigValues extends CreateConfigValues {
  repoUrl?: string;
  vcpus?: number;
  timeoutSec?: number;
  snapshotId?: string;
  // maxTurns and model are missing here
}

model and maxTurns are accessed in both buildEmissoSandboxConfig (via ev.maxTurns, ev.model) and in config-fields.tsx (via values!.maxTurns, values!.model) but are absent from the interface. They're inherited from the base CreateConfigValues, but if that base ever tightens its types, these will silently fail TS checks. Explicit declaration improves clarity.


Summary

Severity Count Key issues
🔴 P1 2 proc.stdin unhandled EPIPE crash in runner.mjs; SSH URL silently loses git credentials
🟡 P2 4 AbortError misclassified as sandbox timeout; edit-mode MCP servers saved to wrong key; seed non-idempotent/untransacted; UI result entry always shows 0 tokens
🔵 P3 3 Parallel clone loses secondary errors; stdout cap overshoot; sessions accumulate on re-auth

The two P1s are worth fixing before merging since they cause silent failures that are hard to debug in production (the stdin EPIPE crash and the SSH URL credential loss both produce confusing non-zero exits with no actionable error message).

cdotte and others added 4 commits March 21, 2026 00:44
Seed the Emisso Factory company with 3 agents (Engineer, SDR, Marketing),
all using emisso_sandbox adapter pointing to emisso-hq. Includes project,
workspace, goal, sample issues, and budget policies.

Port the SDR skill from emisso-hq and add a new Marketing content skill.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove dead process launch in emisso-sandbox execute (P1: spawned
  claude then immediately killed it, wasting sandbox compute)
- Add missing "-" after --print in runner script (P1: stdin prompt
  was silently ignored without the positional arg)
- Move emisso-specific fields out of shared CreateConfigValues into
  EmissoCreateConfigValues to avoid polluting upstream types
- Fix MCP textarea stale state by always writing to mcpServersJson
- Use higher-entropy session token (256-bit hex vs UUID)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Log warning when instructionsFilePath not found (was silently ignored)
- Clamp cloneDepth to [1, 10000] (depth 0 caused full clones)
- Skip additionalRepos with empty repoUrl (was causing cryptic git errors)
- Broaden timeout detection to catch SDK-specific errors (TimeoutError, SandboxTimeoutError, 408, 504)
- Cap fullStdout accumulation at 5 MB to prevent unbounded memory growth
- Upgrade Vercel auth check from warn to error (missing auth blocks execution)
- Collect all tool_result blocks per user message, not just the first

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1 fixes:
- Add proc.on('error') and proc.stdin.on('error') handlers in runner
  script to prevent unhandled EPIPE crashes
- Throw on SSH/invalid URLs in embedGitCredentials instead of silently
  returning the original URL without credentials

P2 fixes:
- Edit-mode MCP textarea now writes both mcpServersJson (display) and
  mcpServers (parsed object the server reads)
- Seed is now idempotent (checks if company exists) and wrapped in a
  transaction to prevent partial state on failure
- Parse-stdout result entry now extracts real token counts from
  modelUsage/usage instead of hardcoding 0

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cdotte
cdotte force-pushed the feat/phase3-factory-seed-skills branch from 9eaf304 to 5005bfe Compare March 21, 2026 03:44
@github-actions

Copy link
Copy Markdown

Code review

3 issues found. Checked for bugs and CLAUDE.md compliance.


1. CLAUDE.md violation — packages/shared/src/constants.ts line 34

AGENT_ADAPTER_TYPES lives in @paperclipai/shared, a Paperclip-origin package. Adding "emisso_sandbox" directly into the array violates CLAUDE.md:

Rule: Never modify Paperclip shared types in-place — extend them

The compliant approach is to define an Emisso-owned constant that extends the upstream array in non-Paperclip code, rather than mutating AGENT_ADAPTER_TYPES directly. This spans multiple files: the constant definition plus all registry files and type guards that compare against this list.


2. CLAUDE.md violation — packages/adapter-utils/src/types.ts lines 347–349

CreateConfigValues is defined in @paperclipai/adapter-utils, a Paperclip-origin package. Injecting mcpServersJson and maxTurns directly into this interface violates the same rule.

Instead, declare an Emisso-owned extension in a new file and use it in the sandbox adapter config:

export interface EmissoCreateConfigValues extends CreateConfigValues {
  mcpServersJson?: string;
  maxTurns?: number;
}

3. Security — server/src/adapters/emisso-sandbox/execute.ts lines 230–233

When a git clone fails, stderr typically contains the full URL that was attempted. Since embedGitCredentials() embeds the token directly into the URL, the raw stderr.substring(0, 500) in the thrown error will expose the GITHUB_TOKEN. This propagates as errorMessage in the returned AdapterExecutionResult, stored in the database and displayed in the UI. The same pattern exists at line 198 (Claude CLI install error).

Fix — redact credentials before including stderr in the error message:

const safeStderr = stderr.replace(/\/\/[^:]+:[^@]+@/g, '//***:***@');
throw new Error(`Git clone failed for ${dirName} (exit ${result.exitCode}): ${safeStderr.substring(0, 500)}`);

@cdotte
cdotte merged commit c2302a4 into master Mar 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant