Finalize profile comparison implementation
This commit is contained in:
13
docs/cli.md
13
docs/cli.md
@@ -138,20 +138,27 @@ The comparison status is `succeeded` only when every selected profile succeeds
|
||||
and the bundle is published. Individual profile failures still publish a
|
||||
complete partial bundle and return a failed command result. Cancellation or a
|
||||
failure before publication omits the artifact paths and returns a safe
|
||||
top-level error; it does not expose provider diagnostics. See the
|
||||
top-level error; the resolved `outputDirectory` and finalized timestamp remain
|
||||
when available. The safe error includes only a category and message: aggregate
|
||||
and unclassified application failures use `application`; cancellation uses
|
||||
`canceled`; deadlines use `deadline_exceeded`; prompt execution uses its
|
||||
published Promptkit category; destination failures use `destination_<kind>`;
|
||||
and committed cleanup failures use `publication_cleanup`. It does not expose
|
||||
provider diagnostics, filesystem causes, or recovery paths. See the
|
||||
[comparison bundle contract](integrations/comparison-bundle.md) for durable
|
||||
artifact fields and failure invariants.
|
||||
|
||||
If the bundle is published but cleanup of its replaced prior bundle fails, the
|
||||
summary still includes the published artifact paths and has status `failed`.
|
||||
Its JSON error remains generic; the returned command error identifies the
|
||||
Its JSON error is `publication_cleanup` with the message `comparison published
|
||||
but cleanup did not complete`; the returned command error identifies the
|
||||
retained backup path for operator recovery.
|
||||
|
||||
## Flag Reference
|
||||
|
||||
| Flag | Accepted by | Meaning |
|
||||
| --- | --- | --- |
|
||||
| `-h`, `--help` | top level | Show help. |
|
||||
| `-h`, `--help` | top level, `compare` | Show help without loading configuration or contacting a provider. |
|
||||
| `--config PATH` | all commands | Load `PATH` instead of `/usr/local/etc/weatherreporter/config.yml`. |
|
||||
| `--units VALUE` | `generate`, `run`, `compare` | Override `weather_api.units` for this command. |
|
||||
| `--tz NAME` | `generate`, `run`, `compare` | Override `weather_api.timezone` for this command. |
|
||||
|
||||
@@ -27,6 +27,11 @@ debugging, prompt inspection, or collection. It then inspects the one prompt
|
||||
and every selected profile, collects once, and delegates shared report
|
||||
construction to the prepared-report flow. It does not accept a notifier.
|
||||
|
||||
Once the destination is resolved, the partial result retains its absolute
|
||||
output directory even when later preflight, debug initialization, inspection,
|
||||
collection, or preparation fails. Every initialized result is finalized with a
|
||||
finished timestamp. Artifact paths are added only after publication commits.
|
||||
|
||||
The comparison execution core starts each inspected profile independently,
|
||||
keeps results in selection order, and waits for all started work. Independent
|
||||
profile failures are recorded and do not stop peers. Context cancellation marks
|
||||
@@ -39,7 +44,8 @@ internals](comparison-publication.md).
|
||||
When publication has committed its new bundle, application results contain the
|
||||
absolute manifest, data-package, and successful report paths even if removal of
|
||||
the previous sibling backup then fails. That cleanup failure is still returned
|
||||
as an operational error rather than treating the new bundle as unpublished.
|
||||
as an operational error rather than treating the new bundle as unpublished;
|
||||
the returned error retains the recovery path and underlying filesystem cause.
|
||||
|
||||
## Boundaries And Verification
|
||||
|
||||
|
||||
@@ -16,8 +16,10 @@ outcomes as skipped or failed, joins work, and prevents bundle publication.
|
||||
When debugging is enabled, each execution receives a deterministic reference
|
||||
derived from the comparison identity, ordered profile position, and safe
|
||||
profile slug. This keeps concurrent captures separate. The debug writer itself
|
||||
owns secure-root validation and file permissions; operational retention and
|
||||
sensitivity are documented in the [operations guide](../operations.md).
|
||||
owns secure-root validation and file permissions. It safely creates shared
|
||||
missing ancestors during concurrent writes, then rejects symlink and non-
|
||||
directory components. Operational retention and sensitivity are documented in
|
||||
the [operations guide](../operations.md).
|
||||
|
||||
The output result and its safe errors are converted into the durable contract
|
||||
only by comparison publication. See [comparison publication
|
||||
|
||||
@@ -10,15 +10,17 @@ Destination planning is read-only. It requires an exact absolute target that
|
||||
is neither the filesystem root nor the working directory, rejects unsafe
|
||||
symlinks and non-directories, accepts a missing or empty directory, and permits
|
||||
replacement only for a recognized current bundle. Publication rechecks that
|
||||
plan, writes a private sibling staging directory, and installs it atomically.
|
||||
Replacement temporarily moves the recognized prior bundle aside, reauthorizes
|
||||
that moved entry, and restores it if installation fails.
|
||||
authorization immediately before it writes a private sibling staging directory.
|
||||
For replacement, it moves the prior bundle to a private sibling backup,
|
||||
reauthorizes that moved entry, and restores it if installing the new bundle
|
||||
fails.
|
||||
|
||||
Publication reports whether the new bundle committed. Once the new bundle is
|
||||
installed, a failure to remove the prior sibling backup does not roll it back.
|
||||
The result retains the absolute backup path and returns an inspectable cleanup
|
||||
error that unwraps the filesystem cause so an operator can recover the prior
|
||||
bundle if needed.
|
||||
The new bundle is committed only after the staged directory has been installed
|
||||
at the target. From that point its artifact paths are authoritative: a failure
|
||||
to remove the retained sibling backup does not roll back the new bundle.
|
||||
Publication returns an inspectable cleanup error with the absolute backup path
|
||||
and underlying filesystem cause so an operator can recover or remove that
|
||||
backup manually.
|
||||
|
||||
The application preflights before prompt inspection and collection, then
|
||||
preflights again before publication. A cancellation or any failure before the
|
||||
|
||||
@@ -152,9 +152,10 @@ cancellation and pre-publication errors leave the prior destination unchanged.
|
||||
|
||||
If a replacement commits but cleanup of its prior sibling backup fails, the new
|
||||
bundle remains valid and its artifact paths appear in the failed command
|
||||
summary. Preserve the retained backup path reported by the command error until
|
||||
it has been inspected and cleaned up manually; do not remove the new bundle to
|
||||
retry that cleanup.
|
||||
summary. The summary records a safe `publication_cleanup` error, while the
|
||||
returned command error reports the retained backup path. Preserve that backup
|
||||
until it has been inspected and cleaned up manually; do not remove the new
|
||||
bundle to retry that cleanup.
|
||||
|
||||
Enable explicit debug capture only when content-rich Promptkit diagnostics are
|
||||
necessary.
|
||||
|
||||
@@ -1,19 +1,18 @@
|
||||
# LLM Profile Comparison Implementation Plan
|
||||
|
||||
Status: Follow-up work planned after post-implementation review.
|
||||
Status: Complete.
|
||||
|
||||
## Purpose And Authority
|
||||
|
||||
This document is the ordered implementation plan for the accepted [LLM
|
||||
Profile Comparison Roadmap](profile-comparison.md). The roadmap owns the
|
||||
feature purpose, policy, scope, and desired end state. This plan records the
|
||||
completed implementation and defines the remaining corrective work discovered
|
||||
during post-implementation review.
|
||||
completed implementation and records the corrective work completed during
|
||||
post-implementation review.
|
||||
|
||||
Stages 1 through 10 are complete. Implement Stages 11 through 15 in numeric
|
||||
order, using one gpt-5.6-terra coding prompt per stage. Each stage is scoped to
|
||||
leave the repository compiling, tested, documented to its implemented boundary,
|
||||
and internally coherent.
|
||||
All implementation work in this plan is complete. The recorded work leaves the
|
||||
repository compiling, tested, documented to its implemented boundary, and
|
||||
internally coherent.
|
||||
|
||||
## Implementation Rules
|
||||
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
# LLM Profile Comparison Roadmap
|
||||
|
||||
Status: Implemented; retained temporarily for post-implementation review.
|
||||
Status: Implemented; retained as the feature roadmap.
|
||||
|
||||
## Purpose
|
||||
|
||||
|
||||
@@ -35,7 +35,6 @@ type comparisonProfileOutcome struct {
|
||||
Markdown []byte
|
||||
LLMDebugPath string
|
||||
Error *comparison.SafeError
|
||||
err error
|
||||
}
|
||||
|
||||
func executeComparisonProfiles(ctx context.Context, req comparisonExecutionRequest) comparisonExecutionResult {
|
||||
@@ -100,14 +99,12 @@ func executeComparisonProfile(ctx context.Context, req comparisonExecutionReques
|
||||
outcome.ValidationStatus = execution.ValidationStatus
|
||||
outcome.LLMDebugPath = execution.LLMDebugPath
|
||||
if err != nil {
|
||||
outcome.err = err
|
||||
safe := comparisonSafeExecutionError(err)
|
||||
outcome.Error = &safe
|
||||
return outcome
|
||||
}
|
||||
reportPath, err := comparison.ReportFilename(position, len(req.Inspection.Profiles), profile.ProfileID)
|
||||
if err != nil {
|
||||
outcome.err = err
|
||||
safe := comparison.NewSafeError("application", "derive comparison report filename failed")
|
||||
outcome.Error = &safe
|
||||
return outcome
|
||||
@@ -133,7 +130,6 @@ func markCanceledComparisonOutcome(outcome *comparisonProfileOutcome, err error)
|
||||
outcome.ValidationStatus = promptexec.ValidationSkipped
|
||||
outcome.ReportPath = ""
|
||||
outcome.Markdown = nil
|
||||
outcome.err = err
|
||||
safe := comparisonSafeExecutionError(err)
|
||||
outcome.Error = &safe
|
||||
}
|
||||
|
||||
@@ -73,7 +73,7 @@ func TestExecuteComparisonProfilesContinuesAfterProfileFailure(t *testing.T) {
|
||||
t.Fatalf("outcomes = %#v", result.Outcomes)
|
||||
}
|
||||
failure := result.Outcomes[1]
|
||||
if failure.Error == nil || failure.Error.Category != string(promptexec.Generation) || failure.Error.Message != "execute prompt failed" || failure.err == nil || failure.ReportPath != "" || len(failure.Markdown) != 0 {
|
||||
if failure.Error == nil || failure.Error.Category != string(promptexec.Generation) || failure.Error.Message != "execute prompt failed" || failure.ReportPath != "" || len(failure.Markdown) != 0 {
|
||||
t.Fatalf("failure outcome = %#v", failure)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -137,23 +137,25 @@ func TestComparisonInputFailuresDoNotConstructOrExecute(t *testing.T) {
|
||||
return nil, nil
|
||||
},
|
||||
}
|
||||
if _, err := runner.executeComparison(context.Background(), test.args); err == nil {
|
||||
t.Fatal("executeComparison() error = nil")
|
||||
var stdout, stderr bytes.Buffer
|
||||
if err := runner.Run(context.Background(), append([]string{"compare"}, test.args...), &stdout, &stderr); err == nil {
|
||||
t.Fatal("Run() error = nil")
|
||||
}
|
||||
if factoryCalls != 0 || applicationCalls != 0 {
|
||||
t.Fatalf("factory/application calls = %d/%d", factoryCalls, applicationCalls)
|
||||
if factoryCalls != 0 || applicationCalls != 0 || stdout.Len() != 0 || stderr.Len() != 0 {
|
||||
t.Fatalf("factory/application calls/output = %d/%d/%q/%q", factoryCalls, applicationCalls, stdout.String(), stderr.String())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestExecuteComparisonUsesOneExecutorAndInjectedApplication(t *testing.T) {
|
||||
func TestCompareCommandUsesOneExecutorAndInjectedApplication(t *testing.T) {
|
||||
workingDir := t.TempDir()
|
||||
configPath := comparisonConfigPath(t, "weather_api:\n base_url: https://weather.api.example.com/\n")
|
||||
executor := &factoryExecutor{}
|
||||
factoryCalls, applicationCalls := 0, 0
|
||||
var received app.ComparisonRequest
|
||||
wantResult := &app.ComparisonResult{ComparisonID: "comparison_test"}
|
||||
comparisonErr := errors.New("comparison completed with 1 failed profiles")
|
||||
runner := Runner{
|
||||
Clock: timeutil.FixedClock{Time: time.Date(2026, 5, 29, 8, 30, 0, 0, time.UTC)}, WorkingDir: workingDir,
|
||||
ExecutorFactory: func(PromptExecutorConfig) (promptexec.Executor, error) {
|
||||
@@ -163,14 +165,15 @@ func TestExecuteComparisonUsesOneExecutorAndInjectedApplication(t *testing.T) {
|
||||
compareDetailed: func(_ context.Context, req app.ComparisonRequest) (*app.ComparisonResult, error) {
|
||||
applicationCalls++
|
||||
received = req
|
||||
return wantResult, errors.New("comparison completed with 1 failed profiles")
|
||||
return wantResult, comparisonErr
|
||||
},
|
||||
}
|
||||
result, err := runner.executeComparison(context.Background(), []string{
|
||||
"daily", "--date", "2026-05-29", "--profile", "weather-light", "--profile", "weather-deep", "--out-dir", "comparison", "--replace", "--config", configPath,
|
||||
})
|
||||
if result != wantResult || err == nil || factoryCalls != 1 || applicationCalls != 1 || received.Executor != executor || received.OutputDir != filepath.Join(workingDir, "comparison") || !received.Replace {
|
||||
t.Fatalf("result/error/calls/request = %#v/%v/%d/%d/%#v", result, err, factoryCalls, applicationCalls, received)
|
||||
var stdout, stderr bytes.Buffer
|
||||
err := runner.Run(context.Background(), []string{
|
||||
"compare", "daily", "--date", "2026-05-29", "--profile", "weather-light", "--profile", "weather-deep", "--out-dir", "comparison", "--replace", "--config", configPath,
|
||||
}, &stdout, &stderr)
|
||||
if !errors.Is(err, comparisonErr) || factoryCalls != 1 || applicationCalls != 1 || received.Executor != executor || received.OutputDir != filepath.Join(workingDir, "comparison") || !received.Replace || stdout.Len() == 0 || stderr.Len() != 0 {
|
||||
t.Fatalf("error/calls/request/output = %v/%d/%d/%#v/%q/%q", err, factoryCalls, applicationCalls, received, stdout.String(), stderr.String())
|
||||
}
|
||||
if !reflect.DeepEqual(received.ProfileIDs, []string{"weather-light", "weather-deep"}) {
|
||||
t.Fatalf("profile IDs = %#v", received.ProfileIDs)
|
||||
@@ -320,6 +323,13 @@ func TestCompareHelpIncludesCommand(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestCompareHelpDoesNotRequireConfiguration(t *testing.T) {
|
||||
var stdout, stderr bytes.Buffer
|
||||
if err := (Runner{}).Run(context.Background(), []string{"compare", "--help"}, &stdout, &stderr); err != nil || !strings.Contains(stdout.String(), "weatherreporter compare REPORT") || stderr.Len() != 0 {
|
||||
t.Fatalf("help/error/stderr = %q/%v/%q", stdout.String(), err, stderr.String())
|
||||
}
|
||||
}
|
||||
|
||||
func comparisonRunner(t *testing.T, workingDir string) Runner {
|
||||
t.Helper()
|
||||
return Runner{
|
||||
|
||||
@@ -64,6 +64,10 @@ func (r Runner) Run(ctx context.Context, args []string, stdout io.Writer, stderr
|
||||
_, err := fmt.Fprint(stdout, helpText)
|
||||
return err
|
||||
}
|
||||
if len(args) == 2 && args[0] == "compare" && (args[1] == "--help" || args[1] == "-h") {
|
||||
_, err := fmt.Fprint(stdout, helpText)
|
||||
return err
|
||||
}
|
||||
if args[0] == "--version" {
|
||||
if len(args) != 1 {
|
||||
return fmt.Errorf("--version does not accept arguments")
|
||||
@@ -252,23 +256,6 @@ func (r Runner) resolveRun(args []string) (app.BatchRequest, error) {
|
||||
return req, err
|
||||
}
|
||||
|
||||
func (r Runner) resolveComparison(args []string) (app.ComparisonRequest, error) {
|
||||
req, _, err := r.resolveComparisonAction(args)
|
||||
return req, err
|
||||
}
|
||||
|
||||
func (r Runner) executeComparison(ctx context.Context, args []string) (*app.ComparisonResult, error) {
|
||||
req, _, err := r.resolveComparisonAction(args)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
compareDetailed := r.compareDetailed
|
||||
if compareDetailed == nil {
|
||||
compareDetailed = app.CompareDetailed
|
||||
}
|
||||
return compareDetailed(ctx, req)
|
||||
}
|
||||
|
||||
func (r Runner) resolveComparisonAction(args []string) (app.ComparisonRequest, commonOptions, error) {
|
||||
if r.Clock == nil {
|
||||
r.Clock = timeutil.SystemClock{}
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
// Package comparison defines the durable logical contract for profile
|
||||
// comparison bundles. It deliberately has no filesystem or application
|
||||
// orchestration dependencies.
|
||||
// Package comparison owns logical profile-comparison bundle contracts and
|
||||
// guarded filesystem destination planning and publication.
|
||||
package comparison
|
||||
|
||||
import (
|
||||
|
||||
Reference in New Issue
Block a user