Test: Self-analysis with intentional dead code #2
Conversation
WalkthroughAdds a dead-code test fixture, tightens API-key parsing/validation and expands runtime debug and API error-response logging; switches graph-generation call to Supermodel, updates README wording, changes CI job permissions, and tweaks PR comment wording. No public API signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🔇 Additional comments (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/example-dead-code.ts (1)
1-24: Nice test fixture! Consider adding more patterns to make it comprehensive.This file does exactly what it says on the tin - provides intentional dead code for testing. The four different patterns you've included (simple function, array formatter, async function, and object method) are solid starting points.
If you want to level up this test fixture, here are some additional patterns that might be worth throwing in:
Exported but unused - to verify the tool distinguishes between "exported" and "actually used elsewhere"
export function exportedButUnused() { return 42; }Class with unused methods - another common pattern
class UnusedClass { unusedMethod() { return 'hello'; } }Mixed scenario - some used code alongside unused code, to test that the tool doesn't flag false positives
function usedHelper() { return 1; } function unusedHelper() { return 2; } usedHelper(); // This one is calledThese would make your dogfooding test more thorough, but what you have now absolutely works for a first pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/index.ts:
- Around line 44-49: The current logging exposes a substring of the secret API
key via core.info; update the code that references apiKey so it never logs any
part of the key (remove apiKey.substring usage). Keep a non-secret diagnostic
such as key length or a boolean result of apiKey.startsWith('smsk_') via
core.info/core.warning, or replace the info call with something like
core.info(`API key configured (${apiKey.length} chars)`) or only log
presence/validation results; ensure no code paths log apiKey or any substring of
it.
🧹 Nitpick comments (1)
src/index.ts (1)
127-127: Preferunknownoveranyfor better type safety.Using
error: anydisables type checking entirely. The modern TypeScript pattern is to useerror: unknown, which forces you to narrow the type before using it (you're already doing this with theerror instanceof Errorcheck later).This is a small change but helps catch bugs at compile time. For example, if you accidentally tried to access
error.somePropertywithout checking the type first, TypeScript would catch it withunknownbut not withany.♻️ Proposed refactor
- } catch (error: any) { + } catch (error: unknown) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dist/example-dead-code.d.tsis excluded by!**/dist/**dist/example-dead-code.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T23:52:24.392Z
Learnt from: jonathanpopham
Repo: Arch-Gen-App/Supermodel PR: 520
File: api-server/node/src/services/ai-file-domain-suggester.service.ts:135-141
Timestamp: 2025-09-29T23:52:24.392Z
Learning: In api-server/node/src/services/ai-file-domain-suggester.service.ts, the team prefers to enforce data validation via structured output schema constraints rather than post-processing cleanup steps. The existing JSON schema with strict: true should handle type enforcement, and any additional validation should be added to the schema itself (e.g., minLength, pattern) rather than manual type-checking after LLM response.
Applied to files:
src/index.ts
🔇 Additional comments (1)
src/index.ts (1)
127-137: Confirm the SDK's error response structure by checking the actual library.The code assumes
error.responseexists with a.text()method that returns a Promise and a.statusproperty. While this matches the standard fetch Response API pattern used by most modern HTTP clients, the@supermodeltools/sdkpackage isn't publicly available on npm, so we can't verify this from public docs.To fix this, either:
- Check the SDK's source or type definitions (if you have access) to confirm the error shape
- Look at any examples in the SDK's internal documentation
- Test the error path locally with a bad API key to see what actually gets thrown
The nested try-catch inside your catch block is defensive and reasonable, but ideally you'd know exactly what you're catching before relying on it in production.
src/index.ts
Outdated
|
|
||
| // Validate API key format | ||
| if (!apiKey.startsWith('smsk_')) { | ||
| core.warning('API key does not start with expected prefix "smsk_"'); | ||
| } | ||
| core.info(`API key configured (${apiKey.length} chars, starts with: ${apiKey.substring(0, 10)}...)`); |
There was a problem hiding this comment.
Remove API key prefix from logs (security risk).
Logging the first 10 characters of the API key exposes a partial secret in GitHub Actions logs, which can be publicly visible (especially in public repos) and may get exported to external logging systems.
Even partial key exposure is a security risk. For example, if someone knows the first 10 chars are smsk_abc12 they've reduced the search space for brute-forcing the rest of the key.
🔒 Proposed fix to remove key prefix logging
// Validate API key format
if (!apiKey.startsWith('smsk_')) {
core.warning('API key does not start with expected prefix "smsk_"');
}
-core.info(`API key configured (${apiKey.length} chars, starts with: ${apiKey.substring(0, 10)}...)`);
+core.info(`API key configured (${apiKey.length} characters)`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate API key format | |
| if (!apiKey.startsWith('smsk_')) { | |
| core.warning('API key does not start with expected prefix "smsk_"'); | |
| } | |
| core.info(`API key configured (${apiKey.length} chars, starts with: ${apiKey.substring(0, 10)}...)`); | |
| // Validate API key format | |
| if (!apiKey.startsWith('smsk_')) { | |
| core.warning('API key does not start with expected prefix "smsk_"'); | |
| } | |
| core.info(`API key configured (${apiKey.length} characters)`); |
🤖 Prompt for AI Agents
In @src/index.ts around lines 44 - 49, The current logging exposes a substring
of the secret API key via core.info; update the code that references apiKey so
it never logs any part of the key (remove apiKey.substring usage). Keep a
non-secret diagnostic such as key length or a boolean result of
apiKey.startsWith('smsk_') via core.info/core.warning, or replace the info call
with something like core.info(`API key configured (${apiKey.length} chars)`) or
only log presence/validation results; ensure no code paths log apiKey or any
substring of it.
Dead Code HunterFound 2 potentially unused functions:
Powered by Supermodel call graph analysis |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/index.ts:
- Around line 43-50: The code is exposing parts of the secret by logging
substrings of apiKey; update the core.info call that currently prints
apiKey.substring(...) so it only logs non-secret diagnostics (e.g., the
apiKey.length and whether it starts with 'smsk_' or validation result) and
remove any printing of prefix/suffix characters; keep the
core.getInput('supermodel-api-key') trim and the prefix check using
apiKey.startsWith('smsk_') but ensure no substring of apiKey is ever included in
logs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
dist/index.js (1)
core(32461-32461)
🔇 Additional comments (1)
src/index.ts (1)
128-139: Nice enhancement for debugging API failures.This error handling improvement is solid. Here's what you're doing right:
- Type annotation (
error: any) - Good call, since catch blocks can catch literally anything (not just Error objects)- Checking
error.responseexists - Smart, because not all errors come from API calls- Nested try-catch - Nice defensive coding! If reading
response.text()fails for some reason, you still log the status- Falls through - Preserves the existing error handling for regular Error instances
Concrete example: If the API returns a 500 error with a message like
{"error": "Database connection failed"}, you'll now see both the status code (500) and that JSON body in your logs, which makes debugging way easier than just seeing "An error occurred."
src/index.ts
Outdated
| const apiKey = core.getInput('supermodel-api-key', { required: true }).trim(); | ||
|
|
||
| // Validate API key format | ||
| if (!apiKey.startsWith('smsk_')) { | ||
| core.warning('API key does not start with expected prefix "smsk_"'); | ||
| } | ||
| // Log key details for debugging (safe - only shows prefix/suffix) | ||
| core.info(`API key configured (${apiKey.length} chars, starts: ${apiKey.substring(0, 12)}..., ends: ...${apiKey.substring(apiKey.length - 4)})`); |
There was a problem hiding this comment.
Still logging parts of the secret API key (now even more of it) - security risk.
Hey, so I see you're trimming the key now (nice!) and validating the prefix (also nice!), but there's still a security problem on line 50. A previous review flagged logging the first 10 characters of the API key as a security risk. Now the code logs the first 12 characters and the last 4 characters - that's 16 total characters of the secret exposed in logs.
Here's why this matters:
- GitHub Actions logs can be publicly visible (especially in public repos)
- If someone knows
smsk_abc1234(12 chars) andxyz9(last 4), they've learned 16 characters of your secret - This dramatically reduces the brute-force search space
- Logs might get exported to external systems, persisted, etc.
Concrete example: If your API key is smsk_abc123456789_secret_xyz9 and you log smsk_abc1234 and xyz9, an attacker now only needs to guess the middle part.
The safe approach: just log the length or a boolean validation result, but never log any part of the actual key value.
🔒 Proposed fix to remove key substring logging
// Validate API key format
if (!apiKey.startsWith('smsk_')) {
core.warning('API key does not start with expected prefix "smsk_"');
}
-// Log key details for debugging (safe - only shows prefix/suffix)
-core.info(`API key configured (${apiKey.length} chars, starts: ${apiKey.substring(0, 12)}..., ends: ...${apiKey.substring(apiKey.length - 4)})`);
+core.info(`API key configured (${apiKey.length} characters, valid prefix: ${apiKey.startsWith('smsk_')})`);This way you still get useful debug info (how long is the key? does it have the right prefix?) without exposing any part of the secret itself.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const apiKey = core.getInput('supermodel-api-key', { required: true }).trim(); | |
| // Validate API key format | |
| if (!apiKey.startsWith('smsk_')) { | |
| core.warning('API key does not start with expected prefix "smsk_"'); | |
| } | |
| // Log key details for debugging (safe - only shows prefix/suffix) | |
| core.info(`API key configured (${apiKey.length} chars, starts: ${apiKey.substring(0, 12)}..., ends: ...${apiKey.substring(apiKey.length - 4)})`); | |
| const apiKey = core.getInput('supermodel-api-key', { required: true }).trim(); | |
| // Validate API key format | |
| if (!apiKey.startsWith('smsk_')) { | |
| core.warning('API key does not start with expected prefix "smsk_"'); | |
| } | |
| core.info(`API key configured (${apiKey.length} characters, valid prefix: ${apiKey.startsWith('smsk_')})`); |
🤖 Prompt for AI Agents
In @src/index.ts around lines 43 - 50, The code is exposing parts of the secret
by logging substrings of apiKey; update the core.info call that currently prints
apiKey.substring(...) so it only logs non-secret diagnostics (e.g., the
apiKey.length and whether it starts with 'smsk_' or validation result) and
remove any printing of prefix/suffix characters; keep the
core.getInput('supermodel-api-key') trim and the prefix check using
apiKey.startsWith('smsk_') but ensure no substring of apiKey is ever included in
logs.
Dead Code HunterFound 2 potentially unused functions:
Powered by Supermodel call graph analysis |
Dead Code HunterFound 5 potentially unused functions:
Powered by Supermodel call graph analysis |
Dead Code HunterFound 5 potentially unused functions:
Powered by Supermodel call graph analysis |
Dead Code HunterFound 5 potentially unused functions:
Powered by Supermodel graph analysis |
- Remove debug logging and test dead code file - Clean up action output messages - Point users to dashboard.supermodeltools.com for API keys - Improve error messages for invalid API keys
Dead Code HunterFound 2 potentially unused functions:
Powered by Supermodel graph analysis |
Summary
This PR adds intentional dead code to test the dead-code-hunter action on itself.
Added functions that should be detected:
unusedHelperFunctionformatUnusedDatafetchUnusedDataunusedProcessor.processExpected Result
The dead-code-hunter action should:
src/example-dead-code.tsThis is a dogfooding test - the action analyzing its own codebase.
Summary by CodeRabbit
Tests
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.