hardcodedfilepath: detect full fmt directives (incl. %x) and ignore escaped %% #42178
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,21 @@ func okFormatVerb() string { | |
| return fmt.Sprintf("/tmp/gh-aw/runs/%s/output.json", "run-id") | ||
| } | ||
|
|
||
| // ok: path template literal with %x should be treated as a format template. | ||
|
Contributor
There was a problem hiding this comment. [/tdd] Good coverage for 💡 Suggested additional fixtures// ok: path with flags+width format directive should be treated as a template.
func okFlagWidthTemplateLiteral() string {
return "/tmp/gh-aw/%-10s/output.log"
}
// ok: path with indexed argument should be treated as a template.
func okIndexedArgTemplateLiteral() string {
return "/tmp/gh-aw/%[1]s/output.log"
}
// ok: path with precision format should be treated as a template.
func okPrecisionTemplateLiteral() string {
return "/tmp/gh-aw/%8.2f/output.log"
}These mirror real-world format strings and lock in the regex behaviour for the full directive grammar. @copilot please address this. |
||
| func okHexTemplateLiteral() string { | ||
| return "/tmp/gh-aw/%x.tmp" | ||
|
Contributor
There was a problem hiding this comment. Exclusion is now broader than verb-in-path actually implies: the old allow-list ( 💡 Concern and tradeoffA path like This is an inherent limitation of any string-literal heuristic, but widening the set increases the false-negative surface. Consider documenting this tradeoff in the function comment — the existing comment says "Strings with directives are format templates" but that is only true by assumption, not by proof from AST context. Suggestion: Acknowledge in the comment that the filter is a heuristic and over-exclusion is a known tradeoff, so reviewers understand the guarantee is approximate: // hasFormatVerb reports whether val contains fmt-style format directives.
// This is a heuristic: any string matching a directive pattern is treated
// as a format template and excluded, which may produce false negatives
// when a literal % sequence appears in an actual path (not a format arg). |
||
| } | ||
|
|
||
|
Comment on lines
+73
to
+77
|
||
| // bad: escaped %% is not a format verb and should still be reported. | ||
| func badEscapedPercentPath() string { | ||
| return "/tmp/gh-aw/100%%-done.log" // want `hard-coded file path.*consider extracting` | ||
| } | ||
|
|
||
| // ok: path template with indexed width/precision/value directive (e.g. %[3]*.[2]*[1]x). | ||
| func okIndexedArgDirective() string { | ||
| return "/tmp/gh-aw/%[3]*.[2]*[1]x" | ||
| } | ||
|
|
||
| // ok: very short path segment (no trailing slash after prefix). | ||
| func okShortSegment() string { | ||
| return ".github" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] Naming inconsistency: the companion variable is
fmtDirectivePatternand the PR description/comments now use "directives" throughout, but the function is still calledhasFormatVerb. This mixed vocabulary makes it harder to grep and reason about the code.💡 Suggested rename
Rename to align with the updated terminology:
Update all call sites accordingly.
@copilot please address this.