Repository navigation
Conversation
|
CI Fix: replace Generated by Claude Code |
ja573
left a comment
There was a problem hiding this comment.
REVIEW MET-WP4-WIDGET-01 @ f58d090: CHANGES REQUIRED
Reviewer: fresh Claude Opus 5.5 (max), HIGH leg, independent of the Fable leg. Read-only.
Binding: head, tree 36d5193c, base cab25eba and spec R10 sha256 6fa77f37… match; 5 commits authored and committed by Javier Arias, no trailers; metric_dashboard/tests.rs blob unchanged.
Checks: budget, anonymous boundary and bypasses, cost bounds, dashboard unchanged, correctness against s3 and s4, tests and docs PASS; CI FAIL (lint).
Tests (head-built binaries, local database): metric_widget 58 passed; metric_dashboard 87 passed, 1 ignored; thoth_api lib 2218 passed, 3 ignored (3 Redis failures were a full-disk fault and passed on a fresh Redis); other crates pass. fmt passes. Local clippy not run; rustc 1.97.0 would not flag F1.
Findings: F1 (P1) CI lint fails on deprecated AtomicUsize::fetch_update (metric_widget/mod.rs:833); use a compare_exchange_weak loop, no allow. F2 (P2) the equivalence oracle never has an unserved cross pair; add a diagonal section. F3 (P3) the engine deadline is tested only at the first engine statement; add an ACCOUNTS_SQL case. F4 (P3) tests.rs:518 comment says "No database". F5 (P3) the report's clippy and CI wording. F6 (P3 note) the #1008 CHANGELOG wording, at release prep.
Implementer's four spec findings: agreed, none blocking.
Generated by Claude Code
ja573
left a comment
There was a problem hiding this comment.
REVIEW MET-WP4-WIDGET-01 @ f58d090: CHANGES REQUIRED
Reviewer: fresh Claude Fable 5.2, HIGH leg, independent of the Opus leg. Read-only.
Binding: head, tree 36d5193c, base cab25eba, clean worktree, spec R10 sha256 6fa77f37… and grant 6777125645 match.
Checks by reading: budget, identity, anonymous boundary, cost bounds, dashboard unchanged, contract and docs in order. cargo fmt --all -- --check passes. No local clippy or tests ran (environment), so this leg has no test counts.
Findings: F-01 (P1) CI lint fails on deprecated AtomicUsize::fetch_update (metric_widget/mod.rs:833). F-02 (P2) the 500-children bound counts children after deduplication, so a duplicated has-child row could hide a truncated set. F-03 (P2) the equivalence oracle never tests a grid smaller than the cross product (tests.rs:2166-2283). F-04 (P3) no FrontierMismatch test. F-05 (P3) contracts.md:82 not re-wrapped. F-06 (P3) orchestrator-side report commands lack --workspace. F-07 (P3) in-repo report says "CI status: PENDING".
Implementer's four spec findings: accepted (the fourth as a note).
Generated by Claude Code
ja573
left a comment
There was a problem hiding this comment.
REVIEW MET-WP4-WIDGET-01 @ b2cb073: APPROVED
Reviewer: fresh Claude Fable 5.2, HIGH leg, delta review of the fix pass from f58d0905, independent of the Opus leg. Read-only.
Binding: PR head, worktree HEAD, tree 6100aa08, base cab25eba and spec R10 sha256 6fa77f37… match; the delta touches 4 budget files; new commits authored and committed by Javier Arias, no trailers.
Local (rustc 1.97.0): cargo fmt --all -- --check and cargo clippy --all --all-targets --all-features -- -D warnings exit 0; cargo test --workspace exit 0 (thoth_api lib 2222 passed, 3 ignored); widget model 53, widget GraphQL 9, dashboard model 67 (1 ignored), dashboard GraphQL 20. CI at this head: all ten checks passed.
Prior findings: all resolved or correctly ruled. Fable F-02's premise was false (work_relation_relator_related_uniq); the explicit repeated-row refusal cannot hit valid data and adds no message, code or log field. The diagonal oracle, the deadline-between-statements test and the frontier test cannot pass for the wrong reason. Opus F6 is left for release preparation.
New (all P3, no change before merge): N-01 the deadline test has two timing windows, so a slow runner could fail falsely, never pass falsely (7 of 7 here). N-02 the frontier test copies the prelude and label binding from transact. N-03 PR body footer is not a commit trailer.
Scope note: this reviews the head on cab25eba. B2 has since landed (HOLD 6781693857); the refresh gets its own review.
Generated by Claude Code
ja573
left a comment
There was a problem hiding this comment.
REVIEW MET-WP4-WIDGET-01 @ b2cb073: CHANGES REQUIRED
Reviewer: fresh Claude Opus 5.5 (max), HIGH leg, delta review of the fix pass from f58d0905, independent of the Fable leg. Read-only.
Fix pass: correct. All 13 prior findings are resolved, confirmed or deferred as ruled. The orchestrator's F-02 ruling holds (work_relation_relator_related_uniq is unique and not deferrable). The new tests cannot pass for the wrong reason. CI lint now passes on the runner that failed. fmt and clippy pass; widget model 53, widget GraphQL 9, dashboard model 67 (1 ignored), dashboard GraphQL 20, registry 12.
D2 (P1, the HOLD in 6781693857): B2 (#1010) merged into develop as af6719f6 during this review, so b2cb0730 no longer merges. There are 21 conflict blocks in metric_dashboard/mod.rs and 1 in the contract register, where both PRs add a section 4.3. B2 rebuilt the read path pair mode is built on. It added a 3-read process limit, a 20 s deadline and MetricReadError::Busy, and made private again engine constants the widget imports. Fix: refresh onto af6719f6 with the same authorization cab25eba had. Rebuild pair mode on the new read path. Settle and record the widget's relation to B2's limit, deadline and Busy against spec s4 and s10, or amend the spec. Renumber the register section, then run the full tests, CI and an exact-head review of the resolution.
D1 (P3): in the deadline test (metric_widget/tests.rs:1511-1523), move capturing_pool() above hold_lock, and optionally sleep 2 s instead of 1.5 s.
Generated by Claude Code
Implements MET-WP4-WIDGET-01 (#1007): a public, read-only
metricWidgetGraphQL query for the embeddable metrics widget. It ships inactive behindTHOTH_METRIC_WIDGET(defaultOFF). Authority: ADR-0014 (#1008, merged ascab25eba). Spec: the #1007 body, R10 (APPROVED). Grant: #1007 comment 6777125645.What it does
metricWidget(input: { doi, chapterDois, startDate, endDate })reads one work and its child works, for publishers whose package grantsMETRICS_WIDGET. It serves only registry-public, enabled and additive measures, monthly, with up to 12 months per read and no credential.metric_dashboard/mod.rs, Rust parameters only). The dashboard path, its SQL and its tests are unchanged.METRIC_WIDGET_UNAVAILABLE. Widget errors use fixed widget-owned messages, never an engine message, SQL or package value.METRICS_WIDGETdescription (ADR-0014 s4.2), and two clauses indocs/metrics/README.mdandcontracts.md(granted budget addition). One CHANGELOG line.Base and commits
develop@cab25eba. Headb2cb0730, ten commits (five implementation commits plus five review-fix commits), all authored and committed by Javier Arias.metric_dashboard/tests.rsis unmodified. Three existing assertions change, as the spec names.Review fixes (exact-head reviews at
f58d0905, both CHANGES REQUIRED)lintfailed atf58d0905: the runner's newer stable Rust deprecatesAtomicUsize::fetch_update, which local 1.97.0 does not flag. The slot limiter now uses acompare_exchange_weakloop.INTERNAL_ERROR(impossible underwork_relation_relator_related_uniq, now explicit).contracts.mdwrap and the implementation report corrected.Local validation at
b2cb0730(implementation report in the PR)cargo fmt --all -- --checkpasses.cargo clippy --all --all-targets --all-features -- -D warningspasses on rustc 1.97.0 and on 1.99.1.cargo test --workspacepasses against a local test database (thoth_apilib 2222 passed, 3 ignored; 53 model and 9 GraphQL widget tests).Not in scope
Gates
Generated by Claude Code