fix(platform): seed new orgs from builtin catalog, not default workspace#1751
Merged
Conversation
scaffoldNewOrganization sourced from `domain.resolve('default')` — the
default org's writable workspace. That dir doubles as both the seeded
shipped-templates location and a mutable workspace, so anything created
while using the default org (test workflows, scratch agents) got
permanently copied into every newly-scaffolded org. For agents and
providers — which use raw `<slug>/` per-org subdirs with no `@` marker —
this also leaked other tenants' subdirs into new orgs, since copyTree's
`@`-skip didn't cover them.
Source the scaffold from the immutable builtin catalog instead:
- Add `*_BUILTIN_DIR` ENV declarations to the platform Dockerfile
(stage 4 + stage 5 squash). The platform entrypoint already pushes
all platform env vars into Convex's deployment env, which is what
Node actions actually read.
- Extend the DOMAINS table in scaffold.ts with a `builtinEnv` per
domain and read `process.env[builtinEnv] ?? domain.resolve('default')`
in scaffoldNewOrganization. The fallback preserves current behavior
for local `bun dev` (where `*_BUILTIN_DIR` is unset and
`examples/{domain}` is intentionally the catalog) and degrades
gracefully on rollback to a pre-fix platform image.
- In copyTree, swap `stat` → `lstat` and skip symbolic links — defence
in depth so a symlink ever planted in any catalog dir can't escape.
No data backfill: the per-domain `dirHasFiles` guard keeps scaffolding
idempotent, so existing orgs keep whatever they already have. Only orgs
created after this change start clean.
branding and retention are intentionally not in scope: branding is
global by design (all readers hardcode 'default') and retention uses a
different per-org shape (single `{slug}.json` with explicit fallback to
`default.json` at read time), so neither exhibits the leak this targets.
Tests cover catalog-source isolation, the agents cross-tenant leak,
symlink rejection, dev fallback, the @/dot/secrets skips, per-domain
idempotency, and the default-org early return.
Contributor
📝 WalkthroughWalkthroughThis PR introduces immutable per-domain builtin catalog directories for organization scaffolding. Docker environment variables now declare five Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![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: 2
🤖 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 `@services/platform/convex/organizations/scaffold.test.ts`:
- Around line 28-29: The test uses unsafe assertion casts around
scaffoldNewOrganization and passes "{} as never" into scaffoldHandler; remove
those casts and instead provide properly typed mocks so the handler and its
calls type-check. Import or reference the ActionConfig/handler input types used
by scaffoldNewOrganization, declare scaffoldHandler by typing
scaffoldNewOrganization with that type (or use TypeScript's "satisfies" to
assert the proper shape) rather than "as unknown as ActionConfig", and replace
every "{} as never" passed into scaffoldHandler(...) with concrete mock objects
that match the handler input/Context type (create small typed fixtures for the
request, ctx, and params expected by scaffoldHandler). Ensure all uses of
scaffoldHandler(...) use those typed mocks so no assertion casts remain.
In `@services/platform/convex/organizations/scaffold.ts`:
- Around line 260-261: The code sets sourceDir using
process.env[domain.builtinEnv] ?? domain.resolve('default'), which treats an
empty string as valid and can cause copyTree to read an unintended path; change
the selection to treat empty or whitespace-only env values as unset (e.g., check
process.env[domain.builtinEnv] is non-empty after trim and only then use it,
otherwise call domain.resolve('default')) and update scaffold.test.ts to add a
test case where the *_BUILTIN_DIR env var is set to '' (and to whitespace) to
assert the fallback to domain.resolve('default'); reference the symbols
domain.builtinEnv, domain.resolve('default'), sourceDir, copyTree, and
scaffold.test.ts when making the change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f2c3cc60-32a0-409e-8dec-e6fffba7d5da
📒 Files selected for processing (3)
services/platform/Dockerfileservices/platform/convex/organizations/scaffold.test.tsservices/platform/convex/organizations/scaffold.ts
Comment on lines
+28
to
+29
| const scaffoldHandler = (scaffoldNewOrganization as unknown as ActionConfig) | ||
| .handler; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current assertion usage in this test file.
rg -nP '\sas\s+(unknown|never|[A-Z])|as unknown as' services/platform/convex/organizations/scaffold.test.tsRepository: tale-project/tale
Length of output: 615
Remove as/as unknown as/as never casts from scaffold.test.ts; type the mocks directly
services/platform/convex/organizations/scaffold.test.ts still contains disallowed TypeScript assertion casts:
- Line 28:
scaffoldNewOrganization as unknown as ActionConfig - Lines 94, 119, 144, 162, 187, 206, 218:
{} as neverpassed intoscaffoldHandler(...)
Problematic cast
const scaffoldHandler = (scaffoldNewOrganization as unknown as ActionConfig)
.handler;Replace these with a typed mock surface/contract (e.g., define the handler input type and use satisfies/proper mock objects) so .handler and scaffoldHandler(...) calls type-check without any as assertions.
🤖 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 `@services/platform/convex/organizations/scaffold.test.ts` around lines 28 -
29, The test uses unsafe assertion casts around scaffoldNewOrganization and
passes "{} as never" into scaffoldHandler; remove those casts and instead
provide properly typed mocks so the handler and its calls type-check. Import or
reference the ActionConfig/handler input types used by scaffoldNewOrganization,
declare scaffoldHandler by typing scaffoldNewOrganization with that type (or use
TypeScript's "satisfies" to assert the proper shape) rather than "as unknown as
ActionConfig", and replace every "{} as never" passed into scaffoldHandler(...)
with concrete mock objects that match the handler input/Context type (create
small typed fixtures for the request, ctx, and params expected by
scaffoldHandler). Ensure all uses of scaffoldHandler(...) use those typed mocks
so no assertion casts remain.
Comment on lines
+260
to
+261
| const sourceDir = | ||
| process.env[domain.builtinEnv] ?? domain.resolve('default'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify env-based source selection and whether empty-value behavior is covered by tests.
rg -n "process\\.env\\[domain\\.builtinEnv\\].*\\?\\?|builtinEnv|sourceDir" services/platform/convex/organizations/scaffold.ts
rg -n "empty|unset|BUILTIN_DIR" services/platform/convex/organizations/scaffold.test.tsRepository: tale-project/tale
Length of output: 1509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- services/platform/convex/organizations/scaffold.ts (source selection) ----"
sed -n '230,310p' services/platform/convex/organizations/scaffold.ts
echo "---- services/platform/convex/organizations/scaffold.test.ts (env-related tests) ----"
sed -n '1,260p' services/platform/convex/organizations/scaffold.test.tsRepository: tale-project/tale
Length of output: 10300
Guard against empty *_BUILTIN_DIR values when selecting sourceDir.
process.env[domain.builtinEnv] ?? domain.resolve('default') treats '' as a valid value, so sourceDir can become '' and copyTree will read from an unintended path. scaffold.test.ts only covers the “unset” fallback case (it deletes env vars), not the empty-string case.
Proposed fix
- const sourceDir =
- process.env[domain.builtinEnv] ?? domain.resolve('default');
+ const builtinDir = process.env[domain.builtinEnv];
+ const sourceDir =
+ builtinDir && builtinDir.trim().length > 0
+ ? builtinDir
+ : domain.resolve('default');📝 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
| const sourceDir = | |
| process.env[domain.builtinEnv] ?? domain.resolve('default'); | |
| const builtinDir = process.env[domain.builtinEnv]; | |
| const sourceDir = | |
| builtinDir && builtinDir.trim().length > 0 | |
| ? builtinDir | |
| : domain.resolve('default'); |
🤖 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 `@services/platform/convex/organizations/scaffold.ts` around lines 260 - 261,
The code sets sourceDir using process.env[domain.builtinEnv] ??
domain.resolve('default'), which treats an empty string as valid and can cause
copyTree to read an unintended path; change the selection to treat empty or
whitespace-only env values as unset (e.g., check process.env[domain.builtinEnv]
is non-empty after trim and only then use it, otherwise call
domain.resolve('default')) and update scaffold.test.ts to add a test case where
the *_BUILTIN_DIR env var is set to '' (and to whitespace) to assert the
fallback to domain.resolve('default'); reference the symbols domain.builtinEnv,
domain.resolve('default'), sourceDir, copyTree, and scaffold.test.ts when making
the change.
…edges Consolidate the 5 per-domain *_BUILTIN_DIR envs into one root-with-derivation TALE_CONFIG_BUILTIN_DIR, mirroring the existing TALE_CONFIG_DIR/<domain>/ pattern used for the writable side; rename the FS layout /app/<domain>-builtin/ → /app/builtin/<domain>/ to match /app/data/<domain>/. One env knob, one source of truth. Harden scaffold idempotency and observability: - dirHasFiles now only ignores atomicWrite .tmp orphans; .history/ counts as occupied so re-scaffold (e.g. via backfill migration) cannot silently write the catalog on top of a user's surviving edit trail. - env-set-but-missing catalog path now logs a loud console.error with the env name and resolved path; was previously a silent zero-seed at copyTree's ENOENT branch and invisible to operators on image version skew. Tests cover .history-only target, .tmp orphan retry, and missing-catalog misconfig.
…fresh → --override Default tale deploy now docker-cp's host workspace into the convex container without clobbering files the operator has edited through the UI. Mirrors the .history-based skip logic the convex entrypoint's seed loops have always used, so the two write paths apply the same "user edit wins" rule. Coverage: agents, workflows, providers (config files; encrypted secrets were already always-preserved), branding. Integrations has no history mechanism today and stays at the unconditional overwrite — separate work. Flag rename --fresh → --override: the old name described the convex entrypoint behavior (force re-seed builtin) but missed that docker cp always clobbered regardless. New semantics are unified — --override bypasses .history-based protection on both layers. Encrypted provider secrets and UI-uploaded skill bundles stay preserved even under --override (no host source to re-derive from). Refactor: stageProvidersWithoutConflictingSecrets + stageSkillsWithoutConflictingBundles collapse into one stageWithSkips helper driven by a per-reason SkipEntry list. New listContainerHistorySlugs reuses the same find-in-container pattern. Preserved-message logging now groups by reason so operators see which protection kicked in and the correct escape hatch per reason. tale start --fresh is unchanged — its scope is only the convex entrypoint (no docker cp from host CWD), and "fresh seed" still describes it accurately.
The flag's apparent purpose — re-seed builtin configs after an upgrade —
is already automatic: the convex entrypoint's seed marker is version-scoped
(/app/data/.seeded-${TALE_VERSION}), so a new version always re-runs the
seed and picks up new builtin templates, with per-file skip-if-exists
preserving local edits.
What remained was a niche "reset builtin-originated config to factory while
keeping custom files" operation behind a misleadingly-named flag that
silently overwrote local dev edits — the same footgun class just removed
from tale deploy. Operators who want that reset can delete the specific
files (or git checkout) and restart; the underlying FORCE_SEED env stays
available via compose override for the rare power-user case.
Removes the flag, its plumbing through start.ts and the dev compose
generator, and the documented line in all three READMEs.
…override Replace the per-slug `.history` skip machinery with a two-mode model: - default `tale deploy`: do not push host config at all. The convex container self-seeds builtin defaults on first start and UI edits stay authoritative; host config is pushed only on explicit `--override`. - `tale deploy --override`: overwrite container config from the host workspace, always preserving `*.secrets.json` files and `.history/` directories. Since `docker cp` is additive, preservation is just excluding those paths from the staged copy via an fs.cp filter — no container queries needed. This deletes buildSkipsForDir, HISTORY_SLUG_TO_RELPATH, logPreserved, findInContainer and the listContainer* helpers, and resolves several review findings at once: providers' inert `.history` guard, the unprotected integrations/per-org paths, the skills special-case, and host-committed `.history/` polluting the container edit trail. Also fixes the stage temp-dir leak on copy failure (register before I/O). Drop the FORCE_SEED coupling from `--override`: it re-seeded from the builtin catalog (a different source than the host workspace) and could resurrect files deleted locally. `--override` is now purely host- authoritative. The entrypoint FORCE_SEED branches are left in place (now dead-on-deploy, harmless). scaffold: - fix EACCES double-log: a non-ENOENT stat error no longer also logs a false "does not exist" line. - harden copyTree: agents/providers are flat domains and never recurse into subdirs, making the raw-slug cross-tenant leak structurally impossible on any source path (catalog or dev fallback), with no change to dev ergonomics.
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.
Why
scaffoldNewOrganizationsourced fromdomain.resolve('default')— thedefault org's writable workspace. That dir doubles as both the seeded
shipped-templates location and a mutable workspace, so anything created
while using the default org (test workflows, scratch agents) got
permanently copied into every newly-scaffolded org. Users saw stale
"Default / testing 2 / testing this" workflows in the Create automation
→ From template dialog for orgs they never touched.
For agents and providers this was worse: they use raw
<slug>/per-org subdirs with no
@marker, socopyTree's@-skip didn'tcatch them — scaffolding a new org would recursively copy other
tenants' agent/provider subdirs into the new org. Already flagged in
the
copyTreecomment as a known leak awaiting a domain-aware fix.Diagnosis confirmed it is not a live cross-org query leak:
listWorkflowsreads only its own dir,@-prefixed foreign paths failslug validation, and
resolveOrgSlugthrows rather than defaulting.What
Source the scaffold from the immutable builtin catalog instead of the
default org's workspace.
services/platform/Dockerfile— add five*_BUILTIN_DIRENVlines (stage 4 + stage 5 squash). The platform entrypoint's env-sync
loop pushes all platform env vars into Convex's deployment env, which
is what Node actions actually read via
process.env.services/platform/convex/organizations/scaffold.ts—DOMAINStable withbuiltinEnvper domain.scaffoldNewOrganization, readprocess.env[builtinEnv] ?? domain.resolve('default'). The fallbackpreserves behavior for local
bun dev(where*_BUILTIN_DIRisunset and
examples/{domain}is intentionally the catalog) anddegrades gracefully on rollback to a pre-fix platform image.
copyTree, swapstat→lstatand skip symbolic links —defence-in-depth so a symlink ever planted in any catalog dir can't
escape. Matches the existing precedent at
services/platform/convex/skills/file_utils.ts:249.services/platform/convex/organizations/scaffold.test.ts— newfile, 7 vitest cases covering catalog-source isolation, the agents
cross-tenant leak, symlink rejection, dev fallback, the
@/.history/*.secrets.jsonskips, per-domain idempotency, and thedefault-org early return.
Out of scope (explicit)
dirHasFilesguard keepsscaffolding idempotent, so existing orgs keep whatever junk they
already have. Only orgs created after this lands start clean. A
cleanup pass for existing orgs is a separate task.
has been created in it — that's its own workspace and the leak only
ever was about propagation to other orgs.
brandingandretentiondomains. Branding is global by design(all readers hardcode
'default'); retention uses a different per-orgshape — a single
{slug}.jsonper org with explicit fallback todefault.jsonat read time — so neither exhibits this leak class.Notes on degradation modes
*_BUILTIN_DIRvars via the entrypoint's "remove vars not seen onplatform" step. Scaffold then falls back to
domain.resolve('default')— the original pre-fix behavior.Intentional graceful degradation; no flapping in normal forward-only
deploys.
ENV_SYNC_DENYLIST(would block push too) andORPHAN_DERIVED(removes matching values) are both the wrong toolsfor the job — left as-is.
skillswas added toDOMAINS):backfill_skill_scaffoldingre-invokesscaffoldNewOrganization; with the fix, domains that don't alreadyexist for an org are now seeded from the catalog instead of the
default workspace. Strict improvement.
bun devdoesn't set*_BUILTIN_DIR, so the leakcan still occur locally if a developer creates content in their
local default org. Acceptable: dev is single-developer and
examples/{domain}is intentionally the catalog there. Flagged sothe next person doesn't chase a "but it works in prod" ghost.
Pre-PR checklist
bun run check(format, lint, typecheck, all tests). Onlyfailure is the preexisting
@tale/docslink-check (3 brokenlinks from
fr/legal/subprocessors.mdto a not-yet-translatedfr/legal/data-processing-agreement) — verified to fail on aclean
mainwith the same 3 failed / 200 passed counts;unrelated to this change.
services/platform/messages/{en,de,fr}.json— N/A(no user-facing strings touched).
/docs/{en,de,fr}/for every user-visible change —N/A (internal scaffolding behavior; no UI/CLI surface change).
(no docs pages touched).
bun run --filter @tale/docs lintandbun run --filter @tale/docs test— covered bybun run check;preexisting failure noted above.
README.md/README.de.md/README.fr.md— N/A.Summary by CodeRabbit
New Features
Bug Fixes
Chores