Demote read_context to pure retrieval; RLS user_id hardening - #475
Merged
Conversation
…de_by_path) Adds optional user_id param to log_node_read, has_read_node_in_conversation, get_conversation_reads (db/pg_queries/node_memory.py) and get_node_by_path (db/pg_queries/nodes.py). When provided, binds user_id as an explicit WHERE/VALUES filter instead of relying solely on the app.current_user_id session GUC -- defense-in-depth, correct even on an unscoped connection. When None (default), behavior unchanged (RLS-only), backward compatible. REMAINING (not done in this commit): - Bind user_id=get_user_id() at call sites in tether_mcp/tools/read_context.py, read_node_memory.py, write_node_memory.py, server.py. - Main BRIEF 1a change: conversation_id optional in execute_read_context, delete out_of_scope error dicts + get_node_tree_distance calls in execute_read_context (~243-268) and _add_children (~163-173), rename N -> traverse_depth. - server.py tool docstring rewrite (no authorization claims). - Test updates: tests/mcp/test_read_context_enforcement.py needs a full rewrite (its whole premise -- conversation_id required, out_of_scope errors -- is being removed). tests/tether_mcp/test_read_context_cascade.py needs get_node_tree_distance mocks removed + N->traverse_depth rename + delete test_conversation_id_required. - REQUIRED regression test: real (non-mocked) execute_read_context with conversation_id=None returning real root nodes -- not yet written. - RLS hardening acceptance test (mismatched user_id returns nothing on unscoped connection) -- not yet written. See cc-context-store/tether/docs/handoffs/neon-audit-premium-handoff-2026-07-04.md for full status and matched-pair contract with the premium side.
…sites read_context is no longer a scope enforcer. PermissionGate (interactive_agent_layer/permissions.py) is the sole enforcer per the conversational-core design review (5.1/5.2); by the time a read_context call reaches this module the gate has already judged whether it should happen. This PR: - Deletes the conversation_id_required error dict — conversation_id is now fully optional. Absent conversation_id means no read-credit logging, not a refusal. - Deletes the out_of_scope refusal machinery (get_node_tree_distance calls + error dicts) from execute_read_context and _add_children. Cascade descent is still cost-bounded, just never produces an error. - Renames N -> traverse_depth throughout (tool signature, internals, tests) to make clear it's a cost bound, not an authorization radius. - server.py docstring rewritten to drop all authorization claims for read_context; M is a detail request the gate judges, not enforced here. - write_node_memory's read-before-write enforcement is UNCHANGED (still requires conversation_id + a prior logged read). RLS hardening (0e addendum, folded into this PR per the phase-1 plan): - Completes the optional user_id bind at the read_context / read_node_memory / write_node_memory tool call sites (server.py), wiring get_user_id() into the already-hardened node_memory.py / nodes.py query functions from the prior WIP commit. - Adds a regression test using the real (non-mocked) execute_read_context with conversation_id=None returning real root nodes, plus an RLS acceptance test proving a mismatched explicit user_id returns nothing even on an unscoped connection. Matched pair with tether-premium feature/brief-1a-cascade-traverse-depth (prompt_assembler.load_context_cascade kwarg N= -> traverse_depth=).
Code review of the read_context demotion commit caught an inconsistency: get_node_by_path was hardened with an optional user_id explicit-bind param, but get_node (used by read_context's node_ids branch and the children section_types/children_count backfill in _build_node_response) was not touched at all — leaving id-based lookups one RLS layer weaker than path-based ones within the same tool. This adds the same optional user_id param to get_node and wires it through read_context.py's two call sites plus read_node_memory.py's node lookup. Also corrects two RLS-hardening test docstrings that overstated what they verify: the local dev Postgres role connects BYPASSRLS (same root cause as the pre-existing tests/db/test_rls.py:: test_app_db_role_is_not_superuser failure), so "RLS denies without an explicit bind" cannot be exercised in this sandbox. What the new tests (tests/db/test_pg_nodes.py) and the corrected existing test actually verify — meaningfully, independent of RLS/role privileges — is that the explicit user_id bind is a real filter: a mismatched user_id never resolves another user's node, and a matching one does.
jlunder00
force-pushed
the
feature/brief-1a-read-context-demotion
branch
from
July 5, 2026 02:11
e38abfc to
b18062b
Compare
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
read_contextfrom a scope-enforcing tool to pure retrieval, per the conversational-core design review (§5.1/§5.2): PermissionGate (interactive_agent_layer/permissions.py) is now the sole scope enforcer. This is safe to land ahead of brief 1b because the gate already enforcesread_contextscope today viacan_use_tool/_check_read_context_scope(shipped in scope-wiring) — demoting the tool doesn't open an enforcement gap. Brief 1b will refine the gate (graded 2D envelope); this PR only removes the tool's now-redundant/inconsistent second enforcement layer.conversation_id_requirederror dict —conversation_idis fully optional now (credit-logs when present, silent no-log when absent).out_of_scoperefusal machinery (get_node_tree_distancecalls + error dicts) fromexecute_read_contextand_add_children.N→traverse_depththroughout (tool signature, internals, tests) — it's a cascade cost bound, not an authorization radius.write_node_memory's read-before-write enforcement is unchanged (still requiresconversation_id+ a prior logged read).user_idexplicit-bind param threaded throughdb/pg_queries/node_memory.py,db/pg_queries/nodes.py(including aget_nodegap caught by code review — it previously had no such param at all, unlikeget_node_by_path), and theread_context/read_node_memory/write_node_memoryMCP tool call sites inserver.py.Test coverage notes
execute_read_context(conversation_id=None)returns real root nodes (the "no mocks" lesson from a prior incident).tests/db/test_pg_nodes.pyandtests/tether_mcp/test_read_context_cascade.pyverify the explicituser_idbind is a real filter (mismatched user_id never resolves another user's node). Sandbox caveat, noted in the test docstrings: the local dev Postgres role connectsBYPASSRLS(same root cause as the pre-existing, out-of-scope failure oftests/db/test_rls.py::test_app_db_role_is_not_superuser), so "RLS denies without an explicit bind" can't be exercised as a deny path in this environment — only the explicit-bind filter logic itself, which is the part this hardening actually adds. This is a sandbox/CI-environment limitation, not a coverage gap in the change.Test plan
tests/mcp/test_read_context_enforcement.py,tests/tether_mcp/test_read_context_cascade.py,tests/mcp/test_write_node_memory_enforcement.py,tests/db/test_pg_nodes.pyall greengit stashdiff against this branch's base — zero regressions), 228 skippedpr-review-toolkit:code-reviewerrun on the diff; both findings addressed in a follow-up commit