From 1716702c99c8ce808e7f5c5cd910df5b4ad46e4b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 2 Aug 2026 13:11:41 +0000 Subject: [PATCH] Make prompt debug creation concurrency safe --- docs/roadmap/implementation.md | 1049 +++++++-------------- internal/app/comparison_execution_test.go | 57 +- internal/promptdebug/debug_writer.go | 18 +- internal/promptdebug/debug_writer_test.go | 69 ++ 4 files changed, 453 insertions(+), 740 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 2a87c09..a23202e 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,774 +1,372 @@ # LLM Profile Comparison Implementation Plan -Status: Complete. +Status: Follow-up work planned after post-implementation review. ## Purpose And Authority This document is the ordered implementation plan for the accepted [LLM -Profile Comparison Roadmap](profile-comparison.md). Implement every stage in -numeric order. The roadmap owns feature purpose, policy, scope, and target -state; this plan owns implementation sequence, concrete interfaces, and stage -exit criteria. +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. -This is one feature delivered over ten bounded coding prompts. A stage may -refactor code needed by a later stage, but it must leave the repository -compiling, tested, and internally coherent. Do not expose an incomplete -`compare` command before the stages that implement its full command contract. +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. ## Implementation Rules -Apply these rules in every stage: +Apply these rules in every remaining stage: -- Read `docs/development.md`, the task-specific documents it identifies, and - all files under `docs/policy/` before changing code. Treat those documents - and the feature roadmap as binding. -- Preserve the existing behavior of `generate` and `run`, including output - precedence, report/date semantics, notification, debug capture, structured - results, and exit behavior. +- Read `docs/development.md`, the task-specific documents it identifies, all + files under `docs/policy/`, and the feature roadmap before changing code. +- Preserve the existing `generate`, `run`, and `compare` command contracts + except for the explicit comparison corrections defined below. - Keep Promptkit types and calls behind `internal/adapters/promptkit` and the - dependency-neutral `internal/promptexec` interface. Do not import Promptkit - from application, CLI, or comparison-artifact packages. + dependency-neutral `internal/promptexec` interface. - Keep comparison artifacts operator-owned and explicit. They are not durable application state and must never be discovered or consumed implicitly by a later invocation. -- Do not add a Weatherreporter concurrency limit or semaphore. One shared - Promptkit executor is safe for concurrent use, and Promptkit v0.5.0 owns its - engine-local backend capacity. -- Use dependency injection and deterministic fakes for application and CLI - tests. Tests must be offline, credential-free, race-safe, and independent of - completion timing and machine-specific paths. -- Add regression coverage in the same stage as each behavior. Prefer - contract-level assertions over incidental implementation detail, in - accordance with `docs/policy/testing.md`. +- Preserve comparison's prepare-once, execute-concurrently, order-results-by- + selection, publish-on-profile-failure, and never-notify invariants. +- Do not add a Weatherreporter concurrency limit. Promptkit owns backend + capacity. +- Never expose provider bodies, prompts, schemas, model output, endpoints, + credentials, or arbitrary wrapped error text in normal JSON summaries or + manifests. +- Use deterministic, offline, credential-free tests. Test filesystem safety, + concurrency, and recovery through the narrowest stable behavioral boundary; + do not rely on timing-only sleeps or host permission behavior. - Run `gofmt` on changed Go files and `git diff --check` in every stage. Run - focused tests while developing, then `GOWORK=off go test -count=1 ./...` - before completing the stage. Stages that add concurrency or filesystem - replacement must also run the relevant packages with `-race`. + focused tests while developing and `GOWORK=off go test -count=1 ./...` before + completing each stage. Stages involving concurrency or filesystem mutation + must also run affected packages with `-race`. - Do not commit, tag, push, or prepare a release unless the implementing prompt separately requests it. -## Locked Implementation Contracts +## Completed Stages -The following choices make the stages decision-complete. Do not substitute a -different command shape, artifact schema, publication algorithm, or concurrency -policy without first updating the accepted roadmap and this plan. +### Stage 1: Comparison Artifact And Naming Contracts -### Application Request And Result +Added the dependency-neutral comparison model, schema version, manifest +validation and encoding, safe errors, deterministic profile filenames, +comparison identities, default directory names, and content hashing. -Add this exported application request, using the existing application aliases -and interfaces: +### Stage 2: Destination Recognition And Transactional Publication -```go -type ComparisonRequest struct { - Config config.Config - Report ReportKind - ProfileIDs []string - WorkingDir string - OutputDir string - Replace bool - LLMDebugDir string - Date time.Time - Clock timeutil.Clock - Collector Collector - Executor promptexec.Executor -} -``` +Added read-only destination planning, strict recognition of current comparison +bundles, private sibling staging, guarded replacement, rollback, and atomic +directory publication. -`OutputDir` is the optional exact bundle-directory override. `Clock` supplies -the comparison identity plus start and finish timestamps and defaults to -`timeutil.SystemClock` when nil. `Date` retains the existing report-date -meaning. +### Stage 3: Ordered Multi-Profile Preflight -Do not include a notifier in this request. Making notification structurally -unavailable is clearer and safer than passing a disabled notifier through the -comparison path. +Added exact prompt inspection followed by sequential profile inspection before +weather collection, including effective backend, model, and credential checks. -Add `app.CompareDetailed(context.Context, ComparisonRequest)` returning a -non-nil `*app.ComparisonResult` whenever report and destination resolution have -progressed far enough to describe the attempted comparison. Use these field -names and meanings: +### Stage 4: Immutable Shared Report Preparation -```go -type ComparisonResult struct { - ComparisonID string - ReportID report.ID - ReportName string - PromptID string - PromptVersion string - PromptHash string - StartedAt time.Time - FinishedAt time.Time - Timezone string - ValidPeriod timeutil.Period - OutputDirectory string - ManifestPath string - DataPackagePath string - Total int - Succeeded int - Failed int - Results []ComparisonProfileResult -} +Extracted one immutable prepared-report value so comparison collection, +derivation, module construction, and data-package serialization happen once. -type ComparisonProfileResult struct { - Position int - ProfileID string - BackendID string - ModelName string - Status string - ValidationStatus promptexec.ValidationStatus - ReportPath string - LLMDebugPath string - Error *comparison.SafeError -} -``` +### Stage 5: Profile Execution And In-Memory Rendering -`OutputDirectory` is absolute once destination resolution succeeds. Manifest -and data-package paths are absolute and nonblank only after publication. -Successful report paths and any returned debug paths are also absolute. -Profile results always follow selection order. +Separated profile-specific Promptkit execution, generated-text validation, and +Markdown rendering from output publication while preserving ordinary report +generation behavior. -Profile counts describe profile outcomes only. A publication failure after all -profiles succeed therefore leaves `total == succeeded` and `failed == 0`, while -the command-level status and returned error still report failure. +### Stage 6: Concurrent Ordered Profile Execution -Reject fewer than two profile IDs, whitespace-only IDs, and exact -case-sensitive duplicates. Preserve every accepted ID byte-for-byte and in -request order; do not trim, case-fold, deduplicate, or merge it with -`config.promptkit.profile`. Validate this invariant both in the CLI and at the -application boundary. +Added one goroutine per selected profile using one shared executor and one +prepared input, deterministic debug identities, isolated profile failures, +joined cancellation, and selection-ordered results. -### Comparison And File Identity +### Stage 7: Application-Level Comparison -Use the resolved report metadata's existing run ID to form: +Added `app.CompareDetailed`, coherent complete and partial bundle construction, +aggregate profile-failure behavior, absolute published paths, and the +application-level guarantee that comparison never notifies Distributor. -```text -comparison_ -``` +### Stage 8: Compare Command Parsing -This comparison ID is computed once from the injected clock and shared by the -application result, manifest, and debug identities. +Added the `compare` command request path, repeatable ordered `--profile`, exact +`--out-dir`, guarded `--replace`, applicable common flags, validation, and one +executor construction per invocation. -Derive a profile filename slug as follows: +### Stage 9: CLI Results And Exit Behavior -1. retain ASCII letters, ASCII digits, `-`, and `_`; -2. replace each run of every other character, including dots, whitespace, - separators, control characters, and non-ASCII characters, with one `-`; -3. trim leading and trailing `-` and `_`; -4. use `profile` if the result is empty; and -5. truncate the result to 64 ASCII bytes, then trim trailing `-` and `_` again, - falling back to `profile` if necessary. +Added structured success and failure summaries, quiet-mode suppression, +ordered per-profile results, safe bounded errors, and nonzero exit behavior for +partial or command-level failure. -The report filename is: +### Stage 10: Canonical Documentation And Initial Validation -```text --.md -``` +Documented the implemented CLI, operations, Promptkit integration, comparison +bundle, application orchestration, execution, publication, architecture, and +development contracts, then passed the original repository-wide validation +gate. -The one-based ordinal width is the greater of two and the number of decimal -digits in the profile count. Filename collisions after slug normalization are -safe because the ordinal is authoritative. The exact profile ID remains the -logical identity in results and the manifest. - -Derive a default comparison directory by requiring the resolved report output -name to end in the exact `.md` suffix, removing that suffix, and prefixing the -remaining basename with `comparison-`. Resolve it beneath configured -`output.directory`, or beneath the invocation working directory when no output -directory is configured. An explicit `--out-dir` is the exact bundle directory, -resolves relative to the invocation working directory, and completely -overrides configured output. Return an absolute cleaned path. - -### Versioned Bundle Contract - -Create a dependency-neutral `internal/comparison` package. It owns deterministic -naming, the logical bundle and manifest types, validation, recognition of prior -bundles, hashing, staging, and transactional publication. It must not import -CLI, configuration, Promptkit, collectors, or report-generation packages. - -Use the schema version: - -```text -weatherreporter.comparison.v1 -``` - -The UTF-8 `comparison.json` document is indented by two spaces and ends with one -newline. Its exact top-level JSON fields, in struct declaration order, are: - -```text -schemaVersion -comparisonId -startedAt -finishedAt -reportId -validPeriod -timezone -promptId -promptVersion -promptHash -dataPackage -total -succeeded -failed -results -``` - -`validPeriod` has `start` and `end`. `dataPackage` has `path` and `sha256`. -Each ordered result has: - -```text -position -profileId -backendId (omit when empty) -modelName -status -validationStatus (omit when unavailable) -reportPath (omit for failures) -error (omit for successes) -``` - -An error object has `category` and `message`. Status values are exactly -`succeeded` and `failed`. A successful result must have a relative report path, -must not have an error, and must have passed generated-text validation. A -failed result must not have a report path and must have a safe error. Use the -dependency-neutral string returned by `promptexec.CategoryOf` when available; -use `application` for an unclassified application, validation, rendering, or -debug-callback failure. Bound a manifest or normal-summary error message to -1,024 UTF-8 bytes without splitting an encoded rune. Never serialize wrapped -causes, provider bodies, prompts, schemas, model output, endpoints, or -credentials. - -Encode all manifest timestamps using Go's standard `time.Time` JSON encoding. -Comparison start and finish timestamps are UTC; valid-period start and end -retain their resolved timezone offsets. Require nonzero timestamps, a finish -that is not before the comparison start, and a valid-period end strictly after -its start. - -Use lowercase 64-character hexadecimal SHA-256 digests. `data-package.yml` -contains the exact immutable byte slice supplied to every execution. A logical -bundle contains that data package, ordered successful Markdown report bytes, -and the complete manifest. Manifest counts and paths must agree exactly with -the ordered results and logical bundle. - -### Destination Safety And Publication - -Perform a read-only destination preflight before prompt inspection, weather -collection, directory creation, or model execution. Reject: - -- a filesystem root; -- the exact invocation working directory; -- an existing regular file or non-directory; -- an existing symlink, including a dangling symlink; -- an uninspectable target; and -- a nonempty directory unless `--replace` is present and it is a recognized - current-schema Weatherreporter comparison bundle. - -An absent target and an existing empty real directory are accepted without -`--replace`. `--replace` is also harmless for those cases. Recognition must -decode the manifest with unknown fields rejected, validate every manifest -invariant, require `data-package.yml` and every declared successful report to -be regular non-symlink files, verify the data-package digest, and require the -directory entries to be exactly the manifest, data package, and declared -successful reports. Extra files, subdirectories, links, missing files, invalid -paths, duplicate paths, or a failed digest make the directory unrecognized. -Never follow a manifest path outside the bundle. - -Re-run the destination safety check immediately before publication. Build the -entire logical bundle in memory, create the parent only when publishing, and -write a private uniquely named sibling staging directory with directory mode -`0700` and files mode `0600`. Check cancellation while staging and immediately -before the commit operation. - -For an absent target, atomically rename the completed sibling staging directory -to the target. For an existing empty or recognized target, rename the target to -a unique sibling backup, rename staging to the target, and restore the backup -if the second rename fails. If restoration also fails, retain the backup and -return an actionable joined error that identifies its path. Once this rename -transaction begins, finish commit or rollback without abandoning it because of -a concurrent cancellation. The final pre-commit context check is the -linearization point: cancellation observed before it prevents replacement; -cancellation arriving after it does not retroactively undo a completed commit. -Remove the backup after a successful replacement and clean staging artifacts on -ordinary failures without scanning or modifying unrelated paths. - -### Shared Preparation And Concurrent Execution - -Extract an immutable internal prepared-report value containing the resolved -report, collection-derived facts, module snapshot, briefing metadata, -generated-text handler/render inputs, source warnings, and the one serialized -data-package byte sequence. Preparation resolves, collects, derives, snapshots, -and marshals once. - -Split the generated-text workflow into three internal responsibilities: - -1. deterministic shared preparation; -2. one profile-specific Promptkit execution, validation, and in-memory - Markdown rendering; and -3. publication, with optional ordinary notification. - -`GenerateDetailed` composes those responsibilities once and retains its current -single-report behavior. Comparison never calls `GenerateDetailed` in a loop. - -For comparison, inspect the exact prompt once, then inspect every profile -sequentially in selection order, including effective backend, model, and -credential availability, before weather collection. Initialize an explicitly -requested debug writer before those operations so pre-execution diagnostics -retain current behavior. Any preflight failure causes no collection, model -calls, or bundle publication. - -After shared preparation, start one goroutine per profile against one shared -executor. Pass the same immutable data-package bytes and exact inspected prompt -version to every call. Preallocate an ordered outcome slice and let each -goroutine write only its own index; use synchronization to join all goroutines. -One profile failure does not cancel peers. Parent cancellation propagates to -every call, no additional calls are started after cancellation is observed, and -all started goroutines are joined before return. - -Give each execution a deterministic debug run identity: - -```text -_- -``` - -The existing debug preparation artifact remains authoritative for the exact -profile ID. Debug callback failure is a failure of only that profile and must -not collide with or cancel peers. - -After all non-cancelled outcomes finish, order them by original selection, -capture one UTC finish timestamp, and construct the complete or coherent -partial bundle. Publish partial bundles when one or more profiles fail, then -return the non-nil result with a generic aggregate error so the CLI exits -nonzero. Do not include provider detail in that aggregate error. Cancellation -or deadline expiration publishes no bundle. No comparison path constructs or -calls a Distributor notifier. - -### CLI Contract - -Add: - -```text -weatherreporter compare REPORT [options] -``` - -Support `daily`, `today`, `tomorrow`, and `hourly`, with the same date policy as -`generate`. Accept applicable existing `--config`, `--units`, `--tz`, `--date`, -`--llm-debug-dir`, and `--quiet` flags plus a repeatable custom `--profile`, the -exact-directory `--out-dir`, and boolean `--replace`. Do not accept `--out`, and -do not use the configured default Promptkit profile as a selection. Construct -exactly one executor per command invocation. - -The normal stdout JSON summary has these fields in declaration order: - -```text -command -comparisonId -reportId -reportName -promptId -promptVersion -promptHash -status -startedAt -finishedAt -timezone -validPeriod -outputDirectory -manifestPath (omit when unpublished) -dataPackagePath (omit when unpublished) -total -succeeded -failed -results -error (omit on complete success) -``` - -`command` is `compare`; command status is `succeeded` only when every profile -succeeds and publication succeeds, otherwise `failed`. Each result mirrors the -application profile result with absolute report and debug paths. Preserve -profile order. A coherent partial bundle is still a failed command and exits -nonzero. Failures before a structured result exists use the existing CLI error -path without inventing a partial JSON object. `--quiet` suppresses normal -stdout and routine progress output but never changes work, artifacts, or exit -status. - -## Stage 1: Define Comparison Artifact And Naming Contracts +## Stage 11: Make Concurrent Prompt Debug Creation Race-Safe ### Goal -Introduce the dependency-neutral logical contract without altering any command -or existing generation path. +Ensure concurrent comparison profiles can create their distinct debug runs +under one new report/date directory without spuriously failing or leaving a +test goroutine blocked. ### Work -1. Create `internal/comparison` with manifest, result, data-package reference, - valid-period, safe-error, logical-bundle, and bundle-report types matching - the locked schema. -2. Add constants for the schema version, `comparison.json`, and - `data-package.yml`, plus explicit succeeded and failed statuses. -3. Implement profile-list validation, deterministic slugging, ordinal width, - report filename construction, comparison ID construction, default directory - name derivation, SHA-256 formatting, safe UTF-8 error truncation, manifest - validation, and deterministic JSON encoding. -4. Ensure validation rejects inconsistent counts, positions, statuses, paths, - digests, timestamps, identities, successful results without reports, - failures with reports, duplicate paths, non-basename artifact paths, and - traversal-capable names. -5. Keep all filesystem mutation and application orchestration out of this +1. Update `internal/promptdebug.ensureSecureDirectory` so concurrent creation + of the same missing directory is idempotent. If `os.Mkdir` reports that the + path already exists, inspect the path with `Lstat` and accept it only when it + is the expected real directory. Continue to reject symlinks, non-directories, + unsafe modes, and every unrelated filesystem error. +2. Preserve the existing absolute-path, containment, `0700` directory, `0600` + file, and no-symlink guarantees. Do not weaken debug-root validation or make + all `EEXIST` errors successful. +3. Add a focused prompt-debug concurrency regression that starts multiple + writers beneath a shared missing ancestor, joins every goroutine, and + verifies every expected artifact and permission invariant. +4. Make comparison execution test barriers time-bounded and failure-aware. A + callback failure before executor entry must fail the test promptly rather + than leave `waitForProfileStarts` waiting forever. +5. Retain distinct deterministic debug references and profile-local debug + failure behavior. + +### Tests And Exit Criteria + +- The focused prompt-debug concurrency test passes repeatedly and with the race + detector. +- `TestExecuteComparisonProfilesUsesDistinctDeterministicDebugReferences` + cannot hang when a profile fails before reaching the fake executor. +- Run, at minimum: + + ```sh + GOWORK=off go test -count=100 ./internal/promptdebug + GOWORK=off go test -count=100 -run TestExecuteComparisonProfilesUsesDistinctDeterministicDebugReferences ./internal/app + GOWORK=off go test -race -count=1 ./internal/promptdebug ./internal/app + GOWORK=off go test -count=1 ./... + ``` + +## Stage 12: Make Replacement Authorization Commit-Safe + +### Goal + +Prevent a destination changed after the final read-only preflight from being +treated as the previously authorized empty directory or recognized bundle and +then deleted during replacement. + +### Work + +1. Replace `DestinationPlan.Exists` as the publication decision with an + explicit destination-state classification: absent, empty real directory, or + recognized current-schema bundle. Keep `Replace` in the plan so publication + can apply the same authorization policy at commit time. +2. Continue to call `PlanDestination` immediately before publication. For an + absent target, install staging with one rename; a concurrently created + target must cause that rename to fail without modifying the new target. +3. For an existing target, rename that exact filesystem entry to the unique + sibling backup before deleting or installing anything. Classify the moved + backup while it is at its stable backup path and authorize it under the + original replacement policy: + + - an empty real directory is allowed with or without `Replace`; + - a recognized current-schema comparison bundle is allowed only with + `Replace`; and + - a file, symlink, unrecognized/nonempty directory, unreadable entry, or + other classification failure is not allowed. + +4. Treat this post-move classification as the destructive-action + authorization point. If it fails, restore the moved entry to the target and + return an error without installing staging. If the target has concurrently + reappeared or restoration otherwise fails, retain the backup and return an + actionable joined or typed error that identifies its recovery path; never + delete either entry to force restoration. +5. Install staging only after the moved target has passed authorization. Never + remove a backup that did not pass post-move authorization. +6. Preserve the existing final cancellation linearization rule: cancellation + observed before the rename transaction prevents replacement; after the + transaction starts, finish commit or rollback rather than abandoning it. +7. Add a package-private filesystem-operation seam only if needed for + deterministic tests. Keep the public destination and publication APIs free + of test-only hooks. + +### Tests And Exit Criteria + +- Deterministically replace an initially accepted destination after final + preflight but before its move with each consequential unauthorized type: + unrelated nonempty directory, regular file, and symlink. Publication must + fail, staging must not become the target, and the moved entry must be restored + or retained at a reported recovery path. +- Cover an initially empty directory whose contents change before its move and + a recognized bundle swapped for an unrecognized directory. +- Retain coverage for absent targets, empty directories, recognized bundle + replacement, cancellation before commit, install failure, successful + rollback, failed rollback, and cleanup of ordinary staging failures. +- Run `GOWORK=off go test -race -count=1 ./internal/comparison` and the + repository-wide standard test command. + +## Stage 13: Represent Committed Publication Cleanup Failures Accurately + +### Goal + +Keep application and CLI results truthful when the new comparison bundle has +been committed but removal of the old sibling backup fails. + +### Work + +1. Change comparison publication to return a dependency-neutral result as well + as an error: + + ```go + type PublicationResult struct { + Committed bool + RetainedBackupPath string + } + + func Publish( + ctx context.Context, + plan DestinationPlan, + bundle LogicalBundle, + ) (PublicationResult, error) + ``` + +2. Define `Committed` as meaning the complete staged bundle is now installed + at the target. Pre-commit, staging, authorization, install, and successful- + rollback failures return `Committed == false`. A successful install returns + `Committed == true` even if later backup cleanup fails. +3. Add a typed post-commit cleanup error that unwraps its filesystem cause and + records the retained backup path for operator recovery. On this error, + return `Committed == true` and the absolute retained backup path. Do not + roll back or remove the newly committed valid bundle merely because old + backup cleanup failed. +4. In `app.CompareDetailed`, populate `ManifestPath`, `DataPackagePath`, and + successful profile `ReportPath` values whenever publication reports + `Committed == true`, before returning any cleanup error. +5. Treat post-commit cleanup failure as a command-level operational failure: + return the non-nil structured result plus an error, produce status `failed`, + and exit nonzero even though the published artifact paths are present. The + ordinary safe JSON error must not contain the raw filesystem cause or backup + path; the wrapped diagnostic returned on stderr may identify the retained + backup for recovery. +6. Keep `RetainedBackupPath` out of the versioned comparison manifest. It + describes an incomplete local transaction cleanup, not the logical bundle. + +### Tests And Exit Criteria + +- Inject a deterministic backup-removal failure after successful installation + and assert the target is the new recognized bundle, the old bundle remains + at the reported backup, `Committed` is true, and the error is inspectable by + type. +- At the application boundary, assert all committed artifact paths are + absolute and populated while the method still returns an error. +- At the CLI boundary, assert status `failed`, nonzero return, present artifact + paths, and a bounded generic safe error with no raw filesystem detail. +- Retain tests showing every pre-commit or rolled-back failure omits published + artifact paths. +- Run comparison, application, and CLI tests with `-race`, then the + repository-wide standard test command. + +## Stage 14: Complete Structured Failure Metadata And Classification + +### Goal + +Make every non-nil comparison result a reliable description of the attempted +run and preserve useful safe error categories in the top-level CLI summary. + +### Work + +1. In `app.CompareDetailed`, assign the absolute resolved + `OutputDirectory` immediately after output-directory resolution and before + destination preflight. Do not wait for `PlanDestination` to succeed. +2. Once the initial `ComparisonResult` exists, guarantee that every return path + sets a nonzero UTC `FinishedAt` that is not before `StartedAt`. Use one + centralized finalization path or a defer; do not scatter timestamp writes + across individual failures. +3. Continue to omit manifest, data-package, and report paths until publication + commits. Preserve whatever prompt identity fields have actually been + resolved; never invent a hash or profile result for a phase that did not + run. +4. Update `safeComparisonSummaryError` to use this stable mapping, always + passing messages through `comparison.NewSafeError`: + + | Error | Category | Safe message | + | --- | --- | --- | + | aggregate profile failure | `application` | existing bounded aggregate message | + | `context.Canceled` | `canceled` | `comparison canceled` | + | `context.DeadlineExceeded` | `deadline_exceeded` | `comparison deadline exceeded` | + | categorized `promptexec` error | exact `promptexec.CategoryOf` value | `comparison prompt operation failed` | + | `comparison.DestinationError` | `destination_` | `comparison destination preflight failed` | + | post-commit cleanup error | `publication_cleanup` | `comparison published but cleanup did not complete` | + | any unknown error | `application` | `comparison did not complete` | + +5. Apply the most specific mapping before a more general wrapped match. In + particular, detect the post-commit cleanup and destination types before + falling back to a nested filesystem or context cause. +6. Do not copy `DestinationError.Target`, wrapped causes, or arbitrary + `error.Error()` text into normal JSON. Detailed returned errors remain + available on stderr and through Go error inspection. + +### Tests And Exit Criteria + +- Add application tests for destination-preflight, debug initialization, + prompt-preflight, collection, and preparation failures. Whenever a non-nil + result is returned, assert an absolute output directory, nonzero ordered UTC + timestamps, and omission of unpublished artifact paths. +- Add table-driven CLI tests for every mapping row, including wrapped errors, + and assert that unsafe sentinel text cannot enter serialized output. +- Preserve existing ordered profile-level categories and safe messages. +- Run application and CLI tests with `-race`, then the repository-wide standard + test command. + +## Stage 15: Remove Temporary Seams, Reconcile Documentation, And Validate + +### Goal + +Remove review-discovered maintenance debt, document the corrected implemented +behavior in its canonical owners, and complete the release-equivalent gate. + +### Work + +1. Remove the unused `Runner.resolveComparison` wrapper. Remove the + test-oriented `Runner.executeComparison` seam if it has no production + caller, and rewrite its remaining coverage through `Runner.Run`, + `resolveComparisonAction`, or another stable behavioral boundary. +2. Remove `comparisonProfileOutcome.err` if production code still does not use + it. Keep raw failures in returned/wrapped errors or explicit internal error + types; do not retain an otherwise dead field solely for private test + assertions. +3. Correct the `internal/comparison` package comment so it describes the + package's actual ownership of both logical comparison contracts and + filesystem destination/publication behavior. +4. Update only the canonical current-state documents affected by Stages 11 + through 14: + + - `docs/internal/comparison-publication.md` owns post-move authorization, + commit state, rollback, retained backups, and cleanup mechanics; + - `docs/internal/comparison-execution.md` owns concurrency and debug-write + behavior; + - `docs/internal/app-orchestration.md` owns partial results and committed + publication error handling; + - `docs/cli.md` owns structured status, safe category, path, and exit + behavior; and + - `docs/operations.md` owns operator recovery for a retained sibling backup. + + Link rather than duplicating complete contracts, and update architecture or + integration documentation only if its existing invariant is inaccurate. +5. Mark this plan `Complete` and restore the feature roadmap's implemented + status after every exit criterion below passes. Retain or remove the two + roadmap documents only according to a later maintainer-directed roadmap + cleanup; do not archive them as a second current-state reference in this stage. ### Tests And Exit Criteria -- Table-test unusual profile IDs, empty slugs, separators, dots, controls, - Unicode, length limits, normalized collisions, and profile counts above 99. -- Round-trip the exact JSON field names, ordering, indentation, newline, - optional fields, safe errors, hashes, and manifest invariants. -- Confirm exact case-sensitive profile identity and duplicate handling. -- Run `GOWORK=off go test -count=1 ./internal/comparison` and the repository-wide - standard test command. The repository remains behaviorally unchanged. - -## Stage 2: Implement Safe Bundle Recognition And Transactional Publication - -### Goal - -Complete the isolated filesystem boundary, including output resolution and -whole-directory replacement, before application concurrency depends on it. - -### Work - -1. Add read-only destination planning and preflight to `internal/comparison`, - parameterized by the absolute working directory, exact target, and replace - authorization. -2. Implement strict current-schema prior-bundle recognition with unknown JSON - fields rejected, manifest validation, exact-entry checks, symlink rejection, - regular-file checks, containment, and digest verification. -3. Implement sibling staging, fixed permissions, cancellation checks, absent - target publication, existing-target backup and replacement, rollback, and - targeted cleanup according to the locked algorithm. -4. Add an application output resolver, alongside existing output helpers, that - applies explicit/configured/current-directory precedence and derives the - default comparison name from the resolved report output name. Do not change - ordinary output resolution. -5. Return typed or inspectable errors sufficient for application and CLI code - to add context without exposing filesystem internals as a public schema. - -### Tests And Exit Criteria - -- Cover relative and absolute explicit destinations, configured destination - precedence, all report-derived names, and an invalid default `.md` suffix. -- Cover absent, empty, regular-file, symlink, dangling-symlink, filesystem-root, - exact-working-directory, unreadable, recognized, and unrecognized targets. -- Cover extra entries, missing reports, subdirectories, manifest traversal, - unknown fields, bad counts, digest mismatch, stale-report removal, successful - replacement, second-rename failure with restoration, and cancellation before - commit. Use narrow injected filesystem hooks only where real filesystem - behavior cannot deterministically reach a consequential failure path. -- Verify a failed publication leaves the prior recognized bundle byte-for-byte - intact and does not touch sibling paths. -- Run focused tests with `-race`, then the repository-wide standard tests. - -## Stage 3: Add Ordered Multi-Profile Preflight - -### Goal - -Make prompt and profile validation reusable for comparison without collecting -weather or executing a model. - -### Work - -1. Extend `internal/app/prompt_inspection.go` with a comparison-oriented request - and result that inspect one resolved prompt exactly once and selected - profiles sequentially in request order. -2. Reuse the existing credential-lookup policy and return dependency-neutral - prompt hash/version plus effective backend and model for every profile. -3. Apply the locked application-level profile validation before any executor - inspection. -4. Fail on the first ordered preflight error with prompt/profile context, while - ensuring no collection or execution occurs. Do not silently fall back to - `config.promptkit.profile`. -5. Leave existing single and batch inspection entry points intact unless a - private shared helper can remove duplication without changing behavior. - -### Tests And Exit Criteria - -- Assert one prompt inspection, ordered profile inspections, effective - backend/model preservation, exact profile IDs, and credential failures. -- Assert invalid lists and any prompt/profile preflight failure perform no - collection or execution using explicit fakes. -- Run `GOWORK=off go test -count=1 ./internal/app` and repository-wide tests. - -## Stage 4: Extract Immutable Shared Report Preparation - -### Goal - -Create the prepare-once boundary required for comparison while preserving all -ordinary generation behavior. - -### Work - -1. Extract an unexported immutable prepared-report value in `internal/app` with - the resolved report, facts, module snapshot, briefing metadata, handler and - render inputs, source warnings, and exact serialized data-package bytes. -2. Move current collection, derivation, snapshot, metadata, prompt-input, YAML - marshal, and generated-text-handler construction into one preparation - function. -3. Refactor `GenerateDetailed` and batch generation to use that function once - per generated report without altering external requests or results. -4. Copy mutable inputs where necessary at the boundary so concurrent consumers - cannot mutate shared maps or byte slices. Treat the prepared value as - read-only after construction. -5. Do not add comparison goroutines or the CLI command in this stage. - -### Tests And Exit Criteria - -- Preserve all existing generation and batch tests. -- Add focused tests showing one collector call, one deterministic serialized - package, stable warnings/metadata, and isolation from caller mutation. -- Compare ordinary generated report bytes, result fields, debug preparation, - notification, and failure behavior before and after the refactor through - existing contract tests. -- Run application tests and repository-wide tests. - -## Stage 5: Separate Profile Execution And In-Memory Rendering From Publication - -### Goal - -Permit several profiles to consume one prepared report without duplicating or -prematurely publishing output. - -### Work - -1. Extract a profile-specific internal function that accepts the immutable - prepared report, exact prompt inspection, effective profile inspection, and - optional deterministic debug reference. -2. Have it invoke `promptexec.Executor.Execute`, preserve the existing - preparation callback contract, validate generated text, build render - context, and render Markdown to memory. Return a dependency-neutral outcome - and bytes; do not write the report or notify. -3. Extract ordinary single-report publication into a separate function that - performs the existing context check, atomic file write, and optional - Distributor notification. -4. Recompose `GenerateDetailed` from prepare, execute/render, and publish. Keep - its debug path, validation status, output path, notification, warnings, and - error wrapping unchanged. -5. Make debug callback bookkeeping local to one execution. Remove any shared - mutable workflow field that would race when the execution function is used - concurrently. - -### Tests And Exit Criteria - -- Prove execution/rendering alone performs no filesystem publication or - notification. -- Preserve ordinary success, validation failure, rendering failure, debug - callback failure, cancellation, atomic output, and notification tests. -- Confirm one ordinary generation still prepares and executes exactly once. -- Run application tests with `-race`, then repository-wide tests. - -## Stage 6: Build The Concurrent Ordered Profile Execution Core - -### Goal - -Implement and test concurrency independently of destination publication and CLI -assembly. - -### Work - -1. Add a private comparison execution coordinator in `internal/app` that takes - one immutable prepared report, one prompt inspection, ordered profile - inspections, comparison identity, optional debug writer, and one shared - executor. -2. Preallocate ordered outcomes, start one goroutine per selected profile while - observing parent cancellation, and join every started goroutine. Each - goroutine may write only its own result slot. -3. Pass the exact same data-package byte content and prompt version to every - call. Do not recollect, rebuild, marshal, retry, or use `GenerateDetailed`. -4. Continue peers after an individual execution, validation, rendering, or - debug callback failure. Convert every failure to a bounded safe structured - error while retaining a wrapped internal error for aggregate diagnosis. -5. Use requested-profile order, not completion order, for outcomes and report - filenames. Generate distinct debug identities from comparison ID, ordinal, - and safe slug. -6. On parent cancellation or deadline, propagate context, join all work, and - mark non-successful outcomes safely. Do not construct a publishable bundle - for a cancelled coordinator. - -### Tests And Exit Criteria - -- Use a barrier-based fake executor to prove overlap without sleeps and to - observe more than one in-flight call. -- Assert the exact prompt version and byte-for-byte identical data package for - eight to twelve profiles, deterministic result order under reversed - completion, and no data races. -- Assert one failure does not cancel peers, cancellation reaches all started - calls and joins them, and debug references are distinct and deterministic. -- Do not duplicate Promptkit's internal capacity tests. Retain adapter tests - showing that Weatherreporter delegates prepared execution through one - concurrency-safe engine. -- Run `GOWORK=off go test -race -count=1 ./internal/app - ./internal/adapters/promptkit`, then repository-wide tests. - -## Stage 7: Assemble Application-Level Comparison And Bundle Publication - -### Goal - -Expose the complete application use case with safe partial publication, -aggregate failure behavior, and no notification. - -### Work - -1. Add the locked exported application request, result, profile-result, and - `CompareDetailed` entry point. -2. Orchestrate in this exact order: validate request; resolve report/date and - comparison identity; resolve and preflight destination; initialize optional - debug writing; inspect prompt and every profile; collect and prepare once; - execute profiles concurrently; capture finish time; build manifest and - logical bundle; re-preflight and publish. -3. Build `data-package.yml` from the exact prepared bytes and include only - successful rendered reports. Derive manifest and application results from - the same ordered outcome source so they cannot drift. -4. On one or more profile failures, publish the coherent partial bundle, set - ordered result errors and counts, then return the non-nil result plus a - generic aggregate error such as `comparison completed with N failed - profiles`. -5. On cancellation, deadline, preflight, preparation, or publication failure, - publish nothing new. Preserve a recognized prior bundle. Return the most - complete safe result available and an error with useful operation context. -6. Populate absolute manifest, data-package, successful-report, and debug paths - only when those artifacts exist. Keep manifest paths relative basenames. -7. Ensure no Distributor notifier is accepted, created, or invoked anywhere in - this path. - -### Tests And Exit Criteria - -- Add assembled application tests for complete success, mixed profile failure, - all-profile failure, preflight failure, collection/preparation failure, - cancellation, publication failure, and authorized replacement. -- Assert prompt inspection once, profile inspection before collection, weather - collection once, serialized package reuse, completion-independent order, - exact counts/digest/paths, generic aggregate errors, and no model calls before - all preflight succeeds. -- Assert complete and partial comparison never notify and that cancellation or - publication failure preserves an existing bundle. -- Run application and comparison packages with `-race`, then repository-wide - tests. - -## Stage 8: Add Compare Command Parsing And Request Construction - -### Goal - -Introduce the complete command syntax and validation while keeping execution -injected and testable. - -### Work - -1. Extend `internal/cli.Runner` with an injectable comparison function, default - it to `app.CompareDetailed`, and add a private comparison command parser and - request-construction path. Do not route it from the public root switch until - Stage 9 can provide the complete summary and exit contract. -2. Implement a repeatable flag value that preserves every `--profile` token in - exact command-line order. -3. Parse applicable common flags plus `--out-dir` and `--replace`; reject - missing/unknown reports, unsupported date use, fewer than two profiles, - whitespace-only profiles, exact duplicates, unexpected positional - arguments, and unsupported flags such as `--out`. -4. Reuse existing configuration loading, units/timezone overrides, report/date - resolution semantics, working-directory handling, and debug-root behavior. -5. Pass explicit output override and ordered profiles to the application. Do - not merge the configured default profile. Construct one Promptkit executor - only after syntax/config validation and before the application call. -6. Return the application result and error through a private CLI execution - seam for Stage 9 to format. Do not add public root routing or help text in - this stage, so users cannot reach a command with an incomplete output - contract. - -### Tests And Exit Criteria - -- Table-test report/date behavior, repeated profiles and ordering, invalid - lists, output precedence inputs, `--replace`, common overrides, and - rejected flags/arguments. -- Assert invalid syntax and configuration do not construct an executor or call - the application. -- Call the private comparison execution seam in tests and assert one valid - invocation constructs one executor and one request with the exact selections - and paths. -- Run CLI tests and repository-wide tests. - -## Stage 9: Complete CLI Summaries, Quiet Mode, And Exit Behavior - -### Goal - -Finish the end-to-end user-facing command contract, including coherent partial -failure reporting. - -### Work - -1. Add comparison summary and ordered profile-summary types with the exact - locked JSON field order and `omitempty` behavior. -2. Add public root routing and root help text now that parsing, execution, - formatting, and exit behavior can land as one complete command. -3. Map complete, partial, cancelled, and publication-failed application results - into one normal stdout object whenever a structured result exists. Use - absolute returned paths and safe structured errors; do not serialize wrapped - internal or provider details. -4. Return success only for complete execution and publication. Return the - application error after writing a failed structured summary so the process - exits nonzero. Preserve the existing unstructured error path for failures - before a result exists. -5. Implement `--quiet` as output suppression only. It must not alter execution, - comparison artifacts, replacement, error return, or exit status. - -### Tests And Exit Criteria - -- Golden or exact-structure tests cover successful, partial, all-failed, - pre-execution, cancelled, and publication-failed summaries, field omission, - ordered results, and safe errors. -- CLI integration tests assert stdout/stderr discipline, quiet behavior, - nonzero partial failure, default and explicit output paths, and one executor - per invocation. -- Run CLI and application tests with `-race`, then repository-wide tests. - -## Stage 10: Reconcile Canonical Documentation And Run The Final Gate - -### Goal - -Document only the implemented behavior in its canonical owners, remove -roadmap-only language from current-state documentation, and perform the full -repository validation gate. - -### Work - -1. Update `docs/cli.md` with authoritative syntax, flags, profile validation, - JSON summary, quiet behavior, and exit semantics. -2. Update `docs/operations.md` with the prompt-comparison workflow, output - destination precedence by link to `docs/config.md`, sensitivity guidance, - flat bundle lifecycle, guarded replacement, partial results, cancellation, - and cleanup expectations. Do not duplicate the complete CLI reference. -3. Add `docs/integrations/comparison-bundle.md` as the canonical durable schema - and layout contract. Document the exact version, JSON fields, filenames, - path relativity, hashes, success/failure invariants, compatibility boundary, - and sensitive-content caveat. -4. Update `docs/integrations/promptkit.md` with the consumer-visible - prepare-once/execute-many boundary and Promptkit-owned backend capacity, - linking upstream rather than duplicating its concurrency reference. -5. Add or update focused `docs/internal/` documents for application comparison - orchestration, prepared report flow, concurrent execution, prompt debugging, - and bundle publication mechanics. Link to canonical CLI, configuration, - operations, integration, and architecture owners instead of restating them. -6. Update `docs/policy/architecture.md` for the implemented stateless, - concurrency, notification, and publication invariants; update - `docs/development.md` for new package/file routing; update - `docs/policy/documentation.md` only if the existing ownership table no longer - assigns every introduced topic correctly. -7. Check README and other current-state documents for navigation or stale - claims. Add only short orientation and canonical links where useful. -8. Mark the feature roadmap `Implemented; retained temporarily for - post-implementation review` and this plan `Complete`. Retain both until the - requested implementation review moves durable facts to their owners and the - maintainer authorizes roadmap cleanup. - -### Tests And Exit Criteria - -- Verify all changed relative links and fenced examples, and search for stale - claims that comparison is unavailable or planned. -- Confirm current-state docs do not duplicate full command, configuration, - schema, or implementation contracts outside their canonical owners. +- Confirm no production-only helper or field remains solely to support tests, + and no test loses meaningful behavioral coverage during cleanup. +- Verify changed relative links and fenced examples. Search current-state docs + for stale claims about comparison publication, debug behavior, results, or + recovery. - Run the release-equivalent local gate: ```sh @@ -789,12 +387,13 @@ repository validation gate. git diff --check ``` -- Run `GOWORK=off go run ./cmd/weatherreporter --help`; confirm output matches - `docs/cli.md` without requiring credentials or network access. +- Run `GOWORK=off go run ./cmd/weatherreporter --help` and + `GOWORK=off go run ./cmd/weatherreporter compare --help` without credentials + or network access, and confirm that help agrees with `docs/cli.md`. - Inspect the final diff for accidental generated artifacts, secrets, workspaces, vendored dependencies, release notes, or unrelated changes. ## Open Questions -None. The accepted feature roadmap and the locked contracts in this plan define -the decisions required to implement all ten stages. +None. The roadmap and the contracts in Stages 11 through 15 define the +remaining decisions needed to complete the corrective work. diff --git a/internal/app/comparison_execution_test.go b/internal/app/comparison_execution_test.go index bde5154..c670e29 100644 --- a/internal/app/comparison_execution_test.go +++ b/internal/app/comparison_execution_test.go @@ -26,7 +26,7 @@ func TestExecuteComparisonProfilesRunsOrderedProfilesConcurrently(t *testing.T) Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", Executor: executor, }) }() - waitForProfileStarts(t, executor, profiles) + waitForProfileStarts(t, executor, profiles, results) if executor.maximumInFlight() < 2 { t.Fatalf("maximum in-flight executions = %d, want overlap", executor.maximumInFlight()) } @@ -64,7 +64,7 @@ func TestExecuteComparisonProfilesContinuesAfterProfileFailure(t *testing.T) { Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", Executor: executor, }) }() - waitForProfileStarts(t, executor, profiles) + waitForProfileStarts(t, executor, profiles, results) for _, profile := range profiles { executor.release(profile.ProfileID) } @@ -90,7 +90,7 @@ func TestExecuteComparisonProfilesPropagatesCancellationAndJoins(t *testing.T) { Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", Executor: executor, }) }() - waitForProfileStarts(t, executor, profiles) + waitForProfileStarts(t, executor, profiles, results) cancel() result := <-results if !result.Canceled || executor.inFlightCount() != 0 { @@ -120,7 +120,7 @@ func TestExecuteComparisonProfilesUsesDistinctDeterministicDebugReferences(t *te Prepared: prepared, Inspection: comparisonInspection(prompt, profiles), ComparisonID: "comparison_daily", DebugWriter: debugWriter, Executor: executor, }) }() - waitForProfileStarts(t, executor, profiles) + waitForProfileStarts(t, executor, profiles, results) for _, profile := range profiles { executor.release(profile.ProfileID) } @@ -142,13 +142,14 @@ func TestExecuteComparisonProfilesUsesDistinctDeterministicDebugReferences(t *te } type barrierExecutor struct { - mu sync.Mutex - started chan string - releases map[string]chan struct{} - requests map[string]promptexec.ExecuteRequest - errors map[string]error - inFlight int - maximum int + mu sync.Mutex + started chan string + callbackFailures chan error + releases map[string]chan struct{} + requests map[string]promptexec.ExecuteRequest + errors map[string]error + inFlight int + maximum int } func newBarrierExecutor(profiles []ComparisonProfileInspection) *barrierExecutor { @@ -157,7 +158,7 @@ func newBarrierExecutor(profiles []ComparisonProfileInspection) *barrierExecutor releases[profile.ProfileID] = make(chan struct{}) } return &barrierExecutor{ - started: make(chan string, len(profiles)), releases: releases, + started: make(chan string, len(profiles)), callbackFailures: make(chan error, len(profiles)), releases: releases, requests: make(map[string]promptexec.ExecuteRequest, len(profiles)), errors: map[string]error{}, } } @@ -173,6 +174,7 @@ func (e *barrierExecutor) InspectProfile(context.Context, string) (promptexec.Pr func (e *barrierExecutor) Execute(ctx context.Context, req promptexec.ExecuteRequest, callback promptexec.PreparationCallback) (*promptexec.Execution, error) { stamp := time.Date(2026, 5, 29, 15, 0, 0, 0, time.UTC) if err := callback(promptexec.Preparation{PromptID: req.PromptID, PromptVersion: req.PromptVersion, PromptHash: "prompt-hash", ProfileID: req.ProfileID, BackendID: "backend-" + req.ProfileID, ModelName: "model-" + req.ProfileID, StartedAt: stamp, EndedAt: stamp}, nil); err != nil { + e.callbackFailures <- err return nil, err } e.mu.Lock() @@ -224,6 +226,16 @@ func (e *barrierExecutor) release(profileID string) { close(e.releases[profileID]) } +func (e *barrierExecutor) releaseAll() { + for _, release := range e.releases { + select { + case <-release: + default: + close(release) + } + } +} + func (e *barrierExecutor) maximumInFlight() int { e.mu.Lock() defer e.mu.Unlock() @@ -236,11 +248,28 @@ func (e *barrierExecutor) inFlightCount() int { return e.inFlight } -func waitForProfileStarts(t *testing.T, executor *barrierExecutor, profiles []ComparisonProfileInspection) { +func waitForProfileStarts(t *testing.T, executor *barrierExecutor, profiles []ComparisonProfileInspection, results <-chan comparisonExecutionResult) { t.Helper() + timeout := time.NewTimer(5 * time.Second) + defer timeout.Stop() seen := map[string]struct{}{} for range profiles { - profileID := <-executor.started + var profileID string + select { + case profileID = <-executor.started: + case err := <-executor.callbackFailures: + executor.releaseAll() + select { + case result := <-results: + t.Fatalf("comparison profile preparation failed before executor entry: %v; result: %#v", err, result) + case <-timeout.C: + t.Fatalf("comparison profile preparation failed before executor entry: %v; comparison did not finish", err) + } + case result := <-results: + t.Fatalf("comparison completed before all profiles started: %#v", result) + case <-timeout.C: + t.Fatal("timed out waiting for comparison profile starts") + } if _, duplicate := seen[profileID]; duplicate { t.Fatalf("duplicate execution start for %q", profileID) } diff --git a/internal/promptdebug/debug_writer.go b/internal/promptdebug/debug_writer.go index 5756523..2bd5b9c 100644 --- a/internal/promptdebug/debug_writer.go +++ b/internal/promptdebug/debug_writer.go @@ -269,7 +269,23 @@ func ensureSecureDirectory(path string) error { info, err := os.Lstat(current) if os.IsNotExist(err) { if err := os.Mkdir(current, debugDirectoryMode); err != nil { - return err + if !os.IsExist(err) { + return err + } + info, err = os.Lstat(current) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("directory component %q must not be a symlink", current) + } + if !info.IsDir() { + return fmt.Errorf("directory component %q is not a directory", current) + } + if err := os.Chmod(current, debugDirectoryMode); err != nil { + return err + } + continue } if err := os.Chmod(current, debugDirectoryMode); err != nil { return err diff --git a/internal/promptdebug/debug_writer_test.go b/internal/promptdebug/debug_writer_test.go index e0e09f6..ee76c51 100644 --- a/internal/promptdebug/debug_writer_test.go +++ b/internal/promptdebug/debug_writer_test.go @@ -1,10 +1,12 @@ package promptdebug import ( + "fmt" "os" "path/filepath" "runtime" "strings" + "sync" "testing" "time" @@ -91,6 +93,73 @@ func TestPromptDebugWriterAtomicallyReplacesArtifacts(t *testing.T) { } } +func TestPromptDebugWriterCreatesSharedMissingAncestorsConcurrently(t *testing.T) { + root := filepath.Join(t.TempDir(), "debug") + writer, err := NewPromptDebugWriter(root) + if err != nil { + t.Fatalf("NewPromptDebugWriter() error = %v", err) + } + + const writerCount = 8 + start := make(chan struct{}) + type writeResult struct { + directory string + err error + } + results := make(chan writeResult, writerCount) + var writers sync.WaitGroup + for index := 0; index < writerCount; index++ { + writers.Add(1) + go func(index int) { + defer writers.Done() + <-start + directory, err := writer.WritePreparation(PromptDebugRef{ + ReportID: report.Daily, ValidDate: "2026-05-29", RunID: fmt.Sprintf("run-%02d", index), + }, promptDebugPreparationFixture(), nil) + results <- writeResult{directory: directory, err: err} + }(index) + } + close(start) + + finished := make(chan struct{}) + go func() { + writers.Wait() + close(finished) + }() + select { + case <-finished: + case <-time.After(5 * time.Second): + t.Fatal("concurrent prompt debug writes did not finish") + } + close(results) + + directories := map[string]struct{}{} + for result := range results { + if result.err != nil { + t.Fatalf("WritePreparation() error = %v", result.err) + } + if _, duplicate := directories[result.directory]; duplicate { + t.Fatalf("duplicate debug directory %q", result.directory) + } + directories[result.directory] = struct{}{} + if _, err := os.Stat(filepath.Join(result.directory, "preparation.json")); err != nil { + t.Fatalf("preparation artifact %q: %v", result.directory, err) + } + } + if len(directories) != writerCount { + t.Fatalf("debug directories = %#v", directories) + } + if runtime.GOOS != "windows" { + assertPromptDebugMode(t, root, debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(root, "daily"), debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(root, "daily", "2026-05-29"), debugDirectoryMode) + for directory := range directories { + assertPromptDebugMode(t, directory, debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(directory, "preparation.json"), debugFileMode) + } + } +} + func TestPromptDebugWriterDisabledDoesNotAccessFilesystem(t *testing.T) { writer, err := NewPromptDebugWriter("") if err != nil {