supply: replay deterministic File change pages from opaque cursors - #82
Conversation
|
Warning Review limit reached
Next review available in: 33 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 ignored due to path filters (1)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughAdds a v3 File change feed with deterministic Markdown scanning, signed opaque cursors, bounded content-free pages, ContextControl acceptance, immutable PostgreSQL persistence, capability activation, and security/integration coverage. ChangesFile change feed
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FileChangeProvider
participant ContextControl
participant PostgreSQLControlStore
participant PostgreSQL
FileChangeProvider->>ContextControl: provide signed content-free ChangePage
ContextControl->>PostgreSQLControlStore: verify page and trusted operation
PostgreSQLControlStore->>PostgreSQL: atomically persist page, changes, checkpoint
PostgreSQL-->>PostgreSQLControlStore: committed page acceptance
PostgreSQLControlStore-->>ContextControl: return accepted page and cursor
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: b6f7004f3b
ℹ️ 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".
| "relative_path ~ '^[^/\\\\]+\\.[mM][dD]$' AND relative_path NOT IN ('.', '..')", | ||
| name="ck_file_source_change_markdown_path", |
There was a problem hiding this comment.
When a configured root contains a regular file named .md, FileImportPath and the new provider both accept and emit it, but this database regex requires at least one character before the .md suffix. Page acceptance therefore violates this check and is surfaced as SourceControlUnavailable, permanently preventing that source scan from advancing until the otherwise-valid file is renamed or removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2399121. The database constraint now accepts the exact shallow FileImportPath Markdown domain, including the minimal .md filename, with PostgreSQL regression coverage.
| observed.append((path, payload)) | ||
| return tuple(observed) |
There was a problem hiding this comment.
When files are renamed, deleted, or modified concurrently, each entry is validated only while that individual file is read. A previously observed file can change after its final os.stat while later files are processed, yet this method still returns ProviderOk; the resulting page may contain stale or nonexistent files and may describe a state that never existed atomically. Re-listing and rechecking all observed identities before returning would convert this case into the intended retryable-unavailable outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2399121. The provider relists the accepted directory membership and revalidates every accepted entry identity after completing the snapshot.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
tests/unit/test_file_change_control.py (1)
191-217: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider covering the other two fail-closed branches at this seam.
ContextControl.accept_file_change_pagealso refuses whenfile_change_proofs is Noneand whenpage.organization_id != call.organization_id, and both are cheap to assert here (expectSourceNotAvailable,store.accepted == []). They are the wrong-organization and missing-composition vetoes, so pinning them at the unit boundary is worth more than the usual coverage nit.🤖 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_file_change_control.py` around lines 191 - 217, Extend test_context_control_rejects_modified_provider_page_before_store to also exercise accept_file_change_page with file_change_proofs set to None and with a page whose organization_id differs from the authorized call. Assert SourceNotAvailable and store.accepted == [] for both cases, preserving the existing tampered-page coverage.adapters/file_source.py (1)
499-501: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCompare the pending-cursor signature in constant time.
Re-signing works (Ed25519 is deterministic), but
signature != expectedshort-circuits on the first differing byte, which is an avoidable timing oracle on a signature comparison. Exploitability is low because_unwrap_cursoralready validated the Control-signed envelope, so this is posture hardening rather than an open hole.🔒️ Proposed fix
+import hmacexpected = self._proofs._sign_pending_payload(payload) - if signature != expected: + if not hmac.compare_digest(signature, expected): return 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 `@adapters/file_source.py` around lines 499 - 501, Update the signature comparison in the pending-cursor validation path around _sign_pending_payload to use a constant-time comparison primitive instead of signature != expected, while preserving the existing return None behavior for mismatches.tests/integration/test_file_import_tracer.py (1)
1585-1585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe alembic head revision is hardcoded in two tests instead of referencing its source of truth. Both assertions duplicate the literal
"20260725_0028", so every future migration must touch every copy;tests/integration/test_migrations.pyalready keeps_HEAD_REVISIONfor exactly this purpose.
tests/integration/test_file_import_tracer.py#L1585-L1585: compare against the shared head-revision constant instead of the inline literal.tests/integration/test_zz_file_content_noop.py#L281-L281: replace the same inline literal with that shared constant.As per coding guidelines: "Never blindly delete repository-specific content, commit secrets or credentials, or hardcode volatile values; reference their source of truth instead."
🤖 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_file_import_tracer.py` at line 1585, Replace the hardcoded Alembic revision in the assertion at tests/integration/test_file_import_tracer.py:1585 with the shared _HEAD_REVISION constant from tests/integration/test_migrations.py, and make the same replacement at tests/integration/test_zz_file_content_noop.py:281. Ensure both tests reference that single source of truth for the expected head revision.Source: Coding guidelines
tests/unit/test_schema_security_manifest.py (2)
70-71: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the classification of the two new tables, not just their presence.
Membership in this set only proves the tables are declared. Their
tenant_ownedclassification stays unasserted, even thoughscripts/security_gate/rls.pymaps them as non-owner tenant tables andtests/integration/test_m0_security_gate_rls.pynow counts them intenantOwned: 52. Add them to the existing file-table loop (Lines 211-227) so a manifest regression toglobalwould fail here.💚 Proposed addition
for file_import_table in ( "exact_phrase_candidate", "file_acquisition", "file_acquisition_result", "file_import_job", "file_import_job_event", "file_publication_recovery", "file_resource_cleanup_intent", "file_resource_ingestion_guard", "file_revision_snapshot", "file_revision_replacement_plan", "file_revision_supersession", "file_source_acquisition_checkpoint", + "file_source_change", + "file_source_change_page", "file_source_cleanup_intent", "file_source_publish_watermark", "revision_publication_event", ): assert tables[file_import_table]["classification"] == "tenant_owned"🤖 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_schema_security_manifest.py` around lines 70 - 71, Extend the existing file-table classification loop in the schema security manifest test to include file_source_change and file_source_change_page. Assert that both tables are tenant_owned, preserving the current presence assertions while ensuring a regression to global classification fails.
725-744: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPresence-only checks weaken this constraint guard.
assert "readChanges" in expressionandassert capability_version in expressionhold no matter which availability each version declares, so this test can no longer catch v1 or v2 drifting to declarereadChanges/describeCapabilitiesavailable — which is exactly the drift the DB check constraint exists to prevent. Consider asserting the per-version pairing (v3 available, v1/v2 unavailable) instead of substring presence, e.g. by splitting the expression perdeclarationVersionbranch and asserting the value alongside each dimension.🤖 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_schema_security_manifest.py` around lines 725 - 744, The capability constraint test should verify each declarationVersion branch’s availability values rather than only checking dimension and version names are present. Update the assertions around capability_constraint["expression"] so file-capabilities-v1 and v2 explicitly mark readChanges and describeCapabilities unavailable, while v3 marks them available, preserving checks for the other required expression content.engine/persistence/control_sources.py (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReaching into a private symbol across module boundaries.
_accepted_cursor_payloadis the shared cursor-payload contract between the Control page module and this persistence layer, yet it is imported as a private name while everything else on that contract (AcceptedChangePage,ChangeCursor,FileChangeScanHead) comes through theengine.controlpublic surface. Consider promoting it (or a smallmint_accepted_cursorhelper that also performs the signing) so the payload format stays owned by one module.🤖 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/control_sources.py` around lines 42 - 44, Promote _accepted_cursor_payload to the public engine.control API, or expose a public mint_accepted_cursor helper that owns payload construction and signing, then update the persistence caller to use that public symbol instead of importing a private name from engine.control.file_change_pages. Keep the cursor-payload format centralized in the Control module.tests/unit/test_file_source_progress.py (1)
157-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the new progress invariants.
The field-order assertion now includes
change_scan_head, but none of the new fail-closed rules inengine/control/file_source_progress.pyare pinned here: change head without a checkpoint, head sequence exceeding the checkpoint,FILE_CHANGE_PAGErejected on a publish watermark, and page lineage rejecting publication refs. These are pure dataclass validations, so unit tests are the cheapest place to lock them down — today only the integration happy path exercises them.💚 Suggested additions
def test_change_scan_head_requires_and_never_exceeds_its_checkpoint() -> None: head = FileChangeScanHead( source_version_ref=VERSION_ID, scan_ref="a" * 64, scan_epoch=SCAN_EPOCH, page_limit=1, page_ref="b" * 64, checkpoint_ref="facp_" + "c" * 64, sequence=5, complete=False, ) with pytest.raises(ValueError, match="requires a checkpoint"): FileSourceProgress( organization_id=ORGANIZATION_ID, source_ref=SOURCE_REF, acquisition_checkpoint=None, publish_watermark=None, change_scan_head=head, ) def test_file_change_page_cannot_advance_a_publish_watermark() -> None: with pytest.raises(ValueError, match="cannot advance a publish watermark"): FileSourcePublishWatermark( sequence=1, watermark_ref="fpwm_" + "a" * 64, checkpoint_ref="facp_" + "b" * 64, change_kind=FileSourceChangeKind.FILE_CHANGE_PAGE, outcome=FileSourcePublishOutcome.PUBLISHED, acquisition_ref=None, job_ref=None, cleanup_intent_ref=None, resource_ref=RESOURCE_REF, revision_ref=REVISION_ID, event_ref=None, event_sequence=None, published_at=NOW, )🤖 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_file_source_progress.py` around lines 157 - 164, Extend test_progress_contracts_keep_checkpoint_and_watermark_semantics_separate coverage with unit tests for the new fail-closed validations in FileSourceProgress and FileSourcePublishWatermark: reject a change_scan_head without an acquisition checkpoint, reject a head sequence exceeding its checkpoint, reject FILE_CHANGE_PAGE from advancing a publish watermark, and reject page lineage containing publication references. Assert ValueError with the corresponding validation messages and reuse the existing test fixtures/constants.engine/control/file_source_progress.py (1)
245-256: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider the symmetric lineage check for
change_scan_head.The watermark path (Lines 263-270) rejects a same-sequence signal whose
checkpoint_refdisagrees with the acquisition checkpoint.change_scan_headgets no equivalent check, so a store row that returns a scan head at the checkpoint sequence but with a differentcheckpoint_refwould be accepted as a valid read model.♻️ Proposed tightening
if ( self.change_scan_head.sequence > self.acquisition_checkpoint.sequence ): raise ValueError("File Source change head exceeds its checkpoint") + if ( + self.change_scan_head.sequence + == self.acquisition_checkpoint.sequence + and self.change_scan_head.checkpoint_ref + != self.acquisition_checkpoint.checkpoint_ref + ): + raise ValueError("File Source change head lineage is invalid")🤖 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/control/file_source_progress.py` around lines 245 - 256, Add a lineage validation in the change_scan_head checks alongside the existing sequence comparison: when change_scan_head.sequence equals acquisition_checkpoint.sequence, require change_scan_head.checkpoint_ref to match acquisition_checkpoint.checkpoint_ref, otherwise raise the same invalid-head error used for inconsistent scan heads. Preserve the existing type, missing-checkpoint, and greater-sequence validations.
🤖 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/file_source.py`:
- Around line 306-321: The readChanges flow around _observe_markdown_files
currently re-reads and hashes every Markdown file on each page; cache the
complete observed snapshot for the duration of a scan, keyed by root_ref and
scan_ref, or reuse entries whose _file_identity is unchanged while re-hashing
only changed files. Ensure scan_ref still represents the entire root and
preserve the existing LookupError and RuntimeError handling.
- Around line 193-201: Update the name-processing flow around FileImportPath to
filter invalid or non-accepted names before sorting, allowing FileImportPath’s
ValueError handling to discard surrogate-containing filenames. Sort only the
surviving Markdown names by UTF-8 bytes, then process them as before; do not let
an unrelated invalid filename raise the RuntimeError.
In `@docs/decisions/0054-acknowledge-file-change-pages-before-cursor-advance.md`:
- Around line 9-15: Add the required ## Rationale section to ADR 0054 between
its existing Decision and Consequences sections, preserving the documented
baseline ADR section order and leaving the surrounding content unchanged.
In `@engine/control/file_change_pages.py`:
- Around line 233-237: Update the predecessor binding validation in the relevant
ChangePage initializer or validator so predecessor_page_ref,
predecessor_checkpoint_ref, and predecessor_sequence are either all present or
all absent. Replace the current pairwise condition with a check that rejects
every partially populated combination, preserving the existing ValueError for
incomplete bindings.
- Around line 247-252: Update the changes validation in the relevant
FileChangePage initializer or validator to reject tuples whose length exceeds
page_limit, while retaining the existing MAX_FILE_CHANGE_PAGE_SIZE and
SourceChange type checks. Raise the same contract TypeError before persistence
so oversized pages are rejected consistently with the page’s configured limit.
- Around line 564-599: Update UUID claim parsing in _unwrap_cursor so scanEpoch
and organizationId/sourceId/sourceVersionId are validated as strings before
calling UUID or string-based UUID checks. Ensure malformed types such as null,
numbers, or lists are handled by the existing failure contract and return None,
allowing callers to preserve the ProviderInvalidCheckpoint path.
In `@engine/persistence/schema_security_manifest.yaml`:
- Line 118: Update the file_source_publish_watermark.purpose text to describe
the watermark as the highest accepted publication-bearing sequence with no
earlier unresolved publication-bearing sequence, while noting that content-free
provider pages do not require a visibility outcome; remove the outdated “highest
contiguous acquisition sequence” wording.
- Around line 7244-7261: The schema currently permits multiple acquisition
checkpoints for the same accepted change page. Update the
organizationInclusiveKeys definition for file_source_acquisition_checkpoint and
its migration to enforce uniqueness on organization_id and change_page_ref, and
add matching down-migration cleanup for the new constraint.
In `@migrations/versions/20260725_0028_file_change_feed_activation.py`:
- Around line 460-470: Add REVOKE ALL ON FUNCTION
public.{_READ_PROGRESS}{_READ_PROGRESS_SIGNATURE} FROM PUBLIC immediately after
CREATE FUNCTION and before ALTER FUNCTION in both upgrade site
migrations/versions/20260725_0028_file_change_feed_activation.py#L460-L470 and
downgrade site
migrations/versions/20260725_0028_file_change_feed_activation.py#L908-L918,
preserving the existing owner and {_CONTROL} grant behavior.
In `@tests/catalog/test_validate_security_catalog.py`:
- Around line 1944-1959: Remove the duplicated ADR-0054 documentRefs assertion
block in the test around the existing document_refs checks, or restore the
distinct assertion it replaced; ensure each expected document reference is
asserted only once and the surrounding reconciliation assertion remains
unchanged.
In `@tests/integration/test_file_source_registration.py`:
- Around line 127-133: Scope the orphan cleanup in the disposable file-change
finalizer to the created user only, rather than deleting every membership-less
account. Update the finalizer registration around
_delete_disposable_file_change_organization to pass user_id, then use that value
in the DELETE condition alongside the existing membership check.
In `@tests/integration/test_migrations.py`:
- Around line 780-856: Add a separate migration test alongside
test_file_change_feed_revision_downgrades_and_reapplies_cleanly that preserves
non-empty file_source_change_page or file_source_change rows, calls
command.downgrade(..., "20260724_0027"), and asserts the expected RuntimeError.
Verify the downgrade is vetoed before schema assertions, then restore the
database to head so the test leaves the migration state unchanged.
In `@tests/unit/test_m0_rls_inventory.py`:
- Around line 48-49: Update NON_OWNER_EVIDENCE_BY_TABLE entries for
file_source_change and file_source_change_page to use PG-FILE-CHANGE-DENY-081
instead of the acceptance evidence. Update the corresponding manifest
nonOwnerEvidence values to match, ensuring the auditor uses the denial evidence
for table isolation.
---
Nitpick comments:
In `@adapters/file_source.py`:
- Around line 499-501: Update the signature comparison in the pending-cursor
validation path around _sign_pending_payload to use a constant-time comparison
primitive instead of signature != expected, while preserving the existing return
None behavior for mismatches.
In `@engine/control/file_source_progress.py`:
- Around line 245-256: Add a lineage validation in the change_scan_head checks
alongside the existing sequence comparison: when change_scan_head.sequence
equals acquisition_checkpoint.sequence, require change_scan_head.checkpoint_ref
to match acquisition_checkpoint.checkpoint_ref, otherwise raise the same
invalid-head error used for inconsistent scan heads. Preserve the existing type,
missing-checkpoint, and greater-sequence validations.
In `@engine/persistence/control_sources.py`:
- Around line 42-44: Promote _accepted_cursor_payload to the public
engine.control API, or expose a public mint_accepted_cursor helper that owns
payload construction and signing, then update the persistence caller to use that
public symbol instead of importing a private name from
engine.control.file_change_pages. Keep the cursor-payload format centralized in
the Control module.
In `@tests/integration/test_file_import_tracer.py`:
- Line 1585: Replace the hardcoded Alembic revision in the assertion at
tests/integration/test_file_import_tracer.py:1585 with the shared _HEAD_REVISION
constant from tests/integration/test_migrations.py, and make the same
replacement at tests/integration/test_zz_file_content_noop.py:281. Ensure both
tests reference that single source of truth for the expected head revision.
In `@tests/unit/test_file_change_control.py`:
- Around line 191-217: Extend
test_context_control_rejects_modified_provider_page_before_store to also
exercise accept_file_change_page with file_change_proofs set to None and with a
page whose organization_id differs from the authorized call. Assert
SourceNotAvailable and store.accepted == [] for both cases, preserving the
existing tampered-page coverage.
In `@tests/unit/test_file_source_progress.py`:
- Around line 157-164: Extend
test_progress_contracts_keep_checkpoint_and_watermark_semantics_separate
coverage with unit tests for the new fail-closed validations in
FileSourceProgress and FileSourcePublishWatermark: reject a change_scan_head
without an acquisition checkpoint, reject a head sequence exceeding its
checkpoint, reject FILE_CHANGE_PAGE from advancing a publish watermark, and
reject page lineage containing publication references. Assert ValueError with
the corresponding validation messages and reuse the existing test
fixtures/constants.
In `@tests/unit/test_schema_security_manifest.py`:
- Around line 70-71: Extend the existing file-table classification loop in the
schema security manifest test to include file_source_change and
file_source_change_page. Assert that both tables are tenant_owned, preserving
the current presence assertions while ensuring a regression to global
classification fails.
- Around line 725-744: The capability constraint test should verify each
declarationVersion branch’s availability values rather than only checking
dimension and version names are present. Update the assertions around
capability_constraint["expression"] so file-capabilities-v1 and v2 explicitly
mark readChanges and describeCapabilities unavailable, while v3 marks them
available, preserving checks for the other required expression content.
🪄 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: 9aa57513-8910-4e01-957c-0529a6751921
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
adapters/file_source.pydocs/decisions/0054-acknowledge-file-change-pages-before-cursor-advance.mddocs/decisions/README.mdengine/_opaque.pyengine/control/__init__.pyengine/control/authority.pyengine/control/contracts.pyengine/control/file_change_pages.pyengine/control/file_imports.pyengine/control/file_source_progress.pyengine/control/module.pyengine/persistence/control_sources.pyengine/persistence/schema_security_manifest.yamleval/catalogs/m0-security-evidence.schema.jsoneval/catalogs/m0-security-evidence.yamleval/catalogs/security-catalog.schema.jsoneval/catalogs/security-invariants.yamlmigrations/versions/20260725_0028_file_change_feed_activation.pypyproject.tomlscripts/security_gate/rls.pyscripts/validate_security_catalog.pytests/catalog/test_validate_security_catalog.pytests/integration/test_file_change_pages.pytests/integration/test_file_import_tracer.pytests/integration/test_file_source_registration.pytests/integration/test_m0_security_gate_rls.pytests/integration/test_migrations.pytests/integration/test_zz_file_content_noop.pytests/support/file_source_progress.pytests/unit/test_context_control.pytests/unit/test_file_change_control.pytests/unit/test_file_change_provider.pytests/unit/test_file_source_progress.pytests/unit/test_m0_rls_inventory.pytests/unit/test_schema_security_manifest.py
b6f7004 to
9f30a83
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f30a83fdf
ℹ️ 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".
| ordered_names = sorted(names, key=lambda value: value.encode("utf-8")) | ||
| except UnicodeEncodeError: | ||
| raise RuntimeError("File root observation is unstable") from None |
There was a problem hiding this comment.
On POSIX filesystems, os.listdir() decodes arbitrary byte filenames with surrogate escapes, so sorting a root containing an unrelated non-UTF-8 filename (for example b"\xff.txt") raises UnicodeEncodeError here before FileImportPath can filter it out. Every read_changes call then returns ProviderRetryableUnavailable indefinitely even though all Markdown files are valid; filter invalid/non-Markdown names before UTF-8 ordering, or enumerate bytes and explicitly discard names outside the public path domain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2399121. Out-of-domain undecodable names are discarded before UTF-8 ordering, while valid Markdown entries still produce a deterministic page.
9f30a83 to
2399121
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2399121388
ℹ️ 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".
| except LookupError: | ||
| raise RuntimeError("File root observation is unstable") from None |
There was a problem hiding this comment.
stometa, when a Markdown file is deleted or renamed after _read_regular() completes but before the following os.stat(), that call raises FileNotFoundError/OSError, while this handler catches only LookupError. The exception therefore escapes both _observe_markdown_files() and read_changes() instead of producing the documented ProviderRetryableUnavailable closed outcome; catch OSError for this revalidation step and translate it to the unstable-observation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 91a05cb. The post-read no-follow stat now translates OSError into the established unstable-snapshot path, and a deterministic deletion-race test proves ProviderRetryableUnavailable.
2399121 to
91a05cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
migrations/versions/20260725_0028_file_change_feed_activation.py (2)
639-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
latest_page_completeis selected but never used.It is declared at Line 520 and assigned here, then never read — the new-scan branch only compares
requested_superseded_scan_epochwithlatest_scan_epoch, and the continuation branch only compares the predecessor triple. Either drop it from theSELECT ... INTOand theDECLAREblock, or wire in the completeness invariant that was presumably intended (e.g. refusing supersession of a scan whose head page is already complete). As written a reader cannot tell which.♻️ Drop the dead binding if no invariant is intended
SELECT checkpoint.change_page_ref, checkpoint.checkpoint_ref, - checkpoint.sequence, page.complete, page.scan_epoch + checkpoint.sequence, page.scan_epoch INTO latest_change_page_ref, latest_checkpoint_ref, - latest_checkpoint_sequence, latest_page_complete, - latest_scan_epoch + latest_checkpoint_sequence, latest_scan_epoch🤖 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/20260725_0028_file_change_feed_activation.py` around lines 639 - 664, Remove the unused latest_page_complete binding from the declaration and the SELECT ... INTO statement in the file-change activation procedure, since neither the new-scan nor continuation validation uses it. Keep the existing scan-epoch and predecessor-triple checks unchanged.
616-636: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
FULL JOINhalf that detects extra supplied changes is unreachable.
change.organization_id,change.source_id, andchange.page_refare filtered in theWHEREclause, which runs after the outer join, so every supplied-only row (stored side all NULL) is discarded andchange.change_ordinal IS NULLcan never be true. The join therefore behaves as a LEFT JOIN from the stored side. It still catches divergence today becausepage.change_count = requested_change_count(Line 614) plus unique ordinals guarantee an unmatched stored row, but half of the comparison is dead. Moving the stored-side predicates into a derived table restores the intended symmetry (LATERALis also unnecessary here — the subquery only reads the function parameter).♻️ Make both outer-join halves reachable
AND NOT EXISTS ( - SELECT 1 FROM public.{_CHANGE} AS change - FULL JOIN LATERAL ( + SELECT 1 + FROM ( + SELECT change.change_ordinal, change.change_kind, + change.relative_path, change.content_sha256, + change.content_length + FROM public.{_CHANGE} AS change + WHERE change.organization_id = page.organization_id + AND change.source_id = page.source_id + AND change.page_ref = page.page_ref + ) AS stored + FULL JOIN ( SELECT ordinality::smallint AS ordinal, element->>'kind' AS kind, element->>'path' AS path, element->>'contentSha256' AS digest, (element->>'contentLength')::bigint AS length FROM pg_catalog.jsonb_array_elements(requested_changes) WITH ORDINALITY AS item(element, ordinality) ) AS supplied - ON supplied.ordinal = change.change_ordinal - AND supplied.kind = change.change_kind - AND supplied.path = change.relative_path - AND supplied.digest = change.content_sha256 - AND supplied.length = change.content_length - WHERE change.organization_id = page.organization_id - AND change.source_id = page.source_id - AND change.page_ref = page.page_ref - AND (change.change_ordinal IS NULL OR supplied.ordinal IS NULL) + ON supplied.ordinal = stored.change_ordinal + AND supplied.kind = stored.change_kind + AND supplied.path = stored.relative_path + AND supplied.digest = stored.content_sha256 + AND supplied.length = stored.content_length + WHERE stored.change_ordinal IS NULL + OR supplied.ordinal IS NULL );🤖 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/20260725_0028_file_change_feed_activation.py` around lines 616 - 636, Update the outer join in the change-comparison query so the organization_id, source_id, and page_ref filters are applied to a derived stored-change relation before joining, rather than in the post-join WHERE clause. Preserve the existing ordinal and field comparisons, remove the unnecessary LATERAL usage around requested_changes, and retain filtering that allows both unmatched stored rows and unmatched supplied rows to be detected.tests/support/migrations.py (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the head revision from Alembic instead of pinning the literal.
Alembic's script directory already owns this value, and a stale literal turns every head assertion into a silent false negative until someone remembers to bump it.
As per coding guidelines: "Never blindly delete repository-specific content, commit secrets or credentials, or hardcode volatile values; reference their source of truth instead."
♻️ Proposed refactor
"""Shared migration assertions for tests that require the current schema head.""" -HEAD_REVISION = "20260725_0028" +from pathlib import Path + +from alembic.config import Config +from alembic.script import ScriptDirectory + +_ALEMBIC_INI = Path(__file__).parents[2] / "alembic.ini" +HEAD_REVISION = ScriptDirectory.from_config(Config(_ALEMBIC_INI)).get_current_head()🤖 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/support/migrations.py` around lines 1 - 3, Replace the hardcoded HEAD_REVISION value in the migration assertion support with a value derived from Alembic’s script directory or configured migration source of truth. Update the associated imports and initialization so consumers of HEAD_REVISION continue receiving the current schema head automatically.Source: Coding guidelines
tests/unit/test_schema_security_manifest.py (1)
729-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese assertions no longer pin the capability invariant they replaced.
Asserting that a dimension name appears somewhere in the expression passes for any expression mentioning
readChanges, regardless of whether v1/v2 keep it unavailable and v3 makes it available. The two prose assertions then bind the test to comment wording, so a harmless rewording breaks the gate while a real availability inversion can pass. Prefer asserting the per-version availability values (as the previous per-dimension checks did) and keep at most one structural check on the version list.🤖 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_schema_security_manifest.py` around lines 729 - 756, Replace the broad dimension-name and prose-string assertions in the capability invariant test with per-version availability assertions for each relevant dimension, verifying v1/v2 are unavailable and v3 is available. Retain at most one structural assertion that the expression includes the supported capability versions, and remove checks tied to comment wording.tests/unit/test_file_change_provider.py (1)
114-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-UTF-8 name that ends in
.mdto actually cover the new surrogate rejection.
os.fsdecode(b"\xff.txt")is filtered by the.mdsuffix check, so the surrogate clause added inengine/control/file_imports.py(Line 47) is not exercised. Ab"\xff.md"entry is the case that previously passed validation and would then break theencode("utf-8")sort key in_observe_markdown_files.💚 Proposed test addition
with patch( "adapters.file_source.os.listdir", - return_value=["valid.md", os.fsdecode(b"\xff.txt")], + return_value=[ + "valid.md", + os.fsdecode(b"\xff.txt"), + os.fsdecode(b"\xff.md"), + ], ): outcome = provider.read_changes(_source(), InitialScan(), ChangeLimit(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 `@tests/unit/test_file_change_provider.py` around lines 114 - 134, Update test_scan_ignores_non_utf8_names_outside_the_file_import_domain to use a non-UTF-8 filename ending in “.md” (for example, decoded from b"\xff.md") in the mocked os.listdir result, while preserving the existing assertion that only valid.md is returned.tests/unit/test_file_change_control.py (1)
126-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnit tests reach into private proof/provider internals because no public sealing or cursor-minting seam exists.
FileChangeProviderProofsandFileChangeProviderexpose no supported way to mint a sealed page or a signed cursor, so both suites depend on private symbols and will break on any internal rename even when the public contract is unchanged.
tests/unit/test_file_change_control.py#L126-L158: replaceprovider._seal_page(unsigned)with a narrow public sealing seam onFileChangeProviderProofs.tests/unit/test_file_change_provider.py#L279-L286: replaceprovider._encode_cursor(...)(and the private_accepted_cursor_payloadimport at Line 37) with the same public seam once it exists.🤖 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_file_change_control.py` around lines 126 - 158, The tests depend on private proof and cursor internals because no supported public seam exists. Add narrow public sealing and cursor-minting APIs to FileChangeProviderProofs/FileChangeProvider, then update tests/unit/test_file_change_control.py:126-158 to use the public page-sealing seam instead of FileChangeProviderProofs._seal_page; update tests/unit/test_file_change_provider.py:279-286 to use the public cursor seam and remove its _accepted_cursor_payload import, preserving existing test behavior.
🤖 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 `@migrations/versions/20260725_0028_file_change_feed_activation.py`:
- Around line 396-418: Update the change-head and accepted-page resume logic so
the scan head always uses one consistent page_limit for a given scan_epoch.
Reuse the original FileChangeScanHead limit when available, or ensure both
initial and continuation paths derive it from the accepted page consistently; do
not allow a continuation’s differing page limit to alter subsequent reads.
In `@tests/integration/test_file_change_pages.py`:
- Around line 1038-1053: Wrap the downgrade assertion around command.downgrade
in a try/finally, and call command.upgrade with the existing configuration and
"head" in the finally block. Keep the subsequent version check and engine
cleanup unchanged so the shared database is restored even when the veto
assertion fails.
---
Nitpick comments:
In `@migrations/versions/20260725_0028_file_change_feed_activation.py`:
- Around line 639-664: Remove the unused latest_page_complete binding from the
declaration and the SELECT ... INTO statement in the file-change activation
procedure, since neither the new-scan nor continuation validation uses it. Keep
the existing scan-epoch and predecessor-triple checks unchanged.
- Around line 616-636: Update the outer join in the change-comparison query so
the organization_id, source_id, and page_ref filters are applied to a derived
stored-change relation before joining, rather than in the post-join WHERE
clause. Preserve the existing ordinal and field comparisons, remove the
unnecessary LATERAL usage around requested_changes, and retain filtering that
allows both unmatched stored rows and unmatched supplied rows to be detected.
In `@tests/support/migrations.py`:
- Around line 1-3: Replace the hardcoded HEAD_REVISION value in the migration
assertion support with a value derived from Alembic’s script directory or
configured migration source of truth. Update the associated imports and
initialization so consumers of HEAD_REVISION continue receiving the current
schema head automatically.
In `@tests/unit/test_file_change_control.py`:
- Around line 126-158: The tests depend on private proof and cursor internals
because no supported public seam exists. Add narrow public sealing and
cursor-minting APIs to FileChangeProviderProofs/FileChangeProvider, then update
tests/unit/test_file_change_control.py:126-158 to use the public page-sealing
seam instead of FileChangeProviderProofs._seal_page; update
tests/unit/test_file_change_provider.py:279-286 to use the public cursor seam
and remove its _accepted_cursor_payload import, preserving existing test
behavior.
In `@tests/unit/test_file_change_provider.py`:
- Around line 114-134: Update
test_scan_ignores_non_utf8_names_outside_the_file_import_domain to use a
non-UTF-8 filename ending in “.md” (for example, decoded from b"\xff.md") in the
mocked os.listdir result, while preserving the existing assertion that only
valid.md is returned.
In `@tests/unit/test_schema_security_manifest.py`:
- Around line 729-756: Replace the broad dimension-name and prose-string
assertions in the capability invariant test with per-version availability
assertions for each relevant dimension, verifying v1/v2 are unavailable and v3
is available. Retain at most one structural assertion that the expression
includes the supported capability versions, and remove checks tied to comment
wording.
🪄 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: ba61d582-310c-419f-9eb7-28c3462a25d0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
adapters/file_source.pydocs/decisions/0054-acknowledge-file-change-pages-before-cursor-advance.mddocs/decisions/README.mdengine/_opaque.pyengine/control/__init__.pyengine/control/authority.pyengine/control/contracts.pyengine/control/file_change_pages.pyengine/control/file_imports.pyengine/control/file_source_progress.pyengine/control/module.pyengine/persistence/control_sources.pyengine/persistence/schema_security_manifest.yamleval/catalogs/m0-security-evidence.schema.jsoneval/catalogs/m0-security-evidence.yamleval/catalogs/security-catalog.schema.jsoneval/catalogs/security-invariants.yamlmigrations/versions/20260725_0028_file_change_feed_activation.pypyproject.tomlscripts/security_gate/rls.pyscripts/validate_security_catalog.pytests/catalog/test_validate_security_catalog.pytests/integration/test_file_change_pages.pytests/integration/test_file_import_tracer.pytests/integration/test_file_source_registration.pytests/integration/test_m0_security_gate_rls.pytests/integration/test_migrations.pytests/integration/test_zz_file_content_noop.pytests/support/file_source_progress.pytests/support/migrations.pytests/unit/test_context_control.pytests/unit/test_file_change_control.pytests/unit/test_file_change_provider.pytests/unit/test_file_source_progress.pytests/unit/test_m0_rls_inventory.pytests/unit/test_schema_security_manifest.py
🚧 Files skipped from review as they are similar to previous changes (25)
- pyproject.toml
- engine/_opaque.py
- eval/catalogs/m0-security-evidence.schema.json
- scripts/security_gate/rls.py
- engine/control/authority.py
- docs/decisions/README.md
- tests/integration/test_m0_security_gate_rls.py
- tests/integration/test_file_import_tracer.py
- tests/support/file_source_progress.py
- tests/unit/test_m0_rls_inventory.py
- tests/unit/test_context_control.py
- engine/control/init.py
- tests/integration/test_zz_file_content_noop.py
- eval/catalogs/m0-security-evidence.yaml
- engine/control/contracts.py
- tests/catalog/test_validate_security_catalog.py
- docs/decisions/0054-acknowledge-file-change-pages-before-cursor-advance.md
- eval/catalogs/security-invariants.yaml
- engine/persistence/schema_security_manifest.yaml
- engine/persistence/control_sources.py
- eval/catalogs/security-catalog.schema.json
- engine/control/file_source_progress.py
- tests/integration/test_file_source_registration.py
- adapters/file_source.py
- engine/control/file_change_pages.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a05cbc51
ℹ️ 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".
| == _file_identity(after) | ||
| ): | ||
| raise RuntimeError("File root observation is unstable") | ||
| observed.append((path, payload)) |
There was a problem hiding this comment.
stometa, when a configured root contains multiple large but valid Markdown files, this list retains every payload simultaneously before read_changes hashes any of them. Because each file may be up to 64 MiB and the file count is unbounded, even ChangeLimit(1) can consume gigabytes and terminate the provider with an uncaught MemoryError; hash each stable file during observation and retain only its digest and length.
Useful? React with 👍 / 👎.
1 participant
Closes #81
Summary
describeCapabilities/readChangespagesFileImportPathdomain at the SECURITY DEFINER and immutable-table boundariesDefinition of Done
9f30a83fdf283776d2a18bd7e5be16b2a07ead2b: zero findingsmake db-reset && PATH=/Users/stometa/.nvm/versions/node/v22.12.0/bin:$PATH make checkM0 SECURITY PASSSummary by CodeRabbit
New Features
Documentation
Security