feat(sync): audit plaintext #-secrets in synced configs (0.55.0)

[padak]

Summary

Follow-up to #378 (shipped in v0.54.0). That fix is forward-looking — it encrypts #-secrets on future writes but cannot retroactively encrypt secrets written by older versions, which remain plaintext in Storage. This PR adds a read-only audit that finds them, surfaced exactly where a sync user is already working.

What it does

• sync status scans every in-sync config/row in the working tree (local file hash == manifest pull_hash) and surfaces a plaintext_secret_warnings block (human) / array (--json) listing configs whose #-prefixed values are still plaintext — i.e. the remote holds them unencrypted. Key paths only, never the secret values.
• doctor gains a matching sync_secrets check: warn (with affected configs) inside a sync working tree holding plaintext secrets, pass when clean, skip outside a sync tree.

Design notes

• Pending edits are NOT flagged. A locally-edited-but-unpushed config (hash != pull_hash) is skipped — a sync push on >=0.54.0 encrypts it on write, so flagging it would be noise. Only already-synced plaintext (a real remote leak) is reported.
• Read-only, no API. Filesystem + manifest only. Detection reuses _encryption.collect_secrets via the new find_plaintext_secret_keys helper, so already-KBC:: values are ignored.
• Remediation is in the warning text: re-push on >=0.54.0 to encrypt AND rotate the credential — config version history keeps the old plaintext, so overwriting alone does not close the leak.

Tests

tests/test_sync_plaintext_audit.py (11 tests): helper; scan (in-sync plaintext flagged; encrypted / pending-edit / secret-free not flagged; row-level); sync status integration; doctor skip/warn/pass. Full suite green — make check: 3879 passed, 8 skipped.

Docs / release

Version bump to 0.55.0 + changelog; gotchas.md (tagged since v0.55.0) and commands-reference.md (sync status + doctor). sync status / doctor signatures are unchanged (only the output grew), so the CLAUDE.md command list and keboola-expert.md need no edits.

[padak]

Review of #383 — feat(sync): audit plaintext #-secrets in synced configs (0.55.0)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR adds a read-only audit that surfaces plaintext #-secrets left in synced configs by pre-0.54.0 writes. It extends sync status with a plaintext_secret_warnings field and adds a sync_secrets check to doctor. The design is sound: filesystem-only, no API calls, skip-when-not-in-sync-tree, and the helper correctly returns only key paths — never plaintext values. The implementation is clean, the test file is thorough at the service/helper layer, and make check passes fully (3879 passed, 8 skipped).

Two gaps keep this at REQUEST CHANGES: (1) src/keboola_agent_cli/commands/context.py (AGENT_CONTEXT) is not updated despite the sync status JSON output contract changing — a new plaintext_secret_warnings field that AI agents reading kbagent --json context will not know exists; and (2) the command-layer refactor of sync_status (the if/else restructuring + warning display block) has no CliRunner test — only the service is exercised in the new test file.

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 2
• Non-blocking findings: 3
• Nits: 2

Blocking findings

[B-1] src/keboola_agent_cli/commands/context.py:832 — AGENT_CONTEXT not updated for new plaintext_secret_warnings output

sync status now emits a plaintext_secret_warnings array in --json mode (and a red warning block in human mode). AGENT_CONTEXT at line 832 still says only "Show local changes since last pull (SHA256-based)" — no mention of the new field or its semantics. Per CONTRIBUTING.md Plugin synchronization map: "src/keboola_agent_cli/commands/context.py (AGENT_CONTEXT) — Adding/removing/renaming commands; significant flag changes — NO CI coverage." A new JSON output field that an AI agent consumes is a significant behavior change. Without updating AGENT_CONTEXT, the keboola-expert subagent will not know that sync status --json now carries plaintext_secret_warnings, so it cannot surface or act on the audit results.

Fix: add one sentence under the kbagent sync status entry in context.py describing the plaintext_secret_warnings array and the kbagent doctor sync_secrets check, with a (since v0.55.0) tag.

[B-2] tests/test_sync_plaintext_audit.py — no CliRunner test for the sync status warning display path

The PR adds ~30 lines of new command-layer logic in src/keboola_agent_cli/commands/sync.py (lines 613–169): the if/else restructuring of the no-changes path and the plaintext_secret_warnings display block. TestSyncStatusWarning.test_status_surfaces_plaintext_warnings (line 157) exercises the service via SyncService.status() directly — it does not invoke the CLI via CliRunner. Per CONTRIBUTING.md "Tests (mandatory!)": "CLI-layer tests — use CliRunner, test JSON output, error exit codes." The command layer refactor is therefore untested: specifically, (a) human-mode output with warnings when no other local changes exist, (b) human-mode output with warnings when changes also exist, and (c) --json mode propagation of plaintext_secret_warnings through formatter.output(result). The existing test_sync_status_no_changes mock also does not include plaintext_secret_warnings, so the test does not cover the new result.get("plaintext_secret_warnings", []) guard.

