Skip to content

release: 0.2.0#2

Open
stainless-app[bot] wants to merge 58 commits intomainfrom
release-please--branches--main--changes--next
Open

release: 0.2.0#2
stainless-app[bot] wants to merge 58 commits intomainfrom
release-please--branches--main--changes--next

Conversation

@stainless-app
Copy link
Copy Markdown
Contributor

@stainless-app stainless-app Bot commented Apr 3, 2026

Automated Release PR

0.2.0 (2026-05-08)

Full Changelog: v0.1.0...v0.2.0

Features

  • allow - as value representing stdin to binary-only file parameters in CLIs (40329ac)
  • api: api update (e11fbd3)
  • api: api update (cde57e6)
  • api: api update (15efdbc)
  • api: api update (6ea47ba)
  • api: api update (05fc238)
  • api: api update (2f69100)
  • api: api update (07389a5)
  • better error message if scheme forgotten in CLI *_BASE_URL/--base-url (665e431)
  • cli: add --raw-output/-r option to print raw (non-JSON) strings (4222549)
  • cli: alias parameters in data with x-stainless-cli-data-alias (fc1ebc7)
  • cli: send filename and content type when reading input from files (4b76c79)
  • support passing path and query params over stdin (14f99ef)

Bug Fixes

  • cli: correctly load zsh autocompletion (c419250)
  • cli: fix incompatible Go types for flag generated as array of maps (a95a486)
  • fall back to main branch if linking fails in CI (28b2498)
  • fix for failing to drop invalid module replace in link script (ad3cbf1)
  • fix quoting typo (d4f3537)
  • flags for nullable body scalar fields are strictly typed (c983439)

Chores

  • add documentation for ./scripts/link (2586047)
  • ci: support manually triggering release workflow (b5836aa)
  • cli: additional test cases for ShowJSONIterator (5784e71)
  • cli: fall back to JSON when using default "explore" with non-TTY (1b5c7e9)
  • cli: let --format raw be used in conjunction with --transform (d9c0c77)
  • cli: switch long lists of positional args over to param structs (4a9e250)
  • cli: use ShowJSONOpts as argument to formatJSON instead of many positionals (9e0c109)
  • internal: more robust bootstrap script (014bf27)
  • mark all CLI-related tests in Go with t.Parallel() (5fc2402)
  • modify CLI tests to inject stdout so mutating os.Stdout isn't necessary (b7fd553)
  • redact api-key headers in debug logs (6347635)
  • switch some CLI Go tests from os.Chdir to t.Chdir (2f49e45)
  • tests: bump steady to v0.22.1 (522092e)

Documentation


This pull request is managed by Stainless's GitHub App.

The semver version number is based on included commit messages. Alternatively, you can manually set the version number in the title of this pull request.

For a better experience, it is recommended to use either rebase-merge or squash-merge when merging this pull request.

🔗 Stainless website
📚 Read the docs
🙋 Reach out for help or questions

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from e9ea7d3 to 27e3edd Compare April 3, 2026 05:40
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 27e3edd to 25aedf0 Compare April 3, 2026 05:40
@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 5/5 - Safe to Merge

Safe to merge — this release PR for version 0.2.0 appears clean with no issues identified across the reviewed files. The automated review found no logic bugs, security concerns, or correctness problems in any of the 6 reviewed changed files. This looks like a straightforward version bump or release packaging PR with no substantive technical concerns raised.

Key Findings:

  • No review comments were generated across 6 of 7 changed files, indicating the code changes are clean and well-structured.
  • Zero critical, significant, or medium-severity issues were detected by heuristic analysis, suggesting the release changes are safe.
  • The PR is scoped as a release (0.2.0), which typically involves version metadata updates and changelog entries — low-risk changes by nature.

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 25aedf0 to 03c6c45 Compare April 3, 2026 05:41
@stainless-app
Copy link
Copy Markdown
Contributor Author

stainless-app Bot commented Apr 3, 2026

🧪 Testing

To try out this version of the SDK:

Download and unzip: 'https://pkg.stainless.com/s/hyperspell-cli/634763523a93a97f36a758f4a49b0e6df83b1d69/dist.zip'. On macOS, run 'xattr -d com.apple.quarantine {executable name}'.

Expires at: Sun, 07 Jun 2026 05:13:02 GMT
Updated at: Fri, 08 May 2026 05:13:02 GMT

@canaries-inc
Copy link
Copy Markdown

canaries-inc Bot commented Apr 3, 2026

🐤 Canary Summary

This PR enhances CLI error messaging for base URL configuration:

  • Added upfront validation for HYPERSPELL_BASE_URL environment variable and --base-url flag
  • Users now get clear error messages specifying which configuration source is missing the scheme
  • Validation happens before CLI app execution, catching misconfiguration early
  • Error messages explicitly state expected format (http:// or https://), improving troubleshooting experience

Affected User Flows

Component User Flows
CLI Base URL Validation Improved
Configure API endpoint via env var: Generic URL parse errors → clear message 'HYPERSPELL_BASE_URL "example.com" is missing a scheme (expected http:// or https://)'

Configure API endpoint via CLI flag: Runtime URL errors → immediate validation error '--base-url "localhost:8080" is missing a scheme (expected http:// or https://)'

Start CLI with invalid base URL: App crashes with cryptic errors → early exit with actionable error message before app initialization

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 5/5 - Safe to Merge

Safe to merge — this PR cleanly delivers the 0.2.0 release with well-scoped additions including ValidateBaseURL in pkg/cmd/cmdutil.go, onceStdinReader in pkg/cmd/flagoptions.go for safe stdin consumption, and proper wiring of URL validation in both the CLI flag layer (pkg/cmd/cmd.go) and the environment variable path (cmd/hyperspell/main.go). No review comments were generated across the 8 reviewed changed files, and the heuristic analysis found zero critical, significant, or medium issues. The changes are logically coherent — validating the base URL at both the flag and env-var entry points is a sound defensive pattern, and the once-reader abstraction prevents double-consumption of stdin, which is a common correctness concern in CLI tools.

Key Findings:

  • ValidateBaseURL correctly enforces http:///https:// scheme at both the --base-url flag validation site and the HYPERSPELL_BASE_URL environment variable startup path, closing two distinct injection surfaces consistently.
  • onceStdinReader in pkg/cmd/flagoptions.go addresses the classic stdin double-read hazard in CLI pipelines — safe and idiomatic use of sync.Once or equivalent guard for this pattern.
  • All 8 reviewed files produced zero findings from both automated heuristics and manual review comments, indicating the implementation is clean and the PR scope is well-contained to the stated feature set.
Files requiring special attention
  • pkg/cmd/cmdutil.go
  • pkg/cmd/flagoptions.go
  • cmd/hyperspell/main.go

@canaries-inc
Copy link
Copy Markdown

canaries-inc Bot commented Apr 3, 2026

🐤 Canary Proposed Tests

No testable user journeys found for this PR.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 4/5 - Mostly Safe

Safe to merge — this PR introduces well-scoped features including onceStdinReader for enforcing single stdin consumption, isStdinPath() for detecting stdin aliases (-, /dev/fd/0, /dev/stdin), and ValidateBaseURL for HTTP/HTTPS scheme validation in cmdutil.go. The implementation addresses a clear need for binary file parameter support via stdin and tightens base URL validation across the CLI surface. No review comments were generated and all heuristic checks passed cleanly, making this a low-risk release candidate.

Key Findings:

  • onceStdinReader in pkg/cmd/flagoptions.go correctly enforces single-consumption semantics for stdin, which is a sound defensive pattern preventing silent double-read bugs in flag parsing pipelines.
  • ValidateBaseURL in pkg/cmd/cmdutil.go adds meaningful user-facing validation for scheme correctness (http:///https://), reducing the class of misconfiguration errors that could previously surface as cryptic network failures.
  • The isStdinPath() helper correctly enumerates the common stdin path aliases, though coverage of edge cases like /proc/self/fd/0 is not mentioned — this is a minor gap but not a blocking concern.
  • No automated review issues were flagged and 8 of 9 changed files were reviewed, providing high confidence in the change surface.
Files requiring special attention
  • pkg/cmd/flagoptions.go
  • pkg/cmd/cmdutil.go
  • cmd/hyperspell/main.go

@@ -27,6 +29,8 @@ func TestInnerFlagSet(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Adding t.Parallel() inside the range loop without capturing tt (via tt := tt) causes all parallel subtests to share the same loop variable — in Go < 1.22, by the time the subtests run, tt will hold the last iteration's value, making all tests use identical inputs.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In file `internal/requestflag/innerflag_test.go`, inside the `for _, tt := range tests` loop in `TestInnerFlagSet`, `t.Parallel()` is called without first capturing the loop variable. For Go versions before 1.22, this causes a data race where all parallel subtests may use the last value of `tt`. Add `tt := tt` immediately before `t.Parallel()` on the line after `t.Run(tt.name, func(t *testing.T) {` to create a per-iteration local copy.

Comment on lines 348 to 393
// Test initialization and setting
t.Run("PreParse initialization", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.PreParse())
assert.True(t, strFlag.applied)
assert.Equal(t, "default-string", strFlag.Get())
})

t.Run("Set string flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.Set("string-flag", "new-value"))
assert.Equal(t, "new-value", strFlag.Get())
assert.True(t, strFlag.IsSet())
})

t.Run("Set int flag with valid value", func(t *testing.T) {
t.Parallel()

assert.NoError(t, superstitiousIntFlag.Set("int-flag", "100"))
assert.Equal(t, int64(100), superstitiousIntFlag.Get())
assert.True(t, superstitiousIntFlag.IsSet())
})

t.Run("Set int flag with invalid value", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "not-an-int"))
})

t.Run("Set int flag with validator failing", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "13"))
})

t.Run("Set bool flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, boolFlag.Set("bool-flag", "true"))
assert.Equal(t, true, boolFlag.Get())
assert.True(t, boolFlag.IsSet())
})

t.Run("Set slice flag with multiple values", func(t *testing.T) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Adding t.Parallel() to these subtests while they all share the same strFlag, superstitiousIntFlag, and boolFlag instances (defined in the outer TestFlagSet scope) introduces data races — concurrent calls to PreParse, Set, and Get on the same flag objects will race under -race, causing flaky or incorrect test results.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In `internal/requestflag/requestflag_test.go`, the diff adds `t.Parallel()` to subtests inside `TestFlagSet` (around lines 347–393). However, those subtests all operate on the same shared `strFlag`, `superstitiousIntFlag`, and `boolFlag` variables declared in the outer test function. Running them in parallel causes concurrent reads and writes to those shared `Flag` structs, which is a data race.

Fix: Either (a) remove `t.Parallel()` from the subtests that share these outer-scope flag variables, or (b) move the flag construction inside each subtest so each parallel subtest has its own independent instance. The subtests at the bottom of `TestFlagSet` that already create local `sliceFlag` variables are safe to keep parallel.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 3/5 - Review Recommended

Not safe to merge without fixes — while this PR delivers meaningful improvements like ValidateBaseURL, onceStdinReader, and startup validation in main.go, the test suite changes introduce two concrete correctness bugs that undermine the value of the new tests. Specifically, innerflag_test.go adds t.Parallel() inside a range loop without capturing the loop variable tt := tt, meaning all parallel subtests will use the last iteration's value in Go < 1.22. Additionally, requestflag_test.go runs parallel subtests that share mutable flag instances (strFlag, superstitiousIntFlag, boolFlag) defined in the outer TestFlagSet scope, introducing data races under -race that will cause non-deterministic failures.

Key Findings:

  • In internal/requestflag/innerflag_test.go, t.Parallel() is called inside a range loop without tt := tt variable capture — in Go < 1.22, all parallel subtests will reference the same final loop value, making the tests effectively useless as they all run with identical (last) inputs.
  • In internal/requestflag/requestflag_test.go, parallel subtests share the same outer-scope strFlag, superstitiousIntFlag, and boolFlag instances and concurrently call PreParse, Set, and Get on them — this is a real data race detectable with go test -race and can cause intermittent test failures or undefined behavior.
  • The production-side changes (ValidateBaseURL, onceStdinReader, startup exit-code validation) appear well-structured and purposeful, so the PR's core feature work is sound — only the test infrastructure is broken.
  • Both test bugs are in the same package and were introduced together as part of parallelizing the test suite, suggesting a systematic oversight when adding t.Parallel() calls without auditing shared state.
Files requiring special attention
  • internal/requestflag/innerflag_test.go
  • internal/requestflag/requestflag_test.go

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 03c6c45 to 5683b46 Compare April 3, 2026 21:30
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 5683b46 to 373f189 Compare April 4, 2026 05:08
Comment on lines 348 to 398
// Test initialization and setting
t.Run("PreParse initialization", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.PreParse())
assert.True(t, strFlag.applied)
assert.Equal(t, "default-string", strFlag.Get())
})

t.Run("Set string flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.Set("string-flag", "new-value"))
assert.Equal(t, "new-value", strFlag.Get())
assert.True(t, strFlag.IsSet())
})

t.Run("Set int flag with valid value", func(t *testing.T) {
t.Parallel()

assert.NoError(t, superstitiousIntFlag.Set("int-flag", "100"))
assert.Equal(t, int64(100), superstitiousIntFlag.Get())
assert.True(t, superstitiousIntFlag.IsSet())
})

t.Run("Set int flag with invalid value", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "not-an-int"))
})

t.Run("Set int flag with validator failing", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "13"))
})

t.Run("Set bool flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, boolFlag.Set("bool-flag", "true"))
assert.Equal(t, true, boolFlag.Get())
assert.True(t, boolFlag.IsSet())
})

t.Run("Set slice flag with multiple values", func(t *testing.T) {
t.Parallel()

sliceFlag := &Flag[[]int64]{
Name: "slice-flag",
Default: []int64{},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: The subtests now run in parallel but all mutate shared strFlag, superstitiousIntFlag, and boolFlag declared in the outer TestFlagSet scope — this introduces data races on Flag internal fields (value, hasBeenSet, applied, count) with no synchronization, causing non-deterministic test failures and potential panics.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In file `internal/requestflag/requestflag_test.go`, the `t.Parallel()` calls added to subtests inside `TestFlagSet` (starting around line 348) cause data races because `strFlag`, `superstitiousIntFlag`, and `boolFlag` are shared mutable state across parallel subtests. Fix this by either: (1) removing `t.Parallel()` from all subtests that share these outer-scope flag variables, or (2) moving the flag declarations inside each subtest so each parallel subtest operates on its own independent flag instance.

Comment thread pkg/cmd/memory.go Outdated
Comment on lines 375 to 379
Usage: "The file to ingest.",
Required: true,
BodyPath: "file",
},
&requestflag.Flag[any]{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Removing FileInput: true means the requestflag package will no longer treat this flag as a file path to read and stream — it will pass the raw string value (the filename) as the body instead of the file contents, silently breaking the upload endpoint.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/memory.go around line 375, the `FileInput: true` field was removed from the `requestflag.Flag[string]` struct for the 'file' flag in the `memoriesUpload` command. This field is required for the requestflag package to read the actual file contents from the provided path and pass them to the multipart form upload. Without it, only the raw filename string is sent as the body, breaking file uploads silently. Restore `FileInput: true` to the flag definition.

@@ -114,6 +116,8 @@ func TestEncode(t *testing.T) {

for name, test := range tests {
t.Run(name, func(t *testing.T) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Adding t.Parallel() inside the range loop without capturing name and test per iteration causes all subtests to close over the same loop variables — in Go < 1.22 they will all run with the last iteration's values, producing incorrect/flaky test results.

Affected Locations:

  • internal/apiquery/query_test.go:118-118
  • internal/requestflag/innerflag_test.go:31-31
  • internal/requestflag/requestflag_test.go:60-61
🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In internal/apiquery/query_test.go, around line 116-117, the for-range loop over `tests` was given a `t.Parallel()` call inside the `t.Run` callback. Without capturing the loop variables locally, all parallel subtests will share the same `name` and `test` variables (the last iteration's values in Go < 1.22), causing incorrect test behaviour. Add `name, test := name, test` immediately after the `t.Run` open brace (before `t.Parallel()`) to shadow and capture each iteration's values.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 1/5 - Blocking Issues

Not safe to merge — this PR introduces multiple correctness-breaking bugs that will cause non-deterministic test failures and a silent functional regression in production. In internal/requestflag/requestflag_test.go, subtests now run in parallel while mutating shared strFlag, superstitiousIntFlag, and boolFlag instances declared in the outer TestFlagSet scope, creating data races on Flag internal fields. In pkg/cmd/memory.go, removing FileInput: true silently breaks the upload endpoint by passing a raw filename string as the request body instead of streaming the file contents. Additionally, internal/apiquery/query_test.go adds t.Parallel() inside a range loop without capturing loop variables, causing all subtests to close over the last iteration's values in Go < 1.22 — a pre-existing version of this same loop-variable closure bug in internal/requestflag/innerflag_test.go also remains unresolved from a prior review, indicating a pattern of unsafe parallel test refactoring throughout the codebase.

Key Findings:

  • In pkg/cmd/memory.go, removing FileInput: true from the flag options means the requestflag package will pass the raw filename string as the HTTP body instead of reading and streaming the file contents — this silently breaks the upload endpoint with no error surfaced to the user.
  • In internal/requestflag/requestflag_test.go, parallel subtests all mutate the shared strFlag, superstitiousIntFlag, and boolFlag variables from the outer TestFlagSet scope without synchronization, introducing data races on Flag internal fields (value, hasBeenSet, applied, count) that will cause non-deterministic and flaky test results.
  • In internal/apiquery/query_test.go, t.Parallel() is called inside a range loop without capturing name and test per iteration — in Go versions below 1.22 all parallel subtests will close over the final loop variable values, producing incorrect test behavior; this same anti-pattern also remains unresolved in internal/requestflag/innerflag_test.go from a prior review.
  • The PR does deliver meaningful improvements — ValidateBaseURL enforcement at startup, io.Writer abstraction in writeBinaryResponse for testability, and stdin file input support are all well-motivated changes — but the functional regression in memory.go and the data-race bugs in the test suite make the PR unsafe to merge in its current state.
Files requiring special attention
  • pkg/cmd/memory.go
  • internal/requestflag/requestflag_test.go
  • internal/apiquery/query_test.go
  • internal/requestflag/innerflag_test.go

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 373f189 to b0b376a Compare April 7, 2026 08:10
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from b0b376a to 4d95a0b Compare April 7, 2026 08:11
Comment on lines 348 to 392
// Test initialization and setting
t.Run("PreParse initialization", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.PreParse())
assert.True(t, strFlag.applied)
assert.Equal(t, "default-string", strFlag.Get())
})

t.Run("Set string flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.Set("string-flag", "new-value"))
assert.Equal(t, "new-value", strFlag.Get())
assert.True(t, strFlag.IsSet())
})

t.Run("Set int flag with valid value", func(t *testing.T) {
t.Parallel()

assert.NoError(t, superstitiousIntFlag.Set("int-flag", "100"))
assert.Equal(t, int64(100), superstitiousIntFlag.Get())
assert.True(t, superstitiousIntFlag.IsSet())
})

t.Run("Set int flag with invalid value", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "not-an-int"))
})

t.Run("Set int flag with validator failing", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "13"))
})

