diff --git a/README.md b/README.md index 01a8971..d8c6d8d 100644 --- a/README.md +++ b/README.md @@ -26,8 +26,8 @@ Seriatim config contract in `pipeline.yml`: Audita config contract in `pipeline.yml`: - required: `audita.binary` (name or path; existence is checked at execution time, not config validation time) -- optional: `audita.llm_api_key_env` (environment variable name holding the API key secret; defaults to `AUDITA_LLM_API_KEY`) -- defaulted when omitted: `audita.timeout` (`3h`), `audita.llm_api_key_env` (`AUDITA_LLM_API_KEY`), `audita.modules` (`glossary,homophones,glossary,spoken_word,grammar,homophones,glossary`), `audita.base_url` (`https://openrouter.ai/api/v1`), `audita.model` (`openrouter/google/gemma-4-31b-it`), `audita.llm_concurrency` (`1`), `audita.validation_model` (`""`), `audita.validation_llm_concurrency` (`1`), `audita.report` (`true`) +- optional: `audita.llm_api_key_env` (environment variable name holding the API key secret; no automatic default) +- defaulted when omitted: `audita.timeout` (`3h`), `audita.modules` (`glossary,homophones,glossary,spoken_word,grammar,homophones,glossary`), `audita.base_url` (`https://openrouter.ai/api/v1`), `audita.model` (`openrouter/google/gemma-4-31b-it`), `audita.llm_concurrency` (`1`), `audita.validation_model` (`""`), `audita.validation_llm_concurrency` (`1`), `audita.report` (`true`) - allowed `audita.modules` values: `glossary`, `homophones`, `spoken_word`, `grammar` (order and repeats are allowed) - `audita.base_url` must be a valid URL when provided - `audita.llm_concurrency` and `audita.validation_llm_concurrency` must be `> 0` @@ -36,7 +36,8 @@ Audita credentials note: - store only the environment variable **name** in config (`audita.llm_api_key_env`), never the API key value itself - API key values must not be written to pipeline config, generated configs, logs, or manifest metadata -- if the named env var is not set (or is empty), Narratio omits `AUDITA_LLM_API_KEY` from the Audita subprocess environment instead of failing +- if `audita.llm_api_key_env` is configured and the named env var is not set (or is empty), Narratio fails before invocation with a redacted error +- if `audita.llm_api_key_env` is omitted/empty, Narratio does not require a credential and omits `AUDITA_LLM_API_KEY` from the subprocess overrides Audita runtime note: @@ -98,6 +99,10 @@ Default CLI wiring builds and uses: - real Seriatim subprocess adapter from `pipeline.seriatim` - real Audita subprocess adapter from `pipeline.audita` +Subprocess runtime note: + +- subprocesses inherit the parent environment by default, then apply Narratio override values (override values win). + Real `polish` stage output paths: - `transcripts/processed.json` diff --git a/architecture.md b/architecture.md index 5a694c7..499e938 100644 --- a/architecture.md +++ b/architecture.md @@ -132,7 +132,7 @@ Audita config keys: - `pipeline.audita.binary` (required) - `pipeline.audita.timeout` (default: `3h`) -- `pipeline.audita.llm_api_key_env` (optional; default: `AUDITA_LLM_API_KEY`) +- `pipeline.audita.llm_api_key_env` (optional; no automatic default) - `pipeline.audita.modules` (default sequence: `glossary,homophones,glossary,spoken_word,grammar,homophones,glossary`) - `pipeline.audita.base_url` (default: `https://openrouter.ai/api/v1`) - `pipeline.audita.model` (default: `openrouter/google/gemma-4-31b-it`) @@ -145,7 +145,8 @@ Audita secret-handling policy: - `llm_api_key_env` stores only the environment variable **name**. - API key values are read from the process environment at runtime and are not stored in `pipeline.yml`, manifest metadata, generated configs, or logs. -- If the named env var is missing/empty, the Audita adapter omits `AUDITA_LLM_API_KEY` from subprocess env overrides and continues. +- If `llm_api_key_env` is configured and the named env var is missing/empty, the Audita adapter fails before invocation with a redacted error. +- If `llm_api_key_env` is empty/omitted, the Audita adapter omits `AUDITA_LLM_API_KEY` from subprocess env overrides and continues. Validation currently enforces: @@ -331,6 +332,7 @@ All adapters currently have fake/no-op implementations for tests/scaffold execut - Context cancellation + optional timeout. - Explicit executable/args, working dir, env overrides. +- Parent environment inheritance with override merge semantics (override values win). - Stdout/stderr log file handling. - Exit code and timing capture. - Actionable error wrapping. diff --git a/internal/adapters/audita/subprocess.go b/internal/adapters/audita/subprocess.go index 85b87fc..1d2834f 100644 --- a/internal/adapters/audita/subprocess.go +++ b/internal/adapters/audita/subprocess.go @@ -157,14 +157,16 @@ func (r *SubprocessRunner) Run(ctx context.Context, req PolishRequest) (PolishRe } args := r.buildArgs(req, reqModules) + credentialEnvVar := strings.TrimSpace(r.llmAPIKeyEnv) credentialPresent := false env := map[string]string{} - if strings.TrimSpace(r.llmAPIKeyEnv) != "" { - credential, ok := os.LookupEnv(r.llmAPIKeyEnv) - if ok && strings.TrimSpace(credential) != "" { - env["AUDITA_LLM_API_KEY"] = credential - credentialPresent = true + if credentialEnvVar != "" { + credential, ok := os.LookupEnv(credentialEnvVar) + if !ok || strings.TrimSpace(credential) == "" { + return PolishResult{}, fmt.Errorf("audita: required credential environment variable %s is not set", credentialEnvVar) } + env["AUDITA_LLM_API_KEY"] = credential + credentialPresent = true } primaryConcurrencyViaEnv := false if r.llmConcurrency != nil { @@ -187,7 +189,13 @@ func (r *SubprocessRunner) Run(ctx context.Context, req PolishRequest) (PolishRe StderrLogPath: req.StderrLogPath, }) if err != nil { - return r.failureResult(req, reqModules, runRes, credentialPresent, primaryConcurrencyViaEnv), fmt.Errorf("run audita process (binary=%q): %w", r.binary, err) + return r.failureResult(req, reqModules, runRes, credentialPresent, primaryConcurrencyViaEnv), fmt.Errorf( + "run audita process (binary=%q, stdout_log=%q, stderr_log=%q): %w", + r.binary, + req.StdoutLogPath, + req.StderrLogPath, + err, + ) } if err := validateProcessedOutput(req.OutputProcessedPath); err != nil { diff --git a/internal/adapters/audita/subprocess_test.go b/internal/adapters/audita/subprocess_test.go index 8df827a..1c322d5 100644 --- a/internal/adapters/audita/subprocess_test.go +++ b/internal/adapters/audita/subprocess_test.go @@ -120,7 +120,33 @@ func TestSubprocessRunnerSuccessArgsEnvAndValidation(t *testing.T) { } } -func TestSubprocessRunnerMissingCredentialEnvOmitsCredential(t *testing.T) { +func TestSubprocessRunnerMissingConfiguredCredentialFails(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("helper wrapper script uses /bin/sh") + } + llmConcurrency := 1 + runner := mustAuditaRunner(t, SubprocessRunnerConfig{ + Binary: writeAuditaHelperWrapper(t), + Timeout: mustParseAuditaDuration(t, "2s"), + LLMAPIKeyEnv: "MISSING_AUDITA_KEY", + Modules: []string{"glossary"}, + BaseURL: "https://openrouter.ai/api/v1", + Model: "openrouter/google/gemma-4-31b-it", + LLMConcurrency: &llmConcurrency, + Report: false, + }) + req := auditaReqForTest(t, false) + + _, err := runner.Run(context.Background(), req) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "required credential environment variable MISSING_AUDITA_KEY is not set") { + t.Fatalf("error = %q, want missing credential guidance", err.Error()) + } +} + +func TestSubprocessRunnerUnconfiguredCredentialEnvOmitsCredential(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("helper wrapper script uses /bin/sh") } @@ -133,7 +159,7 @@ func TestSubprocessRunnerMissingCredentialEnvOmitsCredential(t *testing.T) { runner := mustAuditaRunner(t, SubprocessRunnerConfig{ Binary: writeAuditaHelperWrapper(t), Timeout: mustParseAuditaDuration(t, "2s"), - LLMAPIKeyEnv: "MISSING_AUDITA_KEY", + LLMAPIKeyEnv: "", Modules: []string{"glossary"}, BaseURL: "https://openrouter.ai/api/v1", Model: "openrouter/google/gemma-4-31b-it", @@ -186,6 +212,9 @@ func TestSubprocessRunnerSubprocessFailure(t *testing.T) { if !strings.Contains(err.Error(), "exit code") { t.Fatalf("error = %q, want exit code context", err.Error()) } + if !strings.Contains(err.Error(), req.StdoutLogPath) || !strings.Contains(err.Error(), req.StderrLogPath) { + t.Fatalf("error = %q, want stdout/stderr log paths", err.Error()) + } } func TestSubprocessRunnerMissingOutputFails(t *testing.T) { diff --git a/internal/adapters/subprocess/run_test.go b/internal/adapters/subprocess/run_test.go index 9f746a5..4670111 100644 --- a/internal/adapters/subprocess/run_test.go +++ b/internal/adapters/subprocess/run_test.go @@ -109,6 +109,67 @@ func TestRunTimeout(t *testing.T) { } } +func TestRunInheritsParentEnvironmentByDefault(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + + t.Setenv("GO_WANT_SUBPROCESS_HELPER", "1") + t.Setenv("SUBPROCESS_HELPER_ENV_KEY", "SUBPROCESS_PARENT_VALUE") + t.Setenv("SUBPROCESS_PARENT_VALUE", "inherited-value") + + dir := t.TempDir() + stdoutPath := filepath.Join(dir, "stdout.log") + req := RunRequest{ + Executable: exe, + Args: []string{"-test.run=TestSubprocessHelper", "--", "printenv"}, + StdoutLogPath: stdoutPath, + } + + if _, err := Run(context.Background(), req); err != nil { + t.Fatalf("Run() error = %v", err) + } + data, err := os.ReadFile(stdoutPath) + if err != nil { + t.Fatalf("read stdout log: %v", err) + } + if strings.TrimSpace(string(data)) != "inherited-value" { + t.Fatalf("stdout = %q, want inherited-value", strings.TrimSpace(string(data))) + } +} + +func TestRunEnvOverridesWinOverInheritedValues(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + + t.Setenv("GO_WANT_SUBPROCESS_HELPER", "1") + t.Setenv("SUBPROCESS_HELPER_ENV_KEY", "SUBPROCESS_PARENT_VALUE") + t.Setenv("SUBPROCESS_PARENT_VALUE", "parent-value") + + dir := t.TempDir() + stdoutPath := filepath.Join(dir, "stdout.log") + req := RunRequest{ + Executable: exe, + Args: []string{"-test.run=TestSubprocessHelper", "--", "printenv"}, + EnvOverrides: map[string]string{"SUBPROCESS_PARENT_VALUE": "override-value"}, + StdoutLogPath: stdoutPath, + } + + if _, err := Run(context.Background(), req); err != nil { + t.Fatalf("Run() error = %v", err) + } + data, err := os.ReadFile(stdoutPath) + if err != nil { + t.Fatalf("read stdout log: %v", err) + } + if strings.TrimSpace(string(data)) != "override-value" { + t.Fatalf("stdout = %q, want override-value", strings.TrimSpace(string(data))) + } +} + func TestWriteYAMLAtomic(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config.generated.yml") @@ -190,6 +251,10 @@ func TestSubprocessHelper(t *testing.T) { case "sleep": time.Sleep(500 * time.Millisecond) os.Exit(0) + case "printenv": + key := os.Getenv("SUBPROCESS_HELPER_ENV_KEY") + _, _ = os.Stdout.WriteString(os.Getenv(key) + "\n") + os.Exit(0) default: os.Exit(2) } diff --git a/internal/config/load.go b/internal/config/load.go index 5ba5cfc..f707af7 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -137,9 +137,6 @@ func applyAuditaDefaults(cfg *AuditaConfig) { if cfg.Timeout == "" { cfg.Timeout = "3h" } - if cfg.LLMAPIKeyEnv == "" { - cfg.LLMAPIKeyEnv = "AUDITA_LLM_API_KEY" - } if cfg.Modules == nil { cfg.Modules = []string{ "glossary", diff --git a/internal/config/load_validate_test.go b/internal/config/load_validate_test.go index 5fe749f..576b224 100644 --- a/internal/config/load_validate_test.go +++ b/internal/config/load_validate_test.go @@ -602,8 +602,8 @@ inputs: if cfg.Pipeline.Audita.Timeout != "3h" { t.Fatalf("audita.timeout = %q, want %q", cfg.Pipeline.Audita.Timeout, "3h") } - if cfg.Pipeline.Audita.LLMAPIKeyEnv != "AUDITA_LLM_API_KEY" { - t.Fatalf("audita.llm_api_key_env = %q, want %q", cfg.Pipeline.Audita.LLMAPIKeyEnv, "AUDITA_LLM_API_KEY") + if cfg.Pipeline.Audita.LLMAPIKeyEnv != "" { + t.Fatalf("audita.llm_api_key_env = %q, want empty by default", cfg.Pipeline.Audita.LLMAPIKeyEnv) } if got := strings.Join(cfg.Pipeline.Audita.Modules, ","); got != "glossary,homophones,glossary,spoken_word,grammar,homophones,glossary" { t.Fatalf("audita.modules = %q, want default sequence", got)