fix(loop): resume legacy frozen briefs - #125
Conversation
Traviinam
left a comment
There was a problem hiding this comment.
Fresh-context review of base 9eb629c → head 4554b66.
COMMENT-only review; this is not an approval or change request.
Findings
RVW-1-1-1 — P1 · high confidence
loops/issue-dev-loop/scripts/lib/run-store.mjs:331
legacyContractSource is selected by content truthiness. If the explicit legacy ## Frozen implementation contract is missing or empty, lines 331–336 fall back to the whole document at H2 level. Issue-controlled top-level decoy sections can then satisfy recordImplementation, including their targeted checks.
Reproduction against this exact head with an empty nested legacy contract selected:
["pnpm test -- issue-controlled-decoy","pnpm verify"]
Require the explicit legacy heading independently of its contents, parse only its H3 contract sections, reject missing/empty legacy contracts, and add a negative regression.
RVW-1-1-2 — P1 · high confidence
loops/issue-dev-loop/scripts/lib/run-store.mjs:328
Issue-controlled text can include <!-- issue-dev-loop:implementation-contract -->. Because marker presence disables legacy parsing, recordImplementation can ignore the later owner-frozen nested legacy contract and validate marker-following decoy sections instead.
Reproduction against this exact head selected the issue-controlled decoy checks rather than the nested legacy checks.
Bind format selection to the trusted generated boundary, prefer the legitimate explicit legacy contract for pre-marker briefs, and add an adversarial-marker regression.
Verification
- Existing targeted tests: 2/2 passed.
- Full runtime suite: 59/59 passed.
- Review was read-only; worktree remained clean at the exact head.
|
Ethandasw repair response for Traviinam review #125 (review) Both P1 findings were accepted and fixed in commit 228a7bd.
Evidence:
PR remains Draft. |
Traviinam
left a comment
There was a problem hiding this comment.
Second-round fresh-context review of Draft PR #125.
Base: 9eb629c1b24ded82dc3b84c71906406cdc66ab5f
Head: 228a7bd33023d4e3bf3ac280a2132ef406a30ae8
COMMENT-only review; this is not an approval or change request.
The worktree is clean and the immutable base/head pair is confirmed.
Prior P1 status:
RVW-1-1-1is closed. Compatibility parsing now distinguishes heading presence from section contents, requires an explicit trusted legacy contract, and rejects empty, missing, or issue-supplied contracts.RVW-1-1-2is closed. Marker and legacy-heading selection are constrained to the boundary after the recorded issue snapshot, with adversarial-marker regression coverage.- Current
freezeBriefstrictness remains intact: legacy H3 parsing is enabled only byrecordImplementation, and the current-format path still rejects H3 section impersonation.
RVW-1-2-1 — P2 · high confidence
File: loops/issue-dev-loop/scripts/lib/run-store.mjs:309
issueSnapshotBoundary() requires the brief to contain run.issueSnapshot.body byte-for-byte, but startRun() inserts ISSUE_BODY before later template substitutions for BASE_SHA and UI_EVIDENCE_REQUIRED. Consequently, a valid issue body containing literal {{BASE_SHA}} or {{UI_EVIDENCE_REQUIRED}} is rewritten in the generated brief while remaining unchanged in run.json. The brief can be completed and frozen, but every subsequent recordImplementation() fails with cannot locate the frozen issue snapshot boundary.
Reproduction:
const body = 'Keep {{BASE_SHA}} and {{UI_EVIDENCE_REQUIRED}} literal.'
const rendered = replaceTemplate(template, {
ISSUE_BODY: body,
BASE_SHA: 'a'.repeat(40),
UI_EVIDENCE_REQUIRED: 'UNSET',
// remaining normal template values
})
rendered.includes(`## Issue snapshot\n\n${body}`) // falseObserved rendering replaces the tokens with the SHA and UNSET, so the new boundary lookup returns -1.
Expected resolution: prevent template replacement from rewriting tokens inside injected issue content—such as with one-pass substitution or by inserting the issue body last—or derive the trusted boundary from a deterministically preserved snapshot representation. Add a regression that starts, freezes, and records an implementation for an issue body containing both literal placeholders.
Verification completed:
pnpm loop:issue-dev:test: 111 passed- Focused runtime suite: 61 passed
- ESLint on both changed files: passed
git diff --check: passed- Worktree remained clean
Ethandasw repair response — round twoReview: #125 (review)
The frozen brief previously inserted the issue body before later system-template substitutions, so issue-authored literals such as Regression coverage: Verification passed:
PR #125 remains Draft. The two-round automated review limit has been reached, so no third automated review will be started. |
Summary
Review response
Round-one Traviinam COMMENT review:
#125 (review)
RVW-1-1-1P1: removed the content-truthiness fallback to issue-controlled top-level sections.RVW-1-1-2P1: ignored issue-supplied contract markers and headings before the frozen issue-snapshot boundary.Round-two fresh-context Traviinam COMMENT review:
#125 (review)
RVW-1-2-1P2: preserved{{BASE_SHA}},{{UI_EVIDENCE_REQUIRED}},{{ISSUE_BODY}}, and replacement-pattern literals inside the authoritative issue body through freeze and implementation recording.Verification
pnpm loop:issue-dev:test: passed (112/112)pnpm loop:issue-dev:validate: passed (68 files)pnpm lint: passedpnpm verify: passed (exit code 0)Authorization
9eb629c1b24ded82dc3b84c71906406cdc66ab5ff9923385795ceca8c69da9cab462a2196e23ae5eEthandaswSafety
This PR must remain Draft. Only
codeacme17may mark it Ready, approve it, or merge it. The two-round automated review limit has been reached; no third automated review will be started.