feat(evals): #1980 spine -- scoring + data contract for golden-company benchmark

[Aureliolo]

Part of #1979. Lands the scoring + data layer of the golden-company benchmark (eval spine), as documented in the Status (2026-05-20) section of the linked issue.

This PR intentionally does not close #1980. The runner (cassette guard + telemetry collector + orchestrator + CLI + baselines + recorded cassette + make benchmark target) is held until a runtime prerequisite gap closes. See "Prerequisites for the runner PR" in #1980 for the full follow-up plan.

What landed

The complete scoring + data contract for the eval spine, 68 unit tests passing:

• evals/models/{brief,scorecard}.py -- frozen Pydantic v2 models, schema-versioned (SCORECARD_SCHEMA_VERSION = 1).
• evals/loader/{briefs,anchors}.py -- YAML loaders via synthorg.api.boundary.parse_typed; path-traversal containment on anchor-set loading.
• evals/scoring/executable.py -- subprocess grader (hidden / build / lint, weighted 60/20/20); rejects shell metacharacters at load time as defence-in-depth.
• evals/scoring/judged.py + spearman.py -- ordinal calibration with a Spearman rho gate (>= 0.7); refuses to score when the judge's ordering disagrees with the hand-scored anchor set.
• evals/scoring/penalties.py + aggregate.py -- process-fact penalty table with cap + floor.
• evals/emit/{json_writer,markdown_writer}.py -- deterministic JSON + Markdown emitters.
• evals/errors.py -- DomainError-rooted hierarchy.
• pyproject.toml -- evals/ wired for ruff (src), pyright (extraPaths), pytest (pythonpath), isort (known-first-party).
• CLAUDE.md -- layout line now mentions the new evals/ top-level directory.

Pydantic models carry cross-field invariant validators so the scorecard is auditable from any single field: BriefResult enforces score == max(grade - deduction, GRADE_FLOOR); JudgeCalibrationReport enforces passed == (rho >= gate); PenaltyEntry enforces raw == points_per_event * count and applied <= raw; AggregatedProcessFacts enforces total_events == sum(events_by_class); ProcessFactReport enforces entries match the tracked events_by_class slice; CheckOutcome ties timed_out=True to the POSIX timeout sentinel; ExecutableGrade enforces pass-flags match per-label outcomes; ScriptedJudge requires non-empty responses or default scores; JudgedGrade refuses construction with a failing calibration; AnchorItem rejects hand_scores outside [0.0, 1.0]; HiddenCheckSpec rejects shell metacharacters in cmd[0].

What is NOT yet built (follow-up PR scope)

• evals/runner/cassette_guard.py -- cassette SHA256 + REPLAY enforcement.
• evals/runner/telemetry_collector.py -- structlog event bucketing.
• evals/runner/orchestrator.py -- per-brief boot/submit/wait/collect loop.
• evals/run.py + evals/__main__.py -- CLI entry point.
• evals/baselines/{reference,broken}.yaml, evals/briefs/*.yaml, evals/cassettes/reference_run.cassette.json.
• Makefile target + .gitignore.
• The acceptance test test_runner_broken_scores_worse.py going green (currently uses pytest.importorskip("evals.run")).

Why the runner did NOT land in this PR

A runtime spike (create_app() + submit one TaskRequirement via the IntakeEngine) confirmed the studio's agent-execution path is not yet wired end-to-end. Intake transitions fire (intake.request.received -> triaging -> scoped), then TaskEngineNotRunningError raises. No BUDGET_* / EXECUTION_* / STAGNATION_* / APPROVAL_GATE_* events appear. Without those events, broken.yaml and reference.yaml would produce identical empty process-fact traces and the scorer could not discriminate. Per project rule "admit you cannot in plain words, BEFORE": surfacing this gap rather than shipping a green acceptance test that asserts nothing.

Prerequisites tracked in #1980's Status section.

Test plan

uv run python -m pytest tests/evals_spine/ -m unit

Result locally: 68 passed in 9.13s.

uv run ruff check src/ tests/ evals/
uv run ruff format --check src/ tests/ evals/
uv run mypy --num-workers=4 tests/evals_spine/ evals/

All green. Pre-push hooks (affected mypy + pytest, ruff, em-dash gate, review-origin gate, migration-framing gate, secret detection, etc.) ran on the push and passed.

Review coverage

/pre-pr-review ran 9 agents in parallel against the spine. 22 findings consolidated in _audit/pre-pr-review/triage.md:

• 2 CRITICAL: path-traversal containment, duplicate SCORECARD_SCHEMA_VERSION -- both fixed.
• 5 MAJOR valid (wrong exception type, three issue back-refs in docstrings, missing evals/ in CLAUDE.md layout) -- all fixed.
• 1 MAJOR invalid (PEP 758 syntax: agent misread CLAUDE.md's "no parens unless binding"; we ARE binding as exc, parens required) -- rejected with rationale.
• 11 MEDIUM valid (function-length refactor, shell-metachar guard, test-quality cleanups, 4 cross-field validators) -- all fixed.
• 3 LOW deferred -- 2 implemented (extra validators on trusted-construction types, JudgeProtocol docstring polish); 1 deferred (Brief discriminated union -- the review agent itself flagged it as a pragmatic trade-off and the current post-validator catches the same XOR errors; restructuring would force a substantial test refactor for marginal benefit).

Net: 19 of 22 review findings applied in this PR, with rationale recorded in the commit body of 815ac88f and in the triage file.

Related

• Part of EPIC: Best-in-class operate tier #1979 (operate-tier EPIC).
• Depends on Client-simulation runtime live (= e2e test harness) #1961 (client-simulation harness, already in place) + EPIC EPIC: Autonomous product studio substrate #1973 (runnable studio, partial -- runtime gap blocks the runner PR) + Agent runtime online + minimal safety spine (runtime root) #1956.
• Blocks Learning loop + curve #1983 (learning curve), Continual-improvement loop (finetuning done right) #1990, Self-extending toolkit #1995, Stakes-aware model routing #1998 -- all consume the scorecard schema landing here.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request establishes the core scoring and data infrastructure for the 'golden-company' benchmark (eval spine). It provides the foundational models, loaders, and grading logic required to evaluate synthetic organization performance, while intentionally deferring the runner implementation until runtime prerequisites are met. The changes include strict schema validation, robust error handling, and comprehensive unit testing to ensure the benchmark's reliability and auditability.

Highlights

• Data Model and Contract: Introduced frozen Pydantic v2 models for the eval spine, including schema-versioned Scorecard and Brief definitions to ensure deterministic data contracts.
• Scoring and Calibration: Implemented a subprocess-based grader for executable briefs and a calibrated LLM judge for judged briefs, featuring a Spearman rho gate (>= 0.7) to ensure judge reliability.
• Penalty System: Added a process-fact penalty table with caps and floors to account for budget, execution, and governance events during benchmark runs.
• Emitters and Loaders: Added deterministic JSON and Markdown emitters for scorecards, along with robust YAML loaders that include path-traversal containment for anchor sets.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[coderabbitai]

Warning

Rate limit exceeded

@Aureliolo has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 59 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: b64c3097-b7fb-4ae2-8214-384842b0aeb4

📥 Commits

Reviewing files that changed from the base of the PR and between e57d7a0 and 2c25a0a.

📒 Files selected for processing (2)

• evals/models/scorecard.py
• tests/evals_spine/test_scorecard_emit.py

Walkthrough

Adds an eval spine implementing frozen Pydantic briefs and scorecard models, YAML loaders (briefs and anchors) with path-safety and validation, executable and judged graders (Spearman-based calibration), penalty table and aggregation logic, JSON and Markdown scorecard emitters, an EvalError hierarchy, observability event constants, pyproject tooling/test discovery updates, and a comprehensive suite of unit and integration tests.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: landing the scoring and data contract (spine) for the golden-company benchmark eval system.
Description check	✅ Passed	The description comprehensively explains what landed (scoring + data layer with 68 passing tests), what was intentionally deferred (runner components), and why (runtime prerequisites gap).
Linked Issues check	✅ Passed	The PR successfully implements the primary coding objectives from #1980: frozen Pydantic models, YAML loaders, executable grading, judged calibration with Spearman gate, penalties + aggregation, JSON/Markdown emitters, and error hierarchy.
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue #1980's first-PR objectives. The runner components (cassette_guard, orchestrator, CLI, baselines, cassettes) are intentionally excluded per the issue status.
Docstring Coverage	✅ Passed	Docstring coverage is 52.78% which is sufficient. The required threshold is 40.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codspeed-hq]

Merging this PR will not alter performance

✅ 33 untouched benchmarks
⏩ 21 skipped benchmarks¹

---

_{Comparing feat/1980-golden-benchmark (2c25a0a) with main (f60e9ed)}

Footnotes

1. 21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[coderabbitai]

Actionable comments posted: 13

🤖 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 `@evals/loader/briefs.py`:
- Around line 43-47: The globbed entries in files are currently trusted by
suffix only, which allows symlinks or non-file entries to escape briefs_dir or
cause I/O errors; update the logic around files / the for path in files loop to
skip any path that is not a regular file and to ensure path.resolve() is
contained within briefs_dir.resolve() (e.g., check path.is_file() and that
path.resolve() is under briefs_dir.resolve() via relative_to in a try/except)
before calling path.read_text() and yaml.safe_load; apply these checks where
files is built or at the start of the loop that constructs briefs (referencing
variables briefs_dir, files, path, yaml.safe_load, and Brief).

In `@evals/models/brief.py`:
- Line 111: Replace the inline numeric literal in the model schema by
introducing a named constant in the settings.definitions module (e.g.,
HIDDEN_CHECK_DEFAULT_TIMEOUT: int = 30) and use that constant in the Field
default for timeout_seconds in the Brief model (timeout_seconds: int =
Field(default=HIDDEN_CHECK_DEFAULT_TIMEOUT, gt=0)); add an explicit module-level
type annotation for the new constant in settings.definitions and update any
imports so evals.models.brief references the constant instead of the literal.

In `@evals/models/scorecard.py`:
- Around line 123-153: BriefResult currently allows inconsistent states between
kind and judge_calibration; add an after model_validator (e.g. name it
_validate_kind_and_calibration) on the BriefResult model that checks
BriefResult.kind (use the BriefKind enum) and enforces that when kind ==
BriefKind.JUDGED then judge_calibration is not None, and when kind !=
BriefKind.JUDGED then judge_calibration is None; on violation raise a ValueError
with a clear message referencing brief_id, kind and judge_calibration to aid
debugging.
- Around line 164-166: AggregatedProcessFacts currently allows negative counts
in events_by_class; add a Pydantic validator on the AggregatedProcessFacts model
(e.g., `@validator`("events_by_class") or a `@root_validator`) that iterates the
dict values and raises a ValueError if any count is negative, and ensure
total_events remains Field(..., ge=0) as a non-negative guard; reference the
AggregatedProcessFacts.events_by_class field and the model's validation methods
when implementing.

In `@evals/scoring/aggregate.py`:
- Line 32: The field event_constant in the Pydantic model should be typed as
NotBlankStr rather than plain str; update the annotation for event_constant to
NotBlankStr, add the appropriate import for NotBlankStr from the project
validation utilities (or pydantic helpers) at the top of
evals/scoring/aggregate.py, and run typechecks to ensure the model now enforces
non-blank identifier constraints.
- Around line 122-128: In the loop in evals/scoring/aggregate.py that iterates
over events_by_class.items(), validate the `count` before using it: if `count <
0` raise a ValueError with a clear message identifying the offending
`event_constant` and `count`; then proceed to call `penalty_table.is_tracked`,
`penalty_table.points_for(event_constant)`, compute `raw = per_event * count`,
clamp with `penalty_table.cap_per_class`, and add to `total_deduction` as
before. Ensure the check occurs before any arithmetic so negative counts cannot
produce negative deductions.
- Around line 72-75: Replace the `@property` on the is_clean method with
Pydantic's `@computed_field`: import computed_field from pydantic and annotate
is_clean with `@computed_field`, keeping the implementation returning
self.deduction == 0 so the derived value complies with the frozen Pydantic model
contract; ensure the decorator is applied to the is_clean method in the same
class where deduction is defined.

In `@evals/scoring/executable.py`:
- Around line 151-157: Replace the inline event name string literals used in the
logger.warning calls with centralized constants: define EVAL_EXECUTABLE_TIMEOUT
and EVAL_EXECUTABLE_TOOL_MISSING in the observability.events.evals module (or
the existing events module) with values "evals.executable.timeout" and
"evals.executable.tool_missing" respectively, then import those constants at the
top of this module and use them in the logger.warning calls (the ones that
currently pass "evals.executable.timeout" and "evals.executable.tool_missing");
update both occurrences where logger.warning is invoked (the calls that pass
cmd=spec.cmd, timeout=spec.timeout_seconds, error_type=type(exc).__name__,
error=safe_error_description(exc) and the similar tool-missing call) to
reference the imported constants instead of string literals.
- Around line 102-105: Replace the `@property` on the is_clean method with
Pydantic's `@computed_field` decorator so the derived value is serialized; import
computed_field from pydantic if missing and keep the same signature (def
is_clean(self) -> bool) and return expression (self.hidden_pass and
self.build_pass and self.lint_pass) so the field is treated as a computed model
field rather than a plain Python property.
- Line 67: Change the Pydantic field annotation for the identifier "label" from
str to NotBlankStr (so it enforces non-empty identifiers like LABEL_HIDDEN,
LABEL_BUILD, LABEL_LINT) and add an import for NotBlankStr from the project's
shared types module (where other models use NotBlankStr); update the model
definition that contains the "label" field in this file to use NotBlankStr
instead of str.
- Around line 131-143: The completed outcome handler currently hardcodes
duration_seconds=0.0 in _outcome_from_completed while timeouts set
duration_seconds=float(spec.timeout_seconds); fix by capturing real elapsed time
around the subprocess invocation and threading that value into the CheckOutcome:
modify the code that calls subprocess.run to record start/end (e.g.,
time.perf_counter()) and compute elapsed_seconds, change _outcome_from_completed
to accept a duration_seconds: float parameter (or overload it) and use that
value when constructing CheckOutcome, and ensure the timeout branch still sets
duration_seconds=float(spec.timeout_seconds) so all outcomes have meaningful
durations.

In `@tests/evals_spine/test_judged_calibration.py`:
- Line 37: Change the predicate parameter's type from object to a typed Callable
to reflect its true signature: add "from typing import Callable" at the top of
the test file, update the function signature(s) that accept the predicate (the
pytest.parametrize entry ("xs", "ys", "predicate") and the test function(s)
using that parameter) to use predicate: Callable[[float], bool], and remove the
now-unnecessary "# type: ignore[operator]" comments; apply the same change to
the other occurrence around lines 80-83 where predicate is used.

🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: eed15755-dcc6-4950-9350-66b34d428804

📥 Commits

Reviewing files that changed from the base of the PR and between 2049885 and 815ac88.

📒 Files selected for processing (27)

• CLAUDE.md
• evals/__init__.py
• evals/emit/__init__.py
• evals/emit/json_writer.py
• evals/emit/markdown_writer.py
• evals/errors.py
• evals/loader/__init__.py
• evals/loader/anchors.py
• evals/loader/briefs.py
• evals/models/__init__.py
• evals/models/brief.py
• evals/models/scorecard.py
• evals/scoring/__init__.py
• evals/scoring/aggregate.py
• evals/scoring/executable.py
• evals/scoring/judged.py
• evals/scoring/penalties.py
• evals/scoring/spearman.py
• pyproject.toml
• tests/evals_spine/__init__.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: Build Backend
• GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
• GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
• GitHub Check: Dashboard Test
• GitHub Check: Test Integration
• GitHub Check: Test E2E
• GitHub Check: Test Unit
• GitHub Check: Analyze (python)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Read docs/design/ page before implementing; deviations need approval. See DESIGN_SPEC.md.

Present every plan for accept/deny before coding.

Ship enforcement gate with every convention PR.

Configuration Precedence: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); Cat-3 bootstrap secrets are pure env at the boot site. No os.environ.get outside startup.

Numerics live in settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final|Final[int]|Final[float] = literal). Enforced by scripts/check_no_magic_numbers.py.

No from __future__ import annotations (3.14 has PEP 649). PEP 758 except: except A, B: must use parens unless binding.

Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.

Errors: <Domain><Condition>Error from DomainError; never inherit Exception/RuntimeError/etc directly. Enforced by check_domain_error_hierarchy.py.

Pydantic v2 frozen + extra="forbid" on every frozen model (gate check_frozen_model_extra_forbid.py; @computed_field auto-exempt, per-line # lint-allow: frozen-extra-forbid -- <reason> for extra="allow"/"ignore"); use @computed_field for derived fields.

Args models at every system boundary; use parse_typed() for every external dict ingestion. Enforced by check_boundary_typed.py.

Immutability: use model_copy(update=...) or copy.deepcopy(); deepcopy at system boundaries.

Async: use asyncio.TaskGroup for fan-out/fan-in; helpers catch Exception (re-raise MemoryError/RecursionError).

Clock seam: clock: Clock | None = None; tests inject FakeClock. Lifecycle: services own _lifecycle_lock; timed-out stops mark unrestartable.

Untrusted content (SEC-1): use wrap_untrusted() from engine.prompt_safety; use HTMLParseGuard for HTML.

Logging: import `from sy...

Files:

• evals/__init__.py
• evals/scoring/__init__.py
• evals/emit/json_writer.py
• evals/loader/__init__.py
• evals/errors.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_brief_loader.py
• evals/loader/briefs.py
• evals/models/scorecard.py
• evals/scoring/spearman.py
• tests/evals_spine/test_executable_grader.py
• evals/emit/markdown_writer.py
• evals/scoring/aggregate.py
• evals/models/__init__.py
• evals/emit/__init__.py
• evals/loader/anchors.py
• evals/scoring/judged.py
• evals/scoring/penalties.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• evals/models/brief.py
• tests/evals_spine/test_scorecard_emit.py
• evals/scoring/executable.py

**/*.{md,rst}

