feat: add poll-content-run Trigger.dev task#109
Conversation
Polls create-content task runs every 30s (up to 30 min) and posts video results back to the content-agent callback endpoint. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a Zod payload schema, a Trigger.dev schemaTask that polls content run statuses until terminal/timeout, aggregates per-run results, derives an overall status, and POSTs a JSON callback to an external API. Changes
Sequence DiagramsequenceDiagram
participant Task as pollContentRunTask
participant Poller as pollContentRuns
participant Runs as runs.retrieve
participant API as Callback API
Task->>Poller: start polling with runIds
loop polling rounds
Poller->>Runs: retrieve(runId)
Runs-->>Poller: run (status, output) / error
alt COMPLETED
Poller-->>Task: result { runId, completed, videoUrl?, captionText? }
else TERMINAL (FAILED/CANCELED/CRASHED/...)
Poller-->>Task: result { runId, failed, error }
else RETRIEVAL ERROR
Poller-->>Poller: increment failure count (may mark failed after threshold)
end
end
Task->>Task: resolveOverallStatus(results)
Task->>API: POST /api/content-agent/callback { threadId, status, results } (x-callback-secret, 30s timeout)
API-->>Task: response (ok / non-ok)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/schemas/pollContentRunSchema.ts (1)
4-4: IndividualrunIdselements can be empty strings.The schema validates that the array has at least one element, but individual strings can be empty. Consider adding
.min(1)to each string element if empty run IDs should be rejected:Proposed fix
- runIds: z.array(z.string()).min(1), + runIds: z.array(z.string().min(1)).min(1),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/pollContentRunSchema.ts` at line 4, The runIds array currently enforces at least one element but allows empty strings; update the schema in pollContentRunSchema.ts so each element is a non-empty string by changing the element schema from z.string() to z.string().min(1) (or z.string().nonempty()), i.e., use z.array(z.string().min(1)).min(1) on the runIds field to reject empty run IDs while still requiring at least one item.src/tasks/pollContentRunTask.ts (1)
90-90: Remove unused variableallCompleted.This variable is declared but never used in the status determination logic.
Proposed fix
// Determine overall status - const allCompleted = results.every(r => r.status === "completed"); const anyCompleted = results.some(r => r.status === "completed"); const anyTimeout = results.some(r => r.status === "timeout");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/pollContentRunTask.ts` at line 90, Remove the unused local variable allCompleted declared in pollContentRunTask (the const allCompleted = results.every(r => r.status === "completed"); line) since it isn’t referenced in the status determination logic; delete that declaration and ensure any logic that determines overall status continues to rely on the existing used variables (e.g., results, anyCompleted, anyFailed) without replacing or duplicating behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tasks/pollContentRunTask.ts`:
- Around line 109-127: The fetch to callbackUrl in pollContentRunTask.ts has no
timeout; wrap the request in an AbortController and start a timer (e.g., 5–15s)
that calls controller.abort() after the timeout, pass signal to fetch, and clear
the timer after the response arrives; handle AbortError in the surrounding
try/catch so the task continues to return success (log a timeout-specific
message), and ensure the existing logger.error branch still records non-timeout
HTTP failures for the response variable.
---
Nitpick comments:
In `@src/schemas/pollContentRunSchema.ts`:
- Line 4: The runIds array currently enforces at least one element but allows
empty strings; update the schema in pollContentRunSchema.ts so each element is a
non-empty string by changing the element schema from z.string() to
z.string().min(1) (or z.string().nonempty()), i.e., use
z.array(z.string().min(1)).min(1) on the runIds field to reject empty run IDs
while still requiring at least one item.
In `@src/tasks/pollContentRunTask.ts`:
- Line 90: Remove the unused local variable allCompleted declared in
pollContentRunTask (the const allCompleted = results.every(r => r.status ===
"completed"); line) since it isn’t referenced in the status determination logic;
delete that declaration and ensure any logic that determines overall status
continues to rely on the existing used variables (e.g., results, anyCompleted,
anyFailed) without replacing or duplicating behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fba914eb-ea82-44f8-b63b-844a9cbed6f1
📒 Files selected for processing (2)
src/schemas/pollContentRunSchema.tssrc/tasks/pollContentRunTask.ts
src/tasks/pollContentRunTask.ts
Outdated
| const response = await fetch(callbackUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "x-callback-secret": callbackSecret ?? "", | ||
| }, | ||
| body: JSON.stringify({ | ||
| threadId: callbackThreadId, | ||
| status: overallStatus, | ||
| results, | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| logger.error("Callback failed", { | ||
| status: response.status, | ||
| body: await response.text().catch(() => ""), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other tasks in this repo handle callback failures differently
rg -n "fetch\(" src/tasks/ -A 10 | head -60Repository: recoupable/tasks
Length of output: 769
🏁 Script executed:
cat -n src/tasks/pollContentRunTask.ts | head -150Repository: recoupable/tasks
Length of output: 4937
🏁 Script executed:
# Check for fetch timeout patterns across the entire codebase
rg "AbortSignal|timeout\(" --type ts --type tsx -i | head -40Repository: recoupable/tasks
Length of output: 87
🏁 Script executed:
# Check for other fetch calls in the codebase (not just src/tasks/)
rg -n "fetch\(" -A 3 | head -80Repository: recoupable/tasks
Length of output: 4327
Add timeout to the callback fetch request.
The fetch call (line 109) has no timeout configured. Although the task's maxDuration of 35 minutes provides an ultimate cap, an unresponsive callback endpoint could block the task until that limit is reached. Adding a timeout is defensive practice:
const response = await fetch(callbackUrl, {
method: "POST",
headers: {
"Content-Type": "application/json",
"x-callback-secret": callbackSecret ?? "",
},
body: JSON.stringify({
threadId: callbackThreadId,
status: overallStatus,
results,
}),
+ signal: AbortSignal.timeout(30_000), // 30 second timeout
});The callback is fire-and-forget by design (task returns success regardless of callback result), so this is a non-critical improvement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(callbackUrl, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| "x-callback-secret": callbackSecret ?? "", | |
| }, | |
| body: JSON.stringify({ | |
| threadId: callbackThreadId, | |
| status: overallStatus, | |
| results, | |
| }), | |
| }); | |
| if (!response.ok) { | |
| logger.error("Callback failed", { | |
| status: response.status, | |
| body: await response.text().catch(() => ""), | |
| }); | |
| } | |
| const response = await fetch(callbackUrl, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| "x-callback-secret": callbackSecret ?? "", | |
| }, | |
| body: JSON.stringify({ | |
| threadId: callbackThreadId, | |
| status: overallStatus, | |
| results, | |
| }), | |
| signal: AbortSignal.timeout(30_000), // 30 second timeout | |
| }); | |
| if (!response.ok) { | |
| logger.error("Callback failed", { | |
| status: response.status, | |
| body: await response.text().catch(() => ""), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/pollContentRunTask.ts` around lines 109 - 127, The fetch to
callbackUrl in pollContentRunTask.ts has no timeout; wrap the request in an
AbortController and start a timer (e.g., 5–15s) that calls controller.abort()
after the timeout, pass signal to fetch, and clear the timer after the response
arrives; handle AbortError in the surrounding try/catch so the task continues to
return success (log a timeout-specific message), and ensure the existing
logger.error branch still records non-timeout HTTP failures for the response
variable.
recoup-coding-agent
left a comment
recoup-coding-agent
left a comment
There was a problem hiding this comment.
Summary
Adds poll-content-run Trigger.dev task that polls multiple create-content runs until all finish, then POSTs results to the content-agent callback endpoint. Companion to recoupable/api#341.
CLEAN Code Assessment
- SRP: Good — single responsibility, one task does one thing.
- DRY: Good — reuses
pollContentRunPayloadSchemafrom a shared schema file. - YAGNI: Minor issue (see below).
- Naming: Clear and consistent with the existing task naming convention.
Issues Found
blocking
Unused variable allCompleted (src/tasks/pollContentRunTask.ts line 96):
const allCompleted = results.every(r => r.status === "completed");allCompleted is computed but never referenced in the status-determination logic. The current logic uses anyCompleted and anyTimeout only. Either remove it or use it intentionally (e.g. distinguish all-completed from partial-completed).
suggestion
Hardcoded callback URL construction (line 103):
const callbackUrl = `${process.env.RECOUP_API_BASE_URL}/api/content-agent/callback`;The path suffix /api/content-agent/callback is a magic string. If the API route ever moves, this silently breaks. Consider extracting as a named constant CONTENT_AGENT_CALLBACK_PATH or reading it from an env var.
callbackSecret ?? "" silently sends empty secret (line 108):
If CONTENT_AGENT_CALLBACK_SECRET is unset, this will POST with an empty header and the API will return 401 — a non-obvious failure mode. Better to throw early if the env var is missing, consistent with validateContentAgentEnv() in the api repo.
nit
MAX_ATTEMPTS = 60andPOLL_INTERVAL_SECONDS = 30— the comment// 30 min totalis helpful, but adding a computed constantMAX_DURATION_MINUTES = (MAX_ATTEMPTS * POLL_INTERVAL_SECONDS) / 60would make it self-documenting.Array.from(pendingRunIds)inside the while loop re-allocates on every iteration. Not a real perf issue at these scales, but[...pendingRunIds]is idiomatic.
Security
No secrets hardcoded. Callback secret read from env var — correct pattern.
Verdict
request-changes — the unused allCompleted variable should be cleaned up before merge. The silent-empty-secret issue is also worth addressing for reliability.
Code Review —
|
| Check | Status |
|---|---|
test |
✅ success |
| CodeRabbit | ✅ success (review), ⏳ pending (status) |
Branch Status
✅ Branch is up to date with main (mergeable_state: clean)
CLEAN Code Assessment
- SRP: ✅ Schema and task are properly separated into distinct files
- DRY: ✅ No duplication
- YAGNI: ✅ Focused implementation, no over-engineering
- OCP: ✅ Result types are extensible
Issues Found
suggestion — Duplicate import from @trigger.dev/sdk/v3
Lines 1 and 3 of pollContentRunTask.ts both import from @trigger.dev/sdk/v3. Merge into a single import:
import { logger, schemaTask, wait, runs } from "@trigger.dev/sdk/v3";suggestion — allCompleted is computed but never used
allCompleted (line 87) is dead code. More importantly, the current overall status logic reports "completed" if any run completed, even if others failed. Consider whether partial failure should yield a distinct status (e.g., "partial") or use allCompleted to distinguish full vs partial success.
suggestion — Missing env var guard
If RECOUP_API_BASE_URL is unset, the callback URL becomes undefined/api/content-agent/callback. Consider failing early:
if (!process.env.RECOUP_API_BASE_URL || !process.env.CONTENT_AGENT_CALLBACK_SECRET) {
throw new Error("Missing required env vars");
}nit — Silent retrieval failures lead to timeout
If runs.retrieve() consistently throws for a given runId, the catch block only logs — the run stays in pendingRunIds until max attempts, then gets reported as "timeout" rather than the actual error.
Security
✅ No hardcoded secrets — callback secret sourced from env var
✅ Proper header-based auth for callback
x-callback-secret if env var is missing (covered by suggestion above)
Verdict: approve ✅
Code is clean, focused, and well-structured. The suggestions above are non-blocking quality improvements that can be addressed in a follow-up.
The API was updated to use CODING_AGENT_CALLBACK_SECRET instead of CONTENT_AGENT_CALLBACK_SECRET so the same env var is shared. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reuses the existing shared constant from consts.ts instead of introducing a new RECOUP_API_BASE_URL env var. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
src/tasks/pollContentRunTask.ts
Outdated
| run: async payload => { | ||
| const { runIds, callbackThreadId } = payload; | ||
|
|
||
| logger.info("Starting poll-content-run", { runIds, callbackThreadId }); |
There was a problem hiding this comment.
Replace logger with shared logStep function.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tasks/pollContentRunTask.ts`:
- Around line 5-6: The polling currently uses MAX_ATTEMPTS and
POLL_INTERVAL_SECONDS which only bound iterations and can exceed the intended
max duration because runs.retrieve() adds variable latency; change to a
time-based timeout by adding a MAX_DURATION_SECONDS (or maxDurationSeconds) and
capture a start timestamp at the top of pollContentRunTask, then in the polling
loop compute elapsed = now - start and break when elapsed >= maxDurationSeconds
(instead of checking MAX_ATTEMPTS). Update logic that previously relied on
MAX_ATTEMPTS (and any per-iteration post-loop handling) to mark remaining runs
as timed out and send the callback when the elapsed timeout is reached; keep
using POLL_INTERVAL_SECONDS for sleep between iterations but ensure the final
sleep is bounded by remaining time (maxDurationSeconds - elapsed) so the task
never waits past the timeout. Ensure references updated around
POLL_INTERVAL_SECONDS, MAX_ATTEMPTS, runs.retrieve(), and the main
pollContentRunTask loop.
- Around line 74-76: The catch currently logs retrieval errors for a runId but
leaves runId in pendingRunIds so transient or permanent retrieval failures are
later misreported as "timeout"; add a retrieval failure counter (e.g., a map
retrievalFailureCounts keyed by runId and a constant RETRIEVAL_FAILURE_THRESHOLD
like 3), increment it in the catch that surrounds logger.error("Error retrieving
run", { runId, error }), and when the counter reaches the threshold remove the
runId from pendingRunIds and immediately mark/emit the run as failed (invoke the
same failure path you use elsewhere — e.g., call the existing failure handler or
emit status "failed" for that runId) while logging the final failure; reset the
counter to 0 on successful retrievals so only consecutive retrieval errors
trigger the forced failure.
- Around line 89-100: The current logic in pollContentRunTask.ts collapses mixed
outcomes to "completed" because overallStatus is set to "completed" whenever any
result has status "completed"; change the decision to use the allCompleted
boolean: keep the timeout precedence (check anyTimeout first), then set
overallStatus = "completed" only when allCompleted is true, otherwise set
overallStatus = "failed" when not allCompleted (and not timed out). Update the
block that assigns overallStatus (referencing results, anyTimeout, anyCompleted,
allCompleted, and overallStatus) so a mixed {completed, failed} array does not
report as fully "completed".
- Around line 104-127: The callback currently can be sent with an empty
x-callback-secret and non-2xx responses are only logged (so pollContentRunTask
treats failure as success); update the callback logic in pollContentRunTask to
(1) validate that callbackSecret (process.env.CODING_AGENT_CALLBACK_SECRET)
exists and if not, log an error and throw or return a failure immediately, (2)
wrap the fetch to catch network exceptions and treat them as failures, and (3)
on non-ok responses from the fetch (response.ok === false) throw or return a
failure instead of only logging—use the existing logger for context
(callbackUrl, status, body) and ensure the task bubbles the error so the caller
sees the failed handoff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6dd3fc7-8ae7-4054-b369-f6854da5b1ac
📒 Files selected for processing (1)
src/tasks/pollContentRunTask.ts
src/tasks/pollContentRunTask.ts
Outdated
| const POLL_INTERVAL_SECONDS = 30; | ||
| const MAX_ATTEMPTS = 60; // 30 min total |
There was a problem hiding this comment.
The poll timeout is attempt-based, not time-based.
MAX_ATTEMPTS only bounds loop iterations. The sequential runs.retrieve() calls add extra time on every pass, so a larger batch or slow Trigger.dev API can hit maxDuration before the task marks remaining runs as timed out or sends the callback. That breaks the advertised 30-minute cap in this PR.
Also applies to: 23-25, 36-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/pollContentRunTask.ts` around lines 5 - 6, The polling currently
uses MAX_ATTEMPTS and POLL_INTERVAL_SECONDS which only bound iterations and can
exceed the intended max duration because runs.retrieve() adds variable latency;
change to a time-based timeout by adding a MAX_DURATION_SECONDS (or
maxDurationSeconds) and capture a start timestamp at the top of
pollContentRunTask, then in the polling loop compute elapsed = now - start and
break when elapsed >= maxDurationSeconds (instead of checking MAX_ATTEMPTS).
Update logic that previously relied on MAX_ATTEMPTS (and any per-iteration
post-loop handling) to mark remaining runs as timed out and send the callback
when the elapsed timeout is reached; keep using POLL_INTERVAL_SECONDS for sleep
between iterations but ensure the final sleep is bounded by remaining time
(maxDurationSeconds - elapsed) so the task never waits past the timeout. Ensure
references updated around POLL_INTERVAL_SECONDS, MAX_ATTEMPTS, runs.retrieve(),
and the main pollContentRunTask loop.
src/tasks/pollContentRunTask.ts
Outdated
| } catch (error) { | ||
| logger.error("Error retrieving run", { runId, error }); | ||
| } |
There was a problem hiding this comment.
Persistent retrieve errors get misreported as timeout.
This catch block leaves the runId in pendingRunIds, so a bad ID or repeated API failure sits here for the full poll window and is later emitted as status: "timeout". That delays failure handling and gives the callback the wrong reason. Consider tracking consecutive retrieval failures per runId and converting them to failed after a small threshold.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/pollContentRunTask.ts` around lines 74 - 76, The catch currently
logs retrieval errors for a runId but leaves runId in pendingRunIds so transient
or permanent retrieval failures are later misreported as "timeout"; add a
retrieval failure counter (e.g., a map retrievalFailureCounts keyed by runId and
a constant RETRIEVAL_FAILURE_THRESHOLD like 3), increment it in the catch that
surrounds logger.error("Error retrieving run", { runId, error }), and when the
counter reaches the threshold remove the runId from pendingRunIds and
immediately mark/emit the run as failed (invoke the same failure path you use
elsewhere — e.g., call the existing failure handler or emit status "failed" for
that runId) while logging the final failure; reset the counter to 0 on
successful retrievals so only consecutive retrieval errors trigger the forced
failure.
src/tasks/pollContentRunTask.ts
Outdated
| const callbackUrl = `${NEW_API_BASE_URL}/api/content-agent/callback`; | ||
| const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET; | ||
|
|
||
| logger.info("Calling callback", { callbackUrl, overallStatus, resultCount: results.length }); | ||
|
|
||
| const response = await fetch(callbackUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "x-callback-secret": callbackSecret ?? "", | ||
| }, | ||
| body: JSON.stringify({ | ||
| threadId: callbackThreadId, | ||
| status: overallStatus, | ||
| results, | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| logger.error("Callback failed", { | ||
| status: response.status, | ||
| body: await response.text().catch(() => ""), | ||
| }); | ||
| } |
There was a problem hiding this comment.
A failed callback is currently reported as success.
If CODING_AGENT_CALLBACK_SECRET is missing, this code still sends an empty auth header. And when the API returns 4xx/5xx, the task only logs the response and still returns success. Since retry.maxAttempts is 0, that can permanently drop the only handoff back to the API/Slack flow.
🔒 Suggested shape
// Call callback endpoint
const callbackUrl = `${NEW_API_BASE_URL}/api/content-agent/callback`;
const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET;
+
+ if (!callbackSecret) {
+ throw new Error("CODING_AGENT_CALLBACK_SECRET is required");
+ }
logger.info("Calling callback", { callbackUrl, overallStatus, resultCount: results.length });
const response = await fetch(callbackUrl, {
method: "POST",
headers: {
"Content-Type": "application/json",
- "x-callback-secret": callbackSecret ?? "",
+ "x-callback-secret": callbackSecret,
},
body: JSON.stringify({
threadId: callbackThreadId,
status: overallStatus,
results,
}),
});
if (!response.ok) {
- logger.error("Callback failed", {
- status: response.status,
- body: await response.text().catch(() => ""),
- });
+ const body = await response.text().catch(() => "");
+ logger.error("Callback failed", { status: response.status, body });
+ throw new Error(`Callback failed with status ${response.status}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const callbackUrl = `${NEW_API_BASE_URL}/api/content-agent/callback`; | |
| const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET; | |
| logger.info("Calling callback", { callbackUrl, overallStatus, resultCount: results.length }); | |
| const response = await fetch(callbackUrl, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| "x-callback-secret": callbackSecret ?? "", | |
| }, | |
| body: JSON.stringify({ | |
| threadId: callbackThreadId, | |
| status: overallStatus, | |
| results, | |
| }), | |
| }); | |
| if (!response.ok) { | |
| logger.error("Callback failed", { | |
| status: response.status, | |
| body: await response.text().catch(() => ""), | |
| }); | |
| } | |
| const callbackUrl = `${NEW_API_BASE_URL}/api/content-agent/callback`; | |
| const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET; | |
| if (!callbackSecret) { | |
| throw new Error("CODING_AGENT_CALLBACK_SECRET is required"); | |
| } | |
| logger.info("Calling callback", { callbackUrl, overallStatus, resultCount: results.length }); | |
| const response = await fetch(callbackUrl, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| "x-callback-secret": callbackSecret, | |
| }, | |
| body: JSON.stringify({ | |
| threadId: callbackThreadId, | |
| status: overallStatus, | |
| results, | |
| }), | |
| }); | |
| if (!response.ok) { | |
| const body = await response.text().catch(() => ""); | |
| logger.error("Callback failed", { status: response.status, body }); | |
| throw new Error(`Callback failed with status ${response.status}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/pollContentRunTask.ts` around lines 104 - 127, The callback
currently can be sent with an empty x-callback-secret and non-2xx responses are
only logged (so pollContentRunTask treats failure as success); update the
callback logic in pollContentRunTask to (1) validate that callbackSecret
(process.env.CODING_AGENT_CALLBACK_SECRET) exists and if not, log an error and
throw or return a failure immediately, (2) wrap the fetch to catch network
exceptions and treat them as failures, and (3) on non-ok responses from the
fetch (response.ok === false) throw or return a failure instead of only
logging—use the existing logger for context (callbackUrl, status, body) and
ensure the task bubbles the error so the caller sees the failed handoff.
- SRP: Extract polling logic into content/pollContentRuns.ts and callback logic into content/sendContentCallback.ts - DRY: Replace raw logger calls with shared logStep utility - Fix overall status: use allCompleted instead of anyCompleted to prevent mixed outcomes from being reported as fully completed - Add AbortSignal.timeout(30s) to callback fetch request - Add env var guard: throw if CODING_AGENT_CALLBACK_SECRET is missing - Throw on callback failure instead of silently succeeding - Track consecutive retrieval failures per runId (threshold: 3) to avoid misreporting persistent API errors as timeout - Remove dead anyCompleted variable and duplicate import Co-Authored-By: Paperclip <noreply@paperclip.ing>
PR Feedback FixesPushed SRP — File decomposed into sub-libs
Fixes Applied
|
src/content/pollContentRuns.ts
Outdated
| const retrievalFailures = new Map<string, number>(); | ||
| let attempts = 0; | ||
|
|
||
| while (pendingRunIds.size > 0 && attempts < MAX_ATTEMPTS) { |
There was a problem hiding this comment.
KISS principle. Remove the max attempts check. only check if there are pending runs.
src/content/sendContentCallback.ts
Outdated
| /** | ||
| * Determines overall status from individual run results. | ||
| */ | ||
| export function resolveOverallStatus( |
There was a problem hiding this comment.
SRP - new file for resolveOverallStatus.
…tCallback TDD compliance: adding tests that cover the content polling and callback modules extracted in the prior refactor commit. - pollContentRuns: 7 tests covering completion, failure statuses, polling, retrieval error threshold, null output - resolveOverallStatus: 6 tests covering all status combinations - sendContentCallback: 3 tests covering env validation, payload, error handling Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/content/sendContentCallback.ts (1)
8-17: SRP violation: two exported functions in a single file.This file exports both
resolveOverallStatusandsendContentCallback. As per coding guidelines, files should implement SRP with one exported function per file. Consider extractingresolveOverallStatusto its own module (e.g.,src/content/resolveOverallStatus.ts).Also applies to: 23-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/sendContentCallback.ts` around lines 8 - 17, Extract the resolveOverallStatus function into its own module (e.g., resolveOverallStatus.ts) so the file containing sendContentCallback exports only that one function; move the implementation of resolveOverallStatus out, export it from the new module (named export), update sendContentCallback to import { resolveOverallStatus } from the new module, and update any tests or other files that reference resolveOverallStatus to import from the new module as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/content/sendContentCallback.ts`:
- Around line 28-31: The code reads the wrong environment variable name: change
the lookup of process.env.CODING_AGENT_CALLBACK_SECRET to
process.env.CONTENT_AGENT_CALLBACK_SECRET and update the thrown error message
accordingly (the const callbackSecret in sendContentCallback.ts should use
CONTENT_AGENT_CALLBACK_SECRET and the Error text should match) so the runtime
expects the PR-specified env var.
---
Nitpick comments:
In `@src/content/sendContentCallback.ts`:
- Around line 8-17: Extract the resolveOverallStatus function into its own
module (e.g., resolveOverallStatus.ts) so the file containing
sendContentCallback exports only that one function; move the implementation of
resolveOverallStatus out, export it from the new module (named export), update
sendContentCallback to import { resolveOverallStatus } from the new module, and
update any tests or other files that reference resolveOverallStatus to import
from the new module as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87c80848-1517-4651-bd9c-618516f8b88e
📒 Files selected for processing (3)
src/content/pollContentRuns.tssrc/content/sendContentCallback.tssrc/tasks/pollContentRunTask.ts
| const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET; | ||
| if (!callbackSecret) { | ||
| throw new Error("CODING_AGENT_CALLBACK_SECRET is required"); | ||
| } |
There was a problem hiding this comment.
Environment variable name mismatch with PR requirements.
The code reads CODING_AGENT_CALLBACK_SECRET, but the PR objectives specify CONTENT_AGENT_CALLBACK_SECRET as the required environment variable. This naming inconsistency will cause the deployment to fail if the env var is set according to the PR documentation.
🐛 Proposed fix
- const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET;
+ const callbackSecret = process.env.CONTENT_AGENT_CALLBACK_SECRET;
if (!callbackSecret) {
- throw new Error("CODING_AGENT_CALLBACK_SECRET is required");
+ throw new Error("CONTENT_AGENT_CALLBACK_SECRET is required");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const callbackSecret = process.env.CODING_AGENT_CALLBACK_SECRET; | |
| if (!callbackSecret) { | |
| throw new Error("CODING_AGENT_CALLBACK_SECRET is required"); | |
| } | |
| const callbackSecret = process.env.CONTENT_AGENT_CALLBACK_SECRET; | |
| if (!callbackSecret) { | |
| throw new Error("CONTENT_AGENT_CALLBACK_SECRET is required"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/sendContentCallback.ts` around lines 28 - 31, The code reads the
wrong environment variable name: change the lookup of
process.env.CODING_AGENT_CALLBACK_SECRET to
process.env.CONTENT_AGENT_CALLBACK_SECRET and update the thrown error message
accordingly (the const callbackSecret in sendContentCallback.ts should use
CONTENT_AGENT_CALLBACK_SECRET and the Error text should match) so the runtime
expects the PR-specified env var.
Re-Review — PR #109 (poll-content-run)SummaryGood progress — the decomposition into CI Status
Branch Status✅ Branch is up to date with CLEAN Code Assessment
Issues FoundBlocking:
Suggestion: Security
Verdict:
|
recoup-coding-agent
left a comment
recoup-coding-agent
left a comment
There was a problem hiding this comment.
Re-review posted — see comment. Verdict: request-changes (2 blocking items from owner feedback).
- KISS: Remove MAX_ATTEMPTS from pollContentRuns — task maxDuration already bounds execution, so the internal attempt cap was redundant - SRP: Extract resolveOverallStatus to its own file - Update imports in pollContentRunTask and test Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/content/__tests__/sendContentCallback.test.ts (1)
31-45: Avoid hardcoding the production callback URL in test assertions.Line 44 hardcodes a specific domain, which makes the test brittle and does not validate env-driven base URL behavior. Prefer setting
RECOUP_API_BASE_URLin test setup and asserting against that value.♻️ Proposed test adjustment
beforeEach(() => { - process.env = { ...originalEnv, CODING_AGENT_CALLBACK_SECRET: "test-secret" }; + process.env = { + ...originalEnv, + CODING_AGENT_CALLBACK_SECRET: "test-secret", + RECOUP_API_BASE_URL: "https://api.test.local", + }; vi.clearAllMocks(); }); @@ - expect(url).toBe("https://recoup-api.vercel.app/api/content-agent/callback"); + expect(url).toBe("https://api.test.local/api/content-agent/callback");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/__tests__/sendContentCallback.test.ts` around lines 31 - 45, The test currently hardcodes the production callback URL; update it to set process.env.RECOUP_API_BASE_URL (or the env var your code reads) in the test setup and assert that sendContentCallback constructs the URL from that base. Specifically, in src/content/__tests__/sendContentCallback.test.ts, before calling sendContentCallback, assign a test base (e.g., "https://test-base/") to RECOUP_API_BASE_URL, call sendContentCallback("thread-1", "completed", results), then assert that the fetch call used `${process.env.RECOUP_API_BASE_URL}/api/content-agent/callback` (referencing sendContentCallback) instead of the hardcoded domain; restore or clear the env var after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/content/__tests__/pollContentRuns.test.ts`:
- Around line 93-109: The test "resets retrieval failure count on successful
retrieve" currently only throws two retrieval errors so a broken cumulative
counter could still pass; update the mockRetrieve implementation used in this
test (mockRetrieve and its callCount logic inside the test for pollContentRuns)
to throw a third non-consecutive error before returning the final COMPLETED
result (e.g., add another conditional like if (callCount === 5) throw new
Error("Network error") and shift the final return accordingly) so that the test
fails unless the failure counter is actually reset between successful retrieves.
---
Nitpick comments:
In `@src/content/__tests__/sendContentCallback.test.ts`:
- Around line 31-45: The test currently hardcodes the production callback URL;
update it to set process.env.RECOUP_API_BASE_URL (or the env var your code
reads) in the test setup and assert that sendContentCallback constructs the URL
from that base. Specifically, in
src/content/__tests__/sendContentCallback.test.ts, before calling
sendContentCallback, assign a test base (e.g., "https://test-base/") to
RECOUP_API_BASE_URL, call sendContentCallback("thread-1", "completed", results),
then assert that the fetch call used
`${process.env.RECOUP_API_BASE_URL}/api/content-agent/callback` (referencing
sendContentCallback) instead of the hardcoded domain; restore or clear the env
var after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 722b2dd3-797e-42f0-b233-27618c021a43
📒 Files selected for processing (3)
src/content/__tests__/pollContentRuns.test.tssrc/content/__tests__/resolveOverallStatus.test.tssrc/content/__tests__/sendContentCallback.test.ts
| it("resets retrieval failure count on successful retrieve", async () => { | ||
| let callCount = 0; | ||
| mockRetrieve.mockImplementation(async () => { | ||
| callCount++; | ||
| if (callCount === 1) throw new Error("Network error"); | ||
| if (callCount === 2) return { status: "EXECUTING" } as any; | ||
| if (callCount === 3) throw new Error("Network error"); | ||
| if (callCount === 4) return { status: "EXECUTING" } as any; | ||
| return { status: "COMPLETED", output: null } as any; | ||
| }); | ||
|
|
||
| const results = await pollContentRuns(["run-1"]); | ||
|
|
||
| expect(results).toEqual([ | ||
| { runId: "run-1", status: "completed", videoUrl: undefined, captionText: undefined }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
“Reset failure count” test can pass even if reset logic is broken.
At Line 97 and Line 99, the test only throws two retrieval errors total. A buggy cumulative counter (never reset) would still pass. Add a third non-consecutive error before completion so the test fails without reset logic.
✅ Suggested test hardening
it("resets retrieval failure count on successful retrieve", async () => {
let callCount = 0;
mockRetrieve.mockImplementation(async () => {
callCount++;
if (callCount === 1) throw new Error("Network error");
if (callCount === 2) return { status: "EXECUTING" } as any;
if (callCount === 3) throw new Error("Network error");
if (callCount === 4) return { status: "EXECUTING" } as any;
- return { status: "COMPLETED", output: null } as any;
+ if (callCount === 5) throw new Error("Network error");
+ if (callCount === 6) return { status: "EXECUTING" } as any;
+ return { status: "COMPLETED", output: null } as any;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("resets retrieval failure count on successful retrieve", async () => { | |
| let callCount = 0; | |
| mockRetrieve.mockImplementation(async () => { | |
| callCount++; | |
| if (callCount === 1) throw new Error("Network error"); | |
| if (callCount === 2) return { status: "EXECUTING" } as any; | |
| if (callCount === 3) throw new Error("Network error"); | |
| if (callCount === 4) return { status: "EXECUTING" } as any; | |
| return { status: "COMPLETED", output: null } as any; | |
| }); | |
| const results = await pollContentRuns(["run-1"]); | |
| expect(results).toEqual([ | |
| { runId: "run-1", status: "completed", videoUrl: undefined, captionText: undefined }, | |
| ]); | |
| }); | |
| it("resets retrieval failure count on successful retrieve", async () => { | |
| let callCount = 0; | |
| mockRetrieve.mockImplementation(async () => { | |
| callCount++; | |
| if (callCount === 1) throw new Error("Network error"); | |
| if (callCount === 2) return { status: "EXECUTING" } as any; | |
| if (callCount === 3) throw new Error("Network error"); | |
| if (callCount === 4) return { status: "EXECUTING" } as any; | |
| if (callCount === 5) throw new Error("Network error"); | |
| if (callCount === 6) return { status: "EXECUTING" } as any; | |
| return { status: "COMPLETED", output: null } as any; | |
| }); | |
| const results = await pollContentRuns(["run-1"]); | |
| expect(results).toEqual([ | |
| { runId: "run-1", status: "completed", videoUrl: undefined, captionText: undefined }, | |
| ]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/__tests__/pollContentRuns.test.ts` around lines 93 - 109, The
test "resets retrieval failure count on successful retrieve" currently only
throws two retrieval errors so a broken cumulative counter could still pass;
update the mockRetrieve implementation used in this test (mockRetrieve and its
callCount logic inside the test for pollContentRuns) to throw a third
non-consecutive error before returning the final COMPLETED result (e.g., add
another conditional like if (callCount === 5) throw new Error("Network error")
and shift the final return accordingly) so that the test fails unless the
failure counter is actually reset between successful retrieves.
recoup-coding-agent
left a comment
recoup-coding-agent
left a comment
There was a problem hiding this comment.
Re-Review — PR #109 (poll-content-run) ✅
Summary
Both blocking items from previous review are resolved. Code is clean, well-structured, and follows CLEAN principles.
CI Status
| Check | Status |
|---|---|
test |
✅ success (181 tests, 38 files) |
Branch Status
✅ Branch is up to date with main (mergeable_state: clean)
CLEAN Code Assessment
- SRP: ✅ Clean separation —
pollContentRuns.ts(polling),resolveOverallStatus.ts(status logic),sendContentCallback.ts(callback),pollContentRunTask.ts(orchestrator ~31 lines) - DRY: ✅ Uses shared
logStepthroughout - KISS: ✅
MAX_ATTEMPTSremoved — loop bounded by taskmaxDurationonly - YAGNI: ✅ No unnecessary abstractions
Issues Found
None blocking. Previous suggestion about env var naming (CODING_AGENT_CALLBACK_SECRET vs CONTENT_AGENT_CALLBACK_SECRET) remains a non-blocking nit — verify alignment with deployment config.
Security
- ✅ No hardcoded secrets
- ✅ Env var guard on callback secret
- ✅ Fetch timeout (30s) via
AbortSignal.timeout - ✅ Zod schema validation on task input
Verdict: approve
Ready to merge.
There was a problem hiding this comment.
Why are we polling the run status in a loop instead of using the native wait function?
https://trigger.dev/docs/runs#waiting-for-runs
Replace manual runs.retrieve() + wait.for() loop with Trigger.dev's native runs.poll() function. Runs are now polled concurrently via Promise.allSettled, simplifying the code and removing manual retry tracking. All 181 tests passing. Co-Authored-By: Paperclip <noreply@paperclip.ing> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactored: Native
|
2 participants
Summary
poll-content-runTrigger.dev task that monitorscreate-contentrunsruns.retrieve()POST /api/content-agent/callbackwith video resultsNew Env Vars Required
RECOUP_API_BASE_URLCONTENT_AGENT_CALLBACK_SECRETTest plan
Summary by CodeRabbit
New Features
Tests