fix(skills): compare skill review findings against base ref baseline

[Battleplus]

Why

Closes #4996

The skill review CI gate (review_changed_public_skills.py) reviews the entire public skill package when any file in it changes. Pre-existing findings in unchanged files block the PR even though they were not introduced by the change.

For example, PR #4995 changes one file in skill-creator, but the CI fails on 4 pre-existing python-subprocess / secret-env-assignment findings in other unchanged files.

What changed

When a base ref is provided (--base-ref/--before), the script now:

1. Extracts the package at the base ref using git archive → temp directory
2. Collects baseline findings by running the review CLI with --format json on the base version
3. Collects head findings on the current working tree version
4. Diffs findings by (path, rule_id, line, message) — only newly introduced findings cause failure
5. Pre-existing findings are logged for visibility but do not block

When no base ref is provided (e.g., direct pushes without comparison), the original behavior is preserved.

Validation

• ruff check + ruff format --check pass
• finding_key, extract_package_at_ref, run_review_json all verified against live repo
• Baseline extraction tested with skills/public/bootstrap at origin/main

Surface area

• CI / scripts
• Backend API
• Frontend UI
• Agents / LangGraph
• Sandbox
• Skills (runtime)

[WillemJiang]

@Battleplus please fix the unit test error.

[Battleplus]

Fixed. The 7 failing tests in test_review_changed_public_skills.py were asserting subprocess.run call patterns that didn't account for the new extract_package_at_ref function (which calls git archive for baseline extraction).

Changes: mocked extract_package_at_ref and run_review_json in all tests that trigger baseline comparison via --before/--after or --base-ref/--head-ref.

CI should be green on the new push.

[willem-bd]

The baseline-diffing direction is right — re-blocking PRs on pre-existing findings in unchanged files (as in #4995) is a real CI problem, and fail-closed behavior when the baseline can't be extracted is the correct default. I have four findings on the implementation: the head-side review now fails open on a CLI crash, the baseline path silently drops the --fail-on error / --fail-on-incomplete gate semantics, finding_key is fragile against line shifts in edited files, and the base-run env prepends a nonexistent harness path. Also noting: the new baseline code paths (extract → diff → new-findings fail / pre-existing pass / baseline-unavailable) have no test coverage — the test changes only stub the new functions so the old tests keep passing, and backend TDD is mandatory in this repo per AGENTS.md.

[WillemJiang]

@Battleplus, some unit tests failed; please check them.

[Battleplus]

Fixed the two issues in this push:

1. Harness path: Changed to . The base review now uses the real repo root for , so PYTHONPATH points to the genuine directory instead of a nonexistent path inside the temp extraction directory.

2. Temp cleanup: Changed to and added a loop that walks up from the extraction root, removing empty parent directories until it finds a non-empty one.

3. Finding key stability (bonus): Dropped from — pre-existing findings that shift due to unrelated insertions/deletions are no longer falsely flagged as new. Path + rule_id + message is a stable identity.

[Battleplus]

Addressed all 4 review threads with a substantive rework of the baseline gate (pushed as 4c8d1de):

1. Head-side review fails open (Important): Fixed. run_review_json now returns the full facts dict or None. None means the review itself failed (CLI nonzero + no stdout, invalid JSON, empty output) and is treated as REVIEW FAILED — the gate exits 1. It is never conflated with "no findings".

2. Baseline mode silently changes gate semantics (Important): Fixed. The baseline path now preserves the original --fail-on error severity semantics — only new blocker/error findings gate; warning/info findings do not. --fail-on-incomplete is also preserved: a head review whose completeness reports not_assessed fails even with no new gating finding.

3. line/message in finding key (Important): Line numbers are already excluded; the key is (path, rule_id, message). Added a regression test asserting a finding at base line 20 vs head line 30 (line shift) is treated as pre-existing.

4. Nonexistent harness path in base env (Suggestion): Fixed in an earlier push (608aa49) — base review now uses the real repo_root for env/PYTHONPATH. Additionally found and fixed a latent crash: run_review_json was calling package.relative_to(repo_root) on the temp-extracted base package (outside repo_root), which raised ValueError whenever a baseline existed. The CLI now receives str(package) (absolute path, which the CLI accepts) while cwd/env still come from the real repo root.

Regression tests added: head crash → fail closed, invalid JSON → fail closed, unchanged finding → pass, line-shifted finding → pass (exempt), new error → fail, new info → pass (severity preserved), incomplete head → fail, baseline review crash → all findings treated as new.

[Battleplus]

Hi @WillemJiang, I've verified the review script tests locally — all 21 tests in test_review_changed_public_skills.py require the full backend dependency chain (langchain, deerflow agents, etc.) which is installed via uv sync --group dev in CI.\n\nThe CI workflows for this PR are currently in action_required state (pending approval for fork PR). Could you please:\n1. Approve the CI workflows so they can run, or\n2. Share the specific test failure output if you've already seen it?\n\nThe core logic changes (fail-closed on crash, finding_key excluding line numbers, severity gate preservation) are tested by the 21 test cases in the file. I want to make sure I'm fixing the right issue.

[willem-bd]

Round 2 - all four prior findings verified fixed at this head: (1) head review now fails closed (run_review_json returns None on crash/invalid output and main() treats it as failure), (2) finding_gates() + review_incomplete() restore the original --fail-on error / --fail-on-incomplete semantics with tests for both directions, (3) finding_key drops line (keeping message, with the rationale documented), and (4) the base review now runs with the real repo root for cwd/env. The new test suite covering the baseline paths is a solid addition. One residual nit below on the temp-directory cleanup introduced in 608aa49.

[willem-bd]

[Nit] The temp-dir cleanup climb is unbounded above the mkdtemp root and rmdir() is unguarded. The walk starts at base_pkg.parent and climbs while parents are empty, but it has no upper bound at the tempfile.mkdtemp root - if the shared $TMPDIR happens to be empty, it will rmdir the system temp dir itself (and in a minimal container could keep climbing). Also, tmp_root.rmdir() can raise an uncaught OSError via a TOCTOU race: another process sharing the temp dir creates a file between the iterdir() emptiness check and the rmdir(), and the whole gate then crashes with a traceback (fail-closed, so low impact, but noisy).

Cleaner fix: have extract_package_at_ref return (extracted, tmp_dir) (or attach the root) and shutil.rmtree(tmp_dir, ignore_errors=True) exactly what was created - no climb, no race on directories you don't own.