Skip to content

refactor(meta): dual-store follow-up with injectappmodel and FlushEffective - #249

Merged
buke merged 12 commits into
mainfrom
feat/meta-dual-store-followup
Aug 6, 2026
Merged

refactor(meta): dual-store follow-up with injectappmodel and FlushEffective#249
buke merged 12 commits into
mainfrom
feat/meta-dual-store-followup

Conversation

@buke

@buke buke commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

  • Slim meta.Entities(), unexport dual-store migrate helpers, delete unused metaeff, and add FlushEffective / CatalogEntities.
  • Unify FieldDefault / AppSetting C2 inject into injectappmodel (InjectAppModels / SupersedeInjectAppModels / BundleInjectAppModels); supersede deletes declarations only.
  • Add EnsureAbstractModel / ReplaceModuleDeclarations; flush effective once at persist (and i18n ACL) boundaries.
  • Unexport Raw* types; tests seed/query via declaration facades and raw table helpers.

Test plan

  • go test ./pkg/meta/ -count=1
  • go test ./internal/module/artifact/build/injectappmodel/ ./internal/module/artifact/build/backend/ -count=1
  • go test ./internal/module/lifecycle/ ./internal/i18n/models/ ./internal/module/evolution/schema/ -count=1
  • Spot-check install path: handwritten AppSetting supersedes prior __generated__ declaration without double Flush mid-supersede
  • Confirm AutoMigrate still creates meta_raw_* via CatalogEntities / DualStoreRawEntities

Made with Cursor


PR Type

Enhancement


Description

  • Go Core: Unify FieldDefault and AppSetting inject logic into a new injectappmodel package with a central Spec registry and Session.

  • Go Core: Unexport raw dual-store types and migrate helpers in pkg/meta, introducing clean declaration facades (EnsureAbstractModel, ReplaceModuleDeclarations, FlushEffective).

  • Go Core: Optimize effective projection recomputation by flushing once at persist/install boundaries, and remove the redundant metaeff package.

  • TS Modules & Compliance: No TypeScript changes in modules/; all 19 new Go files include SPDX LGPL-3.0 headers and are covered by unit tests.


File Walkthrough

Relevant files
Refactor
8 files
i18n_meta.go
Use EnsureAbstractModel facade and FlushEffective for I18n ACL seeding
+12/-12 
app_setting.go
Delegate AppSetting C2 inject and plan management to injectappmodel
+96/-298
builder.go
Unify app model inject and declaration flushing during module builds
+29/-97 
field_default.go
Delegate FieldDefault C2 inject and plan management to injectappmodel
+94/-304
modulemanager.go
Include DualStoreRawEntities in default entity list for AutoMigrate
+3/-2     
declaration.go
Unexport RawModel struct and adjust ListDeclarations to rawModel
+18/-18 
recompute.go
Add FlushEffective alias and unexport internal tree deletion helpers
+18/-19 
model.go
Expose CatalogEntities and DualStoreRawEntities for schema migrations
+19/-24 
Tests
2 files
declaration_test_helpers.go
Add declaration test helpers for raw model seeding and assertion
+111/-0 
injectappmodel_test.go
Add unit tests for injectappmodel session, plan, and supersede
+299/-0 
Enhancement
9 files
inject_app_models.go
Add builder methods adapting ModuleBuilder to injectappmodel session
+56/-0   
inject_host.go
Implement injectappmodel Host interface adapter for ModuleBuilder
+100/-0 
bundle.go
Add multi-app bundle source registration for inject models
+87/-0   
inject.go
Implement Decide, ApplyInject, and Validate for injectable app models
+204/-0 
session.go
Manage per-build inject plans, schedules, and virtual import paths
+136/-0 
spec.go
Define Spec registry for injectable application models     
+88/-0   
supersede.go
Implement declaration tree deletion for superseded generated models
+81/-0   
facade.go
Add public EnsureAbstractModel and ReplaceModuleDeclarations facades
+119/-0 
raw_facade.go
Expose raw table constants and helper queries without exporting Raw*
+45/-0   
Miscellaneous
1 files
recompute.go
Remove obsolete metaeff package in favor of pkg/meta APIs
+0/-23   
Additional files
51 files
i18n_meta_test.go +2/-2     
app_setting_coverage_test.go +39/-94 
app_setting_test.go +4/-1     
builder_test.go +65/-52 
field_default_coverage_test.go +23/-82 
field_default_test.go +5/-1     
compat.go +76/-0   
doc.go +7/-0     
helpers.go +108/-0 
host.go +19/-0   
plan.go +13/-0   
register_builtin.go +21/-0   
source.go +38/-0   
bundles.go +1/-1     
bundles_app_setting_test.go +2/-2     
bundles_field_default_test.go +2/-2     
install_module_test.go +1/-1     
install_prefetch_test.go +1/-1     
module_index_sync_test.go +10/-10 
modulemanager_coverage_test.go +3/-3     
modulemanager_fastfail_test.go +2/-2     
uninstaller_clean_models_test.go +92/-97 
uninstaller_model_data_test.go +17/-17 
upgrade_uninstall_commit_test.go +1/-1     
recompute_test.go +0/-129 
acl_remap.go +2/-2     
acl_remap_coverage_test.go +28/-28 
acl_remap_test.go +5/-5     
declaration_test.go +6/-6     
dual_store_migrate.go +34/-38 
dual_store_migrate_coverage_test.go +47/-47 
dual_store_migrate_test.go +15/-15 
effective_merge.go +9/-9     
effective_merge_coverage_test.go +11/-11 
effective_merge_test.go +14/-14 
extends_expand.go +3/-3     
extends_expand_coverage_test.go +23/-23 
extends_expand_test.go +3/-3     
facade_test.go +70/-0   
meta_model.go +2/-2     
meta_raw_argument.go +4/-4     
meta_raw_decorator.go +7/-7     
meta_raw_field.go +7/-7     
meta_raw_field_test.go +8/-8     
meta_raw_model.go +6/-6     
meta_raw_parameter.go +4/-4     
meta_raw_service.go +7/-7     
meta_raw_typeparameter.go +4/-4     
model_test.go +33/-17 
recompute_coverage_test.go +67/-67 
recompute_test.go +11/-11 

Summary by CodeRabbit

  • New Features

    • Added unified application-model injection for builds and bundles.
    • Automatically generates and manages virtual application-model sources.
    • Added support for injection scheduling, validation, superseding, and cleanup.
    • Added bundle handling that combines and deduplicates model imports.
  • Improvements

    • Improved metadata declaration replacement and effective-model flushing.
    • Simplified metadata catalog and raw-model management.
    • Added safer rollback and cleanup when build or injection operations fail.

…ective

- Slim Entities() to effective catalog only, unexport migrate helpers, and remove unused metaeff.

- Unify FieldDefault/AppSetting C2 inject into injectappmodel with InjectAppModels, SupersedeInjectAppModels, and BundleInjectAppModels.

- Flush effective once at persist and i18n boundaries; add EnsureAbstractModel and ReplaceModuleDeclarations facades.

- Unexport Raw* types and route tests through declaration helpers and table-name APIs.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR introduces a unified application-model injection system with registry-based scheduling, generated sources, validation, supersession, and bundle support. It also adds metadata facades, internalizes raw persistence APIs, adds effective flushing, and updates lifecycle database handling.

Changes

Dual-store metadata and application-model injection

Layer / File(s) Summary
Metadata facades and internal APIs
pkg/meta/..., internal/i18n/models/...
Raw metadata types and recomputation helpers are package-private. New facades support abstract-model registration, module declaration replacement, raw-table operations, catalog entity selection, and effective flushing. i18n registration uses the new facade and flushes the effective projection before ACL seeding.
Shared application-model injection engine
internal/module/artifact/build/injectappmodel/...
A registry and session now coordinate FieldDefault and AppSetting specifications. The engine decides injection plans, claims application ownership, generates virtual sources, validates duplicates, supersedes generated declarations, and aggregates bundle effects.
ModuleBuilder integration
internal/module/artifact/build/backend/...
ModuleBuilder now uses one injection session and registry. Build, validation, persistence, supersession, schedule cleanup, and bundle flows delegate to unified application-model APIs.
Catalog migration and lifecycle support
internal/module/lifecycle/...
Lifecycle setup uses catalog entity migrations and raw metadata helpers. Bundle processing calls one application-model injection operation for merged owners. Tests update raw model persistence and cleanup to use metadata facade APIs.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ModuleBuilder
  participant InjectionSession
  participant Registry
  participant MetadataStore
  ModuleBuilder->>InjectionSession: Decide and inject application models
  InjectionSession->>Registry: Claim application ownership
  InjectionSession->>MetadataStore: Load declarations and generated paths
  InjectionSession->>InjectionSession: Generate virtual source and record imports
  ModuleBuilder->>MetadataStore: Replace module declarations and flush effective keys
  ModuleBuilder->>Registry: Release schedules on failure or cleanup
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the dual-store refactor, injectappmodel integration, and FlushEffective change.
Description check ✅ Passed The description explains the objectives, major changes, test plan, affected areas, and intended behavior with sufficient detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/meta-dual-store-followup

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Unsafe sync.Map Copy

Spec contains scheduled sync.Map as a value field. Register(spec Spec) accepts Spec by value, and Specs() []Spec returns []Spec by value, making copies of sync.Map. In Go, copying a sync.Map after use causes race conditions and copylock violations. scheduled should be a pointer *sync.Map or Specs() should return pointers []*Spec.

// Spec describes one injectable app-scoped model (FieldDefault, AppSetting, …).
type Spec struct {
	ModelName        string
	GeneratedRelPath string
	DuplicateCode    string
	BaseModelFile    string // relative under modules, e.g. core/service/orm/model/field_default_base_model.ts
	SoftDeleteFalse  bool   // AppSetting: emit softDelete: false in @Model options
	// ForeignClaimOnOwnerReinject: when DB virtual rows belong to this module but another
	// in-process builder holds the schedule claim, still NeedInject without adopting release.
	ForeignClaimOnOwnerReinject bool

	scheduled sync.Map // process-wide NeedInject dedup keyed by application
}

var (
	specsMu   sync.RWMutex
	specOrder []string
	specsBy   = map[string]*Spec{}
)

// Register adds a Spec to the process-wide registry. ModelName must be unique.
func Register(spec Spec) {
	specsMu.Lock()
	defer specsMu.Unlock()
	if _, exists := specsBy[spec.ModelName]; exists {
		panic("injectappmodel: duplicate Register for " + spec.ModelName)
	}
	s := spec
	specsBy[spec.ModelName] = &s
	specOrder = append(specOrder, spec.ModelName)
}

// Specs returns registered specs in registration order.
func Specs() []Spec {
	specsMu.RLock()
	defer specsMu.RUnlock()
	out := make([]Spec, 0, len(specOrder))
	for _, name := range specOrder {
		if s, ok := specsBy[name]; ok {
			out = append(out, *s)
		}
	}
	return out
}
Duplicate Entry Imports

applyInject calls sess.host.EntryPointImports() and appends sess.allInjectPaths(). Because SetEntryPointImports mutates the host's entry point imports, injecting a second Spec (e.g., AppSetting after FieldDefault) reads the updated entry point imports and appends allInjectPaths() (which already includes FieldDefault), resulting in duplicate path entries in the host build configuration.

func applyInject(sess *Session, spec *Spec, plan Plan) error {
	if sess == nil || spec == nil || !plan.NeedInject {
		return nil
	}
	mod := sess.host.Module()
	if mod == nil {
		return nil
	}
	if strings.TrimSpace(mod.Path) == "" {
		return xfmt.Errorf("%s inject requires a non-empty module path", spec.ModelName)
	}
	path := generatedPath(spec, mod.Path)
	sess.rememberInjectPath(spec.ModelName, path)

	imports := append(sess.host.EntryPointImports(), sess.allInjectPaths()...)
	sess.host.SetEntryPointImports(imports)

	modulesPath := strings.TrimSpace(sess.host.ModulesPath())
	if modulesPath == "" {
		modulesPath = filepath.Dir(mod.Path)
	}
	source := generatedSource(spec, modulesPath, mod.ApplicationStr)
	sess.host.RegisterVirtualSource(path, source)
	return nil
}

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent runtime panic on nil maps

Ensure lastInjectPath and injectPaths maps are initialized before key assignment in
rememberInjectPath. If a Session instance is created without NewSession (e.g.,
zero-value struct in tests or adapters), writing to uninitialized maps will cause a
nil map assignment runtime panic.

internal/module/artifact/build/injectappmodel/session.go [85-97]

 func (s *Session) rememberInjectPath(modelName, path string) {
 	path = strings.TrimSpace(path)
 	if s == nil || path == "" {
 		return
+	}
+	if s.lastInjectPath == nil {
+		s.lastInjectPath = make(map[string]string)
+	}
+	if s.injectPaths == nil {
+		s.injectPaths = make(map[string][]string)
 	}
 	s.lastInjectPath[modelName] = path
 	for _, existing := range s.injectPaths[modelName] {
 		if existing == path {
 			return
 		}
 	}
 	s.injectPaths[modelName] = append(s.injectPaths[modelName], path)
 }
