fix: silence telemetry network errors in firewalled environments#959
fix: silence telemetry network errors in firewalled environments#959
Conversation
Wrap PostHog fetch with safeTelemetryFetch that catches all network errors and non-2xx responses, returning a synthetic 204 so PostHog never throws PostHogFetchNetworkError. Disable retries, remote config, surveys, and feature flag preloading to eliminate extra network calls. Add 1s request timeout. Surface telemetry opt-out docs earlier in README, installation, and CLI reference. Closes #895
|
Task completed. I'll start by reviewing the PR changes to provide constructive feedback. Powered by 1Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a safe telemetry fetch wrapper with a 1s timeout and silent-failure behavior, configured PostHog to use it with retries and remote features disabled, updated tests to validate the behavior, and documented environment-variable telemetry disablement ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant PostHog as PostHog Client
participant Fetch as safeTelemetryFetch
participant Network as Network/API
Client->>PostHog: Initialize client (custom fetch, timeout=1000, retries=0, disable remote)
PostHog->>Fetch: Call fetch(url, options)
Fetch->>Network: fetch(url, options) with 1s timeout
alt Network Success (2xx)
Network-->>Fetch: Response { ok: true, status: 200 }
Fetch-->>PostHog: Return original Response
else Non-2xx Response
Network-->>Fetch: Response { ok: false, status: 403 }
Fetch-->>PostHog: Return synthetic Response { status: 204 }
else Network/Error/Abort or Timeout
Network--x Fetch: Throws Error / AbortError / Timeout
Fetch-->>PostHog: Return synthetic Response { status: 204 }
end
PostHog->>PostHog: Continue without retries or remote config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions sets CI=true which disables telemetry, causing PostHog to never be instantiated and the fetch wrapper tests to fail.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
108-108:⚠️ Potential issue | 🟡 MinorKeep support-count messaging consistent across README.
Line 108 says “25+ tools,” but Line 131 still says “20+ AI assistants.” Please align these to avoid contradictory claims in the same document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 108, Update the README to make the support-count messaging consistent: replace the conflicting phrase "20+ AI assistants" so it matches "25+ tools" (or vice versa) wherever the text currently uses "25+ tools" and "20+ AI assistants" — search for those exact strings and standardize to a single agreed count, updating any related wording like "tools" vs "AI assistants" so both occurrences (the lines containing "25+ tools" and "20+ AI assistants") convey the same, non-contradictory message.
🧹 Nitpick comments (1)
docs/cli.md (1)
789-793: Move telemetry init examples out of theopenspec configexamples block.These are
openspec initusage examples, so they read as out-of-place in this subsection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli.md` around lines 789 - 793, The two telemetry examples using OPENSPEC_TELEMETRY and DO_NOT_TRACK belong under the openspec init usage section, not inside the openspec config examples block; move the lines "OPENSPEC_TELEMETRY=0 openspec init" and "DO_NOT_TRACK=1 openspec init" out of the current config subsection and place them into the openspec init examples/usage subsection so they align with the init command examples and remove them from the config block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli.md`:
- Around line 822-823: Add the two new telemetry environment variables to the
Environment Variables table: document OPENSPEC_TELEMETRY and DO_NOT_TRACK,
describe their accepted values (e.g., OPENSPEC_TELEMETRY=0 to disable, 1 to
enable; DO_NOT_TRACK=1 to opt-out), note that they must be set before first run
in restricted/firewalled environments, and include a short example usage line
showing how to disable telemetry (e.g., OPENSPEC_TELEMETRY=0) so users can find
and use these settings alongside the other variables in the table.
---
Outside diff comments:
In `@README.md`:
- Line 108: Update the README to make the support-count messaging consistent:
replace the conflicting phrase "20+ AI assistants" so it matches "25+ tools" (or
vice versa) wherever the text currently uses "25+ tools" and "20+ AI assistants"
— search for those exact strings and standardize to a single agreed count,
updating any related wording like "tools" vs "AI assistants" so both occurrences
(the lines containing "25+ tools" and "20+ AI assistants") convey the same,
non-contradictory message.
---
Nitpick comments:
In `@docs/cli.md`:
- Around line 789-793: The two telemetry examples using OPENSPEC_TELEMETRY and
DO_NOT_TRACK belong under the openspec init usage section, not inside the
openspec config examples block; move the lines "OPENSPEC_TELEMETRY=0 openspec
init" and "DO_NOT_TRACK=1 openspec init" out of the current config subsection
and place them into the openspec init examples/usage subsection so they align
with the init command examples and remove them from the config block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 253cd9f4-87c3-4490-971a-4f1568cf25aa
📒 Files selected for processing (5)
README.mddocs/cli.mddocs/installation.mdsrc/telemetry/index.tstest/telemetry/index.test.ts
Addresses CodeRabbit review comment.
Telemetry now fails silently, so users don't need to proactively disable it. The env vars are still documented in the reference table.
These examples document how config works, not telemetry opt-out.
alfred-openspec
left a comment
There was a problem hiding this comment.
Looks good. The fetch wrapper plus the bounded PostHog settings address the real firewalled-environment failure mode, and the added coverage hits the important cases. I still want a small follow-up for the stale openspec config set telemetry.enabled false workaround in issue #895/docs, but that is separate from this fix.
Summary
safeTelemetryFetchthat catches all network errors and non-2xx responses, returning a synthetic 204 — PostHog never throwsPostHogFetchNetworkErrorfetchRetryCount: 0), remote config, surveys, and feature flag preloading to eliminate extra network calls; adds 1s request timeoutOPENSPEC_TELEMETRYandDO_NOT_TRACKenv vars in the CLI reference tabledocs/cli.mdconfig examples to show env-based telemetry opt-outCloses #895
Test plan
CI=true(simulating GitHub Actions)openspec initwith network blocked — no PostHog errors visibleOPENSPEC_TELEMETRY=0— telemetry fully skipped🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
OPENSPEC_TELEMETRYandDO_NOT_TRACKenvironment variables.Bug Fixes