Added some AI identified bugs with tests and small fixes#198
Open
mmcdermott wants to merge 2 commits intomainfrom
Open
Added some AI identified bugs with tests and small fixes#198mmcdermott wants to merge 2 commits intomainfrom
mmcdermott wants to merge 2 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughUpdated project configuration and CI workflows; migrated Polars casting from Utf8 to String; added a ts_format parameter for parsing timestamps in predicate loading; added omegaconf and a dev dependency group; added idempotency tests for extract_subtree and query. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_extract_subtree_idempotency.py (1)
28-36: Consider usingdeepcopyin fixture to prevent cross-test contamination.The
simple_treefixture creates a tree with mutable nodes. If the bug exists, the first test invocation mutates the tree, potentially affecting other tests. Whilepytest.fixture()creates a new fixture instance per test by default, explicitly usingcopy.deepcopycould make the intent clearer.💡 Suggested defensive copy
+import copy + class TestExtractSubtreeIdempotency: """Calling extract_subtree twice on the same tree must produce identical results.""" `@pytest.fixture`() def simple_tree(self): """A minimal tree: trigger -> gap (2-day temporal window).""" root = Node("trigger") child = Node("gap") child.endpoint_expr = TemporalWindowBounds(True, timedelta(days=2), True) child.constraints = {} child.parent = root - return root + return copy.deepcopy(root) # Defensive copy to ensure clean state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_extract_subtree_idempotency.py` around lines 28 - 36, The fixture simple_tree returns a mutable Node tree that can be accidentally shared between tests; to prevent cross-test contamination, construct the tree as shown but return a deep copy (using copy.deepcopy) of the root before returning, ensuring you import copy; alternatively, build the tree inside the fixture and assign the returned value to a local variable like root_copy = copy.deepcopy(root) and return root_copy so Node and TemporalWindowBounds instances are not shared across tests.pyproject.toml (1)
26-26: Consider pinningomegaconfversion for reproducibility.Unlike other dependencies in this file,
omegaconflacks a version constraint. Sincehydra-core ~= 1.3.2is already specified and Hydra depends on omegaconf, consider pinning omegaconf to a compatible version to avoid potential version conflicts or unexpected behavior.💡 Suggested fix
- "omegaconf", + "omegaconf ~= 2.3",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 26, The pyproject.toml currently lists "omegaconf" without a version constraint which can lead to incompatible installs with the declared "hydra-core ~= 1.3.2"; update the dependency entry for omegaconf to a specific compatible version (for example pin to the omegaconf release that matches Hydra 1.3.x, such as "omegaconf ~= 2.3.0" or the exact patch you want) so installs are reproducible and avoid runtime conflicts with hydra-core; locate the dependency line containing "omegaconf" and replace it with the chosen pinned version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyproject.toml`:
- Line 26: The pyproject.toml currently lists "omegaconf" without a version
constraint which can lead to incompatible installs with the declared "hydra-core
~= 1.3.2"; update the dependency entry for omegaconf to a specific compatible
version (for example pin to the omegaconf release that matches Hydra 1.3.x, such
as "omegaconf ~= 2.3.0" or the exact patch you want) so installs are
reproducible and avoid runtime conflicts with hydra-core; locate the dependency
line containing "omegaconf" and replace it with the chosen pinned version.
In `@tests/test_extract_subtree_idempotency.py`:
- Around line 28-36: The fixture simple_tree returns a mutable Node tree that
can be accidentally shared between tests; to prevent cross-test contamination,
construct the tree as shown but return a deep copy (using copy.deepcopy) of the
root before returning, ensuring you import copy; alternatively, build the tree
inside the fixture and assign the returned value to a local variable like
root_copy = copy.deepcopy(root) and return root_copy so Node and
TemporalWindowBounds instances are not shared across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 396cac14-391c-45f2-8857-b3175cd0be5e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.tomlsrc/aces/config.pysrc/aces/predicates.pytests/test_extract_subtree_idempotency.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/python-build.yaml (2)
3-3: Consider restricting the trigger to avoid unnecessary builds.The workflow triggers on every push to any branch. For a build/publish workflow, consider restricting to main branch and tags to avoid unnecessary artifact generation.
🔧 Proposed change
-on: push +on: + push: + branches: [main] + tags: + - 'v*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-build.yaml at line 3, Replace the broad "on: push" trigger so the workflow only runs for intended branch/tags (e.g., restrict to pushes to the main branch and tag creation) instead of every branch; locate the "on: push" entry and update it to specify branches: [main] and tags (or tag pattern) as needed, optionally adding pull_request triggers if you still want PR builds.
14-28: Consider using the composite setup action for consistency.Other workflows (code-quality-main.yaml, code-quality-pr.yaml, tests.yaml) use the new composite action at
./.github/actions/setup. This workflow manually sets up Python and uv separately. While functional, using the composite action would improve maintainability and consistency.♻️ Proposed refactor for consistency
steps: - uses: actions/checkout@v4 - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 - with: - python-version: ${{ matrix.python-version }} - - - name: Install uv - uses: astral-sh/setup-uv@v6 - with: - enable-cache: true + - name: Setup package + uses: ./.github/actions/setup + with: + python-version: ${{ matrix.python-version }} - name: Build run: uv buildNote: You may need to verify that
uv buildworks correctly with the composite action'suv syncstep, or add a build-specific group to the action.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-build.yaml around lines 14 - 28, Replace the manual Python and uv setup steps with the repository's composite action at ./.github/actions/setup to match other workflows; specifically remove the separate "Set up Python" (actions/setup-python@v5) and "Install uv" (astral-sh/setup-uv@v6) steps and invoke the composite action (keep or map inputs like matrix.python-version and enable-cache if the composite supports them), then run the build command (uv build) after the composite action completes and verify uv build works with the composite's uv sync behavior or add a build-specific group to the composite if needed..github/actions/setup/action.yaml (1)
27-28: Quote the input variable to prevent word splitting and potential issues.The
${{ inputs.group }}interpolation is unquoted, which could cause issues if the input contains spaces or special characters. While the input is constrained to known values (benchmarks, dev, docs), quoting is a defensive best practice.🔧 Proposed fix
- name: Install packages shell: bash run: | - uv sync --locked --group ${{ inputs.group }} + uv sync --locked --group "${{ inputs.group }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup/action.yaml around lines 27 - 28, The GitHub Action run step calls the uv sync command with an unquoted interpolation `${{ inputs.group }}`, which can cause word-splitting if the input ever contains spaces or special characters; update the run command that constructs the uv sync invocation (the line containing "uv sync --locked --group ${{ inputs.group }}") to quote the interpolated input (e.g., wrap `${{ inputs.group }}` in quotes or single quotes) so the group value is passed as a single argument..github/workflows/tests.yaml (1)
31-34: Consider updating codecov-action to v5.5.2. The current version v4.0.1 is outdated; v5.5.2 is the latest release and includes bug fixes and improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yaml around lines 31 - 34, Update the GitHub Actions step named "Upload coverage to Codecov" to use the newer action release by changing the uses reference from codecov/codecov-action@v4.0.1 to codecov/codecov-action@v5.5.2; locate the step that currently has uses: codecov/codecov-action@v4.0.1 and replace the version tag ensuring existing inputs like token: ${{ secrets.CODECOV_TOKEN }} remain unchanged and compatible with v5.5.2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/code-quality-pr.yaml:
- Around line 31-34: The "Run pre-commits" step currently passes an empty
--files argument when steps.file_changes.outputs.all_changed_files is empty;
update the step to guard execution by checking the output before invoking
pre-commit (e.g., add an if condition on
steps.file_changes.outputs.all_changed_files != '' or wrap the command in a
shell test like [[ -n "${{ steps.file_changes.outputs.all_changed_files }}" ]]
&& ...), so pre-commit only runs with --files when there are changed files and
is skipped otherwise; reference the step name "Run pre-commits" and the variable
"steps.file_changes.outputs.all_changed_files" when making this change.
---
Nitpick comments:
In @.github/actions/setup/action.yaml:
- Around line 27-28: The GitHub Action run step calls the uv sync command with
an unquoted interpolation `${{ inputs.group }}`, which can cause word-splitting
if the input ever contains spaces or special characters; update the run command
that constructs the uv sync invocation (the line containing "uv sync --locked
--group ${{ inputs.group }}") to quote the interpolated input (e.g., wrap `${{
inputs.group }}` in quotes or single quotes) so the group value is passed as a
single argument.
In @.github/workflows/python-build.yaml:
- Line 3: Replace the broad "on: push" trigger so the workflow only runs for
intended branch/tags (e.g., restrict to pushes to the main branch and tag
creation) instead of every branch; locate the "on: push" entry and update it to
specify branches: [main] and tags (or tag pattern) as needed, optionally adding
pull_request triggers if you still want PR builds.
- Around line 14-28: Replace the manual Python and uv setup steps with the
repository's composite action at ./.github/actions/setup to match other
workflows; specifically remove the separate "Set up Python"
(actions/setup-python@v5) and "Install uv" (astral-sh/setup-uv@v6) steps and
invoke the composite action (keep or map inputs like matrix.python-version and
enable-cache if the composite supports them), then run the build command (uv
build) after the composite action completes and verify uv build works with the
composite's uv sync behavior or add a build-specific group to the composite if
needed.
In @.github/workflows/tests.yaml:
- Around line 31-34: Update the GitHub Actions step named "Upload coverage to
Codecov" to use the newer action release by changing the uses reference from
codecov/codecov-action@v4.0.1 to codecov/codecov-action@v5.5.2; locate the step
that currently has uses: codecov/codecov-action@v4.0.1 and replace the version
tag ensuring existing inputs like token: ${{ secrets.CODECOV_TOKEN }} remain
unchanged and compatible with v5.5.2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff282b73-9e28-4d07-889f-ad6ede686598
📒 Files selected for processing (5)
.github/actions/setup/action.yaml.github/workflows/code-quality-main.yaml.github/workflows/code-quality-pr.yaml.github/workflows/python-build.yaml.github/workflows/tests.yaml
Comment on lines
31
to
+34
| - name: Run pre-commits | ||
| uses: pre-commit/action@v3.0.1 | ||
| with: | ||
| extra_args: --files ${{ steps.file_changes.outputs.files}} | ||
| run: > | ||
| uv run pre-commit run --show-diff-on-failure | ||
| --files ${{ steps.file_changes.outputs.all_changed_files}} |
Contributor
There was a problem hiding this comment.
Consider handling the case when no files are changed.
If no files are changed, ${{ steps.file_changes.outputs.all_changed_files }} will be empty, and the command becomes uv run pre-commit run --show-diff-on-failure --files with no files specified. This may cause pre-commit to run on all files or error unexpectedly.
🔧 Proposed fix to conditionally run pre-commit
- name: Run pre-commits
+ if: steps.file_changes.outputs.any_changed == 'true'
run: >
uv run pre-commit run --show-diff-on-failure
--files ${{ steps.file_changes.outputs.all_changed_files}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/code-quality-pr.yaml around lines 31 - 34, The "Run
pre-commits" step currently passes an empty --files argument when
steps.file_changes.outputs.all_changed_files is empty; update the step to guard
execution by checking the output before invoking pre-commit (e.g., add an if
condition on steps.file_changes.outputs.all_changed_files != '' or wrap the
command in a shell test like [[ -n "${{
steps.file_changes.outputs.all_changed_files }}" ]] && ...), so pre-commit only
runs with --files when there are changed files and is skipped otherwise;
reference the step name "Run pre-commits" and the variable
"steps.file_changes.outputs.all_changed_files" when making this change.
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
1 participant
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.
@justin13601 , these are mostly generated by Claude but I think are reasonable -- can you take a look?
https://claude.ai/share/93456d52-1721-466d-85e3-66e78e8c4dff
Summary by CodeRabbit
New Features
Documentation
Tests
Chores