fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #252: REST route inventory guard test #259

Closed
agent wants to merge 0 commits from worker/fleetd-252-a830e0-3 into main
pull from: worker/fleetd-252-a830e0-3
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-09-03 10:55:16 +02:00
Member
Copy Link
Copy Source

fleetd #252 — REST route inventory guard test

Scope: one new test file, fleetd/src/test/java/dev/ltms/fleet/rest/RestRouteInventoryTest.java. No production code touched, no docs written.

What it does

Modelled on McpContractDocTest (fleetd #114 / CB-609): reads FleetApp.java's source text
with a regex for app.<verb>("<path>") calls and compares the found set against an explicit
EXPECTED_ROUTES list written in the test (15 routes, verified against the code today). On a
mismatch it fails naming exactly which routes were added (in the code, not in the test) and
which were removed (in the test, not in the code), and tells the reader to update both the
test's EXPECTED_ROUTES and the operator wiki's REST-surface entry — never one without the
other.

A second test pins the scrape is not vacuous (it must find at least as many registrations as
EXPECTED_ROUTES has entries), the same denominator-guard shape McpContractDocTest uses.

/mcp is deliberately excluded from the inventory — it's a raw Jetty ServletHolder mount
(FleetApp.build(), the modifyServletContextHandler call), a different registration
mechanism from every app.<verb>(...) route, so the regex correctly does not see it. This is
documented as a comment on EXPECTED_ROUTES in the test.

Verified the regex finds the conditional GET /metrics registration (inside if (metrics != null)) — it does, because the regex matches the call shape regardless of surrounding control
flow, not the top-level statement list.

Proof the guard can actually fail (required by the brief)

  1. Added a throwaway app.get("/zzz-probe", this::healthz); to FleetApp.java.
  2. Ran mvn -Dtest=RestRouteInventoryTest test. It failed, naming /zzz-probe as added:
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.033 s <<< FAILURE! -- in dev.ltms.fleet.rest.RestRouteInventoryTest
[ERROR] dev.ltms.fleet.rest.RestRouteInventoryTest.theRegisteredRoutesMatchTheExpectedInventory -- Time elapsed: 0.016 s <<< FAILURE!
org.opentest4j.AssertionFailedError: FleetApp's registered REST routes no longer match this test's expected inventory. Added (in FleetApp, not in this test): [GET /zzz-probe]. Removed (in this test, not in FleetApp): []. Update EXPECTED_ROUTES in RestRouteInventoryTest AND the operator wiki's REST-surface entry together — this is the fleetd #252 defect: the route list drifted for a month with nothing checking it. Do NOT weaken this test. ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at dev.ltms.fleet.rest.RestRouteInventoryTest.theRegisteredRoutesMatchTheExpectedInventory(RestRouteInventoryTest.java:97)
...
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE
  1. Reverted FleetApp.java. diff -q against a pre-edit copy and git diff --exit-code src/main/java/dev/ltms/fleet/rest/FleetApp.java both confirmed byte-identical (exit 0).
  2. Re-ran mvn -Dtest=RestRouteInventoryTest,FleetAppTest,FleetAppAuthTest test: all green again
    — Tests run: 44, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS.

Build

cd fleetd && mvn clean install, run unpiped (via tee to a log file, exit code captured
separately with PIPESTATUS), full output read.

  • Compile errors: 0
  • Tests run: 1257, Failures: 0, Errors: 0, Skipped: 0
  • BUILD SUCCESS

The new file contributed 2 of those 1257 tests, both passing.

Files changed

  • fleetd/src/test/java/dev/ltms/fleet/rest/RestRouteInventoryTest.java (new)

Caveats for review

  • The expected inventory is hand-verified against FleetApp.java as of 2026-09-03, matching
    exactly the 15 routes the brief specified. It will need the same two-place update (this test +
    operator wiki) the next time a route changes — the test's Javadoc says so.
  • I did not touch or attempt to read wiki/ (submodule) or fleetd/fleetd.yaml, per the brief.
  • Out-of-scope, noted per the brief's "one line, don't chase it": I did not find any other route
    registered in a way the given list missed — the only registration mechanism in FleetApp.java
    besides the 15 app.<verb>(...) calls is the /mcp ServletHolder mount, which is already
    addressed above.
## fleetd #252 — REST route inventory guard test **Scope:** one new test file, `fleetd/src/test/java/dev/ltms/fleet/rest/RestRouteInventoryTest.java`. No production code touched, no docs written. ### What it does Modelled on `McpContractDocTest` (fleetd #114 / CB-609): reads `FleetApp.java`'s source text with a regex for `app.<verb>("<path>")` calls and compares the found set against an explicit `EXPECTED_ROUTES` list written in the test (15 routes, verified against the code today). On a mismatch it fails naming exactly which routes were **added** (in the code, not in the test) and which were **removed** (in the test, not in the code), and tells the reader to update both the test's `EXPECTED_ROUTES` and the operator wiki's REST-surface entry — never one without the other. A second test pins the scrape is not vacuous (it must find at least as many registrations as `EXPECTED_ROUTES` has entries), the same denominator-guard shape `McpContractDocTest` uses. `/mcp` is deliberately excluded from the inventory — it's a raw Jetty `ServletHolder` mount (`FleetApp.build()`, the `modifyServletContextHandler` call), a different registration mechanism from every `app.<verb>(...)` route, so the regex correctly does not see it. This is documented as a comment on `EXPECTED_ROUTES` in the test. Verified the regex finds the conditional `GET /metrics` registration (inside `if (metrics != null)`) — it does, because the regex matches the call shape regardless of surrounding control flow, not the top-level statement list. ### Proof the guard can actually fail (required by the brief) 1. Added a throwaway `app.get("/zzz-probe", this::healthz);` to `FleetApp.java`. 2. Ran `mvn -Dtest=RestRouteInventoryTest test`. It failed, naming `/zzz-probe` as added: ``` [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.033 s <<< FAILURE! -- in dev.ltms.fleet.rest.RestRouteInventoryTest [ERROR] dev.ltms.fleet.rest.RestRouteInventoryTest.theRegisteredRoutesMatchTheExpectedInventory -- Time elapsed: 0.016 s <<< FAILURE! org.opentest4j.AssertionFailedError: FleetApp's registered REST routes no longer match this test's expected inventory. Added (in FleetApp, not in this test): [GET /zzz-probe]. Removed (in this test, not in FleetApp): []. Update EXPECTED_ROUTES in RestRouteInventoryTest AND the operator wiki's REST-surface entry together — this is the fleetd #252 defect: the route list drifted for a month with nothing checking it. Do NOT weaken this test. ==> expected: <true> but was: <false> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63) at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36) at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214) at dev.ltms.fleet.rest.RestRouteInventoryTest.theRegisteredRoutesMatchTheExpectedInventory(RestRouteInventoryTest.java:97) ... [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` 3. Reverted `FleetApp.java`. `diff -q` against a pre-edit copy and `git diff --exit-code src/main/java/dev/ltms/fleet/rest/FleetApp.java` both confirmed byte-identical (exit 0). 4. Re-ran `mvn -Dtest=RestRouteInventoryTest,FleetAppTest,FleetAppAuthTest test`: all green again — `Tests run: 44, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`. ### Build `cd fleetd && mvn clean install`, run unpiped (via `tee` to a log file, exit code captured separately with `PIPESTATUS`), full output read. - Compile errors: **0** - `Tests run: 1257, Failures: 0, Errors: 0, Skipped: 0` - `BUILD SUCCESS` The new file contributed 2 of those 1257 tests, both passing. ### Files changed - `fleetd/src/test/java/dev/ltms/fleet/rest/RestRouteInventoryTest.java` (new) ### Caveats for review - The expected inventory is hand-verified against `FleetApp.java` as of 2026-09-03, matching exactly the 15 routes the brief specified. It will need the same two-place update (this test + operator wiki) the next time a route changes — the test's Javadoc says so. - I did not touch or attempt to read `wiki/` (submodule) or `fleetd/fleetd.yaml`, per the brief. - Out-of-scope, noted per the brief's "one line, don't chase it": I did not find any other route registered in a way the given list missed — the only registration mechanism in `FleetApp.java` besides the 15 `app.<verb>(...)` calls is the `/mcp` `ServletHolder` mount, which is already addressed above.
agent added 1 commit 2026-09-03 10:55:17 +02:00
fleetd #252: guard test for the REST route inventory
CI / contract (pull_request) Successful in 1m16s
Details
CI / build (pull_request) Successful in 1m27s
Details
eb568ff451 
FleetApp's route list has never been checked against anything and has
already drifted once (GET /member-credentials shipped hours before the
#252 ticket and was missing from its list). Add
RestRouteInventoryTest, modelled on McpContractDocTest, which scrapes
FleetApp.java's app.<verb>("path") calls with a regex and compares
them against an explicit expected inventory, failing loudly with the
added/removed routes when they diverge.
ltms commented 2026-09-03 11:00:55 +02:00
Owner
Copy Link
Copy Source

Merged to main as df90862. Verified independently before merging: diff is exactly the one new file, mvn clean install → 0 compile errors, 1257 tests, BUILD SUCCESS — matching your report.

Your /metrics claim needed no separate check: it is proved by the test passing at all. If the regex missed the conditional registration, actual would be 14 against an expected 15 and the test would fail. Green is the proof.

One defect found by verifying, fixed in 3b3063e

I ran a mutation you did not: removal rather than addition. Commenting out app.get("/tasks/{ticket}", this::taskStatus); left the test green.

The scrape read raw source text, so a registration disabled with // still matched. Deleting the same line was caught correctly, so only the commented-out shape was blind — and it is the silent direction. The inventory would keep claiming a route the server no longer serves, which is exactly the failure this test exists to prevent.

Fix: drop whole-line comments before scraping. Only lines whose first non-blank characters are //, * or /* are dropped — deliberately not every // on a line, because that would also cut a string literal containing // (a URL) and could silently delete a real registration sharing that line. The remaining gap is a trailing comment beside real code; no registration in FleetApp has that shape, and your vacuity test catches a scrape that loses registrations wholesale.

Proof: with the fix, the same comment-out mutation fails with Removed (in this test, not in FleetApp): [GET /tasks/{ticket}]. Reverted, FleetApp.java confirmed clean, full build green at 1257.

Credit where it is due

The vacuity test was your own addition and it is the right instinct — a check that can pass by finding nothing is not a check. It also fired during the removal mutation, which is how I could tell the scrape had genuinely lost a route rather than the assertion being miswritten.

The lesson for next time is narrower: when you prove a guard can fail, mutate in both directions. "Added" and "removed" are different code paths through the same diff, and only one of them was exercised.

Merged to `main` as `df90862`. Verified independently before merging: diff is exactly the one new file, `mvn clean install` → 0 compile errors, 1257 tests, BUILD SUCCESS — matching your report. Your `/metrics` claim needed no separate check: it is proved by the test passing at all. If the regex missed the conditional registration, `actual` would be 14 against an expected 15 and the test would fail. Green *is* the proof. ## One defect found by verifying, fixed in `3b3063e` I ran a mutation you did not: **removal** rather than addition. Commenting out `app.get("/tasks/{ticket}", this::taskStatus);` left the test **green**. The scrape read raw source text, so a registration disabled with `//` still matched. Deleting the same line was caught correctly, so only the commented-out shape was blind — and it is the silent direction. The inventory would keep claiming a route the server no longer serves, which is exactly the failure this test exists to prevent. Fix: drop whole-line comments before scraping. Only lines whose first non-blank characters are `//`, `*` or `/*` are dropped — deliberately **not** every `//` on a line, because that would also cut a string literal containing `//` (a URL) and could silently delete a real registration sharing that line. The remaining gap is a trailing comment beside real code; no registration in `FleetApp` has that shape, and your vacuity test catches a scrape that loses registrations wholesale. Proof: with the fix, the same comment-out mutation fails with `Removed (in this test, not in FleetApp): [GET /tasks/{ticket}]`. Reverted, `FleetApp.java` confirmed clean, full build green at 1257. ## Credit where it is due The vacuity test was your own addition and it is the right instinct — a check that can pass by finding nothing is not a check. It also fired during the removal mutation, which is how I could tell the scrape had genuinely lost a route rather than the assertion being miswritten. The lesson for next time is narrower: when you prove a guard can fail, mutate in **both** directions. "Added" and "removed" are different code paths through the same diff, and only one of them was exercised.
ltms closed this pull request 2026-09-03 11:01:00 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m16s
Details
CI / build (pull_request) Successful in 1m27s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#259
Delete Branch "worker/fleetd-252-a830e0-3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?