fix(export): replace fabricated results with real experiment data - #124
fix(export): replace fabricated results with real experiment data#124SHA888 wants to merge 5 commits into
Conversation
The `evoseal export` commands (results, variant, all) returned hardcoded sample data instead of querying the actual experiment database. Now all three commands read from ExperimentDatabase (.evoseal/experiments.db): - export_results: looks up experiment by ID, returns real status/metrics/artifacts - export_variant: exports real artifact files from the experiment - export_all: lists all experiments with real metrics Graceful errors when no database exists or experiment not found. Addresses TODO.md: 'evoseal export fabricates results instead of reporting real failures'
|
Warning Review limit reached
Next review available in: 22 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe export commands now read experiment data from ChangesDatabase-backed exports
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant export_results
participant ExperimentDatabase
participant OutputFile
CLI->>export_results: request result export
export_results->>ExperimentDatabase: load experiment and result data
ExperimentDatabase-->>export_results: return persisted data
export_results->>OutputFile: write serialized result
export_results-->>CLI: report completion or missing-data error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
evoseal/cli/commands/export.py (2)
87-134: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the database on every exit path.
db.close()runs only on the success path. If line 94 finds no experiment, the function raisestyper.Exitand leaves the SQLite connection open. The same pattern repeats inexport_variantandexport_all. Usetry/finallyso the connection always closes.♻️ Proposed fix using try/finally
try: db = ExperimentDatabase(db_path) experiment = db.get_experiment(run_id) except Exception as e: typer.echo(f"Error reading experiment database: {e}") raise typer.Exit(1) from None - if experiment is None: - typer.echo(f"Error: No experiment found with ID '{run_id}'") - typer.echo("Use 'evoseal status' to see available experiments.") - raise typer.Exit(1) - - results: dict[str, Any] = { + try: + if experiment is None: + typer.echo(f"Error: No experiment found with ID '{run_id}'") + typer.echo("Use 'evoseal status' to see available experiments.") + raise typer.Exit(1) + + results: dict[str, Any] = { ... - } - - db.close() + } + ... + finally: + db.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evoseal/cli/commands/export.py` around lines 87 - 134, Update the export flow around ExperimentDatabase and the result-building logic so db.close() executes in a finally block on success, database-read errors, and the experiment-not-found typer.Exit path. Apply the same try/finally cleanup pattern to export_variant and export_all, ensuring each database connection is closed exactly once.
301-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the database bootstrap into one helper.
Lines 81-92, 198-209, and 301-312 repeat the same logic: the
.evoseal/experiments.dbliteral, the existence check, the two error messages, the construction, and the error handling. A single_open_experiment_database()helper removes the duplication and centralizes the database location.♻️ Proposed helper
DB_PATH = Path(".evoseal/experiments.db") def _open_experiment_database() -> ExperimentDatabase: """Open the experiment database or exit with a CLI error.""" if not DB_PATH.exists(): typer.echo(f"Error: No experiment database found at {DB_PATH}") typer.echo("Run an evolution cycle first to generate data.") raise typer.Exit(1) try: return ExperimentDatabase(DB_PATH) except Exception as e: typer.echo(f"Error reading experiment database: {e}") raise typer.Exit(1) from None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evoseal/cli/commands/export.py` around lines 301 - 312, Extract the repeated database setup from the commands into a shared _open_experiment_database() helper, using one module-level DB_PATH for .evoseal/experiments.db. Move the existence check, both CLI error messages, ExperimentDatabase construction, and exception handling into that helper, then update the database-using sections around the existing duplicated blocks to call it and retain their command-specific operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evoseal/cli/commands/export.py`:
- Around line 81-85: Add regression coverage for the registered export CLI
commands: `results`, `variant`, and `all`. Test missing-database behavior,
missing experiment/variant path handling where applicable, and
populated-database exports to both stdout and a file, using the existing CLI
test fixtures and invocation patterns.
- Around line 339-355: Normalize format to lowercase at the start of both
export_results and the all-results dispatch flow, then use that normalized value
for validation and branch selection so uppercase formats write the expected
file. Remove the unreachable csv branch from the all-results export path, since
FORMAT_SUPPORT["all"] permits only json and yaml; apply the same normalization
consistently in export_results and its callers.
- Around line 215-245: Enforce output-directory containment for all export paths
in the export command: validate the variant directory derived from variant_id,
each artifact path built in the artifact export loop, and each dependency path
built from dep.name. Resolve each candidate path and require it to remain
relative to the resolved export output directory before creating directories or
writing files, rejecting or skipping invalid paths consistently with the
existing file-helper behavior.
---
Nitpick comments:
In `@evoseal/cli/commands/export.py`:
- Around line 87-134: Update the export flow around ExperimentDatabase and the
result-building logic so db.close() executes in a finally block on success,
database-read errors, and the experiment-not-found typer.Exit path. Apply the
same try/finally cleanup pattern to export_variant and export_all, ensuring each
database connection is closed exactly once.
- Around line 301-312: Extract the repeated database setup from the commands
into a shared _open_experiment_database() helper, using one module-level DB_PATH
for .evoseal/experiments.db. Move the existence check, both CLI error messages,
ExperimentDatabase construction, and exception handling into that helper, then
update the database-using sections around the existing duplicated blocks to call
it and retain their command-specific operations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4592afbd-3f0f-4961-9357-627b7bc8c80a
📒 Files selected for processing (2)
TODO.mdevoseal/cli/commands/export.py
| db_path = Path(".evoseal/experiments.db") | ||
| if not db_path.exists(): | ||
| typer.echo(f"Error: No experiment database found at {db_path}") | ||
| typer.echo("Run an evolution cycle first to generate data.") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find tests exercising the export CLI commands.
set -euo pipefail
fd -t f -e py . tests | xargs rg -n -C3 'export_results|export_variant|export_all' || echo "No test references found"
# Confirm the CLI registers the export subcommand as documented.
fd -t f 'main.py' evoseal/cli --exec rg -n -C3 'export'Repository: SHA888/EVOSEAL
Length of output: 3542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## export command outline"
ast-grep outline evoseal/cli/commands/export.py --view expanded || true
echo
echo "## export.py relevant content"
cat -n evoseal/cli/commands/export.py | sed -n '1,140p'
echo
echo "## export command tests in repo"
rg -n -C3 'evoseal|cli|export|export_results|export_variant|export_all|export_results_file|export_results_format|no .*database|experiment.*not found|experiments.db|VersionDatabase\(' tests || echo "No matches"
echo
echo "## all py tests mentioning export command imports/commands"
rg -n -C2 'export_results|export_variant|export_all|register.*export|app\.commands|commands\.export|from evoseal\.cli\.commands import export|export\.app' tests evoseal || trueRepository: SHA888/EVOSEAL
Length of output: 50373
Add regression tests for the export CLI commands.
evoseal export results, evoseal export variant, and evoseal export all are registered, but the CLI regression tests do not cover them. Add tests for each command over the exported commands: missing database, experiment/variant missing path, and a populated database with both stdout and file output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evoseal/cli/commands/export.py` around lines 81 - 85, Add regression coverage
for the registered export CLI commands: `results`, `variant`, and `all`. Test
missing-database behavior, missing experiment/variant path handling where
applicable, and populated-database exports to both stdout and a file, using the
existing CLI test fixtures and invocation patterns.
Source: Coding guidelines
| if format == "json": | ||
| with open(results_dir / "results.json", "w") as f: | ||
| json.dump({"results": results}, f, indent=2) | ||
| json.dump({"results": results, "count": len(results)}, f, indent=2) | ||
| elif format == "yaml": | ||
| import yaml | ||
|
|
||
| with open(results_dir / "results.yaml", "w") as f: | ||
| yaml.dump({"results": results}, f, default_flow_style=False) | ||
| yaml.dump({"results": results, "count": len(results)}, f, default_flow_style=False) | ||
| elif format == "csv": | ||
| import csv | ||
|
|
||
| with open(results_dir / "results.csv", "w", newline="") as f: | ||
| if results and isinstance(results, list) and len(results) > 0: | ||
| if results: | ||
| fieldnames = list(results[0].keys()) | ||
| writer = csv.DictWriter(f, fieldnames=fieldnames) | ||
| writer.writeheader() | ||
| writer.writerows(results) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize format before dispatch, and drop the unreachable csv branch.
Line 295 validates format.lower(), but lines 339, 342, and 347 compare the raw format. evoseal export all --format JSON passes validation, matches no branch, writes no file, and then line 358 reports success.
Line 347 is also unreachable: FORMAT_SUPPORT["all"] allows only json and yaml, so line 295 rejects csv first. That branch hides a latent defect, because line 352 derives fieldnames from results[0] only, and records are heterogeneous when some experiments have no exp.result.
Either add csv to FORMAT_SUPPORT["all"] and compute the union of keys for fieldnames, or remove the branch.
🐛 Proposed fix
+ fmt = format.lower()
- if format == "json":
+ if fmt == "json":
with open(results_dir / "results.json", "w") as f:
json.dump({"results": results, "count": len(results)}, f, indent=2)
- elif format == "yaml":
+ elif fmt == "yaml":
import yaml
with open(results_dir / "results.yaml", "w") as f:
yaml.dump({"results": results, "count": len(results)}, f, default_flow_style=False)
- elif format == "csv":
- import csv
-
- with open(results_dir / "results.csv", "w", newline="") as f:
- if results:
- fieldnames = list(results[0].keys())
- writer = csv.DictWriter(f, fieldnames=fieldnames)
- writer.writeheader()
- writer.writerows(results)Apply the same normalization at line 137 in export_results, which has the identical mismatch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if format == "json": | |
| with open(results_dir / "results.json", "w") as f: | |
| json.dump({"results": results}, f, indent=2) | |
| json.dump({"results": results, "count": len(results)}, f, indent=2) | |
| elif format == "yaml": | |
| import yaml | |
| with open(results_dir / "results.yaml", "w") as f: | |
| yaml.dump({"results": results}, f, default_flow_style=False) | |
| yaml.dump({"results": results, "count": len(results)}, f, default_flow_style=False) | |
| elif format == "csv": | |
| import csv | |
| with open(results_dir / "results.csv", "w", newline="") as f: | |
| if results and isinstance(results, list) and len(results) > 0: | |
| if results: | |
| fieldnames = list(results[0].keys()) | |
| writer = csv.DictWriter(f, fieldnames=fieldnames) | |
| writer.writeheader() | |
| writer.writerows(results) | |
| fmt = format.lower() | |
| if fmt == "json": | |
| with open(results_dir / "results.json", "w") as f: | |
| json.dump({"results": results, "count": len(results)}, f, indent=2) | |
| elif fmt == "yaml": | |
| import yaml | |
| with open(results_dir / "results.yaml", "w") as f: | |
| yaml.dump({"results": results, "count": len(results)}, f, default_flow_style=False) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 339-339: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(results_dir / "results.json", "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 344-344: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(results_dir / "results.yaml", "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 349-349: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(results_dir / "results.csv", "w", newline="")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evoseal/cli/commands/export.py` around lines 339 - 355, Normalize format to
lowercase at the start of both export_results and the all-results dispatch flow,
then use that normalized value for validation and branch selection so uppercase
formats write the expected file. Remove the unreachable csv branch from the
all-results export path, since FORMAT_SUPPORT["all"] permits only json and yaml;
apply the same normalization consistently in export_results and its callers.
- Sanitize artifact.file_path in export_variant to prevent path traversal (absolute paths or ../ escaping output_dir) by using Path.name only - Wrap all three commands' post-open logic in try/finally to ensure db.close() is reached even on error paths (ExperimentDatabase supports context manager but explicit try/finally is clearer here) - Remove unused 'UTC' import; fix dead txt format branch that referenced deleted keys (results['timestamp'], results['code']) instead of new ones - Fix export_all CSV writer: compute fieldnames as union across all records instead of only first record, and use extrasaction='ignore' to handle optional keys (fitness/generation/execution_time/artifacts) - Verified: ExperimentDatabase API (get_experiment, list_experiments, close, status.value, result.*, metrics, artifacts) matches usage — reviewer's point 5 was incorrect
|
Addressed review feedback in 5a2080e: 1. Path traversal in 2. DB connection leak on error paths — ✅ Fixed. All three commands now use 3. Unused imports — ✅ Fixed, but the reviewer was partially wrong. 4. CSV writer crash on real data — ✅ Fixed. 5. Unverifiable dependency — ❌ Verified as incorrect. I checked |
…metric dedup comment Address code review feedback on PR #124: - Fix path traversal in dependency-artifact write: the main artifact loop sanitized filenames via Path(...).name, but the include_dependencies block used raw dep.name. Since the main loop already writes ALL artifacts with content (including requirements), the redundant dependency block is removed entirely — no more unsafe double-write. - Wrap db.get_experiment() / db.list_experiments() in try/except so corrupt DB or schema mismatch produces a friendly CLI error instead of a raw traceback. - Add comment noting metric dict comprehension silently deduplicates by name. Review point 4 (created_at nullability) is not a real issue: the model defines created_at as non-optional datetime with a default_factory, and the DB schema uses NOT NULL.
Review feedback addressed (pushed)✅ Fixed
❌ Not changed (reviewer was wrong)
|
- Actually filter dependency artifacts when --no-dependencies is passed (was a no-op: the flag existed but all artifacts were exported regardless) - Deduplicate output filenames to prevent silent overwrites when multiple artifacts collapse to the same basename - Extract hardcoded db_path into DEFAULT_DB_PATH module constant - Add clarifying comment that CLI 'variant' maps to Experiment model - Remove dead include_dependencies pass block
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evoseal/cli/commands/export.py`:
- Around line 243-244: Reserve the metadata.json filename in written_names
before the artifact export loop in the export command. Ensure the artifact
naming/deduplication logic skips or renames any sanitized artifact that would
use this reserved name, preserving the later metadata.json write and avoiding
silent overwrites.
- Around line 240-241: Sanitize variant_id before constructing the variant
directory in the export flow, using the same basename-sanitization logic already
applied to artifact names at line 252. Update the code around output_dir and the
variant directory creation so traversal segments cannot escape the requested
output_dir, while preserving the existing variant output and subsequent writes.
- Around line 150-164: The export format handling is inconsistent with
case-insensitive validation, causing valid uppercase formats to skip output. In
evoseal/cli/commands/export.py lines 150-164, assign a normalized lowercase
value and dispatch on it, removing the unreachable txt branch because
FORMAT_SUPPORT["results"] permits only json and csv; in lines 382-390, dispatch
on format.lower() and either add csv to FORMAT_SUPPORT["all"] or remove the
unreachable csv branch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb34197c-66d4-432a-800d-135235a5fbf9
📒 Files selected for processing (1)
evoseal/cli/commands/export.py
| if format == "json": | ||
| output = json.dumps(results, indent=2) | ||
| elif format == "csv": | ||
| # Simple CSV output for metrics | ||
| import csv | ||
| import io | ||
|
|
||
| output_io = io.StringIO() | ||
| writer = csv.writer(output_io) | ||
| writer.writerow(["Metric", "Value"]) | ||
| if include_metrics: | ||
| for k, v in results.get("metrics", {}).items(): | ||
| writer.writerow([k, v]) | ||
| output = output_io.getvalue() | ||
| else: # txt |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Format validation is case-insensitive, but format dispatch is case-sensitive. Both commands validate format.lower() against FORMAT_SUPPORT, then branch on the raw format. An uppercase value such as JSON passes validation and then produces wrong or missing output.
evoseal/cli/commands/export.py#L150-L164: assignfmt = format.lower()and branch onfmt; remove the unreachable txt branch, becauseFORMAT_SUPPORT["results"]allows onlyjsonandcsv.evoseal/cli/commands/export.py#L382-L390: branch onformat.lower()so no valid format silently skips the file write while line 404 reports success; also addcsvtoFORMAT_SUPPORT["all"]or delete the unreachable csv branch.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 150-150: use jsonify instead of json.dumps for JSON output
Context: json.dumps(results, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
📍 Affects 1 file
evoseal/cli/commands/export.py#L150-L164(this comment)evoseal/cli/commands/export.py#L382-L390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evoseal/cli/commands/export.py` around lines 150 - 164, The export format
handling is inconsistent with case-insensitive validation, causing valid
uppercase formats to skip output. In evoseal/cli/commands/export.py lines
150-164, assign a normalized lowercase value and dispatch on it, removing the
unreachable txt branch because FORMAT_SUPPORT["results"] permits only json and
csv; in lines 382-390, dispatch on format.lower() and either add csv to
FORMAT_SUPPORT["all"] or remove the unreachable csv branch.
- Sanitize variant_id with Path().name before using in output_dir to prevent path traversal via segments like '../' - Pass encoding='utf-8' to both write_text calls in export_variant (artifact content and metadata.json) for consistency with export_results and to avoid corruption on non-UTF-8 default locales - Also includes: env var/db-path overrides, metadata.json dedup reservation, artifacts join for CSV, shutil cleanup
Review feedback addressed (commit c20b7fd)Verified each point against the actual repo state: ✅ Fixed — Missing explicit encoding in
|
1 participant
What
The
evoseal exportcommands (results,variant,all) returned hardcoded sample data instead of querying the actual experiment database. This PR wires all three commands to read fromExperimentDatabase.Changes
evoseal/cli/commands/export.py.evoseal/experiments.db, returns real status, result metrics (best_fitness, generations_completed, execution_time, etc.), and artifact contentAll three commands now:
ExperimentDatabaseAPITODO.mdevoseal exportitemVerification
Addresses
TODO.md P1:
evoseal exportfabricates results instead of reporting real failuresSummary by CodeRabbit