feat(cli): add feedback add and delete commands for quick CLI feedback - #5988
feat(cli): add feedback add and delete commands for quick CLI feedback#5988kanadgupta wants to merge 43 commits into
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>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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@f2529331d4b8f0358f79a4e094ffcd9959d52bb2Preview 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>
kanadgupta
changed the title
…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".
| const FEEDBACK_STAGING: FeedbackEnvironment = { | ||
| url: "https://imrwaufzgcaczqmpnxyr.supabase.co", | ||
| key: "sb_publishable_puOyAlqG5J_XfBMTDM2Ckw_L5mieFdb", | ||
| }; | ||
|
|
||
| // No dedicated production feedback project exists yet (CLI-1946): production | ||
| // intentionally reuses the staging values until one is provisioned. | ||
| const FEEDBACK_PRODUCTION: FeedbackEnvironment = { ...FEEDBACK_STAGING }; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8a427eb17
ℹ️ 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: d8a427eb17
ℹ️ 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".
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
One thing to fix before merge (the open Codex P1 on debug URLs, commented on that thread), one doc fix (PR description still says user_id is never sent), and a couple of non-blocking notes inline.
the PR description says "The access token is never sent; user_id is never sent", but the current code sends the consent-gated gotrue UUID as user_id (add.handler.ts:91), and SIDE_EFFECTS.md documents that correctly. Since the description is what privacy sign-off reads, could you update that bullet to match?
supabase-js passes init.headers as a Headers instance; spreading one into a plain object yields zero entries, so the DoH rewrite dropped apikey, content-type, and x-feedback-token on every feedback request. Rebuild through the Headers constructor (which accepts records, Headers, and entry arrays) and cover the Request-embedded case. The Management API path (Effect's FetchHttpClient) passes a plain record and is byte-identical before/after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The preview/delete URLs carry the row's capability token as a delete_token=eq.<uuid> PostgREST filter; the --debug logger wrote that URL verbatim to stderr, leaking read/delete authority into terminal recordings and shared debug output. Redact the query-param value in the logged line only — the transport still receives the original URL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 1000-character limit was documented but only enforced server-side, so an over-limit message surfaced as a raw PostgREST error classified externalNetwork — a user mistake counted as a backend failure in the actionability KPIs. Mirror the check client-side (invalidInput, no request sent), counted in code points to match Postgres char_length. Same pattern as feedback delete's client-side UUID pre-validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The staging row was only deleted as a sequential test step, so an assertion failure between the add and the delete leaked it. Capture the token before asserting and run a best-effort exact-token delete in finally — inert when the round trip already removed the row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| let request = client | ||
| .from("interfaces_feedback") | ||
| .select("feedback") | ||
| .eq("delete_token", token) |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The delete token is a bearer capability, but it is placed in the PostgREST query string for both preview and deletion. Although the debug wrapper redacts it, HTTPS intermediaries, gateway access logs, tracing, or server request logs can retain delete_token=eq.<token>; anyone obtaining such a log can read and delete the feedback.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The delete_token is being exposed in URL query parameters (?delete_token=eq.<token>) for both the preview (SELECT) and delete (DELETE) operations, which can be retained in HTTP access logs, proxy logs, CDN/gateway traces, etc.
There are two recommended mitigations:
-
For the
preview(SELECT at line 137): Since RLS already gates row visibility via thex-feedback-tokenrequest header (as documented in the file header), the.eq("delete_token", token)URL filter is redundant for authorization. It can be removed from the SELECT query — RLS will correctly limit results to the authorized row via the header alone, keeping the token out of the URL query string. -
For the
delete(DELETE at line 159): PostgREST's filterless-delete protection prevents removing the.eq()filter. The cleanest solution is to expose a server-side RPC (e.g.,delete_interfaces_feedback(token uuid)) analogous to the existingsubmit_interfaces_feedbackRPC already used at line 118. Calling it viaclient.rpc("delete_interfaces_feedback", { token })sends the token in the POST request body rather than the URL, eliminating log exposure. If a new RPC is not feasible, consider using a non-sensitive row identifier (e.g., a surrogate primary key returned during submission) as the URL filter for DELETE, keeping thex-feedback-tokenheader as the actual authorization mechanism.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 129e3d627a
ℹ️ 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 workdir can be an untrusted checkout where supabase/.temp/project-ref is a symlink to a local secret; the resolver forwarded the raw file contents as project_ref to the feedback backend. Filter both the env/flag override and the file contents through PROJECT_REF_PATTERN — the same boundary legacyResolveSoftLinkedRef applies — degrading anything malformed to "unlinked". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
output.interactive is stdout-derived and clack's confirm answers on a single y/n keypress from any stdin, so `printf 'y' | feedback delete <token>` with a TTY stdout could confirm a permanent delete without --yes. Gate the prompt on both streams being TTYs and fail with NonInteractiveError otherwise — the behavior SIDE_EFFECTS.md already documented, and the same gate the add prompt uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
go-cli-divergences.md is a frozen historical record that no longer accumulates entries; the feedback commands are new CLI behavior documented through help text, tests, and their SIDE_EFFECTS.md files. The rows predate the ledger freeze. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The SIDE_EFFECTS notes and -o enum comments framed behavior around Go parity
("no Go counterpart", "Go-compat -o json", "no Go struct"); the TS shell is
the source of truth, so describe the behavior directly. No functional change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tapError skips the interrupt path and the sequential clear() never runs, so Ctrl-C mid-request left the delayed clack spinner (or its pending start timer) running while shutdown finalizers executed. Wrap the three request tasks in an onExit-based settle helper: cleared on success and interruption, failed on a typed failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
readPipedText collects the entire stream before the 1000-character check ran, so piping a huge file consumed unbounded memory before failing. Read piped stdin through pipedBytesStream with a 64 KB cap — 16x the limit's worst-case UTF-8 size, so trim-then-count semantics are unchanged for plausible input — and fail as over-limit once the cap is crossed without consuming further. 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: 3c41e19d54
ℹ️ 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".
| get [ErrorActionabilityId](): CliErrorActionabilityDeclaration { | ||
| // Both branches (PostgREST rejection, network failure/timeout) are | ||
| // failures of the external feedback backend, not user mistakes. | ||
| return actionability.externalNetwork; |
There was a problem hiding this comment.
When PostgREST returns a structured HTTP error—for example, an RPC permission rejection or validation failure—run creates this same error class, and this getter records it as externalNetwork just like a timeout or failed fetch. That misclassifies backend/API-status failures as network outages in KPI telemetry; carry a typed failure reason or status from run and select apiStatus for response errors while retaining externalNetwork for transport failures.
AGENTS.md reference: apps/cli/AGENTS.md:L373-L388
Useful? React with 👍 / 👎.
| const confirmed = yield* output.promptConfirm("Permanently delete this feedback?", { | ||
| defaultValue: false, | ||
| }); |
There was a problem hiding this comment.
When feedback delete <token> -o json runs in a TTY without --yes, the legacy flag leaves output.format === "text", and the quiet-progress wrapper still delegates this Clack prompt, which writes ANSI and prompt text to stdout before the handler emits the raw JSON payload. PTY-based automation therefore cannot parse stdout; the add command has the same problem when -o json reaches its missing-message prompt. Fresh evidence after the earlier payload fix is that both prompts still execute before the goFmt output branch; require noninteractive inputs/--yes for legacy machine mode or render these prompts to stderr.
AGENTS.md reference: apps/cli/AGENTS.md:L442-L450
Useful? React with 👍 / 👎.
| return total <= LEGACY_FEEDBACK_PIPE_CAP_BYTES; | ||
| }), | ||
| ), | ||
| Effect.catchTag("PlatformError", () => Effect.succeed(undefined)), |
There was a problem hiding this comment.
When piped stdin yields one or more chunks and then fails with PlatformError, this catch converts the stream failure to success but leaves the already buffered parts intact, so the command silently submits the truncated prefix as the user's feedback. Fresh evidence after the bounded-reader fix is that pipedBytesStream intentionally propagates read errors while this consumer resumes from partial state; either discard all buffered input and follow the empty-input path or map the read failure to a typed CLI error instead of sending corrupted feedback.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
…07-22/feedback-command Conflict resolutions and merge fallout: - apps/cli/package.json: kept develop's turbo-based test scripts and root-level tooling (per-package knip/nx blocks dropped); preserved the branch's gen:feedback-types script. - knip.json / .oxlintrc.json: carried the branch's ignore for the generated src/shared/feedback/database.types.ts into develop's root configs. - apps/cli/AGENTS.md: kept the branch's enriched legacy-infrastructure table with develop's LegacyCliConfig -> LegacyCliSettings rename applied (verified against the merged sources: projectId remains a bare SUPABASE_PROJECT_ID passthrough). - Renamed LegacyCliConfig -> LegacyCliSettings across the feedback command (code, tests, SIDE_EFFECTS.md) to match develop's rename. - Ported add.live.test.ts to develop's fixture-based live helper (test/cli from tests/helpers/live.ts), keeping the --profile supabase-staging pin. - pnpm-lock.yaml: took develop's side and regenerated with pnpm install. 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: de7f7372e3
ℹ️ 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".
| const dohFetch = legacyDohFetch({ | ||
| dnsResolver, | ||
| innerFetch: options.innerFetch, | ||
| resolver: options.resolver, | ||
| }); |
There was a problem hiding this comment.
When --dns-resolver https is active and Ctrl-C or the 10-second timeout fires during DNS resolution, this transport starts legacyDohFetch, whose Effect.runPromise(resolver(host)) does not observe the request's abort signal; cancellation can therefore leave the resolver request running in the background and holding the process open until it settles. Fresh evidence beyond the earlier native-transport abort fix is that the newly wired DoH path only forwards the signal to the inner fetch after resolution completes; thread that signal through the resolver so the whole feedback request is cancelled.
AGENTS.md reference: AGENTS.md:L92-L96
Useful? React with 👍 / 👎.
| } | ||
| yield* output.success("Thanks for the feedback!"); | ||
| yield* output.info( | ||
| `To delete this feedback later, run: supabase feedback delete ${deleteToken}`, |
There was a problem hiding this comment.
When feedback is submitted from a linked project, the backend requires the same project ref on preview and deletion, but this advertised command contains only the token. Copying it after leaving that checkout therefore resolves no matching ref and reports that the feedback was not found; include --project-ref when projectRef is present so the one-time receipt remains usable later.
Useful? React with 👍 / 👎.
| // Suppressed under `-o json` as well: stdout must stay payload-only, and | ||
| // the payload already carries the feedback text. | ||
| if (goFmt !== "json" && output.format === "text") { | ||
| yield* output.info(`Found feedback: "${feedbackText}"`); |
There was a problem hiding this comment.
When the stored feedback contains ANSI or OSC control sequences—for example, a malicious submitter gives another user its deletion token—this writes the backend-controlled text directly through Clack in text mode. Those bytes are interpreted by the terminal and can forge the confirmation display or modify terminal state such as the clipboard; strip or visibly escape control characters before rendering the human-readable preview while leaving structured JSON encoding unchanged.
Useful? React with 👍 / 👎.
| const projectRef = yield* legacyResolveFeedbackProjectRef( | ||
| cliSettings.workdir, | ||
| Option.orElse(args.projectRef, () => cliSettings.projectId), | ||
| ).pipe(Effect.map(Option.getOrUndefined)); |
There was a problem hiding this comment.
When --project-ref is malformed, Option.orElse selects it before validation, then legacyResolveFeedbackProjectRef silently discards it and falls through directly to the linked-ref file instead of reporting invalid input or trying SUPABASE_PROJECT_ID. In a checkout linked to another project this sends an unrelated context header and returns the misleading not-found error even though the user supplied the intended override; validate the explicit flag with the standard typed project-ref error before entering the soft fallback path.
AGENTS.md reference: AGENTS.md:L171-L175
Useful? React with 👍 / 👎.
| userId: | ||
| telemetryRuntime.consent === "granted" ? telemetryRuntime.identity.current() : undefined, |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The submit path sends the persisted GoTrue UUID as user_id whenever telemetry consent is granted, alongside free-form feedback that may contain secrets or other sensitive data. This creates an account-to-content record in the feedback backend, contradicting the PR’s stated guarantee that user_id is never sent and expanding identity exposure.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Remove the conditional that sends the GoTrue UUID as user_id. To honor the PR's stated privacy guarantee that user_id is never sent, replace the conditional expression with undefined unconditionally. This prevents any account identifier from being linked to free-form feedback submissions, regardless of telemetry consent status.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| userId: | |
| telemetryRuntime.consent === "granted" ? telemetryRuntime.identity.current() : undefined, | |
| userId: undefined, |
2 participants
What
Adds a TS-only
supabase feedbackcommand family (from the original brainstorm) to the legacy shell 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.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 key is a publishable (anon) key, safe to ship in the binary. 10s timeout.delete_tokenexactly once. Text mode prints it with a "to delete this later" hint;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;user_idis never sent.SUPABASE_PROJECT_ID→<workdir>/supabase/.temp/project-ref(the filesupabase linkwrites) → omitted. Reads the file directly (not viaLegacyProjectRefResolver, whose prompt path needs the platform API) so feedback works logged-out; a broken ref file degrades to "unlinked".How
feedback delete <token>worksPermanently delete this feedback? [y/N]);--yes/SUPABASE_YESskips it. Machine modes (json/stream-json) fail loudly without--yesrather than deleting silently — same contract aslogout.DELETEwithPrefer: count=exact; the CLI verifiesContent-Rangereports exactly one row. Authorization is thex-feedback-tokenrequest header matched by RLS — thedelete_token=eq.URL filter only satisfies PostgREST's filterless-delete rejection.x-feedback-project-refheader. The delete command resolves the ref as--project-ref→SUPABASE_PROJECT_ID→ linked-ref file and always sends whatever resolves (extra context against a context-free row is ignored server-side).{ "feedback": "...", "message": "Feedback deleted." }.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.Reviewer-relevant context
FeedbackSubmitter(insert-only) intoFeedbackClient(submit/preview/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.src/shared/feedback/database.types.tsis generated (supabase gen types) and excluded from formatting/knip.--profile supabase-staging), which also cleans up its own row each run.developpicked up the CLI-1970 docs restructure: the feedback commands are recorded indocs/go-cli-divergences.md(TS-only section) and registered inlegacy-docs-spec.tables.ts(other-commandstag) instead of the old porting-status tracker.LegacyCliConfig.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. TheAGENTS.mdrow that described it as resolving project-id fromconfig.tomlis 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