📄 CodeRabbit inference engine (CLAUDE.md)

Numerics in README and public docs sourced from data/runtime_stats.yaml via <!--RS:NAME--> markers.

Files:

• CLAUDE.md

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use markers: @pytest.mark.{unit,integration,e2e,slow}. Async tests use auto. Global timeout 30s. Coverage minimum 80%.

xdist: use -n 8 --dist=loadfile (prevents 3.14+ Windows ProactorEventLoop leak). Windows: use WindowsSelectorEventLoopPolicy for unit tests; subprocess tests override back.

Test doubles: FakeClock for Clock seam, mock_of[T](**overrides) for typed-boundary substitutions, SimpleNamespace for attribute-bags. Bare MagicMock at typed boundary is blocked by check_mock_spec.py.

FakeClock and mock_of import from tests._shared; inject via clock= and the helper's spec subscript.

Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add @example(...)). Never skip/xfail flaky tests; fix fundamentally.

Files:

• tests/evals_spine/conftest.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_scorecard_emit.py

---

⚙️ CodeRabbit configuration file

Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare @settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which @given() honors automatically.

Files:

• tests/evals_spine/conftest.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_scorecard_emit.py

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: No region/currency/locale privileged; use metric units; British English.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: Timeout/slow test failures are source-code regressions; never edit `tests/baselines/unit_timing.json` or scripts baseline files. Bypass with `ALLOW_BASELINE_GROWTH=1 git commit ...` (requires user approval).

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review`. After PR: `/aurelio-review-pr` for external feedback. Fix EVERYTHING valid; no deferring.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`).

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: Branches: `<type>/<slug>` from main.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: After every squash merge → run `/post-merge-cleanup`.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API.

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T16:02:13.510Z
Learning: API startup lifecycle: two phases - construction (synchronous service wiring) and on_startup (connected persistence wiring). Construction ordering: `agent_registry` BEFORE `auto_wire_meetings`; `tunnel_provider` unconditional. On-startup ordering: `SettingsService` BEFORE `WorkflowExecutionObserver`; `OntologyService` AFTER `persistence.connect()`.

📚 Learning: 2026-05-05T09:04:46.195Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.

Applied to files:

• evals/__init__.py
• evals/scoring/__init__.py
• evals/emit/json_writer.py
• evals/loader/__init__.py
• evals/errors.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_brief_loader.py
• evals/loader/briefs.py
• evals/models/scorecard.py
• evals/scoring/spearman.py
• tests/evals_spine/test_executable_grader.py
• evals/emit/markdown_writer.py
• evals/scoring/aggregate.py
• evals/models/__init__.py
• evals/emit/__init__.py
• evals/loader/anchors.py
• evals/scoring/judged.py
• evals/scoring/penalties.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• evals/models/brief.py
• tests/evals_spine/test_scorecard_emit.py
• evals/scoring/executable.py

📚 Learning: 2026-05-16T18:36:31.446Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.

Applied to files:

• CLAUDE.md

📚 Learning: 2026-05-16T18:36:35.250Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).

Applied to files:

• CLAUDE.md

🔇 Additional comments (21)

tests/evals_spine/test_scorecard_emit.py (1)

1-255: LGTM!

tests/evals_spine/test_penalties.py (1)

1-182: LGTM!

evals/scoring/spearman.py (1)

1-97: LGTM!

evals/scoring/judged.py (1)

1-298: LGTM!

tests/evals_spine/conftest.py (1)

1-59: LGTM!

tests/evals_spine/test_brief_loader.py (1)

1-144: LGTM!

tests/evals_spine/test_runner_broken_scores_worse.py (1)

1-87: LGTM!

evals/scoring/__init__.py (1)

1-13: LGTM!

evals/emit/__init__.py (1)

1-1: LGTM!

evals/emit/json_writer.py (1)

1-34: LGTM!

evals/emit/markdown_writer.py (1)

1-141: LGTM!

evals/__init__.py (1)

1-14: LGTM!

CLAUDE.md (1)

3-3: LGTM!

pyproject.toml (1)

190-190: LGTM!

Also applies to: 283-283, 363-363, 378-378

evals/loader/__init__.py (1)

1-7: LGTM!

evals/loader/anchors.py (1)

39-70: LGTM!

Also applies to: 72-123, 125-132

evals/loader/briefs.py (1)

56-70: LGTM!

evals/errors.py (1)

22-116: LGTM!

evals/models/__init__.py (1)

1-6: LGTM!

tests/evals_spine/test_executable_grader.py (1)

1-231: LGTM!

evals/scoring/executable.py (1)

172-179: Shell metacharacter validation in HiddenCheckSpec is correctly implemented.

The validation exists as a model validator at line 114 of evals/models/brief.py (_leading_token_has_no_shell_metachars), checking the leading token against the _SHELL_METACHARS constant defined at line 27. The docstring (lines 97–105) explicitly documents the defense-in-depth approach: validation at load time catches typos in brief YAML before runtime, even though shell=False mitigates the risk. The noqa comment on line 172 is justified.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use @computed_field instead of @property for derived fields.

The is_clean property is a derived field and should use Pydantic's @computed_field decorator rather than @property. This ensures the field is serialized with the model and aligns with the project's convention for computed fields. As per coding guidelines, derived fields should use @computed_field.

♻️ Proposed fix

-from pydantic import BaseModel, ConfigDict, Field, model_validator
+from pydantic import BaseModel, ConfigDict, Field, computed_field, model_validator

 class ExecutableGrade(BaseModel):
     """Aggregate grade across hidden + build + lint check classes.
 
     Invariant: each ``*_pass`` boolean reflects whether every outcome
     in that label-bucket exited cleanly (``exit_code == 0`` and not
     ``timed_out``). The validator below enforces it.
     """
 
     model_config = ConfigDict(frozen=True, extra="forbid", allow_inf_nan=False)
 
     score: int = Field(ge=0, le=EXEC_TOTAL)
     hidden_pass: bool
     build_pass: bool
     lint_pass: bool
     outcomes: tuple[CheckOutcome, ...]
 
