Skip to content

✨ Add --no-secret-detection to xmd run and xmd test - #330

Merged
taras merged 1 commit into
mainfrom
feat/issue-199-cli-opt-out
Aug 5, 2026
Merged

✨ Add --no-secret-detection to xmd run and xmd test#330
taras merged 1 commit into
mainfrom
feat/issue-199-cli-opt-out

Conversation

@taras

@taras taras commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Why

#329 made secret detection the default policy of execute() with a host-only opt-out.
xmd inherited the default because it calls execute(), but had no way to turn it off
and said nothing when it was off. This is #199's CLI slice. #199 stays open for
cross-backend/runtime certification and website documentation.

What changes

Before: xmd always scanned, and no flag could change it.

After: --no-secret-detection disables detection for the whole invocation, on xmd run,
the default form that omits run, inline -e, and xmd test against a file or a
directory. Detection stays on when the option is absent. A disabled invocation writes one
line to stderr before the first document runs:

WARNING: secret detection is disabled; credentials may be persisted.

How it works

runConfig/testConfig --no-secret-detection → DocumentConfig.secretDetection
  → execute({ secretDetection }) in runDocument
runXmd "run" case / test() → announceSecretDetection() once per invocation

The option is an ordinary boolean field. Configliere negates boolean switches natively —
its README documents --debug / --no-debug — so a secretDetection field makes
--no-secret-detection resolve to false with no argv reading of our own.

DocumentConfig gains the field; TestConfig already extends
Omit<DocumentConfig, "root"> and both command cases already spread ...config, so the
value reaches every document without new plumbing.

The warning is emitted at the command boundary — in runXmd's run case and once inside
test() — never in runDocument. That is what makes a directory of fifty documents warn
once rather than fifty times, and what keeps --help and --version silent, since both
return before either call site.

What I verified about the parser first

Probed configliere@0.4.0 in the workspace rather than assuming, and two results shaped
the implementation:

Input Resolved
(absent) true
--no-secret-detection false
--secret-detection true
--secret-detection=false true
--no-secret-detection=true true
  1. Aliases break the negation. With aliases: ["--no-secret-detection"], that flag
    resolves to true — an opt-out that silently does nothing. The field takes no
    aliases, and this is one of the mutations.
  2. The = form resolves back to the default, i.e. to enabled — the opposite of
    what the caller asked, in silence, on the option that decides whether credentials may
    be persisted. It is now refused with an error naming the spelling that works. Making
    it work instead would have added a second spelling to a surface that deliberately has
    one.
  3. Help renders the positive switch only. Configliere has no negative-form rendering,
    so the description carries --no-secret-detection onto both help pages.

What must stay true

  • The resolved value reaches every document — enforced by threading it through
    DocumentConfig to the single execute() call; checked by SD2–SD6 and SD11, and by
    the drop-from-execute mutation.
  • Absent means enabled — enforced by field.default(true); checked by SD1, SD3–SD6,
    SD10, and the default-disabled mutation.
  • One warning per invocation — enforced by emitting at the command boundary; checked
    by SD7 over a two-document directory and the warn-per-document mutation.
  • The warning is stderr's — checked by SD8 and the warn-on-stdout mutation.
  • One spelling disables detection — checked by SD12 and the accept-value-form and
    alias-negation mutations.

How to verify it

Canaries are assembled at run time, so no usable-looking literal enters the repository,
and no test reads an environment variable, Git credential, or user configuration. Tests
shell out through runCli, so exit status and the two streams are observed separately
and TTY-independently.

packages/cli/tests/secret-detection-cli.test.ts — SD1 to SD12:

  • SD1/SD2 — xmd run refuses a credential-shaped document by default, and runs it under
    the opt-out.
  • SD3/SD4 — the implicit and inline -e forms honour the option in both directions.
  • SD5/SD6 — xmd test on a file and on a directory, both directions.
  • SD7 — a two-document directory warns exactly once, asserted by counting, with both
    documents proven to have run.
  • SD8 — the warning is on stderr, absent from stdout, and carries no canary.
  • SD9 — both help pages list --no-secret-detection.
  • SD10/SD11 — the pair that makes this more than an exit-code test. With detection
    on, --journal is written and the offending event is absent from the file; with the
    opt-out, the same document writes that event. The option changed what persisted, not
    merely whether an error was printed.
  • SD12 — --secret-detection=false and --no-secret-detection=true are refused.

Mutation evidence

Applied to a file copy of the implementation, focused suite run, reddening tests
recorded, then restored. All six killed:

Mutation Reddens
drop secretDetection from the execute() call 7, incl. SD11 (journal)
change the default to disabled 5, incl. SD10 (journal)
warn from runDocument, per document SD7
write the warning to stdout 6, incl. SD8
add --no-secret-detection as an alias 8 — the opt-out goes inert
accept --secret-detection=false SD12

Compiled-binary coverage

A new step in the smoke job runs ./dist/xmd for all three behaviours — default-on
refusal, the opt-out with the warning counted exactly once, and the refused value form —
with the canary assembled in the step. Run locally against a real deno task build
binary as well; all three pass.

Local gates

  • deno task fmt / lint — 0 errors (1097 pre-existing warnings, unchanged)
  • deno task check — clean
  • deno task test351 passed, 0 failed
  • deno task check:jsrSuccess Dry run complete
  • pnpm exec tsc --project tsconfig.node.json --noEmit — clean
  • deno task build + the compiled opt-out smoke — pass
  • git diff --check — clean; packages/cli/src/node.ts still 755 in tree and index

Scope

Included

  • --no-secret-detection on run and test, threaded to execute().
  • The once-per-invocation stderr warning.
  • Refusal of the = value form.
  • Help text on both commands, the specification, and compiled-binary smoke coverage.

Intentionally unchanged

Risks and limitations

  • Disabling detection is exactly as dangerous as it sounds, which is what the warning is
    for. It is host-only: no document, prop, or frontmatter can reach it.
  • The refusal of --secret-detection=false is a new error for a form that previously
    parsed silently. It never worked as written — it resolved to enabled — so nothing that
    relied on it was getting what it asked for.

Scope confirmation

  • Every changed file supports the purpose described above.
  • Unrelated cleanup and formatting changes are excluded.
  • Generated or mechanical changes are clearly identified — there are none.
  • The description matches the final diff and test results.

`xmd` inherited default-on detection from `execute()` but had no way to
turn it off and said nothing when it was off. Both commands now carry
`--no-secret-detection`, the resolved value reaches every document each
form runs, and a disabled invocation says so once.

The option is an ordinary boolean field, because that is what configliere
negates: `--no-secret-detection` resolves a `secretDetection` field to
false with no argv reading of our own. It deliberately takes no aliases —
declaring the negative spelling as one makes the parser read it as the
positive switch, and the opt-out silently stops working.

`--secret-detection=false` is refused rather than obeyed or ignored. The
parser resolves an `=` form on either spelling back to the default, so it
would read as *enabled* — the opposite of what the caller asked for, in
silence, on the option that decides whether credentials may be persisted.
The error names the one spelling that turns detection off.

The warning is written at the command boundary rather than in
runDocument, so testing a directory of fifty documents warns once for the
invocation instead of once per document. Help and version requests execute
nothing and warn not at all.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR #330: ✨ Add --no-secret-detection to xmd run and xmd test

4 files, +371 / -1

Scope

✅ PR scope looks good.

Structural

✅ No structural bloat detected.

Slop

✅ Slop indicators look low.

Static Analysis

✅ Oxlint found no issues.

Correctness

FILE: packages/cli/src/node.ts
PATTERN: Duplicate field declaration
CONCERN: The secretDetection field is declared twice in the configuration, leading to redundancy.
QUESTION: Why is secretDetection added twice in the config? Remove the duplicate.

