feat: Add telemetryDisabled config to .swamp.yaml#364
Conversation
Add telemetryDisabled?: boolean to RepoMarkerData so users can persistently opt out of telemetry via .swamp.yaml instead of passing --no-telemetry on every invocation. The CLI flag still short-circuits before any file I/O; the config check runs after the marker is read but before any telemetry infrastructure is created. Also adds a Telemetry section to README.md documenting the exact payload shape, the redaction policy, and both opt-out methods. Closes #362
a232946 to
772af10
Compare
There was a problem hiding this comment.
Review Summary
This PR adds a telemetryDisabled configuration option to .swamp.yaml for persistent telemetry opt-out, along with documentation. The implementation is clean and follows project conventions.
No blocking issues found.
Analysis
Code Quality ✅
- TypeScript strict mode: No
anytypes used - Named exports used correctly (
isTelemetryDisabledByConfigis properly exported) - AGPLv3 license headers present in modified files
- Function has appropriate JSDoc with
@internaltag for testing exports
Domain-Driven Design ✅
RepoMarkerDatais appropriately a data structure (Value Object pattern) - it represents configuration state without behavior- The
isTelemetryDisabledByConfighelper inmod.tsis a reasonable application service function that checks configuration - No domain logic leakage - the check is at the infrastructure/CLI boundary
Test Coverage ✅
- 4 unit tests for
isTelemetryDisabledByConfig()covering: null marker, field absent, field false, field true - YAML roundtrip test for
telemetryDisabled: trueinrepo_marker_repository_test.ts - Tests follow the project convention of living next to source files
Security ✅
- No security concerns - this is a simple boolean flag for opting out of telemetry
- The check correctly uses strict equality (
=== true) which handles edge cases properly
Documentation ✅
- README clearly documents the telemetry payload shape
- Both opt-out methods documented (per-invocation flag and persistent config)
- Redaction policy is clearly explained
Suggestions (Non-blocking)
-
Consider
telemetry: falseinstead oftelemetryDisabled: true- This is purely stylistic, but positive phrasing (telemetry: true/false) is often easier to reason about than double negatives. Not worth changing at this point. -
Minor: Could add test for invalid values - The strict
=== truecheck handles this, but an explicit test showing thattelemetryDisabled: "yes"ortelemetryDisabled: 1returnsfalsecould document the expected behavior. Low priority since the implementation is already correct.
LGTM! Well-structured feature with good test coverage and documentation.
There was a problem hiding this comment.
Code Review: feat: Add telemetryDisabled config to .swamp.yaml
Summary
This PR adds a telemetryDisabled configuration option to .swamp.yaml to allow users to persistently opt out of telemetry. The implementation is clean, well-tested, and follows the project's conventions.
✅ No Blocking Issues
The PR is ready to merge. All changes adhere to CLAUDE.md requirements and domain-driven design principles.
Code Quality Assessment
TypeScript Strict Mode Compliance: ✅
- No
anytypes used - All types are properly defined
- The
telemetryDisabled?: booleanfield correctly uses optional typing
Named Exports: ✅
isTelemetryDisabledByConfigis a named export as required
License Headers: ✅
- All
.tsfiles have the required AGPLv3 copyright header
Test Coverage: ✅
- 4 unit tests for
isTelemetryDisabledByConfig()covering all edge cases (null marker, field absent, field false, field true) - 1 YAML roundtrip test for persistence of
telemetryDisabled: true - Tests are properly placed next to source files (
mod_test.ts,repo_marker_repository_test.ts)
Domain-Driven Design: ✅
RepoMarkerDatais correctly modeled as a simple data interface (persistence DTO) - appropriate for configuration dataRepoMarkerRepositoryfollows the repository pattern correctly, handling persistence for the swamp configurationisTelemetryDisabledByConfig()is a pure function with no side effects, correctly placed in the CLI layer where it's used
Security: ✅
- No security vulnerabilities introduced
- The feature properly allows users to opt out of telemetry, which is a privacy-positive change
Code Style: ✅
- Code is clean and follows existing patterns
- The
@internal Exported for testingJSDoc annotation is consistent with the existingresolveModelsDirfunction - The strict equality check
=== trueis appropriate for handling potential non-boolean values from YAML parsing
Suggestions (Non-Blocking)
-
Documentation completeness: The README documentation is thorough and clearly explains the telemetry payload and opt-out methods. Consider mentioning that
telemetryDisabled: truepersists across all sessions while--no-telemetryis per-invocation only (though this may already be implied by "Permanently for a repository"). -
Test ordering: The new telemetry roundtrip test is placed between two
createUpgradeMarkertests inrepo_marker_repository_test.ts. This doesn't affect functionality but grouping related tests together (e.g., all roundtrip tests adjacent) could improve readability.
Verification
The PR description states all 1722 tests pass with clean deno check, deno lint, and deno fmt. The implementation is straightforward and low-risk.
LGTM! 👍
Implements the SWAMP_NO_TELEMETRY environment variable mechanism proposed in issue #362. PR #364 added .swamp.yaml telemetryDisabled: true but left out the env var approach, which is the missing piece for CI/UAT environments where editing .swamp.yaml per-run isn't practical. ### Changes src/cli/mod.ts - Added isTelemetryDisabledByEnv() (exported for testing) after isTelemetryDisabledByConfig(), following the same pattern as resolveModelsDir() which already reads SWAMP_MODELS_DIR - Updated runCli() to OR the env var check with the existing flag check: isTelemetryDisabled(args) || isTelemetryDisabledByEnv() src/cli/mod_test.ts - Added 6 tests for isTelemetryDisabledByEnv() covering: not set, "1", "true" (disabled); "0", "false", "" (enabled/explicit opt-in) - Updated import line to include isTelemetryDisabledByEnv README.md - Extended the "Disabling Telemetry" section with the env var usage example and the full priority order ### Precedence (highest to lowest) 1. --no-telemetry CLI flag 2. SWAMP_NO_TELEMETRY env var ← this PR 3. .swamp.yaml telemetryDisabled: true 4. Default (telemetry enabled) The flag and env var checks are evaluated before initTelemetryService() is called, so they short-circuit the config file read entirely. ### Plan vs Implementation The implementation follows the plan exactly with one minor cosmetic difference: deno fmt reflowed the priority sentence in README.md to fit within the line limit. No functional deviation from the plan. ### Verification - deno check — passes - deno lint — passes - deno fmt — passes - deno run test src/cli/mod_test.ts — 15/15 tests pass
Summary
telemetryDisabled?: booleantoRepoMarkerDataso users can persistently opt out of telemetry via.swamp.yamlinstead of passing--no-telemetryon every invocationisTelemetryDisabledByConfig()helper insrc/cli/mod.tsand check it ininitTelemetryService()before creating any telemetry infrastructureCloses #362
Test Plan
telemetryDisabled: trueinrepo_marker_repository_test.tsisTelemetryDisabledByConfig()covering: null marker, field absent, field false, field truedeno check,deno lint,deno fmtclean