Skip to content

feat(addie): instrument agent test runs for staleness-aware prompts#3258

Merged
bokelley merged 3 commits into
mainfrom
claude/issue-3254-instrument-agent-test-runs
Apr 27, 2026
Merged

feat(addie): instrument agent test runs for staleness-aware prompts#3258
bokelley merged 3 commits into
mainfrom
claude/issue-3254-instrument-agent-test-runs

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Closes #3254

Adds the agent_test_runs table and surfaces an agent_testing block on MemberContext so #2299 Stage 2 can fire a "Time for a fresh agent test" suggested prompt to molecule_builder and pragmatic_builder personas who haven't run a test recently. The migration creates the table with constrained outcome/agent_protocol columns. Write calls are wired into the evaluate_agent_quality and run_storyboard handlers; on success, the Slack-path member context cache is invalidated so the updated agent_testing signal is available on the next prompt evaluation without waiting for the 30-minute TTL.

Non-breaking justification: adds a new optional table, two new write paths, and a new optional field on MemberContext — no existing consumers are affected.

⚠️ Pre-existing migration collision: 433_auto_provision_verified_domain.sql and 433_catalog_adagents_lookup_index.sql both use migration number 433. This PR uses 434 to avoid a second collision. The 433 duplication should be resolved before the migration runner reaches a state where ordering matters. Flagged for @bokelley.

Stage 2 design questions (for #2299, not this issue):

  • The flat 14-day staleness threshold fires regardless of last_outcome. Consider outcome-conditioned thresholds: pass + 21d, fail/partial/error + 7d.
  • The rule should gate on total_tests_30d > 0 (or at least one prior run) — users with builder persona but zero test history need a different first-run prompt, not a staleness nudge.
  • No escape hatch (snooze/dismiss) currently exists on the suggested-prompt builder. Without it, builders who test outside Addie will see this every cycle permanently.

Pre-PR review:

  • code-reviewer: approved — no new type errors; nit on COUNT(*)::int misleading re: parseInt (safe but confusing), nit on test_kind using raw storyboardId (text column, no enum constraint needed at this scope)
  • internal-tools-strategist: approved after fixes — renamed agent_urlagent_hostname to match stored value, dropped await on fire-and-forget record calls, added conditional invalidateMemberContextCache after each successful write

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See #3121
for context.

Session: https://claude.ai/code/session_018QWw9FNTxNgKLejx54HYs2


Generated by Claude Code

@bokelley bokelley added the claude-triaged Issue has been triaged by the Claude Code triage routine. Remove to re-triage. label Apr 26, 2026
@bokelley bokelley marked this pull request as ready for review April 26, 2026 21:33
@bokelley bokelley force-pushed the claude/issue-3254-instrument-agent-test-runs branch 2 times, most recently from 67e9ba6 to d1114cc Compare April 26, 2026 21:35
@bokelley

Copy link
Copy Markdown
Contributor Author

TypeScript Build is failing — multiple unit tests crash on WorkOS init at module load:

FAIL server/tests/unit/addie-router.test.ts
FAIL server/tests/unit/mcp-eval-tools.test.ts
FAIL server/tests/unit/mcp-resolve-base-url.test.ts
FAIL server/tests/unit/claude-client-cost-gate.test.ts
Error: WorkOS requires either an API key or a clientId
 ❯ new WorkOS node_modules/@workos-inc/node/src/workos.ts:137:12
 ❯ server/src/middleware/auth.ts:17:16
 ❯ server/src/addie/member-context.ts:20:1

These tests don't directly import member-context. The new import chain in this PR — likely agent-test-db.ts getting pulled in via the member-context module surface — is now reachable from those test files in a path that wasn't before, and the eager WorkOS init at auth.ts:17 triggers before vi.mock can intercept.

Same shape as the fix on PR #2729: convert the offending import to a lazy dynamic import inside the function body so module initialization doesn't pull WorkOS through. Or, if the eager init is unavoidable, the test files that newly transit this chain need explicit vi.mock("../../src/middleware/auth.js", ...) setup.

@bokelley

Copy link
Copy Markdown
Contributor Author

Fixed in 58de812.

The static import { invalidateMemberContextCache } from '../member-context.js' I added on line 21 was the culprit — converting a type-only import boundary into a value import forced member-context.tsauth.ts to load at module init, triggering the eager WorkOS constructor before vi.mock could intercept.

Removed the static import and converted both .then() callbacks to use await import('../member-context.js') lazily (same pattern as line 2195 and #2729). The cache invalidation still fires on success; it just defers the module load until the function body actually executes.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Acknowledged — lazy await import() fix in 58de812 looks right; same pattern as the existing callsites at line 2195 / #2729. No further action needed from triage.


Triaged by Claude Code. Session: https://claude.ai/code/session_01SZLAcg56zWESGWZi1HeBW9


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Two new failures since the lazy-import fix — main moved while the routine was working:

1. Duplicate agent_testing identifier in member-context.ts

member-context.ts(420,3): TS2300: Duplicate identifier 'agent_testing'.
member-context.ts(437,3): TS2300: Duplicate identifier 'agent_testing'.
member-context.ts(437,3): TS2717: Property 'agent_testing' must be of type
  '{ last_test_at: Date | null; last_outcome: "failed"|"passed" | null; }',
  but here has type
  '{ last_test_at: Date | null; total_tests_30d: number; last_outcome: "error"|"pass"|"fail"|"partial" | null; }'.

Main added its own agent_testing field on MemberContext with a narrower type. Reconcile: pick one shape and delete the other declaration. Suggest keeping this PR's richer shape (total_tests_30d + 4-state outcome) since it's the one this PR's logic depends on; the main-side declaration looks like a stub.

2. Migration number 442 now duplicate against main

::error::Duplicate migration number prefixes found

Renumber 442_agent_test_runs.sql to whatever's currently free on main (likely 445+ — check git ls-tree origin/main -- server/src/db/migrations/).

claude added 3 commits April 26, 2026 23:04
…3254)

Adds `agent_test_runs` table and hydrates `agent_testing` on `MemberContext`
so #2299 Stage 2 can fire a "Time for a fresh agent test" suggested prompt
to builder personas who haven't run a test recently.

https://claude.ai/code/session_018QWw9FNTxNgKLejx54HYs2
…tests

The top-level `import { invalidateMemberContextCache }` on member-tools.ts
forced member-context.ts (and transitively auth.ts) to load at module init,
triggering the eager WorkOS constructor before vi.mock could intercept it.

Removed the static import; both call sites in the fire-and-forget .then()
callbacks now use dynamic `await import('../member-context.js')` — same
pattern as line 2195 and as the fix applied in #2729.

https://claude.ai/code/session_018QWw9FNTxNgKLejx54HYs2
Two post-rebase conflicts against main:

1. Remove fetchAgentTesting stub (main's narrow type) — keep this PR's
   richer AgentTestingContext shape (total_tests_30d + 4-state outcome)
   which the staleness-prompt rule actually depends on.

2. Renumber 442_agent_test_runs.sql → 444_agent_test_runs.sql; main
   claimed 442 (addie_prompt_telemetry_clicks) and 443
   (agent_test_history_user_id_index) while this branch was in review.

https://claude.ai/code/session_01W4Djuhqn6SJJk5qBSPSdTt
@bokelley bokelley force-pushed the claude/issue-3254-instrument-agent-test-runs branch from 58de812 to d1c675c Compare April 26, 2026 23:06
@bokelley

Copy link
Copy Markdown
Contributor Author

Both fixes applied in d1c675c (force-pushed after rebase onto main):

1. Duplicate agent_testing resolved — removed main's narrow fetchAgentTesting stub (queries agent_test_history, returns 'passed'|'failed') and its declaration in MemberContext. Kept this PR's richer AgentTestingContext shape (total_tests_30d + 4-state outcome) and the two getAgentTestingContext call sites.

2. Migration renumbered442_agent_test_runs.sql444_agent_test_runs.sql. Main owned 442 (addie_prompt_telemetry_clicks) and 443 (agent_test_history_user_id_index).

Typecheck after fix: the TS2300/TS2717/TS2322 cluster is gone. Remaining TS7006 implicit any errors on m/c parameters in member-context.ts are pre-existing (present on main).


Generated by Claude Code


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Acknowledged — both fixes noted:

  • Duplicate agent_testing / fetchAgentTesting conflict resolved; richer AgentTestingContext shape kept.
  • Migration renumbered to 444 to clear the 442/443 slots claimed by main.

TS2300/TS2717/TS2322 cluster gone; pre-existing TS7006 implicit-any on m/c in member-context.ts confirmed as pre-existing on main. No further action needed from triage.


Triaged by Claude Code


Generated by Claude Code

@bokelley bokelley merged commit ff389e8 into main Apr 27, 2026
15 checks passed
@bokelley bokelley deleted the claude/issue-3254-instrument-agent-test-runs branch April 27, 2026 03:05
bokelley added a commit that referenced this pull request Apr 27, 2026
PRs #3258 and #3136 both shipped a 444_*.sql migration in parallel.
Every deploy after the second merge fails at the duplicate-detection
preflight, blocking the queue.

Renames the later-merged of the two:
- 444_agent_test_runs.sql → 445_* (from #3258, merged after #3136)

Both migrations are idempotent (DROP TABLE IF EXISTS / CREATE TABLE IF
NOT EXISTS). Dev DBs that already pulled 444_agent_test_runs.sql will
see 445 as a no-op next migrate run; prod's release_command was
blocked at the duplicate, so it applies fresh as 445.

Same shape as the earlier 436 collision fix (#3300).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley pushed a commit that referenced this pull request Apr 28, 2026
Top-level import of member-context.ts transitively loads middleware/auth.ts,
which constructs a WorkOS client at module load time. Any test that imports
admin-tools.ts without WORKOS_API_KEY set (or without mocking middleware/auth)
fails with a WorkOS constructor error.

Matches PR #3258's fix for the same pattern in member-tools.ts: remove the
top-level import and use `await import('../member-context.js')` inside the
two handlers that call invalidateMemberContextCache().

https://claude.ai/code/session_01J5x2do9fKEfGg3uZGZtAzw
bokelley pushed a commit that referenced this pull request Apr 28, 2026
Top-level import of member-context.ts transitively loads middleware/auth.ts,
which constructs a WorkOS client at module load time. Any test that imports
admin-tools.ts without WORKOS_API_KEY set (or without mocking middleware/auth)
fails with a WorkOS constructor error.

Matches PR #3258's fix for the same pattern in member-tools.ts: remove the
top-level import and use `await import('../member-context.js')` inside the
two handlers that call invalidateMemberContextCache().

https://claude.ai/code/session_01J5x2do9fKEfGg3uZGZtAzw
bokelley added a commit that referenced this pull request Apr 28, 2026
…fety checks (#3393)

* fix(admin): auto-approve member logo uploads that pass format/size/safety checks

Closes #2568. Mary launch blocker — logos uploaded by members sit in a
pending queue with no admin UI and no member-visible resolution path.

- brand-logos.ts: community uploads now auto-approve (review_status:
  'approved') after passing the existing membership + ban + magic-bytes +
  SVG-sanitization + size gates. Manifest rebuild extended to all uploads
  (was owner-only). Source still tracks brand_owner vs community.
- brand-viewer.html: success message now reflects actual review_status
  from the API response ('Logo uploaded and live.' vs 'pending review.')
  instead of hardcoding the pending message.
- brand-tools.ts: Addie upload_brand_logo tool had the same pending-only
  logic as a separate code path. Fixed to match HTTP route: inserts as
  approved, rebuilds manifest, returns correct status.
- brand-logo-db.ts: insertBrandLogo default changed from 'pending' to
  'approved' so future callers that omit review_status don't silently
  re-introduce the queue.
- brand-logo-digest.ts: comment updated to explain the job is now a
  drain for pre-deploy historical pending logos only.
- changeset auto-approve-verified-owner-logos.md: removed now-incorrect
  note that community uploads still queue.

https://claude.ai/code/session_019WyF5yW7B8jkypARtKHqcg

* fix(admin): lazy-import invalidateMemberContextCache in admin-tools.ts

Top-level import of member-context.ts transitively loads middleware/auth.ts,
which constructs a WorkOS client at module load time. Any test that imports
admin-tools.ts without WORKOS_API_KEY set (or without mocking middleware/auth)
fails with a WorkOS constructor error.

Matches PR #3258's fix for the same pattern in member-tools.ts: remove the
top-level import and use `await import('../member-context.js')` inside the
two handlers that call invalidateMemberContextCache().

https://claude.ai/code/session_01J5x2do9fKEfGg3uZGZtAzw

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-triaged Issue has been triaged by the Claude Code triage routine. Remove to re-triage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instrument agent test runs (prereq for staleness-aware prompts in #2299)

2 participants