fix(storage): lesson-family dedup guard trusts stored type, not the call's type argument - #299
Merged
Merged
Conversation
…all's type argument createEntityInner's content-dedup exemption for lesson_learned/lesson/ mistake entities checked the incoming `type` parameter, never the entity's actual stored type. `INSERT OR IGNORE` is a no-op on an existing row, and the SELECT beside it didn't even fetch `type` — so re-remembering an existing lesson entity under any other type string (schemas.ts declares `type` as an unconstrained 1-100 char string, no enum) took the non-lesson branch and ran content dedup against observations that groupLessons/parseStructuredBlocks read as ordered Error:/Root cause:/Fix:/Prevention: blocks, silently dropping a repeated field line and corrupting the lesson. Found by a security review of PR #288's diff; verified against the actual code (schemas.ts:48, the existing "preserve original type on duplicate entity" test) before fixing. The SELECT now reads `type` and the guard uses the stored value for an existing entity, the incoming value only when the entity is new (there is no stored value yet). Regression test added mirroring the existing lesson-family dedup test; reverting the fix turns exactly that one test red (64 others stay green), confirmed by running it both ways.
kevintseng
force-pushed
the
fix/lesson-dedup-guard-trusts-stored-type
branch
from
September 4, 2026 11:33
6970d1e to
d452db4
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.
What it does
createEntityInner's content-dedup exemption forlesson_learned/lesson/mistakeentities now checks the entity's actual storedtypefor an existing entity, instead of thetypestring this particular call happened to pass.Why it is needed
Found by a security review of PR #288's diff.
INSERT OR IGNOREis a no-op on an existing row, and the SELECT beside it never fetchedtype— so re-remembering an existing lesson entity under any other type string (schemas.tsdeclarestypeas an unconstrained 1-100 char string, no enum) took the non-lesson branch and ran content dedup against observations thatgroupLessons/parseStructuredBlocksread as orderedError:/Root cause:/Fix:/Prevention:blocks — silently dropping a repeated field line and corrupting the lesson, with no signal to the caller.How to test
The new test
re-remembering a lesson entity under a different type string still skips content dedupfails on the pre-fix code (confirmed by reverting the fix and re-running — exactly that one test goes red, the other 64 stay green) and passes with the fix.Linked issues
Found during v4.8.4 pre-release security review, no tracked issue number.