fix(cli): gate auto-upgrade on sys.stdin.isatty#50
Conversation
do_auto_upgrade fired from any path that detected a new release on PyPI, including scheduled (cron/launchd) runs. A compromised PyPI release would be picked up silently on the next heartbeat, with immediate access to the Playwright browser-profile session cookies one directory away. Now skips with an INFO log when stdin is not a TTY; interactive sessions still upgrade as before. README "Auto- upgrade" section documents the trust model. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Adding QA Active — starting review of the isatty gate fix at HEAD |
cmeans
left a comment
There was a problem hiding this comment.
QA review — round 1
Verdict: Ready for QA Signoff (one observation, addressed by follow-up).
HEAD: `1c32a751`
Tests: 486/486 pass (no skips, no deselects, no xfails). `TestDoAutoUpgrade`: 5/5 pass. `ruff check src/ tests/`: clean.
Verification
- Gate placement: `sys.stdin.isatty()` check sits inside `do_auto_upgrade` itself (cli.py:150), covering all call sites. The only call site in `main()` is cli.py:722 inside the post-`check_for_update` block.
- `--heartbeat` path: `scheduler.heartbeat()` (scheduler.py:136) does not call `do_auto_upgrade` directly. It spawns the work in a subprocess via `subprocess.Popen(cmd)` (scheduler.py:228), which inherits the cron/launchd parent's non-TTY stdin — so the gate correctly fires in the spawned process.
- `--check-update` path (cli.py:568-582): doesn't invoke `do_auto_upgrade`; just prints upgrade instructions. Correctly unaffected.
- Existing 4 `TestDoAutoUpgrade` tests updated to stub `sys.stdin.isatty` to True via `_force_tty` helper, preserving the upgrade-path coverage.
- New `test_non_tty_session_skips_upgrade_without_invoking_subprocess` asserts the three correct properties: `subprocess.run` never called (`run_called["count"] == 0`), return value is `False`, INFO log mentions "non-interactive".
- README "Auto-upgrade" section adds a clean trust-model paragraph at line 538, well-placed after the enable/disable example and pointing at `--check-update` and `--revert`.
- CHANGELOG entry under `## [Unreleased]` → `### Security` follows the file's existing convention.
Findings
| # | Type | Finding |
|---|---|---|
| 1 | observation | PR closes #43 with only knob 1 of the two suggested in the issue body. PR body acknowledges the scope cut. Filed #55 as P3 nice-to-have follow-up so knob 2 (delayed auto-upgrade) is not lost from tracking. |
Knob 1 addresses the primary failure mode (silent install from scheduled runs). Knob 2 is genuine defense-in-depth against the "interactive-but-fast" attack but does not block this PR.
Test plan checkboxes
All three boxes were pre-checked by Dev and re-verified in this session.
CI: all 7 checks SUCCESS (test ubuntu, test macos, smoke-macos, on-push, on-label, qa-approved, on-unlabel SKIPPED). `QA Gate` StatusContext PENDING — expected until `QA Approved` lands.
Awaiting maintainer to apply `QA Approved`.
|
Applying Ready for QA Signoff — all verification commands re-run green in current session, gate placement covers all call sites including the heartbeat-spawned subprocess, one observation tracked in #55 (P3 follow-up). No remaining QA work. |
…plate) (#54) Cascades the post-`mcp-synology#63` Dependabot-PR-hygiene playbook from the validated `cmeans/mcp-clipboard#96` rollout. Five files: `.github/dependabot.yml` (pip + github-actions, weekly Mon 06:00 CT, grouped, `chore` prefix with `include: scope` to avoid the `chore(deps)(deps):` doubled-prefix bug), `.github/workflows/dependabot-changelog.yml` (`pull_request_target` filtered to `dependabot[bot]`, App-token-authed so the bot's commit re-fires QA-Gate-required CI checks, includes the post-#63 Keep-a-Changelog ordering fix in the create-`### Changed`-from-scratch path), `.github/PULL_REQUEST_TEMPLATE.md` (humans only — Dependabot bypasses templates), `.github/labels.yml` adds `python` + `github-actions`, and a `CHANGELOG.md` Unreleased / Added entry. Operator prereqs `BOT_APP_ID` + `BOT_APP_PRIVATE_KEY` confirmed in-place. SHA pins on all three third-party actions verified exact match upstream tags. Bot user id `272174644` verified via `gh api 'users/cmeans-claude-dev[bot]'`. Workflow's CHANGELOG insertion logic dry-run on this repo's current `[Unreleased]` (which has `### Changed` already) produces correct output. The naive simple `dependabot.yml` alone would break here because of the strict CHANGELOG-per-PR rule + required CI checks + QA-Gate ruleset combination — Dependabot PRs would QA-fail for missing CHANGELOG, and `GITHUB_TOKEN`-authored auto-fix pushes would not re-fire the required checks. The full playbook handles all three. Notes: this PR sits behind PRs #50, #52, #53 in the queue. Once it lands on main, `sync-labels.yml` runs and creates the two new labels before next Monday's Dependabot schedule fires. Dormant Gotcha 6 (Keep-a-Changelog subsection ordering bug from `mcp-synology#63`) is fixed in the workflow but won't fire on yt-dont-recommend's current `[Unreleased]` layout (which already has `### Changed`). It activates after the next release cuts a fresh empty `[Unreleased]` and a feature PR adds `### Added` before any Dependabot bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #55. Defense-in-depth follow-up to #43 (the isatty gate from #50) and the trusted-publisher OIDC setup that landed in publish.yml. Every newly detected PyPI release sits pending for AUTO_UPGRADE_DELAY_DAYS (default 3 days) before do_auto_upgrade will install it. The maintainer therefore has a window to yank a compromised release on PyPI before users on auto_upgrade are exposed. Threat coverage layered with the existing gates: - Trusted-publisher OIDC (publish.yml `id-token: write`, `environment: pypi`) — eliminates the leaked-API-token channel. - isatty gate (#50) — eliminates silent install on cron / launchd. - N-day delay window (this PR) — covers the "compromised tag-push, fast exploit" channel that trusted-publishers don't address. - --revert (already shipped) — recovery after a bad upgrade. What changed: - src/yt_dont_recommend/state.py — Add pending_upgrade key (None default) to both setdefault and the fresh-state return dict; declare it on the AppState TypedDict so the type contract reflects every state key. - src/yt_dont_recommend/config.py — STATE_VERSION 3 → 4; new AUTO_UPGRADE_DELAY_DAYS = 3 constant; new load_auto_upgrade_config() reading auto_upgrade.delay_days from config.yaml with int() coercion and >= 0 guard. - src/yt_dont_recommend/cli.py — New _record_pending_upgrade(state, version) helper that's idempotent (same version preserves the clock, different version resets it). check_for_update calls it on detection in both the cached and fresh-fetch paths; clears pending_upgrade when the release is no longer newer than current (yank scenario). do_auto_upgrade gates on the delay window before the existing isatty check, with defensive initialization for state files written before STATE_VERSION 4. Successful upgrade clears pending_upgrade; failed upgrade preserves it. - README.md — new "Trust model — N-day delay window before installing" paragraph in the Auto-upgrade section explaining the window, ntfy notification timing, and the auto_upgrade.delay_days override. - CLAUDE.md — State Schema block adds pending_upgrade plus a v4-additions section; State Schema Policy checklist gets a step 7 ("Declare the new field on the AppState TypedDict") so future cascades don't drift the type contract. - CHANGELOG.md — entry under [Unreleased] / Security with full threat-model rationale. Behavior: --check-update remains informational only, does not shorten or bypass the window. The ntfy notification still fires on first detection so users see what's coming. Override the default with auto_upgrade.delay_days in ~/.yt-dont-recommend/config.yaml (set to 0 to disable, not recommended). Implementation choice B2 from #55 (explicit-bless via --check-update) was deliberately not implemented. B1 (silence-as-consent after N days) keeps the current "auto-upgrade is automatic" UX promise; B2 would have turned auto_upgrade into "show me upgrades, don't install" and is over-engineered relative to the threat model after trusted-publishers and #50. QA Round 1 found one substantive gap (AppState TypedDict missing the pending_upgrade declaration) — fixed in commit d321305 with a one-line addition plus a State Schema Policy checklist update so the gap doesn't recur. Codecov flagged 68% patch coverage on the new load_auto_upgrade_config loader — fixed in commit 1abc2ff with 11 new test cases bringing config.py / cli.py / state.py all to 100%. Round 2 / Round 3 verdicts: clean. Final tally: 533 / 533 tests pass (vs 522 baseline; +21 net new tests across the four commits), ruff clean, smoke 19/19, three modules touched all at 100% module coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
do_auto_upgradeinsrc/yt_dont_recommend/cli.pynow returns early with an INFO log whensys.stdin.isatty()is False, so scheduled runs (cron / launchd /--heartbeat) notify-only instead of silently installing whatever PyPI publishes.--revertif something goes wrong.--check-update(manual trigger) and--revert(rollback).Test plan
pytest tests/test_cli.py::TestDoAutoUpgrade -v— 5/5 pass:sys.stdin.isattyto True so they still exercise the upgrade pathtest_non_tty_session_skips_upgrade_without_invoking_subprocessasserts the gate:subprocess.runis never called, return isFalse, and an INFO log mentions "non-interactive"pytest tests/— 486/486 passruff check src/ tests/cleanNotes
The issue suggested two independent knobs (interactive-only gate + delayed auto-upgrade). This PR ships only the first — the smallest behavior change with the largest practical effect. The delayed-auto-upgrade option could land later if helpful, but with the isatty gate in place a malicious release no longer ships from a scheduled run, which was the failure mode that mattered.
🤖 Generated with Claude Code