Add dedicated outcome evaluators for replace_label and close operations; fill spec/compliance gaps#44798
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds type-specific outcome evaluation and aligns related specifications and compliance fixtures.
Changes:
- Adds Go
replace_labeland JavaScript close evaluators. - Expands targeted evaluator tests.
- Updates outcome, compliance, governance, and OTel specifications.
Show a summary per file
| File | Description |
|---|---|
specs/safe-output-outcome-evaluation.md |
Updates evaluator status matrix. |
specs/replace-label-spec.md |
References compliance fixtures. |
specs/replace-label-compliance/rl-002-allowlist-enforcement.yaml |
Adds allowlist scenarios. |
specs/replace-label-compliance/README.md |
Registers RL-002 coverage. |
specs/otel-observability-spec.md |
Adds span-attribute cases. |
specs/intent-attribution-compliance/README.md |
Defines compliance scenarios. |
specs/intent-attribution-agent-governance.md |
Adds safeguards and sync guidance. |
pkg/cli/outcome_eval.go |
Registers replace_label evaluator. |
pkg/cli/outcome_eval_update.go |
Tracks replacement labels. |
pkg/cli/outcome_eval_update_test.go |
Tests retained replacement. |
pkg/cli/outcome_eval_label.go |
Adds replacement evaluator. |
actions/setup/js/evaluate_outcomes.test.cjs |
Tests label and close outcomes. |
actions/setup/js/evaluate_outcomes.cjs |
Adds dedicated JavaScript evaluators. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Medium
| func evalReplaceLabel(item CreatedItemReport, repoOverride string) OutcomeReport { | ||
| return evalRetainedUpdate(item, repoOverride, "label replacement", extractCurrentIssueUpdateState, false) |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44798 does not have the 'implementation' label and has only 41 new lines of code in business logic directories (threshold: 100). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (8 tests)
|
There was a problem hiding this comment.
Review: Add dedicated outcome evaluators for replace_label and close operations
The core mechanics are well-structured — new evaluators are clean, testable, and correctly routed. Two blocking correctness issues found.
Blocking
-
evaluateCloseIssueandevaluateClosePullRequestnever setoutcome_status,evidence_strength, orsignal(inline comments filed). All terminal return paths leaveout.outcome_status = "unknown"andout.evidence_strength = "weak". Since the pipeline readsevalResult.outcome_statusdirectly when writing to the JSONL, those fields will be wrong in every accepted/rejected case. ThenormalizeOutcomefallback produces the rightoutcome_statusfor the aggregated stats, but the EvalResult struct should be self-consistent — as every other evaluator in the file demonstrates. -
evaluateClosePullRequestmerged-rejection path also leavesoutcome_statusat "unknown" —normalizeOutcomehas no dedicated branch forrejected + "merged", so that case falls to the genericrejectedswitch arm and emitssignal: "rejected"/evidence_strength: "medium"instead of a more precise signal.
Non-blocking
- T-RL-021 is duplicated in the test-ID mapping table in
specs/replace-label-compliance/README.md. Duplicate IDs should be merged or given distinct sub-IDs. - Missing test coverage for the api-error and missing-reference early-exit paths in the new close evaluators.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 81.2 AIC · ⌖ 4.74 AIC · ⊞ 4.8K
|
|
||
| it("evaluates update_issue retained and reverted states from persisted execution metadata", () => { | ||
| const retained = evaluateItem( | ||
| { |
There was a problem hiding this comment.
Missing test coverage for api-error and missing-reference paths in evaluateCloseIssue and evaluateClosePullRequest. The tests cover the happy path (accepted/rejected), but not: 1) !repo or !number (missing reference detail), 2) API error path (which also calls setPendingAge, affecting pending_age_sec). The missing-number path was tested for add_labels (line 235 in this file) but not for close_issue or close_pull_request. @copilot please address this.
There was a problem hiding this comment.
Added in the same commit. Both evaluateCloseIssue and evaluateClosePullRequest now have tests covering:
- Missing reference (
!repo || !number) →result: "unknown",detail: "missing issue/pull request reference" - API error (
setPendingAgepath) →result: "unknown",detail: "api error",pending_age_secis a number
The existing happy-path tests were also updated to assert outcome_status, evidence_strength, and signal on every branch, and a merged_at-only detection test was added for evaluateClosePullRequest.
| |---------|---------|-------------| | ||
| | T-RL-020 | `rl-001-glob-semantics.yaml` | Star glob matches label name substring | | ||
| | T-RL-021 | `rl-001-glob-semantics.yaml` | Exact pattern matches only exact name | | ||
| | T-RL-021 | `rl-002-allowlist-enforcement.yaml` | Non-empty allowed-add accepts a matching label | |
There was a problem hiding this comment.
T-RL-021 appears twice in the test-ID mapping table (lines 60 and 61), mapping to two different fixtures. This is not incorrect per se but test IDs are expected to be unique. If T-RL-021 is really two distinct sub-cases they should either share the same row with multiple fixtures listed, or be assigned distinct IDs (e.g., T-RL-021a / T-RL-021b). A duplicate ID will confuse test-suite harnesses that index by ID. @copilot please address this.
There was a problem hiding this comment.
Fixed in the same commit. The two rl-002-specific rows were renamed to T-RL-021b and T-RL-022b so every row in the table has a unique ID. The rl-002 fixture header on line 15 was also updated to reference T-RL-021b and T-RL-022b.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on signal-precision gaps and a missing compliance cross-reference.
📋 Key Themes & Highlights
Key Issues
- Signal precision (
/tdd) —evaluateCloseIssueandevaluateClosePullRequestreturnout.detail = "closed"on success, butnormalizeOutcomehas no registered branch foraccepted + "closed". The result falls through to the generic"acted_on"signal (medium evidence), silently downgrading every successfully-closed outcome in the JSONL. Similarly,out.detail = "merged"on theclose_pull_requestrejection path is unregistered. - API-error path coverage (
/tdd) — ThesetPendingAgebranches in both new JS evaluators (lines 578, 644) have no tests. A future regression there would be invisible. - Compliance cross-reference gap (
/grill-with-docs) —rl-003-blocklist-ordering.yamlwas added separately butspecs/replace-label-compliance/README.mdwas not updated for it in this PR. The RL-002 row was added without a companion RL-003 row, leaving the README incomplete.
Positive Highlights
- ✅ Clean dedicated evaluators that replace generic fallbacks — the intent and logic are clear
- ✅
evaluateClosePullRequestcorrectly distinguishes merged (rejected) from unmerged-closed (accepted) with a well-placed comment explaining the design decision - ✅ Good test coverage for the happy paths and the
missing-numberedge case inadd_labels - ✅
evalReplaceLabelis a satisfying thin wrapper — reusesevalRetainedUpdateinstead of duplicating logic
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 85.7 AIC · ⌖ 4.93 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/evaluate_outcomes.cjs:591
[/tdd] out.detail = "closed" for an accepted close_issue is not handled by normalizeOutcome, so it falls through to the generic "acted_on" signal (medium evidence). The outcome JSONL loses precision on every successfully-closed issue evaluation.
<details>
<summary>💡 Suggested fix</summary>
Add a branch in normalizeOutcome:
if (result === "accepted" && normalizedDetail === "closed") {
return { outcome_status: "accepted", evidence_strength: "strong", signal: "closed" };
…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.cjs:664</summary>
**[/tdd]** `out.detail = "merged"` for a rejected PR is also unregistered in `normalizeOutcome`, so it falls through to the generic `"rejected"` signal (medium evidence) instead of a strong, named signal like `"closed_by_merge"`.
<details>
<summary>💡 Suggested fix</summary>
Register the `merged` rejection case in `normalizeOutcome`:
```js
if (result === "rejected" && normalizedDetail === "merged") {
return { outcome_status: "rejected", evidence_strength: "strong", signal: "closed_by_merge…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.test.cjs:73</summary>
**[/tdd]** The new `close_issue` tests cover accepted and rejected states, but the API-error path (when `api(...)` returns `null` or lacks `state`) is not tested. This gap means the `setPendingAge` call on line 578 of `evaluate_outcomes.cjs` has no coverage, so a regression there would go undetected.
<details>
<summary>💡 Suggested test</summary>
```js
it("returns unknown with pending_age when close_issue API call fails", () => {
const item = {
type: "close_issue",
number: 77,
r…
</details>
<details><summary>specs/replace-label-spec.md:539</summary>
**[/grill-with-docs]** `rl-003-blocklist-ordering.yaml` is already present in the compliance directory but was not added to the README cross-reference table in this PR — the RL-002 update adds `rl-002` to the README but leaves `rl-003` undocumented there. This creates an asymmetry that makes the compliance suite harder to navigate.
<details>
<summary>💡 Suggested fix</summary>
Add a row in `specs/replace-label-compliance/README.md` for `rl-003-blocklist-ordering.yaml` alongside the new `rl-00…
</details>There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs block merge; one design issue needs a decision.
Blocking issues (must fix)
1. outcome_status / evidence_strength / signal never populated in JS close evaluators
Both evaluateCloseIssue and evaluateClosePullRequest initialize these fields as "unknown"/"weak"/"unknown" and never update them on any return path. The aggregation loop re-derives them via normalizeOutcome() so dashboard counts are correct, but the exported EvalResult object carries a permanently malformed schema for any direct consumer. All other evaluators set these fields explicitly.
2. evalReplaceLabel uses full atomic label-set comparison
replace_label swaps exactly one label pair, but evalRetainedUpdate compares the entire labels set against afterState.labels. Any unrelated label added/removed after execution falls into state_replaced → rejected even when the swap itself succeeded. The correctness predicate should be: target label present AND removed label absent — not entire label set unchanged.
Non-blocking concerns
3. rejected/reopened for a never-closed item is factually wrong
If close_issue fails silently, the issue stays open. The evaluator returns detail="reopened" — implying it was previously closed. These have different attribution implications.
4. Merged PR guard has no merged_at fallback
Transient GitHub API lag can briefly return merged: false. Adding || pullRequest.merged_at != null closes the race at zero cost.
5. New tests do not assert outcome_status / evidence_strength / signal
Directly caused by issue #1 — without these assertions the broken schema is invisible to tests.
🔎 Code quality review by PR Code Quality Reviewer · 114.1 AIC · ⌖ 5.03 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/evaluate_outcomes.cjs:591
outcome_status, evidence_strength, and signal are never set: all three fields stay at their initial "unknown"/"weak"/"unknown" on every return path, so any consumer of the raw EvalResult object gets a broken schema.
<details>
<summary>💡 Suggested fix</summary>
Set all three fields alongside result in each branch, as every other evaluator does:
if (issue.state === "closed") {
out.result = "accepted";
out.outcome_status = "accepted";
out.evidence_strength = "strong…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.cjs:599</summary>
**'rejected'/'reopened' is semantically wrong for an issue that was never closed**: if the action failed silently and the issue is still 'open', the evaluator returns `result='rejected', detail='reopened'` — but the issue was never closed in the first place, so 'reopened' is factually incorrect.
<details>
<summary>💡 Suggested fix</summary>
Distinguish the two cases. The same applies to `evaluateClosePullRequest`:
```js
if (issue.state !== "closed") {
// Was it ever closed? Check closed_at…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.cjs:663</summary>
**Merged PR is checked via `merged === true` only — transient race with `merged_at`**: the GitHub API can briefly return `merged: false` for a PR that was just merged before the `merged_at` field propagates. A merged PR could slip through the guard and be evaluated as 'accepted/closed' instead of 'rejected/merged'.
<details>
<summary>💡 Suggested fix</summary>
Use `merged_at` as a secondary signal:
```js
if (pullRequest.merged === true || pullRequest.merged_at != null) {
out.result = "reje…
</details>
<details><summary>pkg/cli/outcome_eval_label.go:14</summary>
**`evalReplaceLabel` uses atomic label-set comparison — any unrelated label change causes a false rejection**: `evalRetainedUpdate` with `mutableTrackedFields("replace_label") = ["labels"]` checks whether the entire label set matches `afterState.labels`. If any unrelated label is added or removed after the `replace_label` executes, `currentState.labels != afterState.labels`, the comparison falls into the `state_replaced` branch, and the outcome is reported as `rejected` — even though the actual…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.test.cjs:272</summary>
**Tests for `close_issue`/`close_pull_request` never assert `outcome_status`, `evidence_strength`, or `signal`**: only `result` and `detail` are verified, so the three fields that are structurally broken (always `"unknown"/"weak"/"unknown"`) are not caught by any test.
<details>
<summary>💡 Suggested fix</summary>
Add assertions for the normalized fields:
```js
const r = evaluateItem(item, "acme/repo", { ghAPI: closed });
expect(r.result).toBe("accepted");
expect(r.outcome_status).toBe("acce…
</details>…ce_label, partial close status Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three unresolved review threads:
Commits: |
|
@copilot please run the
|
All unresolved review feedback has been addressed. Commit Changes in the pending commitBlocking fixes:
Tests updated:
Non-blocking:
Local validation: |
|
@copilot please run the
|
…eturn paths; add missing tests; deduplicate T-RL IDs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
All review feedback has already been addressed in the prior commits. Local validation confirms everything is clean:
Hand-off: A maintainer needs to approve the pending CI workflow runs (or close/reopen the PR to re-trigger them) before merge. |
|
@copilot please run the Unresolved review feedback to address:
|
🤖 PR Triage
Score breakdown: Impact 38/50 · Urgency 20/30 · Quality 15/20 Rationale: Adds dedicated outcome evaluators filling spec/compliance gaps (+776/-18 lines, 13 files). CI fully passing, 11 reviews. High-value correctness improvement to the evaluation pipeline.
|
|
🎉 This pull request is included in a new release. Release: |
Outcome evaluation still had generic fallback behavior in a few high-signal paths, most notably
replace_labelin Go andclose_issue/close_pull_requestin the JS runtime. This change replaces those gaps with type-specific evaluation logic and updates the related specs/compliance fixtures to reflect the current implementation surface.Outcome evaluation
evalReplaceLabeland routereplace_labelaway fromevalGenericStickybefore_state.labels/after_state.labelsagainst current issue stateclose_issueclose_pull_requestadd_labelsevaluation to use persistednumber/repometadata, not just URL-derived targetsTargeted coverage
replace_labeloutcomesadd_labelswith persisted metadataadd_labelsmissing-number error pathclose_issueclose_pull_requestSpec sync
specs/safe-output-outcome-evaluation.mdto reflect:replace_labelas dedicated/partial in Goadd_labelsclose_issueandclose_pull_request