diff --git a/CHANGELOG.md b/CHANGELOG.md index ba76948..30b4211 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Releases could not publish: the `Pack` step wrote into a directory nothing had created.** Since the release workflow was split into an unprivileged `build` job and a `publish` job, `build` ran `npm pack --pack-destination "$RUNNER_TEMP/pkg"` without creating `pkg/`, and npm does not create it (`ENOENT` on npm 10 and 11). Every Release run would have failed at `Pack` and never reached `publish`. The step now runs `mkdir -p` first, and a live test pins that every workflow's pack destination is created earlier in the job that packs. +- **A re-run `pharn init` dropped the `pharn.config.json` keys you added by hand.** Upstream PHARN reads top-level keys that users add themselves: `testResults` (without it `/pharn-loop` stops with `blocked: no-test-runner`) and `ship.requireAttestation`. `add`, `update` and `remove` kept them, but `init` rebuilt the config from its own fields alone. It now copies every top-level key pharn does not own across from the config it replaces, unchanged. It does so from any file that parses as a JSON object, including one the other commands refuse. Keys pharn owns are still written fresh. ## [0.5.0] - 2026-09-10 diff --git a/docs/commands/init.md b/docs/commands/init.md index 0d83be7..7d634df 100644 --- a/docs/commands/init.md +++ b/docs/commands/init.md @@ -135,8 +135,12 @@ edits — are listed **first** and marked `(edited)`, and the prompt says how ma continue, `init` copies each of them to `.pharn-backup//` **before** the first write and prints that directory as soon as it is created (byte-identical files are not edits and are not backed up). Capabilities you added by hand with `pharn add` (`source: "manual"` in `pharn.config.json`) are **kept**: -`init` installs them again and records them as `manual`, as long as upstream still ships them. An -unreadable or invalid existing config never blocks `init` — nothing is carried over from it. The target set is derived from the fetched clone's layout + your resolved selection (`lib/install-manifest.ts`), so it is exact — not a git-history heuristic. +`init` installs them again and records them as `manual`, as long as upstream still ships them. Top-level +keys in `pharn.config.json` that pharn does not own, such as upstream's `testResults` and `ship`, are +copied across unchanged — see [Keys pharn does not own](../reference/pharn-config.md#keys-pharn-does-not-own). +An unreadable or invalid existing config never blocks `init`. Manual capabilities are carried over +only from a config the other commands would accept, and your own keys from any config that parses as +a JSON object; a config that is not valid JSON carries nothing over. The target set is derived from the fetched clone's layout + your resolved selection (`lib/install-manifest.ts`), so it is exact — not a git-history heuristic. ### 7. Install diff --git a/docs/reference/pharn-config.md b/docs/reference/pharn-config.md index 15690d8..96209a0 100644 --- a/docs/reference/pharn-config.md +++ b/docs/reference/pharn-config.md @@ -25,6 +25,9 @@ archetypes/capabilities and the pinned commit). `isArchetypeConfig` treats the presence of a `capabilities` array as the marker of an archetype install. +Any top-level key **not** on this page is yours, and every command keeps it — see +[Keys pharn does not own](#keys-pharn-does-not-own). + `layout` is written only by `pharn init` and `pharn update`, each recording the layout of the clone it actually copied from. `pharn add` never writes the field — it [refuses](../commands/add.md#layout-mismatch) a clone whose layout disagrees with the recorded one, @@ -55,10 +58,12 @@ A `source` present but outside `{auto, manual}` is a hand-edit error: `pharn` re (`capabilities[2].source`) and exits, rather than falling back to "run `pharn init`". Deleting the field is a valid fix — the next update sets it. -> Re-running `pharn init` on an existing project is an explicit **start-over**: it rewrites -> `capabilities` from scratch, so every entry becomes `auto` and previous `manual` tags are lost. -> `init` warns before overwriting `pharn.config.json` and defaults to **No**. Use `pharn update` to -> refresh an existing install; `init` is for installing one. +> Re-running `pharn init` on an existing project **rewrites this file** from its own fields. Two +> things survive: capabilities you added with `pharn add` are installed again and stay `manual` (as +> long as upstream still ships them), and [keys pharn does not own](#keys-pharn-does-not-own) are +> copied across. Everything else pharn owns is written fresh. `init` warns before overwriting +> `pharn.config.json` and defaults to **No**. Use `pharn update` to refresh an existing install; +> `init` is for installing one. A sibling file, [`pharn.records.json`](pharn-records.md), holds a sha256 per installed file. It is written by the same operations that write this config and is **stamped** with this file's @@ -185,10 +190,52 @@ Five hand-edits are rejected by name: an unknown sibling key, an unknown step, a `resolutionOrder` whose last entry is not `ask`, and a `modelConfidenceThreshold` set without a `model` step in the order. +## Keys pharn does not own + +Every field on this page is **pharn's**, including the [legacy fields](#legacy-fields-pre-archetype-configs-still-load) +it no longer writes. Any **other** top-level key is **yours**: `pharn` does not interpret or validate it, +and every command that writes this file keeps it. Upstream PHARN documents two that you add by hand: + +| Key | Read by | What it sets | +| ------------- | -------------------------------------------------------------- | ------------------------------------------------------------ | +| `testResults` | `/pharn-test`, and `/pharn-verify`'s acceptance-criteria check | The JSON report format of each test gate | +| `ship` | `/pharn-ship` (`ship.requireAttestation`) | `true`: the ship stage asks for a named person's attestation | + +```json +{ + "testResults": { "test": "vitest-json", "test:e2e": "playwright-json" }, + "ship": { "requireAttestation": false } +} +``` + +Without `testResults`, `/pharn-loop` stops with `blocked: no-test-runner`. The shape of both keys is +upstream's to define — see the pharn-oss README, +[Per-test results](https://github.com/pharn-dev/pharn-oss#per-test-results). Because `pharn` does not +own these keys it does not check them, so a typo in one is not reported by any `pharn` command. + +How each command keeps them: + +- `pharn add`, `pharn remove` and `pharn update` edit this file in place, so a key they do not write + stays where it is. +- `pharn init` writes the file afresh from its own fields, then **copies every key pharn does not own + across from the config it replaces**, unchanged, after its own. It does this for any file that + parses as a JSON object, including one the other commands refuse: a config missing its `modules` + array (the case where they tell you to run `pharn init`), or one with an invalid `models` or `seam` + block. A file that is **not valid JSON** carries nothing over. Move it aside first + ([troubleshooting](../troubleshooting.md#the-config-is-not-valid-json)), then copy your keys back. + +`init` **never** carries over a key pharn owns. It writes `pharnVersion`, `skillsVersion`, `repo`, +`commit`, `installedAt`, `archetypes`, `capabilities`, `layout` and `modules` fresh. `models` and +`seam` go back to their defaults, so a hand-edit there does not survive a re-run `init`. +`pendingSkillsVersion`, `frozenCapabilities` and the legacy fields below are dropped. (`capabilities` +is rewritten too, but the entries you added with `pharn add` are kept as `manual` — see the +[init command](../commands/init.md#6-summary).) + ## Legacy fields (pre-archetype configs still load) The schema is **additive** (P7): a `pharn.config.json` written by an older, module-based CLI still loads, -and its now-unused fields are preserved on read. +and its now-unused fields are preserved on read. They are still pharn's fields, though, so a re-run +`pharn init` drops them rather than [carrying them over](#keys-pharn-does-not-own). Two fields are nonetheless **load-bearing**, and deleting either makes the file unreadable: a config without a string `skillsVersion` or without a `modules` array is treated as absent, and every command diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9646597..f00df7b 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -419,8 +419,9 @@ JSON parser supplies one, the **line and column** of the offending byte. Open th position and fix it; nothing else is needed, and nothing has been written. **Do not reach for `pharn init` here.** It rewrites `pharn.config.json` wholesale: hand-edited -`models` / `seam` blocks go back to defaults and every capability is re-stamped `source: "auto"`, -which discards the record of which capabilities you added by hand with `pharn add`. That record +`models` / `seam` blocks go back to defaults, keys of your own such as `testResults` are not carried +over, and every capability is re-stamped `source: "auto"`, which discards the record of which +capabilities you added by hand with `pharn add`. That record lives nowhere else, and `pharn update` reads it to keep your manual additions across upgrades. If the file is genuinely beyond repair, move it out of the way first so you can still read it, then diff --git a/src/lib/pharn-config.ts b/src/lib/pharn-config.ts index a3d12fc..5210aac 100644 --- a/src/lib/pharn-config.ts +++ b/src/lib/pharn-config.ts @@ -199,6 +199,79 @@ export function configPath(cwd: string): string { return resolve(cwd, CONFIG_FILENAME); } +/** + * Every top-level key this CLI OWNS: exactly the keys `PharnConfig` declares — + * the ones it writes and validates, plus the module-era fields it no longer + * writes but that are still its own (`constitution`, `stackAnswers`, …). + * + * `satisfies Record` makes the list exhaustive BY + * CONSTRUCTION: declaring a field on `PharnConfig` without listing it here (or + * listing one it does not declare) fails the typecheck. That is what keeps + * `userOwnedConfigEntries` honest — a new pharn-owned field can never be read + * as the user's and carried stale across a re-run init. + * + * A `Set` of the keys, never an `in` test against the literal: `in` walks the + * prototype chain, so a user key named `constructor` or `toString` would read + * as pharn's. + */ +const CLI_OWNED_KEYS: ReadonlySet = new Set( + Object.keys({ + pharnVersion: true, + skillsVersion: true, + pendingSkillsVersion: true, + frozenCapabilities: true, + repo: true, + commit: true, + constitution: true, + isMultiTenant: true, + modules: true, + installedAt: true, + models: true, + seam: true, + stackAnswers: true, + installedSkills: true, + archetypes: true, + capabilities: true, + layout: true, + } satisfies Record), +); + +/** + * The top-level entries of a parsed `pharn.config.json` that this CLI does NOT + * own (`CLI_OWNED_KEYS`): keys the user put there, most of them for upstream + * pharn-oss to read — `testResults` (the per-test results runners `/pharn-test` + * and `/pharn-verify` need) and `ship.requireAttestation`. Returned verbatim and + * never validated: pharn does not own their schema, so it has no business + * judging them. Pure. `pharn init` is the caller — it rebuilds the config from + * its own fields and carries these over (steps/install-archetype.ts). + * + * EVERY such key, not an allowlist of the two known today — deliberately: + * - It is the contract the other writers already keep. `readPharnConfig` passes + * unknown top-level keys through (P7, additive) and `add`/`update`/`remove` + * write that object back, so every other command already answers "does my key + * survive?" with yes. An allowlist would make `init` the one command whose + * answer depends on whether this CLI release has heard of the key. + * - Upstream outpaces CLI releases. A released CLI installs pharn-oss `main` + * HEAD, which added `testResults` in 6.15 and `ship` after it; an allowlist + * would re-arm this exact drop for the next key, in every deployed CLI, until + * a new release shipped. + * - The owned side is the closed, known set, so it is the side to enumerate: + * `CLI_OWNED_KEYS` is exhaustive by construction, while a list of foreign keys + * could only ever be complete by luck. + * The cost, accepted: a key nobody reads (a typo, another tool's leftover) + * survives a re-init too — exactly as it already survives `add`/`update`/`remove`. + * + * `Object.fromEntries` defines OWN data properties, so a `__proto__` key (an own + * key after `JSON.parse`) round-trips as a key and never becomes a prototype. + */ +export function userOwnedConfigEntries( + raw: Record, +): Record { + return Object.fromEntries( + Object.entries(raw).filter(([key]) => !CLI_OWNED_KEYS.has(key)), + ); +} + /** * Read + validate pharn.config.json. * @@ -220,7 +293,8 @@ export function configPath(cwd: string): string { * On success the validated, typed `models`/`seam` (the validators' stripped * return) replace the raw sub-blocks (BUG 3), while unknown TOP-LEVEL keys still * pass through so a legacy config carrying a since-removed field still loads - * (P7, additive). + * (P7, additive) — and so a key the user owns (`userOwnedConfigEntries`) survives + * every command that writes this object back. */ export function readPharnConfig(cwd: string): PharnConfig | null { const path = configPath(cwd); diff --git a/src/steps/install-archetype.ts b/src/steps/install-archetype.ts index 1841ded..e2e309a 100644 --- a/src/steps/install-archetype.ts +++ b/src/steps/install-archetype.ts @@ -1,3 +1,4 @@ +import { readFileSync } from 'node:fs'; import { log, outro, spinner } from '@clack/prompts'; import pc from 'picocolors'; import { FIRST_FEATURE_COMMAND, REPO_URL } from '../lib/constants.js'; @@ -10,8 +11,13 @@ import { buildRecords, writeRecords } from '../lib/install-records.js'; import { DEFAULT_MODEL_ROUTING } from '../lib/model-routing.js'; import { formatModelRoutingLines } from '../lib/model-routing-format.js'; import { DEFAULT_SEAM_CONFIG } from '../lib/seam-config.js'; -import { writePharnConfig } from '../lib/pharn-config.js'; +import { + configPath, + userOwnedConfigEntries, + writePharnConfig, +} from '../lib/pharn-config.js'; import { readSkillsVersion } from '../lib/skills-version.js'; +import { isPlainObject } from '../lib/validate.js'; import { PHARN_VERSION } from '../version.js'; import type { Archetype, @@ -155,7 +161,16 @@ export async function runInstallArchetype( collectExpectedInstallPaths({ repoDir, capabilities, layout }).keys(), ), }); - await writePharnConfig(cwd, config); + // The config this install replaces may hold keys pharn does not own — + // upstream's `testResults` / `ship`, which users add by hand — and the object + // above is built from pharn's own fields alone. Carry them over (why every + // such key: userOwnedConfigEntries). Read HERE, inside init's project lock and + // immediately before the write, for the reason the backup scan above is taken + // late: a key edited while a prompt was open must not be lost. Appended AFTER + // pharn's own keys: the two sets are disjoint by construction, and this keeps + // the user's keys where they most likely added them, so the committed file's + // diff is only what init actually changed. + await writePharnConfig(cwd, { ...config, ...readCarriedEntries(cwd) }); const elapsed = ((Date.now() - startedAt) / 1000).toFixed(1); const check = pc.green('✔'); @@ -198,3 +213,29 @@ export async function runInstallArchetype( ].join('\n'), ); } + +/** + * The user-owned top-level entries (`userOwnedConfigEntries`) of the + * pharn.config.json this install is about to replace — `{}` on ANY failure. + * Read TOLERANTLY, like init's carriedManualCapabilities: init is the command + * every other one points at for recovery, so an absent, unreadable or + * unparseable config means "nothing to carry over", never a refusal. + * + * Deliberately NOT readPharnConfig, whose verdict is about the keys pharn OWNS: + * it returns null for a config with no `modules` array — which every other + * command answers with "Run `pharn init` first" — and throws on a bad + * `models`/`seam` hand-edit. Neither says anything about the user's own keys, + * and reading through it would drop `testResults` on exactly the recovery path + * pharn prescribes. So the one requirement is a JSON object at top level — the + * same shape-only read steps/overwrite-check.ts makes for its one display + * scalar, and file-local for the same reason: a total-catch reader must not be + * importable from the module whose point is that it throws. + */ +function readCarriedEntries(cwd: string): Record { + try { + const raw: unknown = JSON.parse(readFileSync(configPath(cwd), 'utf8')); + return isPlainObject(raw) ? userOwnedConfigEntries(raw) : {}; + } catch { + return {}; + } +} diff --git a/src/types.ts b/src/types.ts index e655815..633b86b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -113,6 +113,12 @@ export interface SeamConfig { haltOnUnknown?: boolean; } +// Every key declared here is pharn-OWNED: `pharn init` rewrites it from scratch +// (CLI_OWNED_KEYS in src/lib/pharn-config.ts must list it — the typecheck +// enforces that). Keys the user adds that are NOT declared — upstream pharn-oss's +// `testResults` and `ship` — are user-owned and survive every command, init +// included (userOwnedConfigEntries). So declaring an upstream key here is not a +// harmless typing convenience: it would make init start dropping it. export interface PharnConfig { pharnVersion: string; skillsVersion: string; diff --git a/tests/init-archetype.test.ts b/tests/init-archetype.test.ts index 166c0e1..dfcbdd2 100644 --- a/tests/init-archetype.test.ts +++ b/tests/init-archetype.test.ts @@ -483,3 +483,210 @@ describe('re-running init over an existing install (PHARN-11)', () => { ).toBe(true); }); }); + +// Upstream pharn-oss (6.15.0+) reads top-level keys this CLI does not own and +// users add by hand: `testResults` (the per-test results runners `/pharn-test` +// and `/pharn-verify` read — without it `/pharn-loop` stops `blocked: +// no-test-runner`) and `ship.requireAttestation`. add/update/remove keep them +// (readPharnConfig's spread); a re-run init rebuilt the config from its own +// fields alone and dropped them. +describe('re-running init keeps the config keys pharn does not own', () => { + const tmp = useTmpDir(); + + const testResults = { test: 'vitest-json', 'test:e2e': 'playwright-json' }; + const ship = { requireAttestation: true }; + + async function install(repo: string, proj: string): Promise { + const { archetypes } = detectArchetypesFromProject(proj); + const selection = resolveCapabilities( + archetypes, + parseCapabilityIndex(repo), + ); + await runInstallArchetype(repo, proj, archetypes, selection, 'sha123'); + } + + async function firstInstall(repo: string, proj: string): Promise { + scaffoldRepo(repo); + write( + join(proj, 'package.json'), + JSON.stringify({ dependencies: { next: '14.0.0' } }), + ); + await install(repo, proj); + } + + const configFile = (proj: string): string => join(proj, 'pharn.config.json'); + const readRaw = (proj: string): Record => + JSON.parse(readFileSync(configFile(proj), 'utf8')) as Record< + string, + unknown + >; + // Hand-edit the installed config, the way upstream's README says to. + const handEdit = ( + proj: string, + edit: (config: Record) => void, + ): void => { + const config = readRaw(proj); + edit(config); + writeFileSync(configFile(proj), JSON.stringify(config, null, 2)); + }; + + it('keeps a valid testResults block (and ship) from the config it replaces', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + const cliKeys = Object.keys(readRaw(proj)); + handEdit(proj, (c) => { + c.testResults = testResults; + c.ship = ship; + }); + + await install(repo, proj); + + const written = readRaw(proj); + expect(written.testResults).toEqual(testResults); + expect(written.ship).toEqual(ship); + // Verbatim, and nothing else: the fresh install's own keys plus exactly the + // two the user wrote. + expect(Object.keys(written).sort()).toEqual( + [...cliKeys, 'ship', 'testResults'].sort(), + ); + // Still a config every other command loads, carried keys included. + expect(readPharnConfig(proj)).toMatchObject({ testResults, ship }); + }); + + it('never carries a key pharn owns — init rewrites every one of them', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + const fresh = readRaw(proj); + handEdit(proj, (c) => { + c.testResults = testResults; + // State an earlier run recorded, stale the moment init reinstalls. + c.pharnVersion = '0.0.1'; + c.skillsVersion = '0.0.1'; + c.repo = 'someone/else'; + c.commit = 'stale-sha'; + c.pendingSkillsVersion = '0.0.2'; + c.frozenCapabilities = ['lens:path-traversal']; + c.layout = 'pharn'; + c.archetypes = ['backend']; + c.capabilities = [ + { name: 'path-traversal', role: 'lens', source: 'auto' }, + ]; + c.models = { + default: { model: 'haiku-4-5', effort: 'low' }, + stages: {}, + }; + c.seam = { resolutionOrder: ['ask'] }; + // Module-era fields: nothing writes them any more, but they are still + // pharn's, not the user's. + c.modules = [{ name: 'core', version: '1.0.0' }]; + c.constitution = 'minimal'; + c.isMultiTenant = true; + c.stackAnswers = { db: 'postgres' }; + c.installedSkills = [{ skill: 'x', from: 'y' }]; + }); + + await install(repo, proj); + + // Exactly what a fresh install writes, plus the one key pharn does not own. + expect({ ...readRaw(proj), installedAt: null }).toEqual({ + ...fresh, + installedAt: null, + testResults, + }); + }); + + // The recovery path pharn itself prescribes: a config with no `modules` array + // is answered by every other command with "No pharn.config.json found. Run + // `pharn init` first." Taking that advice must not cost the user their keys — + // nor may a bad hand-edit of a block pharn owns. + it.each<[string, (config: Record) => void]>([ + [ + 'has no `modules` array ("run `pharn init`")', + (c) => { + delete c.modules; + }, + ], + [ + 'has an invalid `seam` block (a named hand-edit error)', + (c) => { + c.seam = { resolutionOrder: ['model'] }; + }, + ], + ])( + 'keeps them from a config that every other command refuses: it %s', + async (_label, damage) => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + handEdit(proj, (c) => { + c.testResults = testResults; + damage(c); + }); + // The premise: the command-side reader will not use this config. + let usable: boolean; + try { + usable = readPharnConfig(proj) !== null; + } catch { + usable = false; + } + expect(usable).toBe(false); + + await install(repo, proj); + + expect(readRaw(proj).testResults).toEqual(testResults); + expect(readPharnConfig(proj)).not.toBeNull(); + }, + ); + + it.each([ + ['not JSON', '{ "testResults": '], + ['a JSON array', '[{ "testResults": {} }]'], + ['a JSON scalar', '"testResults"'], + ])( + 'carries nothing, and still installs, when the replaced config is %s', + async (_label, text) => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + writeFileSync(configFile(proj), text); + + await install(repo, proj); + + const written = readRaw(proj); + expect(written).not.toHaveProperty('testResults'); + expect(written.skillsVersion).toBe('1.0.0'); + }, + ); + + // Carried as DATA. Keys named like Object.prototype members are the user's + // keys like any other (an `in` test against a plain object would read them as + // pharn's), and `__proto__` — an OWN key after JSON.parse — must round-trip as + // a key, never become the written object's prototype. + it('carries keys named like Object.prototype members as plain data', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + writeFileSync( + configFile(proj), + readFileSync(configFile(proj), 'utf8').replace( + /^\{/, + '{\n "__proto__": { "polluted": true },\n "constructor": "mine",\n "toString": 1,', + ), + ); + + await install(repo, proj); + + const written = readRaw(proj); + expect( + Object.getOwnPropertyDescriptor(written, '__proto__')?.value, + ).toEqual({ polluted: true }); + expect(Object.getPrototypeOf(written)).toBe(Object.prototype); + expect(Object.getOwnPropertyDescriptor(written, 'constructor')?.value).toBe( + 'mine', + ); + expect(Object.getOwnPropertyDescriptor(written, 'toString')?.value).toBe(1); + expect(({} as Record).polluted).toBeUndefined(); + }); +});