FE-1320: Structural private-mission isolation for /compare-specs - #415
FE-1320: Structural private-mission isolation for /compare-specs#415lunelson wants to merge 5 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
| await rm(externalRoot, { recursive: true }); | ||
| await cp(scratchRun, retainedRun, { recursive: true }); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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>
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.
| - **Generic harnesses:** use a small instruction to conduct a question-led specification conversation and author the requested review-ready Markdown document. Do not preload mission facts or prescribe conclusions. | ||
| 3. Preflight only each selected harness's filesystem and adapter prerequisites, including the pinned `pi-interactive-shell` package and cleanup capability where applicable. Do not launch a synthetic conversation to test provider/model access. Report provider/model failure honestly if the real harness launch fails. Check Pi's adapter only when Pi is selected. | ||
| 4. Allocate a fresh isolated target cwd/session for each selected harness. This is the harness's comparison target, not necessarily the controller process cwd: Brunch's controller must launch from the Brunch repository root and address the fresh target through `--workspace <fresh-target-cwd>`, while a generic harness's structured spawn uses the fresh target cwd as its process cwd. The top-level session remains the shared simulated-user actor, so disclose harness order and shared actor context; do not claim fresh-per-harness actor isolation or equivalence to a rigorous campaign. | ||
| 4. Allocate a separate fresh system-temporary external target root for each selected harness using the host's standard temporary-directory facility. Resolve its canonical path and fail setup unless it is outside the controller checkout, scratch run tree, and retained run tree and its ancestor directories contain no `private-mission.md`, `harness-setup.md`, or other controller-private run material. This is the harness's comparison target, not necessarily the controller process cwd: Brunch's controller must launch from the Brunch repository root and address the external target through `--workspace <fresh-target-cwd>`, while a generic harness's structured spawn uses the external target as its process cwd. The top-level session remains the shared simulated-user actor, so disclose harness order and shared actor context; do not claim fresh-per-harness actor isolation or equivalence to a rigorous campaign. |
There was a problem hiding this comment.
Approval omits external target paths
Medium Severity
Step 4 now allocates per-harness system-temporary external target roots, but step 5 still only displays scratch and retained paths. Operators approve the isolation-critical placement without seeing those target roots, so a skipped or in-tree allocation can pass review unnoticed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1a721c7. Configure here.
| 7. Acquire the harness-authored document if it exists. Never author, reconstruct, complete, rewrite, or improve it. Missing or partial output remains missing or partial. | ||
| 8. On every outcome, retain state, harness-visible interaction, target-cwd/session identity, final process status, document that exists, and cleanup notes. Kill remaining processes, dismiss the completed shell record, and verify no comparison-harness shell or process remains before starting another. | ||
| 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. | ||
| 8. On every outcome, kill remaining processes and verify no comparison-harness shell or process remains. Before dismissing the completed shell record, save its exact harness-visible transcript from the controller-owned `interactive_shell` record into controller-owned scratch evidence. Copy only any unchanged harness-authored document from the external target into that scratch evidence; retain target-cwd/session identity, final process status, and cleanup notes. Then dismiss the shell record, remove that harness's external target root, and verify it is gone before starting another harness. |
There was a problem hiding this comment.
Retention conflicts with absolute targets
Medium Severity
The procedure now requires retaining target-cwd identity for system-temporary external roots while still forbidding workstation-absolute retained paths, and it never directs the established <ephemeral-workspace> placeholder. Agents following both rules either drop target identity or leak absolute temp paths into retained evidence.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1a721c7. Configure here.
Why
Our
/compare-specsworkflow runs two AI products side by side on the same task to compare their output. The instructions that drive that comparison are private, sensitive context that must never leak into what either product being tested can see or touch — including via something as simple as a relative file path that happens to reach outside the test's own folder.What
How to test
npm run test:comparisonpasses.