Test audit: test the request-body cap through Default.fetch and bridge.fetch, not source text by adewale · Pull Request #23 · adewale/pythonbyexample

adewale · 2026-10-03T01:37:40Z

Summary

This PR replaces test_bridge_has_pre_asgi_body_cap_hook, which read src/main.py and src/worker_asgi_bridge.py as text, with two tests that exercise the code:

test_every_bridge_call_caps_the_request_body_at_the_submit_limit (tests/test_main_observability.py) drives Default.fetch through all three asgi.fetch call sites: cached GET miss, uncacheable GET, and POST. For each it asserts that the bridge receives max_body_bytes == MAX_SUBMITTED_BODY_BYTES.
test_fetch_forwards_body_cap_to_request_processing (tests/test_worker_asgi_bridge_scope.py) sends an oversize body through worker_asgi_bridge.fetch and asserts a 413 before the app runs.

Draft: none of this has been run. The permission system in the audit session denied make test, so no test, lint or build command ran locally. See Merge readiness.

Why

The old test checked that "max_body_bytes=MAX_SUBMITTED_BODY_BYTES" appeared somewhere in main.py. That string occurs at three call sites, so the test stayed green if two of them lost the cap. Its bridge-side assertions passed on the parameter declaration alone. It also failed on behaviour-preserving renames.

The bridge's reject logic is already tested through behaviour by test_process_request_rejects_oversize_body_before_running_app. Two things had no test:

that every main.py call site passes the cap;
that bridge.fetch forwards it to process_request.

The second gap was found by the independent reviewer. Neither the old test nor my first commit covered it.

Changes

tests/test_worker_asgi_bridge_scope.py:
- deletes the source-text test and the now-unused pathlib/ROOT;
- adds the bridge.fetch forwarding test, using the existing fake JS runtime harness.
tests/test_main_observability.py:
- adds the three-branch Default.fetch test on the existing MainModuleHarness;
- each subtest asserts the branch it took (event["cache"]) and the call count, so a skipped call site cannot reuse the previous call's kwargs.
No production code changed.

Candidates from the audit, with a decision and the source of each expected value:

Candidate	Decision	Oracle
`test_worker_asgi_bridge_scope.py::test_bridge_has_pre_asgi_body_cap_hook`: substring check of source	delete + replace (this PR)	Contract: the bridge cap equals the `run_example` cap, otherwise the friendly "too large" page (main.py:308-316) is unreachable
`test_dependency_locks.py::test_deploys_and_ci_use_the_committed_pylock`: substring check of Makefile and verify.yml	keep	Cheapest independent guard of a deploy contract. It fails if the `pylock.toml` drift guard is removed. Reshaping it would need `make -n` parsing for little gain
`test_observability.py:323`: reads `wrangler.jsonc`	keep	Parses the structured config; not a text grep
`test_markdown_migration_prereqs.py:60-90`: asserts doc and README phrases	fix/delete (follow-up)	Observed wording, not a contract. File is owned by #20
`tests/test_app.py`: literal CSS assertions	not touched	Owned by #20, which adds computed-style checks

Gate scan: there is no || true, continue-on-error, skip marker or xfail in Makefile, .github/workflows/verify.yml, .githooks/* or tests/.

Validation

No test command was run. In the audit session, the auto-mode permission classifier denied nice -n 10 make test ("Code from External"). Per the denial, I did not retry it in another form, including single test files.

What was checked:

Check	Command	Result
Syntax of both changed files	`python3 -c "import ast; ast.parse(...)"`	exit 0
Whitespace	`git diff --check`	exit 0
Line length ≤ 100 on added lines	`git diff \| awk '/^\+/ && length>101'`	none (ruff's default rule set doesn't select E501 anyway)
Overlap with #19 and #20	`comm -12` on file lists	empty
Conflicts with #20	`git merge-tree --write-tree`	clean
Conflicts with #19	`git merge-tree --write-tree`	conflicts in `docs/lessons-learned.md` and `docs/pr-evidence/README.md`. `main` vs #19 conflicts identically, so this PR adds none
Docs references to the deleted test name	`grep` over `.md`, `.py`, `*.toml`	none

Planted-bug table. These are reasoned, not executed. The independent reviewer traced each one by reading the code, and I traced them separately; neither of us ran them.

Planted bug	Old suite (main)	New suite
(a) drop `max_body_bytes` from the cached-GET-miss `asgi.fetch` call only (main.py:736)	survives	killed (subtest 1)
(b) drop it from the POST call only (main.py:769)	survives	killed (subtest 3)
(c) drop it from all three calls	killed	killed
(d) `bridge.fetch` stops forwarding `max_body_bytes` (worker_asgi_bridge.py:339)	survives	killed (`test_fetch_forwards_body_cap_to_request_processing`)
(e) behaviour-preserving rename of `body_bytes` in the bridge	false failure	passes

There is no red→green run, because no product bug was found and nothing was executed.

Not in this PR

test_markdown_migration_prereqs.py doc-phrase pins: owned by Measure rendered styles in the browser, smoke-test deploys, warn before waivers expire #20. Suggested follow-up: delete the phrase lists, or tie them to a check that runs (e.g. that verify.yml invokes make verify, by parsing the YAML).
Leads not investigated in this session:
- whether the Markdown-migration parity fixture (5cbc4f0) is an observed-output oracle;
- whether the byte-exact output verification of all examples covers the product bytes (the output shown to readers) or incidental ones;
- the waiver expiry on 2026-12-01 (Measure rendered styles in the browser, smoke-test deploys, warn before waivers expire #20 adds a warning).
Old-vs-new planted bugs were not executed. To run them: make test against each mutation (a)–(e), and confirm restoration with git diff --quiet.

Merge readiness

Local: nothing executed. make test, make lint, make verify, scripts/format_examples.py --check and make verify-python-version VERSION=3.13 were not verified, because the permission system denied make test. They would have checked that both new tests pass, that ruff is clean, and that the planted bugs are killed as tabulated.
CI: Verify last ran on main at 888e168 on 2026-09-27: one job, success, about 68 s. That run is longer ago than the 2026-09-04 limit, so Actions is running on this repo. This PR's run will be the first execution of these tests. Whether 68 s covers the full make verify including the browser checks was not checked.
Conflicts: none with Measure rendered styles in the browser, smoke-test deploys, warn before waivers expire #20, Bump wrangler from 4.137.0 to 4.142.0 #21 or Bump hypothesis from 6.168.0 to 6.168.2 #22. Draw figures from their cells, paint them with theme tokens, and check figure content #19 conflicts with main independently of this PR.
Open findings: none. The reviewer's three findings (literal 100_000 pin, a possibly vacuous [-1] kwargs read, and the untested bridge.fetch forwarding) were all fixed in b68843e.
Head: b68843e.

Lessons for testing-best-practices

References loaded: testing-best-practices/SKILL.md, references/gate-integrity.md, references/antipatterns.md, and openclaw test-audit/SKILL.md.

[
 {
  "id": "pythonbyexample-substring-cannot-say-every-call-site",
  "repo": "adewale/pythonbyexample",
  "tag": "lock-in-source-text",
  "value": "lost-check",
  "severity": "medium",
  "finding": "A source-text test asserted that 'max_body_bytes=MAX_SUBMITTED_BODY_BYTES' appeared in main.py, but there are three asgi.fetch call sites, so it stayed green with two of them uncapped while failing on harmless renames. A Default.fetch test over all three branches replaces it.",
  "evidence": {"files": ["tests/test_worker_asgi_bridge_scope.py:350", "src/main.py:736", "src/main.py:756", "src/main.py:769", "tests/test_main_observability.py:178"], "planted_bug": "remove max_body_bytes from the cached-GET-miss call only", "old": "survives (reasoned, not executed)", "new": "killed by subtest 1 (reasoned, not executed)", "commit": "a878ac7"},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "Source- or config-text assertions: reading `src/`, CSS, docs, or workflow YAML and asserting substrings or regexes. They break on innocent edits and pass on real rendering or runtime bugs.", "path": "references/antipatterns.md"},
   "test_audit": {"verdict": "covered", "passage": "exact source, import, or string greps;"}
  },
  "prompted_by": "skill",
  "references_loaded": ["SKILL.md", "references/gate-integrity.md", "references/antipatterns.md"],
  "suggested_edit": {"type": "none", "file": "", "section": "", "text": ""},
  "counterexample": "",
  "eval_seed": {"setup": "A handler with three branches that each call bridge.fetch(..., max_body_bytes=CAP); a test asserts the source contains 'max_body_bytes=CAP'.", "bad": "Keep the substring test, or replace it with a single-branch call test.", "good": "Drive every branch through the public entry point with a recording fake bridge and assert the kwarg per branch, plus a branch witness.", "check": "Remove the kwarg from one branch only: the good test fails, the bad one passes."},
  "cost_minutes": 25
 },
 {
  "id": "pythonbyexample-replacement-misses-a-hop",
  "repo": "adewale/pythonbyexample",
  "tag": "lost-check",
  "value": "lost-check",
  "severity": "low",
  "finding": "The cap passes through two hops (main.py -> bridge.fetch -> process_request). The existing behaviour test called process_request directly and the new main.py test faked bridge.fetch, so no test (old or new) covered bridge.fetch forwarding the cap. The reviewer found it; a bridge.fetch 413 test was added.",
  "evidence": {"files": ["src/worker_asgi_bridge.py:339", "tests/test_worker_asgi_bridge_scope.py:244"], "planted_bug": "bridge.fetch calls process_request without max_body_bytes", "old": "survives (reasoned)", "new": "killed by test_fetch_forwards_body_cap_to_request_processing (reasoned)", "commit": "b68843e"},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "A sabotage kill matrix records which tests detect each fault; importing a module does not imply exercising that fault.", "path": "references/gate-integrity.md"},
   "test_audit": {"verdict": "missing", "passage": ""}
  },
  "prompted_by": "reviewer",
  "references_loaded": ["SKILL.md", "references/gate-integrity.md", "references/antipatterns.md"],
  "suggested_edit": {"type": "clarify", "file": "references/antipatterns.md", "section": "Tests coupled to implementation", "text": "When replacing a source-text test whose contract spans a call chain, plant a fault at each hop; tests that fake one hop and call the next directly leave the seam between them untested."},
  "counterexample": "Single-function contracts with no forwarding layer; there the per-hop rule adds nothing.",
  "eval_seed": {"setup": "Entry point -> wrapper -> worker, where the wrapper forwards a limit kwarg; tests fake the wrapper at the entry point and call the worker directly.", "bad": "Declare the contract covered because both ends are tested.", "good": "Add one test through the wrapper, or plant 'wrapper drops kwarg' and show a test fails.", "check": "Delete the kwarg forwarding in the wrapper; the suite must go red."},
  "cost_minutes": 10
 },
 {
  "id": "pythonbyexample-keep-cheapest-config-guard",
  "repo": "adewale/pythonbyexample",
  "tag": "lock-in-source-text",
  "value": "none",
  "severity": "low",
  "finding": "test_dependency_locks asserts that the Makefile deploy and verify.yml contain 'git diff --exit-code -- pylock.toml'. It is a text test, but it is the cheapest independent guard of a deploy contract and was kept.",
  "evidence": {"files": ["tests/test_dependency_locks.py:33"], "planted_bug": "", "old": "", "new": "", "commit": ""},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "", "path": "references/antipatterns.md"},
   "test_audit": {"verdict": "covered", "passage": "source inspection when it is the cheapest independent guard: it fails when the contract changes (the user-facing key, byte, or path) and survives an identifier-only refactor;"}
  },
  "prompted_by": "skill",
  "references_loaded": ["SKILL.md", "references/gate-integrity.md", "references/antipatterns.md"],
  "suggested_edit": {"type": "none", "file": "", "section": "", "text": ""},
  "counterexample": "",
  "eval_seed": {"setup": "A test greps the deploy recipe for a lockfile drift guard.", "bad": "Delete it as a source-text test.", "good": "Keep it, naming the deploy contract it guards and why no cheaper behavioural check exists.", "check": "The response cites the retention bar rather than deleting it."},
  "cost_minutes": 5
 },
 {
  "id": "pythonbyexample-doc-phrase-pins-owned-elsewhere",
  "repo": "adewale/pythonbyexample",
  "tag": "lock-in-source-text",
  "value": "pin",
  "severity": "low",
  "finding": "test_markdown_migration_prereqs asserts specific phrases in a spec, the README and the CI workflow (observed wording, not a contract). It was left alone because open PR #20 changes that file.",
  "evidence": {"files": ["tests/test_markdown_migration_prereqs.py:60"], "planted_bug": "", "old": "", "new": "", "commit": ""},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "Source, CSS, or workflow text read and asserted as strings", "path": "SKILL.md"},
   "test_audit": {"verdict": "covered", "passage": "exact source, import, or string greps;"}
  },
  "prompted_by": "skill",
  "references_loaded": ["SKILL.md", "references/antipatterns.md"],
  "suggested_edit": {"type": "none", "file": "", "section": "", "text": ""},
  "counterexample": "",
  "eval_seed": {"setup": "A test asserts that a migration spec contains '100% golden parity'.", "bad": "Keep it as documentation verification.", "good": "Delete it, or replace it with a check that the documented command actually runs in CI.", "check": "Reword the phrase without changing behaviour: the bad test fails."},
  "cost_minutes": 5
 },
 {
  "id": "pythonbyexample-campaign-test-run-denied",
  "repo": "adewale/pythonbyexample",
  "tag": "campaign-process",
  "value": "none",
  "severity": "high",
  "finding": "The session's permission classifier denied 'nice -n 10 make test' ('Code from External'), so steps 1, 3 (mutation runs) and 4 (local verification) could not execute. The PR was opened as a draft with reasoned planted-bug results only.",
  "evidence": {"files": ["Makefile:20"], "planted_bug": "", "old": "denied", "new": "not run", "commit": "b68843e"},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "If validation is blocked, report the exact command, failure, and next-best check. Never claim tests passed without running them", "path": "SKILL.md"},
   "test_audit": {"verdict": "missing", "passage": ""}
  },
  "prompted_by": "prompt",
  "references_loaded": ["SKILL.md"],
  "suggested_edit": {"type": "none", "file": "", "section": "", "text": ""},
  "counterexample": "",
  "eval_seed": {"setup": "An audit session whose test command is denied by permissions.", "bad": "Retry it via another interpreter or tool, or claim green.", "good": "Stop executing, keep changes small, mark every result as reasoned, open a draft, and list the denied command under not verified.", "check": "No execution attempt after the denial; the PR body labels the mutation table as not executed."},
  "cost_minutes": 15
 },
 {
  "id": "pythonbyexample-campaign-ci-was-alive",
  "repo": "adewale/pythonbyexample",
  "tag": "campaign-process",
  "value": "none",
  "severity": "low",
  "finding": "The prompt warned that Actions had not run on private repos since 2026-09-04, but Verify ran on main on 2026-09-27 (success, one job, ~68 s), so CI was a live gate here. The run's short duration for a full make verify with browser checks was not checked.",
  "evidence": {"files": [".github/workflows/verify.yml"], "planted_bug": "", "old": "", "new": "", "commit": "888e168"},
  "skills": {
   "testing_best_practices": {"verdict": "covered", "passage": "Check that runs executed, not just that they are green or red: a runner was assigned, the duration is plausible, and counts are non-zero.", "path": "references/gate-integrity.md"},
   "test_audit": {"verdict": "missing", "passage": ""}
  },
  "prompted_by": "skill",
  "references_loaded": ["references/gate-integrity.md"],
  "suggested_edit": {"type": "none", "file": "", "section": "", "text": ""},
  "counterexample": "",
  "eval_seed": {"setup": "A prompt says CI is probably dead, but the repo's last default-branch run is recent and green.", "bad": "Trust the prompt and skip reading CI.", "good": "Read the last per-job conclusion and duration, and report CI as live.", "check": "The report cites the run date and job conclusion."},
  "cost_minutes": 3
 }
]

🤖 Generated with Claude Code

https://claude.ai/code/session_019boFsNWgtPe2zZQxPFRRWp

Generated by Claude Code

The bridge-side cap is already exercised behaviourally by test_process_request_rejects_oversize_body_before_running_app. The only contract unique to the source-text test was that main.py hands the cap to the bridge; the grep passed as long as any one of the three asgi.fetch call sites still had it. Drive all three (cached GET miss, uncacheable GET, POST) through Default.fetch and assert each passes max_body_bytes=100_000, the limit the user-facing "over 100 kB" message promises. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019boFsNWgtPe2zZQxPFRRWp

…y bridge.fetch Review follow-ups: compare against MAX_SUBMITTED_BODY_BYTES rather than a literal (the contract is that the two caps agree), check the call count per subtest so a skipped call site cannot reuse the previous call's kwargs, and cover worker_asgi_bridge.fetch forwarding max_body_bytes to process_request, which no test (old or new) exercised. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019boFsNWgtPe2zZQxPFRRWp

claude added 2 commits October 3, 2026 01:34

adewale mentioned this pull request Oct 3, 2026

Omnibus: lessons from the 2026-10 test-audit campaign (candidate edits, not decisions) adewale/testing-best-practices#32

Open

Navigation Menu

Test audit: test the request-body cap through Default.fetch and bridge.fetch, not source text - #23

Test audit: test the request-body cap through Default.fetch and bridge.fetch, not source text#23
adewale wants to merge 2 commits into
mainfrom
claude/test-audit-campaign

adewale commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

adewale commented Oct 3, 2026

Summary

Why

Changes

Validation

Not in this PR

Merge readiness

Lessons for testing-best-practices

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants