Skip to content

fix(cli): propagate PDF/Text helper failures to process exit status - #538

Merged
SunnyHaze merged 5 commits into
OpenDCAI:mainfrom
rikkurea-boop:publish/p08-pr-g1-cli-failure-propagation
Oct 12, 2026
Merged

SunnyHaze merged 5 commits into
OpenDCAI:mainfrom
rikkurea-boop:publish/p08-pr-g1-cli-failure-propagation

Conversation

@rikkurea-boop

@rikkurea-boop rikkurea-boop commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Make the public pdf2model and text2model Typer commands return a nonzero process exit status when their helper reports operational failure via False.

This PR also fixes the existing text2model train CLI/helper mismatch where the public command passes input_dir but cli_text2model_train() did not accept it.

Problem

The four public PDF/Text init/train wrappers call helpers that use boolean success/failure contracts. Before this patch, a helper could print a failure, return False, and still leave the public CLI process at exit status 0.

This is especially unsafe for shell, CI, and orchestration callers that use the process exit code as the workflow success signal.

There was also a pre-existing issue in the Text2Model train path: the public CLI passes input_dir to cli_text2model_train(), but the helper did not accept that argument and instead hard-coded input_dir = "./". As a result, text2model train could fail with TypeError before the helper body ran.

Root cause

The wrappers invoked the helper but ignored its boolean return value. Exceptions already mapped to nonzero exits, but normal False returns were not interpreted at the public CLI boundary.

For Text2Model, the public CLI and helper signature were also out of sync for input_dir.

Fix

  • capture the helper result in each PDF/Text init/train wrapper;
  • preserve the existing exception handling;
  • after the exception block, raise typer.Exit(code=1) when the helper result is false;
  • keep successful helper behavior unchanged;
  • keep the intentional typer.Exit outside the broad exception block to avoid duplicate error reporting;
  • add input_dir to cli_text2model_train() and remove the internal hard-coded input_dir = "./" assignment.

Regression tests

Added test/cpu_only/test_cli_helper_failure_exit.py covering:

  • helper False → nonzero public exit;
  • helper True → exit 0;
  • helper exception → nonzero exit;
  • PDF init/train and Text init/train;
  • no duplicate wrapper error on helper-returned failure.

The helper test doubles now use explicit compatible signatures instead of a permissive variadic helper.

Added test/cpu_only/test_text2model_input_dir_boundary.py to exercise the public Typer CLI → real cli_text2model_train() boundary and verify that the helper accepts and uses the public input_dir argument.

Targeted validation:

  • helper outcome matrix: 12/12 passed;
  • Text2Model input-dir boundary regression: 2/2 passed;
  • git diff --check: passed;
  • py_compile: passed;
  • upstream Python tests CI: passed.

Additional validation

Repeated controlled validation covered the four public commands across helper True, False, and exception outcomes with no counterexample.

The Text2Model boundary regression also confirms that the helper body is reached after the signature fix and that the existing default behavior remains compatible.

Limitations

This PR is intentionally scoped to public CLI failure propagation and the Text2Model input_dir signature mismatch.

It does not change detector behavior, PDF KBC/VQA wiring, broader cache-path propagation, stale-artifact handling, or the rest of the Text2Model data-preparation path.

The available environment did not run real PDF/Text model training E2E.

@SunnyHaze

Copy link
Copy Markdown
Collaborator

Hi, thanks for your contribution. Could we include the fix from 27d58c5 in PR #538, or link a follow-up before merging? The CLI exposes input_dir, but cli_text2model_train does not accept it, so every text2model train invocation currently fails with TypeError before the helper runs. This predates PR #538, but it makes the command unusable. Please add the parameter and regression coverage, or track the fix explicitly.

@rikkurea-boop

Copy link
Copy Markdown
Contributor Author

Thanks — I reproduced this on the previous #538 head.

The text2model train command was passing input_dir to the real cli_text2model_train, whose signature did not accept it, so it failed with an unexpected-keyword TypeError before the helper body ran.

I've added the minimal input_dir fix, added a regression that crosses the public Typer CLI -> real helper boundary, and tightened the previous test doubles so they no longer hide signature mismatches.

The updated PR CI is passing. Broader Text2Model cache/runtime-path work will remain a separate follow-up.

@SunnyHaze

Copy link
Copy Markdown
Collaborator

LGTM, sincerely thanks!

@SunnyHaze
SunnyHaze merged commit 799dc02 into OpenDCAI:main Oct 12, 2026
6 checks passed
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.

2 participants