feat(artifacts): add bounded guide text extraction - #225
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (9)
📝 WalkthroughWalkthroughThis change adds bounded Linux subprocess extraction for text, Markdown, JSON, and CSV artifacts, with deterministic canonicalization, immutable lineage evidence, retry budgeting, scratch-workspace cleanup, a new migration, and extensive security, schema, integration, and lifecycle tests. ChangesGuide extraction framework
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
backend/app/modules/artifacts/guide_extraction_service.py (2)
234-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd deterministic ordering to the replay select.
.limit(1)withoutORDER BYis non-deterministic once a binding has more than one usage row (the unique constraint is(binding_id, extracted_content_id), so multiple formats/extractor versions can coexist). Replay is specified as deterministic.♻️ Proposed change
.where(GuideSourceExtractionUsage.binding_id == facts.binding_id) + .order_by( + GuideSourceExtractionUsage.created_at.asc(), + GuideSourceExtractionUsage.id.asc(), + ) .limit(1)🤖 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/guide_extraction_service.py` around lines 234 - 259, Add an explicit ORDER BY to the replay query in the successful lookup before .limit(1), using the established attempt/usage sequencing fields to select the same deterministic preferred record every time. Keep the existing joins, binding filter, limit, and replay result handling unchanged.
269-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the policy version into a module constant.
"guide-extraction-v1"is hardcoded at lines 270, 290, 300 and again in_persist_failure(line 427), whileextract_preparedtakes it fromextracted.policy_version. A single constant (ideally sourced from the registry/extractor) avoids the two sources drifting on the next policy bump.🤖 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/guide_extraction_service.py` around lines 269 - 300, Define one module-level policy-version constant, ideally sourced from the registry or extractor, and replace every hardcoded "guide-extraction-v1" in the retry-budget/latest-attempt logic and _persist_failure with it. Ensure extract_prepared uses the same constant rather than independently taking a potentially divergent policy version from extracted.policy_version.backend/tests/test_alembic.py (1)
1632-1632: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a module-level
HEAD_REVISIONconstant.Each migration forces the same literal edit in eight places; a single constant would confine future churn to one line.
Also applies to: 1956-1956, 2083-2083, 2480-2480, 2692-2692, 2719-2719, 2747-2747, 2807-2807
🤖 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_alembic.py` at line 1632, Define a module-level HEAD_REVISION constant in the migration test module and replace the repeated "0041_guide_extraction" literals in all eight affected revision assertions with that constant.backend/tests/test_guide_extraction.py (1)
429-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer keyword arguments for
GuideExtractionRequest.
*(uuid4() for _ in range(5)), 1, uuid4(), uuid4()silently keeps passing if the dataclass fields are reordered, which would make these lineage-sensitive tests assert the wrong thing.Also applies to: 478-478
🤖 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_extraction.py` at line 429, Update the GuideExtractionRequest constructions in the tests around the shown locations to use explicit keyword arguments for every field instead of positional values. Preserve the existing UUID and numeric test values while mapping each value to the correct dataclass field, so field reordering cannot silently alter the test scenario.backend/app/modules/artifacts/preparation.py (1)
417-439: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_cleanup_workspace_synccan't remove non-empty subdirectories, and a cleanup failure discards an already-successful extraction result.The loop only handles flat entries: a directory entry is removed with
os.rmdir(entry, dir_fd=workspace_fd)(Line 433), which raisesOSError("Directory not empty") if that entry itself contains files. On failure, the workspace name is added to_pending_workspacesand the exception is re-raised (preparation.pyL1434-1437) — and because this happens inside afinallyininspect_and_cleanup, it replaces any value already returned by a successfulextractor.inspect(...)call, silently discarding a completed extraction.Today this is unreachable because
guide_extraction_worker.py's seccomp allow-list has noopen/openat/mkdir/mkdirat, so the shipped worker can't write anything into the workspace. ButPreparedGuideExtractoris a generic protocol, not scoped to that sandboxed implementation, so any other extractor that leaves nested residue would permanently fail cleanup (repeated retries hit the same non-empty-directory error) and lose successful results in the process. Tests only cover a flat-file residue case (DirtyRunner), not nested residue.Recommend making the removal recursive (walk
dir_fd-relative subdirectories bottom-up) so this holds for any future extractor implementation.🤖 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 417 - 439, Update _cleanup_workspace_sync to recursively remove nested workspace contents using dir_fd-relative traversal, processing child directories bottom-up before removing their parents, while preserving no-follow-links and entry-limit safeguards. Ensure cleanup failures in inspect_and_cleanup do not replace a successfully returned extractor.inspect result; retain pending-workspace handling for cleanup errors.
🤖 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/artifacts/guide_extraction_service.py`:
- Around line 119-129: Update the CancelledError recovery path in the extraction
service to run the cancellation persistence through a bounded timeout and shield
it from repeated cancellation. Catch and swallow secondary cancellation or
timeout errors from that recovery write, then re-raise the original cancellation
after the bounded attempt completes.
- Around line 166-201: The insertion path in the extraction flow around
GuideSourceExtractedContent must handle concurrent identity collisions
deterministically. Replace the unconditional session.add/flush with an ON
CONFLICT DO NOTHING insert, then re-select the existing row when the insert is
skipped, set replayed=True for that path, and let the existing source/output
equality checks determine whether to replay or raise GuideExtractionError.
In `@backend/app/modules/artifacts/guide_extraction_worker.py`:
- Around line 61-91: The _install_seccomp function currently filters only the
native syscall ABI; explicitly add and validate seccomp_arch_add configuration
for every ABI reachable in the deployment, including compat/x32 where
applicable, before installing allow-list rules. If those ABIs are not supported,
disable them before the untrusted parser executes, and treat any
architecture-configuration failure as parser isolation_unavailable.
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 1404-1446: The extraction workspace lifecycle lacks durable
crash-recovery tracking. Update extract_prepared_guide and the workspace
initialization/cleanup paths such as _initialize_layout_locked, cleanup_stale,
and _cleanup_stale_sync to record each extract_* workspace with a TTL before
creation, then remove stale ledger entries and directories during startup/stale
cleanup; preserve the existing finally cleanup and _pending_workspaces fallback
for active failures.
In `@backend/tests/fixtures/guide_extraction_probe_worker.py`:
- Around line 39-51: Extend the "__isolation_probe__" mapping in the guide
extraction probe worker to attempt and deny a write outside the
scratch/workspace directory, alongside the existing workspace_write check. Add
the corresponding expected key and denied result to the isolation assertions in
test_guide_extraction.py.
In `@backend/tests/test_artifact_architecture.py`:
- Around line 529-538: Update the import collection logic in the architecture
test around the AST walk so relative ImportFrom nodes (node.level > 0),
including those with node.module unset, are resolved against the current
module/package context before applying forbidden_prefixes. Ensure relative
imports such as from ..adapters and from . import adapters are normalized to
their fully qualified app.* names and rejected when they cross the boundary,
while preserving existing absolute-import checks.
---
Nitpick comments:
In `@backend/app/modules/artifacts/guide_extraction_service.py`:
- Around line 234-259: Add an explicit ORDER BY to the replay query in the
successful lookup before .limit(1), using the established attempt/usage
sequencing fields to select the same deterministic preferred record every time.
Keep the existing joins, binding filter, limit, and replay result handling
unchanged.
- Around line 269-300: Define one module-level policy-version constant, ideally
sourced from the registry or extractor, and replace every hardcoded
"guide-extraction-v1" in the retry-budget/latest-attempt logic and
_persist_failure with it. Ensure extract_prepared uses the same constant rather
than independently taking a potentially divergent policy version from
extracted.policy_version.
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 417-439: Update _cleanup_workspace_sync to recursively remove
nested workspace contents using dir_fd-relative traversal, processing child
directories bottom-up before removing their parents, while preserving
no-follow-links and entry-limit safeguards. Ensure cleanup failures in
inspect_and_cleanup do not replace a successfully returned extractor.inspect
result; retain pending-workspace handling for cleanup errors.
In `@backend/tests/test_alembic.py`:
- Line 1632: Define a module-level HEAD_REVISION constant in the migration test
module and replace the repeated "0041_guide_extraction" literals in all eight
affected revision assertions with that constant.
In `@backend/tests/test_guide_extraction.py`:
- Line 429: Update the GuideExtractionRequest constructions in the tests around
the shown locations to use explicit keyword arguments for every field instead of
positional values. Preserve the existing UUID and numeric test values while
mapping each value to the correct dataclass field, so field reordering cannot
silently alter the test scenario.
🪄 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: e322290c-0268-48d5-8c5a-21238a291874
📒 Files selected for processing (22)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DECISIONS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3A-extraction-framework-text.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-pr-trust-bundle.md.agent-loop/merge-intents/WS-ART-001-03B3A.jsonbackend/alembic/versions/0041_guide_extraction.pybackend/app/db/models.pybackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/preparation.pybackend/app/modules/artifacts/sources.pybackend/scripts/run_test_lanes.pybackend/tests/conftest.pybackend/tests/fixtures/guide_extraction_probe_worker.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pydocs/spec_artifact_storage_service.md
| def _install_seccomp() -> None: | ||
| try: | ||
| library = ctypes.CDLL("libseccomp.so.2", use_errno=True) | ||
| except OSError as exc: | ||
| raise ExtractionFailure("parser_failure", "isolation_unavailable") from exc | ||
| library.seccomp_init.argtypes = [ctypes.c_uint32] | ||
| library.seccomp_init.restype = ctypes.c_void_p | ||
| library.seccomp_rule_add.argtypes = [ | ||
| ctypes.c_void_p, | ||
| ctypes.c_uint32, | ||
| ctypes.c_int, | ||
| ctypes.c_uint, | ||
| ] | ||
| library.seccomp_rule_add.restype = ctypes.c_int | ||
| library.seccomp_syscall_resolve_name.argtypes = [ctypes.c_char_p] | ||
| library.seccomp_syscall_resolve_name.restype = ctypes.c_int | ||
| library.seccomp_load.argtypes = [ctypes.c_void_p] | ||
| library.seccomp_load.restype = ctypes.c_int | ||
| library.seccomp_release.argtypes = [ctypes.c_void_p] | ||
| context = library.seccomp_init(_ERRNO_EPERM) | ||
| if not context: | ||
| raise ExtractionFailure("parser_failure", "isolation_unavailable") | ||
| try: | ||
| for name in _ALLOWED_SYSCALLS: | ||
| number = library.seccomp_syscall_resolve_name(name.encode("ascii")) | ||
| if number >= 0 and library.seccomp_rule_add(context, _ALLOW, number, 0) != 0: | ||
| raise ExtractionFailure("parser_failure", "isolation_unavailable") | ||
| if library.seccomp_load(context) != 0: | ||
| raise ExtractionFailure("parser_failure", "isolation_unavailable") | ||
| finally: | ||
| library.seccomp_release(context) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does libseccomp seccomp_init only filter the native syscall ABI, and can a process bypass it via the x86 x32/compat syscall ABI on x86_64?
💡 Result:
When you call seccomp_init using libseccomp, the library automatically initializes the filter for the system's native architecture only [1]. It does not automatically include other available syscall ABIs, such as the x32 ABI or the 32-bit (i386) ABI on an x86_64 system [1]. If a developer fails to explicitly add support for these alternative ABIs using seccomp_arch_add, the resulting seccomp filter will not apply its rules to system calls made through those alternative ABIs [1]. Consequently, an attacker or a process can bypass the intended restrictions by invoking system calls using the x32 or compat ABI, effectively operating outside the scope of the filter [2][3]. To secure a process against such bypasses, you must explicitly add all relevant architectures to the libseccomp filter context after calling seccomp_init and before loading the filter [1]. Libseccomp provides the necessary constants (such as SCMP_ARCH_X32 and SCMP_ARCH_X86) to include these ABIs in your filtering logic [4][5][6]. Once added, libseccomp ensures that subsequent rules are applied across all architectures included in the filter [1].
Citations:
- 1: https://www.man7.org/linux/man-pages/man3/seccomp_arch_native.3.html
- 2: https://tripoloski1337.github.io/ctf/2021/07/12/bypassing-seccomp-prctl.html
- 3: https://man7.org/linux/man-pages/man2/seccomp.2.html
- 4: https://github.com/seccomp/libseccomp/blob/main/include/seccomp.h.in
- 5: https://libseccomp.readthedocs.io/
- 6: https://github.com/seccomp/libseccomp
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a guide_extraction_worker.py . || true
echo "== relevant file slice =="
file=$(fd guide_extraction_worker.py . | head -n1)
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo "== seccomp occurrences =="
rg -n "seccomp|_install_seccomp|seccomp_arch" . --glob '!**/.git/**' || trueRepository: Flow-Research/workstream
Length of output: 8204
🌐 Web query:
libseccomp 2.5+ SCMP_ACT_KILL_PROCESS seccomp init default action SCMP_ACT_KILL_PROCESS compat syscall bypass
💡 Result:
In libseccomp, SCMP_ACT_KILL_PROCESS is a filter action that causes the entire process to be terminated with a SIGSYS signal when an unpermitted system call is invoked [1][2]. This action was introduced to provide a more secure, process-wide termination mechanism compared to SCMP_ACT_KILL (or SCMP_ACT_KILL_THREAD), which only terminates the specific thread that triggered the filter [3]. Technical details and considerations regarding your query include: 1. Action Semantics: While SCMP_ACT_KILL terminates only the offending thread [3], SCMP_ACT_KILL_PROCESS forces the entire process group to exit [2][3]. It is often preferred in security-critical applications to prevent scenarios where one thread in a multi-threaded application might be killed while others continue to run [3]. 2. Compat Syscall Bypass: The use of SCMP_ACT_KILL_PROCESS does not inherently prevent syscall bypasses. Seccomp bypasses are typically architectural or logic-based rather than related to the termination action [4]. Common bypass vectors include: - Architecture Confusion: Invoking syscalls through alternative ABIs (e.g., x32 ABI on x86_64) if the seccomp filter is not explicitly configured to cover those syscall numbers for that architecture [4]. - Syscall Multiplexing: Using complex, allowed syscalls (such as io_uring_enter) to perform operations that would otherwise be blocked if invoked directly (e.g., performing network operations without calling network-specific syscalls) [4]. - Profile Misconfiguration: Relying on overly permissive rules or failing to account for how filters are applied across threads (e.g., via TSYNC) [4]. 3. Libseccomp Initialization: When using seccomp_init(uint32_t def_action) to set the default action, passing SCMP_ACT_KILL_PROCESS ensures that any system call not explicitly permitted by your rules will result in the immediate termination of the entire process [5][2]. To defend against bypasses, ensure that your seccomp filters explicitly account for all relevant architectures (including x32 if applicable) and use modern, restrictive allow-lists rather than just relying on the default action to handle unknown calls [4]. SCMP_ACT_KILL_PROCESS acts as a robust enforcement mechanism for your defined policy, but it remains dependent on the completeness and correctness of that policy [4].
Citations:
- 1: https://manpages.debian.org/bullseye/libseccomp-dev/seccomp_init.3.en.html
- 2: https://libseccomp.readthedocs.io/en/latest/man/man3/seccomp_init.3/
- 3: https://www.man7.org/linux/man-pages/man2/seccomp.2.html
- 4: https://www.antitree.com/2026/04/seccomp-in-k8s-2/3-bypasses-and-breakouts/
- 5: https://github.com/seccomp/libseccomp/blob/main/include/seccomp.h.in
🌐 Web query:
man seccomp_arch_native libseccomp default action seccomp_init SCMP_ACT_KILL_PROCESS
💡 Result:
In libseccomp, the functions and constants you mentioned serve distinct roles in configuring secure computing filters for processes [1][2][3]. seccomp_arch_native This function returns the system's architecture token (e.g., SCMP_ARCH_X86_64 or similar) matching one of the defined SCMP_ARCH_* constants [1][4]. It is used to identify the native architecture of the system on which the code is running [1]. seccomp_init and Default Actions The seccomp_init(uint32_t def_action) function initializes the internal seccomp filter state and sets the default action that the kernel should take if a system call does not match any explicitly configured rules [3][5]. SCMP_ACT_KILL_PROCESS When passed to seccomp_init, SCMP_ACT_KILL_PROCESS instructs the kernel to immediately terminate the entire process if an unmatched system call is invoked [3][6]. The process is terminated by the kernel with a SIGSYS signal [3][7]. Unlike SCMP_ACT_KILL (which terminates only the calling thread), SCMP_ACT_KILL_PROCESS ensures that all threads in the thread group are terminated [7]. Summary of Key Points - Initialization: seccomp_init must be called before other libseccomp functions, and it automatically includes the native architecture in the filter context [1][3]. - Native Architecture: You can use seccomp_arch_native if you need to explicitly reference or verify the native architecture token within your filter configuration [1][8]. - Safety Policy: Choosing SCMP_ACT_KILL_PROCESS as the default action is a strict security policy, as it ensures the entire process exits upon encountering any disallowed system call, preventing potential bypasses that might occur if only a single thread were killed [7].
Citations:
- 1: https://www.man7.org/linux/man-pages/man3/seccomp_arch_native.3.html
- 2: https://man7.org/linux/man-pages/man3/seccomp_init.3.html
- 3: https://libseccomp.readthedocs.io/en/latest/man/man3/seccomp_init.3/
- 4: https://man.archlinux.org/man/core/libseccomp/seccomp_arch_native.3.en
- 5: https://manpages.debian.org/testing/libseccomp-dev/seccomp_init.3.en.html
- 6: https://manpages.ubuntu.com/manpages/jammy/man3/seccomp_init.3.html
- 7: https://www.man7.org/linux/man-pages/man2/seccomp.2.html
- 8: https://github.com/seccomp/libseccomp/blob/main/include/seccomp.h.in
Confirm seccomp covers every reachable syscall ABI.
_install_seccomp uses seccomp_init only, which initializes the native ABI filter; any compat/x32/syscall-ABIs reachable from this subprocess are not covered by these allow-list rules. Add explicit seccomp_arch_add calls for each ABI in the deployment, or otherwise ensure compat ABIs are disabled before the untrusted parser runs.
🤖 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/guide_extraction_worker.py` around lines 61 -
91, The _install_seccomp function currently filters only the native syscall ABI;
explicitly add and validate seccomp_arch_add configuration for every ABI
reachable in the deployment, including compat/x32 where applicable, before
installing allow-list rules. If those ABIs are not supported, disable them
before the untrusted parser executes, and treat any architecture-configuration
failure as parser isolation_unavailable.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/modules/artifacts/preparation.py (2)
1093-1098: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer explicit
workspacesin ledger writes over the implicit backfill.
_write_ledgernow re-reads the ledger to synthesize a missingworkspaceskey, which hides the fact that_allocate_sync(Lines 770, 808),_restore_failed_reservation_ledger(Line 824), and_release_sync(Line 918) still build partial ledger dicts. It works (all callers hold the lock), but it costs an extra validated read per write and makes the schema contract implicit. Consider having those call sites carry"workspaces": ledger["workspaces"](they already have the read ledger in hand) and dropping the shim.🤖 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 1093 - 1098, The _write_ledger method should require callers to provide an explicit workspaces field instead of re-reading and backfilling it. Update _allocate_sync, _restore_failed_reservation_ledger, and _release_sync to include the existing ledger["workspaces"] value in every ledger dictionary they write, then remove the implicit workspaces fallback from _write_ledger while retaining validation.
1515-1556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap workspace custody in a manager-owned public API instead of reaching into privates.
extract_prepared_guidedrives four manager internals directly (_reserve_workspace_sync,_workspaces_fd,_release_workspace_sync,_pending_workspaces). The lifecycle is correct — reserve-before-mkdir, cleanup infinally, deferral on cleanup failure — but it lives outside the manager, so it also bypasses_tracked_operation(); the only thing preventingclose()from reclaiming_workspaces_fdmid-extraction is the incidental non-empty_owned_reservations. A manager-side context manager (e.g.with manager.workspace() as workspace_fd:) would own reserve/mkdir/open/release plus the in-flight guard, and keep the service to descriptor-free inspection.🤖 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 1515 - 1556, Move workspace custody out of extract_prepared_guide and expose it through a manager-owned context manager such as workspace(). Have that API perform reservation, directory creation, secure descriptor opening, cleanup/deferred release, and _tracked_operation() protection; update extract_prepared_guide to use the context manager and pass only its returned descriptor-derived workspace path to extractor.inspect, without accessing manager internals.backend/app/modules/artifacts/guide_extraction_service.py (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
except (TimeoutError, Exception)is redundant.
TimeoutErroris a subclass ofException, so the tuple reduces toexcept Exceptionand reads as if two distinct cases are being handled. Neither form catches a re-deliveredCancelledError, which is the intent here, so just simplify.♻️ Proposed simplification
recovery = asyncio.create_task(asyncio.wait_for(record_cancellation(), timeout=5.0)) try: await await_cancellation_resistant(recovery) - except (TimeoutError, Exception): + except Exception: pass raise cancellation from None🤖 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/guide_extraction_service.py` around lines 134 - 138, Simplify the exception handler around await_cancellation_resistant(recovery) in the cancellation recovery flow by replacing the redundant (TimeoutError, Exception) tuple with except Exception, while preserving the existing pass behavior and re-raising cancellation unchanged.
🤖 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.
Nitpick comments:
In `@backend/app/modules/artifacts/guide_extraction_service.py`:
- Around line 134-138: Simplify the exception handler around
await_cancellation_resistant(recovery) in the cancellation recovery flow by
replacing the redundant (TimeoutError, Exception) tuple with except Exception,
while preserving the existing pass behavior and re-raising cancellation
unchanged.
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 1093-1098: The _write_ledger method should require callers to
provide an explicit workspaces field instead of re-reading and backfilling it.
Update _allocate_sync, _restore_failed_reservation_ledger, and _release_sync to
include the existing ledger["workspaces"] value in every ledger dictionary they
write, then remove the implicit workspaces fallback from _write_ledger while
retaining validation.
- Around line 1515-1556: Move workspace custody out of extract_prepared_guide
and expose it through a manager-owned context manager such as workspace(). Have
that API perform reservation, directory creation, secure descriptor opening,
cleanup/deferred release, and _tracked_operation() protection; update
extract_prepared_guide to use the context manager and pass only its returned
descriptor-derived workspace path to extractor.inspect, without accessing
manager internals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1048ec31-8814-4c7e-a6f7-7193246eef2b
📒 Files selected for processing (12)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/preparation.pybackend/tests/conftest.pybackend/tests/fixtures/guide_extraction_probe_worker.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_preparation.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md
c4939a4 to
2d63f73
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/test_alembic.py (1)
2063-2065: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale hardcoded revision — should use
HEAD_REVISION.Both downgrade attempts above this assertion are refused (
RuntimeError), so the revision never leaves head (0042_guide_extraction), yet this checks against the old pre-0042 head literal"0041_project_mutation_evidence". Every other analogous "refused-downgrade-from-head" assertion in this file (Lines 1646, 1966, 2202, 2598, 2810, 2837, 2865-2866, 2924) was updated toHEAD_REVISION; this one was missed and will fail once this migration is head.🐛 Proposed fix
- assert asyncio.run(_current_revision(isolated_database_env)) == ( - "0041_project_mutation_evidence" - ) + assert asyncio.run(_current_revision(isolated_database_env)) == HEAD_REVISION🤖 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_alembic.py` around lines 2063 - 2065, Update the refused-downgrade assertion for _current_revision in backend/tests/test_alembic.py to compare against the shared HEAD_REVISION constant instead of the stale "0041_project_mutation_evidence" literal, preserving the expected unchanged-head behavior.
🧹 Nitpick comments (4)
backend/app/modules/artifacts/guide_extraction_worker.py (2)
104-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-character
unicodedata.categoryscan is a hot path on 32 MiB inputs.This is ~33M Python-level calls for a max-size document and competes directly with the 29s CPU soft limit, so large-but-valid text can spuriously become
cpu_time_limit. A precompiled regex over the control ranges is orders of magnitude faster and equivalent forCc.♻️ Suggested refactor
+_CONTROL = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f]") ... - if any( - unicodedata.category(character) == "Cc" and character not in "\t\n" for character in value - ): + if _CONTROL.search(value): raise ExtractionFailure("malformed", "invalid_control_character")🤖 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/guide_extraction_worker.py` around lines 104 - 107, Replace the per-character unicodedata.category scan in the value validation logic with a module-level precompiled regex matching Cc control-character ranges, excluding tab and newline. Use the regex to detect invalid controls while preserving the existing ExtractionFailure("malformed", "invalid_control_character") behavior.
187-189: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a write-all loop for the result payload.
os.writeon a pipe can return a short count (e.g. interrupted by a signal), and the payload can reach several MiB. A truncated JSON result degrades toinvalid_executor_outputin the parent instead of the real status.♻️ Suggested refactor
- os.write(1, encoded) + view = memoryview(encoded) + while view: + view = view[os.write(1, view) :] return 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/app/modules/artifacts/guide_extraction_worker.py` around lines 187 - 189, Update the result-output path around the worker’s JSON serialization and os.write call to loop until every byte in encoded has been written, advancing by each write’s returned count and retrying interrupted or short writes. Preserve the existing return 0 behavior only after the complete payload is emitted.backend/app/modules/artifacts/guide_extraction_service.py (1)
451-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttempt numbering here ignores
policy_version, unlikeextract_prepared.The uniqueness key is
(binding_id, policy_version, attempt_number), andextract_preparedscopes itsmax()accordingly (Line 152-155). This path takes a global per-binding max, so numbering diverges across policy versions once a second version exists.♻️ Suggested alignment
- ).where(GuideSourceExtractionAttempt.binding_id == facts.binding_id) + ).where( + GuideSourceExtractionAttempt.binding_id == facts.binding_id, + GuideSourceExtractionAttempt.policy_version == EXTRACTION_POLICY_VERSION, + )🤖 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/guide_extraction_service.py` around lines 451 - 458, Update the attempt-number calculation in the extraction path containing GuideSourceExtractionAttempt to scope the max(attempt_number) query by both facts.binding_id and the current policy_version, matching extract_prepared. Preserve the existing fallback to zero and increment behavior while ensuring numbering is independent for each (binding_id, policy_version) pair.backend/app/modules/artifacts/preparation.py (1)
416-438: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
/proc/self/fd/{fd}as the workspace handle is process- and ordering-fragile.The yielded path is only meaningful inside this process, and the consumer (
GuideExtractionRunner) passes it assubprocesscwd. That happens to work solely because CPython's_posixsubprocessperforms thechdirin the child before closing inherited descriptors; it is not a documented ordering guarantee, and it silently breaks under any consumer that resolves the path after the fd is closed or in another process. Consider yielding the real directory path (self._root / "workspaces" / workspace_name) while keeping the fd pinned for cleanup, or documenting the constraint on the protocol.Also note: the
raisein theexcept BaseExceptionhandler replaces the in-flight body exception with the release failure, so the original extraction error is demoted to__context__.🤖 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 416 - 438, Update extraction_workspace to yield the real workspace directory path derived from self._root and workspace_name, while retaining the open descriptor for cleanup and containment. In the release-workspace exception handler, preserve any exception already raised by the extraction body instead of replacing it with the cleanup failure; only propagate the release error when no body exception is active, while still tracking the workspace.
🤖 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/artifacts/guide_extraction_service.py`:
- Around line 342-350: Update the retry-budget branch in the guide extraction
method containing latest_attempt and budget.claimed_slots so a missing
latest_attempt can claim the remaining slot when budget.claimed_slots is below
2, flush, and return None instead of raising unavailability. Preserve returning
_result(latest_attempt) when an attempt exists, and remove the now-unreachable
latest_attempt guard and trailing exhaustion error; only raise the exhaustion
error when the retry budget is already fully claimed.
In `@backend/app/modules/artifacts/guide_extraction_worker.py`:
- Around line 84-87: Update the seccomp allowlist loop in the isolation setup to
treat any negative result from library.seccomp_syscall_resolve_name as an
isolation failure, raising ExtractionFailure with "parser_failure" and
"isolation_unavailable" before attempting to add the rule. Preserve the existing
failure handling for unsuccessful seccomp_rule_add calls.
- Around line 152-163: Update the CSV parsing setup in the extraction flow
around csv.reader to raise the csv module field-size limit above the
32,768-character policy ceiling before reading rows. Keep the existing explicit
cell-length check authoritative so oversized cells raise ExtractionFailure with
limit_exceeded and csv_cell_size_limit, while csv.Error continues to represent
malformed CSV.
In `@backend/app/modules/artifacts/guide_extraction.py`:
- Around line 107-126: Validate error_code in the executor-result parsing flow
alongside status and output before calling _result: require the expected
nullable string shape, enforce its maximum encoded size, and ensure it is null
when status is extracted or unsupported as required by the persistence contract.
Return the existing parser_failure/invalid_executor_output result for invalid
combinations instead of passing them through.
---
Outside diff comments:
In `@backend/tests/test_alembic.py`:
- Around line 2063-2065: Update the refused-downgrade assertion for
_current_revision in backend/tests/test_alembic.py to compare against the shared
HEAD_REVISION constant instead of the stale "0041_project_mutation_evidence"
literal, preserving the expected unchanged-head behavior.
---
Nitpick comments:
In `@backend/app/modules/artifacts/guide_extraction_service.py`:
- Around line 451-458: Update the attempt-number calculation in the extraction
path containing GuideSourceExtractionAttempt to scope the max(attempt_number)
query by both facts.binding_id and the current policy_version, matching
extract_prepared. Preserve the existing fallback to zero and increment behavior
while ensuring numbering is independent for each (binding_id, policy_version)
pair.
In `@backend/app/modules/artifacts/guide_extraction_worker.py`:
- Around line 104-107: Replace the per-character unicodedata.category scan in
the value validation logic with a module-level precompiled regex matching Cc
control-character ranges, excluding tab and newline. Use the regex to detect
invalid controls while preserving the existing ExtractionFailure("malformed",
"invalid_control_character") behavior.
- Around line 187-189: Update the result-output path around the worker’s JSON
serialization and os.write call to loop until every byte in encoded has been
written, advancing by each write’s returned count and retrying interrupted or
short writes. Preserve the existing return 0 behavior only after the complete
payload is emitted.
In `@backend/app/modules/artifacts/preparation.py`:
- Around line 416-438: Update extraction_workspace to yield the real workspace
directory path derived from self._root and workspace_name, while retaining the
open descriptor for cleanup and containment. In the release-workspace exception
handler, preserve any exception already raised by the extraction body instead of
replacing it with the cleanup failure; only propagate the release error when no
body exception is active, while still tracking the workspace.
🪄 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: f4563f8e-6c81-446e-be81-98b64430eba5
📒 Files selected for processing (24)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DECISIONS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3A-extraction-framework-text.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-pr-trust-bundle.md.agent-loop/merge-intents/WS-ART-001-03B3A.jsonbackend/alembic/versions/0042_guide_extraction.pybackend/app/db/models.pybackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/preparation.pybackend/app/modules/artifacts/sources.pybackend/scripts/run_test_lanes.pybackend/tests/conftest.pybackend/tests/fixtures/guide_extraction_probe_worker.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_preparation.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pydocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .agent-loop/merge-intents/WS-ART-001-03B3A.json
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3A-pr-trust-bundle.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/DECISIONS.md
- docs/spec_artifact_storage_service.md
| if latest_attempt is None: | ||
| raise GuideExtractionError("guide extraction is unavailable") | ||
| if budget.claimed_slots < 2: | ||
| budget.claimed_slots += 1 | ||
| await session.flush() | ||
| return None | ||
| if latest_attempt is not None: | ||
| return self._result(latest_attempt) | ||
| raise GuideExtractionError("guide extraction retry budget is exhausted") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A crash between slot claim and attempt persistence permanently bricks the lineage; the tail branches are also dead.
If a worker claims slot 1 (Line 330) and then dies before any GuideSourceExtractionAttempt row is written, the next call finds budget present and latest_attempt is None, so Line 342-343 raises guide extraction is unavailable forever — the reserved second slot can never be used, even though the contract intends one fresh-materialization retry after executor loss. Consider allowing the retry when claimed_slots < 2 and no attempt exists, and reserving the hard failure for the exhausted case.
Note also that once Line 342 guarantees latest_attempt is not None, the if latest_attempt is not None guard at Line 348 and the raise at Line 350 are unreachable.
🐛 Sketch
- if latest_attempt is None:
- raise GuideExtractionError("guide extraction is unavailable")
if budget.claimed_slots < 2:
budget.claimed_slots += 1
await session.flush()
return None
if latest_attempt is not None:
return self._result(latest_attempt)
raise GuideExtractionError("guide extraction retry budget is exhausted")📝 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.
| if latest_attempt is None: | |
| raise GuideExtractionError("guide extraction is unavailable") | |
| if budget.claimed_slots < 2: | |
| budget.claimed_slots += 1 | |
| await session.flush() | |
| return None | |
| if latest_attempt is not None: | |
| return self._result(latest_attempt) | |
| raise GuideExtractionError("guide extraction retry budget is exhausted") | |
| if budget.claimed_slots < 2: | |
| budget.claimed_slots += 1 | |
| await session.flush() | |
| return None | |
| if latest_attempt is not None: | |
| return self._result(latest_attempt) | |
| raise GuideExtractionError("guide extraction retry budget is exhausted") |
🤖 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/guide_extraction_service.py` around lines 342 -
350, Update the retry-budget branch in the guide extraction method containing
latest_attempt and budget.claimed_slots so a missing latest_attempt can claim
the remaining slot when budget.claimed_slots is below 2, flush, and return None
instead of raising unavailability. Preserve returning _result(latest_attempt)
when an attempt exists, and remove the now-unreachable latest_attempt guard and
trailing exhaustion error; only raise the exhaustion error when the retry budget
is already fully claimed.
2 participants
Chunk
WS-ART-001-03B3A — Extraction Framework And Text Formats (L1)
Goal
Extract bounded canonical text from exact verified guide content without weakening artifact custody or allowing raw binaries, provider access, caller authority, or agent invocation across the parser boundary.
What changed
Scope
No PDF/OOXML/image parser, provider write, agent invocation, Celery continuation, public route, submission behavior, legacy cutover, or AUTH availability change.
Evidence
Human review focus
Review default-deny parser isolation, exact lineage locks, policy-scoped replay, serialized retry custody, workspace cleanup, and absence of provider/agent/AUTH scope expansion.
Do not merge without explicit user approval.