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 40 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@3c41e19d542545464ef37d480e885913b824ed29Preview 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".
Applies the accepted Codex review findings on the feedback family: - Abort in-flight feedback requests on fiber interruption: the client's run helper now threads Effect.tryPromise's interruption signal into every postgrest call via AbortSignal.any with the 10s timeout, so Ctrl-C can no longer let a submit commit after cancellation. - Gate the interactive prompt on stdin.isTTY in addition to output.interactive, so whitespace-only piped stdin with a TTY stdout fails with the documented empty-message error instead of opening a prompt against exhausted stdin. - Honor the --agent yes|no override in the submission payload via legacyResolveAgentMode (hoisted from db query to legacy/shared); --agent no also suppresses the detected agent_name. - Refresh ~/.supabase/telemetry.json on every feedback add/delete run via the standard Effect.ensuring(telemetryState.flush) finalizer. - Honor -o json on both commands (machine payload via encodeGoJson, stdout payload-only); values outside feedback's pretty|json enum are rejected pre-run like db query's restricted set. yaml/toml stay unsupported: the struct-spec encoders reproduce Go field names and no Go struct exists for this TS-only command. - Route feedback HTTP through the legacy transport: a composed fetch wires --debug request logging and --dns-resolver https DoH resolution into the supabase-js client. - Move the real-backend add→delete round trip to the gated live tier (add.live.test.ts) and keep a hermetic e2e for subcommand routing via the no-network empty-message path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| 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".
| const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; | ||
| // The logger's write is synchronous (a gated stderr write); running it | ||
| // at this plain-fetch boundary keeps the wrapper a `typeof fetch`. | ||
| Effect.runSync(logger.http(method, url)); |
There was a problem hiding this comment.
When feedback delete <token> --debug runs, both the preview and DELETE requests encode the capability in the URL as delete_token=eq.<token>, and this call writes that entire URL to stderr. This exposes the token—and therefore authority to read and delete potentially sensitive feedback—in terminal recordings, CI logs, or debug output shared with support; redact this query parameter only for logging while sending the original URL to the transport.
AGENTS.md reference: apps/cli/AGENTS.md:L318-L318
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Sounds like an actual risk.
Maybe strip or redact the query string in legacyFeedbackFetch before logger.http(...) the Management API debug logger never has this problem because its credentials ride in headers.
There was a problem hiding this comment.
Agreed — the x-feedback-token header is never logged, but the delete_token=eq.<uuid> filter put the same capability in the URL, and legacyFeedbackFetch logged it verbatim. Fixed in 4ab24af: the debug logger now redacts the delete_token query-parameter value before the gated stderr write (the rest of the query string stays, so --debug output remains useful for diagnosing), while the transport receives the original URL. Unit-tested in feedback.layers.unit.test.ts, and the DoH-path integration test asserts the redacted line alongside the un-redacted wire URL. The filter itself stays — it's defense-in-depth under the RLS boundary, and the header remains the actual gate.
| const deleted = await runSupabaseLive( | ||
| [ |
There was a problem hiding this comment.
If the add succeeds but JSON parsing, an assertion, or the subsequent delete invocation fails, execution never completes this cleanup and the persistent staging row remains. Fresh evidence in the current revision is that deletion is still only a sequential test step after assertions rather than guaranteed teardown; retain the returned token and perform an exact best-effort delete in finally so failing live runs do not pollute staging.
AGENTS.md reference: AGENTS.md:L170-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but minor, a leaked row only happens on assertion failure between the add and the delete, staging-only, and the message self-identifies as test data. Fine to wrap the delete in a finally for hygiene, but I wouldn't block on it.
There was a problem hiding this comment.
Applied in 972df73 — the token is captured before the assertions and a best-effort exact-token delete now runs in a finally, so an assertion failure between the add and the delete no longer leaks the staging row. The delete's own assertions stay inside the try so cleanup can't mask a real failure, and the finally delete is inert when the round trip already removed the row.
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".
| */ | ||
| export function legacyFeedbackFetch(options: LegacyFeedbackFetchOptions): typeof globalThis.fetch { | ||
| const { dnsResolver, logger } = options; | ||
| const dohFetch = legacyDohFetch({ dnsResolver, innerFetch: options.innerFetch }); |
There was a problem hiding this comment.
When --dns-resolver https is used, this wrapper delegates supabase-js requests to legacyDohFetch, but supabase-js supplies init.headers as a Headers instance and legacy-http-dns.ts rebuilds it with { ...init?.headers, Host: ... }; spreading Headers yields no entries. The resulting submit, preview, and delete requests lose apikey, content type, and the x-feedback-* capability headers, so feedback operations fail in the exact environments that need the DoH fallback. Fresh evidence beyond the earlier transport-wiring comment is this incompatibility between the newly wired supabase-js transport and the existing DoH header reconstruction; clone with new Headers(init.headers) and set Host instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the ...init?.headers spread in legacyDohFetch preserves apikey/x-feedback-token as-is. Not an issue for this client
There was a problem hiding this comment.
I double-checked this one empirically because the spread behaves differently by header representation: supabase-js 2.110.7 passes init.headers as a WHATWG Headers instance, and spreading a Headers instance yields zero entries — Object.keys({ ...headers }) is [] while the real set carries apikey, x-feedback-token, and content-type. (The Management API path was never affected because Effect's Headers type is a plain record — which is also the shape the existing unit test happened to cover.) So Codex is right: with --dns-resolver https, feedback requests lost every header. Fixed in c38af45: legacyDohFetch now rebuilds through the Headers constructor (which accepts records, Headers instances, and entry arrays) and sets Host on the result, with a Headers-instance unit test plus a DoH-path integration test on the real feedback client wiring asserting the capability headers survive the rewrite.
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?
| const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; | ||
| // The logger's write is synchronous (a gated stderr write); running it | ||
| // at this plain-fetch boundary keeps the wrapper a `typeof fetch`. | ||
| Effect.runSync(logger.http(method, url)); |
There was a problem hiding this comment.
Sounds like an actual risk.
Maybe strip or redact the query string in legacyFeedbackFetch before logger.http(...) the Management API debug logger never has this problem because its credentials ride in headers.
| */ | ||
| export function legacyFeedbackFetch(options: LegacyFeedbackFetchOptions): typeof globalThis.fetch { | ||
| const { dnsResolver, logger } = options; | ||
| const dohFetch = legacyDohFetch({ dnsResolver, innerFetch: options.innerFetch }); |
There was a problem hiding this comment.
the ...init?.headers spread in legacyDohFetch preserves apikey/x-feedback-token as-is. Not an issue for this client
| const deleted = await runSupabaseLive( | ||
| [ |
There was a problem hiding this comment.
Valid but minor, a leaked row only happens on assertion failure between the add and the delete, staging-only, and the message self-identifies as test data. Fine to wrap the delete in a finally for hygiene, but I wouldn't block on it.
| const config = { | ||
| message: Argument.string("message").pipe( | ||
| Argument.withDescription( | ||
| "Freeform feedback. Bare words are joined with spaces. 1000 character limit.", |
There was a problem hiding this comment.
The char limit is documented here but only enforced server-side, so an over-limit message surfaces as a raw PostgREST error classified externalNetwork, so a user mistake gets counted as a backend failure in the actionability KPIs. A client-side length check that fails with an invalidInput classified error before any request would be nice. Non-blocking if you'd rather do it as a follow-up.
There was a problem hiding this comment.
Applied in 04c4e2a rather than deferring — it's a small check and feedback delete already sets the precedent (client-side UUID pre-validation exists for exactly this reason). Over-limit messages now fail before any request with LegacyFeedbackMessageTooLongError classified invalidInput, and the interactive prompt validates the same rule inline. One detail: the length is counted in code points to match Postgres char_length semantics, so the client never rejects a message the server would accept.
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.
| if (Option.isSome(fromEnv)) return fromEnv; | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| return yield* legacyReadProjectRefFile(fs, path, workdir).pipe( |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
workdir can point to an attacker-controlled checkout, where supabase/.temp/project-ref may be a symlink to a local secret. This read follows the link and sends the entire file as project_ref through feedback add, enabling exfiltration of credentials or other sensitive contents to the remote feedback backend.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Apply the same PROJECT_REF_PATTERN validation boundary that legacyResolveSoftLinkedRef in legacy-linked-state.ts already uses (line 78 of that file). Two changes are needed:
-
Add the import at the top of the file (line 2 area):
import { PROJECT_REF_PATTERN } from "../../config/legacy-project-ref.service.ts";
-
Filter the file ref against the pattern after reading it (lines 19–21). Change:
return yield* legacyReadProjectRefFile(fs, path, workdir).pipe( Effect.orElseSucceed(() => Option.none<string>()), );
to:
return yield* legacyReadProjectRefFile(fs, path, workdir).pipe( Effect.orElseSucceed(() => Option.none<string>()), Effect.map(Option.filter((ref) => PROJECT_REF_PATTERN.test(ref))), );
This ensures that if supabase/.temp/project-ref is a symlink to a secret file (e.g. ~/.supabase/access-token), its content — which will not match /^[a-z]{20}$/ — is silently discarded instead of being sent as project_ref to the remote feedback backend. This mirrors the explicit comment in legacy-linked-state.ts (lines 50–64) documenting this exact security boundary.
There was a problem hiding this comment.
Applied in 0c11b16 — the resolver now filters both the env/flag value and the file contents through the same PROJECT_REF_PATTERN boundary legacyResolveSoftLinkedRef uses, so a symlinked or corrupted .temp/project-ref degrades to "unlinked" instead of shipping its contents as project_ref (an invalid env override now also falls through to the file, matching legacy-linked-state's precedence). Worth noting the blast radius is narrower than "exfiltration": RLS means an attacker can never read the row back — but secret material leaving the machine at all is reason enough, and this restores consistency with the boundary legacy-linked-state.ts already documents. Integration-tested for both the malformed-file and malformed-env cases.
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".
| | `feedback add` | [`../src/legacy/commands/feedback/add/add.command.ts`](../src/legacy/commands/feedback/add/add.command.ts) | Quick feedback submission, legacy shell only (CLI-1946). Submits through the `submit_interfaces_feedback` RPC via supabase-js with a committed publishable key and prints the server-issued delete token once; profile-driven staging/production environments (production currently reuses staging). `-o` accepts `pretty\|json` only (no Go struct exists for the yaml/toml encoders). | | ||
| | `feedback delete` | [`../src/legacy/commands/feedback/delete/delete.command.ts`](../src/legacy/commands/feedback/delete/delete.command.ts) | Deletes previously submitted feedback using the token printed by `feedback add` (CLI-2188). Previews the feedback text, then hard-deletes via the token-gated RLS policy (`x-feedback-token` + optional `x-feedback-project-ref` / `x-feedback-user-id` headers). `-o` accepts `pretty\|json` only (no Go struct exists for the yaml/toml encoders). | |
There was a problem hiding this comment.
Remove these two rows: they document brand-new commands in a file reserved as a frozen historical record of the completed TypeScript port. Continuing to add features here undermines that ledger's historical meaning; the new commands are already documented through help text, tests, and their SIDE_EFFECTS.md files, which are the required destinations for new behavior.
AGENTS.md reference: apps/cli/AGENTS.md:L514-L517
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed in fead113. The rows predate the ledger freeze (they were added back when AGENTS.md required recording TS-only commands here in the same change; the freeze merged into this branch afterwards), but under the current framing they're new-command docs in a historical record — and everything in them is already covered by the two SIDE_EFFECTS.md files and help text.
|
|
||
| ## Notes | ||
|
|
||
| - TS-only command — no Go CLI counterpart, so no Go-parity constraints apply. |
There was a problem hiding this comment.
Remove or rewrite this newly added note, which is duplicated in the delete command's SIDE_EFFECTS.md: the stable TypeScript shell is now authoritative, so new documentation must describe behavior in its own terms rather than asserting that Go parity or a Go counterpart is irrelevant.
AGENTS.md reference: apps/cli/AGENTS.md:L52-L58
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rewritten in 281e791 — both SIDE_EFFECTS.md notes are gone, and I swept the rest of the feedback family for the same framing per the opportunistic-cleanup rule: feedback-output.ts's "no Go struct" rationale now describes the -o enum restriction in terms of the *.go-payload.ts struct specs it lacks, and the "Go-compat -o json wins" comments in both handlers now cite the legacy shell invariant directly.
| agentName, | ||
| }, | ||
| }) | ||
| .pipe(Effect.tapError(() => sending.fail())); |
There was a problem hiding this comment.
When Ctrl-C interrupts an in-flight submission, tapError does not run for the interruption cause and the subsequent sending.clear() is skipped. If the delayed Clack spinner has started, it therefore remains active with terminal state such as the hidden cursor unrecovered while telemetry finalizers and shutdown run; if interruption arrives before the delay, the pending timer can start the spinner after cancellation. Manage the task with an interruption-safe finalizer (onExit/ensuring) and apply the same fix to the lookup and deletion tasks.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mechanism confirmed — tapError skips the interrupt path and the pending 200 ms start timer can fire after cancellation. One severity note: clack's spinner registers its own SIGINT/exit hooks that restore the cursor at process exit, so the residual failure is a cosmetic stray line racing shutdown output rather than a corrupted terminal. Fixed for feedback's three tasks in 6234f41 via an Effect.onExit-based settle helper (feedback-task.ts: cleared on success and interruption, failed on a typed failure), unit-tested including the mid-flight interrupt. Since this is a property of the documented output.task + tapError + clear pattern every legacy command follows, the general fix (an interruption-safe shared combinator, plus making the task handle's settle idempotent) is tracked in CLI-2222 rather than hand-patched per command.
|
|
||
| const stdin = yield* Stdin; | ||
| if (!stdin.isTTY) { | ||
| const piped = yield* stdin.readPipedText; |
There was a problem hiding this comment.
When stdin contains a very large stream—for example, a user accidentally pipes a multi-gigabyte log—the new command calls readPipedText, whose implementation collects the entire stream into memory, and only afterward applies the 1,000-character limit. This can make the CLI consume unbounded memory or be killed instead of returning the documented length error; consume only enough input to establish that the code-point limit was exceeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Applied in 3c41e19 using the constant-memory pipedBytesStream the Stdin service already exposes: piped input is read up to a 64 KB cap and anything past it fails with the existing LegacyFeedbackMessageTooLongError before further buffering. The cap is 16× the limit's worst-case UTF-8 size (4 bytes/code point), so the established trim-then-count semantics are unchanged for any plausible input; read errors still degrade to "no piped input" exactly like readPipedText did. SIDE_EFFECTS.md documents the cap.
| if (!yes) { | ||
| const confirmed = yield* output.promptConfirm("Permanently delete this feedback?", { | ||
| defaultValue: false, | ||
| }); |
There was a problem hiding this comment.
When stdin is piped but stdout remains a terminal—for example, printf 'y\n' | supabase feedback delete <token>—output.format is still text, so this branch invokes Clack against non-TTY stdin instead of enforcing the documented requirement to pass --yes in non-interactive contexts. Depending on the pipe and Clack behavior, it can consume the piped answer and delete without --yes, cancel, fail, or wait unexpectedly; check stdin TTY state before prompting and return the non-interactive error otherwise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same class as the add-side prompt gate from the earlier review round — 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. Fixed in db5e615: the confirm is now gated on stdin.isTTY && output.interactive, failing with NonInteractiveError otherwise — which is what SIDE_EFFECTS.md already documented ("prompt unavailable (non-interactive …) → exit 1"), so the code now matches the contract. Covered with the same pair of asymmetric-TTY integration tests add has, plus a --yes-with-piped-stdin escape-hatch test.
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 👍 / 👎.
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