Artifact/reporting contract cleanup and Phase 6 canonicalization - #11
Merged
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Artifact Paths & Types src/artifact_paths.py, src/trust_identity.py |
Introduce canonical artifact type and filename constants, ARTIFACT_TYPES_DEPRECATED, and measurement_paths(); report_paths() now returns canonical 4‑artifact keys with deprecated aliases; artifact-status logic made migration-aware. |
Report Generation & Campaign Reports src/report.py, src/report_campaign.py, src/rescore.py |
generate_report() now emits campaign-summary.md; campaign report generation retargeted to run-reports.md (ARTIFACT_RUN_REPORTS); artifact index and many fallback labels updated; rescore.py updates messaging to run-reports.md and adds a non‑fatal metadata.json generation call. |
Metadata Export src/export.py |
Add generate_metadata_json(...) and helpers to assemble quantmap-metadata-v1, write metadata.json, compute SHA‑256, and upsert artifact records (best‑effort, non‑fatal); refactor export bundle and summary printing; add standardized sentinel strings and logger. |
DB JSONL Writing & Markers src/db.py |
Refactor write_raw_jsonl() to accept stream and optional merged_path (thread‑safe via _JSONL_WRITE_LOCK); write_jsonl_marker() now forwards stream="marker" and merged_path so markers can be annotated/duplicated. |
Runner & Telemetry Execution src/runner.py, src/telemetry.py |
Consolidate telemetry into a single merged JSONL (raw_telemetry_jsonl_path) with _stream‑annotated records/markers; run writes terminal markers, computes SHA‑256, and registers raw_telemetry_jsonl; TelemetryCollector API updated to accept optional raw_telemetry_jsonl_path. |
Auxiliary Text, Labels & Reports Status src/report_campaign.py, src/trust_identity.py, src/report.py |
Standardize fallback/quality labels (e.g., not recorded, not captured), harden LCB reporting, update provenance/environment rollups, and prefer canonical artifact set when computing completeness. |
Sequence Diagram(s)
sequenceDiagram
participant Runner as Campaign Runner
participant Telemetry as TelemetryCollector
participant Report as Report Generator
participant Export as Metadata Export
participant FS as File System
participant DB as Artifacts DB
Runner->>Telemetry: init(raw_telemetry_jsonl_path)
Runner->>Telemetry: emit samples during cycles
Telemetry->>FS: append sample to raw-telemetry.jsonl ({"_stream":"telemetry", ...})
Runner->>Report: generate_report() -> write campaign-summary.md
Report->>FS: write campaign-summary.md
Report->>DB: upsert artifact campaign_summary_md
Runner->>Report: generate_campaign_report() -> write run-reports.md
Report->>FS: write run-reports.md
Report->>DB: upsert artifact run_reports_md
Runner->>FS: append terminal marker to raw-telemetry.jsonl ({"_stream":"marker", ...})
Runner->>FS: compute SHA-256(raw-telemetry.jsonl)
Runner->>DB: upsert artifact raw_telemetry_jsonl (complete/failed)
Runner->>Export: generate_metadata_json(campaign_id, scores_result, stats)
Export->>DB: read campaign/config/artifact rows
Export->>FS: write metadata.json and compute SHA-256
Export->>DB: upsert artifact metadata_json
Export-->>Runner: return metadata.json path (non-fatal on failure)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | ❓ Inconclusive | The description includes a What/Why summary, scope indicators, validation status, and notes, but lacks explicit coverage of Risk section and specific test validation details required by the template. | Clarify the Risk section (what could this affect?) and explicitly confirm whether tests were added/updated and existing tests passed to complete the template. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely summarizes the main changes: artifact/reporting contract cleanup and Phase 6 canonicalization, which aligns with the core objectives of retiring legacy paths, enforcing canonical constants, and tightening reporting language. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
chore/report-artifact-design
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Pull request overview
This PR completes the Phase 6 “canonical 4-artifact contract” migration by consolidating measurement streams, canonicalizing artifact naming/paths, and updating reporting/export surfaces to treat deprecated artifacts as read-compat only.
Changes:
- Introduces centralized canonical artifact constants/paths and updates runner/reporting code to emit the canonical artifacts (campaign summary, run reports, raw-telemetry stream, metadata.json).
- Retires legacy raw/telemetry JSONL write paths from active flows and updates telemetry collection to write into the merged stream.
- Adds a Phase 6 artifact-contract test suite and relocates documentation into
docs/playbooks/README.md.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test_artifact_contract.py | New tests to enforce the 4-artifact contract and legacy separation. |
| src/artifact_paths.py | Defines canonical artifact type/filename constants and path helpers (plus deprecated aliases). |
| src/db.py | Updates artifacts schema comments and enhances JSONL writing to support merged streams via _stream. |
| src/telemetry.py | Writes telemetry samples into the canonical merged raw-telemetry.jsonl stream. |
| src/runner.py | Removes legacy JSONL creation/writes; registers canonical artifacts and updates campaign listing/reporting flows. |
| src/report.py | Converts “report.md + scores.csv” generation into canonical campaign-summary.md behavior and DB registration. |
| src/report_campaign.py | Renames/reporting canonicalization to run-reports.md and registers it in DB. |
| src/trust_identity.py | Updates campaign report completeness aggregation to prioritize the canonical contract with legacy fallback. |
| src/export.py | Adds metadata.json generation as the 4th formal artifact (structured provenance + scores). |
| rescore.py | Updates rescore flow to regenerate run-reports.md and metadata.json. |
| docs/playbooks/README.md | Adds/moves documentation landing page under playbooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/db.py`:
- Line 859: The function write_jsonl_marker uses the type Any in its signature
but Any is not imported, causing a lint F821; update the module's typing imports
to include Any (e.g., add Any to the existing from typing import Callable import
or add from typing import Any) so the signature def
write_jsonl_marker(jsonl_path: Path, marker_type: str, details: dict[str, Any],
*, merged_path: Path | None = None) -> None: is valid.
In `@src/export.py`:
- Around line 348-363: baseline_raw is only assigned inside the snap_row branch
so if the DB row is missing or an exception occurs generate_metadata_json()
later sees baseline_raw undefined; fix by initializing baseline_raw = {} before
the try block and then keep the existing assignment inside the if snap_row:
block (references: baseline_raw, snap_row, get_connection). Ensure the rest of
the code uses the pre-initialized baseline_raw so model_cfg line works even when
the snapshot lookup fails.
- Around line 459-479: The code builds candidate paths using (meas_dir /
filename) unguarded which raises when meas_dir is None; change the candidate
assignment inside the loop to only do (meas_dir / filename) when art_type ==
ARTIFACT_RAW_TELEMETRY and meas_dir is truthy, otherwise set candidate to None
for missing measurements (and keep using (reports_dir / filename) for report
artifacts); update the subsequent "status" check to rely on candidate being not
None and candidate.exists(); touch the symbols ARTIFACT_RAW_TELEMETRY, meas_dir,
reports_dir, artifact_inventory, and registered_types when making the change.
- Around line 592-599: The metadata export is pulling filters from the wrong
key; update the methodology export to read the saved filter definitions from the
snapshot key by replacing trust_identity.methodology.get("eligibility_filters")
with trust_identity.methodology.get("gates") (i.e., export
trust_identity.methodology.get("gates") into the "eligibility_filters" field),
keeping the surrounding fields (profile_name, profile_version,
methodology_version, source, weights, anchors) unchanged; the relevant symbols
are trust_identity.methodology and load_run_identity which exposes the snapshot
under "gates".
In `@src/report_campaign.py`:
- Around line 239-243: The code sets ac = conf.get("assessment_confidence") or
"not populated" which later feeds the reducer and ends up treated as "high";
change the handling so missing assessment_confidence is represented explicitly
(e.g., None or the literal "not populated") and ensure the downstream reducer
that computes overall_confidence and config_confidences treats that explicit
value as "not populated"/ignored rather than mapping it to "high". Specifically
update the assignment for ac (and any place reading
conf.get("assessment_confidence")) to return a distinct missing marker and
update the reducer logic that consumes confidence_counts/assessment_confidence
to skip or map that marker to a non-high category so legacy/partial run_contexts
don't fall through to "high".
In `@src/runner.py`:
- Around line 2003-2016: The code currently writes the run-separator sentinel to
raw_telemetry_jsonl_path immediately (via write_raw_jsonl with _sentinel), which
can corrupt the canonical stream on no-op exits; change this so the sentinel is
only written once the run is confirmed to proceed (i.e., after the early
no-op/resume checks pass and before measurements start). Locate the
write_raw_jsonl(raw_telemetry_jsonl_path, _sentinel) call and move it (or guard
it behind a boolean like run_will_proceed) to the point where the runner has
determined it will actually execute work (references: write_raw_jsonl,
raw_telemetry_jsonl_path, _sentinel, effective_campaign_id); apply the same
change for the similar block around lines 2442-2496 so no sentinel or terminal
marker is appended on early/no-op exits.
- Around line 1382-1384: The defaulting logic for _eff_environment_dir is wrong
— when environment_dir is omitted it falls back to measurements or STATE_DIR
which prevents generate_campaign_report() from finding canonical environment
artifacts; change the fallback to the canonical environment artifact directory
(use the project constant for the environment artifacts directory, e.g.
ENVIRONMENT_ARTIFACT_DIR or the existing environment artifacts path constant
used by generate_campaign_report) so _eff_environment_dir = environment_dir if
provided else the canonical environment artifact directory (keep
raw_telemetry_jsonl_path and STATE_DIR untouched for other cases).
In `@src/telemetry.py`:
- Around line 1460-1465: The telemetry background writer is appending directly
to self._merged_jsonl_path with open(..., "a") which races with other writers;
change this code to call the centralized append helper
src.db.write_raw_jsonl(...) (or write_raw_jsonl in your db module) instead of
opening the file directly, and implement a shared threading.Lock inside
write_raw_jsonl so all callers (request/marker/telemetry) serialize appends;
ensure write_raw_jsonl also creates the parent directory and writes the JSONL
line for merged_row (including the "_stream": "telemetry" discriminator) so you
can remove the local open(...) block in telemetry.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3803cf0-debc-4141-8b48-84c0cf1bb6b7
⛔ Files ignored due to path filters (1)
test_artifact_contract.pyis excluded by none and included by none
📒 Files selected for processing (10)
docs/playbooks/README.mdrescore.pysrc/artifact_paths.pysrc/db.pysrc/export.pysrc/report.pysrc/report_campaign.pysrc/runner.pysrc/telemetry.pysrc/trust_identity.py
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
src/trust_identity.py (1)
350-381:⚠️ Potential issue | 🟠 MajorLegacy fallback can still mark a campaign complete without any measurement artifact.
The
_LEGACY_TYPEStuple at line 358 only includes("report_md", "report_v2_md", "scores_csv"). A pre-Phase-6 campaign with those three rows but noraw_jsonl/telemetry_jsonlrow will still return"complete", which is a false positive against the canonical 4-artifact contract.Consider including
"raw_jsonl"or"telemetry_jsonl"in the legacy equivalents to ensure measurement artifacts are checked:Suggested fix
- _LEGACY_TYPES = tuple(t for t in ("report_md", "report_v2_md", "scores_csv") if t in ARTIFACT_TYPES_DEPRECATED) + _LEGACY_TYPES = tuple(t for t in ("report_md", "report_v2_md", "scores_csv", "raw_jsonl") if t in ARTIFACT_TYPES_DEPRECATED),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trust_identity.py` around lines 350 - 381, The legacy fallback currently sets _LEGACY_TYPES to ("report_md","report_v2_md","scores_csv") which allows pre-Phase-6 campaigns to be marked complete even when no measurement artifact exists; update the comprehension that builds _LEGACY_TYPES to also include the legacy measurement artifact names ("raw_jsonl" and "telemetry_jsonl") (same pattern as the existing check against ARTIFACT_TYPES_DEPRECATED) so the fallback check_types will require a measurement row when applicable; adjust any references to _LEGACY_TYPES/_NEW_TYPES logic in the function using load_artifact_summaries/by_type/expected_types to ensure measurement artifacts are considered in statuses.src/report_campaign.py (1)
244-248:⚠️ Potential issue | 🟠 MajorMissing confidence still falls through to
high.When
assessment_confidenceis missing, it's stored as"not populated"(line 247). Since this value isn't"low"or"medium", the reducer at lines 288-294 defaults to"high", overstating trust guidance for legacy data.Suggested fix
- ac = conf.get("assessment_confidence") or "not populated" - confidence_counts[ac] = confidence_counts.get(ac, 0) + 1 + ac = conf.get("assessment_confidence") + if ac in {"high", "medium", "low"}: + confidence_counts[ac] = confidence_counts.get(ac, 0) + 1 + else: + confidence_counts["not populated"] = confidence_counts.get("not populated", 0) + 1And at lines 288-294:
+ known_conf_total = sum(confidence_counts.get(k, 0) for k in ("high", "medium", "low")) + if known_conf_total == 0: + overall_confidence = "not populated" - if confidence_counts.get("low", 0) > 0: + elif confidence_counts.get("low", 0) > 0: overall_confidence = "low" elif confidence_counts.get("medium", 0) > total // 2: overall_confidence = "medium" else: overall_confidence = "high",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report_campaign.py` around lines 244 - 248, The current code sets ac = conf.get("assessment_confidence") or "not populated" and then the downstream reducer (the logic around confidence handling that examines confidence_counts at lines ~288-294) treats any value other than "low" or "medium" as "high", so "not populated" is mis-categorized as high; change the reducer to explicitly handle the "not populated" value (or any unknown value) by mapping it to an "unknown"/"not populated" bucket instead of defaulting to "high" (i.e., update the confidence reduction/aggregation logic that reads from confidence_counts/ac to first check for "not populated" or unknown values and assign them to the not-populated/unknown category, otherwise map "low"/"medium"/"high" as before).src/runner.py (1)
2003-2016:⚠️ Potential issue | 🔴 CriticalCritical: Run-separator sentinel written before confirming the run will proceed.
The sentinel is appended to
raw_telemetry_jsonl_pathat line 2016, but the early-exit check for already-complete campaigns happens later at lines 2061-2069. If--no-resumeis used against a complete campaign, the sentinel corrupts the evidence stream for a run that never executes.This concern was raised in a previous review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 2003 - 2016, The run-separator sentinel (_sentinel) is being written to raw_telemetry_jsonl_path via write_raw_jsonl before the early-exit "already-complete" check, which can corrupt evidence when a run is skipped (e.g., --no-resume); move or defer the call to write_raw_jsonl(raw_telemetry_jsonl_path, _sentinel) until after the resume/complete check that inspects effective_campaign_id (the block around the existing early-exit logic), so the sentinel is only written when the run is confirmed to proceed.src/export.py (2)
354-369:⚠️ Potential issue | 🔴 CriticalCritical:
baseline_rawused before definition when snapshot lookup fails.
baseline_rawis assigned only inside theif snap_row:block (line 365), but is referenced unconditionally at line 369. Ifsnap_rowisNoneor an exception occurs,baseline_rawis undefined and causes aNameError.This issue was flagged in a previous review and remains unaddressed.
🐛 Proposed fix
snap: dict = {} + baseline_raw: dict = {} snap_row = None try: with get_connection(db_path) as _conn:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 354 - 369, Initialize baseline_raw before the try/except and avoid referencing it unconditionally after the DB lookup: for example, set baseline_raw = {} prior to calling get_connection and then inside the try block only overwrite it when snap_row is found (where you currently set baseline_raw from yaml.safe_load). Update the except clause to not swallow errors silently (e.g., log or rethrow) and ensure model_cfg = baseline_raw.get("model", {}) uses the pre-initialized baseline_raw; reference variables/functions: baseline_raw, snap_row, snap, get_connection, model_cfg.
471-475:⚠️ Potential issue | 🟠 MajorBug:
meas_dir / filenameraises whenmeas_dirisNone.
find_artifact_dir()can returnNonefor campaigns without a measurements directory. The expressionmeas_dir / filenameat line 473 will raiseTypeErrorwhenmeas_dirisNone.This issue was flagged in a previous review and remains unaddressed.
🐛 Proposed fix
for art_type, filename in { ARTIFACT_CAMPAIGN_SUMMARY: FILENAME_CAMPAIGN_SUMMARY, ARTIFACT_RUN_REPORTS: FILENAME_RUN_REPORTS, ARTIFACT_METADATA: FILENAME_METADATA, ARTIFACT_RAW_TELEMETRY: FILENAME_RAW_TELEMETRY, }.items(): if art_type not in registered_types: - candidate = ( - (meas_dir / filename) if art_type == ARTIFACT_RAW_TELEMETRY - else (reports_dir / filename) - ) + if art_type == ARTIFACT_RAW_TELEMETRY: + candidate = (meas_dir / filename) if meas_dir is not None else None + else: + candidate = reports_dir / filename artifact_inventory.append({ "artifact_type": art_type, "role": ARTIFACT_ROLES.get(art_type, ""), "path": str(candidate) if candidate else None, - "status": "file_present" if (candidate and candidate.exists()) else "not generated", + "status": "file_present" if (candidate is not None and candidate.exists()) else "not generated",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 471 - 475, The current ternary uses meas_dir / filename even when meas_dir may be None (from find_artifact_dir()), causing a TypeError; update the assignment for candidate so it only performs meas_dir / filename when art_type == ARTIFACT_RAW_TELEMETRY and meas_dir is not None, otherwise fall back to reports_dir / filename (or set candidate to None/skip) — change the expression around art_type, meas_dir, filename, ARTIFACT_RAW_TELEMETRY, reports_dir and candidate to explicitly check meas_dir is not None before using the path division.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/export.py`:
- Around line 290-297: The generate_metadata_json function is too large and
complex; refactor by extracting the suggested helper functions to encapsulate
logical sections: implement _build_config_registry(cfg_rows) to assemble config
registry data, _build_ranking_section(scores_result, stats, eliminated) to
produce the ranking/score block, _build_artifact_inventory(artifact_rows,
registered_types, reports_dir, meas_dir) to collect artifact inventories,
_build_environment_summary(baseline_raw, snap, trust_identity) to summarize
environment/trust info, and _build_run_context_summary(db_path, campaign_id,
env_dir) to produce run/context metadata; update generate_metadata_json to call
these helpers, pass only required params, and move corresponding try/except and
local variables into each helper to preserve the "never raises" behavior and
reduce cognitive complexity.
- Around line 366-367: Replace the bare "except Exception: pass" that swallows
errors during snapshot loading/YAML parsing with logging of the exception: catch
Exception as e and call an appropriate logger (e.g., logger.exception(...) or
logging.exception(...)) including context such as the snapshot filename or
metadata being parsed and a clear message; keep the exception re-raising
behavior only if necessary for callers, otherwise log the error and continue.
Locate the exact block containing the "except Exception: pass" in src/export.py
and update it to log the exception details (stack trace) using the module's
logger or the standard logging module.
In `@src/report_campaign.py`:
- Around line 28-31: Extract the duplicated string constants
(_STR_NOT_SET_IN_BASELINE, _STR_NOT_RECORDED, _STR_NOT_CAPTURED) into a single
shared module (e.g., report_constants.py), export them from that module, then
replace the local definitions in report_campaign.py and the other module that
defines them with imports from the new module; update any references that use
those names to import from report_constants so both modules use the same
canonical definitions.
- Around line 496-503: The literal "not in methodology snapshot" is duplicated
in the profile_version/profile_family assignment block; introduce a module-level
constant (e.g. NOT_IN_METHODOLOGY_SNAPSHOT = "not in methodology snapshot") at
the top of the file and replace the three occurrences in the expressions using
methodology.get("profile_version"), getattr(profile_obj, "version", ...), and
the profile_family fallback (used with getattr(getattr(profile_obj,
"experiment_family", None), "value", ...)) to reference that constant instead.
In `@src/runner.py`:
- Around line 1469-1473: The current code uses a conditional expression for side
effects when calling write_jsonl_marker with raw_telemetry_jsonl_path, which is
hard to read; replace the ternary/inline conditional with an explicit if
statement: check if raw_telemetry_jsonl_path is truthy and then call
write_jsonl_marker(raw_telemetry_jsonl_path, "RESTART_CYCLE", {"config_id":
config_id, "cycle_number": cycle_number, "reason": "resume_recovery"}) inside
the block so the call is clear and not performed via a conditional expression.
- Around line 1234-1238: Replace the ternary used for side effects with an
explicit conditional statement: check raw_telemetry_jsonl_path and if truthy
call write_raw_jsonl(raw_telemetry_jsonl_path, result_dict, stream="requests");
this removes the non‑idiomatic expression that discards the function result and
makes the intent clearer (references: write_raw_jsonl, raw_telemetry_jsonl_path,
result_dict, stream="requests").
- Line 2454: Remove the redundant local import statement "import hashlib #
noqa: PLC0415" that appears inside the finally block; rely on the module-level
hashlib import already present and delete that line from the finally block so
there is no duplicate import.
- Line 2009: Replace uses of datetime.now(timezone.utc) with the Python 3.12+
alias datetime.now(datetime.UTC). Specifically update the assignment to
_run_start_iso (and the two other occurrences that call
datetime.now(timezone.utc)) to call datetime.now(datetime.UTC). This keeps the
same behavior but conforms to the preferred alias per Ruff UP017.
---
Duplicate comments:
In `@src/export.py`:
- Around line 354-369: Initialize baseline_raw before the try/except and avoid
referencing it unconditionally after the DB lookup: for example, set
baseline_raw = {} prior to calling get_connection and then inside the try block
only overwrite it when snap_row is found (where you currently set baseline_raw
from yaml.safe_load). Update the except clause to not swallow errors silently
(e.g., log or rethrow) and ensure model_cfg = baseline_raw.get("model", {}) uses
the pre-initialized baseline_raw; reference variables/functions: baseline_raw,
snap_row, snap, get_connection, model_cfg.
- Around line 471-475: The current ternary uses meas_dir / filename even when
meas_dir may be None (from find_artifact_dir()), causing a TypeError; update the
assignment for candidate so it only performs meas_dir / filename when art_type
== ARTIFACT_RAW_TELEMETRY and meas_dir is not None, otherwise fall back to
reports_dir / filename (or set candidate to None/skip) — change the expression
around art_type, meas_dir, filename, ARTIFACT_RAW_TELEMETRY, reports_dir and
candidate to explicitly check meas_dir is not None before using the path
division.
In `@src/report_campaign.py`:
- Around line 244-248: The current code sets ac =
conf.get("assessment_confidence") or "not populated" and then the downstream
reducer (the logic around confidence handling that examines confidence_counts at
lines ~288-294) treats any value other than "low" or "medium" as "high", so "not
populated" is mis-categorized as high; change the reducer to explicitly handle
the "not populated" value (or any unknown value) by mapping it to an
"unknown"/"not populated" bucket instead of defaulting to "high" (i.e., update
the confidence reduction/aggregation logic that reads from confidence_counts/ac
to first check for "not populated" or unknown values and assign them to the
not-populated/unknown category, otherwise map "low"/"medium"/"high" as before).
In `@src/runner.py`:
- Around line 2003-2016: The run-separator sentinel (_sentinel) is being written
to raw_telemetry_jsonl_path via write_raw_jsonl before the early-exit
"already-complete" check, which can corrupt evidence when a run is skipped
(e.g., --no-resume); move or defer the call to
write_raw_jsonl(raw_telemetry_jsonl_path, _sentinel) until after the
resume/complete check that inspects effective_campaign_id (the block around the
existing early-exit logic), so the sentinel is only written when the run is
confirmed to proceed.
In `@src/trust_identity.py`:
- Around line 350-381: The legacy fallback currently sets _LEGACY_TYPES to
("report_md","report_v2_md","scores_csv") which allows pre-Phase-6 campaigns to
be marked complete even when no measurement artifact exists; update the
comprehension that builds _LEGACY_TYPES to also include the legacy measurement
artifact names ("raw_jsonl" and "telemetry_jsonl") (same pattern as the existing
check against ARTIFACT_TYPES_DEPRECATED) so the fallback check_types will
require a measurement row when applicable; adjust any references to
_LEGACY_TYPES/_NEW_TYPES logic in the function using
load_artifact_summaries/by_type/expected_types to ensure measurement artifacts
are considered in statuses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7503bbd-61b4-455d-8de6-9c29eae1f1b0
⛔ Files ignored due to path filters (1)
test_artifact_contract.pyis excluded by none and included by none
📒 Files selected for processing (5)
src/export.pysrc/report.pysrc/report_campaign.pysrc/runner.pysrc/trust_identity.py
…ce cognitive complexity
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/export.py (1)
436-449:⚠️ Potential issue | 🟡 MinorRaw telemetry path falls back to
reports_dirwhenmeas_diris None.When
art_type == ARTIFACT_RAW_TELEMETRYbutmeas_dirisNone, the condition evaluates toFalseand falls through toreports_dir / filename. This placesraw-telemetry.jsonlin the wrong directory. The candidate should beNonefor raw telemetry when no measurements directory exists.Proposed fix
for art_type, filename in canonical_map.items(): if art_type in registered_types: continue - candidate = ( - (meas_dir / filename) if (art_type == ARTIFACT_RAW_TELEMETRY and meas_dir) - else (reports_dir / filename) - ) + if art_type == ARTIFACT_RAW_TELEMETRY: + candidate = (meas_dir / filename) if meas_dir else None + else: + candidate = reports_dir / filename artifact_inventory.append({ "artifact_type": art_type, "role": ARTIFACT_ROLES.get(art_type, ""), - "path": str(candidate) if candidate else None, + "path": str(candidate) if candidate is not None else None, "status": "file_present" if (candidate and candidate.exists()) else "not generated",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 436 - 449, The current candidate selection in the artifact inventory block incorrectly falls back to reports_dir when art_type == ARTIFACT_RAW_TELEMETRY but meas_dir is None; update the logic so that when ARTIFACT_RAW_TELEMETRY and meas_dir is None candidate is set to None (not reports_dir/filename). Modify the candidate assignment around the artifact_inventory.append block (the candidate variable and the conditional that checks art_type, meas_dir, and reports_dir) to explicitly set candidate = (meas_dir / filename) if art_type == ARTIFACT_RAW_TELEMETRY and meas_dir is not None, otherwise set candidate = (reports_dir / filename) if art_type != ARTIFACT_RAW_TELEMETRY and reports_dir is not None, so downstream checks like candidate.exists() handle None correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/export.py`:
- Around line 10-15: The duplicated status string constants
(_STR_NOT_SET_IN_BASELINE, _STR_NOT_IN_SNAPSHOT, _STR_NOT_RECORDED,
_STR_NOT_CAPTURED, _STR_NOT_IN_METHODOLOGY) defined in src/export.py and
src/report_campaign.py should be centralized: create a shared module (e.g.,
src/report_constants.py) that defines these constants, then replace the local
definitions in export.py and report_campaign.py with imports from that module
and remove the duplicated declarations so both files reference the single source
of truth.
- Around line 752-753: The except block handling write failures in src/export.py
currently calls logger.error("metadata.json write failed: %s", write_exc) which
omits the traceback; replace this call with logger.exception("metadata.json
write failed") (or logger.exception("metadata.json write failed: %s",
write_exc)) inside the except Exception as write_exc block so the full stack
trace is logged; locate the handler by the exception variable write_exc in the
export module to make the change.
- Around line 476-479: The except Exception block that sets cycle_rows = [],
total_cycles_db = None, invalid_count = None should log the caught exception at
debug (including the exception message and stacktrace) before continuing; update
the handler in src/export.py to capture the exception as e and call the module
logger (e.g., logger.debug or logging.getLogger(__name__).debug) with a clear
message like "Failed to load cycle DB rows" plus the exception details/traceback
so suppressed DB errors are available for troubleshooting while preserving the
"never raises" behavior.
In `@src/report_campaign.py`:
- Around line 2655-2671: Replace the inline artifact type string
"run_reports_md" in report_campaign.py with the canonical constant
ARTIFACT_RUN_REPORTS from artifact_paths.py: import ARTIFACT_RUN_REPORTS at the
top of the file and use ARTIFACT_RUN_REPORTS in both places where
"run_reports_md" appears (the DELETE execute call and the INSERT execute call)
so the code references the single source-of-truth constant instead of the
literal.
---
Duplicate comments:
In `@src/export.py`:
- Around line 436-449: The current candidate selection in the artifact inventory
block incorrectly falls back to reports_dir when art_type ==
ARTIFACT_RAW_TELEMETRY but meas_dir is None; update the logic so that when
ARTIFACT_RAW_TELEMETRY and meas_dir is None candidate is set to None (not
reports_dir/filename). Modify the candidate assignment around the
artifact_inventory.append block (the candidate variable and the conditional that
checks art_type, meas_dir, and reports_dir) to explicitly set candidate =
(meas_dir / filename) if art_type == ARTIFACT_RAW_TELEMETRY and meas_dir is not
None, otherwise set candidate = (reports_dir / filename) if art_type !=
ARTIFACT_RAW_TELEMETRY and reports_dir is not None, so downstream checks like
candidate.exists() handle None correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1f58222-50e3-4eb7-baf3-41139c93c424
📒 Files selected for processing (2)
src/export.pysrc/report_campaign.py
…eduplicate status loop
There was a problem hiding this comment.
♻️ Duplicate comments (9)
src/export.py (4)
770-771: 🧹 Nitpick | 🔵 TrivialUse
logger.exceptionto include traceback.
logger.erroromits the stack trace. Usinglogger.exceptionautomatically includes the traceback, aiding debugging when writes fail.🔧 Suggested fix
except Exception as write_exc: - logger.error("metadata.json write failed: %s", write_exc) + logger.exception("metadata.json write failed: %s", write_exc),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 770 - 771, In the except block that catches write_exc when writing metadata.json (the except Exception as write_exc handler), replace the logger.error call with logger.exception so the stack trace is captured (e.g., use logger.exception("metadata.json write failed", exc_info=True) or simply logger.exception("metadata.json write failed")). This ensures the traceback for the write_exc is included in logs.
749-749:⚠️ Potential issue | 🟠 MajorExport the methodology filters from the correct snapshot key.
load_run_identity()stores the saved filter definitions undermethodology["gates"], not"eligibility_filters". The current code will always producenullfor this field.🔧 Suggested fix
- "eligibility_filters": trust_identity.methodology.get("eligibility_filters"), + "eligibility_filters": trust_identity.methodology.get("gates"),,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` at line 749, The export currently reads eligibility_filters from trust_identity.methodology.get("eligibility_filters") but load_run_identity() saves the filters under methodology["gates"]; update the exporter to pull the saved filters from the "gates" key (e.g., use trust_identity.methodology.get("gates")) when populating the "eligibility_filters" field so the exported JSON contains the actual saved filters; check the surrounding export construction where "eligibility_filters" is assigned to ensure it uses the "gates" key and preserves None/default behavior if missing.
524-527: 🧹 Nitpick | 🔵 TrivialConsider logging the suppressed database exception.
The
except Exceptionblock silently sets fallback values without logging. While this maintains the "never raises" contract, adding a debug log would aid troubleshooting when cycle data is unexpectedly missing.🔧 Suggested fix
- except Exception: + except Exception as db_exc: + logger.debug("metadata.json: cycle query failed (non-fatal): %s", db_exc) cycle_rows = [] total_cycles_db = None invalid_count = None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 524 - 527, The except block that sets cycle_rows, total_cycles_db, and invalid_count silently swallows exceptions; change "except Exception:" to "except Exception as e:" and log the caught exception before assigning fallbacks. Use the module logger (e.g. logging.getLogger(__name__) or the existing logger variable) and call logger.debug("Error fetching cycle data; using fallbacks", exc_info=True) or logger.exception(...), then keep the assignments to cycle_rows = [], total_cycles_db = None, invalid_count = None so the function still never raises.
337-338:⚠️ Potential issue | 🟡 MinorLog suppressed exceptions for debuggability.
The bare
except Exception: passsilently swallows any error during snapshot loading and YAML parsing. While the "never raises" contract requires catching exceptions, logging at debug level aids troubleshooting when metadata generation produces unexpected results.🔧 Suggested fix
- except Exception: - pass + except Exception as snap_exc: + _logger.debug("metadata.json: snapshot loading failed (non-fatal): %s", snap_exc),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 337 - 338, Replace the silent swallow in the except block that currently reads "except Exception: pass" (the block handling snapshot loading and YAML parsing in src/export.py) with code that captures the exception as a variable (e.g., "except Exception as e") and logs it at debug level without changing the no-raise behavior; for example, call the module logger (or the existing logger used in this file) with a message like "Failed to load snapshot / parse YAML" and include the exception details (use logger.debug(..., exc_info=True) or logger.debug("%s", e, exc_info=True)) so failures remain non-raising but are recorded for troubleshooting.src/runner.py (4)
2456-2457: 🧹 Nitpick | 🔵 TrivialRemove redundant
hashlibimport.
hashlibis already imported at module level (line 50). The re-import inside thefinallyblock is unnecessary.🧹 Suggested fix
try: - import hashlib # noqa: PLC0415 raw_tel_sha = None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 2456 - 2457, Remove the redundant local re-import of hashlib inside the try/finally block (the import hashlib line shown) since hashlib is already imported at module scope; simply delete that import statement in the finally block (in src/runner.py, the try/finally block around the code that currently does "import hashlib # noqa: PLC0415") so the module-level hashlib is used instead.
1234-1238: 🧹 Nitpick | 🔵 TrivialAvoid using conditional expressions for side effects.
The ternary executes a function call for its side effect and discards the result. This is non-idiomatic and harder to read.
🔧 Suggested refactor
- write_raw_jsonl( - raw_telemetry_jsonl_path, result_dict, - stream="requests", - ) if raw_telemetry_jsonl_path else None + if raw_telemetry_jsonl_path: + write_raw_jsonl( + raw_telemetry_jsonl_path, result_dict, + stream="requests", + ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 1234 - 1238, The code uses a ternary conditional to call write_raw_jsonl for its side effect (write_raw_jsonl(raw_telemetry_jsonl_path, result_dict, stream="requests") if raw_telemetry_jsonl_path else None), which is non‑idiomatic; replace it with an explicit if statement that checks raw_telemetry_jsonl_path and then calls write_raw_jsonl(raw_telemetry_jsonl_path, result_dict, stream="requests") to perform the write, keeping the call and arguments identical (refer to write_raw_jsonl, raw_telemetry_jsonl_path, result_dict, stream="requests").
1471-1476: 🧹 Nitpick | 🔵 TrivialSame conditional-expression-for-side-effects pattern.
Same style concern as at lines 1234-1238. Consider using an explicit
ifstatement.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 1471 - 1476, Replace the conditional-expression-for-side-effects with a clear if statement: instead of calling write_jsonl_marker(...) via a ternary expression, check if raw_telemetry_jsonl_path is truthy and then call write_jsonl_marker(raw_telemetry_jsonl_path, "RESTART_CYCLE", {"config_id": config_id, "cycle_number": cycle_number, "reason": "resume_recovery"}). This change should be applied where write_jsonl_marker is imported/used so the side-effect is explicit and easier to read/maintain.
2011-2019:⚠️ Potential issue | 🔴 CriticalSentinel written before confirming run will proceed.
The run-separator sentinel at line 2019 is written unconditionally before the early-exit check at lines 2064-2072 (campaign already complete). A
quantmap run --no-resumeagainst an already-complete campaign will append a separator, then thefinallyblock appends a terminal marker, corrupting the append-only evidence stream for a run that never took measurements.Move the sentinel write to after the early-exit checks, or guard both the sentinel write and the
finallyblock artifact writes with a flag that's only set after confirming the run will proceed.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 2011 - 2019, The sentinel (_sentinel / _run_start_iso) is being written unconditionally via write_raw_jsonl before the early-exit checks, so modify runner.py to only emit the run-separator after confirming the run will proceed: either move the write_raw_jsonl(_sentinel) call to after the early-exit/campaign-complete checks, or introduce a boolean (e.g., run_started_flag) set once the run is confirmed and wrap both the initial sentinel write and the artifact writes in the finally block (the terminal marker emission) with that flag; update references to _sentinel, _run_start_iso, write_raw_jsonl, and the finally-block artifact-write logic accordingly so nothing is appended for runs that exit early.src/trust_identity.py (1)
366-383:⚠️ Potential issue | 🟠 MajorLegacy fallback still permits false-positive "complete" status.
The
_LEGACY_TYPEStuple only contains 3 types (report_md,report_v2_md,scores_csv), excluding the legacy telemetry equivalents (raw_jsonl,telemetry_jsonl). A pre-Phase-6 campaign with those three rows but no telemetry artifact row will still return"complete", which is a false positive against the canonical 4-artifact contract.Consider including the legacy telemetry type:
- _LEGACY_TYPES = tuple(t for t in ("report_md", "report_v2_md", "scores_csv") if t in ARTIFACT_TYPES_DEPRECATED) + _LEGACY_TYPES = tuple(t for t in ("report_md", "report_v2_md", "scores_csv", "raw_jsonl") if t in ARTIFACT_TYPES_DEPRECATED)Alternatively, if legacy campaigns genuinely never had telemetry artifacts registered, this may be acceptable behavior with a documented caveat.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trust_identity.py` around lines 366 - 383, The legacy-type fallback _LEGACY_TYPES currently omits legacy telemetry names so pre-Phase-6 campaigns that have report_md/report_v2_md/scores_csv but lack a telemetry artifact can be mis-reported as "complete"; update the _LEGACY_TYPES construction to include the legacy telemetry artifact names ("raw_jsonl" and "telemetry_jsonl") when they exist in ARTIFACT_TYPES_DEPRECATED (or otherwise ensure telemetry is required) so that the has_any_new/_collect_statuses path correctly checks for telemetry presence as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/export.py`:
- Around line 770-771: In the except block that catches write_exc when writing
metadata.json (the except Exception as write_exc handler), replace the
logger.error call with logger.exception so the stack trace is captured (e.g.,
use logger.exception("metadata.json write failed", exc_info=True) or simply
logger.exception("metadata.json write failed")). This ensures the traceback for
the write_exc is included in logs.
- Line 749: The export currently reads eligibility_filters from
trust_identity.methodology.get("eligibility_filters") but load_run_identity()
saves the filters under methodology["gates"]; update the exporter to pull the
saved filters from the "gates" key (e.g., use
trust_identity.methodology.get("gates")) when populating the
"eligibility_filters" field so the exported JSON contains the actual saved
filters; check the surrounding export construction where "eligibility_filters"
is assigned to ensure it uses the "gates" key and preserves None/default
behavior if missing.
- Around line 524-527: The except block that sets cycle_rows, total_cycles_db,
and invalid_count silently swallows exceptions; change "except Exception:" to
"except Exception as e:" and log the caught exception before assigning
fallbacks. Use the module logger (e.g. logging.getLogger(__name__) or the
existing logger variable) and call logger.debug("Error fetching cycle data;
using fallbacks", exc_info=True) or logger.exception(...), then keep the
assignments to cycle_rows = [], total_cycles_db = None, invalid_count = None so
the function still never raises.
- Around line 337-338: Replace the silent swallow in the except block that
currently reads "except Exception: pass" (the block handling snapshot loading
and YAML parsing in src/export.py) with code that captures the exception as a
variable (e.g., "except Exception as e") and logs it at debug level without
changing the no-raise behavior; for example, call the module logger (or the
existing logger used in this file) with a message like "Failed to load snapshot
/ parse YAML" and include the exception details (use logger.debug(...,
exc_info=True) or logger.debug("%s", e, exc_info=True)) so failures remain
non-raising but are recorded for troubleshooting.
In `@src/runner.py`:
- Around line 2456-2457: Remove the redundant local re-import of hashlib inside
the try/finally block (the import hashlib line shown) since hashlib is already
imported at module scope; simply delete that import statement in the finally
block (in src/runner.py, the try/finally block around the code that currently
does "import hashlib # noqa: PLC0415") so the module-level hashlib is used
instead.
- Around line 1234-1238: The code uses a ternary conditional to call
write_raw_jsonl for its side effect (write_raw_jsonl(raw_telemetry_jsonl_path,
result_dict, stream="requests") if raw_telemetry_jsonl_path else None), which is
non‑idiomatic; replace it with an explicit if statement that checks
raw_telemetry_jsonl_path and then calls
write_raw_jsonl(raw_telemetry_jsonl_path, result_dict, stream="requests") to
perform the write, keeping the call and arguments identical (refer to
write_raw_jsonl, raw_telemetry_jsonl_path, result_dict, stream="requests").
- Around line 1471-1476: Replace the conditional-expression-for-side-effects
with a clear if statement: instead of calling write_jsonl_marker(...) via a
ternary expression, check if raw_telemetry_jsonl_path is truthy and then call
write_jsonl_marker(raw_telemetry_jsonl_path, "RESTART_CYCLE", {"config_id":
config_id, "cycle_number": cycle_number, "reason": "resume_recovery"}). This
change should be applied where write_jsonl_marker is imported/used so the
side-effect is explicit and easier to read/maintain.
- Around line 2011-2019: The sentinel (_sentinel / _run_start_iso) is being
written unconditionally via write_raw_jsonl before the early-exit checks, so
modify runner.py to only emit the run-separator after confirming the run will
proceed: either move the write_raw_jsonl(_sentinel) call to after the
early-exit/campaign-complete checks, or introduce a boolean (e.g.,
run_started_flag) set once the run is confirmed and wrap both the initial
sentinel write and the artifact writes in the finally block (the terminal marker
emission) with that flag; update references to _sentinel, _run_start_iso,
write_raw_jsonl, and the finally-block artifact-write logic accordingly so
nothing is appended for runs that exit early.
In `@src/trust_identity.py`:
- Around line 366-383: The legacy-type fallback _LEGACY_TYPES currently omits
legacy telemetry names so pre-Phase-6 campaigns that have
report_md/report_v2_md/scores_csv but lack a telemetry artifact can be
mis-reported as "complete"; update the _LEGACY_TYPES construction to include the
legacy telemetry artifact names ("raw_jsonl" and "telemetry_jsonl") when they
exist in ARTIFACT_TYPES_DEPRECATED (or otherwise ensure telemetry is required)
so that the has_any_new/_collect_statuses path correctly checks for telemetry
presence as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33d50046-2094-48ec-a694-6eecaa536176
📒 Files selected for processing (3)
src/export.pysrc/runner.pysrc/trust_identity.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runner.py`:
- Around line 438-444: Replace the hardcoded artifact-type strings in the SQL
subqueries with the centralized constants from src.artifact_paths: instead of
using literal 'campaign_summary_md' and 'report_md' in the SELECTs that populate
report_path_new and report_path_legacy, import and interpolate the appropriate
constants (e.g. artifact_paths.CAMPAIGN_SUMMARY_MD and artifact_paths.REPORT_MD
or whatever the exact constant names are) so the SQL uses those values; apply
the same replacement for the other occurrences called out (around the blocks
that currently use 'raw_telemetry_jsonl', 'run_reports_md', etc.) so all
artifact type names are referenced from src.artifact_paths rather than inline
strings.
- Around line 2306-2307: The variables campaign_exit_state and
campaign_exit_detail are set too late and can remain uninitialized if an early
abort occurs after telemetry_stream_started becomes True, causing
UnboundLocalError in the finally block that emits the terminal marker;
initialize campaign_exit_state and campaign_exit_detail to sensible defaults
(e.g., "COMPLETED" and "Normal execution complete.") immediately after or before
telemetry_stream_started is set (or before the separator/pre-loop region) so any
pre-loop abort paths still have defined values, and apply the same
initialization fix for the other finalizer block referenced around the second
occurrence (the region noted at 2447-2455).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9068077b-6da5-4232-ba12-fe6647244dad
⛔ Files ignored due to path filters (1)
test_artifact_contract.pyis excluded by none and included by none
📒 Files selected for processing (2)
src/runner.pysrc/trust_identity.py
Mad-Labs42
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
April 18, 2026 17:30
Stale CodeRabbit change request: all actionable threads from this review have been addressed/resolved, and the current head CodeRabbit status is success.
Mad-Labs42
enabled auto-merge
April 18, 2026 17:57
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/report.py (2)
506-512:⚠️ Potential issue | 🟠 MajorGenerate the artifact table after this file exists and is registered.
_build_markdown()runs beforemd_path.write_text(...)and before theartifactsrow is inserted, so the “Campaign Summary (this file)” row will shownot generatedor stale prior metadata on a fresh generation. The content is then frozen before the DB state is updated. The artifact-status section needs the post-write/post-registration view, not the pre-write one.Also applies to: 518-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.py` around lines 506 - 512, The markdown is being generated before the file is written and before the artifact row is inserted, so move the call to _build_markdown(md_path, ...) to after md_path.write_text(...) and after the code that registers/inserts the artifacts row (the artifact registration code that updates the artifacts table); alternatively, write the file, perform the artifact registration/update, then re-run or rebuild only the artifact-status section (or call _build_markdown again) so the "Campaign Summary (this file)" row reads the post-write/post-registration metadata. Ensure references to md_path, _build_markdown, and the artifact insertion/registration call are used to locate and reorder the operations.
1608-1650:⚠️ Potential issue | 🟠 MajorMap legacy artifact rows into these canonical display slots.
This table only checks canonical
artifact_typekeys. For pre-Phase-6 campaigns that still havereport_v2_md,scores_csv,raw_jsonl, ortelemetry_jsonl, the new summary will render those slots asnot generatedeven thoughsrc/trust_identity.pyalready treats them as valid compatibility equivalents during migration. That breaks backwards-compatible reporting for historical campaigns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.py` around lines 1608 - 1650, The artifact table only looks up exact artifact_type keys via _artifact_rows and so misses legacy names; update the mapping logic that builds _artifact_rows (the comprehension using load_artifact_summaries) to normalize legacy artifact_type values into the canonical constants used later (e.g., map legacy names like "report_v2_md", "scores_csv", "raw_jsonl", "telemetry_jsonl" to ARTIFACT_RUN_REPORTS, ARTIFACT_METADATA, ARTIFACT_RAW_TELEMETRY, etc.), or alternatively consult a compatibility map from src.trust_identity before inserting into _artifact_rows so that _artifact_status(artifact_type, p) finds migrated/legacy rows for each canonical slot. Ensure the mapping uses the same canonical identifiers referenced later (ARTIFACT_CAMPAIGN_SUMMARY, ARTIFACT_RUN_REPORTS, ARTIFACT_RAW_TELEMETRY, ARTIFACT_METADATA) so historical campaigns render correctly.src/export.py (2)
270-270:⚠️ Potential issue | 🟡 MinorTimestamp uses local time, inconsistent with UTC elsewhere.
generate_metadata_jsonat line 659 usesdatetime.now(timezone.utc), but the manifest'sexport_timestamphere usesdatetime.now()without timezone, resulting in local time. This inconsistency can cause confusion when correlating export and metadata timestamps.Proposed fix
- "export_timestamp": datetime.now().isoformat() + "export_timestamp": datetime.now(timezone.utc).isoformat()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` at line 270, The manifest's "export_timestamp" is using datetime.now() (local time) which is inconsistent with generate_metadata_json that uses datetime.now(timezone.utc); update the export_timestamp assignment to use datetime.now(timezone.utc).isoformat() (or equivalent UTC-aware timestamp) and ensure the timezone object is imported where needed so both generate_metadata_json and the manifest use UTC timestamps consistently.
92-147:⚠️ Potential issue | 🟠 MajorResource leak:
src_connnot closed on exception;dest_connleaked if first try fails.Two scenarios leak connections:
- If
dest_connopens successfully butsrc_connfails (lines 93-98),dest_connis never closed before returning.- In the main try block, the exception handler at lines 143-147 only closes
dest_conn, leavingsrc_connopen.Consider using context managers or a
finallyblock to ensure both connections are closed.Proposed fix using try/finally
def _execute_export_bundle( console: object, campaign_id: str, source_db: Path, output_path: Path, lite: bool, strip_env: bool, redaction_root: Path | None, ) -> bool: """Open DB connections, migrate tables, redact, write manifest, and print summary.""" + dest_conn: sqlite3.Connection | None = None + src_conn: sqlite3.Connection | None = None try: dest_conn = sqlite3.connect(output_path) src_conn = sqlite3.connect(source_db) src_conn.row_factory = sqlite3.Row except Exception as e: console.print(f"[red]Error: Database connection failed: {e}[/red]") + if dest_conn: + dest_conn.close() return False try: # ... migration logic ... dest_conn.close() src_conn.close() _print_export_summary(console, output_path, lite, strip_env, redaction_status) return True except Exception as e: - if dest_conn: - dest_conn.close() console.print(f"[bold red]Export Failed:[/bold red] {e}") return False + finally: + if dest_conn: + dest_conn.close() + if src_conn: + src_conn.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 92 - 147, The code leaks DB connections: ensure both dest_conn and src_conn are always closed by replacing the separate try blocks with a single resource-safe pattern—either use context managers (with sqlite3.connect(...) as dest_conn, with sqlite3.connect(...) as src_conn) around the migration block or add a finally that checks and closes both dest_conn and src_conn if they were opened; update the scope where _migrate_with_introspection, _redact_env and _write_manifest are called so they execute while the connections are open and ensure the exception handler only reports the error (closing happens in finally) and gracefully handles None/undefined connection variables.
♻️ Duplicate comments (1)
src/runner.py (1)
1394-1407:⚠️ Potential issue | 🟡 MinorCreate the derived environment directory before using this fallback.
When callers rely on the new
raw_telemetry_jsonl_pathfallback, this block only computes_eff_environment_dir; it never creates it._ctx_path.write_text(...)later will therefore fall into the warning path and skip*_run_context.jsonpersistence, so the Environment section still silently degrades for those callers. Addmkdir(parents=True, exist_ok=True)after selecting_eff_environment_dir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 1394 - 1407, The computed fallback _eff_environment_dir derived from raw_telemetry_jsonl_path is never created, so later _ctx_path.write_text(...) falls into the warning path; fix by creating the directory after selecting _eff_environment_dir (call _eff_environment_dir.mkdir(parents=True, exist_ok=True)) so the environment directory exists before any writes; update the block that sets _eff_environment_dir (referencing variables _eff_environment_dir, raw_telemetry_jsonl_path, measurement_campaign_dir, model_dir, artifacts_root_dir and the later _ctx_path.write_text call) to ensure the directory is created when using the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/artifact_paths.py`:
- Around line 164-173: Replace the hard-coded mapping keys
("campaign_summary_md", "run_reports_md", "metadata_json",
"scores_csv"/deprecated aliases) with the exported constants
ARTIFACT_CAMPAIGN_SUMMARY, ARTIFACT_RUN_REPORTS, ARTIFACT_METADATA and
ARTIFACT_RAW_TELEMETRY respectively so callers using those constants stay in
sync; update both the approved contract block (the dict that returns "dir" and
the artifact entries) and the deprecated-alias block (the aliases that currently
point to campaign_summary/run_reports) to use these constants as the dict keys
while keeping the same values (campaign_summary, run_reports, metadata,
reports_dir / "scores.csv") so behavior is unchanged but keys are driven by the
canonical constants.
In `@src/export.py`:
- Around line 466-476: Replace the explicit for-loop that appends dicts to
artifact_inventory with a single list comprehension that iterates over
artifact_rows and builds each mapping; use
ARTIFACT_ROLES.get(row.get("artifact_type", ""), "not classified") for role and
use row.get("status") or _STR_NOT_RECORDED and row.get("verification_source") or
_STR_NOT_RECORDED for those fields to preserve current fallback behavior; ensure
the comprehension assigns the resulting list back to artifact_inventory and
retains keys "artifact_type", "path", "status", "sha256", "verification_source",
"created_at", and "error_message".
- Line 323: The type hints on _load_campaign_snapshot are unnecessarily quoted;
because the file uses from __future__ import annotations, remove the string
quotes around the return annotation and the db_path parameter so the signature
reads with bare types (e.g., db_path: Path and -> tuple[dict, dict]) and keep
existing imports (Path) and function name _load_campaign_snapshot as-is.
- Around line 779-780: The except block in export.py currently does "except
Exception as write_exc" and then calls logger.exception("metadata.json write
failed: %s", write_exc), which duplicates the exception output; change it to
simply capture the exception implicitly and call logger.exception("metadata.json
write failed") (or remove the "as write_exc" binding) so logger.exception emits
the traceback without the redundant string interpolation; update the except
clause and the logger.exception call that reference "metadata.json write failed"
and "write_exc".
- Around line 662-669: The DB queries using get_connection that assign camp_row
and cfg_rows must be wrapped in a try-except so they don't violate the
function's "Never raises" docstring; catch any exception around the two
_conn.execute calls (querying campaigns and configs with campaign_id), log or
register the partial/failed artifact via the function's existing
error/registration path, and return/exit early in the same way other error
handlers in this function do to avoid propagating the exception.
In `@src/report_campaign.py`:
- Around line 2149-2150: The paragraph referencing a non-existent "Campaign
Artifacts" section and the artifact table mismatch must be fixed: update the
cross-reference text (the string containing "**Raw hardware trace:** ...
`raw-telemetry.jsonl`") to point to the actual section name used elsewhere (or
rename the target section to "Campaign Artifacts" for consistency), and move the
"Database" entry out of the main approved-4-artifact table into the
supporting-files divider so the artifact list matches the approved 4-artifact
contract; locate the strings and tables around the snippet shown and the
artifact table (the approved 4-artifact contract and the "Database" row) and
apply the rename/move consistently in both the primary location and at lines
2237-2258.
In `@src/runner.py`:
- Around line 2446-2453: The KeyboardInterrupt branch sets
campaign_exit_state/campaign_exit_detail and logs/prints but returns without
persisting the final status; modify the interrupt handler to call the same
campaign persistence/update logic used in the generic exception handler (i.e.,
the DB update that runs when Exception exc is caught) before returning so the
campaigns row is updated from 'running' to "INTERRUPTED" for
effective_campaign_id; ensure any error from that update is logged similarly to
the existing exception path.
In `@src/telemetry.py`:
- Around line 1460-1469: The merged JSONL write currently only logs failures;
change this to mirror the SQLite failure path by (1) invoking the same
degradation handler used there (e.g., call the method that marks the
config/campaign degraded — reference the class method used for SQLite failures
such as the existing mark_degraded/_mark_campaign_degraded/_mark_config_degraded
method) when write_raw_jsonl(self._merged_jsonl_path, ...) raises, and (2)
re-raise the exception after marking degraded so the caller sees the failure
(instead of only logger.warning). Update the except block around write_raw_jsonl
to call that degradation method and then raise the exception to ensure the run
is marked degraded and the artifact contract cannot be reported complete.
In `@src/trust_identity.py`:
- Around line 384-393: The current auto-mode branch flips to validate only
_NEW_TYPES when any new row exists (has_any_new), causing partial reports
instead of using legacy-equivalent artifacts; change this to do per-artifact
fallback: when expected_types is None, build check_types by iterating each
canonical new type in _NEW_TYPES and for each include the new type if present in
by_type otherwise include its mapped legacy equivalents (from _LEGACY_TYPES
mapping or a per-type mapping), then call _collect_statuses(check_types,
by_type) as before; update the logic around has_any_new/check_types/statuses
(and preserve use in rescore.py) so validation is determined per-artifact not
globally.
---
Outside diff comments:
In `@src/export.py`:
- Line 270: The manifest's "export_timestamp" is using datetime.now() (local
time) which is inconsistent with generate_metadata_json that uses
datetime.now(timezone.utc); update the export_timestamp assignment to use
datetime.now(timezone.utc).isoformat() (or equivalent UTC-aware timestamp) and
ensure the timezone object is imported where needed so both
generate_metadata_json and the manifest use UTC timestamps consistently.
- Around line 92-147: The code leaks DB connections: ensure both dest_conn and
src_conn are always closed by replacing the separate try blocks with a single
resource-safe pattern—either use context managers (with sqlite3.connect(...) as
dest_conn, with sqlite3.connect(...) as src_conn) around the migration block or
add a finally that checks and closes both dest_conn and src_conn if they were
opened; update the scope where _migrate_with_introspection, _redact_env and
_write_manifest are called so they execute while the connections are open and
ensure the exception handler only reports the error (closing happens in finally)
and gracefully handles None/undefined connection variables.
In `@src/report.py`:
- Around line 506-512: The markdown is being generated before the file is
written and before the artifact row is inserted, so move the call to
_build_markdown(md_path, ...) to after md_path.write_text(...) and after the
code that registers/inserts the artifacts row (the artifact registration code
that updates the artifacts table); alternatively, write the file, perform the
artifact registration/update, then re-run or rebuild only the artifact-status
section (or call _build_markdown again) so the "Campaign Summary (this file)"
row reads the post-write/post-registration metadata. Ensure references to
md_path, _build_markdown, and the artifact insertion/registration call are used
to locate and reorder the operations.
- Around line 1608-1650: The artifact table only looks up exact artifact_type
keys via _artifact_rows and so misses legacy names; update the mapping logic
that builds _artifact_rows (the comprehension using load_artifact_summaries) to
normalize legacy artifact_type values into the canonical constants used later
(e.g., map legacy names like "report_v2_md", "scores_csv", "raw_jsonl",
"telemetry_jsonl" to ARTIFACT_RUN_REPORTS, ARTIFACT_METADATA,
ARTIFACT_RAW_TELEMETRY, etc.), or alternatively consult a compatibility map from
src.trust_identity before inserting into _artifact_rows so that
_artifact_status(artifact_type, p) finds migrated/legacy rows for each canonical
slot. Ensure the mapping uses the same canonical identifiers referenced later
(ARTIFACT_CAMPAIGN_SUMMARY, ARTIFACT_RUN_REPORTS, ARTIFACT_RAW_TELEMETRY,
ARTIFACT_METADATA) so historical campaigns render correctly.
---
Duplicate comments:
In `@src/runner.py`:
- Around line 1394-1407: The computed fallback _eff_environment_dir derived from
raw_telemetry_jsonl_path is never created, so later _ctx_path.write_text(...)
falls into the warning path; fix by creating the directory after selecting
_eff_environment_dir (call _eff_environment_dir.mkdir(parents=True,
exist_ok=True)) so the environment directory exists before any writes; update
the block that sets _eff_environment_dir (referencing variables
_eff_environment_dir, raw_telemetry_jsonl_path, measurement_campaign_dir,
model_dir, artifacts_root_dir and the later _ctx_path.write_text call) to ensure
the directory is created when using the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ede6e302-a89b-4919-8bb2-f53407e2533b
⛔ Files ignored due to path filters (1)
test_artifact_contract.pyis excluded by none and included by none
📒 Files selected for processing (8)
src/artifact_paths.pysrc/db.pysrc/export.pysrc/report.pysrc/report_campaign.pysrc/runner.pysrc/telemetry.pysrc/trust_identity.py
Mad-Labs42
force-pushed
the
chore/report-artifact-design
branch
from
April 18, 2026 18:20
06551c3 to
2e94d6a
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/report.py (1)
1593-1607:⚠️ Potential issue | 🟠 MajorResolve artifact directories with the current model slug, not a campaign-only scan.
_build_markdown()already hasbaseline, but these lookups ignore the resolved model identity and pick whicheverfind_artifact_dir(..., campaign_id)match sorts last. If two models/profiles reuse the same campaign ID, this summary can point its artifact index at another model’srun-reports.md,metadata.json, and telemetry stream.Suggested fix
from src.artifact_paths import ( ARTIFACT_CAMPAIGN_SUMMARY, ARTIFACT_METADATA, ARTIFACT_RAW_TELEMETRY, ARTIFACT_RUN_REPORTS, FILENAME_CAMPAIGN_SUMMARY, FILENAME_METADATA, FILENAME_RAW_TELEMETRY, FILENAME_RUN_REPORTS, + measurement_paths, find_artifact_dir, infer_model_identity, report_paths, ) @@ - report_dir = find_artifact_dir( - effective_lab_root, - "reports", - campaign_id, - ) or (effective_lab_root / "results" / campaign_id) - measurements_dir = find_artifact_dir( - effective_lab_root, - "measurements", - campaign_id, - ) or report_dir + model_cfg = baseline.get("model", {}) if isinstance(baseline.get("model", {}), dict) else {} + model_identity = infer_model_identity( + model_name=model_cfg.get("name"), + model_path=model_cfg.get("path"), + ) + report_dir = report_paths( + effective_lab_root, model_identity, campaign_id, create=False + )["dir"] + if not report_dir.exists(): + report_dir = find_artifact_dir(effective_lab_root, "reports", campaign_id) or ( + effective_lab_root / "results" / campaign_id + ) + measurements_dir = measurement_paths( + effective_lab_root, model_identity, campaign_id, create=False + )["dir"] + if not measurements_dir.exists(): + measurements_dir = find_artifact_dir( + effective_lab_root, "measurements", campaign_id + ) or report_dir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.py` around lines 1593 - 1607, The artifact lookups use find_artifact_dir(..., campaign_id) and thus can return artifacts from a different model that shares the same campaign; update the calls that set report_dir and measurements_dir to include the resolved model identity (e.g., the current baseline’s model slug or resolved_model_slug) instead of (or in addition to) just campaign_id so the paths for _run_reports_md, _metadata_json and _raw_telemetry_jsonl point at the current model’s artifacts; modify the find_artifact_dir invocations (used to compute report_dir and measurements_dir) to pass the model identifier available in the enclosing scope (baseline or resolved model variable) so the directory resolution is model-scoped.
♻️ Duplicate comments (6)
src/telemetry.py (1)
1460-1469:⚠️ Potential issue | 🟠 MajorEscalate merged JSONL write failures the same way SQLite failures are escalated.
Line 1463 now uses the shared append helper, but Lines 1464-1469 still downgrade
raw-telemetry.jsonlwrite failures to warning-only. That leaves one of the canonical artifacts incomplete while the SQLite path below can still succeed, so the run can look healthier than its evidence is. Mirror the SQLite branch here: mark the config degraded via_handle_severity_b(...)and re-raise after logging.Suggested fix
try: # Write to canonical merged stream with _stream discriminator if self._merged_jsonl_path is not None: write_raw_jsonl(self._merged_jsonl_path, row, stream="telemetry") except Exception as exc: - logger.warning( - "Failed to write merged telemetry JSONL to %s: %s", - self._merged_jsonl_path, - exc, - ) + logger.error( + "Severity B: Failed to write merged telemetry JSONL to %s: %s", + self._merged_jsonl_path, + exc, + ) + self._handle_severity_b( + f"[INSTRUMENTATION_FAILURE] merged_jsonl_write_error: {exc}" + ) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telemetry.py` around lines 1460 - 1469, The merged JSONL write currently catches exceptions and only logs a warning; update the except block for the write_raw_jsonl(self._merged_jsonl_path, row, stream="telemetry") call so it mirrors the SQLite error path: call self._handle_severity_b(...) with an appropriate message/identifier to mark the config degraded, log the failure (using logger.warning or logger.error as appropriate) including the exception, and re-raise the exception so callers can see the failure instead of silently continuing; refer to _merged_jsonl_path, write_raw_jsonl, logger.warning, and _handle_severity_b to find and modify the block.src/report_campaign.py (2)
2149-2150:⚠️ Potential issue | 🟡 MinorPoint this to the actual section name.
Lines 2149-2150 send readers to a
Campaign Artifactssection, but this report defines## Supporting Evidence/### Artifact Indexinstead. The current cross-reference still points to a section that does not exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report_campaign.py` around lines 2149 - 2150, Update the cross-reference text that currently points to the non-existent "Campaign Artifacts" section: find the string fragment starting with "> **Raw hardware trace:**" in src/report_campaign.py and replace the reference to "Campaign Artifacts" with the actual section headings used in this report, e.g. "Supporting Evidence" or the exact "Supporting Evidence / Artifact Index" (or "## Supporting Evidence" and "### Artifact Index") so the link points to the correct section.
2237-2260:⚠️ Potential issue | 🟡 MinorThe “4-artifact contract” table still lists five items.
Line 2257 keeps
Databaseinside the formal-contract table, so this section still contradicts its own “approved 4-artifact contract” wording. Move that row below the supporting-files divider with the other non-formal entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report_campaign.py` around lines 2237 - 2260, The table for the "4-artifact contract" currently includes the Database row (the lines.append call that builds the "| Database | `{db_path}` | {_check(db_path)} | ..." row), which contradicts the "approved 4-artifact contract"; remove or relocate that specific lines.append so the Database entry is not inside the formal artifacts table and instead add the same Database row immediately after the lines.append("\n_Supporting files (not formal artifacts):_\n") divider with the other non-formal entries; ensure you preserve the same content string, the call to _check(db_path), and the surrounding table formatting so the formal table remains exactly four rows (campaign_summary_md, run_reports_md, raw_telemetry_jsonl, metadata_json).src/export.py (2)
323-336:⚠️ Potential issue | 🟠 MajorSnapshot payload parsing can still abort
metadata.jsongeneration.
yaml.safe_load()can return a non-dict, andjson.loads(sampling_params_raw)can raise on malformed historical rows. Both cases still flow into a function that promises “Never raises,” so one bad snapshot payload can take out metadata export instead of degrading to sentinel values.Suggested fix
def _load_campaign_snapshot(campaign_id: str, db_path: "Path") -> "tuple[dict, dict]": @@ if snap_row: import yaml # noqa: PLC0415 snap = dict(snap_row) - baseline_raw = yaml.safe_load(snap.get("baseline_yaml_content") or "") or {} + parsed = yaml.safe_load(snap.get("baseline_yaml_content") or "") or {} + baseline_raw = parsed if isinstance(parsed, dict) else {} return snap, baseline_raw @@ def _build_baseline_identity( snap: dict, model_cfg: dict, sources: dict, ) -> dict: @@ sampling_params_raw = snap.get("sampling_params_json") + try: + sampling_params = ( + json.loads(sampling_params_raw) + if sampling_params_raw + else _STR_NOT_IN_SNAPSHOT + ) + except (TypeError, ValueError, json.JSONDecodeError): + sampling_params = _STR_NOT_IN_SNAPSHOT return { @@ - "sampling_params": json.loads(sampling_params_raw) if sampling_params_raw else _STR_NOT_IN_SNAPSHOT, + "sampling_params": sampling_params, }Also applies to: 385-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 323 - 336, The snapshot parsing can raise or return wrong types causing metadata export to fail; in _load_campaign_snapshot ensure the result of yaml.safe_load is a dict (if not, replace with {}) and wrap any yaml import/load in try/except so malformed YAML falls back to {}; likewise locate the code that calls json.loads on sampling_params_raw (around the block referenced at 385-396) and wrap json.loads in try/except returning a safe sentinel (e.g., {}) on JSONDecodeError or if the parsed value is not a dict, so both _load_campaign_snapshot and the sampling params parser never propagate exceptions and always return safe defaults.
661-669:⚠️ Potential issue | 🟠 MajorThe initial DB reads still violate the “Never raises” contract.
These queries run outside any local recovery path, so a missing/corrupt
campaignsorconfigstable still raises before you can emit a partialmetadata.jsonor register a failed artifact row.Suggested fix
- with get_connection(db_path) as _conn: - camp_row = _conn.execute( - "SELECT * FROM campaigns WHERE id=?", (campaign_id,) - ).fetchone() - cfg_rows = _conn.execute( - "SELECT id, variable_value FROM configs WHERE campaign_id=?", - (campaign_id,), - ).fetchall() + try: + with get_connection(db_path) as _conn: + camp_row = _conn.execute( + "SELECT * FROM campaigns WHERE id=?", (campaign_id,) + ).fetchone() + cfg_rows = _conn.execute( + "SELECT id, variable_value FROM configs WHERE campaign_id=?", + (campaign_id,), + ).fetchall() + except Exception as db_exc: + logger.warning("metadata.json: campaign/config query failed: %s", db_exc) + camp_row = None + cfg_rows = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/export.py` around lines 661 - 669, The initial SELECTs using get_connection that populate campaign_row and cfg_rows can raise (e.g., missing/corrupt campaigns or configs tables) before any recovery logic runs; wrap the with get_connection(...) block and the _conn.execute(...) calls in a try/except that catches the DB error (e.g., sqlite3.OperationalError/Exception), and in the except path emit a partial metadata.json and register a failed artifact row for this campaign_id (or call the existing artifact-failure helper) so the failure is recorded instead of propagating; ensure campaign_row and cfg_rows access is guarded after the try so callers don’t assume they exist.src/runner.py (1)
2445-2450:⚠️ Potential issue | 🟠 MajorPersist the interrupted campaign status before returning.
This branch now emits
RUN_INTERRUPTEDin the telemetry stream, but it still returns with thecampaignsrow left asrunning. AfterCtrl+C,list_campaigns()and any completeness logic reading the DB will see stale state.Suggested fix
except KeyboardInterrupt: campaign_exit_state = "INTERRUPTED" campaign_exit_detail = "Interrupted by user" + try: + conn.execute( + "UPDATE campaigns SET status='aborted', failed_at=?, failure_reason=? WHERE id=?", + ( + datetime.now(timezone.utc).isoformat(), + campaign_exit_detail, + effective_campaign_id, + ), + ) + conn.commit() + except Exception as db_exc: + logger.error("Failed to persist interrupted campaign status: %s", db_exc) logger.warning("Campaign %s interrupted by user (KeyboardInterrupt)", effective_campaign_id) console.print("\n[yellow]Interrupted. Progress saved — resume with --resume[/yellow]") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runner.py` around lines 2445 - 2450, When handling KeyboardInterrupt in the except block, persist the interrupted state to the database before returning: set campaign_exit_state = "INTERRUPTED" and campaign_exit_detail = "Interrupted by user" (as already done) and then call the function that updates the campaign row (e.g., update_campaign_status, persist_campaign_state, or whatever function is used elsewhere to save campaign rows) with effective_campaign_id, campaign_exit_state, and campaign_exit_detail so the DB no longer shows the campaign as running; keep the existing logger.warning and console.print calls and only return after the update completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/export.py`:
- Around line 683-684: The measurement and environment artifact lookups use
campaign_id only and should include the resolved model_identity to avoid
cross-model collisions; update the two calls that set meas_dir and env_dir
(currently calling find_artifact_dir(effective_lab_root, "measurements",
campaign_id) and find_artifact_dir(effective_lab_root, "environment",
campaign_id)) to pass model_identity (or a combined identifier using
model_identity and campaign_id) so find_artifact_dir uses the model-scoped path;
ensure the same change is applied wherever artifact_inventory or
run_context_summary are populated from measurements/environment to prevent
pulling data from a different model slug.
In `@src/report.py`:
- Around line 1637-1640: When building the markdown row for "Campaign Summary
(this file)" inside _build_markdown, don't rely on the filesystem fallback that
reports "not generated"; instead detect the known in-flight output (the intended
path report_dir / FILENAME_CAMPAIGN_SUMMARY) and treat it as generated. Update
the code that calls _artifact_status(ARTIFACT_CAMPAIGN_SUMMARY, report_dir /
FILENAME_CAMPAIGN_SUMMARY) (the sections.append block) so that _artifact_status
can accept or check an "in-flight" indicator (e.g., a parameter, or a lookup
into the current build's artifact set like self._in_flight_outputs) and return a
generated status when we know we will write campaign-summary.md during this
_build_markdown run rather than checking disk.
---
Outside diff comments:
In `@src/report.py`:
- Around line 1593-1607: The artifact lookups use find_artifact_dir(...,
campaign_id) and thus can return artifacts from a different model that shares
the same campaign; update the calls that set report_dir and measurements_dir to
include the resolved model identity (e.g., the current baseline’s model slug or
resolved_model_slug) instead of (or in addition to) just campaign_id so the
paths for _run_reports_md, _metadata_json and _raw_telemetry_jsonl point at the
current model’s artifacts; modify the find_artifact_dir invocations (used to
compute report_dir and measurements_dir) to pass the model identifier available
in the enclosing scope (baseline or resolved model variable) so the directory
resolution is model-scoped.
---
Duplicate comments:
In `@src/export.py`:
- Around line 323-336: The snapshot parsing can raise or return wrong types
causing metadata export to fail; in _load_campaign_snapshot ensure the result of
yaml.safe_load is a dict (if not, replace with {}) and wrap any yaml import/load
in try/except so malformed YAML falls back to {}; likewise locate the code that
calls json.loads on sampling_params_raw (around the block referenced at 385-396)
and wrap json.loads in try/except returning a safe sentinel (e.g., {}) on
JSONDecodeError or if the parsed value is not a dict, so both
_load_campaign_snapshot and the sampling params parser never propagate
exceptions and always return safe defaults.
- Around line 661-669: The initial SELECTs using get_connection that populate
campaign_row and cfg_rows can raise (e.g., missing/corrupt campaigns or configs
tables) before any recovery logic runs; wrap the with get_connection(...) block
and the _conn.execute(...) calls in a try/except that catches the DB error
(e.g., sqlite3.OperationalError/Exception), and in the except path emit a
partial metadata.json and register a failed artifact row for this campaign_id
(or call the existing artifact-failure helper) so the failure is recorded
instead of propagating; ensure campaign_row and cfg_rows access is guarded after
the try so callers don’t assume they exist.
In `@src/report_campaign.py`:
- Around line 2149-2150: Update the cross-reference text that currently points
to the non-existent "Campaign Artifacts" section: find the string fragment
starting with "> **Raw hardware trace:**" in src/report_campaign.py and replace
the reference to "Campaign Artifacts" with the actual section headings used in
this report, e.g. "Supporting Evidence" or the exact "Supporting Evidence /
Artifact Index" (or "## Supporting Evidence" and "### Artifact Index") so the
link points to the correct section.
- Around line 2237-2260: The table for the "4-artifact contract" currently
includes the Database row (the lines.append call that builds the "| Database |
`{db_path}` | {_check(db_path)} | ..." row), which contradicts the "approved
4-artifact contract"; remove or relocate that specific lines.append so the
Database entry is not inside the formal artifacts table and instead add the same
Database row immediately after the lines.append("\n_Supporting files (not formal
artifacts):_\n") divider with the other non-formal entries; ensure you preserve
the same content string, the call to _check(db_path), and the surrounding table
formatting so the formal table remains exactly four rows (campaign_summary_md,
run_reports_md, raw_telemetry_jsonl, metadata_json).
In `@src/runner.py`:
- Around line 2445-2450: When handling KeyboardInterrupt in the except block,
persist the interrupted state to the database before returning: set
campaign_exit_state = "INTERRUPTED" and campaign_exit_detail = "Interrupted by
user" (as already done) and then call the function that updates the campaign row
(e.g., update_campaign_status, persist_campaign_state, or whatever function is
used elsewhere to save campaign rows) with effective_campaign_id,
campaign_exit_state, and campaign_exit_detail so the DB no longer shows the
campaign as running; keep the existing logger.warning and console.print calls
and only return after the update completes.
In `@src/telemetry.py`:
- Around line 1460-1469: The merged JSONL write currently catches exceptions and
only logs a warning; update the except block for the
write_raw_jsonl(self._merged_jsonl_path, row, stream="telemetry") call so it
mirrors the SQLite error path: call self._handle_severity_b(...) with an
appropriate message/identifier to mark the config degraded, log the failure
(using logger.warning or logger.error as appropriate) including the exception,
and re-raise the exception so callers can see the failure instead of silently
continuing; refer to _merged_jsonl_path, write_raw_jsonl, logger.warning, and
_handle_severity_b to find and modify the block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2edbacad-40c9-4690-b13b-d9b9ea83783f
⛔ Files ignored due to path filters (1)
test_artifact_contract.pyis excluded by none and included by none
📒 Files selected for processing (7)
src/artifact_paths.pysrc/db.pysrc/export.pysrc/report.pysrc/report_campaign.pysrc/runner.pysrc/telemetry.py
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR finalizes the artifact/reporting cleanup around the canonical 4-artifact contract.
What this does:
Notes:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Reliability