manifest: retire the scheduled-task vocabulary - #21
Conversation
Upstream issue #14, maintainer ruling: scheduled work will never live in this daemon -- it belongs to a separate service. ModuleManifest.scheduled_tasks was required, forcing every module author to declare a field for work the daemon never executes. Rust (crates/subc-protocol/src/manifest.rs): drop ModuleManifest.scheduled_tasks and the ScheduledTask/TaskEligibility/LeaseScope/ModelPolicy/CircuitBreaker types, plus every fixture/constructor reference across subc-client-rs, subc-core, subc-mcp, and subc-protocol tests. control.rs's manifest() test helper built a fully-populated ScheduledTask but the surrounding tests (hello-frame parsing, route liveness, push-op handling) were never about scheduled tasks -- the literal was incidental filler, so the field is dropped and the tests kept. TypeScript (clients/subc-client): drop the mirrored required field, ScheduledTaskInput/TaskEligibilityInput/ModelPolicyInput/CircuitBreakerInput/ LeaseScope, normalizeScheduledTask, and the stale ModelPolicyInput re-export in index.ts. Swift confirmed unaffected -- SubcClient never named this vocabulary. Regenerated crates/subc-protocol/tests/golden/module_hello_body.json (the serialized HELLO body carries the whole manifest, so removing the field necessarily moves it) -- the only golden fixture that changes; crates/subc-control/tests/golden/ is untouched.
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/subc-protocol/src/manifest.rs">
<violation number="1" location="crates/subc-protocol/src/manifest.rs:21">
P1: When a new module sends HELLO to an older daemon, registration fails because the older required `scheduled_tasks` field is now omitted. Coordinate this wire-shape change with a protocol/versioned rollout, or continue emitting the field until older daemons are retired; `deny_unknown_fields` does not provide backwards compatibility for missing required fields.</violation>
</file>
You're on the cubic free plan with 11 free PR reviews remaining this month. Upgrade for unlimited reviews.
Re-trigger cubic
| pub trust_tier: TrustTier, | ||
| pub provides: Vec<ProviderRole>, | ||
| pub consumes: Vec<ConsumerRole>, | ||
| pub scheduled_tasks: Vec<ScheduledTask>, |
There was a problem hiding this comment.
P1: When a new module sends HELLO to an older daemon, registration fails because the older required scheduled_tasks field is now omitted. Coordinate this wire-shape change with a protocol/versioned rollout, or continue emitting the field until older daemons are retired; deny_unknown_fields does not provide backwards compatibility for missing required fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-protocol/src/manifest.rs, line 21:
<comment>When a new module sends HELLO to an older daemon, registration fails because the older required `scheduled_tasks` field is now omitted. Coordinate this wire-shape change with a protocol/versioned rollout, or continue emitting the field until older daemons are retired; `deny_unknown_fields` does not provide backwards compatibility for missing required fields.</comment>
<file context>
@@ -18,7 +18,6 @@ pub struct ModuleManifest {
pub provides: Vec<ProviderRole>,
pub consumes: Vec<ConsumerRole>,
- pub scheduled_tasks: Vec<ScheduledTask>,
pub bindings: Bindings,
}
</file context>
There was a problem hiding this comment.
Valid, and my compatibility claim in the PR body is wrong. I asserted "compatible both directions" and only tested one of them.
Reproduced the failing direction rather than reasoning about it further:
#[derive(serde::Deserialize)]
struct OldManifest { module_id: String, scheduled_tasks: Vec<String> }
serde_json::from_str::<OldManifest>(r#"{"module_id":"aft"}"#)
// → Error: missing field `scheduled_tasks` at line 1 column 19ModuleManifest at master carries no #[serde(default)] on that field, so it is genuinely required on deserialize. deny_unknown_fields governs extra keys and says nothing about missing ones. I conflated the two.
The directions are not symmetric:
| old daemon | new daemon | |
|---|---|---|
| old module sends the field | parses | parses — extra key ignored |
| new module omits it | FAILS — missing required field | parses |
The safe direction is old-module→new-daemon, which is the one I checked and the one my deny_unknown_fields reasoning actually covers. The unsafe one is new-module→old-daemon — exactly the deployment order that happens in practice, since clients upgrade independently of the daemon.
This box demonstrates the hazard: my daemon ran a build 623 commits stale until this afternoon while modules were rebuilt against a much newer tree. A skew window measured in days.
Options, and the call is yours
Cubic's suggestion — keep emitting the field — defeats the purpose, since authors would still have to declare it.
I also can't fix this from the new side. The old daemon is the one that needs the key present, and no change to a new module makes an already-shipped binary tolerate its absence. So:
- Land as-is with a hard ordering constraint — daemon upgrades before any module rebuilt against the new manifest.
ck daemon'sbuild_git_shamakes that checkable rather than hoped-for, and it's a one-time cost on a fleet you control. - Add
#[serde(default)]to the field on master first, ship it, then excise in a later release. The intermediate daemon tolerates both shapes and the ordering constraint disappears.
Option 2 is the correct protocol move and costs one release cycle. Option 1 is defensible only because every consumer is in your fleet.
If you want option 2, this PR should probably become the #[serde(default)] commit and the excision waits behind it. Say which and I'll reshape it.
Worth naming the general shape, since it bit me twice today: I checked the direction that confirmed my conclusion and stopped. Same error as my MASKED verdict on #11 — one explanation enumerated, the other never asked about.
There was a problem hiding this comment.
The parent comment still applies: your reproduction confirms that a new module omitting required scheduled_tasks cannot register with an old daemon, while the reverse direction remains safe. I’d choose option 2—ship a #[serde(default)] compatibility release before excising the field—because the demonstrated multi-day skew makes daemon-first ordering fragile. deny_unknown_fields only addresses extra keys, and continuing to emit the field would not achieve the intended removal.
Thanks for the feedback! I've saved this as a new learning.
|
Reviewed in full — the removal is complete on both halves (Rust types gone, TS interface gone, every constructor site swept, the Two gates before merge, one yours and one mine to run: 1. Version bump, in this PR. 2. Deploy-ordering fact, for the record rather than for you to fix: the old Push the bump and I'll run your head through the internal twin for the full matrix (same flow as #10/#18), then merge on green. |
The scheduled-task excision changes what a module serializes in HELLO, and seven repos path-depend on subc-protocol without being able to see that the code moved (a path dependency records a bare version with no source and no checksum, so a --locked build over there cannot tell). The minor is what carries a manifest-shape change per the maintainer's note on the PR. Three in-workspace dependents pin "0.10", which is a caret requirement that excludes 0.11 -- subc-control, subc-client-rs, and subc-transport all fail to resolve until they take the new minor. Bumped alongside, verified by a cargo check that reproduced the resolution failure first. @cortexkit/subc-client 0.5.0 -> 0.6.0: removing a required interface field is breaking for anyone typechecking against it.
check-wire-crate-versions.sh flags subc-core because this branch edits lines under src/. All three edits are unreachable from a library consumer's build: the control.rs change is inside #[cfg(test)] mod tests, bench_harness.rs is #[cfg(feature = "bench-harness")] and off by default, and fake-aft-stub is a src/bin auto-binary rather than part of the lib. Taking the bump the check asks for rather than arguing past it. Patch, not minor: nothing a consumer compiles by default moved, and ck-subc's own output is byte-unaffected.
The lock records the workspace crates' own versions, so it moves with the bump. Committed separately because the test run that regenerated it landed after the version commit -- a --locked build would have failed on the skew.
|
Both gates addressed, pushed through Version bump — four crates, not one
The check also flagged
|
Closes #14.
scheduled_tasksand its supporting types are gone fromModuleManifest, in Rust and TypeScript.Both halves in one commit, per the reasoning on the issue: a manifest field required by one client and absent from the protocol is the same drift that made
scheduler.*readable as a roadmap.Removed
Rust —
crates/subc-protocol/src/manifest.rs: the field, plusScheduledTask,TaskEligibility,LeaseScope,ModelPolicy,CircuitBreaker, and the fixture that was their only constructor. Ten call sites droppedscheduled_tasks: Vec::new().TypeScript —
clients/subc-client/src/provider.ts: the requiredscheduled_tasksfield,ScheduledTaskInput/TaskEligibilityInput/ModelPolicyInput/CircuitBreakerInput, theLeaseScopetype, both construction sites, andnormalizeScheduledTask. Five re-exports removed fromindex.ts.Swift confirmed unaffected — zero hits under
clients/subc-client-swift.control.rs'smanifest()helper is a generic fixture shared by hello-frame parsing, route-liveness, and push-op tests — none of them about scheduled tasks. ItsScheduledTaskliteral was incidental filler, so the field was dropped and every test kept.The fixture, called out rather than buried
crates/subc-protocol/tests/golden/module_hello_body.jsonloses one line. That's the serialized HELLO body carrying the whole manifest, so the field's removal necessarily moves it — a real wire-shape change, not a test artifact.subc-controlis untouched, per your review criterion.Compatibility both directions: no
deny_unknown_fieldsanywhere insubc-protocol, so a new daemon parses an old module's HELLO that still sendsscheduled_tasks, and vice versa. Only the canonical fixture moves.Migration cost — I under-priced this on the issue and want it stated correctly
I originally said "one line per Rust module author." That was measured on the Rust side and stated as though it covered the fleet. Corrected:
@cortexkit/subc-clientneeds a release — removing a required interface field is breaking for anyone typechecking against it.Verification
bunx tsc --noEmitfails, and it is pre-existing —error TS2688: Cannot find type definition file for 'node'.node_modulesisn't installed in this checkout, sotsccan't resolve anything. I verified this rather than asserting it: the identical error reproduces on a clean worktree of unmodified06a2e76.bun testresolves types through Bun's own toolchain and passes, which is why the TS suite is meaningful and thetscrun isn't.Flagging it because a typecheck that structurally cannot run is a gate whose silence means nothing — if CI installs deps, it will typecheck properly there, and that's the run I'd trust over mine.
One process note
My subagent reported a dangling
ModelPolicyInputre-export as pre-existing upstream drift. It wasn't — I checkedorigin/masterand the type was defined atprovider.ts:153, so the removal was its own, correctly done, and misattributed afterward. Had I forwarded that as filed, this PR would have opened with a phantom bug report against your tree.I mention it because the same habit is why the fixture claim above is stated as a diff rather than a summary.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Retires scheduled-task fields from the manifest across Rust and TypeScript so the daemon no longer exposes unused scheduler vocabulary. Previously,
ModuleManifestrequiredscheduled_tasks; now it omits it. The HELLO payload no longer includesscheduled_tasks. Parsing remains backward/forward compatible because the protocol does notdeny_unknown_fields.scheduled_tasksand supporting types fromsubc-protocol; updates call sites insubc-client-rs,subc-core, andsubc-mcp.@cortexkit/subc-client; Swift client is unaffected.module_hello_body.json;subc-controlgoldens are unchanged.control.rs’smanifest()test helper; tests remain valid.Migration
scheduled_tasksfrom manifests.@cortexkit/subc-clientand removescheduled_tasksfrom manifests; this is a breaking type change.Written for commit f4708fb. Summary will update on new commits.
Deploy ordering (release note)
The daemon must be deployed before any module rebuilt against the new manifest. The old
scheduled_tasksfield carries no#[serde(default)], so an old daemon refuses a new module's HELLO withmissing field scheduled_tasks; a new daemon accepts old modules unchanged. Producer-narrowing, andck daemon'sbuild_git_shamakes the ordering checkable rather than ritual.