Repository navigation
Conversation
❌ Linked Issue Needs Team LabelThis PR is linked to issue #38326, but that issue has no How to fix this:Apply a This comment was automatically generated by the issue linking workflow |
|
Claude finished @wezell's task in 3m 46s —— View job Code Review — core unit-test artifact reuse benchmarkI read all three added files and traced the moving parts: the New Issues
Observations (non-blocking, confirmed OK)
Overall the TDD fixtures are thorough and the "fail, never fall back to compilation" stance is correct for an experiment. The compiler-regex bug is the one that matters: fix it or the · |
| # initialize supplies the Byte Buddy agent path without running compilation. | ||
| ./mvnw "${args[@]}" initialize surefire:test 2>&1 | tee benchmark-maven.log |
There was a problem hiding this comment.
🟡 Medium severity blocking issue identified in your code:
Relative path used for script invocation in hook. Use absolute paths or environment variables like $CLAUDE_PROJECT_DIR, $CURSOR_PROJECT_DIR, or $HOME to ensure the correct script is executed regardless of working directory.
Why this might be safe to ignore:
This is an intentional invocation of the repository’s Maven wrapper from a benchmark/build script, not a hook executing an attacker-controlled relative script. The hook-specific rule appears overly broad for this context, and the surrounding code indicates a controlled project working directory.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by hooks-relative-script-path-bash.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
Summary
Throwaway benchmark — DO NOT MERGE. Test whether the JVM unit-test job is faster when it reuses the production and tests JARs already downloaded in
maven-repo, rather than compiling core again. This is experimental evidence gathering, not a change to production CI.Refs #38326 — reduce CI build and test preparation time.
Changes Made
baselineandreuseon the same Ubuntu runner type with the same initial-build artifacts.testlifecycle. Reuse resolves the project version, validates and unpacks both core JARs (including resources), then runsinitialize surefire:testwithout compilation.Testing
git diff --checkpassed.exec:execcan launch the helper and version evaluation returns1.0.0-SNAPSHOTwithout compilation.Breaking Changes
None to production CI. This branch-only benchmark is deliberately not intended to merge.
Interpretation
The inner benchmark is launched through a common non-recursive Maven
execinvocation after normal artifact restoration. Both paths pay that wrapper overhead. Timings include native setup, Java/cache setup, repository downloads, and candidate JAR restoration; job durations also remain available in Actions. One paired run is exploratory, not a reliable performance estimate. Existing test flakes or runner variance may invalidate the comparison.