Cline
a829d6e8c6
Task 1: Establish catalog identity and atomic file ownership - CatalogService, CatalogServiceInterface, ModelMetadata DTO - PdoCatalogRepository with atomic operations, savepoint support - FOR UPDATE file locks in ascending order, FOR NO KEY UPDATE for metadata - FK-compatible lock composition documented (spec §6) Task 2: M3 import & indexing engine delivery layer - ImportService, ImportWorker with durable collision handling - SpaceChecker, StreamingHasher, VerifiedCopier, StlValidator - FileEnumerator, LocalFilesystem, PathSafety - Console command wiring via ContainerWiringTest Task 3: Importer integration and repair - LegacyGuessedImportGroupingAdapter isolates first-directory guessing - ImportService uses adapter; ImportWorker uses CatalogServiceInterface - Null owners remain null; DomainException mapped to CATALOG_CONFLICT - SQL failure after registration/path update with caller sentinel rollback Task 4: PostgreSQL contention evidence and bounded acceptance - 6 new PostgresCatalogContractsTest cases with independent sessions - Generated schemas, 250ms lock timeouts, 5s statement timeouts - Ascending file locks, FOR SHARE contention, stale-owner rejection - Rollback/retry, nested transaction survival, root creation contention - Negative controls: replay FOR UPDATE removal (11 failures), metadata FOR UPDATE (4 FK KEY SHARE timeouts) - Fresh Docker PostgreSQL 17 / PHP 8.4.26: 473 tests, 2718 assertions, 10 skips, exit 0 All local: 474 tests, 2347 assertions, 54 skips, foundation/lint clean.
116 lines
15 KiB
Markdown
116 lines
15 KiB
Markdown
# Catalog Shared Contracts Implementation Plan
|
||
|
||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||
|
||
**Goal:** Provide one catalog contract for import registration, stale-protected model metadata and file lifecycle state without filesystem changes.
|
||
|
||
**Architecture:** Extend the existing PDO catalog repository and service; expose a service interface and preserve one shared PDO with import jobs. Isolate guessed grouping behind a stateless legacy adapter. Use row locks and existing nested savepoints, not a new transaction manager or revision schema.
|
||
|
||
**Tech Stack:** PHP >=8.3, PDO, PHP-DI, existing Slim/Phinx infrastructure, PHPUnit, PHPCS; SQLite and PostgreSQL.
|
||
|
||
**Spec:** `docs/superpowers/specs/2026-10-13-catalog-shared-contracts-design.md` (full design approved in conversation).
|
||
|
||
## Global Constraints
|
||
|
||
- No new dependencies; use existing PHP/PDO/Phinx/Slim and test infrastructure.
|
||
- Catalog operations never fetch URLs or inspect/copy/move/delete bytes.
|
||
- Preserve nullable owners and independent file/model identities; never infer new ownership.
|
||
- No revision column or schema migration; compare current values, accepting A-to-B-to-A.
|
||
- No routes/forms/CSRF, tags/categories/media management, scanner, deletion, moves, confirmed manifests or publication recovery redesign.
|
||
- SQL/lock errors propagate after operation savepoint rollback; no retries or caller-owned commit/rollback.
|
||
- Work from `6b9fb54` or its unchanged successor in an isolated worktree created at execution time. Preserve the existing catalog-core worktree and original documentation.
|
||
- No commits/pushes or issue closures without authorization. Keep task evidence in an ignored execution ledger; request integration after final review.
|
||
|
||
## Review Focus
|
||
|
||
- Unchanged invalid legacy metadata survives unrelated edits; raw expected tuples are never normalized (Task 2).
|
||
- No-op operations lock and reject stale expected values, including registration versus reassignment (Tasks 1, 2, 4).
|
||
- Model metadata locks must be `FOR NO KEY UPDATE`, compatible with implicit FK key-share locks (Tasks 2, 4).
|
||
- A null imported owner creates an unassigned file, never owner 0; missing non-null owner fails (Tasks 1, 3).
|
||
- Actual worker factory invocation, not mere container resolution, proves wiring and catches undefined `$c` (Task 3).
|
||
|
||
## File map
|
||
|
||
- Extend `src/Repository/CatalogRepositoryInterface.php`, `PdoCatalogRepository.php`: lookups, hardened registration, metadata/status compare-and-set.
|
||
- Extend `src/Catalog/CatalogService.php`; add `CatalogServiceInterface.php`: shared public boundary and transaction composition.
|
||
- Add `src/Catalog/ModelMetadata.php`: focused shared title and changed-field metadata validation; no filesystem or PDO responsibilities.
|
||
- Add `src/Indexer/LegacyGuessedImportGroupingAdapter.php`; modify `ImportService.php`, `ImportWorker.php`, `config/container.php`: actual consumers and compatibility isolation.
|
||
- Add `tests/Integration/CatalogContractsTest.php`, `PostgresCatalogContractsTest.php`, `tests/Unit/Catalog/ModelMetadataTest.php`; extend existing repository, import pipeline and console wiring tests as needed.
|
||
- Update `docs/project-status.md` and approved spec with observed evidence and remaining boundaries only.
|
||
|
||
## Task 1: Shared service boundary and safe registration
|
||
|
||
**Files:** repository/interface, service/interface, `CatalogContractsTest.php`; existing `CatalogCoreTest.php` remains regression coverage.
|
||
|
||
**Interfaces:** Preserve existing service methods. Add `atomic(callable $operation): mixed`, `file(int $fileId): ?array`, `fileByManagedPath(string $managedPath): ?array`, `registerFile(?int $modelId, string $managedPath, string $hash, int $size, ?string $mime = null, string $status = 'Present'): int`. Repository gains matching lookup methods; registration delegates to existing `recordFile`, not duplicate SQL. Interface initially contains implemented methods; Task 2 extends it.
|
||
|
||
- [ ] Write lookup and registration tests: positive IDs/canonical paths, absence returns null without writes, assigned/unassigned rows, normalized integer/null values including nullable hash/size; same-path replay preserves every column despite different initial MIME/status; owner/hash/size mismatch and null stored identity values reject; same hash/different path gives separate ID.
|
||
- [ ] Add validation cases for nonempty hash up to 64 characters, MIME up to 128, nonnegative size, valid UTF-8/no NUL, existing non-null owner and unchanged managed-path rules. Add caller rollback and savepoint failure preservation tests.
|
||
- [ ] Run targeted tests RED and record actual failures before implementation.
|
||
- [ ] Implement public interface and service forwarding; repository invariants apply to direct callers. Root lock precedes path lookup; existing paths get PostgreSQL `FOR UPDATE` before comparison, with no folder creation afterward. New paths derive folders from physical managed path and create them before insert. Missing supplied owner throws a distinct tested DomainException, not fallback or silent adoption.
|
||
- [ ] Document composition restrictions from spec section 6 in service/interface PHPDoc: no arbitrary registration loops, no file-first then root/metadata, no job-lock-first catalog entry.
|
||
- [ ] Run targeted tests GREEN, full existing regression suite and lint. Obtain fresh task review before Task 2; record results without committing.
|
||
|
||
## Task 2: Metadata and status compare-and-set
|
||
|
||
**Files:** same catalog/repository files; new `ModelMetadata.php`, `ModelMetadataTest.php`; extend `CatalogContractsTest.php`, `RepositoryTest.php`.
|
||
|
||
**Interfaces:** Add to service and repository `modelMetadata(int $modelId): ?array`, `updateModelMetadata(int $modelId, array $expected, array $replacement): array`, `changeFileStatus(int $fileId, string $expectedStatus, string $targetStatus): array`. Replace repository `setFileStatus`; update every caller. Metadata shape is exactly `{title: string, notes: ?string, sourceURL: ?string}`. Status returns normalized full file row. `ModelMetadata` exposes static `validateTitle(string $title): void`, `validateShape(array $metadata): void`, `replacement(array $expected, array $replacement): array`; repository invokes content normalization only after locked expected-state comparison.
|
||
|
||
- [ ] Write RED tests for exact keys/types, unknown IDs, whole-tuple stale rejection, same-current-target with stale expectation, no-op timestamp preservation, ABA acceptance, independent-field conflicts, caller rollback and forced later-write failure.
|
||
- [ ] Write validator tests: Unicode title 1–255 characters, nonblank, no NUL, whitespace preserved; null/empty notes and URL clear to null, other notes whitespace preserved. Source URL <=2048 ASCII characters, absolute HTTP(S), nonempty host, valid port; reject userinfo, controls/whitespace, backslash, malformed percent escapes and unencoded Unicode; accept query/fragment, IP literals and punycode. Never fetch.
|
||
- [ ] Test unchanged legacy empty/invalid URL and other unchanged metadata preservation. Compare expected raw tuple before validating changed content; changed fields alone are normalized. Share title validation with creation/legacy creation, including NUL rejection.
|
||
- [ ] Implement metadata locking with PostgreSQL `FOR NO KEY UPDATE`, then strict expected comparison and normalized changed-field update of only name/notes/source_url plus updated_at. Return tuple; omit UPDATE on effective no-op. Preserve added_at, folder, IDs and files.
|
||
- [ ] Implement status with file `FOR UPDATE`, exact existing status vocabulary for both values, stale/missing errors and no-op lock path. Change only status/updated_at, never ownership/hash/size/path. Test every status pair and preserved unrelated values.
|
||
- [ ] Run targeted RED/GREEN cycles, full regressions/lint and fresh task review. Do not add revisions or state-machine policy.
|
||
|
||
## Task 3: Real importer integration and DI
|
||
|
||
**Files:** `src/Indexer/LegacyGuessedImportGroupingAdapter.php`, `ImportService.php`, `ImportWorker.php`, `config/container.php`, `tests/Integration/ImportPipelineTest.php`, `tests/Unit/Console/ContainerWiringTest.php`, `ConsoleServicesWiringTest.php`.
|
||
|
||
**Interfaces:** Stateless adapter constructed with `CatalogRepositoryInterface`; `modelFor(string $sourcePath, ScannedFile $file): int` alone performs legacy grouping and `ensureModel`. ImportService constructor replaces its catalog dependency with adapter; worker replaces repository with `CatalogServiceInterface`. DI aliases interface to the existing concrete service instance and supplies one PDO to both repositories.
|
||
|
||
- [ ] Add RED tests invoking the resolved worker factory, service/interface identity, real adapter behavior across repeated imports and rollback, ambiguous same-title rejection without partial manifest writes, and unchanged legacy directory grouping. Correct misleading claims of explicit grouping.
|
||
- [ ] Add tests for pending null-owner publication registering an unassigned row, and missing non-null owner/conflicting registration failing without stealing identity. Scan-error terminal items remain unregistered.
|
||
- [ ] Extract grouping into the stateless adapter without caching model IDs across calls. Update all constructor sites/mocks; repository legacy creation remains outside the general service interface.
|
||
- [ ] Wire worker through service. Inside existing job transaction, register first, update managed path second, finish item last. Preserve null without casting to 0. Map DomainException only at registration boundary to `ImportException('CATALOG_CONFLICT', ...)`; let other SQL/error behavior remain intact.
|
||
- [ ] Fix factory instance method `$c` references to `$this->c`. Verify real invocation constructs a worker with shared dependencies and stop callback.
|
||
- [ ] Inject failure after registration during completion; prove catalog/item/progress rollback, caller transaction preservation where applicable and existing per-item failure recording. Published and source bytes remain; no compensation deletion or claim of filesystem atomicity.
|
||
- [ ] Run importer, console and catalog tests, full suite/lint and fresh task review.
|
||
|
||
## Task 4: PostgreSQL contention evidence and bounded acceptance
|
||
|
||
**Files:** new `tests/Integration/PostgresCatalogContractsTest.php`; reuse `RepositoryDatabase.php` and patterns in `PostgresCatalogTest.php`/`CatalogLockStatement.php`; update documentation only after evidence exists.
|
||
|
||
- [ ] Use production migrations and separate connections in disposable test schemas. Add deterministic synchronization with bounded lock timeouts, not timing-only sleeps. Prove two stale metadata writers serialize with one conflict; prove status writers serialize with one conflict, including equal-target stale state.
|
||
- [ ] Hold an ownership mutation on a file and attempt registration replay for its old owner: registration must wait for file lock, then observe conflict after commit. Test detached/null replay and no-op paths. Prove new folders derive from physical path rather than model home.
|
||
- [ ] Reproduce the metadata/model-to-file versus ownership/file-to-model FK interaction: metadata `FOR NO KEY UPDATE` must coexist with the ownership FK key-share lock and complete without a lock cycle. Record SQL lock ordering root/model/file/item/job where relevant; do not claim arbitrary compositions are safe.
|
||
- [ ] Prove lock timeout/SQL exceptions escape unchanged, operation savepoint rollback preserves earlier caller writes, and outer success is not committed. Verify ascending bulk ownership locks still hold and status/metadata do not acquire root unnecessarily.
|
||
- [ ] Run negative-control mutations in the isolated checkout: remove existing-file replay lock and strengthen metadata lock to `FOR UPDATE` separately. The corresponding concurrency test must fail deterministically; restore changes and rerun GREEN. If unavailable, report this evidence gap rather than implying mutation-red coverage.
|
||
- [ ] Run full local tests, foundation, PHPCS and diff checks, then full isolated PostgreSQL suite. Never target development credentials/data or dirty `/tmp/MeshVault`; use a fresh remote directory and disposable schema. Record exact source hashes, commands, output, counts and skips. Never record credentials.
|
||
- [ ] Obtain a fresh whole-branch review of spec compliance, lock/FK behavior, failure handling and actual consumer wiring. Fix findings and repeat affected tests plus final full verification.
|
||
- [ ] Reconcile #28 and related issue acceptance with observed evidence in `docs/project-status.md`; inspect current issue text if access exists, otherwise explicitly leave live reconciliation pending. Do not close #28 or claim M2/M3, explicit grouping, UI, scanner or recovery complete. Record remaining dependencies and ask for integration authorization; no push.
|
||
|
||
## Commands and evidence
|
||
|
||
Resolve all paths below against the isolated checkout root (use absolute paths in tool calls). Local executable: `C:\Users\ksolo\.config\herd\bin\php83\php.exe`.
|
||
|
||
```powershell
|
||
$php = 'C:\Users\ksolo\.config\herd\bin\php83\php.exe'
|
||
& $php vendor/phpunit/phpunit/phpunit --filter 'CatalogContractsTest|ModelMetadataTest'
|
||
& $php vendor/phpunit/phpunit/phpunit --filter 'ImportPipelineTest|ContainerWiringTest|ConsoleServicesWiringTest'
|
||
& $php vendor/phpunit/phpunit/phpunit --filter PostgresCatalogContractsTest
|
||
& $php tests/foundation.php
|
||
& $php vendor/phpunit/phpunit/phpunit
|
||
& $php vendor/squizlabs/php_codesniffer/bin/phpcs --standard=phpcs.xml.dist
|
||
git diff --check
|
||
```
|
||
|
||
Run commands from the isolated checkout; prefix script/config paths with that root when invoking tools. Targeted RED means the intended assertion fails, not a bootstrap/dependency failure. GREEN requires exit 0, no unexpected warnings/risky tests and explained skips. PostgreSQL runs require `MESHVAULT_TEST_PG_DSN`, `MESHVAULT_TEST_PG_USER`, `MESHVAULT_TEST_PG_PASSWORD` with isolated-test credentials; confirm actual helper variable names before provisioning. SQLite skips do not prove PostgreSQL locking. Check untracked new files explicitly because `git diff --check` does not cover them.
|
||
|
||
## Self-review / handoff
|
||
|
||
Tasks 1–4 cover spec sections 3–8; scope and follow-on acceptance remain as specified in sections 1–2. Repository direct-call invariants, raw metadata/no-op semantics, lock order, importer factory invocation, transaction rollback and physical-byte preservation each have an owning test step. No product changes or new test execution are claimed by this plan.
|
||
|
||
Recommended execution: subagent-driven, with fresh implementation and review contexts for each task and final whole-branch review. Native execution is also possible, retaining the task gates and a fresh final reviewer. Before implementation, confirm plan and execution approach, create the isolated worktree via the required worktree workflow, and copy the approved spec/plan into it without disturbing untracked originals.
|