t.Run("Set bool flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, boolFlag.Set("bool-flag", "true"))
assert.Equal(t, true, boolFlag.Get())
assert.True(t, boolFlag.IsSet())
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: All subtests now run in parallel but share the same strFlag, superstitiousIntFlag, and boolFlag instances defined in the outer scope — concurrent reads and writes to their internal mutable fields (value, hasBeenSet, applied, count) will cause data races detected by go test -race and produce non-deterministic results.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In `internal/requestflag/requestflag_test.go`, the subtests inside `TestFlagSet` (starting around line 347) were made parallel via `t.Parallel()`, but they all share the same `strFlag`, `superstitiousIntFlag`, and `boolFlag` variables declared in the outer function scope. These `Flag` structs have mutable fields (`value`, `hasBeenSet`, `applied`, `count`) that are written concurrently, causing data races. Fix this by either: (1) removing `t.Parallel()` from subtests that share these outer variables, or (2) moving the flag construction inside each subtest so each parallel subtest has its own local instance.

Comment thread pkg/cmd/flagoptions_test.go
Comment thread pkg/cmd/memory.go
Comment on lines 371 to 380
Suggest: true,
Flags: []cli.Flag{
&requestflag.Flag[string]{
Name: "file",
Usage: "The file to ingest.",
Required: true,
BodyPath: "file",
FileInput: true,
Name: "file",
Usage: "The file to ingest.",
Required: true,
BodyPath: "file",
},
&requestflag.Flag[any]{
Name: "collection",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: The FileInput: true field has been removed from the file flag, which likely means the CLI will pass the raw string (file path) instead of reading and streaming the file contents for multipart upload — breaking the memories upload command.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/memory.go around line 371-380, the `FileInput: true` field was removed from the `requestflag.Flag[string]` definition for the 'file' flag in the `memoriesUpload` command. This field is required so the CLI reads the file from disk and passes its contents (not just the path string) to the multipart form upload. Restore `FileInput: true` to the flag definition to fix the upload functionality.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 2/5 - Changes Needed

Not safe to merge — this PR introduces three high-severity bugs that must be addressed before merging. In pkg/cmd/flagoptions_test.go, parallel subtests iterate over tt without capturing it, meaning all subtests run with the final loop value in Go < 1.22, making the entire parallel test suite unreliable. In internal/requestflag/requestflag_test.go, shared mutable flag instances (strFlag, superstitiousIntFlag, boolFlag) are accessed concurrently across parallel subtests, introducing data races that go test -race will catch — and this concern has been flagged in two prior unresolved review comments. Most critically, the removal of FileInput: true from the file flag in pkg/cmd/memory.go likely breaks the memories upload command by passing a raw file path string instead of streaming file contents to the multipart upload. The PR's goals — stdin support for @file parameters, ValidateBaseURL enforcement, and test modernization — are well-intentioned, but these regressions block a safe release.

Key Findings:

  • In pkg/cmd/flagoptions_test.go, the for _, tt := range tests loop adds t.Parallel() without a tt := tt capture, causing all parallel subtests to close over the same loop variable and execute with only the last test case's data in Go versions prior to 1.22.
  • In internal/requestflag/requestflag_test.go, strFlag, superstitiousIntFlag, and boolFlag are declared once in the outer TestFlagSet scope and then mutated concurrently by multiple t.Parallel() subtests, introducing data races on internal fields like value, hasBeenSet, applied, and count — this was flagged in two prior unresolved review comments and remains unaddressed.
  • In pkg/cmd/memory.go, removing FileInput: true from the file flag definition means the flag no longer signals that it should read and stream file contents; the CLI will instead forward the raw path string to the multipart upload, functionally breaking the memories upload command.
  • The test modernization effort (widespread t.Parallel() adoption) is a positive direction, but the incorrect parallel patterns introduced here undermine test reliability precisely in the areas being modernized.
  • 3 previous unresolved comment(s) likely resolved in latest diff (score-only signal; thread status unchanged)
Files requiring special attention
  • pkg/cmd/memory.go
  • pkg/cmd/flagoptions_test.go
  • internal/requestflag/requestflag_test.go

@@ -114,6 +116,8 @@ func TestEncode(t *testing.T) {

for name, test := range tests {
t.Run(name, func(t *testing.T) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Adding t.Parallel() inside the subtest causes the closure to capture the test loop variable by reference; in Go < 1.22 all parallel subtests will race on the same test value, producing incorrect or flaky results. A local copy (test := test) is needed before t.Parallel() to pin the value per iteration.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In internal/apiquery/query_test.go, around line 118, the subtest closure added `t.Parallel()` but the `test` loop variable is captured by reference. In Go versions before 1.22, all parallel subtests will share the same loop variable, causing data races and incorrect test behavior. Fix by adding `test := test` immediately before `t.Parallel()` inside the closure to create a per-iteration copy of the loop variable.

Comment on lines 348 to +386
// Test initialization and setting
t.Run("PreParse initialization", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.PreParse())
assert.True(t, strFlag.applied)
assert.Equal(t, "default-string", strFlag.Get())
})

t.Run("Set string flag", func(t *testing.T) {
t.Parallel()

assert.NoError(t, strFlag.Set("string-flag", "new-value"))
assert.Equal(t, "new-value", strFlag.Get())
assert.True(t, strFlag.IsSet())
})

t.Run("Set int flag with valid value", func(t *testing.T) {
t.Parallel()

assert.NoError(t, superstitiousIntFlag.Set("int-flag", "100"))
assert.Equal(t, int64(100), superstitiousIntFlag.Get())
assert.True(t, superstitiousIntFlag.IsSet())
})

t.Run("Set int flag with invalid value", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "not-an-int"))
})

t.Run("Set int flag with validator failing", func(t *testing.T) {
t.Parallel()

assert.Error(t, superstitiousIntFlag.Set("int-flag", "13"))
})

t.Run("Set bool flag", func(t *testing.T) {
t.Parallel()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: The subtests PreParse initialization, Set string flag, Set int flag with valid value, Set int flag with invalid value, Set int flag with validator failing, and Set bool flag all share the same strFlag, superstitiousIntFlag, and boolFlag pointers declared in the outer TestFlagSet scope. Adding t.Parallel() to these subtests causes concurrent reads and writes to those shared flag structs (mutating value, hasBeenSet, applied, count), introducing data races that will produce non-deterministic failures or corrupt state under -race.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In `internal/requestflag/requestflag_test.go`, the subtests inside `TestFlagSet` that were made parallel (lines ~348-386) all mutate shared flag instances (`strFlag`, `superstitiousIntFlag`, `boolFlag`) declared in the outer function scope. This creates data races. Fix this by either: (1) removing `t.Parallel()` from all subtests that share these outer-scope flag variables, or (2) constructing a fresh flag instance inside each subtest instead of sharing the outer-scope ones. The 'Set slice flag' subtests are fine since they already create local flags.

Comment thread pkg/cmd/memory.go
Comment on lines 371 to 380
Suggest: true,
Flags: []cli.Flag{
&requestflag.Flag[string]{
Name: "file",
Usage: "The file to ingest.",
Required: true,
BodyPath: "file",
FileInput: true,
Name: "file",
Usage: "The file to ingest.",
Required: true,
BodyPath: "file",
},
&requestflag.Flag[any]{
Name: "collection",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Removing FileInput: true means the file flag will no longer be treated as a file path to open and read — it will be passed as a raw string, causing multipart uploads to send the filename string instead of the actual file contents.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/memory.go around line 371-380, the `FileInput: true` field was removed from the `requestflag.Flag[string]` definition for the 'file' flag in the `memoriesUpload` command. This field is responsible for instructing the flag processing logic to open the file at the given path and read its contents for multipart upload. Without it, the flag value is treated as a plain string (the filename), not the actual file data, breaking file uploads. Please restore `FileInput: true` to this flag definition.

@entelligence-ai-pr-reviews
Copy link
Copy Markdown


Confidence Score: 1/5 - Blocking Issues

Not safe to merge — this PR introduces data races in parallelized test suites and a functional regression in multipart file uploads. In internal/requestflag/requestflag_test.go, multiple subtests running in parallel all mutate shared strFlag, superstitiousIntFlag, and boolFlag instances declared in the outer TestFlagSet scope, producing concurrent read/write races that will cause flaky or incorrect test results — and this concern has been raised in three separate unresolved prior review comments, meaning it has been flagged repeatedly without correction. In internal/apiquery/query_test.go, the loop-variable capture bug (missing test := test before t.Parallel()) will cause all parallel subtests to reference the same final loop value in Go < 1.22. Most critically, the removal of FileInput: true from the file flag in pkg/cmd/memory.go is a behavioral regression that will cause multipart uploads to send the literal filename string rather than the actual file contents, breaking the feature the PR claims to enhance.

Key Findings:

  • In pkg/cmd/memory.go, removing FileInput: true from the file flag definition means the flag is no longer treated as a path to open — multipart uploads will transmit the raw filename string as the body, not the file contents, which is a silent functional regression for all binary file parameter uploads.
  • In internal/requestflag/requestflag_test.go, the parallel subtests Set string flag, Set int flag with valid value, Set int flag with invalid value, etc. all concurrently mutate the same strFlag, superstitiousIntFlag, and boolFlag pointers from the outer scope, introducing data races; this issue has appeared in three consecutive unresolved review comments and remains unfixed.
  • In internal/apiquery/query_test.go, the table-driven subtests call t.Parallel() without a local copy of the test loop variable, so in Go versions before 1.22 every goroutine captures the same final iteration value, rendering the parallel subtests useless for correctness and potentially masking real failures.
  • The combination of a functional file-upload regression (FileInput removal) and multiple confirmed data races in the newly parallelized test suites means neither the production code nor the test harness can be trusted as-is.
  • 4 previous unresolved comment(s) likely resolved in latest diff (score-only signal; thread status unchanged)
Files requiring special attention
  • pkg/cmd/memory.go
  • internal/requestflag/requestflag_test.go
  • internal/apiquery/query_test.go

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 6d009e5 to bfb2be0 Compare April 30, 2026 08:06
Copy link
Copy Markdown

@entelligence-ai-pr-reviews entelligence-ai-pr-reviews Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Releases v0.2.0 of the Hyperspell CLI with new features, type safety improvements, and broad test/code quality refactors.

  • Adds --raw-output/-r flag for stripping JSON quotes from string output (jq-style)
  • Adds stdin support via - for binary file parameters with onceStdinReader gate
  • Adds fileUpload struct propagating filename and MIME-typed content-type for multipart encoding
  • Adds applyDataAliases/rewriteAliases for x-stainless-cli-data-alias parameter aliasing
  • Extends Flag[T] and InnerFlag to support pointer types (*string, *float64, etc.) enabling tri-state (unset/null/value) semantics for nullable fields
  • Refactors all command handlers to use ShowJSONOpts struct replacing positional ShowJSON arguments
  • Adds ValidateBaseURL with startup enforcement and --base-url flag validation
  • Fixes Zsh autocomplete to support both fpath autoload and direct source methods
  • Adds t.Parallel() across all test suites; adds TestNullLiteralHandling and TestInnerFlagDispatchOnUntypedFlag test suites
  • Adds options.recency-half-life-days flag and date body flag to memory commands
  • Hardens shell scripts with ${SKIP_BREW:-} expansion and link script module resolution guard
  • Bumps @stdy/cli mock dependency from 0.20.2 to 0.22.1

Comment thread pkg/cmd/cmdutil.go
Comment on lines 358 to +361
if err := json2yaml.Convert(&yaml, input); err != nil {
return nil, err
}
_, err := expectedOutput.Write([]byte(yaml.String()))
_, err := opts.Stdout.Write([]byte(yaml.String()))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: 🐛 formatJSON for the yaml case writes directly to opts.Stdout and returns nil bytes instead of returning the formatted content — when ShowJSONIterator calls formatJSON to buffer output before deciding whether to use a pager, YAML content is immediately written to real stdout, bypassing pager logic entirely and producing out-of-order or duplicated output.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/cmdutil.go, the `formatJSON` function's `yaml` case (around line 358) writes directly to `opts.Stdout` and returns `(nil, err)` instead of returning the formatted YAML bytes. This breaks `ShowJSONIterator`, which calls `formatJSON` to accumulate bytes before deciding whether to page output — YAML bypasses that buffering and writes straight to stdout. Fix: change `_, err := opts.Stdout.Write([]byte(yaml.String())); return nil, err` to `return []byte(yaml.String()), nil` so the bytes are returned to the caller like every other format case.

Copy link
Copy Markdown

@entelligence-ai-pr-reviews entelligence-ai-pr-reviews Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR delivers the v0.2.0 release of the Hyperspell CLI, introducing major feature additions, type safety improvements, and cross-cutting refactors.

  • Stdin support: ApplyStdinDataToFlags() routes piped YAML/JSON to path, query, and header flags; onceStdinReader prevents duplicate stdin consumption for binary file params
  • File metadata: fileUpload struct enriches file references with basename and MIME-typed contentType for multipart uploads
  • Nullable scalar typing: Flag[T] and InnerFlag type constraints extended to pointer-to-primitive types (*string, *float64, *int64, *bool, etc.) enabling tri-state (unset/null/value) field handling
  • --raw-output flag: New -r flag strips JSON quotes from string results, mirroring jq -r behavior
  • ShowJSONOpts refactor: Replaces positional os.Stdout + individual parameters with a structured options struct across all command handlers; stdout/stderr become injectable for testability
  • ValidateBaseURL: Startup and flag-level validation of HYPERSPELL_BASE_URL with clear protocol scheme error messages
  • Zsh autocompletion fix: Script refactored to support both fpath/autoload (#compdef directive) and direct sourcing with conditional compdef fallback
  • PathParam annotations: PathParam field added to relevant flags across connection, evaluate, folder, integration, memory, and other command files
  • Test parallelization: t.Parallel() added across all test suites in apiform, apiquery, autocomplete, jsonview, requestflag, and cmdutil
  • Script fixes: bootstrap unbound variable fix, link script Go module validation guard, @stdy/cli bumped to 0.22.1 in mock/test scripts

Comment thread pkg/cmd/auth.go
@@ -143,6 +154,8 @@ func handleAuthUserToken(ctx context.Context, cmd *cli.Command) error {
return err
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: AuthUserTokenParams{} is always empty — the user-id (required), expires-in, and origin flag values are never read and assigned into the struct, so the API call is sent without the required user_id field.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/auth.go, the `handleAuthUserToken` function (around line 154) creates an empty `hyperspell.AuthUserTokenParams{}` struct but never populates it with the CLI flag values (`user-id`, `expires-in`, `origin`). Fix this by reading each flag value from `cmd` and assigning them to the appropriate fields of `params` before calling `client.Auth.UserToken`. The `user-id` flag maps to `params.UserID`, `expires-in` maps to `params.ExpiresIn`, and `origin` maps to `params.Origin`.

Comment on lines +144 to +146
// Inner flags (those with an outer flag) are also handled: if the outer flag's body path key exists in the
// data map and contains a nested map with a key matching the inner flag's field (or aliases), the inner
// flag is set from that nested value.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: InnerFlag.IsSet() unconditionally returns false (as seen in innerflag.go), so the flag.IsSet() guard never skips inner flags — any inner flag explicitly set on the command line will be silently overwritten by piped stdin data.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In internal/requestflag/requestflag.go, the ApplyStdinDataToFlags function (starting around line 144) checks `flag.IsSet()` to skip already-set flags before processing inner flags. However, InnerFlag.IsSet() in internal/requestflag/innerflag.go always returns false, so this guard never fires for inner flags. As a result, any inner flag that was explicitly provided on the command line will be overwritten by piped stdin data. Fix this by adding a separate IsSet-like check for inner flags — for example, track whether an InnerFlag has been set via a dedicated field (similar to hasBeenSet on Flag[T]) and expose it through an interface, then check that interface in ApplyStdinDataToFlags before attempting to set the inner flag from stdin.

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from bfb2be0 to fc88702 Compare May 1, 2026 00:31
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from fc88702 to cf22745 Compare May 1, 2026 05:01
Copy link
Copy Markdown

@entelligence-ai-pr-reviews entelligence-ai-pr-reviews Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR delivers the v0.2.0 release of the Hyperspell CLI SDK with major infrastructure improvements and new user-facing features.

  • --raw-output flag: Added to cmd.go; propagated via ShowJSONOpts.RawOutput through all command handlers for jq-style string unquoting
  • Nullable scalar flags: requestflag.go and innerflag.go extended with pointer-typed generics (*string, *bool, *float64, *int64, *DateTimeValue, etc.) supporting null literal parsing and proper JSON serialization
  • Stdin/pipe data aliasing: flagoptions.go introduces onceStdinReader, applyDataAliases, and calls ApplyStdinDataToFlags to map piped YAML/JSON onto unset CLI flags by key/alias matching
  • ShowJSONOpts refactor: cmdutil.go consolidates ShowJSON/ShowJSONIterator parameters into a struct with injectable stdout/stderr, ExplicitFormat, RawOutput, and TTY-aware format fallback; all command files updated accordingly
  • PathParam routing: Added to flag definitions in connection.go, evaluate.go, folder.go, integration.go, and memory.go for URL path parameter binding
  • Base URL validation: ValidateBaseURL added to cmdutil.go and wired into cmd.go flag and main.go startup
  • File upload metadata: fileUpload struct in flagoptions.go wraps readers with filename and MIME contentType for richer multipart encoding
  • Zsh autocompletion fix: zsh_autocomplete.zsh refactored for dual-mode fpath/direct-source loading
  • Parallel tests: t.Parallel() added across apiform, apiquery, autocomplete, jsonview, requestflag, and flagoptions test files
  • Dependency bump: @stdy/cli updated from 0.20.2 to 0.22.1 in scripts/mock and scripts/test
  • Version bump: Manifest, version.go, and CHANGELOG.md updated to 0.2.0

Comment thread pkg/cmd/action.go
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from cf22745 to 9eee0f2 Compare May 5, 2026 19:32
Copy link
Copy Markdown

@entelligence-ai-pr-reviews entelligence-ai-pr-reviews Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR delivers the v0.2.0 release of the Hyperspell CLI with significant feature additions, type safety improvements, and broad refactoring.

  • Stdin support: Adds - as a stdin alias in file/data flags; onceStdinReader prevents double-consumption; piped YAML/JSON keys are dispatched to matching CLI flags via ApplyStdinDataToFlags
  • --raw-output/-r flag: Strips JSON quotes from string results for shell-friendly output
  • ShowJSONOpts refactor: Replaces positional arguments across all ShowJSON/ShowJSONIterator call sites with a structured options struct adding ExplicitFormat and RawOutput support
  • Nullable scalar typing: Introduces pointer-typed Flag[*string], Flag[*float64], etc. for tri-state (unset/null/value) nullable OpenAPI schema support
  • CLI data aliasing: DataAliases field on flags enables YAML key normalization to canonical API names
  • File upload metadata: fileUpload struct wraps readers with filename and MIME content-type derived from extension
  • PathParam annotations: Path-parameter flags across all command files now declare PathParam bindings instead of inline param construction
  • Base URL validation: ValidateBaseURL enforces URL scheme at startup and via --base-url flag
  • Zsh autocompletion fix: Replaces hardcoded shebang with #compdef directive and adds dual-method dispatch
  • Test parallelism: t.Parallel() added across all test files; new test suites for ValidateBaseURL, null literal handling, inner flag dispatch, and ApplyStdinDataToFlags
  • Tooling: workflow_dispatch added to release workflow; bootstrap script fixed for set -u; scripts/link gains module validation guard; @stdy/cli bumped to 0.22.1

Comment thread pkg/cmd/folder.go
Comment on lines 264 to 265
return fmt.Errorf("Unexpected extra arguments: %v", unusedArgs)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Removing params := hyperspell.FolderSetPoliciesParams{} while the complete file still passes params to client.Folders.SetPolicies(...) causes an undefined variable compile error.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In pkg/cmd/folder.go, the diff at lines 264-265 removes the `params := hyperspell.FolderSetPoliciesParams{}` declaration from `handleFoldersSetPolicies`, but `params` is still passed as an argument to `client.Folders.SetPolicies(...)` later in the same function. Either restore the `params` declaration, or also remove (or replace) the `params` argument in the `client.Folders.SetPolicies` call to match the intended change.

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 9eee0f2 to 3f4a0ea Compare May 6, 2026 19:31
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 3f4a0ea to 8845376 Compare May 6, 2026 20:31
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 8845376 to f1ec9cb Compare May 7, 2026 19:32
Comment on lines +148 to +150
for _, flag := range cmd.Flags {
if flag.IsSet() {
continue
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Major: ApplyStdinDataToFlags overwrites CLI-set inner flags because InnerFlag.IsSet() always returns falseInnerFlag.IsSet() unconditionally returns false (innerflag.go:108–109), so the flag.IsSet() guard never skips inner flags regardless of whether the user provided them on the command line. If a user passes --parent.field value via CLI and stdin YAML also contains a matching nested object under outer.GetBodyPath(), ApplyStdinDataToFlags will call flag.Set() on the inner flag, invoking SetInnerField which overwrites the outer map entry with the stdin value.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In file `internal/requestflag/requestflag.go`, in the `ApplyStdinDataToFlags` function (around line 154), after the check `if inner, ok := flag.(HasOuterFlag); ok {`, add a guard to skip inner flags whose outer flag has already been set via the command line. Specifically, after confirming `inner, ok := flag.(HasOuterFlag)` and before looking up values in the data map, check whether the outer flag has already been set for this inner flag's field. One approach: after resolving `outer` at line 155, check `if outer, ok2 := inner.GetOuterFlag().(interface{ IsSet() bool }); ok2 && outer.IsSet() { continue }`. This prevents stdin data from overwriting values the user explicitly provided on the CLI for inner flags, which currently have no protection because `InnerFlag.IsSet()` always returns `false`.

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from f1ec9cb to 894f210 Compare May 8, 2026 03:31
Comment thread internal/requestflag/requestflag.go
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 894f210 to 57ea61c Compare May 8, 2026 05:12
@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 57ea61c to 26ccd3a Compare May 8, 2026 17:31
Comment thread cmd/hyperspell/main.go
Comment on lines +46 to +51
show_err := cmd.ShowJSON(json, cmd.ShowJSONOpts{
ExplicitFormat: app.IsSet("format-error"),
Format: format,
Title: "Error",
Transform: app.String("transform-error"),
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Major: Error handler in main.go omits RawOutput, silently ignoring --raw-output for API errors — pkg/cmd/auth.go (line 87), session.go (line 96), and every other command handler passes RawOutput: cmd.Root().Bool("raw-output") in their ShowJSONOpts. The error-display path in main.go (lines 46–51) constructs ShowJSONOpts without RawOutput, so it always defaults to false. A user pairing --raw-output with --transform-error to extract a plain string from an API error JSON body will get JSON-quoted output instead of the raw string, contradicting the behaviour they see on the success path.

Comment on lines +27 to +28
"cookie",
"set-cookie",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Major: Remove ineffective set-cookie entry from sensitiveHeaderssensitiveHeaders is consumed exclusively in redactRequest() (line 93), which operates on request headers. set-cookie is a response header; the response is dumped at line 61 via httputil.DumpResponse with no redaction pass, so this entry is a no-op and Set-Cookie values (session tokens, auth cookies) are logged in plaintext when debug mode is active.

🤖 AI Agent Prompt for Cursor/Windsurf

📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue

In `internal/debugmiddleware/debug_middleware.go`, the entry `"set-cookie"` at line 27–28 inside `sensitiveHeaders` has no effect. The `sensitiveHeaders` slice is only used in `redactRequest()` (line 93), which processes request headers, but `set-cookie` is a response header. The response is logged at line 61 via `httputil.DumpResponse(resp, true)` with no redaction. To actually redact `Set-Cookie` from responses, add a `redactResponse(*http.Response) (*http.Response, error)` method mirroring `redactRequest`, iterate `m.sensitiveHeaders` against `resp.Header`, and call it before `httputil.DumpResponse` in `Middleware()`. Until then, remove `"set-cookie"` from `sensitiveHeaders` to avoid creating a false sense of security.

@stainless-app stainless-app Bot force-pushed the release-please--branches--main--changes--next branch from 26ccd3a to 0b95788 Compare May 8, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants