PR A: Close the sealed Command hierarchy honestly — sealed module Command, abstract BuiltinCommand open edge, delete the sorbet-runtime ivar pokes, un-private CommandRepository

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.

[d3mlabs-ai-flow]

Implements #117.

Requested by @JPDuchesne.

Closes#117

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[JPDuchesne]

Reworked per review discussion: the BuiltinBody interface and the final BuiltinCommand wrapper are gone.

• Command is now a sealed! abstract!module with the trait defaults. A sealed module's runtime included hook fires only for direct includers — include never transfers singleton methods — so the seal holds with no sorbet-runtime ivar pokes and nothing to suppress.
• BuiltinCommand is an abstract! class that includes Command beside it: the hierarchy's one declared open edge. Builtins subclass it directly again (class CacheCommand < BuiltinCommand), and the composition root wires them straight in — no wrapping.
• It stays a class rather than a module because Sorbet flattens module mixins: a module open edge would make every builtin a direct includer of sealed Command and fail the same-file check statically. A superclass edge is not flattened, so it typechecks and is invisible to the seal's runtime hooks.
• ProjectCommand and OverriddenCommand include Command and are final!, so descent is closed everywhere it isn't explicitly declared. CommandExecutor's exhaustive case + T.absurd is unchanged.
• Tests now assert the real closure properties: direct include Dev::Command raises, subclassing either data leaf raises, and subclassing BuiltinCommand is the declared open edge.

srb tc clean, 887 tests / 0 failures, rubocop clean.

[JPDuchesne]

/ask what's with this line change?

[d3mlabs-ai-flow]

✅ /ask

The removed super() was a no-op, so the diff is dead-code cleanup in the test fake. ExecutorFakeBuiltin subclasses Dev::BuiltinCommand, and that base class (see src/dev/command.rb) defines no initialize of its own — it's an abstract class that only declares the abstract call method, and Command is a plain module with no constructor either. So super() fell through to Object#initialize, which takes no arguments and does nothing; the line initialized nothing and its presence wrongly implied the base class had constructor behavior to run. Removing it also matches how the real builtins under src/dev/builtins/ are written — e.g. CdCommand and CredCommand define initialize without calling super. Behavior of the tests in test/dev/command_executor_test.rb is unchanged. (One caveat: git history access is restricted in this checkout, so I can't confirm whether the line was a leftover from an earlier BuiltinCommand shape — the file's comments mention it previously carried sorbet-runtime workarounds — but as the code stands today the call is provably inert.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Resolved in b8ed631: the super() calls are restored — and the convention is now applied uniformly. The bot's replies were right that the calls are inert today, but that misses the point of the rule: an explicit super() means future base-class constructor state either runs automatically (defaulted params) or fails loudly at first instantiation (required params), instead of silently constructing half-initialized objects. The fakes had actually been the only callers — the 11 real builtins never called super() — so the commit adds it everywhere an initializer derives from the hierarchy: both data leaves, all builtins, and the fakes.

[JPDuchesne]

/ask Same here, why not keep super?

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy (the real builtins had never called super() — the fakes were the outliers in the other direction). See the thread on command_executor_test.rb for the full rationale.

[JPDuchesne]

/ask Same here, why not keep super?

[d3mlabs-ai-flow]

✅ /ask

Because there is nothing for super() to reach: it's a pure no-op here. ServiceFakeBuiltin subclasses Dev::BuiltinCommand (src/dev/command.rb), and neither BuiltinCommand nor the Command module it includes defines an initialize — there is no parent state to set up, so super() falls through to Object#initialize, which does nothing. It isn't needed for the abstract guard either: sorbet-runtime's abstract! enforces non-instantiability by wrapping self.new on the abstract class, not initialize, so the guard runs regardless of whether the subclass calls super. Lint doesn't ask for it either — Lint/MissingSuper is disabled in the inherited rubocop-shopify config. Keeping it would be harmless but would imply the base class has initialization the fake must run, which it doesn't; dropping the dead call is the same reasoning as in the other thread. (If BuiltinCommand ever grows a real initializer, its subclasses would need explicit constructor wiring anyway, per the repo's constructor-injection convention.)

⚙️ workflow run · model: claude-fable-5-high

[JPDuchesne]

Restored in b8ed631, along with every other initializer in the hierarchy. The bot is right that abstract! guards new rather than initialize and that Lint/MissingSuper is disabled in rubocop-shopify — but "provably inert today" is exactly the case the always-super convention exists for. See the thread on command_executor_test.rb for the full rationale.