Skip to content

fix: warn on endpoint-less sessionAuth, document programmatic refreshOn route (#135) - #147

Merged
garretpremo merged 1 commit into
devfrom
issues/135-refreshon-custom-strategy
Aug 4, 2026
Merged

fix: warn on endpoint-less sessionAuth, document programmatic refreshOn route (#135)#147
garretpremo merged 1 commit into
devfrom
issues/135-refreshon-custom-strategy

Conversation

@garretpremo

Copy link
Copy Markdown
Contributor

Follow-up to #135 / #145. No Closes line — #135 is already resolved and labeled merged to dev.

Summary

#145 merged while this commit was still in flight, pinned to 92d73f8, so 9eac68f was 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.json values being cast without validation — is filed separately as #146.

What changes

Diagnosability: warn on an endpoint-less sessionAuth block

#135 narrowed mergedSessionAuth to blocks that define session.endpoint, because resolveRequestHeaders dereferences config.cookies.applyTo unguarded. That narrowing is correct but fails silently: envConfig.sessionAuth arrives from JSON as an untyped Partial, so a misspelled key (sessions: for session:) now falls through to options.auth with no SessionAuthStrategy at all — where previously the user got a loud failure fetching <baseUrl>undefined.

if (rawSessionAuth && !mergedSessionAuth) {
    console.warn(
        '[apijack] sessionAuth is set but missing session.endpoint — SessionAuthStrategy will not be used.',
    );
}

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 both cli-builder.ts wiring sites.

Docs: the programmatic opt-in route

The CLAUDE.md paragraph added in #135 only documented the .apijack/settings.json route, which applies to consumers on the shared apijack binary. A project with its own bin/<cli>.ts opts in via createCli({ refreshOn: [401] }) — documented on CliOptions.refreshOn in src/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.json row, whose only spelled-out example was customCommands — a reader scanning for settings keys wouldn't have found auth.refreshOn several sections earlier under Session Auth.

Acceptance criteria

  • An endpoint-less sessionAuth block emits a warning naming the missing session.endpoint
  • No warning fires when there is no sessionAuth block at all, or when session.endpoint is present
  • refreshOn behavior 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 strategy
  • CLAUDE.md documents the createCli({ refreshOn }) route as well as the .apijack/settings.json one

Test plan

  • bun test — 1041 pass / 0 fail (up from 1038 on 92d73f8)
  • 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 yields refreshOn: [401] with mergedSessionAuth: undefined is unchanged, so the warning is additive rather than a behavior change.

…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.
@github-actions github-actions Bot added the needs review Open PR awaiting review label Aug 4, 2026
@garretpremo garretpremo added review in progress Review is actively underway and removed needs review Open PR awaiting review labels Aug 4, 2026

@garretpremo garretpremo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review by claude — generated by the review-issue skill. 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.ts step 7 assigns options.sessionAuth straight from env.sessionAuth, so a sessions:-for-session: typo in the env config yields a truthy rawSessionAuth with no session.endpoint → warning fires. The guard isn't dead code.
  • refreshOn behavior from #135 is untouched — the existing assertion that an endpoint-less block still yields refreshOn: [401] with mergedSessionAuth: undefined is unchanged; only a spy wrapper was added around it to keep test output quiet.
  • Both cli-builder.ts wiring sites (_buildRoutineRuntime at L199, run() at L552) are covered by the single call site, and _buildRoutineRuntime memoizes, so a process calling cli.runRoutine() repeatedly warns once rather than per invocation.
  • console.warn writes to stderr, so the warning can't corrupt the MCP stdio JSON-RPC stream on the mcp path.
  • [apijack] prefix matches the existing console.warn convention in cli-builder.ts.
Non-blocking observations
  • An endpoint-less sessionAuth: { refreshOn: [401] } block is a deliberate configuration under #135refreshOn is intentionally sourced from rawSessionAuth even when mergedSessionAuth is undefined (asserted by the existing test at tests/auth/refresh-wiring.test.ts:57), and CLAUDE.md's "settings.json auth.refreshOn takes precedence over sessionAuth.refreshOn when both are set" implies sessionAuth.refreshOn alone 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 beyond refreshOn would 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 a sessions:-typo user straight at the mistake rather than leaving them to diff their config against the docs.

@garretpremo garretpremo added first pass reviewed Review passed with no blocking issues and removed review in progress Review is actively underway labels Aug 4, 2026

@garretpremo garretpremo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Final review by claude — generated by the final-review skill. 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 #135refreshOn is intentionally sourced from rawSessionAuth even when mergedSessionAuth is undefined (asserted by the existing test at tests/auth/refresh-wiring.test.ts:57), and CLAUDE.md's "settings.json auth.refreshOn takes precedence over sessionAuth.refreshOn when both are set" implies sessionAuth.refreshOn alone 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 beyond refreshOn would 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)

@garretpremo garretpremo added approved PR has been fully approved and is ready to merge needs review Open PR awaiting review and removed first pass reviewed Review passed with no blocking issues approved PR has been fully approved and is ready to merge labels Aug 4, 2026
@garretpremo
garretpremo merged commit 9b6585f into dev Aug 4, 2026
12 checks passed
@garretpremo garretpremo added approved PR has been fully approved and is ready to merge and removed needs review Open PR awaiting review labels Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR has been fully approved and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant