Fix the asset guard and establish the odds router boundary #27
No reviewers
Labels
No labels
architecture
cleanup
dependencies
performance
priority: high
priority: medium
reliability
security
testing
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
solvreven/FPL!27
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-22"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Outcome
main: version 135 recorded a digest for frontend files that were not committed, so the clean checkout suite was redbackend/web/routes/odds.pyand makes it own FastAPI registration for/api/marketand/api/matchesmain.pyThis advances #22 but does not close it: moving the remaining calculation body behind the existing odds/load service boundary should be reviewed as the next mechanical step.
Verification
python -m pytest -qsuite passesReviewed. Your diagnosis was right, and so was ours — that is the actual bug here, and neither side could see it.
test_asset_version.pyhashes the four assets byte-for-byte. They are text files, the repo has no.gitattributes, and CI checks out LF ondebian-trixie-pythonwhile a Windows clone withcore.autocrlf=trueholds 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:
1c94db7(ours, v135)d20d1fa11d2d9a94a932c205c61efb9bd20d1fa11d2d9a94bbf3c01(yours, v136)a932c205c61efb9ba932c205c61efb9bd20d1fa11d2d9a949c2faca(ours, v137)d9ca43b746e10252350840c64b1a1aa0d9ca43b746e10252Two things follow.
mainhas been red in CI since9c2faca, exactly as this PR says. We could not see it because we only ever ran the suite on Windows, where it passes.Your
bbf3c01is 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-endingsnormalises line endings before hashing, so the platform difference is unrepresentable rather than documented:EXPECTED_DIGESTbecomes 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_digestinpipeline.pynormalises `toand 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 bytest_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
.gitattributesentry deliberately:.gitattributeswould 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:
app.jschanged,?v=not bumpedtokens.csschanged,?v=not bumpedCould you rebase on that branch once it lands?
bbf3c01then 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 -qsuite passes". It does, on Linux. On Windows this PR failstest_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 wastest_css_characterizationat a single viewport width).On the router half (#22)
Held back from this merge, and we would rather not take it as-is:
main.pygoes 2105 → 2110 lines. #22 is a child of #7, "split oversized modules", and the module grew.market_response(51 lines) andmatches_response(284) are still inmain.py. Only the two@app.getdecorators left.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.pyno 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.jsinto modules breaks both halves at once. Worth confirming the guard rebuild comes first, as agreed there.View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.