Skip to content

test(e2e): assert the Factory Worker task-record contract, not shadow task data - #2094

Merged
Soph merged 2 commits into
mainfrom
soph/e2e-factory-task-record-contract
Aug 21, 2026
Merged

Soph merged 2 commits into
mainfrom
soph/e2e-factory-task-record-contract

Conversation

@Soph

@Soph Soph commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/1120

E2E Tests has failed on every push to main since ed9c31c0d (#2032, "capture background subagent work durably") — seven consecutive runs. One job (e2e-tests (factoryai-droid)), two tests, both at expected task checkpoint within 30s, and both failing again on gotestsum's automatic re-run, so this is deterministic rather than agent nondeterminism.

It bisects cleanly: run 32411315115 on ed34d4db5 (#2018) had Factory green, and ed34d4db5 is ed9c31c0d's only parent.

This is not a bug in #2032

waitForTaskCheckpoint polled the shadow branches for a tasks/ path — exactly what #2032 stopped writing. Under the new model a Worker's turn lands as a completed session.TaskRecord on the parent, and the transcript is materialized into the parent's checkpoint under tasks/<toolUseID>/ at condensation time.

#2032 already moved everything else onto that contract — its own integration test now asserts the inverse ("a Worker's turn must write a task record, not shadow data"), and CLAUDE.md says "records never live on the shadow branch". These two E2E tests were the only holdout, and they could not object during review because E2E Tests runs on push-to-main and never on a PR.

What this does

Moves both tests onto the shipped contract, in both directions:

  • waitForCompletedTaskRecord polls .git/entire-sessions/*.json for a record with a non-empty completed_at. omitempty drops the zero value, so presence is the completion signal. Same strength as the existence check it replaces.
  • assertNoShadowTaskData pins the inverse: no shadow branch carries a tasks/ path. Narrowed to task data deliberately — a parent session legitimately has shadow branches of its own, because shadow pinning keys on StepCount and says nothing about task content.

TestFactoryTaskCheckpointExistsBeforeCommit → TestFactoryTaskRecordExistsBeforeCommit. There is no pre-commit checkpoint under the new model, but the durability intent in the name is still what's tested: the Worker's work is captured before the user commits.

Also updates the stale cross-reference and pre-#2032 wording in factoryai_worker_session_test.go's header comments, which named the renamed test.

Scope note

Whether dropping the pre-commit shadow copy weakens durability is #2058's question, not this PR's. These tests assert what shipped; they do not settle whether it should have. Flagging explicitly so the red build isn't mistaken for that question being answered.

Verification

  • mise run fmt && mise run lint — clean (0 issues)
  • mise run test:ci — integration ok, canary 56/56 and 4/4, no failures
  • go vet -tags=e2e ./e2e/tests/ — clean
  • The changed E2E tests themselves are not run here: they need a real Factory Droid agent, so E2E Tests on merge to main is the actual verification. Prompts are untouched (Vogon regexes unaffected) and both tests skip on non-Factory agents (canary unaffected).

Also worth fixing separately

E2E Tests being push-to-main-only is what let this land. The Worker task path has no pre-merge coverage at all, and the canary is free and deterministic — extending it to cover the Worker task record would have caught this in review.

🤖 Generated with Claude Code


Note

Low Risk
Test-only assertion changes; no production behavior. Residual risk is E2E-only verification on merge to main.

Overview
Aligns Factory Droid E2E coverage with the post-#2032 contract: a Worker turn is a completed parent TaskRecord before commit, not tasks/ metadata on a shadow branch.

waitForTaskCheckpoint now polls .git/entire-sessions/*.json for a record with tool_use_id and completed_at, and assertNoShadowTaskData fails if any shadow tree still has a /tasks/ path. The pre-commit test is renamed to TestFactoryTaskRecordExistsBeforeCommit. Integration-test comments are updated to match.

Reviewed by Cursor Bugbot for commit 33d5866. Configure here.

… task data

E2E Tests has failed on every push to main since ed9c31c (#2032, "capture
background subagent work durably"). One job, factoryai-droid, two tests, both
at "expected task checkpoint within 30s" — and failing again on gotestsum's
re-run, so deterministic rather than agent nondeterminism. Bisects cleanly:
ed34d4d (#2018) was green and is ed9c31c's only parent.

It is not a bug in #2032. waitForTaskCheckpoint polled the *shadow branches*
for a tasks/ path, which is precisely what #2032 stopped writing: a Worker's
turn now lands as a completed session.TaskRecord on the parent, and the
transcript is materialized into the parent's checkpoint under tasks/<id>/ at
condensation time. #2032 updated its own integration test to the inverse
assertion ("a Worker's turn must write a task record, not shadow data") and
said so in CLAUDE.md ("records never live on the shadow branch"); these two
E2E tests were the only holdout, and they could not object during review
because E2E Tests runs on push-to-main, never on a PR.

So move them onto the shipped contract, in both directions:

- waitForCompletedTaskRecord polls .git/entire-sessions/*.json for a record
  with a non-empty completed_at (omitempty drops the zero value, so presence
  is the completion signal). Same strength as the existence check it replaces.
- assertNoShadowTaskData pins the inverse — no shadow branch carries a
  tasks/ path. Narrowed to task data on purpose: a parent session
  legitimately has shadow branches of its own, since shadow pinning keys on
  StepCount and says nothing about task content.

TestFactoryTaskCheckpointExistsBeforeCommit becomes
TestFactoryTaskRecordExistsBeforeCommit — there is no pre-commit checkpoint
under the new model, but the durability intent the name carries is still
tested: the Worker's work is captured before the user commits. Prompts are
untouched, so Vogon's regexes are unaffected; both tests skip on non-Factory
agents, so the canary is too.

Whether dropping the pre-commit shadow copy weakens durability is #2058's
question, not this test's. These assert what shipped; they do not settle it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0J4CJ6CCGRXYYVWBSB8Q9H1
Copilot AI lite review requested due to automatic review settings August 21, 2026 12:23
@Soph
Soph requested a review from a team as a code owner August 21, 2026 12:23

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

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

Reviewed by Cursor Bugbot for commit 33d5866. Configure here.

Comment thread e2e/tests/factory_hooks_test.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Factory Droid coverage to assert the post-#2032 “task record → condensation materialization” contract: a Worker’s work should appear as a completed session.TaskRecord in session state before the user commits, and must not be written as tasks/ data on any shadow branch.

Changes:

  • Rename and update the Factory E2E test to wait for a completed task_records[] entry in .git/entire-sessions/*.json rather than polling shadow-branch tasks/ paths.
  • Add an E2E assertion that no shadow branch contains /tasks/ metadata paths.
  • Refresh integration-test header comments to match the task-record model and updated E2E test naming.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
e2e/tests/factory_hooks_test.go Moves E2E assertions from shadow tasks/ metadata to completed task-record state + adds a “no shadow task data” assertion.
cmd/entire/cli/integration_test/factoryai_worker_session_test.go Updates comments to reflect that Workers are attributed via task records on the parent session.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread e2e/tests/factory_hooks_test.go
…own file

Two review findings on the new helper, both correct.

Bugbot: the completion check was wrong. `CompletedAt` is tagged
`json:"completed_at,omitempty"`, and omitempty does not drop a zero struct —
only false, 0, nil, and empty string/slice/map/array. A zero time.Time
therefore serializes as "0001-01-01T00:00:00Z", so testing the decoded string
for != "" reported *every* record complete. Verified by marshaling the real tag
shape: an in-flight record emits the field. Since a Factory Worker's record is
created and completed in the same mutation this path never observed an
in-flight record, so the test still passed — it was asserting almost nothing,
which is worse than failing. Decode into time.Time and use IsZero, which is the
only honest check, and drop the comment that claimed the opposite.

Copilot: the predicate accepted any completed record, so an unrelated task
could satisfy a test whose Worker never produced one. Require the file the
caller already waited for to appear on the record. That is safe as well as
stronger: saveSubagentSessionTaskStep always sets Files from the Worker turn's
detected changes (mergeUnique of modified/new/deleted), and the file is created
during that turn, so it is present by construction. It also turns the helper
from "some task finished" into "this Worker's work is attributed to the task",
which is the contract actually under test.

The failure message now lists the records it did see, so a future failure names
the cause instead of costing a detour — the same lesson the shadow-branch
version taught by reporting "backup hook did not run".

Predicate verified against marshaled records across seven states: in-flight
with and without files (both previously false positives), completed with the
wrong file, completed read-only with no files, completed with the file,
unrelated-plus-ours, and no records at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0J5CB0VPY2JDZTYK5CD53QC
@Soph
Soph merged commit 56258ab into main Aug 21, 2026
12 checks passed
@Soph
Soph deleted the soph/e2e-factory-task-record-contract branch August 21, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants