refactor(mp-m4bn): remove the unreachable encho cap; encho marker stays bare (E) - #376
Open
gitrgoliveira wants to merge 15 commits into
Open
refactor(mp-m4bn): remove the unreachable encho cap; encho marker stays bare (E)#376gitrgoliveira wants to merge 15 commits into
gitrgoliveira wants to merge 15 commits into
Conversation
MaxEnchoPeriods capped how many encho (overtime) periods a match could run, returning 400 max_encho_exceeded past the limit with a force=true override. Nothing could ever set it: no create-wizard field, no settings form field, no import-manifest key, and the settings PUT whitelist in handlers_competition.go never copied the field onto the stored record. The cap was unreachable through the product, so the clamp, the "Maximum encho periods reached" banner, the 400 and its force-retry confirm dialog were all dead code. Removed rather than completed. CHK029 asked whether requirements were DEFINED for a maximum; it was answered by building a mechanism instead of recording a decision, and no functional requirement ever called for a cap (FR-032 mandates tracking the period count, not bounding it). The decision: encho is unbounded. How many overtime periods are fought, and how a still-tied match is finally resolved (hantei for an individual bout, daihyosen for a team encounter), is the shimpan's call. By the time the operator enters a period count those periods have already been fought, so a validator that rejects the entry refuses to record what physically happened, during a live event where results cannot be rolled back (constitution VII). The operator records the outcome; the software does not referee it. Encho.PeriodCount is unchanged and still required by FR-032. The force flag stays on /decision for the unrelated T103 decision-lock override. Also drops a per-request store load: the bulk-score handler loaded the competition solely for this check, along with its 500 failure path. The MAX_ENCHO axis is removed from both score-editor config matrices, halving those suites. Closes mp-m4bn
Every consumer of Encho.PeriodCount tested it as a boolean (> 0), so the magnitude was consumed by nothing except the stepper that produced it: a match that ran three overtime periods rendered identically to one settled in the first, and the count FR-032 mandates persisting was never shown anywhere a result is actually read. The overtime marker now carries the count when more than one period ran: "(E)" for a single period, "(E×N)" for N > 1. One period keeps the bare marker because it is the common case and the terser form keeps narrow bracket nodes and Excel cells readable. This is what makes the operator's stepper taps worth recording. It is the counterpart to removing the cap: the software should not REFEREE how many overtime periods are fought, but it should faithfully RECORD how many were, and show it. Both languages duplicated the "(E)" literal across two call sites, so the logic is extracted to a single enchoLabel() per side (internal/export/ suffix.go and web-mobile/js/bracket.jsx) rather than patched in four places. All downstream surfaces inherit it: the bracket node chip, viewer and TV displays via decisionSuffix, the schedule page and OBS lower-third via formatIpponsScore, and the results workbook via DecisionSuffix. Two existing tests used multi-period fixtures while asserting the boolean-collapsed "(E)" and went red on this change, which is the pinning signal; both are updated and explicit ×N cases added on each side. Visual Rendering Contract updated in contracts/match-decisions.md.
The court-operator decision guide covered kiken, fusenpai/fusensho, hikiwake, daihyosen and chusen, but never encho, so the overtime toggle and its result marker were undocumented. Documents the unbounded period counter (the shimpan decide how many periods are fought and how a still-level match is settled) and the (E) / (E×N) marker, including how it combines with other tags.
… negation Two ignore-rule gaps surfaced while capturing this PR's screenshots. The Playwright MCP writes browser_take_screenshot output to the repo/worktree root, and loose root PNGs were not ignored, so a `git add -A` would sweep a screenshot into a commit. The three captures for this PR sat untracked at the root for exactly that reason. Anchor the rule at the root (`/*.png`): a blanket `*.png` would silently swallow new docs/screenshots/, docs/assets/images/ and specs/*/screenshots/ files, all of which are tracked (30 PNGs today). Separately, the `!specs/*/screenshots/` negation was dead. Git cannot re-include a file whose parent directory is excluded, and `specs/*` excluded the spec directories outright, so git never descended into them and the negation could not fire. The 11 UAT screenshots stayed visible only because they were already tracked; a newly added one would have been ignored, defeating the stated intent of the comment above it. Re-including the directories with `!specs/*/` lets git descend, while `specs/*/*` still excludes their contents. Verified with git check-ignore: root PNGs ignored; docs/assets, docs/screenshots, specs/*/screenshots and specs/openapi.yaml tracked; specs plan.md and contracts/ still ignored; all 30 tracked PNGs unaffected; no previously-hidden file becomes visible in either the worktree or the main checkout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review) Tri-review of PR #376 surfaced three comments still describing the removed cap machinery: - admin_scoring_modal.test.jsx header claimed the helpers implement the "T104 cap + banner" flow this PR deleted, and pinned a "seven" helper count the barrel no longer matches. - failingCompetitionStore's doc comment pointed at the deleted enforceEnchoCap / bulk-score fail-closed path; its only remaining consumer is the override-winner load-error test. - concurrency_test.go listed "LoadCompetition to check encho cap" among the contended lock steps; that per-request load is gone. Comment-only; make go/test exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four-lens cleanup pass (reuse / simplification / efficiency / altitude) over the PR diff. Two confirmed findings, both applied: - bracket.jsx formatIpponsScore: drop the unreachable bare-"(E)" fallback. decisionSuffix() already embeds the encho marker unconditionally, so decSfx can never be empty while enchoLabel(encho) is non-empty; the second encho path was dead and the T097 comment now names decisionSuffix() as the single source of the composed suffix. - admin_scoring_shared.jsx: pass nextEnchoPeriod directly to the state setter instead of wrapping it in an identity arrow. The helper itself stays: it is the exported test seam paired with the clamping prevEnchoPeriod and carries the no-maximum design comment. Skipped: the editors' duplicated "(E) Overtime ×N" eyebrow spans are pre-existing lines outside this diff with a deliberately more verbose format than the compact chip. make go/test exit 0; vitest 2406/2406. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second four-lens pass over the PR diff (reuse and efficiency came back clean). Five findings applied: - bracket.jsx MatchCard: gate the encho badge on enchoLabel's own emptiness instead of a separate periodCount > 0 test, so the guard and the label cannot disagree. - bracket.jsx: update the FR-033 comment still describing the pre-PR bare-"(E)" marker; it now names the (E)/(E×N) contract. - admin_competition_bracket.jsx: drop "maxEncho" from the enrichment comment (the fetch no longer reads it) — twin of the admin_pools.jsx fix already in the diff. - validation.go: drop the "bypassing the cap check" clause from the negative-count guard rationale; the guard's real justification stands. - admin_scoring_modal.test.jsx: delete the duplicate "+ never clamps" case from the floor-invariants block; the nextEnchoPeriod unit block pins the identical loop (and more). Skipped: genericizing the docs' composed-tag examples (concrete operator examples are deliberate); inlining the single-use expandEncho test helper (harmless). make go/test exit 0; vitest 2405/2405. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Third four-lens pass (reuse and efficiency clean again). Three findings, all comment/test level, all applied: - bracket.jsx decisionSuffix: add the reciprocal mirror pointer to Go's DecisionSuffix. The Go docstring declared the mirror contract one-directionally; now an editor of either side sees the lockstep requirement (and the noted deliberate fusensho divergence). - suffix.go + bracket.jsx enchoLabel comments: qualify "keep the three in sync" — the editor eyebrow shares the count-carrying contract but is a live stepper readout that deliberately never collapses ×1, so nobody "fixes" it into false consistency. - admin_scoring_modal.test.jsx: delete the residual "+ / -" describe block. Its - floor loop was a strict subset of the prevEnchoPeriod unit block (which also pins 0 and negative inputs) and the symmetry case is definitionally true of c+1 / max(1, c-1); the block's cap-era purpose (composing across the boundary) no longer exists. make go/test exit 0; vitest 2403/2403. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cleanup pass 4 over the encho diff. - handlers_match.go: drop the now-unused CompetitionStore parameter from registerScoreHandler. enforceEnchoCapWithSubs was its only consumer, and revive's unused-parameter check is disabled, so nothing else caught it. - suffix.go: replace three hand-rolled "space-join, skipping empties" branches (encho append, hantei append, MiddleCellText) with one joinSp helper, matching the one-line shape the JS mirror uses. - bracket.jsx: reattach the decisionSuffix docstring, which the enchoLabel insertion had orphaned above the wrong function, and give enchoLabel its own opening line. - admin_scoring_shared.jsx: the identity-arrow cleanup reached the + button in round 1 but not its twin; pass prevEnchoPeriod by reference too. - score_display.test.jsx: pin the Go/JS enchoLabel mirror. The contract now carries a threshold and a glyph held together only by comments, and the drift is silent (edit the JS + its JS test, the Go suite stays green, the workbook disagrees with the screen). Cases derive the contract from observed JS output and assert internal/export/suffix.go matches; verified red by flipping the Go threshold to > 0. - validation.go / validation_test.go: drop the last "cap check" reference in the tree and rewrap the ragged sentence left by the round-2 edit. - models.go: restore the blank line the MaxEnchoPeriods deletion consumed, which had merged the TeamMatchType doc block onto the duration field. - recording-decisions.md: narrow the marker-coverage sentence. A team encounter's score cell shows team totals, so the marker reaches the bracket and the workbook but not the console/viewer score string. Gates: make go/test exit 0, vitest 2406/2406, make docs/build exit 0.
…golden table
Cleanup pass 5. The main finding is against round 4's own parity test.
That test asserted SOURCE SUBSTRINGS of internal/export/suffix.go from the
JS side (`.includes('PeriodCount > 1')`, `.toContain('"(E×"')`). Verified
hole: switching Go's single-period marker to "(OT)" -- the exact divergence
the case was named for -- passed green. It would also have gone red on a
behaviour-preserving refactor such as fmt.Sprintf.
Replaced with internal/export/testdata/encho_labels.json, one table both
suites drive: TestEnchoLabel_GoldenTable table-drives the Go side,
score_display.test.jsx it.each's the JS side over the same file. This pins
VALUES, so it catches the drift per-language tests structurally cannot --
editing one renderer AND its own expectations in the same commit, which two
independent tables both survive. Verified in both directions: mutating the
Go marker reddens the Go suite, mutating the table reddens the JS suite.
Also:
- .gitignore: negate `*.json` for `**/testdata/*.json`. The blanket rule
silently swallowed the new fixture, so it would have passed locally and
failed CI on a file that never landed. No other testdata fixture was
affected; the gap was latent.
- bracket.jsx: decisionSuffix composes via [...].filter(Boolean).join(" "),
the idiom already used in ui.jsx/display_scoreboard.jsx, replacing two
copies of the ternary-space concat. Round 4 extracted joinSp on the Go
side only, leaving a mirrored pair that no longer read alike; the Go
docstring no longer quotes a JS shape that has since changed.
- Trimmed enchoLabel's docstring in both languages (it stated the same
three-branch contract three times) and the shimpan rationale duplicated
across four sites down to one canonical statement plus pointers.
- Merged two nextEnchoPeriod cases that pinned the same branchless property.
Gates: make go/test exit 0, vitest 2408/2408, make docs/build exit 0.
…le layer
Round 6 audited round 5's additions. Reuse and efficiency lenses CLEAN
(the filter/join allocation was costed at ~100ns per call behind a
React.memo boundary — noise).
- score_display.test.jsx: drive the golden it.each through
decisionSuffix({encho:{periodCount}}) instead of formatIpponsScore.
With no base label and no Ht that returns enchoLabel's output
byte-for-byte, so the table value is asserted EXACTLY and an unrelated
formatIpponsScore change (separator, ippon joining) can no longer
redden the mirror with a false "update both renderers" diagnosis.
Re-verified the pin catches table drift after the rewrite.
- .gitignore: widen !**/testdata/*.json to !**/testdata/**/*.json. The
one-level negation re-created the fixture-swallowing trap one
directory down (testdata/golden/... would silently never land);
probe-verified nested fixtures are now trackable.
- Deduplicate the shared-table rationale, which was written out nearly
verbatim in three places (Go test docstring, JS describe comment,
JSON _comment). The JSON _comment is now canonical — it travels with
the artifact both suites read — and both test comments shrink to
pointers. The vitest empty-it.each guard gains a one-line why (vitest
silently produces zero tests over an empty array, unlike jest).
- Drop the inert "why" fields from the table (neither driver decodes
them; the one non-obvious note, multi-digit N, moved into _comment).
- Drop the encho x7 DecisionSuffix row (identical path and property to
x2 one line above; value variety now lives in the golden table) and
the duplicate periodCount 2/5 it in score_display.test.jsx.
Gates: make go/test exit 0, vitest 2407/2407 (-1 = the deleted
duplicate), nested-fixture probe shows ?? in git status.
The Match Decision Types bullet still described the visual suffix as bare (E), the exact boolean-collapsed contract this PR removed. CLAUDE.md is the instructions layer every future session reads first, and the shared golden table only pins the two existing renderers: a new surface built from this gloss would hardcode bare (E) with nothing going red. Point the gloss at enchoLabel and the shared table instead of restating the values. (Simplify round 8: three lenses clean, altitude found this in the one layer prior rounds never audited.)
…rded, not displayed Operator decision: "Encho is just Encho. Do not add numbers to the marking. It will get confusing." The (E×N) counted marker introduced earlier in this PR is withdrawn. - enchoLabel (Go + JS): any positive periodCount renders bare "(E)"; zero/absent renders "". Docstrings now state the contract and why, and quote the rejected "(E×3)" shape as do-not-reintroduce. - The stepper is unchanged: still unbounded, still records periodCount to bracket.json for the tournament log. Only result displays change. - The editors' "(E) Overtime ×N" eyebrow stays: it is a live readout of the stepper the operator is using, not a result marking. - Golden table (encho_labels.json): all positive counts pin "(E)"; the periodCount 11 case now pins that digits never leak into the marker. _comment records the operator decision. - Test expectations updated on both sides; docs paragraph rewritten (marker is always (E), counter records the number for the log); CLAUDE.md gloss updated. Gates: make go/test exit 0, vitest 2407/2407, make docs/build exit 0.
gitrgoliveira
changed the title
Operator requirement: "If there's an Encho, the mark (E) needs to be in
the centre, between the ippon." formatIpponsScore now renders the encho
marker as the separator ("M (E) ·") instead of a trailing tag
("M–· (E)"), matching the paper score sheet's centre column and the
Excel export, which already writes the marker into the centre "vs" cell
via MiddleCellText. Decision tags (Kiken / Fus. / DH / Ht) stay
trailing; paths with no two-sided score (scoreless X draw, kiken before
any ippon, 0-0 hantei) keep the whole composed decisionSuffix since
there is nothing to sit between.
No Go change: the Excel layout has carried the marker in the centre
cell all along. decisionSuffix/enchoLabel contracts are untouched, so
the shared golden table and the single-tag display surfaces (scoreboard
vs-row, lobby, OBS lower-third) are unaffected.
Docs and CLAUDE.md updated to state the centre-column convention.
Gates: make go/test exit 0, vitest 2407/2407, make docs/build exit 0.
Two new screenshots for the court-operators guide, captured from a live run at 339d55c: the score editor with the overtime counter at two periods, and a public-viewer result showing the (E) marker in the centre of the score. The encho section previously reused the generic score-editor screenshot and showed the marker nowhere.
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
1 participant
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
MaxEnchoPeriodscapped how many overtime periods a match could run, rejecting anything past the limit with a400 max_encho_exceededplus aforce=trueoverride. Nothing could ever set it: no create-wizard field, no settings-form field, no import-manifest key, and the settingsPUTwhitelist never copied the field onto the stored record. The cap was unreachable through the product, so the clamp, the "Maximum encho periods reached" banner, the 400 and its force-retry dialog were all dead code.(E). A counted marker ((E×N)) was trialled in this PR and withdrawn on the tournament owner's decision: "Encho is just Encho. Do not add numbers to the marking. It will get confusing." Any overtime result shows(E), placed in the CENTRE of the score between the two sides' points (M (E) ·), exactly where it goes on a paper score sheet — matching the Excel export, which has always written the marker into the centre "vs" cell. Decision tags (Kiken,DH,Ht) stay trailing. The shared golden table (internal/export/testdata/encho_labels.json) pins the bare-marker contract in both languages, including that multi-digit counts never leak digits into the marker.forceflag stays on/decisionfor the unrelated T103 decision-lock override.Why
CHK029 asked whether requirements were defined for a maximum encho count. It was answered by building an enforcement mechanism instead of recording a decision, and no functional requirement ever called for a cap: FR-032 mandates tracking the period count, not bounding it.
The decision, confirmed with the tournament owner: encho is unbounded. How many overtime periods are fought, and how a still-level match is finally settled (hantei for an individual bout, daihyosen for a team encounter), is the shimpan's call. By the time the operator types a period count those periods have already been fought, so a validator that rejects the entry refuses to record what physically happened, during a live event where results cannot be rolled back (constitution VII). The operator records the outcome; the software does not referee it.
Recording and marking are deliberately split: the stepper faithfully records how many periods were fought (
periodCountpersists tobracket.jsonfor the tournament log), while the result marking stays a single glanceable(E)so brackets and result sheets stay easy to read. The editors' "(E) Overtime ×N" eyebrow keeps the count visible to the operator while they are recording it; results never show the number.Incidental win: the bulk-score handler loaded the competition on every request solely for this check, along with a 500 failure path. Both are gone.
Files changed
internal/state/models.goMaxEnchoPeriodsfieldinternal/mobileapp/handlers_match.gointernal/mobileapp/handlers_decision.goForcefor the T103 decision lock)internal/mobileapp/handlers_match_test.gointernal/export/suffix.goenchoLabel()(bare(E)for any positive count) +joinSp;DecisionSuffixcomposes via itinternal/export/suffix_test.go+testdata/encho_labels.jsonspecs/openapi.yamlmaxEnchoPeriodspropertyweb-mobile/js/admin_scoring_shared.jsxshouldShowEnchoMaxBanner/canIncrementEncho, the clamp, the banner and themax_encho_exceededretry branchweb-mobile/js/bracket.jsxenchoLabel()(bare(E));formatIpponsScorecentres it between the scores (M (E) ·); bracket chip +decisionSuffixweb-mobile/js/admin_scoring_individual.jsxweb-mobile/js/admin_scoring_team.jsxweb-mobile/js/admin_scoring_modal.jsxweb-mobile/css/styles.css.encho-row__max-bannerruleweb-mobile/js/admin_pools.jsx,admin_competition_swiss.jsxweb-mobile/js/__tests__/render/score_editor_matrix_axes.js+ both matrix suitesMAX_ENCHOaxis, halving both matricesweb-mobile/js/__tests__/admin_scoring_modal*.test.jsx,score_display.test.jsx,admin_competition.test.jsxdocs/user-guide/court-operators/recording-decisions.md(E)marker, with two new screenshots (previously undocumented).gitignore!specs/*/screenshots/negation; un-ignore**/testdata/**/*.jsonfixturesScreenshots
Captured at head
339d55cb(fresh build, live run through the real UI; the viewer shot's footer shows the build hash). The same captures ship in the docs (docs/screenshots/mobile-encho-overtime.png,viewer-result-encho.png), embedded in the court-operators guide.1. Overtime counter at ×2, no cap. The + button stays enabled, no "Maximum encho periods reached" banner, no alert. The header's "(E) Overtime ×2" is the operator's live stepper readout — the count is recorded, never displayed on results.
2. Admin bracket: bare
(E)on the completed node (periodCount 2 recorded).3. Public viewer:
M (E) ·— the marker sits in the CENTRE, between the two sides' scores; footer shows build339d55c.Superseded: earlier captures showed the withdrawn (E×N) marker (7c939b3…97c35ae1) and the trailing bare-(E) placement (c13a434)
The counted-marker screenshots (
encho-stepper-x4-uncapped.png,bracket-encho-x4.png,viewer-encho-x4-completed.png,reverify-*.pngon the pr-assets branch) documented the(E×N)display that commitc13a4345withdrew, and thebare-e-*.pngset showed the interim trailing placement (M–· (E)) that339d55cbmoved to the centre. The cap-removal behaviour they prove (stepper uncapped at ×4, no banner, persistence) was re-proven in the current screenshots above.Test plan
make go/testpasses (lint + security scan + tests) — all packages ≥85% coverage,mobileapp85.7%(E)and went red on this change; both updated, with explicit ×N cases added on each sidedocs/(make docs/buildpasses, exit 0).gitignorerules verified withgit check-ignore: root PNGs ignored;docs/screenshots/,docs/assets/images/,specs/*/screenshots/andspecs/openapi.yamlstill tracked; all 30 tracked PNGs unaffected; no previously-hidden file newly exposedBrowser verification at head
339d55cb(real app, throwaway tournament, 4-competitor knockout; earlier passes verified the same cap-removal behaviour at7c939b3f,97c35ae1andc13a4345):+stayed enabled, zero[role=alert], no "Maximum encho" banner;−disabled at the ×1 floor. Header eyebrow reads(E) OVERTIME ×2(live stepper readout).bracket.jsonshowswinner,status: completed,encho: {periodCount: 2}— the count is recorded.(E)chip in the node header (no count), and the winner advanced to the final.M (E) ·— the marker in the CENTRE between the two scores; the page footer shows build339d55c, proving the app under test is this PR's head.Console: 0 errors / 0 warnings. All screenshots were read back as images (legibility, centring, no overflow), not just DOM-asserted.
Closes mp-m4bn