feat(governance): formalise issue fields v2 model and validation#437
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Note
|
🔍 Reviewer Summary for PR #437CI Status: ✅ Recommendations
|
github-actions
Bot
added
area:documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the canonical issue and project field configuration to version 2, introducing organization-level issue fields (such as custom fields for Domain, Team, and Effort) and project-specific iteration settings. It also updates the corresponding documentation and the validation script to enforce these new schemas. Feedback on these changes highlights a potential crash in the validation script if custom fields are malformed or missing options, and recommends documenting the mentioned migration procedures in a central /docs/MIGRATION.md file.
| const keySet = new Set(customFields.map((f) => f.key)); | ||
| for (const key of requiredCustomKeys) { | ||
| if (!keySet.has(key)) fail(`Missing required custom field definition: ${key}`); | ||
| } | ||
|
|
||
| const validTypes = new Set(['single_select', 'number', 'date', 'text']); | ||
| for (const field of customFields) { | ||
| if (!field.key || typeof field.key !== 'string') fail('Each custom field requires a string key'); | ||
| if (!field.type || !validTypes.has(field.type)) { | ||
| fail(`Custom field "${field.key || 'unknown'}" has invalid type "${field.type}"`); | ||
| } | ||
| if (field.type === 'single_select') { | ||
| if (!Array.isArray(field.options) || field.options.length === 0) { | ||
| fail(`Single select custom field "${field.key}" must include options`); | ||
| } | ||
| if (field.options.length > 50) { | ||
| fail(`Single select custom field "${field.key}" exceeds 50 options`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation script does not safely handle cases where elements of customFields are null or not objects. If a custom field is malformed (e.g., defined as null or a primitive value in the YAML), calling f.key or accessing field.key / field.type will throw a TypeError and crash the validation process instead of failing gracefully with a clean error message.
Additionally, because fail does not stop execution, if field.options is undefined or not an array, checking field.options.length on line 140 will throw a TypeError and crash the script.
We should safely map the keys and check that each field is a valid object before accessing its properties, and use else if to avoid evaluating length on non-arrays.
const keySet = new Set(customFields.map((f) => f && typeof f === 'object' ? f.key : undefined));
for (const key of requiredCustomKeys) {
if (!keySet.has(key)) fail(`Missing required custom field definition: ${key}`);
}
const validTypes = new Set(['single_select', 'number', 'date', 'text']);
for (const field of customFields) {
if (!field || typeof field !== 'object') {
fail('Each custom field must be an object');
continue;
}
if (!field.key || typeof field.key !== 'string') {
fail('Each custom field requires a string key');
continue;
}
if (!field.type || !validTypes.has(field.type)) {
fail(`Custom field "${field.key}" has invalid type "${field.type}"`);
continue;
}
if (field.type === 'single_select') {
if (!Array.isArray(field.options) || field.options.length === 0) {
fail(`Single select custom field "${field.key}" must include options`);
} else if (field.options.length > 50) {
fail(`Single select custom field "${field.key}" exceeds 50 options`);
}
}
}
| operational_guidance: | ||
| - Keep issue and project fields aligned; labels remain automation fallback. | ||
| - Renaming custom fields is allowed; update this YAML + docs + validators in same PR. | ||
| - Deleting custom fields requires migration of existing data and view updates first. |
There was a problem hiding this comment.
To ensure contributors can easily follow migration rules across the repository, please document these migration notes and procedures in a central /docs/MIGRATION.md file as per our general guidelines.
References
- Document migration maps and notes in a central
/docs/MIGRATION.mdfile to ensure contributors can follow migration rules mentioned in README files across the repository.
🔍 Reviewer Summary for PR #437CI Status: ❌ Recommendations
|
1 participant
No description provided.