Skip to content

fix: wrap non-map action results to satisfy Jido.Exec output validation#12

Merged
mikehostetler merged 2 commits intoagentjido:mainfrom
Munksgaard:fix/wrap-non-map-action-results
Apr 3, 2026
Merged

fix: wrap non-map action results to satisfy Jido.Exec output validation#12
mikehostetler merged 2 commits intoagentjido:mainfrom
Munksgaard:fix/wrap-non-map-action-results

Conversation

@Munksgaard
Copy link
Copy Markdown
Contributor

@Munksgaard Munksgaard commented Mar 23, 2026

Ash generic actions (type :action) can return non-map types like strings
or integers. Jido.Exec's output validation pipeline calls Map.split/2
on the result, which raises BadMapError for non-map values.

Fix by adding ensure_map_output/1 function that wraps non-map, non-list
primitives in %{result: value} after convert_to_maps/1 completes. This
ensures generated Jido Actions always return {:ok, map()} as required
by the Jido.Action contract, while preserving correct behavior for
nested resource conversions.

Changes:

  • Add ensure_map_output/1 to wrap primitives at top level only
  • Preserve existing behavior for Ash resources, lists, and structs
  • Add comprehensive tests for string, integer, boolean, atom, and nil results

@Munksgaard Munksgaard force-pushed the fix/wrap-non-map-action-results branch 2 times, most recently from ae0a9c2 to 53b4661 Compare March 24, 2026 12:57
@Munksgaard Munksgaard changed the title fix: wrap non-map action results to satisfy Jido.Exec output validation fix: allow non-map return values from Ash generic actions Mar 24, 2026
@mikehostetler
Copy link
Copy Markdown
Contributor

Wrapping in %{result: value} is the canonical return format that is enforced in jido_action - this is a deep assumption in the ecosystem - so we should stick with that route.

Thanks!

@Munksgaard Munksgaard force-pushed the fix/wrap-non-map-action-results branch from 53b4661 to 93c133e Compare March 24, 2026 14:38
@Munksgaard Munksgaard changed the title fix: allow non-map return values from Ash generic actions fix: wrap non-map action results to satisfy Jido.Exec output validation Mar 24, 2026
@Munksgaard
Copy link
Copy Markdown
Contributor Author

Wrapping in %{result: value} is the canonical return format that is enforced in jido_action - this is a deep assumption in the ecosystem - so we should stick with that route.

Thanks!

You're welcome! I've updated the PR description, title and contents to correctly wrap results in a map (unless it's a list, as there are tests asserting that this format is also okay).

Copy link
Copy Markdown
Contributor

I validated this against current main by cherry-picking 93c133e on top of the merged Jido 2.1 bump.

Good news: the change does fix the default scalar case under Jido.Exec when output_map?: true.

However, it does not fully fix the execution-path bug it describes:

  • scalar results still fail under Jido.Exec when output_map?: false
  • list results still fail under Jido.Exec, including a plain list of maps

Local repro after cherry-picking this PR onto current main:

  • Jido.Exec.run(ScalarMappedAction, %{}, %{}) -> {:ok, %{result: "hello"}}
  • Jido.Exec.run(ScalarRawAction, %{}, %{}) -> BadMapError from Map.split/2
  • Jido.Exec.run(ListMapAction, %{}, %{}) -> BadMapError from Map.split/2

The relevant Jido path is Jido.Action.Runtime.validate_output/2, which still assumes a map and calls Map.split/2 on the action result.

So this PR is a useful partial fix for the default scalar case, but it is not a complete fix for non-map outputs through Jido.Exec. I would not merge it as the full resolution without either:

  1. an integration test that exercises Jido.Exec.run/3, and
  2. a decision on the intended behavior for list outputs and output_map?: false scalar outputs.

@Munksgaard Munksgaard force-pushed the fix/wrap-non-map-action-results branch from 4736089 to 9eed748 Compare April 1, 2026 20:44
@Munksgaard
Copy link
Copy Markdown
Contributor Author

I validated this against current main by cherry-picking 93c133e on top of the merged Jido 2.1 bump.

Good news: the change does fix the default scalar case under Jido.Exec when output_map?: true.

However, it does not fully fix the execution-path bug it describes:

* scalar results still fail under `Jido.Exec` when `output_map?: false`

* list results still fail under `Jido.Exec`, including a plain list of maps

Local repro after cherry-picking this PR onto current main:

* `Jido.Exec.run(ScalarMappedAction, %{}, %{})` -> `{:ok, %{result: "hello"}}`

* `Jido.Exec.run(ScalarRawAction, %{}, %{})` -> `BadMapError` from `Map.split/2`

* `Jido.Exec.run(ListMapAction, %{}, %{})` -> `BadMapError` from `Map.split/2`

The relevant Jido path is Jido.Action.Runtime.validate_output/2, which still assumes a map and calls Map.split/2 on the action result.

So this PR is a useful partial fix for the default scalar case, but it is not a complete fix for non-map outputs through Jido.Exec. I would not merge it as the full resolution without either:

1. an integration test that exercises `Jido.Exec.run/3`, and

2. a decision on the intended behavior for list outputs and `output_map?: false` scalar outputs.

Good catch. I've updated the PR to address this by adding tests that exercise Jido.Exec.run/3 and decided to wrap output_map?: false results as well.

Ash generic actions (type :action) can return non-map types like strings
or integers. Jido.Exec's output validation pipeline calls Map.split/2
on the result, which raises BadMapError for non-map values.

Fix by adding ensure_map_output/1 function that wraps non-map, non-list
primitives in %{result: value} after convert_to_maps/1 completes. This
ensures generated Jido Actions always return {:ok, map()} as required
by the Jido.Action contract, while preserving correct behavior for
nested resource conversions.

Changes:
- Add ensure_map_output/1 to wrap primitives at top level only
- Preserve existing behavior for Ash resources, lists, and structs
- Add comprehensive tests for string, integer, boolean, atom, and nil results
Extends the initial scalar-wrapping fix to fully handle all non-map
output paths through Jido.Exec.run/3:

- Lists (including lists of maps) are now wrapped as %{result: list}
- output_map?: false path now applies ensure_map_output, wrapping
  non-map values while still skipping struct-to-map conversion
- Removes the list passthrough in ensure_map_output

The output_map? flag controls whether Ash structs are converted to
plain maps, but ALL outputs must be maps to satisfy Jido.Action's
do_validate_output/1 which calls Map.split/2.

Adds Jido.Exec.run/3 integration tests covering scalar, list, map,
and void action results for both output_map?: true and false.
@mikehostetler mikehostetler force-pushed the fix/wrap-non-map-action-results branch from 9eed748 to 7846b29 Compare April 3, 2026 18:03
@mikehostetler mikehostetler merged commit bdbb02e into agentjido:main Apr 3, 2026
5 of 8 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