Skip to content

feat(review): revise the immediately previous review atomically - #98

Merged
alexschmander merged 10 commits into
mainfrom
feat/previous-review-revision
Sep 7, 2026
Merged

alexschmander merged 10 commits into
mainfrom
feat/previous-review-revision

Conversation

@alexschmander

@alexschmander alexschmander commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Current-head CI: all selected jobs and the required Testing check passed. Successful run. This supersedes earlier awaiting-CI statements below; independent acceptance remains pending.

Behavior or contract

Resolves #12. Reviewers can return to their immediately previous eligible skipped review or revise their previous completed review. Revisions preserve the old outcome and geometry until an explicit atomic decision commit.

Previous uses a shared derived review-history index instead of scanning every image on each reopen. Review claims initialize the index with bounded concurrency; committed event transactions update it by image sequence. Reopen validation and terminal review publication serialize per reviewer/task, and uncertain publication or membership changes invalidate the index.

Previous reopens and loads its target before releasing the displaced assignment. This preserves the server's immediately-previous eligibility rule and keeps current work available when opening fails. Untouched reviews switch directly; changed work requires confirmation. Previous lives in the workspace context toolbar. Previous follows normal review eligibility: a Reviewer may reopen their own review even when they submitted the annotation. Exact review ownership, current role, and freshness checks remain enforced. Known review conflicts have fixed diagnostic codes; arbitrary error text is never logged. Background expiry cleanup does not count as a later skipped review. Runtime failures visibly label the status control Error, while its details retain the full message and annotation save status. While reopening, the current image stays visible with an opening status and disabled conflicting actions; the confirmed transition modal is hidden.

Base: main at 82dbf101a74139feb012b36824586c28a02c59ff. Current head: 8b80a941231421998c15de1b02e7465c889c003f. Parent of #101 and #103.

Production ownership path

Domain policy owns submission-event identity, captured targets, effective decisions, and replay. Storage owns ownership, freshness, leases, locking, atomic commits, and the process-local review-history projection. Ordinary assignment transactions and offline sync observe durable publication before state-cache writes; no persisted index or schema migration is introduced. API/client expose reopening and revision commands. Shared UI stages decisions and replaces the workspace only after the previous assignment loads successfully.

Acceptance criteria and evidence

Criterion Evidence
Exact previous target, ownership, freshness, retry and exclusivity Storage and API revision tests
Preserve outcome, geometry and audit history until commit Domain replay and storage revision tests
Switch while another review is held Production repository regression reopens A while B is active, then releases B and reclaims A
Preserve current work on opening failure UI load-failure regression; no displaced-assignment release
Direct untouched navigation, confirmed changed navigation, toolbar placement Focused UI Previous and cancellation tests

The maintainer-approved follow-up aligns self-review eligibility with existing normal review. This supersedes the original issue wording requiring annotator/reviewer separation for Previous.

Regression protection

previous_review_reopens_before_current_release_and_survives_later_release exercises the production eligibility contract. UI action-order assertions require reopen before release; failure coverage retains the current assignment and correction draft. These cover the interaction missed by the earlier release-first fake-only test. The expiry-cleanup regression reproduces a completed review being incorrectly displaced by maintenance cancellation on another image. A rendered-text UI regression reproduces the failed Previous request retaining an Idle label instead of showing Error.

Verification

Current follow-up: GPT-5.6-Luna Max passed the self-submitted review lifecycle regression (1), existing Previous conflicts (3), revision idempotency (1), conflict mapping/redaction (1), and API diagnostics (3). Final parent files matched the tested checkout; formatting, diff whitespace, and documentation checks passed. Large checks remain assigned to GitHub CI.

GPT-5.6-Luna Max ran focused storage and shared-UI checks. Passed history expiry/tie behavior, cold/warm/restart initialization, observation after state-cache failure, concurrent scan/commit merge, same-key completion/reopen serialization, existing previous-review/expired-later regressions, and delayed UI success/failure with the current texture retained. The test pause mutex is released before waiting, so it does not mask missing commit serialization.

A real warm reopen against a disposable 256-image index succeeded with exactly 2 state loads and 3 event loads in 65 ms. The other 255 records had no image bytes or events. This proves bounded history reads; it is not a live-server latency guarantee.

Commands used focused cargo test --locked -p labello-storage filters and cargo test --locked -p labello-ui --all-features previous_review, plus the failure-preservation filter. Final formatting and diff checks passed. Independent read-only audit found and resolved uncertain image-index publication invalidation; no remaining blocker was identified.

Broad verification runs in GitHub CI after push, as requested. No local canonical matrix, standalone different-key integration race, or fresh live-browser performance measurement was run. Earlier combined canonical and hosted results apply only to their historical heads.

Visual or browser evidence

Focused kittest checks cover Previous within the workspace context bar, confirmation/cancellation, and the actual painted Error label after a rejected reopen request. Fresh native and Chromium matrices were not rerun for this correction.

Documentation

Updated docs/operations.md for bounded conflict diagnostics and docs/assignment.md for normal-review eligibility. Updated docs/assignment.md, docs/ui-ownership.md, docs/architecture.md, and docs/persistence.md for the index, initialization, lock order, recovery, and visible pending navigation.

Risks and omitted checks

Restart, image membership changes, and explicit repair require one index rebuild. Initialization cost is paid before a review claim; warm reopening still reads the target image event history. Historical assignments without captured revision context cannot reopen. Local staged revisions do not survive browser reload. Follow-up independent review is pending; the supplied review requested changes and is not an acceptance decision for this head.

Worktree integrity

  • Unrelated worktree changes were preserved and excluded.
  • No credentials, runtime datasets, generated distributions, or sensitive diagnostics are committed.

Handoff status

  • Implementation evidence is assembled; current-head CI passed.
  • Required Testing succeeded on the current head.
  • Existing author assignments and reviewer requests are preserved.
  • Ready for review after current-head CI and follow-up independent acceptance.

CI follow-up: the prior exact-head run passed all selected jobs except Clippy and its dependent Testing aggregate. Fixed test-module ordering and an unnecessary integer cast in #98; these changes carry through the stack. Fresh hosted CI is required for the updated heads.

GPT-5.6-Luna Max verified the two lint fixes with cargo clippy --locked -p labello-storage --all-targets --all-features -- -D warnings (passed), final formatting/diff checks, exact tested-file parity, clean active worktrees, and main → #98#101#103 ancestry.

@alexschmander alexschmander self-assigned this Sep 5, 2026
@alexschmander
alexschmander marked this pull request as ready for review September 5, 2026 16:54
@alexschmander
alexschmander marked this pull request as draft September 5, 2026 20:09
@alexschmander
alexschmander marked this pull request as ready for review September 5, 2026 20:35
An error occurred while trying to automatically change base from fix/migration-discovered-pairs to main September 6, 2026 17:07
@alexschmander
alexschmander force-pushed the feat/previous-review-revision branch from 59c90c2 to 7942172 Compare September 6, 2026 17:45
@alexschmander
alexschmander changed the base branch from fix/migration-discovered-pairs to main September 6, 2026 17:46
@alexschmander alexschmander reopened this Sep 7, 2026
@alexschmander
alexschmander force-pushed the feat/previous-review-revision branch from 7942172 to 81fa982 Compare September 7, 2026 09:32
@alexschmander
alexschmander merged commit efc05ae into main Sep 7, 2026
10 checks passed
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.

Allow returning to the previous assignment during review

1 participant