CLI-244 CLI-245 callback infrastructure#157
CLI-244 CLI-245 callback infrastructure#157kirill-knize-sonarsource wants to merge 2 commits intomasterfrom
Conversation
SummaryThis PR implements callback infrastructure for the Claude Code integration, specifically adding a PreToolUse secrets scanner that blocks file reads if hardcoded secrets are detected. The main implementation (CLI-245) adds a
Infrastructure additions (CLI-244) include:
No changes to public-facing commands or existing behavior—purely additive infrastructure for AI agent integration. What reviewers should knowWhere to start:
Key design decisions:
Non-obvious details:
Watch for:
|
da371c5 to
91be621
Compare
91be621 to
e5bc9f8
Compare
e5bc9f8 to
6f2f053
Compare
6f2f053 to
aa6bc64
Compare
b2c895c to
b8e4288
Compare
aa6bc64 to
3a31a77
Compare
3a31a77 to
c4b4665
Compare
b8e4288 to
26b6e97
Compare
e44c9d3 to
d117a3a
Compare
26b6e97 to
74d9271
Compare
sophio-japharidze-sonarsource
left a comment
There was a problem hiding this comment.
I love the direction this is going 😍
I left a few comments & suggestions, mainly about duplication of secrets scanning logic. I will test it now.
| const auth = await resolveAuth().catch(() => null); | ||
| if (!auth) return; // not authenticated — allow gracefully | ||
|
|
||
| const binaryPath = resolveSecretsBinaryPath(); | ||
| if (!binaryPath) return; // binary not installed — allow gracefully |
There was a problem hiding this comment.
Has this been agreed with PM & UX? I agree it's good not to fail the Claude process completely, but at the same time somehow we need to let the users know that their integration is broken, so that they don't have a false sense of security.
There was a problem hiding this comment.
Maybe it's possible to put something in Claude status bar at this point?
74d9271 to
8e2ad74
Compare
d117a3a to
4b1d963
Compare
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| return await Promise.race([ | ||
| new Promise<string>((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| process.stdin.on('data', (chunk: Buffer) => chunks.push(chunk)); | ||
| process.stdin.on('end', () => { | ||
| resolve(Buffer.concat(chunks).toString('utf-8')); | ||
| }); | ||
| process.stdin.on('error', reject); | ||
| }), | ||
| new Promise<never>((_, reject) => { | ||
| timeoutId = setTimeout(() => { | ||
| reject(new Error(`stdin read timed out after ${STDIN_TIMEOUT_MS}ms`)); | ||
| }, STDIN_TIMEOUT_MS); | ||
| }), | ||
| ]); | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } |
There was a problem hiding this comment.
Logic duplication: The Promise.race + setTimeout + finally clearTimeout pattern here is identical in structure to scanFiles in secrets-scan.ts (lines 57–80). Both do:
- Race an operation against a timeout
- Optionally trigger a cleanup (kill process / nothing for stdin)
- Clear the timeout in
finally
With more handlers coming, this will be copy-pasted again. Extract a shared utility, e.g.:
export async function withTimeout<T>(
operation: Promise<T>,
ms: number,
onTimeout?: () => void,
): Promise<T> {
let timeoutId: ReturnType<typeof setTimeout> | undefined;
try {
return await Promise.race([
operation,
new Promise<never>((_, reject) => {
timeoutId = setTimeout(() => {
onTimeout?.();
reject(new Error(`Operation timed out after ${ms}ms`));
}, ms);
}),
]);
} finally {
clearTimeout(timeoutId);
}
}Then readRawStdin and scanFiles each become a single withTimeout(...) call.
- Mark as noise
4b1d963 to
fc175e7
Compare
d3d8cdc to
b83cb4f
Compare
There was a problem hiding this comment.
Conclusion: Solid refactor that cleanly moves hook logic from brittle shell scripts into testable TypeScript. One bug needs fixing before merge — the README was regenerated before this commit's renames took effect, so it documents commands that don't exist or have wrong names.
|
|
||
| #### `sonar hook codex-pre-tool-use` | ||
|
|
||
| PreToolUse handler for Codex: scan files for secrets before agent reads them |
There was a problem hiding this comment.
The README was regenerated before this commit's renames were applied, so three entries here are stale:
sonar hook codex-pre-tool-use(this line) — removed fromcommand-tree.tsin this commit, but still documented heresonar hook agent-prompt-submit(~line 413) — renamed toclaude-prompt-submit; old name is registered nowheresonar hook agent-post-tool-use(~line 419) — renamed toclaude-post-tool-use; same issue
Run gen:docs again after the renames and re-commit before merge.
- Mark as noise
b83cb4f to
8617798
Compare
src/cli/command-tree.ts
Outdated
| // Hidden callback command — internal handlers for agent and git hooks. | ||
| // Shell hook scripts call `sonar hook <event>` to delegate all business logic to TypeScript. | ||
| export const hookCommand = COMMAND_TREE.command('hook', { hidden: true }) | ||
| .description('Internal callback handlers for agent and git hooks') |
There was a problem hiding this comment.
| .description('Internal callback handlers for agent and git hooks') | |
| .description('Internal hook handlers for agent and git hooks') |
e04ce87 to
a0b217b
Compare
a0b217b to
f4cbc21
Compare
f4cbc21 to
468e45c
Compare
8e2ad74 to
b9df45c
Compare
468e45c to
38f15b5
Compare
00173a3 to
9191141
Compare
PR feedback: rename symbols, remove codex stub, fix hook naming, update tests - Rename scanFiles → scanFilesForSecrets in secrets-scan.ts - Rename callbackCommand → hookCommand in command-tree.ts - Remove codex-pre-tool-use command (out of scope) - Remove PR-specific comments from command-tree.ts - Rename agent-prompt-submit → claude-prompt-submit - Rename agent-post-tool-use → claude-post-tool-use - Consolidate duplicate hook.test.ts tests, assert description shown - Update README.md via gen:docs Improve coverage: unit tests for readStdinJson / readRawStdin
9191141 to
129cc50
Compare
|



No description provided.