fix(server): harden practice review execution pipeline - #979
Merged
FelixTJDietrich merged 4 commits intoApr 10, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request implements architecture-aware Bun installation across Docker images, refactors the precompute grep utility to spawn processes directly instead of using bash, introduces a new Pi runner script with multi-attempt retry logic and session tracking, transitions repository handling from on-demand cloning to bind-mounted local checkouts, and dynamically configures the LLM proxy port based on the application server port. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Pi Runner Script
participant FS as File System
participant CLI as Pi CLI Process
participant Sess as Session Manager
rect rgba(100, 150, 255, 0.5)
Note over Runner,Sess: Attempt 1 (Initial)
Runner->>FS: Read /workspace/.prompt
Runner->>CLI: spawnSync("pi", [...], timeout=__INITIAL_TIMEOUT_MS__)
CLI->>Sess: Write session JSONL to /tmp/pi-sessions/initial
CLI-->>Runner: Return stdout/stderr
Runner->>FS: Check /workspace/.output/result.json
alt result.json valid
Runner->>Runner: Extract findings
Runner->>Sess: Scan session, aggregate usage
Runner->>FS: Write usage.json
Runner-->>Runner: Exit 0 (success)
else stdout contains findings
Runner->>FS: Write stdout findings to result.json
Runner->>Sess: Scan session, aggregate usage
Runner->>FS: Write usage.json
Runner-->>Runner: Exit 0 (success)
else no valid result
Runner->>FS: Record attempt in runner-debug.json
end
end
rect rgba(150, 100, 255, 0.5)
Note over Runner,Sess: Attempt 2 (Continuation or Fresh Retry)
alt session JSONL found
Runner->>CLI: spawnSync("pi", [continuation prompt], timeout=__RETRY_TIMEOUT_MS__)
CLI->>Sess: Append to existing /tmp/pi-sessions/initial
else no prior session
Runner->>CLI: spawnSync("pi", [...], timeout=__RETRY_TIMEOUT_MS__)
CLI->>Sess: Write fresh session to /tmp/pi-sessions/retry-1-fresh
end
CLI-->>Runner: Return stdout/stderr
Runner->>FS: Validate result.json or stdout findings
alt success
Runner->>Sess: Aggregate cumulative usage
Runner->>FS: Write usage.json, runner-debug.json
Runner-->>Runner: Exit 0
else continue to attempt 3
Runner->>FS: Record attempt in runner-debug.json
end
end
rect rgba(255, 150, 100, 0.5)
Note over Runner,Sess: Attempt 3 (Final Fresh Retry)
Runner->>CLI: spawnSync("pi", [...], timeout=__RETRY_TIMEOUT_MS__)
CLI->>Sess: Write fresh session to /tmp/pi-sessions/retry-2-fresh
CLI-->>Runner: Return stdout/stderr/exit code
Runner->>FS: Validate result or stdout findings
Runner->>Sess: Aggregate final cumulative usage
Runner->>FS: Write usage.json, runner-debug.json
alt final success
Runner-->>Runner: Exit 0
else final failure
Runner-->>Runner: Exit with last pi exit code or 1
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java (1)
100-102: Consider adding explicit grouping to improve readability of the fallback chain.The mixed
&&/||operators in the fallback branch work correctly but are easy to misread. Adding explicit subshell grouping clarifies intent and reduces risk of precedence misunderstandings in future edits:Suggested improvement
" || (echo '[precompute] failed, continuing without hints'" + - " && cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null || true" + - " && tail -200 /tmp/precompute-runner.log 2>/dev/null || true)) && " + " && (cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null || true)" + + " && (tail -200 /tmp/precompute-runner.log 2>/dev/null || true))) && "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java` around lines 100 - 102, The fallback shell snippet in AgentAdapter (the string concatenation that builds the precompute command) mixes && and || without explicit grouping, making precedence hard to read; wrap the fallback chain in a grouped subshell (e.g., surround the sequence starting with "echo '[precompute] failed, continuing without hints' ..." through the final "tail -200 ..." with parentheses or braces) so the OR/AND logic is explicit and preserve the existing redirects and || true fallbacks; update the string in the method that constructs this command in AgentAdapter.java to use the grouped subshell syntax.server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java (1)
373-374: Rename this test to the repository’sshould…When…form.
shouldParseUsageAndRunnerDebugis clear, but the new method still misses theWhen[Condition]suffix used in this test suite. Something likeshouldParseUsageAndRunnerDebugWhenArtifactsArePresentwould match the local convention. As per coding guidelines, "Test method names should followshould[ExpectedBehavior]When[Condition]naming pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java` around lines 373 - 374, Rename the test method shouldParseUsageAndRunnerDebug to follow the repository convention should[Expected]When[Condition]; change the method name to shouldParseUsageAndRunnerDebugWhenArtifactsArePresent (and update the `@DisplayName` string if you want it to match the new name) so references to shouldParseUsageAndRunnerDebug in PiAgentAdapterTest are replaced and the test name follows the should…When… pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/agents/claude-code/Dockerfile`:
- Around line 20-30: The Dockerfile currently downloads bun using ARG
BUN_VERSION and the TARGETARCH->bun_arch mapping but does not verify integrity;
fetch the release SHASUMS256.txt (for bun-v${BUN_VERSION}), extract the expected
SHA-256 for "bun-linux-${bun_arch}.zip", verify the downloaded /tmp/bun.zip
against that checksum (fail the build on mismatch) before unzipping, and only
proceed to unzip/mv/chmod if verification passes; apply the identical change to
the docker/agents/pi/Dockerfile (the same ARG BUN_VERSION / TARGETARCH ->
bun_arch logic and /tmp/bun.zip path).
In `@docker/agents/opencode/Dockerfile`:
- Around line 19-29: The RUN block that downloads bun using ARG BUN_VERSION and
sets bun_arch via the case statement must verify the archive integrity: fetch
the expected SHA256 for the specific bun release (use BUN_VERSION and bun_arch
to construct the URL) and verify the downloaded /tmp/bun.zip matches that
checksum before unzipping; update the RUN sequence around the curl/unzip steps
(the lines that set bun_arch, curl -fsSL
"https://github.com/.../bun-linux-${bun_arch}.zip" -o /tmp/bun.zip, unzip, mv,
chmod, rm) to download a .sha256 (or embed a hardcoded fingerprint for the
pinned BUN_VERSION), run sha256sum -c or equivalent to compare, and fail the
build if verification fails, keeping removal of /tmp artifacts afterward.
In `@docker/agents/pi/Dockerfile`:
- Around line 19-29: The Dockerfile RUN block installs Bun from GitHub without
integrity verification—modify the RUN sequence around ARG BUN_VERSION and the
bun download/move steps to download the corresponding checksum (e.g.,
bun-linux-${bun_arch}.zip.sha256 or a release assets checksum), verify the
archive with a strong hash utility (sha256sum or sha512sum) and fail the build
on mismatch, only then unzip/move /tmp/bun-linux-${bun_arch}/bun to
/usr/local/bin/bun and clean up; apply the same pattern to the other agent
Dockerfiles (claude-code, opencode) so all bun installs use checksum validation.
In `@docker/agents/precompute/lib/grep.ts`:
- Around line 29-34: The grep and findFiles implementations build a shell
command string (cmd) and execute it with "bash -c", which allows command
injection and hides errors via "2>/dev/null || true"; change them to call
Bun.spawn with an argument array (e.g., ["grep", "-rn", fixedFlag,
"--include="+glob, "-m", String(maxResults), escapedPattern, dir]) instead of a
shell string, remove the shell redirection and "|| true", capture stderr/stdout
from the Bun.spawn result, and explicitly handle non-zero exit codes (throw or
return a clear error) so failures are not silently ignored; update the code
paths that construct cmd and the Bun.spawn invocation in functions named grep
and findFiles accordingly.
- Around line 87-88: The catch block that currently swallows errors and returns
new Map() must log the failure with context before returning; update the catch
to accept the error (e.g., catch (err)) and call the module's logger (or
console.error if no logger exists) to record the error message and stack plus
relevant contextual info such as the file path/pattern or function inputs that
led to the read (reference the catch block in grep.ts where it returns new
Map()); after logging the error, continue to return new Map() so behavior is
preserved.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapter.java`:
- Around line 173-179: The timeout split currently reserves two retry windows
and floors the initial timeout at 60_000ms which can exceed agentTimeoutMs;
change the logic in PiAgentAdapter where retryTimeoutMs and initialTimeoutMs are
computed so you cap the retry window first (e.g. retryTimeoutMs =
Math.min(desiredRetryMs, agentTimeoutMs / 2)) and then derive initialTimeoutMs
from the remaining budget (initialTimeoutMs = Math.max(minInitialMs,
agentTimeoutMs - retryTimeoutMs)), removing the hard floor that reserved two
retries; update the values used in the script replacement
(.replace("__INITIAL_TIMEOUT_MS__", ...) and .replace("__RETRY_TIMEOUT_MS__",
...)) so both attempts are budgeted from the same agentTimeoutMs.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 857-863: The code currently only checks "if (pathNode == null)"
but must also treat Jackson NullNode and empty/"null" strings as missing: change
the guard around path handling (the code referencing pathNode,
pathNode.asText(), isInternalContextPath, diffFiles, and hasInScopeLocation) to
skip when pathNode is null OR pathNode.isNull() OR pathNode.asText() is blank or
equals the literal "null"; only then call asText() and compare against
diffFiles/isInternalContextPath and set hasInScopeLocation. Ensure this prevents
treating JSON nulls as the string "null" so valid findings are not filtered out.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.java`:
- Line 59: In SandboxProperties.java validate the constructor/config parameter
llmProxyPort (the `@Nullable` Integer llmProxyPort) and in the
resolvedLlmProxyPort resolution logic ensure you fail fast: if llmProxyPort is
provided but <= 0 throw an IllegalArgumentException (or similar), and when
falling back to serverPort validate serverPort > 0 before using it; update the
resolvedLlmProxyPort method to guard the fallback and throw a clear exception if
neither llmProxyPort nor serverPort are valid so LLM_PROXY_URL cannot become
invalid at runtime.
In `@server/application-server/src/main/resources/agent/pi-runner.mjs`:
- Around line 243-252: The recordAttempt function is currently storing raw
stdout/stderr previews into runnerDebug.attempts (stdoutPreview/stderrPreview)
which may leak secrets; change it to avoid persisting verbatim snippets by
replacing stdoutPreview/stderrPreview with either a deterministic hash (e.g.,
sha256) of the clipped output or a redaction marker plus a preview length field,
while keeping stdoutBytes/stderrBytes and exit metadata; update references to
clipPreview(result.stdout || "") and clipPreview(result.stderr || "") to instead
produce a hashedPreview or redactedPreview field (e.g., previewHash and
previewRedacted=true) so runner-debug.json contains only non-sensitive derived
data instead of raw content.
- Around line 75-79: The current stdout validation falls back in the catch block
to a naive substring check of out for '"findings"' and '"practiceSlug"', which
lets non-JSON prose pass as a valid payload; change the catch to reject
non-parseable output instead. Specifically, in the try/catch that inspects
JSON.parse(out) (the block returning Array.isArray(JSON.parse(out)?.findings)),
remove or replace the catch branch so it does not return out.includes(...);
instead return false (or throw) when JSON.parse fails, ensuring only parseable
JSON with an array findings is accepted before the code that writes result.json
and exits.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java`:
- Around line 100-102: The fallback shell snippet in AgentAdapter (the string
concatenation that builds the precompute command) mixes && and || without
explicit grouping, making precedence hard to read; wrap the fallback chain in a
grouped subshell (e.g., surround the sequence starting with "echo '[precompute]
failed, continuing without hints' ..." through the final "tail -200 ..." with
parentheses or braces) so the OR/AND logic is explicit and preserve the existing
redirects and || true fallbacks; update the string in the method that constructs
this command in AgentAdapter.java to use the grouped subshell syntax.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java`:
- Around line 373-374: Rename the test method shouldParseUsageAndRunnerDebug to
follow the repository convention should[Expected]When[Condition]; change the
method name to shouldParseUsageAndRunnerDebugWhenArtifactsArePresent (and update
the `@DisplayName` string if you want it to match the new name) so references to
shouldParseUsageAndRunnerDebug in PiAgentAdapterTest are replaced and the test
name follows the should…When… pattern.
🪄 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: 05919e5e-9575-447a-85c9-a3f80540de85
📒 Files selected for processing (18)
docker/agents/claude-code/Dockerfiledocker/agents/opencode/Dockerfiledocker/agents/pi/Dockerfiledocker/agents/precompute/lib/grep.tsdocker/agents/precompute/lib/types.tsserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionResultParser.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxConfiguration.javaserver/application-server/src/main/resources/agent/PI-AGENTS.mdserver/application-server/src/main/resources/agent/pi-runner.mjsserver/application-server/src/main/resources/application.ymlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.java
Comment on lines
+114
to
+117
| // Step 3: Extract findings array. Some Pi runs on authentic Swift repos currently | ||
| // return a top-level `errors` array with negative practice entries instead of the | ||
| // expected `findings` array; normalize that shape so delivery can still proceed. | ||
| JsonNode findingsNode = extractFindingsNode(root); |
Contributor
There was a problem hiding this comment.
errors compatibility is incomplete on mixed-text fallback path
parse() now supports errors via extractFindingsNode, but extractJsonFromText() still only returns JSON objects that contain findings (Line 610). For phase-prefixed outputs that contain only top-level errors, parsing still exits early and never reaches normalization.
Suggested fix
@@
- if (node != null && node.isObject() && node.has("findings")) {
+ if (
+ node != null &&
+ node.isObject() &&
+ (
+ (node.has("findings") && node.get("findings").isArray()) ||
+ (node.has("errors") && node.get("errors").isArray())
+ )
+ ) {
return node;
}Also applies to: 180-207
| @DefaultValue("60") @Min(10) int reconciliationIntervalSeconds, | ||
| @Nullable String containerRuntime, | ||
| @DefaultValue("8080") @Min(1) int llmProxyPort, | ||
| @Nullable Integer llmProxyPort, |
Contributor
There was a problem hiding this comment.
Fail fast on invalid port values instead of silently falling back.
On Line 86, resolvedLlmProxyPort accepts invalid values implicitly. If both llmProxyPort and fallback serverPort are non-positive, downstream LLM_PROXY_URL becomes invalid at runtime. Add validation on Line 59 and guard the fallback in the resolver.
💡 Proposed fix
@@
- `@Nullable` Integer llmProxyPort,
+ `@Nullable` `@Min`(1) Integer llmProxyPort,
@@
public int resolvedLlmProxyPort(int serverPort) {
if (llmProxyPort != null && llmProxyPort > 0) {
return llmProxyPort;
}
+ if (serverPort <= 0) {
+ throw new IllegalStateException(
+ "Invalid server port for LLM proxy fallback: " + serverPort
+ );
+ }
return serverPort;
}Also applies to: 86-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.java`
at line 59, In SandboxProperties.java validate the constructor/config parameter
llmProxyPort (the `@Nullable` Integer llmProxyPort) and in the
resolvedLlmProxyPort resolution logic ensure you fail fast: if llmProxyPort is
provided but <= 0 throw an IllegalArgumentException (or similar), and when
falling back to serverPort validate serverPort > 0 before using it; update the
resolvedLlmProxyPort method to guard the fallback and throw a clear exception if
neither llmProxyPort nor serverPort are valid so LLM_PROXY_URL cannot become
invalid at runtime.
Comment on lines
+243
to
+252
| function recordAttempt(label, result, sessionSummary) { | ||
| runnerDebug.attempts.push({ | ||
| label, | ||
| exitCode: result.status, | ||
| signal: result.signal, | ||
| timedOut: result.error?.code === "ETIMEDOUT", | ||
| stdoutBytes: (result.stdout || "").length, | ||
| stderrBytes: (result.stderr || "").length, | ||
| stdoutPreview: clipPreview(result.stdout || ""), | ||
| stderrPreview: clipPreview(result.stderr || ""), |
Contributor
There was a problem hiding this comment.
Avoid persisting raw stdout/stderr previews in runner-debug.json.
These previews can contain exact secret values or other sensitive student data from the diff, and this file is now surfaced downstream via runnerDebug. Keep lengths and exit metadata, but redact or hash the payload instead of storing snippets verbatim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/application-server/src/main/resources/agent/pi-runner.mjs` around
lines 243 - 252, The recordAttempt function is currently storing raw
stdout/stderr previews into runnerDebug.attempts (stdoutPreview/stderrPreview)
which may leak secrets; change it to avoid persisting verbatim snippets by
replacing stdoutPreview/stderrPreview with either a deterministic hash (e.g.,
sha256) of the clipped output or a redaction marker plus a preview length field,
while keeping stdoutBytes/stderrBytes and exit metadata; update references to
clipPreview(result.stdout || "") and clipPreview(result.stderr || "") to instead
produce a hashedPreview or redactedPreview field (e.g., previewHash and
previewRedacted=true) so runner-debug.json contains only non-sensitive derived
data instead of raw content.
…ness - Harden pi-runner.mjs retry with continuation and fresh-authorized modes - Fix grep.ts to handle binary files, large outputs, and edge cases - Add grep.test.ts for precompute grep library - Improve PracticeDetectionResultParser for defensive JSON extraction - Add PullRequestReviewHandler delivery suppression for merged PRs - Update Dockerfiles for agent containers - Add production practice import and benchmark results - Include run-go98weh precompute reference files
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens the server-side practice review execution pipeline by improving sandbox configuration, tightening PR review scoping behavior, and introducing a more robust Pi agent runner with diagnostics/usage tracking, alongside multi-arch Docker agent image updates.
Changes:
- Add a dedicated Pi runner script with persisted diagnostics (
runner-debug.json) and usage tracking (usage.json), and update the Pi adapter accordingly. - Refine sandbox LLM proxy port resolution to follow the active server port when no explicit override is provided.
- Improve PR review robustness by requiring a pre-prepared local repo checkout and filtering findings by diff scope with a small internal-context allowlist.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxLiveTest.java | Updates sandbox adapter construction to pass server port. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.java | Adjusts tests for nullable llmProxyPort and validates port fallback behavior + HOME env behavior. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java | Adds tests for missing local checkout and diff-scope filtering/--name-only parsing. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java | Updates tests for new Pi runtime layout and verifies usage/debug parsing. |
| server/application-server/src/main/resources/application.yml | Changes sandbox LLM proxy port default to fall back to ${server.port}. |
| server/application-server/src/main/resources/agent/pi-runner.mjs | New Pi runner with retries, validation, diagnostics, and usage aggregation. |
| server/application-server/src/main/resources/agent/PI-AGENTS.md | Clarifies that security analysis is authorized and should not be refused. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxConfiguration.java | Injects server.port into Docker sandbox adapter bean. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapter.java | Uses resolved proxy port (override or server port) and allows HOME through env filtering. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.java | Makes llmProxyPort nullable and adds resolvedLlmProxyPort(serverPort). |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java | Requires pre-prepared checkouts; switches diff file detection to --name-only; adds internal-context allowlist for diff scoping. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionResultParser.java | Introduces a helper for extracting the canonical findings node. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java | Enhances precompute failure logging by preserving/tailing runner logs. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapter.java | Stages Pi config outside workspace, copies into writable HOME, and parses usage/debug outputs. |
| run-go98weh/.precompute/lib/types.ts | Adds precompute type definitions (new). |
| run-go98weh/.precompute/lib/grep.ts | Adds precompute grep utilities (new). |
| run-go98weh/.precompute/lib/diff-parser.ts | Adds diff parsing utilities for precompute scripts (new). |
| production-practice-review/generated/practices-production-import.json | Adds generated production practice definitions + precompute scripts (new). |
| production-practice-review/BENCHMARK-RESULTS.md | Adds benchmark results documentation (new). |
| docker/agents/precompute/lib/types.ts | Expands precompute metadata typing (adds commits) and re-indents. |
| docker/agents/precompute/lib/grep.ts | Reworks grep/find implementation to avoid shell interpolation and support path-aware globs. |
| docker/agents/precompute/lib/grep.test.ts | Adds Bun tests for the new grep/find behavior (new). |
| docker/agents/pi/Dockerfile | Adds multi-arch Bun install with sha256 verification and clarifies Git hardening notes. |
| docker/agents/opencode/Dockerfile | Adds multi-arch Bun install with sha256 verification and clarifies Git hardening notes. |
| docker/agents/claude-code/Dockerfile | Adds multi-arch Bun install with sha256 verification and updates base packages. |
Comment on lines
+175
to
+179
| String scriptTemplate = new String(AgentAdapter.loadClasspathResource("pi-runner.mjs"), StandardCharsets.UTF_8); | ||
| String script = scriptTemplate | ||
| .replace("__MAX_STDOUT_BUFFER_BYTES__", Integer.toString(MAX_STDOUT_BUFFER_BYTES)) | ||
| .replace("__INITIAL_TIMEOUT_MS__", Long.toString(initialTimeoutMs)) | ||
| .replace("__RETRY_TIMEOUT_MS__", Long.toString(retryTimeoutMs)); |
There was a problem hiding this comment.
String#replace only replaces the first occurrence. pi-runner.mjs uses __RETRY_TIMEOUT_MS__ multiple times, so the generated runner can still contain unresolved placeholders and crash at runtime. Use replaceAll (or a global regex replacement) for all placeholders, and consider adding a test assertion that the script contains no __...__ tokens.
Comment on lines
81
to
85
| "LD_PRELOAD", | ||
| "LD_LIBRARY_PATH", | ||
| "PATH", | ||
| "HOME", | ||
| "SHELL", | ||
| "USER", |
There was a problem hiding this comment.
Removing HOME from the blocked env-var list means untrusted job input (if it can influence spec.environment()) could set HOME and affect tool/config resolution inside the container. If HOME must be writable for Pi, a safer pattern is to keep HOME blocked for incoming env, then inject/override HOME (and XDG_CONFIG_HOME) as a trusted internal value after filtering, or allow it only for specific adapters via an internal allowlist.
| @DefaultValue("60") @Min(10) int reconciliationIntervalSeconds, | ||
| @Nullable String containerRuntime, | ||
| @DefaultValue("8080") @Min(1) int llmProxyPort, | ||
| @Nullable Integer llmProxyPort, |
There was a problem hiding this comment.
llmProxyPort is now nullable (good), but it also lost its @Min(1) validation. That allows explicit configuration values like 0 or negative ports to silently pass binding and later produce invalid URLs. Add @Min(1) (or @Positive) to the Integer llmProxyPort field to reject invalid overrides while still allowing null.
Suggested change
| @Nullable Integer llmProxyPort, | |
| @Nullable @Min(1) Integer llmProxyPort, |
Comment on lines
134
to
142
| public SandboxManager dockerSandboxAdapter( | ||
| SandboxNetworkManager networkManager, | ||
| SandboxWorkspaceManager workspaceManager, | ||
| SandboxContainerManager containerManager, | ||
| ContainerSecurityPolicy securityPolicy, | ||
| SandboxProperties properties, | ||
| @Value("${server.port:8080}") int serverPort, | ||
| MeterRegistry meterRegistry | ||
| ) { |
There was a problem hiding this comment.
Using ${server.port} as the injected serverPort breaks when the app runs with an ephemeral port (server.port=0, e.g., RANDOM_PORT tests): the resolved LLM proxy URL will use port 0. If you need the actual bound port, prefer local.server.port (when available) or inject it from the running web server (e.g., via WebServerApplicationContext) rather than the configured server.port value.
Comment on lines
791
to
+797
| String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha); | ||
| if (range == null) return Set.of(); | ||
|
|
||
| String diffStat = runGit(repoPath, "diff", "--stat", range[0] + ".." + range[1]); | ||
| if (diffStat == null || diffStat.isBlank()) return Set.of(); | ||
| String nameOnly = runGit(repoPath, "diff", "--name-only", range[0] + ".." + range[1]); | ||
| if (nameOnly == null || nameOnly.isBlank()) return Set.of(); | ||
|
|
||
| return parseDiffNameOnlyPaths(nameOnly); |
There was a problem hiding this comment.
This logic no longer computes diff stat files; it now computes changed file paths from git diff --name-only. If the surrounding method is still named computeDiffStatFiles (as implied by the context), rename it (and any related helpers/comments) to reflect the new behavior (e.g., computeDiffFiles / computeChangedFiles) to avoid misleading future maintenance.
Comment on lines
+100
to
+102
| " || (echo '[precompute] failed, continuing without hints'" + | ||
| " && cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null || true" + | ||
| " && tail -200 /tmp/precompute-runner.log 2>/dev/null || true)) && " |
There was a problem hiding this comment.
The failure-handling subcommand mixes && and || chains without grouping, which is easy to misread and can be fragile if edited later. Consider restructuring the fallback block using explicit grouping (e.g., { ...; ...; } or parenthesized sub-steps with ;) so each cp/tail is clearly best-effort without relying on operator precedence.
Suggested change
| " || (echo '[precompute] failed, continuing without hints'" + | |
| " && cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null || true" + | |
| " && tail -200 /tmp/precompute-runner.log 2>/dev/null || true)) && " | |
| " || ({ echo '[precompute] failed, continuing without hints';" + | |
| " cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null || true;" + | |
| " tail -200 /tmp/precompute-runner.log 2>/dev/null || true; })) && " |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
docker/agents/precompute/lib/grep.ts (2)
189-198:⚠️ Potential issue | 🟠 MajorLog
readFileLinesfailures with context.This catch still swallows the error and returns an empty
Map, so broken inputs look identical to empty files. As per coding guidelines, "Always log errors with contextual information".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/agents/precompute/lib/grep.ts` around lines 189 - 198, The catch in readFileLines currently swallows all errors and returns an empty Map, making failures indistinguishable from legitimate empty files; update the catch in the readFileLines function to log the caught error with contextual information (include the path variable and optionally the error stack/message) before returning new Map(), using the project's logger (e.g., processLogger.error) or console.error if no logger is available so failures are visible for debugging.
37-40:⚠️ Potential issue | 🟠 MajorDon’t treat grep failures as “no matches.”
stderr: "ignore"plus no exit-code check means invalid regexes and I/O errors are silently flattened into an empty hint set. That still hides real precompute failures; the code should distinguish grep’s expected “no matches” exit from actual errors before returning.What exit codes does GNU grep return for "match found", "no matches found", and "error", and how is a spawned process's exit status exposed by Bun.spawn?Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/agents/precompute/lib/grep.ts` around lines 37 - 40, The grep Bun.spawn call currently drops stderr and never checks the process exit status (variable child), so regex errors and I/O failures get treated like “no matches”; change stderr from "ignore" to "pipe", await/inspect the child's exit code (use the child.exited promise/exit status returned by Bun.spawn) and distinguish GNU grep codes (0 = matches, 1 = no matches, >=2 = error), returning an empty result only for code 1 but logging/throwing for code >=2 and including stderr content for diagnostics; apply the same fix to the other Bun.spawn invocation around the second usage (lines ~83-86) so both grep spawns behave the same.server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
874-880:⚠️ Potential issue | 🟠 MajorTreat
NullNodeand blank"path"values as missing.Line 875 only skips absent fields. An explicit JSON
nullstill reachesasText(), which becomes"null"and can make an otherwise valid finding look out-of-scope. Please also skipisNull(),isMissingNode(), blank strings, and the literal"null".Suggested fix
- if (pathNode == null) { + if (pathNode == null || pathNode.isNull() || pathNode.isMissingNode()) { continue; } String path = pathNode.asText(); + if (path.isBlank() || "null".equals(path)) { + continue; + } if (diffFiles.contains(path) || isInternalContextPath(path)) { hasInScopeLocation = true; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 874 - 880, The code currently only checks for a missing "path" node before calling pathNode.asText(); update the logic in PullRequestReviewHandler so that after retrieving JsonNode pathNode from loc you also skip when pathNode.isNull() or pathNode.isMissingNode(), and treat empty/blank strings or the literal "null" as missing by retrieving the text (e.g., pathNode.asText() into a local String path) and returning early if path.trim().isEmpty() or path.equalsIgnoreCase("null"); only then evaluate diffFiles.contains(path) or isInternalContextPath(path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@run-go98weh/.precompute/lib/diff-parser.ts`:
- Around line 76-80: The isInDiff function currently falls back to suffix
matching using simple endsWith which can produce false positives; update the
fallback to use normalized, path-boundary-aware matching: normalize both
filePath and each DiffFile.path (convert backslashes to '/', strip './'
prefixes), then consider a match only if one is exactly equal OR one
endsWith('/' + other) (i.e., ensure the suffix is preceded by a path separator);
keep using the diffFiles.get(filePath) exact lookup first, and return
df.addedLines.has(lineNum) as before.
In `@run-go98weh/.precompute/lib/grep.ts`:
- Around line 79-88: The catch block swallows file-read errors; change it to
catch the error (e.g., catch (err)) and log contextual details before returning
an empty Map — for example call console.error or the repo logger with the file
path and the error (mentioning the path variable and the Bun.file(path).text()
call and the lines Map) so failures reading the file are visible for debugging.
- Around line 28-32: Replace the shell-interpolated command construction (the
escapedPattern, cmd string and the Bun.spawn call that uses ["bash","-c", cmd])
with a direct exec using Bun.spawn with an argv array (e.g., ["grep","-rn",
fixedFlag, "--include="+glob, "-m", String(maxResults), escapedPattern, dir]) so
no user input is passed through a shell; remove the "2>/dev/null || true" shell
redirection and instead read the spawned process's stdout/stderr and check the
exit code explicitly (treat code 0 as results, code 1 as no matches, and
non-zero >1 as an error to surface). Apply the same change pattern to the other
instance referenced (the block around lines 95-104) so both uses avoid shell
interpolation and properly handle exit codes and errors.
---
Duplicate comments:
In `@docker/agents/precompute/lib/grep.ts`:
- Around line 189-198: The catch in readFileLines currently swallows all errors
and returns an empty Map, making failures indistinguishable from legitimate
empty files; update the catch in the readFileLines function to log the caught
error with contextual information (include the path variable and optionally the
error stack/message) before returning new Map(), using the project's logger
(e.g., processLogger.error) or console.error if no logger is available so
failures are visible for debugging.
- Around line 37-40: The grep Bun.spawn call currently drops stderr and never
checks the process exit status (variable child), so regex errors and I/O
failures get treated like “no matches”; change stderr from "ignore" to "pipe",
await/inspect the child's exit code (use the child.exited promise/exit status
returned by Bun.spawn) and distinguish GNU grep codes (0 = matches, 1 = no
matches, >=2 = error), returning an empty result only for code 1 but
logging/throwing for code >=2 and including stderr content for diagnostics;
apply the same fix to the other Bun.spawn invocation around the second usage
(lines ~83-86) so both grep spawns behave the same.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 874-880: The code currently only checks for a missing "path" node
before calling pathNode.asText(); update the logic in PullRequestReviewHandler
so that after retrieving JsonNode pathNode from loc you also skip when
pathNode.isNull() or pathNode.isMissingNode(), and treat empty/blank strings or
the literal "null" as missing by retrieving the text (e.g., pathNode.asText()
into a local String path) and returning early if path.trim().isEmpty() or
path.equalsIgnoreCase("null"); only then evaluate diffFiles.contains(path) or
isInternalContextPath(path).
🪄 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: 77d82b53-cdf2-45b2-a523-a5c9b4423b7b
⛔ Files ignored due to path filters (1)
production-practice-review/generated/practices-production-import.jsonis excluded by!**/generated/**
📒 Files selected for processing (17)
docker/agents/claude-code/Dockerfiledocker/agents/opencode/Dockerfiledocker/agents/pi/Dockerfiledocker/agents/precompute/lib/grep.test.tsdocker/agents/precompute/lib/grep.tsproduction-practice-review/BENCHMARK-RESULTS.mdrun-go98weh/.precompute/lib/diff-parser.tsrun-go98weh/.precompute/lib/grep.tsrun-go98weh/.precompute/lib/types.tsserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionResultParser.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapter.javaserver/application-server/src/main/resources/agent/pi-runner.mjsserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxLiveTest.java
✅ Files skipped from review due to trivial changes (3)
- production-practice-review/BENCHMARK-RESULTS.md
- docker/agents/pi/Dockerfile
- run-go98weh/.precompute/lib/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docker/agents/opencode/Dockerfile
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapter.java
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionResultParser.java
- docker/agents/claude-code/Dockerfile
Comment on lines
+28
to
+32
| // Use single quotes for pattern to avoid shell interpretation; escape any ' in pattern | ||
| const escapedPattern = pattern.replace(/'/g, "'\\''"); | ||
| const cmd = `grep -rn ${fixedFlag} --include='${glob}' -m ${maxResults} '${escapedPattern}' '${dir}' 2>/dev/null || true`; | ||
|
|
||
| const result = await Bun.spawn(["bash", "-c", cmd], { |
Contributor
There was a problem hiding this comment.
Stop passing interpolated input through bash -c.
Both helpers compose shell strings from untrusted inputs (pattern, glob, dir, extension). That reintroduces command-injection risk, and the 2>/dev/null || true pattern also turns real grep/find failures into silent empty results. Use Bun.spawn(...) with argument arrays and handle exit codes explicitly.
Also applies to: 95-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run-go98weh/.precompute/lib/grep.ts` around lines 28 - 32, Replace the
shell-interpolated command construction (the escapedPattern, cmd string and the
Bun.spawn call that uses ["bash","-c", cmd]) with a direct exec using Bun.spawn
with an argv array (e.g., ["grep","-rn", fixedFlag, "--include="+glob, "-m",
String(maxResults), escapedPattern, dir]) so no user input is passed through a
shell; remove the "2>/dev/null || true" shell redirection and instead read the
spawned process's stdout/stderr and check the exit code explicitly (treat code 0
as results, code 1 as no matches, and non-zero >1 as an error to surface). Apply
the same change pattern to the other instance referenced (the block around lines
95-104) so both uses avoid shell interpolation and properly handle exit codes
and errors.
…ltering - Fix precompute grep basename-only glob matching (*.swift → **/\*.swift) - Add grep test for auto-expanded basename globs - Fix NullNode/blank path handling in PullRequestReviewHandler filterByDiffScope - Harden pi-runner stdout validation (reject non-JSON in catch path) - Add uncaught exception handler and redaction to pi-runner - Fix shell command grouping in AgentAdapter precompute fallback - Remove unused import in PracticeDetectionResultParser - Add arch test exception for DockerSandboxConfiguration.dockerSandboxAdapter - Rename test to follow should…When… convention - Clean up test artifacts (run-go98weh, production-practice-review) E2E validated: 3 reviews, 39 findings, 100% delivery, $0.19/review avg Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java (1)
91-103:⚠️ Potential issue | 🟠 MajorFix shell redirection scope to capture setup-step failures in the precompute log.
The redirection
> /tmp/precompute-runner.log 2>&1on line 99 applies only tobun run, not the entire pipeline. Ifmkdir,cp, orln-sffails first, the fallback handler executes but cannot inspect the error because the log file is empty or missing—the failing setup step never wrote to/tmp/precompute-runner.log. This blind spot prevents operators from debugging common failure causes during pipeline hardening.Move the closing parenthesis and redirection to wrap all setup commands:
Proposed fix
return ( - "(mkdir -p /workspace/.precompute-out/practices" + + "((mkdir -p /workspace/.precompute-out/practices" + " && cp /workspace/.precompute/practices/*.ts /workspace/.precompute-out/practices/" + " && ln -sf /opt/precompute/lib /workspace/.precompute-out/lib" + " && bun run /opt/precompute/runner.ts" + " --repo /workspace/repo" + " --diff /workspace/.context/diff.patch" + " --metadata /workspace/.context/metadata.json" + " --output /workspace/.precompute-out" + - " > /tmp/precompute-runner.log 2>&1" + + ") > /tmp/precompute-runner.log 2>&1" + " || { echo '[precompute] failed, continuing without hints'" + " && cp /tmp/precompute-runner.log /workspace/.precompute-out/precompute-runner.log 2>/dev/null" + " ; tail -200 /tmp/precompute-runner.log 2>/dev/null" + " ; true; }) && " );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java` around lines 91 - 103, The shell redirection currently only applies to the `bun run` invocation inside the command string built in AgentAdapter.java, so failures in the setup steps (`mkdir -p /workspace/.precompute-out/practices`, `cp /workspace/.precompute/practices/*.ts ...`, `ln -sf /opt/precompute/lib ...`) are not recorded; change the string so the entire subshell (all setup commands plus `bun run`) is wrapped and then redirect stdout/stderr for that whole group to /tmp/precompute-runner.log (i.e., move the closing parenthesis so `(...) > /tmp/precompute-runner.log 2>&1` covers the setup steps and `bun run`), keeping the existing fallback `|| { echo '[precompute] failed, continuing without hints' ... }` intact to ensure the log file contains any setup or runtime errors.
♻️ Duplicate comments (1)
docker/agents/precompute/lib/grep.ts (1)
37-40:⚠️ Potential issue | 🟠 MajorSurface real
grepfailures instead of treating them like “no matches”.Line 39 drops
stderr, and Lines 83-85 never inspect the exit status.grepreturns1for “no match” but>1for actual failures, so bad patterns, unreadable files, or missing binaries currently collapse into silent hint loss.🔎 Proposed fix
async function collectGrepMatches( args: string[], dir: string, maxResults: number, ): Promise<GrepMatch[]> { const child = Bun.spawn(args, { stdout: "pipe", - stderr: "ignore", + stderr: "pipe", }); + let killedForLimit = false; ... matches.push(match); if (matches.length >= maxResults) { - child.kill(); + killedForLimit = true; + child.kill(); break; } } } ... } finally { reader.releaseLock(); - await child.exited; + const exitCode = await child.exited; + if (!killedForLimit && exitCode > 1) { + const stderr = await new Response(child.stderr).text(); + throw new Error(`grep failed for ${dir}: ${stderr.trim()}`); + } }As per coding guidelines, "Use try/catch blocks for async operations" and "Always log errors with contextual information".
Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/agents/precompute/lib/grep.ts` around lines 37 - 40, The spawned grep process currently discards stderr and never checks the exit status, hiding real failures; change the Bun.spawn call (the child variable created with Bun.spawn(args,...)) to capture stderr (e.g., "pipe"), await the process completion (use child.exited or child.wait()), then inspect the exit code: treat exitCode === 1 as “no matches” but if exitCode > 1 throw or log an error including the captured stderr and contextual info (args/pattern) so bad patterns, missing files, or missing grep are surfaced; update the downstream logic that reads stdout to only proceed on exitCode 0 (matches) or 1 (no matches) and propagate/fail on >1.
🧹 Nitpick comments (1)
server/application-server/src/main/resources/agent/pi-runner.mjs (1)
49-61: Secret redaction pattern may miss some sensitive values.The
SECRET_PATTERNregex only matcheskey=valuepatterns. Secrets that appear as bare values (e.g., in JSON like"token": "sk-...") or multi-line formats won't be redacted. Consider whether the 4KB preview is necessary for debugging or if a hash/length-only approach would be safer.Additionally, there's a past review comment flagging that
stdoutPreview/stderrPreviewinrunner-debug.jsoncan leak sensitive data from diffs. The currentclipPreviewredaction helps but may not catch all patterns.🛡️ Consider more conservative redaction or hashing
function clipPreview(text, maxChars = 4000) { if (!text) { return ""; } const clipped = text.length <= maxChars ? text : text.slice(text.length - maxChars); - return clipped.replace(SECRET_PATTERN, (match) => { + // Redact key=value patterns + let redacted = clipped.replace(SECRET_PATTERN, (match) => { const eqIdx = match.indexOf("="); return eqIdx >= 0 ? match.slice(0, eqIdx + 1) + "[REDACTED]" : match; }); + // Also redact common secret prefixes in any context + redacted = redacted.replace(/\b(sk-[a-zA-Z0-9]{20,}|ghp_[a-zA-Z0-9]{36}|gho_[a-zA-Z0-9]{36})\b/g, "[REDACTED]"); + return redacted; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/resources/agent/pi-runner.mjs` around lines 49 - 61, The current SECRET_PATTERN in pi-runner.mjs only catches key=value snippets and misses quoted JSON fields, bare tokens, and multi-line secrets; update clipPreview (and reconsider runner-debug.json previewing) to more conservatively redact/hide secrets by either extending SECRET_PATTERN to also match quoted JSON keys and common token formats (e.g., keys like "token"/"api_key"/"secret" with quoted values and typical token prefixes) and multi-line blocks, or replace the 4KB plaintext preview with a non-reversible representation (e.g., length/hash-only and truncated text) and reduce maxChars; adjust the logic in clipPreview to apply the new pattern or hashing approach and ensure stdoutPreview/stderrPreview generation uses the same safe routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.java`:
- Around line 91-103: The shell redirection currently only applies to the `bun
run` invocation inside the command string built in AgentAdapter.java, so
failures in the setup steps (`mkdir -p /workspace/.precompute-out/practices`,
`cp /workspace/.precompute/practices/*.ts ...`, `ln -sf /opt/precompute/lib
...`) are not recorded; change the string so the entire subshell (all setup
commands plus `bun run`) is wrapped and then redirect stdout/stderr for that
whole group to /tmp/precompute-runner.log (i.e., move the closing parenthesis so
`(...) > /tmp/precompute-runner.log 2>&1` covers the setup steps and `bun run`),
keeping the existing fallback `|| { echo '[precompute] failed, continuing
without hints' ... }` intact to ensure the log file contains any setup or
runtime errors.
---
Duplicate comments:
In `@docker/agents/precompute/lib/grep.ts`:
- Around line 37-40: The spawned grep process currently discards stderr and
never checks the exit status, hiding real failures; change the Bun.spawn call
(the child variable created with Bun.spawn(args,...)) to capture stderr (e.g.,
"pipe"), await the process completion (use child.exited or child.wait()), then
inspect the exit code: treat exitCode === 1 as “no matches” but if exitCode > 1
throw or log an error including the captured stderr and contextual info
(args/pattern) so bad patterns, missing files, or missing grep are surfaced;
update the downstream logic that reads stdout to only proceed on exitCode 0
(matches) or 1 (no matches) and propagate/fail on >1.
---
Nitpick comments:
In `@server/application-server/src/main/resources/agent/pi-runner.mjs`:
- Around line 49-61: The current SECRET_PATTERN in pi-runner.mjs only catches
key=value snippets and misses quoted JSON fields, bare tokens, and multi-line
secrets; update clipPreview (and reconsider runner-debug.json previewing) to
more conservatively redact/hide secrets by either extending SECRET_PATTERN to
also match quoted JSON keys and common token formats (e.g., keys like
"token"/"api_key"/"secret" with quoted values and typical token prefixes) and
multi-line blocks, or replace the 4KB plaintext preview with a non-reversible
representation (e.g., length/hash-only and truncated text) and reduce maxChars;
adjust the logic in clipPreview to apply the new pattern or hashing approach and
ensure stdoutPreview/stderrPreview generation uses the same safe routine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 693d33c2-00e1-4661-8343-5c4fe19c594e
📒 Files selected for processing (10)
.gitignoredocker/agents/precompute/lib/grep.test.tsdocker/agents/precompute/lib/grep.tsserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/adapter/spi/AgentAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionResultParser.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.javaserver/application-server/src/main/resources/agent/pi-runner.mjsserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/CodeQualityTest.java
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- docker/agents/precompute/lib/grep.test.ts
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/adapter/PiAgentAdapterTest.java
FelixTJDietrich
deleted the
fix/server-practice-review-execution-pipeline
branch
April 10, 2026 09:20
Contributor
📚 Documentation Preview
|
Collaborator
Author
|
🎉 This PR is included in version 0.56.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Improvements