From b39b68add74881fc049e424b2a780f64aa42c912 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 10 Aug 2026 21:20:59 +0000 Subject: [PATCH] Unify previous source resolution --- docs/internal/command-restore.md | 11 +- docs/internal/stage-prepare.md | 6 +- docs/operations.md | 4 + docs/roadmap/implementation.md | 2 +- internal/adapters/storage/fake.go | 10 +- internal/app/operator_helpers_test.go | 30 ++++ internal/app/operator_inspection.go | 38 +++-- internal/app/operator_session_validate.go | 15 +- internal/app/operator_status.go | 8 +- internal/app/restore_commit_test.go | 50 ++++++ internal/app/restore_discovery.go | 2 + internal/app/restore_execute.go | 5 + internal/app/restore_execution_test.go | 3 + internal/app/restore_plan.go | 7 +- internal/app/restore_report.go | 3 + internal/artifacts/current_state.go | 1 + internal/artifacts/current_state_commit.go | 1 + internal/previouscache/previouscache.go | 119 +++++++++----- internal/previouscache/previouscache_test.go | 160 +++++++++++++++++++ internal/stage/prepare_previous.go | 7 +- 20 files changed, 406 insertions(+), 76 deletions(-) diff --git a/docs/internal/command-restore.md b/docs/internal/command-restore.md index cbcc40f..5f8b38c 100644 --- a/docs/internal/command-restore.md +++ b/docs/internal/command-restore.md @@ -43,8 +43,9 @@ Planner behavior: For a non-dry-run restore, planning/classification happens only after acquiring the session lock. Runner manifest/reuse checks acquire that same lock first. -Previous-cache files are planned separately through `previouscache.BuildPlan` -when configured previous-session requirements exist. +Previous-cache readiness is resolved through `previouscache.Resolve` for restore, +prepare, status, and validation. A committed source is selected only by its +exact source identity; legacy fallback remains isolated and rejects ambiguity. ## Execution Contract @@ -56,6 +57,9 @@ Execution order and safety: - manifest replacement is validated before rename; - each committed object is verified against its declared checksum, size, and generation before installation; +- a committed manifest already verified during discovery is retained for the + matching restore action and revalidated before installation, avoiding a + second body transfer; - failed installs do not roll back files already written in the same execution. - a durable `.restore-incomplete.json` marker is written before installation. It blocks runners until a restore retry completes all verified installs and @@ -75,7 +79,8 @@ Audio restore path: ## Reporting Contract -- dry-run mode prints a summary and performs no local writes; +- dry-run mode prints a summary, performs no durable session writes, and may + read remote current-state or object-identity data to produce that summary; - execution mode persists the canonical restore report described in [Operations](../operations.md#restore-workflow); - report includes plan counts, per-action status, and execution failures. diff --git a/docs/internal/stage-prepare.md b/docs/internal/stage-prepare.md index 4439b33..99a6175 100644 --- a/docs/internal/stage-prepare.md +++ b/docs/internal/stage-prepare.md @@ -34,11 +34,13 @@ Materialize canonical current-session inputs before processing stages. - scans enabled configured artifact inputs for `narratio.previous_session.artifact.*` requirements. - when previous requirements exist: - clears managed `previous/` state; - - builds previous-cache remote plan; + - resolves the pointer-selected previous source through the shared resolver; - downloads previous manifest/artifacts; - records previous inputs in `manifest.inputs`. -Required previous-session inputs fail when unavailable; optional missing inputs are skipped. +Required previous-session inputs fail when unavailable; optional missing inputs +are typed skipped results. Committed sources use their exact source-to-destination +mapping, while the isolated legacy reader rejects ambiguous fallback matches. ## Invariants diff --git a/docs/operations.md b/docs/operations.md index 592be34..a754b31 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -269,6 +269,10 @@ Apply: narratio session restore 2026-04-04 ``` +`--dry-run` does not write durable session files. It still reads the selected +remote current state and may read object identity/content needed to classify the +plan, so it is not a network-free operation. + Default restore scope: - the committed session manifest and the committed transcript/artifact objects diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 684e33e..e636ac8 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -35,7 +35,7 @@ All stages are pending when this plan is created. | 17 | Bind restore/status to a committed snapshot and reject conflicts | COR-008, COR-009, TST-005 | Completed | | 18 | Serialize restore transitions and make restored paths portable | RSK-006, RSK-008 | Completed | | 19 | Bind audio cache reuse to remote object identity | RSK-007 | Completed | -| 20 | Unify previous-source readiness and eliminate duplicate transfers | COR-010, EFF-001 | Pending | +| 20 | Unify previous-source readiness and eliminate duplicate transfers | COR-010, EFF-001 | Completed | | 21 | Tighten configuration parsing, values, and expectations | COR-012–COR-015, TST-011, TST-014 | Pending | | 22 | Make product configuration truthful and own remote temp files | COR-024, RSK-009, ARC-004 | Pending | | 23 | Stream WhisperX uploads and make the adapter race-safe | COR-016, EFF-002, TST-001 | Pending | diff --git a/internal/adapters/storage/fake.go b/internal/adapters/storage/fake.go index a10f4e0..8e378d5 100644 --- a/internal/adapters/storage/fake.go +++ b/internal/adapters/storage/fake.go @@ -42,6 +42,7 @@ type FakeUploadCall struct { type FakeDownloadCall struct { Key string LocalPath string + Bytes int64 } // FakeObject is a deterministic fake object-store record. @@ -151,12 +152,13 @@ func (f *FakeBackend) DownloadTo(ctx context.Context, key string, destination io return err } defer source.Close() - f.mu.Lock() - f.Downloads = append(f.Downloads, FakeDownloadCall{Key: normalizeObjectKey(key)}) - f.mu.Unlock() - if _, err := io.Copy(destination, source); err != nil { + count, err := io.Copy(destination, source) + if err != nil { return fmt.Errorf("download object %q: write destination: %w", key, err) } + f.mu.Lock() + f.Downloads = append(f.Downloads, FakeDownloadCall{Key: normalizeObjectKey(key), Bytes: count}) + f.mu.Unlock() return nil } diff --git a/internal/app/operator_helpers_test.go b/internal/app/operator_helpers_test.go index ea5c7d9..6bcd3ee 100644 --- a/internal/app/operator_helpers_test.go +++ b/internal/app/operator_helpers_test.go @@ -1099,6 +1099,36 @@ func TestExecuteStatusReportsPreviousStateReadinessWithoutFailing(t *testing.T) } } +func TestExecuteStatusDetectsMissingRequiredPreviousArtifact(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, sessionPath := writeValidConfigFilesWithScriptoriumArtifacts(t, workspaceRoot) + replaceInFileOrFatal(t, pipelinePath, "source: narratio.artifact.session_recap", "source: narratio.previous_session.artifact.session_recap") + replaceInFileOrFatal(t, sessionPath, "session_id: 2026-05-03\n", "session_id: 2026-05-03\nprevious_session_id: 2026-04-26\n") + cfg, err := config.LoadWithSessionOptions(pipelinePath, campaignPath, sessionPath, config.SessionLoadOptions{}) + if err != nil { + t.Fatalf("LoadWithSessionOptions() error = %v", err) + } + fake := &storage.FakeBackend{} + seedRestorePreviousCurrentManifestOnly(t, fake, cfg) + var storeInitCalls int + restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{sessionPath}) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{ + "session", "status", "2026-05-03", + "--config", pipelinePath, + "--campaign-file", campaignPath, + "--session", sessionPath, + }, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !strings.Contains(stdout.String(), `Previous-session artifacts: unavailable: remote required previous-session artifact "session_recap" object missing`) { + t.Fatalf("stdout = %q, want missing required previous artifact", stdout.String()) + } +} + func TestExecuteSessionValidateReportsPreviousStateFindingAndReturnsFindingError(t *testing.T) { workspaceRoot := t.TempDir() pipelinePath, campaignPath, sessionPath := writeValidConfigFilesWithScriptoriumArtifacts(t, workspaceRoot) diff --git a/internal/app/operator_inspection.go b/internal/app/operator_inspection.go index 2afff23..d90aacd 100644 --- a/internal/app/operator_inspection.go +++ b/internal/app/operator_inspection.go @@ -2,6 +2,7 @@ package app import ( "context" + "errors" "fmt" "os" "path" @@ -12,6 +13,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/previouscache" ) type stableInputCheck struct { @@ -34,9 +36,9 @@ type remoteAudioCheck struct { } type previousArtifactReadiness struct { - Requirements []artifacts.PreviousArtifactRequirement - MissingID bool - Err error + Requirements []artifacts.PreviousArtifactRequirement + SkippedMissing []string + Err error } type remoteCurrentStateCheck struct { @@ -152,23 +154,27 @@ func inspectPreviousArtifactReadiness( if len(requirements) == 0 { return out } - if strings.TrimSpace(cfg.Session.PreviousSessionID) == "" { - out.MissingID = true + if cfg == nil || cfg.Pipeline == nil || cfg.Session == nil { + out.Err = fmt.Errorf("resolved config with pipeline/session is required") return out } - if store == nil { - out.Err = fmt.Errorf("previous-session artifacts cannot be checked because storage is unavailable") + paths := artifacts.NewLocalStore(cfg.Pipeline.Workspace.Root).SessionPathsFor(cfg.Session.Campaign, cfg.Session.SessionID) + plan, err := previouscache.Resolve(ctx, cfg, paths, requirements, store) + if err != nil { + var pointerMissing *artifacts.CurrentRunPointerMissingError + var manifestMissing *artifacts.CurrentManifestMissingError + if errors.As(err, &pointerMissing) { + out.Err = fmt.Errorf("remote %w", pointerMissing) + return out + } + if errors.As(err, &manifestMissing) { + out.Err = fmt.Errorf("remote %w", manifestMissing) + return out + } + out.Err = fmt.Errorf("remote %w", err) return out } - - prefix := artifacts.S3SessionPrefix(cfg.Pipeline.Storage.S3.RootPrefix, cfg.Session.Campaign, cfg.Session.PreviousSessionID) - if _, err := artifacts.LoadCurrentState(ctx, store, prefix, artifacts.CurrentStateValidation{ - ExpectedSessionID: strings.TrimSpace(cfg.Session.PreviousSessionID), - ExpectedCampaign: strings.TrimSpace(cfg.Session.Campaign), - ValidateRunID: true, - }); err != nil { - out.Err = fmt.Errorf("remote %v", err) - } + out.SkippedMissing = append([]string(nil), plan.SkippedMissing...) return out } diff --git a/internal/app/operator_session_validate.go b/internal/app/operator_session_validate.go index abfcd43..ce53309 100644 --- a/internal/app/operator_session_validate.go +++ b/internal/app/operator_session_validate.go @@ -57,12 +57,14 @@ func SessionValidate(ctx context.Context, args []string, out io.Writer) error { previous := inspectPreviousArtifactReadiness(ctx, cfg, store, requirements) if len(previous.Requirements) == 0 { findings = append(findings, okFinding("previous", "no previous-session artifacts required")) - } else if previous.MissingID { - findings = append(findings, errorFinding("previous", "previous_session_id is required by configured previous-session artifacts")) } else if previous.Err != nil { findings = append(findings, errorFinding("previous", previous.Err.Error())) } else { for _, req := range previous.Requirements { + if !req.Required && previousRequirementSkipped(previous.SkippedMissing, req.Name) { + findings = append(findings, okFinding("previous", fmt.Sprintf("%s required=false unavailable", req.Name))) + continue + } findings = append(findings, okFinding("previous", fmt.Sprintf("%s required=%t", req.Name, req.Required))) } } @@ -82,3 +84,12 @@ func SessionValidate(ctx context.Context, args []string, out io.Writer) error { } return renderFindings(out, cfg.Session.Campaign, cfg.Session.SessionID, findings) } + +func previousRequirementSkipped(values []string, name string) bool { + for _, value := range values { + if value == name { + return true + } + } + return false +} diff --git a/internal/app/operator_status.go b/internal/app/operator_status.go index c0cc5f0..11bb74a 100644 --- a/internal/app/operator_status.go +++ b/internal/app/operator_status.go @@ -154,10 +154,6 @@ func writeStatusPreviousArtifacts(out io.Writer, readiness previousArtifactReadi fmt.Fprintln(out, "Previous-session artifacts: not required") return } - if readiness.MissingID { - fmt.Fprintln(out, "Previous-session artifacts: unavailable: previous_session_id is required by configured previous-session artifacts") - return - } if readiness.Err != nil { fmt.Fprintf(out, "Previous-session artifacts: unavailable: %v\n", readiness.Err) return @@ -167,5 +163,9 @@ func writeStatusPreviousArtifacts(out io.Writer, readiness previousArtifactReadi names = append(names, fmt.Sprintf("%s(required=%t)", req.Name, req.Required)) } sort.Strings(names) + if len(readiness.SkippedMissing) > 0 { + fmt.Fprintf(out, "Previous-session artifacts: ready: %s; optional unavailable: %s\n", strings.Join(names, ", "), strings.Join(readiness.SkippedMissing, ", ")) + return + } fmt.Fprintf(out, "Previous-session artifacts: ready: %s\n", strings.Join(names, ", ")) } diff --git a/internal/app/restore_commit_test.go b/internal/app/restore_commit_test.go index edf15de..22cb6b8 100644 --- a/internal/app/restore_commit_test.go +++ b/internal/app/restore_commit_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "sort" + "strings" "testing" "time" @@ -42,6 +43,45 @@ func TestCommittedRestorePlanUsesOnlyDeclaredObjects(t *testing.T) { } } +func TestCommittedRestoreReusesVerifiedManifestCandidate(t *testing.T) { + cfg := restorePlanConfig(t) + fake := &storage.FakeBackend{} + current := seedCommittedRestoreSnapshot(t, cfg, fake, "20260519T010203Z-a1b2c3d4", nil) + + plan, err := buildRestorePlan(context.Background(), cfg, current, fake, RestorePlanOptions{}) + if err != nil { + t.Fatalf("buildRestorePlan() error = %v", err) + } + if _, err := executeRestorePlan(context.Background(), cfg, current, plan, nil, fake); err != nil { + t.Fatalf("executeRestorePlan() error = %v", err) + } + if got := fakeDownloadCount(fake, current.CurrentManifestKey); got != 1 { + t.Fatalf("manifest downloads = %d, want one discovery transfer reused by restore", got) + } + if got := fakeDownloadBytes(fake, current.CurrentManifestKey); got != int64(len(current.ManifestData)) { + t.Fatalf("manifest bytes transferred = %d, want one verified candidate (%d)", got, len(current.ManifestData)) + } +} + +func TestCommittedRestoreRejectsChangedGenerationForVerifiedManifestCandidate(t *testing.T) { + cfg := restorePlanConfig(t) + fake := &storage.FakeBackend{} + current := seedCommittedRestoreSnapshot(t, cfg, fake, "20260519T010203Z-a1b2c3d4", nil) + plan, err := buildRestorePlan(context.Background(), cfg, current, fake, RestorePlanOptions{}) + if err != nil { + t.Fatalf("buildRestorePlan() error = %v", err) + } + fake.SeedObject(storage.FakeObject{Key: current.CurrentManifestKey, Data: []byte("changed manifest"), ETag: "changed-generation"}) + + _, err = executeRestorePlan(context.Background(), cfg, current, plan, nil, fake) + if err == nil || !strings.Contains(err.Error(), "generation mismatch") { + t.Fatalf("executeRestorePlan() error = %v, want generation mismatch", err) + } + if got := fakeDownloadCount(fake, current.CurrentManifestKey); got != 1 { + t.Fatalf("manifest downloads = %d, want no second transfer for rejected candidate", got) + } +} + func TestCommittedStatusReportsOnlyDeclaredPublishedOutputs(t *testing.T) { cfg := restorePlanConfig(t) cfg.Pipeline.Publish = &config.PublishConfig{Outputs: []config.PublishOutputRule{{ @@ -252,6 +292,16 @@ func remoteRestoreArtifact(fake *storage.FakeBackend, artifactType artifacts.Rem } } +func fakeDownloadBytes(fake *storage.FakeBackend, key string) int64 { + var count int64 + for _, call := range fake.Downloads { + if call.Key == key { + count += call.Bytes + } + } + return count +} + func restoreCommitSHA256(data []byte) string { sum := sha256.Sum256(data) return hex.EncodeToString(sum[:]) diff --git a/internal/app/restore_discovery.go b/internal/app/restore_discovery.go index d5b2abf..b2cfdcc 100644 --- a/internal/app/restore_discovery.go +++ b/internal/app/restore_discovery.go @@ -21,6 +21,7 @@ type RemoteCurrentState struct { SessionID string Campaign string Manifest *manifest.Manifest + ManifestData []byte Commit *artifacts.RemoteCommitManifest } @@ -60,6 +61,7 @@ func discoverRemoteCurrentState(ctx context.Context, cfg *config.Config, store s SessionID: strings.TrimSpace(current.Manifest.SessionID), Campaign: strings.TrimSpace(current.Manifest.Campaign), Manifest: current.Manifest, + ManifestData: append([]byte(nil), current.ManifestData...), Commit: current.Commit, }, nil } diff --git a/internal/app/restore_execute.go b/internal/app/restore_execute.go index 7436828..7dca615 100644 --- a/internal/app/restore_execute.go +++ b/internal/app/restore_execute.go @@ -1,6 +1,7 @@ package app import ( + "bytes" "context" "crypto/sha256" "encoding/hex" @@ -104,6 +105,10 @@ func executeRestoreDownloadAction( return fmt.Errorf("create destination directory: %w", err) } temporary, err := fileops.DownloadToSiblingTemp(safeLocalPath, func(destination io.Writer) error { + if len(action.VerifiedContent) > 0 { + _, err := io.Copy(destination, bytes.NewReader(action.VerifiedContent)) + return err + } return storage.DownloadTo(ctx, store, action.RemoteKey, destination) }) if err != nil { diff --git a/internal/app/restore_execution_test.go b/internal/app/restore_execution_test.go index be35400..c090506 100644 --- a/internal/app/restore_execution_test.go +++ b/internal/app/restore_execution_test.go @@ -235,6 +235,9 @@ func TestExecuteRestoreDryRunReportsPreviousCacheWithoutWriting(t *testing.T) { if !strings.Contains(stdout.String(), "previous/artifacts/session_recap.md") { t.Fatalf("stdout = %q, want planned previous-cache artifact", stdout.String()) } + if !strings.Contains(stdout.String(), "may read remote data; no session files will be written") { + t.Fatalf("stdout = %q, want dry-run remote-read notice", stdout.String()) + } sessionRoot := artifacts.SessionWorkDirForCampaign(workspaceRoot, cfg.Session.Campaign, cfg.Session.SessionID) if _, err := os.Stat(filepath.Join(sessionRoot, "previous", "artifacts", "session_recap.md")); !os.IsNotExist(err) { diff --git a/internal/app/restore_plan.go b/internal/app/restore_plan.go index 01efc27..9e811ee 100644 --- a/internal/app/restore_plan.go +++ b/internal/app/restore_plan.go @@ -51,6 +51,7 @@ type RestoreAction struct { Conflict bool ConflictKind RestoreConflictKind Reason string + VerifiedContent []byte } // RestorePlan is the deterministic output of restore planning. @@ -159,6 +160,9 @@ func buildCommittedRestoreActions( if err != nil { return nil, fmt.Errorf("classify committed object %q: %w", artifact.DestinationKey, err) } + if artifact.Type == artifacts.RemoteArtifactTypeSessionManifest && artifact.DestinationKey == current.CurrentManifestKey { + action.VerifiedContent = append([]byte(nil), current.ManifestData...) + } actions = append(actions, action) } return actions, nil @@ -343,7 +347,7 @@ func buildPreviousCacheRestoreActions( if len(requirements) == 0 { return nil, nil } - plan, err := previouscache.BuildPlan(ctx, cfg, sessionPaths, requirements, store) + plan, err := previouscache.Resolve(ctx, cfg, sessionPaths, requirements, store) if err != nil { return nil, fmt.Errorf("plan previous-session cache restore: %w", err) } @@ -355,6 +359,7 @@ func buildPreviousCacheRestoreActions( if err != nil { return nil, fmt.Errorf("classify previous-session cache object %q: %w", record.RemoteKey, err) } + action.VerifiedContent = append([]byte(nil), record.VerifiedContent...) actions = append(actions, action) } return actions, nil diff --git a/internal/app/restore_report.go b/internal/app/restore_report.go index adf4c78..b87ed63 100644 --- a/internal/app/restore_report.go +++ b/internal/app/restore_report.go @@ -176,6 +176,9 @@ func writeRestoreDryRunSummary(out io.Writer, report *RestoreReport) error { if _, err := fmt.Fprintf(out, "Conflicts: %d\n", report.Plan.Conflicts); err != nil { return err } + if _, err := fmt.Fprintln(out, "Remote current-state and object-identity checks may read remote data; no session files will be written."); err != nil { + return err + } for _, action := range report.Actions { line := "" switch action.Status { diff --git a/internal/artifacts/current_state.go b/internal/artifacts/current_state.go index 238112c..dd3332b 100644 --- a/internal/artifacts/current_state.go +++ b/internal/artifacts/current_state.go @@ -46,6 +46,7 @@ type CurrentState struct { CurrentPointerKey string RunID string Manifest *manifest.Manifest + ManifestData []byte Commit *RemoteCommitManifest Pointer *CurrentCommitPointer } diff --git a/internal/artifacts/current_state_commit.go b/internal/artifacts/current_state_commit.go index 7a8dc7a..e0bab04 100644 --- a/internal/artifacts/current_state_commit.go +++ b/internal/artifacts/current_state_commit.go @@ -67,6 +67,7 @@ func loadCommittedCurrentState( CurrentManifestKey: sessionManifest.DestinationKey, RunID: commit.RunID, Manifest: m, + ManifestData: append([]byte(nil), manifestData...), Commit: commit, Pointer: pointer, } diff --git a/internal/previouscache/previouscache.go b/internal/previouscache/previouscache.go index 11321c1..10946a8 100644 --- a/internal/previouscache/previouscache.go +++ b/internal/previouscache/previouscache.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "path" "path/filepath" "sort" "strings" @@ -40,9 +39,13 @@ type Record struct { Size int64 Generation string S3Bucket string + VerifiedContent []byte } -func BuildPlan( +// Resolve evaluates previous-session requirements against the one selected +// remote current state. Optional absence is represented in SkippedMissing; +// required absence returns an error. +func Resolve( ctx context.Context, cfg *config.Config, paths artifacts.SessionPaths, @@ -137,45 +140,44 @@ func BuildPlan( manifestRecord.SHA256 = committedManifest.SHA256 manifestRecord.Size = committedManifest.Size manifestRecord.Generation = committedManifest.Generation + manifestRecord.VerifiedContent = append([]byte(nil), current.ManifestData...) } result.Records = append(result.Records, manifestRecord) for _, requirement := range orderedRequirements { - candidates := artifactRelativePathCandidates(requirement.Name, previousManifest, cfg) - if len(candidates) == 0 { - if requirement.Required { - return nil, fmt.Errorf( - "required previous-session artifact %q is unavailable in previous-session manifest/published-state", - requirement.Name, - ) - } - result.SkippedMissing = append(result.SkippedMissing, requirement.Name) - continue - } - selectedRel := "" selectedKey := "" var selectedArtifact *artifacts.RemoteArtifact - for _, candidate := range candidates { - if current.Commit != nil { - artifact, ok := committedPublishedArtifact(current.Commit, previousSessionPrefix, candidate) - if !ok { - continue + if current.Commit != nil { + sourceID := previousArtifactSourceID(requirement.Name) + artifact, ok := committedPublishedArtifactBySource(current.Commit, sourceID) + if ok { + selectedRel, ok = sourceRelativePath(sourceID, requirement.Name, previousManifest, cfg) + if ok { + selectedKey = artifact.DestinationKey + selectedArtifact = &artifact } - selectedRel = candidate - selectedKey = artifact.DestinationKey - selectedArtifact = &artifact - break } - remoteKey := artifacts.S3PublishedOutputKey(previousSessionPrefix, candidate) - exists, err := store.Exists(ctx, remoteKey) - if err != nil { - return nil, fmt.Errorf("check previous-session artifact object %q: %w", remoteKey, err) + } else { + // Legacy current state predates immutable source-to-destination + // mappings. Keep its compatibility candidates isolated here. + matchedCandidates := make([]string, 0, 1) + for _, candidate := range legacyArtifactRelativePathCandidates(requirement.Name, previousManifest, cfg) { + remoteKey := artifacts.S3PublishedOutputKey(previousSessionPrefix, candidate) + exists, err := store.Exists(ctx, remoteKey) + if err != nil { + return nil, fmt.Errorf("check previous-session artifact object %q: %w", remoteKey, err) + } + if exists { + matchedCandidates = append(matchedCandidates, candidate) + if len(matchedCandidates) == 1 { + selectedRel = candidate + selectedKey = remoteKey + } + } } - if exists { - selectedRel = candidate - selectedKey = remoteKey - break + if len(matchedCandidates) > 1 { + return nil, fmt.Errorf("legacy previous-session artifact %q matches multiple published candidates: %s", requirement.Name, strings.Join(matchedCandidates, ", ")) } } if selectedRel == "" { @@ -224,13 +226,27 @@ func BuildPlan( return result, nil } -func committedPublishedArtifact(commit *artifacts.RemoteCommitManifest, sessionPrefix, relativePath string) (artifacts.RemoteArtifact, bool) { +// BuildPlan is retained for callers that need the historical name. +func BuildPlan( + ctx context.Context, + cfg *config.Config, + paths artifacts.SessionPaths, + requirements []artifacts.PreviousArtifactRequirement, + store storage.ObjectStore, +) (*Plan, error) { + return Resolve(ctx, cfg, paths, requirements, store) +} + +func committedPublishedArtifactBySource(commit *artifacts.RemoteCommitManifest, source string) (artifacts.RemoteArtifact, bool) { if commit == nil { return artifacts.RemoteArtifact{}, false } - want := artifacts.S3RunRelativeDestinationKey(artifacts.S3RunPrefix(sessionPrefix, commit.RunID), relativePath) + want := strings.TrimSpace(source) + if want == "" { + return artifacts.RemoteArtifact{}, false + } for _, artifact := range commit.Artifacts { - if artifact.Type == artifacts.RemoteArtifactTypePublishedOutput && artifact.DestinationKey == want { + if artifact.Type == artifacts.RemoteArtifactTypePublishedOutput && artifact.Source == want { return artifact, true } } @@ -260,7 +276,30 @@ func optionalPreviousArtifactNames(requirements []artifacts.PreviousArtifactRequ return names } -func artifactRelativePathCandidates( +func previousArtifactSourceID(artifactName string) string { + sourceID := artifactpolicy.ConfiguredSourceID(artifactName) + if sourceDescriptor, err := artifactpolicy.PreviousSessionSourceDescriptorForConfiguredKey(artifactName); err == nil { + sourceID = sourceDescriptor.ConfiguredSourceID + } + return sourceID +} + +func sourceRelativePath(sourceID, artifactName string, previousManifest *manifest.Manifest, cfg *config.Config) (string, bool) { + if rel, ok := manifestArtifactRelativePathBySourceID(previousManifest, sourceID); ok { + return rel, true + } + if cfg != nil && cfg.Pipeline != nil && cfg.Pipeline.Scriptorium != nil { + if artifactCfg, ok := cfg.Pipeline.Scriptorium.Artifacts[artifactName]; ok { + normalized, err := pathsafe.NormalizeRelativeDestination(artifactCfg.OutputPath) + if err == nil { + return normalized, true + } + } + } + return "", false +} + +func legacyArtifactRelativePathCandidates( artifactName string, previousManifest *manifest.Manifest, cfg *config.Config, @@ -274,16 +313,12 @@ func artifactRelativePathCandidates( candidates = append(candidates, normalized) } - sourceDescriptor, err := artifactpolicy.PreviousSessionSourceDescriptorForConfiguredKey(artifactName) - sourceID := artifactpolicy.ConfiguredSourceID(artifactName) - if err == nil { - sourceID = sourceDescriptor.ConfiguredSourceID - } + sourceID := previousArtifactSourceID(artifactName) if rel, ok := manifestArtifactRelativePathBySourceID(previousManifest, sourceID); ok { appendCandidate(rel) - base := path.Base(rel) + base := filepath.Base(rel) for _, published := range manifestPublishedPaths(previousManifest) { - if path.Base(published) == base { + if filepath.Base(published) == base { appendCandidate(published) } } diff --git a/internal/previouscache/previouscache_test.go b/internal/previouscache/previouscache_test.go index 16e82a3..bb405ac 100644 --- a/internal/previouscache/previouscache_test.go +++ b/internal/previouscache/previouscache_test.go @@ -2,6 +2,8 @@ package previouscache import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "path/filepath" "strings" @@ -58,6 +60,76 @@ func TestBuildPlanFallsBackToConfiguredOutputPath(t *testing.T) { } } +func TestResolveUsesCommittedSourceMappingForCustomDestination(t *testing.T) { + cfg, paths := previousCacheTestConfig(t) + store := &storage.FakeBackend{} + previous := previousManifestWithOutput(t, cfg, "artifacts/session_recap.md", nil) + customKey := seedCommittedPreviousState(t, store, cfg, previous, []artifacts.RemoteArtifact{ + committedPublishedArtifactForTest("narratio.artifact.session_recap", "history/recap-v2.txt", []byte("# recap\n"), "recap-generation"), + committedPublishedArtifactForTest("narratio.artifact.player_handout", "other/recap-v2.txt", []byte("handout"), "handout-generation"), + }) + + plan, err := Resolve(context.Background(), cfg, paths, []artifacts.PreviousArtifactRequirement{{Name: "session_recap", Required: true}}, store) + if err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if len(plan.Records) != 2 { + t.Fatalf("records = %#v, want manifest and artifact", plan.Records) + } + if manifestRecord := findRecord(plan.Records, InputKindManifest); manifestRecord == nil || len(manifestRecord.VerifiedContent) == 0 { + t.Fatalf("manifest record = %#v, want retained verified content", manifestRecord) + } + artifact := findRecord(plan.Records, InputKindArtifact) + if artifact == nil { + t.Fatal("artifact record is missing") + } + if artifact.RemoteKey != customKey { + t.Fatalf("remote key = %q, want exact configured source destination %q", artifact.RemoteKey, customKey) + } + if artifact.LocalRelativePath != "previous/artifacts/session_recap.md" { + t.Fatalf("local relative path = %q, want source-local cache path", artifact.LocalRelativePath) + } +} + +func TestResolveDoesNotUseCommittedBasenameFallback(t *testing.T) { + cfg, paths := previousCacheTestConfig(t) + store := &storage.FakeBackend{} + previous := previousManifestWithOutput(t, cfg, "artifacts/session_recap.md", nil) + seedCommittedPreviousState(t, store, cfg, previous, []artifacts.RemoteArtifact{ + committedPublishedArtifactForTest("narratio.artifact.player_handout", "history/session_recap.md", []byte("wrong source"), "handout-generation"), + }) + + _, err := Resolve(context.Background(), cfg, paths, []artifacts.PreviousArtifactRequirement{{Name: "session_recap", Required: true}}, store) + if err == nil || !strings.Contains(err.Error(), `required previous-session artifact "session_recap" object missing`) { + t.Fatalf("Resolve() error = %v, want source-mapping absence", err) + } +} + +func TestResolveRejectsAmbiguousLegacyCandidates(t *testing.T) { + cfg, paths := previousCacheTestConfig(t) + store := &storage.FakeBackend{} + seedPreviousCurrent(t, store, cfg, previousManifestWithOutput(t, cfg, "artifacts/session_recap.md", []string{"archive/session_recap.md"})) + prefix := artifacts.S3SessionPrefix("dnd", cfg.Session.Campaign, cfg.Session.PreviousSessionID) + store.SeedObject(storage.FakeObject{Key: prefix + "artifacts/session_recap.md", Data: []byte("source candidate")}) + store.SeedObject(storage.FakeObject{Key: prefix + "archive/session_recap.md", Data: []byte("published candidate")}) + + _, err := Resolve(context.Background(), cfg, paths, []artifacts.PreviousArtifactRequirement{{Name: "session_recap", Required: true}}, store) + if err == nil || !strings.Contains(err.Error(), "matches multiple published candidates") { + t.Fatalf("Resolve() error = %v, want legacy ambiguity error", err) + } +} + +func TestResolveHonorsCancellation(t *testing.T) { + cfg, paths := previousCacheTestConfig(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := Resolve(ctx, cfg, paths, []artifacts.PreviousArtifactRequirement{{Name: "session_recap", Required: true}}, &storage.FakeBackend{}) + if err == nil || !strings.Contains(err.Error(), "context canceled") { + t.Fatalf("Resolve() error = %v, want cancellation", err) + } +} + func TestBuildPlanSkipsMissingOptionalPreviousArtifact(t *testing.T) { cfg, paths := previousCacheTestConfig(t) store := &storage.FakeBackend{} @@ -238,3 +310,91 @@ func recordRelPaths(records []Record) []string { } return out } + +func findRecord(records []Record, kind string) *Record { + for index := range records { + if records[index].Kind == kind { + return &records[index] + } + } + return nil +} + +func seedCommittedPreviousState(t *testing.T, store *storage.FakeBackend, cfg *config.Config, m *manifest.Manifest, published []artifacts.RemoteArtifact) string { + t.Helper() + prefix := artifacts.S3SessionPrefix(cfg.Pipeline.Storage.S3.RootPrefix, cfg.Session.Campaign, cfg.Session.PreviousSessionID) + runID := "previous-run" + manifestKey := artifacts.S3RunSessionManifestKey(prefix, runID) + manifestData, err := marshalManifestForPreviousCacheTest(m) + if err != nil { + t.Fatalf("marshal previous manifest: %v", err) + } + manifestGeneration := "manifest-generation" + commit := artifacts.RemoteCommitManifest{ + FormatVersion: artifacts.RemoteCommitFormatVersion, + Campaign: cfg.Session.Campaign, + SessionID: cfg.Session.PreviousSessionID, + RunID: runID, + Artifacts: append([]artifacts.RemoteArtifact{{ + Type: artifacts.RemoteArtifactTypeSessionManifest, + Source: "session.manifest", + DestinationKey: manifestKey, + SHA256: sha256ForPreviousCacheTest(manifestData), + Size: int64(len(manifestData)), + Generation: manifestGeneration, + }}, published...), + } + commitData, err := artifacts.EncodeRemoteCommitManifest(commit) + if err != nil { + t.Fatalf("encode committed previous state: %v", err) + } + commitKey := artifacts.S3RunCommitKey(prefix, runID) + commitGeneration := "commit-generation" + pointerData, err := artifacts.EncodeCurrentCommitPointer(artifacts.CurrentCommitPointer{ + FormatVersion: artifacts.RemoteCommitFormatVersion, + Campaign: cfg.Session.Campaign, + SessionID: cfg.Session.PreviousSessionID, + RunID: runID, + CommitKey: commitKey, + CommitSHA256: sha256ForPreviousCacheTest(commitData), + CommitSize: int64(len(commitData)), + CommitGeneration: commitGeneration, + }) + if err != nil { + t.Fatalf("encode committed previous pointer: %v", err) + } + + store.SeedObject(storage.FakeObject{Key: artifacts.S3CurrentCommitPointerKey(prefix), Data: pointerData, ETag: "pointer-generation"}) + store.SeedObject(storage.FakeObject{Key: commitKey, Data: commitData, ETag: commitGeneration}) + store.SeedObject(storage.FakeObject{Key: manifestKey, Data: manifestData, ETag: manifestGeneration}) + for _, artifact := range published { + store.SeedObject(storage.FakeObject{Key: artifact.DestinationKey, Data: publishedDataForTest(artifact.Source), ETag: artifact.Generation}) + } + return published[0].DestinationKey +} + +func committedPublishedArtifactForTest(source, destination string, data []byte, generation string) artifacts.RemoteArtifact { + return artifacts.RemoteArtifact{ + Type: artifacts.RemoteArtifactTypePublishedOutput, + Source: source, + DestinationKey: "dnd/campaigns/sample-campaign/sessions/2026-04-26/runs/previous-run/published/" + destination, + SHA256: sha256ForPreviousCacheTest(data), + Size: int64(len(data)), + Generation: generation, + } +} + +func publishedDataForTest(source string) []byte { + if source == "narratio.artifact.session_recap" { + return []byte("# recap\n") + } + if source == "narratio.artifact.player_handout" { + return []byte("handout") + } + return []byte("published") +} + +func sha256ForPreviousCacheTest(data []byte) string { + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/stage/prepare_previous.go b/internal/stage/prepare_previous.go index 5705e79..02e551f 100644 --- a/internal/stage/prepare_previous.go +++ b/internal/stage/prepare_previous.go @@ -1,6 +1,7 @@ package stage import ( + "bytes" "context" "fmt" "io" @@ -43,7 +44,7 @@ func hydratePreviousSessionArtifacts( if env.ArtifactStore == nil { return nil, fmt.Errorf("artifact store is required") } - plan, err := previouscache.BuildPlan(ctx, env.Config, paths, requirements, env.ObjectStore) + plan, err := previouscache.Resolve(ctx, env.Config, paths, requirements, env.ObjectStore) if err != nil { return nil, err } @@ -58,6 +59,10 @@ func hydratePreviousSessionArtifacts( } if err := fileops.DownloadAndInstall(record.LocalPath, fileops.WorkspaceFileMode, func(destination io.Writer) error { + if len(record.VerifiedContent) > 0 { + _, err := io.Copy(destination, bytes.NewReader(record.VerifiedContent)) + return err + } return storage.DownloadTo(ctx, env.ObjectStore, record.RemoteKey, destination) }); err != nil { return nil, err