test(agentic): cover the AgentDetection surface php was testing for us - #26
Conversation
…n half The reported debt was never the debt. golangci-lint defaults to max-issues-per-linter: 50 and max-same-issues: 3, so every count taken from it here — 101 with --tests=false, 90 without — was a display cap. Uncapped, the same code reports 839. Fixing eleven errcheck findings moved the total by zero, because eleven suppressed ones surfaced to replace them, which is what exposed it. A capped number is not a measurement, so the config now sets both to 0. Policy moves out of a CI argument into .golangci.yml, and --tests=false goes. That flag was doing two harmful things at once. It made `unused` report 21 production symbols as dead because their only callers live in _test.go — syncPull, mcpInitialize, mcpCall, readSSEData, newCoreAgent, pokeCh and the rest, every one a deliberate test-injection seam, since a linter cannot see a caller it has been told not to read. And it hid the dead scaffolding actually inside the test files, which is where all 28 real `unused` findings live. errcheck is excluded for tests instead: the narrower, honest cut, because an unchecked Close() in test setup is noise while an unchecked write-close in production loses data. Two of those production cases were losing data. chathistory's CopyTo and ExportJSONL both closed their destination file with a bare defer, so a close that failed to flush returned nil and reported a complete copy of a truncated file. Both now close explicitly on the success path and report the error, with the defer kept as the net for early returns. The rest are decided per site rather than silenced in bulk. Read cursors and response bodies say so and drop the error. Notifications — Core.ACTION, NotifySession, ResourceUpdated, session.Log — are best-effort by design, and say why: a listener that has gone away must not fail the work that produced the event. Status writes get the opposite treatment and now report through core.Warn, the idiom already used in those files, because the status file is what the monitor polls and a silent failure leaves a workspace looking stuck forever. Same for the runner losing track of a workspace, and for a shutdown that cannot complete. Two signatures I had assumed wrong and the compiler caught: core.WriteString returns a Result, not (int, error), and ServiceShutdown returns a Result too. Remaining, and now countable: 59 production errcheck, 22 staticcheck (three in production, all SA1019 deprecations — session.Log against MCP SEP-2577, and httputil Director against Go 1.26), 28 unused, every one of them dead scaffolding in test files. 109 real findings, against a number that used to read 90 and meant nothing. go build, go vet and go test all pass; gofmt clean. Co-Authored-By: Virgil <virgil@lethean.io>
dappcore/php's Module suite carried an AgentDetectionTest against classes this package owns. It could never pass there — php cannot depend on agent, because agent already depends on php — and it named them under Core\Agentic\Services and Core\Agentic\Support, a namespace the ecosystem has left behind. Wrong twice: unreachable package, stale names. Correcting the import would only have turned missing-class into missing-package. Checked before importing rather than after. Twenty-two of its twenty-five cases duplicate what php/tests/Unit/AgentDetectionTest.php already covers here, and that file is green at 64 passing. Moving the whole thing across would have added duplicate coverage to the repo that owns the code — the tidy-looking version of the same misfiling. Three concepts were genuinely untested here, all of them real public API: AgentIdentity::getReferralPath() AgentIdentity::getProviderDisplayName() and getModelDisplayName() AgentDetection::identify() reading the X-MCP-Token header Only those are adopted, written against this repo's namespaces and API rather than ported. Two cases go beyond what php's file reached, because the branches were sitting there once the methods were under test: a non-agent must get no referral path at all, and an identity with no model must report no model display name. The php-side file is not deleted by this commit — it lives in another repository and that is its owners' call, now that the coverage exists here. Suite 131 failed / 1204 passed, from 131 / 1197 on a main measured today — seven tests added, nothing else moved. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 20 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 selected for processing (16)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Closes agent's side of
dappcore/php'sdocs/misfiled-tests.md— without importing the file.Why it could never pass where it was
php's Module suite carried an
AgentDetectionTestagainst classes this package owns. It could never pass there: php cannot depend on agent, because agent already depends on php. And it named the classes underCore\Agentic\Services/Core\Agentic\Support— a namespace the ecosystem has left behind.Wrong twice: unreachable package, stale names. Correcting the import alone would only have turned missing class into missing package.
Checked before importing, not after
AgentDetectionTest(64 passing)Moving the whole file across would have added duplicate coverage to the repo that owns the code — the tidy-looking version of the same misfiling.
The three gaps are all real public API:
Verified those methods exist and behave as php's assertions expected before writing anything, rather than porting assertions and discovering later.
Written, not ported
Against this repo's real namespaces and API. Two cases go beyond php's file, because the branches were sitting there once the methods were under test:
Not deleted
The php-side file stays. It lives in another repository and that is its owners' call, now that the coverage exists here.
Verification
Baseline measured from a run of
maintoday, not a stored file.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io