chore: land PR #8 into main (stacked merge landed on the wrong base) - #9
Merged
Merged
Conversation
…yment tree PR #7 made app/test/typecheck work in a clean clone. The mcp service still could not start there: it attaches to the external agentic-ops network (start fails with "Could not attach to network … not found"), pins NODE_ENV=production against an empty SEO_MCP_TOKEN from the template (the server refuses to boot), and bind-mounts a sibling Hugo checkout that Docker then auto-creates as a root-owned directory next to the clone. Compose merges overlays additively, so a volume or network in the base file cannot be removed by one — only parameterised. The frontend mount source therefore becomes ${SEO_FRONTEND_DIR:-…}, defaulting to the production path so a deployment whose .env never heard of the variable is unaffected. `docker compose config mcp` on the base file alone is byte-identical before and after this commit. The overlay is named docker-compose.standalone.yml and must be passed explicitly, rather than following the docker-compose.override.yml convention used by sibling projects. That filename is auto-loaded by any bare `docker compose up`, so once such a file existed in this repo, a deploy that forgot to exclude it would silently switch NODE_ENV to development — which turns the MCP auth guard off and publishes an unauthenticated endpoint. Requiring both -f flags makes that impossible by accident. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review of #8: seo-tools is a standalone application and a public repository, so its compose file should describe the app and nothing about where it runs. The previous shape had it backwards — the lkv deployment was the base configuration and docker-compose.standalone.yml was an escape hatch from it, including a frontend mount defaulting to a sibling website checkout. Inverting it is also the only layering Compose supports cleanly: overlays merge additively, so a base file's network or volume can never be subtracted by one, only added to. Base as the generic app and deployment as the overlay composes naturally; the reverse needed the ${VAR:-…} parameterisation this removes. Dropped from the mcp service: ../.env.shared, the VIRTUAL_*/LETSENCRYPT_HOST/ HTTPS_METHOD proxy labels, and the external agentic-ops network. The frontend mount now defaults to ./storage/frontend inside the repo. Comments that narrated one particular VPS were rewritten to state the constraint rather than the host — "only a trusted reverse proxy may set X-Forwarded-For" survives, the topology does not. docker-compose.standalone.yml is deleted; with the base standalone it has nothing left to override. NODE_ENV=production and the mandatory SEO_MCP_TOKEN stay. Requiring a token is the right posture for anything listening on a socket, standalone included; a tokenless default is how an open endpoint happens. BREAKING CHANGE: deployments that need a reverse proxy, an external network or extra mounts must now supply their own compose overlay and pass both -f flags. Verified that base + such an overlay reproduces the previous production config byte-identically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
refactor(compose)!: make seo-tools a standalone application
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Mechanical follow-up — no new code. This branch is 3 commits ahead of
mainand its diff is exactly the content of #8.What happened
#8 was stacked on #7, with
fix/sandbox-standalone-findingsas its base. Both were merged within the same minute, but in an order that stranded #8:#7 merged the branch tip as it stood before #8 landed on it. GitHub correctly marked #8 as merged — into its own base branch — but its content never reached
main. Automatic retargeting tomainonly happens when the base branch is deleted after merging, and it wasn't.Consequence
maincurrently has #7's crawler fixes (the__nameshim, the bounded scroll) but not #8's compose work. It still contains:env_file: ../.env.shared— a path outside the repoagentic-ops— 4 references to an external networkSo
mainis still coupled to one particular deployment tree, which is precisely what #8 removed.This PR
Brings
mainup to the branch, landing #8's 5 files unchanged:See #8 for the reasoning; nothing here differs from what was reviewed there.
After merging, delete both
fix/sandbox-standalone-findingsandfix/mcp-standalone— leaving them is what allowed the mis-stacking in the first place.🤖 Generated with Claude Code