Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

## Current status

PLAN4 end-to-end planning refresh is complete and internally reviewed from
current main `3479ee71`. No REV runtime module, table, route, action activation,
or product behavior exists.
`WS-REV-001-03A1` implements the hidden queue/admission persistence foundation.
PR #262 is reconciled with trusted main `2feaf47d`; ART retains migration 0050
and REV owns its exact 0051 successor. The chunk adds no REV route, action
activation, checker hook, lease, Review, revision behavior, or contribution
behavior.

AUTH `WS-XINT-003-02A` through `02D` are merged. REV now has stable policy
lineage/mutation, complete unavailable action/principal registration, and typed
Expand Down Expand Up @@ -40,6 +42,7 @@ REV does not own Project/Task/Submission/Checker/AUTH/ART/CON internals.

## Next step

Await human approval. Then refresh and implement only `WS-REV-001-03A1`
queue/admission-idempotency persistence from then-current main. Stop before
03A2.
Publish and review only the `WS-REV-001-03A1` PR. GitHub Actions must provide
the full-suite and repository-coverage proof. After human merge approval, stop;
`WS-REV-001-03A2` remains a separate explicit start and still depends on its
named CON policy-version FK target.
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,25 @@ No expedited SLA.

## Allowed files

Freeze exact migration name from then-current main; expected scope:
Current main now has the single ART-owned head `0050_guide_source_v2`. After
rebasing PR #262, this chunk owns its exact successor
`0051_review_queue_foundation.py` and the following scope:

```text
backend/app/modules/reviews/__init__.py
backend/app/modules/reviews/models.py
backend/app/modules/reviews/repository.py
backend/app/modules/reviews/schemas.py
backend/alembic/versions/<next>_review_queue_foundation.py
backend/app/db/models.py (metadata registration only)
backend/alembic/versions/0051_review_queue_foundation.py
backend/tests/test_alembic.py
backend/tests/test_review_queue_persistence.py
backend/tests/conftest.py (schema fingerprint/fixture registration only)
backend/scripts/run_test_lanes.py (canonical lane registration only)
backend/tests/test_ci_test_lanes.py (lane registration assertion only)
docs/architecture_data_model.md
.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/**
.agent-loop/merge-intents/WS-REV-001-03A1.json
```

The preimplementation refresh must replace `<next>` and confirm exact metadata
Expand All @@ -64,30 +70,44 @@ registration conventions before code.
attempt without authorizing it or mutating upstream rows.
- Database constraints permit at most one queue identity per Submission and
reject cross-project/task/Submission lineage.
- A REV-owned PostgreSQL write-time guard rejects any mismatch among stored
project, task, Submission/version, and admitting CheckerRun identities. A
pending queue or committed admission requires that exact CheckerRun to be
completed, current for the Submission, and `allow_review`; no checker hook or
automatic admission is added.
- No migration backfills historical submissions or fabricates CheckerRun/ART
facts. Required foreign facts may remain unpopulated only in explicitly
non-admitted setup shapes that cannot become pending.
- Queue history cannot be updated into a different Submission/task/project.
- Models contain no AUTH handle, token, grant query, ART locator/bytes, or CON
state.
- No router is registered and every REV lifecycle action remains unavailable.
- 03A1 cannot persist `leased` or an active-lease reference. Those shapes enter
only with the real REV-owned ReviewLease FK in 03A2.
- Admission idempotency enforces exact SHA-256 request digests, one replay
namespace/operation identity, pending-without-queue and committed-with-queue
shapes, and rejects conflicting reuse at the database boundary.

## Verification commands

Freeze exact node IDs at start. Minimum proof:

```text
cd backend && .venv/bin/alembic heads
cd backend && .venv/bin/pytest -q tests/test_alembic.py -k review_queue
cd backend && .venv/bin/pytest -q tests/test_alembic.py -k review_queue_foundation
cd backend && .venv/bin/pytest -q tests/test_review_queue_persistence.py
cd backend && .venv/bin/ruff check app/modules/reviews tests/test_review_queue_persistence.py tests/test_alembic.py
cd backend && .venv/bin/pytest --cov=app.modules.reviews.models --cov=app.modules.reviews.repository --cov-branch --cov-report=term-missing --cov-fail-under=90 -q tests/test_review_queue_persistence.py
cd backend && .venv/bin/pytest --cov=app.modules.reviews --cov-branch --cov-report=term-missing --cov-fail-under=90 -q tests/test_review_queue_persistence.py
python3 scripts/check_stale_review_contracts.py
python3 scripts/check_markdown_links.py
git diff --check
```

GitHub Actions runs the full sharded suite and repository coverage floor.
Focused PostgreSQL proof must include mismatched task/project/Submission/checker
refusal, non-final/non-current/non-`allow_review` refusal, immutable lineage and
first-queued time, replay conflicts, no historical backfill, and populated
downgrade refusal followed by an empty safe round trip.

## Required reviewers

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# External Review Response: WS-REV-001-03A1

## Comments addressed

- CodeRabbit: queue updates could reopen a closed row or decrease routing or
lifecycle generations. The PostgreSQL guard now rejects both transitions,
with direct-SQL tests.
- CodeRabbit: `ReviewQueueEntryInput` exposed closed-state fields that every
database insert rejected. The insert schema now contains only admission
fields and the repository always inserts `pending` with no close metadata.
- CodeRabbit: the invalid `leased` assertion depended on PostgreSQL check
evaluation order. The assertion now accepts either relevant named check while
still proving that the state cannot persist.
- CodeRabbit nitpick: the empty migration round trip did not assert both
truncate-reject triggers. Both are now part of the exact expected state.
- GitHub Backend: every semantic lane failed inventory collection with
`missing_lane_modules:tests/test_review_queue_persistence.py`. The focused REV
module is now registered once in `task_lifecycle`, and the canonical lane
ownership assertion is updated.
- Trusted `main` advanced through ART PR #249 while the repair was under review.
ART retains its merged `0050_guide_source_v2`; REV is reconciled as the exact
`0051_review_queue_foundation` successor, with both shared test conflicts
resolved additively.

## Comments deferred

- CodeRabbit's 30.30 percent docstring warning is not a repository CI failure.
GitHub's authoritative docstring-coverage step passed on the reviewed head,
and the new runtime classes and methods already carry docstrings. No unrelated
test/migration-function documentation expansion was added.

## Human decisions needed

None. Every actionable finding was in scope and resolved without adding product
behavior or crossing REV ownership.

## Commands rerun

- Ruff over REV, migration tests, and lane-inventory files: PASS.
- `PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest -q tests/test_ci_test_lanes.py`:
PASS, 33 tests.
- Isolated PostgreSQL `tests/test_review_queue_persistence.py` with complete
`app.modules.reviews` branch coverage and 90 percent floor: PASS.
- Isolated PostgreSQL `tests/test_alembic.py -k review_queue_foundation`: PASS.
- Reconciled ART+REV schema fingerprint and sole 0051 head: PASS.
- Two tests that needed a second lineage were updated for ART's merged unique
project/actor behavior; their isolated exact-node reruns pass.

## Remaining risks

Fresh GitHub semantic lanes/full coverage and CodeRabbit incremental review must
pass on the repaired commit. Human merge approval remains required, and this
repair does not start 03A2.
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# Internal Review Evidence: WS-REV-001-03A1

## Candidate

- Trusted base: `10720382cd9639f00f09578f772b97ab3afc358b`
- Reviewed implementation commit: `a5a778b4c1be2602d406fdf23c05bd8320f1c8cb`
- Scope: hidden REV queue/admission persistence, originally migration 0050 and
reconciled to migration 0051 after ART PR #249, focused tests,
data-model documentation, initiative status, and one merge intent
- Runtime exposure: none; no route, checker hook, lease, Review, revision,
contribution, AUTH, ART, or upstream mutation behavior is added

## Reviewer results

| Track | Result | Resolution |
|---|---:|---|
| Architecture | PASS | Scope includes metadata registration and merge intent; no ownership drift remains. |
| Senior engineering | PASS | Database-owned insert stamps and focused invariant tests resolved the original concerns. |
| QA/test | PASS | Fresh-id replay and all checker-admissibility branches are covered. |
| Product/ops | PASS | The change remains hidden persistence at the `allow_review` boundary. |
| Security/auth | PASS | Exact lineage, immutable identity, delete/truncate refusal, and downgrade safety are database-enforced. |
| Docs | PASS | Data-model wording distinguishes current persistence from later lease behavior. |
| CI integrity | PASS | No workflow or package-script change; no coverage gate was weakened. |
| Reuse/dedup | PASS with low risk | REV-specific repository patterns are appropriate; digest syntax remains locally duplicated to avoid importing an owner-specific AUTH or ART type. |
| Test delta | PASS | Direct constraint, trigger, replay, downgrade, and absence-of-lease proofs are present; no test was weakened or skipped. |

## Findings repaired

- Exact replay no longer depends on reuse of an internal row primary key.
- The contract now explicitly permits central metadata registration and the
required merge-intent artifact.
- Queue/admission creation timestamps and queue generations are stamped by
PostgreSQL, preventing caller-controlled queue age.
- Tests now cover non-completed, non-current, non-`allow_review`, checker
lineage, task/Submission lineage, and project mismatch refusal.
- Direct database tests isolate replay-key, operation, checker-run, digest,
state-shape, and committed-queue identity constraints.
- Admission-only and queue-only populated downgrade refusal are isolated and
prove that revision and protected rows survive the failed downgrade.
- The focused coverage command includes the complete new REV package.

## Deterministic evidence

- `backend/.venv/bin/alembic heads`: PASS on the original implementation; after
reconciliation with ART PR #249, the sole head is the REV successor
`0051_review_queue_foundation`.
- Isolated `tests/test_alembic.py -k review_queue_foundation`: PASS.
- Isolated `tests/test_review_queue_persistence.py`: PASS, 10 focused tests.
- Isolated `--cov=app.modules.reviews --cov-branch --cov-fail-under=90`: PASS.
- Ruff over the new REV package and focused tests: PASS.
- `python3 scripts/check_stale_review_contracts.py`: PASS.
- `python3 scripts/check_markdown_links.py`: PASS.
- `git diff --check`: PASS.

The full test suite and repository-wide 78 percent coverage floor are reserved
for GitHub Actions, per repository operations guidance and the user instruction.

## Remaining risks and gates

- The same SHA-256 syntax exists in multiple bounded owner modules. Creating a
cross-owner shared type is not justified inside this REV chunk.
- PostgreSQL validates upstream lineage at the REV write boundary; the queue
row records that fact and does not constrain future upstream-owned mutation.
- GitHub Actions, CodeRabbit, and human review remain pending.
- Merge does not start 03A2.

## Disposition

PASS for PR publication after the evidence-only documentation delta receives
its final narrow review. No reviewer session may remain open at publication.
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# PR Trust Bundle: WS-REV-001-03A1

## Chunk

`WS-REV-001-03A1` — Queue And Admission Persistence.

## Goal

Add the smallest hidden REV-owned persistence foundation for one exact
reviewable Submission queue identity and one idempotent admission operation.

## Human-approved intent

Start at a completed, current `allow_review` CheckerRun; consume the existing
Submission/version without changing upstream owners; stop before selection,
leases, Reviews, revisions, FinalAcceptance, or contributions.

## What changed and why

- Added `ReviewQueueEntry` and `ReviewAdmissionIdempotencyRecord` models,
schemas, and caller-transaction repository operations.
- Added REV Alembic revision 0051, following ART-owned 0050, with exact
lineage/admissibility guards,
immutable identity, replay constraints, delete/truncate protection, and
populated downgrade refusal.
- Registered the models and schema fingerprint, added focused PostgreSQL tests,
and clarified the data-model boundary.

