Skip to content

Fix: canSee() narrowing was inert, replace with a working mechanism - #13

Merged
alies-dev merged 3 commits into
mainfrom
alies-dev/fix-6-cansee-mechanism
Sep 2, 2026
Merged

Fix: canSee() narrowing was inert, replace with a working mechanism#13
alies-dev merged 3 commits into
mainfrom
alies-dev/fix-6-cansee-mechanism

Conversation

@alies-dev

@alies-dev alies-dev commented Sep 2, 2026

Copy link
Copy Markdown
Member

Context

#12 shipped four stub-based fixes for false-positive InvalidArgument/ArgumentTypeCoercion on Nova field callbacks. A post-merge review (checked against real laravel/nova 5.10.1, not just this repo's own fixtures) found that fix 3 — narrowing canSee() to NovaRequest on FieldElement — never actually worked. canSee() is declared only on the AuthorizedToSee trait, which FieldElement merely inherits (via Element); a Psalm plugin stub can override a method the stubbed class itself declares, or add a genuinely new one, but not override a method the class only inherits from a used trait. I confirmed this empirically against real Nova: the original false positive was still reproducible after the "fix" shipped, whether the override sat on FieldElement.phpstub or on Element.phpstub (the class that actually uses the trait).

The review's suggested alternative, MethodParamsProviderInterface, is also a dead end here, for the same reason it was already ruled out for the resource-model-generic problem: Psalm keys it by the exact called class with no hierarchy walk, so it would need to enumerate every concrete Field subclass — impossible for an open, user-extensible hierarchy.

The review also caught why this slipped through: the test fixture registered fake Nova via psalm.xml's <stubs> config, and config-declared stubs are scanned before a plugin's own addStubFile() stubs, so the plugin's stub always "wins" there — regardless of whether its override mechanism would actually work against normally-scanned source. AuthorizedToSee.phpstub itself turned out to be a pure no-op too: it redeclared canSee() with the exact type Nova already has.

Solution

Traced the actual resolution path (Methods::getDeclaringMethodId() reads declaring_method_ids['cansee'] off the called class's own storage) and built NovaFieldAuthorizationHandler, a post-populate hook that walks every class extending FieldElement and points its own declaring_method_ids/methods['cansee'] at a narrowed clone of whatever AuthorizedToSee::canSee() currently declares — the same storage fields a real public function canSee(...) override would populate, just built programmatically instead of textually. This is the same category of fix NovaResourceQueryMethodHandler already uses elsewhere in this plugin for "narrow because we know more than the vendor signature admits" cases. Tool/Dashboard/Filters\Filter/Menu\* share the same trait but are never touched, so their canSee() stays wide — verified against real Nova in both directions (a Tool::canSee(fn(Request):bool) closure still passes, a Tool::canSee(fn(NovaRequest):bool) closure is still rejected). AuthorizedToSee.phpstub is deleted.

Closed the fixture gap that masked this: the fake Nova/Illuminate classes now reach Psalm only through the composer autoload-dev classmap, exactly as a real Nova app's vendor/ would, instead of psalm.xml's <stubs> section. Running the existing test suite against this change alone (before writing the real fix) reproduced the exact failure the external review found — confirming the bug was real and this fixture change is what actually catches it.

While in AcceptanceTest.php: re-keyed the issue assertions on selected_text instead of line number (a leading comment shifting every later line number was a standing footgun for this file), moved stderr from a second pipe to a temp file (draining two live pipes sequentially can deadlock if the undrained one fills), and asserted the exit code against Psalm's actual contract (0 = clean, 2 = issues found — IssueBuffer::finish()) instead of discarding it.

Two smaller findings from the same review, addressed rather than reverted: Filterable's Builder|Relation template accepts a closure typed against only one shape even for a field reachable via both request kinds (Nova passes a Relation for relationship-index requests) — now documented as an explicit trade-off in the stub and README, since the shared contract type remains available for a field that needs to be safe against both. FieldElement's bounded template is also stricter than Nova's own docblocks for setters with no upstream @phpstan-param (hideFromIndex and friends were bare mixed before this plugin existed) — documented, with a fixture regression case.

A second review pass on the mechanism itself (before merging) found two more real issues, both fixed: the narrowing guard only checked whether canSee()'s declaring class was the leaf class being visited, so a user's own canSee() override on an intermediate base class would have been silently clobbered — verified against real Nova that declaring_method_ids['cansee'] resolves straight to Laravel\Nova\AuthorizedToSee for the whole clean hierarchy, so the guard now checks against that exact class instead, with a fixture regression case covering a custom base-class override. Separately, AcceptanceTest.php's stderr temp file could leak if proc_open() failed before the cleanup line; now wrapped in try/finally.

https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R

External review (post-merge) found that the merged PR's fix 3 was dead
code: canSee() is declared only on the AuthorizedToSee trait, which
FieldElement merely inherits (via Element), and a Psalm plugin stub
cannot override a method the stubbed class only inherits from a used
trait. Confirmed empirically against real Nova 5.10.1 — the original
false positive was still reproducible on canSee() after the "fix"
shipped, and MethodParamsProviderInterface (the review's suggested
alternative) is also a dead end for the same open-hierarchy reason
already established for the resource-model-generic problem.

The programmatic mechanism this repo already uses elsewhere for
"narrow because we know more than the vendor signature admits" cases
does work: NovaFieldAuthorizationHandler walks every FieldElement
descendant post-populate and points its own declaring_method_ids at a
narrowed clone of canSee(), the same storage fields
Methods::getDeclaringMethodId() actually reads. Tool/Dashboard/
Filters\Filter/Menu\* share the same trait but are never touched, so
their canSee() stays wide. AuthorizedToSee.phpstub is deleted — it
redeclared canSee() with the exact type Nova already has, a no-op that
happened to survive review because it changed nothing to break.

Also fixes the two masking factors that let this ship in the first
place: the test fixture registered fake Nova via psalm.xml's <stubs>
config, where config stubs are scanned before a plugin's own
addStubFile() stubs and always "win" regardless of whether the
override mechanism actually works against normally-scanned code. Fake
Nova now reaches Psalm only through the composer autoload-dev
classmap, exactly as a real Nova app's vendor/ would, so the suite
exercises the plugin's actual registration path. AcceptanceTest.php's
issue assertions are now keyed on selected_text instead of line
number (a leading comment shifting every later line number was a
standing footgun), stderr is captured to a temp file instead of a
second pipe (sequential pipe draining can deadlock), and the exit code
is asserted against Psalm's real {0 = clean, 2 = issues found} rather
than being silently discarded.

Two smaller findings from the same review, addressed in place:
Filterable's Builder|Relation template accepts a closure typed against
only one shape even for a field reachable via both request kinds —
documented as an explicit trade-off in the stub and README rather than
reverted, since the shared contract type remains available for a field
that needs both-safe. FieldElement's bounded template is also stricter
than Nova's own docblocks for setters with no upstream @phpstan-param
(hideFromIndex et al.) — documented, with a fixture regression case.

Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
@alies-dev alies-dev self-assigned this Sep 2, 2026
External review (ChatGPT, xhigh) on the canSee mechanism fix found
two real issues:

The narrowing guard only checked whether canSee()'s declaring class
equals the leaf class being visited, so a user's own canSee() override
declared on an intermediate base class (ChildField extends
CustomBaseField, where CustomBaseField declares its own canSee())
would get silently clobbered with Nova's narrow signature. Verified
against real Nova that declaring_method_ids['cansee'] resolves
straight to Laravel\Nova\AuthorizedToSee for the whole clean Field
hierarchy — the guard now checks against that exact class instead,
so only Nova's own trait method is ever narrowed. Added a fixture
regression case with a custom base class overriding canSee().

The stderr temp file in AcceptanceTest.php was created before
proc_open(), but only unlinked after the assertions that could
throw first — wrapped in try/finally.

Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
@alies-dev
alies-dev marked this pull request as ready for review September 2, 2026 14:27
@alies-dev
alies-dev merged commit 1c16734 into main Sep 2, 2026
6 checks passed
@alies-dev
alies-dev deleted the alies-dev/fix-6-cansee-mechanism branch September 2, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant