Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/commands/feedback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ const FEEDBACK_ENDPOINT =
process.env.INSTA_FEEDBACK_URL ||
'https://feedback.instacloud.com/v1/feedback'
const FEEDBACK_INGEST_TOKEN = process.env.INSTA_FEEDBACK_TOKEN || 'insta-feedback-public-v1'
const FEEDBACK_TIMEOUT_MS = 10_000
// 15s gives the backend's scale-to-zero cold start room to answer (the ingest service waits out
// the DB wake and persists, so a report can land after the old 10s deadline gave up on it).
// An expired deadline is reported as UNCONFIRMED, not failed — the report may well be stored.
const FEEDBACK_TIMEOUT_MS = 15_000
const MAX_FILE_BYTES = 256 * 1024

export type FeedbackOpts = {
Expand Down Expand Up @@ -190,6 +193,9 @@ export async function buildPayload(

export type SubmitResult =
| { status: 'received' | 'duplicate'; id: string | null }
// unconfirmed = the deadline expired with the request in flight: the server does not abort
// mid-request, so the report may have been stored — materially different from 'error'.
| { status: 'unconfirmed'; error: string }
| { status: 'error'; error: string }

/** One POST, 10s timeout, zero retries — feedback is a side quest and must never hang the CLI.
Expand All @@ -208,8 +214,10 @@ export async function submit(payload: Record<string, unknown>, fetchImpl: typeof
signal: AbortSignal.timeout(FEEDBACK_TIMEOUT_MS),
})
} catch (e) {
const timedOut = e instanceof Error && e.name === 'TimeoutError'
return { status: 'error', error: timedOut ? `timed out after ${FEEDBACK_TIMEOUT_MS / 1000}s` : `network error: ${e instanceof Error ? e.message : String(e)}` }
if (e instanceof Error && e.name === 'TimeoutError') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When headers arrive before 30s but the response body is still streaming, res.json() throws TimeoutError and the inner catch reports received. Catch deadline expiry during body parsing and return unconfirmed so the CLI does not claim a receipt when persistence is unknown.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/feedback.ts, line 217:

<comment>When headers arrive before 30s but the response body is still streaming, `res.json()` throws `TimeoutError` and the inner catch reports `received`. Catch deadline expiry during body parsing and return `unconfirmed` so the CLI does not claim a receipt when persistence is unknown.</comment>

<file context>
@@ -208,8 +214,10 @@ export async function submit(payload: Record<string, unknown>, fetchImpl: typeof
   } catch (e) {
-    const timedOut = e instanceof Error && e.name === 'TimeoutError'
-    return { status: 'error', error: timedOut ? `timed out after ${FEEDBACK_TIMEOUT_MS / 1000}s` : `network error: ${e instanceof Error ? e.message : String(e)}` }
+    if (e instanceof Error && e.name === 'TimeoutError') {
+      return { status: 'unconfirmed', error: `no response after ${FEEDBACK_TIMEOUT_MS / 1000}s — the report may have been recorded anyway` }
+    }
</file context>

return { status: 'unconfirmed', error: `no response after ${FEEDBACK_TIMEOUT_MS / 1000}s — the report may have been recorded anyway` }
}
return { status: 'error', error: `network error: ${e instanceof Error ? e.message : String(e)}` }
}
let body: any = {}
try {
Expand Down Expand Up @@ -240,6 +248,13 @@ export async function feedback(opts: FeedbackOpts, deps: FeedbackDeps = {}): Pro

const result = await submit(payload, deps.fetchImpl ?? fetch)

if (result.status === 'unconfirmed') {
// NOT a failure claim: the request was still in flight at the deadline and the server
// finishes what it started, so saying "not submitted" here would be a false negative.
if (opts.json) return printJson({ status: 'unconfirmed', error: result.error })
process.stderr.write(`warning: feedback receipt unconfirmed (${result.error}) — continue with your task, do not retry\n`)
return
}
if (result.status === 'error') {
// Deliberate exit 0: an agent CANNOT fix a down/rate-limited backend, and feedback must never
// fail or distract from the task the user actually asked for. Do not retry.
Expand Down
21 changes: 21 additions & 0 deletions test/feedback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,17 @@ describe('submit', () => {
expect(result.status).toBe('error')
expect((result as { error: string }).error).toContain('ENOTFOUND')
})

it('a timeout is UNCONFIRMED, not an error — the server finishes in-flight requests', async () => {
const fetchImpl = (async () => {
const e = new Error('aborted')
e.name = 'TimeoutError'
throw e
}) as unknown as typeof fetch
const result = await submit({}, fetchImpl)
expect(result.status).toBe('unconfirmed')
expect((result as { error: string }).error).toContain('may have been recorded')
})
})

describe('feedback command', () => {
Expand All @@ -162,6 +173,16 @@ describe('feedback command', () => {
}) as unknown as typeof fetch
await expect(feedback({ ...valid, json: true }, { interactive: false, cliVersion: 'x', fetchImpl })).resolves.toBeUndefined()
})

it('an unconfirmed timeout does not throw either', async () => {
const fetchImpl = (async () => {
const e = new Error('aborted')
e.name = 'TimeoutError'
throw e
}) as unknown as typeof fetch
await expect(feedback({ ...valid, json: true }, { interactive: false, cliVersion: 'x', fetchImpl })).resolves.toBeUndefined()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This test gives false confidence: it asserts only 'does not throw' and exitCode 0, but the PR's whole point is the JSON output status. After afterEach() resets process.exitCode to 0, the exitCode assertion is trivially true, so a regression that printed {status:'error'} (or the success shape) in the unconfirmed --json path would pass this test. Capture stdout and assert the printed JSON contains status 'unconfirmed' and the 'may have been recorded' message.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/feedback.test.ts, line 183:

<comment>This test gives false confidence: it asserts only 'does not throw' and exitCode 0, but the PR's whole point is the JSON output status. After afterEach() resets process.exitCode to 0, the exitCode assertion is trivially true, so a regression that printed {status:'error'} (or the success shape) in the unconfirmed --json path would pass this test. Capture stdout and assert the printed JSON contains status 'unconfirmed' and the 'may have been recorded' message.</comment>

<file context>
@@ -162,6 +173,16 @@ describe('feedback command', () => {
+      e.name = 'TimeoutError'
+      throw e
+    }) as unknown as typeof fetch
+    await expect(feedback({ ...valid, json: true }, { interactive: false, cliVersion: 'x', fetchImpl })).resolves.toBeUndefined()
+    expect(process.exitCode ?? 0).toBe(0)
+  })
</file context>

expect(process.exitCode ?? 0).toBe(0)
})
})

describe('redact', () => {
Expand Down
Loading