feat: add deterministic PR dossier artifacts

[rianjs]

No description provided.

[rianjs]

Findings

• Blocker: renderDossierDiscussion renders review Event values into the reviewer-facing final dossier, so an approving review can show up as (approve) and influence the next reviewer. That violates the Define the checkout-native review contract #367 contract that reviewer-facing context must not include approvals. Filter approval/dismissed/pending review records out of the final discussion projection, or include only non-approval review discussion without event/state signals, and add a test that an approved review body/event does not appear in dossier/final/discussion.md.

• Major: The final discussion dossier writes full top-level comment, review, and inline-thread bodies via singleLine(...) with no size or count bound. This avoids prompt stuffing only mechanically; it still creates an unbounded reviewer-facing dossier file that later agents may read wholesale. Until Add durable discussion summarization before dossier assembly #369 introduces summarization, this fallback should be compact and bounded, for example capped excerpts plus anchors/provenance, with full bodies kept only in dossier/raw/.

• Major: dossierInlineThreads preserves provider order for threads and nested comments. Since dossier/index.json hashes these generated files and later fingerprints will depend on those hashes, the dossier content needs deterministic ordering independent of provider pagination/order. Sort inline threads by stable anchor/id fields and sort comments by timestamp/id/url before rendering raw and final artifacts.

• Minor: dossierChildPath rejects slashes but still accepts "." and "..", which can resolve outside or to the directory itself when future callers pass a variable artifact name. Reject clean-path special names as well, since these helpers are exported on ArtifactPaths.

[rianjs]

Findings

• Blocker: Adding Body to gitprovider.PR now causes the existing prompt builders to include the full PR body immediately, because selection and rollup prompts serialize pr directly. That violates this ticket’s stated intent to persist reviewer-useful PR context “without stuffing that context into prompts.” Keep PR body in the dossier-specific artifact projection, or change the prompt builders to use a prompt-specific PR projection that omits Body until the later dossier/workbench routing ticket intentionally changes prompt shape. Add a regression assertion that selection/rollup prompt payloads do not include the PR body in this ticket.

• Major: renderDossierDiscussion trusts any pre-existing dossier/summary/discussion.md when assembling the final dossier. In fresh run-owned directories that is fine, but SelectionOnly supports caller-owned artifact roots, so a reused directory can silently feed stale summary text into a new final dossier. Until Add durable discussion summarization before dossier assembly #369 creates a durable, fingerprinted summary artifact for the current raw context, this ticket should ignore summary files or require same-run provenance/digest metadata before preferring them.

[rianjs]

Findings

No findings.

The fixes address the architectural issues from the prior review: PR body is now kept out of existing selection and rollup prompt payloads, approval review content is excluded from the reviewer-facing discussion dossier while remaining available in raw artifacts, final discussion output is capped/excerpted, inline discussion ordering is deterministic, and dossier child paths reject traversal-like names.

The remaining behavior is aligned with ticket #368’s boundary: persist raw context plus compact final dossier artifacts, without switching reviewer execution or durable summarization yet.

[rianjs]

Findings

• Major: The dossier tests at internal/pipeline/pipeline_test.go#L166 prove that an issue comment, one inline thread, and the approved review body are handled, but they never assert that a non-approval review body still appears in the reviewer-facing dossier. A bug that drops all review records from discussion.md while keeping issue comments and threads would still pass these assertions.
• Major: The new index helper at internal/pipeline/pipeline_test.go#L3888 only checks that index.json exists, has sha256, and contains one expected path. It does not verify that every dossier artifact is indexed or that the recorded hashes match file contents, so a partial or bogus index could still satisfy the test and defeat the “deterministic dossier index” guarantee.
• Minor: The prompt-redaction coverage at internal/pipeline/pipeline_test.go#L480 only does substring checks for the PR body. That would still pass if promptPRFromPR accidentally stripped other prompt-critical fields like title, URL, head/base refs, or author. A small JSON assertion on the pr object would pin the intended contract without broadening the test much.

[monit-reviewer]

Automated PR Review

Reviewed commit: e63664b

Approved with 4 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-knowledge-reviewer	3
harness-engineering:harness-self-documenting-code-reviewer	1

harness-engineering:harness-knowledge-reviewer (3 findings)

💡 Suggestion - internal/pipeline/pipeline.go:2584

The three-tier dossier structure (raw/summary/final) has no documentation of tier semantics or rationale. Future contributors extending the dossier won't know why raw vs. final are separated or what each tier is intended for. A brief docs/ entry capturing the artifact contract and tier semantics would make this discoverable.

💡 Suggestion - internal/pipeline/pipeline.go:2226

The promptPR type enforces a non-obvious invariant: gitprovider.PR.Body must not appear in selection or rollup prompt payloads. The type name doesn't convey this intent — a name like promptPayloadPR or bodyExcludedPR would signal the omission intentionally. A future contributor adding a field to gitprovider.PR won't know this exclusion exists or why, risking inadvertent prompt leakage. A short comment or docs/ pointer would make the contract discoverable. (Consolidates naming finding from harness-self-documenting-code-reviewer at the same line.)

💡 Suggestion - internal/pipeline/pipeline.go:2975

The approval-filtering behavior in reviewerFacingTopLevelComments — approval reviews excluded from the final reviewer-facing dossier, retained in raw — is undocumented in code or docs/. The rationale (preventing approval state from influencing subsequent reviewers) lives only in PR discussion threads. If this invariant is load-bearing for the review system's correctness contract, a brief inline note or docs/ entry would prevent it from being accidentally reverted.

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - internal/pipeline/pipeline.go:3063

In filePatchStatus, the 'added' case detects new files by scanning raw diff text for both "new file mode" and "--- /dev/null" — two distinct git diff header formats. A reader unfamiliar with git diff internals won't know why both strings are needed or what each covers. Extracting this to a named helper like isNewFileInDiff(patch string) bool would encapsulate the heuristic and make the switch case intent immediately clear.

1 info-level observations excluded. Run with --verbose to include.

4 PR discussion threads considered.

---

Completed in 4m 09s | $2.29 | sonnet | daemon 0.2.132 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	4m 09s wall · 4m 05s compute (Reviewers: 2m 30s · Synthesis: 1m 35s)
Cost	$2.29 (estimated)
Tokens	389.6k in / 26.8k out
Turns	12

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	40.8k	5.5k	13.1k	27.6k (1h)	$0.25
harness-engineering:harness-architecture-reviewer	sonnet	66.9k	1.7k	13.1k	53.7k (1h)	$0.35
harness-engineering:harness-enforcement-reviewer	sonnet	68.3k	2.8k	13.1k	55.2k (1h)	$0.38
harness-engineering:harness-knowledge-reviewer	sonnet	68.4k	3.0k	13.1k	55.2k (1h)	$0.38
harness-engineering:harness-self-documenting-code-reviewer	sonnet	76.6k	10.4k	13.1k	63.5k (1h)	$0.54
security:security-code-auditor	sonnet	68.6k	3.4k	13.1k	55.4k (1h)	$0.39

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

[monit-reviewer]

🔵 Low (harness-engineering:harness-knowledge-reviewer): The three-tier dossier structure (raw/summary/final) has no documentation of tier semantics or rationale. Future contributors extending the dossier won't know why raw vs. final are separated or what each tier is intended for. A brief docs/ entry capturing the artifact contract and tier semantics would make this discoverable.

Reply to this thread when addressed.

[monit-reviewer]

🔵 Low (harness-engineering:harness-knowledge-reviewer): The promptPR type enforces a non-obvious invariant: gitprovider.PR.Body must not appear in selection or rollup prompt payloads. The type name doesn't convey this intent — a name like promptPayloadPR or bodyExcludedPR would signal the omission intentionally. A future contributor adding a field to gitprovider.PR won't know this exclusion exists or why, risking inadvertent prompt leakage. A short comment or docs/ pointer would make the contract discoverable. (Consolidates naming finding from harness-self-documenting-code-reviewer at the same line.)

Reply to this thread when addressed.

[monit-reviewer]

🔵 Low (harness-engineering:harness-knowledge-reviewer): The approval-filtering behavior in reviewerFacingTopLevelComments — approval reviews excluded from the final reviewer-facing dossier, retained in raw — is undocumented in code or docs/. The rationale (preventing approval state from influencing subsequent reviewers) lives only in PR discussion threads. If this invariant is load-bearing for the review system's correctness contract, a brief inline note or docs/ entry would prevent it from being accidentally reverted.

Reply to this thread when addressed.

[monit-reviewer]

🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): In filePatchStatus, the 'added' case detects new files by scanning raw diff text for both "new file mode" and "--- /dev/null" — two distinct git diff header formats. A reader unfamiliar with git diff internals won't know why both strings are needed or what each covers. Extracting this to a named helper like isNewFileInDiff(patch string) bool would encapsulate the heuristic and make the switch case intent immediately clear.

Reply to this thread when addressed.