feat(sandcastle): add multi-critic ensemble with voted merge and parallel execution

[jerome-benoit]

Summary

Add a multi-critic ensemble to .sandcastle/ with parallel critic execution, voted merge, optional arbiter synthesis, severity-aware DoS bounds, and backward-compatible N=1 behavior.

LoopStrategy additions

Field	Type	Default	Purpose
criticPool	readonly [AgentSpec, ...AgentSpec[]]	AGENT_CRITIC_POOL_DEFAULT	Non-empty critic catalog of { model, effort } pairs.
criticCount	number	max(AGENT_CRITIC_COUNT, criticPool.length)	Critic slots per round, capped at MAX_CRITIC_COUNT = 8.
criticAgreementThreshold	`number	((validCount: number) => number)`	⌈validCount × CRITIC_AGREEMENT_FRACTION⌉
criticFillStrategy	`round-robin	random-with-replacement`	round-robin
criticEnsembleSeed	number	unset	Required for deterministic random-with-replacement fill.
arbiter	{ agent?: AgentSpec; promptFile: string }	unset	Optional stage-2 arbiter over merged HIGH/CRITICAL findings.

Behavior

1. Parallel critics — critic slots fan out with Promise.allSettled; each slot keeps one parse retry with retry nonce ${nonce}-r1.
2. Quorum gate — merge runs only when validCount >= ceil(resolvedCriticCount / 2).
3. Cross-critic dedup — key ${file}::${normalizeCategory(category)}::${ctxHash(line)}. Severity is intentionally absent so the same defect at different severities aggregates.
4. Voted merge — severity and confidence aggregate by median tie-up. title / description / suggestion come from the lowest-slot voter. voters[] preserves original critic-slot indices even when middle slots parse-fail.
5. Escape hatch — a below-threshold finding survives when at least one voter flags it with severity ∈ {HIGH, CRITICAL} and confidence = HIGH; contested = true is set and merged severity is clamped to [MEDIUM, HIGH]. Same-slot duplicate findings preserve escape-qualified evidence within a critic slot.
6. Per-slot runaway cap — unilateral escape-hatch contributions from a slot are dropped wholesale when they exceed CRITIC_ESCAPE_CAP_PER_SLOT.
7. Arbiter guard — parse failure, throw, or empty arbiter output preserves the merge result.
8. Registry validation — misconfiguration fails at module load with StrategyValidationError before any sandbox is spawned.
9. DoS hardening — FindingSchema enforces per-field bounds and parseFindings truncates each critic payload to MAX_FINDINGS_PER_CRITIC = 200 by severity-aware stable sort.

See .sandcastle/README.md for the full operational specification.

Files

• Types & defaults — .sandcastle/types.ts, .sandcastle/constants.ts
• Pure merge module — .sandcastle/merge-findings.ts
• Loop kernel — .sandcastle/refinement-loop.ts, .sandcastle/loop-control.ts
• Parser — .sandcastle/parse-findings.ts
• Typed errors — .sandcastle/errors.ts (SandcastleError extends Error with discriminant code)
• Registry validation — .sandcastle/strategies/index.ts
• Default strategy — .sandcastle/strategies/implement/strategy.ts
• Documentation — .sandcastle/README.md
• Tests — .sandcastle/tests/{merge-findings,refinement-loop,strategies,parse-findings,errors}.test.ts plus supporting kernel coverage

Backward compatibility

When no ensemble fields are set:

• resolveCriticSlots returns slots from AGENT_CRITIC_POOL_DEFAULT.
• The loop runs one critic slot with one parse retry.
• Quorum is 1 / 1; the merge kernel is bypassed on the N=1 path.
• The arbiter is skipped unless explicitly configured.

Quality gates

All quality gates at HEAD are green:

• pnpm typecheck
• pnpm lint
• pnpm test:sandcastle
• pnpm test
• pnpm build

Out of scope