FILE: packages/cli/src/node.ts
PATTERN: Redundant constant declaration
CONCERN: SECRET_DETECTION_OPTION and NEGATED_SECRET_DETECTION are defined but only used in a duplicated context.
QUESTION: Are these constants necessary, or can they be removed to reduce noise?

FILE: packages/cli/tests/secret-detection-cli.test.ts
PATTERN: Superfluous test helper
CONCERN: useWorkspace creates a temporary directory but could be simplified or removed if not critical.
QUESTION: Is useWorkspace essential, or can its logic be inlined to avoid abstraction?

@taras

taras commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

Checked all three findings against the code at c3f8a71. None of them hold, and each is answered with evidence below.

All three name packages/cli/src/node.ts. That file is the 40-line Node entrypoint — it installs two API.Env providers and calls runXmd. It contains no configuration fields, no constants, and no tests:

$ grep -n "secretDetection\|SECRET_DETECTION\|useWorkspace" packages/cli/src/node.ts
(no matches)

The code being described lives in packages/cli/src/cli.ts and packages/cli/tests/secret-detection-cli.test.ts.


1. "Duplicate field declaration" — two commands, not a duplicate

The two occurrences are one line each, in two different configliere command configs, and both reference the same constant:

packages/cli/src/cli.ts:159    secretDetection: SECRET_DETECTION_FIELD,   # inside runConfig
packages/cli/src/cli.ts:189    secretDetection: SECRET_DETECTION_FIELD,   # inside testConfig

SECRET_DETECTION_FIELD is the deduplication — the description, type and default are declared once. xmd run and xmd test are separate command configs, so an option has to be declared on each to exist on each.

I applied the suggested removal to see what it does. Removing the testConfig line:

Test Result
SD5 — xmd test <file> is default-on and honours the opt-out FAILED
SD6 — xmd test <directory> is default-on and honours the opt-out FAILED
SD9 — both help pages list the option FAILED

xmd test in file and directory form is part of this slice's deliverable, so the removal deletes a third of the feature.

Worth noting: SD7 still passed under that mutation, because with the field gone config.secretDetection is undefined and !undefined warns anyway. The change would have half-broken the feature in a way a thinner suite would have missed.

2. "Redundant constant declaration" — four uses across two contexts

packages/cli/src/cli.ts:100        `disable with ${NEGATED_SECRET_DETECTION}`,      # help description
packages/cli/src/cli.ts:290        arg.startsWith(`${SECRET_DETECTION_OPTION}=`)    # grammar check
packages/cli/src/cli.ts:291        arg.startsWith(`${NEGATED_SECRET_DETECTION}=`)   # grammar check
packages/cli/src/cli.ts:295        `and \`${NEGATED_SECRET_DETECTION}\` is what turns it off`   # error text

They keep the help text, the parser rejection, and the error message agreeing on the exact spelling — which is the subject of this PR, since configliere renders only the positive switch in help while the negative spelling is what a caller writes.

3. "Superfluous test helper" — used by 11 of 12 tests

useWorkspace does five things per test: mkdtemp, ensure(() => rm(...)) for scoped cleanup, and three fixture writes. Inlining it would repeat that eleven times and drop the cleanup, leaving a temp directory behind on every run.

The two extra .test.md documents are not incidental: SD7 proves the warning appears exactly once across a multi-document directory run, and asserts both documents ran. A single-document fixture would make that assertion vacuous.


The working tree is unchanged and the suite is green (12/12). Given the file attribution is wrong on all three, this review may have run against a different or stale diff — worth re-running against c3f8a71360e61457bb1d702b4d96334aec1f428e.

@taras
taras merged commit 51224dc into main Aug 5, 2026
10 checks passed
@taras
taras deleted the feat/issue-199-cli-opt-out branch August 5, 2026 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant