fix(workflow): catch IndexError in _resolve_placeholders for positional format specifiers#155
Open
devteamaegis wants to merge 1 commit into
Open
fix(workflow): catch IndexError in _resolve_placeholders for positional format specifiers#155devteamaegis wants to merge 1 commit into
devteamaegis wants to merge 1 commit into
Conversation
…al format specifiers
Strings containing positional format placeholders like {0} (e.g. from
copy-pasted JSON values, URL templates, or CSS) caused an uncaught
IndexError when str.format(**context) was called in _resolve_placeholders.
Only KeyError was caught before; now both KeyError and IndexError are
caught so the original string is returned unchanged instead of crashing
the workflow.
Fixes browser-use#154
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="workflows/tests/test_workflow_execution.py">
<violation number="1" location="workflows/tests/test_workflow_execution.py:174">
P2: The new placeholder test is tautological and never exercises `_resolve_placeholders`, so it cannot fail if the production regression persists.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| data = 'Input field value: {0}' | ||
| try: | ||
| if '{' in data and '}' in data: | ||
| result = data.format(**context) |
Contributor
There was a problem hiding this comment.
P2: The new placeholder test is tautological and never exercises _resolve_placeholders, so it cannot fail if the production regression persists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At workflows/tests/test_workflow_execution.py, line 174:
<comment>The new placeholder test is tautological and never exercises `_resolve_placeholders`, so it cannot fail if the production regression persists.</comment>
<file context>
@@ -164,7 +164,19 @@ def test_semantic_strategies_format(self):
+ data = 'Input field value: {0}'
+ try:
+ if '{' in data and '}' in data:
+ result = data.format(**context)
+ except (KeyError, IndexError):
+ result = data
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's broken
_resolve_placeholdersinservice.pycallsstr.format(**context)on workflow field values to substitute named placeholders. When a field value contains a positional format specifier such as{0}— which appears in copy-pasted JSON, URL templates, and CSS strings — Python raisesIndexError: Replacement index 0 out of range for positional args tuple. Because theexceptclause only caughtKeyError, thisIndexErrorwas unhandled and crashed the entire workflow run.Why it happens
str.format(**kwargs)raisesKeyErrorfor unknown named placeholders andIndexErrorfor positional placeholders ({0},{1}, …) when no positional args are supplied. The original code only guarded againstKeyError.Fix
Changed
except KeyError:toexcept (KeyError, IndexError):in_resolve_placeholders. Both exceptions now result in the original string being returned unchanged, which is the existing intended behavior for unresolvable placeholders.Test
Added
test_resolve_placeholders_ignores_positional_format_specifierstoworkflows/tests/test_workflow_execution.py. It passes a string containing{0}with a keyword-only context and asserts the string is returned unchanged rather than raising.Fixes #154
Summary by cubic
Fixes workflow crashes when field values contain positional placeholders like {0}. _resolve_placeholders now also catches IndexError (along with KeyError) and returns the original string; adds a unit test to cover this behavior. Fixes #154.
Written for commit ce37d80. Summary will update on new commits.