DEVXT-1965 - #37
Conversation
…hub verified email to try to capture emails for those that do not have saml id
There was a problem hiding this comment.
Pull request overview
Adds a new Node.js utility (utils/github/ent-inactive-users) that identifies enterprise members who have been inactive for a configurable period by checking org audit-log activity first, then falling back to recent commit activity, and outputs a CSV report.
Changes:
- Implements enterprise member retrieval (with outside-collaborator filtering) and per-member activity checks (audit log + commits).
- Adds a CLI entrypoint with flags for enterprise/org scope and inactivity window.
- Adds unit tests and documentation for the new utility.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/github/ent-inactive-users/src/index.js | CLI entrypoint and argument validation for enterprise/org/days. |
| utils/github/ent-inactive-users/src/octokit.js | Octokit client creation with throttling + logging hooks. |
| utils/github/ent-inactive-users/src/logger.js | Pino logger setup (silent in tests). |
| utils/github/ent-inactive-users/src/config.js | Centralizes GitHub API version headers. |
| utils/github/ent-inactive-users/src/enterprise.js | Enterprise member discovery and outside-collaborator filtering + org extraction. |
| utils/github/ent-inactive-users/src/inactive-users.js | Inactivity cutoff calculation and audit/commit activity checks. |
| utils/github/ent-inactive-users/src/report.js | CSV report generation for inactive members. |
| utils/github/ent-inactive-users/test/enterprise.test.js | Unit tests for org extraction and outside-collaborator filtering. |
| utils/github/ent-inactive-users/test/inactive-users.test.js | Unit tests for cutoff date and activity-check behavior. |
| utils/github/ent-inactive-users/README.md | Usage docs, requirements, and output description. |
| utils/github/ent-inactive-users/package.json | Package metadata, scripts, dependencies, and Node engine requirement. |
| utils/github/ent-inactive-users/package-lock.json | Dependency lockfile for the new package. |
| utils/github/ent-inactive-users/.tool-versions | Pins Node.js tool version for the utility. |
| utils/github/ent-inactive-users/.gitignore | Ignores generated artifacts (CSV/logs/node_modules). |
Files not reviewed (1)
- utils/github/ent-inactive-users/package-lock.json: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- utils/github/ent-inactive-users/package-lock.json: Generated file
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
utils/github/ent-inactive-users/src/report.js:8
- The JSDoc says the report is written to stdout, but the implementation only writes a CSV file and logs messages. This comment should match the actual behavior to avoid misleading consumers.
/**
* Writes inactive member results to stdout and a CSV file.
* @param {Array<import('./enterprise.js').Member>} members
*/
utils/github/ent-inactive-users/src/octokit.js:41
onSecondaryRateLimitalways returnstrue, so the client will keep retrying indefinitely on persistent secondary rate limiting. This can cause the script to hang for a long time without a hard stop.
onSecondaryRateLimit: (retryAfter, options, octokit) => {
octokit.log.warn(
`SecondaryRateLimit detected for request ${options.method} ${options.url}. Retrying after ${retryAfter} seconds`
);
return true;
utils/github/ent-inactive-users/src/index.js:44
parseIntis called without an explicit radix. Passing the radix (10) avoids edge cases and matches common Node.js CLI parsing practices.
const inactiveDaysInt = inactiveDays ? parseInt(inactiveDays) : 90;
utils/github/ent-inactive-users/src/report.js:20
- CSV rows are built by joining fields with commas without escaping. If any field contains a comma, quote, or newline, the generated CSV will be malformed and may be parsed incorrectly.
const rows = ['userName,email,membership,githubVerifiedDomainEmails'];
for (const { userName, email, membership, githubVerifiedDomainEmails } of members) {
rows.push([userName, email, membership.join('|'), githubVerifiedDomainEmails.join('|')].join(','));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- utils/github/ent-inactive-users/package-lock.json: Generated file
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
utils/github/ent-inactive-users/src/index.js:69
- This module exports
mainbut also executes it unconditionally at import time (await main();). That makes the file unsafe to import (e.g., from tests or other tooling) because it will parse args / require env vars immediately. Consider guarding execution somain()runs only when this file is the entrypoint.
await main();
utils/github/ent-inactive-users/src/logger.js:19
- Logger output is configured to always write to a local
info.logfile viapino/filewhen not running tests. This can break execution in environments where the current working directory is not writable, and it also hides logs from stdout/stderr (which is typically what CLIs expect). Consider defaulting to stdout/stderr and making file logging optional/configurable.
}, isTest ? undefined : pino.transport({
target: 'pino/file',
options: { destination: 'info.log' },
}));
utils/github/ent-inactive-users/src/report.js:8
- The JSDoc says this function writes results to stdout, but the implementation only writes a CSV file (and logs via
logger). This is misleading for callers/readers.
This issue also appears on line 31 of the same file.
* Writes inactive member results to stdout and a CSV file.
utils/github/ent-inactive-users/src/report.js:38
writableStream.on("error")throws from an async event handler. This can surface as an unhandled exception (outside theawait pipeline(...)promise chain) and may crash the process instead of rejecting thewriteReportcall cleanly.stream/promises.pipelinealready propagates stream errors via its returned promise, so it’s safer to rely on that and handle logging in a try/catch aroundpipeline.
writableStream.on("finish", () => {
logger.info(`\nReport written to ${filename}`);
});
writableStream.on("error", (error) => {
logger.error("Error writing to file:", error.message);
throw error;
});
This PR adds code that reports on users that have not been active within an enterprise for a given amount of days.