[lewisjared]

Resolves the metric bundle through the reader's artifacts.bundle, using the path recorded on the execution instead of a hardcoded diagnostic.json. An execution with no recorded bundle (a failed one) gets a 404.

GET /executions/ now raises when a group fails to build, instead of logging and leaving it out of the page. Dropping it made total_count disagree with data and hid the failure from the client, and GET /executions/{group_id} already raised for the same group.

Also makes ExecutionOutput's filename, short_name, long_name and description nullable, matching climate-ref, since a null was the most plausible way a group could fail to build. This changes the generated client types, and the figure components now handle the missing values.

Every successful execution in the fixture database records diagnostic.json, so bundle responses are unchanged there. Adds tests for a loaded bundle and the 404.

Summary by CodeRabbit

• Bug Fixes
  • Metric bundle downloads now use the path recorded for each execution and return a clear error when the bundle is unavailable.
  • Execution listings now report an error if an execution group cannot be loaded, rather than silently omitting it.
  • Execution outputs with empty filenames, names or descriptions can now be loaded.
  • Figure captions and filtering handle missing names and descriptions more reliably.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: eac614d7-553b-413e-b790-d3bd65323e53

Commits

Reviewing files that changed from the base of the PR and between 60f6994 and 59cf342.

Files selected for processing: 15

• backend/openapi.json
• backend/src/ref_backend/api/routes/executions.py
• backend/src/ref_backend/api/routes/results.py
• backend/src/ref_backend/core/file_handling.py
• backend/src/ref_backend/models/executions.py
• backend/tests/test_api/test_routes/test_executions.py
• backend/tests/test_api/test_routes/test_results.py
• changelog/173.fix.1.md
• changelog/173.fix.md
• changelog/173.improvement.md
• changelog/173.trivial.md
• frontend/src/client/types.gen.ts
• frontend/src/components/diagnostics/figure.tsx
• frontend/src/components/diagnostics/figureGalleryData.ts
• scripts/generate-client-sdk.sh

Walkthrough

Execution routes now resolve recorded metric bundle paths and propagate execution group build errors. Execution output fields can be null, and frontend diagnostics handle nullable values.

Changes: Execution handling

Layer / File(s)	Summary
Resolve recorded metric bundles backend/src/ref_backend/core/file_handling.py, backend/src/ref_backend/api/routes/executions.py, backend/tests/test_api/test_routes/test_executions.py, changelog/173.fix.md	The route resolves each execution’s recorded bundle path. It returns 404 when the resolved file is unavailable. Tests cover successful retrieval, recorded paths and missing bundles.
Propagate execution group errors backend/src/ref_backend/api/routes/executions.py, backend/tests/test_api/test_routes/test_executions.py, changelog/173.improvement.md	Execution group build errors now propagate instead of being caught and omitted. The test checks that the endpoint returns HTTP 500.
Handle nullable execution output fields backend/src/ref_backend/models/executions.py, frontend/src/client/types.gen.ts, frontend/src/components/diagnostics/figure.tsx, frontend/src/components/diagnostics/figureGalleryData.ts, changelog/173.fix.1.md	The backend model and frontend type allow four output fields to be null. Figure captions, image alt text and figure filtering handle nullable values.

Possibly related PRs

• Climate-REF/ref-app#177: This PR also changes the generated ExecutionOutput API contract.

Change: Bug fix

Merge Risk: Low · previous review 60f69

The changes are mergeable with a bounded accessibility follow-up: figures without a long name need a fallback text alternative. The missing-bundle response assertion is now present.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names two changes: the metric bundle route now reads the recorded bundle path, and the executions list no longer drops groups that fail to build. Both appear in the raw summary and objective…
Docstring Coverage		Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names two changes: the metric bundle route now reads the recorded bundle path, and the executions list no longer drops groups that fail to build. Both appear in the raw summary and objectives. The title is specific and matches the main changes. It uses the conventional commit prefix 'refactor(api):', which is acceptable. It does not mention the nullable ExecutionOutput fields or the frontend figure changes, which is expected for a title.

Full details: Docstring Coverage

Explanation

Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 7 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[coderabbitai]

Nitpick comments: 3

backend/tests/test_api/test_routes/test_executions.py: 2

689-692: Trivial · Maintainability & Code Quality · Quick win

Test the recorded bundle path, not only diagnostic.json.

metric_bundle now resolves the bundle from the path recorded on the execution. The success test uses only fixture data that records diagnostic.json. A route that still reads the fixed diagnostic.json name would return the same file and pass. The change to the recorded path is not covered.

Add a fixture where an execution records a different bundle filename, with that file present in the expected output location. Assert that the endpoint returns that file's contents. Keep the existing 404 test for an execution with no recorded bundle.

---

696-697: Trivial · Maintainability & Code Quality · Quick win

Assert the missing-bundle detail, not only the 404 status.

The test test_metric_bundle_404_without_recorded_bundle checks only status_code == 404. The route returns 404 from two branches. A missing execution, or one that belongs to another group, returns Result not found. A recorded bundle file that is missing returns Metrics bundle not found. The status-only assertion cannot tell these apart. If the lookup regresses to the first branch, the test still passes. Asserting the detail text confirms that the request reaches the bundle branch.

Suggested fix

 def test_metric_bundle_404_without_recorded_bundle(client: TestClient, settings) -> None:
     r = client.get(f"{settings.API_V1_STR}/executions/37/metric_bundle?execution_id=20")
     assert r.status_code == 404
+    assert r.json()["detail"] == "Metrics bundle not found"

backend/src/ref_backend/api/routes/executions.py: 1

140-140: Trivial · Maintainability & Code Quality · Quick win

Add a regression test for a failed group build in GET /executions/.

The list route no longer catches a build failure. A failing group now fails the whole request. No test covers this path, so restoring the former catch-and-omit logic would still pass the current suite. The shared client fixture uses TestClient(app) with default exception handling, so the exception re-raises in the test instead of returning a response. The test must disable that behaviour and assert the 500 response.

🧪 Suggested test

+def test_execution_list_fails_when_a_group_cannot_be_built(app, settings, monkeypatch) -> None:
+    def fail_build(*args, **kwargs):
+        raise RuntimeError("build failed")
+
+    monkeypatch.setattr(executions_routes.ExecutionGroup, "build", fail_build)
+
+    with TestClient(app, raise_server_exceptions=False) as client:
+        r = client.get(f"{settings.API_V1_STR}/executions")
+
+    assert r.status_code == 500

---

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: ca98b268-e13a-45b7-b8f7-9a94fde81403

Commits

Reviewing files that changed from the base of the PR and between 7130f0f and 21fb1f1.

Files selected for processing: 5

• backend/src/ref_backend/api/routes/executions.py
• backend/src/ref_backend/core/file_handling.py
• backend/tests/test_api/test_routes/test_executions.py
• changelog/173.fix.md
• changelog/173.improvement.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[coderabbitai]

Actionable comments posted: 2

Nitpick comments: 2

backend/src/ref_backend/core/file_handling.py: 1

30-30: Trivial · Maintainability & Code Quality · Low value

Add a unit test that passes a keyword argument to resolve_artifact.

The endpoint tests do not cover the **kwargs forwarding. Five tests replace resolve_artifact with monkeypatch. The unmocked test_metric_bundle reaches the real helper, but only with positional arguments. No test passes a keyword argument. If a later change removes **kwargs, a keyword call fails with TypeError, and no test detects it.

The current forwarding is correct. Add a direct test of the real helper:

🧪 Suggested test

from pathlib import Path

from ref_backend.core.file_handling import resolve_artifact


def test_resolve_artifact_forwards_keyword_arguments() -> None:
    def resolve(*, fragment: str, filename: str) -> Path:
        return Path(fragment) / filename

    assert resolve_artifact(resolve, fragment="some", filename="custom.json") == Path("some/custom.json")

frontend/src/components/diagnostics/figureGalleryData.ts: 1

51-52: Trivial · Maintainability & Code Quality · Low value

Add a regression test for null description values.

filterFigures skips null description and filename values before it runs the pattern. No test in figureGalleryData.test.ts uses a null field. If the null check were removed, pattern.test(null) would convert null to "null". A search for null would then match the figure. The current tests would still pass.

Add a figure with a null description and a filename that does not contain null, then assert that the search excludes it:

🧪 Suggested test

   it("keeps the figures whose group matches the selector filters", () => {
     expect(
       filenames(filterFigures(figures, "", { source_id: ["MIROC6"] })),
     ).toEqual(["pr_bias.png"]);
     expect(filterFigures(figures, "tas", { source_id: ["MIROC6"] })).toEqual(
       [],
     );
   });
+
+  it("ignores null metadata when matching text", () => {
+    const nullDescription = collectLatestPlotFigures([
+      group([
+        [
+          {
+            id: nextId++,
+            filename: "lat_bias.png",
+            description: null,
+            output_type: "plot",
+          } as ExecutionOutput,
+        ],
+      ]),
+    ]);
+    expect(filterFigures(nullDescription, "null", {})).toEqual([]);
+  });
 });

The fixture uses an object literal because the output() helper does not accept null. The filename lat_bias.png does not match null, so the assertion depends only on the description guard.

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: a568dffe-4d24-4c35-a192-230b67fbdae5

Commits

Reviewing files that changed from the base of the PR and between 21fb1f1 and 60f6994.

Files selected for processing: 7

• backend/src/ref_backend/core/file_handling.py
• backend/src/ref_backend/models/executions.py
• backend/tests/test_api/test_routes/test_executions.py
• changelog/173.fix.1.md
• frontend/src/client/types.gen.ts
• frontend/src/components/diagnostics/figure.tsx
• frontend/src/components/diagnostics/figureGalleryData.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.