fix: handle undefined author in plugins and remove deprecated Projects API#1777
fix: handle undefined author in plugins and remove deprecated Projects API#1777artik0din wants to merge 4 commits intolowlighter:masterfrom
Conversation
GitHub deprecated Projects (classic) in May 2024. This removes the projects field from the GraphQL query and disables the Manager achievement that depended on it. See: https://github.blog/changelog/2024-05-23-sunset-notice-projects-classic/
Some commits from PushEvents may not have an author property set. This adds a null check to prevent TypeError when destructuring.
The filter on line 140 destructured author.email directly, causing a TypeError when author is undefined. This adds a null check before accessing the email property.
github-actions
bot
added
🧩 plugins
🏆 plugin achievements
📰 plugin activity
💡 plugin habits
labels
- Add Dockerfile.web for web server mode - Add heroku.yml for container deployment - Add Procfile as fallback - Support METRICS_TOKEN and PORT environment variables
Changes SummaryThis PR fixes three critical plugin bugs that cause crashes: removes deprecated GitHub Projects API usage from achievements plugin, adds null checks for commits missing author metadata in habits and activity plugins, and adds Docker/Heroku deployment configuration with environment variable support. Type: bugfix Components Affected: achievements plugin, habits plugin, activity plugin, web deployment/configuration Files Changed
Architecture Impact
Risk Areas: Silent filtering of commits without author in habits and activity plugins - may mask data quality issues, Disabled Manager achievement represents lost functionality for users who created projects, Docker image includes multiple language runtimes (Node, Deno, Ruby, Python) increasing attack surface, Environment variable configuration (METRICS_TOKEN) requires secure secret management in Heroku/Docker deployment Suggestions
Full review in progress... | Powered by diffray |
|
|
||
| if (!conf.settings.templates) | ||
| conf.settings.templates = {default: "classic", enabled: []} | ||
| if (!conf.settings.plugins) |
There was a problem hiding this comment.
🔴 CRITICAL - JSON.parse without try-catch for package.json
Agent: bugs
Category: bug
Description:
JSON.parse() can throw a SyntaxError if package.json contains invalid JSON, but this operation is not wrapped in a try-catch block. The previous try-catch block ended at line 64, leaving this JSON.parse unprotected.
Suggestion:
Wrap the JSON.parse operation in a try-catch block, similar to the pattern used for settings.json in lines 46-64.
Why this matters: Unhandled errors crash the application or hide bugs.
Confidence: 92%
Rule: bug_missing_try_catch
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| # Run web server | ||
| CMD ["npm", "start"] |
There was a problem hiding this comment.
🟠 HIGH - Missing HEALTHCHECK in Docker container
Agent: bugs
Category: bug
Description:
The Dockerfile does not include a HEALTHCHECK instruction. Without it, Docker orchestrators cannot automatically detect if the application is unhealthy.
Suggestion:
Add a HEALTHCHECK instruction to monitor the application health, e.g., HEALTHCHECK --interval=30s --timeout=5s CMD node -e "require('http').get('http://localhost:3000', (r) => {if (r.statusCode !== 200) throw new Error()})"
Why this matters: Missing healthcheck prevents automatic recovery from application failures.
Confidence: 85%
Rule: docker_missing_healthcheck
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| FROM node:20-bookworm-slim | ||
|
|
||
| # Copy repository | ||
| COPY . /metrics |
There was a problem hiding this comment.
🟠 HIGH - Docker cache invalidation: COPY all files before dependency install
Agent: microservices
Category: quality
Description:
COPY . /metrics copies all source files before dependency installation, which will invalidate the Docker layer cache on every code change, triggering rebuilding of expensive layers like apt packages and npm dependencies.
Suggestion:
First copy only dependency manifest files (package.json, package-lock.json) before RUN npm ci, then copy the remaining source files after installation.
Why this matters: Any file change invalidates cached npm install layer.
Confidence: 95%
Rule: docker_copy_before_install
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| # Base image | ||
| FROM node:20-bookworm-slim | ||
|
|
||
| # Copy repository | ||
| COPY . /metrics | ||
| WORKDIR /metrics | ||
|
|
||
| # Setup | ||
| RUN chmod +x /metrics/source/app/action/index.mjs \ | ||
| # Install latest chrome dev package, fonts to support major charsets and skip chromium download on puppeteer install | ||
| # Based on https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#running-puppeteer-in-docker | ||
| && apt-get update \ | ||
| && apt-get install -y wget gnupg ca-certificates libgconf-2-4 \ | ||
| && wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - \ | ||
| && sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' \ | ||
| && apt-get update \ | ||
| && apt-get install -y google-chrome-stable fonts-ipafont-gothic fonts-wqy-zenhei fonts-thai-tlwg fonts-kacst fonts-freefont-ttf libxss1 libx11-xcb1 libxtst6 lsb-release --no-install-recommends \ | ||
| # Install deno for miscellaneous scripts | ||
| && apt-get install -y curl unzip \ | ||
| && curl -fsSL https://deno.land/x/install/install.sh | DENO_INSTALL=/usr/local sh \ | ||
| # Install ruby to support github licensed gem | ||
| && apt-get install -y ruby-full git g++ cmake pkg-config libssl-dev \ | ||
| && gem install licensed \ | ||
| # Install python for node-gyp | ||
| && apt-get install -y python3 \ | ||
| # Clean apt/lists | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| # Install node modules and rebuild indexes | ||
| && npm ci \ | ||
| && npm run build | ||
|
|
||
| # Environment variables | ||
| ENV PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true | ||
| ENV PUPPETEER_BROWSER_PATH="google-chrome-stable" | ||
| ENV NODE_ENV=production | ||
|
|
||
| # Run web server | ||
| CMD ["npm", "start"] |
There was a problem hiding this comment.
🟠 HIGH - Container runs as root user
Agent: security
Category: security
Description:
The Dockerfile does not specify a USER instruction. Docker containers without an explicit non-root user will run as root (UID 0), increasing blast radius if the container is compromised.
Suggestion:
Add a USER instruction to run the container as a non-root user. Create a dedicated user with RUN useradd -m -u 1000 appuser && USER appuser
Why this matters: Root in container may have elevated privileges on host depending on configuration.
Confidence: 95%
Rule: docker_running_as_root
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| # Base image | ||
| FROM node:20-bookworm-slim | ||
|
|
||
| # Copy repository | ||
| COPY . /metrics | ||
| WORKDIR /metrics | ||
|
|
||
| # Setup | ||
| RUN chmod +x /metrics/source/app/action/index.mjs \ | ||
| # Install latest chrome dev package, fonts to support major charsets and skip chromium download on puppeteer install | ||
| # Based on https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#running-puppeteer-in-docker | ||
| && apt-get update \ | ||
| && apt-get install -y wget gnupg ca-certificates libgconf-2-4 \ | ||
| && wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - \ | ||
| && sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' \ | ||
| && apt-get update \ | ||
| && apt-get install -y google-chrome-stable fonts-ipafont-gothic fonts-wqy-zenhei fonts-thai-tlwg fonts-kacst fonts-freefont-ttf libxss1 libx11-xcb1 libxtst6 lsb-release --no-install-recommends \ | ||
| # Install deno for miscellaneous scripts | ||
| && apt-get install -y curl unzip \ | ||
| && curl -fsSL https://deno.land/x/install/install.sh | DENO_INSTALL=/usr/local sh \ | ||
| # Install ruby to support github licensed gem | ||
| && apt-get install -y ruby-full git g++ cmake pkg-config libssl-dev \ | ||
| && gem install licensed \ | ||
| # Install python for node-gyp | ||
| && apt-get install -y python3 \ | ||
| # Clean apt/lists | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| # Install node modules and rebuild indexes | ||
| && npm ci \ | ||
| && npm run build | ||
|
|
||
| # Environment variables | ||
| ENV PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true | ||
| ENV PUPPETEER_BROWSER_PATH="google-chrome-stable" | ||
| ENV NODE_ENV=production | ||
|
|
||
| # Run web server | ||
| CMD ["npm", "start"] |
There was a problem hiding this comment.
🟠 HIGH - Single-stage Docker build leaves build tools in production
Agent: security
Category: security
Description:
The Dockerfile uses a single-stage build pattern. Build dependencies (npm, python3, ruby, g++, cmake, git, deno, curl, unzip) are included in the final production image, increasing attack surface and image size.
Suggestion:
Use multi-stage Docker build with 'AS' aliases. Separate build stage from runtime stage. Only copy necessary runtime artifacts from build stage using 'COPY --from=build'.
Why this matters: Security issues can lead to data breaches.
Confidence: 90%
Rule: docker_implement_multi_stage_builds_for_product
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| COPY . /metrics | ||
| WORKDIR /metrics | ||
|
|
||
| # Setup | ||
| RUN chmod +x /metrics/source/app/action/index.mjs \ | ||
| # Install latest chrome dev package, fonts to support major charsets and skip chromium download on puppeteer install | ||
| # Based on https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#running-puppeteer-in-docker | ||
| && apt-get update \ | ||
| && apt-get install -y wget gnupg ca-certificates libgconf-2-4 \ | ||
| && wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - \ | ||
| && sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' \ | ||
| && apt-get update \ | ||
| && apt-get install -y google-chrome-stable fonts-ipafont-gothic fonts-wqy-zenhei fonts-thai-tlwg fonts-kacst fonts-freefont-ttf libxss1 libx11-xcb1 libxtst6 lsb-release --no-install-recommends \ | ||
| # Install deno for miscellaneous scripts | ||
| && apt-get install -y curl unzip \ | ||
| && curl -fsSL https://deno.land/x/install/install.sh | DENO_INSTALL=/usr/local sh \ | ||
| # Install ruby to support github licensed gem | ||
| && apt-get install -y ruby-full git g++ cmake pkg-config libssl-dev \ | ||
| && gem install licensed \ | ||
| # Install python for node-gyp | ||
| && apt-get install -y python3 \ | ||
| # Clean apt/lists | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| # Install node modules and rebuild indexes | ||
| && npm ci \ | ||
| && npm run build |
There was a problem hiding this comment.
🟡 MEDIUM - Copying entire project directory into production image
Agent: security
Category: security
Description:
Line 5 copies the entire project directory (COPY . /metrics) into the production image. This includes test files, source code, documentation, and git files that are unnecessary at runtime.
Suggestion:
Use .dockerignore to exclude unnecessary files (tests, docs, source, .git, .env files). Consider using multi-stage build where only necessary runtime artifacts are copied.
Why this matters: Smaller images pull faster, have fewer vulnerabilities, and reduce secrets exposure.
Confidence: 80%
Rule: docker_remove_unnecessary_files_from_production
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| @@ -0,0 +1,38 @@ | |||
| # Base image | |||
| FROM node:20-bookworm-slim | |||
There was a problem hiding this comment.
🟡 MEDIUM - Node.js base image uses floating patch version
Agent: microservices
Category: quality
Description:
Node.js base image uses 'node:20-bookworm-slim' which is a floating tag that resolves to the latest patch version of Node 20. This can lead to non-deterministic builds.
Suggestion:
Use a specific patch version like 'node:20.11.1-bookworm-slim' instead of 'node:20-bookworm-slim' to ensure consistent, reproducible builds.
Why this matters: Improves code reliability.
Confidence: 75%
Rule: docker_pin_specific_patch_versions_for_node_js
Review ID: 8aeb9742-af0a-4aae-8c5e-a98c17691acc
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 16 issues: 12 kept (critical bugs, security issues, Docker best practices), 4 filtered (incorrect analysis, low value, speculative) Issues Found: 12💬 See 7 individual line comment(s) for details. 📊 9 unique issue type(s) across 12 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - JSON.parse without try-catch for package.json (2 occurrences)Agent: bugs Category: bug Why this matters: Unhandled errors crash the application or hide bugs. 📍 View all locations
Rule: 🟠 HIGH - Container runs as root userAgent: security Category: security Why this matters: Root in container may have elevated privileges on host depending on configuration. File: Description: The Dockerfile does not specify a USER instruction. Docker containers without an explicit non-root user will run as root (UID 0), increasing blast radius if the container is compromised. Suggestion: Add a USER instruction to run the container as a non-root user. Create a dedicated user with RUN useradd -m -u 1000 appuser && USER appuser Confidence: 95% Rule: 🟠 HIGH - Single-stage Docker build leaves build tools in productionAgent: security Category: security Why this matters: Security issues can lead to data breaches. File: Description: The Dockerfile uses a single-stage build pattern. Build dependencies (npm, python3, ruby, g++, cmake, git, deno, curl, unzip) are included in the final production image, increasing attack surface and image size. Suggestion: Use multi-stage Docker build with 'AS' aliases. Separate build stage from runtime stage. Only copy necessary runtime artifacts from build stage using 'COPY --from=build'. Confidence: 90% Rule: 🟠 HIGH - Docker cache invalidation: COPY all files before dependency installAgent: microservices Category: quality Why this matters: Any file change invalidates cached npm install layer. File: Description: COPY . /metrics copies all source files before dependency installation, which will invalidate the Docker layer cache on every code change, triggering rebuilding of expensive layers like apt packages and npm dependencies. Suggestion: First copy only dependency manifest files (package.json, package-lock.json) before RUN npm ci, then copy the remaining source files after installation. Confidence: 95% Rule: 🟠 HIGH - HTTP request to external service without timeout (2 occurrences)Agent: security Category: security Why this matters: Hanging requests exhaust connections and can cause cascading failures. 📍 View all locations
Rule: 🟠 HIGH - Missing HEALTHCHECK in Docker containerAgent: bugs Category: bug Why this matters: Missing healthcheck prevents automatic recovery from application failures. File: Description: The Dockerfile does not include a HEALTHCHECK instruction. Without it, Docker orchestrators cannot automatically detect if the application is unhealthy. Suggestion: Add a HEALTHCHECK instruction to monitor the application health, e.g., HEALTHCHECK --interval=30s --timeout=5s CMD node -e "require('http').get('http://localhost:3000', (r) => {if (r.statusCode !== 200) throw new Error()})" Confidence: 85% Rule: 🟠 HIGH - Unhandled promise rejection from external HTTP call (2 occurrences)Agent: security Category: security Why this matters: Unhandled rejections can crash Node.js servers and cause downtime. 📍 View all locations
Rule: 🟡 MEDIUM - Copying entire project directory into production imageAgent: security Category: security Why this matters: Smaller images pull faster, have fewer vulnerabilities, and reduce secrets exposure. File: Description: Line 5 copies the entire project directory (COPY . /metrics) into the production image. This includes test files, source code, documentation, and git files that are unnecessary at runtime. Suggestion: Use .dockerignore to exclude unnecessary files (tests, docs, source, .git, .env files). Consider using multi-stage build where only necessary runtime artifacts are copied. Confidence: 80% Rule: 🟡 MEDIUM - Node.js base image uses floating patch versionAgent: microservices Category: quality Why this matters: Improves code reliability. File: Description: Node.js base image uses 'node:20-bookworm-slim' which is a floating tag that resolves to the latest patch version of Node 20. This can lead to non-deterministic builds. Suggestion: Use a specific patch version like 'node:20.11.1-bookworm-slim' instead of 'node:20-bookworm-slim' to ensure consistent, reproducible builds. Confidence: 75% Rule: ℹ️ 5 issue(s) outside PR diff (click to expand)
🔴 CRITICAL - Async file operation not awaited, Promise passed to yaml.load()Agent: bugs Category: bug Why this matters: Unhandled errors crash the application or hide bugs. File: Description: On line 116, fs.promises.readFile() returns a Promise but is not awaited. The Promise object is converted to string in template literals and passed to yaml.load(), which will receive '[object Promise]' instead of file contents. Suggestion: Await the fs.promises.readFile() call: Confidence: 98% Rule: 🟠 HIGH - HTTP request to external service without timeout (2 occurrences)Agent: security Category: security Why this matters: Hanging requests exhaust connections and can cause cascading failures. 📍 View all locations
Rule: 🟠 HIGH - Unhandled promise rejection from external HTTP call (2 occurrences)Agent: security Category: security Why this matters: Unhandled rejections can crash Node.js servers and cause downtime. 📍 View all locations
Rule: Review ID: |
2 participants
Summary
This PR fixes three bugs causing plugin crashes:
achievements plugin: Remove deprecated GitHub Projects (classic) API usage. GitHub sunset this API in May 2024 (announcement), causing
GraphqlResponseErrorwhen generating metrics.habits plugin: Add null check for commits without
authorproperty. Some PushEvent commits may not have author set, causingTypeError: Cannot destructure property 'author' of 'undefined'.activity plugin: Fix filter that destructures
author.emaildirectly. Added null check to preventTypeErrorwhenauthoris undefined.Changes
source/plugins/achievements/queries/achievements.graphql: Commented out deprecatedprojectsfieldsource/plugins/achievements/list/users.mjs: Disabled "Manager" achievement that relied on Projects APIsource/plugins/habits/index.mjs: Added.filter(commit => commit && commit.author)before processingsource/plugins/activity/index.mjs: Changed destructuring to explicit null checkTest plan
🤖 Generated with Claude Code