fix: warn on endpoint-less sessionAuth, document programmatic refreshOn route (#135) - #147
Merged
Merged
Conversation
…On route (#135) - resolveRefreshWiring now warns when sessionAuth is set but missing session.endpoint (e.g. a "sessions:" typo), so silently falling through to the base strategy stays diagnosable instead of failing opaquely against "<baseUrl>undefined". - CLAUDE.md: document the createCli({ refreshOn }) programmatic route alongside the .apijack/settings.json one, and cross-reference auth.refreshOn from the Project Extensions table.
garretpremo
commented
Aug 4, 2026
garretpremo
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Automated review by claude — generated by the
review-issueskill. Treat as advisory; a human still owns the merge decision.
Looks good — the orphaned follow-up commit from #145 does exactly what it says: one console.warn at the shared decision point in resolveRefreshWiring, three new tests covering the positive and both negative cases, and two CLAUDE.md additions. All four stated acceptance criteria are met, the diff is scoped to those two concerns only, and all 12 CI checks pass.
Verified:
- The motivating scenario actually reaches the warning:
bin/apijack.tsstep 7 assignsoptions.sessionAuthstraight fromenv.sessionAuth, so asessions:-for-session:typo in the env config yields a truthyrawSessionAuthwith nosession.endpoint→ warning fires. The guard isn't dead code. refreshOnbehavior from #135 is untouched — the existing assertion that an endpoint-less block still yieldsrefreshOn: [401]withmergedSessionAuth: undefinedis unchanged; only a spy wrapper was added around it to keep test output quiet.- Both
cli-builder.tswiring sites (_buildRoutineRuntimeat L199,run()at L552) are covered by the single call site, and_buildRoutineRuntimememoizes, so a process callingcli.runRoutine()repeatedly warns once rather than per invocation. console.warnwrites to stderr, so the warning can't corrupt the MCP stdio JSON-RPC stream on themcppath.[apijack]prefix matches the existingconsole.warnconvention incli-builder.ts.
Non-blocking observations
- An endpoint-less
sessionAuth: { refreshOn: [401] }block is a deliberate configuration under #135 —refreshOnis intentionally sourced fromrawSessionAutheven whenmergedSessionAuthisundefined(asserted by the existing test attests/auth/refresh-wiring.test.ts:57), and CLAUDE.md's "settings.jsonauth.refreshOntakes precedence oversessionAuth.refreshOnwhen both are set" impliessessionAuth.refreshOnalone is supported. A project on that route now gets the warning on every CLI invocation for a config that is working as intended. If that turns out to be noisy in practice, gating the warning on the block carrying keys beyondrefreshOnwould keep the typo diagnostic while staying quiet for the intentional case. - The module JSDoc still reads "Kept pure so both sites stay in lockstep (#135)" — the function now has an I/O side effect, so "pure" is no longer literally true even though the lockstep rationale still holds.
Nitpicks
- The warning message says what is missing but not which key was found instead. Echoing the offending block's top-level keys (e.g.
found: sessions, cookies) would point asessions:-typo user straight at the mistake rather than leaving them to diff their config against the docs.
5 tasks
garretpremo
commented
Aug 4, 2026
garretpremo
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Final review by claude — generated by the
final-reviewskill. CI is green, no blockers, soak window elapsed. Marking approved and merging.
Approved.
Outstanding observations from first-pass review
- An endpoint-less
sessionAuth: { refreshOn: [401] }block is a deliberate configuration under #135 —refreshOnis intentionally sourced fromrawSessionAutheven whenmergedSessionAuthisundefined(asserted by the existing test attests/auth/refresh-wiring.test.ts:57), and CLAUDE.md's "settings.jsonauth.refreshOntakes precedence oversessionAuth.refreshOnwhen both are set" impliessessionAuth.refreshOnalone is supported. A project on that route now gets the warning on every CLI invocation for a config that is working as intended. If that turns out to be noisy in practice, gating the warning on the block carrying keys beyondrefreshOnwould keep the typo diagnostic while staying quiet for the intentional case. (#148) - The module JSDoc still reads "Kept pure so both sites stay in lockstep (#135)" — the function now has an I/O side effect, so "pure" is no longer literally true even though the lockstep rationale still holds. (#148)
7 tasks
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
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.
Follow-up to #135 / #145. No
Closesline — #135 is already resolved and labeledmerged to dev.Summary
#145 merged while this commit was still in flight, pinned to
92d73f8, so9eac68fwas left orphaned on the branch. It carries the three non-blocking observations from #145's automated review that were cheap and in-scope, and nothing else.The out-of-scope observation from the same review —
.apijack/settings.jsonvalues being cast without validation — is filed separately as #146.What changes
Diagnosability: warn on an endpoint-less
sessionAuthblock#135 narrowed
mergedSessionAuthto blocks that definesession.endpoint, becauseresolveRequestHeadersdereferencesconfig.cookies.applyTounguarded. That narrowing is correct but fails silently:envConfig.sessionAutharrives from JSON as an untypedPartial, so a misspelled key (sessions:forsession:) now falls through tooptions.authwith noSessionAuthStrategyat all — where previously the user got a loud failure fetching<baseUrl>undefined.Failing soft is the improvement; failing soft and quiet was the regression. One line at the single shared decision point (
src/auth/refresh-wiring.ts) covers bothcli-builder.tswiring sites.Docs: the programmatic opt-in route
The CLAUDE.md paragraph added in #135 only documented the
.apijack/settings.jsonroute, which applies to consumers on the sharedapijackbinary. A project with its ownbin/<cli>.tsopts in viacreateCli({ refreshOn: [401] })— documented onCliOptions.refreshOninsrc/types.ts, but invisible to a reader in CLAUDE.md. Now covered alongside the settings.json route.Also cross-references the Project Extensions table's
.apijack/settings.jsonrow, whose only spelled-out example wascustomCommands— a reader scanning for settings keys wouldn't have foundauth.refreshOnseveral sections earlier under Session Auth.Acceptance criteria
sessionAuthblock emits a warning naming the missingsession.endpointsessionAuthblock at all, or whensession.endpointis presentrefreshOnbehavior from sessionAuth.refreshOn is unreachable for projects using a custom AuthStrategy #135 is unchanged — an endpoint-less block still activates refresh + retry, it just doesn't wrap the strategycreateCli({ refreshOn })route as well as the.apijack/settings.jsononeTest plan
bun test— 1041 pass / 0 fail (up from 1038 on92d73f8)bun run lint— 0 errors (118 pre-existing warnings, unchanged)New coverage in
tests/auth/refresh-wiring.test.ts: the warning fires for an endpoint-less block, and does not fire in either negative case. The existing assertion that an endpoint-less block still yieldsrefreshOn: [401]withmergedSessionAuth: undefinedis unchanged, so the warning is additive rather than a behavior change.