feat(cli): enforce required policy flags from man docs - #3840
Open
alkalescent wants to merge 2 commits into
Open
feat(cli): enforce required policy flags from man docs#3840alkalescent wants to merge 2 commits into
alkalescent wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughPolicy command documentation now defines required flags. ChangesPolicy CLI required flags
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
alkalescent
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
The man-doc frontmatter already declared `required: true` on many policy command flags, but `man.DocFlag` never parsed it, so required-ness was only validated at runtime via GetRequiredString/GetRequiredID. Nothing marked the flags required at the cobra layer, so tools generated from the command tree (e.g. MCP) could not advertise which inputs are mandatory. - Add `Required` to `man.DocFlag` and a `Doc.MarkRequiredFlags()` method (mirrors MarkSensitiveFlags; skips flags not registered on the command). - Apply it to every policy command via a sweep in policy.InitCommands(). - Reconcile the doc `required: true` set with the run functions: add it where a flag is unconditionally required but the doc omitted it, and remove it where the doc over-claimed (deprecated no-op flags, "one of" groups, and paginated limit/offset that default rather than enforce).
Mark the unconditionally-required multi-value flags that the handlers reject when empty but the docs left optional, so cobra enforces them and generated schemas advertise them: - subject-mappings create --action (handler errors on zero actions) - dynamic-value-mappings create --action (handler errors on zero actions) - resource-mappings create --terms (GetStringSlice Min: 1) The subject-condition-set "one of" groups stay unmarked (genuinely conditional).
alkalescent
force-pushed
the
feat/policy-required-flags-from-docs
branch
from
August 6, 2026 05:12
bb487b8 to
6dfcb9d
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
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
Policy command flags declared
required: truein their man-doc frontmatter werenever enforced at the cobra layer.
man.DocFlagdid not parse therequiredkey, so required-ness was only checked at runtime via
GetRequiredString/GetRequiredID. As a result, generated tooling that readsthe command tree could not advertise which inputs are
mandatory, and shell completion did not mark them required.
Requiredtoman.DocFlagand aDoc.MarkRequiredFlags()method,mirroring the existing
MarkSensitiveFlags. It skips flags declared requiredin the doc but not registered on the command (subcommand / shared injector),
so a central sweep is safe.
policy.InitCommands(), once all flags are registered.required: trueset with its runfunction so the declared requirements match actual behavior:
required: truewhere a flag is unconditionally required in Gobut the doc omitted it (e.g.
namespaces get --id,resource-mappingsandresource-mapping-groupsid/create flags,attributes valuesids,dynamic-value-mappings createattribute/selector/operator).required: truewhere the doc over-claimed: deprecated no-opflags (
kas-grants assign/unassign), mutually-exclusive "one of" groups(
subject-condition-sets create --subject-sets,attributes create --value, kas key--kas), and paginatedlimit/offsetthat fall back toa default rather than being enforced.
Behavior
Before:
otdfctl policy namespaces getfailed at runtime with a custom error.After: cobra reports
required flag(s) "id" not setand the requirement isvisible in
--help, completion, and generated tool schemas. Commands whose docsover-claimed (e.g.
kas-registry key listwith default pagination) are no longerblocked.
Testing
go build ./...go test ./otdfctl/pkg/man/... ./otdfctl/cmd/...(new tests cover parsing andmarking; existing policy command tests pass)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests