Skip to content

[Refactoring] Create a new "AI" vertical#1195

Open
paulpetit-gg-ext wants to merge 2 commits intomainfrom
ppetit/-/ai-vertical
Open

[Refactoring] Create a new "AI" vertical#1195
paulpetit-gg-ext wants to merge 2 commits intomainfrom
ppetit/-/ai-vertical

Conversation

@paulpetit-gg-ext
Copy link
Copy Markdown
Contributor

@paulpetit-gg-ext paulpetit-gg-ext commented Mar 26, 2026

Context

Because we will add new AI governance capabilities to GIM, this PR is a refactoring that extracts what has been done for AI hooks into a new ai vertical that will be expanded upon later (new commands will arrive in the ai vertical, and hooks won't only serve to scan secrets).

It is quite a lot of change, but this is 99% moving code from a place to another and fixing imports.
I think it is worth having it in a distinct PR so that later work is more easily reviewed.

There is only one notable change that is not purely code shuffling: Flavor has been renamed to the less cryptic Agent and has become an abstract class. The rationale is to keep everything specific to an AI agent (Cursor, Claude Code, etc.) into a single class/file even when the number of AI governance features will grow.

Note that we still need at some point to be able to reuse both logic from the scan vertical (to scan secrets) and from the new ai vertical (for hooks, payload model, etc.).
Instead of adding coupling between verticals (ai -> scan or scan -> ai), this PR chose to have commands import what they need from each vertical in order to work. This is what I think maintains the clearer boundary between modules.

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@paulpetit-gg-ext paulpetit-gg-ext changed the title feat(ai_hook): refactor into new "ai" vertical [Refactoring] Create a new "AI" vertical Mar 26, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 98.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.85%. Comparing base (d3b9004) to head (17f9363).

Files with missing lines Patch % Lines
ggshield/verticals/ai/hooks.py 97.18% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   92.85%   92.85%   -0.01%     
==========================================
  Files         168      170       +2     
  Lines        8178     8198      +20     
==========================================
+ Hits         7594     7612      +18     
- Misses        584      586       +2     
Flag Coverage Δ
unittests 92.85% <98.75%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulpetit-gg-ext paulpetit-gg-ext force-pushed the ppetit/-/ai-vertical branch 2 times, most recently from 0d5cfd1 to 66bc455 Compare March 27, 2026 06:54
@paulpetit-gg-ext paulpetit-gg-ext marked this pull request as ready for review March 27, 2026 07:01
@paulpetit-gg-ext paulpetit-gg-ext requested a review from a team as a code owner March 27, 2026 07:01
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coupling between the ai and secret vertical doesn't feel right to me. I wonder if we rather should move ggshield/verticals/secret/secret_scan_collection.py and ggshield/verticals/secret/secret_scanner.py into ggshield/core/scan and this file into the ai vertical. That way we could ensure a clean separation between verticals.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking to Aurélien about this and he had some interesting insights: The verticals represent GitGuardian business verticals (although auth is a pretty wide stretch for a vertical 😅). Therefore, the AI hooks' logic should reside in the secret vertical.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I suppose leaving the hook part in secret/ makes sense, but there are still a few questions:

  • Adding an ggshield ai subcommand is still relevant for us (for instance we are developing a ggshield ai discover command that lists agents and MCP servers configured on a developer's machine and which wouldn't make much sense under ggshield secret). Does it mean we can create the vertical or should we have a new command subgroup but not a new vertical/ subfolder?
  • In case we have part of the new AI governance code under a specific folder, it means two things for the hook code:
    • either it imports model from it (coupling)
    • either everything useful for hooks stays under secret/ but we may have code duplication, and the logic specific to a given agent like Cursor is split between multiple folders.

I think in this case some form of coupling may be warranted. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @agateau-gg , I pushed a new solution: both verticals are now independent, and the commands are able (with documented exceptions in the import linter) to pull the logic they need from both verticals (scan and ai).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

Copy link
Copy Markdown
Contributor

@6d7a 6d7a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants