Fix fragile parameter parsing in concore.py and concoredocker.py#201
Fix fragile parameter parsing in concore.py and concoredocker.py#201GREENRAT-K405 wants to merge 3 commits intoControlCore-Project:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous regex-based concore.params parsing in the Python runtime (concore.py) and Docker runtime (concoredocker.py) with explicit key=value parsing plus per-value ast.literal_eval coercion.
Changes:
- Added
parse_params(...)helper(s) to parse delimitedkey=valuepairs andliteral_evalvalues when possible. - Updated
concore.paramsloading to call the new parser and simplified error handling/logging in both runtimes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
concore.py |
Introduces a semicolon-delimited parse_params and switches params loading to use it. |
concoredocker.py |
Introduces a comma-delimited parse_params and switches params loading to use it with improved exception reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pradeeban please review this before merging: The original parsing logic in concore.py splits around ';' deliminater, whereas the concoredocker.py parsing logic splits around ',' comma was this intentional? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # keep backward compatibility: comma-separated params | ||
| for item in s.split(","): | ||
| if "=" in item: | ||
| key, value = item.split("=", 1) | ||
| key = key.strip() | ||
| value = value.strip() | ||
| #try to convert to python type (int, float, list, etc.) | ||
| # Use literal_eval to preserve backward compatibility (integers/lists) | ||
| # Fallback to string for unquoted values (paths, URLs) | ||
| try: | ||
| params[key] = literal_eval(value) | ||
| except (ValueError, SyntaxError): | ||
| params[key] = value |
There was a problem hiding this comment.
parse_params splits on commas unconditionally (s.split(",")), which will corrupt values that legally contain commas (e.g., list literals like a=[1,2], CSV-like strings, or quoted strings containing commas). This contradicts the goal of supporting structured values via literal_eval. Consider implementing a delimiter-splitting routine that respects quotes/bracket nesting (or require full dict literals/JSON when values may contain commas).
| for item in s.split(";"): | ||
| if "=" in item: | ||
| key, value = item.split("=", 1) # split only once | ||
| key=key.strip() | ||
| value=value.strip() | ||
| #try to convert to python type (int, float, list, etc.) | ||
| # Use literal_eval to preserve backward compatibility (integers/lists) | ||
| # Fallback to string for unquoted values (paths, URLs) | ||
| try: | ||
| params[key] = literal_eval(value) | ||
| except (ValueError, SyntaxError): | ||
| params[key] = value | ||
| return params |
There was a problem hiding this comment.
parse_params splits on ; without honoring quoting/escaping, so any value containing a semicolon (e.g., URLs or user strings) will be split into multiple items and parsed incorrectly. If semicolons are expected inside values, switch to a parser that tokenizes key/value pairs while tracking quote/bracket depth (or require dict/JSON literals for such cases).
1 participant
I have replaced the regex-based parameter parsing logic in concore.py and concoredocker.py with a deterministic key=value parser, eliminating data corruption and parse failures when parameter values contain spaces, =, or delimiters.
I have ensured backward compatibility by continuing to automatically convert numeric and structured values (e.g., int, float, list) using ast.literal_eval, while safely falling back to raw strings for values that cannot be evaluated (e.g., paths, URLs).
(This closes #184.)