Fix: add two CliRunner tests in TestSyncStatusCli (or test_sync_plaintext_audit.py): one for human mode where the service returns a non-empty plaintext_secret_warnings, asserting the red warning text and remediation line appear; one for --json mode asserting data["plaintext_secret_warnings"] is present and structured correctly.

Non-blocking findings

[NB-1] src/keboola_agent_cli/services/sync_service.py:1129 — scan_synced_plaintext_secrets called without error guard in status()

SyncService.status() calls scan_synced_plaintext_secrets(project_root) at line 1129 with no try/except. The scan function documents that it raises FileNotFoundError if no manifest is found — but since status() already calls load_manifest() at line 1073 (which raises the same error and is caught by the command layer), a second manifest error from the scan is also fine. However, _read_yaml and _in_sync in the scan function each call path.read_bytes() / path.read_text() separately, meaning a file deleted between the _read_yaml call and the _in_sync call (unlikely but possible on an active workspace) could raise FileNotFoundError or PermissionError that propagates unhandled through status() and crashes with exit 1 instead of being surfaced as a warning. The _check_sync_secrets in doctor_service.py wraps the same scan in except Exception (line 132), so doctor is resilient but status is not.

Fix: wrap the scan_synced_plaintext_secrets(project_root) call in status() with try/except Exception: plaintext_secret_warnings = [] (mirroring the doctor pattern), or make scan_synced_plaintext_secrets itself swallow file-race errors.

[NB-2] src/keboola_agent_cli/services/sync_service.py:188 — scan_synced_plaintext_secrets calls load_manifest redundantly

SyncService.status() calls load_manifest(project_root) at line 1073 then passes the results through its own manifest-walking loop. scan_synced_plaintext_secrets(project_root) at line 188 calls load_manifest(project_root) again independently. This means every sync status invocation reads and parses manifest.json twice. It works correctly, but the scan function's signature accepts only project_root, so it cannot receive the already-loaded manifest. For large projects with many configs this is a minor waste; more importantly it means the two manifest reads could theoretically diverge if the manifest is modified between calls (very unlikely, but the right design passes the manifest in).

Fix (NON-BLOCKING — not required for this PR): accept an optional manifest: Manifest | None = None parameter in scan_synced_plaintext_secrets, load only if None. This aligns with the standard service layer pattern in this codebase.

[NB-3] plugins/kbagent/agents/keboola-expert.md:179 — existing config create/update gotcha does not mention sync status/doctor audit

The inline gotcha at line 179 covers the 0.54.0 auto-encrypt fix and includes a VERSION GATE note about pre-0.54.0 plaintext writes. It is the natural place to mention that 0.55.0 provides an audit (sync status + doctor) to find the retroactive damage. Per CONTRIBUTING.md §5 "Manually review keboola-expert.md" §3 Inline Gotchas — "new behavior the agent would get wrong by default". Without this addition, an agent running on 0.55.0 will not know to recommend sync status when a user reports a potential pre-0.54.0 plaintext leak; it will only say "update to 0.54.0+". This is the difference between detecting a past leak and not.

Fix: append one bullet to the existing config create/update/row-* auto-encrypt #-secrets gotcha: "Use sync status or doctor (0.55.0+) to audit synced working trees for any configs that passed through pre-0.54.0 unencrypted." Mark it (0.55.0+).

Nits

• [NIT-1] src/keboola_agent_cli/services/sync_service.py:1071 — docstring for status() says "Dict with lists of modified/added/deleted configs and count of unchanged" but the return dict now also contains plaintext_secret_warnings. Update the docstring to include the new field.

• [NIT-2] src/keboola_agent_cli/commands/sync.py:160 — the warning emoji ⚠ appears in the Rich console output even though the project convention (checked in CLAUDE.md and CONTRIBUTING.md) is to avoid emoji in text output. Using [bold red]WARNING[/bold red] or [bold red]PLAINTEXT SECRETS[/bold red] instead of ⚠ would be consistent with how other warning blocks in this codebase are styled (e.g., the sync pull --force conflict banner uses the word "CONFLICT" without emoji).

Verification log

