Skip to content
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

🤖 Improve error handling and state management in commit_changes #1981

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

🤖 Improve error handling and state management in commit_changes #1981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
504630b
Improve error handling and state management in `commit_changes`
sentry-autofix[bot] Feb 21, 2025
a72bc42
:hammer_and_wrench: apply pre-commit fixes
getsantry[bot] Feb 21, 2025
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
185 changes: 98 additions & 87 deletions src/seer/automation/autofix/autofix_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,13 @@ def commit_changes(
state = self.state.get()
for codebase_state in state.codebases.values():
if repo_external_id is None or codebase_state.repo_external_id == repo_external_id:
# Fetch changes step by key instead of using stored index
changes_step = state.find_step(key="changes")
if not changes_step:
raise ValueError("Changes step not found")
changes_step = cast(ChangesStep, changes_step)

# Find the change state for this repo
change_state, changes_state_index = next(
(
(change, i)
Expand All @@ -217,105 +220,113 @@ def commit_changes(
),
(None, None),
)
if codebase_state.file_changes and change_state and changes_state_index is not None:
key = codebase_state.repo_external_id
if not (
codebase_state.file_changes and change_state and changes_state_index is not None
):
logger.warning(
f"No valid changes found for repo {codebase_state.repo_external_id}"
)
continue

if key is None:
raise ValueError("Repo key not found")
key = codebase_state.repo_external_id
if key is None:
raise ValueError("Repo key not found")

repo_definition = self.repos_by_key().get(key)
repo_definition = self.repos_by_key().get(key)
if repo_definition is None:
raise ValueError(f"Repo definition not found for key {key}")

if repo_definition is None:
raise ValueError(f"Repo definition not found for key {key}")
repo_client = self.get_repo_client(
repo_external_id=repo_definition.external_id, type=RepoClientType.WRITE
)

repo_client = self.get_repo_client(
repo_external_id=repo_definition.external_id, type=RepoClientType.WRITE
)
branch_name = f"autofix/{change_state.title}"
branch_ref = repo_client.create_branch_from_changes(
pr_title=change_state.title,
file_patches=change_state.diff,
branch_name=branch_name,
)

if branch_ref is None:
logger.warning("Failed to create branch from changes")
return None

branch_name = f"autofix/{change_state.title}"
branch_ref = repo_client.create_branch_from_changes(
pr_title=change_state.title,
file_patches=change_state.diff,
branch_name=branch_name,
# Update branch name by re-fetching the changes step
with self.state.update() as state:
cs = state.find_step(key="changes")
if not cs or not isinstance(cs, ChangesStep):
raise ValueError("Changes step not found or invalid during update")
cs.changes[changes_state_index].branch_name = branch_ref.ref.replace(
"refs/heads/", ""
)

if branch_ref is None:
logger.warning("Failed to create branch from changes")
return None

with self.state.update() as state:
step = cast(ChangesStep, state.steps[changes_step.index])
step.changes[changes_state_index].branch_name = branch_ref.ref.replace(
"refs/heads/", ""
)
if not make_pr:
return

pr_title = f"""🤖 {change_state.title}"""

ref_note = ""
org_slug = self.get_org_slug(state.request.organization_id)
if org_slug:
issue_url = f"https://sentry.io/organizations/{org_slug}/issues/{state.request.issue.id}/"
issue_link = (
f"[{state.request.issue.short_id}]({issue_url})"
if state.request.issue.short_id
else issue_url
)
suspect_pr_link = (
f", which was likely introduced in [this PR]({pr_to_comment_on_url})."
if pr_to_comment_on_url
else ""
)
ref_note = f"Fixes {issue_link}{suspect_pr_link}\n"

pr_description = textwrap.dedent(
"""\
👋 Hi there! This PR was automatically generated by Autofix 🤖
{user_line}

{ref_note}
{description}

If you have any questions or feedback for the Sentry team about this fix, please email [[email protected]](mailto:[email protected]) with the Run ID: {run_id}."""
).format(
run_id=state.run_id,
user_line=(
f"\nThis fix was triggered by {state.request.invoking_user.display_name}"
if state.request.invoking_user
else ""
),
description=change_state.description,
ref_note=ref_note,
if not make_pr:
return

# Rest of the PR creation code...
pr_title = f"""🤖 {change_state.title}"""

ref_note = ""
org_slug = self.get_org_slug(state.request.organization_id)
if org_slug:
issue_url = f"https://sentry.io/organizations/{org_slug}/issues/{state.request.issue.id}/"
issue_link = (
f"[{state.request.issue.short_id}]({issue_url})"
if state.request.issue.short_id
else issue_url
)
suspect_pr_link = (
f", which was likely introduced in [this PR]({pr_to_comment_on_url})."
if pr_to_comment_on_url
else ""
)
ref_note = f"Fixes {issue_link}{suspect_pr_link}\n"

pr_description = textwrap.dedent(
"""\
👋 Hi there! This PR was automatically generated by Autofix 🤖
{user_line}

{ref_note}
{description}

If you have any questions or feedback for the Sentry team about this fix, please email [[email protected]](mailto:[email protected]) with the Run ID: {run_id}."""
).format(
run_id=state.run_id,
user_line=(
f"\nThis fix was triggered by {state.request.invoking_user.display_name}"
if state.request.invoking_user
else ""
),
description=change_state.description,
ref_note=ref_note,
)

pr = repo_client.create_pr_from_branch(branch_ref, pr_title, pr_description)

pr = repo_client.create_pr_from_branch(branch_ref, pr_title, pr_description)
change_state.pull_request = CommittedPullRequestDetails(
pr_number=pr.number, pr_url=pr.html_url, pr_id=pr.id
)

with self.state.update() as state:
step = cast(ChangesStep, state.steps[changes_step.index])
step.changes[changes_state_index].pull_request = change_state.pull_request

change_state.pull_request = CommittedPullRequestDetails(
pr_number=pr.number, pr_url=pr.html_url, pr_id=pr.id
with Session() as session:
pr_id_mapping = DbPrIdToAutofixRunIdMapping(
provider=repo_client.provider,
pr_id=pr.id,
run_id=state.run_id,
)
session.add(pr_id_mapping)
session.commit()

with self.state.update() as state:
step = cast(ChangesStep, state.steps[changes_step.index])
step.changes[changes_state_index].pull_request = change_state.pull_request

with Session() as session:
pr_id_mapping = DbPrIdToAutofixRunIdMapping(
provider=repo_client.provider,
pr_id=pr.id,
run_id=state.run_id,
)
session.add(pr_id_mapping)
session.commit()

if (
pr_to_comment_on_url
): # for GitHub Copilot, leave a comment that the PR is made
repo_client.comment_pr_generated_for_copilot(
pr_to_comment_on_url=pr_to_comment_on_url,
new_pr_url=pr.html_url,
run_id=state.run_id,
)
if pr_to_comment_on_url: # for GitHub Copilot, leave a comment that the PR is made
repo_client.comment_pr_generated_for_copilot(
pr_to_comment_on_url=pr_to_comment_on_url,
new_pr_url=pr.html_url,
run_id=state.run_id,
)

def comment_root_cause_on_pr(
self, pr_url: str, repo_definition: RepoDefinition, root_cause: str
Expand Down
70 changes: 70 additions & 0 deletions tests/automation/autofix/test_autofix_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,76 @@ def test_process_stacktrace_paths_unknown_object_exception(self, mock_RepoClient
should_completely_error=True,
)

def test_commit_changes_handles_missing_step(self):
"""Test that commit_changes handles missing changes step gracefully"""
context = self.autofix_context

# Set up state without changes step
with context.state.update() as state:
state.steps = [] # Empty steps list

with self.assertRaises(ValueError, match="Changes step not found"):
context.commit_changes()

def test_commit_changes_refetches_step(self):
"""Test that commit_changes correctly refetches changes step when updating state"""
context = self.autofix_context

# Set up initial state with changes step
changes_step = ChangesStep(
key="changes",
title="Test Changes",
changes=[
CodebaseChange(
repo_external_id="test-repo",
repo_name="test/repo",
title="Test Change",
description="Test",
diff=[],
)
],
)

with context.state.update() as state:
state.steps = [changes_step]
state.codebases["test-repo"] = CodebaseState(
repo_external_id="test-repo",
file_changes=[FileChange(path="test.py", content="test")],
)

self.mock_repo_client.create_branch_from_changes.return_value = MagicMock(
ref="refs/heads/test-branch"
)

# Call commit_changes
context.commit_changes(repo_external_id="test-repo")

# Verify changes step was refetched and updated correctly
state = context.state.get()
updated_step = state.find_step(key="changes")
self.assertIsNotNone(updated_step)
self.assertIsInstance(updated_step, ChangesStep)
self.assertEqual(updated_step.changes[0].branch_name, "test-branch")

def test_commit_changes_handles_invalid_changes_state(self):
"""Test that commit_changes handles invalid changes state index gracefully"""
context = self.autofix_context

# Set up state with changes step but invalid change state
changes_step = ChangesStep(
key="changes", title="Test Changes", changes=[] # Empty changes list
)

with context.state.update() as state:
state.steps = [changes_step]
state.codebases["test-repo"] = CodebaseState(
repo_external_id="test-repo",
file_changes=[FileChange(path="test.py", content="test")],
)

# Should not raise IndexError
context.commit_changes(repo_external_id="test-repo") # Should log warning and continue


class TestAutofixContextPrCommit(unittest.TestCase):
def setUp(self):
Expand Down
Loading