Promote the deployed TerraGate v2 release - #2
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to the TerraGate platform, focusing on enhancing the public demo experience, refining the risk scoring model, and improving the frontend dashboard's accessibility and operational workflows. Key changes include the addition of a /runtime-capabilities endpoint to dynamically report active environment features, a bounded and explainable risk scoring algorithm, and a comprehensive showcase utility to simulate production-style reviews. Feedback on the code changes highlights a type mismatch where user.email and user.role are accessed on AuthUser but are missing from its schema, potential runtime crashes when calling .lower() on a null langsmith_tracing setting or when runbook_checklist is undefined, a validation bug where untrimmed GitHub context strings are matched against regex, and a cross-browser compatibility issue with the beforeunload event handler.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return [ | ||
| { label: "Frontend", value: frontendTarget, detail: frontendHost, tone: "success", icon: Cloud }, | ||
| { label: "API", value: apiTarget, detail: `${capabilities.environment} / ${capabilities.review_execution_mode} execution`, tone: "success", icon: Server }, | ||
| { label: "Identity", value: capabilities.auth_provider === "cognito" ? "Amazon Cognito" : "Development identity", detail: user ? `${user.email} / ${user.role}` : "Identity unavailable", tone: capabilities.auth_provider === "cognito" ? "success" : "warn", icon: KeyRound }, |
There was a problem hiding this comment.
The AuthUser type defined in apps/web/types/api.ts (and the corresponding AuthUser Pydantic schema in apps/api/app/schemas/review.py) only contains the auth_provider field. Accessing user.email and user.role here (and in other components like ApprovalCenter and NewReviewForm) will cause TypeScript compilation errors and runtime undefined values. Please update the AuthUser type/schema definitions to include the email and role fields.
| tracing_enabled = bool(settings.langsmith_api_key) and settings.langsmith_tracing.lower() in { | ||
| "1", | ||
| "true", | ||
| "yes", | ||
| "on", | ||
| } |
There was a problem hiding this comment.
If settings.langsmith_api_key is configured but settings.langsmith_tracing is not set (or is None), calling .lower() on it will raise an AttributeError and crash the endpoint. Safely convert the value to a string or check for None before calling .lower().
| tracing_enabled = bool(settings.langsmith_api_key) and settings.langsmith_tracing.lower() in { | |
| "1", | |
| "true", | |
| "yes", | |
| "on", | |
| } | |
| tracing_enabled = bool(settings.langsmith_api_key) and str(settings.langsmith_tracing or "").lower() in { | |
| "1", | |
| "true", | |
| "yes", | |
| "on", | |
| } |
| function validateGitHubContext(owner: string, repo: string, pullNumber: string, required = false): string | null { | ||
| const values = [owner.trim(), repo.trim(), pullNumber.trim()]; | ||
| if (!required && values.every((value) => !value)) return null; | ||
| if (values.some((value) => !value)) return "Enter the repository owner, repository name, and pull request number together."; | ||
| if (!/^[A-Za-z0-9_.-]+$/.test(owner) || !/^[A-Za-z0-9_.-]+$/.test(repo)) { | ||
| return "Repository owner and name may contain letters, numbers, periods, underscores, and hyphens."; | ||
| } | ||
| const parsedPull = Number(pullNumber); | ||
| if (!Number.isInteger(parsedPull) || parsedPull < 1) return "Pull request number must be a positive whole number."; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The validation checks on line 545 are performed on the original owner and repo arguments instead of their trimmed counterparts from the values array. If a user accidentally includes leading or trailing whitespace, the validation will fail with a regex mismatch error even though the trimmed values are perfectly valid. Update the function to use the trimmed values for all validation checks.
function validateGitHubContext(owner: string, repo: string, pullNumber: string, required = false): string | null {
const trimmedOwner = owner.trim();
const trimmedRepo = repo.trim();
const trimmedPull = pullNumber.trim();
const values = [trimmedOwner, trimmedRepo, trimmedPull];
if (!required && values.every((value) => !value)) return null;
if (values.some((value) => !value)) return "Enter the repository owner, repository name, and pull request number together.";
if (!/^[A-Za-z0-9_.-]+$/.test(trimmedOwner) || !/^[A-Za-z0-9_.-]+$/.test(trimmedRepo)) {
return "Repository owner and name may contain letters, numbers, periods, underscores, and hyphens.";
}
const parsedPull = Number(trimmedPull);
if (!Number.isInteger(parsedPull) || parsedPull < 1) return "Pull request number must be a positive whole number.";
return null;
}
| const warnBeforeUnload = (event: BeforeUnloadEvent) => { | ||
| event.preventDefault(); | ||
| }; |
There was a problem hiding this comment.
In many modern browsers (such as Google Chrome and Microsoft Edge), calling event.preventDefault() alone on a beforeunload event is not sufficient to trigger the unsaved changes confirmation dialog. You must also set event.returnValue to an empty string to ensure cross-browser compatibility.
| const warnBeforeUnload = (event: BeforeUnloadEvent) => { | |
| event.preventDefault(); | |
| }; | |
| const warnBeforeUnload = (event: BeforeUnloadEvent) => { | |
| event.preventDefault(); | |
| event.returnValue = ""; | |
| }; |
|
|
||
| function buildRunbookItems(selected: Finding | null, findings: Finding[], run: RunDetail | null): string[] { | ||
| const selectedItems = selected?.runbook_checklist ?? []; | ||
| const fallbackFindingItems = findings.flatMap((finding) => finding.runbook_checklist).slice(0, 6); |
There was a problem hiding this comment.
If finding.runbook_checklist is optional or missing from the API response, calling flatMap directly on it will throw a runtime error. Use nullish coalescing (?? []) to safely default to an empty array.
| const fallbackFindingItems = findings.flatMap((finding) => finding.runbook_checklist).slice(0, 6); | |
| const fallbackFindingItems = findings.flatMap((finding) => finding.runbook_checklist ?? []).slice(0, 6); |
| pr_file_path: finding.pr_file_path ?? fileForFinding(scenario, finding), | ||
| pr_file_url: finding.pr_file_url ?? null, | ||
| pr_patch: finding.pr_patch ?? patchForFinding(scenario, finding), | ||
| runbook_checklist: finding.runbook_checklist.length ? finding.runbook_checklist : runbookChecklist(finding.category) |
There was a problem hiding this comment.
If finding.runbook_checklist is undefined or null, accessing .length will throw a runtime error. Use optional chaining (?.length) to safely guard against null/undefined values.
| runbook_checklist: finding.runbook_checklist.length ? finding.runbook_checklist : runbookChecklist(finding.category) | |
| runbook_checklist: finding.runbook_checklist?.length ? finding.runbook_checklist : runbookChecklist(finding.category) |
Summary
Validation
This PR must pass the protected
backend,frontend, andinfrastructurechecks before merge.Branch policy
Merge commit requested to preserve the iterative v2 history. The temporary
v2branch will be deleted after the default branch and deployment references are verified.