Skip to content

fix(notifications): match goal owner by canonical forms, not '%user%' substring#205

Merged
khustup2 merged 1 commit into
mainfrom
fix/open-goals-owner-exact-match
May 26, 2026
Merged

fix(notifications): match goal owner by canonical forms, not '%user%' substring#205
khustup2 merged 1 commit into
mainfrom
fix/open-goals-owner-exact-match

Conversation

@khustup2
Copy link
Copy Markdown
Contributor

@khustup2 khustup2 commented May 26, 2026

Summary

The open-goals SessionStart banner (src/notifications/sources/open-goals.ts) filtered goals with owner LIKE '%${userName}%'. That substring match has two real failure modes — the same ones CodeRabbit flagged on #203 for the SessionStart context block:

  1. Substring collision'%ali%' matches malice@…, leaking another user's goals into the banner.
  2. Reverse-alias miss — a full-email userName never matches a row whose owner is the short form.

The context-renderer already fixed this with listOpenGoals (exact full / exact short / short@% alias), but the banner was never ported and kept the worse query plus a loose bidirectional includes() JS guard.

This PR makes the banner reuse listOpenGoals instead of duplicating its own SQL. Net change is a deletion: the substring query and the JS guard both go away.

Changes

  • open-goals.ts: drop the owner LIKE '%…%' SQL + includes() guard; call listOpenGoals(sql => api.query(sql), goalsTable, userName, { limit: 25 }).
  • Updated notifications-open-goals-source.test.ts SQL-shape assertions to the canonical-forms query. 18/18 pass.

Verified

Built the bundle, installed locally, and ran the actual SessionStart hook against the live API. It now issues:

WHERE (owner = 'sasun' OR owner = 'sasun' OR owner LIKE 'sasun@%')

exact owner match, no substring scan — rendered the banner correctly, exit 0.

⚠️ Note / follow-up (NOT fixed here)

While testing I confirmed the "N goals open" double-count is a separate bug that this PR does not fix. listOpenGoals carries a version = MAX(version) filter, but version is vestigial — always 1 (deeplake-fs.ts:459-461), so it never dedups. The duplicate rows come from Deeplake's UPDATE-coalescing quirk (deeplake-fs.ts:466-468): a single logical goal can have 2+ rows with the same (goal_id, version). A real fix needs DISTINCT ON (goal_id) / GROUP BY goal_id in both readers (open-goals and context-renderer). Happy to do that in a follow-up.

Test plan

  • vitest run tests/claude-code/notifications-open-goals-source.test.ts (18/18)
  • tsc --noEmit --skipLibCheck
  • Live SessionStart hook emits canonical-forms query, exit 0

Summary by CodeRabbit

  • Refactor
    • Enhanced open goals notification fetching with more precise canonical owner-form matching instead of substring patterns, improving accuracy and security while maintaining existing deduplication and filtering.
  • Tests
    • Updated tests to verify canonical owner-form matching and edge-case filtering behavior.

Review Change Stack

… substring

The open-goals SessionStart banner filtered goals with
`owner LIKE '%${userName}%'`, which has two real failure modes
(the same ones CodeRabbit flagged on PR #203 for the context block):

  1. Substring collision — `'%ali%'` matches `malice@…`, leaking
     another user's goals into the banner.
  2. Reverse-alias miss — a full-email userName never matches a row
     whose owner is the short form.

The context-renderer already solved this with `listOpenGoals`
(exact full / exact short / `short@%` alias), but the banner was
never ported. Reuse that reader instead of duplicating a worse query;
the loose bidirectional `includes()` JS guard goes away with it.

Note: `listOpenGoals` also carries a `version = MAX(version)` filter,
but `version` is vestigial (always 1, see deeplake-fs.ts), so it does
NOT dedup the duplicate-row "N goals" double-count from Deeplake's
UPDATE-coalescing quirk. That needs a separate `DISTINCT ON (goal_id)`
fix in both readers — out of scope here.
@khustup2 khustup2 requested a review from efenocchi May 26, 2026 00:30
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a22d378f-4ff3-41de-bf7f-819016085dd2

📥 Commits

Reviewing files that changed from the base of the PR and between cf60724 and b1f8b16.

📒 Files selected for processing (2)
  • src/notifications/sources/open-goals.ts
  • tests/claude-code/notifications-open-goals-source.test.ts

📝 Walkthrough

Walkthrough

This PR refactors fetchOpenGoals to delegate owner matching and version deduplication to the shared listOpenGoals reader, replacing ad-hoc SQL construction with canonical owner-form matching. Tests are updated to validate the new query shape and ensure rows with non-canonical owners are filtered out.

Changes

Open Goals Canonical Owner Matching

Layer / File(s) Summary
Import and documentation updates
src/notifications/sources/open-goals.ts
Adds listOpenGoals import and updates fetchOpenGoals header documentation to describe canonical owner-form matching (exact full email, exact short, short@% alias) and MAX(version) deduplication, removing references to substring-collision behavior from '%user%' LIKE scans.
Refactored fetchOpenGoals implementation
src/notifications/sources/open-goals.ts
Replaces direct SQL construction and manual row post-processing with a call to listOpenGoals reader; removes manual SQL building, substring owner matching via LIKE, and extra normalization/guards, while preserving null-on-empty behavior and goal-label extraction from the first markdown line.
Test updates for canonical owner matching
tests/claude-code/notifications-open-goals-source.test.ts
Test documentation updated to reference canonical-form owner matching without substring collisions. SQL shape assertions now validate canonical owner clauses (exact / short / short@% alias) instead of %user% LIKE pattern. SQL injection test expectations updated to match escaped owner literals. Defense-in-depth test name and assertion clarified to filter rows with non-canonical owners.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A reader refactors with care,
Swapping SQL for shared flair,
Canonical forms now take the stage,
No LIKE collisions on this page,
Tests applaud the cleaner way!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing substring matching with canonical form matching for goal owner filtering in notifications.
Description check ✅ Passed The description is comprehensive and well-structured, covering summary, changes, verification, and test results. However, the version bump section is incomplete—no version number is provided in package.json.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/open-goals-owner-exact-match

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

Coverage Report

Scope: files changed in this PR. Enforced threshold: 90% per metric (per file via vitest.config.ts).

Status Category Percentage Covered / Total
🟢 Lines 96.15% (🎯 90%) 25 / 26
🟢 Statements 94.29% (🎯 90%) 33 / 35
🟢 Functions 100.00% (🎯 90%) 7 / 7
🔴 Branches 85.19% (🎯 90%) 23 / 27
File Coverage — 1 file changed
File Stmts Branches Functions Lines
src/notifications/sources/open-goals.ts 🟢 94.3% 🔴 85.2% 🟢 100.0% 🟢 96.2%

Generated for commit 531cfc2.

@khustup2 khustup2 merged commit cafd321 into main May 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants