Skip to content

fix: improve subprocess exception in git_utils for V3#5811

Merged
lucasjia-aws merged 4 commits into
aws:masterfrom
lucasjia-aws:master
Apr 30, 2026
Merged

fix: improve subprocess exception in git_utils for V3#5811
lucasjia-aws merged 4 commits into
aws:masterfrom
lucasjia-aws:master

Conversation

@lucasjia-aws

@lucasjia-aws lucasjia-aws commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Description

Improve error handling in git_utils.py for HTTPS clone operations. Subprocess exceptions now have URL details sanitized before propagating to callers, resulting in cleaner and more readable error messages when clone failures occur.

Changes

  • Added _redact_credentials_from_url utility function to clean up URL content in error output
  • Updated _run_clone_command with structured exception handling on the HTTPS path:
  • CalledProcessError is caught and re-raised with sanitized command args
  • Generic Exception catch-all with fallback to RuntimeError for non-standard exception types
  • Added 11 unit tests covering the new error handling behavior
  • SSH clone paths are unchanged (no URL sanitization needed)

Testing

  • All 35 unit tests pass (24 existing + 11 new), no regressions
  • Locally verified with a real git clone failure against an invalid HTTPS URL, confirmed error output is sanitized as expected

@lucasjia-aws lucasjia-aws changed the title fix: imporove subprocess exception in git_utils fix: improve subprocess exception in git_utils Apr 30, 2026
jam-jee
jam-jee previously approved these changes Apr 30, 2026
X-AI-Prompt: add catch-all exception handler to redact credentials
X-AI-Tool: kiro-cli
jam-jee
jam-jee previously approved these changes Apr 30, 2026
output=e.output,
stderr=e.stderr,
) from None
except Exception as e:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The catch-all except Exception as e uses type(e)(str(e).replace(repo_url, safe_url)) which assumes the exception type can be reconstructed from a single string argument. This could fail for custom exception types with different constructors. Consider wrapping in a secondary try/except or logging a warning.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point, adding in a new commit.


token_url = "https://ghp_secret123@github.com/user/repo.git"
with patch(
"subprocess.check_call",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mocked subprocess.check_call method is fine but we should consider adding an integration test with an actual invalid git URL

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We did test this locally with a real git clone against an actual invalid URL (https://myuser:ghp_SuperSecretToken123@github.com/nonexistent/repo-xyz) — no mocks. GitHub returned a real Authentication failed, and we confirmed the re-raised exception contains only with no trace of the original credentials.

We didn't add it as a formal integration test in CI because it depends on external network access to GitHub, which makes it flaky and environment-dependent — CI runners may have restricted outbound access, and GitHub rate limits or outages would cause false failures. The unit tests with mocked subprocess cover the same code paths deterministically.

Checked the existing integ test suite — there are no integration tests that perform real git clone operations against GitHub. All git_utils tests are unit tests with mocked subprocess. So not adding a network-dependent integ test is consistent with the current test approach.

nargokul
nargokul previously approved these changes Apr 30, 2026
X-AI-Prompt: add TypeError fallback for generic exception redaction
X-AI-Tool: kiro-cli
@piyushdaftary

Copy link
Copy Markdown

Looks good to me.

@jam-jee jam-jee self-requested a review April 30, 2026 20:28
@lucasjia-aws lucasjia-aws merged commit ed4b183 into aws:master Apr 30, 2026
18 of 23 checks passed
@lucasjia-aws lucasjia-aws changed the title fix: improve subprocess exception in git_utils fix: improve subprocess exception in git_utils for V3 Apr 30, 2026
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.

4 participants