mirror of
https://github.com/newliver666/apk-reverse.git
synced 2026-09-28 21:56:59 +02:00
Make the command gate machine-independent, and calibrate the budget fallback
CI went red on `1dc1d9b` in both jobs while the identical command passed locally, and the reason is
worth keeping: the checks depended on state that only exists on a developer's machine.
check_commands.py -- the gate's verdict depended on a gitignored directory
The previous commit's fix recognised a workbench script by matching its name against a walk of
`tools/`. That tree is gitignored, so a clean checkout has none of it: `dcc.py` matched locally and
skipped, and on CI nothing matched, it fell through to `missing-script`, and the gate reported
drift for three quoted lines. A check whose verdict changes with a directory CI does not have is
not a check. Two changes remove the dependency:
* `qualified_basenames()` collects every `.py` the *document itself* writes with a path, so a
bare name the document has already located elsewhere is a skip, computed from the text.
* `./dcc.py` is a declared upstream entry point, checked before the workbench-name test.
* a bare name after a `cd` that resolves nowhere is `after-cd-unresolved` -- unverifiable, which
is not the same claim as broken, and is now reported as such instead of as drift.
Verified by running the whole CI sequence against a tree with `tools/` hidden: five gates exit 0,
the suite passes. That experiment is the one that should have been run before the first push.
check_budget.py -- the measurement was installation-dependent
The gate measures the always-loaded body in tokens, preferring `tiktoken` and falling back to
`characters / N`. `N` had been guessed at 3.6 rather than calibrated, so a machine with tiktoken
saw ~11.1k tokens and passed while a machine without it saw ~14.2k and warned -- and CI's 3.11 job
is the one without it. `N` is now the measured 4.285 characters per token (47,629 chars -> 11,114
tokens on SKILL.md), both paths agree within one token, and `tests/unit/test_budget_measurement.py`
forces both paths and asserts they do not straddle the budget.
CI now installs `tiktoken` so the comparison exercises the real tokenizer instead of skipping.
Also: two `.py` files are formatted for consistency, and the reconstruction record documents the
CI-only failure, since "it passed on my machine" is the lesson rather than an aside.
Gates: check_repo 0 problem, check_refs 0 dangling / 0 warning, check_routing coherent,
check_budget 0 failure / 0 warning (both measurement paths), check_commands 154 checked / 0 drift,
pytest 617 passed on 3.14 and 609 on 3.11.
This commit is contained in:
@@ -68,29 +68,84 @@ def test_a_path_qualified_workbench_reference_is_a_skip_not_drift(gate):
|
||||
"""`tools/_work/...` is deliberate in an evidence record: a skip, reported with its reason."""
|
||||
name = _workbench_script_name(gate)
|
||||
doc = str(REPO_ROOT / "docs" / "tool-verification" / "EXTENSION-extraction-shell-bench.md")
|
||||
_path, why = gate.resolve_script("tools/_work/somewhere/%s" % name, doc, include_workbench=False)
|
||||
_path, why = gate.resolve_script("tools/_work/bench/unpack/b3/%s" % name, doc,
|
||||
include_workbench=False)
|
||||
assert why == "workbench"
|
||||
|
||||
|
||||
def test_a_bare_name_after_a_cd_in_the_same_block_is_not_drift(gate):
|
||||
"""A block that says `cd tools/_work/bench/repos/dcc` tells the reader where to stand."""
|
||||
name = _workbench_script_name(gate)
|
||||
doc = str(REPO_ROOT / "docs" / "tool-verification" / "EXTENSION-java2c.md")
|
||||
text = "```\n$ cd tools/_work/bench/repos/dcc\n$ python %s --no-build\n```\n" % name
|
||||
@pytest.fixture
|
||||
def fake_repo(gate, tmp_path, monkeypatch):
|
||||
"""A repository-shaped tree with its own workbench, so resolution can be tested anywhere.
|
||||
|
||||
This exists because the workbench-dependent tests above cannot run on a clean checkout: the
|
||||
`tools/` tree is gitignored, so on CI they skip and the gate's most important property goes
|
||||
unverified exactly where it matters. The tree built here has the layout the resolver assumes
|
||||
(a root, a skill dir, a gitignored `tools/`) and nothing else.
|
||||
"""
|
||||
root = tmp_path / "repo"
|
||||
(root / "skills" / "apk-reverse" / "scripts").mkdir(parents=True)
|
||||
(root / "docs" / "tool-verification").mkdir(parents=True)
|
||||
(root / "tools" / "_work" / "bench").mkdir(parents=True)
|
||||
(root / "README.md").write_text("# root\n", encoding="utf-8")
|
||||
(root / "skills" / "apk-reverse" / "scripts" / "shipped.py").write_text(
|
||||
'"""capability: x\n"""\nimport argparse\np = argparse.ArgumentParser()\n'
|
||||
'p.add_argument("--flag")\n', encoding="utf-8")
|
||||
(root / "tools" / "_work" / "bench" / "solo_in_workbench.py").write_text(
|
||||
"print('x')\n", encoding="utf-8")
|
||||
(root / "docs" / "tool-verification" / "record.md").write_text("# record\n", encoding="utf-8")
|
||||
|
||||
monkeypatch.setattr(gate, "ROOT", str(root))
|
||||
monkeypatch.setattr(gate, "SKILL_DIR", str(root / "skills" / "apk-reverse"))
|
||||
monkeypatch.setattr(gate, "SCRIPT_DIR", str(root / "skills" / "apk-reverse" / "scripts"))
|
||||
gate._WORKBENCH_INDEX.clear()
|
||||
yield root
|
||||
gate._WORKBENCH_INDEX.clear()
|
||||
|
||||
|
||||
def test_resolution_order_is_root_then_skill_then_document(gate, fake_repo):
|
||||
"""The corrected order, asserted against a tree that exists on any machine."""
|
||||
doc = str(fake_repo / "docs" / "tool-verification" / "record.md")
|
||||
path, why = gate.resolve_script("README.md", doc, include_workbench=False)
|
||||
assert why == "ok" and path.endswith("README.md")
|
||||
path, why = gate.resolve_script("shipped.py", doc, include_workbench=False)
|
||||
assert why == "ok" and path.endswith("shipped.py")
|
||||
|
||||
|
||||
def test_a_workbench_only_name_is_drift_and_a_qualified_one_is_not(gate, fake_repo):
|
||||
doc = str(fake_repo / "docs" / "tool-verification" / "record.md")
|
||||
path, why = gate.resolve_script("solo_in_workbench.py", doc, include_workbench=False)
|
||||
assert (path, why) == (None, "unqualified-workbench")
|
||||
path, why = gate.resolve_script("tools/_work/bench/solo_in_workbench.py", doc,
|
||||
include_workbench=False)
|
||||
assert (path, why) == (None, "workbench")
|
||||
|
||||
|
||||
def test_a_bare_name_the_document_paths_elsewhere_is_a_skip(gate, fake_repo):
|
||||
"""The fix that made the gate behave the same on CI as on a populated machine.
|
||||
|
||||
`solo_in_workbench.py` appears once bare and once with a path in the same document. The
|
||||
document has already told the reader where it lives, so the bare line is a shorthand -- and
|
||||
that verdict must not depend on the gitignored tree being visible, which is how the first
|
||||
version of this fix passed locally and failed on CI.
|
||||
"""
|
||||
doc = str(fake_repo / "docs" / "tool-verification" / "record.md")
|
||||
text = ("Path: `tools/_work/bench/solo_in_workbench.py` explains it.\n\n"
|
||||
"```\n$ python solo_in_workbench.py\n```\n")
|
||||
findings = gate.command_findings(doc, text, {}, False)
|
||||
kinds = [f["kind"] for f in findings]
|
||||
assert "unqualified-workbench" not in kinds, findings
|
||||
assert "workbench-after-cd" in kinds, findings
|
||||
assert "missing-script" not in kinds, findings
|
||||
assert "skipped" in kinds, findings
|
||||
|
||||
|
||||
def test_a_shipped_script_resolves_from_the_repository_root(gate):
|
||||
"""The order that matters: root, then the skill's scripts, then the document's own directory."""
|
||||
doc = str(REPO_ROOT / "docs" / "tool-verification" / "README.md")
|
||||
path, why = gate.resolve_script("check_repo.py", doc, include_workbench=False)
|
||||
assert why == "ok" and path and path.endswith("check_repo.py")
|
||||
path, why = gate.resolve_script("skills/apk-reverse/scripts/scan_leaks.py", doc,
|
||||
include_workbench=False)
|
||||
assert why == "ok" and path and path.endswith("scan_leaks.py")
|
||||
def test_a_bare_name_after_cd_with_no_resolution_is_skipped_not_drift(gate, fake_repo):
|
||||
"""`cd <somewhere cloned>` then a bare name: unverifiable, and reported as such."""
|
||||
doc = str(fake_repo / "docs" / "tool-verification" / "record.md")
|
||||
text = "```\n$ cd tools/_work/bench/repos/elsewhere\n$ python upstream_tool.py --x\n```\n"
|
||||
findings = gate.command_findings(doc, text, {}, False)
|
||||
kinds = [f["kind"] for f in findings]
|
||||
assert "missing-script" not in kinds, findings
|
||||
assert "after-cd-unresolved" in kinds, findings
|
||||
|
||||
|
||||
def test_the_committed_tree_reports_no_drift(gate, tmp_path):
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
"""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. `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 this
|
||||
repository's gate runner (CI's 3.11 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.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from conftest import REPO_ROOT
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
@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
|
||||
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
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 = (REPO_ROOT / "skills" / "apk-reverse" / "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 = (REPO_ROOT / "skills" / "apk-reverse" / "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))
|
||||
Reference in New Issue
Block a user