Skip to content

fix(cli): remove --evaluators, which never affected a run - #91

Merged
kmadan merged 1 commit into
mainfrom
fix/evaluators-flag-no-op
Aug 30, 2026
Merged

fix(cli): remove --evaluators, which never affected a run#91
kmadan merged 1 commit into
mainfrom
fix/evaluators-flag-no-op

Conversation

@kmadan

@kmadan kmadan commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

🚀 Overview

Removes --evaluators from the evaluate subcommand. The flag was accepted and
had no effect on any run. Follow-up to #90, which removed it from the documented
options.

🔍 Why it never worked

aicertify_app_for_policy derives the evaluator set from the RequiredMetrics
the selected policies declare:

# aicertify/api/policy.py:103
evaluators = evaluator_registry.discover_evaluators(metrics)

_run_evaluate accepted an evaluators argument and did not forward it:

return await aicertify_app_for_policy(
    contract=contract, policy_folder=policy_folder,
    output_dir=output_dir, report_format=report_format,
    custom_params=custom_params,      # evaluators not passed
)

The parser accepted the flag, argparse bound it, and the run proceeded
identically.

🎯 Why removed rather than wired through

Restricting the evaluator set below what a policy requires leaves that policy's
metric absent from the OPA input. An absent metric is not a failed one: a
rule that reads it goes undefined and its default stands.

A flag whose plain meaning is "run fewer evaluators" would therefore convert a
policy that should deny into one reporting no obstacle. That is a worse outcome
than the flag doing nothing, and it is the same fail-open class GOPAL's
empty-input test gate exists to catch.

Explicit evaluator selection remains available on evaluate_by_policy
(aicertify/api/policy.py:297) for callers using the API directly.

🎯 Why removed rather than warned

Removing the registration makes the change visible at the point of use:

$ aicertify evaluate --contract c.json --policy eu_ai_act --evaluators Fairness
aicertify: error: unrecognized arguments: --evaluators Fairness
$ echo $?
2

Nothing depended on the flag's effect, because it had none. Only its acceptance
changes.

📜 Changes Made

  • ✅ Dropped the evaluators parameter from _run_evaluate (dead)
  • ✅ Dropped evaluators=args.evaluators at the call site (dead)
  • ✅ Dropped the ev.add_argument("--evaluators", ...) registration

Three deletions, 7 lines. No other behaviour changes.

🎯 How to Test

aicertify evaluate --help | grep -c -- --evaluators     # -> 0
aicertify evaluate --contract c.json --policy eu_ai_act --evaluators X; echo $?   # -> 2

📄 Notes for reviewers

Every flag the parser registers was audited for the same defect. All sixteen
others have a destination that is read somewhere in cli.py; --evaluators was
the only one that did not. --with-llm-metrics on the demo subcommand, its
nearest neighbour, is genuinely wired (cli.py:112).

aicertify_app_for_policy derives the evaluator set from the RequiredMetrics the
selected policies declare: it calls discover_evaluators(metrics) and runs what
that returns. _run_evaluate accepted an evaluators argument and did not forward
it, so a value supplied on the command line reached nothing. The parser
accepted the flag, argparse bound it, and the run proceeded identically.

Wiring it through was considered and rejected. Restricting the evaluator set
below what a policy requires leaves that policy's metric absent from the OPA
input, and an absent metric is not a failed one: a rule that reads it goes
undefined and its default stands. A flag whose plain meaning is "run fewer
evaluators" would therefore turn a policy that should deny into one that
reports no obstacle, which is a worse outcome than the flag doing nothing.
Selecting evaluators explicitly remains available on evaluate_by_policy for
callers using the API directly.

Removing the registration rather than warning on it makes the change visible at
the point of use. `aicertify evaluate --evaluators Fairness` now exits 2 with
"unrecognized arguments" instead of accepting the flag and ignoring it. Nothing
depended on its effect, because it had none; only its acceptance changes.

#90 removed the flag from the documented options at the same time.

Every other flag the parser registers was checked: all sixteen have a
destination that is read. --evaluators was the only one that did not.
@github-actions github-actions Bot added the 🚀 enhancement New feature or request label Aug 30, 2026
@kmadan
kmadan merged commit 15f6f06 into main Aug 30, 2026
7 checks passed
kmadan added a commit that referenced this pull request Aug 31, 2026
… drift

Two things a first-time contributor hits before writing a line.

The three example run.py scripts call OpenAI. DeepEval judges toxicity with an
LLM and activates off the presence of OPENAI_API_KEY, so anyone with a key in
their shell gets billed for running an example, and anyone with an exhausted
quota watches nine retries per interaction scroll past and concludes the
example is broken. The bundled demo already guards against this by popping the
key unless --with-llm-metrics is passed; the examples never got the same
treatment. They do now, opting in through AICERTIFY_WITH_LLM_METRICS=1, which
each example README documents.

customer-support-bot goes from a wall of 429s to zero OpenAI calls and still
produces its report.

The guard has to run before `from aicertify import ...`, because the evaluator
stack reads the key at import time. That puts the import below module-level
code, which ruff flags as E402; the ordering is load-bearing, so it carries a
noqa and a comment saying why rather than being reordered into correctness bug.

Separately, the README flag tables had drifted in both directions:

  --evaluators   removed in #91, still documented in all four translations
  --params       has always existed, documented in none of the five READMEs

Neither shows up in a test, a lint or a build. A reader following the Japanese
README gets `unrecognized arguments` and reasonably concludes the tool is
broken. tests/test_readme_flags.py now asserts every documented flag exists,
across all five languages, and that the English table covers every `evaluate`
flag. Mutation-tested in both directions before committing.

The translations are still missing a --params row. That needs a native speaker
rather than a machine translation nobody can vouch for, so the test does not
demand it and it stays a contribution task.
kmadan added a commit that referenced this pull request Aug 31, 2026
… drift

Two things a first-time contributor hits before writing a line.

The three example run.py scripts call OpenAI. DeepEval judges toxicity with an
LLM and activates off the presence of OPENAI_API_KEY, so anyone with a key in
their shell gets billed for running an example, and anyone with an exhausted
quota watches nine retries per interaction scroll past and concludes the
example is broken. The bundled demo already guards against this by popping the
key unless --with-llm-metrics is passed; the examples never got the same
treatment. They do now, opting in through AICERTIFY_WITH_LLM_METRICS=1, which
each example README documents.

customer-support-bot goes from a wall of 429s to zero OpenAI calls and still
produces its report.

The guard has to run before `from aicertify import ...`, because the evaluator
stack reads the key at import time. That puts the import below module-level
code, which ruff flags as E402; the ordering is load-bearing, so it carries a
noqa and a comment saying why rather than being reordered into correctness bug.

Separately, the README flag tables had drifted in both directions:

  --evaluators   removed in #91, still documented in all four translations
  --params       has always existed, documented in none of the five READMEs

Neither shows up in a test, a lint or a build. A reader following the Japanese
README gets `unrecognized arguments` and reasonably concludes the tool is
broken. tests/test_readme_flags.py now asserts every documented flag exists,
across all five languages, and that the English table covers every `evaluate`
flag. Mutation-tested in both directions before committing.

The translations are still missing a --params row. That needs a native speaker
rather than a machine translation nobody can vouch for, so the test does not
demand it and it stays a contribution task.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚀 enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant