feat(s33): collect-and-continue error policy via RunFailure slots

One project's exception no longer cancels its siblings: the wave's gather
runs with return_exceptions=True and each raised project becomes a frozen
RunFailure(project_id, error, error_type) in the defaulted
PortfolioResult.failures, while every completed project keeps its full
RunResult. RunFailure is a distinct type rather than an error field on
RunResult (six required non-defaulted fields -> dummies would be fabricated
provenance); the deviation from the spec's wording is stated in both
docstrings. TaskGroup is rejected: it cancels siblings on first exception.

snapshots reaches _merge_wave unfiltered, and results are paired back to pids
by POSITION (gather resolves in argument order) so a mid-wave failure cannot
disturb store order or misattribute the failure.

Detach points measured, not asserted:
  drop return_exceptions=True      -> RED (both new tests)
  reorder snapshots before merge   -> RED (+ Session 1's determinism test)
  pair results by sorted(), not position -> RED (failure misattributed)
  filter failed members before merge -> GREEN, measured

The last one corrects the plan: its carried-forward requirement implied
filtering before the merge was the hazard. Filtering preserves relative order,
which is all _merge_wave consumes, so the variant is undetectable AND harmless.
The docstring now names the reorder as the detach point and records the
filter asymmetry, rather than claiming a detach point that does not exist.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015vYbqW4MppACRvPhEMDpvF

src/portfolio_optimiser/run.py

@@ -95,6 +95,24 @@ class RunResult:
     checker_verdict: str = "absent"
 
 
+@dataclass(frozen=True)
+class RunFailure:
+    """One project that RAISED during a portfolio pass (S3.3, SC3 collect-and-continue).
+
+    A DISTINCT type from ``RunResult`` rather than an error field on it, and deliberately so: the
+    session spec's wording was "a ``RunResult`` slot with an error field", but ``RunResult`` is
+    frozen with six required non-defaulted fields (``:89-94``) — a run that raised before producing
+    an outcome has no honest value for ``outcome``, ``provenance`` or ``verdict``. Filling them with
+    dummies would put FABRICATED provenance into the aggregate, which is the failure mode this
+    repo's provenance rules exist to prevent. The exception is recorded as text (``error``) plus its
+    class name (``error_type``) rather than the live exception object, so a ``PortfolioResult``
+    stays a plain frozen value with no traceback frames held alive."""
+
+    project_id: str
+    error: str
+    error_type: str
+
+
 @dataclass(frozen=True)
 class DryRunReport:
     """S4.2 offline ``--live-dry-run`` outcome (comparison protocol §4 pkt 3): everything a real run
@@ -132,8 +150,14 @@ class PortfolioResult:
     The remaining fields are a thin aggregate over ``runs``: ``validated_count`` /
     ``rejected_count`` partition the outcomes; ``sum_claimed_saving_nok`` totals the claimed
     saving of the validated proposals only; ``sum_token_usage`` totals every run's
-    provenance token usage. ``stopped_early`` / ``stop_reason`` record a Step-8 goal-stop: they
+    provenance token usage. ``stopped_early`` / ``stop_reason`` record a Step-8 goal-stop, and
-    default so the frozen aggregate and every existing constructor call are unaffected."""
+    ``failures`` records the projects that RAISED (S3.3 collect-and-continue): all three default so
+    the frozen aggregate and every existing constructor call are unaffected.
+
+    ``runs`` and ``failures`` PARTITION the projects that were actually submitted — a project
+    appears in exactly one of them, never both, and the counts do not overlap. ``validated_count`` /
+    ``rejected_count`` therefore total ``len(runs)``, not the portfolio size: a failure is neither a
+    validation nor a rejection, and folding it into either would misreport the pass."""
 
     runs: tuple[RunResult, ...]
     store: VerdictStore
@@ -143,6 +167,7 @@ class PortfolioResult:
     sum_token_usage: int
     stopped_early: bool = False
     stop_reason: GoalReached | None = None
+    failures: tuple[RunFailure, ...] = ()
 
 
 def _authored_texts(result: Any, name: str) -> list[str]:
@@ -656,7 +681,16 @@ async def run_portfolio(
 
     ``concurrency`` (S3.3, D-D wave model) caps how many projects run at once. It is validated
     BEFORE anything loads — an unusable cap must surface as an error, never as a silently-empty
-    pass. At the default ``1`` the pass is the sequential one described above."""
+    pass. At the default ``1`` the pass is the sequential one described above.
+
+    Error policy (S3.3, SC3) is COLLECT-AND-CONTINUE: a project that raises does not cancel its
+    siblings and does not abort the pass. Its exception is recorded as a ``RunFailure`` in
+    ``failures`` while every project that completed keeps its full ``RunResult``, so a portfolio of
+    independent projects degrades one project at a time rather than all at once. This holds at
+    every ``k``, including ``k=1``. The policy is carried by ``return_exceptions=True`` on the
+    wave's ``gather``; ``asyncio.TaskGroup`` would cancel the siblings and is rejected for that
+    reason. Note the pass still raises for errors that are NOT one project's failure — an unknown
+    ``project_id`` and a non-positive ``concurrency`` are caller mistakes and fail fast."""
     if concurrency < 1:
         raise ValueError(
             f"concurrency must be >= 1, got {concurrency}: a non-positive wave size would run no "
@@ -673,6 +707,7 @@ async def run_portfolio(
     portfolio_baseline_ore = _to_ore(sum(projects[p].total_cost for p in ids if p in projects))
 
     runs: list[RunResult] = []
+    failures: list[RunFailure] = []
     stopped_early = False
     stop_reason: GoalReached | None = None
     for wave_ids in _waves(ids, concurrency):
@@ -713,6 +748,9 @@ async def run_portfolio(
 
         # run_portfolio only drives full runs (never dry-run), so the return narrows to RunResult;
         # the cast keeps the widened run_project signature honest without an @overload duplication.
+        # ``return_exceptions=True`` is the collect-and-continue seam (SC3): one project's
+        # exception must not cancel its siblings. ``asyncio.TaskGroup`` is deliberately NOT used —
+        # it cancels the remaining tasks on first exception, which is this policy's exact negation.
         wave_results = await asyncio.gather(
             *(
                 run_project(
@@ -733,19 +771,41 @@ async def run_portfolio(
                     meter=meter_factory() if meter_factory is not None else None,
                 )
                 for pid, snapshot in snapshots
-            )
+            ),
+            return_exceptions=True,
         )
         # ``gather`` resolves in ARGUMENT order, not completion order, and waves follow
-        # ``project_ids`` — so ``runs`` stays in caller order however the schedule interleaved.
+        # ``project_ids`` — so ``runs`` stays in caller order however the schedule interleaved, and
-        runs.extend(cast(RunResult, r) for r in wave_results)
+        # position is a sound key for pairing each result back to the pid that produced it.
+        for (pid, _snapshot), outcome in zip(snapshots, wave_results, strict=True):
+            if isinstance(outcome, BaseException):
+                failures.append(
+                    RunFailure(
+                        project_id=pid, error=str(outcome), error_type=type(outcome).__name__
+                    )
+                )
+            else:
+                runs.append(cast(RunResult, outcome))
+        # ``snapshots`` is handed over UNFILTERED. ``_merge_wave`` consumes the ORDER this sequence
+        # arrives in and nothing else, so that order is the contract. Measured honestly: filtering
+        # the failed members out here would in fact be harmless, because filtering preserves
+        # relative order — the variant is green. What is NOT harmless is anything that REORDERS
+        # (or re-pairs) the sequence, and keeping the list whole is what leaves no place for that
+        # mistake to be written. A failed project's snapshot holds no new verdicts, so it costs
+        # nothing to pass through.
         _merge_wave(store, snapshots)
 
         if stopped_early:
             break
 
     base = _aggregate(tuple(runs), store)
-    if stopped_early or stop_reason is not None:
+    if stopped_early or stop_reason is not None or failures:
-        return replace(base, stopped_early=stopped_early, stop_reason=stop_reason)
+        return replace(
+            base,
+            stopped_early=stopped_early,
+            stop_reason=stop_reason,
+            failures=tuple(failures),
+        )
     return base
 
 

tests/test_portfolio_concurrent_loadbearing.py

@@ -23,7 +23,7 @@ import pytest
 from conftest import _PORTFOLIO_DEFAULT_REPLY, _ProjectAwareUsageChatClient
 from test_portfolio import REPLIES
 
-from portfolio_optimiser.run import PortfolioResult, _waves, run_portfolio
+from portfolio_optimiser.run import PortfolioResult, RunFailure, _waves, run_portfolio
 from portfolio_optimiser.verdicts import VerdictStore
 
 # The shipped 3-project fixture, in submission order (mirrors tests/test_portfolio.py:57).
@@ -312,3 +312,191 @@ async def test_runs_follow_project_ids_not_completion_order() -> None:
         f"runs came back in {probe.completion_order()} (completion) rather than "
         f"{_PORTFOLIO_IDS} (submission) order"
     )
+
+
+# --------------------------------------------------------------------------------------------
+# Step 4 — collect-and-continue: one project's failure must not cancel its siblings, and must
+# not disturb the order the surviving projects merge in.
+# --------------------------------------------------------------------------------------------
+
+# The MIDDLE project of the wave. Middle is deliberate: a failure at either end can be dropped by
+# a truncation bug and still leave the survivors in the right relative order, so an end position
+# would let the ordering assertion below pass for the wrong reason.
+_FAILING_PID = "RV13-RAS-TP"
+
+
+class _FailingProbeClient(_OrderProbeClient):
+    """The order probe, plus a synthetic backend failure for exactly ONE project.
+
+    The failure is keyed on the prompt blob for the same reason N is (see ``_OrderProbeClient``):
+    ``client_factory`` receives the ROLE, not the project id, so per-project behaviour can only be
+    selected from the prompt. It is raised from inside a coroutine rather than synchronously,
+    because the canonical ``_inner_get_response`` is a sync method RETURNING an awaitable — raising
+    synchronously would blow up at ``asyncio.gather`` argument-construction time, before any
+    concurrency exists, and would therefore test a different thing than a mid-flight failure.
+
+    The stream path is delegated untouched, exactly as in the base probe."""
+
+    def __init__(
+        self,
+        replies: dict[str, str],
+        *,
+        default_reply: str,
+        recorder: _Recorder,
+        failing_pid: str,
+    ) -> None:
+        super().__init__(replies, default_reply=default_reply, recorder=recorder)
+        self._failing_pid = failing_pid
+
+    def _inner_get_response(
+        self,
+        *,
+        messages: Sequence[Any],
+        options: Mapping[str, Any],
+        stream: bool = False,
+        **kwargs: Any,
+    ) -> Any:
+        blob = " ".join(getattr(m, "text", "") or "" for m in messages)
+        if not stream and self._failing_pid in blob:
+            recorder = self._recorder
+            failing_pid = self._failing_pid
+
+            async def _boom() -> Any:
+                recorder.enter(failing_pid)
+                try:
+                    raise RuntimeError(f"synthetic backend failure for {failing_pid}")
+                finally:
+                    recorder.leave(failing_pid)
+
+            return _boom()
+        return super()._inner_get_response(
+            messages=messages, options=options, stream=stream, **kwargs
+        )
+
+
+async def _failing_pass(k: int, recorder: _Recorder) -> tuple[list[str], PortfolioResult]:
+    """A pass over the same 3-project fixture where the MIDDLE project's backend raises."""
+    store = VerdictStore(verdicts=[])
+
+    def factory(_role: str) -> Any:
+        return _FailingProbeClient(
+            REPLIES,
+            default_reply=_PORTFOLIO_DEFAULT_REPLY,
+            recorder=recorder,
+            failing_pid=_FAILING_PID,
+        )
+
+    result = await run_portfolio(
+        _PORTFOLIO_IDS, "local", store=store, client_factory=factory, concurrency=k
+    )
+    return [v.id for v in result.store.verdicts], result
+
+
+async def test_one_project_failure_does_not_cancel_its_siblings() -> None:
+    """Collect-and-continue (SC3): one project's exception is captured as a ``RunFailure`` slot
+    while every sibling in the same wave completes normally, and the pass itself does not raise.
+
+    **Detach point 1: drop ``return_exceptions=True`` from the wave's ``asyncio.gather`` -> this
+    test goes RED (measured).** Without it the first exception propagates out of ``gather``,
+    ``run_portfolio`` raises, and the surviving projects' results are discarded — the caller loses
+    two completed runs because a third failed.
+
+    **Detach point 2: pair the gathered results back to project ids by anything other than POSITION
+    -> this test goes RED (measured with ``sorted(snapshots)``).** ``gather`` resolves in argument
+    order, which is the only thing that makes positional pairing sound; a pairing keyed on a sorted
+    or completion-derived sequence attributes the failure to an innocent project, and the assertion
+    on ``failure.project_id`` is what catches it.
+
+    ``asyncio.TaskGroup`` is rejected for the same reason and is NOT an implementation detail: it
+    cancels its siblings on first exception, which is collect-and-continue's exact negation.
+
+    **Why a separate ``RunFailure`` rather than the spec's literal "``RunResult`` slot with an error
+    field".** ``RunResult`` is frozen with six required non-defaulted fields (``run.py:88-93``); a
+    run that never reached generation has no honest value for ``provenance``, ``verdict`` or
+    ``outcome``, and inventing them would put fabricated provenance into the aggregate — the one
+    thing this repo's provenance rules exist to prevent. The deviation is stated in the plan's
+    Step 4 and in ``PortfolioResult``'s docstring, not hidden."""
+    recorder = _Recorder()
+    store_ids, result = await _failing_pass(3, recorder)
+
+    # The pass returned rather than raising — that is half the contract, and it is asserted by
+    # having reached this line at all.
+    assert len(result.runs) == 2, (
+        f"expected the two healthy projects to survive, got {len(result.runs)} runs — a sibling "
+        f"was cancelled by {_FAILING_PID}'s failure"
+    )
+    assert len(result.failures) == 1, f"expected exactly one failure slot, got {result.failures}"
+
+    failure = result.failures[0]
+    assert isinstance(failure, RunFailure)
+    assert failure.project_id == _FAILING_PID, (
+        f"failure was attributed to {failure.project_id!r}, not {_FAILING_PID!r} — the result "
+        "list and the wave's submission list have drifted out of alignment"
+    )
+    assert failure.error_type == "RuntimeError"
+    assert "synthetic backend failure" in failure.error
+
+    # The survivors are the two healthy projects, in SUBMISSION order — 200k is FV42 (submitted
+    # first), 210k is BRU (submitted last). ``measure_type`` is not a discriminator here: FV42 and
+    # BRU deliberately share "Reduce scope".
+    assert [r.verdict.proposal_features.claimed_saving_nok for r in result.runs] == [
+        200_000.0,
+        210_000.0,
+    ]
+    # The aggregate counts only what actually ran: a failure is neither a validation nor a
+    # rejection, so it must not inflate either partition.
+    assert result.validated_count + result.rejected_count == 2
+    assert len(store_ids) == 2, (
+        f"the failed project contributed a verdict to the store ({store_ids}) — a run that never "
+        "produced an outcome must not leave one behind"
+    )
+
+
+async def test_store_order_survives_a_mid_wave_failure() -> None:
+    """The merge barrier still sees SUBMISSION order when a wave member failed.
+
+    **Detach point: REORDER the wave list on its way to ``_merge_wave`` -> this test goes RED
+    (measured with ``reversed(snapshots)``, which also takes Session 1's determinism test red).**
+    ``_merge_wave`` derives store order from the order ``snapshots`` arrives in and from nothing
+    else, so that sequence is the contract.
+
+    This is the trap Session 1 could not have caught, and said so: its determinism test contains no
+    failing project, so it cannot exercise a ``return_exceptions=True`` handler at all.
+
+    **What this test does NOT catch, measured rather than assumed.** The plan's carried-forward
+    requirement was "filter when building ``runs``/``failures``, never before the merge". Filtering
+    the failed members out of ``snapshots`` before the barrier was measured GREEN — filtering
+    preserves RELATIVE order, and relative order is all ``_merge_wave`` consumes, so the variant is
+    both undetectable here and harmless in fact. The executor still passes ``snapshots`` unfiltered,
+    but the honest reason is defensive rather than tested: it removes the place where the
+    reorder-shaped mistake would be written. Recording this asymmetry is the point — a docstring
+    that claimed "filter -> RED" would name a detach point that does not exist, which is precisely
+    the green-but-dead defect this repo's method is built to prevent.
+
+    The two probe self-checks are repeated here rather than inherited, because without them this
+    assertion is satisfied by a sequential pass, where submission and completion order coincide and
+    no ordering bug can be observed at all."""
+    recorder = _Recorder()
+    store_ids, result = await _failing_pass(3, recorder)
+
+    assert recorder.max_in_flight > 1, (
+        f"max in-flight was {recorder.max_in_flight}: the wave never overlapped two projects, so "
+        f"the ordering assertion below holds trivially. call sequence={recorder.entries}"
+    )
+    observed = recorder.completion_order()
+    assert observed != _PORTFOLIO_IDS, (
+        f"completion order {observed} equals submission order {_PORTFOLIO_IDS}: nothing reordered, "
+        "so this test would stay green with the barrier's ordering discipline removed"
+    )
+
+    # The contract: the store's verdict SEQUENCE is the surviving projects in submission order —
+    # which is exactly the order ``runs`` carries. Compared as sequences, never sets.
+    assert store_ids == [r.verdict.id for r in result.runs], (
+        f"store order {store_ids} diverged from submission order "
+        f"{[r.verdict.id for r in result.runs]} once a wave member failed (completion order was "
+        f"{observed})"
+    )
+    assert len(set(store_ids)) == 2, (
+        f"the two survivors did not mint distinct verdict ids ({store_ids}), so there is no "
+        "ordering left for this test to pin"
+    )