fix(masters): normalize whitespace around a non-empty Metrika counter id - #810
Conversation
_metrika_counter_identity extracts the numeric counter id shared by the suggestion-text and read-back tag-display formats so _verify_saved can compare them for equality. The non-empty branch returned the identity unstripped, so an id with incidental surrounding whitespace on one side but not the other (e.g. read-back "...12345 \n30 целей" vs. suggestion "...12345") would not compare equal, producing a false save-mismatch even though the add actually succeeded. Closes #809 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XPPspf5omgkZJpq6uk6yAR
🔍 Local review (cycle 1) — round 564e95fe-cb4c-4e02-8413-a38e69742e36Reviewed locally (
Cycle 1/3. |
Cycle-review finding (Codex, round 2, PR #810): stripping whitespace around a non-empty identity token also collapsed distinct non-numeric malformed tokens that differ only by surrounding whitespace (e.g. "Label A • foo " vs "Label B • foo "), narrowing the docstring's documented 'malformed inputs must not silently match' invariant beyond the intended numeric-id case. Restrict the stripping normalization to tokens that are genuinely numeric (.isdigit()) -- real counter ids are numeric per the original live recon. A stripped non-numeric token now falls back to the unstripped original text, fully restoring the invariant for malformed input while keeping the #809 fix for real numeric counter ids. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XPPspf5omgkZJpq6uk6yAR
🔍 Local review (cycle 2) — round 4243714e-0aac-4d6c-ac75-1e225fe32aefReviewed locally (
No FIX verdicts this round — reason to defer the design observation: the three guards each protect a distinct, tested, narrow scenario (empty token, whitespace padding, non-numeric malformed token); a regex rewrite risks silently changing behavior for one of the 12 existing regression tests without a corresponding maintainability win, and the current code passes cycle-review round 2 clean on correctness. Left as-is. Cycle 2/3 — clean round, no FIX. |
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 1 SKIP, 0 UNVERIFIED. Codex companion output was manually inspected both rounds due to a JSON-parse gap in the companion tool itself (prose preceding the JSON verdict block); accepted by explicit user direction as noted in this PR's round-1 comment. |
Summary
_metrika_counter_identity(direct_cli/browser/masters.py, added in PR #801) extracts the numeric counter id shared by the suggestion-text and read-back tag-display formats so_verify_savedcan compare them for equality. The non-empty branch returned the identity unstripped:If a real (non-empty) id has surrounding whitespace on one side but not the other — e.g. read-back
"domain • 12345 \n30 целей"(trailing space before the newline) vs. suggestion"label • domain/path • 12345"(no trailing space) — the two identities ("12345 "vs"12345") wouldn't compare equal, and_verify_saved'sCountercomparison would report a false save-mismatch even though the add actually succeeded.Fix
The existing empty/whitespace-only fallback behavior (PR #808's guard: return the original text unchanged when there's no real id after the last
" • "separator) is unchanged — only the non-empty branch now normalizes whitespace.Testing
test_strips_trailing_whitespace_around_a_real_id,test_strips_leading_whitespace_around_a_real_id, andtest_suggestion_and_whitespace_padded_tag_display_share_identity(the exact scenario from the issue: suggestion text with no padding vs. read-back tag-display text with incidental whitespace, both must normalize to the same identity) toTestMetrikaCounterIdentityintests/test_masters.py.pytest tests/test_masters.py -k MetrikaCounterIdentity— 10 passedpytest tests/test_masters.py— 843 passedpytest— 3492 passed, 23 skipped, no regressionsblack/flake8clean on all changed filesCloses #809
🤖 Generated with Claude Code