Skip to content

Review: Adopt-a-Pet mirror + task verifiers (site by @sarendis56, verifiers by reviewer) - #100

Open
JeremyJC67 wants to merge 15 commits into
aiming-lab:mainfrom
JeremyJC67:review/pr-94-adopt_a_pet
Open

Review: Adopt-a-Pet mirror + task verifiers (site by @sarendis56, verifiers by reviewer)#100
JeremyJC67 wants to merge 15 commits into
aiming-lab:mainfrom
JeremyJC67:review/pr-94-adopt_a_pet

Conversation

@JeremyJC67

Copy link
Copy Markdown
Contributor

Summary

Review vehicle for #94 (adopt_a_pet, site by @sarendis56). Carries the contributor's commit unchanged (8e11e7d7, authorship preserved) plus reviewer commits that add the grading contract and the two mechanical fixes the review found. The full evidence-backed review is posted on #94; this PR is what should merge once the contributor repacks the HF tarball.

What the reviewer commits add

  • sites/adopt_a_pet/verify/verify_0.py … verify_19.py, shared verify_lib.py (package validation, loopback origin + port check, navigation gates with filter/page params, negation-aware answer matchers, SQLite snapshot contract with exact row deltas, werkzeug-scrypt password check with stdlib only), ground_truth.py (frozen catalog; a drifted seed fails every verifier closed), README.md (per-task contract table).
  • sites/adopt_a_pet/tasks.jsonl — every row now carries verifier_path + an English judge_rubric (rules and fact-checkpoints, no answers; no answer key). Three questions were re-anchored because they were not gradable as written: T1 ("find the kitten" matched two kittens → "the kitten whose listing is in Scottsdale itself"), T2 (the correct answer flipped between Phoenix, AZ = 7 dogs / Batman $165 and Phoenix = 2 dogs / Horus $210 → the query is now quoted and every results page must be checked), T19 ("young dogs near Arizona" returned exactly one dog, so the "lowest fee among those good with children" comparison was vacuous → all Arizona dogs, good with children, lowest fee: 8 profiles on 2 pages).
  • sites/adopt_a_pet/verify/tests/ — 272-test harness (synthetic snapshots + agent.py-shaped trajectories, every verifier run as a subprocess; dockerignored via the existing sites/*/verify/tests/ rule).
  • sites/adopt_a_pet/requirements.txt — was Flask only; import app failed on flask_sqlalchemy outside the image. Pinned to the image's versions.
  • Docs/tests sweep to 25 sites / 40000-40024 (README, AGENTS, CONTRIBUTING, CLAUDE, agent_demo/README, sites/walmart_careers/README.md, sites/walmart_careers/tests/test_integration.py, sites/rotten_tomatoes/tests/test_environment_quality.py) — the same sweep feat(webmd_doctor): add WebMD Doctor mirror (site 25, port 40024) #95 did; the doc-range test now derives the range from control_server.SITES.

Validation

Check Result
Byte-identical seed (standalone, 4 boots/resets) 376b1ca1… every time
20 tasks driven through the UI with Playwright, run dirs in the agent.py shape 20/20 PASS
no-op / shortcut / wrong-answer variants (×20) + state-mismatch (×6) 66/66 FAIL, unexpected: none
agent_demo/eval_judge.py --verifier True PASS on the genuine T0 run, FAIL (visited_dog_search_near_phoenix) on its shortcut
uv run python -m unittest discover -s sites/adopt_a_pet/verify/tests 272 tests OK
Registry / docs tests (walmart_careers, rotten_tomatoes) on this branch green
Docker (bundle extracted by hand because its layout is rejected) build ok (4.69 GB); 25/25 sites 200; /health 25/25; POST /reset/adopt_a_pet byte-identical to the build-generated seed (993d6113…) before/after a dirty request and after docker restart; /reset-all 25/25 in 1.46 s; no-op verifier matrix in the container 20/20 FAIL; registry tests green; site suite 272 passed

Still required from the contributor (see the review on #94)

  1. HF tarball layout — members must sit under adopt_a_pet/ (currently static/images/…, instance_seed/… at the root plus 25 ._* AppleDouble members); scripts/validate_asset_archive.py rejects it, so fetch_assets.sh and a fresh-clone Docker build fail. Repack, push to HF Add GOV.UK mirror + task verifiers (site by @lamawmouk, verifiers by reviewer) #67, then pin .assets-revision to the merged SHA.
  2. Photos vs. listings — 12 of the 20 pets show a photo of the wrong species, breed or colour (e.g. Archie the Australian Shepherd is a grey cat; Olive the Siamese and Teddy the Poodle share one dog-plus-tabby photo; the six article-*.avif files double as the /blog banners). Re-harvest per-pet photos or rewrite those rows.

Notes for maintainers

  • Port 40024 collides with feat(webmd_doctor): add WebMD Doctor mirror (site 25, port 40024) #95 (webmd_doctor, also index 24); whichever merges second needs re-indexing (websyn_start.sh, control_server.py, Dockerfile EXPOSE, all 20 web fields, docs/tests).
  • Verifiers are deterministic-first and decide with --no_llm True; llm_* helpers exist only for parity with merriam_webster.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP

sarendis56 and others added 12 commits September 10, 2026 03:04
requirements.txt listed only Flask, but app.py imports flask_sqlalchemy;
`python -c "import app"` failed with ModuleNotFoundError outside the Docker
image (which masks it by installing Flask-SQLAlchemy globally). Pin the same
versions the image uses.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
…all 20 tasks

Reviewer grading contract for PR aiming-lab#94: sites/adopt_a_pet/verify/verify_0..19.py
with a shared verify_lib.py (package validation, loopback origin + port check,
navigation gates with filter/page params, negation-aware answer matchers,
SQLite snapshot contract with exact row deltas, stdlib werkzeug-scrypt check)
and ground_truth.py (frozen catalog; a drifted seed fails every verifier
closed). Every tasks.jsonl row now carries verifier_path + judge_rubric; no
answer key. Deterministic-first: verdicts never depend on an LLM.

Three questions re-anchored because they were not gradable as written:
- T1: "find the kitten" matched two kittens (Neo, Amba) -> the kitten whose
  listing is in Scottsdale itself.
- T2: the answer flipped with the query string ("Phoenix, AZ" -> 7 dogs,
  Batman $165; "Phoenix" -> 2 dogs, Horus $210) -> quote the query, check
  every results page.
- T19: "young dogs near Arizona" returned exactly one dog, so the comparison
  was vacuous -> all Arizona dogs good with children, lowest fee (8 profiles
  on 2 pages).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
272 unittest cases under sites/adopt_a_pet/verify/tests (dockerignored):
synthetic snapshots built from ground_truth.py, trajectories in the
agent_demo/agent.py shape, every verifier run as a subprocess. Per task:
genuine PASS, run-dir snapshot discovery, no-op, wrong task id, off-origin
URL, corrupt PNG, missing snapshots and catalog drift (fail closed),
shortcut without each gate page, wrong / misattributed answers, read-only
writes; stateful tasks add state mismatch, collateral rows, wrong user,
duplicates and reordered workflows.

Run: cd agent_demo && uv run python -m unittest discover \
     -s ../sites/adopt_a_pet/verify/tests -p 'test_*.py'

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
PR aiming-lab#94 registers adopt_a_pet as site 25 but left the shared docs and the
registry assertions in sites/walmart_careers/tests/test_integration.py and
sites/rotten_tomatoes/tests/test_environment_quality.py at 24 sites /
40000-40023, so both suites were red on the branch. Same sweep as aiming-lab#95; the
doc-range test now derives the expected range from control_server.SITES.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
The mirror shipped a hard-coded SECRET_KEY, so a session cookie signed with it
granted another account without a password (verified: forged user_id=2 returned
200 and "Hi, Bob Smith" on /account). It also had no CSRF protection, bounced
POST /favorite to an attacker-supplied Referer, honoured ?next=http://evil, and
accepted empty, over-long and out-of-vocabulary form values silently.

- SECRET_KEY from ADOPT_A_PET_SECRET_KEY or secrets.token_hex(32), matching
  compass / osu / target / ted / rotten_tomatoes / walmart_careers
- user() refuses a non-int session user_id (a forged {"user_id": {...}} was a 500)
- CSRFProtect + csrf_token in the six state-changing forms (Flask-WTF 1.2.2 is
  already installed in the image; added to requirements.txt)
- redirects restricted to same-site relative paths
- bounded_text / one_of / bounded_int on the inquiry, alert and registration
  forms; invalid input now renders the form with an error and HTTP 400
- MAX_CONTENT_LENGTH=256*1024 and PRAGMA foreign_keys=ON, as in walmart_careers
- 400/404/500 error handlers and templates
- email validated by regex, deliberately not email_validator, which rejects the
  RFC 6761 .test TLD this offline mirrors own accounts use

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
…gation chrome

The Previous/Next hrefs re-emitted the current page inside filters|urlencode and
then appended the new one, producing ?...&page=2&page=1. Werkzeug returns the
FIRST value, so Previous from page 2 served page 2 again - reachable on the two
paginated task paths (2 and 19). The range label also read "Showing 1 - 12 of 12"
on page 2 because the start index was hard-coded to 1.

- pagination hrefs built from a base query string with page stripped
- start index (page-1)*per_page+1
- Distance select disabled with a title: the app has no geo data and ignores it
- Shop and Help are inert labels rather than links that both went to /blog
- the /#rehome nav target now has a real section on the homepage
- hero placeholder "City, State or ZIP" instead of "Phoenix, AZ", which was the
  exact location tasks 0 and 2 ask the agent to type

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
Measured in Chromium at 1440/768/390/320 with checkVisibility() plus screenshot
pixel sampling to falsify each finding.

- hero and search-summary labels were #999/#aaa: 2.85:1 and 2.32:1 against the
  white they actually paint on, below the 4.5:1 body-text floor -> #5f5f5f
- /search overflowed at 768 (scrollWidth 827) and 320 (360), /shelters at 390
  and 320 (417): form controls carry an intrinsic min-content width, so the 1fr
  grid tracks could not shrink -> min-width:0 and minmax(0,1fr)
- the stylesheet defined no focus styles at all; added an explicit :focus-visible
  ring in the ink colour

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
Task 4 said "Find Arno through the pet search", but the search box scores tokens
against city, state and postal only, never the pet name: /search?location=Arno
returns 0 results. The task now names the location that reaches him and says so
explicitly. Same verifier gates (a /search visit, /pet/arno, /shelter/1).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
…rt 40026)

main gained fedex (40024) and webmd_doctor (40025). Merged rather than rebased so every
contributor and reviewer commit on this branch is preserved verbatim. Shared registry,
Dockerfile EXPOSE, docs and every site-local port reference move to index 26 / port 40026;
the slot stays provisional and the maintainer reassigns it at merge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
@sarendis56

Copy link
Copy Markdown
Contributor

Contributor follow-up for the remaining pet-photo and archive findings:

Focused photo mapping / credits PR, rebased onto your latest 1166ad6 review tip, targets this review branch directly. It includes desktop/mobile screenshots and preserves your tasks, verifiers, and every non-image catalog field.

HF PR #67 is updated at 8316809f5eed94078c9b188e5c315f67df751be1: 20 distinct pet photos at usable resolution, corrected species/breed/coat pairings, article banners kept separate, and matching seed image paths. The archive uses supported roots without Mac metadata; the downloaded upload passes the archive validator. The code PR provides complete photo provenance and a public credits page.

Verification: 272 existing verifier tests passed; 80 visible-browser checks passed across 1440, 768, 390, and 320 px; the rebased container build passed with 27/27 sites ready; reset returned ready and a direct byte comparison matched the seed database.

Please land the small mapping fix with the asset update, then pin the HF merge revision in .assets-revision after HF merge.

…erge

adopt_a_pet ships no instance_seed: its seed is generated at image build time. Merging the
26-site main took upstream's Dockerfile wholesale, which dropped that RUN step, so
websyn_start.sh failed at 'cp -a .../instance_seed' and the container exited 1 before the
control plane came up. Step restored verbatim ahead of EXPOSE.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
@JeremyJC67

Copy link
Copy Markdown
Contributor Author

Both halves check out. Thank you for turning these around so quickly.

Assets. Downloaded refs/pr/67 at 8316809f on the review host and ran the repo's own validator rather than trusting the summary:

bytes: 2422612   md5: 70273804f142687affaab540b9dab424
members: 34      AppleDouble/DS_Store: 0
archive roots: adopt_a_pet/    second level: adopt_a_pet/instance_seed  adopt_a_pet/static
validate_asset_archive.py -> validated 34 managed members for adopt_a_pet

That closes the archive-layout blocker.

Code PR (JeremyJC67#1). Reviewed against the current review tip. It merges cleanly, touches no tasks.jsonl and no verify/ file, and the app.py change is confined to the image column — the reused article-*.avif and rehome*.avif banners are replaced by per-pet files, which is exactly the finding. The footer gains a Photo credits link. I will land it once the container run currently in flight finishes, so the verification covers the final tree rather than an intermediate one.

One small thing I will add on top when I merge, rather than ask you for another round: this branch's .dockerignore does not exclude review-reports/, so the two remediation screenshots would be COPYd into the published image. I added that exclusion on the 4shared branch for the same reason and will do the same here.

After that the only open item is release mechanics: a maintainer merges HF #67, then I pin .assets-revision to the merge sha.

JeremyJC67 and others added 2 commits September 12, 2026 22:02
Pairs each of the 20 listings with its own matching photograph, replacing the reused
article banners and the species/breed mismatches the audit filed. Touches no tasks.jsonl
and no verifier; app.py changes only the image column.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
The photo remediation adds two screenshots under review-reports/, which this branch's
.dockerignore did not exclude, so they would be COPYd into the published image. Same
exclusion the 4shared review branch already carries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AkNTAZMziDodR6uqa6QBtP
@JeremyJC67

Copy link
Copy Markdown
Contributor Author

Rebased onto the 26-site main — this branch is now site 27 / port 40026

main merged FedEx (40024) and WebMD Doctor (40025) overnight, which put every open site
branch into conflict. This branch has been brought up to date by merging main rather than
rebasing, so every contributor and reviewer commit is preserved verbatim, and re-slotted to
index 26 / port 40026: registry, Dockerfile EXPOSE, the shared docs and every
site-local port reference follow. The slot is provisional as always — say the word at merge
time and I will re-slot in whatever order you land things.

The whole environment was then rebuilt and re-verified at the new slot:

check result
asset fetch fetch_assets.sh adopt_a_pet at refs/pr/67validated 34 managed members through the normal path, no hand extraction
HTTP sweep 27/27 return 200
GET /health ok, 27/27 alive and ready
POST /reset/adopt_a_pet → md5 2d56013ab6ea2b115e13ad4a59d530bd, identical to the seed
POST /reset-all ok, 27/27 ready, 1.51 s
verifier no-op matrix 20/20 FAIL
registry + docs tests 4 passed, 3,343 subtests

@sarendis56 — your photo remediation is merged. JeremyJC67/WebHarbor#1 landed as decb0d6;
the branch head is 6c56371. Before merging I confirmed it touches no tasks.jsonl and no
verify/ file, that the app.py change is confined to the image column, and that all 20 image
paths it references resolve against HF #67 (20 referenced, 28 present in the bundle, none
missing). I added one commit on top: .dockerignore now excludes review-reports/, so your two
remediation screenshots do not ship inside the published image.

That the fetch above succeeded through the ordinary path is itself the proof that the archive
layout fix is real — the previous bundle had to be extracted by hand. The audit assertions also
hold in the image: GET /logout 405, POST /favorite/<slug> without a token 400,
POST /alerts with radius=abc 400 rather than a 500, a branded 404, the hero placeholder no
longer naming the location tasks 0 and 2 dictate, and five read-only page loads leaving the DB
byte-identical.

Still blocked only on HF #67 being merged, after which I repin .assets-revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants