Review: validate NBA mirror and task grading (#30) - #101
Draft
jackjin1997 wants to merge 13 commits into
Draft
Conversation
Contributor
Author
|
Independent blind review (Claude Code, read-only) —
Checks this pass did not execute or assert
Scope: a read-only judgment of whether each frozen guided-regression execution completed its frozen task under the supplied non-answer-leaking rubric. It does not certify deterministic-verifier status, verifier correctness, source fidelity, asset provenance, or final readiness. |
Contributor
Author
|
NBA--12 blind-review reconciliation complete
The PR remains Draft. Live NBA.com visual/source comparison and the resource-gated full Docker rebuild/all-site smoke are still not verified. The unrelated Beta Daily Dunk panel's ordering is recorded as a non-blocking residual; no accepted task depends on it. |
Live same-condition comparison recorded the mirror as a visual-fidelity FAIL: a header that wrapped into stacked rows, oversized one-column card stacks, generic loose typography and a footer with a different hierarchy. Measured nba.com at 1440/768/390 and rebuilt the shared layer against those values: - 1280px content column with 28px gutters and a 55px single-row header at every width, with a no-JS hamburger panel below 1280px and a horizontally scrollable inline menu, so no destination is ever clipped away. - Media-led hero: right-hand media under a 90deg gradient, 474px copy column, related-content list, and slide controls with progress bars; the measured 485px mobile composition below 768px. - Stories rail as 188x282 portrait tiles and content rails as fixed 240px slides, both horizontally scrollable at every width instead of reflowing into full-width stacks. - Condensed uppercase display typography, measured section-title rules, card radii and the image-left Around the NBA row at narrow widths. - Footer rebuilt into the five measured link columns that collapse into a CSS-only accordion below 1024px, plus the social row and legal block. Upstream breakpoints replace the previous 1080/680 pair, so the narrow navigation regression test now asserts the invariant it was protecting (the menu scrolls and the hamburger panel repeats every destination) instead of a literal breakpoint string. Adds rail and footer regressions; all three fail against the previous revision. Verifier and contract suite: 20/20 test methods pass. Reset stays byte identical to the seed and the seed hash is unchanged.
Two gaps found while reviewing a task trajectory at the rebuilt header. The bar showed a static Sign In link on every page even while authenticated, so a signed-in run looked signed out. That has been true since the original contribution, and the cart count the app computes on every request was never rendered anywhere. The header now shows the account name linking to the account page, with a flyout for account, profile, orders, bag (using that count) and sign out; signed out it keeps Sign In plus create-account. NBA.com also opens a white flyout under each primary item on hover, which the mirror did not reproduce. Measured on the live site: panel flush under the 55px bar, 4px padding, links Roboto 600 14px/21px with 12px 16px padding, and a 720px Teams panel holding all 30 clubs in six division groups with logos at Roboto 400 13px and 4px 16px padding. Both shapes are rebuilt with the measured values, opened by :hover and :focus-within with no JavaScript. Panels default to display:none so their links stay out of the accessibility tree while closed, which keeps existing by-name locators resolving to the visible bar. Below 1280px, where the hamburger takes over, the flyouts are switched off and the drawer now repeats every flyout destination, so nothing reachable on a desktop becomes unreachable on a narrow screen. The menu scroller was moved to that same range because a scroll container clips absolutely positioned panels; between 1280 and 1365 item padding tightens so the full menu still fits without one. Adds regressions for the signed-in header and for the flyouts; both fail against the previous commit. Suite is 22/22. All 38 routes still return 200 with no horizontal overflow at 1440, 1280, 768 and 390.
Upstream main advanced to e7f8a5d while this review was open, merging the FedEx mirror into the port slot this branch had given NBA. Registry conflicts in websyn_start.sh and control_server.py are resolved by keeping the slot FedEx already merged into and appending NBA after it, so FedEx stays on 40024 and NBA takes 40025. No earlier site moves. The Dockerfile EXPOSE range and site count, and the shared port-range references in README, AGENTS, CONTRIBUTING, CLAUDE and the agent_demo README, follow the 26-site registry. Task paths are unchanged; only the origin moves, so every row in sites/nba/tasks.jsonl is re-pointed from :40024 to :40025. A new regression derives NBA's port from both launchers and asserts the task rows match it, rather than freezing the number, so the next registry reorder fails there instead of inside a benchmark run. It fails against the stale port and skips where the registry is not mounted. .assets-revision keeps this review's candidate 0cfa54bf rather than main's 68dcbf2c: 0cfa54bf is a superset carrying both fedex.tar.gz and nba.tar.gz, while 68dcbf2c has no NBA archive. The shared registry test is updated to that pin with a note that it is still an open Hugging Face PR and must be re-pinned to the HF merge commit before release. Registry invariants 6/6 and the NBA verifier suite 23/23.
…signs main advanced to f20b5ee with the WebMD Doctor mirror, which took the slot this branch had just been given. Same resolution as before: keep the slot the newly merged site occupies and append NBA after it, so WebMD Doctor stays on 40025 and NBA takes 40026 with no earlier site moved. The Dockerfile site count and EXPOSE range, the README site list and range, and the shared port-range references follow the 27-site registry. Task paths are unchanged; only the origin moves, so every row in sites/nba/tasks.jsonl is re-pointed to :40026. The registry-port regression added in the previous merge caught the drift and now passes against the new index. .assets-revision keeps 0cfa54bf. main pins ad6f424f, which carries 27 archives including webmd_doctor but no nba; 0cfa54bf carries 28 including all three of fedex, webmd_doctor and nba, so it remains the only pin that lets the integrated image build. The note that it is an open Hugging Face PR and must be re-pinned to the HF merge commit before release is retained. Run evidence is reused rather than re-executed. This merge touches exactly one NBA file, the web metadata port in tasks.jsonl; no template, stylesheet, application, seed or verifier code changed. The 20 frozen runs were re-graded against the updated task file and all 20 still pass, and the verifiers match on loopback plus path and query rather than on a port. Registry invariants 6/6 and the NBA suite 23/23.
Contributor
Author
|
Independent blind review — postfix-v3 execution bundle Model: Bundle:
Integrity: all 130 enumerated inputs (task contract, state evidence, 20 trajectories, 108 screenshots) matched their recorded SHA-256. Coverage: 20 / 20 tasks reviewed, none unexecuted. Not established by this pass / unexecuted checks:
This pass judges frozen-execution completion only. It is not a verifier-correctness, source-fidelity, or final-readiness sign-off. |
The standings page ranked Oklahoma City first in the West while team_seed(), used by the Games page, the game strip and the series page, ranked Denver first. The same site answered "who is the West top seed" two different ways depending on which page you were on. Oklahoma City and Denver both finished 2023-24 at 57-25 and the league seeds Oklahoma City first. That tiebreaker is not derivable from the seed data, so the standings page carried a hardcoded published order while team_seed() fell back to sorting by city, where "Denver" precedes "Oklahoma City". Hoist the published order into OFFICIAL_SEED_ORDER and a shared seed_sort_key() so every surface that displays a seed reads one source. Add SeedingConsistency, which asserts both call sites use the shared key, that neither sorts by city, and that the West resolves to Thunder then Nuggets on an identical record. Both tests fail against the previous code. They avoid importing the app so they run on a bare checkout. No accepted task changes answer: NBA--4 routes through the standings page, which was already correct, and no task asks for the seed badge shown on the Games page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Independent blind review — postfix-v5 packet
Packet integrity: all 130 enumerated inputs hash-matched (contract, state evidence, 20 trajectories, 108 screenshots), no missing files, all 20 task texts and rubrics match the frozen contract, and all screenshot dimensions are consistent with the declared full-page capture mode — so no decisive value sat below a fold in this review. Every comparison/superlative claim (NBA--2, --5, --6, --12, --15, --16, --18) was re-checked against the full-page image and none is contradicted by what the page shows. Disclosure. The manifest's reuse rationale asserts that no image in this packet shows a conference-seed badge for Oklahoma City or Denver. Two images in the packet contradict it — This blind pass covers frozen-execution completion only. It does not certify source fidelity, deterministic-verifier correctness, current NBA.com similarity, or final readiness. PR left in Draft; no GitHub or Hugging Face PR merged. |
Upstream merged healthline (index 26) and kaggle (index 27) while this review was open. Healthline took 40026, the slot this branch held, so NBA moves to index 28 and port 40028. No earlier site moves, which is what the shared registry invariants require. Nothing under sites/nba changes except the task file's declared port: the 20 rows move from :40026 to :40028. Paths, queries and required routes are byte-identical, and verifiers match on loopback plus path and query rather than a port, so no task answer or verdict can change. Known limitation, recorded rather than papered over: neither asset pin now covers the 29-site registry. Ours (0cfa54bf) carries nba but predates healthline and kaggle; main's (64264d06) carries those two but has no nba archive. This branch keeps its own pin so the NBA site builds, which means a full image build cannot fetch healthline or kaggle assets until HF PR aiming-lab#71 is refreshed onto the newer dataset main. That refresh must happen before this PR is mergeable. Shared registry invariants pass 6/6 and the NBA suite passes 25/25 in the runtime image, with the registry-port guard passing on a checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The merge commit moved NBA to index 28 in both launchers but left the task file declaring :40026, because the rewrite happened after the conflict resolutions were staged and `git commit` only takes what is staged. The committed tree was internally inconsistent. test_tasks_use_the_registered_nba_port, added when the previous merge moved the site, fails against that tree and passes against this one. That is what the guard was for: it derives the port from both launchers instead of freezing a number, so a registry move that the task file misses fails in the suite rather than inside a benchmark run. Only the declared origin changes. Paths, queries and required routes are byte-identical and verifiers match on loopback plus path and query. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Review takeover
This Draft supersedes and preserves the NBA mirror contribution in #30. The original mirror commit remains authored by
yichao; this branch adds reviewer-owned fixes, task hardening, deterministic verifiers, and an immutable asset candidate.0cfa54bf8e18558f32c44d128fa6ab1f384b5da9nba.tar.gzSHA-256:4ade0429edc12ffbfa1ee42a8f0192fc3009647912ff57a8d232d52abe7b349dc66dc67f571a35e3eb70a74896965fb0a3611417(merged ontomainf20b5ee)Changes in this review branch
mainand takes the port the 26-site registry now assigns;Visual fidelity repair
A same-condition comparison against live NBA.com recorded the previous UI as a fidelity failure: the header wrapped into stacked rows and clipped trailing navigation, card rails collapsed into oversized single-column stacks, display typography was generic, and the footer hierarchy differed. NBA.com was measured in a real browser at 1440, 768 and 390 px plus a 1440 → 320 sweep, a bounded difference table was frozen before any edit, and the shared layer was then rebuilt:
display: none, so their links stay out of the accessibility tree while closed. Below 1280px they are switched off and the hamburger drawer repeats every one of their destinations.Homepage document height against NBA.com under identical conditions:
Upstream / before / after screenshot sets were captured for 16 page templates at three widths each. The repaired result was reviewed against the live site in a real browser and accepted, so the post-fix bundle is now the approved visual regression baseline for this branch. Deliberate differences are retained and recorded: the seeded in-season scoreboard stays even though live NBA.com is currently in an offseason state, no advertising or third-party widgets are reproduced, no video playback is faked, benchmark answer-leak protections are untouched, and the upstream display face is licensed so the mirror uses a condensed fallback stack.
Verified at the current head
8c38e65and all 20 pass the deterministic verifiers, re-graded again against the current task file;sites/walmart_careers/tests/test_integration.pypass 6/6;POST /reset/nbaleaves the database byte-identical to the unchanged seed;reset-all, with the control plane reporting 26/26 alive and ready and NBA answering on its registry port 40025;ac356f97and returned 20 PASS / 0 FAIL, and its NBA--12 finding was fixed and independently re-checked.Upstream integration
mainmoved twice while this review was open, merging the FedEx mirror and then the WebMD Doctor mirror into the port slot this branch had been given. Both are integrated with the same rule: keep the slot the newly merged site occupies and append NBA after it, so no earlier site is remapped. NBA now takes 40026 in the 27-site registry, and the Dockerfile site count andEXPOSErange, the README site list and the shared port-range references follow it.Task paths are unchanged throughout; only the origin moves, so
sites/nba/tasks.jsonlis re-pointed each time. Verifiers match on loopback plus path and query and never on a port, so no verdict changes. A regression derives NBA's port from both launchers and asserts the task rows match it rather than freezing the number, so a future registry reorder fails in the suite instead of inside a benchmark run; it caught the second shift.All 20 tasks were re-executed at the first merge commit. For the second, which touches only the
webmetadata port and no NBA template, stylesheet, application, seed or verifier code, the frozen runs were re-graded against the updated task file instead: all 20 still pass.Draft limitations / pending review
.assets-revisionpoints at0cfa54bf, which carries bothfedex.tar.gzandnba.tar.gz;main's68dcbf2chas no NBA archive. HF PR Add Drugs.com mirror + task verifiers (site by @boyugou, verifiers by reviewer) #71 must merge and the pin must move to the HF merge commit before release.ac356f97, before the layout rebuild. A refrozen 20-run packet exists for the current head and is awaiting an isolated reviewer; the agent that made the UI change does not supply that verdict.reset-alltiming is unmeasured. It took 59.7s against the ~1s the repository documents, but the host carried 22 unrelated containers at load 150-306 and the health endpoint was itself timing out, while a direct sweep showed all 26 sites serving. No timing claim is made; it needs re-measuring on a quiet host.This PR intentionally remains Draft until the independent re-review, the Owner visual decision, the source audit and the full-build constraint are resolved.