feat: AI analysis for failed builds (migration + edge fn + CLI)#2284
Open
WcaleNieWolny wants to merge 28 commits into
Open
feat: AI analysis for failed builds (migration + edge fn + CLI)#2284WcaleNieWolny wants to merge 28 commits into
WcaleNieWolny wants to merge 28 commits into
Conversation
Failing tests for aiAnalyzeBuild — module does not exist yet. Task 13 will implement supabase/functions/_backend/public/build/ai_analyze.ts.
Implements the AI build analysis endpoint that validates permissions, checks build ownership/status/idempotency, proxies to capgo_builder, and flips the ai_analyzed flag on success.
Adds test/test-ai-analyze-flow.mjs covering decideAnalyzeBehavior matrix, writeLocalAiFile, and postAnalyzeRequest (200/409/5xx). Fails with "Cannot find module '../src/ai/analyze.ts'" until Task 18 implements it.
Adds the --ai-analytics CLI flag to `build request`, types it in the buildRequestOptionsSchema, and wires the full AI analysis flow in request.ts: interactive menu (Capgo AI vs local file), CI auto-upload, ask_then_menu for TTY without flag, and a log-too-big guard path.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an AI-assisted build-failure analysis flow: a byte-identical SYSTEM_PROMPT with CI sync, TTY-gated per-job log capture, decision/local/remote analyze logic, CLI integration to capture and act on failed builds, a backend proxy endpoint with idempotency, DB typing, and tests. ChangesAI-Assisted Build Failure Analysis
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Disk
participant API as /functions/v1/build/ai_analyze
participant Builder as BUILDER_URL/jobs/{jobId}/ai-analyze
participant DB as Supabase
User->>CLI: build request (maybe --ai-analytics)
CLI->>Disk: startCaptureForJob, appendCapturedLine (stream logs)
CLI->>CLI: build fails, decideAnalyzeBehavior
alt auto_upload
CLI->>Disk: read captured logs
CLI->>API: POST /build/ai_analyze (jobId, appId, logs)
API->>API: checkPermission, load build_requests
API->>API: validate failed status & ai_analyzed flag
API->>Builder: POST /jobs/{jobId}/ai-analyze (logs)
Builder-->>API: { analysis }
API->>DB: UPDATE build_requests SET ai_analyzed=true
API-->>CLI: { analysis }
CLI->>User: renderMarkdown(analysis) to stdout/stderr
else show_menu
CLI->>User: select Capgo AI or local AI or skip
alt Capgo AI selected
CLI->>API: same flow as auto_upload
else local AI selected
CLI->>Disk: writeLocalAiFile (write SYSTEM_PROMPT + logs)
CLI->>User: print local prompt file path
end
end
CLI->>Disk: cleanupCapturedJobFiles (on exit/signal)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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: 5dc0930f3b
ℹ️ 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".
| const res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'capgo-api-key': input.apikey, |
There was a problem hiding this comment.
postAnalyzeRequest sends the API key as capgo-api-key, but middlewareKey only accepts authorization, capgkey, or x-api-key in supabase/functions/_backend/utils/hono_middleware.ts. In practice this makes the new /build/ai_analyze request fail auth even with a valid key, so the CLI AI analysis path cannot succeed.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // --- Task 20: AI failure flow --- | ||
| if (finalStatus === 'failed' && captureEnabled && capturedJobId) { |
There was a problem hiding this comment.
The failure-path gate requires captureEnabled && capturedJobId, but captureEnabled is derived from shouldCaptureLogs() (TTY-only). In CI/non-TTY runs this condition is false, so the new --ai-analytics auto-upload path is never reached even though decideAnalyzeBehavior explicitly maps non-interactive+flag to auto_upload.
Useful? React with 👍 / 👎.
| void cleanupCapturedJobFiles(jobId, { keepAiPromptFile: getKeepPromptFile() }) | ||
| } | ||
| const onExit = () => cleanup() | ||
| const onSignal = () => { cleanup(); process.exit(130) } |
There was a problem hiding this comment.
This handler calls process.exit(130) on SIGINT. Because it is registered before the build-specific SIGINT handler in request.ts, Ctrl+C in interactive builds exits the process before the normal cancel flow can call /build/cancel/:jobId, so user cancellation no longer performs graceful remote cancellation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
cli/src/build/request.ts (1)
1896-1905: ⚡ Quick winAvoid duplicating path construction logic.
The path
${process.env.CAPGO_AI_LOG_BASE_DIR || '/tmp/capgo-builds'}/${capturedJobId}.logduplicates the logic fromlog-capture.ts. If the path format changes, this will become inconsistent.Consider exporting and reusing a
getLogPath(jobId)helper fromlog-capture.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/build/request.ts` around lines 1896 - 1905, The logsPath construction in runCapgoAi duplicates the path logic from log-capture.ts; update runCapgoAi to import and call a shared helper (e.g., getLogPath(jobId)) exported from log-capture.ts instead of building `${process.env.CAPGO_AI_LOG_BASE_DIR || '/tmp/capgo-builds'}/${capturedJobId}.log` directly: add an import for getLogPath, replace the logsPath assignment with getLogPath(capturedJobId) and keep the rest of the file read logic unchanged.tests/build-ai-analyze.test.ts (1)
67-126: 💤 Low valueConsider using
it.concurrent()for parallel test execution.Per coding guidelines, tests should use
it.concurrent()instead ofit()to run tests in parallel within the same file for faster CI/CD.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/build-ai-analyze.test.ts` around lines 67 - 126, Replace the synchronous test calls with concurrent ones inside the describe('aiAnalyzeBuild', ...) block: change each it( ... ) to it.concurrent( ... ) for all test cases (the tests that call aiAnalyzeBuild, mockCheckPermission, mockBuildRequestRow, and use global.fetch), ensuring you update every occurrence in that block so the suite runs tests in parallel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-ai-prompt-sync.yml:
- Around line 14-17: The workflow job check-prompt-sync currently relies on
default GITHUB_TOKEN scopes; add an explicit permissions block for least
privilege (at minimum set contents: read) to the job definition so the job uses
the restricted token scope. Update the jobs.check-prompt-sync section to include
a permissions: block (e.g., permissions: { contents: read }) directly under the
job name so GitHub applies the limited token for that job.
In `@cli/src/ai/analyze.ts`:
- Around line 1-3: Reorder the import block so built-in modules
(node:fs/promises) come first, then external packages, then local modules
(SYSTEM_PROMPT, getLogCapturePath, getAiPromptPath) to satisfy import-order
rules; ensure any if statements (look for conditionals using
getAiPromptPath/getLogCapturePath or around the ANALYZE logic) have a blank line
after the closing brace as required by newline-after-if; and fix operator
linebreaks by placing operators at the end of the previous line (not at the
start of the next line) in expressions around string concatenation or multi-line
conditions (search for lines near where SYSTEM_PROMPT is combined or where
readFile/writeFile/stat results are used). Run the linter to confirm all ESLint
errors are resolved before committing.
- Around line 39-40: The PostAnalyzeResult type includes a { kind: 'too_big' }
variant but postAnalyzeRequest never returns it; modify postAnalyzeRequest to
explicitly detect an HTTP 413 response from the backend (check response.status
=== 413) and return { kind: 'too_big' } before the generic error handling path
that produces { kind: 'error', ... }; update any fetch/response branches in
postAnalyzeRequest so 413 is handled early and not treated as a generic error.
In `@cli/src/ai/log-capture.ts`:
- Line 1: Reformat the top-level imports and the offending lines to satisfy
ESLint: reorder the import so it follows the project's import ordering
(third-party and local imports first, then builtin node:fs/promises) and keep a
single import statement per line for mkdir, unlink, writeFile, appendFile;
ensure any multi-statement lines (around the usages of writeFile/appendFile at
the mentioned spots) are split so each statement is on its own line; replace
string concatenation with template literals where required and normalize
newlines/semicolons to match the repo style so bun run cli:test no longer flags
import ordering, template usage, or single-statement-per-line/newline
violations.
- Around line 63-68: The onUncaught handler currently only calls cleanup() which
prevents Node's default fatal-exit; update the uncaughtException listener (the
onUncaught function used in process.once('uncaughtException', onUncaught)) to
perform cleanup and then terminate the process (e.g., call process.exit(1)) or
rethrow the error so the process doesn't continue in a corrupted state; ensure
cleanup() is awaited if it returns a promise before exiting.
In `@cli/src/build/request.ts`:
- Around line 1606-1617: The registered cleanup handler stored in
_unregisterCleanup is never called; update the flow that handles capture (around
startCaptureForJob/registerCleanupHandlers and keepPromptFile) to ensure
_unregisterCleanup is invoked when the build completes or errors (e.g., in a
finally block), using optional chaining like _unregisterCleanup?.() and then set
_unregisterCleanup = null so the exit/signal handlers registered by
registerCleanupHandlers are properly unregistered when done.
In `@supabase/functions/_backend/public/build/ai_analyze.ts`:
- Around line 79-86: The fetch call that assigns builderResp (calling
`${builderUrl}/jobs/${jobId}/ai-analyze` with headers using builderApiKey and
body JSON.stringify({ logs })) needs a timeout: create an AbortController, pass
its signal into the fetch options, start a timer (configurable or reasonable
default) that calls controller.abort() when exceeded, clear the timer on
successful response, and handle the abort error path so the function
returns/throws a clear timeout error rather than hanging; update the fetch
invocation to include the signal and ensure any cleanup/try/catch around
builderResp handles aborted requests.
In `@tests/build-ai-analyze.test.ts`:
- Around line 63-65: Replace uses of the Node-restricted global identifier and
the unnecessary TypeScript suppression with the standard globalThis: change
occurrences where the test assigns the mock (e.g., the line setting global.fetch
= vi.fn() and the other occurrences at the places referenced in the comment) to
use globalThis.fetch = vi.fn() and remove the leading "// `@ts-expect-error`"
directive; update the same replacement at the other occurrences noted (the
references around lines 99, 110, and 120 in tests/build-ai-analyze.test.ts) so
all fetch mocks use globalThis.fetch consistently.
---
Nitpick comments:
In `@cli/src/build/request.ts`:
- Around line 1896-1905: The logsPath construction in runCapgoAi duplicates the
path logic from log-capture.ts; update runCapgoAi to import and call a shared
helper (e.g., getLogPath(jobId)) exported from log-capture.ts instead of
building `${process.env.CAPGO_AI_LOG_BASE_DIR ||
'/tmp/capgo-builds'}/${capturedJobId}.log` directly: add an import for
getLogPath, replace the logsPath assignment with getLogPath(capturedJobId) and
keep the rest of the file read logic unchanged.
In `@tests/build-ai-analyze.test.ts`:
- Around line 67-126: Replace the synchronous test calls with concurrent ones
inside the describe('aiAnalyzeBuild', ...) block: change each it( ... ) to
it.concurrent( ... ) for all test cases (the tests that call aiAnalyzeBuild,
mockCheckPermission, mockBuildRequestRow, and use global.fetch), ensuring you
update every occurrence in that block so the suite runs tests in parallel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ee945cc-ee92-4a71-b374-6869c7ea2000
📒 Files selected for processing (15)
.github/workflows/check-ai-prompt-sync.ymlcli/package.jsoncli/src/ai/analyze.tscli/src/ai/log-capture.tscli/src/ai/prompt.tscli/src/build/request.tscli/src/index.tscli/src/schemas/build.tscli/test/test-ai-analyze-flow.mjscli/test/test-ai-log-capture.mjssupabase/functions/_backend/public/build/ai_analyze.tssupabase/functions/_backend/public/build/index.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260518120000_add_ai_analyzed_to_build_requests.sqltests/build-ai-analyze.test.ts
Comment on lines
+1606
to
+1617
| // --- Task 19: /tmp log capture setup --- | ||
| const captureEnabled = shouldCaptureLogs() | ||
| let capturedJobId: string | null = null | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| let _unregisterCleanup: (() => void) | null = null | ||
| let keepPromptFile = false // Task 20: mutable so local-AI flow can set it true | ||
|
|
||
| if (captureEnabled && buildRequest.job_id) { | ||
| capturedJobId = buildRequest.job_id | ||
| await startCaptureForJob(buildRequest.job_id) | ||
| _unregisterCleanup = registerCleanupHandlers(buildRequest.job_id, () => keepPromptFile) | ||
| } |
Contributor
There was a problem hiding this comment.
_unregisterCleanup is assigned but never called.
The cleanup handler is registered but never unregistered when the build completes normally. This means the process exit/signal handlers remain attached, which could cause issues if this function is called multiple times (e.g., in SDK usage) or if cleanup runs unexpectedly after the function returns.
🔧 Proposed fix
- // eslint-disable-next-line `@typescript-eslint/no-unused-vars`
- let _unregisterCleanup: (() => void) | null = null
+ let unregisterCleanup: (() => void) | null = null
let keepPromptFile = false // Task 20: mutable so local-AI flow can set it true
if (captureEnabled && buildRequest.job_id) {
capturedJobId = buildRequest.job_id
await startCaptureForJob(buildRequest.job_id)
- _unregisterCleanup = registerCleanupHandlers(buildRequest.job_id, () => keepPromptFile)
+ unregisterCleanup = registerCleanupHandlers(buildRequest.job_id, () => keepPromptFile)
}Then call unregisterCleanup?.() before returning from the function (in the finally block or after the AI flow completes).
🧰 Tools
🪛 ESLint
[error] 1609-1609: Definition for rule '@typescript-eslint/no-unused-vars' was not found.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/build/request.ts` around lines 1606 - 1617, The registered cleanup
handler stored in _unregisterCleanup is never called; update the flow that
handles capture (around startCaptureForJob/registerCleanupHandlers and
keepPromptFile) to ensure _unregisterCleanup is invoked when the build completes
or errors (e.g., in a finally block), using optional chaining like
_unregisterCleanup?.() and then set _unregisterCleanup = null so the exit/signal
handlers registered by registerCleanupHandlers are properly unregistered when
done.
Without this, the CI auto_upload branch returned by decideAnalyzeBehavior never had logs to send because the capture gate was TTY-only. Also drops the no-op _unregisterCleanup local var.
Calling process.exit(130) from our cleanup handler ran before the build command's own SIGINT handler, so Ctrl+C skipped the graceful /build/cancel/:jobId call. The cleanup now only removes /tmp files and returns; Node exits naturally when no other handler defers it.
- Handle HTTP 413 explicitly in postAnalyzeRequest -> { kind: 'too_big' }
so the type variant we declared is actually reachable; backend can use 413
when it rejects oversized logs (cli/src/ai/analyze.ts + new bun test).
- Add 60s AbortSignal.timeout to the edge fn's builder fetch so a hung
Workers AI call fails fast with builder_error instead of leaving the
edge fn open until the platform's 150s wall-clock timeout.
- uncaughtException handler now rethrows after cleanup (via setImmediate)
so Node's default print+exit behavior is preserved; signal handlers
still yield to the build's own SIGINT cancel flow.
- Lint cleanup auto-applied via 'bun run lint:fix' (import ordering,
template literals, max-statements-per-line, etc.) across analyze.ts,
log-capture.ts, request.ts.
- Tests: replace 'global.fetch' with 'globalThis.fetch' and drop the
unused @ts-expect-error directive (eslint no-restricted-globals).
- New test: postAnalyzeRequest returns too_big on 413.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/build-ai-analyze.test.ts (1)
67-124: ⚡ Quick winUse
it.concurrent()for this suite to match test parallelization rules.These new tests currently use
it(...); please convert toit.concurrent(...)and keep mocks isolated per test as needed.As per coding guidelines, "Design all tests for parallel execution across files; use it.concurrent() instead of it() to run tests in parallel within the same file for faster CI/CD".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/build-ai-analyze.test.ts` around lines 67 - 124, Convert each test invocation from it(...) to it.concurrent(... ) in the aiAnalyzeBuild suite (all tests that call aiAnalyzeBuild, mockCheckPermission, mockBuildRequestRow, and globalThis.fetch), and ensure mocks remain isolated by resetting/clearing them in a beforeEach (e.g., jest.clearAllMocks()/mockReset) so each concurrent test sets up its own mockCheckPermission/mockBuildRequestRow/globalThis.fetch state; verify you update the test cases that assert updateEqApp and fetch call parameters to use it.concurrent as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/build/request.ts`:
- Around line 1902-1908: The catch block that silently returns when reading
logsPath fails should emit a short stderr error so users can diagnose failed
auto-upload paths; update the try/catch around the dynamic import and readFile
(the block that reads logsPath) to log a concise message to stderr (e.g., via
console.error or process.stderr.write) including the logsPath and the caught
error message, then return as before to preserve behavior.
In `@tests/build-ai-analyze.test.ts`:
- Line 36: The failures come from lint rules about inline object type delimiters
and formatting of if/newline and chained calls; fix by extracting the inline
parameter type into a named type or interface (e.g., type MockBuildRequestRow =
{ app_id: string; status: string; ai_analyzed: boolean } | null) and use that
type in mockBuildRequestRow(row: MockBuildRequestRow), and then update the
affected blocks (the mockBuildRequestRow declaration and the other occurrences
at the listed locations) to follow consistent chaining and if-newline style:
ensure multi-line if statements put the body on the next line and break long
chains so each dot-starting call is on its own line. Apply these same changes to
the other instances noted (lines ~59-60, 70, 77, 84, 92, 101) to satisfy
style/member-delimiter-style, antfu/if-newline, and antfu/consistent-chaining.
---
Nitpick comments:
In `@tests/build-ai-analyze.test.ts`:
- Around line 67-124: Convert each test invocation from it(...) to
it.concurrent(... ) in the aiAnalyzeBuild suite (all tests that call
aiAnalyzeBuild, mockCheckPermission, mockBuildRequestRow, and globalThis.fetch),
and ensure mocks remain isolated by resetting/clearing them in a beforeEach
(e.g., jest.clearAllMocks()/mockReset) so each concurrent test sets up its own
mockCheckPermission/mockBuildRequestRow/globalThis.fetch state; verify you
update the test cases that assert updateEqApp and fetch call parameters to use
it.concurrent as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb660968-a749-4e38-9064-55e77d230156
📒 Files selected for processing (7)
.github/workflows/check-ai-prompt-sync.ymlcli/src/ai/analyze.tscli/src/ai/log-capture.tscli/src/build/request.tscli/test/test-ai-analyze-flow.mjssupabase/functions/_backend/public/build/ai_analyze.tstests/build-ai-analyze.test.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/check-ai-prompt-sync.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/test/test-ai-analyze-flow.mjs
- cli/src/ai/log-capture.ts
- supabase/functions/_backend/public/build/ai_analyze.ts
3 tasks
The ai_analyzed column migration is now in its own PR so the DB schema change can be reviewed and applied independently. This PR keeps the types update (supabase.types.ts) since edge function tests reference the new field name; the actual column is created by the migration in the standalone migration PR. Depends on #2285 being merged + the migration applied to the shared DB before this PR's runtime path works.
riderx
pushed a commit
that referenced
this pull request
May 18, 2026
Adds a boolean column with default FALSE used by the upcoming AI build analytics feature for one-analysis-per-job idempotency. Backward-compatible schema change — existing rows get the default; no app code on main reads or writes this column yet. The application code that consumes this column lands in PR #2284 (AI build analytics feature). This migration ships separately so the schema change can be reviewed and applied independently from the larger feature.
…upabase
postAnalyzeRequest was hitting ${apiHost}/functions/v1/build/ai_analyze
(Supabase Edge Functions convention), but apiHost is the Capgo CF Workers
API gateway (api.capgo.app / api.preprod.capgo.app). Routes there are
mounted directly under the host — every other /build/* endpoint (start,
cancel, status, logs) uses just ${host}/build/... and that's what works
in preprod.
Verified against preprod:
/functions/v1/build/ai_analyze -> 404
/build/ai_analyze -> 401 (route exists, just needs auth)
Test updated to expect the correct URL.
… warning
Four small additions following preprod testing feedback:
1. Prompt-injection defense (matches capgo_builder change):
- SYSTEM_PROMPT (kept in sync) gains a SECURITY section telling Kimi
the user message is untrusted DATA inside <BUILD_LOG>...</BUILD_LOG>
tags, not instructions.
- writeLocalAiFile now uses the same boundary so users running the
local-AI file against any LLM get the same protection.
2. Spinner during the Capgo AI request — wraps postAnalyzeRequest with
@clack/prompts spinner so the user sees 'Analyzing build log…' instead
of an unexplained pause. Skipped when stdout isn't a TTY (CI).
3. Tiny terminal markdown renderer (cli/src/ai/render-markdown.ts, no new
dep, ~85 lines). Handles ### headers, fenced code blocks, **bold**,
*italic*, `inline code`, numbered + bullet lists via raw ANSI escapes.
Passes input through unchanged when stdout isn't a TTY so output stays
pipeable. 8 unit tests.
4. 'AI can make mistakes — verify before applying fixes' warning printed
after every analysis (both Capgo AI and Local AI flows).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/src/build/request.ts (1)
2000-2002: ⚡ Quick winUse
formatError(...)for this user-facing error output.Line 2001 currently prints a raw error string; route it through the shared formatter for consistent CLI UX.
As per coding guidelines, "For user-visible error messages, format errors with
formatError(...)instead of dumping raw exceptions when possible".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/build/request.ts` around lines 2000 - 2002, The catch block that writes AI analysis errors to stderr currently prints a raw error string; replace that raw output with the shared user-facing formatter by passing the caught error into formatError(...) before writing to process.stderr. Locate the catch handling in the AI analysis flow (the catch block containing process.stderr.write(`AI analysis flow errored: ...`)) and change the message to use formatError(err) (or formatError(err instanceof Error ? err : new Error(String(err))) if needed) so the CLI displays the formatted error consistently.cli/test/test-ai-analyze-flow.mjs (1)
16-21: ⚡ Quick winPrefer async/await in the test harness instead of
.then()chaining.Line 16-Line 21 can be rewritten as
async/awaitto match repo standards and keep error flow clearer.Proposed change
-function test(name, fn) { - return Promise.resolve() - .then(() => fn()) - .then(() => { console.log(`✅ ${name}`); passed++ }) - .catch((err) => { console.error(`❌ ${name}\n ${err.message}`); failed++ }) +async function test(name, fn) { + try { + await fn() + console.log(`✅ ${name}`) + passed++ + } + catch (err) { + console.error(`❌ ${name}\n ${err.message}`) + failed++ + } }As per coding guidelines, "**/*.{ts,tsx,js}: Always use async/await instead of promises with .then() chains".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/test/test-ai-analyze-flow.mjs` around lines 16 - 21, The test harness uses Promise.then() chaining in the test function; rewrite function test(name, fn) as an async function using try/catch and await to call fn(), then increment passed or failed and log results; keep the same messages (`✅ ${name}` and `❌ ${name}\n ${err.message}`) and the passed/failed increments so behavior is unchanged while replacing the .then()/.catch() chain with async/await and try/catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/ai/render-markdown.ts`:
- Around line 57-75: The three regex-based matchers (headerMatch, numberedMatch,
bulletMatch) on the raw line can cause super-linear backtracking; replace them
with bounded parsing using cheap prefix checks and index/slice instead: for
headers, check if raw startsWith '#' then count leading '#' up to 6, verify next
char is space and slice the header text to produce the same text and level used
with stylize; for numbered lists, scan for leading whitespace, then parse
consecutive digits up to the first '.' (validate the '.' and following space) to
extract indent, number (n) and rest, then call renderInline/restyled number as
before; for bullets, detect leading whitespace then check next non-space char is
'-' or '*' and a following space, slice out the rest and handle as before; keep
using stylize, ANSI constants and renderInline to preserve behavior.
- Around line 34-38: The renderMarkdown function contains multiple lint/regex
issues: make the first regex in stylization (the pattern using *? at line with
replace) greedy by removing the useless lazy quantifier (change `*?` to `*`),
fix the if statement that returns early when !isTTY by adding a newline or
braces so the `if` has its own block, and refactor the three vulnerable regexes
(the patterns at/near lines 57, 66, 74 in render-markdown.ts) to eliminate
overlapping `\s+` with `.+`/`.*` (replace with negated character classes like
`[^\\s]+` or use atomic grouping/possessive constructs) to prevent super-linear
backtracking while preserving the intended matches; update the associated calls
in renderMarkdown/stylize accordingly.
---
Nitpick comments:
In `@cli/src/build/request.ts`:
- Around line 2000-2002: The catch block that writes AI analysis errors to
stderr currently prints a raw error string; replace that raw output with the
shared user-facing formatter by passing the caught error into formatError(...)
before writing to process.stderr. Locate the catch handling in the AI analysis
flow (the catch block containing process.stderr.write(`AI analysis flow errored:
...`)) and change the message to use formatError(err) (or formatError(err
instanceof Error ? err : new Error(String(err))) if needed) so the CLI displays
the formatted error consistently.
In `@cli/test/test-ai-analyze-flow.mjs`:
- Around line 16-21: The test harness uses Promise.then() chaining in the test
function; rewrite function test(name, fn) as an async function using try/catch
and await to call fn(), then increment passed or failed and log results; keep
the same messages (`✅ ${name}` and `❌ ${name}\n ${err.message}`) and the
passed/failed increments so behavior is unchanged while replacing the
.then()/.catch() chain with async/await and try/catch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d81943f0-ed4e-4efe-b786-d4d8693ae7bf
📒 Files selected for processing (7)
cli/package.jsoncli/src/ai/analyze.tscli/src/ai/prompt.tscli/src/ai/render-markdown.tscli/src/build/request.tscli/test/test-ai-analyze-flow.mjscli/test/test-ai-render-markdown.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/ai/prompt.ts
- cli/package.json
- cli/src/ai/analyze.ts
Old: every code line in cyan with the literal triple-backtick fence lines shown in gray — fence syntax leaked through and cyan content was harder to read than default terminal text. New: fence lines hidden entirely (the bar IS the 'this is code' signal); each code line prefixed with '▎ ' in dim gray, content in default color. Blank lines inside the block still get the bar so the left edge stays unbroken. Mirrors git-diff / GitHub-review styling. Updated tests cover both fence-hiding and the unbroken-bar invariant.
cli/src/build/request.ts (runCapgoAi auto-upload path) Silent catch hid genuine failures from users (e.g. log file missing because capture wasn't enabled, or /tmp permissions). Now write a concise diagnostic to stderr with the log path and the underlying error message before returning. tests/build-ai-analyze.test.ts ESLint auto-fix only — splits chained .rejects.toThrow across lines (antfu/consistent-chaining), adds newline after multi-line if bodies (antfu/if-newline), changes inline-object-type delimiters from ';' to ',' (style/member-delimiter-style), and adds the required trailing comma in the multi-arg Response constructor. No behavioural change. Skipped: coderabbit's nitpick to convert it(...) to it.concurrent(...). The suite finishes in ~250ms; the speedup isn't worth the mock-isolation risk the nitpick itself flags, and beforeEach already uses mockReset() which would need a careful audit to confirm safe under concurrent runs.
5 lint errors flagged by regexp/no-super-linear-backtracking and friends: 1. Removed useless lazy quantifier in *italic* regex (`[^*]*?` → `[^*]*` — `[^*]` already excludes the closing `*`, so lazy/greedy makes no difference) — auto-fix. 2. `if (!isTTY) return` split onto two lines (antfu/if-newline) — auto-fix. 3-5. Header/numbered-list/bullet-list regexes used `\s+` which overlaps with the captured `.+`/`.*` rest, allowing polynomial backtracking on input like `### " + ' '*1000`. Switched to literal ` +` for the separator and anchored the captured rest with `\S` so the engine can't backtrack into the spaces. Real markdown headers/list items always have a non-space character after the marker anyway. All 9 renderer tests still pass; lint now clean on the file.
# Conflicts: # cli/package.json
…now) The bidirectional check needed cross-repo read access — feasible for the public capgo repo (anonymous raw URL works) but required a PAT secret (GH_TOKEN_READ_BUILDER) for the private capgo_builder side. Not worth the secret-provisioning overhead. The prompt files already document capgo_builder as the source of truth. The remaining check on the capgo_builder side catches drift whenever the worker prompt is edited; drift introduced by editing the CLI mirror first gets caught the next time anything touches the worker prompt — close enough for v1. If you want stricter protection later, options: - Make the worker prompt the literal source via a generate-mirror script - Use a GitHub App with cross-repo read scope - Vendor the prompt as a tiny shared npm package
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Single PR covering the capgo half of the AI build analytics feature. Worker side is at Cap-go/capgo_builder#93.
Migration
Edge function (`supabase/functions/_backend/public/build/ai_analyze.ts`)
CLI (`cli/`)
CI
Deferred
Design doc: docs/superpowers/specs/2026-05-18-capgo-ai-build-analytics-design.md (in this branch).
Test plan
Summary by CodeRabbit
New Features
Tests
Chores