* refactor: extract FetchTAT sharing the TAT-rejection classifier
doResolveTAT minted the tenant access token inline. Extract the HTTP call
into FetchTAT(ctx, httpClient, brand, appID, appSecret) so callers that
already hold plaintext credentials — notably the post-config-init probe —
can validate them without a second keychain round-trip.
FetchTAT routes a non-zero TAT body code through the same
classifyTATResponseCode the credential layer already uses, so a rejection is
the canonical CategoryConfig / SubtypeInvalidClient (10003 / 10014) typed
error — identical to what every token-resolving command returns. Transport,
HTTP-status and JSON-parse failures stay raw (untyped) so callers can use
errs.IsTyped to separate a deterministic credential rejection from upstream
noise. doResolveTAT now delegates to FetchTAT; observable behavior unchanged.
* feat: validate credentials after config init
After config init saves the App ID / App Secret, fire a best-effort probe:
mint a tenant access token with the just-saved credentials, then POST the
application probe endpoint. When the credentials are deterministically
rejected, FetchTAT returns a typed errs.* error and runProbe propagates it,
so config init exits non-zero with the canonical ConfigError / invalid_client
envelope (the same one every other command shows for the same bad creds)
instead of letting the user discover the mistake on a later request.
Ambiguous failures (transport, HTTP non-200, JSON parse, timeout,
http-client init) come back untyped and are swallowed (errs.IsTyped is the
discriminator), so a valid configuration is never blocked by upstream noise.
The probe is wired into all four init paths and skipped when the user reused
an existing secret. The saved config is not rolled back on rejection: stdout
still records what was saved, stderr carries the typed error envelope.
Drive-domain errors now leave the CLI as typed, machine-branchable
envelopes — a stable `type` plus `subtype` and named fields (param,
params, retryable, log_id, hint) — so scripts and AI agents can branch on
structure and act on a recovery hint instead of parsing prose.
Changes:
- Every error produced in the drive domain — validation, file I/O, and the
failures returned from its Lark API calls — is emitted as a typed errs.*
error; the exit code is derived from the error category. Drive's API calls
now go through a shared typed classifier, so failures carry subtype,
troubleshooter, a recovery hint, and the request's log_id whether the
server returns it in the response body or the x-tt-logid header; an
already-typed network/auth error is never downgraded into a generic API
error.
- Known API conditions (resource conflict, cross-tenant, cross-brand, ...)
carry a recovery hint keyed by their error class; a command can refine
that hint with command-specific guidance.
- Batch partial failures (+push / +pull / +sync, where some items succeed
and some fail) now report an honest ok:false multi-status result on
stdout — the summary and every per-item outcome stay machine-readable —
and exit non-zero, instead of a misleading ok:true success envelope.
- Duplicate rel_path conflicts report each colliding path as a structured
params entry (RFC 7807 invalid-params style).
- Static guards lock the drive path so legacy error construction — direct
envelopes or the auto-classifying API helpers — cannot be reintroduced,
making drive the template for the remaining domains.
Output changes worth noting for consumers:
- Error envelopes now carry typed type/subtype and named fields; exit
codes follow the error category (malformed or incomplete API responses
are reported as internal errors rather than generic API errors).
- Batch partial failures (+push / +pull / +sync) emit an ok:false result
envelope on stdout (summary + per-item items[]) and exit non-zero; the
per-item results stay on stdout rather than in a stderr error envelope.
Errors surfaced through shared cross-domain helpers (scope precheck, media
import upload, metadata lookup, save-path resolution) are not yet typed;
they migrate with the shared layer in a follow-up change.
Interactive card messages (msg_type: interactive) can contain @user elements in their card
body. The json_attachment.at_users field stores resolved user info, but the user_id there is
the sender-side platform user_id — not the reading app's canonical open_id. When the backend
populates a mention_key on each at_users entry, it signals that the API-level mentions[]
array carries a more authoritative open_id and display name for the reading context. This PR adds
support for this two-level lookup: it threads the raw mentions[] array into the card converter,
indexes it by mention_key for O(1) access, and renders the canonical open_id + display name
whenever the link is resolvable. All existing fallback paths (no mention_key, nil mentions) are
preserved without behavioral change.
Change-Id: I00f846d76482adba315d07361c35909b71ca74c7
Follow-up to #1223. The hand-written FLAGS block in `lark-cli --help`
restated leaf-command flags at the root level — flags that are not
registered on the root command (they error "unknown flag" there). Even
trimmed to an illustrative example list, it duplicated information Cobra's
per-command `--help` already renders authoritatively, and any static list
in root help drifts from the real per-command flag sets over time.
Drop the section entirely: Cobra's per-command `Flags:` output is the
single source of truth. `USAGE:`/`EXAMPLES:` still show flags in context,
and the `Flags:` block at the bottom of root help lists the actual root
flags. Also removes the now-obsolete TestRootLong_FlagsSectionPointsToCommandHelp.
The hand-written FLAGS block in `lark-cli --help` listed --params, --data,
--as, --format, --page-all, --page-size, --page-limit, --page-delay, -o,
--jq, -q and --dry-run as if they were global flags. None are registered
on the root command — they all error "unknown flag" at the top level and
exist only on leaf commands (api, service). The block also contradicted
the Cobra-generated "Flags:" section rendered directly below it, which
shows only -h/--help, --profile, -v/--version.
Replace it with a short illustrative example list (common flags first) and
a pointer to `lark-cli <command> --help` for the full per-command set.
Root help stays a discovery signpost without claiming the flags are global
or restating defaults/descriptions that drift from the real flag sets.
Change-Id: Ia1cab889dd70b6b49a61dac468dedfd7fe39043f
Simplifies the markdown-to-post rendering pipeline in the IM shortcut. The previous
implementation split markdown at blank-line boundaries into multiple post paragraphs,
using zero-width space (\u200B) sentinel characters to preserve visual spacing.
While well-intentioned, this approach introduced fragility around edge cases such as
blank lines inside fenced code blocks, messages with only blank lines, and interactions
with the heading-normalization pass. This change consolidates rendering back into a
single {"tag":"md"} segment, making the output more predictable, the code significantly
easier to follow, and the test surface easier to maintain.
Change-Id: Ic2870ecbcb31ae7d36121f120102f2ff964f5169
* feat: unconditionally inject --format flag for all shortcuts
Removes three HasFormat guards in runner.go so every shortcut
gets --format regardless of the Shortcut.HasFormat field value.
Shortcuts that already define a custom 'format' flag in Flags[]
are skipped to avoid redefinition panics (e.g. mail +triage, +watch).
HasFormat is retained in the struct but marked deprecated.
Change-Id: I5e8fe07e839d5aed4cefaf7d753dabbaee68fb6e
* test: isolate config dir in format-universal test
Change-Id: I3a59942aa8a6753cd949ca42f2a19a72f032ff55
* test: revert unnecessary config-dir isolation (mount-only test)
Change-Id: I0146e5a2f57f5419863bdeeaa1a662fd8f70bddf
internal/util imported internal/proxyplugin (SharedTransport, FallbackTransport,
NewHTTPClient, and WarnIfProxied via proxyPluginStatus), so a foundational util
package depended up into a feature package, pulling binding/core/vfs into the
transitive cone of every util importer.
Move internal/proxyplugin -> internal/transport and make it the single owner of
outbound transport: fold the two SharedTransport functions into one Shared()
(proxy-plugin override -> LARK_CLI_NO_PROXY -> http.DefaultTransport), and move
Fallback/NewHTTPClient/WarnIfProxied/DetectProxyEnv/noProxyTransport out of the
now-deleted internal/util/proxy.go into the new package. The proxy-plugin probe
is demoted to a private pluginTransport(); the duplicate redactProxyURL collapses
to one. internal/util keeps no proxy code and is a leaf again.
Re-point all consumers (registry, doctor, config, auth, cmdutil, update) to
internal/transport. Behavior-preserving: package move + symbol rename + dedup.
Two new tests lock the fail-closed contract (plugin overrides NO_PROXY; malformed
config never falls through to direct egress).
Every failure on the authentication, authorization, and configuration
path now surfaces as a typed structured error instead of an ad-hoc
envelope. Users and scripts that consume CLI output get:
- a fixed nine-category taxonomy on the wire, each mapped to a
stable shell exit code (authentication/authorization/config = 3,
network = 4, internal = 5, policy = 6, confirmation = 10)
- identity-aware detail fields (missing_scopes, requested_scopes,
granted_scopes, console_url, log_id, retryable, hint) carried
uniformly on the envelope
- a single canonical policy envelope at exit 6; the legacy
auth_error carve-out is retired
- per-subtype canonical message + hint that preserves Lark's
diagnostic phrasing and routes recovery to the right actor:
app developer (app_scope_not_applied), user (missing_scope,
token_scope_insufficient, user_unauthorized), or tenant admin
(app_unavailable, app_disabled)
- wrong app credentials classify as config/invalid_client whether
surfaced by the Open API endpoint (99991543) or the tenant
access-token mint endpoint (10003 / 10014), instead of
collapsing to a transport error or api/unknown
- local shortcut scope preflight emits the same
authorization/missing_scope envelope (identity + deterministic
missing-scope set) used by the post-call permission path, so AI
consumers read the same structured shape from precheck and from
server-returned permission denial
- streaming download/upload failures keep the same network subtype
split (timeout / TLS / DNS / transport) as the non-stream path
instead of collapsing every cause to a generic transport failure
- console_url is carried only on the bot-perspective
app_scope_not_applied envelope (where the recovery action is
"developer applies the scope at the developer console"); the
user-perspective missing_scope envelope drops the field, since
the only actionable user recovery is `lark-cli auth login --scope`
and pointing an end user at a console they cannot modify is
misleading
- bind workflows (Hermes / OpenClaw / lark-channel) flatten dynamic
Type tags to wire 'config' with the original module name kept
as a metric label
All 10 typed errors are cause-bearing, nil-safe on .Error() and
.Unwrap(), and defensively clone slice setter inputs. Four lint
rules (CheckNilSafeError / CheckBuilderImmutable / CheckUnwrapSymmetry
/ CheckBuildAPIErrorArms) lock these invariants on migrated paths.
* feat(platform): support multiple policy rules per plugin
Extend the command policy framework from single-Rule to multi-Rule
semantics. A plugin (or policy.yml) may now contribute several scoped
Rules; the engine combines them with OR -- a command is allowed when it
satisfies every axis of at least one rule. This lets one integration
apply different risk ceilings and identity restrictions to different
command groups.
The cross-plugin fail-closed boundary is preserved: two distinct plugins
both calling Restrict still aborts startup (multiple_restrict_plugins).
Single-Rule behaviour is fully backward compatible -- the rejection
reason_code / rule_name / envelope shape are byte-for-byte unchanged;
multi-rule rejection surfaces the aggregate reason_code no_matching_rule.
- engine: New keeps single-rule compat, add NewSet for OR over rules
- resolver: dedupe by owner (one plugin may contribute many rules),
return []*Rule; yaml gains a top-level rules: list
- registrar/builder/staging: Restrict may be called more than once;
retire the double_restrict error
- config policy show / config plugins show: emit a rules array
- inventory: PluginEntry.Rules is now a slice (fixes last-rule-wins
overwrite when a plugin contributes multiple rules)
* fix(platform): clone rules in Builder.Restrict and inventory snapshot
Address review feedback. Builder.Restrict stored the caller's *Rule
directly, so reusing and mutating one Rule object across multiple
Restrict calls collapsed entries to the last mutation; clone the rule and
its slices on append, mirroring the staging registrar.
BuildInventory likewise reused the source Allow/Deny/Identities slices;
copy them when building the RuleView snapshot instead of relying on
cloneInventory downstream.
Add a regression test: reusing and mutating one Rule across two Restrict
calls now yields two independent rules.
* fix(platform): skip yaml when a plugin owns policy; reject empty rules list
Two policy-config robustness fixes from review:
- A malformed ~/.lark-cli/policy.yml could abort a plugin-governed
binary. applyUserPolicyPruning read yaml before resolving, and
build.go fail-closes on any policy error when a plugin is present.
Plugin rules shadow yaml anyway, so skip reading yaml entirely when a
plugin contributed rules -- an unrelated broken file on the user's
machine can no longer lock the CLI.
- A present-but-empty "rules: []" collapsed to a single all-zero Rule
that allows every annotated command ("looks like policy, enforces
almost nothing"). yaml.Parse now distinguishes absent from
present-but-empty (Rules is a pointer) and rejects the empty list.
Add regression tests for both.
Fix 3 occurrences of --minute-token (singular) to --minute-tokens
(plural) in lark-vc-recording.md to match the actual CLI flag
definition in minutes_download.go.
The size==1 (64-bit "largesize") branch of all three MP4 box walkers
(findMP4Box, readMp4DurationBytes, readMp4Duration) set boxEnd to the raw
largesize instead of offset+largesize — even though the 32-bit branch right
below correctly uses offset+size. Two consequences:
- Correctness: for any MP4 that carries a 64-bit box size at a non-zero
offset, the box walk is computed from the wrong end, so the moov/mvhd
lookup is truncated and the media duration is silently lost.
- Robustness/security (CWE-190): the unguarded uint64->int(64) conversion of
a largesize with the high bit set yields a negative boxEnd. The in-memory
walkers then assign it to offset and feed it back as a slice index
(data[offset:]), panicking with "slice bounds out of range" and crashing
the CLI on a crafted or corrupt MP4. This is reachable via URL-sourced IM
media, whose bytes the caller does not control.
Fix: compute boxEnd as offset+largesize (matching the 32-bit branch) and
reject largesize values smaller than the 16-byte header or larger than the
remaining input. Malformed media now honours the parsers' best-effort
contract by returning 0/-1 instead of panicking, and the bounds guarantee
the conversion can no longer overflow.
Add regression tests covering both the overflow (must not panic) and a
64-bit box at a non-zero offset (must walk correctly).
Two parity edge cases from a follow-up audit:
- []<named string> element types (type ID string; field []ID) passed parse
validation but panicked at bind via reflect.Convert([]string -> []ID). Build
the slice element-by-element with SetString (stringsToSlice) so plain
[]string, named slice types and named element types all bind correctly.
- nil Execute: mountTyped now skips mounting (matching legacy
Shortcut.MountWithContext) instead of mounting a command that errors only at
invocation, keeping the command tree identical after migration.
Adds binder_namedslice_nilexec_test.go (4 cases).
Add a new --types flag (string_slice; values from {group, p2p}) to
+chat-list, backed by the new GET /open-apis/im/v1/chats `types` query
parameter. Accepts CSV (--types group,p2p) and repeated-flag forms
(--types group --types p2p).
Defaults to groups-only (backward compatible). Under user identity,
p2p single chats appear with chat_mode="p2p" plus p2p_target_type /
p2p_target_id fields. Under bot identity:
- --types=p2p alone is rejected at validation
- --types=p2p,group is silently downgraded to types=group (no runtime
notice; skill docs document this contract)
Updates Shortcut.Description, lark-im SKILL.md (frontmatter trigger
+ shortcut table row), and the chat-list reference doc with command
examples, the new parameter, output field documentation, and a
dedicated "Bot identity and p2p" section.
Change-Id: I637ce23b3c6ce4ec350f0ac26dbac8120761bb71
Close the last legacy/typed capability gaps so migration never downgrades:
- []string fields: registerLeaf/bindLeaf gain a Slice case. Default maps to
cobra StringSlice (comma-separated, repeatable); `split:"none"` opts into
StringArray (repeatable, no comma split). Non-[]string slices and split on
non-slice fields error at Mount time. bucketLeafValue handles slices in
OneOf/group too.
- per-flag hidden: `hidden:"true"` tag → fieldSpec.Hidden → MarkHidden;
typed help skips hidden flags (parity with legacy Flag.Hidden).
- @file/stdin discoverability: typed help now appends a (supports @file /
- for stdin) hint for input-tagged flags, matching legacy.
Adds binder_slice_hidden_test.go (11 cases).
Bring TypedShortcut[T] to parity with legacy common.Shortcut on two flag
capabilities that were silently missing on the typed path:
- @file / stdin: declare `input:"file,stdin"` on an Args field; the binder
resolves @path (file) and - (stdin) before binding, recursing into OneOf
buckets and groups. resolveInputForFlag is extracted from the legacy
resolveInputFlags so both paths behave identically.
- enum: registerLeaf registers shell completion and typed help renders the
candidate list, matching legacy. enum/input tags on non-string fields now
error at Mount time instead of being silently skipped at runtime.
Legacy behavior unchanged. Adds binder_input_enum_test.go (11 cases).
UserOpenIDList ([]string) had zero production callers and cannot be bound
as a typed flag field: the binder only handles string/bool/int scalars, so
a []string field panics at bind time (reflect.Value.Convert: string cannot
convert to []string). Remove the type, its ParseUserOpenIDList helper and
the test instead of special-casing slice binding for a type nobody uses.
* fix(install): detect curl version before using --ssl-revoke-best-effort
(cherry picked from commit da14737702)
* test(install): cover curl version gate and refactor for testability
Extract the version comparison out of curlSupportsSslRevokeBestEffort()
into a pure isCurlVersionSupported(output), so the >= 7.70.0 logic is unit
testable without spawning curl. Add cases for 7.55.1 / 7.69.0 / 7.70.0 /
8.x plus the unparseable and libcurl-token edge cases (the regex must read
the leading "curl X.Y.Z", not the trailing "libcurl/X.Y.Z").
Memoize the `curl --version` probe: curl's version is invariant for the
install's lifetime while download() runs once per mirror URL, so probe at
most once instead of re-spawning curl on every attempt.
---------
Co-authored-by: EllienTang <146210093+Ellien-Tang@users.noreply.github.com>
Co-authored-by: liangshuo-1 <266696938+liangshuo-1@users.noreply.github.com>
Two issues caught in review of #1132 that the existing tests missed because
they constructed RuntimeContext/CliConfig directly, bypassing the credential
edge where the bug lives.
P1 — Lang dropped at credential boundary
credential.Account had no Lang field, so AccountFromCliConfig and
ToCliConfig silently dropped cfg.Lang. The production Factory builds
CliConfig via acct.ToCliConfig() (factory_default.go Phase 3), which
meant RuntimeContext.Lang() always returned "" in production and
shortcuts/mail/mail_signature.go always fell back to zh_cn — defeating
the whole point of persisting --lang.
Fix: add Lang i18n.Lang to Account and copy it in both directions.
Regression test: TestFullChain_LangSurvivesProductionPath walks the
real path (SaveMultiAppConfig -> DefaultAccountProvider.ResolveAccount
-> ToCliConfig) and asserts Lang survives, so any future field added
to CliConfig forces the same audit.
P2 — priorLang ignored CurrentApp in multi-profile workspaces
priorLang scanned all Apps and returned the first non-empty Lang. If a
user had multiple profiles and the active one disagreed with Apps[0],
a re-bind without --lang would silently inherit the wrong profile's
preference.
Fix: read multi.CurrentAppConfig("").Lang instead.
Regression tests cover CurrentApp wins over Apps[0], single-app
fallback, and malformed bytes.
Change-Id: If7a276605f84f398cec329c2c942b471b4c32749
Maybe[T] solved a real problem (distinguishing "user did not provide this
flag" from "user provided the type's zero value, e.g. --notify=false or
--limit 0"), but it added a framework-specific wrapper that every migrator
had to learn. Two independent dogfood migrations both flagged it as the
guide's biggest cognitive bump.
This change retires Maybe[T] and reuses the existing OneOf pointer
convention at the top level: *T now means "optional with tri-state
semantics — nil iff the user did not set the flag". The rule is the same
whether the *T sits inside a OneOf bucket (Chat *ChatID — variant not
selected) or at the top level of an Args struct (Notify *bool — flag not
given), so users learn one concept instead of two.
Code:
- binder.go: bindLeaf now gates pointer allocation behind
cmd.Flags().Changed(name); previously a *T top-level pointer was always
allocated, defeating the "nil = not given" semantic. fieldSpec loses the
IsMaybe field; parseFieldSpec drops Maybe-shape detection; registerFlags
/ bindFlags drop their IsMaybe branches; the bindMaybe helper is deleted.
- protocol.go: Maybe[T] type removed.
- protocol_test.go: TestMaybe_UnsetVsZero removed.
- binder_coverage_test.go: bindMaybe tests replaced with two
TestBindLeaf_Ptr* tests covering the new "*T top-level pointer" contract
— nil when absent, non-nil with the user's value (including the tri-state
case --notify=false) when explicitly set.
Verified: go build ./..., go test ./shortcuts/common/..., gofmt clean, and
`go run golang.org/x/tools/cmd/deadcode@v0.31.0 -test ./...` vs origin/main
shows zero new dead code.
Follow-up to #1095. The reactions auto-enrichment shipped, but on busy chats the strictly-serial per-resource fetches in EnrichReactions, ExpandThreadReplies, and merge_forward expansion stretched the command's wall time above 14s — enough that wrapper agents (30–60s wall-clock budgets) saw timeouts even though the CLI itself never errored. This PR parallelizes all three with the same bounded-concurrency pattern, batches the follow-up contact-API sender resolution so it doesn't fan back out into a serial stall, and fixes two correctness bugs that surfaced during review. Scoped to convert_lib/{reactions,thread,merge,content_convert}.go + tests + the 4 shortcut Execute hooks + the reference doc.
Change-Id: I0206d10ad204382170bd42aec67f82578923736e
The six TestDriveInspectExecute_* tests set
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) but build the CLI via
cmdutil.TestFactory(t, cfg), which provides an in-memory config closure
(func() (*core.CliConfig, error) { return config, nil }) and never reads the
filesystem. Per the repo learning from PR #343, this env var should only be
set for tests exercising the real NewDefault() factory path. None of these
tests use NewDefault(), so the calls are dead and removed.
No behavior change; all TestDriveInspect* tests still pass.
Co-authored-by: kyalpha313 <kyalpha313@users.noreply.github.com>
Three fixes surfaced by a dogfood migration of an existing legacy shortcut
to the TypedShortcut framework.
1. Required flags rendered under OPTIONAL
The typed help renderer had no notion of required vs optional and lumped
every top-level leaf into a single OPTIONAL section. Add renderRequiredSection
that lists fields whose `required` tag is set under a REQUIRED: header; the
updated renderOptionalSection skips those so each leaf appears in exactly
one section.
2. Default values silently dropped
Fields tagged `default:"x"` showed no `(default "x")` suffix, unlike cobra's
legacy default help. Add a formatLeafLine helper that appends `(default "x")`
whenever fieldSpec.DefaultValue is non-empty; the REQUIRED, OPTIONAL, and
CHOOSE ONE bucket renderers all reuse it for consistent information density.
3. TypedShortcut.Mount was missing
Legacy common.Shortcut exposes BOTH .Mount(parent, factory) and
.MountWithContext. The framework dropped .Mount in 8d8acb82 to satisfy
deadcode, but a dogfood migration revealed the asymmetry forces every
migrating shortcut's tests to also switch to MountWithContext. Restore the
3-line Mount convenience method (delegates to MountWithContext with a
background context) and add a unit test exercising it directly — the test
doubles as API documentation and keeps deadcode happy.
Tests added:
- TestTypedShortcut_Mount: verifies the legacy-shaped Mount(parent, factory)
call still wires the subcommand
- TestTypedHelp_RequiredSectionAndDefaults: asserts REQUIRED comes before
OPTIONAL, --limit lands in REQUIRED not OPTIONAL, `(default "20")` appears
for --page-size, and a plain --verbose carries no default suffix
Verified locally: go build ./..., go test ./shortcuts/common/..., gofmt -l,
and `go run golang.org/x/tools/cmd/deadcode@v0.31.0 -test ./...` vs
origin/main — all pass with zero new dead code.
CI fast-gate's `gofmt -l .` flagged an alignment issue in contentBucket:
gofmt aligns struct field tags to the longest *named* tag-bearing field
in the group; my hand-written extra spaces in `Text *string \`flag:"ct"\``
got normalised to a single space.
Behaviour-neutral; tests still pass.
The previous checkOneOf required group / bucket variants inside a OneOf
to mark exactly one inner field with `oneof_trigger:"true"`, so the
framework could identify "the flag that signals the variant was selected".
This forces the Args writer to think about an implementation detail and
produces a misleading shortcut_oneof_missing when a user supplies only a
companion field (e.g. --video-cover without --video) — the user actually
attempted that variant, but the framework couldn't see it.
Replace the explicit trigger with implicit detection: any inner flag of a
group / bucket variant that is Changed counts as attempting that variant.
The follow-up checkGroup catches the partial-fill case and surfaces a
shortcut_group_incomplete pointing at the missing flag, which is strictly
more useful than the prior oneof_missing.
Code changes:
- binder.go: drop fieldSpec.OneOfTrig, drop parseFieldSpec's oneof_trigger
tag handling, rewrite checkOneOf with the inferred-attempt logic, delete
the isTrigger helper (now inlined and simplified).
- typed_help.go: renderFlagsInBucket flattens all inner flags to the
parent's indent (no more "trigger vs companion" visual distinction,
matching the framework's new semantics).
- binder_test.go: add three behavioural tests covering the new contract:
(1) companion alone surfaces group_incomplete (not oneof_missing),
(2) full group passes,
(3) simple variant + group companion both attempted yields oneof_multiple.
Verified: go build ./..., go test ./shortcuts/common/...,
`go run golang.org/x/tools/cmd/deadcode@v0.31.0 -test ./...` against HEAD
vs origin/main yields zero new dead code.
Net: -1 tag, -1 field on fieldSpec, -1 helper function, simpler writer
ergonomics, more accurate error messages.
bindFlags and bindBuckets only populated top-level leaves and OneOf
buckets; a top-level group sub-struct (a regular nested struct without
OneOf() marker) had its inner flags registered and group-completeness
checked, but its field values were never written back into the Args
struct — Execute would read empty values from args.Group.X.
Add bindGroups as the top-level counterpart to bindBuckets for IsGroup
fields, and invoke it from mountTyped's Validate closure right after
bindBuckets. Behavior mirrors bindBuckets / bindBucketInner:
- Value-type top-level group: always populated; inner fields receive
cobra flag values (including defaults).
- Pointer-type top-level group: allocated iff at least one inner flag
was Changed, so a nil group still signals "user did not engage this
group" while a non-nil group means "the user opted into it".
Tests cover four cases: value group with explicit flags, value group
with defaults applied, pointer group allocated when an inner flag is
set, pointer group left nil when nothing is set.
Verified: go build ./..., go test ./shortcuts/common/..., and
`go run golang.org/x/tools/cmd/deadcode@v0.31.0 -test ./...` against
HEAD vs origin/main yields zero new dead code.
This closes the previously-documented limitation that group sub-structs
had to be nested inside an OneOf bucket to actually bind values.
