Skip to content

feat: unify cloud resource management with .env integration - #100

Merged
bighadj22 merged 2 commits into
bighadj22:mainfrom
mouradsme:improvement/unified-codflow-cloud-resource-values
Sep 9, 2026
Merged

feat: unify cloud resource management with .env integration#100
bighadj22 merged 2 commits into
bighadj22:mainfrom
mouradsme:improvement/unified-codflow-cloud-resource-values

Conversation

@mouradsme

Copy link
Copy Markdown
Contributor

Unified CodFlow Cloud resource values — single source of truth for the seeder scripts, the D1 migration wrapper, the R2 CORS setup, and the storefront deploy helper.

@bighadj22

Copy link
Copy Markdown
Owner

Hi @mouradsme,

Thank you for this contribution — and for being the first contributor to CodFlow! 🎉

I reviewed the PR thoroughly (full read of every file, plus testing the
scripts end-to-end in an isolated environment), and the core of it is
genuinely valuable and well-built:

What we verified and like:

  • The problem is real: codflow-os-db (and the R2 bucket name) were
    hardcoded in five different scripts, and self-hosters who rename their
    database silently break seeding, migrations, and the EcoTrack catalog
    setup. Centralizing these into one gitignored .env with a
    process.env → .env → default precedence is the right design.
  • cod-server/scripts/cloud-env.mjs is clean — the parser handles
    comments, blank lines, and quoted values correctly (we tested all three),
    and the precedence order is correct.
  • The d1.mjs wrapper injects the database name into the correct positional
    slot for execute, migrations apply, migrations list, and delete
    (we ran it against a real local D1 — works).
  • The cross-package imports (seed-admin.mjs and the storefront deploy
    helper importing from cod-server/scripts/) resolve correctly in the
    npm-workspaces layout.
  • As a bonus, this PR fixes something we had gotten wrong ourselves: our
    theme01 deploy script hardcoded a production URL in package.json,
    which violates our own "no production URLs in committed files" rule.
    Your approach is the correct replacement — thank you!

Before we can merge, there are three blockers we'd ask you to address:

1. Rebase onto current main — the PR conflicts with recently merged work

We merged a large PR (#102) after you branched, and two files now conflict:
cod-astro/theme01/package.json and cod-astro/theme01/wrangler.jsonc.
Please rebase onto the latest main and resolve. Note that after the
rebase, the deploy-script conflict resolution should keep your
.env-driven approach — just make sure the final wrangler.jsonc keeps
the observability block that now exists on main.

2. Fail loudly when the deploy URL is missing (silent localhost deploy)

In cod-astro/theme01/scripts/deploy.mjs, when no .env exists and no
COD_SERVER_URL env var is set, getCloudEnv() falls back to
http://localhost:8787 — and npm run deploy will happily deploy a
storefront whose API URL points at localhost. This is the exact footgun our
old wrangler.jsonc comment warned about ("Forgetting the override breaks
every storefront API fetch"). Could you make the deploy script abort with a
clear error (or require an explicit --force-local flag) when the resolved
URL is the localhost default? Something like:

const { serverUrl } = getCloudEnv();
if (serverUrl === "http://localhost:8787") {
  console.error(
    "COD_SERVER_URL resolved to the localhost default.\n" +
    "Set COD_SERVER_URL in the root .env (see .env.example) before deploying,\n" +
    "or pass --force-local to deploy anyway."
  );
  process.exit(1);
}

3. The KV namespace keys are dead config

COD_KV_RATE_LIMIT_ID and COD_KV_OAUTH_ID are read by cloud-env.mjs
and exported by getCloudEnv(), but nothing consumes them — the KV
bindings still live only in the gitignored wrangler.toml files. For now
we'd prefer to drop those two keys (they can be re-added when something
actually consumes them), so the .env.example only promises what the code
delivers. Keeping them would invite "I set it in .env but nothing happened"
confusion.

Smaller items (nice-to-have, not blocking):

  • wrangler.jsonc: with the vars block removed, local dev only works
    because src/core/api/client.ts has a code-level fallback
    (?? "http://localhost:8787"). That fallback exists, so the behavior is
    correct — but your comment says local dev reads the value from
    .dev.vars, which isn't accurate (.dev.vars is not wired into the
    astro env schema for this variable). Could you adjust the comment to
    describe the real mechanism? Accurate comments matter a lot in the core.
  • Docs drift: our AGENTS.md, README.md, and the setup skill all still
    reference codflow-os-db directly. A short docs pass updating them to
    mention the root .env would complete the story — happy to help scope
    that if useful.
  • d1.mjs: args containing double quotes would break the shell command
    (the quoting only wraps args with whitespace). A spawn-based invocation
    would remove the shell-quoting class of bugs entirely — optional.
  • Five files are missing trailing newlines (.env.example, deploy.mjs,
    cloud-env.mjs, d1.mjs, wrangler.jsonc).

Once the rebase and the deploy guard are in, we'll re-run the full test
suite on our side and merge. Thanks again for putting this together — the
direction is exactly right, and it's a great first contribution to the
project.

…t enhancements

# Conflicts:
#	cod-astro/theme01/package.json
#	cod-astro/theme01/wrangler.jsonc
@mouradsme
mouradsme force-pushed the improvement/unified-codflow-cloud-resource-values branch from 253968a to 795e060 Compare September 8, 2026 15:13
@bighadj22
bighadj22 merged commit f700937 into bighadj22:main Sep 9, 2026
3 checks passed
bighadj22 added a commit that referenced this pull request Sep 9, 2026
docs: align setup skill, docs and security suite with unified .env cloud config (#100 follow-up)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants