supply: detect deleted File paths in durable change pages - #86
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds file-capabilities-v4 delete observations based on bounded complete-scan baselines. Provider diffs emit deterministic upsert/delete pages, persistence validates and stores observation lineage, and tests verify replay, isolation, migration safety, and zero tombstone or publication effects. ChangesFile deletion observation flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FileChangeProvider
participant ContextControl
participant PostgreSQLControlStore
participant DeleteObservationPage
participant RuntimeState
FileChangeProvider->>ContextControl: emit baseline-bound delete page
ContextControl->>PostgreSQLControlStore: accept authenticated page
PostgreSQLControlStore->>DeleteObservationPage: validate and persist observation
DeleteObservationPage-->>PostgreSQLControlStore: accepted cursor and checkpoint
PostgreSQLControlStore-->>ContextControl: replayable page
DeleteObservationPage-->>RuntimeState: no tombstone or visibility mutation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 59bae4b694
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
engine/control/module.py (1)
133-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
activate_file_delete_observationsmissing fromrequired_methods.Unlike
activate_file_change_feed(unconditionally required here), the newactivate_file_delete_observationscapability — exposed unconditionally byContextControlwith no constructor gate — isn't added torequired_methods. A store lacking it will only fail at first call (converted to a genericSourceControlUnavailableby the blindexcept Exceptionat line 289) instead of failing fast at construction with a clearTypeError, breaking the fail-fast guarantee this list exists to provide.♻️ Proposed fix
required_methods = [ "activate_file_change_feed", + "activate_file_delete_observations", "offboard_file_source", "prepare_file_import", "register_file_source", "read_source", "read_file_source_progress", "tombstone_file_resource", ]🤖 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/module.py` around lines 133 - 141, Update the required_methods list in the control-module initialization validation to include activate_file_delete_observations, matching the unconditional capability exposed by ContextControl and preserving fail-fast construction-time validation for stores that do not implement it.engine/control/contracts.py (1)
106-182: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
delete_observationsisn't constrained for v1/v2 manifests.The new
delete_observationsfield is only validated inside thedeclaration_version in {"file-capabilities-v3", "file-capabilities-v4"}branch (lines 160-165). Forfile-capabilities-v1/v2, neither thealways_unavailable_statusestuple nor the v1/v2 status list (lines 169-182) includesdelete_observations, so a manifest declared as v1/v2 withdelete_observations=CapabilityStatus.AVAILABLEpasses__post_init__even though this capability should never be exposed pre-v4. This weakens the "recognized snapshot" exactness guarantee this validation exists to enforce.🛡️ Proposed fix
or ( self.declaration_version in {"file-capabilities-v1", "file-capabilities-v2"} and any( status is not CapabilityStatus.UNAVAILABLE for status in ( self.cursor_semantics, self.checkpoint_semantics, self.batch_limits, self.describe_capabilities, self.read_changes, self.checkpoint, + self.delete_observations, ) ) )🤖 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/contracts.py` around lines 106 - 182, Update the validation in __post_init__ for declaration_version values "file-capabilities-v1" and "file-capabilities-v2" so delete_observations must be CapabilityStatus.UNAVAILABLE. Include it in the existing pre-v3 status validation rather than changing the v3/v4 rules or the always_unavailable_statuses set.
🧹 Nitpick comments (5)
engine/persistence/control_sources.py (1)
455-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the v4 discriminator from the manifest instead of a repeated literal.
"file-capabilities-v4"is hardcoded twice whileFILE_DELETE_OBSERVATION_CAPABILITY_MANIFEST(already imported) carries the samedeclaration_version. Binding to the manifest keeps the branch and the stored capability document from drifting apart.♻️ Suggested tightening
- function_name = ( - "context_control_accept_file_delete_observation_page" - if value.capability_version == "file-capabilities-v4" - else "context_control_accept_file_change_page" - ) - baseline_argument = ( - ", CAST(:baseline AS jsonb)" - if value.capability_version == "file-capabilities-v4" - else "" - ) + delete_observations = ( + value.capability_version + == FILE_DELETE_OBSERVATION_CAPABILITY_MANIFEST.declaration_version + ) + function_name = ( + "context_control_accept_file_delete_observation_page" + if delete_observations + else "context_control_accept_file_change_page" + ) + baseline_argument = ( + ", CAST(:baseline AS jsonb)" if delete_observations else "" + )🤖 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 455 - 464, Update the capability-version checks used to select function_name and baseline_argument so they compare against FILE_DELETE_OBSERVATION_CAPABILITY_MANIFEST.declaration_version instead of repeating the "file-capabilities-v4" literal, keeping both branches aligned with the imported manifest.tests/unit/test_schema_security_manifest.py (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new table's security properties, not just its presence.
file_source_delete_observation_pageis added to the name set, but nothing here pins its classification, forced RLS,nonOwnerEvidence.evidenceId, or definer-only grants — andscripts/security_gate/rls.pyLine 48 now depends on that evidence id being exactlyPG-FILE-DELETE-PAGE-085. Neighbouring tables get explicit assertions (Lines 170-171), so mirroring them keeps a manifest edit from silently loosening the new table.💚 Suggested assertions
assert tables["file_source_change_page"]["classification"] == "tenant_owned" assert tables["file_source_change"]["classification"] == "tenant_owned" + binding = tables["file_source_delete_observation_page"] + assert binding["classification"] == "tenant_owned" + assert binding["nonOwnerEvidence"]["evidenceId"] == "PG-FILE-DELETE-PAGE-085" + assert binding["rowLevelSecurity"]["enabled"] is True + assert binding["rowLevelSecurity"]["forced"] is True + assert binding["permittedOperations"]["context_engine_runtime"] == [] + assert binding["permittedOperations"]["context_engine_worker"] == [] + assert binding["permittedOperations"][ + "context_engine_worker_lease_definer" + ] == ["SELECT", "INSERT", "DELETE"]🤖 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` at line 72, Extend the assertions for file_source_delete_observation_page in the schema security manifest tests to verify its classification, forced RLS, nonOwnerEvidence.evidenceId equals PG-FILE-DELETE-PAGE-085, and definer-only grants. Mirror the explicit security-property assertions used by neighboring table tests rather than checking only membership in the table-name set.tests/unit/test_context_control.py (1)
384-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an unauthorized-path test for delete-observation activation.
This test only exercises operators who already have
ControlOperation.ACTIVATE_FILE_DELETE_OBSERVATIONS. Add a table-driven/unit test asserting that a call without that operation is refused before touching the store, ideally parameterized overControlOperationso new authorities are covered automatically.🤖 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_context_control.py` around lines 384 - 429, Add a table-driven unit test near test_authorized_operator_explicitly_activates_delete_observations that attempts activate_file_delete_observations without ControlOperation.ACTIVATE_FILE_DELETE_OBSERVATIONS, parameterized across available operations. Assert authorization is refused and the store remains untouched, covering each unauthorized authority configuration.tests/integration/test_file_change_pages.py (1)
414-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a concrete
Connectiontype overAny.
sqlalchemy.Connectionis already imported-adjacent in this module (Engine,text); annotating it keeps the helper type-checked.♻️ Proposed annotation
-def _delete_observation_effect_snapshot( - connection: Any, - organization_id: UUID, -) -> tuple[object, ...]: +def _delete_observation_effect_snapshot( + connection: Connection, + organization_id: UUID, +) -> tuple[object, ...]:🤖 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_change_pages.py` around lines 414 - 418, Update the connection parameter annotation in _delete_observation_effect_snapshot from Any to SQLAlchemy’s concrete Connection type, using the existing SQLAlchemy imports or adding the required import while leaving the helper behavior unchanged.adapters/file_source.py (1)
474-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-obvious idempotence trick in
_changes()deserves a comment.When nothing has changed since the baseline, this returns
baseline.reference.comparison_baseline_ref(the baseline's own parent) rather thanbaseline.referenceitself. This is intentional — it reproduces the exact same_scan_refinputs as the prior scan so repeated no-op scans hash to the samescan_ref— but it's subtle enough that a maintainer could easily "fix" it tobaseline.referenceand silently break scan_ref stability for unchanged scans. A short comment explaining the rationale would help.🤖 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 474 - 529, Add a concise comment in `_changes()` immediately before the unchanged-baseline return, explaining that using `baseline.reference.comparison_baseline_ref` preserves the prior `_scan_ref` inputs and keeps repeated no-op scans idempotent. Do not alter the existing return 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 `@engine/persistence/control_sources.py`:
- Around line 929-951: Update _complete_change_baseline to sort the collected
baseline entries by their canonical UTF-8 byte-sorted path before constructing
FileChangeBaseline, ensuring multi-page SQL result ordering cannot violate
FileChangeBaseline.__post_init__ requirements. Preserve filtering of rows
without baseline_entry_kind and pass the sorted entries as the tuple.
In `@engine/persistence/schema_security_manifest.yaml`:
- Around line 7237-7238: Update the schema manifest entry for
fk_file_source_delete_observation_page_exact to record that the foreign key is
deferrable and initially deferred, matching the migration declaration. Leave the
related baseline constraint unchanged unless its migration also declares the
same characteristic.
In `@migrations/versions/20260725_0030_file_delete_observations.py`:
- Around line 234-249: Update the capability trigger around the two lookups
after set_config so it captures the caller’s existing app.organization_id,
restores it immediately after both SELECTs, and also restores it before
re-raising from any RAISE or exception path. Ensure the trigger never leaves
NEW.organization_id as the caller’s transaction-local tenant context.
---
Outside diff comments:
In `@engine/control/contracts.py`:
- Around line 106-182: Update the validation in __post_init__ for
declaration_version values "file-capabilities-v1" and "file-capabilities-v2" so
delete_observations must be CapabilityStatus.UNAVAILABLE. Include it in the
existing pre-v3 status validation rather than changing the v3/v4 rules or the
always_unavailable_statuses set.
In `@engine/control/module.py`:
- Around line 133-141: Update the required_methods list in the control-module
initialization validation to include activate_file_delete_observations, matching
the unconditional capability exposed by ContextControl and preserving fail-fast
construction-time validation for stores that do not implement it.
---
Nitpick comments:
In `@adapters/file_source.py`:
- Around line 474-529: Add a concise comment in `_changes()` immediately before
the unchanged-baseline return, explaining that using
`baseline.reference.comparison_baseline_ref` preserves the prior `_scan_ref`
inputs and keeps repeated no-op scans idempotent. Do not alter the existing
return behavior.
In `@engine/persistence/control_sources.py`:
- Around line 455-464: Update the capability-version checks used to select
function_name and baseline_argument so they compare against
FILE_DELETE_OBSERVATION_CAPABILITY_MANIFEST.declaration_version instead of
repeating the "file-capabilities-v4" literal, keeping both branches aligned with
the imported manifest.
In `@tests/integration/test_file_change_pages.py`:
- Around line 414-418: Update the connection parameter annotation in
_delete_observation_effect_snapshot from Any to SQLAlchemy’s concrete Connection
type, using the existing SQLAlchemy imports or adding the required import while
leaving the helper behavior unchanged.
In `@tests/unit/test_context_control.py`:
- Around line 384-429: Add a table-driven unit test near
test_authorized_operator_explicitly_activates_delete_observations that attempts
activate_file_delete_observations without
ControlOperation.ACTIVATE_FILE_DELETE_OBSERVATIONS, parameterized across
available operations. Assert authorization is refused and the store remains
untouched, covering each unauthorized authority configuration.
In `@tests/unit/test_schema_security_manifest.py`:
- Line 72: Extend the assertions for file_source_delete_observation_page in the
schema security manifest tests to verify its classification, forced RLS,
nonOwnerEvidence.evidenceId equals PG-FILE-DELETE-PAGE-085, and definer-only
grants. Mirror the explicit security-property assertions used by neighboring
table tests rather than checking only membership in the table-name set.
🪄 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: f221452f-722a-4326-a2de-0c0bf5117c2c
📒 Files selected for processing (31)
CONTEXT.mdadapters/file_source.pydocs/decisions/0056-detect-file-deletions-without-tombstone-authority.mddocs/security/context-engine-threat-model.mdengine/control/__init__.pyengine/control/authority.pyengine/control/contracts.pyengine/control/file_change_pages.pyengine/control/file_source_progress.pyengine/control/module.pyengine/persistence/control_sources.pyengine/persistence/schema_security_manifest.yamleval/catalogs/m0-security-evidence.yamleval/catalogs/security-catalog.schema.jsoneval/catalogs/security-invariants.yamlmigrations/versions/20260725_0030_file_delete_observations.pyscripts/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_m0_security_gate_rls.pytests/integration/test_migrations.pytests/integration/test_z_egress_grant_file.pytests/support/file_source_progress.pytests/support/migrations.pytests/unit/test_context_control.pytests/unit/test_file_delete_observation_contracts.pytests/unit/test_file_source_progress.pytests/unit/test_m0_rls_inventory.pytests/unit/test_schema_security_manifest.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 627ba46e8f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6ea0d5b8b
ℹ️ 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".
|
@codex review The incomplete-superseding-scan P1 is fixed at bf73298. A no-op snapshot reuses the prior comparison parent only when the complete baseline is also the exact complete durable head. Otherwise it binds the new scan to the latest complete baseline and supersedes the current incomplete head. Fresh local DoD: 1271 unit, 128 catalog, 6 process smoke, 397 real-PostgreSQL integration with 4 deselected, and 144 M0 security tests; M0 SECURITY PASS. Independent spec and standards reviews both PASS. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Closes #85
Outcome
Verification
make db-resetmake check(locked Node 22.12.0 / npm 10.9.0)M0 SECURITY PASSSummary by CodeRabbit