diff --git a/codev/projects/bugfix-1333-afx-send-builder-non-spawning-/status.yaml b/codev/projects/bugfix-1333-afx-send-builder-non-spawning-/status.yaml new file mode 100644 index 000000000..2545fda7e --- /dev/null +++ b/codev/projects/bugfix-1333-afx-send-builder-non-spawning-/status.yaml @@ -0,0 +1,16 @@ +id: bugfix-1333 +title: afx-send-builder-non-spawning- +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: pending + requested_at: '2026-08-02T04:35:30.554Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-02T04:02:14.892Z' +updated_at: '2026-08-02T04:35:30.554Z' +pr_ready_for_human: true diff --git a/codev/state/bugfix-1333_thread.md b/codev/state/bugfix-1333_thread.md new file mode 100644 index 000000000..bb740fb82 --- /dev/null +++ b/codev/state/bugfix-1333_thread.md @@ -0,0 +1,129 @@ +# bugfix-1333 — afx send drops descriptive NOT_FOUND reason + +Issue #1333 (area/tower). BUGFIX protocol, strict mode. + +## Investigate (complete) + +**Bug:** `afx send architect:` from a builder to a non-spawning architect prints +only `[error] NOT_FOUND`. The human/agent can't tell "no such architect" from "not +authorized to address that architect" — both look identical. + +**Root cause (verified in code):** Tower *produces* a descriptive message; the client +*drops* it in favor of the machine code. + +1. `tower-messages.ts:229-240` `resolveArchitectByName` returns a helpful `message`: + - spoofing: `builder may only address its own spawning architect` + - genuinely missing: `Architect '' not found in workspace ''.` +2. `tower-routes.ts:1472` `handleSend` serializes BOTH → `{ error: code, message }`. Wire OK. +3. **`core/src/tower-client.ts:221`** `error = json.error || json.message || text` + — prefers `json.error` (the code) over `json.message` (the detail). **← root cause.** +4. `tower-client.ts:695` `sendMessage` → `{ ok:false, error: result.error }` = `'NOT_FOUND'`. +5. `send.ts:329,334` `throw new Error(result.error)` → `fatal('NOT_FOUND')` → bare code. + +**Fix decision:** single point, `tower-client.ts:221`. When the response carries both a +machine `error` code and a human `message`, surface `" ()"`; otherwise fall +back to whichever is present, then `text`. This keeps the machine code visible (issue's +"keep the machine code available") and keeps the two cases distinguishable (issue's ask). + +**Blast radius (checked):** +- No client-side programmatic comparison of `.error` against code strings — the only + `=== 'NOT_FOUND'/'AMBIGUOUS'/'NO_CONTEXT'` checks are server-side on `result.code`. +- No existing test exercises `request()`'s extraction: core has no tower-client test and + no fetch mock; `send.test.ts` mocks `sendMessage` directly; server tests assert on + `result.code` and the HTTP response body. All unaffected. +- Change is strictly MORE informative for every Tower CLI error, not just send. + +**Fix chosen at `request()`** (global) rather than `send.ts` (local): the message is already +collapsed by the time it reaches `send.ts`, so a send-only fix would require plumbing a new +`message` field through `sendMessage` + `request` (3 files, return-type changes). The +one-line-family change at the drop site is smaller and benefits every caller. + +Scope: ~10 LOC + regression test. Comfortably within BUGFIX. + +## Fix (complete) + +**Change:** `packages/core/src/tower-client.ts` +- Extracted a module-private helper `extractTowerError(text)` (the previous + inline try/catch extraction was duplicated verbatim at TWO sites — `request()` + and `pasteImage()`; both had the identical drop-the-message defect). Per + lessons-critical "consolidate duplicates rather than syncing them", one helper + now feeds both sites. +- New logic: when both a machine `error` code and a human `message` are present + and distinct → `" ()"`; else `message || code || rawText`. + `typeof === 'string'` guards also harden against non-string `error` fields. + +**Regression test:** `packages/codev/src/agent-farm/__tests__/bugfix-1333-error-surfacing.test.ts` +- Lives in the codev package because porch's `test` check = `npm test` = + `pnpm --filter @cluesmith/codev test` (codev vitest only; core's own vitest is + NOT run by porch). It imports `TowerClient` via the codev re-export → built + core dist → the exact artifact the CLI consumes. So `npm run build` (rebuilds + core first) must precede `npm test`; porch runs them in that order. +- 6 cases: the #1333 spoofing scenario via `sendMessage`; spoofing-vs-missing + distinguishability; + 4 backward-compat guards (code-only, message-only, + equal code==message, non-JSON body). + +**Proven fails-without / passes-with:** stashed the core source, rebuilt, re-ran +→ cases 1 & 2 fail with `Received: "NOT_FOUND"` (the exact bug); 4 guards still +pass. Restored + rebuilt → all 6 pass. + +Full build + full codev suite: 4047 passed, 48 skipped, 0 failed. porch check ✓. + +## PR (in progress) + +Two atomic commits (Fix, Test). Pushed to `origin` (cluesmith/codev). +**PR #1334** → https://github.com/cluesmith/codev/pull/1334 (base main, "Fixes #1333"). +Remote topology: origin = cluesmith/codev (PR target), fork = mohidmakhdoomi/codev. + +CMAP (gemini/codex/claude, --protocol bugfix --type pr) run in background. +NOTE: `consult` couldn't auto-resolve the project in this worktree — the +disambiguator regex expects `.builders/-` but our worktree is +`.builders/bugfix-1333` (no suffix after the digits), so it errored "Multiple +projects found". Worked around with `--project-id 1333` (matches `bugfix-1333-…` +via the `bugfix-` branch in consult/index.ts:285). Possible separate bug. + +⚠️ BASE-SCOPE FLAG (for architect, not mine to fix): this branch was spawned +from an architect HEAD that is 2 commits ahead of origin/main — `fef6bddf +[Spec 1313] Initial specification draft` sits BELOW the porch-init commit. So +`origin/main...HEAD` includes `codev/specs/1313-afx-send-mailbox-first-delivery.md` +(+240 lines) — unrelated to #1333. Merging PR #1334 with --merge would land that +spec in main too. Not rewriting porch-tracked history unilaterally; flagged the +architect to decide (drop via rebase onto origin/main, or intentionally land it). +My own diff is exactly: tower-client.ts, the new test, the thread, + porch's +status.yaml. + +### CMAP verdicts (PR #1334) +- **gemini = REQUEST_CHANGES** (HIGH): code + tests "excellent"; sole blocker = + the unrelated Spec 1313 file. +- **codex = REQUEST_CHANGES** (HIGH): implementation "correct", tests "sound"; + sole blocker = same Spec 1313 file. +- **claude = APPROVE** (HIGH): independently verified the root-cause chain + + blast radius against source (confirmed no client-side `result.error===CODE` + consumers; the `.error?.includes` sites at tower-routes 562/621/1923/2580 are + server-side; typeof guards are genuine hardening). Spec 1313 flagged + NON-blocking. + +**Unanimous on the code: fix + test are correct and well-scoped.** The only +blocker is the inherited Spec 1313 file. Key de-risking fact (from claude): +`fef6bddf` is ALSO an ancestor of open PR #1330 (builder/spir-1313), so dropping +it here is LOSSLESS — the spec still lands via #1330. + +### Escalated to architect (queued via mailbox, main busy) +Asked for go-ahead before `git rebase --onto origin/main fef6bddf +builder/bugfix-1333` + force-with-lease — because that force-pushes already-pushed +history on a shared branch (outward-facing / hard-to-reverse → confirm first). +HOLDING the porch `pr`-gate request (`porch done`) until the base-scope question +is resolved. Awaiting architect decision: rebase-to-clean vs. merge-as-is. + +### Architect approved rebase-to-clean (2026-08-02) +Verified lossless (fef6bddf ⊂ PR #1330) + conflict-free (tower-client.ts untouched +on origin/main across 05b2f766..3f622fe6; test is a clean add). Executed: +1. committed the thread update (clean tree for rebase) +2. `git fetch origin` → origin/main = 3f622fe6 +3. `git rebase --onto origin/main fef6bddf builder/bugfix-1333` → 6 commits + replayed cleanly, NO conflicts, fef6bddf dropped +4. `git push --force-with-lease` → e26ce59a…2164b4bc (forced) +Pre-rebase HEAD saved: scratchpad/pre-rebase-head.txt (e26ce59a) for recovery. + +**PR #1334 now clean** — 4 files, no spec: status.yaml, thread, the test, +tower-client.ts (+249/-14). Re-verifying build+test on the rebased branch (base +moved), then re-request the `pr` gate. diff --git a/packages/codev/src/agent-farm/__tests__/bugfix-1333-error-surfacing.test.ts b/packages/codev/src/agent-farm/__tests__/bugfix-1333-error-surfacing.test.ts new file mode 100644 index 000000000..569b0910c --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/bugfix-1333-error-surfacing.test.ts @@ -0,0 +1,93 @@ +/** + * Regression test for #1333 — `afx send` surfaced a bare `[error] NOT_FOUND`, + * dropping the descriptive reason Tower produced. A builder addressing a + * non-spawning architect (`afx send architect:`) could not tell + * "no such architect" from "not authorized to address that architect": both + * showed the same opaque code. + * + * Root cause: TowerClient.request() extracted `json.error || json.message`, + * preferring the machine code and discarding the human `message`. The fix + * (packages/core/src/tower-client.ts) surfaces the descriptive `message` with + * the code as a parenthetical suffix, keeping both available. + * + * These tests exercise TowerClient via the codev re-export (`../lib/tower-client`) + * against a stubbed fetch, so they run under codev's vitest — the suite porch's + * `test` check runs — and validate the built core artifact the CLI consumes. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { TowerClient } from '../lib/tower-client.js'; + +/** Minimal fetch Response stand-in for an error body (JSON object or raw text). */ +function errorResponse(status: number, body: unknown): Response { + const text = typeof body === 'string' ? body : JSON.stringify(body); + return { + ok: false, + status, + text: async () => text, + } as unknown as Response; +} + +function stubFetch(response: Response): void { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response)); +} + +/** getAuthKey override keeps the constructor from touching the local key file. */ +function client(): TowerClient { + return new TowerClient({ getAuthKey: () => null }); +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +// The two distinct NOT_FOUND reasons Tower generates for `architect:`. +const SPOOFING = 'builder bugfix-1333 may only address its own spawning architect'; +const MISSING = "Architect 'codex-architect' not found in workspace 'codev'."; + +describe('TowerClient error surfacing (#1333)', () => { + it('sendMessage surfaces the descriptive spoofing reason with the code, not a bare NOT_FOUND', async () => { + stubFetch(errorResponse(404, { error: 'NOT_FOUND', message: SPOOFING })); + + const result = await client().sendMessage('architect:codex-architect', 'hi', { + from: 'bugfix-1333', + }); + + expect(result.ok).toBe(false); + // Pre-fix this was the bare code 'NOT_FOUND'; the reason is now preserved. + expect(result.error).toBe(`${SPOOFING} (NOT_FOUND)`); + }); + + it('keeps the spoofing and genuinely-missing NOT_FOUND reasons distinguishable', async () => { + stubFetch(errorResponse(404, { error: 'NOT_FOUND', message: MISSING })); + + const result = await client().request('/api/send', { method: 'POST' }); + + expect(result.error).toBe(`${MISSING} (NOT_FOUND)`); + // The two NOT_FOUND cases no longer collapse to the same opaque string. + expect(result.error).not.toContain('may only address'); + }); + + it('falls back to the bare code when the response carries no message', async () => { + stubFetch(errorResponse(503, { error: 'STARTING_UP' })); + const result = await client().request('/health'); + expect(result.error).toBe('STARTING_UP'); + }); + + it('uses the message alone when the response carries no code', async () => { + stubFetch(errorResponse(500, { message: 'something broke' })); + const result = await client().request('/x'); + expect(result.error).toBe('something broke'); + }); + + it('does not duplicate when code and message are identical', async () => { + stubFetch(errorResponse(500, { error: 'boom', message: 'boom' })); + const result = await client().request('/x'); + expect(result.error).toBe('boom'); + }); + + it('falls back to the raw body for a non-JSON error response', async () => { + stubFetch(errorResponse(502, 'Bad Gateway')); + const result = await client().request('/x'); + expect(result.error).toBe('Bad Gateway'); + }); +}); diff --git a/packages/core/src/tower-client.ts b/packages/core/src/tower-client.ts index dcc1c6196..e29478595 100644 --- a/packages/core/src/tower-client.ts +++ b/packages/core/src/tower-client.ts @@ -179,6 +179,31 @@ export interface TowerClientOptions { import { encodeWorkspacePath } from './workspace.js'; +/** + * Extract a human-facing error string from a Tower error response body (#1333). + * + * Tower error responses carry a machine `error` code and, for many cases, a + * human-readable `message` explaining *why* (e.g. the builder spoofing guard: + * "builder may only address its own spawning architect"). The previous + * extraction preferred the bare code and discarded the message, so the CLI + * surfaced an opaque `NOT_FOUND` with no reason. Surface the descriptive + * message when present, keeping the code as a parenthetical suffix so both the + * human reason and the machine code reach the caller. Falls back to the code + * alone, then to the raw (non-JSON) body text. + */ +function extractTowerError(text: string): string { + try { + const json = JSON.parse(text) as { error?: unknown; message?: unknown }; + const code = typeof json.error === 'string' ? json.error : undefined; + const detail = typeof json.message === 'string' ? json.message : undefined; + return detail && code && detail !== code + ? `${detail} (${code})` + : detail || code || text; + } catch { + return text; + } +} + export class TowerClient { private readonly baseUrl: string; private readonly getAuthKey: () => string | null; @@ -215,13 +240,7 @@ export class TowerClient { if (!response.ok) { const text = await response.text(); - let error: string; - try { - const json = JSON.parse(text); - error = json.error || json.message || text; - } catch { - error = text; - } + const error = extractTowerError(text); return { ok: false, status: response.status, error }; } @@ -624,13 +643,7 @@ export class TowerClient { }); if (!response.ok) { const text = await response.text(); - let error: string; - try { - const json = JSON.parse(text); - error = json.error || json.message || text; - } catch { - error = text; - } + const error = extractTowerError(text); return { ok: false, error }; } const data = (await response.json()) as { path: string };