This separates stable queue/admission identity from the later concurrency and
policy-version concerns of REV-owned lease persistence.

## Design chosen

One immutable queue row references the existing project, Task,
Submission/version, and admitting CheckerRun. A separate pending-to-committed
idempotency row records replay identity and binds only to the exact matching
queue. PostgreSQL is the final invariant boundary; repository methods flush but
never commit the caller's transaction.

## Alternatives rejected

- No checker completion hook or automatic admission.
- No route, backlog read, reviewer selection, claim, lease, or active-lease
placeholder.
- No upstream row changes, AUTH lookup, ART locator/bytes, or CON state.
- No historical backfill or fabricated checker fact.
- No destructive downgrade after either protected table contains a row.

## Scope and product behavior

This PR changes only the reviewed 03A1 contract, REV initiative evidence/status,
REV models/repository/schemas, migration, metadata registration, focused tests,
data-model docs, and one schema-v2 merge intent. It exposes no product API or
review lifecycle action.

## Acceptance criteria proof

Database constraints and triggers prove one queue per Submission, exact
project/task/Submission/version/checker lineage, current completed
`allow_review`, server-owned queue age/generations, immutable identity,
open/preferred storage without lease shape, exact replay namespaces/digest, and
pending-to-committed admission binding. Direct tests cover every refusal path
and isolated downgrade refusal for each protected table.

## Tests, test delta, and CI integrity

Focused isolated PostgreSQL migration and persistence tests pass. Focused
branch coverage for the complete new REV package passes the 90 percent floor.
Ruff, the stale review-contract scan, changed Markdown links, Alembic one-head,
and diff integrity pass. No existing test, assertion, skip, workflow, package
script, global 78 percent baseline, or CI gate was weakened. GitHub Actions will
run the full suite and repository coverage.

## Reviewer results and external review

Architecture, senior engineering, QA/test, product/ops, security/auth, docs,
CI integrity, reuse/dedup, and test-delta tracks pass after resolving replay,
server-stamping, scope, coverage, constraint, and downgrade-test findings.
The first GitHub Backend run failed before test execution because the new test
module was absent from the closed semantic-lane inventory. The module is now
registered exactly once in `task_lifecycle`, and all 33 lane-integrity tests
pass. All three actionable CodeRabbit comments and its truncate-trigger nitpick
were resolved: queue lifecycle is monotonic, the insert schema is pending-only,
the lease-state assertion is constraint-order independent, and the migration
round trip asserts both truncate guards. CodeRabbit's docstring warning is not
an authoritative CI failure; GitHub's docstring gate passed. Fresh external
checks remain required on the repaired head.

## Remaining risks and follow-up

The queue preserves the checker admission fact at write time; it does not own
or constrain later upstream state. Digest syntax remains locally repeated
rather than introducing a cross-owner abstraction in this chunk. 03A2 remains
a separately approved successor and must not begin from this PR.

## Human review focus

Review exact cross-owner lineage, current `allow_review` enforcement, fresh-ID
replay semantics, server-stamped queue age, no lease/API shape, protected
downgrade behavior, and absence of AUTH/ART/CON ownership leakage.

## Human merge ownership

Only the user may approve and merge this specific PR. Do not merge while any
current-head GitHub or CodeRabbit check is pending or failed, or while an
actionable review comment remains unresolved. Merge does not authorize 03A2.
9 changes: 9 additions & 0 deletions .agent-loop/merge-intents/WS-REV-001-03A1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"chunk_id": "WS-REV-001-03A1",
"chunk_title": "Queue And Admission Persistence",
"initiative_id": "WS-REV-001",
"next_chunk_id": "WS-REV-001-03A2",
"next_chunk_title": "Lease And Preference Persistence",
"next_requires_explicit_start": true,
"schema_version": 2
}
Loading
Loading