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

PackageCyclesTest's javadoc promises a guarantee its implementation does not give: a new dependency between an already-excepted package pair is NOT checked #749

New Issue
Closed
opened 2026-10-05 06:59:16 +02:00 by ltms · 1 comment
ltms commented 2026-10-05 06:59:16 +02:00
Owner
Copy Link
Copy Source

Found by an architect during #748 and verified by the lead on 2026-10-05, main = 7f0c4a8.

The contradiction

fleetd/src/test/java/dev/ltms/fleet/PackageCyclesTest.java:82-86 states a guarantee:

Accepts today's known cycle between two top-level packages, and nothing else.
Ignoring both directions removes exactly this pair from cycle detection; every
other dependency -- including any new one added later, between these same two
packages or any other pair -- is still checked.

The implementation, four lines below it:

private static SliceRule ignoreCycle(SliceRule rule, String packageA, String packageB) {
    return rule
            .ignoreDependency(residesIn(packageA), residesIn(packageB))
            .ignoreDependency(residesIn(packageB), residesIn(packageA));
}

private static DescribedPredicate<JavaClass> residesIn(String topLevelPackage) {
    return Predicates.resideInAPackage("dev.ltms.fleet." + topLevelPackage + "..");
}

residesIn matches a whole package, so each ignoreDependency call removes every dependency between the two packages, in that direction. Both calls together remove the pair entirely. There is no edge baseline anywhere in the class.

So the sentence "including any new one added later, between these same two packages ... is still checked" is false. A new import added tomorrow between, say, msg and session in either direction is silently accepted.

What is and is not still protected

To be fair to the test, it is not useless:

  • A cycle involving a pair not in the exception list is still caught.
  • A cycle spanning three or more packages where at least one edge is outside every excepted pair is still caught.

What is lost: any new coupling between the five excepted pairs. Those pairs are auth/mcp, mcp/msg, inject/msg, metrics/msg, and msg/session. Four of the five involve msg, which is also the package an architect flagged in #748 as carrying the most concurrent state and the highest change cost. So the gate is weakest exactly where the design is most fragile.

Why this is worth a ticket rather than a comment fix

Two options, and they are not equivalent:

  1. Correct the comment. Smallest change. Say that the pair is removed from cycle detection entirely and that new coupling between these packages is not detected. Honest, and it stops a future reader trusting a guarantee that does not exist.
  2. Make the implementation match the comment. Replace the package-wide predicates with an exact baseline of allowed dependency edges, so a new edge inside an excepted pair fails and removing an old edge shrinks the baseline. This is what the comment already describes, and it is what the project seems to have intended.

Option 2 is the better end state and option 1 is not a substitute for it — but option 1 should not wait for option 2, because a wrong comment on a quality gate is actively misleading today.

The shape, which is probably not unique to this file

This is a test whose comment claims a stronger guarantee than its assertions deliver. That family has bitten this project repeatedly: a negative-only assertion that passes when its subject leaves the file, a control assertion that looks like an expectation, a barrier whose fastest pass is its most broken state.

Whoever picks this up should look for the same shape elsewhere rather than only fixing this instance — other architecture or policy tests whose javadoc describes a baseline, an exhaustive check, or a "nothing else" guarantee. Report what you find; do not fix beyond the assigned scope.

A cheap first probe: for each architecture-style test, mutate the thing the comment says is protected and confirm the test actually goes red. A survivor means the comment is the only thing holding the rule.

Not measured

  • I have not run the mutation that proves the gap — adding a fresh import between two excepted packages and confirming the test still passes. The claim above is read from the predicate's definition, which is unambiguous, but it is code reading and not a run. Anyone taking option 2 should start by writing that failing case.
  • I have not checked whether the five exceptions are all still real. The javadoc for each cites file-and-line evidence from #131; those line numbers may have moved, and one or more cycles may already be gone. Worth re-measuring before baselining them, since baselining a cycle that no longer exists would cement it.
  • I have not surveyed other tests for the shape described above.

Related

  • #748 — the code-quality standards decision this was found under. One architect proposed exactly option 2 (an exact edge baseline) as an enforcement mechanism.
  • #131 — the ticket the five exceptions cite as their origin, each describing itself as naming its own follow-up step before removal.
Found by an architect during #748 and verified by the lead on 2026-10-05, `main` = `7f0c4a8`. ## The contradiction `fleetd/src/test/java/dev/ltms/fleet/PackageCyclesTest.java:82-86` states a guarantee: > Accepts today's known cycle between two top-level packages, and nothing else. > Ignoring both directions removes exactly this pair from cycle detection; every > other dependency -- **including any new one added later, between these same two > packages** or any other pair -- is still checked. The implementation, four lines below it: ```java private static SliceRule ignoreCycle(SliceRule rule, String packageA, String packageB) { return rule .ignoreDependency(residesIn(packageA), residesIn(packageB)) .ignoreDependency(residesIn(packageB), residesIn(packageA)); } private static DescribedPredicate<JavaClass> residesIn(String topLevelPackage) { return Predicates.resideInAPackage("dev.ltms.fleet." + topLevelPackage + ".."); } ``` `residesIn` matches a **whole package**, so each `ignoreDependency` call removes *every* dependency between the two packages, in that direction. Both calls together remove the pair entirely. There is no edge baseline anywhere in the class. So the sentence "including any new one added later, between these same two packages ... is still checked" is **false**. A new import added tomorrow between, say, `msg` and `session` in either direction is silently accepted. ## What is and is not still protected To be fair to the test, it is not useless: - A cycle involving a pair **not** in the exception list is still caught. - A cycle spanning three or more packages where at least one edge is outside every excepted pair is still caught. What is lost: any new coupling between the five excepted pairs. Those pairs are `auth`/`mcp`, `mcp`/`msg`, `inject`/`msg`, `metrics`/`msg`, and `msg`/`session`. Four of the five involve `msg`, which is also the package an architect flagged in #748 as carrying the most concurrent state and the highest change cost. So the gate is weakest exactly where the design is most fragile. ## Why this is worth a ticket rather than a comment fix Two options, and they are not equivalent: 1. **Correct the comment.** Smallest change. Say that the pair is removed from cycle detection entirely and that new coupling between these packages is not detected. Honest, and it stops a future reader trusting a guarantee that does not exist. 2. **Make the implementation match the comment.** Replace the package-wide predicates with an exact baseline of allowed dependency edges, so a new edge inside an excepted pair fails and removing an old edge shrinks the baseline. This is what the comment already describes, and it is what the project seems to have intended. Option 2 is the better end state and option 1 is not a substitute for it — but option 1 should not wait for option 2, because a wrong comment on a quality gate is actively misleading today. ## The shape, which is probably not unique to this file This is a **test whose comment claims a stronger guarantee than its assertions deliver**. That family has bitten this project repeatedly: a negative-only assertion that passes when its subject leaves the file, a control assertion that looks like an expectation, a barrier whose fastest pass is its most broken state. Whoever picks this up should look for the same shape elsewhere rather than only fixing this instance — other architecture or policy tests whose javadoc describes a baseline, an exhaustive check, or a "nothing else" guarantee. **Report what you find; do not fix beyond the assigned scope.** A cheap first probe: for each architecture-style test, mutate the thing the comment says is protected and confirm the test actually goes red. A survivor means the comment is the only thing holding the rule. ## Not measured - I have **not** run the mutation that proves the gap — adding a fresh import between two excepted packages and confirming the test still passes. The claim above is read from the predicate's definition, which is unambiguous, but it is code reading and not a run. Anyone taking option 2 should start by writing that failing case. - I have not checked whether the five exceptions are all still real. The javadoc for each cites file-and-line evidence from #131; those line numbers may have moved, and one or more cycles may already be gone. Worth re-measuring before baselining them, since baselining a cycle that no longer exists would cement it. - I have not surveyed other tests for the shape described above. ## Related - #748 — the code-quality standards decision this was found under. One architect proposed exactly option 2 (an exact edge baseline) as an enforcement mechanism. - #131 — the ticket the five exceptions cite as their origin, each describing itself as naming its own follow-up step before removal.
ltms commented 2026-10-05 07:50:32 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged locally — be835aa

The package-wide ignoreDependency pairs are gone. BASELINE_EDGES now names the 45 exact origin class -> target class dependencies that cross those five pairs today, and SliceRule.ignoreDependency(String, String) matches only on an exact fully-qualified name.

The worker found a hole in my brief

I briefed this as "make the javadoc true by making the exception exact". That is not sufficient on its own, and the worker caught why:

A brand-new one-directional dependency inside an already-excepted pair does not necessarily create a cycle by itself, because the opposite direction's old edges are all exempted by the baseline — so relying only on beFreeOfCycles() would miss it.

That is correct and it matters. An exact baseline narrows what is ignored, but the thing doing the catching was still beFreeOfCycles(), and a single new edge in one direction is not a cycle. So it added checkBaselineMatchesTodaysEdges(), which compares the live dependency set against the baseline directly and fails on a new edge or a stale one, naming the exact origin, target and package pair. It runs before rule.check(classes) so its message is what a developer sees first.

My acceptance criterion — prove the gate bites — is what forced that discovery. A green suite would have hidden it completely, because the baseline matched reality.

I verified it myself, on a pair the worker did not test

The worker proved the gate with a msg -> session edge. A test that passes on one pair proves one pair, so I mutated a different one. I added a new auth -> mcp edge with both a new origin and a new target — Authz -> PrimaryRegistry, where the baseline holds only CallerResolver -> ConnectionIdentity and its nested $Caller:

private static final Class<?> LEAD_PROBE = dev.ltms.fleet.mcp.PrimaryRegistry.class;

Gated on mvn -o compile first (COMPILE_EXIT=0, so the mutation is real and reachable), then:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.PackageCyclesTest
org.opentest4j.AssertionFailedError:
  new dependency not in the baseline: dev.ltms.fleet.auth.Authz -> dev.ltms.fleet.mcp.PrimaryRegistry (packages auth -> mcp)
[INFO] BUILD FAILURE

A runtime kill, naming the exact edge — not a survivor I would have to explain away. Reverted from a pre-mutation copy and confirmed byte-identical with diff -q.

Stale entries fail the build

The worker chose this over merely reporting, and I agree with its reason: an exact baseline is supposed to mirror reality, so a removed edge left rotting in it would let a cycle-removal land with nobody noticing the pair's exception could shrink or go.

Build, measured not read off the log

rm -rf target/surefire-reports, then mvn -B -Pcontract clean install from fleetd/:

MVN_EXIT=0
xml_files=183 tests=2154 failures=0 errors=0 skipped=0

That is the contract profile — 2154, not the hermetic 2118 — so the Docker-backed AMQP tests pass with this change too.

What I did not check

  • The 45 baseline edges are the worker's measurement from a scratch ArchUnit scan. I did not independently re-derive the set. If it over-lists, the test is merely wider than needed; if it under-lists, the build would have failed, and it did not.
  • The five cycles themselves are untouched — still tracked separately under #131.
  • No sweep for other gates whose javadoc over-claims what their predicate delivers. That shape is worth a hunter pass and is not done.

This also delivers the cycle half of rule 5 from the #748 decision.

## Fixed and merged locally — `be835aa` The package-wide `ignoreDependency` pairs are gone. `BASELINE_EDGES` now names the **45 exact `origin class -> target class`** dependencies that cross those five pairs today, and `SliceRule.ignoreDependency(String, String)` matches only on an exact fully-qualified name. ### The worker found a hole in my brief I briefed this as "make the javadoc true by making the exception exact". That is not sufficient on its own, and the worker caught why: > A brand-new *one-directional* dependency inside an already-excepted pair does not necessarily create a *cycle* by itself, because the opposite direction's old edges are all exempted by the baseline — so relying only on `beFreeOfCycles()` would miss it. That is correct and it matters. An exact baseline narrows what is ignored, but the thing doing the catching was still `beFreeOfCycles()`, and a single new edge in one direction is not a cycle. So it added `checkBaselineMatchesTodaysEdges()`, which compares the live dependency set against the baseline directly and fails on a new edge **or** a stale one, naming the exact origin, target and package pair. It runs before `rule.check(classes)` so its message is what a developer sees first. My acceptance criterion — *prove the gate bites* — is what forced that discovery. A green suite would have hidden it completely, because the baseline matched reality. ### I verified it myself, on a pair the worker did not test The worker proved the gate with a `msg -> session` edge. A test that passes on one pair proves one pair, so I mutated a different one. I added a new `auth -> mcp` edge with **both** a new origin and a new target — `Authz -> PrimaryRegistry`, where the baseline holds only `CallerResolver -> ConnectionIdentity` and its nested `$Caller`: ```java private static final Class<?> LEAD_PROBE = dev.ltms.fleet.mcp.PrimaryRegistry.class; ``` Gated on `mvn -o compile` first (`COMPILE_EXIT=0`, so the mutation is real and reachable), then: ``` [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 -- in dev.ltms.fleet.PackageCyclesTest org.opentest4j.AssertionFailedError: new dependency not in the baseline: dev.ltms.fleet.auth.Authz -> dev.ltms.fleet.mcp.PrimaryRegistry (packages auth -> mcp) [INFO] BUILD FAILURE ``` A **runtime kill**, naming the exact edge — not a survivor I would have to explain away. Reverted from a pre-mutation copy and confirmed byte-identical with `diff -q`. ### Stale entries fail the build The worker chose this over merely reporting, and I agree with its reason: an exact baseline is supposed to mirror reality, so a removed edge left rotting in it would let a cycle-removal land with nobody noticing the pair's exception could shrink or go. ### Build, measured not read off the log `rm -rf target/surefire-reports`, then `mvn -B -Pcontract clean install` from `fleetd/`: ``` MVN_EXIT=0 xml_files=183 tests=2154 failures=0 errors=0 skipped=0 ``` That is the **contract** profile — 2154, not the hermetic 2118 — so the Docker-backed AMQP tests pass with this change too. ### What I did not check - The 45 baseline edges are the worker's measurement from a scratch ArchUnit scan. I did not independently re-derive the set. If it over-lists, the test is merely wider than needed; if it under-lists, the build would have failed, and it did not. - The five cycles themselves are untouched — still tracked separately under #131. - No sweep for other gates whose javadoc over-claims what their predicate delivers. That shape is worth a `hunter` pass and is not done. This also delivers the cycle half of rule 5 from the #748 decision.
ltms closed this issue 2026-10-05 07:50:35 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#749
Delete Branch "%!s()"

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?