Added X integration for automatic post sharing#27366
Open
doronkatz wants to merge 6 commits intoTryGhost:mainfrom
Open
Added X integration for automatic post sharing#27366doronkatz wants to merge 6 commits intoTryGhost:mainfrom
doronkatz wants to merge 6 commits intoTryGhost:mainfrom
Conversation
These repo-local skills mirror the upstream Symphony skill set so the Ghost repo can use the same commit, debug, land, linear, pull, and push workflows checked into version control.
github-actions
bot
added
the
migration
Contributor
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
Contributor
WalkthroughThe pull request adds comprehensive agent skill definitions and an end-to-end automated workflow orchestration framework, alongside a complete X (Twitter) integration for Ghost. New agent skills document structured Git workflows (commit, pull, push), PR merging (land), debugging, Linear issue tracking, and app launching. A WORKFLOW.md file defines state-based automation for Linear tickets. The X integration includes backend OAuth 1.0a authentication, auto-post-to-X functionality on post publication, settings management, API endpoints, database schema extensions, and frontend UI components. Supporting changes include test coverage, migrations, serializer updates, configuration defaults, and a Python-based PR watcher utility for concurrent CI and review monitoring. 🚥 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 |
Author
|
Addressing the migration checklist:
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4b1497b. Configure here.
| authors: model.related('authors').toJSON() | ||
| }).catch((error) => { | ||
| logging.error(error); | ||
| }); |
There was a problem hiding this comment.
Per-post X opt-out never works at runtime
High Severity
The xListener calls model.toJSON() on the Bookshelf post model, which serializes posts_meta as a nested object (e.g., {posts_meta: {x_post_enabled: false}}). But publishPost checks post.x_post_enabled === false at the top level, which will always be undefined. Since undefined === false is false, the per-post opt-out check never triggers, and posts are always auto-published to X even when the user explicitly disabled it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4b1497b. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b1497b668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return; | ||
| } | ||
|
|
||
| if (post.x_post_enabled === false) { |
There was a problem hiding this comment.
The per-post toggle is persisted in posts_meta.x_post_enabled, but this guard checks post.x_post_enabled at the top level. In the same file, xListener forwards model.toJSON() to publishPost, which keeps metadata nested under posts_meta, so this condition is typically undefined !== false and the skip never triggers. In practice, posts that authors explicitly disabled for X auto-posting can still be published to X when the integration is connected.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
ghost/admin/tests/acceptance/editor-test.js-388-391 (1)
388-391:⚠️ Potential issue | 🟡 MinorTighten the initial
xPostEnabledassertion.
to.not.equal(false)also passes when the field isundefinedornull, so this test would miss a broken default or missing serialization. Asserttrueexplicitly, or check the checkbox state instead.Suggested fix
expect( this.server.db.posts.find(post.id).xPostEnabled, 'x posting starts enabled' - ).to.not.equal(false); + ).to.equal(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/acceptance/editor-test.js` around lines 388 - 391, The assertion for xPostEnabled is too loose (it allows undefined/null); update the expectation on this.server.db.posts.find(post.id).xPostEnabled to assert true explicitly (use expect(...).to.equal(true)) or alternatively assert the UI checkbox state tied to xPostEnabled to ensure the default is serialized correctly; adjust the test in editor-test.js where the current expect(...).to.not.equal(false) is used so it verifies a concrete true value.ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js-56-70 (1)
56-70:⚠️ Potential issue | 🟡 MinorDon't delete settings that this migration may not have created.
up()explicitly skips any pre-existingx_*row, butdown()deletes every matching key unconditionally. That makes rollback non-symmetric and can remove manually provisioned data that predates this migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js` around lines 56 - 70, The down() migration currently deletes any matching x_* key unconditionally; change it to only remove settings that this migration actually created by comparing the existing row's value to the expected value from X_SETTINGS before deleting. In the down function, for each setting in X_SETTINGS check existingSetting.value === setting.value (or another unique marker set by up) and only call .del() when it matches; otherwise log and skip to avoid removing pre-existing/manual settings.apps/admin-x-settings/test/acceptance/advanced/integrations/x.test.ts-101-104 (1)
101-104:⚠️ Potential issue | 🟡 MinorWait for the disconnect request before asserting it fired.
lastApiRequests.disconnectXis populated asynchronously after the confirmation click, so this direct assertion can race and intermittently fail.💡 Suggested fix
- expect(lastApiRequests.disconnectX).toBeTruthy(); + await expect.poll(() => lastApiRequests.disconnectX).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/advanced/integrations/x.test.ts` around lines 101 - 104, The assertion on lastApiRequests.disconnectX races because it's set asynchronously after the confirmation click; after calling xModal.getByRole(...).click() and page.getByTestId('confirmation-modal').getByRole(...).click(), wait for lastApiRequests.disconnectX to be populated before asserting (e.g., use a short polling loop or test helper like waitFor that repeatedly checks lastApiRequests.disconnectX until truthy, or await a page.waitForResponse matching the X disconnect endpoint), then assert expect(lastApiRequests.disconnectX).toBeTruthy(); reference the existing symbols lastApiRequests.disconnectX, xModal.getByRole, and page.getByTestId('confirmation-modal').getByRole when implementing the wait..agents/skills/land/land_watch.py-577-582 (1)
577-582:⚠️ Potential issue | 🟡 MinorDocument exit code
5before callers depend on this watcher contract.This script now exits with
SystemExit(5)for merge conflicts, but.agents/skills/land/SKILL.md:110-135only documents exit codes 2, 3, and 4. Anything branching on the documented outcomes will treat merge conflicts as an unknown failure.Also applies to: 591-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/land/land_watch.py around lines 577 - 582, The watcher now raises SystemExit(5) when is_merge_conflicting(pr) in land_watch.py (also the same pattern at the second occurrence around the other raise), so update .agents/skills/land/SKILL.md to document exit code 5 with a brief description (e.g., "exit code 5 — PR has merge conflicts; resolve/rebase against main and push") and ensure callers that rely on documented outcomes know to treat 5 as a handled/expected state; verify both occurrences of SystemExit(5) are covered in the documentation..codex/skills/commit/SKILL.md-60-75 (1)
60-75:⚠️ Potential issue | 🟡 MinorAdd a language to the fenced template block.
This new file currently fails
markdownlint(MD040) because the commit-message example fence is untyped.🧹 Proposed fix
-``` +```text <type>(<scope>): <short summary> Summary: - <what changed> @@ Co-authored-by: Codex <codex@openai.com></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.codex/skills/commit/SKILL.md around lines 60 - 75, The fenced code block
for the commit-message template is missing a language identifier causing
markdownlint MD040; update the opening triple-backtick of the example block (the
block that starts withand contains the commit template with "Summary:", "Rationale:", "Tests:", and "Co-authored-by:") to include a language such as "text" (i.e., change the openingto ```text) so the fence is typed and MD040
is resolved.</details> </blockquote></details> <details> <summary>.agents/skills/pull/SKILL.md-24-24 (1)</summary><blockquote> `24-24`: _⚠️ Potential issue_ | _🟡 Minor_ **Document minimum Git version requirement (2.35) for `merge.conflictstyle=zdiff3`.** The `zdiff3` conflict style (line 28) requires Git 2.35+, which is not documented in this skill. While the repository supports this version, the skill should explicitly state the minimum Git version required for reliable execution, particularly for the more recent `zdiff3` feature that was introduced in January 2022. Consider adding a prerequisites section noting Git 2.35+ is required, or providing a fallback merge strategy for older Git versions. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/pull/SKILL.md at line 24, Update the SKILL.md to document the minimum Git version required for the zdiff3 conflict style: state that Git 2.35+ is required (zdiff3 introduced Jan 2022) and add this to a "Prerequisites" section; reference the existing mention of merge.conflictstyle=zdiff3 so readers see the requirement, and optionally add guidance for a fallback merge strategy or configuration for older Git versions (e.g., alternate conflict style) so the skill can run reliably on older environments. ``` </details> </blockquote></details> <details> <summary>.codex/skills/land/SKILL.md-110-115 (1)</summary><blockquote> `110-115`: _⚠️ Potential issue_ | _🟡 Minor_ **Document exit code 5 too.** The watcher also exits with code 5 when the PR is merge-conflicting, so this table is currently incomplete. Any caller that only maps 2/3/4 will mis-handle the conflict path. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.codex/skills/land/SKILL.md around lines 110 - 115, The Exit codes table is missing an entry for the watcher exit code 5; update the "Exit codes:" list in SKILL.md to include "5: PR merge-conflicting" (or similar wording like "5: PR merge-conflicting (merge conflict detected)") so callers mapping exit codes handle the conflict path; ensure the new entry follows the existing format alongside 2/3/4 in the same section. ``` </details> </blockquote></details> <details> <summary>.agents/skills/land/SKILL.md-110-115 (1)</summary><blockquote> `110-115`: _⚠️ Potential issue_ | _🟡 Minor_ **Document exit code 5 too.** The watcher also exits with code 5 when the PR is merge-conflicting, so this table is incomplete for anyone automating against it. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/land/SKILL.md around lines 110 - 115, Update the Exit codes list in SKILL.md to include the missing code 5: add an entry such as "- 5: PR merge-conflicting (watcher detected merge conflict)" so the table documents the watcher exit when a PR is merge-conflicting; modify the existing Exit codes section (the list containing codes 2, 3, 4) to insert this new line and keep formatting consistent with the other entries. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>apps/admin-x-settings/src/components/settings/advanced/integrations/x-modal.tsx (1)</summary><blockquote> `6-21`: **Avoid forcing these settings to `string` during destructuring.** `getSettingValues<string>()` hides the fact that both keys can be `null` at runtime, so malformed or missing settings still type-check as strings here. Read them with runtime `typeof === 'string'` guards before deriving `isConnected`. <details> <summary>💡 Suggested refactor</summary> ```diff -import {getSettingValues, useDeleteXSettings, useVerifyXSettings} from '@tryghost/admin-x-framework/api/settings'; +import {getSettingValue, useDeleteXSettings, useVerifyXSettings} from '@tryghost/admin-x-framework/api/settings'; @@ - const [xAccessToken, xUsername] = getSettingValues<string>(settings, ['x_access_token', 'x_username']); + const rawAccessToken = getSettingValue(settings, 'x_access_token'); + const rawUsername = getSettingValue(settings, 'x_username'); + const xAccessToken = typeof rawAccessToken === 'string' ? rawAccessToken : null; + const xUsername = typeof rawUsername === 'string' ? rawUsername : null; ``` </details> Based on learnings, "avoid unsafe type assertions for destructuring settings values... perform runtime type checks, and apply sensible defaults." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/advanced/integrations/x-modal.tsx` around lines 6 - 21, The code currently forces settings to string via getSettingValues<string>(...) and then derives isConnected from xAccessToken/xUsername, which can mask null values; change the call to getSettingValues without the <string> generic and treat the returned xAccessToken and xUsername as possibly null/unknown, perform runtime typeof checks (e.g. typeof xAccessToken === 'string' && typeof xUsername === 'string') before computing isConnected, and apply sensible defaults or null-handling where xAccessToken, xUsername, or isConnected are used (refer to getSettingValues, xAccessToken, xUsername, and isConnected to locate and update the logic). ``` </details> </blockquote></details> <details> <summary>.agents/skills/pull/SKILL.md (2)</summary><blockquote> `23-26`: **Consider adding error handling guidance for fast-forward pull failures.** Step 5 uses `git pull --ff-only`, which will fail if the remote branch has diverged from the local branch (e.g., force-pushed or has conflicting commits). Consider adding guidance on what to do when this command fails. <details> <summary>📝 Suggested addition for error handling</summary> Add after line 26: ```diff before merging `origin/main`. + - If `--ff-only` fails (diverged history), verify the remote changes are + safe, then either: + - Use `git pull --rebase origin $(git branch --show-current)`, or + - Reset to the remote: `git reset --hard origin/$(git branch --show-current)` + (only if local commits should be discarded). 6. Merge in order: ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/pull/SKILL.md around lines 23 - 26, Update the SKILL.md step that uses "git pull --ff-only" to include error-handling guidance: detect a non-zero exit from git pull --ff-only, run git fetch and inspect divergence with git status or git log --left-right --graph origin/$(git branch --show-current)...HEAD, then describe safe recovery options (rebase onto origin, merge, or reset --hard if force-pushed) and remind to back up local changes (stash or create a temp branch) before destructive commands; reference the existing step text ("git pull --ff-only origin $(git branch --show-current)") so reviewers can insert a short paragraph explaining these diagnostics and the recommended actions. ``` </details> --- `28-29`: **Document Git version requirement or provide fallback for `zdiff3` conflict style.** The `zdiff3` conflict style was introduced in Git 2.35 (January 2022). Since SKILL.md is used across various deployment environments, consider either: - Adding a note that `zdiff3` requires Git 2.35+ (verified to work with Git 2.39+), or - Providing a fallback command: `git -c merge.conflictstyle=diff3 merge origin/main` for older versions. This ensures agents using this skill can adapt if their environment uses an older Git version. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/pull/SKILL.md around lines 28 - 29, Update SKILL.md to document that the git merge.conflictstyle value "zdiff3" requires Git 2.35+ (note that it was verified with 2.39+) and add a fallback suggestion using "diff3" for older Git versions; specifically mention the example command shown (`git -c merge.conflictstyle=zdiff3 merge origin/main`) and provide the alternative (`git -c merge.conflictstyle=diff3 merge origin/main`) so users without zdiff3 know which command to run or to upgrade Git. ``` </details> </blockquote></details> <details> <summary>ghost/core/test/unit/server/api/endpoints/utils/serializers/output/posts.test.js (1)</summary><blockquote> `13-16`: **Refactor rewire cleanup to prevent test pollution.** `sinon.restore()` only resets Sinon fakes; it does not restore the module-level bindings changed by `postsMapper.__set__`. While this file currently has only one test, the pattern should be fixed to prevent future tests from inheriting mocked `url`, `extraAttrs`, `clean`, and `utils`. Use `__set__` with an object to capture the revert function, and call it in a `finally` block: <details> <summary>Suggested refactor</summary> ```diff it('preserves false values for x_post_enabled', async function () { - postsMapper.__set__('url', {forPost: sinon.stub()}); - postsMapper.__set__('extraAttrs', {forPost: sinon.stub()}); - postsMapper.__set__('clean', {post: sinon.stub()}); - postsMapper.__set__('utils', {isContentAPI: () => false}); - - const result = await postsMapper({ - id: 'post_1', - email_recipient_filter: null, - posts_meta: { - x_post_enabled: false - } - }, { - options: {} - }, { - tiers: [] - }); - - assert.equal(result.x_post_enabled, false); + const restoreRewire = postsMapper.__set__({ + url: {forPost: sinon.stub()}, + extraAttrs: {forPost: sinon.stub()}, + clean: {post: sinon.stub()}, + utils: {isContentAPI: () => false} + }); + + try { + const result = await postsMapper({ + id: 'post_1', + email_recipient_filter: null, + posts_meta: { + x_post_enabled: false + } + }, { + options: {} + }, { + tiers: [] + }); + + assert.equal(result.x_post_enabled, false); + } finally { + restoreRewire(); + } }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/api/endpoints/utils/serializers/output/posts.test.js` around lines 13 - 16, Replace the current multiple postsMapper.__set__ calls with a single call that captures the revert function (e.g., const revert = postsMapper.__set__({url: ..., extraAttrs: ..., clean: ..., utils: ...})), then ensure you call revert() in a finally block (alongside sinon.restore()) so the module-level bindings are restored after the test; target the setup where postsMapper.__set__ is currently used and wrap the test execution in try/finally to call revert() to avoid polluting other tests. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/commit/SKILL.md:
- Around line 29-34: Replace the risky instruction that tells users to "Stage
intended changes, including new files (git add -A)" with guidance to stage only
intended changes—e.g., recommend interactive or path-specific staging (replace
the phrase "git add -A" in that line) and update surrounding steps so step 4
instructs checking staged changes (git status/git diff --staged) before
committing and step 5 advises amending the index or asking for confirmation if
unrelated files are present; ensure the SKILL.md text explicitly suggests safer
alternatives to git add -A and clarifies the workflow for selective commits.In @.agents/skills/debug/SKILL.md:
- Around line 53-55: The current rg command (rg -o "session_id=[^ ;]+"
log/symphony.log*) scans all logs and can return unrelated sessions; change it
to pipe from the ticket-specific filtered output instead of scanning
log/symphony.log* directly — i.e., reuse the prior ticket-filtered match stream
(the earlier rg/filter that narrows to the ticket) as the input and then extract
session IDs by applying the session_id regex (the "session_id=[^ ;]+" pattern)
and sort -u; update the step that references rg -o "session_id=[^ ;]+" to
operate on the ticket-filtered lines rather than all log files.In @.agents/skills/land/land_watch.py:
- Around line 306-327: The code computes a latest review-request cutoff
(latest_review_request_at) but never applies it when evaluating human feedback,
so older unresolved feedback can still trigger raise_on_human_feedback; update
filter_human_issue_comments and also apply the same cutoff in
filter_codex_review_issue_comments, filter_human_review_comments, and
is_blocking_review so they ignore comments/reviews with comment_time() (or
created_time) <= latest_review_request_at() before raise_on_human_feedback is
called; ensure you reference and use latest_review_request_at() (or pass
latest_ack) as the gating value and skip items earlier than that in those
functions (e.g., inside filter_human_issue_comments,
filter_codex_review_issue_comments, filter_human_review_comments, and
is_blocking_review) so only activity after the most recent "@codex review" is
considered.In @.agents/skills/push/SKILL.md:
- Around line 47-55: The documented PR-body flow is a no-op because the template
path and GH CLI invocation are wrong: update references from
.github/pull_request_template.mdto the actual file
.github/PULL_REQUEST_TEMPLATE.md(or rename the template to the referenced
lowercase name) and modify the gh command used in the workflow/script so it
passes the template as the body file (use gh pr create/edit --body-file
or the equivalent flag your CLI version supports) instead of only
validating the existing body; also ensuremix pr_body.checkis pointed at the
correct template path or updated to validate the file that will actually be
supplied to gh.- Around line 29-30: The SKILL.md includes hardcoded Elixir validation commands
("make -C elixir all" and "(cd elixir && mix pr_body.check ...)") that will fail
because there is no elixir/ workspace; remove these commands or wrap them with a
presence check: either replace them with the repository's actual validation
steps or add a conditional gate that detects the elixir/ directory and the mix
task before invoking "make -C elixir all" or "mix pr_body.check" so the push
skill skips those steps gracefully when the workspace or task is absent.In @.codex/skills/debug/SKILL.md:
- Around line 53-55: The current extractor rg -o "session_id=[^ ;]+" against
log/symphony.log* can return session_ids from unrelated tickets; restrict the
search to lines for the ticket being debugged by filtering the logs for that
ticket first (e.g., pipe or chain a search for the ticket identifier) and then
run the rg -o "session_id=[^ ;]+" extractor and sort -u on the ticket-scoped
output so only session_id values seen for that ticket are returned.In @.codex/skills/land/land_watch.py:
- Around line 575-600: The current mergeability check in watch_pr uses
is_merge_conflicting(pr) which only treats CONFLICTING/DIRTY as blocking, so if
GitHub returns mergeable = UNKNOWN/null we can proceed prematurely; modify
watch_pr (and the inner head_monitor) to poll get_pr_info until mergeable is
resolved (i.e., not UNKNOWN/null) before proceeding: loop with
asyncio.sleep(POLL_SECONDS) and a reasonable timeout/retry count, re-evaluating
is_merge_conflicting(current) once mergeable is known, and only continue or exit
after mergeability has settled (use existing POLL_SECONDS and raise
SystemExit(5) on confirmed conflicts as currently done). Ensure the initial
pre-check for pr and the head_monitor both implement this retry-until-resolved
behavior.- Around line 523-543: The watcher currently allows exit as soon as CI finishes
even when the latest review_request_at has not yet produced any Codex comment;
update the logic that collects bot_comments (bot_issue_comments +
bot_review_comments) to compute bot_comments_after_request = [c for c in
bot_comments if parse_time(c["created_at"]) >= review_request_at] and use that
list for the “latest”/message/exit decision so that if no bot comment exists
after review_request_at the function does not proceed (i.e., do not treat
checks_done.is_set() as success when bot_comments_after_request is empty); keep
raise_on_human_feedback as-is and only inspect/latest/sanitize/print/raise
SystemExit(2) when bot_comments_after_request contains entries.In @.codex/skills/push/SKILL.md:
- Line 29: Replace the hardcoded validation step "make -C elixir all" in
.codex/skills/push/SKILL.md with the repository's documented pnpm-based
validation command from AGENTS.md (see AGENTS.md:41-96) so contributors run the
canonical checks; update both occurrences of the string "make -C elixir all"
(around the current lines showing that text) to the exact pnpm validation
command used in AGENTS.md and ensure the wording references running the repo
validation (not an Elixir-specific make target).- Around line 89-104: The PR body draft in tmp_pr_body should be passed to gh pr
create/edit via the --body-file flag before running the linter; update the flow
so that gh pr create (when pr_state empty) and gh pr edit (when pr_state set)
both include --body-file "$tmp_pr_body" (using the same tmp_pr_body used later)
and only after that invoke (cd elixir && mix pr_body.check --file
"$tmp_pr_body"); ensure tmp_pr_body is populated with the drafted template
(replace or skip gh pr view > "$tmp_pr_body" which validates the existing remote
body).In
@ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js:
- Around line 58-59: The generic settings serializer currently includes the
OAuth long-lived credentials 'x_access_token' and 'x_access_token_secret' in
EDITABLE_SETTINGS, which allows writes outside the dedicated X endpoints; remove
those two entries from EDITABLE_SETTINGS in the settings serializer so OAuth
tokens are only writable via the dedicated /x endpoints (leave isSecretSetting
as-is to continue treating them as secrets and do not add alternate validation
here).In
@ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js:
- Around line 45-53: When inserting X token rows in the migration (the await
connection('settings').insert({...}) block), ensure rows with setting.key ===
'x_access_token' or 'x_access_token_secret' have their flags set to mark them
secret (e.g. flags: 'secret') instead of null so
settingsBREADService._formatBrowse() will mask them and /x/verify/ won't return
raw tokens; update the insert logic to conditionally set flags based on the key
before calling insert.In
@skills-lock.json:
- Around line 24-52: The skills-lock.json entries for "commit", "land",
"linear", "pull", and "push" point to the remote repo odysseus0/symphony which
will override the local skill code edited in this PR; regenerate or update
skills-lock.json so those keys resolve to the local implementations changed in
.agents/skills by either removing/replacing the "source"/"sourceType" with the
local reference or updating the "computedHash" values to match the local files,
then commit the regenerated lockfile (use your project's skill-lock generation
command/tool to ensure consistency).In
@WORKFLOW.md:
- Around line 69-77: Replace the sentence that instructs the agent to "stop and
ask the user to configure Linear" under the "Prerequisite:linear_graphqltool
is available" section with an instruction to treat a missinglinear_graphql
tool as a blocked-access condition: log the blockage in the workpad, move the
issue according to the existing blocked-access workflow, and continue without
prompting the user for configuration; ensure the revised text references the
linear_graphqlprerequisite and the workpad/blocked-access handling so the
agent follows the documented blocked path instead of asking humans to set up
Linear.- Around line 21-23: The after_create hook hardcodes a machine-specific repo
path in the git clone command (git clone --depth 1
file:///Users/doronkatz/Development/Ghost .) which will fail on other runners;
change after_create to read the repository URL from configuration or environment
(e.g., an env var like BOOTSTRAP_REPO or derive it from the current git remote)
and use that variable in the git clone command so the clone is not tied to
/Users/doronkatz/Development/Ghost and works across CI/workspaces.
Minor comments:
In @.agents/skills/land/land_watch.py:
- Around line 577-582: The watcher now raises SystemExit(5) when
is_merge_conflicting(pr) in land_watch.py (also the same pattern at the second
occurrence around the other raise), so update .agents/skills/land/SKILL.md to
document exit code 5 with a brief description (e.g., "exit code 5 — PR has merge
conflicts; resolve/rebase against main and push") and ensure callers that rely
on documented outcomes know to treat 5 as a handled/expected state; verify both
occurrences of SystemExit(5) are covered in the documentation.In @.agents/skills/land/SKILL.md:
- Around line 110-115: Update the Exit codes list in SKILL.md to include the
missing code 5: add an entry such as "- 5: PR merge-conflicting (watcher
detected merge conflict)" so the table documents the watcher exit when a PR is
merge-conflicting; modify the existing Exit codes section (the list containing
codes 2, 3, 4) to insert this new line and keep formatting consistent with the
other entries.In @.agents/skills/pull/SKILL.md:
- Line 24: Update the SKILL.md to document the minimum Git version required for
the zdiff3 conflict style: state that Git 2.35+ is required (zdiff3 introduced
Jan 2022) and add this to a "Prerequisites" section; reference the existing
mention of merge.conflictstyle=zdiff3 so readers see the requirement, and
optionally add guidance for a fallback merge strategy or configuration for older
Git versions (e.g., alternate conflict style) so the skill can run reliably on
older environments.In @.codex/skills/commit/SKILL.md:
- Around line 60-75: The fenced code block for the commit-message template is
missing a language identifier causing markdownlint MD040; update the opening
triple-backtick of the example block (the block that starts withand contains the commit template with "Summary:", "Rationale:", "Tests:", and "Co-authored-by:") to include a language such as "text" (i.e., change the openingto ```text) so the fence is typed and MD040 is resolved.In @.codex/skills/land/SKILL.md:
- Around line 110-115: The Exit codes table is missing an entry for the watcher
exit code 5; update the "Exit codes:" list in SKILL.md to include "5: PR
merge-conflicting" (or similar wording like "5: PR merge-conflicting (merge
conflict detected)") so callers mapping exit codes handle the conflict path;
ensure the new entry follows the existing format alongside 2/3/4 in the same
section.In
@apps/admin-x-settings/test/acceptance/advanced/integrations/x.test.ts:
- Around line 101-104: The assertion on lastApiRequests.disconnectX races
because it's set asynchronously after the confirmation click; after calling
xModal.getByRole(...).click() and
page.getByTestId('confirmation-modal').getByRole(...).click(), wait for
lastApiRequests.disconnectX to be populated before asserting (e.g., use a short
polling loop or test helper like waitFor that repeatedly checks
lastApiRequests.disconnectX until truthy, or await a page.waitForResponse
matching the X disconnect endpoint), then assert
expect(lastApiRequests.disconnectX).toBeTruthy(); reference the existing symbols
lastApiRequests.disconnectX, xModal.getByRole, and
page.getByTestId('confirmation-modal').getByRole when implementing the wait.In
@ghost/admin/tests/acceptance/editor-test.js:
- Around line 388-391: The assertion for xPostEnabled is too loose (it allows
undefined/null); update the expectation on
this.server.db.posts.find(post.id).xPostEnabled to assert true explicitly (use
expect(...).to.equal(true)) or alternatively assert the UI checkbox state tied
to xPostEnabled to ensure the default is serialized correctly; adjust the test
in editor-test.js where the current expect(...).to.not.equal(false) is used so
it verifies a concrete true value.In
@ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js:
- Around line 56-70: The down() migration currently deletes any matching x_* key
unconditionally; change it to only remove settings that this migration actually
created by comparing the existing row's value to the expected value from
X_SETTINGS before deleting. In the down function, for each setting in X_SETTINGS
check existingSetting.value === setting.value (or another unique marker set by
up) and only call .del() when it matches; otherwise log and skip to avoid
removing pre-existing/manual settings.
Nitpick comments:
In @.agents/skills/pull/SKILL.md:
- Around line 23-26: Update the SKILL.md step that uses "git pull --ff-only" to
include error-handling guidance: detect a non-zero exit from git pull --ff-only,
run git fetch and inspect divergence with git status or git log --left-right
--graph origin/$(git branch --show-current)...HEAD, then describe safe recovery
options (rebase onto origin, merge, or reset --hard if force-pushed) and remind
to back up local changes (stash or create a temp branch) before destructive
commands; reference the existing step text ("git pull --ff-only origin $(git
branch --show-current)") so reviewers can insert a short paragraph explaining
these diagnostics and the recommended actions.- Around line 28-29: Update SKILL.md to document that the git
merge.conflictstyle value "zdiff3" requires Git 2.35+ (note that it was verified
with 2.39+) and add a fallback suggestion using "diff3" for older Git versions;
specifically mention the example command shown (git -c merge.conflictstyle=zdiff3 merge origin/main) and provide the alternative (git -c merge.conflictstyle=diff3 merge origin/main) so users without zdiff3 know
which command to run or to upgrade Git.In
@apps/admin-x-settings/src/components/settings/advanced/integrations/x-modal.tsx:
- Around line 6-21: The code currently forces settings to string via
getSettingValues(...) and then derives isConnected from
xAccessToken/xUsername, which can mask null values; change the call to
getSettingValues without the generic and treat the returned
xAccessToken and xUsername as possibly null/unknown, perform runtime typeof
checks (e.g. typeof xAccessToken === 'string' && typeof xUsername === 'string')
before computing isConnected, and apply sensible defaults or null-handling where
xAccessToken, xUsername, or isConnected are used (refer to getSettingValues,
xAccessToken, xUsername, and isConnected to locate and update the logic).In
@ghost/core/test/unit/server/api/endpoints/utils/serializers/output/posts.test.js:
- Around line 13-16: Replace the current multiple postsMapper.set calls with
a single call that captures the revert function (e.g., const revert =
postsMapper.set({url: ..., extraAttrs: ..., clean: ..., utils: ...})), then
ensure you call revert() in a finally block (alongside sinon.restore()) so the
module-level bindings are restored after the test; target the setup where
postsMapper.set is currently used and wrap the test execution in try/finally
to call revert() to avoid polluting other tests.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `86be2e52-2db4-483b-b64c-97a93ecd8ebe` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b5c0a92cc8e8ba3cb546590d9d6c8b113319db0c and 4b1497b668985c8cd233358731f8d3bcbd7be6e6. </details> <details> <summary>📒 Files selected for processing (51)</summary> * `.agents/skills/commit/SKILL.md` * `.agents/skills/debug/SKILL.md` * `.agents/skills/land/SKILL.md` * `.agents/skills/land/land_watch.py` * `.agents/skills/launch-app/SKILL.md` * `.agents/skills/linear/SKILL.md` * `.agents/skills/pull/SKILL.md` * `.agents/skills/push/SKILL.md` * `.codex/skills/commit/SKILL.md` * `.codex/skills/debug/SKILL.md` * `.codex/skills/land/SKILL.md` * `.codex/skills/land/land_watch.py` * `.codex/skills/linear/SKILL.md` * `.codex/skills/pull/SKILL.md` * `.codex/skills/push/SKILL.md` * `WORKFLOW.md` * `apps/admin-x-framework/src/api/settings.ts` * `apps/admin-x-framework/src/test/responses/settings.json` * `apps/admin-x-settings/src/components/providers/routing/modals.tsx` * `apps/admin-x-settings/src/components/providers/settings-router.tsx` * `apps/admin-x-settings/src/components/settings/advanced/integrations.tsx` * `apps/admin-x-settings/src/components/settings/advanced/integrations/x-modal.tsx` * `apps/admin-x-settings/test/acceptance/advanced/integrations/x.test.ts` * `ghost/admin/app/components/gh-post-settings-menu.hbs` * `ghost/admin/app/components/gh-post-settings-menu.js` * `ghost/admin/app/models/post.js` * `ghost/admin/app/serializers/page.js` * `ghost/admin/tests/acceptance/editor-test.js` * `ghost/core/core/boot.js` * `ghost/core/core/server/api/endpoints/index.js` * `ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js` * `ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-group-mapper.js` * `ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-type-mapper.js` * `ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/pages.js` * `ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.js` * `ghost/core/core/server/api/endpoints/utils/serializers/output/utils/clean.js` * `ghost/core/core/server/api/endpoints/x.js` * `ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js` * `ghost/core/core/server/data/schema/default-settings/default-settings.json` * `ghost/core/core/server/data/schema/schema.js` * `ghost/core/core/server/models/posts-meta.js` * `ghost/core/core/server/services/posts/posts-service.js` * `ghost/core/core/server/services/settings/settings-utils.js` * `ghost/core/core/server/services/x.js` * `ghost/core/core/server/web/api/endpoints/admin/routes.js` * `ghost/core/core/shared/config/defaults.json` * `ghost/core/test/unit/server/api/endpoints/utils/serializers/output/posts.test.js` * `ghost/core/test/unit/server/services/posts/posts-service.test.js` * `ghost/core/test/unit/server/services/settings/settings-utils.test.js` * `ghost/core/test/unit/server/services/x.test.js` * `skills-lock.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+29
to
+34
| 3. Stage intended changes, including new files (`git add -A`) after confirming | ||
| scope. | ||
| 4. Sanity-check newly added files; if anything looks random or likely ignored | ||
| (build artifacts, logs, temp files), flag it to the user before committing. | ||
| 5. If staging is incomplete or includes unrelated files, fix the index or ask | ||
| for confirmation. |
Contributor
There was a problem hiding this comment.
Avoid git add -A in a selective commit workflow.
git add -A sweeps every tracked deletion and untracked file in the repo into the index. For a skill that is supposed to commit only the intended work, that's too easy to turn into an unrelated mixed commit.
Safer wording
-3. Stage intended changes, including new files (`git add -A`) after confirming
- scope.
+3. Stage only the intended paths after confirming scope. Prefer explicit
+ `git add <path>...` (or `git add -p` when needed) so unrelated repo changes
+ are not swept into the commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/commit/SKILL.md around lines 29 - 34, Replace the risky
instruction that tells users to "Stage intended changes, including new files
(git add -A)" with guidance to stage only intended changes—e.g., recommend
interactive or path-specific staging (replace the phrase "git add -A" in that
line) and update surrounding steps so step 4 instructs checking staged changes
(git status/git diff --staged) before committing and step 5 advises amending the
index or asking for confirmation if unrelated files are present; ensure the
SKILL.md text explicitly suggests safer alternatives to git add -A and clarifies
the workflow for selective commits.
Comment on lines
+53
to
+55
| # 3) Pull session IDs seen for that ticket | ||
| rg -o "session_id=[^ ;]+" log/symphony.log* | sort -u | ||
|
|
Contributor
There was a problem hiding this comment.
Filter session IDs through the ticket-specific log slice.
The prose says “session IDs seen for that ticket”, but this command scans every rotated log and can surface unrelated sessions. Reuse the ticket-filtered matches here before extracting IDs.
🛠️ Proposed fix
# 3) Pull session IDs seen for that ticket
-rg -o "session_id=[^ ;]+" log/symphony.log* | sort -u
+rg -n "issue_identifier=MT-625" log/symphony.log* \
+ | rg -o "session_id=[^ ;]+" \
+ | sort -u📝 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.
Suggested change
| # 3) Pull session IDs seen for that ticket | |
| rg -o "session_id=[^ ;]+" log/symphony.log* | sort -u | |
| # 3) Pull session IDs seen for that ticket | |
| rg -n "issue_identifier=MT-625" log/symphony.log* \ | |
| | rg -o "session_id=[^ ;]+" \ | |
| | sort -u |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/debug/SKILL.md around lines 53 - 55, The current rg command
(rg -o "session_id=[^ ;]+" log/symphony.log*) scans all logs and can return
unrelated sessions; change it to pipe from the ticket-specific filtered output
instead of scanning log/symphony.log* directly — i.e., reuse the prior
ticket-filtered match stream (the earlier rg/filter that narrows to the ticket)
as the input and then extract session IDs by applying the session_id regex (the
"session_id=[^ ;]+" pattern) and sort -u; update the step that references rg -o
"session_id=[^ ;]+" to operate on the ticket-filtered lines rather than all log
files.
Comment on lines
+306
to
+327
| def filter_human_issue_comments(comments: list[dict[str, Any]]) -> list[dict[str, Any]]: | ||
| latest_ack = latest_codex_issue_reply_time(comments) | ||
| filtered: list[dict[str, Any]] = [] | ||
| for comment in comments: | ||
| if is_bot_user(comment.get("user", {})): | ||
| continue | ||
| body = (comment.get("body") or "").strip() | ||
| if is_codex_reply_body(body): | ||
| continue | ||
| if is_codex_review_body(body): | ||
| continue | ||
| if "@codex review" in body: | ||
| continue | ||
| created_time = comment_time(comment) | ||
| if ( | ||
| latest_ack is not None | ||
| and created_time is not None | ||
| and created_time <= latest_ack | ||
| ): | ||
| continue | ||
| filtered.append(comment) | ||
| return filtered |
Contributor
There was a problem hiding this comment.
Reset feedback gating at the newest @codex review request.
latest_review_request_at() is computed, but the human issue-comment, human review-comment, and review-state filters never use it. That means unresolved feedback from an older review cycle can still trigger SystemExit(2) after a fresh @codex review, which defeats the “only consider activity after the latest request” behavior described for this watcher.
🛠️ Proposed fix
def filter_human_issue_comments(
comments: list[dict[str, Any]],
+ review_requested_at: datetime | None,
) -> list[dict[str, Any]]:
@@
created_time = comment_time(comment)
+ if (
+ review_requested_at is not None
+ and created_time is not None
+ and created_time <= review_requested_at
+ ):
+ continue
if (
latest_ack is not None
and created_time is not None
and created_time <= latest_ack
):
continueApply the same cutoff in filter_codex_review_issue_comments(), filter_human_review_comments(), and is_blocking_review() before raise_on_human_feedback() evaluates them.
Also applies to: 330-347, 379-398, 401-466, 488-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/land/land_watch.py around lines 306 - 327, The code computes
a latest review-request cutoff (latest_review_request_at) but never applies it
when evaluating human feedback, so older unresolved feedback can still trigger
raise_on_human_feedback; update filter_human_issue_comments and also apply the
same cutoff in filter_codex_review_issue_comments, filter_human_review_comments,
and is_blocking_review so they ignore comments/reviews with comment_time() (or
created_time) <= latest_review_request_at() before raise_on_human_feedback is
called; ensure you reference and use latest_review_request_at() (or pass
latest_ack) as the gating value and skip items earlier than that in those
functions (e.g., inside filter_human_issue_comments,
filter_codex_review_issue_comments, filter_human_review_comments, and
is_blocking_review) so only activity after the most recent "@codex review" is
considered.
Comment on lines
+29
to
+30
| 2. Run local validation (`make -C elixir all`) before pushing. | ||
| 3. Push branch to `origin` with upstream tracking if needed, using whatever |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for an elixir workspace:"
fd -HI '^elixir$' -td .
echo
echo "Checking for pr_body.check / related Mix task definitions:"
rg -n --hidden -S 'pr_body\.check|Mix\.Tasks\..*PrBody|defmodule\s+Mix\.Tasks\.[A-Za-z0-9_.]*PrBody' .Repository: TryGhost/Ghost
Length of output: 545
🏁 Script executed:
cat -n .agents/skills/push/SKILL.md | head -120Repository: TryGhost/Ghost
Length of output: 5271
🏁 Script executed:
# Double-check for any elixir directory at any location, not just root
find . -type d -name elixir 2>/dev/null | head -20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if there's any .gitignore or similar that might hide it
git ls-files | grep -i elixir || echo "No elixir files in git index"Repository: TryGhost/Ghost
Length of output: 86
Remove or conditionally gate the hardcoded Elixir validation commands, as the elixir/ workspace does not exist in this repository.
Lines 29, 65, and 104 assume a make -C elixir all target and mix pr_body.check Mix task that are not present in this repo. The skill will fail at execution:
- Line 65:
make -C elixir allwill fail (noelixir/directory) - Line 104:
(cd elixir && mix pr_body.check ...)will fail (directory and task missing)
Either remove these validation steps, replace them with actual repository validation steps, or add a guard that skips them gracefully if the workspace is absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/push/SKILL.md around lines 29 - 30, The SKILL.md includes
hardcoded Elixir validation commands ("make -C elixir all" and "(cd elixir &&
mix pr_body.check ...)") that will fail because there is no elixir/ workspace;
remove these commands or wrap them with a presence check: either replace them
with the repository's actual validation steps or add a conditional gate that
detects the elixir/ directory and the mix task before invoking "make -C elixir
all" or "mix pr_body.check" so the push skill skips those steps gracefully when
the workspace or task is absent.
Comment on lines
+47
to
+55
| 6. Write/update PR body explicitly using `.github/pull_request_template.md`: | ||
| - Fill every section with concrete content for this change. | ||
| - Replace all placeholder comments (`<!-- ... -->`). | ||
| - Keep bullets/checkboxes where template expects them. | ||
| - If PR already exists, refresh body content so it reflects the total PR | ||
| scope (all intended work on the branch), not just the newest commits, | ||
| including newly added work, removed work, or changed approach. | ||
| - Do not reuse stale description text from earlier iterations. | ||
| 7. Validate PR body with `mix pr_body.check` and fix all reported issues. |
Contributor
There was a problem hiding this comment.
The PR body flow is still a no-op.
This points to .github/pull_request_template.md, but the template here is .github/PULL_REQUEST_TEMPLATE.md. The command block also never passes a body file to gh pr create/edit; it only validates whatever body already exists. New PRs can therefore open with empty/stale descriptions and the documented template never gets applied.
Possible fix
-6. Write/update PR body explicitly using `.github/pull_request_template.md`:
+6. Write/update PR body explicitly using `.github/PULL_REQUEST_TEMPLATE.md`:
@@
-# Write/edit PR body to match .github/pull_request_template.md before validation.
+# Write/edit PR body to match .github/PULL_REQUEST_TEMPLATE.md before validation.
# Example workflow:
# 1) open the template and draft body content for this PR
-# 2) gh pr edit --body-file /tmp/pr_body.md
+# 2) gh pr create/edit --body-file /tmp/pr_body.md
# 3) for branch updates, re-check that title/body still match current diffAlso applies to: 96-105
🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: The official name of this software platform is spelled with a capital “H”.
Context: .... Write/update PR body explicitly using .github/pull_request_template.md: - Fill ev...
(GITHUB)
[style] ~55-~55: Consider using a different verb for a more formal wording.
Context: ...te PR body with mix pr_body.check and fix all reported issues. 8. Reply with the ...
(FIX_RESOLVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/push/SKILL.md around lines 47 - 55, The documented PR-body
flow is a no-op because the template path and GH CLI invocation are wrong:
update references from `.github/pull_request_template.md` to the actual file
`.github/PULL_REQUEST_TEMPLATE.md` (or rename the template to the referenced
lowercase name) and modify the gh command used in the workflow/script so it
passes the template as the body file (use gh pr create/edit --body-file
<template> or the equivalent flag your CLI version supports) instead of only
validating the existing body; also ensure `mix pr_body.check` is pointed at the
correct template path or updated to validate the file that will actually be
supplied to gh.
Comment on lines
+58
to
+59
| 'x_access_token', | ||
| 'x_access_token_secret', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Definitions and usages of the X settings keys"
rg -n "\b(x_access_token|x_access_token_secret|x_user_id|x_username)\b" ghost/core apps
echo
echo "Secret/obfuscation handling in settings serialization/services"
rg -n "isSecretSetting|obfuscatedSetting|obfuscat|secret" ghost/core/core/server/services ghost/core/core/server/api
echo
echo "Settings writers touching the X keys"
rg -n "Settings\.edit|settingsBREADService\.edit|models\.Settings\.edit" ghost/core appsRepository: TryGhost/Ghost
Length of output: 30158
Remove OAuth token credentials from generic settings edit path.
Lines 58–59 make the long-lived X credentials writable through the generic non-internal settings serializer, in addition to the dedicated /x endpoints. Since these keys are already handled by dedicated X endpoints that presumably enforce OAuth flow validation, including them in EDITABLE_SETTINGS widens the write surface unnecessarily.
Although isSecretSetting correctly identifies these as secrets (the regex matches "access_token"), allowing writes through two paths still creates an additional attack vector that bypasses the validation logic in the dedicated endpoints.
Suggested change
- 'x_access_token',
- 'x_access_token_secret',
'x_user_id',
'x_username',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js`
around lines 58 - 59, The generic settings serializer currently includes the
OAuth long-lived credentials 'x_access_token' and 'x_access_token_secret' in
EDITABLE_SETTINGS, which allows writes outside the dedicated X endpoints; remove
those two entries from EDITABLE_SETTINGS in the settings serializer so OAuth
tokens are only writable via the dedicated /x endpoints (leave isSecretSetting
as-is to continue treating them as secrets and do not add alternate validation
here).
Comment on lines
+45
to
+53
| await connection('settings').insert({ | ||
| id: ObjectId().toHexString(), | ||
| key: setting.key, | ||
| value: setting.value, | ||
| group: setting.group, | ||
| type: setting.type, | ||
| flags: null, | ||
| created_at: connection.raw('CURRENT_TIMESTAMP') | ||
| }); |
Contributor
There was a problem hiding this comment.
Mark the X token settings as secret before inserting them.
settingsBREADService._formatBrowse() only masks secret settings, and /x/verify/ returns a fresh settings browse response right after these rows are saved. With flags: null, x_access_token and x_access_token_secret will be exposed back to the admin client as raw values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/data/migrations/versions/6.29/2026-04-13-00-12-50-add-x-auto-posting.js`
around lines 45 - 53, When inserting X token rows in the migration (the await
connection('settings').insert({...}) block), ensure rows with setting.key ===
'x_access_token' or 'x_access_token_secret' have their flags set to mark them
secret (e.g. flags: 'secret') instead of null so
settingsBREADService._formatBrowse() will mask them and /x/verify/ won't return
raw tokens; update the insert logic to conditionally set flags based on the key
before calling insert.
Comment on lines
24
to
+52
| "commit": { | ||
| "source": "./.agents", | ||
| "sourceType": "local", | ||
| "computedHash": "f2f9a52e210a32ad42d4bf5826ce7cec265a2c472de03bee3840a5b295d734d7" | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "9384add5572ef41749c7b24dbff7131db267506aff3bce7ff7c2effe4ec0e7f8" | ||
| }, | ||
| "debug": { | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "9ba57239460065b1acf67d80293b1332e26077cab5dc0047867ad53949fff97d" | ||
| }, | ||
| "land": { | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "b2a4891b2454d35e300620ef3c44e444f64c207c59fbbfe9a1d1af2b01fc1a1a" | ||
| }, | ||
| "linear": { | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "50a8e09b2223e9b958e780a877a64e5b6f0b225650e14714cf94847987837086" | ||
| }, | ||
| "pull": { | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "f32735dab559a59353bcb5855065c6dce991f4f31b3dbe2925493842e9aa324f" | ||
| }, | ||
| "push": { | ||
| "source": "odysseus0/symphony", | ||
| "sourceType": "github", | ||
| "computedHash": "b9ace137b2f25f73887e6cee47b91b548f300ea771a41a81999b02a7ce6c12a9" |
Contributor
There was a problem hiding this comment.
This lockfile now points away from the local skills changed in the same PR.
commit, land, linear, pull, and push now resolve from odysseus0/symphony, while this PR edits the corresponding local implementations under .agents/skills/.... If the resolver honors skills-lock.json, it will fetch the remote versions and ignore the files modified here until that external repo is updated to the same hashes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills-lock.json` around lines 24 - 52, The skills-lock.json entries for
"commit", "land", "linear", "pull", and "push" point to the remote repo
odysseus0/symphony which will override the local skill code edited in this PR;
regenerate or update skills-lock.json so those keys resolve to the local
implementations changed in .agents/skills by either removing/replacing the
"source"/"sourceType" with the local reference or updating the "computedHash"
values to match the local files, then commit the regenerated lockfile (use your
project's skill-lock generation command/tool to ensure consistency).
Comment on lines
+21
to
+23
| after_create: | | ||
| git clone --depth 1 file:///Users/doronkatz/Development/Ghost . | ||
| /bin/zsh -lc 'cd "$PWD" && corepack enable pnpm && pnpm run setup' |
Contributor
There was a problem hiding this comment.
Parameterize the bootstrap repo path.
file:///Users/doronkatz/Development/Ghost is machine-specific, so any other runner will fail during workspace creation. This needs to come from config/env or the current repo remote instead of a developer home directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WORKFLOW.md` around lines 21 - 23, The after_create hook hardcodes a
machine-specific repo path in the git clone command (git clone --depth 1
file:///Users/doronkatz/Development/Ghost .) which will fail on other runners;
change after_create to read the repository URL from configuration or environment
(e.g., an env var like BOOTSTRAP_REPO or derive it from the current git remote)
and use that variable in the git clone command so the clone is not tied to
/Users/doronkatz/Development/Ghost and works across CI/workspaces.
Comment on lines
+69
to
+77
| 1. This is an unattended orchestration session. Never ask a human to perform follow-up actions. | ||
| 2. Only stop early for a true blocker (missing required auth/permissions/secrets). If blocked, record it in the workpad and move the issue according to workflow. | ||
| 3. Final message must report completed actions and blockers only. Do not include "next steps for user". | ||
|
|
||
| Work only in the provided repository copy. Do not touch any other path. | ||
|
|
||
| ## Prerequisite: `linear_graphql` tool is available | ||
|
|
||
| The agent talks to Linear via the `linear_graphql` tool injected by Symphony's app-server. If it is not present, stop and ask the user to configure Linear. Do not use a Linear MCP server - it returns full JSON payloads that waste tokens. Use `linear_graphql` with narrowly scoped queries instead. |
Contributor
There was a problem hiding this comment.
Don't tell an unattended session to ask the user for Linear setup.
Line 69 says this workflow never asks humans for follow-up, but Line 77 instructs the agent to stop and ask the user to configure Linear. In the missing-tool case that creates a dead end instead of following the blocked-access path later in the document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WORKFLOW.md` around lines 69 - 77, Replace the sentence that instructs the
agent to "stop and ask the user to configure Linear" under the "Prerequisite:
`linear_graphql` tool is available" section with an instruction to treat a
missing `linear_graphql` tool as a blocked-access condition: log the blockage in
the workpad, move the issue according to the existing blocked-access workflow,
and continue without prompting the user for configuration; ensure the revised
text references the `linear_graphql` prerequisite and the workpad/blocked-access
handling so the agent follows the documented blocked path instead of asking
humans to set up Linear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Testing
Refs DOR-599
Note
High Risk
High risk because this introduces new admin API endpoints, stores/obfuscates OAuth credentials, adds a DB migration, and hooks into the publish event to call an external service—any issues could affect publishing or leak/mishandle tokens.
Overview
Adds a built-in X integration end-to-end: new admin UI entry + modal for connecting via out-of-band OAuth verifier, verifying/disconnecting, and showing connected state.
Implements new admin API endpoints (
/x/,/x/verify/,DELETE /x/) plus a backendxservice that performs OAuth 1.0a request-token/access-token flows, stores X credentials in settings (now grouped asx), and publishes a post link to X onpost.publishedwhen configured.Introduces a
posts_meta.x_post_enabledflag (DB migration + editor toggle) to opt out per post, ensures the flag is preserved through serialization/copy flows, and expands secret-setting detection to obfuscate*_access_token*values in settings responses.Reviewed by Cursor Bugbot for commit 4b1497b. Bugbot is set up for automated code reviews on this repo. Configure here.