chore: tighten types and introduce TimeoutToken protocol#24
Merged
Conversation
Typing cleanup across activity, event-store, workflow paths. Uncovered a cluster of compiler bugs (epic BT-2024, issues BT-2016..BT-2023) that prevent full type coverage; work around them with @expect type at the documented suppression sites. - Introduce TimeoutToken protocol + NullTimer to replace the nil-guard cascade around Timer in ActivityWorkerPool - Extract retriesExhausted:config: helper in ActivityRetryExecutor using the isNil early-return narrowing idiom - Add return-type annotations on ActivityRetryExecutor execute:/tryAttempt: - Correct WorkflowContext>>runActivity:method:with:options: return type from Dictionary to Object (activity results are arbitrary) - Suppress block-param Dynamic warnings in event_store reloadFromDisk and two test files pending fixes upstream in beamtalk
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens and adds type annotations across actors and public APIs, introduces a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Pool as ActivityWorkerPool
participant Sup as ActivityWorkerSupervisor
participant Worker as ActivityWorker
rect rgba(0,150,136,0.5)
participant Timer as TimeoutToken (Timer / NullTimer)
end
Caller->>+Pool: executeActivity(activityClass, ...)
Pool->>+Sup: startChild(...)
Sup-->>-Pool: startResult (ok / isError)
alt startResult isError
Pool-->>Caller: ActivityOutcome rejected
else started
Pool->>Timer: killTimerFor: timeout
Pool->>+Worker: dispatch work
Worker-->>-Pool: result / raises
Pool->>Timer: timer cancel
alt timer isActive not
Pool-->>Caller: ActivityOutcome timedOut
else result returned
Pool-->>Caller: ActivityOutcome completed (result)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/null_timer.bt (1)
13-15: Add method-level doc comments forcancelandisActive.These two methods define the core semantics of
NullTimer; adding///docs (with tiny examples) will make the contract easier to maintain.As per coding guidelines, "Always add doc comments (
///) on all classes and meaningful methods, with examples in Beamtalk code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/null_timer.bt` around lines 13 - 15, Add triple-slash doc comments for the NullTimer methods `cancel` and `isActive`: for each method (`cancel -> Boolean` and `isActive -> Boolean`) add a /// summary that explains its contract, include a tiny Beamtalk example showing typical usage and expected return, and mention that `NullTimer` is a no-op timer (e.g., `cancel` always returns false, `isActive` always true). Place these docs immediately above the method definitions so tooling picks them up and follow existing doc style in the repository.src/activity_worker_pool.bt (1)
100-100: Add the specific BT issue ID to this@expect typesuppression.Please include the concrete child issue (e.g., BT-2016..BT-2023) in the inline comment so this workaround is auditable and easier to remove later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activity_worker_pool.bt` at line 100, The inline suppression "@expect type" in src/activity_worker_pool.bt lacks a specific BT issue reference; update that annotation to include the concrete child issue ID (for example "BT-2016" or a range like "BT-2016..BT-2023") in the same comment so the workaround is auditable and removable later—locate the "@expect type" token and change the inline comment to read something like "@expect type // BT-XXXX" referencing the exact BT issue number.src/workflow_engine.bt (1)
257-257: Consider documenting the new returned value explicitly in method docs.Now that these methods return
List(ExduraEvent), a one-line note in each doc comment about the returned list would make the contract clearer for callers.Also applies to: 268-268, 686-686, 769-769
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflow_engine.bt` at line 257, Update the doc comment for recordActivityFailed to explicitly state that it returns a List(ExduraEvent) (one concise sentence describing the returned list semantics), and make the same one-line addition to the doc comments of the other affected functions that were changed to return List(ExduraEvent) so callers understand the contract; locate the signature recordActivityFailed: reason :: Object workflowId: workflowId :: String -> List(ExduraEvent) => and add the return-note there, then mirror that wording in the other updated function docs.src/activity_retry_executor.bt (1)
91-93: Add a usage example to the new helper doc comment.The new meaningful method has doc text but no example snippet. Please include a small Beamtalk example for consistency with repository doc standards.
As per coding guidelines, "Always add doc comments (
///) on all classes and meaningful methods, with examples in Beamtalk code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activity_retry_executor.bt` around lines 91 - 93, Add a short Beamtalk usage example to the doc comment for the retriesExhausted helper: keep the existing triple-slash comments (///) and append a small example showing how to call retriesExhausted with a sample attempt integer and a sample ActivityRetryConfig (e.g., maxAttempts set) that demonstrates both true and false outcomes; reference the symbol names attempt, config, ActivityRetryConfig and the class name retriesExhausted so reviewers can find and validate the snippet quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/activity_retry_executor.bt`:
- Line 70: In the class-side method tryAttempt: replace the erroneous send to
the metaclass by removing the extra "class" receiver; change the message send
from "self class retriesExhausted: attempt config: config" to call
retriesExhausted: on the current receiver (self) so the call becomes "self
retriesExhausted: attempt config: config", ensuring the method
retriesExhausted:attempt:config: on the class itself is invoked correctly.
In `@src/activity_worker_pool.bt`:
- Around line 83-87: The timeout vs crash classification is wrong because you
call timer cancel before checking timer isActive; capture the timer's active
state into a local variable (e.g., wasActive := timer isActive) immediately
before canceling, then call timer cancel, decrement self.activeCount, and use
the captured wasActive to decide the branch that previously used timer isActive
not; update the exception handler around worker execute to reference the
captured flag and preserve correct handling of NullTimer and non-nil timers.
In `@src/event_store.bt`:
- Around line 276-277: Remove the deprecated "@expect type" directive on the
events handling line and instead annotate the involved variables and types: make
the events variable a List parameterized with ExduraEvent (e.g.,
List(ExduraEvent)), annotate the eventsTable and any state that holds lists
accordingly, and add explicit return type annotations (using :: syntax) on the
surrounding method/function so the recursive call infers types; where a value
can be nil use a union type like Integer | Nil for wfId or other vars to narrow
types. Update the line that reads events isEmpty ifFalse: [eventsTable at: wfId
put: events] to remove the directive and rely on the newly added
List(ExduraEvent) and return-type annotations so the compiler no longer needs
`@expect` type.
In `@src/workflow_runner.bt`:
- Around line 86-87: The dynamic send to registerQueries: on self.workflow must
be guarded with a respondsTo: check (and similarly guard run:ctx:) to avoid a
does_not_understand when workflow is misconfigured; update the code around the
call site that currently does "self.workflow registerQueries: ctx" to first
verify "self.workflow respondsTo: `#registerQueries`:" and raise/return a clear
error (fail fast) if not, and likewise check for `#run`:ctx: before invoking
run:ctx: so the workflow class misconfiguration surfaces with a descriptive
error message instead of a does_not_understand exception.
- Line 16: The WorkflowRunner actor is declared untyped while callers expect a
typed WorkflowRunner; update the actor declaration for WorkflowRunner to include
the typed keyword (i.e., change "Actor subclass: WorkflowRunner" to a typed
actor declaration) so its definition matches uses like the typed return
annotations in src/workflow_engine.bt (references: WorkflowRunner in method
return types). If the untyped declaration was intentional, instead add a TODO
comment on the WorkflowRunner declaration noting the limitation and include a
tracker ID so callers know why it remains untyped.
---
Nitpick comments:
In `@src/activity_retry_executor.bt`:
- Around line 91-93: Add a short Beamtalk usage example to the doc comment for
the retriesExhausted helper: keep the existing triple-slash comments (///) and
append a small example showing how to call retriesExhausted with a sample
attempt integer and a sample ActivityRetryConfig (e.g., maxAttempts set) that
demonstrates both true and false outcomes; reference the symbol names attempt,
config, ActivityRetryConfig and the class name retriesExhausted so reviewers can
find and validate the snippet quickly.
In `@src/activity_worker_pool.bt`:
- Line 100: The inline suppression "@expect type" in src/activity_worker_pool.bt
lacks a specific BT issue reference; update that annotation to include the
concrete child issue ID (for example "BT-2016" or a range like
"BT-2016..BT-2023") in the same comment so the workaround is auditable and
removable later—locate the "@expect type" token and change the inline comment to
read something like "@expect type // BT-XXXX" referencing the exact BT issue
number.
In `@src/null_timer.bt`:
- Around line 13-15: Add triple-slash doc comments for the NullTimer methods
`cancel` and `isActive`: for each method (`cancel -> Boolean` and `isActive ->
Boolean`) add a /// summary that explains its contract, include a tiny Beamtalk
example showing typical usage and expected return, and mention that `NullTimer`
is a no-op timer (e.g., `cancel` always returns false, `isActive` always true).
Place these docs immediately above the method definitions so tooling picks them
up and follow existing doc style in the repository.
In `@src/workflow_engine.bt`:
- Line 257: Update the doc comment for recordActivityFailed to explicitly state
that it returns a List(ExduraEvent) (one concise sentence describing the
returned list semantics), and make the same one-line addition to the doc
comments of the other affected functions that were changed to return
List(ExduraEvent) so callers understand the contract; locate the signature
recordActivityFailed: reason :: Object workflowId: workflowId :: String ->
List(ExduraEvent) => and add the return-note there, then mirror that wording in
the other updated function docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 214a19e3-ed34-4850-b199-8488d01d56af
📒 Files selected for processing (10)
src/activity_retry_executor.btsrc/activity_worker_pool.btsrc/event_store.btsrc/null_timer.btsrc/timeout_token.btsrc/workflow_context.btsrc/workflow_engine.btsrc/workflow_runner.bttest/event_store_persistence_test.bttest/workflow_options_test.bt
Adapt to the Phase 1 stdlib migration (BT-1997 / ADR 0080) where `supervise`, `startChild`, `stop`, and `which:` now return Result. Startup paths use `valueOrDo: [:e | self error: ...]` for useful messages; the activity worker hot path checks `isError` out-of-line to preserve the active-count decrement (Actor state can't be mutated inside a closure). Tests use bare `unwrap` for loud failures. Also extract `StoredSnapshot fromBinary:` to encapsulate the `Binary deserialize:` cast at one typed boundary, removing the DNU warning on `snapshot eventId` in `readEventsFromSnapshot:`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves 35 of 39 lint issues (remaining 4 are BT-2024 Dynamic-inference warnings pending a stdlib Phase 2 migration of Supervisor>>which:). - Switch `:: Class` annotations to the new `Workflow class` / `Activity class` metaclass type across WorkflowEngine, WorkflowContext, WorkflowRunner, ExduraClient, ActivityRetryExecutor, ActivityWorker, ActivityWorkerPool (eliminates 9 spurious "Class, got Class" warnings) - Remove 2 stale `@expect type` directives in activity_worker_pool.bt and event_store.bt - Narrow `config maximumInterval` via `ifNil:ifNotNil:` block form in ActivityRetryExecutor (Integer | Nil keyword-arg narrowing) - Type-annotate Integer locals in timer_manager.bt and workflow_context.bt where `at:ifAbsent:` results were inferred as Object - Suppress 4 String|Nil narrowing sites with documented `@expect type` (3× appendEvents in WorkflowEngine, 1× supervisor stop in ExduraClient) - Type test-helper `awaitWorkflow:timeout:` and `awaitChildStarted:timeout:` with explicit return types, and terminate with `self error: "unreachable"` so typed returns validate against the `[true] whileTrue:` loop Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/activity_worker_pool.bt (1)
94-99:⚠️ Potential issue | 🔴 CriticalTimeout/crash classification is still order-dependent and can misclassify failures.
At
Line 94,timer cancelruns before theLine 98timer isActive notcheck. If cancel deactivates the timer, crash paths get mislabeled as timeout. Capture timeout state before canceling.Proposed fix
- timer cancel - self.activeCount := self.activeCount - 1 - // Distinguish timeout from crash: an inactive timer has fired, - // meaning the worker was killed by timeout. NullTimer stays active. - timer isActive not ifTrue: [ + timedOut := timer isActive not + timer cancel + self.activeCount := self.activeCount - 1 + // Distinguish timeout from crash using pre-cancel state. + timedOut ifTrue: [ ^ActivityOutcome timedOut ] ifFalse: [^ActivityOutcome failed: e message]For the Beamtalk/Erlang timer model used by `Timer after: ms do: [...]`, after calling `cancel` on an active timer, should `isActive` (or equivalent `read_timer`) immediately report inactive?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activity_worker_pool.bt` around lines 94 - 99, The timeout vs crash detection is order-dependent because you call timer cancel before checking timer isActive; change the logic in the block containing timer cancel, timer isActive, self.activeCount and the ActivityOutcome timedOut return so that you first read/store the timer's active state (e.g., local variable timedOutFlag := timer isActive not) before calling timer cancel and decrementing self.activeCount, then use that stored flag to decide whether to return ActivityOutcome timedOut or treat it as a crash; update all references in that code path (the timer cancel + timer isActive check and the ActivityOutcome timedOut return) to use the stored flag.
🧹 Nitpick comments (1)
src/exdura_client.bt (1)
134-136: Align the destroy-branch comment with actual behavior.The comment says files are cleaned after supervisor stop, but
Line 136returns immediately. Either perform cleanup in this branch or update the comment to match the real flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exdura_client.bt` around lines 134 - 136, The comment for the "destroy" branch is incorrect because after calling self.supervisor stop valueOrDo: [:e | nil] the method returns ^nil immediately and no cleanup runs; either invoke the cleanup routine here after a successful supervisor stop (e.g., call the existing cleanup/remove-files method or perform file deletion after valueOrDo returns) — referencing the call site self.supervisor stop valueOrDo: [:e | nil] and the destroy branch — or update the comment above this branch to state that files are cleaned up elsewhere (and point to the actual cleanup function) so behavior and comment match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/activity_worker_pool.bt`:
- Around line 94-99: The timeout vs crash detection is order-dependent because
you call timer cancel before checking timer isActive; change the logic in the
block containing timer cancel, timer isActive, self.activeCount and the
ActivityOutcome timedOut return so that you first read/store the timer's active
state (e.g., local variable timedOutFlag := timer isActive not) before calling
timer cancel and decrementing self.activeCount, then use that stored flag to
decide whether to return ActivityOutcome timedOut or treat it as a crash; update
all references in that code path (the timer cancel + timer isActive check and
the ActivityOutcome timedOut return) to use the stored flag.
---
Nitpick comments:
In `@src/exdura_client.bt`:
- Around line 134-136: The comment for the "destroy" branch is incorrect because
after calling self.supervisor stop valueOrDo: [:e | nil] the method returns ^nil
immediately and no cleanup runs; either invoke the cleanup routine here after a
successful supervisor stop (e.g., call the existing cleanup/remove-files method
or perform file deletion after valueOrDo returns) — referencing the call site
self.supervisor stop valueOrDo: [:e | nil] and the destroy branch — or update
the comment above this branch to state that files are cleaned up elsewhere (and
point to the actual cleanup function) so behavior and comment match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1108b523-1c5c-4a5f-9816-b8a75fb48cdf
📒 Files selected for processing (18)
src/activity_retry_executor.btsrc/activity_worker.btsrc/activity_worker_pool.btsrc/event_store.btsrc/exdura_client.btsrc/exdura_worker.btsrc/stored_snapshot.btsrc/timer_manager.btsrc/workflow_context.btsrc/workflow_engine.btsrc/workflow_runner.bttest/activity_worker_pool_test.bttest/cancellation_test.bttest/child_workflow_test.bttest/event_store_persistence_test.bttest/exdura_client_test.bttest/exdura_supervisor_test.bttest/signal_test.bt
✅ Files skipped from review due to trivial changes (2)
- test/signal_test.bt
- src/activity_worker.bt
🚧 Files skipped from review as they are similar to previous changes (3)
- src/workflow_runner.bt
- src/activity_retry_executor.bt
- src/workflow_engine.bt
BT-1947 removed `| Nil = nil` defaults on state fields assumed to be
initialized via an `initialize` selector, but several actors use
`initWith*:` (not `initialize`) and are supervisor-spawned. Bare `spawn`
on these typed Actors raised instantiation_error → 125/212 tests failed
with "Cannot call 'spawn' on WorkflowEngine / WorkflowContext / ...".
- WorkflowEngine state: eventStore — restore `| Nil = nil` so the
ExduraSupervisor `supervisionSpec` spawn path succeeds; `withStore:`
class methods switch to `spawnWith:` for one-shot initialization
- ActivityWorker state: activityClass/selector — restore `| Nil = nil`;
supervisor's startChild creates the worker with no state before
`initWith:selector:args:` fills it in
- TimerManager state: engine/eventStore — restore `| Nil = nil`; same
supervisor-startup pattern
- ActivityRetryExecutor: fix `self class retriesExhausted:config:` to
`self retriesExhausted:config:` — `tryAttempt:` is already a class
method, so `self class` was the metaclass
- ExduraWorker: drop `valueOrDo:` wrapping on `sup which:` — `which:`
returns the child directly or nil (pre-ADR-0080-Phase-2), never a
Result; the wrapper DNU'd at runtime
- WorkflowRunner: `WorkflowContext spawn` → `spawnWith: #{#engine ⇒ ...}`
- Test files: same treatment for bare `WorkflowContext spawn` and
`TimerManager spawn` in signal_test, query_test, workflow_options_test,
signal_buffering_test, durable_timer_test
All 212 tests pass (25s, down from 10+ min of 125 cascading timeouts).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/workflow_runner.bt (1)
16-17:⚠️ Potential issue | 🔴 CriticalRestore
typedonWorkflowRunnerdeclaration to avoid parser breakage.Line 16 changed to untyped actor declaration, while Line 17+ keeps typed
statedeclarations; this matches the current CI warning thatsrc/workflow_runner.bthas parse errors in fmt-check.Proposed fix
-Actor subclass: WorkflowRunner +typed Actor subclass: WorkflowRunner state: workflow :: Workflow classUse this read-only check to confirm the mismatch and formatter behavior:
#!/bin/bash set -euo pipefail rg -n --type-add 'bt:*.bt' --type=bt '^(typed\s+)?Actor subclass: WorkflowRunner|^\s*state:\s+workflow\s+::' src/workflow_runner.bt -C2 if command -v beamtalk >/dev/null 2>&1; then beamtalk fmt-check src/workflow_runner.bt fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflow_runner.bt` around lines 16 - 17, The actor declaration for WorkflowRunner lost the leading keyword "typed", causing a mismatch with the subsequent typed state lines (e.g., "state: workflow :: Workflow") and breaking the parser; restore the "typed" modifier on the WorkflowRunner declaration (the line that currently reads "Actor subclass: WorkflowRunner") so it reads as a typed actor declaration, ensuring the "WorkflowRunner" actor header matches its typed state entries and passes beamtalk fmt-check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/activity_retry_executor.bt`:
- Around line 91-93: The doc comment for the helper class
retriesExhausted:attempt:config: is missing a Beamtalk example; update the
triple-slash (///) doc block above class retriesExhausted:attempt:config: to
include a short Beamtalk snippet that demonstrates calling retriesExhausted with
an ActivityRetryConfig (showing both a finite maxAttempts case where it returns
true when attempt >= maxAttempts and a nil/0 case returning false), and document
the expected boolean results so readers can see concrete usage and edge cases.
- Line 14: The doc comment for the method retriesExhausted:config: is missing a
usage example; update the comment above the method named
retriesExhausted:config: in class ActivityRetryExecutor (or symbol
ActivityRetryExecutor::retriesExhausted:config:) to include a one-line example
showing expected use and return, e.g. add a line like "///
(ActivityRetryExecutor retriesExhausted: 5 config: myConfig) // => true or
false" immediately beneath the description so the documentation includes a
concrete usage example.
---
Duplicate comments:
In `@src/workflow_runner.bt`:
- Around line 16-17: The actor declaration for WorkflowRunner lost the leading
keyword "typed", causing a mismatch with the subsequent typed state lines (e.g.,
"state: workflow :: Workflow") and breaking the parser; restore the "typed"
modifier on the WorkflowRunner declaration (the line that currently reads "Actor
subclass: WorkflowRunner") so it reads as a typed actor declaration, ensuring
the "WorkflowRunner" actor header matches its typed state entries and passes
beamtalk fmt-check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5edf0fe5-fedb-4899-b409-c53e0065875b
📒 Files selected for processing (11)
src/activity_retry_executor.btsrc/activity_worker.btsrc/exdura_worker.btsrc/timer_manager.btsrc/workflow_engine.btsrc/workflow_runner.bttest/durable_timer_test.bttest/query_test.bttest/signal_buffering_test.bttest/signal_test.bttest/workflow_options_test.bt
✅ Files skipped from review due to trivial changes (1)
- test/workflow_options_test.bt
🚧 Files skipped from review as they are similar to previous changes (5)
- src/activity_worker.bt
- src/timer_manager.bt
- test/signal_test.bt
- src/exdura_worker.bt
- src/workflow_engine.bt
Follow-up to the spawn-regression fix: re-adding `| Nil = nil` on WorkflowEngine.eventStore surfaces two legitimate "EventStore | UndefinedObject" warnings at sites that pass self.eventStore to methods expecting a plain EventStore (ActivityRetryExecutor execute:, ChildWorkflowHandle for:engine:eventStore:). The supervisor's initialize: guarantees these are non-nil by the time any message is processed — annotate both sites with @expect type. Also removes 3 stale @expect type directives at the appendEvents: sites; the narrowing issue they were papering over is now handled by the compiler, and the leftover annotations produced stale-@expect warnings themselves. Lint: 6 → 1 (remaining warning is the pre-existing BT-2024 Dynamic block-param in activity_worker_pool.bt, pending upstream stdlib Block(T, R) parameterisation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/workflow_engine.bt (1)
17-17: Type declaration aligns with supervisor initialization pattern.Making
eventStoreoptional at the type level correctly reflects the spawn-then-initialize lifecycle, whereExduraSupervisor.initialize:guaranteesinitWithStore:pool:is called before any workflow messages arrive.Note: The accessor at line 875 (
eventStore -> EventStore) now has a type mismatch since it returns non-optionalEventStorewhile the state isEventStore | Nil. Consider adding@expect typethere for consistency with lines 210 and 662, or updating the return type toEventStore | Nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflow_engine.bt` at line 17, The state declaration changed to make eventStore optional (state: eventStore :: EventStore | Nil = nil) but the accessor (eventStore -> EventStore) still claims a non-optional return type; update the accessor to match the state by either adding an `@expect` type annotation to the accessor (e.g., `@expect` type EventStore | Nil) or change its signature to return EventStore | Nil so types are consistent with the spawn-then-initialize pattern enforced by ExduraSupervisor.initialize:initWithStore:pool:, and ensure any callers handle the Nil case accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/workflow_engine.bt`:
- Line 17: The state declaration changed to make eventStore optional (state:
eventStore :: EventStore | Nil = nil) but the accessor (eventStore ->
EventStore) still claims a non-optional return type; update the accessor to
match the state by either adding an `@expect` type annotation to the accessor
(e.g., `@expect` type EventStore | Nil) or change its signature to return
EventStore | Nil so types are consistent with the spawn-then-initialize pattern
enforced by ExduraSupervisor.initialize:initWithStore:pool:, and ensure any
callers handle the Nil case accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 954db373-414a-492e-b7f8-010dba6fc5b1
📒 Files selected for processing (1)
src/workflow_engine.bt
ExduraSupervisor class initialize:, ExduraWorker start:, and the supervisor test updated to the valueOrDo:/isOk pattern now that Supervisor>>which: returns Result(Object, Error) (ADR 0080 Phase 2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- activity_worker_pool: capture timer.isActive before calling cancel; the old order always classified crashes under a non-nil timeout as #timedOut because cancel deactivates the timer. Added regression test testPoolCrashWithTimeoutClassifiesAsFailed. - workflow_runner: mark WorkflowRunner as `typed Actor subclass` to match the `-> WorkflowRunner` return annotations used by callers in workflow_engine.bt. - activity_retry_executor: add concrete Beamtalk example to retriesExhausted:config: doc comment, covering the finite and 0/nil (unlimited) cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comment read "narrowing doesn't propagate" but nothing narrows — Supervisor>>which: is typed as -> Result(Object, Error) regardless of the class passed in, so the Object type is the signature, not a failed narrowing. Clarify that the caller is the one asserting the concrete class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Instead of scattering @expect type/dnu at every downstream method send, annotate the locals with their known concrete class (e.g. `store :: EventStore := (sup which: EventStore) valueOrDo: [...]`) and suppress the single Object→ConcreteClass mismatch at the binding. Downstream sends then type-check cleanly without further annotations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Typed local syntax (`store :: EventStore := (sup which: EventStore) valueOrDo: ...`) accepts the Object-typed ok-value without a type mismatch, so the preceding @expect type directives are stale. Removes 7 stale-@expect lint warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ings Three residual Dynamic warnings suppressed with @expect type, all tracked under epic BT-2044: - activity_worker_pool.bt: `e message` inside `on: Exception do: [:e | ...]` — BT-2045 (infer block-param type from `on: E do:` first arg) - workflow_runner.bt: outer `self.snapshot ifNil: [..] ifNotNil: [:snap | ..]` statement — BT-2047 (branch-union return type) - workflow_runner.bt: `ctx initWithSnapshot: snap` inside the `ifNotNil:` block — BT-2046 (narrow block param to non-Nil branch) With these, lint is clean and all 213 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cross-cutting typing cleanup in exdura. Adds annotations, extracts helpers to enable existing narrowing idioms, introduces a
TimeoutTokenprotocol +NullTimerto replace theTimer | Nilcascade inActivityWorkerPool.During this work, uncovered a cluster of compiler bugs in the beamtalk type checker that prevent full type coverage — filed as epic BT-2024 (child issues BT-2016..BT-2023).
@expect typesuppressions in this PR mark the workaround sites; they should disappear as the upstream fixes land.Key changes
Timerand "no timeout" share a uniform interface.timer ifNotNil:guards with unconditional calls via the protocol; extractkillTimerFor:timeout:helper using the documentedisNil ifTrue: [^...]narrowing idiom.retriesExhausted:config:helper, add-> Objecttoexecute:andtryAttempt:config:.runActivity:method:with:options:fromDictionary(Symbol, Object)toObject(activity results are arbitrary values, not always dictionaries).@expect typeat sites blocked by upstream compiler bugs.Test plan
@expect typesuppressions against the epic — each should map to a BT-2016..BT-2023 child issueRelated
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style