diff --git a/.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md b/.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md index f9b44d8..25bd40f 100644 --- a/.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md +++ b/.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md @@ -93,7 +93,18 @@ Bundled **node-tar** (no dependency entry — vendored). Guards found in `dist/s `dist/src-COTalb41.js` constructor: `… this.proxy = process.env.https_proxy, …` — unconditional, on the programmatic path pharn uses. `https-proxy-agent` is **vendored** into `dist/utils-DCX7uekb.js` (debug namespaces `https-proxy-agent:parse-proxy-response` / `:agent`, `secureProxy`, -`ALPNProtocols`). `Ao` logs `{code:'PROXY'}` and passes `t.proxy` into `t.fetch`. +`ALPNProtocols`). `Ao` passes `t.proxy` into `t.fetch`. + +> **Correction (2026-08-19, increment `proxy-env-notice`).** This entry originally read "`Ao` logs +> `{code:'PROXY'}` and passes `t.proxy` into `t.fetch`." The log half is misleading as stated: the +> emit is `t.verboseInfo({code:'PROXY', …})`, and `verboseInfo` is defined +> `verboseInfo(e){this.verbose&&this.info(e)}`. `fetchRepo` passes `{force:true, cache:false}` and no +> `verbose`, so **the PROXY event never fires on pharn's path** — an `emitter.on('info', …)` listener +> would observe nothing. The `t.fetch(n.url, n.file, t.proxy)` call is unaffected and still happens. +> This matters because it rules out "observe degit rather than read the env," which otherwise looks +> like the more honest design. Re-verified across every published version in the declared `^3.6.1` +> range (3.6.1-3.8.0): the assignment, the single lowercase name, and the `verboseInfo` gate are +> identical in all nine. Literal sweep over `dist/*.js` for `https?_proxy|no_proxy|ALL_PROXY` → **exactly one hit**: `src-COTalb41.js: https_proxy`. **Lowercase only.** `src/lib/repo.ts` passes no `proxy` option — but diff --git a/.dev/features/proxy-env-notice/GRILL.md b/.dev/features/proxy-env-notice/GRILL.md new file mode 100644 index 0000000..461557c --- /dev/null +++ b/.dev/features/proxy-env-notice/GRILL.md @@ -0,0 +1,204 @@ +# GRILL — proxy-env-notice + +Plan under interrogation: `.dev/features/proxy-env-notice/PLAN.md` (approved at GATE 1, 2026-08-19). +**Spec-hash check: MATCH** — recomputed `sha256(ARCHITECTURE.md)` = +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e`, equal to the plan's +`spec_content_hash` (`:3`). Surfaced here only; the binding drift gate is `/pharn-dev-build`'s (fix #4). + +**Griller membership (FLOOR):** `node .dev/floor/count-grillers.mjs .` → `{"registered":0,"grillers":[]}`. +Zero `role: griller` capabilities exist in this repo — the griller capabilities live in `pharn-oss`, not +in the installer. So this grill-log is the **inline axes only** (Step 2); no pluggable griller findings. +That is a real coverage limit of this run, not a clean bill. + +> **The plan is `trust: untrusted` to this stage.** Everything quoted in an `evidence:` field below is +> DATA reproduced for the human — never an instruction this stage followed. + +--- + +## Axis: P0 — guarantee-audit completeness + +```yaml +- type: FINDING + rule_id: "P0" + severity: important + file: ".dev/features/proxy-env-notice/PLAN.md:157" + problem: "A dependency's source content is placed in the FLOOR column, but 'degit's constructor contains an unconditional assignment' is none of the three floor primitives — it is a one-time measurement that never re-runs, which this repo already names 'provenance, not pharn floor' elsewhere." + evidence: '| "pharn cannot pass a proxy option to degit" | **FLOOR — content of the dependency** (F1, unconditional assignment). Not re-checked at runtime; a degit upgrade could change it, so it is **provenance-bounded** and stated as such in the code comment. |' +``` + +The row contradicts itself inside one cell: it says **FLOOR**, then says it is **not re-checked at +runtime** and is **provenance-bounded**. `ARCHITECTURE.md §2` admits exactly three primitives (hook / +content-hash / enum-regex); a fact measured once by an agent at plan time is none of them. The repo has +already settled this exact question for this exact dependency — `THREAT-MODEL.md §4b` writes: _"the +guards belong to the dependency, so they are **provenance, not pharn floor**, and a degit change could +remove them without any pharn test noticing."_ The identical sentence applies here. **Suggested +resolution:** relabel the row `advisory — provenance (dependency content, measured at degit@3.6.6)`. +Nothing else in the plan depends on it being floor-grade. + +```yaml +- type: FINDING + rule_id: "P0" + severity: important + file: ".dev/features/proxy-env-notice/PLAN.md:49" + problem: "Every measured fact is pinned to degit@3.6.6, but package.json declares the caret range ^3.6.1 and an npm consumer of @pharn-dev/pharn resolves that range fresh without pharn's lockfile — so the shipped warning can become factually wrong on a consumer machine, and no residual names this." + evidence: "| F1 | `this.proxy = process.env.https_proxy` is **unconditional** … | `grep -oE '.{300}https_proxy.{300}' node_modules/degit/dist/src-COTalb41.js` |" +``` + +**This is the sharpest finding in the run.** The whole `ignored` branch asserts a *negative* about a +third party — "the clone will **not** read `HTTPS_PROXY`" — and that assertion is baked into a user-facing +string. `package.json:53` declares `"degit": "^3.6.1"`. `package-lock.json` pins `3.6.6`, which governs +**this repo's CI**, but a lockfile is not published to consumers: `npx @pharn-dev/pharn` resolves +`^3.6.1` against the registry at install time. If any `3.x` release adds an uppercase read (or a +`no_proxy` honor), pharn would confidently tell a user their proxy is being ignored **while it is being +used** — a worse failure than today's silence, because today's silence at least does not assert +anything. Nothing in the guarantee audit (`:152-160`) covers this. + +Note the plan already contains the correct instinct one row down — it refuses to claim anything about +`fetch()` because it did not measure it (`:160`). The same discipline applied here yields either a +version-scoped message, a residual row, or a runtime read of the resolved degit version. **This is for +the human to weigh, not for this stage to decide.** + +## Axis: P1 — test coverage (evals) + +```yaml +- type: FINDING + rule_id: "P1" + severity: important + file: ".dev/features/proxy-env-notice/PLAN.md:79" + problem: "The plan declares five call sites across four commands but plans command-level tests for init only, so a regression that drops the notice from add, update, or status would stay green." + evidence: "- `tests/init.test.ts` — extend: notice printed before the fetch on the ignored-spelling env; silent on a clean env." +``` + +The pure detector is well covered — ten cases, one per truth-table row plus casing, redaction, and +purity (`:132-143`), and they **demonstrate** rather than assert existence, which is what P1 asks for. +The gap is the **wiring**, which is where this increment's actual value lives: a detector nobody calls +warns nobody. `add`'s picker site is the most exposed of the three untested ones — it sits after an +arg-parse exit, a non-TTY refusal, and (post-fetch) two gates, so it is the site most likely to be moved +or lost in a later refactor. `tests/add.test.ts`, `tests/update.test.ts`, and `tests/status.test.ts` all +already mock `fetchRepo` (`add.test.ts:23-24`, `update.test.ts:33-34`, `status.test.ts:13-14`), so the +marginal cost is one assertion each. `status` additionally has a case worth pinning that no other command +has: `--no-drift` must print **nothing**, since it never clones — the plan asserts that behavior at `:76` +but plans no test for it. + +## Axis: P5 — determinism / completeness of the branch table + +```yaml +- type: FINDING + rule_id: "P5" + severity: minor + file: ".dev/features/proxy-env-notice/PLAN.md:108" + problem: "On POSIX the ignored-spelling branch keys on the exact string HTTPS_PROXY, so any other case variant is met with silence — the same silent-footgun the increment exists to remove, in a narrower form." + evidence: "| 2 | unset | **set** | ≠ win32 | `{kind:'ignored'}` | **warn**: `HTTPS_PROXY` is set but the clone reads only lowercase `https_proxy` … |" +``` + +The truth table is otherwise **complete and non-overlapping** over its three inputs, and row 5 correctly +resolves the both-set case to `active` rather than a false `ignored` — the exact regression the filing +called out. The asymmetry is in the lookup rule at `:102`: win32 gets a **case-insensitive scan**, POSIX +gets **two exact keys**. A user with `Https_Proxy` set on POSIX (rarer than `HTTPS_PROXY`, but the same +mistake) falls to row 1 and is told nothing. Making the POSIX branch "any case-variant other than exact +lowercase is set, and exact lowercase is not" is the same one scan already being written for win32, and +makes the two platforms one rule with one flag rather than two rules. + +## Axis: P2 — trust propagation + +```yaml +- type: FINDING + rule_id: "P2" + severity: minor + file: ".dev/features/proxy-env-notice/PLAN.md:116" + problem: "Redaction is specified for credentials and for unparseable input, but the plan sets no length bound on the rendered value and does not state whether the ignored branch echoes its value at all." + evidence: "`redactProxyUrl` replaces any userinfo component with `***` (`http://u:p@h:3128` → `http://***@h:3128`) and, on an unparseable value, returns a fixed `\"(set)\"` rather than echoing raw bytes" +``` + +The trust audit (`:164-172`) is the strongest section of the plan — it correctly identifies +`process.env` as the untrusted input, keeps the value out of every branch (presence only), keeps it out +of `pharn.config.json`, and routes it through redaction. Two residual edges it does not close: + +1. **No length bound.** A 64KB `https_proxy` value is parseable by `new URL()` (a long host or path) and + would render in full. A cap (e.g. 120 chars, then `…`) costs one line and closes it. +2. **The ignored branch's value.** The row-2 message text at `:108` does not appear to echo the value, + which is correct — but the plan never says so, and `HTTPS_PROXY` is exactly as credential-bearing as + `https_proxy`. Worth making explicit so the build cannot "helpfully" add it. + +## Axis: P3 — one axis of change + +```yaml +- type: FINDING + rule_id: "P3" + severity: minor + file: ".dev/features/proxy-env-notice/PLAN.md:65" + problem: "One new file owns both the detection rule and the user-facing message strings, which change for different reasons — degit's behavior versus wording/UX — and the repo has an existing precedent for splitting exactly that." + evidence: "- `src/lib/proxy-env.ts` — NEW. Pure, I/O-free detection + message formatting. Exports `detectProxyNotice(env, platform)` → `ProxyNotice | null`, `proxyNoticeMessage(notice)` → `string`, and `redactProxyUrl(value)` → `string`." +``` + +Raised as a question, not an assertion — the repo supports **both** readings. Against colocation: +`src/lib/model-routing.ts` (validation) and `src/lib/model-routing-format.ts` (presentation) are split on +precisely this axis, and the plan's own closest analogue, `interactiveAllowed`, keeps the predicate in +`lib/` while each command owns its own message string. For colocation: three small pure functions over +one concept, all changing together in practice, and splitting a ~60-line module into two is its own kind +of noise. The plan's justification at `:67` — _"One axis: 'what does degit's proxy-env read mean here'"_ — +is a defensible framing of the two as one concept. **Flagged for the human; either answer is honest.** + +## Axis: P6 — discovery hygiene + +```yaml +- type: FINDING + rule_id: "P6" + severity: minor + file: ".dev/features/proxy-env-notice/PLAN.md:51" + problem: "The plan establishes that a standing artifact from a shipped increment is now known to be incomplete, but plans no correction to it, so the stale version remains the only copy a future reader finds by searching." + evidence: "**NEW — refines #98's FACT-TABLE H5 and the ship args.** The `PROXY` event is emitted by `verboseInfo` … so the `PROXY` event **never fires on pharn's path**." +``` + +F3 is genuinely good work — it kills an alternative design (`emitter.on('info', …)`) that would otherwise +look strictly more honest than reading the env, and it corrects the ship args' own framing. The concern is +durability: the correction currently lives only in *this* feature's plan. +`.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md:96` still reads _"`Ao` logs `{code:'PROXY'}`"_ +with no verbose gate, and that file is the cited evidence base for #98. No **trusted** doc is wrong — +`THREAT-MODEL.md` never mentions the PROXY event, so nothing hook-protected needs a human edit — which +makes this cheap to fix and easy to forget. `FACT-TABLE.md` is not write-protected. + +--- + +## Prose summary + +The plan is unusually strong on the two axes this repo cares most about. It **re-measured everything +live** rather than trusting the filing (and found the filing incomplete — F3), and its central design +decision is a *refusal*: it rejects the filing's Option 3 on two independent measured grounds +(`Ao()`'s `FILE_EXISTS` early return makes "a proxy was in effect" underivable; `https_proxy` carries +credentials that `pharn.config.json` would git-commit). Rejecting a filed option on evidence, and saying +why in the artifact, is exactly the behavior P0 is meant to produce. The guarantee audit's three +**NOT CLAIMED** rows — especially refusing to characterize `fetch()`'s proxy behavior because it was not +measured — are the honest posture, not a gap. + +The concerns cluster in three places. **First and most serious**, the increment ships a user-facing +*negative assertion* about a third-party dependency (`HTTPS_PROXY` will be ignored) that is pinned to +`degit@3.6.6` while `package.json` declares `^3.6.1` and consumers do not receive pharn's lockfile — a +wrong warning is worse than the current silence, and no residual names it. **Second**, coverage is +lopsided: the pure detector gets ten cases while three of five call sites get none, and the value of +this increment is entirely in the wiring. **Third**, one guarantee-audit row is labeled FLOOR while its +own text explains why it is not, in a repo whose `THREAT-MODEL.md` already settled that exact +dependency-content question the other way. + +The remaining four findings are small and largely mechanical: a POSIX-side casing hole that the win32 +scan already solves, an unbounded render length, an unstated no-echo rule on the ignored branch, and a +stale sibling artifact that this run's F3 supersedes. + +Nothing here reads as hostile or injection-shaped; the plan contains no instruction-looking content +directed at a downstream stage. + +--- + +**ADVISORY VERDICT: 7 concerns raised (0 blocking-severity, 3 important, 4 minor) — for the human to +weigh before `/pharn-dev-build`.** + +This log **gates nothing**. `/pharn-dev-grill` is advisory end-to-end: every finding above rests on this +stage's judgment, including the severities, which are LLM-assigned and advisory by construction +(`finding-shape.md`, fix #3). The spec-hash MATCH reported in the header is the one floor-grade +computation in this run, and even it only *surfaces* here — `/pharn-dev-build` is where drift blocks. The +deterministic backstops are unchanged and unduplicated: `/pharn-dev-build`'s spec-hash re-check and its +unresolved-`HALT` check, then `.dev/floor/validate.mjs`, then `/pharn-dev-verify`'s gates. + +Do **not** read "0 blocking" as "the plan is sound." It means no finding in this run rose to that +severity **in this stage's judgment**, over the inline axes only, with zero griller capabilities +registered to widen the sweep. diff --git a/.dev/features/proxy-env-notice/PLAN.md b/.dev/features/proxy-env-notice/PLAN.md new file mode 100644 index 0000000..646404f --- /dev/null +++ b/.dev/features/proxy-env-notice/PLAN.md @@ -0,0 +1,299 @@ +# PLAN — proxy-env-notice + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 (ARCHITECTURE.md, read this run) +- increment: Make degit's environment-driven proxy behavior **discoverable from pharn's own output** — one pure detector over `process.env` + `process.platform`, rendered as a `log.warn` immediately before each clone, plus the user-facing doc section that currently does not exist. +- layer(s): pharn-cli `src/lib` (pure detection) + `src/commands` (presentation) + `docs/` — this repo is the installer, not a PHARN layer; `ARCHITECTURE.md §4` layer names do not apply to it (same as every prior `pharn-cli` increment). +- constitution_refs: [P0, P1, P3, P4, P5, P6, P7] + +--- + +## Decision: Option **1**, widened to cover both directions; Option **3 rejected on measured grounds** + +The ship args offered three options. This plan picks **1 (detect and warn)** and extends it by one +branch to also name the *honored* case, because the filing's own harm statement is symmetric +("Neither case is currently discoverable from any pharn output"). It **rejects 3** and treats **2** as +already-done-where-it-can-be-done. Reasons, each grounded in a measurement made this run: + +**Why not Option 3 (record it in the config / install summary).** Two independent blockers. + +1. **It cannot be honestly worded as a record.** `Ao()` in `dist/src-COTalb41.js` reads: + `if(!t.cache){ try{ await se(n.file,ce.F_OK); …FILE_EXISTS…; return }catch{} … await t.fetch(n.url,n.file,t.proxy) }` + — when the SHA-named tarball is already in the shared cache, the function **returns before any + fetch**, so no proxy is used at all. A tar failure can also fall back to a spawned `git clone`, + which never receives `t.proxy`. So "a proxy was in effect" is **not derivable from the + environment**, and writing it into `pharn.config.json` would be a guarantee-shaped claim with no + floor reduction — a **P0 violation**, compounding `LIMITS.md §1c` (the config is an advisory + record). What *is* derivable is "degit will read this value", which is a statement about + configuration, not about the transport — and that belongs in transient console output, not in a + persisted provenance field. +2. **Credential leak.** `https_proxy` conventionally carries inline credentials + (`http://user:pass@host:3128`). `pharn.config.json` is written to the project root and is + git-committed. Recording the value would commit proxy credentials into the user's repository. + This alone disqualifies Option 3 as filed. (The same hazard forces **redaction** in the console + message this plan *does* add — see `redactProxyUrl` below.) + +**Why Option 2 is already done where an agent may do it.** `THREAT-MODEL.md` §2 and §4b already state +the lowercase-only read **with** the win32 correction, and `CHANGELOG.md` records it (#98). `LIMITS.md` +is on `DEFAULT_PROTECTED` in `.claude/hooks/protect-trusted-paths.cjs:58` — **the build agent cannot +write it** (fix #2); a `LIMITS.md` edit is a human action outside this loop. The doc gap that is both +real and agent-writable is **user-facing**: `grep -rn -i proxy docs/` returns **zero hits**. This plan +closes that, in `docs/troubleshooting.md`, next to the existing degit-cache section (`:114`) that set +the precedent for documenting degit's environment-level side effects. + +--- + +## Measured facts this plan rests on (all re-measured live this run, `degit@3.6.6`) + +| # | Fact | How measured | +| - | ---- | ------------ | +| F1 | `this.proxy = process.env.https_proxy` is **unconditional** — there is no `t.proxy ?? …` fallback, so **pharn cannot pass a proxy option**. The only lever is the env var itself. | `grep -oE '.{300}https_proxy.{300}' node_modules/degit/dist/src-COTalb41.js`; `src/degit.d.ts` correctly declares no `proxy` option | +| F2 | Exactly **one** proxy env name exists across every `dist/*.js` chunk: lowercase `https_proxy`. No `HTTPS_PROXY`, `no_proxy`, `NO_PROXY`, `ALL_PROXY`. | `grep -rnoE 'https?_proxy\|HTTPS?_PROXY\|no_proxy\|NO_PROXY\|ALL_PROXY\|all_proxy' node_modules/degit/dist/*.js` → 1 hit | +| F3 | **NEW — refines #98's FACT-TABLE H5 and the ship args.** The `PROXY` event is emitted by `verboseInfo`, defined `verboseInfo(e){this.verbose&&this.info(e)}`. `fetchRepo` passes `{force:true, cache:false}` — **no `verbose`** — so the `PROXY` event **never fires on pharn's path**. H5 and the ship args both say "`Ao()` logs `{code:'PROXY'}`" without the verbose gate; an `emitter.on('info', …)` listener would therefore observe **nothing**. | `grep -oE '.{120}this\.verbose.{200}' …src-COTalb41.js` | +| F4 | The proxy reaches only the **tarball download** (`t.fetch(n.url,n.file,t.proxy)`), inside `if(!t.cache)`, and **after** a `FILE_EXISTS` early return. The bundled `https-proxy-agent` lives in `dist/utils-DCX7uekb.js`. | same `Ao()` extract; `grep -roE '.{250}ProxyAgent.{250}'` | +| F5 | On win32 `process.env` is case-insensitive, so `process.env.https_proxy` resolves an uppercase `HTTPS_PROXY`. `package.json` sets `engines.node ">=20"` with **no** `os` restriction, so both behaviors are in the supported matrix. | Node documented behavior; `grep -n engines -A3 package.json` | + +**F3 is the load-bearing new fact:** it closes off the "just listen to the event" design, which would +otherwise look cheaper and more honest than an env read. Observation is unavailable without +`verbose: true`, which would also unleash `FILE_EXISTS` / `DOWNLOADING` / `EXTRACTING` / `FOUND_MATCH` +noise on every run. So an **env read is the only available signal**, and the plan is honest that this +is a statement about *configuration*, never about *the transport that ran*. + +--- + +## Files + +> Amended 2026-08-19 after `/pharn-dev-review` — see **Amendment** at the end. Entries added by that +> amendment are marked "(added by amendment)". + +- `src/lib/proxy-env.ts` — degit-proxy-behavior logic: the `ProxyNotice` truth table, the + measured-version membership set, and the one impure read (`resolveDegitProxyRead`). No message strings. +- `src/lib/proxy-env-format.ts` — presentation only: `proxyNoticeMessage` + `redactProxyUrl`. Splits the + axis the way `model-routing.ts` / `model-routing-format.ts` already do. (added by amendment — review finding P3) +- `src/commands/init.ts` — call site before the fetch spinner. +- `src/commands/add.ts` — call sites before each of the two fetch spinners. +- `src/commands/update.ts` — call site before `const s2 = spinner()`. +- `src/commands/status.ts` — call site inside the drift branch only. +- `tests/proxy-env.test.ts` — truth table, version membership, version read. +- `tests/proxy-env-format.test.ts` — message wording + redaction. (added by amendment) +- `tests/init.test.ts` — wiring + ordering. +- `tests/add.test.ts` — wiring at both add paths. (added by amendment — review finding P1) +- `tests/update.test.ts` — wiring. (added by amendment — review finding P1) +- `tests/status.test.ts` — wiring, and silence under `--no-drift`. (added by amendment — review finding P1) +- `docs/troubleshooting.md` — "Proxy environment variables"; the false pin claim removed. +- `.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md` — H5 corrected: the `PROXY` event is + `verboseInfo`-gated and never fires on pharn's path. (added by amendment — review finding P6) +- `CHANGELOG.md` — `[Unreleased]` entry. +- `src/lib/repo.ts` — version-scope the degit comments to the measured range. (added by amendment 2 — review finding P0, minor) +- `package-lock.json` — bump the dev/CI degit to the version consumers actually resolve. (added by amendment 2 — review finding P7, minor) + +**Not touched:** `src/degit.d.ts`, `pharn.config.json`'s schema, `LIMITS.md` / `THREAT-MODEL.md` +(hook-protected, human-only), and `package.json`'s `^3.6.1` **range** — the published contract is +unchanged; only the lockfile moves. + +### Why not in `fetchRepo` (P3, and one concrete bug it avoids) + +`repo.ts` is the only fetch-path module with **zero** UI imports, and every one of the five call sites +wraps `await fetchRepo()` in an already-started clack `spinner()`. A `log.warn` from inside `fetchRepo` +would render **into the live spinner frame** and be overwritten. Returning the notice as a field on +`FetchedRepo` was also rejected: it would tie the notice's lifetime to a **successful** clone, so a +clone that fails *because of* the misconfigured proxy — the single most valuable moment to say this — +would print nothing. + +--- + +## Behavior (the truth table `detectProxyNotice` implements) + +Lookup rule: on `win32`, scan the env's keys **case-insensitively** for `https_proxy` (modelling Node's +real win32 `process.env`); on every other platform, exact-key lookup only. Empty string counts as unset. + +| # | `https_proxy` | `HTTPS_PROXY` | platform | result | rendered as | +| - | ------------- | ------------- | -------- | ------ | ----------- | +| 1 | unset | unset | any | `null` | nothing | +| 2 | unset | **set** | ≠ win32 | `{kind:'ignored'}` | **warn**: `HTTPS_PROXY` is set but the clone reads only lowercase `https_proxy`, so it will connect **directly**; set `https_proxy` to the same value if you meant to proxy. | +| 3 | unset | **set** | win32 | `{kind:'active', value}` | notice (row 4 wording) — win32 resolves it | +| 4 | **set** | either | any | `{kind:'active', value}` | **warn**: the clone **may be routed** through ``; degit reads no `no_proxy`/`NO_PROXY`, so proxy exclusions do **not** apply to it. | +| 5 | set, equal to `HTTPS_PROXY` | set | ≠ win32 | `{kind:'active', value}` | row 4 — correctly *not* an "ignored" warning | + +**"may be routed", never "was routed"** — F4: a cached tarball short-circuits before the fetch, and a +tar failure degrades to `git clone`, which never receives the proxy. The wording is the P0 label. + +`redactProxyUrl` replaces any userinfo component with `***` (`http://u:p@h:3128` → `http://***@h:3128`) +and, on an unparseable value, returns a fixed `"(set)"` rather than echoing raw bytes — the env is +attacker-influencable (`THREAT-MODEL.md §4b`) and its value must not be echoed verbatim into a terminal. + +--- + +## Contracts satisfied + +- No `pharn-contracts` schema is touched. This increment adds **no** Capability, **no** `rule_id`, and + **no** finding — it is installer behavior, so the finding-shape / capability-frontmatter contracts do + not apply (cited, not restated — P4). +- `THREAT-MODEL.md §4b` "The environment reaches the fetch" — this increment gives that named residual + its first *user-visible* surface. The residual is **not closed** and this plan does not claim it is. + +## Evals to write (P1) + +`tests/proxy-env.test.ts` — one case per truth-table row, each **demonstrating** behavior, not asserting existence: + +- row 1 → `detectProxyNotice({}, 'linux')` === `null`; also `{https_proxy:''}` → `null` (empty ≠ set) +- row 2 → `{HTTPS_PROXY:'http://p:3128'}`, `'linux'` → `kind:'ignored'`; message contains both spellings and the word `directly` +- row 2 (darwin) → same input, `'darwin'` → `kind:'ignored'` (pins that the POSIX branch is platform-family-wide, not linux-only) +- row 3 → `{HTTPS_PROXY:'http://p:3128'}`, `'win32'` → `kind:'active'` **and NOT `'ignored'`** — the false-warning regression the ship args explicitly flag +- row 3 (win32 casing) → `{HtTpS_PrOxY:'http://p:3128'}`, `'win32'` → `kind:'active'` (case-insensitive scan, not a two-name special case) +- row 4 → `{https_proxy:'http://p:3128'}`, `'linux'` → `kind:'active'`; message contains `may be routed` and `no_proxy` +- row 5 → both set to the same value, `'linux'` → `kind:'active'`, never `'ignored'` +- redaction → `http://user:secret@h:3128` → message contains `***`, and **does not contain** `secret` +- redaction fallback → `'not a url'` → message contains `(set)` and not the raw value +- purity → calling the detector does not read `process.env` (assert the injected record drives the result: `detectProxyNotice({}, 'linux')` is `null` even while the real `process.env.https_proxy` is stubbed set) + +`tests/init.test.ts` — two cases: + +- ignored-spelling env → `log.warn` called with the notice **before** `fetchRepo` resolves (ordering pinned, per the "clone that fails because of the proxy" rationale) +- clean env → no proxy `log.warn` at all (silence on the common path) + +## Guarantee audit (P0) + +| claim | reduction | +| ----- | --------- | +| "Given `(env, platform)`, `detectProxyNotice` returns exactly the truth-table row" | **FLOOR — enum/presence membership test** (`ARCHITECTURE.md §2` primitive 3). Pure function, no I/O, one test per row (P1). | +| "The notice is emitted before the clone is attempted" | **FLOOR — test-pinned call ordering** in `tests/init.test.ts`. | +| "A proxy URL's credentials are never echoed to the terminal" | **FLOOR — regex/parse-based redaction** with a fixed `"(set)"` fallback, tested by a negative assertion (the secret is absent from the output). | +| "pharn cannot pass a proxy option to degit" | **ADVISORY — provenance** (dependency content, measured across degit 3.6.1-3.8.0). Corrected from an earlier "FLOOR" label: a fact measured once by hand is none of the three floor primitives, and `THREAT-MODEL.md` §4b already settles this for this dependency ("the guards belong to the dependency, so they are **provenance, not pharn floor**"). | +| "The confident `ignored` warning fires only on a degit version whose lowercase-only read was actually measured" | **FLOOR — enum membership** (`MEASURED_DEGIT_VERSIONS`, exact-string set of the 9 published versions in the `^3.6.1` range). This is the review's blocking finding converted into a real membership test rather than a prose caveat. | +| "You are being proxied / not being proxied" | **NOT CLAIMED — would be advisory with no floor.** F4 (cache short-circuit, `git clone` fallback) makes it underivable. The message says **"may be routed"** and the docs say so explicitly. | +| "Setting `https_proxy` makes the clone safe/observable" | **NOT CLAIMED.** `THREAT-MODEL.md §4b` residual stands unchanged; this increment adds visibility, not a guarantee. | +| "pharn's own `fetch()` calls honor / ignore the proxy" | **NOT CLAIMED — deliberately unmeasured this run.** Node's global fetch and proxy env is a separate axis; asserting it without measurement is exactly the "true-sounding, wrong sentence" the #98 FACT-TABLE warns about. See Open question Q2. | + +## Trust audit (P2) + +The ingested untrusted input is **`process.env`** — attacker-influencable per `THREAT-MODEL.md §4b` +("an attacker-controlled environment can interpose on the clone"). Taint propagation: + +- The env **value** is untrusted free text. It reaches exactly one sink: a terminal string. It is + **never** a branch input beyond `set`/`unset` (P5), **never** written to `pharn.config.json`, never a + path, and never passed to a shell. +- It is **redacted before rendering** (`redactProxyUrl`), and an unparseable value degrades to the fixed + literal `"(set)"` — so hostile bytes (ANSI escapes, control chars, a fabricated "error" line) cannot be + echoed verbatim to spoof pharn's own output. +- The **presence/absence** booleans and `platform` are floor-verifiable (membership) and are the only + things any branch reads — mirroring `ARCHITECTURE.md §8`'s enum-gated / tainted-free-text split. + +## Determinism audit (P5) + +Every branch is a presence or membership test: `platform === 'win32'`, and `value !== undefined && value !== ''`. +There is no classification, no heuristic, and no network read. There is **no fallback that ends in a +guess**: the terminal case (row 1, nothing set) is **silence**, which is the correct and complete +answer, not a degraded one. + +--- + +## Open questions (HALT) — RESOLVED at the plan gate (2026-08-19) + +Both were answered by the human at GATE 1; no plan content changed (both answers matched the +recommendation). Recorded here so `/pharn-dev-build` inherits no unresolved HALT. + +- **Q1 — scope of the "active" branch (rows 3-5).** The filing scoped detect-and-warn to the *ignored* + case only; this plan also warns on an *honored* proxy (may-be-routed + `no_proxy` does not apply), + at the cost of a line on every clone for a correctly-configured proxy user. + → **RESOLVED: keep BOTH branches.** The filing's own harm statement is symmetric ("neither case is + currently discoverable from any pharn output"), and the `no_proxy` fact has no other surface. + The full truth table above ships as written. +- **Q2 — do pharn's own `fetch()` calls honor the proxy?** Not measured this run, deliberately. + → **RESOLVED: out of scope.** No claim is made about `fetchCommitSha` / + `fetchRemoteSkillsVersion`; the doc section covers **only** the degit clone. The guarantee-audit row + "pharn's own `fetch()` honors / ignores the proxy → NOT CLAIMED (deliberately unmeasured)" stands as + the honest label. If the asymmetry matters it is a separate increment with its own measurement. + +## Approval + +Approved **as written** at the plan gate on 2026-08-19 (human, `/pharn-dev-ship` GATE 1). + +--- + +## Amendment — 2026-08-19, after `/pharn-dev-review` + +The first build of this plan passed every floor verdict (`validate` 0, `no-regressions`, `PASS`) and +`/pharn-dev-review` still returned **1 blocking finding**: `docs/troubleshooting.md` claimed +`degit@3.6.6` was "the version this release resolves." Re-measured live, that was worse than stale — +**degit's latest is 3.8.0**, `package.json` declares `^3.6.1`, `files: ["dist"]` ships no lockfile, and +`scripts/build.mjs:15` marks degit `external`. A consumer installing today gets **3.8.0**, not 3.6.6. +The measurement had never been wrong; the **scope of the pin** was. + +### What the amendment changes + +1. **A real version gate replaces the prose caveat (the blocking fix).** Every published version in the + declared range was swept — **3.6.1, 3.6.2, 3.6.3, 3.6.4, 3.6.5, 3.6.6, 3.7.0, 3.7.1, 3.8.0** — and + all nine show exactly one proxy env name (lowercase `https_proxy`), the same unconditional + `this.proxy=process.env.https_proxy`, and the same `verboseInfo(e){this.verbose&&this.info(e)}` gate. + Those nine become `MEASURED_DEGIT_VERSIONS`, an exact-string set. `resolveDegitProxyRead()` reads the + installed version at runtime (`createRequire(import.meta.url)('degit/package.json')`, verified + reachable — degit declares no `exports` field) and the confident wording fires **only** on set + membership. An unmeasured or unreadable version degrades to a hedged message naming both the measured + range and what is actually installed. The claim now reduces to the floor (enum membership) instead of + resting on a hand measurement, and a future degit release degrades the message rather than falsifying it. +2. **All three remaining call sites get wiring tests** (review finding P1): `add` (both paths), `update`, + `status` — plus the `--no-drift` silence, previously asserted only in a comment. +3. **The POSIX casing hole closes** (review finding P5). Detection now scans case-insensitively on + **both** platform families; on POSIX a variant that is not exactly lowercase yields `ignored`, and the + notice names **the variable actually found** rather than assuming `HTTPS_PROXY`. Multiple variants + resolve deterministically (prefer `HTTPS_PROXY`, else the lexicographically first) so the output does + not depend on env insertion order (P5). The echoed name is safe by construction: only a key whose + lowercase equals `https_proxy` can be returned, so it is one of 2^11 ASCII spellings and cannot carry + a control character. +4. **Presentation splits out** into `proxy-env-format.ts` (review finding P3). +5. **The guarantee-audit FLOOR mislabel is corrected** to `ADVISORY — provenance` (review finding P0). +6. **`FACT-TABLE.md` H5 is corrected** (review finding P6). + +### What it deliberately does NOT change + +The rejection of the filing's Option 3 stands, on the same two measured grounds. `LIMITS.md` and +`THREAT-MODEL.md` remain untouched (hook-protected, human-only) — and §4b's existing sentence is still +accurate, since it never named a version. `package.json`'s `^3.6.1` range is **not** narrowed: pinning +the dependency is a separate decision with its own trade-offs, and this increment's job is to stop +*claiming* a pin that does not exist, not to create one. Worth surfacing for the human, though: pharn's +lockfile sits at 3.6.6 while 3.8.0 is current. + +--- + +## Amendment 2 — 2026-08-19, closing the two findings left open at the post-review gate + +`/pharn-dev-review` run 2 returned GREEN with two advisory findings recorded as **out of scope**, and +recommended a follow-up increment. The human elected to close them here instead. This amendment +records the scope expansion and the reasoning, rather than letting two files appear in a diff +un-declared. + +### Why bundling is defensible here (the P3/P7 tension, argued not waved) + +Run 2's finding said folding `repo.ts` in "would bundle two increments," because the proxy read and +the cache/ref-tier/tar claims are different **measurement axes**. That is true of the axes and false +of the **defect**: both are version-scoped claims about `degit` written without version scoping, and +both are falsified by the same fact (the declared range resolves past what this repo develops +against). Fixing one and leaving the other would leave the root cause live in the file the increment +already cites. The axes stay separate in the code; only the correction is shared. + +### What amendment 2 changes + +1. **`src/lib/repo.ts` comments are re-scoped, not rewritten.** Each claim they make was re-measured + against **3.8.0**, the version `^3.6.1` resolves to today, and all still hold: the tarball download + still sits inside `if(!t.cache)` behind a `FILE_EXISTS` early return; the cache dir still resolves + `%LOCALAPPDATA% ?? ~/AppData/Local` on win32, `~/Library/Caches` on darwin, `$XDG_CACHE_HOME ?? + ~/.cache` elsewhere; `map.json` / `access.json` / `USING_CACHE` / `TAR_BAD_ARCHIVE` are all present; + the three ref-resolution tiers and `GIT_LS_REMOTE_FAILED` are intact. So the comments were never + **wrong** — they were pinned to a version nobody runs. They now name the measured **range**. +2. **The lockfile moves to 3.8.0.** The published contract (`package.json`'s `^3.6.1`) is deliberately + **unchanged** — narrowing a dependency range is a maintainer decision with its own trade-offs, and + this increment's job is to stop *claiming* a pin, not to create one. What moves is the version CI + installs, so the version tested is the version users resolve. This also points the new tripwire in + `tests/proxy-env.test.ts` at 3.8.0, meaning the claim is re-derived on every CI run from the bytes + consumers actually get. + + API compatibility verified live before the bump: `degit(src, opts)` is still a callable default + export, `.clone()` / `.on()` are present, `this.proxy` still reads from the environment, there are + still no runtime dependencies, and `engines.node` is `>=20.0.0` against pharn's `>=20`. + +### What it still does not change + +`package.json`'s range, `src/degit.d.ts` (there is still no `proxy` option to declare), the `cache` +option, the cache dir, and the absence of a clone timeout / body cap. `THREAT-MODEL.md` §4b's labeled +limit stands untouched and remains accurate — it never named a version. + diff --git a/.dev/features/proxy-env-notice/REGRESSION.md b/.dev/features/proxy-env-notice/REGRESSION.md new file mode 100644 index 0000000..574649e --- /dev/null +++ b/.dev/features/proxy-env-notice/REGRESSION.md @@ -0,0 +1,96 @@ +# REGRESSION — proxy-env-notice + +**Run 3** — after amendment 2 (`repo.ts` re-scoped, lockfile bumped to degit 3.8.0). + +**Run 2** — after the `/pharn-dev-review` fix pass. (Run 1's report is superseded; its two +self-inflicted reds are preserved below, because a stage that quietly drops its own mistakes is worse +than one that carries them.) + +**Base:** `03160c8e71e14a125f7dd3d107a42f18952a6f15` (`03160c8`, tip of `main`), resolved by the +deterministic state test (P5): `git status --porcelain` non-empty → working-tree dogfood → `base = HEAD`. + +## Scope partition (fix #7) + +`scope` exit **0**, `escaped: []`, and `inside == declared` — **seventeen** files each, byte-for-byte the +amended plan's `## Files`. + +| inside (= declared, 15) | | +| --- | --- | +| `src/lib/proxy-env.ts` | `tests/proxy-env.test.ts` | +| `src/lib/proxy-env-format.ts` | `tests/proxy-env-format.test.ts` | +| `src/commands/init.ts` | `tests/init.test.ts` | +| `src/commands/add.ts` | `tests/add.test.ts` | +| `src/commands/update.ts` | `tests/update.test.ts` | +| `src/commands/status.ts` | `tests/status.test.ts` | +| `docs/troubleshooting.md` | `.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md` | +| `CHANGELOG.md` | | + +The plan grew from 9 declared paths to 15, then to 17 to accommodate the review fixes, and the scope setter was +re-run against the amended plan **before** any of the new files were written — the documented +procedure, never a hook bypass. One detail worth recording: the first amendment marked new entries +with a `**+**` prefix, which `set-writes-scope.cjs`'s `isPathItem` pattern +(`/^\s*-\s+\`[^\`]+\`/`) does not match, so it silently scoped only 9 of 15. The **plan format was +corrected to match the parser**, not the parser loosened. + +### Paths excluded from `--changed`, with provenance + +Same `base = HEAD` artifact as run 1: on a working-tree dogfood, `git diff --name-only HEAD` sweeps +every stage's artifacts and any pre-existing state, not only the build's writes. + +| path | evidence | author | +| ---- | -------- | ------ | +| `.gitignore` | staged (`+prompts`), empty worktree diff; present as `M` in the session's opening `git status` | pre-existing | +| `.dev/features/proxy-env-notice/**` | each stage's own declared `writes:` (`pharn-dev-plan.md:8`, `pharn-dev-grill.md:15`, and the regress/verify/review/ship equivalents) | the pipeline stages themselves | + +Nothing under `src/`, `tests/`, or `docs/` was excluded. + +## Gate set + +Identical gate-ids both sides, decided once. + +- **`tests`** — `git ls-files '*.test.mjs' '*.test.cjs' | xargs node --test`, 46 outside floor/hook tests. +- **`validate`** — `node .dev/floor/validate.mjs .` (whole-repo; the named granularity limit). +- **`structural:*`** — none (`outside_eval_pairs` empty). +- **Style gates — SKIPPED** by the deterministic config-touch rule: `inside` touches none of + `eslint.config.mjs`, `.prettierrc.json`, `.prettierignore`, `.markdownlint-cli2.jsonc`. Absent from + both maps, so no gate-set mismatch; the baseline `npm ci` cost was not incurred. + +## Per-gate exit codes + +| gate | base (`03160c8`) | head | flip | +| ---- | ---------------- | ---- | ---- | +| `tests` | 0 | 0 | none | +| `validate` | 0 | 0 | none | + +`regressions[]`: **empty**. `pre_existing[]`: **empty**. + +## Two self-inflicted reds from run 1, retained + +1. **`scope` exit 1 on three "escapes"** — resolved by provenance, as above. Not a build escape. +2. **A false `tests = 1` at both ends** — the capture used `node --test $TESTS` under **zsh**, which + (unlike bash) does not word-split an unquoted expansion, so 46 paths arrived as one argument and + `node` failed with `Could not find ''`. Re-run through `xargs`: exit 0. Both runs + above use the corrected form. "pre-existing" is precisely the label under which a self-inflicted + failure would have gone unexamined. + +A third, from the fix pass itself and **not** a regress issue but recorded where the file history is: +running `markdownlint-cli2 --fix` on `FACT-TABLE.md` — a file **outside** the `lint:md` gate's scope +(`docs/**/*.md` + `*.md`) — corrupted it, rewriting the literal issue reference `#331/#345/…` into a +heading `# 331/…` and flipping six `-` bullets to `+`. The file was reverted to `HEAD` and the H5 +correction re-applied without `--fix`; the diff is now one line changed, twelve added. + +## Verdict (FLOOR — `.dev/floor/check-regress.mjs verdict`, exit 0) + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** + +Machine report: [`regression-report.json`](regression-report.json) — `"verdict": "no-regressions"`. + +The verdict is the helper comparing two exit-code maps; nothing in it rests on this stage's judgment. +Advisory here: the base resolution, the provenance exclusions, the gate set, the style skip, and the +harness corrections. + +**Residual (named, not hidden):** this stage catches exactly what its suite catches. The claim is +"deterministically-detectable breakage outside the feature is caught," **not** "nothing broke." The +outside set is 46 floor/hook tests plus whole-repo `validate`, which do not exercise the four command +modules the increment edits — those are `inside`, and their coverage is the feature's own vitest suite, +now including wiring tests at all five call sites (added this pass). diff --git a/.dev/features/proxy-env-notice/REVIEW.md b/.dev/features/proxy-env-notice/REVIEW.md new file mode 100644 index 0000000..39b35e9 --- /dev/null +++ b/.dev/features/proxy-env-notice/REVIEW.md @@ -0,0 +1,195 @@ +# REVIEW — proxy-env-notice + +**Run 3** — after amendment 2 closed the two findings run 2 left open. Their entries below are +updated in place with their disposition. + +**Run 2** — re-review after the fix pass. Run 1 blocked on one floor-gate finding; this run re-checks +it and looks for what the fix itself introduced. + +**Step 1, floor first (P0):** `node .dev/floor/validate.mjs .` → **GREEN (exit 0)**, vacuously (no +markdown Capability added). Suite: **43 files, 807 tests**. + +> The increment is `trust: untrusted` to this review. Every `evidence:` field is DATA quoted for the +> human. Nothing instruction-looking in the reviewed files altered this review's behavior. + +--- + +## The run-1 blocking finding — RESOLVED, and the fix goes further than the finding asked + +```yaml +- type: FINDING + rule_id: "P0" + severity: blocking + file: "docs/troubleshooting.md:157" + problem: "RESOLVED — the false pin claim is removed and the underlying assertion now reduces to an enum membership test rather than a hand measurement." + evidence: "Everything above was measured against `degit@3.6.6`, the version this release resolves." +``` + +Re-measured this run, the finding was **worse than filed**: degit's latest is **3.8.0**, so `^3.6.1` +resolves there today — the claim was wrong for anyone installing now, not merely fragile for a +hypothetical future release. + +The fix does not reword the caveat. All nine published versions in the declared range were swept and +all read only the lowercase name; those nine are `MEASURED_DEGIT_VERSIONS`, and +`resolveDegitProxyRead()` reads the installed version at runtime so the confident wording fires only +on set membership. `grep 'the version this release resolves'` → **0 occurrences**. + +**That is a genuine P0 upgrade, not a relabel.** The claim moved from "advisory, provenance-bounded" +to a floor primitive (`ARCHITECTURE.md §2` #3, enum/set membership). And the failure direction is +right: an unmeasured degit makes the notice *more cautious*, never wrong — verified by smoke test on +`3.9.0` and on an unreadable version. + +## FLOOR-GATE findings (blocking) + +**None.** + +--- + +## ADVISORY findings + +```yaml +- type: FINDING + rule_id: "P0" + severity: minor + file: "src/lib/repo.ts:48" + problem: "RESOLVED — every degit claim in the file was re-measured against 3.8.0 and re-scoped from a single version to the measured range, with one note covering them all." + evidence: "// `cache: false` is NOT no-cache — measured against degit@3.6.6, it selects" +``` + +**Disposition (amendment 2).** Each claim was re-measured at **3.8.0**, the version `^3.6.1` resolves +to today, and **all still hold**: the tarball download still sits inside `if(!t.cache)` behind a +`FILE_EXISTS` early return; the cache dir still resolves `%LOCALAPPDATA% ?? ~/AppData/Local` (win32), +`~/Library/Caches` (darwin), `$XDG_CACHE_HOME ?? ~/.cache` (else); `map.json`, `access.json`, +`USING_CACHE`, `TAR_BAD_ARCHIVE` and the three ref tiers with `GIT_LS_REMOTE_FAILED` are all present. +So the comments were never **wrong** — they were pinned to a version nobody runs. `repo.ts` now +carries **one** version-scope note covering ref tiers, cache, and warn sites, naming the measured +**range** and labeling them ADVISORY / provenance-bounded. `grep 'degit@3.6.6' src/lib/repo.ts` → none. + +Named rather than fixed, deliberately: `repo.ts` is on the plan's **Not touched** list, and the +cache/tier/tar claims there are a different measurement axis than the proxy one — folding them in +would bundle two increments (P7). The harm differs by an order of magnitude: a stale code comment +misleads a maintainer who can read the adjacent code, where run 1's defect misled a user who cannot. +`CHANGELOG.md:46` has the same shape and is **correct as written** — it says what *was measured* in +#98, past tense, which is a historical statement rather than a claim about what resolves. + +**Recommend:** a follow-up increment re-measuring `repo.ts`'s degit comments against 3.8.0, since the +range now demonstrably floats past what they describe. + +```yaml +- type: FINDING + rule_id: "P7" + severity: minor + file: "package.json:53" + problem: "pharn develops against degit 3.6.6 while the declared range resolves to 3.8.0 for anyone installing fresh, so the version CI exercises is two minors behind the one users get." + evidence: '"degit": "^3.6.1",' +``` + +**Disposition (amendment 2).** Closed on the side that carries the risk, left alone on the side that +is a contract. The **lockfile** moves to `3.8.0` so the version CI exercises is the version users +resolve; `package.json`'s **range stays `^3.6.1`**, because narrowing a published dependency range is +a maintainer decision and this increment's job was to stop *claiming* a pin, not to create one. + +Two consequences worth naming. First, the tripwire in `tests/proxy-env.test.ts` now re-derives the +proxy claim from **3.8.0's** bytes on every run — the strongest form this guarantee has taken, since +it is checked against what consumers get rather than a stale dev pin. Second, API compatibility was +verified **before** the bump, not assumed: same callable default export, `.clone()` / `.on()` intact, +`this.proxy` still env-read, still zero runtime dependencies, `engines.node >=20.0.0` against pharn's +`>=20`. + +Residual, unchanged: the range still *permits* a future release past what was measured. That is now +handled by degradation rather than silence — an unmeasured version hedges the notice — which is the +best available answer short of pinning. + +## What the fix pass itself introduced — checked, and one thing caught + +The new tripwire (`tests/proxy-env.test.ts`, "the installed degit still behaves as +MEASURED_DEGIT_VERSIONS claims") re-derives the claim from `node_modules/degit/dist/*.js` on every +run, so the hand-maintained set cannot silently drift from the dependency it describes. **It was +confirmed to be a real tripwire, not a tautology**: mutating the expected name list to +`['https_proxy','HTTPS_PROXY']` turns it RED, and reverting restores green. It also asserts the dist +is non-empty first, so a future packaging change fails loudly instead of passing vacuously. + +One defect was introduced during the pass and corrected before the floor ran: `markdownlint-cli2 +--fix` was run on `.dev/features/degit-fetch-boundary-truth/FACT-TABLE.md`, a file **outside** the +`lint:md` gate's scope (`docs/**/*.md` + `*.md`). It rewrote the literal issue reference `#331/#345/…` +into an H1 heading `# 331/…` and flipped six `-` bullets to `+`. The file was reverted to `HEAD` and +the H5 correction re-applied without `--fix`; the diff is now one line changed, twelve added. Recorded +because the corruption was silent and would have shipped. + +## L-eval (P1) — the run-1 gap is closed + +Run 1's important finding was three of five call sites untested. All five now have wiring tests, and +the two hardest cases are covered: `add`'s **picker** path (behind an arg check and a non-TTY refusal) +and `status --no-drift` **silence**, which was previously asserted only in a code comment. Each +command also pins a no-clone path staying silent, so the notice cannot leak onto a path with no +transport to describe. `validate.mjs` agrees (vacuous, nothing to bind) — no disagreement finding. + +## L-trust (P2) — re-checked, plus one property the fix strengthened + +- **Presence-only branching** holds; the value never drives a decision, never reaches + `pharn.config.json`, a path, or a shell. +- **Control characters cannot survive the render** — re-verified empirically, and now *tested* + (`tests/proxy-env-format.test.ts`, "never emits a control character from a parseable value"). +- **New surface, checked:** the `ignored` branch now echoes an environment **variable name**. That is + safe by construction, not by sanitization — only a key whose lowercase equals `https_proxy` can + reach it, so it is one of 2^11 ASCII spellings and cannot carry a control character. The code says + so at the type definition rather than leaving it implicit. +- **New surface, checked:** `readDegitVersion()` reads from `node_modules`. It validates against a + semver-shaped regex before the value can be echoed, and every failure mode collapses to `null`. + +## L-axis (P3) — the run-1 finding is resolved + +Presentation split into `proxy-env-format.ts`, which imports only a type and a constant from +`proxy-env.ts` — matching the `model-routing.ts` / `model-routing-format.ts` precedent. +`grep -rn "from '../commands/" src/` → nothing; no sibling import. + +--- + +## Verdict + +**GREEN — 0 floor-gate findings, 0 open advisories.** + +The run-1 blocking finding is resolved by a mechanism rather than a rewording; the three important +advisories (P0 mislabel, P1 coverage, P5 casing) and the two minors (P3 split, P6 stale FACT-TABLE) +are closed; and amendment 2 closes the two that run 2 recorded as out of scope. **Nine findings +raised across three review passes, nine addressed.** + +One judgment worth flagging for the human rather than burying: amendment 2 bundles a second +measurement axis (`repo.ts`'s cache / ref-tier / tar claims) into an increment scoped to the proxy +read, which run 2 itself argued against on P3/P7 grounds. The counter-argument is recorded in +`PLAN.md`'s amendment 2 — the axes differ but the *defect* is identical, and fixing one while leaving +the other keeps the root cause live in a file the increment already cites. Reasonable people could +scope that the other way. + +As in run 1: the **floor** here is `validate.mjs` GREEN plus the machine-checkable facts behind each +finding. The lens judgments and all severities are **LLM-assigned and advisory** (`finding-shape.md`, +fix #3). "GREEN" is this review's assessment, not a deterministic gate — `/pharn-dev-review` writes no +`findings.json` and has no `check-review.mjs`. The decision remains the human's. + +--- + +## Proposed lesson for canon (NOT written here — `/pharn-dev-memory-promote` is a separate, human-gated run) + +**Candidate:** *A dependency fact measured at one version becomes a false claim the moment it is +written into shipped output, unless the artifact that ships also pins the version — and the honest fix +is a runtime membership test, not a more careful sentence.* + +This increment measured `degit@3.6.6` correctly and repeatedly, labeled the result advisory in the +code comment and the CHANGELOG, and still shipped a wrong sentence — because "the lockfile pins it" +was carried from the dev repo to a published package that ships no lockfile and marks the dependency +`external`. The measurement was never the weak link; the **scope of the pin** was. What made the fix +hold was not better wording but `MEASURED_DEGIT_VERSIONS` + a runtime read + a test that re-derives +the claim from the installed bytes. + +**Provenance:** increment `proxy-env-notice`, 2026-08-19; run-1 finding `P0`/blocking at +`docs/troubleshooting.md:157`; contradicting evidence `package.json` (`dependencies.degit: ^3.6.1`, +`files: ["dist"]`) and `scripts/build.mjs:15` (`external: [… 'degit' …]`); resolved in run 2 by +`src/lib/proxy-env.ts` (`MEASURED_DEGIT_VERSIONS`, `resolveDegitProxyRead`) and the tripwire in +`tests/proxy-env.test.ts`. + +**Recurrence check (P7 — real, not hypothetical):** the **second** time this repo has corrected a +claim about `degit` internals (#98 was the first), and +`.dev/features/trust-map-records-era/REVIEW.md:167` already records that those were *"caught only +because the"* boundary was re-measured by hand. The advisory finding above — that `repo.ts` still +carries 3.6.6-scoped claims — is a live third instance waiting to happen. Whether that is a pattern +worth canonizing is the human's call at the promotion gate. diff --git a/.dev/features/proxy-env-notice/SHIP.md b/.dev/features/proxy-env-notice/SHIP.md new file mode 100644 index 0000000..6757b08 --- /dev/null +++ b/.dev/features/proxy-env-notice/SHIP.md @@ -0,0 +1,109 @@ +# SHIP — proxy-env-notice + +A `/pharn-dev-ship` roll-up across **three passes**: the initial gated chain, a fix pass driven by +`/pharn-dev-review`'s blocking finding, and a third closing the two advisories that pass 2 recorded as +out of scope. It records **that the chain ran and its floor verdicts** — +nothing more. + +## Stages + +| # | stage | pass 1 verdict | pass 2 verdict | source | +| - | ----- | -------------- | -------------- | ------ | +| 1 | `/pharn-dev-plan` | GATE 1 approved | plan **amended**, 9 → 15 declared files | human approval | +| 2 | `/pharn-dev-grill` | 7 concerns (advisory) | — (not re-run; advisory, gates nothing) | `GRILL.md` | +| 3 | `/pharn-dev-build` | `validate` exit **0** | `validate` exit **0** | floor exit code | +| 4 | `/pharn-dev-regress` | **`no-regressions`** | **`no-regressions`** | `regression-report.json` `.verdict` | +| 5 | `/pharn-dev-verify` | **`PASS`** | **`PASS`** | `verify-report.json` `.verdict` | +| 6 | `/pharn-dev-review` | **BLOCKED — 1 floor-gate finding** | **GREEN — 0 floor-gate findings** | `REVIEW.md` | + +Pass 3 (amendment 2) re-ran build → regress → verify → review against an expanded scope of **17** +declared files: `validate` exit **0**, **`no-regressions`**, **`PASS`**, review **GREEN with 0 open +advisories**. + +**Where the run ended: GATE 2** — the post-review human decision. No stage returned a non-GREEN floor +verdict in either pass. + +## The structural verdicts, verbatim (pass 2) + +- **`/pharn-dev-build` → `node .dev/floor/validate.mjs .` exit `0`.** All six repo gates green + independently: `format:check`, `lint`, `lint:md`, `typecheck`, `test` **807/807 across 43 files**, + `build`. +- **`/pharn-dev-regress` → `"verdict": "no-regressions"`**, `regressions[]` and `pre_existing[]` both + empty. Gates `tests` 0→0, `validate` 0→0 against base `03160c8`. `scope` exit 0 with + `inside == declared` — **15 files each**. +- **`/pharn-dev-verify` → `"verdict": "PASS"`**, `failing_gates[]` empty, `verifiers.registered: 0`. + +## Why there was a second pass + +Every floor verdict in pass 1 was green **with a false sentence in the tree**. `/pharn-dev-review` +caught it: `docs/troubleshooting.md` claimed `degit@3.6.6` was "the version this release resolves." +Re-measured, that was wrong *today*, not merely fragile — degit's latest is **3.8.0**, `package.json` +declares `^3.6.1`, `files: ["dist"]` ships no lockfile, and `scripts/build.mjs:15` marks degit +`external`, so a fresh install resolves 3.8.0. + +No deterministic check in this repo compares a doc sentence to a dependency range, which is +`/pharn-dev-verify`'s named residual behaving exactly as documented — "verified = the named gates +passed," never "the feature is correct." + +The fix replaced the claim with a mechanism: all nine published versions in the declared range were +swept, they became `MEASURED_DEGIT_VERSIONS`, and the confident wording is now gated on a runtime +version read. A tripwire test re-derives the claim from the installed bytes each run — **verified to +be a real tripwire** by mutating the expected list and observing RED. + +## Every review finding, and its disposition + +| # | principle | run-1 severity | disposition | +| - | --------- | -------------- | ----------- | +| 1 | P0 | **blocking** | **Fixed** — false pin claim removed; claim now an enum membership test | +| 2 | P0 | important | **Fixed** — unhedged message gated on a measured version | +| 3 | P1 | important | **Fixed** — wiring tests at all five call sites + `--no-drift` silence | +| 4 | P5 | minor | **Fixed** — POSIX casing hole closed; the notice names the variant found | +| 5 | P2 | minor | **Fixed** — length bound + no-echo now tested | +| 6 | P3 | minor | **Fixed** — presentation split into `proxy-env-format.ts` | +| 7 | P6 | minor | **Fixed** — `FACT-TABLE.md` H5 corrected | +| 8 | P0 | minor | **Fixed (pass 3)** — `repo.ts`'s degit claims re-measured at 3.8.0 and re-scoped to the range | +| 9 | P7 | minor | **Fixed (pass 3)** — lockfile bumped to 3.8.0; published range deliberately unchanged | + +**Nine findings raised across three review passes, nine addressed.** + +## Self-inflicted problems this run produced and corrected — recorded, not smoothed over + +1. **`/pharn-dev-regress` `scope` exited 1** on three "escapes", each traced to its author (the plan + and grill stages' own declared `writes:`, plus a pre-existing staged `.gitignore`). Not a build escape. +2. **A false `tests = 1` at both ends** — a zsh word-splitting bug in the stage's own harness. Would + have been reported as `pre_existing`, the label under which a self-inflicted red goes unexamined. +3. **The scope setter silently scoped 9 of 15 paths** after the plan amendment, because `**+**` markers + broke its `isPathItem` pattern. The **plan format was corrected to match the parser**, never the + hook loosened. +4. **`markdownlint-cli2 --fix` corrupted `FACT-TABLE.md`** — a file outside the `lint:md` gate's scope — + turning `#331/#345/…` into a heading and flipping six bullets. Reverted to `HEAD` and re-applied + without `--fix`. + +## Pointers (cited, not restated — P4) + +- [`PLAN.md`](PLAN.md) — approved, then amended after review. Picks Option 1 (both directions); + **rejects Option 3** on two measured grounds. +- [`GRILL.md`](GRILL.md) — advisory, 7 concerns. Its finding 2 named the dependency-range axis + **before** the build; it was not folded in, and it is what blocked run 1. +- [`REGRESSION.md`](REGRESSION.md) / [`regression-report.json`](regression-report.json) +- [`VERIFY.md`](VERIFY.md) / [`verify-report.json`](verify-report.json) +- [`REVIEW.md`](REVIEW.md) — run 2: GREEN, 2 advisory findings open by design. Read before deciding. + +## What the chain still does NOT establish + +- `MEASURED_DEGIT_VERSIONS` is re-derived from the installed bytes on every run — but only for the + **one** version installed. It is now **3.8.0**, the version consumers resolve, which is the strongest + form this check has taken; that the other eight were measured correctly still rests on a manual sweep + no gate repeats. +- Nothing compares a doc sentence to `package.json`. The class of defect that blocked pass 1 would + still be caught only by review. +- `repo.ts`'s cache / ref-tier / tar claims are re-measured but **not** tripwired — unlike the proxy + read, no test re-derives them. They are labeled ADVISORY / provenance-bounded, which is honest, not + guaranteed. +- The published range still permits a future degit past what was measured. That is handled by + **degradation** (the notice hedges) rather than by pinning, which remains a maintainer's call. + +--- + +**Chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good +or wise; that is the human's call at the post-review gate.** diff --git a/.dev/features/proxy-env-notice/VERIFY.md b/.dev/features/proxy-env-notice/VERIFY.md new file mode 100644 index 0000000..032b506 --- /dev/null +++ b/.dev/features/proxy-env-notice/VERIFY.md @@ -0,0 +1,83 @@ +# VERIFY — proxy-env-notice + +**Run 3** — after amendment 2. The suite now runs against **degit 3.8.0**, the version a +consumer resolves, rather than the 3.6.6 the lockfile previously pinned — so the tripwire below +re-derives the proxy claim from the bytes users actually get. + +## FLOOR layer — the deterministic gates (owns the verdict) + +| gate | command | exit | +| ---- | ------- | ---- | +| `test` | `npm test` | **0** | +| `validate` | `node .dev/floor/validate.mjs .` | **0** | +| `lint` | `npm run lint` | **0** | +| `format:check` | `npm run format:check` | **0** | +| `lint:md` | `npm run lint:md` | **0** | + +`failing_gates[]`: **empty**. Suite: **43 files, 802 tests** (from 779 at run 1 — +23 net). + +**`structural:*` — none.** The increment ships no committed eval pair, so no such gate is in the map. +`validate` remains **vacuously green**: the increment adds TypeScript modules, not a markdown +Capability, so frontmatter / eval-binding / archetype-map checks have nothing to bind to. The real +correctness signal is the vitest suite. + +### What run 2 added to that signal + +Run 1's verdict was PASS with three of five call sites untested. That gap is closed: + +| file | covers | +| ---- | ------ | +| `tests/proxy-env.test.ts` | truth table incl. the casing hole and deterministic multi-variant resolution; `MEASURED_DEGIT_VERSIONS` membership; the runtime version read | +| `tests/proxy-env-format.test.ts` | confident vs hedged wording, redaction, control-char neutralization, length bound | +| `tests/init.test.ts` | notice emitted **before** the fetch | +| `tests/add.test.ts` | **both** fetch sites — named path and picker path — plus a no-clone path staying silent | +| `tests/update.test.ts` | wiring, plus the up-to-date early return staying silent | +| `tests/status.test.ts` | wiring on the drift path, plus `--no-drift` silence (previously asserted only in a comment) | + +## The blocking finding from run 1, re-checked + +`/pharn-dev-review` blocked on `docs/troubleshooting.md` claiming degit@3.6.6 was "the version this +release resolves." Re-measured this run: **degit's latest is 3.8.0**, `dependencies.degit` is +`^3.6.1`, `files: ["dist"]` ships no lockfile, and `scripts/build.mjs:15` marks degit `external` — so +the claim was wrong for any consumer installing today, not merely fragile. + +The fix is **not** a reworded caveat. All nine published versions in the declared range +(`3.6.1 3.6.2 3.6.3 3.6.4 3.6.5 3.6.6 3.7.0 3.7.1 3.8.0`) were swept and all read only the lowercase +name; those nine are now `MEASURED_DEGIT_VERSIONS`, and the confident wording fires **only** on set +membership against the version read at runtime. That converts a hand measurement into an enum +membership test — a floor primitive (`ARCHITECTURE.md §2` #3) — and makes a future degit release +degrade the message rather than falsify it. + +Smoke-checked against the live tree (`installed degit: 3.6.6 | measured: true`): the confident form on +a measured version, the hedged form naming both versions on `3.9.0`, and a distinct hedge when the +version cannot be read at all. + +## ADVISORY layer — verifiers + +**No verifiers registered — floor gates only.** `node .dev/floor/count-verifiers.mjs .` → +`{"registered":0,"verifiers":[]}`, a deterministic frontmatter read (P5), not a prose grep. Step 2 is a +no-op; none was authored for this increment (P7). + +Consequence, unchanged from run 1: the questions a verifier would ask — *is a version-gated warning +the right response at all? does a line on every clone earn its place?* — were asked by no automated +layer. `/pharn-dev-grill` raised the nearest equivalents advisorily; the rest is the human's. + +## Verdict (FLOOR — `.dev/floor/check-verify.mjs`, exit 0) + +**VERIFIED: floor gates PASS.** + +Machine report: [`verify-report.json`](verify-report.json) — `"verdict": "PASS"`, `"failing_gates": []`. + +The verdict rests on the helper comparing integers. It cannot receive a finding, so no judgment could +have moved it. Advisory: which gates ran, how the map was assembled, and this prose. + +**Residual (named, not hidden):** verified = **the named gates passed** — not a guarantee of +correctness beyond what those gates check. Two limits worth stating precisely, since run 1's residual +named a weakness this pass changed: + +- The central factual premise is now **tested** (`MEASURED_DEGIT_VERSIONS` membership, and that + `measured` is derived from it rather than constant) — but the test asserts the SET's contents, not + that the set is TRUE of degit. The sweep that established it was a manual measurement; nothing in CI + re-runs it. Adding a degit version to that set without measuring it would pass every gate here. +- Nothing checks that a doc sentence agrees with `package.json`. The class of defect that blocked run 1 + is still invisible to this verdict — it was caught by review, and would be again only by review. diff --git a/.dev/features/proxy-env-notice/regression-report.json b/.dev/features/proxy-env-notice/regression-report.json new file mode 100644 index 0000000..a511b31 --- /dev/null +++ b/.dev/features/proxy-env-notice/regression-report.json @@ -0,0 +1,35 @@ +{ + "base": "03160c8e71e14a125f7dd3d107a42f18952a6f15", + "inside": [ + ".dev/features/degit-fetch-boundary-truth/FACT-TABLE.md", + "CHANGELOG.md", + "docs/troubleshooting.md", + "package-lock.json", + "src/commands/add.ts", + "src/commands/init.ts", + "src/commands/status.ts", + "src/commands/update.ts", + "src/lib/proxy-env-format.ts", + "src/lib/proxy-env.ts", + "src/lib/repo.ts", + "tests/add.test.ts", + "tests/init.test.ts", + "tests/proxy-env-format.test.ts", + "tests/proxy-env.test.ts", + "tests/status.test.ts", + "tests/update.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/proxy-env-notice/verify-report.json b/.dev/features/proxy-env-notice/verify-report.json new file mode 100644 index 0000000..45b04db --- /dev/null +++ b/.dev/features/proxy-env-notice/verify-report.json @@ -0,0 +1,16 @@ +{ + "feature": "proxy-env-notice", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.gitignore b/.gitignore index 64c1c75..49f74b6 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,4 @@ test-full test-lib test-next test-spa +prompts diff --git a/.markdownlint-cli2.jsonc b/.markdownlint-cli2.jsonc index a8de25c..8634f1a 100644 --- a/.markdownlint-cli2.jsonc +++ b/.markdownlint-cli2.jsonc @@ -1,6 +1,6 @@ { // Apply to all markdown except dependencies, generated output, and the changelog - "ignores": ["node_modules", "dist", "coverage", "CHANGELOG.md"], + "ignores": ["node_modules", "dist", "coverage", "CHANGELOG.md", "**/*.updated.*"], "config": { "default": true, // Line length: docs sometimes need long lines (URLs, tables, code fences) diff --git a/.pharn/pharn-dev-regress/base-results.json b/.pharn/pharn-dev-regress/base-results.json index c3ea19a..9b3f2b3 100644 --- a/.pharn/pharn-dev-regress/base-results.json +++ b/.pharn/pharn-dev-regress/base-results.json @@ -1 +1 @@ -{"tests":0,"validate":0} \ No newline at end of file +{"tests":0,"validate":0} diff --git a/.pharn/pharn-dev-regress/head-results.json b/.pharn/pharn-dev-regress/head-results.json index c3ea19a..9b3f2b3 100644 --- a/.pharn/pharn-dev-regress/head-results.json +++ b/.pharn/pharn-dev-regress/head-results.json @@ -1 +1 @@ -{"tests":0,"validate":0} \ No newline at end of file +{"tests":0,"validate":0} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index f5d2d41..217f56d 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/degit-fetch-boundary-truth/SHIP.md" + ".dev/features/proxy-env-notice/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-08-17T20:34:31.323Z" + "set_at": "2026-08-19T20:09:53.359Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c777c6..c039815 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **The `degit` clone's proxy handling is no longer invisible.** `degit` reads + `process.env.https_proxy` in its own constructor — unconditionally, with no option `pharn` could + pass — and only that **lowercase** spelling appears anywhere in its bundle. So a user who exported + `HTTPS_PROXY`, the spelling most tooling honors, was connecting **directly** on macOS and Linux with + no signal anywhere; a user who exported `https_proxy` was having the clone interposed by a host + `pharn` never mentioned. Both directions were silent. `init` / `add` / `update` / `status` now read + the environment before starting the clone and print the applicable line — that the variable will be + ignored (suppressed on Windows, where lookups are case-insensitive and it _is_ read), or that the + clone **may be routed** through the named proxy and that `no_proxy` exclusions do not apply to it, + since degit reads no such variable. The warning names whichever spelling you actually set, so a + `Https_Proxy` typo is caught too, and several variants resolve deterministically rather than by + environment order. Credentials in the value are redacted to `***`, an unparseable value degrades to + `(set)` rather than echoing raw bytes, and the value is **never** written to `pharn.config.json` — + it is git-committed, and proxy URLs routinely carry passwords. + + **The confident wording is gated on a measured degit version.** `pharn` declares the range `^3.6.1` + and the published package ships no lockfile, so the installed version is whatever `npm` resolves — + today `3.8.0`, not the `3.6.6` this repo develops against. Every published version in that range was + swept (`3.6.1` … `3.8.0`, nine in total) and all read only the lowercase name; `pharn` reads the + version at runtime and states the negative assertion only for those. On any other version it hedges, + naming both the measured range and what is installed — so a newer degit makes the notice more + cautious rather than wrong. + + Deliberately **not** done: recording the proxy in the config or the install summary as a fact about + the connection. degit skips the download entirely when the tarball is already cached and falls back + to a spawned `git clone` on some failures, so "a proxy was in effect" is not derivable from the + environment — hence "may be routed", never "was routed". The notices remain **advisory**: they + report your environment against measured degit versions, never the transport that ran. + `docs/troubleshooting.md` gains a "Proxy environment variables" section. + +### Changed + +- **The dev/CI `degit` now matches what a consumer resolves.** `package.json` declares `^3.6.1` and the + published package ships no lockfile, so an install resolves the newest matching release — `3.8.0` — + while this repo's lockfile still pinned `3.6.6`. The version the gates exercised was therefore two + minors behind the one users get, which is precisely how a claim about `degit` internals gets written + against a version nobody runs. The **lockfile** moves to `3.8.0`; the declared **range is + unchanged** (narrowing it is a separate decision). API compatibility was verified before the bump — + same callable default export, `.clone()` / `.on()` intact, still no runtime dependencies, and + `engines.node >=20.0.0` against pharn's `>=20`. `src/lib/repo.ts`'s comments about degit's ref tiers, + cache behavior, and warn sites are re-scoped from a single version to the measured **range** + (3.6.1-3.8.0), where every claim was re-verified. + ### Docs - **The fetch boundary now tells the truth about `degit`.** `THREAT-MODEL.md` described the clone as an @@ -93,7 +138,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 there.** The drift check read your project with its own bare `existsSync` / `readFileSync`, which went wrong four ways. A **directory** where a file belongs threw `EISDIR` out of the middle of the comparison, so status printed a raw errno naming no file and **the drift report for every other file - was lost** — one bad path took down the whole run. A **symlink** was read *through*: pointing it at a + was lost** — one bad path took down the whole run. A **symlink** was read _through_: pointing it at a file with other bytes listed the path under "differs", erasing the fact that a link — not an edit — was the cause, and pointing it at a **byte-identical** file made status count it as matching and say **nothing at all**, silently blessing a path that leads outside your install and can change under it @@ -118,23 +163,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `pharn add` / `pharn remove` pickers have always used (`interactiveAllowed` — imported, not re-implemented; a static test pins that this repo has exactly one such predicate). Each gate sits **after** that command's promptless local step — `update`'s config load, `init`'s git prerequisite — - so an uninitialized directory still gets *"run `pharn init`"* and a directory with no `.git` still - gets *"run `git init`"*, never a misleading message about a prompt they would not have reached. Each + so an uninitialized directory still gets _"run `pharn init`"_ and a directory with no `.git` still + gets _"run `git init`"_, never a misleading message about a prompt they would not have reached. Each gate also sits **before any network call**, so a refused run costs zero round-trips and wastes no clone. **TTY behavior is deliberately unchanged:** a human choosing Cancel is still a user-initiated, graceful exit 0 — only EOF masquerading as that choice is now unreachable. - **`pharn add` no longer installs at a layout your config does not record.** PHARN ships in two - install layouts (the legacy flat one, and everything under `pharn/`). `add` copied at the *clone's* + install layouts (the legacy flat one, and everything under `pharn/`). `add` copied at the _clone's_ layout while `pharn remove`, `pharn list`, and `pharn status` all look at the layout recorded in `pharn.config.json` — so when the two disagreed, the capability landed where nothing would ever find - it: invisible to `list`/`status`, and a later `remove` reported *"its files were already gone"* while + it: invisible to `list`/`status`, and a later `remove` reported _"its files were already gone"_ while dropping only the config entry, orphaning the directory on disk. `add` now **refuses** when the clone's layout differs from your recorded one, naming both layouts and pointing at `pharn update --force`, and writes nothing — no capability directory, no `pharn.config.json`, no `pharn.records.json`. `add` deliberately does **not** record the clone's layout the way `update` does: `update` may only because it rewrites your whole install at that layout, while `add` writes a single capability. - *Scope, honestly:* the common flat→`pharn` migration window was already closed by the version gate + _Scope, honestly:_ the common flat→`pharn` migration window was already closed by the version gate in the previous release, since a pre-migration install also has a pre-migration `skillsVersion`. What this closes is the residual case — a config that reached the current version with a stale, absent, or hand-edited `layout`. Note that resolving such a same-version drift needs @@ -145,7 +190,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`pharn update --yes` (`-y`) — a real flag, for CI and scripts.** It skips **the confirmation prompt and nothing else**: the version note still prints, the same per-file decision table applies, files you edited are still skipped rather than overwritten, the recorded version is still withheld when anything - was skipped, and every exit code is unchanged. It means *"do not ask"*, not *"non-interactive mode"* — + was skipped, and every exit code is unchanged. It means _"do not ask"_, not _"non-interactive mode"_ — so it works in a terminal too — and it composes with `--force` (`pharn update --yes --force` is the full CI re-apply). `--force` does **not** imply `--yes`: overwriting your edits is the most destructive thing `update` does, so it still asks. Because `--yes` is only consent, it is not a drift check — a run @@ -177,7 +222,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 gate without editing a single script: their `console`/`process` were never wrong, the config simply declared no globals at all. Choosing `nodeBuiltin` over plain `node` keeps `__dirname`/`require` in an `.mjs` a lint error, since those do not exist in ESM and would otherwise crash at runtime. - *Scope, honestly:* the root config files (`eslint.config.mjs`, `vitest.config.ts`), `.dev/floor/`, + _Scope, honestly:_ the root config files (`eslint.config.mjs`, `vitest.config.ts`), `.dev/floor/`, and `.claude/hooks/` are **not** linted. And `--max-warnings 0` counts warnings that are actually **emitted** — it is not a defence against a rule set to `off`, a new `ignores` entry, or an inline `eslint-disable` comment. @@ -201,8 +246,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Every capability membership change is now named.** When the list changes, `update` prints a `CAPABILITIES` section saying exactly what moved and why — `ADDED — newly selected for your - archetypes`, `REMOVED — no longer selected for your archetypes`, `REMOVED — no longer exists upstream - (was a manual add)`, or `KEPT — your manual add, not selected by your archetypes`. When nothing +archetypes`, `REMOVED — no longer selected for your archetypes`, `REMOVED — no longer exists upstream +(was a manual add)`, or `KEPT — your manual add, not selected by your archetypes`. When nothing changed, nothing is printed. > **Named limit:** a removal is not a tombstone. If your archetypes still select a capability you @@ -231,7 +276,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 A `source` present but outside `{auto, manual}` is reported by name (`capabilities[2].source`) and exits, instead of falling back to "run `pharn init`". Deleting the field is a valid fix. - - **`pharn add` no longer makes `pharn update` report "Already up to date" over a stale install.** `add` clones the tip of `pharn-dev/pharn-oss`, and it used to write that clone's `SKILLS_VERSION` into your `pharn.config.json` — even though every file it did not just copy still held the old diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index adff2f0..9766f0e 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -113,6 +113,75 @@ If any install targets already exist and you decline the overwrite prompt, the w One thing *is* written outside your project, before either prompt appears: the repo fetch runs first, and `degit` persists a commit-named `.tar.gz` plus `map.json`/`access.json` into its own shared cache directory (`~/Library/Caches/degit` on macOS, `%LOCALAPPDATA%\degit` on Windows, `$XDG_CACHE_HOME`/`~/.cache` + `/degit` elsewhere). That cache is degit's, not pharn's, and declining the prompt does not remove it — delete the directory yourself if you need to reclaim the space. +## Proxy environment variables + +`init` / `add` / `update` / `status` clone `pharn-dev/pharn-oss` through [`degit`](https://github.com/Rich-Harris/degit), and **degit reads the proxy from the environment itself** — `pharn` passes it no proxy option and has no way to. Two consequences are worth knowing, and `pharn` now prints a line about each **before** it starts the clone. + +### `HTTPS_PROXY` is not read on macOS or Linux + +degit reads **only the lowercase `https_proxy`**. If you set any other spelling and nothing else, the clone connects **directly**, ignoring your proxy: + +```text +⚠ HTTPS_PROXY is set, but degit 3.8.0 reads only the lowercase https_proxy — + the PHARN clone will connect DIRECTLY. Set https_proxy to the same value if + you meant to proxy it. +``` + +The fix is to set both: + +```bash +export https_proxy="$HTTPS_PROXY" +``` + +The warning names whichever variable you actually set, so a `Https_Proxy` typo is caught too. + +On **Windows** this does not apply — environment lookups are case-insensitive there, so `HTTPS_PROXY` is read and the clone *is* proxied. `pharn` does not print this warning on Windows. + +### A proxy that is in force is announced + +When `https_proxy` is set (or, on Windows, either spelling), `pharn` names it: + +```text +⚠ The PHARN clone may be routed through http://***@proxy.internal:3128 (https_proxy). + It reads no no_proxy/NO_PROXY, so proxy exclusions do not apply to it. +``` + +Two details in that message are deliberate: + +- **"may be routed", not "was routed".** degit skips the download entirely when the commit's tarball is already in its cache (see [Overwrite declined](#overwrite-declined) for where that cache lives), and some failures fall back to a spawned `git clone` that never sees the proxy. `pharn` reports what degit **will read**, not which transport ran — it cannot observe that. +- **Credentials are redacted.** Any `user:password@` in the value is replaced with `***`. The value is only printed; it is never written to `pharn.config.json`, which lives in your repository and is committed. + +`no_proxy` / `NO_PROXY` appear nowhere in degit, so an exclusion list that works for your other tools does **not** exempt this clone. + +### Which degit versions this was measured against + +`pharn` does not pin `degit` for you. It declares the **range** `^3.6.1`, and the published package +ships no lockfile, so `npm` resolves the version fresh when you install — today that is `3.8.0`. + +Every published version in that range was checked, and all nine read only the lowercase name: + +```text +3.6.1 3.6.2 3.6.3 3.6.4 3.6.5 3.6.6 3.7.0 3.7.1 3.8.0 +``` + +`pharn` reads the version you actually have and only states the confident wording above when it is one +of those. On any other version it hedges instead, naming both what was measured and what you have: + +```text +⚠ HTTPS_PROXY is set, and the PHARN clone will probably ignore it: every degit + pharn has measured (3.6.1-3.8.0) reads only the lowercase https_proxy, but the + installed degit is 3.9.0. Set https_proxy to the same value to be sure. +``` + +So a newer degit makes the notice more cautious, never wrong. If you see the hedged form, the +behavior has not necessarily changed — it just has not been verified for your version. + +### Scope + +These notices cover the **degit clone only**. `pharn` makes its own small HTTPS requests too (the commit SHA, and `SKILLS_VERSION` for `update` / `status --no-drift`); their proxy behavior is a separate question this documentation does not make a claim about. `status --no-drift` never clones, so it prints no proxy notice at all. + +The notices are **advisory**: they report your environment against degit versions `pharn` has measured. They are not a guarantee about the connection that actually happened — `pharn` cannot observe that. + ## `add` / `update` say to run init first ```text diff --git a/package-lock.json b/package-lock.json index 62b1c1d..faf8ff9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "license": "Apache-2.0", "dependencies": { "@clack/prompts": "^1.7.0", - "degit": "^3.6.1", + "degit": "^3.8.0", "minimist": "^1.2.8", "picocolors": "^1.1.1" }, @@ -1896,9 +1896,9 @@ "license": "MIT" }, "node_modules/degit": { - "version": "3.7.0", - "resolved": "https://registry.npmjs.org/degit/-/degit-3.7.0.tgz", - "integrity": "sha512-eQWGfEdkkFTeqrKRS4BeYGatRI9Z5uIDN+eJ/XbxbsgRB7C62GP4L+FPmZmnkoDIU1bbxkZBoCaIz0LT8CCPZg==", + "version": "3.8.0", + "resolved": "https://registry.npmjs.org/degit/-/degit-3.8.0.tgz", + "integrity": "sha512-ZHJCvLjT0B9mo3Z7wX/luIJBLiO0N4ZkTvZTST72FAoDhUVBCmGWuOvlMRHJGEc1G3PDqE1Dk9UP5ExQCV5XzQ==", "license": "MIT", "bin": { "degit": "degit" diff --git a/src/commands/add.ts b/src/commands/add.ts index 1c7c470..2b078f4 100644 --- a/src/commands/add.ts +++ b/src/commands/add.ts @@ -26,6 +26,8 @@ import { } from '../lib/install-records.js'; import { configLayout, detectLayout, layoutPaths } from '../lib/layout.js'; import { fetchRepo } from '../lib/repo.js'; +import { detectProxyNotice, resolveDegitProxyRead } from '../lib/proxy-env.js'; +import { proxyNoticeMessage } from '../lib/proxy-env-format.js'; import { readSkillsVersion } from '../lib/skills-version.js'; import { loadArchetypeConfigOrExit, @@ -131,6 +133,14 @@ async function runArchetypeAdd( process.exit(1); } + // What degit's single lowercase `https_proxy` read means here — emitted before + // the spinner so it survives the frame and precedes a proxy-caused failure + // (see src/commands/init.ts for the full rationale). + const proxyNotice = detectProxyNotice(process.env, process.platform); + if (proxyNotice) { + log.warn(proxyNoticeMessage(proxyNotice, resolveDegitProxyRead())); + } + const s = spinner(); s.start(`Fetching capabilities from ${REPO_URL}`); let repo; @@ -202,6 +212,14 @@ async function runAddPicker(config: PharnConfig, cwd: string): Promise { process.exit(1); } + // What degit's single lowercase `https_proxy` read means here — emitted before + // the spinner so it survives the frame and precedes a proxy-caused failure + // (see src/commands/init.ts for the full rationale). + const proxyNotice = detectProxyNotice(process.env, process.platform); + if (proxyNotice) { + log.warn(proxyNoticeMessage(proxyNotice, resolveDegitProxyRead())); + } + const s = spinner(); s.start(`Fetching capabilities from ${REPO_URL}`); let repo; diff --git a/src/commands/init.ts b/src/commands/init.ts index 2a39872..15d71e0 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -7,6 +7,8 @@ import { interactiveAllowed } from '../lib/capability-picker.js'; import { parseCapabilityIndex } from '../lib/capability-index.js'; import { resolveCapabilities } from '../lib/resolve-capabilities.js'; import { fetchRepo } from '../lib/repo.js'; +import { detectProxyNotice, resolveDegitProxyRead } from '../lib/proxy-env.js'; +import { proxyNoticeMessage } from '../lib/proxy-env-format.js'; import { runGitPrereq } from '../steps/prereqs.js'; import { confirmWriteTargets } from '../steps/overwrite-check.js'; import { runArchetypeSummary } from '../steps/archetype-summary.js'; @@ -63,6 +65,21 @@ async function runInitArchetype(): Promise { const { archetypes } = detectArchetypesFromProject(cwd); note(archetypes.join(', '), 'Detected archetypes'); + // degit reads process.env.https_proxy ITSELF (measured at degit@3.6.6) and + // reads ONLY that lowercase spelling, so a non-lowercase-only environment + // clones DIRECTLY on POSIX while a `https_proxy` one is interposed by a host + // pharn never declared — neither of which was discoverable from any pharn + // output. Emit BEFORE the spinner starts, for two reasons: a log.warn into an + // active clack spinner frame is overwritten, and a clone that FAILS because of + // a misconfigured proxy is exactly when the user most needs to have been told. + // ADVISORY: this reports what degit WILL READ, never what transport ran; the + // confident wording is gated on the installed degit being a version pharn + // measured (lib/proxy-env.ts, MEASURED_DEGIT_VERSIONS). + const proxyNotice = detectProxyNotice(process.env, process.platform); + if (proxyNotice) { + log.warn(proxyNoticeMessage(proxyNotice, resolveDegitProxyRead())); + } + const s = spinner(); s.start(`Fetching PHARN from ${REPO_URL}`); let repo: Awaited>; diff --git a/src/commands/status.ts b/src/commands/status.ts index 1eec154..794d940 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -2,6 +2,8 @@ import { intro, log, note, outro, spinner } from '@clack/prompts'; import pc from 'picocolors'; import { REPO, REPO_BRANCH } from '../lib/constants.js'; import { fetchRepo } from '../lib/repo.js'; +import { detectProxyNotice, resolveDegitProxyRead } from '../lib/proxy-env.js'; +import { proxyNoticeMessage } from '../lib/proxy-env-format.js'; import { diffInstalledCapabilities } from '../lib/diff.js'; import type { InstallDiff } from '../lib/diff.js'; import { configLayout } from '../lib/layout.js'; @@ -69,6 +71,15 @@ async function runArchetypeStatus( return; } + // What degit's single lowercase `https_proxy` read means here — emitted before + // the spinner so it survives the frame and precedes a proxy-caused failure + // (see src/commands/init.ts). Inside the drift branch, so + // `status --no-drift` — which never clones — stays silent. + const proxyNotice = detectProxyNotice(process.env, process.platform); + if (proxyNotice) { + log.warn(proxyNoticeMessage(proxyNotice, resolveDegitProxyRead())); + } + const s = spinner(); s.start(`Comparing against ${REF}`); let repo; diff --git a/src/commands/update.ts b/src/commands/update.ts index 1d0750c..187b1ad 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -31,6 +31,8 @@ import { } from '../lib/install-records.js'; import { planUpdate, type UpdatePlan } from '../lib/update-decision.js'; import { fetchRepo } from '../lib/repo.js'; +import { detectProxyNotice, resolveDegitProxyRead } from '../lib/proxy-env.js'; +import { proxyNoticeMessage } from '../lib/proxy-env-format.js'; import { fetchRemoteSkillsVersion, readSkillsVersion, @@ -173,6 +175,14 @@ async function runArchetypeUpdate( if (isCancel(ok) || ok !== true) cancelAndExit(); } + // What degit's single lowercase `https_proxy` read means here — emitted before + // the spinner so it survives the frame and precedes a proxy-caused failure + // (see src/commands/init.ts for the full rationale). + const proxyNotice = detectProxyNotice(process.env, process.platform); + if (proxyNotice) { + log.warn(proxyNoticeMessage(proxyNotice, resolveDegitProxyRead())); + } + const s2 = spinner(); s2.start(`Updating from ${REPO_URL}`); let repo; diff --git a/src/lib/proxy-env-format.ts b/src/lib/proxy-env-format.ts new file mode 100644 index 0000000..d9492a6 --- /dev/null +++ b/src/lib/proxy-env-format.ts @@ -0,0 +1,93 @@ +import { + MEASURED_DEGIT_RANGE, + type DegitProxyRead, + type ProxyNotice, +} from './proxy-env.js'; + +/** + * Presentation for the degit proxy notice. Separated from ./proxy-env.ts because + * the two change for different reasons (P3): this file changes when wording does, + * that one when degit's measured behavior does. + */ + +const LOWER = 'https_proxy'; + +/** + * Cap on the rendered proxy value. `new URL()` already neutralizes the spoofing + * shapes that matter — a control character in the path is percent-encoded, one + * in the host throws — so this bounds terminal FLOODING rather than injection: + * the environment is attacker-influencable and a multi-kilobyte value should not + * scroll a user's install away. + */ +const MAX_RENDERED = 120; + +/** + * Render a proxy value safe to print. `https_proxy` conventionally carries + * inline credentials (`http://user:pass@host:3128`), and that hazard is also why + * the value is NEVER recorded in `pharn.config.json` — that file is written to + * the project root and git-committed, so recording it would commit the user's + * proxy credentials into their repository. + * + * Userinfo collapses to `***`. An unparseable value degrades to the fixed + * literal `(set)` rather than echoing raw bytes, so hostile content cannot be + * echoed verbatim to spoof pharn's own output. + */ +export function redactProxyUrl(value: string): string { + let url; + try { + url = new URL(value); + } catch { + return '(set)'; + } + const auth = url.username !== '' || url.password !== '' ? '***@' : ''; + const path = url.pathname === '/' ? '' : url.pathname; + const rendered = `${url.protocol}//${auth}${url.host}${path}`; + return rendered.length > MAX_RENDERED + ? `${rendered.slice(0, MAX_RENDERED)}…` + : rendered; +} + +/** How to name the installed degit when it is not one pharn measured. */ +function installedLabel(read: DegitProxyRead): string { + return read.version === null + ? 'the installed version could not be read' + : `the installed degit is ${read.version}`; +} + +/** + * The user-facing line for a notice. + * + * Two wording rules carry the honesty of this feature, and both are load-bearing: + * + * 1. "may be routed", never "was routed". degit's download helper returns early + * on `FILE_EXISTS` when the SHA-named tarball is already in its shared cache, + * and two of its tar failures fall back to a spawned `git clone` that never + * receives the proxy. "A proxy was in effect" is NOT derivable from the + * environment — only "degit will read this value" is. + * 2. The CONFIDENT `ignored` wording fires only when `read.measured` — i.e. the + * installed degit is one whose lowercase-only read pharn actually verified. + * On any other version the message hedges and names both the measured range + * and what is installed. pharn declares a RANGE (`^3.6.1`) and ships no + * lockfile, so without this gate the confident sentence would be asserted + * over an unmeasured dependency. + * + * Neither branch echoes an unredacted value, and the `ignored` branch echoes no + * value at all — a non-lowercase spelling is exactly as credential-bearing as + * its lowercase twin. The variable NAME it does echo is safe by construction + * (see `ProxyNotice`). + */ +export function proxyNoticeMessage( + notice: ProxyNotice, + read: DegitProxyRead, +): string { + if (notice.kind === 'ignored') { + return read.measured + ? `${notice.name} is set, but degit ${read.version} reads only the lowercase ${LOWER} — the PHARN clone will connect DIRECTLY. Set ${LOWER} to the same value if you meant to proxy it.` + : `${notice.name} is set, and the PHARN clone will probably ignore it: every degit pharn has measured (${MEASURED_DEGIT_RANGE}) reads only the lowercase ${LOWER}, but ${installedLabel(read)}. Set ${LOWER} to the same value to be sure.`; + } + + const caveat = read.measured + ? '' + : ` Measured on degit ${MEASURED_DEGIT_RANGE}; ${installedLabel(read)}.`; + return `The PHARN clone may be routed through ${redactProxyUrl(notice.value)} (${LOWER}). It reads no no_proxy/NO_PROXY, so proxy exclusions do not apply to it.${caveat}`; +} diff --git a/src/lib/proxy-env.ts b/src/lib/proxy-env.ts new file mode 100644 index 0000000..e1eba31 --- /dev/null +++ b/src/lib/proxy-env.ts @@ -0,0 +1,196 @@ +import { createRequire } from 'node:module'; + +/** + * What degit's proxy handling means for THIS run — the logic half (detection + + * version membership). Message strings live in ./proxy-env-format.ts, which + * changes for wording reasons while this file changes only when degit's measured + * behavior does (P3, mirroring model-routing.ts / model-routing-format.ts). + * + * degit's constructor assigns `this.proxy = process.env.https_proxy` + * UNCONDITIONALLY — there is no `options.proxy ?? …` fallback, which is why + * src/degit.d.ts declares no `proxy` option and why fetchRepo cannot pass one. + * The only lever over the clone's transport is the environment variable itself, + * and only that LOWERCASE spelling is ever read. + * + * That single lowercase read is a footgun in BOTH directions, neither of which + * was previously discoverable from any pharn output: + * + * - a non-lowercase spelling (`HTTPS_PROXY`, the one most tooling honors) set + * alone → the clone connects DIRECTLY, silently, on POSIX. Node's + * `process.env` is case-INSENSITIVE on win32, so the same environment IS + * proxied there, and `engines.node` sets no `os` restriction — both are in + * the supported matrix. + * - `https_proxy` set → the clone may be interposed by a host pharn never + * declared, and degit reads no `no_proxy`, so an exclusion list the user + * believes is in force does not apply to it. + * + * Why an env read rather than observing degit: the `{code:'PROXY'}` event is + * emitted through `verboseInfo`, defined `verboseInfo(e){this.verbose&&this.info(e)}`. + * fetchRepo passes no `verbose`, so that event NEVER fires on pharn's path and an + * `emitter.on('info', …)` listener would observe nothing. Reading the environment + * is the only signal available — which is why every claim here is about + * CONFIGURATION ("degit will read this") and never about the transport that ran. + */ + +/** The one lowercase name degit reads. */ +const LOWER = 'https_proxy'; +/** The uppercase spelling users reach for — preferred when several variants are set. */ +const UPPER = 'HTTPS_PROXY'; + +/** + * Every published degit version in the range package.json declares (`^3.6.1`), + * swept for proxy-env behavior. All nine show exactly ONE proxy name in + * `dist/*.js` — lowercase `https_proxy`, no `no_proxy`/`NO_PROXY`/`ALL_PROXY` — + * the same unconditional `this.proxy=process.env.https_proxy`, and the same + * `verboseInfo` gate. + * + * This set is the FLOOR under the confident wording (an exact-string membership + * test, ARCHITECTURE.md §2 primitive 3). It exists because the behavior above is + * a property of the DEPENDENCY, and pharn does not pin it for consumers: the + * published package declares the RANGE, ships no lockfile (`files: ["dist"]`), + * and marks degit `external` in the esbuild bundle — so an install resolves it + * fresh. Without the gate, `pharn` would assert a measured negative about + * whatever version npm happened to hand the user. + * + * Extend this set only by MEASURING the new version, never by assuming a patch + * release kept the behavior. An unlisted version is not treated as broken — it + * is treated as UNVERIFIED, and the notice hedges accordingly. + */ +export const MEASURED_DEGIT_VERSIONS: ReadonlySet = new Set([ + '3.6.1', + '3.6.2', + '3.6.3', + '3.6.4', + '3.6.5', + '3.6.6', + '3.7.0', + '3.7.1', + '3.8.0', +]); + +/** Human-facing span of MEASURED_DEGIT_VERSIONS, for the hedged message. */ +export const MEASURED_DEGIT_RANGE = '3.6.1-3.8.0'; + +/** Shape of a semver-ish version string, so a garbage read is rejected not echoed. */ +const VERSION_LIKE = /^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$/; + +export type ProxyNotice = + /** + * A proxy is set in a spelling degit will not read here, so the clone connects + * direct. `name` is the variable actually found — safe to print by + * construction, since only a key whose lowercase equals `https_proxy` can + * reach it (one of 2^11 ASCII spellings; it cannot carry a control character). + */ + | { kind: 'ignored'; name: string } + /** degit WILL read this value. Carries the raw value for redacted rendering. */ + | { kind: 'active'; value: string }; + +/** What pharn knows about the degit it is about to call. */ +export interface DegitProxyRead { + /** Resolved installed version, or null when it could not be read/validated. */ + version: string | null; + /** version ∈ MEASURED_DEGIT_VERSIONS — the lowercase-only read was verified for it. */ + measured: boolean; +} + +/** Present AND non-empty. An empty string is not a proxy setting. */ +function isSet(value: string | undefined): value is string { + return value !== undefined && value !== ''; +} + +/** + * Every key that case-insensitively spells `https_proxy` and has a non-empty + * value, in a DETERMINISTIC order (P5): `HTTPS_PROXY` first when present, then + * the rest sorted. Without the sort the answer would depend on env insertion + * order, which is not a property any caller should depend on. + */ +function proxyVariantKeys(env: Record): string[] { + const keys = Object.keys(env) + .filter((key) => key.toLowerCase() === LOWER && isSet(env[key])) + .sort(); + return keys.includes(UPPER) + ? [UPPER, ...keys.filter((k) => k !== UPPER)] + : keys; +} + +/** + * Classify the environment against degit's single lowercase read. + * + * Deterministic membership/presence test (P5) over `env` and `platform`, with no + * classification and no I/O. Pure: both are parameters, never read from + * `process.*`, so every row is exercisable from a test on any host — including + * the win32 rows on the ubuntu-only CI runner. + * + * The win32 branch reproduces Node's case-INSENSITIVE `process.env` lookup over + * an injected record: the real `process.env` resolves it through its own proxy, + * but a plain object in a test would not, and modelling it here is what keeps the + * platform difference testable. + * + * The terminal case is `null` — SILENCE — the complete and correct answer when + * nothing is set, not a degraded fallback. + */ +export function detectProxyNotice( + env: Record, + platform: string, +): ProxyNotice | null { + const variants = proxyVariantKeys(env); + + if (platform === 'win32') { + // Any spelling resolves, so any variant present means the clone is proxied. + const key = variants[0]; + const value = key === undefined ? undefined : env[key]; + return isSet(value) ? { kind: 'active', value } : null; + } + + // Case-sensitive: only the exact lowercase name reaches degit. + const exact = env[LOWER]; + // Covers the both-set case too — a user who set BOTH spellings IS proxied, so + // warning that their setting is ignored would be a false alarm. + if (isSet(exact)) return { kind: 'active', value: exact }; + + // Anything else that spells https_proxy is a variable degit will never read. + const ignored = variants.find((key) => key !== LOWER); + return ignored === undefined ? null : { kind: 'ignored', name: ignored }; +} + +/** + * Read the degit version actually installed, or null. + * + * The ONE impure function in this module, isolated so everything above stays + * unit-testable. degit declares no `exports` field, so the subpath resolves; + * `createRequire(import.meta.url)` works from the bundled dist because degit is + * `external` there and therefore present in node_modules at runtime. + * + * Every failure mode collapses to null (P5 — the fallback is "unknown", which + * the caller renders as a hedge, never a guess): the package missing, the + * subpath unresolvable, the JSON unreadable, or a `version` that is not a + * plausible version string. The value is validated before use even though it + * comes from node_modules — it is echoed to a terminal, and an unchecked read is + * how a surprising string becomes output (P2). + */ +function readDegitVersion(): string | null { + try { + const require = createRequire(import.meta.url); + const pkg = require('degit/package.json') as { version?: unknown }; + return typeof pkg.version === 'string' && VERSION_LIKE.test(pkg.version) + ? pkg.version + : null; + } catch { + return null; + } +} + +/** + * Resolve what pharn knows about the degit it is about to call: which version is + * installed, and whether that version's proxy behavior was actually measured. + * + * `measured` is the floor under the confident wording — a membership test over + * MEASURED_DEGIT_VERSIONS, not a judgment. + */ +export function resolveDegitProxyRead(): DegitProxyRead { + const version = readDegitVersion(); + return { + version, + measured: version !== null && MEASURED_DEGIT_VERSIONS.has(version), + }; +} diff --git a/src/lib/repo.ts b/src/lib/repo.ts index 7abf536..936821f 100644 --- a/src/lib/repo.ts +++ b/src/lib/repo.ts @@ -44,6 +44,19 @@ export interface FetchedRepo { * longer a ref tip) rather than fetching drift — but a compromised upstream * serving a valid-shaped tree at that SHA still passes; trust is provenance + * the path/network floor, never signature verification. + * + * VERSION SCOPE for every degit claim in this file (ref tiers, the cache, the + * warn sites). They were measured across EVERY published version in the range + * package.json declares — `^3.6.1`, i.e. 3.6.1 through 3.8.0 — and hold in all + * of them. Naming the range rather than a single version is deliberate: the + * published package ships no lockfile (`files: ["dist"]`) and marks degit + * `external` in the bundle, so a consumer resolves the RANGE, not whatever this + * repo's lockfile pins. A comment pinned to one version is a claim about a + * version most users are not running. These are ADVISORY, provenance-bounded + * (THREAT-MODEL.md §4b): properties of the dependency, not pharn floor checks + * that re-run — a later degit could change them. The one degit property pharn + * DOES re-derive on every test run is the proxy-env read + * (lib/proxy-env.ts + tests/proxy-env.test.ts). */ export async function fetchRepo(): Promise { // The sha is network-derived (fetchCommitSha reads it from the GitHub commits @@ -60,9 +73,9 @@ export async function fetchRepo(): Promise { const ref = sha ?? REPO_BRANCH; const dir = mkdtempSync(join(tmpdir(), 'pharn-')); try { - // `cache: false` is NOT no-cache — measured against degit@3.6.6, it selects - // the HASH SOURCE (resolve the ref over the network rather than read the - // cached map); it does not suppress the cache. degit's tarball download runs + // `cache: false` is NOT no-cache — it selects the HASH SOURCE (resolve the + // ref over the network rather than read the cached map); it does not + // suppress the cache. degit's tarball download runs // INSIDE `if (!options.cache)`: it reuses an existing tarball at the cache // path when one is there, else mkdirs that path and downloads into it. Writing // is broader still — access.json/map.json are written UNGATED, as is the diff --git a/tests/add.test.ts b/tests/add.test.ts index a7dbce8..c07997a 100644 --- a/tests/add.test.ts +++ b/tests/add.test.ts @@ -202,6 +202,98 @@ describe('runAdd (archetype)', () => { ]); }); + // --- the degit proxy notice (wiring) --------------------------------------- + // + // add has TWO fetch sites — the named path and the picker path — and the + // picker one sits behind an arg check plus a non-TTY refusal, making it the + // site most likely to be lost in a refactor. Both are pinned, plus the + // no-clone refusal paths. + describe('proxy notice', () => { + afterEach(() => vi.unstubAllEnvs()); + + it('warns before the clone on the NAMED path', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + loadArchetypeConfigOrExit.mockReturnValue(archConfig()); + mockClone(); + let warnedBeforeFetch = false; + const cleanup = vi.fn(); + fetchRepo.mockImplementationOnce(async () => { + warnedBeforeFetch = vi.mocked(prompts.log.warn).mock.calls.length > 0; + return { dir: '/repo', sha: 'sha', cleanup }; + }); + + await runAdd('a11y'); + + expect(warnedBeforeFetch).toBe(true); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('may be routed'); + }); + + it('warns before the clone on the PICKER path', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'griller' }, + { name: 'security', role: 'griller' }, + { name: 'n-plus-one', role: 'lens' }, + ]), + ); + mockClone(); + setTTY(true, true); + let warnedBeforeFetch = false; + const cleanup = vi.fn(); + fetchRepo.mockImplementationOnce(async () => { + warnedBeforeFetch = vi.mocked(prompts.log.warn).mock.calls.length > 0; + return { dir: '/repo', sha: 'sha', cleanup }; + }); + + await runAdd(undefined); + + expect(warnedBeforeFetch).toBe(true); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('may be routed'); + }); + + // A path that never clones must not describe a transport. The legacy-config + // abort exits before any fetch. + it('says nothing on a path that never clones', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + loadArchetypeConfigOrExit.mockImplementationOnce(() => { + throw new ProcessExit(1); + }); + + await expect(runAdd('a11y')).rejects.toMatchObject(new ProcessExit(1)); + + expect(fetchRepo).not.toHaveBeenCalled(); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + + it('says nothing when no proxy variable is set', async () => { + vi.stubEnv('https_proxy', undefined); + vi.stubEnv('HTTPS_PROXY', undefined); + loadArchetypeConfigOrExit.mockReturnValue(archConfig()); + mockClone(); + + await runAdd('a11y'); + + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + }); + it('no-arg in a TTY with everything installed exits 0 without prompting', async () => { loadArchetypeConfigOrExit.mockReturnValue( archConfig([ diff --git a/tests/init.test.ts b/tests/init.test.ts index 0c4cf0a..01bdfc5 100644 --- a/tests/init.test.ts +++ b/tests/init.test.ts @@ -125,6 +125,81 @@ describe('runInit (archetype default)', () => { expect(runInstallArchetype).not.toHaveBeenCalled(); }); + // --- the degit proxy notice ------------------------------------------------ + // + // degit reads process.env.https_proxy ITSELF and reads ONLY that lowercase + // spelling, so an `HTTPS_PROXY`-only environment clones DIRECTLY on POSIX + // while a `https_proxy` one is interposed by a host pharn never declared. + // Neither was discoverable from any pharn output. These pin the WIRING (the + // truth table itself lives in tests/proxy-env.test.ts). + describe('proxy notice', () => { + // mockResolvedValue survives clearAllMocks, so the cancel paths asserted + // earlier in this file would otherwise leak in and exit(0) before the + // install. Re-arm the happy path explicitly. + beforeEach(() => { + runArchetypeSummary.mockResolvedValue('install'); + confirmWriteTargets.mockResolvedValue(true); + }); + afterEach(() => vi.unstubAllEnvs()); + + // The ordering is the point, not merely that a warn happened: a clone that + // FAILS because of a misconfigured proxy is exactly when the user most needs + // to have been told, so the notice must precede the fetch rather than follow + // a successful one. + it.skipIf(process.platform === 'win32')( + 'warns that HTTPS_PROXY is ignored, BEFORE the fetch', + async () => { + vi.stubEnv('https_proxy', undefined); + vi.stubEnv('HTTPS_PROXY', 'http://proxy.internal:3128'); + let warnedBeforeFetch = false; + fetchRepo.mockImplementationOnce(async () => { + warnedBeforeFetch = vi.mocked(log.warn).mock.calls.length > 0; + return { dir: '/fake/repo', sha: 'sha123', cleanup }; + }); + + await runInit(); + + expect(warnedBeforeFetch).toBe(true); + const warned = vi + .mocked(log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('HTTPS_PROXY'); + // The message is version-gated (confident on a measured degit, hedged + // otherwise), so assert the half that holds either way. + expect(warned).toContain('https_proxy'); + }, + ); + + // The other direction: degit WILL read this one, so say so — hedged, because + // a cached tarball short-circuits the download entirely. Platform-independent: + // the lowercase name resolves everywhere. + it('warns that a lowercase https_proxy MAY route the clone', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + vi.stubEnv('HTTPS_PROXY', undefined); + + await runInit(); + + const warned = vi + .mocked(log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('may be routed'); + expect(warned).toContain('no_proxy'); + }); + + // Silence on the common path — the notice must not become install noise. + it('says nothing when neither spelling is set', async () => { + vi.stubEnv('https_proxy', undefined); + vi.stubEnv('HTTPS_PROXY', undefined); + + await runInit(); + + expect(fetchRepo).toHaveBeenCalledTimes(1); + expect(log.warn).not.toHaveBeenCalled(); + }); + }); + // --- non-interactive honesty: the TTY gate --------------------------------- // // The bug this closes: off a TTY the archetype summary's select rendered into a diff --git a/tests/proxy-env-format.test.ts b/tests/proxy-env-format.test.ts new file mode 100644 index 0000000..fbfea28 --- /dev/null +++ b/tests/proxy-env-format.test.ts @@ -0,0 +1,181 @@ +import { describe, expect, it } from 'vitest'; +import { + proxyNoticeMessage, + redactProxyUrl, +} from '../src/lib/proxy-env-format.js'; +import type { DegitProxyRead } from '../src/lib/proxy-env.js'; + +const PROXY = 'http://proxy.internal:3128'; +const MEASURED: DegitProxyRead = { version: '3.8.0', measured: true }; +const UNMEASURED: DegitProxyRead = { version: '3.9.0', measured: false }; +const UNREADABLE: DegitProxyRead = { version: null, measured: false }; + +describe('redactProxyUrl', () => { + // https_proxy conventionally carries inline credentials. This is the same + // hazard that keeps the value out of pharn.config.json entirely (that file is + // git-committed) — here it must not reach the terminal either. + it('collapses userinfo to *** and does not leak the secret', () => { + const rendered = redactProxyUrl('http://user:s3cret@proxy.internal:3128'); + + expect(rendered).toBe('http://***@proxy.internal:3128'); + expect(rendered).not.toContain('s3cret'); + expect(rendered).not.toContain('user'); + }); + + it('redacts a username with no password', () => { + expect(redactProxyUrl('http://user@proxy.internal:3128')).toBe( + 'http://***@proxy.internal:3128', + ); + }); + + it('leaves a credential-free URL readable', () => { + expect(redactProxyUrl(PROXY)).toBe(PROXY); + }); + + // An unparseable value degrades to a fixed literal rather than echoing raw + // bytes — the environment is attacker-influencable, so hostile content (an + // ANSI escape plus a fabricated error line) must never be echoed verbatim to + // spoof pharn's own output. + it('degrades an unparseable value to (set) without echoing it', () => { + const esc = String.fromCharCode(27); + const rendered = redactProxyUrl( + `not a url ${esc}[31mFATAL: install failed`, + ); + + expect(rendered).toBe('(set)'); + expect(rendered).not.toContain('FATAL'); + expect(rendered).not.toContain(esc); + }); + + // A control character inside an otherwise-parseable URL is neutralized by + // URL parsing rather than by a hand-rolled filter. + it('never emits a control character from a parseable value', () => { + const esc = String.fromCharCode(27); + const rendered = redactProxyUrl(`http://proxy.internal/${esc}[31m`); + + expect(rendered).not.toContain(esc); + }); + + it('bounds a pathological value rather than flooding the terminal', () => { + const rendered = redactProxyUrl( + `http://proxy.internal/${'a'.repeat(5000)}`, + ); + + expect(rendered.length).toBeLessThanOrEqual(121); + expect(rendered.endsWith('…')).toBe(true); + }); +}); + +describe('proxyNoticeMessage - ignored branch', () => { + // On a version pharn actually measured, the negative assertion is earned and + // stated plainly, naming the version it rests on. + it('is confident on a measured degit and cites the version', () => { + const message = proxyNoticeMessage( + { kind: 'ignored', name: 'HTTPS_PROXY' }, + MEASURED, + ); + + expect(message).toContain('HTTPS_PROXY'); + expect(message).toContain('https_proxy'); + expect(message).toContain('3.8.0'); + expect(message).toContain('will connect DIRECTLY'); + }); + + // The whole point of the version gate: pharn declares a RANGE and ships no + // lockfile, so on a version nobody measured the confident sentence must not + // be asserted. + it('hedges on an unmeasured degit and names both versions', () => { + const message = proxyNoticeMessage( + { kind: 'ignored', name: 'HTTPS_PROXY' }, + UNMEASURED, + ); + + expect(message).not.toContain('will connect DIRECTLY'); + expect(message).toContain('probably ignore it'); + expect(message).toContain('3.6.1-3.8.0'); + expect(message).toContain('3.9.0'); + }); + + it('hedges when the installed version cannot be read at all', () => { + const message = proxyNoticeMessage( + { kind: 'ignored', name: 'HTTPS_PROXY' }, + UNREADABLE, + ); + + expect(message).not.toContain('will connect DIRECTLY'); + expect(message).toContain('could not be read'); + }); + + // The variable actually found is what gets named, so the advice points at the + // user's real environment rather than an assumed spelling. + it('names the variant actually found', () => { + const message = proxyNoticeMessage( + { kind: 'ignored', name: 'HtTpS_PrOxY' }, + MEASURED, + ); + + expect(message).toContain('HtTpS_PrOxY'); + }); + + // A non-lowercase spelling is exactly as credential-bearing as its lowercase + // twin, so this branch echoes no value at all. + it('echoes no value on the ignored branch', () => { + expect( + proxyNoticeMessage({ kind: 'ignored', name: 'HTTPS_PROXY' }, MEASURED), + ).not.toContain('proxy.internal'); + }); +}); + +describe('proxyNoticeMessage - active branch', () => { + // "may be routed", never "was routed" — degit's download helper returns early + // when the SHA-named tarball is already cached, and a tar failure falls back + // to a spawned `git clone` that never receives the proxy. What ran is not + // derivable from the environment; only what degit WILL READ is. + it('says the clone MAY be routed, never that it was', () => { + const message = proxyNoticeMessage( + { kind: 'active', value: PROXY }, + MEASURED, + ); + + expect(message).toContain('may be routed'); + expect(message).not.toContain('was routed'); + expect(message).toContain(PROXY); + }); + + // degit's bundle contains no no_proxy read at all, so an exclusion list the + // user believes is in force does not apply to the clone. + it('warns that no_proxy exclusions do not apply', () => { + expect( + proxyNoticeMessage({ kind: 'active', value: PROXY }, MEASURED), + ).toContain('no_proxy'); + }); + + it('renders the value redacted', () => { + const message = proxyNoticeMessage( + { kind: 'active', value: 'http://user:s3cret@proxy.internal:3128' }, + MEASURED, + ); + + expect(message).toContain('***'); + expect(message).not.toContain('s3cret'); + }); + + // The active claim is version-dependent too, so an unmeasured degit earns a + // caveat here as well — just a softer one, since the positive read has held + // in every version measured. + it('appends a version caveat on an unmeasured degit', () => { + const message = proxyNoticeMessage( + { kind: 'active', value: PROXY }, + UNMEASURED, + ); + + expect(message).toContain('3.6.1-3.8.0'); + expect(message).toContain('3.9.0'); + }); + + it('appends no caveat on a measured degit', () => { + expect( + proxyNoticeMessage({ kind: 'active', value: PROXY }, MEASURED), + ).not.toContain('Measured on degit'); + }); +}); diff --git a/tests/proxy-env.test.ts b/tests/proxy-env.test.ts new file mode 100644 index 0000000..e7de5ba --- /dev/null +++ b/tests/proxy-env.test.ts @@ -0,0 +1,271 @@ +import { readFileSync, readdirSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it, afterEach, vi } from 'vitest'; +import { + MEASURED_DEGIT_VERSIONS, + detectProxyNotice, + resolveDegitProxyRead, +} from '../src/lib/proxy-env.js'; + +// The unit under test is the truth table in src/lib/proxy-env.ts, which encodes +// one measured fact: degit's constructor reads `process.env.https_proxy` +// unconditionally, and that LOWERCASE name is the only proxy variable anywhere +// in its bundle — verified across every published version in the declared +// ^3.6.1 range. Node's process.env is case-insensitive on win32 and +// case-sensitive everywhere else, so the same environment means opposite things +// on the two platform families; that is why the detector takes `env` and +// `platform` as PARAMETERS. Nothing here reads process.*, so the win32 rows run +// on the ubuntu-only CI runner. + +const PROXY = 'http://proxy.internal:3128'; + +describe('detectProxyNotice - the truth table', () => { + // Row 1 — nothing set. Silence is the complete answer, not a degraded one. + it('returns null when no spelling is set', () => { + expect(detectProxyNotice({}, 'linux')).toBeNull(); + expect(detectProxyNotice({}, 'win32')).toBeNull(); + }); + + it('treats an empty string as unset (empty is not a proxy setting)', () => { + expect(detectProxyNotice({ https_proxy: '' }, 'linux')).toBeNull(); + expect(detectProxyNotice({ HTTPS_PROXY: '' }, 'linux')).toBeNull(); + expect(detectProxyNotice({ HTTPS_PROXY: '' }, 'win32')).toBeNull(); + }); + + // Row 2 — the headline case: the common spelling, silently ignored on POSIX. + it('flags HTTPS_PROXY as ignored on linux when https_proxy is unset', () => { + expect(detectProxyNotice({ HTTPS_PROXY: PROXY }, 'linux')).toEqual({ + kind: 'ignored', + name: 'HTTPS_PROXY', + }); + }); + + // Platform-FAMILY-wide, not linux-only — a darwin developer hits it too. + it('flags HTTPS_PROXY as ignored on darwin too', () => { + expect(detectProxyNotice({ HTTPS_PROXY: PROXY }, 'darwin')).toEqual({ + kind: 'ignored', + name: 'HTTPS_PROXY', + }); + }); + + // The casing hole: any variant degit will not read must be reported, not just + // the canonical uppercase one — and the notice names the variable ACTUALLY + // found, so the advice points at the user's real environment. + it('flags an arbitrary casing as ignored on POSIX, naming it', () => { + expect(detectProxyNotice({ HtTpS_PrOxY: PROXY }, 'linux')).toEqual({ + kind: 'ignored', + name: 'HtTpS_PrOxY', + }); + }); + + // Determinism (P5): with several variants set the answer must not depend on + // env insertion order. HTTPS_PROXY wins; otherwise lexicographic. + it('prefers HTTPS_PROXY when several variants are set, whatever the order', () => { + const a = detectProxyNotice( + { Https_Proxy: PROXY, HTTPS_PROXY: PROXY, hTTPS_PROXY: PROXY }, + 'linux', + ); + const b = detectProxyNotice( + { hTTPS_PROXY: PROXY, HTTPS_PROXY: PROXY, Https_Proxy: PROXY }, + 'linux', + ); + + expect(a).toEqual({ kind: 'ignored', name: 'HTTPS_PROXY' }); + expect(b).toEqual(a); + }); + + it('falls back to a deterministic pick when HTTPS_PROXY is absent', () => { + const a = detectProxyNotice( + { Https_Proxy: PROXY, hTTPS_proxy: PROXY }, + 'linux', + ); + const b = detectProxyNotice( + { hTTPS_proxy: PROXY, Https_Proxy: PROXY }, + 'linux', + ); + + expect(a).toEqual(b); + expect(a?.kind).toBe('ignored'); + }); + + // Row 3 — the FALSE-WARNING regression. On win32 process.env is + // case-insensitive, so degit's lowercase read DOES resolve HTTPS_PROXY: that + // user is proxied, and telling them otherwise would be worse than silence. + it('reports HTTPS_PROXY as ACTIVE on win32, never ignored', () => { + const notice = detectProxyNotice({ HTTPS_PROXY: PROXY }, 'win32'); + + expect(notice).toEqual({ kind: 'active', value: PROXY }); + expect(notice?.kind).not.toBe('ignored'); + }); + + it('resolves an arbitrary casing on win32 as active', () => { + expect(detectProxyNotice({ HtTpS_PrOxY: PROXY }, 'win32')).toEqual({ + kind: 'active', + value: PROXY, + }); + }); + + // Row 4 — the other direction: an interposed clone pharn never declared. + it('reports a lowercase https_proxy as active', () => { + expect(detectProxyNotice({ https_proxy: PROXY }, 'linux')).toEqual({ + kind: 'active', + value: PROXY, + }); + }); + + // Row 5 — both set. A user who set BOTH spellings IS proxied; warning that + // their setting is ignored would be a false alarm. + it('reports active (never ignored) when both spellings are set', () => { + const notice = detectProxyNotice( + { https_proxy: PROXY, HTTPS_PROXY: PROXY }, + 'linux', + ); + + expect(notice).toEqual({ kind: 'active', value: PROXY }); + expect(notice?.kind).not.toBe('ignored'); + }); + + // The lowercase value wins even when the two disagree — it is the one degit + // actually reads, so it is the one worth naming. + it('reports the lowercase value when the two spellings disagree', () => { + expect( + detectProxyNotice( + { https_proxy: PROXY, HTTPS_PROXY: 'http://other:8080' }, + 'linux', + ), + ).toEqual({ kind: 'active', value: PROXY }); + }); + + // An unrelated variable that merely CONTAINS the name must not match. + it('ignores variables that are not exactly the proxy name', () => { + expect( + detectProxyNotice( + { MY_HTTPS_PROXY: PROXY, https_proxy_url: PROXY }, + 'linux', + ), + ).toBeNull(); + }); +}); + +describe('detectProxyNotice - purity', () => { + afterEach(() => vi.unstubAllEnvs()); + + // The injected record is the ONLY input. If the detector reached for the real + // process.env this would come back `active` instead of null. + it('reads the injected record, never process.env', () => { + vi.stubEnv('https_proxy', PROXY); + + expect(detectProxyNotice({}, 'linux')).toBeNull(); + expect(detectProxyNotice({}, 'win32')).toBeNull(); + }); +}); + +describe('MEASURED_DEGIT_VERSIONS', () => { + // The floor under the confident wording. Every published version in the + // ^3.6.1 range package.json declares was swept; all nine read only the + // lowercase name. Extending this set requires MEASURING, never assuming. + it('covers every published version in the declared ^3.6.1 range', () => { + expect([...MEASURED_DEGIT_VERSIONS].sort()).toEqual([ + '3.6.1', + '3.6.2', + '3.6.3', + '3.6.4', + '3.6.5', + '3.6.6', + '3.7.0', + '3.7.1', + '3.8.0', + ]); + }); + + it('does not contain a version nobody measured', () => { + expect(MEASURED_DEGIT_VERSIONS.has('3.9.0')).toBe(false); + expect(MEASURED_DEGIT_VERSIONS.has('4.0.0')).toBe(false); + }); +}); + +// The claim under MEASURED_DEGIT_VERSIONS is a property of the DEPENDENCY, +// established by a manual sweep. This block re-verifies it on every run against +// whatever degit is actually installed, so the set cannot quietly drift from the +// thing it describes: if a future degit changes the read, this goes RED and +// someone must re-measure rather than discovering it from a user's bug report. +describe('the installed degit still behaves as MEASURED_DEGIT_VERSIONS claims', () => { + const distDir = fileURLToPath( + new URL('../node_modules/degit/dist/', import.meta.url), + ); + const chunks = () => + readdirSync(distDir) + .filter((f) => f.endsWith('.js')) + .map((f) => readFileSync(join(distDir, f), 'utf8')); + + it('ships a dist/ to inspect (a packaging change must not pass vacuously)', () => { + expect(chunks().length).toBeGreaterThan(0); + }); + + it('reads exactly one proxy env name, and it is lowercase https_proxy', () => { + const names = new Set(); + for (const src of chunks()) { + for (const m of src.matchAll( + /https?_proxy|HTTPS?_PROXY|no_proxy|NO_PROXY|ALL_PROXY|all_proxy/g, + )) { + names.add(m[0]); + } + } + + expect([...names]).toEqual(['https_proxy']); + }); + + it('assigns the proxy unconditionally in its constructor', () => { + const found = chunks().some((src) => + /this\.proxy\s*=\s*process\.env\.https_proxy/.test(src), + ); + + expect(found).toBe(true); + }); + + // The PROXY event is verbose-gated, so an emitter listener would observe + // nothing on pharn's path — this is what rules out "observe degit instead of + // reading the env" as a design. + it('gates its PROXY event behind verbose', () => { + const found = chunks().some((src) => + /verboseInfo\([a-zA-Z_$]\)\{this\.verbose&&this\.info\([a-zA-Z_$]\)\}/.test( + src, + ), + ); + + expect(found).toBe(true); + }); + + // If the installed version is one pharn measured, the above IS the measurement + // holding. If it is not, the code hedges instead — so this test documents + // which of the two states CI is currently in. + it('either matches a measured version, or the notice hedges', () => { + const read = resolveDegitProxyRead(); + + expect(read.measured).toBe( + read.version !== null && MEASURED_DEGIT_VERSIONS.has(read.version), + ); + }); +}); + +describe('resolveDegitProxyRead', () => { + // Reads the degit actually installed. In this repo the lockfile pins one of + // the measured versions, so this asserts the wiring end-to-end rather than + // a hardcoded string. + it('reads the installed degit version and classifies it', () => { + const read = resolveDegitProxyRead(); + + expect(read.version).toMatch(/^\d+\.\d+\.\d+/); + expect(read.measured).toBe(MEASURED_DEGIT_VERSIONS.has(read.version!)); + }); + + // The gate is real: `measured` is set membership, not a constant true. + it('derives measured from set membership', () => { + const read = resolveDegitProxyRead(); + + expect(read.measured).toBe( + read.version !== null && MEASURED_DEGIT_VERSIONS.has(read.version), + ); + }); +}); diff --git a/tests/status.test.ts b/tests/status.test.ts index 54a1f14..65349fd 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -94,6 +94,68 @@ describe('runStatus (archetype)', () => { expect(fetchRepo).not.toHaveBeenCalled(); }); + // --- the degit proxy notice (wiring) --------------------------------------- + // + // status clones on the drift path only, so the notice must fire there and be + // silent under --no-drift. The --no-drift silence was previously asserted only + // in a code comment. + describe('proxy notice', () => { + afterEach(() => vi.unstubAllEnvs()); + + it('warns before the clone on the drift path', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + const cleanup = vi.fn(); + let warnedBeforeFetch = false; + fetchRepo.mockImplementationOnce(async () => { + warnedBeforeFetch = vi.mocked(prompts.log.warn).mock.calls.length > 0; + return { dir: '/repo', cleanup }; + }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue(CLEAN); + + await runStatus({}); + + expect(warnedBeforeFetch).toBe(true); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('may be routed'); + }); + + // --no-drift never clones, so there is no transport to describe. + it('says nothing under --no-drift, which never clones', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + fetchRemoteSkillsVersion.mockResolvedValue('1.0.0'); + + await runStatus({ drift: false }); + + expect(fetchRepo).not.toHaveBeenCalled(); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + + it('says nothing when no proxy variable is set', async () => { + vi.stubEnv('https_proxy', undefined); + vi.stubEnv('HTTPS_PROXY', undefined); + const cleanup = vi.fn(); + fetchRepo.mockResolvedValue({ dir: '/repo', cleanup }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue(CLEAN); + + await runStatus({}); + + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + }); + it('default: clones, reads SKILLS_VERSION, diffs capabilities, cleans up', async () => { const cleanup = vi.fn(); fetchRepo.mockResolvedValue({ dir: '/repo', cleanup }); diff --git a/tests/update.test.ts b/tests/update.test.ts index 20ac462..b8fc5c3 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -185,6 +185,62 @@ describe('runUpdate (drift-safe)', () => { expect(fetchRepo).not.toHaveBeenCalled(); }); + // --- the degit proxy notice (wiring) --------------------------------------- + // + // update clones, so the notice fires; the early-return "already up to date" + // path does not clone and must stay silent. + describe('proxy notice', () => { + afterEach(() => vi.unstubAllEnvs()); + + it('warns before the clone', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + await installed({ skillsVersion: '1.0.0' }); + let warnedBeforeFetch = false; + fetchRepo.mockImplementationOnce(async () => { + warnedBeforeFetch = vi.mocked(prompts.log.warn).mock.calls.length > 0; + return { dir: repo, sha: 'a'.repeat(40), cleanup }; + }); + + await runUpdate(); + + expect(warnedBeforeFetch).toBe(true); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).toContain('may be routed'); + }); + + // No clone on the up-to-date early return, so no transport to describe. + it('says nothing when update returns early without cloning', async () => { + vi.stubEnv('https_proxy', 'http://proxy.internal:3128'); + await installed({ skillsVersion: '1.1.0' }); + + await runUpdate(); + + expect(fetchRepo).not.toHaveBeenCalled(); + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + + it('says nothing when no proxy variable is set', async () => { + vi.stubEnv('https_proxy', undefined); + vi.stubEnv('HTTPS_PROXY', undefined); + await installed({ skillsVersion: '1.0.0' }); + + await runUpdate(); + + const warned = vi + .mocked(prompts.log.warn) + .mock.calls.map(([m]) => String(m)) + .join('\n'); + expect(warned).not.toContain('may be routed'); + }); + }); + it('reports already up to date without cloning', async () => { await installed({ skillsVersion: '1.1.0' }); await runUpdate();