codeant-ai-for-open-source[bot] commented on PR #36961: URL: https://github.com/apache/superset/pull/36961#issuecomment-3721335903
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36961/files#diff-8465dbe7c668bd32ba74030d8c96150ef49084ff578035c5e887491bdf2cedb4R270-R280'><strong>Exit code logic</strong></a><br>The script's comment says it should "Return non-zero if there are changes (useful for CI)" but the implementation always returns 0. CI/automation relying on this script to fail when role changes are present will not detect changes. Consider returning non-zero when diff["changes"] is non-empty.<br> - [ ] <a href='https://github.com/apache/superset/pull/36961/files#diff-2f21e7b0b665a94322a41c9e22ad62f9570c043715aefef5596d198710f53dd6R120-R139'><strong>Reference resolution</strong></a><br>The current `resolve_references` implementation only resolves one-level `$NAME` placeholders and converts all resolved values to strings when building the final set. This loses original types and doesn't handle recursive or transitive references (A -> B -> C) or detect circular references. Validate resolution semantics for nested references and ensure cycles are handled safely.<br> - [ ] <a href='https://github.com/apache/superset/pull/36961/files#diff-2f21e7b0b665a94322a41c9e22ad62f9570c043715aefef5596d198710f53dd6R55-R75'><strong>AST node coverage</strong></a><br>`parse_set_or_tuple` and `parse_element` handle `ast.Set`, `ast.Tuple`, `ast.Call`, `ast.BinOp` and `ast.Name`, but they don't handle `ast.Attribute` (e.g., `module.CONST`), `ast.List`, or other literal forms that might appear in real code. Missing coverage can cause silently empty results for some constants.<br> - [ ] <a href='https://github.com/apache/superset/pull/36961/files#diff-2f21e7b0b665a94322a41c9e22ad62f9570c043715aefef5596d198710f53dd6R126-R138'><strong>Lossy normalization</strong></a><br>Values are converted to strings early (via `str(v)` in `resolve_references`) and deduplicated using those strings. This loses type information (tuples vs strings vs ints) and may merge distinct values that stringify the same way. Consider preserving original types until serialization.<br> - [ ] <a href='https://github.com/apache/superset/pull/36961/files#diff-8465dbe7c668bd32ba74030d8c96150ef49084ff578035c5e887491bdf2cedb4R121-R137'><strong>Role implication mapping coverage</strong></a><br>The hard-coded `role_implications` mapping may not include all constant names or variants (e.g., different casing). If a constant name in the extracted JSON doesn't match exactly, its role hint will be generic ("Various"). This can cause reviewers to miss important context; consider making the mapping configurable or using a more systematic mapping approach.<br> </td></tr> </table> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
