Exit non-zero from planemo autoupdate when a tool fails to update

[jmchilton]

Fixes #1478.

Root cause

planemo autoupdate already tracked per-tool failures — cmd_autoupdate.cli builds an
exit_codes list and appends EXIT_CODE_GENERIC_FAILURE whenever handle_tool_load_error
fires. It then did:

return coalesce_return_codes(exit_codes, assert_at_least_one=assert_tools)

Click discards a command callback's return value, so that code never reached the process.
Planemo's exit codes travel via ctx.exit(), which raises ExitCodeException for
command_function to turn into sys.exit(). autoupdate is the only command that computes
an exit code and forgets to hand it to ctx.exit() — cmd_lint.py, cmd_shed_lint.py,
cmd_workflow_lint.py, cmd_test.py, cmd_run.py all do. So the accounting was right and
the result was thrown away.

Reproduced exactly as reported: the malformed-XML diagnostics print, "Could not update
<path> due to malformed xml." prints, and the process exits 0.

Changes

1. ctx.exit(coalesce_return_codes(...)) instead of return.
2. An exception out of autoupdate_tool now records a failure. Previously only the
   tool-load-error path counted, so a tool that errored for any other reason printed a red
   error and still scored EXIT_CODE_OK.
3. Workflow targets append EXIT_CODE_OK. Without this, wiring up ctx.exit would make
   planemo autoupdate some_workflow.ga exit 2 — assert_at_least_one was only ever
   satisfied by the tool loop.

Behavior change worth a look

Because assert_at_least_one=assert_tools is now live, pointing autoupdate at a path with
neither tools nor workflows exits EXIT_CODE_NO_SUCH_TARGET (2) rather than 0. That is what
the existing argument always intended and it matches planemo lint (tests/test_lint.py:75),
but it is a visible change for anyone running planemo autoupdate over a directory list in
CI where some directories hold no tools. Happy to drop it to assert_at_least_one=False if
that is too sharp an edge.

Tests

tests/test_cmd_autoupdate.py, both reusing the existing tests/data/repos/bad_invalid_tool_xml
fixture / _isolate() rather than adding new data:

• test_autoupdate_malformed_xml — asserts exit code 1 and the "due to malformed xml" message.
• test_autoupdate_no_targets — pins the exit-2 behavior above.

Both need neither conda nor Galaxy, so they run in the fast tier.

Red-to-green: with the cmd_autoupdate.py change stashed, both fail; restored, both pass.
Also added assert result.exit_code == 0 to test_autoupdate_workflow_unexisting_tool to
guard the workflow-only path against the assert_at_least_one regression described above.

🤖 Generated with Claude Code

[mvdbeek]

Thanks, the malformed-XML fix looks right. One problem before merging: the assert_at_least_one check now takes effect, and it will break the planemo-autoupdate weekly job.

The job runs planemo autoupdate . --skiplist … || errors="…Cannot autoupdate $REPO…" for each repo. If errors ends up non-empty, the job comments on the "Autoupdate errors" issue and runs exit 1. Skipped tools hit continue before an exit code is appended, and skipped workflows are filtered out of workflows. So a repo whose only tool or workflow is on the skip list now exits 2 (EXIT_CODE_NO_SUCH_TARGET); on master it exits 0.

Under the current skip lists, these repos contain nothing but the skipped entry:

• tools-iuc: tools/optitype, tools/interproscan, data_managers/data_manager_interproscan
• bgruening/galaxytools: tools/diff, chemicaltoolbox/autodock_vina/prepare_box
• iwc: fragment-based-docking-scoring, sars-cov-2-pe-illumina-artic-ivar-analysis, sars-cov-2-ont-artic-variant-calling

To reproduce on this branch: a directory with one tool, with that tool on the skip list, prints Skipping tool … and exits 2.

Suggested fix: count skipped tools and workflows as successful targets, e.g. exit_codes.append(EXIT_CODE_OK) before the continue, and the same for skipped workflows. Alternatively, use assert_at_least_one=False as you offered. A test that covers the skiplist-only case would be good too.

[jmchilton]

Posted by Claude (AI assistant) on jmchilton's behalf, not authored by them personally.

Good catch, confirmed and fixed in a0cb85c.

Reproduced against the real job inputs rather than just a synthetic case: tools/optitype from
tools-iuc main, with the live galaxyproject_tools-iuc_skip_list, running
planemo autoupdate . --skiplist ... from inside the repo directory exactly as the workflow does.

planemo	tools/optitype	tools/interproscan
master b8547ffd	0	0
this PR @ 9d4c397d	2	2
this PR @ a0cb85ce	0	0

Both print only Skipping tool .... planemo ci_find_repos tools/ returns 720 repos and both are
among them, so the weekly job does visit them.

Went with your first suggestion rather than assert_at_least_one=False, so a genuinely empty path
still reports "no such target":

• skipped tools append EXIT_CODE_OK before the continue
• skipped workflows are counted separately from the ones that get updated, since they were being
  filtered out of workflows before anything was recorded

An empty directory still exits 2.

Two notes on the census, neither affecting the fix:

• data_managers/data_manager_interproscan isn't reachable from this job - the tools-iuc matrix
  entry sets upstream_repo_dir: tools/, and ci_find_repos tools/ returns no data_managers/
  paths.
• mitos2.xml is on the skip list too, but tools/mitos also ships mitos.xml, so it was never
  affected.

Two tests added, both in the fast tier (no conda, no Galaxy):

• test_autoupdate_skiplist_only_tool - single_tool repo with cat.xml skiplisted
• test_autoupdate_skiplist_only_workflow - one .ga target, skiplisted

Verified red-to-green: with the source change stashed both fail, with it restored both pass.

[mvdbeek]

Awesome, merge away if you're happy