#1293 Present and classify discretionary findings as actionable - #1295
Merged
Conversation
Fixes post-review guidance that suppressed the next-steps menu whenever a review's findings were all Recommendations and Suggestions, leaving the reader no implement-directly option, no author-handoff routing, and no recommendation gradient. The Findings sub-block now triggers on any finding, expressed as a rule derived from the Actionability gate that every emitted finding has already cleared, rather than as a tier list that goes stale whenever the finding scheme moves. The recommendation rules collapse into one cascade shared by the `review-branch` and `review-pr` variants, and every option carries a firing condition, replacing a conditionless first option that outranked the conditioned alternatives in practice. "Implement directly" is conditioned on determinacy, meaning the finding's Recommendation states the exact change, and the rules record what that condition bounds: implementing forfeits the second look, and determinacy is what makes the forfeit acceptable. The tiebreaker favoring a human in the loop is scoped to genuine ties. A follow-up-ticket disposition joins both option pools, routed per `scope-and-deferral.md`, which the sub-block already cited while offering no option that reached it. A legacy-only review renders the sub-block with the full pool. The consumer row for this menu in `complexity-classification.md` drops the superseded trivial-findings test in favor of determinacy.
Fixes a criticality mapping that placed a review holding only Suggestions in the `none` bucket, glossed "No actionable findings". Orchestration reads `none` as report-only, so an unattended run skipped the fix cycle for a review whose findings had all cleared the Actionability gate. `S` now maps to `low` alongside `R` and `T`, and `none` means no authored finding at all. The default `budget_threshold` of `low` therefore reaches suggestion-only reviews, which is the intended reading of `low` as worth fixing if budget allows; the configured threshold remains the control. Legacy-only reviews stay at `none` so an unattended fix cycle stays clear of pre-existing code, and the mapping records that this is a classification judgment rather than a presentation one. The `none` glosses in `review-cycle.md` and the disposition condition in `respond-to-review` drop the word "actionable", which understated what requires disposition now that legacy findings take one too.
The Findings sub-block explained its lack of a legacy carve-out by asserting that a legacy fix rarely satisfies the determinacy test, which biases the cascade away from rule 2 for exactly the cheap, in-file legacy edits worth making on the branch. The sentence now states the mechanism instead: a legacy finding naming its exact edit satisfies rule 2 like any other, and one that does not falls through to a ticket or a hand-off.
The post-review cascade's first rule fired on any single finding clearing the spin-off bar while selecting an option that carried every finding, so a review holding three determinate suggestions and one separable recommendation routed all four into a ticket, against the fold-in default. Rule 1 now tests the open findings as a collection, matching the quantifier the other rules and the marker-strengths section already use, and a new paragraph names the mixed case: the recommendation comes from the findings that stay, and the follow-up-ticket line names the separable subset it would carry. The `none` gloss in `review-cycle.md` reads "where reviewers produced no authored findings" at both sites. The previous wording denied that any finding was written, which a legacy-only review contradicts. The scheme table's `S` row moves to criticality `low`, matching the rollup. Leaving it at `none` made the rollup's suggestion-only row unreachable by the table's own "highest finding present" definition. `complexity-classification.md`'s consumer row for this menu drops its `1–2` levels cell, which contradicted the same row's decision text: a level-1 finding that poses a question rather than naming an edit is not determinate, so level alone does not select "Implement directly".
…iers Findings the scheme marks "Never" merge-blocking could halt an orchestrated run for manual review. Every configuration that set `approval_threshold` to `low` gated approval on `T`, `R`, and `S` alike, so a review whose only output was deferrable items could exit `needs_manual_review`. Approval now floors at `medium` in the engine default, in `orchestrate-review`'s default, and in `orchestrate-dev`'s high-effort preset, which leaves `W` and `F` gating and matches the finding scheme's Merge-blocking column. Budget-threshold is untouched and keeps descending with effort, so high effort still buys opportunistic fix cycles on the deferrable tiers; the two knobs now carry distinct values rather than collapsing at a shared default. The effort table's inversion rule records the floor and why it exists. The piggyback rule in `orchestrate-dev` names `S`-tier suggestions instead of "none-severity" ones, which stopped naming anything once `S` moved to `low`.
The engine and effort-preset text called the approval threshold a floor and stated a universal, that nothing below a Warning gates approval, which no mechanism enforces: `low` remains selectable through `--approval-threshold` and through `orchestration.approval_threshold`, both of which outrank the default. Both sites now say the defaults and presets stop at `medium` and that an explicit `low` is honored, so a project wanting the deferrable tiers to gate can still ask for it. `packages/agents/README.md`'s preferences reference gave `orchestration.approval_threshold` a default of `low`, and its example set the same value, as did the example inside `orchestrate/SKILL.md`. All three now read `medium`; each was a shipped configuration that reinstated approval-gating on `T`, `R`, and `S`. Both threshold enums in `preferences.json` drop `none`, a value neither the README nor any CLI surface documents. `approval_threshold: none` gated approval on a review that found nothing, since every criticality compares at or above it.
Both option pools described the follow-up-ticket disposition as spinning "the findings" into a ticket, the semantics the subset rule replaced, in the column an agent reads to learn what selecting the option does. An agent working from that description swept every open finding into the ticket in the mixed case. Both descriptions now name the separable findings and say the rest route by the next matching rule. The subset rule states what disposes of the remainder: the ticket option composes with the recommendation rather than replacing it, the cascade's remaining rules run on the findings the ticket does not carry, and answering with both numbers disposes of every open finding.
Dependency auditProduction dependency audit passed. |
williamthorsen
marked this pull request as ready for review
August 12, 2026 00:34
williamthorsen
added a commit
that referenced
this pull request
Aug 13, 2026
…e-v0.3.0 codeassembly-v0.9.0 factory-v0.2.6 codeassembly-mcp-v0.2.5 codeassembly-v0.9.0 - #1288 feat: State mutability and name the lede reader at the artifact type lists (#1303) - #1285 fix: Require plain bullets for checklists on both update-jira-ticket paths (#1302) - deps: Upgrade all deps to latest version - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - #1294 feat: Specify artifact mutability and its sanctioned exceptions (#1297) - #1293 fix: Present and classify discretionary findings as actionable (#1295) - #1289 tooling: Remove shelled nmr calls from package manifests (#1291) - #1280 feat: Add a refresh path that edits an existing AGENTS.md instead of redrafting it (#1287) - #1055 feat: Replace the lede doctrine with corpus-derived guidance and retire the changelog-writer (#1286) - deps: Upgrade all deps to latest version - #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284) - #1263 refactor: Retire the deferred-lint mechanism and adopt toolbelt.errors in agents (#1279) - #1262 refactor: Hoist unreadable for-of expressions and promote the lint rule to an error (#1276) - #1267 fix: Extend the no-hard-wrapping instruction to every GitHub composer (#1275) - #1177 fix: Rewrite links, tokens, and template variables in a Markdown file support entry (#1273) - #1176 feat: Add a ticketing-preferences hook to the skills that split and create tickets (#1268) - #1252 fix: Keep a skill's own sections out of injected guidance (#1266) - #1260 refactor: Fix lint and retire rule deferrals (#1265) - #1247 feat: Add a personal tooling-preferences rulebook (#1257) - #1251 feat: Let a rulebook declare that a guidance hook is how it is reached (#1259) - #1246 feat: Guard home-domain writes and record their provenance (#1254) - #1175 feat: Link new tickets to their blockers and parents (#1249) factory-v0.2.6 - deps: Upgrade all deps to latest version - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - deps: Upgrade all deps to latest version - deps: Upgrade all deps to latest version - #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284) kb-v0.6.0 - deps: Upgrade all deps to latest version - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - #1289 tooling: Remove shelled nmr calls from package manifests (#1291) - #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284) - #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272) - #1260 refactor: Fix lint and retire rule deferrals (#1265) codeassembly-lifecycle-v0.4.0 - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - #1289 tooling: Remove shelled nmr calls from package manifests (#1291) - #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272) - #1260 refactor: Fix lint and retire rule deferrals (#1265) codeassembly-mcp-v0.2.5 - deps: Upgrade all deps to latest version - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - #1289 tooling: Remove shelled nmr calls from package manifests (#1291) - #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284) codeassembly-run-core-v0.3.0 - deps: Upgrade all deps to latest version - #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301) - #1289 tooling: Remove shelled nmr calls from package manifests (#1291) - #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284) - #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272) - #1260 refactor: Fix lint and retire rule deferrals (#1265)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the post-review next-steps menu, which
review-branchandreview-prwithheld if the only findings were recommendations or suggestions. The menu is now triggered by every actionable finding.Separately, orchestration stops reading discretionary findings as an absence of findings. A suggestion-only review now has a criticality of
lowrather thannone. The approval threshold now defaults tomedium, and no effort preset sets it lower.Why
Discretionary findings were going nowhere. A review whose findings were all Recommendations and Suggestions offered the developer no menu to act on them and reported criticality
noneto an orchestrated run, which read that as a clean review and ran no fix cycle, even though every emitted finding had already cleared a gate requiring it to hand the author a concrete decision. Severity was deciding whether a finding was shown and whether it could be acted on, which is not what severity is for.Details
🐛 Bug fixes
_partials/next-steps-after-review.md): The Findings sub-block's trigger changes from anF/W/Tenumeration to any finding, stated as a principle derived from the Actionability gate rather than as a list of tiers, so a later change to the finding scheme cannot silently withdraw the menu from a tier the list forgot. Legacy (-L) findings trigger it on the same terms and with the full option pool.scope-and-deferral.md. Where only some open findings clear the spin-off bar, its rendered line names the subset it would carry and the cascade's remaining rules run on the rest, so selecting it cannot sweep in work the fold-in default governs._data/artifact-conventions.md):Smoves fromnonetolowin both the finding-scheme table and the overall rollup, which previously disagreed.nonenow means no authored findings. Legacy-only reviews staynoneon the classification axis, keeping unattended fix cycles out of pre-existing code, while still rendering the menu on the presentation axis.orchestrate/SKILL.md,orchestrate-review/SKILL.md,orchestrate-dev/SKILL.md): The approval threshold defaults tomediumand no effort preset sets it lower, so no default or preset gates approval onT,R, orS, all three of which the finding scheme marks never merge-blocking. The budget threshold stays atlow, so those tiers still draw opportunistic fix cycles.orchestrate/modules/review-cycle.md(two sites) andrespond-to-review/SKILL.mdno longer glossnoneas an absence of actionable findings.orchestrate-dev's piggyback rule namesS-tier suggestions rather than "none-severity" ones, which named nothing onceSmoved._data/complexity-classification.md'snext-steps-after-reviewrow drops its1–2levels cell, since that consumer now routes on determinacy rather than on complexity level.packages/agents/schemas/preferences.jsondropnone, a value neither the README nor any CLI surface documented and which gated approval on a review that found nothing, since every criticality compares at or above it.Closes #1293