-    `@property`
+    `@computed_field`  # type: ignore[misc]
+    `@property`
     def is_clean(self) -> bool:
         """Whether every declared check class passed."""
         return self.hidden_pass and self.build_pass and self.lint_pass

🤖 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 `@evals/scoring/executable.py` around lines 102 - 105, Replace the `@property` on
the is_clean method with Pydantic's `@computed_field` decorator so the derived
value is serialized; import computed_field from pydantic if missing and keep the
same signature (def is_clean(self) -> bool) and return expression
(self.hidden_pass and self.build_pass and self.lint_pass) so the field is
treated as a computed model field rather than a plain Python property.

[gemini-code-assist]

Code Review

This pull request introduces the golden-company benchmark framework, a comprehensive evaluation suite for autonomous agents featuring executable checks and calibrated LLM judging. The implementation includes Pydantic models for briefs and scorecards, YAML loaders, and scoring logic that incorporates process-fact penalties. Review feedback highlighted the need to measure actual execution duration in the executable grader and to handle PermissionError during subprocess calls. Security improvements were also suggested regarding credential redaction in logs and command outputs, along with a correction to ensure the atomicity of JSON file writes as described in the documentation.

[gemini-code-assist]

Update _outcome_from_completed to accept a duration parameter. The current implementation hardcodes duration_seconds=0.0. Additionally, ensure that the cmd field in CheckOutcome is redacted if it contains URLs with credentials to prevent sensitive data leakage in tool execution results, as required by repository security rules.

Suggested change

	def _outcome_from_completed(
	spec: HiddenCheckSpec,
	label: str,
	completed: subprocess.CompletedProcess[bytes],
	) -> CheckOutcome:
	return CheckOutcome(
	label=label,
	cmd=spec.cmd,
	exit_code=completed.returncode,
	duration_seconds=0.0,
	stdout_tail=_tail(completed.stdout),
	stderr_tail=_tail(completed.stderr),
	)
	def _outcome_from_completed(
	spec: HiddenCheckSpec,
	label: str,
	completed: subprocess.CompletedProcess[bytes],
	duration: float,
	) -> CheckOutcome:
	return CheckOutcome(
	label=label,
	cmd=spec.cmd,
	exit_code=completed.returncode,
	duration_seconds=duration,
	stdout_tail=_tail(completed.stdout),
	stderr_tail=_tail(completed.stderr),
	)

References

1. Ensure that URLs containing potential credentials are redacted in all output channels, including tool execution results (e.g., ToolExecutionResult content).

[gemini-code-assist]

Measure the actual execution time using time.perf_counter() and handle PermissionError. subprocess.run raises PermissionError if the file exists but is not executable, which should be caught and reported as an EvalToolMissingError. When logging the error, ensure that spec.cmd is redacted if it contains URLs with credentials to prevent sensitive data leakage to logs.

def _run_check(spec: HiddenCheckSpec, label: str, work_dir: Path) -> CheckOutcome:
    """Run one check; never raises on non-zero exit; raises on tool-missing."""
    start = time.perf_counter()
    try:
        completed = subprocess.run(  # noqa: S603 -- args validated, no shell
            list(spec.cmd),
            cwd=work_dir,
            timeout=spec.timeout_seconds,
            capture_output=True,
            check=False,
            shell=False,
        )
        duration = time.perf_counter() - start
        return _outcome_from_completed(spec, label, completed, duration)
    except subprocess.TimeoutExpired as exc:
        return _outcome_from_timeout(spec, label, exc)
    except (FileNotFoundError, NotADirectoryError, PermissionError) as exc:
        logger.error(
            "evals.executable.tool_missing",
            cmd=spec.cmd,
            error_type=type(exc).__name__,
            error=safe_error_description(exc),
        )
        msg = f"Required eval tool not found or not executable: {spec.cmd[0]!r}"
        raise EvalToolMissingError(msg) from exc

References

1. Ensure that URLs containing potential credentials are redacted in all output channels, including log messages.

[gemini-code-assist]

Add import time to support measuring the actual duration of subprocess execution, which is currently hardcoded to 0.0 for successful outcomes.

Suggested change

	import subprocess
	from typing import TYPE_CHECKING, Final, Self
	import subprocess
	import time
	from typing import TYPE_CHECKING, Final, Self

[gemini-code-assist]

The docstring claims that write_scorecard_json overwrites the file atomically via write_text, but pathlib.Path.write_text is not atomic (it truncates the file upon opening). To ensure atomicity and prevent corruption if the process crashes during writing, use a temporary file and os.replace.

def write_scorecard_json(scorecard: Scorecard, out_dir: Path) -> Path:
    """Write *scorecard* as JSON into *out_dir*; return the file path."""
    import os
    import tempfile

    out_dir.mkdir(parents=True, exist_ok=True)
    target = out_dir / SCORECARD_JSON_FILENAME
    payload = scorecard.model_dump_json(indent=JSON_INDENT) + "\n"

    with tempfile.NamedTemporaryFile(
        mode="w", dir=out_dir, encoding="utf-8", delete=False
    ) as tf:
        tf.write(payload)
        temp_name = tf.name

    try:
        os.replace(temp_name, target)
    except Exception:
        try:
            os.unlink(temp_name)
        except OSError:
            pass
        raise
    return target

[codecov]

Codecov Report

❌ Patch coverage is 80.12270% with 486 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.00%. Comparing base (f60e9ed) to head (2c25a0a).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...c/synthorg/persistence/sqlite/conversation_repo.py	62.89%	56 Missing and 3 partials ⚠️
...synthorg/persistence/postgres/conversation_repo.py	65.27%	47 Missing and 3 partials ⚠️
...persistence/sqlite/conversational_proposal_repo.py	65.57%	41 Missing and 1 partial ⚠️
...rsistence/postgres/conversational_proposal_repo.py	70.00%	32 Missing and 1 partial ⚠️
...rg/engine/workspace/git_backend/external_remote.py	58.10%	29 Missing and 2 partials ⚠️
src/synthorg/engine/workspace/push_queue.py	73.39%	21 Missing and 8 partials ⚠️
src/synthorg/meta/chief_of_staff/propose.py	86.74%	20 Missing and 2 partials ⚠️
...thorg/persistence/sqlite/project_workspace_repo.py	68.57%	22 Missing ⚠️
src/synthorg/persistence/conversational_factory.py	54.28%	15 Missing and 1 partial ⚠️
...org/persistence/postgres/project_workspace_repo.py	75.00%	16 Missing ⚠️
... and 21 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2025      +/-   ##
==========================================
- Coverage   85.09%   85.00%   -0.10%     
==========================================
  Files        1913     1948      +35     
  Lines      113388   115799    +2411     
  Branches     9673     9839     +166     
