test(postgres): skip the bind-site tests when the postgres extra is absent — main is red - #464
Merged
Merged
Conversation
…bsent main is red. The four bind-site tests added in #453 call real collection methods that compose SQL through psycopg, and the fixture imported psycopg unconditionally. The test-linux CI job installs mempalace without the postgres extra, so all three matrix legs failed with: ModuleNotFoundError: No module named 'psycopg' tests/test_postgres_unstorable_bytes.py:202: in _real_sql It passed locally because this workstation's venv has the extra installed -- the gap is between the dev environment and the CI matrix, not between branches, which is why the PR run looked clean enough to merge. pytest.importorskip on psycopg.sql skips exactly those four and leaves the other seventeen running everywhere. That is the right split: the scrub behaviour under test is pure string/dict logic and needs no driver; only the "did this actually reach the bind" assertions do, and test-postgres covers that leg with the extra installed. Verified the mechanism directly -- importorskip inside a fixture yields a skip, not a collection error. The shadow-module trick normally used to prove this cannot work in this repo: mempalace/__init__ filters leaked PYTHONPATH entries out of sys.path (the behaviour #454 is about), so a fake psycopg placed on PYTHONPATH is removed before the import is attempted. Part of #411 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 |
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…extra Same trap as #464: the fixture composes real SQL through psycopg, and the test-linux CI job installs mempalace without the postgres extra. Applied to this PR's own reconnect tests, and #464's fix to the file inherited from main is picked up here so this branch's CI is green before the two land in either order (the duplicate drops cleanly on rebase once #464 merges). Part of #385 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
main is red: the bind-site tests inherited from #453 import psycopg unconditionally and the test-linux job installs mempalace without the postgres extra. Carried here so this branch's CI is green before the two land in either order; the duplicate drops cleanly on rebase once #464 merges. Part of #413 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
Entry + the three renderers, plus the README/CLAUDE test count moved 6815 -> 6822 for the 7 new cases. Rebased onto main after #453 and #464 landed; the entry's commit hash tracks the rebased commit so check-docs's hash-resolution check stays green. Part of #385 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
- fork-changes entry hash fbc5d34 -> 28518f2 (rebase rewrote it). - entry body records the transcript staleness-caveat rule and the measurement behind it (48 production transcript hits: 15/21/12). - test count 61 -> 65 for this branch; README + CLAUDE.md re-derived on the rebased tree, 6876 -> 6880. - all three renderers re-run; scripts/check-docs.sh clean 7/7. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
Entry + the three renderers, plus the README/CLAUDE test count moved 6815 -> 6821 for the 6 new cases. Rebased onto main after #453 and #464 landed; the entry's commit hash tracks the rebased commit so check-docs's hash-resolution check stays green. Part of #413 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
- fork-changes entry hash fbc5d34 -> 28518f2 (rebase rewrote it). - entry body records the transcript staleness-caveat rule and the measurement behind it (48 production transcript hits: 15/21/12). - test count 61 -> 65 for this branch; README + CLAUDE.md re-derived on the rebased tree, 6876 -> 6880. - all three renderers re-run; scripts/check-docs.sh clean 7/7. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
Entry + the three renderers, plus the README/CLAUDE test count moved 6815 -> 6822 for the 7 new cases. Rebased onto main after #453 and #464 landed; the entry's commit hash tracks the rebased commit so check-docs's hash-resolution check stays green. The entry's retry-safety paragraph carries the reviewer's correction with the code: a connection error does not imply the statement never ran, so the justification is the per-statement idempotency invariant, not autocommit. Part of #385 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 11, 2026
…it (#452) * feat(search): source_kind + source_stale provenance on every search hit A palace search returns the indexed copy of whatever was mined, and transcripts are the only thing mined continuously (Stop / PreCompact hooks) — curated project documents are re-indexed only when someone runs `mempalace mine` by hand. So the copy that comes back for a project fact is usually a session transcript quoting a claim, never the document that later corrected it, and nothing on the hit said which of the two the reader was holding. Measured: 2g/CLAUDE.md added `FIVE HANDSETS REFUSE THIS NETWORK` on 2026-09-03 and a REFUTED banner on 2026-09-05; the indexed card is from 2026-09-01 and contains neither. A wing-wide keyword query at limit 40 returned 21 hits — 20 .jsonl, 1 diary, 0 CLAUDE.md. Two peer sessions read the transcript copy as current. New `mempalace/provenance.py` — pure predicates, one os.stat: source_kind(hit) transcript | memory | diary | file | unknown source_stale(hit) True/False/None; None means undecidable, which is NOT a denial — a daemon hit carries a basename only, so it can never be located annotate(results) stamps both in place; idempotent, never raises provenance_note(hit) the human caveat all_transcript() / no_curated_source() for the header line Wired into every surface the fleet reads: the CLI's `_daemon_search_fast` / `_daemon_search_hybrid` / MCP-envelope routes, all three `searcher` result-assembly sites (so MCP `mempalace_search` carries the fields from the daemon host), and the table / compact / header renderers. JSON output carries `source_kind` and, where decidable, `source_stale` + `source_indexed_at`. `auto_query.runner._is_curated` now delegates to `source_kind(...) == "memory"` — the ranking that prefers curated hits and the caveat the CLI prints must not be able to disagree about which hits are curated. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork-changes): search source provenance entry Renders FORK_CHANGELOG.md, the README fork-change-queue table, website/public/llms-full.txt and the python-api reference (new provenance.md page). README + CLAUDE.md test counts re-derived: 6794 -> 6855 (61 new tests). Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(provenance): correct the basename-only claim; note tzinfo is load-bearing Review correction. Two docstring claims were wrong, and one load-bearing dependency was undocumented. 1. "daemon/MCP hits carry a basename only and can never be located on disk" is false. `searcher` writes the basename to `source_file` for display and keeps the FULL path in `source_path`; only `_source_file_full` / `_chunk_index` are stripped before a public return, so most hits carry both and staleness DOES resolve. The one real exception is `_bm25_only_via_postgres`, which sets no `source_path`. Verified against production via the MCP envelope route: hits come back with `source_path` and `created_at`, and `source_stale` resolves. Documented with it: which host's filesystem answers. Under MCP that is the palace host, and its copy is the right comparison — it is the copy the miner read. The CLI then re-annotates locally, and since `annotate` writes `source_stale` only when it can decide, a second host refines the answer rather than clobbering it with None. Where the file is Syncthing-replicated both agree (mtime is preserved); missing on both yields None. 2. `parse_date_bound` dropping tzinfo is load-bearing, not tidiness. Production `filed_at` is naive but `diary_ingest` writes an aware UTC one, and both downstream comparisons are against naive datetimes (`datetime.fromtimestamp`, `datetime.now()`). An aware value reaching either raises TypeError from inside a search the caller already paid for — normalising on the way in is what lets `source_stale` promise it never raises. Also states the `now` parameter's naive precondition, which the "never raises" contract depends on. Docstrings and comments only — no behaviour change. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: re-render after rebase onto main (#453) Post-rebase fixups, no prose changes: - fork-changes entry commit hash 19b84d0 -> fbc5d34 (the rebase rewrote it; a stale hash resolves via git cat-file but points off-branch). - test count re-derived on the rebased tree: 6815 -> 6876 in README.md and CLAUDE.md (#453 landed 21, this branch adds 61). - all three renderers re-run; scripts/check-docs.sh clean 7/7. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(search): don't render the staleness caveat on transcripts Measured over 48 production transcript hits: 15 stale, 21 not, 12 undecidable. The 31% is real but it is mostly an artefact of what a transcript IS — a live session's file is appended to continuously, so any wing with an open session reads stale. The note would fire on every open session while saying nothing the transcript caveat already says ("quoted copy … verify at the curated source"). Staleness is the interesting signal for a CURATED document, which is the case #451 was actually filed about: a project CLAUDE.md that grew a REFUTED banner after its drawer was indexed. So `provenance_note` emits only the transcript wording for a stale transcript, and the compact tag drops the ⟨stale⟩ half for transcripts to match. Curated files and memory files are unaffected and still carry both. This is a RENDERING rule, not a measurement one — `annotate` still stamps `source_stale` on every kind, so JSON and MCP consumers keep the raw field and can apply their own policy. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: re-render after rebase onto main (#464) - fork-changes entry hash fbc5d34 -> 28518f2 (rebase rewrote it). - entry body records the transcript staleness-caveat rule and the measurement behind it (48 production transcript hits: 15/21/12). - test count 61 -> 65 for this branch; README + CLAUDE.md re-derived on the rebased tree, 6876 -> 6880. - all three renderers re-run; scripts/check-docs.sh clean 7/7. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: re-render after rebase onto main (#455) - fork-changes entry hash 28518f2 -> 5632dc2, and verified with `git merge-base --is-ancestor <hash> HEAD` rather than existence alone — the check-docs gap filed as #472. - test count re-derived on the rebased tree: 6880 -> 6916. - all three renderers re-run; scripts/check-docs.sh clean 7/7. Part of #451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
main is red, and this fixes it
The four bind-site tests added in #453 (issue #411) call real
PostgresCollectionmethods that compose SQL through psycopg, and the fixture imported psycopg unconditionally. Thetest-linuxjob installs mempalace without thepostgresextra, so all three matrix legs fail:Failing:
test_update_path_binds_scrubbed_metadata,test_insert_path_binds_scrubbed_id,test_get_binds_scrubbed_ids,test_delete_binds_scrubbed_ids— on main, and on every branch based on it.Why it got through
It passes locally because this workstation's venv has the postgres extra installed. The gap is between the dev environment and the CI matrix, not between branches — my pre-PR full-suite run was green and could not have caught it. Mine, and I should have checked the job's install step before adding a test that composes real SQL.
The fix
pytest.importorskip("psycopg.sql")in the fixture skips exactly those four and leaves the other seventeen running everywhere. That is the right split: the scrub behaviour under test is pure string/dict logic and needs no driver; only the "did this actually reach the bind" assertions do, and thetest-postgresjob covers that leg with the extra installed.Verification
The mechanism is verified directly —
importorskipinside a fixture yields a skip, not a collection error. Worth recording why the usual proof is unavailable here: the shadow-module trick (a fakepsycopgonPYTHONPATHthat raisesImportError) cannot work in this repo, becausemempalace/__init__filters leakedPYTHONPATHentries out ofsys.path— the behaviour #454 is about — so the fake is removed before the import is attempted. CI is the positive control.Locally: 21 passed with the extra present.
ruff checkandruff format --checkclean. No source change, no docs entry (a test-harness repair, not a fork-ahead behaviour change).Please merge this before #460 and #463 — both inherit the broken file from main.
Part of #411