From 0b9834a90b20a90908acc8f698742c7218450008 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Sun, 23 Aug 2026 19:54:08 +0900 Subject: docs(plugin): bring boundary text in line with the implementation Comments that pointed at design notes kept outside the repository are dead ends for anyone reading only the tree, so what each of them explained now lives in a tagged TODO at the site it applies to. Several of those sites also stated something the implementation does not do, and the TODOs record the actual gap instead: the two halves of the codec recognize handle descriptors by different rules, the scripts Command path drops the exception class and collects output in a BufferedOutput that cannot carry an interactive command, find_shortest_path panics where PHP throws, and the package dispatch hand-rolls the variant selection AnyPackage should own. The classifier document likewise described rust-snapshot, plugin-constructible and several of the open questions as designed rather than as built. Co-Authored-By: Claude Opus 5 (1M context) --- crates/shirabe-php-rpc/php/worker.php | 8 +- crates/shirabe-php-rpc/src/value.rs | 7 ++ .../src/event_dispatcher/event_dispatcher.rs | 22 +++- crates/shirabe/src/plugin/php_plugin_proxy.rs | 29 +++-- crates/shirabe/src/util/filesystem.rs | 10 +- docs/dev/plugin-class-classification.md | 118 +++++++++++++++------ 6 files changed, 145 insertions(+), 49 deletions(-) diff --git a/crates/shirabe-php-rpc/php/worker.php b/crates/shirabe-php-rpc/php/worker.php index 164d73ea..6ed5df40 100644 --- a/crates/shirabe-php-rpc/php/worker.php +++ b/crates/shirabe-php-rpc/php/worker.php @@ -267,6 +267,12 @@ final class ShirabeRpcRuntime /** * Converts a decoded wire value: handle descriptor arrays become live objects. A * materialized value arrives as a real instance already, revived by unserialize(). + * + * TODO(type-model): a descriptor travels in-band as a plain array, so its exact key set is + * the only thing separating it from plugin data of the same shape. Every check below except + * __pclass matches on the reserved key alone, so an array a plugin built with a __rhandle or + * __phandle key of its own is read as a handle rather than kept as data. The Rust half + * (`decode_handle` in `src/value.rs`) matches the whole key set, and diverges the other way. */ public static function fromWire($value) { @@ -806,7 +812,7 @@ ShirabeRpcRuntime::$dispatch = [ }, // An already-fulfilled promise for a Rust-side call whose PHP signature declares // PromiseInterface. The Rust future ran to completion before this is called, so there is - // nothing left to defer; see .ken/plugin-arch/design.md §10.1.6. + // nothing left to defer. '__shirabe_resolved_promise' => static function ($args) { if (!function_exists('React\\Promise\\resolve')) { throw new RuntimeException( diff --git a/crates/shirabe-php-rpc/src/value.rs b/crates/shirabe-php-rpc/src/value.rs index c19c386f..7203c9fb 100644 --- a/crates/shirabe-php-rpc/src/value.rs +++ b/crates/shirabe-php-rpc/src/value.rs @@ -612,6 +612,13 @@ fn parse_quoted(payload: &[u8], pos: &mut usize, terminator: u8) -> anyhow::Resu } /// Recognizes the reserved handle-descriptor arrays by their exact key sets. +/// +/// TODO(type-model): a descriptor travels in-band as a plain array, so its exact key set is the +/// only thing separating it from plugin data of the same shape. An array that carries a reserved +/// key without matching a descriptor's whole key set is user data and should decode as an array; +/// it is a decode error here instead, and that is the fatal lane, so a plugin passing +/// `['__rhandle' => 1]` to a proxied method takes the channel down with it. The PHP half +/// (`fromWire` in `php/worker.php`) diverges the other way, matching on the reserved key alone. fn decode_handle(entries: &IndexMap, PluginValue>) -> anyhow::Result> { let get = |key: &[u8]| entries.get(key); diff --git a/crates/shirabe/src/event_dispatcher/event_dispatcher.rs b/crates/shirabe/src/event_dispatcher/event_dispatcher.rs index fb3ad5f4..a5a238bd 100644 --- a/crates/shirabe/src/event_dispatcher/event_dispatcher.rs +++ b/crates/shirabe/src/event_dispatcher/event_dispatcher.rs @@ -751,11 +751,12 @@ impl EventDispatcher { // PHP hosts the user's Command class in a throwaway, bare // `Symfony\Component\Console\Application` (NOT Composer's Application), - // built by a generated snippet running inside the worker. The command's - // output is captured in a BufferedOutput and written back through the - // dispatcher's IO; upstream hands the live output object of `$this->io` - // to `$app->run()` instead, so only the interleaving with concurrent - // writes differs. + // built by a generated snippet running inside the worker. + // + // TODO(plugin): the BufferedOutput has to go. The command has to run + // against the real output stream, the way upstream hands the live output + // object of `$this->io` to `$app->run()`; collecting the output and + // writing the buffer back once the run has returned is not a substitute. let args = additional_args .iter() .map(|arg| ProcessExecutor::escape(arg)) @@ -775,6 +776,12 @@ impl EventDispatcher { } else { output_interface::VERBOSITY_NORMAL }; + // TODO(error-model): the snippet's try/catch does not reproduce upstream's + // boundary. Upstream wraps `$app->run()` alone and catches `\Exception`, so + // an `\Error` from the command, and a throw from `new $className(...)`, + // both escape without the "terminated with an exception" line. Here the + // catch is `\Throwable`, and a constructor throw leaves the snippet as a + // `Throw` reply from `__shirabe_eval`, so the line is written either way. let snippet = format!( r#" $className = {class_name_lit}; @@ -821,6 +828,8 @@ try {{ )?; let result = match outcome { Ok(value) => value.to_php_mixed()?, + // TODO(error-model): `throw.exception_class` is dropped, so the class + // upstream rethrows unchanged collapses to RuntimeException here. Err(throw) => { self.io.write_error3( &format!( @@ -846,6 +855,9 @@ try {{ self.io.write3(&command_output, false, crate::io::NORMAL); } if let Some(throw) = result.as_array().and_then(|map| map.get("throw")) { + // TODO(error-model): the snippet reports `get_class($e)` as the first + // field and nothing reads it, so the class upstream rethrows unchanged + // collapses to RuntimeException here. let fields = throw .as_list() .expect("the eval snippet reports exceptions as a list"); diff --git a/crates/shirabe/src/plugin/php_plugin_proxy.rs b/crates/shirabe/src/plugin/php_plugin_proxy.rs index c9d6986b..8469593f 100644 --- a/crates/shirabe/src/plugin/php_plugin_proxy.rs +++ b/crates/shirabe/src/plugin/php_plugin_proxy.rs @@ -732,9 +732,10 @@ fn dispatch_config_method( /// The download manager's contract is asynchronous on both sides: PHP declares a /// `PromiseInterface` return, Rust an `async fn`. The Rust future is driven to completion here -/// and its value handed back as an already-settled React promise, which is the synchronous -/// fallback of the promise design (`.ken/plugin-arch/design.md` §10.1.6) rather than the -/// deferred resolution a concurrent engine would allow. +/// and its value handed back as an already-settled React promise. +/// +/// TODO(async): the boundary has no representation for a promise that is still pending, so the +/// deferred resolution the PHP contract allows collapses into a blocking wait here. fn dispatch_download_manager_method( dm: &std::rc::Rc>, method_name: &str, @@ -1006,12 +1007,15 @@ fn dispatch_event_dispatcher_method( if dispatcher.borrow_mut().has_event_listeners(&probe) { // TODO(plugin): dispatching a worker-constructed event through the Rust-side // dispatcher needs the event object (and the console input it carries) proxied - // back into this process; until then only the no-listener case — where - // upstream's dispatch is observably a no-op returning 0 — is supported. + // back into this process, so only the no-listener case is answered here. return Err(runtime_throw(format!( "dispatching `{name}` from the plugin process is not supported yet while listeners are registered for it" ))); } + // TODO(plugin): answering 0 here skips what `do_dispatch` does before it reaches the + // listener loop, and upstream does both regardless of the listener count: the + // `COMPOSER_DEBUG_EVENTS` trace line, and `push_event`'s circular-call detection + // (a nested dispatch of the same event name throws there even with no listeners). Ok(PluginValue::Int(0)) } other => Err(runtime_throw(format!( @@ -1610,6 +1614,15 @@ fn dispatch_package_method( } // `RootAliasPackage` overrides each of these to write through to its alias target, and // `RootPackage` reaches the same base state either way, so both go through the interface. + // + // TODO(type-model): choosing the body for the concrete variant belongs on `AnyPackage`, + // not here. The stub surface already decides which classes carry a method, so the + // `as_*_mut` accessors' "not available on an alias package" arms are unreachable for a + // method the alias stubs do not declare, and what is left is a per-variant dispatch that + // this guard only approximates: it covers `RootPackage` as well, where the extra hop is + // equivalent only while that impl keeps delegating to the base package, and nothing + // checks it. Method names repeated in the base-package arm below make the answer depend + // on arm order, and a new variant compiles into the wrong body without a diagnostic. "setRequires" | "setDevRequires" | "setConflicts" | "setProvides" | "setReplaces" | "setAutoload" | "setDevAutoload" | "setSuggests" | "setExtra" if package.borrow().is_root() => @@ -2497,8 +2510,10 @@ impl PhpInstallerProxy { /// The `?PromiseInterface` half of the installer contract. The Rust callers await the /// installer's effects rather than chaining continuations, so a returned promise is drained /// here: an already-settled one yields its value (or raises its rejection reason), while a - /// still-pending one is an explicit error — resolving it would need the concurrent execution - /// engine the boundary does not have (`.ken/plugin-arch/design.md` §10.1.6). + /// still-pending one is an explicit error. + /// + /// TODO(async): resolving a still-pending promise would need a concurrent execution engine + /// the boundary does not have. fn promise_result(&self, method: &str, value: PluginValue) -> anyhow::Result> { let handle = match value { PluginValue::Null => return Ok(None), diff --git a/crates/shirabe/src/util/filesystem.rs b/crates/shirabe/src/util/filesystem.rs index 8408a8a9..b20f5f48 100644 --- a/crates/shirabe/src/util/filesystem.rs +++ b/crates/shirabe/src/util/filesystem.rs @@ -533,8 +533,12 @@ impl Filesystem { prefer_relative: bool, ) -> String { if !self.is_absolute_path(from) || !self.is_absolute_path(to) { - // PHP throws InvalidArgumentException - // Returning early-formatted Result is not possible without changing signature; panic to surface in tests. + // TODO(error-model): PHP throws InvalidArgumentException. Plugins reach this method + // through the RPC proxy, where a relative path is ordinary input rather than a + // programming error, and this panic kills the process instead of reaching their + // catch block; the plugin dispatcher repeats the check for that reason. Returning + // `anyhow::Result` from here and from `find_shortest_path_code` removes both the + // panic and the duplicated check. panic!( "{}", format!("$from ({}) and $to ({}) must be absolute paths.", from, to) @@ -596,6 +600,8 @@ impl Filesystem { prefer_relative: bool, ) -> String { if !self.is_absolute_path(from) || !self.is_absolute_path(to) { + // TODO(error-model): as in `find_shortest_path` — PHP throws + // InvalidArgumentException, and this panic cannot reach a plugin's catch block. panic!( "{}", format!("$from ({}) and $to ({}) must be absolute paths.", from, to) diff --git a/docs/dev/plugin-class-classification.md b/docs/dev/plugin-class-classification.md index 7046ea48..1c7aec09 100644 --- a/docs/dev/plugin-class-classification.md +++ b/docs/dev/plugin-class-classification.md @@ -36,7 +36,7 @@ exactly one category. | Category | Entity lives | PHP child process sees | Rust obligation | |---|---|---|---| | `rust-proxy` | Rust | generated proxy stub (methods RPC to Rust) | full-fidelity reproduction; every public/protected method needs an RPC handler | -| `rust-snapshot` | Rust | generated snapshot class (`__rhandle` + eagerly copied fields, getters answer locally) | full-fidelity reproduction; snapshot serializer | +| `rust-snapshot` | Rust | the real class, revived from the object record the wire carries (no handle, getters answer locally) | full-fidelity reproduction; both halves of the value codec | | `contract` | n/a (interface / abstract type) | generated declaration preserving the `extends`/`implements` hierarchy | depends on direction attributes | | `two-world` | both, independent siblings | the real PHP implementation (same FQCN, re-defined or vendor-loaded) | independent Rust implementation; only the seam objects (composer / io / dispatcher) are shared | | `php-native` | PHP | the real, unmodified PHP source | none — Rust may or may not have its own port for internal use, and that port is free to diverge in shape | @@ -59,10 +59,15 @@ entity. #### rust-snapshot -Immutable value objects, e.g. `Link` and the security-advisory family. The -child receives the field values together with an interned `__rhandle`, so -identity (`===`) is preserved while getters answer locally with zero -round-trips. +Immutable value objects, e.g. `Link` and the security-advisory family. A +value has no entity to point at, so it gets no handle and no stub: the wire +carries the object record `serialize()` writes for it, `unserialize()` +revives a genuine instance of the real class without running a constructor, +and getters answer locally with zero round-trips. Identity is not preserved +— two calls of the same getter yield two objects in the child (see +`docs/dev/php-rpc.md`). Only the classes on the codec's closed list cross +this way today (`Link` and the `composer/semver` constraints it holds); the +rest of the category has no artifact yet and is guarded. #### contract @@ -124,8 +129,10 @@ Drives which side needs stubs and which needs adapters. A non-abstract class with a public constructor that is also reachable from the graph (e.g. `JsonFile`, obtainable via `Locker::getJsonFile()` *and* freely `new`ed by plugins). These need a constructor story on the stub (the -stub ctor must RPC a `NewObject` so the entity is allocated Rust-side); the -classifier surfaces them because they are individually design-sensitive. +stub constructor forwards to `__shirabeConstruct` on the runtime service +endpoint, and the Rust side allocates the entity and answers with its +handle); the classifier surfaces them because they are individually +design-sensitive. #### mutable-static @@ -350,8 +357,10 @@ reviewable outcome in the report, not a silent guess. ## Known deviations and open questions The mechanical rules surfaced several points where earlier design prose was -incomplete or a decision is still owed. Each needs an explicit user -decision; the tool keeps them visible instead of resolving them silently. +incomplete or a decision is still owed. Some have been decided since, and +this section records what the implementation does instead; the rest still +need an explicit user decision, and the tool keeps them visible instead of +resolving them silently. ### ProcessExecutor and HttpDownloader are reachable @@ -365,28 +374,55 @@ it as a stateless utility — survives only for plugin-`new`ed instances. This is exactly the dual-instantiation situation the `plugin-constructible` attribute exists to surface. -Proxy-side access to the graph-owned instances (the `getLoop()` route) is -left unimplemented — a `todo!()`-style explicit error, not a silent stub — -until a real plugin demonstrates the need. Note that `executeAsync()` -throws a `LogicException` without the `@internal` `enableAsync()` (called -only by `Loop::__construct`), so plugin-`new`ed instances never reach the -async path anyway. +Both routes raise an explicit error today, and which one gets a story first +is still open. The graph-owned instances cannot be obtained at all: the +`Composer` proxy answers `getLoop()` with an explicit error, and `Loop`, +`ProcessExecutor` and `HttpDownloader` are guarded classes, so a +plugin-`new`ed instance is an explicit error too rather than a second +instance the Rust side never sees. + +An earlier argument for leaving plugin-`new`ed instances alone does not +hold: `executeAsync()` does throw a `LogicException` unless `enableAsync()` +ran first, but `enableAsync()`, `wait()` and `countActiveJobs()` are all +public — `@internal` is a docblock note — so nothing stops a plugin from +driving its own instance through the async path. ### Dual instantiation `Locker::getJsonFile(): JsonFile` makes `JsonFile` reachable, so it is `rust-proxy` + `plugin-constructible` — and plugins `new JsonFile(...)` -constantly. The question is not cosmetic: `ProcessExecutor`, `JsonFile`, -and `Util\Filesystem` being `rust-proxy` is what demotes the VCS/auth -utility belt (`Git`, `GitHub`, `GitLab`, `Bitbucket`, `Svn`, `AuthHelper`, -`RemoteFilesystem`) to `unsupported` — each of them constructs one of those -three internally. `ArrayLoader` is a fourth member: plugins `new -ArrayLoader` constantly, and it drives constructor-plus-setters on the -package classes, so its fate follows theirs. These utilities can become -php-native the moment plugin-`new`ed instances of the trio may live -PHP-locally (or the stub `NewObject` constructor story lands); until the -user decides, the tool reports them as `unsupported` with the constructing -site named. +constantly. The question is not cosmetic: a `rust-proxy` class a plugin +constructs demotes the constructing class, and the demotions cascade. +`ProcessExecutor` is what carries the VCS/auth utility belt to +`unsupported`: `GitHub`, `GitLab`, `Bitbucket` and `Svn` construct one, +`Git` constructs a `Bitbucket`, `AuthHelper` a `GitHub`, and +`RemoteFilesystem` an `AuthHelper`. `JsonFile` takes `ConfigValidator` and +`RepositoryFactory` the same way, and `ArrayLoader` — which plugins `new` +just as constantly, and which drives constructor-plus-setters on the +package classes — takes `VersionSelector` and `VersionBumper`. + +The construction half now exists as a mechanism rather than a question: a +stub constructor forwards to `__shirabeConstruct`, the Rust side allocates +the entity and answers with its handle, and the plugin-`new`ed object is +then the same entity the graph sees. It is filled in per class, driven by +the explicit errors real plugins hit — today `Package`, `CompletePackage`, +the three alias packages, the five solver operations and +`Composer\Util\Filesystem` can be built this way, the last one only without +a `ProcessExecutor` argument (that class has no stub, so the argument could +only be an instance the Rust side never sees). Every other proxied class +answers with an explicit error naming it. + +`JsonFile`, `ArrayLoader` and `ProcessExecutor` are still undecided, so the +classes above stay `unsupported` — but a guard now shadows each of them in +the child, so touching one is an explicit error instead of real code +running against a second instance. + +The demotion rule has not been taught about the construction stories: it +demotes for constructing any `rust-proxy` service, whether or not that +service can now be built. `Composer\Package\Archiver\ArchivableFilesFinder` +is `unsupported` for `new Filesystem` alone, which no longer forks any +state; promoting such a class means feeding the per-class construction +stories back into the rule. ### Process: dual instantiation split by caller @@ -425,13 +461,26 @@ The child process necessarily `require`s the project's real `InstalledVersions`) to autoload plugin code, before any stub could load. A same-FQCN stub cannot coexist; both are overridden to `php-native`. `InstalledVersions::$installed` is nonetheless genuinely shared state — -Rust rewrites `installed.php` on every dump — so the Rust side must push a -reload (`InstalledVersions::reload()`) after installs, or post-install -event handlers read stale data. Whether and when that reload push happens, -and how plugin-class autoloading is split between the two worlds, is not -yet decided; until it is, the `InstalledVersions` state a plugin observes -after a Rust-side dump is undefined. The Rust-side reload site carries a -`TODO(plugin)` marker. +Rust rewrites `installed.php` on every dump — and the Rust side pushes the +reload: right after writing the file it calls +`__shirabe_installed_versions_reload` in the worker, which mirrors the tail +of `FilesystemRepository::write` (the unconditional +`InstalledVersions::reload($versions)` plus the reflection-based +`selfDir`/`installedIsLocalDir` restore). It is skipped when no worker runs, +and inside the worker when the class is not even autoloadable there; in +both cases there is no observer code either. + +Plugin classes are autoloaded from the Rust side. The loaders +`PluginManager::registerPackage` builds live in the Rust process, and the +worker's autoloader asks them for a file through the runtime service +endpoint (`__shirabe_find_file`); the files-autoload entries and the +`_composer_tmp` class-rename path run in the worker. One gap remains: the +Rust `spl_autoload_register` is a no-op and `ClassLoader::register` keeps +one loader per vendor directory (matching upstream's `$registeredLoaders`) +where PHP's autoload stack keeps every registered loader, so a second +plugin loader registered under the same vendor directory evicts the first, +and a class of the earlier plugin that was never loaded becomes +unresolvable. ### ConsoleIO leaks world-2 objects @@ -440,7 +489,8 @@ seam: `getTable(): Table` / `getProgressBar(): ProgressBar`, and its constructor takes `InputInterface`/`OutputInterface`/`HelperSet` — none of which can cross the wire as values. The stub needs a bespoke story (e.g. a local Table bound to a proxying `OutputInterface`); until one is designed, -both members raise explicit errors. +both members raise explicit errors, as does the constructor — the Rust side +has no construction story for this class. ## The classifier tool -- cgit v1.3.1-4-g156e