Add the local MCP gateway for online pipeline control - #18

distribution/cli-bundle/pom.xml (1)

47-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add CI coverage for mcp-jar-fallback.

The release job builds and packages only -Pnative and tests only libexec/tapstate-mcp. Build the fallback archive and run the CLI with libexec/tapstate-mcp.jar under Java 21.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@distribution/cli-bundle/pom.xml` around lines 47 - 72, Add CI coverage for
the mcp-jar-fallback Maven profile: update the release job to build the fallback
archive with -Pmcp-jar-fallback and execute the CLI using
libexec/tapstate-mcp.jar under Java 21, alongside the existing native build and
libexec/tapstate-mcp test.

app/src/test/java/io/tapstate/app/PipelineLogAppenderTest.java (1)

94-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse withAppender by adding a redactor overload.

This test repeats the attach, try/finally, detach, and stop sequence that withAppender already owns. An overload that accepts a SecretRedactor removes the duplicated lifecycle code and keeps the cleanup rules in one place.

♻️ Proposed refactor

-    private void withAppender(RingBufferLogSink sink, String loggerName, java.util.function.Consumer<Logger> body) {
+    private void withAppender(RingBufferLogSink sink, String loggerName, java.util.function.Consumer<Logger> body) {
+        withAppender(sink, new SecretRedactor(), loggerName, body);
+    }
+
+    private void withAppender(RingBufferLogSink sink, SecretRedactor redactor, String loggerName,
+            java.util.function.Consumer<Logger> body) {
         Logger logger = (Logger) LoggerFactory.getLogger(loggerName);
-        PipelineLogAppender appender = new PipelineLogAppender(sink, new SecretRedactor());
+        PipelineLogAppender appender = new PipelineLogAppender(sink, redactor);

     `@Test`
     void redactsRegisteredSecretsBeforeWritingToTheSink() {
         RingBufferLogSink sink = new RingBufferLogSink(8, 8);
         SecretRedactor redactor = new SecretRedactor();
-        redactor.replace("source-orders", java.util.List.of("sentinel-password"));
-        Logger logger = (Logger) LoggerFactory.getLogger("test.logs.redacted");
-        PipelineLogAppender appender = new PipelineLogAppender(sink, redactor);
-        appender.setContext(logger.getLoggerContext());
-        appender.start();
-        logger.addAppender(appender);
-        try {
-            MDC.put(MDC_KEY, "orders");
-            logger.warn("connector rejected {}", "sentinel-password");
-        } finally {
-            MDC.remove(MDC_KEY);
-            logger.detachAppender(appender);
-            appender.stop();
-        }
+        redactor.replace("source-orders", List.of("sentinel-password"));
+        withAppender(sink, redactor, "test.logs.redacted", logger -> {
+            MDC.put(MDC_KEY, "orders");
+            logger.warn("connector rejected {}", "sentinel-password");
+        });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/test/java/io/tapstate/app/PipelineLogAppenderTest.java` around lines
94 - 111, Refactor redactsRegisteredSecretsBeforeWritingToTheSink to use the
existing withAppender helper instead of manually managing appender attachment
and cleanup. Add a withAppender overload that accepts a SecretRedactor,
constructs PipelineLogAppender with it, and preserves the helper’s existing
lifecycle behavior.

control/control-core/src/test/java/io/tapstate/control/core/TokenAdminServiceTest.java (1)

58-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the documented idempotent revocation.

TokenAdminService.revoke documents revocation as idempotent. No test revokes an unknown token id or revokes the same id twice. Both paths write an audit record and change no state, so the contract deserves a pinned case.

💚 Proposed additional test

`@Test`
void revokingAnUnknownOrAlreadyRevokedTokenStaysIdempotent() {
    FakeTokenStore tokens = new FakeTokenStore();
    List<AuditRecord> audit = new ArrayList<>();
    TokenAdminService service = service(tokens, audit::add);
    service.create("alice", Scope.READ);
    audit.clear();

    service.revoke("alice", "tok-1");
    service.revoke("alice", "tok-1");
    service.revoke("alice", "no-such-token");

    assertThat(service.list()).containsExactly(new TokenInfo("tok-1", Scope.READ, true, NOW));
    assertThat(audit).hasSize(3);
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/control-core/src/test/java/io/tapstate/control/core/TokenAdminServiceTest.java`
around lines 58 - 77, Add a test alongside
listIsSecretFreeAndRevokeIsAuditedAndImmediatelyAuthoritative that revokes the
same token twice and then an unknown token ID, verifying the token remains
revoked, no additional state changes occur, and three audit records are written.

control/control-client/src/test/java/io/tapstate/control/client/HttpControlClientTest.java (1)

85-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a case for a non-2xx status with a non-JSON body.

malformedJsonBodyIsReturnedAsUnreachable covers status 200 only. A gateway answers 502 or 504 with an HTML body, and the current decode returns Unreachable for it. Add a test that asserts the status survives as Rejected. This pins the behaviour described in the comment on HttpControlClient.decode.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/control-client/src/test/java/io/tapstate/control/client/HttpControlClientTest.java`
around lines 85 - 107, Add a test alongside
malformedJsonBodyIsReturnedAsUnreachable in HttpControlClientTest that returns a
non-2xx status such as 502 or 504 with a non-JSON body, then assert
HttpControlClient.get produces ControlResponse.Rejected preserving that status
rather than ControlResponse.Unreachable. Keep the existing generic rejection
fields consistent with rejectionWithoutAStableCodeUsesTheGenericControlContract.

control/control-client/src/main/java/io/tapstate/control/client/HttpControlClient.java (1)

162-169: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Shut down the HttpClient before its executor.

The project targets Java 21, where HttpClient.shutdownNow() is available. Shutting down executor does not terminate the HttpClient selector manager. Call client.shutdownNow() before executor.shutdownNow().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/control-client/src/main/java/io/tapstate/control/client/HttpControlClient.java`
around lines 162 - 169, Update HttpControlClient.close() to call
client.shutdownNow() after cancelling in-flight futures and before
executor.shutdownNow(). Preserve the existing compare-and-set guard and
cancellation behavior.

e2e/src/test/java/io/tapstate/e2e/McpOnlineControlIT.java (2)

109-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This close is redundant.

The try-with-resources at line 73 closes input, which wraps process.getOutputStream(). Resources close before the finally block runs, so this call always acts on a closed stream.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/src/test/java/io/tapstate/e2e/McpOnlineControlIT.java` at line 109,
Remove the redundant process.getOutputStream().close() call from the finally
block in McpOnlineControlIT, since the try-with-resources input already closes
that stream before finally executes.

---

108-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Surface the child stderr when the process fails.

This test launches a separate JVM. If that JVM fails to start or exits early, receive blocks for 10 seconds and then fails with a bare timeout. The reason sits in the stderr file at line 71, but no assertion message includes it.

Also, line 117 asserts the exit value after a possible destroyForcibly. A forced kill produces a non-zero exit value and masks the original cause.

Attach the stderr content to the failure output.

💚 Proposed change

         } finally {
             process.getOutputStream().close();
             if (!process.waitFor(5, TimeUnit.SECONDS)) {
                 process.destroyForcibly();
                 assertThat(process.waitFor(5, TimeUnit.SECONDS)).isTrue();
             }
             server.stop(0);
         }
 
