fix: strip HTML comments before preview/routing; refuse apply when the sealed sidecar is gone

[devin-ai-integration]

Summary

Four follow-ups to #26 — two from adversarially re-testing it, two from Devin Review on #26 (which merged before they could land there).

1. A secret hidden in an HTML comment still reached the model. #26 routed and previewed splitPrivate(input).clean, but comment stripping lived in stripPrivate() — so <!-- Bank account for payout: 12345678 --> was echoed verbatim by debrief --smart, debrief --dry-run and the ingest_propose MCP tool result, even though resume/dashboard stayed clean. Comments now go first, inside splitPrivate(), which is what both the preview and stripPrivate() read:

 function splitPrivate(md) {
-  const text = String(md || '')
+  const text = String(md || '')
+    .replace(/<!--[\s\S]*?-->/g, '')
+    .replace(/<!--[\s\S]*$/, '')

The second replace is new behavior: an unterminated comment now seals to EOF rather than leaking its tail, matching how an unclosed <private> is handled — fail closed.

2. A missing sidecar silently dropped the sealed note. #26's .debrief-private was written after .debrief-propose, so a symlinked sidecar hit refuseSymlinkWrite()'s process.exit(1) with the proposal already on disk (and left a stale .debrief-private.lock). A later debrief --apply then reported success while the sealed note existed nowhere — same outcome if a tool deletes the sidecar between propose and apply.

-write .debrief-propose ; write .debrief-private     // marker can outlive its content
+refuseSymlinkWrite(privatePath, { soft: true })     // check before taking the lock
+write .debrief-private ; write .debrief-propose     // seal first

and apply now fails closed rather than losing data:

sealed = readSealedProposal(eng)
if (!sealed.length && input.includes(PRIVATE_MARKER)) → exit 1
  "refused: the proposal seals a private note but .debrief-private is missing or unreadable
   - applying now would drop it silently."

3. An unclosed block poisoned context.md permanently. splitPrivate() returns an unclosed block exactly as read, so persisting it left a dangling opener — and every note appended afterwards (later ## Debrief, ## Session end capture, preserve) was swallowed into it and rendered as (private - redacted) forever. Normalized at the persistence boundary, so the scanner still returns what it actually read:

function sealedText(blocks) {
  return blocks
    .map(b => (/<\/private\b[^>]*>$/i.test(b.trim()) ? `${b}\n` : `${b}\n</private>\n`))
    .join('')
}

4. The sidecar existed world-readable before being chmodded. atomicWriteFile() created the temp file and renamed it at 0666 & ~umask, tightening to 0600 only afterwards. Mode is now set at creation:

-fs.writeFileSync(tmp, content)
+fs.writeFileSync(tmp, content, opts.mode ? { mode: opts.mode } : undefined)
+if (opts.mode) fs.chmodSync(tmp, opts.mode)
 fs.renameSync(tmp, p)

opts.mode is opt-in, so every existing caller is unchanged; the explicit chmodSync covers a permissive umask masking writeFileSync's mode.

Three regression tests: the comment matrix (terminated + unterminated, across --smart, --dry-run from stdin, the propose file and post-apply memory); the loss matrix (deleted sidecar refused; symlinked sidecar refused with the target untouched, no proposal left behind, no stale lock); and the unclosed-block matrix (balanced in the sidecar at 0600 and in context.md, with a later plain debrief note still visible in resume --full). 81/81 tests + 51 gates pass.

Link to Devin session: https://app.devin.ai/sessions/f135381c4682413bae73dff38eb6d1a3
Requested by: @suboss87

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 1 additional finding in Devin Review.

[devin-ai-integration]

🟡 A single stray comment opener hides all memory written after it

Everything after an unterminated <!-- is deleted from the text (.replace(/<!--[\s\S]*$/, '') at bin/fde.js:222) on every read of stored engagement memory, so notes recorded after such a line silently vanish from resume, triage, prep and the dashboard.
Impact: An FDE can lose sight of all decisions, risks and context recorded after one note that happens to contain a comment opener, with no warning anywhere.

Why the new EOF-truncating comment rule leaks out of the debrief-input path into persisted memory reads

splitPrivate() is not only used for previewing/routing pasted notes; it is the engine behind stripPrivate() (bin/fde.js:259-260) and therefore behind readClean() (bin/fde.js:283), which every model-facing view uses (bin/fde.js:1085, 1791, 2243, 2520 …).

Before this PR only the terminated form <!--[\s\S]*?--> was stripped, so a lone <!-- sitting in decisions.md/context.md was harmless. Now the second replace deletes the entire remainder of the file. A lone <!-- can reach memory through paths that do not go through splitPrivate(), e.g. fde log decision "migrate the API <!-- TODO confirm" (log entries pass only through stripControlChars), or through files the agent writes directly.

Because context.md is append-only, one such line permanently hides every later ## Debrief, ## Session end capture and preserve block from every view, while the file on disk still looks fine.

A narrower rule (e.g. only apply the EOF truncation to freshly ingested debrief/ingest input, or truncate only up to the end of the current line/block) keeps the fail-closed property for untrusted paste without blanking stored memory.

Prompt for agents

splitPrivate() in bin/fde.js now drops everything after an unterminated `<!--`. That is the desired fail-closed behavior for freshly pasted debrief/ingest input, but splitPrivate() is also the read path for all persisted memory (stripPrivate -> readClean), so a single stray `<!--` already stored in context.md/decisions.md (e.g. written by `fde log`, which only runs stripControlChars) now silently hides every line after it from resume, triage, prep, receipts and the dashboard. Consider scoping the EOF truncation to the untrusted-input entry points (writeProposal / routeDebriefInput input) rather than the generic read path, or making the truncation visible (replace with a redaction marker) so an FDE can tell that content was hidden.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Confirmed and fixed in 0ab7889. Reproduced it: a stray <!-- appended to context.md (reachable via fde log, which only runs stripControlChars) hid every later line from resume.

Fail-closed is right for untrusted paste but wrong for the read path, so the EOF seal is now opt-in and only the input entry points ask for it:

-function splitPrivate(md) {
-  const text = String(md || '')
-    .replace(/<!--[\s\S]*?-->/g, '')
-    .replace(/<!--[\s\S]*$/, '')
+function splitPrivate(md, opts = {}) {
+  let text = String(md || '').replace(/<!--[\s\S]*?-->/g, '')
+  if (opts.sealDangling) text = text.replace(/<!--[\s\S]*$/, '')

writeProposal() and routeDebriefInput() pass { sealDangling: true }; stripPrivate()/readClean() keep the pre-PR behavior, so a stray opener already in memory stays visible. Terminated comments are still stripped everywhere. Regression test appends a dangling <!-- to context.md and asserts the following note is still in resume --full.

[devin-ai-integration]

🟡 Confirming a debrief can be blocked forever when the notes merely mention the redaction wording

A proposal is refused at confirmation time (input.includes(PRIVATE_MARKER) at bin/fde.js:1473) whenever the notes contain the literal redaction wording, even though no private note ever existed, so the routed debrief can never be confirmed.
Impact: An FDE who pastes text that quotes the "(private - redacted)" wording gets a hard, unrecoverable refusal and must hand-edit an internal file to proceed.

Marker presence is used as a proxy for "a sealed block existed"

writeProposal() (bin/fde.js:1364-1379) only writes .debrief-private when blocks.length > 0, and splitPrivate() only emits PRIVATE_MARKER for real blocks — but the marker string can also arrive verbatim in the user's notes (very plausible: it is printed by fde resume/previews, so pasting earlier output or an exported context excerpt back into fde debrief --smart reproduces it).

In that case .debrief-private legitimately does not exist, readSealedProposal() returns [], and the new guard exits 1 with "missing or unreadable" on every fde debrief --apply / fde ingest apply. There is no --force escape; re-running propose reproduces the same state, so the only way out is manually editing .debrief-propose.

A robust check would record whether the propose step actually sealed blocks (e.g. a count in a small marker/receipt written alongside the proposal) instead of inferring it from the presence of the marker text in the proposal body.

Prompt for agents

cmdDebrief in bin/fde.js infers "the propose step sealed a private note" from the presence of the literal PRIVATE_MARKER string inside .debrief-propose. That string can legitimately appear in user-pasted notes (it is printed by resume/preview output), in which case no sidecar was ever written and apply refuses permanently with no bypass. Consider persisting the sealed-block count from writeProposal() (a tiny sidecar/receipt file, or a header line) and comparing that against readSealedProposal() results, so the refusal only fires when a sidecar really went missing.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Agreed, and fixed in 0ab7889 along the lines you suggest — the marker was a proxy, and since the CLI prints it, pasting earlier output back in was a realistic way to get permanently stuck.

writeProposal() now records the actual count, and apply compares against it:

withFileLock(sealPath, () => atomicWriteFile(sealPath, `${blocks.length}\n`))   // .debrief-seal

sealed = readSealedProposal(eng)
const expected = readSealCount(eng)   // null when there is no receipt
if (expected === null ? (!sealed.length && input.includes(PRIVATE_MARKER)) : sealed.length < expected) → exit 1

So notes that merely quote the wording produce .debrief-seal = 0 and apply proceeds; the refusal now fires only when a receipt says N blocks were sealed and fewer came back. The marker heuristic survives solely as the fallback for a proposal written before this change (no receipt), where refusing is still the safer default.

.debrief-seal joins MEMORY_EPHEMERAL and the memory .gitignore, and is unlinked with the other two after apply. Regression test drives notes containing a literal (private - redacted) through --smart → --apply and asserts it applies and the decision lands.

[devin-ai-integration]

Runtime verification — comment scoping + receipt-based sealed-block accounting (0ab7889)

Verified the two-fix delta end-to-end on 0ab7889 with the real CLI in sandboxed engagement roots, the ingest server driven by the official @modelcontextprotocol/sdk 1.30.0, a cross-build contrast against the pre-fix 02a0f60, and the dashboard checked in Chrome via view-source.

100/100 delta harness · 129/129 prior adversarial matrix · 18/18 MCP-over-SDK · clean-clone npm run check = 51 gates + 82/82 tests. No product failures found.

Fix 1 — read-path vs write-path scoping, proven by before/after contrast

Identical scenario on both builds: a decision containing a stray <!-- landed via fde log, then a later decision that must stay visible.

	🔴 pre-fix 02a0f60	🟢 this build 0ab7889
prep shows the later decision	0	1
dashboard shows the later decision	0	1

The same run confirms fresh untrusted input still seals: an unclosed <!-- Bank account for payout: 12345678 followed by a tail yields 0 secret hits and 0 tail hits in the model-facing preview, while the public decision still routes. Terminated comments remain stripped on both paths, and preview == apply holds.

The Fieldbook shows both the stray-comment decision and the one after it, with the private payout block sealed:

Chrome view-source: 12345678 → 0/0, private - redacted → 1/1, and context.md on disk still holds the secret — so the block was preserved, not lost.

Fix 2 — receipt accounting: false positive gone, still fail-closed

Notes containing the literal (private - redacted) now produce receipt 0, no sidecar, and apply cleanly — previously an unrecoverable refusal. All five sidecar-loss variants through both debrief --apply and ingest apply (10 cases) still refuse with exit 1, byte-identical memory and zero leakage.

attack	result
receipt + sidecar both deleted	refuses via marker fallback
receipt says 2, sidecar sabotaged to 1 block	refuses — partial loss caught
receipt zeroed / banana / -3 (sidecar intact)	applies, note kept
receipt 99	refuses (conservative)
stale receipt → new no-private propose	receipt reset to 0, stale sidecar cleaned, apply succeeds, 0 stale secret resurrected

.debrief-seal is gitignored, untracked, absent from git status, generates no doctor/triage/status tamper noise, and is removed after apply. Critically, no variant ever applies and drops the sealed note.

Regression slices + a methodology note

Unclosed-block balancing (opens=2 closes=2) with later notes visible and sealed content retained; ingest stage→propose→apply with an unclosed block and a dangling comment leaks nothing at any step and writes no memory before apply; official SDK handshake + all 4 tools; no secret across 10 CLI read surfaces and both HTML files with a non-vacuous on-disk control.

Methodology note: the first pass flagged 7 "hidden content" failures that were all harness errors — resume --full/fde log don't render decisions.md bullets, prep doesn't render context.md ## Current state bullets, and the dashboard renders only the first bullet of a section (true even with zero comments). Rewritten as differential checks against no-construct controls plus the cross-build contrast, all 7 pass. Worth knowing for future reviews of this area.

Not covered: dashboard --open remains untested (this sandbox's google-chrome shim URI-encodes the path given to xdg-open). Packaging/framing/manifest conformance were not redone since the delta touches neither transport nor manifests.