From a85a6788b484d4214a0e8e46c67eb4a2bdacb0cc Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 5 Nov 2026 12:38:10 +0000 Subject: [PATCH] Record the first six remediation assessments --- docs/roadmap/planning-assessment.md | 239 +++++++++++++++++++++++++++- 1 file changed, 238 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/planning-assessment.md b/docs/roadmap/planning-assessment.md index e5fc559..f7102c3 100644 --- a/docs/roadmap/planning-assessment.md +++ b/docs/roadmap/planning-assessment.md @@ -12,7 +12,9 @@ The [catalog's six accepted decisions](audit/catalog.md#resolved-decisions) remain authoritative. This assessment selects ways to implement those decisions; it does not reopen them or repeat the repository-wide audits. Catalog numbers below identify the relevant findings, not additional implementation stages. -All assessment topics are initially pending. +Assessment started on 2026-11-05 at clean commit +`21fea46fb3b0ce9483db2399c9587f4a2323d687`. Assessment records below distinguish +completed analysis from still-pending implementation. ## Assessment Method And Outputs @@ -75,6 +77,32 @@ work to schedule, not permission to install one during assessment or to stop independent source analysis. Retain the accepted separation between maintainer selection and consumer minimum; do not add consumer advisories or scan gates. +### Assessment Record + +**Status:** Assessed; no environment installation is needed. + +**Evidence:** Default `/snap/bin/go` reports Go 1.27.0, but the existing module +cache contains executable Go 1.27.3 at +`/home/eric/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.27.3.linux-amd64/bin/go`; +invoking it reports `go1.27.3 linux/amd64`. Other inspected SDK/cache installs +are older. `go.mod` declares 1.27.0, ConfigKit 1.1.0, jsonschema/v6 6.0.2, and +ReleaseKit 0.9.0. `scripts/releasekit.sh` sets `GOTOOLCHAIN=local`, so its +toolchain is selected by `PATH`, not automatically upgraded. + +**Recommendation and alternatives:** Put the existing 1.27.3 binary directory +first on `PATH`, with `GOTOOLCHAIN=local` and `GOWORK=off`, for subsequent +assessment commands and implementation validation. The authoritative affected +ranges for [GO-2026-6611](https://pkg.go.dev/vuln/GO-2026-6611) and +[GO-2026-6809](https://pkg.go.dev/vuln/GO-2026-6809) exclude 1.27.3: the two +specific audited exposures are addressed by that selection. This is a focused +version/applicability reassessment, not a new full vulnerability scan or a claim +about consumer binaries. Continuing with default 1.27.0 retains the identified +exposure and is not recommended; installation is unnecessary. + +**Plan handoff:** Clarify ordinary development toolchain selection in +`docs/development.md`; leave `go 1.27.0` unchanged. Record the actually selected +patch when validating remediation. No consumer advisory or recurring scan gate. + ## 2. Specify Direct ConfigKit Source Handoff **Catalog:** 3, 5; informs 24. @@ -92,6 +120,38 @@ a replacement converter, pre-validation pass, or upstream feature. Existing upstream ownership evidence is sufficient; exhaustive ConfigKit limit testing is not an assessment deliverable. +### Assessment Record + +**Status:** Assessed; remove the compatibility adapter without replacement. + +**Evidence:** `ProfileSource` (`profiles.go:29`) and `BackendSource` +(`backends.go:82`) already project optional typed fields into built-in values +and wrap definitions in `[]any`. Their only recursive compatibility dependency +is `providerMapForMapping(config.ExtraParams)`. The pinned ConfigKit 1.1.0 +`source.go:Mapping` promises immediate capture and deferred unsupported/cyclic +diagnostics. Its `internal/engine/source.go:Mapping` builds an owned document +and retains `mapIssue`; `readSource` reports it when consumed. PromptKit's +`provider_values.go` instead expands maps/slices before that bounded handoff +and parses `json.Number` through a lossy float fallback. Existing +`sources_contract_test.go` mainly protects flat-map ownership, not this broader +input boundary. + +**Recommendation and alternatives:** Assign `config.ExtraParams` directly into +the ordinary fields map in both helpers, then call `configkit.Mapping` as now. +Delete `provider_values.go` after its two callers are replaced. A shallow map +copy adds no capture protection beyond Mapping; another recursive converter or +raw validation would retain the responsibility being removed. Runtime overrides +continue using their separate JSON-value boundary. + +**Plan handoff:** Cover both helpers with nested built-in maps/`[]any`, mutation +after source creation, and deferred failures for representative `json.Number`, +named scalar/typed container, pointer, and cyclic inputs. Keep construction +order/optional-field coverage. Explain caller conversion to supported forms: +for example, `[]string` becomes `[]any` of strings; callers choose and check +numeric conversion themselves. Do not assert preserved numeric spelling or +duplicate ConfigKit's entire resource-limit suite. Source-helper GoDoc and +migration notes own the changed accepted-value boundary. + ## 3. Specify Self-Contained Schema Resolution And Compilation **Catalog:** 6, 15, 21; coordinates with 32. @@ -116,6 +176,55 @@ direct-validation cases. Specify how the provider receives the self-contained document unchanged in meaning. No bundler, schema rewriting prototype, or live provider compatibility experiment is needed. +### Assessment Record + +**Status:** Assessed; use explicit root registration and a no-external-resource +loader, with coverage for unused schema definitions. + +**Evidence:** `internal/validate/standard_validator.go` has four compilation +paths; its root loaders already preserve JSON numbers, validate the 2020-12 +dialect, and check source containment/context. `Runner.prepareValidation` uses +only `ValidationPreparer`, despite storing `Validator`. In pinned jsonschema +6.0.2, `Compiler.AddResource` registers the decoded document, `roots.go` collects +embedded IDs/anchors, and `loader.go` consults registered resources and bundled +metaschemas before the custom loader. + +An offline dependency-contract probe in +`/tmp/promptkit-assessment-aqn3B8/schema.go`, run with Go 1.27.3 using +`go run -mod=readonly` and `GOPROXY=off`, registered one document and installed a +loader that only counts/rejects external loads. Fragment, anchor, recursion, +absolute self-ID, embedded child-ID, and standard metaschema cases compiled +with zero external loads; a real external reference failed with one load. +Crucially, an unused `$defs` entry with an external reference did not trigger a +load until compiled explicitly. An example payload containing a literal `$ref` +did not trigger loading. A denying loader alone therefore does not establish +that every declared schema is self-contained. + +**Recommendation and alternatives:** Retain source-specific root loading, then +share `compileSchemaDocument(ctx, resourceURL, document)` returning the compiled +executor. Register the root, retain Draft2020, and install a context-aware loader +returning an actionable external-resource error without filesystem/network I/O. +Keep compiler resolution authoritative for IDs and anchors. To cover otherwise +unused declarations, enumerate schema locations using a small, explicit table +of the pinned 2020-12 schema-bearing keyword positions (including its supported +legacy positions) and compile those locations with the same compiler/cache. +Do not traverse annotation/example objects as schemas. This bounded policy walk +is not a URI resolver, bundler, or general schema framework. A lexical ban on +non-fragment references rejects embedded IDs; root-only compilation misses the +demonstrated unused-definition case. Neither is recommended. + +**Plan handoff:** Specify escaped JSON-pointer location enumeration and retained +embedded-ID/dynamic-reference semantics in focused cases; preserve dialect and +number handling. Both direct schema callbacks call the shared compilation only +after instance decoding, while prepared paths call it eagerly as before. Remove +external loaders, not root path containment/read helpers. Change runner fields +and constructor parameters to `validate.ValidationPreparer`, retaining the nil +no-op and nil-plan error behavior. Validator constructors can return their +concrete pointer types so existing direct `Validate` callers and preparation +both compile; no public facade signature changes are needed. Replace external +resource success fixtures with early-error cases, retain independent root-source +tests, and add public `WithSchemaFile` option/replacement/error coverage. + ## 4. Specify Ownership And Retention Remedies **Catalog:** 4, 16, 18. @@ -133,6 +242,37 @@ allocations comparing unequal. Identify the session-string detachment point. List relevant concurrent and mutation regressions for implementation, without building a new ownership framework or changing lifetimes during assessment. +### Assessment Record + +**Status:** Assessed; narrow references rather than adding lifecycle machinery. + +**Evidence:** `artifact_reader.go:publicArtifactReaderAdapter.Read` copies the +returned body only after the consumer call returns. `PreparedExecution` in +`internal/usecase/prepared_execution.go` retains `owner *Runner` even after its +mutex-protected claim/discard drops the payload; `Details` uses separately +captured data. `Runner` owns repositories, clients, and admission. Namespace +views delegate to the same internal engine/runner. `NormalizeSessionID` in +`internal/domain/session.go` returns `strings.TrimSpace(raw)` directly. + +**Recommendation and alternatives:** Document successful artifact return as +ownership transfer, as accepted, and keep adapter copying. Replace the prepared +handle's runner pointer with a pointer to a non-zero-sized private identity +object allocated once by each runner constructor and copied into its handles. +Keep that immutable token after claim/discard; it retains no runner graph and +preserves foreign-owner validation order without a new owner-field race. +Clearing a mutable runner pointer is viable but complicates synchronized checks +and error ordering unnecessarily. Clone the validated nonempty normalized +session string with `strings.Clone` before returning it; this detaches padded +input without imposing a new limit. + +**Plan handoff:** Retain ready-to-claimed/discarded transitions under the current +mutex, private payload clearing, one-shot execution, and detached details. +Protect cross-engine rejection, same-engine namespace views, concurrent +claim/discard, and details after discard/use. Artifact regressions must use +independently returned buffers and avoid claiming safety for contract-violating +readers. Existing mutation, trimming, Unicode, hash, and wire assertions remain +the semantic oracle; a GC timing assertion is unnecessary. + ## 5. Specify Error Ownership And Preparation-Stage Information **Catalog:** 7, 8, 9; informs 25. @@ -150,6 +290,57 @@ boundaries where possible. Give a small expected-outcome matrix of public category, retained cause, diagnostic completion, and dispatch behavior. Retained causes may match multiple sentinels; category exclusivity is not the target. +### Assessment Record + +**Status:** Assessed; carry ownership and preparation provenance explicitly. + +**Evidence:** `errors.go:publicErrorFor` uses whole-chain `errors.Is` in a fixed +order; `mapPublicError` also searches nested capacity/exhaustion/provider types +before normal mapping. `wrapGenerationError` and artifact loading add operation +sentinels but no separately readable owning category. In +`internal/llm/openai_compatible_client.go:decodeOpenAIChatResponse`, both decoder +calls replace non-context errors with malformed-response messages. In +`generation.Coordinator.generate`, `PrepareGeneration` fails before any capacity +attempt, while `SmokeTester.runInference` concludes every generation error as +report data. `GenerationEvidence` currently lacks preparation-stage information. + +**Recommendation and alternatives:** Add a small internal use-case operation +error carrying category and cause, with `Unwrap`/`Is` behavior. Apply it at owning +boundaries and let facade mapping select that category independently of nested +causes. Preserve legitimate direct not-found mappings when assigning the +category. Keep specialized public capacity/provider/exhaustion projections, but +do not let their presence inside a collaborator cause erase the outer owning +category. A globally reordered sentinel switch cannot express this distinction. + +Add `PreparationFailed bool` to internal `GenerationEvidence`, set only at the +built-in `PrepareGeneration` failure return. Diagnostic orchestration checks it +after caller cancellation and returns `ErrInvalidRequest` with the cause. +Do not infer this stage from error identity or attempt count; injected failures +and intentional missing-key/capacity reports keep their established semantics. +This needs no new public field or retry policy. + +Wrap the successful-response reader with a small read-error recorder beneath +the existing limited reader. At decoding/EOF verification failure, prefer an +observed context error; preserve size overflow as malformed; otherwise propagate +an actual non-EOF reader failure through `requestFailedError`. Parser-generated +syntax/type/truncation failures, EOF-truncated JSON, and actual extra values +remain malformed. Distinguish errors returned by the reader from those created +by `encoding/json`, rather than categorizing every `io.ErrUnexpectedEOF` alike. +Retain redacted default formatting and exact underlying read causes. + +**Plan handoff:** Protect these expected outcomes at the public boundary: + +| Case | Owning category / result | Retained evidence | +| --- | --- | --- | +| Client returns child missing-prompt error | `ErrLLMGenerate`; error | Child cause remains matchable, including its category. | +| Reader returns the same child error | `ErrArtifactLoad`; error | Child cause remains matchable. | +| Direct missing prompt/profile | Existing not-found category; error | No conversion into a generic load failure. | +| 2xx body read fails during decode or final EOF check | Generation plus transport; diagnostic transport failure | Original reader cause, no raw body/detail formatting. | +| Invalid/truncated JSON, extra value, size overflow | Existing malformed-response behavior | No new retry or parser-detail disclosure. | +| Built-in diagnostic preparation fails | `ErrInvalidRequest`; not a completed diagnostic | Zero dispatch and original preparation cause. | +| Missing required credential/capacity report | Existing completed diagnostic outcome | No inferred construction failure. | +| Caller cancellation | Existing cancellation/error handling | Cancellation takes precedence over publishing success. | + ## 6. Specify Authentication Overflow Reporting And Adapter Ownership **Catalog:** 2, 26, 27; uses topic 5's error/stage conclusions. @@ -172,6 +363,52 @@ Recommend the outbound protocol-adapter seam and narrow target-copy operation. Keep the body-bound fix independently implementable before adapter relocation; do not add a public budget option or general diagnostic plugin framework. +### Assessment Record + +**Status:** Assessed; one reason constant suffices, without a new report field. + +**Evidence:** Public/domain smoke reports already have `Reason`, `Failure`, +nullable `CredentialsAccepted`, HTTP status on attempts, and timing. Current +`smoke_authentication.go` reads a body only to recognize HTTP 200; 401 establishes +rejection without reading it. Other statuses retain their existing outcome. +`smoke_tester.go` copies these facts into attempts and `finishHTTP` handles +caller cancellation. The outbound client already owns authenticated GET and +HTTP evidence. The broad domain request/report cloners are used only by their +self-tests and a target-only copy respectively, as recorded in audit 11. + +**Recommendation and alternatives:** Add matching public/domain +`SmokeTestReasonAuthenticationResponseLimitExceeded` constants with value +`authentication_response_limit_exceeded`. On a 200 recognition overflow set that +reason, existing `ExecutionFailureOther`, acceptance nil, and retain HTTP/timing +facts; conclude with a nil Go error unless caller cancellation applies. A new +general failure enum or report Boolean is unnecessary: the existing reason +already provides an explicit machine-readable diagnostic. Keep 401 rejection +and non-200 behavior independent of unused response bodies. + +Read at most allowance plus one byte with `io.LimitReader`/`io.ReadAll`, detect +length greater than 1 MiB before JSON recognition, and close without draining. +Do not trust `ContentLength`. Existing bounded bytes plus one-byte lookahead are +sufficient; streaming the small account parser adds complexity without need. +Cancellation takes its existing precedence; overflow is not malformed content. + +Move account-response recognition and bounded response ownership into an +`internal/llm` OpenRouter authentication operation, returning HTTP evidence, +optional acceptance, overflow/unrecognized indicators, and operational error. +Move default endpoint derivation into a small llm adapter helper used by +selection; explicit overrides remain unchanged. Use-case orchestration maps +those factual results to report reasons/completion and consumes a narrow +authentication capability alongside reachability HTTP capability. No reverse +dependency or plugin registry is needed. Replace target-only full-report cloning +with direct target/provider-map detachment; retain facade projection and active +backend cloners. + +**Plan handoff:** Fix and test the byte bound before relocation. Include exact +limit versus limit-plus-one, escaping/invalid bodies, inaccurate declared length, +closure, 401 without body consumption, context interruption, redaction, and +detached report values. Update reason GoDoc/public conversion and focused +diagnostic documentation in the same implementation unit. No secret-bearing +body, header, or provider detail is added to report data. + ## 7. Assess JSON Copying Compatibility And Provider Overlay Cost **Catalog:** 19, 24; uses topic 2's converter-removal boundary.