From 8557289dc0082a8779734809b32830facb5452eb Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sun, 6 Sep 2026 19:54:36 +0700 Subject: [PATCH] fleetd #360 review round 2: extend SystemdUnitSafetyTest to herdr-inner.sh herdr.service's ExecStart only names deploy/herdr-inner.sh, so that script was the only place herdr's login-shell and pty-size properties lived, and nothing was reading it -- the same silent-at-startup shape the ticket was about, one file further down the chain. A mutation dropping both the login shell and the stty sizing left SystemdUnitSafetyTest green. Add herdrInnerScriptUsesALoginShell and herdrInnerScriptSetsANonZeroPtySize, extend the vacuity guard to cover herdr-inner.sh too, and tighten execStartUsesALoginShell's check from a bare contains("-lc") substring match to the same login-shell-invocation regex the new checks use. --- .../fleet/deploy/SystemdUnitSafetyTest.java | 98 ++++++++++++++++--- 1 file changed, 84 insertions(+), 14 deletions(-) diff --git a/fleetd/src/test/java/dev/ltms/fleet/deploy/SystemdUnitSafetyTest.java b/fleetd/src/test/java/dev/ltms/fleet/deploy/SystemdUnitSafetyTest.java index db916c2..f27f3cf 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/deploy/SystemdUnitSafetyTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/deploy/SystemdUnitSafetyTest.java @@ -43,9 +43,22 @@ import static org.junit.jupiter.api.Assertions.assertTrue; * member that cannot open a pull request. * * - *

None of these three failures makes the unit fail to start or makes {@code /healthz} report - * unhealthy, so nothing short of reading the unit file catches a regression. This test is that - * read. + *

Review round 2 on fleetd #360 found the same shape one file further down the chain: + * {@code herdr.service}'s {@code ExecStart} only names {@code deploy/herdr-inner.sh}, so that + * script is the ONLY place two more of these properties live, and nothing was reading it. + * + *

+ * + *

None of these five failures makes the unit (or the script) fail to start, or makes + * {@code /healthz} report unhealthy, so nothing short of reading the files catches a regression. + * This test is that read. * *

It checks source text, not behaviour -- it cannot start systemd or fork an mount * namespace in a build sandbox. It parses the same two files an operator would install and fails @@ -57,14 +70,38 @@ class SystemdUnitSafetyTest { /** Tests run with the module directory (fleetd/) as cwd; deploy/ is the repo-root sibling. */ private static final Path FLEETD_SERVICE = Path.of("../deploy/fleetd.service"); private static final Path HERDR_SERVICE = Path.of("../deploy/herdr.service"); + private static final Path HERDR_INNER_SCRIPT = Path.of("../deploy/herdr-inner.sh"); private static final List NAMESPACING_DIRECTIVES = List.of( "ProtectSystem", "ProtectHome", "ProtectKernelTunables", "ProtectControlGroups"); + /** + * A shell invoked with a login flag, e.g. {@code zsh -lc '...'} or {@code /bin/sh -l}. The + * property under test is the {@code -l}, not the specific shell or the rest of its flags, so + * this matches a token ending in "sh" followed by a flag cluster containing "l" -- tighter + * than a bare {@code contains("-lc")}, which a "-lc" anywhere in the line would also satisfy. + */ + private static final Pattern LOGIN_SHELL_INVOCATION = + Pattern.compile("(?:^|\\s)\\S*sh\\s+-[A-Za-z]*l[A-Za-z]*\\b"); + + private static final Pattern POSITIVE_ROWS = Pattern.compile("\\brows\\s+([1-9]\\d*)\\b"); + private static final Pattern POSITIVE_COLS = Pattern.compile("\\bcols\\s+([1-9]\\d*)\\b"); + static Stream bothUnits() { return Stream.of(FLEETD_SERVICE, HERDR_SERVICE); } + private static boolean invokesALoginShell(List activeLines) { + return activeLines.stream().anyMatch(l -> LOGIN_SHELL_INVOCATION.matcher(l).find()); + } + + /** An active {@code stty} line naming both a positive row count and a positive column count. */ + private static boolean setsANonZeroPtySize(List activeLines) { + return activeLines.stream() + .filter(l -> l.startsWith("stty")) + .anyMatch(l -> POSITIVE_ROWS.matcher(l).find() && POSITIVE_COLS.matcher(l).find()); + } + /** Lines that are actually in force: comments and blank lines don't count. */ private static List activeLines(Path unit) throws Exception { List active = new ArrayList<>(); @@ -125,23 +162,51 @@ class SystemdUnitSafetyTest { + execStartLines.size() + ": " + execStartLines); String execStart = execStartLines.get(0); - assertTrue(execStart.contains("-lc"), - FLEETD_SERVICE + "'s ExecStart (" + execStart + ") does not run a login shell (\"-lc\"). " - + "Every secret fleetd needs (AI_GATEWAY_TOKEN, WORKER_GITEA_TOKEN, LAVINMQ_URI, " - + "COORD_AMQP_URI) lives in a file only the login shell sources; systemd runs no " - + "login shell on its own. Running java directly boots fine with every credential " - + "empty, and the failure surfaces hours later as a member that cannot open a " - + "pull request (fleetd #360)."); + assertTrue(LOGIN_SHELL_INVOCATION.matcher(execStart).find(), + FLEETD_SERVICE + "'s ExecStart (" + execStart + ") does not run a login shell (a shell " + + "invoked with a \"-l\" flag, e.g. \"zsh -lc\"). Every secret fleetd needs " + + "(AI_GATEWAY_TOKEN, WORKER_GITEA_TOKEN, LAVINMQ_URI, COORD_AMQP_URI) lives in a " + + "file only the login shell sources; systemd runs no login shell on its own. " + + "Running java directly boots fine with every credential empty, and the failure " + + "surfaces hours later as a member that cannot open a pull request (fleetd #360)."); + } + + @Test + @DisplayName("[SOURCE TEXT] herdr-inner.sh execs a login shell -- otherwise herdr and every member it spawns start with empty credentials") + void herdrInnerScriptUsesALoginShell() throws Exception { + List active = activeLines(HERDR_INNER_SCRIPT); + + assertTrue(invokesALoginShell(active), + HERDR_INNER_SCRIPT + " does not exec a login shell (a shell invoked with a \"-l\" flag, " + + "e.g. \"zsh -lc\"). herdr.service's ExecStart only names this script, so this " + + "is the ONLY place herdr's login-shell property lives. Without it, herdr -- and " + + "every member pane it spawns as a child of herdr -- starts with none of the " + + "secrets that only a login shell sources (this host: ~/.fleet/secrets.sh via " + + "~/.zprofile), and the failure surfaces hours later as a member with no " + + "credentials at all, not just fleetd (fleetd #360)."); + } + + @Test + @DisplayName("[SOURCE TEXT] herdr-inner.sh sets a non-zero pty size before starting herdr -- otherwise every pane spawn fails with \"ghostty error -2\"") + void herdrInnerScriptSetsANonZeroPtySize() throws Exception { + List active = activeLines(HERDR_INNER_SCRIPT); + + assertTrue(setsANonZeroPtySize(active), + HERDR_INNER_SCRIPT + " does not run \"stty rows N cols M\" with both N and M positive " + + "before starting herdr. Without a real pty size, herdr reports a 0x0 window and " + + "every pane spawn fails with \"ghostty error -2\" -- which surfaces as a fleetd " + + "spawn failure, not a herdr one, and gives no hint that the actual cause is this " + + "script (fleetd #360)."); } /** * The denominator guard, same shape as {@code McpContractDocTest}'s: a check that scans for a - * forbidden pattern passes trivially if it is handed nothing to scan. Pin that both files exist, - * are non-trivial, and that the DO-NOT block's commented mentions are still there -- so the - * "comment survives, directive doesn't" distinction above is actually being exercised. + * forbidden pattern passes trivially if it is handed nothing to scan. Pin that all three files + * exist, are non-trivial, and that the DO-NOT block's commented mentions are still there -- so + * the "comment survives, directive doesn't" distinction above is actually being exercised. */ @Test - @DisplayName("[SOURCE TEXT] the namespacing check is not vacuous -- both files exist and the DO-NOT block still mentions every forbidden directive in a comment") + @DisplayName("[SOURCE TEXT] the namespacing check is not vacuous -- all three files exist and the DO-NOT block still mentions every forbidden directive in a comment") void theCheckActuallyHasSomethingToCheck() throws Exception { assertTrue(Files.size(FLEETD_SERVICE) > 200, FLEETD_SERVICE + " is missing or unexpectedly small -- the checks above would pass " @@ -149,6 +214,11 @@ class SystemdUnitSafetyTest { assertTrue(Files.size(HERDR_SERVICE) > 200, HERDR_SERVICE + " is missing or unexpectedly small -- the checks above would pass " + "vacuously against an empty or absent file"); + assertTrue(Files.size(HERDR_INNER_SCRIPT) > 50, + HERDR_INNER_SCRIPT + " is missing or unexpectedly small -- the login-shell and " + + "pty-size checks above would either pass vacuously or fail with an opaque " + + "\"file not found\" instead of naming the actual consequence (empty " + + "credentials / \"ghostty error -2\") against an empty or absent file"); String fleetdService = Files.readString(FLEETD_SERVICE); for (String directive : NAMESPACING_DIRECTIVES) {