fix(php): clear both quarantined test groups, and the dead code they were hiding - #14
Merged
Conversation
php/Agentic/ held eight files whose namespaces resolve one directory higher: Core\Mod\Agentic\ maps to php/, so Core\Mod\Agentic\Livewire\ HubComponent had to live at php/Livewire/HubComponent.php and instead sat at php/Agentic/Livewire/HubComponent.php. Composer never loaded any of them. The three component classes extend HubComponent, so every one of them was fatal on load, and BrainExplorer, CreditLedger and FleetOverview have never been reachable in a running application. The tests hid it. LivewireTestCase::livewireComponent() require_once'd the component file by path, and the two service tests carried their own loadAgenticPhpClass() doing the same, so the classes were pulled in by hand and the missing autoloading never showed. Both workarounds go; the files are moved to php/Livewire, php/Data and php/Services, which is where their declared namespaces have always pointed. Their view paths moved with them: the components resolve Blade through __DIR__.'/../../resources/views/...', which was correct from php/Agentic/ Livewire and is one level too deep from php/Livewire. The three Feature/Agentic/Livewire tests were excluded in phpunit.xml rather than fixed. Pest rejects a second uses() binding on a file no matter the inheritance, so a file-level uses(LivewireTestCase::class) under a directory-wide uses(TestCase::class) always throws TestCaseAlreadyInUse. Class-style tests are not matched by uses()->in() at all, which is why the thirteen sibling files in Feature/Livewire have always run — these three are now written the same way. BrainExplorerTest also fakes HTTP, because forgetting a memory posts a delete to Qdrant and was reaching localhost:6334 for real. Feature/Agentic: 15 passed, was 9 uncollectable plus 2 failing. Co-Authored-By: Virgil <virgil@lethean.io>
Twenty production files imported Mod\Content\* — ContentTask, ContentBrief,
AIGatewayService, GenerateContentJob and the rest. No such namespace exists.
The real one is Core\Mod\Content\, shipped by dappcore/php-content, which
was not a dependency of this package at all. So BatchContentGeneration::
handle() would fatal on "Class not found" the moment a worker picked it up,
and the MCP content tools with it.
dappcore/php-content joins require-dev, matching how dappcore/php-tenant is
already carried here: it is a sibling module a host application supplies.
Its only new transitive dependency is ezyang/htmlpurifier.
Wiring the real classes in exposed three things the phantom ones had been
hiding:
interpolateVariables() built its placeholder as '{{{'.$key.'}}}' — three
braces. Every template in the package writes {{name}}, and Prompt::
interpolate() builds the same placeholder correctly as "{{{$key}}}", which
is $key between two brace pairs, not three. The concatenated form was a
mis-transcription of it, so no variable has ever been substituted and user
templates reached the provider verbatim.
An entitlement denial recorded "Entitlement denied: " and nothing else.
EntitlementResult keeps its text in a readonly $reason behind getMessage()
and has no $message property and no __get, so $result->message was always
an undefined property read.
The tests were written against the phantoms and had to be corrected, not
merely re-pointed: mocks of a non-existent class accept anything, so they
asserted nothing. ContentTask and Prompt doubles are makePartial() so real
Eloquent attribute handling runs; EntitlementResult and UsageRecord are the
real classes, since can() and recordUsage() are typed and reject look-alikes;
recordUsage()'s expectation is positional because a named-argument
expectation leaves $user unmatched. ProcessContentTaskTest also passed a
ContentProcessingService as handle()'s second argument — a signature this
job has never had — and Unit/ProcessContentTaskTest asserted the
three-brace placeholder.
BatchContentGenerationTest's remaining skip goes too. It alias-mocked
ContentTask to fake the static query(), which replaces the class for the
whole PHP process and is why it needed isolation; it now runs against a
real content_tasks table created for that test. Only that table, because
php-content's migration also recreates prompts with columns stricter than
this package's own.
Feature/Jobs + Unit/ProcessContentTask: 70 passed, 3 failed, was 43 passed,
27 failed. The 3 are pre-existing and reach a live Qdrant/Elasticsearch.
Co-Authored-By: Virgil <virgil@lethean.io>
Five tests in ContentServiceTest did nothing but markTestSkipped('Help
article prompt not found'), on every machine and in CI. The prompt was
never going to be there: ContentService resolves its paths as base_path()
plus a relative prefix defaulting to app/Mod/Agentic/Resources/*, which
belongs to a host application, and under Testbench base_path() is the bare
skeleton. Three more tests in the same file failed rather than skipped, for
the same reason — they read a batch-001-link-getting-started fixture that
only a host app would have.
All three prefixes are config-driven, so the suite now points them at a
sandbox under base_path() and writes the prompt template and batch spec it
needs, cleaning up afterwards. The per-test finally blocks go with it,
since afterEach removes the whole tree.
ContentServiceTest: 10 passed, was 2 passed, 3 failed, 5 skipped.
The CI comment is brought back in line with the suite: 1311 tests, 1155
passing, 156 failing, nothing skipped or excluded. It named the missing
Core\Mcp dependency trait, so that is now described accurately —
ValidatesDependencies and HasDependencies belong to dappcore/mcp, which
claims the same Core\Mcp\ PSR-4 root this package also maps onto php/Mcp,
so adding it is a namespace collision to resolve, not a plain install.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
composer.lock pins symfony/{clock,string,translation,css-selector,
event-dispatcher} at v8.1.x, every one of which requires php >=8.4.1. The
job ran 8.3, so composer install aborted with "Your lock file does not
contain a compatible set of packages" and the step failed before a single
test executed. That has been true on main since the lock was written — the
suite this job exists to run has never run in it, which is also why the
step's own comment still quoted 993 passing and 303 failing, numbers no CI
run has ever produced.
The pin predates this branch: symfony/clock v8.1.0 is in main's lock at the
same version, and none of the three commits before this one touch those
entries.
composer.json declared php ^8.2 against that lock, so the floor was both
untested and uninstallable — a false declaration. Raised to ^8.4 to match
what the dependency tree actually requires. Wanting 8.2 back is a deliberate
piece of work (pin config.platform.php and resolve the tree down to it), not
something to leave asserted and broken in the meantime.
Co-Authored-By: Virgil <virgil@lethean.io>
Snider
force-pushed
the
fix/quarantine-clearance
branch
from
August 8, 2026 08:53
cb00c2b to
a8552d6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Clears both quarantined PHP test groups. Every test now either runs or is gone with a receipt — no
markTestSkipped, no->skip(), no testsuite<exclude>left in the repo.Suite
Zero regressions — failing-test names were diffed before and after; no test that passed at baseline fails now. Total rises by 9 because those 9 could not previously be collected at all.
Adjudication
Group 1 —
<exclude>php/tests/Feature/Agentic/Livewire(3 files, 9 tests) → badly linkedThe stated reason was true but shallow. Root cause:
php/Agentic/held eight files whose namespaces resolve one directory higher.Core\Mod\Agentic\maps tophp/, soCore\Mod\Agentic\Livewire\HubComponenthad to live atphp/Livewire/HubComponent.phpand instead sat atphp/Agentic/Livewire/HubComponent.php. Composer never loaded any of them. All three components extendHubComponent, soBrainExplorer,CreditLedgerandFleetOverviewhave never been reachable in a running application.The tests hid it:
LivewireTestCase::livewireComponent()require_once'd the component file by path, and the twoFeature/Agentic/Servicestests carried their ownloadAgenticPhpClass()doing the same. Both workarounds are gone; the files moved tophp/Livewire,php/Dataandphp/Services, where their declared namespaces have always pointed. Their__DIR__-relative Blade paths moved with them.The three tests are now PHPUnit classes. Pest raises
TestCaseAlreadyInUsefor any seconduses()binding on a file, by class name, so a subclass is rejected just the same — class-style tests are not matched byuses()->in()at all, which is why the thirteen siblings inFeature/Livewirehave always run.Feature/Agentic: 15 passed (was 9 uncollectable + 2 failing).Group 2 — the six skips → both fixable
ContentServiceTest, 5×markTestSkipped('Help article prompt not found').ContentServiceresolves paths asbase_path()plus a config prefix defaulting toapp/Mod/Agentic/Resources/*— a host application path. Under Testbenchbase_path()is the bare skeleton, so the prompt could never exist: a permanent skip on every machine and in CI. Three more tests in the file failed for the same reason. The prefixes are config-driven, so the suite now points them at a sandbox and writes its own fixtures.ContentServiceTest: 10 passed (was 2 passed / 3 failed / 5 skipped).BatchContentGenerationTest->skip('Alias mocking requires process isolation'). It alias-mockedMod\Content\Models\ContentTask— a namespace that exists nowhere. The real one isCore\Mod\Content\, shipped bydappcore/php-content, which was not a dependency of this package at all. Twenty production files imported the bareMod\Content\*, soBatchContentGeneration::handle()and the MCP content tools would fatal on "Class not found" the moment a worker picked them up.dappcore/php-contentjoinsrequire-dev, matching howdappcore/php-tenantis already carried. The test now runs against a realcontent_taskstable — only that table, because the package migration also recreatespromptswith columns stricter than this package's own.Nothing was deleted. Nothing was adjudicated obsolete.
Three production bugs the phantom classes were hiding
Mocks of a non-existent class accept anything, so these tests asserted nothing:
ProcessContentTask::interpolateVariables()built its placeholder as'{{{'.$key.'}}}'— three braces. Every template writes{{name}}, andPrompt::interpolate()builds the same placeholder correctly as"{{{$key}}}", which is$keybetween two brace pairs. The concatenated form was a mis-transcription of it, so user templates reached the provider verbatim.$result->messageis an undefined property read —EntitlementResultkeeps its text in a readonly$reasonbehindgetMessage(), and has no__get.Feature/Jobs/ProcessContentTaskTestpassed aContentProcessingServiceashandle()'s second argument — a signature this job has never had.Feature/Jobs+Unit/ProcessContentTask: 70 passed, 3 failed (was 43 passed, 27 failed). The 3 are pre-existing and reach a live Qdrant/Elasticsearch.Not in scope
The 156 remaining failures are pre-existing and unrelated to either group; CI keeps
continue-on-error, and its comment is corrected here (it claimed 993/303).The largest remaining cluster (25) is
Core\Mcp\Tools\Concerns\ValidatesDependenciesandCore\Mcp\Dependencies\HasDependencies. Both are real and live indappcore/mcp— deliberately not wired here, because that package claims theCore\Mcp\PSR-4 root this repo also maps ontophp/Mcp/, withCircuitBreaker,ToolRegistryandCircuitOpenExceptionexisting on both sides. That is a namespace collision needing a decision on which copy is canonical, not a blind install. Follow-up work.Two further findings filed separately: the three Livewire components are registered nowhere (no Boot, no route, no menu — they load, but nothing reaches them), and
BatchContentGenerationTest's "does not dispatch any ProcessContentTask when collection is empty" reimplements the branch inline rather than calling the job.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io