Review: WebHarbor reset and smoke verification script (#47, original by @Lxr-max) - #108
Review: WebHarbor reset and smoke verification script (#47, original by @Lxr-max)#108jackjin1997 wants to merge 5 commits into
Conversation
Review of aiming-lab#47. The reset and homepage checks were correct; the DB parity check and the registry failure paths were not. DB parity hashed <repo>/sites/<site>/{instance,instance_seed}, but the control plane resets /opt/WebSyn/<site>/instance inside the deployment and the Dockerfile lays sites out with `COPY sites/ /opt/WebSyn/`. Under the workflow the README documents those are never the same files, so the verdict was independent of the environment it claimed to check: with the container stopped it still reported "runtime DB matches seed DB", and dirtying only the local checkout produced a failing "differs from seed DB after reset" against a container verified clean. - add --docker-container to hash under /opt/WebSyn/<site> inside the running deployment, and --db-root for a host deployment - report md5_source on every site result and in --json; a PASS now always names the DBs it read - with no source configured, SKIP instead of silently comparing the checkout, and drop the warning that made --strict fail a correct docker environment - do not report local parity when no reset succeeded, and reserve the "after reset" wording for a source that is the reset target - raise RegistryError for drift, missing and unparseable registries so they are structured findings instead of tracebacks, and name BASE_PORT mismatches specifically rather than as list drift - record a failed --reset-all once instead of once per registered site Tests: 11 added for the DB source contract, registry failures and --reset-all counting; the pre-existing md5 tests now run against a live control plane because a parity verdict requires a reset to be "after". 18 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to the previous commit, from an independent review of the frozen runs. The SKIP-with-source-none path was only reached when the checkout had no sites/<site>/instance directory. Any checkout that happened to carry one -- anyone who has run a site on the host, or extracted assets and booted locally -- silently fell back to comparing the checkout and issued a parity verdict labelled local:, contradicting the documented behaviour that a flagless run reports SKIP rather than comparing this checkout's files. Two concrete consequences, both reproduced against a live control plane: - a stale local DB failed a healthy deployment: reset=PASS, home=PASS, the deployment's own instance and instance_seed byte-identical, yet exit 1 on "local runtime DB differs from local seed DB" - a flagless --json run reported md5_status=PASS for every site without ever reading /opt/WebSyn -- the same false confidence the previous commit set out to remove, just narrowed to checkouts that have an instance/ directory A flagless run now always reports SKIP with source none. Under --db-root, a root that does not hold the site's DBs is an error (the requested check cannot run), while an undecidable DB pair stays a warning. --reset-all failures now carry the HTTP detail instead of discarding it. Tests: 2 added for the flagless contract; the tests that exercise the local source now pass --db-root explicitly. 20 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Update — an independent review of the frozen runs found a real defect in my own first pass, fixed in The
A flagless run now always reports Two regression tests were written first and confirmed failing before the fix; the tests that exercise the local source now pass Still draft: the re-frozen runs are going back for an independent second pass before this is marked ready. |
The second independent review of the frozen runs flagged that the shipped contract contradicted itself: the README says a flagless run skips the DB check, while --db-root's help still claimed it "defaults to this checkout's sites/ directory". That default was removed in the previous commit, so anyone reading --help would believe the check runs against the checkout when it does not. Also drops the now-dead root parameter from check_site(), unused since the DB source stopped being derived from the repository root. No behaviour change; 20 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewer-owned continuation of #47 by @Lxr-max. The original commit
777fcdfis preservedas the first commit on this branch; the rest are the reviewer's.
What the original change got right
scripts/check_reset_smoke.pydiscovers the site registry fromwebsyn_start.shandcontrol_server.py, drivesPOST /reset/<site>,POST /reset-allandGET /health,smoke-checks each homepage, and degrades cleanly when the control plane is absent.
Ambiguous DB pairs and unknown
--siteslugs were already structured results rather thancrashes. All of that is kept.
What this change fixes
The DB parity check read a filesystem the reset never touches. It hashed
<repo>/sites/<site>/{instance,instance_seed}, butcontrol_server.reset_db()operates on/opt/WebSyn/<site>/instanceand the Dockerfile lays sites out withCOPY sites/ /opt/WebSyn/.Under the workflow the README documents — script on the host, environment in
docker run—those are never the same files. Reproduced against a live control plane with two real sites:
md5=PASS"runtime DB matches seed DB"md5=SKIP"no successful reset to verify"md5=FAIL"differs from seed DB after reset", exit 1md5=SKIP [none], exit 0;--docker-containerreportsPASSMD5 0/0/24— the advertised check never ranSKIPnaming where the real DBs live and how to point at theminstance/is gitignored and created inside the container, so on a fresh checkout the checkwas skipped for every site; four sites (
compass,walmart_careers,osu,rotten_tomatoes) regenerateinstance_seedduring the image build, so even a fullyasset-fetched checkout holds a different seed than the deployment.
--docker-container NAMEhashes under/opt/WebSyn/<site>inside the running deployment;--db-root PATHcovers a host deployment. There is no default — without one of them thecheck reports
SKIPwith sourcenonerather than quietly comparing the checkout.md5_source(human output and--json), so aPASSalwaysnames the DBs it read.
reserved for a source that is the reset target.
Registry faults escaped as tracebacks. Registry drift, a missing
websyn_start.sh, anunparseable
SITESand aBASE_PORTmismatch all reached the user as unhandled exceptions —and registry drift is exactly what a reset/smoke checker should report cleanly. They are now
RegistryErrorrendered as structured findings, withBASE_PORTmismatches namedspecifically instead of being reported as list drift.
--strictfailed a correct environment. Absent localinstance/emitted a warning and--strictpromoted it to failure, so the README's own--strictexample failed on thedocumented Docker workflow. That warning is gone;
--strictstill escalates genuine warningssuch as an undecidable DB pair.
A failed
--reset-allwas counted once per registered site on top of the endpoint error,and discarded the HTTP detail. Now recorded once, with the detail.
Verification
python3 -m pytest scripts/ -q→ 20 passed, from a cold clone of this branch into anempty directory. 13 tests added; the pre-existing md5 tests now run against a live control
plane, because a parity verdict needs a reset for "after" to mean anything.
control_server.pyandsite_runner.pywith two real sites (merriam_webster,cambridge_dictionary, assets65c479f8) copied into/opt/WebSyn/<site>, matching theDockerfile layout. Hashes reported by
--docker-containerwere cross-checked againstdocker exec md5suminside the container and matched exactly.failures that resolved to one real defect in my own first fix — a flagless run still
compared the checkout whenever it happened to have an
instance/directory, which faileda healthy deployment on a stale local DB and produced a green
--jsonparity resultwithout reading
/opt/WebSyn. Fixed infda2266with regression tests written first. Thesecond pass over the re-frozen runs returned 20/20, and additionally caught that
--db-root's help text still advertised the removed default — fixed inf3a799d.main(FedEx, Review: Add FedEx WebHarbor mirror (original by @Lxr-max) #82), which changed both registries thisscript parses: 25 sites discovered,
fedexat40024, no traceback.Not covered
Site UI fidelity, upstream source fidelity, agent task runs and Hugging Face assets are not
applicable to this change; it touches no site, seed, asset or Dockerfile. The scenario
environment ran two real sites rather than all 25 — the 25-site path is covered by registry
discovery against the real registries plus the unit tests, not by live resets of every site.
--docker-containershells out to thedockerCLI; podman and remote daemons are untested.A
--db-rootverdict is a statement about that root only: the utility reports the source itread rather than trying to prove that root is what the control plane reset.