==========================================
+ Hits        96485    98430    +1945     
- Misses      14533    14943     +410     
- Partials     2370     2426      +56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@evals/emit/json_writer.py`:
- Around line 37-45: The temporary JSON file is opened in text mode without
specifying newline, causing platform-dependent CRLF/LF translations; update the
NamedTemporaryFile call in evals/emit/json_writer.py (the with
tempfile.NamedTemporaryFile(...) as handle block) to include newline="\n" so the
written payload uses LF-only line endings across platforms, keeping mode="w",
encoding="utf-8", delete=False, prefix/suffix as-is.

In `@evals/scoring/aggregate.py`:
- Around line 86-87: The validator
AggregationResult._score_matches_grade_minus_deduction currently enforces the
floor using the global GRADE_FLOOR while aggregate_brief_score uses
penalty_table.floor; change the validator to use the same floor source as
aggregation (e.g., compute expected = max(self.grade - self.deduction,
self.penalty_table.floor) or otherwise obtain penalty_table.floor from the
AggregationResult instance/context) so both validation and scoring use
penalty_table.floor consistently; ensure any null/absent penalty_table is
handled (fall back to GRADE_FLOOR only if penalty_table is unavailable).

🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 570e0380-6ccd-4433-aa7a-6eb407842c8e

📥 Commits

Reviewing files that changed from the base of the PR and between 815ac88 and 48c3121.

📒 Files selected for processing (14)

• evals/emit/json_writer.py
• evals/errors.py
• evals/loader/briefs.py
• evals/models/brief.py
• evals/models/scorecard.py
• evals/scoring/aggregate.py
• evals/scoring/executable.py
• evals/scoring/penalties.py
• src/synthorg/observability/events/evals.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_scorecard_emit.py
• tests/unit/observability/test_events.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
• GitHub Check: Dashboard Test
• GitHub Check: Test Unit
• GitHub Check: Test Integration
• GitHub Check: Analyze (python)

🧰 Additional context used

📓 Path-based instructions (4)

src/synthorg/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Follow configuration precedence: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); Cat-3 bootstrap secrets are pure env at boot site; no os.environ.get outside startup

No hardcoded numeric values; numerics live in settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final|Final[int]|Final[float] = literal)

Use comments to explain WHY only; no reviewer citations, issue back-refs, or migration framing; enforced by check_no_review_origin_in_code.py and check_no_migration_framing.py

No from __future__ import annotations (Python 3.14 has PEP 649); PEP 758 except: except A, B: must have parens unless binding

Type hints on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines

Define errors as Error inheriting from DomainError, never directly from Exception/RuntimeError/etc; enforced by check_domain_error_hierarchy.py

Use Pydantic v2 frozen + extra='forbid' on every frozen model project-wide (gate check_frozen_model_extra_forbid.py); @computed_field for derived; NotBlankStr for identifiers

Args models at every system boundary; parse_typed() for every external dict ingestion; enforced by check_boundary_typed.py

Use immutability: model_copy(update=...) or copy.deepcopy(); deepcopy at system boundaries

Use asyncio.TaskGroup for fan-out/fan-in async operations; helpers must catch Exception and re-raise MemoryError/RecursionError

Implement Clock seam: clock: Clock | None = None; tests inject FakeClock

Lifecycle: services own _lifecycle_lock; timed-out stops mark unrestartable

Wrap untrusted content with wrap_untrusted() from engine.prompt_safety; use HTMLParseGuard for HTML (SEC-1)

Import logger as: from synthorg.observability import get_logger; variable always named logger; never use import logging or print() in app code

Use e...

Files:

• src/synthorg/observability/events/evals.py

src/synthorg/observability/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Telemetry: opt-in, off by default; every event property must be in _ALLOWED_PROPERTIES

Files:

• src/synthorg/observability/events/evals.py

src/**/*.py

⚙️ CodeRabbit configuration file

This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.

Files:

• src/synthorg/observability/events/evals.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use test markers: @pytest.mark.{unit,integration,e2e,slow}; async auto; timeout 30s global; coverage 80% min

xdist -n 8 --dist=loadfile auto-applied via pyproject addopts (loadfile prevents 3.14+Windows ProactorEventLoop leak)

Windows: unit tests use WindowsSelectorEventLoopPolicy (3.14 IOCP teardown race); subprocess tests override back

Test doubles: FakeClock for Clock seam; mock_ofT for typed-boundary substitutions; SimpleNamespace for attribute-bags; bare MagicMock at typed boundary blocked by scripts/check_mock_spec.py (zero-tolerance)

FakeClock and mock_of import from tests._shared; inject via clock= and helper's spec subscript

Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add @example(...))

Flaky tests: NEVER skip/xfail; fix fundamentally; use asyncio.Event().wait() not sleep(large)

Files:

• tests/unit/observability/test_events.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_scorecard_emit.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_judged_calibration.py

---

⚙️ CodeRabbit configuration file

Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare @settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which @given() honors automatically.

Files:

• tests/unit/observability/test_events.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_scorecard_emit.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_judged_calibration.py

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Read design spec page before implementing; deviations need approval from DESIGN_SPEC.md

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Present every plan for accept/deny before coding

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Apply no region/currency/locale privilege; use metric units and British English

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Every convention PR must ship its enforcement gate

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Never edit tests/baselines/unit_timing.json or any scripts/*_baseline.{txt,json} / scripts/_*_baseline.py; timeout/slow failures indicate source-code regression

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: After issue: branch + commit + push (no auto-PR); use /pre-pr-review before PR; after PR use /aurelio-review-pr for external feedback; fix EVERYTHING valid before merging

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Commits: <type>: <description> (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via synthorg-repo-bot)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Branches: <type>/<slug> from main

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: Squash merge; PR body becomes squash commit; trailers (Release-As, Closes `#N`) must be in PR body

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: GitHub queries: use gh issue list via Bash, NOT MCP list_issues

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: After every squash merge → /post-merge-cleanup

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:31:34.221Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API

📚 Learning: 2026-05-05T09:04:46.195Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.

Applied to files:

• src/synthorg/observability/events/evals.py
• tests/unit/observability/test_events.py
• evals/errors.py
• evals/scoring/aggregate.py
• tests/evals_spine/test_penalties.py
• evals/models/scorecard.py
• evals/scoring/executable.py
• evals/models/brief.py
• evals/scoring/penalties.py
• evals/emit/json_writer.py
• tests/evals_spine/test_scorecard_emit.py
• evals/loader/briefs.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_judged_calibration.py

🔇 Additional comments (13)

src/synthorg/observability/events/evals.py (1)

1-13: LGTM!

tests/unit/observability/test_events.py (1)

336-339: LGTM!

evals/errors.py (1)

43-54: LGTM!

Also applies to: 120-120

evals/scoring/aggregate.py (1)

12-12: LGTM!

Also applies to: 34-34, 131-133

tests/evals_spine/test_penalties.py (1)

21-21: LGTM!

Also applies to: 185-210

evals/models/scorecard.py (1)

21-21: LGTM!

Also applies to: 62-62, 82-85, 147-162, 166-166, 190-191, 197-205, 209-209, 259-259, 271-271

evals/scoring/executable.py (1)

18-18: LGTM!

Also applies to: 25-30, 73-73, 83-83, 109-113, 121-121, 148-168, 186-215, 231-231

evals/models/brief.py (1)

53-57: LGTM!

Also applies to: 117-117, 121-121, 253-253

evals/scoring/penalties.py (1)

17-21: LGTM!

Also applies to: 91-91, 101-110

tests/evals_spine/test_scorecard_emit.py (1)

31-41: LGTM!

Also applies to: 53-53, 236-283

evals/loader/briefs.py (1)

12-16: LGTM!

Also applies to: 44-45, 49-49, 53-66

tests/evals_spine/test_brief_loader.py (1)

9-13: LGTM!

Also applies to: 131-161

tests/evals_spine/test_judged_calibration.py (1)

3-3: LGTM!

Also applies to: 81-81, 84-84

[coderabbitai]

Actionable comments posted: 12

🤖 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 `@evals/emit/json_writer.py`:
- Line 25: Remove the unused logger initialization: delete the statement that
creates the module-level logger (logger = get_logger(__name__)) and also remove
the corresponding get_logger import if it isn't used elsewhere in this module;
ensure no references to the symbol logger remain (search for logger and
get_logger in this file) before committing the change.

In `@evals/emit/markdown_writer.py`:
- Around line 128-133: The write_scorecard_md function currently calls
target.write_text(render_scorecard_md(scorecard), encoding="utf-8") which allows
platform-dependent newline translation; change this call to pass newline="\n" so
the file is written with deterministic LF line endings (consistent with
json_writer.py). Update the write_text invocation in write_scorecard_md (and
ensure it still uses SCORECARD_MD_FILENAME and render_scorecard_md) to include
newline="\n" while preserving encoding="utf-8".
- Line 23: The module defines an unused logger via the variable logger =
get_logger(__name__) in markdown_writer.py; remove that unused initialization
and any related unused imports (get_logger or logger) from the file, or if
logging is intended, replace it with a used logging call (e.g., use the logger
inside functions like write_markdown or export_markdown) — locate the
logger/get_logger symbols in the module and either delete them or integrate them
into existing functions so the symbol is actually used.

In `@evals/models/scorecard.py`:
- Around line 252-255: The is_passing method currently truncates the threshold
by using int(self.max_total * PASS_FRACTION); replace that with a non-truncating
ceiling comparison so the required pass count isn't lowered for non-integer
thresholds—use math.ceil(self.max_total * PASS_FRACTION) (import math if needed)
and update is_passing to compare self.total against that ceiling value.

In `@evals/scoring/__init__.py`:
- Around line 3-12: The module docstring incorrectly states "Three concerns"
while listing four modules; update the top-level docstring in evals.scoring (the
module docstring in __init__.py) to read "Four concerns" (or a neutral phrase
like "Several concerns") so the count matches the listed modules
(:mod:`evals.scoring.penalties`, :mod:`evals.scoring.executable`,
:mod:`evals.scoring.judged`, :mod:`evals.scoring.aggregate`).

In `@evals/scoring/aggregate.py`:
- Around line 112-115: Update the docstring that describes the parameter "grade"
so it matches the implementation: remove the statement that out-of-range values
are "clamped" and instead say that grades must be in [0, 100] and values outside
that range raise a ValueError (no clamping). Ensure the docstring for the
function/method that accepts the "grade" parameter mirrors the actual validation
behavior where a ValueError is raised when the grade is out of bounds.

In `@evals/scoring/judged.py`:
- Around line 210-216: Replace the hardcoded event string
"evals.judge.calibration_failed" used in the logger.warning call inside
judged.py with the corresponding event constant exported from
synthorg.observability.events.evals (e.g., EVALS_JUDGE_CALIBRATION_FAILED); add
the import for that constant at the top of the file and use the constant in the
logger.warning call so the code matches the existing event-constant convention.

In `@tests/evals_spine/test_executable_grader.py`:
- Around line 78-96: The test matrix in pytest.mark.parametrize for
("hidden_pass", "build_pass", "lint_pass", "expected_score") is missing the two
boolean combinations (hidden=True, build=False, lint=False) and (hidden=False,
build=False, lint=True); add entries for (True, False, False, EXEC_TOTAL -
EXEC_WEIGHT_BUILD - EXEC_WEIGHT_LINT) and (False, False, True, EXEC_TOTAL -
EXEC_WEIGHT_HIDDEN - EXEC_WEIGHT_BUILD) and append corresponding ids (e.g.,
"build_and_lint_fail" and "hidden_and_build_fail") so the full 8-combination
coverage is present in tests/evals_spine/test_executable_grader.py using the
existing pytest.mark.parametrize block and the constants EXEC_TOTAL,
EXEC_WEIGHT_HIDDEN, EXEC_WEIGHT_BUILD, EXEC_WEIGHT_LINT.

In `@tests/evals_spine/test_judged_calibration.py`:
- Line 306: Add a single module-level import for load_anchor_set from
evals.loader.anchors (alongside the other anchors imports) and remove the two
local imports that appear inside the tests (the local load_anchor_set imports
within the two test functions). This consolidates imports: declare "from
evals.loader.anchors import load_anchor_set" at top-level and delete the
duplicate local import statements in the test bodies so the tests use the
module-level symbol.
- Line 287: Consolidate the local pydantic ValidationError imports by adding a
single module-level import "from pydantic import ValidationError as
PydValidationError" at the top of the test module and removing the three
redundant local imports of PydValidationError inside the individual tests;
update any local references to use the module-level PydValidationError symbol so
tests continue to raise/handle Pydantic ValidationError consistently.
- Line 324: Remove the redundant local import of AnchorItem by deleting the
duplicate "from evals.loader.anchors import AnchorItem" line in the
test_judged_calibration.py module so only the module-level import of AnchorItem
remains; search for any extra occurrences of the symbol AnchorItem within that
file to ensure you remove only the duplicate import and not any necessary
usages.

In `@tests/evals_spine/test_runner_broken_scores_worse.py`:
- Line 53: The assertion in the test uses a strict greater-than comparison
(reference.total > broken.total + SCORE_MARGIN) but the contract requires an
inclusive threshold ("at least" SCORE_MARGIN); change the comparison to a
greater-than-or-equal comparison so the check becomes reference.total >=
broken.total + SCORE_MARGIN, ensuring a gap exactly equal to SCORE_MARGIN
passes.

🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: a85a438d-9fc5-40c1-a517-15ae67cf84d8

📥 Commits

Reviewing files that changed from the base of the PR and between 48c3121 and 6dfda3a.

📒 Files selected for processing (29)

• CLAUDE.md
• evals/__init__.py
• evals/emit/__init__.py
• evals/emit/json_writer.py
• evals/emit/markdown_writer.py
• evals/errors.py
• evals/loader/__init__.py
• evals/loader/anchors.py
• evals/loader/briefs.py
• evals/models/__init__.py
• evals/models/brief.py
• evals/models/scorecard.py
• evals/scoring/__init__.py
• evals/scoring/aggregate.py
• evals/scoring/executable.py
• evals/scoring/judged.py
• evals/scoring/penalties.py
• evals/scoring/spearman.py
• pyproject.toml
• src/synthorg/observability/events/evals.py
• tests/evals_spine/__init__.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_brief_loader.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py
• tests/unit/observability/test_events.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
• GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
• GitHub Check: Test Integration
• GitHub Check: Test E2E
• GitHub Check: Dashboard Test
• GitHub Check: Test Unit
• GitHub Check: Analyze (python)

🧰 Additional context used

📓 Path-based instructions (5)

src/synthorg/!(persistence)/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Only src/synthorg/persistence/ may import sqlite/psycopg or emit raw SQL

Files:

• src/synthorg/observability/events/evals.py

src/synthorg/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Configuration precedence: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); Cat-3 bootstrap secrets are pure env at boot site; no os.environ.get outside startup

Files:

• src/synthorg/observability/events/evals.py

src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

No hardcoded numeric values; numerics live in settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the form NAME: int|float|Final|Final[int]|Final[float] = literal

Comments should explain WHY only; no reviewer citations, issue back-refs, or migration framing

No from __future__ import annotations (Python 3.14 has PEP 649); use PEP 758 except: except A, B: requires parens if binding

Type hints required on public functions; mypy strict mode; Google-style docstrings

Line length 88; functions <50 lines; files <800 lines

Errors must use <Domain><Condition>Error naming and inherit from DomainError; never inherit directly from Exception/RuntimeError/etc

Pydantic v2 frozen models must use extra="forbid" on every frozen model; @computed_field is auto-exempt; per-line # lint-allow: frozen-extra-forbid -- <reason> for exceptions

@computed_field should be used for derived properties in Pydantic models

Use NotBlankStr for identifiers in Pydantic models

Args models required at every system boundary; use parse_typed() for every external dict ingestion

Immutability: use model_copy(update=...) or copy.deepcopy(); deepcopy at system boundaries

Async code must use asyncio.TaskGroup for fan-out/fan-in; helpers catch Exception and re-raise MemoryError/RecursionError

Use Clock seam: clock: Clock | None = None; tests inject FakeClock

Lifecycle services must own _lifecycle_lock; timed-out stops mark unrestartable

Untrusted content (SEC-1) must be wrapped via wrap_untrusted() from engine.prompt_safety; use HTMLParseGuard for HTML

Import logger via from synthorg.observability import get_logger; variable always named logger; never use import logging or print() in app code

Event names must come from observability.events.<domain> constants; use structured kwargs in log calls (logger.info(EVENT, key=value))

Error paths must log WARNING/ERROR w...

Files:

• src/synthorg/observability/events/evals.py

---

⚙️ CodeRabbit configuration file

This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.

Files:

• src/synthorg/observability/events/evals.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Test markers: @pytest.mark.{unit,integration,e2e,slow}

Async tests use auto mode; global timeout 30s; coverage 80% minimum

