Fix: [for cherry-picking] Login now creates a valid default project, ra - #19
Conversation
|
PR author is in the excluded authors list. |
ApprovabilityVerdict: Needs human review Unable to check for correctness in bff3199. This PR modifies auth flow behavior (auto-creating projects on login), adds API URL validation with HTTPS enforcement, and changes SSE error handling. These runtime behavior changes in security-sensitive areas, combined with the author not owning any of the affected files (all owned by wave-av/core-team), warrant review by the designated code owners. You can customize Macroscope's approvability policy. Learn more. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (done) { | ||
| throw new Error("SSE connection closed"); | ||
| } |
There was a problem hiding this comment.
🔴 Streaming commands never stop and repeatedly show a connection error when the server ends the stream normally
A normally finished event stream is now turned into a failure (throw new Error("SSE connection closed") at src/lib/sse-client.ts:96-98) instead of a clean finish, so users see repeated error messages and the command keeps reconnecting without ever ending.
Impact: Log/listen/dev streaming commands print spurious errors and hang indefinitely instead of finishing when the server closes the stream.
Why the retry limit never stops the loop
On every successful connection reconnectAttempts is reset to 0 (src/lib/sse-client.ts:84). A server that accepts the connection and then closes it (e.g. periodic SSE timeouts – the log command explicitly handles a timeout event, src/commands/logs/index.ts:60) will therefore always reconnect: connect → done → throw → onError (prints "Connection error: SSE connection closed") → backoff of initialDelay (counter was reset so the exponent is always 1) → reconnect, forever. onClose is never invoked, so consumers never learn the stream ended, and each retry is a nested return connect() call from the catch block, growing the promise chain.
A cleaner approach is to distinguish an intentional server-side end (call onClose) from an unexpected drop, and to not reset the attempt counter unless the connection actually delivered data / stayed up for a meaningful period.
Prompt for agents
In src/lib/sse-client.ts, the read loop now throws "SSE connection closed" when the reader reports done. Because reconnectAttempts is reset to 0 on each successful connection (line 84), a server that closes the stream right after connecting causes an endless reconnect loop with a constant 1s delay, an error message on every cycle, and onClose is never called. Consider tracking whether the connection was up long enough / received events before resetting the attempt counter, and treat a graceful server close distinctly (e.g. reconnect quietly without emitting onError, and still honour maxReconnectAttempts so the loop terminates and onClose fires).
Was this helpful? React with 👍 or 👎 to provide feedback.
| const base = new URL(baseUrl); | ||
| const requested = new URL(path, `${base.origin}/`); | ||
| if (requested.protocol !== "https:" || requested.origin !== base.origin) { | ||
| throw new Error("API requests must use the configured WAVE HTTPS host"); | ||
| } | ||
| const url = requested.toString(); |
There was a problem hiding this comment.
🟡 API requests lose any path prefix configured for the WAVE host
Request addresses are now rebuilt from only the host part of the configured address (new URL(path, ${base.origin}/) at src/commands/api/index.ts:28) instead of the full configured address, so any folder path in that setting is silently dropped and requests go to the wrong location.
Impact: Users whose configured WAVE address includes a path prefix will have their raw API calls sent to a wrong URL and fail.
Mechanism and secondary effect
Previously the URL was ${baseUrl}${path} (old src/commands/api/index.ts:27-29), preserving any path in baseUrl (the schema allows any URL, src/lib/config/schema.ts:6). Now only base.origin is used, so with baseUrl = https://wave.online/gateway, wave api GET /v1/streams hits https://wave.online/v1/streams. Other commands still concatenate onto the full base URL (e.g. src/commands/auth/index.ts:99), so behaviour is inconsistent.
Additionally, the hard requested.protocol !== "https:" check makes the api command unusable against a non-HTTPS base URL (e.g. a local dev host set via WAVE_BASE_URL), even though login accepts such a value (src/commands/auth/index.ts:27).
Prompt for agents
In src/commands/api/index.ts the request URL is resolved against base.origin, discarding any path component of the configured baseUrl, unlike other commands which append to the full baseUrl. Resolve the path against the full base URL (ensuring a trailing slash for relative resolution, or concatenating for absolute paths) while still validating that the final origin matches the configured origin. Also consider whether the strict https-only requirement should allow the configured base URL's own scheme (e.g. http://localhost for development).
Was this helpful? React with 👍 or 👎 to provide feedback.
| "@types/inquirer": "^9.0.7", | ||
| "@types/node": "^22.13.0", | ||
| "@types/ws": "^8.5.14", | ||
| "eslint": "^9.17.0", |
There was a problem hiding this comment.
🟡 Changelog was not updated for these user-facing fixes
The repository contract requires the Unreleased section of CHANGELOG.md to be updated for user-facing changes, but this PR changes login behaviour, API request handling and streaming reconnects without adding any entry.
Impact: Users and maintainers get no record of these behaviour changes in the changelog.
Rule reference
AGENTS.md states: "Conventional Commit titles; update CHANGELOG.md (Unreleased) for user-facing changes." The Unreleased section in CHANGELOG.md:7 is still empty.
Was this helpful? React with 👍 or 👎 to provide feedback.
| while (!controller.signal.aborted) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| if (done) { | ||
| throw new Error("SSE connection closed"); | ||
| } |
There was a problem hiding this comment.
🔍 SSE reader is never released on error/reconnect
When the loop now throws on stream end (or on any parsing error), the ReadableStreamDefaultReader obtained at src/lib/sse-client.ts:87 is never cancelled or released before connect() recurses. With the new reconnect-on-clean-close behaviour this happens on every cycle, so readers/response bodies accumulate for long-lived sessions. Adding a try/finally around the read loop with reader.cancel()/releaseLock() would bound this.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixed Findings
Automated fix from agentic review of #18
Note
Fix login to create a valid default project entry and update related auth/API handling
auth logincommand now initializes aprojectsconfig entry (includingbaseUrl,organizationId,organizationName) and setscurrentProjectwhen authenticating via--api-keyor device flow.apicommand now validates that requests use HTTPS and match the configured base URL's origin, throwing an error on mismatch instead of silently proceeding.@wave/sdkto@wave-av/sdkat^2.0.0.initnow correctly differentiates betweendist(built) and source layouts.connectSSEnow throws an error when the SSE stream closes rather than silently exiting the read loop.Macroscope summarized bff3199.