java-code-reviewer

# Java Code Reviewer for OpenInference Instrumentors

Review a Java OpenInference instrumentation package against the project's established
patterns and conventions. Report findings with file paths and line numbers, organized
by severity (Critical / High / Medium / Low).

## Workflow

**Step 1: Identify the package to review**
- Ask the user which instrumentor to review if not already clear from context
- The package lives under `java/instrumentation/openinference-instrumentation-<name>/`
- Read the instrumentor source, `build.gradle`, and `src/test/` directory

**Step 2: Use the instrumented library source as ground truth**

Before flagging any finding, verify it against the actual library code. Do NOT assume
how the instrumented library works — read it. Do NOT present findings without having
read the library source first.

- Find the library version from `java/build.gradle` `ext` block
- Check `~/.gradle/caches/modules-2/files-2.1/` for cached sources
- If not cached, download the sources jar from Maven Central (`repo1.maven.org`).
  Some libraries split across multiple artifacts — check `build.gradle` dependency
  declarations and fetch all relevant ones.
- If you cannot obtain the source through any means, explicitly tell the user you
  were unable to verify against the library source before presenting findings.
- Calibrate severity by what the library actually does: a bug on a common code path is
  High/Critical; an edge case for a type that can't appear at runtime is Low

**Step 3: Run all review sections below**

**Step 4: Present findings** in a severity table, list what's working well, then ask
the user: fix issues, run tests (`./gradlew :instrumentation:...:test`), or done.

---

## Section 1: Gradle Setup

Read the instrumentor's `build.gradle` and the root `java/build.gradle`.

- Instrumented library must be `compileOnly` (not `implementation`) — **High**
- `openinference-instrumentation` must be `api`
- Version constants should be in root `ext` block, not hardcoded — **Medium**
- Module must be in `java/settings.gradle` — **Critical** if missing
- Run `cd java && ./gradlew spotlessCheck` (Palantir Java Format)

---

## Section 2: Testing Patterns

### Exhaustive attribute assertions

This is the most important testing pattern. Tests must verify ALL span attributes, not
spot-check a few. The remove-and-verify pattern catches both unexpected additions and
silent removals:

```java
Map<String, Object> attributes = new HashMap<>();
span.getAttributes().forEach((key, value) -> attributes.put(key.getKey(), value));

assertThat(attributes.remove("openinference.span.kind")).isEqualTo("LLM");
assertThat(attributes.remove("llm.model_name")).isEqualTo("gpt-4");
// ... remove and assert all remaining attributes ...
assertThat(attributes).isEmpty();  // Nothing unexpected left
```

Missing emptiness check — **High**.

### Other required test coverage

- Error handling: exception -> span has `StatusCode.ERROR` + recorded exception — **High**
- Context attribute propagation: session_id, user_id, metadata, tags
- TraceConfig masking: verify `hideInputMessages` etc. actually suppress attributes
- Missing test files entirely — **Critical**

---

## Section 3: OpenInference Semantic Conventions

Read `SemanticConventions.java` for the full attribute catalog:
`java/openinference-semantic-conventions/src/main/java/com/arize/semconv/trace/SemanticConventions.java`

Also read the spec files under `spec/` (`semantic_conventions.md`, `traces.md`,
`llm_spans.md`, `embedding_spans.md`, `tool_calling.md`) for expected behavior.

For the library type being reviewed, verify the instrumentor sets all applicable
attributes. Key checks:

- Every span needs `OPENINFERENCE_SPAN_KIND`, `INPUT_VALUE` + `INPUT_MIME_TYPE`,
  `OUTPUT_VALUE` + `OUTPUT_MIME_TYPE`. Missing MIME type when value is set — **High**
- All attributes should use constants from `SemanticConventions`, not hardcoded strings — **Medium**
- Read `TraceConfig.java` for the full list of hide flags; verify each is respected
  where applicable — **Medium** if missing

---

## Section 4: Span Lifecycle and Hierarchy

- `span.end()` must ALWAYS be called — **Critical** if missing
- `Scope` from `context.makeCurrent()` must be closed (try-with-resources) — **High**
- For multi-span instrumentors: verify parent-child nesting and shared `traceId`
- The instrumentor should use `OITracer` (not raw `Tracer`) to get TraceConfig support

---

## Presenting Results

Organize findings into a table:

| Severity | Section | Finding | Location |
|----------|---------|---------|----------|
| Critical | 4 | span.end() not called in error path | `SomeListener.java:142` |
| High | 2 | Tests don't verify all span attributes | `SomeTest.java:85` |
| ... | ... | ... | ... |

Then list what's working well — positive findings help the user understand what doesn't
need to change.