Refactor objective mapping source-of-truth: remove dead label/value constants and align docs/specs to runtime defaults - #52831
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for implementing this refactor! 🎯 You've tackled the key issues cleanly:
Observation on contribution type: What looks good:
Ready for technical review by maintainers.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52831 does not have the 'implementation' label and has only 79 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines.
Review notes
I checked the refactor against the touched code paths and did not find a correctness, concurrency, or performance regression in the runtime behavior. The main behavioral change is the intentional removal of stale exported score constants in favor of DefaultObjectiveMapping() as the sole source of truth, and the updated tests/docs are aligned with that direction.
The background grumpy-coder pass did not yield usable findings during the review window, so this verdict is based on the direct code review of the diff.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.73 AIC · ⌖ 5.17 AIC · ⊞ 4K
Comment /review to run again
There was a problem hiding this comment.
Review: Approved
This PR correctly removes dead label/value constants that had diverged from the runtime defaults in DefaultObjectiveMapping() (e.g., ObjectiveValueSecurityFix was 70 in constants but 75 at runtime; ObjectiveValueHighPriority was 35 vs runtime 50). Key improvements:
- Single source of truth:
DefaultObjectiveMapping()is now the only canonical definition of label values. - DRY refactor:
objectiveValueForLabelwith nil-guard consolidates 4 duplicate normalization call sites. - Constants preserved where needed:
MultiLabelLogicMax/Sum/Firstcorrectly moved to the main file. - Tests updated:
TestSpec_DefaultObjectiveMapping_Valuesnow tests the live function output rather than stale constants.
No correctness issues found. The README values match the actual Go function.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.7 AIC · ⌖ 7.05 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Refactors objective scoring so DefaultObjectiveMapping() defines runtime defaults.
Changes:
- Removes obsolete objective label/value constants.
- Centralizes normalized label lookup.
- Aligns tests and documentation with runtime defaults.
Show a summary per file
| File | Description |
|---|---|
pkg/github/label_objective_mapping.go |
Adds shared lookup helpers and uses live logic constants. |
pkg/github/label_objective_mapping_constants.go |
Removes legacy exported constants. |
pkg/github/spec_test.go |
Tests default mappings directly. |
pkg/github/README.md |
Documents current defaults and logic constants. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52831 · auto · 21.1 AIC · ⌖ 7.59 AIC · ⊞ 6.8K
Comment /ponytail to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /grill-with-docs — commenting on a few targeted issues; no blocking changes.
📋 Key Themes & Highlights
Key Themes
- Test spec completeness:
TestSpec_DefaultObjectiveMapping_ExcludesUnmappedLabelsonly checks"bug"— the former constants file had ~15 excluded labels worth documenting as intentional. - Nil-guard placement:
objectiveValueForLabeladds a nil check that is already handled by all callers; the contract should live in one place. - Documentation gap: The README doesn't explain why formerly-constant labels (
bug,testing,reliability) are now absent from the default mapping, which may confuse future contributors.
Positive Highlights
- ✅ Excellent call removing the drifted constants file — tautological tests are a subtle but real correctness risk.
- ✅
objectiveValueForLabelhelper is a clean deep-module move: one normalize+lookup path instead of three. - ✅ Replacing raw string literals with
MultiLabelLogicMax/Sum/Firstin the switch makes intent explicit. - ✅ New spec test asserts real behaviour rather than round-tripping through exported constants.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 47 AIC · ⌖ 8.64 AIC · ⊞ 7K
Comment /matt to run again
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Maintainer-readiness triage is complete on current HEAD
This branch is ready for a fresh CI pass and maintainer handoff. |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address any remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Maintainer-readiness triage is complete on current HEAD
No additional code changes were required in this pass; branch is ready for maintainer CI re-trigger and handoff. |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address any remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Maintainer-readiness triage is complete on current HEAD
This branch is ready for a fresh maintainer-triggered CI pass and handoff. |
Maintainer follow-up is complete on current HEAD
This branch is maintainer-ready for a fresh maintainer-triggered CI pass and final merge review. |
|
@copilot quick triage for this PR:
Run: https://github.com/github/gh-aw/actions/runs/31883373617
|
Triage complete on current HEAD
This PR is maintainer-ready for a fresh maintainer-triggered CI pass and final merge review. |
PR Triage
Removes dead objective-mapping constants, aligns docs to runtime defaults. CI passing (5/5). Low functional risk despite refactor scope. Automated triage — see [PR Triage Report] for full context.
|
Maintainer-readiness follow-up is complete on current HEAD
This PR is maintainer-ready for a fresh maintainer-triggered CI pass and final merge review. |
|
🎉 This pull request is included in a new release. Release: |
pkg/githubcarried a full table of exported objective label/value constants that were not used by runtime code and had drifted fromDefaultObjectiveMapping(), creating contradictory scoring semantics. This change makes runtime defaults the single source of truth and keeps only the logic constants that are actively used by the implementation.Single source of truth for objective scores
pkg/github/label_objective_mapping_constants.go(dead exported label/value constants).DefaultObjectiveMapping().Use live constants in computation path
MultiLabelLogicMax|Sum|Firstas exported constants.label_objective_mapping.goand replaced raw string literals inComputeObjectiveValue.Deduplicate label normalization/lookup logic
ToLower/TrimSpacelookup sites while preserving behavior.Spec and package docs now reflect runtime behavior
pkg/github/spec_test.goto assert default mapping values directly instead of tautological constant checks.pkg/github/README.mdto document multi-label logic constants and the default mapping table fromDefaultObjectiveMapping().Run context: https://github.com/github/gh-aw/actions/runs/31867777089> Generated by 👨🍳 PR Sous Chef · gpt54 · 33.2 AIC · ⌖ 5.85 AIC · ⊞ 8.7K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31883373617> Generated by 👨🍳 PR Sous Chef · gpt54 · 16.4 AIC · ⌖ 5.78 AIC · ⊞ 6.3K · ◷