feat: cache stack API tokens to disk and lazy-load OIDC discovery#116
Merged
Conversation
Contributor
WalkthroughImplements disk-based caching for Stack API tokens (read/write per profile/org/stack) and introduces a lazy relying-party wrapper to defer OIDC discovery. Token source constructors and NewStackClient are updated to propagate cmd/profile/org/stack context used for cache persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant TokenSrc as stackTokenSource
participant DiskCache as Disk Cache
participant RemoteAuth as Remote Auth
Client->>TokenSrc: Token()
alt Valid cached token exists and not expired
TokenSrc->>DiskCache: ReadCachedStackAPIToken(cmd, profile, org, stack)
DiskCache-->>TokenSrc: CachedStackAPIToken
TokenSrc-->>Client: AccessToken (from cache)
else No valid cache / expired
TokenSrc->>RemoteAuth: Fetch/Refresh token
RemoteAuth-->>TokenSrc: AccessToken
TokenSrc->>DiskCache: WriteCachedStackAPIToken(cmd, profile, org, stack)
DiskCache-->>TokenSrc: ✓ Written (best-effort)
TokenSrc-->>Client: AccessToken
end
sequenceDiagram
participant Code as Client Code
participant LazyRP as lazyRelyingParty
participant Discovery as OIDC Discovery
Code->>LazyRP: HttpClient()
LazyRP-->>Code: *http.Client (no discovery)
Code->>LazyRP: OAuthConfig() / other OIDC call
LazyRP->>LazyRP: mustDelegate() / init() (once, may retry)
LazyRP->>Discovery: perform OIDC discovery
Discovery-->>LazyRP: delegate RelyingParty
LazyRP->>LazyRP: delegate.OAuthConfig()
LazyRP-->>Code: *oauth2.Config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sylr
force-pushed
the
feat/cache-stack-token
branch
3 times, most recently
from
ed887e7 to
dffbb6a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/clients.go (1)
590-598: Best-effort cache write silently discards errors — acceptable but worth a debug log.The
_ =on line 593 silently swallows write failures (e.g., disk full, permission denied). This is fine for best-effort caching, but adding a debug-level log would help diagnose cache misses in the field.♻️ Add debug logging on cache write failure
- _ = WriteCachedStackAPIToken(t.cmd, t.profileName, t.organizationID, t.stackID, CachedStackAPIToken{ + if err := WriteCachedStackAPIToken(t.cmd, t.profileName, t.organizationID, t.stackID, CachedStackAPIToken{ AccessToken: token.AccessToken, TokenType: token.TokenType, Expiry: token.Expiry, - }) + }); err != nil { + pterm.Debug.Printfln("Failed to write stack API token cache: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clients.go` around lines 590 - 598, The best-effort cache write currently swallows errors with `_ = WriteCachedStackAPIToken(...)`; update the call in the token refresh/issuance path (where t.cmd, t.profileName, t.organizationID, t.stackID and CachedStackAPIToken are used) to capture the returned error and, on non-nil error, emit a debug-level log (e.g., using processLogger or the existing logger in scope) that includes a concise message and the error value so cache write failures (disk full, permission denied) are visible without changing behavior when successful. Ensure you do not change the call semantics beyond logging the error.pkg/relyingparty.go (1)
33-41: Storingcontext.Contextin a struct is discouraged by Go conventions, though practical risk is low here.The
ctxis captured fromcmd.Context()(Cobra command context), which outlives the entire command execution. Lazy initialization occurs when methods are invoked during command execution, so the context should remain valid. However, the pattern is unconventional; consider acceptingcontext.Contextat the point of use instead if context cancellation becomes relevant.More concerning:
sync.Oncecaches initialization errors permanently. If OIDC discovery encounters a transient failure (network timeout, temporary service unavailability), the error is cached forever and all subsequent method calls will panic with the same error. There is no recovery mechanism. If retry-on-failure is important, reconsider thesync.Oncepattern.The panic on discovery failure is consistent with error handling patterns used throughout this codebase (see
pkg/http.go), so that aspect is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/relyingparty.go` around lines 33 - 41, The struct lazyRelyingParty currently stores a context (ctx) and uses sync.Once which permanently caches init errors; remove the ctx field from lazyRelyingParty and change all initialization/use sites (methods on lazyRelyingParty and any callers) to accept a context.Context parameter at call time instead of reading p.ctx; replace sync.Once with a short, explicit init routine protected by a sync.Mutex (e.g., check if delegate != nil under the mutex, otherwise attempt initialization using the passed context, set delegate on success and set initErr only transiently) so transient discovery/network failures do not become permanently cached—refer to lazyRelyingParty, once, delegate, and initErr to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/clients.go`:
- Around line 559-571: The cache logic in NewStackTokenSource (t.accessToken
flow) ignores the promised --stack-token-cache-ttl and uses only token Expiry,
and it accepts cached tokens with no grace period; fix by adding a cache TTL
parameter to NewStackTokenSource (and its callers) that reads the flag/env
(STACK_TOKEN_CACHE_TTL) with default 5m and treating 0 as disabled, then when
loading cached tokens via ReadCachedStackAPIToken apply TTL/grace logic: compute
now := time.Now(); if ttl == 0 skip cache; otherwise treat cached token as valid
only if cached.Expiry.After(now.Add(cacheGrace)) and if cachedSavedAt + ttl >
now (or use cached.Expiry.Sub(cachedSavedAt) >= minTTL) so tokens with less than
a short grace (e.g., 30s) remaining are rejected; ensure the flag name and
parameter flow (NewStackTokenSource, ReadCachedStackAPIToken usage, and
t.accessToken assignment) are updated accordingly to enforce TTL and grace
period and document that 0 disables caching.
In `@pkg/profile.go`:
- Around line 108-110: GetAuthRelyingParty currently hides discovery failures by
returning a Lazy relying party that will panic later in mustDelegate; restore a
surfaced error path by changing NewLazyRelyingParty/GetAuthRelyingParty to
return an error (or perform OIDC discovery eagerly) and propagate that error to
callers like EnsureMembershipAccess and EnsureOrganizationAccess. Specifically,
update NewLazyRelyingParty to return (RelyingParty, error) or add an explicit
Validate/Init call that does discovery and returns an error instead of deferring
to mustDelegate, change GetAuthRelyingParty to return that error instead of nil,
and update callers (EnsureMembershipAccess, EnsureOrganizationAccess) to handle
and return the error rather than allowing a panic from mustDelegate.
In `@pkg/relyingparty.go`:
- Around line 43-55: The current lazyRelyingParty uses sync.Once in init() so a
transient GetAuthRelyingParty error (stored in l.initErr) becomes permanent and
mustDelegate() panics; replace this with a retry-capable initialization: remove
sync.Once usage and instead protect initialization with a sync.Mutex (or
RWMutex), have init attempt to call GetAuthRelyingParty (function name) and on
failure clear any partial state and return the error to callers (propagate via
mustDelegate() by returning a recoverable error or a nil-safe fallback) rather
than panic; update mustDelegate() to call the new init that can retry, avoid
panic(fmt.Sprintf(...)) and either return an error up the call chain or ensure
higher-level code handles the error/recovers, referencing lazyRelyingParty.init,
lazyRelyingParty.mustDelegate, l.delegate, l.initErr, and GetAuthRelyingParty to
locate changes.
---
Nitpick comments:
In `@pkg/clients.go`:
- Around line 590-598: The best-effort cache write currently swallows errors
with `_ = WriteCachedStackAPIToken(...)`; update the call in the token
refresh/issuance path (where t.cmd, t.profileName, t.organizationID, t.stackID
and CachedStackAPIToken are used) to capture the returned error and, on non-nil
error, emit a debug-level log (e.g., using processLogger or the existing logger
in scope) that includes a concise message and the error value so cache write
failures (disk full, permission denied) are visible without changing behavior
when successful. Ensure you do not change the call semantics beyond logging the
error.
In `@pkg/relyingparty.go`:
- Around line 33-41: The struct lazyRelyingParty currently stores a context
(ctx) and uses sync.Once which permanently caches init errors; remove the ctx
field from lazyRelyingParty and change all initialization/use sites (methods on
lazyRelyingParty and any callers) to accept a context.Context parameter at call
time instead of reading p.ctx; replace sync.Once with a short, explicit init
routine protected by a sync.Mutex (e.g., check if delegate != nil under the
mutex, otherwise attempt initialization using the passed context, set delegate
on success and set initErr only transiently) so transient discovery/network
failures do not become permanently cached—refer to lazyRelyingParty, once,
delegate, and initErr to locate the code to modify.
Comment on lines
559
to
+571
| if t.accessToken == nil || t.accessToken.Expiry.Before(time.Now()) { | ||
| // Try to load from disk cache | ||
| if t.cmd != nil { | ||
| cached, err := ReadCachedStackAPIToken(t.cmd, t.profileName, t.organizationID, t.stackID) | ||
| if err == nil && cached != nil && cached.Expiry.After(time.Now()) { | ||
| t.accessToken = &oauth2.Token{ | ||
| AccessToken: cached.AccessToken, | ||
| TokenType: cached.TokenType, | ||
| Expiry: cached.Expiry, | ||
| } | ||
| return t.accessToken, nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TTL flag and implementations
echo "=== Searching for stack-token-cache-ttl flag ==="
rg -n "stack-token-cache-ttl" --type=go
echo -e "\n=== Searching for STACK_TOKEN_CACHE_TTL ==="
rg -n "STACK_TOKEN_CACHE_TTL" --type=go
echo -e "\n=== Searching for cacheTTL, tokenCacheTTL, CacheTTL patterns ==="
rg -n "cacheTTL|tokenCacheTTL|CacheTTL|cacheTimeout|cache.*[Tt][Tt][Ll]" --type=go
echo -e "\n=== Searching for time.Duration related to cache ==="
rg -n "Duration.*cache|cacheDuration|cache.*Duration" --type=go
echo -e "\n=== Looking for NewStackTokenSource function signature ==="
rg -n "func NewStackTokenSource" --type=go -A 10Repository: formancehq/fctl
Length of output: 832
🏁 Script executed:
#!/bin/bash
# Search for grace period, clock skew, buffer logic
echo "=== Searching for grace period / skew / buffer logic ==="
rg -n "grace|skew|buffer|before.*duration|after.*duration" --type=go pkg/clients.go
echo -e "\n=== Reading lines 559-571 from pkg/clients.go with context ==="
sed -n '555,575p' pkg/clients.go
echo -e "\n=== Checking if there's any TTL or flag definition in cmd/ or commands/ ==="
find . -type f -name "*.go" -path "*/cmd/*" -o -path "*/commands/*" | head -20 | xargs grep -l "stack.*token.*cache\|cache.*ttl" 2>/dev/null || echo "No matches found"Repository: formancehq/fctl
Length of output: 977
🏁 Script executed:
#!/bin/bash
# Let's search for proxy.go mentioned in the review scratchpad to understand context
echo "=== Searching for proxy.go and similar caching implementations ==="
find . -type f -name "proxy.go" | head -5
echo -e "\n=== Checking proxy.go for TTL implementation ==="
rg -n "stack-token-cache-ttl|cacheTTL|CacheTTL" --type=go pkg/proxy.go 2>/dev/null || echo "Not found or file doesn't exist"
echo -e "\n=== Looking at the PR/commit message or CHANGELOG ==="
git log --oneline -20 2>/dev/null || echo "Git not available"
echo -e "\n=== Search for any flag definitions in the codebase ==="
rg -n "Flag.*cache|AddFlag.*cache" --type=go | head -20Repository: formancehq/fctl
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Get full commit message
echo "=== Full commit message ==="
git show --no-patch --format=%B dffbb6a 2>/dev/null || echo "Cannot retrieve full commit"
echo -e "\n=== Look for any PR or description files ==="
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "stack.*token.*cache.*ttl\|stack-token-cache-ttl" 2>/dev/null || echo "No matches"
echo -e "\n=== Verify the cache read logic has correct semantics ==="
echo "Checking if cached.Expiry.After(time.Now()) is correct check:"
sed -n '559,571p' pkg/clients.goRepository: formancehq/fctl
Length of output: 1343
🏁 Script executed:
#!/bin/bash
# Final verification: confirm the flag is missing from the actual codebase
echo "=== Search for any flag parsing in main.go or root command ==="
rg -n "PersistentFlags|Flags()" --type=go cmd/ | grep -i cache || echo "No cache flag setup found"
echo -e "\n=== Check if NewStackTokenSource is called with any TTL parameter ==="
rg -n "NewStackTokenSource" --type=go -B 5 -A 5
echo -e "\n=== Verify ReadCachedStackAPIToken doesn't handle TTL ==="
rg -n "func ReadCachedStackAPIToken" --type=go -A 20Repository: formancehq/fctl
Length of output: 2536
Cache implementation missing promised --stack-token-cache-ttl flag and grace period.
The commit message states: "Adds --stack-token-cache-ttl flag (default 5m, env STACK_TOKEN_CACHE_TTL) to control cache duration. Setting it to 0 disables caching." However, this flag is not implemented anywhere in the codebase. The NewStackTokenSource function receives no TTL parameter, and the cache validity check on line 563 only verifies the token's own Expiry field—there is no TTL-based expiration or option to disable caching.
Additionally, returning a cached token with 1 second of validity remaining (line 563: cached.Expiry.After(time.Now())) provides no grace period for clock skew, risking token expiration mid-request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/clients.go` around lines 559 - 571, The cache logic in
NewStackTokenSource (t.accessToken flow) ignores the promised
--stack-token-cache-ttl and uses only token Expiry, and it accepts cached tokens
with no grace period; fix by adding a cache TTL parameter to NewStackTokenSource
(and its callers) that reads the flag/env (STACK_TOKEN_CACHE_TTL) with default
5m and treating 0 as disabled, then when loading cached tokens via
ReadCachedStackAPIToken apply TTL/grace logic: compute now := time.Now(); if ttl
== 0 skip cache; otherwise treat cached token as valid only if
cached.Expiry.After(now.Add(cacheGrace)) and if cachedSavedAt + ttl > now (or
use cached.Expiry.Sub(cachedSavedAt) >= minTTL) so tokens with less than a short
grace (e.g., 30s) remaining are rejected; ensure the flag name and parameter
flow (NewStackTokenSource, ReadCachedStackAPIToken usage, and t.accessToken
assignment) are updated accordingly to enforce TTL and grace period and document
that 0 disables caching.
Comment on lines
+108
to
110
| relyingParty := NewLazyRelyingParty(cmd.Context(), GetHttpClient(cmd), profile.GetMembershipURI()) | ||
|
|
||
| return profile, profileName, relyingParty, nil |
Contributor
There was a problem hiding this comment.
Error path removed — OIDC discovery failures now surface as panics.
Previously, GetAuthRelyingParty returned an error that was propagated to the caller. Now NewLazyRelyingParty never fails at construction time, but discovery errors will eventually panic inside mustDelegate() (in pkg/relyingparty.go). This means callers of the returned relyingParty (e.g., EnsureMembershipAccess, EnsureOrganizationAccess) will see panics instead of handleable errors if OIDC discovery fails.
This is the downstream consequence of the design choice discussed in pkg/relyingparty.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/profile.go` around lines 108 - 110, GetAuthRelyingParty currently hides
discovery failures by returning a Lazy relying party that will panic later in
mustDelegate; restore a surfaced error path by changing
NewLazyRelyingParty/GetAuthRelyingParty to return an error (or perform OIDC
discovery eagerly) and propagate that error to callers like
EnsureMembershipAccess and EnsureOrganizationAccess. Specifically, update
NewLazyRelyingParty to return (RelyingParty, error) or add an explicit
Validate/Init call that does discovery and returns an error instead of deferring
to mustDelegate, change GetAuthRelyingParty to return that error instead of nil,
and update callers (EnsureMembershipAccess, EnsureOrganizationAccess) to handle
and return the error rather than allowing a panic from mustDelegate.
Contributor
Author
There was a problem hiding this comment.
The RelyingParty interface methods don't return errors, so we can't propagate discovery failures through them. The panic is an inherent constraint of the interface.
We've mitigated this by replacing sync.Once with sync.Mutex in the lazy init (addressing the related comment on relyingparty.go), so transient failures are retried on the next call instead of being permanently cached. If discovery fails permanently, the panic message includes the underlying error, which is reasonable for a CLI tool.
Comment on lines
+43
to
+55
| func (l *lazyRelyingParty) init() { | ||
| l.once.Do(func() { | ||
| l.delegate, l.initErr = GetAuthRelyingParty(l.ctx, l.httpClient, l.membershipURI) | ||
| }) | ||
| } | ||
|
|
||
| func (l *lazyRelyingParty) mustDelegate() client.RelyingParty { | ||
| l.init() | ||
| if l.initErr != nil { | ||
| panic(fmt.Sprintf("OIDC discovery failed: %v", l.initErr)) | ||
| } | ||
| return l.delegate | ||
| } |
Contributor
There was a problem hiding this comment.
sync.Once + panic makes transient failures permanent and unrecoverable.
Two concerns:
-
Permanent failure on transient errors:
sync.OnceguaranteesDoruns exactly once. If OIDC discovery fails due to a transient network issue, every subsequent call tomustDelegate()will panic with the cached error — no retry is possible for the lifetime of the process. -
panicinstead of error propagation: For a CLI tool, panicking produces an ugly stack trace instead of a user-friendly error message. Since theRelyingPartyinterface methods don't return errors, this is understandably constrained, but consider at minimum recovering the panic at a higher level, or using a pattern likesync.Mutexwith retry capability.
♻️ Sketch: retry-capable lazy init
type lazyRelyingParty struct {
ctx context.Context
httpClient *http.Client
membershipURI string
- once sync.Once
+ mu sync.Mutex
delegate client.RelyingParty
initErr error
}
func (l *lazyRelyingParty) init() {
- l.once.Do(func() {
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ if l.delegate != nil {
+ return
+ }
+ l.delegate, l.initErr = GetAuthRelyingParty(l.ctx, l.httpClient, l.membershipURI)
- })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/relyingparty.go` around lines 43 - 55, The current lazyRelyingParty uses
sync.Once in init() so a transient GetAuthRelyingParty error (stored in
l.initErr) becomes permanent and mustDelegate() panics; replace this with a
retry-capable initialization: remove sync.Once usage and instead protect
initialization with a sync.Mutex (or RWMutex), have init attempt to call
GetAuthRelyingParty (function name) and on failure clear any partial state and
return the error to callers (propagate via mustDelegate() by returning a
recoverable error or a nil-safe fallback) rather than panic; update
mustDelegate() to call the new init that can retry, avoid
panic(fmt.Sprintf(...)) and either return an error up the call chain or ensure
higher-level code handles the error/recovers, referencing lazyRelyingParty.init,
lazyRelyingParty.mustDelegate, l.delegate, l.initErr, and GetAuthRelyingParty to
locate changes.
Cache the stack API token (obtained from FetchStackToken) to disk so that repeated CLI invocations skip the stack OIDC discovery and JWT bearer exchange. Also wrap the membership relying party in a lazy proxy that defers the membership OIDC discovery call until actually needed. The lazy relying party uses sync.Mutex (not sync.Once) so that transient OIDC discovery failures are retried on the next call rather than cached permanently. With a warm cache, stack commands make zero auth network calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sylr
force-pushed
the
feat/cache-stack-token
branch
from
dffbb6a to
4e38284
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/clients.go (1)
559-563: Capturetime.Now()once; extract the 5 s grace constant.Two separate
time.Now()calls (line 559 and line 563) produce different instants within the same locked block. While the 5-second guard makes the gap inconsequential in practice, a single capturednowis more idiomatic. The5*time.Secondis also a magic number worth naming.♻️ Proposed refactor
+const cacheGracePeriod = 5 * time.Second func (t *stackTokenSource) Token() (*oauth2.Token, error) { t.mu.Lock() defer t.mu.Unlock() - if t.accessToken == nil || t.accessToken.Expiry.Before(time.Now()) { + now := time.Now() + if t.accessToken == nil || t.accessToken.Expiry.Before(now) { // Try to load from disk cache if t.cmd != nil { cached, err := ReadCachedStackAPIToken(t.cmd, t.profileName, t.organizationID, t.stackID) - if err == nil && cached != nil && cached.Expiry.After(time.Now().Add(5*time.Second)) { + if err == nil && cached != nil && cached.Expiry.After(now.Add(cacheGracePeriod)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clients.go` around lines 559 - 563, Capture time.Now() once into a local variable (e.g., now := time.Now()) at the start of the check and use that single variable for both expiry comparisons (replace uses in the t.accessToken expiry check and cached.Expiry check), and extract the 5*time.Second magic number into a named constant (e.g., cacheGrace := 5 * time.Second) to replace the literal; apply this change around the block referencing t.accessToken, t.cmd, ReadCachedStackAPIToken and the cached variable so both comparisons use the same timestamp and the grace period is named.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/clients.go`:
- Around line 559-571: The code currently uses a hardcoded 5s grace window and
accepts cached tokens without honoring a max cache TTL; implement the promised
--stack-token-cache-ttl flag (env STACK_TOKEN_CACHE_TTL, default 5m, "0"
disables caching) and use it when deciding to accept cached tokens in the token
retrieval logic (the method that checks t.accessToken and calls
ReadCachedStackAPIToken). Add parsing of the flag/env into a duration variable
(e.g., stackTokenCacheTTL) and enforce two checks before using cached: 1)
cached.Expiry must be after time.Now().Add(gracePeriod) (keep gracePeriod
configurable or maintain current small grace), and 2) the cached token's age
(derived from cached.Expiry and token lifetime or a cached.Created/Stored
timestamp returned by ReadCachedStackAPIToken) must not exceed
stackTokenCacheTTL (treat 0 as disable caching). Wire these checks into the
branch around ReadCachedStackAPIToken and ensure cached is rejected when TTL
expired so a fresh token is fetched.
In `@pkg/profile.go`:
- Around line 108-109: NewLazyRelyingParty defers OIDC discovery, but
mustDelegate currently panics on discovery failure so callers (e.g.,
EnsureMembershipAccess, EnsureOrganizationAccess) can't handle errors; revert to
error propagation: change mustDelegate to return (delegate, error) instead of
panicking, have mustDelegate surface discovery errors from NewLazyRelyingParty
and update callers (EnsureMembershipAccess, EnsureOrganizationAccess, and any
code that assumed GetAuthRelyingParty semantics) to check and return the error
instead of allowing a panic, preserving the original GetAuthRelyingParty-style
error handling contract.
In `@pkg/relyingparty.go`:
- Around line 57-62: mustDelegate() currently panics on OIDC discovery failure
(in lazyRelyingParty.mustDelegate calling l.init()), which still causes an
unrecoverable crash; instead add a top-level panic recovery around the CLI
command dispatch (the main/top-level command handler that invokes your commands)
that calls recover(), converts the panic value to a user-friendly error message
and exits cleanly, and leave lazyRelyingParty.mustDelegate as-is so init()
errors still trigger a controlled panic that will be caught by that global
recover; ensure the recover logic formats the recovered value (from panic) into
a readable error and returns a non-zero exit code.
---
Nitpick comments:
In `@pkg/clients.go`:
- Around line 559-563: Capture time.Now() once into a local variable (e.g., now
:= time.Now()) at the start of the check and use that single variable for both
expiry comparisons (replace uses in the t.accessToken expiry check and
cached.Expiry check), and extract the 5*time.Second magic number into a named
constant (e.g., cacheGrace := 5 * time.Second) to replace the literal; apply
this change around the block referencing t.accessToken, t.cmd,
ReadCachedStackAPIToken and the cached variable so both comparisons use the same
timestamp and the grace period is named.
gfyrag
approved these changes
Feb 20, 2026
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
2 participants
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.
Cache the stack API token (obtained from FetchStackToken) to disk so that repeated CLI invocations skip the stack OIDC discovery and JWT bearer exchange. Also wrap the membership relying party in a lazy proxy that defers the membership OIDC discovery call until actually needed.
With a warm cache, stack commands make zero auth network calls.