Skip to content

Orchestrator hang: fresh-task retry + richer error diagnostics #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stevegaossou merged 3 commits into from
Jun 24, 2026
Merged

Orchestrator hang: fresh-task retry + richer error diagnostics #244

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
581c266
Orchestrator: resubmit a fresh task once if it hangs
stevegaossou Jun 24, 2026
ad06320
Richer error diagnostics for agent calls
stevegaossou Jun 24, 2026
b28e9a3
Address Council review on #244
stevegaossou Jun 24, 2026
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
93 changes: 74 additions & 19 deletions .github/workflows/scripts/council_of_claudes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ const sleep = ms => new Promise(res => setTimeout(res, ms));
// ::workflow-command:: strings into the Actions log.
const safe = s => String(s ?? '').replace(/::/g, ':​:');

// Node's fetch (undici) surfaces generic messages like "fetch failed" /
// "terminated" and puts the actual reason (ECONNRESET, UND_ERR_*, etc.) on
// e.cause. Surface both so error logs are diagnosable.
function errDetail(e) {

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — errDetail is useful for surfacing undici’s cause. For future debugging, consider optionally including e.stack when an env var (e.g., DEBUG_ERRORS=1) is set; this keeps default logs terse while making it easy to toggle deeper diagnostics when investigating intermittent failures:

function errDetail(e) {
  const c = e?.cause;
  const cause = c ? ` | cause: ${c.code || c.message || String(c)}` : '';
  const stack = process.env.DEBUG_ERRORS ? ` | stack: ${e?.stack || ''}` : '';
  return `${e?.message || e}${cause}${stack}`;
}

const c = e?.cause;
const cause = c ? ` | cause: ${c.code || c.message || c}` : '';
return `${e?.message || e}${cause}`;
}

