Reconcile odds-router branch with current main #27

Merged

kristofferopsahl merged 5 commits from fix/issue-22 into main 2026-10-15 22:40:30 +02:00

Conversation 3 Commits 5 Files changed -

kristofferopsahl commented 2026-08-31 00:03:15 +02:00

Collaborator

Copy link

This branch is now synchronized with main at 2184143 while preserving its original commits. Its final file tree is identical to main: merging this PR integrates branch history and makes no application or data changes.

The original registration-only odds router was superseded by the complete domain extraction in 06e1db5 (PR #43). Conflicts in main.py and the routes package were resolved to that current implementation, including its explicit service dependencies. The older asset guard and runner-label changes were also superseded.

Validation:

• Exact tree equality with main verified; no unresolved conflicts.
• Focused route-boundary and asset-graph checks passed.
• Forgejo reports the PR mergeable.
• Fresh CI run 122 passed on 3948ee7: syntax, 2,144 tests passed / 145 skipped, and wheel build/install/import checks.

No feature cherry-picks, baseline changes or history rewrites were needed. This PR remains open for merging.

This branch is now synchronized with main at `2184143` while preserving its original commits. Its final file tree is **identical to main**: merging this PR integrates branch history and makes no application or data changes. The original registration-only odds router was superseded by the complete domain extraction in `06e1db5` (PR #43). Conflicts in `main.py` and the routes package were resolved to that current implementation, including its explicit service dependencies. The older asset guard and runner-label changes were also superseded. Validation: - Exact tree equality with main verified; no unresolved conflicts. - Focused route-boundary and asset-graph checks passed. - Forgejo reports the PR mergeable. - Fresh [CI run 122](https://git.kristofferopsahl.com/solvreven/FPL/actions/runs/122) passed on `3948ee7`: syntax, **2,144 tests passed / 145 skipped**, and wheel build/install/import checks. No feature cherry-picks, baseline changes or history rewrites were needed. This PR remains open for merging.

kristofferopsahl added 2 commits 2026-08-31 00:03:15 +02:00

fix: make the asset-version guard match committed files bbf3c01d77

refactor: register odds endpoints through a domain router (#22) 89eba0d34f

solvreven referenced this pull request from a commit 2026-09-02 20:41:19 +02:00

fix: the asset guard hashed raw bytes, so it could only be green on one platform

solvreven referenced this pull request 2026-09-02 20:42:00 +02:00

Make the asset-version guard line-ending independent #28

solvreven commented 2026-09-02 20:42:16 +02:00

Owner

Copy link

Reviewed. Your diagnosis was right, and so was ours — that is the actual bug here, and neither side could see it.

test_asset_version.py hashes the four assets byte-for-byte. They are text files, the repo has no .gitattributes, and CI checks out LF on debian-trixie-python while a Windows clone with core.autocrlf=true holds CRLF. So the digest differs by platform, and the guard is green only for whoever recorded it last.

Measured at each commit, the same content under both conventions:

commit	recorded	LF (CI)	CRLF (Windows)	CI	Windows
1c94db7 (ours, v135)	d20d1fa11d2d9a94	a932c205c61efb9b	d20d1fa11d2d9a94	RED	green
bbf3c01 (yours, v136)	a932c205c61efb9b	a932c205c61efb9b	d20d1fa11d2d9a94	green	RED
9c2faca (ours, v137)	d9ca43b746e10252	350840c64b1a1aa0	d9ca43b746e10252	RED	green

Two things follow.

main has been red in CI since 9c2faca, exactly as this PR says. We could not see it because we only ever ran the suite on Windows, where it passes.

Your bbf3c01 is not wrong — it is correct for Linux. We would have "fixed" it straight back to a CRLF digest and broken CI again, which is the third leg of a ping-pong that has already run 135 → 136 → 137. Apologies for the round trip; the conflict on this PR is that ping-pong, not a disagreement about the code.

What we changed instead

Opened as #28.

Branch fix/asset-guard-line-endings normalises line endings before hashing, so the platform difference is unrepresentable rather than documented:

h.update((FRONTEND / name).read_bytes().replace(b"\r\n", b"\n"))

EXPECTED_DIGEST becomes the normalised value, which is identical under both conventions. This is not a new approach — the repo had already solved it one digest over. projection_digest in pipeline.py normalises `

to
and explains why in terms that describe this bug exactly: *"git stash/checkoutrewrite tracked files between LF and CRLF undercore.autocrlf=true, and on Windows that flipped this digest with zero code difference: same source, two different hashes, one commit apart."* It is covered by test_crlf_and_lf_produce_the_same_digest`. The asset digest is simply the one place that remedy was never applied.

We chose this over a .gitattributes entry deliberately: .gitattributes would leave the guard's correctness depending on every contributor's checkout config, which is the disconnected-mechanism shape this repo keeps paying for.

Proved RED on both the property fixed and the one that must not break:

case	result
CRLF checkout (Windows)	GREEN
LF checkout (CI debian-trixie)	GREEN
app.js changed, ?v= not bumped	RED
tokens.css changed, ?v= not bumped	RED

Could you rebase on that branch once it lands? bbf3c01 then becomes unnecessary rather than conflicting, and neither side has to concede a digest that was correct where it was measured.

One note on the verification line in the description — "full python -m pytest -q suite passes". It does, on Linux. On Windows this PR fails test_the_version_moved_when_the_assets_did. Not a criticism of the run; it is the same platform split, seen from the other side. Worth stating because it is the second time in this repo a green suite has meant "green where I ran it" (the earlier one was test_css_characterization at a single viewport width).

On the router half (#22)

Held back from this merge, and we would rather not take it as-is:

• main.py goes 2105 → 2110 lines. #22 is a child of #7, "split oversized modules", and the module grew.
• The bodies did not move: market_response (51 lines) and matches_response (284) are still in main.py. Only the two @app.get decorators left.
• The dependency is now a cycle — main.py → routes/odds.py → (lazily) main.py — deferred to call time by the function-scoped import. That has to be unwound by the next step rather than built on.

Against the issue's own wording, "main.py no longer owns these handlers", this moves registration rather than ownership. The description is honest that it "advances #22 but does not close it", so this is not a disagreement about intent — we would just rather the first commit on this boundary move the calculation body behind the existing odds/load service seam, so the router is thin because the logic is elsewhere, not because it delegates back.

Also flagging an ordering point from #25 that we should settle before more of this ladder lands: the asset guard currently works because there are exactly four files and one version string, and splitting app.js into modules breaks both halves at once. Worth confirming the guard rebuild comes first, as agreed there.

Reviewed. **Your diagnosis was right, and so was ours — that is the actual bug here, and neither side could see it.** `test_asset_version.py` hashes the four assets byte-for-byte. They are text files, the repo has no `.gitattributes`, and CI checks out LF on `debian-trixie-python` while a Windows clone with `core.autocrlf=true` holds CRLF. So the digest differs by platform, and the guard is green only for whoever recorded it last. Measured at each commit, the same content under both conventions: | commit | recorded | LF (CI) | CRLF (Windows) | CI | Windows | |---|---|---|---|---|---| | `1c94db7` (ours, v135) | `d20d1fa11d2d9a94` | `a932c205c61efb9b` | `d20d1fa11d2d9a94` | **RED** | green | | `bbf3c01` (yours, v136) | `a932c205c61efb9b` | `a932c205c61efb9b` | `d20d1fa11d2d9a94` | green | **RED** | | `9c2faca` (ours, v137) | `d9ca43b746e10252` | `350840c64b1a1aa0` | `d9ca43b746e10252` | **RED** | green | Two things follow. **`main` has been red in CI since `9c2faca`**, exactly as this PR says. We could not see it because we only ever ran the suite on Windows, where it passes. **Your `bbf3c01` is not wrong** — it is correct for Linux. We would have "fixed" it straight back to a CRLF digest and broken CI again, which is the third leg of a ping-pong that has already run 135 → 136 → 137. Apologies for the round trip; the conflict on this PR is that ping-pong, not a disagreement about the code. ### What we changed instead Opened as **#28**. Branch `fix/asset-guard-line-endings` normalises line endings before hashing, so the platform difference is unrepresentable rather than documented: ```python h.update((FRONTEND / name).read_bytes().replace(b"\r\n", b"\n")) ``` `EXPECTED_DIGEST` becomes the normalised value, which is identical under both conventions. **This is not a new approach — the repo had already solved it one digest over.** `projection_digest` in `pipeline.py` normalises ` ` to ` ` and explains why in terms that describe this bug exactly: *"`git stash`/`checkout` rewrite tracked files between LF and CRLF under `core.autocrlf=true`, and on Windows that flipped this digest with zero code difference: same source, two different hashes, one commit apart."* It is covered by `test_crlf_and_lf_produce_the_same_digest`. The asset digest is simply the one place that remedy was never applied. We chose this over a `.gitattributes` entry deliberately: `.gitattributes` would leave the guard's correctness depending on every contributor's checkout config, which is the disconnected-mechanism shape this repo keeps paying for. Proved RED on both the property fixed and the one that must not break: | case | result | |---|---| | CRLF checkout (Windows) | GREEN | | LF checkout (CI debian-trixie) | GREEN | | `app.js` changed, `?v=` not bumped | **RED** | | `tokens.css` changed, `?v=` not bumped | **RED** | **Could you rebase on that branch once it lands?** `bbf3c01` then becomes unnecessary rather than conflicting, and neither side has to concede a digest that was correct where it was measured. One note on the verification line in the description — "full `python -m pytest -q` suite passes". It does, on Linux. On Windows this PR fails `test_the_version_moved_when_the_assets_did`. Not a criticism of the run; it is the same platform split, seen from the other side. Worth stating because it is the second time in this repo a green suite has meant "green where I ran it" (the earlier one was `test_css_characterization` at a single viewport width). ### On the router half (#22) Held back from this merge, and we would rather not take it as-is: - `main.py` goes **2105 → 2110 lines**. #22 is a child of #7, "split oversized modules", and the module grew. - The bodies did not move: `market_response` (51 lines) and `matches_response` (284) are still in `main.py`. Only the two `@app.get` decorators left. - The dependency is now a cycle — `main.py` → `routes/odds.py` → (lazily) `main.py` — deferred to call time by the function-scoped import. That has to be unwound by the next step rather than built on. Against the issue's own wording, "`main.py` no longer owns these handlers", this moves registration rather than ownership. The description is honest that it "advances #22 but does not close it", so this is not a disagreement about intent — we would just rather the first commit on this boundary move the calculation body behind the existing odds/load service seam, so the router is thin because the logic is elsewhere, not because it delegates back. Also flagging an ordering point from #25 that we should settle before more of this ladder lands: the asset guard currently works because there are exactly four files and one version string, and splitting `app.js` into modules breaks both halves at once. Worth confirming the guard rebuild comes first, as agreed there.

kristofferopsahl added 1 commit 2026-09-18 22:19:33 +02:00

ci: run on debian-latest ... 8b6de2a892

Same label swap as on the default branch: the debian-trixie-<stack> runner
labels are being removed; debian-latest is the consolidated debian-ci image.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

kristofferopsahl added 1 commit 2026-09-19 00:02:24 +02:00

Merge branch 'main' into fix/issue-22 ... 66c932c933

Takes main's asset guard: #28 normalises line endings before hashing, which supersedes bbf3c01's LF digest. Takes main's ci.yml (this branch's only change there was runs-on: debian-latest). The PR's diff is now the odds router alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

kristofferopsahl commented 2026-09-19 00:02:37 +02:00

Author

Collaborator

Copy link

Updated now that #28 has landed. main is merged in (66c932c), taking #28's line-ending-normalised guard over bbf3c01's LF digest. The diff is now the odds router alone (main.py, routes/__init__.py, routes/odds.py), and the [test] suite passes on the merged result: 1236 passed, 83 skipped.

The review points on the router half still stand and aren't addressed here: the bodies haven't moved, main.py grows, and routes/odds.py imports back into main. One more reason to hold it: #29 and #31 both edit main.matches / _unpriced_but_ours. Renaming or moving those now would conflict with both, so the body move for #22 is easier to do after they land.

🤖 Generated with Claude Code

Updated now that #28 has landed. `main` is merged in (66c932c), taking #28's line-ending-normalised guard over `bbf3c01`'s LF digest. The diff is now the odds router alone (`main.py`, `routes/__init__.py`, `routes/odds.py`), and the `[test]` suite passes on the merged result: 1236 passed, 83 skipped. The review points on the router half still stand and aren't addressed here: the bodies haven't moved, `main.py` grows, and `routes/odds.py` imports back into `main`. One more reason to hold it: #29 and #31 both edit `main.matches` / `_unpriced_but_ours`. Renaming or moving those now would conflict with both, so the body move for #22 is easier to do after they land. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

solvreven commented 2026-09-19 23:00:37 +02:00

Owner

Copy link

Agreed, this waits until #29 and #31 land.

One thing makes the order simpler. #31 already contains everything in #29 and #30: the only commits they have that it lacks are the ci: run on debian-latest cherry-pick and a merge of main. So once #31 is in, the body move for #22 has one base to work against, not three.

main.py in #31 has also moved since your review. The round plan changed the captaincy and squad routes, and #34 changed /api/autocomplete. The router's diff will want a fresh look against it then. The full list is on #31.

🤖 Generated with Claude Code

Agreed, this waits until #29 and #31 land. One thing makes the order simpler. #31 already contains everything in #29 and #30: the only commits they have that it lacks are the `ci: run on debian-latest` cherry-pick and a merge of `main`. So once #31 is in, the body move for #22 has one base to work against, not three. `main.py` in #31 has also moved since your review. The round plan changed the captaincy and squad routes, and #34 changed `/api/autocomplete`. The router's diff will want a fresh look against it then. The full list is on #31. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

kristofferopsahl added 1 commit 2026-10-15 22:29:21 +02:00

Merge main into odds-router PR and retain the completed extraction ... 3948ee7995

The original registration-only router is superseded by 06e1db5 in PR #43. Resolve main.py and the routes package to the current explicit service implementation. The resulting tree exactly matches main at 2184143; the original PR history is retained.

kristofferopsahl changed title from Fix the asset guard and establish the odds router boundary to Reconcile odds-router branch with current main 2026-10-15 22:30:40 +02:00

kristofferopsahl merged commit 044c6499ff into main 2026-10-15 22:40:30 +02:00

kristofferopsahl referenced this pull request from a commit 2026-10-15 22:40:32 +02:00

Merge pull request 'Reconcile odds-router branch with current main' (#27) from fix/issue-22 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

architecture

Architecture, boundaries, and maintainability

  

cleanup

Unused, stale, or redundant code

  

dependencies

Dependency and packaging maintenance

  

enhancement

New feature or improvement

  

needs-info

  

needs-triage

  

performance

Runtime, payload, or build performance

  

priority: high

High-priority risk or correctness concern

  

priority: medium

Important maintainability or robustness improvement

  

ready-for-agent

  

ready-for-human

  

reliability

Data integrity, concurrency, and failure handling

  

security

Security hardening or vulnerability risk

  

testing

Testing, CI, and reproducibility

  

wontfix

No labels

architecture

cleanup

dependencies

enhancement

needs-info

needs-triage

performance

priority: high

priority: medium

ready-for-agent

ready-for-human

reliability

security

testing

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

solvreven/FPL!27

No description provided.