-        assertThat(process.exitValue()).isZero();
+        assertThat(process.exitValue())
+                .as("MCP process stderr:%n%s", Files.readString(stderr))
+                .isZero();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/src/test/java/io/tapstate/e2e/McpOnlineControlIT.java` around lines 108 -
124, Update the child-process lifecycle assertions around process and stderr so
failures include the contents of the stderr file. Capture whether
process.waitFor completes before forcibly terminating it, and report a failure
with stderr when it times out instead of asserting process.exitValue() after a
forced kill. Preserve the zero-exit assertion only for processes that terminate
normally.

control/mcp-server/src/main/resources/application.properties (1)

1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate settings are fine, but keep one source of truth in mind.

spring.main.web-application-type and spring.main.banner-mode repeat what TapstateMcpApplication.start sets programmatically. The programmatic values win. Keep both only if the properties file must also serve direct java -jar launches without the start helper. The e2e test launches the boot JAR through main, which calls start, so the properties are redundant today.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@control/mcp-server/src/main/resources/application.properties` around lines 1
- 2, Remove the redundant spring.main.web-application-type and
spring.main.banner-mode entries from application.properties, since
TapstateMcpApplication.start already sets them for the current launch paths.

control/mcp-server/src/test/java/io/tapstate/mcp/McpOperationExecutorTest.java (2)

58-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the imported Operation type.

Operation is imported at line 7. The fully qualified io.tapstate.control.core.Operation in the declaration at line 58 and the loop at line 77 adds no information.

♻️ Proposed change

-            List<Map.Entry<io.tapstate.control.core.Operation, Map<String, Object>>> calls = List.of(
+            List<Map.Entry<Operation, Map<String, Object>>> calls = List.of(

-            for (Map.Entry<io.tapstate.control.core.Operation, Map<String, Object>> call : calls) {
+            for (Map.Entry<Operation, Map<String, Object>> call : calls) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/mcp-server/src/test/java/io/tapstate/mcp/McpOperationExecutorTest.java`
around lines 58 - 77, Replace the fully qualified
io.tapstate.control.core.Operation references in the calls declaration and
enhanced for-loop within McpOperationExecutorTest with the already imported
Operation type, leaving the surrounding test logic unchanged.

---

55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

State the limit clamp explicitly.

The request sets limit to 999. The path assertion at line 90 expects limit=200. That difference encodes a clamp contract, but the test does not say so. A reader cannot tell whether 200 is a clamp or a fixed default that discards the caller value.

Add a short comment, or add a second case with a limit below the bound to prove the caller value passes through.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/mcp-server/src/test/java/io/tapstate/mcp/McpOperationExecutorTest.java`
around lines 55 - 56, Clarify the limit-clamping contract in the test around the
logs map and its path assertion: document that the requested limit of 999 is
capped at 200, or add a below-bound case proving caller values are preserved.
Keep the existing assertions and behavior unchanged.

control/mcp-server/src/test/java/io/tapstate/mcp/McpOptionsTest.java (1)

42-51: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Cover the --token= form.

McpOptions.parse rejects both --token and --token=<value> at line 28 of McpOptions.java. This test only covers the space form. The startsWith("--token=") branch guards a credential-leak rule, so add a case for it.

💚 Proposed addition

         assertThatThrownBy(() -> McpOptions.parse(
                 new String[] {"--token", "leak"}, Map.of("TAPSTATE_TOKEN", "token")))
                 .isInstanceOf(IllegalArgumentException.class)
                 .hasMessageContaining("--token");
+        assertThatThrownBy(() -> McpOptions.parse(
+                new String[] {"--token=leak"}, Map.of("TAPSTATE_TOKEN", "token")))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("--token");
         assertThatThrownBy(() -> McpOptions.parse(new String[0], Map.of()))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@control/mcp-server/src/test/java/io/tapstate/mcp/McpOptionsTest.java` around
lines 42 - 51, Extend tokenFlagAndMissingEnvironmentTokenAreRejected to call
McpOptions.parse with a "--token=leak" argument and assert
IllegalArgumentException with a message containing "--token", covering the
startsWith("--token=") rejection branch while preserving the existing
assertions.

control/mcp-server/src/test/java/io/tapstate/mcp/McpEnvironmentTest.java (1)

22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The test name promises null coverage that the test does not provide.

Map.of rejects null values, so this case only covers an empty string value. Rename the test, or build the map with HashMap and add a real null entry. If McpEnvironment must tolerate null values from System.getenv-like maps, the second option adds real coverage.

♻️ Proposed rename

     `@Test`
-    void emptyAndNullEnvironmentValuesAreRetainedWithoutControlVariables() {
+    void emptyEnvironmentValuesAreRetainedWithoutControlVariables() {
         McpEnvironment environment = new McpEnvironment(Map.of(
                 "EMPTY", "", "OTHER", "value"));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@control/mcp-server/src/test/java/io/tapstate/mcp/McpEnvironmentTest.java`
around lines 22 - 29, Update
emptyAndNullEnvironmentValuesAreRetainedWithoutControlVariables to either rename
it so it only claims empty-value coverage, or construct the input with a
nullable map implementation and add an actual null entry to verify
McpEnvironment.values() retains it. Prefer the latter if McpEnvironment is
expected to handle System.getenv-like maps.

control/mcp-server/src/test/java/io/tapstate/mcp/McpStdioProcessTest.java (1)

26-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use @TempDir so the stderr file is cleaned up on assertion failure.

Files.createTempFile creates stderr at line 27, but Files.deleteIfExists(stderr) runs only at line 75, after multiple assertions (lines 37-65, 73-74) that can throw. If any of them fails, the temp file is never deleted. Use a JUnit-managed @TempDir so cleanup happens regardless of test outcome.

♻️ Proposed fix

 class McpStdioProcessTest {
 
     `@Test`
-    void stdioNegotiatesListsReadToolsAndExitsAfterEofWithoutStdoutNoise() throws Exception {
-        Path stderr = Files.createTempFile("tapstate-mcp-", ".stderr");
+    void stdioNegotiatesListsReadToolsAndExitsAfterEofWithoutStdoutNoise(`@TempDir` Path tempDir) throws Exception {
+        Path stderr = tempDir.resolve("tapstate-mcp.stderr");
         Process process = start(stderr, unusedLoopbackPort());

The last two lines (assertThat(process.exitValue())... and Files.deleteIfExists(stderr)) can then drop the explicit delete, since @TempDir removes the directory afterward.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@control/mcp-server/src/test/java/io/tapstate/mcp/McpStdioProcessTest.java`
around lines 26 - 76, Update
stdioNegotiatesListsReadToolsAndExitsAfterEofWithoutStdoutNoise to use a JUnit
`@TempDir` directory for the stderr file instead of Files.createTempFile. Store
the stderr file beneath that managed directory, remove the explicit
Files.deleteIfExists cleanup, and retain the existing process and assertion
behavior.

cli/src/main/java/io/tapstate/cli/HttpControlPlaneClient.java (1)

60-93: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Close the lazily-built httpClient too, not just sharedClient.

close() releases sharedClient but never touches the pre-existing httpClient field used by login, apply, get, list, test, testResult, discoverSchema, schema, register, connectorList, lifecycle, status, metrics, snapshot, logs, and the websocket stream() helper. Since Java 21, java.net.http.HttpClient implements AutoCloseable and exposes close()/shutdown() precisely so callers are not left relying on GC to reclaim the selector thread and pooled connections. Adding AutoCloseable to this class now promises deterministic cleanup; closing only half of its two underlying HTTP clients does not fully honor that promise, particularly for long REPL sessions that also open websocket streams.

♻️ Proposed fix

     `@Override`
     public void close() {
         sharedClient.close();
+        if (httpClient != null) {
+            httpClient.close();
+        }
     }

Confirm the project's declared Java target for the cli module supports HttpClient.close() (Java 21+); the pattern-matching switch already used in tokenCreate/tokenList/tokenRevoke suggests it does.

Also applies to: 183-186

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/main/java/io/tapstate/cli/HttpControlPlaneClient.java` around lines
60 - 93, Update HttpControlPlaneClient.close() to deterministically close the
lazily initialized httpClient as well as sharedClient, guarding against it never
being created; first verify the cli module targets Java 21 or newer so
HttpClient.close() is available, and preserve cleanup for long-lived sessions
and stream() resources.

cli/src/main/java/io/tapstate/cli/RemoteCreatedToken.java (1)

3-5: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Mask the bearer value in toString().

The record's generated toString() prints token in clear text. Any future log line, assertion message, or exception that includes this record leaks the bearer value. McpOptions.toString() in this same change already masks its token, so apply the same rule here.

🔒️ Proposed change

 record RemoteCreatedToken(String tokenId, String scope, String token, String createdAt) {
+
+    `@Override`
+    public String toString() {
+        return "RemoteCreatedToken[tokenId=" + tokenId + ", scope=" + scope
+                + ", token=********, createdAt=" + createdAt + "]";
+    }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/main/java/io/tapstate/cli/RemoteCreatedToken.java` around lines 3 -
5, Override RemoteCreatedToken.toString() to mask the token field using the same
masking behavior as McpOptions.toString(), while retaining the other record
components in the representation and ensuring the bearer value is never emitted
in clear text.

cli/src/main/java/io/tapstate/cli/McpLauncher.java (1)

74-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Reuse executable for the native sidecar so Windows resolution matches the Java lookup.

executable(Path, String) already handles the .exe suffix, but the native sidecar probe only tries tapstate-mcp. On Windows a bundled tapstate-mcp.exe is not found, and the launcher falls back to the JAR path, which then requires a separate Java 21 runtime.

♻️ Proposed refactor

         Path libexec = bin.resolve("../libexec").normalize();
-        Path nativeSidecar = libexec.resolve("tapstate-mcp");
+        Path nativeSidecar = executable(libexec, "tapstate-mcp");
         List<String> command = new ArrayList<>();
-        if (Files.isRegularFile(nativeSidecar) && Files.isExecutable(nativeSidecar)) {
+        if (nativeSidecar != null) {
             command.add(nativeSidecar.toString());
         } else {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/main/java/io/tapstate/cli/McpLauncher.java` around lines 74 - 77,
Update the native sidecar lookup in McpLauncher to resolve the candidate through
the existing executable(Path, String) helper using the tapstate-mcp name, then
use that resolved Path for the regular-file and executable checks and command
construction. Preserve the existing JAR fallback when no executable is found.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

control/control-client/src/test/java/io/tapstate/control/client/HttpControlClientTest.java (2)

25-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add negative-duration cases to the constructor test.

constructorRejectsNonPositiveTimeouts checks only Duration.ZERO. Add a negative duration for both lightTimeout and heavyTimeout. Otherwise, a constructor that accepts negative values can pass this test.

Suggested additions

+        assertThatThrownBy(() -> new HttpControlClient(
+                Duration.ofMillis(-1), Duration.ofSeconds(1)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("lightTimeout");
+
+        assertThatThrownBy(() -> new HttpControlClient(
+                Duration.ofSeconds(1), Duration.ofMillis(-1)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("heavyTimeout");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/control-client/src/test/java/io/tapstate/control/client/HttpControlClientTest.java`
around lines 25 - 33, Extend constructorRejectsNonPositiveTimeouts to assert
IllegalArgumentException for negative lightTimeout and negative heavyTimeout
values, while preserving the existing zero-duration assertions and message
checks for each parameter.

---

130-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Assert the configured request budget.

The client uses 250 ms timeouts, but response.get(5, TimeUnit.SECONDS) allows a several-second regression to pass. Measure elapsed time and assert completion within the request budget plus explicit scheduler slack.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@control/control-client/src/test/java/io/tapstate/control/client/HttpControlClientTest.java`
around lines 130 - 151, Update requestBudgetBoundsAStalledExchange to measure
elapsed time around the client request and assert that response completion
occurs within the configured 250 ms request budget plus an explicit, narrowly
defined scheduler slack. Replace the five-second response.get allowance with a
timeout aligned to that budget, while preserving the existing Unreachable
assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes