From cd7c22325b73a21cb51dc9ccef79691b73d5265a Mon Sep 17 00:00:00 2001 From: Ayobami Adegoke Date: Fri, 10 Jul 2026 07:06:00 +0100 Subject: [PATCH] =?UTF-8?q?ci:=20run=20the=20Python=20test=20suite=20?= =?UTF-8?q?=E2=80=94=20CONTRIBUTING.md=20asks=20for=20tests=20CI=20never?= =?UTF-8?q?=20executes=20(#100)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: run the Python test suite - CONTRIBUTING.md asks for tests CI never executes CONTRIBUTING.md tells contributors to put Python tool tests in tests/ and run the relevant suites, and tests/ now holds real ones (test_salary_lookup.py, test_convert_salary_excel.py from #75) - but no CI job executes them. A suite that never runs in CI can't gate a PR and silently rots. New python-tests job: unittest discover over tests/, stdlib only, no new dependencies. Future test files run without any workflow change. Also lands tests/test_security_guards.py, which missed #84's merge window (pushed to the branch as #84 was being merged; the merge took 2a6cb8c, the tests were 260c37a). 13 unittest cases in the existing tests/ style: each copies the guard script into a synthetic repo tree and runs it as a subprocess - the same way CI invokes it - asserting real exit codes and messages. Every forbidden state fails (Bash(*) and Bash(curl:*) additions, each personal-data gitignore rule removed one at a time, each forbidden lifecycle script, trustedDependencies, invalid settings JSON, zero manifests); every non-event passes (dropped shipped permission, extra ignore rules, benign scripts, hostile manifest inside node_modules); and the real repo passes its own guards. 22 tests total, all passing locally via the exact command the job runs. * test: use benign lifecycle-script values in fixtures - AV heuristics flag attack-shaped strings Review found the curl-pipe-to-sh fixture value matches a real Defender signature (Trojan:Script/Stealer.HAX!MTB): Windows quarantines the temp package.json mid-test, making the suite flaky for any Windows contributor who runs it - while proving nothing extra, since the guard flags the script KEY and never inspects the value. Fixture values are now 'echo test' (also in the node_modules-ignored test, same class of string), with a comment on the key-only test explaining why the value must stay benign so a future 'make the fixture realistic' cleanup doesn't reintroduce the quarantine flake. Coverage is unchanged: same keys, same assertions, 13 tests passing. --- .github/workflows/ci.yml | 10 ++ tests/test_security_guards.py | 169 ++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+) create mode 100644 tests/test_security_guards.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0279bed..43fc0b0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,6 +54,16 @@ jobs: python-version: "3.12" - run: python tools/security_guards.py + python-tests: + name: Python tool tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 + with: + python-version: "3.12" + - run: python -m unittest discover -s tests -t . -v + dependency-review: name: Dependency review (upstream PRs only) # Requires the repo's Dependency graph, which forks never inherit and diff --git a/tests/test_security_guards.py b/tests/test_security_guards.py new file mode 100644 index 0000000..9f9297e --- /dev/null +++ b/tests/test_security_guards.py @@ -0,0 +1,169 @@ +import json +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +GUARD_SCRIPT = REPO_ROOT / "tools" / "security_guards.py" + +sys.path.insert(0, str(REPO_ROOT / "tools")) +import security_guards # noqa: E402 (imported for its allowlist constants) + + +def run_guards(root: Path) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(root / "tools" / "security_guards.py")], + capture_output=True, + text=True, + ) + + +class GuardRepoFixture(unittest.TestCase): + """Builds a minimal repo tree the guards pass on, then breaks one thing per test. + + The guard script resolves the repo root from its own location, so each test + copies it into a temp tree and runs it as a subprocess - the same way CI + invokes it - asserting on real exit codes and messages. + """ + + def setUp(self): + self.root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.root, ignore_errors=True) + + (self.root / "tools").mkdir() + shutil.copy(GUARD_SCRIPT, self.root / "tools" / "security_guards.py") + + self.settings = self.root / ".claude" / "settings.json" + self.settings.parent.mkdir() + self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS)) + + self.gitignore = self.root / ".gitignore" + self.write_gitignore(security_guards.REQUIRED_IGNORE_RULES) + + self.manifest = self.root / ".agents" / "skills" / "example-search" / "cli" / "package.json" + self.manifest.parent.mkdir(parents=True) + self.write_manifest({"name": "example-cli", "scripts": {"start": "bun run src/cli.ts"}}) + + def write_settings(self, allow): + self.settings.write_text(json.dumps({"permissions": {"allow": list(allow)}})) + + def write_gitignore(self, rules): + self.gitignore.write_text("\n".join(rules) + "\n") + + def write_manifest(self, data, path=None): + (path or self.manifest).write_text(json.dumps(data)) + + +class CleanTreeTests(GuardRepoFixture): + def test_clean_tree_passes(self): + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("security_guards: OK", result.stdout) + + +class PermissionGuardTests(GuardRepoFixture): + def test_wildcard_bash_permission_fails(self): + self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS) + ["Bash(*)"]) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("not in the reviewed allowlist", result.stdout) + self.assertIn("Bash(*)", result.stdout) + + def test_network_fetch_permission_fails(self): + self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS) + ["Bash(curl:*)"]) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("not in the reviewed allowlist", result.stdout) + + def test_dropped_allowlisted_permission_still_passes(self): + # Removing a shipped permission narrows exposure; the guard only + # rejects additions, it must not force entries to exist. + allow = sorted(security_guards.ALLOWED_PERMISSIONS)[:-1] + self.write_settings(allow) + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_invalid_settings_json_fails(self): + self.settings.write_text("{not json") + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("invalid JSON", result.stdout) + + +class GitignoreGuardTests(GuardRepoFixture): + def test_each_missing_personal_data_rule_fails(self): + for rule in security_guards.REQUIRED_IGNORE_RULES: + with self.subTest(rule=rule): + remaining = [r for r in security_guards.REQUIRED_IGNORE_RULES if r != rule] + self.write_gitignore(remaining) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("required personal-data rule missing", result.stdout) + self.assertIn(rule, result.stdout) + self.write_gitignore(security_guards.REQUIRED_IGNORE_RULES) + + def test_extra_rules_are_allowed(self): + self.write_gitignore(list(security_guards.REQUIRED_IGNORE_RULES) + ["*.bak", "scratch/"]) + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + +class ManifestGuardTests(GuardRepoFixture): + def test_each_lifecycle_script_fails(self): + for script in sorted(security_guards.FORBIDDEN_SCRIPTS): + with self.subTest(script=script): + # The guard flags the script KEY; the value is never inspected, + # so it must stay benign: attack-shaped values (curl-pipe-to-sh + # etc.) written to disk trip AV heuristics - Windows Defender + # quarantines the fixture mid-test and the suite goes flaky. + self.write_manifest( + {"name": "example-cli", "scripts": {script: "echo test"}} + ) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("lifecycle script", result.stdout) + self.assertIn(script, result.stdout) + self.write_manifest({"name": "example-cli", "scripts": {}}) + + def test_trusted_dependencies_fails(self): + self.write_manifest({"name": "example-cli", "trustedDependencies": ["left-pad"]}) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("trustedDependencies", result.stdout) + + def test_benign_scripts_pass(self): + self.write_manifest( + {"name": "example-cli", "scripts": {"start": "bun run src/cli.ts", "test": "bun test", "typecheck": "tsc --noEmit"}} + ) + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_node_modules_manifests_are_ignored(self): + # Installed dependencies are not repo-tracked code; a hostile manifest + # inside node_modules must not fail the guard (and bun blocks its + # lifecycle scripts anyway). + nm = self.manifest.parent / "node_modules" / "some-dep" / "package.json" + nm.parent.mkdir(parents=True) + self.write_manifest({"name": "some-dep", "scripts": {"postinstall": "echo test"}}, path=nm) + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_no_manifests_at_all_fails(self): + self.manifest.unlink() + result = run_guards(self.root) + self.assertEqual(result.returncode, 1) + self.assertIn("no package.json files found", result.stdout) + + +class RealRepoTests(unittest.TestCase): + def test_guards_pass_on_this_repo(self): + # The live check CI runs: the actual repo tree must satisfy its own guards. + result = run_guards(REPO_ROOT) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + +if __name__ == "__main__": + unittest.main()