Address PR review feedback

oauthcallback:
- Validate auth code is non-empty before accepting callback
- Accept pre-bound net.Listener for ephemeral port tests
- Non-blocking channel sends to handle duplicate requests
- Tests use 127.0.0.1:0 instead of fixed ports

output:
- AsError handles nil error without panicking
- writeCount returns 0 for nil data, propagates write errors

profile: Sort names before calling Picker for deterministic order

surface: Sort Diff results for stable CI output

credstore:
- Random probe key to avoid collisions with real credentials
- FallbackWarning exported instead of writing to stderr

actions:
- rubric-check: Verify exit code is exactly 1, not just non-zero
- surface-compat: Sort baseline before comm for locale safety
- sync-skills: Preserve subdirectory structure during copy

Author: jeremy

actions/rubric-check/action.yml

@@ -64,10 +64,11 @@ runs:
 
         # 1B.1: Exit codes - bad usage should exit 1
         "$BINARY" --nonexistent-flag >/dev/null 2>&1
-        if [ $? -ne 0 ]; then
-          check "1B.1" "Non-zero exit on bad usage" "pass"
+        EXIT_CODE=$?
+        if [ "$EXIT_CODE" -eq 1 ]; then
+          check "1B.1" "Exit code 1 on bad usage" "pass"
         else
-          check "1B.1" "Non-zero exit on bad usage" "fail"
+          check "1B.1" "Exit code 1 on bad usage (got $EXIT_CODE)" "fail"
         fi
 
         if [ "$PROFILE" = "api-cli" ]; then

actions/surface-compat/action.yml

@@ -73,6 +73,10 @@ runs:
         BASELINE="${{ inputs.baseline }}"
         CURRENT="/tmp/surface-current.txt"
 
+        # comm requires sorted input — sort both with consistent locale
+        LC_ALL=C sort "$BASELINE" -o "$BASELINE"
+        LC_ALL=C sort "$CURRENT" -o "$CURRENT"
+
         REMOVED=$(comm -23 "$BASELINE" "$CURRENT" | wc -l | tr -d ' ')
         ADDED=$(comm -13 "$BASELINE" "$CURRENT" | wc -l | tr -d ' ')
 

actions/sync-skills/action.yml

@@ -67,7 +67,11 @@ runs:
           dest="${TARGET_DIR}/${CLI_NAME}/${skill_name}"
           echo "  Syncing ${skill_name}..."
           mkdir -p "$dest"
-          find "$skill_dir" -type f ! -name '*.go' ! -name '.*' -exec cp {} "$dest/" \;
+          # Preserve subdirectory structure; exclude Go sources and dotfiles
+          (cd "$skill_dir" && find . -type f ! -name '*.go' ! -name '.*' | while read -r f; do
+            mkdir -p "$dest/$(dirname "$f")"
+            cp "$f" "$dest/$f"
+          done)
           MANAGED_SKILLS+=("${CLI_NAME}/${skill_name}")
         done
 

credstore/store.go

@@ -1,6 +1,8 @@
 package credstore
 
 import (
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -28,26 +30,38 @@ type Store struct {
 	fallbackDir string
 }
 
+// FallbackWarning is set when NewStore falls back to file storage.
+// The caller can check this and display the warning as appropriate.
+var FallbackWarning string
+
 // NewStore creates a credential store. It probes the system keyring
 // and falls back to file storage if unavailable.
 func NewStore(opts StoreOptions) *Store {
+	FallbackWarning = ""
+
 	if opts.DisableEnvVar != "" && os.Getenv(opts.DisableEnvVar) != "" {
 		return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir}
 	}
 
-	// Test if keyring is available
-	testKey := opts.ServiceName + "::test"
-	err := keyring.Set(opts.ServiceName, testKey, "test")
+	// Probe keyring with a random key to avoid collisions.
+	probeKey := probeKeyName()
+	err := keyring.Set(opts.ServiceName, probeKey, "probe")
 	if err == nil {
-		_ = keyring.Delete(opts.ServiceName, testKey)
+		_ = keyring.Delete(opts.ServiceName, probeKey)
 		return &Store{serviceName: opts.ServiceName, useKeyring: true, fallbackDir: opts.FallbackDir}
 	}
 
-	fmt.Fprintf(os.Stderr, "warning: system keyring unavailable, credentials stored in plaintext at %s\n",
+	FallbackWarning = fmt.Sprintf("system keyring unavailable, credentials stored in plaintext at %s",
 		filepath.Join(opts.FallbackDir, "credentials.json"))
 	return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir}
 }
 
+func probeKeyName() string {
+	b := make([]byte, 8)
+	_, _ = rand.Read(b)
+	return "__probe_" + hex.EncodeToString(b)
+}
+
 func (s *Store) key(name string) string {
 	return fmt.Sprintf("%s::%s", s.serviceName, name)
 }

oauthcallback/server.go

@@ -9,19 +9,39 @@ import (
 	"time"
 )
 
-// WaitForCallback starts a local HTTP server and waits for an OAuth callback.
-// It returns the authorization code from the callback.
-func WaitForCallback(ctx context.Context, expectedState, authURL, listenAddr string) (string, error) {
-	lc := net.ListenConfig{}
-	listener, err := lc.Listen(ctx, "tcp", listenAddr)
-	if err != nil {
-		return "", fmt.Errorf("failed to start callback server: %w", err)
+// WaitForCallback starts a local HTTP server on listener and waits for an
+// OAuth callback. It returns the authorization code from the callback.
+//
+// If listener is nil, one is created on listenAddr. Passing a pre-bound
+// listener (e.g., from net.Listen("tcp", "127.0.0.1:0")) is preferred
+// for tests to avoid port conflicts.
+func WaitForCallback(ctx context.Context, expectedState string, listener net.Listener, listenAddr string) (string, error) {
+	if listener == nil {
+		lc := net.ListenConfig{}
+		var err error
+		listener, err = lc.Listen(ctx, "tcp", listenAddr)
+		if err != nil {
+			return "", fmt.Errorf("failed to start callback server: %w", err)
+		}
 	}
 	defer listener.Close()
 
 	codeCh := make(chan string, 1)
 	errCh := make(chan error, 1)
-	var shutdownOnce sync.Once
+	var once sync.Once
+
+	send := func(ch chan<- string, val string) {
+		select {
+		case ch <- val:
+		default:
+		}
+	}
+	sendErr := func(ch chan<- error, val error) {
+		select {
+		case ch <- val:
+		default:
+		}
+	}
 
 	server := &http.Server{
 		ReadHeaderTimeout: 10 * time.Second,
@@ -30,37 +50,42 @@ func WaitForCallback(ctx context.Context, expectedState, authURL, listenAddr str
 		IdleTimeout:       30 * time.Second,
 	}
 
+	shutdown := func() {
+		once.Do(func() {
+			shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			go func() { defer cancel(); server.Shutdown(shutdownCtx) }()
+		})
+	}
+
 	server.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		state := r.URL.Query().Get("state")
 		code := r.URL.Query().Get("code")
 		errParam := r.URL.Query().Get("error")
 
 		if errParam != "" {
-			errCh <- fmt.Errorf("OAuth error: %s", errParam)
+			sendErr(errCh, fmt.Errorf("OAuth error: %s", errParam))
 			fmt.Fprint(w, "<html><body><h1>Authentication failed</h1><p>You can close this window.</p></body></html>")
-			shutdownOnce.Do(func() {
-				shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-				go func() { defer cancel(); server.Shutdown(shutdownCtx) }()
-			})
+			shutdown()
 			return
 		}
 
 		if state != expectedState {
-			errCh <- fmt.Errorf("state mismatch: CSRF protection failed")
+			sendErr(errCh, fmt.Errorf("state mismatch: CSRF protection failed"))
 			fmt.Fprint(w, "<html><body><h1>Authentication failed</h1><p>State mismatch.</p></body></html>")
-			shutdownOnce.Do(func() {
-				shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-				go func() { defer cancel(); server.Shutdown(shutdownCtx) }()
-			})
+			shutdown()
+			return
+		}
+
+		if code == "" {
+			sendErr(errCh, fmt.Errorf("OAuth callback missing authorization code"))
+			fmt.Fprint(w, "<html><body><h1>Authentication failed</h1><p>Missing authorization code.</p></body></html>")
+			shutdown()
 			return
 		}
 
-		codeCh <- code
+		send(codeCh, code)
 		fmt.Fprint(w, "<html><body><h1>Authentication successful!</h1><p>You can close this window.</p></body></html>")
-		shutdownOnce.Do(func() {
-			shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-			go func() { defer cancel(); server.Shutdown(shutdownCtx) }()
-		})
+		shutdown()
 	})
 
 	go server.Serve(listener)
@@ -73,6 +98,6 @@ func WaitForCallback(ctx context.Context, expectedState, authURL, listenAddr str
 	case <-ctx.Done():
 		return "", ctx.Err()
 	case <-time.After(5 * time.Minute):
-		return "", fmt.Errorf("authentication timeout waiting for callback on %s", listenAddr)
+		return "", fmt.Errorf("authentication timeout waiting for callback on %s", listener.Addr())
 	}
 }

oauthcallback/server_test.go

@@ -2,6 +2,8 @@ package oauthcallback
 
 import (
 	"context"
+	"fmt"
+	"net"
 	"net/http"
 	"testing"
 	"time"
@@ -10,30 +12,37 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func listen(t *testing.T) net.Listener {
+	t.Helper()
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	t.Cleanup(func() { ln.Close() })
+	return ln
+}
+
 func TestWaitForCallback_Success(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
+	ln := listen(t)
+	addr := ln.Addr().String()
 	state := "test-state-123"
-	listenAddr := "127.0.0.1:18976"
 
 	codeCh := make(chan string, 1)
 	errCh := make(chan error, 1)
 
 	go func() {
-		code, err := WaitForCallback(ctx, state, "", listenAddr)
+		code, err := WaitForCallback(ctx, state, ln, "")
 		if err != nil {
 			errCh <- err
 		} else {
 			codeCh <- code
 		}
 	}()
 
-	// Give server time to start
 	time.Sleep(100 * time.Millisecond)
 
-	// Simulate callback
-	resp, err := http.Get("http://127.0.0.1:18976/callback?state=test-state-123&code=auth-code-456")
+	resp, err := http.Get(fmt.Sprintf("http://%s/callback?state=test-state-123&code=auth-code-456", addr))
 	require.NoError(t, err)
 	resp.Body.Close()
 
@@ -47,21 +56,49 @@ func TestWaitForCallback_Success(t *testing.T) {
 	}
 }
 
+func TestWaitForCallback_MissingCode(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	ln := listen(t)
+	addr := ln.Addr().String()
+	errCh := make(chan error, 1)
+
+	go func() {
+		_, err := WaitForCallback(ctx, "state", ln, "")
+		errCh <- err
+	}()
+
+	time.Sleep(100 * time.Millisecond)
+
+	resp, err := http.Get(fmt.Sprintf("http://%s/callback?state=state", addr))
+	require.NoError(t, err)
+	resp.Body.Close()
+
+	select {
+	case err := <-errCh:
+		assert.Contains(t, err.Error(), "missing authorization code")
+	case <-time.After(3 * time.Second):
+		t.Fatal("timeout")
+	}
+}
+
 func TestWaitForCallback_StateMismatch(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	listenAddr := "127.0.0.1:18977"
+	ln := listen(t)
+	addr := ln.Addr().String()
 	errCh := make(chan error, 1)
 
 	go func() {
-		_, err := WaitForCallback(ctx, "expected-state", "", listenAddr)
+		_, err := WaitForCallback(ctx, "expected-state", ln, "")
 		errCh <- err
 	}()
 
 	time.Sleep(100 * time.Millisecond)
 
-	resp, err := http.Get("http://127.0.0.1:18977/callback?state=wrong-state&code=abc")
+	resp, err := http.Get(fmt.Sprintf("http://%s/callback?state=wrong-state&code=abc", addr))
 	require.NoError(t, err)
 	resp.Body.Close()
 
@@ -77,17 +114,18 @@ func TestWaitForCallback_OAuthError(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	listenAddr := "127.0.0.1:18978"
+	ln := listen(t)
+	addr := ln.Addr().String()
 	errCh := make(chan error, 1)
 
 	go func() {
-		_, err := WaitForCallback(ctx, "state", "", listenAddr)
+		_, err := WaitForCallback(ctx, "state", ln, "")
 		errCh <- err
 	}()
 
 	time.Sleep(100 * time.Millisecond)
 
-	resp, err := http.Get("http://127.0.0.1:18978/callback?error=access_denied")
+	resp, err := http.Get(fmt.Sprintf("http://%s/callback?error=access_denied", addr))
 	require.NoError(t, err)
 	resp.Body.Close()
 
@@ -102,11 +140,11 @@ func TestWaitForCallback_OAuthError(t *testing.T) {
 func TestWaitForCallback_ContextCancellation(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 
-	listenAddr := "127.0.0.1:18979"
+	ln := listen(t)
 	errCh := make(chan error, 1)
 
 	go func() {
-		_, err := WaitForCallback(ctx, "state", "", listenAddr)
+		_, err := WaitForCallback(ctx, "state", ln, "")
 		errCh <- err
 	}()
 

output/envelope.go

@@ -280,12 +280,17 @@ func (w *Writer) writeCount(v any) error {
 	data := NormalizeData(resp.Data)
 
 	switch d := data.(type) {
+	case nil:
+		_, err := fmt.Fprintln(w.opts.Writer, 0)
+		return err
 	case []any:
-		fmt.Fprintln(w.opts.Writer, len(d))
+		_, err := fmt.Fprintln(w.opts.Writer, len(d))
+		return err
 	case []map[string]any:
-		fmt.Fprintln(w.opts.Writer, len(d))
+		_, err := fmt.Fprintln(w.opts.Writer, len(d))
+		return err
 	default:
-		fmt.Fprintln(w.opts.Writer, 1)
+		_, err := fmt.Fprintln(w.opts.Writer, 1)
+		return err
 	}
-	return nil
 }

output/errors.go

@@ -127,6 +127,9 @@ func ErrAmbiguous(resource string, matches []string) *Error {
 
 // AsError attempts to convert an error to an *Error.
 func AsError(err error) *Error {
+	if err == nil {
+		return &Error{Code: CodeAPI, Message: "unknown error"}
+	}
 	var e *Error
 	if errors.As(err, &e) {
 		return e

profile/resolve.go

@@ -1,6 +1,9 @@
 package profile
 
-import "fmt"
+import (
+	"fmt"
+	"sort"
+)
 
 // ResolveOptions controls how profile resolution behaves.
 type ResolveOptions struct {
@@ -78,6 +81,7 @@ func Resolve(opts ResolveOptions) (string, error) {
 		for name := range opts.Profiles {
 			names = append(names, name)
 		}
+		sort.Strings(names)
 		return opts.Picker(names)
 	}