feat: CMS autoTriage improvements and review feedback#283
Open
RossHastie wants to merge 2 commits intomicrosoft:mainfrom
Open
feat: CMS autoTriage improvements and review feedback#283RossHastie wants to merge 2 commits intomicrosoft:mainfrom
RossHastie wants to merge 2 commits intomicrosoft:mainfrom
Conversation
autoTriage: extract shared constants, refactor 360-line triage_issues into 3 helpers (212 lines), add type hints, replace emojis with text indicators, bounded caching with eviction, LLM rate limiting, input truncation, pin dependencies, add Dependabot config, GitHub Enterprise URL support. Feedback: add all CMS V1 review reports and consolidated findings register from 4 independent code review agents (security, architecture, code quality, evaluation methodology). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@RossHastie please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the autoTriage tooling by refactoring the intake pipeline, adding LLM call protections (rate limiting + input truncation/sanitization), improving caching behavior in GitHub API wrappers, and aligning user-facing output with repo conventions. It also adds operational documentation artifacts and enables Dependabot for Python dependency updates.
Changes:
- Refactors
triage_issues()by extracting helpers and standardizing outputs (including removing emoji status markers in autoTriage outputs). - Adds LLM request hardening: rolling-window rate limiter, consistent input truncation, and user-content sanitization before prompt interpolation.
- Replaces
@lru_cacheusage on GitHubService instance methods with a bounded, TTL-based module cache; pins Python dependencies and adds Dependabot config.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/cli/install-cli.ps1 | Adds strict mode, prerequisite validation, and comment-based help for CLI installation script. |
| scripts/cli/Auth/New-Agent365ToolsServicePrincipalProdPublic.ps1 | Improves robustness via strict mode and error handling for module install/import. |
| autoTriage/services/teams_service.py | Replaces emoji markers in Teams Adaptive Cards with text labels. |
| autoTriage/services/llm_service.py | Adds sanitization, truncation, and process-wide rate limiting; updates prompt formatting. |
| autoTriage/services/intake_service.py | Adds URL parsing via urlparse, refactors triage orchestration into helpers, and applies sanitization in Copilot paths. |
| autoTriage/services/github_service.py | Introduces bounded cache eviction and migrates repository label caching to TTL cache. |
| autoTriage/requirements.txt | Pins dependency versions and adds guidance for hash verification. |
| autoTriage/constants.py | Introduces shared constants module (e.g., contributors-to-show limit). |
| Feedback/*.md | Adds multiple review reports, consolidated findings, and verification documentation. |
| .github/dependabot.yml | Enables weekly Dependabot updates for /autoTriage pip dependencies. |
Comment on lines
+56
to
+90
| class RateLimiter: | ||
| """Simple token-bucket rate limiter for LLM API calls. | ||
|
|
||
| Tracks call timestamps over a rolling 60-second window and blocks the | ||
| calling thread when the configured limit would be exceeded. This is | ||
| intentionally synchronous because autoTriage uses synchronous I/O | ||
| throughout; no asyncio primitives are needed. | ||
| """ | ||
|
|
||
| def __init__(self, max_calls_per_minute: int = 60): | ||
| self._max_calls = max_calls_per_minute | ||
| self._calls: List[float] = [] | ||
|
|
||
| def wait_if_needed(self) -> None: | ||
| """Block until a call is allowed under the rate limit.""" | ||
| now = time.monotonic() | ||
| # Remove timestamps that have aged out of the 60-second window. | ||
| self._calls = [t for t in self._calls if now - t < 60] | ||
|
|
||
| if len(self._calls) >= self._max_calls: | ||
| # Sleep until the oldest recorded call falls outside the window. | ||
| sleep_time = 60 - (now - self._calls[0]) | ||
| if sleep_time > 0: | ||
| logger.info( | ||
| "LLM rate limit reached (%d/%d calls). Waiting %.1fs.", | ||
| len(self._calls), | ||
| self._max_calls, | ||
| sleep_time, | ||
| ) | ||
| time.sleep(sleep_time) | ||
| # Refresh the window after sleeping. | ||
| now = time.monotonic() | ||
| self._calls = [t for t in self._calls if now - t < 60] | ||
|
|
||
| self._calls.append(time.monotonic()) |
There was a problem hiding this comment.
Rate limiting behavior is newly introduced but there are no tests covering it (e.g., enforcing the rolling 60s window, and not sleeping when under the threshold). Given the existing pytest suite, consider adding unit tests for RateLimiter.wait_if_needed() using a controllable clock to keep tests fast and deterministic.
Comment on lines
+788
to
+789
| return IssueClassification( | ||
| issue_url=f"https://github.com/{owner}/{repo}/issues/{issue.number}", |
There was a problem hiding this comment.
_classify_single_issue() hardcodes issue_url to https://github.com/... which breaks the new GitHub Enterprise URL support (and can be wrong even on GitHub.com for forks/renames). Prefer using the URL from the issue object itself (e.g., issue.html_url) or build it from the parsed host when issue_url was provided.
Suggested change
| return IssueClassification( | |
| issue_url=f"https://github.com/{owner}/{repo}/issues/{issue.number}", | |
| # Prefer the canonical URL from the issue object (supports GitHub Enterprise), | |
| # and fall back to the GitHub.com URL pattern if not available. | |
| issue_url = getattr(issue, "html_url", None) or f"https://github.com/{owner}/{repo}/issues/{issue.number}" | |
| return IssueClassification( | |
| issue_url=issue_url, |
Comment on lines
18
to
24
| from services.llm_service import LlmService, MAX_ISSUE_BODY_LENGTH, _sanitise_user_content | ||
| from services.config_parser import ConfigParser | ||
| from services.teams_service import TeamsService | ||
| from services.copilot_service import CopilotService | ||
| from models.issue_classification import IssueClassification, TriageRationale | ||
|
|
||
|
|
There was a problem hiding this comment.
intake_service imports sanitise_user_content from llm_service even though it is a private (underscore-prefixed) helper. To avoid tight coupling and accidental breaking changes, consider moving this sanitization helper (and related MAX_ISSUE* constants) into a shared utility/constants module and exporting it as a public API.
Suggested change
| from services.llm_service import LlmService, MAX_ISSUE_BODY_LENGTH, _sanitise_user_content | |
| from services.config_parser import ConfigParser | |
| from services.teams_service import TeamsService | |
| from services.copilot_service import CopilotService | |
| from models.issue_classification import IssueClassification, TriageRationale | |
| from services.llm_service import LlmService, MAX_ISSUE_BODY_LENGTH | |
| from services.config_parser import ConfigParser | |
| from services.teams_service import TeamsService | |
| from services.copilot_service import CopilotService | |
| from models.issue_classification import IssueClassification, TriageRationale | |
| def _sanitise_user_content(content: Any) -> str: | |
| """ | |
| Sanitize user-provided content before sending it to downstream services. | |
| This helper is local to the intake service to avoid depending on | |
| private helpers from other modules. | |
| """ | |
| if not isinstance(content, str): | |
| return "" | |
| sanitized = content.strip() | |
| if len(sanitized) > MAX_ISSUE_BODY_LENGTH: | |
| sanitized = sanitized[:MAX_ISSUE_BODY_LENGTH] | |
| return sanitized |
Comment on lines
+27
to
+34
| Parse a GitHub or GitHub Enterprise issue URL to extract owner, repo, | ||
| and issue number. | ||
|
|
||
| Supports both github.com and GitHub Enterprise Server URLs of the form: | ||
| https://<host>/<owner>/<repo>/issues/<number> | ||
|
|
||
| URL parsing is used deliberately instead of a hardcoded domain check so | ||
| that any enterprise hostname is accepted without code changes. |
There was a problem hiding this comment.
_parse_issue_url() claims GitHub Enterprise support by accepting any hostname, but the parsed host is never used later and GitHubService is still hardwired to GitHub.com (PyGithub default). As-is, an enterprise URL will parse successfully but subsequent API calls will still target GitHub.com. Consider either (a) restricting parsing to github.com for now, or (b) plumbing the host through and configuring PyGithub base_url accordingly.
Comment on lines
+25
to
40
| def _parse_issue_url(issue_url: str) -> Optional[Tuple[str, str, int]]: | ||
| """ | ||
| Parse a GitHub issue URL to extract owner, repo, and issue number. | ||
| Parse a GitHub or GitHub Enterprise issue URL to extract owner, repo, | ||
| and issue number. | ||
|
|
||
| Supports both github.com and GitHub Enterprise Server URLs of the form: | ||
| https://<host>/<owner>/<repo>/issues/<number> | ||
|
|
||
| URL parsing is used deliberately instead of a hardcoded domain check so | ||
| that any enterprise hostname is accepted without code changes. | ||
|
|
||
| Args: | ||
| issue_url: URL like https://github.com/owner/repo/issues/123 | ||
| issue_url: A GitHub issue URL, e.g. | ||
| https://github.com/owner/repo/issues/123 | ||
| https://github.example.com/owner/repo/issues/123 | ||
|
|
There was a problem hiding this comment.
There are existing tests for _parse_issue_url(), but none validate the new GitHub Enterprise forms described in the docstring. Consider adding a test for an enterprise host (e.g., https://github.example.com/owner/repo/issues/123) and, if you decide to restrict accepted hosts/schemes, tests that ensure those are rejected.
Comment on lines
31
to
33
| Write-Host "⚠ IMPORTANT: This requires admin permissions!" -ForegroundColor Yellow | ||
| Write-Host "⚠ This only needs to be run ONCE per tenant!" -ForegroundColor Yellow | ||
| Write-Host "" |
There was a problem hiding this comment.
This script still prints emoji glyphs (e.g., ⚠, ✓, ✗). The repo convention in CLAUDE.md forbids emojis in logs/output; consider replacing these with text tokens like [WARNING]/[OK]/[FAILED] for consistency and for terminals/fonts that don’t render these reliably.
Comment on lines
+95
to
+98
| _rate_limiter = RateLimiter( | ||
| max_calls_per_minute=int(os.environ.get("LLM_MAX_CALLS_PER_MINUTE", "60")) | ||
| ) | ||
|
|
There was a problem hiding this comment.
The module-level rate limiter reads LLM_MAX_CALLS_PER_MINUTE with int(...). If the env var is set to a non-integer (e.g., empty or '60/min'), this will raise at import time and prevent the service from starting. Consider parsing defensively (try/except) and falling back to the default with a warning.
Suggested change
| _rate_limiter = RateLimiter( | |
| max_calls_per_minute=int(os.environ.get("LLM_MAX_CALLS_PER_MINUTE", "60")) | |
| ) | |
| _default_max_calls_per_minute = 60 | |
| _env_max_calls = os.environ.get("LLM_MAX_CALLS_PER_MINUTE") | |
| if _env_max_calls is not None: | |
| try: | |
| _max_calls_per_minute = int(_env_max_calls) | |
| except (TypeError, ValueError): | |
| logger.warning( | |
| "Invalid LLM_MAX_CALLS_PER_MINUTE value '%s'; falling back to default %d", | |
| _env_max_calls, | |
| _default_max_calls_per_minute, | |
| ) | |
| _max_calls_per_minute = _default_max_calls_per_minute | |
| else: | |
| _max_calls_per_minute = _default_max_calls_per_minute | |
| _rate_limiter = RateLimiter(max_calls_per_minute=_max_calls_per_minute) |
Comment on lines
+207
to
+216
| safe_title = _sanitise_user_content(title, MAX_ISSUE_TITLE_LENGTH) | ||
| safe_body = _sanitise_user_content(body) | ||
|
|
||
| user_prompt = self.prompts.format( | ||
| "classify_issue_user", | ||
| default=f"Classify this issue:\nTitle: {title}\nBody: {body}", | ||
| title=title, | ||
| body=body, | ||
| default=( | ||
| f"Classify this issue:\n" | ||
| f"<issue_title>{safe_title}</issue_title>\n" | ||
| f"<issue_body>{safe_body}</issue_body>" | ||
| ), |
There was a problem hiding this comment.
User-controlled title/body are wrapped in <issue_title>/<issue_body> tags, but _sanitise_user_content does not escape '<' or '>' characters. A malicious issue can inject closing tags or new tags, undermining the delimiter strategy. Consider escaping angle brackets (or serializing user content as JSON) before embedding it in the prompt.
- Sanitise exceptions to prevent token/key leakage in logs - Add XML entity escaping to defend against prompt injection - Bound _repo_cache with FIFO eviction at CACHE_MAX_ENTRIES - Guard rate limiter against negative sleep times - Validate LLM JSON output against allowlisted type/priority enums Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
triage_issues()function, deduplicated constants intoconstants.py, replaced emojis with text labels, added type hintsLLM_MAX_CALLS_PER_MINUTEconfig@lru_cacheon instance methods with bounded dict cache (CACHE_MAX_ENTRIES=100) and two-phase evictionMAX_ISSUE_TITLE_LENGTH=200,MAX_ISSUE_BODY_LENGTH=2000applied uniformlyurlparse-based URL handling instead of hardcoded regex.github/dependabot.ymlfor automated dependency updates23 files changed, +4,607/-292 lines
Test plan
🤖 Generated with Claude Code