diff --git a/README.md b/README.md index e9898e1..b1baa02 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,12 @@ Key points: `--review-base `, `CODE_CONVERGE_REVIEW_BASE` and `.code-converge/review-base` explicitly select the base using the normal configuration precedence. A branch already merged into the selected base has no committed delta but still reviews worktree changes; a fully clean run follows the existing clean/no-change path. It uses the model and reasoning effort resolved from the selected mode and any explicit stage overrides. +### Document review prompts + +Ordinary code review remains the default. Select at most one explicit review mode: `--review-prompt-file ` reads a regular Markdown file from an absolute or current-directory-relative path; `--review-prompt ` reads only `.code-converge/.md` (names contain letters, digits, `_` or `-`); and `--document-review` reviews only changed `.md` files in the same private snapshot, excluding `memory-bank/prompts/**`. The document mode uses `.code-converge/default.md` when it exists and otherwise uses its built-in prompt. Missing, unreadable, non-Markdown, invalid or conflicting selections exit `2` without fallback. If no eligible documents changed, no Codex review is started. Document mode is review-only: after a clean scoped review it exits successfully without publication or CI, so unrelated worktree changes cannot be shipped by the whole-worktree publication path. + +Run `code-converge init-document-review-prompt` to write the built-in document prompt to `.code-converge/default.md`; an existing file is preserved unless `--force` is supplied. In document mode, `--document-fix-prompt-file ` selects a Markdown fix instruction. It requires `--document-review` and conflicts with `--fix-prompt-file`; without it, the built-in document-fix instruction is used. + The review adapter supplies a strict JSON Schema and, after a zero Codex exit, reads only the file named by `--output-last-message`. The response must contain exactly `findings`, `overall_correctness`, `overall_explanation`, and `overall_confidence_score`; every finding must contain `title`, `body`, `confidence_score`, numeric `priority`, and `code_location` in the documented nested shape. An empty `findings` array is the only clean result. Plain text, terminal stdout/stderr, duplicate or unknown fields, invalid priorities, missing/empty/malformed files, and non-zero command exits cannot be classified as clean and produce operational exit `2`. Codex compatibility is capability-based: the configured CLI must support `exec`, `--output-schema`, and `--output-last-message`; unsupported invocations fail closed without falling back to terminal parsing. For metrics, schema priorities are normalized as follows: `0` (`P0`) → `critical`, `1` (`P1`) → `high`, `2` (`P2`) → `medium`, and `3` (`P3`) → `low`. Any other priority makes the response invalid. The public `unknown` counter remains present for event-schema compatibility and is zero for accepted structured responses. `findings_total` must equal the sum of all five counters. @@ -216,10 +222,10 @@ The required event catalog is: | --- | --- | | `run_started` | No fields beyond `ts` and `event`. | | `stage_started` | `stage`, `model`, `reasoning_effort`; also `review_phase` and `cycle` for `review` and `fix-findings`, and `review_phase` for `fix-ci`. | -| `review_completed` | `stage=review`, `model`, `reasoning_effort`, `review_phase`, `cycle`, `status=clean\|findings\|failed`, and `duration_ms`. A classified result (`clean` or `findings`) also requires all findings counters plus `review_scope=branch_and_worktree`, `review_base` (resolved commit SHA), `review_merge_base` and `review_base_source=explicit\|open_pr\|branch_merge_base\|remote_default`; on command or classification failure these fields and counters are omitted. This is the review stage's sole completion record. | +| `review_completed` | `stage=review`, `model`, `reasoning_effort`, `review_phase`, `cycle`, `status=clean\|findings\|scope_empty\|failed`, and `duration_ms`. A classified result (`clean`, `findings`, or `scope_empty`) also requires `review_scope=branch_and_worktree`, `review_base` (resolved commit SHA), `review_merge_base` and `review_base_source=explicit\|open_pr\|branch_merge_base\|remote_default`; `clean` and `findings` include all findings counters. `scope_empty` is emitted when document review has no eligible Markdown changes and terminates successfully without publication. On command or classification failure these fields and counters are omitted. This is the review stage's sole completion record. | | `stage_completed` | `stage=fix-findings\|publish\|ci\|fix-ci`, `status`, and `duration_ms`; Codex-backed stages also include model and reasoning effort. `ci` status is `success`, `skipped`, `failed`, or `timeout`; a CI timeout additionally has `timeout_ms`, the configured deadline. | | `step_completed` | `stage=publish`, `step=commit\|push\|change_request`, and `status=success\|skipped\|failed\|unknown`; CI emits its own `stage=ci` step with `success\|skipped\|failed\|timeout`. | -| `run_completed` | `status=success\|findings_remaining\|operational_failure\|ci_timeout\|ci_failure\|cancelled`, `exit_code`, and `total_duration_ms`. `cancelled` always has `exit_code=130`; `ci_timeout` has `exit_code=2`. For `findings_remaining`, also `checkpoint_status=committed_local\|no_changes\|not_attempted`; `committed_local` additionally requires percent-encoded `checkpoint_branch` and `checkpoint_commit`, while `not_attempted` requires `checkpoint_reason=fix_budget_exhausted\|pre_existing_changes`. | +| `run_completed` | `status=success\|scope_empty\|findings_remaining\|operational_failure\|ci_timeout\|ci_failure\|cancelled`, `exit_code`, and `total_duration_ms`. `scope_empty` has `exit_code=0` and means document review found no eligible Markdown changes; publication and CI are not reached. `cancelled` always has `exit_code=130`; `ci_timeout` has `exit_code=2`. For `findings_remaining`, also `checkpoint_status=committed_local\|no_changes\|not_attempted`; `committed_local` additionally requires percent-encoded `checkpoint_branch` and `checkpoint_commit`, while `not_attempted` requires `checkpoint_reason=fix_budget_exhausted\|pre_existing_changes`. | For example: diff --git a/docs/document-review.md b/docs/document-review.md new file mode 100644 index 0000000..bc37150 --- /dev/null +++ b/docs/document-review.md @@ -0,0 +1,56 @@ +# Configurable document review prompts + +`code-converge` reviews code by default. Since v1.1.0 the review stage also supports +explicit, deterministic document review and custom review prompts. + +## What is it for + +Teams that keep specifications, plans, and knowledge bases (including the Memory Bank) +in Markdown next to the code can now run the same automated review-and-fix loop on +those documents: consistency checks, contradiction detection, unresolved material +questions — reviewed by Codex with the same strict findings schema as code review. + +## Review prompt selection + +At most one selector may be given; conflicting selections exit `2` with no fallback: + +| Selector | Source | +| --- | --- | +| *(none)* | Built-in code-review prompt (unchanged default) | +| `--review-prompt-file ` | Any readable Markdown file (absolute or relative to the current directory) | +| `--review-prompt ` | Only `.code-converge/.md`; names may contain letters, digits, `_`, `-` | +| `--document-review` | `.code-converge/default.md` if it exists, otherwise the built-in document prompt | + +Named prompts live in `.code-converge/`, so they are versioned and reviewed together +with the project. Missing, unreadable, non-Markdown, or invalid selections fail +predictably with exit code `2`. + +## Document review mode + +`--document-review` reviews only changed `.md` files in the merge-base-to-worktree +snapshot, excluding `memory-bank/prompts/**`. If no eligible documents changed, the +run completes cleanly without invoking Codex. Document mode is review-only: after a +clean scoped review it exits successfully without publishing or waiting for CI. + +Bootstrap a project template with: + +```sh +code-converge init-document-review-prompt # writes .code-converge/default.md +code-converge init-document-review-prompt --force # overwrite an existing file +``` + +Findings in document mode are fixed with the built-in document-fix instruction, or +with `--document-fix-prompt-file ` (requires `--document-review`, conflicts +with `--fix-prompt-file`). + +## Examples + +```sh +code-converge --document-review +code-converge --review-prompt security-audit +code-converge --review-prompt-file ./prompts/api-review.md +code-converge --document-review --document-fix-prompt-file ./prompts/doc-fix.md +``` + +See the [root README](../README.md#document-review-prompts) for the full CLI and +configuration contract. diff --git a/internal/app/app.go b/internal/app/app.go index f88261e..05a5ef7 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "time" @@ -42,6 +43,14 @@ var globalFlagSpecs = []globalFlagSpec{ {"fix-model", "Stage overrides", "Fix-findings model.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "fix-model", &o.FixModel) }}, {"fix-reasoning-effort", "Stage overrides", "Fix-findings reasoning effort.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "fix-reasoning-effort", &o.FixEffort) }}, {"fix-prompt-file", "Stage overrides", "Fix-findings prompt file.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "fix-prompt-file", &o.FixPromptPath) }}, + {"review-prompt-file", "Stage overrides", "Explicit Markdown review prompt file.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "review-prompt-file", &o.ReviewPromptPath) }}, + {"review-prompt", "Stage overrides", "Project-local review prompt name.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "review-prompt", &o.ReviewPromptName) }}, + {"document-review", "Stage overrides", "Review changed Markdown documentation.", func(f *flag.FlagSet, o *config.Overrides) { + f.BoolVar(&o.DocumentReview, "document-review", false, "review changed Markdown documentation") + }}, + {"document-fix-prompt-file", "Stage overrides", "Markdown fix prompt for document review.", func(f *flag.FlagSet, o *config.Overrides) { + bind(f, "document-fix-prompt-file", &o.DocumentFixPromptPath) + }}, {"ci-fix-model", "Stage overrides", "CI-fix model.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "ci-fix-model", &o.CIFixModel) }}, {"ci-fix-reasoning-effort", "Stage overrides", "CI-fix reasoning effort.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "ci-fix-reasoning-effort", &o.CIFixEffort) }}, {"ci-fix-prompt-file", "Stage overrides", "CI-fix prompt file.", func(f *flag.FlagSet, o *config.Overrides) { bind(f, "ci-fix-prompt-file", &o.CIFixPromptPath) }}, @@ -106,6 +115,68 @@ func (a App) Run(ctx context.Context, args []string) int { } return updater.Run(ctx, assumeYes) } + if len(args) > 0 && args[0] == "init-document-review-prompt" { + force, err := initDocumentReviewArgs(args[1:]) + if err != nil { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } + cwd := a.Cwd + if cwd == "" { + cwd, err = os.Getwd() + if err != nil { + fmt.Fprintf(stderr, "code-converge: current directory: %v\n", err) + return workflow.ExitOperational + } + } + root, err := config.FindGitRoot(cwd) + if err != nil { + fmt.Fprintf(stderr, "code-converge: %v\n", err) + return workflow.ExitOperational + } + path := filepath.Join(root, ".code-converge", "default.md") + if info, err := os.Lstat(path); err == nil { + if !info.Mode().IsRegular() { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %s is not a regular file\n", path) + return workflow.ExitOperational + } + if !force { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %s already exists; use --force to overwrite\n", path) + return workflow.ExitOperational + } + } else if !os.IsNotExist(err) { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } + parent := filepath.Dir(path) + if info, err := os.Lstat(parent); err == nil { + if !info.IsDir() { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %s is not a real directory\n", parent) + return workflow.ExitOperational + } + } else if os.IsNotExist(err) { + if err := os.MkdirAll(parent, 0o700); err != nil { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } + } else { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } + if info, err := os.Lstat(parent); err != nil { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } else if !info.IsDir() { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %s is not a real directory\n", parent) + return workflow.ExitOperational + } + if err := writeDocumentReviewPrompt(path, []byte(config.DocumentReviewPrompt+"\n"), force); err != nil { + fmt.Fprintf(stderr, "code-converge init-document-review-prompt: %v\n", err) + return workflow.ExitOperational + } + fmt.Fprintln(stdout, path) + return workflow.ExitSuccess + } cwd := a.Cwd if cwd == "" { var err error @@ -220,14 +291,14 @@ func (a App) Run(ctx context.Context, args []string) int { } } } - reviewScope := &repository.ReviewScope{Runner: processRunner, Base: cfg.ReviewBase, Root: cfg.Root} + reviewScope := &repository.ReviewScope{Runner: processRunner, Base: cfg.ReviewBase, Root: cfg.Root, DocumentReview: cfg.DocumentReview} defer reviewScope.Close() var agentOutput func(string, []byte) if view != nil { agentOutput = logger.AgentOutput } agent := codex.Adapter{Runner: processRunner, Config: cfg, ReviewScope: reviewScope, Output: agentOutput} - w := workflow.Workflow{Config: cfg, Agent: agent, Repository: repository.Status{Runner: processRunner}, Log: &logger, Err: stderr, Now: a.Now} + w := workflow.Workflow{Config: cfg, Agent: &agent, Repository: repository.Status{Runner: processRunner}, Log: &logger, Err: stderr, Now: a.Now} return w.Run(runCtx) } @@ -237,6 +308,7 @@ func rootUsage(out io.Writer) { fmt.Fprintln(out, "Commands:") fmt.Fprintln(out, " config Show effective configuration and its sources.") fmt.Fprintln(out, " update [--yes|-y] Check for and install a newer release.") + fmt.Fprintln(out, " init-document-review-prompt [--force] Write the editable document-review prompt.") fmt.Fprintln(out, "") fmt.Fprintln(out, "Global options:") group := "" @@ -270,11 +342,24 @@ func helpCommand(out io.Writer, args []string) bool { fmt.Fprintln(out, "") fmt.Fprintln(out, "Check for and install a newer release. --yes and -y skip confirmation.") return true + case "init-document-review-prompt": + fmt.Fprintln(out, "usage: code-converge init-document-review-prompt [--force]") + return true default: return false } } +func initDocumentReviewArgs(args []string) (bool, error) { + if len(args) == 0 { + return false, nil + } + if len(args) == 1 && args[0] == "--force" { + return true, nil + } + return false, fmt.Errorf("usage: code-converge init-document-review-prompt [--force]") +} + func updateArgs(args []string) (bool, error) { if len(args) == 0 { return false, nil diff --git a/internal/app/app_test.go b/internal/app/app_test.go index bde2163..4c5ecc1 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -115,6 +115,74 @@ func TestVersionCommand(t *testing.T) { } } +func TestInitDocumentReviewPromptForceRepairsPermissions(t *testing.T) { + root, home := testRepo(t) + promptPath := filepath.Join(root, ".code-converge", "default.md") + if err := os.MkdirAll(filepath.Dir(promptPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(promptPath, []byte("old\n"), 0o644); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + code := (App{Stdout: &stdout, Stderr: &stderr, Cwd: root, Home: home}).Run(context.Background(), []string{"init-document-review-prompt", "--force"}) + if code != workflow.ExitSuccess || stderr.Len() != 0 { + t.Fatalf("code=%d stderr=%q", code, stderr.String()) + } + info, err := os.Stat(promptPath) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("prompt mode = %o, want 600", got) + } + if got, err := os.ReadFile(promptPath); err != nil || string(got) != config.DocumentReviewPrompt+"\n" { + t.Fatalf("prompt = %q, error=%v", got, err) + } +} + +func TestInitDocumentReviewPromptForceRejectsSymlink(t *testing.T) { + root, home := testRepo(t) + promptDir := filepath.Join(root, ".code-converge") + if err := os.MkdirAll(promptDir, 0o700); err != nil { + t.Fatal(err) + } + outside := filepath.Join(t.TempDir(), "outside.md") + if err := os.WriteFile(outside, []byte("must remain\n"), 0o600); err != nil { + t.Fatal(err) + } + promptPath := filepath.Join(promptDir, "default.md") + if err := os.Symlink(outside, promptPath); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var stdout, stderr bytes.Buffer + code := (App{Stdout: &stdout, Stderr: &stderr, Cwd: root, Home: home}).Run(context.Background(), []string{"init-document-review-prompt", "--force"}) + if code != workflow.ExitOperational || !strings.Contains(stderr.String(), "not a regular file") { + t.Fatalf("code=%d stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } + if got, err := os.ReadFile(outside); err != nil || string(got) != "must remain\n" { + t.Fatalf("outside prompt = %q, error=%v", got, err) + } +} + +func TestInitDocumentReviewPromptRejectsSymlinkedDirectory(t *testing.T) { + root, home := testRepo(t) + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(root, ".code-converge")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var stdout, stderr bytes.Buffer + code := (App{Stdout: &stdout, Stderr: &stderr, Cwd: root, Home: home}).Run(context.Background(), []string{"init-document-review-prompt"}) + if code != workflow.ExitOperational || !strings.Contains(stderr.String(), "not a real directory") { + t.Fatalf("code=%d stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } + if _, err := os.Stat(filepath.Join(outside, "default.md")); !os.IsNotExist(err) { + t.Fatalf("outside prompt exists or could not be checked: %v", err) + } +} + func TestRootHelpAliasesExitBeforeOperationalSetup(t *testing.T) { for _, args := range [][]string{{"-h"}, {"--help"}} { t.Run(args[0], func(t *testing.T) { diff --git a/internal/app/prompt_file.go b/internal/app/prompt_file.go new file mode 100644 index 0000000..075f23f --- /dev/null +++ b/internal/app/prompt_file.go @@ -0,0 +1,47 @@ +package app + +import ( + "fmt" + "os" + "path/filepath" +) + +// writeDocumentReviewPrompt never writes through an existing directory entry. +// In force mode it writes a sibling temporary file and renames it into place; +// rename replaces a symlink itself rather than following it. +func writeDocumentReviewPrompt(path string, data []byte, force bool) error { + if !force { + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return err + } + if _, err := file.Write(data); err != nil { + _ = file.Close() + return err + } + return file.Close() + } + + temporary, err := os.CreateTemp(filepath.Dir(path), ".default.md.tmp-*") + if err != nil { + return err + } + temporaryPath := temporary.Name() + defer os.Remove(temporaryPath) + + if err := temporary.Chmod(0o600); err != nil { + _ = temporary.Close() + return err + } + if _, err := temporary.Write(data); err != nil { + _ = temporary.Close() + return err + } + if err := temporary.Close(); err != nil { + return err + } + if err := os.Rename(temporaryPath, path); err != nil { + return fmt.Errorf("replace prompt: %w", err) + } + return nil +} diff --git a/internal/codex/adapter.go b/internal/codex/adapter.go index 09c5112..79c9feb 100644 --- a/internal/codex/adapter.go +++ b/internal/codex/adapter.go @@ -29,10 +29,11 @@ type Counts struct { func (c Counts) Total() int { return c.Critical + c.High + c.Medium + c.Low + c.Unknown } type ReviewResult struct { - Clean bool - Counts Counts - Report string - Scope repository.ReviewTarget + Clean bool + ScopeEmpty bool + Counts Counts + Report string + Scope repository.ReviewTarget } type structuredReview struct { @@ -61,13 +62,14 @@ type structuredLineRange struct { } type Adapter struct { - Runner runner.Runner - Config config.Config - ReviewScope *repository.ReviewScope - Output func(source string, data []byte) + Runner runner.Runner + Config config.Config + ReviewScope *repository.ReviewScope + Output func(source string, data []byte) + documentPaths []string } -func (a Adapter) Review(ctx context.Context) (ReviewResult, error) { +func (a *Adapter) Review(ctx context.Context) (ReviewResult, error) { if a.ReviewScope == nil { return ReviewResult{}, errors.New("review scope is required") } @@ -78,6 +80,10 @@ func (a Adapter) Review(ctx context.Context) (ReviewResult, error) { if strings.TrimSpace(target.BaseCommit) == "" || strings.TrimSpace(target.MergeBase) == "" { return ReviewResult{}, errors.New("review target requires a selected base commit and merge base") } + a.documentPaths = append(a.documentPaths[:0], target.DocumentPaths...) + if a.Config.DocumentReview && len(target.DocumentPaths) == 0 { + return ReviewResult{Clean: true, ScopeEmpty: true, Scope: target}, nil + } args, err := scopedReviewArgs(a.Config, target) if err != nil { return ReviewResult{}, err @@ -102,7 +108,7 @@ func (a Adapter) Review(ctx context.Context) (ReviewResult, error) { Args: args, Env: target.Env, UnsetEnv: target.UnsetEnv, - Stdin: reviewPrompt(target), + Stdin: reviewPrompt(target, a.Config), Output: a.output(), }); err != nil { return ReviewResult{}, err @@ -195,24 +201,52 @@ func environmentValue(environment []string, name string) (string, bool) { return "", false } -func reviewPrompt(target repository.ReviewTarget) string { - return fmt.Sprintf( +func reviewPrompt(target repository.ReviewTarget, configuration config.Config) string { + diffCommand := fmt.Sprintf("git diff --cached %s", target.MergeBase) + inspectionInstruction := "Inspect related files when needed" + if configuration.DocumentReview { + pathspecs := make([]string, 0, len(target.DocumentPaths)) + for _, path := range target.DocumentPaths { + pathspecs = append(pathspecs, shellQuote(":(top,literal)"+path)) + } + diffCommand += " -- " + strings.Join(pathspecs, " ") + inspectionInstruction = "Inspect only the eligible paths listed below when needed" + } + prompt := fmt.Sprintf( `Review the changes in the prepared private Git index. Selected base commit: %s Merge base and comparison start: %s -A scoped Git helper exposes the private snapshot only to Git commands that target the reviewed repository. Review the equivalent of git diff --cached %s so the comparison covers the merge-base-to-private-snapshot change. Inspect related files when needed, but do not modify the repository, the real index, or the worktree. +A scoped Git helper exposes the private snapshot only to Git commands that target the reviewed repository. Review the equivalent of %s so the comparison covers the merge-base-to-private-snapshot change. %s, but do not modify the repository, the real index, or the worktree. Return actionable code-review findings. Use an empty findings array when there are none. Return only the JSON object required by the supplied output schema.`, target.BaseCommit, target.MergeBase, - target.MergeBase, + diffCommand, + inspectionInstruction, ) + if configuration.ReviewPrompt != "" { + prompt += "\n\nAdditional review criteria:\n\n" + configuration.ReviewPrompt + } + if configuration.DocumentReview { + prompt += "\n\nEligible Markdown paths:\n" + strings.Join(target.DocumentPaths, "\n") + } + return prompt +} + +func shellQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" } -func (a Adapter) FixFindings(ctx context.Context, report string) error { +func (a *Adapter) FixFindings(ctx context.Context, report string) error { prompt := a.Config.FixPrompt + "\n\nReview findings to address:\n\n" + report + if a.Config.DocumentReview { + if len(a.documentPaths) == 0 { + return errors.New("document review fix scope is unavailable") + } + prompt += "\n\nDocument fix scope:\nFix only confirmed findings in the eligible Markdown paths listed below. Do not inspect or modify any other file in the worktree.\n" + strings.Join(a.documentPaths, "\n") + } _, err := a.Runner.Run(ctx, runner.Invocation{Args: append(modelArgs(a.Config.FixModel, a.Config.FixEffort), "exec", "-"), Stdin: prompt, Output: a.output()}) return err } diff --git a/internal/codex/adapter_test.go b/internal/codex/adapter_test.go index 7659d1f..89082d7 100644 --- a/internal/codex/adapter_test.go +++ b/internal/codex/adapter_test.go @@ -434,6 +434,66 @@ func TestReviewUsesOnlyFinalResponseAndPreservesTarget(t *testing.T) { } } +func TestDocumentReviewPromptScopesDiffToEligiblePaths(t *testing.T) { + prompt := reviewPrompt(repository.ReviewTarget{ + BaseCommit: "base", + MergeBase: "merge-base", + DocumentPaths: []string{ + "README.md", + "docs/it's.md", + }, + }, config.Config{DocumentReview: true}) + + for _, want := range []string{ + "git diff --cached merge-base -- ':(top,literal)README.md' ':(top,literal)docs/it'\\''s.md'", + "Eligible Markdown paths:\nREADME.md\ndocs/it's.md", + } { + if !strings.Contains(prompt, want) { + t.Errorf("document review prompt does not contain %q:\n%s", want, prompt) + } + } + if strings.Contains(prompt, "git diff --cached merge-base so") { + t.Fatalf("document review prompt contains an unscoped diff instruction:\n%s", prompt) + } +} + +func TestDocumentFixPromptScopesFixesToEligiblePaths(t *testing.T) { + r := &recordingRunner{} + a := Adapter{ + Runner: r, + Config: config.Config{DocumentReview: true, FixPrompt: "fix documents"}, + documentPaths: []string{"README.md", "docs/guide.md"}, + } + if err := a.FixFindings(context.Background(), `{"findings":[]}`); err != nil { + t.Fatal(err) + } + invocations := codexInvocations(r.invocations) + if len(invocations) != 1 { + t.Fatalf("codex invocations = %#v", invocations) + } + for _, want := range []string{ + "Fix only confirmed findings in the eligible Markdown paths listed below.", + "Do not inspect or modify any other file in the worktree.", + "README.md\ndocs/guide.md", + } { + if !strings.Contains(invocations[0].Stdin, want) { + t.Errorf("fix prompt does not contain %q:\n%s", want, invocations[0].Stdin) + } + } +} + +func TestDocumentReviewEmptyScopeReturnsExplicitResultWithoutCodex(t *testing.T) { + r := &recordingRunner{} + a := newReviewAdapter(t, r, config.Config{DocumentReview: true}) + result, err := a.Review(context.Background()) + if err != nil || !result.Clean || !result.ScopeEmpty { + t.Fatalf("review = %#v, %v", result, err) + } + if got := codexInvocations(r.invocations); len(got) != 0 { + t.Fatalf("codex invocations = %#v", got) + } +} + func TestReviewTargetValidation(t *testing.T) { for _, test := range []struct { name string diff --git a/internal/config/config.go b/internal/config/config.go index 103c96a..9bac6bb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,25 +27,29 @@ type OptionalString struct { } type Overrides struct { - LogFormat OptionalString - Heartbeat OptionalString - Color OptionalString - Mode OptionalString - MaxCycles OptionalString - MaxCIRecoveries OptionalString - CITimeout OptionalString - ReviewModel OptionalString - ReviewEffort OptionalString - FixModel OptionalString - FixEffort OptionalString - FixPromptPath OptionalString - CIFixModel OptionalString - CIFixEffort OptionalString - CIFixPromptPath OptionalString - ReviewBase OptionalString - SessionLogDir OptionalString - SessionLogRetention OptionalString - NoSessionLog bool + LogFormat OptionalString + Heartbeat OptionalString + Color OptionalString + Mode OptionalString + MaxCycles OptionalString + MaxCIRecoveries OptionalString + CITimeout OptionalString + ReviewModel OptionalString + ReviewEffort OptionalString + FixModel OptionalString + FixEffort OptionalString + FixPromptPath OptionalString + ReviewPromptPath OptionalString + ReviewPromptName OptionalString + DocumentReview bool + DocumentFixPromptPath OptionalString + CIFixModel OptionalString + CIFixEffort OptionalString + CIFixPromptPath OptionalString + ReviewBase OptionalString + SessionLogDir OptionalString + SessionLogRetention OptionalString + NoSessionLog bool } type Setting struct { @@ -72,6 +76,8 @@ type Config struct { FixModel string FixEffort string FixPrompt string + ReviewPrompt string + DocumentReview bool CIFixModel string CIFixEffort string CIFixPrompt string @@ -153,6 +159,10 @@ func Load(cwd, home string, overrides Overrides) (Config, error) { return Config{}, fmt.Errorf("resolve user home: %w", err) } } + reviewPrompt, documentReview, documentFixPrompt, err := resolveReviewPrompts(cwd, root, overrides) + if err != nil { + return Config{}, err + } projectDir := filepath.Join(root, ".code-converge") userDir := filepath.Join(home, ".code-converge") if err := rejectObsoleteFinalizeSettings(userDir, projectDir); err != nil { @@ -217,6 +227,18 @@ func Load(cwd, home string, overrides Overrides) (Config, error) { {name: "session-log-dir", file: "session-log-dir", env: "CODE_CONVERGE_SESSION_LOG_DIR", def: filepath.Join(home, ".code-converge", "session-logs"), builtIn: filepath.Join(home, ".code-converge", "session-logs"), defSource: SourceDefault, override: overrides.SessionLogDir}, {name: "session-log-retention", file: "session-log-retention", env: "CODE_CONVERGE_SESSION_LOG_RETENTION", def: "24h", builtIn: "24h", defSource: SourceDefault, override: overrides.SessionLogRetention}, } + if documentReview { + // The ordinary fix prompt is not part of document-review mode. Do not + // resolve it or expose it in config output: an invalid ordinary prompt + // must not prevent document review from starting. + filtered := specs[:0] + for _, item := range specs { + if item.name != "fix-prompt" { + filtered = append(filtered, item) + } + } + specs = filtered + } values := make(map[string]string, len(specs)) settings := make([]Setting, 0, len(specs)+4) @@ -269,16 +291,112 @@ func Load(cwd, home string, overrides Overrides) (Config, error) { } } + if documentReview { + values["fix-prompt"] = documentFixPrompt + } return Config{ Root: root, LogFormat: logFormat, Heartbeat: heartbeat, Color: color, Mode: mode, MaxCycles: maxCycles, MaxCIRecoveries: maxCI, CITimeout: ciTimeout, ReviewModel: values["review-model"], ReviewEffort: values["review-reasoning-effort"], - FixModel: values["fix-model"], FixEffort: values["fix-reasoning-effort"], FixPrompt: values["fix-prompt"], + FixModel: values["fix-model"], FixEffort: values["fix-reasoning-effort"], FixPrompt: values["fix-prompt"], ReviewPrompt: reviewPrompt, DocumentReview: documentReview, CIFixModel: values["ci-fix-model"], CIFixEffort: values["ci-fix-reasoning-effort"], CIFixPrompt: values["ci-fix-prompt"], Settings: settings, ReviewBase: values["review-base"], SessionLogDir: sessionLogDir, SessionLogRetention: sessionLogRetention, NoSessionLog: overrides.NoSessionLog, }, nil } +const DocumentReviewPrompt = `Review only the listed changed Markdown documents. Check system-engineering consistency, contradictions, unresolved material questions, and compliance with Memory Bank principles. Report findings only for the eligible documents.` +const DocumentFixPrompt = `Fix only confirmed findings in the changed Markdown documents. Preserve document intent, consistency, and Memory Bank principles.` + +func resolveReviewPrompts(cwd, root string, o Overrides) (string, bool, string, error) { + count := 0 + for _, selected := range []bool{o.ReviewPromptPath.Set, o.ReviewPromptName.Set, o.DocumentReview} { + if selected { + count++ + } + } + if count > 1 { + return "", false, "", fmt.Errorf("review prompt selectors are mutually exclusive") + } + if o.DocumentFixPromptPath.Set && !o.DocumentReview { + return "", false, "", fmt.Errorf("document-fix-prompt-file requires --document-review") + } + if o.DocumentFixPromptPath.Set && o.FixPromptPath.Set { + return "", false, "", fmt.Errorf("document-fix-prompt-file conflicts with fix-prompt-file") + } + if o.ReviewPromptPath.Set { + prompt, _, err := readMarkdownPrompt(cwd, o.ReviewPromptPath.Value) + return prompt, false, "", err + } + if o.ReviewPromptName.Set { + name := o.ReviewPromptName.Value + if !validPromptName(name) { + return "", false, "", fmt.Errorf("review-prompt must be a safe name without path separators") + } + prompt, _, err := readMarkdownPrompt(root, filepath.Join(".code-converge", name+".md")) + return prompt, false, "", err + } + if !o.DocumentReview { + return "", false, "", nil + } + prompt := DocumentReviewPrompt + defaultPath := filepath.Join(root, ".code-converge", "default.md") + if info, err := os.Lstat(defaultPath); err == nil { + if !info.Mode().IsRegular() { + return "", false, "", fmt.Errorf("read document review default: %s is not a regular file", defaultPath) + } + var readErr error + prompt, _, readErr = readMarkdownPrompt(root, filepath.Join(".code-converge", "default.md")) + if readErr != nil { + return "", false, "", readErr + } + } else if !os.IsNotExist(err) { + return "", false, "", fmt.Errorf("read document review default: %w", err) + } + fix := DocumentFixPrompt + if o.DocumentFixPromptPath.Set { + var err error + fix, _, err = readMarkdownPrompt(cwd, o.DocumentFixPromptPath.Value) + if err != nil { + return "", false, "", err + } + } + return prompt, true, fix, nil +} + +func validPromptName(name string) bool { + if name == "" { + return false + } + for i, r := range name { + if !(r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || (i > 0 && (r == '-' || r == '_'))) { + return false + } + } + return true +} + +func readMarkdownPrompt(base, value string) (string, string, error) { + path := value + if !filepath.IsAbs(path) { + path = filepath.Join(base, path) + } + if strings.ToLower(filepath.Ext(path)) != ".md" { + return "", "", fmt.Errorf("prompt file %s must be a Markdown (.md) file", path) + } + info, err := os.Stat(path) + if err != nil { + return "", "", fmt.Errorf("read prompt file %s: %w", path, err) + } + if !info.Mode().IsRegular() { + return "", "", fmt.Errorf("prompt file %s is not a regular file", path) + } + content, err := os.ReadFile(path) + if err != nil { + return "", "", fmt.Errorf("read prompt file %s: %w", path, err) + } + return string(content), path, nil +} + // rejectObsoleteFinalizeSettings makes the deliberate Finalize-stage removal // actionable. Leaving a previously supported setting silently ignored would // make an operator believe it still controls delivery behavior. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0c5d5a8..566c1a9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -170,6 +170,84 @@ func TestDefaultsAndProfileResolution(t *testing.T) { } } +func TestDocumentReviewPromptResolution(t *testing.T) { + cleanEnv(t) + root, home := repo(t) + write(t, filepath.Join(root, ".code-converge", "default.md"), "project document prompt") + cfg, err := Load(root, home, Overrides{DocumentReview: true}) + if err != nil || !cfg.DocumentReview || cfg.ReviewPrompt != "project document prompt" || cfg.FixPrompt != DocumentFixPrompt { + t.Fatalf("cfg=%#v err=%v", cfg, err) + } + outside := filepath.Join(t.TempDir(), "custom.md") + write(t, outside, "custom") + cfg, err = Load(root, home, Overrides{ReviewPromptPath: OptionalString{Value: outside, Set: true}}) + if err != nil || cfg.DocumentReview || cfg.ReviewPrompt != "custom" { + t.Fatalf("cfg=%#v err=%v", cfg, err) + } +} + +func TestDocumentReviewDanglingDefaultPromptFailsClosed(t *testing.T) { + cleanEnv(t) + root, home := repo(t) + defaultPath := filepath.Join(root, ".code-converge", "default.md") + if err := os.MkdirAll(filepath.Dir(defaultPath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("missing.md", defaultPath); err != nil { + t.Fatal(err) + } + + _, err := Load(root, home, Overrides{DocumentReview: true}) + if err == nil || !strings.Contains(err.Error(), "read document review default") { + t.Fatalf("err=%v", err) + } +} + +func TestDocumentReviewNonRegularDefaultPromptFailsClosed(t *testing.T) { + cleanEnv(t) + root, home := repo(t) + defaultPath := filepath.Join(root, ".code-converge", "default.md") + if err := os.MkdirAll(defaultPath, 0o755); err != nil { + t.Fatal(err) + } + + _, err := Load(root, home, Overrides{DocumentReview: true}) + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("err=%v", err) + } +} + +func TestDocumentReviewIgnoresOrdinaryFixPrompt(t *testing.T) { + cleanEnv(t) + root, home := repo(t) + writeConfig(t, root, "fix-prompt-file: prompts/missing.md\n") + t.Setenv("CODE_CONVERGE_FIX_PROMPT_FILE", filepath.Join(t.TempDir(), "missing.md")) + + cfg, err := Load(root, home, Overrides{DocumentReview: true}) + if err != nil { + t.Fatal(err) + } + if cfg.FixPrompt != DocumentFixPrompt { + t.Fatalf("fix prompt = %q, want document prompt", cfg.FixPrompt) + } + if source(cfg, "fix-prompt") != "" { + t.Fatalf("ordinary fix prompt should not be reported in document mode: %#v", cfg.Settings) + } +} + +func TestDocumentReviewPromptConflictsFailClosed(t *testing.T) { + cleanEnv(t) + root, home := repo(t) + _, err := Load(root, home, Overrides{DocumentReview: true, ReviewPromptName: OptionalString{Value: "x", Set: true}}) + if err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Fatalf("err=%v", err) + } + _, err = Load(root, home, Overrides{DocumentFixPromptPath: OptionalString{Value: "x.md", Set: true}}) + if err == nil || !strings.Contains(err.Error(), "requires") { + t.Fatalf("err=%v", err) + } +} + func cleanEnv(t *testing.T) { t.Helper() for _, name := range codeConvergeEnv { diff --git a/internal/event/event.go b/internal/event/event.go index 0602bfa..dad1438 100644 --- a/internal/event/event.go +++ b/internal/event/event.go @@ -508,6 +508,8 @@ func renderHuman(eventName string, fields []Field, maxCycles, maxCIRecoveries in switch values["status"] { case "clean": return fmt.Sprintf("%s: clean (%s)", prefix, d), nil + case "scope_empty": + return fmt.Sprintf("%s: scope empty (%s)", prefix, d), nil case "failed": return fmt.Sprintf("%s failed (%s)", prefix, d), nil case "findings": @@ -605,6 +607,8 @@ func renderHuman(eventName string, fields []Field, maxCycles, maxCIRecoveries in switch values["status"] { case "success": return fmt.Sprintf("Done (%s)", d), nil + case "scope_empty": + return fmt.Sprintf("Done: review scope was empty; publication was not reached (%s)", d), nil case "findings_remaining": switch values["checkpoint_status"] { case "committed_local": diff --git a/internal/event/event_test.go b/internal/event/event_test.go index 06d71f0..9078150 100644 --- a/internal/event/event_test.go +++ b/internal/event/event_test.go @@ -72,6 +72,7 @@ func TestHumanEventCatalog(t *testing.T) { {"ci start", "stage_started", []Field{F("stage", "fix-ci"), F("review_phase", "1")}, "10:04:05 [1/3] [gpt-test/high] CI recovery\n"}, {"ci done", "stage_completed", []Field{F("stage", "fix-ci"), F("review_phase", "1"), F("status", "success"), F("duration_ms", "68000")}, "10:04:05 [1/3] [gpt-test/high] CI recovery fixed (1m 8s)\n"}, {"done", "run_completed", []Field{F("status", "success"), F("exit_code", "0"), F("total_duration_ms", "525000")}, "10:04:05 Done (8m 45s)\n"}, + {"empty document scope", "run_completed", []Field{F("status", "scope_empty"), F("exit_code", "0"), F("total_duration_ms", "525000")}, "10:04:05 Done: review scope was empty; publication was not reached (8m 45s)\n"}, {"findings remain", "run_completed", []Field{F("status", "findings_remaining"), F("exit_code", "1"), F("checkpoint_status", "committed_local"), F("checkpoint_branch", "feature/checkpoints"), F("checkpoint_commit", "abc1234"), F("total_duration_ms", "525000")}, "10:04:05 Stopped: fix budget exhausted; publication was not reached; checkpoint committed locally on feature/checkpoints at abc1234 and not pushed (8m 45s, exit 1)\n"}, {"encoded checkpoint branch", "run_completed", []Field{F("status", "findings_remaining"), F("exit_code", "1"), F("checkpoint_status", "committed_local"), F("checkpoint_branch", "feature%3Da"), F("checkpoint_commit", "abc1234"), F("total_duration_ms", "525000")}, "10:04:05 Stopped: fix budget exhausted; publication was not reached; checkpoint committed locally on feature=a at abc1234 and not pushed (8m 45s, exit 1)\n"}, {"checkpoint skipped", "run_completed", []Field{F("status", "findings_remaining"), F("exit_code", "1"), F("checkpoint_status", "not_attempted"), F("checkpoint_reason", "pre_existing_changes"), F("total_duration_ms", "525000")}, "10:04:05 Stopped: fix budget exhausted; publication was not reached; checkpoint was skipped because the worktree had pre-existing changes (8m 45s, exit 1)\n"}, diff --git a/internal/repository/review.go b/internal/repository/review.go index 1a9b291..558be49 100644 --- a/internal/repository/review.go +++ b/internal/repository/review.go @@ -31,12 +31,13 @@ const scopedGitNoIndexEnvironment = "CODE_CONVERGE_SCOPED_GIT_NO_INDEX" // ReviewTarget is the resolved base and scoped Git environment used for one review. type ReviewTarget struct { - Base string - BaseCommit string - MergeBase string - Source string - Env []string - UnsetEnv []string + Base string + BaseCommit string + MergeBase string + Source string + Env []string + UnsetEnv []string + DocumentPaths []string } var gitTransportEnvironment = []string{ @@ -52,9 +53,10 @@ var gitTransportEnvironment = []string{ // ReviewScope discovers a base once and refreshes a private index before each review. // It never changes the caller's real Git index or worktree. type ReviewScope struct { - Runner runner.Runner - Base string - Root string + Runner runner.Runner + Base string + Root string + DocumentReview bool base, baseCommit, mergeBase, source string tempDir, gitWrapperDir, gitHelperDir string @@ -113,12 +115,34 @@ func (s *ReviewScope) Prepare(ctx context.Context) (ReviewTarget, error) { if err := s.snapshotWorktree(ctx); err != nil { return ReviewTarget{}, fmt.Errorf("snapshot worktree for review: %w", err) } + var paths []string + if s.DocumentReview { + paths, err = s.documentPaths(ctx) + if err != nil { + return ReviewTarget{}, err + } + } return ReviewTarget{ Base: s.base, BaseCommit: s.baseCommit, MergeBase: s.mergeBase, Source: s.source, - Env: env, UnsetEnv: reviewEnvironmentRemovals(), + Env: env, UnsetEnv: reviewEnvironmentRemovals(), DocumentPaths: paths, }, nil } +func (s *ReviewScope) documentPaths(ctx context.Context) ([]string, error) { + result, err := s.runGit(ctx, s.snapshotEnvironment(), s.rootGitArgs("diff", "--cached", "--name-only", "-z", s.mergeBase)...) + if err != nil { + return nil, fmt.Errorf("list review snapshot files: %w", err) + } + var paths []string + for _, path := range strings.Split(strings.TrimSuffix(result.Stdout, "\x00"), "\x00") { + if path == "" || !strings.HasSuffix(strings.ToLower(path), ".md") || strings.HasPrefix(path, "memory-bank/prompts/") { + continue + } + paths = append(paths, path) + } + return paths, nil +} + func (s *ReviewScope) snapshotWorktree(ctx context.Context) error { environment := s.snapshotEnvironment() if _, err := s.git(ctx, environment, s.rootGitArgs("add", "--sparse", "-A")...); err == nil { diff --git a/internal/repository/status.go b/internal/repository/status.go index 992a3e3..8ea949e 100644 --- a/internal/repository/status.go +++ b/internal/repository/status.go @@ -78,10 +78,36 @@ func (s Status) Head(ctx context.Context) (string, error) { return strings.TrimSpace(result.Stdout), nil } -// Checkpoint records a commit made during a fix stage and, when allowed, -// creates one for remaining worktree changes. initialHead must be captured -// immediately before the agent starts fixing findings. -func (s Status) Checkpoint(ctx context.Context, initialHead string, canCommit bool) (Checkpoint, error) { +// ChangedPaths returns the paths already changed before a fix stage starts. +// It is used to distinguish a dirty worktree baseline from paths introduced +// by the findings-fix agent. +func (s Status) ChangedPaths(ctx context.Context) ([]string, error) { + return s.changedPaths(ctx, "HEAD") +} + +// Checkpoint records a local fix commit, optionally restricting all resulting +// changes to the supplied repository-relative paths. The restriction is +// checked before staging so document-mode fixes cannot make git add -A capture +// unrelated worktree changes. Pre-existing paths are not exempt: a dirty +// out-of-scope worktree is rejected rather than trusted by pathname alone. +// It is independent of canCommit: dirty worktrees cannot be checkpointed, but +// their fix-stage delta must still stay within document scope. +func (s Status) Checkpoint(ctx context.Context, initialHead string, canCommit bool, eligiblePaths, _ []string) (Checkpoint, error) { + if len(eligiblePaths) > 0 { + changed, err := s.changedPaths(ctx, initialHead) + if err != nil { + return Checkpoint{}, fmt.Errorf("inspect findings checkpoint scope: %w", err) + } + allowed := make(map[string]struct{}, len(eligiblePaths)) + for _, path := range eligiblePaths { + allowed[path] = struct{}{} + } + for _, path := range changed { + if _, ok := allowed[path]; !ok { + return Checkpoint{}, fmt.Errorf("findings fix changed out-of-scope path %q", path) + } + } + } hasChanges, err := s.HasChanges(ctx) if err != nil { return Checkpoint{}, err @@ -116,6 +142,35 @@ func (s Status) Checkpoint(ctx context.Context, initialHead string, canCommit bo return Checkpoint{Created: true, Branch: branchName, Commit: commitID}, nil } +func (s Status) changedPaths(ctx context.Context, initialHead string) ([]string, error) { + if strings.TrimSpace(initialHead) == "" { + return nil, errors.New("initial checkpoint head is empty") + } + tracked, err := s.Runner.Run(ctx, runner.Invocation{Executable: "git", Args: []string{"diff", "--name-only", "--no-renames", "-z", initialHead}}) + if err != nil { + return nil, fmt.Errorf("list tracked checkpoint changes: %w", err) + } + untracked, err := s.Runner.Run(ctx, runner.Invocation{Executable: "git", Args: []string{"ls-files", "--others", "--exclude-standard", "-z"}}) + if err != nil { + return nil, fmt.Errorf("list untracked checkpoint changes: %w", err) + } + seen := make(map[string]struct{}) + var paths []string + for _, output := range []string{tracked.Stdout, untracked.Stdout} { + for _, path := range strings.Split(output, "\x00") { + if path == "" { + continue + } + if _, ok := seen[path]; ok { + continue + } + seen[path] = struct{}{} + paths = append(paths, path) + } + } + return paths, nil +} + // Publish commits only changes that appeared in a clean run, pushes through a // direct refspec (so a local tracking-ref update cannot make publication look // unsuccessful), then reuses or creates exactly one open pull request. diff --git a/internal/repository/status_test.go b/internal/repository/status_test.go index 2dfc55a..543db75 100644 --- a/internal/repository/status_test.go +++ b/internal/repository/status_test.go @@ -114,6 +114,20 @@ func TestStatusPropagatesRunnerError(t *testing.T) { } } +func TestReviewScopeDocumentPathsPreservesLeadingWhitespace(t *testing.T) { + fake := &fakeRunner{result: runner.Result{Stdout: " docs.md\x00README.md\x00memory-bank/prompts/review.md\x00"}} + scope := &ReviewScope{Runner: fake, mergeBase: "base"} + + paths, err := scope.documentPaths(context.Background()) + if err != nil { + t.Fatalf("documentPaths() error = %v", err) + } + want := []string{" docs.md", "README.md"} + if !reflect.DeepEqual(paths, want) { + t.Fatalf("documentPaths() = %#v, want %#v", paths, want) + } +} + func TestPublishUsesDirectRefspecAndReusesPR(t *testing.T) { fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { switch strings.Join(inv.Args, " ") { @@ -393,7 +407,7 @@ func TestStatusCheckpointCommitsLocallyWithoutPush(t *testing.T) { return runner.Result{}, nil } }} - checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true) + checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true, nil, nil) if err != nil || checkpoint != (Checkpoint{Created: true, Branch: "feature/checkpoints", Commit: "abc1234"}) { t.Fatalf("checkpoint=%#v err=%v", checkpoint, err) } @@ -406,12 +420,144 @@ func TestStatusCheckpointCommitsLocallyWithoutPush(t *testing.T) { func TestStatusCheckpointSkipsEmptyCommit(t *testing.T) { fake := &fakeRunner{result: runner.Result{Stdout: ""}} - checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "", true) + checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "", true, nil, nil) if err != nil || checkpoint.Created || len(fake.invocations) != 2 { t.Fatalf("checkpoint=%#v err=%v invocations=%#v", checkpoint, err, fake.invocations) } } +func TestStatusCheckpointRejectsOutOfScopeChangesBeforeStaging(t *testing.T) { + fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { + switch strings.Join(inv.Args, " ") { + case "diff --name-only --no-renames -z old-sha": + return runner.Result{Stdout: "README.md\x00internal/app/app.go\x00"}, nil + case "ls-files --others --exclude-standard -z": + return runner.Result{}, nil + default: + t.Fatalf("unexpected invocation: %#v", inv) + return runner.Result{}, nil + } + }} + _, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true, []string{"README.md"}, nil) + if err == nil || !strings.Contains(err.Error(), `out-of-scope path "internal/app/app.go"`) { + t.Fatalf("error=%v", err) + } + for _, invocation := range fake.invocations { + if len(invocation.Args) > 0 && (invocation.Args[0] == "add" || invocation.Args[0] == "commit") { + t.Fatalf("out-of-scope changes were staged: %#v", fake.invocations) + } + } +} + +func TestStatusCheckpointRejectsOutOfScopePreExistingChangesWhenCommitIsDisabled(t *testing.T) { + fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { + switch strings.Join(inv.Args, " ") { + case "diff --name-only --no-renames -z old-sha": + return runner.Result{Stdout: "README.md\x00internal/app/app.go\x00"}, nil + case "ls-files --others --exclude-standard -z": + return runner.Result{}, nil + case "status --porcelain --untracked-files=all": + return runner.Result{Stdout: " M internal/app/app.go\n M README.md\n"}, nil + case "rev-parse HEAD": + return runner.Result{Stdout: "old-sha\n"}, nil + default: + t.Fatalf("unexpected invocation: %#v", inv) + return runner.Result{}, nil + } + }} + _, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", false, []string{"README.md"}, []string{"internal/app/app.go"}) + if err == nil || !strings.Contains(err.Error(), `out-of-scope path "internal/app/app.go"`) { + t.Fatalf("error=%v", err) + } +} + +func TestStatusCheckpointRejectsNewOutOfScopeChangesWhenCommitIsDisabled(t *testing.T) { + fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { + switch strings.Join(inv.Args, " ") { + case "diff --name-only --no-renames -z old-sha": + return runner.Result{Stdout: "README.md\x00internal/app/app.go\x00"}, nil + case "ls-files --others --exclude-standard -z": + return runner.Result{}, nil + default: + t.Fatalf("unexpected invocation: %#v", inv) + return runner.Result{}, nil + } + }} + _, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", false, []string{"README.md"}, []string{"README.md"}) + if err == nil || !strings.Contains(err.Error(), `out-of-scope path "internal/app/app.go"`) { + t.Fatalf("error=%v", err) + } +} + +func containsInvocation(invocations []runner.Invocation, want string) bool { + for _, invocation := range invocations { + if strings.Join(invocation.Args, " ") == want { + return true + } + } + return false +} + +func TestStatusCheckpointAllowsEligibleTrackedAndUntrackedChanges(t *testing.T) { + fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { + switch strings.Join(inv.Args, " ") { + case "diff --name-only --no-renames -z old-sha": + return runner.Result{Stdout: "README.md\x00"}, nil + case "ls-files --others --exclude-standard -z": + return runner.Result{Stdout: "docs/new.md\x00"}, nil + case "status --porcelain --untracked-files=all": + return runner.Result{Stdout: " M README.md\n?? docs/new.md\n"}, nil + case "add -A", "commit -m chore: checkpoint review fixes": + return runner.Result{}, nil + case "rev-parse HEAD": + return runner.Result{Stdout: "new-sha\n"}, nil + case "branch --show-current": + return runner.Result{Stdout: "feature/checkpoints\n"}, nil + case "rev-parse --short HEAD": + return runner.Result{Stdout: "abc1234\n"}, nil + default: + t.Fatalf("unexpected invocation: %#v", inv) + return runner.Result{}, nil + } + }} + checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true, []string{"README.md", "docs/new.md"}, nil) + if err != nil || !checkpoint.Created { + t.Fatalf("checkpoint=%#v err=%v", checkpoint, err) + } +} + +func TestStatusCheckpointPreservesLiteralGitPathnamesInScope(t *testing.T) { + const leadingSpace = " docs.md" + const internalSpace = "docs/guide .md" + const newline = "docs/line\nbreak.md" + fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { + switch strings.Join(inv.Args, " ") { + case "diff --name-only --no-renames -z old-sha": + return runner.Result{Stdout: leadingSpace + "\x00" + internalSpace + "\x00"}, nil + case "ls-files --others --exclude-standard -z": + return runner.Result{Stdout: newline + "\x00"}, nil + case "status --porcelain --untracked-files=all": + return runner.Result{Stdout: " M " + leadingSpace + "\n?? " + internalSpace + "\n?? " + newline + "\n"}, nil + case "add -A", "commit -m chore: checkpoint review fixes": + return runner.Result{}, nil + case "rev-parse HEAD": + return runner.Result{Stdout: "new-sha\n"}, nil + case "branch --show-current": + return runner.Result{Stdout: "feature/checkpoints\n"}, nil + case "rev-parse --short HEAD": + return runner.Result{Stdout: "abc1234\n"}, nil + default: + t.Fatalf("unexpected invocation: %#v", inv) + return runner.Result{}, nil + } + }} + paths := []string{leadingSpace, internalSpace, newline} + checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true, paths, nil) + if err != nil || !checkpoint.Created { + t.Fatalf("checkpoint=%#v err=%v", checkpoint, err) + } +} + func TestStatusCheckpointPropagatesCommitFailure(t *testing.T) { fake := &scriptedRunner{t: t, run: func(inv runner.Invocation) (runner.Result, error) { switch strings.Join(inv.Args, " ") { @@ -426,7 +572,7 @@ func TestStatusCheckpointPropagatesCommitFailure(t *testing.T) { return runner.Result{}, nil } }} - _, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true) + _, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", true, nil, nil) if err == nil || !strings.Contains(err.Error(), "commit findings checkpoint") { t.Fatalf("error=%v", err) } @@ -448,7 +594,7 @@ func TestStatusCheckpointDetectsAgentCommitOnCleanWorktree(t *testing.T) { return runner.Result{}, nil } }} - checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", false) + checkpoint, err := (Status{Runner: fake}).Checkpoint(context.Background(), "old-sha", false, nil, nil) if err != nil || checkpoint != (Checkpoint{Created: true, Branch: "feature/fix", Commit: "abc1234"}) { t.Fatalf("checkpoint=%#v err=%v", checkpoint, err) } diff --git a/internal/workflow/workflow.go b/internal/workflow/workflow.go index c7ef9ba..18b102c 100644 --- a/internal/workflow/workflow.go +++ b/internal/workflow/workflow.go @@ -33,7 +33,8 @@ type Repository interface { HasChanges(context.Context) (bool, error) IsClean(context.Context) (bool, error) Head(context.Context) (string, error) - Checkpoint(context.Context, string, bool) (repository.Checkpoint, error) + ChangedPaths(context.Context) ([]string, error) + Checkpoint(context.Context, string, bool, []string, []string) (repository.Checkpoint, error) Publish(context.Context, bool) (repository.Publication, error) WaitCI(context.Context, repository.Publication) (repository.CIResult, error) } @@ -127,6 +128,9 @@ func (w *Workflow) Run(ctx context.Context) int { if review.Clean { status = "clean" } + if review.ScopeEmpty { + status = "scope_empty" + } fields := []event.Field{event.F("stage", "review"), event.F("model", w.stageModel("review")), event.F("reasoning_effort", w.stageReasoningEffort("review")), intField("review_phase", phase), intField("cycle", cycle), event.F("status", status)} if review.Scope.Source != "" { fields = append(fields, @@ -136,18 +140,23 @@ func (w *Workflow) Run(ctx context.Context) int { event.F("review_base_source", review.Scope.Source), ) } - fields = append(fields, countFields(review.Counts)...) + if !review.ScopeEmpty { + fields = append(fields, countFields(review.Counts)...) + } fields = append(fields, duration) if !w.emit("review_completed", fields...) { return w.complete("operational_failure", ExitOperational, now().Sub(runStarted)) } - + if review.ScopeEmpty { + return w.complete("scope_empty", ExitSuccess, now().Sub(runStarted)) + } if !review.Clean { if fixes >= w.Config.MaxCycles { return w.completeFindingsRemaining(now().Sub(runStarted), lastCheckpoint, fixes > 0, checkpointSkipReason) } canCheckpoint := w.Repository != nil initialHead := "" + var baselinePaths []string if w.Repository != nil { clean, err := w.Repository.IsClean(ctx) if err != nil { @@ -171,6 +180,16 @@ func (w *Workflow) Run(ctx context.Context) int { w.diagnostic("checkpoint head failed", err) return w.complete("operational_failure", ExitOperational, now().Sub(runStarted)) } + if w.Config.DocumentReview { + baselinePaths, err = w.Repository.ChangedPaths(ctx) + if err != nil { + if ctx.Err() != nil { + return w.complete("cancelled", ExitInterrupted, now().Sub(runStarted)) + } + w.diagnostic("checkpoint baseline failed", err) + return w.complete("operational_failure", ExitOperational, now().Sub(runStarted)) + } + } } stageStarted = now() if !w.emit("stage_started", event.F("stage", "fix-findings"), event.F("model", w.stageModel("fix-findings")), event.F("reasoning_effort", w.stageReasoningEffort("fix-findings")), intField("review_phase", phase), intField("cycle", cycle)) { @@ -221,7 +240,11 @@ func (w *Workflow) Run(ctx context.Context) int { return w.complete("operational_failure", ExitOperational, now().Sub(runStarted)) } if w.Repository != nil { - checkpoint, checkpointErr := w.Repository.Checkpoint(ctx, initialHead, canCheckpoint) + var eligiblePaths []string + if w.Config.DocumentReview { + eligiblePaths = review.Scope.DocumentPaths + } + checkpoint, checkpointErr := w.Repository.Checkpoint(ctx, initialHead, canCheckpoint, eligiblePaths, baselinePaths) if checkpointErr != nil { if ctx.Err() != nil { return w.complete("cancelled", ExitInterrupted, now().Sub(runStarted)) @@ -238,6 +261,13 @@ func (w *Workflow) Run(ctx context.Context) int { cycle++ continue } + if w.Config.DocumentReview { + // Document mode deliberately has no publication path. Its review + // snapshot is scoped to Markdown files, while the repository status + // and publication APIs operate on the whole worktree. Continuing here + // could therefore stage and publish unrelated source changes. + return w.complete("success", ExitSuccess, now().Sub(runStarted)) + } if w.Repository != nil { hasChanges, err := w.Repository.HasChanges(ctx) diff --git a/internal/workflow/workflow_test.go b/internal/workflow/workflow_test.go index e083ab2..6f1f862 100644 --- a/internal/workflow/workflow_test.go +++ b/internal/workflow/workflow_test.go @@ -76,8 +76,9 @@ func (r *workflowRepository) IsClean(context.Context) (bool, error) { r.clean = r.clean[1:] return value, nil } -func (*workflowRepository) Head(context.Context) (string, error) { return "head", nil } -func (*workflowRepository) Checkpoint(context.Context, string, bool) (repository.Checkpoint, error) { +func (*workflowRepository) Head(context.Context) (string, error) { return "head", nil } +func (*workflowRepository) ChangedPaths(context.Context) ([]string, error) { return nil, nil } +func (*workflowRepository) Checkpoint(context.Context, string, bool, []string, []string) (repository.Checkpoint, error) { return repository.Checkpoint{}, nil } func (r *workflowRepository) Publish(context.Context, bool) (repository.Publication, error) { @@ -127,6 +128,29 @@ func TestCleanReviewPublishesAndWaitsForCI(t *testing.T) { } } +func TestEmptyDocumentReviewScopeDoesNotPublish(t *testing.T) { + repo := &workflowRepository{changes: []bool{true}, ci: []repository.CIResult{repository.CISuccess}} + result := codex.ReviewResult{Clean: true, ScopeEmpty: true} + code, output := runWorkflow(t, config.Config{CITimeout: time.Minute, DocumentReview: true}, &workflowAgent{reviews: []codex.ReviewResult{result}}, repo) + if code != ExitSuccess || repo.publishes != 0 || repo.ciWaits != 0 { + t.Fatalf("code=%d publishes=%d waits=%d", code, repo.publishes, repo.ciWaits) + } + if !strings.Contains(output, "status=scope_empty") || !strings.Contains(output, "event=run_completed status=scope_empty exit_code=0") { + t.Fatalf("output=%s", output) + } +} + +func TestDocumentReviewDoesNotPublishAfterCleanScopedReview(t *testing.T) { + repo := &workflowRepository{changes: []bool{true}, ci: []repository.CIResult{repository.CISuccess}} + code, output := runWorkflow(t, config.Config{CITimeout: time.Minute, DocumentReview: true}, &workflowAgent{reviews: []codex.ReviewResult{cleanReview()}}, repo) + if code != ExitSuccess || repo.publishes != 0 || repo.ciWaits != 0 { + t.Fatalf("code=%d publishes=%d waits=%d", code, repo.publishes, repo.ciWaits) + } + if strings.Contains(output, "stage=publish") || strings.Contains(output, "stage=ci") { + t.Fatalf("document review reached publication or CI: %s", output) + } +} + func TestNoApplicableCISucceeds(t *testing.T) { code, output := runWorkflow(t, config.Config{CITimeout: time.Minute}, &workflowAgent{reviews: []codex.ReviewResult{cleanReview()}}, &workflowRepository{changes: []bool{true}, ci: []repository.CIResult{repository.CISkipped}}) if code != ExitSuccess || !strings.Contains(output, "stage=ci step=ci status=skipped") { diff --git a/memory-bank/features/FT-042/README.md b/memory-bank/features/FT-042/README.md new file mode 100644 index 0000000..41e8bda --- /dev/null +++ b/memory-bank/features/FT-042/README.md @@ -0,0 +1,17 @@ +--- +title: "FT-042: Configurable document review prompts" +doc_kind: feature +doc_function: index +purpose: "Navigation for issue #42's configurable document-review prompt delivery." +derived_from: + - ../../flows/feature.md + - brief.md +status: active +audience: humans_and_agents +--- + +# FT-042: Configurable document review prompts + +- [brief.md](brief.md) — canonical problem, validation decision and verification contract. +- [design.md](design.md) — selected CLI, prompt-resolution and document-scope contract. +- [implementation-plan.md](implementation-plan.md) — grounded execution and verification mapping. diff --git a/memory-bank/features/FT-042/brief.md b/memory-bank/features/FT-042/brief.md new file mode 100644 index 0000000..9f5053b --- /dev/null +++ b/memory-bank/features/FT-042/brief.md @@ -0,0 +1,98 @@ +--- +title: "FT-042: Configurable document review prompts" +doc_kind: feature +doc_function: canonical +purpose: "Canonical problem, scope, validation profile and verification contract for issue #42." +derived_from: + - ../../flows/feature.md + - ../../engineering/validation-profiles.md + - ../../../README.md + - https://github.com/dapi/code-converge/issues/42 +status: active +delivery_status: done +audience: humans_and_agents +must_not_define: + - implementation_sequence + - solution_space +--- + +# FT-042: Configurable document review prompts + +## What + +The review stage currently has one built-in code-review instruction. Operators need an explicit, deterministic document-review mode and explicit prompt selection without replacing ordinary code review by default. + +## Scope + +- `REQ-01` The CLI accepts at most one explicit review-prompt selection: a Markdown prompt path, a safe project-local prompt name, or document-review mode; no selector retains ordinary code review, while conflicting selections fail predictably with exit code `2` and no implicit fallback. +- `REQ-02` A named review prompt resolves only from `.code-converge/.md`; invalid names and unreadable or missing selected files fail predictably with exit code `2`. +- `REQ-03` Document-review mode reviews only changed Markdown files in the current merge-base-to-worktree snapshot, excludes `memory-bank/prompts/**`, reports a clean, explicit empty scope without invoking Codex when none remain, and never publishes or waits for CI after a clean scoped review. +- `REQ-04` The built-in document-review instruction evaluates changed in-scope documents for consistency, contradictions, unresolved material questions, and Memory Bank principles while preserving the existing strict review-result schema and findings loop. +- `REQ-05` `code-converge init-document-review-prompt [--force]` writes the built-in document-review prompt to `.code-converge/default.md`; it refuses to overwrite an existing file unless `--force` is explicit and diagnoses creation errors with exit code `2`. +- `REQ-06` Document review uses either an explicitly selected document-fix prompt file or a built-in document-fix instruction; it remains isolated from ordinary `--fix-prompt-file`, and invalid combinations or unreadable files fail with exit code `2`. +- `REQ-07` The root README documents the final CLI/config contract, precedence, errors, export behavior, and document-scope limitation with runnable examples. + +## Non-Scope + +- `NS-01` Changing the default ordinary code-review behavior, result schema, finding priorities, review/fix budgets, publication, or CI workflow. +- `NS-02` Loading named prompts from arbitrary paths, other extensions, user-level config, environment variables, or an implicit search path. +- `NS-03` Reviewing non-Markdown files in document-review mode or treating `memory-bank/prompts/**` as document-review scope. +- `NS-04` Adding a reusable project-wide architecture rule or changing Memory Bank governance itself. + +## Assumptions and Constraints + +- `ASM-01` The owner decisions recorded in issue comments on 2026-08-05 are accepted source facts for this feature. +- `CON-01` Errors must fail closed and be visible through the existing operational-error contract (exit `2`), never by falling back to another selected prompt. +- `CON-02` The existing private review snapshot and strict JSON schema remain the compatibility boundary. + +## Design Requirement Decision + +`Design required: yes` — the delivery changes public CLI and configuration contracts, document scope, workflow behavior, and prompt/file-resolution semantics. + +## Validation Profile Decision + +Validation profile: `standard`. + +Triggers / rationale: executable behavior changes public CLI/configuration and workflow contracts. No security, persistent-data, financial, concurrency, cross-system protocol, or production-release trigger applies. + +Downgrade approval: none. + +## Verify + +| Scenario | Observable result | +| --- | --- | +| `SC-01` | Each supported review-prompt selector resolves its stated source, and any pair of selectors fails with exit `2` without invoking Codex. | +| `SC-02` | Named prompt validation accepts only safe names and reads only `.code-converge/.md`; missing/unreadable prompt sources fail with actionable exit `2`. | +| `SC-03` | Document-review mode sends the schema-constrained review instruction for only changed eligible Markdown files; an empty eligible set completes clean without a Codex invocation. | +| `SC-04` | `init-document-review-prompt` creates the template once, rejects overwrite without `--force`, and overwrites only with `--force`. | +| `SC-05` | Document findings enter fix using the selected document-fix file or built-in document-fix prompt; ordinary review/fix prompt behavior is unchanged. | +| `SC-06` | README examples and contract agree with help/config behavior and project documentation lint passes. | + +| Negative case | Observable result | +| --- | --- | +| `NEG-01` | `--document-fix-prompt-file` without `--document-review`, or together with `--fix-prompt-file`, exits `2` with an actionable diagnostic. | +| `NEG-02` | A malformed name, non-Markdown selected prompt, unreadable source, or document-review scope that includes excluded prompt artifacts cannot silently broaden scope or fall back; an explicitly supplied readable Markdown path may be outside the repository root. | + +## Traceability + +| Requirement | Acceptance scenarios | Negative coverage | +| --- | --- | --- | +| `REQ-01` | `SC-01` | `NEG-01`, `NEG-02` | +| `REQ-02` | `SC-02` | `NEG-02` | +| `REQ-03` | `SC-03` | `NEG-02` | +| `REQ-04` | `SC-03` | none | +| `REQ-05` | `SC-04` | `NEG-02` | +| `REQ-06` | `SC-05` | `NEG-01`, `NEG-02` | +| `REQ-07` | `SC-06` | none | + +| Check ID | Covers | Required evidence | +| --- | --- | --- | +| `CHK-01` | `SC-01`–`SC-05`, `NEG-01`, `NEG-02` | Focused deterministic config/app/codex/workflow/repository tests using fake runner/executable. | +| `CHK-02` | all executable behavior | `go test ./...`; `go vet ./...`; `git diff --check`. | +| `CHK-03` | `REQ-07`, package artifacts | `make docs-lint`; semantic README/help/config read-through. | +| `CHK-04` | final changed behavior | Independent `codex review --base master` with findings triaged under the same convergence episode. | + +- `EVID-01` Focused table-driven prompt selection, resolution, scope, export, and fix-stage tests. +- `EVID-02` Full local Go tests, vet, and diff integrity. +- `EVID-03` Documentation lint and contract read-through. +- `EVID-04` Clean independent implementation-review result and CI evidence for the published head. diff --git a/memory-bank/features/FT-042/design.md b/memory-bank/features/FT-042/design.md new file mode 100644 index 0000000..bcc0fc6 --- /dev/null +++ b/memory-bank/features/FT-042/design.md @@ -0,0 +1,90 @@ +--- +title: "FT-042: Document review prompt design" +doc_kind: feature +doc_function: canonical +purpose: "Selected CLI, prompt-resolution and document-review behavior for FT-042." +derived_from: + - brief.md + - ../../../README.md + - https://github.com/dapi/code-converge/issues/42 +status: active +audience: humans_and_agents +must_not_define: + - ft_042_scope + - ft_042_acceptance_criteria + - implementation_sequence +--- + +# FT-042: Document review prompt design + +## Design pack + +| Artifact | Role | Owns | +| --- | --- | --- | +| `design.md` | Feature solution | `SOL-*`, `SD-*`, contracts, invariants and failure modes | + +## C4 applicability decision + +`C4-00: not required` — existing CLI, configuration, repository snapshot and Codex adapter components retain their boundaries; the change adds local policy within them. + +## Selected solution + +- `SOL-01` Add mutually exclusive CLI flags `--review-prompt-file `, `--review-prompt `, and `--document-review`, parsed before configuration loading; their values are runtime review choices, not YAML/environment configuration. +- `SOL-02` Resolve a selected prompt once: a file path must name a readable `.md` regular file (relative to the invocation directory; absolute paths are allowed); a name must match `[A-Za-z0-9][A-Za-z0-9_-]*` and maps only to `/.code-converge/.md`; document review chooses `/.code-converge/default.md` when present, otherwise the built-in prompt. No other fallback is allowed. +- `SOL-03` Add `init-document-review-prompt [--force]` as a no-workflow command. It creates the project directory as needed, writes the built-in document-review prompt with owner-only permissions, refuses an existing target without `--force`, and has no configuration dependency. +- `SOL-04` Extend the review target with an eligible changed-file list computed from the same private merge-base snapshot. Document review permits only `.md` paths and excludes `memory-bank/prompts/`; an empty list returns a clean review result without invoking Codex. +- `SOL-05` Compose the selected review prompt with the existing immutable snapshot/schema instructions. The built-in document prompt explicitly names the eligible paths and asks only for consistency, contradictions, material open questions, and Memory Bank principles. +- `SOL-06` Add `--document-fix-prompt-file `, valid only with `--document-review`. It replaces the built-in document fix prompt; it conflicts with `--fix-prompt-file`. Ordinary fix behavior remains unchanged. + +## Alternatives and trade-offs + +| ID | Alternative | Decision | +| --- | --- | --- | +| `ALT-01` | One overloaded selector argument | Rejected: individual flags make conflicts and provenance deterministic. | +| `ALT-02` | Always use project default or silently fall back after a selected file error | Rejected: explicit selection must fail closed; only absence of the document-mode default has a built-in fallback. | +| `TRD-01` | Permit explicit absolute/relative prompt paths but constrain names to project-local files | Chosen: direct path fulfills the requested file mode; names remain safe and reproducible. | + +## Architecture coverage + +| Aspect | Status | Canonical refs | Note | +| --- | --- | --- | --- | +| Components | covered | `SOL-01`–`SOL-06` | app parses commands; config resolves file contents; repository derives scope; adapter composes prompts. | +| Connectors | covered | `CTR-01` | Local filesystem read/write and existing Codex stdin are synchronous. | +| Configuration | covered | `SD-01` | New selectors deliberately do not join persistent YAML/env precedence. | +| Behavioral semantics | covered | `INV-01`–`INV-04`, `FM-01`–`FM-03` | Selection, scope and fix transitions are deterministic. | +| Quality / evolution | covered | `TRD-01`, `RB-01` | No migration; removal is a source revert. | + +## Accepted local decisions, contracts and invariants + +- `SD-01` Prompt selectors are CLI-only to avoid hidden precedence and to keep ordinary config output stable. +- `CTR-01` The adapter receives a resolved prompt plus a review target; user prompt text supplements but cannot replace the snapshot, schema, or no-mutation instructions. +- `INV-01` Ordinary review and `--fix-prompt-file` preserve their current behavior when document flags are absent. +- `INV-02` Any invalid selector, conflict, or selected-file read error exits `2` before Codex starts. +- `INV-03` Document mode never passes non-Markdown or excluded prompt-catalog files to Codex. +- `INV-04` Empty document scope is clean and non-publishing; any non-empty document review also terminates before publication and CI because the existing publication API stages the whole worktree. +- `FM-01` Missing, unreadable, directory, non-Markdown, invalid-name, or write failure → actionable operational exit `2`. +- `FM-02` Two review selectors, document fix without document mode, or both fix selectors → actionable operational exit `2`. +- `FM-03` No eligible documents → structured `scope-empty` diagnostic and no Codex process. +- `RB-01` Backout is one source revert; no config migration, repository mutation, or remote state is introduced. + +## Design verification + +| Analysis | Required | Method / result | +| --- | --- | --- | +| Contract compatibility | yes | Preserve default flags/config/schema; table tests and README comparison required. | +| State / transition completeness | yes | Workflow tests cover clean empty scope and document findings/fix/review. | +| Failure propagation | yes | Table tests cover every selector and export failure; errors fail before Codex. | +| Concurrency / ordering | no | Existing sequential workflow only; no shared mutable state is added. | +| Security boundaries | no | Prompt content is local user input; no auth/trust boundary changes. | +| Capacity / latency | no | One additional Git file-list call is bounded by the existing review snapshot. | +| Migration / evolution safety | yes | Default remains ordinary review; no persistent setting is introduced. | + +## Traceability + +| Requirement | Solution refs | +| --- | --- | +| `REQ-01`–`REQ-02` | `SOL-01`, `SOL-02`, `TRD-01`, `INV-02` | +| `REQ-03`–`REQ-04` | `SOL-04`, `SOL-05`, `CTR-01`, `INV-03`, `INV-04` | +| `REQ-05` | `SOL-03`, `FM-01` | +| `REQ-06` | `SOL-06`, `FM-02` | +| `REQ-07` | `SD-01`, `RB-01` | diff --git a/memory-bank/features/FT-042/implementation-plan.md b/memory-bank/features/FT-042/implementation-plan.md new file mode 100644 index 0000000..295715c --- /dev/null +++ b/memory-bank/features/FT-042/implementation-plan.md @@ -0,0 +1,71 @@ +--- +title: "FT-042: Implementation plan" +doc_kind: feature +doc_function: derived +purpose: "Execution and verification mapping for configurable document-review prompts." +derived_from: + - brief.md + - design.md + - ../../engineering/testing-policy.md +status: archived +audience: humans_and_agents +--- + +# FT-042: Implementation plan + +## Grounding + +| Path | Role | Reuse | +| --- | --- | --- | +| `internal/app/app.go` | root flags, commands and error/event dispatch | command dispatch and `OptionalString` binding pattern | +| `internal/config/config.go` | prompt-file loading and source diagnostics | explicit file validation/read error pattern only | +| `internal/repository/review.go` | private snapshot lifecycle | derive changed paths from the pinned snapshot | +| `internal/codex/adapter.go` | schema-constrained review and fix stdin | preserve schema/scope safety prefix and model invocation | +| `internal/*/*_test.go` | deterministic fakes | table-driven runner/command tests | + +Open questions: none; issue-comment decisions and `design.md` settle CLI semantics. + +## Test strategy + +| Surface | Refs | Automated coverage | Commands | +| --- | --- | --- | --- | +| selector/resolver/export | `REQ-01`, `REQ-02`, `REQ-05`, `SOL-01`–`SOL-03` | conflicts, relative and absolute Markdown paths (including outside repo), names, fallback, create/force/failure | `go test ./internal/app ./internal/config` | +| snapshot and adapter | `REQ-03`, `REQ-04`, `SOL-04`, `SOL-05` | Markdown filtering, excluded paths, empty scope, prompt/schema composition | `go test ./internal/repository ./internal/codex` | +| workflow fix routing | `REQ-06`, `SOL-06` | built-in/custom document fix and unchanged ordinary fix | `go test ./internal/workflow ./internal/codex` | +| public docs | `REQ-07` | help/config examples and links | `make docs-lint` | +| full contract | all | regression and hygiene | `go test ./...`; `go vet ./...`; `git diff --check` | + +Manual-only gaps: none. Required CI: repository Verify workflow for published head. + +## Preconditions + +| ID | Ref | State | +| --- | --- | --- | +| `PRE-01` | `SD-01`, `INV-01`–`INV-04` | Feature package is Plan Ready and no external approval is required for local code/doc changes. | + +## Design realization mapping + +| Refs | Target | Steps | Checks | Evidence | +| --- | --- | --- | --- | --- | +| `SOL-01`–`SOL-03`, `SD-01`, `FM-01`–`FM-02` | app/config command and resolver | `STEP-01` | `CHK-01` | `EVID-01` | +| `SOL-04`, `SOL-05`, `CTR-01`, `INV-02`–`INV-04`, `FM-03` | repository and Codex adapter | `STEP-02` | `CHK-01` | `EVID-01` | +| `SOL-06`, `INV-01`, `FM-02` | workflow/adapter fix wiring | `STEP-03` | `CHK-01` | `EVID-01` | +| `TRD-01`, `RB-01` | README and package evidence | `STEP-04` | `CHK-03` | `EVID-03` | + +## Steps + +| ID | Implements | Work | Verifies | Evidence | +| --- | --- | --- | --- | --- | +| `STEP-01` | `REQ-01`, `REQ-02`, `REQ-05` | Add selectors, safe resolution and export command with deterministic errors. | `CHK-01` | `EVID-01` | +| `STEP-02` | `REQ-03`, `REQ-04` | Add snapshot file filtering, built-in/default prompt composition and empty-scope completion. | `CHK-01` | `EVID-01` | +| `STEP-03` | `REQ-06` | Route document findings to the selected/built-in fix prompt without changing ordinary fix. | `CHK-01` | `EVID-01` | +| `STEP-04` | `REQ-07` | Update README/help contract and feature artifacts. | `CHK-03` | `EVID-03` | +| `STEP-05` | all | Run focused and full validation, simplify review, independent implementation review, commit/push and CI. | `CHK-02`–`CHK-04` | `EVID-02`–`EVID-04` | + +## Checkpoints and risks + +- `CP-01`: `STEP-01` and `STEP-02` pass focused tests before workflow wiring. +- `CP-02`: all changed behavior passes full local validation before independent review. +- `ER-01`: private snapshot filtering may not see untracked Markdown files; stop and return to `design.md` if it cannot preserve the current snapshot contract. +- `ER-02`: a public event-schema change is discovered; stop and update `brief.md`/`design.md` before implementation. +- `STOP-01`: any new security, persistent-data, cross-system, or release trigger raises the validation profile before further code changes. diff --git a/memory-bank/features/README.md b/memory-bank/features/README.md index 464b8fa..a30ba48 100644 --- a/memory-bank/features/README.md +++ b/memory-bank/features/README.md @@ -50,3 +50,4 @@ audience: humans_and_agents - [`FT-036/README.md`](FT-036/README.md) — planned discoverable root and subcommand CLI help for issue #36. - [`FT-038/README.md`](FT-038/README.md) — strict YAML configuration delivery for issue #38. - [`FT-039/README.md`](FT-039/README.md) — active deterministic repository publication and CI orchestration for issue #39. +- [`FT-042/README.md`](FT-042/README.md) — active configurable document-review prompt delivery for issue #42.