Skip to content

feat: implement Prompt.save() with automatic dirty tracking for name changes #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thiagobomfin-galileo wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: implement Prompt.save() with automatic dirty tracking for name changes #504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b18565
feat(future): implement Prompt.save() for DIRTY state
thiagobomfin-galileo Mar 13, 2026
8f14053
test(future): add TestPromptSave covering dirty tracking and save() d…
thiagobomfin-galileo Mar 13, 2026
b1e89cf
fix CI
thiagobomfin-galileo Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions src/galileo/__future__/prompt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ class Prompt(StateManagementMixin):
prompt.delete()
"""

# Fields tracked for dirty-state detection (name changes → DIRTY → save() calls update())
_TRACKED_FIELDS: frozenset[str] = frozenset({"name"})

# Type annotations for instance attributes
id: str | None
name: str
Expand All @@ -203,6 +206,17 @@ def __repr__(self) -> str:
version_info = f", version={self.selected_version_number}" if self.selected_version_number else ""
return f"Prompt(name='{self.name}', id='{self.id}', messages={len(self.messages)} messages{version_info})"

def __setattr__(self, attr_name: str, value: Any) -> None:
"""Track mutations to name and transition to DIRTY state when synced."""
if attr_name in self._TRACKED_FIELDS and hasattr(self, "_sync_state") and self._sync_state == SyncState.SYNCED:
try:
current = object.__getattribute__(self, attr_name)
except AttributeError:
current = object()
if current != value:
self._set_state(SyncState.DIRTY)
object.__setattr__(self, attr_name, value)

Comment on lines +209 to +219
Copy link
Contributor

@baz-reviewer baz-reviewer bot Mar 13, 2026

Choose a reason for hiding this comment

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

Prompt.setattr flips a synced object's state to DIRTY on any assignment to tracked fields without checking equality, so prompt.name = prompt.name becomes DIRTY and save() (lines 775–788) then calls update(name=self.name), issuing an unnecessary API rename. Can we compare the new value to the current one before flipping the sync state so redundant assignments don't trigger updates?

Finding type: Logical Bugs | Severity: 🟠 Medium

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents:

In src/galileo/__future__/prompt.py around lines 209-214, the __setattr__ method
unconditionally marks tracked fields DIRTY when synced. Change it to first obtain the
current value via object.__getattribute__(self, attr_name) (or use a unique sentinel
with getattr) and compare it to the new value; only call
self._set_state(SyncState.DIRTY) if the values differ. Ensure this comparison handles
the missing-attribute case safely (treat missing as different) and then delegate to
object.__setattr__ to assign the value.

Copy link
Contributor

@baz-reviewer baz-reviewer bot Mar 18, 2026

Choose a reason for hiding this comment

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

Commit b1e89cf addressed this comment by checking the current tracked attribute value and only marking the prompt DIRTY if the new value differs, preventing redundant rename updates.

def __init__(
self,
name: str | None = None,
Expand Down Expand Up @@ -291,7 +305,7 @@ def _update_from_api_response(self, retrieved_prompt: Any) -> None:
retrieved_prompt: The prompt data retrieved from the API.
"""
self.id = retrieved_prompt.id
self.name = retrieved_prompt.name
object.__setattr__(self, "name", retrieved_prompt.name)

Comment on lines 306 to 309
Copy link
Contributor

@baz-reviewer baz-reviewer bot Mar 13, 2026

Choose a reason for hiding this comment

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

What's wrong: Both _update_from_api_response and update() now manually call object.__setattr__(self, "name", …) to bypass the new dirty-tracking override when syncing the name. The same three-line pattern appears twice (lines 301‑304 and again around 506‑511), so any future tracked field or additional sync flow will need the same boilerplate.
Impact: We risk forgetting to bypass the overridden __setattr__ in new code paths, which would incorrectly mark the prompt as DIRTY and block save()/update() operations or cause redundant API calls.
Ask: Can we extract a helper (e.g., _set_synced_name(self, name: str) or a generic _assign_tracked_attr(attr, value)) and call it from both _update_from_api_response and update() so the bypass logic lives in one place and isn’t duplicated?

Finding type: Code Dedup and Conventions | Severity: 🟢 Low

Want Baz to fix this for you? Activate Fixer

