Add general feedback issue template#10
Merged
Merged
Conversation
Lightweight template for first impressions, usability observations, and use case ideas. Adds memory prompts link to config.yml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
19 tasks
9 tasks
cmeans-claude-dev Bot
added a commit
that referenced
this pull request
Apr 22, 2026
#377) Closes R2 of #359 (RLS harness coverage extension tracking). Closes #362. ## Summary Complements R1 (#372) and R3 (merged in #373) by covering the execution contexts the request-path `rls_store` fixture doesn't reach: the `_do_cleanup` daemon thread, the `upsert_embedding` pool path used by `server._embedding_pool`, and Postgres's transaction-local `set_config` semantics plus the combined pool/Postgres contract. Test-only change — no production code modified. **What the tests actually catch (honest framing, per QA round-1 feedback):** the background-thread and pool tests verify that regressions dropping *all* of the layered cross-tenant defenses (SQL `WHERE owner_id = %s`, `_set_rls_context`, and the pool role's `BYPASSRLS` / fixture `NOBYPASSRLS` re-entry) are caught in aggregate. They do **not** each isolate one layer — that's defense-in-depth doing its job, and the module docstring now says so explicitly. The two `test_set_config_is_local_…` tests are the ones that isolate a single guarantee (Postgres's transaction-local `set_config` semantic) from everything else, using a raw `psycopg.connect` with no pool involved. ## Scope **2 files changed, +518, -0** relative to origin/main (`git diff --shortstat origin/main` → `2 files changed, 518 insertions(+)`). | File | ± | Purpose | |---|---|---| | `tests/test_rls_background.py` | +517, -0 (new) | 11 tests across 3 classes | | `CHANGELOG.md` | +1, -0 | `### Security` bullet under `[Unreleased]` (pure append to existing section) | ## Test inventory (11 tests, 3 classes) ### `TestRLSBackgroundCleanup` (4 tests) 1. `test_cleanup_isolates_expired_deletions_per_owner` — alice opts in, bob does not; both have expired entries. After `_do_cleanup`, alice's expired entries are gone, bob's remain. Exercises the full cleanup call path (owner enumeration + per-owner DELETE) under a `NOBYPASSRLS` request-path fixture. 2. `test_cleanup_skips_owners_without_preference` — cleanup is a no-op for owners who haven't set `auto_cleanup=true`. 3. `test_cleanup_preserves_non_expired_entries_for_opted_in_owner` — only rows with `expires <= now` are deleted; future-dated entries survive. 4. `test_cleanup_expired_background_thread_preserves_isolation` — runs cleanup through the spawned daemon thread (via `_cleanup_expired()`) and verifies isolation holds. Exercises the real threaded path rather than the synchronous call. ### `TestRLSBackgroundEmbedding` (2 tests) 5. `test_upsert_embedding_respects_owner_isolation` — alice's embedding is not visible to bob via `get_entries_without_embeddings`. Covers the full upsert call path including both the `WHERE owner_id = %s` SQL filter and RLS policies. 6. `test_upsert_embedding_from_worker_thread_preserves_isolation` — submits `upsert_embedding` via a `ThreadPoolExecutor`, same pattern as `server._embedding_pool`. ### `TestRLSPoolGuarantees` (5 tests) 7. **`test_set_config_is_local_true_does_not_persist_across_transactions`** — direct Postgres check on a raw `psycopg.connect` (no pool): `set_config(..., true)` is reverted at COMMIT. This is the test that isolates the transaction-local semantic from `psycopg_pool`'s `RESET ALL` check-in reset. 8. **`test_set_config_is_local_false_persists_across_transactions`** — direct Postgres check counterpart: `set_config(..., false)` does persist across transactions. Together with #7, these prove the `is_local` flag is what's producing the behavior, not some ambient reset. 9. `test_pool_checkout_does_not_see_prior_rls_context` — after a store operation, a fresh pool checkout sees no `app.current_user`. Verifies the *combined* pool+Postgres contract, not either layer alone. 10. `test_rls_context_cleared_after_exception_rollback` — an exception inside a store-style transaction + pool check-in cleanup combine to leave no residue. Same combined-contract pattern as #9. 11. `test_concurrent_owners_do_not_cross_contaminate` — two threads on different owners; each lands writes correctly and cannot see the other's data. Verifies the full call path under real concurrency (the pool physically hands out distinct connections per thread, which makes `app.current_user`-based cross-contamination impossible on its own; this test proves the broader call path is also clean). ## What changed vs. round-1 reviewed head `263250e2` 1. Replaced the round-1 `test_rls_context_does_not_persist_between_transactions` with **two new direct-Postgres tests** (#7 and #8 above). Those two actually fail when you flip `true` ↔ `false` in the test — meta-verified in-session before pushing. The old test is now renamed `test_pool_checkout_does_not_see_prior_rls_context` and its docstring is honest about being a combined-contract check. 2. Softened the docstrings of the remaining combined-contract tests (#10, #11) so they no longer claim to isolate a layer they don't isolate. 3. Module docstring rewritten to describe the defense-in-depth design accurately: cleanup and embedding are doubly scoped (SQL `WHERE owner_id = %s` + `_set_rls_context`), pool tests split into two camps (direct-Postgres isolation vs. combined-contract). 4. CHANGELOG bullet updated to reflect the new test count (11, not 9) and to describe what the tests verify without overclaiming. ## Runtime cost 11 tests, ~2.6 s against the shared testcontainers Postgres. Full pytest suite went from 989 (pre-R2) to **1000 passing** on this branch; 7 skipped unchanged. ## References - Parent tracking: #359 - Closes: #362 - Prior R-series: #372 (R1, merged), #373 (R3, merged 2026-04-22) - R4 (hypothesis fuzz): #364 — remaining sub-PR - Round-1 QA review: flagged three inaccurate meta-verifications; this revision addresses each one ## QA ### Prerequisites Docker (testcontainers spins up Postgres). Same as any other pytest run. The test file is self-contained; uses the shared `pg_dsn` fixture from `conftest.py`. ### Manual tests 1. - [ ] **Run the new test file in isolation:** `python -m pytest tests/test_rls_background.py -v`. Expected: `11 passed in ~2.6s`. 2. - [ ] **Meta-verify #7 is load-bearing** — flip `true` to `false` in `test_set_config_is_local_true_does_not_persist_across_transactions`. Re-run the test. Expected: **FAIL** with `AssertionError: set_config(..., true) survived COMMIT: got 'alice'`. Revert after verification. (Verified by Dev in-session on `e1708d7`.) 3. - [ ] **Meta-verify #8 is load-bearing** — flip `false` to `true` in `test_set_config_is_local_false_persists_across_transactions`. Expected: **FAIL** with `AssertionError: assert '' == 'alice'`. Revert. (Verified by Dev in-session on `e1708d7`.) 4. - [ ] **Defense-in-depth framing (not a failure scenario).** Per QA round-1 finding: a single-layer regression (e.g., commenting out only `self._set_rls_context(...)` inside `cleanup_expired`) does **not** cause tests #1–#6 to fail — the redundant `WHERE owner_id = %s` in the SQL files and the pool role's `BYPASSRLS` still enforce isolation on their own. That's the point: the tests verify the *aggregate* contract, not any individual layer. To cause a failure you'd need to drop both the SQL filter and `_set_rls_context`. 5. - [ ] **Scope** — `git diff --stat origin/main` shows exactly `CHANGELOG.md` (+1, -0) and `tests/test_rls_background.py` (+517, -0); net: 2 files, +518, -0. ### Acceptance - ☐ CI green on all matrix entries - ✅ 1000/1000 tests passing locally (989 → 1000, +11 new) - ✅ `ruff check`, `ruff format --check`, `mypy` all clean - ✅ Single-concern: background-thread + pool coverage only - ✅ Module docstring accurately describes what tests catch (defense-in-depth aggregate, not single-layer) - ✅ Two new tests (`test_set_config_is_local_true/false_…`) directly codify Postgres's `is_local` semantic independently of `psycopg_pool` — meta-verified to fail when the `is_local` flag is flipped - ✅ Deferred refactor explicitly noted (shared helpers module — follow-up) - ✅ Bot commit identity verified (`272174644+cmeans-claude-dev[bot]`, author + committer); push attributed to bot (`e1708d7`, verified via `gh api repos/.../activity`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <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
1 participant
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.
Summary
🤖 Generated with Claude Code