diff --git a/cmd/conduit/internal/mcp/catalog.go b/cmd/conduit/internal/mcp/catalog.go index 782fbe206..d91342887 100644 --- a/cmd/conduit/internal/mcp/catalog.go +++ b/cmd/conduit/internal/mcp/catalog.go @@ -87,6 +87,24 @@ var catalog = []ToolInfo{ Description: "Scaffolds a new Go (WASM) processor plugin repository. Same engine as `conduit processor new`.", Mutates: true, }, + { + Name: ToolRepair, + Description: "Scans a pipeline configuration for findings that carry a structured, machine-appliable " + + "fix (a deprecated/renamed field, an unambiguous invalid status value, a negative processor " + + "workers count, an over-long description), classifies each fix's safety (safe / restart / " + + "data_path), and returns a plan hash. Mutates nothing. Same engine as `conduit pipelines repair`.", + Mutates: false, + }, + { + Name: ToolRepairApply, + Description: "Applies the plan computed by repair — safe fixes only, unless select names others — " + + "only if hash still matches the freshly recomputed plan (a stale hash is refused, nothing " + + "mutated). A data-path-adjacent fix (connector settings, a connector's own plugin/type, DLQ " + + "config, any id field) is NEVER applied by this tool, even if selected explicitly — it comes " + + "back in the result as a skipped fix; only the CLI's --escalate flag (human-only) can apply " + + "one. Same engine as `conduit pipelines repair --apply`.", + Mutates: true, + }, { Name: ToolStart, Description: "Starts a pipeline registered in a running Conduit (transitions to Running). " + diff --git a/cmd/conduit/internal/mcp/server.go b/cmd/conduit/internal/mcp/server.go index 0767850de..3ee4cec7e 100644 --- a/cmd/conduit/internal/mcp/server.go +++ b/cmd/conduit/internal/mcp/server.go @@ -38,6 +38,8 @@ const ( ToolStop = "stop" ToolScaffoldConnector = "scaffold_connector" ToolScaffoldProcessor = "scaffold_processor" + ToolRepair = "repair" + ToolRepairApply = "repair_apply" ) // Config configures NewServer. @@ -93,18 +95,23 @@ type server struct { // NewServer builds the `conduit mcp` tool catalog over cfg. // -// Read tools — validate, lint, dry_run, doctor, deploy, inspect — are always -// registered; none of them mutate anything (deploy computes a Diff/hash but -// never executes it; see deploy.go). +// Read tools — validate, lint, dry_run, doctor, deploy, inspect, repair — +// are always registered; none of them mutate anything (deploy computes a +// Diff/hash but never executes it, see deploy.go; repair computes a plan +// over CONTENT and returns proposed fixes, never touching a store or a +// running pipeline, see cmd/conduit/internal/repair's doc). // -// Write tools — apply, start, stop, scaffold_connector, scaffold_processor — -// are registered only when cfg.AllowMutations is set (design doc §3, -// AC-2/AC-3; start/stop added by 20260712-cli-pipeline-lifecycle-verbs.md -// §4). +// Write tools — apply, start, stop, scaffold_connector, scaffold_processor, +// repair_apply — are registered only when cfg.AllowMutations is set (design +// doc §3, AC-2/AC-3; start/stop added by +// 20260712-cli-pipeline-lifecycle-verbs.md §4; repair_apply added by +// 20260712-repair-command.md §5.2 — even when registered, it never applies +// a data-path-adjacent fix; see tools_repair.go's doc). // // Every tool is a thin handler over the exact engine the matching CLI verb // calls (cmd/conduit/internal/validate, pkg/conduit/check via doctorcheck, -// cmd/conduit/internal/deploy, pkg/scaffold, the API client) — see doc.go. +// cmd/conduit/internal/deploy, cmd/conduit/internal/repair, pkg/scaffold, +// the API client) — see doc.go. func NewServer(cfg Config) *sdkmcp.Server { if cfg.newDeployService == nil { cfg.newDeployService = deploy.NewService @@ -120,11 +127,15 @@ func NewServer(cfg Config) *sdkmcp.Server { Instructions: "Conduit pipeline tools. validate/lint/dry_run/deploy check and preview a pipeline " + "configuration offline (content-in: pass the pipeline YAML as `config`, never a server file path). " + "doctor checks whether the local machine/configuration is ready for `conduit run`. inspect reads a " + - "running pipeline's live status (requires the server to have been started with --api-address). If " + - "present, apply/start/stop/scaffold_connector/scaffold_processor mutate the local pipeline store, a " + - "running pipeline, or the filesystem — apply requires the hash from a prior deploy call and is " + - "refused on a stale hash or a running pipeline; start/stop require --api-address like inspect and " + - "are refused on an invalid transition (already running / not running); these tools only appear when " + + "running pipeline's live status (requires the server to have been started with --api-address). " + + "repair scans a configuration for machine-appliable fixes and returns them classified " + + "(safe/restart/data_path) with a plan hash; repair_apply, when present, applies the safe ones " + + "(or an explicit `select`) but NEVER a data_path fix — those require the human-only CLI --escalate " + + "path. If present, apply/start/stop/scaffold_connector/scaffold_processor/repair_apply mutate the " + + "local pipeline store, a running pipeline, or the filesystem — apply requires the hash from a prior " + + "deploy call and is refused on a stale hash or a running pipeline; start/stop require --api-address " + + "like inspect and are refused on an invalid transition (already running / not running); these tools " + + "(plus repair_apply) only appear when " + "the operator started conduit mcp with --allow-mutations.", }) @@ -134,6 +145,7 @@ func NewServer(cfg Config) *sdkmcp.Server { sdkmcp.AddTool(srv, tool(ToolDoctor), s.doctor) sdkmcp.AddTool(srv, tool(ToolDeploy), s.deploy) sdkmcp.AddTool(srv, tool(ToolInspect), s.inspect) + sdkmcp.AddTool(srv, tool(ToolRepair), s.repair) if cfg.AllowMutations { sdkmcp.AddTool(srv, tool(ToolApply), s.apply) @@ -141,6 +153,7 @@ func NewServer(cfg Config) *sdkmcp.Server { sdkmcp.AddTool(srv, tool(ToolStop), s.stop) sdkmcp.AddTool(srv, tool(ToolScaffoldConnector), s.scaffoldConnector) sdkmcp.AddTool(srv, tool(ToolScaffoldProcessor), s.scaffoldProcessor) + sdkmcp.AddTool(srv, tool(ToolRepairApply), s.repairApply) } return srv diff --git a/cmd/conduit/internal/mcp/server_test.go b/cmd/conduit/internal/mcp/server_test.go index ffa5e5a8e..4e1bb6ddc 100644 --- a/cmd/conduit/internal/mcp/server_test.go +++ b/cmd/conduit/internal/mcp/server_test.go @@ -26,9 +26,9 @@ import ( sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) -var readToolNames = []string{ToolValidate, ToolLint, ToolDryRun, ToolDoctor, ToolDeploy, ToolInspect} +var readToolNames = []string{ToolValidate, ToolLint, ToolDryRun, ToolDoctor, ToolDeploy, ToolInspect, ToolRepair} -var writeToolNames = []string{ToolApply, ToolStart, ToolStop, ToolScaffoldConnector, ToolScaffoldProcessor} +var writeToolNames = []string{ToolApply, ToolStart, ToolStop, ToolScaffoldConnector, ToolScaffoldProcessor, ToolRepairApply} // TestNewServer_ReadToolsAlwaysRegistered is AC-1's catalog half: every read // tool is present regardless of AllowMutations. diff --git a/cmd/conduit/internal/mcp/tools_repair.go b/cmd/conduit/internal/mcp/tools_repair.go new file mode 100644 index 000000000..455603c36 --- /dev/null +++ b/cmd/conduit/internal/mcp/tools_repair.go @@ -0,0 +1,92 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mcp + +import ( + "context" + + "github.com/conduitio/conduit/cmd/conduit/internal/repair" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// RepairArgs is the repair (read) tool's input: the pipeline configuration +// YAML CONTENT to scan for machine-appliable fixes. +type RepairArgs struct { + Config string `json:"config" jsonschema:"the pipeline configuration YAML content to scan for machine-appliable fixes (a single pipeline document)"` +} + +// repair implements the repair (read) tool: run the same +// cmd/conduit/internal/repair.CollectContent engine `conduit pipelines +// repair` uses over the content, and return the proposed fixes, their +// safety classification, and a plan hash. Mutates nothing — parity with +// the always-on validate/lint/dry_run/deploy tools. Same engine as +// `conduit pipelines repair`. +func (s *server) repair(ctx context.Context, _ *sdkmcp.CallToolRequest, in RepairArgs) (*sdkmcp.CallToolResult, Result[repair.Plan], error) { + plan, err := repair.CollectContent(ctx, in.Config) + if err != nil { + return toolErr[repair.Plan](err) + } + return toolOK(true, plan, repair.SummarizePlan(plan)) +} + +// RepairApplyArgs is the repair_apply tool's input: the same content the +// repair tool was called with, the plan Hash from repair's result, and an +// optional Select of specific configPaths to apply. +// +// There is deliberately NO Escalate field here (design doc §5.2, AC-15): +// repair.ApplyInput.Escalate — the human-only Tier-1 override that permits +// applying a FixClassDataPath fix — is reachable only from the CLI's +// --escalate flag (cmd/conduit/root/pipelines/repair.go). An agent that +// explicitly Selects a data-path-adjacent fix's configPath still gets it +// back as FixOutcomeSkipped with repair.CodeDataPathFixRefused in the +// result's Fixes report — repair_apply NEVER applies one, and there is no +// schema field an agent could set to force it (mirrors AllowMutations +// being process-set only, server.go's Config doc). +type RepairApplyArgs struct { + Config string `json:"config" jsonschema:"the pipeline configuration YAML content to repair (must match what the repair tool scanned)"` + // Hash is schema-optional (like ApplyArgs.Hash) so the handler's own + // check below gives an actionable conduiterr-mapped error rather than + // the SDK's generic schema-validation error. + Hash string `json:"hash,omitempty" jsonschema:"the plan hash from the repair tool's result; repair_apply refuses to run unless this matches the freshly recomputed plan hash exactly"` + Select []string `json:"select,omitempty" jsonschema:"apply only the fix(es) at these configPaths; omit to apply every safe fix (the default)"` +} + +// repairApply implements the repair_apply (write) tool: recompute the plan +// from Config and apply it only if Hash matches the freshly recomputed +// plan's hash exactly (design doc §5.2, mirroring the apply tool's own +// stance — no "skip the hash" escape hatch for MCP). Applies FixClassSafe +// fixes by default (or the Select-ed subset); a FixClassDataPath fix is +// never applied — see RepairApplyArgs' doc. Only registered when the +// server was started with --allow-mutations. Same engine as `conduit +// pipelines repair --apply`. +func (s *server) repairApply(ctx context.Context, _ *sdkmcp.CallToolRequest, in RepairApplyArgs) (*sdkmcp.CallToolResult, Result[repair.Result], error) { + if in.Hash == "" { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, "hash is required (from the repair tool's result)") + ce.Suggestion = "call the repair tool first, review its proposed fixes, then pass its hash to repair_apply" + return toolErr[repair.Result](ce) + } + + res, err := repair.Apply(ctx, repair.ApplyInput{ + Content: in.Config, + Hash: in.Hash, + Select: in.Select, + // Escalate is always false here — see RepairApplyArgs' doc. + }) + if err != nil { + return toolErr[repair.Result](err) + } + return toolOK(true, res, repair.SummarizeResult(res)) +} diff --git a/cmd/conduit/internal/mcp/tools_repair_test.go b/cmd/conduit/internal/mcp/tools_repair_test.go new file mode 100644 index 000000000..c1e3bb90f --- /dev/null +++ b/cmd/conduit/internal/mcp/tools_repair_test.go @@ -0,0 +1,207 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mcp + +import ( + "context" + "testing" + + "github.com/conduitio/conduit/cmd/conduit/internal/repair" + json "github.com/goccy/go-json" + "github.com/matryer/is" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const repairablePipelineYAML = `version: "2.2" +pipelines: + - id: orders + status: " Running " + connectors: + - id: src + type: source + plugin: builtin:generator + processors: + - id: proc1 + type: base64.encode + workers: -2 +` + +// TestRepair_ReturnsClassifiedFixesAndHash is the repair (read) tool's core +// contract: it returns the proposed fixes, classified, with a hash, and +// mutates nothing (it is always registered, like deploy/validate/lint). +func TestRepair_ReturnsClassifiedFixesAndHash(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: false}) + cs := connectTestClient(t, srv) + + res, err := cs.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: ToolRepair, + Arguments: map[string]any{"config": repairablePipelineYAML}, + }) + is.NoErr(err) + is.True(!res.IsError) + + env := decodeStructuredContent[Result[repair.Plan]](t, res) + is.True(env.OK) + is.True(env.Result.Hash != "") + is.True(len(env.Result.Fixes) >= 2) // rename + status; workers is restart-class but still proposed +} + +// TestRepairApply_NotRegisteredWithoutAllowMutations is AC-16: repair_apply +// is absent from tool discovery (not merely erroring) unless the server was +// started with --allow-mutations — repair (read) is unaffected. +func TestRepairApply_NotRegisteredWithoutAllowMutations(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: false}) + cs := connectTestClient(t, srv) + names := listToolNames(t, cs) + + is.True(!containsName(names, ToolRepairApply)) + is.True(containsName(names, ToolRepair)) +} + +// TestRepairApply_AppliesSafeFixesByDefault is AC-17 through the MCP +// transport: a matching hash applies the safe fixes only, and returns the +// repaired content directly (it never writes to any file — MCP is +// content-in/content-out only). +func TestRepairApply_AppliesSafeFixesByDefault(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: true}) + cs := connectTestClient(t, srv) + + readRes, err := cs.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: ToolRepair, + Arguments: map[string]any{"config": repairablePipelineYAML}, + }) + is.NoErr(err) + plan := decodeStructuredContent[Result[repair.Plan]](t, readRes).Result + + applyRes, err := cs.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: ToolRepairApply, + Arguments: map[string]any{"config": repairablePipelineYAML, "hash": plan.Hash}, + }) + is.NoErr(err) + is.True(!applyRes.IsError) + + env := decodeStructuredContent[Result[repair.Result]](t, applyRes) + is.True(env.OK) + is.True(env.Result.Content != repairablePipelineYAML) // something changed + appliedAny := false + for _, f := range env.Result.Fixes { + if f.Outcome == repair.FixOutcomeApplied { + appliedAny = true + } + } + is.True(appliedAny) +} + +// TestRepairApply_MissingHash_RejectedBeforeApplying mirrors +// TestApply_MissingHash_RejectedBeforeCallingApplyPlan: repair_apply +// refuses a call with no hash, before ever touching the plan. +func TestRepairApply_MissingHash_RejectedBeforeApplying(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: true}) + cs := connectTestClient(t, srv) + + res, err := cs.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: ToolRepairApply, + Arguments: map[string]any{"config": repairablePipelineYAML}, + }) + is.NoErr(err) + is.True(res.IsError) + + env := decodeStructuredContent[Result[repair.Result]](t, res) + is.True(env.Error != nil) + is.True(env.Error.Suggestion != "") +} + +// TestRepairApply_StaleHash_Refused is AC-9 through the MCP transport: a +// hash that does not match the freshly recomputed plan is refused with +// repair.plan_stale. +func TestRepairApply_StaleHash_Refused(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: true}) + cs := connectTestClient(t, srv) + + res, err := cs.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: ToolRepairApply, + Arguments: map[string]any{"config": repairablePipelineYAML, "hash": "not-the-real-hash"}, + }) + is.NoErr(err) + is.True(res.IsError) + + env := decodeStructuredContent[Result[repair.Result]](t, res) + is.True(env.Error != nil) + is.Equal(env.Error.Code, repair.CodePlanStale.Reason()) +} + +// TestRepairApply_ToolSchema_HasNoEscalateField is AC-15's structural half: +// the repair_apply tool's input schema exposes no field an agent could set +// to force a data-path fix through — Escalate is reachable only via the +// CLI's --escalate flag (repair.ApplyInput.Escalate's doc), never from this +// tool's arguments. +func TestRepairApply_ToolSchema_HasNoEscalateField(t *testing.T) { + is := is.New(t) + + srv := NewServer(Config{AllowMutations: true}) + cs := connectTestClient(t, srv) + + res, err := cs.ListTools(context.Background(), nil) + is.NoErr(err) + + var found *sdkmcp.Tool + for _, tool := range res.Tools { + if tool.Name == ToolRepairApply { + found = tool + } + } + is.True(found != nil) // the tool was found + + // Round-trip the whole Tool through JSON and inspect the schema's + // declared properties generically — InputSchema's concrete client-side + // type is an SDK implementation detail; the JSON shape on the wire is + // the actual contract an agent sees. + b, err := json.Marshal(found) + is.NoErr(err) + var decoded struct { + InputSchema struct { + Properties map[string]any `json:"properties"` + } `json:"inputSchema"` + } + is.NoErr(json.Unmarshal(b, &decoded)) + + _, hasEscalate := decoded.InputSchema.Properties["escalate"] + is.True(!hasEscalate) + _, hasConfig := decoded.InputSchema.Properties["config"] + is.True(hasConfig) + _, hasHash := decoded.InputSchema.Properties["hash"] + is.True(hasHash) + _, hasSelect := decoded.InputSchema.Properties["select"] + is.True(hasSelect) +} + +func containsName(names []string, want string) bool { + for _, n := range names { + if n == want { + return true + } + } + return false +} diff --git a/cmd/conduit/internal/repair/apply.go b/cmd/conduit/internal/repair/apply.go new file mode 100644 index 000000000..cfe94c5fd --- /dev/null +++ b/cmd/conduit/internal/repair/apply.go @@ -0,0 +1,449 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "context" + "os" + "path/filepath" + "strconv" + + "github.com/conduitio/conduit/cmd/conduit/internal/validate" + "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/conduitio/conduit/pkg/provisioning/config" + "github.com/conduitio/yaml/v3" +) + +// conduiterr.Fix.Op's closed enum (design doc §3): the applier accepts only +// these three values, never a free-form string. +const ( + opSet = "set" + opRemove = "remove" + opAdd = "add" +) + +// FixOutcome is one selected fix's outcome after an Apply call. +type FixOutcome string + +const ( + FixOutcomeApplied FixOutcome = "applied" + FixOutcomeSkipped FixOutcome = "skipped" +) + +// AppliedFix reports what happened to one selected fix. +type AppliedFix struct { + ConfigPath string `json:"configPath"` + Outcome FixOutcome `json:"outcome"` + // Reason is set only when Outcome is FixOutcomeSkipped: the stable + // conduiterr code reason explaining why (repair.data_path_fix_refused, + // repair.fix_no_longer_applies). + Reason string `json:"reason,omitempty"` +} + +// ApplyInput is Apply's argument. Exactly one of Path/Content must be set — +// Path for the CLI (Apply reads and, on success, the CLI writes back to it +// atomically; Apply itself never writes to disk — see Apply's doc), Content +// for MCP (content-in, matching CollectContent/withTempConfigFile's rule +// elsewhere in this codebase: an MCP agent supplies pipeline config CONTENT, +// never a path on the server host). +type ApplyInput struct { + Path string + Content string + + // Hash is required, from a prior Collect/CollectContent call against + // the exact same bytes — no "skip the hash" escape hatch (design doc + // §5.2, mirroring the deploy/apply tool's own stance). + Hash string + + // Select is the list of ProposedFix.ConfigPath values to apply. Empty + // means "every FixClassSafe fix in the plan" (AC-17's default) — + // FixClassRestart and FixClassDataPath fixes are NEVER included in the + // default selection, regardless of Escalate. + Select []string + + // Escalate permits applying an explicitly Select-ed FixClassDataPath + // fix — the human-only Tier-1 override (design doc §4.2). It must be + // wired ONLY from the CLI's --escalate flag; the MCP repair_apply tool + // input has no field that can set this (see design doc §5.2's + // "deliberately no agent-settable escalate field" and AC-15's test). + // An explicitly Select-ed data-path fix without Escalate comes back in + // the result as FixOutcomeSkipped with Reason + // CodeDataPathFixRefused.Reason() — Apply itself never hard-fails for + // this alone (it is a per-fix policy skip, not a parse/hash failure); + // it is the CLI shell's job (not this engine's) to treat "I explicitly + // asked for this fix and it was refused" as the run's hard error + // (AC-14) — see cmd/conduit/root/pipelines/repair.go. MCP repair_apply + // surfaces the same skip as part of a normal, successful result + // (AC-15), since it never sets Escalate and therefore never has a + // "the human explicitly overrode this" case to enforce. + Escalate bool +} + +// Result is Apply's return value: the repaired content plus a per-fix +// applied/skipped report. Writing REPAIRED content to disk is the caller's +// choice (design doc §4.1) — Apply itself performs no file writes (AC-6): +// the CLI command atomically writes Content back to ApplyInput.Path (see +// WriteFileAtomic) only after Apply returns success; the MCP repair_apply +// tool returns Content directly and writes nothing. +type Result struct { + Path string `json:"path,omitempty"` + Content string `json:"content"` + Fixes []AppliedFix `json:"fixes"` +} + +// Apply re-collects the plan from the current bytes (Path or Content — +// exactly one of ApplyInput's two must be set), verifies the presented Hash +// still matches (else CodePlanStale, AC-9), resolves the selected fix set +// (default: every FixClassSafe fix, AC-17), and performs the edits on an +// in-memory YAML node tree — never touching a store or a running pipeline +// (AC-6, doc.go's central invariant). +// +// Each selected fix is applied independently, in ConfigPath order, against +// the SAME tree the previous fixes in this call already mutated — so a +// fix whose target the current tree no longer has (hand-edited away, a +// fix consumed by an earlier fix in this same batch, or the destination of +// a rename already occupied) is reported FixOutcomeSkipped with +// CodeFixNoLongerApplies, and every OTHER selected fix still applies +// (AC-19's partial-success contract). The write — via the caller — is +// all-or-nothing on the resulting bytes: there is exactly one call to +// marshalDoc, over the tree reflecting every fix that actually applied, so +// there is no such thing as a "half-written" fix. +// +// A structurally invalid fix — an Op outside {set, remove, add}, or a +// Value that cannot be coerced to its target field's known type (the +// v1 fix set's one non-string field, /workers) — aborts the ENTIRE call +// with an internal error and returns before any edit is even attempted +// (AC-3): that is a producer bug, not a runtime condition partial success +// can paper over. +// +// After every edit, Apply re-runs the same validate/lint engine over the +// repaired bytes and refuses to return success if any APPLIED fix's own +// ConfigPath is still flagged by a finding (AC-10) — the safety net that +// makes a bad producer visible in CI rather than in a user's pipeline. +func Apply(ctx context.Context, in ApplyInput) (Result, error) { + raw, validatePath, displayPath, cleanup, err := resolveApplyInput(in) + if err != nil { + return Result{}, err + } + defer cleanup() + + fresh, err := collect(ctx, validatePath, displayPath, raw) + if err != nil { + return Result{}, err + } + + if in.Hash == "" { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, "hash is required (from a prior repair Collect call)") + ce.Suggestion = "call repair's read step first, review the proposed fixes, then pass its hash to apply" + return Result{}, ce + } + if fresh.Hash != in.Hash { + ce := conduiterr.New(CodePlanStale, + "repair plan is stale: the presented hash does not match the current plan hash; "+ + "the file changed since the plan was computed") + ce.Suggestion = "re-run the repair read step to compute a fresh plan, review it, then apply its hash" + return Result{}, ce + } + + selected, err := selectFixes(fresh.Fixes, in.Select) + if err != nil { + return Result{}, err + } + if len(selected) == 0 { + ce := conduiterr.New(CodeNoFixesAvailable, "no appliable fixes in the plan") + ce.Suggestion = "nothing to repair — the config already passes validate for every machine-fixable finding" + return Result{}, ce + } + + doc, err := parseDoc(raw) + if err != nil { + return Result{}, err + } + root := documentRoot(doc) + if root == nil { + // Cannot happen: collect() above already required singlePipelineNode + // to succeed against these exact bytes. Guard anyway rather than + // panic on a nil deref below. + return Result{}, conduiterr.New(conduiterr.CodeInternal, "repair: could not re-parse the pipeline config for editing") + } + + report := make([]AppliedFix, 0, len(selected)) + for _, pf := range selected { + if pf.Code == "" { + // selectFixes's marker for "an explicitly selected ConfigPath + // that has no fix in the fresh plan at all" — never a producer + // fix, so it never reaches applyOne's op validation (which + // would otherwise misreport this as an invalid-op producer + // bug and abort the whole call). + report = append(report, AppliedFix{ConfigPath: pf.ConfigPath, Outcome: FixOutcomeSkipped, Reason: CodeFixNoLongerApplies.Reason()}) + continue + } + + if skip, reason := gateFix(pf, in.Escalate); skip { + report = append(report, AppliedFix{ConfigPath: pf.ConfigPath, Outcome: FixOutcomeSkipped, Reason: reason}) + continue + } + + applied, err := applyOne(root, pf) + if err != nil { + // AC-3: a structurally invalid fix aborts the whole call before + // any write — never a partial or corrupted result. + return Result{}, err + } + if !applied { + report = append(report, AppliedFix{ConfigPath: pf.ConfigPath, Outcome: FixOutcomeSkipped, Reason: CodeFixNoLongerApplies.Reason()}) + continue + } + report = append(report, AppliedFix{ConfigPath: pf.ConfigPath, Outcome: FixOutcomeApplied}) + } + + out, err := marshalDoc(doc) + if err != nil { + return Result{}, err + } + + if err := revalidate(ctx, report, out); err != nil { + return Result{}, err + } + + return Result{Path: displayPath, Content: string(out), Fixes: report}, nil +} + +// resolveApplyInput validates ApplyInput's Path/Content exclusivity and +// returns the source bytes plus the path validate.RunWithOptions should run +// against (a temp file for Content). cleanup always removes anything +// resolveApplyInput created — call it unconditionally via defer even on a +// later error. +func resolveApplyInput(in ApplyInput) (raw []byte, validatePath, displayPath string, cleanup func(), err error) { + noop := func() {} + switch { + case in.Path != "" && in.Content != "": + return nil, "", "", noop, conduiterr.New(conduiterr.CodeInvalidArgument, "repair: exactly one of path or content must be set, not both") + case in.Path != "": + b, rerr := os.ReadFile(in.Path) + if rerr != nil { + ce := conduiterr.Wrap(conduiterr.CodeInvalidArgument, "could not read pipeline config file: "+rerr.Error(), rerr) + ce.Suggestion = "check that the file exists and is readable" + return nil, "", "", noop, ce + } + return b, in.Path, in.Path, noop, nil + case in.Content != "": + raw := []byte(in.Content) + dir, derr := os.MkdirTemp("", "conduit-repair-*") + if derr != nil { + return nil, "", "", noop, conduiterr.Wrap(conduiterr.CodeInternal, "could not create a temporary directory for the pipeline config", derr) + } + path := filepath.Join(dir, "pipeline.yaml") + if werr := os.WriteFile(path, raw, 0o600); werr != nil { + _ = os.RemoveAll(dir) + return nil, "", "", noop, conduiterr.Wrap(conduiterr.CodeInternal, "could not write the temporary pipeline config file", werr) + } + return raw, path, "", func() { _ = os.RemoveAll(dir) }, nil + default: + return nil, "", "", noop, conduiterr.New(conduiterr.CodeInvalidArgument, "repair: path or content is required") + } +} + +// selectFixes resolves ApplyInput.Select against plan (AC-17's default and +// AC-20's ambiguity guard). Empty select means every FixClassSafe fix. +// Duplicate ConfigPaths across the FIXES BEING APPLIED (not just within +// select) are refused outright with CodeAmbiguousFix — repair never +// silently picks one of two candidates targeting the same field. +func selectFixes(plan []ProposedFix, sel []string) ([]ProposedFix, error) { + byPath := map[string][]ProposedFix{} + for _, pf := range plan { + byPath[pf.ConfigPath] = append(byPath[pf.ConfigPath], pf) + } + for path, group := range byPath { + if len(group) > 1 { + ce := conduiterr.New(CodeAmbiguousFix, "more than one candidate fix targets "+strconv.Quote(path)) + ce.ConfigPath = path + ce.Suggestion = "select the exact fix by its full identity is not yet supported; this configPath has ambiguous fixes and cannot be auto-applied" + return nil, ce + } + } + + if len(sel) == 0 { + var out []ProposedFix + for _, pf := range plan { + if pf.Class == FixClassSafe { + out = append(out, pf) + } + } + return out, nil + } + + out := make([]ProposedFix, 0, len(sel)) + for _, path := range sel { + group, ok := byPath[path] + if !ok || len(group) == 0 { + // Not a known fix in the current plan at all — reported as a + // skip (AC-19's "no longer applies" umbrella covers "was never + // there"), not a hard failure of the whole call. ProposedFix{} + // with a zero Code is this function's marker for "no fix + // exists for this selection"; Apply's loop checks Code == "" + // BEFORE calling applyOne, so this is never misread as a + // producer emitting an invalid Op. + out = append(out, ProposedFix{ConfigPath: path}) + continue + } + out = append(out, group[0]) + } + return out, nil +} + +// gateFix is Apply's Tier-1 policy check (design doc §4.2), factored out so +// it is directly unit-testable against synthetic ProposedFix values — +// nothing in the v1 producer set actually classifies FixClassDataPath (by +// design; see classify's doc), so exercising this gate end-to-end through +// Collect->Apply would need a producer this codebase deliberately does not +// have yet. skip is true iff pf must not be applied given escalate; reason +// is the stable code to report on the AppliedFix when skip is true. +func gateFix(pf ProposedFix, escalate bool) (skip bool, reason string) { + if pf.Class == FixClassDataPath && !escalate { + return true, CodeDataPathFixRefused.Reason() + } + return false, "" +} + +// applyOne performs pf's edit against root. It returns (applied=false, +// err=nil) for any "not applicable right now" condition (AC-19's +// CodeFixNoLongerApplies umbrella: target missing, rename destination +// already occupied) and a non-nil err only for a structurally invalid fix +// (AC-3) — the distinction the caller uses to decide "skip this one, keep +// going" vs. "abort the whole Apply". +func applyOne(root *yaml.Node, pf ProposedFix) (applied bool, err error) { + segs := documentPath(pf.Code, pf.Fix.ConfigPath) + + if pf.Code == config.CodeFieldRenamed.Reason() { + parent, last, ok := navigateParent(root, segs) + if !ok { + return false, nil + } + return applyRename(parent, last, pf.Fix.Value), nil + } + + switch pf.Fix.Op { + case opSet, opRemove, opAdd: + default: + return false, conduiterr.New(conduiterr.CodeInternal, + "repair: fix at "+pf.ConfigPath+" has an invalid op "+strconv.Quote(pf.Fix.Op)+" (want \"set\", \"remove\", or \"add\") — this is a producer bug") + } + + numeric := isNumericPath(pf.Fix.ConfigPath) + if numeric && pf.Fix.Op != opRemove { + if _, cerr := strconv.Atoi(pf.Fix.Value); cerr != nil { + return false, conduiterr.New(conduiterr.CodeInternal, + "repair: fix at "+pf.ConfigPath+" has a value that cannot be coerced to the target field's integer type: "+strconv.Quote(pf.Fix.Value)) + } + } + + parent, last, ok := navigateParent(root, segs) + if !ok { + return false, nil + } + + switch pf.Fix.Op { + case opSet: + return applySet(parent, last, pf.Fix.Value, numeric), nil + case opRemove: + return applyRemove(parent, last), nil + case opAdd: + return applyAdd(parent, last, pf.Fix.Value, numeric), nil + } + return false, nil // unreachable — validated above +} + +// revalidate re-runs the same validate/lint engine over the repaired bytes +// and fails closed (AC-10) if any APPLIED fix's own ConfigPath is still +// flagged by a finding — meaning the fix did not actually clear the problem +// it claimed to. This never runs against disk state the caller could see +// (a fresh temp file, cleaned up before returning) and never causes a +// partial write: Apply's caller only ever sees Content once this passes. +func revalidate(ctx context.Context, applied []AppliedFix, newRaw []byte) error { + dir, err := os.MkdirTemp("", "conduit-repair-verify-*") + if err != nil { + return conduiterr.Wrap(conduiterr.CodeInternal, "could not create a temporary directory to re-validate the repaired config", err) + } + defer func() { _ = os.RemoveAll(dir) }() + + path := filepath.Join(dir, "pipeline.yaml") + if err := os.WriteFile(path, newRaw, 0o600); err != nil { + return conduiterr.Wrap(conduiterr.CodeInternal, "could not write the repaired config for re-validation", err) + } + + report, err := validate.RunWithOptions(ctx, path, validate.Options{Warnings: true}) + if err != nil { + return conduiterr.Wrap(conduiterr.CodeInternal, "could not re-validate the repaired config", err) + } + if len(report.Files) != 1 { + return conduiterr.New(conduiterr.CodeInternal, "repair: re-validation produced an unexpected report shape") + } + + stillFlagged := make(map[string]bool, len(report.Files[0].Findings)) + for _, f := range report.Files[0].Findings { + stillFlagged[f.ConfigPath] = true + } + for _, a := range applied { + if a.Outcome != FixOutcomeApplied { + continue + } + if stillFlagged[a.ConfigPath] { + return conduiterr.New(conduiterr.CodeInternal, + "repair: the fix applied at "+a.ConfigPath+" did not clear its finding on re-validation; nothing was written") + } + } + return nil +} + +// WriteFileAtomic writes content to path via a temp file in the same +// directory plus rename — Invariant 5's "torn writes on crash must be +// impossible" applied to a config file: a crash between the temp write and +// the rename leaves the original path untouched (AC-11); a crash after the +// rename leaves the new content, fully written, never a half-written file. +// This is the CLI repair command's write step (see ApplyInput/Result's +// doc — Apply itself never writes to disk); MCP repair_apply never calls +// this at all. +func WriteFileAtomic(path string, content []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, ".repair-*.tmp") + if err != nil { + return cerrors.Errorf("could not create a temp file to write %q atomically: %w", path, err) + } + tmpPath := tmp.Name() + // Always attempt to remove the temp file; once Rename succeeds below + // this is a no-op (the path no longer exists under tmpPath). + defer func() { _ = os.Remove(tmpPath) }() + + if _, err := tmp.Write(content); err != nil { + _ = tmp.Close() + return cerrors.Errorf("could not write to temp file for %q: %w", path, err) + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return cerrors.Errorf("could not sync temp file for %q: %w", path, err) + } + if err := tmp.Close(); err != nil { + return cerrors.Errorf("could not close temp file for %q: %w", path, err) + } + if err := os.Chmod(tmpPath, perm); err != nil { + return cerrors.Errorf("could not set permissions on temp file for %q: %w", path, err) + } + if err := os.Rename(tmpPath, path); err != nil { + return cerrors.Errorf("could not atomically replace %q: %w", path, err) + } + return nil +} diff --git a/cmd/conduit/internal/repair/apply_test.go b/cmd/conduit/internal/repair/apply_test.go new file mode 100644 index 000000000..4f075d943 --- /dev/null +++ b/cmd/conduit/internal/repair/apply_test.go @@ -0,0 +1,306 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/matryer/is" +) + +// conduiterrFix is a tiny test constructor for conduiterr.Fix, purely to +// keep the ProposedFix literals below on one line each. +func conduiterrFix(path, op, value string) conduiterr.Fix { + return conduiterr.Fix{ConfigPath: path, Op: op, Value: value} +} + +// errAsConduitErr reads back a *conduiterr.ConduitError's Code reason for +// assertions below. +func errAsConduitErr(err error) (string, bool) { + ce, ok := conduiterr.Get(err) + if !ok { + return "", false + } + return ce.Code.Reason(), true +} + +// TestApply_Rename_AppliesAndPreservesComments is AC-10 (re-validation +// passes), AC-12 (comments/unrelated formatting preserved — only the +// targeted lines change), and the core happy path: Collect -> Apply with +// the fresh hash applies every FixClassSafe fix by default. +func TestApply_Rename_AppliesAndPreservesComments(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + plan, err := Collect(ctx, "testdata/rename.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 2) + + res, err := Apply(ctx, ApplyInput{Path: "testdata/rename.yaml", Hash: plan.Hash}) + is.NoErr(err) + is.Equal(len(res.Fixes), 2) + for _, f := range res.Fixes { + is.Equal(f.Outcome, FixOutcomeApplied) + } + + // Both "type" fields became "plugin"; the file no longer has any "type:" + // processor field, and every comment survives verbatim. + is.True(!strings.Contains(res.Content, "type: base64")) + is.True(strings.Contains(res.Content, "plugin: base64.encode")) + is.True(strings.Contains(res.Content, "plugin: base64.decode")) + is.True(strings.Contains(res.Content, "# deprecated field, should be renamed to \"plugin\"")) + is.True(strings.Contains(res.Content, "# trailing comment survives")) + + // Re-collecting the repaired content finds no more fixable findings. + after, err := CollectContent(ctx, res.Content) + is.NoErr(err) + is.Equal(len(after.Fixes), 0) + + // Apply never writes to disk itself — the source file is untouched. + orig, err := os.ReadFile("testdata/rename.yaml") + is.NoErr(err) + is.True(!strings.Contains(string(orig), "plugin: base64.encode")) +} + +// TestApply_Workers_ClassifiedRestart_NotInDefaultSelection is AC-17: the +// default selection (Select empty) only ever applies FixClassSafe fixes — +// workers.yaml's fix is FixClassRestart, so a default Apply call finds +// nothing to do. +func TestApply_Workers_ClassifiedRestart_NotInDefaultSelection(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + plan, err := Collect(ctx, "testdata/workers.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 1) + is.Equal(plan.Fixes[0].Class, FixClassRestart) + + _, err = Apply(ctx, ApplyInput{Path: "testdata/workers.yaml", Hash: plan.Hash}) + is.True(err != nil) // CodeNoFixesAvailable: nothing safe to apply by default + + // Explicitly selecting the restart fix DOES apply it. + res, err := Apply(ctx, ApplyInput{Path: "testdata/workers.yaml", Hash: plan.Hash, Select: []string{plan.Fixes[0].ConfigPath}}) + is.NoErr(err) + is.Equal(len(res.Fixes), 1) + is.Equal(res.Fixes[0].Outcome, FixOutcomeApplied) + is.True(strings.Contains(res.Content, "workers: 1")) + is.True(!strings.Contains(res.Content, "workers: -3")) +} + +// TestApply_StaleHash is AC-9: a hash that no longer matches the current +// file is refused with repair.plan_stale, nothing applied. +func TestApply_StaleHash(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + _, err := Apply(ctx, ApplyInput{Path: "testdata/rename.yaml", Hash: "not-a-real-hash"}) + is.True(err != nil) + ce, ok := errAsConduitErr(err) + is.True(ok) + is.Equal(ce, CodePlanStale.Reason()) +} + +// TestApply_MissingHash is AC-8's analogue for the engine (the CLI's +// --plan-hash/--yes UX sits above this): Hash is required, always. +func TestApply_MissingHash(t *testing.T) { + is := is.New(t) + _, err := Apply(context.Background(), ApplyInput{Path: "testdata/rename.yaml"}) + is.True(err != nil) +} + +// TestApply_NoFixesAvailable_OnCleanFile is AC-21: --apply against an +// already-clean file is repair.no_fixes_available, not silently OK:true +// with nothing in it. +func TestApply_NoFixesAvailable_OnCleanFile(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + plan, err := Collect(ctx, "testdata/clean.yaml") + is.NoErr(err) + + _, err = Apply(ctx, ApplyInput{Path: "testdata/clean.yaml", Hash: plan.Hash}) + is.True(err != nil) + ce, ok := errAsConduitErr(err) + is.True(ok) + is.Equal(ce, CodeNoFixesAvailable.Reason()) +} + +// TestApply_FixNoLongerApplies is AC-19: selecting a configPath the fresh +// plan doesn't actually have a fix for (e.g. it was hand-fixed between +// Collect and this Apply call using a DIFFERENT plan/hash than the one +// bound here — modeled directly by selecting an unknown path alongside a +// real one) skips that one with repair.fix_no_longer_applies while the +// other selected fix still applies. +func TestApply_FixNoLongerApplies_PartialSuccess(t *testing.T) { + is := is.New(t) + ctx := context.Background() + + plan, err := Collect(ctx, "testdata/rename.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 2) + + res, err := Apply(ctx, ApplyInput{ + Path: "testdata/rename.yaml", + Hash: plan.Hash, + Select: []string{ + plan.Fixes[0].ConfigPath, + "/pipelines/0/processors/99/type", // does not exist + }, + }) + is.NoErr(err) // partial success is not a hard failure + is.Equal(len(res.Fixes), 2) + + byPath := map[string]AppliedFix{} + for _, f := range res.Fixes { + byPath[f.ConfigPath] = f + } + is.Equal(byPath[plan.Fixes[0].ConfigPath].Outcome, FixOutcomeApplied) + is.Equal(byPath["/pipelines/0/processors/99/type"].Outcome, FixOutcomeSkipped) + is.Equal(byPath["/pipelines/0/processors/99/type"].Reason, CodeFixNoLongerApplies.Reason()) +} + +// TestApply_InvalidOp_AbortsEntireCall is AC-3: a structurally invalid fix +// (Op outside {set,remove,add}) aborts the WHOLE Apply call before any edit +// — never a partial write for this reason. +func TestApply_InvalidOp_AbortsEntireCall(t *testing.T) { + is := is.New(t) + + root, err := parseDoc([]byte("a: b\n")) + is.NoErr(err) + _, err = applyOne(documentRoot(root), ProposedFix{ + Code: "config.field_invalid", + ConfigPath: "/a", + Fix: conduiterrFix("/a", "rename", "b"), + }) + is.True(err != nil) // invalid op ("rename" is not set/remove/add) +} + +// TestApply_UncoercibleNumericValue_AbortsEntireCall is AC-3's other half: +// a Fix targeting the v1 fix set's one numeric field (/workers) whose Value +// cannot be parsed as an integer aborts the whole call — a producer bug, +// never a silent bad write. +func TestApply_UncoercibleNumericValue_AbortsEntireCall(t *testing.T) { + is := is.New(t) + + root, err := parseDoc([]byte("workers: 3\n")) + is.NoErr(err) + _, err = applyOne(documentRoot(root), ProposedFix{ + Code: "config.field_invalid", + ConfigPath: "/workers", + Fix: conduiterrFix("/workers", "set", "not-a-number"), + }) + is.True(err != nil) +} + +// TestRevalidate_CatchesAFixThatDidNotClearItsFinding is AC-10: the safety +// net that makes a bad producer visible in CI rather than in a user's +// pipeline. It calls the unexported revalidate directly against bytes that +// STILL have a finding at the ConfigPath a (simulated) "applied" fix +// claimed to clear — every real v1 producer is correct (see +// TestApply_Rename_AppliesAndPreservesComments etc.), so this is the only +// way to exercise the guard without hand-writing a deliberately-buggy +// producer. +func TestRevalidate_CatchesAFixThatDidNotClearItsFinding(t *testing.T) { + is := is.New(t) + + // status is still invalid ("bogus") after the "fix" — revalidate must + // refuse, since /status is still flagged. + const stillBroken = `version: "2.2" +pipelines: + - id: p1 + status: bogus + connectors: + - id: src + type: source + plugin: builtin:generator +` + err := revalidate(context.Background(), []AppliedFix{ + {ConfigPath: "/status", Outcome: FixOutcomeApplied}, + }, []byte(stillBroken)) + is.True(err != nil) +} + +// TestApply_AmbiguousFix proves repair never silently picks between two +// candidate fixes for the same ConfigPath (AC-20), by exercising +// selectFixes directly — the v1 producer set cannot itself generate a +// genuine collision, so this is tested at the unit level, same rationale +// as TestGateFix. +func TestApply_AmbiguousFix(t *testing.T) { + is := is.New(t) + + plan := []ProposedFix{ + {ConfigPath: "/status", Class: FixClassSafe, Fix: conduiterrFix("/status", "set", "running")}, + {ConfigPath: "/status", Class: FixClassSafe, Fix: conduiterrFix("/status", "set", "stopped")}, + } + _, err := selectFixes(plan, nil) + is.True(err != nil) + ce, ok := errAsConduitErr(err) + is.True(ok) + is.Equal(ce, CodeAmbiguousFix.Reason()) +} + +// TestWriteFileAtomic_ReplacesContentWholesale is AC-11: WriteFileAtomic +// never leaves a torn file — the original bytes are intact right up until +// the moment the new bytes are fully in place. +func TestWriteFileAtomic_ReplacesContentWholesale(t *testing.T) { + is := is.New(t) + + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte("original"), 0o600)) + + is.NoErr(WriteFileAtomic(path, []byte("repaired"), 0o600)) + + got, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(got), "repaired") + + // No leftover temp file in the directory. + entries, err := os.ReadDir(dir) + is.NoErr(err) + is.Equal(len(entries), 1) + is.Equal(entries[0].Name(), "pipeline.yaml") +} + +// TestWriteFileAtomic_FailureLeavesOriginalIntact simulates a crash mid +// -write (an unwritable target directory for the temp file) and asserts the +// original file is untouched — AC-11's "the original file is intact" +// half. +func TestWriteFileAtomic_FailureLeavesOriginalIntact(t *testing.T) { + is := is.New(t) + + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte("original"), 0o600)) + + // Make the directory read-only so CreateTemp in the same dir fails — + // simulates a write failure between "compute the repaired bytes" and + // "make them durable". + is.NoErr(os.Chmod(dir, 0o500)) + defer func() { _ = os.Chmod(dir, 0o700) }() // t.TempDir() cleanup needs write back + + err := WriteFileAtomic(path, []byte("repaired"), 0o600) + is.True(err != nil) + + is.NoErr(os.Chmod(dir, 0o700)) + got, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(got), "original") +} diff --git a/cmd/conduit/internal/repair/classify.go b/cmd/conduit/internal/repair/classify.go new file mode 100644 index 000000000..5d2ef6383 --- /dev/null +++ b/cmd/conduit/internal/repair/classify.go @@ -0,0 +1,143 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import "strings" + +// FixClass is the classifier's safety verdict on a ProposedFix, keyed on its +// conduiterr.Fix.ConfigPath. See classify's doc for the default-deny +// contract. +type FixClass string + +const ( + // FixClassSafe is pipeline/processor metadata or a structural field + // with no data-path effect — auto-appliable without escalation. + FixClassSafe FixClass = "safe" + // FixClassRestart would be an EffectRestart change + // (pkg/provisioning/plan.go's Effect) if deployed to a running + // pipeline, but is not itself ack/position/checkpoint-adjacent. + // Auto-appliable to the file; flagged so the human/agent knows a + // subsequent `deploy`/`apply` against a running pipeline would need to + // stop it first. + FixClassRestart FixClass = "restart" + // FixClassDataPath is ack/position/checkpoint-adjacent config — + // connector settings, a connector's own plugin/type, DLQ config, or any + // *.id field. Refused for auto-apply; routed to the human Tier-1 path + // (Escalate, CLI-only — see ApplyInput.Escalate's doc). + FixClassDataPath FixClass = "data_path" +) + +// classify classifies configPath (a conduiterr.Fix.ConfigPath — either +// pipeline-relative, e.g. "/status", or document-rooted, e.g. +// "/pipelines/0/processors/0/type"; classification is prefix-agnostic, see +// below) against the data-integrity invariants, before repair ever offers to +// apply it (design doc §4.2 — "the Tier-1 crux"). +// +// The check order matters and is deliberately conservative / default-deny +// (mirrors deploy.NewLocalService's own default-deny posture): +// +// 1. An explicit DENY pattern (connector settings, a connector's own +// plugin/type, DLQ config, any *.id field) always wins and classifies +// FixClassDataPath, regardless of anything else about the path. +// 2. Only then is the path checked against the small, explicit v1 SAFE/ +// RESTART allowlists (design doc §6's four producer classes). +// 3. Anything that matches neither — an unrecognized ConfigPath — falls +// through to FixClassDataPath. This is the default-deny guarantee +// (AC-5): an omission in the deny-list is an invariant risk (a +// data-path fix silently auto-applied); an omission in the allowlist +// is merely "repair can't auto-apply this yet," the safe direction to +// fail in. +// +// Matching is done on path SEGMENTS (split on "/", ignoring a leading +// empty segment from the leading "/"), not on a fixed prefix depth, so the +// same rules apply whether configPath is pipeline-relative (as +// pkg/provisioning/config.Validate's fixes are) or document-rooted (as the +// parser-warning rename fix is, see linter.go) — repair's own engine is the +// only caller that needs to reconcile the two addressing conventions (see +// yamlnode.go's documentPath), and it does so before storage, not before +// classification, so classify itself stays a pure, convention-agnostic +// pattern match. +func classify(configPath string) FixClass { + segs := pathSegments(configPath) + if len(segs) == 0 { + return FixClassDataPath // default-deny: no path at all + } + + // 1. Explicit deny patterns. + for _, s := range segs { + if s == "settings" || s == "dead-letter-queue" || s == "dlq" { + return FixClassDataPath + } + } + if segs[len(segs)-1] == "id" { + return FixClassDataPath + } + // A connector's OWN plugin/type (".../connectors//plugin" or + // ".../connectors//type") is data-path: changing which plugin a + // connector runs, or whether it's a source/destination, is a + // topology-changing, ack/position-adjacent edit. This is narrower than + // "any field named plugin/type" — a PROCESSOR's plugin/type (v1 item + // #1's rename target) is deliberately NOT caught here; see the safe + // allowlist below. + if n := len(segs); n >= 3 && segs[n-3] == "connectors" && (segs[n-1] == "plugin" || segs[n-1] == "type") { + return FixClassDataPath + } + + // 2. v1 SAFE / RESTART allowlist, anchored to the path SHAPES the producers + // emit (see classifyAllowlisted). Extracted so classify stays under the + // cyclomatic limit and the anchored shapes live in one place. + if class, ok := classifyAllowlisted(segs); ok { + return class + } + + // 3. Default-deny: an unrecognized ConfigPath shape is never safe. + return FixClassDataPath +} + +// classifyAllowlisted matches the specific config-path SHAPES the v1 fix +// producers emit (design doc §6, items #1-#4) — anchored on shape, NOT on bare +// segment names. Anchoring is what keeps the classifier default-deny by +// construction: a future field that merely ends in "status"/"description"/ +// "type"/"workers" in some other location returns ok=false and falls through to +// data_path in classify. ok=false means "not an allowlisted shape." +func classifyAllowlisted(segs []string) (FixClass, bool) { + n := len(segs) + last := segs[n-1] + switch { + case n == 1 && last == "status": // pipeline-level /status (item #2) + return FixClassSafe, true + case n == 1 && last == "description": // pipeline-level /description (item #4) + return FixClassSafe, true + case n >= 3 && segs[n-3] == "processors" && last == "type": + // A PROCESSOR's deprecated type->plugin rename (item #1). A connector's + // own /connectors//type was already denied in classify. + return FixClassSafe, true + case n >= 3 && segs[n-3] == "processors" && last == "workers": // processor /workers (item #3) + return FixClassRestart, true + } + return FixClassDataPath, false +} + +// pathSegments splits a JSON-pointer-shaped configPath into its non-empty +// segments — "/status" -> ["status"], "/connectors/0/settings/table" -> +// ["connectors","0","settings","table"]. An empty or root-only path yields +// an empty slice. +func pathSegments(configPath string) []string { + trimmed := strings.Trim(configPath, "/") + if trimmed == "" { + return nil + } + return strings.Split(trimmed, "/") +} diff --git a/cmd/conduit/internal/repair/classify_test.go b/cmd/conduit/internal/repair/classify_test.go new file mode 100644 index 000000000..ee03c3675 --- /dev/null +++ b/cmd/conduit/internal/repair/classify_test.go @@ -0,0 +1,97 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "testing" + + "github.com/matryer/is" +) + +// TestClassify_TableTest is AC-5: every ConfigPath a v1 producer can emit +// classifies as expected, and — the default-deny guarantee — anything this +// package does not explicitly recognize classifies FixClassDataPath, never +// FixClassSafe. +func TestClassify_TableTest(t *testing.T) { + tests := []struct { + name string + path string + want FixClass + }{ + {"pipeline status", "/status", FixClassSafe}, + {"pipeline description", "/description", FixClassSafe}, + {"processor type (pipeline-level, document-rooted)", "/pipelines/0/processors/0/type", FixClassSafe}, + {"processor type (connector-nested, document-rooted)", "/pipelines/0/connectors/1/processors/0/type", FixClassSafe}, + {"processor workers", "/processors/0/workers", FixClassRestart}, + {"nested processor workers", "/connectors/0/processors/2/workers", FixClassRestart}, + + {"connector settings", "/connectors/0/settings/table", FixClassDataPath}, + {"nested processor settings", "/connectors/0/processors/0/settings/key", FixClassDataPath}, + {"connector plugin", "/connectors/0/plugin", FixClassDataPath}, + {"connector type", "/connectors/0/type", FixClassDataPath}, + {"dlq field", "/dlq/plugin", FixClassDataPath}, + {"dead-letter-queue field (document-rooted)", "/pipelines/0/dead-letter-queue/plugin", FixClassDataPath}, + {"pipeline id", "/id", FixClassDataPath}, + {"connector id", "/connectors/0/id", FixClassDataPath}, + {"processor id", "/processors/0/id", FixClassDataPath}, + + // Default-deny: never seen before, must NOT be safe. + {"unrecognized field", "/some/made/up/field", FixClassDataPath}, + {"empty path", "", FixClassDataPath}, + + // Anchored allowlist (review M1): a field ending in a safe/restart name + // but in a NON-producer location must fall through to default-deny, not + // be auto-classified safe/restart on the bare name. + {"status nested, not pipeline-level", "/pipelines/0/status", FixClassDataPath}, + {"description nested, not pipeline-level", "/foo/description", FixClassDataPath}, + {"workers under connectors not processors", "/connectors/0/workers", FixClassDataPath}, + {"type not processor-anchored", "/schema/type", FixClassDataPath}, + {"workers not processor-anchored", "/foo/bar/workers", FixClassDataPath}, + {"root only", "/", FixClassDataPath}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + is := is.New(t) + is.Equal(classify(tc.path), tc.want) + }) + } +} + +// TestGateFix proves the Tier-1 escalation gate directly (design doc §4.2): +// a data_path fix is refused unless escalate is true; every other class is +// never gated here (Apply's default selection already excludes them, but +// gateFix itself must not second-guess an explicit safe/restart selection). +func TestGateFix(t *testing.T) { + tests := []struct { + name string + class FixClass + escalate bool + wantSkip bool + wantCode string + }{ + {"data_path without escalate is refused", FixClassDataPath, false, true, CodeDataPathFixRefused.Reason()}, + {"data_path with escalate is allowed", FixClassDataPath, true, false, ""}, + {"safe is never gated", FixClassSafe, false, false, ""}, + {"restart is never gated", FixClassRestart, false, false, ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + is := is.New(t) + skip, reason := gateFix(ProposedFix{Class: tc.class}, tc.escalate) + is.Equal(skip, tc.wantSkip) + is.Equal(reason, tc.wantCode) + }) + } +} diff --git a/cmd/conduit/internal/repair/codes.go b/cmd/conduit/internal/repair/codes.go new file mode 100644 index 000000000..521c345f7 --- /dev/null +++ b/cmd/conduit/internal/repair/codes.go @@ -0,0 +1,60 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "google.golang.org/grpc/codes" +) + +// Error codes this package registers in the single conduiterr registry +// (design doc §7). All five map to exit code 2 via pkg/conduit/exitcode's +// existing FailedPrecondition/InvalidArgument -> Validation-bucket mapping, +// consistent with deploy/apply's provisioning.CodePlanStale / +// provisioning.CodePipelineRunning. +var ( + // CodePlanStale is raised by Apply when the caller-presented hash does + // not match the hash freshly recomputed from the current file bytes — + // the direct analogue of provisioning.CodePlanStale (see plan.go). The + // file changed since the plan was shown and approved. + CodePlanStale = conduiterr.Register("repair.plan_stale", codes.FailedPrecondition) + + // CodeFixNoLongerApplies is raised per-fix (not for the whole Apply + // call) when a selected fix's target field no longer matches what the + // fix was computed against — either hand-edited between Collect and + // Apply in a way the hash's per-fix granularity didn't already catch, + // or consumed by an earlier fix in the same Apply batch (design doc §9, + // failure mode 1 and AC-19). Other selected fixes in the same call + // still apply; this fix is skipped, not fatal to the run. + CodeFixNoLongerApplies = conduiterr.Register("repair.fix_no_longer_applies", codes.FailedPrecondition) + + // CodeDataPathFixRefused is raised when a FixClassDataPath fix is + // selected for apply without the human-only Escalate path (design doc + // §4.2, the Tier-1 crux). The CLI surfaces this as a hard refusal + // (AC-14); MCP repair_apply — which never sets Escalate — surfaces it + // as a per-fix skip, never a fatal call (AC-15). + CodeDataPathFixRefused = conduiterr.Register("repair.data_path_fix_refused", codes.FailedPrecondition) + + // CodeAmbiguousFix is raised when more than one candidate fix targets + // the same ConfigPath and the selection does not disambiguate — repair + // never silently picks one (design doc §9, failure mode 3, AC-20). + CodeAmbiguousFix = conduiterr.Register("repair.ambiguous_fix", codes.FailedPrecondition) + + // CodeNoFixesAvailable is raised when --apply (or repair_apply) is + // requested but the plan contains zero appliable fixes. A read-mode + // call with zero fixes is exit 0 ("already clean"), never this code + // (AC-21). + CodeNoFixesAvailable = conduiterr.Register("repair.no_fixes_available", codes.InvalidArgument) +) diff --git a/cmd/conduit/internal/repair/collect.go b/cmd/conduit/internal/repair/collect.go new file mode 100644 index 000000000..79dea65f6 --- /dev/null +++ b/cmd/conduit/internal/repair/collect.go @@ -0,0 +1,246 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "sort" + "strconv" + + "github.com/conduitio/conduit/cmd/conduit/internal/validate" + "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/conduitio/yaml/v3" + json "github.com/goccy/go-json" +) + +// Plan is Collect/CollectContent's result: every fixable finding in the +// config, classified, plus a Hash binding this exact Plan — Apply refuses to +// run unless the caller presents this Hash (design doc §4.1, mirroring +// provisioning.Diff.Hash / ApplyPlan's hash check exactly). +type Plan struct { + // Path is the file path Collect was called with; empty for a + // CollectContent call (MCP content-in — no server-side path is ever + // echoed back to an agent, matching withTempConfigFile's own rule + // elsewhere in this codebase). + Path string `json:"path,omitempty"` + // Hash is a digest of the source bytes plus the ordered Fixes — ANY + // byte-for-byte change to the file (or, since Fixes is included, a + // change to the fixes computed from it) changes the hash. See + // computeHash. + Hash string `json:"hash"` + // Fixes is every finding in the config that carries a machine + // -appliable conduiterr.Fix, classified. A config with zero fixable + // findings still returns a valid (empty-Fixes) Plan and a nil error — + // "already clean" is not itself an error (design doc §5.1, AC-21). + Fixes []ProposedFix `json:"fixes"` +} + +// ProposedFix is one fixable finding, ready to render as a diff line (design +// doc §4.1). ConfigPath and Fix.ConfigPath are always identical: ConfigPath +// exists purely so a --json consumer doesn't have to reach into Fix for the +// single most common field. (The two are NOT always identical to the +// underlying validate.Finding.ConfigPath a rename-class warning carries — +// see documentPath's doc for why: repair always uses the Fix's own +// ConfigPath, which is unambiguous and directly walkable, never the +// finding's.) +type ProposedFix struct { + Code string `json:"code"` + Message string `json:"message"` + ConfigPath string `json:"configPath"` + Fix conduiterr.Fix `json:"fix"` + Class FixClass `json:"class"` + // Before/After are single-line, human-readable renderings of the + // current and proposed values — never a connector Settings value (see + // classify's doc: any settings-adjacent fix is always FixClassDataPath, + // and repair never even reaches this rendering step for one, since + // building Before/After only happens for fixes the v1 producer set + // actually emits, none of which touch settings). + Before string `json:"before,omitempty"` + After string `json:"after,omitempty"` +} + +// Collect runs the offline validate/lint engine (validate.RunWithOptions +// with Warnings enabled — the same engine `conduit pipelines lint` uses, +// which is a strict superset of what `validate` collects) over path, +// gathers every Finding that carries a non-nil Fix, classifies each, and +// returns a Plan. Pure read: no file writes, no store access (AC-6, AC-7). +func Collect(ctx context.Context, path string) (Plan, error) { + raw, err := os.ReadFile(path) + if err != nil { + ce := conduiterr.Wrap(conduiterr.CodeInvalidArgument, "could not read pipeline config file: "+err.Error(), err) + ce.Suggestion = "check that the file exists and is readable" + return Plan{}, ce + } + return collect(ctx, path, path, raw) +} + +// CollectContent is Collect's content-in variant for MCP (mirrors +// cmd/conduit/internal/mcp's withTempConfigFile pattern): same engine, YAML +// content instead of a server-side path. The returned Plan.Path is always +// empty — no server path is ever echoed back to an agent. +func CollectContent(ctx context.Context, content string) (Plan, error) { + raw := []byte(content) + dir, err := os.MkdirTemp("", "conduit-repair-*") + if err != nil { + return Plan{}, conduiterr.Wrap(conduiterr.CodeInternal, "could not create a temporary directory for the pipeline config", err) + } + defer func() { _ = os.RemoveAll(dir) }() + + // path is built from a fresh os.MkdirTemp directory plus a fixed literal + // filename — never from content or any other caller-supplied string — + // so there is no path-traversal surface here despite content itself + // being untrusted (an MCP agent's YAML); gosec's taint analysis flags + // this purely because CollectContent is an exported entry point. + path := filepath.Join(dir, "pipeline.yaml") + if err := os.WriteFile(path, raw, 0o600); err != nil { //nolint:gosec // path is MkdirTemp+literal, not derived from content + return Plan{}, conduiterr.Wrap(conduiterr.CodeInternal, "could not write the temporary pipeline config file", err) + } + + return collect(ctx, path, "", raw) +} + +// collect is Collect/CollectContent's shared body. validatePath is always a +// real file on disk (Collect's own path, or CollectContent's temp file) — +// validate.RunWithOptions needs one; displayPath is what the returned +// Plan.Path is set to (empty for CollectContent). +func collect(ctx context.Context, validatePath, displayPath string, raw []byte) (Plan, error) { + report, err := validate.RunWithOptions(ctx, validatePath, validate.Options{Warnings: true}) + if err != nil { + return Plan{}, err + } + if len(report.Files) != 1 { + // Can only happen for a directory input with zero or multiple + // resolved files — validate.Run tolerates that; repair does not + // (doc.go's v1 scope boundary). + ce := conduiterr.New(conduiterr.CodeInvalidArgument, "repair requires exactly one pipeline config file, not a directory") + ce.Suggestion = "pass the path to one .yml/.yaml file" + return Plan{}, ce + } + fr := report.Files[0] + if len(fr.Pipelines) == 0 { + // Zero pipelines almost always means the file didn't parse at all + // (a genuine YAML/version error, not "zero pipelines defined") — + // surface the ACTUAL parse finding's message/suggestion here rather + // than a generic "wrong pipeline count", which would otherwise mask + // what's really wrong (adversarial self-review finding, see the PR + // description). + if len(fr.Findings) > 0 { + f := fr.Findings[0] + ce := conduiterr.New(conduiterr.CodeInvalidArgument, "repair could not read a pipeline from "+validatePath+": "+f.Message) + ce.ConfigPath = f.ConfigPath + ce.Suggestion = f.Suggestion + if ce.Suggestion == "" { + ce.Suggestion = "run 'conduit pipelines validate " + validatePath + "' for the full finding" + } + return Plan{}, ce + } + ce := conduiterr.New(conduiterr.CodeInvalidArgument, "repair found no pipeline definition in "+validatePath) + ce.Suggestion = "run 'conduit pipelines validate " + validatePath + "' to see why" + return Plan{}, ce + } + if len(fr.Pipelines) > 1 { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, + "repair requires exactly one pipeline definition per file (found "+strconv.Itoa(len(fr.Pipelines))+")") + ce.Suggestion = "split the file into one pipeline per file — repair, like deploy/apply, edits a single pipeline document" + return Plan{}, ce + } + + doc, err := parseDoc(raw) + if err != nil { + return Plan{}, err + } + if _, ok := singlePipelineNode(doc); !ok { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, + "repair only supports a version-2.x pipeline config file with a single pipeline document") + ce.Suggestion = `set "version" to "2.2" (or run 'conduit pipelines validate' first) — v1 (map-keyed) configs are not supported for automated editing` + return Plan{}, ce + } + root := documentRoot(doc) + + var fixes []ProposedFix + for _, f := range fr.Findings { + if f.Fix == nil { + continue + } + fixes = append(fixes, buildProposedFix(root, f)) + } + sort.Slice(fixes, func(i, j int) bool { return fixes[i].ConfigPath < fixes[j].ConfigPath }) + + return Plan{ + Path: displayPath, + Hash: computeHash(raw, fixes), + Fixes: fixes, + }, nil +} + +// buildProposedFix converts one Finding carrying a non-nil Fix into a +// ProposedFix: classified, with Before/After rendered from the current YAML +// tree when the target resolves (it may legitimately not — e.g. a producer +// bug, or content Collect was handed that doesn't quite match what the +// finding was computed against — in which case Before/After are left empty +// rather than guessed; Apply's own re-navigation is the actual correctness +// gate, not this rendering step). +// +// Secrets guard (design doc §9, failure mode 7): no v1 producer targets a +// connector's settings, so every fix this codebase actually emits today is +// classified FixClassSafe/FixClassRestart and Before/After is never a +// secret. But that is true only because of what the current producer set +// happens to do — it is not enforced anywhere else. This function is the +// one place ProposedFix.Before/After get populated at all, so it is where +// the invariant is enforced structurally: a FixClassDataPath fix NEVER gets +// a rendered Before/After, even if some future producer targeted +// `settings` directly, closing the gap before it could ever leak a +// credential into a --json/MCP response or a rendered diff. +func buildProposedFix(root *yaml.Node, f validate.Finding) ProposedFix { + class := classify(f.Fix.ConfigPath) + pf := ProposedFix{ + Code: f.Code, + Message: f.Message, + ConfigPath: f.Fix.ConfigPath, + Fix: *f.Fix, + Class: class, + } + if class == FixClassDataPath { + return pf + } + + segs := documentPath(f.Code, f.Fix.ConfigPath) + if before, ok := valueAt(root, segs); ok { + pf.Before = before + } + pf.After = f.Fix.Value + return pf +} + +// computeHash returns a deterministic digest of raw (the source bytes) and +// fixes (in Plan.Fixes' own sorted order) — see Plan.Hash's doc. Marshal +// cannot fail for ProposedFix (a plain struct of strings and a nested plain +// struct); a failure here would be a bug in this package, matching +// pkg/provisioning/plan.go's own computeHash panic rationale. +func computeHash(raw []byte, fixes []ProposedFix) string { + h := sha256.New() + h.Write(raw) + b, err := json.Marshal(fixes) + if err != nil { + panic(cerrors.Errorf("repair: could not marshal fixes for hashing: %w", err)) + } + h.Write(b) + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/cmd/conduit/internal/repair/collect_test.go b/cmd/conduit/internal/repair/collect_test.go new file mode 100644 index 000000000..d6d227a64 --- /dev/null +++ b/cmd/conduit/internal/repair/collect_test.go @@ -0,0 +1,264 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/conduitio/conduit/cmd/conduit/internal/validate" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/conduitio/conduit/pkg/pipeline" + "github.com/matryer/is" +) + +// TestCollect_Rename is AC-1's golden test for the v1 starter set's item #1 +// (deprecated processor "type" -> "plugin", both pipeline-level and +// connector-nested): the Fix is correct, classified safe, and identical +// whether the fix comes from the pipeline-level or the connector-nested +// processor. +func TestCollect_Rename(t *testing.T) { + is := is.New(t) + + plan, err := Collect(context.Background(), "testdata/rename.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 2) // pipeline-level + connector-nested processor + + for _, f := range plan.Fixes { + is.Equal(f.Code, "config.field_renamed") + is.Equal(f.Class, FixClassSafe) + is.Equal(f.Fix.Op, "remove") + is.Equal(f.Fix.Value, "plugin") + is.True(strings.HasSuffix(f.Fix.ConfigPath, "/type")) + is.Equal(f.ConfigPath, f.Fix.ConfigPath) + } +} + +// TestCollect_Status is AC-1's golden test for item #2: an unambiguous +// case/whitespace-normalizable /status value gets a Fix; a genuinely +// ambiguous one does not (§6: "otherwise no fix"). +func TestCollect_Status(t *testing.T) { + is := is.New(t) + + plan, err := Collect(context.Background(), "testdata/status.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 1) + f := plan.Fixes[0] + is.Equal(f.Code, "config.field_invalid") + is.Equal(f.ConfigPath, "/status") + is.Equal(f.Class, FixClassSafe) + is.Equal(f.Fix.Op, "set") + is.Equal(f.Fix.Value, "running") + + plan, err = Collect(context.Background(), "testdata/status-ambiguous.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 0) // "bogus" has no deterministic canonical target +} + +// TestCollect_Workers is AC-1's golden test for item #3: a negative +// /workers gets Fix{Op:"set", Value:"1"}, classified restart (not safe). +func TestCollect_Workers(t *testing.T) { + is := is.New(t) + + plan, err := Collect(context.Background(), "testdata/workers.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 1) + f := plan.Fixes[0] + is.Equal(f.Code, "config.field_invalid") + is.True(strings.HasSuffix(f.ConfigPath, "/workers")) + is.Equal(f.Class, FixClassRestart) + is.Equal(f.Fix.Op, "set") + is.Equal(f.Fix.Value, "1") +} + +// TestCollect_Description is AC-1's golden test for item #4: an over-long +// /description gets Fix{Op:"set", Value:}, classified safe. The +// fixture is generated in-test (8192 is pipeline.DescriptionLengthLimit — +// too long to hand-write as a static YAML fixture). +func TestCollect_Description(t *testing.T) { + is := is.New(t) + + long := strings.Repeat("d", pipeline.DescriptionLengthLimit+100) + src := "version: \"2.2\"\npipelines:\n - id: repair-demo\n status: running\n description: \"" + long + "\"\n" + + " connectors:\n - id: src\n type: source\n plugin: builtin:generator\n" + + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte(src), 0o600)) + + plan, err := Collect(context.Background(), path) + is.NoErr(err) + is.Equal(len(plan.Fixes), 1) + f := plan.Fixes[0] + is.Equal(f.Code, "config.field_too_long") + is.Equal(f.ConfigPath, "/description") + is.Equal(f.Class, FixClassSafe) + is.Equal(f.Fix.Op, "set") + is.Equal(len(f.Fix.Value), pipeline.DescriptionLengthLimit) + is.Equal(f.Fix.Value, long[:pipeline.DescriptionLengthLimit]) +} + +// TestCollect_Clean is AC-21's read-mode half: a file with no fixable +// findings returns a valid, empty-Fixes Plan and a nil error — not an +// error. +func TestCollect_Clean(t *testing.T) { + is := is.New(t) + + plan, err := Collect(context.Background(), "testdata/clean.yaml") + is.NoErr(err) + is.Equal(len(plan.Fixes), 0) + is.True(plan.Hash != "") +} + +// TestCollect_CollectContent_Identical is AC-4: Collect and CollectContent +// return byte-identical Plan.Fixes/Plan.Hash for the same config supplied +// as a file vs. as content. +func TestCollect_CollectContent_Identical(t *testing.T) { + is := is.New(t) + + raw, err := os.ReadFile("testdata/rename.yaml") + is.NoErr(err) + + fromFile, err := Collect(context.Background(), "testdata/rename.yaml") + is.NoErr(err) + + fromContent, err := CollectContent(context.Background(), string(raw)) + is.NoErr(err) + + is.Equal(fromFile.Hash, fromContent.Hash) + is.Equal(len(fromFile.Fixes), len(fromContent.Fixes)) + for i := range fromFile.Fixes { + is.Equal(fromFile.Fixes[i], fromContent.Fixes[i]) + } + is.Equal(fromContent.Path, "") // never echoes a server-side path back +} + +// TestCollect_RejectsMultiPipeline is the v1 scope-boundary guard (doc.go): +// a file with more than one pipeline document is refused with a clear, +// coded error rather than repair silently picking one. +func TestCollect_RejectsMultiPipeline(t *testing.T) { + is := is.New(t) + + const src = `version: "2.2" +pipelines: + - id: p1 + status: running + - id: p2 + status: running +` + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte(src), 0o600)) + + _, err := Collect(context.Background(), path) + is.True(err != nil) +} + +// TestCollect_RejectsMultiDocument is the regression test for review B1: a +// multi-document ('---'-separated) file whose first document holds exactly one +// pipeline and whose trailing documents contribute zero pipelines slips past the +// pipeline-count guard (len(Pipelines)==1). Without the document-level check in +// parseDoc, marshalDoc re-emits only the first document, so --apply would +// silently drop the rest — config data loss. Collect must reject it. +func TestCollect_RejectsMultiDocument(t *testing.T) { + is := is.New(t) + + const src = `version: "2.2" +pipelines: + - id: p1 + status: RUNNING +--- +version: "2.2" +pipelines: [] +` + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte(src), 0o600)) + + _, err := Collect(context.Background(), path) + is.True(err != nil) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.True(strings.Contains(ce.Message, "single YAML document")) +} + +// TestCollect_UnparseableFile_SurfacesParseError is a regression test +// (adversarial self-review finding): a file that fails to parse entirely +// resolves to zero pipelines, which — before this fix — collect() +// misreported as "repair requires exactly one pipeline definition per file +// (found 0)", masking the actual YAML syntax error. It must instead surface +// the real parse finding's own message. +func TestCollect_UnparseableFile_SurfacesParseError(t *testing.T) { + is := is.New(t) + + const broken = "version: \"2.2\"\npipelines: [this is not valid yaml structure for pipelines\n" + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte(broken), 0o600)) + + _, err := Collect(context.Background(), path) + is.True(err != nil) + is.True(!strings.Contains(err.Error(), "found 0")) // the misleading message must be gone +} + +// TestBuildProposedFix_DataPathFix_NeverRendersBeforeAfter is the secrets +// guard from buildProposedFix's doc (design doc §9, failure mode 7): a +// FixClassDataPath fix never gets a rendered Before/After, even though its +// raw Fix is still carried through. No real v1 producer emits a data_path +// fix (see classify's doc), so this is exercised directly against a +// synthetic Finding, the same rationale as TestGateFix. +func TestBuildProposedFix_DataPathFix_NeverRendersBeforeAfter(t *testing.T) { + is := is.New(t) + + doc, err := parseDoc([]byte("pipelines:\n - connectors:\n - settings:\n apiKey: super-secret\n")) + is.NoErr(err) + root := documentRoot(doc) + + pf := buildProposedFix(root, validate.Finding{ + Code: "config.field_invalid", + Message: "hypothetical future finding", + ConfigPath: "/connectors/0/settings/apiKey", + Fix: &conduiterr.Fix{ConfigPath: "/connectors/0/settings/apiKey", Op: "set", Value: "super-secret-replacement"}, + }) + + is.Equal(pf.Class, FixClassDataPath) + is.Equal(pf.Before, "") + is.Equal(pf.After, "") + // The raw Fix itself (not a rendered diff) is still carried through — + // Apply needs it; it's the human/--json diff surface this guards. + is.Equal(pf.Fix.Value, "super-secret-replacement") +} + +// TestCollect_RejectsV1Config is the v1 scope-boundary guard for the +// map-keyed v1 config format (doc.go: "v1 configs are not supported for +// automated editing"). +func TestCollect_RejectsV1Config(t *testing.T) { + is := is.New(t) + + const src = `version: "1.1" +pipelines: + p1: + status: running +` + dir := t.TempDir() + path := filepath.Join(dir, "pipeline.yaml") + is.NoErr(os.WriteFile(path, []byte(src), 0o600)) + + _, err := Collect(context.Background(), path) + is.True(err != nil) +} diff --git a/cmd/conduit/internal/repair/doc.go b/cmd/conduit/internal/repair/doc.go new file mode 100644 index 000000000..b535130f7 --- /dev/null +++ b/cmd/conduit/internal/repair/doc.go @@ -0,0 +1,86 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package repair is the engine behind `conduit pipelines repair` and the MCP +// `repair`/`repair_apply` tools (see +// docs/design-documents/20260712-repair-command.md). It is the single code +// path both shells call — neither the CLI command nor the MCP tool contains +// repair logic of its own, matching cmd/conduit/internal/deploy and +// cmd/conduit/internal/validate's own "engine that thin cobra/MCP shells +// call" shape (CLAUDE.md's MCP rule: "Tools mirror CLI verbs... Same code +// paths as the CLI — no divergent logic"). +// +// # What repair is +// +// A ConduitError can carry a structured, machine-appliable Fix +// (pkg/foundation/cerrors/conduiterr.Fix — {ConfigPath, Op, Value}) alongside +// its human Suggestion. repair collects every Finding a validate/lint run +// produces that carries a non-nil Fix (see Collect), and can apply a chosen +// subset of them to the config file's YAML. +// +// # Invariant: repair edits a config FILE, never the store or a running +// pipeline +// +// This is the single most important safety property of this package. +// Collect/CollectContent are pure reads: they run the offline +// validate/lint engine and parse the file's own bytes into a YAML node tree +// purely to render a diff; neither opens a database, dials a Conduit +// server, or touches a running pipeline. Apply re-validates its inputs and +// performs the edits on an in-memory YAML node tree, returning the repaired +// bytes; it does not write to disk itself (see Apply's doc) and, like +// Collect, never opens a store or a running pipeline. The only path from a +// repaired file into a live engine is `deploy`/`apply`, which keeps its own +// plan-hash and Invariant-7 running-pipeline guard — repair does not +// duplicate, bypass, or shortcut that gate. +// +// # Invariant: default-deny classification +// +// Every proposed fix is classified (see Classify) against a JSON-pointer +// pattern of ack/position/checkpoint-adjacent config: connector `settings`, +// a connector's own `plugin`/`type`, any DLQ config, and any `id` field. +// A ConfigPath the classifier does not recognize is treated as data-path +// (FixClassDataPath) — refused for auto-apply — never as safe. This is +// deliberately conservative: an omission in the deny-list is an invariant +// risk (a data-path fix silently auto-applied), so the default runs the +// other way. See classify.go. +// +// # v1 scope +// +// Only a small, deliberately scoped starter set of error/warning sites +// populate a Fix today (design doc §6): a deprecated/renamed field +// (processor `type` -> `plugin`), an unambiguous `/status` enum +// normalization, a negative processor `/workers` clamped to 1, and an +// over-long `/description` truncation. Everything else keeps its human +// Suggestion and carries a nil Fix, exactly as before this package existed +// — repair reports "no machine-appliable fix for this finding" for those, +// honestly, rather than fabricating one. Widening this set (e.g. a plugin +// "did-you-mean" fix, or an ID-rename fix) is future, separate work — see +// the design doc's §6 "explicitly out of scope" table for why each is +// deferred. +// +// # v1 scope: single-pipeline v2 files only +// +// repair operates on exactly one file containing exactly one v2-format +// pipeline document — the same constraint deploy/apply already impose (see +// deploy.ParseSinglePipeline). This is a deliberate, documented +// scope-narrowing, not an oversight: the comment-preserving YAML node +// editor (yamlnode.go) locates a fix's target field by walking a +// JSON-pointer-shaped path from the document root, and a v1 (map-keyed, +// non-deterministically-ordered) config or a multi-pipeline/multi-document +// file would need a materially more complex addressing scheme for no +// benefit to the v1 fix set, which never touches those shapes. A file +// repair cannot edit losslessly is refused with a clear, coded error +// (config.parse_error) rather than a silent comment-stripping rewrite — the +// documented fallback from the design doc's §10 schedule-risk note. +package repair diff --git a/cmd/conduit/internal/repair/summary.go b/cmd/conduit/internal/repair/summary.go new file mode 100644 index 000000000..7d9a4a6d2 --- /dev/null +++ b/cmd/conduit/internal/repair/summary.go @@ -0,0 +1,62 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +// PlanSummary is the --json/MCP envelope's `summary` payload for a read +// -mode (Collect/CollectContent) repair result — a rollup of Plan.Fixes by +// FixClass. Lives here (not in the CLI or MCP shell) so both call the exact +// same rollup, mirroring cmd/conduit/internal/deploy.Summary/Summarize. +type PlanSummary struct { + Safe int `json:"safe"` + Restart int `json:"restart"` + DataPath int `json:"dataPath"` +} + +// SummarizePlan rolls plan.Fixes up by FixClass. +func SummarizePlan(plan Plan) PlanSummary { + var s PlanSummary + for _, f := range plan.Fixes { + switch f.Class { + case FixClassSafe: + s.Safe++ + case FixClassRestart: + s.Restart++ + case FixClassDataPath: + s.DataPath++ + } + } + return s +} + +// ApplySummary is the --json/MCP envelope's `summary` payload for an +// Apply result — a rollup of Result.Fixes by outcome. +type ApplySummary struct { + Applied int `json:"applied"` + Skipped int `json:"skipped"` +} + +// SummarizeResult rolls res.Fixes up by FixOutcome. +func SummarizeResult(res Result) ApplySummary { + var s ApplySummary + for _, f := range res.Fixes { + switch f.Outcome { + case FixOutcomeApplied: + s.Applied++ + case FixOutcomeSkipped: + s.Skipped++ + } + } + return s +} diff --git a/cmd/conduit/internal/repair/testdata/clean.yaml b/cmd/conduit/internal/repair/testdata/clean.yaml new file mode 100644 index 000000000..c179ce97d --- /dev/null +++ b/cmd/conduit/internal/repair/testdata/clean.yaml @@ -0,0 +1,8 @@ +version: "2.2" +pipelines: + - id: repair-demo + status: running + connectors: + - id: src + type: source + plugin: builtin:generator diff --git a/cmd/conduit/internal/repair/testdata/rename.yaml b/cmd/conduit/internal/repair/testdata/rename.yaml new file mode 100644 index 000000000..268eecf03 --- /dev/null +++ b/cmd/conduit/internal/repair/testdata/rename.yaml @@ -0,0 +1,15 @@ +version: "2.2" +pipelines: + - id: repair-demo + status: running + connectors: + - id: src + type: source + plugin: builtin:generator + processors: + - id: conn-proc + # deprecated field, should be renamed to "plugin" + type: base64.encode + processors: + - id: pipe-proc + type: base64.decode # trailing comment survives diff --git a/cmd/conduit/internal/repair/testdata/status-ambiguous.yaml b/cmd/conduit/internal/repair/testdata/status-ambiguous.yaml new file mode 100644 index 000000000..bb32e74b4 --- /dev/null +++ b/cmd/conduit/internal/repair/testdata/status-ambiguous.yaml @@ -0,0 +1,8 @@ +version: "2.2" +pipelines: + - id: repair-demo + status: bogus + connectors: + - id: src + type: source + plugin: builtin:generator diff --git a/cmd/conduit/internal/repair/testdata/status.yaml b/cmd/conduit/internal/repair/testdata/status.yaml new file mode 100644 index 000000000..dfcaca56d --- /dev/null +++ b/cmd/conduit/internal/repair/testdata/status.yaml @@ -0,0 +1,8 @@ +version: "2.2" +pipelines: + - id: repair-demo + status: " Running " + connectors: + - id: src + type: source + plugin: builtin:generator diff --git a/cmd/conduit/internal/repair/testdata/workers.yaml b/cmd/conduit/internal/repair/testdata/workers.yaml new file mode 100644 index 000000000..82a0b162f --- /dev/null +++ b/cmd/conduit/internal/repair/testdata/workers.yaml @@ -0,0 +1,12 @@ +version: "2.2" +pipelines: + - id: repair-demo + status: running + connectors: + - id: src + type: source + plugin: builtin:generator + processors: + - id: pipe-proc + plugin: base64.decode + workers: -3 diff --git a/cmd/conduit/internal/repair/yamlnode.go b/cmd/conduit/internal/repair/yamlnode.go new file mode 100644 index 000000000..e6a49adb9 --- /dev/null +++ b/cmd/conduit/internal/repair/yamlnode.go @@ -0,0 +1,310 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package repair + +import ( + "bytes" + "io" + "strconv" + "strings" + + "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/conduitio/conduit/pkg/provisioning/config" + "github.com/conduitio/yaml/v3" +) + +// yamlIndent is the indent width repair re-encodes a repaired file with. It +// matches the 2-space indent every hand-written and generated pipeline +// config fixture in this repository uses (see +// cmd/conduit/internal/validate/testdata and docs/design-documents' own +// examples) — using the encoder's own default (4) would reformat every +// line's indentation on any edit, which is not "unrelated formatting... +// preserved" (AC-12) even though the *comments* would still survive. +const yamlIndent = 2 + +// parseDoc decodes raw into a comment-preserving *yaml.Node document tree — +// the node-level editor this package's Apply mutates in place instead of a +// marshal-from-struct round trip (which would strip comments; see doc.go and +// the design doc's §10 schedule-risk note). +func parseDoc(raw []byte) (*yaml.Node, error) { + dec := yaml.NewDecoder(bytes.NewReader(raw)) + var doc yaml.Node + if err := dec.Decode(&doc); err != nil { + return nil, cerrors.Errorf("could not parse pipeline config as YAML: %w", err) + } + // repair edits a SINGLE YAML document (doc.go's v1 scope). The encoder in + // marshalDoc re-emits only the first document, so on a multi-document + // ('---'-separated) file --apply would silently drop every document after the + // first — a config data-loss bug the pipeline-count guard in collect does not + // catch (a first document with one pipeline followed by pipeline-less + // documents passes it). Reject a multi-document file here rather than corrupt + // it. io.EOF from the second Decode is the single-document case. + if err := dec.Decode(new(yaml.Node)); err == nil { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, + "repair requires a single YAML document per file; this file contains multiple '---'-separated documents") + ce.Suggestion = "split the file so each '---' document is its own file — repair edits one document and would otherwise drop the rest" + return nil, ce + } else if !cerrors.Is(err, io.EOF) { + return nil, cerrors.Errorf("could not parse pipeline config as YAML: %w", err) + } + return &doc, nil +} + +// marshalDoc re-encodes doc, preserving comments and (via yamlIndent) +// matching this repository's 2-space indent convention. +func marshalDoc(doc *yaml.Node) ([]byte, error) { + var buf bytes.Buffer + enc := yaml.NewEncoder(&buf) + enc.SetIndent(yamlIndent) + if err := enc.Encode(doc); err != nil { + return nil, cerrors.Errorf("could not re-encode repaired pipeline config: %w", err) + } + if err := enc.Close(); err != nil { + return nil, cerrors.Errorf("could not finalize repaired pipeline config: %w", err) + } + return buf.Bytes(), nil +} + +// documentRoot returns the top-level mapping node of a parsed document (doc +// is what parseDoc returns: a DocumentNode wrapping exactly one root node), +// or nil if doc is not a well-formed single-document mapping. +func documentRoot(doc *yaml.Node) *yaml.Node { + if doc == nil || doc.Kind != yaml.DocumentNode || len(doc.Content) != 1 { + return nil + } + root := doc.Content[0] + if root.Kind != yaml.MappingNode { + return nil + } + return root +} + +// singlePipelineNode returns the sole pipeline document's own node — root's +// "pipelines" sequence, required to have exactly one element — reporting ok +// = false for anything else (v1's map-keyed "pipelines" field, zero or more +// than one pipeline document, or a missing "pipelines" key entirely). This +// is the v1 scope boundary doc.go documents: repair only edits a single v2 +// -shaped pipeline document. +func singlePipelineNode(doc *yaml.Node) (*yaml.Node, bool) { + root := documentRoot(doc) + if root == nil { + return nil, false + } + _, pipelines, ok := mapLookup(root, "pipelines") + if !ok || pipelines.Kind != yaml.SequenceNode || len(pipelines.Content) != 1 { + return nil, false + } + return pipelines.Content[0], true +} + +// mapLookup returns the index of key's KEY node in m.Content (mapping +// Content is a flat [k0,v0,k1,v1,...] list — index is always even) and its +// paired VALUE node. ok is false when m is not a MappingNode or key is not +// present. +func mapLookup(m *yaml.Node, key string) (keyIdx int, value *yaml.Node, ok bool) { + if m == nil || m.Kind != yaml.MappingNode { + return -1, nil, false + } + for i := 0; i+1 < len(m.Content); i += 2 { + if m.Content[i].Value == key { + return i, m.Content[i+1], true + } + } + return -1, nil, false +} + +// documentPath reconciles the two ConfigPath addressing conventions +// producers use (see classify's doc and doc.go) into one document-rooted +// segment slice navigateParent can walk from documentRoot: +// +// - pkg/provisioning/config.Validate's fixes (v1 items #2-#4) are +// pipeline-relative ("/status", "/processors/0/workers", +// "/description") — repair.Collect only ever operates on a single +// pipeline document (see singlePipelineNode), so these are prefixed +// with "pipelines/0". +// - the parser-warning rename fix (v1 item #1, code == +// config.CodeFieldRenamed) is already document-rooted (e.g. +// "/pipelines/0/processors/0/type") — the decoder-hook path it was +// built from (linter.go's newWarning) already starts at the document +// root, so it is used as-is. +func documentPath(code, fixConfigPath string) []string { + segs := pathSegments(fixConfigPath) + if code == config.CodeFieldRenamed.Reason() { + return segs + } + return append([]string{"pipelines", "0"}, segs...) +} + +// navigateParent walks root along segments, returning the parent node that +// directly contains the FINAL segment (a MappingNode whose key is +// segments[len-1], or the SequenceNode index is not supported as a final +// segment — every v1 fix targets a scalar mapping value, never a whole +// sequence element) plus that final segment, for a caller to interpret via +// applySet/applyRemove/applyAdd/applyRename. ok is false if any +// intermediate segment does not resolve (a mapping key that doesn't exist, +// a sequence index out of range, or a scalar where a container was +// expected) — the caller's job is to turn that into +// CodeFixNoLongerApplies, never to guess. +func navigateParent(root *yaml.Node, segments []string) (parent *yaml.Node, last string, ok bool) { + if root == nil || len(segments) == 0 { + return nil, "", false + } + cur := root + for _, seg := range segments[:len(segments)-1] { + switch cur.Kind { + case yaml.MappingNode: + _, v, found := mapLookup(cur, seg) + if !found { + return nil, "", false + } + cur = v + case yaml.SequenceNode: + idx, err := strconv.Atoi(seg) + if err != nil || idx < 0 || idx >= len(cur.Content) { + return nil, "", false + } + cur = cur.Content[idx] + case yaml.DocumentNode, yaml.ScalarNode, yaml.AliasNode: + // A scalar/alias/document node has no child to descend into by + // key or index — the path expected more structure than the + // tree has. Same "not applicable" outcome as an unresolved + // mapping key or out-of-range sequence index, above. + return nil, "", false + default: + return nil, "", false + } + } + return cur, segments[len(segments)-1], true +} + +// valueAt returns the CURRENT scalar value at segments (via navigateParent + +// a mapping lookup of the final segment), for rendering ProposedFix.Before — +// never for editing. ok is false if the path does not resolve to an +// existing scalar mapping value. +func valueAt(root *yaml.Node, segments []string) (string, bool) { + parent, last, ok := navigateParent(root, segments) + if !ok || parent.Kind != yaml.MappingNode { + return "", false + } + _, v, found := mapLookup(parent, last) + if !found { + return "", false + } + return v.Value, true +} + +// applySet overwrites an EXISTING mapping key's value scalar in place +// (mutating the value node's Value/Tag directly, never replacing the node +// itself), so any comment attached to that value node survives untouched. +// ok is false if parent is not a mapping or key is not present — the +// CodeFixNoLongerApplies case. +func applySet(parent *yaml.Node, key, value string, numeric bool) bool { + _, v, ok := mapLookup(parent, key) + if !ok { + return false + } + setScalar(v, value, numeric) + return true +} + +// applyRemove splices a key/value pair out of a mapping's flat Content +// list. ok is false if parent is not a mapping or key is not present. +func applyRemove(parent *yaml.Node, key string) bool { + idx, _, ok := mapLookup(parent, key) + if !ok { + return false + } + parent.Content = append(parent.Content[:idx], parent.Content[idx+2:]...) + return true +} + +// applyAdd appends a brand-new key/value scalar pair to a mapping. ok is +// false if parent is not a mapping or key already exists (an "add" fix is +// only ever appliable when the target key is genuinely absent — a key that +// already exists is a producer bug or a stale fix, not silently overwritten +// by an add). +func applyAdd(parent *yaml.Node, key, value string, numeric bool) bool { + if parent == nil || parent.Kind != yaml.MappingNode { + return false + } + if _, _, exists := mapLookup(parent, key); exists { + return false + } + k := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key} + v := &yaml.Node{} + setScalar(v, value, numeric) + parent.Content = append(parent.Content, k, v) + return true +} + +// applyRename renames an existing mapping key IN PLACE — mutating only the +// KEY node's Value/Tag, leaving the paired VALUE node (and any comments +// attached to either node) completely untouched. This is how the compound +// rename fix (config.CodeFieldRenamed, v1 item #1) is actually applied: it +// is functionally identical to the "remove old key, add new key with the +// old value" the design doc frames it as (§3.1 — staying within the frozen +// three-op {set,remove,add} model), but preserves field position and +// comment attachment exactly, which a remove+add pair on a mapping's flat +// Content list would not reliably do. ok is false if oldKey is not present. +func applyRename(parent *yaml.Node, oldKey, newKey string) bool { + if parent == nil || parent.Kind != yaml.MappingNode { + return false + } + if _, _, exists := mapLookup(parent, newKey); exists { + // The destination key is already present (e.g. a processor that — + // unusually — sets both the deprecated "type" and the current + // "plugin"). Renaming into it would create a duplicate mapping key, + // which is not a valid edit. Report "not applicable" rather than + // corrupt the file. + return false + } + idx, _, ok := mapLookup(parent, oldKey) + if !ok { + return false + } + parent.Content[idx].Value = newKey + parent.Content[idx].Tag = "!!str" + return true +} + +// setScalar mutates node in place into a plain scalar carrying value, +// tagged !!int when numeric (the only non-string v1 target — /workers) or +// !!str otherwise (SetString's convenience styling, e.g. literal-block style +// for a multi-line description). Any comments already on node are +// untouched, since only Kind/Tag/Value/Style are reassigned here. +func setScalar(node *yaml.Node, value string, numeric bool) { + if numeric { + node.Kind = yaml.ScalarNode + node.Tag = "!!int" + node.Value = value + node.Style = 0 + node.Content = nil + return + } + node.Content = nil + node.SetString(value) +} + +// isNumericPath reports whether configPath's target is the v1 fix set's one +// non-string field — a processor's /workers — so the applier writes an +// unquoted integer scalar rather than guessing from Fix.Value's own +// (always-string) type. This is the "applier interprets Value per the +// target node's expected type from the config schema, not by guessing" +// rule from the design doc §3 — schema knowledge the v1 producer set is +// small enough to hardcode rather than needing a real schema lookup. +func isNumericPath(configPath string) bool { + return strings.HasSuffix(configPath, "/workers") +} diff --git a/cmd/conduit/internal/validate/engine.go b/cmd/conduit/internal/validate/engine.go index 1be840bd8..e697e9fdb 100644 --- a/cmd/conduit/internal/validate/engine.go +++ b/cmd/conduit/internal/validate/engine.go @@ -280,15 +280,25 @@ func findingFromError(e error) Finding { // warningFinding converts one parser Warning into an advisory (SeverityWarning) // Finding. Warnings locate by line/column in the source file rather than by a -// config-path JSON pointer, and carry the fixed CodeLintWarning code (the -// parser's warnings have no per-field conduiterr code of their own). A warning -// never affects the exit code unless `lint --strict` is set. +// config-path JSON pointer, and most carry the fixed CodeLintWarning code +// (the parser's warnings mostly have no per-field conduiterr code of their +// own) — except a rename-class warning (w.Code == config.CodeFieldRenamed, +// repair v1 starter set item #1), which carries its own code and a +// machine-appliable w.Fix straight through, so +// cmd/conduit/internal/repair.Collect (which runs this engine with +// Options.Warnings set) can find and offer it. A warning never affects the +// exit code unless `lint --strict` is set. func warningFinding(w yaml.Warning) Finding { + code := CodeLintWarning + if w.Code != "" { + code = w.Code + } return Finding{ Severity: SeverityWarning, - Code: CodeLintWarning, + Code: code, Message: w.Message, ConfigPath: w.Field, + Fix: w.Fix, Line: w.Line, Column: w.Column, } diff --git a/cmd/conduit/internal/validate/lint_test.go b/cmd/conduit/internal/validate/lint_test.go index d56b53208..a59e19560 100644 --- a/cmd/conduit/internal/validate/lint_test.go +++ b/cmd/conduit/internal/validate/lint_test.go @@ -38,10 +38,19 @@ func TestRunWithOptions_Warnings_SurfacesLocatedWarning(t *testing.T) { w := report.Files[0].Findings[0] is.Equal(w.Severity, SeverityWarning) - is.Equal(w.Code, CodeLintWarning) is.True(w.Line > 0) // located by line... is.True(w.Column > 0) // ...and column is.True(w.Message != "") + + // testdata/warning.yaml's processor uses the deprecated "type" field + // (processor 2.2 rename to "plugin") — a repair v1 starter set fix + // candidate (design doc 20260712-repair-command.md §6, item #1), so + // this warning carries the dedicated config.field_renamed code and a + // machine-appliable Fix rather than the generic CodeLintWarning. + is.Equal(w.Code, "config.field_renamed") + is.True(w.Fix != nil) + is.Equal(w.Fix.Op, "remove") + is.Equal(w.Fix.Value, "plugin") } // validate (Options{}) is errors-only: the same file lint flags a warning on diff --git a/cmd/conduit/root/pipelines/pipelines.go b/cmd/conduit/root/pipelines/pipelines.go index 67220664a..7840a68ae 100644 --- a/cmd/conduit/root/pipelines/pipelines.go +++ b/cmd/conduit/root/pipelines/pipelines.go @@ -39,6 +39,7 @@ func (c *PipelinesCommand) SubCommands() []ecdysis.Command { &DryRunCommand{}, &DeployCommand{}, &ApplyCommand{}, + &RepairCommand{}, &StartCommand{}, &StopCommand{}, } diff --git a/cmd/conduit/root/pipelines/repair.go b/cmd/conduit/root/pipelines/repair.go new file mode 100644 index 000000000..42457b378 --- /dev/null +++ b/cmd/conduit/root/pipelines/repair.go @@ -0,0 +1,297 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pipelines + +import ( + "context" + "fmt" + "io" + "os" + "strings" + + "github.com/conduitio/conduit/cmd/conduit/cecdysis" + "github.com/conduitio/conduit/cmd/conduit/internal/repair" + "github.com/conduitio/conduit/cmd/conduit/internal/ui" + "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/conduitio/ecdysis" +) + +var ( + _ cecdysis.CommandWithResult = (*RepairCommand)(nil) + _ ecdysis.CommandWithDocs = (*RepairCommand)(nil) + _ ecdysis.CommandWithFlags = (*RepairCommand)(nil) + _ ecdysis.CommandWithArgs = (*RepairCommand)(nil) +) + +// RepairArgs holds RepairCommand's positional argument. +type RepairArgs struct { + // Path is a single .yml/.yaml pipeline config file containing exactly + // one pipeline document — the same single-pipeline scope deploy/apply + // already impose (see cmd/conduit/internal/repair's doc for why). + Path string +} + +// RepairFlags holds RepairCommand's flags (design doc §5.1). +type RepairFlags struct { + Apply bool `long:"apply" usage:"apply the plan instead of only previewing it — safe fixes only, unless --fix selects others"` + PlanHash string `long:"plan-hash" usage:"the hash from a prior 'repair' read to apply; required with --apply unless --yes"` + Yes bool `long:"yes" short:"y" usage:"with --apply, apply the freshly recomputed plan directly, without requiring --plan-hash"` + Fix []string `long:"fix" usage:"apply only the fix(es) at this configPath; repeatable. Default (with --apply): every safe fix"` + Escalate bool `long:"escalate" usage:"permit applying an explicitly --fix-selected data-path-adjacent fix (ack/position/checkpoint-adjacent config) — human-only; never available to an agent via MCP"` + NoColor bool `long:"no-color" usage:"disable colored/glyph output even on a color-capable terminal"` +} + +// RepairCommand implements `conduit pipelines repair `: collects every +// machine-appliable Fix a validate/lint run finds (cmd/conduit/internal/repair.Collect), +// renders it as a diff with a plan hash (read, the default), and — only with +// --apply and a matching hash — applies the safe subset (or an explicit +// --fix selection) and atomically rewrites the file. See +// docs/design-documents/20260712-repair-command.md. +// +// Tier-1 safety (AC-14): a data-path-adjacent fix named via --fix without +// --escalate is refused — the whole run, nothing written — with +// repair.data_path_fix_refused. This CLI-level check exists because +// repair.Apply itself treats a refused data-path fix as a per-fix skip +// (procedural, like every other engine call in this codebase) so that MCP's +// repair_apply tool — which never sets Escalate — can report the same skip +// as part of a normal, successful result (AC-15) instead of a failed tool +// call. The CLI and MCP therefore see IDENTICAL engine behavior; they only +// differ in how they react to an explicitly-requested fix coming back +// skipped for this reason — see repair.ApplyInput.Escalate's doc. +type RepairCommand struct { + args RepairArgs + flags RepairFlags + + renderer *ui.Renderer +} + +func (c *RepairCommand) Usage() string { return "repair " } + +func (c *RepairCommand) Docs() ecdysis.Docs { + return ecdysis.Docs{ + Short: "Preview (and optionally apply) machine-appliable fixes for a pipeline config file", + Long: `Collects every finding 'conduit pipelines validate'/'lint' can find a structured, machine +-appliable fix for (a deprecated/renamed field, an unambiguous invalid /status value, a negative +processor /workers, an over-long /description) and shows the proposed edit as a diff, with a plan +hash. Mutates nothing on its own. + +Pass --apply (with --plan-hash from the read step, or --yes to bind to a freshly recomputed plan) +to apply the safe fixes — restart-class and data-path-adjacent fixes are never applied by default; +select one explicitly with --fix . A data-path-adjacent fix (connector settings, a +connector's own plugin/type, DLQ config, any id field) additionally requires --escalate — the +human-only Tier-1 override; there is no equivalent for the MCP repair_apply tool. + +repair edits the config FILE only — it never touches the pipeline store or a running pipeline. +Getting a repaired file into a running engine is still 'conduit pipelines deploy'/'apply', which +keeps its own plan-hash and running-pipeline guard.`, + Example: "conduit pipelines repair orders.yaml\n" + + "conduit pipelines repair orders.yaml --json\n" + + "conduit pipelines repair orders.yaml --apply --plan-hash 9f3a2c...\n" + + "conduit pipelines repair orders.yaml --apply --yes\n" + + "conduit pipelines repair orders.yaml --apply --yes --fix /processors/0/workers", + } +} + +func (c *RepairCommand) Flags() []ecdysis.Flag { + return ecdysis.BuildFlags(&c.flags) +} + +func (c *RepairCommand) Args(args []string) error { + if len(args) == 0 { + return cerrors.Errorf("requires a path to a pipeline config file") + } + if len(args) > 1 { + return cerrors.Errorf("too many arguments") + } + c.args.Path = args[0] + return nil +} + +func (c *RepairCommand) ResultCommand() string { return "pipelines.repair" } + +func (c *RepairCommand) ExecuteWithResult(ctx context.Context) (cecdysis.Outcome, error) { + c.renderer = ui.NewRenderer(rendererOutput(ctx), c.flags.NoColor) + + plan, err := repair.Collect(ctx, c.args.Path) + if err != nil { + return cecdysis.Outcome{}, err + } + + if !c.flags.Apply { + // Read mode: exit 0 whether or not there are fixes to offer (design + // doc §5.1) — this is procedural (like deploy), not evaluative. + return cecdysis.Outcome{ + OK: true, + Summary: repair.SummarizePlan(plan), + Result: plan, + }, nil + } + + hash := c.flags.PlanHash + if hash == "" { + if !c.flags.Yes { + ce := conduiterr.New(conduiterr.CodeInvalidArgument, + "repair --apply requires --plan-hash (from a prior 'conduit pipelines repair' read), or --yes to apply the freshly recomputed plan directly") + ce.Suggestion = "run 'conduit pipelines repair " + c.args.Path + "' first, review the plan, then pass its hash" + return cecdysis.Outcome{}, ce + } + hash = plan.Hash + } + + res, err := repair.Apply(ctx, repair.ApplyInput{ + Path: c.args.Path, + Hash: hash, + Select: c.flags.Fix, + Escalate: c.flags.Escalate, + }) + if err != nil { + return cecdysis.Outcome{}, err + } + + // AC-14: an EXPLICITLY --fix-selected data-path fix that came back + // skipped (no --escalate) refuses the whole run — see RepairCommand's + // doc for why this check lives here, at the CLI layer, rather than in + // the engine. + if len(c.flags.Fix) > 0 { + if bad, ok := firstDataPathRefusal(res.Fixes); ok { + ce := conduiterr.New(repair.CodeDataPathFixRefused, + fmt.Sprintf("fix at %q is data-path-adjacent (ack/position/checkpoint-adjacent config) and requires --escalate", bad)) + ce.ConfigPath = bad + ce.Suggestion = "re-run with --escalate to apply this fix (human-only — never available via MCP), or drop it from --fix" + return cecdysis.Outcome{}, ce + } + } + + appliedAny := false + for _, f := range res.Fixes { + if f.Outcome == repair.FixOutcomeApplied { + appliedAny = true + break + } + } + if appliedAny { + perm := os.FileMode(0o644) + if info, statErr := os.Stat(c.args.Path); statErr == nil { + perm = info.Mode().Perm() + } + if err := repair.WriteFileAtomic(c.args.Path, []byte(res.Content), perm); err != nil { + return cecdysis.Outcome{}, conduiterr.Wrap(conduiterr.CodeInternal, "could not write the repaired pipeline config file", err) + } + } + + return cecdysis.Outcome{ + OK: true, + Summary: repair.SummarizeResult(res), + Result: res, + }, nil +} + +func (c *RepairCommand) Render(outcome cecdysis.Outcome) string { + if c.renderer == nil { + c.renderer = ui.NewRenderer(io.Discard, true) + } + + switch result := outcome.Result.(type) { + case repair.Plan: + return renderPlan(c.renderer, result) + case repair.Result: + return renderApplyResult(c.renderer, result) + default: + return "" + } +} + +// firstDataPathRefusal returns the ConfigPath of the first fix skipped for +// repair.CodeDataPathFixRefused, if any. +func firstDataPathRefusal(fixes []repair.AppliedFix) (string, bool) { + for _, f := range fixes { + if f.Outcome == repair.FixOutcomeSkipped && f.Reason == repair.CodeDataPathFixRefused.Reason() { + return f.ConfigPath, true + } + } + return "", false +} + +func renderPlan(r *ui.Renderer, plan repair.Plan) string { + var b strings.Builder + if len(plan.Fixes) == 0 { + fmt.Fprintf(&b, "No machine-appliable fixes for %q — already clean.\n", plan.Path) + return b.String() + } + + fmt.Fprintf(&b, "Proposed fixes for %q (hash %s):\n", plan.Path, shortHash(plan.Hash)) + for _, f := range plan.Fixes { + fmt.Fprintf(&b, " %s [%-9s] %-24s %s -> %s\n", + r.DiffGlyph(diffGlyphForClass(f.Class)), string(f.Class), f.ConfigPath, renderValue(f.Before), renderValue(f.After)) + fmt.Fprintf(&b, " %s: %s\n", f.Code, f.Message) + } + fmt.Fprintf(&b, "\nRun: conduit pipelines repair --apply --plan-hash %s\n", plan.Hash) + return b.String() +} + +func renderApplyResult(r *ui.Renderer, res repair.Result) string { + var b strings.Builder + applied, skipped := 0, 0 + for _, f := range res.Fixes { + glyph := r.Glyph(ui.StatusPass) + if f.Outcome == repair.FixOutcomeSkipped { + glyph = r.Glyph(ui.StatusWarn) + skipped++ + } else { + applied++ + } + fmt.Fprintf(&b, " %s %-9s %s", glyph, string(f.Outcome), f.ConfigPath) + if f.Reason != "" { + fmt.Fprintf(&b, " (%s)", f.Reason) + } + fmt.Fprintln(&b) + } + fmt.Fprintf(&b, "\n%d applied, %d skipped.\n", applied, skipped) + if applied > 0 { + fmt.Fprintf(&b, "%q was updated.\n", res.Path) + } + return b.String() +} + +func renderValue(v string) string { + if v == "" { + return "" + } + return v +} + +func diffGlyphForClass(c repair.FixClass) ui.DiffAction { + switch c { + case repair.FixClassSafe: + return ui.DiffUpdate + case repair.FixClassRestart: + return ui.DiffUpdate + case repair.FixClassDataPath: + return ui.DiffDelete + default: + return ui.DiffUpdate + } +} + +// shortHash mirrors deploy/render.go's own shortHash — display-only, never +// used as the actual comparison token. +func shortHash(hash string) string { + const n = 8 + if len(hash) <= n { + return hash + } + return hash[:n] +} diff --git a/cmd/conduit/root/pipelines/repair_test.go b/cmd/conduit/root/pipelines/repair_test.go new file mode 100644 index 000000000..67cc289ad --- /dev/null +++ b/cmd/conduit/root/pipelines/repair_test.go @@ -0,0 +1,190 @@ +// Copyright © 2026 Meroxa, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pipelines + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/conduitio/conduit/cmd/conduit/internal/repair" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" + "github.com/matryer/is" +) + +const repairableYAML = `version: "2.2" +pipelines: + - id: orders + status: " Running " + connectors: + - id: src + type: source + plugin: builtin:generator + processors: + - id: proc1 + type: base64.encode + workers: -2 +` + +// TestRepairCommand_ReadMode_NeverWrites is AC-7: the default (no --apply) +// run writes nothing to disk and returns a non-empty hash. +func TestRepairCommand_ReadMode_NeverWrites(t *testing.T) { + is := is.New(t) + path := writePipelineFile(t, repairableYAML) + before, err := os.ReadFile(path) + is.NoErr(err) + + cmd := &RepairCommand{args: RepairArgs{Path: path}} + outcome, err := cmd.ExecuteWithResult(context.Background()) + is.NoErr(err) + is.True(outcome.OK) + + plan, ok := outcome.Result.(repair.Plan) + is.True(ok) + is.True(plan.Hash != "") + is.True(len(plan.Fixes) > 0) + + after, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(before), string(after)) + + is.True(cmd.Render(outcome) != "") +} + +// TestRepairCommand_Apply_MissingHashAndYes_Rejected mirrors +// TestApplyCommand_MissingHashAndYes_Rejected (AC-8): --apply requires +// --plan-hash unless --yes, checked before any write. +func TestRepairCommand_Apply_MissingHashAndYes_Rejected(t *testing.T) { + is := is.New(t) + path := writePipelineFile(t, repairableYAML) + + cmd := &RepairCommand{args: RepairArgs{Path: path}, flags: RepairFlags{Apply: true}} + _, err := cmd.ExecuteWithResult(context.Background()) + is.True(err != nil) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.Equal(ce.Code.Reason(), conduiterr.CodeInvalidArgument.Reason()) +} + +// TestRepairCommand_Apply_Yes_WritesSafeFixesOnly is the default-selection +// happy path (AC-17): --apply --yes applies only the safe fixes (the +// rename + status normalization here — /workers is restart-class and is +// left alone) and atomically rewrites the file. +func TestRepairCommand_Apply_Yes_WritesSafeFixesOnly(t *testing.T) { + is := is.New(t) + path := writePipelineFile(t, repairableYAML) + + cmd := &RepairCommand{args: RepairArgs{Path: path}, flags: RepairFlags{Apply: true, Yes: true}} + outcome, err := cmd.ExecuteWithResult(context.Background()) + is.NoErr(err) + is.True(outcome.OK) + + res, ok := outcome.Result.(repair.Result) + is.True(ok) + + after, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(after), res.Content) + is.True(strings.Contains(string(after), "plugin: base64.encode")) + is.True(strings.Contains(string(after), "status:")) + is.True(!strings.Contains(string(after), " Running ")) // normalized, whatever the quoting style + is.True(strings.Contains(string(after), "workers: -2")) // restart-class, not in default selection + + is.True(cmd.Render(outcome) != "") +} + +// TestRepairCommand_Apply_StaleHash_RefusedNoMutation is AC-9: a hand-edited +// file between the read step and --apply is refused with +// repair.plan_stale, and the file on disk is left byte-for-byte unchanged. +func TestRepairCommand_Apply_StaleHash_RefusedNoMutation(t *testing.T) { + is := is.New(t) + path := writePipelineFile(t, repairableYAML) + + before, err := os.ReadFile(path) + is.NoErr(err) + + cmd := &RepairCommand{args: RepairArgs{Path: path}, flags: RepairFlags{Apply: true, PlanHash: "not-the-real-hash"}} + _, err = cmd.ExecuteWithResult(context.Background()) + is.True(err != nil) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.Equal(ce.Code.Reason(), repair.CodePlanStale.Reason()) + + after, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(before), string(after)) +} + +// TestRepairCommand_Apply_UnknownFixSelection_NoMutation is the "selecting +// an unknown/no-longer-applicable configPath never mutates anything" half +// of AC-19: --fix naming something outside the current plan applies +// nothing and leaves the file untouched (it does not silently fall back to +// "apply the safe defaults instead"). +func TestRepairCommand_Apply_UnknownFixSelection_NoMutation(t *testing.T) { + is := is.New(t) + path := writePipelineFile(t, repairableYAML) + + before, err := os.ReadFile(path) + is.NoErr(err) + + plan, err := repair.Collect(context.Background(), path) + is.NoErr(err) + + cmd := &RepairCommand{ + args: RepairArgs{Path: path}, + flags: RepairFlags{ + Apply: true, + PlanHash: plan.Hash, + Fix: []string{"/connectors/0/settings/table"}, // not a fix the plan actually offers + }, + } + outcome, err := cmd.ExecuteWithResult(context.Background()) + is.NoErr(err) // an unmatched --fix selection is a per-fix skip, not a hard failure + + res, ok := outcome.Result.(repair.Result) + is.True(ok) + is.Equal(len(res.Fixes), 1) + is.Equal(res.Fixes[0].Outcome, repair.FixOutcomeSkipped) + is.Equal(res.Fixes[0].Reason, repair.CodeFixNoLongerApplies.Reason()) + + after, err := os.ReadFile(path) + is.NoErr(err) + is.Equal(string(before), string(after)) // nothing applied, nothing written +} + +// TestFirstDataPathRefusal is a direct unit test of the CLI-level AC-14 +// check (RepairCommand's doc explains why this lives here rather than in +// the engine): the v1 producer set never actually classifies a fix +// FixClassDataPath (see cmd/conduit/internal/repair's classify doc), so +// there is no fixture that can drive this path end-to-end through +// ExecuteWithResult — this proves the detection logic itself directly +// against a synthetic report, the same rationale as the engine's own +// TestGateFix. +func TestFirstDataPathRefusal(t *testing.T) { + is := is.New(t) + + path, ok := firstDataPathRefusal([]repair.AppliedFix{ + {ConfigPath: "/status", Outcome: repair.FixOutcomeApplied}, + {ConfigPath: "/connectors/0/settings/table", Outcome: repair.FixOutcomeSkipped, Reason: repair.CodeDataPathFixRefused.Reason()}, + }) + is.True(ok) + is.Equal(path, "/connectors/0/settings/table") + + _, ok = firstDataPathRefusal([]repair.AppliedFix{ + {ConfigPath: "/status", Outcome: repair.FixOutcomeApplied}, + }) + is.True(!ok) +} diff --git a/docs/architecture-decision-records/20260713-repair-edits-file-not-store.md b/docs/architecture-decision-records/20260713-repair-edits-file-not-store.md new file mode 100644 index 000000000..7acb5f094 --- /dev/null +++ b/docs/architecture-decision-records/20260713-repair-edits-file-not-store.md @@ -0,0 +1,89 @@ +# `repair` edits the config file, never the store or a running pipeline + +## Summary + +`conduit pipelines repair` and the MCP `repair`/`repair_apply` tools apply a structured, +machine-appliable `conduiterr.Fix` to a pipeline configuration by editing an in-memory YAML node +tree and returning (or writing back) the repaired bytes. `repair` never opens the provisioning +store, never dials a running Conduit server, and never mutates a running pipeline — the only path +from a repaired file into a live engine remains `conduit pipelines deploy`/`apply`, unchanged. +This is an architectural boundary, not an incidental implementation detail, and future work on +`repair` (widening the v1 fix set, adding a store-aware fix class) must not cross it without a new +ADR. + +## Context + +The execution plan (`docs/design-documents/20260704-phase-1-execution-plan.md` §2, §3) calls for a +`repair` verb that applies a `ConduitError`'s structured `Fix` field, "sharing MCP `repair`'s +engine so the human isn't a second-class citizen to the agent," and explicitly requires that "agent +`repair` touching ack/position/checkpoint-adjacent config still requires the human Tier-1 sign-off +path" (§2). `docs/design-documents/20260712-repair-command.md` is the design doc that scoped the +feature; §11 ("Alternatives considered") records the store-vs-file decision made there and flags +that it should be promoted to an ADR once built. This is that ADR. + +Two shapes were on the table for what "apply a fix" means: + +1. **Apply directly to the pipeline store** (or a running pipeline via the API), the same way + `deploy`/`apply` do — `repair` would call into `pkg/provisioning.Service` and inherit its + plan-hash + running-pipeline guard machinery directly. +2. **Apply to the config file only**, as a text edit — `repair` never touches the store; getting a + repaired file into a running engine stays `deploy`/`apply`'s job, which already owns that gate. + +## Decision + +`repair` is a **config file editor**, never a store/pipeline mutator (option 2). Concretely: + +- `cmd/conduit/internal/repair.Collect`/`CollectContent` are pure reads: they run the existing + offline `validate`/`lint` engine (`cmd/conduit/internal/validate`) and parse the file's own bytes + into a `*yaml.Node` tree purely to render a diff. Neither function imports the + `pkg/provisioning` service, `pkg/pipeline`, or any store/API client package — the only + provisioning dependency is `pkg/provisioning/config` (a pure config/validation package, for the + `config.CodeFieldRenamed` constant), which touches no store or running pipeline. The + store/service boundary is enforced structurally by the import graph, not by convention or a + runtime check. +- `repair.Apply` performs its edits on that same in-memory node tree and returns the repaired + bytes. It does not write to disk itself (design doc §4.1: "Writing to disk is the caller's + choice"): the CLI command atomically rewrites the source file (temp + rename) only after `Apply` + returns success; the MCP `repair_apply` tool returns the repaired content directly and writes + nothing anywhere. +- The classifier (`classify`, `cmd/conduit/internal/repair/classify.go`) is default-deny over + ack/position/checkpoint-adjacent `ConfigPath`s (connector `settings`, a connector's own + `plugin`/`type`, DLQ config, any `id` field) — but this classification governs whether a fix is + _offered for auto-apply to the file at all_, not whether it may reach a running pipeline. Even a + `FixClassSafe`-classified fix, once written to the file, still has to pass through + `deploy`/`apply`'s own plan-hash and Invariant-7 running-pipeline guard + (`provisioning.CodePlanStale`, `provisioning.CodePipelineRunning`) before it affects anything + live. `repair` does not shortcut, bypass, or duplicate that gate. + +## Consequences + +- **`repair` cannot, by construction, corrupt or reorder a record, or violate invariants 1-7**: it + has no code path that reaches the data path at all. The worst a bad fix can do is write an + incorrect config file, which `deploy`/`apply`'s own re-validation and running-pipeline refusal + catch before anything live is affected — and `repair.Apply` itself re-validates the repaired + bytes before returning success (AC-10 in the design doc), so a bad producer is visible in CI, not + in a user's pipeline. +- **A repaired file is not itself a deployed change.** A human or agent that runs `repair --apply` + still has to run `deploy`/`apply` (or the MCP `deploy`/`apply` tools) to get the change into a + running Conduit — this is a deliberate two-step flow, not a missing feature. `repair` never grew + a `--deploy`-after-apply convenience flag in v1; if that gap proves painful, it is a separate, + explicit follow-up (composing two already-gated verbs, not a new gate). +- **This bounds what a future wider fix set can do without a new ADR.** A fix class that could only + be applied correctly with knowledge of the live/stored pipeline state (e.g. the design doc's + deferred connector-ID-rename fix, which needs to know whether a pipeline is new or already has a + stored position) cannot be added to `repair` as currently architected without either (a) still + routing through `deploy`/`apply`'s existing store access, unchanged, or (b) revisiting this + decision explicitly. Silently reaching into the store from a future `repair` producer or the + engine itself would violate the boundary this ADR records. + +## Related + +- `docs/design-documents/20260712-repair-command.md` — the design doc this ADR promotes §11's + decision from; §4.3 ("What repair mutates, and what it does not") and §9 (failure mode 2) reason + through the same boundary in more detail. +- `docs/design-documents/20260704-phase-1-execution-plan.md` §2, §3 — the source requirement. +- `docs/design-documents/20260708-cli-pipeline-deploy-apply.md` / + `20260708-live-server-deploy-apply.md` — the plan-hash + running-pipeline gate `repair` + deliberately does not duplicate. +- `cmd/conduit/internal/repair` — the implementation; see its package `doc.go` for the same + invariant stated at the code level. diff --git a/docs/design-documents/20260712-repair-command.md b/docs/design-documents/20260712-repair-command.md index 174054c5a..05249d08f 100644 --- a/docs/design-documents/20260712-repair-command.md +++ b/docs/design-documents/20260712-repair-command.md @@ -1,10 +1,14 @@ # `conduit pipelines repair` + MCP `repair` — design/plan -_Status: proposed (design/plan only — no implementation). Advances the Phase 1 execution +_Status: implemented (v1 starter set — see §6). Advances the Phase 1 execution plan (`docs/design-documents/20260704-phase-1-execution-plan.md`) §3 (CLI verb parity) and §2 (agent-native), which call for `conduit pipeline repair [--apply]` as a "thin applier of the structured `fix` field, sharing MCP `repair`'s engine so the human isn't a second-class citizen -to the agent."_ +to the agent." Implementation: `cmd/conduit/internal/repair` (engine), +`cmd/conduit/root/pipelines/repair.go` (CLI), `cmd/conduit/internal/mcp/tools_repair.go` (MCP). +The store-vs-file boundary this doc's §11 reasoned through is now recorded as +[ADR 20260713](../architecture-decision-records/20260713-repair-edits-file-not-store.md). See +that PR's description for exactly which of the acceptance criteria below are met._ > **The honest headline, up front.** The structured `Fix` field this feature applies **already > exists** in the codebase and already round-trips over gRPC and JSON. What does **not** exist is diff --git a/docs/operations/mcp-server.md b/docs/operations/mcp-server.md index dc5e07546..d8416aa88 100644 --- a/docs/operations/mcp-server.md +++ b/docs/operations/mcp-server.md @@ -32,16 +32,39 @@ conduit mcp --api-address localhost:8084 | `doctor` | no | no | `conduit doctor` | | `deploy` | no (preview only) | no | `conduit pipelines deploy` | | `inspect` | no | no | `conduit pipelines inspect` (requires `--api-address`) | +| `repair` | no (preview only) | no | `conduit pipelines repair` | | `apply` | yes | **yes** | `conduit pipelines apply` | | `start` | yes (running pipeline) | **yes** | `conduit pipelines start` (requires `--api-address`) | | `stop` | yes (running pipeline) | **yes** | `conduit pipelines stop` (requires `--api-address`) | | `scaffold_connector` | yes (filesystem) | **yes** | `conduit connector new` | | `scaffold_processor` | yes (filesystem) | **yes** | `conduit processor new` | - -`validate`/`lint`/`dry_run`/`deploy`/`apply` take pipeline configuration as a `config` string -(inline YAML content) — never a server-side file path. An agent naturally has content, not a path -on the machine `conduit mcp` runs on; accepting a path would let an agent read/validate arbitrary -files on that host. +| `repair_apply` | yes (the config content only — never the store or a running pipeline) | **yes** | `conduit pipelines repair --apply` | + +`validate`/`lint`/`dry_run`/`deploy`/`apply`/`repair`/`repair_apply` take pipeline configuration as +a `config` string (inline YAML content) — never a server-side file path. An agent naturally has +content, not a path on the machine `conduit mcp` runs on; accepting a path would let an agent +read/validate arbitrary files on that host. + +### `repair` / `repair_apply` + +`repair` scans a pipeline configuration for findings that carry a structured, machine-appliable fix +(a deprecated/renamed field, an unambiguous invalid `status` value, a negative processor +`workers` count, an over-long `description` — see +[`docs/design-documents/20260712-repair-command.md`](../design-documents/20260712-repair-command.md) +§6 for the full v1 scope) and returns each proposed fix classified `safe` / `restart` / `data_path`, +with a plan hash. It never mutates anything, including the config content itself — it only reads. + +`repair_apply` applies the plan `repair` computed — every `safe` fix by default, or the `select`-ed +subset — only if `hash` still matches the freshly recomputed plan exactly. It **never applies a +`data_path` fix**, even if one is named explicitly via `select`: that fix comes back in the result +as a skipped entry with `repair.data_path_fix_refused`. There is **no `escalate` field** in this +tool's input schema — the data-path override is reachable only from the CLI's `conduit pipelines +repair --apply --escalate` flag, a human-only path, by design (mirrors `--allow-mutations` being +process-set only, never a tool argument). + +Both tools are content-in/content-out only: `repair_apply` returns the repaired YAML in its result; +it never writes to a file, the pipeline store, or a running pipeline. Getting the repaired +configuration into a running engine is still `deploy`/`apply`'s job. Every tool result is a structured envelope: `{ok, summary, result, error}`. On a domain failure, `error` carries `{code, message, suggestion, fix}` — the same machine-actionable fields the CLI's @@ -150,15 +173,21 @@ Rotating the shared token today means restarting `conduit mcp` with a new `--tok ## Known limitations (Wave 3) -- **`repair` tool**: not built — the CLI `repair` verb doesn't exist yet. It will ship as an MCP - tool in the same PR as the CLI command (same-engine rule). -- **`llms.txt` tool-catalog generation**: the full multi-source `llms.txt` generator (config - schema, connector list, error registry, and this tool catalog) is deferred; the tool catalog - itself is stable and documented here in the interim. +- **`repair` scope is v1-narrow by design**: only the four fix classes in + [`docs/design-documents/20260712-repair-command.md`](../design-documents/20260712-repair-command.md) + §6 carry a machine-appliable fix today (a deprecated/renamed field, an unambiguous invalid + `status`, a negative processor `workers`, an over-long `description`); every other finding still + keeps its human `suggestion` with no `fix`. `repair`/`repair_apply` also only operate on a single + file containing exactly one v2-format pipeline document — the same scope deploy/apply already + impose. +- **`llms.txt`/`llms-full.txt` generation**: `cmd/conduit/internal/llmsgen` (`go generate ./...`) + now generates both from source — config schema, connector list, the error registry (via AST + scan of every `conduiterr.Register` call, including `repair`'s new codes), and this tool catalog + (`mcp.Catalog()`, including `repair`/`repair_apply`) — with a CI drift guard. This is no longer a + gap. - **North-star E2E** (a scripted agent going zero → running pipeline using only MCP + `llms.txt`): - deferred pending the `llms.txt` generator. The live-server RPC path (#2588) has landed — `apply` + the `llms.txt` generator has landed; the live-server RPC path (#2588) has also landed — `apply` now reaches a genuinely running pipeline when a server is reachable, gated by the server's `--api.allow-live-restart-apply` operator flag (see - [`docs/operations/live-restart-apply.md`](live-restart-apply.md)) for restart-class changes — so - what remains for the north-star E2E is the `llms.txt` generator itself, not the apply-to-running - capability. + [`docs/operations/live-restart-apply.md`](live-restart-apply.md)) for restart-class changes. A + scripted end-to-end run proving this has not been executed as part of this change. diff --git a/llms-full.txt b/llms-full.txt index 20d125bc2..f9f9b1587 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -614,7 +614,7 @@ Author: Meroxa, Inc. | `sdk.schema.extract.key.enabled` | bool | true | no | Whether to extract and decode the record key with a schema. | | | `sdk.schema.extract.payload.enabled` | bool | true | no | Whether to extract and decode the record payload with a schema. | | -## 3. Error codes (40) +## 3. Error codes (46) | Reason | gRPC status | Description | | --- | --- | --- | @@ -622,6 +622,7 @@ Author: Meroxa, Inc. | `common.not_found` | NotFound | CodeNotFound is a generic not-found error. | | `common.unavailable` | Unavailable | common: unavailable | | `config.field_invalid` | InvalidArgument | config: field invalid | +| `config.field_renamed` | InvalidArgument | CodeFieldRenamed is the stable code carried by a lint warning for a deprecated field that was mechanically renamed (the old key's value moves unchanged to a new key) — see pkg/provisioning/config/yaml/internal.Change.RenamedTo and (*configLinter).newWarning. It is distinct from the generic validate.CodeLintWarning so cmd/conduit/internal/repair's fix-producer scan (design doc 20260712-repair-command.md §6, item #1) can find exactly these findings without pattern-matching warning text. | | `config.field_required` | InvalidArgument | config: field required | | `config.field_too_long` | InvalidArgument | config: field too long | | `config.id_duplicate` | InvalidArgument | config: id duplicate | @@ -651,6 +652,11 @@ Author: Meroxa, Inc. | `provisioning.pipeline_running` | FailedPrecondition | provisioning: pipeline running | | `provisioning.plan_stale` | FailedPrecondition | provisioning: plan stale | | `provisioning.standalone_unsafe` | FailedPrecondition | provisioning: standalone unsafe | +| `repair.ambiguous_fix` | FailedPrecondition | CodeAmbiguousFix is raised when more than one candidate fix targets the same ConfigPath and the selection does not disambiguate — repair never silently picks one (design doc §9, failure mode 3, AC-20). | +| `repair.data_path_fix_refused` | FailedPrecondition | CodeDataPathFixRefused is raised when a FixClassDataPath fix is selected for apply without the human-only Escalate path (design doc §4.2, the Tier-1 crux). The CLI surfaces this as a hard refusal (AC-14); MCP repair_apply — which never sets Escalate — surfaces it as a per-fix skip, never a fatal call (AC-15). | +| `repair.fix_no_longer_applies` | FailedPrecondition | CodeFixNoLongerApplies is raised per-fix (not for the whole Apply call) when a selected fix's target field no longer matches what the fix was computed against — either hand-edited between Collect and Apply in a way the hash's per-fix granularity didn't already catch, or consumed by an earlier fix in the same Apply batch (design doc §9, failure mode 1 and AC-19). Other selected fixes in the same call still apply; this fix is skipped, not fatal to the run. | +| `repair.no_fixes_available` | InvalidArgument | CodeNoFixesAvailable is raised when --apply (or repair_apply) is requested but the plan contains zero appliable fixes. A read-mode call with zero fixes is exit 0 ("already clean"), never this code (AC-21). | +| `repair.plan_stale` | FailedPrecondition | CodePlanStale is raised by Apply when the caller-presented hash does not match the hash freshly recomputed from the current file bytes — the direct analogue of provisioning.CodePlanStale (see plan.go). The file changed since the plan was shown and approved. | | `scaffold.build_failed` | Internal | CodeBuildFailed is raised when the verified `go build ./...` step fails. Maps to gRPC Internal -> exitcode.Runtime (1), per this package's explicit exit-code routing ("verified go build failure -> 1"). | | `scaffold.destination_exists` | AlreadyExists | CodeDestinationExists is raised when the target directory already exists and --force was not given. Maps to gRPC AlreadyExists -> exitcode.Validation (2). | | `scaffold.generate_failed` | Internal | CodeGenerateFailed is raised when installing the code-gen tool or running it fails after files are already on disk (in the temp staging directory — see scaffold.go's atomic-rename discipline, so this never leaves a partial directory at the requested destination). Maps to gRPC Internal -> exitcode.Runtime (1). | @@ -659,7 +665,7 @@ Author: Meroxa, Inc. | `scaffold.unsupported_language` | InvalidArgument | CodeUnsupportedLanguage is raised for --lang values other than "go" — today that's every value, since Go is the only ready target (see the design doc's feasibility verdict; Python is blocked on a connector SDK that does not exist yet). Maps to gRPC InvalidArgument -> exitcode.Validation (2). | | `scaffold.write_failed` | Internal | CodeWriteFailed is raised for internal I/O failures unpacking the embedded template snapshot into the staging directory, or performing the atomic rename into place. These are not user-fixable the way a bad flag or a missing toolchain is; they indicate something is wrong with the local filesystem itself. Maps to gRPC Internal -> exitcode.Runtime (1). | -## 4. MCP tool catalog (11) +## 4. MCP tool catalog (13) | Name | Class | Description | | --- | --- | --- | @@ -669,6 +675,8 @@ Author: Meroxa, Inc. | `dry_run` | read | Everything validate checks, plus the fully-enriched pipeline graph `conduit run` would load and a builtin-plugin existence check. Same engine as `conduit pipelines dry-run`. | | `inspect` | read | Reports a running pipeline's live status, per-stage connector summary, and DLQ. Requires a running Conduit (conduit mcp --api-address). Same engine as `conduit pipelines inspect`. | | `lint` | read | Everything validate checks, plus the parser's advisory warnings (deprecated/renamed/unknown fields, version fallback). Same engine as `conduit pipelines lint`. | +| `repair` | read | Scans a pipeline configuration for findings that carry a structured, machine-appliable fix (a deprecated/renamed field, an unambiguous invalid status value, a negative processor workers count, an over-long description), classifies each fix's safety (safe / restart / data_path), and returns a plan hash. Mutates nothing. Same engine as `conduit pipelines repair`. | +| `repair_apply` | write (requires --allow-mutations) | Applies the plan computed by repair — safe fixes only, unless select names others — only if hash still matches the freshly recomputed plan (a stale hash is refused, nothing mutated). A data-path-adjacent fix (connector settings, a connector's own plugin/type, DLQ config, any id field) is NEVER applied by this tool, even if selected explicitly — it comes back in the result as a skipped fix; only the CLI's --escalate flag (human-only) can apply one. Same engine as `conduit pipelines repair --apply`. | | `scaffold_connector` | write (requires --allow-mutations) | Scaffolds a new Go connector plugin repository (source and/or destination). Same engine as `conduit connector new`. | | `scaffold_processor` | write (requires --allow-mutations) | Scaffolds a new Go (WASM) processor plugin repository. Same engine as `conduit processor new`. | | `start` | write (requires --allow-mutations) | Starts a pipeline registered in a running Conduit (transitions to Running). Requires --api-address, like inspect; no offline fallback. Refused if the pipeline is already running (pipeline.running). Same engine as `conduit pipelines start`. | diff --git a/llms.txt b/llms.txt index 30a148eab..8084b2087 100644 --- a/llms.txt +++ b/llms.txt @@ -18,12 +18,12 @@ (6 built-in connectors; full parameters in llms-full.txt) ## Error codes -- 40 stable error codes, dotted `domain.name` reasons, each with a gRPC status class. Full list with descriptions in llms-full.txt. +- 46 stable error codes, dotted `domain.name` reasons, each with a gRPC status class. Full list with descriptions in llms-full.txt. ## MCP tools -- Read: deploy, doctor, dry_run, inspect, lint, validate -- Write (require --allow-mutations): apply, scaffold_connector, scaffold_processor, start, stop -(11 tools; full schemas in llms-full.txt) +- Read: deploy, doctor, dry_run, inspect, lint, repair, validate +- Write (require --allow-mutations): apply, repair_apply, scaffold_connector, scaffold_processor, start, stop +(13 tools; full schemas in llms-full.txt) ## Full detail - [llms-full.txt](llms-full.txt) diff --git a/pkg/provisioning/config/codes.go b/pkg/provisioning/config/codes.go index 197a185ae..1cba926dc 100644 --- a/pkg/provisioning/config/codes.go +++ b/pkg/provisioning/config/codes.go @@ -27,6 +27,15 @@ var ( CodeFieldInvalid = conduiterr.Register("config.field_invalid", codes.InvalidArgument) CodeFieldTooLong = conduiterr.Register("config.field_too_long", codes.InvalidArgument) CodeIDDuplicate = conduiterr.Register("config.id_duplicate", codes.InvalidArgument) + // CodeFieldRenamed is the stable code carried by a lint warning for a + // deprecated field that was mechanically renamed (the old key's value + // moves unchanged to a new key) — see + // pkg/provisioning/config/yaml/internal.Change.RenamedTo and + // (*configLinter).newWarning. It is distinct from the generic + // validate.CodeLintWarning so cmd/conduit/internal/repair's fix-producer + // scan (design doc 20260712-repair-command.md §6, item #1) can find + // exactly these findings without pattern-matching warning text. + CodeFieldRenamed = conduiterr.Register("config.field_renamed", codes.InvalidArgument) // CodeParseError is raised when a pipeline config document can't be parsed // at all (invalid YAML, unrecognized version). It exists for offline // consumers (cmd/conduit/internal/validate) that need a stable code for @@ -45,3 +54,20 @@ func fieldError(code conduiterr.Code, configPath, msg, suggestion string, sentin e.Suggestion = suggestion return e } + +// fieldErrorWithFix is fieldError plus a structured, machine-appliable +// conduiterr.Fix — the repair v1 starter set's producer sites (design doc +// 20260712-repair-command.md §6, items #2-#4: /status, negative +// processor/workers, over-long /description) use this instead of fieldError +// so cmd/conduit/internal/repair can offer the fix without any additional +// per-site wiring. fix.ConfigPath is always set to configPath by this +// helper (callers pass a zero-value fix.ConfigPath) so the two can never +// drift. +func fieldErrorWithFix(code conduiterr.Code, configPath, msg, suggestion string, sentinel error, fix conduiterr.Fix) error { + e := conduiterr.Wrap(code, msg, sentinel) + e.ConfigPath = configPath + e.Suggestion = suggestion + fix.ConfigPath = configPath + e.Fix = &fix + return e +} diff --git a/pkg/provisioning/config/validate.go b/pkg/provisioning/config/validate.go index d776729bd..068cf8e2d 100644 --- a/pkg/provisioning/config/validate.go +++ b/pkg/provisioning/config/validate.go @@ -16,9 +16,11 @@ package config import ( "fmt" + "strings" "github.com/conduitio/conduit/pkg/connector" "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" "github.com/conduitio/conduit/pkg/pipeline" ) @@ -29,6 +31,11 @@ const ( TypeDestination = "destination" ) +// fixOpSet is conduiterr.Fix.Op's "set" value — the only op this package's +// v1 repair fix producers ever emit (§6 items #2-#4; item #1's rename is +// produced by the yaml package's linter, not here). +const fixOpSet = "set" + // Validate validates config field values for a pipeline. Each error carries a // ConduitError code, a JSON-pointer configPath to the offending field, and a // Suggestion for how to fix it; the original sentinel stays in the chain for @@ -48,12 +55,16 @@ func Validate(cfg Pipeline) error { fmt.Sprintf("shorten \"name\" to at most %d characters", pipeline.NameLengthLimit), pipeline.ErrNameOverLimit)) } if len(cfg.Description) > pipeline.DescriptionLengthLimit { - errs = append(errs, fieldError(CodeFieldTooLong, "/description", pipeline.ErrDescriptionOverLimit.Error(), - fmt.Sprintf("shorten \"description\" to at most %d characters", pipeline.DescriptionLengthLimit), pipeline.ErrDescriptionOverLimit)) + // repair v1 starter set item #4 (design doc §6): truncation is lossy + // but deterministic and non-data-path, so it is a machine-appliable + // fix — always shown in the repair diff before apply, never silent + // (design doc's failure mode 6). + errs = append(errs, fieldErrorWithFix(CodeFieldTooLong, "/description", pipeline.ErrDescriptionOverLimit.Error(), + fmt.Sprintf("shorten \"description\" to at most %d characters", pipeline.DescriptionLengthLimit), pipeline.ErrDescriptionOverLimit, + conduiterr.Fix{Op: fixOpSet, Value: cfg.Description[:pipeline.DescriptionLengthLimit]})) } if cfg.Status != StatusRunning && cfg.Status != StatusStopped { - errs = append(errs, fieldError(CodeFieldInvalid, "/status", `"status" is invalid`, - fmt.Sprintf("set \"status\" to %q or %q", StatusRunning, StatusStopped), ErrInvalidField)) + errs = append(errs, statusFieldError(cfg.Status)) } errs = append(errs, validateConnectors(cfg.Connectors)...) @@ -62,6 +73,25 @@ func Validate(cfg Pipeline) error { return cerrors.Join(errs...) } +// statusFieldError builds the /status config.field_invalid error. repair v1 +// starter set item #2 (design doc §6): the fix is offered only when the +// invalid value unambiguously maps to a canonical enum member after +// case/whitespace normalization (e.g. " Running " -> "running") — any other +// value is ambiguous (there is no deterministic canonical target), so no Fix +// is attached and repair reports "no machine-appliable fix for this +// finding", exactly as the design doc requires. +func statusFieldError(status string) error { + msg := `"status" is invalid` + suggestion := fmt.Sprintf("set \"status\" to %q or %q", StatusRunning, StatusStopped) + + normalized := strings.ToLower(strings.TrimSpace(status)) + if normalized == StatusRunning || normalized == StatusStopped { + return fieldErrorWithFix(CodeFieldInvalid, "/status", msg, suggestion, ErrInvalidField, + conduiterr.Fix{Op: fixOpSet, Value: normalized}) + } + return fieldError(CodeFieldInvalid, "/status", msg, suggestion, ErrInvalidField) +} + // validateConnectors validates config field values for connectors. pathPrefix is // the JSON-pointer to the connectors slice ("/connectors"). func validateConnectors(mp []Connector) []error { @@ -123,8 +153,16 @@ func validateProcessors(mp []Processor, pathPrefix string) []error { fmt.Sprintf("set %s/%d.plugin (e.g. \"js\")", pathPrefix, i), ErrMandatoryField)) } if cfg.Workers < 0 { - errs = append(errs, fieldError(CodeFieldInvalid, path+"/workers", fmt.Sprintf(`processor %q: "workers" can't be negative`, cfg.ID), - fmt.Sprintf("set %s/%d.workers to zero or a positive number", pathPrefix, i), ErrInvalidField)) + // repair v1 starter set item #3 (design doc §6): 1 is the + // ordering-preserving default (workers>1 can reorder records + // within a key, invariant 4) — the safe direction to fix + // toward. Classified `restart` by the repair engine's + // classifier (not `safe`), since changing a processor's worker + // count would be an EffectRestart change if deployed to a + // running pipeline. + errs = append(errs, fieldErrorWithFix(CodeFieldInvalid, path+"/workers", fmt.Sprintf(`processor %q: "workers" can't be negative`, cfg.ID), + fmt.Sprintf("set %s/%d.workers to zero or a positive number", pathPrefix, i), ErrInvalidField, + conduiterr.Fix{Op: fixOpSet, Value: "1"})) } if ids[cfg.ID] { errs = append(errs, fieldError(CodeIDDuplicate, path+"/id", fmt.Sprintf(`processor %q: "id" must be unique`, cfg.ID), diff --git a/pkg/provisioning/config/validate_test.go b/pkg/provisioning/config/validate_test.go index 1ec3b14ba..228f9dee2 100644 --- a/pkg/provisioning/config/validate_test.go +++ b/pkg/provisioning/config/validate_test.go @@ -430,3 +430,90 @@ func TestValidator_DuplicateID(t *testing.T) { is.True(err != nil) is.True(cerrors.Is(err, ErrDuplicateID)) } + +// TestValidate_RepairFixes_GoldenPerClass is AC-1 for the repair v1 starter +// set's producers that live in this package (design doc +// 20260712-repair-command.md §6, items #2-#4 — item #1 is the yaml +// package's linter, tested there): for each error class, the producing +// site emits a non-nil Fix with a correct ConfigPath, a valid Op, and a +// Value that, applied, clears the finding. +func TestValidate_RepairFixes_GoldenPerClass(t *testing.T) { + is := is.New(t) + + t.Run("status: unambiguous normalization gets a fix", func(t *testing.T) { + cfg := Pipeline{ID: "p1", Status: " Running "} + err := Validate(cfg) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.True(ce.Fix != nil) + is.Equal(ce.Fix.ConfigPath, "/status") + is.Equal(ce.Fix.Op, "set") + is.Equal(ce.Fix.Value, "running") + + // Applying it clears the finding. + fixed := cfg + fixed.Status = ce.Fix.Value + is.True(Validate(fixed) == nil || !cerrors.Is(Validate(fixed), ErrInvalidField)) + }) + + t.Run("status: ambiguous value gets no fix", func(t *testing.T) { + cfg := Pipeline{ID: "p1", Status: "bogus"} + err := Validate(cfg) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.True(ce.Fix == nil) // no deterministic canonical target — honestly no fix + }) + + t.Run("processor workers: negative gets a fix to 1", func(t *testing.T) { + cfg := Pipeline{ID: "p1", Status: StatusRunning, Processors: []Processor{ + {ID: "proc1", Plugin: "js", Workers: -5}, + }} + err := Validate(cfg) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.True(ce.Fix != nil) + is.Equal(ce.Fix.ConfigPath, "/processors/0/workers") + is.Equal(ce.Fix.Op, "set") + is.Equal(ce.Fix.Value, "1") + + fixed := cfg + fixed.Processors[0].Workers = 1 + is.True(!cerrors.Is(Validate(fixed), ErrInvalidField)) + }) + + t.Run("description: over-long gets a truncation fix", func(t *testing.T) { + long := strings.Repeat("d", pipeline.DescriptionLengthLimit+50) + cfg := Pipeline{ID: "p1", Status: StatusRunning, Description: long} + err := Validate(cfg) + ce, ok := conduiterr.Get(err) + is.True(ok) + is.True(ce.Fix != nil) + is.Equal(ce.Fix.ConfigPath, "/description") + is.Equal(ce.Fix.Op, "set") + is.Equal(len(ce.Fix.Value), pipeline.DescriptionLengthLimit) + is.Equal(ce.Fix.Value, long[:pipeline.DescriptionLengthLimit]) + + fixed := cfg + fixed.Description = ce.Fix.Value + is.True(len(fixed.Description) <= pipeline.DescriptionLengthLimit) + }) +} + +// TestValidate_RepairFixes_SurviveGRPCStatusRoundTrip is AC-2: a real +// producer's Fix survives ConduitError -> gRPC status.Status -> ConduitError +// intact — extending conduiterr's own TestRoundTrip_AllFields (which uses a +// synthetic Fix) to an actual producer in this codebase. +func TestValidate_RepairFixes_SurviveGRPCStatusRoundTrip(t *testing.T) { + is := is.New(t) + + cfg := Pipeline{ID: "p1", Status: " Running "} + err := Validate(cfg) + orig, ok := conduiterr.Get(err) + is.True(ok) + is.True(orig.Fix != nil) + + got := conduiterr.FromStatus(conduiterr.ToStatus(orig)) + is.Equal(got.Fix, orig.Fix) + is.Equal(got.Code.Reason(), orig.Code.Reason()) + is.Equal(got.ConfigPath, orig.ConfigPath) +} diff --git a/pkg/provisioning/config/yaml/internal/changelog.go b/pkg/provisioning/config/yaml/internal/changelog.go index eefec6b3b..45fadf6b7 100644 --- a/pkg/provisioning/config/yaml/internal/changelog.go +++ b/pkg/provisioning/config/yaml/internal/changelog.go @@ -34,6 +34,18 @@ type Change struct { // message is the log message that will be printed if a file is detected // that uses this field with an unsupported version. Message string + // RenamedTo is the new field's key name, set only when this + // FieldDeprecated change is a pure rename (the old key's value moves, + // unchanged, to a new key) rather than a removal with no replacement. + // It is the repair fix-synthesis producer's signal (see + // cmd/conduit/internal/repair's doc and + // pkg/provisioning/config/yaml/linter.go's newWarning): a warning for a + // change with a non-empty RenamedTo carries a machine-appliable + // conduiterr.Fix; a plain deprecation (RenamedTo == "") never does, + // since there is no deterministic replacement value to write (design + // doc docs/design-documents/20260712-repair-command.md §6, "Selection + // rule: a fix ships in v1 only if its Value is deterministic"). + RenamedTo string } // ChangeType defines the type of the change introduced in a specific version. diff --git a/pkg/provisioning/config/yaml/linter.go b/pkg/provisioning/config/yaml/linter.go index e9a82b59a..569179990 100644 --- a/pkg/provisioning/config/yaml/linter.go +++ b/pkg/provisioning/config/yaml/linter.go @@ -17,9 +17,12 @@ package yaml import ( "context" "sort" + "strings" "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" "github.com/conduitio/conduit/pkg/foundation/log" + "github.com/conduitio/conduit/pkg/provisioning/config" "github.com/conduitio/conduit/pkg/provisioning/config/yaml/internal" "github.com/conduitio/yaml/v3" ) @@ -60,7 +63,7 @@ func (cl *configLinter) DecoderHook(version string, warn *warnings) yaml.Decoder func (cl *configLinter) InspectNode(version string, path []string, node *yaml.Node) (warning, bool) { if c, ok := cl.findChange(version, path); ok { - return cl.newWarning(path[len(path)-1], node, c.Message), true + return cl.newWarning(path, node, c), true } return warning{}, false } @@ -90,14 +93,42 @@ func (cl *configLinter) findChange(version string, path []string) (internal.Chan return internal.Change{}, false } -func (cl *configLinter) newWarning(field string, node *yaml.Node, message string) warning { - return warning{ - field: field, +// newWarning builds the warning for a changelog match at path (the full, +// document-rooted traversal path the decoder hook reports — real slice +// indices, not wildcards; see decode.go's triggerHook) against node (the +// field's current value node). +// +// When c is a pure rename (RenamedTo != "", the repair v1 starter set's +// item #1 — see internal.Change.RenamedTo's doc), the warning additionally +// carries a machine-appliable conduiterr.Fix and the dedicated +// config.CodeFieldRenamed code, so cmd/conduit/internal/repair can offer it +// as a fix. The Fix is a deliberate, documented special case of the +// {ConfigPath,Op,Value} shape (design doc §3.1's "compound Fix bundle"): +// ConfigPath is the OLD field's full JSON pointer (path, joined), Op is +// "remove" (the literal action against that path), and Value carries the +// NEW field's key name rather than a value to set — repair's compound +// registry (keyed on config.CodeFieldRenamed) reads Value as "the sibling +// key to add", not as an add/set payload, and copies the old node's current +// value into it. This keeps the wire Fix within the frozen three-op model +// without inventing a fourth "rename" op or a []Fix field (see the design +// doc's alternatives-considered §11). +func (cl *configLinter) newWarning(path []string, node *yaml.Node, c internal.Change) warning { + w := warning{ + field: path[len(path)-1], line: node.Line, column: node.Column, value: node.Value, - message: message, + message: c.Message, + } + if c.RenamedTo != "" { + w.code = config.CodeFieldRenamed.Reason() + w.fix = &conduiterr.Fix{ + ConfigPath: "/" + strings.Join(path, "/"), + Op: "remove", + Value: c.RenamedTo, + } } + return w } type warnings []warning @@ -121,6 +152,11 @@ type warning struct { column int value string message string + // code and fix are set only for a rename-class warning (see newWarning); + // every other warning leaves both zero, exactly as before this field was + // added. + code string + fix *conduiterr.Fix } func (w warning) Log(ctx context.Context, logger log.CtxLogger) { diff --git a/pkg/provisioning/config/yaml/parse_warnings_test.go b/pkg/provisioning/config/yaml/parse_warnings_test.go index fc0f2a710..a66059c4d 100644 --- a/pkg/provisioning/config/yaml/parse_warnings_test.go +++ b/pkg/provisioning/config/yaml/parse_warnings_test.go @@ -20,8 +20,10 @@ import ( "os" "reflect" "sort" + "strings" "testing" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" "github.com/conduitio/conduit/pkg/foundation/log" "github.com/conduitio/conduit/pkg/provisioning/config" "github.com/matryer/is" @@ -143,3 +145,75 @@ func TestParser_WarningsExposure_DoesNotChangeRunBehavior(t *testing.T) { is.Equal(warnings[0].Column, 5) is.Equal(warnings[0].Field, "unknownField") } + +// TestParseWithWarnings_RenamedField_CarriesFix is AC-1's golden test for +// the repair v1 starter set's item #1 (design doc +// 20260712-repair-command.md §6): a processor's deprecated "type" field +// (2.2's rename to "plugin") produces a warning carrying +// config.CodeFieldRenamed and a Fix whose ConfigPath is the full, +// document-rooted JSON pointer to the old field (real slice indices, not +// wildcards) and whose Value is the new field's key name. +func TestParseWithWarnings_RenamedField_CarriesFix(t *testing.T) { + is := is.New(t) + + const src = `version: "2.2" +pipelines: + - id: p1 + status: running + processors: + - id: proc1 + type: base64.encode +` + _, warns, err := NewParser(log.Nop()).ParseWithWarnings(context.Background(), strings.NewReader(src)) + is.NoErr(err) + + var found *Warning + for i := range warns { + if warns[i].Code == config.CodeFieldRenamed.Reason() { + found = &warns[i] + } + } + is.True(found != nil) + is.True(found.Fix != nil) + is.Equal(found.Fix.ConfigPath, "/pipelines/0/processors/0/type") + is.Equal(found.Fix.Op, "remove") + is.Equal(found.Fix.Value, "plugin") +} + +// TestParseWithWarnings_RenamedField_SurvivesGRPCStatusRoundTrip is AC-2 for +// item #1's producer: the rename Fix survives ConduitError -> gRPC +// status.Status -> ConduitError, via the same conversion validate/lint +// findings use for a coded error (findingFromError in +// cmd/conduit/internal/validate wraps a Warning's Code/Fix into exactly +// this shape when Code is set). +func TestParseWithWarnings_RenamedField_SurvivesGRPCStatusRoundTrip(t *testing.T) { + is := is.New(t) + + const src = `version: "2.2" +pipelines: + - id: p1 + status: running + processors: + - id: proc1 + type: base64.encode +` + _, warns, err := NewParser(log.Nop()).ParseWithWarnings(context.Background(), strings.NewReader(src)) + is.NoErr(err) + + var w Warning + for _, ww := range warns { + if ww.Code == config.CodeFieldRenamed.Reason() { + w = ww + } + } + is.True(w.Fix != nil) + + code, ok := conduiterr.LookupCode(w.Code) + is.True(ok) + orig := conduiterr.New(code, w.Message) + orig.Fix = w.Fix + + got := conduiterr.FromStatus(conduiterr.ToStatus(orig)) + is.Equal(got.Fix, orig.Fix) + is.Equal(got.Code.Reason(), orig.Code.Reason()) +} diff --git a/pkg/provisioning/config/yaml/parser.go b/pkg/provisioning/config/yaml/parser.go index e16bbbed4..59f42d893 100644 --- a/pkg/provisioning/config/yaml/parser.go +++ b/pkg/provisioning/config/yaml/parser.go @@ -22,6 +22,7 @@ import ( "strings" "github.com/conduitio/conduit/pkg/foundation/cerrors" + "github.com/conduitio/conduit/pkg/foundation/cerrors/conduiterr" "github.com/conduitio/conduit/pkg/foundation/log" "github.com/conduitio/conduit/pkg/provisioning/config" "github.com/conduitio/conduit/pkg/provisioning/config/yaml/internal" @@ -88,6 +89,15 @@ type Warning struct { Column int Value string Message string + // Code is the warning's stable conduiterr code reason, set only for a + // rename-class warning (config.CodeFieldRenamed) — every other warning + // leaves this empty, and cmd/conduit/internal/validate.warningFinding + // falls back to its own generic CodeLintWarning in that case. + Code string + // Fix is a structured, machine-appliable fix for this warning, set only + // alongside a non-empty Code — see (*configLinter).newWarning's doc for + // what ConfigPath/Op/Value mean for a rename. + Fix *conduiterr.Fix } // ParseWithWarnings is Parse plus the advisory warnings the parser would @@ -100,7 +110,7 @@ func (p *Parser) ParseWithWarnings(ctx context.Context, reader io.Reader) ([]con warn = warn.Sort() ws := make([]Warning, len(warn)) for i, w := range warn { - ws[i] = Warning{Field: w.field, Line: w.line, Column: w.column, Value: w.value, Message: w.message} + ws[i] = Warning{Field: w.field, Line: w.line, Column: w.column, Value: w.value, Message: w.message, Code: w.code, Fix: w.fix} } return configs.ToConfig(), ws, err } diff --git a/pkg/provisioning/config/yaml/v2/model.go b/pkg/provisioning/config/yaml/v2/model.go index 696585201..3c80ebe5b 100644 --- a/pkg/provisioning/config/yaml/v2/model.go +++ b/pkg/provisioning/config/yaml/v2/model.go @@ -55,11 +55,17 @@ var Changelog = internal.Changelog{ Field: "pipelines.*.processors.*.type", ChangeType: internal.FieldDeprecated, Message: "please use field 'plugin' (introduced in version 2.2)", + // RenamedTo makes this a repair fix-synthesis candidate (v1 + // starter set item #1, design doc §6): "type" and "plugin" carry + // the exact same value, so the rename is semantics-preserving by + // definition. + RenamedTo: "plugin", }, { Field: "pipelines.*.connectors.*.processors.*.type", ChangeType: internal.FieldDeprecated, Message: "please use field 'plugin' (introduced in version 2.2)", + RenamedTo: "plugin", }, }, }