fix: 2026.06 polish batch (#137, #138, #139, #140, #143) (#241)

* fix: 2026.06 polish batch (#137, #138, #139, #140, #143)

- #137: document the always-re-sign dispatch contract in
  ActionHandler.DispatchAction and drop the dead lint stubs.
- #138: change indexer health-addr default from :8082 to :8090 so it
  no longer collides with control's InternalListenAddr default on
  single-host dev setups.
- #139: distinguish jwt.ErrTokenExpired from other ValidateToken
  failures so the web client can pick refresh-and-retry vs
  forced-relogin instead of treating every failure as expired.
- #140: ToSnakeCase now keeps acronyms intact via SDK update; bumps
  SDK pin to 16f6ede929e7 and updates the validator-error assertion
  from "i_d must be a valid ULID" to "id must be a valid ULID".
- #143: errors_parity_test strips // and /* */ comments before
  scanning sdk/ts/errors.ts so a commented-out export no longer
  counts as live and skews the parity check.

* test(auth): align interceptor tests with #139 expired-vs-invalid split

The "invalid or expired token" wording was a single message for both
JWT failure modes — the #139 split swapped it for distinct
"token expired" / "invalid token" messages so the web client can pick
refresh-and-retry vs forced-relogin instead of treating every 401 as
expired. Update the existing tests to:

- TestAuthInterceptor_InvalidToken: assert "invalid token" + assert
  the error does NOT contain "expired" (locks the disjoint behavior).
- TestAuthInterceptor_ExpiredToken: assert "token expired" so the
  refresh-and-retry path stays covered.
- TestAuthInterceptor_WrongSecret: assert "invalid token" + assert
  not "expired" — signature-invalid is a hard reject, not refreshable.

CI caught the stale assertion on PR #241 (Unit Tests job) — fix
applied as a follow-up so the polish bundle stays single-PR.

Author: PaulDotterer

cmd/indexer/main.go

@@ -189,7 +189,11 @@ func parseFlags() *Config {
 	flag.StringVar(&cfg.LogFormat, "log-format", "text", "Log format (text, json)")
 	flag.DurationVar(&cfg.ReconcileInterval, "reconcile-interval", time.Hour, "Interval for periodic full rebuild (0 to disable)")
 	flag.IntVar(&cfg.Concurrency, "concurrency", 5, "Number of concurrent Asynq workers")
-	flag.StringVar(&cfg.HealthAddr, "health-addr", ":8082", "Health check endpoint address")
+	// :8090 was previously :8082, which collided with the control
+	// server's InternalListenAddr default on single-host dev setups
+	// (#138). Operators on multi-host deploys can still pin either
+	// via CONTROL_INTERNAL_LISTEN_ADDR + INDEXER_HEALTH_ADDR.
+	flag.StringVar(&cfg.HealthAddr, "health-addr", ":8090", "Health check endpoint address")
 
 	flag.Parse()
 

go.mod

@@ -106,4 +106,4 @@ require (
 // whatever happens to be in a local ../sdk checkout. Developers who
 // want to iterate against a local SDK override this with a per-dev
 // go.work at their workspace root — see server/README.md for setup.
-replace github.com/manchtools/power-manage/sdk => github.com/manchtools/power-manage-sdk v0.4.1-0.20260512183037-dda53a00b743
+replace github.com/manchtools/power-manage/sdk => github.com/manchtools/power-manage-sdk v0.4.1-0.20260512213837-16f6ede929e7

go.sum

@@ -111,8 +111,8 @@ github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 h1:6E+4a0GO5zZEnZ
 github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0/go.mod h1:zJYVVT2jmtg6P3p1VtQj7WsuWi/y4VnjVBn7F8KPB3I=
 github.com/magiconair/properties v1.8.10 h1:s31yESBquKXCV9a/ScB3ESkOjUYYv+X0rg8SYxI99mE=
 github.com/magiconair/properties v1.8.10/go.mod h1:Dhd985XPs7jluiymwWYZ0G4Z61jb3vdS329zhj2hYo0=
-github.com/manchtools/power-manage-sdk v0.4.1-0.20260512183037-dda53a00b743 h1:ANagVtDVc6PmBoFXVcT2gx23tYVrRHAE3OiQoK2sdms=
-github.com/manchtools/power-manage-sdk v0.4.1-0.20260512183037-dda53a00b743/go.mod h1:hG7NcdIGAG0VzhLL/W12FDAgVQJxeK+jHFtHb2eF3A8=
+github.com/manchtools/power-manage-sdk v0.4.1-0.20260512213837-16f6ede929e7 h1:vBwzVqiiOJamAvF6TPhmhJ80DeMj2ofvwB6qCP8iB0I=
+github.com/manchtools/power-manage-sdk v0.4.1-0.20260512213837-16f6ede929e7/go.mod h1:hG7NcdIGAG0VzhLL/W12FDAgVQJxeK+jHFtHb2eF3A8=
 github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
 github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
 github.com/mdelapenya/tlscert v0.2.0 h1:7H81W6Z/4weDvZBNOfQte5GpIMo0lGYEeWbkGp5LJHI=

internal/api/action_dispatch.go

@@ -109,16 +109,23 @@ func (h *ActionHandler) DispatchAction(ctx context.Context, req *connect.Request
 		}
 		inputs.timeoutSeconds = action.TimeoutSeconds
 		inputs.actionID = &source.ActionId
-		// NOTE: action.Signature and action.ParamsCanonical are
-		// loaded from the stored row in the original PR-1 shape but
-		// were unconditionally overwritten by the re-sign call below.
-		// The contract decision (re-sign every dispatch vs. respect
-		// stored signature) is tracked in #137. Until that lands,
-		// drop the dead loads — keeping them only as //lint:ignore
-		// stubs trips ineffassign in golangci-lint and adds noise
-		// without addressing the underlying ambiguity. The comment
-		// stays here so a future contributor sees why these fields
-		// are conspicuously absent on the stored-action branch.
+		// Contract (closes #137 audit F002): always re-sign every
+		// dispatch. action.Signature + action.ParamsCanonical on the
+		// stored row are NOT consumed here — the re-sign step below
+		// overwrites both with a fresh signature against the current
+		// signing key. Reasons:
+		//   1. The signing key may have rotated since the action was
+		//      originally created. A stored signature would become
+		//      unverifiable against the current public key the agent
+		//      has cached.
+		//   2. The agent's verification path doesn't accept "old"
+		//      signatures — it always checks against whatever the
+		//      current control-cert chain says.
+		//   3. The cost of re-signing is one HMAC, dwarfed by the
+		//      Asynq enqueue and the bidi-stream RTT.
+		// The columns stay on the row as audit-history (the original
+		// signature at create-time) but never round-trip into a
+		// dispatch.
 
 	case *pm.DispatchActionRequest_InlineAction:
 		action := source.InlineAction

internal/api/errors_parity_test.go

@@ -118,12 +118,21 @@ func extractSDKCodes(t *testing.T) []string {
 		return nil
 	}
 
+	// Strip comments before matching so a commented-out export (left
+	// as a TODO or for documentation) doesn't get counted as live and
+	// skew the parity check (#143). Removes:
+	//   - `// ...` line comments (until end-of-line)
+	//   - `/* ... */` block comments (single-line; multi-line block
+	//     comments containing export lines are rare in this file but
+	//     handled with the non-greedy `(?s)` form below)
+	src := stripTSComments(string(data))
+
 	// Accept single, double, or backtick-quoted string literals plus
 	// an optional `: string` type annotation so a future refactor to
 	// `export const ErrFoo: string = "…"` or a template literal
 	// doesn't silently hide the code from the parity check.
 	re := regexp.MustCompile(`export\s+const\s+Err\w+(?:\s*:\s*string)?\s*=\s*['"` + "`" + `]([a-z][a-z0-9_]*)['"` + "`" + `]`)
-	matches := re.FindAllStringSubmatch(string(data), -1)
+	matches := re.FindAllStringSubmatch(src, -1)
 	seen := make(map[string]struct{}, len(matches))
 	for _, m := range matches {
 		seen[m[1]] = struct{}{}
@@ -136,6 +145,21 @@ func extractSDKCodes(t *testing.T) []string {
 	return out
 }
 
+// stripTSComments removes `// ...` line comments and `/* ... */`
+// block comments from a TypeScript source string before regex
+// matching. Without this, a commented-out export (left as a TODO or
+// for documentation) would be counted as a live export by the
+// parity check (#143). Imperfect — a `//` inside a string literal
+// would also be eaten — but the parity-check input is the SDK's
+// curated errors.ts file, which doesn't carry such literals.
+func stripTSComments(src string) string {
+	// Block comments first (greedy across newlines via (?s) inline flag).
+	src = regexp.MustCompile(`(?s)/\*.*?\*/`).ReplaceAllString(src, "")
+	// Then line comments (until end of line).
+	src = regexp.MustCompile(`//[^\n]*`).ReplaceAllString(src, "")
+	return src
+}
+
 // diff returns elements in a that are not in b, sorted.
 func diff(a, b []string) []string {
 	present := make(map[string]struct{}, len(b))

internal/api/validator_test.go

@@ -55,7 +55,7 @@ func TestValidate_InvalidULID(t *testing.T) {
 		ID:    "not-a-ulid",
 	})
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "i_d must be a valid ULID")
+	assert.Contains(t, err.Error(), "id must be a valid ULID")
 }
 
 func TestValidate_ValidULID(t *testing.T) {
@@ -89,13 +89,23 @@ func TestValidate_Optional_TooShort(t *testing.T) {
 }
 
 func TestToSnakeCase(t *testing.T) {
+	// Acronym handling pinned by #140 — the previous shape split
+	// contiguous uppercase letters one at a time (`UserID` →
+	// `user_i_d`), which leaked nonsense into operator-facing
+	// validation error messages. New rule: `_` is inserted before an
+	// uppercase letter only at word boundaries (lowercase→upper or
+	// end-of-acronym), so acronyms ride together.
 	tests := map[string]string{
 		"Name":           "name",
-		"UserID":         "user_i_d",
-		"ActionSetID":    "action_set_i_d",
+		"UserID":         "user_id",
+		"ActionSetID":    "action_set_id",
 		"createdAt":      "created_at",
 		"simple":         "simple",
-		"HTTPStatusCode": "h_t_t_p_status_code",
+		"HTTPStatusCode": "http_status_code",
+		"IDOnly":         "id_only", // acronym at start
+		"ID":             "id",      // pure acronym
+		"a":              "a",       // single lowercase
+		"":               "",        // empty
 	}
 	for input, expected := range tests {
 		assert.Equal(t, expected, sdkvalidate.ToSnakeCase(input), "ToSnakeCase(%q)", input)

internal/auth/interceptor.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"connectrpc.com/connect"
+	"github.com/golang-jwt/jwt/v5"
 
 	pm "github.com/manchtools/power-manage/sdk/gen/go/pm/v1"
 	"github.com/manchtools/power-manage/server/internal/middleware"
@@ -223,10 +224,18 @@ func (i *AuthInterceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
 			return nil, authErrorCtx(ctx, errNotAuthenticated, connect.CodeUnauthenticated, "missing authentication credentials")
 		}
 
-		// Validate token
+		// Validate token. Distinguish "expired" from "malformed /
+		// signature-invalid / wrong-type" so the web client can show
+		// the right UX (silent refresh vs forced re-login). Falls back
+		// to errNotAuthenticated for non-expiry failures so the web
+		// error mapping doesn't trigger refresh-and-retry on a token
+		// that can never become valid (#139, audit Bundle A).
 		claims, err := i.jwtManager.ValidateToken(tokenString, TokenTypeAccess)
 		if err != nil {
-			return nil, authErrorCtx(ctx, errTokenExpired, connect.CodeUnauthenticated, "invalid or expired token")
+			if errors.Is(err, jwt.ErrTokenExpired) {
+				return nil, authErrorCtx(ctx, errTokenExpired, connect.CodeUnauthenticated, "token expired")
+			}
+			return nil, authErrorCtx(ctx, errNotAuthenticated, connect.CodeUnauthenticated, "invalid token")
 		}
 
 		// Add user context with permissions from JWT

internal/auth/interceptor_test.go

@@ -189,7 +189,11 @@ func TestAuthInterceptor_MissingHeader(t *testing.T) {
 	assert.Contains(t, err.Error(), "missing authentication credentials")
 }
 
-// TestAuthInterceptor_InvalidToken verifies that an invalid JWT is rejected.
+// TestAuthInterceptor_InvalidToken verifies that a malformed JWT is
+// rejected with the "invalid token" message — distinct from the
+// "token expired" wording covered by TestAuthInterceptor_ExpiredToken.
+// The split (#139) lets the web client choose refresh-and-retry vs
+// forced-relogin instead of reflexively retrying on every 401.
 func TestAuthInterceptor_InvalidToken(t *testing.T) {
 	serverURL, _ := setupInterceptorTest(t)
 
@@ -203,7 +207,9 @@ func TestAuthInterceptor_InvalidToken(t *testing.T) {
 	_, err := client.CallUnary(context.Background(), req)
 	require.Error(t, err)
 	assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
-	assert.Contains(t, err.Error(), "invalid or expired token")
+	assert.Contains(t, err.Error(), "invalid token")
+	assert.NotContains(t, err.Error(), "expired",
+		"malformed-token failures must not surface as 'expired' — keeps the web error-mapping branches disjoint")
 }
 
 // TestAuthInterceptor_CookieNoLongerAccepted verifies that cookie-based auth
@@ -297,6 +303,11 @@ func TestAuthInterceptor_ExpiredToken(t *testing.T) {
 	_, err = client.CallUnary(context.Background(), req)
 	require.Error(t, err)
 	assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
+	// #139: expired tokens specifically must surface the "token
+	// expired" wording so the web client can pick refresh-and-retry.
+	// Malformed-token failures use "invalid token" instead — see
+	// TestAuthInterceptor_InvalidToken.
+	assert.Contains(t, err.Error(), "token expired")
 }
 
 // TestAuthInterceptor_WrongSecret verifies that a token signed with a different
@@ -321,6 +332,10 @@ func TestAuthInterceptor_WrongSecret(t *testing.T) {
 	_, err = client.CallUnary(context.Background(), req)
 	require.Error(t, err)
 	assert.Equal(t, connect.CodeUnauthenticated, connect.CodeOf(err))
+	// #139: signature-invalid is a hard "invalid", not an expiry —
+	// the web client must NOT treat this as a refreshable failure.
+	assert.Contains(t, err.Error(), "invalid token")
+	assert.NotContains(t, err.Error(), "expired")
 }
 
 // setupRateLimitedInterceptorTest stands up an httptest server that