A fresh random fence per call makes budget plans non-reproducible run to run

[devops-thiago]

Found while diagnosing #586.

PromptTemplateEscaper.fence() mints a fresh 32-hex-char CSPRNG token per call. A random hex string does not tokenize to a fixed BPE width — measured over 2000 draws: min 53, max 81 tokens, spread 28.

Both AbstractPrSuggestionGenerator.sharedPromptOverhead (shared by /describe, /improve, /add-docs, /generate-tests) and DiffBudgetPlanner.plan(...) size their overhead by drawing a live fence. So the overhead subtracted from the budget varies by up to ~30 tokens between runs of the same input, and the resulting batch plan is not reproducible.

This is not a production bug. The sample is an unbiased estimate of an overhead that genuinely is random per call, and a ~30-token error is absorbed by token-safety-margin at real budgets of 10⁵. It is recorded because:

1. It cost real time to diagnose. In Disclosure classification races under the parallel batch pass (flaky test, possible real bug) #586 it was the entire cause of a 1-in-1500 CI failure that had been filed as a possible concurrency race in the review pipeline — a hypothesis it took a full investigation to refute.
2. Sibling tests that set a small max-input-tokens carry the same latent fragility, and will fail the same way, rarely, for the same reason. Disclosure classification races under the parallel batch pass (flaky test, possible real bug) #586's fix pads its own fixtures past the jitter but does not address the others.
3. Non-reproducible planning is awkward to reason about generally — the same PR can batch differently on two runs with no input change.

Suggested direction: size the overhead from a fixed-width representative fence (the width is what matters, not the value; the real fence still gets used in the emitted prompt). Plans then become reproducible for a given input, and the class of rare test flake disappears rather than being padded around.

[devops-thiago]

This is no longer latent — it is failing CI now

Predicted in #586's investigation ("sibling tests using small max-input-tokens carry the same latent fragility") and it has now happened, on an unrelated PR.

PR #596, head fdd9602, test job failed:

[ERROR] DocGenerationServiceTest.keepsTheDocsFromTheBatchesThatSucceededWhenOneBatchFails:943
  📝 ThrillhouseBot added **1** documentation suggestion(s) for changed symbols...
[ERROR] Tests run: 2814, Failures: 1, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

The change on that PR is confined to SummarySurfaceDeduplicator and touches nothing in the doc-generation path. The full suite passed locally at 2789 tests on the same commit.

DocGenerationServiceTest is the same construction as #586's ChangelogEntryGeneratorTest, line for line:

135:  private void budgetWithDiffRoom(int diffTokens) {
141:                    + PromptTemplateEscaper.fence(" ")     <-- live fence, 53-81 tokens
145:    when(activeModel.maxInputTokens()).thenReturn(overhead + diffTokens);
...
923:  void keepsTheDocsFromTheBatchesThatSucceededWhenOneBatchFails() {
924:    budgetWithDiffRoom(40);                                  <-- 40-token margin
943:    assertTrue(postedSummary().contains("Partial pass"), postedSummary());

Same 40-token margin, same live fence draw sized against a jitter #586 measured at spread 28 (min 53, max 81), and the same "the partial-pass disclosure did not appear" assertion. There are eleven budgetWithDiffRoom(40) call sites in this one file (lines 227, 839, 859, 887, 924, 959, 977, 1001, 1064, …), so this is not a single fragile test.

Why this changes the priority

Filed as tech-debt on the reasoning that it costs diagnosis time rather than correctness. That reasoning is now out of date: it fails CI on PRs that did not touch the code, and the cost lands on whoever is unlucky. The first person to hit it will do what happened with #586 — open a bug against the production pipeline and investigate a race that does not exist.

#586's fix padded its own fixtures above the jitter but deliberately did not touch siblings. The general fix proposed here (size the overhead from a fixed-width representative fence, keep the real fence in the emitted prompt) fixes every call site at once and is the right move rather than padding eleven more fixtures.

Re-running the failed job to confirm it is nondeterministic rather than a genuine regression; result to follow.

[devops-thiago]

Confirmed nondeterministic.

The failing commit fdd9602 and the passing commit fa7267b differ only in javadoc text — no production logic, no test code, and DocGenerationServiceTest is byte-identical between them. Same suite, opposite results:

commit	change	test job
fdd9602	contraction handling in SummarySurfaceDeduplicator	FAILURE — DocGenerationServiceTest.keepsTheDocsFromTheBatchesThatSucceededWhenOneBatchFails:943
fa7267b	javadoc wording only	SUCCESS

So this is the #586 failure mode reproducing in a sibling test, exactly as predicted there, and not a regression from either commit. No production code was changed and no fixture was padded in response — the PR was left as-is and the next run went green on its own.

That is the whole problem: the failure carries no signal about its own cause, lands on an unrelated PR, and resolves itself on a re-run. Someone will eventually spend the diagnosis time #586 already spent.

The fix proposed above — size the overhead from a fixed-width representative fence while the real fence still goes into the emitted prompt — makes every budgetWithDiffRoom call site deterministic at once, across both this file's eleven sites and any sibling that uses the same helper.