-
Notifications
You must be signed in to change notification settings - Fork 0
Normalise selectors in plan synthesiser and verifier #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @staticmethod | ||
| def _normalise_selector(raw_selector: str) -> str: | ||
| token = raw_selector.split(",", 1)[0].strip() | ||
| token = token.rstrip(")").strip() | ||
| token = token.strip("\"'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truncating selectors at first comma breaks valid matches
The normaliser now calls raw_selector.split(",", 1) and discards everything after the first comma. Legitimate CSS selectors often contain commas inside attribute values ([aria-label="Hello, world"]) or intentionally select multiple elements (#foo, #bar). After this change _plan_compatible will normalise such selectors to only the prefix, so even when the context contains the full selector the synthesiser (and the identical logic in PlanVerifier._normalise_selector) will reject a compatible plan as missing its selector. The normalisation should respect quoted segments or only strip comma-separated arguments rather than blindly truncating.
Useful? React with 👍 / 👎.
| if "(" in token and token.endswith(")"): | ||
| prefix, arg_str = token.split("(", 1) | ||
| verb_name = prefix.strip() | ||
| args = [arg_str[:-1]] if arg_str.endswith(")") else [arg_str] | ||
| if arg_str.endswith(")"): | ||
| arg_str = arg_str[:-1] | ||
| raw_args = [part for part in arg_str.split(",") if part.strip()] | ||
|
|
||
| def _clean_argument(value: str) -> str: | ||
| cleaned = value.strip().rstrip(")").strip() | ||
| cleaned = cleaned.strip("\"'") | ||
| return cleaned | ||
|
|
||
| args = [_clean_argument(part) for part in raw_args] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument parsing ignores quoted commas and corrupts tokens
When _handle_plan_sketch splits arg_str by "," it does not consider quoting, so a token such as Type(#field, "hello, world") produces args = ["#field", "hello", "world"] instead of keeping the comma inside the second argument. Any plan where argument values legitimately contain commas or closing parentheses will now be broken and the original string cannot be reconstructed. Using a parsing approach that honours quoted strings or balanced parentheses would avoid fragmenting arguments.
Useful? React with 👍 / 👎.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f6f8f5417c83279350661ed4f833e9