[JacobStephens2]

Summary

Interpret each Triage run's recommendation once, through one private module shared by the initial session, its correction, recommend-only and both publication modes.

Session final message
  correction::recommendation
    Recommendation::interpret(message, Captured)
      associated issue + required Filing
      associated comment + parsed Issue URL
  GitHub preflight (read-only)
  recommend-only JSON / Report publication / existing-issue publication
    record successful effects → JSON and Run notification

The module owns category/state fallback, repository spelling, confirmation blocks, questionnaires, existing-target rules, readiness permission and combined diagnostics. Wire values remain as recommended until publication records actual results. Report readiness without permission stays needs-triage; existing-issue readiness becomes ready-for-human. GitHub effects, publication order, existing categories, partial-publication links and private-finding notices retain their behavior. The domain-to-code index identifies the new owner.

Evidence

Execution evidence

This is a behavior-preserving refactor. The before/after assertion demonstrates removal of caller-owned optional resolved state:

Before, at the fixed base 69c8689b:

git show refs/remotes/origin/main:src/triage.rs | python3 -c 'import sys; assert "triaged: Option<Label>" not in sys.stdin.read(), "caller-owned optional resolved state remains"; print("PASS: no caller-owned optional resolved state")'

Actual output (exit 1):

AssertionError: caller-owned optional resolved state remains

After, at b24e503f, the same assertion against the branch:

git show HEAD:src/triage.rs | python3 -c 'import sys; assert "triaged: Option<Label>" not in sys.stdin.read(), "caller-owned optional resolved state remains"; print("PASS: no caller-owned optional resolved state")'

Actual output (exit 0):

PASS: no caller-owned optional resolved state

Test seams: Recommendation::interpret, its publication projections and result recording; the thirdshift triage CLI through the existing fake Harness/GitHub adapters.

• cargo test --bin thirdshift triage::recommendation::tests: 15 passed; 0 failed. Covers combined category/state rules, both readiness modes, confirmation blocks, questionnaire normalization, combined errors, serialization and notifications.
• cargo test --test triage_correction --test triage_run --test triage_issue --test triage_spec_regressions --test triage_pass --test issue_1711_retained_home: 73 passed; 0 failed; 12 ignored (subprocess helper entry points). Results by binary: correction 12, Report 33, existing issue 12, Spec regressions 3, Pass 11, retained home 2.
• Full-suite validation reused successful CI for the unchanged head b24e503fd39ac173c85af2ab68446d5c703b68b5: Rust coverage gate, overall test gate, site and Console. No equivalent full-suite rerun was made on this already covered head.
• The earlier implementation ran scripts/affected-tests origin/main; it selected the full Rust suite. Its local sandbox dependency failures were followed by successful CI on the delivered head. This continuation changed no tracked files.

Visual evidence

Not applicable: no rendered UI changed.

Review

Independent Standards and Spec reviewers used thirdshift-code-review against refs/remotes/origin/main (69c8689b), reviewing all three branch commits through b24e503f.

• Standards: 0 findings; no standards violation or actionable smell.
• Spec: 0 findings; no missing requirement or concrete compatibility defect.
• Both reviewers read all six changed files. Unread changed files: none. Generated-output exclusions: none.
• Reports and each reviewer's final files-read list: .thirdshift-review-VDiM9M/standards.md and spec.md; summary.json contains {"standards":0,"spec":0} in the Session artifacts.

Unaddressed findings

The current review raised none. These earlier review decisions remain documented for the whole branch:

Standards

• Earlier finding 4, remaining portion: Publishing mirrors Proposals. Retained: Recommendation::publishing, line 202 lends the selected proposal and disjoint mutable comments together to each publication adapter. Proposals does not own the comments. This implements Deepen Triage recommendation interpretation #2147 Decision 4's associated publication projections; repeated Input matches were removed.
• Earlier finding 5: move factory-label exclusion and notification sanitization elsewhere. Retained: is_factory_label, line 516 and one_line, line 537 belong here under Deepen Triage recommendation interpretation #2147 Decisions 6 (factory-label exclusion) and 11 (notification projection and sanitization). The inconsistent import was fixed.

Spec

• Earlier finding 3: record bodies after the successful write rather than before attempting it. Retained: investigate, line 353 returns Err before returning JSON on publication failure. Failure notifications omit bodies; no user-visible failure output changes. Successful URL/category/state updates retain their effect points. Recording successful effects follows Deepen Triage recommendation interpretation #2147 Decision 10.
• Earlier finding 4: internal zip during construction. Retained: Recommendation::interpret, line 150 joins two complete, ordered resolutions of the same proposed issues inside construction. Callers receive associated proposals, never independent vectors. The separate category scan preserves the combined-error contract in Deepen Triage recommendation interpretation #2147 Decision 8.

None of these decisions defers a call to the Day shift or reopens the Spec; no new triage issue is required.

Security

• No unaddressed introduced findings.

Reviewed git diff 69c8689b378ba86623875415db9fd36696532cf5...HEAD in guidance mode, including the recommendation module and its triage callers. The repository has no SECURITY.md. The applicable boundaries are ADR 0013 (a Harness is fully trusted and the worktree is the boundary), ADR 0015 (a private-finding notification carries title and link, not a write-up), and ADR 0005 (the Run notification leaves the machine through Resend). Readiness permission, confirmation blocking, factory-label exclusion, same-repository open-comment checks, notification field sanitization, and private-finding notification replacement stay on the existing GitHub write paths. No introduced boundary violation was established.

Security review outcome

No unaddressed introduced findings.

Merge Danger

Door: two-way

An internal refactor; reverting restores the previous implementation without a data or configuration migration.

Blast Radius: Triage

Interpretation, recommendation JSON, notifications, issue labels and comments. Existing CLI regressions and exact-head CI cover these paths.

