From 7a858c2365e1a3625de67a46f5c488e0fe31856b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 22 Sep 2026 04:21:13 +0000 Subject: [PATCH] Plan the public facade refactor --- AGENTS.md | 33 ++++ docs/roadmap/implementation.md | 272 +++++++++++++++++++++++++++++++++ docs/roadmap/public-facade.md | 21 ++- 3 files changed, 315 insertions(+), 11 deletions(-) create mode 100644 docs/roadmap/implementation.md diff --git a/AGENTS.md b/AGENTS.md index 34531ca..68a03c4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,2 +1,35 @@ Please review `docs/development.md` for initial orientation in this repository and follow its task-specific reading guide. + +## Implementation Discipline + +Simplify the mechanism, not the user's choices. + +**Keep implementation machinery proportionate.** Use straightforward, +idiomatic code and introduce only the machinery needed to support the requested +behavior and flexibility. Optimize for conceptual simplicity and +maintainability, not for the fewest lines, fewest options, or narrowest product +surface. + +**Do not invent restrictions.** Do not reject, prohibit, constrain, cap, +harden, or otherwise narrow behavior unless required by the user, an explicit +repository contract, or an unavoidable correctness constraint. Preserve +operator and caller choice where policy has not been established. If an +additional restriction appears advisable, present it as a decision rather than +silently implementing it. + +A correctness constraint must follow from the requested behavior or an +established invariant, not from the convenience of the chosen implementation. +Explain the specific failure it prevents and why a less restrictive +implementation cannot satisfy the contract. + +**An implementation detail is not repository policy.** Tests and documentation +must not elevate incidental behavior into a durable guarantee or restriction +unless that behavior was expressly selected as part of the contract. Existing +explicit contracts remain authoritative; changing or weakening one requires a +deliberate decision rather than being inferred from this principle. + +Agent-authored plans, tests, documentation, and prior implementations are not +evidence that an additional restriction was deliberately approved. Identify the +authorizing requirement or decision; when that authority is unclear, surface +the ambiguity rather than treating repetition as approval. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..db4ee3d --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,272 @@ +# Public Facade Refactor Implementation Plan + +## Scope And Authority + +Implement the [public facade roadmap](public-facade.md) without changing the +consumer API or execution behavior. This plan owns implementation sequence; +the feature roadmap owns the target state and compatibility requirements. +The provisional [profile policy](profile-policy.md) is a separate, subsequent +scope. Do not update catalog dependencies, introduce standard profiles, change +profile stability promises, or prepare a release in these stages. + +Read `AGENTS.md`, the [development guide](../development.md), and all three +policies before starting: [architecture](../policy/architecture.md), +[documentation](../policy/documentation.md), and [testing](../policy/testing.md). +Use existing GoDoc and canonical consumer/format documentation to distinguish +intentional contracts from incidental implementation. If a relocation appears +to require a consumer-facing change, stop and report it rather than silently +changing the contract. Existing exported declarations are not removal candidates +merely because a guide does not mention them. + +Each numbered stage is one implementation prompt. Complete stages in order; +keep the repository building and maintain one active production path after +each stage. Update affected durable documentation when its described ownership +changes, rather than leaving inaccurate descriptions until the final stage. +For each stage, run focused tests while developing and the complete +[maintainer validation workflow](../development.md#maintainer-validation) +before acceptance. Report changes, validation results, and any unresolved +failures. Do not use live providers, credentials, sibling repositories, module +replacements, or a Go workspace to validate the result. + +## Implementation Decisions + +- Add `internal/engine` as the composition owner. It owns engine construction, + defaults, source assembly, catalog loading, registry and capacity construction, + and runner/repairer wiring. It delegates execution to `internal/usecase`; + it does not become a second runner. +- Keep root-defined public types, errors, options, conversions, collaborator + adapters, and formatting. Root `Engine` holds a private `*engine.Engine`. + The internal engine exposes only the domain-level operations needed for + the existing facade methods. No public bridge methods or internal aliases. +- Preserve root `Option` and its sealed `apply(*engineOptions) error` mechanism. + Ultimately, private root `engineOptions` wraps an internal `engine.Options` + value whose private state and setters own source construction and option + accumulation. Root options remain closures applying in caller order; do not + replace them with a deferred list that changes when errors or copies occur. +- Put the memory profile repository in `internal/profile`. Existing domain, + profile validation, and JSON-copy helpers remain the owners of their rules. + Do not duplicate validation in the facade and repository. +- Keep public error classification and redacted public error construction in + the root. Internal construction returns descriptive errors; `NewEngine` + translates construction failures to `ErrInvalidConfig` as it does today. + Runtime errors continue through `mapPublicError` at the public boundary. +- Keep `PreparedExecution` root-defined with its existing private use-case + handle. Pass that internal handle through engine delegation without copying + its execution state or creating another lifecycle owner. Private root + references to specialized internal packages are allowed. +- Do not impose a single internal import on the root, introduce a generic + builder framework, add caching, broaden validation, or migrate all private + helpers merely to reduce root line count. Straightforward public convenience + constructors and boundary conversions may remain where they are. + +## Stage 1: Establish The Compatibility Baseline + +Purpose: make the compatibility review concrete before moving ownership. + +1. Read the root declarations and tests, the + [consumer guide](../consumers/pkg-promptkit.md), + [component overview](../internal/overview.md), + [sources document](../internal/sources.md), and + [runner document](../internal/runner.md). Identify existing contract coverage + for constructor options, profile sources, injection, errors, and prepared + execution. Read their owning internal tests before adding new coverage. +2. Record the pre-refactor commit ID in this plan and capture an exported API + inventory for the final comparison. Include exported declarations, named + types, fields and tags, interface signatures, value and pointer method sets, + constants, error variables, and serialization/formatting contracts. `go doc + -all` is a useful starting point, not a complete method-set comparison. + Temporary comparison output belongs outside tracked source; do not introduce + a permanent snapshot of private structure or freeze every future API addition. +3. Use existing tests first. Add only missing, consequential public regression + coverage for relocation risks: source replacement versus backend accumulation, + option application/capture timing, ownership of injected values and profiles, + nil/zero facade behavior, and typed error translation. Keep public behavior + tests in the external test package where practical. Do not duplicate full + source, transport, capacity, or runner matrices. +4. In particular, inspect the existing contracts around `WithProfiles` copying + during option application and `WithBackend` capturing the queue-capacity + pointer value during option creation. Preserve explicit zero versus unset. + Keep errors from an invalid earlier option observable even if a later option + would replace it. Do not add reflection-based typed-nil rejection or eagerly + read lazy consumer sources as part of the refactor. + +Acceptance: the pre-refactor baseline is identified, necessary regression +coverage passes, and no production ownership or behavior has changed. If the +existing tests already cover these risks, do not add tests solely to fill this +stage; record the coverage reviewed and proceed. + +## Stage 2: Relocate The Memory Profile Repository + +Purpose: move a self-contained repository before changing engine assembly. + +1. Introduce `internal/profile.NewMemoryRepository` accepting internal profile + definitions. Move the map-backed repository and lookup implementation from + root `profiles.go` to that package. Preserve normalized duplicate-ID detection, + definition validation, missing-profile classification, inheritance metadata, + and independently owned values on storage and lookup. +2. Separate public field conversion from repository normalization as necessary. + Root `WithProfiles` converts at application time and invokes the new internal + constructor. Use the existing definition-normalization and JSON-copy owners + once at the repository boundary; preserve rejection of invalid extra values + and deep-copy guarantees without an unnecessary second validation pass. + Audit other callers before changing the responsibilities of `toDomainProfile`. +3. Remove the root repository implementation. Leave public `Profile` and + `OpenAICompatibleProfile` unchanged, including the convenience constructor's + documented copy behavior; do not strengthen that contract incidentally. +4. Move private repository tests to `internal/profile`, adapting inputs to + internal values. Retain representative public `WithProfiles` integration and + conversion coverage at the root. Add only uncovered copy/validation cases. + Update the internal source document and inventory for the new owner. + +Acceptance: `WithProfiles` uses the internal repository in the active engine; +public behavior is unchanged and no duplicate root repository remains. + +## Stage 3: Move Engine Assembly And Delegation Inward + +Purpose: establish the internal engine while leaving source-option relocation +for the next prompt. + +1. Add internal `Config`, `Options`, and `Engine` types. Internal `Config` carries + the existing construction settings, not a new consumer configuration API. + For this stage, `Options` may carry the already-prepared collaborators, + repositories, backend definitions, and source-presence flags from the root + option state. Do not lose distinctions between an absent source and an + explicitly supplied empty source. Stage 4 removes this transitional mapping. +2. Move the assembly portion of root `NewEngine`, after option application, + into `internal/engine.New`, including maintained module imports and + `newProfileRepository`. Preserve the existing assembly semantics: + - prompt-source requirement and configured source defaults; + - validated maintained catalogs and engine-local backend registry; + - raw profile precedence: memory, ordinary configured source, application + fallback, maintained catalog; an explicit ordinary source replaces + `Config.ProfileDir`, and one resolver wraps the complete overlay; + - schema default and supplied validator selection; + - supplied or default client and artifact reader selection; and + - one capacity manager per engine, one capacity wrapper around the selected + client, and that same wrapped client used by runner and repairer. +3. Give internal `Engine` domain-level forwarding methods for the existing + prepare, run, prepared-execution, and inspection operations. Keep runner + orchestration and prepared-handle lifecycle in `internal/usecase`. Do not + add another queue, retry loop, mutex, source cache, or execution state machine. +4. Change root `Engine` to hold the internal engine. Root `NewEngine` still + applies options and maps constructor errors. Root methods still check + nil/zero receivers, convert/validate public requests at their existing + boundary, convert results, and translate runtime errors. Do not wrap an + error twice or expose internal typed provider errors instead of public ones. +5. Adapt tests that inspect private runner fields to the new owner or, where + feasible, observable contract assertions. Reuse existing tests for prepared + handle engine binding, shared copied-handle state, consumption/discard, + cancellation, redaction, repair, and capacity. Add an assembly test only if + a consequential wiring obligation is otherwise untested. +6. Update architecture, component inventory, sources, and runner documentation + to describe the implemented split. Remove the superseded root assembly path. + +Acceptance: all public operations route through one internal engine and the +existing runner; maintained catalogs and component wiring are internally owned. +Prepared-execution behavior and public error contracts remain unchanged. + +## Stage 4: Move Option State And Source Construction Inward + +Purpose: finish the ownership split without changing the public option API. + +1. Replace the root collaborator/flag record with a private wrapper around + `engine.Options`. Move the construction state into private fields of that + internal type. Remove Stage 3's transitional root-to-internal state mapping. + Internal setters correspond to the existing source, profile, backend, client, + and artifact options; no generic option registry or callback framework. +2. Move filesystem/`fs.FS` repository and validator construction, `fileSource`, + source-presence flags, replacement rules, and backend accumulation into + internal option methods. Invoke them during each root option's `apply`, not + collectively after all options. Preserve current file checks and lazy source + loading; do not add path restrictions or normalize actual paths beyond the + existing contract. Preserve separate blank checks for whitespace-bearing + valid paths. +3. Keep public-to-internal conversion and public collaborator adapters at the + root. Preserve closure capture and validation timing from Stage 1, including + queue-capacity capture, profile copies, nil options, and ordinary interface + nil checks. Internal methods consume internal values/interfaces without + importing root types. Do not copy injected client or reader implementations. +4. Root `NewEngine` now performs only ordered option application, configuration + translation, internal construction, and public error translation. Review the + remaining root imports/helpers: retain boundary work, remove dead assembly + helpers, and do not relocate formatting or conversion for appearance alone. +5. Move implementation-only option/source setup tests with their owner, while + retaining public timing and error assertions at the facade. Update affected + documentation immediately. No exact private field layout becomes a test + requirement. + +Acceptance: the root no longer constructs repositories, validators, registries, +capacity managers, catalogs, or runners. Public options still have their existing +names, signatures, ordering, capture, and error behavior. No transitional state +mapping or duplicate source-assembly path remains. + +## Stage 5: Guard The Public Boundary And Dependency Direction + +Purpose: protect the intended architecture without freezing private layout. + +1. Add a focused root architecture test using Go type information. Load the + compiled root package with standard-library `go list` export data and + `go/importer`/`go/types`; avoid a new production dependency or parsing only + surface syntax. It must work in the ordinary offline maintainer workflow + once normal module dependencies are available. +2. Inspect exported declarations, public field types, function signatures, + interface methods, and both pointer and value method sets. Follow nested + type expressions and aliases with a visited set to avoid recursion. Reject + references to this module's internal types in accessible public surfaces, + public aliases to those types, and internal anonymous embedding that exposes + implementation methods. Account for generics if encountered rather than + silently ignoring their signatures or constraints. +3. Permit private internal fields and the sealed `Option` method. Do not follow + inaccessible fields as if they were public contracts or reject all aliases + indiscriminately. Check accessible promoted methods even if the embedded + field itself is unexported. Do not require the root to import only + `internal/engine`, or impose rules on unrelated third-party internals. +4. Check production package imports, using package metadata, for any dependency + from this module's `internal/` packages to its root. Exclude external test + packages from that restriction. Do not assert an exact internal package list. +5. Exercise the boundary checker with a small in-memory fixture set: allowed + private internal state and sealed option; forbidden direct/nested internal + signature or field; forbidden internal alias and promoted method. Keep the + test helper local and proportionate, not a reusable architecture framework. + +Acceptance: the completed refactor passes the checks; representative violations +fail for the intended reason, and ordinary private implementation changes remain +possible. No ConfigKit checkout or other sibling is needed. + +## Stage 6: Complete Compatibility And Documentation Review + +Purpose: verify the whole refactor, not just successful compilation of each move. + +1. Compare the final exported API against the Stage 1 commit, including method + sets, fields/tags, type ownership, interfaces, and public error identities. + Review any difference; no API additions/removals/redesigns are authorized. + Inspect supported serialization and formatting alongside the existing tests. +2. Review the final root package for retained business assembly or duplicated + repositories, and the internal engine for duplicated runner behavior. Review + test moves for lost assertions and duplicate matrices. Existing specialized + packages should still own their detailed behavior tests; facade tests should + prove conversion, compatibility, and representative assembled behavior. +3. Finish the durable documentation review: architecture owns dependency rules; + the internal overview inventories owners; sources and runner docs explain + their current implementation. Correct stale links and root-assembly claims. + Keep public GoDoc authoritative and avoid adding consumer migration steps, + format rules, or new guarantees for an internal relocation. No separate ADR + is needed merely to restate the existing facade policy. +4. Run the entire maintainer workflow, including race tests, vet, build, both + offline examples, formatting, local links, and repository hygiene checks. + Review the complete diff against the baseline. Report any environmental + validation blocker explicitly; do not call an unrun check successful. +5. Report completion against the feature roadmap criteria. Retain these two + temporary documents for the maintainer's completion review; their later + retirement follows documentation policy. Do not start the profile-policy + work or tag a release as part of this plan. + +Acceptance: compatibility review finds no unauthorized changes, all required +validation passes, documentation matches the implemented architecture, and the +refactor is ready for independent review before subsequent feature work. + +## Open Questions + +None requiring a product decision for this scope. The refactor preserves the +existing API and behavior; the provisional catalog policy and its release +decisions remain outside this plan. diff --git a/docs/roadmap/public-facade.md b/docs/roadmap/public-facade.md index 1494d89..4f758f4 100644 --- a/docs/roadmap/public-facade.md +++ b/docs/roadmap/public-facade.md @@ -3,9 +3,9 @@ ## Status Accepted direction; implementation has not begun. This feature roadmap defines -the intended end state and compatibility requirements. A separate staged -implementation plan will define the work sequence. Current GoDoc and durable -documentation remain authoritative until implementation lands. +the intended end state and compatibility requirements. The companion +[implementation plan](implementation.md) defines the work sequence. Current +GoDoc and durable documentation remain authoritative until implementation lands. ## Purpose @@ -97,8 +97,8 @@ internal package or require duplicate forwarding APIs to achieve that shape. ## Compatibility Requirements -This is a behavior-preserving refactor. Inventory the actual exported surface, -including methods and fields, before relocation. An exported declaration is +This is a behavior-preserving refactor covering the actual exported surface, +including methods and fields. An exported declaration is accessible even when it is absent from the consumer guide. Do not infer that an apparently low-level declaration is safe to remove: injected model clients and other intentional extension contracts may rely on it. @@ -155,13 +155,12 @@ private internal state and the established sealed option mechanism. Protect the no-internal-production-imports-of-root dependency rule without freezing an incidental file layout or exact internal package inventory. -Compare the exported surface before and after the refactor. Use existing -behavioral coverage first, adding tests only for consequential gaps introduced -by the new boundary. Pay particular attention to option timing, conversion -ownership, typed errors, and prepared-execution lifecycle. Run the complete +Compatibility evidence includes an unchanged exported surface and behavioral +coverage of consequential boundary risks: option timing, conversion ownership, +typed errors, and prepared-execution lifecycle. Acceptance requires the complete [maintainer validation workflow](../development.md#maintainer-validation), -including race checks and both offline examples, before accepting the result. -No live provider calls or sibling repository are required for validation. +including race checks and both offline examples. No live provider calls or +sibling repository are required for validation. ## Relationship To The Profile Policy Work