fix(sync): ignore keboola.mcp-server-tool and honor manifest ignoredComponents - #691
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #691 — fix(sync): ignore keboola.mcp-server-tool and honor manifest ignoredComponents
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR fixes issue #689 by (1) adding keboola.mcp-server-tool to ALWAYS_IGNORED_COMPONENTS in constants.py, and (2) making the manifest's previously-dead ignoredComponents field live, unioning it with the hardcoded set and threading it through sync pull, sync diff, sync push (which builds on diff), and scope_manifest. It also closes a real "delete-dir-then-push destroys production" trap by filtering the local side of the diff too, and adds a new "ignored" pull action distinct from "removed" for stale-entry cleanup. The change is well-scoped to the sync engine, has strong test coverage (8 service tests + 2 CLI tests, all passing), and make check is fully green on the current PR head (6164 passed, 12 skipped) after a follow-up commit fixed an initial ty typing issue. Verdict: COMMENT — one hand-maintained doc surface (commands/context.py AGENT_CONTEXT) was missed while five sibling doc surfaces were correctly updated.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/context.py — AGENT_CONTEXT sync-pull section not updated for the new "ignored" action / ignoredComponents
kbagent context (the primary reference AI agents load at session start) documents sync pull's output nuances in detail — --force conflict semantics, --theirs, isDisabled round-tripping, renamed-config detection — but says nothing about the always-ignored component set or the new "ignored" pull-action value this PR introduces (services/sync_service.py:1078, commands/sync.py:171). Every other hand-maintained doc surface in the Plugin synchronization map (CLAUDE.md, keboola-expert.md, gotchas.md, sync-workflow.md, commands-reference.md) was correctly updated with a (since vNEXT) tag — this is the one that was missed. An agent parsing --json pull output and seeing action: "ignored" for the first time, or wondering why keboola.mcp-server-tool never shows up in a pulled tree, has no primary-reference answer. Add a short note to the kbagent sync pull entry in commands/context.py mirroring the one already added to commands-reference.md.
[NB-2] PR-description "live e2e round-trip" is not captured as a permanent regression test
The PR description cites a one-off script (~/kbagent/e2e/sync_ignored_components_e2e.py) run against a real project to verify pull-skip / ignore-cleanup / stale-manifest / un-ignore behavior. That script is outside the repo and won't run again on the next release. Since this is a behavior change to existing commands (not a new command), tests/test_e2e.py coverage is not strictly mandated by CONTRIBUTING.md's "every new command" rule, but given the sharp production-delete failure mode this PR closes, a permanent test_e2e.py case (even a light one exercising ignoredComponents end-to-end) would guard against a future regression that the 10 mocked unit tests could miss if a client/API contract shifts. Not a merge blocker given the thorough mock-level coverage.
Nits
[NIT-1]src/keboola_agent_cli/services/sync_service.py:1074-1080— the"ignored" if old_cfg.component_id in ignored_components else "removed"conditional is doing meaningful classification work inline inside a dict literal; a tiny named helper (e.g._classify_stale_entry(component_id, ignored_components) -> str) would read slightly cleaner and give the branch a name, though the accompanying comment already makes the intent clear.
Verification log
gh pr view 691 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,fix(sync):prefix matches the change type (bug fix to existing sync commands), 12 files changed, +579/-14 (initial), +585/-14 after the follow-uptyfix commit ✓git rev-parse --abbrev-ref HEAD→claude/issue-689-pr-7ad2c7, matches<branch>input; working tree confirmed on the PR branch throughout ✓gh pr diff 691→ 865 lines, re-fetched after a follow-up commit landed on the PR mid-review (see below) ✓- 3-layer compliance:
grepfor typer/click/formatter/console.print inservices/, httpx/requests incommands/→ all empty, no layer violations ✓ - Plugin synchronization map walk: no new CLI command in this PR (behavior fix to existing
sync pull/diff/push), sopermissions.pyOPERATION_REGISTRY,server/routers/sync.py(sync has no REST router — filesystem-local per CLAUDE.md), and the SKILL.md decision table are unaffected. Confirmedplugins/kbagent/agents/keboola-expert.md§3 gotcha added (no new command group, so §2 matrix correctly untouched),gotchas.mdnew entry tagged(since vNEXT),commands-reference.mdupdated forsync pull/push/diff,sync-workflow.mdnew "Ignored components" section,CLAUDE.md## All CLI Commandsinline comment updated.commands/context.pyAGENT_CONTEXTNOT updated — see NB-1. - Convention checks (magic numbers, raw
error_code=strings, bareexcept:,print()in prod code, token leaks, newtuple[...]returns): allgreppatterns from the playbook → empty on the added lines ✓ make check— first run (against the PR's first commit152ed5a5only) failed attypecheck: 4tydiagnostics,unresolved-attributeonSyncService._test_clientintests/test_sync_service.py(mypy-style# type: ignore[attr-defined]comments do not suppresstyerrors; CONTRIBUTING.md requires# ty: ignore[rule]). A second commit (7dde1a3a, "test(sync): satisfy ty -- return the mock client from the helper instead of stashing it on SyncService") landed on the PR branch mid-review and fixes exactly this, refactoring_svcinto_svc_with_clientreturningtuple[SyncService, MagicMock]instead of monkeypatching an untyped attribute. Re-ranmake checkagainst the current PR head: exit 0, all gates green —ruff check,ruff format --check,ty check(0 diagnostics besides the pre-existing unrelatedhatchlingwarning),check_version_gates.py,check_command_sync.py,generate_changelog.py --check,check_error_codes.py,check_sentinel_guards.py,check_file_size.py, andpytest tests/ -m "not e2e"→ 6164 passed, 12 skipped. No BLOCKING finding remains formake check.- Test coverage:
tests/test_sync_service.py::TestIssue689IgnoredComponents→ 8 service-layer tests (pull-skip, manifest-honor, ignored-vs-removed classification, force-pull conflict guard, diff exclusion both sides, push no-op, untracked-directory guard);tests/test_sync_cli.py→ 2 CLI-layer tests (human-mode "Dropped ... component now ignored" rendering,_pull_one_linerignored count). Matches the PR description's claimed counts exactly ✓ - Backward compatibility: new pull-action value
"ignored"is additive to thedetails[].actionJSON field;grep'dweb/frontend/srcfor any consumer keying off pull action values → none found (Agents.tsx/Lineage.tsx hits were unrelated "pull" matches). No breaking rename/removal of existing fields.scope_manifest()'s newignored_componentsparameter has a safe default (frozenset()) and its single call site (services/sync_service.py:1304) was updated ✓ - Could not reproduce the live round-trip against a real Keboola project myself (no credentials in this review environment; per project convention, AI agents never handle API tokens) — the PR author states a live e2e round-trip was already run manually; the unit-level mock tests independently exercise the same scenarios and all pass.
Open questions for the author
(none)
- NB-1: document ignored components + the new 'ignored' pull action in commands/context.py AGENT_CONTEXT (sync pull + sync diff entries), the one silent-drift surface the PR missed. - NB-2: add test_sync_ignored_components_round_trip to tests/test_e2e.py (TestE2ESyncWorkflow) so the ignoredComponents round-trip -- including the stale-manifest trap -- is exercised permanently by the nightly E2E run, not just by a one-off out-of-repo script. - NIT-1: name the stale-entry classification branch (stale_action) in SyncService.pull.
|
Addressed all review findings:
|
fd91860 to
d42e5c8
Compare
- NB-1: document ignored components + the new 'ignored' pull action in commands/context.py AGENT_CONTEXT (sync pull + sync diff entries), the one silent-drift surface the PR missed. - NB-2: add test_sync_ignored_components_round_trip to tests/test_e2e.py (TestE2ESyncWorkflow) so the ignoredComponents round-trip -- including the stale-manifest trap -- is exercised permanently by the nightly E2E run, not just by a one-off out-of-repo script. - NIT-1: name the stale-entry classification branch (stale_action) in SyncService.pull.
…omponents (#689) Two related gaps in the GitOps sync engine: - keboola.mcp-server-tool workspace records (empty configs auto-created by the Keboola MCP server, one per project) joined ALWAYS_IGNORED_COMPONENTS next to keboola.sandboxes: same category, managed through separate APIs. - The manifest field ignoredComponents was declared in the schema but read by nothing. It is now live: unioned with ALWAYS_IGNORED_COMPONENTS and honored by sync pull, diff and push (push builds on diff), plus the force-pull conflict guard. Stale-tree cleanup: on the next pull, manifest entries for a newly-ignored component are dropped and their local directories removed, reported with the new pull action "ignored" (distinct from "removed" = deleted on remote). The diff also filters its LOCAL side by the ignored set (manifest entries, untracked-config walk, untracked-row walk), so a stale entry or leftover directory for an ignored component contributes nothing to the changeset -- without that, a stale entry with no remote counterpart classified as "added" and every push CREATEd a duplicate of a live config. Closes #689
…ead of stashing it on SyncService ty does not honor mypy-style type: ignore comments, so the dynamic _test_client attribute failed CI's type-check step. Also corrects the test-class docstring to the verified failure mode: a stale local entry with a filtered remote counterpart classifies as 'added' (duplicate create on every push), not 'deleted'.
Moves the (since vNEXT) tags from three section headings -- the two new ignored-components sections plus the pre-existing serve --config-dir gotcha from #681 -- to the sections' first body line, so the heading anchor slugs stay stable when the placeholder resolves at release.
- NB-1: document ignored components + the new 'ignored' pull action in commands/context.py AGENT_CONTEXT (sync pull + sync diff entries), the one silent-drift surface the PR missed. - NB-2: add test_sync_ignored_components_round_trip to tests/test_e2e.py (TestE2ESyncWorkflow) so the ignoredComponents round-trip -- including the stale-manifest trap -- is exercised permanently by the nightly E2E run, not just by a one-off out-of-repo script. - NIT-1: name the stale-entry classification branch (stale_action) in SyncService.pull.
…ading (same CONTRIBUTING rule as 0af5071)
3f2e62b to
9e35174
Compare
Resolves the #691 (ignoredComponents) overlap: _is_conflict and _detect_force_pull_conflicts moved to _sync_baseline.py by this branch, so main's ignored_components parameter and filter are carried into the moved detect_force_pull_conflicts; _effective_ignored_components stays on SyncService. Doc conflicts combine both sides.
Moves the (since vNEXT, #687) tag from the workspace-workflow.md load-types heading onto the section's first body line, keeping the heading anchor slug stable when the placeholder resolves at release (CONTRIBUTING rule). The gotchas.md portion of this PR was already applied identically by the merged #691, so it lands as a no-op.
Summary
Fixes #689 — both gaps:
keboola.mcp-server-toolis now onALWAYS_IGNORED_COMPONENTS(constants.py), next tokeboola.sandboxes— same category: empty workspace-record configs auto-created by the Keboola MCP server, one per project, managed through separate APIs.sync pullno longer materializes them;diff/pushno longer see them.ignoredComponentsis now live (it was declared in the schema but read by nothing). A newSyncService._effective_ignored_components()helper unions it withALWAYS_IGNORED_COMPONENTS, computed once per operation and threaded into every filtering site: the pull fetch loop, the force-pull conflict guard, the diff remote build,scope_manifest(branch scoping, sync: production diff/push after 'sync pull --branch' flags the whole orphaned main/ tree as added (mass-duplicate risk) #649), and the untracked-config/untracked-row walks.Stale-tree cleanup (the issue's "already-polluted tree" ask)
On the next
sync pull, manifest entries for a newly-ignored component are dropped and their local directories removed, reported with the new pull action"ignored"— deliberately distinct from"removed"(which means the config was genuinely deleted on the remote). Human output renders it as its own "Dropped (N) -- component now ignored" section.One correction to the issue's framing (verified by test)
With the remote side filtered but the local side not, a stale manifest entry does not classify as
DELETED—compute_changesetderives deletes from remote keys. It classifies asaddedwhile keeping its existing config id, so everysync pushwould CREATE a duplicate of a live config. (TheDELETEDthe issue reproduced is the pre-fix behavior where the component was not ignored at all — deleting the dir of a tracked config is a legitimate GitOps delete.) The fix is the same either way: diff filters the local side too, so an ignored component contributes nothing — notadded, notdeleted, notorphaned.Changes
constants.py—keboola.mcp-server-tooladded toALWAYS_IGNORED_COMPONENTSservices/sync_service.py—_effective_ignored_components()+ threading into pull/diff/conflict-guard/untracked walks; pull's stale-entry sweep distinguishesignoredvsremovedsync/branch_scope.py—scope_manifest(..., ignored_components=...)drops ignored entries from every partitioncommands/sync.py— renders theignoredaction (human + one-liner)(since vNEXT)TestIssue689IgnoredComponents(8 service tests, TDD-verified failing pre-fix) + 2 CLI rendering testsNo version bump, no changelog entry (feature PR per the release process).
Verification
make lint/format-check/skill-check/version-check/version-gate-check/loc-check/check-sentinel-guardsall clean~/kbagent/e2e/sync_ignored_components_e2e.py): pull-skip, manifest ignore,"ignored"action cleanup, stale-manifest trap (no delete, no duplicate), un-ignore re-materialization