Suggestion importance[1-10]: 6

__

Why: Guarding against uninitialized maps in rememberInjectPath prevents potential runtime panics if a Session struct is instantiated directly without NewSession.

Low

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🧹 Nitpick comments (14)
internal/module/artifact/build/injectappmodel/injectappmodel_test.go (3)

46-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reset the process-wide claims after each test.

newTestSession calls ResetScheduledForTest at setup only. spec.scheduled is process-wide, so a claim from one test stays visible to any later test that does not use newTestSession. Register a cleanup to keep tests independent.

♻️ Proposed fix
 	t.Helper()
 	ResetScheduledForTest()
+	t.Cleanup(ResetScheduledForTest)
🤖 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 `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go` around
lines 46 - 63, Update newTestSession to register t.Cleanup that calls
ResetScheduledForTest after the test completes, while retaining the existing
setup reset so process-wide spec.scheduled claims cannot leak between tests.

189-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Handle the lookup result at Line 198.

spec, _ := specByName("FieldDefault") discards ok. If the spec name changes, spec is nil and the next line panics with an unclear failure. Other tests in this file already fail fast with t.Fatal. Use specByNameOrPanic here.

🤖 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 `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go` around
lines 189 - 209, Update TestReleaseSchedules_ClearsClaim to use
specByNameOrPanic("FieldDefault") instead of discarding the lookup result from
specByName, so a missing spec fails fast with the established test error rather
than panicking on spec.scheduled.

250-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the entry-point imports, not only the virtual source.

fakeHost records entryImports and setImportCalls, but no test reads them. InjectAppModels runs both specs and each spec appends the full cumulative path set, so host.entryImports can hold duplicates. Add an assertion on host.entryImports here. That test then pins the duplication defect flagged in inject.go Line 171.

🤖 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 `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go` around
lines 250 - 266, Extend TestInjectRegistersVirtualSource to assert
host.entryImports contains the expected entry-point import paths after
InjectAppModels, accounting for the cumulative duplicate entries produced across
both specs. Use the recorded setImportCalls or existing module/spec path helpers
to verify the expected duplicated imports and preserve the current
virtual-source assertions.
internal/module/artifact/build/injectappmodel/bundle.go (1)

45-87: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

bundleSpec runs one DB query per application per spec.

dbLoadModels executes inside the module loop for every registered spec. With N applications and M specs, the bundle path issues N×M queries. Consider loading declarations once per spec with a single query filtered by model name, then grouping by application in memory.

🤖 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 `@internal/module/artifact/build/injectappmodel/bundle.go` around lines 45 -
87, Update bundleSpec to load all existing declarations for the current spec
with one dbLoadModels query filtered by spec.ModelName, then group the returned
models by application in memory before iterating modules. Use each module’s
application group for handwrittenModels and generatedModels decisions,
preserving path registration and skipping behavior while eliminating the
per-application database query.
internal/module/artifact/build/injectappmodel/source.go (1)

15-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Validate Spec fields at registration instead of trusting them here.

application is escaped with strconv.Quote, which is correct. spec.ModelName is interpolated raw at Line 35 inside a single-quoted literal and at Line 36 as the class name and base import name. Today Register is called only from register_builtin.go with literal names, so no external input reaches this template. A future registration with a quote, a space, or a non-identifier character would emit source that fails to parse, and the failure would appear as an esbuild error far from the cause. Add a validation check in Register for ModelName, GeneratedRelPath, and BaseModelFile.

Also note Line 16: filepath.Clean("") returns ".", so an empty modulesPath produces relative imports. Both current callers substitute a fallback before the call, so this is defensive only.

🤖 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 `@internal/module/artifact/build/injectappmodel/source.go` around lines 15 -
37, Add validation in Register for Spec.ModelName, Spec.GeneratedRelPath, and
Spec.BaseModelFile, rejecting empty or invalid values before registration
succeeds; ensure ModelName is a valid identifier suitable for both generated
string literals and class/import-name positions, and the path fields are
non-empty valid relative paths. Also defensively reject an empty modulesPath
before generatedSource constructs imports, rather than allowing
filepath.Clean("") to produce ".".
internal/module/artifact/build/backend/app_setting.go (2)

208-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use appSettingDuplicateCode instead of repeating the literal.

Line 19 defines appSettingDuplicateCode = "APP_SETTING_DUPLICATE", and Spec.DuplicateCode carries the same value. The literal in the format string can drift from the constant that tests assert on.

♻️ Proposed fix
-	return fmt.Errorf("APP_SETTING_DUPLICATE: application %q build produced multiple AppSetting models", app)
+	return fmt.Errorf("%s: application %q build produced multiple AppSetting models", appSettingDuplicateCode, app)
🤖 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 `@internal/module/artifact/build/backend/app_setting.go` at line 208, Update
the duplicate AppSetting error construction in the relevant build flow to use
the existing appSettingDuplicateCode constant instead of the repeated
"APP_SETTING_DUPLICATE" literal, while preserving the current error message and
formatting.

220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

resetAppSettingScheduledAppsForTest now clears every registered spec.

injectappmodel.ResetScheduledForTest iterates all specs. The function name states AppSetting scope, but the call also clears FieldDefault claims. A test that resets AppSetting state between phases now also drops a FieldDefault claim it intended to keep.

Add a per-spec reset in injectappmodel, or rename these helpers to state the global scope.

🤖 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 `@internal/module/artifact/build/backend/app_setting.go` around lines 220 -
222, Update resetAppSettingScheduledAppsForTest to reset only the
AppSetting-related spec instead of calling injectappmodel.ResetScheduledForTest,
which clears all registered specs; add and use a per-spec reset API in
injectappmodel, preserving unrelated FieldDefault claims. Alternatively, rename
the helper and its callers to explicitly reflect global reset scope if that is
the intended behavior.
internal/module/artifact/build/injectappmodel/spec.go (2)

83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ScheduledApps returns a throwaway map for an unknown modelName.

If modelName is not registered, the function returns a fresh &sync.Map{}. Callers then store claims that no other code can observe, and the dedup silently stops working. A typo in a caller string literal produces no error.

Consider returning (*sync.Map, bool), or panicking in this test-only helper so a wrong modelName fails loudly.

🤖 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 `@internal/module/artifact/build/injectappmodel/spec.go` around lines 83 - 88,
The ScheduledApps helper silently returns a throwaway map for unknown model
names, causing stored claims to be lost. Change ScheduledApps to fail loudly for
unregistered modelName values, preferably by panicking in this test-only helper,
while preserving the registered spec.scheduled map behavior.

19-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Keep deduplication state out of Spec. Spec contains a named sync.Map, which must not be copied after first use. Specs() copies each registered Spec at line 47, and Register copies the by-value parameter at line 35; go vet flags these copies. Store one registry-owned map per ModelName and route all accesses, including ScheduledApps, through an accessor.

🤖 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 `@internal/module/artifact/build/injectappmodel/spec.go` around lines 19 - 20,
Remove the named sync.Map field from Spec and maintain deduplication state in
registry-owned storage keyed by ModelName. Update Register and Specs to avoid
copying synchronization state, and route all scheduled-state reads and
writes—including ScheduledApps and NeedInject-related accesses—through a
dedicated accessor that returns the map for the relevant model.
internal/module/artifact/build/backend/field_default_test.go (1)

558-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the effective count before the flush as well.

The comment states the new contract: supersede does not flush, and the persist boundary does. The test proves only the post-flush state. If a future change reintroduces a flush inside supersede, this test still passes.

💚 Proposed addition
 	// EDS-opt-2: supersede does not FlushEffective; persist boundary does.
+	var before int64
+	if err := db.Model(&meta.Model{}).Where("name = ?", "FieldDefault").Count(&before).Error; err != nil {
+		t.Fatalf("count effective before flush: %v", err)
+	}
+	if before != 2 {
+		t.Fatalf("expected effective rows untouched before flush, count=%d", before)
+	}
 	if err := meta.FlushEffective(db, []meta.LogicalKey{{Application: "partner", Name: "FieldDefault"}}); err != nil {
 		t.Fatalf("flush: %v", err)
 	}

Adjust the expected before value to match what seedFieldDefaultDeclaration writes into the effective table.

🤖 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 `@internal/module/artifact/build/backend/field_default_test.go` around lines
558 - 567, Update the test around FlushEffective to count the FieldDefault
effective row before flushing and assert the expected pre-flush count based on
seedFieldDefaultDeclaration. Keep the existing post-flush count assertion,
ensuring the test verifies supersede leaves the effective table unchanged until
the explicit FlushEffective call.
internal/module/artifact/build/backend/field_default.go (1)

26-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

field_default.go and app_setting.go are now near-identical wrappers.

Both files repeat the same plan converter, sync helper, release helper, and six *ForTest shims, and they differ only by the model-name literal. The PR moves the real logic into injectappmodel, so this remaining layer can collapse into one generic helper set parameterized by modelName. Doing so removes the risk that a later fix lands in one file only.

This is a follow-up cleanup, not a blocker for this PR.

🤖 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 `@internal/module/artifact/build/backend/field_default.go` around lines 26 -
104, Collapse the duplicated FieldDefault wrapper logic in fieldDefaultPlanFrom,
toInject, syncFieldDefaultFromSession, rememberFieldDefaultInjectPath,
releaseFieldDefaultSchedule, and the six test shim helpers by introducing a
shared model-name parameterized helper path, matching the existing
app_setting.go pattern. Keep the current FieldDefault behavior intact while
replacing the repeated literal-based wrappers with one reusable implementation
so future changes only land in one place.
internal/module/artifact/build/backend/declaration_test_helpers.go (1)

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the positional string parameters with a struct.

seedVirtualDeclarationTree takes 11 string parameters and seedVirtualDeclarationErrorBranchTree takes 9. Call sites pass bare literals such as "arg1", "tp1", "p1". If two arguments are swapped, the code still compiles and the test still passes with wrong identifiers.

Pass an options struct with named fields, and let the helper default any empty field.

Also applies to: 67-71

🤖 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 `@internal/module/artifact/build/backend/declaration_test_helpers.go` around
lines 21 - 25, Replace the positional string arguments of
seedVirtualDeclarationTree and seedVirtualDeclarationErrorBranchTree with an
options struct using named fields. Update all call sites to construct the
struct, preserving existing values while allowing omitted fields to remain
empty. Add defaults inside both helpers for any empty option fields, and remove
the long positional parameter lists.
internal/module/artifact/build/backend/inject_host.go (1)

13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a compile-time Host assertion and name the repeated plugin interfaces.

injectHost must satisfy injectappmodel.Host. Today a signature change in Host surfaces only at the call site in ensureInjectSession. A package-level assertion documents the contract and fails at the definition.

The two anonymous interfaces are each written twice, once per plugin field.

♻️ Proposed refactor
 // injectHost adapts ModuleBuilder to injectappmodel.Host.
 type injectHost struct {
 	b *ModuleBuilder
 }
+
+var _ injectappmodel.Host = injectHost{}
+
+type entryPointImportsSetter interface {
+	SetEntryPointImports([]string)
+}
+
+type virtualSourceRegistrar interface {
+	RegisterVirtualSource(path string, contents string)
+}

Then assert against entryPointImportsSetter and virtualSourceRegistrar in both methods.

Also applies to: 45-71

🤖 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 `@internal/module/artifact/build/backend/inject_host.go` around lines 13 - 15,
Add a package-level compile-time assertion that injectHost implements
injectappmodel.Host so any Host signature change fails at the type definition
instead of only at ensureInjectSession. Also extract the repeated anonymous
plugin interfaces used in injectHost’s methods into named interfaces, and reuse
entryPointImportsSetter and virtualSourceRegistrar in both places rather than
duplicating the same interface literals.
internal/module/artifact/build/backend/app_setting_coverage_test.go (1)

429-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove redundant meta.EnsureDualStoreTables() calls.

meta.CatalogEntities() already includes both effective and raw dual-store entities. The explicit calls at Lines 492–495 and 640–643 are redundant.

🤖 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 `@internal/module/artifact/build/backend/app_setting_coverage_test.go` at line
429, Remove the redundant meta.EnsureDualStoreTables() calls near the
AutoMigrate setup and other referenced locations. Keep
db.AutoMigrate(meta.CatalogEntities()...) as the sole initialization path for
both effective and raw dual-store entities, without changing unrelated setup
logic.
🤖 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 `@internal/module/artifact/build/backend/app_setting_coverage_test.go`:
- Around line 281-290: The blank-Id fixtures are normalized before the filter
runs, so both tests miss the intended branch. In
internal/module/artifact/build/backend/app_setting_coverage_test.go lines
281-290 and
internal/module/artifact/build/backend/field_default_coverage_test.go lines
281-290, either assert after persistDeclaration that the stored row received a
generated Id, or insert the whitespace-only Id through a raw database helper
that bypasses ensureBaseModelID, while preserving the existing
supersedeVirtualAppSettings assertions.

