morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #2958] [Bug] Failed skill upgrades can advertise an older backup that cannot restore the pre-upgrade installation #1042

New issue

Closed

opened 2026-10-15 18:30:36 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-15 18:30:36 +00:00

Owner

Originally created by @dakjdakd on GitHub (Oct 13, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/2958

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Setup, packaging, or upgrade

Observed behavior

A failed managed-skill upgrade can report an existing backup as the recovery location even though that backup contains instructions from a much earlier installation. If the upgrade has already published the new content and its rollback also fails, the reported file cannot restore the installation that existed immediately before the upgrade.

The sequence is easiest to explain with three versions of a single managed file:

File/state	Contents before the operation
`SKILL.md.rea.backup`	V0: the first snapshot retained by an earlier setup
`SKILL.md`	V2: the currently installed instructions
Canonical skill bundle	V3: the instructions this setup intends to install

During the attempted V2 → V3 upgrade, backup preparation encounters the existing V0 .rea.backup. It treats that condition as success without establishing that V2 has been preserved. The writer then publishes V3 and rejects; the transaction attempts to restore V2, but the rollback write also rejects. SetupSkillInstallError.recoveryBackupPaths nevertheless points to the V0 file.

Both write failures matter to this report. A normal successful upgrade does not produce the misleading recovery error, and a successful rollback restores V2 from the transaction's in-memory original. The defect becomes consequential precisely when rollback cannot complete and the backup is the remaining recovery option.

I reproduced this with real temporary files and the production skill installer. A separate probe retained the default backup operation and actual atomic writer, injecting only rejection after publication and failure of the rollback write. On the affected revision, the advertised recovery file contained V0; it did not contain V2.

Reproduction

Use a source checkout at the revision below, the pinned toolchain, and its generated skill bundle:

npm ci
npm run build:cached

The smallest deterministic reproduction uses the existing SkillInstallFileOperations boundary to create the two necessary failure conditions. This avoids relying on machine-specific permission timing while still exercising real filesystem copies and atomic publication. Save the following as reproduce-skill-recovery.mjs in the repository root and run node reproduce-skill-recovery.mjs:

import assert from 'node:assert/strict';
import { constants } from 'node:fs';
import { copyFile, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import writeFileAtomic from 'write-file-atomic';
import { installCanonicalSkill } from './dist/application/SetupSkill.js';

const home = await mkdtemp(join(tmpdir(), 'rea-skill-recovery-'));
const manifest = join(home, '.agents/skills/reverse-engineer-anything/SKILL.md');
const current = 'current instructions V2\n';
const first = 'first instructions V0\n';
let writes = 0;

try {
  await mkdir(dirname(manifest), { recursive: true });
  await writeFile(manifest, current);
  await writeFile(`${manifest}.rea.backup`, first);

  const result = await installCanonicalSkill(home, [], {}, process.platform, {
    mkdir: async (path) => { await mkdir(path, { recursive: true }); },
    backup: async (source, target) => {
      try {
        await copyFile(source, target, constants.COPYFILE_EXCL);
      } catch (cause) {
        // Matches the installer's existing first-backup behavior.
        if (cause?.code !== 'EEXIST') throw cause;
      }
    },
    write: async (path, content, mode) => {
      writes += 1;
      if (writes === 1) {
        await writeFileAtomic(path, content, { encoding: 'utf8', mode });
        throw new Error('injected rejection after publication');
      }
      throw new Error('injected rollback failure');
    },
    remove: async (path) => { await rm(path, { force: true }); },
  });

  assert.equal(result.ok, false);
  assert.equal(result.error.stage, 'write');
  assert.equal(result.error.rollbackFailures.length, 1);
  assert.notEqual(await readFile(manifest, 'utf8'), current);
  const recovery = result.error.recoveryBackupPaths[0];
  console.log({ recovery, recoveryContents: await readFile(recovery, 'utf8') });
  assert.equal(await readFile(recovery, 'utf8'), current);
} finally {
  await rm(home, { recursive: true, force: true });
}

The final assertion fails on the affected revision: recovery contents are first instructions V0\n, while the expected recoverable original is current instructions V2\n. The canonical generated skill takes the role of V3; no particular released version number is required to demonstrate the difference.

Expected behavior

Before replacing an existing managed file, setup should establish that a persistent recovery snapshot contains the original content read for this transaction. Preserving the first-ever snapshot is useful, but its existence does not establish that it can restore a later installation.

If the first .rea.backup already matches V2, setup can reuse it. If it contains V0, setup should preserve V0 and save V2 separately. The configuration setup workflow already has a suitable convention: <first-backup-path>.<sha256-of-current-content>.recovery.

The snapshot should be read back and checked before publication begins. A directory, unreadable file, or different content occupying the selected recovery path should produce a backup-stage failure with no canonical files written. If publication and rollback later fail, recoveryBackupPaths and the recovery message should identify the verified V2 snapshot, rather than reconstructing the first-backup path from the destination name.

Impact and scope

This affects recovery from managed-skill installation or repair after an earlier backup has been retained. It applies to SKILL.md and managed reference files, because they share the same backup and error-path logic. A partial bundle write can make accurate per-file recovery paths particularly useful.

The user-facing consequence is restoration of the wrong instructions during an already failed upgrade. The error's recommendation is actionable, but following it would revert farther than the installation immediately preceding the operation. The first snapshot is valid historical data; the mistake is presenting it as the current transaction's recovery snapshot.

This report concerns persistent recovery availability and the correctness of recovery guidance. It does not imply an analysis-provider failure or a change to Evidence identity. It also does not claim protection against another process replacing a backup after verification, filesystem hardware faults, or arbitrary edits being retained when setup successfully installs its canonical managed bundle.

REA version

Source revision ebaaa79ca02bde4a73013a2a9a781d0b07a57ee4 (package metadata: 6.4.0).

Runtime and platform

Node.js 24.18.0; npm 11.16.0; Windows x64. No native analysis provider is needed for this filesystem reproduction.

Target and artifact context

A managed reverse-engineer-anything skill bundle in a disposable client home, with an existing first-backup snapshot whose contents differ from the currently installed file.

Interface

Package or setup

Earliest failure stage

Setup or provider admission

Backup preparation first accepts an unverified historical snapshot. The incorrect recovery recommendation becomes observable after publication and rollback have both failed.

Relevant output or evidence

The decisive result fields on the affected revision are stage: "write", one entry in rollbackFailures, and recoveryBackupPaths containing SKILL.md.rea.backup. Reading that advertised path returns V0. The reproduction checks those fields and compares the actual file bytes rather than relying on the error string alone.

Typed outcome or recovery expected

Keep SetupSkillInstallError and its current failure-stage/rollback information. For attempted replacements of existing files, populate recoveryBackupPaths only with snapshot paths validated against the transaction's original contents. If backup validation fails, return a backup-stage error before attempting any skill publication.

Analysis or suspected cause

Source-derived cause: systemSkillInstallFileOperations.backup returns normally on COPYFILE_EXCL EEXIST. installCanonicalSkill then proceeds without comparing that file with original.content, and its catch block creates recovery paths as ${destination}.rea.backup. There is no retained mapping from a destination to the snapshot actually valid for this transaction.

SetupClientConfiguration already owns selectCurrentRecoveryBackup, which compares snapshot bytes and selects a content-specific recovery path when the first backup is older. Sharing that existing owner with skill setup would address the producing boundary and avoid two independent interpretations of the same backup convention.

Additional context

Useful regression coverage includes an older first backup plus failed publication/rollback, a successful upgrade retaining both snapshots, reruns that reuse the matching snapshot, an occupied recovery directory, different bytes at a recovery path, and a copy operation whose output fails readback validation. These cases distinguish preservation of historical data from availability of the current recovery snapshot.

The fix is available in PR #2959, from the independent fix/skill-recovery-snapshots branch. That PR addresses this issue only.

Originally created by @dakjdakd on GitHub (Oct 13, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/2958 ## Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ## Area Setup, packaging, or upgrade ## Observed behavior A failed managed-skill upgrade can report an existing backup as the recovery location even though that backup contains instructions from a much earlier installation. If the upgrade has already published the new content and its rollback also fails, the reported file cannot restore the installation that existed immediately before the upgrade. The sequence is easiest to explain with three versions of a single managed file: | File/state | Contents before the operation | | --- | --- | | `SKILL.md.rea.backup` | V0: the first snapshot retained by an earlier setup | | `SKILL.md` | V2: the currently installed instructions | | Canonical skill bundle | V3: the instructions this setup intends to install | During the attempted V2 → V3 upgrade, backup preparation encounters the existing V0 `.rea.backup`. It treats that condition as success without establishing that V2 has been preserved. The writer then publishes V3 and rejects; the transaction attempts to restore V2, but the rollback write also rejects. `SetupSkillInstallError.recoveryBackupPaths` nevertheless points to the V0 file. Both write failures matter to this report. A normal successful upgrade does not produce the misleading recovery error, and a successful rollback restores V2 from the transaction's in-memory original. The defect becomes consequential precisely when rollback cannot complete and the backup is the remaining recovery option. I reproduced this with real temporary files and the production skill installer. A separate probe retained the default backup operation and actual atomic writer, injecting only rejection after publication and failure of the rollback write. On the affected revision, the advertised recovery file contained V0; it did not contain V2. ## Reproduction Use a source checkout at the revision below, the pinned toolchain, and its generated skill bundle: ```sh npm ci npm run build:cached ``` The smallest deterministic reproduction uses the existing `SkillInstallFileOperations` boundary to create the two necessary failure conditions. This avoids relying on machine-specific permission timing while still exercising real filesystem copies and atomic publication. Save the following as `reproduce-skill-recovery.mjs` in the repository root and run `node reproduce-skill-recovery.mjs`: ```js import assert from 'node:assert/strict'; import { constants } from 'node:fs'; import { copyFile, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import writeFileAtomic from 'write-file-atomic'; import { installCanonicalSkill } from './dist/application/SetupSkill.js'; const home = await mkdtemp(join(tmpdir(), 'rea-skill-recovery-')); const manifest = join(home, '.agents/skills/reverse-engineer-anything/SKILL.md'); const current = 'current instructions V2\n'; const first = 'first instructions V0\n'; let writes = 0; try { await mkdir(dirname(manifest), { recursive: true }); await writeFile(manifest, current); await writeFile(`${manifest}.rea.backup`, first); const result = await installCanonicalSkill(home, [], {}, process.platform, { mkdir: async (path) => { await mkdir(path, { recursive: true }); }, backup: async (source, target) => { try { await copyFile(source, target, constants.COPYFILE_EXCL); } catch (cause) { // Matches the installer's existing first-backup behavior. if (cause?.code !== 'EEXIST') throw cause; } }, write: async (path, content, mode) => { writes += 1; if (writes === 1) { await writeFileAtomic(path, content, { encoding: 'utf8', mode }); throw new Error('injected rejection after publication'); } throw new Error('injected rollback failure'); }, remove: async (path) => { await rm(path, { force: true }); }, }); assert.equal(result.ok, false); assert.equal(result.error.stage, 'write'); assert.equal(result.error.rollbackFailures.length, 1); assert.notEqual(await readFile(manifest, 'utf8'), current); const recovery = result.error.recoveryBackupPaths[0]; console.log({ recovery, recoveryContents: await readFile(recovery, 'utf8') }); assert.equal(await readFile(recovery, 'utf8'), current); } finally { await rm(home, { recursive: true, force: true }); } ``` The final assertion fails on the affected revision: recovery contents are `first instructions V0\n`, while the expected recoverable original is `current instructions V2\n`. The canonical generated skill takes the role of V3; no particular released version number is required to demonstrate the difference. ## Expected behavior Before replacing an existing managed file, setup should establish that a persistent recovery snapshot contains the original content read for this transaction. Preserving the first-ever snapshot is useful, but its existence does not establish that it can restore a later installation. If the first `.rea.backup` already matches V2, setup can reuse it. If it contains V0, setup should preserve V0 and save V2 separately. The configuration setup workflow already has a suitable convention: `<first-backup-path>.<sha256-of-current-content>.recovery`. The snapshot should be read back and checked before publication begins. A directory, unreadable file, or different content occupying the selected recovery path should produce a backup-stage failure with no canonical files written. If publication and rollback later fail, `recoveryBackupPaths` and the recovery message should identify the verified V2 snapshot, rather than reconstructing the first-backup path from the destination name. ## Impact and scope This affects recovery from managed-skill installation or repair after an earlier backup has been retained. It applies to `SKILL.md` and managed reference files, because they share the same backup and error-path logic. A partial bundle write can make accurate per-file recovery paths particularly useful. The user-facing consequence is restoration of the wrong instructions during an already failed upgrade. The error's recommendation is actionable, but following it would revert farther than the installation immediately preceding the operation. The first snapshot is valid historical data; the mistake is presenting it as the current transaction's recovery snapshot. This report concerns persistent recovery availability and the correctness of recovery guidance. It does not imply an analysis-provider failure or a change to Evidence identity. It also does not claim protection against another process replacing a backup after verification, filesystem hardware faults, or arbitrary edits being retained when setup successfully installs its canonical managed bundle. ## REA version Source revision `ebaaa79ca02bde4a73013a2a9a781d0b07a57ee4` (package metadata: 6.4.0). ## Runtime and platform Node.js 24.18.0; npm 11.16.0; Windows x64. No native analysis provider is needed for this filesystem reproduction. ## Target and artifact context A managed `reverse-engineer-anything` skill bundle in a disposable client home, with an existing first-backup snapshot whose contents differ from the currently installed file. ## Interface Package or setup ## Earliest failure stage Setup or provider admission Backup preparation first accepts an unverified historical snapshot. The incorrect recovery recommendation becomes observable after publication and rollback have both failed. ## Relevant output or evidence The decisive result fields on the affected revision are `stage: "write"`, one entry in `rollbackFailures`, and `recoveryBackupPaths` containing `SKILL.md.rea.backup`. Reading that advertised path returns V0. The reproduction checks those fields and compares the actual file bytes rather than relying on the error string alone. ## Typed outcome or recovery expected Keep `SetupSkillInstallError` and its current failure-stage/rollback information. For attempted replacements of existing files, populate `recoveryBackupPaths` only with snapshot paths validated against the transaction's original contents. If backup validation fails, return a backup-stage error before attempting any skill publication. ## Analysis or suspected cause Source-derived cause: `systemSkillInstallFileOperations.backup` returns normally on `COPYFILE_EXCL` EEXIST. `installCanonicalSkill` then proceeds without comparing that file with `original.content`, and its catch block creates recovery paths as `${destination}.rea.backup`. There is no retained mapping from a destination to the snapshot actually valid for this transaction. `SetupClientConfiguration` already owns `selectCurrentRecoveryBackup`, which compares snapshot bytes and selects a content-specific recovery path when the first backup is older. Sharing that existing owner with skill setup would address the producing boundary and avoid two independent interpretations of the same backup convention. ## Additional context Useful regression coverage includes an older first backup plus failed publication/rollback, a successful upgrade retaining both snapshots, reruns that reuse the matching snapshot, an occupied recovery directory, different bytes at a recovery path, and a copy operation whose output fails readback validation. These cases distinguish preservation of historical data from availability of the current recovery snapshot. The fix is available in [PR #2959](https://github.com/morluto/rea/pull/2959), from the independent `fix/skill-recovery-snapshots` branch. That PR addresses this issue only.

bsakaguchi

2026-10-15 18:30:36 +00:00

closed this issue
added the
area: setup

bug
labels

bsakaguchi commented

2026-10-15 18:30:37 +00:00

Author

Owner

@morluto commented on GitHub (Oct 14, 2026):

Addressed on main in ec56c1da0.

Failed upgrades retain a byte-exact recovery snapshot of the immediate pre-upgrade installation while preserving older historical backups.

Validation: Historical-backup and invalid-UTF8 recovery regressions passed; package setup/readback/recovery verification passed.

Verification scope: Persistent cleanup failures remain reported with retained ownership.

@morluto commented on GitHub (Oct 14, 2026): Addressed on main in [ec56c1da0](https://github.com/morluto/rea/commit/ec56c1da0). Failed upgrades retain a byte-exact recovery snapshot of the immediate pre-upgrade installation while preserving older historical backups. Validation: Historical-backup and invalid-UTF8 recovery regressions passed; package setup/readback/recovery verification passed. Verification scope: Persistent cleanup failures remain reported with retained ownership.

bsakaguchi referenced this issue

2026-10-16 21:02:34 +00:00

[GH-ISSUE #2383] [Tracker] Audit analysis and debugging coverage for the TIOBE top 100 languages (October 2026) #794

bsakaguchi referenced this issue

2026-10-17 21:08:05 +00:00

[PR #1042] [MERGED] fix(javascript): reconstruct HTML script loads accurately #2601