Add support to detect Assisted-By trailer#33
Conversation
MoralCode
left a comment
There was a problem hiding this comment.
This change diff feels like it is rewriting lots of the other functions that are checking for signs of other AI related uses besides Assisted-By trailiers.
I like the additional unit tests, but If theres additional formatting/refactoring changes that need to be made, I'd like those to be explained in the PR description or discussed beforehand.
Is there anything preventing this change from being a handful of lines of new unit tests and a smaller handful of lines to add a new entry to the commitMessagePatterns list that satisfies the unit tests?
|
Thanks Adrian, I've added a Changes list to PR description above, which should hopefully answer all questions related to other function changes. Kindly go through it and let me know if you've any follow-up questions. |
|
Thanks for the detailed writeup. I agree with Adrian that the diff is larger than it needs to be. The root issue is that A few other things:
I'd suggest making |
|
Thanks for the detailed review, I'll update the PR accordingly. |
c707d90 to
e59e9dc
Compare
|
@andrew I've updated the PR as per your suggestions here. Please find all changes in description here. Thanks. |
|
i see this new version adds an entirely new type of detector for assisted by trailers. I dont see why this needs to be a whole new detector.
|
e59e9dc to
d90cf1e
Compare
MoralCode
left a comment
There was a problem hiding this comment.
This does not require a new detector, we already have a detector for commit messages
I see two main reasons why we should keep it in a separate detector:
Also this was suggested in @andrew's suggestions comment which I've fully incorporated:
|
|
Additionally I think So these are separate concerns and we should try to keep them separate to encourage the fact that newcomers can easily develop and maintain them separately. |
Signed-off-by: Omkar P <45419097+omkar-foss@users.noreply.github.com>
d90cf1e to
7fd2837
Compare
|
PR rebased with main, conflicts resolved, ready to review. Please have a look when possible, thanks. |
Signed-off-by: Omkar P <45419097+omkar-foss@users.noreply.github.com>
|
@MoralCode Hey, as per our discussion in #49, I've got the trailer-based detection logic together. Adding it to this PR since the discussion started here with Assisted-By addition. So now we've only 1 package PR is ready to review from my side, please have a look. Since this is a big change, I'll merge this only after approval from @andrew or you. Thanks. Issue for reference: #53 |
andrew
left a comment
There was a problem hiding this comment.
Thanks for reworking this. The consolidation into one package is fine and tests pass locally. A few things before merging:
detection/constants.go extracts constants that aren't shared. Every value in that file is used by exactly one detector: KnownAgentCommitters/GithubNoReplyEmailSuffix only by committer, SupportedToolsInMentions only by toolmention, GitNotesAuthorshipPrefix only by gitnotes, and the rest only by trailer. Nothing is reused across packages, so this is pure indirection: to change the Replit regex you now edit a different file from the code that runs it. I'd keep each constant next to its sole consumer and only lift things into detection when a second package actually needs them. That also drops the committer.go, gitnotes.go and toolmention.go diffs from this PR entirely.
extractToolsFromText rewrites the tool name instead of reporting it verbatim. Assisted-By: gpt-4o comes out as Gpt-4o, llama.cpp as Llama.cpp. The trailer is the disclosure, so we should report what the user wrote. Related:
Assisted-By: <noreply@anthropic.com>(email only, no name) yields zero findings, so an explicit disclosure is silently dropped.strings.Split(text, "\n")is dead code:AssistedByPatterncaptures[^\r\n]+, somatch[1]never contains a newline.
I'd reduce the function to: strip a trailing <…>, strip a trailing (…), TrimSpace, and if that leaves an empty string fall back to the raw trailer value so nothing is dropped.
No dedup on Assisted-By. Two Assisted-By: Claude lines produce two identical findings. detectTrailerCoauthoredBy already dedupes via a seen map; Assisted-By should do the same.
Duplicate subtest names in trailer_test.go. "Claude trailer with Opus model", "Cursor trailer", "multiple AI trailers", "case variation" etc. each appear twice. Go appends #01 so they still run, but you can't -run a specific one. Prefixing with coauthor: / assistedby: would fix it.
Smaller bits:
copilot@github.comwas added toKnownCoAuthorEmails. Fine, but worth its own commit or a mention in the description.detectCustomTrailershandles theaider:subject prefix andGenerated with Claude Codebody text, neither of which is a trailer. Something likedetectMessagePatternswould be more accurate.- In
Detect,var findings; findings = slices.Concat(...); return findingscan bereturn slices.Concat(...).
The constants extraction is the main one; the rest are small.
Signed-off-by: Omkar P <45419097+omkar-foss@users.noreply.github.com>
Signed-off-by: Omkar P <45419097+omkar-foss@users.noreply.github.com>
|
Hi @andrew thanks for your reviewing, I've updated the PR as per your comments, my comments below quoting yours:
It'll be great if we could retain this. Idea is to keep all the detection related constants together so we can incrementally replace them with more dynamic lists, see: #42
This is expected behaviour, I'm capitalizing the tool name for readability. Also
Done, added dedupe logic with corresponding test. Thanks.
Done, added the prefixes. Thanks.
Thanks, have added a line about KnownCoAuthorEmails to PR description above (see last point under changes). Also have renamed the function and returned findings accordingly. All set! |
andrew
left a comment
There was a problem hiding this comment.
Thanks for the updates. Dedup, test names, rename and the return simplification all look good.
On constants.go: #42 is really about the tool-name lists rather than parsing patterns like ReplitAttributionRegex or GitNotesAuthorshipPrefix, but I'm happy to leave it and revisit when #42 lands.
One factual note on the \n split, non-blocking: AssistedByPattern's capture group is [^\r\n]+?, so match[1] is always a single line. For the multiline test case the regex returns "Claude 4.7 Opus" and the indented (logic...) continuation is never passed to extractToolsFromText — the test passes because the continuation is ignored, not because the split handles it. Fine to leave as-is.
LGTM.
Closes #28 and #53.
This PR adds support for detecting one or more Assisted-By trailers in git commit message. Also includes tests for supported cases.
Changes in this PR:
KnownCoAuthorEmailsunder detection/constants.go.