[JacobStephens2]

Summary

Existing-issue Triage could copy a Security finding's security-finding marker into its recommended category, causing publication to fail and leave agent-failed. The session now receives actual repository category choices, and the embedded Triage skill explains why a valid category is required even when publication preserves an existing category.

repository labels → shared category exclusions → Session prompt
  → valid recommendation → publication validates again
  → preserve security-finding and existing category, or add the chosen category

The branch also consolidates Experiment wait-state publication to its Command record and stored details into publish_waiting_for. Callers retain their existing save and final-state ordering. The reconciliation with main keeps both sets of Triage tests.

Closes #2018

Seams under test: the thirdshift triage CLI through fake GitHub and Harness adapters (prompt, labels, comments, recommendation-only writes and private notifications), and the Experiment CLI's host coordination.

Evidence

Execution evidence

cargo test --test triage_issue security_finding_triage_uses_repository_categories_instead_of_its_factory_label -- --exact --nocapture

• Before: the branch regression overlaid on the supplied merge base, 3beacb7f03fa45053914763a5d8a8668ea8cc93c, exited 101: the Triage category must be a repository category label, not a triage state or another factory label. The CLI returned Some(1) instead of Some(0), labels were ["security-finding", "ready-for-human", "agent-failed"], and comments were null. Result: FAILED. 0 passed; 1 failed.

• After: the unchanged regression on issue-2018 passed: ok. 1 passed; 0 failed. It covers findings with and without an existing category, publishing and --recommend-only. Publishing posts exactly one comment without agent-failed; recommendation-only leaves fake GitHub state unchanged.

• Cause confirmed: changing only the simulated Harness recommendation to bug on the merge base made all four combinations pass. The missing category guidance caused the invalid recommendation; the factory-label validator correctly rejected it. The existing branch fix was retained.

• cargo test --test triage_issue --test triage_spec_regressions: 15 passed, two subprocess entry points intentionally ignored, against the reconciled sources. Includes private investigation, questionnaire and failure diagnostics staying out of email, actual escaped category names, category preservation and factory-category rejection.

• UPDATE_PROMPTS=1 cargo test --bin thirdshift prompts_page: 14 passed; generated artifacts stayed unchanged. cargo test prompts_page verified the generated output again. The manual was regenerated with npm run docs --prefix site.

• NEXTEST_TEST_THREADS=8 CARGO_BUILD_JOBS=2 scripts/affected-tests refs/remotes/origin/main: all 68 site checks passed (100.28s), including manual generation checks. The Rust build completed in 4m19s. Its duplicate local test execution was intentionally interrupted after exact-head CI passed, following docs/testing.md's CI-reuse rule; 658 local tests had passed before eight running tests received SIGINT. This local attempt is not claimed as a completed full-suite pass.

• Complete Rust validation: CI on exact head de5eb315 passed 4,338 tests across all four partitions, plus the exact-coverage gate. Partition execution took 313.949–402.601s, excluding compilation. The same run passed site and Console browser checks, formatting, clippy and other configured checks.

• Release checks on the same head passed both x86_64 and aarch64 musl artifact builds.

• cargo fmt --check and git diff --check refs/remotes/origin/main...HEAD passed. Both independent reviewers rechecked the merged context; no new findings or unread changed files.

Visual evidence

Matched captures compare the supplied merge base with the final branch at 1440×900 and 390×844, with the same sections open and the same scroll targets. They contain documentation, not private findings.

Existing-issue prompt catalogue

1440×900 — Existing-issue prompt catalogue

390×844 — Existing-issue prompt catalogue

Architecture: Triage run

1440×900 — Architecture: Triage run

390×844 — Architecture: Triage run

How it works: Triage

1440×900 — How it works: Triage

390×844 — How it works: Triage

Home: Triage workflow

1440×900 — Home: Triage workflow

390×844 — Home: Triage workflow

Manual: existing-issue Triage

1440×900 — Manual: existing-issue Triage

390×844 — Manual: existing-issue Triage

Review coverage

Independent Standards and Spec reviews against refs/remotes/origin/main: 0 Standards findings; 0 Spec findings. Both axes read all 14 changed files, including generated hunks. No generated-output exclusions and no changed file left unread. Final reports and summary.json are retained in .thirdshift-review-xtd7RR/ with the Session trace.

Unaddressed findings

Standards

None raised in this review. Earlier declined suggestions about the Experiment helper remain documented:

• Fold experiment.save() into the helper: declined. src/evals/experiment.rs:481–492 clears waiting names, sets the final state, end time and pause reason, then saves once. Saving inside the helper would precede those updates and require another save.
• Move the helper onto Stored (possible Feature Envy): declined. src/evals/experiment/coordination.rs:9–12 owns the paired wait-state publication; keeping it there follows the documented locality principle. Stored owns persistence, which callers retain.

Spec

None raised in this review. Earlier declined findings remain documented:

• Experiment refactor is outside A Triage of an existing Security finding fails: security-finding is rejected as its category #2018: retained as part of the existing branch, as the Continuation explicitly directs. The owner authored 31328859 and dcc9b2d0 to consolidate and name this operation; the new Spec review inspected it and found no actionable scope finding.
• Initial empty wait-state publication changes behavior: declined. A new Command initializes waiting_for to an empty vector (src/logs/command_record.rs:471); src/logs/live.rs:25 treats it as waiting only when nonempty. src/evals/experiment/coordination.rs:22 therefore republishes the initial empty value while clearing stored details.

Security review outcome

No unaddressed introduced findings.

Merge Danger

Door: two-way

Reverting restores the earlier guidance and wait-state calls; no migration is required.

Blast Radius: Factory

Existing-issue Triage receives category choices. Report publication retains the shared exclusions, existing category and finding-marker preservation, readiness permissions and private-notification protections. Experiment wait-state publication uses one operation.

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

[JacobStephens2]

Review run. Pushed fixes to issue-2018 at 1135e54. Spec axis used the linked closing issues.

Review of PR #2024 (issue-2018)

The PR implements issue #2018 as specified. The review found four style judgement calls and no wrong behaviour. I fixed most of them in one commit, 1135e54f, and did not push it. Final validation did not fully pass: two Security-fix tests fail, and they fail the same way on origin/main.

The fixed point I was given, refs/remotes/origin/2dc95dd9e81af7f21c756de0c74ce27655471354, is not a ref in this clone. The commit it names, 2dc95dd9, does exist: it is origin/main and the point this branch was built from, so I reviewed against it. The Spec is linked closing issue #2018 and its trusted Agent Brief comment. The PR is still open.

Standards (4 findings, all judgement calls, no hard violations)

• S1: a second name for factory labels. The new paragraphs in docs/guide.md:3920, site/docs/index.html, site/how-it-works/index.html:274 and site/index.html:441 said "factory markers" and called security-finding a "marker". The code, the skill and the error message all say "factory label", and the glossary defines neither term. The regression test name repeated "factory marker".
• S2: paragraph out of order. In site/index.html and site/architecture/index.html, the new paragraph came two paragraphs before the one that introduces thirdshift triage <Issue URL or number>. The other pages put it after that introduction.
• S3: possible Duplicated Code in tests/triage_issue.rs. The two new tests repeat the same loop over --recommend-only and the same setup. One test nests two loops to cover four scenarios.
• S4: possible Primitive Obsession, and a stale doc comment. repository_categories returns a plain Vec<String>, so the checks it already made (no factory labels, no commas) are not carried by the type. The doc comment on prompt::triage_issue did not mention the new categories argument.

Spec (0 findings)

Every Acceptance item and every part of the Agent Brief's recommended implementation is met:

• The session receives the repository's real category names, for a finding with and without an existing category.
• A valid recommendation publishes exactly one comment and no agent-failed.
• --recommend-only writes nothing.
• Report filing still rejects factory categories.
• Private investigation text stays out of email.
• The prompts are regenerated, and the manual and site wording were checked.

The reviewer also noted two things that are not findings:

• O1: a repository with no usable categories still fails at publication, as it did before.
• O2: repository labels are fetched twice per run, which is intended.

Fixed in 1135e54f

• S1: changed "marker" to "label" in the guide, the regenerated manual and the three hand-written site pages. Renamed the test to security_finding_triage_uses_repository_categories_instead_of_its_factory_label.
• S2: moved the paragraph after the existing-issue introduction in site/index.html and site/architecture/index.html.
• S4 (doc comment part): the prompt::triage_issue doc comment now mentions the supplied repository categories.

Unaddressed findings

Standards

• S3, test duplication: declined. It is a judgement call. Each test is self-contained, and the four-scenario loop maps directly onto the Acceptance item for "no category and existing category, publish and recommend-only". No behaviour is wrong, and the renamed test passed. It needs no issue, because the call is mine to make.
• S4, the newtype part: declined. There are only two callers, both in src/triage.rs, and the filter lives in one place (repository_categories, src/triage.rs:291), so a new type adds little. The doc-comment part is fixed. It needs no issue.

Spec

None.

Review coverage

• Both reviewers read all 12 changed files, including the generated prompts/triage-issue.md, site/prompts/index.html and site/docs/index.html, so neither needed a second pass.
• Nothing was excluded as generated output, and no changed file was left unread.
• The reports are in .thirdshift-review-L5ALrk/standards.md and spec.md, and summary.json is {"standards":4,"spec":0}.

Validation

Command	Result
npm run docs --prefix site	First run exit 1: ERR_MODULE_NOT_FOUND because site/node_modules was missing. After npm ci in site/ (exit 0), it exited 0.
npm run docs:check --prefix site	exit 0
cargo test --test triage_issue security_finding_triage_uses_repository_categories_instead_of_its_factory_label -- --exact	exit 0, 1 passed
cargo test --bin thirdshift prompts_page	exit 0, 14 passed
scripts/affected-tests main	exit 1. Local main is stale (6bcde725, versus origin/main = 2dc95dd9), so it selected unrelated changes. The Console tests then failed because console/node_modules was missing; this is a setup gap, not a code failure.
NEXTEST_TEST_THREADS=8 scripts/affected-tests origin/main (after npm ci in console/)	exit 1. npm test --prefix site passed (31s). Without nextest it fell back to cargo test, which stopped at --test process with 2 failures, so the Console browser tests were never reached.
cargo test --no-fail-fast	exit 101: 4236 passed, 2 failed, 97 ignored. The failures are process findings::closing_rechecks_each_merged_base_and_leaves_a_failing_finding_for_the_day_shift and findings::fixing_dispatches_each_reproduced_finding_and_keeps_unreproduced_findings_private.
cargo test --test process findings:: on the branch, 4 runs	exit 101 each time, with 1–2 failures that moved between tests from run to run
cargo test --test process findings:: on origin/main (temporary worktree), 2 runs	exit 0, 18 passed each time
Single test findings::fixing_dispatches_… alternated between trees, 3 runs each	branch failed 2 of 3 and passed once; origin/main failed 3 of 3

The two process::findings failures come from Security fix publishing: they report an issue created "outside the staged path", and issues_created_since(started) looks timing-sensitive. They are not caused by this PR, for two reasons:

• The same test also fails on origin/main.
• The PR does not touch Security fix publishing.

I found no open issue for this failure and did not file one, since it is not a review finding.

Not run: the Console browser tests (npm test --prefix console). The full affected-tests run never reached them, and the earlier run against stale main failed only because of missing dependencies.

Disposable logs are in target/review-logs/ (ignored by git), and the temporary origin/main worktree has been removed.

Security

No unaddressed introduced findings.

Delivery failed

pull request changed during Review run