Skip to content

test(persistence): require every timestamped record to declare whether it is read by time - #1029

Merged
mforce merged 2 commits into
mainfrom
test/1024-chronology-signal
Oct 3, 2026
Merged

mforce merged 2 commits into
mainfrom
test/1024-chronology-signal

Conversation

@mforce

@mforce mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Closes #1024

test only, no call graph change

What changes

BusinessRecordModelTests now requires every mapped ICreatedRecord to appear in exactly one of two lists:

  • ChronologicalListTypes (existing, 12 types);
  • NotReadByTimeTypes (new, 14 types, each with a one-line reason).

The new test fails when a timestamped record is in neither list, in both, or when a listed type is not a mapped timestamped record. The existing Only_chronological_lists_have_a_unique_generated_sequence already fails when the chronological list and the module contributions disagree. Together, a new timestamped record can no longer skip the chronology decision silently.

The guard does not check that a declaration is true. A time-paged record declared in NotReadByTimeTypes stays green, and review of its reason is the only check. The 819 decision record and the AGENTS.md #819 paragraph state exactly this.

The set was measured on d6dbd47d by running the guard with an empty NotReadByTimeTypes: 14 undeclared plus 12 chronological, 26 timestamped records.

Mutation table

M2 is the #970 probe: a mapped TimePagedProbeRecord : IMutableRecord with a real IEntityTypeConfiguration, declared nowhere. "main" means origin/main's test file over the same tree.

# Mutation main branch
M0 none green (2/2) green (3/3)
M1 Expense removed from FinanceBusinessRecords only (the issue's row) red, Only_chronological_lists... red, Only_chronological_lists...
M2 new time-paged record, no declaration anywhere green red, names TimePagedProbeRecord
M3 Expense removed from the contribution and ChronologicalListTypes green red, names Expense
M4 Flock row deleted from NotReadByTimeTypes n/a red, names Flock
M5 Expense added to NotReadByTimeTypes too n/a red, "Declared more than once: Expense"
M6 AuditEvent (not timestamped) added to NotReadByTimeTypes n/a red, "Declared but not a mapped timestamped record: AuditEvent"
M7 Expense removed from ChronologicalListTypes only, contribution kept n/a red, both tests

M1 was already red on main, so the issue's own proof row was not discriminating. M2 and M3 are the gap, and they flip from green to red.

The harness that ran M0 to M6 (M7 was run by hand) is below.

1024-mutations.sh
#!/usr/bin/env bash
# Mutation run for #1024. Usage: 1024-mutations.sh <worktree> main|branch
set -uo pipefail
cd "$1"
mode=$2
test_file=tests/Cluckwork.Api.IntegrationTests/BusinessRecordModelTests.cs
expense=src/Cluckwork.Infrastructure/Persistence/Configurations/ExpenseConfiguration.cs
probe=src/Cluckwork.Infrastructure/Persistence/TimePagedProbeRecord.cs

restore() {
  git checkout -q HEAD -- src tests
  rm -f "$probe"
  [ "$mode" = main ] && git checkout -q origin/main -- "$test_file"
  return 0
}

run() {
  local label=$1 out failed
  out=$(dotnet test tests/Cluckwork.Api.IntegrationTests --filter "FullyQualifiedName~BusinessRecordModelTests" 2>&1)
  if grep -q "error CS" <<<"$out"; then echo "$mode $label BUILD-ERROR"; grep -m3 "error CS" <<<"$out"; restore; return; fi
  failed=$(grep -oE "^\s+Failed [A-Za-z_.]+" <<<"$out" | awk '{print $2}' | sed 's/.*\.//' | sort -u | paste -sd, -)
  echo "$mode $label $(grep -oE '^(Passed|Failed)!.*Total: +[0-9]+' <<<"$out" | head -1) ${failed:+failed=$failed}"
  grep -oP "Timestamped records missing.*?(?=\. A record)|Declared [^:]*: [A-Za-z, ]+" <<<"$out" | head -2 | sed 's/^/    /'
  restore
}

drop_expense_contribution() { sed -i 's/\[typeof(Expense)\],/[],/' "$expense"; }

restore
run M0-unmutated

drop_expense_contribution
run M1-contribution-entry-removed

cat > "$probe" <<'CS'
using Cluckwork.Domain.Common;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Builders;

namespace Cluckwork.Infrastructure.Persistence;

public sealed class TimePagedProbeRecord : IMutableRecord
{
    public Guid Id { get; private set; }
    public DateOnly Date { get; private set; }
    public DateTimeOffset CreatedAtUtc { get; private set; }
    public DateTimeOffset UpdatedAtUtc { get; private set; }
}

internal sealed class TimePagedProbeRecordConfiguration : IEntityTypeConfiguration<TimePagedProbeRecord>
{
    public void Configure(EntityTypeBuilder<TimePagedProbeRecord> builder) => builder.ToTable("TimePagedProbeRecords");
}
CS
run M2-new-time-paged-record-undeclared

drop_expense_contribution
sed -i 's/ typeof(Expense), typeof(DailyEntry)/ typeof(DailyEntry)/' "$test_file"
run M3-expense-removed-from-contribution-and-test-list

[ "$mode" = main ] && exit 0

sed -i '/(typeof(Flock), /d' "$test_file"
run M4-not-read-by-time-row-deleted

sed -i 's/^\(        (typeof(Account), \)/        (typeof(Expense), "mutation"),\n\1/' "$test_file"
run M5-type-in-both-lists

sed -i 's/^\(        (typeof(Account), \)/        (typeof(Cluckwork.Domain.Auditing.AuditEvent), "mutation"),\n\1/' "$test_file"
run M6-stale-row-not-timestamped

Test counts

Project main d6dbd47d branch
Application.Tests 675 (unchanged; this diff touches no file in that project) 675 passed
Api.IntegrationTests, --list-tests 1888 1889
BusinessRecordModelTests 2 passed 3 passed

The full integration suite was not run locally; CI runs it.

Deslop

pstack:deslop ran over the branch diff and removed nothing: the diff has no comments, casts or defensive code. Reason is deliberately not read by the test; it exists for reviewers.

…r it is read by time

Every mapped ICreatedRecord must now appear in exactly one of
ChronologicalListTypes and NotReadByTimeTypes in BusinessRecordModelTests.
A new timestamped record omitted from both lists fails, which closes the
#819 census gap where an omitted chronological contribution stayed green.

Closes #1024
@mforce

mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Review of record: Codex gpt-6-astra, round 1, at head de7b9ec9. Posted by the coordinator. Evidence paths named below are local to the review host.

Reviewed PR #1029 at de7b9ec9973f0c5de77f2f2a394fd07e368b37b0 against base d6dbd47da165ac4e12d6cf96a2751423535138a7. Read #1024, #970 and its recorded review/disposition, the #819 decision, AGENTS.md's guard rules, BusinessRecordModel, and all six module contributions. Review performed without delegation.

P2 | CONFIRMED | tests/Cluckwork.Api.IntegrationTests/BusinessRecordModelTests.cs:122

The new guard silently excludes owned ICreatedRecord entities, contradicting its promise to require a declaration for every mapped timestamped record.

Concrete failure scenario: add OwnedProbeRecord : IMutableRecord to a real Account.ReviewRecords collection, map it with OwnsMany into its own OwnedProbeRecords table, and read that collection with OrderBy(r => r.CreatedAtUtc).Skip(1).Take(10). Leave it out of both declaration lists and every chronological contribution. All three existing/added guard tests remain green. This is a mapped, timestamped record with a valid chronological paged query, not an unmapped CLR helper or an untimestamped owned value object.

Evidence:

  • owned-real.log: 4/4 passed. The fourth assertion independently proves that the entity is owned, implements ICreatedRecord, maps both timestamps and its own table, and produces SQL containing ORDER BY, LIMIT, and that table.
  • owned-real-filter-removed.log: removing only !entity.IsOwned() from the new declaration guard produces 1 failure and 3 passes. The failure explicitly names Cluckwork.Domain.Accounts.OwnedProbeRecord as undeclared.
  • Reproduction script and probe assertions preserve the exact mutation. No database was used; the query check compiles SQL through the real provider.
  • docs/decisions/819-business-record-chronology.md:114 says every mapped ICreatedRecord must appear in a list, and line 119 says a new timestamped record cannot be omitted silently. AGENTS.md:91 repeats that claim. Neither states the owned-type exception. The production model walk already excludes owned types, so it provides no fallback rejection.

Make this case fail closed by including owned timestamped entities in the declaration check, or explicitly rejecting them if the persistence policy does not support them. If owned records are deliberately outside this slice, narrow the documented guarantee and explicitly record the remaining gap instead of claiming universal coverage.

This is a guard-coverage and documentation defect, not a current product defect. No product defect introduced by this PR was found.

Verification results

Every run targeted BusinessRecordModelTests in Cluckwork.Api.IntegrationTests.csproj, with -m:1 -p:UseSharedCompilation=false. The baseline passed 3/3. The base comparisons replaced only the test file with the exact PR-base version, matching the PR's comparison method.

Probe Result
M2: new IMutableRecord, registered only through IEntityTypeConfiguration Base green; head red, naming TimePagedProbeRecord.
M3: remove Expense from its contribution and test chronology list Base green; head red, naming Expense.
Indirect ICreatedRecord implementation Caught by M2. IMutableRecord inherits ICreatedRecord; IsAssignableFrom includes it.
Explicit model registration, queried via Set<T>(), no DbSet property Red, naming the probe; independent paged-query SQL assertion passes. M2 also verifies configuration-only registration and Set<T>().
TPH derived record, with only its base explicitly declared Red, naming DerivedProbeRecord.
TPT derived record, with only its base explicitly declared Red, naming DerivedProbeRecord.
Shadow timestamps, no timestamp interface Red in the existing production census: no business-record timestamp policy.
Shadow timestamps with explicit interface implementation Red in the new declaration guard, naming ShadowProbeRecord.
Owned timestamped collection in a separate table Green, confirming the finding above.

M2's totals include one extra passing assertion proving the model registration and paged Set<T>() query: base 3/3, head 3 passed and 1 failed. M3 totals are base 2/2, head 2 passed and 1 failed. Logs and source snapshots are in the evidence directory. Initial owned-probe setup attempts had an analyzer error and then an invalid shadow navigation; neither is counted as guard evidence. The final owned probe uses a real CLR navigation and passes its independent assertions.

The subject set comes from db.Model.GetEntityTypes(), not a recalled list of entities or DbSet properties. The two handwritten lists are declarations checked against that discovered set. The owned filter is the confirmed omission. Duplicate and stale declarations are explicitly rejected. Set operations make pass/fail independent of EF enumeration order; culture-sensitive diagnostic sorting does not change the result. No current-model false positive was found. The targeted build enforces #985 and produced no style violation in the PR.

Exclusion reasons checked

All 14 classifications agree with current reads. No newly misclassified time-paged screen or list was found.

Type Code evidence
Account AccountRepository.cs:13,48 resolves the current farm or slug; ListAccountsCliCommand.cs:33 orders by slug.
ApplicationUser IdentityProvider.cs:1408 orders the user list by email.
Customer CustomerRepository.cs:17,39 orders both paged lists by name, then id.
DailyEntryGrade DailyEntryRepository.cs:14,55 loads grades through their entry; ExportQueries.cs:139 orders by entry id, then id.
EggGrade EggGradeRepository.cs:18,24 orders by sort order, then name.
EggUnitConversion CatalogRepositories.cs:51 orders by unit code.
ExpenseCategory ExpenseRepositories.cs:18,24 orders by name.
FarmLogo FarmLogoRepository.cs reads one farm row; FarmLogoConfiguration.cs enforces unique (AccountId, FarmId).
Flock FlockRepository.cs:40,76 pages by name and id. ExportQueries.cs:110 uses placement date and id, explicitly preserved by the existing #819 decision.
InventoryItem InventoryItemRepository.cs:18 orders by name.
Product CatalogRepositories.cs:17 orders by name, then id.
ProductEggGradeMapping CatalogRepositories.cs:31,34 reads by product or loads mappings; CatalogConfiguration.cs:48 enforces one mapping per product. No chronological ordering.
SalesOrderAllocation SalesOrderAllocationRepository.cs:16 filters by order; ExportQueries.cs:194 orders by order id, then id.
UserRoleAssignment UserRoleAssignmentRepository.cs:16,49 loads memberships by id or as a set, with no chronological ordering.

The documentation correctly admits that declarations can be false and that review must assess their reasons. Its unsupported claim is the coverage of every mapped ICreatedRecord, addressed in the finding above.

Nits: none.

All throwaway source changes were restored. The final clean-tree run passed 3/3; see final-clean.log. git status --short and git diff --check were clean at the assigned head. No commits, pushes, GitHub comments, other checkouts, or branches were changed. Full integration tests were not run, as requested.

Verdict: request changes for one confirmed P2 guard/documentation gap; no product defect found.

… declaration guard

Astra round 1 on #1029: an owned ICreatedRecord mapped with OwnsMany to
its own table skipped the declaration check. BusinessRecordModel
configures no timestamps or Sequence for owned types, so the guard now
rejects them outright instead of asking for a declaration.
@mforce

mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Astra round 1, P2 (owned ICreatedRecord skipped the declaration check): fixed in d8fb16e.

The fix rejects owned types rather than including them. BusinessRecordModel.Apply drops owned entity types before it configures timestamps or Sequence, so the persistence policy cannot support an owned timestamped record. Declaring one would therefore always be false. Every_timestamped_record_is_declared_chronological_or_not_read_by_time now fails first on any owned entity type implementing ICreatedRecord, names it, and says to map it as an entity type. Today's model has none, so the clean run passes 3/3.

Run Head Result
Your owned probe (OwnedProbeRecord via OwnsMany into OwnedProbeRecords), copied verbatim from owned-real-Account.cs + owned-real-ReviewProbeRecord.cs de7b9ec green (your owned-real.log)
Same probe d8fb16e red, 1 failed / 2 passed: "Owned entity types cannot be timestamped business records: Cluckwork.Domain.Accounts.OwnedProbeRecord"
Clean tree, probe reverted d8fb16e green, 3/3
M0 to M6 harness rerun d8fb16e unchanged: M0 green, M1 to M6 red naming the expected type

Docs changed in the same commit, at exactly this strength. The 819 record's "Migration and enforcement" section and the AGENTS.md #819 paragraph now say owned timestamped types are rejected outright. The existing limit stays stated: a declaration is not checked for truth.

Test only, no call graph change. This round found no product defect.

@mforce

mforce commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Review of record: Codex gpt-6-astra, round 2, at head d8fb16e8. Posted by the coordinator.

Round-two review of PR #1029 at d8fb16e8497a7c200867359be82a04280321c799, limited to the fix since de7b9ec9. Read the fix-only diff and the worker's reply. Review performed without delegation.

No new P0–P3 findings. The round-one P2 is CONFIRMED FIXED at tests/Cluckwork.Api.IntegrationTests/BusinessRecordModelTests.cs:121.

The original failure scenario was an undeclared OwnedProbeRecord : IMutableRecord, mapped through Account.ReviewRecords with OwnsMany into its own table and queried with creation-time ordering and pagination. That scenario passed the guard in round one. Re-running the same mapping and query on this head produces exactly one failure, at the new owned-type assertion:

Owned entity types cannot be timestamped business records: Cluckwork.Domain.Accounts.OwnedProbeRecord.

The other three tests pass, including the independent assertion proving the owned mapping, both timestamp properties, the separate table, and successful paged-query SQL generation. That assertion also confirms CreatedAtUtc has ValueGenerated.Never and no Sequence exists. This is the intended guard failure, not a build error or invalid EF mapping. See owned-marker.log and the exact probe.

No product defect was found. This remains a test-and-documentation change with no production behavior change.

Policy and current-model check

The worker's explanation is accurate. BusinessRecordModel.cs:24 removes owned entity types before the timestamped set is built at line 27 and configured at line 46. Sequence configuration follows ValidateCensus, whose subset check at line 85 rejects chronological contributions outside that timestamped set. An owned record therefore cannot acquire this policy's timestamp and sequence configuration merely by joining either test declaration list. Rejecting timestamp-interface owned records is appropriate for the current implementation; supporting them would require an explicit persistence-policy extension.

The real model contains five owned mappings, all for Money: FeedUsage.EstimatedCost, InventoryItem.DefaultUnitCost, InventoryLot.UnitCost, SalesOrder.TotalAmount, and SalesOrderItem.UnitPrice. None implements ICreatedRecord or maps CreatedAtUtc. The inventory assertion and the existing tests passed 4/4. See owned-inventory.tsv.

The rejection tests interface assignability, not property names. A synthetic owned value with an ordinary CreatedAtUtc property and no timestamp interface remains green, 4/4. The same is true for a shadow CreatedAtUtc, 4/4. Both were valid separate-table mappings with independently verified paged queries. Thus the fix does not reject an owned value merely because it carries a similarly named field.

Reachable edge cases

Shape Evidence and result
Owned type declaring only IMutableRecord This is the original probe, and it is rejected by name. IMutableRecord : ICreatedRecord in src/Cluckwork.Domain/Common/IMutableRecord.cs:3; implementing the former while not implementing the latter is not a reachable CLR shape.
Owned CLR timestamp, no interface owned-plain-timestamp.log: 4/4 pass. Outside the interface-defined policy.
Owned shadow timestamp, no interface owned-shadow-timestamp.log: 4/4 pass. The probe asserts that the timestamp is genuinely a shadow property.
Non-owned shadow timestamps, no interface or explicit exclusion nonowned-shadow-no-interface.log: all three tests fail in the existing production census, naming ShadowProbeRecord as having no business-record timestamp policy.
Undeclared timestamp-interface keyless table keyless-table.log: one failure naming TimePagedProbeRecord; three passes.
Undeclared timestamp-interface keyless view keyless-view.log: one failure naming TimePagedProbeRecord; three passes.
Undeclared timestamp-interface keyed view keyed-view.log: one failure naming TimePagedProbeRecord; three passes.

Each keyless/view probe separately asserts its actual key and table/view metadata and compiles a paged query through Npgsql. These are reachable EF mappings, and neither keylessness nor view mapping escapes discovery through GetEntityTypes().

The remaining boundary is explicit: this guard classifies ICreatedRecord implementations; it does not discover business-record intent from arbitrary timestamp columns or query sites. Owned values without the interface remain outside that guarantee. The updated #819 decision and AGENTS.md accurately add the owned-interface prohibition and retain the declaration-truth limitation. No further fix-scoped documentation finding.

Nits: none. The new code follows #985, and targeted compilation produced no style violations.

Verification used only BusinessRecordModelTests, with -m:1 -p:UseSharedCompilation=false; no database or full integration suite was needed. The mutation script and evidence directory preserve the probes. All mutations were restored, and the final clean-tree run passed 3/3. git status --short and git diff --check were clean at the assigned head. No commits, pushes, GitHub comments, other checkouts, or branches were changed.

Verdict: approve the round-two fix; the prior P2 is resolved, with no new finding or product defect.

@mforce
mforce merged commit b88ec0e into main Oct 3, 2026
16 checks passed
@mforce
mforce deleted the test/1024-chronology-signal branch October 3, 2026 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(arch): fail-closed signal that a table is read by time (#819 census gap)

1 participant