test(managed-agent): state which coordinator refusal paths are real - #13099

Reviewed at head 697d38a33e270265ebee7c336aa72786ceec2bc0.

Verdict: APPROVE

No Critical has ever been filed on this PR — both automated rounds recorded only Suggestions — and my own scan of the complete diff found none. The change is a net fidelity improvement, not a weakened gate.

What I verified

The diff replaces one @CsvSource parameterized test whose four rows drove a single branchy if (expectRetry) … else … assertion with six focused tests plus a shared dispatchWithCreateOrLoadFailure(failure, submitted, retryCount) helper. Each new test asserts one classification cell and its negation, which is what makes them mutation-sensitive rather than vacuous:

• failsOnWorkspaceAuthorizationRefusalBeforeSubmission (@ValueSource(ints = {0, 5})) pins failTurn(…, "workspace_unavailable", refusal.getMessage()) plus never() scheduleTurnRetry.
• retriesWorkspaceAuthorizationRefusalAfterARecordedSubmission pins the inverse for submitted=true, retryCount=5 — retry and never() failTurn.
• retriesAConflictReportedByTheHarness pins the 409 cell of the DaemonHttpException arm; failsAPermanentHarnessRejectionAfterSubmission pins the non-409 4xx cell against hosted_harness_rejected; retriesATransientHarnessFailureAfterSubmission pins the 5xx cell. Together these are the three-way status classification that previously had only its 409-retry cell covered.
• retriesARetryableBrokerRefusalBeforeSubmissionDefensively is explicitly labelled defensive, and the comment says outright that no current producer reaches the coordinator with a retryable RuntimeBrokerException before submission. Pinning the isRetryable() clause while stating that it models nothing real is the honest form of that test.

The mis-certification the first round reported is genuinely corrected rather than reworded. The group comment now states that workspace_busy is raised inside the Broker's tool-execution transport and consumed there, and that the 409 test pins session-level Harness conflicts (hosted_turn_active, hosted_prompt_conflict, hosted_event_epoch_mismatch) rather than a create-time conflict, which the connector answers by loading the existing authority. The old @CsvSource row that minted a RuntimeBrokerException(409, "workspace_busy", …) as a stand-in for Broker lease contention is gone, so no test now claims a path that does not exist.

Every test still ends with verify(harness, never()).submit(…), so the pre-submission boundary stays pinned across all six.

Non-blocking, and not part of the verdict

The one open thread I would flag to the author as worth a look, though it is graded Suggestion and does not gate: the helper stubs only the loadExisting=false arity of createOrLoad, while three of the six callers pass submitted=true — a state production reaches with loadExisting computed from session.harnessBootId() != null. So those three tests drive a state combination production does not produce. The classification logic they pin is still real and still mutation-sensitive, which is why this is fidelity rather than a false green.

CI

At this head 4 checks succeeded and 96 were skipped, with no failures and nothing pending. The lanes that would execute these Java tests report skipped, so nothing on the PR page evidences that the six new tests have run; the author also noted the two mutations were not run locally. I am not treating that as a blocker — this channel does not gate on CI state, and the assertions are readable as non-vacuous — but it is worth a maintainer confirming the Java lane executes them before merge.

Scan tier — LGTM

The CsvSource decomposition is accurate: each new test targets exactly one scenario, with the submitted / retryable / status-code axes independently exercised. The defensive test (retriesARetryableBrokerRefusalBeforeSubmissionDefensively) correctly pins the isRetryable() clause without overstating production coverage — the header comment is explicit that no current producer reaches this arm. The three DaemonHttpException rows (409 conflict, 400 permanent, 503 transient) are genuine new coverage consistent with the Harness contract described in the comments.

dispatchWithCreateOrLoadFailure returning store and leaving assertion to each caller is a better factoring than the original if-else: mutations that delete one arm no longer silently pass (the test fails with a missing interaction rather than the wrong branch executing).

The rename doesNotExhaustAfterSubmissionMayHaveBeenAdmitted → neverTerminatesOnceSubmissionMayHaveBeenAdmitted and @ParameterizedTest(name = "workspace refusal = {0}") improve CI failure messages for the post-submission invariant.

No issues found.

Scan tier — LGTM

The CsvSource decomposition is accurate: each new test targets exactly one scenario, with the submitted / retryable / status-code axes independently exercised. The defensive test (retriesARetryableBrokerRefusalBeforeSubmissionDefensively) correctly pins the isRetryable() clause without overstating production coverage — the header comment is explicit that no current producer reaches this arm. The three DaemonHttpException rows (409 conflict, 400 permanent, 503 transient) are genuine new coverage consistent with the Harness contract described in the comments.

dispatchWithCreateOrLoadFailure returning store and leaving assertion to each caller is a better factoring than the original if-else: mutations that delete one arm no longer silently pass (the test fails with a missing interaction rather than the wrong branch executing).

The rename doesNotExhaustAfterSubmissionMayHaveBeenAdmitted → neverTerminatesOnceSubmissionMayHaveBeenAdmitted and @ParameterizedTest(name = "workspace refusal = {0}") improve CI failure messages for the post-submission invariant.

No issues found.

Approving at 697d38a33e27. The change replaces one @CsvSource table with three named tests, and the naming is the substance: the old table encoded four boolean/int combinations without saying which correspond to a producer that exists.

What the new shape states explicitly, and what I checked:

• failsOnWorkspaceAuthorizationRefusalBeforeSubmission pins the only refusal that actually reaches the coordinator pre-submission — WorkspaceExecutionStore.unavailable() (409, workspace_unavailable, not retryable) from the connector's authorization in createOrLoad — and asserts both halves: failTurn with that code, and scheduleTurnRetry never called.
• retriesWorkspaceAuthorizationRefusalAfterARecordedSubmission pins the invariant the old table carried as an anonymous true, false, 0, true row: a claim that already recorded a submission attempt may have been admitted, so even a permanent refusal must not end it before the pre-admission budget. Naming it is what stops a future reader from "simplifying" it back into the permanent-failure arm.
• retriesARetryableBrokerRefusalBeforeSubmissionDefensively is labelled defensive in its own comment ("no current producer reaches the coordinator with a retryable RuntimeBrokerException before submission"), so it pins the isRetryable() clause without claiming a path that does not exist. That is the right way to keep an unreachable-arm test.
• The comment at :86-91 records why Broker lease contention is absent: workspace_busy is raised inside the Broker's tool-execution transport and consumed there as a turn_error, so it never reaches this arm.

Mechanical checks, since an import edit in Java is a compile error rather than a lint warning: CsvSource has 0 occurrences at this head, so dropping the import is correct; DaemonHttpException is imported at :17 and used at :142, :157, :171; dispatchWithCreateOrLoadFailure(RuntimeException, boolean, int) at :181-182 matches every new call site; new RuntimeBrokerException(409, "defensive_retryable", …) at :126 matches the (int, String, String, boolean) constructor. All of that is also settled empirically — CI is 22 pass / 0 fail with eight Java lanes green (ubuntu-latest JDK 11/17/21, macos-latest 21, windows-latest 21, Real daemon E2E 11, Runtime Broker + Managed Agent MariaDB 21, Hosted process fault gates MySQL 8.4 / 21).

Links #13055 and #13056. No findings.

Disclosure: I did not execute the Java suite; per the standing rule on this repo I do not run or patch Java locally. The verdict rests on reading the test plus the eight green Java lanes.