fix(workforce): do not crash on non-string recovery_strategy in _fix_common_issues

[Shxiao101]

Related Issue

Closes #4318

Description

_fix_common_issues calls .lower() on TaskAnalysisResult.recovery_strategy unconditionally. When a weak model emits a non-string value for that field (e.g. "recovery_strategy": ["retry"]), the best-effort fix path itself raises AttributeError, which escapes parse_structured_response (only ValidationError is caught) and crashes the Workforce task-analysis flow instead of using fallback_values.

This PR guards the normalization with an isinstance(strategy, str) check. Non-string values are left untouched, so validation fails into the existing fallback chain (caller fallback_values → default instance) — exactly how unfixable string values already behave. Failure analysis keeps its documented default-to-RETRY behavior, and string normalization (lowercase + partial match) is unchanged.

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Testing evidence

New regression tests in test/workforce/test_structured_output_handler.py (fail on master with AttributeError, pass with this fix):

test/workforce/test_structured_output_handler.py::TestFixCommonIssuesRecoveryStrategy PASSED 5 passed in 2.32s

Existing suite unaffected: the failures in test/workforce/test_workforce.py on my machine are pre-existing missing-API-key errors, identical before and after this patch (11 passed / 7 failed in both cases).

Checklist

• I have read and agree to the AI-Generated Code Policy (required)
• I have linked this PR to an issue (required)
• I have checked if any dependencies need to be added or updated in pyproject.toml and run uv lock
• I have updated the tests accordingly (required for a bug fix or a new feature)
• I have updated the documentation if needed
• I have added examples if this is a new feature

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fa8fb700-7363-4bbf-9407-f88b63d8b975

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ebarkhordar]

I ran this against dfcf5194 in a clean container (python:3.11-slim, pip install -e . 'mcp<2.0.0').

The isinstance guard looks right, and it matches what the TaskAssignResult branch already does at :374. The new test is the part I would look at again: test_string_recovery_strategy_normalization_unchanged passes with the whole TaskAnalysisResult branch deleted, so it does not pin the normalization it is named for. 5 passed at dfcf5194, 5 passed with lines 384-407 removed.

That is not something this PR introduced. strategy is a local, and it reaches fixed_data only inside the partial-match loop at :404, so when strategy.lower() is already valid nothing is written back and the original casing goes to validation:

input       _fix returns   result   path taken
RETRY       'RETRY'        retry    _fix_common_issues -> default instance
rety        'rety'         retry    _fix_common_issues -> default instance
replan      'replan'       replan   validated on first try
REPLAN      'REPLAN'       retry    _fix_common_issues -> default instance
Decompose   'Decompose'    retry    _fix_common_issues -> default instance

So REPLAN and Decompose fail validation and fall through to the default instance, which is RETRY. All three cases in the test miss this: RETRY and rety land on that same default and RETRY is what it returns, and replan is already valid so it never enters _fix_common_issues. 'retry'.startswith('rety') is False, so rety is not a partial match either.

One line inside your isinstance block covers it:

strategy = strategy.lower()
fixed_data['recovery_strategy'] = strategy

I applied that and re-ran: REPLAN gives replan, Decompose gives decompose, and your 5 tests still pass.

Not checked: the async paths, and the rest of the workforce suite.

[Shxiao101]

Good catch on all three points — thanks for running it in a clean container.

Confirmed on my side: at dfcf5194, REPLAN/Decompose both landed on the default instance and _fix_common_issues('REPLAN') returned the input unchanged — the lowered value only lived in the local. Your one-liner is applied in 397c28f.

Tests reworked to pin paths instead of outcomes: test_mixed_case_strategy_is_normalized_in_place asserts the write-back at unit level and end-to-end (3 failed at dfcf5194, 8 pass now); RETRY/rety moved to test_unfixable_string_value_falls_back_to_default_instance, documenting that rety is not a partial match; the non-string guard is pinned at unit level too (test_non_string_value_is_left_untouched_for_validation).

On async paths: _fix_common_issues has a single call site (sync parse_structured_response), so they're unaffected.

[ebarkhordar]

Thanks, that matches what I measured. 397c28f puts the write-back exactly where the lowered value was being dropped, and asserting _fix_common_issues directly is better than the end to end check alone, since the RETRY default makes a failed normalization look like a pass. Nothing further from me.