Skip to content

feat(visibility): three-tier agent visibility with tier gate#2793

Merged
bokelley merged 2 commits into
mainfrom
bokelley/agent-visibility-tiers
Apr 22, 2026
Merged

feat(visibility): three-tier agent visibility with tier gate#2793
bokelley merged 2 commits into
mainfrom
bokelley/agent-visibility-tiers

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • Replaces agent is_public boolean with visibility: 'private' | 'members_only' | 'public'. Public listing requires Professional+ (API access) tier; Explorer and non-paying users can set private or members_only.
  • members_only is the paid-member discovery pool: non-paying orgs can register agents and be found by Professional+ members via MCP list_agents / get_member, without landing in the public directory or brand.json. Solves the Scope3-style use case.
  • Stripe webhook auto-demotes publicmembers_only on any tier change where the new tier lacks API access (including full cancellation), and strips those agents from the community brand.json manifest.
  • MCP list_agents, get_agent, list_members, get_member are tier-aware: API-access callers receive members_only agents; non-API callers never do. Tool descriptions spell out the semantics. The visibility field is omitted for non-API callers to cut context bloat.
  • PUT /api/me/member-profile coerces smuggled visibility=public down to members_only for non-API callers and surfaces the coercion via a warnings[] array in the response — closes the whole-profile bypass path.
  • applyAgentVisibility runs in a transaction with SELECT ... FOR UPDATE, eliminating the publish/downgrade race.
  • /check endpoint is report-only now — returns a drift value instead of silently mutating visibility.
  • UI: server/public/member-card.js renders a three-radio selector with an Upgrade-to-Professional upsell on the disabled public option; new agents default to members_only.
  • Drive-by fix: brand_revisions.domain was renamed to brand_domain in migration 389 but the code still queried the old name — caused 500s on the second updateManifestAgents call for the same community brand.
  • Migration 419 rewrites existing agents JSONB (is_public: truevisibility: 'public', false → 'private') and drops any pre-existing visibility key to prevent pre-migration smuggling.

Test plan

  • Unit: agent-visibility.test.ts (tier helper + type guard, 10 tests)
  • Unit: agent-service-visibility.test.ts (filter by viewer tier, 6 tests)
  • Unit: agent-visibility-enforcement.test.ts (demote helper, 8 tests)
  • Unit: mcp-tools-viewer-context.test.ts (MCP callerHasApiAccess filtering, 6 tests)
  • Integration (docker postgres): agent-visibility-e2e.test.ts — Explorer 403, Professional 200, PUT-bypass coercion, private-profile members_only, tier downgrade, full cancellation, legacy is_public normalization, 11 tests
  • Full 39/39 tests pass against a real migrated Postgres via docker-compose
  • tsc --noEmit clean across server/
  • Manual smoke via dev server: add agent in member dashboard, toggle radio, verify PATCH network call + brand.json snippet dialog
  • Verify upsell copy / link in member-profile page for Explorer tier

Deferred (not blockers)

  • Rate limits on PATCH /visibility (low abuse surface)
  • Reconciler for non-Stripe tier changes (admin direct UPDATE, trial expiry) — current hook covers Stripe webhook path
  • Error code standardization across /publish//check (PATCH uses error: tier_required; others still human strings)
  • Upsell telemetry ("47 Professional members viewed this agent")

🤖 Generated with Claude Code

Replaces the boolean agent is_public flag with visibility:
'private' | 'members_only' | 'public'. Public listing is gated on
API-access membership tiers (Professional+). members_only is the
discovery pool for paid-member-only agents, solving the Scope3 use
case: a non-paying org can register agents and be discoverable by
Professional+ members without landing in the public directory.

Stripe tier-downgrade auto-demotes public agents to members_only and
strips them from the community brand.json manifest. MCP list_agents,
get_agent, list_members, get_member receive the caller's org tier and
return members_only agents to API-access callers only. PUT on the
whole profile silently coerces smuggled visibility=public to
members_only for non-API callers and surfaces the coercion via a
warnings[] array.

Drive-by fix: brand_revisions.domain was renamed to brand_domain in
migration 389 but the code still queried the old column name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread server/public/member-card.js Fixed
github-code-quality bot flagged an unused variable left over from the
visibility refactor. No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@EmmaLouise2018

EmmaLouise2018 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Review — Three-Tier Agent Visibility

Blockers

2. Tier bypass via POST /api/me/member-profile (profile creation). The PUT handler coerces publicmembers_only for non-API tiers, but the POST creation path persists the request body's agents verbatim via createProfile. An Explorer user who hasn't created a profile yet can land visibility:'public' on first write, and subsequent readers filter strictly on visibility === 'public' with no tier re-check. Apply the same isValidAgentVisibility + tier-gate coercion on create.

3. addAgent MCP path defaults visibility:'public' with no tier gate. server/src/addie/mcp/member-tools.ts:4870 — an unpaid org calling this tool writes a public agent straight into its profile. Either default to members_only (matches the UI) or resolve caller tier and gate. Option A is safer.

High

4. TOCTOU on the tier gate. applyAgentVisibility uses SELECT … FOR UPDATE on member_profiles, but requireApiAccessTier reads the org tier before the transaction. A concurrent Stripe downgrade webhook can land between the tier read and the UPDATE, committing a public write on a now-downgraded org. Re-read tier inside the transaction.

5. demotePublicAgentsOnTierDowngrade is not transactional. services/agent-visibility-enforcement.ts:38-73 calls getProfileByOrgId then updateProfileByOrgId as two statements — no row lock. A concurrent PUT/PATCH can reinsert public entries. Wrap in a transaction with FOR UPDATE.

Medium

  • brand.json + agents JSONB updates are not atomic. brandDb.updateManifestAgents opens its own pool connection outside applyAgentVisibility's transaction — if the JSONB UPDATE/commit fails after the manifest is written, drift results (the exact thing /check detects). Pass client through so both ride the same transaction.
  • profile_visibility field in MCP responses (mcp-tools.ts:817,868) conflates profile-level is_public with agent-level visibility on the wire under the same naming. Rename to directory_listed: boolean.
  • Error shapes are inconsistent across /publish, /check, /visibility, PUT profile ({error} vs {error,message} vs {error, valid}). UI copes via data.message || data.error. PR defers standardization; acceptable with a follow-up.
  • New helpers typed req: any, res: any (resolveUserOrgId, requireApiAccessTier in member-profiles.ts). Use Request/Response + an AuthenticatedRequest to drop the six req.user!.* non-null assertions.
  • No exhaustiveness guards on AgentVisibility. agent-service.ts:isVisibleToViewer and the applyAgentVisibility body builder use ternary chains with implicit fallthrough. Switch to switch + assertNever so adding a tier becomes a compile error.
  • warnings: Array<Record<string, unknown>> (member-profiles.ts:498). Now that this is part of the PUT response contract, model a ProfileWarning discriminated union with code: 'visibility_downgraded'.

Nits

  • listAgents(typeOrOptions?: AgentType | AgentListOptions) dual-signature — the as AgentType | undefined cast launders arbitrary strings. Consider splitting or adding a runtime guard.
  • normalizeAgentConfig is duplicated inline in applyAgentVisibility's raw-SQL path. Extract to one module.
  • is_public dropped from AgentConfig without a deprecated-optional step — consider keeping it deprecated-optional for one release.
  • mcp-tools.ts:callerHasApiAccess comment claims memoization but no cache map is passed.
  • .changeset/agent-visibility-tiers.md has a doubled --- frontmatter delimiter.

Positives

  • Report-only /check with a three-state drift enum — material correctness improvement.
  • Structured warnings[] with visibility_downgraded on PUT is a great pattern.
  • safeId derived only from URL prevents user-controlled name from poisoning brand.json.
  • applyAgentVisibility correctly centralizes /publish, DELETE /publish, and PATCH /visibility; /check stays out of it.
  • Good test coverage (39 tests, real Postgres e2e), dependency-injectable enforcement service.

Recommendation

Request changes. Blockers: migration idempotency, POST creation tier bypass, MCP addAgent default. The two TOCTOU fixes likely share the same transactional-tier-read change.

@bokelley bokelley merged commit 525e2da into main Apr 22, 2026
13 checks passed
bokelley added a commit that referenced this pull request Apr 22, 2026
Main landed two migrations numbered 419: #2793 (agent visibility, first)
and #2800 (OAuth client-credentials, second). Per-PR "No duplicate
migration numbers" CI passed on each because at check-time the other
wasn't on main yet. Merge-queue race.

Every fresh DB boot now fails with:
  Migration filename validation failed:
  Duplicate migration version 419: 419_agent_visibility.sql
  and 419_oauth_client_credentials.sql

Every already-deployed instance fails on next restart.

Fix: rename 419_oauth_client_credentials.sql to 420. The SQL is
idempotent (ADD COLUMN IF NOT EXISTS, DROP VIEW IF EXISTS + CREATE VIEW)
so the rename is safe for all three deployment states:
 - Fresh DBs apply 419+420 in order.
 - Instances that ran the old 419_oauth_client_credentials.sql: 420
   re-runs cleanly, columns/view no-op.
 - Instances that ran #2793's 419_agent_visibility.sql: 420 applies
   fresh.

Verified locally: docker compose up proceeds past the load step.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 22, 2026
* fix(visibility): close tier-gate blockers from #2793 review

#2793 (three-tier agent visibility) shipped before the EmmaLouise2018
review was addressed. This PR closes the three Blocker / two High
findings.

- POST /api/me/member-profile (create) persisted agents verbatim, so
  an Explorer could land visibility:'public' on first write and later
  readers filtered strictly on === 'public' with no tier re-check.
  Extracted coercion from PUT into a shared agent-visibility-gate
  helper; both paths use it. Create response surfaces structured
  warnings[] on downgrade, matching PUT.

- MCP addAgent (member-tools.ts) defaulted to visibility:'public'
  with no tier gate. Changed default to 'members_only' — safer than
  resolving caller tier inline and matches the UI default for new
  agents.

- applyAgentVisibility had a TOCTOU: the outer requireApiAccessTier
  ran before the transaction. A concurrent Stripe downgrade could
  commit between the outer check and the UPDATE, letting a 'public'
  write land on a now-downgraded org. Now re-reads membership tier
  inside the profile FOR UPDATE transaction and rolls back with 403
  if the current tier lacks API access.

- demotePublicAgentsOnTierDowngrade called getProfileByOrgId then
  updateProfileByOrgId as two separate statements — no row lock.
  Rewritten to hold a pg client, BEGIN, SELECT FOR UPDATE on
  member_profiles, UPDATE, COMMIT. Combined with the inner-tx tier
  check above, a publish and a demote cannot interleave past each
  other's lock on the same profile.

Tests:
- New agent-visibility-gate.test.ts — 10 scenarios for the shared
  helper
- agent-visibility-enforcement.test.ts rewritten to mock getPool()
  and the pg client (9 scenarios, including "commits profile tx
  before touching brand.json")
- Full server unit suite green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(visibility): expert-review follow-ups on #2822

Addresses Must-Fix and Should-Fix items from the three-expert review:

- DX Must-Fix: dashboard client silently dropped warnings[]. member-
  profile.html now renders a "saved, but N agents could not be listed
  publicly" banner with an upgrade CTA when the response carries
  downgrade warnings. Applies to both POST (new create flow) and PUT
  (existing update flow) — both paths return the same shape.
- DX Must-Fix: save_agent tool description didn't mention visibility.
  Added explicit members_only default + publish-flow semantics to the
  tool description, and Addie's save confirmation now names the
  visibility state instead of the generic "added to dashboard".
- Security Should-Fix: POST /api/me/member-profile and PUT bulk update
  accepted `is_public: true` verbatim — same smuggle class as the
  agent-visibility bug this PR fixes. Both paths now route the flag
  through orgDb.hasActiveSubscription, fall back to false, and emit a
  structured warning.
- Security Should-Fix: agents[].type was cast from arbitrary caller
  string into brand.json's manifest. Now gated through isValidAgentType
  and dropped if unknown.
- Code-reviewer Should-Fix: dead memberDb DI arg on
  demotePublicAgentsOnTierDowngrade removed. Enforcement test rewritten
  to assert the SELECT contains FOR UPDATE and the params[0] matches
  orgId (regression guard for the transactional invariant).
- DX Should-Fix: VisibilityWarning.agent_url typed as string and coerced
  at emit time — clients no longer risk rendering whatever raw shape
  arrived in the request body.

Out of scope, filed as follow-ups:
- #2825: brand.json write inside applyAgentVisibility's profile tx
- #2826: inner-tx tier column list duplicates resolveMembershipTier

Full server unit suite: 1814 pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test: silence CodeQL property-access warnings on mocked pg client

CodeQL was flagging every read of `__client` / `__connect` (imported
through `vi.mock` via a `@ts-expect-error` shim) as "property access
on null or undefined" — the mock factory always provides them, but
the static analyzer sees unknown/untyped imports.

Replace the bare imports with a validated module-level binding:
reach into the mocked module once, assert the expected shape, and
hand the rest of the file concretely-typed locals (`mockedClient`,
`mockedConnect`). Fails loudly with a descriptive error if the mock
factory ever drifts, which is a test-setup bug we'd want to surface
immediately rather than silently degrade into nullish reads.

Same behavior, same test coverage (10 scenarios pass). No `?.`
noise sprinkled across the assertion bodies.

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 Apr 26, 2026
…ction (#3288)

The pull_request workflow correctly catches a stale-view-of-main case
when a single PR runs against an out-of-date base, but two PRs whose
checks run in parallel against different snapshots can both pass
legitimately. That's how #3235 + #3244 both landed with migration 433
(and #2793 + #2800 with 419 in February).

Three layered fixes:

1. merge_group trigger — runs the check on the real merge commit when
   the GitHub merge queue is enabled. Only way to fully serialize.

2. Daily scheduled run as a safety net. If a duplicate slips through,
   the workflow goes red on main within 24 hours and branch protection
   blocks subsequent merges until the duplicate is renumbered.

3. Filesystem invariant test in server/tests/unit/migrate.test.ts that
   scans the migrations directory and asserts no duplicate version
   numbers / malformed filenames. Runs locally on npm test:unit and on
   every CI run regardless of workflow timing.

Error message also clearer about the rebase-and-renumber recovery
procedure, including the IF NOT EXISTS requirement so re-running on
systems that already applied the old number is a no-op.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants