[HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327)#1369
[HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327)#1369100yenadmin wants to merge 1 commit into
Conversation
… eval) (#1327) Slice 2: qa/nightly_harvest.py scores UNSCORED qa/nominations.jsonl entries via HV1's score_artifact_panel seam, off the duo hot path (bounded --max-per-run, resumable, idempotent, non-fatal per-artifact isolation). Slice 3: additive library_metrics table in qa/scores.db (qa/scores_db.py) + qa/library_metrics.py's snapshot_library() reads library/ + .promoted.jsonl to record size-by-class/tier, Σreuse_count, and promotion pass-rate — the "less AI dependence" trend the epic names. 70 new offline tests (fabricated fixtures, no live LLM); fast_gate.sh green (257); library-lint clean; scores_db.py additions are additive-only.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughPriority Level: P1
WalkthroughThis PR adds two independent QA scripts: ChangesLibrary Metrics Snapshot
Estimated code review effort: 3 (Moderate) | ~25 minutes Nightly Harvest Batch Scorer
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as library_metrics.main
participant Snapshot as snapshot_library
participant Scan as scan_library/scan_promotion_log
participant DB as scores_db
CLI->>Snapshot: invoke with args
Snapshot->>Scan: scan library dir and promoted.jsonl
Scan-->>Snapshot: size/reuse/pass_rate metrics
Snapshot->>DB: add_library_metrics(payload)
DB-->>Snapshot: row_id
Snapshot-->>CLI: payload with row_id
CLI->>DB: render_library_metrics_markdown (optional)
sequenceDiagram
participant CLI as nightly_harvest.main
participant Batch as harvest_batch
participant DB as scores_db
participant Loader as _load_nomination_artifact
participant Scorer as artifact_score.score_artifact_panel
participant Log as progress log
CLI->>Batch: run with args
Batch->>DB: fetch_artifacts (scored ids)
Batch->>Batch: dedup + cap nominations
loop each nomination
Batch->>Loader: load artifact JSON
Loader-->>Batch: artifact or load-failed
Batch->>Scorer: score_artifact_panel(artifact)
Scorer-->>Batch: score result or exception
Batch->>Log: append verdict (scored/load-failed/score-failed)
end
Batch-->>CLI: batch report JSON
Possibly related issues
Possibly related PRs
AI Agent Review Notes (adversarial pass, confidence-scored)
Overall: no critical correctness bugs identified from the summaries alone (i.e., nothing that would definitely crash or corrupt data), but the concurrency/idempotency assumptions in 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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 `@qa/nightly_harvest.py`:
- Around line 195-219: The report’s already_scored value is being derived from
the post-dedup candidates list in the nightly harvest flow, which mixes
pre-scored skips with intra-batch duplicate removal. Update the logic around the
noms/candidates processing in nightly_harvest so already_scored counts only
artifact_id values filtered by already_scored, and track the seen-based de-dup
separately for the report. Keep the candidates and report fields in sync so the
summary reflects true pre-scored items without double-counting duplicate
nominations.
In `@qa/scores_db.py`:
- Around line 538-597: Add range validation inside add_library_metrics for the
percent-style fields promotion_pass_rate and pct_library_sourced so invalid
values cannot be persisted. Before building the INSERT, check any provided
values for these keys are numeric and within 0.0 to 1.0 inclusive, and raise a
ValueError with a clear message if not. Keep the validation in
add_library_metrics so both CLI paths that feed it are covered, and leave the
existing unknown-field handling and JSON coercion unchanged.
In `@qa/test_nightly_harvest.py`:
- Around line 199-208: The duplicate-artifact test in
test_duplicate_artifact_id_in_queue_scored_once only checks report["scored"], so
it misses the already_scored reporting bug. Update this test to also assert
report["already_scored"] is 0 after _run(env), using the existing
harvest_batch/report output and the same duplicate nomination setup, so the
metric regression is caught when duplicates are deduped rather than pre-scored.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94be5377-138c-45b7-8951-6ce0252cf0b9
📒 Files selected for processing (5)
qa/library_metrics.pyqa/nightly_harvest.pyqa/scores_db.pyqa/test_library_metrics.pyqa/test_nightly_harvest.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test
- GitHub Check: viewer-tests
- GitHub Check: viewer-tests
- GitHub Check: test
⚠️ CI failures not shown inline (2)
GitHub Actions: LLM Quality Gate (advisory) / quality-gate: [HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
GitHub Actions: LLM Quality Gate (advisory) / 0_quality-gate.txt: [HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
🧰 Additional context used
🪛 ast-grep (0.44.1)
qa/library_metrics.py
[info] 240-240: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
qa/test_nightly_harvest.py
[info] 58-58: use jsonify instead of json.dumps for JSON output
Context: json.dumps(obj, indent=2, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 63-63: use jsonify instead of json.dumps for JSON output
Context: json.dumps(r)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 214-214: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_nom(f"quest:bg:q{i}", p))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 228-228: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_nom(f"quest:bg:q{i}", p))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 346-346: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_nom(f"quest:bg:q{i}", p))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 388-388: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_nom("quest:bg:q1", p))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
qa/scores_db.py
[info] 567-567: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_jv, ensure_ascii=False, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1455-1455: use jsonify instead of json.dumps for JSON output
Context: json.dumps(fetch_library_metrics(db), indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
qa/nightly_harvest.py
[info] 141-141: use jsonify instead of json.dumps for JSON output
Context: json.dumps(rec, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 286-286: use jsonify instead of json.dumps for JSON output
Context: json.dumps(report, indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
qa/test_library_metrics.py
[info] 137-137: use jsonify instead of json.dumps for JSON output
Context: json.dumps(entry)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 211-211: use jsonify instead of json.dumps for JSON output
Context: json.dumps(l)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 260-260: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"artifact_id": "a1", "verdict": "promoted"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.23.0)
qa/scores_db.py
[ERROR] 354-356: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 578-578: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.15.20)
qa/library_metrics.py
[warning] 94-94: Unnecessary dict comprehension for iterable; use dict.fromkeys instead
Replace with dict.fromkeys(iterable))
(C420)
[warning] 153-153: for loop variable line overwritten by assignment target
(PLW2901)
qa/test_nightly_harvest.py
[warning] 42-42: Missing type annotation for **payload_extra
(ANN003)
[warning] 90-90: Missing return type annotation for private function _fake
(ANN202)
[warning] 90-90: Unused function argument: budget
(ARG001)
[warning] 107-107: Missing return type annotation for private function _fake
Add return type annotation: NoReturn
(ANN202)
[warning] 107-107: Missing type annotation for *a
(ANN002)
[warning] 107-107: Unused function argument: a
(ARG001)
[warning] 107-107: Missing type annotation for **k
(ANN003)
[warning] 107-107: Unused function argument: k
(ARG001)
[warning] 108-108: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 112-112: Missing return type annotation for private function _run
(ANN202)
[warning] 112-112: Missing type annotation for **kw
(ANN003)
[warning] 241-241: Unused function argument: capsys
(ARG001)
[warning] 285-285: Unused function argument: failing_panel
(ARG001)
[warning] 305-305: Missing return type annotation for private function _flaky
(ANN202)
[warning] 305-305: Missing type annotation for **kw
(ANN003)
[warning] 308-308: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 343-343: Unused function argument: fake_panel
(ARG001)
[warning] 358-358: Unused function argument: fake_panel
(ARG001)
[error] 364-364: Ambiguous variable name: l
(E741)
[warning] 371-371: Unused function argument: failing_panel
(ARG001)
[error] 377-377: Ambiguous variable name: l
(E741)
[warning] 386-386: Unused function argument: fake_panel
(ARG001)
[warning] 402-402: Unused function argument: fake_panel
(ARG001)
qa/scores_db.py
[warning] 544-544: Dynamically typed expressions (typing.Any) are disallowed in **fields
(ANN401)
[warning] 558-560: Avoid specifying long messages outside the exception class
(TRY003)
[error] 578-578: Possible SQL injection vector through string-based query construction
(S608)
qa/nightly_harvest.py
[warning] 106-106: for loop variable line overwritten by assignment target
(PLW2901)
[warning] 155-155: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 159-162: Avoid specifying long messages outside the exception class
(TRY003)
qa/test_library_metrics.py
[warning] 65-65: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 127-127: Missing type annotation for **extra
(ANN003)
[error] 212-212: Ambiguous variable name: l
(E741)
[warning] 372-372: Unused function argument: capsys
(ARG001)
🔇 Additional comments (11)
qa/nightly_harvest.py (4)
94-115: LGTM!
121-163: LGTM!
226-254: LGTM!
260-292: LGTM!qa/test_nightly_harvest.py (1)
42-410: LGTM!qa/scores_db.py (3)
254-293: LGTM!Also applies to: 318-358
886-948: LGTM!
1389-1397: LGTM!Also applies to: 1443-1469, 1496-1498
qa/library_metrics.py (2)
83-132: LGTM!Also applies to: 138-170, 176-215, 221-247
70-77: 🗄️ Data Integrity & IntegrationDrop the tier-drift warning.
_CLASS_SUBDIRSandPROCESSED_LOG_NAMEmatchtools/library/promote.py;_VALID_TIERSis only a local metrics bucket list, not a promotion-side contract. Confidence 94%.> Likely an incorrect or invalid review comment.qa/test_library_metrics.py (1)
1-393: LGTM!
| # De-dup by artifact_id within this batch's candidate list (a nominations.jsonl may carry the | ||
| # same artifact_id more than once across separate nominate.py runs) — score it at most once. | ||
| seen: set[str] = set() | ||
| candidates: list[dict] = [] | ||
| for nom in noms: | ||
| aid = nom["artifact_id"] | ||
| if aid in already_scored or aid in seen: | ||
| continue | ||
| seen.add(aid) | ||
| candidates.append(nom) | ||
|
|
||
| work = candidates[:max_per_run] | ||
| remaining = len(candidates) - len(work) | ||
|
|
||
| report: dict[str, Any] = { | ||
| "nominations_total": len(noms), | ||
| "already_scored": len(noms) - len(candidates) if noms else 0, | ||
| "candidates_unscored": len(candidates), | ||
| "scored": 0, | ||
| "load_failed": 0, | ||
| "score_failed": 0, | ||
| "remaining_for_next_run": max(remaining, 0), | ||
| "dry_run": dry_run, | ||
| "details": [], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files qa/nightly_harvest.py
git ls-files | rg '(^|/)test|(^|/)tests|nightly_harvest|harvest'
printf '\n== outline qa/nightly_harvest.py ==\n'
ast-grep outline qa/nightly_harvest.py --view expanded || true
printf '\n== relevant lines qa/nightly_harvest.py ==\n'
wc -l qa/nightly_harvest.py
sed -n '160,240p' qa/nightly_harvest.py | cat -n
printf '\n== search tests ==\n'
rg -n "duplicate_artifact_id_in_queue_scored_once|already_scored|candidates_unscored|remaining_for_next_run|nominations_total" -S .Repository: electricsheephq/WorldOS
Length of output: 18459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== qa/test_nightly_harvest.py around relevant tests ==\n'
sed -n '120,245p' qa/test_nightly_harvest.py | cat -n
printf '\n== tiny behavioral probe ==\n'
python3 - <<'PY'
noms = [{"artifact_id": "a"}, {"artifact_id": "a"}, {"artifact_id": "b"}]
already_scored = set()
seen = set()
candidates = []
for nom in noms:
aid = nom["artifact_id"]
if aid in already_scored or aid in seen:
continue
seen.add(aid)
candidates.append(nom)
print({
"nominations_total": len(noms),
"candidates": len(candidates),
"already_scored_metric": len(noms) - len(candidates),
"duplicate_count": len(noms) - len(set(n["artifact_id"] for n in noms)),
})
PYRepository: electricsheephq/WorldOS
Length of output: 6161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\balready_scored\b" -S qa servers viewer . | sed -n '1,200p'Repository: electricsheephq/WorldOS
Length of output: 1185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\balready_scored\b" -S qa servers viewer . | sed -n '1,220p'Repository: electricsheephq/WorldOS
Length of output: 1185
Fix already_scored accounting at qa/nightly_harvest.py:211 (confidence 95%) already_scored is computed as len(noms) - len(candidates), but candidates also drops intra-batch duplicates via seen. That makes the report overstate “already scored” whenever the same unscored artifact_id appears twice; the batch still scores it once, yet the summary claims one was pre-scored. Count the already_scored filter separately, or split duplicate-dedup from pre-scored skips into distinct metrics.
🤖 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 `@qa/nightly_harvest.py` around lines 195 - 219, The report’s already_scored
value is being derived from the post-dedup candidates list in the nightly
harvest flow, which mixes pre-scored skips with intra-batch duplicate removal.
Update the logic around the noms/candidates processing in nightly_harvest so
already_scored counts only artifact_id values filtered by already_scored, and
track the seen-based de-dup separately for the report. Keep the candidates and
report fields in sync so the summary reflects true pre-scored items without
double-counting duplicate nominations.
| # --------------------------------------------------------------------------- | ||
| # The one append helper every future LIBRARY_METRICS snapshot should call (HV5 slice 2, #1327) | ||
| # --------------------------------------------------------------------------- | ||
| def add_library_metrics( | ||
| *, | ||
| db_path: Path | str = DB_PATH, | ||
| **fields: Any, | ||
| ) -> int: | ||
| """Append ONE library-health snapshot row to the additive `library_metrics` table. | ||
|
|
||
| Mirrors :func:`add_run` / :func:`add_artifact`'s validation discipline, but writes the SEPARATE | ||
| `library_metrics` table and NEVER touches `runs` or `artifacts`. Pass any subset of | ||
| :data:`LIBRARY_METRICS_COLUMNS` as keyword args. Unknown keys raise (a typo is caught, not | ||
| silently dropped). ``size_by_class_json`` / ``size_by_tier_json`` may be passed as dicts and are | ||
| JSON-encoded automatically. ``ts`` defaults to now (UTC, ISO8601) if omitted. Unlike ``runs``/ | ||
| ``artifacts``, there is no caller-supplied id — every call INSERTS a new row (a snapshot never | ||
| replaces a prior one; the row id is autoincrement). Returns the new row's integer id. | ||
| """ | ||
| unknown = set(fields) - set(LIBRARY_METRICS_COLUMNS) | ||
| if unknown: | ||
| raise ValueError( | ||
| f"unknown field(s) {sorted(unknown)}; valid: {sorted(LIBRARY_METRICS_COLUMNS)}" | ||
| ) | ||
|
|
||
| if fields.get("ts") is None: | ||
| fields["ts"] = datetime.now(timezone.utc).isoformat(timespec="seconds") | ||
|
|
||
| for _jcol in ("size_by_class_json", "size_by_tier_json"): | ||
| _jv = fields.get(_jcol) | ||
| if _jv is not None and not isinstance(_jv, str): | ||
| fields[_jcol] = json.dumps(_jv, ensure_ascii=False, sort_keys=True) | ||
|
|
||
| cols = [c for c in LIBRARY_METRICS_COLUMNS if c in fields] | ||
| vals = [fields[c] for c in cols] | ||
| placeholders = ", ".join("?" for _ in cols) | ||
| quoted = ", ".join(f'"{c}"' for c in cols) | ||
|
|
||
| own = isinstance(db_path, (str, os.PathLike)) | ||
| conn = connect(db_path) if own else db_path # type: ignore[arg-type] | ||
| try: | ||
| cur = conn.execute(f"INSERT INTO library_metrics ({quoted}) VALUES ({placeholders})", vals) | ||
| conn.commit() | ||
| return int(cur.lastrowid) | ||
| finally: | ||
| if own: | ||
| conn.close() | ||
|
|
||
|
|
||
| def fetch_library_metrics(db_path: Path | str = DB_PATH) -> list[dict]: | ||
| """Return all `library_metrics` snapshot rows as dicts, newest-first (ts desc, then id desc).""" | ||
| conn = connect(db_path) | ||
| try: | ||
| rows = conn.execute( | ||
| "SELECT * FROM library_metrics ORDER BY ts DESC, id DESC" | ||
| ).fetchall() | ||
| return [dict(r) for r in rows] | ||
| finally: | ||
| conn.close() | ||
|
|
||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
No range guard on the two percent-shaped columns (confidence: 75%).
add_library_metrics() validates unknown keys but never validates that promotion_pass_rate / pct_library_sourced fall inside 0.0-1.0. Both CLI entrypoints that ultimately funnel into this function (scores_db.py's --libmetric-promotion-pass-rate/--libmetric-pct-library-sourced at Line 1446-1451, and library_metrics.py's --pct-library-sourced at Line 228-229) only do a bare float() cast — a fat-fingered 50 instead of 0.5 silently persists as a nonsensical 5000% in the ledger and downstream trend charts (render_library_metrics_markdown at Line 940 would print "5000%"), with no error raised anywhere in the write path. Root cause is here, not at either CLI layer, so fixing here covers both call sites.
🛡️ Proposed fix
for _jcol in ("size_by_class_json", "size_by_tier_json"):
_jv = fields.get(_jcol)
if _jv is not None and not isinstance(_jv, str):
fields[_jcol] = json.dumps(_jv, ensure_ascii=False, sort_keys=True)
+ for _pcol in ("promotion_pass_rate", "pct_library_sourced"):
+ _pv = fields.get(_pcol)
+ if _pv is not None and not (0.0 <= float(_pv) <= 1.0):
+ raise ValueError(f"{_pcol} must be in [0.0, 1.0], got {_pv!r}")
+
cols = [c for c in LIBRARY_METRICS_COLUMNS if c in fields]📝 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.
| # --------------------------------------------------------------------------- | |
| # The one append helper every future LIBRARY_METRICS snapshot should call (HV5 slice 2, #1327) | |
| # --------------------------------------------------------------------------- | |
| def add_library_metrics( | |
| *, | |
| db_path: Path | str = DB_PATH, | |
| **fields: Any, | |
| ) -> int: | |
| """Append ONE library-health snapshot row to the additive `library_metrics` table. | |
| Mirrors :func:`add_run` / :func:`add_artifact`'s validation discipline, but writes the SEPARATE | |
| `library_metrics` table and NEVER touches `runs` or `artifacts`. Pass any subset of | |
| :data:`LIBRARY_METRICS_COLUMNS` as keyword args. Unknown keys raise (a typo is caught, not | |
| silently dropped). ``size_by_class_json`` / ``size_by_tier_json`` may be passed as dicts and are | |
| JSON-encoded automatically. ``ts`` defaults to now (UTC, ISO8601) if omitted. Unlike ``runs``/ | |
| ``artifacts``, there is no caller-supplied id — every call INSERTS a new row (a snapshot never | |
| replaces a prior one; the row id is autoincrement). Returns the new row's integer id. | |
| """ | |
| unknown = set(fields) - set(LIBRARY_METRICS_COLUMNS) | |
| if unknown: | |
| raise ValueError( | |
| f"unknown field(s) {sorted(unknown)}; valid: {sorted(LIBRARY_METRICS_COLUMNS)}" | |
| ) | |
| if fields.get("ts") is None: | |
| fields["ts"] = datetime.now(timezone.utc).isoformat(timespec="seconds") | |
| for _jcol in ("size_by_class_json", "size_by_tier_json"): | |
| _jv = fields.get(_jcol) | |
| if _jv is not None and not isinstance(_jv, str): | |
| fields[_jcol] = json.dumps(_jv, ensure_ascii=False, sort_keys=True) | |
| cols = [c for c in LIBRARY_METRICS_COLUMNS if c in fields] | |
| vals = [fields[c] for c in cols] | |
| placeholders = ", ".join("?" for _ in cols) | |
| quoted = ", ".join(f'"{c}"' for c in cols) | |
| own = isinstance(db_path, (str, os.PathLike)) | |
| conn = connect(db_path) if own else db_path # type: ignore[arg-type] | |
| try: | |
| cur = conn.execute(f"INSERT INTO library_metrics ({quoted}) VALUES ({placeholders})", vals) | |
| conn.commit() | |
| return int(cur.lastrowid) | |
| finally: | |
| if own: | |
| conn.close() | |
| def fetch_library_metrics(db_path: Path | str = DB_PATH) -> list[dict]: | |
| """Return all `library_metrics` snapshot rows as dicts, newest-first (ts desc, then id desc).""" | |
| conn = connect(db_path) | |
| try: | |
| rows = conn.execute( | |
| "SELECT * FROM library_metrics ORDER BY ts DESC, id DESC" | |
| ).fetchall() | |
| return [dict(r) for r in rows] | |
| finally: | |
| conn.close() | |
| # --------------------------------------------------------------------------- | |
| # The one append helper every future LIBRARY_METRICS snapshot should call (HV5 slice 2, `#1327`) | |
| # --------------------------------------------------------------------------- | |
| def add_library_metrics( | |
| *, | |
| db_path: Path | str = DB_PATH, | |
| **fields: Any, | |
| ) -> int: | |
| """Append ONE library-health snapshot row to the additive `library_metrics` table. | |
| Mirrors :func:`add_run` / :func:`add_artifact`'s validation discipline, but writes the SEPARATE | |
| `library_metrics` table and NEVER touches `runs` or `artifacts`. Pass any subset of | |
| :data:`LIBRARY_METRICS_COLUMNS` as keyword args. Unknown keys raise (a typo is caught, not | |
| silently dropped). ``size_by_class_json`` / ``size_by_tier_json`` may be passed as dicts and are | |
| JSON-encoded automatically. ``ts`` defaults to now (UTC, ISO8601) if omitted. Unlike ``runs``/ | |
| ``artifacts``, there is no caller-supplied id — every call INSERTS a new row (a snapshot never | |
| replaces a prior one; the row id is autoincrement). Returns the new row's integer id. | |
| """ | |
| unknown = set(fields) - set(LIBRARY_METRICS_COLUMNS) | |
| if unknown: | |
| raise ValueError( | |
| f"unknown field(s) {sorted(unknown)}; valid: {sorted(LIBRARY_METRICS_COLUMNS)}" | |
| ) | |
| if fields.get("ts") is None: | |
| fields["ts"] = datetime.now(timezone.utc).isoformat(timespec="seconds") | |
| for _jcol in ("size_by_class_json", "size_by_tier_json"): | |
| _jv = fields.get(_jcol) | |
| if _jv is not None and not isinstance(_jv, str): | |
| fields[_jcol] = json.dumps(_jv, ensure_ascii=False, sort_keys=True) | |
| for _pcol in ("promotion_pass_rate", "pct_library_sourced"): | |
| _pv = fields.get(_pcol) | |
| if _pv is not None and not (0.0 <= float(_pv) <= 1.0): | |
| raise ValueError(f"{_pcol} must be in [0.0, 1.0], got {_pv!r}") | |
| cols = [c for c in LIBRARY_METRICS_COLUMNS if c in fields] | |
| vals = [fields[c] for c in cols] | |
| placeholders = ", ".join("?" for _ in cols) | |
| quoted = ", ".join(f'"{c}"' for c in cols) | |
| own = isinstance(db_path, (str, os.PathLike)) | |
| conn = connect(db_path) if own else db_path # type: ignore[arg-type] | |
| try: | |
| cur = conn.execute(f"INSERT INTO library_metrics ({quoted}) VALUES ({placeholders})", vals) | |
| conn.commit() | |
| return int(cur.lastrowid) | |
| finally: | |
| if own: | |
| conn.close() | |
| def fetch_library_metrics(db_path: Path | str = DB_PATH) -> list[dict]: | |
| """Return all `library_metrics` snapshot rows as dicts, newest-first (ts desc, then id desc).""" | |
| conn = connect(db_path) | |
| try: | |
| rows = conn.execute( | |
| "SELECT * FROM library_metrics ORDER BY ts DESC, id DESC" | |
| ).fetchall() | |
| return [dict(r) for r in rows] | |
| finally: | |
| conn.close() |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 567-567: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_jv, ensure_ascii=False, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.23.0)
[ERROR] 578-578: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 Ruff (0.15.20)
[warning] 544-544: Dynamically typed expressions (typing.Any) are disallowed in **fields
(ANN401)
[warning] 558-560: Avoid specifying long messages outside the exception class
(TRY003)
[error] 578-578: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@qa/scores_db.py` around lines 538 - 597, Add range validation inside
add_library_metrics for the percent-style fields promotion_pass_rate and
pct_library_sourced so invalid values cannot be persisted. Before building the
INSERT, check any provided values for these keys are numeric and within 0.0 to
1.0 inclusive, and raise a ValueError with a clear message if not. Keep the
validation in add_library_metrics so both CLI paths that feed it are covered,
and leave the existing unknown-field handling and JSON coercion unchanged.
| def test_duplicate_artifact_id_in_queue_scored_once(env, fake_panel): | ||
| """The same artifact_id nominated twice (two separate nominate.py runs appending to the same | ||
| queue) must be scored — and billed — at most once per batch.""" | ||
| p = _artifact_json(env["artifacts_out"], "quest:bg:q1") | ||
| _write_noms(env["noms"], [_nom("quest:bg:q1", p), _nom("quest:bg:q1", p)]) | ||
|
|
||
| report = _run(env) | ||
|
|
||
| assert report["scored"] == 1 | ||
| assert fake_panel == ["quest:bg:q1"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Coverage gap that hides the already_scored metric bug (confidence ~80%).
This is the one test that exercises duplicate artifact_ids, but it only asserts scored == 1. Add an assertion on already_scored (expected 0 — nothing was pre-scored, just deduped) and it will fail against the current harvest_batch arithmetic, catching the reporting defect flagged in nightly_harvest.py Line 211.
💚 Suggested assertion
assert report["scored"] == 1
+ assert report["already_scored"] == 0 # a de-duped nomination is NOT "already scored"
assert fake_panel == ["quest:bg:q1"]📝 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.
| def test_duplicate_artifact_id_in_queue_scored_once(env, fake_panel): | |
| """The same artifact_id nominated twice (two separate nominate.py runs appending to the same | |
| queue) must be scored — and billed — at most once per batch.""" | |
| p = _artifact_json(env["artifacts_out"], "quest:bg:q1") | |
| _write_noms(env["noms"], [_nom("quest:bg:q1", p), _nom("quest:bg:q1", p)]) | |
| report = _run(env) | |
| assert report["scored"] == 1 | |
| assert fake_panel == ["quest:bg:q1"] | |
| def test_duplicate_artifact_id_in_queue_scored_once(env, fake_panel): | |
| """The same artifact_id nominated twice (two separate nominate.py runs appending to the same | |
| queue) must be scored — and billed — at most once per batch.""" | |
| p = _artifact_json(env["artifacts_out"], "quest:bg:q1") | |
| _write_noms(env["noms"], [_nom("quest:bg:q1", p), _nom("quest:bg:q1", p)]) | |
| report = _run(env) | |
| assert report["scored"] == 1 | |
| assert report["already_scored"] == 0 # a de-duped nomination is NOT "already scored" | |
| assert fake_panel == ["quest:bg:q1"] |
🤖 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 `@qa/test_nightly_harvest.py` around lines 199 - 208, The duplicate-artifact
test in test_duplicate_artifact_id_in_queue_scored_once only checks
report["scored"], so it misses the already_scored reporting bug. Update this
test to also assert report["already_scored"] is 0 after _run(env), using the
existing harvest_batch/report output and the same duplicate nomination setup, so
the metric regression is caught when duplicates are deduped rather than
pre-scored.
evaOS review status: completedPR: #1369 - [HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1369 Review URL: #1369 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #1369 - [HV5] nightly harvest scoring + library_metrics (the flywheel's own eval) (#1327)
Head: 435c73c3c10c9800f0ae08d27b469418194f498a into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~60 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
qa/library_metrics.py |
added | +251/-0 | Changed file | Elevated: large change |
qa/nightly_harvest.py |
added | +292/-0 | Changed file | Moderate: validated P3 finding |
qa/scores_db.py |
modified | +215/-1 | Changed file | Elevated: large change |
qa/test_library_metrics.py |
added | +393/-0 | Test coverage | Elevated: large change |
qa/test_nightly_harvest.py |
added | +409/-0 | Test coverage | Elevated: large change |
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 0, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Runtime correctness: 2
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - WorldOS repo profile implies Unity runtime risk. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #1327, #1342.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| Pure w.r.t. everything except: (a) new `artifacts` rows via score_artifact_panel for each | ||
| artifact actually scored, and (b) an appended line per attempt in ``log_path`` — neither happens | ||
| under ``dry_run`` (a pure preview of the capped work list, writing nothing, scoring nothing). | ||
|
|
There was a problem hiding this comment.
P3: dry-run initializes/creates the scores.db file despite "writes nothing" contract
harvest_batch() computes already_scored = _scored_ids(db_path) unconditionally, before the if dry_run: return guard. _scored_ids -> scores_db.fetch_artifacts -> connect() -> _ensure_schema(), which runs CREATE TABLE IF NOT EXISTS for runs/artifacts/library_metrics and commits. So a --dry-run invocation that targets a non-existent db_path will CREATE and schema-initialize that db file on disk. The module docstring and harvest_batch docstring both promise dry-run "writes nothing (no scoring call, no db row, no log line)". The existing test (test_dry_run_reports_would_score_and_writes_nothing) only asserts len(fetch_artifacts(...)) == 0 (zero rows) and not env["log"].exists(), so it does not catch the db-file creation; it would pass even though the contract is partially violated. Move the _scored_ids(db_path) call to after the dry-run early return (dry-run does not need the already-scored set to report would_score, only candidates_unscored/remaining_for_next_run, which are derivable from noms alone), or document that dry-run may initialize an empty db file.
Category: Runtime correctness
Why this matters: A nightly operator running --dry-run as a pure preview against a fresh path (e.g. a temp/scratch db, or to validate the work list without side effects) will silently get a new scores.db created on disk. For an unattended cron that uses dry-run as a guard, this is an unexpected side effect that contradicts the stated "writes nothing" invariant and could surprise operators who rely on dry-run being truly side-effect-free.
|
|
||
| report: dict[str, Any] = { | ||
| "nominations_total": len(noms), | ||
| "already_scored": len(noms) - len(candidates) if noms else 0, |
There was a problem hiding this comment.
P3: already_scored report counter conflates already-scored with intra-batch duplicates
already_scored is computed as len(noms) - len(candidates) when noms is non-empty. But candidates excludes BOTH already-scored ids AND intra-batch duplicates (the seen set at line 197-204 dedups). So if nominations.jsonl contains the same unscored artifact_id twice, already_scored is inflated by the duplicate count, misreporting how many nominations were truly already scored. For example, two unscored nominations for the same id yields already_scored=1 even though neither was previously scored. This only affects the diagnostic report (not scoring/billing — the dedup is correct and the duplicate test test_duplicate_artifact_id_in_queue_scored_once does not assert on already_scored), so impact is limited to trend/dashboards reading the report.
Category: Runtime correctness
Why this matters: The nightly report is the operator's view into queue health (how backed up the scoring lane is). An inflated already_scored makes it look like more artifacts have been scored than actually have, which can mask a stuck/behind scoring lane in trend monitoring.
1 participant
What (HV5 epic #1327, slices 2+3 of the flywheel-ops sprint)
Builds on slice 1 (#1342, closeout auto-nomination →
qa/nominations.jsonl, merged). This PR shipsthe two remaining core slices; the weekly-curation stub (item 3 in the dispatch) was deprioritized
in favor of full test depth on 2+3 — flagged below, not silently dropped.
Slice 2 — nightly batch scoring (
qa/nightly_harvest.py)Reads the accumulated
qa/nominations.jsonl, finds nominations whoseartifact_idhas no scoredartifactsrow yet (same "unscored" definitionpromote.pyuses: row missing ORoverall is None),and scores each via HV1's plain callable entrypoint
qa/artifact_score.score_artifact_panel— theSAME seam
promote.py'sscore_if_unscoredcalls, so there is exactly one place a live LLM panelcall happens anywhere in the harvest loop.
is the separate, off-line scoring step.
--max-per-run(default 20) caps one invocation; the remainder waits for the nextnightly tick.
artifactstable itself is the "already scored" source of truth(mirrors
promote.py); nothing is double-scored or double-billed across runs.source_path, id mismatch) or scoringfailure (score.sh sentinel/exception) is caught, logged to its own resumable progress log
(
qa/nightly_harvest_log.jsonl, separate fromlibrary/.promoted.jsonl), and retried next run —never aborts the batch.
--dry-runpreviews the capped work list; scores nothing, writes nothing.Slice 3 —
library_metricstable (the flywheel's own eval)Additive
library_metricstable inqa/scores.db, added viaqa/scores_db.py(the sole schema/writer surface —
add_library_metrics/fetch_library_metrics/render_library_metrics_markdown,mirroring the
artifactstable's own pattern). One row per SNAPSHOT (time-series, not a replace-by-key like
runs/artifacts):size_total,size_by_class_json,size_by_tier_json,reuse_count_sum(Σ reuse_count),promotion_pass_rate(+promoted_total/rejected_total), andpct_library_sourced(leftNone— HV4 wiring doesn't exist yet in this repo state; an honest"not measured" rather than a guess).
qa/library_metrics.py'ssnapshot_library()is the sole writer: a pure, read-only scan oflibrary/**/*.json+library/.promoted.jsonl, then oneadd_library_metricscall. Never a secondwriter of
library/—promote.pystays its sole writer; this only reads it.Deferred (flagged, not silently dropped)
Item 3 (weekly-curation stub listing stable-tier entries eligible for canonical review) was not
built in this PR — prioritized full offline test depth on slices 2+3 instead. Follow-up scoped
separately if wanted; canonical promotion stays human-only regardless.
Offline-safety
Every test in
qa/test_nightly_harvest.pymonkeypatchesartifact_score.score_artifact_panelto afabricated stub (mirrors
test_promote_pipeline.py'stest_score_if_unscored_is_isolated_from_promotion_pathdiscipline) — no test invokes score.sh or a live
claude -p.qa/test_library_metrics.pyis purefilesystem + sqlite over fabricated
tmp_pathtrees. Nightly runs (a live LLM panel) are theorchestrator's job, never CI/pytest.
Invariants held
library_metricsis a new, separate table —runsandartifactsschemasuntouched (confirmed via
detect_changes: onlyscores_db.py's schema-ensure chain is touched,same blast radius HV1's
artifactstable addition already established).qa/scores_db.py:add_library_metricsis the sole writer;nightly_harvest.pynever hand-rolls SQL (it callsartifact_score.record_artifact_scoreinternally via
score_artifact_panel, the existing writer).promote.pystays sole writer oflibrary/: both new modules are read-only over it.nightly_harvest.pyisnever invoked from
run_duo.sh/nominate.py's hot-path hook.chk(): these are harvest-ops readers/writers, not behavioral-gate checks — nothingregistered in
BEHAVIORAL_GATE_TAXONOMY.json/gate_corpus/manifest.json.Tests
qa/test_nightly_harvest.py— 21 offline tests: scores-only-unscored, idempotency, resumabilityacross capped runs, bounded
--max-per-run(incl. CLI rejection of a negative cap), non-fatalload/score-failure isolation + retry-next-run, dry-run (zero scorer calls), malformed-line
tolerance, never touches
library/.qa/test_library_metrics.py— 28 offline tests:scores_db.pyschema/round-trip/render (incl.cross-table isolation from
runs/artifacts),scan_library(size/tier/reuse counting, malformed-entry handling),
scan_promotion_log(pass-rate math, gated-vs-non-gated verdicts),snapshot_libraryend-to-end against a known fabricated
library/state, CLI dry-run/render.-p no:xdist). Full regression: 212 passed, 1 pre-existingskip across
test_nominate.py/test_promote_pipeline.py/test_scores_db.py/test_artifact_evals.py/test_scores_db_comparability.py/test_closeout.py.qa/fast_gate.shgreen — 257 passed, $0 deterministic.python3 tools/library/library_lint.py— clean.Flags
pct_library_sourcedstays unset until HV4 (questgen._derive_hookslibrary candidate source)wires per-run attribution — today's expected state, not a bug (documented in the column comment
and the rendered ledger's blockquote).
qa/library_metrics_ledger.mdis intentionally NOT committed here (mirrorsqa/nominations.jsonl'sown precedent in slice 1 — a runtime-generated artifact, not founding-PR content); it materializes
on the first real
snapshot_library()run.Do NOT merge — orchestrator review.