feat(cli): add feedback add and delete commands for quick CLI feedback - #5988
Conversation
The vendored effect clone in .repos/ drowns out workspace results in editor-wide search. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 8c53f2e.
LegacyCliConfig.projectId is a bare SUPABASE_PROJECT_ID env passthrough, so the feedback submission's project_ref was null in a linked project unless that env var happened to be set. Fall back to <workdir>/supabase/.temp/ project-ref, the file supabase link writes, mirroring the soft-load half of LegacyProjectRefResolver.resolveOptional. The file is read directly rather than through the resolver so the command keeps working unauthenticated; a broken ref file degrades to unlinked instead of failing the submission. The previous integration test injected projectId straight into the config mock, so it only proved the handler forwarded the field and never exercised resolution -- despite being named for the workdir-linked scenario that did not work. Replace it with coverage that seeds the real file, plus env precedence, unlinked, and unreadable-file cases. Also correct the AGENTS.md row claiming LegacyCliConfig reads project-id from config.toml, which is what made this field look project-aware. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 656f13a667
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@d6a27ec0881fa27a12993e6bf312b5cbf44a69e8Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830e565f2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae5d202bd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alias Restructures the TS-only feedback command from a single `supabase feedback` command (with a `btw` alias) into a `feedback` group with an `add` subcommand, following the nested-subcommand layout. Telemetry now records `command: "feedback add"`; behavior is otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…07-22/feedback-command # Conflicts: # apps/cli/src/legacy/commands/functions/delete/delete.integration.test.ts # apps/cli/src/legacy/commands/functions/download/download.integration.test.ts # apps/cli/src/legacy/telemetry/legacy-command-instrumentation.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ca265f84d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The production feedback project has its own connection constants since eccb3fa; staging is the persistent branch of that project. Drop the "reuses staging until provisioned" wording that described the interim state. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both commands create spinner tasks unconditionally. Outside text mode the shared output layers report them: stream-json emits a log event per task before the result event, and json writes [task] lines to stderr. The SIDE_EFFECTS examples showed only the result event. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The text-mode "Found feedback" line renders backend-stored text written by whoever submitted the row. Anyone holding a delete token can be handed one, so ESC/CSI/OSC sequences, C1 controls, and bidi overrides in that text could forge the confirmation display or write the clipboard. Reuse the shared control-character stripper for the human-readable preview; the json and -o json payloads still carry the text verbatim. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The capped pipe reader appended chunks as they arrived and turned a mid-stream PlatformError into success, so a pipe that delivered part of the message and then failed submitted the truncated prefix as the user's feedback. Track the failure and drop the buffer instead, degrading to "no piped input" exactly as a failure on the first read already did. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The -o/--output flag is independent of --output-format and leaves the text output layer active, so on a TTY without --yes the delete confirm prompt (and add's missing-message prompt) wrote ANSI and prompt text to stdout ahead of the raw JSON payload. Gate both prompts on -o json: delete fails with the existing NonInteractiveError (pass --yes), add falls through to the empty-message error. stdout stays payload-only whenever a machine format is requested. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A malformed --project-ref or SUPABASE_PROJECT_ID was silently filtered out and resolution fell through to the linked ref file, so a typo produced a misleading "not found" against a different project's context. Validate the user-supplied sources the way ProjectRefResolver does for every other command and fail with the shared InvalidProjectRefError; an empty --project-ref counts as unset. The linked ref file keeps its soft degradation so feedback still works logged-out and an untrusted checkout cannot inject a non-ref value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A row submitted from a linked project can only be deleted with the same ref presented, but the one-time receipt printed only the token. Copied into another directory it resolved no ref and reported "not found". Append --project-ref <ref> to the hint whenever a ref was sent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
FeedbackBackendError declared every failure as externalNetwork, so a PostgREST rejection (a permission or validation error the backend returned) counted as a network outage in KPI telemetry. Carry a typed reason: postgrest-js converts fetch rejections, timeouts, and aborts into an error envelope with status 0, which separates transport failures from real responses. Responses classify as apiStatus with the api_response fingerprint suffix, matching the decode/status split every Management API error class already makes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With --dns-resolver https, the DoH lookup ran through Effect.runPromise without the request's abort signal, so Ctrl-C or a caller timeout during resolution left the resolver fiber running until the DoH server answered. Pass the init (or Request) signal to runPromise so the whole request, lookup included, is cancelled together. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every other call site invokes the combinator; match the established style. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed the open review threads on the branch (each in its own commit):
|
avallete
left a comment
There was a problem hiding this comment.
Looks good — add/delete contracts hold (TTY/-o json gates, piped discard, consent-gated user_id, debug redact, DoH Headers + abort).
Non-blocking follow-ups inline: declare --project-ref on feedback add (same as delete), drop the remaining Go PersistentPostRun framing, and reuse BRANCH_UUID_PATTERN in delete.
|
Tested the command working on my end 👍 One design question I have after playing w/ this is whether we should prohibit reads of feedback on backend. IIUC that is the main reason for the extra project ref / user ID checks. I'd think that since the motivating risk is a user accidentally submitting sensitive data, it should be easy to delete and hard to read. Currently it's a little backwards of that philosophy in some ways. If you forget which project / account feedback was associated you might lose the ability to delete it even if you have the UUID, or if you submitted it without an associated project/user then others can read that feedback if they obtain the UUID. I was surprised to see it print out the submitted feedback (or a confirmation at all) before deletion since there's no harm to the user in deleting feedback, it's mainly for our benefit. Risk is low anyway so not thinking of that as a blocker, just a thought. |
feedback delete accepts --project-ref and the add receipt tells users to pass it, but feedback add itself rejected the flag as unknown and could only be attributed through SUPABASE_PROJECT_ID or a linked checkout. Declare the same optional flag with the same resolution order (flag, env, linked ref file; an empty flag counts as unset; a malformed flag fails with the shared InvalidProjectRefError). The flag is recorded in telemetry by name only with its value redacted, as delete does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Describe the telemetry-state finalizer in the CLI's own terms instead of "PersistentPostRun-shaped", per the agent guide, and reuse the shared permissive UUID pattern in feedback delete instead of a second copy. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Thanks @mattrossman! Appreciate you providing some fresh perspective on this — I understand why this could be confusing. I'll include some context on my thought process around this design (e.g., a confirmation prompt with the content preview) below:
Let me know if this thought process (all of which is completely based on my hunches 🙃) doesn't match your understanding, and/or any changes you'd like to see. This is Claude's suggestion for next steps if we do want to remove the preview/confirmation:
|
mattrossman
left a comment
There was a problem hiding this comment.
@kanadgupta That makes sense. I'm fine to keep it as-is to unblock this, and ideally make that proposed security change as a follow-up.
Assuming users have a record of the output of this command (which they'd need in order to delete anyways), I think it's reasonable to assume they have a record of the input feedback too. For instance I know Claude can chug away running commands without keeping you in the loop of everything it ran, but it still keeps the command history recorded in the session so you can ask it what was submitted. If I know my agent submitted feedback but it isn't able to tell me what it submitted, I'd prob err on the side of deletion anyway. Just my 2 cents, but maybe you had a different situation in mind where user can see the output but not input.
Anyways, not a blocker to this PR I don't think. Approving from a functionality POV, I'll leave it to CLI team for the implementation details approval.
…07-22/feedback-command # Conflicts: # apps/cli/src/cli/root.ts # apps/cli/src/command-internal/http-dns.ts # apps/cli/src/command-internal/http-dns.unit.test.ts # apps/cli/src/command-internal/http-errors.ts # apps/cli/src/commands/functions/delete/delete.integration.test.ts
develop added a guard that pins every Data.TaggedError tag literal to a committed fixture. The six feedback tags predate that guard on this branch, so record them; no tag was renamed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
feedback delete no longer reads the row before deleting it. The token-scoped
SELECT was the CLI's only read of feedback text; with it gone there is no
client contract keeping the backend read path alive, and CLI-2406 removes
the anon select grant and read policy. The confirmation prompt stays (now
ahead of the row's existence check, which comes from the DELETE's exact
count), the delete policy is unchanged, and machine payloads shrink to the
acknowledgement: --output-format json returns { message }, -o json returns
{ deleted: true }. FeedbackClient.preview and its tests are removed; the
control-character stripper goes back to being private to http-errors.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mattrossman agreed with this and you've convinced me! I decided to make the following change in 0adce4c:
With no client reading rows, the backend's anon read path has nothing to serve, so removing the select grant and read policy is tracked in CLI-2406 as a follow-up. That closes the "others can read it if they get the UUID" half without touching the CLI again. |
What
Adds a TS-only
supabase feedbackcommand family (from the original brainstorm) so users — and agents — can send quick, low-friction feedback to the Supabase team without filing a GitHub issue, and revoke a submission later (e.g. an accidentally pasted secret):Part of CLI-1946; the delete command is CLI-2188. Scope evolved in this thread:
feedback add(nobtwalias) plus a token-based delete path, rather than full user-scoped CRUD.How
feedback addworks--sentinel. Piped stdin is read in constant memory with a 64 KB cap; a read error mid-pipe discards the partial buffer rather than submitting a truncated message. Messages over the 1000-character limit are rejected client-side (counted in code points, matching Postgreschar_length).SECURITY DEFINERRPCsubmit_interfaces_feedback(feat: table for collecting interfaces feedback supabase#48420) viasupabase-js— the table has no insert grant, so the RPC is the only door and the delete token is always server-generated. The committed keys are publishable (anon) keys, safe to ship in the binary. 10s timeout.delete_tokenexactly once. Text mode prints it with a "to delete this later" hint (including--project-ref <ref>when the submission carried one, since the row can only be deleted with that ref presented);json/stream-jsoncarry it asdelete_tokenin the result payload. The CLI never persists it.is_agent/agent_namevia@vercel/detect-agent, to support the activation analysis in AI-961), and the linked project ref.metadata.source: "cli"distinguishes CLI rows from the future MCP path. The access token is never sent. The persisted gotrue user id (distinct_idintelemetry.json, stamped at login) is sent asuser_idonly when the user is logged in and telemetry consent is granted — best-effort attribution; logged-out or opted-out runs omit it. A row submitted with auser_idcan only be deleted by the same account.SUPABASE_PROJECT_ID→<workdir>/supabase/.temp/project-ref(the filesupabase linkwrites) → omitted. A malformedSUPABASE_PROJECT_IDfails with the shared invalid-ref error, like every other command's explicit ref. The file is read directly (not viaProjectRefResolver, whose prompt path needs the platform API) so feedback works logged-out; a broken or malformed ref file degrades to "unlinked".supabase-staging/supabase-localpost to the persistent staging branch of the feedback project; every other profile (including YAML-file profiles) posts to the production feedback project. Connection constants live insrc/shared/feedback/feedback-client.layer.ts.How
feedback delete <token>worksPermanently delete this feedback? [y/N]) as the guard against a mistyped or pasted token;--yes/SUPABASE_YESskips it. The prompt runs before the row's existence is known, so a wrong token gets the prompt and then the not-found error. Machine modes (json/stream-json, and-o json) fail loudly without--yesrather than deleting silently — same contract aslogout. Both stdin and stdout must be TTYs for the prompt, soprintf 'y' | supabase feedback delete <token>cannot confirm a delete without--yes.DELETEwithPrefer: count=exact; the CLI verifiesContent-Rangereports exactly one row. Zero rows → a friendly not-found error covering all three indistinguishable causes (wrong token, already deleted, project-ref/user-id context mismatch). Authorization is thex-feedback-tokenrequest header matched by RLS — thedelete_token=eq.URL filter only satisfies PostgREST's filterless-delete rejection.--debugrequest lines redact the filter.x-feedback-project-refheader, and rows submitted while logged in requirex-feedback-user-id. The delete command resolves the ref as--project-ref→SUPABASE_PROJECT_ID→ linked-ref file (flag and env validated, file soft) and always sends whatever resolves (extra context against a context-free row is ignored server-side). The user-id header is not consent-gated: it is functional auth context, and gating it would strand rows submitted before an opt-out.--output-format jsongives{ "message": "Feedback deleted." },-o jsongives{ "deleted": true }.Privacy note for reviewers
The feedback message, the delete token, and the
--project-refvalue go only to the feedback backend — never to PostHog. Message and token are positional arguments, whichextractChangedFlagNamesstructurally excludes from theflagstelemetry property;--project-refis recorded by name only with its value redacted. Regression tests assert none of them appear in captured analytics events.user_idis sent to the feedback backend under the conditions above and is documented inadd/SIDE_EFFECTS.md.Reviewer-relevant context
FeedbackSubmitter(insert-only) intoFeedbackClient(submit/delete) insrc/shared/feedback/feedback-client.{service,layer}.ts, and the profile→environment mapping and cli-config layer wiring were hoisted to the feedback family root (feedback.layers.ts,feedback-project-ref.ts) now that two commands share them.FeedbackBackendErrorcarries areason(responsevstransport) so a PostgREST rejection classifies asapiStatusin error telemetry while network failures stayexternalNetwork. postgrest-js reports fetch rejections, timeouts, and aborts as an error envelope withstatus: 0, which is the discriminator.fetch(supabase-js), not Effect'sHttpClient, so--debuglogging and--dns-resolver httpscompose at the fetch boundary (feedbackFetch). Along the way the shared DoH wrapper gained two fixes that apply to every command: WHATWGHeadersinstances survive the rewrite, and the request's abort signal now cancels the DoH lookup itself.src/shared/feedback/database.types.tsis generated (pnpm gen:feedback-types, from the production project) and excluded from formatting/knip; it is markedlinguist-generated.add.live.test.tsunder the gated live project. The default e2e file only exercises subcommand routing with zero network.docs-spec.tables.ts(other-commandstag) and each has aSIDE_EFFECTS.md.CommandSettings.projectId: it is a bareSUPABASE_PROJECT_IDenv passthrough — it does not readconfig.tomlor the linked-project file, so it isNonein a linked project unless that env var is set. An earlier revision of this branch used it directly as "the linked project ref", which meantproject_refwas alwaysnullin practice. The agent-guide row that described it as resolving project-id fromconfig.tomlwas corrected here, since that phrasing is what made the field look project-aware.services.integration.test.tsnow uses an isolated temp workdir instead ofprocess.cwd(), fixing machine-dependent behavior when the developer has localsupabase startstate.🤖 Generated with Claude Code