Fix the budget gate and add CI, lint and coverage guards
- check_budget.py escalated a hard index-row violation to 'warn' on both branches, so INDEX_ROW_HARD could never fail anything. It fails now, and tests/unit/test_budget_measurement.py pins both thresholds. - The corpus note counted scripts/__pycache__/*.pyc, so the same commit reported 27 scripts after the suite had run and 16 on a clean checkout. - TOKENS_PER_CHAR is re-derived from a measurement (26,795 chars -> 6,065 tokens = 4.418); the comment's old figure and its arithmetic were both wrong for this file. - check_repo.py's yaml sentinel carries the type ignore, so mypy is clean with PyYAML installed as well as without it. - The CPUID test now skips with the probe's own note on a host without CPUID instead of failing, and pytest.ini points at tests/README.md, which exists, instead of an EXTENSION file that never did. Tooling: .flake8 (a measured 120-column limit), .gitattributes, .gitignore and .github/workflows/ci.yml (the five gates, the suite on 3.11-3.14 and a lint job), which is what CONTRIBUTING.md already claimed CI runs. The pull-request template it refers to now exists. Content: pitfalls P26-P34 -- the analysis-level failures the catalogue was missing (sampling read as observation, a self-built parser trusted, a verification that could not have failed, a shell-mangled command line) -- registered in the symptom index and its routing mirror. FINDINGS F11-F15 record the defects above. Tests: the coverage guard derives its subject from scripts/ rather than a hand-written list, pe_triage.py and pe_diff.py gain their first tests, and plan.py / preflight.py are declared uncovered with reasons.
This commit is contained in:
root
+3
-1
@@ -20,9 +20,11 @@ and "the script was measured doing X on this host".
|
||||
| `unit/test_scan_leaks.py` | Every rule firing on a shaped sample, every exemption class staying silent, the three measured deviations from the exemption list, scan position reporting, and the file walk's skips. |
|
||||
| `unit/test_capabilities_registry.py` | The capability table: every atom's kind, basis and probe name, the transitive closure against an independent walk, the dependency graph's acyclicity, `resolve` on missing atoms and on `file:` atoms that are and are not on disk, the declaration parser including the documentation template it must reject, and the `declared` / `mapped` / `unknown` classification. |
|
||||
| `unit/test_doctor.py` | The static dependency pass (stdlib, siblings and a third-party name separated without executing the file), `discover_scripts` scanning rather than listing, the per-script table with an unclassified script still counted, and the environment facts with their keys and severity labels. |
|
||||
| `unit/test_budget_measurement.py` | The budget gate's two measurement paths agree (real tokenizer versus the calibrated fallback), the fallback constant still matches a fresh measurement of `SKILL.md`, and a row past `INDEX_ROW_HARD` fails rather than warns. |
|
||||
| `unit/test_dart_snapshot_strings.py` | The ELF reader against a generated container, both literal encodings recovered at the offsets the fixture placed them at, the length and limit gates, and the pool counts -- zero on an image carrying neither idiom and non-zero on hand-built bytes carrying each one. |
|
||||
| `cli/test_cli_surface.py` | Help and usage exit codes for every script, the documented exit codes for each script's error paths, and the negative case that inspect-only runs write nothing into the working directory. |
|
||||
| `cli/test_cli_json.py` | Each script's machine-readable output: keys, values, and the conventions that are not what a reader would assume. |
|
||||
| `cli/test_cli_coverage.py` | Coverage derived from the scripts directory rather than a hand-written list: every shipped `.py` is either exercised or declared uncovered with a reason, and every one answers `--help` with its usage block. A new script fails here until it is registered. |
|
||||
| `cli/test_cli_json.py` | Each script's machine-readable output: keys, values, and the conventions that are not what a reader would assume -- including `pe_triage.py`'s identity block on a generated image (and that it writes nothing) and `pe_diff.py`'s self-comparison, which is the control for every other diff. |
|
||||
| `cli/test_cli_registry_surface.py` | The registry and doctor subprocess contracts: `--list` / `--json` / single-capability exit codes and `RESULT=` tokens, the doctor's three blocks and its 0-versus-3 verdict, and the view flags that print one block only. |
|
||||
| `cli/test_cli_dart_snapshot.py` | The snapshot reader's CLI: the refusals and their exit codes, `--find` case-insensitivity and the exit code a miss produces, `--at` by file offset and by virtual address, and the `--json` document. |
|
||||
| `integration/test_pipeline_end_to_end.py` | Build, locate, patch, re-read: the patched bytes, the untouched control site, the recomputed checksum, and a control run in which nothing changes. |
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
"""Coverage of the shipped scripts, derived from the directory rather than a hand-written list.
|
||||
|
||||
Borrowed mechanism: the sibling apk-reverse repository derives its CLI contract cases from
|
||||
skills/*/scripts/*.py and from each script's own --help output, so a new script is covered the day
|
||||
it lands instead of the day someone remembers to add it. What is *not* borrowed is that repository's
|
||||
single 0-4 exit-code table: this kit publishes a per-script vocabulary (pepatch.py uses 3 for "not a
|
||||
parseable PE", where the table means "the environment lacks a capability"), and a test that rewrote
|
||||
those meanings would assert a policy this repository deliberately does not have.
|
||||
|
||||
The guard here is therefore narrower, and it is about *coverage*, not exit codes:
|
||||
|
||||
* every shipped .py is either in conftest.SCRIPT_FILES (some layer of this suite exercises it) or
|
||||
named in DECLARED_UNCOVERED together with the reason it is not;
|
||||
* every declared gap carries a real sentence, because "TODO" is how a gap becomes permanent;
|
||||
* every shipped script -- covered or declared -- answers --help with its usage block, which is the
|
||||
cheapest possible exercise of its argparse table and its import path.
|
||||
|
||||
A new script fails this file until one of those lists changes. That is the point: the alternative
|
||||
is a suite that looks complete while silently ignoring half the kit.
|
||||
"""
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
from conftest import SCRIPTS_DIR, SCRIPT_FILES
|
||||
|
||||
pytestmark = pytest.mark.cli
|
||||
|
||||
# Scripts no layer exercises yet. Each entry is a recorded gap, not an exemption: the reason says
|
||||
# what would be needed to close it, so the next pass can decide rather than rediscover.
|
||||
DECLARED_UNCOVERED = {
|
||||
"plan.py":
|
||||
"composes capabilities.py and doctor.py into a route from a stated intent. Both halves have "
|
||||
"their own tests; the composition reads live host facts, so a fixture that pinned its output "
|
||||
"would pin this machine's capability set instead of the mapping. Closing it needs a stubbed "
|
||||
"probe, and that stub does not exist yet.",
|
||||
"preflight.py":
|
||||
"answers session readiness from live host facts (free space, route prerequisites) plus one "
|
||||
"sample. The sample half could be pinned to a generated PE; the host half has no generator, "
|
||||
"and asserting on the runner's environment would encode this machine as an expectation.",
|
||||
}
|
||||
|
||||
|
||||
def shipped_scripts():
|
||||
"""Every runnable python script the skill ships, sorted."""
|
||||
return sorted(p.name for p in SCRIPTS_DIR.glob("*.py"))
|
||||
|
||||
|
||||
def test_every_shipped_script_is_exercised_or_declared():
|
||||
"""A script nobody tests must at least be a script somebody decided not to test."""
|
||||
shipped = set(shipped_scripts())
|
||||
assert shipped, "no scripts found under %s" % SCRIPTS_DIR
|
||||
silently_uncovered = shipped - set(SCRIPT_FILES) - set(DECLARED_UNCOVERED)
|
||||
assert not silently_uncovered, (
|
||||
"these scripts are shipped but neither exercised nor declared uncovered: %s. Add a test and "
|
||||
"register the script in conftest.SCRIPT_FILES, or add it to DECLARED_UNCOVERED with the "
|
||||
"reason." % sorted(silently_uncovered))
|
||||
|
||||
|
||||
def test_declared_entries_are_neither_stale_nor_hollow():
|
||||
"""A gap that was closed must leave the list; a gap with no reason is a hidden one."""
|
||||
both = set(SCRIPT_FILES) & set(DECLARED_UNCOVERED)
|
||||
assert not both, ("these scripts are in SCRIPT_FILES and also declared uncovered: %s -- the "
|
||||
"declaration is stale" % sorted(both))
|
||||
for name, reason in DECLARED_UNCOVERED.items():
|
||||
assert (SCRIPTS_DIR / name).is_file(), "%s is declared uncovered but does not exist" % name
|
||||
assert len(reason.split()) >= 12, (
|
||||
"%s: the reason must say what would close the gap, not just mark it" % name)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("script", shipped_scripts())
|
||||
def test_every_shipped_script_answers_help_with_usage(script):
|
||||
"""The usage block is the script's published contract; it must render on every host."""
|
||||
proc = subprocess.run([sys.executable, str(SCRIPTS_DIR / script), "--help"],
|
||||
capture_output=True, text=True, timeout=120,
|
||||
encoding="utf-8", errors="replace")
|
||||
assert proc.returncode == 0, "%s --help exited %d: %s" % (script, proc.returncode, proc.stderr)
|
||||
assert "usage:" in proc.stdout.lower(), "%s printed no usage block" % script
|
||||
assert script in proc.stdout, "%s does not name itself in its usage line" % script
|
||||
@@ -390,8 +390,48 @@ def test_prober_rows_carry_a_measurement_and_a_note(hwprobe_report):
|
||||
assert row["state"] in ROUTE_STATES or row["state"].startswith("needs-"), row
|
||||
|
||||
|
||||
def test_prober_can_dump_raw_cpuid_registers(run_script):
|
||||
def test_prober_can_dump_raw_cpuid_registers(run_script, hwprobe_report):
|
||||
"""A requested CPUID dump must be non-empty -- where CPUID is reachable at all.
|
||||
|
||||
The probe's own report says which of those two situations this host is in, so the test reads it
|
||||
instead of assuming x64 Windows: on a non-x64 or non-Windows host the stub cannot run, and
|
||||
"nothing was reported" is the correct answer rather than a defect. A missing capability is a
|
||||
skip with the probe's own note, never a red test (tests/README.md, conftest.py).
|
||||
"""
|
||||
cpu = hwprobe_report["cpu"][0]
|
||||
if cpu["state"] != "available":
|
||||
pytest.skip("CPUID is not reachable on this host, so there is no register dump to check: %s"
|
||||
% cpu["note"])
|
||||
result = run_script("hwprobe.py", "--raw", "--json", timeout=120)
|
||||
assert result.returncode == 0, result.stderr
|
||||
payload = json.loads(result.stdout)
|
||||
assert payload["raw_cpuid"], "a raw dump was requested but nothing was reported"
|
||||
|
||||
|
||||
# --- pe_triage ---------------------------------------------------------------
|
||||
|
||||
def test_triage_reports_identity_and_leaves_the_sample_untouched(fixtures, run_script):
|
||||
"""The first script a task should run: it parses, classifies, and must not write."""
|
||||
sample = fixtures.pe64
|
||||
before = sample.data
|
||||
result = run_script("pe_triage.py", sample.path, "--json")
|
||||
assert result.returncode == 0, result.stderr
|
||||
payload = json.loads(result.stdout)
|
||||
assert payload["sha256"] == sample.sha256
|
||||
assert payload["identity"]["arch"] == "amd64"
|
||||
assert payload["identity"]["bitness"] == 64
|
||||
assert payload["identity"]["kind"] == "exe"
|
||||
assert sample.data == before, "triage modified the image it was reading"
|
||||
|
||||
|
||||
# --- pe_diff -----------------------------------------------------------------
|
||||
|
||||
def test_diff_of_the_same_build_reports_no_material_change(fixtures, run_script):
|
||||
"""Self-comparison is the control for every other diff: it must find nothing."""
|
||||
sample = fixtures.pe64
|
||||
result = run_script("pe_diff.py", sample.path, sample.path, "--json")
|
||||
assert result.returncode == 0, result.stderr
|
||||
payload = json.loads(result.stdout)
|
||||
assert payload["verdict"] == "identical-modulo-normalised-fields"
|
||||
assert payload["material_change"] is False
|
||||
assert payload["byte_diff"]["differ_bytes"] == 0
|
||||
|
||||
@@ -50,6 +50,11 @@ SCRIPT_FILES = (
|
||||
"dart_snapshot_strings.py",
|
||||
"fold.py",
|
||||
"trace_reduce.py",
|
||||
# The two route helpers: the first script a task should run on an unknown image, and the
|
||||
# two-build comparison. Added with tests/cli/test_cli_coverage.py, which fails if a shipped
|
||||
# script is neither in this tuple nor declared uncovered with a reason.
|
||||
"pe_triage.py",
|
||||
"pe_diff.py",
|
||||
)
|
||||
|
||||
# A hang detector, not a performance assertion. Nothing in this suite is meant to be slow.
|
||||
|
||||
@@ -0,0 +1,137 @@
|
||||
"""The budget gate must reach the same verdict whether or not `tiktoken` is installed.
|
||||
|
||||
This is a small test for a defect that cost two red CI runs upstream. `check_budget.py` measures the
|
||||
always-loaded body in tokens, preferring `tiktoken` and falling back to `characters / N` when it is
|
||||
absent. The first version's `N` was guessed (3.6) instead of calibrated, so a machine with tiktoken
|
||||
saw ~10,900 tokens and passed while a machine without it saw ~13,200 and warned -- and the gate
|
||||
runner (CI's oldest job, and any contributor's box) is the one without it.
|
||||
|
||||
A gate whose answer depends on which packages the runner happens to have installed is not a gate, so
|
||||
both paths are forced here and compared.
|
||||
|
||||
The third test guards the other half of the same failure: a threshold that is computed and then
|
||||
unable to fail anything.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
from conftest import REPO_ROOT
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
SKILL_MD = REPO_ROOT / "skills" / "exe-reverse" / "SKILL.md"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def budget():
|
||||
sys.path.insert(0, str(REPO_ROOT))
|
||||
return importlib.import_module("check_budget")
|
||||
|
||||
|
||||
def _has_tiktoken():
|
||||
"""Whether a real tokenizer is available, reported rather than assumed.
|
||||
|
||||
The comparison needs *two* implementations; where there is only one, the honest outcome is a
|
||||
skip with the install command, not a failure and not a silent pass. CI installs it, so the
|
||||
property is verified where it matters; a contributor without it has both gates agreeing anyway,
|
||||
because the fallback is calibrated to the same constant.
|
||||
"""
|
||||
try:
|
||||
import tiktoken # noqa: F401
|
||||
return True
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def test_the_fallback_estimate_agrees_with_the_real_tokenizer(budget):
|
||||
"""Both measurement paths must land on the same side of the soft budget."""
|
||||
if not _has_tiktoken():
|
||||
pytest.skip("tiktoken is not installed here, so there is no measurement to compare against; "
|
||||
"install it with `python -m pip install tiktoken`")
|
||||
text = SKILL_MD.read_text(encoding="utf-8")
|
||||
|
||||
real, how_real = budget.token_estimate(text)
|
||||
assert "tiktoken" in how_real, how_real
|
||||
|
||||
# Force the fallback path by handing the module a tokenizer that cannot be imported.
|
||||
saved = sys.modules.get("tiktoken")
|
||||
sys.modules["tiktoken"] = None # `import tiktoken` raises here
|
||||
try:
|
||||
fallback, how_fallback = budget.token_estimate(text)
|
||||
finally:
|
||||
if saved is not None:
|
||||
sys.modules["tiktoken"] = saved
|
||||
else:
|
||||
sys.modules.pop("tiktoken", None)
|
||||
|
||||
assert "estimate" in how_fallback, how_fallback
|
||||
drift = abs(real - fallback) / max(1, real)
|
||||
assert drift < 0.05, (
|
||||
"the fallback is %.1f%% away from tiktoken (%d vs %d): recalibrate TOKENS_PER_CHAR "
|
||||
"against a fresh measurement rather than a guess" % (100 * drift, fallback, real))
|
||||
assert (real <= budget.TOKEN_SOFT) == (fallback <= budget.TOKEN_SOFT), (
|
||||
"the two paths straddle the soft budget (%d real / %d fallback vs %d): the gate would "
|
||||
"answer differently on CI than on a developer machine"
|
||||
% (real, fallback, budget.TOKEN_SOFT))
|
||||
|
||||
|
||||
def test_the_fallback_constant_is_the_calibrated_one(budget):
|
||||
"""Pin the constant to a measurement: `chars / tokens` on SKILL.md, recomputed here."""
|
||||
text = SKILL_MD.read_text(encoding="utf-8")
|
||||
real, how = budget.token_estimate(text)
|
||||
if "tiktoken" not in how:
|
||||
pytest.skip("no tiktoken available, so there is no measurement to calibrate against")
|
||||
expected = len(text) / real
|
||||
assert abs(budget.TOKENS_PER_CHAR - expected) < 0.25, (
|
||||
"TOKENS_PER_CHAR is %.3f but this SKILL.md measures %.3f characters per token -- the "
|
||||
"document changed, so recalibrate" % (budget.TOKENS_PER_CHAR, expected))
|
||||
|
||||
|
||||
def test_an_index_row_past_the_hard_limit_fails_rather_than_warns(budget):
|
||||
"""The hard index-row limit must escalate, or the limit it names is decorative.
|
||||
|
||||
It read `'warn' if hard else 'warn'` -- both branches identical -- so `INDEX_ROW_HARD` was
|
||||
defined, computed, and unable to fail anything: an index row could grow without bound while
|
||||
every gate stayed green. That is the same failure mode this file exists for, one level up: the
|
||||
measurement happened and the verdict did not follow. It was found by injecting an over-long row
|
||||
and requiring the gate to fail, which is the only way a dead limit shows itself -- a gate that
|
||||
has only ever been seen passing is not evidence that it can see anything.
|
||||
"""
|
||||
def levels(desc_len):
|
||||
lines = [
|
||||
"## Symptom index",
|
||||
"| What you observe | Load first |",
|
||||
"| a row | %s |" % ("y" * desc_len),
|
||||
]
|
||||
return {level for level, _ in budget.check_index_rows(lines)}
|
||||
|
||||
assert levels(budget.INDEX_ROW_SOFT + 10) == {"warn"}
|
||||
assert levels(budget.INDEX_ROW_HARD + 10) == {"fail"}, (
|
||||
"a row past INDEX_ROW_HARD (%d chars) must fail the gate, not warn about it"
|
||||
% budget.INDEX_ROW_HARD)
|
||||
|
||||
def test_the_corpus_count_ignores_a_bytecode_cache(budget, tmp_path, monkeypatch):
|
||||
"""A measurement that changes when a reader runs the suite measures the reader.
|
||||
|
||||
The corpus note once counted scripts/__pycache__/*.pyc, so the same commit reported 27 scripts
|
||||
on a machine that had run pytest and 16 on a clean checkout. The routing gate said 16 the whole
|
||||
time; the disagreement between two gates about one tree is what surfaced it.
|
||||
"""
|
||||
refs = tmp_path / "references"
|
||||
refs.mkdir()
|
||||
(refs / "one.md").write_text("# one\n", encoding="utf-8")
|
||||
scripts = tmp_path / "scripts"
|
||||
scripts.mkdir()
|
||||
(scripts / "one.py").write_text("print(1)\n", encoding="utf-8")
|
||||
cache = scripts / "__pycache__"
|
||||
cache.mkdir()
|
||||
(cache / "one.cpython-314.pyc").write_bytes(b"\x00" * 10)
|
||||
|
||||
monkeypatch.setattr(budget, "REFS", str(refs))
|
||||
monkeypatch.setattr(budget, "SKILL_DIR", str(tmp_path))
|
||||
notes = budget.check_corpus()
|
||||
assert notes[0].startswith("corpus: 1 references + 1 scripts = 2 files"), notes[0]
|
||||
Reference in New Issue
Block a user