feat(syncer): get actor type and node id - #194
Merged
Merged
Conversation
Living implementation plan (design doc 051) for persisting the acting account's GraphQL __typename and node id on PRTimelineEvent, so bot classification stops depending on hardcoded downstream login lists. Motivated by a qb-notebook data-quality investigation: the mathlib bots were renamed on 2026-02-03, the downstream login list went stale, and six months of the "open -> first review" metric were wrong. Drafted downstream, then reviewed against the tree. Notable revisions from the original draft: - Backfill route is targeted `nodes(ids:)` resolution over stored timeline-item node ids, not a login->type map. The map's mechanism does not exist (GraphQL has no bot-by-login lookup) and it is blind to the retired logins that motivated the work. Probed live: ~1 point per batch, ~6k points total vs the <24h a full rewalk wave took. - actor_node_id added to scope. actor_type alone leaves the machine-user half of the downstream list login-keyed, so it does not close the motivating rename bug. - Archive-imported rows' missing actor_login is healed by the same backfill; the legacy query is no longer a live fetch path, so this is the only available fix. - PRReviewInlineComment.author_login stays out of scope, with reasoning: probed 33 inline comments across 30 PRs, 0 author mismatches against the parent review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Persist the timeline actor's GraphQL __typename and node id so bot classification is exact and downstream lists can key on a rename-stable identity instead of a login (design doc 051, chunk 1). - PRActorType choices (User/Bot/Mannequin, GitHub's wire casing) and the two nullable columns; NULL means unknown, never "User". - No index and no CHECK constraint: the columns are meaningful on every event type, actor_type is three-valued, and the only consumer is a flat parquet export. - Select `id` alongside `login` in the actor/author unions of pr_bundle / timeline_page / timeline_page_back. Adding fields to an existing selection is free on the GraphQL point budget. The inline comment author union is left alone — typing it is a Non-Goal. - PRTimelineEventAdmin picks up both fields per the AGENTS.md admin-sync rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Populate actor_type/actor_node_id on every ingest path (design doc 051,
chunk 2). _extract_event_fields is the single funnel for the live bundle,
both timeline page loops, and the archive importer, so one change covers
all of them.
- _actor_type_or_none / _actor_node_id_or_none primitives plus an
_actor_identity pair-builder; the allowed typename set is derived from
PRActorType.values so it cannot drift from the model.
- Wired into all 12 branches that set actor_login, including the two
distinct idioms (_login_or_empty and raw .get("login")) and the
IssueComment / PullRequestReview `author` selections.
- REVIEW_DISMISSED denormalizes dismissed_review_author_type/_node_id
into `extra` so _synthesize_dismissed_review_parent can type the row
it materializes — those are review rows, exactly what the motivating
downstream metric reads.
- actor_type/actor_node_id added to the fill-empty allowlist in
sync_timeline_events. Without this, every ordinary rewalk would leave
the new columns null forever.
Unmodelled typenames store the node id but leave actor_type null: NULL
means unknown, never "User".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Backfill actor_type/actor_node_id on historical rows by re-resolving each row's stored github_node_id through GitHub's nodes(ids:) root field (design doc 051, chunk 3). Exact rather than heuristic: it resolves the actual actor attached to each specific event, so login reuse cannot mis-type anything. The same response carries `login`, so archive-imported rows get their missing actor_login filled in the same pass — fill-only, so ingest-time logins and the rename history survive. - actor_types_by_node_ids.graphql, registered with scripts/validate_github_graphql.py (which uses an explicit path list, not a glob) and validated against ids fetched earlier in the same run. - GitHubClient.get_timeline_actors_by_node_ids, with the 100-id cap enforced client-side. - The command splits a batch in half on GraphQL errors, so one unresolvable id cannot poison the other 99. - Per-repository clients so GitHub App operation tokens resolve, matching every other syncer entry point. - Stops at the rate floor by default and is resumable; --wait-for-rate sleeps until resetAt for an unattended drain. The two actor helpers are now public (actor_type_or_none / actor_node_id_or_none) since the command imports them. Measured live: a full 100-id call costs 1 GraphQL point, so the whole ~598k-row table is ~6k points. 154 real mathlib4 node ids resolved cleanly. That probe also corrected the doc's premise — the 2026-02-03 mathlib bot changeover was an account replacement (machine users → GitHub Apps), not a rename; the plan records the correction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Chunk 4's assertions all read production rows that do not exist until the drain runs, so it cannot be pre-verified. Record the ordered unblock sequence (deploy → drain → canaries → export → downstream switch) and the reason the export must come after the drain: a first export written while the columns are entirely NULL lands as all-NaN float64 in parquet. Export coverage and sanitization are verified statically and noted as settled: EXPORT_TABLE_QUERIES is SELECT *, validate_backup_policy.py passes, and sanitize_backup.py never touches the table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Constructing a GitHubClient requires a token, so `--repo X` with nothing left to backfill failed with "GitHub token not found" instead of reporting a no-op. Filter the explicit-repo case on actual work, matching what the cross-repo path already did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A full drain is ~6 k calls, so meeting a transient failure is near certain. Three failure shapes now get told apart, because they mean different things and the reported counters are what the rollout canaries read: - Transport failures (5xx, resets, timeouts) are retried with backoff, then recorded as `call_failed` and skipped, so one bad call no longer aborts the run before it prints its counters. Deliberately not split: an outage would fan one batch out into hundreds of sleeping calls. - A rate-limit rejection unwinds to the existing resumable stop. The gate reads a cached snapshot, so it can be stale; retrying or splitting would only spend a budget that is already gone. - Only "could not resolve" still counts as `unresolved`, i.e. a fact about the row. Previously any GraphQL error ended up there, so a stale-snapshot rate rejection would have reported up to 100 rows as nodes GitHub no longer has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per docs/design-decisions/README.md: the architecture is settled and chunks 1-3 are implemented, so the living plan collapses into ADR shape (Context / Decision / Consequences / Operational Notes / Alternatives). Preserved: the invariants (NULL is unknown and never User, "Bot" means GitHub App only, key residual automation lists on actor_node_id, the fill-empty allowlist requirement, fill-only login semantics), the 2026-02-03 account-replacement correction, the parquet dtype trap, and the inline-comment non-goal with its reasoning. Dropped: chunk sequencing and the progress log, both in git history. Operational Notes now carry the rollout as a runbook -- deploy, dry run, detached drain, what each counter means, the canary queries, then export, then the downstream switch -- plus the fact that this needs no new settings or env vars. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The drain moved none of the existing convergence counters, so there was nowhere to watch it except the command's own stdout. Two per-repo counters on SyncerConvergenceSnapshot, following the precedent of archive_resync_remaining for the doc-043 drain: - timeline_events_missing_actor_type is the backfill command's exact target set, so the page and the command agree. It plateaus at the null-actor floor instead of reaching 0. - timeline_events_untyped_with_login narrows that to rows carrying a login. Such a row demonstrably had an actor, so this is typeable work: it converges to ~0 and stays there, which makes it the standing canary for the fill-empty column allowlist in sync_timeline_events. Both are per-repo counts every ANALYTICS_CONVERGENCE_PERIOD_SECONDS (900s), sequential scans given there is no index on actor_type; a partial index WHERE actor_type IS NULL is the escape hatch if the collector ever gets slow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A --limit run is the cost probe before committing to the full drain, so it should report what it actually spent instead of leaving the operator to multiply api_calls by an assumed price. `points` sums `rateLimit.cost` from the responses, and a closing `rate:` line gives the remaining/used/ resetAt the run leaves behind. Also note in --dry-run's help that it resolves for real and only skips the writes, so it costs the same as a live run of the same size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The production probe measured the installation budget at ~9700 points/hr (remaining=9621 used=79), not the 5000 a PAT gets, and confirmed 1 point per 100-id call at scale: 2000 rows resolved in 20 calls for 20 points. So the full drain is ~6.1k points and fits in one rate window; the binding constraint is wall clock, not budget. That makes an ad-hoc 500-point floor the wrong default. Raise it to 2500, matching ARCHIVE_RESYNC_MIN_RATE_REMAINING from the doc-043 drain: the live syncer stops itself only at SYNCER_RATE_REMAINING_MIN (200), so this keeps it fed while the drain runs. Also label the dry-run count `would_write` instead of `written` — these reports get pasted into runbooks, where "written" on a dry run reads as a claim that rows changed. Doc records the measured numbers, including why null_actor (0.3% here vs 12% in the recent-timeline probe) should rise as the drain reaches newer rows, and why logins=0 early is an artifact of id order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The live drain hit dense runs of deleted IssueComment ids. One
unresolvable id makes GitHub reject the whole call, and the halving path
then spends 13 calls on a 100-id batch to rediscover which id was
bad — a fact the error message already states verbatim
("...global id of 'IC_kwDOFcwZ1c7vKdns'").
Parse the named ids out, drop them, retry the remainder: 2 calls instead
of 13. At 0.25% deleted rows over ~608k that is ~1.5k points instead of
~18k, i.e. the difference between one rate window and four. Halving
stays as the fallback for messages with no id to parse, and dead ids
still land in `unresolved` because they are absent from the result.
Also collapse the per-id log line into one per batch: a dense run of
deleted comments was burying everything else in the dyno log.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
565,269 of 607,558 mathlib4 rows typed; the 42,289 remaining are exactly the final pass's null_actor + unresolved, which is what shows the drain finished rather than stalled. 39.7% of the second pass's typed rows were Bot. Two corrections to earlier estimates, both from measured data: - Rejected calls are free, so the bisection penalty was ~6 charged points per affected batch (~2k total), not the 12-13 I projected by counting calls as points. Its real cost was wall clock, and dead ids were 0.26% of rows rather than the ~1% a dense log window implied. - The archive actor_login healing recovered nothing, because every empty-login row the drain saw also had no actor on GitHub. Those events are permanently unattributable rather than merely unattributed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Production confirms the central claim at scale: the mathlib-* replacements are Bots and the retired mathlib4-* accounts are Users. 207,795 automation events are now caught by actor_type alone; 27,879 machine-user events still need a list, keyed on the four frozen node ids. New invariant, and the one thing the design did not anticipate: 678 events carry a known automation login but no type -- 83% of the 812 unresolvable node ids, 564 of them mathlib-dependent-issues comments the bot deletes and reposts. Those are ISSUE_COMMENTED rows, a first-touch type, so filtering on actor_type == 'Bot' alone would count them as human touches and reintroduce a smaller version of the bug this change exists to fix. The downstream predicate must be the union of type, node id, and login. Also: only 10 archive rows still have an empty actor_login, so the healing folded into scope had already happened via ordinary rewalks. Co-Authored-By: Claude Opus 5 (1M context) <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.
This is useful for classifying bot actions as well as persisting data across renames.