[njakobsen]

Stacked on #15. Association tracking is experimental; the README says so.

Changes

• belongs_to: recording an association with a record (or nil) stores its foreign key, and type when polymorphic, so it resolves across sources like any attribute. Values JSON cannot hold, such as objects, now raise LaForge::InvalidValue when recorded instead of coming back as hashes.

• has_many and has_one: laforged associations: { … } declares tracked associations (a has_and_belongs_to_many raises), with keys derived from the reflection where possible. A polymorphic reference adds its type column to a derived key. Member order is written to the integer column the scope orders by, in the scope's direction, never one that identifies the member or its owner, or to the column order_column: names; order_column: false writes none.

• Membership per member: record_data_entries(..., mode:) says what a source's list is: :complete (the default), the source's whole member list; :partial, the members it includes or excludes; or :edit, changes to the members it names, the rest of its report kept; forgetting a member of a complete list raises, since it would exclude it. The highest-precedence report with an opinion on a member decides it. A source that stops including a member withdraws it, and rows no report has named, such as ones from before tracking, are left alone.

• Editing one member: mode: :edit takes { include: [...], exclude: [...], forget: [...] }, as an editor makes changes, and accepts the shapes a Rails form submits: *_attributes for a tracked association, and *_ids. Where the owner declares accepts_nested_attributes_for, its reject_if: and allow_destroy: apply to the rows as they do in Rails, a method-based reject_if: called on the owner itself, new or not; limit: and update_only: raise, as LaForge does not apply them.

• Removed members reported again: excluding a member keyed by id destroys it and keeps a tombstone, which tombstone_matcher: uses to recognise the member when a source reports it again, so it stays out until a source that outranks the exclusion includes it. LaForge.tombstone_blocking lets code that creates members itself check first.

• Orphan sweeps: withdraw_source withdraws everything a source reported, instead of destroying its entries.

• Member attributes: a list names its members by key, and keeps what the source says about each one as pending until a forge of the owner includes that member. That forge records them on the member as that source's entries, as if the source had recorded them on the member directly, nested tracked associations included. Nothing is created for a member membership excludes; its attributes wait in the report until membership includes it or the source's next report replaces them, so an owner can be saved and forged later. A newer value the member has from the same source is not overwritten, judged by a new recorded_at column on data entries (updated_at keeps its usual meaning). Existing installs add it with rails generate laforge:recorded_at, which fills it from updated_at. A saved record can be listed in place of a hash; an unsaved one raises.

• Keys: members need a value for each key column. Rows missing one are left alone; rows sharing a key raise LaForge::DuplicateMemberKey at forge, and a list naming a member twice raises.

• Members that can't be created: a member included that no report describes is built from its key, or skipped with a warning and reported under skipped: in forge_diff when invalid with its key alone.

• Precedence: a precedence: given with a list is given to everything the payload records, nested lists included. Sources rank by level first, a tie at a source's most specific level falling to the sources' own precedence, and update_precedence_order sets the order of sources for one record's attribute, a model's attribute, a whole model, or globally.

• Attribute groups, inactive sources and change events: laforged attribute_groups: keeps attributes such as an address's lines from one source together; a source's active flag (rails generate laforge:active_sources) leaves its entries out of forging while keeping them; forge_attributes(except_sources:) answers what would be forged without a source; and data_entry_changed.laforge is published for each entry saved with a changed value or destroyed, once the write is committed, and not for a write that rolls back, with the values as they were saved. Change events need Rails 7.2 or later.

• Merging duplicates: LaForge.merge and LaForge.reverse_merge move a duplicate's entries, precedence rules and tracked members into the survivor.

• Removal: members no longer included are removed as assigning the collection would remove them, per dependent:: destroyed for :destroy, deleted without callbacks for :delete_all, detached without validation otherwise, once the owner is saved. A replaced has_one member is removed when the owner is saved, and a member a later forge includes again before the save, has_one or not, is kept. Destroying records is otherwise out of scope.

• Inspection: source_values returns only the members the association's scope includes, and forge_diff(attributes:) previews only the associations given.

• Specs: spec/lib/association_capabilities_spec.rb covers each capability by id, and spec/lib/association_importer_scenarios_spec.rb walks through importer runs.

• Structure: eight refactor: commits at the top restructure association tracking around LaForge::TrackedAssociation (one object per owner and association) and MemberList::Complete/MemberList::Partial reports with methods for each change, hold the forge's writes in PendingWrites, and move LaForge's methods onto LaForge::Record. Each laforged model has its own subclass, Model.laforge, holding the model-level methods, and each record an instance, record.laforge, holding its methods, pending writes and precedence resolver. The concerns AttributeGroups, Recording, Forging, Precedence and Inspection make it up, and the model delegates the documented API to it, so place.forge! is place.laforge.forge!. Behaviour is unchanged; the suite proves it at each commit.

Follow-ups

• A reactivated source's exclusion doesn't apply to a member the application created itself while the source was inactive, after LaForge.tombstone_blocking allowed it, since later reports name it by its id rather than a tombstone. Matching saved members against tombstones would change tombstone_matcher:'s contract and could exclude a legitimate row; the README now describes the narrower behaviour.
• forge(sources:) filters attribute entries only; association membership is still decided by every active report. Limiting membership to some sources needs its own semantics for the reports left out.
• forge_diff doesn't preview what withdrawing a source from retained members changes, down nested lists.
• recorded_at advances only when a value changes, so re-recording an unchanged value doesn't protect it from an older report kept pending on an owner; refreshing it for every unchanged attribute would write a row per attribute on each import.
• Replacing a keyed has_one member whose owner foreign key has a unique index: the old member is removed after the save, so autosave's insert of the new one fails first.
• Key columns whose type JSON doesn't keep, such as dates, compare as strings once a report is reloaded.
• A saved record listed in a member's nested list is kept in the pending JSON as its attributes rather than its key.
• LaForge.merge doesn't remap entries recorded on moved members that point back at the duplicate owner through a belongs_to.
• A has_one member's own nested lists are recorded as complete reports in edit mode, not as edits.
• LaForge.merge keeps both records' un-forged pending attributes for the same member, so the newer one wins rather than the merge winner's. Forging both records before merging avoids it.

UAT Steps

1. bundle exec rspec: 309 examples, 0 failures, 2 pending.

Closes

Closes #10

Special Handling

• This PR requires user testing
• Include this PR in the changelog

🤖 Generated with Claude Code

[timtait]

Checking the upgrade against client data

We tested whether moving Sparkle from the LaForge it pins today (dbc77faa) to this stack changes what any record forges.

How

1. For each client database, copied laforge_data_sources and laforge_data_entries into two scratch databases ("before" and "after"). The source databases were only read.
2. Ran the upgrade migrations on the "after" copy in the order Sparkle will: precedence_rules (feat!: precedence rules at four levels, and rename priority to precedence #15), then recorded_at and active_sources (feat: track association membership across sources #16).
3. Forged every record in both copies without saving: "before" with LaForge at dbc77faa, "after" with this PR's head (838a041), on the same Rails 8.1 bundle. Each record type gets a stand-in model with a setter for every attribute its entries name, so the gem's own forge_attributes picks the winners.
4. Diffed the winning value of every record and attribute.

Results

Client	Copy as of	Forged values	Changed
Atrium	2026-10-01 (production data)	499,843	0
Scripps	2026-08-23	202,345	0
UCSF	2026-05-28	372,142	0
AtlantiCare	2025-11-12	53,361	0
Westchester	2025-01-10	46,418	0

Control. Before upgrading, we added one Sparkle entry to both Atrium copies: an admin edit of department_id on an address where Epic also has one. The diff showed exactly that change, with the edit winning before and Epic's value winning after. So the harness does catch a changed winner, and level-first ranking is the cause. The cause is EpicDepartmentImporter recording Epic's address fields with priority: 100. The migration turns those into per-record rules (19,092 on Atrium), which now outrank Sparkle's global precedence.

No current Atrium data hits this, but future admin edits to department_id, exists_in_ehr and scheduling_enabled on Epic addresses would be overwritten. We reran the control with a candidate Sparkle-side data migration added: it deletes those record rules and sets Address.update_precedence_order([sparkle, epic], attribute: ...) for each of the three fields. The diff was back to zero, with the edit holding. We'll ship that conversion, and drop priority: 100 from the importer, in the same Sparkle deploy as the gem bump. Scripps has the same Epic rules (about 2,100) but no Sparkle source, and Epic already outranked MDStaff and UHC there, so nothing changes.

Caveats

• Only Atrium's copy is recent. The Scripps, UCSF, AtlantiCare and Westchester copies are local snapshots that are months old, and Westchester's is from January 2025. We'll rerun the check against fresh copies before upgrading.
• Clients without a local copy (UMMS, Christus and others) haven't been checked yet.
• This covers attribute forging only, which is all Sparkle records through LaForge today. No associations are tracked yet. Sparkle's own model code, such as setters with side effects, isn't exercised.

(Generated using Claude Code)

[timtait]

Requesting changes for the inline findings. Each one has a repro on 838a041 and a suggested fix that keeps the full suite green (269 examples, 0 failures, 2 pending):

1. High: derived_order_column can choose the primary key, so forge rewrites member ids, or raises RecordNotUnique. Sparkle's Address hits this, and should also be declared with key: :id.
2. Payload ignores reject_if: and allow_destroy:. A blank form row fails the save, and _destroy deletes members that Rails would keep.
3. forge_diff leaves a precedence rule in memory, and the next owner save persists it.
4. Minor: precedence_order with no attribute reports a model-wide rule as :attribute.

Change events for rolled-back writes are covered in the earlier comment: #16 (comment)

The PR description is also out of date. It says partial: true, but the code uses mode: :partial/mode: :edit. It also says 221 examples; it's 269 now.

(Generated using Claude Code)

[timtait]

Forging a tracked association can rewrite its members' primary keys

derived_order_column excludes key columns, but not the primary key. That's only safe when members are keyed by id. Once a member has a second belongs_to, derived_key makes that foreign key the key. Then id becomes "an integer column the scope orders by", and forge runs record[config.order_column] = index on it.

Sparkle's Address is shaped like this. It belongs to locale (the owner) plus location, address_group and department. The core AddressBehavior scope for addresses ends in .order(virtual: :asc, id: :asc), and the position ordering is a raw CASE, so derivation doesn't see it. laforged associations: { addresses: {} } would therefore derive key: [location_id, address_group_id, department_id] and order_column: "id".

Here's what forge issued for an owner with addresses 2 and 3:

UPDATE `pz_addresses` SET `pz_addresses`.`id` = 0 WHERE `pz_addresses`.`id` = 2
UPDATE `pz_addresses` SET `pz_addresses`.`id` = 1 WHERE `pz_addresses`.`id` = 3

There are two outcomes:

• If the new id is free, the row moves silently. Its own laforge_data_entries still point at the old id, and so does anything else that references it (appointments, Kyruus ids, caches).
• If the new id is taken, forge! raises ActiveRecord::RecordNotUnique: Duplicate entry '0' for key 'repro_addresses.PRIMARY'.

Repro. Drop this in spec/lib/. All 3 examples fail on 838a041:

spec

require 'spec_helper'

describe 'association tracking on a member with a second belongs_to' do
  before(:all) do
    ActiveRecord::Schema.define do
      create_table(:repro_owners, force: true) { |t| t.string :name }
      create_table(:repro_departments, force: true) { |t| t.string :name }
      create_table(:repro_addresses, force: true) { |t| t.bigint :repro_owner_id; t.bigint :repro_department_id; t.string :city }
    end
    Object.const_set(:ReproDepartment, Class.new(ActiveRecord::Base))
    Object.const_set(:ReproAddress, Class.new(ActiveRecord::Base))
    Object.const_set(:ReproOwner, Class.new(ActiveRecord::Base))
    ReproAddress.class_eval { belongs_to :repro_owner; belongs_to :repro_department, optional: true; laforged }
    ReproOwner.class_eval do
      has_many :repro_addresses, -> { order(id: :asc) }
      laforged associations: { repro_addresses: {} }
    end
  end
  after(:all) { %w[repro_owners repro_departments repro_addresses].each { |t| ActiveRecord::Base.connection.drop_table(t, if_exists: true) } }

  let(:source) { LaForge::DataSource.find_or_create_by!(name: 'Import') { |s| s.precedence = 1 } }
  let(:department) { ReproDepartment.create!(name: 'Cardiology') }

  it 'does not use the primary key as the order column' do
    expect(LaForge::AssociationConfig.new(ReproOwner, :repro_addresses).order_column).not_to eq('id')
  end

  it 'does not change the id of the address it matches' do
    owner = ReproOwner.create!
    address = owner.repro_addresses.create!(repro_department: department, city: 'Charlotte')
    address.record_data_entries({ city: 'Charlotte' }, source)
    address.save!

    owner.reload.record_data_entries({ repro_addresses: [{ 'repro_department_id' => department.id }] }, source)
    owner.forge!

    expect(ReproAddress.exists?(address.id)).to be(true)
    expect(LaForge::DataEntry.where(record_type: 'ReproAddress').pluck(:record_id)).to all(eq(address.id))
  end

  it 'forges when another row already holds the position number' do
    ReproAddress.create!(city: 'unrelated')
    owner = ReproOwner.create!
    owner.repro_addresses.create!(repro_department: department, city: 'Charlotte')
    owner.repro_addresses.create!(repro_department: ReproDepartment.create!(name: 'Oncology'), city: 'Concord')

    owner.reload.record_data_entries({ repro_addresses: owner.repro_addresses.map { |a| { 'repro_department_id' => a.repro_department_id } } }, source)
    expect { owner.forge! }.not_to raise_error
  end
end

1) does not use the primary key as the order column
   expected: value != "id"
        got: "id"
2) does not change the id of the address it matches
   expected true
        got false
3) forges when another row already holds the position number
   expected no Exception, got #<ActiveRecord::RecordNotUnique: Mysql2::Error: Duplicate entry '0' for key 'repro_addresses.PRIMARY'>

Suggested fix. Never derive a column that identifies the member or its owner:

-    # An integer column the association scope orders by, other than a key column, is where member order is written
+    # An integer column the association scope orders by is where member order is written, unless it identifies the
+    # member or its owner: the primary key, a key column, or the foreign key back to the owner
     def derived_order_column
