Fix the five library adoption review findings
This commit is contained in:
@@ -63,15 +63,28 @@ artifacts and process exit status must be assessed together.
|
||||
|
||||
## Redaction
|
||||
|
||||
Effective configuration uses ConfigKit redaction. LLM diagnostic and error paths
|
||||
use `llm.ConfiguredSecrets` and the runtime's redaction helpers. The CLI also
|
||||
redacts every resolved backend secret from both report copies. Keep these separate
|
||||
mechanisms aligned with the [architecture requirement](../policy/architecture.md#errors-and-secret-handling).
|
||||
Effective configuration uses ConfigKit redaction. The CLI supplies all resolved
|
||||
backend credentials, including unselected backends, to report and diagnostic
|
||||
writers. `core/redaction` sanitizes the serialized representation rather than
|
||||
mutating processing results. JSON string values and keys are redacted after
|
||||
parsing with number preservation; YAML redaction preserves scalar types.
|
||||
Serialization failure returns an error without writing the original object.
|
||||
Interaction artifacts, correction ledgers, execution/utilization diagnostics,
|
||||
selected profiles, both report copies, and surfaced LLM errors share this
|
||||
string-redaction implementation. Longer overlapping values take precedence,
|
||||
and replacement text is not processed again within a redaction pass.
|
||||
|
||||
Execution reports index events by call ID. Utilization groups calls directly by
|
||||
backend and actual module instance, keeping module result order and retaining
|
||||
usage knownness/completeness. Section and batch indices remain separate from
|
||||
module identity; section-qualified interaction filenames avoid collisions.
|
||||
These boundaries implement the [architecture requirement](../policy/architecture.md#errors-and-secret-handling).
|
||||
Operational handling of transcript-bearing artifacts belongs to
|
||||
[operations](../operations.md#artifact-handling).
|
||||
|
||||
## Key Tests
|
||||
|
||||
- `internal/core/redaction/redaction_test.go`
|
||||
- `internal/core/diagnostics/*_test.go`
|
||||
- `internal/framework/processreport/*_test.go`
|
||||
- `internal/core/reporting/report_test.go`
|
||||
|
||||
@@ -4,7 +4,9 @@
|
||||
**Scope:** The twelve implementation commits after `1ad2119`, compared with the
|
||||
[implementation plan](implementation.md), [combined target](library-adoption.md),
|
||||
and [configuration target](configkit.md).
|
||||
**Status:** Findings confirmed; fixes have not been implemented.
|
||||
**Status:** R1–R5 addressed and verified on 2026-09-24. The evidence in the
|
||||
original findings below describes source commit `7cefb3f`; line references are
|
||||
historical. See the resolution record at the end for the implemented fixes.
|
||||
|
||||
## Architecture Assessment
|
||||
|
||||
@@ -190,10 +192,98 @@ wall-clock timing assertions to the default test suite.
|
||||
The existing root `go test ./...`, focused race tests for LLM/runner/subprocess
|
||||
packages, `go vet ./...`, and Linux amd64/arm64 builds passed. Temporary review
|
||||
probes reproduced R1–R4 and measured R5; those probe files were removed afterward.
|
||||
The retained suite currently passes despite these defects. Add durable regression
|
||||
At review time, the retained suite passed despite these defects. Add durable regression
|
||||
coverage with the fixes, using the narrowest useful production boundaries.
|
||||
|
||||
Only this remediation document is changed by the review. No application fixes,
|
||||
The initial review changed only this remediation document. No application fixes,
|
||||
upstream changes, live provider calls, release publication, or compatibility work
|
||||
were performed. Release-distribution compliance remains the explicitly deferred
|
||||
release task and is not counted as an implementation defect here.
|
||||
|
||||
## Resolution Record
|
||||
|
||||
All five findings are resolved without compatibility layers, new scheduling,
|
||||
or changes to library ownership.
|
||||
|
||||
### R1 and R2 — Shared Serialization Boundary
|
||||
|
||||
`core/redaction` now sanitizes decoded JSON strings and YAML string nodes,
|
||||
retaining numeric/boolean types and avoiding mutation of domain objects.
|
||||
Longest matching credentials take precedence in a single replacement pass;
|
||||
JSON-escaped credential forms are handled for embedded response content.
|
||||
Serialization or key-collision failures return no payload. The unredacted report
|
||||
fallback has been removed.
|
||||
|
||||
The CLI supplies all resolved backend credentials to the external report writer
|
||||
and run directory. Report copies, ledgers, execution/utilization artifacts,
|
||||
interaction diagnostics, selected profiles, and error text share the redactor.
|
||||
ConfigKit still owns credential-reference redaction of effective configuration.
|
||||
|
||||
Evidence:
|
||||
|
||||
- `TestSubprocessRedactsValidModelReasonsOnSuccessAndPartialFailure` exercises
|
||||
schema-valid negative assessments containing selected and unselected secrets,
|
||||
including quotes/backslashes. Both reports and retained artifacts are checked;
|
||||
partial failure preserves earlier decisions and writes no corrected output.
|
||||
- `TestReportWritersRedactStringsAndFailClosed` exercises both actual report
|
||||
writers with credential `2`, preserving numeric configuration version 2 while
|
||||
redacting string content, and verifies failed serialization writes no artifact.
|
||||
- `TestStringRedactionPreservesSerializationAndDomainData` covers overlapping
|
||||
secrets, escaped nested content, large numbers, booleans, YAML scalar types,
|
||||
unchanged domain data, and failures without an original-payload fallback.
|
||||
|
||||
### R3 — Immediate Shared Cancellation
|
||||
|
||||
Validation workers record their error and cancel shared work immediately.
|
||||
All accesses to the originating-error state use a mutex. Proposal collection
|
||||
stops launching validation after a recorded failure; active workers are joined
|
||||
and available results retain deterministic collection order.
|
||||
|
||||
`TestValidatorFailureCancelsActiveAndPendingProviderWork` uses the real PromptKit
|
||||
runtime with backend concurrency one and queue capacity one. A failing validator
|
||||
cancels two peer executions, one active in a controlled transport and one pending,
|
||||
without manually releasing either. The originating error survives, only one
|
||||
provider attempt occurs, and both logical calls retain finished observations.
|
||||
The runner and runtime pass the race detector.
|
||||
|
||||
### R4 — Independent Module and Section Identity
|
||||
|
||||
Validation requests carry the configured module instance and an explicit section
|
||||
index. The structured call forwards both; only interaction filenames combine
|
||||
module, section, validator, and batch into a diagnostic name. The runner test
|
||||
helper now uses section index rather than batch index to identify sections.
|
||||
|
||||
`TestSubprocessRepeatedModulesKeepSectionAttribution` runs two grammar instances
|
||||
across two sections through the actual CLI/runtime. It verifies exactly two
|
||||
utilization entries with two proposal and two validation calls each, explicit
|
||||
section/batch identity, four distinct validation response artifacts, and unchanged
|
||||
proposal ordering in rejected decisions.
|
||||
|
||||
### R5 — Indexed Report Aggregation
|
||||
|
||||
Execution mapping indexes events by call ID once. Utilization directly groups
|
||||
calls/events by backend and indexes modules by real instance, seeded in module
|
||||
result order. Neither builder scans all events for each logical call.
|
||||
|
||||
Existing summary tests and `TestIndexedReportsPreserveOrderAndPartialUsage`
|
||||
verify pre-dispatch failures, original call/event order, backend grouping, and
|
||||
known-but-incomplete usage. `BenchmarkExecutionReports` retains a reproducible
|
||||
1,000/10,000/20,000-call benchmark without timing assertions.
|
||||
|
||||
A local three-iteration sample measured 20,000 calls at approximately **43 ms**
|
||||
for execution mapping and **84 ms** for utilization, compared with the review
|
||||
probe's 8.08 s and 2.42 s respectively. Timings are illustrative; the direct
|
||||
indexes establish removal of the quadratic scans independently of machine load.
|
||||
|
||||
### Completion Validation
|
||||
|
||||
- Full offline suite: `go test ./...`.
|
||||
- Race checks: runner, LLM runtime, process reporting, redaction, and CLI subprocess
|
||||
packages.
|
||||
- Static analysis: `go vet ./...`.
|
||||
- Both required Linux architectures: `scripts/build-release.sh` in a temporary
|
||||
output directory, without tagging or publishing.
|
||||
- Documentation/diff whitespace checks.
|
||||
|
||||
Release publication and deferred release-distribution work remain outside this
|
||||
remediation unit.
|
||||
|
||||
Reference in New Issue
Block a user