From e8ab933cbcf0c020942826b773a4a09b6dab27f5 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 5 Oct 2026 07:42:55 +0200 Subject: [PATCH] fleetd #748: fix dead test-class references and orphaned javadoc blocks Drop 2 dead *WiringTest names from 3 comment sites (renamed to *AssemblyTest), rewriting each sentence to state the code's guarantee instead of naming a test class. Reattach 5 javadoc blocks that were orphaned behind a second /** block to the member they actually describe, trimming history/evidence text down to the current contract per the project's comment rule. Comment-only; no production logic changed. --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 25 +++------ .../dev/ltms/fleet/config/FleetConfig.java | 14 ++--- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 8 +-- .../fleet/member/CompositePeerLauncher.java | 42 +++++++------- .../ltms/fleet/member/EnvAllowListScrub.java | 2 +- .../dev/ltms/fleet/session/GitWorktrees.java | 56 ++++++++----------- 6 files changed, 63 insertions(+), 84 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index cb6028a4..291ff750 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -336,22 +336,6 @@ public final class Fleetd { }, reasonByCredential::get); } - /** - * fleetd #415 (review follow-up): package-private factory for the CB-578 stage A {@code - * exhaustedPattern} startup coverage line, paired explicitly with {@link - * CompletionResolver.UnsetMeaning#OFF} — {@code exhaustedPattern} has no fallback, so a - * profile with none configured really does have the classification off. - * - *

Extracted out of {@code main} for the same reason {@link #capacitySource} and {@link - * #worktreeBranchLookup} were: {@code coverage()}'s own tests ({@code CompletionResolverTest}) - * prove it words {@code OFF} and {@link CompletionResolver.UnsetMeaning#BUILT_IN_DEFAULT} - * correctly when a test supplies the meaning itself — they cannot prove {@code main} pairs the - * right meaning with the right key, which is the actual fleetd #415 defect. Measured: - * swapping the {@code UnsetMeaning} arguments between this method and {@link - * #errorPatternCoverageLine} — recreating #415's defect with the two keys exchanged — compiled - * with 0 errors and left all 1506 existing tests green before {@code - * FleetdPatternCoverageLineTest} was added to catch exactly that swap. - */ /** * fleetd #446 follow-up: the criterion-2 WARNING text — "name the fix, not just the fact" — * for a profile whose {@code model:} is configured. Extracted out of the {@code @@ -386,6 +370,11 @@ public final class Fleetd { + "s quarantine above is the only thing keeping new spawns off it for now"; } + /** + * Package-private factory for the {@code exhaustedPattern} startup coverage line, paired + * explicitly with {@link CompletionResolver.UnsetMeaning#OFF} — {@code exhaustedPattern} has + * no fallback, so a profile with none configured really does have the classification off. + */ static String exhaustedPatternCoverageLine(Set allProfiles, Set configuredProfiles) { return CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, allProfiles, configuredProfiles); @@ -734,8 +723,8 @@ public final class Fleetd { * {@code CompletionResolver} constructor call — provably untested wiring, the whole reason * fleetd #248 exists: dropping that one argument (passing {@code _ -> null} instead) compiled * clean and left every test green. Extracted here, {@code main} now calls this factory instead - * of building the lambda inline, and a source assertion on that call site - * ({@code FleetdCompletionResolverWiringTest}) proves the argument is still actually passed. + * of building the lambda inline, so the argument reaching the {@code CompletionResolver} + * constructor is a named, directly testable call rather than an inline lambda. * *

Takes the roster as a plain {@link Supplier} — not a {@link SessionManager} — so this is * directly testable with a hand-built session list; no real {@code SessionManager} (launcher, diff --git a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 0a6ce5ac..6195ab48 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/fleetd/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -2095,13 +2095,6 @@ public record FleetConfig( } } - /** - * The top-level keys in {@code yaml} that this build does not understand, sorted. Package-private - * so the guardrail is asserted directly rather than through a log appender. - * - * @return empty when everything is known, or when {@code yaml} is not a mapping at all (a - * malformed file is {@code readValue}'s error to report, not this method's) - */ /** * Top-level keys renamed by the member taxonomy, mapped old → new. * @@ -2636,6 +2629,13 @@ public record FleetConfig( } } + /** + * The top-level keys in {@code yaml} that this build does not understand, sorted. Package-private + * so the guardrail is asserted directly rather than through a log appender. + * + * @return empty when everything is known, or when {@code yaml} is not a mapping at all (a + * malformed file is {@code readValue}'s error to report, not this method's) + */ static List unknownTopLevelKeys(String yaml) { Map raw; try { diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java index 9cb10993..34f7a7c0 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -349,11 +349,9 @@ public final class FleetMcp { * overload instead of failing to compile, and every existing test — none of which exercises * {@code Fleetd.main} itself — stays green while the live daemon quietly answers * {@code NOT_CONFIGURED} to {@code fleet_handover} forever. Collapsing every overload into one - * required-everything constructor turns that mistake into a compile error instead: this - * project's own antidote for a defaulted parameter surviving as an untested decision (see - * {@code FleetdCompletionResolverWiringTest} / {@code FleetdLeadRolloverWiringTest}'s own - * javadoc for the same lesson applied to a different seam). A caller that genuinely wants a - * feature off must now say so explicitly at the call site — {@code null}, + * required-everything constructor turns that mistake into a compile error instead, the same + * defaulted-parameter guard this codebase applies to other required wiring. A caller that + * genuinely wants a feature off must now say so explicitly at the call site — {@code null}, * {@link OutageSource#none()}, {@link LeadSeatSource#none()}, {@code List.of()} are all still * perfectly fine values, just never an implicit default reached by omission. * diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java index 3a33f433..8e0af9a3 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/CompositePeerLauncher.java @@ -445,27 +445,6 @@ public final class CompositePeerLauncher implements PeerLauncher { + " distinct candidate(s): " + String.join(", ", unreachable)); } - /** - * Refuse an explicit-profile spawn when the profile is at its {@code maxLoad} cap. - * - *

maxLoad is a documented, unconditional capacity limit (see {@code FleetConfig.Profile#maxLoad}), - * and the charter makes explicit-profile spawns the normal path — so enforcing it only in placement - * ({@code PlacementPolicyUtil}, package-private, hence not linked) would leave the cap dead config - * on every call that names a profile. Same rule as placement: {@code live >= cap} is at capacity. - * - *

Deliberately no fallback to another profile: the caller named {@code profile} for a cost/model - * reason, and silently re-routing a paid-tier (subscription) request elsewhere is worse than - * refusing it. A caller that wants placement should omit the profile and let the policy pick. - * - *

Known TOCTOU limitation — documented, not fixed. {@link #liveCount} is read outside any lock and - * {@code SessionManager} registers a session only after {@code launcher.spawn} returns, so two - * genuinely concurrent spawns can both pass this check. The race already exists on the placement - * path. Closing it needs slot reservation in the registry; serializing spawn here would block on - * the readiness gate and is a far worse trade. - * - * @param profile the profile the caller explicitly named - * @throws PlacementException when the profile is at capacity - */ /** * Refuse an explicit-profile spawn whose credential is quarantined (CB-578 stage B): a prior * {@code BACKEND_EXHAUSTED} classification on this profile, or on another profile sharing its @@ -527,6 +506,27 @@ public final class CompositePeerLauncher implements PeerLauncher { .collect(Collectors.toSet()); } + /** + * Refuse an explicit-profile spawn when the profile is at its {@code maxLoad} cap. + * + *

maxLoad is a documented, unconditional capacity limit (see {@code FleetConfig.Profile#maxLoad}), + * and the charter makes explicit-profile spawns the normal path — so enforcing it only in placement + * ({@code PlacementPolicyUtil}, package-private, hence not linked) would leave the cap dead config + * on every call that names a profile. Same rule as placement: {@code live >= cap} is at capacity. + * + *

No fallback to another profile: the caller named {@code profile} for a cost/model + * reason, and silently re-routing a paid-tier (subscription) request elsewhere is worse than + * refusing it. A caller that wants placement should omit the profile and let the policy pick. + * + *

Known TOCTOU limitation — documented, not fixed. {@link #liveCount} is read outside any lock and + * {@code SessionManager} registers a session only after {@code launcher.spawn} returns, so two + * genuinely concurrent spawns can both pass this check. The race already exists on the placement + * path. Closing it needs slot reservation in the registry; serializing spawn here would block on + * the readiness gate and is a far worse trade. + * + * @param profile the profile the caller explicitly named + * @throws PlacementException when the profile is at capacity + */ private void enforceMaxLoad(String profile) { // Absent config, or a config whose maxLoad normalized to null (ABSENT ⇒ unlimited at load), // means no cap — never cap what wasn't configured. Note "non-positive ⇒ unlimited" was true diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java index b9942a4d..8b219c42 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -474,7 +474,6 @@ public final class EnvAllowListScrub { } } - /** Best-effort recursive delete; failures are swallowed — JVM-exit cleanup is the backstop. */ /** * Remove generated directories left behind by an earlier daemon process. * @@ -511,6 +510,7 @@ public final class EnvAllowListScrub { } } + /** Best-effort recursive delete; failures are swallowed — JVM-exit cleanup is the backstop. */ static void deleteRecursively(Path dir) { if (dir == null || !Files.exists(dir)) { return; diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index 71898d23..9fff2861 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -442,38 +442,6 @@ public final class GitWorktrees implements Worktrees { ENVIRONMENT_CREDENTIAL_HELPER); } - /** - * {@link #configureEnvironmentCredentialHelper} only ever fires for an HTTPS origin — Git never - * consults a {@code credential.helper} for an SSH transport. This repo's own origin is - * {@code ssh://git@git.ltms.dev:2224/fleet/fleetd.git}, so a member sitting on that origin never - * reaches the helper and the repo-scoped {@code WORKER_GITEA_TOKEN} is simply not used. - * - *

An earlier version of this javadoc justified the rewrite by claiming a member cannot - * push once {@code memberCredentials.policy: allow-list} blocks {@code SSH_AUTH_SOCK}, because - * "there is no private key file on this host, only an ssh-agent socket". That premise is false - * (fleetd #184): {@code ssh -G} resolves a readable, passphrase-free {@code IdentityFile} outside - * {@code ~/.ssh}, and a member — same OS user — pushes over SSH with the socket blanked. The - * rewrite is still worth having, but for the reason below rather than that one: it routes the - * member through its own scoped token instead of the operator's ssh identity, which is what makes - * a member's pushes attributable and revocable. - * - *

The fix is a worktree-scoped URL rewrite: {@code url..insteadOf - * }, set with {@code --worktree} so it lands only in - * {@code /.git/worktrees//config.worktree} (enabled by - * {@code extensions.worktreeConfig}, already turned on above) and never touches the shared - * repo-level config the primary checkout also reads. {@code insteadOf} — not - * {@code pushInsteadOf} — because a member may also need to fetch or rebase, and both should go - * through the member's own token for the same reason. - * - *

The host (and, for the rewrite's SSH-side match, the port) come from parsing the origin - * itself — never a hardcoded forge host, which is exactly what #177 removed. An origin that is - * already {@code https://} is left alone; the credential helper already covers it. An origin - * that is neither {@code ssh://} nor {@code https://} — including the scp-like shorthand - * ({@code git@host:path}, no scheme) — is left untouched deliberately: that shorthand's - * {@code host:path} split is defined by the user's ssh_config aliases, not by URI syntax, so - * guessing at it risks rewriting to the wrong place. A repo provisioned from that form keeps - * today's (broken, if the policy blocks the agent) SSH-only behaviour rather than a wrong rewrite. - */ /** * Blank the user-info of a remote URL before it reaches a log. A remote URL is not obviously a * credential channel, which is exactly why one has leaked here three times ({@code git remote -v} @@ -485,6 +453,30 @@ public final class GitWorktrees implements Worktrees { return url == null ? null : url.replaceAll("://[^@/]*@", "://@"); } + /** + * {@link #configureEnvironmentCredentialHelper} only fires for an HTTPS origin — Git never + * consults a {@code credential.helper} for an SSH transport. This repo's own origin is + * {@code ssh://git@git.ltms.dev:2224/fleet/fleetd.git}, so a member on that origin never + * reaches the helper, and the repo-scoped token goes unused without a separate rewrite. + * + *

This method routes the member through its own scoped token instead of the operator's ssh + * identity, which is what makes a member's pushes attributable and revocable. + * + *

The fix is a worktree-scoped URL rewrite: {@code url..insteadOf + * }, set with {@code --worktree} so it lands only in + * {@code /.git/worktrees//config.worktree} and never touches the shared + * repo-level config the primary checkout also reads. {@code insteadOf} — not + * {@code pushInsteadOf} — because a member may also need to fetch or rebase through its own + * token. + * + *

The host (and, for the rewrite's SSH-side match, the port) come from parsing the origin + * itself, never a hardcoded forge host. An origin already {@code https://} is left alone; the + * credential helper already covers it. An origin that is neither {@code ssh://} nor + * {@code https://} — including the scp-like shorthand ({@code git@host:path}, no scheme) — is + * left untouched: that shorthand's {@code host:path} split is defined by the user's ssh_config + * aliases, not by URI syntax, so guessing at it risks rewriting to the wrong place, and that + * origin keeps SSH-only push behaviour instead. + */ private void configureHttpsUrlRewriteForSshOrigin(String repoRoot, String worktreePath) { if (exitCode("git", "-C", repoRoot, "config", "--get", "remote.origin.url") != 0) { return;