Skip to content

feat(slm): deployed-vs-source file drift detection (#2834)#3430

Merged
mrveiss merged 3 commits intoDev_new_guifrom
fix/2834-code-sync-drift
Apr 3, 2026
Merged

feat(slm): deployed-vs-source file drift detection (#2834)#3430
mrveiss merged 3 commits intoDev_new_guifrom
fix/2834-code-sync-drift

Conversation

@mrveiss
Copy link
Copy Markdown
Owner

@mrveiss mrveiss commented Apr 3, 2026

Summary

Implements issue #2834 — adds a GET /code-sync/drift endpoint to the SLM backend that compares SHA-256 checksums between code_source and deployed files, detecting manual patches or incomplete Ansible deploys.

  • New services/drift_checker.py: directory walk with checksum comparison, classifies files as modified | source_only | deployed_only
  • New GET /code-sync/drift?component=<name> endpoint returning FileDriftReport
  • DriftedFile / FileDriftReport Pydantic schemas added to models/schemas.py
  • useCodeSync composable: fetchDrift() method + TypeScript types
  • CodeSyncView: "File Drift Check" card with expandable drifted-files table
  • Fix for bug: path traversal in GET /code-sync/drift component query param #3427: component allowlist (ALLOWED_COMPONENTS) prevents path traversal via the query param; HTTP 400 for unknown components

Issues

Closes #2834
Fixes #3427

Test plan

  • GET /code-sync/drift returns FileDriftReport with correct counts when dirs differ
  • GET /code-sync/drift?component=../../etc returns HTTP 400
  • GET /code-sync/drift?component=autobot-slm-frontend returns report for that component
  • UI "File Drift Check" card renders, spinner shows during fetch, table expands on drift detected

🤖 Generated with Claude Code

mrveiss and others added 2 commits March 30, 2026 16:50
- New services/drift_checker.py: SHA-256 checksum comparison between
  code_source and deployed directories, skipping .pyc/__pycache__/venv/.git
- New GET /code-sync/drift endpoint returns FileDriftReport with per-file
  drift status (modified | source_only | deployed_only)
- Added DriftedFile and FileDriftReport Pydantic schemas to models/schemas.py
- useCodeSync composable: fetchDrift() method + FileDriftReport/DriftedFile types
- CodeSyncView: "File Drift Check" card with expandable drifted-files table

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… endpoint (#3427)

- Add ALLOWED_COMPONENTS frozenset in drift_checker.py
- Validate component param against allowlist in get_file_drift(); raise HTTP 400 on mismatch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mrveiss
Copy link
Copy Markdown
Owner Author

mrveiss commented Apr 3, 2026

Code Review — feat(slm): deployed-vs-source file drift detection (#2834)

Reviewer: code-reviewer agent
Files reviewed: services/drift_checker.py, api/code_sync.py, models/schemas.py, src/composables/useCodeSync.ts, src/views/CodeSyncView.vue


Overall Assessment

The feature is well-scoped and solves a real operational problem (detecting manual patches and incomplete Ansible deploys). The service layer is clean, the ALLOWED_COMPONENTS allowlist prevents path traversal, and the async offloading pattern is correct. There are no blocking issues, but several medium and low items need attention before merge.


Issues Found

MEDIUM — Missing finally block in handleCheckDrift leaves isDriftLoading stuck on error

File: autobot-slm-frontend/src/views/CodeSyncView.vue, lines 360-368

// Current — isDriftLoading never resets if fetchDrift throws
async function handleCheckDrift(): Promise<void> {
  isDriftLoading.value = true
  const result = await codeSync.fetchDrift()
  if (result) {
    driftReport.value = result
    showDriftDetails.value = true
  }
  isDriftLoading.value = false  // not reached on exception
}

codeSync.fetchDrift() catches its own errors and returns null, so this is unlikely to throw in practice. However, fetchDrift also sets loading (the shared composable state), so if the caller somehow throws before returning, the button stays disabled indefinitely. Every other async handler in this file uses a try/finally pattern. This should match for consistency and defensive correctness:

async function handleCheckDrift(): Promise<void> {
  isDriftLoading.value = true
  try {
    const result = await codeSync.fetchDrift()
    if (result) {
      driftReport.value = result
      showDriftDetails.value = true
    }
  } finally {
    isDriftLoading.value = false
  }
}

MEDIUM — DriftedFile.status should use Literal instead of bare str

File: autobot-slm-backend/models/schemas.py, line 1566

status: str  # "modified" | "source_only" | "deployed_only"

The comment documents the constraint but Pydantic will accept any string. Use Literal to enforce it at validation time and produce a more useful OpenAPI schema:

from typing import Literal

status: Literal["modified", "source_only", "deployed_only"]

The TypeScript interface in useCodeSync.ts already uses the union type correctly — the Python schema should match.


MEDIUM — get_default_source_dir silently falls back to repo root, masking misconfiguration

File: autobot-slm-backend/services/drift_checker.py, lines 207-222

candidate = Path(source_root) / component
return str(candidate) if candidate.is_dir() else source_root

When the component sub-directory does not exist, this silently falls back to source_root and compares the entire monorepo against the deployed component directory. This will produce a large, misleading drift report (thousands of "source_only" files). A missing component directory is a configuration problem that should be surfaced as an error, not silently masked. Raise FileNotFoundError or ValueError and let the API return a 500 (or a clear 400) with an actionable message.


LOW — SLM_REPO_PATH is read twice via os.environ.get instead of reusing DEFAULT_REPO_PATH

File: autobot-slm-backend/services/drift_checker.py, line 220

git_tracker.py already exports DEFAULT_REPO_PATH = os.environ.get("SLM_REPO_PATH", "/opt/autobot/code_source"). Importing and reusing it eliminates the duplicated constant:

from services.git_tracker import DEFAULT_REPO_PATH

def get_default_source_dir(component: str = "autobot-slm-backend") -> str:
    candidate = Path(DEFAULT_REPO_PATH) / component
    return str(candidate) if candidate.is_dir() else DEFAULT_REPO_PATH

This also means a runtime change to SLM_REPO_PATH is picked up consistently across both services.


LOW — SLM_DEPLOYED_ROOT should be added to Settings in config.py

File: autobot-slm-backend/services/drift_checker.py, line 203

All other environment-sourced paths in the SLM backend are declared in the Settings class in config.py (e.g. SLM_BACKUP_DIR, SLM_DATABASE_URL). SLM_DEPLOYED_ROOT is the only path variable that bypasses the settings object and reads from os.environ directly. Add it to Settings and inject settings rather than calling os.environ.get inside the service function.


LOW — No unit tests for drift_checker.py

File: autobot-slm-backend/services/drift_checker.py (new file, 0 tests)

The co-located test pattern used by this codebase (a2a_card_fetcher_test.py, git_tracker_test.py) expects a drift_checker_test.py alongside the service file, or an entry under tests/services/. The core logic in compute_drift and _collect_checksums is easily unit-testable with tmp_path fixtures (no network, no database). The following cases are missing coverage:

  • Both dirs identical — no drift
  • File modified — checksum mismatch
  • File in source only
  • File in deployed only
  • Source directory does not exist
  • Extension filter excludes .pyc / .so files
  • _SKIP_DIRS are pruned from traversal

Per CLAUDE.md, missing tests are a required discovery issue. Please file one and link it here.


LOW — Frontend hardcodes autobot-slm-backend with no component selector UI

File: autobot-slm-frontend/src/views/CodeSyncView.vue
File: autobot-slm-frontend/src/composables/useCodeSync.ts, line 631

The backend exposes four components via ALLOWED_COMPONENTS but the UI only ever checks the default. A <select> dropdown or radio group exposing the four allowed values would let operators check drift on autobot-backend or autobot-frontend without needing direct API access. This does not need to block the PR — acceptable as a follow-on issue.


LOW — checked_at is a bare str instead of datetime in both schema and frontend

File: autobot-slm-backend/models/schemas.py, line 1577

checked_at: str

build_drift_report produces an ISO 8601 string via datetime.now(timezone.utc).isoformat(). The Pydantic field should be datetime (Pydantic serialises it to ISO 8601 automatically), which enables type-safe comparisons in the future and produces a better OpenAPI doc. The TypeScript interface can remain string since Date objects are not directly usable in templates.


LOW — _collect_checksums catches IOError which is an alias for OSError in Python 3

File: autobot-slm-backend/services/drift_checker.py, line 91

except (OSError, IOError) as exc:

IOError has been an alias of OSError since Python 3.3. Catching both is redundant — use except OSError.


Positives

  • Async offloading via run_in_executor is correct. Blocking I/O is never called from the event loop directly.
  • ALLOWED_COMPONENTS allowlist correctly prevents path traversal from user-supplied component values.
  • _SKIP_DIRS covers all the important noise directories (__pycache__, .git, venv, node_modules, dist, build).
  • Block-reading in _file_checksum prevents memory exhaustion on large files.
  • build_drift_report keeps the dict/Pydantic boundary clean — internal functions work with plain dicts, the API layer constructs the model.
  • TypeScript interface mirrors the Python schema correctly including the null-able checksum fields.
  • Error handling in fetchDrift follows the existing composable pattern.
  • The drift summary UI correctly handles both the clean and drifted states.

Summary

Priority Count Description
MEDIUM 3 finally block missing, Literal type missing, silent fallback masking config error
LOW 5 Duplicated env constant, Settings bypass, no tests, no component selector, checked_at type, redundant IOError

The MEDIUM items should be addressed before merge. The LOW items can be filed as follow-on issues if the author prefers to keep the PR focused.

- Use Literal["modified","source_only","deployed_only"] in DriftedFile.status
- Raise ValueError (surfaced as HTTP 500) in get_default_source_dir when
  component dir is missing, rather than silently falling back to monorepo root
- Wrap get_file_drift source_dir resolution in try/except ValueError → HTTP 500
- Add try/finally to handleCheckDrift in CodeSyncView so isDriftLoading resets
  on error

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

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