RAN-33: Hygiene cluster from RAN-6 review (5 LOW items batched)#70
Conversation
…N-33)
Bundles five low-severity hygiene items so reviewers don't pay per-PR
overhead on items that aren't individually severe.
1. Determinism — Analyzer breakdown-map ordering
`nodeBreakdown`, `edgeBreakdown`, `frameworkBreakdown`, and
`languageBreakdown` were `HashMap`s, which made the JSON serialization
order of `AnalysisResult` non-deterministic across runs. Switched the
12 declarations across `run`, `runBatchedIndex`, and `runSmartIndex`
to `TreeMap`. Adds `breakdownMapsAreSortedDeterministically` test
covering all four breakdowns.
2. Branding hygiene — VersionCommand + BundleCommand
- VersionCommand banner: stale `Code IQ` → `codeiq` (matches the
post-rename CLI / package / repo branding).
- BundleCommand: added a footnote on the bundle-structure javadoc
explaining that `code-iq-*-cli.jar` keeps the historical filename
because it tracks the Maven `artifactId` (intentionally unchanged
per `CLAUDE.md`). Future readers won't trip over it.
Other remaining `code-iq` strings (the `<artifactId>` snippet in the
README and the JAR/Maven-URL templates inside BundleCommand) are
intentional Maven-coordinate references and stay as-is.
3. CORS defaults explicit — CorsConfig
Promoted defaults to named constants
(`DEFAULT_ALLOWED_ORIGIN_PATTERNS`, `API_ALLOWED_METHODS`,
`MCP_ALLOWED_METHODS`, `ALLOWED_HEADERS`) and added a javadoc
explaining the read-only API posture (no `PUT`/`PATCH`/`DELETE`)
and the MCP `GET`/`POST` requirement. Behaviour unchanged — existing
`CorsConfigTest` still passes verbatim. The `@Value` default still
inlines the constant, and the field initializer keeps direct
`new CorsConfig()` test usage working.
4. ExecutorService close/lifecycle regression coverage
Promoted `Analyzer.BoundedExecutor` from `private` to package-private
so its bounded-shutdown contract can be tested without spinning the
full pipeline. Added `AnalyzerBoundedExecutorTest` with three cases:
- long-running task: `close()` returns inside the
graceful (10s) + force (5s) window and the task is interrupted;
- idle executor: `close()` returns promptly;
- interrupted closer thread: delegate is still shut down and the
caller's interrupt flag is restored.
5. IntelligenceController.evidence — symlink hardening
The `/api/intelligence/evidence` endpoint had only the lexical
`normalize` + `startsWith` guard. Aligned it with the two-stage
`toRealPath` guard that already protects `/api/file` (RAN-8): after
the lexical check, resolve symlinks via `Path.toRealPath()` and
re-check containment against the canonical root.
`NoSuchFileException` is treated as "logical-only graph reference"
and the lexical guard is sufficient (no symlink to traverse).
Added `evidenceEndpointRejectsSymlinkEscapingRoot` and
`evidenceEndpointAllowsInRepoSymlink`, both skip gracefully on
filesystems without symlink support — same shape as the existing
GraphController symlink tests.
Verification
- `mvn test -Dnpm.skip=true ...` → 3401 tests run, 0 failures, 0 errors,
31 skipped (E2E without external repos).
- Targeted reruns of `AnalyzerTest`, `AnalyzerBoundedExecutorTest`,
`CorsConfigTest`, `IntelligenceControllerTest`, `VersionCommandTest`
all green.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c3ba4c67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!resolved.startsWith(root)) { | ||
| Path root; | ||
| try { | ||
| root = Path.of(config.getRootPath()).toRealPath(); |
There was a problem hiding this comment.
getEvidence now canonicalizes codeiq.rootPath with Path#toRealPath, which throws when the configured source directory is missing (for example, serving a graph artifact on a host without the original checkout). In that case any request with file=... returns HTTP 500 before reaching assembler.assemble, even though the implementation explicitly supports logical-only graph references (it already ignores NoSuchFileException for the candidate path). This is a behavior regression from the previous lexical guard and breaks valid file-based evidence queries in source-detached deployments.
Useful? React with 👍 / 👎.
|
1 participant
Summary
Closes RAN-33. Batches the five LOW-severity follow-ups from the RAN-6 Reviewer audit so reviewers don't pay per-PR overhead on items that are not individually severe.
Analyzerbreakdown maps (language,node,edge,framework) switched fromHashMaptoTreeMapacross all three pipelines (run,runBatchedIndex,runSmartIndex).AnalysisResultJSON is now byte-stable across runs. NewbreakdownMapsAreSortedDeterministicallytest inAnalyzerTest.VersionCommandbanner: staleCode IQ→codeiq.BundleCommandjavadoc gains a footnote explainingcode-iq-*-cli.jarkeeps the historical filename because it tracks the MavenartifactId(intentionally unchanged perCLAUDE.md). Other remainingcode-iqreferences in README/BundleCommand are intentional Maven-coordinate strings and stay as-is.CorsConfigdefaults promoted to named constants (DEFAULT_ALLOWED_ORIGIN_PATTERNS,API_ALLOWED_METHODS,MCP_ALLOWED_METHODS,ALLOWED_HEADERS) with javadoc explaining the read-only API posture (no PUT/PATCH/DELETE) and the MCPGET/POSTrequirement. Behaviour unchanged; existingCorsConfigTestpasses verbatim.Analyzer.BoundedExecutorpromoted fromprivateto package-private. NewAnalyzerBoundedExecutorTestcovers: long-running task is interrupted within the bounded shutdown window, idle close returns promptly, interrupted closer thread still shuts the delegate down and restores the interrupt flag./api/intelligence/evidencesymlink hardening — Added the same two-stagetoRealPathguard that protects/api/file(RAN-8).NoSuchFileExceptionis treated as a logical-only graph reference (lexical guard already passed). Tests mirror the GraphController symlink shape and skip gracefully on filesystems without symlink support.Test plan
mvn test -Dnpm.skip=true ...→ 3401 tests, 0 failures, 0 errors, 31 skipped (E2E without external repos).AnalyzerTest,AnalyzerBoundedExecutorTest,CorsConfigTest,IntelligenceControllerTest,VersionCommandTestall green.Out of scope
artifactId/ JAR filename — intentionally unchanged perCLAUDE.md.🤖 Generated with Claude Code