-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
automod: identical reply rule #466
Conversation
can trigger this against prod data with:
|
// use a specific period (IncrementPeriod()) to reduce the number of counters (one per unique post text) | ||
period := automod.PeriodDay | ||
bucket := evt.Account.Identity.DID.String() + "/" + HashOfString(post.Text) | ||
if evt.GetCount("reply-text", bucket, period) >= identicalReplyLimit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, this is proceeding to act immediately on a hash collision. Do you think it would be reasonable to do a more expensive check to see if things are actually identical?
I don't know how bad a false positive is here. Maybe if the threshhold for identical in sheer count is moderate, it's unlikely to trigger on reasonable real human behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naive false positive rate (for the 64bit variant of murmur3) is low enough to not worry about it and not do secondary network requests to check for exact matches.
This isn't a cryptographic hash, so attacks could be a concern for some rules.
Generally, I feel like all the counters we are using here should be treated as a bit fuzzy, at least for record-level counts. It is totally possible for events to get partially-processed (and partially persisted) and then re-processed again after a crash. I think the semantics and kinds of rules and actions we write are generally resilient to this: doing things like reporting for human review, or having large margins before taking fully-automated action.
period := automod.PeriodDay | ||
bucket := evt.Account.Identity.DID.String() + "/" + HashOfString(post.Text) | ||
if evt.GetCount("reply-text", bucket, period) >= identicalReplyLimit { | ||
evt.AddAccountFlag("multi-identical-reply") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new to this PR, but I think some docs on the semantic distinctions between AccountLabels
, AccountFlags
, and AccountReports
are needed somewhere (and then perhaps most methods touching them should have a quick pointer to that doc). The last one of the group is close to self-explanatory, but the first two are both just []string
in the code and don't autounpack their meaning very readily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm! added doc comments to all the fields for most of the RepoEvent variants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an overall LGTM, given that it can be iterated on further if the collision rate were to turn out problematically high.
Will have some merge conflicts to resolve.
Merged with main (copied additions to countstore over to that package), and added a bunch of doc comments. I'm not too concerned about distinct counter collisions, but can revisit. |
This PR is currently rebased on top of #466, to demonstrate testing that rule. **UPDATE:** that PR merged, so now against `main` Adds a `hepa` command to "capture" the current state of a real-world account: currently some account metadata (identity, profile, etc), plus some recent post records. This gets serialized to JSON for easy dumping to file, like: ```shell go run ./cmd/hepa/ capture-recent atproto.com > automod/testdata/capture_atprotocom.json ``` Then, a test helper function which loads this file, and processes all the post records using an engine fixture. Combined, these fixtures make it easy to do test-driven-development of new rules. You find an account which recently sent spam or violated some policy, take a capture snapshot, set up a test case, and then write a rule which triggers and satisfies the test. Some notes: - tried moving the "test helpers" in to a sub-package (`indigo/automod/automodtest`) but hit a circular import, so left where it is - this won't work with all rule types, and some captures/rules may need additional mocking (eg, additional identities in the mock directory), but that should be fine - it usually isn't appropriate to capture real-world content in to public code. we can be careful about what we add in this repo (indigo); the "hackerdarkweb" example included in this PR seems fine to snapshot to me. the code does strip "Private" account metadata by default. - probably could use docs/comments. i'm not sure where best to put effort, feedback welcome!
Two enabling features:
This initial version of the rule counts replies to any other user in the same bucket, not distinct-accounts-with-same-reply-text. I'm a little worried about redis memory growth if we have a HyperLogLog for each author+text combination (as opposed to simple counter int). Maybe the redis implementation is clever and efficient for the small-distinct case? Or maybe RAM is cheap enough?
This branch will conflict with #464. Plan to merge that one first, then i'll rebase this one.