WS-ARCH-001-02E: expose ART admission consumption capability - #323
Merged
abiorh-claw merged 15 commits intoAug 12, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis change adds a deny-only ART capability that validates submission admission lineage, creates immutable bindings, records consumption state, and supports replay, concurrency, rollback, and migration verification. Initiative records mark 02E complete and 02F as the next boundary. ChangesART admission consumption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TASK
participant SubmissionAdmissionConsumptionService
participant ARTDatabase
participant AUTH
TASK->>SubmissionAdmissionConsumptionService: provide immutable Submission and lineage facts
SubmissionAdmissionConsumptionService->>AUTH: authorize admission consumption
SubmissionAdmissionConsumptionService->>ARTDatabase: lock records and validate lineage
SubmissionAdmissionConsumptionService->>AUTH: consume validated authority
SubmissionAdmissionConsumptionService->>ARTDatabase: create binding and mark admission consumed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agent-loop/CURRENT_STATE.md:
- Line 31: Define and consistently use the established module-level or
hidden-capability term instead of “public capability” for WS-ARCH-001-02E.
Update `.agent-loop/CURRENT_STATE.md` lines 31-31 and
`.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/CHUNK_MAP.md`
lines 12-13, preserving the explicit statement that the capability is
route-unreachable.
In
@.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02E-art-admission-binding-api.md:
- Around line 108-109: Update the documented focused test lane to include the
authoritative architecture checks from shared_foundations_a and
shared_foundations_b in backend/scripts/run_test_lanes.py, specifically the
route-isolation and module-boundary tests; alternatively, explicitly state that
those shared lanes provide the required coverage.
In `@backend/alembic/versions/0002_submission_admission_consumed_version.py`:
- Around line 13-33: The drop_constraint and create_check_constraint calls for
ck_submission_bundle_admissions_terminal_shape must both pass
terminal_shape=True, matching the active naming convention so the existing
constraint is dropped and recreated with the expected physical name.
In `@backend/tests/test_alembic.py`:
- Around line 123-126: Update the constraint query in the migration test around
the definition lookup to restrict results to the submission_bundle_admissions
table, joining or filtering through pg_constraint.conrelid and the table
identity. Keep the existing constraint-name filter and ensure the fetched
definition is specifically for that table’s migrated admission constraint.
In `@backend/tests/test_artifact_bindings_db.py`:
- Around line 47-66: The concurrency test must use the production-equivalent
admission and binding schema, including the terminal-shape check, partial unique
consumer index, and foreign-key constraints; update the schema setup at
backend/tests/test_artifact_bindings_db.py:47-66 to run Alembic or reproduce all
relevant constraints. At backend/tests/test_artifact_bindings_db.py:145-149,
make both competing admissions reuse the same task_context and submission
version, varying only admission identity and admission-owned evidence.
In `@backend/tests/test_artifact_bindings.py`:
- Around line 61-63: Replace the vague persisted ART admission lifecycle values
with subsystem-specific names across the API, model, migration, service, and
tests. Update the fixture’s admission.status in
backend/tests/test_artifact_bindings.py:61-63, the result and admission
assertions in backend/tests/test_artifact_bindings.py:169-178, and PostgreSQL
outcome assertions in backend/tests/test_artifact_bindings_db.py:142-192 to use
the renamed values consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b66850bc-b0f9-4fa3-adb7-6360f142cb4b
📒 Files selected for processing (20)
.agent-loop/CURRENT_STATE.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/CHUNK_MAP.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/STATUS.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02E-art-admission-binding-api.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/evidence/WS-ARCH-001-02E-admission-binding-manifest.md.ci/behavior-ownership/partition.v1.jsonbackend/alembic/versions/0002_submission_admission_consumed_version.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/api/__init__.pybackend/app/modules/artifacts/api/submission_admission.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/submission_bindings.pybackend/scripts/behavior_ownership.pybackend/scripts/run_test_lanes.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_bindings.pybackend/tests/test_artifact_bindings_db.pydocs/architecture_data_model.mddocs/spec_artifact_storage_service.md
💤 Files with no reviewable changes (1)
- backend/app/interfaces/artifact_operations.py
Comment on lines
+13
to
+33
| op.drop_constraint( | ||
| "ck_submission_bundle_admissions_terminal_shape", | ||
| "submission_bundle_admissions", | ||
| type_="check", | ||
| ) | ||
| op.add_column( | ||
| "submission_bundle_admissions", | ||
| sa.Column("consumed_by_submission_version", sa.Integer(), nullable=True), | ||
| ) | ||
| op.create_check_constraint( | ||
| "ck_submission_bundle_admissions_terminal_shape", | ||
| "submission_bundle_admissions", | ||
| "(status='ready' and consumed_at is null and consumed_by_submission_id is null " | ||
| "and consumed_by_submission_version is null and stale_at is null and stale_reason is null) " | ||
| "or (status='consumed' and consumed_at is not null and " | ||
| "consumed_by_submission_id is not null and consumed_by_submission_version > 0 " | ||
| "and stale_at is null and stale_reason is null) or " | ||
| "(status='stale' and consumed_at is null and consumed_by_submission_id is null " | ||
| "and consumed_by_submission_version is null and stale_at is not null " | ||
| "and octet_length(stale_reason) between 1 and 500)", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the SQLAlchemy naming convention and how sibling migrations name check constraints.
set -uo pipefail
fd -t f 'env.py' backend/alembic --exec sed -n '1,120p'
echo '--- naming_convention declarations ---'
rg -n -C4 'naming_convention' --type=py backend
echo '--- terminal_shape references ---'
rg -n -C3 'terminal_shape' --type=py backend
echo '--- drop_constraint / create_check_constraint usage in migrations ---'
rg -n -C2 'drop_constraint\(|create_check_constraint\(' backend/alembicRepository: Flow-Research/workstream
Length of output: 7775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- naming convention ---'
sed -n '1,45p' backend/app/db/base.py
echo '--- migration 0001 constraint creation ---'
fd -t f '0001_v01_baseline.py' backend/alembic/versions --exec rg -n -C4 'submission_bundle_admissions|terminal_shape|CheckConstraint|create_check_constraint' {}
echo '--- migration 0002 complete ---'
cat -n backend/alembic/versions/0002_submission_admission_consumed_version.py
echo '--- Alembic constraint tests ---'
sed -n '80,150p' backend/tests/test_alembic.py
echo '--- sibling migration constraint naming ---'
rg -n -C3 'drop_constraint|create_check_constraint' backend/alembic/versionsRepository: Flow-Research/workstream
Length of output: 799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
base = Path("backend/app/db/base.py").read_text()
m = re.search(r"NAMING_CONVENTION\s*=\s*(\{.*?\})", base, re.S)
print("NAMING_CONVENTION literal:")
print(m.group(1) if m else "not found")
migration = Path("backend/alembic/versions/0002_submission_admission_consumed_version.py").read_text()
for call in ("op.drop_constraint", "op.create_check_constraint"):
pos = migration.find(call)
print(f"{call}:")
print(migration[pos: migration.find(")", pos) + 1] if pos >= 0 else "not found")
print("Expected physical name under ck_%(table_name)s_%(constraint_name)s:")
print("ck_submission_bundle_admissions_terminal_shape")
print("Expanded name if the supplied name already contains the prefix:")
print("ck_submission_bundle_admissions_ck_submission_bundle_admissions_terminal_shape")
PYRepository: Flow-Research/workstream
Length of output: 1199
🏁 Script executed:
#!/bin/bash
set -u
echo '--- candidate baseline migrations ---'
fd -t f . backend/alembic/versions | sort | head -40
echo '--- constraint references in migrations and tests ---'
rg -n -C4 'submission_bundle_admissions|terminal_shape|drop_constraint|create_check_constraint' backend/alembic backend/tests
echo '--- installed SQLAlchemy/Alembic availability ---'
python3 - <<'PY'
for name in ("sqlalchemy", "alembic"):
try:
module = __import__(name)
print(name, getattr(module, "__version__", "version unavailable"), getattr(module, "__file__", "path unavailable"))
except Exception as exc:
print(name, type(exc).__name__, str(exc))
PYRepository: Flow-Research/workstream
Length of output: 50381
🌐 Web query:
Alembic op.create_check_constraint naming convention constraint_name op.drop_constraint logical name
💡 Result:
In Alembic, when using constraint naming conventions (specifically those that include the %(constraint_name)s token), the library automatically applies the convention to the string passed to operations like op.create_check_constraint and op.drop_constraint [1][2][3]. If you pass a logical name to these methods, Alembic treats it as the input for the %(constraint_name)s token within your configured convention [1][4]. Consequently, the actual constraint name generated or targeted in the database will be the result of the full naming convention expansion, rather than the logical name you provided [1][2]. To avoid this behavior and ensure the constraint is treated by its exact, literal name, you must use the op.f construct [1][5][3]. 1. Using op.f: This wrapper tells Alembic that the string has already been processed and should be used as-is, bypassing the automatic application of the naming convention [5][3]. Example for dropping a constraint: op.drop_constraint(op.f("your_logical_name"), "table_name", type_="check") [1][3] 2. Without op.f: If your naming convention is {"ck": "ck_%(table_name)s_%(constraint_name)s"} and you call op.drop_constraint("my_check", "users", type_="check"), Alembic will attempt to drop a constraint named ck_users_my_check [1][2]. For operations like op.create_check_constraint, using op.f on the name ensures that the constraint is created with the exact name you provide, which is often required to maintain consistency with existing database schemas or to avoid unintended naming conflicts [5].
Citations:
- 1: https://alembic.sqlalchemy.org/en/latest/naming.html?highlight=naming+convention
- 2: https://alembic.sqlalchemy.org/en/latest/naming.html
- 3: https://github.com/sqlalchemy/alembic/blob/main/docs/build/naming.rst
- 4: drop_constraint(... type_='check') working too well with naming conventions sqlalchemy/alembic#453
- 5: https://alembic.sqlalchemy.org/en/latest/ops.html
🏁 Script executed:
#!/bin/bash
set -u
echo '--- candidate baseline migrations ---'
fd -t f . backend/alembic/versions | sort | head -40
echo '--- relevant constraint references ---'
rg -n -C4 'submission_bundle_admissions|terminal_shape|drop_constraint|create_check_constraint' backend/alembic backend/tests
echo '--- installed package availability ---'
python3 - <<'PY'
for name in ("sqlalchemy", "alembic"):
try:
module = __import__(name)
print(name, getattr(module, "__version__", "unknown"))
except Exception as exc:
print(name, type(exc).__name__, str(exc))
PYRepository: Flow-Research/workstream
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- migration filenames ---'
fd -t f . backend/alembic/versions | sort | sed -n '1,80p'
echo '--- relevant files from prior log ---'
rg -n -m 40 '0001|0002_submission|0035_project|0036_art|terminal_shape|drop_constraint|create_check_constraint' /tmp/coderabbit-shell-logs/shell-output-hulEB2
echo '--- sibling migration snippets ---'
for file in $(fd -t f . backend/alembic/versions | sort | rg '/0035_|/0036_'); do
echo "### $file"
rg -n -C8 'drop_constraint|create_check_constraint|op\.f\(' "$file" || true
done
echo '--- focused Alembic tests ---'
rg -n -C8 'terminal_shape|0002|constraint' backend/tests/test_alembic.py | sed -n '1,240p'Repository: Flow-Research/workstream
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- dependency versions ---'
rg -n -C2 '^(alembic|SQLAlchemy|sqlalchemy)' pyproject.toml requirements*.txt backend/requirements*.txt 2>/dev/null || true
echo '--- Alembic implementation references ---'
curl -fsSL https://raw.githubusercontent.com/sqlalchemy/alembic/main/alembic/operations/ops.py |
rg -n -C8 'class DropConstraintOp|def drop_constraint|class CreateCheckConstraintOp|def create_check_constraint' |
sed -n '1,260p'
echo '--- Alembic naming documentation ---'
curl -fsSL https://raw.githubusercontent.com/sqlalchemy/alembic/main/docs/build/naming.rst |
rg -n -C8 'drop_constraint|create_check_constraint|op\.f|constraint_name' |
sed -n '1,260p'Repository: Flow-Research/workstream
Length of output: 9165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- migration filenames ---'
fd -t f . backend/alembic/versions | sort | sed -n '1,80p'
echo '--- relevant log lines ---'
rg -n -m 40 '0001|0002_submission|0035_project|0036_art|terminal_shape|drop_constraint|create_check_constraint' /tmp/coderabbit-shell-logs/shell-output-hulEB2
echo '--- sibling migration snippets ---'
for file in $(fd -t f . backend/alembic/versions | sort | rg '/0035_|/0036_'); do
echo "### $file"
rg -n -C8 'drop_constraint|create_check_constraint|op\.f\(' "$file" || true
done
echo '--- focused Alembic tests ---'
rg -n -C8 'terminal_shape|0002|constraint' backend/tests/test_alembic.py | sed -n '1,240p'Repository: Flow-Research/workstream
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- dependency versions ---'
rg -n -C2 '^(alembic|SQLAlchemy|sqlalchemy)' pyproject.toml requirements*.txt backend/requirements*.txt 2>/dev/null || true
echo '--- Alembic operation implementation ---'
curl -fsSL https://raw.githubusercontent.com/sqlalchemy/alembic/main/alembic/operations/ops.py |
rg -n -C8 'class DropConstraintOp|def drop_constraint|class CreateCheckConstraintOp|def create_check_constraint' |
sed -n '1,260p'
echo '--- Alembic naming documentation ---'
curl -fsSL https://raw.githubusercontent.com/sqlalchemy/alembic/main/docs/build/naming.rst |
rg -n -C8 'drop_constraint|create_check_constraint|op\.f|constraint_name' |
sed -n '1,260p'Repository: Flow-Research/workstream
Length of output: 9164
Pass terminal_shape to both Alembic operations.
The active convention expands the current name to ck_submission_bundle_admissions_ck_submission_bundle_admissions_terminal_shape. The drop then misses the existing constraint, and the create produces the wrong physical name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/alembic/versions/0002_submission_admission_consumed_version.py`
around lines 13 - 33, The drop_constraint and create_check_constraint calls for
ck_submission_bundle_admissions_terminal_shape must both pass
terminal_shape=True, matching the active naming convention so the existing
constraint is dropped and recreated with the expected physical name.
Source: Learnings
Comment on lines
+61
to
+63
| admission = SimpleNamespace( | ||
| id=str(request.admission_id), | ||
| status="ready", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Replace vague persisted ART admission lifecycle values.
ready, consumed, and stale are persisted ART admission lifecycle values. These names do not identify the ART admission subsystem. Define subsystem-specific values in the API, model, migration, service, and tests before release.
backend/tests/test_artifact_bindings.py#L61-L63: initialize the fixture with the renamed ART admission lifecycle value.backend/tests/test_artifact_bindings.py#L169-L178: update result and admission assertions for the renamed lifecycle value.backend/tests/test_artifact_bindings_db.py#L142-L192: update PostgreSQL outcome assertions for the renamed lifecycle values.
As per coding guidelines: “Use subsystem- or actor-specific names for workflow states, persisted tokens, API enum values, roles, and lifecycle names instead of vague labels.”
📍 Affects 2 files
backend/tests/test_artifact_bindings.py#L61-L63(this comment)backend/tests/test_artifact_bindings.py#L169-L178backend/tests/test_artifact_bindings_db.py#L142-L192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_artifact_bindings.py` around lines 61 - 63, Replace the
vague persisted ART admission lifecycle values with subsystem-specific names
across the API, model, migration, service, and tests. Update the fixture’s
admission.status in backend/tests/test_artifact_bindings.py:61-63, the result
and admission assertions in backend/tests/test_artifact_bindings.py:169-178, and
PostgreSQL outcome assertions in
backend/tests/test_artifact_bindings_db.py:142-192 to use the renamed values
consistently.
Source: Coding guidelines
abiorh-claw
self-requested a review
August 12, 2026 10:16
abiorh-claw
approved these changes
Aug 12, 2026
abiorh-claw
deleted the
codex/ws-arch-001-02e-art-admission-binding-api
branch
August 12, 2026 11:15
This was referenced Aug 12, 2026
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.
Chunk
WS-ARCH-001-02E — ART ready-admission consumption and binding public capability (L1).
Goal
Expose one hidden, deny-by-default ART capability that consumes an exact verified ready admission into one TASK-supplied immutable Submission identity and provider-neutral artifact binding.
Human-approved intent
Continue the reviewed WS-ARCH-001 02A–02I boundary repair sequence. This PR completes only 02E; it does not begin 02F or activate product behavior.
What changed
SubmissionAdmissionConsumptionPortrequest/result/error contracts underartifacts.api.ArtifactBinding; removed the obsolete submission binding seam fromartifact_operations.0002_admission_versionwhile keeping generic bindingscope_versionindependent.Why it changed
TASK must later compose Submission creation with ART admission consumption without importing ART persistence or allowing a raw authorization context/provider operation to cross the durable boundary.
Design chosen
The public port carries immutable IDs/facts only. ART locks its admission, evidence, and content; compares the typed TASK public context; consumes self-sufficient exact authority facts; serializes the generic binding scope using a PostgreSQL transaction advisory lock; then atomically binds and terminally consumes or stales the admission.
Alternatives rejected
ArtifactBinding.scope_versionas Submission lifecycle version.Scope control
The diff is limited to the 02E contract, exact CI registrations, one forward migration, focused tests, and required durable documentation. No dependencies or CI thresholds changed.
Product behavior
No live product behavior is activated. Production default authorization denies and no route reaches this capability.
Acceptance criteria proof
Tests/checks run
git diff --check: pass.WORKSTREAM_TEST_DATABASE_URLand full local execution is prohibitively slow.Test delta
Added
test_artifact_bindings.pyandtest_artifact_bindings_db.py; updated architecture and Alembic contract tests. No tests were skipped, removed, or weakened.CI integrity
Only exact new production targets and test modules were registered. No workflow, threshold, skip, package script, or gate semantics were weakened. CI-integrity review passed.
Reviewer results
Architecture, security, product/ops, QA, senior engineering, CI integrity, docs, and test-delta reviews pass after addressing exact replay, complete authority facts, ART corruption vs stale semantics, version persistence, schema proof, concurrency serialization, and terminal losing-admission behavior. Reuse review is the final internal confirmation.
External review
Pending GitHub Actions and CodeRabbit on this exact head.
Remaining risks
The focused PostgreSQL race test uses a compact isolated schema containing the relevant binding uniqueness/check constraints; hosted Alembic tests separately prove the real current-head consumed-version column and terminal constraint.
Follow-up work
WS-ARCH-001-02F may add the TASK-owned immutable Submission command and hidden composed transaction only after this PR merges. 02G–02I remain later boundaries.
Human review focus
Review exact authority facts, stale reasons, advisory-lock scope, admission terminal states, migration constraint, and absence of TASK persistence/provider ownership in ART.
Human merge ownership
A human maintainer owns the merge decision. Codex will not merge this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation