Skip to content

Comments

core: add AST-based linter for undeclared state reads in function-bas…#656

Open
Smitaambiger wants to merge 3 commits intoapache:mainfrom
Smitaambiger:feature/action-reads-linter
Open

core: add AST-based linter for undeclared state reads in function-bas…#656
Smitaambiger wants to merge 3 commits intoapache:mainfrom
Smitaambiger:feature/action-reads-linter

Conversation

@Smitaambiger
Copy link

Closes #407

This PR adds AST-based validation for undeclared state reads in function-based actions.

What this does

  • Parses the originating function using inspect.getsource
  • Walks the AST to detect state["<literal>"] accesses
  • Compares detected keys with declared reads=[...]
  • Raises ValueError if undeclared keys are accessed
  • Skips validation when source is unavailable

Why

Prevents silent bugs caused by accessing state keys that were not declared in reads, enforcing stricter action correctness.

Tests

  • Added unit tests covering:
    • Valid declared reads
    • Undeclared read detection

@Smitaambiger
Copy link
Author

@elijahbenizzy

Initial implementation for AST-based undeclared state read validation.

Currently implemented as a hard ValueError at action construction time.

Happy to adjust:

  • validation timing (build-time vs decorator-time)
  • severity (warning vs error)

Looking forward to feedback.

return


import textwrap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imports should be top level

Comment on lines 10 to 23
def test_undeclared_state_read_raises_error():
with pytest.raises(ValueError):

@action(reads=["foo"], writes=[])
def bad_action(state: State):
x = state["bar"]
return {}, state


def test_declared_state_read_passes():
@action(reads=["foo"], writes=[])
def good_action(state: State):
x = state["foo"]
return {}, state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test for multiple missing keys interleaved with non-missing keys please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll also need a test to ensure this doesn't impact pydantic typed state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this test should be housed under test_action.py

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good start, a few things to add.

@Smitaambiger
Copy link
Author

@skrawcz ,Thanks for the detailed feedback!

Addressed the requested changes:

  • Moved imports to top level
  • Consolidated tests into test_action.py
  • Added test for multiple undeclared keys interleaved with declared keys
  • Added regression test to ensure pydantic-based actions are not impacted
  • Cleaned up lint/formatting issues

Core tests are passing locally and CI should reflect the same.

(.venv) PS C:\Users\gouta\OneDrive\Desktop\burr> python -m pytest tests/core -q
................................................................................................................................................................................................. [ 69%]
....................................................................................                                                                                                              [100%]
277 passed in 9.10s
(.venv) PS C:\Users\gouta\OneDrive\Desktop\burr> pre-commit run --all-files
black....................................................................Passed
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
fix requirements.txt.....................................................Passed
check python ast.........................................................Passed
isort....................................................................Passed
flake8...................................................................Passed
frontend-lint-staged.....................................................Passed

Please let me know if any further adjustments are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linter for action reads in state, function-based-actions

2 participants