From ad85d71b0f7c77246215e32369e7252509131694 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 8 Aug 2026 21:47:51 +0000 Subject: [PATCH] Audit LLM runtime and prompt assets --- docs/roadmap/audit.md | 244 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 241 insertions(+), 3 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index ef5525d..35eb962 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -20,9 +20,10 @@ change only roadmap audit documents do not change that production target. Pending final synthesis. The initial baseline is healthy. The architecture, configuration/CLI, pipeline composition, reference/handoff, runtime, and state -reviews have found two High findings, four Medium findings, and ten Low +reviews have found three High findings, four Medium findings, and thirteen Low findings, with no production dependency inversion, unbounded framework worker -pool, completion-order-dependent result assembly, or debug-to-cache coupling. +pool, completion-order-dependent result assembly, debug-to-cache coupling, or +model-visible credential material in the embedded LLM assets. ## Finding Index @@ -45,6 +46,10 @@ Final cross-area ordering is pending synthesis. | STATE-001 | Medium | Correctness | Preserve distinct identities in state paths | | STATE-002 | Low | Efficiency | Remove redundant post-decode clones from canonical codecs | | STATE-003 | Low | Duplication | Publish output through the confined file writer | +| LLM-001 | High | Correctness | Keep raw provider errors inside the adapter | +| LLM-002 | Low | Correctness | Recheck cancellation after scheduler admission | +| LLM-003 | Low | Correctness | Reject duplicate virtual prompt names | +| LLM-004 | Low | Duplication | Share the read-only in-memory filesystem mechanics | ## Findings @@ -566,6 +571,148 @@ Final cross-area ordering is pending synthesis. rename failure. - **Grouping:** Independent. +### LLM Runtime, Prompt Filesystems, And Assets + +### LLM-001 — Keep raw provider errors inside the adapter + +- **Severity:** High +- **Category:** Correctness +- **Evidence:** Preparation and ordinary execution failures wrap + `redactPromptKitError` with `%w` at + `internal/framework/llm/promptkit_client.go:140`–`145` and 149–172. The + returned `redactedProviderError` replaces bearer-token text only in its + `Error` method, then exposes the original upstream error unchanged through + `Unwrap` at lines 424–442. The outer error therefore prints a redacted + diagnostic, but `errors.Unwrap`, `errors.Is`, or `errors.As` can recover the + raw PromptKit/provider error, including its original text and concrete type. + This contradicts the adapter contract in `docs/internal/llm.md:177`–`190` + and 231–236, which requires provider-specific errors and credentials to stay + behind the provider-neutral boundary. Capacity exhaustion avoids this + particular chain by mapping the PromptKit sentinel explicitly and formatting + the sanitized diagnostic with `%v`. +- **Impact:** Any framework collaborator, logger, test helper, or future error + serializer that walks the standard error chain can disclose a bearer + credential that normal string formatting appeared to redact. It can also + couple application code to PromptKit/provider error types despite the + documented neutral transport boundary. +- **Recommendation:** Make sanitized upstream diagnostics opaque: do not + implement `Unwrap` and do not wrap a provider error directly after crossing + the adapter boundary. Classify context cancellation, capacity, invalid + structured output, and any other intentionally supported neutral categories + before sanitization, then expose only the corresponding contracts sentinel + plus credential-redacted text. +- **Preserve:** Keep caller-context precedence, prompt context, the + provider-neutral capacity and invalid-output sentinels, full internal + PromptKit error inspection before adaptation, and useful redacted operational + diagnostics. +- **Validation:** Return a typed upstream error containing a bearer secret from + both preparation and generation. Assert that every reachable error-chain + level and formatted form omits the secret, `errors.As` cannot recover the + upstream type, and `errors.Is` still recognizes only the documented context, + capacity, and invalid-output categories. +- **Grouping:** Independent. + +### LLM-002 — Recheck cancellation after scheduler admission + +- **Severity:** Low +- **Category:** Correctness +- **Evidence:** A queued scheduler waiter selects between its closed `ready` + channel and `ctx.Done()` at `internal/framework/llm/scheduler.go:50`–`73`. + `grantQueuedLocked` marks the waiter granted and closes `ready` at lines + 103–111. If cancellation and that grant become observable together, Go may + choose the ready case, so `Acquire` returns a permit even though the context + is already canceled. `Scheduler.Run` then invokes `fn(ctx)` immediately at + lines 77–86 without another cancellation check. The focused queued- + cancellation test cancels before releasing the existing permit and therefore + does not exercise the simultaneous grant/cancel branch. +- **Impact:** A scheduler user that does not independently reject an already- + canceled context can begin backend work after the caller canceled while it + was queued. The production PromptKit path receives the canceled context and + ordinarily stops downstream work, and the deferred release prevents a permit + leak, which limits current severity; the scheduler's own cancellation + contract is nevertheless incomplete. +- **Recommendation:** After admission and before dispatch, recheck `ctx.Err()` + while retaining the deferred permit release. Keep `Acquire`'s locked + grant-versus-remove accounting as the owner of queue state; the extra gate + should only prevent the admitted callback from starting. +- **Preserve:** Retain one process-wide FIFO queue, the fixed concurrency + bound, idempotent release functions, cancellation removal for still-queued + waiters, and permit transfer/release after all errors. +- **Validation:** Add a focused grant/cancel race case with a callback that + records invocation and intentionally ignores its context. Prove that an + already-canceled admitted request returns `context.Canceled`, never invokes + the callback, releases its permit, and allows the next FIFO waiter to run; + retain the concurrency, cancellation, and idempotent-release tests under + `-race`. +- **Grouping:** Independent; this is the provider scheduler boundary rather + than RUN-001's pipeline module-dispatch boundary. + +### LLM-003 — Reject duplicate virtual prompt names + +- **Severity:** Low +- **Category:** Correctness +- **Evidence:** `promptfs.ModulePromptFS` validates and reads every declared + module file, then assigns its bytes directly into a map at + `internal/framework/promptfs/prompt_fs.go:40`–`58`; shared files use the same + direct assignment at lines 60–80. Two names that are equal after trimming, + leading-slash removal, and cleaning therefore select the same virtual path, + and the later declaration silently replaces the earlier bytes. The asset + registry rejects duplicate flattened roots, but the module manifest boundary + has no equivalent check or focused duplicate test. All fourteen current D&D + manifests declare unique normalized names, so no maintained asset is + presently shadowed. +- **Impact:** A future or programmatically supplied prompt manifest can hash, + register, and execute successfully while using different prompt or shared + fragment bytes than one of its declarations implies. List order silently + decides the winner instead of producing the contextual asset-construction + error expected at startup. +- **Recommendation:** Track each cleaned virtual destination while assembling + `ModulePromptFS` and reject a second declaration before overwriting it. Report + whether the collision is module-owned or shared and include only the safe + virtual name, not file content. +- **Preserve:** Keep module and `sharedassets` namespaces distinct, reject + nested virtual names, read only explicitly declared files, return owned + bytes, and retain manifest order for fingerprinting and diagnostics where it + is semantically relevant. +- **Validation:** Add exact and normalization-equivalent duplicate cases for + module files and for shared files backed by different filesystems. Assert + deterministic contextual rejection while distinct module/shared basenames + remain valid; run the package under `-race`. +- **Grouping:** Independent. + +### LLM-004 — Share the read-only in-memory filesystem mechanics + +- **Severity:** Low +- **Category:** Duplication +- **Evidence:** `internal/framework/llm/asset_registry.go:279`–`430` implements + a complete map-backed read-only filesystem—path cleaning, `Open`, `ReadFile`, + `ReadDir`, directory discovery, sorted entries, cursor reads, file metadata, + and modes. `internal/framework/promptfs/prompt_fs.go:84`–`240` independently + repeats the same mechanics with renamed types. The copies have already + drifted: the asset filesystem represents an empty root as an existing empty + directory while the prompt filesystem reports it missing, and directory + `Read` returns a custom error in one implementation versus `io.EOF` in the + other. +- **Impact:** Filesystem-contract fixes and edge-case tests must be duplicated, + while callers receive subtly different behavior from two byte maps assembled + for the same PromptKit engine. The maintained registries are non-empty, so + the observed drift is a maintenance risk rather than a current prompt load + failure. +- **Recommendation:** Extract one narrow framework-owned read-only byte-map + filesystem with standard `io/fs` behavior and independently owned file + handles. Keep asset flattening, duplicate/root validation, schema selection, + and module/shared manifest construction in their existing domain owners. +- **Preserve:** Retain valid-path enforcement, deterministic directory order, + 0444/0555 metadata, immutable source bytes, independent reader offsets, + contextual domain errors before filesystem construction, and the asset + registry's direct owned `ReadFile` result. +- **Validation:** Run `fstest.TestFS` over empty, single-file, and nested trees; + test root and directory `Open`/`ReadDir`/`Read` behavior, file-handle + independence, sorted entries, missing/invalid paths, modes, and mutation + isolation. Retain both asset-registry and `ModulePromptFS` integration tests. +- **Grouping:** Independent; implement after or alongside LLM-003 without + moving manifest collision policy into the generic filesystem. +