Skip to content

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

Draft
adewale wants to merge 4 commits into
mainfrom
claude/test-audit-campaign
Draft

adewale wants to merge 4 commits into
mainfrom
claude/test-audit-campaign

Conversation

@adewale

@adewale adewale commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

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. The independent reviewer and I first traced each bug by reading the code. On 2026-10-10 all five were executed at fc3b7bc with uv run python -m unittest discover -s tests, against main's versions of the two test files (old) and this PR's (new). Exit codes (old / new): control 0 / 0, (a) 0 / 1, (b) 0 / 1, (c) 1 / 1, (d) 0 / 1, (e) 1 / 0. They match the table exactly, and the tree was restored after every run (git diff --quiet HEAD).

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

Merge readiness

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
 }
]

Merged main and ran the checks (2026-10-10)

fc3b7bc merges main, which brings in the Wrangler security patch (#27). The verify job failed on 2026-10-03 at its first command, npm audit --audit-level=high, on the undici advisories in Wrangler's dependencies. That happened before any test ran. It had nothing to do with this PR's two test files.

The CI steps, run locally on fc3b7bc with CI=true, Node 22 and uv 0.12.22:

Command Result
uv sync --locked --all-groups and npm ci --ignore-scripts exit 0
npm audit --audit-level=high exit 0, found 0 vulnerabilities
git diff --exit-code -- pylock.toml, after pywrangler dev started locally exit 0
make verify The build, unit tests (250 tests, OK, including every test this PR adds), SEO cache lint, example verification and quality checks all passed. It stopped at browser-layout-test, which could not run in this container: its Chromium runs as root with --no-sandbox, and the page's readiness signal never fired. This PR changes only tests/test_main_observability.py and tests/test_worker_asgi_bridge_scope.py, so it cannot affect browser layout. CI's Chrome runs it
make lint and make check-generated exit 0
scripts/format_examples.py --check exit 0
make verify-python-version VERSION=3.13 exit 0
git diff --check exit 0

GitHub CI on fc3b7bc: verify passed, including browser-layout-test and search-ranking-test in Chrome. The earlier "Draft: none of this has been run" note is out of date for the tests. The PR stays a draft until the owner decides; nothing blocks marking it ready. The planted-bug table has now been executed too, and matches.

Cost and limits

  • CI: no workflow change. The new tests are added to two existing test files and run in the existing verify job.
  • The verify job is red on this head. No test command was run locally when this draft was written; see Merge readiness above.
  • PBT, fuzzing, mutation: none.
  • Moved to manual: nothing.

Reviewed against the owner's verification-cost standard on 2026-10-09: no code change needed.

Merged main after #19 (2026-10-11)

e270703 merges main at 2ab5b23. That brings in #19, which changes figures, theme tokens and figure-content checks. It is a normal merge commit with no rebase.

No conflicts: #19 and this PR change no common file. make build and make check-social-cards regenerated the generated files, and nothing drifted. No lockfile changed.

The CI steps, run locally on e270703 with CI=true:

Command Result
uv sync --locked --all-groups and npm ci --ignore-scripts exit 0
npm audit --audit-level=high exit 0
git diff --exit-code -- pylock.toml, after pywrangler dev exit 0
make verify, run with -k Every target except browser-layout-test passes. That includes 255 unit tests (both tests this PR adds among them), search-ranking-test, lint and check-generated. browser-layout-test fails with "Browser layout page did not become ready", and it fails the same way on the pre-merge head fc3b7bc. The failure is environmental, as on 2026-10-10
scripts/format_examples.py --check exit 0
make verify-python-version VERSION=3.13 exit 0
git diff --check exit 0

GitHub CI on e270703: verify passed in 53 s. It ran 255 tests, including both new tests, and browser-layout-test and search-ranking-test ran in Chrome.

The PR stays a draft.

🤖 Generated with Claude Code

https://claude.ai/code/session_019boFsNWgtPe2zZQxPFRRWp


Generated by Claude Code


Generated by Claude Code

claude added 2 commits October 3, 2026 01:34
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
Clean merge: #19 touched figures, theme tokens and figure-content
checks; this branch only touches the request-body-cap tests. Generated
files were rebuilt with make build and check-social-cards; nothing
drifted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012eisRQmQvg4TphwztxhQGJ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants