test: add build diagnostic metadata tests#42
Conversation
📝 WalkthroughWalkthroughbuild.py adds an encryptly preflight check, a message_blocker diagnostic field, and git-based commit logic for diagnostic artifacts, gating the build in main(). New tests validate diagnostic report shape. Separately, frontend/src/services/api.ts restructures HTTP error handling into structured ApiError objects with interceptor processing and new parsing helpers. ChangesBuild diagnostics and encryptly preflight
Estimated code review effort: 3 (Moderate) | ~30 minutes Frontend API Error Handling
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant apiRequest as request()
participant Fetch as fetch API
participant ErrorParser as safeParseErrorBody()
participant Interceptors as errorInterceptors
Client->>apiRequest: call request()
apiRequest->>Fetch: fetch(url, options)
Fetch-->>apiRequest: response
alt response not ok
apiRequest->>ErrorParser: parse error body
ErrorParser-->>apiRequest: message/details/suggestion
apiRequest->>apiRequest: build ApiError (code, message, requestId)
apiRequest->>Interceptors: apply errorInterceptors(error)
Interceptors-->>apiRequest: processed ApiError
apiRequest-->>Client: throw ApiError
else response ok
apiRequest-->>Client: return parsed data
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_diagnostic_metadata.py (1)
320-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider avoiding filesystem side effects in
test_diagnostic_path_pattern.
diagnostic_paths_for_commit()callsDIAGNOSTIC_DIR.mkdir(parents=True, exist_ok=True), so this test creates adiagnostic/directory in the project root. Using pytest'stmp_pathfixture withmonkeypatchto redirectDIAGNOSTIC_DIRwould keep the test self-contained and avoid leaving artifacts behind.♻️ Optional refactor using tmp_path and monkeypatch
-def test_diagnostic_path_pattern(): +def test_diagnostic_path_pattern(tmp_path, monkeypatch): """diagnostic_paths_for_commit() returns paths matching build-PATTERN.logd/json.""" - from build import diagnostic_paths_for_commit + from build import diagnostic_paths_for_commit, diagnostic_paths_for_commit as _dpfc + import build + + monkeypatch.setattr(build, "DIAGNOSTIC_DIR", tmp_path / "diagnostic") logd, meta, commit_id = diagnostic_paths_for_commit() assert logd.name == f"build-{commit_id}.logd" assert meta.name == f"build-{commit_id}.json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_diagnostic_metadata.py` around lines 320 - 326, `test_diagnostic_path_pattern` is creating a real `diagnostic/` directory because `diagnostic_paths_for_commit()` uses `DIAGNOSTIC_DIR.mkdir(...)`. Update the test to stay isolated by using pytest’s `tmp_path` and `monkeypatch` to redirect `build.DIAGNOSTIC_DIR` to a temporary location before calling `diagnostic_paths_for_commit()`, so the assertions on `logd`, `meta`, and `commit_id` remain the same without leaving filesystem artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.py`:
- Around line 883-887: Persist the encryptly preflight blocker into the
diagnostic metadata path used by build.py. In the `check_encryptly_runs()`
failure branch, the `blocker` is currently only printed and stored in `results`,
so `generate_logd()` leaves `diagnostic_logd_error` and `message_blocker` empty;
update the `generate_logd` call or its inputs so the blocker is carried through
and written into the JSON report, or emit the blocked metadata report directly
from this branch. Use the existing `blocker`, `results`, and `generate_logd`
flow to keep the top-level blocker contract intact.
- Around line 231-256: The preflight pack step in the preflight check is
including the output bundle path itself via workspace, and the non-zero
return-code validation is disabled. Update the preflight logic around
subprocess.run and logd_path so the generated preflight.logd is excluded from
the pack inputs, then restore the result.returncode failure check in this block
to fail fast on any pack error before checking logd_path.exists().
- Around line 553-579: The git subprocess calls in commit_diagnostic_artifacts
can still raise exceptions before a returncode is available, so generate_logd
may fail instead of handling the error cleanly. Update the subprocess.run usage
for the status, add, and commit steps in commit_diagnostic_artifacts to catch
OSError and SubprocessError/TimeoutExpired, log a concise failure message, and
return False. Keep the behavior consistent with the existing return-false paths
and use the commit_diagnostic_artifacts function name to locate the changes.
In `@frontend/src/services/api.ts`:
- Around line 307-329: The fallback in the error parsing logic is broken because
calling cloned.json() consumes the body before cloned.text() can run. Update the
response handling in api.ts so the raw text is read first (or use a separate
clone for JSON parsing), then parse JSON from that string inside the existing
response parsing flow, preserving the message/details/suggestion extraction in
the same error-shaping logic.
---
Nitpick comments:
In `@tests/test_diagnostic_metadata.py`:
- Around line 320-326: `test_diagnostic_path_pattern` is creating a real
`diagnostic/` directory because `diagnostic_paths_for_commit()` uses
`DIAGNOSTIC_DIR.mkdir(...)`. Update the test to stay isolated by using pytest’s
`tmp_path` and `monkeypatch` to redirect `build.DIAGNOSTIC_DIR` to a temporary
location before calling `diagnostic_paths_for_commit()`, so the assertions on
`logd`, `meta`, and `commit_id` remain the same without leaving filesystem
artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97e0d9ce-623a-4df2-b21f-e417678f4a3f
⛔ Files ignored due to path filters (2)
tools/encryptly/windows-arm64/encryptly.exeis excluded by!**/*.exetools/encryptly/windows-x64/encryptly.exeis excluded by!**/*.exe
📒 Files selected for processing (8)
build.pyfrontend/src/services/api.tstests/__init__.pytests/test_diagnostic_metadata.pytools/encryptly/linux-arm64/encryptlytools/encryptly/linux-x64/encryptlytools/encryptly/macos-arm64/encryptlytools/encryptly/macos-x64/encryptly
| 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(): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of build.py with line numbers.
sed -n '190,290p' build.py
# Find any other handling of result.returncode or preflight .logd paths.
rg -n "returncode|preflight\.logd|--include|encryptly pack preflight failed" build.pyRepository: jackjin1997/TentOfTrials
Length of output: 3966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the other pack call for comparison.
sed -n '650,700p' build.py
# Search for any documentation/comments explaining the preflight pack behavior.
rg -n "preflight|encryptly pack|include .*workspace|safe_dir|logd_path" .Repository: jackjin1997/TentOfTrials
Length of output: 4789
Exclude the output bundle from preflight inputs and fail on pack errors.
preflight.logd sits under workspace, so --include workspace can pull the file being written into the pack; restore the non-zero return-code check so a partial .logd can’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: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.py` around lines 231 - 256, The preflight pack step in the preflight
check is including the output bundle path itself via workspace, and the non-zero
return-code validation is disabled. Update the preflight logic around
subprocess.run and logd_path so the generated preflight.logd is excluded from
the pack inputs, then restore the result.returncode failure check in this block
to fail fast on any pack error before checking logd_path.exists().
| 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], |
There was a problem hiding this comment.
🩺 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 commit_diagnostic_artifacts. subprocess.run(..., timeout=...) can raise TimeoutExpired or OSError before returncode exists, so a missing or hung git can escape generate_logd instead of returning False. Wrap these calls and return False on OSError/SubprocessError.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 566-572: Command coming from incoming request
Context: subprocess.run(
["git", "add", "--", *relpaths],
cwd=str(ROOT),
capture_output=True,
text=True,
timeout=30,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 553-553: subprocess call: check for execution of untrusted input
(S603)
[error] 554-554: Starting a process with a partial executable path
(S607)
[error] 567-567: subprocess call: check for execution of untrusted input
(S603)
[error] 568-568: Starting a process with a partial executable path
(S607)
[error] 578-578: subprocess call: check for execution of untrusted input
(S603)
[error] 579-579: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.py` around lines 553 - 579, The git subprocess calls in
commit_diagnostic_artifacts can still raise exceptions before a returncode is
available, so generate_logd may fail instead of handling the error cleanly.
Update the subprocess.run usage for the status, add, and commit steps in
commit_diagnostic_artifacts to catch OSError and SubprocessError/TimeoutExpired,
log a concise failure message, and return False. Keep the behavior consistent
with the existing return-false paths and use the commit_diagnostic_artifacts
function name to locate the changes.
Source: Linters/SAST tools
| 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) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the preflight blocker in the diagnostic metadata.
Here blocker is only placed in the module output. If check_encryptly_runs() fails but the later generate_logd() call succeeds, the JSON report has diagnostic_logd_error=None and message_blocker=None, losing the top-level blocker contract.
Pass the preflight error/blocker into diagnostic report generation, or write the blocked metadata report directly on this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.py` around lines 883 - 887, Persist the encryptly preflight blocker
into the diagnostic metadata path used by build.py. In the
`check_encryptly_runs()` failure branch, the `blocker` is currently only printed
and stored in `results`, so `generate_logd()` leaves `diagnostic_logd_error` and
`message_blocker` empty; update the `generate_logd` call or its inputs so the
blocker is carried through and written into the JSON report, or emit the blocked
metadata report directly from this branch. Use the existing `blocker`,
`results`, and `generate_logd` flow to keep the top-level blocker contract
intact.
| 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(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Fetch API Response.json() consume the response body so a later Response.text() call on the same clone fails after JSON parse failure?
💡 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:
- 1: https://stackoverflow.com/questions/53511974/javascript-fetch-failed-to-execute-json-on-response-body-stream-is-locked
- 2: https://stackoverflow.com/questions/73548742/uncaught-in-promise-syntaxerror-unexpected-end-of-json-input-with-cors-http-p
- 3: https://stackoverflow.com/questions/46742251/why-fetch-body-object-can-be-read-only-once
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Response/json
- 5: https://stackoverflow.com/questions/66287364/fetch-inspect-return-value-along-side-with-json-parsing
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Response/clone
- 7: https://jakearchibald.com/2014/reading-responses/
- 8: https://thenewtoys.dev/blog/2020/07/23/why-make-fetch-response-a-stream/
- 9: https://stackoverflow.com/questions/78593968/failed-to-execute-json-on-response-body-stream-already-read
🏁 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
cloned.json() consumes the body, so the later cloned.text() fallback can’t recover the raw error message after a JSON parse failure. Read text first (or use a second clone) and parse JSON from that string instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/services/api.ts` around lines 307 - 329, The fallback in the
error parsing logic is broken because calling cloned.json() consumes the body
before cloned.text() can run. Update the response handling in api.ts so the raw
text is read first (or use a separate clone for JSON parsing), then parse JSON
from that string inside the existing response parsing flow, preserving the
message/details/suggestion extraction in the same error-shaping logic.
1 participant
Summary
Add 18 deterministic tests for build diagnostic report generation metadata.
Changes
tests/test_diagnostic_metadata.pywith 18 tests covering:diagnostic_logd=None,diagnostic_logd_errorset, no password/decrypt_command when archive fails,message_blockerpopulatedchunk_size_bytes, forward-slash pathscurrent_commit_id()format, path naming patternsValidation
python3 -m pytest tests/test_diagnostic_metadata.py -v # 18/18 passCloses #3
Summary by CodeRabbit
New Features
Bug Fixes