163 lines
12 KiB
Markdown
163 lines
12 KiB
Markdown
# Resource Inspection Implementation Review
|
||
|
||
## Scope And Results
|
||
|
||
Reviewed the twelve stages of [implementation.md](implementation.md) against
|
||
the accepted [feature roadmap](inspect-backend.md) and the current code and
|
||
documentation at `f083ad5`. The implementation commits run from `8add7c4`
|
||
through `f083ad5`, following planning commit `8563379`.
|
||
|
||
The selected code architecture is implemented. Resource inspection delegates
|
||
from the facade through the internal engine to captured configuration records;
|
||
execution keeps its backend-completed targets, and requirement discovery keeps
|
||
its lightweight metadata path. Public projections detach mutable values.
|
||
Typed sources use ordinary ConfigKit mapping, composition, and resource context.
|
||
|
||
The initial review identified one documentation finding. The subsequent
|
||
algorithmic review adds one low-priority optimization opportunity. No
|
||
correctness defect or unintended new restriction or guarantee was identified
|
||
in the reviewed implementation. The
|
||
resource-oriented profile result and changed post-handoff null fallback are
|
||
deliberately selected compatibility changes, not accidental regressions.
|
||
|
||
## Findings
|
||
|
||
### R1. Reconcile Remaining Inspection Contract Documentation [P2]
|
||
|
||
**Status:** Open. **Affected stages:** 1, 11, and final reconciliation in 12.
|
||
|
||
**Locations:**
|
||
|
||
- [Package GoDoc](../../doc.go), lines 50–68: the JSON section still explicitly
|
||
lists `ProfileInspection` and `PromptInspection` among values without stable
|
||
JSON representations.
|
||
- [Configuration migration guidance](../consumers/configuration-migration.md),
|
||
lines 133–139: it still says that `InspectProfile` applies backend completion.
|
||
|
||
These statements contradict the implemented result contracts in
|
||
[resource_inspection.go](../../resource_inspection.go), the method GoDoc in
|
||
[engine.go](../../engine.go), and the updated
|
||
[consumer guidance](../consumers/pkg-promptkit.md#inspect-a-profile-before-prompt-work).
|
||
The result types now deliberately own JSON/YAML keys, nesting, and omission
|
||
semantics. Profile inspection reports only the resolved profile's fields;
|
||
backend-completed settings belong to request preparation.
|
||
|
||
**Impact:** Consumers consulting package documentation are told that the new
|
||
generic rendering contract is unavailable. Consumers following the migration
|
||
guide may still expect backend endpoints, credential sources, or provider
|
||
defaults in a profile report even though those fields can now be absent. This
|
||
undermines the primary migration and reporting purpose of the feature.
|
||
|
||
**Required correction:**
|
||
|
||
1. Update the package GoDoc's serialization description to include the selected
|
||
resource-result contract and remove the contradictory exclusions. Link to
|
||
the result/method owners for details. Preserve the distinction from source
|
||
inputs, provenance inspection, and unrelated values without a serialization
|
||
contract; do not add byte-format, ordering, or round-trip guarantees.
|
||
2. Replace the migration guide's backend-completion claim with the resolved
|
||
resource boundary and point readers needing completed request settings to
|
||
the existing preparation APIs/current consumer guidance.
|
||
3. Check the remaining current documentation for equivalent stale claims.
|
||
Published historical release notes should remain unchanged.
|
||
|
||
**Verification:** Review the rendered package GoDoc and the affected migration
|
||
passage against the implemented types and method behavior; check edited local
|
||
links and whitespace. This is a documentation correction and does not require
|
||
another implementation path, compatibility field, or new runtime test.
|
||
|
||
### R2. Hoist The Prompt Presence Lookup Out Of The Input Loop [P3]
|
||
|
||
**Status:** Open for consideration. **Affected stage:** 4.
|
||
|
||
**Location:** [Prompt resource projection](../../internal/configuration/resources.go),
|
||
`InspectPrompt`, lines 77–85, especially `state.promptMetadata[id][i]` at line 81.
|
||
|
||
Every input repeats a lookup in the same map using the same prompt ID. The map
|
||
and selected metadata slice do not change during the loop; only the slice index
|
||
changes. This loop organization was not an algorithm selected by the roadmap.
|
||
|
||
**Complexity:** Let `I` be the selected prompt's input count, `M` its message
|
||
count, and `L` the prompt ID's byte length. With expected hash-table probing,
|
||
map lookup is constant in the number of catalog entries, but hashing a string
|
||
key can still cost `O(L)`. The current loop can therefore spend `O(I × L)` on
|
||
repeated key processing, for `O(L + I × L + I + M)` total adapter projection
|
||
time. Valid resource IDs have no fixed length limit. Go 1.27.3's ordinary
|
||
string-map lookup hashes the supplied key on each lookup; its small-map fast
|
||
path is an optimization, not a general elimination of this cost.
|
||
|
||
**Potential refactoring:** Fetch `presence := state.promptMetadata[id]` once
|
||
before iterating over the inputs and use `presence[i].required`. This changes
|
||
the projection bound to `O(L + I + M)` with constant additional storage. The
|
||
existing `O(I + M)` result allocation and public ownership copies remain
|
||
unchanged. No new cache, API, validation, or ID limit is needed.
|
||
|
||
This is a modest loop-invariant hoist, not a claim that inspection is quadratic
|
||
in catalog size. When ID length is treated as a fixed small constant, both
|
||
versions are linear in inputs/messages; the improvement removes an avoidable
|
||
multiplicative dependence when IDs and input counts grow independently.
|
||
|
||
**Verification:** Existing inherited-input presence and generic encoding tests
|
||
should continue to pass, including absent versus explicit false. Source review
|
||
can verify that the invariant lookup is outside the loop. No timing threshold,
|
||
real-world performance study, or test of an exact map-call count is needed.
|
||
|
||
## Stage-By-Stage Review Record
|
||
|
||
| Stage | Evidence inspected and result |
|
||
| --- | --- |
|
||
| 1 — Result declarations | `resource_inspection.go` and nested diagnostic tags in `backends.go` match the selected fields and optional-pointer representation. Package-level serialization documentation still needs R1. |
|
||
| 2 — Null interpretation | `captureExecutionProfile` uses configured pointer/map presence; profile-only tri-state capture is removed. Target and isolation tests cover null/omission fallback, inherited null, empty-string clearing, map overlay, and the required-key rule. |
|
||
| 3 — Backend/profile resources | `internal/configuration/resources.go` reads indexed resolved records, not completed targets. Adapter tests cover partial/inherited profiles, derived queue defaults, missing IDs, and configured/inherited diagnostics. |
|
||
| 4 — Prompt resources | Prompt capture retains the resolved record, materialized messages, and required/repair presence. Prompt tests cover inherited imported text and omitted versus explicit values; inspection does not reopen sources or resolve referenced profiles/schemas. |
|
||
| 5 — Facade wiring | The internal engine's resource interface delegates to the snapshot after namespace qualification. Root conversion detaches pointers, collections, and nested diagnostics; backend not-found mapping is local to `InspectBackend`. Public error and cancellation tests pass. |
|
||
| 6 — Existing consumers | Superseded runner wrappers and their dedicated tests are removed. Execution helpers and the narrow requirement repository remain. Existing profile, inheritance, source, namespace, bundle, and preparation-example tests are migrated. |
|
||
| 7 — Generic encoding | `resource_serialization_test.go` encodes real results with ordinary JSON and YAML encoders and checks semantic keys, omission, explicit values, empty collections, diagnostic objects, and provider nulls. The existing YAML dependency is direct; its version is unchanged. |
|
||
| 8 — Isolation/selection | Public tests cover scoped backend lookup, qualified bypass, no namespace fallback, independent results, concurrent inspection/mutation, offline references, execution fallback, and retained null provenance. |
|
||
| 9 — Typed sources | `prompts.go` and `BackendSource` use explicit mapping construction with optional pointers/collections and sibling `extends`. They do not validate incomplete records, resolve inheritance, or read content files. |
|
||
| 10 — Real composition | Typed-source tests cover snapshots, layered completion, nested settings, order, inheritance, explicit values, relative file context, invalid final definitions, and empty top-level input. |
|
||
| 11 — Documentation | Main consumer, format, and internal guidance, README, and the unpublished v0.20.0 release note reflect the selected changes. Two remaining current-documentation contradictions require R1. |
|
||
| 12 — Combined validation | Race-enabled suite, vet, module tidy comparison, formatting, and changed Markdown link-target checks pass. The documentation finding prevents declaring reconciliation fully complete. |
|
||
|
||
## Validation Performed
|
||
|
||
Using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, and `GOPROXY=off`:
|
||
|
||
- `go test -race ./...` passed, including the maintained offline examples.
|
||
- `go vet ./...` passed.
|
||
- `go mod tidy -diff` reported no changes.
|
||
- Formatting checks on surviving changed Go files and `git diff --check` passed.
|
||
- Local link targets in the changed Markdown documents exist.
|
||
|
||
The review made no production-code or test changes. These checks do not include
|
||
live provider diagnostics, release publication, or downstream application changes.
|
||
|
||
## Algorithmic Review
|
||
|
||
This follow-up reviewed the same production revision, now at repository commit
|
||
`101a284` (the intervening commit adds only this review document). It used static
|
||
loop, call-path, ownership, and data-size analysis, not performance measurements.
|
||
The existing architecture, testing, and documentation policies apply. Explicitly
|
||
selected algorithms and dependency-owned composition/encoding were not reopened.
|
||
|
||
| Stage | Algorithmic assessment |
|
||
| --- | --- |
|
||
| 1 — Result declarations | Types and tags introduce no traversal algorithm. Optional values preserve presence without requiring consumer-side reconstruction. |
|
||
| 2 — Null interpretation | The fixed set of profile-field checks is constant-sized. Existing backend-provider overlay and target materialization are retained decisions, not new algorithms to redesign in this audit. |
|
||
| 3 — Backend/profile resources | Resolved-record indexes are captured once. Individual inspection selects one record rather than scanning the catalog; public provider-map detachment visits the selected value graph. |
|
||
| 4 — Prompt resources | Input and message loops visit their respective sequences once, but the input loop has the avoidable repeated string-key lookup described in R2. Scalar presence capture does not scan all prompts for each input. |
|
||
| 5 — Facade wiring | Namespace qualification is proportional to ID length. Pointer projection is fixed-field work; nested provider copying is proportional to the copied value graph. No per-field whole-catalog lookup is introduced. |
|
||
| 6 — Existing consumers | Removed wrappers do not leave a second resolution pass. Requirement discovery continues using the narrow metadata/snapshot path, rather than copying the expanded resource report. |
|
||
| 7 — Generic encoding | Standard JSON/YAML encoding and presence-aware result types are selected planning decisions. No Promptkit serialization round trip or generic reflection layer was added. |
|
||
| 8 — Isolation/selection | Inspection itself performs no admission, provider work, schema compilation, or file reads. Caller-owned copies scale with the selected report rather than unrelated resources. |
|
||
| 9 — Typed sources | Constructors iterate once over supplied records and their nested input/message sequences, then delegate snapshotting to ConfigKit. These are additive nested sizes, not an all-inputs-by-all-messages cross product. |
|
||
| 10 — Real composition | Tests exercise the dependency-owned loading path; no second inheritance resolver or normalization pass was added for typed sources. |
|
||
| 11 — Documentation | No production algorithm; the previously recorded documentation correction remains R1. No additional performance guarantee is selected. |
|
||
| 12 — Combined validation | The reviewed new production paths contain no additional cross-resource quadratic scan or correctness defect requiring a finding. Existing successful suite/vet results remain applicable because production code is unchanged. |
|
||
|
||
The R2 complexity analysis accounts for string-key processing, rather than
|
||
assuming every map lookup is free of key-size cost. The selected Go toolchain's
|
||
`internal/runtime/maps/runtime_faststr.go` was inspected to confirm that point.
|
||
No production implementation, test, benchmark, dependency, or public contract was
|
||
changed by this follow-up.
|