• gh pr view 383 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → state=OPEN, 12 files, +429/-24, conventional feat(sync): prefix ✓, base=main
• git rev-parse --abbrev-ref HEAD → feat/sync-secret-audit ✓ (working tree matches PR branch)
• Read CONTRIBUTING.md → loaded Plugin synchronization map and checklist ✓
• Read CLAUDE.md → loaded convention #17 silent-drift surfaces ✓
• Read plugins/kbagent/agents/keboola-expert.md → §1 rules and §3 gotchas loaded ✓
• make check → 3879 passed, 8 skipped exit 0 ✓
• Layer violation grep (typer/formatter in services) → formatter.console.print calls are all in commands/sync.py only ✓
• Layer violation grep (httpx in commands) → no results ✓
• grep -n 'sync\.\|doctor\.' permissions.py → "sync.status": "read" present (line 272); "doctor": "read" present (line 294) ✓
• grep -n 'sync status\|kbagent doctor' CLAUDE.md → both listed with unchanged signatures (lines 434, 548) ✓; signatures unchanged so no CLAUDE.md All CLI Commands update needed
• grep -n 'sync status' context.py → shows only "Show local changes since last pull (SHA256-based)" — no mention of new plaintext_secret_warnings field → BLOCKING gap [B-1]
• grep -n 'plaintext_secret_warnings' tests/test_sync_plaintext_audit.py → service-layer test only (line 166); no CliRunner test for command layer → BLOCKING gap [B-2]
• grep -n 'scan_synced_plaintext_secrets' sync_service.py → line 1129 call has no try/except; _check_sync_secrets in doctor does wrap in Exception → NON-BLOCKING [NB-1]
• wc -l services/sync_service.py → 3659 lines (hard ceiling for services is 1500 LOC per CONTRIBUTING.md). This file was already above the hard ceiling before this PR (+91 lines); the CONTRIBUTING.md ceiling concern predates this PR (noted as a known issue). This PR adds 91 lines — the split-first rule applies only "when a file crosses the hard ceiling" and it already was there, so this PR is not incrementally worse. Not raising as a new finding.
• grep 'find_plaintext_secret_keys' _encryption.py → confirms function returns sorted(secrets) (keys only), never secret values ✓
• grep 'kboola-expert.md' config.*create.*0.54 → inline gotcha at line 179 covers auto-encrypt but not the new 0.55.0 audit surface → NON-BLOCKING [NB-3]
• wc -l services/doctor_service.py → 445 lines (well under soft ceiling of 1000) ✓
• grep -c '^ def test_' tests/test_sync_plaintext_audit.py → 11 tests total; coverage is good at helper and service layers; CliRunner gap identified ✓
• No bare except: in new code; no print() in production code; no raw error_code="..." strings; no magic numbers ✓
• No new httpx/requests calls outside client files ✓
• plaintext_secret_warnings field is additive; consumers use result.get("plaintext_secret_warnings", []) defensively → backward compatible with existing CLI tests and external callers ✓
• E2E test coverage: tests/test_e2e.py has no new test for sync status plaintext warning (requires pre-seeded plaintext secret in synced tree). Per CONTRIBUTING.md: "every CLI command must have E2E coverage." This is a NON-BLOCKING gap given environmental constraints — the behavior is filesystem-local and the unit/service coverage is strong.

Open questions for the author

• The PR description states "sync status / doctor signatures are unchanged, so the CLAUDE.md command list and keboola-expert.md need no edits." The CLAUDE.md All CLI Commands section lists signatures only (unchanged ✓), so that decision is correct. But context.py (AGENT_CONTEXT) is a separate file from CLAUDE.md and it describes behavior (not just signatures) — was context.py deliberately excluded, or is it grouped mentally with CLAUDE.md?

(none else)

[padak]

Thanks for the thorough review — all 7 findings addressed in 308a1ab.

Finding	Fix
B-1 context.py AGENT_CONTEXT	Documented plaintext_secret_warnings under sync status + the doctor sync_secrets check (since 0.55.0).
B-2 no CliRunner test for warning display	Added TestSyncStatusCliWarning: human-mode warning block, --json propagation, and the clean (no-warning) case.
NB-1 scan not error-guarded in status()	status() now wraps the scan in try/except (logs + degrades to []), mirroring the doctor guard.
NB-2 redundant load_manifest	scan_synced_plaintext_secrets takes an optional preloaded manifest; status() passes its own — one read per invocation.
NB-3 keboola-expert gotcha	Appended a bullet pointing at sync status / doctor (0.55.0+) for auditing pre-0.54.0 leaks.
NIT-1 stale status() docstring	Now documents plaintext_secret_warnings.
NIT-2 ⚠ emoji	Replaced with PLAINTEXT SECRETS text (no-emoji convention); a CLI test asserts the emoji is absent.

On the open question: context.py was an oversight on my part — I mentally grouped it with the CLAUDE.md "signatures only" call, but you're right that AGENT_CONTEXT describes behavior, so a new JSON field belongs there. Fixed.

make check: 3882 passed, 8 skipped.