Files
promptkit/docs/roadmap/remediation.md

167 lines
9.1 KiB
Markdown
Raw Normal View History

# Conversation Continuation Review Remediation
## Review Scope
Reviewed all 13 stages of the [implementation plan](implementation.md) against
the [accepted target](conversation-continuation.md), implementation, tests, and
documentation. Baseline: `5c135c5`, including the implementation commits after
`5cc3fa0`.
The implementation retains the intended boundaries: immutable parent-linked
history, ordinary preparation and execution for continuations, a separate
sequential coordinator, and explicitly selected observer-based groups. No runtime
correctness defect or inadvertently strengthened restriction was identified.
The initial finding concerns the durable public documentation in Stage 13.
Validation completed with the locally installed Go 1.27.3 toolchain,
`GOTOOLCHAIN=local`, `GOWORK=off`, and a writable temporary build cache:
- `go test -race ./...` passed, including the maintained examples.
- `go vet ./...` passed.
- `git diff --check` passed.
### Algorithmic Follow-Up
A subsequent static audit of all 13 stages, at the same implementation baseline,
identified the two efficiency opportunities below. No additional ordinary
correctness bug or avoidable quadratic-time algorithm was found. No real-world
performance data was collected.
The planned algorithms remain fixed: suffix-only immutable capture, iterative
history flattening, sequential coordination, synchronous observer delivery, and
map-based incremental group aggregation. The findings address extra allocation
and copying around those choices, not replacements for them. Complexity statements
describe implementation costs, not new public performance guarantees.
For orientation, capture is linear in the new turn's message count; flattening is
linear in retained message count; coordinator bookkeeping is constant per turn,
excluding application callbacks and ordinary execution. Group updates are
expected constant time per event for fixed-length run identities, with linear
membership storage and linear full snapshots. An observer dispatch is linear in
its selected callbacks. These implementations do not rescan all prior turns or
group members on each incremental update.
## 1. Complete The Public Exchange Capture And Continuation Contract
**Priority:** P2. **Status:** Complete. **Plan stage:** 13.
**Completion:** Expanded the public request, exchange, details, and prepared-handle
GoDoc and linked the consumer walkthrough to those owners. Checked comments
against capture, preparation, and repair behavior; local links and diff verified.
**Locations:** [request fields](../../types.go),
[exchange declarations and accessor](../../exchange.go), and
[consumer guidance](../consumers/pkg-promptkit.md#capture-and-continue-a-conversation).
The `CaptureExchange` and `ContinueFrom` GoDoc currently provides only a short
description of opt-in capture, invalid handles, and fresh follow-up settings.
The `Exchange` and `Details` comments describe reuse and detached history, but do
not define its exact contents or explain that inspection uses only retained
state. Important selected semantics remain documented only in temporary roadmaps
or prose/internal guidance:
- History contains the effective original request and exact final assistant
content, preserving roles and cache metadata; intermediate structural-repair
candidates and corrective instructions are excluded.
- Continuation assembles captured history, rendered follow-up definition
messages, and caller-appended messages in that order, without re-rendering or
reading the original inputs. It adds no history-size limit or truncation.
- `PrepareExecution` retains capture selection for execution of its snapshot;
preparation and concrete-request validation alone produce no completed exchange.
- `Details` reads the retained snapshot without rendering, filesystem reads, or
provider calls. Its run identities identify the originating execution and
immediate parent execution.
These behaviors are implemented. The gap is that the exported declarations and
GoDoc do not yet own their complete contract as required by the documentation
policy and the final implementation stage. Retiring the roadmap would otherwise
leave consumers and future maintainers without a complete canonical reference.
**Required remediation:** Document these existing semantics at the appropriate
request fields, exchange type/details, and prepared-execution declarations, using
GoDoc cross-references instead of repeating the same contract across declarations.
Link the new consumer walkthrough to those public owners. Preserve the selected
scope: do not add persistence, limits, stronger serialization guarantees, or
private storage/allocation promises while documenting the behavior.
**Verification:** Check the comments against the existing capture, continuation,
prepared-execution, ownership, and repair tests and implementation. Validate the
affected documentation links and diff. This is documentation remediation; it
does not require changing runtime behavior or duplicating existing tests.
## 2. Avoid Intermediate History Copies During Assembly And Inspection
**Priority:** P3. **Status:** For consideration. **Plan stages:** 2 and 5.
**Locations:** `completePreparationWithStructuredOutput` in
[the runner](../../internal/usecase/runner.go), `Exchange.Messages` in
[history storage](../../internal/domain/exchange.go), and
[`Exchange.Details`](../../exchange.go) with `fromDomainRenderedMessages` in
[facade conversion](../../convert.go).
Let H be the captured history's message count and S the newly rendered and
caller-appended message count. Continuation preparation currently:
1. allocates and copies S messages into the new suffix;
2. materializes H history messages into a detached intermediate slice; and
3. allocates the final H + S slice and copies both intermediate slices again.
Thus assembly copies 2H + 2S message entries, rather than the H + S needed to
populate the final execution-owned buffer. Each pass also clones non-nil
cache-control structs. The public `Details` path similarly materializes a
detached domain history and then traverses it again to allocate public messages
and clone cache controls for a second time. Strings share immutable content;
these are message-header and cache-struct copies, not duplicate text-byte buffers.
**Complexity:** Current and proposed assembly are both Θ(H + S) in time and
auxiliary space; details remain Θ(H). The opportunity removes an extra linear
pass and temporary allocations, not an asymptotic quadratic defect. Successive
full-history requests still entail work proportional to the sum of their history
lengths. This audit does not propose truncation or a different
conversation protocol.
**Potential refactoring:** Add an internal fill/flatten helper that writes history
and the new message parts directly into one correctly sized final domain slice.
For inspection, consider an internal indexed traversal/projection helper that
fills the public destination without first constructing a complete domain slice.
Keep the immutable parent-linked representation, separate public types, and
detachment of every exposed mutable cache-control value. Prioritize the simple
assembly change; avoid adding a general projection framework solely to eliminate
the inspection intermediate.
**Verification:** Reuse history ordering, cache-control mutation, branching,
prepared-execution, and mutating-client tests. Review the copy/allocation paths
statically. Do not add public allocation guarantees or exact-allocation tests.
## 3. Size Observer Storage For Selected Callbacks
**Priority:** P3. **Status:** For consideration. **Plan stage:** 9.
**Location:** [`Compose`](../../internal/observation/compose.go).
`Compose` allocates `selected` with capacity equal to every supplied observer
slot before filtering nil callbacks. The returned closure retains that backing
array even when only a small subset of slots survives. An all-nil input also
allocates the full temporary array before returning nil.
Let K be the number of supplied slots and A the number of non-nil callbacks,
including intentionally repeated callbacks. Construction is Θ(K) time with
Θ(K) allocated storage, and a nonempty composition retains Θ(K) storage.
Dispatch is already Θ(A) and does not need changing. Selected-function storage
can instead be Θ(A), with constant storage for A = 0, while preserving Θ(K)
construction time.
**Potential refactoring:** Count non-nil callbacks first, then allocate exactly A
slots and fill them in order. This trades a second linear scan for avoiding
storage proportional to disabled slots. Alternatively, append into an initially
nil slice for a simpler one-pass filter with amortized Θ(K) construction and
Θ(A) retained storage, accepting growth allocations for dense inputs. A
zero/single-callback fast path can also avoid an unnecessary fan-out closure.
The usual coordinator supplies only two slots, so its benefit is small; this is
a low-priority general-helper scalability opportunity, not a release blocker.
**Verification:** Preserve copied function values, nil skipping, duplicate
delivery, supplied order, and ordinary panic propagation. Existing composition
and prepared-observer ownership tests protect those contracts; no benchmark or
new performance gate is required.