WS-AUTH-003-01: establish AUTH module boundary foundation - #305
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a public authorization API, static import-boundary enforcement, test-structure debt validation, behavior-ownership controls, architecture tests, and a mandatory CI preflight. ChangesAuthorization boundary foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant AuthorizationBoundary
participant TestStructureBoundary
participant BehaviorOwnership
participant ArchitectureTests
participant BackendLanes
GitHubActions->>AuthorizationBoundary: Validate import boundary
GitHubActions->>TestStructureBoundary: Validate debt ledger and assertion maps
GitHubActions->>BehaviorOwnership: Validate ownership transitions
GitHubActions->>ArchitectureTests: Run targeted architecture tests
ArchitectureTests-->>GitHubActions: Return validation results
GitHubActions->>BackendLanes: Start lanes after preflight success
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md (1)
104-120: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDo not document a synchronous-first validation job.
These lines prescribe synchronous validator commands and a blocking preflight job. The applicable rule requires asynchronous execution by default and prohibits documentation for synchronous-first checkers or jobs. Define an asynchronous validation workflow, or add an approved CI-gate exception before merge.
As per coding guidelines, “Use asynchronous execution by default; do not document synchronous-first checkers or jobs.”
🤖 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 @.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md around lines 104 - 120, Replace the synchronous-first validation instructions around the listed validator and pytest commands with an asynchronous-by-default workflow, including concurrent execution where applicable. If a blocking preflight job is required, add the approved CI-gate exception before documenting it; otherwise describe the asynchronous validation path without synchronous-first terminology.Source: Coding guidelines
🧹 Nitpick comments (3)
backend/scripts/test_structure_boundary.py (2)
457-478: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
invariant_categoryto a named set.
target_layeris validated againstTEST_LAYERS.invariant_categoryaccepts any non-empty string, so a mapping can record a vague label such as"misc"and still pass. Define an explicit frozenset of AUTH invariant categories and validate against it, in the same way asTEST_LAYERS.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."
🤖 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/scripts/test_structure_boundary.py` around lines 457 - 478, Define a named frozenset of allowed AUTH invariant categories alongside TEST_LAYERS, then update _validate_mapping_entry to validate value["invariant_category"] against that set instead of accepting any non-empty string. Preserve the existing required-key and string validations.Source: Coding guidelines
128-143: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the scoped-path scan.
scoped_test_pathsparses every file underbackend/testswith_read_source.validatereaches it three times:observed_debt,_assert_current_matches, and_test_nodes. Each call re-reads and re-parses the full tree, and the ledger shows single test files above 14,000 lines. Cache the result per root, or reuse one parsed inventory across the validate pass.🤖 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/scripts/test_structure_boundary.py` around lines 128 - 143, Cache the result of scoped_test_paths per root so repeated calls from validate, including observed_debt, _assert_current_matches, and _test_nodes, reuse the same scanned path set instead of re-reading and parsing backend/tests. Keep results isolated by root and preserve the existing sorted-path output.backend/tests/architecture/test_test_structure_boundary.py (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the untested fail-closed paths.
The suite does not cover four rejection paths of the new validator:
noncanonical_debt_ledgerinload_ledger, when the committed file bytes differ from_canonical_json.duplicate_assertion_dispositioninvalidate_assertion_maps, when two mapping entries reuse oneold_assertion_id.invalid_old_test_nodein_old_source, when the node path does not start withtests/.trusted_revision_unavailableand theNonebranch in_trusted_ledger.These are the security-relevant denial paths of the gate.
As per coding guidelines: "New or materially changed backend subsystems must maintain at least 90% test coverage".
🤖 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/architecture/test_test_structure_boundary.py` around lines 29 - 31, Add focused tests covering the validator’s fail-closed paths: make load_ledger reject bytes differing from _canonical_json with noncanonical_debt_ledger; make validate_assertion_maps reject reused old_assertion_id values with duplicate_assertion_disposition; make _old_source reject node paths not prefixed by tests/ with invalid_old_test_node; and cover both trusted_revision_unavailable and the None return path in _trusted_ledger. Keep existing success-path assertions intact and ensure these branches satisfy the required coverage.Source: Coding guidelines
🤖 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-AUTH-003-module-boundary-recovery/CHUNK_MAP.md:
- Line 5: Define the AUTH-specific lifecycle vocabulary and apply the matching
boundary-foundation states in both records: update CHUNK_MAP.md line 5 to
replace “in review” with the defined AUTH-specific chunk lifecycle state, and
update STATUS.md line 3 to replace “implementation” with the corresponding
AUTH-specific initiative lifecycle state.
In @.ci/behavior-ownership/shared/auth-boundary-test-structure-validator.json:
- Around line 51-65: Add
test_test_structure_boundary.py::test_old_assertion_inventory_binds_exact_span_and_hash
to the JSON record’s tests array, preserving the existing entries and ordering.
In `@backend/app/modules/authorization/api/facts.py`:
- Around line 49-53: Update the service identity normalization in the actor
validation logic so whitespace-only values become None before validation and
persistence. Ensure non-service actors cannot retain an empty normalized
identity, while preserving the existing service-actor mismatch validation.
- Around line 69-78: Update ResourceFacts validation to reject any resource_id
that is neither a UUID nor a string, raising the existing validation error
before the facts are frozen. Preserve the current blank-string check and
trimming behavior for string identifiers, and allow valid UUID values unchanged.
In `@backend/scripts/authorization_boundary.py`:
- Around line 86-155: Extend _DynamicImportVisitor to reject dynamic access to
import capabilities through sys.modules, including aliases, so expressions such
as sys.modules["builtins"].__import__ raise AuthorizationBoundaryError during
scanning. Add the requested fixture and assertion in
backend/tests/architecture/test_authorization_boundary.py lines 143-172 covering
this bypass; update backend/scripts/authorization_boundary.py lines 86-155 for
the detection logic.
- Around line 55-84: Update _resolve_from and its caller around _module_name to
preserve whether the source file is a package __init__.py, so relative imports
resolve from the package directory rather than treating the initializer as a
regular module; update backend/scripts/authorization_boundary.py lines 158-174
to pass this context, and add the package __init__.py fixture and canonical
outbound-target assertion in
backend/tests/architecture/test_authorization_boundary.py lines 175-204.
In `@backend/scripts/behavior_ownership.py`:
- Around line 207-209: Ensure the trusted assignment validation rejects
reordered trusted assignments by requiring `trusted_targets` to be sorted before
constructing `current_by_target` and comparing against `trusted_assignments`;
preserve the existing `BehaviorOwnershipError("untrusted_partition_change")`
failure. Add a regression test covering two trusted targets whose assignments
are reversed, with the digest recomputed for the reordered data.
In `@backend/scripts/test_structure_boundary.py`:
- Around line 596-611: Replace the inventories.setdefault call in the mapping
loop with an explicit lookup and conditional initialization so _old_source and
_assertion_inventory run only when revision_node is not already cached. Preserve
reuse of the cached inventory for subsequent mappings and keep
_validate_old_assertion unchanged.
- Around line 320-323: Update the validation condition in the debt-ledger
structure check to require value["policy_sha256"] to be a string before calling
re.fullmatch, matching the isinstance guard used near line 362. Ensure
non-string values raise TestStructureError("invalid_debt_ledger") so main
preserves the mapped error outcome.
---
Outside diff comments:
In
@.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md:
- Around line 104-120: Replace the synchronous-first validation instructions
around the listed validator and pytest commands with an asynchronous-by-default
workflow, including concurrent execution where applicable. If a blocking
preflight job is required, add the approved CI-gate exception before documenting
it; otherwise describe the asynchronous validation path without
synchronous-first terminology.
---
Nitpick comments:
In `@backend/scripts/test_structure_boundary.py`:
- Around line 457-478: Define a named frozenset of allowed AUTH invariant
categories alongside TEST_LAYERS, then update _validate_mapping_entry to
validate value["invariant_category"] against that set instead of accepting any
non-empty string. Preserve the existing required-key and string validations.
- Around line 128-143: Cache the result of scoped_test_paths per root so
repeated calls from validate, including observed_debt, _assert_current_matches,
and _test_nodes, reuse the same scanned path set instead of re-reading and
parsing backend/tests. Keep results isolated by root and preserve the existing
sorted-path output.
In `@backend/tests/architecture/test_test_structure_boundary.py`:
- Around line 29-31: Add focused tests covering the validator’s fail-closed
paths: make load_ledger reject bytes differing from _canonical_json with
noncanonical_debt_ledger; make validate_assertion_maps reject reused
old_assertion_id values with duplicate_assertion_disposition; make _old_source
reject node paths not prefixed by tests/ with invalid_old_test_node; and cover
both trusted_revision_unavailable and the None return path in _trusted_ledger.
Keep existing success-path assertions intact and ensure these branches satisfy
the required coverage.
🪄 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: fa02f14f-b49d-4709-9cae-e14c4114b488
📒 Files selected for processing (27)
.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/CHUNK_MAP.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/STATUS.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/TEST_STRUCTURE_DEBT.json.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/TEST_STRUCTURE_POLICY.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md.ci/behavior-ownership/auth/auth-boundary-action-ids.json.ci/behavior-ownership/auth/auth-boundary-decisions.json.ci/behavior-ownership/auth/auth-boundary-errors.json.ci/behavior-ownership/auth/auth-boundary-facts.json.ci/behavior-ownership/auth/auth-boundary-ports.json.ci/behavior-ownership/partition.v1.json.ci/behavior-ownership/shared/auth-boundary-import-validator.json.ci/behavior-ownership/shared/auth-boundary-test-structure-validator.json.github/workflows/backend.ymlbackend/app/modules/authorization/api/__init__.pybackend/app/modules/authorization/api/action_ids.pybackend/app/modules/authorization/api/decisions.pybackend/app/modules/authorization/api/errors.pybackend/app/modules/authorization/api/facts.pybackend/app/modules/authorization/api/ports.pybackend/scripts/authorization_boundary.pybackend/scripts/behavior_ownership.pybackend/scripts/test_structure_boundary.pybackend/tests/architecture/test_authorization_boundary.pybackend/tests/architecture/test_test_structure_boundary.pybackend/tests/test_behavior_ownership.pydocs/spec_authorization_service.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/scripts/authorization_boundary.py (1)
149-180: 🔒 Security & Privacy | 🟠 MajorTrack aliases of
sysbefore checkingsys.modules.
alias = sysdoes not updatesys_names. A lateralias.modules["builtins"].__import__(...)bypasses the dynamic-import guard and can load a private AUTH module without a static edge.Add direct
sysalias tracking invisit_Assign. Add an architecture test for this assignment-alias form.Proposed fix
def visit_Assign(self, node: ast.Assign) -> None: """Track simple aliases of recognized import callables.""" + if isinstance(node.value, ast.Name) and node.value.id in self.sys_names: + for target in node.targets: + if isinstance(target, ast.Name): + self.sys_names.add(target.id) if self._is_import_callable(node.value): for target in node.targets: if isinstance(target, ast.Name): self.import_call_names.add(target.id)🤖 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/scripts/authorization_boundary.py` around lines 149 - 180, Update visit_Assign to track direct assignments where the value is a recognized sys name, adding each Name target to sys_names before the existing import-call alias handling. Preserve current alias tracking and add an architecture test covering alias = sys followed by alias.modules access that must raise the dynamic-import guard.
🤖 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.
Duplicate comments:
In `@backend/scripts/authorization_boundary.py`:
- Around line 149-180: Update visit_Assign to track direct assignments where the
value is a recognized sys name, adding each Name target to sys_names before the
existing import-call alias handling. Preserve current alias tracking and add an
architecture test covering alias = sys followed by alias.modules access that
must raise the dynamic-import guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0738dff5-bcbb-4ed4-adc5-0a7be72f7633
📒 Files selected for processing (16)
.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/CHUNK_MAP.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/STATUS.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/reviews/WS-AUTH-003-01-external-review-response.md.ci/behavior-ownership/auth/auth-boundary-facts.json.ci/behavior-ownership/shared/auth-boundary-import-validator.json.ci/behavior-ownership/shared/auth-boundary-test-structure-validator.json.github/workflows/backend.ymlbackend/app/modules/authorization/api/facts.pybackend/scripts/authorization_boundary.pybackend/scripts/behavior_ownership.pybackend/scripts/run_test_lanes.pybackend/scripts/test_structure_boundary.pybackend/tests/architecture/test_authorization_boundary.pybackend/tests/architecture/test_test_structure_boundary.pybackend/tests/test_behavior_ownership.py
💤 Files with no reviewable changes (1)
- .github/workflows/backend.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- .agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/STATUS.md
- .ci/behavior-ownership/shared/auth-boundary-import-validator.json
- .agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/CHUNK_MAP.md
- .ci/behavior-ownership/auth/auth-boundary-facts.json
- backend/app/modules/authorization/api/facts.py
- .agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/chunks/WS-AUTH-003-01-boundary-foundation.md
- backend/tests/architecture/test_authorization_boundary.py
- backend/tests/architecture/test_test_structure_boundary.py
- backend/scripts/test_structure_boundary.py
- backend/scripts/behavior_ownership.py
abiorh-claw
self-requested a review
August 9, 2026 22:04
abiorh-claw
approved these changes
Aug 9, 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.
Workstream PR Trust Bundle
Chunk
WS-AUTH-003-01— AUTH module-boundary foundationGoal
Install the minimal dependency-free public AUTH namespace, freeze existing bidirectional private-import debt, and prevent new AUTH structural-test debt without moving product behavior.
Human-approved intent
Workstream remains a modular monolith. Modules communicate only through owning-module public interfaces so later HTTP, gRPC, or asynchronous extraction does not require untangling private imports. Recovery is incremental: freeze existing debt, repair one touched capability at a time, and preserve one primary observable behavior per test.
What changed
app.modules.authorization.apiwith opaque identifiers, immutable facts, stable decisions/errors, and typed Protocol ports.Why it changed
AUTH and its consumers currently import private implementation details in both directions. Rewriting everything in one PR would be too broad to verify; leaving it unenforced would deepen the debt. This foundation makes all existing violations explicit and blocks new ones before capability-level repair begins.
Design chosen
Use dependency-free in-process contracts plus deterministic AST/ledger gates. Existing edges remain frozen only as temporary measured debt. Later feature chunks expose and migrate one exact capability, shrink both ledgers, and preserve assertions through trusted ancestor source mappings.
Alternatives rejected
Scope control
No AUTH runtime evaluator, model, repository, database query, migration, route, permission, action, role, service identity, consumer import, or product lifecycle behavior changed.
Product behavior
No Workstream product behavior changed. This is an architecture and CI foundation only.
Acceptance criteria proof
IMPORT_LEDGER.md.Tests/checks run
Test delta
No tests were removed, skipped, xfailed, or weakened. New tests prove one primary validator/API invariant each; parametrization covers equivalent bypass variants. One existing expected error changed from generic unauthorized drift to the more precise malformed-trusted-partition error.
CI integrity
The new exact-head preflight runs static boundary validators once before the matrix. The two architecture test modules are registered exactly once in the canonical
shared_foundationslane, so hosted coverage owns their execution. There are no path filters, fallback success,continue-on-error, threshold changes, or test/coverage replacements.Reviewer results
External review
CodeRabbit supplied nine actionable findings after its initially rate-limited status. All nine are fixed and resolved, and internal security re-review passed after five focused bypass-probe rounds. Exact-head hosted checks passed on corrective implementation head
250cf7baand final status-only head96aa9455. The earlier lane-inventory failure remains superseded by the canonical one-time registration fix.Remaining risks
The foundation intentionally freezes rather than removes legacy debt. Static limits cannot prove semantic cohesion, so every later capability repair still requires architecture, security, QA, and test-delta review.
Follow-up work
After human merge, amend and resume preserved
WS-POL-003-03Aas the first capability repair through the new AUTH API, shrinking the exact import and test-debt ledgers it touches.Human review focus
Inspect public API purity, dynamic-import fail-closed coverage, trusted ledger/ownership custody, assertion-map completeness, and confirmation that no runtime authorization behavior moved.
Human merge ownership
Summary by CodeRabbit
New Features
Documentation
Tests