diff --git a/docs/cli.md b/docs/cli.md index 35ef091..703a95d 100644 --- a/docs/cli.md +++ b/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_`; +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. | diff --git a/docs/internal/app-orchestration.md b/docs/internal/app-orchestration.md index 1182725..e965221 100644 --- a/docs/internal/app-orchestration.md +++ b/docs/internal/app-orchestration.md @@ -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 diff --git a/docs/internal/comparison-execution.md b/docs/internal/comparison-execution.md index db3f681..af9ade0 100644 --- a/docs/internal/comparison-execution.md +++ b/docs/internal/comparison-execution.md @@ -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 diff --git a/docs/internal/comparison-publication.md b/docs/internal/comparison-publication.md index 544d5de..f427280 100644 --- a/docs/internal/comparison-publication.md +++ b/docs/internal/comparison-publication.md @@ -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 diff --git a/docs/operations.md b/docs/operations.md index aef10d4..582c9c8 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -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. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index a23202e..4312e21 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -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 diff --git a/docs/roadmap/profile-comparison.md b/docs/roadmap/profile-comparison.md index 3b2433f..009671c 100644 --- a/docs/roadmap/profile-comparison.md +++ b/docs/roadmap/profile-comparison.md @@ -1,6 +1,6 @@ # LLM Profile Comparison Roadmap -Status: Implemented; retained temporarily for post-implementation review. +Status: Implemented; retained as the feature roadmap. ## Purpose diff --git a/internal/app/comparison_execution.go b/internal/app/comparison_execution.go index 24fe7d0..85d902d 100644 --- a/internal/app/comparison_execution.go +++ b/internal/app/comparison_execution.go @@ -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 } diff --git a/internal/app/comparison_execution_test.go b/internal/app/comparison_execution_test.go index c670e29..5cf4143 100644 --- a/internal/app/comparison_execution_test.go +++ b/internal/app/comparison_execution_test.go @@ -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) } } diff --git a/internal/cli/comparison_test.go b/internal/cli/comparison_test.go index 3e23fca..5f1ef5e 100644 --- a/internal/cli/comparison_test.go +++ b/internal/cli/comparison_test.go @@ -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{ diff --git a/internal/cli/root.go b/internal/cli/root.go index 8abd632..331b91f 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -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{} diff --git a/internal/comparison/comparison.go b/internal/comparison/comparison.go index 24b5933..fe5f738 100644 --- a/internal/comparison/comparison.go +++ b/internal/comparison/comparison.go @@ -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 (