fix: feedback timeout 15s + honest UNCONFIRMED on expired deadline

[Fermionic-Lyu]

Follow-up to the async-vs-sync design discussion: submission stays synchronous (the result — received/duplicate/id — is worth one tool call; agents that don't want to wait already have &), with two changes:

1. Timeout 10s → 15s. The ingest backend now waits out its scale-to-zero DB cold start and persists reports arriving mid-wake, so the slow path returns a real result within 15s instead of tripping the old deadline.
2. An expired deadline is unconfirmed, not error. The server finishes in-flight requests after a client abort, so "not submitted … do not retry" was a false negative for a report that WAS stored. Now: warning: feedback receipt unconfirmed (no response after 15s — the report may have been recorded anyway) — continue with your task, do not retry / --json {status:'unconfirmed'}. Hard transport failures (ECONNREFUSED etc.) keep the explicit not submitted wording. Exit 0 either way, validation errors still exit 1.

Companion: insta-mcp same semantics.

🤖 Generated with Claude Code

---

Summary by cubic

Extends feedback submit timeout to 15s and treats a deadline expiry as unconfirmed instead of error. This prevents false “not submitted” when the ingest service finishes in-flight requests after the client deadline.

• Old: 10s timeout; deadline → error. New: 15s timeout; deadline → status 'unconfirmed' with a warning; exit code stays 0. Validation errors still exit 1.
• JSON may return {status: 'unconfirmed', error: '...'}; CLI prints "warning: feedback receipt unconfirmed … do not retry".
• Hard transport failures remain status 'error' with "not submitted" wording; no retries added.
• Tests cover 'unconfirmed' for submit and command flows.

Migration: If you parse feedback results, handle status 'unconfirmed' distinctly from 'error' and do not retry.

^{Written for commit 35200f6. Summary will update on new commits.}

[jwfing]

Review — fix: feedback timeout 30s + honest UNCONFIRMED on expired deadline

Summary: A small, well-reasoned change that raises the feedback POST timeout to 30s and reclassifies a deadline-expiry as unconfirmed (exit 0) rather than a false error/"not submitted" — correct and cleanly scoped.

Requirements context: No matching spec/plan found — the repo has no docs/superpowers/, docs/specs/, or any feedback design doc. Assessed against the PR description and the surrounding code alone.

I verified the one thing that could have silently defeated the whole change: in Node 20, a real fetch timeout via AbortSignal.timeout rejects with a DOMException whose name === 'TimeoutError' and which is instanceof Error, so the guard at src/commands/feedback.ts:217 fires for genuine production timeouts — not just the plain-Error mocks in the tests. The feature works end-to-end.

Critical

(none)

Suggestion

• Software engineering — stale doc comment. src/commands/feedback.ts:201 still opens with "One POST, 10s timeout, zero retries". The constant is now 30_000 (feedback.ts:47). Update the JSDoc to 30s so the contract comment matches the code.
• Software engineering — test asserts behavior but not the emitted shape. The new command-level test (test/feedback.test.ts:177-185) asserts no-throw + exit 0, which is the important safety property, but nothing asserts that --json actually emits { status: 'unconfirmed', error: … } on stdout. Given the PR's migration note tells consumers to branch on status: 'unconfirmed', a small assertion capturing printJson output (as other flows here don't currently, but this is the one whose contract external parsers depend on) would lock the wire shape. Low blast radius; the existing error path has the same gap.

Information

• Functionality — JSON schema asymmetry. The unconfirmed JSON output is { status, error } (feedback.ts:254), whereas both error paths emit { status, submitted: false, error } (feedback.ts:244, feedback.ts:261) and success emits { status, id }. Omitting submitted for unconfirmed is actually defensible — the submission state is genuinely unknown, so submitted: false would be its own false negative — but consumers should be aware the field is simply absent (not true/false) in this case. Matches the PR's stated migration guidance.
• Performance — larger foreground worst case. The timeout tripling means the CLI foreground can now block up to 30s on a slow/black-holed backend (single POST, no retries). This is the documented, deliberate tradeoff for the scale-to-zero cold start, and agents can background with &; noting it only for visibility, not as a concern.
• Security — no security-relevant changes. No new user input reaches SQL/shell/HTTP; no secrets/PII newly logged; the unconfirmed message is a static client-side string (not server-echoed content); no auth changes; no new dependencies.

Verdict

approved (informational — human approval is still a separate action). Zero Critical findings; the Suggestions and Information notes are non-blocking.

[cubic-dev-ai]

2 issues found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="test/feedback.test.ts">

<violation number="1" location="test/feedback.test.ts:183">
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.</violation>
</file>

<file name="src/commands/feedback.ts">

<violation number="1" location="src/commands/feedback.ts:217">
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.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

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>

[cubic-dev-ai]

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>

[jwfing]

LGTM - approved.