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 ++++++-- 5 files changed, 61 insertions(+), 15 deletions(-) (limited to 'crates') 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) -- cgit v1.3.1-4-g156e