From 7faec5121d63ded647477ecd61ae15e9cc72e581 Mon Sep 17 00:00:00 2001 From: Moshe Abramovitch Date: Wed, 12 Aug 2026 15:22:36 -0500 Subject: [PATCH] test(metadata): stop conflating drift with the warning gate The --no-ai case asserted on main()'s bare exit code. --check returns 1 for output drift as well as for skill warnings, so any PR that edits skill content failed this test for reasons unrelated to enrichment. PR #445 is the first to hit it: 70 changed files including SKILL.md at 62+/147-, which legitimately drifts the checked-in metadata. Neutralise diff_text for the duration of the run so the exit code is attributable to the warning gate alone. Drift is already covered by case 4. A first attempt simply excused drift with 'rc == 0 or drifted'. That was worse than the original bug: removing the --no-ai gate entirely still passed, because drift was present. Mutation-checked properly this time. Verified against PR #445's content: reproduces the failure without this change, passes with it, and still fails when the --no-ai gate is removed -- on both #445's tree and a clean one. Signed-off-by: Moshe Abramovitch --- .../test_generate_skill_metadata.py | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/scripts/marketplace/test_generate_skill_metadata.py b/.github/scripts/marketplace/test_generate_skill_metadata.py index 95b79238..c7e64003 100644 --- a/.github/scripts/marketplace/test_generate_skill_metadata.py +++ b/.github/scripts/marketplace/test_generate_skill_metadata.py @@ -212,6 +212,11 @@ def capture(obj): # with AI available must fail. A synthetic skill is injected into discovery # so the outcome does not depend on whether the working tree happens to # contain an unenriched skill today. + # + # --check returns 1 for drift as well as for warnings, and a PR that edits + # skill content legitimately drifts the checked-in metadata. Asserting on + # the bare exit code therefore failed on unrelated PRs. Capture the output + # and judge the warning path on its own. original_discover = gen.discover_skills ghost = gen.Skill( path="skills/zzz-unenriched-fixture", @@ -224,19 +229,32 @@ def discover_with_ghost(exclusions): found, excluded = original_discover(exclusions) return list(found) + [ghost], excluded - def exit_code_for(argv, client): + original_diff = gen.diff_text + + def run_for(argv, client): gen.discover_skills = discover_with_ghost gen.build_ai_client = lambda allow_ai=True: client + # Suppress drift reporting for the duration of the run. --check returns + # 1 for drift as well as for warnings, and any PR that edits skill + # content drifts the checked-in metadata legitimately. Neutralising the + # comparison makes the exit code attributable to the warning gate alone, + # which is what this case is about. Drift itself is covered by case 4. + gen.diff_text = lambda *a, **k: "" + out, err = io.StringIO(), io.StringIO() try: - with contextlib.redirect_stderr(io.StringIO()), \ - contextlib.redirect_stdout(io.StringIO()): - return gen.main(argv) + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(out): + rc = gen.main(argv) finally: gen.discover_skills = original_discover gen.build_ai_client = original_client + gen.diff_text = original_diff + return rc, out.getvalue() + err.getvalue() + + rc_no_ai, log_no_ai = run_for(["--check", "--no-ai"], None) + rc_with_ai, _ = run_for(["--check"], failing_api) - rc_no_ai = exit_code_for(["--check", "--no-ai"], None) - rc_with_ai = exit_code_for(["--check"], failing_api) + r.check("unenriched new skill under --no-ai -> reported as a warning", + "PARTIAL SUCCESS" in log_no_ai) r.check("unenriched new skill under --no-ai -> does not fail PR CI", rc_no_ai == 0, f"rc={rc_no_ai}") r.check("unenrichable new skill with AI available -> fails the run", rc_with_ai == 1,