diff --git a/README.md b/README.md index f9bc0f6..35b01c6 100644 --- a/README.md +++ b/README.md @@ -578,7 +578,9 @@ Every write verb the VS Code extension drives runs **without a TTY** — explici `list-open-issues --repo= [--exclude=]` is a second viewer read surface: it emits a repo's **open** issues as JSON (`{repo, issues:[…]}`, the same per-issue shape as `export`). The extension's **Slot** command uses it to offer a pick-list instead of a typed number; `--exclude` drops the track's current issues so they don't reappear. Unlike `export`'s `untracked` (open issues in *no* track), this includes issues tracked by *other* tracks — they're valid slot targets. Read-only. -`auth-status [--json]` is the viewer's **auth probe**: it runs `gh auth status` and emits `{gh_present, authenticated, user, error}` (exit `0` authenticated / `1` gh present but not signed in / `2` gh not found). Because every GitHub read goes through `gh` and the fetch helpers return empty rather than erroring, an unauthenticated session would otherwise look like an empty-but-working one. The extension calls this at activation (and after every refresh) to **fast-fail with a clear "Not signed in to GitHub" banner + a Sign in path** instead of rendering a misleadingly empty tree — and distinguishes "not signed in" (`gh auth login`) from "gh not installed" (a different fix). Read-only. +`auth-status [--json]` is the viewer's **auth probe**: it runs `gh auth status` and emits `{gh_present, authenticated, probe_ok, user, error}` (exit `0` authenticated / `1` gh present but not signed in / `2` gh not found / `3` the probe couldn't reach a verdict). Because every GitHub read goes through `gh` and the fetch helpers return empty rather than erroring, an unauthenticated session would otherwise look like an empty-but-working one. The extension calls this at activation (and after every refresh) to **fast-fail with a clear "Not signed in to GitHub" banner + a Sign in path** instead of rendering a misleadingly empty tree — and distinguishes "not signed in" (`gh auth login`) from "gh not installed" (a different fix). Read-only. + +**`probe_ok` is the trust flag** — and the reason this is more than an exit-code check. `gh auth status` performs a *live* token-validation request, so a momentary network failure (a waking laptop, a reconnecting VPN) exits non-zero and actively misreports the cause as `The token in keyring is invalid.` It also exits non-zero when *any* configured account fails validation, so one stale second account would otherwise read as a logout of a healthy active one. Only two verdicts are therefore treated as authoritative (`probe_ok: true`): a clean exit `0`, and the explicit "not logged into any GitHub hosts" message. Everything else — a validation failure, a timeout, an unrecognised exit — is `probe_ok: false`, meaning *unverified, not signed out*; callers keep whatever state they last had rather than prompting for a sign-in that isn't needed. `plan-status --json` is the viewer's **Plans view** read surface: alongside each doc's verdict it now also emits `manifest_last_touched` (the most recent commit date across the plan's declared files), `stalled`, `lie_gap`, `unchecked_items`, and `stall_days`. The staleness window honors `stall_days:` in `~/.claude/work-plan/config.yml` and a `--stall-days=` flag (precedence: flag → config → default 14). The viewer consumes these to flag plans whose declared-file build has gone cold — a `partial` plan with no recent commit on its manifest ("stalled") — and plans scored shipped whose own phase checkboxes are mostly unticked ("lie-gap"). It also emits `override` (the human `verdict_override`, `shipped`/`partial`/`dead` or `null`, set via `plan-confirm` — when present the CLI pins the verdict to it and forces `lie_gap` false), `acknowledged` (the durable frontmatter ack set via `plan-ack`), and `verdict_baseline` + `verdict_drift` (the drift tripwire set via `plan-baseline` — `verdict_drift` is true when the live verdict no longer matches the stamped baseline, suppressed under an override). It also emits `offtree_paths`: declared manifest paths that resolve **outside** the repo (absolute, `~`, `..`-escape, junk `/`) — a read-only flag for a typo or misfiled plan that would otherwise silently drag the file score down (the 🧳 foreign verdict only fires when *most* paths are off-tree; this surfaces the sub-threshold ones too). Never auto-fixed — surfacing only. diff --git a/skills/work-plan/commands/auth_status.py b/skills/work-plan/commands/auth_status.py index 56f74c4..6dd3383 100644 --- a/skills/work-plan/commands/auth_status.py +++ b/skills/work-plan/commands/auth_status.py @@ -8,7 +8,9 @@ Read-only; never mutates anything. Exit code mirrors auth state so a shell caller can gate on it: 0 = authenticated, 1 = gh present but not logged in, 2 = gh not -found. +found, 3 = the probe couldn't reach a verdict (#485 — a live-network validation +failure or a timeout, which must NOT be reported as a logout). `rc == 0` remains +the "am I usable" gate; 3 is additive detail for callers that care why not. """ import json @@ -20,6 +22,10 @@ def run(args: list) -> int: flags, _ = parse_flags(args, {"--json"}) status = github_state.gh_auth_status() + # Absent probe_ok (an older/hand-built status dict) means "trust the verdict", + # preserving the historical two-way split. + probe_ok = status.get("probe_ok", True) + if flags.get("--json"): print(json.dumps(status)) elif status["authenticated"]: @@ -27,9 +33,19 @@ def run(args: list) -> int: print(f"✓ Authenticated to GitHub{who}.") elif not status["gh_present"]: print("✗ GitHub CLI (gh) not found on PATH. Install it: https://cli.github.com") + elif not probe_ok: + # The probe failed to reach a verdict — do NOT send a signed-in user + # through a pointless sign-in flow (#485). Relay gh's own words; they + # carry the real remediation (wait for the network, or `gh auth refresh`). + print("? Couldn't verify GitHub sign-in — the probe didn't reach a verdict.") + print(" This is usually a transient network failure, not a logout.") + if status.get("error"): + print(f" gh said: {status['error']}") else: print("✗ Not logged in to GitHub. Run: gh auth login") if status["authenticated"]: return 0 - return 2 if not status["gh_present"] else 1 + if not status["gh_present"]: + return 2 + return 1 if probe_ok else 3 diff --git a/skills/work-plan/lib/github_state.py b/skills/work-plan/lib/github_state.py index 9bc7bda..50ed592 100644 --- a/skills/work-plan/lib/github_state.py +++ b/skills/work-plan/lib/github_state.py @@ -87,41 +87,67 @@ def set_issue_in_progress(repo: str, number: int, clear: bool = False) -> tuple: return (True, (proc.stdout or f"{verb} #{number} in-progress").strip()) +# `gh auth status` says this — and only this — when no credentials are +# configured at all. Every OTHER non-zero exit means "gh has credentials but +# couldn't confirm them", which is NOT a logout (#485). Both the current and the +# legacy phrasings begin the same way. +_NO_CREDENTIALS_RE = re.compile(r"not logged in ?to any (?:GitHub )?hosts?", re.I) + + def gh_auth_status() -> dict: """Probe `gh` authentication so callers can fast-fail instead of silently degrading (#auth). Returns: - {"gh_present": bool, "authenticated": bool, + {"gh_present": bool, "authenticated": bool, "probe_ok": bool, "user": str | None, "error": str | None} - Distinguishes the two failure modes the UI must handle differently: + Distinguishes the failure modes the UI must handle differently: `gh` not installed (`gh_present` False — fix is "install gh") vs installed - but not logged in (`authenticated` False — fix is "gh auth login"). - - Never raises. `gh auth status` exits 0 when at least one host is logged in, - non-zero otherwise; it prints the human status to STDERR. We parse a - best-effort `user` from that text but treat the EXIT CODE as authoritative.""" + but not logged in (`authenticated` False — fix is "gh auth login") vs the + probe never reaching a verdict at all (`probe_ok` False — fix is "wait / + retry", NOT a sign-in prompt). + + `probe_ok` is the trust flag, and it is the whole reason this function is + more than an exit-code check (#485). `gh auth status` performs a LIVE + token-validation request, so a momentary network failure — waking from + sleep, a reconnecting VPN — exits non-zero and actively misreports the cause + as "The token in keyring is invalid." Treating that as a logout wipes the + viewer's tree and tells an already-signed-in user to sign in again. It also + exits non-zero when ANY configured account fails validation, so one stale + second account would otherwise read as a logout of a healthy active one. + + So only two verdicts are authoritative: exit 0 (signed in), and the explicit + "not logged into any hosts" message (signed out). Everything else — a + validation failure, a timeout, an OS error, an exit code we don't recognise + — is indeterminate, and callers should keep whatever state they last had. + + Never raises. `gh` prints its human status to STDERR; we parse a best-effort + `user` from that text.""" try: proc = subprocess.run( ["gh", "auth", "status"], capture_output=True, text=True, timeout=GH_TIMEOUT, ) except FileNotFoundError: - return {"gh_present": False, "authenticated": False, + # Authoritative: the binary genuinely isn't there. + return {"gh_present": False, "authenticated": False, "probe_ok": True, "user": None, "error": "gh CLI not found on PATH"} except Exception as e: # timeout / OS error — gh present but unusable now - return {"gh_present": True, "authenticated": False, + return {"gh_present": True, "authenticated": False, "probe_ok": False, "user": None, "error": f"gh auth status failed: {e}"} blob = f"{proc.stdout}\n{proc.stderr}" authenticated = proc.returncode == 0 + # Authoritative only for a clean success or an explicit no-credentials + # message. Anything else is a probe that failed to reach a verdict. + probe_ok = authenticated or bool(_NO_CREDENTIALS_RE.search(blob)) # `gh auth status` prints e.g. "✓ Logged in to github.com account USER" or # the older "Logged in to github.com as USER". Match either phrasing. m = re.search(r"Logged in to \S+ (?:account|as) (\S+)", blob) user = m.group(1) if (authenticated and m) else None error = None if authenticated else (blob.strip() or "not logged in to GitHub") return {"gh_present": True, "authenticated": authenticated, - "user": user, "error": error} + "probe_ok": probe_ok, "user": user, "error": error} def fetch_issue(repo: str, number: int) -> Optional[dict]: diff --git a/skills/work-plan/tests/test_auth_status.py b/skills/work-plan/tests/test_auth_status.py index 18dc7c1..0e97c9e 100644 --- a/skills/work-plan/tests/test_auth_status.py +++ b/skills/work-plan/tests/test_auth_status.py @@ -27,6 +27,7 @@ def test_authenticated_parses_user(self): s = github_state.gh_auth_status() self.assertTrue(s["authenticated"]) self.assertTrue(s["gh_present"]) + self.assertTrue(s["probe_ok"]) self.assertEqual(s["user"], "evemcgivern") self.assertIsNone(s["error"]) @@ -43,6 +44,7 @@ def test_not_logged_in(self): s = github_state.gh_auth_status() self.assertFalse(s["authenticated"]) self.assertTrue(s["gh_present"]) # gh ran, just not logged in + self.assertTrue(s["probe_ok"]) # authoritative: no credentials at all self.assertIsNone(s["user"]) self.assertIn("not logged", s["error"].lower()) @@ -51,14 +53,74 @@ def test_gh_not_installed(self): s = github_state.gh_auth_status() self.assertFalse(s["gh_present"]) self.assertFalse(s["authenticated"]) + self.assertTrue(s["probe_ok"]) # authoritative: gh is genuinely absent self.assertIn("not found", s["error"].lower()) - def test_timeout_is_present_but_unauthenticated(self): + +class GhAuthStatusIndeterminateTest(unittest.TestCase): + """#485 — a probe that could not REACH a verdict must never be reported as a + logout. `gh auth status` makes a live token-validation call, so a network + blip (waking from sleep, VPN reconnecting) exits non-zero and claims the + keyring token is invalid. Treating that as "signed out" wipes the viewer's + tree and shows a sign-in banner to an already-signed-in user.""" + + # Verbatim `gh auth status` output with the network unreachable, while the + # keyring token is in fact perfectly valid. + NETWORK_BLIP = ( + "github.com\n" + " X Failed to log in to github.com account evemcgivern (keyring)\n" + " - Active account: true\n" + " - The token in keyring is invalid.\n" + " - To re-authenticate, run: gh auth refresh -h github.com\n" + ) + + def test_validation_failure_is_indeterminate_not_logged_out(self): + out = _proc(1, stderr=self.NETWORK_BLIP) + with mock.patch("lib.github_state.subprocess.run", return_value=out): + s = github_state.gh_auth_status() + self.assertFalse(s["authenticated"]) + self.assertTrue(s["gh_present"]) + self.assertFalse(s["probe_ok"]) # the whole point: NOT authoritative + self.assertIn("token in keyring is invalid", s["error"]) + + def test_timeout_is_indeterminate(self): with mock.patch("lib.github_state.subprocess.run", side_effect=subprocess.TimeoutExpired("gh", 30)): s = github_state.gh_auth_status() self.assertTrue(s["gh_present"]) self.assertFalse(s["authenticated"]) + self.assertFalse(s["probe_ok"]) + + def test_os_error_is_indeterminate(self): + with mock.patch("lib.github_state.subprocess.run", + side_effect=OSError("resource temporarily unavailable")): + s = github_state.gh_auth_status() + self.assertTrue(s["gh_present"]) + self.assertFalse(s["authenticated"]) + self.assertFalse(s["probe_ok"]) + + def test_unrecognised_failure_defaults_to_indeterminate(self): + """Unknown non-zero exits stay indeterminate. Keeping a stale tree is a + far cheaper error than falsely telling a signed-in user to sign in.""" + out = _proc(1, stderr="error connecting to api.github.com") + with mock.patch("lib.github_state.subprocess.run", return_value=out): + s = github_state.gh_auth_status() + self.assertFalse(s["authenticated"]) + self.assertFalse(s["probe_ok"]) + + def test_one_broken_account_alongside_a_healthy_one_is_indeterminate(self): + """gh exits non-zero if ANY configured account fails validation, so a + stale second account must not read as a logout of the active one.""" + out = _proc(1, stderr=( + "github.com\n" + " ✓ Logged in to github.com account evemcgivern (keyring)\n" + " - Active account: true\n" + " X Failed to log in to github.com account eve-mcgivern (keyring)\n" + " - The token in keyring is invalid.\n" + )) + with mock.patch("lib.github_state.subprocess.run", return_value=out): + s = github_state.gh_auth_status() + self.assertFalse(s["probe_ok"]) class AuthStatusCommandTest(unittest.TestCase): @@ -70,29 +132,58 @@ def _run(self, status, args): return rc, buf.getvalue() def test_json_authenticated_exit_0(self): - status = {"gh_present": True, "authenticated": True, "user": "eve", "error": None} + status = {"gh_present": True, "authenticated": True, "probe_ok": True, + "user": "eve", "error": None} rc, out = self._run(status, ["--json"]) self.assertEqual(rc, 0) self.assertEqual(json.loads(out), status) def test_not_logged_in_exit_1(self): - status = {"gh_present": True, "authenticated": False, "user": None, "error": "x"} + status = {"gh_present": True, "authenticated": False, "probe_ok": True, + "user": None, "error": "x"} rc, out = self._run(status, []) self.assertEqual(rc, 1) self.assertIn("gh auth login", out) def test_gh_missing_exit_2(self): - status = {"gh_present": False, "authenticated": False, "user": None, "error": "x"} + status = {"gh_present": False, "authenticated": False, "probe_ok": True, + "user": None, "error": "x"} rc, out = self._run(status, []) self.assertEqual(rc, 2) self.assertIn("not found", out.lower()) def test_human_authenticated_names_user(self): - status = {"gh_present": True, "authenticated": True, "user": "eve", "error": None} + status = {"gh_present": True, "authenticated": True, "probe_ok": True, + "user": "eve", "error": None} rc, out = self._run(status, []) self.assertEqual(rc, 0) self.assertIn("eve", out) + def test_indeterminate_exit_3_does_not_say_not_logged_in(self): + """#485 — the terminal output must not tell a signed-in user to run + `gh auth login` when the probe merely failed to reach a verdict.""" + status = {"gh_present": True, "authenticated": False, "probe_ok": False, + "user": None, "error": "The token in keyring is invalid."} + rc, out = self._run(status, []) + self.assertEqual(rc, 3) + self.assertNotIn("gh auth login", out) + self.assertIn("couldn't verify", out.lower()) + self.assertIn("token in keyring is invalid", out) + + def test_indeterminate_json_still_exits_3(self): + status = {"gh_present": True, "authenticated": False, "probe_ok": False, + "user": None, "error": "boom"} + rc, out = self._run(status, ["--json"]) + self.assertEqual(rc, 3) + self.assertEqual(json.loads(out), status) + + def test_missing_probe_ok_key_is_treated_as_authoritative(self): + """Back-compat: a status dict from an older code path (no probe_ok) keeps + the historical exit-1 behaviour rather than becoming indeterminate.""" + status = {"gh_present": True, "authenticated": False, "user": None, "error": "x"} + rc, _ = self._run(status, []) + self.assertEqual(rc, 1) + if __name__ == "__main__": unittest.main() diff --git a/vscode/README.md b/vscode/README.md index db0d087..f4ed6e2 100644 --- a/vscode/README.md +++ b/vscode/README.md @@ -25,7 +25,8 @@ The human face of the [`work-plan`](https://github.com/stylusnexus/work-plan-too **Get started from empty** — a cold-start a new user can drive without the CLI: -- **Not signed in to GitHub?** Because all issue data comes through the GitHub CLI (`gh`), the view **fast-fails** instead of showing a misleadingly empty tree: a **"Not signed in to GitHub"** banner replaces the tracks, with a **Sign in to GitHub** button that opens `gh auth login` in a terminal and a **Retry** once you're done. A distinct **"GitHub CLI not found"** banner covers the case where `gh` isn't installed (with an install link). A third **"Couldn't verify GitHub sign-in"** banner covers the case where the `work-plan` CLI ran but returned no usable result — a missing CLI dependency (`gh` / `git` / `yq`), not a GitHub problem — so a missing tool no longer masquerades as "not signed in" and sends you into a futile sign-in loop. Signed-in users never see any of this. +- **Not signed in to GitHub?** Because all issue data comes through the GitHub CLI (`gh`), the view **fast-fails** instead of showing a misleadingly empty tree: a **"Not signed in to GitHub"** banner replaces the tracks, with a **Sign in to GitHub** button that opens `gh auth login` in a terminal and a **Retry** once you're done. A distinct **"GitHub CLI not found"** banner covers the case where `gh` isn't installed (with an install link). A third **"Couldn't verify GitHub sign-in"** banner covers the case where the sign-in check ran but didn't reach a verdict — most often a transient network failure, sometimes a missing CLI dependency (`gh` / `git` / `yq`) — so neither masquerades as "not signed in" and sends you into a futile sign-in loop. Signed-in users never see any of this. +- **A network blip won't log you out of the viewer.** `gh auth status` validates your token over the network, so a waking laptop or a reconnecting VPN makes it fail — and `gh` misreports that as `The token in keyring is invalid.` The viewer treats an unverifiable check as *unverified*, not *signed out*: your tracks stay on screen with a subtle stale indicator, and the last-good tree is **persisted across window reloads**, so coming back to a sleeping laptop no longer greets you with a sign-in banner you don't need. A genuine `gh auth logout` still shows the real one. - When you have no repos yet, the tree shows a welcome with **Add a repo** and **Set notes location** buttons. - **Add Repo** runs `init-repo`; **Set Notes Location** runs `set-notes-root` so your private track notes live wherever you choose (not just the hidden default). Config itself is auto-seeded by the CLI on first run. @@ -219,8 +220,9 @@ The webview loads **`dist/mermaid.min.js`** — the **UMD bundle** from Mermaid ## Status -**Published — v0.19.9 on the [VS Code Marketplace](https://marketplace.visualstudio.com/items?itemName=stylusnexus.work-plan-viewer) and [Open VSX](https://open-vsx.org/extension/stylusnexus/work-plan-viewer)** (publisher `stylusnexus`). +**Published — v0.19.10 on the [VS Code Marketplace](https://marketplace.visualstudio.com/items?itemName=stylusnexus.work-plan-viewer) and [Open VSX](https://open-vsx.org/extension/stylusnexus/work-plan-viewer)** (publisher `stylusnexus`). +- **v0.19.10** — **A network blip no longer looks like being signed out.** Returning to a sleeping laptop could greet you with a **"Not signed in to GitHub"** banner and an empty tree while `gh` was perfectly authenticated: `gh auth status` validates your token over the network, so a reconnecting VPN or waking machine makes it fail — and `gh` reports that as `The token in keyring is invalid.` The viewer took that at face value. It now distinguishes *unverified* from *signed out*: only a clean success or an explicit "not logged into any hosts" is treated as authoritative, so a failed check keeps your tracks on screen behind a "couldn't verify" banner instead of an onboarding prompt. The last-good tree is also **persisted across window reloads**, which is what made the banner reappear every time you came back. Also fixes multi-account setups, where one stale `gh` account made the check fail and read as a logout of the healthy active one. A genuine `gh auth logout` still shows the real sign-in banner. Paired with a CLI release adding a `probe_ok` trust flag to `auth-status` — but the viewer also recognises the failure without it, so the fix works against an already-installed CLI. - **v0.19.9** — **Clarifies and reorganizes the track right-click menu.** The 18 flat, mechanism-named actions are now grouped by how often you use them, with clearer intent-first labels: **Sync Issue States from GitHub → Refresh Track from GitHub**, **Mark for Cleanup → Mark as Stale 🧹** (moved out of the destructive group — it deletes nothing), **Set Next-Up Order → Change Next-Up Ranking** (it's config, not a list edit), and the two next-up methods paired as **Set Next-Up (pick manually / auto-suggest)**. The rarely-touched and config actions (Edit Fields, ranking, cross-track reference, label-drift, publish, rename, stale flags) move into a **More Actions ▸** submenu; **Delete Track (Permanent)** is now the only item in the fenced danger zone; and Close / Archive move to a reversible "lifecycle" group. Adds a **What Do These Actions Do?** help entry that opens the docs, and a track hover tooltip showing the next-up glance plus a left/right-click hint. Extension-only; CLI unchanged. - **v0.19.8** — Follow-up wording fix for a convergence track's tree row: the previous `N open · X references (Y open)` label used "open" for two different scopes (issues the track owns vs. issues it references elsewhere) with no visual cue for the shift, reading as self-contradictory at a glance. Reworded to `N owned · Y of X referenced still open`, scoping each number to a distinct noun; the tooltip's fuller explanation is unchanged. - **v0.19.7** — Fixes a misleading tree-row count: a convergence track that owns zero issues but has open cross-track references (`github.references`) showed only a bare `0 open · N references`, which reads as "nothing to do" even when references are still open. The row now shows `0 open · N references (M open)`, and the tooltip states how many referenced issues are open; the underlying `demote-to-reference` migration CLI command (#462) is what surfaced the gap. diff --git a/vscode/package.json b/vscode/package.json index d22db48..d3dc7fc 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -2,7 +2,7 @@ "name": "work-plan-viewer", "displayName": "Work Plan", "publisher": "stylusnexus", - "version": "0.19.9", + "version": "0.19.10", "description": "Browse and manage GitHub issues as tracks — dependency graph, per-track detail, and read/write (slot, move, reconcile, close) in the sidebar.", "license": "MIT", "icon": "media/icon.png", @@ -857,7 +857,7 @@ { "view": "workPlan.tree", "when": "workPlanGitHubAuthed == probe-error", - "contents": "Couldn't verify GitHub sign-in.\n\nThe `work-plan` CLI ran but didn't return a result, so Work Plan can't tell whether you're signed in. This is usually a missing CLI dependency, not a GitHub problem — the CLI needs `gh`, `git`, and `yq` (the Go mikefarah/yq) on the same PATH the editor runs in.\n\nCheck dependencies, then:\n[Retry](command:workPlan.checkGitHubAuth)\n\n[Install instructions](https://github.com/stylusnexus/work-plan-toolkit#install)" + "contents": "Couldn't verify GitHub sign-in.\n\nThe check didn't reach a verdict, so Work Plan can't tell whether you're signed in. You have NOT been signed out — don't start a sign-in flow yet.\n\nMost often this is transient: `gh auth status` makes a live network call, so a waking laptop or a reconnecting VPN makes it fail (and misreport your token as invalid). Wait a moment and retry.\n\n[Retry](command:workPlan.checkGitHubAuth)\n\nIf it persists, it's usually a CLI dependency rather than a GitHub problem — the CLI needs `gh`, `git`, and `yq` (the Go mikefarah/yq) on the same PATH the editor runs in.\n\n[Install instructions](https://github.com/stylusnexus/work-plan-toolkit#install)" }, { "view": "workPlan.tree", diff --git a/vscode/src/authCache.test.ts b/vscode/src/authCache.test.ts new file mode 100644 index 0000000..dc30d78 --- /dev/null +++ b/vscode/src/authCache.test.ts @@ -0,0 +1,140 @@ +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; + +import { + SNAPSHOT_VERSION, + SNAPSHOT_MAX_AGE_MS, + SNAPSHOT_MAX_BYTES, + SNAPSHOT_MIN_WRITE_INTERVAL_MS, + fitsSizeCap, + makeSnapshot, + readSnapshot, + shouldPersist, +} from "./authCache.ts"; +import type { Snapshot, SnapshotStore } from "./authCache.ts"; +import type { Export } from "./model.ts"; + +// --------------------------------------------------------------------------- +// authCache (#485) — last-good tree survives a window reload +// --------------------------------------------------------------------------- + +const EXPORT = { + tracks: [{ name: "alpha", repo: "o/r" }], + repos: [{ github: "o/r" }], +} as unknown as Export; + +/** In-memory stand-in for the globalState memento. */ +function memStore(initial?: unknown): SnapshotStore & { written: unknown } { + let held = initial; + return { + get written() { return held; }, + get: () => held as Snapshot | undefined, + set: (v) => { held = v; }, + }; +} + +const NOW = 1_700_000_000_000; + +describe("authCache", () => { + test("a snapshot written now round-trips back", () => { + const store = memStore(); + store.set(makeSnapshot(EXPORT, { authenticated: true }, NOW)); + const got = readSnapshot(store, NOW); + assert.deepEqual(got?.export, EXPORT); + assert.equal(got?.wasAuthenticated, true); + }); + + test("stamps the current schema version", () => { + assert.equal(makeSnapshot(EXPORT, { authenticated: true }, NOW).version, SNAPSHOT_VERSION); + }); + + test("empty store → null (cold first run, no crash)", () => { + assert.equal(readSnapshot(memStore(), NOW), null); + }); + + test("a snapshot from a future schema version is ignored", () => { + // Forward-compat: a newer extension wrote a shape we can't read. Dropping it + // costs one refresh; misreading it would corrupt the tree. + const store = memStore({ ...makeSnapshot(EXPORT, { authenticated: true }, NOW), version: SNAPSHOT_VERSION + 1 }); + assert.equal(readSnapshot(store, NOW), null); + }); + + test("a snapshot from an older schema version is ignored", () => { + const store = memStore({ ...makeSnapshot(EXPORT, { authenticated: true }, NOW), version: SNAPSHOT_VERSION - 1 }); + assert.equal(readSnapshot(store, NOW), null); + }); + + test("a snapshot within the age cap is kept", () => { + const store = memStore(makeSnapshot(EXPORT, { authenticated: true }, NOW)); + assert.notEqual(readSnapshot(store, NOW + SNAPSHOT_MAX_AGE_MS - 1), null); + }); + + test("a snapshot past the age cap is dropped", () => { + // A tree from a fortnight ago is misinformation, not a cache. + const store = memStore(makeSnapshot(EXPORT, { authenticated: true }, NOW)); + assert.equal(readSnapshot(store, NOW + SNAPSHOT_MAX_AGE_MS + 1), null); + }); + + test("a snapshot stamped in the future is dropped (clock skew)", () => { + const store = memStore(makeSnapshot(EXPORT, { authenticated: true }, NOW + 60_000)); + assert.equal(readSnapshot(store, NOW), null); + }); + + test("garbage in the store never throws", () => { + for (const junk of [null, 42, "nope", [], {}, { version: SNAPSHOT_VERSION }]) { + assert.equal(readSnapshot(memStore(junk), NOW), null, `junk: ${JSON.stringify(junk)}`); + } + }); + + test("a snapshot with a non-object export is rejected", () => { + const store = memStore({ version: SNAPSHOT_VERSION, savedAt: NOW, export: "tracks", wasAuthenticated: true }); + assert.equal(readSnapshot(store, NOW), null); + }); + + test("only an authenticated state is ever persisted as authenticated", () => { + // We restore the signed-in context key optimistically on cold start, so a + // negative state must never be resurrected — that would show a sign-in + // banner before the probe has even run. + assert.equal(makeSnapshot(EXPORT, { authenticated: false }, NOW).wasAuthenticated, false); + assert.equal(makeSnapshot(EXPORT, null, NOW).wasAuthenticated, false); + }); + + test("the first write is never throttled", () => { + assert.equal(shouldPersist(null, NOW), true); + }); + + test("a second write inside the throttle window is skipped", () => { + assert.equal(shouldPersist(NOW, NOW + SNAPSHOT_MIN_WRITE_INTERVAL_MS - 1), false); + }); + + test("a write past the throttle window goes through", () => { + assert.equal(shouldPersist(NOW, NOW + SNAPSHOT_MIN_WRITE_INTERVAL_MS), true); + }); + + test("a backwards clock doesn't wedge the throttle shut", () => { + // Without this, a clock correction could block persistence indefinitely. + assert.equal(shouldPersist(NOW, NOW - 60_000), true); + }); + + test("a normal export fits the size cap", () => { + assert.equal(fitsSizeCap(makeSnapshot(EXPORT, { authenticated: true }, NOW)), true); + }); + + test("an oversized export is rejected rather than bloating globalState", () => { + const huge = { tracks: [{ name: "x".repeat(SNAPSHOT_MAX_BYTES + 1) }] } as unknown as Export; + assert.equal(fitsSizeCap(makeSnapshot(huge, { authenticated: true }, NOW)), false); + }); + + test("an unserialisable export is rejected, not thrown", () => { + const cyclic: Record = { tracks: [] }; + cyclic.self = cyclic; + assert.equal(fitsSizeCap(makeSnapshot(cyclic as unknown as Export, null, NOW)), false); + }); + + test("readSnapshot does not mutate the store", () => { + const store = memStore(makeSnapshot(EXPORT, { authenticated: true }, NOW)); + const before = store.written; + readSnapshot(store, NOW); + assert.equal(store.written, before); + }); +}); diff --git a/vscode/src/authCache.ts b/vscode/src/authCache.ts new file mode 100644 index 0000000..fbdc688 --- /dev/null +++ b/vscode/src/authCache.ts @@ -0,0 +1,127 @@ +import type { Export } from "./model.ts"; + +// --------------------------------------------------------------------------- +// Last-good tree persistence (#485) +// --------------------------------------------------------------------------- +// +// `gh auth status` makes a live token-validation call, so a momentary network +// failure — waking from sleep, a reconnecting VPN — reports a healthy session as +// signed out. tree.ts already keeps the last-good tree through an untrustworthy +// probe, but only while a tree exists in memory: after a window reload the cache +// is null, so the blip wipes the view and shows a sign-in banner to someone who +// never signed out. That's the "annoying every time I come back" case. +// +// Persisting the last-good export closes it. The snapshot is only ever consulted +// as a fallback — the very next successful refresh overwrites it — so the worst +// case is briefly showing tracks that are a few minutes stale, which is exactly +// what the load-error indicator is for. + +/** Bump when the persisted shape changes. A mismatch is dropped, not migrated — + * the cost is one refresh, and a mis-read snapshot would corrupt the tree. */ +export const SNAPSHOT_VERSION = 1; + +/** Past this, a snapshot is misinformation rather than a cache. A week covers a + * long weekend offline without resurrecting a tree from a previous sprint. */ +export const SNAPSHOT_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; + +/** A real multi-repo export runs to hundreds of KB, so persisting on EVERY + * refresh would push that much through globalState each time — and with + * `workPlan.autoRefreshInterval` on, on a timer. The snapshot only has to be + * fresh enough to survive a reload, so once a minute is ample. */ +export const SNAPSHOT_MIN_WRITE_INTERVAL_MS = 60_000; + +/** Refuse to persist beyond this. globalState is a shared per-user store, not a + * place to park an unbounded blob; a workspace this large gives up cross-reload + * caching rather than degrading the whole editor's state file. */ +export const SNAPSHOT_MAX_BYTES = 4 * 1024 * 1024; + +export type Snapshot = { + version: number; + /** Epoch ms when this was written. */ + savedAt: number; + export: Export; + /** Whether the probe that produced this export said we were signed in. Only + * ever true for an authoritative success — a negative state is never + * resurrected, since we restore the signed-in context key optimistically. */ + wasAuthenticated: boolean; +}; + +/** The slice of `vscode.Memento` we need, narrowed so this module stays testable + * without the vscode runtime. */ +export interface SnapshotStore { + get(): Snapshot | undefined; + set(value: Snapshot | undefined): void; +} + +/** The globalState key. Exported so extension.ts and any future migration agree + * on one spelling. */ +export const SNAPSHOT_KEY = "workPlan.lastGoodSnapshot"; + +export function makeSnapshot( + exp: Export, + auth: { authenticated: boolean } | null, + now: number, +): Snapshot { + return { + version: SNAPSHOT_VERSION, + savedAt: now, + export: exp, + wasAuthenticated: auth?.authenticated === true, + }; +} + +/** + * Throttle gate for persistence. `lastWrittenAt` is null before the first write + * of a session, which always goes through so a reload right after startup is + * still protected. A `now` earlier than `lastWrittenAt` means the clock moved + * backwards; allow the write rather than wedging the throttle shut. + */ +export function shouldPersist(lastWrittenAt: number | null, now: number): boolean { + if (lastWrittenAt === null) return true; + const since = now - lastWrittenAt; + return since < 0 || since >= SNAPSHOT_MIN_WRITE_INTERVAL_MS; +} + +/** Whether a snapshot is small enough to persist. Also the serialisability + * check — a value that can't be stringified can't be stored either. */ +export function fitsSizeCap(snap: Snapshot): boolean { + try { + return JSON.stringify(snap).length <= SNAPSHOT_MAX_BYTES; + } catch { + return false; + } +} + +/** + * Returns the persisted snapshot when it's still trustworthy, else null. Never + * throws: the store holds whatever a previous version (or a corrupted profile + * sync) left behind, and a bad snapshot must degrade to "no cache", never to a + * broken activation. + */ +export function readSnapshot(store: SnapshotStore, now: number): Snapshot | null { + let raw: unknown; + try { + raw = store.get(); + } catch { + return null; + } + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) return null; + + const snap = raw as Partial; + if (snap.version !== SNAPSHOT_VERSION) return null; + if (typeof snap.savedAt !== "number" || !Number.isFinite(snap.savedAt)) return null; + // Future-stamped means the clock moved backwards; the age check can't bound it. + const age = now - snap.savedAt; + if (age < 0 || age > SNAPSHOT_MAX_AGE_MS) return null; + if (typeof snap.export !== "object" || snap.export === null || Array.isArray(snap.export)) { + return null; + } + if (!Array.isArray((snap.export as Export).tracks)) return null; + + return { + version: SNAPSHOT_VERSION, + savedAt: snap.savedAt, + export: snap.export as Export, + wasAuthenticated: snap.wasAuthenticated === true, + }; +} diff --git a/vscode/src/cli.test.ts b/vscode/src/cli.test.ts index 2079279..a5a683f 100644 --- a/vscode/src/cli.test.ts +++ b/vscode/src/cli.test.ts @@ -5,6 +5,7 @@ import type { CliResult, CliRunner } from "./cli.ts"; import { CliError, isAlreadyExistsError, + summariseAuthError, exportJson, listRepoOpenIssues, planStatus, @@ -625,6 +626,151 @@ describe("checkAuth", () => { }); }); + // -- #485: a probe that couldn't reach a verdict must not read as a logout --- + + const KEYRING_BLIP = + "github.com\n X Failed to log in to github.com account eve (keyring)\n" + + " - The token in keyring is invalid.\n - To re-authenticate, run: gh auth refresh -h github.com"; + + test("#485 probe_ok:false from the CLI → probeOk:false and the reason survives", async () => { + // A live-network validation failure. gh exits non-zero and actively + // misreports the cause as an invalid keyring token; the CLI now flags the + // verdict as untrustworthy so the tree is kept instead of wiped. + const run = fakeRunner({ + code: 3, + stdout: JSON.stringify({ + gh_present: true, authenticated: false, probe_ok: false, user: null, error: KEYRING_BLIP, + }), + stderr: "", + }); + assert.deepEqual(await checkAuth(run), { + authenticated: false, cliPresent: true, ghPresent: true, probeOk: false, user: null, + error: KEYRING_BLIP, + }); + }); + + test("#485 probe_ok:true with authenticated:false → still an authoritative logout", async () => { + const run = fakeRunner({ + code: 1, + stdout: JSON.stringify({ + gh_present: true, authenticated: false, probe_ok: true, user: null, + error: "You are not logged into any GitHub hosts. To log in, run: gh auth login", + }), + stderr: "", + }); + assert.deepEqual(await checkAuth(run), { + authenticated: false, cliPresent: true, ghPresent: true, probeOk: true, user: null, error: null, + }); + }); + + test("#485 probe_ok:true with authenticated:true is unaffected", async () => { + const run = fakeRunner({ + code: 0, + stdout: JSON.stringify({ + gh_present: true, authenticated: true, probe_ok: true, user: "eve", error: null, + }), + stderr: "", + }); + assert.deepEqual(await checkAuth(run), { + authenticated: true, cliPresent: true, ghPresent: true, probeOk: true, user: "eve", error: null, + }); + }); + + test("#485 older CLI (no probe_ok): a validation-failure error is still downgraded", async () => { + // Version skew — an installed CLI predating probe_ok. We can still recognise + // gh's own validation-failure wording in `error`, so the fix works without + // requiring a CLI upgrade first. + const run = fakeRunner({ + code: 1, + stdout: JSON.stringify({ + gh_present: true, authenticated: false, user: null, error: KEYRING_BLIP, + }), + stderr: "", + }); + assert.deepEqual(await checkAuth(run), { + authenticated: false, cliPresent: true, ghPresent: true, probeOk: false, user: null, + error: KEYRING_BLIP, + }); + }); + + test("#485 older CLI (no probe_ok): an explicit logout stays authoritative", async () => { + const run = fakeRunner({ + code: 1, + stdout: JSON.stringify({ + gh_present: true, authenticated: false, user: null, + error: "You are not logged into any GitHub hosts. To log in, run: gh auth login", + }), + stderr: "", + }); + const got = await checkAuth(run); + assert.equal(got.probeOk, true); + assert.equal(got.authenticated, false); + }); + + test("#485 older CLI (no probe_ok): a timeout reason is downgraded", async () => { + const run = fakeRunner({ + code: 1, + stdout: JSON.stringify({ + gh_present: true, authenticated: false, user: null, + error: "gh auth status failed: Command 'gh auth status' timed out after 30 seconds", + }), + stderr: "", + }); + assert.equal((await checkAuth(run)).probeOk, false); + }); + + test("#485 an authenticated probe is never downgraded by a stray error string", async () => { + const run = fakeRunner({ + code: 0, + stdout: JSON.stringify({ + gh_present: true, authenticated: true, user: "eve", error: "Failed to log in to ghe.corp", + }), + stderr: "", + }); + const got = await checkAuth(run); + assert.equal(got.authenticated, true); + assert.equal(got.probeOk, true); + assert.equal(got.error, null); + }); + + test("#485 gh genuinely missing stays authoritative even with probe_ok absent", async () => { + const run = fakeRunner({ + code: 2, + stdout: JSON.stringify({ + gh_present: false, authenticated: false, user: null, error: "gh CLI not found on PATH", + }), + stderr: "", + }); + const got = await checkAuth(run); + assert.equal(got.ghPresent, false); + assert.equal(got.probeOk, true); + }); + + test("#485 summariseAuthError picks the first line carrying prose, not the bare host", () => { + assert.equal( + summariseAuthError(KEYRING_BLIP), + " (Failed to log in to github.com account eve (keyring))", + ); + }); + + test("#485 summariseAuthError returns empty for nothing useful", () => { + assert.equal(summariseAuthError(null), ""); + assert.equal(summariseAuthError(""), ""); + assert.equal(summariseAuthError(" \n\n "), ""); + assert.equal(summariseAuthError("github.com"), ""); // single token, no prose + }); + + test("#485 summariseAuthError caps a runaway reason", () => { + const got = summariseAuthError(`x ${"y ".repeat(400)}`); + assert.ok(got.length < 160, `too long: ${got.length}`); + assert.ok(got.endsWith("…)")); + }); + + test("#485 summariseAuthError strips gh's gutter markers", () => { + assert.equal(summariseAuthError("- The token in keyring is invalid."), + " (The token in keyring is invalid.)"); + }); + test("CLI not found (ENOENT) → cliPresent:false, probeOk:false (#402, NOT a sign-in problem)", async () => { // The work-plan binary isn't on PATH — makeSpawnRunner rejects with a // CliError{notFound:true}. This must read as a missing CLI, not "not signed diff --git a/vscode/src/cli.ts b/vscode/src/cli.ts index 2fea467..1a84437 100644 --- a/vscode/src/cli.ts +++ b/vscode/src/cli.ts @@ -464,16 +464,20 @@ export async function checkVersion( * When `cliPresent` is false, `ghPresent` is unknown (we never reached gh) and * reported false so callers don't show a misleading gh-specific message. * - * `probeOk` signals whether the auth probe itself ran and returned a parseable, - * authoritative answer (`true`), or whether the probe errored / couldn't be - * trusted (`false` — transient). When `probeOk` is false the caller should keep - * the last-good tree rather than switching to an onboarding banner. + * `probeOk` signals whether the probe reached a TRUSTWORTHY verdict (`true`) or + * merely failed to reach one (`false`). Note this is about trust, not about + * parseability: a perfectly well-formed `authenticated:false` can still be + * untrustworthy, because `gh auth status` validates the token over the network + * and reports a momentary network failure as an invalid keyring token (#485). + * When `probeOk` is false the caller must keep the last-good tree and say + * "couldn't verify" — never "signed out", which it does not know. * - * `error` is a short human reason set ONLY when the probe ran but produced no - * trustworthy answer (`probeOk:false` with `cliPresent:true`) — typically the - * launcher's own stderr, e.g. "work-plan: missing required tool(s) on PATH: yq". - * It lets the caller say "the CLI couldn't run: " instead of the - * misleading "not signed in to GitHub". Null whenever there's nothing to add. */ + * `error` is a short human reason set ONLY when the answer isn't trustworthy + * (`probeOk:false` with `cliPresent:true`) — either gh's own diagnosis or the + * launcher's stderr, e.g. "work-plan: missing required tool(s) on PATH: yq". + * It lets the caller name the real problem instead of the misleading "not + * signed in to GitHub". Null whenever there's nothing to add; run it through + * `summariseAuthError` before putting it in a notification. */ export type AuthState = { authenticated: boolean; cliPresent: boolean; @@ -483,6 +487,43 @@ export type AuthState = { error: string | null; }; +/** gh's wording when it HAS credentials but couldn't confirm them — a live + * token-validation call that failed, or our own timeout wrapper. Used only as + * the back-compat fallback for a CLI that predates `probe_ok` (#485). Note gh + * says "the token ... is invalid" for a plain network failure, so this wording + * means "unverified", never "definitely logged out". */ +const INDETERMINATE_AUTH_RE = + /failed to log in|token .*is invalid|auth status failed|timed out|timeout|connect|network|temporarily unavailable/i; + +function looksIndeterminate(reason: string | null): boolean { + return reason !== null && INDETERMINATE_AUTH_RE.test(reason); +} + +/** Max chars of `AuthState.error` to inline in a notification. */ +const AUTH_ERROR_MAX = 140; + +/** + * Condenses an `AuthState.error` into a parenthesised clause for a one-line + * notification, or `""` when there's nothing useful to say (#485). + * + * `gh auth status` emits a multi-line report whose first line is a bare hostname + * and whose useful content is an indented, bulleted diagnosis. Pasting the whole + * blob into a toast is unreadable, and taking line 1 verbatim yields a message + * that just says "(github.com)". So: pick the first line that actually carries a + * sentence, strip gh's ✓/X/- gutter markers, and cap it. + */ +export function summariseAuthError(reason: string | null): string { + if (!reason) return ""; + const line = reason + .split("\n") + .map((l) => l.trim().replace(/^[-*✓✗×xX!]\s+/, "").trim()) + // "github.com" or "-" alone tells the user nothing; require real prose. + .find((l) => l.length > 0 && /\s/.test(l)); + if (!line) return ""; + const clipped = line.length > AUTH_ERROR_MAX ? `${line.slice(0, AUTH_ERROR_MAX - 1)}…` : line; + return ` (${clipped})`; +} + /** * Runs `auth-status --json` and reports whether `gh` is installed + signed in. * Never throws — auth detection must degrade gracefully, not break activation. @@ -516,15 +557,33 @@ export async function checkAuth(run: CliRunner): Promise { try { const blob = JSON.parse(result.stdout) as Partial<{ - authenticated: boolean; gh_present: boolean; user: string | null; + authenticated: boolean; gh_present: boolean; probe_ok: boolean; + user: string | null; error: string | null; }>; + const authenticated = Boolean(blob.authenticated); + const ghPresent = blob.gh_present !== false; // default true unless explicitly false + const reason = blob.error ?? null; + // A verdict is trustworthy when the CLI says so (`probe_ok`, #485). Older + // CLIs predating that field don't say — so fall back to recognising gh's own + // validation-failure wording in `error`, which lets the fix work against an + // already-installed CLI instead of waiting on a coordinated upgrade. Note the + // fallback is deliberately the INVERSE of the CLI's rule: there we trust only + // known-authoritative verdicts, here we downgrade only known-transient ones, + // so a CLI we can't interrogate keeps its historical behaviour. + const probeOk = + authenticated || !ghPresent + ? true + : blob.probe_ok ?? !looksIndeterminate(reason); return { - authenticated: Boolean(blob.authenticated), + authenticated, cliPresent: true, - ghPresent: blob.gh_present !== false, // default true unless explicitly false - probeOk: true, + ghPresent, + probeOk, user: blob.user ?? null, - error: null, + // Per the AuthState contract, `error` is populated only when the answer + // isn't trustworthy — that's the case where the caller must explain itself + // rather than render an onboarding banner. + error: probeOk ? null : reason, }; } catch { // The CLI ran (we reached this binary) but returned nothing parseable. That diff --git a/vscode/src/extension.ts b/vscode/src/extension.ts index 031f277..eebbeaa 100644 --- a/vscode/src/extension.ts +++ b/vscode/src/extension.ts @@ -1,12 +1,14 @@ import * as vscode from "vscode"; import * as fs from "node:fs"; import { - exportJson, listRepoOpenIssues, makeSpawnRunner, checkVersion, checkAuth, CliError, + exportJson, listRepoOpenIssues, makeSpawnRunner, checkVersion, checkAuth, summariseAuthError, CliError, isAlreadyExistsError, notesVcsStatus, notesVcsRun, notesVcsUndo, suggestNextUp, autoTriageScan, doctorScan, } from "./cli.ts"; import type { NotesVcsStatus, AuthState, DoctorFinding } from "./cli.ts"; +import { SNAPSHOT_KEY } from "./authCache.ts"; +import type { Snapshot, SnapshotStore } from "./authCache.ts"; import { buildDoctorStatus } from "./doctor.ts"; import { pickAutoFocusSlug } from "./autofocus.ts"; import { WorkPlanTreeProvider } from "./tree.ts"; @@ -50,9 +52,17 @@ export function activate(context: vscode.ExtensionContext): void { // `--include-archived` flag without reconstructing the provider. let showArchivedTracks = false; + // Last-good tree persistence (#485). globalState (not workspaceState) because + // the export spans every configured repo, not just the open folder. + const snapshotStore: SnapshotStore = { + get: () => context.globalState.get(SNAPSHOT_KEY), + set: (value) => void context.globalState.update(SNAPSHOT_KEY, value), + }; + const provider = new WorkPlanTreeProvider( () => exportJson(runner, showArchivedTracks), () => checkAuth(runner), + snapshotStore, ); void vscode.commands.executeCommand("setContext", "workPlanShowArchived", showArchivedTracks); @@ -667,11 +677,12 @@ export function activate(context: vscode.ExtensionContext): void { "Work Plan: GitHub CLI (gh) not found — install it, then Retry.", ); } else if (auth && !auth.probeOk) { - // Probe ran but gave no trustworthy answer — a CLI dependency/runtime - // problem, not a sign-in state. Don't claim "still not signed in". - const detail = auth.error ? ` (${auth.error})` : ""; + // Probe ran but gave no trustworthy answer — a transient network failure + // or a CLI dependency problem, not a sign-in state (#485). Never claim + // "still not signed in": we don't know that, and it's usually false. + const detail = summariseAuthError(auth.error); vscode.window.showWarningMessage( - `Work Plan: couldn't verify GitHub sign-in — the work-plan CLI didn't return a result${detail}. Check its dependencies (gh, git, yq), then Retry.`, + `Work Plan: couldn't verify GitHub sign-in — you have not been signed out${detail}. Usually transient (gh checks your token over the network); wait a moment and Retry.`, ); } else { vscode.window.showInformationMessage( @@ -4202,14 +4213,14 @@ function maybeShowAuthToast(auth: AuthState | null): void { if (c === "Install gh") void vscode.commands.executeCommand("workPlan.openGhInstallDocs"); }, () => { /* ignore */ }); } else if (!auth.probeOk) { - // The probe ran but returned no trustworthy answer — a CLI runtime / - // dependency problem (e.g. an older launcher gating the probe behind a - // missing yq), NOT a sign-in state. Surface the launcher's own reason and - // offer Retry instead of sending the user into a futile sign-in loop. - const detail = auth.error ? ` (${auth.error})` : ""; + // The probe ran but returned no trustworthy answer — most often a transient + // network failure (gh validates the token over the wire), sometimes a CLI + // dependency problem. NOT a sign-in state. Surface the real reason and offer + // Retry instead of sending the user into a futile sign-in loop (#485). + const detail = summariseAuthError(auth.error); vscode.window .showWarningMessage( - `Work Plan: couldn't verify GitHub sign-in — the work-plan CLI didn't return a result${detail}. Check its dependencies (gh, git, yq), then Retry.`, + `Work Plan: couldn't verify GitHub sign-in — you have not been signed out${detail}. Usually transient; wait a moment and Retry.`, "Retry", ) .then((c) => { diff --git a/vscode/src/tree.ts b/vscode/src/tree.ts index debced1..7434b7e 100644 --- a/vscode/src/tree.ts +++ b/vscode/src/tree.ts @@ -9,6 +9,8 @@ import { lensShouldApply } from "./autofocus.ts"; import type { LensSource } from "./autofocus.ts"; import type { AuthState } from "./cli.ts"; import { SingleFlight } from "./singleFlight.ts"; +import { fitsSizeCap, makeSnapshot, readSnapshot, shouldPersist } from "./authCache.ts"; +import type { SnapshotStore } from "./authCache.ts"; // Re-export the node types so extension.ts only needs to import from tree.ts. export type { RepoNode, TrackNode, UntrackedGroupNode, UntrackedIssueNode, EmptyRepoNode, FetchUntrackedNode, TierDupWarningNode, SuggestedGroupNode, SuggestedIssueNode, NeedsReviewGroupNode }; @@ -99,13 +101,76 @@ export class WorkPlanTreeProvider // the `workPlanGitHubAuthed` context key + lets activation show its one-time // toast off the same probe the tree already ran (no second `gh` call). private _lastAuth: AuthState | null = null; + // Epoch ms of the last last-good-snapshot write attempt (#485), null before + // the first. Throttles persistence; see _saveSnapshot. + private _lastSnapshotWriteAt: number | null = null; private readonly _refreshFlight: SingleFlight; constructor( private readonly load: () => Promise, private readonly checkAuth: () => Promise, + /** Optional last-good-tree persistence (#485). Omitted in tests and in any + * caller that doesn't want cross-reload caching. */ + private readonly snapshotStore?: SnapshotStore, ) { this._refreshFlight = new SingleFlight(() => this._doRefresh()); + this._hydrateFromSnapshot(); + } + + /** + * Seeds the tree from the persisted last-good export (#485) so a window reload + * during a network blip doesn't blank the view. The activation refresh + * overwrites this within a second on the happy path; the snapshot only + * actually SURVIVES when that refresh comes back untrustworthy. + * + * Deliberately does not fire the tree-data event — nothing is subscribed yet + * during construction, and the activation refresh fires one regardless. + */ + private _hydrateFromSnapshot(): void { + if (!this.snapshotStore) return; + const snap = readSnapshot(this.snapshotStore, Date.now()); + if (!snap) return; + this.cache = snap.export; + this._filteredCache = applyLens(this.cache, this._activeLens); + this.roots = this._applySortToRepos( + mergeStaleUntracked(buildTree(this._filteredCache), this._lastGoodUntrackedByRepo), + ); + // Restore the signed-in context key ONLY on a positive prior state, so the + // tree can render immediately instead of flashing an onboarding banner while + // the probe runs. A negative state is never resurrected — that would assert + // a logout we haven't confirmed this session. + if (snap.wasAuthenticated) { + void vscode.commands.executeCommand("setContext", "workPlanGitHubAuthed", true); + void vscode.commands.executeCommand("setContext", "workPlanConfigured", true); + void vscode.commands.executeCommand( + "setContext", "workPlanHasTracks", this.cache.tracks.length > 0, + ); + } + } + + /** Persists the current export as the last-good snapshot (#485). Throttled, + * because a real export runs to hundreds of KB and refreshes can be on a + * timer. Best-effort: a storage failure must never break a refresh that + * otherwise succeeded. */ + private _saveSnapshot(exp: Export, auth: AuthState): void { + if (!this.snapshotStore) return; + const now = Date.now(); + if (!shouldPersist(this._lastSnapshotWriteAt, now)) return; + // Stamp the attempt regardless of outcome, so an export that can't be + // persisted isn't re-serialised on every single refresh. + this._lastSnapshotWriteAt = now; + try { + const snap = makeSnapshot(exp, auth, now); + if (fitsSizeCap(snap)) { + this.snapshotStore.set(snap); + } else { + // Too big to keep current — drop any prior snapshot rather than leave a + // stale tree we've stopped updating. + this.snapshotStore.set(undefined); + } + } catch { + /* persistence is an optimisation, never a requirement */ + } } /** The most recent auth probe result (null before the first refresh). */ @@ -262,6 +327,12 @@ export class WorkPlanTreeProvider * b. Authoritative logged-out (probeOk true) OR no last-good → clear tree. * 3. Authenticated — run export; keep last-good on load failure. * + * 2a is the common case, not the exotic one (#485): `gh auth status` validates + * the token over the network, so every sleep/VPN blip lands here. It depends on + * a last-good tree existing, which is why the export is persisted across + * reloads (see _hydrateFromSnapshot) — without that, the first refresh after a + * window reload has no cache to protect and falls through to 2b. + * * viewsWelcome is driven off CONFIG state (repos present) not tracks.length, so a * configured-but-empty user never sees "No repos yet" onboarding (#398). */ @@ -322,6 +393,9 @@ export class WorkPlanTreeProvider } this.cache = loaded; + // Persist it as the last-good tree so a reload during a later network + // blip has something to fall back on (#485). + this._saveSnapshot(loaded, auth); // Update the per-repo last-good cache from every repo that fetched // successfully THIS round (export.py omits a failed repo from // `untracked` entirely, so every entry here is genuinely fresh data).