Repository navigation
Bring Devvit and downstream target workflow to main - #4
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR adds a new "reddit" Devvit Web build target across the monorepo: a new ChangesReddit Devvit Web Target Implementation
Unrelated iOS project and packaging tool changes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
1d802ea to
a037b77
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
native-plugins/capacitor-game-services/Package.swift (1)
19-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the unused Cordova dependency
native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swiftonly importsCapacitorandFoundation, soCordovainPackage.swiftlooks unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native-plugins/capacitor-game-services/Package.swift` around lines 19 - 20, The Package.swift dependency list includes an unused Cordova product for the CapacitorGameServices plugin. Remove the Cordova package product from the target dependencies in Package.swift, keeping only the Capacitor dependency used by CapacitorGameServicesPlugin.swift, and verify the package still builds with the remaining imports..github/workflows/build-target.yml (1)
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDevvit dist glob uploaded for every target, not just reddit.
apps/target-devvit/dist/**is now included in the upload-artifact path unconditionally, so android/ios/ait/web-preview runs will also attempt to match this glob even though it's never populated for those targets. Harmless with defaultif-no-files-found: warn, but adds noise to every non-devvit build log.Suggested scoping
- - uses: actions/upload-artifact@v6 + - uses: actions/upload-artifact@v6 + if: always() with: name: ${{ inputs.target }}-${{ inputs.profile }} path: | artifacts/** release-output/** - apps/target-devvit/dist/** + ${{ inputs.target == 'reddit' && 'apps/target-devvit/dist/**' || '' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-target.yml around lines 68 - 74, The upload-artifact step in the build target workflow always includes the Devvit dist glob, which causes unnecessary warnings on non-reddit targets. Scope the `apps/target-devvit/dist/**` path so it is only added when the target is `reddit`, keeping the existing `actions/upload-artifact@v6` step and other artifact paths unchanged..github/workflows/release-devvit.yml (1)
41-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant preflight validation before
build:target.Lines 42-49 already run
validate:catalog,validate:ads,validate:target-config,validate:effective-config,validate:targets(and build-packages runs as part ofpnpm check/earlier steps). SinceMPGD_SKIP_BUILD_TARGET_PREFLIGHTisn't set here,pnpm build:target reddit(line 54) reruns the same validate steps andbuild-packagesagain internally (pertools/target/build-target.ts's preflight guard). This duplicates work the release.yml workflow explicitly avoids viaMPGD_SKIP_BUILD_TARGET_PREFLIGHT=1.♻️ Suggested fix
- run: pnpm build:target reddit "${PROFILE}" env: PROFILE: ${{ inputs.profile }} + MPGD_SKIP_BUILD_TARGET_PREFLIGHT: '1'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-devvit.yml around lines 41 - 57, The release workflow is doing duplicate preflight validation before build:target because buildTarget still runs its own checks. In the release-devvit workflow, set MPGD_SKIP_BUILD_TARGET_PREFLIGHT=1 for the pnpm build:target reddit step so the already-run validations are not repeated. Use the buildTarget path in tools/target/build-target.ts and the existing release workflow build step as the key places to align behavior.packages/game-services/src/types.ts (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the ledger-target literal set.
This union/assert pair duplicates
LeaderboardLedgerTarget/assertLedgerTargetinbackend/leaderboard-ledger/src/index.tsalmost verbatim (same literals, same manual!==chain, same error message text). Both needed manual edits in this PR to add'reddit'; a future target change risks updating only one side. Since both are already subsets of the canonicalPlatformTarget(packages/platform/src/index.ts), consider exporting a single ledger-target literal array/type (and shared assertion) from@mpgd/platformfor both packages to consume.Also applies to: 243-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/game-services/src/types.ts` around lines 10 - 13, The ledger-target set is duplicated between GameServicesLedgerTarget and the LeaderboardLedgerTarget/assertLedgerTarget logic, so future platform changes can drift. Centralize the canonical ledger-target literals in `@mpgd/platform` by exporting a shared literal array/type and shared assertion, then update the types.ts definition and the backend/leaderboard-ledger/src/index.ts target check to consume those shared symbols instead of maintaining separate literal unions/manual comparisons.tools/target/build-target.ts (1)
130-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the archive/simulator mutual-exclusion check before the side-effecting
cap sync.The invariant check for
MPGD_RUN_IOS_ARCHIVE+MPGD_RUN_IOS_SIMULATOR_BUILDruns afterensureCapacitorPlatform/cap sync(Line 134-135) already executed. A misconfigured env would waste that (potentially slow, native-tooling) work before failing.♻️ Proposed reordering
case 'capacitor-ios': { const webDir = targetPath(requireString(target.webDir, `${targetName}.webDir`)); const shellApp = targetPath(requireString(target.shellApp, `${targetName}.shellApp`)); + if (process.env.MPGD_RUN_IOS_ARCHIVE === '1' && process.env.MPGD_RUN_IOS_SIMULATOR_BUILD === '1') { + throw new Error('Set only one of MPGD_RUN_IOS_ARCHIVE or MPGD_RUN_IOS_SIMULATOR_BUILD.'); + } replaceDirectory(`${gameApp}/dist`, webDir); ensureCapacitorPlatform(shellApp, 'ios', env); run('pnpm', ['--dir', shellApp, 'cap', 'sync', 'ios'], env); let releaseArtifact = requireString(target.shellApp, `${targetName}.shellApp`) + '/ios'; - - if (process.env.MPGD_RUN_IOS_ARCHIVE === '1' && process.env.MPGD_RUN_IOS_SIMULATOR_BUILD === '1') { - throw new Error('Set only one of MPGD_RUN_IOS_ARCHIVE or MPGD_RUN_IOS_SIMULATOR_BUILD.'); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/target/build-target.ts` around lines 130 - 141, Move the MPGD_RUN_IOS_ARCHIVE and MPGD_RUN_IOS_SIMULATOR_BUILD mutual-exclusion check in the capacitor-ios branch of build-target.ts so it runs before any side effects like ensureCapacitorPlatform and run('pnpm', ['--dir', shellApp, 'cap', 'sync', 'ios'], env). Keep the check near the top of the case block, using the existing targetName/shellApp logic only after the invariant is validated, so misconfigured env fails fast before native tooling work starts.tools/smoke/adapter-effective-config.ts (1)
174-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting disabled capabilities are actually not delegated for reddit.
verifyDevvitAdapteronly exercisesleaderboard.submitScore(the enabled path) and infers the disabled path from omission. The capacitor test (Lines 103-123) explicitly exercisespurchase/showRewardedto prove they are delegated when enabled; an analogous explicit attempt-and-assert-rejection forgateway.commerce.purchase/gateway.ads.showRewardedon reddit would give stronger confidence thatwithTargetAvailabilityactually blocks these for the disabled effective config, rather than relying on them simply never being called.Also applies to: 307-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/smoke/adapter-effective-config.ts` around lines 174 - 212, verifyDevvitAdapter only checks the enabled leaderboard.submitScore path, so add explicit assertions that disabled reddit capabilities are blocked and not delegated. In verifyDevvitAdapter, use gateway.commerce.purchase and gateway.ads.showRewarded with reddit-effective config, then assert they reject and that createRecordingBridge does not record those method calls; this should mirror the enabled-path coverage already used in the capacitor test and validate withTargetAvailability behavior directly.tools/validate-public-readiness.ts (1)
49-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlso add
.devvit/toblockedTrackedGeneratedPrefixes. It holds local Devvit auth state (~/.devvit/token), so a committed copy should fail the public-readiness check too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/validate-public-readiness.ts` at line 49, The public-readiness check currently ignores `.devvit/` in the tracked generated paths, so add `.devvit/` to blockedTrackedGeneratedPrefixes in validate-public-readiness.ts. Update the logic around the prefix list so committed copies of local Devvit auth state are treated as blocked by the public-readiness validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-ait.yml:
- Around line 19-21: The release-ait workflow’s actions/checkout step is missing
the credential-hardening setting used elsewhere in this PR. Update the checkout
configuration in the release workflow to include persist-credentials: false
alongside fetch-depth: 0 so the GitHub token is not left in local git config;
use the existing actions/checkout step in the release-ait workflow as the place
to apply this change.
In @.github/workflows/release-android.yml:
- Around line 19-22: The checkout step in the Android release workflow is
missing the same credential-hardening used elsewhere. Update the
`actions/checkout` usage in this workflow to set `persist-credentials: false`,
matching the pattern already used in `release-devvit.yml` and `release.yml`, so
the `checkout` step does not retain credentials after cloning.
In @.github/workflows/release-ios.yml:
- Around line 22-26: The checkout step in the release-ios workflow is missing
the same credential hardening used in the other release workflows. Update the
actions/checkout invocation in this workflow to set persist-credentials to
false, matching the existing checkout configuration pattern in the related
release workflows.
In `@apps/target-devvit/src/server/index.ts`:
- Around line 533-547: `currentDisplayName()` is reading a server-context
`username` field that does not exist, so it always falls back instead of
returning the real display name. Update the display-name lookup in
`currentDisplayName` to use `context.reddit.getCurrentUsername()` and make the
function async if needed, then adjust the `identity.getPlayer` call path to
await this lookup and pass the fetched username through instead of relying on
`context.username`.
- Around line 21-63: The /internal/menu/create-post handler in app.post is
exposed over HTTP and currently calls reddit.submitCustomPost without verifying
the caller is authorized. Add an explicit authorization gate before the
create-post logic runs, using the request context available in this route and
the existing currentSubredditName check to ensure only valid menu invocations or
moderators can proceed. If the caller is not authorized, return a neutral
UiResponse and skip the reddit.submitCustomPost call. Keep the existing success
and error handling in the same handler after the authz check.
- Around line 446-458: The leaderboard lock setup in acquireLeaderboardLock
currently passes a Date from leaderboardLockExpirationDate into redis.set, but
Devvit Redis expects a numeric TTL in seconds for expiration. Update the lock
acquisition flow to pass the TTL as a number (using leaderboardLockTtlSeconds
directly or an equivalent numeric helper) and remove the Date-based helper so
the lock lifetime is applied correctly.
In `@tools/smoke/embedded-target-config.ts`:
- Around line 131-137: The failure check in the command execution path is gating
on stdout instead of the exit code, which can miss real unzip errors. Update the
error handling around the command/result check in embedded-target-config.ts to
treat nonzero status as failure by default and only allow the specific benign
warning exit codes you intentionally want to ignore; use the existing
result.status, result.stdout, and result.stderr handling in this block to narrow
the tolerated cases.
---
Nitpick comments:
In @.github/workflows/build-target.yml:
- Around line 68-74: The upload-artifact step in the build target workflow
always includes the Devvit dist glob, which causes unnecessary warnings on
non-reddit targets. Scope the `apps/target-devvit/dist/**` path so it is only
added when the target is `reddit`, keeping the existing
`actions/upload-artifact@v6` step and other artifact paths unchanged.
In @.github/workflows/release-devvit.yml:
- Around line 41-57: The release workflow is doing duplicate preflight
validation before build:target because buildTarget still runs its own checks. In
the release-devvit workflow, set MPGD_SKIP_BUILD_TARGET_PREFLIGHT=1 for the pnpm
build:target reddit step so the already-run validations are not repeated. Use
the buildTarget path in tools/target/build-target.ts and the existing release
workflow build step as the key places to align behavior.
In `@native-plugins/capacitor-game-services/Package.swift`:
- Around line 19-20: The Package.swift dependency list includes an unused
Cordova product for the CapacitorGameServices plugin. Remove the Cordova package
product from the target dependencies in Package.swift, keeping only the
Capacitor dependency used by CapacitorGameServicesPlugin.swift, and verify the
package still builds with the remaining imports.
In `@packages/game-services/src/types.ts`:
- Around line 10-13: The ledger-target set is duplicated between
GameServicesLedgerTarget and the LeaderboardLedgerTarget/assertLedgerTarget
logic, so future platform changes can drift. Centralize the canonical
ledger-target literals in `@mpgd/platform` by exporting a shared literal
array/type and shared assertion, then update the types.ts definition and the
backend/leaderboard-ledger/src/index.ts target check to consume those shared
symbols instead of maintaining separate literal unions/manual comparisons.
In `@tools/smoke/adapter-effective-config.ts`:
- Around line 174-212: verifyDevvitAdapter only checks the enabled
leaderboard.submitScore path, so add explicit assertions that disabled reddit
capabilities are blocked and not delegated. In verifyDevvitAdapter, use
gateway.commerce.purchase and gateway.ads.showRewarded with reddit-effective
config, then assert they reject and that createRecordingBridge does not record
those method calls; this should mirror the enabled-path coverage already used in
the capacitor test and validate withTargetAvailability behavior directly.
In `@tools/target/build-target.ts`:
- Around line 130-141: Move the MPGD_RUN_IOS_ARCHIVE and
MPGD_RUN_IOS_SIMULATOR_BUILD mutual-exclusion check in the capacitor-ios branch
of build-target.ts so it runs before any side effects like
ensureCapacitorPlatform and run('pnpm', ['--dir', shellApp, 'cap', 'sync',
'ios'], env). Keep the check near the top of the case block, using the existing
targetName/shellApp logic only after the invariant is validated, so
misconfigured env fails fast before native tooling work starts.
In `@tools/validate-public-readiness.ts`:
- Line 49: The public-readiness check currently ignores `.devvit/` in the
tracked generated paths, so add `.devvit/` to blockedTrackedGeneratedPrefixes in
validate-public-readiness.ts. Update the logic around the prefix list so
committed copies of local Devvit auth state are treated as blocked by the
public-readiness validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b558905-718c-438c-a9dc-baf8bf771391
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/build-target.yml.github/workflows/ci.yml.github/workflows/release-ait.yml.github/workflows/release-android.yml.github/workflows/release-devvit.yml.github/workflows/release-ios.yml.github/workflows/release.yml.gitignore.mcp.json.mise.tomlREADME.mdadapters/devvit/CHANGELOG.mdadapters/devvit/package.jsonadapters/devvit/src/index.test.tsadapters/devvit/src/index.tsadapters/devvit/tsconfig.jsonapps/game-phaser/package.jsonapps/game-phaser/src/platform/demoGameServices.tsapps/game-phaser/src/platform/installPlatform.tsapps/game-phaser/src/platform/runtimeDetector.test.tsapps/game-phaser/src/platform/runtimeDetector.tsapps/game-phaser/src/scenes/ResultScene.tsapps/mobile-capacitor/ios/App/App.xcodeproj/project.pbxprojapps/mobile-capacitor/ios/App/App.xcodeproj/xcshareddata/xcschemes/App.xcschemeapps/target-devvit/README.mdapps/target-devvit/devvit.jsonapps/target-devvit/package.jsonapps/target-devvit/src/server/index.tsapps/target-devvit/tsconfig.jsonapps/target-devvit/vite.server.config.tsbackend/leaderboard-ledger/src/index.tsdocs/AGENTIC_GAME_WORKFLOW.mddocs/GAME_DEVELOPMENT.mddocs/PUBLIC_RELEASE_CHECKLIST.mddocs/ROADMAP_CHECKLIST.mdexamples/phaser-starter/agent/game.manifest.jsonnative-plugins/capacitor-game-services/Package.swiftpackage.jsonpackages/game-services/src/client.test.tspackages/game-services/src/client.tspackages/game-services/src/server.test.tspackages/game-services/src/types.tspackages/target-config/src/runtime.tspackages/target-config/targets.jsonplatform.targets.jsontools/package/build-packages.tstools/package/workspace.tstools/smoke/adapter-effective-config.tstools/smoke/build-target-artifacts.tstools/smoke/effective-target-config.tstools/smoke/embedded-target-config.tstools/smoke/target-config-runtime.tstools/smoke/verify-target-artifacts.tstools/target/build-target.tstools/target/effective-config.tstools/target/generate-release-manifest.tstools/target/platform-targets.tstools/target/schemas.tstools/target/validate-target-config.tstools/validate-public-readiness.tstools/validate-starter-workflow.tstools/validate-target-config.tstsconfig.graph.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (7)
native-plugins/capacitor-game-services/Package.swift (1)
19-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the unused Cordova dependency
native-plugins/capacitor-game-services/ios/Sources/CapacitorGameServices/CapacitorGameServicesPlugin.swiftonly importsCapacitorandFoundation, soCordovainPackage.swiftlooks unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native-plugins/capacitor-game-services/Package.swift` around lines 19 - 20, The Package.swift dependency list includes an unused Cordova product for the CapacitorGameServices plugin. Remove the Cordova package product from the target dependencies in Package.swift, keeping only the Capacitor dependency used by CapacitorGameServicesPlugin.swift, and verify the package still builds with the remaining imports..github/workflows/build-target.yml (1)
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDevvit dist glob uploaded for every target, not just reddit.
apps/target-devvit/dist/**is now included in the upload-artifact path unconditionally, so android/ios/ait/web-preview runs will also attempt to match this glob even though it's never populated for those targets. Harmless with defaultif-no-files-found: warn, but adds noise to every non-devvit build log.Suggested scoping
- - uses: actions/upload-artifact@v6 + - uses: actions/upload-artifact@v6 + if: always() with: name: ${{ inputs.target }}-${{ inputs.profile }} path: | artifacts/** release-output/** - apps/target-devvit/dist/** + ${{ inputs.target == 'reddit' && 'apps/target-devvit/dist/**' || '' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-target.yml around lines 68 - 74, The upload-artifact step in the build target workflow always includes the Devvit dist glob, which causes unnecessary warnings on non-reddit targets. Scope the `apps/target-devvit/dist/**` path so it is only added when the target is `reddit`, keeping the existing `actions/upload-artifact@v6` step and other artifact paths unchanged..github/workflows/release-devvit.yml (1)
41-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant preflight validation before
build:target.Lines 42-49 already run
validate:catalog,validate:ads,validate:target-config,validate:effective-config,validate:targets(and build-packages runs as part ofpnpm check/earlier steps). SinceMPGD_SKIP_BUILD_TARGET_PREFLIGHTisn't set here,pnpm build:target reddit(line 54) reruns the same validate steps andbuild-packagesagain internally (pertools/target/build-target.ts's preflight guard). This duplicates work the release.yml workflow explicitly avoids viaMPGD_SKIP_BUILD_TARGET_PREFLIGHT=1.♻️ Suggested fix
- run: pnpm build:target reddit "${PROFILE}" env: PROFILE: ${{ inputs.profile }} + MPGD_SKIP_BUILD_TARGET_PREFLIGHT: '1'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-devvit.yml around lines 41 - 57, The release workflow is doing duplicate preflight validation before build:target because buildTarget still runs its own checks. In the release-devvit workflow, set MPGD_SKIP_BUILD_TARGET_PREFLIGHT=1 for the pnpm build:target reddit step so the already-run validations are not repeated. Use the buildTarget path in tools/target/build-target.ts and the existing release workflow build step as the key places to align behavior.packages/game-services/src/types.ts (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the ledger-target literal set.
This union/assert pair duplicates
LeaderboardLedgerTarget/assertLedgerTargetinbackend/leaderboard-ledger/src/index.tsalmost verbatim (same literals, same manual!==chain, same error message text). Both needed manual edits in this PR to add'reddit'; a future target change risks updating only one side. Since both are already subsets of the canonicalPlatformTarget(packages/platform/src/index.ts), consider exporting a single ledger-target literal array/type (and shared assertion) from@mpgd/platformfor both packages to consume.Also applies to: 243-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/game-services/src/types.ts` around lines 10 - 13, The ledger-target set is duplicated between GameServicesLedgerTarget and the LeaderboardLedgerTarget/assertLedgerTarget logic, so future platform changes can drift. Centralize the canonical ledger-target literals in `@mpgd/platform` by exporting a shared literal array/type and shared assertion, then update the types.ts definition and the backend/leaderboard-ledger/src/index.ts target check to consume those shared symbols instead of maintaining separate literal unions/manual comparisons.tools/target/build-target.ts (1)
130-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the archive/simulator mutual-exclusion check before the side-effecting
cap sync.The invariant check for
MPGD_RUN_IOS_ARCHIVE+MPGD_RUN_IOS_SIMULATOR_BUILDruns afterensureCapacitorPlatform/cap sync(Line 134-135) already executed. A misconfigured env would waste that (potentially slow, native-tooling) work before failing.♻️ Proposed reordering
case 'capacitor-ios': { const webDir = targetPath(requireString(target.webDir, `${targetName}.webDir`)); const shellApp = targetPath(requireString(target.shellApp, `${targetName}.shellApp`)); + if (process.env.MPGD_RUN_IOS_ARCHIVE === '1' && process.env.MPGD_RUN_IOS_SIMULATOR_BUILD === '1') { + throw new Error('Set only one of MPGD_RUN_IOS_ARCHIVE or MPGD_RUN_IOS_SIMULATOR_BUILD.'); + } replaceDirectory(`${gameApp}/dist`, webDir); ensureCapacitorPlatform(shellApp, 'ios', env); run('pnpm', ['--dir', shellApp, 'cap', 'sync', 'ios'], env); let releaseArtifact = requireString(target.shellApp, `${targetName}.shellApp`) + '/ios'; - - if (process.env.MPGD_RUN_IOS_ARCHIVE === '1' && process.env.MPGD_RUN_IOS_SIMULATOR_BUILD === '1') { - throw new Error('Set only one of MPGD_RUN_IOS_ARCHIVE or MPGD_RUN_IOS_SIMULATOR_BUILD.'); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/target/build-target.ts` around lines 130 - 141, Move the MPGD_RUN_IOS_ARCHIVE and MPGD_RUN_IOS_SIMULATOR_BUILD mutual-exclusion check in the capacitor-ios branch of build-target.ts so it runs before any side effects like ensureCapacitorPlatform and run('pnpm', ['--dir', shellApp, 'cap', 'sync', 'ios'], env). Keep the check near the top of the case block, using the existing targetName/shellApp logic only after the invariant is validated, so misconfigured env fails fast before native tooling work starts.tools/smoke/adapter-effective-config.ts (1)
174-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting disabled capabilities are actually not delegated for reddit.
verifyDevvitAdapteronly exercisesleaderboard.submitScore(the enabled path) and infers the disabled path from omission. The capacitor test (Lines 103-123) explicitly exercisespurchase/showRewardedto prove they are delegated when enabled; an analogous explicit attempt-and-assert-rejection forgateway.commerce.purchase/gateway.ads.showRewardedon reddit would give stronger confidence thatwithTargetAvailabilityactually blocks these for the disabled effective config, rather than relying on them simply never being called.Also applies to: 307-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/smoke/adapter-effective-config.ts` around lines 174 - 212, verifyDevvitAdapter only checks the enabled leaderboard.submitScore path, so add explicit assertions that disabled reddit capabilities are blocked and not delegated. In verifyDevvitAdapter, use gateway.commerce.purchase and gateway.ads.showRewarded with reddit-effective config, then assert they reject and that createRecordingBridge does not record those method calls; this should mirror the enabled-path coverage already used in the capacitor test and validate withTargetAvailability behavior directly.tools/validate-public-readiness.ts (1)
49-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlso add
.devvit/toblockedTrackedGeneratedPrefixes. It holds local Devvit auth state (~/.devvit/token), so a committed copy should fail the public-readiness check too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/validate-public-readiness.ts` at line 49, The public-readiness check currently ignores `.devvit/` in the tracked generated paths, so add `.devvit/` to blockedTrackedGeneratedPrefixes in validate-public-readiness.ts. Update the logic around the prefix list so committed copies of local Devvit auth state are treated as blocked by the public-readiness validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-ait.yml:
- Around line 19-21: The release-ait workflow’s actions/checkout step is missing
the credential-hardening setting used elsewhere in this PR. Update the checkout
configuration in the release workflow to include persist-credentials: false
alongside fetch-depth: 0 so the GitHub token is not left in local git config;
use the existing actions/checkout step in the release-ait workflow as the place
to apply this change.
In @.github/workflows/release-android.yml:
- Around line 19-22: The checkout step in the Android release workflow is
missing the same credential-hardening used elsewhere. Update the
`actions/checkout` usage in this workflow to set `persist-credentials: false`,
matching the pattern already used in `release-devvit.yml` and `release.yml`, so
the `checkout` step does not retain credentials after cloning.
In @.github/workflows/release-ios.yml:
- Around line 22-26: The checkout step in the release-ios workflow is missing
the same credential hardening used in the other release workflows. Update the
actions/checkout invocation in this workflow to set persist-credentials to
false, matching the existing checkout configuration pattern in the related
release workflows.
In `@apps/target-devvit/src/server/index.ts`:
- Around line 533-547: `currentDisplayName()` is reading a server-context
`username` field that does not exist, so it always falls back instead of
returning the real display name. Update the display-name lookup in
`currentDisplayName` to use `context.reddit.getCurrentUsername()` and make the
function async if needed, then adjust the `identity.getPlayer` call path to
await this lookup and pass the fetched username through instead of relying on
`context.username`.
- Around line 21-63: The /internal/menu/create-post handler in app.post is
exposed over HTTP and currently calls reddit.submitCustomPost without verifying
the caller is authorized. Add an explicit authorization gate before the
create-post logic runs, using the request context available in this route and
the existing currentSubredditName check to ensure only valid menu invocations or
moderators can proceed. If the caller is not authorized, return a neutral
UiResponse and skip the reddit.submitCustomPost call. Keep the existing success
and error handling in the same handler after the authz check.
- Around line 446-458: The leaderboard lock setup in acquireLeaderboardLock
currently passes a Date from leaderboardLockExpirationDate into redis.set, but
Devvit Redis expects a numeric TTL in seconds for expiration. Update the lock
acquisition flow to pass the TTL as a number (using leaderboardLockTtlSeconds
directly or an equivalent numeric helper) and remove the Date-based helper so
the lock lifetime is applied correctly.
In `@tools/smoke/embedded-target-config.ts`:
- Around line 131-137: The failure check in the command execution path is gating
on stdout instead of the exit code, which can miss real unzip errors. Update the
error handling around the command/result check in embedded-target-config.ts to
treat nonzero status as failure by default and only allow the specific benign
warning exit codes you intentionally want to ignore; use the existing
result.status, result.stdout, and result.stderr handling in this block to narrow
the tolerated cases.
---
Nitpick comments:
In @.github/workflows/build-target.yml:
- Around line 68-74: The upload-artifact step in the build target workflow
always includes the Devvit dist glob, which causes unnecessary warnings on
non-reddit targets. Scope the `apps/target-devvit/dist/**` path so it is only
added when the target is `reddit`, keeping the existing
`actions/upload-artifact@v6` step and other artifact paths unchanged.
In @.github/workflows/release-devvit.yml:
- Around line 41-57: The release workflow is doing duplicate preflight
validation before build:target because buildTarget still runs its own checks. In
the release-devvit workflow, set MPGD_SKIP_BUILD_TARGET_PREFLIGHT=1 for the pnpm
build:target reddit step so the already-run validations are not repeated. Use
the buildTarget path in tools/target/build-target.ts and the existing release
workflow build step as the key places to align behavior.
In `@native-plugins/capacitor-game-services/Package.swift`:
- Around line 19-20: The Package.swift dependency list includes an unused
Cordova product for the CapacitorGameServices plugin. Remove the Cordova package
product from the target dependencies in Package.swift, keeping only the
Capacitor dependency used by CapacitorGameServicesPlugin.swift, and verify the
package still builds with the remaining imports.
In `@packages/game-services/src/types.ts`:
- Around line 10-13: The ledger-target set is duplicated between
GameServicesLedgerTarget and the LeaderboardLedgerTarget/assertLedgerTarget
logic, so future platform changes can drift. Centralize the canonical
ledger-target literals in `@mpgd/platform` by exporting a shared literal
array/type and shared assertion, then update the types.ts definition and the
backend/leaderboard-ledger/src/index.ts target check to consume those shared
symbols instead of maintaining separate literal unions/manual comparisons.
In `@tools/smoke/adapter-effective-config.ts`:
- Around line 174-212: verifyDevvitAdapter only checks the enabled
leaderboard.submitScore path, so add explicit assertions that disabled reddit
capabilities are blocked and not delegated. In verifyDevvitAdapter, use
gateway.commerce.purchase and gateway.ads.showRewarded with reddit-effective
config, then assert they reject and that createRecordingBridge does not record
those method calls; this should mirror the enabled-path coverage already used in
the capacitor test and validate withTargetAvailability behavior directly.
In `@tools/target/build-target.ts`:
- Around line 130-141: Move the MPGD_RUN_IOS_ARCHIVE and
MPGD_RUN_IOS_SIMULATOR_BUILD mutual-exclusion check in the capacitor-ios branch
of build-target.ts so it runs before any side effects like
ensureCapacitorPlatform and run('pnpm', ['--dir', shellApp, 'cap', 'sync',
'ios'], env). Keep the check near the top of the case block, using the existing
targetName/shellApp logic only after the invariant is validated, so
misconfigured env fails fast before native tooling work starts.
In `@tools/validate-public-readiness.ts`:
- Line 49: The public-readiness check currently ignores `.devvit/` in the
tracked generated paths, so add `.devvit/` to blockedTrackedGeneratedPrefixes in
validate-public-readiness.ts. Update the logic around the prefix list so
committed copies of local Devvit auth state are treated as blocked by the
public-readiness validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b558905-718c-438c-a9dc-baf8bf771391
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/build-target.yml.github/workflows/ci.yml.github/workflows/release-ait.yml.github/workflows/release-android.yml.github/workflows/release-devvit.yml.github/workflows/release-ios.yml.github/workflows/release.yml.gitignore.mcp.json.mise.tomlREADME.mdadapters/devvit/CHANGELOG.mdadapters/devvit/package.jsonadapters/devvit/src/index.test.tsadapters/devvit/src/index.tsadapters/devvit/tsconfig.jsonapps/game-phaser/package.jsonapps/game-phaser/src/platform/demoGameServices.tsapps/game-phaser/src/platform/installPlatform.tsapps/game-phaser/src/platform/runtimeDetector.test.tsapps/game-phaser/src/platform/runtimeDetector.tsapps/game-phaser/src/scenes/ResultScene.tsapps/mobile-capacitor/ios/App/App.xcodeproj/project.pbxprojapps/mobile-capacitor/ios/App/App.xcodeproj/xcshareddata/xcschemes/App.xcschemeapps/target-devvit/README.mdapps/target-devvit/devvit.jsonapps/target-devvit/package.jsonapps/target-devvit/src/server/index.tsapps/target-devvit/tsconfig.jsonapps/target-devvit/vite.server.config.tsbackend/leaderboard-ledger/src/index.tsdocs/AGENTIC_GAME_WORKFLOW.mddocs/GAME_DEVELOPMENT.mddocs/PUBLIC_RELEASE_CHECKLIST.mddocs/ROADMAP_CHECKLIST.mdexamples/phaser-starter/agent/game.manifest.jsonnative-plugins/capacitor-game-services/Package.swiftpackage.jsonpackages/game-services/src/client.test.tspackages/game-services/src/client.tspackages/game-services/src/server.test.tspackages/game-services/src/types.tspackages/target-config/src/runtime.tspackages/target-config/targets.jsonplatform.targets.jsontools/package/build-packages.tstools/package/workspace.tstools/smoke/adapter-effective-config.tstools/smoke/build-target-artifacts.tstools/smoke/effective-target-config.tstools/smoke/embedded-target-config.tstools/smoke/target-config-runtime.tstools/smoke/verify-target-artifacts.tstools/target/build-target.tstools/target/effective-config.tstools/target/generate-release-manifest.tstools/target/platform-targets.tstools/target/schemas.tstools/target/validate-target-config.tstools/validate-public-readiness.tstools/validate-starter-workflow.tstools/validate-target-config.tstsconfig.graph.json
🛑 Comments failed to post (7)
.github/workflows/release-ait.yml (1)
19-21: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Checkout step missing
persist-credentials: false.Static analysis (zizmor) flags credential persistence through artifacts here. Every other updated workflow in this PR (
build-target.yml,ci.yml) setspersist-credentials: falseonactions/checkout, but this one only setsfetch-depth: 0, leaving the GitHub token persisted in the local git config for the remainder of the job.Suggested fix
- uses: actions/checkout@v5 with: fetch-depth: 0 + persist-credentials: false📝 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.- uses: actions/checkout@v5 with: fetch-depth: 0 persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 19-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-ait.yml around lines 19 - 21, The release-ait workflow’s actions/checkout step is missing the credential-hardening setting used elsewhere in this PR. Update the checkout configuration in the release workflow to include persist-credentials: false alongside fetch-depth: 0 so the GitHub token is not left in local git config; use the existing actions/checkout step in the release-ait workflow as the place to apply this change.Source: Linters/SAST tools
.github/workflows/release-android.yml (1)
19-22: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing
persist-credentials: falseon checkout.Sibling workflows touched in this same PR (
release-devvit.yml,release.yml) explicitly setpersist-credentials: falseon everyactions/checkoutstep, but this one doesn't. zizmor also flags this as a credential-persistence risk (artipacked).🔒 Proposed fix
- uses: actions/checkout@v5 with: fetch-depth: 0 + persist-credentials: false📝 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.- uses: actions/checkout@v5 with: fetch-depth: 0 persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 19-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-android.yml around lines 19 - 22, The checkout step in the Android release workflow is missing the same credential-hardening used elsewhere. Update the `actions/checkout` usage in this workflow to set `persist-credentials: false`, matching the pattern already used in `release-devvit.yml` and `release.yml`, so the `checkout` step does not retain credentials after cloning.Source: Linters/SAST tools
.github/workflows/release-ios.yml (1)
22-26: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Missing
persist-credentials: falseon checkout.Flagged by zizmor (artipacked): the credential is left persisted in the workspace after checkout, unlike the sibling
release-devvit.ymlandrelease.ymlworkflows updated in this PR which explicitly setpersist-credentials: falseon every checkout step.🔒 Suggested fix
- uses: actions/checkout@v5 with: fetch-depth: 0 + persist-credentials: false📝 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.steps: - uses: actions/checkout@v5 with: fetch-depth: 0 persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 23-25: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-ios.yml around lines 22 - 26, The checkout step in the release-ios workflow is missing the same credential hardening used in the other release workflows. Update the actions/checkout invocation in this workflow to set persist-credentials to false, matching the existing checkout configuration pattern in the related release workflows.Source: Linters/SAST tools
apps/target-devvit/src/server/index.ts (3)
21-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Devvit's@devvit/webserver framework restrict "/internal/*" routes to internal menu-action invocations only, or are they publicly reachable HTTP endpoints?💡 Result:
Routes under the /internal/* path in a Devvit Web application are not automatically restricted to platform invocations and are publicly reachable HTTP endpoints [1]. While the Devvit platform uses these /internal/* endpoints to invoke app-specific logic—such as menu actions, form submissions, triggers, and scheduled tasks—they are exposed as standard HTTP routes on your server [2][3][4][5]. Because they are reachable via HTTP, any user or external actor capable of reaching your app's server URL can theoretically attempt to hit these endpoints [1]. Security best practices for Devvit applications dictate that you must manually implement authentication and authorization checks within your server-side handlers to ensure they are only executed by authorized users or the platform itself [1]. For example, if a menu action is intended for moderators only, the handler must explicitly verify the requester's permissions (e.g., calling a requireModerator function) rather than relying on the route's path or the app's configuration [1]. Relying solely on the platform's configuration or the route name for security is insufficient [1].
Citations:
- 1: StephenSook/context-mod-devvit#34
- 2: reddit/devvit-docs@411421f
- 3: https://github.com/StephenSook/context-mod-devvit/blob/main/API.md
- 4: https://github.com/chrismaz11/devvit-expert-skill
- 5: https://github.com/Beach-Brews/devvit-daily-post-example
Add an explicit authz check before creating the post.
apps/target-devvit/src/server/index.ts:21-63is reachable over HTTP, so anyone who can hit this endpoint can triggerreddit.submitCustomPost()unless the handler validates that the caller is an authorized menu invocation/moderator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/target-devvit/src/server/index.ts` around lines 21 - 63, The /internal/menu/create-post handler in app.post is exposed over HTTP and currently calls reddit.submitCustomPost without verifying the caller is authorized. Add an explicit authorization gate before the create-post logic runs, using the request context available in this route and the existing currentSubredditName check to ensure only valid menu invocations or moderators can proceed. If the caller is not authorized, return a neutral UiResponse and skip the reddit.submitCustomPost call. Keep the existing success and error handling in the same handler after the authz check.
446-458: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@devvit/web redis SetOptions expiration field: does it accept a Date object or a duration in seconds/ms?💡 Result:
In the Devvit Redis client, key expiration is handled using a duration in seconds, not a Date object [1]. When setting an expiration for a key, the
expiremethod specifically expects a number representing the duration in seconds (e.g.,await redis.expire(key, 3600)for one hour) [1]. Regarding thesetoperation, while the standard Devvit Redis interface provides aset(key: string, value: string, options?: SetOptions)method [1], the underlying implementation typically uses integer values for expiration arguments (often representing seconds) [2]. There is no indication that the DevvitSetOptionssupports aDateobject for expiration [1]. Developers should rely on providing numeric durations in seconds to meet theexpirerequirements [1].Citations:
- 1: https://context7.com/reddit/devvit/llms.txt
- 2: https://github.com/reddit/devvit/blob/main/packages/redis/src/test/mocks/RedisMock.ts
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate Redis SetOptions and related expiration handling in the repository. git ls-files | rg -n '(^|/)(redis|Redis|setOptions|SetOptions|expiration)\.(ts|tsx|js|jsx|md)$|(^|/)server/index\.ts$' || true echo '--- SEARCH: SetOptions definitions and expiration usage ---' rg -n "interface SetOptions|type SetOptions|expiration\b|redis\.set\(" apps packages . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || trueRepository: imjlk/mpgd-kit
Length of output: 857
🌐 Web query:
Devvit Redis SetOptions expiration Date set(key value options) type definition documentation💡 Result:
In the Reddit Devvit framework, the redis.set method is used to store string values, and it supports optional parameters—including an expiration time—via an options object [1]. Based on implementation patterns in the Devvit Redis package, the set method signature generally follows this structure: await redis.set(key, value, options?); The options object supports an expiration field, which defines the time-to-live (TTL) for the key in seconds [1]. Example usage: await redis.set('myKey', 'myValue', { expiration: 60 }); // Key expires in 60 seconds [1] For managing expiration separately or updating it after a key has been created, Devvit also provides a dedicated expire method [2]: await redis.expire(key, seconds); Key technical details: - Expiration: When provided in the set options, the expiration value is interpreted in seconds [1]. - Alternatives: If you need to set expiration conditions (such as NX or XX, which are common in standard Redis), the current Devvit API usage commonly utilizes the explicit expire method for lifecycle management [2] or standard key-value setting [1]. While the internal Redis mocks in the Devvit codebase reference nx and xx flags, the primary documented approach for developers is to use the provided Redis client methods (set and expire) as exposed in the Devvit context [2][1]. Always ensure you are importing or accessing redis through the Devvit context (e.g., const { redis } = context) to ensure it is correctly scoped to your installation [2].
Citations:
- 1: https://www.reddit.com/r/Devvit/comments/1tjh0tg/three_things_i_learned_building_a_realtime_mod/
- 2: https://context7.com/reddit/devvit/llms.txt
🌐 Web query:
site:github.com/reddit/devvit "interface SetOptions" redis expiration Date seconds💡 Result:
In the context of the Reddit Devvit framework's Redis client implementation, SetOptions is an interface used within the Redis abstraction [1]. When using this client, expiration for a value is typically set by providing a numeric value for the expiration property (representing seconds) in the request object [1]. For example, the internal mock implementation handles expiration as follows: if (request.expiration && request.expiration > 0) { args.push('EX', request.expiration); } The Devvit Redis client uses standard Redis commands (such as SET with EX options) [1], where the expiration duration is specified in seconds. While the framework supports standard
Dateobjects in other areas of its API (such as scheduling jobs withrunAt[2]), Redis expiration within theSetOptionspattern relies on a numeric integer representing seconds, not aDateobject [1].Citations:
- 1: https://github.com/reddit/devvit/blob/main/packages/redis/src/test/mocks/RedisMock.ts
- 2: reddit/devvit#259
🌐 Web query:
site:github.com/reddit/devvit packages/redis/src/test/mocks/RedisMock.ts expiration number EX request.expiration💡 Result:
In the Reddit Devvit project's Redis mock implementation located at packages/redis/src/test/mocks/RedisMock.ts, the logic for handling key expiration during set operations is implemented as follows [1]: if (request.expiration && request.expiration > 0) { args.push('EX', request.expiration); } This code snippet checks if a valid expiration value (a number greater than 0) exists in the request object [1]. If it does, it appends the 'EX' command and the corresponding expiration value to the arguments array, effectively simulating the Redis EXPIRE behavior for the mocked command [1].
Citations:
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for the Redis client type and any expiration examples/usages. rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \ "expiration:\s*|interface\s+SetOptions|type\s+SetOptions|redis\.set\(" . echo '--- potential Devvit redis package files ---' fd -a 'RedisMock.ts|redis.*ts|SetOptions.*ts|index.d.ts' . || trueRepository: imjlk/mpgd-kit
Length of output: 501
Use a numeric TTL for
expiration. Devvit Redissetexpects the expiration value in seconds (EX), so passing aDatehere can set the lock to the wrong lifetime or prevent it from expiring as intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/target-devvit/src/server/index.ts` around lines 446 - 458, The leaderboard lock setup in acquireLeaderboardLock currently passes a Date from leaderboardLockExpirationDate into redis.set, but Devvit Redis expects a numeric TTL in seconds for expiration. Update the lock acquisition flow to pass the TTL as a number (using leaderboardLockTtlSeconds directly or an equivalent numeric helper) and remove the Date-based helper so the lock lifetime is applied correctly.
533-547: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@devvit/web server context object: does it expose userId and username fields directly, or is a separate Reddit API call required to get the current username?💡 Result:
In the Devvit framework, the context object does not expose username information directly. While
context.userIdis available on the context object (and is present only when a user is logged in), the username is not [1]. To retrieve the current user's username, you must perform an asynchronous call to the Reddit API usingcontext.reddit.getCurrentUsername()[2][3]. The pattern typically involves checking the current user's identity only when needed, as shown in the following example: const username = await context.reddit.getCurrentUsername; if (username) { // Use the username }Citations:
- 1: https://developers.reddit.com/docs/guides/logged-out-users
- 2: https://github.com/1ennyTM/devvit-local-dev-template
- 3: https://context7.com/reddit/devvit/llms.txt
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target file around the reported lines. sed -n '500,580p' apps/target-devvit/src/server/index.ts # Find where `context` is typed or imported in this file. rg -n "context|username|userId|currentPlayerId|currentDisplayName|reddit.getCurrentUsername" apps/target-devvit/src/server/index.ts # Search the repo for explicit Devvit context typing and username access patterns. rg -n "getCurrentUsername|context\.username|context\.userId|readonly username\?:|readonly userId\?:|Context" apps/target-devvit/src -g '!**/dist/**' -g '!**/build/**'Repository: imjlk/mpgd-kit
Length of output: 3384
Replace
context.usernamewithcontext.reddit.getCurrentUsername()
context.userIdis fine, butcontext.usernameisn’t a server-context field. As written,currentDisplayName()will fall back toplayerId, soidentity.getPlayercan only return a real display name if you fetch it asynchronously fromcontext.reddit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/target-devvit/src/server/index.ts` around lines 533 - 547, `currentDisplayName()` is reading a server-context `username` field that does not exist, so it always falls back instead of returning the real display name. Update the display-name lookup in `currentDisplayName` to use `context.reddit.getCurrentUsername()` and make the function async if needed, then adjust the `identity.getPlayer` call path to await this lookup and pass the fetched username through instead of relying on `context.username`.tools/smoke/embedded-target-config.ts (1)
131-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
unzip exit status codes meaning warning error severe💡 Result:
The unzip utility uses specific exit status codes to indicate the outcome of an operation. On most systems (excluding VMS), these codes generally approximate the standards defined by PKWARE [1][2][3]. Common exit status codes include: 0: The operation was successful [4][5]. 1: One or more files were processed successfully, but some errors or warnings occurred [4][5]. 2: A generic error in the zipfile format was detected; processing may have completed successfully, but a warning was generated [4][5]. 3: A severe error in the zipfile format was detected; processing likely failed immediately [4][5]. 4: Unable to allocate memory for buffers during initialization [4][5]. 5: Unable to allocate memory or obtain a TTY to read a decryption password [4][5]. 6: Unable to allocate memory during decompression to disk [4][5]. 9: The specified zipfiles were not found [4][5]. 10: Invalid options were specified on the command line [6][4][5]. 11: No matching files were found [6][4][5]. 50: The disk is, or was, full during extraction [6][4][5]. 51: The end of the ZIP archive was encountered prematurely [6][4][5]. 80: The user aborted the operation (e.g., via Ctrl+C) [6][4][5]. 81: Testing or extraction failed due to unsupported compression methods or decryption [6][4][5]. 82: No files were found due to an incorrect decryption password [4][5]. Note that some distributions or compilation configurations may provide additional human-readable explanations if the RETURN_CODES option is defined [1][2][3]. VMS systems map these standard codes to specific VMS-style status codes [1][2][3].
Citations:
- 1: https://man.archlinux.org/man/extra/unzip/unzip.1.en
- 2: https://manpages.debian.org/bookworm/unzip/unzip.1.en.html
- 3: https://manpages.ubuntu.com/manpages/jammy/man1/unzip.1.html
- 4: https://qnx.com/developers/docs/7.1/com.qnx.doc.neutrino.utilities/topic/u/unzip.html
- 5: https://manpages.opensuse.org/Leap-15.6/unzip-doc/unzip.1.en.html
- 6: https://www.linux.org/docs/man1/unzip.html
🏁 Script executed:
sed -n '1,220p' tools/smoke/embedded-target-config.tsRepository: imjlk/mpgd-kit
Length of output: 4031
Zip failures need to be gated on the exit code, not stdout
unzipcan emit partial output before fatal failures, sostatus !== 0 && stdout.length > 0still lets corrupt archives, disk-full, EOF, abort, and decryption failures slip through. Narrow this to the specific benign warning codes you intend to tolerate.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/smoke/embedded-target-config.ts` around lines 131 - 137, The failure check in the command execution path is gating on stdout instead of the exit code, which can miss real unzip errors. Update the error handling around the command/result check in embedded-target-config.ts to treat nonzero status as failure by default and only allow the specific benign warning exit codes you intentionally want to ignore; use the existing result.status, result.stdout, and result.stderr handling in this block to narrow the tolerated cases.
|
@codex review Patched the post-merge PR #3 review follow-up in
Validation run:
Current PR state: clean and mergeable. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Validation
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation