Skip to content

pstack statechart: output replacement deletes foreign nested files #773

Open

Description

@raiscan

Summary

The new pstack statechart tool can silently delete unrelated nested files when replacing an existing output folder. foreign() checks only immediate entry names, while publish() recursively removes the previous output directory. An allowed name can therefore hide foreign descendants or even be a directory where a generated file is expected.

This contradicts the preservation contract described in #741: a run should refuse files it did not write rather than overwrite/delete them.

Affected source

Verified at commit eb4db2b55b2b7dec3d9282754cddf0a56bf4ff10:

Also rechecked against the current main source returned by GitHub at reporting time, blob 6e2ebfccdafff7900016b5cf2ed5384c84181545: the same isolated reproduction below confirms all three cases.

Observed behavior

For newly created disposable output fixtures:

  • tlc/notes.md: foreign(output) returns []; publication deletes the sentinel.
  • chart.mmd/notes.md: a directory named like an expected generated file is accepted; publication deletes its sentinel.
  • .DS_Store/notes.md: the same type-confusion case is accepted and deleted.

This does not require overlapping runs. It affects the output-replacement path, including an explicitly supplied --out directory. A root-level foreign notes.md is correctly refused by the existing check; nested/type-confusion cases are missed.

A separate symlink probe removed an accepted tlc symlink but preserved its target contents. This report does not claim recursive deletion through symlinks into their targets.

Minimal isolated reproduction (no TLC required)

Save this as repro.cjs in a checkout, then run:

node repro.cjs pstack/skills/statechart/scripts/statechart.mjs

The probe extracts and executes the unchanged actual ownership/publication function bodies in a Node VM. It creates only fresh temporary fixtures; it does not run a full TLC invocation or touch an existing output directory. No user data is needed. The fixture tree is left in the temporary directory for inspection.

// Isolated reproduction: only newly created disposable fixtures are touched.
const fs = require('node:fs');
const path = require('node:path');
const os = require('node:os');
const vm = require('node:vm');
const assert = require('node:assert/strict');
const source = fs.readFileSync(process.argv[2] || 'pstack/skills/statechart/scripts/statechart.mjs', 'utf8');
const start = source.indexOf('const OWN =');
const end = source.indexOf('function secondModel(', start);
assert(start >= 0 && end > start, 'Unexpected upstream source layout');
const context = { fs, path, rel: String };
vm.createContext(context);
vm.runInContext('class CannotFinish extends Error {}\n' + source.slice(start, end) + '\nglobalThis.probe = { foreign, publish };', context);
const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'statechart-ownership-repro-'));
for (const [name, relative] of [['nested-tlc', 'tlc/notes.md'], ['wrong-type-chart', 'chart.mmd/notes.md'], ['wrong-type-dsstore', '.DS_Store/notes.md']]) {
  const output = path.join(scratch, name, 'output');
  const work = path.join(scratch, name, 'work');
  const sentinel = path.join(output, relative);
  fs.mkdirSync(path.dirname(sentinel), { recursive: true });
  fs.mkdirSync(work, { recursive: true });
  fs.writeFileSync(sentinel, 'disposable sentinel');
  fs.writeFileSync(path.join(work, 'chart.mmd'), 'replacement');
  const foreign = Array.from(context.probe.foreign(output));
  context.probe.publish(work, output, new Map());
  const preserved = fs.existsSync(sentinel);
  console.log(JSON.stringify({ case: name, foreign, sentinelPreserved: preserved }));
  assert.equal(foreign.length, 0);
  assert.equal(preserved, false);
}

Observed output:

{"case":"nested-tlc","foreign":[],"sentinelPreserved":false}
{"case":"wrong-type-chart","foreign":[],"sentinelPreserved":false}
{"case":"wrong-type-dsstore","foreign":[],"sentinelPreserved":false}

Expected: reject the foreign nested content/wrong entry type, identify the offending path, and preserve the existing directory contents.

Suggested regression coverage / correction

  • Type-check reserved generated leaves; a directory named chart.mmd or .DS_Store must not imply ownership of all its descendants.
  • Validate the permitted tlc subtree rather than accepting everything beneath its top-level name; reject unexpected descendants, symlinks and special/unreadable entries without following them.
  • Apply preservation checks at the existing initial-selection and publication/moved-aside boundaries, including foreign content appearing during a run.
  • Keep safe rerun behavior for valid generated outputs, and ensure a failed replacement restores or retains the original contents with their location reported.
  • Treat concurrent mutation separately: a recursive scan alone does not establish that a subsequent recursive deletion cannot race a writer.

Verification and limits

  • Environment: Node v22.22.2, Linux aarch64.
  • Existing statechart test suite: 27 passed, 31 skipped, 0 failed; TLC is unavailable here. Existing root-foreign-file cases do not exercise the nested/type cases above.
  • The isolated ownership/publication reproduction above was executed successfully. It is not a claim of a full Java/TLC or PNG end-to-end run.

Activity

  1. deanrie commented on Oct 23, 2026

    @deanrie

    Hey @raiscan, reproduced on today's main, after #776 and #779 moved the lines: OWN is still matched against immediate entry names only (foreign(), now L668), with no recursion and no isDirectory check, and publish() (L757) ends with fs.rmSync(old, { recursive: true, force: true }) after refuseForeign(out, foreign(old)) has passed. So tlc/notes.md, a directory named chart.mmd, and a directory named .DS_Store all survive the check and go with the folder; a root-level notes.md is refused as you say. The symlink observation matches too: the link is removed, the target is left. The fix that keeps #741's contract is a recursive foreign() that owns tlc only as a directory of the files the run writes there and treats an owned name of the wrong type as foreign; your VM probe is a ready-made test input for the three cases. @poteto has been landing statechart fixes daily, so this is his to take first; if it is still open at midday I will put up a PR with those tests.

  2. deanrie commented on Oct 23, 2026

    @deanrie

    Hey @raiscan, #785 is up with the fix and your three fixtures plus two more (a symlink named chart.mmd, refused with its target untouched, and a folder of only owned files passing to the next step). OWN now covers top-level files only, OWN_TLC covers what the script writes into tlc/ (*.cfg, *.out, *.dot, regions-proof.mmd, regions-all.mmd), foreign() reads entry types and looks one level into tlc/, and a folder or link under an owned name is reported by its path. If your VM probe still has the fixtures around, running it against the branch is the one check I could not do with your exact setup.

  3. added a commit that references this issue on Oct 24, 2026
    900f39f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions