feat(registry): type_reclassification_log table — audit trail for type transitions (closes #3550)#3567
Merged
Conversation
Contributor
|
Issue #3574 proposes adding a PostHog counter in the Generated by Claude Code |
Base automatically changed from
EmmaLouise2018/registry-backfill-stale-types
to
main
April 30, 2026 03:27
Single-call insert helper for the type_reclassification_log table. Idempotent only at the row level — deduplication is the caller's responsibility. On insert failure we log and swallow: the audit log is observability, not a write barrier, and a failed log insert must not roll back the caller's primary intent. Refs #3550.
When the type-update policy decides 'disagreement' (stored non-unknown
differs from inferred non-unknown), also write a type_reclassification_log
row with source='crawler_promote' and notes={decision: 'logged_only_no_promote'}.
The crawler still does not auto-flip — the disagreement event itself is
what the audit log captures. Operator runs the backfill to flip
explicitly. See #3538.
Refs #3550.
After resolveAgentTypes runs at any of the three call sites (POST create, PUT bulk-update, admin update), diff against the pre-resolve agent array and write a type_reclassification_log row per flipped agent. source is 'member_write', member_id is the workos_organization_id (or profile id on the admin path). Pulls the diff/log logic into a new exported helper (logResolvedTypeChanges) so the three sites stay tight. Refs #3550.
Every real-mode flip from backfill-member-agent-types.ts now writes a type_reclassification_log row with source='backfill_script' and a generated run_id (backfill-<unix-ms>). Dry-run skips the audit log — no writes, no audit rows. run_id is also echoed in the script's summary so an operator can answer "what did the 2026-04-29 backfill change?" with a single SELECT. Refs #3550.
…hook - type-reclassification-log-db.test.ts: 6 tests covering canonical insert shape, null-padding for omitted optionals, JSONB serialization, explicit null oldType (first-classification), error swallow (audit log must never block caller), and per-source acceptance. - crawler-type-update-policy.test.ts: extends with 3 tests pinning that the disagreement branch writes source='crawler_promote' to the audit log, while the agree/promote branches do not. Plus changeset. Refs #3550.
…docstring Pre-review nit: logResolvedTypeChanges captures `before` arrays by reference at three call sites and diffs against the resolved array. Whole audit-log diff depends on resolveAgentTypes returning a new array, never mutating in place. Pinning the contract in the docstring so a future refactor that switches to in-place mutation surfaces the constraint at the source rather than silently zeroing out audit log entries. Closes #3550.
f6e9ec2 to
e0c28c0
Compare
#3541 + 457_agent_verification_badges_per_version both landed on main while this PR was open, so 457 is now occupied. Migration runner explicitly errors on duplicate version numbers (server/src/db/migrate.ts:80) — would crash on next deploy. Renames the migration file and the doc cross-reference in the helper module's header comment. No test/code references the version number, so no other call sites change.
bokelley
added a commit
that referenced
this pull request
May 1, 2026
CI on Phase 2b PR #3722 caught two pre-existing issues: 1. main has two migrations numbered 459 — 459_addie_escalation_dedup_key (mine, from earlier work) and 459_create_type_reclassification_log (from #3567, merged independently). Rename the latter to 461 (Phase 1's 460_identities is taken). Migration uses CREATE TABLE / INDEX IF NOT EXISTS so it's idempotent on systems that already applied it as 459. 2. CodeQL flagged the email-validation regex /^[^\s@]+@[^\s@]+\.[^\s@]+$/ as polynomial (overlap between [^\s@]+ and the literal \. allows backtracking). Replace with a linear isPlausibleEmail helper. WorkOS validates real syntax upstream — this is just a shape check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
May 1, 2026
…med (#3722) * feat(identity): admin "bind sign-in email" tool (Phase 2b) POST /api/admin/users/:userId/linked-emails creates a fresh WorkOS user for the given email and binds it under the existing user's identity as a non-primary credential. After this, the user can sign in with either email — the auth middleware id-swaps non-primary logins to the canonical workos_user_id so they see the same workspace. UI: "Add sign-in email" button on the admin /admin/people detail panel for users that have a workos_user_id. Trust model: admin asserts the email belongs to the person. The endpoint refuses if the email is already an existing AAO account (higher-risk consolidation path; tracked in issue #3719). Direct fix for Ahmed-class escalations: admin opens his person detail, clicks "Add sign-in email," enters gmail, done. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(identity): address Phase 2b security review findings Security review caught three real improvements (no blockers): 1. Partial-failure leak: if local INSERT or mergeUsers throws after WorkOS createUser succeeded, the WorkOS user was orphaned and a retry would silently create a new one. Now best-effort deleteUser cleanup, with the WorkOS id only surfaced when cleanup itself fails. 2. Admin UI confirmation: the bind action grants permanent sign-in access. Strengthen the prompt to require typing the target user's existing primary email to confirm. The button now shows whose account it'll bind to (display name + email). Avoids the "panel-open-on-the-wrong-row" footgun. 3. Test coverage gaps: add a test for the WorkOS 422 path (existing account in WorkOS that's not in our local DB) and the partial-failure rollback path. Deferred to follow-up: add CREATE UNIQUE INDEX CONCURRENTLY users_email _lower_unique. WorkOS's own gate currently makes the local TOCTOU unexploitable end-to-end, but the local check shouldn't depend on that. Also fixed the onclick HTML-injection footgun by switching to data-* attributes + event listener instead of inline JSON.stringify. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ci): renumber colliding migration; replace regex flagged by CodeQL CI on Phase 2b PR #3722 caught two pre-existing issues: 1. main has two migrations numbered 459 — 459_addie_escalation_dedup_key (mine, from earlier work) and 459_create_type_reclassification_log (from #3567, merged independently). Rename the latter to 461 (Phase 1's 460_identities is taken). Migration uses CREATE TABLE / INDEX IF NOT EXISTS so it's idempotent on systems that already applied it as 459. 2. CodeQL flagged the email-validation regex /^[^\s@]+@[^\s@]+\.[^\s@]+$/ as polynomial (overlap between [^\s@]+ and the literal \. allows backtracking). Replace with a linear isPlausibleEmail helper. WorkOS validates real syntax upstream — this is just a shape check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): identity invariant — Phase 2a/2b legitimately creates non-primary bindings The Phase 1 identity-layer test asserted "every user has a primary binding," which was correct in Phase 1 (singleton identities only). After Phase 2a's mergeUsers rewrite and Phase 2b's admin bind tool, bound users intentionally have is_primary=FALSE — so on CI when all tests run together, the global invariant query saw the bind-email test's freshly- bound non-primary user and fired a false negative. Loosen to two scoped invariants: - every user has exactly one binding - every identity has exactly one primary binding The partial unique index idx_identity_workos_users_one_primary already enforces the second at the DB level; keeping it as a test gives us a clear failure if the index is ever dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
May 1, 2026
…ion block) Two migrations landed on main with prefix 459 (PR #3672 09:14 UTC, PR #3567 22:31 UTC) without renumbering. Subsequent PRs trip: - `No duplicate migration numbers` workflow check - `migrate.test.ts > has no duplicate migration version numbers on disk` - `loadMigrations` validation in `Server integration tests` and `Built migrations against Postgres` Renumber the later one (`459_create_type_reclassification_log` → 462) since 460 (identities) and 461 (measurement_capabilities, this branch) are already taken. Filename reference in `type-reclassification-log-db.ts` updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
May 1, 2026
…3613) (#3726) * feat(registry): measurement-vendor discovery on /api/registry/agents (#3613) Crawler ingests each measurement agent's get_adcp_capabilities.measurement block (AdCP 3.x, schema #3652) and the public /api/registry/agents endpoint gains three filters: - metric_id=attention_units (exact, repeatable, JSONB containment) - accreditation=MRC (exact, repeatable; verified_by_aao always false) - q=attention (substring on metric_id; v1 scope only) All three imply type=measurement; explicit non-measurement type returns 400. Filtering at SQL level so no live fan-out per request. sources counts recomputed against filtered set (sum(sources) === count invariant). Crawler calls get_adcp_capabilities on agents that expose the tool, parses the measurement block, and persists to new measurement_capabilities_json JSONB column. 10s timeout. A measurement fetch failure does not fail the whole discovery — other capability blocks still land normally. Per security review: - Per-field caps at write time (metrics ≤500, description ≤2000, metric_id ≤256, URI ≤2048, accreditations/metric ≤32). Reject — don't silently truncate — so failure is visible via discovery_error. - Belt-and-braces 256KB DB CHECK on the column. - Strip C0 controls + DEL (keep \t, \n). - Reject <script / javascript: / data:text/html / inline event handlers after NFKC normalization. - URI fields https-only in production. - q rejects %/_ outright (substring search, not pattern); remaining ILIKE escaping uses ESCAPE '\\' (mirrors catalog-db.ts:353 pattern, not brand-db.ts which omits the explicit ESCAPE clause). Closes #3613, closes #3614 (direct-call vs index doc folded into docs/registry/index.mdx). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(migrations): renumber duplicate 459 → 462 (resolves CI dup-migration block) Two migrations landed on main with prefix 459 (PR #3672 09:14 UTC, PR #3567 22:31 UTC) without renumbering. Subsequent PRs trip: - `No duplicate migration numbers` workflow check - `migrate.test.ts > has no duplicate migration version numbers on disk` - `loadMigrations` validation in `Server integration tests` and `Built migrations against Postgres` Renumber the later one (`459_create_type_reclassification_log` → 462) since 460 (identities) and 461 (measurement_capabilities, this branch) are already taken. Filename reference in `type-reclassification-log-db.ts` updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(measurement): clarify stripControlChars comment + add \r preservation test Reviewer noticed the comment claimed "\r is stripped" but the regex explicitly preserves 0x0D — exact intent was to keep all whitespace controls (\t, \n, \r), strip everything else in C0 plus DEL. Comment now matches code; test asserts \r survives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
EmmaLouise2018
added a commit
that referenced
this pull request
May 1, 2026
) * ops(observability): emit audit_log_insert_failed metric on type_reclassification_log insert failure (closes #3574) The audit-log insert helper intentionally swallows DB errors so observability never blocks a profile save / crawler promote / backfill row. Until now the only signal of a failed insert was a warn log line — too fragile to alert on. The catch block now also fires a PostHog metric: captureEvent("server-metrics", "audit_log_insert_failed", { source, error_class }) error_class is the 2-char SQLSTATE class (e.g. '23' = integrity violation, '08' = connection failure) or 'unknown' when the thrown error has no pg code. The class — not the full 5-char code — is what ops alerts on. Pairs with #3567 (the table itself) and the future audit-log query UI, both of which assume audit-log volume is reliable. * ops(observability): tighten SQLSTATE check + document cardinality choice (review nits) Two follow-ups from review on PR #3769: 1. SQLSTATE length check is now strict (=== 5 instead of >= 2). A 5-char well-formed SQLSTATE yields its 2-char class; anything else is 'unknown'. The previous '>= 2' guard was inconsistent: a malformed 1-char code fell back to 'unknown' but a malformed 3-char code yielded a truncated 2-char "class" that wouldn't map to any real PostgreSQL class. Refuse ambiguous input — pick one shape and stick to it. 2. Added a comment next to the captureEvent call explaining why agent_url and member_id are not labels: unbounded cardinality would blow up event volume. The {source × error_class} cross product is the alertable shape; the warn log already carries per-row detail for forensics. New test pins the malformed-code boundary: code: 'XYZ' → error_class: 'unknown' (not 'XY'). 10/10 unit tests pass.
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.
Closes #3550. Refs #3538 / #3541.
#3541's backfill script writes type changes but the only record is whatever stdout dumps during the run. Future audit ("when did Bidcliq flip from buying to sales?") should answer with a row, not a screen-scrape.
This PR adds
type_reclassification_log— append-only table capturing every agent type transition the system makes.Schema
server/src/db/migrations/457_create_type_reclassification_log.sqlcreates:Indexed on
agent_url,member_id(partial),changed_at DESC,run_id(partial). Append-only. No FK toagents— historical record survives agent deletion.Hooks
Three write paths, three log entries:
server/src/crawler.ts:592-607) —source: 'crawler_promote'. Writes a row whenever the disagreement-log path ops(registry): backfill stale member_profiles.agents types + crawler reclassify-on-disagreement #3541 added fires. Captures the disagreement event without auto-flipping.server/src/routes/member-profiles.ts:97-138, 432, 656, 1791,resolveAgentTypes+ newlogResolvedTypeChangeshelper) —source: 'member_write'. Writes on every type change at all three call sites (POST create, PUT bulk-update, admin update).server/scripts/backfill-member-agent-types.ts:34, 70-83) —source: 'backfill_script',run_idpopulated. Real-mode only; dry-run skips the log.DB helper at
server/src/db/type-reclassification-log-db.ts. Insert failure is logged + swallowed — the audit log is observability, not a write barrier.Stacking
This PR is stacked on #3541 (
base: EmmaLouise2018/registry-backfill-stale-types). The crawlerdisagreementpath and the backfill--dry-runit hooks both ship in #3541 and don't exist onmainyet. After #3541 merges, retarget this PR tomain.Recommended merge order from #3538: 3540 -> 3542 -> 3543 -> 3541 -> this PR.
Test plan
npx vitest run server/tests/unit/type-reclassification-log-db.test.ts— 6/6 pass.npx vitest run server/tests/unit/crawler-type-update-policy.test.ts— extended, 8/8 pass (5 existing + 3 new).npm run test:server-unit— 2642 pass / 69 skipped.npm run typecheckclean.type_reclassification_logto confirm Bidcliq + Swivel rows are present.Out of scope