Address review feedback for state attribute feature#1098
Conversation
- Add state attribute to Gremlin class initialized in __init__ to None - Populate gremlin.state in initialize_with_runtime via build_state() - Update _collect_stages() to set gremlin.state before each stage.run() call via make_runner closure - Add comprehensive tests for gremlin.state attribute accessibility Stages can now access current state via self.gremlin.state in addition to the state parameter. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Extract duplicate cwd computation to _resolve_cwd() helper - Remove dead gremlin_ref alias; use self directly in closure - Remove redundant _set_gremlin_recursive() call in test Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
xbrianh
left a comment
xbrianh
left a comment
There was a problem hiding this comment.
One correctness bug: the new build_state call in initialize_with_runtime sits outside the try/except that guards worktree cleanup, so a failure there leaks the worktree. Everything else looks clean — the default-arg capture in _set_state_and_run is the right pattern, _resolve_cwd is a nice extraction, and the tests cover the key scenarios.
| raise | ||
|
|
||
| cwd = self._resolve_cwd() | ||
| self.state = build_state( |
There was a problem hiding this comment.
This build_state call is outside the try/except block that cleans up worktree_created on failure (the handler ends at line 532). If build_state raises, the worktree is leaked.
Move these lines inside the try block:
try:
...
# existing registry setup
...
cwd = self._resolve_cwd()
self.state = build_state(...)
except Exception:
if worktree_created:
_git_mod.remove_worktree(self.project_root, worktree_created)
raiseThere was a problem hiding this comment.
Fixed. Moved the build_state() call inside the try block to ensure worktree cleanup on error.
There was a problem hiding this comment.
Pull request overview
This PR refines the in-progress “Gremlin carries a .state attribute” feature (Issue #1090) by reducing duplicated cwd logic and ensuring Gremlin.state is available both after initialization and during stage execution.
Changes:
- Extracts repeated cwd selection logic into
Gremlin._resolve_cwd(). - Initializes
Gremlin.stateininitialize_with_runtime()viabuild_state(...). - Wraps top-level stage runners so
Gremlin.stateis set immediately beforestage.run(...)executes, and adds unit tests covering initialization/access patterns.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
gremlins/executor/gremlin.py |
Adds/initializes Gremlin.state, factors out cwd computation, and wraps top-level runners to set self.state during execution. |
tests/test_gremlin_state.py |
Adds tests asserting Gremlin.state is populated after initialization and visible during a stage run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class Gremlin: | ||
| registry: ArtifactRegistry | ||
| state: State | None | ||
|
|
There was a problem hiding this comment.
Added state: Any to GremlinProtocol to enable type checking for stage code using self.gremlin.state.
| base_runner = stage_state.make_runner(e, scope=stages) | ||
|
|
||
| async def _set_state_and_run( | ||
| runner: Callable[[], Awaitable[Any]] = base_runner, | ||
| state: State = stage_state, | ||
| ) -> Any: | ||
| self.state = state | ||
| return await runner() | ||
|
|
||
| built.append((e.name, _set_state_and_run)) |
There was a problem hiding this comment.
Fixed. Wrapped nested stage runners in sequence, loop, and parallel stages to set child.gremlin.state before execution. This ensures nested stages have access to their correct child state.
- Move build_state() call inside try/except to prevent worktree leak on error - Add state attribute to GremlinProtocol for type checking - Wrap nested stage runners to set child.gremlin.state during execution Fixes the issue where nested stages in sequence, loop, and parallel groups did not have their gremlin.state updated to the appropriate child state. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Summary
Address review feedback on the state attribute feature:
_resolve_cwd()helper methodself.stateis set during stage execution via closure wrapperself.stateafterbuild_state()is called in the executorTest coverage
Added comprehensive tests for the state attribute initialization and access patterns.
Closes #1090