fix(gate-14): a COMMENT naming Routes::standard() injects ten phantom AppHost routes — 4 false findings on larpingapp, and scholiq is affected too - #316
Merged
Conversation
…m AppHost routes
The AppHost route-table detector read appinfo/routes.php as raw text:
if [ -f appinfo/routes.php ] \
&& grep -qE 'AppHost\\+Routes::standard[[:space:]]*\(' appinfo/routes.php; then
so a COMMENT mentioning the builder switched the table on, and all ten
canonical route names were appended to the list gate-14 iterates — for an app
that never adopts AppHost.
Measured on larpingapp, whose routes.php is a plain literal array of 20 entries
carrying one accurate comment:
// Canonical AppHost settings write (OpenRegister\AppHost\Routes::standard()).
Comment-stripped, Routes::standard does not occur in that file at all. It was
nevertheless told four routes it does not declare were unreachable, naming two
controllers it does not ship:
[gate-14] route-reachability: FAIL - 4 unrouted method(s) or wrong-target route(s)
lib/Controller/DashboardController.php route='dashboard#catchAll' rule=method-not-found-on-target-controller
lib/Controller/HealthController.php route='health#index' rule=controller-class-not-found
lib/Controller/MetricsController.php route='metrics#index' rule=controller-class-not-found
lib/Controller/SettingsController.php route='settings#load' rule=method-not-found-on-target-controller
THE TWO DETECTORS FOR THE SAME SUBJECT DISAGREED
_HYDRA_APPHOST has always stripped comments through _php_code_only before
grepping for Bootstrap::register. _HYDRA_APPHOST_ROUTE_TABLE did not. On
larpingapp that produced _HYDRA_APPHOST=0 (correct) and
_HYDRA_APPHOST_ROUTE_TABLE=1 (incorrect) on one checkout — visible in the
findings themselves, because _apphost_serves correctly declined to exempt
health/metrics precisely because _HYDRA_APPHOST was 0, which is why the
phantom routes surfaced as findings rather than being waved through.
WHY IT MATTERED MORE THAN AN ORDINARY FALSE POSITIVE
The cheapest way to clear it was to reword or delete an accurate comment — the
same prose-satisfaction #191 and #184 exist to stop, and here the prose was
worth keeping. larpingapp was left red rather than edited.
VERIFIED IN BOTH ARMS, same package, same command, comment text the only
difference in arm 1:
arm 1 larpingapp (comment only, no call)
before [gate-14] route-reachability: FAIL - 4
after [gate-14] route-reachability: PASS
arm 2 doriath (genuinely calls Routes::standard())
before [gate-14] route-reachability: FAIL - 1
after [gate-14] route-reachability: FAIL - 1
Arm 2 is the one that proves this did not simply switch the feature off: the
ten canonical names are still injected for real adopters.
BLAST RADIUS: comment-stripping every appinfo/routes.php under apps-extra that
mentions Routes::standard gives REAL-CALL for decidesk, docudesk, doriath,
openbuild, procest and shillinq, and COMMENT-ONLY for larpingapp and scholiq.
Both of those should be re-measured after this lands.
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
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.
Closes #296.
The defect
The AppHost route-table detector read
appinfo/routes.phpas raw text:So a comment mentioning the builder switched the table on, and all ten canonical route names were appended to the list gate-14 iterates — for an app that never adopts AppHost.
Measured on larpingapp, whose
routes.phpis a plain literal array of 20 entries carrying one accurate comment:// Canonical AppHost settings write (OpenRegister\AppHost\Routes::standard()).Comment-stripped,
Routes::standarddoes not occur in that file at all. It was nevertheless told four routes it does not declare were unreachable, naming two controllers it does not ship:The two detectors for the same subject disagreed
_HYDRA_APPHOSThas always stripped comments through_php_code_onlybefore grepping forBootstrap::register._HYDRA_APPHOST_ROUTE_TABLEdid not. On larpingapp that produced_HYDRA_APPHOST=0(correct) and_HYDRA_APPHOST_ROUTE_TABLE=1(incorrect) on the same checkout.That split is visible in the findings themselves:
_apphost_servescorrectly declined to exempthealth/metricsbecause_HYDRA_APPHOSTwas 0, which is exactly why the phantom routes surfaced as findings instead of being silently waved through.The fix
Route the detection through
_php_code_only, as the sibling detector already does.Verified in both arms
Same package, same command; in arm 1 the only difference is the wording of one comment, with the code byte-identical and
Routes::standardabsent from the compiled text in both cases.FAIL — 4PASSRoutes::standard()FAIL — 1FAIL — 1Arm 2 is the one that proves this did not simply switch the feature off: the ten canonical names are still injected for real adopters, and doriath's genuine finding is untouched.
The full helper suite is green with this change:
passed: 62, quarantined: 2, failed: 0.Why it mattered more than an ordinary false positive
The cheapest way to clear it was to reword or delete an accurate comment — the same prose-satisfaction #191 and #184 exist to stop, and here the prose documents why
settings#updateexists and which dialect it mirrors. larpingapp was left red rather than edited.It also cuts the other way: an app that adopts AppHost through a differently-spelled call gets the ten names injected on no basis at all.
Blast radius
Comment-stripping every
appinfo/routes.phpunderapps-extrathat mentionsRoutes::standard:scholiq is affected too and should be re-measured once this lands — it will be carrying the same phantom
dashboard#catchAll/health#index/metrics#index/settings#loadfindings unless it happens to ship those controllers.Note on a regression test
A test wants both arms: a
routes.phpthat callsRoutes::standard()(ten names injected) and one that only mentions it in a comment (none injected). With only the real-call arm this fix cannot be shown to have changed anything. The detection is inline inrun-hydra-gates.shrather than in ascripts/lib/helper, so it has no auto-discovered suite to extend; the two-arm measurement above was run end-to-end through the runner against real repositories instead. Happy to add a fixture pair if you would rather have it pinned intests/.