Closes #2147

Built with codex codex-cli 0.167.2 · gpt-6.1-sol · xhigh

[JacobStephens2]

Delivery completed on agent-authored or reviewed head 296dde2.

[JacobStephens2]

Review run. Pushed fixes to issue-2147 at b24e503. Spec axis used the linked closing issues.

I reviewed PR #2148 against 69c8689b on both axes and made four small fixes, committed as b24e503f (not pushed). None of the 12 findings says behaviour is wrong, so none needed a failing test. The full Rust suite and clippy pass after the fixes.

The Spec is the linked closing issue #2147. Both reviewers read all six changed files, so no follow-up was needed and no changed file was left unread. There is no generated output in this diff, so nothing was excluded. The reports and summary.json ({"standards":9,"spec":3}) are in .thirdshift-review-X9u306/.

Standards

9 findings, all judgement calls, no hard violations. The reviewer compared the held-readiness wording, the ready-for-human hold for an existing issue and the self-comment rule against 69c8689b; all behave as before.

Against documented standards:

1. Filing::readiness_held is pub but only the tests use it outside the module. Rule (DEEPENING.md, "Seam discipline"): don't expose internal seams just because tests use them.
2. The one-issue rule is checked in one function and relied on in another. check_existing checks there is exactly one issue; interpret then assumes it with .expect("resolve gives an existing target exactly one issue"). Rule (SKILL.md): invariants are part of the interface.
3. Callers must call record_* after each GitHub write. This is an ordering rule callers have to remember.
4. The "Existing-issue Triage run" row in docs/code-navigation.md doesn't list src/triage/recommendation/tests.rs.

Smells:

5. Repeated Switches: interpret decides Report vs existing issue several times (input.issue(), held, match captured.input, resolve).
6. Duplicated Code: the comment-posting loop appears in both src/triage.rs publish and src/triage/existing.rs publish; only the body format differs. Related Data Clump: (&GitHub, &mut Vec<IssueUrl>).
7. Middle Man (minor): record_in_place only forwards to record.
8. Inconsistent same-repo check: check_comment_targets compares owner and repo by hand instead of using in_same_repo.
9. Misleading test name: notes_carry_a_confirmation_block_then_a_questionnaire claims an order the code can never produce.

Spec

3 findings, none saying behaviour is wrong. The reviewer checked behaviour against 69c8689b and found it unchanged across every rule the issue lists. Every acceptance criterion is met, and there is no scope creep.

1. A new test calls is_factory_label directly instead of going through the interface. The test is workflow_labels_are_factory_labels_whatever_their_case. Decision 13 says "The interface is the test surface… rather than internal storage or helper call order".
2. The module decides factory-label exclusion, but the caller applies it. Filtering happens in repository_categories in src/triage.rs. Decision 6 says the module owns "factory-label exclusion". Behaviour is unchanged.
3. The PR body's selector run wasn't verified by the reviewer. I re-ran the selector; results are under Validation.

What I fixed

• Standards 1: Filing::readiness_held is now private; the child test module can still reach it.
• Standards 4: added src/triage/recommendation/tests.rs (existing-target rules) to the Existing-issue Triage run row.
• Standards 9: renamed the test to notes_carry_a_confirmation_block_or_a_questionnaire.
• Spec 1: removed the helper-level test and its import. The CLI tests already cover factory-label exclusion, including the mixed-case SECURITY-FINDING label: tests/triage_issue.rs::finding_category_guidance_uses_actual_names_and_filters_factory_labels and the factory_label_category case in tests/triage_run.rs.

Unaddressed findings

Standards

• 2 and 5 (one-issue expect, repeated mode switches): declined. Both target = captured.input.issue() and the later match captured.input read the same captured.input inside one function (src/triage/recommendation.rs, interpret). The expect cannot fire while check_existing rejects anything but exactly one issue. The existing-target cardinality test still passes.
• 3 (callers must call record_*): declined. Decision 4 asks for "explicit recording of actual publication results", and Decision 10 keeps GitHub writes in the two publication adapters.
• 6 (duplicated comment loop): declined. Decision 2 says the module "performs no GitHub reads"; Decision 10 says to keep "the two publication adapters and their ordering". Each adapter formats its own comment body.
• 7 (record_in_place only forwards): declined. The separate per-mode name was added on purpose in the previous review round, as the PR body says.
• 8 (hand-written same-repo check): declined. check_comment_targets compares against a Repo, not an IssueUrl, so in_same_repo doesn't fit. Adding a Repo-side helper is outside this Ticket.

Spec

• 2 (factory-label exclusion applied by the caller): declined. Decision 2 says to "supply the captured repository category names … as values". is_factory_label lives in the module, and the filtered list also feeds the session's category guidance (tested by finding_category_guidance_uses_actual_names_and_filters_factory_labels). Behaviour matches origin.

No finding needed the Day shift to decide, so I filed no needs-triage issues.

Validation

Run in this worktree; the PR was open and not a draft before validation.

Command	Result
cargo test --bin thirdshift triage (before fixes)	exit 0, 42 passed
cargo test --test triage_correction --test triage_issue --test triage_run --test triage_spec_regressions --test triage_pass (before fixes)	exit 0: 12, 12, 11, 33 and 3 passed
cargo test --bin thirdshift triage::recommendation::tests (after fixes)	exit 0, 16 passed (one test later removed)
cargo clippy --all-targets -- -D warnings (after fixes)	exit 0
scripts/affected-tests main (after fixes)	exit 0, PASS cargo test (1005.60s); it selected the full Rust suite

The extra-command list was empty, so nothing else ran. Logs are in .review-logs/ and .affected-tests/.

Security

No unaddressed introduced findings.

Delivery failed

pull request changed during Review run