Review follow-ups to the merged backlog batches (#293/#294) - #295
Merged
Conversation
Adversarial re-review of the two merged PRs surfaced three defects, all in the newest code. **Transition flush is not a flush when a save is in flight (#278 gap).** `saveNow()` with a save already in flight returns THAT promise — whose FormData snapshot predates any keystrokes typed since it started — and merely queues a follow-up. `submitTransition`'s single await therefore narrowed the publish race from the 1.2s debounce to one save RTT without closing it, and the queued follow-up save is a plain fetch the navigation aborts. The handler now repeats the flush (bounded at 5 attempts) until nothing is dirty or in flight, clears the re-armed timer and queued flag so no doomed fetch fires into the navigation, and refuses the transition if it exits the loop still dirty. **/admin/config 500s on a malformed URL-shaped value (#273 hardening).** `urlsplit` raises on an unbalanced IPv6 bracket and `.port` raises on a non-numeric or out-of-range port, and the new `_scrub_userinfo` / `_origin_only` helpers called both unguarded — so a typo'd proxy or webhook URL crashed exactly the page an operator opens to see their broken config. Both helpers now catch the parse failure; a URL-ish value that may carry a credential which cannot be safely carved out is hidden wholesale rather than guessed at. **Stale queued-save flag after a 409 (#271 polish).** A save queued behind the one that hit the optimistic-lock conflict survived recovery and fired one redundant re-post of the same stale version. The 409 branch now drops the queued flag so recovery starts clean. Regression tests for all three; suite 669 passed, all static gates clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQpYwRbt1YhpTXPb46MR5v
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An adversarial re-review of the two merged backlog PRs (#293, #294) surfaced three defects, all in the newest code. One commit, all three fixed with regression tests. No tracked issues — these are review findings on already-merged work.
Changes
tags.js, gap in the Report publish/transition races the 1.2s autosave debounce, can freeze stale content into the immutable snapshot #278 fix — medium).saveNow()with a save already running returns that promise, whoseFormDatasnapshot predates any keystrokes typed since it started, and merely queues a follow-up. The singleawaittherefore narrowed the publish race from the 1.2 s debounce to one save RTT without closing it — and the queued follow-up save is a plain fetch (nokeepalive) that navigation aborts, so nothing self-heals. Same consequence as the original bug: pre-edit text frozen into the immutable snapshot.submitTransitionnow repeats the flush (bounded at 5 attempts) until nothing is dirty or in flight, clears the re-armed timer and queued flag so no doomed fetch fires into the navigation, and refuses the transition if it exits the loop still dirty./admin/config500s on a malformed URL-shaped value (effective_config.py, hardening of the /admin/config leaks credential-bearing URLs (inline proxy creds, Slack webhook URL) despite 'never their value' promise #273 fix — low). The new_scrub_userinfo/_origin_onlyhelpers calledurlsplit(...)and.portunguarded;urlsplitraises on an unbalanced IPv6 bracket and.porton a non-numeric or out-of-range port. A typo'd proxy or webhook URL crashed exactly the page an operator opens to see their broken config. Parse failures are now caught, and an unparseable URL-ish value that may carry a credential which cannot be safely carved out is hidden wholesale ((unparseable URL — value hidden)) — failing toward redaction, consistent with /admin/config leaks credential-bearing URLs (inline proxy creds, Slack webhook URL) despite 'never their value' promise #273's deny-by-default posture.tags.js, polish of the Report editor: stale-write 409 is silent and unrecoverable under the one-save model #271 fix — nit). A save queued behind the one that hit the optimistic-lock conflict survived recovery and fired one redundant re-post of the same stale version. The 409 branch now drops the flag so recovery starts clean.Noted in review but deliberately unchanged: the NO_PROXY decision for OIDC uses the discovery URL's host for all three outbound calls (true for all four supported IdPs, documented assumption); the
rolesapp-claim allowlist in the #269 fix is name-based (claim naming is operator-controlled config, the intended trust boundary); andapplyAiReportFieldsshares the in-flight-snapshot staleness, but its queued follow-up save self-heals since nothing navigates away.Testing
uv run pytestpasses — 669 passed, 16 skippedruff,bandit,mypy,vulture,djlintall cleanNew tests: the flush-loop contract (bounded repeat while dirty/in-flight, timer + queued-flag cleanup, refuse-when-still-dirty ordering); malformed proxy/webhook URLs neither crash
snapshot()nor leak the embedded password, on both the env and DB rows; non-URL values with an@(the dev-login email) still render; and the 409 branch clearing the queued flag.Standing caveat from the parent PRs:
biome lintand theassetsdrift job run only in CI (not available in the dev sandbox). Only JS/CSS-comment-level changes here — no new Tailwind utilities.Checklist
ARCHITECTURE.md(the Report publish/transition races the 1.2s autosave debounce, can freeze stale content into the immutable snapshot #278 flush sentence now describes the repeat-while-in-flight behaviour),CHANGELOG.md🤖 Generated with Claude Code
https://claude.ai/code/session_01LQpYwRbt1YhpTXPb46MR5v
Generated by Claude Code