test(resync): prove a resync record reaches DING as stream work - #366
test(resync): prove a resync record reaches DING as stream work#366schickling-assistant wants to merge 1 commit into
Conversation
`tests/resync.rs` deferred the DING wake to "the existing delivery suite", but that suite only covers a publicly emitted stream event, which reaches the inbox through a different admission path than `emit_builtin_resync` and is read with `message::list_inbox` rather than the `new_arrivals` scan the live loop actually uses. Nothing proved a resync record survives that scan. Add `tests/resync_ding.rs` covering the join against the real supervisor ingress, and register the target in the derivation that already gates live resync integration. Verified the test is load-bearing: filtering resync out of `new_arrivals` makes it fail on the arrival assertion. agent-identity: dev3.direct.claude.gvacdkt7 agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@cab57ad
|
The
Leaving it alone per #368 rather than folding an unrelated fix into this PR. Posted on behalf of @schickling
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387a4a6417
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| .is_empty() | ||
| ); | ||
| std::thread::sleep(Duration::from_millis(300)); |
There was a problem hiding this comment.
ResyncSupervisor::refresh only queues Msg::WatchSet and returns without waiting for the worker to install it, so this fixed 300 ms delay is racy. On a loaded CI runner, the goal can be written before the worker processes the refresh; rebuild_carriers then treats the new contents as its initial baseline and emits nothing, causing the 15-second wait to fail. Use the acknowledged install_live path or another readiness handshake before writing the carrier.
Useful? React with 👍 / 👎.
1 participant
Why
tests/resync.rs:2-3says, in its own module doc:That deferral does not land anywhere. The existing delivery suite proves the wake for a publicly
emitted stream event —
tests/event_e2e.rs::event_emit_cli_returns_a_stable_json_receipt_and_ding_marks_the_record— andthat test differs from the resync case in the two ways that matter:
emitadmission path, notStreamAdmission::BuiltinResync.message::list_inboxdirectly, not withding::new_arrivals— the scanthe live DING loop actually runs, and the one place a stream predicate would silently swallow a
record.
So the sentence "resync records reach a live agent" was, until now, unproven at the seam. The
Bounded idempotent stream ingressinvariant's "DING marks them as stream work" clause is provenonly for the public path.
What
One new integration target,
tests/resync_ding.rs, that drives a realResyncSupervisorover atemp catalog, changes a goal carrier, and then asserts through DING's own arrival scan that the
resulting record is delivered and rendered as stream work.
How
The test mirrors
run_ding's startup discipline — seed theseenset from the current unread setfirst, so the post-change scan can only report the new arrival — and then asserts four things:
new_arrivalsat all (the filter-out case);stream: resyncand an event id;poke_textrenders it[DING] » hetz.worker/resync: resource goal changed …, i.e. with thenon-agent source marker rather than a relationship arrow;
tests/resync.rs's module doc is updated to point at the new file instead of deferring to a suitethat did not cover it.
The target is registered in
st2WasmResolverCheck, the derivation that already gates live resyncintegration. The test itself needs no
wasm-resolverfeature — it uses a native catalog-relativebinding — but that derivation is where the resync supervisor is demonstrably exercised in CI today,
so it is the placement with evidence behind it rather than a guess. Since test targets are
enumerated explicitly, registering it is what makes it run at all.
Rationale
Why a separate file rather than adding to
tests/resync.rs. That file's stated scope is theingress half, and its module doc explicitly excludes the wake. A separate target keeps that boundary
honest and makes the new gate independently nameable.
The test is load-bearing, and I checked rather than assuming. Patching
new_arrivalsto filterstream == "resync"out of its result — the exact regression this guards — makes it fail on thearrival assertion (
left: 0, right: 1). The patch was reverted;src/is untouched by this branch.No production behavior changes. This is coverage plus one comment correction and one flake test
registration.
Verification
4 tests pass locally (3 pre-existing in
resync, 1 new). CI drives the same targets throughst2WasmResolverCheck.Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile