docs: add Conductor context and learning workflow - #114
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📦 Bundle Size Analysis
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds valuable Conductor context documentation and a learning workflow system. However, there are 5 critical defects that will prevent the workflow from functioning correctly and could cause crashes or data loss.
Critical Issues Found
- GitHub Workflow Permission Error: Missing write permissions will cause silent failures when recording learning candidates
- HTTP Error Handling Gap: Unhandled exceptions can expose sensitive data in error messages
- Pagination Logic Bug: Infinite loop risk and silent result truncation in alert fetching
- Duplicate Detection Bug: Substring matching will cause false positives and block legitimate entries
- File I/O Error Handling: Missing exception handling for disk operations will cause workflow crashes
All issues have been marked with specific code suggestions for immediate resolution. The documentation additions are well-structured, but the Python scripts need these fixes before the learning workflow can operate reliably.
Required Actions
Address all 5 critical findings before merge. Each comment includes a commit-able code suggestion.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
🛑 Security Vulnerability: Missing write permissions for committing learning candidates. The workflow attempts to modify conductor/improvement-backlog.md via record_learning_candidate.py (line 94 writes to the file), but only has contents: read permission. This will cause the workflow to fail silently when the script attempts to write the file, making the learning workflow ineffective.
| permissions: | |
| contents: read | |
| permissions: | |
| contents: write |
| def _http_get_json(url: str, token: str | None) -> tuple[Any, dict[str, str]]: | ||
| request = urllib.request.Request( # noqa: S310 | ||
| url, | ||
| headers={ | ||
| "Accept": "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| **({"Authorization": f"Bearer {token}"} if token else {}), | ||
| }, | ||
| ) | ||
| with urllib.request.urlopen(request, timeout=30) as response: # noqa: S310 | ||
| payload = response.read().decode("utf-8") | ||
| return json.loads(payload), dict(response.headers.items()) |
There was a problem hiding this comment.
🛑 Crash Risk: Missing exception handling for HTTP errors. When _http_get_json encounters HTTP errors (4xx/5xx), urllib.request.urlopen raises exceptions that propagate with sensitive data. The noqa comment acknowledges security concerns but doesn't address the exception handling gap, which can expose internal URLs and tokens in error messages.
| def _http_get_json(url: str, token: str | None) -> tuple[Any, dict[str, str]]: | |
| request = urllib.request.Request( # noqa: S310 | |
| url, | |
| headers={ | |
| "Accept": "application/vnd.github+json", | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| **({"Authorization": f"Bearer {token}"} if token else {}), | |
| }, | |
| ) | |
| with urllib.request.urlopen(request, timeout=30) as response: # noqa: S310 | |
| payload = response.read().decode("utf-8") | |
| return json.loads(payload), dict(response.headers.items()) | |
| def _http_get_json(url: str, token: str | None) -> tuple[Any, dict[str, str]]: | |
| request = urllib.request.Request( # noqa: S310 | |
| url, | |
| headers={ | |
| "Accept": "application/vnd.github+json", | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| **({"Authorization": f"Bearer {token}"} if token else {}), | |
| }, | |
| ) | |
| try: | |
| with urllib.request.urlopen(request, timeout=30) as response: # noqa: S310 | |
| payload = response.read().decode("utf-8") | |
| except urllib.error.HTTPError as exc: | |
| msg = f"HTTP {exc.code} error accessing GitHub API" | |
| raise RuntimeError(msg) from exc | |
| return json.loads(payload), dict(response.headers.items()) |
| def iter_open_alerts( | ||
| repository: str, | ||
| token: str | None, | ||
| *, | ||
| per_page: int = DEFAULT_PER_PAGE, | ||
| ) -> list[dict[str, Any]]: | ||
| alerts: list[dict[str, Any]] = [] | ||
| for page in range(1, 101): | ||
| page_alerts = _fetch_page(repository, token, page, per_page) | ||
| alerts.extend(page_alerts) | ||
| if len(page_alerts) < per_page: | ||
| break | ||
| return alerts |
There was a problem hiding this comment.
🛑 Logic Error: Infinite loop risk in pagination. When API returns exactly 100 alerts per page repeatedly, the loop continues indefinitely without checking the deadline. The hardcoded range(1, 101) also arbitrarily limits maximum alerts to 10,000, silently truncating results beyond this limit.
| def iter_open_alerts( | |
| repository: str, | |
| token: str | None, | |
| *, | |
| per_page: int = DEFAULT_PER_PAGE, | |
| ) -> list[dict[str, Any]]: | |
| alerts: list[dict[str, Any]] = [] | |
| for page in range(1, 101): | |
| page_alerts = _fetch_page(repository, token, page, per_page) | |
| alerts.extend(page_alerts) | |
| if len(page_alerts) < per_page: | |
| break | |
| return alerts | |
| def iter_open_alerts( | |
| repository: str, | |
| token: str | None, | |
| *, | |
| per_page: int = DEFAULT_PER_PAGE, | |
| max_pages: int = 100, | |
| ) -> list[dict[str, Any]]: | |
| alerts: list[dict[str, Any]] = [] | |
| for page in range(1, max_pages + 1): | |
| page_alerts = _fetch_page(repository, token, page, per_page) | |
| if not page_alerts: | |
| break | |
| alerts.extend(page_alerts) | |
| if len(page_alerts) < per_page: | |
| break | |
| return alerts |
| def section_has_entry(section_lines: list[str], candidate_lines: list[str]) -> bool: | ||
| if not section_lines: | ||
| return False | ||
| raw = "\n".join(section_lines) | ||
| return "\n".join(candidate_lines) in raw |
There was a problem hiding this comment.
🛑 Logic Error: Race condition in file modification check. The section_has_entry function uses substring matching on joined lines, which will produce false positives when one candidate message is a substring of another. For example, "CI failure for Test" will match "CI failure for Test Suite", preventing legitimate new entries.
| def section_has_entry(section_lines: list[str], candidate_lines: list[str]) -> bool: | |
| if not section_lines: | |
| return False | |
| raw = "\n".join(section_lines) | |
| return "\n".join(candidate_lines) in raw | |
| def section_has_entry(section_lines: list[str], candidate_lines: list[str]) -> bool: | |
| if not section_lines or not candidate_lines: | |
| return False | |
| # Match the complete checklist item, not substrings | |
| target = candidate_lines[0].strip() | |
| for line in section_lines: | |
| if line.strip() == target: | |
| return True | |
| return False |
| def append_candidate(path: Path, message: str, evidence: List[str]) -> Tuple[bool, List[str]]: | ||
| lines = read_text_lines(path) | ||
| candidate_lines = build_entry_lines(message, evidence) | ||
|
|
||
| start = find_section(lines, SECTION_HEADING) | ||
| if start == -1: | ||
| lines.extend(["", SECTION_HEADING, ""]) | ||
| start = len(lines) - 1 | ||
|
|
||
| end = len(lines) | ||
| for idx in range(start + 1, len(lines)): | ||
| if lines[idx].startswith("## "): | ||
| end = idx | ||
| break | ||
|
|
||
| section_body = lines[start + 1 : end] | ||
| if section_has_entry(section_body, candidate_lines): | ||
| return False, candidate_lines | ||
|
|
||
| insert_at = end | ||
| if section_body and section_body[-1].strip(): | ||
| candidate_lines = [""] + candidate_lines | ||
|
|
||
| updated_lines = lines[:insert_at] + candidate_lines + lines[insert_at:] | ||
| output = "\n".join(updated_lines).rstrip() + "\n" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_text(output, encoding="utf-8") | ||
| return True, candidate_lines |
There was a problem hiding this comment.
🛑 Crash Risk: Missing error handling for file write operations. The append_candidate function performs file I/O operations (lines 93-94) without catching exceptions. If the file system is read-only or disk is full, the workflow will crash without recording the learning candidate, losing valuable debugging information.
| def append_candidate(path: Path, message: str, evidence: List[str]) -> Tuple[bool, List[str]]: | |
| lines = read_text_lines(path) | |
| candidate_lines = build_entry_lines(message, evidence) | |
| start = find_section(lines, SECTION_HEADING) | |
| if start == -1: | |
| lines.extend(["", SECTION_HEADING, ""]) | |
| start = len(lines) - 1 | |
| end = len(lines) | |
| for idx in range(start + 1, len(lines)): | |
| if lines[idx].startswith("## "): | |
| end = idx | |
| break | |
| section_body = lines[start + 1 : end] | |
| if section_has_entry(section_body, candidate_lines): | |
| return False, candidate_lines | |
| insert_at = end | |
| if section_body and section_body[-1].strip(): | |
| candidate_lines = [""] + candidate_lines | |
| updated_lines = lines[:insert_at] + candidate_lines + lines[insert_at:] | |
| output = "\n".join(updated_lines).rstrip() + "\n" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| path.write_text(output, encoding="utf-8") | |
| return True, candidate_lines | |
| def append_candidate(path: Path, message: str, evidence: List[str]) -> Tuple[bool, List[str]]: | |
| lines = read_text_lines(path) | |
| candidate_lines = build_entry_lines(message, evidence) | |
| start = find_section(lines, SECTION_HEADING) | |
| if start == -1: | |
| lines.extend(["", SECTION_HEADING, ""]) | |
| start = len(lines) - 1 | |
| end = len(lines) | |
| for idx in range(start + 1, len(lines)): | |
| if lines[idx].startswith("## "): | |
| end = idx | |
| break | |
| section_body = lines[start + 1 : end] | |
| if section_has_entry(section_body, candidate_lines): | |
| return False, candidate_lines | |
| insert_at = end | |
| if section_body and section_body[-1].strip(): | |
| candidate_lines = [""] + candidate_lines | |
| updated_lines = lines[:insert_at] + candidate_lines + lines[insert_at:] | |
| output = "\n".join(updated_lines).rstrip() + "\n" | |
| try: | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| path.write_text(output, encoding="utf-8") | |
| except (OSError, PermissionError) as exc: | |
| msg = f"Failed to write to {path}: {exc}" | |
| raise RuntimeError(msg) from exc | |
| return True, candidate_lines |
📦 Bundle Size Analysis
|
📦 Bundle Size Analysis
|
What changed
Add the project Conductor context, reusable track templates, learning-candidate workflow, and ANZ provider/source tracks.
Why
Make the repository's delivery context and improvement workflow explicit and maintainable.
Validation