Repository navigation
ci: check the Python inside %%capture-style cells - #58
Merged
Merged
Conversation
The smoke check skipped every cell that starts with a cell magic, so a syntax error inside a %%capture cell passed CI (Qodo review note on GenAI_Agents #182). Cells under %%bash, %%writefile and other non-Python magics are still skipped. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
Code Review by Qodo
🟠 Medium 1. Indented notebook cells pass the check
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+52
to
+55
| if magic not in {"capture", "time", "timeit", "prun"}: | ||
| continue | ||
| # These wrap ordinary Python, so check the body. | ||
| body = body[1:] |
There was a problem hiding this comment.
🟠 Medium
1. Some invalid magic cells pass the check 🐞 Bug ≡ Correctness
cells.append() collects the newly admitted magic bodies, but the workflow compiles all cells as one string rather than checking each body independently. Two cells containing %%time\nx = ( and %%capture\n1) each have invalid Python bodies, yet their combined source compiles successfully.
Agent Prompt
## Issue description
The newly included cell-magic bodies can complete each other's incomplete Python when the workflow compiles their concatenated source, hiding errors in individual cells.
## Fix Focus Areas
- .github/workflows/notebook-smoke.yml[48-62]
## Recommended Fix
After stripping an eligible magic header and handling IPython-only lines, compile each processed code cell independently rather than compiling the joined notebook. Keep failures attributed to the notebook, preferably with the cell index.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # IPython line magics and shell escapes are not Python; drop them. | ||
| lines = [ | ||
| line for line in cell.source.splitlines() | ||
| line for line in body |
There was a problem hiding this comment.
🟠 Medium
2. Valid magic cells can fail the check 🐞 Bug ≡ Correctness
The line filter removes lines starting with % or ! from the newly included magic bodies without distinguishing IPython commands from text inside Python strings. For example, a %%time body containing text = """\n%""" is valid Python, but removing its closing line leaves an unterminated string for compile().
Agent Prompt
## Issue description
Applying the existing prefix-based line filter to newly included magic bodies can delete string-literal content and make valid cells fail compilation.
## Fix Focus Areas
- .github/workflows/notebook-smoke.yml[48-62]
## Recommended Fix
Make IPython-command removal aware of Python string boundaries so `%` and `!` lines inside strings remain intact. Add a selected-magic test containing a multiline string whose closing line starts with `%`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # A cell magic such as %%bash makes the whole cell non-Python; skip it. | ||
| if cell.source.lstrip().startswith("%%"): | ||
| continue | ||
| body = cell.source.lstrip().splitlines() |
There was a problem hiding this comment.
🟠 Medium
3. Indented notebook cells pass the check 🐞 Bug ≡ Correctness
cell.source.lstrip() now removes leading whitespace from the first line of every code cell before compilation. In a first cell containing if True:\n value = 1, the workflow compiles a dedented if statement even though compiling the original cell source raises IndentationError.
Agent Prompt
## Issue description
The new unconditional `lstrip()` changes ordinary code-cell source before the syntax check and can conceal invalid initial indentation.
## Fix Focus Areas
- .github/workflows/notebook-smoke.yml[48-55]
## Recommended Fix
Split the original cell source without stripping it. If leading whitespace must be ignored when identifying a cell-magic header, apply `lstrip()` only to the line being inspected, not to the source passed onward for compilation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
The notebook smoke check skipped every cell that starts with a cell magic, so a syntax error inside a %%capture cell passed CI. This compiles the body of %%capture, %%time, %%timeit and %%prun cells and still skips %%bash, %%writefile and other non-Python magics. Found by the Qodo review on GenAI_Agents #182. Checked locally: the check passes on main's notebooks.
🤖 Generated with Claude Code
PR Summary by Qodo
Compile Python bodies in supported notebook cell magics
🐞 Bug fix🧪 Tests🕐 Less than 10 minutesAI Description
Diagram
graph TD CI["CI smoke check"] --> Scan["Notebook cells"] --> Kind{"Cell magic?"} Kind -->|supported Python magic| Strip["Remove directive"] --> Filter["Filter IPython lines"] --> Compile["Compile notebook"] Kind -->|ordinary code| Filter Kind -->|other magic| Skip["Skip cell"]High-Level Assessment
Files changed (1)
+9 / -4Bug fix (1)
+9 / -4notebook-smoke.yml
Check Python code inside supported cell magics+9/-4Summary by CodeRabbit
capture,time,timeit, andprunmagics, while continuing to skip cells with other magics.