In `@internal/module/artifact/build/backend/builder.go`:
- Around line 891-900: Update the key-enumeration block in the builder to
iterate over all registered specs from injectappmodel.Specs() instead of
hardcoding FieldDefault and AppSetting, appending each superseded spec’s
LogicalKey only when the application name is non-empty. Before using this loop,
fix Specs() so it snapshots the registry without copying sync.Map per element,
then continue checking each spec through
injectSession.Plan(name).SupersedeInject.

In `@internal/module/artifact/build/backend/declaration_test_helpers.go`:
- Around line 4-12: Rename declaration_test_helpers.go to
declaration_test_helpers_test.go so the Go toolchain treats its testing helpers
and testing import as test-only; leave the existing call sites in
app_setting_coverage_test.go and field_default_coverage_test.go unchanged.

In `@internal/module/artifact/build/backend/field_default.go`:
- Line 181: The append in the relevant field-default build flow must not mutate
the cached slice returned by entryPointImports(). Create an independent copy
before appending b.fieldDefaultInjectPaths, and apply the same protection to the
identical append pattern in app_setting.go so cached imports remain unchanged
across builder calls.

In `@internal/module/artifact/build/injectappmodel/inject.go`:
- Around line 171-172: Prevent duplicate imports and mutation of borrowed host
slices when applying inject paths. In
internal/module/artifact/build/injectappmodel/inject.go lines 171-172, update
applyInject to construct a fresh import slice and deduplicate the merged
EntryPointImports and allInjectPaths results before SetEntryPointImports. In
internal/module/artifact/build/injectappmodel/bundle.go lines 26-29, likewise
copy EntryPointImports into a new slice, deduplicate the merged imports before
SetEntryPointImports, and preserve each unique path only once.
- Around line 157-167: Enforce zero-value Session safety across all affected
entry points: in internal/module/artifact/build/injectappmodel/inject.go lines
157-167, update applyInject to return before calling sess.host.Module() when
host is nil; in internal/module/artifact/build/injectappmodel/inject.go lines
36-46, replace direct plans writes in DecideAndInjectOne and DecideOne with
SetPlan; in internal/module/artifact/build/injectappmodel/session.go lines
85-97, initialize lastInjectPath and injectPaths in rememberInjectPath before
writing to them.
- Around line 36-46: Update DecideAndInjectOne and DecideOne to store plans
through sess.SetPlan instead of assigning directly to sess.plans. Preserve the
existing plan values and error flow while relying on SetPlan’s lazy map
initialization so zero-value Session instances do not panic.

In `@internal/module/artifact/build/injectappmodel/session.go`:
- Around line 85-97: Update Session.rememberInjectPath to lazily initialize both
lastInjectPath and injectPaths before writing, matching SetPlan’s zero-value
handling. Preserve the existing nil-session, empty-path, deduplication, and
append behavior, and ensure a zero-value Session cannot panic on either map
write.

In `@pkg/meta/facade.go`:
- Around line 103-115: Make the module replacement flow atomic by wrapping the
previous-key reads, DeleteRawModelsForModule, and every PersistModelTreeAsRaw
call in a single database transaction. Update the relevant logic around the
shown replacement function to use the transaction handle consistently, commit
only after all trees persist successfully, and roll back on any failure. Add a
failure-path test that makes a later tree persistence fail and verifies the
original raw declarations remain unchanged.

---

Nitpick comments:
In `@internal/module/artifact/build/backend/app_setting_coverage_test.go`:
- Line 429: Remove the redundant meta.EnsureDualStoreTables() calls near the
AutoMigrate setup and other referenced locations. Keep
db.AutoMigrate(meta.CatalogEntities()...) as the sole initialization path for
both effective and raw dual-store entities, without changing unrelated setup
logic.

In `@internal/module/artifact/build/backend/app_setting.go`:
- Line 208: Update the duplicate AppSetting error construction in the relevant
build flow to use the existing appSettingDuplicateCode constant instead of the
repeated "APP_SETTING_DUPLICATE" literal, while preserving the current error
message and formatting.
- Around line 220-222: Update resetAppSettingScheduledAppsForTest to reset only
the AppSetting-related spec instead of calling
injectappmodel.ResetScheduledForTest, which clears all registered specs; add and
use a per-spec reset API in injectappmodel, preserving unrelated FieldDefault
claims. Alternatively, rename the helper and its callers to explicitly reflect
global reset scope if that is the intended behavior.

In `@internal/module/artifact/build/backend/declaration_test_helpers.go`:
- Around line 21-25: Replace the positional string arguments of
seedVirtualDeclarationTree and seedVirtualDeclarationErrorBranchTree with an
options struct using named fields. Update all call sites to construct the
struct, preserving existing values while allowing omitted fields to remain
empty. Add defaults inside both helpers for any empty option fields, and remove
the long positional parameter lists.

In `@internal/module/artifact/build/backend/field_default_test.go`:
- Around line 558-567: Update the test around FlushEffective to count the
FieldDefault effective row before flushing and assert the expected pre-flush
count based on seedFieldDefaultDeclaration. Keep the existing post-flush count
assertion, ensuring the test verifies supersede leaves the effective table
unchanged until the explicit FlushEffective call.

In `@internal/module/artifact/build/backend/field_default.go`:
- Around line 26-104: Collapse the duplicated FieldDefault wrapper logic in
fieldDefaultPlanFrom, toInject, syncFieldDefaultFromSession,
rememberFieldDefaultInjectPath, releaseFieldDefaultSchedule, and the six test
shim helpers by introducing a shared model-name parameterized helper path,
matching the existing app_setting.go pattern. Keep the current FieldDefault
behavior intact while replacing the repeated literal-based wrappers with one
reusable implementation so future changes only land in one place.

In `@internal/module/artifact/build/backend/inject_host.go`:
- Around line 13-15: Add a package-level compile-time assertion that injectHost
implements injectappmodel.Host so any Host signature change fails at the type
definition instead of only at ensureInjectSession. Also extract the repeated
anonymous plugin interfaces used in injectHost’s methods into named interfaces,
and reuse entryPointImportsSetter and virtualSourceRegistrar in both places
rather than duplicating the same interface literals.

In `@internal/module/artifact/build/injectappmodel/bundle.go`:
- Around line 45-87: Update bundleSpec to load all existing declarations for the
current spec with one dbLoadModels query filtered by spec.ModelName, then group
the returned models by application in memory before iterating modules. Use each
module’s application group for handwrittenModels and generatedModels decisions,
preserving path registration and skipping behavior while eliminating the
per-application database query.

