WS-ART-001-03B3B1: approve guide parser dependencies - #230
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 (6)
📝 WalkthroughWalkthroughThe change adds a hashed parser dependency manifest, validates dependency and import boundaries, enforces exact-head pull-request approval, wires the checks into CI, and adds focused tests, documentation, and review records. ChangesParser dependency governance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant DependencyChecker
participant Allowlist
participant GitHubReviews
GitHubActions->>DependencyChecker: Run dependency validation
DependencyChecker->>Allowlist: Load and hash manifest
DependencyChecker->>DependencyChecker: Compare base and head allowlist
DependencyChecker->>GitHubReviews: Fetch pull-request reviews
GitHubReviews-->>DependencyChecker: Return review payloads
DependencyChecker-->>GitHubActions: Return gate status
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 |
|
@abiorh-claw The WS-ART-001-03B3B1 dependency allowlist is ready for independent review at exact head |
|
@abiorh-claw CI repair is pushed at exact head |
![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
🧹 Nitpick comments (7)
backend/tests/test_guide_extractor_dependencies.py (2)
306-321: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
test_github_review_fetch_returns_valid_payloaddoes not verify the request sent to GitHub.This test calls the real
gate.fetch_reviews, but theurlopenstub accepts and ignores*_args, **_kwargs. The test confirms thatfetch_reviewsparses a response correctly, but it does not confirm thatfetch_reviewsbuilds the correct URL, HTTP method, or Authorization header for the GitHub reviews endpoint. A regression in request construction (wrong path, missing token header) would not be caught here.Capture the
Requestobject passed tourlopenand assert on itsfull_urlandheaders.🤖 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_guide_extractor_dependencies.py` around lines 306 - 321, Update test_github_review_fetch_returns_valid_payload to capture the Request argument received by the urlopen stub instead of ignoring it, then assert that the request full_url targets the expected repository, pull request, and reviews endpoint and that its headers contain the correct Authorization token and HTTP method. Keep the existing response parsing assertion.
62-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional indexing into
data["dependencies"]couples tests to manifest ordering.The mutation table uses
data["dependencies"][0]for pypdf-specific cases anddata["dependencies"][2]for the native-wheelpython_versionmismatch (Line 86). If the manifest entries are ever reordered, these tests silently mutate a different package than intended, and a failure message can point to the wrong root cause without the test itself failing to compile or immediately signal the mismatch.Look up the target entry by
nameinstead of by list position.♻️ Proposed fix
+def _dependency(data: dict[str, object], name: str) -> dict[str, object]: + return next(d for d in data["dependencies"] if d["name"] == name) + + `@pytest.mark.parametrize`( ("mutation", "failure"), [ ( - lambda data: data["dependencies"][0].update(version=">=6"), + lambda data: _dependency(data, "pypdf").update(version=">=6"), "guide_dependency_version_unpinned", ), ... ( - lambda data: data["dependencies"][2]["approved_artifacts"][0].update( + lambda data: _dependency(data, "pillow")["approved_artifacts"][0].update( python_version="3.13" ), "guide_dependency_artifact_platform_mismatch", ), ... ], )🤖 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_guide_extractor_dependencies.py` around lines 62 - 119, Update the mutation cases in test_manifest_rejects_unpinned_hash_drifted_or_wrong_scope_entries to locate dependency entries by their name instead of positional indices, including the pypdf and native-wheel cases; keep each mutation targeting the same intended package and preserve the existing failure expectations.backend/scripts/check_guide_extractor_dependencies.py (4)
457-461: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne transient GitHub error blocks the merge.
Any single 5xx response, rate-limit response, or socket error raises
guide_dependency_github_reviews_unavailableand fails the required check. Add a small bounded retry with backoff for retryable statuses, and keep the fail-closed result after the retries.🤖 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/check_guide_extractor_dependencies.py` around lines 457 - 461, Update the GitHub review request flow around the urllib.request.urlopen call to retry transient failures, including 5xx responses, rate-limit responses, and socket/OSError failures, using a small bounded retry count and backoff. After retries are exhausted, preserve the existing fail-closed DependencyGateError("guide_dependency_github_reviews_unavailable") behavior; do not retry non-retryable errors.
371-391: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDynamic imports bypass the parser import boundary.
The AST walk inspects only
ImportandImportFromnodes. A parser module can reach an unapproved package throughimportlib.import_moduleor__import__, becauseimportlibis a stdlib name. Consider rejectingimportlib.import_moduleand__import__calls inside the parser modules.🤖 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/check_guide_extractor_dependencies.py` around lines 371 - 391, The AST validation around the existing Import and ImportFrom handling must also reject dynamic imports in parser modules. Extend the ast.walk checks to detect calls to importlib.import_module and __import__, while retaining the current approved-import validation for static imports and allowing unrelated calls.
274-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNative entries do not need to cover every approved Python version.
Each artifact is checked against
approved_python_versions, but no check requires a wheel for each approved version. A manifest with only a cp311 wheel passes, and a hash-pinned install on 3.12 then fails. Require full coverage per native package.♻️ Proposed check after the artifact loop
+ if entry["native_code"]: + require( + {artifact["python_version"] for artifact in artifacts} + == set(policy["approved_python_versions"]), + "guide_dependency_artifact_python_coverage_incomplete", + ) by_name[canonical_name] = entry🤖 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/check_guide_extractor_dependencies.py` around lines 274 - 290, Update the native artifact validation around the native-entry loop to require every native package to provide a wheel for each version in policy["approved_python_versions"], while preserving the existing filename and platform checks. Track the versions encountered per native package and add a post-loop validation that rejects incomplete coverage, using the existing guide_dependency_artifact_platform_mismatch error.
301-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the keyword form for
maxsplitin_dependency_name.
re.splitacceptsmaxsplitpositionally now, but CPython 3.13 treats it as deprecated. Passmaxsplit=1so the backend script is not flagged in future Python versions.♻️ Proposed change
- return normalize_package_name(re.split(r"[<>=!~\s;\[]", requirement.strip(), 1)[0]) + return normalize_package_name( + re.split(r"[<>=!~\s;\[]", requirement.strip(), maxsplit=1)[0] + )🤖 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/check_guide_extractor_dependencies.py` around lines 301 - 302, Update the re.split call in _dependency_name to pass the split limit with the maxsplit=1 keyword argument instead of positionally, preserving the existing parsing behavior.backend/config/guide_extractor_dependencies.json (1)
74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApproved artifacts cover only manylinux x86_64.
require_hashesistrueand only x86_64 manylinux Pillow wheels are approved. When parser code declares the hash-bound direct reference, installation fails on macOS and on arm64 Linux. Decide now whether contributor and arm64 environments need approved wheels, or record that only the hosted x86_64 runner installs parser dependencies.Also applies to: 109-118
🤖 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/config/guide_extractor_dependencies.json` around lines 74 - 84, Update the Pillow artifacts in the dependency guide so the hash-bound approvals support every required contributor and runtime platform, including macOS and arm64 Linux, rather than only manylinux x86_64. Add the corresponding platform-specific wheel entries with their exact filenames, Python versions, SHA-256 hashes, and URLs; if those environments are intentionally unsupported, explicitly document that the parser dependencies install only on the hosted x86_64 runner.
🤖 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-03B3B1-external-review-response.md:
- Around line 16-20: Document that fresh independent approval of the repaired
final PR head is pending in the “Human decisions needed” section of
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B1-external-review-response.md
(lines 16-20), replacing “None”; in
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md (lines
70-74), replace “exact allowlist head” with “exact final PR head” so both
records require approval on the repaired final head.
In @.github/workflows/backend.yml:
- Around line 63-78: Update the “Guide extractor dependency approval” workflow
step so pull_request_review events validate the pull-request head checkout
rather than the default-branch ref. Ensure the referenced WORKSTREAM_PR_BASE_SHA
and WORKSTREAM_PR_HEAD_SHA commits are available, including for fork PRs, before
invoking check_guide_extractor_dependencies.py with --require-pr-approval;
alternatively restrict this step to pull_request events and preserve separate
approval enforcement for review events.
In `@backend/scripts/check_guide_extractor_dependencies.py`:
- Around line 233-236: Update validate_manifest at
backend/scripts/check_guide_extractor_dependencies.py lines 233-236 so
release_uploaded_at is passed through _string with
guide_dependency_release_timestamp_invalid before calling endswith("Z"). Apply
the same validation at lines 259-262 for artifact["sha256"], using _string with
guide_dependency_artifact_hash_invalid before SHA256_PATTERN.fullmatch.
- Around line 413-436: Update the review aggregation before the candidate filter
so latest_by_reviewer is updated only for state-bearing reviews, excluding
COMMENTED entries that should not replace an existing approval. Preserve the
existing per-reviewer deduplication and exact-head approval checks in the
candidates loop, ensuring a later non-state-bearing comment cannot erase a valid
APPROVED review.
---
Nitpick comments:
In `@backend/config/guide_extractor_dependencies.json`:
- Around line 74-84: Update the Pillow artifacts in the dependency guide so the
hash-bound approvals support every required contributor and runtime platform,
including macOS and arm64 Linux, rather than only manylinux x86_64. Add the
corresponding platform-specific wheel entries with their exact filenames, Python
versions, SHA-256 hashes, and URLs; if those environments are intentionally
unsupported, explicitly document that the parser dependencies install only on
the hosted x86_64 runner.
In `@backend/scripts/check_guide_extractor_dependencies.py`:
- Around line 457-461: Update the GitHub review request flow around the
urllib.request.urlopen call to retry transient failures, including 5xx
responses, rate-limit responses, and socket/OSError failures, using a small
bounded retry count and backoff. After retries are exhausted, preserve the
existing fail-closed
DependencyGateError("guide_dependency_github_reviews_unavailable") behavior; do
not retry non-retryable errors.
- Around line 371-391: The AST validation around the existing Import and
ImportFrom handling must also reject dynamic imports in parser modules. Extend
the ast.walk checks to detect calls to importlib.import_module and __import__,
while retaining the current approved-import validation for static imports and
allowing unrelated calls.
- Around line 274-290: Update the native artifact validation around the
native-entry loop to require every native package to provide a wheel for each
version in policy["approved_python_versions"], while preserving the existing
filename and platform checks. Track the versions encountered per native package
and add a post-loop validation that rejects incomplete coverage, using the
existing guide_dependency_artifact_platform_mismatch error.
- Around line 301-302: Update the re.split call in _dependency_name to pass the
split limit with the maxsplit=1 keyword argument instead of positionally,
preserving the existing parsing behavior.
In `@backend/tests/test_guide_extractor_dependencies.py`:
- Around line 306-321: Update test_github_review_fetch_returns_valid_payload to
capture the Request argument received by the urlopen stub instead of ignoring
it, then assert that the request full_url targets the expected repository, pull
request, and reviews endpoint and that its headers contain the correct
Authorization token and HTTP method. Keep the existing response parsing
assertion.
- Around line 62-119: Update the mutation cases in
test_manifest_rejects_unpinned_hash_drifted_or_wrong_scope_entries to locate
dependency entries by their name instead of positional indices, including the
pypdf and native-wheel cases; keep each mutation targeting the same intended
package and preserve the existing failure expectations.
🪄 Autofix (Beta)
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: 9f0d6003-de29-4f5c-9338-6c67f9760bec
📒 Files selected for processing (11)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.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-03B3B1-parser-dependency-approval.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B1-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B1-pr-trust-bundle.md.github/workflows/backend.ymlbackend/config/guide_extractor_dependencies.jsonbackend/scripts/check_guide_extractor_dependencies.pybackend/scripts/run_test_lanes.pybackend/tests/test_guide_extractor_dependencies.pydocs/spec_artifact_storage_service.md
| - name: Guide extractor dependency approval | ||
| working-directory: backend | ||
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
| WORKSTREAM_PR_AUTHOR: ${{ github.event.pull_request.user.login }} | ||
| WORKSTREAM_PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| WORKSTREAM_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| WORKSTREAM_PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| approval_args=() | ||
| if [[ "${GITHUB_EVENT_NAME}" != "push" ]]; then | ||
| approval_args+=(--require-pr-approval) | ||
| fi | ||
| python scripts/check_guide_extractor_dependencies.py "${approval_args[@]}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect workflow triggers, checkout inputs, and identity assertions for event coupling.
set -uo pipefail
fd -t f 'backend.yml' .github/workflows --exec rg -n -C4 'on:|pull_request|checkout|ref:|fetch-depth|GITHUB_SHA|tree_sha|require-pr-approval'Repository: Flow-Research/workstream
Length of output: 3403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Candidate files"
fd -t f 'check_guide_extractor_dependencies.py|guide_extractor|guide.*dependency|dependency' .
echo
echo "## workflows backend relevant lines"
sed -n '36,86p;250,290p;420,435p' .github/workflows/backend.yml
echo
echo "## scripts outline"
for f in $(fd -t f 'check_guide_extractor_dependencies.py' .); do
echo "--- $f"
wc -l "$f"
sed -n '1,260p' "$f"
done
echo
echo "## Diff semantics references in repo"
rg -n "allowlist_changed|git diff|guide_dependency_git_diff_failed|pull request head|head_sha|base_sha|require-pr-approval|dependency" scripts .github/workflows backend -SRepository: Flow-Research/workstream
Length of output: 50047
On pull_request_review, the dependency approval gate checks out the default branch while comparing against the pull-request head.
pull_request_review sets ${{ github.sha }} to the default-branch ref, but this job enables --require-pr-approval for ${{ github.event.pull_request.head.sha }} and the script runs git diff ${base_sha}...${head_sha} -- guide_extractor_dependencies.json. The approved head_sha can pass while the checked-out workflow actually validates default-branch artifacts.
For a fork PR, the head commit is missing from the clone, so git diff base...head -- guide_extractor_dependencies.json exits non-zero and the script fails with guide_dependency_git_diff_failed.
Use the pull-request head checkout for review events, or restrict this step to pull_request events and enforce approval by head separately.
🤖 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 @.github/workflows/backend.yml around lines 63 - 78, Update the “Guide
extractor dependency approval” workflow step so pull_request_review events
validate the pull-request head checkout rather than the default-branch ref.
Ensure the referenced WORKSTREAM_PR_BASE_SHA and WORKSTREAM_PR_HEAD_SHA commits
are available, including for fork PRs, before invoking
check_guide_extractor_dependencies.py with --require-pr-approval; alternatively
restrict this step to pull_request events and preserve separate approval
enforcement for review events.
|
@abiorh-claw Final CodeRabbit repair is pushed at exact head |
…to codex/ws-art-001-03b3b1-parser-dependency-approval
2 participants
WS-ART-001-03B3B1 PR Trust Bundle
Chunk
WS-ART-001-03B3B1— Parser Dependency Approval.Goal
Approve and enforce the smallest exact PDF, OOXML-safety, and image-metadata
dependency set without installing packages or changing runtime behavior.
Human-approved intent
The merged 03B3B split contract requires dependency approval before any PDF,
OOXML, or image parser implementation. The human owner explicitly started this
chunk after PR #228 merged.
What changed
pypdf==6.14.2,defusedxml==0.7.1, andPillow==12.3.0, including exact approved wheel URLs and SHA-256 hashes.hash-bound future declarations, prohibited packages, format-scoped imports,
and independent exact-head GitHub approval.
Why it changed
Untrusted document parsers are a supply-chain boundary. A package name or
version alone cannot prove which bytes will later be installed, and a
repository-authored approval file cannot independently authorize itself.
Design chosen
pypdf.defusedxmlplus later ART-owned bounded containerand format adapters.
Pillow, restricted to two named Python 3.11/3.12manylinux x86_64 wheels.
SHA-256 fragment. Optional/dependency-group parser declarations are denied.
independent human owner/member/collaborator on the exact PR head.
Alternatives rejected
python-docx,python-pptx,openpyxl,lxml, andXlsxWriter: larger,unnecessary graphs for the bounded v0.1 OOXML extraction design.
pull_request_target: an unnecessary privileged workflow boundary.Scope control
No
pyproject.toml, lockfile, dependency install, application import, parseradapter, AUTH, Celery, submission, or product lifecycle change is included.
Product behavior
None. Project Manager guide-source handling remains distinct from contributor
one-ZIP submission handling. All parser behavior remains unimplemented.
Acceptance criteria proof
transitive graph, native-code, malformed-input, network, cancellation,
import, format, source, wheel URL, and hash facts.
optional-group bypasses, cross-format imports, self/bot/stale/dismissed
approvals, and GitHub API failure all reject in focused tests.
Tests/checks run
and
git diff --check: pass.Test delta
One new focused test module; no tests removed, skipped, or weakened.
CI integrity
The gate runs before package installation. Existing backend tests, semantic
lanes, provider proof, and 78/90 percent coverage floors are unchanged. Token
permissions remain read-only:
contents: readandpull-requests: read.Reviewer results
Architecture, security, QA, product/ops, senior engineering, CI integrity,
reuse/dedup, and test delta pass after valid findings were repaired. Docs
review is rerun on the staged evidence.
External review
CodeRabbit and hosted GitHub checks have not run yet. Their exact PR-head
results remain required external evidence.
Remaining risks
accepted low operational cost for the v0.1 fail-closed required check.
defusedxmlis a stable, slow-release security utility; its isolation andbounded OOXML use remain mandatory in later chunks.
Follow-up work
After this chunk merges,
WS-ART-001-03B3B2may install only the approved PDFwheel and implement bounded PDF extraction. Later OOXML and image chunks remain
separate and require explicit starts.
Human review focus
Confirm the three-package minimum, wheel URL/hash selection, native Pillow
platform bounds, prohibited package set, and independent exact-head approval
semantics.
Human merge ownership
Only the human owner may approve and merge this PR. The PR must receive an
independent qualifying GitHub approval on its exact final head; this agent will
not merge it.
Summary by CodeRabbit
Security
Reliability
Testing
Documentation