Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Battleplus wants to merge 4 commits into
base: main
Choose a base branch
from
+450 −18
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5467054
fix(skills): compare skill review findings against base ref baseline
Battleplus Aug 24, 2026
6c8ff73
fix(tests): mock baseline extraction in review script tests
Battleplus Aug 24, 2026
608aa49
fix(skills): pass real repo root for PYTHONPATH in base review env
Battleplus Aug 25, 2026
4c8d1de
fix(skills): make skill review gate fail-closed and preserve original…
Battleplus Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 218 additions & 0 deletions backend/tests/test_review_changed_public_skills.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def fail_review(*args, **kwargs):

monkeypatch.setattr(runner.subprocess, "run", fake_run)
monkeypatch.setattr(runner, "run_review", fail_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}})

exit_code = runner.main(
[
Expand Down Expand Up @@ -91,6 +93,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "run_review", fake_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: [])

exit_code = runner.main(
[
Expand Down Expand Up @@ -134,6 +138,8 @@ def fail_review(*args, **kwargs):

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "run_review", fail_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: [])

exit_code = runner.main(
[
Expand Down Expand Up @@ -184,6 +190,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "run_review", fake_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: [])

exit_code = runner.main(
[
Expand Down Expand Up @@ -222,6 +230,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "run_review", fake_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}})

exit_code = runner.main(
[
Expand Down Expand Up @@ -258,6 +268,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "run_review", fake_review)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}})

exit_code = runner.main(
[
Expand Down Expand Up @@ -301,6 +313,8 @@ def fake_run(command, **kwargs):
return _completed(command, returncode=1)

monkeypatch.setattr(runner.subprocess, "run", fake_run)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: [])

exit_code = runner.main(
[
Expand Down Expand Up @@ -339,6 +353,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:
return 0

monkeypatch.setattr(runner.subprocess, "run", fake_run)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: [])
monkeypatch.setattr(runner, "run_review", fake_review)

exit_code = runner.main(
Expand Down Expand Up @@ -375,6 +391,208 @@ def test_is_fully_removed_package_false_when_any_status_is_not_a_deletion(tmp_pa
assert runner.is_fully_removed_package(package_rel, ["D", "M"], tmp_path) is False



def _baseline_facts(*findings):
return {"findings": list(findings), "completeness": {}}


def _diff_stdout():
sep = bytes([0])
return b"M" + sep + b"skills/public/alpha/SKILL.md" + sep



def test_main_fails_closed_when_head_review_crashes(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: None)

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 1
assert "head review failed" in output


def test_main_fails_closed_when_head_review_returns_invalid_json(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: "not-a-dict")

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 1
assert "head review failed" in output


def test_main_passes_when_baseline_finding_unchanged(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()
finding = {
"path": "skills/public/alpha/SKILL.md",
"rule_id": "R001",
"line": 20,
"message": "pre-existing",
"severity": "error",
}

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: tmp_path / "base-pkg")
monkeypatch.setattr(
runner,
"run_review_json",
lambda *a, **kw: _baseline_facts(finding)
if "base-pkg" in str(a[0])
else _baseline_facts({**finding, "line": 30}),
)

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 0
assert "no new gating findings" in output


def test_main_fails_on_new_error_finding(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()
new_error = {
"path": "skills/public/alpha/SKILL.md",
"rule_id": "R002",
"line": 5,
"message": "new error",
"severity": "error",
}

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: _baseline_facts(new_error))

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 1
assert "NEW error R002" in output


def test_main_passes_on_new_info_finding_preserving_severity_gate(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()
new_info = {
"path": "skills/public/alpha/SKILL.md",
"rule_id": "R003",
"line": 5,
"message": "new info",
"severity": "info",
}

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: _baseline_facts(new_info))

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 0
assert "no new gating findings" in output


def test_main_fails_when_head_review_incomplete(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None)
monkeypatch.setattr(
runner,
"run_review_json",
lambda *a, **kw: {"findings": [], "completeness": {"not_assessed": ["SKILL.md"]}},
)

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 1
assert "review incomplete" in output


def test_main_treats_all_as_new_when_baseline_review_fails(tmp_path, monkeypatch, capsys):
_write_skill(tmp_path, "alpha")
diff_output = _diff_stdout()
head_error = {
"path": "skills/public/alpha/SKILL.md",
"rule_id": "R004",
"line": 5,
"message": "head error",
"severity": "error",
}

def fake_git_diff(command, **kwargs):
return _completed(command, stdout=diff_output)

monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: tmp_path / "base-pkg")
monkeypatch.setattr(
runner,
"run_review_json",
lambda *a, **kw: None
if "base-pkg" in str(a[0])
else _baseline_facts(head_error),
)

exit_code = runner.main(
["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]
)
output = capsys.readouterr().out
assert exit_code == 1
assert "baseline review failed" in output
assert "NEW error R004" in output


def test_finding_key_excludes_line_number():
base = {"path": "p", "rule_id": "R", "line": 20, "message": "m"}
head = {"path": "p", "rule_id": "R", "line": 30, "message": "m"}
assert runner.finding_key(base) == runner.finding_key(head)


def test_finding_gates_only_blocker_and_error():
assert runner.finding_gates({"severity": "error"}) is True
assert runner.finding_gates({"severity": "blocker"}) is True
assert runner.finding_gates({"severity": "warning"}) is False
assert runner.finding_gates({"severity": "info"}) is False
assert runner.finding_gates({}) is False

def test_is_zero_sha_requires_full_sha_length() -> None:
assert runner.is_zero_sha("0" * 40) is True
assert runner.is_zero_sha("0" * 64) is True
Expand Down
Loading