Keep duplicate-reply clusters processing after per-tweet failures - #95
Open
jnadeau207-collab wants to merge 1 commit into
Open
Keep duplicate-reply clusters processing after per-tweet failures#95jnadeau207-collab wants to merge 1 commit into
jnadeau207-collab wants to merge 1 commit into
Conversation
jnadeau207-collab
left a comment
Author
There was a problem hiding this comment.
Adversarial review (PR #95)
Claims vs code
| Claim | Evidence |
|---|---|
Filter / for fail the whole batch via Future.collect, and batches >50 only continue through prior-batch flatMap |
Confirmed. Filter.java / ForEach.java: MAX_CONCURRENCY = 50, Future.collect per batch, later batches chained with flatMap. |
Per-tweet TryOrElse isolates failures |
Confirmed. TryOrElse rescues exceptions (Future.rescue / sync catch) and returns the backup value, so Future.collect sees a successful FALSE/TRUE, not an exception. |
Reject -1 unavailable-author sentinel |
Confirmed and necessary. GetTweetAuthorId.df is already TryOrElse(GetTweet(...).user_id, -1) — author lookup does not throw. Without :tweetAuthor != -1, IsUserGrayVerified(-1) does bare GetUser(:userId) and can throw, failing the whole Filter collect. Conjunction/&& short-circuits, so the new check prevents that call. |
| “Rescue author/exemption failures” | Partially overstated. Author lookup is already soft-failed to -1; what this PR rescues is exemption-helper failures (IsUserGrayVerified, GetLongList, IsTestUser, etc.) plus the explicit -1 reject. |
| Label-before-marker so a failed label is never marked complete | Confirmed for label→marker ordering inside one TryOrElse try-arm. Not solving the inverse: label succeeds and SetBigQuerytoBotmakerLabel fails → labeled without marker (pre-existing class of partial write). |
| “Addresses #65” | Overclaim relative to the issue. #65 asks whether CJK near-duplicate detection works. This PR only hardens actioning for clusters the BBQ jobs already emit. Body scope disclaimer is honest; the “Addresses #65” one-liner is not. |
| Unchanged policy literals / job names / rate limit | Looks true from the diff; only control-flow wrappers + three metric names + -1 check added. |
Correctness
- Fail-closed on eligibility/exemption lookup failure (exclude / do not label) is the right default for a spam labeler.
- Action-loop isolation is the highest-value part of the change and matches runtime semantics.
-1short-circuit returnsFALSEon the try path, so it does not incrementexemption_lookup_failed. Unavailable authors are silent in that counter and only show up inunactionable_tweets(mixed with real exemptions).- Exemption lookup failures now also land in
unactionable_tweets, polluting that log vs “legitimately exempt.” RateLimitedsuccess path always returnsTRUEeven if every per-tweet actionTryOrElsereturnedFALSE(return value appears unused by this rule; mainly type uniformity).
Security
- No new privilege surface. More cluster members may receive
COPYPASTA_SPAMwhen a sibling previously aborted the batch — that is the intended behavior for an already-detected duplicate cluster. - Fail-closed on exemption uncertainty avoids labeling accounts that might be protected.
Missing tests / verification honesty
- Fork workflow https://github.com/jnadeau207-collab/x-algorithm/actions/runs/33702548512 is useful (grammar parse + Future.collect model) but is not a Scarecrow semantic/runtime test. “Proves” is strong language for a model.
- Public tree still needs the internal rule compiler before deploy (PR already says this).
Gaps / asymmetry
- Sibling
BBQDuplicateTextProd.bothas the same Filter/forfail-fast shape and is untouched. Fine for reply-scoped #65 narrative; incomplete for the BBQ duplicate family. - Pre-existing: unused
:downrankNote; bizarre far-futureexpiryon this rule.
Verdict: COMMENT
Core change is sound and evidence-backed against Filter/ForEach/TryOrElse/GetTweetAuthorId. Not blocking on code defects.
MUST_FIX
- None for merge-blocking code. (Docs-only: drop or rephrase “Addresses #65” to “Related to #65 — hardens actioning only”.)
NIT
- Soften #65 framing in the PR description.
- Add a dedicated counter when
:tweetAuthor == -1(short-circuit currently skipsexemption_lookup_failed). - Consider not stuffing lookup failures into
unactionable_tweets(or tag them) so ops logs stay meaningful. - Same isolation pattern for
BBQDuplicateTextProd.botin a follow-up. - Tone down “regression model proves” → “models / demonstrates under Future.collect assumptions.”
- Optional: nest marker write so label-success/marker-failure is distinguishable in metrics (still no perfect two-phase commit).
No fix commits pushed from this review (no clear code MUST_FIX). Write access exists on the PR head fork jnadeau207-collab/x-algorithm; upstream xai-org/x-algorithm is pull-only for this token.
Author
|
contributed by Jesse Nadeau @villain_thropic on X |
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
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.
Addresses #65
Bug
The public tree already has a dedicated BotMaker rule for duplicate reply clusters from both the unigram and CJK-character jobs:
BBQDuplicateTextRepliesProd.That rule evaluates each tweet through
Filterand appliesCOPYPASTA_SPAMthrough afor/ForEach. Both BotMaker implementations aggregate each batch withFuture.collect. A single failed per-tweet future therefore fails the whole filter or action batch. For clusters larger than the 50-item concurrency batch, later batches are reached only through the previous batch's successfulflatMap, so one failed member prevents all later members from being processed.A stale or temporarily unavailable tweet, safety-label lookup failure, exemption lookup failure, or label/marker write failure can therefore suppress healthy tweets in an already-detected duplicate-reply cluster.
Fix
-1unavailable-author sentinel, and continue with the rest of the cluster.COPYPASTA_SPAMlabel plus BigQuery-to-BotMaker marker transaction independently, preserving label-before-marker ordering so a failed label is never marked complete.RateLimitedbranches explicit Boolean results after the per-tweet loop.The cluster job names, event condition,
COPYPASTA_SPAMlabel, exemption policy, rate limit, marker namespace, and already-labeled checks are unchanged.Scope
This hardens actioning for clusters that the existing unigram/CJK scheduled jobs have already produced. The scheduled BigQuery clustering query is not included in the public repository, so this PR does not claim to change text normalization or cluster formation.
Verification
main(85ac72a1bba41f21615e3f0bca56da75970a6633) and changes one production rule file.BotMaker.ggrammar.IncrementStatadded.FilterandForEachfail-fastFuture.collectboundaries, 50-item batching,TryOrElserescue behavior, and sequential label-before-marker block execution.git diff --checkpasses.Verification run: https://github.com/jnadeau207-collab/x-algorithm/actions/runs/33702548512
The public export does not provide a runnable Scarecrow/BotMaker semantic compilation target, so the internal rule compiler remains required before deployment.