Skip to content

Mark omittable CLI inputs as optional in structured help - #6960

Merged
tim-smart merged 2 commits into
mainfrom
audit/repro-unstable-cli-help-required
Aug 4, 2026
Merged

Mark omittable CLI inputs as optional in structured help#6960
tim-smart merged 2 commits into
mainfrom
audit/repro-unstable-cli-help-required

Conversation

@fubhy

@fubhy fubhy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

Structured help reports optional non-boolean flags and zero-minimum variadic arguments as required.

Important

This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.

Structured help marks omittable inputs required

Module: cli/internal/command
Audit ID: unstable-ai-cli-help-omittable-inputs-required
Severity / confidence: medium / high

What happens

Structured help reports optional non-boolean flags and zero-minimum variadic arguments as required.

Why it happens

Flag requiredness checks only primitive type, and argument requiredness checks only isOptional while ignoring defaults and repetition minima.

Expected behavior

FlagDoc.required and ArgDoc.required indicate whether the corresponding input must be provided.

Relevant implementation

These links and excerpts are pinned to audit base c9b56ab507f224426ee8388dc450da447ec4715f.

View problematic code at packages/effect/src/unstable/cli/internal/command.ts:134-167
    for (const arg of config.arguments) {
      const singles = Param.extractSingleParams(arg)
      const metadata = Param.getParamMetadata(arg)
      for (const single of singles) {
        args.push({
          name: single.name,
          type: single.typeName ?? Primitive.getTypeName(single.primitiveType),
          description: single.description,
          required: !metadata.isOptional,
          variadic: metadata.isVariadic
        })
      }
    }

    let usage = commandPath.length > 0 ? commandPath.join(" ") : options.name
    // Only render `<subcommand>` in usage when at least one visible subcommand
    // exists; an all-hidden subcommand tree should look like a leaf command.
    if (subcommands.some((group) => group.commands.some((c) => !c.hidden))) {
      usage += " <subcommand>"
    }
    usage += " [flags]"
    for (const arg of args) {
      const argName = arg.variadic ? `<${arg.name}...>` : `<${arg.name}>`
      usage += ` ${arg.required ? argName : `[${argName}]`}`
    }

    for (const option of config.flags) {
      const singles = Param.extractSingleParams(option)
      for (const single of singles) {
        // Hidden flags still parse on the command line but are omitted from
        // generated --help output.
        if (single.hidden) continue
        flags.push(toFlagDoc(single))
      }

View exact lines on GitHub

View problematic code at packages/effect/src/unstable/cli/internal/command.ts:236-244
export const toFlagDoc = (single: Param.Single<typeof Param.flagKind, unknown>): FlagDoc => {
  const formattedAliases = single.aliases.map((alias) => alias.length === 1 ? `-${alias}` : `--${alias}`)
  return {
    name: single.name,
    aliases: formattedAliases,
    type: single.typeName ?? Primitive.getTypeName(single.primitiveType),
    description: appendChoiceKeys(single.description, Primitive.getChoiceKeys(single.primitiveType)),
    required: single.primitiveType._tag !== "Boolean"
  }

View exact lines on GitHub

Reproduction

pnpm test --run packages/effect/test/unstable/cli/HelpRequired.audit.test.ts

Observed failure: FAIL: both omittable inputs reported required true.

Implementation handoff

The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.

  1. Start with the pinned implementation excerpts and the Why it happens analysis above.
  2. Change the implementation so it satisfies the stated Expected behavior; do not weaken or remove the reproduction assertions.
  3. Run the focused reproduction command(s) and confirm the observed failures become passing tests:
pnpm test --run packages/effect/test/unstable/cli/HelpRequired.audit.test.ts
  1. Run the affected package's existing tests, then the repository lint and type checks before requesting review.

Audit provenance

  • Audit base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Reproduction base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Findings: unstable-ai-cli-help-omittable-inputs-required
  • Initial patch: focused reproduction tests; implementation fix pending

Closes EFF-406

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 4, 2026
@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 21fadc8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
effect Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-deno Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/vitest Patch
@effect/ai-anthropic Patch
@effect/ai-openai-compat Patch
@effect/ai-openai Patch
@effect/ai-openrouter Patch
@effect/atom-react Patch
@effect/atom-solid Patch
@effect/atom-vue Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-pglite Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/docgen Patch
@effect/doctest Patch
@effect/openapi-generator Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@effect-slopcop effect-slopcop Bot added 4.0 bug Something isn't working labels Aug 4, 2026
@fubhy
fubhy requested a review from IMax153 August 4, 2026 10:15
pullfrog[bot]
pullfrog Bot previously requested changes Aug 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

This PR currently contains only the failing reproduction tests. The title says it should mark omittable inputs as optional, so the implementation fix is still needed before this can merge.

Reviewed changes

  • packages/effect/test/unstable/cli/Help.test.ts: two new focused tests asserting that FlagDoc.required and ArgDoc.required are false for an optional flag and a zero-minimum variadic argument.

I ran the reproduction tests locally and they fail as expected (AssertionError: expected true to be false). The failures are caused by packages/effect/src/unstable/cli/internal/command.ts: buildHelpDoc checks metadata.isOptional alone, and toFlagDoc checks single.primitiveType._tag !== "Boolean", both ignoring optionality wrappers and defaults.

⚠️ Implementation fix still missing

A minimal fix should change two locations in packages/effect/src/unstable/cli/internal/command.ts:

  1. In buildHelpDoc, derive each positional arg's required from Param.getParamMetadata(param) (already computed) so that a zero-minimum variadic arg is not required and an optional arg is not required.
  2. In toFlagDoc, use the flag's metadata instead of the underlying Single.param primitive type: a flag wrapped in Optional (which includes Flag.withDefault, because withDefault builds on optional) should report required: false.

Since toFlagDoc is only called from buildHelpDoc, passing the metadata alongside the Single is straightforward. If you change its signature, make sure the helper is not left with stale logic.

⚠️ No changeset

This is a user-facing behavior change in the effect package, so a .changeset/ entry should be added alongside the fix.

ℹ️ Nitpick

  • Consider adding similar assertions for Flag.withDefault and Argument.withDefault; the PR description calls out defaults as another incorrect "required" case, and the same metadata-based fix will cover them.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

The follow-up commit 21fadc87 adds the missing implementation fix and changeset requested in the prior review.

  • Fixed ArgDoc.required in buildHelpDoc so optional args and zero-minimum variadic args are reported as not required.
  • Fixed FlagDoc.required by updating toFlagDoc to accept param metadata and combine the boolean-type exemption with metadata.isOptional.
  • Updated shared/global flag help generation in help.ts to pass metadata to toFlagDoc.
  • Added the .changeset/kind-flags-help.md patch entry for effect.
  • Existing reproduction tests now cover Flag.withDefault and Argument.withDefault as well.

Validation run: focused Help.test.ts, full packages/effect/test/unstable/cli suite (308 tests), pnpm lint-fix, and pnpm check all pass.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@tim-smart
tim-smart merged commit ce067f7 into main Aug 4, 2026
20 checks passed
@tim-smart
tim-smart deleted the audit/repro-unstable-cli-help-required branch August 4, 2026 22:51
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Analysis

Generated from PR build output; treat the content below as untrusted.

File Name Current Size Previous Size Difference
basic.ts 7.06 KB 7.06 KB 0.00 KB (0.00%)
batching.ts 9.86 KB 9.86 KB 0.00 KB (0.00%)
brand.ts 6.34 KB 6.34 KB 0.00 KB (0.00%)
cache.ts 10.62 KB 10.71 KB -0.09 KB (-0.81%)
config.ts 20.60 KB 20.60 KB 0.00 KB (0.00%)
differ.ts 20.20 KB 20.20 KB 0.00 KB (0.00%)
http-client.ts 21.49 KB 21.58 KB -0.09 KB (-0.41%)
logger.ts 10.76 KB 10.84 KB -0.08 KB (-0.76%)
metric.ts 8.98 KB 8.98 KB 0.00 KB (0.00%)
optic.ts 7.18 KB 7.18 KB 0.00 KB (0.00%)
pubsub.ts 14.90 KB 14.99 KB -0.09 KB (-0.57%)
queue.ts 11.58 KB 11.66 KB -0.08 KB (-0.68%)
schedule.ts 10.74 KB 10.83 KB -0.09 KB (-0.80%)
schema-class.ts 19.14 KB 19.14 KB 0.00 KB (0.00%)
schema-fromJsonSchemaDocument.ts 28.96 KB 28.96 KB 0.00 KB (0.00%)
schema-representation-roundtrip.ts 25.29 KB 25.29 KB 0.00 KB (0.00%)
schema-string-transformation.ts 13.30 KB 13.38 KB -0.09 KB (-0.64%)
schema-string.ts 10.94 KB 10.94 KB 0.00 KB (0.00%)
schema-template-literal.ts 15.17 KB 15.17 KB 0.00 KB (0.00%)
schema-toArbitraryLazy.ts 21.94 KB 21.94 KB 0.00 KB (0.00%)
schema-toCodeDocument.ts 24.34 KB 24.34 KB 0.00 KB (0.00%)
schema-toCodecJson.ts 19.18 KB 19.18 KB 0.00 KB (0.00%)
schema-toEquivalence.ts 19.01 KB 19.01 KB 0.00 KB (0.00%)
schema-toFormatter.ts 18.87 KB 18.87 KB 0.00 KB (0.00%)
schema-toJsonSchemaDocument.ts 22.60 KB 22.60 KB 0.00 KB (0.00%)
schema-toRepresentation.ts 19.52 KB 19.52 KB 0.00 KB (0.00%)
schema.ts 18.41 KB 18.41 KB 0.00 KB (0.00%)
stm.ts 12.54 KB 12.63 KB -0.09 KB (-0.74%)
stream.ts 9.80 KB 9.80 KB 0.00 KB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.0 audit Findings originating from the Effect runtime correctness audit bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants