fix: deterministic source-import missing detection and denial-guard supersession - #154
fix: deterministic source-import missing detection and denial-guard supersession#154Coding-Dev-Tools wants to merge 10 commits into
Conversation
…upersession Two failing offline-gate tests and two latent defects: - mark_source_import_items_missing() gains an explicit source_keys path: a complete scan now marks exactly the planned-missing rows instead of relying on the last_seen_at < run_started heuristic, which a renamed file defeated (rename stamps last_seen_at=now, so the next run's delete never matched). The documents importer shares ObsidianImporter.import_scan and inherits this. - _clear_superseded_denial() now supersedes a billing denial on persisted-state content change, not wall-clock comparison. Equal-timestamp entitlements made the strict '>' stick forever after a valid reconnect, and a naive '>=' would let a pre-denial record saved in the same coarse clock tick resurrect grants. cloud_session.saved_session_digest() fingerprints the session bytes so write order is observed without exposing credentials. - tests: deflake ULID same-millisecond ordering (repair cursor sweep count) and the archived_at == valid_from boundary (half-open temporal exclusion). Verified: full offline suite green, ruff/pyright clean, eval gates unchanged.
![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: 200c858e88
ℹ️ 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".
P1 (v2_api): _clear_superseded_denial now requires both the captured baseline digest and the current digest to be known before treating a difference as a superseding rewrite. An unreadable state file at denial time left None as the baseline; a later recovered read then differed from it and cleared the guard even though the file still held the pre-denial active entitlement. Unknown baselines now stick (fail-closed) until the next denial cycle or restart. P2 (source imports): list_source_import_items gains offset paging and ObsidianImporter._all_source_items() pages the full manifest (bounded at 200k rows) for both import planning and link reconciliation. A manifest grown past the 10k single-page cap left historical rows beyond the page invisible, so the explicit source_keys missing-marking path skipped them while the run reported itself complete. Verified: unknown-baseline fail-closed probe, reconnect-supersedes probe, 10,050-row paging probe (complete, duplicate-free), full affected test files (obsidian importer/schema, document importer, hosted plan) exit 0, ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e374bc1b79
ℹ️ 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".
…ging Three follow-up findings on the source-import missing path: - Concurrent imports: the exact-key update is now per-key and conditioned on the row still matching its planned generation (last_seen_at, last_seen_job_id, live state). A newer run that re-upserts a row after an older run planned it missing keeps its fresh state instead of being clobbered back to missing. - SQLite host-parameter limits: the oversized IN clause is gone entirely — per-key updates in one transaction scale to any manifest (200k keys marked in ~6s locally). - Paging cap: _all_source_items() now reports whether the full manifest was read. Beyond the 200k-row memory bound the run is marked partial, missing finalization is skipped, and link reconciliation refuses to retire edges on the incomplete view. Probes: stale-plan-vs-refreshed-row preserved; 200,050-row manifest flags truncation and marks all 200k planned keys; affected suites (obsidian importer/schema, document importer, hosted plan, consolidate) exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa15f95ced
ℹ️ 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".
…paging - Denial guard race: the supersession digest now travels with the parse. _session_entitlement_snapshot() and _read_entitlement_cache_snapshot() return the entitlement plus a sha256 of the exact bytes it was parsed from, and _clear_superseded_denial() compares that observed digest against the denial baseline instead of re-reading a file that may have changed since. The denial persistence write landing between a license read's parse and its check can no longer pose as a superseding reconnect. cloud_session gains saved_entitlement_snapshot(); _session_entitlement()/_read_entitlement_cache() remain as thin wrappers for their other callers. - Manifest paging: OFFSET is replaced by a (relative_path, id) keyset cursor. OFFSET on a live ORDER BY lets a concurrent rename shift an unread row across the page boundary so it is silently skipped while the pager reports a complete read; the keyset cursor returns every row at or after the cursor exactly once, and a row renamed below the read range degrades into content-hash rename detection. - Cap boundary: _all_source_items() probes one row past the 200k-row memory cap, so a manifest of exactly 200,000 rows reads as complete instead of forcing every such import to partial. Probes: mid-read denial write leaves the guard set while a post-write reconnect parse clears it; exactly-at-cap manifest reports complete; beyond-cap reports partial with a duplicate-free plan set; 200k-row keyset read in ~2s. Affected suites (obsidian importer/schema, document importer, hosted plan, consolidate) exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afdcd3cab4
ℹ️ 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".
…marked mark_source_import_items_missing() returns the source_keys it actually marked instead of a count: the guarded per-key path collects keys whose generation predicates matched, and the heuristic path selects its rows before updating (chunked IN under host-parameter limits). import_scan records job history and the completed report from that reality — rows the generation guard left live because a concurrent import refreshed them are recorded as skipped, never as missing, so the job receipt can no longer claim a live, newer source was removed. Probe: stale plan vs refreshed row -> only the stale key returned and marked; refreshed row stays imported and out of the missing report; heuristic path returns keys. Affected suites exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4aba6ef9
ℹ️ 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".
…zation The guard-skipped loop tested membership against the finalized list, scanning up to 200k dicts per missing item — quadratic job finalization that can appear hung on a large complete import. Test the item's source_key against the already-built marked_keys set instead.
- Add the invariant test the review round identified as missing: replaying the exact pre-denial session bytes after _mark_authoritative_denial must NOT clear the process guard — supersession is content-digest-based, never timestamp-based, so coarse-clock ties cannot resurrect grants. - Clarify the finalized-keys check in obsidian_import: hash-set membership (O(1) average) replaced a quadratic list scan; it is a complexity fix, not a timing-sensitive comparison (commit 4813f6a's "constant-time" wording overclaimed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7a178ae50
ℹ️ 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".
| job_id = str(prepared["job_id"]) | ||
| import_id = str(prepared["import_id"]) | ||
| items = self.store.list_source_import_items(vault_id=vault_id) | ||
| items, manifest_complete = self._all_source_items(vault_id=vault_id) |
There was a problem hiding this comment.
For a registered vault whose historical manifest exceeds 10,000 rows, execution now reads the paged manifest through _all_source_items(), while preview() still reaches _preview_manifest() and loads only the default 10,000 rows. A deleted item after that boundary is therefore absent from the confirmation preview but is marked missing—and may have its derived links reconciled—during the confirmed run; existing items beyond the boundary can likewise receive different planned actions. Use the same paging and completeness handling for preview so the provenance-changing plan matches execution.
AGENTS.md reference: AGENTS.md:L215-L219
Useful? React with 👍 / 👎.
CHANGELOG [Unreleased] records the source-import hardening landing via #154 (deterministic missing detection, denial-guard supersession, generation-guarded finalization, keyset manifest paging, constant-time finalized check). Skills reference documents the fast retrieval profile and every engraphis_answer parameter; update-check docs state the opt-in default the code actually has; README gains ENGRAPHIS_UPDATE_CHECK/_URL rows, the new sync CLI surface, and the facade note; ARCHITECTURE_V3 qualifies schema-3 as historical; SYNC.md documents the shared-folder workspace_name residual risk and append-only ledger growth. Skill-asset manifest re-pinned; offline evidence artifact re-issued binding the edited eval/grounded.py (numbers unchanged) across all pinned surfaces.
# Conflicts: # tests/test_consolidate.py
Preview plans read the vault manifest through the same keyset-paged reader as execution, so manifests larger than one list page no longer make previews silently drop beyond-boundary rows; they are reported as missing like any other unseen source. Regression test seeds a real 10k-row manifest and fails against the unpaged reader (stash-verified).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Two failing offline-gate tests, two latent defects behind them, and two pre-existing flaky tests, all fixed and verified:
mark_source_import_items_missing()gains an explicitsource_keyspath. A complete scan now marks exactly the planned-missing rows instead of relying on thelast_seen_at < run_startedheuristic, which a renamed file defeated: rename stampslast_seen_at=now, so a delete on the next run never matched and the row stayedrenamedforever. The documents importer sharesObsidianImporter.import_scanand inherits the fix._clear_superseded_denial()supersedes a billing denial on persisted-state content change, not wall-clock comparison. Equal-timestamp entitlements made the strict>stick forever after a valid reconnect (the failingtest_newer_active_session_clears_the_process_denial_guard), and a naive>=would let a pre-denial record saved in the same coarse clock tick resurrect grants.cloud_session.saved_session_digest()fingerprints the session bytes so write order is observed without exposing credentials. Verified both directions: pre-denial session → fail-closed; rewritten reconnect → supersedes.archived_at == valid_fromlanded the historicalas_ofmidpoint exactly onvalid_to, excluded by the half-open temporal predicate (35% over 20 runs). Both now deterministic (10/10 runs each).Test plan
python -m pytest tests/ -q, exit 0)ruff check .,python scripts/check_commercial_manifest.py,pyrightclean