• Specialized critic prompts per domain (security / perf / style)
• Logprob-based aggregation
• Dawid-Skene critic calibration
• Telemetry sidecar persistence of perCriticOutputs
• Task-discovery redesign in task-source.ts
• Error-hierarchy unification across all sandcastle-specific error types

References

Load-bearing prior art:

• Wang et al. 2024 — Mixture-of-Agents, arXiv:2406.04692
• Verga et al. 2024 — PoLL Panel of LLM Evaluators, arXiv:2404.18796
• Castro & Liskov 1999 — Practical Byzantine Fault Tolerance, OSDI 1999

Background reading:

• Wang et al. 2022 — Self-Consistency, arXiv:2203.11171
• Zheng et al. 2023 — LLM-as-a-Judge biases, arXiv:2306.05685

AI-generated code disclosure

Per SAP CONTRIBUTING_USING_GENAI guideline, this PR was produced with substantial AI assistance.

[hyperspace-insights]

Summary

The following content is AI-generated and provides a summary of the pull request:

---

Add Multi-Critic Ensemble with Voted Merge and Parallel Execution

New Feature

✨ Extends the .sandcastle/ actor↔critic refinement loop from a single critic per round to an N-critic ensemble with parallel execution, majority-vote deduplication, and ensemble disagreement signalling. Fully backward-compatible: when no ensemble fields are configured on a strategy, the loop runs the legacy single-critic path with byte-equivalent behavior.

The design addresses the single-point-of-failure in code review quality by running up to 8 heterogeneous critic models simultaneously and aggregating their findings via ordinal severity aggregation, cross-critic deduplication, and quorum gating — drawing on LLM ensemble research (PoLL, MoA, Self-Consistency).

Changes

• .sandcastle/README.md (new): Comprehensive documentation covering module map, ensemble semantics (slot resolution, parallel execution, quorum, merge algorithm, singleton CRITICAL escape, disagreement scoring), validation rules, cost model, and literature references.
• .sandcastle/constants.ts: Added ensemble constants: AGENT_CRITIC_COUNT, AGENT_CRITIC_MODELS, CRITIC_AGREEMENT_FRACTION, CRITIC_FILL_STRATEGY_DEFAULT, and MAX_CRITIC_COUNT = 8.
• .sandcastle/merge-findings.ts (new): Pure, deterministic merge module implementing mergeCriticFindings (voted dedup with median tie-up severity, singleton CRITICAL escape, disagreement scoring), resolveCriticSlots (round-robin and seeded random-with-replacement fill strategies), findingDedupKey, and normalizeCategory.
• .sandcastle/refinement-loop.ts: Refactored the critic phase into parallel Promise.allSettled fan-out over N slots (runCritic), per-slot retry logic (runOneCritic), quorum enforcement (⌈N/2⌉), and an optional stage-2 MoA-pattern arbiter pass (maybeRunArbiter). Legacy single-critic behavior preserved when only one slot is resolved.
• .sandcastle/strategies/index.ts: Added validateLoopStrategyEnsemble (exported for tests) and validateEnsembleFields (called at registry load) to fail fast on misconfigured ensemble fields before any sandbox is spawned.
• .sandcastle/types.ts: Added CriticSlot interface; extended Finding with optional votes, voters, disagreementScore, and contested fields; added all new ensemble fields to LoopStrategy; added validCriticCount to RoundSnapshot.
• tests/sandcastle/merge-findings.test.ts (new): 24 unit tests covering backward compat (N=1 identity), slot resolution, severity median tie-up, category-phrasing dedup, singleton CRITICAL escape with HIGH cap, disagreement scoring, and output ordering.
• tests/sandcastle/strategies-validation.test.ts (new): 17 unit tests verifying all validation rules (invalid counts, empty models, effort length mismatch, threshold bounds, unseeded random fill, half-set arbiter).

---

• 🔄 Regenerate and Update Summary
• ✏️ Insert as PR Description (deletes this comment)
• 🗑️ Delete comment

PR Bot Information

Version: 1.20.51

