Skip to content

test: add unit tests, fix issues, and align argument spelling - #45

Merged
araujof merged 11 commits into
mainfrom
hl/fix_pr
Sep 10, 2026
Merged

araujof merged 11 commits into
mainfrom
hl/fix_pr

Conversation

@dhl123

@dhl123 dhl123 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Summary

A cleanup and bug-fix pass over the test-generation pipeline, and test scripts creation, plus the unit and integration test suites.

Closes

#12 #46 #49

Changes

1. args-migration.

  • Changed arguments in docs/policy_creation.md, docs/test_generation.md, policy_cross_validation.mdREADME.md, and the two docs guides to args for cpex compatibility. (problems left in last PR)

2. agent's default value changes in promptfoo config.

  • Changed the default agent port from 8000 to 9000 in promptfooconfig.yaml files.

3. made adjusts to cpex hr-agent example

  • Delete guidance that cannot be enforced by opa (requires approval etc).
  • Added a readme file for running hr-agent example.

4. delete usage of top-k

  • We changed all openai client to aws. aws models only take either temp or topk, so we deleted topk.

5. null values handling in convert_test_case.py.

  • _convert_var in src/smith/test_generation/convert_test_case.py called int(None) / float(None) when a generated case supplied null for a numeric system variable, raising TypeError. Most often triggered by adversarial Promptfoo cases that omit an integer field (e.g. queries_this_session). Now returns None, leaving the field absent so OPA treats it as unset.

6. remove any previously-appended tool-parameter block in promptfoo config generation (problems left in last PR)

  • Promptfoo config updates weren't idempotent. generate_promptfoo_config.py re-appended the tool-parameter block on every run, so the prompt grew each update. Added a sentinel marker and a strip step so the block is replaced instead of duplicated.

Problem identified when writing unit and integration tests.

7. Missing env setup values

  • Added variables the CLI reads but .env_template omitted. Added variables: TEST_OUTPUT_DIR, TEST_PATH, MCP_URL, TOP_P, MAX_LLM_CALLS.

8. Two modules read the environment themselves

  • attack.py and attack_promptfoo.py called load_dotenv()/os.getenv inside the function the CLI invokes. Both now take the value as a parameter, resolved in cli.py with every other path. Three functions gained required parameters; cli.py is their only caller.

9. AST graph indices inherited across calls

  • node ids came from the size of two module-level dicts that were never reset, so the same policy parsed twice produced different graphs. init_graph now clears them first.

10. convert_test_coverage.py ran its whole pipeline at import

  • and crashed without configuration, which broke the policy_testing unit lane. Environment resolution and I/O moved into main(); the transformation logic extracted into importable functions. Output byte-identical, so the frozen coverage numbers still hold.

11. apply_cross_validate crashed when the destination didn't exist

  • shutil.move doesn't create directories, so promoting a case into an absent allow/ raised FileNotFoundError mid-loop, leaving the tree partially applied. Added os.makedirs(..., exist_ok=True) before each move.

12. Whitespace-only lines now dropped in decompose.py

  • remove_empty_line tested len(strr) != 0, so a line of spaces survived and became a rule, spending an LLM call to produce a meaningless record. Now judged with .strip(); line content untouched.

13. Discarded exit code hid Regal failures

  • subprocess.run ran without check=True and its result was thrown away, and the shell pipeline reported sed's status anyway. So a missing regal binary produced an empty report and a successful flag: the same answer as a clean policy. Regal is now invoked directly and its exit code classified (0 clean, 3 violations, anything else a failure). check=True would have been wrong — it raises on 3, the normal result. Output byte-identical wherever Regal actually runs.

Tests

  • To run the unit tests: make unit
  • To run the integration test: make integration

dhl123 added 2 commits August 21, 2026 14:34
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@dhl123 dhl123 changed the title Fix: Fix tool parms' duplication in pormptfoo config file auto generation, change remaining arguments to args Fix: Fix duplication in pormptfoo config file auto generation, change remaining arguments to args Aug 21, 2026
@dhl123 dhl123 linked an issue Aug 21, 2026 that may be closed by this pull request
@dhl123
dhl123 marked this pull request as ready for review August 21, 2026 19:19
@dhl123
dhl123 requested a review from araujof as a code owner August 21, 2026 19:19
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@dhl123
dhl123 marked this pull request as draft September 2, 2026 20:01
@dhl123 dhl123 changed the title Fix: Fix duplication in pormptfoo config file auto generation, change remaining arguments to args Tests + Fix: Unit tests, Fix duplication in pormptfoo config file auto generation, change remaining arguments to args Sep 2, 2026
@dhl123 dhl123 linked an issue Sep 2, 2026 that may be closed by this pull request
5 tasks
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@dhl123 dhl123 changed the title Tests + Fix: Unit tests, Fix duplication in pormptfoo config file auto generation, change remaining arguments to args [Tests + Fix]: Added unit tests, Fixed duplication in promptfoo config file generation, changed remaining "arguments" to args for CPEX Sep 4, 2026
@dhl123 dhl123 linked an issue Sep 9, 2026 that may be closed by this pull request
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@dhl123
dhl123 marked this pull request as ready for review September 9, 2026 17:51
@araujof araujof self-assigned this Sep 9, 2026
@araujof araujof added bug Something isn't working tests labels Sep 9, 2026
@araujof araujof added this to the 0.1.2 milestone Sep 9, 2026
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
Signed-off-by: Hailun Ding <hailun.ding@ibm.com>

@araujof araujof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work! This will be a great improvement to code base stability.

A few things we should address before merging:

  • src/smith/test_generation/convert_test_case.py:16: Returning None does not omit the field; _fill_template writes it as JSON null. OPA treats null differently from a missing field, so generated cases can test the wrong policy outcome. Skip null-valued fields when filling the template.

  • src/smith/test_generation/generate_promptfoo_config.py:249: The duplicate-block fix only recognizes the new marker. Existing configs have unmarked generated blocks, so the next run keeps those blocks and appends another. The old block also remains when the current tool list is empty. Remove legacy blocks and strip generated instructions before checking the current tool list.

  • src/smith/test_generation/variable_extraction.py:73: Using a local filtered copy stops the mutation, but later stages now receive action_list and action_description. case_generation includes them in the system-variable prompt, allowing internal metadata to leak into generated test subjects. Pass a filtered copy to later stages too.

  • Makefile:188: The new unit suite is added to make ci, but the GitHub workflow never runs make ci or make unit. PR checks therefore pass without running the 356 new tests. Add a unit-test job to .github/workflows/ci.yml.

@araujof araujof changed the title [Tests + Fix]: Added unit tests, Fixed duplication in promptfoo config file generation, changed remaining "arguments" to args for CPEX test: add unit tests, fix issues, and align argument spelling Sep 10, 2026
@araujof araujof changed the title test: add unit tests, fix issues, and align argument spelling test: add unit tests, cleanup and fix issues, and align argument spelling Sep 10, 2026
@araujof araujof changed the title test: add unit tests, cleanup and fix issues, and align argument spelling test: add unit tests, fix issues, and align argument spelling Sep 10, 2026
@dhl123

dhl123 commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

Nice work! This will be a great improvement to code base stability.

A few things we should address before merging:

  • src/smith/test_generation/convert_test_case.py:16: Returning None does not omit the field; _fill_template writes it as JSON null. OPA treats null differently from a missing field, so generated cases can test the wrong policy outcome. Skip null-valued fields when filling the template.
  • src/smith/test_generation/generate_promptfoo_config.py:249: The duplicate-block fix only recognizes the new marker. Existing configs have unmarked generated blocks, so the next run keeps those blocks and appends another. The old block also remains when the current tool list is empty. Remove legacy blocks and strip generated instructions before checking the current tool list.
  • src/smith/test_generation/variable_extraction.py:73: Using a local filtered copy stops the mutation, but later stages now receive action_list and action_description. case_generation includes them in the system-variable prompt, allowing internal metadata to leak into generated test subjects. Pass a filtered copy to later stages too.
  • Makefile:188: The new unit suite is added to make ci, but the GitHub workflow never runs make ci or make unit. PR checks therefore pass without running the 356 new tests. Add a unit-test job to .github/workflows/ci.yml.
  • Solved 1,2,4.

  • For 3, the cli --flag test_generation reads system variables, and it was passed to variable_extraction(system variables), case_generation(system variables) independently. In variable_extraction.py, we do not need action_list and description, so we filtered them with a new copy of system variables. But in case_generation.py we need action_list etc, so we do not created a filtered copy.

  • Removed lint policy in Makefile, since our assets/policy.rego should stay empty.

Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@araujof

araujof commented Sep 10, 2026

Copy link
Copy Markdown
Member
  • Removed lint policy in Makefile, since our assets/policy.rego should stay empty.

Should we also remove the job the CI workflow?

rego-lint:
name: Rego lint (Regal)
# Disabled: Regal lint is not enforced in CI. Re-enable by restoring the
# original `if` condition below (drop the leading `false &&`).
if: false && (github.event_name != 'pull_request' || !github.event.pull_request.draft)
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
- name: Install Regal
run: |
curl -fsSL -o regal https://github.com/StyraInc/regal/releases/latest/download/regal_Linux_x86_64
chmod +x regal
sudo mv regal /usr/local/bin/regal
- name: make lint-policy
run: make lint-policy

Signed-off-by: Hailun Ding <hailun.ding@ibm.com>
@araujof
araujof self-requested a review September 10, 2026 23:15

@araujof araujof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@araujof
araujof merged commit b5bfbf2 into main Sep 10, 2026
6 checks passed
@araujof
araujof deleted the hl/fix_pr branch September 10, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working tests

Projects

None yet

2 participants