fix(visibility): close tier-gate blockers from #2793 review#2822
Merged
Conversation
#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>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This was referenced Apr 22, 2026
Closed
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>
Contributor
Author
|
Expert review pass complete. Addressed: Must-Fix (DX):
Should-Fix (Security):
Should-Fix (Code review):
Should-Fix (DX):
Filed as follow-ups (out of scope for this PR):
1814 server unit tests green, typecheck clean, no |
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>
Contributor
Author
|
Addressed CodeQL bot comments — all flagged the same class (property access on mock-factory exports that TS/CodeQL see as potentially undefined). Replaced the bare |
4 tasks
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
#2825) (#2830) * fix(visibility): move brand.json write outside applyAgentVisibility tx (#2825) The sibling `demotePublicAgentsOnTierDowngrade` was rewritten in #2822 to commit the profile tx before touching brand.json — a failed manifest write afterward just logs as drift for the /check reconciler to surface, rather than orphaning a manifest entry whose profile JSONB commit silently rolled back. `applyAgentVisibility` still had the old shape: `updateManifestAgents` was called inside the profile tx, so a succeeded-manifest-then-failed- commit combination would publish to brand.json without the authoritative visibility:'public' committing on the profile row. Rewrite to match the demote path: - Hold the profile lock + re-read tier + validate inside the tx - Stage the manifest op (add or remove, targeting a specific domain) - UPDATE profile + COMMIT - Execute `updateManifestAgents` outside the tx; on failure, log a structured `brand_json_drift` event so /check surfaces the divergence and return the success response anyway (the JSONB is now authoritative). Drive-by: `tests/integration/agent-visibility-e2e.test.ts` had two `demotePublicAgentsOnTierDowngrade(...)` calls with the pre-#2822 signature (passing `memberDb` as the 4th positional arg where `brandDb` is now expected). The root tsconfig excludes `tests/` so typecheck didn't catch this; fixed. New integration test pins the invariant: with updateManifestAgents forced to throw, POST /publish still returns 200, the manifest write was attempted (not skipped), and the profile JSONB reflects visibility:'public'. 1833 server unit tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(visibility): expert-review follow-ups on #2830 All Should-Fix items from the code / security / testing reviewers: - `client.release()` in the `applyAgentVisibility` finally block is now wrapped in try/catch + warn-log. A release failure will no longer mask the actual error (e.g., a COMMIT throw) from the try block. (code-reviewer) - `brand_json_drift` log now serializes `manifestErr` to `{ message, name }` explicitly instead of handing pino the raw Error. pg / other library errors can carry `.query` / `.parameters` that pino's default err serializer would emit — drops that risk without losing the diagnostic we actually want. (security-reviewer) - The failing-manifest integration test now seeds a community `brands` row via `brandDb.upsertDiscoveredBrand` so it exercises the intended `target==='public' && !isSelfHosted` branch rather than relying on the null-discovery fallthrough. (testing-expert) - Added a companion integration test for the self-hosted brand path that asserts `updateManifestAgents` is NOT called and the response contains the brand.json `snippet`. Locks in the specificity of the failing-manifest test — a future refactor that always calls `updateManifestAgents` would now flip the companion test red. (testing-expert) - Dropped the dead `originalUpdate` / `void originalUpdate` dance and its misleading comment. (code-reviewer, testing-expert) No behavior changes to production code beyond the log-payload shape and the release-failure handling. All Must-Fix items: none (reviewers cleared the core invariant). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(visibility): remove dead `committed` flag in applyAgentVisibility CodeQL (two bot comments on #2830) flagged: - line 854: `committed = true` assignment is useless (never read). - line 857: `if (!committed)` always evaluates to true. Both correct — the flag never actually guards the rollback because nothing between the `await client.query('COMMIT')` and the end of the try block can throw (plain assignments only). If COMMIT itself throws, `committed` is still false and the guard passes trivially. Dropping the flag. The catch now unconditionally attempts ROLLBACK with the existing `.catch(() => {})` swallow — pg no-ops a ROLLBACK on a finished tx, and the swallow keeps driver noise out of the bubbled error. Same behavior, less dead code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
bokelley
added a commit
that referenced
this pull request
Apr 23, 2026
`applyAgentVisibility` (inner-tx tier re-read from #2822) and the seat-check path in organization-db duplicated the same SQL and row shape against a pg client. A future column that `resolveMembershipTier` needs to consider had to land in three places (the resolver's input type, both SELECTs) or silently miss one. - Export `MembershipTierRow` type + `MEMBERSHIP_TIER_COLUMNS` tuple as the single source of truth for the resolver's input shape. `resolveMembershipTier` now takes the named type. - Export `readMembershipTierFromClient(client, orgId, { forUpdate? })` that runs the SELECT with the shared column list, parses the row, and returns `MembershipTier | null`. Optional `forUpdate` for callers that need to lock the `organizations` row too. - Both inline call sites (`organization-db.ts` seat check, `member-profiles.ts` applyAgentVisibility) use the helper. No behavior change. 1920 server + 631 root unit tests pass. Co-authored-by: Claude Opus 4.7 (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.
Summary
#2793 (three-tier agent visibility) shipped before the @EmmaLouise2018 review was addressed. This PR closes the three Blocker / two High findings from that review.
Blockers
High
Not addressed (reviewer's Medium / Nits)
Filing separately rather than scope-creeping this PR:
Test plan
🤖 Generated with Claude Code