feat: Contributor Rewards Config v3 - Bounty #47 ($300)#90
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a complete new UbiquityOS plugin called Possibly related issues
🚥 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (16)
contributor-rewards-with-config/.gitattributes-2-3 (1)
2-3:⚠️ Potential issue | 🟠 MajorDon’t mark lockfiles as generated.
Lines 2–3 hide dependency-lock diffs in PRs, which weakens dependency review and security auditing.
Suggested fix
dist/** linguist-generated -*.lockb linguist-generated -*.lock linguist-generatedcontributor-rewards-with-config/.github/workflows/release-please.yml-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorPin the action to a commit SHA.
The workflow has write permissions to contents and pull-requests. Line 17 uses a movable tag (
@v4) instead of a commit SHA, creating a supply-chain risk.Suggested fix
- - uses: googleapis/release-please-action@v4 + - uses: googleapis/release-please-action@<FULL_COMMIT_SHA> # release-please-action v4contributor-rewards-with-config/.gitignore-20-20 (1)
20-20:⚠️ Potential issue | 🟠 MajorDo not ignore
manifest.jsonif it is a source-of-truth config file.Line 20 can hide critical listener/config changes from version control and review.
Suggested fix
-manifest.jsoncontributor-rewards-with-config/.github/workflows/conventional-commits.yml-12-12 (1)
12-12:⚠️ Potential issue | 🟠 MajorPin third-party action to a commit SHA. Using
@masteris mutable and weakens workflow integrity.Proposed hardening
- - uses: ubiquity/action-conventional-commits@master + - uses: ubiquity/action-conventional-commits@<commit_sha>contributor-rewards-with-config/.github/workflows/knip.yml-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorHandle non-PR runs before writing
pr-number.txt.Lines 20-21 assume a PR context; this is not true for
workflow_dispatch. That can cascade into reporter failures.Proposed fix
- - name: Store PR number - run: echo ${{ github.event.number }} > pr-number.txt + - name: Store PR number + if: ${{ github.event_name == 'pull_request' }} + run: echo ${{ github.event.pull_request.number }} > pr-number.txtAlso applies to: 20-21
contributor-rewards-with-config/.github/workflows/jest-testing.yml-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorScope workflow token permissions down.
Line 11 grants
write-allfor a test-only job. Prefer least privilege.Proposed fix
- permissions: write-all + permissions: + contents: readcontributor-rewards-with-config/tsconfig.json-32-32 (1)
32-32:⚠️ Potential issue | 🟠 MajorAdd Node types to the
typesarray.The
"types": ["jest"]setting excludes@types/node, which causes incomplete typing for Node globals used in source files. Properties likeprocess.env.NODE_ENVare accessed insrc/action.tsandsrc/worker.tsbut lack proper typing. The customprocess-env.d.tsattempts to extendNodeJS.ProcessEnv, but without@types/nodein the ambient types, it cannot resolve the base types.Fix
- "types": ["jest"] /* Specify type package names to be included without being referenced in a source file. */, + "types": ["node", "jest"] /* Include Node globals for src and Jest globals for tests. */,contributor-rewards-with-config/src/types/env.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟠 MajorMove
dotenv/configloading to Node-only entrypoint.The
dotenv/configimport at line 2 causes Node-specific side effects whenworker.ts(Cloudflare Worker) imports from./types. Cloudflare Workers don't support Node'sdotenvmodule, causing bundling/runtime errors.Move the import to
action.ts(GitHub Actions entrypoint) where it belongs, leavingenv.tsas pure type definitions.contributor-rewards-with-config/.github/workflows/compute.yml-25-29 (1)
25-29:⚠️ Potential issue | 🟠 MajorReduce permissions from
write-all.Line 28 grants far more than this job shows it needs. Please scope the token to the exact repo permissions required by the plugin execution path.
contributor-rewards-with-config/tests/__mocks__/helpers.ts-14-40 (1)
14-40:⚠️ Potential issue | 🟠 MajorMake
setupTests()safe to call more than once.This helper recreates the same repo and issue IDs on every call without resetting the in-memory DB first. Reusing it from
beforeEachor multiple suites will leak state or hit primary-key conflicts.contributor-rewards-with-config/src/worker.ts-33-35 (1)
33-35:⚠️ Potential issue | 🟠 MajorMove NODE_ENV to the env schema.
Line 35 reads
process.env.NODE_ENV, but environment variables in Cloudflare Workers should be declared inenvSchemaand passed through theenvparameter. AddNODE_ENVto the schema inenv.tsand read it fromenv.NODE_ENVinstead.contributor-rewards-with-config/.github/workflows/update-configuration.yml-134-134 (1)
134-134:⚠️ Potential issue | 🟠 MajorAvoid
write-all; scope job permissions minimally.Line 134 and Line 156 grant full write permissions. That is broader than necessary for deploy workflows.
Proposed tightening
- permissions: write-all + permissions: + contents: write + actions: read + id-token: noneAlso applies to: 156-156
contributor-rewards-with-config/src/handlers/rewards.ts-172-174 (1)
172-174:⚠️ Potential issue | 🟠 MajorLabel overrides currently reward non-target contributors.
Line 173 applies label overrides even when Line 172 computed
baseReward = 0.
That can create rewards for users who did not match any target role.Proposed fix
- const finalReward = applyLabelOverrides(baseReward, labels, labelOverrideConfig); + const finalReward = baseReward === 0 ? 0 : applyLabelOverrides(baseReward, labels, labelOverrideConfig);contributor-rewards-with-config/tests/__mocks__/handlers.ts-59-72 (1)
59-72:⚠️ Potential issue | 🟠 Major
getValuecan truncate payloads and returnundefinedunexpectedly.Line 62 reads a single stream chunk only.
Lines 43/51 destructure the result; parse/read failure here can throw at runtime.Proposed fix
-async function getValue(body: ReadableStream<Uint8Array> | null) { - if (body) { - const reader = body.getReader(); - const streamResult = await reader.read(); - if (!streamResult.done) { - const text = new TextDecoder().decode(streamResult.value); - try { - return JSON.parse(text); - } catch (error) { - console.error("Failed to parse body as JSON", error); - } - } - } +async function getValue(body: ReadableStream<Uint8Array> | null): Promise<Record<string, unknown> | undefined> { + if (!body) return undefined; + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + while (true) { + const { done, value } = await reader.read(); + if (done) break; + if (value) chunks.push(value); + } + if (chunks.length === 0) return undefined; + const total = chunks.reduce((n, c) => n + c.length, 0); + const merged = new Uint8Array(total); + let offset = 0; + for (const c of chunks) { + merged.set(c, offset); + offset += c.length; + } + try { + return JSON.parse(new TextDecoder().decode(merged)); + } catch (error) { + console.error("Failed to parse body as JSON", error); + return undefined; + } }contributor-rewards-with-config/src/handlers/rewards.ts-71-78 (1)
71-78:⚠️ Potential issue | 🟠 MajorActionless events cannot resolve reward config.
Line 77 always indexes by
action. For events likepush(parsed withaction: ""), this returnsundefined, so configured push rewards never apply.Proposed fix
export function getEventRewardConfig(settings: PluginSettings, eventName: string): EventRewardType | undefined { const { category, action } = parseEventName(eventName); const categoryConfig = settings[category as keyof PluginSettings]; if (!categoryConfig || typeof categoryConfig !== "object") { return undefined; } + if (action === "" && ("pull" in (categoryConfig as object) || "issue" in (categoryConfig as object))) { + return categoryConfig as EventRewardType; + } return (categoryConfig as Record<string, EventRewardType | undefined>)[action]; }contributor-rewards-with-config/.github/workflows/update-configuration.yml-139-140 (1)
139-140:⚠️ Potential issue | 🟠 MajorPin action refs to commit SHAs instead of mutable tags.
Lines 139 and 163 use mutable
@mainrefs, breaking reproducibility and increasing supply-chain risk.actions/checkout@v6(line 160) is stable and available, but pinning to a specific commit SHA (e.g.,@afaa513f0354a1e1ddde9e0f38cd2191a9b3dd7c # v6.0.2) is the recommended best practice for production workflows per GitHub security guidelines.Current state
139: uses: ubiquity-os/action-deploy-plugin@main 160: - uses: actions/checkout@v6 163: - uses: ubiquity-os/deno-deploy@main
🟡 Minor comments (4)
contributor-rewards-with-config/.dev.vars.example-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAlign example vars with actual plugin env keys.
Line 1 uses
MY_SECRET, but runtime config appears to rely onLOG_LEVELandKERNEL_PUBLIC_KEY. This can confuse local setup.Suggested update
-MY_SECRET="MY_SECRET" +LOG_LEVEL="INFO" +KERNEL_PUBLIC_KEY=""contributor-rewards-with-config/tests/__mocks__/users-get.json-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorDuplicate
loginvalue can make tests ambiguous.Line 10 repeats
login: "user1"for a different user. With current seeding, this can produce nondeterministic expectations in login-based lookups.Suggested fix
{ "id": 2, "name": "user2", - "login": "user1" + "login": "user2" }contributor-rewards-with-config/src/types/process-env.d.ts-3-5 (1)
3-5:⚠️ Potential issue | 🟡 MinorMake
GITHUB_TOKENoptional or remove it entirely.
GITHUB_TOKENis declared as required but is unused and not included inenvSchema. Either remove it or change toGITHUB_TOKEN?: string;to match the codebase pattern for optional environment variables (e.g.,KERNEL_PUBLIC_KEY).contributor-rewards-with-config/.github/workflows/jest-testing.yml-27-29 (1)
27-29:⚠️ Potential issue | 🟡 MinorGuard summary append when report file is missing.
Line 29 will fail if
test-dashboard.mdwas not generated.Proposed fix
- - name: Add Jest Report to Summary - if: always() - run: echo "$(cat test-dashboard.md)" >> $GITHUB_STEP_SUMMARY + - name: Add Jest Report to Summary + if: always() + run: | + if [ -f test-dashboard.md ]; then + cat test-dashboard.md >> "$GITHUB_STEP_SUMMARY" + fi
🧹 Nitpick comments (8)
contributor-rewards-with-config/.github/CODEOWNERS (1)
1-1: Add a default CODEOWNERS rule.This file is currently a no-op. Add at least a catch-all mapping (for example,
*@org/team``) so ownership and review routing are actually enforced.contributor-rewards-with-config/.env.example (1)
1-1: Make.env.exampleself-contained with required keys.Line 1 is useful, but adding explicit required variable placeholders here will reduce setup friction.
contributor-rewards-with-config/tests/__mocks__/issue-template.ts (1)
10-10: Prefer fixed timestamps in fixtures.new Date()in test data can make failures harder to reproduce.Proposed tweak
- created_at: new Date().toISOString(), + created_at: "2026-01-01T00:00:00.000Z", ... - updated_at: "", + updated_at: "2026-01-01T00:00:00.000Z",Also applies to: 23-23
contributor-rewards-with-config/.github/workflows/formatting-checks.yml (1)
19-24: Use the repo script to avoid CI drift. Reusingcheck-formattingkeeps local and CI behavior aligned.Proposed simplification
- name: Run formatting checks run: | bun install - bun eslint --fix-dry-run --ignore-pattern dist/ - bun format:cspell - bun prettier --check . + bun run check-formattingcontributor-rewards-with-config/.cspell.json (1)
4-4:ignorePathsis too broad. Ignoring**/*.jsondisables spellcheck for most config files.Proposed change
- "ignorePaths": ["**/*.json", "**/*.css", "node_modules", "**/*.log", "./src/adapters/supabase/**/**.ts", "dist"], + "ignorePaths": ["**/*.css", "node_modules", "**/*.log", "./src/adapters/supabase/**/**.ts", "dist"],contributor-rewards-with-config/.github/knip.ts (1)
4-4: Include worker entry in Knip scope. This avoids false positives/negatives for worker-only imports.Proposed update
- entry: ["src/action.ts"], + entry: ["src/action.ts", "src/worker.ts"],contributor-rewards-with-config/package.json (1)
27-29: Pin the manifest tool version.Using
@latesthere makes installs and builds non-reproducible. A new upstream release can break CI or generate a different manifest without any code change in this repo.contributor-rewards-with-config/tests/main.test.ts (1)
194-200: Add a regression test for non-target + label override behavior.Current tests verify override math, but not whether overrides should apply when base reward is
0for a non-target contributor.Suggested test addition
+ it("should not grant label override to non-target contributors", () => { + const localSettings: PluginSettings = { + pull_request: { + opened: { pull: { targets: [TargetRole.ISSUER], value: 5 } }, + }, + labelOverrides: { + bug: { value: 3 }, + }, + }; + const rewards = calculateRewards( + localSettings, + "pull_request.opened", + [{ login: "bob", issueAuthor: "alice", isOrgMember: false }], + "pull", + [{ name: "bug" }] + ); + expect(rewards).toHaveLength(0); + });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7973e93a-9aaa-45ea-95b9-f7699204c4da
📒 Files selected for processing (47)
contributor-rewards-with-config/.cspell.jsoncontributor-rewards-with-config/.dev.vars.examplecontributor-rewards-with-config/.env.examplecontributor-rewards-with-config/.gitattributescontributor-rewards-with-config/.github/CODEOWNERScontributor-rewards-with-config/.github/knip.tscontributor-rewards-with-config/.github/pull_request_template.mdcontributor-rewards-with-config/.github/workflows/compute.ymlcontributor-rewards-with-config/.github/workflows/conventional-commits.ymlcontributor-rewards-with-config/.github/workflows/formatting-checks.ymlcontributor-rewards-with-config/.github/workflows/jest-testing.ymlcontributor-rewards-with-config/.github/workflows/knip-reporter.ymlcontributor-rewards-with-config/.github/workflows/knip.ymlcontributor-rewards-with-config/.github/workflows/release-please.ymlcontributor-rewards-with-config/.github/workflows/update-configuration.ymlcontributor-rewards-with-config/.gitignorecontributor-rewards-with-config/.husky/commit-msgcontributor-rewards-with-config/.husky/pre-commitcontributor-rewards-with-config/.nvmrccontributor-rewards-with-config/.prettierignorecontributor-rewards-with-config/.prettierrccontributor-rewards-with-config/.yarnrc.ymlcontributor-rewards-with-config/README.mdcontributor-rewards-with-config/eslint.config.mjscontributor-rewards-with-config/jest.config.tscontributor-rewards-with-config/package.jsoncontributor-rewards-with-config/src/action.tscontributor-rewards-with-config/src/handlers/rewards.tscontributor-rewards-with-config/src/index.tscontributor-rewards-with-config/src/types.tscontributor-rewards-with-config/src/types/context.tscontributor-rewards-with-config/src/types/env.tscontributor-rewards-with-config/src/types/index.tscontributor-rewards-with-config/src/types/plugin-input.tscontributor-rewards-with-config/src/types/process-env.d.tscontributor-rewards-with-config/src/worker.tscontributor-rewards-with-config/strings.jsoncontributor-rewards-with-config/tests/__mocks__/db.tscontributor-rewards-with-config/tests/__mocks__/handlers.tscontributor-rewards-with-config/tests/__mocks__/helpers.tscontributor-rewards-with-config/tests/__mocks__/issue-template.tscontributor-rewards-with-config/tests/__mocks__/node.tscontributor-rewards-with-config/tests/__mocks__/strings.tscontributor-rewards-with-config/tests/__mocks__/users-get.jsoncontributor-rewards-with-config/tests/main.test.tscontributor-rewards-with-config/tsconfig.jsoncontributor-rewards-with-config/wrangler.toml
| // For now, compute reward for the sender | ||
| const contributors = [ | ||
| { | ||
| login: sender.login, | ||
| issueAuthor: issueOrPull.user?.login, | ||
| assignees: issueOrPull.assignees, | ||
| isOrgMember: false, // Would need API call to determine | ||
| }, | ||
| ]; | ||
|
|
||
| const rewards: ContributorReward[] = calculateRewards(config, eventName, contributors, contextType, labels); |
There was a problem hiding this comment.
Only rewarding the sender breaks target-based payouts.
This path never materializes REVIEWERS, COMMENTERS, or COMMITTERS, and COLLABORATOR can never match because Line 44 hard-codes isOrgMember: false. Events like pull_request.review_requested will pay the actor instead of the requested reviewer.
- Add build steps to workflow (setup-node, npm ci, npm run build) - Scope permissions from write-all to minimum required - Fix PR comment detection: check payload.issue.pull_request for issue_comment events - Fix issueAuthor to be boolean (was comparing to login string)
… reviewer/assignee extraction
- Fix relative imports in src/index.ts (../types → ./types, ../handlers/rewards → ./handlers/rewards) - Harden knip-reporter.yml: restrict permissions to least-privilege (contents: read, pull-requests: write) and pin action to specific commit - Fix PATCH comment handler in tests: use route :id param instead of issue_number, properly lookup existing record
|
Hi! 👋 Friendly follow-up on this PR (submitted 16+ days ago). Happy to address any feedback or make adjustments if needed. Please let me know if there's anything I can help with to move this forward. 🙏 |
1 participant
Contributor Rewards With Config v3
Implements #47 — the third stage of the Contributor Role series ($300 bounty).
Series Overview
What it does
review_request_removedgives -1)Configuration Example
Architecture
src/types/plugin-input.tssrc/handlers/rewards.tssrc/index.tssrc/worker.tssrc/action.tsmanifest.jsontests/main.test.tsHow it works
pull_request.opened)Test Results
All 30 tests pass:
Tech Stack
Standalone Repository
Also available as a standalone repo: https://github.com/zhaog100/contributor-rewards-with-config