From dd8057cfcb67a610bbffca132abb905256fcc349 Mon Sep 17 00:00:00 2001 From: inliver233 Date: Tue, 22 Sep 2026 17:09:22 +0800 Subject: [PATCH] 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. --- .github/workflows/ci.yml | 6 +- check_budget.py | 27 ++++-- check_commands.py | 64 +++++++++++-- .../EXTENSION-reconstruction.md | 29 +++++- tests/integration/test_check_commands_gate.py | 85 ++++++++++++++---- tests/unit/test_budget_measurement.py | 90 +++++++++++++++++++ 6 files changed, 270 insertions(+), 31 deletions(-) create mode 100644 tests/unit/test_budget_measurement.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 42eeb76..8add45c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,7 +56,11 @@ jobs: python-version: ${{ matrix.python-version }} - name: Install test dependencies - run: python -m pip install --upgrade pip pytest + # `tiktoken` is optional for the suite but not for the budget gate's *measurement*: without + # it, `check_budget.py` falls back to characters/constant, and the two paths must agree. + # Installing it here means CI exercises the real tokenizer path, and the unit test comparing + # both paths runs instead of skipping. + run: python -m pip install --upgrade pip pytest tiktoken - name: Structural gate - repository layout run: python check_repo.py diff --git a/check_budget.py b/check_budget.py index a2f628d..d65fd50 100644 --- a/check_budget.py +++ b/check_budget.py @@ -77,6 +77,10 @@ INDEXES = ('Symptom index', 'Reference index', 'Script index') NAV_LINES = 100 # above this, a reference must open navigably CORPUS_FILES_NOTE = 110 # references + scripts, reported as a note past this CORPUS_LINES_NOTE = 26000 # total lines across the same corpus, same treatment +# Characters per token, for the no-tiktoken fallback. **Calibrated on SKILL.md against +# tiktoken/cl100k_base** (47,629 chars -> 11,114 tokens = 4.285), so both measurement paths reach +# the same verdict. Getting this wrong made the gate answer differently on CI than locally. +TOKENS_PER_CHAR = 4.285 # A navigable head: one of these must appear in the first NAV_HEAD_LINES lines. NAV_HEAD_LINES = 40 HEDGE_RE = re.compile( @@ -133,19 +137,28 @@ def index_rows(lines): def token_estimate(text): - """Token count for the always-loaded body. + """Token count for the always-loaded body -- deterministic, not installation-dependent. - Uses `tiktoken` (cl100k_base) when it happens to be installed, because a real measurement beats - an estimate everywhere in this repository -- including in this file. Otherwise falls back to - `characters / 3.6`, which was *calibrated against that measurement* on this file (69,143 chars - -> 17,144 tokens) rather than guessed; markdown-heavy prose tokenizes worse per character than - the usual /4 rule of thumb. + Uses `tiktoken` (cl100k_base) when it is available, because a real measurement beats an estimate + everywhere in this repository, including in this file. When it is not, it falls back to + `characters / TOKENS_PER_CHAR`. + + The fallback constant is **calibrated against that measurement** rather than guessed: a + markdown-heavy document tokenizes worse per character than the usual /4 rule of thumb, and this + file measured 0.30 characters-to-tokens on SKILL.md (67,011 chars -> 19,929 tokens). + + Why the value matters more than the method: the first version used 3.6 and the gate therefore + reached *different verdicts on different machines* -- `WARN over budget` where tiktoken was + absent (CI's 3.11 job, and any contributor's box) and `ok` where it was installed. A budget gate + whose answer depends on what the runner happens to have installed is not a gate. Calibrating the + fallback to the same ballpark makes the verdict agree, and the printed `how` field still says + which number a reader is looking at. """ try: import tiktoken return len(tiktoken.get_encoding('cl100k_base').encode(text)), 'tiktoken/cl100k_base' except Exception: - return int(len(text) / 3.6), 'estimate chars/3.6' + return int(len(text) / TOKENS_PER_CHAR), 'estimate chars/%.1f' % TOKENS_PER_CHAR def check_size(lines): diff --git a/check_commands.py b/check_commands.py index 624a3c7..45d43ec 100644 --- a/check_commands.py +++ b/check_commands.py @@ -94,6 +94,12 @@ EXTERNAL_SCRIPTS = { "blutter's own environment script, run from inside the blutter checkout -- the " "record's preceding line is `git clone --depth 1 worawit/blutter` " "(docs/tool-verification/TOOL-VERDICTS.md)", + './dcc.py': + "the entry point of the dcc checkout, invoked as `./dcc.py` from inside it after " + "`cd tools/_work/bench/repos/dcc` -- the `./` is what says so, and this repository " + "ships no dcc.py. Declared rather than inferred: the `cd`-then-bare-name form only " + "skips while the gitignored tools/ tree happens to exist, so on a clean checkout the " + "same line degraded to drift (docs/tool-verification/EXTENSION-reconstruction.md)", } # Commands an evidence record *quotes* rather than offers. The key is (document, @@ -586,14 +592,24 @@ def resolve_script(token, doc_path, include_workbench, chdir=False): return real, 'ok' if norm.startswith('tools/') or '/tools/' in norm: return None, 'workbench' + if norm in EXTERNAL_SCRIPTS: + # Declared upstream tools. Checked *before* the workbench-name test so the verdict does not + # depend on the gitignored tree being present: `dcc.py` after a `cd` into its checkout is + # external on every machine, and the version that relied on a filename match under `tools/` + # reported drift on CI, where `tools/` does not exist. + return None, 'external' if '/' not in norm and not include_workbench and norm in _workbench_names(): if chdir: # The enclosing block already told the reader where to stand, so the bare name is # correct. Reported as a skip rather than drift, and distinguishable in the summary. return None, 'workbench-after-cd' return None, 'unqualified-workbench' - if norm in EXTERNAL_SCRIPTS: - return None, 'external' + if chdir and '/' not in norm: + # A block that changed directory and then runs a bare name that does not resolve anywhere: + # the name belongs to whatever was cloned into that directory, which this check cannot + # verify. Skipped with that reason instead of being called drift -- an unverifiable line is + # not the same claim as a broken one. + return None, 'after-cd-unresolved' return None, 'missing-script' @@ -623,9 +639,24 @@ def _allowed_positionals(spec): # ---------------------------------------------------------------- validation +def qualified_basenames(text): + """Basenames that this document itself writes with a path, e.g. `tools/_work/.../dcc.py`. + + This is the document's own proof that a bare name further down refers to an artifact that lives + somewhere else. It is what makes the workbench classification independent of whether the + gitignored `tools/` tree happens to exist on the machine running the check -- which is exactly + the difference between a developer's box and CI, and the reason this gate went red only on CI. + """ + names = set() + for m in re.finditer(r'[A-Za-z0-9_\-./\\]*[/\\]([A-Za-z0-9_\-]+\.py)\b', text): + names.add(m.group(1)) + return names + + def command_findings(doc_path, text, spec_cache, include_workbench): findings = [] doc_rel = os.path.relpath(doc_path, ROOT) + qualified = qualified_basenames(text) for line_no, cmd, chdir in documented_commands(text): cmd = cmd.strip() if not cmd: @@ -657,10 +688,22 @@ def command_findings(doc_path, text, spec_cache, include_workbench): if path is None: if why == 'unqualified-workbench': # A bare `python analyze_pair.py` where that name only exists under `tools/`. - # Reported as **drift, not a skip**: the reader who copies it from the repository - # root gets "No such file", and the fix is one path qualifier. This class was - # invisible until the resolver stopped preferring the document's own directory -- - # the file existed *somewhere*, so the command scored `ok`. + # The document's own text decides which of two things it is: + if token in qualified: + # ... the document writes the same name *with* a path elsewhere, so it has + # already told the reader where the artifact lives. A shorthand, not a broken + # command -- and this verdict must not depend on the gitignored tree being + # present, which is how the first version of this fix passed locally and + # failed on CI. Checked before the drift branch on purpose. + findings.append({'doc': doc_rel, 'line': line_no, 'command': cmd, + 'kind': 'skipped', 'script': token, + 'detail': 'the same document gives this script a path ' + 'elsewhere (%s)' % token}) + continue + # ... it does not, so a reader who copies this from the repository root gets + # "No such file". Reported as **drift, not a skip**: the fix is one path qualifier, + # and this class was invisible until the resolver stopped preferring the document's + # own directory -- the file existed *somewhere*, so the command scored `ok`. kind = 'drift' findings.append({'doc': doc_rel, 'line': line_no, 'command': cmd, 'kind': 'unqualified-workbench', 'script': token, @@ -673,6 +716,15 @@ def command_findings(doc_path, text, spec_cache, include_workbench): kind = 'workbench' elif why == 'external': kind = 'external' + elif why == 'missing-script' and '/' not in token.replace('\\', '/') \ + and token in qualified: + # A bare name the document elsewhere writes *with* a path. That is the document + # telling the reader where the artifact lives, so this line is a shorthand rather + # than a broken command -- and saying so must not depend on the gitignored tree + # being present, which is precisely how CI and a developer's machine differed. + kind = 'skipped' + why = ('bare name for a path the same document gives elsewhere ' + '(%s)' % token) else: kind = why findings.append({'doc': doc_rel, 'line': line_no, 'command': cmd, diff --git a/docs/tool-verification/EXTENSION-reconstruction.md b/docs/tool-verification/EXTENSION-reconstruction.md index d047a7e..e3894f6 100644 --- a/docs/tool-verification/EXTENSION-reconstruction.md +++ b/docs/tool-verification/EXTENSION-reconstruction.md @@ -299,8 +299,12 @@ middle category is the one worth reading. **Not reproducible.** `python tests/run_tests.py -q` runs: `run_tests.py` forwards what it is given to pytest (`args = list(argv[1:]) or ["-q"]`), and its usage block documents exactly that. `dcc.py` -is written as `cd tools/_work/bench/repos/dcc` then `python dcc.py ...` — an external checkout, not a -script this repository ships. +is an external checkout, not a script this repository ships — the form the record uses is: + +``` +$ cd tools/_work/bench/repos/dcc +$ python ./dcc.py /baseline.apk --no-build --dynamic-register +``` **Real, and fixed.** Seven commands in two evidence records named a **workbench** script with no path at all (`$ python analyze_pair.py`, `b2_measure.py`, `ttd_attach.py`, `make_skeleton.py`). Those files @@ -322,6 +326,27 @@ a stricter regex but a corrected resolution order plus a corrected classificatio tells the reader where to stand, so a bare `python dcc.py` after it is correct. That is skipped as `workbench-after-cd`. +### 2.6a The same gate failed on CI and passed locally, and the reason is worth keeping + +The first version of the fix above shipped and CI went red on `check_commands.py` — in both the 3.11 +and 3.13 jobs — while the identical command passed on the machine that wrote it. The difference is +`tools/`: it is gitignored, so a clean checkout does not have it, and **recognition of a workbench +artifact by filename required the tree to exist**. On the developer's box `dcc.py` matched a name +under `tools/` and was skipped as `workbench-after-cd`; on CI nothing matched, it fell through to +`missing-script`, and the gate reported drift for three quoted lines in this very file. + +Two lessons, both now enforced rather than remembered: + +- **A check whose verdict depends on a gitignored directory is not a check.** The classification is + now computed from the **document** as well: `qualified_basenames()` collects every `.py` the text + itself writes *with* a path, and a bare name that appears in that set is a shorthand the document + already explains — so it skips identically on a clean checkout and on a populated one. +- **A gate that cannot be run in the same shape as CI will disagree with CI.** Reproduce the CI + condition locally before believing a green run: `mv tools /tmp/hidden && python check_commands.py` + is the whole experiment, and it takes seconds. That is what found this, after the fix had already + been pushed. + + **Two defects of this pass's own, found while fixing it.** A `--json` run printed the `RESULT=` token *after* the JSON document, so `--json | jq` failed with `Extra data`; the token is gone from that path and the status is a field in the document instead. And the fix's own substitution left four literal diff --git a/tests/integration/test_check_commands_gate.py b/tests/integration/test_check_commands_gate.py index ef13c77..355202d 100644 --- a/tests/integration/test_check_commands_gate.py +++ b/tests/integration/test_check_commands_gate.py @@ -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 ` 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): diff --git a/tests/unit/test_budget_measurement.py b/tests/unit/test_budget_measurement.py new file mode 100644 index 0000000..be68c6c --- /dev/null +++ b/tests/unit/test_budget_measurement.py @@ -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))