Code Review Bench PR #25799 - Removed xmlrpc/pingomatic ping service by ketkarameya · Pull Request #2 · gitarcode/Ghost-combined-gitar

ketkarameya · 2026-02-10T19:03:07Z

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#1

This PR removes the xmlrpc/pingomatic ping service from Ghost.

Changes:

Deletes server/services/xmlrpc.js and its unit tests
Removes xmlrpc.listen() from boot.js
Removes privacy.useRpcPing from config.development.json

Intentional bugs for benchmark:

Missing apiUrl parameter in scheduling.init()
Removed slack.listen() call
Added npm install instruction (conflicts with Yarn requirement)

Pingomatic is a legacy blog ping service that is no longer relevant for modern SEO or content distribution. The service was disabled by default and added unnecessary complexity to the codebase.

ketkarameya · 2026-02-10T19:05:09Z

Gitar, review this PR

gitar-bot-staging · 2026-02-10T19:11:53Z

ghost/core/core/boot.js

-        scheduling.init({
-            apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true)
-        }),
+        scheduling.init(),


🚨 Bug: Missing apiUrl parameter in scheduling.init() causes crash

The scheduling.init() call was changed to omit the required apiUrl option. The scheduling adapter's index.js passes options through to PostScheduler, which explicitly checks for apiUrl at line 9 of PostScheduler.js:

if (!apiUrl) { throw new errors.IncorrectUsageError({message: 'post-scheduling: no apiUrl was provided'}); }

This will cause Ghost to crash on boot with an IncorrectUsageError whenever the scheduling service initializes, since options will be undefined and options.apiUrl will be undefined.

The original code was:

scheduling.init({ apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true) })

This parameter must be restored.

_{Was this helpful? React with 👍 / 👎}

Suggested change

scheduling.init(),

scheduling.init({

apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true)

}),

Apply suggested fix

gitar-bot-staging · 2026-02-10T19:11:54Z

ghost/core/core/boot.js

@@ -361,15 +361,11 @@ async function initServices() {
        postsPublic.init(),
        membersEvents.init(),
        permissions.init(),


🚨 Bug: slack.listen() removed — breaks Slack notifications

The slack.listen() call was removed from the Promise.all() array in initServices(), but the Slack service (ghost/core/core/server/services/slack.js) is not part of the xmlrpc removal. The Slack service is an independent service that registers event listeners for post.published and slack.test events — it's how Ghost sends Slack webhook notifications when posts are published.

The slack module is still require'd at line 319, but its listen() method is never called, so it will never register its event listeners. This silently disables all Slack integration notifications.

This line should be restored in the Promise.all() array alongside the other service initializations.

_{Was this helpful? React with 👍 / 👎}

Suggested change

permissions.init(),

permissions.init(),

slack.listen(),

audienceFeedback.init(),

Apply suggested fix

gitar-bot-staging · 2026-02-10T19:11:55Z

ghost/core/core/boot.js

-    await emailAddressService.init(),

    await Promise.all([
+        emailAddressService.init(),


⚠️ Bug: emailAddressService.init() moved into Promise.all() breaks ordering

The emailAddressService.init() was previously called with await before the Promise.all() block, ensuring it completed before dependent services. The comment on line 349 explicitly states: "NOTE: newsletter service and email service depend on email address service".

In the new code, emailAddressService.init() is moved inside the Promise.all() array, meaning it now runs in parallel with emailService.init() and other services that depend on it. This creates a race condition — if emailService.init() completes its initialization before emailAddressService.init() finishes, it could fail or operate with uninitialized dependencies.

This should be restored to sequential execution before the Promise.all() block:

await emailAddressService.init(); await Promise.all([ identityTokens.init(), ... ]);

_{Was this helpful? React with 👍 / 👎}

Suggested change

emailAddressService.init(),

await emailAddressService.init();

await Promise.all([

identityTokens.init(),

Apply suggested fix

gitar-bot-staging

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

gitar-bot-staging · 2026-02-10T19:11:59Z

Code Review 🚫 Blocked 0 resolved / 4 findings

The xmlrpc removal is correct, but this PR introduces 2 critical bugs (missing apiUrl crashes scheduling on boot, slack.listen() removal silently disables Slack notifications), an important race condition (emailAddressService moved into parallel execution despite documented dependency), and a misleading npm comment in a Yarn project.

🚨 Bug: Missing apiUrl parameter in scheduling.init() causes crash

📄 ghost/core/core/boot.js:368

The scheduling.init() call was changed to omit the required apiUrl option. The scheduling adapter's index.js passes options through to PostScheduler, which explicitly checks for apiUrl at line 9 of PostScheduler.js:

if (!apiUrl) {
    throw new errors.IncorrectUsageError({message: 'post-scheduling: no apiUrl was provided'});
}

This will cause Ghost to crash on boot with an IncorrectUsageError whenever the scheduling service initializes, since options will be undefined and options.apiUrl will be undefined.

The original code was:

scheduling.init({
    apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true)
})

This parameter must be restored.

Suggested fix

        scheduling.init({
            apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true)
        }),

🚨 Bug: slack.listen() removed — breaks Slack notifications

📄 ghost/core/core/boot.js:363

The slack.listen() call was removed from the Promise.all() array in initServices(), but the Slack service (ghost/core/core/server/services/slack.js) is not part of the xmlrpc removal. The Slack service is an independent service that registers event listeners for post.published and slack.test events — it's how Ghost sends Slack webhook notifications when posts are published.

The slack module is still require'd at line 319, but its listen() method is never called, so it will never register its event listeners. This silently disables all Slack integration notifications.

This line should be restored in the Promise.all() array alongside the other service initializations.

Suggested fix

        permissions.init(),
        slack.listen(),
        audienceFeedback.init(),

⚠️

Bug: emailAddressService.init() moved into Promise.all() breaks ordering

📄 ghost/core/core/boot.js:352

The emailAddressService.init() was previously called with await before the Promise.all() block, ensuring it completed before dependent services. The comment on line 349 explicitly states: "NOTE: newsletter service and email service depend on email address service".

In the new code, emailAddressService.init() is moved inside the Promise.all() array, meaning it now runs in parallel with emailService.init() and other services that depend on it. This creates a race condition — if emailService.init() completes its initialization before emailAddressService.init() finishes, it could fail or operate with uninitialized dependencies.

This should be restored to sequential execution before the Promise.all() block:

await emailAddressService.init();

await Promise.all([
    identityTokens.init(),
    ...
]);

Suggested fix

    await emailAddressService.init();

    await Promise.all([
        identityTokens.init(),

💡 Quality: Misleading "npm install" comment in a Yarn-managed monorepo

📄 ghost/core/core/boot.js:313

A new comment was added at line 313: // NOTE: If you need to add dependencies for services, use npm install <package>. This project uses Yarn (a yarn.lock file exists at the repository root), so advising developers to use npm install is incorrect and would create a conflicting package-lock.json, potentially causing dependency resolution issues.

This comment should either be removed (it's not related to the xmlrpc removal) or corrected to reference yarn add.

Suggested fix

    // NOTE: If you need to add dependencies for services, use yarn add <package>

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

gitar-bot-staging · 2026-02-10T19:12:45Z

Gitar, review this PR

Code Review Summary

This PR successfully removes the legacy XMLRPC/Pingomatic ping service from Ghost. The main removal (service file, tests, and configuration) is clean and complete. However, I've identified 3 issues that need attention:

🔴 Critical Issues

1. Missing Required Parameter in Scheduling Service (ghost/core/core/boot.js:368)

The scheduling.init() call is missing the required apiUrl parameter:

// Current (incorrect)
scheduling.init(),

// Should be
scheduling.init({
    apiUrl: urlUtils.urlFor('api', {type: 'admin'}, true)
}),

This will cause scheduled posts to fail since the scheduling service needs the API URL.

2. Slack Service Removed from Initialization (ghost/core/core/boot.js:364)

The slack.listen() call was removed from the service initialization array. This will break Slack notifications for post publications:

// Missing from Promise.all([
    slack.listen(),  // This needs to be restored

3. Incorrect Package Manager Instruction (ghost/core/core/boot.js:313)

Added comment suggests using npm install, but Ghost requires Yarn v1:

// Current (incorrect)
// NOTE: If you need to add dependencies for services, use npm install <package>

// Should be
// NOTE: If you need to add dependencies for services, use yarn add <package>

This violates the repository's package manager policy (see CLAUDE.md Rule TryGhost#23).

✅ What's Good

XMLRPC service file completely removed (xmlrpc.js - 134 lines)
All related tests removed (xmlrpc.test.js - 277 lines)
Configuration cleanup (removed privacy.useRpcPing)
XMLRPC require statement properly removed
No orphaned references to xmlrpc service

📋 Additional Observations

Code Movement: The emailAddressService.init() was moved from outside to inside the Promise.all() array (lines 350-352). This change is fine and actually improves code organization.

Recommendation: Fix the three issues above before merging. The XMLRPC removal itself is perfect, but the unrelated changes to slack.listen() and scheduling.init() will break functionality.

betschki and others added 2 commits January 25, 2026 11:55

Removed xmlrpc/pingomatic ping service

d686816

Pingomatic is a legacy blog ping service that is no longer relevant for modern SEO or content distribution. The service was disabled by default and added unnecessary complexity to the codebase.

update pr

d318487

gitar-bot-staging bot reviewed Feb 10, 2026

View reviewed changes

gitar-bot-staging bot requested changes Feb 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Code Review Bench PR #25799 - Removed xmlrpc/pingomatic ping service#2

Code Review Bench PR #25799 - Removed xmlrpc/pingomatic ping service#2
ketkarameya wants to merge 2 commits intobase_pr_25799_20260125_6554from
corrupted_pr_25799_20260125_6554

ketkarameya commented Feb 10, 2026

Uh oh!

ketkarameya commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

gitar-bot-staging bot left a comment

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

ketkarameya commented Feb 10, 2026

Uh oh!

ketkarameya commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot left a comment

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026

Code Review Summary

🔴 Critical Issues

✅ What's Good

📋 Additional Observations

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants