Skip to content

chore: centralize interactive terminal detection#1000

Open
thymikee wants to merge 1 commit into
feat/replay-test-progress-reporterfrom
chore/centralize-ci-env-checks
Open

chore: centralize interactive terminal detection#1000
thymikee wants to merge 1 commit into
feat/replay-test-progress-reporterfrom
chore/centralize-ci-env-checks

Conversation

@thymikee

@thymikee thymikee commented Jul 1, 2026

Copy link
Copy Markdown
Member

Summary

Adds a shared isInteractive(stream, env) helper for repeated CI + TTY guards and reuses it across auth, install/upload progress, update notifier, and replay reporter interactivity checks.

CI detection is kept internal to the helper and follows ci-info-style markers with explicit CI=false and CI=0 opt-outs. CI-sensitive tests now opt out explicitly with CI=0 so GitHub Actions vendor markers do not suppress expected interactive behavior. The branch stays scoped to environment/interactivity guard cleanup.

Validation

Verified with focused unit coverage for env-map, auth, replay reporter, daemon client, and update-check call sites; pnpm check:quick; pnpm format; and a final pnpm check:unit pass including smoke tests before the test-only CI setup adjustment.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.5 MB 1.5 MB +291 B
JS gzip 470.7 kB 470.9 kB +253 B
npm tarball 572.9 kB 573.1 kB +190 B
npm unpacked 2.0 MB 2.0 MB +291 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.8 ms 28.8 ms +0.0 ms
CLI --help 49.4 ms 49.6 ms +0.2 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/cli.js -63 B -17 B
dist/src/495.js -5 B +7 B

@thymikee thymikee force-pushed the chore/centralize-ci-env-checks branch from d7411ee to bb8aeb5 Compare July 1, 2026 13:01
@thymikee

thymikee commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

CI is blocked on Unit Tests and Coverage. The failed assertions look tied to the centralized CI-environment detection change: src/tests/update-check.test.ts no longer sees the cached upgrade notice or stale-cache background spawn, src/utils/tests/daemon-client.test.ts no longer sees the Installing... stderr notice, and src/replay/test/tests/reporters-default.test.ts still gets no cursor-hide write for the tty progress case. Please update the detection/test setup so the intended non-CI vs CI behavior is explicit, then rerun the failing checks.

@thymikee thymikee force-pushed the chore/centralize-ci-env-checks branch 5 times, most recently from 1896c21 to ac835e8 Compare July 1, 2026 13:25
Base automatically changed from feat/replay-test-progress-reporter to main July 1, 2026 14:21
@thymikee thymikee force-pushed the chore/centralize-ci-env-checks branch from ac835e8 to 30b3a96 Compare July 2, 2026 12:16
@thymikee thymikee changed the base branch from main to feat/replay-test-progress-reporter July 2, 2026 12:19
@thymikee thymikee force-pushed the chore/centralize-ci-env-checks branch from 30b3a96 to 9e71fff Compare July 2, 2026 12:27
@thymikee thymikee changed the title chore: centralize CI environment detection chore: centralize interactive terminal detection Jul 2, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread src/utils/env-map.ts
Comment on lines +19 to +24
function isCI(env: EnvMap = process.env): boolean {
if (env.CI === 'false' || env.CI === '0') return false;
return (
CI_ENV_MARKERS.some((name) => Boolean(env[name])) ||
CI_TRUE_MARKERS.some((name) => env[name] === 'true')
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 CI='false' now overrides all other CI markers, changing auth behavior on GitHub Actions

The new isCI function at src/utils/env-map.ts:20 returns false immediately when CI='false' or CI='0', before checking any other markers like GITHUB_ACTIONS or BUILDKITE. This means { CI: 'false', GITHUB_ACTIONS: 'true' } is now treated as non-CI (tests at src/utils/__tests__/env-map.test.ts:13-14 confirm this). The old isCi in src/cli/auth-session.ts (removed at old lines 485-487) checked env.GITHUB_ACTIONS === 'true' independently, so it would have returned true (CI) in that scenario. In practice this means if a GitHub Actions workflow explicitly sets CI=false (some do for build tools), detectAuthMode would now return 'local-browser' or 'device-code' instead of 'non-interactive', potentially attempting to open a browser on a CI runner. The tests assert this is intentional, but it's a meaningful behavioral change worth confirming with the team.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@thymikee thymikee force-pushed the chore/centralize-ci-env-checks branch from 9e71fff to 55d06be Compare July 2, 2026 12:39
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