From 9092718be2fa87740b8f4b6114bafe43d287e870 Mon Sep 17 00:00:00 2001 From: Adam Frisby Date: Thu, 16 Jul 2026 14:30:51 +0000 Subject: [PATCH 1/4] docs: evaluate caveman output-compression as a per-agent token-saving layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spike evaluating the caveman skill (MIT) as an output-compression layer over CodeyBox's existing agents. Maps the stdout capture/parse path and finds the compatibility gate PASSES: parsers key on JSON envelope fields, the failure classifier on literal CLI error signatures, and turn success on process exit + git state — none of which caveman rewrites (it compresses only NL prose and keeps code/commands/structured output byte-exact). Documents the residual exact-literal contract risk (commit trailers, verdict sentinels, .codeybox JSON), measures realistic savings as modest on our tool-heavy thinking-model workload (output-only, reasoning + tool-use payloads untouched, ~1-1.5k input-token/turn overhead), flags the headless-activation caveat, and designs a default-off, hot-reloadable, per-agent opt-in via the existing prompt-preprocessor seam. No behaviour wired on — implementation deferred behind the designed gates. CodeyBox-Prompt-Revision: 1 Co-Authored-By: CodeyBox --- docs/README.md | 6 ++ docs/caveman-evaluation.md | 190 +++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+) create mode 100644 docs/caveman-evaluation.md diff --git a/docs/README.md b/docs/README.md index 912d9d83..04764c94 100644 --- a/docs/README.md +++ b/docs/README.md @@ -88,6 +88,12 @@ at anything that matters. | [manual UAT](development/manual-uat/) | operator checklists for what automated tests cannot cover | | [`AGENTS.md`](../AGENTS.md) | the engineering contract every change is graded against | +## Evaluations — spikes + +| Page | What it covers | +|---|---| +| [caveman evaluation](caveman-evaluation.md) | spike: evaluating the `caveman` output-compression skill as a token-saving layer over existing agents — capture/parse compatibility gate, measured savings, per-agent opt-in design | + ## The other clients The **admin dashboard** ([`tools/CodeyBox.Admin/`](../tools/CodeyBox.Admin/README.md)) diff --git a/docs/caveman-evaluation.md b/docs/caveman-evaluation.md new file mode 100644 index 00000000..4a76134b --- /dev/null +++ b/docs/caveman-evaluation.md @@ -0,0 +1,190 @@ +# Evaluation: caveman as an output-compression layer over CodeyBox agents + +**Status:** Spike / evaluation. No behavioural code shipped with this document — +the capture-compatibility gate is analysed, savings and risks are measured against +real captured streams, and a config-driven per-agent opt-in is designed and left +ready to build. See [Recommendation](#recommendation). + +**Subject:** [`caveman`](https://github.com/JuliusBrussee/caveman) (`v1.8.x`, MIT, +© 2026 Julius Brussee) — a token-**compression** skill/plugin, *not* a runner +agent. It injects a system-prompt ruleset that makes an existing coding-agent CLI +emit terser natural-language prose ("drop articles, filler, pleasantries") while, +per its own ruleset, keeping "code, commands, errors, file paths, URLs, JSON, +identifiers" and "structured output and machine-readable formats" **byte-for-byte +exact**. It hooks across Claude Code, Codex, Gemini, Cursor, Copilot, and 30+ +agents. + +The question this spike answers: does caveman interfere with the way CodeyBox +tees and **parses** agent stdout / structured streams? If yes → NO-GO, document +and stop. If no → measure real savings vs. risk and design a per-agent opt-in. + +--- + +## 1. What CodeyBox actually depends on in agent output + +CodeyBox requests each CLI's structured streaming mode (`claude --print +--output-format stream-json --verbose`, `codex exec --json`, gemini/agy +stream-json when probed-capable), tees stdout to JSONL under `logs/agents/…`, +and parses it (see [`agent-streams.md`](agent-streams.md), +[`stream-analysis.md`](stream-analysis.md)). The parse/consume path was mapped +end-to-end. Every dependency falls into one of three buckets, and **none of them +branches on the wording of the model's natural-language prose** — which is the +only thing caveman rewrites: + +| # | Consumer | Keys off | Touched by caveman? | +|---|----------|----------|---------------------| +| 1 | Stream parsers (`FlexibleAgentStreamParser` + per-agent `ClaudeStreamParser`/`CodexStreamParser`/`GeminiStreamParser`/`CursorStreamParser`) | JSON envelope **fields**: event `type`, `usage`/`token_usage` counts, `total_cost_usd`, `tool_use`/`tool_result` ids + names, `is_error`, durations | **No.** Requires exact field names/types; the CLI serialises the envelope, not the model. Final assistant text is *stored*, never keyword-scanned. | +| 2 | `AgentFailureClassifier` | Literal **CLI stderr/stdout error signatures**: `usage_limit`, `HTTP 429`, `overloaded_error`, `401 Unauthorized`, `invalid_api_key`, `ECONNRESET`, `command not found`, CLI login-prompt sentences, `turn.failed` JSON `error.message` | **No.** These are CLI/HTTP/provider strings, kept byte-exact. The classifier *deliberately* refuses to trust free-form stdout precisely because "stdout can be model-controlled" — terser prose only *shrinks* its false-positive surface. | +| 3 | Turn success / completion (`PipelineRunner`) | **Process exit code** (`AgentResult.Success = ExitCode==0`) + **git state** (`git diff --cached --quiet` exit, `HasMeaningfulAgentChangesAsync`) | **No.** `AgentResult.Summary` is consumed only for exit-code extraction and redacted logging, never prose-branched. | + +The single place model prose flows *downstream* is the **cross-agent handoff +brief** (`AgentStreamBriefBuilder` → `CrossAgentHandoffPromptPreprocessor`): a +≤2000-char tail of the prior agent's final message, injected into a fallback +agent's prompt inside a fenced `[UNTRUSTED DATA SECTION]` block. It is advisory +context for another LLM — concatenated and sanitised, never parsed for exact +content — and is already opt-in (`EnableHandoffSeeding`, default `false`). +Terser prior-agent prose summarises fine here; there is no exact-content contract. + +The plaintext-fallback summariser counts lines containing `error`/`fatal`/… +into an observability integer (`[plaintext-fallback … errors=N]`). Nothing +branches on it; it is a dashboard counter. Cosmetic at most. + +## 2. The real risk surface: model-emitted **exact-literal** contracts + +CodeyBox requires the *model itself* to emit several byte-exact structured +artifacts. These are **not prose** — they are exactly the "structured output / +machine-readable formats / code / identifiers" that caveman's ruleset lists as +**never-compress**. A correctly-scoped caveman leaves them intact; the risk is +purely one of **imperfect model compliance** with a soft prompt instruction: + +- **Commit messages + the `CodeyBox-Prompt-Revision` trailer.** When the + *orchestrator* commits it composes the trailer block itself (byte-exact, safe). + But the design also has the **agent echo** `CODEYBOX_PROMPT_REVISION` as a + trailer on commits it makes itself; a `process:prompt-revision-trailer` auditor + verifies it via regex `^\s*\*?CodeyBox-Prompt-Revision\s*:\s*(\d+)\s*\*?\s*$`. + A terse-rewrite of a commit message would fail that audit → blocks merge. +- **Check-and-Act verdict sentinels** — the model must output + `<<>> {json} <<>>`; the parser hard-fails on + missing sentinels or fields. +- **`…`** blocks — strict regex. +- **Plan-audit verdict JSON** and **`.codeybox/suggestions.json`** enum tokens — + strict JSON with fixed vocabularies; malformed entries are dropped/blocking. + +**Gate verdict:** the capture/parse path is **compatible** — CodeyBox parses JSON +envelopes, CLI error signatures, exit codes, and git state, all of which caveman +leaves exact by design. The residual exposure is not a parse-*interference* but a +*probabilistic compliance* risk against the exact-literal contracts above, which +collides directly with this repo's "false machine-facing evidence is the gravest +offense" contract. That risk is real but bounded and testable (§4). + +## 3. Measured savings — small on *our* workload, not the headline 65 % + +caveman's "average 65 % output reduction" is measured on **prose-heavy** prompts +(bug explanations 87 %, Q&A 72 %, web-search summarisation 68 %). Its own numbers +for the category that matches CodeyBox — **"Code edits: 50 %"**, "Architecture +discussion: 30 %" — are lower, and three multipliers shrink them further for a +tool-using coding agent: + +1. **Output-tokens only.** Input and **reasoning** tokens are untouched. For the + thinking models CodeyBox runs (e.g. `claude-opus-*-thinking`), reasoning is a + large share of billed output — entirely outside caveman's reach. +2. **Tool-use payloads are untouched.** Inspecting CodeyBox's own captured + stream fixtures (`tests/…/Fixtures/AgentStreams/*.jsonl`), a coding turn's + `usage.output_tokens` is dominated by `tool_use` inputs — the exact commands + and file edits (`dotnet test …`, `Edit` diffs) — which caveman keeps + byte-exact. The compressible surface is only the interstitial NL prose plus + the short final `result` summary. +3. **~1–1.5 k input tokens/turn overhead** for the ruleset itself. On short + tool-heavy turns this can make the change **net-negative**. + +Net: expect low-single- to low-double-digit percent of *output* tokens on real +work-phase turns, with the largest wins on the rare prose-heavy turns (audit +explanations, question text). This is worth trialling behind a flag, not +enabling globally. + +**A live end-to-end measurement was not run in this spike** because it requires +API-credentialed agent invocations that this sandbox does not hold. CodeyBox +already persists the exact substrate for that measurement: `agent_stream_summaries` +records `output_tokens`, `input_tokens`, and `estimated_usd` **per invocation**. +The operator A/B is therefore first-class (§5): run a fixed work-item corpus with +the flag off, then on, and read the deltas straight from that table — no new +instrumentation needed. + +## 4. Activation caveat (headless mode) + +caveman's always-on path for Claude Code is a **SessionStart/UserPromptSubmit +hook** that writes a per-session flag file. CodeyBox invokes every CLI headless +and single-shot (`--print` / `exec`), in a **fresh cloned VM per work item** with +no prior interactive session to set the flag. Whether those hooks fire in headless +mode is unverified upstream. The robust enablement therefore does **not** rely on +caveman's session hook — it injects the ruleset explicitly per invocation (§5), +which also makes the toggle deterministic and hot-reloadable. + +## 5. Proposed per-agent opt-in (config-driven, hot-reloadable) — design, not yet built + +Reuse existing seams; add no generic "arbitrary extra args" passthrough (there is +none today, deliberately). + +- **Enablement channel — the existing `IAgentPromptPreprocessor` chain.** Add a + `CavemanPromptPreprocessor` that prepends the caveman ruleset (vendored, pinned + by commit — MIT permits this; do not depend on a live `npx`/marketplace fetch + inside the sandbox) to the work/rework prompt **only when enabled for that + agent kind**. This is the same ordered seam already used by + `CrossAgentHandoffPromptPreprocessor`, so it is provider-agnostic and needs no + per-CLI flag plumbing. For Claude specifically, an optional enhancement is to + route the ruleset through its separate system-prompt channel + (`SupportsSeparateSystemPrompt` / `--append-system-prompt`) instead of the + prompt body. +- **Config — a hot-reloadable per-agent snapshot**, mirroring the established + `AgentNetworkToleranceSnapshot` / `AgentDefaultsSnapshot` / `PipelineTuningSnapshot` + patterns (all `IOptionsMonitor`-backed). Shape: + + ```json + { + "CodeyBox": { + "Caveman": { + "Enabled": false, + "PerAgent": { "claude": true, "codex": false, "gemini": false }, + "RulesetPath": "vendor/caveman/SKILL.md", + "Channel": "AppendSystemPrompt" + } + } + } + ``` + + Default **off** everywhere. `Enabled=false` is a hard master switch. `RulesetPath` + points at the vendored, version-pinned ruleset (no network at dispatch). + `Channel` ∈ `{PromptPrefix, AppendSystemPrompt}`. +- **Do NOT adopt caveman's opt-in sub-features:** `/caveman-compress` rewrites + repo memory files (e.g. `CLAUDE.md`) — that is a real working-tree mutation and + must never run in an autonomous work sandbox. The `caveman-shrink` MCP + middleware compresses tool *descriptions* and would sit on the tool-call path — + out of scope and explicitly excluded. +- **Guardrails the ruleset text must carry** (belt-and-braces against the §2 + compliance risk): an explicit instruction that commit messages, the + `CodeyBox-Prompt-Revision`/`Co-Authored-By` trailers, the + `<<>>`/`` blocks, and any `.codeybox/*.json` + are structured output to be emitted verbatim. caveman's stock ruleset already + covers "structured output / machine-readable formats"; the vendored copy should + name CodeyBox's specific contracts too. +- **Ship behind these tests before default-on for any agent:** (a) a regression + asserting the `CodeyBox-Prompt-Revision` trailer still matches the auditor regex + on a caveman-enabled commit; (b) round-trip parse of a `<<>>` + block emitted under caveman; (c) an A/B token-savings check reading + `agent_stream_summaries.output_tokens` across a fixed corpus, asserting net + savings > 0 for the enabled agent before promoting it. + +## Recommendation + +**Compatible — conditional GO, gated behind a default-off per-agent opt-in.** The +capture/parse gate passes: nothing CodeyBox parses or branches on is what caveman +rewrites. But the realistic savings on CodeyBox's tool-heavy, thinking-model +workload are modest and possibly net-negative on short turns, and there is a real +(if bounded) compliance risk against the exact-literal machine-facing contracts in +§2. The responsible path is the §5 design: vendored+pinned ruleset injected via the +existing prompt-preprocessor seam, hot-reloadable per-agent config defaulting to +**off**, the two parse-safety regressions and the A/B savings check as gates before +enabling any single agent. This spike delivers that analysis and design; it +intentionally does **not** wire the feature on, because the measurement does not +justify shipping enablement blindly, and a default-off feature with no live +consumer would be speculative machinery. From 1becd6ccc6174038432658eb4cca1a13c26a8469 Mon Sep 17 00:00:00 2001 From: Adam Frisby Date: Tue, 21 Jul 2026 20:04:08 +0000 Subject: [PATCH 2/4] fix: isolate NuGet home for required build gate Pin HOME alongside DOTNET_CLI_HOME while preserving the original global package cache. Strengthen the regression fake to model NuGet builds that ignore DOTNET_CLI_HOME. CodeyBox-Prompt-Revision: 1 Co-Authored-By: CodeyBox --- .../SandboxRequiredBuildVerifier.cs | 10 ++++++++-- tests/CodeyBox.Tests/RequiredBuildGateTests.cs | 17 +++++++++-------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/CodeyBox.Orchestrator/SandboxRequiredBuildVerifier.cs b/src/CodeyBox.Orchestrator/SandboxRequiredBuildVerifier.cs index 5c5f4730..17d90bd3 100644 --- a/src/CodeyBox.Orchestrator/SandboxRequiredBuildVerifier.cs +++ b/src/CodeyBox.Orchestrator/SandboxRequiredBuildVerifier.cs @@ -106,6 +106,7 @@ public sealed class SandboxRequiredBuildVerifier : IRequiredBuildVerifier # assemblies. Redirect the CLI/NuGet per-user home to a directory this # script owns so the gate no longer depends on $HOME being writable. dotnet_home="$tmp_root/codeybox-dotnet-home-$$" + original_home="${HOME:-}" cleanup() { rm -rf "$targets_file" "$dotnet_home"; } trap cleanup EXIT INT TERM @@ -121,10 +122,15 @@ trap cleanup EXIT INT TERM # against an empty folder and require network access. Preserve that # cache (read access is sufficient — restore never writes to an # already-extracted package) so offline/pinned images keep working. - if [ -z "${NUGET_PACKAGES:-}" ] && [ -n "${HOME:-}" ] && [ -d "$HOME/.nuget/packages" ]; then - export NUGET_PACKAGES="$HOME/.nuget/packages" + if [ -z "${NUGET_PACKAGES:-}" ] && [ -n "$original_home" ] && [ -d "$original_home/.nuget/packages" ]; then + export NUGET_PACKAGES="$original_home/.nuget/packages" fi + # Some NuGet builds resolve their user-config path from HOME even when + # DOTNET_CLI_HOME is set. Point both variables at the isolated writable + # directory, after preserving the original package-cache path above. + export HOME="$dotnet_home" + find . -maxdepth 1 -type f \( -name '*.slnx' -o -name '*.sln' \) | sort > "$targets_file" if [ ! -s "$targets_file" ]; then diff --git a/tests/CodeyBox.Tests/RequiredBuildGateTests.cs b/tests/CodeyBox.Tests/RequiredBuildGateTests.cs index 49175389..98535460 100644 --- a/tests/CodeyBox.Tests/RequiredBuildGateTests.cs +++ b/tests/CodeyBox.Tests/RequiredBuildGateTests.cs @@ -186,6 +186,8 @@ public async Task BuildScript_FakeDotnet_IsADiscriminatingDetector() psi.Environment.Remove("DOTNET_CLI_HOME"); psi.Environment.Remove("NUGET_PACKAGES"); psi.Environment["HOME"] = brokenHome; + psi.Environment["EXPECTED_NUGET_PACKAGES"] = + Path.Combine(brokenHome, ".nuget", "packages"); using var proc = Process.Start(psi)!; var stderr = await proc.StandardError.ReadToEndAsync(); @@ -229,6 +231,7 @@ public async Task BuildScript_FakeDotnet_IsADiscriminatingDetector() psi.Environment.Remove("NUGET_PACKAGES"); psi.Environment["HOME"] = home; psi.Environment["TMPDIR"] = tmpDir; + psi.Environment["EXPECTED_NUGET_PACKAGES"] = Path.Combine(home, ".nuget", "packages"); psi.Environment["PATH"] = Path.GetDirectoryName(fakeDotnet) + Path.PathSeparator + "/usr/bin:/bin"; @@ -242,10 +245,9 @@ public async Task BuildScript_FakeDotnet_IsADiscriminatingDetector() /// /// Writes a fake dotnet that models NuGet restore's real - /// precondition: it reads/creates the per-user settings directory under the - /// CLI home ($DOTNET_CLI_HOME, else $HOME) and fails if that directory is - /// not writable, and it fails if a pre-baked $HOME/.nuget/packages cache - /// was not preserved via NUGET_PACKAGES. + /// precondition: it reads/creates the per-user settings directory from HOME, + /// deliberately ignoring DOTNET_CLI_HOME as affected NuGet builds do, and it + /// fails if the pre-baked package cache was not preserved via NUGET_PACKAGES. /// private async Task WriteNuGetSensitiveFakeDotnetAsync() { @@ -255,15 +257,14 @@ private async Task WriteNuGetSensitiveFakeDotnetAsync() await File.WriteAllTextAsync(dotnet, """ #!/bin/sh # Model NuGet's writable per-user settings-directory requirement. - cli_home="${DOTNET_CLI_HOME:-$HOME}" - ngdir="$cli_home/.nuget/NuGet" + ngdir="$HOME/.nuget/NuGet" mkdir -p "$ngdir" 2>/dev/null || true if ! touch "$ngdir/NuGet.Config" 2>/dev/null; then echo "error : Failed to read NuGet.Config due to unauthorized access. Path: '$ngdir/NuGet.Config'." >&2 exit 1 fi - if [ -n "${HOME:-}" ] && [ -d "$HOME/.nuget/packages" ] \ - && [ "${NUGET_PACKAGES:-}" != "$HOME/.nuget/packages" ]; then + if [ -n "${EXPECTED_NUGET_PACKAGES:-}" ] \ + && [ "${NUGET_PACKAGES:-}" != "$EXPECTED_NUGET_PACKAGES" ]; then echo "error : pre-baked NuGet package cache not preserved (NUGET_PACKAGES=${NUGET_PACKAGES:-unset})" >&2 exit 1 fi From 9af2e5eec8f8e17528befe095c559f8395556849 Mon Sep 17 00:00:00 2001 From: Adam Frisby Date: Tue, 21 Jul 2026 20:59:16 +0000 Subject: [PATCH 3/4] fix: preserve recovered state during log path persistence CodeyBox-Prompt-Revision: 1 Co-Authored-By: CodeyBox --- src/CodeyBox.Orchestrator/PipelineRunner.cs | 18 +++++++-------- .../AgentLogPathHelpersTests.cs | 22 +++++++++++++++++++ .../HostShutdownCancellationTests.cs | 3 +++ .../RecoveryCancellationPipelineTests.cs | 10 +++++++-- 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/CodeyBox.Orchestrator/PipelineRunner.cs b/src/CodeyBox.Orchestrator/PipelineRunner.cs index ee9d325b..d665aa6d 100644 --- a/src/CodeyBox.Orchestrator/PipelineRunner.cs +++ b/src/CodeyBox.Orchestrator/PipelineRunner.cs @@ -18200,18 +18200,19 @@ tree is null /// /// Persists on BEFORE /// the agent runs so a SIGTERM mid-invocation lets the shutdown teardown - /// handler read the path out of the store. Re-reads the latest row so we - /// do not regress a concurrent update from another worker thread on the - /// same item (priority bump, prompt edit, etc). + /// handler read the path out of the store. The write is guarded by the + /// state and update stamp from the row read here so it cannot restore a + /// stale lifecycle snapshot over concurrent recovery or cancellation. /// private Task PersistAgentLogPathAsync(WorkItemId id, string agentLogPath, CancellationToken ct) => PersistAgentLogPathAsync(_store, _log, id, agentLogPath, ct); /// /// Static testable core of . - /// Returns true when a write was issued, false when short-circuited (item - /// missing, path already matches) or swallowed (store exception). Cancellation - /// is propagated; every other exception is logged at warning and absorbed. + /// Returns true when the guarded write succeeds, false when short-circuited + /// (item missing, path already matches), the row changes concurrently, or a + /// store exception is swallowed. Cancellation is propagated; every other + /// exception is logged at warning and absorbed. /// internal static async Task PersistAgentLogPathAsync( IWorkItemStore store, @@ -18226,12 +18227,11 @@ internal static async Task PersistAgentLogPathAsync( if (fresh is null) return false; if (string.Equals(fresh.AgentLogPath, agentLogPath, StringComparison.Ordinal)) return false; - await store.UpdateAsync(fresh with + return await store.TryUpdateIfStateAndUpdatedAtAsync(fresh with { AgentLogPath = agentLogPath, UpdatedAt = DateTimeOffset.UtcNow, - }, ct); - return true; + }, fresh.State, fresh.UpdatedAt, ct); } catch (Exception ex) when (ex is not OperationCanceledException) { diff --git a/tests/CodeyBox.Tests/AgentLogPathHelpersTests.cs b/tests/CodeyBox.Tests/AgentLogPathHelpersTests.cs index d2e4a59a..ca76211b 100644 --- a/tests/CodeyBox.Tests/AgentLogPathHelpersTests.cs +++ b/tests/CodeyBox.Tests/AgentLogPathHelpersTests.cs @@ -126,6 +126,26 @@ public async Task Persist_IsIdempotent_WhenPathAlreadyMatches() Assert.False(wrote); } + [Fact] + public async Task Persist_DoesNotRestoreStaleStateOverConcurrentRecovery() + { + var item = MakeItem(); + await _store.CreateAsync(item); + var racingStore = new RaceAdvancingStore(_store); + racingStore.ArmRace(); + + var wrote = await PipelineRunner.PersistAgentLogPathAsync( + racingStore, NullLogger.Instance, item.Id, + "/work/.codeybox/agent-logs/raced.log", CancellationToken.None); + + Assert.False(wrote); + Assert.True(racingStore.RaceInjected); + var after = await _store.GetAsync(item.Id); + Assert.NotNull(after); + Assert.Equal(WorkItemState.Queued, after!.State); + Assert.Null(after.AgentLogPath); + } + [Fact] public async Task Persist_ReturnsFalse_WhenItemMissing() { @@ -183,6 +203,8 @@ private sealed class ThrowOnUpdateStore : IWorkItemStore }); public Task UpdateAsync(WorkItem item, CancellationToken ct = default) => throw new InvalidOperationException("simulated store hiccup"); + public Task TryUpdateIfStateAndUpdatedAtAsync(WorkItem item, WorkItemState onlyIfState, DateTimeOffset onlyIfUpdatedAt, CancellationToken ct = default) => + throw new InvalidOperationException("simulated store hiccup"); // ── unused ── public Task CreateAsync(WorkItem item, CancellationToken ct = default) => Task.CompletedTask; diff --git a/tests/CodeyBox.Tests/HostShutdownCancellationTests.cs b/tests/CodeyBox.Tests/HostShutdownCancellationTests.cs index db31788f..a499df66 100644 --- a/tests/CodeyBox.Tests/HostShutdownCancellationTests.cs +++ b/tests/CodeyBox.Tests/HostShutdownCancellationTests.cs @@ -1618,6 +1618,8 @@ public ShutdownTestHarness(PipelineRunner pipeline, SqliteWorkItemStore store, L /// internal sealed class BlockingAgentRunner : IAgentRunner { + public TaskCompletionSource Started { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + public AgentKind Kind { get; init; } = AgentKind.Claude; public async Task RunAsync( @@ -1630,6 +1632,7 @@ public async Task RunAsync( CancellationToken ct = default, Action? stdoutChunkCallback = null, bool captureStructuredStream = false) { + Started.TrySetResult(); await Task.Delay(Timeout.Infinite, ct); return new AgentResult(false, "unreachable", null, null); } diff --git a/tests/CodeyBox.Tests/RecoveryCancellationPipelineTests.cs b/tests/CodeyBox.Tests/RecoveryCancellationPipelineTests.cs index c0210998..5cd16978 100644 --- a/tests/CodeyBox.Tests/RecoveryCancellationPipelineTests.cs +++ b/tests/CodeyBox.Tests/RecoveryCancellationPipelineTests.cs @@ -181,6 +181,7 @@ public async Task OperatorCancel_RowAdvancedBetweenSnapshotAndWrite_DoesNotOverw var seed = await TestSupport.CreateSeedRepoAsync(_workspace); using var registry = new CancellationRegistry(CancellationToken.None); var webhooks = new RecordingWebhookDispatcher(); + var agent = new BlockingAgentRunner(); // Inject a one-shot race ONLY when the work item is in Working state // (the typical state when the cancel handler runs) and the race has been // armed via SetArmed(true). The wrapper returns the pre-race snapshot @@ -191,7 +192,7 @@ public async Task OperatorCancel_RowAdvancedBetweenSnapshotAndWrite_DoesNotOverw using var harness = BuildPipeline( seed, - new BlockingAgentRunner(), + agent, registry, webhooks, storeDecorator: raceFactory); @@ -208,6 +209,7 @@ public async Task OperatorCancel_RowAdvancedBetweenSnapshotAndWrite_DoesNotOverw harness.Pipeline.RunAsync(item, registration.Token, hostShutdownCts.Token)); await WaitForStateAsync(harness.Store, item.Id, WorkItemState.Working, TimeSpan.FromSeconds(30)); + await agent.Started.Task.WaitAsync(TimeSpan.FromSeconds(30)); // Arm the race: the NEXT GetAsync that returns a Working row (the one // inside HandleOperatorCancelAsync) will silently advance the persisted @@ -247,11 +249,12 @@ public async Task OperatorCancel_RowRecoveredBeforeCancelHandlerRead_DoesNotOver var seed = await TestSupport.CreateSeedRepoAsync(_workspace); using var registry = new CancellationRegistry(CancellationToken.None); var webhooks = new RecordingWebhookDispatcher(); + var agent = new BlockingAgentRunner(); var raceFactory = (SqliteWorkItemStore inner) => new RaceAdvancingStore(inner); using var harness = BuildPipeline( seed, - new BlockingAgentRunner(), + agent, registry, webhooks, storeDecorator: raceFactory); @@ -268,6 +271,7 @@ public async Task OperatorCancel_RowRecoveredBeforeCancelHandlerRead_DoesNotOver harness.Pipeline.RunAsync(item, registration.Token, hostShutdownCts.Token)); await WaitForStateAsync(harness.Store, item.Id, WorkItemState.Working, TimeSpan.FromSeconds(30)); + await agent.Started.Task.WaitAsync(TimeSpan.FromSeconds(30)); raceStore.ArmRace(); var staleSnapshot = await raceStore.GetAsync(item.Id); @@ -544,6 +548,8 @@ internal sealed class RaceAdvancingStore : IWorkItemStore public Task UpdateAsync(WorkItem item, CancellationToken ct = default) => _inner.UpdateAsync(item, ct); public Task TryUpdateIfStateAsync(WorkItem item, WorkItemState onlyIfState, CancellationToken ct = default) => _inner.TryUpdateIfStateAsync(item, onlyIfState, ct); + public Task TryUpdateIfStateAndUpdatedAtAsync(WorkItem item, WorkItemState onlyIfState, DateTimeOffset onlyIfUpdatedAt, CancellationToken ct = default) => + _inner.TryUpdateIfStateAndUpdatedAtAsync(item, onlyIfState, onlyIfUpdatedAt, ct); public Task UpdatePriorityAsync(WorkItemId id, int priority, DateTimeOffset updatedAt, CancellationToken ct = default) => _inner.UpdatePriorityAsync(id, priority, updatedAt, ct); public Task UpdateDependsOnAsync(WorkItemId id, IReadOnlyList dependsOn, DateTimeOffset updatedAt, CancellationToken ct = default) => From 222f11c35649c64f01363a7faee42be34b35067e Mon Sep 17 00:00:00 2001 From: Adam Frisby Date: Sun, 13 Sep 2026 22:48:30 +0000 Subject: [PATCH 4/4] fix: serialize SqliteReleaseStore connection access to prevent cross-thread transaction race Concurrent GetAsync polls raced SaveE2eReplayResultsAsync transactions on the shared SqliteConnection, causing 'transaction object is not associated with the same connection' failures. Adds the per-instance _connectionLock used by sibling stores and wraps every connection use; writes nest the shared write gate inside it, reads take only the connection lock. CodeyBox-Prompt-Revision: 1 Co-Authored-By: CodeyBox --- .../SqliteReleaseStore.cs | 120 +++++++++++++----- 1 file changed, 85 insertions(+), 35 deletions(-) diff --git a/src/CodeyBox.Orchestrator/SqliteReleaseStore.cs b/src/CodeyBox.Orchestrator/SqliteReleaseStore.cs index 890f96df..7ab92cfe 100644 --- a/src/CodeyBox.Orchestrator/SqliteReleaseStore.cs +++ b/src/CodeyBox.Orchestrator/SqliteReleaseStore.cs @@ -14,6 +14,7 @@ namespace CodeyBox.Orchestrator; public sealed class SqliteReleaseStore : IReleaseStore, IDisposable { private readonly SqliteConnection _conn; + private readonly SemaphoreSlim _connectionLock = new(1, 1); private readonly SqliteDatabaseWriteGate _writeLock; private int _disposed; @@ -99,52 +100,68 @@ created_at TEXT NOT NULL public async Task CreateAsync(Release release, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { - using var cmd = _conn.CreateCommand(); - cmd.CommandText = """ - INSERT INTO releases (id, project_id, name, description, state, base_commit_sha, - branch_name, created_at, closed_at, review_started_at, released_at, - failed_reason, target_tag, config_json) - VALUES ($id, $pid, $name, $desc, $state, $sha, $branch, $ca, $closed, $review, $released, - $failed, $tag, $cfg); - """; - Bind(cmd, release); - await cmd.ExecuteNonQueryAsync(ct); + await _writeLock.WaitAsync(ct); + try + { + using var cmd = _conn.CreateCommand(); + cmd.CommandText = """ + INSERT INTO releases (id, project_id, name, description, state, base_commit_sha, + branch_name, created_at, closed_at, review_started_at, released_at, + failed_reason, target_tag, config_json) + VALUES ($id, $pid, $name, $desc, $state, $sha, $branch, $ca, $closed, $review, $released, + $failed, $tag, $cfg); + """; + Bind(cmd, release); + await cmd.ExecuteNonQueryAsync(ct); + } + finally + { + _writeLock.Release(); + } } finally { - _writeLock.Release(); + _connectionLock.Release(); } } public async Task UpdateAsync(Release release, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { - using var cmd = _conn.CreateCommand(); - cmd.CommandText = """ - UPDATE releases SET - project_id = $pid, name = $name, description = $desc, state = $state, - base_commit_sha = $sha, branch_name = $branch, - closed_at = $closed, review_started_at = $review, released_at = $released, - failed_reason = $failed, target_tag = $tag, config_json = $cfg - WHERE id = $id; - """; - Bind(cmd, release); - await cmd.ExecuteNonQueryAsync(ct); + await _writeLock.WaitAsync(ct); + try + { + using var cmd = _conn.CreateCommand(); + cmd.CommandText = """ + UPDATE releases SET + project_id = $pid, name = $name, description = $desc, state = $state, + base_commit_sha = $sha, branch_name = $branch, + closed_at = $closed, review_started_at = $review, released_at = $released, + failed_reason = $failed, target_tag = $tag, config_json = $cfg + WHERE id = $id; + """; + Bind(cmd, release); + await cmd.ExecuteNonQueryAsync(ct); + } + finally + { + _writeLock.Release(); + } } finally { - _writeLock.Release(); + _connectionLock.Release(); } } public async Task GetAsync(ReleaseId id, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { using var cmd = _conn.CreateCommand(); @@ -155,13 +172,13 @@ UPDATE releases SET } finally { - _writeLock.Release(); + _connectionLock.Release(); } } public async Task GetByNameAsync(ProjectId projectId, string name, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { using var cmd = _conn.CreateCommand(); @@ -173,7 +190,7 @@ UPDATE releases SET } finally { - _writeLock.Release(); + _connectionLock.Release(); } } @@ -184,7 +201,7 @@ public async Task> ListAsync( int? offset = null, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { using var cmd = _conn.CreateCommand(); @@ -221,13 +238,16 @@ public async Task> ListAsync( } finally { - _writeLock.Release(); + _connectionLock.Release(); } } /// public async Task TrySetBranchAsync(ReleaseId id, string branchName, string baseCommitSha, CancellationToken ct = default) { + await _connectionLock.WaitAsync(ct); + try + { await _writeLock.WaitAsync(ct); try { @@ -245,10 +265,18 @@ public async Task TrySetBranchAsync(ReleaseId id, string branchName, strin { _writeLock.Release(); } + } + finally + { + _connectionLock.Release(); + } } public async Task TryTransitionStateAsync(Release release, ReleaseState expectedCurrentState, CancellationToken ct = default) { + await _connectionLock.WaitAsync(ct); + try + { await _writeLock.WaitAsync(ct); try { @@ -269,11 +297,19 @@ UPDATE releases SET { _writeLock.Release(); } + } + finally + { + _connectionLock.Release(); + } } public async Task SaveAuditIterationAsync(ReleaseAuditIteration iteration, CancellationToken ct = default) { var findingsJson = JsonSerializer.Serialize(iteration.Findings, _findingsSerializerOptions); + await _connectionLock.WaitAsync(ct); + try + { await _writeLock.WaitAsync(ct); try { @@ -298,11 +334,16 @@ INSERT OR IGNORE INTO release_audit_iterations { _writeLock.Release(); } + } + finally + { + _connectionLock.Release(); + } } public async Task> ListAuditIterationsAsync(ReleaseId releaseId, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { using var cmd = _conn.CreateCommand(); @@ -334,7 +375,7 @@ public async Task> ListAuditIterationsAsync } finally { - _writeLock.Release(); + _connectionLock.Release(); } } @@ -342,6 +383,9 @@ public async Task SaveE2eReplayResultsAsync(ReleaseId releaseId, int iteration, { if (results.Count == 0) return; + await _connectionLock.WaitAsync(ct); + try + { await _writeLock.WaitAsync(ct); try { @@ -387,11 +431,16 @@ INSERT INTO release_e2e_results ( { _writeLock.Release(); } + } + finally + { + _connectionLock.Release(); + } } public async Task> ListE2eReplayResultsAsync(ReleaseId releaseId, int? iteration = null, CancellationToken ct = default) { - await _writeLock.WaitAsync(ct); + await _connectionLock.WaitAsync(ct); try { using var cmd = _conn.CreateCommand(); @@ -429,7 +478,7 @@ public async Task> ListE2eReplayResultsAsy } finally { - _writeLock.Release(); + _connectionLock.Release(); } } @@ -458,6 +507,7 @@ public void Dispose() } finally { + _connectionLock.Dispose(); _writeLock.Dispose(); } }