demos: introduce DevModule + rename bin/app.php to bin/be.php - #19
Merged
Merged
Conversation
Each runnable demo (hello-world, medical-triage, user-registration, order-processing) now wraps Becoming with a per-demo DevBecoming that flushes the semantic log to var/log/<demo>.json in a finally block, so failed runs are captured too. The dev-specific bindings are isolated in DevModule (AppModule stays focused on the demo's own bindings). A generated var/log/<demo>.json is checked in alongside each demo as a reference transcript, readable without running the code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR adds development environment infrastructure across demo projects by introducing Changes
Sequence DiagramsequenceDiagram
actor Script as bin/be.php
participant Injector as Ray\\Di\\Injector
participant DevModule as DevModule
participant DevBecoming as DevBecoming
participant Becoming as Becoming<br/>(Wrapped)
participant Logger as SemanticLogger
participant FileSystem as var/log/
Script->>Injector: new Injector(DevModule)
Injector->>DevModule: configure()
DevModule->>Injector: Install AppModule & bind interfaces
DevModule->>Injector: Bind SemanticLoggerInterface via Provider
Script->>Injector: getInstance(BecomingInterface)
Injector->>DevBecoming: new (wrapped, logger)
Injector-->>Script: DevBecoming instance
Script->>DevBecoming: __invoke(input)
activate DevBecoming
DevBecoming->>Becoming: __invoke(input)
activate Becoming
Becoming-->>DevBecoming: result (object)
deactivate Becoming
Note over DevBecoming: finally block executes<br/>regardless of success/failure
DevBecoming->>Logger: flush()
Logger-->>DevBecoming: events array
DevBecoming->>DevBecoming: json_encode(events)
DevBecoming->>FileSystem: file_put_contents(*.json)
deactivate DevBecoming
DevBecoming-->>Script: result
Script-->>Script: assert & print output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
demos/user-registration/src/Becoming/DevBecoming.php (2)
30-46: Exception raised insidefinallywould mask the original Becoming failure.If writing the log throws (e.g., disk full, permissions,
json_encodereturningfalsewithJSON_THROW_ON_ERROR-like semantics indirectly via a later call), that exception replaces any exception propagating from($this->becoming)($input), hiding the real pipeline failure from the developer running the demo. For a dev-only tool this is acceptable, but wrapping the persistence in its owntry/catch(logging to stderr instead of throwing) would keep the observed failure signal intact.♻️ Optional hardening
try { return ($this->becoming)($input); } finally { - $dir = dirname(__DIR__, 2) . '/var/log'; - if (! is_dir($dir)) { - mkdir($dir, 0755, true); - } - - file_put_contents( - $dir . '/user-registration.json', - json_encode($this->logger->flush(), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE), - ); + try { + $dir = dirname(__DIR__, 2) . '/var/log'; + if (! is_dir($dir)) { + mkdir($dir, 0755, true); + } + + $json = json_encode( + $this->logger->flush(), + JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE, + ); + if ($json !== false) { + file_put_contents($dir . '/user-registration.json', $json); + } + } catch (\Throwable $e) { + fwrite(STDERR, "DevBecoming log write failed: {$e->getMessage()}\n"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/user-registration/src/Becoming/DevBecoming.php` around lines 30 - 46, The finally block in DevBecoming::__invoke currently writes logs and can throw, which would mask exceptions from ($this->becoming)($input); wrap the entire persistence sequence (the dirname/ mkdir check, json_encode of $this->logger->flush(), and file_put_contents) in its own try/catch inside the finally so any errors are caught and do not propagate; on catch write a concise error to STDERR (e.g., via fwrite(STDERR, ...)) and do not rethrow; additionally validate json_encode succeeded (or use JSON_THROW_ON_ERROR inside that try) before calling file_put_contents to avoid silent false results from $this->logger->flush().
22-47: Heads-up: near-identicalDevBecomingwill land in each demo.This class is structurally the same across the four demos, differing only in the JSON filename (
user-registration.json,hello-world.json, etc.). That’s consistent with the PR’s “per-demo DevModule” philosophy, and the filename-by-demo is the only real variation — so a shared base class probably isn’t worth it. Flagging only so future maintainers know any bug fix here (e.g., thefinallyconcern above) needs to be replicated in the other threeDevBecomingfiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/user-registration/src/Becoming/DevBecoming.php` around lines 22 - 47, DevBecoming's __invoke finally block is duplicated across demos with only the output filename differing (e.g., user-registration.json vs hello-world.json); when you fix the finally behavior (e.g., ensure logger->flush() errors are handled, directory creation permissions, or atomic file write), apply the same change to the other DevBecoming classes so all demos stay consistent—locate the DevBecoming class and its __invoke method and update the file write logic (logger->flush(), dirname(...)/var/log creation, and file_put_contents call) and replicate the identical fix to the other three DevBecoming files, preserving each demo's distinct JSON filename.demos/medical-triage/bin/be.php (1)
22-27: Consider asserting the final type for parity with other demos.The sibling entrypoints (hello-world, user-registration, order-processing) all do
assert($final instanceof <FinalClass>)before using the result. This script only prints$final::class, so if the pipeline ever returns an unexpected object the demo silently accepts it. Adding a matching assert keeps the demos consistent and documents the expected outcome.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/medical-triage/bin/be.php` around lines 22 - 27, Add an instanceof assertion to verify the pipeline output ($final) matches the expected final class before using it: locate the code that calls $becoming(new PatientInput(...)) and immediately after the result is produced (before or after the printf) add an assertion like assert($final instanceof <ExpectedFinalClass>) — replace <ExpectedFinalClass> with the actual final type returned by the triage pipeline (e.g., TriageResult) to match the other demos' pattern and document the expected outcome.demos/user-registration/bin/be.php (1)
33-33: Relativevendor/bin/streepath couples script to CWD.
passthru('vendor/bin/stree ' . ...)only resolves when invoked from the demo directory (as documented in the PR test plan). Runningphp demos/user-registration/bin/be.phpfrom the repo root will fail to locatestreeeven though the script otherwise works viadirname(__DIR__)for autoload. Consider resolving the binary path relative to the script, mirroring howvendor/autoload.phpis resolved.♻️ Proposed fix
-passthru('vendor/bin/stree ' . escapeshellarg(dirname(__DIR__) . '/var/log/user-registration.json')); +passthru( + escapeshellarg(dirname(__DIR__) . '/vendor/bin/stree') . ' ' + . escapeshellarg(dirname(__DIR__) . '/var/log/user-registration.json'), +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/user-registration/bin/be.php` at line 33, The passthru call uses a relative 'vendor/bin/stree' which breaks when the script is run from a different CWD; change the command to build the binary path relative to the script directory (use dirname(__DIR__, 3) to reach the repo root from demos/user-registration/bin), e.g. compute the full path to vendor/bin/stree via dirname(__DIR__, 3) . '/vendor/bin/stree', wrap it with escapeshellarg as before, and use that value in the passthru call (keep the existing passthru, escapeshellarg and dirname usage but resolve vendor/bin/stree relative to the script).demos/medical-triage/src/Becoming/DevBecoming.php (1)
22-47: Consider extracting sharedDevBecominglogic to reduce per-demo duplication.This class is nearly identical across
hello-world,medical-triage,order-processing, anduser-registration— only the output filename differs. Since each demo has its ownsrc/tree andcomposer.json, full deduplication may be out of scope, but you could at least parameterize the log filename (e.g., pass it via a#[Named]string binding inDevModule) so the class bodies become trivially mergeable later, or ship a single shared helper. Optional — purely a maintainability nit for dev-only code.Also, two minor robustness notes for the
finallyblock:
json_encode(...)can returnfalse(e.g., on malformed UTF-8 or a circular structure thrown fromflush());file_put_contentswill then silently write an empty file, masking the real problem. AddingJSON_THROW_ON_ERRORwould surface it.mkdir(...)return value is ignored; if creation fails due to a race or permissions, the subsequent write fails silently.♻️ Suggested tightening
- $dir = dirname(__DIR__, 2) . '/var/log'; - if (! is_dir($dir)) { - mkdir($dir, 0755, true); - } - - file_put_contents( - $dir . '/medical-triage.json', - json_encode($this->logger->flush(), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE), - ); + $dir = dirname(__DIR__, 2) . '/var/log'; + if (! is_dir($dir) && ! mkdir($dir, 0755, true) && ! is_dir($dir)) { + return; + } + + file_put_contents( + $dir . '/medical-triage.json', + json_encode( + $this->logger->flush(), + JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR, + ), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/medical-triage/src/Becoming/DevBecoming.php` around lines 22 - 47, DevBecoming's finally block should be made robust and the filename parameterized: change DevBecoming to accept a filename (e.g., via the constructor/#[Named] binding) instead of hardcoding 'medical-triage.json', and update __invoke to use that injected filename; in the finally block, call json_encode/flush with JSON_THROW_ON_ERROR to surface encoding errors (catching and logging if needed), ensure mkdir succeeded (or re-check is_dir and log/throw if creation failed) before writing, and verify file_put_contents returned a non-false result (log/throw on failure) after calling $this->logger->flush() so write errors aren’t silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/order-processing/var/log/order-processing.json`:
- Line 53: Replace the real-looking test PAN used in the JSON transcript by
changing the "cardNumber" value (currently "4111111111111111") to an
obviously-fake or masked string like "************1111" or "<redacted>" so
secret scanners won’t flag the committed sample while keeping the transcript
readable.
In `@demos/user-registration/var/log/user-registration.json`:
- Around line 94-98: The sample transcript contains deterministic-looking
credentials (welcomeToken, hashedPassword, avatarUrl derived from email) that
trigger secret scanners; update the fixture output in user-registration.json to
use obviously-scrubbed placeholders (e.g., set welcomeToken to "<generated>",
hashedPassword to "<bcrypt>", avatarUrl to "<avatar>") or otherwise redact the
values so the JSON still shows the shape but no longer looks like real secrets,
ensuring any code that reads these fixtures handles the placeholder format where
relevant (search for uses of welcomeToken, hashedPassword, avatarUrl in relevant
test/fixture loaders and adjust expectations if needed).
---
Nitpick comments:
In `@demos/medical-triage/bin/be.php`:
- Around line 22-27: Add an instanceof assertion to verify the pipeline output
($final) matches the expected final class before using it: locate the code that
calls $becoming(new PatientInput(...)) and immediately after the result is
produced (before or after the printf) add an assertion like assert($final
instanceof <ExpectedFinalClass>) — replace <ExpectedFinalClass> with the actual
final type returned by the triage pipeline (e.g., TriageResult) to match the
other demos' pattern and document the expected outcome.
In `@demos/medical-triage/src/Becoming/DevBecoming.php`:
- Around line 22-47: DevBecoming's finally block should be made robust and the
filename parameterized: change DevBecoming to accept a filename (e.g., via the
constructor/#[Named] binding) instead of hardcoding 'medical-triage.json', and
update __invoke to use that injected filename; in the finally block, call
json_encode/flush with JSON_THROW_ON_ERROR to surface encoding errors (catching
and logging if needed), ensure mkdir succeeded (or re-check is_dir and log/throw
if creation failed) before writing, and verify file_put_contents returned a
non-false result (log/throw on failure) after calling $this->logger->flush() so
write errors aren’t silently ignored.
In `@demos/user-registration/bin/be.php`:
- Line 33: The passthru call uses a relative 'vendor/bin/stree' which breaks
when the script is run from a different CWD; change the command to build the
binary path relative to the script directory (use dirname(__DIR__, 3) to reach
the repo root from demos/user-registration/bin), e.g. compute the full path to
vendor/bin/stree via dirname(__DIR__, 3) . '/vendor/bin/stree', wrap it with
escapeshellarg as before, and use that value in the passthru call (keep the
existing passthru, escapeshellarg and dirname usage but resolve vendor/bin/stree
relative to the script).
In `@demos/user-registration/src/Becoming/DevBecoming.php`:
- Around line 30-46: The finally block in DevBecoming::__invoke currently writes
logs and can throw, which would mask exceptions from ($this->becoming)($input);
wrap the entire persistence sequence (the dirname/ mkdir check, json_encode of
$this->logger->flush(), and file_put_contents) in its own try/catch inside the
finally so any errors are caught and do not propagate; on catch write a concise
error to STDERR (e.g., via fwrite(STDERR, ...)) and do not rethrow; additionally
validate json_encode succeeded (or use JSON_THROW_ON_ERROR inside that try)
before calling file_put_contents to avoid silent false results from
$this->logger->flush().
- Around line 22-47: DevBecoming's __invoke finally block is duplicated across
demos with only the output filename differing (e.g., user-registration.json vs
hello-world.json); when you fix the finally behavior (e.g., ensure
logger->flush() errors are handled, directory creation permissions, or atomic
file write), apply the same change to the other DevBecoming classes so all demos
stay consistent—locate the DevBecoming class and its __invoke method and update
the file write logic (logger->flush(), dirname(...)/var/log creation, and
file_put_contents call) and replicate the identical fix to the other three
DevBecoming files, preserving each demo's distinct JSON filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cca40b2-6691-41ff-a0c1-a9277aa45456
📒 Files selected for processing (22)
demos/hello-world/bin/be.phpdemos/hello-world/src/Becoming/DevBecoming.phpdemos/hello-world/src/Module/AppModule.phpdemos/hello-world/src/Module/DevModule.phpdemos/hello-world/src/Module/DevSemanticLoggerProvider.phpdemos/hello-world/var/log/hello-world.jsondemos/medical-triage/bin/be.phpdemos/medical-triage/src/Becoming/DevBecoming.phpdemos/medical-triage/src/Module/DevModule.phpdemos/medical-triage/src/Module/DevSemanticLoggerProvider.phpdemos/medical-triage/var/log/medical-triage.jsondemos/order-processing/bin/be.phpdemos/order-processing/src/Becoming/DevBecoming.phpdemos/order-processing/src/Module/DevModule.phpdemos/order-processing/src/Module/DevSemanticLoggerProvider.phpdemos/order-processing/var/log/order-processing.jsondemos/user-registration/bin/be.phpdemos/user-registration/src/Becoming/DevBecoming.phpdemos/user-registration/src/Module/AppModule.phpdemos/user-registration/src/Module/DevModule.phpdemos/user-registration/src/Module/DevSemanticLoggerProvider.phpdemos/user-registration/var/log/user-registration.json
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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
bin/app.php→bin/be.phpacross all demos (aligns with skeleton's universal entry convention).DevModulethat installsAppModuleand rebindsBecomingInterfacetoDevBecomingplus aSemanticLoggerInterfaceprovider — mirrors the skeleton's dev/app module split.DevBecomingwrapper per demo: writesvar/log/<demo>.jsonfrom afinallyblock so failed pipelines are captured too.var/log/<demo>.json) as reference outputs readers can inspect alongside the code.Test plan
cd demos/hello-world && composer install && php bin/be.phpproduces expected output and logmedical-triage,user-registration,order-processingvendor/bin/stree var/log/<demo>.jsonrenders each transcript🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation