Repository navigation
Unify Go and JavaScript outcome semantics and evidence rules - #67519
Conversation
Use conservative action-specific acceptance and shared conformance fixtures. Preserve execution identifiers and normalized evidence through collector summaries and telemetry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extract action classification, attribution and summary helpers. Preserve permissive API field decoding through checked accessors and remove redundant evidence assignments. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
✅ Ponytail Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One small opportunity to make the normalization table leaner.
net: -18 lines possible.
Generated by ✂️ Ponytail Reviewer for #67519 · codex · gpt56 · 17.6 AIC · ⌖ 5.08 AIC · ⊞ 13.5K
Comment /ponytail to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1214 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 🔍 Evidence used
❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. This PR is a deliberate behavioural break (
|
There was a problem hiding this comment.
🟡 Changes recommended
Label casing, zero-touch accounting, identifier validation, and collector guidance can still produce incorrect classifications.
6 open findings
What changed in this PR
Unifies Go and JavaScript outcome classification with conservative evidence rules, shared conformance fixtures, telemetry updates, and a formal TLA+ model.
Changes:
- Adds action-specific evidence evaluation and cross-runtime conformance coverage.
- Preserves unknown, error, lifecycle, human-effort, and evidence-strength metrics.
- Updates schemas, documentation, telemetry, and the scheduled collector.
Static workflow review found no security-control weakening; compilation and scanners were not independently rerun.
| File | Description |
|---|---|
specs/outcomes/README.md |
Documents the formal model and verification. |
specs/outcomes/OutcomeEvaluation.tla |
Defines outcome semantics and invariants. |
specs/outcomes/OutcomeEvaluation.cfg |
Configures TLC checks. |
specs/outcomes/check.mjs |
Runs formal checks and negative controls. |
schemas/logs-jsonl.schema.json |
Adds unknown summary counts. |
schemas/audit.schema.json |
Adds unknown audit counts. |
pkg/cli/testdata/outcome_conformance.json |
Provides shared conformance cases. |
pkg/cli/outcome_evaluation.go |
Normalizes typed outcome evidence. |
pkg/cli/outcome_eval.go |
Adds counting, pagination, and parsing helpers. |
pkg/cli/outcome_eval_workflow.go |
Refines workflow dispatch outcomes. |
pkg/cli/outcome_eval_workflow_test.go |
Updates unsupported discussion expectations. |
pkg/cli/outcome_eval_update.go |
Uses typed retained-state results. |
pkg/cli/outcome_eval_test.go |
Tests summary reconciliation and parsing. |
pkg/cli/outcome_eval_review.go |
Adds attributable review classification. |
pkg/cli/outcome_eval_pr.go |
Adds complete PR effort evidence. |
pkg/cli/outcome_eval_label.go |
Verifies executed label deltas. |
pkg/cli/outcome_eval_jsonl.go |
Exports human-review counts. |
pkg/cli/outcome_eval_issue.go |
Refines issue lifecycle classification. |
pkg/cli/outcome_eval_helpers.go |
Tightens actor and closure handling. |
pkg/cli/outcome_eval_generic.go |
Adds evidence-specific generic evaluators. |
pkg/cli/outcome_eval_formal_test.go |
Updates formal behavior tests. |
pkg/cli/outcome_eval_evidence.go |
Adds shared evidence helpers. |
pkg/cli/outcome_eval_comment.go |
Refines comment engagement evaluation. |
pkg/cli/outcome_eval_agent.go |
Requires attributable agent PRs. |
pkg/cli/outcome_conformance_test.go |
Tests Go against shared fixtures. |
docs/src/content/docs/reference/outcomes.md |
Documents revised semantics. |
actions/setup/js/safe_output_manifest.test.cjs |
Tests persisted execution identities. |
actions/setup/js/safe_output_manifest.cjs |
Persists milestone and commit identities. |
actions/setup/js/outcome_types.d.ts |
Defines JavaScript outcome results. |
actions/setup/js/outcome_review_evaluators.cjs |
Implements review/update evaluators. |
actions/setup/js/outcome_evidence.cjs |
Adds JavaScript evidence helpers. |
actions/setup/js/outcome_conformance.test.cjs |
Tests JavaScript conformance. |
actions/setup/js/outcome_action_evaluators.cjs |
Implements action-specific evaluation. |
actions/setup/js/evaluate_outcomes.test.cjs |
Consolidates evaluator tests. |
actions/setup/js/emit_outcome_spans.test.cjs |
Tests expanded telemetry. |
actions/setup/js/emit_outcome_spans.cjs |
Exports new outcome attributes. |
.github/workflows/outcome-collector.md |
Updates collector reporting semantics. |
.github/workflows/outcome-collector.lock.yml |
Regenerates workflow metadata. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with minor non-blocking suggestions on an otherwise well-constructed unification effort.
📋 Key Themes & Highlights
Key Themes
- Minor internal duplication (Go):
outcomeCloseActor(outcome_eval_evidence.go) andisLatestCloseByBot(outcome_eval_helpers.go) implement nearly identical bot-close detection; consolidating would reinforce the parity goal of this PR. - Brittle string-matching for persistent-404 classification (JS):
outcome_action_evaluators.cjs's catch block reconstructs expected endpoint strings per type rather than having evaluators mark their primary fetch explicitly. - Test gap on the new
ghAPIpagination/error wrapper: the rewrittenghAPI()(now load-bearing for "deleted" classification via HTTP-status parsing) has no direct unit test, unlike its Go counterpart which is covered byTestOutcomeAPIArrayPagination.
Positive Highlights
- ✅ Excellent cross-runtime discipline: 63 shared conformance fixtures (
pkg/cli/testdata/outcome_conformance.json) consumed by both the Go test suite andoutcome_conformance.test.cjsis a strong parity guardrail. - ✅ Clean extraction of
outcome_action_evaluators.cjs,outcome_review_evaluators.cjs, andoutcome_evidence.cjsfrom the 1800-line monolith — each evaluator is now independently readable. - ✅ TLA+ formal spec (
specs/outcomes/OutcomeEvaluation.tla) with a model-checking runner is a rigorous addition for an evidence-classification system with this many branches. - ✅ Documentation (
outcome-collector.md,docs/reference/outcomes.md) was updated in lockstep with the newunknown/error/lifecyclestatuses — no drift between behavior and docs.
Note on diff truncation
The pre-fetched diff was capped at 3000 lines and only covered .github/workflows/outcome-collector.md, emit_outcome_spans.cjs/test, and the start of evaluate_outcomes.cjs/test. I read the remaining high-impact files (outcome_action_evaluators.cjs, outcome_review_evaluators.cjs, outcome_evidence.cjs, the Go pkg/cli/outcome_eval_*.go files, and the conformance fixtures) directly from the checked-out tree to complete this review, and confirmed go build ./pkg/cli/... succeeds. JS tests could not be executed in this sandbox (npm registry blocked by a self-signed-cert proxy), so JS findings are based on static reading of the refactored modules and existing test files rather than a live test run.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 260.9 AIC · ⌖ 15 AIC · ⊞ 10.3K
Comment /matt to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Why
The CLI and scheduled outcome collector classified identical safe outputs differently and could report acceptance from target existence or unrelated activity. This change gives both runtimes the same conservative, action-specific evidence contract.
Approach
Validation
Focused Go outcomes, the shared Go/JavaScript corpus, JavaScript transport tests, typechecking, and TLC checks pass. The final repository progress gate covers formatting, standard/custom lint, impacted unit tests, schemas, and workflow lock drift.
Architecture decision
ADR: ADR-67519: Unify Outcome Evidence Semantics Across the Go and JavaScript Runtimes
The completed proposal records the rationale, alternatives, compatibility impact, and formal-verification limits. Architectural acceptance remains subject to maintainer review.
Scope and compatibility
Existing manifests without required execution evidence may produce unknown rather than accepted. Generic existence checks no longer inflate acceptance, and stale open creations are not ignored merely because time elapsed. Collector revisit/retry scheduling and per-outcome AIC attribution are unchanged. The pre-existing docs link failure in experimental/drive-memory.md is unrelated to this change.