@@
-      columns.find { |column| !key.include?(column) && klass.columns_hash[column]&.type == :integer }
+      identifying = key + [klass.primary_key, owner_foreign_key, owner_type_column].compact
+      columns.find { |column| identifying.exclude?(column) && klass.columns_hash[column]&.type == :integer }

With that change, the repro passes 3/3 and the full suite passes (269 examples, 0 failures, 2 pending).

Separately, for Sparkle: the README (line 379) says an owned model that happens to belong to another record needs key: :id. Address is exactly that. Without it, addresses would be keyed by [location_id, address_group_id, department_id], and any address missing one of those values can't be identified at all. The upgrade-path doc's addresses: { tombstone_matcher: … } example raises at declaration for the same reason. It should read addresses: { key: :id, tombstone_matcher: … }.

A follow-up worth considering: since derivation can't see position inside Sparkle's CASE, an explicit order_column: (and order_column: false) option would let an owner name its order column or opt out. That's better than relying on whatever the scope happens to mention.

(Generated using Claude Code)

[njakobsen]

Fixed in 0203833: the derived order column skips the primary key, key columns and owner columns, and order_column: (a column or false) names it outright.

[timtait]

Payload ignores accepts_nested_attributes_for's reject_if: and allow_destroy:

A Rails form for an owner that declares accepts_nested_attributes_for :tenants, reject_if: :all_blank, allow_destroy: false behaves differently when recorded through LaForge than when assigned directly:

params	Rails assign_attributes	LaForge record_data_entries(params, mode: :edit) + forge!
a blank template row { 'name' => '', 'unit' => '' }	rejected, 0 new tenants	included, so forge! raises RecordInvalid: Tenants name can't be blank
{ 'id' => cafe.id, '_destroy' => '1' }	ignored, tenant kept	excluded, so the tenant is destroyed

Sparkle's addresses use accepts_nested_attributes_for :addresses, allow_destroy: true, reject_if: :all_blank_with_exceptions, so admin forms with an empty "add address" row would hit the first case.

Suggested fix: where the owner declares nested attributes, apply them the way Rails does. Otherwise keep the current behaviour, which the existing specs rely on.

     def nested_attributes(config, value, owner)
+      options = config.owner_class.nested_attributes_options[config.name.to_sym] # nil where the owner does not accept nested attributes
       unless config.collection?
         attributes = value.to_h.stringify_keys
-        return destroy?(attributes) ? nil : member(config, attributes.except(DESTROY), owner)
+        return destroy?(attributes, options) ? nil : member(config, attributes.except(DESTROY), owner)
       end
 ...
       Array(items).map { |attributes| attributes.to_h.stringify_keys }.each do |attributes|
-        if !destroy?(attributes)
-          include << member(config, attributes.except(DESTROY), owner)
+        if !destroy?(attributes, options)
+          include << member(config, attributes.except(DESTROY), owner) unless reject?(config, attributes, owner, options)
 ...
-    def destroy?(attributes)
-      ::ActiveModel::Type::Boolean.new.cast(attributes[DESTROY]) == true
+    # Where the owner accepts nested attributes, `_destroy` counts only if they allow it, as in Rails
+    def destroy?(attributes, options)
+      (options.nil? || options[:allow_destroy]) && ::ActiveModel::Type::Boolean.new.cast(attributes[DESTROY]) == true
+    end
+
+    # Whether the association's `reject_if:` drops the row, as assigning it through Rails would
+    def reject?(config, attributes, owner, options)
+      case (reject_if = options&.dig(:reject_if))
+      when Symbol
+        target = owner || config.owner_class.new
+        target.method(reject_if).arity.zero? ? target.send(reject_if) : target.send(reject_if, attributes)
+      when Proc
+        reject_if.call(attributes.with_indifferent_access)
+      end
     end

(:all_blank is stored as a Proc by Rails, so the Proc branch covers it.) With this change, both rows match Rails: the blank row is dropped and the tenant is kept. The full suite passes (269 examples, 0 failures, 2 pending). Rails' limit: isn't handled either; I left it out because it's rarely used.

(Generated using Claude Code)

[njakobsen]

Fixed in 71b0c41: reject_if: and allow_destroy: apply as Rails applies them; limit: and update_only: raise.

[timtait]

forge_diff can leave a precedence rule behind that the next save persists

laforge_forge_diff_with puts data_entries back after the preview, but not precedence_rules. When a pending report carries a precedence, laforge_record_reported → write_data_entry builds a rule on the member, and it stays in memory. precedence_rules is autosave: true, so any later save of the owner writes it, even though nothing was forged.

Repro on 838a041, using the spec fixtures:

place = Place.create!(name: 'P')
place.record_data_entries({ tenants: [{ name: 'Cafe' }] }, low); place.forge!
tenant = place.tenants.first
place.reload
place.record_data_entries({ tenants: [{ id: tenant.id, unit: '2B' }] }, low, precedence: 50)
place.forge_diff              # a preview
place.update!(name: 'P2')     # an unrelated save, no forge
LaForge::PrecedenceRule.where(record_type: 'Tenant').pluck(:attribute_name, :precedence)
# => [["unit", 50]], but the tenant's unit is still nil and it has no "unit" entry

Suggested fix: snapshot and restore the rules alongside the entries.

       entries = data_entries.to_a
       values = entries.to_h { |entry| [entry, [entry.value, entry.recorded_at]] }
+      rules = precedence_rules.to_a
+      precedences = rules.to_h { |rule| [rule, [rule.precedence, rule.marked_for_destruction?]] }
       reported.each { |source_id, attributes, recorded_at, edit, precedence| laforge_record_reported(attributes, source_id, recorded_at, edit: edit, precedence: precedence) }
       forge_diff
     ensure
       data_entries.target.replace(entries)
       values.each { |entry, (value, recorded_at)| entry.value = value; entry.recorded_at = recorded_at }
+      precedence_rules.target.replace(rules)
+      precedences.each { |rule, (precedence, destroyed)| rule.precedence = precedence; rule.instance_variable_set(:@marked_for_destruction, destroyed) }

With this change, the same steps leave no Tenant rules after the save, and a real forge! afterwards still writes ["unit", 50] and sets unit to "2B". The full suite passes (269 examples, 0 failures, 2 pending).

(Generated using Claude Code)

[njakobsen]

Fixed in 5336613: the preview snapshots and restores the member's rules, with their precedence and destruction mark.

[timtait]

Minor: with no attribute, precedence_order labels a model-wide rule :attribute

Place.update_source_precedence(low, 5)
Place.precedence_order                     # => [{ source: low, level: :attribute }]   expected :model
Place.precedence_order(attribute: :name)   # => [{ source: low, level: :model }]

With attribute: nil, rank looks up [ANY_ID, '', source] at the model-attribute level, which is the model-wide rule. The order is unaffected, because the same value appears at both levels. The label is wrong, though, and anything that shows it (an admin "why does this source win" view) would mislead.

-        levels = [record_rule(record, attribute_name, source_id), @model_rules[[ANY_ID, attribute_name, source_id]], @model_rules[[ANY_ID, ANY_ATTRIBUTE, source_id]]]
+        attribute_rule = @model_rules[[ANY_ID, attribute_name, source_id]] unless attribute_name == ANY_ATTRIBUTE
+        levels = [record_rule(record, attribute_name, source_id), attribute_rule, @model_rules[[ANY_ID, ANY_ATTRIBUTE, source_id]]]

With this change it returns :model, and the full suite passes.

(Generated using Claude Code)

[njakobsen]

Fixed in cf6d4aa: with no attribute, the model-attribute level is empty, so a model-wide rule is labelled :model.

[njakobsen]

Thanks for the thorough review — each point is fixed in its own commit:

1. d98725f: the derived order column now skips the primary key, the key columns, and the owner's foreign key and type column. order_column: (a column name, or false) states it explicitly, and a bad column raises naming what to pass. Your repro is C107; the option and its errors are C108.
2. 419d7ce: Payload applies the owner's reject_if: (Proc, Symbol or :all_blank) and allow_destroy: to nested rows as Rails does. That includes existing rows, a row being destroyed is never rejected, and a new owner's rows are checked on an unsaved instance. limit: and update_only: raise rather than being silently ignored. Your repro is C111; C112 and C113 cover the rest.
3. c58cfa6: forge_diff snapshots and restores the member's precedence rules, with their precedence and destruction flags. Your repro is C109.
4. bdd02bb: a model-wide rule is labelled :model.
5. 42f1b05: we went with your preference. The payload is captured at save time and published after commit, and dropped on rollback, withdrawn-entry events included. Specs cover a save outside a transaction, inside one, a rollback, and a rolled-back forge (C110). This uses current_transaction, so the gemspec now requires Rails 7.2 or later.
6. The description is updated: mode:, the example count (278), and a few other claims that had gone stale.

The expiry work that builds on this is stacked above it in #18.

[Copilot]

🟡 Changes recommended

Unresolved correctness issues can remove intended members, apply the wrong merge values, and produce inaccurate previews.

15 open issues

• Use callback-free deletion for delete-all dependencies · New
• Include polymorphic type in derived member keys · New
• Cancel stale pending removal when reusing members · New
• Resolve pending attributes using the merge winner · New
• Pass source filters through association forging · New
• Honor association and source filters in previews · New
• Pass unsaved owners to method-based reject_if · New
• Preserve descending order direction during forging · New
• Treat forgotten members as neutral in complete reports · New
• Preview member changes caused by source withdrawals · New
• Respect association scopes when loading members · New
• Set inverse owner when building has-one members · New
• Deep-copy deferred event payload values · New
• Invalidate inactive-source cache after rollback · New
• Apply global precedence after specific rule ties · New

What changed in this PR

Adds experimental association tracking to LaForge’s source-based attribute resolution, addressing association support requested in #10 and building on #15’s precedence rules.

Changes:

• Tracks association membership, deferred member attributes, nested form edits, withdrawals, and tombstones.
• Adds duplicate merging, attribute groups, inactive sources, precedence ordering, and committed change events.
• Adds migration generators and extensive capability/importer specs; raises the Rails minimum to 7.2.

File	Description
spec/​support/​association_capabilities.rb	Defines association test models and schema.
spec/​spec_helper.rb	Adds reference fields and tracking columns.
spec/​lib/​usage_spec.rb	Covers references, groups, sources, events, and precedence orders.
spec/​lib/​association_importer_scenarios_spec.rb	Exercises multi-run importer scenarios.
spec/​lib/​association_capabilities_spec.rb	Covers association tracking capabilities.
spec/​lib/​activerecord_spec.rb	Updates precedence-ranking expectations.
Readme.md	Documents experimental APIs and upgrade steps.
lib/​laforge/​tombstone.rb	Represents exclusions and exposes blocking checks.
lib/​laforge/​precedence_rule.rb	Adds level-first ranking and record-rule queries.
lib/​laforge/​precedence_cache.rb	Updates cache documentation for inactive sources.
lib/​laforge/​payload.rb	Translates Rails form payloads into association edits.
lib/​laforge/​merge.rb	Merges duplicate records’ tracking data and members.
lib/​laforge/​member_list.rb	Represents membership reports and pending attributes.
lib/​laforge/​data_source.rb	Adds source activation and global ordering.
lib/​laforge/​data_entry.rb	Adds recording timestamps, events, and value checks.
lib/​laforge/​association_tracking.rb	Resolves membership, member attributes, and previews.
lib/​laforge/​association_config.rb	Configures association keys, ordering, and validation.
lib/​laforge/​activerecord.rb	Integrates tracking into recording and forging APIs.
lib/​laforge.rb	Loads the new tracking components.
lib/​generators/​laforge/​templates/​recorded_at_migration.rb.tt	Adds and backfills recording timestamps.
lib/​generators/​laforge/​templates/​migration.rb.tt	Includes new columns in fresh installs.
lib/​generators/​laforge/​templates/​active_sources_migration.rb.tt	Adds source activation to existing installs.
lib/​generators/​laforge/​recorded_at_generator.rb	Generates the timestamp upgrade migration.
lib/​generators/​laforge/​active_sources_generator.rb	Generates the source-activation migration.
laforge.gemspec	Requires Rails 7.2 or later.
Gemfile.lock	Aligns the locked Rails dependency requirement.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Custom association keys and successive unsaved recording and forging operations still have correctness issues that can misassign or retain members.

11 open issues

• Remove omitted unsaved members before autosave · New
• Use the survivor's reflected key when moving members · New
• Preserve excluded keys when merging complete reports
• Use association deletion to maintain counter caches
• Use configured primary keys for polymorphic associations · New
• Use reflected association keys when querying source values · New
• Preserve withdrawals when replacing a marked report · New
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Membership edits can lose removal history or fail to track an explicit exclusion, leaving members attached when they should be removed.

14 open issues

• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Preserve excluded keys when merging complete reports
• Use association deletion to maintain counter caches
• Repeated forget edit erases withdrawal before forge · New
• Explicit exclusions fail to track unreported existing members · New
• Batch inspection rescans reports quadratically per owner · New
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

_{Review effort: Balanced}

[njakobsen]

In general there are a lot of methods in the ActiveRecord submodules and many we end up prefixing with laforge_. Are they all reading instance data, or could we avoid cluttering the model namespace and extract some of these into singleton methods on the module that do not get mixed in. Let's discuss.

[njakobsen]

"Schedule" is the wrong word to use. It implies setting a date. How else could we describe these? Or do the belong with recording.rb since it's about recording that we want a specific change to happen. Or do they belong with forging, because these are for manipulating metadata used in forging, not the data we're storing on the record itself?

[njakobsen]

Fixed in d8e3bf3: these are now pending writes, the writes a forge has decided on but not made, which the record's next save makes. They belong with forging, as its output. ScheduledChanges is replaced by LaForge::PendingWrites, one per record, reached through laforge_pending_writes, with remove, remove_on_save, removed_members, cancel_removal, delete_entry, created_members and write!. The model loses ten laforge_* methods and four instance variables, and no "schedule" wording is left.

[Copilot]

🔵 Needs a closer look

Unresolved association validation, membership lifecycle, and polymorphic reference issues can persist invalid or unintended records.

13 open issues

• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Use association deletion to maintain counter caches
• Batch inspection rescans reports quadratically per owner
• Explicit exclusions fail to track unreported existing members
• Repeated forget edit erases withdrawal before forge
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

7 previously missed issues

In code that hasn't changed since last review

Remove unsaved members from autosaved targets

lib/​laforge/​pending_writes.rb:27

For a natural-key collection without dependent:, forging a new member and then forging an empty list before saving leaves that unsaved member in the autosaved target. Autosave inserts it before the queued removal nullifies its owner, creating an orphan that the final report excludes. Remove never-persisted members from the autosaved target while preserving reuse if another unsaved forge includes their key again.

Use the association primary key for polymorphic foreign keys

lib/​laforge/​recording.rb:155

With belongs_to :subject, polymorphic: true, primary_key: :uuid, foreign_key: :subject_uuid, this records the target's model primary key (id) instead of its uuid. Forging then produces a foreign key that cannot resolve to the intended target. Use the reflection's association primary key with the target class for polymorphic associations too.

Register validation callbacks when enabling autosave

lib/​laforge/​tracked_association.rb:27

For a has_one declared without autosave: true or validate: true, setting autosave here does not install Rails' validation callbacks, yet autosave saves the member with validation disabled. Consequently, forge! can persist a member that fails its model validations instead of rolling back. Register the autosave validation callbacks when enabling autosave, as Rails' nested-attributes setup does.

Use polymorphic_name for polymorphic entry queries

lib/​laforge/​tracked_association.rb:37

These polymorphic entry queries use the Ruby class name rather than Active Record's stored polymorphic name. With a namespaced model and store_full_class_name = false, source_values misses the owner's reports and the member-entry query below misses member attributes. Use polymorphic_name for both queries, as the owner-type filter already does.

Reject conflicting first reports for keyed has_one associations

lib/​laforge/​tracked_association.rb:209

For a keyed has_one whose existing row has never been reported, a first report naming a different key appends the old row to records as undecided and removes it from the removal candidates. Only records.first becomes the target, so saving leaves both rows attached or fails a unique owner constraint. Reject this conflict to preserve the unreported row, or explicitly schedule its replacement according to dependent:.

Destroy vacant partial reports without withdrawal keys

lib/​laforge/​tracked_association.rb:451

Forgetting a source's last exclusion or tombstone leaves a vacant partial report with no withdrawal keys, so this guard prevents its destruction even after forge!. The source remains in data_source_names and data_source? despite having no opinion, which can prevent orphan checks from recognizing an unclaimed record. Check for vacant reports even when there is nothing to withdraw.

Preserve tombstone mappings until exclusions are saved

lib/​laforge/​tracked_association.rb:476

After a higher-precedence source excludes a saved tenant and forge runs without saving, the exclusion names only a tombstone while the original row still exists. If a lower source re-reports that row's id before forge!, the exclusion no longer matches and the tenant's pending destruction is cancelled. Preserve the id-to-tombstone mapping until removal is saved so the lower source cannot bypass the exclusion.

1 resolved since last review

• Preserve excluded keys when merging complete reports

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved membership fallback, has_one validation, and member ordering issues can cause incorrect persisted data.

14 open issues

• Preserve lower-precedence members when destroying reports · New
• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Use association deletion to maintain counter caches
• Batch inspection rescans reports quadratically per owner
• Explicit exclusions fail to track unreported existing members
• Repeated forget edit erases withdrawal before forge
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

2 previously missed issues

In code that hasn't changed since last review

Register validation callback when enabling autosave

lib/​laforge/​tracked_association.rb:27

Enabling autosave after declaration does not install the associated-record validation callback that a plain has_one lacks by default. Its autosave path then saves the member without validation, allowing forge! to persist invalid reported attributes instead of raising and rolling back. Register the autosave validation callback when enabling autosave, as Rails does for nested attributes.

Reapply association order after forging members

lib/​laforge/​tracked_association.rb:211

When an owner forge applies pending member attributes, the member's own forge can reapply a directly recorded order-column value after write_order assigns the list position. Saving then persists that value instead of the resolved position, so reloading can reorder the members. Assign the association's order after forging its members so the list remains authoritative.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved association validation and merge data-loss issues prevent safe approval.

16 open issues

• Reject multi-key merged has_one reports to prevent data loss · New
• Install autosave validation callbacks for has_one members · New
• Preserve lower-precedence members when destroying reports
• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Use association deletion to maintain counter caches
• Batch inspection rescans reports quadratically per owner
• Explicit exclusions fail to track unreported existing members
• Repeated forget edit erases withdrawal before forge
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

1 previously missed issue

In code that hasn't changed since last review

Use collection deletion API to preserve counter-cache updates

lib/​laforge/​pending_writes.rb:98

For a collection with dependent: :delete_all and a belongs_to counter cache, member.delete bypasses both callbacks and Rails' association-level counter update. Removing a member through forge! therefore leaves the owner's cached count too high, unlike ordinary collection removal. Use the collection's deletion API here so it preserves callback-free deletion while maintaining the counter cache.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved membership-removal, forge-dispatch, reference-recording, and payload-validation issues can cause data loss or incorrect saved state.

20 open issues

• Dispatch forge through the record to preserve model wrappers · New
• Treat complete reports as withdrawals, not empty membership reports · New
• Use the association primary key for polymorphic references · New
• Reject unknown action keys in partial reports · New
• Install autosave validation callbacks for has_one members
• Reject multi-key merged has_one reports to prevent data loss
• Preserve lower-precedence members when destroying reports
• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Use association deletion to maintain counter caches
• Batch inspection rescans reports quadratically per owner
• Explicit exclusions fail to track unreported existing members
• Repeated forget edit erases withdrawal before forge
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

_{Review effort: Balanced}

[njakobsen]

The recording twice scenario is presented as though it was a common scenario, but maybe we should treat the first part of any subsection as the "golden path" and then talk about more complex scenarios in a subsubsection. I'm not sure if there are enough of these to make it a rule, but in this specific case it feels like the complex case is presented as if it was the natural continuation of the normal usage.

Let's discuss.

[njakobsen]

Agreed, and made it the rule for the whole README: each section opens with the common case and its example, and situations that only arise in unusual cases follow in a subsection named for the situation. Here that's When a new member is created twice. The same pass moved edge cases in Edits (Several edits before a forge, What an edit can't do) and Recording a source's members (When a source also records on the member directly) in 4064ce1, Expiring a change on #18, and Rulesets on #19.

[Copilot]

🔵 Needs a closer look

Unresolved association identity, singleton replacement, inspection, and inherited form-option issues can produce incorrect results.

20 open issues

• Reject unknown action keys in partial reports
• Use the association primary key for polymorphic references
• Treat complete reports as withdrawals, not empty membership reports
• Dispatch forge through the record to preserve model wrappers
• Install autosave validation callbacks for has_one members
• Reject multi-key merged has_one reports to prevent data loss
• Preserve lower-precedence members when destroying reports
• Use the survivor's reflected key when moving members
• Remove omitted unsaved members before autosave
• Use association deletion to maintain counter caches
• Batch inspection rescans reports quadratically per owner
• Explicit exclusions fail to track unreported existing members
• Repeated forget edit erases withdrawal before forge
• Preserve withdrawals when replacing a marked report
• Use reflected association keys when querying source values
• Use configured primary keys for polymorphic associations
• Assign resolved positions after forging association members
• Reject unmatched IDs before translating association keys
• Validate has_one destroy IDs before clearing the association
• Prune empty partial reports without withdrawn keys

6 previously missed issues

In code that hasn't changed since last review

Use subclass payload options for inherited associations

lib/​laforge/​payload.rb:174

Inherited tracked associations keep the parent as config.owner_class, so this lookup ignores accepts_nested_attributes_for options declared or overridden on the subclass. A subclass's allow_destroy: false, reject_if:, or unsupported-option checks can therefore be bypassed when recording its form. Pass the actual payload model through to the options lookup instead of using the class that originally declared tracking.

Query members by the configured owner key

lib/​laforge/​tracked_association.rb:36

owner_ids contains record ids, but the member foreign key contains values of the association's configured primary_key:. When those differ, source_values queries the wrong members and can omit their recorded attributes or associate another owner's rows with this owner. Query and group members by the configured owner key, while keeping the returned results keyed by record id.

Use polymorphic_name for association queries

lib/​laforge/​tracked_association.rb:37

These owner and member entry queries use base_class.name, but Active Record stores a polymorphic association's type using polymorphic_name. With a namespaced model and store_full_class_name = false, the stored type is demodulized, so source_values misses the reports and member attributes even though recording and forging work. Please use the polymorphic name in both queries, as PrecedenceRule.model_type already does.

Apply owner-dependent association scopes per owner

lib/​laforge/​tracked_association.rb:69

Owner-dependent scopes are silently dropped here, so source_values can include attributes from members outside an association such as has_many :tenants, ->(place) { where(unit: place.name) }. The instance method uses this batch path too, so even a single-owner lookup is affected. Apply the association's scope separately for each owner when it cannot be shared across the batch.

Handle conflicting keyed has_one reports

lib/​laforge/​tracked_association.rb:209

For a keyed has_one, a first report naming a different key retains the existing, unreported member as undecided while assigning the new member as the target. The old member is removed from existing, so it is never scheduled for removal; saving can leave both rows attached, and reloading can return the old one. Explicitly reject this singleton conflict or schedule the old member's replacement.

Respect the association's configured owner primary key

lib/​laforge/​tracked_association.rb:537

When an association declares a different owner primary_key:, this overwrites the foreign key supplied by scope_for_create with owner.id. The unsaved member then carries the wrong owner reference during key-only validation and tombstone matching, even if autosave later corrects it. Use the owner key specified by the reflection here.

_{Review effort: Balanced}