• Event Trigger: pull_request.opened
• Output Template: Default Template
• Correlation ID: 1abf1399-ffb5-44e1-a760-c7562d5faf13
• Summary Prompt: Default Prompt
• File Content Strategy: Full file content
• LLM: anthropic--claude-4.6-sonnet

[hyperspace-insights]

Summary

The PR introduces a well-structured multi-critic ensemble with parallel execution, majority voting, and deterministic merge semantics. The core design is sound and backward-compatible. Several substantive issues were identified:

1. Floating-point precision loss in deterministicIndex — combining two UInt32 values via high * 0x100000000 + low exceeds Number.MAX_SAFE_INTEGER, making the modulo result unreliable and breaking the reproducibility guarantee. Should use BigInt.

2. criticEfforts validation uses wrong upper bound in validateLoopStrategyEnsemble — when criticCount is set without criticModels, the expected length falls back to 1 instead of criticCount, causing valid configurations to be wrongly rejected and invalid ones accepted.

3. Per-slot effort array mismatch in resolveCriticSlots — resolvePerSlotEfforts is called with count (total fill-extended slots) but is validated against criticModels.length, so fill-extension slots silently lose their declared per-model effort and fall back to the scalar criticEffort.

4. Docstring incorrectly says "sample variance" for severityDisagreementScore — the implementation correctly uses population variance; the mislabeling could cause confusion when the formula is compared to textbook references.

5. Missing abort guard before retry in runOneCritic — if the abort signal fires after a parse failure but before the retry sandbox.run, the slot wastes a full idle timeout before the parent runCritic can propagate the abort.

PR Bot Information

Version: 1.20.51

• Event Trigger: pull_request.opened
• Correlation ID: 1abf1399-ffb5-44e1-a760-c7562d5faf13
• Agent Instructions:
  • AGENTS.md
  • .github/copilot-instructions.md
  • CLAUDE.md
• File Content Strategy: Full file content
• LLM: anthropic--claude-4.6-sonnet

[Copilot]

Pull request overview

Extends the .sandcastle/ actor↔critic refinement loop to support a multi-critic ensemble per round, running critics in parallel and merging their findings via deterministic voting + deduplication, with optional stage-2 arbiter refinement and strategy-level fail-fast validation.

Changes:

• Added ensemble configuration fields to strategy/types and canonical defaults (count/models/efforts/quorum/arbiter).
• Implemented ensemble merge logic (resolveCriticSlots, dedup keying, majority voting, severity/confidence aggregation, disagreement scoring).
• Refactored the refinement loop to run critics in parallel, enforce quorum, merge outputs, and (optionally) run an arbiter; added documentation and unit tests.

Reviewed changes

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

Show a summary per file

File	Description
tests/sandcastle/strategies-validation.test.ts	Unit tests for ensemble-field validation errors and backward compatibility.
tests/sandcastle/merge-findings.test.ts	Unit tests for slot resolution and deterministic merge/vote behavior.
.sandcastle/types.ts	Adds ensemble strategy fields and new optional merged-finding metadata fields.
.sandcastle/constants.ts	Introduces ensemble-related canonical defaults and limits (e.g., MAX_CRITIC_COUNT).
.sandcastle/merge-findings.ts	New pure module implementing slot resolution and voted merge/dedup/aggregation.
.sandcastle/refinement-loop.ts	Runs multiple critics in parallel, merges results, and optionally invokes an arbiter.
.sandcastle/strategies/index.ts	Adds registry-load validation for ensemble configuration + exports validator for tests.
.sandcastle/README.md	Documents module map and ensemble semantics/validation/cost model.

Comments suppressed due to low confidence (1)

.sandcastle/refinement-loop.ts:792

• runOneCritic derives nonce as ${baseNonce}-c${slot.index}, but parseFindings rejects any nonce that isn’t strictly hex (/^[0-9a-f]+$/). This makes parsing fail for every slot (including the retry), so all critics will return null and quorum will typically fail. Use a hex-only per-slot nonce (e.g., derive a hex digest from baseNonce+slotIndex) or change parseFindings to accept/escape the nonce format safely.

 * @param baseNonce - Round-level nonce; per-slot nonce is `${baseNonce}-c{slot.index}`.
 * @param slot - Resolved critic slot (model + effort + index).
 * @returns Parsed findings, or null on definitive parse failure.
 */
async function runOneCritic (
  ctx: LoopContext,
  round: number,
  baseNonce: string,
  slot: CriticSlot
): Promise<Finding[] | null> {
  const { baseBranch, sandbox, signal, spec, strategy } = ctx
  const nonce = `${baseNonce}-c${String(slot.index)}`

  let critic = await sandbox.run({
    agent: agentProvider(slot.model, slot.effort),
    completionSignal: COMPLETION_SIGNAL,
    idleTimeoutSeconds: AGENT_IDLE_TIMEOUT_S,
    maxIterations: 1,
    name: `Critic #${spec.id} R${String(round)} C${String(slot.index)}`,
    promptArgs: { ...strategy.buildCriticArgs(spec, baseBranch), NONCE: nonce },
    promptFile: strategy.criticPromptFile,
    signal,
  })

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

.sandcastle/refinement-loop.ts:569

• The perCriticOutputs param doc says "null entries excluded", but runCritic passes the raw perCriticOutputs array including nulls (and you stringify it as-is). Either filter out nulls before passing to the arbiter, or update the docstring/arbiter prompt expectations accordingly.

 * @param perCriticOutputs - Raw per-slot findings (null entries excluded).
 * @param merged - Merged finding list from `mergeCriticFindings`.

.sandcastle/merge-findings.ts:207

• resolveCriticSlots defaults count to max(AGENT_CRITIC_COUNT, pool.length), which means setting a multi-entry criticPool without also setting criticCount will run all pool entries by default. This conflicts with the docs/PR narrative that criticCount defaults to 1, and it can be a surprising cost increase. Consider defaulting count strictly to AGENT_CRITIC_COUNT (1) unless criticCount is explicitly set, or update the documentation to clearly state the pool-length-driven default.

  const pool = strategy.criticPool ?? AGENT_CRITIC_POOL_DEFAULT
  const count = strategy.criticCount ?? Math.max(AGENT_CRITIC_COUNT, pool.length)
  const fillStrategy = strategy.criticFillStrategy ?? CRITIC_FILL_STRATEGY_DEFAULT

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

.sandcastle/refinement-loop.ts:569

• The maybeRunArbiter docstring still refers to criticArbiterModel/criticArbiterPromptFile, but the code gates on strategy.arbiter. Also, perCriticOutputs is documented as having null entries excluded, but the function serializes the array including nulls (JSON.stringify(perCriticOutputs, ...)). Update the comment (or filter nulls before passing) so the documentation matches the actual behavior and current field names.

/**
 * Optional stage-2 arbiter pass (MoA pattern, arXiv:2406.04692). Triggered
 * only when `criticArbiterModel`/`criticArbiterPromptFile` are both set AND
 * the merged list contains at least one HIGH or CRITICAL finding. Failure
 * is non-fatal: returns the unrefined merged list on any error.
 * @param ctx - Loop context.
 * @param round - Current round number.
 * @param baseNonce - Round-level nonce; arbiter nonce is `${baseNonce}-arbiter`.
 * @param perCriticOutputs - Raw per-slot findings (null entries excluded).
 * @param merged - Merged finding list from `mergeCriticFindings`.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

Comments suppressed due to low confidence (1)

.sandcastle/refinement-loop.ts:664

• Per-slot nonce is ${baseNonce}-c${slot.index}, but parseFindings only accepts lowercase-hex nonces. This causes every critic parse to fail (both attempts), which will cascade into quorum failures and effectively disable the critic phase. Derive per-slot nonces using hex-only characters (e.g., append a hex digit/prefix) or adjust parseFindings to safely handle non-hex nonces.

  const { baseBranch, sandbox, signal, spec, strategy } = ctx
  const nonce = `${baseNonce}-c${String(slot.index)}`