ART 04B2: execute default pre-submit checks - #282
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 (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughART-04B2 adds fixed-service authorization, sealed ZIP projection, reservation-aware scratch custody, hidden default checker execution, bounded results, cancellation handling, cleanup, tests, CI coverage gates, and documentation. Project-policy execution and durable evidence remain deferred. ChangesART-04B2 default checker execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PreparedBundleMaterializationService
participant Authorization
participant ArtifactScratchManager
participant SubmissionArchiveInspector
participant DefaultPreSubmissionProcessor
PreparedBundleMaterializationService->>Authorization: consume fixed materializer capability
Authorization-->>PreparedBundleMaterializationService: authorize or deny
PreparedBundleMaterializationService->>ArtifactScratchManager: reserve bounded workspace
PreparedBundleMaterializationService->>SubmissionArchiveInspector: project validated ZIP
SubmissionArchiveInspector->>DefaultPreSubmissionProcessor: execute with sealed tree
DefaultPreSubmissionProcessor-->>PreparedBundleMaterializationService: return bounded results
SubmissionArchiveInspector->>ArtifactScratchManager: revoke tree and clean workspace
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 review |
|
021ed21 to
faf40e7
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md (1)
141-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the CI gate list with the local verification commands.
The local verification block at Line 129 covers
app/core/cancellation.py,app/core/config.py,app/core/file_locks.py. The CI gate at Line 144 covers onlyapp/core/cancellation.py,app/core/file_locks.py. This chunk changesapp/core/config.py(a new scratch setting is consumed). Addapp/core/config.pyto the CI gate, or state why the config gate stays local only.📝 Proposed gate alignment
-coverage report --include='app/core/cancellation.py,app/core/file_locks.py' --precision=2 --fail-under=90 +coverage report --include='app/core/cancellation.py,app/core/config.py,app/core/file_locks.py' --precision=2 --fail-under=90🤖 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-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md around lines 141 - 146, Update the CI coverage command for core modules in the default checker execution instructions to include app/core/config.py alongside app/core/cancellation.py and app/core/file_locks.py, matching the local verification command. Keep the existing coverage threshold and other gate entries unchanged.backend/app/modules/checkers/runner.py (1)
773-779: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the legacy
matched_categoriesmetadata shape.
matched_low_quality_patternsreturns a sorted tuple, while the legacy standalone precheck result exposed categories as a list in declaration order.matched_categoriesmust remain a list with the old category order for outcomes before 05B.🤖 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/runner.py` around lines 773 - 779, Update the low-quality artifact check around matched_low_quality_patterns and the _warning metadata construction so matched_categories remains a list in the legacy declaration order, rather than exposing the helper’s sorted tuple. Preserve the existing matched-pattern count and ensure this shape applies to outcomes before 05B.
🧹 Nitpick comments (7)
backend/tests/test_default_pre_submit_execution.py (2)
406-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish the three invalid-state failures.
pytest.raises(PreSubmissionInfrastructureUnavailable)carries nomatch. All three parameter cases (stale_entry,duplicate,unknown) raise the same exception class with different messages. The test passes if two cases collapse onto one code path. Addmatch=per case so each parameter proves its own guard.🤖 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_default_pre_submit_execution.py` around lines 406 - 407, Update the parameterized test around service.materialize_prepared_bundle to provide a case-specific match pattern in pytest.raises for stale_entry, duplicate, and unknown. Use each expected exception message so the test verifies the distinct guard path rather than only the shared PreSubmissionInfrastructureUnavailable exception type.
608-610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet the deadline through configuration instead of private attributes.
This line mutates
preparation._active[...]and readsrequest.prepared_artifact._binding. Both are private. The same pattern appears at Lines 659-661. A refactor ofArtifactPreparationServicebreaks these two tests with an opaqueKeyErrororAttributeError.
_limits()already buildsArtifactPreparationLimits. Add a smalltotal_deadline_secondsto the limits used by these two tests, and drive the timeout through the public configuration path.🤖 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_default_pre_submit_execution.py` around lines 608 - 610, Replace the private-state deadline mutations in both affected tests with public configuration: extend the ArtifactPreparationLimits produced by _limits() with a small total_deadline_seconds value, then configure ArtifactPreparationService through that limits path so the timeout is exercised without accessing preparation._active or request.prepared_artifact._binding.backend/app/modules/checkers/pre_submit_defaults.py (1)
55-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed structure instead of a heterogeneous dict.
The return type is
dict[str, bool | list[str]]. Every caller must narrow the value type by key.runner.pyLine 730 assignsmissing_attestation_termsand uses it both as a truth value and as metadata.pre_submit_execution.pyLine 283 wraps each lookup inbool(...). A frozen dataclass withhas_required_length: bool, ...,missing_attestation_terms: tuple[str, ...]removes the narrowing and makes the shared contract explicit.♻️ Proposed typed contract
+@dataclass(frozen=True, slots=True) +class AttestationValidationFacts: + """Canonical attestation facts shared by both pre-submission paths.""" + + has_required_length: bool + has_non_generic_text: bool + has_confidentiality_term: bool + has_credential_term: bool + has_source_or_platform_term: bool + missing_attestation_terms: tuple[str, ...] + + def attestation_validation_facts( worker_attestation: str, *, required_terms: Sequence[str] = (), -) -> dict[str, bool | list[str]]: +) -> AttestationValidationFacts:🤖 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/pre_submit_defaults.py` around lines 55 - 76, Replace the heterogeneous return from attestation_validation_facts with a frozen typed dataclass containing the boolean validation fields and missing_attestation_terms as a tuple[str, ...]. Update the function to construct that dataclass and adjust callers such as runner.py and pre_submit_execution.py to access typed attributes directly, preserving truth-value checks and metadata handling without bool() conversions or key-based narrowing.backend/app/modules/artifacts/preparation.py (2)
1213-1218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe version-3 fallback is unreachable and weakens fail-closed reads.
No writer in this module produces a version-3 ledger without
workspaces.__init__writes all three keys, and_write_ledgervalidates the complete key set before writing. This branch therefore only accepts a malformed version-3 document instead of rejecting it.Restrict the compatibility allowance to version 2 so a truncated version-3 ledger fails closed.
♻️ Proposed narrowing of the compatibility branch
- if ( - isinstance(ledger, dict) - and ledger.get("version") == _LEDGER_VERSION - and set(ledger) == {"version", "reservations"} - ): - ledger = {**ledger, "workspaces": []} if isinstance(ledger, dict) and ledger.get("version") == 2:🤖 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/artifacts/preparation.py` around lines 1213 - 1218, Update the compatibility branch around the ledger shape check to apply only to version 2 documents, not _LEDGER_VERSION/version 3. Preserve the existing addition of an empty workspaces list for valid version-2 ledgers, while allowing malformed version-3 ledgers to fail closed.
1699-1699: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the processor as a Protocol instead of
Any.This method requires
process(reader, workspace)andabort(). TheAnyannotation hides that contract, so a processor withoutabortfails only at the moment of cancellation or deadline expiry, which is the least testable path.Declare a
PreparedSubmissionProcessorProtocol next to the existingPreparedArtifactInspectorandPreparedGuideExtractorports, then annotate the parameter with it.♻️ Proposed typed port
Add the port beside the other prepared-artifact ports:
class PreparedSubmissionProcessor(Protocol[_InspectionResultCo]): """Project one prepared submission inside a scratch workspace.""" def process(self, reader: BinaryIO, workspace: Path) -> _InspectionResultCo: """Return bounded results without retaining either capability.""" def abort(self) -> None: """Deny checker access after cancellation or deadline expiry."""Then narrow the parameter:
async def _process_prepared_submission( self, prepared: PreparedArtifact, - processor: Any, + processor: PreparedSubmissionProcessor[_InspectionResult], *, reserved_bytes: int, maximum_entries: int, ) -> _InspectionResult:🤖 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/artifacts/preparation.py` at line 1699, Replace the Any annotation on the processor parameter in the surrounding preparation method with a new PreparedSubmissionProcessor Protocol. Define the Protocol beside PreparedArtifactInspector and PreparedGuideExtractor, including process(reader, workspace) returning the inspection result type and abort() returning None, then use that Protocol for the parameter.backend/app/modules/artifacts/submission_materialization.py (1)
113-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming the sealed pairing explicitly.
PreparedBundleMaterializationServicecallsArtifactPreparationService._process_prepared_submission, a private method of another module. The preparation side documents this as the only authority-gated caller and re-validates handle ownership, so the coupling is deliberate. Keeping the method private also keeps the byte-access surface narrow, so do not make it public.Add a short comment here that records the intentional friend-class relationship. A future reader will otherwise treat this call as an encapsulation mistake and try to widen the API.
🤖 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/artifacts/submission_materialization.py` around lines 113 - 118, Add a concise comment immediately above the _process_prepared_submission call in PreparedBundleMaterializationService documenting the intentional friend-class relationship with ArtifactPreparationService, including that this is the sole authority-gated caller and that the private API must remain private. Do not change method visibility or surrounding behavior.backend/app/modules/artifacts/submission_archive.py (1)
359-374: 🚀 Performance & Scalability | 🔵 TrivialTrack the resident-memory ceiling of sealed projection.
_seal_projected_contentsnapshots every projected file into memory before the callback runs. The aggregate is bounded byartifact_submission_zip_maximum_expanded_bytes, whose default and ceiling are both 512 MiB, andartifact_scratch_maximum_concurrencydefaults to 4. Worst-case resident bytes therefore reach roughly 2 GiB per process.The design is deliberate: it removes filesystem authority from checker adapters, and the workspace reservation already charges the same expanded bytes against the scratch quota. Before AUTH activation, size the API and worker containers against this ceiling and add a memory alert. If a later checker needs only a few files, consider lazy per-path reads through the retained root descriptor instead of a full snapshot.
🤖 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/artifacts/submission_archive.py` around lines 359 - 374, Track the resident-memory ceiling created by _seal_projected_content: use artifact_submission_zip_maximum_expanded_bytes multiplied by artifact_scratch_maximum_concurrency to size both API and worker containers, and add a memory alert covering that aggregate bound. Preserve the full snapshot behavior and document the resulting ceiling before AUTH activation.
🤖 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-ART-001-immutable-artifact-storage/DISCOVERY.md:
- Around line 352-357: The authorization tests should use approved AUTH test
support instead of the locally defined _AllowAuthority in
test_default_pre_submit_execution.py. Replace or document that local double with
an existing AUTH fixture or capability, while keeping
DenyPreSubmitMaterializationAuthorization imported from
app.modules.artifacts.submission_materialization for the denial path.
In
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-pr-trust-bundle.md:
- Line 102: Update the CodeRabbit review status in the PR trust-bundle record to
state that no CodeRabbit findings are available because the service was
externally rate-limited and the review did not complete, replacing the
implication that the review completed without actionable comments.
In @.github/workflows/backend.yml:
- Around line 148-151: Update the workflow step containing the printf command so
its output redirection remains on the same command, writing the job_start_epoch
value directly to job-start-epoch.txt. Keep the existing lane-specific directory
setup and ensure printf does not emit the epoch only to the job log.
In `@backend/app/modules/checkers/pre_submit_defaults.py`:
- Around line 39-52: Update attestation_term_is_satisfied so a normalized term
with no non-ignored words is not treated as satisfied by all attestations;
require at least one required word before applying the all(...) check,
preserving the legacy rule for terms such as “the exclusion.”
In `@backend/app/modules/checkers/pre_submit_execution.py`:
- Around line 198-206: Update the dependency validation around the unmet
computation in the pre-submit execution flow to distinguish missing statuses for
dependencies outside _EXECUTED_PHASES from dependencies that are merely not yet
run. Reject unknown or out-of-slice dependencies by raising
PreSubmissionInfrastructureUnavailable, while preserving the existing
DEPENDENCY_NOT_RUN behavior for valid dependencies whose execution has not
occurred.
- Around line 207-232: Move the disabled-mandatory check and the duplicate
definition identity check to the beginning of the loop handling each entry,
before evaluating blocked/unmet dependencies or calling _dispatch. Ensure
disabled mandatory entries always raise PreSubmissionInfrastructureUnavailable,
and duplicate entry.definition_id values are rejected before any checker
executes; then retain the existing dependency, disabled-advisory, and dispatch
result handling for valid entries.
In `@backend/tests/test_checker_materialization.py`:
- Around line 190-209: Extend
test_workspace_cleanup_bound_is_separate_from_prepared_file_limit to create a
nested directory chain whose depth reaches the workspace entry limit, placing an
entry at each level and using directory-relative operations as needed. Keep the
existing cleanup assertion and ensure the workspace is fully removed after
context exit, proving recursive cleanup at the maximum nested layout.
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 140-149: Expand the assertions in the test using
_AllowAuthority.consume to verify all eleven required materializer facts, plus
the received action ID and service identity, rather than only
prepared_generation_id. Update _handle so it creates PreparedAuthorizationHandle
through its real constructor, or replace it with a typed fake that preserves and
exercises the handle contract instead of bypassing initialization with
object.__new__.
In `@docs/roadmap_status.md`:
- Around line 92-96: Move the 04B2 hidden contributor-ZIP execution entry from
“Implemented On main” to “Integration In Progress” in the roadmap status ledger,
preserving its existing description. Do not mark it implemented until PR `#282`
has merged and the hosted gates and human merge are complete.
In `@docs/spec_artifact_storage_service.md`:
- Line 890: Rewrite the sealed-tree sentence in
docs/spec_artifact_storage_service.md to present the events as a clear sequence:
the tree is destroyed first, then only bounded, path-redacted platform/default
results return. Preserve the existing meaning and terminology.
---
Outside diff comments:
In
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md:
- Around line 141-146: Update the CI coverage command for core modules in the
default checker execution instructions to include app/core/config.py alongside
app/core/cancellation.py and app/core/file_locks.py, matching the local
verification command. Keep the existing coverage threshold and other gate
entries unchanged.
In `@backend/app/modules/checkers/runner.py`:
- Around line 773-779: Update the low-quality artifact check around
matched_low_quality_patterns and the _warning metadata construction so
matched_categories remains a list in the legacy declaration order, rather than
exposing the helper’s sorted tuple. Preserve the existing matched-pattern count
and ensure this shape applies to outcomes before 05B.
---
Nitpick comments:
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 1213-1218: Update the compatibility branch around the ledger shape
check to apply only to version 2 documents, not _LEDGER_VERSION/version 3.
Preserve the existing addition of an empty workspaces list for valid version-2
ledgers, while allowing malformed version-3 ledgers to fail closed.
- Line 1699: Replace the Any annotation on the processor parameter in the
surrounding preparation method with a new PreparedSubmissionProcessor Protocol.
Define the Protocol beside PreparedArtifactInspector and PreparedGuideExtractor,
including process(reader, workspace) returning the inspection result type and
abort() returning None, then use that Protocol for the parameter.
In `@backend/app/modules/artifacts/submission_archive.py`:
- Around line 359-374: Track the resident-memory ceiling created by
_seal_projected_content: use artifact_submission_zip_maximum_expanded_bytes
multiplied by artifact_scratch_maximum_concurrency to size both API and worker
containers, and add a memory alert covering that aggregate bound. Preserve the
full snapshot behavior and document the resulting ceiling before AUTH
activation.
In `@backend/app/modules/artifacts/submission_materialization.py`:
- Around line 113-118: Add a concise comment immediately above the
_process_prepared_submission call in PreparedBundleMaterializationService
documenting the intentional friend-class relationship with
ArtifactPreparationService, including that this is the sole authority-gated
caller and that the private API must remain private. Do not change method
visibility or surrounding behavior.
In `@backend/app/modules/checkers/pre_submit_defaults.py`:
- Around line 55-76: Replace the heterogeneous return from
attestation_validation_facts with a frozen typed dataclass containing the
boolean validation fields and missing_attestation_terms as a tuple[str, ...].
Update the function to construct that dataclass and adjust callers such as
runner.py and pre_submit_execution.py to access typed attributes directly,
preserving truth-value checks and metadata handling without bool() conversions
or key-based narrowing.
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 406-407: Update the parameterized test around
service.materialize_prepared_bundle to provide a case-specific match pattern in
pytest.raises for stale_entry, duplicate, and unknown. Use each expected
exception message so the test verifies the distinct guard path rather than only
the shared PreSubmissionInfrastructureUnavailable exception type.
- Around line 608-610: Replace the private-state deadline mutations in both
affected tests with public configuration: extend the ArtifactPreparationLimits
produced by _limits() with a small total_deadline_seconds value, then configure
ArtifactPreparationService through that limits path so the timeout is exercised
without accessing preparation._active or request.prepared_artifact._binding.
🪄 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: 4dfda595-be5a-4feb-9cfe-df631e940156
📒 Files selected for processing (35)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/CHUNK_MAP.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DISCOVERY.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-pr-trust-bundle.md.github/workflows/backend.ymlbackend/app/adapters/artifacts/__init__.pybackend/app/core/config.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/preparation.pybackend/app/modules/artifacts/sources.pybackend/app/modules/artifacts/submission_archive.pybackend/app/modules/artifacts/submission_materialization.pybackend/app/modules/checkers/pre_submit_defaults.pybackend/app/modules/checkers/pre_submit_execution.pybackend/app/modules/checkers/runner.pybackend/scripts/run_test_lanes.pybackend/tests/test_artifact_cleanup_wiring.pybackend/tests/test_checker_materialization.pybackend/tests/test_config.pybackend/tests/test_default_pre_submit_execution.pydocs/architecture_checker_framework.mddocs/architecture_data_model.mddocs/architecture_lockdown.mddocs/glossary.mddocs/operations_authorization_service.mddocs/roadmap_status.mddocs/spec_artifact_storage_service.mddocs/spec_authorization_service.mddocs/template_checker_policy.mddocs/template_submission_artifact_policy.mddocs/template_submission_packet.md
| - Prepared authorization is consumed through the existing opaque handle at the | ||
| hidden fixed-service composition seam before any prepared-byte read, ZIP open, | ||
| workspace reservation, or checker fact. Until XINT-06A activates the action | ||
| owned in the catalogue by `AUTH_ART_04B`, production composition remains fail | ||
| closed; tests use existing AUTH fixtures/capabilities, never a production | ||
| ART-local fake protocol. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'(_AllowAuthority|DenyPreSubmitMaterializationAuthorization)' \
backend/tests/test_default_pre_submit_execution.py backend/appRepository: Flow-Research/workstream
Length of output: 20462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- test imports and local auth classes ---\n'
sed -n '1,45p' backend/tests/test_default_pre_submit_execution.py
sed -n '135,150p' backend/tests/test_default_pre_submit_execution.py
printf '\n--- all _AllowAuthority references ---\n'
rg -n -C 4 '_AllowAuthority' backend/tests test backend || true
printf '\n--- DenyPreSubmitMaterializationAuthorization references ---\n'
rg -n -C 5 'DenyPreSubmitMaterializationAuthorization' backend/app backend/tests || true
printf '\n--- production deny class body ---\n'
sed -n '56,85p' backend/app/modules/artifacts/submission_materialization.py
sed -n '600,630p' backend/app/modules/artifacts/submission_materialization.py 2>/dev/null || trueRepository: Flow-Research/workstream
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test imports and local auth classes ---'
sed -n '1,45p' backend/tests/test_default_pre_submit_execution.py
sed -n '135,150p' backend/tests/test_default_pre_submit_execution.py
printf '%s\n' ''
printf '%s\n' '--- all _AllowAuthority references ---'
rg -n -C 4 '_AllowAuthority' backend/tests backend || true
printf '%s\n' ''
printf '%s\n' '--- DenyPreSubmitMaterializationAuthorization references ---'
rg -n -C 5 'DenyPreSubmitMaterializationAuthorization' backend/app backend/tests || true
printf '%s\n' ''
printf '%s\n' '--- production deny class body ---'
sed -n '56,90p' backend/app/modules/artifacts/submission_materialization.py
printf '%s\n' ''
printf '%s\n' '--- tests requesting allow authorization calls/consume ---'
rg -n -C 5 'consume|facts|faced|service_identity|AuthorizationError|ArtifactAuthorityDeniedError|PreSubmitMaterializationAuthorityFacts' backend/tests/test_default_pre_submit_execution.py || trueRepository: Flow-Research/workstream
Length of output: 33514
Use approved AUTH test support for _AllowAuthority.
_AllowAuthority is defined in backend/tests/test_default_pre_submit_execution.py instead of using an approved AUTH fixture or capability, so allow-authority success paths exercise a local authorization implementation rather than AUTH test support. Replace the local double or document an approved AUTH test double.
Also keep DenyPreSubmitMaterializationAuthorization on the production code boundary: the denial test imports the production implementation from app.modules.artifacts.submission_materialization, not an AUTH test double.
🤖 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-ART-001-immutable-artifact-storage/DISCOVERY.md
around lines 352 - 357, The authorization tests should use approved AUTH test
support instead of the locally defined _AllowAuthority in
test_default_pre_submit_execution.py. Replace or document that local double with
an existing AUTH fixture or capability, while keeping
DenyPreSubmitMaterializationAuthorization imported from
app.modules.artifacts.submission_materialization for the denial path.
| The first GitHub run found stale AUTH wording, a docstring-floor regression, | ||
| and a failure-path lane-directory assumption. All three are fixed and recorded | ||
| in the external review response; replacement hosted checks are pending. | ||
| CodeRabbit is externally rate-limited and reported no actionable comment. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record the CodeRabbit review as incomplete.
The PR objectives state that CodeRabbit did not complete a review because the service was rate-limited. “Reported no actionable comment” implies that a review completed. State that no CodeRabbit findings are available.
Proposed wording
- CodeRabbit is externally rate-limited and reported no actionable comment.
+ CodeRabbit review was not completed because the service was rate-limited; no CodeRabbit findings are available.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CodeRabbit is externally rate-limited and reported no actionable comment. | |
| CodeRabbit review was not completed because the service was rate-limited; no CodeRabbit findings are available. |
🤖 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-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-pr-trust-bundle.md
at line 102, Update the CodeRabbit review status in the PR trust-bundle record
to state that no CodeRabbit findings are available because the service was
externally rate-limited and the review did not complete, replacing the
implication that the review completed without actionable comments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/test_artifact_preparation.py (1)
1253-1255: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert normalization immediately after reopen.
The assertions at Line [1253] through Line [1255] run only after
allocate()andrelease(). Those operations mutate the ledger, so the test can pass if normalization occurs on the first mutation instead of duringArtifactScratchManager(...). Read and assert.ledger.jsonimmediately after reopening. Keep allocation and release as a separate usability check.Proposed test ordering
reopened = ArtifactScratchManager(root=root, limits=preparation_limits()) + normalized = json.loads(ledger_path.read_text(encoding="utf-8")) + assert normalized["version"] == 3 + assert normalized["workspaces"] == [] + reservation, descriptor = await reopened.allocate() os.close(descriptor) await reopened.release(reservation) - normalized = json.loads(ledger_path.read_text(encoding="utf-8")) - assert normalized["version"] == 3 - assert normalized["workspaces"] == []🤖 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_preparation.py` around lines 1253 - 1255, Update the test setup around ArtifactScratchManager reopening to read and assert the normalized .ledger.json immediately after constructing the manager, before calling allocate() or release(). Keep the existing allocate() and release() operations afterward as a separate usability check, preserving the version and empty-workspaces assertions for the immediate post-reopen state.
🤖 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-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-external-review-response.md:
- Around line 19-23: Update the CodeRabbit review-status statement in the
document to match the still-rate-limited state described later: attribute the
items to reported findings validated on the current head rather than claiming a
completed review. Alternatively, if a completed review artifact exists, record
its artifact reference and commit SHA consistently with the lines describing the
rate-limit status.
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 58-65: Change PreparedSubmissionProcessor.process and abort to
async/awaitable operations, then update every implementation and
caller—including the blocking-worker flow around process()—to await them rather
than expose synchronous checker methods. Preserve blocking archive work only
behind an explicit bounded async adapter, and remove synchronous-first checker
or job contracts and documentation.
---
Nitpick comments:
In `@backend/tests/test_artifact_preparation.py`:
- Around line 1253-1255: Update the test setup around ArtifactScratchManager
reopening to read and assert the normalized .ledger.json immediately after
constructing the manager, before calling allocate() or release(). Keep the
existing allocate() and release() operations afterward as a separate usability
check, preserving the version and empty-workspaces assertions for the immediate
post-reopen state.
🪄 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: 16be3f33-f40f-4728-b275-3299158a6afa
📒 Files selected for processing (17)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DISCOVERY.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-pr-trust-bundle.md.github/workflows/backend.ymlbackend/app/modules/artifacts/preparation.pybackend/app/modules/artifacts/submission_materialization.pybackend/app/modules/checkers/pre_submit_defaults.pybackend/app/modules/checkers/pre_submit_execution.pybackend/app/modules/checkers/runner.pybackend/tests/test_artifact_preparation.pybackend/tests/test_checker_materialization.pybackend/tests/test_checkers.pybackend/tests/test_ci_test_lanes.pybackend/tests/test_default_pre_submit_execution.pydocs/roadmap_status.mddocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/spec_artifact_storage_service.md
- backend/app/modules/artifacts/submission_materialization.py
- backend/tests/test_default_pre_submit_execution.py
- backend/app/modules/checkers/runner.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B2-pr-trust-bundle.md
- docs/roadmap_status.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04B2-default-checker-execution.md
- backend/app/modules/checkers/pre_submit_execution.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DISCOVERY.md
| class PreparedSubmissionProcessor(Protocol[_InspectionResultCo]): | ||
| """Project one prepared submission inside a bounded scratch workspace.""" | ||
|
|
||
| def process(self, reader: BinaryIO, workspace: Path) -> _InspectionResultCo: | ||
| """Return bounded results without retaining either capability.""" | ||
|
|
||
| def abort(self) -> None: | ||
| """Deny checker access after cancellation or deadline expiry.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use an async processor contract.
Lines 61 and 64 define synchronous checker operations. Line 1728 executes process() in a blocking worker. This implements a synchronous-first checker contract. Change the protocol and its implementations to awaitable operations. Keep blocking archive operations behind an explicit bounded async adapter.
As per coding guidelines, “Execution is async-first; do not document or implement 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 `@backend/app/modules/artifacts/preparation.py` around lines 58 - 65, Change
PreparedSubmissionProcessor.process and abort to async/awaitable operations,
then update every implementation and caller—including the blocking-worker flow
around process()—to await them rather than expose synchronous checker methods.
Preserve blocking archive work only behind an explicit bounded async adapter,
and remove synchronous-first checker or job contracts and documentation.
Source: Coding guidelines
2 participants
WS-ART-001-04B2 PR Trust Bundle
Chunk
WS-ART-001-04B2— default checker execution.Goal
Materialize one prepared contributor ZIP only after fixed-service authority,
project its exact 04A semantic manifest into bounded private scratch, and run
only the ordered Workstream platform/default pre-submission slice.
Human-approved intent
Workstream owns the generic default pre-submission checks. Project-specific
rules remain a later phase of the same effective plan and do not execute in
this chunk. Production authority remains planned and unavailable.
What changed
manifest facts, revoked before scratch cleanup completes.
reports, and canonical documentation.
Why it changed
04B1 locked what must run. 04B2 supplies the hidden execution boundary needed
before project-policy execution and durable evidence can be composed safely.
Design chosen
AUTH is consumed before any prepared-byte or workspace access. The canonical
archive inspector alone projects the verified tree. Checker code receives no
path, provider handle, scratch handle, directory fd, or serializable authority.
Cancellation and deadlines abort before checker execution while cleanup still
runs to completion.
Alternatives rejected
PreparedArtifactprocessing: bypassed fixed-service authority.boundary.
replaced and mix project-policy work into 04B2.
Scope control
No public route, project-policy execution, durable evidence, admission,
Submission, review decision, provider I/O, AUTH activation, or legacy removal.
Product behavior
None is publicly active. Results are non-durable internal values:
passed,warning,failed,advisory_disabled, anddependency_not_run; they are not review decisions.Acceptance criteria proof
Tests prove authority-before-access, exact plan/catalogue/archive/manifest
identity, closed dispatch, disabled mandatory failure, advisory visibility,
project-policy isolation, semantic drift rejection, scratch capacity, fixed
modes, callback revocation, cancellation/deadline abort, cleanup, and no legacy
runner dependency.
Tests/checks run
lightweight agent, Markdown-link, stale-wording, and diff checks passed.
Test delta
Two focused modules were added; existing cleanup/config tests were strengthened.
No test was removed, skipped, xfailed, or weakened.
CI integrity
The repository 78 percent floor remains. Existing checker 90 percent coverage
remains, and ART module, cancellation/lock, and artifact-interface 90 percent
reports are explicit. Both new tests are assigned to semantic lanes.
Reviewer results
Architecture, security, QA, senior engineering, reuse, CI integrity, test
delta, and docs passed. Product/ops passed with one non-blocking observation
that the platform/default slice is derived from the locked full plan.
External review
Pending GitHub Backend Gates and CodeRabbit on the draft PR.
Remaining risks
The production materializer remains unavailable until AUTH activation. This
chunk intentionally creates no durable checker evidence or admission.
Follow-up work
04B3 owns project-policy execution and durable checker evidence. Later AUTH
activation and the 05B admission-backed cutover remain separate ordered work.
Human review focus
scratch authority or content after close.
Human merge ownership
Only the repository owner may approve and merge this PR.
Summary by CodeRabbit