Conversation
New() returns Result, accepts CoreOption functionals. Restores v0.3.3 service registration contract: - WithService(factory func(*Core) Result) — service factory receives Core - WithOptions(Options) — key-value configuration - WithServiceLock() — immutable after construction Services registered in New() form the application conclave with shared IPC access. Each Core instance has its own bus scope. Co-Authored-By: Virgil <virgil@lethean.io>
…. Check: 1) API contract ...' (#29) from agent/review-pr--28--read-claude-md-first--che into dev
- WithOptions copies the Options slice (constructor isolation regression) - WithService auto-discovers service name from package path via reflect - WithService auto-registers HandleIPCEvents if present (v0.3.3 parity) - Add test for failing option short-circuit in New() Co-Authored-By: Virgil <virgil@lethean.io>
- HandleIPCEvents only auto-registered for services the factory didn't
register itself (prevents double handler registration)
- Auto-discovery only creates Service{} placeholder when factory didn't
call c.Service() — factories that register themselves keep full lifecycle
Addresses Codex review findings 1 and 2 from third pass.
Co-Authored-By: Virgil <virgil@lethean.io>
WithService is now a simple factory call — no reflect, no auto-registration. New() calls discoverHandlers() after all opts run, scanning Config for service instances that implement HandleIPCEvents. This eliminates both double-registration and empty-placeholder issues: - Factories wire their own lifecycle via c.Service() - HandleIPCEvents discovered once, after all services are registered - No tension between factory-registered and auto-discovered paths Co-Authored-By: Virgil <virgil@lethean.io>
Restores v0.3.3 service manager capabilities: - RegisterService(name, instance) stores the raw instance - Auto-discovers Startable/Stoppable interfaces → wires lifecycle - Auto-discovers HandleIPCEvents → wires to IPC bus - ServiceFor[T](c, name) for typed instance retrieval - Service DTO gains Instance field for instance tracking WithService is a simple factory call — no reflect, no magic. discoverHandlers removed — RegisterService handles it inline. No double-registration: IPC wired once at registration time. Co-Authored-By: Virgil <virgil@lethean.io>
Options is now a proper struct with New(), Set(), Get(), typed accessors.
Result gains New(), Result(), Get() methods on the struct.
WithOption("key", value) convenience for core.New().
options_test.go: 22 tests passing against the new contract.
Other test files mechanically updated for compilation.
Co-Authored-By: Virgil <virgil@lethean.io>
App.New() creates from Options. App.Find() locates programs on PATH. Both are struct methods — no package-level functions. 8 tests passing. Co-Authored-By: Virgil <virgil@lethean.io>
Cli struct unchanged — already conforms. Tests use WithOption() convenience. 9 tests passing. Co-Authored-By: Virgil <virgil@lethean.io>
Cli{}.New(c) replaces &Cli{core: c} in contract.go.
9 tests passing.
Co-Authored-By: Virgil <virgil@lethean.io>
Cli as service with ServiceRuntime, incomplete. Need to properly port v0.3.3 service_manager, message_bus, WithService with full name/IPC discovery. Co-Authored-By: Virgil <virgil@lethean.io>
…tration
- WithService now calls factory, discovers service name from package path via
reflect/runtime (last path segment, _test suffix stripped, lowercased), and
calls RegisterService — which handles Startable/Stoppable/HandleIPCEvents
- If factory returns nil Value (self-registered), WithService returns OK without
a second registration
- Add contract_test.go with _Good/_Bad tests covering all three code paths
- Fix core.go Cli() accessor: use ServiceFor[*Cli](c, "cli") (was cli.New())
- Fix pre-existing })) → }}) syntax errors in command_test, service_test, lock_test
- Fix pre-existing Options{...} → NewOptions(...) in core_test, data_test,
drive_test, i18n_test (Options is a struct, not a slice)
Co-Authored-By: Virgil <virgil@lethean.io>
…tration WithService now: calls factory, discovers service name from instance's package path via reflect.TypeOf, discovers HandleIPCEvents method, calls RegisterService. If factory returns nil Value, assumes self-registered. Also fixes: Cli() accessor uses ServiceFor, test files updated for Options struct. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
…arity Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
WithService: full name discovery + IPC handler auto-registration via reflect WithName: explicit service naming RegisterService: Startable/Stoppable/HandleIPCEvents auto-discovery MustServiceFor[T]: panics if not found WithServiceLock: enable/apply split (v0.3.3 parity) Cli: registered as service via CliRegister, accessed via ServiceFor @todo Codex: Fix data_test.go and embed_test.go — embed path resolution after Options changed from []Option to struct. Mount paths need updating. Co-Authored-By: Virgil <virgil@lethean.io>
…ixes
Root cause: Result.New didn't mark single-value results as OK=true,
breaking Mount/ReadDir/fs helpers that used Result{}.New(value, err).
Also: data_test.go and embed_test.go updated for Options struct,
doc comments updated across data.go, drive.go, command.go, contract.go.
All tests green. Coverage 82.2%.
Co-Authored-By: Virgil <virgil@lethean.io>
Config.New() initialises ConfigOptions. Fs.New(root) sets sandbox root. ErrorLog uses Default() fallback — no explicit init needed. contract.go uses constructors instead of struct literals. All tests green. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
…' (#28) from feat/service-options into dev
- Run() uses context.Background() for shutdown (c.context is cancelled) - Stoppable closure uses context.Background() for OnShutdown - WithService delegates HandleIPCEvents to RegisterService only Fixes Codex review findings 1, 2, 3. Co-Authored-By: Virgil <virgil@lethean.io>
…(#36) from fix/codex-review-findings into dev
Tests: Run, RegisterService, ServiceFor, MustServiceFor _Bad/_Ugly variants. Fix: Lock map is now per-Core instance, not package-level global. This prevents deadlocks when multiple Core instances exist (e.g. tests). Coverage: 82.4% → 83.6% Co-Authored-By: Virgil <virgil@lethean.io>
… (#37) from feat/test-coverage into dev
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR refactors the core framework with a functional options pattern, service-oriented architecture, and improved resource isolation. Changes include restructuring the Changes
Comment |
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
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 by CodeRabbit
Release Notes
New Features
Refactor