Test doubles ladder: FakeClock for Clock seam, mock_of[T](**overrides) for typed-boundary substitutions, SimpleNamespace for attribute-bags; bare MagicMock at typed boundary is blocked by scripts/check_mock_spec.py

FakeClock and mock_of import from tests._shared; inject via clock= and helper's spec subscript

Flaky tests: NEVER skip/xfail; fix fundamentally; use asyncio.Event().wait() not sleep(large)

Files:

• tests/unit/observability/test_events.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py
• tests/evals_spine/test_brief_loader.py

---

⚙️ CodeRabbit configuration file

Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare @settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which @given() honors automatically.

Files:

• tests/unit/observability/test_events.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py
• tests/evals_spine/test_brief_loader.py

tests/**/*test*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Windows unit tests use WindowsSelectorEventLoopPolicy (3.14 IOCP teardown race); subprocess tests override back

Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add @example(...))

Files:

• tests/unit/observability/test_events.py
• tests/evals_spine/conftest.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py
• tests/evals_spine/test_brief_loader.py

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Read `docs/design/` page before implementing; deviations need approval

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Present every plan for accept/deny before coding

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: No region/currency/locale privileged; use metric units; British English

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Every convention PR must ship its enforcement gate

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Never edit `tests/baselines/unit_timing.json` or any `scripts/*_baseline.{txt,json}` / `scripts/_*_baseline.py` files; timeout/slow failures indicate source-code regression

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review` before creating PR

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: After PR creation: use `/aurelio-review-pr` for external feedback and fix all valid issues before merging

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Use `go -C cli` command, never `cd cli`

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: xdist `-n 8 --dist=loadfile` auto-applied via pyproject `addopts` (`loadfile` prevents 3.14+Windows ProactorEventLoop leak)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Branches: `<type>/<slug>` from main

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Pre-commit/pre-push hooks via `.pre-commit-config.yaml`; tool-call gates via `.claude/settings.json` PreToolUse (`scripts/check_*.sh`/`.py`)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: Squash merge; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T18:59:08.797Z
Learning: After every squash merge → `/post-merge-cleanup`

📚 Learning: 2026-05-05T09:04:46.195Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.

Applied to files:

• evals/emit/__init__.py
• evals/loader/__init__.py
• src/synthorg/observability/events/evals.py
• evals/__init__.py
• evals/models/__init__.py
• evals/scoring/__init__.py
• evals/scoring/spearman.py
• evals/errors.py
• tests/unit/observability/test_events.py
• tests/evals_spine/conftest.py
• evals/emit/markdown_writer.py
• evals/emit/json_writer.py
• tests/evals_spine/test_executable_grader.py
• evals/loader/anchors.py
• evals/models/brief.py
• evals/models/scorecard.py
• evals/scoring/aggregate.py
• tests/evals_spine/test_penalties.py
• tests/evals_spine/test_judged_calibration.py
• evals/scoring/executable.py
• tests/evals_spine/test_runner_broken_scores_worse.py
• tests/evals_spine/test_scorecard_emit.py
• evals/loader/briefs.py
• evals/scoring/judged.py
• evals/scoring/penalties.py
• tests/evals_spine/test_brief_loader.py

📚 Learning: 2026-05-16T18:36:31.446Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.

Applied to files:

• CLAUDE.md

📚 Learning: 2026-05-16T18:36:35.250Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).

Applied to files:

• CLAUDE.md

🔇 Additional comments (45)

tests/evals_spine/conftest.py (1)

1-60: LGTM!

tests/evals_spine/test_brief_loader.py (1)

19-180: LGTM!

evals/loader/briefs.py (1)

1-94: LGTM!

evals/loader/__init__.py (1)

1-8: LGTM!

evals/loader/anchors.py (1)

1-133: LGTM!

tests/evals_spine/test_runner_broken_scores_worse.py (1)

1-52: LGTM!

Also applies to: 54-88

evals/scoring/spearman.py (1)

1-97: LGTM!

evals/scoring/penalties.py (1)

1-153: LGTM!

tests/evals_spine/test_penalties.py (1)

1-233: LGTM!

tests/evals_spine/test_scorecard_emit.py (1)

1-316: LGTM!

tests/unit/observability/test_events.py (1)

336-339: LGTM!

tests/evals_spine/test_executable_grader.py (2)

26-75: LGTM!

---

97-230: LGTM!

src/synthorg/observability/events/evals.py (2)

1-10: LGTM!

---

12-13: The constants at lines 12-13 are properly imported and used in the executable scoring module (evals/scoring/executable.py). Both EVALS_EXECUTABLE_TIMEOUT and EVALS_EXECUTABLE_TOOL_MISSING are used correctly in structured logging calls with appropriate context kwargs and secure error handling (safe_error_description(exc) instead of raw exception strings).

CLAUDE.md (1)

3-3: LGTM!

evals/__init__.py (1)

1-15: LGTM!

pyproject.toml (1)

190-190: LGTM!

Also applies to: 283-283, 363-363, 378-378

evals/errors.py (1)

1-130: LGTM!

evals/models/__init__.py (1)

1-7: LGTM!

evals/models/brief.py (1)

1-269: LGTM!

evals/models/scorecard.py (1)

1-251: LGTM!

Also applies to: 256-313

evals/scoring/executable.py (4)

1-91: LGTM!

---

93-134: LGTM!

---

136-216: LGTM!

---

218-300: LGTM!

evals/scoring/judged.py (5)

1-52: LGTM!

---

54-119: LGTM!

---

122-171: LGTM!

---

174-222: LGTM!

---

225-298: LGTM!

evals/emit/json_writer.py (2)

37-45: Past review finding has been addressed.

The past review comment flagged the missing newline="\n" parameter. The current code includes it at line 41, ensuring platform-independent LF line endings for byte-level determinism.

---

31-57: LGTM!

evals/emit/__init__.py (1)

1-1: LGTM!

evals/emit/markdown_writer.py (6)

34-37: LGTM!

---

40-48: LGTM!

---

51-69: LGTM!

---

72-79: LGTM!

---

82-97: LGTM!

---

100-125: LGTM!

tests/evals_spine/test_judged_calibration.py (5)

1-32: LGTM!

---

36-102: LGTM!

---

108-159: LGTM!

---

162-197: LGTM!

---

200-258: LGTM!

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

evals/models/scorecard.py (1)

129-177: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Persist the per-brief score floor in BriefResult.

aggregate_brief_score() now allows score = max(grade - deduction, penalty_table.floor), but BriefResult still revalidates against max(grade - deduction, GRADE_FLOOR) and has no field to carry the configured floor. Any non-zero PenaltyTable.floor will make a valid aggregation impossible to serialize into the scorecard without either failing validation or lying about how the score was derived. Either thread the floor through BriefResult/Scorecard and version the schema, or reject non-zero floors before scorecard construction.

🤖 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 `@evals/models/scorecard.py` around lines 129 - 177, The BriefResult validation
assumes a global GRADE_FLOOR but aggregate_brief_score can use a
PenaltyTable.floor; add a score_floor int field to BriefResult (defaulting to
GRADE_FLOOR for compatibility) and update the
_score_matches_grade_minus_deduction validator to compute expected =
max(self.grade - self.deduction, self.score_floor) so the serialized row records
the actual floor used. Also ensure aggregate_brief_score (and Scorecard
construction) populates BriefResult.score_floor from PenaltyTable.floor when
creating BriefResult instances (or else reject non-zero PenaltyTable.floor
before building scorecards if you choose to forbid non-default floors).

🤖 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.

Outside diff comments:
In `@evals/models/scorecard.py`:
- Around line 129-177: The BriefResult validation assumes a global GRADE_FLOOR
but aggregate_brief_score can use a PenaltyTable.floor; add a score_floor int
field to BriefResult (defaulting to GRADE_FLOOR for compatibility) and update
the _score_matches_grade_minus_deduction validator to compute expected =
max(self.grade - self.deduction, self.score_floor) so the serialized row records
the actual floor used. Also ensure aggregate_brief_score (and Scorecard
construction) populates BriefResult.score_floor from PenaltyTable.floor when
creating BriefResult instances (or else reject non-zero PenaltyTable.floor
before building scorecards if you choose to forbid non-default floors).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: f2fa10d1-679d-449d-bd31-72249dc71096

📥 Commits

Reviewing files that changed from the base of the PR and between 6dfda3a and e57d7a0.

📒 Files selected for processing (10)

• evals/emit/json_writer.py
• evals/emit/markdown_writer.py
• evals/models/scorecard.py
• evals/scoring/__init__.py
• evals/scoring/aggregate.py
• evals/scoring/judged.py
• src/synthorg/observability/events/evals.py
• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py

💤 Files with no reviewable changes (1)

• evals/emit/json_writer.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
• GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
• GitHub Check: Dashboard Test
• GitHub Check: Test Unit
• GitHub Check: Test Integration
• GitHub Check: Analyze (python)

🧰 Additional context used

📓 Path-based instructions (3)

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Async test fixtures: use auto mode; global timeout 30s; coverage minimum 80%; xdist -n 8 --dist=loadfile auto-applied via pyproject addopts (loadfile prevents 3.14+ Windows ProactorEventLoop leak)

Windows unit tests use WindowsSelectorEventLoopPolicy (3.14 IOCP teardown race); subprocess tests override back

Test markers: @pytest.mark.{unit,integration,e2e,slow}; test doubles ladder in conventions.md section 12.1; use FakeClock and mock_of[T](**overrides) from tests._shared; SimpleNamespace for attribute-bags; bare MagicMock at typed boundary blocked by scripts/check_mock_spec.py

Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add @example(...))

Flaky tests: NEVER skip/xfail; fix fundamentally; use asyncio.Event().wait() instead of sleep(large)

Files:

• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py

---

⚙️ CodeRabbit configuration file

Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare @settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which @given() honors automatically.

Files:

• tests/evals_spine/test_executable_grader.py
• tests/evals_spine/test_judged_calibration.py
• tests/evals_spine/test_runner_broken_scores_worse.py

src/synthorg/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Configuration precedence order: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); Cat-3 bootstrap secrets are pure env; YAML is ingestion format only, not a precedence tier; no os.environ.get outside startup; pre-init Cat-2 reads use settings.bootstrap_resolver.resolve_init_value

Lifecycle services must own _lifecycle_lock; timed-out stops mark services unrestartable

Files:

• src/synthorg/observability/events/evals.py

src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

No hardcoded numeric values; numerics live in settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of form NAME: int|float|Final|Final[int]|Final[float] = literal per scripts/check_no_magic_numbers.py

Use asyncio.TaskGroup for fan-out/fan-in in async code; helpers must catch Exception and re-raise MemoryError/RecursionError

Use Clock seam pattern: clock: Clock | None = None; tests inject FakeClock from tests._shared

Wrap untrusted content via wrap_untrusted() from engine.prompt_safety; use HTMLParseGuard for HTML per SEC-1

Use from synthorg.observability import get_logger with variable always named logger; never use import logging or print() in app code

Event names must come from observability.events.<domain> constants; use structured kwargs (logger.info(EVENT, key=value))

Error paths must log WARNING/ERROR with context before raising; state transitions log INFO via *_STATUS_TRANSITIONED AFTER persistence write

Secret-log redaction (SEC-1): never use error=str(exc) or interpolate {exc}; use error_type=type(exc).__name__ + error=safe_error_description(exc); never exc_info=True; OTel forbidden span.record_exception(exc), use span.set_attribute("exception.message", safe_error_description(exc)) + record_exception=False, set_status_on_exception=False per check_logger_exception_str_exc.py

Comments must explain WHY only; no reviewer citations, issue back-refs, or migration framing per check_no_review_origin_in_code.py and check_no_migration_framing.py

No from __future__ import annotations (Python 3.14+ has PEP 649); PEP 758 except: except A, B: requires parens when binding

Type hints required on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines

Error classes must follow <Domain><Condition>Error naming and inherit from DomainError; never inherit directly from `Exce...