# Extract messages from the selected_version template using helper
self.messages = _parse_template_to_messages(retrieved_prompt.selected_version.template)
Expand Down Expand Up @@ -497,8 +511,8 @@ def update(self, *, name: str) -> Prompt:
logger.info(f"Prompt.update: id='{self.id}' name='{name}' - started")
prompt_service = GlobalPromptTemplates()
updated_prompt = prompt_service.update(template_id=self.id, name=name)
# Update our instance attributes
self.name = updated_prompt.name
# Update our instance attributes (bypass dirty tracking — this is an internal sync)
object.__setattr__(self, "name", updated_prompt.name)
self.updated_at = updated_prompt.updated_at
# Set state to synced after successful update
self._set_state(SyncState.SYNCED)
Expand Down Expand Up @@ -764,19 +778,18 @@ def save(self) -> Prompt:
prompt.update(name="renamed-prompt")
"""
if self.sync_state == SyncState.LOCAL_ONLY:
# Prompt hasn't been created yet, create it
return self.create()
if self.sync_state == SyncState.SYNCED:
# Already synced, nothing to do
logger.debug(f"Prompt.save: id='{self.id}' - already synced, no action needed")
return self
if self.sync_state == SyncState.DELETED:
raise ValueError("Cannot save a deleted prompt.")
if self.sync_state == SyncState.FAILED_SYNC:
raise ValueError(
"Prompt is in FAILED_SYNC state from a previous operation. "
"Use refresh() to re-sync from the API, or retry the original operation."
)

# DIRTY or FAILED_SYNC states
# For now, we don't track dirty state for prompts
# Users should use update() directly for changes
raise NotImplementedError(
"Saving modified prompts is not yet implemented. "
"Use update(name='...') to rename or create_version(messages=[...]) to create a new version."
)
if self.id is None:
raise ValueError("Prompt ID is not set. Cannot update a prompt without an ID.")
return self.update(name=self.name)
145 changes: 145 additions & 0 deletions tests/future/test_prompt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,151 @@ def test_repr_includes_version_when_available(self, reset_configuration: None) -
assert "version=3" in repr(prompt)


class TestPromptSave:
"""Test suite for Prompt.save() — focused on the DIRTY branch and dirty tracking."""

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_save_dirty_calls_update_and_syncs_attributes(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a synced prompt whose name was changed via attribute assignment
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt

updated_response = MagicMock()
updated_response.configure_mock(name="Renamed Prompt")
updated_response.updated_at = MagicMock()
mock_service.update.return_value = updated_response

prompt = Prompt.get(id=mock_prompt.id)
assert prompt.is_synced()

# When: name is reassigned (triggers DIRTY) and save() is called
prompt.name = "Renamed Prompt"
assert prompt.sync_state == SyncState.DIRTY

result = prompt.save()

# Then: update is called with the new name and state is synced
mock_service.update.assert_called_once_with(template_id=mock_prompt.id, name="Renamed Prompt")
assert result.name == "Renamed Prompt"
assert result.is_synced()

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_save_without_id_raises_value_error(
self, mock_templates_class: MagicMock, reset_configuration: None
) -> None:
# Given: a prompt in DIRTY state with no ID
messages = [Message(role=MessageRole.user, content="{{input}}")]
prompt = Prompt(name="Test Prompt", messages=messages)
prompt._set_state(SyncState.DIRTY)

# When/Then: save() raises ValueError because there is no ID to update
with pytest.raises(ValueError, match="Prompt ID is not set"):
prompt.save()

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_save_handles_api_failure(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a dirty prompt and an API that raises an error
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt
mock_service.update.side_effect = RuntimeError("API error")

prompt = Prompt.get(id=mock_prompt.id)
prompt._set_state(SyncState.DIRTY)

# When/Then: the exception propagates and state is FAILED_SYNC
with pytest.raises(RuntimeError, match="API error"):
prompt.save()

assert prompt.sync_state == SyncState.FAILED_SYNC

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_name_assignment_transitions_synced_to_dirty(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a synced prompt
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt

prompt = Prompt.get(id=mock_prompt.id)
assert prompt.is_synced()

# When: name is reassigned
prompt.name = "New Name"

# Then: state transitions to DIRTY
assert prompt.sync_state == SyncState.DIRTY

def test_name_assignment_during_init_stays_local_only(self, reset_configuration: None) -> None:
# Given/When: a prompt is created locally (name set in __init__)
messages = [Message(role=MessageRole.user, content="{{input}}")]
prompt = Prompt(name="My Prompt", messages=messages)

# Then: state is LOCAL_ONLY, not DIRTY
assert prompt.sync_state == SyncState.LOCAL_ONLY

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_same_value_assignment_stays_synced(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a synced prompt
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt

prompt = Prompt.get(id=mock_prompt.id)
assert prompt.is_synced()
original_name = prompt.name

# When: name is reassigned to the same value
prompt.name = original_name

# Then: state stays SYNCED (no redundant API call on save)
assert prompt.sync_state == SyncState.SYNCED

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_save_failed_sync_raises_value_error(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a prompt in FAILED_SYNC state from a failed operation (e.g. create_version)
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt

prompt = Prompt.get(id=mock_prompt.id)
prompt._set_state(SyncState.FAILED_SYNC)

# When/Then: save() raises ValueError instead of silently masking the failure
with pytest.raises(ValueError, match="FAILED_SYNC"):
prompt.save()

@patch("galileo.__future__.prompt.GlobalPromptTemplates")
def test_refresh_does_not_trigger_dirty(
self, mock_templates_class: MagicMock, reset_configuration: None, mock_prompt: MagicMock
) -> None:
# Given: a synced prompt
mock_service = MagicMock()
mock_templates_class.return_value = mock_service
mock_service.get.return_value = mock_prompt

prompt = Prompt.get(id=mock_prompt.id)
assert prompt.is_synced()

# When: _update_from_api_response is called (as refresh() does internally)
mock_prompt.configure_mock(name="Updated From API")
prompt._update_from_api_response(mock_prompt)

# Then: state remains SYNCED (object.__setattr__ bypass works)
assert prompt.sync_state == SyncState.SYNCED
assert prompt.name == "Updated From API"


class TestJsonTemplateParsing:
"""Test suite for JSON string template parsing."""

Expand Down
Loading