Skip to content

feat: conduit pipelines repair + MCP repair with fix-synthesis (§3/§2)#2614

Merged
devarismeroxa merged 2 commits into
mainfrom
feat/pipeline-repair
Jul 13, 2026
Merged

feat: conduit pipelines repair + MCP repair with fix-synthesis (§3/§2)#2614
devarismeroxa merged 2 commits into
mainfrom
feat/pipeline-repair

Conversation

@devarismeroxa

Copy link
Copy Markdown
Contributor

Summary

Implements conduit pipelines repair + MCP repair/repair_apply per
docs/design-documents/20260712-repair-command.md.
Advances execution-plan §3 (CLI verb parity) and §2 (agent-native "human isn't
a second-class citizen to the agent").

Risk tier: 2 (feature — connectors/processors/CLI/UI/registry). repair
edits a config file only; it never opens the provisioning store or a
running pipeline (see the new ADR below), so it cannot reach the data path.

The design doc's honest headline, which this PR verifies and closes: the
structured Fix{ConfigPath, Op, Value} field already existed on
ConduitError and already round-tripped over gRPC/JSON — but no error site
populated it
. The load-bearing work here is fix synthesis, not command
wiring: four producer sites now emit a real Fix, a shared engine
classifies and applies it, and the CLI/MCP shells are thin callers of that
one engine.

What shipped

Producers (v1 starter set, design doc §6, items #1-#4 — deliberately not
wider; see "explicitly out of scope" in §6):

# Finding Fix Class
1 Deprecated processor type (v2.2 rename to plugin) compound rename, in-place key mutation preserving comments/position safe
2 /status invalid but case/whitespace-normalizable set to canonical enum safe
3 Negative processor /workers set to 1 restart
4 Over-long /description set to truncated value safe

Everything else keeps its existing human Suggestion and a nil Fix
repair reports "no machine-appliable fix" honestly rather than fabricating
one (matches §6's out-of-scope table: plugin-name/type/ID-duplicate fixes
need closest-match logic or a store-state check that don't exist yet).

Shared engine (cmd/conduit/internal/repair, new package):

  • Collect/CollectContent run the existing validate/lint engine
    (Options{Warnings: true}) and classify every fix.
  • classify is default-deny: connector settings, a connector's own
    plugin/type, DLQ config, any id field → data_path; an
    unrecognized ConfigPath also falls through to data_path — never
    silently safe.
  • Apply edits an in-memory, comment-preserving *yaml.Node tree
    (github.com/conduitio/yaml/v3, already a dependency — no new dep) rather
    than a marshal-from-struct round trip, which would strip comments. Verified
    against a fixture with comments: only the targeted lines change.
  • Re-validates the repaired bytes before returning success (a bad producer
    fails closed, in CI, not in a user's pipeline).
  • repair v1 scope is intentionally narrowed to one file, one v2-format
    pipeline document
    — the same constraint deploy/apply already impose
    (deploy.ParseSinglePipeline). Documented in the package doc.go.

CLI: conduit pipelines repair <file> [--apply] [--plan-hash H] [--fix PATH]... [--escalate] [--json] [--no-color] — diff-first, mirrors
deploy/apply's hash-bound UX exactly. --apply without --fix applies
every safe fix only; a data_path fix named explicitly via --fix without
--escalate refuses the whole run, nothing written. Atomic file write
(temp + rename in the same directory).

MCP: repair (read, always registered) and repair_apply (write, gated
by --allow-mutations), registered through the mcp.Catalog()/tool()
pattern that landed on main after this branch's original base (#2608,
#2613) — I rebased onto latest main mid-implementation to pick this up
rather than hand-rolling sdkmcp.Tool{} literals. repair_apply's input
schema has no escalate field at all — a data_path fix selected via
select always comes back skipped, never applied, and there is no
agent-settable way to force it (asserted directly against the tool's JSON
schema in TestRepairApply_ToolSchema_HasNoEscalateField).

New error codes (registered in the one conduiterr registry):
config.field_renamed, repair.plan_stale, repair.fix_no_longer_applies,
repair.data_path_fix_refused, repair.ambiguous_fix,
repair.no_fixes_available. llms.txt/llms-full.txt regenerated
(go generate) — both the 6 new codes and the 2 new MCP tools appear
automatically via the existing AST-scan/Catalog() generators, zero manual
doc duplication.

New ADR:
20260713-repair-edits-file-not-store.md
records the store-vs-file decision the design doc's §11 said should be
promoted once built.

Two corrections to the task brief, found while implementing

  1. The MCP "Catalog pattern... recently merged" did not exist in the
    commit my worktree started from (bfe093d) — it landed one commit later,
    in #2613 (2966987), alongside the llmsgen generator (#2608). I
    rebased this branch onto latest main mid-implementation (see commit
    history / the two-phase build below) specifically to land on the real
    pattern rather than the manual sdkmcp.Tool{} literals I'd started with.
    TestCatalog_MatchesRegisteredToolSets is green.
  2. llms.txt generation — same story: didn't exist in the base commit,
    exists on latest main. Ran go generate ./...; both new error codes and
    both new MCP tools show up automatically (diffs above), confirming the
    generator's AST-scan/Catalog() approach picked this feature up with zero
    special-casing.

Both are stated here per CLAUDE.md ("do not claim a bar is met when it
structurally isn't" / "say what was actually run") — the discrepancy was
between the task brief and the commit my worktree happened to start from, not
a false claim in the design doc itself.

Acceptance criteria (design doc §8) — status

Met: AC-1 (golden test per producer class — TestCollect_Rename/Status/Workers/Description

  • TestValidate_RepairFixes_GoldenPerClass), AC-2 (Fix survives producer →
    gRPC status → back, extended to real producers —
    TestValidate_RepairFixes_SurviveGRPCStatusRoundTrip,
    TestParseWithWarnings_RenamedField_SurvivesGRPCStatusRoundTrip), AC-3
    (invalid op / uncoercible value aborts the whole call, never written —
    TestApply_InvalidOp_AbortsEntireCall, TestApply_UncoercibleNumericValue_AbortsEntireCall),
    AC-4 (Collect/CollectContent byte-identical —
    TestCollect_CollectContent_Identical), AC-5 (default-deny classifier table
    test), AC-6 (pure recompute+edit — structurally true: cmd/conduit/internal/repair
    imports neither pkg/provisioning nor pkg/pipeline in any non-test file),
    AC-7 (read mode never writes), AC-8 (--apply needs --plan-hash/--yes),
    AC-9 (stale hash refused, file unchanged), AC-10 (re-validation gate —
    TestRevalidate_CatchesAFixThatDidNotClearItsFinding), AC-11 (atomic write,
    crash-mid-write leaves original intact — TestWriteFileAtomic_*), AC-12
    (comments preserved), AC-13 (single JSON object — inherited structurally from
    the shared cecdysis.CommandWithResultDecorator every other command uses),
    AC-14 (data-path fix named via --fix without --escalate refuses the whole
    CLI run), AC-15 (MCP never applies a data_path fix; no escalate field in
    schema), AC-16 (repair_apply absent without --allow-mutations), AC-17
    (default = safe fixes only), AC-18 (no store mutation — structural, see
    AC-6), AC-19 (partial success — one selected fix no-longer-applying doesn't
    block others), AC-20 (ambiguous-fix guard), AC-21 (no_fixes_available on
    empty --apply; clean read is exit 0), AC-22 (codes registered; llms.txt/
    tool catalog regenerated), AC-23 (godoc + package doc.go invariants).

Partially met, honestly: AC-14/AC-15's data_path branch has no
end-to-end fixture, because no v1 producer classifies a fix data_path
(by design — see the out-of-scope table). I exercised the gate logic
(gateFix, firstDataPathRefusal, the classifier itself, and the MCP tool's
schema) directly against synthetic/crafted inputs instead — documented at
each such test (TestGateFix, TestFirstDataPathRefusal,
TestBuildProposedFix_DataPathFix_NeverRendersBeforeAfter). This is a real
coverage gap that will close naturally once a wider fix producer set exists.

Adversarial self-review (per CLAUDE.md)

Found and fixed two real bugs before opening this PR:

  1. Misleading error on unparseable files. collect() originally checked
    "exactly one pipeline" before checking whether the file parsed at all, so
    a genuinely broken YAML file (0 pipelines) reported "repair requires
    exactly one pipeline definition per file (found 0)" instead of the real
    parse error. Fixed to surface the actual parse finding's message/suggestion
    when the pipeline count is zero; regression test:
    TestCollect_UnparseableFile_SurfacesParseError.
  2. Secrets guard was implicit, not enforced. Before/After diff
    rendering was populated unconditionally; it happened to never leak a
    secret only because no current producer targets connector settings.
    Hardened buildProposedFix to skip Before/After entirely for any
    FixClassDataPath fix, closing the gap structurally before a future
    producer could reopen it. Test:
    TestBuildProposedFix_DataPathFix_NeverRendersBeforeAfter.

Other things checked and found sound: no goroutines/shared mutable state in
the engine (race detector clean); every temp-dir path has a defer RemoveAll; WriteFileAtomic creates its temp file in the same directory
as the target (same-filesystem rename guarantee) and is exercised under a
simulated write failure; the rename-vs-remove+add compound-fix
implementation is documented as a deliberate simplification (in-place key
rename, not literal remove-then-add) that preserves field order/comment
attachment better than the design doc's literal framing, with no behavioral
difference. Known, accepted gap: two concurrent repair --apply
invocations against the same file can race (last writer wins) — this is
inherent to editing a plain file with two processes, not a data-path/store
concern pkg/provisioning's pipelineLocks addresses, and is unchanged from
how any other CLI file-editing tool behaves.

Testing

  • go build ./..., go vet ./... — clean (one pre-existing, unrelated go vet warning in pkg/lifecycle-poc/funnel/worker.go, not touched by this
    PR).
  • go test -race -short ./...77 packages, 0 failures (run twice: once
    pre-rebase, once post-rebase on top of #2608/#2613).
  • golangci-lint run (bare, matching make verify) — 0 issues in any file
    this PR touches. 34 pre-existing issues remain in unrelated files (the
    vendored pkg/plugin/processor/builtin/internal/diff package and
    conduiterr.go's constructors) — confirmed present before this PR, out of
    scope here.
  • npx markdownlint-cli2@0.18.1 on the full repo glob make verify uses —
    0 errors.
  • go generate -x ./... + buf generate — stable, zero drift after the
    changes in this PR (the "validate-generated-files" make verify step).
  • New tests: producers (pkg/provisioning/config/validate_test.go,
    pkg/provisioning/config/yaml/parse_warnings_test.go), the engine
    (cmd/conduit/internal/repair/*_test.go — classify, collect, apply, the
    YAML node editor via the apply tests, atomic write), the CLI
    (cmd/conduit/root/pipelines/repair_test.go), MCP
    (cmd/conduit/internal/mcp/tools_repair_test.go), and the catalog
    consistency test inherited from #2613
    (TestCatalog_MatchesRegisteredToolSets, green).

Docs

  • docs/operations/mcp-server.md — tool table + a new repair/repair_apply
    section; "Known limitations" corrected (llms.txt generation is no longer a
    gap; repair's v1 scope documented instead).
  • docs/design-documents/20260712-repair-command.md — status line updated to
    "implemented".
  • New ADR: docs/architecture-decision-records/20260713-repair-edits-file-not-store.md.
  • No committed static CLI-reference markdown exists for deploy/apply
    either (only the auto-generated, non-committed conduit docs output) —
    repair's Docs()/Usage()/Example() follow the same pattern, so it's
    covered the same way those commands are.

Not done / explicitly out of scope

  • Everything in the design doc's §6 "explicitly out of scope" table
    (plugin-name did-you-mean, connector /type, missing required fields, ID
    duplicate) — unchanged, still human-Suggestion-only, nil Fix.
  • A --deploy-after---apply convenience flag — two explicit steps by
    design (see the new ADR's consequences section).
  • A scripted north-star E2E (zero → running pipeline using only MCP +
    llms.txt) — the pieces now all exist on main (this PR, #2608,
    #2588) but the scripted run itself hasn't been executed.

Coverage floor / benchi regression gate / property-fuzz tests are by
Phase 1
per CLAUDE.md's Process Maturity table, not live yet — none
claimed here.

…thesis (§3/§2)

Implements docs/design-documents/20260712-repair-command.md: the honest finding
there is that conduiterr.Fix already round-trips end-to-end but NO error site
populates it, so this PR's real work is fix synthesis, not command wiring.

Producers (v1 starter set, §6, items #1-#4):
  - deprecated processor `type` -> `plugin` rename (pkg/provisioning/config/yaml
    linter, new config.field_renamed code carrying the compound-rename Fix)
  - unambiguous invalid `/status` normalization (pkg/provisioning/config/validate.go)
  - negative processor `/workers` clamp to 1 (same)
  - over-long `/description` truncation (same)

Shared engine (cmd/conduit/internal/repair): Collect/CollectContent run the
existing validate/lint engine, classify every fix default-deny (safe/restart/
data_path) against the ack/position/checkpoint-adjacent invariants, and Apply
edits a comment-preserving *yaml.Node tree (github.com/conduitio/yaml/v3,
already a dependency) rather than a marshal-from-struct round trip. repair
never opens the provisioning store or a running pipeline — see the new ADR
(docs/architecture-decision-records/20260713-repair-edits-file-not-store.md).

CLI: `conduit pipelines repair <file> [--apply] [--plan-hash] [--fix]...
[--escalate] [--json]`, diff-first like deploy/apply, atomic (temp+rename)
file write.

MCP: `repair` (read, always registered) and `repair_apply` (write, gated by
--allow-mutations), registered via the mcp.Catalog()/tool() pattern. There is
deliberately no `escalate` field on repair_apply's schema — a data-path fix is
always refused for auto-apply from MCP; only the CLI's --escalate is a valid
override, keeping the Tier-1 human gate real.

New error codes: config.field_renamed, repair.plan_stale,
repair.fix_no_longer_applies, repair.data_path_fix_refused,
repair.ambiguous_fix, repair.no_fixes_available. llms.txt/llms-full.txt
regenerated (go generate) to include them and the two new MCP tools.

Roadmap: docs/design-documents/20260704-phase-1-execution-plan.md §2, §3.
Risk tier: 2 (feature; repair never reaches the data path by construction).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3cLokwNJYLLBpdyt3cgGr
@devarismeroxa
devarismeroxa requested a review from a team as a code owner July 13, 2026 18:04
… classifier

Resolves the independent review's blocking findings on #2614:

- B1 (BLOCKER, data loss): repair --apply silently dropped trailing YAML
  documents. The guard counted PIPELINES, not DOCUMENTS; parseDoc decoded only
  the first document and marshalDoc re-emitted only that, so a multi-document file
  whose first document holds one pipeline and whose trailing documents contribute
  zero pipelines slipped past every guard, and --apply rewrote the file with
  everything after the first document deleted. parseDoc now rejects multi-document
  files at the document level (a second Decode that isn't io.EOF). Regression test
  added (TestCollect_RejectsMultiDocument) — the suite previously had none.

- M1 (MAJOR, the Tier-1 gate): the data-path classifier's safe/restart allowlist
  matched on the bare final segment name, so a future field ending in
  status/description/type/workers in a non-producer location would be
  auto-classified safe/restart. Anchored the allowlist to the full path SHAPES the
  v1 producers emit (extracted to classifyAllowlisted, keeping classify under the
  cyclomatic limit) so an unrecognized shape falls through to default-deny.
  Over-match negative cases added to the classifier table.

- n1 (honesty): corrected the ADR — repair does import pkg/provisioning/config (a
  pure config/validation package for CodeFieldRenamed); it's the store/service
  packages it does not import.

Verified: go build ./... clean; go test -race green on the repair package;
golangci-lint 0 issues; ADR markdownlint clean; no llms.txt drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3cLokwNJYLLBpdyt3cgGr
@devarismeroxa
devarismeroxa merged commit 599031b into main Jul 13, 2026
6 checks passed
@devarismeroxa
devarismeroxa deleted the feat/pipeline-repair branch July 13, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant