Add state attribute and property delegates to Gremlin, address review feedback by xbrianh · Pull Request #1097 · xbrianh/gremlins

xbrianh · 2026-06-03T18:52:15Z

Summary

Adds state attribute and property delegates to the Gremlin class, and addresses review feedback to reduce boilerplate and eliminate state-initialization guards.

Changes

State initialization: Add state attribute to track State object and initialize it in initialize_with_runtime
Property delegates: Add property delegates to Gremlin for direct access to State's methods and attributes (client, artifacts, cwd, worktree, base_ref, loop_iteration, attempt, framework_subs, done_for, mark_done, clear_done, record_bail, record_stage_progress, record_state_field)
Consolidate guards: Replace 13 identical state-initialization guards with a single _s property accessor, reducing boilerplate by 52 lines
Extract cwd resolution: Move three-way cwd resolution (worktree → project_root → cwd) into reusable _resolved_cwd property, eliminating duplication between _collect_stages and initialize_with_runtime
Fix state overwrite: Remove self.state overwrite inside _collect_stages loop; per-stage states are now used only where needed, preserving the canonical state set in initialize_with_runtime
Add tests: Include test_gremlin_state_delegates_after_initialize and test_gremlin_state_unchanged_after_run to verify property delegation and state preservation

Test Coverage

Verify state is initialized after initialize_with_runtime
Verify property delegates work correctly (artifacts, client, cwd, base_ref, etc.)
Verify gremlin.state properties reflect canonical values after run() completes

Closes #1075

- Add State instance attribute to Gremlin class - Initialize state in initialize_with_runtime() and populate before each stage in _collect_stages() - Add property delegates for all fields stages currently access: - gremlin.client → gremlin.state.client - gremlin.artifact_dir → gremlin.state.artifact_dir - gremlin.artifacts → gremlin.state.artifacts - gremlin.cwd → gremlin.state.cwd - gremlin.worktree → gremlin.state.worktree - gremlin.base_ref → gremlin.state.base_ref - gremlin.loop_iteration → gremlin.state.data.loop_iteration - gremlin.attempt → gremlin.state.data.attempt - gremlin.framework_subs() → gremlin.state.framework_subs() - gremlin.done_for() → gremlin.state.done_for() - gremlin.mark_done() → gremlin.state.mark_done() - gremlin.clear_done() → gremlin.state.clear_done() - gremlin.record_bail() → gremlin.state.record_bail() - gremlin.record_stage_progress() → gremlin.state.record_stage_progress() - gremlin.record_state_field() → gremlin.state.record_state_field() - Add unit test verifying delegates are accessible after initialize_with_runtime() Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

1. Consolidate 13 identical state-initialization guards into a single _s property accessor, reducing boilerplate by 52 lines. 2. Extract the three-way cwd resolution (worktree → project_root → cwd) into a reusable _resolved_cwd property, eliminating a verbatim duplication between _collect_stages and initialize_with_runtime. 3. Remove self.state overwrite inside _collect_stages loop. Per-stage states are now used only where needed (stage_state.make_runner); the canonical state set in initialize_with_runtime is no longer clobbered by intermediate iterations. 4. Add test_gremlin_state_unchanged_after_run to verify that gremlin.state properties (client, base_ref, repo) correctly reflect canonical values after run() completes, exercising the fixed code path. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

Adds a canonical state attribute to Gremlin, along with a _s accessor and a set of delegation properties/methods so callers can access State fields via Gremlin without repeated “state not initialized” guards. Also factors out cwd resolution into _resolved_cwd and adds smoke tests for delegation + state preservation across run().

Changes:

Introduce Gremlin.state plus _s accessor and delegate properties/methods (client, artifacts, cwd, base_ref, progress/done helpers, etc.).
Extract three-way cwd resolution into _resolved_cwd and reuse it in _collect_stages and initialize_with_runtime.
Add smoke tests asserting delegation works after initialization and that gremlin.state remains canonical after run().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`gremlins/executor/gremlin.py`	Adds `state` storage + `_s` accessor and delegates; refactors cwd/base_ref wiring for stage-state construction and runtime initialization.
`tests/test_gremlin_smoke.py`	Adds smoke tests for state initialization/delegation and ensuring `gremlin.state` isn’t overwritten by `run()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

xbrianh · 2026-06-03T18:58:35Z

+    gremlin = Gremlin.initialize_with_runtime(
+        gremlin_id=gremlin_id,
+        state_dir=sd,
+        project_dir=project_dir,
+        pipeline_ref=str(pipeline_yaml),
+        project_root=str(project_dir),
+        base_ref="main",
+        repo="test/repo",
+    )
+
+    # Test that state is initialized after initialize_with_runtime
+    assert gremlin.state is not None
+
+    # Test property delegates
+    assert isinstance(gremlin.artifact_dir, pathlib.Path)
+    assert gremlin.artifact_dir == sd / "artifacts"
+    assert gremlin.artifacts is not None
+    assert isinstance(gremlin.artifacts, ArtifactRegistry)
+    assert gremlin.client is not None
+    assert isinstance(gremlin.cwd, str)
+    assert gremlin.base_ref == "main"
+    assert gremlin.repo == "test/repo"
+    assert gremlin.loop_iteration == 1
+    assert gremlin.attempt == ""


Fixed: Added cleanup to save/restore working directory and remove worktree after test.

xbrianh

Clean implementation — the _s guard, _resolved_cwd extraction, and the fix to stop overwriting self.state in the stage loop are all solid. One style note on the tests and a minor naming observation below.

xbrianh · 2026-06-03T18:55:27Z

+    # Test that state is initialized after initialize_with_runtime
+    assert gremlin.state is not None
+
+    # Test property delegates


These comments narrate what the assertions already say. Drop them — assert gremlin.state is not None and assert isinstance(gremlin.artifacts, ArtifactRegistry) are self-explanatory. Same for # Capture state values before run() (line 196) and # Assert state still reflects canonical values after run() (line 204) in the second test.

Done: Removed all redundant comments from both tests.

xbrianh · 2026-06-03T18:55:27Z

+    @property
+    def base_ref(self) -> str:
+        return self._s.base_ref
+


gremlin.worktree (State delegate, pathlib.Path | None) now shadows the name but reads from a different slot than gremlin.worktree_dir (the raw Gremlin attribute). After initialize_with_runtime they carry the same value, but having both names on the same object is a latent confusion. Worth documenting or unifying — callers reaching for one when they should use the other won't get an error, just a wrong-feeling access pattern.

Fixed: Renamed worktree_dir to _worktree_dir (internal only) to eliminate shadowing. Public API now uses state delegate (gremlin.worktree) consistently.

1. Add cleanup to test_gremlin_state_delegates_after_initialize: - Save/restore working directory with try/finally block - Clean up worktree directory to prevent state leakage across tests - Use gremlin.worktree instead of gremlin.worktree_dir 2. Remove redundant comments from both delegate tests: - Drop "# Test that state is initialized..." comments - Drop "# Capture state values..." and "# Assert state still..." comments - Let assertions speak for themselves 3. Resolve worktree_dir/worktree shadowing confusion: - Rename worktree_dir to _worktree_dir (internal only) - Update all internal references to use _worktree_dir - Update public API callers to use state delegate (gremlin.worktree) - Remove worktree_dir assertion from test_gremlin_open Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

xbrianh and others added 2 commits June 3, 2026 12:48

xbrianh requested a review from Copilot June 3, 2026 18:52

Copilot started reviewing on behalf of xbrianh June 3, 2026 18:52 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

xbrianh commented Jun 3, 2026

View reviewed changes

xbrianh closed this Jun 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add state attribute and property delegates to Gremlin, address review feedback#1097

Add state attribute and property delegates to Gremlin, address review feedback#1097
xbrianh wants to merge 3 commits into
mainfrom
issue-1075-state-delegates

xbrianh commented Jun 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

xbrianh Jun 3, 2026

Uh oh!

xbrianh left a comment

Uh oh!

xbrianh Jun 3, 2026

Uh oh!

xbrianh Jun 3, 2026

Uh oh!

xbrianh Jun 3, 2026

Uh oh!

xbrianh Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

xbrianh commented Jun 3, 2026

Summary

Changes

Test Coverage

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

xbrianh Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

xbrianh left a comment

Choose a reason for hiding this comment

Uh oh!

xbrianh Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

xbrianh Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

xbrianh Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

xbrianh Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants