fix: address PR review comments

- src/main.ts: Use a length-aware fallback (deployment.branch?.length
  ? deployment.branch : inputs.branch ?? "") so the step summary still
  falls back when the API returns an empty string, while documenting
  why we treat empty-string as equivalent to missing here.
- src/secrets.ts: Stop calling `.trim()` on secret values. Strip only
  the leading whitespace separator after `:` and a matching pair of
  outer quotes; trailing whitespace and inner whitespace are preserved
  verbatim. Also pass the un-trimmed raw input to parseBlockMap so a
  trailing space on the last line isn't lost. Whitespace-significant
  values still need quoting to make the boundaries explicit.
- src/api.ts: Move the timing log to AFTER the response body is read
  so the duration reflects true end-to-end request latency, not just
  time-to-first-byte. Body-read errors retry on the next attempt and
  fail with a clear `Failed to read response body for …` message on
  exhaustion.

New tests cover trailing whitespace preserved in unquoted secret
values, leading whitespace preserved inside quoted secret values, and
multiple-spaces-after-colon handling.

Author: lukekim

__tests__/secrets.test.ts

@@ -36,6 +36,24 @@ describe("parseSecrets", () => {
       ]);
     });
 
+    it("preserves trailing whitespace in unquoted values (don't auto-trim secrets)", () => {
+      // The leading separator after `:` is stripped, but trailing whitespace
+      // is preserved verbatim. Whitespace-significant secret values should
+      // be quoted to make the boundaries explicit.
+      const result = parseSecrets("KEY: trailing-space ");
+      expect(result).toEqual([{ name: "KEY", value: "trailing-space " }]);
+    });
+
+    it("preserves leading whitespace inside quoted values", () => {
+      expect(parseSecrets('KEY: "  inner-leading"')).toEqual([
+        { name: "KEY", value: "  inner-leading" },
+      ]);
+    });
+
+    it("strips multiple spaces between ':' and a quoted value", () => {
+      expect(parseSecrets('KEY:    "value"')).toEqual([{ name: "KEY", value: "value" }]);
+    });
+
     it("ignores blank lines and #-comment lines", () => {
       expect(parseSecrets("\n# header\nFOO: bar\n  # indented\nBAZ: qux")).toEqual([
         { name: "FOO", value: "bar" },

dist/index.js

@@ -120,24 +120,44 @@ export class SpiceApiClient {
         );
       }
 
+      // Read the body before logging so the timing covers true end-to-end
+      // request latency (request send → response headers → full body received),
+      // not just time-to-first-byte. 204 No Content has no body to read.
+      let bodyText = "";
+      let bodyError: Error | undefined;
+      if (res.status !== 204) {
+        try {
+          bodyText = await res.text();
+        } catch (err) {
+          bodyError = err as Error;
+        }
+      }
       const durationMs = Date.now() - startMs;
       core.info(`${method} ${path} → ${res.status} ${res.statusText} (${durationMs}ms)`);
 
       if (res.status === 204) {
         return undefined as T;
       }
 
+      if (bodyError) {
+        if (attempt < this.maxAttempts) {
+          await this.sleep(this.backoff(attempt));
+          continue;
+        }
+        throw new SpiceApiError(
+          `Failed to read response body for ${method} ${path}: ${bodyError.message}`,
+          res.status,
+          url,
+        );
+      }
+
       if (res.ok) {
-        if (res.status === 202 || res.status === 201 || res.status === 200) {
-          const text = await res.text();
-          if (!text) return undefined as T;
-          try {
-            return JSON.parse(text) as T;
-          } catch {
-            return text as unknown as T;
-          }
+        if (!bodyText) return undefined as T;
+        try {
+          return JSON.parse(bodyText) as T;
+        } catch {
+          return bodyText as unknown as T;
         }
-        return (await res.json()) as T;
       }
 
       if (RETRYABLE_STATUSES.has(res.status) && attempt < this.maxAttempts) {
@@ -149,7 +169,7 @@ export class SpiceApiClient {
         continue;
       }
 
-      const errorBody = await readErrorBody(res);
+      const errorBody = parseErrorBody(bodyText);
       throw new SpiceApiError(
         formatApiError(method, path, res, errorBody),
         res.status,
@@ -176,8 +196,7 @@ export class SpiceApiClient {
   }
 }
 
-async function readErrorBody(res: Response): Promise<ApiErrorBody | string | undefined> {
-  const text = await res.text().catch(() => "");
+function parseErrorBody(text: string): ApiErrorBody | string | undefined {
   if (!text) return undefined;
   try {
     return JSON.parse(text) as ApiErrorBody;

dist/index.js.map

@@ -124,12 +124,17 @@ async function writeSummary(args: {
         ? "**failed**"
         : `_${deployment.status}_`;
 
-  // The list-deployments endpoint sometimes returns Deployment objects without
-  // the `branch` field (and occasionally `commit_sha`), even when the create
-  // request explicitly set them. Fall back to the inputs we sent so the step
-  // summary stays accurate.
-  const branch = deployment.branch || inputs.branch || "";
-  const commitSha = deployment.commit_sha || inputs.commitSha || "";
+  // The list-deployments endpoint sometimes returns Deployment objects with
+  // the `branch` field missing (or, in observed runs, populated as an empty
+  // string) even when the create request explicitly set it. Fall back to the
+  // inputs we sent in either case so the step summary stays accurate. We
+  // intentionally treat an empty string from the API as equivalent to
+  // missing — an explicit empty branch carries no information for the
+  // summary, and the user-reported symptom was an empty Branch cell.
+  const branch = deployment.branch?.length ? deployment.branch : (inputs.branch ?? "");
+  const commitSha = deployment.commit_sha?.length
+    ? deployment.commit_sha
+    : (inputs.commitSha ?? "");
 
   const summary = core.summary.addHeading("Spice Cloud Deploy", 2).addTable([
     [

src/api.ts

@@ -30,7 +30,10 @@ export function parseSecrets(raw: string | undefined): ParsedSecret[] {
   const trimmed = raw.trim();
   if (!trimmed) return [];
 
-  const entries = trimmed.startsWith("{") ? parseJsonObject(trimmed) : parseBlockMap(trimmed);
+  // For the JSON-form check we use the trimmed view, but the block-map parser
+  // needs the original `raw` so trailing whitespace on the last line (which
+  // could be part of a secret's value) is preserved.
+  const entries = trimmed.startsWith("{") ? parseJsonObject(trimmed) : parseBlockMap(raw);
   for (const { value } of entries) {
     if (value !== "") core.setSecret(value);
   }
@@ -83,7 +86,7 @@ function parseBlockMap(raw: string): ParsedSecret[] {
     }
 
     const name = line.slice(0, colon).trim();
-    const value = stripQuotes(line.slice(colon + 1).trim());
+    const value = parseValue(line.slice(colon + 1));
 
     validateName(name, `secrets line ${i + 1}`);
     if (seen.has(name)) {
@@ -95,14 +98,25 @@ function parseBlockMap(raw: string): ParsedSecret[] {
   return out;
 }
 
-function stripQuotes(value: string): string {
-  if (value.length < 2) return value;
-  const first = value.charAt(0);
-  const last = value.charAt(value.length - 1);
-  if ((first === '"' && last === '"') || (first === "'" && last === "'")) {
-    return value.slice(1, -1);
+/**
+ * Extract a secret value from the substring after the first `:` separator.
+ *
+ * Strips leading whitespace (the canonical YAML `KEY: VALUE` separator) and
+ * a matching pair of outer quotes if present. Trailing whitespace and inner
+ * whitespace are preserved verbatim, so a secret value can contain any
+ * characters; users that need leading or trailing whitespace must wrap the
+ * value in matching single or double quotes.
+ */
+function parseValue(rest: string): string {
+  const ltrimmed = rest.replace(/^\s+/, "");
+  if (ltrimmed.length >= 2) {
+    const first = ltrimmed.charAt(0);
+    const last = ltrimmed.charAt(ltrimmed.length - 1);
+    if ((first === '"' && last === '"') || (first === "'" && last === "'")) {
+      return ltrimmed.slice(1, -1);
+    }
   }
-  return value;
+  return ltrimmed;
 }
 
 function validateName(name: string, context: string): void {