WS-ARCH-001-02C: expose CHECKER pre-submit public API - #320
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds a public CHECKERS pre-submit API with immutable planning and execution contracts. It routes ART and TASK callers through the planning port, removes private module edges, adds bounded execution facts, and records boundary validation and completion state. ChangesCHECKERS pre-submit boundary
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PreSubmitContext
participant EffectivePreSubmissionPlanningPort
participant PreSubmissionCheckerCatalogue
participant PreSubmissionExecutionResult
PreSubmitContext->>EffectivePreSubmissionPlanningPort: compile_effective_plan(lineage, policy, bundle)
EffectivePreSubmissionPlanningPort->>PreSubmissionCheckerCatalogue: compile_effective_plan(...)
PreSubmissionCheckerCatalogue-->>PreSubmitContext: EffectivePreSubmissionExecutionPlan
PreSubmitContext->>PreSubmissionExecutionResult: bounded_facts()
PreSubmissionExecutionResult-->>PreSubmitContext: PreSubmissionExecutionFacts
Possibly related PRs
🚥 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 |
77157e1 to
5e4f04b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@backend/app/modules/checkers/api/pre_submit.py`:
- Around line 91-94: Rename the vague lifecycle fields in the CHECKERS
pre-submit models to checker-specific names, such as checker_definition_state
and checker_execution_status, and propagate those names consistently through
EffectivePreSubmissionPlanEntry.as_dict(), _entry_from_definition(), and
PreSubmissionExecutionResult.bounded_facts(). Update the related public API
tests to assert the renamed fields while preserving their existing behavior.
In `@backend/tests/architecture/test_module_boundaries.py`:
- Around line 243-246: Update the import boundary assertion to match the exact
`app.modules.checkers.api` namespace, allowing the module itself or descendants
with a following dot, while rejecting names such as `api_private` and `apix`.
In `@backend/tests/checkers/test_pre_submit_public_api.py`:
- Around line 46-71: Update
test_public_planning_port_compiles_the_canonical_plan to assign
build_pre_submission_checker_catalogue() to an
EffectivePreSubmissionPlanningPort reference before invoking
compile_effective_plan. Compile the same lineage, effective_policy, and
compiled_bundle twice through that public-port reference, then assert both
plans’ as_dict() results and plan_sha256 values are equal while preserving the
existing type, lineage, and self-hash checks.
- Around line 87-118: The test around bounded_facts() should include forbidden
custody, archive, scratch, provider, and evidence metadata in the execution
result, then assert those fields are absent from the returned public facts. Also
verify that only the allowlisted metadata keys entry_count, finding_count, and
matched_category_count are exposed, while preserving the existing plan,
eligibility, and entry assertions.
🪄 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: be7f710c-e920-4a98-88b5-74dda6368c08
📒 Files selected for processing (13)
.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02C-checker-pre-submit-api.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/evidence/WS-ARCH-001-02C-checker-manifest.md.ci/module-boundaries/private-edge-debt.v1.jsonbackend/app/modules/artifacts/submission_admission.pybackend/app/modules/checkers/api/__init__.pybackend/app/modules/checkers/api/pre_submit.pybackend/app/modules/checkers/catalogue.pybackend/app/modules/checkers/effective_plan.pybackend/app/modules/checkers/pre_submit_execution.pybackend/app/modules/tasks/pre_submit_context.pybackend/tests/architecture/test_module_boundaries.pybackend/tests/checkers/test_pre_submit_public_api.pydocs/architecture_lockdown.md
💤 Files with no reviewable changes (1)
- .ci/module-boundaries/private-edge-debt.v1.json
| classification: str | ||
| state: str | ||
| disabled_behavior: str | ||
| dispatch_kind: str |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use CHECKER-specific lifecycle field names.
state and status are vague outside the CHECKERS module. Rename them to names such as checker_definition_state and checker_execution_status.
Update EffectivePreSubmissionPlanEntry.as_dict(), _entry_from_definition(), PreSubmissionExecutionResult.bounded_facts(), and public API tests in the same change.
As per coding guidelines, use subsystem- or actor-specific names for workflow states and API lifecycle values instead of vague labels.
Also applies to: 192-195
🤖 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/app/modules/checkers/api/pre_submit.py` around lines 91 - 94, Rename
the vague lifecycle fields in the CHECKERS pre-submit models to checker-specific
names, such as checker_definition_state and checker_execution_status, and
propagate those names consistently through
EffectivePreSubmissionPlanEntry.as_dict(), _entry_from_definition(), and
PreSubmissionExecutionResult.bounded_facts(). Update the related public API
tests to assert the renamed fields while preserving their existing behavior.
Source: Coding guidelines
| def test_public_planning_port_compiles_the_canonical_plan() -> None: | ||
| """The public port delegates to the sole deterministic CHECKER compiler.""" | ||
| policy = _effective_policy() | ||
| policy_hash = canonical_json_hash(policy) | ||
| compiled = compile_effective_project_submission_artifact_policy(policy, policy_hash) | ||
| lineage = EffectivePreSubmissionPlanLineage( | ||
| project_id=uuid4(), | ||
| guide_id=uuid4(), | ||
| guide_version=1, | ||
| source_snapshot_id=uuid4(), | ||
| source_snapshot_hash="sha256:" + "1" * 64, | ||
| effective_policy_id=uuid4(), | ||
| effective_policy_hash=policy_hash, | ||
| pre_submit_policy_id=uuid4(), | ||
| pre_submit_policy_bundle_hash=compiled.compiled_bundle_hash, | ||
| ) | ||
|
|
||
| plan = build_pre_submission_checker_catalogue().compile_effective_plan( | ||
| lineage=lineage, | ||
| effective_policy=policy, | ||
| compiled_bundle=compiled.compiled_bundle, | ||
| ) | ||
|
|
||
| assert type(plan) is EffectivePreSubmissionExecutionPlan | ||
| assert plan.lineage is lineage | ||
| assert plan.plan_sha256 == canonical_json_hash(plan.as_dict()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exercise the public deterministic planning contract.
The test imports EffectivePreSubmissionPlanningPort but invokes compile_effective_plan on the concrete catalogue. A catalogue that no longer conforms to the public port could still pass. Bind and validate the catalogue through EffectivePreSubmissionPlanningPort.
The self-hash assertion checks only one output. Compile identical inputs twice through the public port and compare both as_dict() and plan_sha256.
🤖 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/checkers/test_pre_submit_public_api.py` around lines 46 - 71,
Update test_public_planning_port_compiles_the_canonical_plan to assign
build_pre_submission_checker_catalogue() to an
EffectivePreSubmissionPlanningPort reference before invoking
compile_effective_plan. Compile the same lineage, effective_policy, and
compiled_bundle twice through that public-port reference, then assert both
plans’ as_dict() results and plan_sha256 values are equal while preserving the
existing type, lineage, and self-hash checks.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02C-checker-pre-submit-api.md:
- Around line 91-92: Extend the coverage command in the pre-submit verification
block to include app.modules.checkers.catalogue,
app.modules.checkers.effective_plan, and
app.modules.checkers.pre_submit_execution alongside app.modules.checkers.api,
and add tests/test_checker_catalogue.py and
tests/test_effective_pre_submit_execution.py to the pytest targets while
preserving the 90% coverage threshold.
In
@.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/reviews/WS-ARCH-001-02C-external-review-response.md:
- Around line 5-17: Update the review-response entry to identify the four
CodeRabbit requests as findings from the earlier review and label the
no-actionable-comments result as the follow-up exact-head review. Use the
repository history to record the full commit SHA for each referenced review
round, including the commits associated with the preflight and hosted-lane
outcomes where they are referenced.
🪄 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: daf2eb8f-21f3-4e2e-aaf3-347efbe6b30c
📒 Files selected for processing (12)
.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-02C-checker-pre-submit-api.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/reviews/WS-ARCH-001-02C-external-review-response.md.ci/behavior-ownership/partition.v1.jsonbackend/app/modules/checkers/api/pre_submit.pybackend/app/modules/checkers/effective_plan.pybackend/app/modules/checkers/pre_submit_execution.pybackend/scripts/behavior_ownership.pybackend/tests/architecture/test_module_boundaries.pybackend/tests/test_checker_catalogue.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/modules/checkers/api/pre_submit.py
- backend/app/modules/checkers/pre_submit_execution.py
72013b4 to
9bd4b56
Compare
2 participants
Chunk
WS-ARCH-001-02C — CHECKER Pre-Submit Public API
Goal
Expose dependency-safe CHECKER planning and bounded execution-result contracts so later ART preparation work can stop importing CHECKER internals.
Human-approved intent
Repair modular-monolith boundaries incrementally while continuing the existing product path. Use the canonical ARCH numbering, one active implementation worktree, and no broad rewrite.
What changed
app.modules.checkers.apiwith immutable plan lineage, plan, entry, packet, bounded result, error, and planning-port contracts.PreSubmissionCheckerCatalogueimplement the public planning port while retaining the sole existing compiler.app.modules.checkers.api.Why it changed
TASK and ART admission imported CHECKER catalogue, compiler, and execution internals. That coupling would make later ART/AUTH activation unsafe and prevent module extraction.
Design chosen
CHECKERS owns one typed planning port. TASK supplies exact locked lineage and policy values; the existing CHECKER catalogue delegates to the existing compiler. ART retains custody and evidence ownership. Public result facts expose only checker outcomes.
Alternatives rejected
Scope control and product behavior
No route, action, permission, durable job, persistence, schema, migration, provider operation, or authorization activation changed. Remaining ART materialization/evidence private CHECKER edges remain frozen for 02D.
Acceptance criteria proof
Tests and checks
61 passed.2 passedin a new CHECKER-owned file.94.96%locally.75 passedplus one setup error because this shell lacksWORKSTREAM_TEST_DATABASE_URL; hosted isolated DB lanes own final database-backed proof.Test delta and CI integrity
No test was removed, skipped, xfailed, or weakened. No workflow, dependency, timeout, lane, or coverage threshold changed.
Reviewer results
Base reconciliation and external review
Merged current
mainto adopt the atomic chunk-state gate, then recorded the 02C contract, chunk map, initiative status, and current-state projection together. The first reconciled-head Backend preflight exposed a missing behavior-ownership partition entry for the new public API target; that exact target and its deterministic partition digest are now registered without weakening any gate. Hosted lane collection then rejected the new standalone test module, so its two focused tests were preserved in the already-canonical architecture lane without changing lane topology. CodeRabbit’s initial four findings and follow-up coverage/review-evidence findings were fixed: CHECKER-specific lifecycle names, exact dotted namespace enforcement, repeated deterministic public-port proof, independently allowlisted non-negative metadata projection, coverage proof for every changed CHECKERS module, and exact review-round SHAs. Fresh GitHub Actions and CodeRabbit are running on exact head61f76043.Remaining risks and follow-up
Closed result vocabularies remain string-typed because production construction is validated by the private executor. Later consumers may tighten these to closed public aliases. ARCH-02D must migrate the remaining ART materialization/evidence consumers to this public surface.
Human review focus
Confirm single CHECKER ownership, exact ledger reduction, custody exclusion, unchanged behavior, and that 02D—not AUTH activation—is the next boundary.
Human merge ownership
Human maintainers retain merge authority. The agent will not merge without explicit approval.
Summary by CodeRabbit
New Features
Architecture
Documentation
Tests