Repository navigation
fix(memory): scope TrafficLearner's SQLite reads to its user_id - #4850
chopratejas wants to merge 4 commits into
Conversation
TrafficLearner saves rows under its configured user_id, but both direct SQLite reads (startup hydration and the persisted-pattern collector) read every user's traffic_learner rows. With several user IDs sharing one memory.db, one user's learned patterns ended up in another user's learned context, and hydration could bind a hash to another user's row and bump its evidence. Both queries now filter by user_id, before the hydration LIMIT. Assay-Kind: bugfix Assay-Ask: TrafficLearner read and updated other users' learned patterns when several user IDs shared one memory database. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR governanceThis PR does not yet satisfy the required template fields:
Please update the PR body, or move the PR back to draft while it is still in progress. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
JerrettDavis
left a comment
There was a problem hiding this comment.
No defect found in the user-scoping change. Independent SQLite checks with rows for Alice, Bob and default confirm hydration respects the configured user before LIMIT 1 and collection returns only that user’s patterns; a quoted unknown user returns none. These checks pass on this head and reproduce the leakage on recorded main. This was isolated method validation, not a full pytest run. Qualification remains pending: the shared workflow audit retains an inherited gitleaks integrity hold, and the staged Security run reports Rust advisory and pip-audit failures whose logs were not available for diagnosis. These are separate from the reviewed code change.
Picks up the dependency updates from #4835 so the audit checks pass. Assay-Kind: deps Assay-Ask: The dependency audit checks failed on this PR because the branch predates the dependency updates on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JerrettDavis
left a comment
There was a problem hiding this comment.
The updated head passes four independently executed isolated SQLite checks: hydration filters by user before LIMIT, collection excludes other users, quoted user IDs remain bound parameters, and Bob creates his own evidence row for content already saved by Alice. The same checks fail on recorded main. No defect found in the code change; these checks do not constitute the full pytest suite or a live proxy run. Security now passes. CI is red only on commitlint (run 43519004770, job 132208203465); please provide its failing annotations/log and resolve any reported violation before qualification. The cause is not established by the staged check metadata. The inherited gitleaks installer integrity hold remains separate from this PR.
JerrettDavis
left a comment
There was a problem hiding this comment.
The remaining CI failure is now diagnosed: commitlint rejects merge commit 457cceb because its Assay-Ask footer is 116 characters, over the 100-character limit (footer-max-line-length). See failed commitlint job. Please wrap that footer and ensure commitlint passes for the PR commit range. For example:
Assay-Ask: The dependency audit checks failed because this branch predates
the dependency updates on main.
No code defect found in the user-scoping change. The prior independent SQLite checks pass at this identical head and reproduce the leakage on recorded main; those results and source coverage were reused. Full pytest and a live proxy run were not executed locally. The inherited workflow provenance hold remains separate and does not explain this PR-specific commit-message failure.
Sync PR #4850 with latest main.
Sync PR #4850 with latest main.
JerrettDavis
left a comment
There was a problem hiding this comment.
The previously reported commitlint failure remains after merging main: the current run still rejects commit 457cceb because its Assay-Ask footer is 116 characters (limit 100). That commit remains in the PR range. Please wrap the footer and obtain passing commitlint for the complete PR commit range.
No additional defect found in the current user-scoping delta. Current hosted Python checks and Security report success; no new local runtime tests were executed. The inherited workflow integrity hold remains separate from this PR-specific failure.
|
Jerrett, the commitlint blocker is cleared. The footer-length rule fix (#4868) is merged into main, and I merged main into this branch, so commitlint now passes on this head. No code changes in this update. |
7070d36 to
7d7b676
Compare
|
Applied the requested mechanical fix: rewrote the PR-range commits to wrap the oversized Assay-Ask footer(s) to under the 100-character commitlint limit (content preserved, wrapped at word boundaries). Branch force-pushed; commitlint and CI are re-running at the new head. |
Description
TrafficLearnersaves its rows under its configureduser_id, but its two direct SQLite reads ignoreduser_id:_hydrate_persisted_state(startup) loaded every user'straffic_learnerrows into the dedup maps, then appliedLIMIT dedup_window. Bob's learner could bind a pattern hash to Alice's row and later bump her evidence instead of creating his own row. Other users' rows also used up his window._load_persisted_patterns_from_sqlite(used by_collect_all_patternswhen writing learned context files) returned every user's patterns, so Alice's learned text ended up in Bob's files.Both queries now filter on
user_id = ?. In hydration the filter runs before theLIMIT. Only matters when several user IDs share one memory database; a single-user setup (user_id="default") is unchanged.Type of Change
Changes Made
headroom/memory/traffic_learner.py:WHERE user_id = ?in both reads._load_persisted_patterns_from_sqlite(db_path, user_id)takes the user explicitly (private helper; its only caller is updated).tests/test_memory/test_traffic_learner.py: newTestUserScopedPersistence, which checks that hydration leaves another user's row alone and collection excludes other users. Hand-built test schemas gain theuser_idcolumn that the realmemoriestable has, and direct helper calls pass"default".Testing
pytest)ruff check .)mypy headroom)Test Output
Real Behavior Proof
memory.dbholding atraffic_learnerrow foraliceand one forbob.TrafficLearner(backend=<backend pointing at that db>, user_id="bob")._collect_all_patterns().--learn --memoryand several users.Runtime Rollout Safety
Review Readiness
Checklist
CHANGELOG.md— it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)Screenshots (if applicable)
N/A
Additional Notes
Rows written before this change already carry the correct
user_id(the save path always set it), so no migration is needed.🤖 Generated with Claude Code
Reported by @JoaoMarcos44. Thank you for your contribution! 🩵