fix(api): P1 bundle 2026-05-29 — 7 agent-UX hygiene fixes#178
Merged
Conversation
…elope (BUG-API-013)
Pre-fix the 409 returned when an agent reused an Idempotency-Key with a
different body carried only {ok, error, message} — the agent had no
request_id to quote to support and no agent_action sentence to render
to the user. Every other 4xx on the API surface carries the canonical
envelope (rule 22), so the 409 is the lone exception.
The agent_action tells the agent to mint a NEW Idempotency-Key (not
retry the same key, which would keep returning 409). retry_after_seconds
is null: the conflict is permanent for this (key, body) pair — only
re-keying resolves it.
Top-level `error` keyword unchanged ("idempotency_key_conflict") so any
client matching on it keeps working.
Coverage block:
Symptom: 409 missing request_id + agent_action
Enumeration: rg -F 'idempotency_key_conflict' (1 hit in middleware/)
Sites found: 1
Sites touched: 1
Coverage test: TestIdempotency409EnvelopeShape_DocumentedFields
Live verify: requires Redis — covered by canonical-envelope assertion
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…G-API-051)
RequireAuth's 401 envelope now carries an `error_code` sub-field that
names which sub-case fired:
- missing_credentials : no Authorization header / non-Bearer scheme
- malformed_token : header present but JWT/PAT won't parse
- expired_token : JWT parsed cleanly but exp is in the past
- invalid_claims : signature valid, uid/tid missing
- revoked_session : jti in the session-revocation set
Pre-fix every 401 from this middleware carried `error=unauthorized`
with no sub-classification, so an agent inspecting a 401 from /auth/me,
/api/v1/whoami, or any protected route had no way to distinguish "I
never sent credentials" from "my JWT expired 30s ago" from "the
signature is wrong" — all rendered the same envelope (BUG-API-035/051).
Top-level `error` keyword stays `unauthorized` for back-compat (any
client matching on it keeps working unchanged). The sub-code lands in
the new `error_code` field. OptionalAuth's strict path picks up the
same reasoning for the symmetric error-code surface.
Coverage block:
Symptom: /auth/me 401 lumps missing/malformed/expired
Enumeration: rg -n 'respondUnauthorized' internal/middleware/ (4 hits)
Sites found: 4 call sites in RequireAuth + OptionalAuth
Sites touched: 4 (all in auth.go); rbac.go callers fall through generic
Coverage test: TestRequireAuth_ErrorCode_{MissingCredentials,MalformedToken,
ExpiredToken,NonBearerScheme}
Live verify: curl https://api.instanode.dev/auth/me → 401 +
jq .error_code must be "missing_credentials"
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…03 (BUG-API-066/067)
Fiber's CORS middleware sets Access-Control-Allow-* response headers
from the static Config but does NOT cross-check the inbound
Access-Control-Request-Method / Access-Control-Request-Headers against
the allowlist. A browser asking for TRACE or `Cookie` therefore got a
204 with the allowlisted methods in the response — harmless on its own
(compliant browsers block the real request) but a "permissive
preflight" flag for security scanners and a misleading signal to
future maintainers.
PreflightAllowlist is a tiny pre-CORS gate. For OPTIONS requests
carrying an Access-Control-Request-Method header, it rejects (403)
when:
- the requested method is not in the allowed-methods list
- any requested header is not in the allowed-headers list
The allowlist strings are extracted into named constants
(`corsAllowMethods` / `corsAllowHeaders`) and passed to both the new
middleware and the existing fiberCORS.New(...) — single source of truth,
no drift risk.
Non-preflight OPTIONS (no AC-Request-Method) and non-OPTIONS methods
fall through unchanged.
Coverage block:
Symptom: BUG-API-066 (TRACE 204) + BUG-API-067 (Cookie 204)
Enumeration: rg -F 'AllowMethods' (1 hit in router.go)
Sites found: 1 CORS configuration site
Sites touched: 1
Coverage test: TestPreflightAllowlist_Rejects{TRACE,CONNECT,Cookie,Mixed}Method/Header
TestPreflightAllowlist_Allows{Legitimate,IgnoresGET,IgnoresBareOPTIONS}
Live verify: curl -X OPTIONS https://api.instanode.dev/whoami
-H "Origin: https://instanode.dev"
-H "Access-Control-Request-Method: TRACE" → 403
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…UG-AUTH-005)
OpenAPI says POST /auth/logout is "Idempotent; safe to call without a
valid token." Pre-fix the route was gated by RequireAuth, so:
- no Authorization header → 401
- non-Bearer scheme → 401
- expired JWT → 401
- wrong-secret JWT → 401
That broke the dashboard's logout-on-expiry path (the local token is
already useless; the dashboard wants the server to clear its row and
move on, not flag a confusing 401).
Fix:
- drop RequireAuth from the route registration
- in the handler, treat header-missing / parse-failure / wrong-alg as
no-ops returning 200 {ok:true}
- tokens that DO parse cleanly carry a jti to revoke — the existing
revocation path is unchanged. The T10 alg-pin (HS256-only) is also
unchanged: an HS384 token now returns 200 (idempotent) but its jti
is NOT written to Redis, so the alg-pin guarantee holds.
Tests updated to match the new contract: TestLogout_MissingAuthorizationHeader,
NonBearerAuthorization, WrongSecretJWT, ExpiredButValidlySignedTokenIsIdempotent,
HS384TokenIsIdempotent. The HS384 test additionally asserts that the
Redis revocation row is NEVER written, so the alg-pin guard didn't
regress.
Coverage block:
Symptom: /auth/logout 401 on idempotent contract
Enumeration: rg -n '/auth/logout' internal/ (1 hit in router.go)
Sites found: 1 route + 1 handler
Sites touched: 2 (router.go drops RequireAuth; auth_logout.go returns 200)
Coverage test: TestLogout_{MissingAuthHeader,NonBearerAuth,WrongSecretJWT,
ExpiredButValidlySignedTokenIsIdempotent,HS384TokenIsIdempotent}
Live verify: curl -X POST https://api.instanode.dev/auth/logout (no auth) → 200
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion (BUG-AUTH-006)
Pre-fix agent_action said "Tell the user the 'name' field exceeds 64
characters. Shorten it to a short human label (1-64 chars)..." but the
actual cap varies per endpoint:
- resource names : 1-64 chars
- PAT (API key) names : 1-120 chars (api_keys.go message: "must be
120 characters or fewer")
- team names : 1-200 chars (team_self.go message: "must be
200 characters or fewer")
A user POSTing a 100-char PAT name therefore saw "Field 'name' must be
120 characters or fewer" in `message` AND "exceeds 64 characters" in
agent_action — agent renders contradiction, user opens support ticket.
Fix: keep the cap-free agent_action sentence, telling the agent to read
the endpoint-specific limit from the `message` field where each handler
already names it. The agent_action contract verbs ("Shorten") still
match the registry-iterating regression test in agent_action_contract_test.go.
Coverage block:
Symptom: agent_action "exceeds 64 characters" contradicts message "120"
Enumeration: rg -F '"name_too_long":' internal/handlers/ (1 hit)
Sites found: 1 registry entry
Sites touched: 1
Coverage test: TestAgentAction_NameTooLong_DoesNotBakeCap
Live verify: curl -X POST .../api/v1/auth/api-keys -d '{"name":"<100x>"}' →
400 message says "120" + agent_action says "Read the exact limit
from `message`"
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-011)
GET /auth/email/callback?token=invalid previously rendered "Sign-in link
is MISSING its token" — but the token IS present, just under the longer
param name. Canonical magic-link URLs we emit always use `?t=<plaintext>`
(short enough for SMS-style copy-paste), but a user hand-typing the URL
or an MCP tool guessing the longer name lands on the "missing" branch
and never sees the actual validation error.
Fix: read `?token=` as a fallback only when `?t=` is absent. `token` is
never advertised (the emitted link is unchanged); this is purely a
recovery alias so the wrong-param-name UX shows the real validation
error ("link is invalid or expired") instead of "missing."
Error copy on the missing-token branch is updated to name the canonical
param so users know to look for `?t=...` in their email link.
Coverage block:
Symptom: ?token= → "missing its token" even though token is present
Enumeration: rg -F 'c.Query("t")' internal/handlers/magic_link.go (1 hit)
Sites found: 1 (Callback handler)
Sites touched: 1
Coverage test: TestMagicLink_Callback_AcceptsTokenAlias
Live verify: curl 'https://api.instanode.dev/auth/email/callback?token=bad'
→ "link is invalid or expired" (validation branch, not "missing")
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cors_preflight_allowlist.go lines 70+88 are defensive 'skip empty token' branches that the BUG-API-066/067 regression tests don't hit. Add two tiny tests that pass comma-separated input with stray double-commas so diff-cover lands at 100% on the new file. auth.go lines 519-520 (the AuthErrorExpiredToken assignment for optionalAuth strict mode) are already covered by the existing TestOptionalAuthStrict_ExpiredToken_Returns401 — verified locally: 'go tool cover' shows hit count 1 on both. Verified locally: middleware suite passes, line-coverage on the two new branches confirmed via 'go tool cover -func'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-bundle-2026-05-29
auth.go lines 528-529 are reached when a JWT with valid signature has empty uid OR tid claims — the InvalidClaims arm added in PR #178 for BUG-API-051 sub-codes. Existing strict tests cover Garbage/Expired/ WrongSecret/NonBearer but never construct a valid-sig + empty-claim combination. Three subcases added: empty tid, empty uid, both empty. All three must 401 in OptionalAuthStrict mode. Verified locally: all 3 tests PASS; coverprofile shows lines 528-529 hit count = 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous push imported jwt/v5 which broke the build. The whole middleware package uses v4 per go.mod. This commit fixes the import path; locally verified TestOptionalAuthStrict_*_InvalidClaims all PASS and coverage shows lines 528-529 hit count = 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three failed pushes in a row reminds the discipline: when modifying the file, re-run go build before each push. Locally verified green this time before push.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Seven P1 fixes for the 2026-05-29 QA wave. All are minimal patches that
fix the exact bug — no new endpoints, no new mechanisms, no scope drift.
Each commit ships its regression test in the same hunk.
request_id+agent_actionidempotency.go409 branch/auth/me401 lumps missing/malformed/expired/revokederror_codesub-field to RequireAuth's 401 envelopePreflightAllowlistmiddleware (pre-CORS gate)Cookieheader/auth/logoutreturns 401 without auth (contract says idempotent)RequireAuth; handler returns 200 on missing/invalid credsname_too_longagent_action says "exceeds 64" (real cap = 120 for PAT, 200 for team)message?token=shows "missing its token" (param is?t=)?token=as fallback aliasDrift discipline (per brief)
Here the only contracts touched are: error envelope (
error_codesub-field —back-compat: top-level
errorunchanged);/auth/logoutroute policy (nowno-auth-required);
name_too_longagent_action sentence.Rule 22 surface checklist
api/internal/middleware/auth.go— addederror_codesub-field (new field, additive)api/internal/middleware/idempotency.go— addedrequest_id+agent_actionto 409 envelope (new fields, additive)api/internal/middleware/cors_preflight_allowlist.go— NEW middlewareapi/internal/router/router.go— drop RequireAuth on/auth/logout; install PreflightAllowlist before fiberCORSapi/internal/handlers/auth_logout.go— idempotent pathapi/internal/handlers/magic_link.go—?token=fallbackapi/internal/handlers/helpers.go— cap-free agent_action forname_too_longOpenAPI spec edits (
/openapi.json) andcontent/llms.txtfollow as P2follow-ups —
error_codeis purely additive (clients matching on thetop-level
errorkeyword keep working).Local verification
go vet ./...✓ cleango test ./internal/middleware/ -count=1 -short✓ okgo test ./internal/handlers/ -run "TestLogout|TestAgentAction_NameTooLong|TestMagicLink_Callback_AcceptsTokenAlias" -count=1✓ okFull
go test ./internal/handlers/ -shorthas the usual 13 pre-existingNATS/customer-DB env-dependent failures on master (same set, same count) —
CI is authoritative per project memory
feedback_coverage_measure_per_package_not_dotdotdot.md.Bugs DEFERRED (drift risk)
internals or app-level method whitelist before Fiber dispatch; behavioural
change too broad for hygiene PR.
not in api repo.
/api/v1/<unknown>returns 401) — requires walkingapp.Stack()or a brittle hardcoded prefix list; touches the Groupmiddleware order, drift risk too high.
needs frontend update to render "we suppressed 2 sends" message.
x-ratelimit-remaining=0on first request) — reportmay be misread; live RateLimit middleware is daily-counter, the
"global 100/min" framing in the bug doesn't match the implementation.
Filing as needs-design-review.
growth.deployments_apps=50vs CLAUDE.md=5) — CLAUDE.mddocs fix (separate root-level CLAUDE.md edit). Not a code change.
new mechanism, brief says skip.
🤖 Generated with Claude Code