Skip to content

fix(render): close-side </div> tolerance + consolidate dj-root scanners (#1751)#1754

Merged
johnrtipton merged 2 commits into
mainfrom
fix/dj-root-scanner-consolidation-1751
Jun 8, 2026
Merged

fix(render): close-side </div> tolerance + consolidate dj-root scanners (#1751)#1754
johnrtipton merged 2 commits into
mainfrom
fix/dj-root-scanner-consolidation-1751

Conversation

@johnrtipton

Copy link
Copy Markdown
Contributor

Closes #1751. Completes the #1749/#1750 dj-root-boundary fix class.

Problem

Two issues in the dj-root region boundary scan:

  1. Close-side whitespace. _find_closing_div_pos (the shared scanner, 6 call sites) located the close as re.search("</div>") — missing </div > / </div\n>. That's the close-side twin of fix: render_full_template ejects page content outside dj-root for multi-line <div> tags (leaks across dj-navigate) #1749's open-side under-count: a whitespace close over-counts depth so the close is never found → caller falls back / mis-splices.
  2. Two divergent scanners. render_full_template had its own hand-rolled depth loop (its open side was patched in fix(render): count multi-line <div> opening tags in dj-root boundary scan (#1749) #1750) separate from _find_closing_div_pos. Parallel-path drift (fix(auth): wrap per-event check_object_permission in sync_to_async (#1638) #1646) is exactly what let the open-side bug exist in one copy and not the other.

Fix

  • _find_closing_div_pos: </div></div\s*> (benefits all 6 call sites; close_match.end() consumes the full tag so splice points stay correct).
  • Replace render_full_template's hand-rolled loop with a _find_closing_div_pos call. The helper is multi-line-safe on the open side (<div\b, subsumes fix(render): count multi-line <div> opening tags in dj-root boundary scan (#1749) #1750) and now whitespace-tolerant on close; the rendered shell carries no {% %} tags so the helper's if/else handling is inert there. One scanner now, not two.

Verification

🤖 Generated with Claude Code

johnrtipton and others added 2 commits June 7, 2026 20:57
…root scanners (#1751)

Completes the #1749/#1750 dj-root-boundary fix class.

- `_find_closing_div_pos` (the shared scanner, 6 call sites) hardcoded the
  close tag as `</div>`, missing `</div >` / `</div\n>`. That is the close-side
  twin of #1749's open-side under-count: a whitespace close over-counts depth so
  the close is never found. Now matches `</div\s*>` (`close_match.end()` consumes
  the full tag, so splice points stay correct).
- Replaced `render_full_template`'s separate hand-rolled depth loop (its open
  side was patched in #1750) with a call to `_find_closing_div_pos`. The helper
  is multi-line-safe on the open side (`<div\b`) and now whitespace-tolerant on
  the close side; the rendered shell has no `{% %}` tags so the helper's
  if/else handling is inert. Removing the duplicate scanner closes the
  parallel-path-drift (#1646) that let the open-side bug exist in one copy only.

Adds close-side gate-off test (reverting `</div\s*>`→`</div>` makes the
whitespace test fail); existing multi-line + render-path suites stay green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@johnrtipton

Copy link
Copy Markdown
Contributor Author

Stage-7 Adversarial Review — PR #1754 (fix #1751)

Verdict: APPROVE

Close-side </div\s*> tolerance + consolidation of render_full_template's hand-rolled depth loop into the shared _find_closing_div_pos scanner. Empirically verified behavioral equivalence, close-regex correctness, all 6 call sites, gate-off non-tautology, no-regression, and full parallel-path-drift closure. Two 🟡 notes below; none blocking.

Mandatory gates

Empirical verification

  • 🟢 Close-regex correctness (python/djust/mixins/template.py:809): </div\s*> matches </div>, </div >, </div\n>, </div\t>, </div >, </DIV>; correctly rejects </divx>, </divider>, unterminated </div. close_match.end() consumes the FULL tag incl. trailing whitespace (verified end()==len for all whitespace variants) — so the FINAL-close splice boundary (return close_pos, pos + close_match.end(), line 839) is correct.

  • 🟢 Gate-off (non-tautology, tech-debt: tautology test detection #1200/Add remaining 13 Django built-in template filters #254): reverted ONLY </div\s*></div>; test_trailing_whitespace_close_tags_are_matched FAILED (scanner returned (None, None)) and the control test_plain_close_tag_still_works PASSED. The test genuinely exercises the change. Restored cleanly (no working-tree diff).

  • 🟢 All 6 call sites safe (lines 451, 727, 749, 769, 860, 992): every caller uses the RETURNED positions (result[0]=close_start, result[1]=close_end) for slicing. NONE does +6 arithmetic or assumes a 6-char close tag. The streaming path (451→465/467) uses result[0] for both body_content end and body_close_chunk start — the full close tag (any length) flows intact into the close chunk.

  • 🟢 No-regression: 35 passed (test_render_full_template_multiline_div_boundary.py + _djroot_substring_1746.py + test_ssr_render_normalization_1737.py + tests/unit/test_template_inheritance.py). The pre-existing TestMultilineDivBoundary exercises render_full_template end-to-end with multi-line divs and stays green under the consolidation.

  • 🟢 Parallel-path-drift fully closed (fix(auth): wrap per-event check_object_permission in sync_to_async (#1638) #1646): the old hand-rolled char-slice loop is GONE — grep for [i : i + 6] / == "</div>" / shell_html[i...] finds NOTHING in production (the one shell_html[i:i+5] hit is inside a docstring in the test file documenting the old buggy form; the only depth +=/-= 1 is inside the canonical _find_closing_div_pos itself). One scanner now, not two.

Consolidation behavioral-equivalence determination

  • 🟢 if/else ({% %}) branch logic is genuinely inert here. shell_html = temp_rust.render() (line 956) is RENDERED output — the Rust engine resolves and removes all {% if/else/endif %} tags before this point. The helper's flow_tags pre-scan finds nothing, pending stays empty, only the balanced-div scan runs. No {% %} can survive into shell_html.

  • 🟡 Open-matcher divergence on <div-…> custom elements (PRE-EXISTING, not introduced by this PR). OLD hand-rolled loop counted an open only when <div was followed by a char in " \t\r\n>/" — which does NOT include -, so <div-foo> counted as 0 opens. The shared scanner uses <div\b, and \b fires before - (non-word char), so <div-foo> counts as 1 open with no matching close → scanner returns (None, None)render_full_template falls through WITHOUT splicing the view. I reproduced this divergence (<div dj-root>…<div-foo>x</div-foo>…</div>: OLD close_end=50, NEW=None). However, _find_closing_div_pos already used <div\b on main and is already consumed by the HTTP/streaming path (render_to_response/arender_chunks, line 451) on the same rendered-shell input. So this is a pre-existing property of the shared scanner; the consolidation makes the WS push path CONSISTENT with the already-shipped HTTP path rather than introducing new divergence. A literal <div--prefixed custom element is also not a realistic rendered-shell shape. Worth a follow-up only if custom elements named div-* ever appear; not a regression of this PR.

  • 🟡 pos = close_pos + 6 advance is hardcoded for non-final close tags (line 840). When advancing PAST a non-final whitespace-padded close tag (e.g. </div > = 7 chars), +6 lands mid-tag (on the > / interior whitespace), not past it. Verified self-healing: the leftover \s*> fragment can never re-match </div\s*> (which requires the </div prefix), so the next iteration correctly finds the next real close. The new test's inner </div\n> is itself a non-final whitespace tag and exercises this path green. Correct today but fragile — a future change to the close regex that no longer requires the </div literal prefix would break it. Consider pos = pos + close_match.end() for symmetry with the final-close return. Non-blocking; behavior is correct.

Minor

  • 🟡 No NEW end-to-end render_full_template test for the close-side whitespace case in a rendered shell — the new tests target _find_closing_div_pos directly. Acceptable: the unit test gates the exact changed line, and render_full_template now merely delegates to the tested helper (and pre-existing end-to-end multi-line tests still cover the splice path).

Summary

The fix is correct, the consolidation is behaviorally equivalent on the inputs render_full_template actually sees (rendered shell, no {% %}, no div-* custom elements), the close regex is precise, all call sites are position-based, the gate-off proves the test is non-tautological, and the parallel-path drift is fully retired. APPROVE.

@johnrtipton johnrtipton merged commit 0fbf6cd into main Jun 8, 2026
14 checks passed
@johnrtipton johnrtipton deleted the fix/dj-root-scanner-consolidation-1751 branch June 8, 2026 01:07
@johnrtipton

Copy link
Copy Markdown
Contributor Author

Retrospective — PR #1754 (#1751)

Task: close-side </div> whitespace tolerance in _find_closing_div_pos + consolidate render_full_template's duplicate hand-rolled scanner into it. Completes the #1749/#1750 dj-root-boundary fix class.

Quality: 5/5

Closed the bug class (not just an instance): both the close-side twin and the duplicate scanner that enabled the original drift.

What went well

What could be improved / noted (non-blocking)

  • Stage-7 surfaced a pre-existing <div\b-vs-boundary-set divergence (<div-foo> custom elements counted differently) — already present in the HTTP path's use of _find_closing_div_pos; this PR makes the WS path consistent with it rather than introducing it. Very narrow (literal div--prefixed custom element in a rendered shell); left as-is.
  • The helper's non-final-close advance hardcodes pos = close_pos + 6 (len </div>); now self-healing under </div\s*> (the stray > fragment can't re-match), but + close_match.end() would be symmetric. Cosmetic; left to a future touch of that function.

Verified

RETRO_COMPLETE

johnrtipton added a commit that referenced this pull request Jun 8, 2026
…1752) (#1755)

nav-hooks-guard and playwright-tests duplicated ~9 setup steps (Python/uv/cache/
uv sync/maturin build/playwright install/migrate/start server/wait). #1753 had to
patch the missing maturin step into nav-hooks-guard precisely because the copy had
drifted. Extract the shared harness into .github/actions/djust-playwright-server
(composite); both jobs now `uses:` it after checkout, then run their own test step
+ log upload + server stop.

One definition can't drift a step out of one job (parallel-path-drift, #1646).
Both jobs keep their identity: playwright-tests stays continue-on-error (optional),
nav-hooks-guard stays blocking; test-summary wiring unchanged.

Closes #1752. Item 1 (maturin) shipped in #1753; item 2 (blocking-soak) resolved by
decision — keep nav-hooks-guard blocking (green across #1748/#1753/#1754); item 3
(this dedup).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

fix: close-side </div> whitespace tolerance + consolidate the two dj-root boundary scanners (follow-up to #1749)

1 participant