Skip to content

Commit 0816365

Browse files
shenxianpengclaude
andcommitted
fix: stop reporting a skipped run as a failure
Review found the same defect this PR set out to fix, one layer down. run_commit_check has always failed only on "fail", but add_job_summary, add_pr_comments (two call sites) and set_result_output each asked `all(scope.status == "pass")` instead. Those were equivalent only while pass and fail were the only statuses. With skip added, a skipped-only run rendered "⊘ All N checks skipped" and then exited 1 and emitted result status "fail" -- a bypassed policy turned into a broken build. Four copies of a two-state assumption is the same shape as the four copies of the reduce-to-overall rule in commit-check#537, so it gets the same remedy: overall_status() and exit_code_for(), defined once and used by every path. Three smaller findings, all real: * _markdown_table skipped only `pass` scopes, so a run with both a failure and a skip added a row with no value and no rule links -- a blank accusation under a "Failed checks" heading. It now takes failures only. * The all-skipped verdict fired on an empty result set, where `skipped == total` is trivially true, reporting "All 0 checks skipped". It now requires a scope, matching the guard the step log already had. * The step log said "all checks passed (2 skipped)" for a partial skip, contradicting its own report headline. It now reads "2 of 3 checks passed, 1 skipped". Each fix is pinned by a test that fails when the fix is reverted, checked one at a time. Not changed: the requirements.txt pin. The README documents 2.13.4 as the minimum for ⊘ to appear, and 2.13.4 does not exist yet -- commit-check#537 is still open. Pinning it now would install a version that cannot be resolved. The rendering is back-compatible by construction, so the bump belongs with the release, not here. The subject avoids "treat", which reads as imperative but is absent from the 396-verb list in commit-check 2.13.1 -- the version this action still pins. It was added by commit-check#527 and shipped in 2.13.2, so the whitelist on the runner is three releases behind the project's own. The pin bump in #258 fixes that; this only sidesteps it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
1 parent a4bcac9 commit 0816365

2 files changed

Lines changed: 121 additions & 8 deletions

File tree

‎main.py‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,32 @@ def failures(self) -> list[dict[str, str]]:
123123
return [c for c in self.checks if c["status"] == "fail"]
124124

125125

126+
def overall_status(results: list[ScopeResult]) -> str:
127+
"""Reduce scope statuses to one of ``pass``/``fail``/``skip``.
128+
129+
One function, used by every completion path, because the alternative
130+
is what this replaced: four separate ``all(... == "pass")`` tests, each
131+
correct only while exactly two statuses existed. The moment ``skip``
132+
appeared they all silently reclassified a skipped run as a failure.
133+
134+
``skip`` requires at least one scope and all of them skipped.
135+
"""
136+
if any(scope.status == "fail" for scope in results):
137+
return "fail"
138+
if results and all(scope.status == "skip" for scope in results):
139+
return "skip"
140+
return "pass"
141+
142+
143+
def exit_code_for(results: list[ScopeResult]) -> int:
144+
"""Only a failure is an error.
145+
146+
A skipped run validated nothing, but it violated no policy either, so
147+
it must not fail the workflow.
148+
"""
149+
return 1 if overall_status(results) == "fail" else 0
150+
151+
126152
def log_env_vars():
127153
"""Logs the environment variables for debugging purposes.
128154
@@ -337,7 +363,7 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]:
337363
args = [a for a in args if a != "--message"]
338364
results.extend(run_other_checks(args))
339365

340-
exit_code = 1 if any(scope.status == "fail" for scope in results) else 0
366+
exit_code = exit_code_for(results)
341367
return exit_code, results
342368

343369

@@ -487,7 +513,10 @@ def render_step_log(results: list[ScopeResult]) -> None:
487513
if total and skipped == total:
488514
print("\u2298 commit-check: all checks skipped, nothing was validated")
489515
elif skipped:
490-
print(f"\u2714 commit-check: all checks passed ({skipped} skipped)")
516+
print(
517+
f"\u2714 commit-check: {total - skipped} of {total} checks passed, "
518+
f"{skipped} skipped"
519+
)
491520
else:
492521
print("\u2714 commit-check: all checks passed")
493522

@@ -538,7 +567,10 @@ def _markdown_table(results: list[ScopeResult]) -> str:
538567
"|---|---|---|",
539568
]
540569
for scope in results:
541-
if scope.status == "pass":
570+
# Only failures belong in this table. A skipped scope has no failed
571+
# checks and no checked value, so it contributed an entirely blank
572+
# row \u2014 an empty accusation in a table headed "Failed checks".
573+
if scope.status != "fail":
542574
continue
543575
value = _scope_value(scope)
544576
value_display = f"`{value}`" if value else "\u2014"
@@ -723,7 +755,7 @@ def render_report(results: list[ScopeResult]) -> str:
723755
if failed:
724756
lines.append(f"❌ **{failed} of {total} {unit} failed**")
725757
lines.extend(["", _markdown_table(results), ""])
726-
elif skipped == total:
758+
elif total and skipped == total:
727759
# Nothing ran, so there is no success to announce. Saying "all
728760
# checks passed" here is the defect this branch exists to prevent.
729761
lines.append(f"⊘ **All {total} {unit} skipped** — nothing was validated")
@@ -763,7 +795,7 @@ def add_job_summary(results: list[ScopeResult]) -> int:
763795
with open(GITHUB_STEP_SUMMARY, "a", encoding="utf-8") as summary_file:
764796
summary_file.write(render_job_summary(results))
765797

766-
return 0 if all(scope.status == "pass" for scope in results) else 1
798+
return exit_code_for(results)
767799

768800

769801
def set_result_output(results: list[ScopeResult]) -> None:
@@ -775,7 +807,7 @@ def set_result_output(results: list[ScopeResult]) -> None:
775807
if not output_path:
776808
return
777809
payload = {
778-
"status": "pass" if all(s.status == "pass" for s in results) else "fail",
810+
"status": overall_status(results),
779811
"scopes": [
780812
{"label": scope.label, "status": scope.status, "checks": scope.checks}
781813
for scope in results
@@ -933,7 +965,7 @@ def add_pr_comments(results: list[ScopeResult]) -> int:
933965
if target is not None:
934966
if target.body == pr_comment_body:
935967
print(f"PR comment already up-to-date for PR #{pr_number}.")
936-
return 0 if all(scope.status == "pass" for scope in results) else 1
968+
return exit_code_for(results)
937969
print(f"Updating the last comment on PR #{pr_number}.")
938970
target.edit(pr_comment_body)
939971
for comment in stale:
@@ -943,7 +975,7 @@ def add_pr_comments(results: list[ScopeResult]) -> int:
943975
print(f"Creating a new comment on PR #{pr_number}.")
944976
pull_request.create_comment(body=pr_comment_body)
945977

946-
return 0 if all(scope.status == "pass" for scope in results) else 1
978+
return exit_code_for(results)
947979
except GithubException as e:
948980
if e.status == 403:
949981
# GithubException.data is whatever the response decoded to, which

‎main_test.py‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,3 +1433,84 @@ def test_older_engine_without_skip_is_unaffected(self):
14331433
"""Back-compat: engines that only emit pass/fail render as before."""
14341434
results = [pass_scope("Branch", value="main")]
14351435
self.assertIn("✅ **All 1 check passed**", main.render_report(results))
1436+
1437+
1438+
class TestSkipCompletionSemantics(unittest.TestCase):
1439+
"""A skipped run must not be treated as a failing one.
1440+
1441+
run_commit_check has always failed only on "fail", but the completion
1442+
paths each asked `all(status == "pass")` instead. That was equivalent
1443+
only while pass and fail were the only statuses; with skip added, a
1444+
skipped-only run exited 1 and reported "fail" while rendering ⊘.
1445+
"""
1446+
1447+
def test_skipped_only_run_is_not_a_failure(self):
1448+
self.assertEqual(main.exit_code_for([skip_scope(), skip_scope("Branch")]), 0)
1449+
1450+
def test_partial_skip_is_not_a_failure(self):
1451+
self.assertEqual(main.exit_code_for([pass_scope(), skip_scope()]), 0)
1452+
1453+
def test_a_real_failure_still_fails(self):
1454+
self.assertEqual(main.exit_code_for([fail_scope(), skip_scope()]), 1)
1455+
1456+
def test_result_output_status_reports_skip(self):
1457+
self.assertEqual(
1458+
main.overall_status([skip_scope(), skip_scope("Branch")]), "skip"
1459+
)
1460+
1461+
def test_result_output_status_of_partial_skip_is_pass(self):
1462+
self.assertEqual(main.overall_status([pass_scope(), skip_scope()]), "pass")
1463+
1464+
def test_set_result_output_emits_skip(self):
1465+
"""The `result` action output must not call a skipped run failed."""
1466+
with tempfile.NamedTemporaryFile("w+", delete=False) as f:
1467+
out_path = f.name
1468+
try:
1469+
with patch.dict(os.environ, {"GITHUB_OUTPUT": out_path}):
1470+
main.set_result_output([skip_scope(), skip_scope("Branch")])
1471+
written = open(out_path, encoding="utf-8").read()
1472+
finally:
1473+
os.unlink(out_path)
1474+
payload = json.loads(written.split("result<<EOF\n", 1)[1].rsplit("\nEOF", 1)[0])
1475+
self.assertEqual(payload["status"], "skip")
1476+
1477+
def test_add_job_summary_returns_success_for_a_skipped_run(self):
1478+
with tempfile.NamedTemporaryFile("w+", delete=False) as f:
1479+
summary_path = f.name
1480+
try:
1481+
with (
1482+
patch.object(main, "JOB_SUMMARY_ENABLED", True),
1483+
patch.object(main, "GITHUB_STEP_SUMMARY", summary_path),
1484+
):
1485+
rc = main.add_job_summary([skip_scope(), skip_scope("Branch")])
1486+
finally:
1487+
os.unlink(summary_path)
1488+
self.assertEqual(rc, 0)
1489+
1490+
1491+
class TestSkipRenderingEdgeCases(unittest.TestCase):
1492+
def test_failure_table_omits_skipped_scopes(self):
1493+
"""A skipped scope has no failed checks, so it must not get a row.
1494+
1495+
It previously contributed a row with an empty value and an empty
1496+
rule list — a blank accusation under "Failed checks".
1497+
"""
1498+
table = main._markdown_table([fail_scope("Commit 1/1"), skip_scope("PR title")])
1499+
self.assertIn("| Commit 1/1 |", table)
1500+
self.assertNotIn("PR title", table)
1501+
# header + separator + exactly one data row
1502+
self.assertEqual(len(table.splitlines()), 3)
1503+
1504+
def test_empty_result_set_is_not_reported_as_all_skipped(self):
1505+
"""`skipped == total` is trivially true for no scopes at all."""
1506+
body = main.render_report([])
1507+
self.assertNotIn("skipped", body)
1508+
self.assertIn("✅ **All 0 checks passed**", body)
1509+
1510+
def test_step_log_partial_skip_does_not_claim_all_passed(self):
1511+
buf = io.StringIO()
1512+
with patch("sys.stdout", buf):
1513+
main.render_step_log([pass_scope(), skip_scope()])
1514+
out = buf.getvalue()
1515+
self.assertIn("1 of 2 checks passed, 1 skipped", out)
1516+
self.assertNotIn("all checks passed", out)

0 commit comments

Comments
 (0)