Skip to content

chore(instrumentation): add persona analytics extension#6927

Open
octavian-snyk wants to merge 1 commit into
mainfrom
chore/CLI-1604
Open

chore(instrumentation): add persona analytics extension#6927
octavian-snyk wants to merge 1 commit into
mainfrom
chore/CLI-1604

Conversation

@octavian-snyk

@octavian-snyk octavian-snyk commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Extends CLI instrumentation with a dedicated persona package that reports how the CLI is invoked and, when detectable, which AI coding agent is driving the run.

Changes:

New cliv2/internal/persona package — centralises persona analytics reporting via persona.Report().
Richer interactive telemetry — moves TTY detection out of cliv2/internal/utils and adds a per-stream bitmask:
persona.interactive (bool) — unchanged: true when stdin is a TTY
persona.interactive_mode (int) — new: bitmask of stdin (1), stdout (2), and stderr (4) TTY attachment

AI agent detection — new optional analytics field persona.agent when an agent is recognised from environment/filesystem signals.

Wiring — cliv2/pkg/core/main.go now calls persona.Report(cliAnalytics) instead of inline AddExtensionBoolValue("persona.interactive", ...).

Removed: cliv2/internal/utils/interactive.go and its tests (logic moved into persona).

Where should the reviewer start?

How should this be manually tested?

Run the built binary with -d using Cursor, Gemini, Claude Code, and other agents you have available, and verify the persona.agent output.

What's the product update that needs to be communicated to CLI users?

N/A

What are the relevant tickets?

CLI-1604

Risk assessment (Low | Medium | High)?

Low

@octavian-snyk octavian-snyk requested a review from a team as a code owner June 22, 2026 12:26
@snyk-io

snyk-io Bot commented Jun 22, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

This comment has been minimized.


// getInteractiveMode builds an InteractiveMode bitmask using check to test
// each standard stream.
func getInteractiveMode(check func(*os.File) bool) InteractiveMode {

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.

Question: The PR description mentions this code is unchanged but the file seems different.

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.

The persona.interactive key remains the same (true only if stdin is a TTY) here. The function simply generates a bitmask for all streams and records it in persona.interactive_mode.

I'm happy to adjust this based on your preference. I could:

  • Keep it as is
  • Rename it to be clearer
  • Drop it completely
  • Add an IsInteractive function (taking InteractiveMode and returning InteractiveMode & StdinTTY) rather than leaving it inline in persona.go.

Please let me know what you think is best 😅

@snyk-pr-review-bot

This comment has been minimized.

@robertolopezlopez robertolopezlopez Jun 23, 2026

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.

Why are agent* and interactive* in the package persona? Without looking down below, I have the feeling that agent, interactive and persona have their own (complex?) internal logic.

Wouldn't it be cleaner to have them in separate packages and offer just the necessary resources, to isolate such domain specific logic? (i.e. avoiding tight coupling between them)

Comment thread cliv2/internal/persona/agent.go
Comment thread cliv2/internal/persona/agent.go
Comment on lines +13 to +17
const (
StdinTTY InteractiveMode = 1 << iota // stdin is attached to a terminal
StdoutTTY // stdout is attached to a terminal
StderrTTY // stderr is attached to a terminal
)

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 am not sure if I follow what is happening here, how is it that those constants (even those not initialised) are attached to a terminal? - do you mind to explain? :-)

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.

🤣 i will change the comments

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.

thanks


func report(a Analytics, mode InteractiveMode, detect func() (Agent, bool)) {
interactive := mode&StdinTTY != 0
if a.IsCiEnvironment() {

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.

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.

please add a comment to document it in code

@snyk-pr-review-bot

This comment has been minimized.

@@ -0,0 +1,37 @@
package persona

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.

nit: probably this file should be named analytics.go, or something different. I cannot see any persona type in the whole PR. Not blocker

Comment thread cliv2/internal/persona/persona.go Outdated
}

func report(a Analytics, mode InteractiveMode, detect func() (Agent, bool)) {
interactive := mode&StdinTTY != 0

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.

this line is scary - do we have any other way to check this apart from bit operations? For sake of simplicity

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.

please document the bitmask

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.

I added a Has method, and I documented the bitmask on InteractiveMode definition and on keyInteractiveMode definition. Please let me know if you'd like it be documented somewhere else additionally.

}

// Report adds the persona extension values to the given analytics sink.
func Report(a Analytics) {

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 would prefer to use a receiver here and in report() - you are even modifying the a (look in lines 32 to 35)

@octavian-snyk octavian-snyk Jun 24, 2026

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.

That would be nice, but main.go will probably end up looking the same
To clarify the approach, I set up Analytics as a local interface to capture a subset of GAF interface methods for testing.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Unsanitized User Input 🟡 [minor]

The detectAgent function reads the AI_AGENT environment variable and, if present, passes it through canonicalAgent which returns the raw string value to the analytics backend via AddExtensionStringValue. This allows arbitrary, potentially very large, or malicious strings to be injected into the persona.agent analytics field. Given this is for 'canonical' names, consider validating the input against a known list or enforcing a maximum string length.

	if name := strings.TrimSpace(l.getenv("AI_AGENT")); name != "" {
		return canonicalAgent(name), true
	}

	for _, s := range signatures {
		if s.match(l) {
			return s.agent, true
		}
	}

	return "", false
}

// canonicalAgent normalises an explicitly declared AI_AGENT value onto the set
// of canonical agent names.
func canonicalAgent(name string) Agent {
	switch Agent(name) {
	case "github-copilot-cli":
		return AgentGitHubCopilot
	default:
		return Agent(name)
	}
📚 Repository Context Analyzed

This review considered 16 relevant code sections from 10 files (average relevance: 1.00)

🤖 Repository instructions applied (from AGENTS.md)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants