From 3ddb3a947bf0ca6d155900b4cbfc794ad94bf0e6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 27 May 2026 08:35:02 -0500 Subject: [PATCH] Update pipeline defaults so trim is enabled when omitted --- docs/config.md | 10 +-- docs/internal/stage-trim.md | 8 +- examples/pipeline.full.annotated.yml | 9 +-- examples/pipeline.production.yml | 3 - internal/app/commands_test.go | 57 ++++++++++++++ internal/app/run_stage_test.go | 4 +- internal/config/config.go | 2 +- internal/config/defaults.go | 21 ++++-- internal/config/load.go | 25 +++++++ internal/config/trim_test.go | 108 +++++++++++++-------------- internal/config/validate.go | 5 +- internal/stage/trim.go | 2 +- internal/stage/trim_test.go | 6 +- 13 files changed, 174 insertions(+), 86 deletions(-) diff --git a/docs/config.md b/docs/config.md index 797150b..f2ca18f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -184,12 +184,12 @@ Rules: | `pipeline.normalize.output_path` | string | No | `transcripts/final.json` | | `pipeline.normalize.output_schema` | string | No | `seriatim-intermediate` | | `pipeline.normalize.report` | bool | No | `true` | -| `pipeline.trim.enabled` | bool | No | `false` | -| `pipeline.trim.output_path` | string | Conditional | required when trim enabled | -| `pipeline.trim.bounds.prompt_id` | string | Conditional | required when trim enabled | +| `pipeline.trim.enabled` | bool | No | `true` | +| `pipeline.trim.output_path` | string | No | `transcripts/final.trimmed.json` | +| `pipeline.trim.bounds.prompt_id` | string | No | `dnd.session_bounds` | | `pipeline.trim.bounds.profile_id` | string | No | empty | -| `pipeline.trim.bounds.transcript_input_name` | string | Conditional | required when trim enabled | -| `pipeline.trim.bounds.output_path` | string | Conditional | required when trim enabled | +| `pipeline.trim.bounds.transcript_input_name` | string | No | `transcript` | +| `pipeline.trim.bounds.output_path` | string | No | `artifacts/session_bounds.json` | | `pipeline.trim.bounds.timeout` | duration | No | `10m` | | `pipeline.trim.bounds.render_debug` | bool | No | `false` | | `pipeline.trim.bounds.render_output_path` | string | Conditional | required when `render_debug` is true | diff --git a/docs/internal/stage-trim.md b/docs/internal/stage-trim.md index 16b66bc..8e2e430 100644 --- a/docs/internal/stage-trim.md +++ b/docs/internal/stage-trim.md @@ -1,7 +1,7 @@ # Stage: trim ## Purpose -Produce a final-trimmed transcript; optionally generate bounds-driven trim. +Produce a final-trimmed transcript. By default, the stage generates bounds and applies a bounds-driven trim. ## Inputs - `transcripts/final.json` @@ -11,9 +11,6 @@ Produce a final-trimmed transcript; optionally generate bounds-driven trim. - when trim enabled: `artifacts/session_bounds.json` ## Key Behavior -When `trim.enabled=false`: -- copies normalized transcript to trimmed output. - When `trim.enabled=true`: - runs Scriptorium bounds artifact generation; - optionally runs render-debug output generation; @@ -22,6 +19,9 @@ When `trim.enabled=true`: - either copies unchanged transcript or runs Seriatim trim; - validates trimmed transcript and materializes bounds output. +When `trim.enabled=false`: +- copies normalized transcript to trimmed output. + ## Invariants - normalized transcript is required input. - bounds output exists only in enabled trim path. diff --git a/examples/pipeline.full.annotated.yml b/examples/pipeline.full.annotated.yml index ba6e5b5..8ae161e 100644 --- a/examples/pipeline.full.annotated.yml +++ b/examples/pipeline.full.annotated.yml @@ -110,17 +110,16 @@ normalize: report: true trim: - # Keep disabled unless bounds prompt integration is configured. - enabled: false + # Optional; defaults shown explicitly. + enabled: true output_path: transcripts/final.trimmed.json bounds: prompt_id: dnd.session_bounds - profile_id: local-fast + profile_id: "" transcript_input_name: transcript - output_path: reports/session_bounds.json + output_path: artifacts/session_bounds.json timeout: 10m render_debug: false - render_output_path: reports/session_bounds.render.json seriatim: report: false diff --git a/examples/pipeline.production.yml b/examples/pipeline.production.yml index affae6d..743122e 100644 --- a/examples/pipeline.production.yml +++ b/examples/pipeline.production.yml @@ -71,9 +71,6 @@ normalize: output_schema: seriatim-intermediate report: true -trim: - enabled: false - scriptorium: binary: scriptorium config_path: /usr/local/etc/scriptorium/config.yml diff --git a/internal/app/commands_test.go b/internal/app/commands_test.go index 55f7e63..2429a9b 100644 --- a/internal/app/commands_test.go +++ b/internal/app/commands_test.go @@ -466,10 +466,13 @@ func writeValidConfigFiles(t *testing.T, workspaceRoot string, transcribeURL ... url = transcribeURL[0] } seriatimBinary := writeSeriatimAppTestWrapper(t) + scriptoriumBinary := writeScriptoriumAppTestWrapper(t) auditaBinary := writeAuditaAppTestWrapper(t) t.Setenv("GO_WANT_APP_SERIATIM_HELPER", "1") + t.Setenv("GO_WANT_APP_SCRIPTORIUM_HELPER", "1") t.Setenv("GO_WANT_APP_AUDITA_HELPER", "1") t.Setenv("AUDITA_LLM_API_KEY", "test-audita-key") + t.Setenv("PATH", filepath.Dir(scriptoriumBinary)+string(os.PathListSeparator)+os.Getenv("PATH")) pipelineYAML := `workspace: root: ` + workspaceRoot + ` @@ -591,6 +594,60 @@ func writeSeriatimAppTestWrapper(t *testing.T) string { return path } +func writeScriptoriumAppTestWrapper(t *testing.T) string { + t.Helper() + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + path := filepath.Join(t.TempDir(), "scriptorium") + content := "#!/bin/sh\nexec \"" + exe + "\" -test.run=TestScriptoriumAppHelper -- \"$@\"\n" + if err := os.WriteFile(path, []byte(content), 0o755); err != nil { + t.Fatalf("WriteFile(%q): %v", path, err) + } + return path +} + +func TestScriptoriumAppHelper(t *testing.T) { + if os.Getenv("GO_WANT_APP_SCRIPTORIUM_HELPER") != "1" { + return + } + + args := os.Args + start := -1 + for i := range args { + if args[i] == "--" { + start = i + 1 + break + } + } + if start < 0 || start >= len(args) { + _, _ = os.Stderr.WriteString("missing -- args separator\n") + os.Exit(2) + } + runArgs := args[start:] + + outputPath := appSeriatimFlagValue(runArgs, "--out") + if strings.TrimSpace(outputPath) == "" { + outputPath = appSeriatimFlagValue(runArgs, "--output") + } + if strings.TrimSpace(outputPath) == "" { + _, _ = os.Stderr.WriteString("missing output flag\n") + os.Exit(2) + } + if err := os.MkdirAll(filepath.Dir(outputPath), 0o755); err != nil { + _, _ = os.Stderr.WriteString(fmt.Sprintf("mkdir output dir: %v\n", err)) + os.Exit(2) + } + if err := os.WriteFile(outputPath, []byte(`{"trim_action":"copy","warnings":[]}`), 0o644); err != nil { + _, _ = os.Stderr.WriteString(fmt.Sprintf("write output: %v\n", err)) + os.Exit(2) + } + _, _ = os.Stdout.WriteString("scriptorium helper stdout\n") + _, _ = os.Stderr.WriteString("scriptorium helper stderr\n") + os.Exit(0) +} + func TestSeriatimAppHelper(t *testing.T) { if os.Getenv("GO_WANT_APP_SERIATIM_HELPER") != "1" { return diff --git a/internal/app/run_stage_test.go b/internal/app/run_stage_test.go index d5b1c3e..e2c36b3 100644 --- a/internal/app/run_stage_test.go +++ b/internal/app/run_stage_test.go @@ -236,8 +236,8 @@ func TestRunStageTrimExecutes(t *testing.T) { if m.Stages["trim"] == nil || m.Stages["trim"].Status != manifest.StatusSucceeded { t.Fatalf("trim stage = %#v, want succeeded", m.Stages["trim"]) } - if m.Stages["trim"].Metadata == nil || m.Stages["trim"].Metadata["trim_action"] != "copy_disabled" { - t.Fatalf("trim stage metadata = %#v, want trim_action=copy_disabled", m.Stages["trim"].Metadata) + if m.Stages["trim"].Metadata == nil || m.Stages["trim"].Metadata["trim_action"] != "copy" { + t.Fatalf("trim stage metadata = %#v, want trim_action=copy", m.Stages["trim"].Metadata) } } diff --git a/internal/config/config.go b/internal/config/config.go index 38bcbb1..28a9b36 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -188,7 +188,7 @@ type NormalizeConfig struct { // TrimConfig configures trim-stage transcript boundary behavior. type TrimConfig struct { - Enabled bool `yaml:"enabled"` + Enabled *bool `yaml:"enabled"` OutputPath string `yaml:"output_path"` Bounds TrimBoundsConfig `yaml:"bounds"` Seriatim TrimSeriatimConfig `yaml:"seriatim"` diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 2b6892a..10f06e0 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -38,14 +38,19 @@ const ( DefaultScriptoriumTimeout = "10m" DefaultScriptoriumArtifactOutputRoot = "artifacts" - DefaultTrimBoundsTimeout = "10m" - DefaultTrimSeriatimReport = false - DefaultRenderEnabled = true - DefaultRenderFormat = "markdown" - DefaultRenderTitle = "" - DefaultRenderTimestamps = true - DefaultRenderSegmentIDs = true - DefaultRenderMetadata = false + DefaultTrimEnabled = true + DefaultTrimOutputPath = artifactmodel.TranscriptPathFinalTrimmed + DefaultTrimBoundsPromptID = "dnd.session_bounds" + DefaultTrimBoundsTranscriptInputName = "transcript" + DefaultTrimBoundsOutputPath = "artifacts/session_bounds.json" + DefaultTrimBoundsTimeout = "10m" + DefaultTrimSeriatimReport = false + DefaultRenderEnabled = true + DefaultRenderFormat = "markdown" + DefaultRenderTitle = "" + DefaultRenderTimestamps = true + DefaultRenderSegmentIDs = true + DefaultRenderMetadata = false DefaultNormalizeOutputPath = artifactmodel.TranscriptPathFinal DefaultNormalizeOutputSchema = "seriatim-intermediate" diff --git a/internal/config/load.go b/internal/config/load.go index 23c6944..0655ed6 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -336,7 +336,13 @@ func applyPipelineDefaults(cfg *PipelineConfig) { cfg.Normalize = &NormalizeConfig{} } applyNormalizeDefaults(cfg.Normalize) + if cfg.Trim == nil { + cfg.Trim = &TrimConfig{} + } applyTrimDefaults(cfg.Trim) + if trimEnabled(cfg.Trim) && cfg.Scriptorium == nil { + cfg.Scriptorium = &ScriptoriumConfig{} + } applyRenderDefaults(&cfg.Render) applyScriptoriumDefaults(cfg.Scriptorium) } @@ -500,6 +506,21 @@ func applyTrimDefaults(cfg *TrimConfig) { if cfg == nil { return } + if cfg.Enabled == nil { + cfg.Enabled = boolPtr(DefaultTrimEnabled) + } + if strings.TrimSpace(cfg.OutputPath) == "" { + cfg.OutputPath = DefaultTrimOutputPath + } + if strings.TrimSpace(cfg.Bounds.PromptID) == "" { + cfg.Bounds.PromptID = DefaultTrimBoundsPromptID + } + if strings.TrimSpace(cfg.Bounds.TranscriptInputName) == "" { + cfg.Bounds.TranscriptInputName = DefaultTrimBoundsTranscriptInputName + } + if strings.TrimSpace(cfg.Bounds.OutputPath) == "" { + cfg.Bounds.OutputPath = DefaultTrimBoundsOutputPath + } if cfg.Bounds.Timeout == "" { cfg.Bounds.Timeout = DefaultTrimBoundsTimeout } @@ -508,6 +529,10 @@ func applyTrimDefaults(cfg *TrimConfig) { } } +func trimEnabled(cfg *TrimConfig) bool { + return cfg != nil && cfg.Enabled != nil && *cfg.Enabled +} + func applyRenderDefaults(cfg **RenderConfig) { if cfg == nil { return diff --git a/internal/config/trim_test.go b/internal/config/trim_test.go index ca95607..c303ea8 100644 --- a/internal/config/trim_test.go +++ b/internal/config/trim_test.go @@ -13,6 +13,20 @@ func TestTrimLoadAndValidate(t *testing.T) { wantValidateErr string assert func(t *testing.T, cfg *Config) }{ + { + name: "trim defaults when omitted", + trimYAML: "", + assert: func(t *testing.T, cfg *Config) { + t.Helper() + assertDefaultTrimConfig(t, cfg) + if cfg.Pipeline.Scriptorium == nil { + t.Fatal("scriptorium config should be defaulted when trim is enabled by default") + } + if cfg.Pipeline.Scriptorium.Binary != DefaultScriptoriumBinary { + t.Fatalf("scriptorium.binary = %q, want %q", cfg.Pipeline.Scriptorium.Binary, DefaultScriptoriumBinary) + } + }, + }, { name: "valid trim config", trimYAML: `trim: @@ -34,8 +48,8 @@ func TestTrimLoadAndValidate(t *testing.T) { if cfg.Pipeline.Trim == nil { t.Fatal("trim config should be present") } - if cfg.Pipeline.Trim.Enabled != true { - t.Fatalf("trim.enabled = %t, want true", cfg.Pipeline.Trim.Enabled) + if cfg.Pipeline.Trim.Enabled == nil || !*cfg.Pipeline.Trim.Enabled { + t.Fatalf("trim.enabled = %#v, want true", cfg.Pipeline.Trim.Enabled) } if cfg.Pipeline.Trim.Bounds.ProfileID != "" { t.Fatalf("trim.bounds.profile_id = %q, want empty", cfg.Pipeline.Trim.Bounds.ProfileID) @@ -43,67 +57,25 @@ func TestTrimLoadAndValidate(t *testing.T) { }, }, { - name: "enabled omitted defaults disabled", + name: "enabled omitted defaults enabled", trimYAML: `trim: - output_path: transcripts/final.trimmed.json - bounds: - prompt_id: dnd_session.bounds - transcript_input_name: transcript - output_path: artifacts/session_bounds.json `, assert: func(t *testing.T, cfg *Config) { t.Helper() - if cfg.Pipeline.Trim == nil { - t.Fatal("trim config should be present") - } - if cfg.Pipeline.Trim.Enabled { - t.Fatal("trim.enabled should default to false when omitted") - } + assertDefaultTrimConfig(t, cfg) }, }, { - name: "missing prompt id fails when enabled", + name: "explicit disabled remains disabled", trimYAML: `trim: - enabled: true - output_path: transcripts/final.trimmed.json - bounds: - transcript_input_name: transcript - output_path: artifacts/session_bounds.json + enabled: false `, - wantValidateErr: "pipeline.trim.bounds.prompt_id is required when pipeline.trim.enabled is true", - }, - { - name: "missing transcript input name fails when enabled", - trimYAML: `trim: - enabled: true - output_path: transcripts/final.trimmed.json - bounds: - prompt_id: dnd_session.bounds - output_path: artifacts/session_bounds.json -`, - wantValidateErr: "pipeline.trim.bounds.transcript_input_name is required when pipeline.trim.enabled is true", - }, - { - name: "missing bounds output path fails when enabled", - trimYAML: `trim: - enabled: true - output_path: transcripts/final.trimmed.json - bounds: - prompt_id: dnd_session.bounds - transcript_input_name: transcript -`, - wantValidateErr: "pipeline.trim.bounds.output_path is required when pipeline.trim.enabled is true", - }, - { - name: "missing trimmed output path fails when enabled", - trimYAML: `trim: - enabled: true - bounds: - prompt_id: dnd_session.bounds - transcript_input_name: transcript - output_path: artifacts/session_bounds.json -`, - wantValidateErr: "pipeline.trim.output_path is required when pipeline.trim.enabled is true", + assert: func(t *testing.T, cfg *Config) { + t.Helper() + if cfg.Pipeline.Trim == nil || cfg.Pipeline.Trim.Enabled == nil || *cfg.Pipeline.Trim.Enabled { + t.Fatalf("trim.enabled = %#v, want false", cfg.Pipeline.Trim) + } + }, }, { name: "invalid timeout fails", @@ -185,3 +157,31 @@ func TestTrimLoadAndValidate(t *testing.T) { }) } } + +func assertDefaultTrimConfig(t *testing.T, cfg *Config) { + t.Helper() + if cfg.Pipeline.Trim == nil { + t.Fatal("trim config should be present") + } + if cfg.Pipeline.Trim.Enabled == nil || !*cfg.Pipeline.Trim.Enabled { + t.Fatalf("trim.enabled = %#v, want true", cfg.Pipeline.Trim.Enabled) + } + if cfg.Pipeline.Trim.OutputPath != DefaultTrimOutputPath { + t.Fatalf("trim.output_path = %q, want %q", cfg.Pipeline.Trim.OutputPath, DefaultTrimOutputPath) + } + if cfg.Pipeline.Trim.Bounds.PromptID != DefaultTrimBoundsPromptID { + t.Fatalf("trim.bounds.prompt_id = %q, want %q", cfg.Pipeline.Trim.Bounds.PromptID, DefaultTrimBoundsPromptID) + } + if cfg.Pipeline.Trim.Bounds.TranscriptInputName != DefaultTrimBoundsTranscriptInputName { + t.Fatalf("trim.bounds.transcript_input_name = %q, want %q", cfg.Pipeline.Trim.Bounds.TranscriptInputName, DefaultTrimBoundsTranscriptInputName) + } + if cfg.Pipeline.Trim.Bounds.OutputPath != DefaultTrimBoundsOutputPath { + t.Fatalf("trim.bounds.output_path = %q, want %q", cfg.Pipeline.Trim.Bounds.OutputPath, DefaultTrimBoundsOutputPath) + } + if cfg.Pipeline.Trim.Bounds.Timeout != DefaultTrimBoundsTimeout { + t.Fatalf("trim.bounds.timeout = %q, want %q", cfg.Pipeline.Trim.Bounds.Timeout, DefaultTrimBoundsTimeout) + } + if cfg.Pipeline.Trim.Seriatim.Report == nil || *cfg.Pipeline.Trim.Seriatim.Report != DefaultTrimSeriatimReport { + t.Fatalf("trim.seriatim.report = %#v, want %t", cfg.Pipeline.Trim.Seriatim.Report, DefaultTrimSeriatimReport) + } +} diff --git a/internal/config/validate.go b/internal/config/validate.go index c5b5d30..70de29a 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -276,7 +276,10 @@ func validateTrim(cfg *TrimConfig) error { if cfg == nil { return nil } - if !cfg.Enabled { + if cfg.Enabled == nil { + return fmt.Errorf("pipeline.trim.enabled must be set (defaults should populate this)") + } + if !*cfg.Enabled { return nil } diff --git a/internal/stage/trim.go b/internal/stage/trim.go index cadfb9e..be44cc9 100644 --- a/internal/stage/trim.go +++ b/internal/stage/trim.go @@ -71,7 +71,7 @@ func (trimStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Stag } trimCfg := env.Config.Pipeline.Trim - enabled := trimCfg != nil && trimCfg.Enabled + enabled := trimCfg != nil && trimCfg.Enabled != nil && *trimCfg.Enabled canonicalTrimmedPath, err := resolveTrimmedOutputPath(paths, trimCfg) if err != nil { diff --git a/internal/stage/trim_test.go b/internal/stage/trim_test.go index f42d844..16db3b0 100644 --- a/internal/stage/trim_test.go +++ b/internal/stage/trim_test.go @@ -275,7 +275,8 @@ func TestTrimStageDisabledCopiesNormalizedTranscript(t *testing.T) { writeFile(t, normalized, normalizedBody) disabled := *env.Config.Pipeline.Trim - disabled.Enabled = false + enabled := false + disabled.Enabled = &enabled env.Config.Pipeline.Trim = &disabled result, err := (trimStage{}).Run(context.Background(), env, m) @@ -424,6 +425,7 @@ func setupTrimEnv(t *testing.T) (*Env, *manifest.Manifest, *boundsScriptoriumRun writeFile(t, pipelinePath, "workspace:\n root: "+workspace+"\n") seriatimReport := false + trimEnabled := true cfg := &config.Config{ PipelinePath: pipelinePath, SessionPath: sessionPath, @@ -437,7 +439,7 @@ func setupTrimEnv(t *testing.T) (*Env, *manifest.Manifest, *boundsScriptoriumRun Report: &seriatimReport, }, Trim: &config.TrimConfig{ - Enabled: true, + Enabled: &trimEnabled, OutputPath: "transcripts/final.trimmed.json", Bounds: config.TrimBoundsConfig{ PromptID: "dnd_session.bounds",