FE-1320: Structural private-mission isolation for /compare-specs - #415
Conversation
lunelson
changed the title
PR SummaryLow Risk Overview Evidence retention — teardown now saves harness-visible transcripts from the controller-owned Tests and docs — Reviewed by Cursor Bugbot for commit 1a721c7. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
This PR hardens /compare-specs private-mission isolation by changing the workflow to allocate each harness a fresh, system-temporary external target root outside controller-owned trees, and updates the written procedure + planning docs to match.
Changes:
- Update the
/compare-specsoperator prompt to require per-harness external target roots and specify copy/cleanup sequencing. - Add/extend tests around the prompt contract and the external-target placement invariants.
- Update SPEC/PLAN/testing notes to record FE-1320 as the closeout of the prior placement-risk finding.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TESTING_FINDINGS.md | Updates CS2 disposition to reflect FE-1320 closing the placement risk. |
| src/dev/tests/compare-specs-prompt.test.ts | Adds prompt assertions and filesystem-based isolation/retention tests. |
| memory/SPEC.md | Updates D134-L and I67-L to include external target placement boundary language. |
| memory/PLAN.md | Marks the isolation hardening frontier as completed and removes it from “Next”. |
| docs/archive/PLAN_HISTORY.md | Archives the FE-1320 closeout entry and its evidence summary. |
| .pi/prompts/compare-specs.md | Implements the new external-target-root procedure and retention/cleanup ordering in the operator prompt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
4f5538e to
d63c33a
Compare
14deb5d to
1a721c7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/dev/tests/compare-specs-prompt.test.ts:17
pathExiststreats anyaccess()failure as “does not exist”. That can make the test pass even if the path exists but is unreadable (e.g.,EACCES). Restrict the false case toENOENTand rethrow other errors so the assertion is reliable.
const pathExists = async (path: string): Promise<boolean> => {
try {
await access(path);
return true;
} catch {
src/dev/tests/compare-specs-prompt.test.ts:100
- These assertions rely on filesystem enumeration order (
readdir/ recursive traversal). Directory entry order is not guaranteed across platforms/filesystems, so this can be flaky in CI. Sort the collected names/paths (or usetoContain) before asserting exact equality.
const lsVisible = await readdir(target);
expect(lsVisible).toEqual(['visible.md']);
const findVisible = await findEntries(target);
expect(findVisible.map((entry) => relative(target, entry))).toEqual(['visible.md']);
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a721c7. Configure here.
kostandinang
left a comment
kostandinang
left a comment
There was a problem hiding this comment.
Placing each harness in a fresh temp root closes the known ../../private-mission.md walk. The prompt and SPEC already say this is not an OS sandbox, which matches what the tests actually prove.
d63c33a to
e705386
Compare
1a721c7 to
ec06b46
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.pi/prompts/compare-specs.md:96
- The prompt now relies on target placement to prevent reading controller-private files, but it still doesn't explicitly require rejecting adversarial document paths that escape the external target root (e.g. absolute paths,
../traversal, or symlink escapes) when locating/copying the harness-authored output. This leaves a gap vs the PR's stated "reject adversarial paths" goal and risks copying unintended host/controller files into retained evidence.
7. Locate the harness-authored document if it exists without altering it. Never author, reconstruct, complete, rewrite, or improve it. Missing or partial output remains missing or partial.
src/dev/tests/compare-specs-prompt.test.ts:62
- This test suite validates the new placement language, but it doesn't assert the prompt requires rejecting adversarial paths that would escape the external target root when resolving/copying the harness-authored output (absolute paths,
../traversal, symlink escapes). Adding explicit prompt assertions here will lock in the "reject escape paths" contract and prevent regressions.
expect(prompt).toContain(
'It is not an OS sandbox and does not claim isolation from unrestricted absolute-path or whole-host discovery',
);
Place each harness in a fresh external temporary root so controller-private mission material is absent from ordinary target-relative traversal while retained evidence stays exact. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the external-target contract coupled to the existing single-actor, immutable-evidence, and prompt-owned workflow boundaries. Co-authored-by: Cursor <cursoragent@cursor.com>
Return I67-L to its pre-frontier content so any evidence refresh remains owned by ln-sync. Co-authored-by: Cursor <cursoragent@cursor.com>
ec06b46 to
f1c74bf
Compare
e705386 to
29dc700
Compare
Merge activity
|
Why
The
/compare-specscontroller holds private mission material that comparison targets must never be able to discover through their workspace or relative paths.What
How to test