Conversation
drdavella
force-pushed
the
detector-findings
branch
3 times, most recently
from
15ef3bc to
c48da73
Compare
drdavella
force-pushed
the
detector-findings
branch
from
c48da73 to
9677ec6
Compare
| "type": "string", | ||
| "description": "The ID of the rule" | ||
| }, | ||
| "name": { |
There was a problem hiding this comment.
I can imagine also a web link to the rule definition.
I think the name and ID might be the same in some cases. For CodeQL, this is definitely the case. Is that okay?
There was a problem hiding this comment.
Yeah I think that's fine. This purpose is to enable us to be consistent with "Fixes rule from tool (id: )", but I don't think it's a problem if name == id.
| "description": "Reason the finding was not fixed" | ||
| } | ||
| }, | ||
| "additionalProperties": true, |
There was a problem hiding this comment.
I like this section, but it could be potentially massive. Scanners commonly find pathological conditions where they end up reporting 50,000 things. Is there anything we can/should do in the spec to protect ourselves from massive, redundant info?
There was a problem hiding this comment.
We can add a maxItems although it's incumbent upon the consumer to enforce that. Basically it would mean we'd ignore any results beyond maxItems.
| "name": { | ||
| "type": "string", | ||
| "description": "The name of the rule" | ||
| }, |
There was a problem hiding this comment.
I think description is too onerous to add for codemod authors. We should provide a link to their documentation, if they have one, and that's it.
There was a problem hiding this comment.
Okay that's fair. So optional ruleUrl or something similar?
There was a problem hiding this comment.
Yes, I think rule URL is needed and scratches this itch
| }, | ||
| "additionalProperties": true, | ||
| "required": ["guid", "fixed"], | ||
| "if": { |
There was a problem hiding this comment.
TIL you can have if statements in here
| }, | ||
| "description": { | ||
| "type": "string", | ||
| "description": "A description of the rule" |
There was a problem hiding this comment.
This raises internationalization concerns for me, but I already have those same concerns about other descriptions in CodeTF, so we're no worse off by adding this. 🙏🏻'ing that LLM can just do all the English -> SomethingElse translations.
| "if": { | ||
| "properties": { | ||
| "fixed": { "boolean": false } | ||
| }, | ||
| "required": ["reason"] |
| "type": "string", | ||
| "description": "Short description of the tool vendor rule that detected the issue" | ||
| "findings": { | ||
| "type": "array", |
There was a problem hiding this comment.
What's the case where a codemod would have multiple findings for a single tool within a single change?
There was a problem hiding this comment.
The detectionTool belongs to the result and not to the change so I think we can expect potentially multiple findings here.
There was a problem hiding this comment.
I see, I misread that.
I know it's not explicit in the data model, but to help with my understanding, what's the relationship that we expect between change and finding? Is it, every finding that has fixed: true has one or more changes ?
There was a problem hiding this comment.
Yeah I think so. Would it make sense to include the same finding ID as an optional field of cange or changeSet as well?
There was a problem hiding this comment.
I'll vote YAGNI for now but we can consider enriching the data model with that link later.
| "type": "string", | ||
| "description": "ID of the tool vendor rule that detected the issue" | ||
| "rule": { | ||
| "$ref": "#/definitions/detector/rule", |
There was a problem hiding this comment.
I kind of like the name detector. Consider chaining detectionTool to detector.
3 participants
A bit of a strawman so we can have a discussion about it. Also took @gilday's suggestion to nest the rule in a separate object.