From 1dc1d9b707d33320d2bb1de6172689caedb8e5ca Mon Sep 17 00:00:00 2001 From: inliver233 Date: Tue, 22 Sep 2026 02:18:40 +0800 Subject: [PATCH] Fix the command gate's blind spot, and make --json actually be JSON A reviewer reported seven drifting commands in the evidence record. Reproduced, the report split three ways, and the middle category was the useful one: the gate had been scoring them clean. Not reproducible, with the measurement: tests/run_tests.py -q runs. The wrapper forwards its arguments to pytest (`args = list(argv[1:]) or ["-q"]`), and documents that in its own usage. dcc.py already written as `cd tools/_work/bench/repos/dcc` followed by `python dcc.py ...` -- an external checkout, not a shipped script. Real, and fixed here: seven commands in two evidence records named a **workbench** script with no path at all (`analyze_pair.py`, `make_skeleton.py`, `ttd_attach.py`, `b2_measure.py`). The files exist under `tools/`, which .gitignore excludes, so a reader copying one from the repository root gets "No such file" -- and two of those documents wrote the correct path elsewhere, so they disagreed with themselves. **The actual defect was in `check_commands.py`, not in the seven lines.** `resolve_script()` tried the document's own directory first, so a bare `analyze_pair.py` inside `docs/tool-verification/` resolved to `tools/_work/bench/unpack/b3/analyze_pair.py` -- a real file -- and passed. Resolution now follows the order a reader would try (repository root, then the skill's scripts, then the document's own directory, which is what `../x.py` in a nested README legitimately means); a bare name that only resolves under `tools/` is `unqualified-workbench`; and that class counts as **drift, not as a skip**, because it is a one-token fix that a reader cannot run. The first attempt classified it as a low-confidence warning, which would have left all seven in place. One exception is honoured and pinned: a code block opening with `cd tools/_work/bench/repos/dcc` is telling the reader where to stand, so a bare `python dcc.py` after it is correct. `documented_commands()` now reports whether a `cd` preceded the command in the same block, and that case is skipped as `workbench-after-cd`. Two defects of this pass's own, found while fixing it: --json emitted the RESULT= token after the JSON document, so `--json | jq` failed with "Extra data". The token is gone from that path; the status is a field in the document, and the exit code carries the same meaning. The fix's own substitution left four literal backslashes before `$` in the two files it edited -- a character a reader would copy by mistake. Removed, and verified absent. `tests/integration/test_check_commands_gate.py` pins the resolution order, the drift classification, the `cd` exception, and a clean pass over the committed tree, so this cannot come back silently. Gates: check_commands 153 checked / 0 drift, check_repo 0 problem, check_refs 0 dangling / 0 warning, check_routing coherent, check_budget 0 failure / 0 warning, pytest 613 passed / 158 skipped / 25 xfailed. --- check_commands.py | 69 ++++++++--- .../EXTENSION-benchmark-l1-l3.md | 6 +- .../EXTENSION-extraction-shell-bench.md | 4 +- .../EXTENSION-reconstruction.md | 41 ++++++- tests/integration/test_check_commands_gate.py | 111 ++++++++++++++++++ 5 files changed, 210 insertions(+), 21 deletions(-) create mode 100644 tests/integration/test_check_commands_gate.py diff --git a/check_commands.py b/check_commands.py index 68b68ef..624a3c7 100644 --- a/check_commands.py +++ b/check_commands.py @@ -485,9 +485,13 @@ def documented_commands(text): for idx, raw in enumerate(lines, 1): if FENCE_RE.match(raw): if in_fence: + # Whether an earlier line in the same block changed directory. A block that opens + # with `cd tools/_work/bench/repos/dcc` tells the reader where to stand, so a bare + # `python dcc.py` after it is correct -- and must not be reported as unqualified. + chdir = any(re.match(r'\s*(\$\s*)?cd\s+\S', ln) for ln in block) for off, cmd in enumerate(join_continuations(block)): for sub in split_shell(cmd): - yield (block_start + off, sub) + yield (block_start + off, sub, chdir) block, in_fence = [], False else: in_fence, block, block_start = True, [], idx + 1 @@ -499,7 +503,7 @@ def documented_commands(text): body = m.group(1).strip() if body.startswith(('python ', 'python3 ', 'py ')): for sub in split_shell(body): - yield (idx, sub) + yield (idx, sub, True) # an inline mention has no block context to honour def find_script_token(tokens): @@ -541,8 +545,20 @@ def _workbench_names(): return _WORKBENCH_INDEX['names'] -def resolve_script(token, doc_path, include_workbench): - """Where a documented script path points, or why it cannot be followed.""" +def resolve_script(token, doc_path, include_workbench, chdir=False): + """Where a documented script path points, or why it cannot be followed. + + The order matters, and getting it wrong made this gate blind to a real class of drift: an + earlier revision resolved a relative path against the **document's own directory first**. A + bare `python analyze_pair.py` written inside `docs/tool-verification/` therefore resolved to + `tools/_work/bench/.../analyze_pair.py` -- a real file, hidden behind a gitignored directory -- + and was scored `ok`. A reader who copies that command from the repository root gets + "No such file", which is the failure this gate exists to prevent. + + So the roots are tried in the order a reader would: repository root, then the skill's script + directory, and only then the document's own directory (which is what `../x.py` in a nested + README legitimately means). A hit under `tools/` is a workbench artifact either way. + """ norm = token.replace('\\', '/') if '...' in norm: # `python .../vmp_diff_harness.py compare ...`: the record elides the path on @@ -552,27 +568,30 @@ def resolve_script(token, doc_path, include_workbench): if os.path.isabs(token): cands.append(token) else: - # relative to the document that wrote it, which is how `../x.py` in a nested - # README reads; the repository-rooted and skill-rooted forms follow - cands.append(os.path.join(os.path.dirname(doc_path), norm)) cands.append(os.path.join(ROOT, norm)) if not norm.startswith('skills/'): cands.append(os.path.join(SKILL_DIR, norm)) if not norm.startswith(('skills/', 'scripts/')): cands.append(os.path.join(SCRIPT_DIR, norm)) + cands.append(os.path.join(os.path.dirname(doc_path), norm)) for c in cands: if os.path.isfile(c): real = os.path.realpath(c) tools_root = os.path.realpath(os.path.join(ROOT, 'tools')) + os.sep if real.startswith(tools_root) and not include_workbench: - # reached by `../../tools/_work/...`: still a workbench artifact, and - # `.gitignore` excludes tools/, so a reader cannot open it either + # reached by `../../tools/_work/...`, or by a bare name that only exists there: + # still a workbench artifact, and `.gitignore` excludes tools/, so a reader who + # copies the command cannot open it return None, 'workbench' return real, 'ok' if norm.startswith('tools/') or '/tools/' in norm: return None, 'workbench' if '/' not in norm and not include_workbench and norm in _workbench_names(): - return None, 'workbench' + 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' return None, 'missing-script' @@ -607,7 +626,7 @@ def _allowed_positionals(spec): def command_findings(doc_path, text, spec_cache, include_workbench): findings = [] doc_rel = os.path.relpath(doc_path, ROOT) - for line_no, cmd in documented_commands(text): + for line_no, cmd, chdir in documented_commands(text): cmd = cmd.strip() if not cmd: continue @@ -634,9 +653,23 @@ def command_findings(doc_path, text, spec_cache, include_workbench): continue token = tokens[idx] - path, why = resolve_script(token, doc_path, include_workbench) + path, why = resolve_script(token, doc_path, include_workbench, chdir) if path is None: - if why.startswith('workbench'): + 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`. + kind = 'drift' + findings.append({'doc': doc_rel, 'line': line_no, 'command': cmd, + 'kind': 'unqualified-workbench', 'script': token, + 'suggest': ['a path that resolves from the repository root, e.g. ' + '`tools/.../%s`' % token], + 'detail': 'bare script name that only exists under tools/ ' + '(gitignored): a reader cannot run it'}) + continue + if why == 'workbench': kind = 'workbench' elif why == 'external': kind = 'external' @@ -867,7 +900,11 @@ def main(argv=None): print('RESULT=%s' % TOKENS['internal']) return EXIT_INTERNAL - drift = [f for f in all_findings if f['kind'] in ('unknown-flag', 'missing-script')] + # `unqualified-workbench` counts as drift: the command names a script that only exists under a + # gitignored directory and gives no path, so a reader who copies it fails. It is a one-token fix, + # which is exactly why it should be reported rather than skipped. + drift = [f for f in all_findings + if f['kind'] in ('unknown-flag', 'missing-script', 'unqualified-workbench')] warnings = [f for f in all_findings if f['kind'] == 'warning'] unverifiable = [f for f in all_findings if f['kind'] == 'unverifiable'] workbench = [f for f in all_findings if f['kind'] == 'workbench'] @@ -914,7 +951,9 @@ def main(argv=None): 'findings': all_findings, } print(json.dumps(payload, indent=2, ensure_ascii=False)) - print('RESULT=%s' % token) + # No `RESULT=` line here on purpose: appending one would make the stream invalid JSON for a + # caller doing `| jq .` or `json.loads(stdout)`, which is the entire point of `--json`. The + # status is a field in the document, and the exit code carries the same meaning. return code if args.fix_report: diff --git a/docs/tool-verification/EXTENSION-benchmark-l1-l3.md b/docs/tool-verification/EXTENSION-benchmark-l1-l3.md index 2e52587..1ade2e0 100644 --- a/docs/tool-verification/EXTENSION-benchmark-l1-l3.md +++ b/docs/tool-verification/EXTENSION-benchmark-l1-l3.md @@ -427,7 +427,7 @@ for the OK button. needs: ``` -$ python ttd_attach.py --package --activity sg.vantagepoint.uncrackable3.MainActivity \ +$ python tools/_work/bench/l1l3/ttd_attach.py --package --activity sg.vantagepoint.uncrackable3.MainActivity \ --js l3_read_crc.js --launch-delay 1.5 --seconds 20 [i] pre-attach main pid=7020 at 1.88s [i] attached at +0.00s (probe loaded) @@ -523,7 +523,7 @@ the expected original bytes before each write. Patched condition: ``` -$ python b2_measure.py --js l3_hook_patch_v2.js --tag PATCHED --probe-seconds 25 +$ python tools/_work/bench/l1l3/b2_measure.py --js l3_hook_patch_v2.js --tag PATCHED --probe-seconds 25 [i] spawned pid=10000 (paused) [i] resumed pid=10000 [0.04s][i] libfoo.so base=0x764b89d000 size=90112 @@ -545,7 +545,7 @@ I/Zygote( 1007): Process 10000 exited due to signal 6 (Aborted) Control condition (identical reads and writes, original bytes written back): ``` -$ python b2_measure.py --js l3_hook_control_v2.js --tag control_v2 --probe-seconds 22 +$ python tools/_work/bench/l1l3/b2_measure.py --js l3_hook_control_v2.js --tag control_v2 --probe-seconds 22 [0.04s][CONTROL] 0x2ff0 unchanged (wrote back 40 1a 80 52 c0 03 5f d6) [0.04s][CONTROL] 0x2ff8 unchanged (wrote back c0 0c 80 52 c0 03 5f d6) [0.04s][CONTROL] 0x3080 unchanged (wrote back fd 7b bf a9 fd 03 00 91) diff --git a/docs/tool-verification/EXTENSION-extraction-shell-bench.md b/docs/tool-verification/EXTENSION-extraction-shell-bench.md index 570fabc..27d9aad 100644 --- a/docs/tool-verification/EXTENSION-extraction-shell-bench.md +++ b/docs/tool-verification/EXTENSION-extraction-shell-bench.md @@ -44,7 +44,7 @@ A byte diff of the pair (`tools/_work/bench/unpack/b3/analyze_pair.py`) locates difference: ``` -$ python analyze_pair.py +$ python tools/_work/bench/unpack/b3/analyze_pair.py len A=12536 len B=12536 first diff offset = 8 (0x8) diff byte count = 6 @@ -83,7 +83,7 @@ the tail is nop-filled, so offsets, class table and string table are untouched a header is re-fixed with `dexutil.fix_dex_header` (signature first, checksum last). ``` -$ python make_skeleton.py src-r2pay.dex skel +$ python tools/_work/bench/unpack/b3/make_skeleton.py src-r2pay.dex skel source: src-r2pay.dex ctrl.dex fraction=0.00 mode=return emptied=0/5061 skel-25.dex fraction=0.25 mode=return emptied=1265/5061 diff --git a/docs/tool-verification/EXTENSION-reconstruction.md b/docs/tool-verification/EXTENSION-reconstruction.md index 8183246..d047a7e 100644 --- a/docs/tool-verification/EXTENSION-reconstruction.md +++ b/docs/tool-verification/EXTENSION-reconstruction.md @@ -292,7 +292,46 @@ Defect 1 is the instructive one: a checker whose parser silently shortens a comm for exactly the inputs it was written to reject, and nothing in its own output looks wrong. The defence is not more careful reading, it is a fixture that is known to be bad. -### 2.6 What this check does not cover +### 2.6 A second blind spot, found by a reviewer reading this gate's output + +A reviewer reported seven "drifting" commands. Reproducing them split the report three ways, and the +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. + +**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 +exist under `tools/`, which `.gitignore` excludes, so a reader copying the command from the +repository root gets `No such file`. Two of those documents wrote the correct path elsewhere, so they +disagreed with themselves. + +**The part that matters: this gate had been scoring those `ok`.** `resolve_script()` tried the +*document's own directory* first, so a bare `analyze_pair.py` inside `docs/tool-verification/` +resolved to `tools/_work/bench/unpack/b3/analyze_pair.py` — a real file — and passed. The fix is not +a stricter regex but a corrected resolution order plus a corrected classification: + +1. Resolution now follows the order a **reader** would try: repository root, then the skill's script + directory, then the document's own directory (which is what `../x.py` in a nested README means). +2. A bare name that only resolves under `tools/` is `unqualified-workbench`, and it counts as + **drift, not a skip** — a one-token fix that a reader cannot run. A first attempt classified it as + a low-confidence warning, which would have left all seven in place. +3. The one honest exception is pinned too: a block that opens with `cd tools/_work/bench/repos/dcc` + tells the reader where to stand, so a bare `python dcc.py` after it is correct. That is skipped as + `workbench-after-cd`. + +**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 +backslashes before `$` in the two files — a reader would have copied them. + +`tests/integration/test_check_commands_gate.py` pins the resolution order, the drift classification, +the `cd` exception, and a clean pass over the committed tree, so this blind spot cannot return +silently. + +### 2.7 What this check does not cover - It validates **flags and (weakly) positional counts**, not semantics. A command with correct flags and a wrong path still passes; so does a documented `--duration 14` where the useful value is diff --git a/tests/integration/test_check_commands_gate.py b/tests/integration/test_check_commands_gate.py new file mode 100644 index 0000000..ef13c77 --- /dev/null +++ b/tests/integration/test_check_commands_gate.py @@ -0,0 +1,111 @@ +"""Regression tests for the documented-command gate itself. + +`check_commands.py` is the gate that proves other documentation is runnable, so a blind spot in it +is worth more than a blind spot elsewhere: it turns "146 commands checked, 0 drift" into a claim the +repository cannot back. One such blind spot is pinned here, exactly as it was found. + +**What was wrong.** `resolve_script()` tried the *document's own directory* first when resolving a +relative path. A bare `python analyze_pair.py` written inside `docs/tool-verification/` therefore +resolved to `tools/_work/bench/unpack/b3/analyze_pair.py` -- a real file, inside the gitignored +workbench -- and scored `ok`. Seven commands in two evidence files used that form while the same +files wrote the correct path elsewhere, and a reader copying one from the repository root gets +"No such file". The gate said the documentation was clean. + +The tests below drive the resolver and the finding classifier directly rather than through the CLI, +so they stay fast and do not depend on the current state of the documentation. +""" +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +from conftest import REPO_ROOT + +pytestmark = pytest.mark.integration + +CHECK_COMMANDS = REPO_ROOT / "check_commands.py" + + +@pytest.fixture(scope="module") +def gate(): + """The gate as a module, so its internals can be driven without a subprocess.""" + sys.path.insert(0, str(REPO_ROOT)) + import importlib + + return importlib.import_module("check_commands") + + +def _workbench_script_name(gate): + """A `.py` basename that exists only under tools/, or skip if the workbench is absent.""" + names = sorted(gate._workbench_names()) + if not names: + pytest.skip("no workbench present (tools/ is gitignored and may not exist in a checkout)") + shipped = {p.name for p in (REPO_ROOT / "skills" / "apk-reverse" / "scripts").glob("*.py")} + root = {p.name for p in REPO_ROOT.glob("*.py")} + for name in names: + if name not in shipped and name not in root: + return name + pytest.skip("every workbench script name is also shipped; nothing to test with") + + +def test_a_bare_name_that_only_exists_in_the_workbench_is_drift(gate): + """The exact defect: the name exists *somewhere*, which used to be enough to pass it.""" + name = _workbench_script_name(gate) + doc = str(REPO_ROOT / "docs" / "tool-verification" / "EXTENSION-extraction-shell-bench.md") + path, why = gate.resolve_script(name, doc, include_workbench=False) + assert path is None + assert why == "unqualified-workbench", why + + text = "```\n$ python %s --flag\n```\n" % name + findings = gate.command_findings(doc, text, {}, False) + kinds = [f["kind"] for f in findings] + assert "unqualified-workbench" in kinds, findings + + +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) + 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 + 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 + + +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_the_committed_tree_reports_no_drift(gate, tmp_path): + """The gate's own verdict on this repository, run in-process so it cannot drift silently.""" + docs = gate.collect_docs(None) + findings = [] + cache = {} + for doc in docs: + try: + text = Path(doc).read_text(encoding="utf-8") + except OSError: + continue + findings.extend(gate.command_findings(doc, text, cache, False)) + drift = [f for f in findings + if f["kind"] in ("unknown-flag", "missing-script", "unqualified-workbench")] + detail = "\n".join(" %s:%s %s (%s)" % (f["doc"], f["line"], f["command"], f["kind"]) + for f in drift[:20]) + assert not drift, "documented commands that cannot run:\n%s" % detail