correctness: Add degenerate calibration guard to Calibration transform#5
Conversation
- Add checks in `Calibration` constructor for `scale == 0` or non-finite inputs - Throw `IllegalArgumentException` on degenerate cases - Add tests in `CalibrationTest` to verify degenerate case behavior Co-authored-by: Unluckyathecking <111193970+Unluckyathecking@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Kimi Code Review
This PR adds constructor validation to Calibration.java to prevent degenerate values (zero scale, non-finite doubles) and adds corresponding tests. The core logic is sound, but there are several quality issues: missing validation for Double.NEGATIVE_INFINITY on scale, missing test coverage for negative infinity in origin/angle, a code style violation (commented-out-style bare comment), and test coverage gaps for the two-argument and copy constructors that bypass validation. The PR should not be merged without addressing the negative infinity gap and the style issue.
Score: 72/100
| Severity | Count |
|---|---|
| 🟡 warning | 4 |
| ⚪ nitpick | 1 |
📋 Action plan for @jules
Jules, complete the items below — critical first, then warning. Push fixes to the same branch and I will re-review automatically. If any item genuinely can't be fixed in this PR's scope, leave a yes/no question for me in the PR description (prefix it with @kimi) and proceed with the rest.
- 🟡 [warning]
src/main/java/org/opensourcephysics/cabrillo/tracker/calibration/Calibration.java:27— Scale validation rejects POSITIVE_INFINITY but not NEGATIVE_INFINITY explicitly in message- Why: The validation
!Double.isFinite(scale)already catches bothPOSITIVE_INFINITYandNEGATIVE_INFINITY, so the check is functionally correct. However, the error message says"Invalid scale: " + scalewhich will print-Infinityfor ne
- Why: The validation
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:85— Missing test coverage for Double.NEGATIVE_INFINITY on scale and origin- Why: The
testDegenerateScaleandtestDegenerateOrigintests coverNaNandPOSITIVE_INFINITYbut omitDouble.NEGATIVE_INFINITY. SinceDouble.isFiniterejects both infinities, both should be tested to prevent regression if the implemen - Suggested fix:
@Test public void testDegenerateScale() { assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); } @Test public void testDegenerateOrigin() { assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0)); assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0)); // … (truncated; full diff in the inline comment)
- Why: The
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:99— Missing test for Double.NEGATIVE_INFINITY on angle- Why:
testDegenerateAngleonly testsDouble.NaN. It should also testDouble.POSITIVE_INFINITYandDouble.NEGATIVE_INFINITYfor completeness, matching the pattern of the other degenerate tests. - Suggested fix:
@Test public void testDegenerateAngle() { assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN)); assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY)); assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY)); }
- Why:
- 🟡 [warning]
src/main/java/org/opensourcephysics/cabrillo/tracker/calibration/Calibration.java:19— Two-argument constructor delegates to validated constructor — OK, but withScale bypasses validation- Why: The
withScale,withAngle, andwithOriginmethods all call the 4-arg constructor which now has validation. This is correct. However, the no-arg constructornew Calibration()callsthis(1.0, 0, 0, 0)which passes validation. The two
- Why: The
Token Usage & Cost
- Input: 3,107 tokens
- Output: 1,291 tokens
- Cached: 2,560 tokens
- Estimated cost: $0.0039
Powered by Kimi Code Reviewer — Moonshot AI 256K context
Note: Some inline comments could not be placed on the diff.
- Add checks in `Calibration` constructor for `scale == 0` or non-finite inputs - Throw `IllegalArgumentException` on degenerate cases - Add tests in `CalibrationTest` to verify degenerate case behavior Co-authored-by: Unluckyathecking <111193970+Unluckyathecking@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Kimi Code Review
This PR adds constructor validation to Calibration.java to reject non-finite and zero scale values, and adds corresponding tests. The core change is correct and necessary. However, the PR includes update_tests.py and fix.patch — artifacts of the AI agent's own editing process — which must not be merged into the repository. The test file also contains extraneous blank lines. The validation logic itself is sound: Double.isFinite correctly rejects NaN and infinities, and scale == 0.0 catches zero (both positive and negative zero are handled since 0.0 == -0.0). I would not approve until the artifacts are removed.
Score: 55/100
| Severity | Count |
|---|---|
| 🔴 critical | 2 |
| 🔵 suggestion | 2 |
| ⚪ nitpick | 1 |
📋 Action plan for @jules
Jules, complete the items below — critical first, then warning. Push fixes to the same branch and I will re-review automatically. If any item genuinely can't be fixed in this PR's scope, leave a yes/no question for me in the PR description (prefix it with @kimi) and proceed with the rest.
- 🔴 [critical]
update_tests.py:1— Build artifact / AI tool script committed to repository- Why: The file
update_tests.pyis a Python script used by the AI agent to mechanically edit the test file. It is not part of the codebase, has no production or test value, and should never be committed. It contains hardcoded string replacements
- Why: The file
- 🔴 [critical]
fix.patch:1— Patch file committed to repository- Why: The file
fix.patchis a git diff representing the test changes. It is redundant with the actual changes already applied toCalibrationTest.javaand serves no purpose in the repository. Patch files committed to source control are a class
- Why: The file
Token Usage & Cost
- Input: 6,176 tokens
- Output: 974 tokens
- Cached: 2,560 tokens
- Estimated cost: $0.0045
Powered by Kimi Code Reviewer — Moonshot AI 256K context
| }""") | ||
|
|
||
| with open(file_path, "w") as f: | ||
| f.write(content) |
There was a problem hiding this comment.
🔴 [critical] Build artifact / AI tool script committed to repository
The file update_tests.py is a Python script used by the AI agent to mechanically edit the test file. It is not part of the codebase, has no production or test value, and should never be committed. It contains hardcoded string replacements that are brittle and specific to this one-time editing task. Committing this creates maintenance burden and confusion. Remove entirely.
| + assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY)); | ||
| + assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 [critical] Patch file committed to repository
The file fix.patch is a git diff representing the test changes. It is redundant with the actual changes already applied to CalibrationTest.java and serves no purpose in the repository. Patch files committed to source control are a classic AI-agent slop artifact. Remove entirely.
| if (!Double.isFinite(scale) || scale == 0.0) { | ||
| throw new IllegalArgumentException("Invalid scale: " + scale); | ||
| } | ||
| if (!Double.isFinite(originX)) { |
There was a problem hiding this comment.
🔵 [suggestion] Negative zero scale passes validation but may be degenerate
The check scale == 0.0 allows -0.0 to pass since -0.0 == 0.0 is true in Java. However, Double.isFinite(-0.0) is also true. The downstream method toPixel divides by scale (worldX / scale and worldY / scale). Division by -0.0 produces -Infinity or Infinity (depending on sign of numerator), which is likely not intended. If the goal is to reject any zero scale, the current code actually works because scale == 0.0 catches both 0.0 and -0.0. But the error message will print -0.0 which may confuse. Consider using scale == 0.0 (already done) but documenting that both zeros are rejected, or explicitly checking Double.compare(scale, 0.0) == 0 if you want to be explicit. This is minor since the behavior is correct; flagging for awareness only.
| assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0)); |
There was a problem hiding this comment.
🔵 [suggestion] Tests do not verify exception messages
The assertThrows calls verify the exception type but not the message content. A future change could throw IllegalArgumentException for the wrong parameter (e.g. a copy-paste error where originX validation throws with "Invalid scale" instead of "Invalid originX"). Consider adding message verification using assertThrows with the returned exception, e.g.:
IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
assertTrue(ex.getMessage().contains("scale"));This is suggestion-level because the current tests do exercise the correct code paths.
- Add checks in `Calibration` constructor for `scale == 0` or non-finite inputs - Throw `IllegalArgumentException` on degenerate cases - Add tests in `CalibrationTest` to verify degenerate case behavior Co-authored-by: Unluckyathecking <111193970+Unluckyathecking@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Kimi Code Review
This PR adds constructor parameter validation to Calibration.java to prevent degenerate values (non-finite scale, origin, angle; zero scale) and adds corresponding tests. The validation logic is correct and necessary—zero scale would cause divide-by-zero in toPixel, and non-finite values would propagate NaN/infinity through all coordinate transforms. However, the tests have significant issues: missing assertions for exception messages (allowing false positives if wrong exception type is thrown), no coverage for the withScale/withOrigin/withAngle convenience methods that bypass validation by delegating to the constructor (actually they do delegate, so they are covered indirectly), but more critically the test file has excessive blank lines and the exception tests don't verify which specific parameter triggered the exception. The implementation itself is solid but lacks Javadoc on the thrown exception and doesn't use a constant for the error message prefix pattern.
Score: 72/100
| Severity | Count |
|---|---|
| 🟡 warning | 1 |
| 🔵 suggestion | 1 |
| ⚪ nitpick | 1 |
📋 Action plan for @jules
Jules, complete the items below — critical first, then warning. Push fixes to the same branch and I will re-review automatically. If any item genuinely can't be fixed in this PR's scope, leave a yes/no question for me in the PR description (prefix it with @kimi) and proceed with the rest.
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:90— Exception tests do not assert exception message or cause- Why: The
assertThrowscalls only verify that someIllegalArgumentExceptionis thrown, not that it relates to the expected parameter. If a future refactoring introduces a differentIllegalArgumentExceptionearlier in the constructor (e.g. - Suggested fix:
@Test public void testDegenerateScale() { IllegalArgumentException ex; ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); }
- Why: The
Token Usage & Cost
- Input: 1,664 tokens
- Output: 956 tokens
- Cached: 4,352 tokens
- Estimated cost: $0.0029
Powered by Kimi Code Reviewer — Moonshot AI 256K context
| public void testDegenerateScale() { | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); | ||
| assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); |
There was a problem hiding this comment.
🟡 [warning] Exception tests do not assert exception message or cause
The assertThrows calls only verify that some IllegalArgumentException is thrown, not that it relates to the expected parameter. If a future refactoring introduces a different IllegalArgumentException earlier in the constructor (e.g., a null check on a new parameter), these tests could pass for the wrong reason. Use assertThrows with the returned exception to verify the message contains "scale", "originX", etc., or use assertEquals on the message. For example, line 91 assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)) should verify the message is "Invalid scale: 0.0".
Suggested fix:
| assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); | |
| @Test | |
| public void testDegenerateScale() { | |
| IllegalArgumentException ex; | |
| ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); | |
| assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); | |
| ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); | |
| assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); | |
| ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); | |
| assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); | |
| ex = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); | |
| assertTrue(ex.getMessage().contains("scale"), ex.getMessage()); | |
| } |
| if (!Double.isFinite(scale) || scale == 0.0) { | ||
| throw new IllegalArgumentException("Invalid scale: " + scale); | ||
| } | ||
| if (!Double.isFinite(originX)) { |
There was a problem hiding this comment.
🔵 [suggestion] Magic string pattern in exception messages
The error message prefix "Invalid scale: " and similar for other parameters use inline string literals. While the messages are clear, extracting a format pattern or using a helper would improve maintainability if error message formatting needs to change consistently across the class. This is minor given the small size of the class.
Suggested fix:
| if (!Double.isFinite(originX)) { | |
| private static String invalidParam(String name, double value) { | |
| return "Invalid " + name + ": " + value; | |
| } | |
| public Calibration(double scale, double originX, double originY, double angle) { | |
| if (!Double.isFinite(scale) || scale == 0.0) { | |
| throw new IllegalArgumentException(invalidParam("scale", scale)); | |
| } |
- Add checks in `Calibration` constructor for `scale == 0` or non-finite inputs - Throw `IllegalArgumentException` on degenerate cases - Add tests in `CalibrationTest` to verify degenerate case behavior Co-authored-by: Unluckyathecking <111193970+Unluckyathecking@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Kimi Code Review
This PR adds constructor validation to Calibration.java to reject non-finite and zero scale values, and non-finite origin/angle values. The validation logic is correct and the tests cover the specified cases. However, the tests contain a subtle but real bug: assertTrue(ex.getMessage().contains("originX")) will pass for ex2 in testDegenerateOrigin because the exception message contains "Invalid originX: NaN" which does contain the substring "originX" — wait, let me re-check. Actually ex2 is for originY with NaN, and the message is "Invalid originY: NaN". The assertion checks contains("originY") which is correct. Let me re-verify all assertions more carefully against the actual exception messages produced by the constructor.
Re-checking: The constructor uses "Invalid scale: ", "Invalid originX: ", "Invalid originY: ", "Invalid angle: ". All test assertions check for the correct substring. The tests are actually correct.
However, I found a real issue: the test testDegenerateScale does not test negative scale values. Negative scale is mathematically valid (flips the coordinate system) but may be unintentional. More importantly, the PR description mentions "degenerate bounds" which doesn't match the actual change — the change validates constructor parameters, not bounds.
The most significant issue is that testDegenerateOrigin has ex3 through ex6 testing infinities, but ex1 and ex2 test NaN for originX and originY respectively. The assertions all correctly match their respective parameter names. No bug found there.
Wait — I need to re-examine more carefully. Looking at testDegenerateOrigin line by line:
ex1:originX=NaN, checkscontains("originX")✓ (message: "Invalid originX: NaN")ex2:originY=NaN, checkscontains("originY")✓ (message: "Invalid originY: NaN")ex3:originX=+∞, checkscontains("originX")✓ex4:originX=-∞, checkscontains("originX")✓ex5:originY=+∞, checkscontains("originY")✓ex6:originY=-∞, checkscontains("originY")✓
All correct. But I need to look for other issues. The test file has 4 blank lines added (lines 85-88 in the diff: +\n+\n+\n+), which is sloppy. The testDegenerateScale doesn't test negative scale — but negative scale is allowed by the validation (scale == 0.0 only rejects zero, not negative). This is actually correct mathematically.
Actually, I found a real issue: testDegenerateOrigin line with ex3 checks contains("originX") for POSITIVE_INFINITY on originX. But wait — let me re-read the diff more carefully. The ex variables are declared as IllegalArgumentException ex1 etc., and each assertion is separate. This is fine.
Hmm, let me look for the actual bug I'm missing. The PR says "degenerate bounds" in description but implements constructor parameter validation — that's a documentation/communication issue, not a code bug.
Actually, re-reading: I need to check if there's an issue with the scale == 0.0 check. Double.compare(scale, 0.0) == 0 would distinguish -0.0 from 0.0, but scale == 0.0 is true for both 0.0 and -0.0. Since -0.0 is a valid double with Double.isFinite(-0.0) == true, but scale == 0.0 is also true for -0.0. However, -0.0 as scale would cause division by zero in toPixel just like 0.0 would. So this is actually correct — -0.0 should be rejected too, and it is.
Let me check once more for real issues. The blank lines in the test (4 consecutive newlines) are a style issue. The test methods don't have @DisplayName annotations, but that's not required.
Wait — I need to re-verify my understanding of the code. In toPixel, we have double wx = worldX / scale; — if scale is negative, this just flips the world coordinates, which is mathematically valid. The scale == 0.0 check correctly allows negative scale.
I think I need to be more skeptical. Let me check: does Double.isFinite(0.0) return true? Yes. Does Double.isFinite(-0.0) return true? Yes. Is -0.0 == 0.0? Yes. So -0.0 is correctly rejected.
Is there any issue with the validation order? If scale is NaN, we check !Double.isFinite(scale) first, which is true, so we throw before checking scale == 0.0. Good — no weird short-circuit issues.
I'm struggling to find a real bug. Let me re-read the Anti-Slop Checklist more carefully.
Item 6: Tests — "new tests actually assert behavior. NOT expect(true).toBe(true)". The tests here assert that exceptions are thrown and that messages contain certain substrings. This is valid but minimal. They don't test edge cases like -0.0 for scale. They don't test that valid values near boundaries work (e.g., Double.MIN_NORMAL for scale).
Item 8: Magic values — the tests use magic values like 1.0, 0, 0, 0 repeatedly. These are reasonable in tests.
Item 15: Migrations & destructive operations — not applicable.
Item 16: AI-generated giveaways — the 4 blank lines between methods is a giveaway. The tests are very formulaic.
Let me check if there's a bug in the existing tests that I missed. Looking at testToPixel:
c = c.withScale(2.0, 0, 0);
Calibration.PixelPoint pixel = c.toPixel(200.0, 400.0);
assertEquals(100.0, pixel.x(), 0.001);
assertEquals(-200.0, pixel.y(), 0.001); // Y flips backWith scale=2.0, world (200, 400) -> wx=100, wy=200. No rotation, so dx=100, dy=200. Pixel is (0+100, 0-200) = (100, -200). Correct.
I think the actual issues are:
- The 4 blank lines added in the test file (style/nitpick)
- The tests don't cover negative scale (which is allowed — this is actually fine, not a bug)
- The tests don't verify the exact exception message, just that it contains a substring (could be too permissive if message format changes, but not a bug)
- Missing test for
-0.0scale — but this is handled correctly byscale == 0.0
Actually, I want to double-check one thing. In testDegenerateScale, the assertion is:
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
assertTrue(ex1.getMessage().contains("scale"));The message is "Invalid scale: 0.0". contains("scale") returns true. Correct.
For ex2: "Invalid scale: NaN". contains("scale") — wait, does it? "Invalid scale: NaN" contains "scale"? Let me check: "Invalid scale: NaN" — yes, "scale" appears in "scale".
Hmm, but what about ex1 in testDegenerateOrigin: message is "Invalid originX: NaN". contains("originX") — "originX" is in "originX". Yes.
Wait — I need to be more careful. The string "originX" contains "originX". But does the message "Invalid originX: NaN" contain the substring "originX"? Yes, it literally contains "originX".
OK so the tests are correct. Let me think about what real issues exist...
The PR is actually quite solid. The main issues are:
- 4 blank lines in test file (style)
- Missing
@DisplayNameor more descriptive test names (suggestion) - Tests could be more thorough — e.g., test that the exception message format is exactly as expected, or test boundary values
- The PR description mentions "degenerate bounds" which is inaccurate — it's constructor parameter validation
But I need to be strict. Let me look once more for any real bug...
Actually, I want to check: is there any issue with Double.isFinite on Java versions? Double.isFinite was added in Java 8. If this project targets Java 7 or earlier, this would be a compilation error. But looking at the existing code, it uses var (local variable type inference) which is Java 10+, and records which are Java 16+. So Java 8+ is assumed. Double.isFinite is fine.
Hmm, wait. Let me re-check the source file. The Calibration class uses public static record WorldPoint(double x, double y) {} — records are Java 16+. Double.isFinite is Java 8+. No issue.
I think I've been sufficiently skeptical. The code is correct. The only real issues are style-level: the 4 blank lines. But wait — let me re-check if there's a bug in my understanding of the validation.
!Double.isFinite(scale) || scale == 0.0
For scale = -0.0: Double.isFinite(-0.0) is true, so !Double.isFinite(-0.0) is false. Then scale == 0.0 — is -0.0 == 0.0 in Java? Yes, -0.0 == 0.0 evaluates to true. So -0.0 is correctly rejected.
For scale = Double.NaN: Double.isFinite(Double.NaN) is false, so !Double.isFinite(Double.NaN) is true. Short-circuits, throws.
For scale = Double.POSITIVE_INFINITY: Double.isFinite(...) is false, throws.
All correct.
I'm going to file the blank lines as a nitpick, and note that the tests are somewhat minimal but functional. Given the strict scoring guidelines, this is probably an 85-90 score — the code is correct and well-tested, but has minor style issues.
Actually, re-reading the instructions: "Bias toward LOWER scores — most AI-generated diffs score 60–80 honestly. Do not award 90+ unless the diff would survive review by a skeptical staff engineer."
The 4 blank lines are sloppy. The tests are formulaic and minimal (just checking exception type and message substring). I'll score this around 82.
Wait, I want to check one more thing. Are there any issues with the existing tests that the new tests might conflict with? No, the new tests are independent.
Is there any issue with thread safety? Calibration is immutable, so it's inherently thread-safe. The validation in the constructor happens before publication, so no issue.
Final check: the diff shows +\n+\n+\n+ at lines 85-88 of the test diff. That's 4 blank lines between testRotatedRoundTrip and testDegenerateScale. In the original file, there was already a blank line after testRotatedRoundTrip (line 84 ends with } and line 85 in the new file is blank). The diff adds 3 more blank lines. This is definitely sloppy.
I'll file:
- nitpick: excessive blank lines in test file
- suggestion: tests could be more rigorous (exact message matching, boundary values)
Actually, let me reconsider. The instructions say "Silence on a non-trivial diff (>30 lines of real logic) reflects badly on YOU." This diff is about 53 lines (12 + 41), so it's non-trivial. I need to make sure I've found everything.
Let me re-examine: is there any issue with the IllegalArgumentException messages leaking internal details? The messages contain the actual invalid values. For scale, this could leak if the value is somehow sensitive, but these are just calibration parameters. Not a security issue.
Is there any issue with the validation preventing legitimate use cases? Negative scale is allowed, which is correct. Zero scale is rejected, which is correct (would cause division by zero). Non-finite values are rejected, which is correct.
One more thought: should angle validation reject infinite angles? An infinite angle doesn't make mathematical sense for a rotation, so yes, rejecting it is correct.
OK, I'm confident the code is correct. I'll file the style issue and a suggestion for more thorough testing.
Actually, I want to check one more thing. Looking at the test file structure:
@Test
public void testRotatedRoundTrip() {
...
}
@Test
public void testDegenerateScale() {That's 4 blank lines. In Java style guides (Google, Sun), typically 1 blank line between methods is standard. 4 is excessive and indicates sloppiness.
Also, I notice the new tests don't follow the naming pattern of existing tests perfectly. Existing: testDefaultIdentityTransform, testSetScale, testToPixel, testWithOrigin, testIdentityRotation, testRotation90Degrees, testRotation180Degrees, testRotatedRoundTrip. New: testDegenerateScale, testDegenerateOrigin, testDegenerateAngle. These are reasonable names.
Hmm, I want to check if there's an issue with assertTrue(ex.getMessage().contains(...)) vs assertEquals or assertThrows with expected message. Using contains is less strict and could pass if the message format changes inadvertently. But it's not a bug.
Wait — I need to re-verify one thing. In testDegenerateOrigin, ex3 checks contains("originX") for POSITIVE_INFINITY on originX. But the message is "Invalid originX: Infinity". Does "Infinity" contain "Infinity"? Yes. But wait — in Java, Double.POSITIVE_INFINITY string representation is "Infinity", not "POSITIVE_INFINITY". So the message is "Invalid originX: Infinity". And contains("originX") checks for the substring "originX", which is present. Correct.
For Double.NEGATIVE_INFINITY, the string representation is "-Infinity". Message: "Invalid originX: -Infinity". contains("originX") — yes.
All good.
Final answer: score 82, with annotations for excessive blank lines and a suggestion for more rigorous test assertions.
Score: 82/100
| Severity | Count |
|---|---|
| 🔵 suggestion | 1 |
| ⚪ nitpick | 1 |
Token Usage & Cost
- Input: 2,158 tokens
- Output: 3,780 tokens
- Cached: 4,352 tokens
- Estimated cost: $0.0085
Powered by Kimi Code Reviewer — Moonshot AI 256K context
| public void testDegenerateScale() { | ||
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); | ||
| assertTrue(ex1.getMessage().contains("scale")); | ||
| IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); |
There was a problem hiding this comment.
🔵 [suggestion] Test assertions use substring matching instead of exact validation
The degenerate tests use assertTrue(ex.getMessage().contains("scale")) which is overly permissive. If the exception message format changes to "Invalid originX: scale parameter" by mistake, the testDegenerateScale test for originX would still pass. More robust tests would verify the exact message or use a custom matcher. Additionally, the tests do not cover the -0.0 scale boundary case (which is correctly rejected by scale == 0.0, but not explicitly tested).
Suggested fix:
| IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); | |
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); | |
| assertEquals("Invalid scale: 0.0", ex1.getMessage()); |
|
Kimi has reviewed this PR — no remaining critical or warning issues. claude:suggested merge |
There was a problem hiding this comment.
🤖 Kimi Code Review
This PR adds constructor parameter validation to Calibration.java to prevent degenerate values (non-finite scale, zero scale, non-finite origins/angles) and adds corresponding tests. The validation logic is correct and addresses a real correctness issue. However, there are several quality issues: the tests contain excessive blank lines (likely AI-generated slop), tests only verify exception message substrings rather than ensuring the exact field is validated first (permitting false positives if message format changes), and the tests lack coverage for negative scale values which the constructor currently accepts but may be semantically invalid. The code is mergeable after cleanup.
Score: 72/100
| Severity | Count |
|---|---|
| 🟡 warning | 5 |
| ⚪ nitpick | 1 |
📋 Action plan for @jules
Jules, complete the items below — critical first, then warning. Push fixes to the same branch and I will re-review automatically. If any item genuinely can't be fixed in this PR's scope, leave a yes/no question for me in the PR description (prefix it with @kimi) and proceed with the rest.
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:89— Weak test assertions using message substring matching- Why: The
testDegenerateScalemethod (lines 89-98) usesassertTrue(ex.getMessage().contains("scale"))to verify which validation failed. This is brittle: if theIllegalArgumentExceptionmessage format changes to `"originX must be finite, go - Suggested fix:
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); assertEquals("Invalid scale: 0.0", ex1.getMessage()); IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); assertEquals("Invalid scale: NaN", ex2.getMessage()); IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); assertEquals("Invalid scale: Infinity", ex3.getMessage()); IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); assertEquals("Invalid scale: -Infinity", ex4.getMessage());
- Why: The
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:89— Missing test for negative scale values- Why: The constructor at line 27 in
Calibration.javaaccepts negative scale values (scale == -2.0passes thescale != 0.0check). Negative scale would causetoWorldto flip coordinates andtoPixelto produce inverted results, which is l
- Why: The constructor at line 27 in
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:100— Weak test assertions using message substring matching- Why: The
testDegenerateOriginmethod (lines 100-111) usesassertTrue(ex.getMessage().contains("originX"))and similar substring checks. Same brittleness issue astestDegenerateScale: these assertions could pass for the wrong reasons if mes - Suggested fix:
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0)); assertEquals("Invalid originX: NaN", ex1.getMessage()); IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0)); assertEquals("Invalid originY: NaN", ex2.getMessage()); IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0)); assertEquals("Invalid originX: Infinity", ex3.getMessage()); IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0)); assertEquals("Invalid originX: -Infinity", ex4.getMessage()); IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0)); assertEquals("Invalid originY: Infinity", ex5.getMessage()); IllegalArgumentException ex6 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NEGATIVE_INFINITY, 0)); assertEquals("Invalid originY: -Infinity", ex6.getMessage());
- Why: The
- 🟡 [warning]
src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java:113— Weak test assertions using message substring matching- Why: The
testDegenerateAnglemethod (lines 113-120) usesassertTrue(ex.getMessage().contains("angle"))for validation. Same brittleness issue as other degenerate tests. - Suggested fix:
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN)); assertEquals("Invalid angle: NaN", ex1.getMessage()); IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY)); assertEquals("Invalid angle: Infinity", ex2.getMessage()); IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY)); assertEquals("Invalid angle: -Infinity", ex3.getMessage());
- Why: The
- 🟡 [warning]
src/main/java/org/opensourcephysics/cabrillo/tracker/calibration/Calibration.java:27— Negative scale values accepted but likely semantically invalid- Why: Line 27 checks
scale == 0.0but permits negative scale values. A negative scale (meters per pixel) would invert coordinate transformations intoWorld(line 54:rx * scale) andtoPixel(line 66:worldX / scale), producing mirrored - Suggested fix:
if (!Double.isFinite(scale) || scale <= 0.0) {
- Why: Line 27 checks
Token Usage & Cost
- Input: 110 tokens
- Output: 1,739 tokens
- Cached: 6,400 tokens
- Estimated cost: $0.004
Powered by Kimi Code Reviewer — Moonshot AI 256K context
| IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); | ||
| assertTrue(ex3.getMessage().contains("scale")); | ||
| IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); | ||
| assertTrue(ex4.getMessage().contains("scale")); |
There was a problem hiding this comment.
🟡 [warning] Weak test assertions using message substring matching
The testDegenerateScale method (lines 89-98) uses assertTrue(ex.getMessage().contains("scale")) to verify which validation failed. This is brittle: if the IllegalArgumentException message format changes to "originX must be finite, got scale: ..." or similar, the test would still pass incorrectly. The tests should assert the exact message or use a more precise validation mechanism. Additionally, the tests do not verify that the first failing field is the one reported, so a bug that validates originX before scale would not be caught.
Suggested fix:
| assertTrue(ex4.getMessage().contains("scale")); | |
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0)); | |
| assertEquals("Invalid scale: 0.0", ex1.getMessage()); | |
| IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0)); | |
| assertEquals("Invalid scale: NaN", ex2.getMessage()); | |
| IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); | |
| assertEquals("Invalid scale: Infinity", ex3.getMessage()); | |
| IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); | |
| assertEquals("Invalid scale: -Infinity", ex4.getMessage()); |
| IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0)); | ||
| assertTrue(ex3.getMessage().contains("scale")); | ||
| IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0)); | ||
| assertTrue(ex4.getMessage().contains("scale")); |
There was a problem hiding this comment.
🟡 [warning] Missing test for negative scale values
The constructor at line 27 in Calibration.java accepts negative scale values (scale == -2.0 passes the scale != 0.0 check). Negative scale would cause toWorld to flip coordinates and toPixel to produce inverted results, which is likely not semantically valid for a physical calibration (meters per pixel should be positive). The tests should verify that negative scale is rejected, or the constructor should be updated to require scale > 0.0 instead of scale != 0.0. Given the physical meaning of scale, scale > 0.0 is more appropriate.
| assertTrue(ex3.getMessage().contains("originX")); | ||
| IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0)); | ||
| assertTrue(ex4.getMessage().contains("originX")); | ||
| IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0)); |
There was a problem hiding this comment.
🟡 [warning] Weak test assertions using message substring matching
The testDegenerateOrigin method (lines 100-111) uses assertTrue(ex.getMessage().contains("originX")) and similar substring checks. Same brittleness issue as testDegenerateScale: these assertions could pass for the wrong reasons if message formats change or if validation ordering bugs are introduced.
Suggested fix:
| IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0)); | |
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0)); | |
| assertEquals("Invalid originX: NaN", ex1.getMessage()); | |
| IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0)); | |
| assertEquals("Invalid originY: NaN", ex2.getMessage()); | |
| IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0)); | |
| assertEquals("Invalid originX: Infinity", ex3.getMessage()); | |
| IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0)); | |
| assertEquals("Invalid originX: -Infinity", ex4.getMessage()); | |
| IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0)); | |
| assertEquals("Invalid originY: Infinity", ex5.getMessage()); | |
| IllegalArgumentException ex6 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NEGATIVE_INFINITY, 0)); | |
| assertEquals("Invalid originY: -Infinity", ex6.getMessage()); |
| @Test | ||
| public void testDegenerateAngle() { | ||
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN)); | ||
| assertTrue(ex1.getMessage().contains("angle")); |
There was a problem hiding this comment.
🟡 [warning] Weak test assertions using message substring matching
The testDegenerateAngle method (lines 113-120) uses assertTrue(ex.getMessage().contains("angle")) for validation. Same brittleness issue as other degenerate tests.
Suggested fix:
| assertTrue(ex1.getMessage().contains("angle")); | |
| IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN)); | |
| assertEquals("Invalid angle: NaN", ex1.getMessage()); | |
| IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY)); | |
| assertEquals("Invalid angle: Infinity", ex2.getMessage()); | |
| IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY)); | |
| assertEquals("Invalid angle: -Infinity", ex3.getMessage()); |
| } | ||
|
|
||
| public Calibration(double scale, double originX, double originY, double angle) { | ||
| if (!Double.isFinite(scale) || scale == 0.0) { |
There was a problem hiding this comment.
🟡 [warning] Negative scale values accepted but likely semantically invalid
Line 27 checks scale == 0.0 but permits negative scale values. A negative scale (meters per pixel) would invert coordinate transformations in toWorld (line 54: rx * scale) and toPixel (line 66: worldX / scale), producing mirrored world coordinates. For a physical calibration system, scale should be strictly positive. The check should be scale <= 0.0 instead of scale == 0.0, or the validation should use scale > 0.0.
Suggested fix:
| if (!Double.isFinite(scale) || scale == 0.0) { | |
| if (!Double.isFinite(scale) || scale <= 0.0) { |
1 participant
Work order id:
2026-05-25T17-52-02Z.1I found that
Calibration.javalacked validation on its constructor parameters as specified in the work order. Downstream code using theCalibrationobject could potentially be exposed to divide-by-zero or non-finite coordinates if degenerate bounds were provided.I added checks to ensure
scaleis finite and non-zero, andoriginX,originY, andangleare finite. If invalid, anIllegalArgumentExceptionis thrown. I also added corresponding test cases toCalibrationTest.java.PR created automatically by Jules for task 10208146024754781858 started by @Unluckyathecking