Files:

• src/synthorg/observability/events/evals.py

---

⚙️ CodeRabbit configuration file

This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.

Files:

• src/synthorg/observability/events/evals.py

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Read design specification in `docs/design/` before implementing; deviations require approval per [DESIGN_SPEC.md](docs/DESIGN_SPEC.md)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Present every plan for accept/deny before coding

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: No region/currency/locale should be privileged; use metric units and British English per [docs/reference/regional-defaults.md](docs/reference/regional-defaults.md)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Every convention PR must ship its enforcement gate per [docs/reference/convention-gates.md](docs/reference/convention-gates.md)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Timeout/slow test failures indicate source-code regression; never edit `tests/baselines/unit_timing.json` or baseline files; bypass only via `ALLOW_BASELINE_GROWTH=1 git commit ...` with explicit user approval

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: After issue implementation: branch + commit + push without auto-PR; use `/pre-pr-review` (gh pr create is blocked); after PR use `/aurelio-review-pr` for external feedback; fix all valid issues without deferring

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Git commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Branch naming: `<type>/<slug>` from main

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Pre-commit/pre-push hooks via `.pre-commit-config.yaml`; tool-call gates via `.claude/settings.json` PreToolUse (`scripts/check_*.sh`/`.py`)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Squash merge; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: After every squash merge → run `/post-merge-cleanup`

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Use `uv sync` for all dependencies, `uv sync --group docs` for docs toolchain; run `bash scripts/install_cli_tools.sh` once per machine for golangci-lint; run `bash scripts/install_git_hooks.sh` once per clone to wire `core.hooksPath -> scripts/git-hooks`

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Linting: `uv run ruff check src/ tests/ --fix` + `uv run ruff format src/ tests/`; type checking: `uv run mypy --num-workers=4 src/ tests/` (strict)

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Testing: `uv run python -m pytest tests/ -m {unit|integration|e2e}` with `-n 8 --dist=loadfile` via pyproject `addopts`; coverage minimum 80%; use `HYPOTHESIS_PROFILE={dev|fuzz}` for property tests

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Pre-commit checks: `uv run pre-commit run --all-files`; schema drift: `uv run python scripts/check_schema_drift_revisions.py --backend {sqlite|postgres}`

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Documentation build: `PYTHONPATH=. uv run zensical build` for docs; reference [docs/reference/claude-reference.md](docs/reference/claude-reference.md) for doc layout, Docker, releasing, CI, dependencies, Hypothesis

Learnt from: CR
Repo: Aureliolo/synthorg

Timestamp: 2026-05-20T19:19:19.402Z
Learning: Web: see `web/CLAUDE.md` for guidelines; CLI: see `cli/CLAUDE.md` and use `go -C cli` (never `cd cli`); Shell: see `~/.claude/rules/common/bash.md` for canonical `cd`/`git -C`/Bash file-write rules

📚 Learning: 2026-05-05T09:04:46.195Z

Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.

Applied to files:

• evals/scoring/__init__.py
• tests/evals_spine/test_executable_grader.py
• src/synthorg/observability/events/evals.py
• evals/scoring/aggregate.py
• evals/emit/markdown_writer.py
• tests/evals_spine/test_judged_calibration.py
• evals/models/scorecard.py
• evals/scoring/judged.py
• tests/evals_spine/test_runner_broken_scores_worse.py

🔇 Additional comments (9)

src/synthorg/observability/events/evals.py (1)

14-14: LGTM!

tests/evals_spine/test_executable_grader.py (1)

85-87: LGTM!

Also applies to: 95-97

tests/evals_spine/test_judged_calibration.py (1)

7-8: LGTM!

Also applies to: 11-16, 26-26, 30-30

evals/scoring/__init__.py (1)

3-3: LGTM!

evals/scoring/aggregate.py (1)

112-116: LGTM!

evals/scoring/judged.py (1)

24-24: LGTM!

Also applies to: 211-217

evals/models/scorecard.py (1)

10-10: LGTM!

Also applies to: 252-261

evals/emit/markdown_writer.py (1)

124-133: LGTM!

tests/evals_spine/test_runner_broken_scores_worse.py (1)

53-53: LGTM!

[Aureliolo]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.