Conversation
Remove the PersistentPreRunE override that forced IsTerminalFunc to return false whenever --output json was set. Interactive prompts now run normally in a TTY regardless of output format; JSON serialisation and TTY detection are fully orthogonal. Also collapse the JSON-specific error branch in resolveRequestIDInteractive to the standard ErrNotInteractive path, and delete the now-obsolete TestResolveRequestIDInteractive_JSONMode test.
…ant request submit - Add --refresh flag to grant request submit to bypass the on-demand role and eligibility caches (mirrors grant/grant env --refresh behavior) - Expand interactive role selection to SUBSCRIPTION, RESOURCE_GROUP, and RESOURCE workspace types using the naive 2-level ancestors pattern validated in the PoC (subscription: 876 roles, resource_group: 873 roles); custom roles scoped to intermediate management groups may be absent - Add refresh bool to NewCachedRolesLister signature (mirrors CachedEligibilityLister) - Delete untracked poc_subscription.go and poc_ondemand_roles.go
There was a problem hiding this comment.
Pull request overview
This PR enhances the grant request submit workflow by adding cache-bypass support (--refresh) and expanding interactive Azure role selection to additional Azure scope workspace types, while also redefining --output json to be serialization-only (not forcing non-interactive behavior).
Changes:
- Add
--refreshtogrant request submitand plumb it through eligibility + on-demand role caching. - Expand
buildOnDemandRequestto support interactive role selection for AzureSUBSCRIPTION,RESOURCE_GROUP, andRESOURCEworkspaces. - Make
--output jsonno longer force non-interactive mode (prompts can still run in a TTY; JSON remains on stdout).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cache/cached_roles.go | Add refresh support to bypass cache reads while still rewriting cache entries. |
| internal/cache/cached_roles_test.go | Update constructor calls and add coverage for refresh behavior. |
| cmd/request_submit.go | Add --refresh, plumb through resolution, and extend on-demand request building for additional Azure scope types. |
| cmd/request_test.go | Update injected function signatures and expand supported workspace-type coverage. |
| cmd/root.go | Stop forcing non-interactive behavior when --output json is set. |
| cmd/request_picker.go | Remove JSON-output-specific non-interactive error path. |
| cmd/request_picker_test.go | Remove the JSON-mode-specific test case accordingly. |
| CLAUDE.md | Update docs for new refresh flag, expanded Azure scope support, and JSON interactivity semantics. |
| CHANGELOG.md | Document the JSON-output behavior change and the new/expanded request submit capabilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ResourceType: resourceType, | ||
| Ancestors: []string{ | ||
| "/" + ws.OrganizationID, | ||
| "/" + ws.WorkspaceID, | ||
| }, |
There was a problem hiding this comment.
For Azure resource scopes, Ancestors is built as "/" + ws.WorkspaceID. In this codebase, subscription workspace IDs can already start with a leading slash (e.g. "/subscriptions/..."), which would produce a double-slash ancestor ("//subscriptions/..."). Normalize to exactly one leading slash (e.g. only prefix when missing, or use strings.TrimPrefix/TrimLeft) before building the ancestors.
| {"management_group", models.WorkspaceType("MANAGEMENT_GROUP"), "providers/Microsoft.Management/managementGroups/root", "dir-456", "azure_resource", 2}, | ||
| {"subscription", models.WorkspaceType("SUBSCRIPTION"), "sub-1", "dir-456", "azure_resource", 2}, | ||
| {"resource_group", models.WorkspaceType("RESOURCE_GROUP"), "rg-1", "dir-456", "azure_resource", 2}, | ||
| {"resource", models.WorkspaceType("RESOURCE"), "res-1", "dir-456", "azure_resource", 2}, |
There was a problem hiding this comment.
TestBuildOnDemandRequest_SupportedTypes uses placeholder workspace IDs like "sub-1"/"rg-1"/"res-1", but real SCA eligibility data can include fully-qualified IDs with a leading slash (e.g. "/subscriptions/..." per existing eligibility tests). Updating these cases to use realistic IDs and asserting ResourceType (and that Ancestors contains the normalized workspace ID without double slashes) would better cover the new Azure scope support.
…ouble-slash Workspace IDs for SUBSCRIPTION/RESOURCE_GROUP/RESOURCE (and MANAGEMENT_GROUP orgID) can arrive with a leading slash from the SCA eligibility API (e.g. /subscriptions/abc-123). Prepending "/" naively produced double-slash ancestors. Use ensureLeadingSlash() to strip any existing leading slashes before re-adding one. Addresses Copilot review comment on PR #42.
…minate race fetchEligibility with no --provider fetches all CSPs concurrently. The mock returned the same response for both AZURE and AWS queries, so allTargets contained the same workspace ID tagged with each CSP in non-deterministic order. resolveTargetCSP would pick whichever appeared first, causing the "az CLI session" Azure post-elevation message to appear or not depending on goroutine scheduling. Fix: set CSP: models.CSPAzure on the selector item so resolveTargetCSP short-circuits at the target.CSP != "" early return.
Summary
--refreshflag togrant request submitto bypass the on-demand role and eligibility caches (mirrorsgrant --refresh/grant env --refresh)SUBSCRIPTION,RESOURCE_GROUP, andRESOURCEworkspace types using the same naive 2-level ancestors pattern asMANAGEMENT_GROUP; validated against the POC tenant (subscription: 876 roles, resource_group: 873 roles vs 879 with full hierarchy — ~1% gap is custom roles on intermediate management groups)NewCachedRolesListersignature updated to includerefresh bool, mirroringCachedEligibilityListerpoc_ondemand_roles.go,poc_subscription.go) removed — both were untrackedTest plan
make testpasses (all unit tests including newTestCachedRolesLister_Refreshand expandedTestBuildOnDemandRequest_SupportedTypescovering subscription/resource_group/resource)make lintpassesgo vet ./...cleangrant request submitwith--refreshshows log line"Cache refresh requested for on-demand roles..."under--verboseSUBSCRIPTION/RESOURCE_GROUPworkspace presents interactive role list (if surfaced by eligibility on test tenant)