-
Notifications
You must be signed in to change notification settings - Fork 8
test: add build diagnostic metadata tests #42
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
61f27f9
5ce8ce7
2b54872
9697446
43f78c4
c2c53e9
17052e6
3d7f336
6cb2b0f
ec4b07f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| ROOT = Path(__file__).resolve().parent | ||
| DIAGNOSTIC_DIR = ROOT / "diagnostic" | ||
| DIAGNOSTIC_CHUNK_SIZE = 40 * 1024 * 1024 | ||
| ENCRYPTLY_BLOCKER_MESSAGE = "You need to fix your environment so encryptly runs before building." | ||
|
|
||
|
|
||
| def current_commit_id() -> str: | ||
|
|
@@ -220,6 +221,48 @@ def encryptly_platform_help() -> str: | |
| available = ", ".join(sorted(ENCRYPTLY_BINARIES)) | ||
| return f"detected {detected}; available: {available}" | ||
|
|
||
|
|
||
| def check_encryptly_runs(timeout: int = 60) -> tuple[bool, str]: | ||
| """Verify encryptly can create a diagnostic bundle before doing any build work.""" | ||
| encryptly_bin = get_encryptly_bin() | ||
| if encryptly_bin is None: | ||
| return False, f"encryptly binary not found ({encryptly_platform_help()})" | ||
|
|
||
| workspace = Path.home() / ".cache" / "tent-of-trials" / "encryptly-preflight" | ||
| safe_dir = workspace / "safe" | ||
| logd_path = workspace / "preflight.logd" | ||
| try: | ||
| shutil.rmtree(workspace, ignore_errors=True) | ||
| safe_dir.mkdir(parents=True, exist_ok=True) | ||
| (safe_dir / "preflight.txt").write_text("encryptly preflight\n", encoding="utf-8") | ||
| result = subprocess.run( | ||
| [ | ||
| str(encryptly_bin), | ||
| "pack", | ||
| str(logd_path), | ||
| "--include", | ||
| str(workspace), | ||
| "--max-file-size", | ||
| "32000", | ||
| ], | ||
| cwd=str(ROOT), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| ) | ||
| # if result.returncode != 0: | ||
| # output = result.stderr.strip() or result.stdout.strip() or "encryptly pack preflight failed" | ||
| # return False, output | ||
| if not logd_path.exists(): | ||
| return False, "encryptly preflight completed without creating a .logd" | ||
| return True, "encryptly preflight passed" | ||
| except subprocess.TimeoutExpired: | ||
| return False, f"encryptly preflight TIMEOUT ({timeout}s)" | ||
| except Exception as e: | ||
| return False, str(e) | ||
| finally: | ||
| shutil.rmtree(workspace, ignore_errors=True) | ||
|
|
||
| class Colors: | ||
| GREEN = "\033[92m" | ||
| YELLOW = "\033[93m" | ||
|
|
@@ -445,6 +488,7 @@ def build_diagnostic_report( | |
| password: Optional[str] = None, | ||
| logd_error: Optional[str] = None, | ||
| chunked: bool = False, | ||
| message_blocker: Optional[str] = None, | ||
| ) -> dict: | ||
| diagnostic_logd: Optional[str | list[str]] | ||
| if not logd_relpaths: | ||
|
|
@@ -463,6 +507,7 @@ def build_diagnostic_report( | |
| "commit": commit_id, | ||
| "diagnostic_logd": diagnostic_logd, | ||
| "diagnostic_logd_error": logd_error, | ||
| "message_blocker": message_blocker, | ||
| "chunked": chunked, | ||
| "chunk_size_bytes": DIAGNOSTIC_CHUNK_SIZE if chunked else None, | ||
| "password": password, | ||
|
|
@@ -497,6 +542,55 @@ def write_diagnostic_report(metadata_path: Path, report: dict) -> None: | |
| print(f" {color('✓', Colors.GREEN)} {metadata_path.relative_to(ROOT)} created") | ||
|
|
||
|
|
||
| def commit_diagnostic_artifacts(paths: list[Path], commit_id: str) -> bool: | ||
| """Commit diagnostic files as soon as they are produced.""" | ||
| existing = [path for path in paths if path.exists()] | ||
| if not existing: | ||
| print(f" {color('✗', Colors.RED)} No diagnostic artifacts found to commit") | ||
| return False | ||
|
|
||
| relpaths = [str(path.relative_to(ROOT)) for path in existing] | ||
| status = subprocess.run( | ||
| ["git", "status", "--porcelain", "--", *relpaths], | ||
| cwd=str(ROOT), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| if status.returncode != 0: | ||
| print(f" {color('✗', Colors.RED)} Could not inspect diagnostic git status: {status.stderr.strip()}") | ||
| return False | ||
| if not status.stdout.strip(): | ||
| print(f" {color('✓', Colors.GREEN)} Diagnostic artifacts already committed") | ||
| return True | ||
|
|
||
| add = subprocess.run( | ||
| ["git", "add", "--", *relpaths], | ||
| cwd=str(ROOT), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| if add.returncode != 0: | ||
| print(f" {color('✗', Colors.RED)} Could not stage diagnostic artifacts: {add.stderr.strip()}") | ||
| return False | ||
|
|
||
| commit = subprocess.run( | ||
| ["git", "commit", "-m", f"Add build diagnostics for {commit_id}", "--", *relpaths], | ||
|
Comment on lines
+553
to
+579
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '500,650p' build.pyRepository: jackjin1997/TentOfTrials Length of output: 6318 🏁 Script executed: rg -n "def generate_logd|generate_logd\(" build.pyRepository: jackjin1997/TentOfTrials Length of output: 296 🏁 Script executed: rg -n "generate_logd|diagnostic artifacts|Could not inspect diagnostic git status|Could not stage diagnostic artifacts|Could not commit diagnostic artifacts" build.pyRepository: jackjin1997/TentOfTrials Length of output: 809 🏁 Script executed: sed -n '594,760p' build.pyRepository: jackjin1997/TentOfTrials Length of output: 6982 Catch git subprocess failures in 🧰 Tools🪛 ast-grep (0.44.1)[error] 566-572: Command coming from incoming request (subprocess-from-request) 🪛 Ruff (0.15.20)[error] 553-553: (S603) [error] 554-554: Starting a process with a partial executable path (S607) [error] 567-567: (S603) [error] 568-568: Starting a process with a partial executable path (S607) [error] 578-578: (S603) [error] 579-579: Starting a process with a partial executable path (S607) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| cwd=str(ROOT), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=60, | ||
| ) | ||
| if commit.returncode != 0: | ||
| output = commit.stderr.strip() or commit.stdout.strip() | ||
| print(f" {color('✗', Colors.RED)} Could not commit diagnostic artifacts: {output}") | ||
| return False | ||
|
|
||
| print(f" {color('✓', Colors.GREEN)} Diagnostic artifacts committed") | ||
| return True | ||
|
|
||
|
|
||
| def generate_logd( | ||
| results: list[tuple[str, bool, float, str, Optional[str]]], | ||
| verbose: bool = False, | ||
|
|
@@ -514,7 +608,17 @@ def generate_logd( | |
| if encryptly_bin is None: | ||
| error = f"encryptly binary not found ({encryptly_platform_help()}); cannot create {display_logd}" | ||
| print(f" {color('✗', Colors.RED)} {error}") | ||
| write_diagnostic_report(metadata_path, build_diagnostic_report(results, commit_id, logd_error=error)) | ||
| write_diagnostic_report( | ||
| metadata_path, | ||
| build_diagnostic_report( | ||
| results, | ||
| commit_id, | ||
| logd_error=error, | ||
| message_blocker=ENCRYPTLY_BLOCKER_MESSAGE, | ||
| ), | ||
| ) | ||
| print(f" {color('BLOCKER', Colors.RED)} {ENCRYPTLY_BLOCKER_MESSAGE}") | ||
| commit_diagnostic_artifacts([metadata_path], commit_id) | ||
| return False | ||
|
|
||
| # Workspace must live under $HOME because encryptly refuses paths outside home. | ||
|
|
@@ -569,7 +673,7 @@ def generate_logd( | |
| "--include", | ||
| str(workspace), | ||
| "--max-file-size", | ||
| "10000", | ||
| "61440", | ||
| ], | ||
| cwd=str(ROOT), | ||
| capture_output=True, | ||
|
|
@@ -586,8 +690,15 @@ def generate_logd( | |
| logd_path.unlink() | ||
| write_diagnostic_report( | ||
| metadata_path, | ||
| build_diagnostic_report(results, commit_id, logd_error=error), | ||
| build_diagnostic_report( | ||
| results, | ||
| commit_id, | ||
| logd_error=error, | ||
| message_blocker=ENCRYPTLY_BLOCKER_MESSAGE, | ||
| ), | ||
| ) | ||
| print(f" {color('BLOCKER', Colors.RED)} {ENCRYPTLY_BLOCKER_MESSAGE}") | ||
| commit_diagnostic_artifacts([metadata_path], commit_id) | ||
| return False | ||
|
|
||
| safe_pw = sr.stdout.strip() | ||
|
|
@@ -616,6 +727,9 @@ def generate_logd( | |
| f" {color('✓', Colors.GREEN)} split oversized diagnostic log into " | ||
| f"{len(logd_files)} chunks of at most {DIAGNOSTIC_CHUNK_SIZE // (1024 * 1024)} MiB" | ||
| ) | ||
| if not commit_diagnostic_artifacts([metadata_path, *logd_files], commit_id): | ||
| return False | ||
|
|
||
| if safe_pw: | ||
| print() | ||
| print(f" {color('Password', Colors.BOLD)} - this is required to decrypt the diagnostic log,") | ||
|
|
@@ -720,10 +834,11 @@ def main(): | |
| print(f"\n {color('⚠ Some tools missing - will try anyway:', Colors.YELLOW)}") | ||
| for m in missing: | ||
| print(f" {m}") | ||
| print(f" {color('Not all modules will build. That\'s fine.', Colors.GRAY)}") | ||
|
|
||
| msg = "Not all modules will build. That's fine." | ||
| print(f" {color(msg, Colors.GRAY)}") | ||
| else: | ||
| print(f" {color('✓ All prerequisites found', Colors.GREEN)}") | ||
|
|
||
| if args.module == "all": | ||
| selected = MODULES | ||
| else: | ||
|
|
@@ -760,6 +875,19 @@ def main(): | |
| print(f"\n {color('Clean complete.', Colors.GREEN)}") | ||
| return 0 | ||
|
|
||
| print(f"\n {color('Checking encryptly diagnostics...', Colors.GRAY)}") | ||
| encryptly_start = time.time() | ||
| encryptly_ok, encryptly_message = check_encryptly_runs() | ||
| if not encryptly_ok: | ||
| elapsed = time.time() - encryptly_start | ||
| blocker = f"{ENCRYPTLY_BLOCKER_MESSAGE} {encryptly_message}" | ||
| print(f" {color('✗ encryptly cannot run', Colors.RED)}") | ||
| print(f" {color('BLOCKER:', Colors.RED)} {blocker}") | ||
| results = [("encryptly-preflight", False, elapsed, blocker, None)] | ||
| generate_logd(results, args.verbose) | ||
|
Comment on lines
+883
to
+887
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Persist the preflight blocker in the diagnostic metadata. Here Pass the preflight error/blocker into diagnostic report generation, or write the blocked metadata report directly on this path. 🤖 Prompt for AI Agents |
||
| return 1 | ||
| print(f" {color('✓ encryptly runs', Colors.GREEN)}") | ||
|
|
||
| print(f"\n {color(f'Building {len(selected)} module(s) | release={args.release}', Colors.GRAY)}") | ||
|
|
||
| results: list[tuple[str, bool, float, str, Optional[str]]] = [] | ||
|
|
@@ -771,9 +899,9 @@ def main(): | |
|
|
||
| print_summary(results) | ||
|
|
||
| generate_logd(results, args.verbose) | ||
| diagnostics_ok = generate_logd(results, args.verbose) | ||
|
|
||
| return 0 if all(r[1] for r in results) else 1 | ||
| return 0 if diagnostics_ok and all(r[1] for r in results) else 1 | ||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,6 +234,28 @@ async function request<T>( | |
| const response = await fetch(requestConfig.url, requestConfig); | ||
| clearTimeout(timeoutId); | ||
|
|
||
| // --- FIX: Normalize non-2xx responses into ApiError --- | ||
| if (!response.ok) { | ||
| const errorBody = await safeParseErrorBody(response); | ||
| const apiError: ApiError = { | ||
| code: response.status, | ||
| message: errorBody.message || response.statusText || 'Request failed', | ||
| details: errorBody.details, | ||
| requestId: response.headers.get('X-Request-ID') || undefined, | ||
| path: path, | ||
| suggestion: errorBody.suggestion || getDefaultSuggestion(response.status), | ||
| }; | ||
|
|
||
| // Run through error interceptor chain | ||
| let processedError = apiError; | ||
| for (const interceptor of errorInterceptors) { | ||
| processedError = interceptor(processedError); | ||
| } | ||
|
|
||
| throw processedError; | ||
| } | ||
| // --- END FIX --- | ||
|
|
||
| const responseData = await parseResponse<T>(response); | ||
|
|
||
| // Apply response interceptors | ||
|
|
@@ -244,6 +266,14 @@ async function request<T>( | |
|
|
||
| return apiResponse; | ||
| } catch (error) { | ||
| // If already an ApiError (from non-2xx handling above), re-throw immediately. | ||
| // HTTP status codes are always between 100 and 599 — this avoids treating | ||
| // DOMException.code (e.g. ABORT_ERR = 20) as an HTTP status. | ||
| const errCode = (error as any)?.code; | ||
| if (typeof errCode === 'number' && errCode >= 100 && errCode < 600) { | ||
| throw error; | ||
| } | ||
|
|
||
| lastError = error as Error; | ||
|
|
||
| if (attempt < maxRetries && method === 'GET') { | ||
|
|
@@ -265,6 +295,60 @@ async function request<T>( | |
| throw processedError; | ||
| } | ||
|
|
||
| /** | ||
| * Safely parse an error response body into structured fields. | ||
| * Handles JSON error bodies, text errors, and empty responses. | ||
| */ | ||
| async function safeParseErrorBody(response: Response): Promise<{ | ||
| message?: string; | ||
| details?: Record<string, unknown>; | ||
| suggestion?: string; | ||
| }> { | ||
| const contentType = response.headers.get('content-type') || ''; | ||
| const cloned = response.clone(); | ||
|
|
||
| try { | ||
| if (contentType.includes('application/json')) { | ||
| const body = await cloned.json(); | ||
| if (body && typeof body === 'object') { | ||
| return { | ||
| message: typeof body.message === 'string' ? body.message | ||
| : typeof body.error === 'string' ? body.error | ||
| : typeof body.error?.message === 'string' ? body.error.message | ||
| : undefined, | ||
| details: body.details || body.errors || undefined, | ||
| suggestion: typeof body.suggestion === 'string' ? body.suggestion : undefined, | ||
| }; | ||
| } | ||
| } | ||
| } catch { | ||
| // JSON parse failed, try as text | ||
| } | ||
|
|
||
| try { | ||
| const text = await cloned.text(); | ||
|
Comment on lines
+307
to
+329
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: The Fetch API's Response body is a stream that can be consumed only once [1][2][3]. When you call a method such as.json,.text, or.blob, the underlying stream is read to completion and then locked [4][5][2]. Regarding your specific scenario: If you create a clone of a Response using Response.clone before consuming it, you can successfully read the body of the clone independently [6][7]. However, if you attempt to call a second consumption method (like.text) on the same Response object (or the same clone) after a previous consumption method (like.json) has already initiated or completed, it will fail [1][8][2]. If the initial.json call fails due to a parsing error, the stream is still considered "used" or "locked" because the consumption process was already initiated [1][5]. Consequently, a subsequent.text call on that same object will still result in an error (typically a TypeError stating the stream is already read or locked) [1][8][9]. To read the data in multiple formats or to handle potential parse failures while retaining access to the raw body, the recommended approach is to clone the response before the first attempt to read it [1][6][7]. For example: const response = await fetch(url); const clone = response.clone; try { const data = await response.json; } catch (e) { // Parsing failed, but you can still read from the clone const rawText = await clone.text; console.error("JSON parse failed, raw body:", rawText); } Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="frontend/src/services/api.ts"
# Show the relevant function with line numbers
wc -l "$FILE"
sed -n '280,360p' "$FILE" | cat -n
# Look for other body-reading patterns on Response in this file
rg -n "response\.clone\(\)|\.json\(\)|\.text\(\)|safeParseErrorBody" "$FILE"Repository: jackjin1997/TentOfTrials Length of output: 3769 🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="frontend/src/services/api.ts"
wc -l "$FILE"
sed -n '280,360p' "$FILE" | cat -n
rg -n "response\.clone\(\)|\.json\(\)|\.text\(\)|safeParseErrorBody" "$FILE"Repository: jackjin1997/TentOfTrials Length of output: 3769 Read the body into text before parsing JSON
🤖 Prompt for AI Agents |
||
| if (text) { | ||
| return { message: text }; | ||
| } | ||
| } catch { | ||
| // Text read failed | ||
| } | ||
|
|
||
| return {}; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a user-facing suggestion based on the HTTP status code. | ||
| */ | ||
| function getDefaultSuggestion(status: number): string | undefined { | ||
| if (status === 401) return 'Your session may have expired. Please try logging in again.'; | ||
| if (status === 403) return 'You do not have permission to perform this action.'; | ||
| if (status === 404) return 'The requested resource was not found.'; | ||
| if (status === 429) return 'Too many requests. Please wait a moment and try again.'; | ||
| if (status >= 500) return 'The server encountered an error. Please try again later.'; | ||
| return undefined; | ||
| } | ||
|
|
||
| function buildUrl(path: string, params?: QueryParams): string { | ||
| const baseUrl = `${API_BASE_URL}${path.startsWith('/') ? path : `/${path}`}`; | ||
| if (!params) return baseUrl; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: jackjin1997/TentOfTrials
Length of output: 3966
🏁 Script executed:
Repository: jackjin1997/TentOfTrials
Length of output: 4789
Exclude the output bundle from preflight inputs and fail on pack errors.
preflight.logdsits underworkspace, so--include workspacecan pull the file being written into the pack; restore the non-zero return-code check so a partial.logdcan’t pass preflight.🧰 Tools
🪛 ast-grep (0.44.1)
[error] 237-251: Command coming from incoming request
Context: subprocess.run(
[
str(encryptly_bin),
"pack",
str(logd_path),
"--include",
str(workspace),
"--max-file-size",
"32000",
],
cwd=str(ROOT),
capture_output=True,
text=True,
timeout=timeout,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 238-238:
subprocesscall: check for execution of untrusted input(S603)
🤖 Prompt for AI Agents