fix: bounded micro-batch local embedding and one-hop multi-anchor exclusion - #230
Conversation
📝 WalkthroughWalkthroughThe change adds distinct timeout and document-refusal contracts for local embeddings, persists acquired-document refusals through a protected database function, and updates one-hop graph resolution to exclude the complete requested anchor set. ChangesEmbedding refusal flow
One-hop anchor resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a892dd2de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| thread_name="context-engine-local-embedding", | ||
| in_flight_lock=self._inference_lock, | ||
| ) | ||
| for raw_vector in raw_vectors: |
There was a problem hiding this comment.
stometa, when individual backend responses have incorrect counts that cancel across batches—for example, seven vectors for the first eight inputs and two for the final input—the concatenated list still has the expected total length, so the final validate_embedding_batch accepts it and persists shifted or unrelated Fragment embeddings. Validate each raw_vectors result against its corresponding batch so every wrong-cardinality response fails closed as required by ADR-0102.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97daac1d23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._reduce_vectors( | ||
| (_LOCAL_EMBEDDING_WARMUP_TEXT,), | ||
| self._encode_model_inputs([_LOCAL_EMBEDDING_WARMUP_TEXT]), | ||
| ) |
There was a problem hiding this comment.
stometa, if the model wedges during its first inference, this direct _encode_model_inputs call makes provider construction—and therefore process startup—hang indefinitely instead of returning the generic bounded failure required by ADR-0102 clause 6. Run the warmup through invoke_bounded with the same lock and deadline, or remove it.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
| except BoundedCallTimedOut: | ||
| if document and len(batch) == 1: | ||
| raise EmbeddingDocumentRefused( | ||
| "Embedding document is outside provider bounds" | ||
| ) from None |
There was a problem hiding this comment.
stometa, whenever a valid single-fragment inference exceeds 30 seconds because the host is overloaded or the backend stalls transiently, this converts the timeout into EmbeddingDocumentRefused; the worker then deletes the acquired recovery state and permanently reports unsupported_document_shape, even though a retry may succeed. ADR-0102 clause 6 explicitly classifies timeouts as provider unavailability that leaves acquired publication recoverable, so reserve terminal document refusal for a deterministic input-bound check rather than elapsed time.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f9b9942aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| OR requested_compilation_refusal_category <> | ||
| 'unsupported_document_shape' |
There was a problem hiding this comment.
stometa, when a worker invokes this SECURITY DEFINER function with a valid acquired lease but a NULL category, PostgreSQL evaluates the <> expression to NULL and the IF does not return. The function then deletes the recoverable checkpoint and permanently fails the job while storing no unsupported_document_shape category, bypassing the function's sole admitted refusal reason; use a NULL-safe comparison such as IS DISTINCT FROM.
Useful? React with 👍 / 👎.
![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 (3)
tests/integration/test_one_hop_graph_http_sdk.py (2)
119-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing
_RootOnlyCandidateIndexinto_MultipleRootCandidateIndex.
_RootOnlyCandidateIndexat lines 75-116 is now the single-root case of this class. The three methods are identical except for the candidate tuple. One class removes about 40 duplicated lines and keeps the two tests on one seam implementation.Keep
_RootOnlyCandidateIndexonly if you want the single-root name to document intent at the call sites.♻️ Proposed consolidation
-class _RootOnlyCandidateIndex: - """Submit only the main-path root; graph structure is not index authority.""" - - def __init__(self, root: CandidateRef) -> None: - self.root = root +class _MultipleRootCandidateIndex: + """Submit authorized main-path roots; graph structure is not index authority.""" + + def __init__(self, roots: tuple[CandidateRef, ...]) -> None: + self.roots = rootsThen call
_MultipleRootCandidateIndex((root,))at the existing single-root call site and delete the duplicated class.🤖 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 `@tests/integration/test_one_hop_graph_http_sdk.py` around lines 119 - 162, Consolidate the duplicate _RootOnlyCandidateIndex implementation into _MultipleRootCandidateIndex by deleting the former class and updating its single-root call site to construct _MultipleRootCandidateIndex with a one-element tuple, while preserving the existing test behavior.
746-750: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the one-hop exclusion, not just resource-ref presence.
_MultipleRootCandidateIndexsubmits bothfirstandsecondas requested anchors. Both resource refs reachdelivered_resourcesthrough the main path, so these assertions pass whethersecondis returned as a one-hop neighbor offirstor excluded. Count the evidence items and assertsecondis not emitted as a graph-expanded neighbor offirst, or assert that each delivered fragment ref appears exactly once.🤖 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 `@tests/integration/test_one_hop_graph_http_sdk.py` around lines 746 - 750, Update the assertions around delivered_resources in the one-hop graph test to verify the exclusion behavior, not merely resource-ref presence. Count the evidence items or otherwise inspect their fragment refs so the test proves second is not emitted as a graph-expanded neighbor of first, while preserving validation that the requested anchors are delivered.Source: Coding guidelines
tests/unit/test_local_qwen_embedding_provider.py (1)
251-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that a lock-contended call is not a document refusal.
EmbeddingDocumentRefusedsubclassesEmbeddingProviderUnavailable, so bothpytest.raisesblocks pass for either exception. The distinction is important: a document refusal permanently fails the import job through_refuse_acquired_embedding_document, while provider unavailability produces a retryable interruption. The second call here is rejected by the in-flight lock and must stay retryable.Add a negative assertion so the test proves the classification.
💚 Proposed test strengthening
started = monotonic() - with pytest.raises(EmbeddingProviderUnavailable): + with pytest.raises(EmbeddingProviderUnavailable) as first_failure: provider.embed_documents(("document text",)) first_elapsed = monotonic() - started started = monotonic() - with pytest.raises(EmbeddingProviderUnavailable): + with pytest.raises(EmbeddingProviderUnavailable) as second_failure: provider.embed_documents(("document text",)) second_elapsed = monotonic() - started release.set() + assert not isinstance(second_failure.value, EmbeddingDocumentRefused) + assert type(first_failure.value) is EmbeddingProviderUnavailable assert first_elapsed < 0.5 assert second_elapsed < 0.5 assert model.calls == 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 `@tests/unit/test_local_qwen_embedding_provider.py` around lines 251 - 264, Strengthen the two pytest.raises checks around provider.embed_documents in the lock-contention test to assert the raised exception is specifically EmbeddingProviderUnavailable and not its EmbeddingDocumentRefused subclass. Preserve the existing timing and model.calls assertions while proving the contended second call remains retryable.
🤖 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 `@adapters/embeddings.py`:
- Around line 294-302: Update the warmup encode in
LocalQwenEmbeddingProvider.__init__ to invoke _encode_model_inputs through
invoke_bounded, passing self._inference_lock and the same timeout configuration
used for other model calls; keep the existing _reduce_vectors flow and
EmbeddingProviderUnavailable error translation intact.
In `@engine/persistence/schema_security_manifest.yaml`:
- Around line 8868-8869: Correct the schema manifest’s DELETE authority by
removing it from the file_import_job declaration and adding it to
file_publication_recovery for context_engine_worker_lease_definer. Add
context_worker_refuse_acquired_embedding_document to
file_publication_recovery.functionOnlyMutation.databaseFunctions and add
file_publication_recovery to fail_file_import.atomicWrites. Confirm the manifest
versioning convention and advance manifestVersion beyond 44.0.0 if required.
In `@migrations/versions/20260803_0054_bounded_embedding_refusal.py`:
- Around line 102-104: The recovery-row DELETE authorization must be restricted
to exactly one row with checkpoint='acquired'. Update
context_worker_refuse_acquired_embedding_document and the corresponding
policy/command entry in engine/persistence/schema_security_manifest.yaml at
lines 8868-8869 to enforce that predicate, and update
context_worker_fail_file_import_with_category to capture the DELETE row count
and return true only when exactly one acquired recovery row was deleted.
In `@tests/integration/test_fragment_embeddings.py`:
- Around line 396-427: Extend the test covering
context_worker_refuse_acquired_embedding_document to assert the scenario’s
file_publication_recovery row is deleted, not merely that the job is failed and
vectors are absent. Also inspect the captured traceback or error output and
assert it does not contain the provider detail string “provider document detail
must not escape”, while preserving the existing refusal and job-state
assertions.
---
Nitpick comments:
In `@tests/integration/test_one_hop_graph_http_sdk.py`:
- Around line 119-162: Consolidate the duplicate _RootOnlyCandidateIndex
implementation into _MultipleRootCandidateIndex by deleting the former class and
updating its single-root call site to construct _MultipleRootCandidateIndex with
a one-element tuple, while preserving the existing test behavior.
- Around line 746-750: Update the assertions around delivered_resources in the
one-hop graph test to verify the exclusion behavior, not merely resource-ref
presence. Count the evidence items or otherwise inspect their fragment refs so
the test proves second is not emitted as a graph-expanded neighbor of first,
while preserving validation that the requested anchors are delivered.
In `@tests/unit/test_local_qwen_embedding_provider.py`:
- Around line 251-264: Strengthen the two pytest.raises checks around
provider.embed_documents in the lock-contention test to assert the raised
exception is specifically EmbeddingProviderUnavailable and not its
EmbeddingDocumentRefused subclass. Preserve the existing timing and model.calls
assertions while proving the contended second call remains retryable.
🪄 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: c7519a2d-2129-4c6b-bc96-fb0c545a6300
📒 Files selected for processing (12)
adapters/_bounded_call.pyadapters/embeddings.pyengine/persistence/file_imports.pyengine/persistence/schema_security_manifest.yamlengine/supply/__init__.pyengine/supply/embeddings.pymigrations/versions/20260803_0054_bounded_embedding_refusal.pymigrations/versions/20260803_0055_exclude_full_one_hop_anchor_set.pytests/integration/test_fragment_embeddings.pytests/integration/test_one_hop_graph_http_sdk.pytests/unit/test_local_qwen_embedding_provider.pytests/unit/test_schema_security_manifest.py
| try: | ||
| self._reduce_vectors( | ||
| (_LOCAL_EMBEDDING_WARMUP_TEXT,), | ||
| self._encode_model_inputs([_LOCAL_EMBEDDING_WARMUP_TEXT]), | ||
| ) | ||
| except Exception: | ||
| raise EmbeddingProviderUnavailable( | ||
| "Embedding provider is unavailable" | ||
| ) from None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the warmup encode with invoke_bounded.
The model load on Line 283 is deadline-bounded. The warmup encode on Lines 295-298 is not. If the sentence-transformers backend stalls during the first encode, LocalQwenEmbeddingProvider.__init__ blocks with no deadline and no recovery. Every other model call in this class goes through invoke_bounded. Apply the same bound here.
Note that the warmup runs after invoke_bounded released self._inference_lock, so passing the lock again is safe.
🛡️ Proposed fix to bound the warmup call
self._model: Any = model
try:
self._reduce_vectors(
(_LOCAL_EMBEDDING_WARMUP_TEXT,),
- self._encode_model_inputs([_LOCAL_EMBEDDING_WARMUP_TEXT]),
+ invoke_bounded(
+ partial(
+ self._encode_model_inputs,
+ [_LOCAL_EMBEDDING_WARMUP_TEXT],
+ ),
+ timeout_seconds=_LOCAL_EMBEDDING_TIMEOUT_SECONDS,
+ thread_name="context-engine-local-embedding",
+ in_flight_lock=self._inference_lock,
+ ),
)
- except Exception:
+ except Exception: # noqa: BLE001
raise EmbeddingProviderUnavailable(
"Embedding provider is unavailable"
) from None📝 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.
| try: | |
| self._reduce_vectors( | |
| (_LOCAL_EMBEDDING_WARMUP_TEXT,), | |
| self._encode_model_inputs([_LOCAL_EMBEDDING_WARMUP_TEXT]), | |
| ) | |
| except Exception: | |
| raise EmbeddingProviderUnavailable( | |
| "Embedding provider is unavailable" | |
| ) from None | |
| try: | |
| self._reduce_vectors( | |
| (_LOCAL_EMBEDDING_WARMUP_TEXT,), | |
| invoke_bounded( | |
| partial( | |
| self._encode_model_inputs, | |
| [_LOCAL_EMBEDDING_WARMUP_TEXT], | |
| ), | |
| timeout_seconds=_LOCAL_EMBEDDING_TIMEOUT_SECONDS, | |
| thread_name="context-engine-local-embedding", | |
| in_flight_lock=self._inference_lock, | |
| ), | |
| ) | |
| except Exception: # noqa: BLE001 | |
| raise EmbeddingProviderUnavailable( | |
| "Embedding provider is unavailable" | |
| ) from None |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 299-299: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@adapters/embeddings.py` around lines 294 - 302, Update the warmup encode in
LocalQwenEmbeddingProvider.__init__ to invoke _encode_model_inputs through
invoke_bounded, passing self._inference_lock and the same timeout configuration
used for other model calls; keep the existing _reduce_vectors flow and
EmbeddingProviderUnavailable error translation intact.
| "UPDATE", | ||
| "DELETE acquired recovery only through context_worker_refuse_acquired_embedding_document" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The DELETE authority is recorded against the wrong table.
context_worker_refuse_acquired_embedding_document never deletes from file_import_job. It only runs UPDATE public.file_import_job (migration Lines 105-112). The DELETE it performs targets public.file_publication_recovery (migration Lines 102-104), and the migration grants DELETE ON TABLE public.file_publication_recovery to the definer (migration Lines 31-33).
Three declarations are now wrong:
- Line 8869 grants
file_import_joba DELETE authority that no migration grants and no code exercises. This overstates the privilege surface of the most sensitive job table. - The
file_publication_recoveryentry (Lines 9057-9061) still lists onlySELECT,INSERT,UPDATEforcontext_engine_worker_lease_definer, so the real DELETE grant is undeclared. file_publication_recovery.functionOnlyMutation.databaseFunctions(Lines 9039-9044) omitscontext_worker_refuse_acquired_embedding_document, andfail_file_import.atomicWrites(Lines 706-708) omitsfile_publication_recovery.
Move the DELETE declaration to the table it applies to and record the new mutation path there. Also confirm whether manifestVersion must advance past 44.0.0 for this change.
🔒️ Proposed manifest corrections
"context_engine_worker_lease_definer": [
"SELECT",
"INSERT",
- "UPDATE",
- "DELETE acquired recovery only through context_worker_refuse_acquired_embedding_document"
+ "UPDATE"
],Then update the file_publication_recovery entry:
"functionOnlyMutation": {
"databaseFunctions": [
"context_worker_acquire_file_publication",
"context_worker_prepare_file_publication",
"context_worker_index_file_publication",
- "context_worker_activate_recoverable_file_publication"
+ "context_worker_activate_recoverable_file_publication",
+ "context_worker_refuse_acquired_embedding_document"
], "context_engine_worker_lease_definer": [
"SELECT",
"INSERT",
- "UPDATE"
+ "UPDATE",
+ "DELETE acquired checkpoint only through context_worker_refuse_acquired_embedding_document"
]And the fail_file_import operation:
"atomicWrites": [
- "file_import_job"
+ "file_import_job",
+ "file_publication_recovery"
]🤖 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 `@engine/persistence/schema_security_manifest.yaml` around lines 8868 - 8869,
Correct the schema manifest’s DELETE authority by removing it from the
file_import_job declaration and adding it to file_publication_recovery for
context_engine_worker_lease_definer. Add
context_worker_refuse_acquired_embedding_document to
file_publication_recovery.functionOnlyMutation.databaseFunctions and add
file_publication_recovery to fail_file_import.atomicWrites. Confirm the manifest
versioning convention and advance manifestVersion beyond 44.0.0 if required.
| DELETE FROM public.file_publication_recovery AS recovery | ||
| WHERE recovery.organization_id = requested_organization_id | ||
| AND recovery.job_id = requested_job_id; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm existing RLS policies, grants, and manifest declarations for file_publication_recovery.
set -euo pipefail
echo "== migrations touching file_publication_recovery =="
rg -n -C 8 'file_publication_recovery' --glob 'migrations/**/*.py' \
| rg -n 'POLICY|ROW LEVEL SECURITY|FORCE|GRANT|REVOKE|DELETE|CREATE TABLE'
echo "== manifest declaration =="
rg -n -A 70 '"name": "file_publication_recovery"' engine/persistence/schema_security_manifest.yamlRepository: stone16/context-engine
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate migration file =="
fd -a '20260803_0054_bounded_embedding_refusal\.py' . || true
echo "== migrations containing file_publication_recovery or context_worker_refuse_acquired_embedding_document =="
rg -n -i 'file_publication_recovery|context_worker_refuse_acquired_embedding_document|context_engine_worker_lease_definer' migrations engine -g '*.py' -g '*.yaml' | sed -n '1,220p'
echo "== migration file line count and relevant snippets =="
if [ -f migrations/versions/20260803_0054_bounded_embedding_refusal.py ]; then
wc -l migrations/versions/20260803_0054_bounded_embedding_refusal.py
sed -n '80,125p' migrations/versions/20260803_0054_bounded_embedding_refusal.py | cat -n
fi
echo "== schema_security_manifest relevant sections =="
rg -n -A 90 '"name": "file_publication_recovery"|name: file_publication_recovery|file_import_job' engine/persistence/schema_security_manifest.yaml | sed -n '1,260p'Repository: stone16/context-engine
Length of output: 44627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bounded_embedding_refusal migration =="
sed -n '1,145p' migrations/versions/20260803_0054_bounded_embedding_refusal.py | cat -n
echo "== file_publication_recovery manifest section =="
sed -n '8870,9045p' engine/persistence/schema_security_manifest.yaml | cat -n
echo "== file_import_job definer section around DELETE comment =="
sed -n '8800,8885p' engine/persistence/schema_security_manifest.yaml | cat -n
echo "== all file_publication_recovery policy names in manifest =="
python3 - <<'PY'
import re, pathlib
path=pathlib.Path("engine/persistence/schema_security_manifest.yaml")
text=path.read_text()
# simple state machine around table entries is not needed; grab lines mentioning file_publication_recovery policies
for line in open(path):
if "file_publication_recovery_file_import_definer" in line or "file_publication_recovery" in line and "POLICY" in line:
print(line.rstrip())
PYRepository: stone16/context-engine
Length of output: 18672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== context_worker_fail_file_import migration around acquire/delete policy =="
rg -n -A 120 -B 25 'context_worker_fail_file_import' migrations/versions/20260803_0054_bounded_embedding_refusal.py | cat -n
echo "== all file_import_job RLS definer policies in migration files =="
rg -n -A 4 'file_import_job_.*definer_(select|insert|update|delete)|file_import_job .* FOR (SELECT|INSERT|UPDATE|DELETE) .* FOR PUBLIC|GRANT SELECT, INSERT, UPDATE, DELETE' migrations/versions/*.py | sed -n '1,260p'
echo "== all CREATE POLICY lines in migration files with file_publication_recovery and file_import_job =="
rg -n -C 6 'CREATE POLICY .*file_(publication_recovery|import_job).*definer|CREATE POLICY file_publication_recovery.*FOR DELETE|CREATE POLICY file_import_job.*FOR DELETE' migrations/versions/*.py | sed -n '1,260p'
echo "== read-only Python parser of PLpgSQL RETURN FOUND assignment and UPDATE target =="
python3 - <<'PY'
from pathlib import Path
p=Path("migrations/versions/20260803_0054_bounded_embedding_refusal.py")
s=p.read_text()
start=s.index("CREATE FUNCTION")
end=s.index("END; $function$")
body=s[start:end]
print("contains RETURN FOUND:", "RETURN FOUND;\n" in body)
print("contains assignment to FOUND variable:", "FOUND :=" in body or "Found :=\n" in body)
print("contains DELETE from acquisition table:", "DELETE FROM public.file_publication_recovery" in body)
print("contains UPDATE target with FOR UPDATE before RETURN FOUND:", "UPDATE public.file_import_job AS job" in body[s.find("RETURN FOUND;"):])
PYRepository: stone16/context-engine
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== context_worker_fail_file_import in migration files =="
rg -n -C 40 'context_worker_fail_file_import' migrations engine | sed -n '1,260p'
echo "== RLS policies for file_publication_recovery and file_import_job definers =="
rg -n -C 6 'file_(publication_recovery|import_job)_(migrator|file_import|.*definer|.*acquisition).*FOR' migrations -g '*.py' | sed -n '1,320p'
echo "== grant lines with DELETE on recovery or import tables =="
rg -n 'GRANT .*DELETE.*ON TABLE (public\.)?file_(publication_recovery|import_job)|GRANT SELECT, INSERT, UPDATE, DELETE (ON TABLE (public\.)?)?file_import_job|GRANT SELECT, INSERT, UPDATE(, DELETE| FROM _DEFINER| ON file_publication_recovery)' migrations/versions -g '*.py' | sed -n '1,260p'
echo "== read-only Python parser of control-flow comments =="
python3 - <<'PY'
from pathlib import Path
p=Path("migrations/versions/20260803_0054_bounded_embedding_refusal.py")
text=p.read_text()
print("DELETE from public.file_publication_recovery:", "DELETE FROM public.file_publication_recovery" in text)
print("UPDATE public.file_import_job AS job:", "UPDATE public.file_import_job AS job" in text)
idx = text.index("RETURN FOUND;")
last_statement = text[text.rfind(";", 0, idx):idx+1]
print("last control-flow statement before RETURN FOUND:", repr(last_statement.strip()))
PYRepository: stone16/context-engine
Length of output: 28488
🌐 Web query:
PostgreSQL FORCE ROW LEVEL SECURITY implicit USING policy default deny if no matching policy
💡 Result:
In PostgreSQL, when Row Level Security (RLS) is enabled on a table, the system enforces a default-deny policy if no specific policies are defined [1][2]. This means that if RLS is enabled but no policies have been created, no rows will be visible or modifiable by anyone subject to RLS [1][3]. The command ALTER TABLE... FORCE ROW LEVEL SECURITY does not change this default-deny behavior; rather, it extends the enforcement of existing RLS policies to the table owner [1][4]. By default, PostgreSQL table owners are exempt from RLS policies [1][5]. Using FORCE ROW LEVEL SECURITY ensures that the table owner is also subject to these policies, which is a common best practice for production environments to prevent accidental data access or modification by highly privileged roles [4]. In summary: 1. RLS Enabled (ENABLE ROW LEVEL SECURITY): Standard users are subject to RLS; table owners are typically exempt [1][6]. 2. Default-Deny: If RLS is enabled but no policy exists, all rows are effectively hidden/locked for those subject to RLS [1][2]. 3. FORCE ROW LEVEL SECURITY: This option forces the table owner to be subject to the same RLS policies as other users, ensuring that no user (including the owner) can bypass the defined restrictions [1][4]. Superusers and roles with the BYPASSRLS attribute are always exempt from RLS, regardless of whether FORCE ROW LEVEL SECURITY is enabled [1][6].
Citations:
- 1: https://www.postgresql.org/docs/current/ddl-rowsecurity.html
- 2: https://www.postgresql.org/docs/current/sql-createpolicy.html
- 3: https://www.postgresql.org/docs/19/sql-createpolicy.html
- 4: https://www.promptstoproduct.com/how-row-level-security-actually-works
- 5: https://www.postgresql.org/docs/14/ddl-rowsecurity.html
- 6: https://www.postgresql.org/docs/16/ddl-rowsecurity.html
Add DELETE authorization only for the acquired recovery row. context_worker_refuse_acquired_embedding_document deletes where recovery.checkpoint = 'acquired', but context_worker_fail_file_import_with_category uses RETURN FOUND after deleting the lock-row and updating file_import_job. A broad recovery DELETE policy or a DELETE grant on file_publication_recovery alone can make that path return true without deleting exactly one acquired recovery. Narrow the policy/command to the acquired checkpoint and use an explicit row-count comparison before returning.
📍 Affects 2 files
migrations/versions/20260803_0054_bounded_embedding_refusal.py#L102-L104(this comment)engine/persistence/schema_security_manifest.yaml#L8868-L8869
🤖 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 `@migrations/versions/20260803_0054_bounded_embedding_refusal.py` around lines
102 - 104, The recovery-row DELETE authorization must be restricted to exactly
one row with checkpoint='acquired'. Update
context_worker_refuse_acquired_embedding_document and the corresponding
policy/command entry in engine/persistence/schema_security_manifest.yaml at
lines 8868-8869 to enforce that predicate, and update
context_worker_fail_file_import_with_category to capture the DELETE row count
and return true only when exactly one acquired recovery row was deleted.
| with pytest.raises(FileImportRefused, match="File import is unavailable"): | ||
| _run( | ||
| _worker( | ||
| scenario, | ||
| guarded_worker_engine, | ||
| _BoundedDocumentRefusalProvider(), | ||
| ), | ||
| scenario, | ||
| scenario.token, | ||
| ) | ||
|
|
||
| engine = create_database_engine(migration_configuration) | ||
| try: | ||
| with engine.connect() as connection: | ||
| job = connection.execute( | ||
| text( | ||
| """ | ||
| SELECT state, compilation_refusal_category | ||
| FROM file_import_job | ||
| WHERE organization_id = :organization_id | ||
| AND job_id = :job_id | ||
| """ | ||
| ), | ||
| { | ||
| "organization_id": scenario.organization_id, | ||
| "job_id": scenario.prepared.job_id, | ||
| }, | ||
| ).one() | ||
| assert tuple(job) == ("failed", "unsupported_document_shape") | ||
| finally: | ||
| engine.dispose() | ||
| assert _stored_vectors(migration_configuration, scenario) == () |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert that the acquired recovery row is deleted.
The purpose of context_worker_refuse_acquired_embedding_document is to clear the acquired file_publication_recovery checkpoint and seal the job. This test verifies the job state and the absence of vectors, but never verifies the deletion. A DELETE that removes zero rows would still pass every current assertion, because RETURN FOUND reflects the later UPDATE. See the related concern on migrations/versions/20260803_0054_bounded_embedding_refusal.py Lines 102-104.
Also assert that the provider message does not reach the traceback. _embedding_document raises from None, and the double deliberately carries "provider document detail must not escape".
💚 Proposed additional assertions
- with pytest.raises(FileImportRefused, match="File import is unavailable"):
+ with pytest.raises(
+ FileImportRefused, match="File import is unavailable"
+ ) as failure:
_run(
_worker(
scenario,
guarded_worker_engine,
_BoundedDocumentRefusalProvider(),
),
scenario,
scenario.token,
)
+ assert failure.value.__cause__ is None
engine = create_database_engine(migration_configuration)
try:
with engine.connect() as connection:
job = connection.execute(
text(
"""
SELECT state, compilation_refusal_category
FROM file_import_job
WHERE organization_id = :organization_id
AND job_id = :job_id
"""
),
{
"organization_id": scenario.organization_id,
"job_id": scenario.prepared.job_id,
},
).one()
+ recovery_rows = connection.execute(
+ text(
+ """
+ SELECT count(*)
+ FROM file_publication_recovery
+ WHERE organization_id = :organization_id
+ AND job_id = :job_id
+ """
+ ),
+ {
+ "organization_id": scenario.organization_id,
+ "job_id": scenario.prepared.job_id,
+ },
+ ).scalar_one()
assert tuple(job) == ("failed", "unsupported_document_shape")
+ assert recovery_rows == 0
finally:
engine.dispose()🤖 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 `@tests/integration/test_fragment_embeddings.py` around lines 396 - 427, Extend
the test covering context_worker_refuse_acquired_embedding_document to assert
the scenario’s file_publication_recovery row is deleted, not merely that the job
is failed and vectors are absent. Also inspect the captured traceback or error
output and assert it does not contain the provider detail string “provider
document detail must not escape”, while preserving the existing refusal and
job-state assertions.
stone16
changed the title
1 participant
Two defects surfaced by the Qwen measured-acceptance run against the real corpus, fixed here (both discovered under issue #147's acceptance work; see ADR-0102):
Bounded local-embedding inference (micro-batch size 1) with warm preload. Model load happens once in the provider constructor, outside every per-call deadline. Each document is embedded via per-fragment micro-batches (
_LOCAL_EMBEDDING_MICRO_BATCH_SIZE = 1— note: NOT 8-fragment batches; the effective per-fragment deadline is the full 30s call budget) under the unchanged 30-second per-call rail, with the hash-verified profile identity on every call. Fail-closed at both layers: a failing batch stops the document with a content-free typed failure and no partial Revision is ever published. A singleton batch that still exceeds the deadline yields a closed refusal for that document (BoundedCallTimedOutis currently sealed under theunsupported_document_shaperefusal category — a deliberate, disclosed conflation pending a dedicated category decision).One-hop expansion multi-anchor exclusion — Runtime authorization-path change. Migration
20260803_0055replaces the SECURITY DEFINER one-hop expansion function so the exclusion applies against the request's FULL anchor set (previously each candidate was only checked against its own anchor, so an authorized other main anchor could be returned and the Runtime guard atmaterialized.pyraised). The Runtime raise remains in place as defense-in-depth. The migration also addsGRANT DELETE ON file_publication_recoveryto the definer role. A live HTTP regression reproduces the original multi-anchor defect (verified to fail on the downgraded function and pass on the fixed one).Verification (builder + independent E-230 review): lint / strict typecheck / 2582 unit / 135 catalog + governance / integration slice green; warm-preload mutation kills 8 of 9 pinned tests; deployed function body byte-equivalent to the migration on the live database.
Coordinator note: this PR body was corrected after independent review found the original record understated the security-relevant migration and misstated the batch size.