fix(update_project): numeric field values now correctly infer NUMBER type - #49073
Conversation
…andler Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business dirs). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes numeric project-field inference and creation.
Changes:
- Infers finite numbers as
NUMBER. - Creates missing number fields.
- Adds numeric-value tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_project.cjs |
Adds NUMBER inference and creation. |
actions/setup/js/update_project.test.cjs |
Tests numeric inference and updates. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| core.warning(`Failed to create field "${fieldName}": ${getErrorMessage(createError)}`); | ||
| continue; | ||
| } | ||
| } else if (isNumberField) { |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes correctly add NUMBER type inference and field creation for finite numeric values. Logic is sound, tests cover zero, negative, and Infinity edge cases, and the checkFieldTypeMismatch function handles NUMBER via exact-type comparison without any special-casing needed. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.1 AIC · ⌖ 5.41 AIC · ⊞ 5.3K
There was a problem hiding this comment.
REQUEST_CHANGES — the fix reintroduces a mirror-image regression: existing SINGLE_SELECT fields holding numeric-looking option values will now get spurious "Field type mismatch" warnings on every run.
💡 Themes
- Correctness regression (high, blocking):
inferFieldDataTypenow unconditionally maps any finite number toNUMBER, with no way for an existingSINGLE_SELECTfield with numeric option labels to avoid a new spurious type-mismatch warning — the exact class of bug this PR claims to fix, just flipped to the opposite field type. No test covers this scenario. - Maintainability (medium): the new NUMBER field-creation GraphQL block duplicates the adjacent DATE/TEXT creation blocks almost verbatim; a shared helper would reduce triplicated maintenance and the risk of divergent fixes (already visible here, since the new
dataTypefragment field wasn't added to the other two branches). - Test coverage for the new happy paths (NUMBER creation, zero value, existing NUMBER field) is solid, but the SINGLE_SELECT-with-numeric-value regression scenario is untested and should be added regardless of how the fix is implemented.
🔎 Code quality review by PR Code Quality Reviewer · auto · 64.8 AIC · ⌖ 4.79 AIC · ⊞ 7.8K
Comment /review to run again
| if (isTextField) { | ||
| return "TEXT"; | ||
| } | ||
| if (typeof fieldValue === "number" && Number.isFinite(fieldValue)) { |
There was a problem hiding this comment.
Any numeric YAML value is now inferred as NUMBER, which is a breaking regression for pre-existing SINGLE_SELECT fields that legitimately hold numeric-looking option labels.
💡 Details
Before this PR, any finite number fell through to SINGLE_SELECT. Now inferFieldDataType unconditionally returns NUMBER for typeof value === "number" && Number.isFinite(value), with no way to opt back into SINGLE_SELECT for numeric values.
Consider a workflow with an existing SINGLE_SELECT field named Priority whose options are 1, 2, 3 (numeric labels), configured with priority: 2. Previously this worked with no warning. After this change, expectedDataType becomes NUMBER, and checkFieldTypeMismatch(fieldName, field, "NUMBER") will fire a Field type mismatch for "Priority": Expected NUMBER but found SINGLE_SELECT warning on every single run, even though the field type is correct and intentional.
The fix in the PR description is scoped to "missing field creates the wrong type" and "existing NUMBER field produces spurious warning" — but it introduces the mirror-image bug for existing SINGLE_SELECT fields with numeric values, which isn't covered by any new test.
Suggested fix: only infer NUMBER when there's no existing field, or when the existing field's dataType is unknown — i.e. pass the existing field (or its dataType) into inferFieldDataType and prefer the already-established type over re-inferring from the value when a field already exists.
| } else if (isNumberField) { | ||
| try { | ||
| field = ( | ||
| await github.graphql( | ||
| `mutation($projectId: ID!, $name: String!, $dataType: ProjectV2CustomFieldType!) { | ||
| createProjectV2Field(input: { | ||
| projectId: $projectId, | ||
| name: $name, | ||
| dataType: $dataType | ||
| }) { | ||
| projectV2Field { | ||
| ... on ProjectV2Field { | ||
| id | ||
| name | ||
| dataType | ||
| } | ||
| } | ||
| } | ||
| }`, | ||
| { projectId, name: normalizedFieldName, dataType: "NUMBER" } | ||
| ) | ||
| ).createProjectV2Field.projectV2Field; | ||
| } catch (createError) { | ||
| core.warning(`Failed to create number field "${fieldName}": ${getErrorMessage(createError)}`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This NUMBER field-creation branch is a near copy-paste of the adjacent DATE-field mutation (differs only in dataType and warning text), tripling the maintenance surface for the same GraphQL call.
💡 Details
The DATE branch (lines ~514-537), this NUMBER branch (~573-597), and the TEXT branch (~544-571) all issue functionally identical createProjectV2Field mutations that differ only in the dataType argument, the response fragment selected, and the warning message on failure. Any future fix to the mutation shape (e.g. adding a new field to select, changing error handling, adding retries) now needs to be applied in three places, and this PR already demonstrates the risk: the fragment ... on ProjectV2Field { id name dataType } was added here but not to the TEXT or DATE branches, so they remain inconsistent in what they request back.
Suggested fix: extract a shared createSimpleProjectField(github, projectId, name, dataType, extraFragment) helper used by DATE/TEXT/NUMBER creation, keeping only the SINGLE_SELECT case (which needs options) separate.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause properly fixed: The missing
NUMBERbranch ininferFieldDataTypeis the real issue and it's addressed directly — not just the symptom. - Good edge case coverage: Zero, negative, and
Infinityvalues are all tested. One small gap:NaNlacks a dedicated unit test (see inline comment). - GraphQL fragment consistency: The new NUMBER field creation mutation uses only the
ProjectV2Fieldfragment (correct), but a brief comment would clarify intent vs. the TEXT branch's union-type pattern.
Positive Highlights
- ✅
Number.isFinitecorrectly excludes bothNaNand±Infinity— handles the tricky JS numeric edge cases right - ✅ Integration tests cover the three key scenarios: create-missing, set-zero, no-spurious-warning on existing field
- ✅ JSDoc return type updated to include
"NUMBER" - ✅
parseFloatfallback in the update path (line 641) handles string-coerced numbers from YAML gracefully
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.5 AIC · ⌖ 5.02 AIC · ⊞ 7K
Comment /matt to run again
| expect(inferFieldDataType("score", Infinity, datePattern)).toBe("SINGLE_SELECT"); | ||
| }); | ||
|
|
||
| it("returns SINGLE_SELECT for boolean field value", () => { |
There was a problem hiding this comment.
[/tdd] Missing test: NaN is not covered by inferFieldDataType unit tests — only Infinity is tested as a non-finite case.
💡 Suggested test
it('returns SINGLE_SELECT for NaN', () => {
expect(inferFieldDataType('score', NaN, datePattern)).toBe('SINGLE_SELECT');
});NaN satisfies typeof value === 'number' but fails Number.isFinite, so it correctly falls to SINGLE_SELECT. A test confirms this and prevents future regressions if the guard condition changes.
@copilot please address this.
| core.warning(`Failed to create field "${fieldName}": ${getErrorMessage(createError)}`); | ||
| continue; | ||
| } | ||
| } else if (isNumberField) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The new NUMBER field creation GraphQL fragment only requests id, name, and dataType — but the TEXT field creation branch (line ~557) also requests the options fragment via ProjectV2SingleSelectField. The NUMBER branch doesn't need options, but the inconsistency in the inline fragment pattern is worth noting: if the API response ever returns a union type, the missing fragment could silently return null for field.
💡 Suggestion
This is low-risk since NUMBER maps to ProjectV2Field (not ProjectV2SingleSelectField), but consider adding a comment explaining why only the ProjectV2Field fragment is needed here — matching the defensive inline-comment style used elsewhere in this function.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel ReportAnalyzed 29 test(s): 27 design, 2 implementation, 0 violations.
|
Triage Summary
Fixes NUMBER field type inference bug in update_project.cjs (spurious warnings / wrong field type creation). Has tests, not draft.
|
|
@copilot run pr-finisher skill |
… branch Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit The |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the skill, address unresolved review feedback if any, and rerun checks once the branch is up to date.
|
|
🎉 This pull request is included in a new release. Release: |
inferFieldDataTypehad noNUMBERbranch — finite JS numbers fell through toSINGLE_SELECT. For an existingNUMBERfield this produced a spurious type-mismatch warning; for a missing field it created the wrong type entirely.Changes
update_project.cjsinferFieldDataType: returns"NUMBER"fortypeof value === "number" && Number.isFinite(value)(handles zero, negatives, floats; excludesNaN/Infinitywhich still fall toSINGLE_SELECT). JSDoc return type updated.applyFieldUpdates: addsisNumberFieldflag and a newelse if (isNumberField)creation branch that callscreateProjectV2FieldwithdataType: "NUMBER"when the field is missing.update_project.test.cjsinferFieldDataTypetest that assertedSINGLE_SELECTfor a numeric value.NUMBERfield, set zero, and verify no spurious warning on an existingNUMBERfield.Example
{ "type": "update_project", "fields": { "Comment Count": 30 } }Previously logged
Field type mismatch for "Comment Count": Expected SINGLE_SELECT but found NUMBER. Now resolves correctly with no warning.