🐛 fix(bones): the safety fixes from the CLI audit - #111
Conversation
deploy
- Refuse a destination that is the plugin, one of its parents or inside
it, whatever the flags: `deploy ..` deleted every plugin beside this one
and the plugin itself, `deploy build` copied the plugin into itself
until the path was too long.
- Replace an existing folder only when it is empty or holds a previous
deploy of this plugin (its main file, same Plugin Name); anything else
needs the new --force. The five-second pause before deleting is gone.
make:*
- Never overwrite an existing file unless --force is passed.
- Validate the class name: PHP identifiers, "/" between folders, no "..".
- One generateClass() for every generator, so Folder/Class works in all
of them with the namespace following the folder: make:provider kept
"Folder/Class" as the class name and wrote nothing, six generators
wrote into folders they never created, and all of them said "Created"
when the write failed.
- The eloquent-model stub wrote `namespace ...\Models\;`, which does not
parse; seven stubs gain {Path}.
- migrate:create asks for a missing name instead of a TypeError, and
creates database/migrations.
- make:widget keeps the plugin's widget views when they already exist.
tinker
- A loop instead of a recursive call from `finally`: it ends with `exit`
or with its input, where it used to recurse until the process died.
- Every Throwable is reported; an Error used to print nothing, and the
catch block ran eval() on the exception message.
- A returned value is printed; variables persist between lines.
exit codes and shell
- Unknown command, missing class name and declined confirmations
(version, rename, migrate:to-v2) exit 1; errors go to STDERR.
- install, optimize, update and require stream the command through
passthru() and use its status: `line(null)` after a silent
`composer dump-autoload` was a TypeError, and require renamed after a
failed install. The package name is escaped.
44 tests in tests/Console (deploy, make, tinker, exit codes) over a
shared BonesProcess helper; 144 tests in all.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Three unresolved issues in src/Console/bin/bones must be addressed before approval.
Review effort: Lite
Findings: None
What changed in this PR
Hardens the bones CLI against destructive deploys, unsafe generation, tinker failures, and incorrect exit statuses.
Changes:
- Adds deploy safety, validation, overwrite protection, and process-based regression tests.
- Centralizes class generation and updates namespaces in stubs.
- Improves tinker behavior and shell command status handling.
Unresolved findings remain in src/Console/bin/bones: EOF input can cause a TypeError (critical, 2 votes), standalone optimize may exit successfully after Composer failure (moderate, 1 vote), and widget --force does not overwrite existing views (moderate, 1 vote).
| File | Summary |
|---|---|
tests/Support/BonesProcess.php |
Provides isolated CLI process testing. |
tests/Console/TinkerTest.php |
Tests tinker behavior. |
tests/Console/MakeCommandsTest.php |
Tests generators and migrations. |
tests/Console/ExitCodeTest.php |
Tests exit codes and stderr handling. |
tests/Console/DeploySafetyTest.php |
Tests destructive deploy prevention. |
src/Console/stubs/widget.stub |
Updates widget generation behavior. |
src/Console/stubs/shortcode.stub |
Adds folder-aware namespace support. |
src/Console/stubs/schedule.stub |
Adds folder-aware namespace support. |
src/Console/stubs/provider.stub |
Adds folder-aware namespace support. |
src/Console/stubs/eloquent-model.stub |
Fixes model namespace generation. |
src/Console/stubs/ctt.stub |
Adds folder-aware namespace support. |
src/Console/stubs/cpt.stub |
Adds folder-aware namespace support. |
src/Console/stubs/ajax.stub |
Adds folder-aware namespace support. |
src/Console/bin/bones |
Implements the CLI safety and behavior fixes; unresolved findings remain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- resolvePath() collapsed ".." before following symlinks, so `deploy out/alias/.. --force`, with alias pointing into the plugin, passed the guard as out/ while deleteDirectory() emptied the plugin (Codex). It now resolves one segment at a time, like the filesystem. - ask() returned null for a null default (askPackageManager() without npm) from a function declared to return string, and gave no way to tell that input had ended: `rename` with no arguments and no stdin asked forever. It now returns a string and records the end of input, and rename stops there with exit 1. - `php bones optimize` exits with Composer's status (Copilot). - make:app exits 1 on a missing, invalid, reserved or existing name. - make:widget says why it keeps the shared views even with --force.
|
Review round 1, all in 9ededc9:
Also: |
On Windows `deploy '\' --force` targets the current drive's root, but the guard kept the target as "\" while the source was "C:\...", so the "contains the plugin" check missed it; and `c:\` against `C:\` missed the same way (Codex, second round). A path starting with one separator gets the current drive, the drive letter is upper-cased, and on Windows the comparison is case-insensitive. Reasoned from the code: no Windows machine here to run it on.
The fixes from an audit of the
bonesCLI, all reproduced before they were fixed. Nothing documented changes behaviour except where the old behaviour destroyed something.deploy
php bones deploy ..removed every plugin beside this one and the plugin itself,deploy buildcopied the plugin intobuild/build/build/…until the path was too long.Plugin Name); anything else is left alone and needs the new--force. The pause is gone.make:*
--forcedoes, wherever it is on the line./between folders, no..(../../Escapedused to write outside the folder,my-modelwrote a file that does not parse).generateClass()behind all of them, soFolder/Classworks everywhere and the namespace follows the folder.make:provider Shop/BillingkeptShop/Billingas the class name and wrote nothing; six generators wrote into folders they never created; every one said "Created" when the write failed.eloquent-model.stubwrotenamespace …\Models\;, which never parsed. Seven stubs gain{Path}.migrate:createasks for a missing name instead of a TypeError, and createsdatabase/migrations.make:widgetkeeps the widget views it finds instead of rewriting them.tinker
finally: it ends withexitor with its input (it used to recurse until the process died with 255).Throwableis shown. AnErrorused to print nothing, and the catch block raneval()on the exception message:throw new \Exception("print 6*7;")printed 42.Exit codes and shell
version,rename,migrate:to-v2) exit 1, with the error on STDERR. Aversionnobody answered used to exit 0 having changed nothing.install,optimize,updateandrequirestream throughpassthru()and use the status.optimizehandedline()a null whencomposer dump-autoloadwrote only to stderr, a TypeError after everymake:controllerin a plugin without post-autoload-dump scripts;requirerenamed after a failed install. The package name is escaped.Tests
tests/Consoleover a sharedBonesProcesshelper (runs the CLI with stdin, separate stdout/stderr, a timeout):DeploySafetyTest,MakeCommandsTest,TinkerTest,ExitCodeTest. On v2.0.9, 33 of them fail (the two self-copy deploy cases were not run there: they nest 160 folders deep).composer test: 144 tests, OK.