In `@internal/module/artifact/build/injectappmodel/injectappmodel_test.go`:
- Around line 46-63: Update newTestSession to register t.Cleanup that calls
ResetScheduledForTest after the test completes, while retaining the existing
setup reset so process-wide spec.scheduled claims cannot leak between tests.
- Around line 189-209: Update TestReleaseSchedules_ClearsClaim to use
specByNameOrPanic("FieldDefault") instead of discarding the lookup result from
specByName, so a missing spec fails fast with the established test error rather
than panicking on spec.scheduled.
- Around line 250-266: Extend TestInjectRegistersVirtualSource to assert
host.entryImports contains the expected entry-point import paths after
InjectAppModels, accounting for the cumulative duplicate entries produced across
both specs. Use the recorded setImportCalls or existing module/spec path helpers
to verify the expected duplicated imports and preserve the current
virtual-source assertions.

In `@internal/module/artifact/build/injectappmodel/source.go`:
- Around line 15-37: Add validation in Register for Spec.ModelName,
Spec.GeneratedRelPath, and Spec.BaseModelFile, rejecting empty or invalid values
before registration succeeds; ensure ModelName is a valid identifier suitable
for both generated string literals and class/import-name positions, and the path
fields are non-empty valid relative paths. Also defensively reject an empty
modulesPath before generatedSource constructs imports, rather than allowing
filepath.Clean("") to produce ".".

In `@internal/module/artifact/build/injectappmodel/spec.go`:
- Around line 83-88: The ScheduledApps helper silently returns a throwaway map
for unknown model names, causing stored claims to be lost. Change ScheduledApps
to fail loudly for unregistered modelName values, preferably by panicking in
this test-only helper, while preserving the registered spec.scheduled map
behavior.
- Around line 19-20: Remove the named sync.Map field from Spec and maintain
deduplication state in registry-owned storage keyed by ModelName. Update
Register and Specs to avoid copying synchronization state, and route all
scheduled-state reads and writes—including ScheduledApps and NeedInject-related
accesses—through a dedicated accessor that returns the map for the relevant
model.
🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: e8f72984-989c-42d8-9bf7-4cff1f423d06

📥 Commits

Reviewing files that changed from the base of the PR and between 6751d8c and 97def8e.

📒 Files selected for processing (71)
  • internal/i18n/models/i18n_meta.go
  • internal/i18n/models/i18n_meta_test.go
  • internal/module/artifact/build/backend/app_setting.go
  • internal/module/artifact/build/backend/app_setting_coverage_test.go
  • internal/module/artifact/build/backend/app_setting_test.go
  • internal/module/artifact/build/backend/builder.go
  • internal/module/artifact/build/backend/builder_test.go
  • internal/module/artifact/build/backend/declaration_test_helpers.go
  • internal/module/artifact/build/backend/field_default.go
  • internal/module/artifact/build/backend/field_default_coverage_test.go
  • internal/module/artifact/build/backend/field_default_test.go
  • internal/module/artifact/build/backend/inject_app_models.go
  • internal/module/artifact/build/backend/inject_host.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/compat.go
  • internal/module/artifact/build/injectappmodel/doc.go
  • internal/module/artifact/build/injectappmodel/helpers.go
  • internal/module/artifact/build/injectappmodel/host.go
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/plan.go
  • internal/module/artifact/build/injectappmodel/register_builtin.go
  • internal/module/artifact/build/injectappmodel/session.go
  • internal/module/artifact/build/injectappmodel/source.go
  • internal/module/artifact/build/injectappmodel/spec.go
  • internal/module/artifact/build/injectappmodel/supersede.go
  • internal/module/lifecycle/bundles.go
  • internal/module/lifecycle/bundles_app_setting_test.go
  • internal/module/lifecycle/bundles_field_default_test.go
  • internal/module/lifecycle/install_module_test.go
  • internal/module/lifecycle/install_prefetch_test.go
  • internal/module/lifecycle/module_index_sync_test.go
  • internal/module/lifecycle/modulemanager.go
  • internal/module/lifecycle/modulemanager_coverage_test.go
  • internal/module/lifecycle/modulemanager_fastfail_test.go
  • internal/module/lifecycle/uninstaller_clean_models_test.go
  • internal/module/lifecycle/uninstaller_model_data_test.go
  • internal/module/lifecycle/upgrade_uninstall_commit_test.go
  • internal/module/metaeff/recompute.go
  • internal/module/metaeff/recompute_test.go
  • pkg/meta/acl_remap.go
  • pkg/meta/acl_remap_coverage_test.go
  • pkg/meta/acl_remap_test.go
  • pkg/meta/declaration.go
  • pkg/meta/declaration_test.go
  • pkg/meta/dual_store_migrate.go
  • pkg/meta/dual_store_migrate_coverage_test.go
  • pkg/meta/dual_store_migrate_test.go
  • pkg/meta/effective_merge.go
  • pkg/meta/effective_merge_coverage_test.go
  • pkg/meta/effective_merge_test.go
  • pkg/meta/extends_expand.go
  • pkg/meta/extends_expand_coverage_test.go
  • pkg/meta/extends_expand_test.go
  • pkg/meta/facade.go
  • pkg/meta/facade_test.go
  • pkg/meta/meta_model.go
  • pkg/meta/meta_raw_argument.go
  • pkg/meta/meta_raw_decorator.go
  • pkg/meta/meta_raw_field.go
  • pkg/meta/meta_raw_field_test.go
  • pkg/meta/meta_raw_model.go
  • pkg/meta/meta_raw_parameter.go
  • pkg/meta/meta_raw_service.go
  • pkg/meta/meta_raw_typeparameter.go
  • pkg/meta/model.go
  • pkg/meta/model_test.go
  • pkg/meta/raw_facade.go
  • pkg/meta/recompute.go
  • pkg/meta/recompute_coverage_test.go
  • pkg/meta/recompute_test.go
💤 Files with no reviewable changes (2)
  • internal/module/metaeff/recompute_test.go
  • internal/module/metaeff/recompute.go

Comment thread internal/module/artifact/build/backend/app_setting_coverage_test.go Outdated
Comment thread internal/module/artifact/build/backend/builder.go
Comment thread internal/module/artifact/build/backend/declaration_test_helpers.go Outdated
Comment thread internal/module/artifact/build/backend/field_default.go Outdated
Comment thread internal/module/artifact/build/injectappmodel/inject.go
Comment thread internal/module/artifact/build/injectappmodel/inject.go Outdated
Comment thread internal/module/artifact/build/injectappmodel/inject.go Outdated
Comment thread internal/module/artifact/build/injectappmodel/session.go
Comment thread pkg/meta/facade.go Outdated
buke and others added 4 commits August 5, 2026 23:00
- Drop field_default.go and app_setting.go so ModuleBuilder only keeps injectSession and the three injectappmodel APIs.

- Move decide/bundle/dedup matrix coverage into injectappmodel and keep a thin backend lifecycle integration test.

- Wire lifecycle bundles to BundleInjectAppModels only and drop Ensure*VirtualImports stubs.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Deduplicate entry-point inject imports on a fresh slice and harden Session zero-value map writes via SetPlan.

- Store Spec.scheduled as *sync.Map and drive persist flush keys from the Specs registry.

- Wrap ReplaceModuleDeclarations delete+persist in one transaction and add a rollback regression test.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Add injectappmodel and backend inject coverage suites that exercise nil guards, decide/bundle/supersede branches, and BuildWithoutPersist failure releases.

- Cover facade/raw_facade APIs including Prefer failures and replace rollback paths, and surface empty-entryPoint build plugin GetParserResults errors.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise nil and empty name/path owners in appendUnique so bundles.go patch coverage is complete.

Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
internal/module/artifact/build/backend/inject_coverage_test.go (3)

235-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The variable named prebuildPlugin holds the build plugin.

newInjectTestBuilder returns buildPlugin as the second value (line 48). Line 235 binds that value to a variable named prebuildPlugin. Line 238 then assigns the build plugin to builder.prebuildPlugin, so both plugin fields reference one object. The test still passes, because it only asserts that prebuild fails on the missing entry file. Remove the misleading assignment.

♻️ Proposed cleanup
-	builder, prebuildPlugin, _ := newInjectTestBuilder(t, mod, nil)
+	builder, _, _ := newInjectTestBuilder(t, mod, nil)
 	// Force prebuild esbuild path then fail: set entryPoint to missing file.
 	builder.entryPoint = filepath.Join(t.TempDir(), "missing-entry.ts")
-	builder.prebuildPlugin = prebuildPlugin
🤖 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 `@internal/module/artifact/build/backend/inject_coverage_test.go` around lines
235 - 238, Remove the redundant builder.prebuildPlugin = prebuildPlugin
assignment in the test setup around newInjectTestBuilder, since the returned
plugin is already assigned to the builder’s plugin field and the variable name
is misleading. Leave the missing-entry setup and failure assertions unchanged.

166-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the flush keys, not only the absence of an error.

Both tests are named for supersede flush-key behavior, but they only check that persistModuleModels returns nil. A regression that computes the wrong flush keys, or computes none, would still pass. Add an assertion on the observable result, for example the raw declaration rows for FieldDefault and AppSetting after the call.

Also applies to: 244-256

🤖 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 `@internal/module/artifact/build/backend/inject_coverage_test.go` around lines
166 - 181, Update TestPersistModuleModels_SupersedeFlushKeys and the
corresponding test around the second supersede case to assert the observable
flush-key results after persistModuleModels succeeds. Inspect the raw
declaration rows for both FieldDefault and AppSetting and verify they contain
the expected flush keys, rather than only checking that no error was returned.

32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Check the AutoMigrate error.

Line 32 fails the test when EnsureDualStoreTables fails, but line 35 discards the AutoMigrate error. If the Application or Module table is not created, later tests fail with unrelated query errors instead of a clear setup failure. Make the setup consistent.

♻️ Proposed change
-	_ = db.AutoMigrate(&meta.Application{}, &meta.Module{})
+	if err := db.AutoMigrate(&meta.Application{}, &meta.Module{}); err != nil {
+		t.Fatal(err)
+	}
🤖 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 `@internal/module/artifact/build/backend/inject_coverage_test.go` around lines
32 - 35, Handle the error returned by db.AutoMigrate in the test setup,
alongside the existing EnsureDualStoreTables check. Fail the test immediately
with t.Fatal when migration of meta.Application or meta.Module fails, rather
than discarding the error.
internal/module/artifact/build/injectappmodel/coverage_test.go (1)

346-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Two assertions have empty bodies and cannot fail. Both sites evaluate a condition and place only a comment inside the block, so the check has no effect. Remove the dead blocks and keep the real assertions.

  • internal/module/artifact/build/injectappmodel/coverage_test.go#L346-L351: delete the isGeneratedPath block with the empty body; line 353 already covers exact rel-path equality.
  • internal/module/artifact/build/injectappmodel/coverage_test.go#L381-L391: delete the empty strconvQuoteFallback block and the strconvQuoteFallback helper; line 386 already checks the default application literal.
🤖 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 `@internal/module/artifact/build/injectappmodel/coverage_test.go` around lines
346 - 351, Remove the empty isGeneratedPath assertion block in
internal/module/artifact/build/injectappmodel/coverage_test.go lines 346-351;
the exact relative-path assertion already provides coverage. Also remove the
empty strconvQuoteFallback block and its helper in lines 381-391, preserving the
existing assertion that checks the default application literal.
🤖 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.

Nitpick comments:
In `@internal/module/artifact/build/backend/inject_coverage_test.go`:
- Around line 235-238: Remove the redundant builder.prebuildPlugin =
prebuildPlugin assignment in the test setup around newInjectTestBuilder, since
the returned plugin is already assigned to the builder’s plugin field and the
variable name is misleading. Leave the missing-entry setup and failure
assertions unchanged.
- Around line 166-181: Update TestPersistModuleModels_SupersedeFlushKeys and the
corresponding test around the second supersede case to assert the observable
flush-key results after persistModuleModels succeeds. Inspect the raw
declaration rows for both FieldDefault and AppSetting and verify they contain
the expected flush keys, rather than only checking that no error was returned.
- Around line 32-35: Handle the error returned by db.AutoMigrate in the test
setup, alongside the existing EnsureDualStoreTables check. Fail the test
immediately with t.Fatal when migration of meta.Application or meta.Module
fails, rather than discarding the error.

In `@internal/module/artifact/build/injectappmodel/coverage_test.go`:
- Around line 346-351: Remove the empty isGeneratedPath assertion block in
internal/module/artifact/build/injectappmodel/coverage_test.go lines 346-351;
the exact relative-path assertion already provides coverage. Also remove the
empty strconvQuoteFallback block and its helper in lines 381-391, preserving the
existing assertion that checks the default application literal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f598bf5d-1da5-48f8-bf3e-6a900adcbbfd

📥 Commits

Reviewing files that changed from the base of the PR and between b8b91b3 and 4b3f027.

📒 Files selected for processing (15)
  • internal/module/artifact/build/backend/builder.go
  • internal/module/artifact/build/backend/inject_coverage_test.go
  • internal/module/artifact/build/backend/inject_host.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/compat.go
  • internal/module/artifact/build/injectappmodel/coverage_test.go
  • internal/module/artifact/build/injectappmodel/helpers.go
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/session.go
  • internal/module/artifact/build/injectappmodel/spec.go
  • internal/module/lifecycle/bundles_app_setting_test.go
  • pkg/meta/facade.go
  • pkg/meta/facade_coverage_test.go
  • pkg/meta/facade_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/injectappmodel/helpers.go
  • internal/module/lifecycle/bundles_app_setting_test.go
  • internal/module/artifact/build/backend/builder.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/session.go
  • pkg/meta/facade.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/compat.go

- Add Registry with DefaultRegistry lazy builtins and package API forwards.

- Bind NewSession to a registry and drive Decide/Inject/Bundle/Supersede from it.

- Add WithInjectRegistry on ModuleBuilder and flush supersede keys via the session registry.

- Migrate tests to NewRegistryWithDefaults/ResetClaims and deprecate package ResetScheduledForTest/ScheduledApps.

Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@internal/module/artifact/build/injectappmodel/registry.go`:
- Around line 123-129: Synchronize the complete claim-map operations in
TryClaim, ReleaseClaim, and ClaimOwner with ResetClaims using r.mu, so no claim
can be inserted, removed, or read concurrently with reset processing. Ensure
ResetClaims cannot finish before a concurrent TryClaim operation completes,
preserving the existing return behavior and claim ownership semantics.
🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3e72fe08-2bc0-475f-9ad2-064b005bf425

📥 Commits

Reviewing files that changed from the base of the PR and between 4b3f027 and 5e054d9.

📒 Files selected for processing (16)
  • internal/module/artifact/build/backend/builder.go
  • internal/module/artifact/build/backend/builder_test.go
  • internal/module/artifact/build/backend/inject_coverage_test.go
  • internal/module/artifact/build/backend/inject_host.go
  • internal/module/artifact/build/backend/inject_integration_test.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/compat.go
  • internal/module/artifact/build/injectappmodel/coverage_test.go
  • internal/module/artifact/build/injectappmodel/doc.go
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/register_builtin.go
  • internal/module/artifact/build/injectappmodel/registry.go
  • internal/module/artifact/build/injectappmodel/session.go
  • internal/module/artifact/build/injectappmodel/spec.go
  • internal/module/artifact/build/injectappmodel/supersede.go
🚧 Files skipped from review as they are similar to previous changes (10)
  • internal/module/artifact/build/injectappmodel/doc.go
  • internal/module/artifact/build/backend/inject_integration_test.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/supersede.go
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/backend/inject_host.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/backend/builder.go
  • internal/module/artifact/build/backend/builder_test.go
  • internal/module/artifact/build/backend/inject_coverage_test.go

Comment thread internal/module/artifact/build/injectappmodel/registry.go
- Make injectappmodel return Effects (virtual files + imports) instead of writing through Host.

- Bind Session to read-only BuildCtx; backend applies Effects onto esbuild plugins.

- Migrate unit tests to assert Effects and mutate BuildCtx via Session.Context().

Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@internal/module/artifact/build/injectappmodel/inject.go`:
- Around line 26-28: The effects returned by these functions must contain
imports only for their current virtual files. In
internal/module/artifact/build/injectappmodel/inject.go lines 26-28 and
internal/module/artifact/build/injectappmodel/bundle.go lines 26-28, merge each
current fx.Imports into out alongside fx.Files instead of assigning session-wide
imports from sess.effectsImports(); in
internal/module/artifact/build/injectappmodel/inject.go lines 83-88, preserve
and return the imports produced by materializeInject without replacing them with
session-wide imports.
🪄 Autofix

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4d677d50-daa8-43ea-93bf-a5b210194b46

📥 Commits

Reviewing files that changed from the base of the PR and between 5e054d9 and 25849d6.

📒 Files selected for processing (13)
  • internal/module/artifact/build/backend/inject_app_models.go
  • internal/module/artifact/build/backend/inject_coverage_test.go
  • internal/module/artifact/build/backend/inject_host.go
  • internal/module/artifact/build/injectappmodel/bundle.go
  • internal/module/artifact/build/injectappmodel/context.go
  • internal/module/artifact/build/injectappmodel/coverage_test.go
  • internal/module/artifact/build/injectappmodel/doc.go
  • internal/module/artifact/build/injectappmodel/effects.go
  • internal/module/artifact/build/injectappmodel/helpers.go
  • internal/module/artifact/build/injectappmodel/inject.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/session.go
  • internal/module/artifact/build/injectappmodel/supersede.go
💤 Files with no reviewable changes (1)
  • internal/module/artifact/build/injectappmodel/helpers.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • internal/module/artifact/build/injectappmodel/doc.go
  • internal/module/artifact/build/injectappmodel/supersede.go
  • internal/module/artifact/build/injectappmodel/injectappmodel_test.go
  • internal/module/artifact/build/injectappmodel/coverage_test.go
  • internal/module/artifact/build/backend/inject_coverage_test.go

Comment thread internal/module/artifact/build/injectappmodel/inject.go Outdated
buke and others added 4 commits August 6, 2026 11:43
- Merge inject_host and inject_app_models into inject.go; drop stale Host naming.

- Merge coverage and integration tests into inject_test.go; remove leftover sessionDB helper.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Hold the registry mutex for the full TryClaim/ReleaseClaim/ClaimOwner path and replace claim maps on ResetClaims to avoid stale post-reset stores.

- Return Effects.Imports only for files produced by the current inject/bundle call.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Re-extract meta.pot and sync zh_CN.po for new Meta*Raw model fields.

- Fill zh_CN msgstr from existing non-Raw Meta* field translations.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise nil BuildCtx, mergeImportPaths edge cases, and BundleInjectAppModels errors.

- Cover lazy session maps, package specsList, and claimMapLocked nil-map paths.

Co-authored-by: Cursor <cursoragent@cursor.com>
@buke

buke commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread internal/module/artifact/build/backend/builder.go
- Drop remembered inject paths if Effects were not applied, so reused builders cannot import stale generated files via buildOptions.

- Cover partial FieldDefault success followed by AppSetting Decide failure.

Co-authored-by: Cursor <cursoragent@cursor.com>
@buke

buke commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 1eca959. Configure here.

@buke
buke merged commit aac583f into main Aug 6, 2026
46 checks passed
@buke
buke deleted the feat/meta-dual-store-followup branch August 6, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant