Council of Claudes: orchestrator agent for cross-persona comment dedup

[stevegaossou]

What

Adds the orchestrator — an intelligence layer between persona fan-out and posting that removes redundant inline comments (multiple personas making the same point), the #1 reviewer complaint from team feedback.

Per the design (orchestrator-design.md), it does one thing — dedup. All other quality control (nit/volume reduction) stays with the individual personas as a separate future iteration. This keeps the orchestrator simple and avoids a single point of complexity, consistent with the multi-agent philosophy.

How it works

1. Personas review in parallel as today → inline comments + summaries.
2. The script collects all personas' anchorable inline findings, each with a stable id, as {id, persona, file, line, body} (comments-only — no diff).
3. The orchestrator (new kagent agent, async submit+poll) returns {clusters:[{survivor, duplicates, reason}]} — decisions only, never rewrites bodies.
4. The script drops only the flagged duplicates; survivors post under their original personas (Nell/Casey voices preserved).

Safety (lossless by construction)

• omission = keep (anything unmentioned is kept), keep-wins on conflict, unknown ids ignored.
• Any failure / timeout / unparseable response / unset secret → post everything unfiltered. Never lose a review.
• Gated on its secret → this PR is a no-op until the orchestrator agent + secrets exist.

To activate

1. Create the orchestrator agent on kagent with scripts/orchestrator.md (model gpt-5).
2. Add ORCHESTRATOR_AGENT_URL / ORCHESTRATOR_AGENT_TOKEN secrets.

Validation

Mock-tested: dedup (incl. fenced-JSON parsing), graceful degradation (unconfigured + garbage response), keep-wins, and unknown-id handling — all pass. End-to-end validation will be a fresh duplicate PR vs. the labeled baselines (#218/#200/#201), checking that the redundant clusters humans flagged collapse.

Note: this iteration targets redundancy only. Nit-noise (Maintainability test-padding, Nell/Casey nits) is unchanged and is the separate, planned Lever-2 change.

🤖 Generated with Claude Code

[github-actions]

🔎 Council of Claudes — Correctness

Note

Correctness lens · bugs · completeness · concurrency · edge cases

No correctness issues found

The orchestrator integration appears to meet the stated design goals: it deduplicates only inline comments, is gated on its own secrets, degrades safely to “post everything” on any failure/unset secret, and applies defensive parsing and application of decisions (unknown IDs ignored, keep-wins on conflicts). The ephemeral per-run IDs are unique per persona and used consistently through apply/post. Logging provides useful visibility without affecting behavior. I don’t see any obvious logic bugs, missed edge cases, or API misuse in the added paths based on the diff provided.

No line-specific findings.

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

🧪 Council of Claudes — Maintainability & Tests

Tip

Maintainability & Tests lens · simplicity · tests · docs · idioms

Well-scoped, defensively implemented addition; however, the new dedup logic lands without automated tests. Recommend extracting helper logic for unit tests and adding a small self-test workflow; a couple of minor maintainability/documentation tweaks suggested.

• Tests: This PR introduces nontrivial, stateful logic (parseClusters, applyClusters, orchestrateDedup) at the core of comment posting, but there are no automated tests. Given this sits on the critical path of review output, a lightweight Node-based unit test suite would pay back quickly and prevent regressions (e.g., shape drift in the orchestrator’s response, parsing fallbacks, keep-wins behavior, unknown-id handling).
• Separation for testability: Consider moving the orchestrator helpers into a small module (or at least exporting them via a test-only handle) to enable hermetic unit tests without invoking GitHub APIs or kagent.
• Documentation cross-link/versioning: You’ve added an excellent design doc and an orchestrator system prompt. To prevent future drift, add an explicit “schema v1” tag and a “keep in sync with council_of_claudes.js” pointer in orchestrator.md, and a reciprocal link in the JS.
• Optional CI: A tiny GH Action job that runs “node” unit tests for this script would give fast feedback without touching Calico’s main CI.

Line-specific findings:

_{🤖 Council of Claudes · 9 inline comment(s)}

[github-actions]

🛡️ Council of Claudes — Security

Caution

Security lens · validation · secrets · authz · isolation

2 potential security issues

• Outbound call to a third-party “orchestrator” service is gated on secrets (good), but the URL is taken verbatim from env without scheme/host validation. A misconfigured secret could send the dedup payload and bearer token to a plaintext HTTP endpoint or an unintended host, risking token leakage and exfiltration of PR-derived data. Recommend enforcing HTTPS and a host allowlist before making the request.
• Double-check workflow trigger/context so these new secrets are never exposed in untrusted PR contexts (e.g., forks). The code itself doesn’t echo tokens, but if the job runs with secrets on forked PRs, an attacker could cause external calls or glean endpoint details via error logs. Ensure this job only runs where GitHub will not expose repository secrets to forks, or add explicit guards.

_{🤖 Council of Claudes · 3 inline comment(s)}

[github-actions]

Council of Claudes — Nell

Important

Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments

No correctness issues found; looks thoughtfully defensive. A couple of small clarity/robustness nits and one comment/code mismatch. If we can, I'd love a tiny unit harness for parseClusters/applyClusters (even as a hack/ script) to lock in the keep-wins/unknown-id behaviours — but fine to defer.

• The “pure decision logic” comment on applyClusters reads a bit stronger than what the function does (it logs). Either move the logs out or soften the comment?
• Input robustness: we already parse fenced JSON from the orchestrator, but we don’t fence the input JSON we send to it. Wrapping the payload in a ```json block might help its parser.
• parseClusters only tries the first fenced block; consider scanning all fenced blocks (minor).
• nit: safe(cl.reason) — safe handles undefined/null, no? Just checking.

Line-specific findings:

_{🤖 Council of Claudes · 4 inline comment(s)}

[github-actions]

Council of Claudes — Casey

Warning

Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness

No correctness issues found. I think the overall shape looks good — conservative, “lossless by construction,” and well-fenced behind secrets so this is a no-op until configured. Nice touches on the keep-wins rule, unknown-id defense, and logging enough to audit the orchestrator’s decisions. My main nits are around simplifying/strengthening the JSON parsing path and making the request text a bit more prescriptive so we don’t need such a permissive parser. One thought on tests: since parseClusters/applyClusters are pure and central to the behavior, I think a tiny unit-test harness for those two (even as a simple node script in hack/ that runs in CI) would pull its weight — we generally try to land tests alongside behavior changes. WDYT?

• I am a little bit skeptical of the “brace slice” fallback in parseClusters — it feels clever but hard to reason about under weird outputs. If we can make the orchestrator reliably respond with only JSON (or a fenced block), I think we can drop that path and reduce the chance of accidentally parsing the wrong substring.
• Minor robustness: safe(e.message) assumes an Error-like object; worth guarding for thrown strings/unknowns.
• Small ergonomics: adding one sentence to the orchestrator prompt message (“Respond with only the JSON object, no prose”) would let us tighten the parser and rely on the fence path — less parsing surface area, fewer surprises.
• Non-blocking: IDs are ephemeral by design, which is fine — if you ever want easier cross-referencing in logs, including file:line in the id or in the keep/drop logs might help debugging. Not required though.

_{🤖 Council of Claudes · 6 inline comment(s)}

[Copilot]

Pull request overview

Adds an “orchestrator” stage to the Council of Claudes GitHub Action that deduplicates redundant inline review comments across personas, while preserving original comment bodies/persona attribution and failing open (posting everything) when unconfigured or on any error.

Changes:

• Introduces an orchestrator system prompt (orchestrator.md) that instructs the agent to return duplicate clusters by stable comment IDs only.
• Extends council_of_claudes.js to collect all anchorable inline findings across personas, call the orchestrator, and skip posting IDs marked as duplicates (keep-wins; unknown IDs ignored; failure → drop nothing).
• Wires optional orchestrator URL/token secrets into the workflow environment.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
.github/workflows/scripts/orchestrator.md	Defines the orchestrator agent contract (input schema, dedup criteria, and strict JSON output shape).
.github/workflows/scripts/council_of_claudes.js	Implements cross-persona inline-comment dedup orchestration with defensive parsing and lossless fallback behavior.
.github/workflows/council_of_claudes.yml	Adds optional ORCHESTRATOR_AGENT_URL/TOKEN env wiring; workflow remains a no-op when unset.

[github-actions]

🧪 Maintainability & Tests — Consider extracting the “Orchestrator” block (parseClusters, applyClusters, orchestrateDedup and the ORCH_* constants) into a dedicated module (e.g., .github/workflows/scripts/orchestrator.js) and import it here. This reduces file size/complexity and makes the logic easier to unit test in isolation. If you prefer not to split files, minimally export the helpers for tests:

// near the bottom, before module.exports default:
module.exports._orchestratorTest = { parseClusters, applyClusters, orchestrateDedup };

This keeps production behavior unchanged while enabling direct imports in a small Node test.

[github-actions]

🧪 Maintainability & Tests — parseClusters has several tolerant fallback paths (fenced json, substring braces, raw text). Add unit tests covering:

• Pure JSON object, fenced single-line and multi-line ```json blocks, and responses with leading/trailing prose.
• Multiple fences present (ensure the right candidate is chosen).
• Invalid JSON and non-matching shapes (returns null).
  A minimal node:test or assert-based test file under .github/workflows/scripts/ can exercise these without any build tooling.

[github-actions]

🧪 Maintainability & Tests — applyClusters is pure and central to safety (omission=keep, keep-wins, unknown-id ignore). Add unit tests for:

• Unknown ids in survivor/duplicates are ignored (drop set empty).
• Conflicting listing (an id appears as survivor in one cluster and duplicate in another) → keep-wins triggers; drop set excludes that id and a warning is logged.
• Survivor with empty duplicates is ignored (no-op).
• Multiple clusters applied with disjoint ids.
  Codify these to prevent future regressions in the safety guarantees.

[github-actions]

🧪 Maintainability & Tests — Please verify reviewWithAgent already has a bounded poll/timeout. If not, consider adding an explicit timeout via AbortController/fetch timeout or a max poll duration passed into reviewWithAgent, so a stuck orchestrator can’t stall posting indefinitely. Add a brief comment here pointing to the timeout semantics to aid future maintainers.

[github-actions]

🧪 Maintainability & Tests — Great inline rationale for ephemeral ids. For maintainability, add a brief note that these ids are intentionally not correlated with GitHub review comment IDs and exist only within a single run to drive orchestrator decisions. This helps future refactors avoid accidentally persisting or reusing them across runs.

[github-actions]

🧪 Maintainability & Tests — Now that dedup happens globally, consider logging a short per-persona summary in addition to the global dropped count (e.g., “Correctness: posted X, dropped Y”), to make it easier to spot if one persona is consistently being deduped away. This is optional, but it can help tune prompt-level levers later.

[github-actions]

🧪 Maintainability & Tests — Nice addition to the final summary. If you add unit tests, also assert the log string format in at least one test to detect accidental changes (people often grep CI logs for this).

[github-actions]

🧪 Maintainability & Tests — Add a simple version tag and cross-link to the JS that consumes this schema to prevent drift:

• At the top: “Schema: v1 (keep in sync with .github/workflows/scripts/council_of_claudes.js parseClusters/applyClusters)”
• If the output shape changes, the code must be updated alongside this file in the same PR.

[github-actions]

🧪 Maintainability & Tests — The ITER suffix is a helpful addition. Consider adding a one-liner to hack/council-of-claudes/README.md under the Tools section documenting usage (e.g., “Use ITER=v2 to create parallel duplicate PRs for the same sample: ITER=v2 ./gen-benchmark-pr.sh ”). This makes the knob discoverable without having to read the design doc.

[github-actions]

🛡️ Security — The workflow introduces new secrets (ORCHESTRATOR_AGENT_URL/TOKEN). If this job can run in contexts where repository secrets are available to untrusted fork PRs, an attacker could cause outbound calls to an attacker-controlled URL or probe for environment details through log messages. Mitigation: ensure the workflow only runs in trusted contexts (e.g., not on untrusted forks) or add an explicit guard like if: ${{ !github.event.pull_request.head.repo.fork }}. Alternatively, trigger via pull_request_target with extreme caution and strict input validation on all consumed data.

[github-actions]

🛡️ Security — The orchestrator URL is read from the environment and used for an outbound request without validating the scheme or host. A misconfigured URL could downgrade to plaintext HTTP or redirect to an unintended host, leaking the dedup payload and the bearer token. Mitigation: before calling reviewWithAgent, validate the URL:

• Require HTTPS: new URL(url).protocol === 'https:'
• Optionally enforce an allowlist for hostnames (e.g., agents.tigera.ai)
• Reject/skip orchestration (post everything) if validation fails, and log a warning without printing the raw URL.

Example:

const u = new URL(url);
if (u.protocol !== 'https:' || !['agents.tigera.ai'].includes(u.hostname)) {
  core.warning('Orchestrator URL failed validation — skipping dedup and posting all comments.');
  return empty;
}

[github-actions]

🛡️ Security — On error, the code logs safe(e.message). Depending on the underlying HTTP client, error messages can include request URLs or other connection details. While tokens aren’t in the URL here, exposing internal hostnames in public logs may still be undesirable. Mitigation: redact/omit URLs/hostnames in error messages (e.g., replace them with a constant tag) or map exceptions to a generic message. Ensure reviewWithAgent also avoids logging tokens/URLs on failures.

[github-actions]

Nell — The comment says “Pure decision logic … No env / network — directly unit testable.” but the function logs via core.info/core.warning. So it isn’t pure anymore. Soften the comment, or move the logging into the caller and keep this one side‑effect free?

Suggested change

	// Pure decision logic: given the inline comments and the orchestrator's parsed
	// Decision logic: given the inline comments and the orchestrator's parsed
	// clusters, return the SET of ids to drop. Defensive against arbitrary LLM
	// output: honor a cluster only if its survivor is a known id; drop only known
	// duplicate ids; keep-wins (an id that survives anywhere is never dropped);
	// anything not mentioned is implicitly kept.

[github-actions]

Nell — parseClusters only checks the first fenced block. If the model emits multiple fences (preamble, then JSON), should we scan all fenced blocks and try each? Not a blocker, but might make it a touch more robust.

[github-actions]

Nell — safe(cl.reason): OK to assume reason is always a string? If the field is missing or non‑string, does safe() handle that? (I think it does, but just checking.)

[github-actions]

Nell — We send the raw JSON array inline in messageText. Do we want to fence it as JSON to help the orchestrator’s parser? Something like:

Suggested change

	`Here are ${allInline.length} inline review comments from one pull request, as a JSON array. ` +
	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` +
	"```json\n" + payload + "\n```";

[github-actions]

Casey — I think this lenient “first { … last }” slice might be too permissive — it could grab the wrong substring if prose contains braces or multiple JSON-ish snippets. Since you already (a) try the whole message and (b) prefer a fenced block, can we drop this fallback (or at least only use it when there’s exactly one well-formed object between balanced backticks)?

Suggested change

	const first = text.indexOf('{'), last = text.lastIndexOf('}');
	const fence = text.match(/```[a-zA-Z]*\s*([\s\S]*?)```/);
	if (fence) tries.push(fence[1].trim());
	// Drop the brace-slice fallback to avoid accidental mis-parses; rely on fenced or plain-JSON only.

Reason: simpler and less error-prone; the agent instructions can enforce JSON-only or a fenced block.

[github-actions]

Casey — I wonder if we should be more prescriptive here to reduce parser flexibility later — can we explicitly ask for “only the JSON object, no prose” in the message? That’d let us rely on the fence/plain-JSON path and avoid the broad brace-slice.

Suggested change

	`Here are ${allInline.length} inline review comments from one pull request, as a JSON array. ` +
	const messageText =
	`Here are ${allInline.length} inline review comments from one pull request, as a JSON array. ` +
	`Identify redundant duplicate groups and return ONLY the JSON object described (no prose). ` +
	`A fenced \`\`\`json block is acceptable.\n\n${payload}`;

[github-actions]

Casey — Do we know if reviewWithAgent ever throws a non-Error (e.g., a string)? safe(e.message) will be undefined in that case — might be safer to stringify the whole error.

Suggested change

	core.warning(`Orchestrator: unexpected error (${safe(e.message)}) — posting all comments unfiltered`);
	core.warning(`Orchestrator: unexpected error (${safe(e && (e.message || String(e)))}) — posting all comments unfiltered`);

No big deal though if safe() already handles undefined cleanly.

[github-actions]

Casey — I think using i is fine as an ephemeral id, but would it help debuggability to include an easy-to-eye-catch hint (e.g., file:line) in the id or (maybe better) in the “keep/drop” logs? The id never persists, so this is just about making the action logs easier to correlate. Fine to leave as-is, though.

Suggested change

	const id = `${p.key}-${i}`;
	const id = `${p.key}-${i}`; // consider `${p.key}-${f.file}:${f.line}-${i}` for easier log correlation

[github-actions]

Casey — nit: This log can get a bit long if reasons are verbose. Maybe cap reason length to keep the action log tidy?

Suggested change

	if (dups.length) core.info(` orchestrator: keep ${cl.survivor}, drop [${dups.join(', ')}] — ${safe(cl.reason)}`);
	const reason = safe(cl.reason || '');
	const trimmed = reason.length > 200 ? `${reason.slice(0, 200)}…` : reason;
	if (dups.length) core.info(` orchestrator: keep ${cl.survivor}, drop [${dups.join(', ')}] — ${trimmed}`);

Totally optional; up to you.

[github-actions]

Casey — Small portability question: this uses bash parameter expansion (:+). I don’t remember the shebang offhand — is this file bash-scripted already? If not, we should add/update it to bash to avoid sh quirks. WDYT?

Suggested change

	SUFFIX="${ITER:+-$ITER}"
	# At top of file:
	#!/usr/bin/env bash

If it’s already there, ignore me.