// Strip a single markdown fence that wraps the *entire* response, while
// leaving any inner code/suggestion fences intact.
function stripOuterFence(text) {
Expand Down Expand Up @@ -161,7 +170,13 @@ async function rpc(url, token, body) {
body: JSON.stringify(body),

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛡️ Security — The fetch call sends an Authorization: Bearer token to whatever URL is configured. If ORCHESTRATOR_AGENT_URL (or persona URLs) are accidentally set to http://, the bearer token will traverse the network in plaintext, risking credential exposure or interception (especially in shared CI runners or through proxies).
Mitigation:

  • Enforce HTTPS before issuing requests. For example:
    • Validate at the call site (or in rpc): if (!/^https:///i.test(url)) { core.setFailed('Refusing to send token to non-HTTPS URL'); return; }
    • Optionally allow http only when an explicit “I know what I’m doing” flag is set in a trusted CI environment without tokens.

signal: AbortSignal.timeout(RPC_TIMEOUT_MS),
});
if (!r.ok) throw new Error(`http ${r.status}`);
if (!r.ok) {
// Include statusText and a short body snippet — a 5xx from the gateway often

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛡️ Security — Including a snippet of the HTTP response body in error messages improves diagnostics but can leak sensitive server-side information (stack traces, internal hostnames/paths, or echoed request fragments) into public CI logs. On public PRs, this can reveal internal details of the orchestrator or upstream provider.
Mitigation:

  • Keep the truncation (already 200 chars) but consider gating body-snippet logging behind a DEBUG flag or redact obvious sensitive patterns (e.g., Authorization:, Bearer , AWS keys).
  • Avoid including body snippets for 401/403/407 responses, and optionally for 5xx in public repos, or hash the snippet for correlation without content exposure.

// carries a reason (e.g. upstream timeout) that the bare status code hides.
let snippet = '';
try { snippet = (await r.text()).replace(/\s+/g, ' ').trim().slice(0, 200); } catch { /* ignore */ }

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nell Nell — OK to ignore read errors on the body snippet? I think that’s fine, but do we want to log if reading fails (for diagnosability), or is silence preferable here?

throw new Error(`http ${r.status} ${r.statusText}${snippet ? ` — ${snippet}` : ''}`);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nell Nell — Would it help triage to include the JSON-RPC method in the error? That makes it clear whether this was tasks/new vs tasks/get.

Suggested change
throw new Error(`http ${r.status} ${r.statusText}${snippet ? ` — ${snippet}` : ''}`);
throw new Error(`http ${r.status} ${r.statusText}${snippet ? ` — ${snippet}` : ''} (method: ${body?.method || '?'})`);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — Minor thought: r.text() reads the entire body before slicing to 200 chars. Most error bodies are small, so probably fine, but if you’re worried about a huge gateway payload this could allocate more than necessary. No action required — just calling it out; the whitespace collapse + slice already helps.

}
const json = await r.json();
if (json.error) {
throw new Error(`json-rpc error ${json.error.code ?? '?'}: ${json.error.message ?? 'unknown'}`);
Expand All @@ -177,8 +192,10 @@ function extractText(task) {
}

// Submit the review asynchronously and poll to completion. Returns the
// persona's markdown review, or null on failure.
async function reviewWithAgent({ core, title, url, token, messageText, msgId }) {
// persona's markdown review, or null on failure. `maxPollMs` bounds the poll

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nell Nell — Returning null here means we lose the reason and just treat all failures the same. Return error here? Even a minimal change like returning a tagged outcome (e.g. {text, err, reason: 'timeout'|'submit'|'task-failed'|'parse'}) would let callers decide when to retry vs degrade.

// window (defaults to MAX_POLL_MS; the orchestrator passes a shorter per-attempt
// budget so it can afford a fresh-task retry within the job timeout).
async function reviewWithAgent({ core, title, url, token, messageText, msgId, maxPollMs = MAX_POLL_MS }) {

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — The new maxPollMs parameter is a good addition. To improve readability and IDE hints, consider adding a brief JSDoc block documenting the parameter types and behavior, and link it to the orchestrator usage. Also, within this function, the submit-retry loop uses hard-coded values (3 attempts and a 5s incremental backoff) across multiple lines — extracting these into named constants will reduce drift and make logs self-consistent.

Example:

// Tunables for submit retry behavior.
const SUBMIT_ATTEMPTS = 3;
const SUBMIT_BACKOFF_MS = 5000;

// ...
for (let attempt = 1; attempt <= SUBMIT_ATTEMPTS; attempt++) { /* ... */ }
if (attempt < SUBMIT_ATTEMPTS) await sleep(attempt * SUBMIT_BACKOFF_MS);

And reference SUBMIT_ATTEMPTS in the “could not submit after … attempts” log.

let taskId = null;
for (let attempt = 1; attempt <= 3; attempt++) {
try {
Expand All @@ -193,37 +210,54 @@ async function reviewWithAgent({ core, title, url, token, messageText, msgId })
if (taskId) break;
core.info(` ${title}: submit returned no task id (attempt ${attempt})`);
} catch (e) {
core.info(` ${title}: submit attempt ${attempt} failed (${safe(e.message)})`);
core.info(` ${title}: submit attempt ${attempt} failed (${safe(errDetail(e))})`);
}
if (attempt < 3) await sleep(attempt * 5000);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — The incremental backoff uses a hard-coded 5000ms step. Extract to a named constant (e.g., SUBMIT_BACKOFF_MS) with a short comment about why that value is chosen, to make future tuning clearer and avoid hidden magic numbers.

}
if (!taskId) return null;
if (!taskId) {
core.warning(`${title}: could not submit a task after 3 attempts`);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — The log string hard-codes “3 attempts”, which can drift if the loop is ever changed. Tie this to a constant (e.g., SUBMIT_ATTEMPTS) and reuse it in both the loop guard and the message to keep them in sync.

return null;
}

// Track poll outcomes so a timeout is diagnosable: a stuck task shows a
// non-terminal last state with healthy polls; a network problem shows poll errors.
const start = Date.now();
while (Date.now() - start < MAX_POLL_MS) {
let polls = 0, pollErrs = 0, lastState = 'none';
while (Date.now() - start < maxPollMs) {
await sleep(POLL_INTERVAL_MS);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nell Nell — nit: We sleep before the first poll. Would it be neater to poll immediately once (t=0) and then sleep between subsequent polls?

let task;
try {
const g = await rpc(url, token, { jsonrpc: '2.0', id: `${msgId}-get`, method: 'tasks/get', params: { id: taskId } });
task = g.result;
polls++;
} catch (e) {
core.info(` ${title}: poll error (${safe(e.message)}), will retry`);
pollErrs++;
core.info(` ${title}: poll error (${safe(errDetail(e))}), will retry`);

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — I am a little bit skeptical of logging every poll error at info — this could spam the log under intermittent network issues. Maybe bump to debug or rate-limit (e.g., only every Nth occurrence) and rely on the final “(X polls ok, Y poll errors)” line for summary?

Suggested change
core.info(` ${title}: poll error (${safe(errDetail(e))}), will retry`);
core.debug(` ${title}: poll error (${safe(errDetail(e))}), will retry`);

No big deal though if we want to keep it at info.

continue;
}
const state = task?.status?.state;
if (state) lastState = state;
if (state === 'completed') return stripOuterFence(extractText(task));
if (state === 'failed' || state === 'canceled' || state === 'rejected') {

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — I think we should surface any server-provided reason on terminal failure. Right now we only log “task failed/canceled/rejected”.
Suggestion:

Suggested change
if (state === 'failed' || state === 'canceled' || state === 'rejected') {
if (state === 'failed' || state === 'canceled' || state === 'rejected') {
const reason = task?.status?.reason || task?.status?.error || task?.status?.message;
core.warning(`${title}: task ${state}${reason ? ` — ${reason}` : ''}`);
return null;
}

That’ll make failures more diagnosable without another repro.

core.warning(`${title}: task ${state}`);
return null;
}
}
core.warning(`${title}: task did not complete within ${MAX_POLL_MS / 1000}s`);
core.warning(`${title}: task did not complete within ${maxPollMs / 1000}s ` +

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — The timeout warning now includes last state and poll counters, which is great. Consider also logging the taskId once at submission success (redacting if needed) so that timeouts and failures can be correlated with server-side logs. A single core.info at submission with taskId would help later investigations without cluttering every poll.

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — nit: It might be helpful to include the taskId in the timeout warning so we can correlate with server-side logs, especially when we have multiple concurrent jobs.

Suggested change
core.warning(`${title}: task did not complete within ${maxPollMs / 1000}s ` +
core.warning(`${title}: task ${taskId} did not complete within ${maxPollMs / 1000}s (last state: ${lastState}, ${polls} polls ok, ${pollErrs} poll errors)`);

`(last state: ${lastState}, ${polls} polls ok, ${pollErrs} poll errors)`);
return null;
}

// --- Orchestrator: cross-persona deduplication of inline comments ---
const ORCH_URL_ENV = 'ORCHESTRATOR_AGENT_URL';
const ORCH_TOKEN_ENV = 'ORCHESTRATOR_AGENT_TOKEN';
// A healthy orchestrator dedups in ~2-3 min; a much longer poll means the task
// is stuck server-side, not slow. So bound each attempt and resubmit a *fresh*
// task once — a stuck task usually doesn't recur on a new submission. Two
// attempts at this budget still fit comfortably inside the 30m job timeout
// (alongside the persona phase + posting).
const ORCH_ATTEMPTS = 2;

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — ORCH_ATTEMPTS and ORCH_POLL_MS are good abstractions. For maintainability and operational tuning without code changes, consider allowing environment overrides with sane defaults, e.g.:

const ORCH_ATTEMPTS = Number(process.env.ORCHESTRATOR_ATTEMPTS || 2);
const ORCH_POLL_MS = Number(process.env.ORCHESTRATOR_POLL_MS || 7 * 60 * 1000);

Document these env vars in a header comment.

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nell Nell — nit: Make ORCH_ATTEMPTS / ORCH_POLL_MS overridable via env (with sensible defaults)? Might be handy to tune in CI if we hit edge cases, but definitely not a blocking concern.

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — I wonder if we should compute ORCH_POLL_MS based on the remaining job budget (or tie it to MAX_POLL_MS) rather than a fixed 7m. If personas run long, two fixed 7m windows could still bump into the 30m job limit. Not a blocker — just raising it — but we could do something like “min(remaining_time - safety_margin, 7m)” per attempt.

const ORCH_POLL_MS = 7 * 60 * 1000;

// Robustly extract the orchestrator's { clusters: [...] } JSON from its response
// (tolerates a ```json fence — single- or multi-line — or stray prose). Returns
Expand Down Expand Up @@ -287,20 +321,39 @@ async function orchestrateDedup({ core, allInline }) {
if (allInline.length < 2) return empty; // nothing to dedupe (need ≥2 comments)

try {
const msgId = `council-orchestrator-${process.env.GITHUB_RUN_ID}`;
const payload = JSON.stringify(allInline.map(c =>
({ id: c.id, persona: c.persona, file: c.file, line: c.line, body: c.body })));
const messageText =
`Here are ${allInline.length} inline review comments from one pull request, as a JSON array. ` +
`Identify redundant duplicate groups and return the clusters JSON per your instructions.\n\n${payload}`;

const out = await reviewWithAgent({ core, title: 'Orchestrator', url, token, messageText, msgId });
if (!out) { core.warning('Orchestrator: no response — posting all comments unfiltered'); return empty; }
const parsed = parseClusters(out);
if (!parsed) { core.warning('Orchestrator: unparseable response — posting all comments unfiltered'); return empty; }
return applyClusters({ core, allInline, parsed });
// Up to ORCH_ATTEMPTS, each a *fresh* task bounded by ORCH_POLL_MS — so a
// task that hangs server-side OR returns garbled output gets a second shot.
const runKey = `${process.env.GITHUB_RUN_ID}-${process.env.GITHUB_RUN_ATTEMPT || '1'}`;
for (let attempt = 1; attempt <= ORCH_ATTEMPTS; attempt++) {

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — The fresh-task retry loop in orchestrateDedup is the main new behavior and should be unit tested. To make that straightforward, add a small dependency-injection seam so tests can stub the agent call without touching network/RPC. For example, accept a reviewFn parameter (defaulting to reviewWithAgent), or export reviewWithAgent and orchestrateDedup from a module where tests can inject a stub. That enables tests for: no response then success; unparseable then success; both attempts fail; poll timeout path.

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — Do we want to consider canceling the previous attempt’s task before resubmitting a fresh one? Right now we create a new task after the prior attempt times out and the old one keeps running (and may never finish). If the API exposes a cancel, we could plumb it through reviewWithAgent (e.g., return the taskId so a caller can cancel on retry), but I could be swayed either way if that’s not available or adds too much complexity.

// RUN_ATTEMPT + per-attempt suffix → a distinct msgId (hence a fresh
// server-side task) on every submission, including GitHub "Re-run" re-runs.
const msgId = `council-orchestrator-${runKey}-${attempt}`;
const out = await reviewWithAgent({
core, title: `Orchestrator (attempt ${attempt}/${ORCH_ATTEMPTS})`,
url, token, messageText, msgId, maxPollMs: ORCH_POLL_MS,
});
const last = attempt === ORCH_ATTEMPTS;
if (!out) {
if (!last) core.info('Orchestrator: no response — resubmitting a fresh task');
continue;
}
const parsed = parseClusters(out);
if (!parsed) {
if (!last) core.info('Orchestrator: unparseable response — resubmitting a fresh task');
continue;
}
return applyClusters({ core, allInline, parsed });
}
core.warning('Orchestrator: no usable response after retries — posting all comments unfiltered');
return empty;
} catch (e) {
core.warning(`Orchestrator: unexpected error (${safe(e.message)}) — posting all comments unfiltered`);
core.warning(`Orchestrator: unexpected error (${safe(errDetail(e))}) — posting all comments unfiltered`);
return empty;
}
}
Expand Down Expand Up @@ -363,7 +416,9 @@ module.exports = async ({ github, context, core }) => {
// 3. Fan out to all configured personas in parallel; isolate failures.
core.info(`Reviewing PR #${pull_number} with ${active.length} persona(s): ${active.map(p => p.title).join(', ')}`);
const results = await Promise.all(active.map(async p => {
const msgId = `council-${p.key}-${pull_number}-${process.env.GITHUB_RUN_ID}`;
// Include RUN_ATTEMPT so a GitHub "Re-run" gets a fresh server-side task (the

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — Run key/msgId construction includes RUN_ATTEMPT here, and a similar runKey is built inside orchestrateDedup. Factor this into a tiny helper (e.g., getRunKey() or buildMsgId(prefix, parts...)) to avoid duplication and keep the shape consistent across personas and the orchestrator.

// run id alone is stable across re-runs and could return a stale cached task).
const msgId = `council-${p.key}-${pull_number}-${process.env.GITHUB_RUN_ID}-${process.env.GITHUB_RUN_ATTEMPT || '1'}`;

@github-actions github-actions Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — nit: Since we now build msgIds in a couple places (personas and orchestrator), would it be worth a small helper like buildMsgId(prefix, keyParts...) to keep the RUN_ID/RUN_ATTEMPT pattern consistent and avoid drift? Fine to leave as-is though.

const review = await reviewWithAgent({ core, title: p.title, url: p.url, token: p.token, messageText, msgId });
if (!review) {
core.warning(`${p.title}: no review produced, skipping`);
Expand Down Expand Up @@ -427,7 +482,7 @@ module.exports = async ({ github, context, core }) => {
try {
await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c.id });
} catch (e) {
core.info(` ${p.title}: could not delete inline #${c.id} (${safe(e.message)})`);
core.info(` ${p.title}: could not delete inline #${c.id} (${safe(errDetail(e))})`);
}
}
let inline = 0;
Expand All @@ -441,7 +496,7 @@ module.exports = async ({ github, context, core }) => {
inline++;
} catch (e) {
// Line wasn't commentable after all — fold it into the summary so it isn't lost.
core.info(` ${p.title}: inline anchor failed at ${safe(f.file)}:${f.line} (${safe(e.message)}), folding into summary`);
core.info(` ${p.title}: inline anchor failed at ${safe(f.file)}:${f.line} (${safe(errDetail(e))}), folding into summary`);
folded.push(f);
}
}
Expand Down Expand Up @@ -471,7 +526,7 @@ module.exports = async ({ github, context, core }) => {
}
summaries++;
} catch (e) {
core.warning(`${p.title}: failed to post/update summary (${safe(e.message)})`);
core.warning(`${p.title}: failed to post/update summary (${safe(errDetail(e))})`);
}
}
core.info(`Done: ${summaries}/${active.length} summaries, ${inlineTotal} inline posted, ${dropped} duplicate(s) dropped on PR #${pull_number}`);
Expand Down
Loading