Deploy fingerprint rules from client study - #28
Open
alltheseas wants to merge 2 commits into
Open
Conversation
Replace 7 original rules (4 broken content_pattern, 2 wrong case) with 14 validated rules achieving ≥99.9% precision. Key changes: - Remove content_pattern rules (Primal, Snort, Coracle, Nostrudel) that matched mentions not origin - Fix case-sensitive matching (damus→Damus, amethyst→Amethyst) - Rename Mostr→"Bridge traffic" (proxy tag matches all bridges) - Add bot/service rules: momostr, lnp2pbot, Ditto Trends, bitbot - Add Primal platform-specific rules (Android/Web/iOS/Desktop/iOS Native) - Add tag_name_present matching for proxy tag detection - Add disagreement logging: warn when fingerprint contradicts client tag - Add dev-pubkeys.json (informational, not used for matching) Study results: 13 rules recover 57.3% of invalid unattributed events (28,472 of 49,696) with 32,174 total new attributions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces several content-pattern fingerprints with tag-based rules, adds a developer pubkeys data file, extends the fingerprint schema with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
data/app-fingerprints.json (1)
3-6: Consider moving the broadproxyrule below specific service rules.Because Tier 4 picks the first match, this generic rule can preempt more specific attributions when both match. If specificity is preferred, place
Bridge trafficlater or add explicit rule priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/app-fingerprints.json` around lines 3 - 6, The "Bridge traffic" fingerprint (app_name "Bridge traffic", tag_name_present ["proxy"]) is a broad rule that can preempt more specific Tier 4 matches; move this fingerprint entry after more specific service fingerprints or add an explicit priority field (e.g., "priority": lower_number_for_higher_priority) so specific rules are evaluated first. Locate the "Bridge traffic" object (app_name "Bridge traffic") and either relocate its entire JSON object to after service-specific rule objects or add/adjust a priority key to ensure it has lower precedence than specific services; update any Tier evaluation logic if you add a priority field so Tier 4 still respects explicit priorities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/attribution/fingerprints.ts`:
- Around line 28-30: The validation for tag_pattern in
src/attribution/fingerprints.ts is too loose and can allow null or non-object
entries that later cause matchFingerprint (resolve.ts) to access pattern.tag and
crash; update the tag_pattern check so it requires an array where every element
is a non-null object with a string "tag" property (and, if applicable, other
optional string fields like "pattern"), similar to the existing content_pattern
and tag_name_present checks—i.e., change the condition on e.tag_pattern to
validate Array.isArray(e.tag_pattern) && e.tag_pattern.every(v => v && typeof v
=== 'object' && typeof v.tag === 'string') to reject malformed entries.
In `@src/attribution/resolve.ts`:
- Around line 18-24: The current loop in resolve.ts flags disagreements by
comparing fp.app_name to clientTag.name, which yields false positives when
display names differ from canonical tag tokens (e.g., "Primal Web" vs
"primal-web"); update the check to compare the fingerprint app_name against the
client tag's canonical identifier(s) (for example clientTag.token or
clientTag.patterns) or a normalized/canonicalized form of clientTag (lowercase,
spaces→dashes, remove punctuation), using matchFingerprint(event, fp) and then
checking fp.app_name against that canonical value(s) instead of clientTag.name
so only true mismatches are logged.
---
Nitpick comments:
In `@data/app-fingerprints.json`:
- Around line 3-6: The "Bridge traffic" fingerprint (app_name "Bridge traffic",
tag_name_present ["proxy"]) is a broad rule that can preempt more specific Tier
4 matches; move this fingerprint entry after more specific service fingerprints
or add an explicit priority field (e.g., "priority":
lower_number_for_higher_priority) so specific rules are evaluated first. Locate
the "Bridge traffic" object (app_name "Bridge traffic") and either relocate its
entire JSON object to after service-specific rule objects or add/adjust a
priority key to ensure it has lower precedence than specific services; update
any Tier evaluation logic if you add a priority field so Tier 4 still respects
explicit priorities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83a61ef4-fc26-4c78-81c0-873d9e598103
📒 Files selected for processing (5)
data/app-fingerprints.jsondata/dev-pubkeys.jsonsrc/attribution/fingerprints.tssrc/attribution/resolve.tssrc/types.ts
Comment on lines
28
to
+30
| if (e.tag_pattern !== undefined && !Array.isArray(e.tag_pattern)) return false; | ||
| if (e.content_pattern !== undefined && !(Array.isArray(e.content_pattern) && e.content_pattern.every(v => typeof v === 'string'))) return false; | ||
| if (e.tag_name_present !== undefined && !(Array.isArray(e.tag_name_present) && e.tag_name_present.every(v => typeof v === 'string'))) return false; |
There was a problem hiding this comment.
Harden tag_pattern element validation to prevent matcher crashes.
Line 30 correctly validates tag_name_present, but tag_pattern still accepts malformed elements. A null item can later crash matchFingerprint when Line 70 in src/attribution/resolve.ts reads pattern.tag.
Proposed fix
- if (e.tag_pattern !== undefined && !Array.isArray(e.tag_pattern)) return false;
+ if (
+ e.tag_pattern !== undefined &&
+ !(
+ Array.isArray(e.tag_pattern) &&
+ e.tag_pattern.every((p) => {
+ if (typeof p !== 'object' || p === null) return false;
+ const pattern = p as Record<string, unknown>;
+ if (typeof pattern.tag !== 'string') return false;
+ if (pattern.value !== undefined && typeof pattern.value !== 'string') return false;
+ return true;
+ })
+ )
+ ) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (e.tag_pattern !== undefined && !Array.isArray(e.tag_pattern)) return false; | |
| if (e.content_pattern !== undefined && !(Array.isArray(e.content_pattern) && e.content_pattern.every(v => typeof v === 'string'))) return false; | |
| if (e.tag_name_present !== undefined && !(Array.isArray(e.tag_name_present) && e.tag_name_present.every(v => typeof v === 'string'))) return false; | |
| if ( | |
| e.tag_pattern !== undefined && | |
| !( | |
| Array.isArray(e.tag_pattern) && | |
| e.tag_pattern.every((p) => { | |
| if (typeof p !== 'object' || p === null) return false; | |
| const pattern = p as Record<string, unknown>; | |
| if (typeof pattern.tag !== 'string') return false; | |
| if (pattern.value !== undefined && typeof pattern.value !== 'string') return false; | |
| return true; | |
| }) | |
| ) | |
| ) return false; | |
| if (e.content_pattern !== undefined && !(Array.isArray(e.content_pattern) && e.content_pattern.every(v => typeof v === 'string'))) return false; | |
| if (e.tag_name_present !== undefined && !(Array.isArray(e.tag_name_present) && e.tag_name_present.every(v => typeof v === 'string'))) return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/attribution/fingerprints.ts` around lines 28 - 30, The validation for
tag_pattern in src/attribution/fingerprints.ts is too loose and can allow null
or non-object entries that later cause matchFingerprint (resolve.ts) to access
pattern.tag and crash; update the tag_pattern check so it requires an array
where every element is a non-null object with a string "tag" property (and, if
applicable, other optional string fields like "pattern"), similar to the
existing content_pattern and tag_name_present checks—i.e., change the condition
on e.tag_pattern to validate Array.isArray(e.tag_pattern) &&
e.tag_pattern.every(v => v && typeof v === 'object' && typeof v.tag ===
'string') to reject malformed entries.
Comment on lines
+18
to
+24
| // Check if any fingerprint disagrees with the client tag | ||
| for (const fp of fingerprints) { | ||
| if (matchFingerprint(event, fp) && fp.app_name.toLowerCase() !== clientTag.name.toLowerCase()) { | ||
| console.warn(`[fingerprint-disagree] client_tag="${clientTag.name}" fingerprint="${fp.app_name}" event=${event.id.slice(0, 8)}`); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Disagreement check can produce false positives due to canonical-name mismatch.
Comparing fp.app_name to clientTag.name (Line 20) flags valid matches when display name and client tag token differ (e.g., Primal Web vs primal-web). This will pollute disagreement logs.
Proposed fix (compare against client-tag patterns, not display name)
if (clientTag) {
- // Check if any fingerprint disagrees with the client tag
+ // Check disagreement only among fingerprints that define a client tag value
+ const normalizedClient = clientTag.name.toLowerCase();
for (const fp of fingerprints) {
- if (matchFingerprint(event, fp) && fp.app_name.toLowerCase() !== clientTag.name.toLowerCase()) {
+ const clientValues = fp.tag_pattern
+ ?.filter(p => p.tag === 'client' && typeof p.value === 'string')
+ .map(p => p.value!.toLowerCase()) ?? [];
+ if (clientValues.length === 0) continue;
+ if (!matchFingerprint(event, fp)) continue;
+ if (!clientValues.includes(normalizedClient)) {
console.warn(`[fingerprint-disagree] client_tag="${clientTag.name}" fingerprint="${fp.app_name}" event=${event.id.slice(0, 8)}`);
break;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/attribution/resolve.ts` around lines 18 - 24, The current loop in
resolve.ts flags disagreements by comparing fp.app_name to clientTag.name, which
yields false positives when display names differ from canonical tag tokens
(e.g., "Primal Web" vs "primal-web"); update the check to compare the
fingerprint app_name against the client tag's canonical identifier(s) (for
example clientTag.token or clientTag.patterns) or a normalized/canonicalized
form of clientTag (lowercase, spaces→dashes, remove punctuation), using
matchFingerprint(event, fp) and then checking fp.app_name against that canonical
value(s) instead of clientTag.name so only true mismatches are logged.
Rules matching on tag_pattern [{tag:"client", value:X}] can never fire
at Tier 4 — extractClientTag catches the same events at Tier 1 and
returns before fingerprints are consulted. Keeping them also caused
false disagreement warnings for alias pairs like primal-web/Primal Web.
Retain the 5 non-client-tag rules (Bridge traffic, momostr, lnp2pbot,
Ditto Trends, bitbot) which are the only rules that actually reach
Tier 4 and produce new attributions.
Client tag values documented in the fingerprint study report and
dev-pubkeys.json remain as reference.
Co-Authored-By: Claude Opus 4.6 <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
content_patternrules that matched mentions, not event origintag_name_presentfield for proxy tag detection (bridge traffic)dev-pubkeys.json(informational reference, not used in matching)Impact
All 14 rules achieve ≥99.9% precision against ground truth. Combined:
Test plan
npm run build— TypeScript compiles cleanlyCloses #27
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores