feat(interop): v0.7.5 — French locale (fr_FR), BNF SRU/UNIMARC, all interop plugins (OAI-PMH, NCIP, Z39.50, VIAF, BIBFRAME, ResourceSync)#132
Open
fabiodalez-dev wants to merge 99 commits into
Conversation
…bels (#123, #121) - Add iiif_manifest_url VARCHAR(2000) column to archival_units (idempotent migration) - GET /admin/archives/{id}/manifest.json and /archives/{id}/manifest.json return IIIF Presentation API 3.0 JSON with metadata crosswalk, seeAlso links to DC/OAI-PMH, and a Canvas for locally stored cover images; CORS header for external viewers - iiif_manifest_url field in edit form links to external IIIF server manifests - <link rel="alternate"> for manifest.json added to admin show and public detail pages - form.php: wrap fields in ISAD(G) area section headers (Area 1 Identity Statement, Area 3 Content & Structure, Area 4 Conditions of Access) matching AtoM presentation
…tion endpoint
- iiifManifestAction: real canvas width/height via getimagesize() (fallback 1500×2000)
- iiifManifestAction: partOf link to parent Collection or root collection.json
- iiifManifestAction: structures[] with ancestor breadcrumb Range (IIIF TOC)
- iiifBuildAncestorChain(): walk parent_id chain to build Fondo › Serie › … path
- iiifCollectionAction(): GET /archives/collection.json (all top-level fondi)
and GET /archives/{id}/collection.json (direct children as Manifest or Collection)
— child units with children become sub-Collections, leaf units stay Manifests
- Added two public routes for Collection endpoints (CORS header included)
…authority links - behavior: viewer UX hint (paged/individuals/unordered) derived from specific_material - requiredStatement + provider: institution attribution as IIIF Agent (machine + human) - rights: new rights_statement_url column (VARCHAR 500) exposed as manifest['rights'] - structures: replace flat breadcrumb with nested Range hierarchy (§1.1) — each ancestor wraps the next as an outer Range; innermost Range holds Canvas items - authority links: creator/subject names + VIAF/Wikidata URIs from external_refs added to manifest metadata - form: rights_statement_url field in ISAD(G) Area 4 (Conditions of Access) - new private helpers: iiifBuildNestedStructures(), iiifFetchAuthoritiesWithRefs()
…oints
- ARK identifier (ark_identifier VARCHAR 255): surfaced in IIIF seeAlso via
n2t.net resolver, EAD3 <recordid>, Dublin Core dc:identifier, form Area 1
- Version note (version_note VARCHAR 500): shown in IIIF metadata, form
- EAD3 per-unit: GET /archives/{id}/ead.xml (public + admin) — single-unit
finding aid from existing writeEad3Document; adds <dao> IIIF manifest link
and optional cover image to the bulk exporter as well
- METS per-unit: GET /archives/{id}/mets.xml (public + admin) — wraps DC
inline + EAD3 by reference + IIIF manifest link; suitable for Europeana
submission; covers fileSec with thumbnail if cover image present
- Dublin Core: adds ark as second dc:identifier, rights_statement_url as dc:rights
- IIIF seeAlso: adds EAD3, METS, ARK entries alongside existing DC and OAI-PMH
- headLinks: EAD3 and METS link rel="alternate" in admin and public show views
…erop standards Covers all new fields (ark_identifier, version_note, rights_statement_url), IIIF 3.0 advanced features (behavior, nested Ranges, requiredStatement, provider, rights, authority links), per-unit EAD3 and METS exports, headLinks, and ISAD(G) form labels validation.
…new fields in show.php version_note and iiif_manifest_url are record-level metadata (not material-specific) — moved outside the collapsible <details> accordion so they are always visible. Added ark_identifier, rights_statement_url, version_note display to admin show view. Fixed E2E test 8 to open the accordion before selecting specific_material. All 25 interop E2E tests now pass.
… seed for new fields - tests/seeds/librarything-aldebaran.tsv: 18 Aldebaran volumes (4 cycles) to test LibraryThing series import with collana/numero_serie parsing - tests/seeds/books-general.csv: 15 classic books for standard CSV import - tests/seeds/books-seed.sql: idempotent SQL seed (INSERT IGNORE) for fast setup; covers all 33 books, authors, publishers, series, and libri_collane links - tests/seeds/archives-feature-20.sql (v2): adds ark_identifier, rights_statement_url, version_note to temp table and INSERT/ON DUPLICATE KEY - .gitignore: allow *.csv and *.tsv in tests/seeds/
P1 — fresh-install schema integrity:
- Add 4 missing columns to ddlArchivalUnits() DDL (iiif_manifest_url,
rights_statement_url, ark_identifier, version_note) so a fresh CREATE TABLE
includes all interoperability-standards columns without needing ALTER
- Move migrateImageColumns() call to AFTER the CREATE TABLE foreach loop so
the table exists before the additive ALTER migrations attempt to run
P2 — language-code parsing:
- iiifManifestAction(): use preg_split('/[,;\s]+/', ...) instead of
explode(',', ...) so seed values like 'ita;eng' are correctly split
- Dublin Core output: replace single compound writeElementNs with a loop that
emits one dc:language element per code (RFC 4646 / MARC 21 compliant)
…-up) The seed archives-feature-20.sql inserts iiif_manifest_url, rights_statement_url, ark_identifier, and version_note, but the migration that archives-feature-documents.spec.js applies on a clean DB stopped at document_filename. On a fresh install with the migration- only path (no ensureSchema() call), the seed would fail with 'Unknown column'. Add four idempotent ALTER TABLE blocks following the same pattern as all other additive columns in the migration.
cleanupTag (spec) — FK-safe deletion order: delete archival_unit_authority rows first via JOIN, then archival_units children (parent_id IS NOT NULL) then roots; avoids MySQL 1093 from self-referential NOT EXISTS subquery. show.php — validate rights_statement_url scheme before rendering as <a href>: only http/https URLs get a clickable link; anything else (incl. javascript:) renders as plain escaped text, preventing href injection. ArchivesPlugin — IIIF 3.0 §3.3 compliance: manifest items must have cardinality ≥ 1; when no local cover image is present, add a minimal placeholder Canvas so clients receive a valid (if unpainted) manifest. ArchivesPlugin — EAD3 schema fix: move <daoset> block inside <did> (before its closing tag) where EAD3 requires it; the block was previously emitted after </did>, violating the EAD3 DTD. form.php — add rel="noopener noreferrer" to external rightsstatements.org link that uses target="_blank" (prevents tabnabbing/opener leakage).
ScrapeController::byIsbn() now calls session_write_close() before
hitting Discogs/OpenLibrary. PHP file-based sessions hold an exclusive
lock for the entire request duration, causing subsequent page.goto()
calls in Playwright (or any same-session request) to block until the
slow external fetch completes — typically 10–30 s, long enough to
cascade-fail all subsequent serial tests.
Also hardens seed-catalog.spec.js: replaces waitForTimeout(8000) with
waitForLoadState('networkidle'), adds trySave() helper that logs a
warning instead of throwing on save failure (seeder resilience), and
clears isbn13/isbn10 before fallback fill to avoid unique-constraint
violations.
ci-quality.yml (new): - PHP syntax check with xargs -P4 on app/, config/, public/, installer/, server/ - PHPStan level 5 (mirrors pre-commit hook in CI for PRs from forks or CI-only runs) - Composer security audit against Packagist CVE database - Triggers on any *.php / composer.json / phpstan.neon change codeql.yml (new): - CodeQL security-and-quality queries for PHP and JavaScript - Detects SQL injection, XSS, path traversal, CSRF, open redirect - Runs on push/PR to main + weekly schedule test-migrations.yml (updated): - Added trigger on version.json changes - Job verify-versions: checks every migrate_*.sql has version ≤ version.json; catches the silent-skip bug (updater guards with version_compare <=) - Job test-full-chain: applies schema.sql + all migrations in sort -V order, then asserts 5 expected tables and 6 expected columns from 0.5.x/0.5.9.x migrations are present - Existing job test-migrations (0.4.0 upgrade path) preserved unchanged
…ckout@v6 PHPStan installed via setup-php tools: doesn't auto-discover phpstan.neon when run as a global binary in some 2.x versions, causing "At least one path must be specified to analyse". Passing --configuration makes it explicit and version-agnostic. Also upgrades actions/checkout v4→v6 and actions/cache v4→v5 across all three workflows to suppress the Node.js 20 deprecation warning (Node 24 becomes default June 2026).
Adds E2E_DB_HOST and E2E_DB_PORT env vars to mysqlArgs() in archives-seed-50, archives-interop-standards and full-test specs. Prioritises TCP (host+port) over socket when DB_HOST is set, enabling remote DB access via SSH wrapper for production smoke-tests.
Both files were gitignored, so GitHub Actions could not find the PHPStan config file during CI runs (exit 1: "does not exist"). Remove them from .gitignore so the runner picks them up on checkout.
v3 (toolset 2.25.3) does not recognize 'php' as a language, causing the Analyze (php) job to fail. v4 restores PHP support and removes the Node.js 20 deprecation warnings.
The /server/ directory is gitignored and absent in CI, causing PHPStan to abort with "Path does not exist". No analyzed code in app/ or plugins references classes from server/src, so the entry is safe to remove.
PHP 8.2 constants like CURLOPT_REDIR_PROTOCOLS_STR were reported as 'not found' because phpVersion was set to 80100. The CI and production server both run PHP 8.2; aligning the phpstan target version prevents false positives on 8.2-only constants.
With phpVersion bumped to 80200, PHPStan resolves CURLOPT_REDIR_PROTOCOLS_STR and collapses the old array-shape offset errors. Regenerated baseline now captures only the two remaining legitimate suppressions.
PHPStan 2.1.54 (installed in CI via tools:phpstan) reports: - CURLOPT_REDIR_PROTOCOLS_STR not found: constant is PHP 8.2 but absent from PHPStan 2.1.54 stubs; guarded by defined() at runtime - isset.offset for traduttore/illustratore: stricter array-shape narrowing in 2.1.54 vs 2.1.40 (reportUnmatchedIgnoredErrors=false keeps these baseline entries harmless on older local versions)
thepixeldeveloper/sitemap is abandoned (no replacement available) — the audit exits 2 by default. --abandoned=report logs it without failing the pipeline.
CodeQL CLI 2.25.x does not recognize 'php' as a language, causing the Analyze (php) job to fail. PHP security is already covered by PHPStan level 5. Retain only javascript-typescript CodeQL analysis.
- tests/archives-interop-standards.spec.js: add escapeLike() helper and use
ESCAPE '\\' in cleanupTag() LIKE queries to prevent SQL wildcard expansion
on underscores in TAG (critical fix); remove -p${DB_PASS} from argv and
pass password via MYSQL_PWD env var instead; fix test 12 n2t.net check
to use startsWith() instead of includes(); refactor test 25 to use
Playwright locators instead of page.content(); add Number.isInteger()
guard in afterAll cleanup
- tests/archives-seed-50.spec.js: same MYSQL_PWD argv fix
- tests/full-test.spec.js: same MYSQL_PWD argv fix
- .github/workflows/test-migrations.yml: add workflow file to PR paths;
replace || true with targeted idempotency handler that propagates
unexpected errors; add rights_statement_url to column count (6→7)
- installer/database/migrations/migrate_0.5.9.7.sql: add UNIQUE KEY on
archival_units.ark_identifier for upgrade paths
- storage/plugins/archives/ArchivesPlugin.php: add URL/length validation
for iiif_manifest_url, rights_statement_url, ark_identifier, version_note
in validateArchivalUnit(); add app-level ARK uniqueness check using
excludeId to allow in-place updates; add UNIQUE KEY uq_ark_identifier to
ensureSchema() CREATE TABLE; fix O(n) per-child queries in
iiifCollectionAction() by computing has_children via EXISTS() subquery in
the initial SELECT
- storage/plugins/archives/views/form.php: wrap placeholder in $e(); wrap
parent_id help text in __()/$e() for i18n consistency
- app/Controllers/ScrapeController.php: add comment after session_write_close()
warning plugin authors not to write to $_SESSION in Hook callbacks
- app/Support/MaintenanceService.php: add expired_pickups? to runIfNeeded()
return type shape to fix PHPStan nullCoalesce.offset warning properly
- phpstan-baseline.neon: remove expired_pickups suppression (now fixed in
the type annotation)
- tests/seeds/archives-feature-20.sql: version_note TEXT → VARCHAR(500) to
match archival_units column type and prevent silent truncation
- migrate_0.5.9.7.sql: add deduplication pre-check before UNIQUE KEY (NULL-out duplicate ARKs keeping earliest row, guarded by idx_exists) - phpstan-baseline.neon: remove 3 stale suppressions (traduttore, illustratore, CURLOPT_REDIR_PROTOCOLS_STR); keep booleanNot.alwaysTrue which is a real PHPStan finding in QueryCache while-loop - ArchivesPlugin validateArchivalUnit(): remove AND deleted_at IS NULL from ARK uniqueness check to align with DB UNIQUE KEY semantics - ArchivesPlugin iiifCollectionAction(): restrict root collection to level='fonds' only (parent_id IS NULL was too broad) - tests: (body['seeAlso'] || []).find(...) guard against absent seeAlso; 'Content & Structure' selector instead of ambiguous 'Content'
The migration renames the typo column classificazione_dowey→classificazione_dewey. On fresh schema installs the correct column already exists, so CHANGE COLUMN fails with 'Unknown column' — not in the CI tolerated-error list. Wrap with INFORMATION_SCHEMA guard (same pattern as migrate_0.5.9.7.sql).
…0.5.9.7 upgrades migrate_0.5.9.sql has version 0.5.9 which is older than the released 0.5.9.6, so it never runs when upgrading from 0.5.9.x. This caused Test B of the reinstall test to fail with 'archival_units doesn't exist'. The fix merges the full archives DDL (CREATE TABLE IF NOT EXISTS + idempotent column additions + plugin row) into migrate_0.5.9.7.sql so that any upgrade path from before 0.5.9.7 creates all archives tables in a single idempotent pass. Also move the book-type badge above the <h1> on the frontend book-detail page and set an absolute font-size (0.7rem) so it renders small regardless of heading size.
…0.5.9.7 upgrades migrate_0.5.9.sql has version 0.5.9 which is older than released 0.5.9.6, so it never runs when upgrading from 0.5.9.x. Confirmed by reinstall-test.sh Test B: upgrade from v0.5.9.6 failed with 'archival_units doesn't exist'. Merged the full archives DDL into this file: CREATE TABLE IF NOT EXISTS for all 4 tables, idempotent column additions via INFORMATION_SCHEMA guards, plugin row upsert, and the original dedup+UNIQUE KEY — so any pre-0.5.9.7 upgrade path gets all archives tables in a single idempotent pass.
PHPStan baseline: - Restore 3 suppressed entries removed erroneously: CURLOPT_REDIR_PROTOCOLS_STR (PHP 8.1 compatibility), traduttore/illustratore isset.offset in BookRepository migrate_0.5.9.7.sql: - Fix plugin version 1.0.0 → 1.1.0 to match storage/plugins/archives/plugin.json - Use full extended ENUM for specific_material ADD COLUMN (avoids redundant MODIFY) - Emit dedup count as migration_notice before nulling duplicate ARKs ArchivesPlugin.php: - dc.xml headLinks/seeAlso: application/rdf+xml → application/xml (OAI-DC is not RDF/XML; wrong MIME breaks parsers) - validateArchivalUnit: normalize resolver URLs (https://n2t.net/ark:/...) to canonical ark:/... form and reject non-ARK strings with a clear error message - migrateImageColumns: add idempotent guard for uq_ark_identifier UNIQUE KEY so in-place upgrades get the same DB-level uniqueness as fresh installs tests/archives-interop-standards.spec.js: - Test 20 EAD3 bulk export: remove manual Cookie forwarding (page.request already shares the browser context's storage)
…nded enum + fixes)
- Restore `novalidate` on #bookForm (removed in 2c0264e), which caused browser HTML5 validation to intercept submit before the SweetAlert2 handler could fire — breaking book create/edit confirmation dialog - Fix smoke-install.spec.js search parameter: the /api/libri endpoint uses `search_text` not `search[value]` (DataTables format), so the wrong parameter silently returned the first 5 alphabetical books instead of the searched title, making createdBookId wrong and the edit-title verification fail
- fr_FR.json: +65 translated strings for LibraryThing plugin sections (Recensione e Valutazione, Visibilità nel Frontend, Condizione Fisica, etc.) - en_US.json, de_DE.json: add missing "Valore Corrente" translation - book_form.php: wrap LibraryThing field labels with __() so they respect the active locale (previously always rendered in Italian) - full-test.spec.js: Phase 21 (4 serial tests) verifies fr/en/de/it language switching including LibraryThing French strings assertion - Minor fixes: archives plugin.json trailing comma, CI workflow tweaks, phpstan baseline cleanup, test file updates
Upgrades from 0.7.4 → 0.7.5 now execute migrate_0.7.5.sql which inserts the fr_FR language row (INSERT IGNORE, idempotent). 0.7.4 installs that already have the row will see a no-op.
…activate fr_FR INSERT IGNORE left pre-existing fr_FR rows with is_active=0 untouched. ON DUPLICATE KEY UPDATE now forces is_active=1 on upgrade, so the language bootstrap (step 3: WHERE is_default=1 AND is_active=1) can find the French locale after the admin sets it as default.
setDefault() now sets is_active=1 alongside is_default=1, preventing the inconsistent state where a language is marked default but inactive. I18n::bootstrap() step 3 requires is_active=1, so an inactive default language was silently ignored and the app stayed in the previous locale. Also adds missing bibframe.book route key to routes_de_DE.json.
… dev schema detected Pre-release dev builds used source_code/source_id column names. The current migration expects source/authority_id. CREATE TABLE IF NOT EXISTS skipped the DDL on installations that had the dev table, causing ADD KEY idx_authority (source, authority_id) to fail with 'Key column source does not exist'. Add a guard that detects the old schema via source_code column presence and drops the empty dev table before recreating with the correct schema.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const { execFileSync } = require('child_process'); | ||
|
|
||
| const DB_HOST = process.env.E2E_DB_HOST || ''; | ||
| const DB_PORT = process.env.E2E_DB_PORT || ''; |
| return args; | ||
| } | ||
|
|
||
| function dbQuery(sql) { |
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const BASE = process.env.E2E_BASE_URL || 'http://localhost:8081'; |
|
Important Review skippedToo many files! This PR contains 160 files, which is 10 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (160)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…sue #130) On Windows, RecursiveDirectoryIterator::getPathname() returns backslash paths, breaking str_replace prefix-stripping and mkdir target construction in copyDirectoryRecursive() and copyDirectory(). All paths now normalized to forward slashes at function entry; realpath() returns also normalized. Also remove unused SEED_FILES variable from install-compat-standard.spec.js (CodeQL js/unused-local-variable #20) and harden test selectors.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…ition archives-upload-assets.spec.js ensureUploadFixtures(): - Remove existsSync+writeFileSync TOCTOU pattern (race condition fix) - Replace hardcoded /tmp/ paths with path.join(os.tmpdir(), ...) for cross-platform safety and to satisfy CodeQL js/insecure-tmp rule
__() uses Italian as the native fallback — keys ARE Italian text, so if a key is missing from it_IT.json the function returns the key itself. it_IT.json is intentionally sparse (overrides only). Only en_US ↔ de_DE requires exact key-set parity.
CodeQL js/insecure-temporary-file:
- Add binary fixtures to tests/fixtures/ (jpg/pdf/mp3, csv/tsv) and
allow the directory in .gitignore (mirrors tests/seeds/ pattern)
- archives-upload-assets.spec.js reads from tests/fixtures/ instead of
writing predictable filenames to os.tmpdir()
OAI-PMH:
- Catch remaining \Throwable in record serialisation loop to skip
malformed metadata with a warning log instead of crashing
E2E test stability:
- bibframe-persistent-uri: check @id contains /id/work/{id}, not suffix /work
- book-form-comprehensive: simplified Choices.js check (no XPath)
- bulk-enrich: nullify ISBN before DELETE to avoid UK constraint on re-run
- multisource-scraping: accept 'Z39.50/SRU' as valid source for Nevermind
- smoke-install: make author input fill conditional (visible guard)
Owner
Author
Code reviewBranch: Found 29 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (3)
Details and fix proposalsF002 — In
|
| # | Score | Impact | File | Issue |
|---|---|---|---|---|
| F031 | 48 | security | storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php:2497-2524 |
requireAdminForDownload() accepts HTTP Basic Auth credentials in-band over potentially unencrypted HTTP connections, transmitting admin passwords in cleartext if TLS is not enforced at the infrastructure level. |
| F032 | 58 | correctness | app/Controllers/FrontendController.php:830-890 |
FrontendController catalog search builds LIKE patterns by wrapping $wordBase in '%...' without escaping literal % and _ wildcards, unlike AutoriApiController and CollaneController which use str_replace escaping — users searching for underscore or percent get wrong matches on the main book catalog. |
Phase 4 couldn't confirm decisively. Re-run /adamsreview:review if you suspect this deserves
further investigation with fresh context.
Fix runs
Run fixrun_01KRBB2ZT91936NP8ZB0X076FB — 2026-05-11T11:17:54Z
- Outcomes: 9 fixed and verified
- Commits:
6a93828
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F002 | FG-1 | ✓ fixed and verified | |
| F003 | FG-2 | ✓ fixed and verified | |
| F005 | FG-3 | ✓ fixed and verified | |
| F007 | FG-1 | ✓ fixed and verified | |
| F010 | FG-4 | ✓ fixed and verified | |
| F020 | FG-5 | ✓ fixed and verified | |
| F023 | FG-6 | ✓ fixed and verified | |
| F027 | FG-7 | ✓ fixed and verified | |
| F028 | FG-8 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
6 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 6 auto-promoted via batch, 0 promoted with an edited or alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Each |
Fix groups (committed): - [FG-1] F002, F007 — storage/plugins/archives/ArchivesPlugin.php: verified - [FG-2] F003 — storage/plugins/bibframe-linked-data/BibframeLinkedDataPlugin.php: verified - [FG-3] F005 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified - [FG-4] F010 — app/Controllers/FrontendController.php: verified - [FG-5] F020 — storage/plugins/oai-pmh-server/views/book-digital-assets.php: verified - [FG-6] F023 — storage/plugins/archives/views/authorities/show.php: verified - [FG-7] F027 — app/Views/admin/plugins.php: verified - [FG-8] F028 — storage/plugins/archives/views/show.php: verified Post-fix review: 8/8 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
| const TEST_AUDIO = path.join(os.tmpdir(), 'archive-test-audio.wav'); | ||
|
|
||
| function ensureAudioFixture() { | ||
| fs.writeFileSync(TEST_AUDIO, Buffer.from([ |
Owner
Author
Code reviewBranch: Found 47 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (8)
Cross-cutting group G1: F024 + F025 — Both findings are the same soft-delete invariant violation in the same file (NcipServerPlugin.php), targeting two UPDATE libri statements in adjacent transactional flows (createLoanAtomic at line 1212 and closeLoan at line 1323). They must be patched together in a single edit pass to ensure the loan create/close lifecycle remains consistent and both WHERE clauses are updated atomically. Details and fix proposalsF003 — errorResponse() builds child elements with createElement() while all other response methods use createElementNS(NS_SRU,...) — error responses will have children in no namespaceFile: Evidence:
Approach: Add $ns = self::NS_SRU; and replace every createElement() child with createElementNS($ns, ...) in errorResponse(), mirroring the FIX 1 / FIX 1b pattern in sibling methods. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F018 — UNIMARC 461 $t (series title) written to numero_serie (volume number / UNSIGNED INT); series title should go to collana/series fieldFile: Evidence:
Approach: Map UNIMARC 461 $t to collana (series title) and 461 $v to numero_serie (volume number); fallback to 225 $a/$v when 461 absent; truncate to varchar(50) defensively. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F019 — localBookUrl() hardcodes '/libro/{id}' instead of RouteTranslator::route('book'); en_US installs get 404File: Evidence:
Approach: Replace localBookUrl(request, int $bookId) with localBookUrl(request, array $book) and use book_url($book) instead of manual /libro/ concatenation; prepend only $origin (scheme://host[:port]) since book_url() already includes base path. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F020 — fetchChangedBooks() (no-since) SELECT from libri without deleted_at IS NULL — leaks soft-deleted book IDs to harvesters and violates CLAUDE.md ruleFile: Evidence:
Approach: Two options: (a) Strict CLAUDE.md compliance — add WHERE deleted_at IS NULL to no-since branch, accept no tombstones without ?from; (b) Bounded tombstone mode — add WHERE updated_at >= DATE_SUB(NOW(),INTERVAL 30 DAY) OR deleted_at >= DATE_SUB(NOW(),INTERVAL 30 DAY) to bound tombstone window. Recommend (b) for protocol alignment. Since-branch unchanged. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F024 — UPDATE libri in createLoanAtomic() missing AND deleted_at IS NULL — violates CLAUDE.md absolute soft-delete ruleFile: Evidence:
Approach: Add AND deleted_at IS NULL to both UPDATE libri WHERE clauses (F024 at line 1212 and F025 at line 1323) in a single patch. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F025 — UPDATE libri in closeLoan() missing AND deleted_at IS NULL — violates CLAUDE.md absolute soft-delete ruleFile: Evidence:
Approach: Same combined patch as F024 — add AND deleted_at IS NULL to WHERE clause at line 1323. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F027 — ensureSchema() returns void instead of array{created,failed}; onActivate() cannot check $result['failed'] per Plugin Schema RuleFile: Evidence:
Approach: Change ensureSchema() to return array{created:list,failed:list}; wrap ensureSchemaColumns() and ensureAlternatesTable() calls in try/catch Throwable blocks, accumulating created/failed lists. Update onActivate() and onInstall() to check $result['failed'] and throw RuntimeException. Mirror OaiPmhServerPlugin pattern exactly. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified F045 — requireAdmin() checks only tipo_utente === 'admin'; plugin docs state 'admin/staff' can access write endpoints — privilege mismatchFile: Evidence:
Approach: Rename requireAdmin() to requireAdminOrStaff() and broaden role check to in_array(tipo_utente, ['admin','staff'], true). Update 4 call sites. Optionally reuse existing isStaff() helper. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRBHK6KSS54SJQP08FVWQ8C4): fixed and verified ℹ Uncertain (2)
Phase 4 couldn't confirm decisively. Re-run Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F003 | FG-1 | ✓ fixed and verified | |
| F018 | FG-2 | ✓ fixed and verified | |
| F019 | FG-3 | ✓ fixed and verified | |
| F020 | FG-4 | ✓ fixed and verified | |
| F023 | FG-5 | ✓ fixed and verified | |
| F024 | FG-6 | ✓ fixed and verified | |
| F025 | FG-6 | ✓ fixed and verified | |
| F027 | FG-7 | ✓ fixed and verified | |
| F028 | FG-8 | ✓ fixed and verified | |
| F029 | FG-9 | ✓ fixed and verified | |
| F045 | FG-6 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
4 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 4 auto-promoted via batch, 0 promoted with edited/alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Promoted findings are now |
Fix groups (committed): - [FG-1] F003 — storage/plugins/z39-server/classes/SRUServer.php: verified - [FG-2] F018 — storage/plugins/z39-server/classes/SruClient.php: verified - [FG-3] F019 — storage/plugins/openurl-resolver/OpenUrlResolverPlugin.php: verified - [FG-4] F020 — storage/plugins/resource-sync/ResourceSyncPlugin.php: verified - [FG-5] F023 — app/Controllers/ScrapeController.php: verified - [FG-6] F024, F025, F045 — storage/plugins/ncip-server/NcipServerPlugin.php: verified - [FG-7] F027 — storage/plugins/viaf-authority/ViafAuthorityPlugin.php: verified - [FG-8] F028 — storage/plugins/archives/ArchivesPlugin.php: verified - [FG-9] F029 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified Post-fix review: 9/9 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Owner
Author
Walkthrough decisions — 2026-05-11T13:15:03ZReviewer: fabiodalezbackup@gmail.com Pre-existing findings
Generated by |
Owner
Author
Code reviewBranch: Found 34 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (6)
Cross-cutting group G1: F007 + F041 — Both findings are manifestations of the same root cause: inconsistent local-timezone date emission inside OaiPmhServerPlugin.php. Fix together as a single 'normalize all catalog date emissions to UTC' pass: convert all five sites to strtotime()+gmdate() pattern (lines 596-601, 610-615 for F007; lines 1148, 1153, 1634 for F041), while explicitly preserving line 2211 (resumption-token expires_at). The UNIMARC sibling at line 1348 already uses gmdate() — use it as the canonical pattern throughout. Details and fix proposalsF007 — earliestDatestamp in oaiIdentify uses DateTime::createFromFormat with no timezone — parses MySQL datetime in PHP's default TZ, then format('Y-m-d\TH:i:s\Z') appends Z literally; emitted datestamp is local time mislabelled as UTC. recordDatestamp() in the same file correctly uses strtotime+gmdate; this is the outlier.File: Evidence:
Approach: Replace DateTime::createFromFormat (no timezone) + escaped \Z suffix with strtotime()+gmdate() pattern, matching the existing correct recordDatestamp() implementation. Apply to both the libri block (lines 596-601) and the archival_units block (610-615). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRDDAGWPXE3XMZNNRSRW3ATQ): fixed and verified F008 — bookDetail() issues SELECT 1 FROM plugins on every render — N+1 DB round-trip on the hottest page. No caching; runs even for anonymous catalog crawls.File: F012 — writeBookMag calls fetchDigitalAsset per record — the batch path in fetchRecordsPage does NOT pre-fetch digital_assets, so every MAG record on a page triggers an extra prepared-statement query (100+ queries per page at PAGE_SIZE=100).File: Evidence:
Approach: Add a 6th batch-fetch of digital_assets in fetchRecordsPage() alongside the existing 5 batched fetches; stamp _digital_asset on each row; update writeBookMag() to consume _digital_asset via array_key_exists() — mirroring the _authors/_publisher/_genre/_mag_config pattern already in the function. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KRDDAGWPXE3XMZNNRSRW3ATQ): fixed and verified F018 — Path-traversal guard uses prefix-only comparison
|
| # | Score | Impact | File | Finding | Disposition |
|---|---|---|---|---|---|
| F029 | 55 | ux | app/Views/frontend/layout.php:1924 |
The archive icon inside each search-dropdown result item lacks aria-hidden="true", causing screen readers to announce a meaningless icon in addition to the label text. (human-confirmed) | auto-fixable |
| F033 | 50 | ux | app/Views/frontend/book-detail.php:1608-1612 |
The media-type icon inside the newly added badge lacks aria-hidden="true", so screen readers announce the icon class name alongside the display label. (human-confirmed) | auto-fixable |
Promoted findings — details
F029 — The archive icon inside each search-dropdown result item lacks aria-hidden="true", causing screen readers to announce a meaningless icon in addition to the label text.
File: app/Views/frontend/layout.php:1924
Score: 55
Reason: uncertain (Phase 4 inconclusive)
Human-confirmed: @fabiodalezbackup@gmail.com at 2026-05-12T08:25:01Z — Confirmed: fas fa-archive icon in JS search dropdown has no aria-hidden — announced on every keypress. Project convention is aria-hidden=true on all decorative FA icons (25+ instances).
Promoted from disposition=uncertain / actionability=auto_fixable / score_phase4=55
Fix direction: layout.php line 1925 JS string: change '' to ''
F033 — The media-type icon inside the newly added badge lacks aria-hidden="true", so screen readers announce the icon class name alongside the display label.
File: app/Views/frontend/book-detail.php:1608-1612
Score: 50
Reason: uncertain (Phase 4 inconclusive)
Human-confirmed: @fabiodalezbackup@gmail.com at 2026-05-12T08:25:01Z — Confirmed: media-type icon in book-detail badge has no aria-hidden — screen readers announce icon class name alongside label. Inconsistent with book-detail.php lines 1638 and 1263 which already use aria-hidden=true correctly.
Promoted from disposition=uncertain / actionability=auto_fixable / score_phase4=50
Fix direction: book-detail.php line 1610: add aria-hidden="true" to <i class="fas ..." — change to
Fix runs
Run fixrun_01KRDDAGWPXE3XMZNNRSRW3ATQ — 2026-05-12T07:22:42Z
- Outcomes: 5 fixed and verified
- Commits:
332f723
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F007 | FG-1 | ✓ fixed and verified | |
| F012 | FG-1 | ✓ fixed and verified | |
| F031 | FG-2 | ✓ fixed and verified | |
| F034 | FG-3 | ✓ fixed and verified | |
| F041 | FG-1 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Auto-recommendation acceptance
2 auto-rec finding(s) eligible at threshold ≥ 60. Of those, 2 auto-promoted via batch, 0 promoted with an edited or alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Each |
Fix groups (committed): - [FG-1] F007, F041, F012 — storage/plugins/oai-pmh-server/OaiPmhServerPlugin.php: verified - [FG-2] F031 — app/Views/frontend/layout.php, locale/en_US.json, locale/de_DE.json, locale/fr_FR.json, locale/it_IT.json: verified - [FG-3] F034 — app/Views/admin/plugins.php: verified Post-fix review: 3/3 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Owner
Author
Walkthrough decisions — 2026-05-12T08:05:17ZReviewer: fabiodalezbackup@gmail.com
Summary: 5 promoted, 0 skipped. Run 🤖 Generated with Adam's Claude Code Review Command |
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.
Summary
migrate_0.7.5.sqlusesON DUPLICATE KEY UPDATEto activate fr_FR on upgrades (wasINSERT IGNORE);Language::setDefault()now auto-activates the language to preventis_default=1, is_active=0inconsistencyauthor_authority_alternatesif old pre-release dev columns (source_code) are detected — fixes upgrade failure on installations that had dev migrations applied manuallybibframe.bookkey toroutes_de_DE.json(parity with it_IT, en_US, fr_FR)ensureSchema()fix), BIBFRAME/RDF, ResourceSync, VIAF authority controlRoot causes fixed in this batch
fr_FR not activating after upgrade
migrate_0.7.5.sqlusedINSERT IGNORE— if the row existed withis_active=0, the row was silently skipped. Switched toON DUPLICATE KEY UPDATE is_active=1.Site staying Italian after setting French as default
I18n::loadFromDatabase()queriesWHERE is_active=1— anis_active=0language is invisible to the entire locale resolution chain even ifis_default=1. Root fix:Language::setDefault()now forcesis_active=1on the target language."Key column 'source' doesn't exist in table" on upgrade
Some installations had
author_authority_alternatescreated by an early dev migration with columnsource_codeinstead ofsource.CREATE TABLE IF NOT EXISTSskipped the table, then the subsequentADD KEY idx_authority (source, authority_id)failed. Fixed by adding a guard that DROPs the table when the oldsource_codecolumn is detected.Test Plan
author_authority_alternates: verify migration completes without errorphpstan analyse --memory-limit=512M(0 errors)full-test.spec.js(97 tests)🤖 Generated with Claude Code