fix(gate-57): the #[McpTool] attribute-scanner seam is a real registration (#200) - #215
Merged
Merged
Conversation
…ation (#200) gate-57 reported `pipelinq LeadService::createLead` — a curated, spec'd MCP write tool — as an orphaned write capability. Acting on that verdict would have DELETED live code, which is the failure mode this module's own docstring already records from hydra#106 and the reason it says false positives are the primary risk of this gate. `createLead` has no syntactic `->createLead(` call site and never will. It is reached by attribute reflection (ADR-063): an `#[McpTool]` attribute on the method, the class listed by an `IMcpScannableServices` implementation, and a `registerServiceAlias('OCA\OpenRegister\Mcp\IMcpScannableServices::pipelinq', PipelinqScannableServices::class)` binding in Application.php. OpenRegister's AttributeToolScanner reflects the class and invokes the method. The caller index cannot see any of that. THE SEAM REQUIRES ALL THREE PIECES, mirroring what the register.d-handler and event-listener seams already demand, and it is applied PER METHOD: * a bare `#[McpTool]` on a class nobody registers stays RED * a registered class's write methods that carry no attribute stay RED * an alias naming a class that does not implement the interface grants nothing * an implementation nobody aliases grants nothing `LeadService::createInvoice` in the fixture is precisely that control: same class, same scannable registration, no attribute, still reported. THE gate-64 SHAPE, CHECKED AS ASKED, AND PRESENT gate-64's `has_prelude()` grepped a quoted string literal, so it missed every constant and matched inside comments — failing in both directions at once. The attribute seam would have had the comment half of that for free: pipelinq's real LeadService.php opens with * Both public entry points are annotated `#[McpTool]` (OpenRegister ADR-063 on line 7, seven lines above anything executable. A raw-text search matches that sentence. So the seam reads a comment-blanked copy of the file, and the walk-up from the method declaration only ever sees executable lines. `_blank_php_comments()` preserves length and line numbers, is string-aware, and knows that `#` opens a line comment in PHP while `#[` opens an ATTRIBUTE — blanking those would delete the thing being looked for. The two PRE-EXISTING seams had the same latent shape: a commented-out `registerEventListener(...)` or a `<!-- <job>…</job> -->` exempts a WHOLE class, which is a false GREEN. Both now strip comments first. MEASURED before changing: a sweep of the eight repos under this gate found ZERO comment-only matches, so this changes no verdict today — it closes the hole rather than reacting to it. Both directions are tested: a commented registration must not exempt, a live one must still exempt. THE COUNT-AS-EXIT-STATUS BUG (#209) IS NOT PRESENT HERE This helper prints one line per finding and always returns 0; the bash gate counts the printed lines. There is no count anywhere near the exit status. Asserted explicitly now so it cannot drift into one. MEASURED, OLD vs NEW, over lib/Service/** in the eight repos at origin/development: openconnector 1 -> 1 scholiq 0 -> 0 pipelinq 5 -> 4 nldesign 0 -> 0 shillinq 19 -> 19 softwarecatalog 0 -> 0 launchpad 2 -> 2 portaliq 0 -> 0 Exactly one finding removed — the documented false positive — and every other line byte-identical. The gate did not go quiet. 22 new tests alongside the suite's existing 21 (43 total, all green). Ten mutations of the changed predicates were each killed: the seam exempting the whole class, the seam removed, comment blanking disabled, the implements-interface requirement dropped, the attribute boundary reverted to the word-based form that `#[McpTool(handler: Foo::class)]` defeats, the alias requirement dropped, the attribute searched file-wide, the interface name blanked so any alias counts, the XML comment strip removed, and `#[` treated as a comment. Also records in the module docstring what the issue's second point asks for: a zero-caller write capability has TWO remedies, and the finding text only suggests one. softwarecatalog's `publishEntryForFederation()` had zero callers while `PublicationService::publish()` served the same capability through a live route — wiring the orphan would have duplicated a live endpoint and widened the auth surface. Deletion was correct (softwarecatalog#447). Closes #200
rubenvdlinde
added a commit
that referenced
this pull request
Aug 9, 2026
…could not see the defect they exist to catch Acceptance test applied to all nine gates in the 56-64 band: plant one textbook true positive in a real fleet repo, require the gate to FAIL and NAME it, remove the plant, require the prior verdict back, and require a clean fixture to still pass. Five gates passed unchanged (56, 58, 60, 62, 63). Four did not. AN UNOPENED SCOPE IS NOT A PASS — gates 56, 57, 58, 59, 60, 61 -------------------------------------------------------------- #242/#240 established that a gate must not report PASS over a scope it never opened, and #268 that an empty ADR-020 scope is `na` rather than `structural`. Both were applied to gates 19, 25, 62 and 63 and to nothing else. Measured on shillinq, one docs-only commit, --scope-to-diff: [gate-56] register-handler-resolution: PASS <- 153 registers, 0 opened [gate-57] orphaned-write-capability: PASS <- 316 services, 0 opened [gate-58] e2e-networkidle: PASS <- 60 e2e files, 0 opened [gate-59] unclosable-gate: PASS <- lib/ untouched [gate-60] icon-vocabulary: PASS <- no manifest in the diff [gate-61] listener-work-placement: PASS <- all 15 out of scope [gate-62] store-plane: NOT APPLICABLE (already fixed) [gate-63] settings-surface: NOT APPLICABLE (already fixed) Six gates asserting a verdict about code the run had not looked at, beside two that had already learned not to — and --require-full-coverage cannot see a PASS, so nothing reported that six gates had gone quiet. All six now emit `na` with a reason naming the rule and the count of subjects that exist but were not inspected. GATE 59 COULD NOT RUN A FULL-TREE AUDIT AT ALL ---------------------------------------------- CHANGED_FILES is populated only under --scope-to-diff. Gate 59's guard read `grep -qE '^lib/.*\.php$'` against it unconditionally, so on every unscoped run the guard was false and the gate printed PASS having walked no PHP. That is #240's sentence — "a full-tree audit was the one mode this gate could never reach" — in a gate #240 did not visit. The scoping now applies only when the caller asked for it. Gate 61's unconditional --base is DELIBERATE by contrast (it is about new debt; the fleet's 149-registration backlog is a work-list). Switching it to --all was tried and reverted: the builder runs unscoped, so it would have surfaced the whole backlog as blocking findings on every build. Recorded in the code so the next reader does not repeat it. GATE 59 WAS WRONG IN BOTH DIRECTIONS AT ONCE (#184's shape) ----------------------------------------------------------- It caught the textbook case and then failed on the next four mutations, each reproduced against docudesk: false GREEN a COMMENTED-OUT setter counted as a write. `// TODO: setValueString('app','configuration_version',$v)` closed the finding — and that is the single most likely comment to sit beside a key nobody writes. The gate whose subject is "this guard never closes" was itself closed by a comment promising to close a guard. false GREEN `"key"` in double quotes was invisible on both sides. false POSITIVE read 'key' / write "key" — code that closes its gate correctly reported as never closing it, with no remedy available to the app but changing its quote style. false GREEN a key held in a class constant was invisible on both sides — and a constant is the idiomatic way to write a key used twice, which is the shape a CLOSABLE gate has. Fixed with the established remedies: source_scope.php_mask for the comment regions (offsets preserved; string contents kept, because the key literal is the evidence), both quote styles, and constant resolution that keeps read and write symmetric. The suppression keeps reading raw text because it is authored as a comment — and it now requires the key QUOTED ON THE MARKER'S OWN LINE, because the old four-line window spanned the read and so suppressed whatever was near it rather than what it named. Fleet sweep: 21 repos, zero verdict changes. GATE 57'S MCP SEAM DISSOLVED ON A LEADING BACKSLASH --------------------------------------------------- Both halves of the attribute seam (#200/#215) matched a namespace prefix as `(?:[A-Za-z_]\w*\s*\\+\s*)*` — every segment had to start with a letter. So the fully-qualified spelling, which is how you write a name with no `use` import for it, matched neither: registerServiceAlias('…::app', \OCA\App\Mcp\Impl::class) #[\OCA\OpenRegister\Mcp\Attribute\McpTool(name: 'createLead')] Either miss alone empties the seam and puts every #[McpTool] write method in the app back on the finding list — which is #200 verbatim, a finding whose only remedy is deleting a live, curated MCP write tool. Reproduced on shillinq with the two spellings side by side. The regression test's mutant restores BOTH pre-fix patterns together, because the seam needs both and reverting one alone reads as "the fix changed nothing". Fleet sweep: 21 repos, zero verdict changes — the trap is latent, not live. GATE 64 SAW ONE OF PHP'S THREE WAYS TO NAME A CLASS --------------------------------------------------- Rule 2 required a QUOTED literal, so of these four only the first was a finding; the other three were injected into larpingapp's register() and the gate reported OK for every one: class_exists('OCA\OpenRegister\AppHost\…\GenericHealthController') class_exists(\OCA\OpenRegister\AppHost\…\GenericHealthController::class) use …\GenericHealthController; class_exists(GenericHealthController::class) const AH = 'OCA\OpenRegister\AppHost\…'; class_exists(self::AH) That is #184's lesson in the file where it was learned. All four spellings now resolve through one resolver, so a spelling that escapes it escapes both rules rather than whichever was written second. A bare `use` import stays clean on purpose — `use` is a compile-time alias and does not autoload; flagging it would newly redden four repos for code that works. AND A GREEN GATE-64 WAS NOT EVIDENCE ABOUT THE WIDER PROBE ---------------------------------------------------------- The hard rule is scoped to OCA\OpenRegister\AppHost\, but the autoloader mechanism has nothing to do with AppHost: during register() the whole OCA\OpenRegister\ prefix is absent for any app sorting earlier, so ANY class_exists() on it answers FALSE and everything it guards silently never happens. Measured across apps-extra, no prelude present: larpingapp 3 Event\{DeepLinkRegistration,ObjectCreating,ObjectUpdating} — the last two carry larpingapp's server-authoritative skill-requirement / XP-budget enforcement on character writes, which therefore never registers hermiq 3 flow-node, leaf-provider and shareable-config registration nldesign 1 shareable-config registration Reported as a non-blocking NOTE, printed by the runner, and deliberately not a FAIL: this gate is not diff-scoped, so failing it would block every PR in three repos on code the PR did not touch — the trap check_store_and_settings_surface.py already records. A probe in boot() is NOT noted, because boot() runs after every app has registered. ALSO ---- Gate 64's log path was a hardcoded /tmp/hydra-gate-apphost-autoload-prelude .log — the shared-path non-determinism HYDRA_GATE_LOG_DIR exists to remove, left behind in one gate. TESTS ----- test_check_unclosable_gate.py NEW — gate 59 shipped with no helper suite at all; 23 cases, every arm of the matrix above plus the mutant that proves the comment mask is load-bearing test_gate_5661_empty_scope_is_not_a_pass.sh NEW — 28 assertions over gates 56-61: clean in-scope subject PASSes, empty scope is `na` with a reason and exit 0 under --require-full-coverage, and a planted true positive per gate still FAILs. Against the pre-fix package it reports 13 failures, naming the six PASSes test_check_apphost_autoload_prelude.py +14 cases test_check_orphaned_write_capability.py +5 cases test_check_listener_placement.py EXIT_EMPTY_SCOPE 51 discovered helper suites green (2 quarantined, unchanged). Full-tree run on shillinq before/after: verdicts byte-identical for all nine gates.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 9, 2026
…could not see the defect they exist to catch (#278) * fix(gates 56-64): six gates passed over an unopened scope, and three could not see the defect they exist to catch Acceptance test applied to all nine gates in the 56-64 band: plant one textbook true positive in a real fleet repo, require the gate to FAIL and NAME it, remove the plant, require the prior verdict back, and require a clean fixture to still pass. Five gates passed unchanged (56, 58, 60, 62, 63). Four did not. AN UNOPENED SCOPE IS NOT A PASS — gates 56, 57, 58, 59, 60, 61 -------------------------------------------------------------- #242/#240 established that a gate must not report PASS over a scope it never opened, and #268 that an empty ADR-020 scope is `na` rather than `structural`. Both were applied to gates 19, 25, 62 and 63 and to nothing else. Measured on shillinq, one docs-only commit, --scope-to-diff: [gate-56] register-handler-resolution: PASS <- 153 registers, 0 opened [gate-57] orphaned-write-capability: PASS <- 316 services, 0 opened [gate-58] e2e-networkidle: PASS <- 60 e2e files, 0 opened [gate-59] unclosable-gate: PASS <- lib/ untouched [gate-60] icon-vocabulary: PASS <- no manifest in the diff [gate-61] listener-work-placement: PASS <- all 15 out of scope [gate-62] store-plane: NOT APPLICABLE (already fixed) [gate-63] settings-surface: NOT APPLICABLE (already fixed) Six gates asserting a verdict about code the run had not looked at, beside two that had already learned not to — and --require-full-coverage cannot see a PASS, so nothing reported that six gates had gone quiet. All six now emit `na` with a reason naming the rule and the count of subjects that exist but were not inspected. GATE 59 COULD NOT RUN A FULL-TREE AUDIT AT ALL ---------------------------------------------- CHANGED_FILES is populated only under --scope-to-diff. Gate 59's guard read `grep -qE '^lib/.*\.php$'` against it unconditionally, so on every unscoped run the guard was false and the gate printed PASS having walked no PHP. That is #240's sentence — "a full-tree audit was the one mode this gate could never reach" — in a gate #240 did not visit. The scoping now applies only when the caller asked for it. Gate 61's unconditional --base is DELIBERATE by contrast (it is about new debt; the fleet's 149-registration backlog is a work-list). Switching it to --all was tried and reverted: the builder runs unscoped, so it would have surfaced the whole backlog as blocking findings on every build. Recorded in the code so the next reader does not repeat it. GATE 59 WAS WRONG IN BOTH DIRECTIONS AT ONCE (#184's shape) ----------------------------------------------------------- It caught the textbook case and then failed on the next four mutations, each reproduced against docudesk: false GREEN a COMMENTED-OUT setter counted as a write. `// TODO: setValueString('app','configuration_version',$v)` closed the finding — and that is the single most likely comment to sit beside a key nobody writes. The gate whose subject is "this guard never closes" was itself closed by a comment promising to close a guard. false GREEN `"key"` in double quotes was invisible on both sides. false POSITIVE read 'key' / write "key" — code that closes its gate correctly reported as never closing it, with no remedy available to the app but changing its quote style. false GREEN a key held in a class constant was invisible on both sides — and a constant is the idiomatic way to write a key used twice, which is the shape a CLOSABLE gate has. Fixed with the established remedies: source_scope.php_mask for the comment regions (offsets preserved; string contents kept, because the key literal is the evidence), both quote styles, and constant resolution that keeps read and write symmetric. The suppression keeps reading raw text because it is authored as a comment — and it now requires the key QUOTED ON THE MARKER'S OWN LINE, because the old four-line window spanned the read and so suppressed whatever was near it rather than what it named. Fleet sweep: 21 repos, zero verdict changes. GATE 57'S MCP SEAM DISSOLVED ON A LEADING BACKSLASH --------------------------------------------------- Both halves of the attribute seam (#200/#215) matched a namespace prefix as `(?:[A-Za-z_]\w*\s*\\+\s*)*` — every segment had to start with a letter. So the fully-qualified spelling, which is how you write a name with no `use` import for it, matched neither: registerServiceAlias('…::app', \OCA\App\Mcp\Impl::class) #[\OCA\OpenRegister\Mcp\Attribute\McpTool(name: 'createLead')] Either miss alone empties the seam and puts every #[McpTool] write method in the app back on the finding list — which is #200 verbatim, a finding whose only remedy is deleting a live, curated MCP write tool. Reproduced on shillinq with the two spellings side by side. The regression test's mutant restores BOTH pre-fix patterns together, because the seam needs both and reverting one alone reads as "the fix changed nothing". Fleet sweep: 21 repos, zero verdict changes — the trap is latent, not live. GATE 64 SAW ONE OF PHP'S THREE WAYS TO NAME A CLASS --------------------------------------------------- Rule 2 required a QUOTED literal, so of these four only the first was a finding; the other three were injected into larpingapp's register() and the gate reported OK for every one: class_exists('OCA\OpenRegister\AppHost\…\GenericHealthController') class_exists(\OCA\OpenRegister\AppHost\…\GenericHealthController::class) use …\GenericHealthController; class_exists(GenericHealthController::class) const AH = 'OCA\OpenRegister\AppHost\…'; class_exists(self::AH) That is #184's lesson in the file where it was learned. All four spellings now resolve through one resolver, so a spelling that escapes it escapes both rules rather than whichever was written second. A bare `use` import stays clean on purpose — `use` is a compile-time alias and does not autoload; flagging it would newly redden four repos for code that works. AND A GREEN GATE-64 WAS NOT EVIDENCE ABOUT THE WIDER PROBE ---------------------------------------------------------- The hard rule is scoped to OCA\OpenRegister\AppHost\, but the autoloader mechanism has nothing to do with AppHost: during register() the whole OCA\OpenRegister\ prefix is absent for any app sorting earlier, so ANY class_exists() on it answers FALSE and everything it guards silently never happens. Measured across apps-extra, no prelude present: larpingapp 3 Event\{DeepLinkRegistration,ObjectCreating,ObjectUpdating} — the last two carry larpingapp's server-authoritative skill-requirement / XP-budget enforcement on character writes, which therefore never registers hermiq 3 flow-node, leaf-provider and shareable-config registration nldesign 1 shareable-config registration Reported as a non-blocking NOTE, printed by the runner, and deliberately not a FAIL: this gate is not diff-scoped, so failing it would block every PR in three repos on code the PR did not touch — the trap check_store_and_settings_surface.py already records. A probe in boot() is NOT noted, because boot() runs after every app has registered. ALSO ---- Gate 64's log path was a hardcoded /tmp/hydra-gate-apphost-autoload-prelude .log — the shared-path non-determinism HYDRA_GATE_LOG_DIR exists to remove, left behind in one gate. TESTS ----- test_check_unclosable_gate.py NEW — gate 59 shipped with no helper suite at all; 23 cases, every arm of the matrix above plus the mutant that proves the comment mask is load-bearing test_gate_5661_empty_scope_is_not_a_pass.sh NEW — 28 assertions over gates 56-61: clean in-scope subject PASSes, empty scope is `na` with a reason and exit 0 under --require-full-coverage, and a planted true positive per gate still FAILs. Against the pre-fix package it reports 13 failures, naming the six PASSes test_check_apphost_autoload_prelude.py +14 cases test_check_orphaned_write_capability.py +5 cases test_check_listener_placement.py EXIT_EMPTY_SCOPE 51 discovered helper suites green (2 quarantined, unchanged). Full-tree run on shillinq before/after: verdicts byte-identical for all nine gates. * fix(gates 56-64): six gates passed over an unopened scope, and three could not see the defect they exist to catch Acceptance test applied to all nine gates in the 56-64 band: plant one textbook true positive in a real fleet repo, require the gate to FAIL and NAME it, remove the plant, require the prior verdict back, and require a clean fixture to still pass. Five gates passed unchanged (56, 58, 60, 62, 63). Four did not. AN UNOPENED SCOPE IS NOT A PASS — gates 56, 57, 58, 59, 60, 61 -------------------------------------------------------------- #242/#240 established that a gate must not report PASS over a scope it never opened, and #268 that an empty ADR-020 scope is `na` rather than `structural`. Both were applied to gates 19, 25, 62 and 63 and to nothing else. Measured on shillinq, one docs-only commit, --scope-to-diff: [gate-56] register-handler-resolution: PASS <- 153 registers, 0 opened [gate-57] orphaned-write-capability: PASS <- 316 services, 0 opened [gate-58] e2e-networkidle: PASS <- 60 e2e files, 0 opened [gate-59] unclosable-gate: PASS <- lib/ untouched [gate-60] icon-vocabulary: PASS <- no manifest in the diff [gate-61] listener-work-placement: PASS <- all 15 out of scope [gate-62] store-plane: NOT APPLICABLE (already fixed) [gate-63] settings-surface: NOT APPLICABLE (already fixed) Six gates asserting a verdict about code the run had not looked at, beside two that had already learned not to — and --require-full-coverage cannot see a PASS, so nothing reported that six gates had gone quiet. All six now emit `na` with a reason naming the rule and the count of subjects that exist but were not inspected. GATE 59 COULD NOT RUN A FULL-TREE AUDIT AT ALL ---------------------------------------------- CHANGED_FILES is populated only under --scope-to-diff. Gate 59's guard read `grep -qE '^lib/.*\.php$'` against it unconditionally, so on every unscoped run the guard was false and the gate printed PASS having walked no PHP. That is #240's sentence — "a full-tree audit was the one mode this gate could never reach" — in a gate #240 did not visit. The scoping now applies only when the caller asked for it. Gate 61's unconditional --base is DELIBERATE by contrast (it is about new debt; the fleet's 149-registration backlog is a work-list). Switching it to --all was tried and reverted: the builder runs unscoped, so it would have surfaced the whole backlog as blocking findings on every build. Recorded in the code so the next reader does not repeat it. GATE 59 WAS WRONG IN BOTH DIRECTIONS AT ONCE (#184's shape) ----------------------------------------------------------- It caught the textbook case and then failed on the next four mutations, each reproduced against docudesk: false GREEN a COMMENTED-OUT setter counted as a write. `// TODO: setValueString('app','configuration_version',$v)` closed the finding — and that is the single most likely comment to sit beside a key nobody writes. The gate whose subject is "this guard never closes" was itself closed by a comment promising to close a guard. false GREEN `"key"` in double quotes was invisible on both sides. false POSITIVE read 'key' / write "key" — code that closes its gate correctly reported as never closing it, with no remedy available to the app but changing its quote style. false GREEN a key held in a class constant was invisible on both sides — and a constant is the idiomatic way to write a key used twice, which is the shape a CLOSABLE gate has. Fixed with the established remedies: source_scope.php_mask for the comment regions (offsets preserved; string contents kept, because the key literal is the evidence), both quote styles, and constant resolution that keeps read and write symmetric. The suppression keeps reading raw text because it is authored as a comment — and it now requires the key QUOTED ON THE MARKER'S OWN LINE, because the old four-line window spanned the read and so suppressed whatever was near it rather than what it named. Fleet sweep: 21 repos, zero verdict changes. GATE 57'S MCP SEAM DISSOLVED ON A LEADING BACKSLASH --------------------------------------------------- Both halves of the attribute seam (#200/#215) matched a namespace prefix as `(?:[A-Za-z_]\w*\s*\\+\s*)*` — every segment had to start with a letter. So the fully-qualified spelling, which is how you write a name with no `use` import for it, matched neither: registerServiceAlias('…::app', \OCA\App\Mcp\Impl::class) #[\OCA\OpenRegister\Mcp\Attribute\McpTool(name: 'createLead')] Either miss alone empties the seam and puts every #[McpTool] write method in the app back on the finding list — which is #200 verbatim, a finding whose only remedy is deleting a live, curated MCP write tool. Reproduced on shillinq with the two spellings side by side. The regression test's mutant restores BOTH pre-fix patterns together, because the seam needs both and reverting one alone reads as "the fix changed nothing". Fleet sweep: 21 repos, zero verdict changes — the trap is latent, not live. GATE 64 SAW ONE OF PHP'S THREE WAYS TO NAME A CLASS --------------------------------------------------- Rule 2 required a QUOTED literal, so of these four only the first was a finding; the other three were injected into larpingapp's register() and the gate reported OK for every one: class_exists('OCA\OpenRegister\AppHost\…\GenericHealthController') class_exists(\OCA\OpenRegister\AppHost\…\GenericHealthController::class) use …\GenericHealthController; class_exists(GenericHealthController::class) const AH = 'OCA\OpenRegister\AppHost\…'; class_exists(self::AH) That is #184's lesson in the file where it was learned. All four spellings now resolve through one resolver, so a spelling that escapes it escapes both rules rather than whichever was written second. A bare `use` import stays clean on purpose — `use` is a compile-time alias and does not autoload; flagging it would newly redden four repos for code that works. AND A GREEN GATE-64 WAS NOT EVIDENCE ABOUT THE WIDER PROBE ---------------------------------------------------------- The hard rule is scoped to OCA\OpenRegister\AppHost\, but the autoloader mechanism has nothing to do with AppHost: during register() the whole OCA\OpenRegister\ prefix is absent for any app sorting earlier, so ANY class_exists() on it answers FALSE and everything it guards silently never happens. Measured across apps-extra, no prelude present: larpingapp 3 Event\{DeepLinkRegistration,ObjectCreating,ObjectUpdating} — the last two carry larpingapp's server-authoritative skill-requirement / XP-budget enforcement on character writes, which therefore never registers hermiq 3 flow-node, leaf-provider and shareable-config registration nldesign 1 shareable-config registration Reported as a non-blocking NOTE, printed by the runner, and deliberately not a FAIL: this gate is not diff-scoped, so failing it would block every PR in three repos on code the PR did not touch — the trap check_store_and_settings_surface.py already records. A probe in boot() is NOT noted, because boot() runs after every app has registered. ALSO ---- Gate 64's log path was a hardcoded /tmp/hydra-gate-apphost-autoload-prelude .log — the shared-path non-determinism HYDRA_GATE_LOG_DIR exists to remove, left behind in one gate. TESTS ----- test_check_unclosable_gate.py NEW — gate 59 shipped with no helper suite at all; 23 cases, every arm of the matrix above plus the mutant that proves the comment mask is load-bearing test_gate_5661_empty_scope_is_not_a_pass.sh NEW — 28 assertions over gates 56-61: clean in-scope subject PASSes, empty scope is `na` with a reason and exit 0 under --require-full-coverage, and a planted true positive per gate still FAILs. Against the pre-fix package it reports 13 failures, naming the six PASSes test_check_apphost_autoload_prelude.py +14 cases test_check_orphaned_write_capability.py +5 cases test_check_listener_placement.py EXIT_EMPTY_SCOPE 51 discovered helper suites green (2 quarantined, unchanged). Full-tree run on shillinq before/after: verdicts byte-identical for all nine gates. * fix(gates 56,57,63,64): a crashed checker read as a clean tree, a real class read as absent, and a lazy closure read as an eager one Second pass over the 56-64 band, planting in a SECOND repo of a different shape per gate. Four more defects, all measured against gate package 48c88ba. A CRASHED CHECKER REPORTED PASS — gates 56 and 57 -------------------------------------------------- Both invoked their helper as `>> log 2>/dev/null || true` and then derived the verdict from `wc -l` on the log. Stderr discarded, exit status discarded, empty log — so a checker that never started reported PASS. Measured on shillinq with a python3 shim that exits 1 for exactly these two helpers: [gate-56] register-handler-resolution: PASS <- 153 registers [gate-57] orphaned-write-capability: PASS <- 316 services and gate-57 had reported 20 real findings over that same tree on the previous run. That is gate-40's defect verbatim. Both helpers ALWAYS exit 0 when they run, by design (#209 — the count goes to stdout, never into the exit byte), so a non-zero exit can only be a crash and never a finding count. Both now emit SKIPPED (wiring) and keep the stderr on disk. Every gate in this band was re-checked for the count-as-exit-status defect found in gates 19, 26 and 52: none of the nine has it. 56 and 57 put the count on stdout and the runner counts lines; 58/59/60/61/62/63/64 return status codes only. A REAL CLASS READ AS ABSENT — gate 56 -------------------------------------- The PSR-4 path guess handles the conventional layout; the fallback walk over lib/ is what finds a type living where PSR-4 does NOT predict — a DI-bound registration, a type not named after its file. That is the shape gate-30 was caught mis-resolving (`AppHost\Controller\GenericHealth` PSR-4-maps to lib/Controller/AppHost/Controller/… while openregister DI-binds it to lib/AppHost/Controller/). The walk matched `class` only, with at most ONE modifier. So every other declaration form was a FALSE POSITIVE on a type that genuinely exists — and the action `guard-class-not-found` invites is to write the class a second time. Measured, each against a real declaration at a non-conventional path: enum ProbeState: string { … } -> guard-class-not-found interface ProbeContract { … } -> guard-class-not-found trait ProbeTrait { … } -> guard-class-not-found final readonly class ReadonlyProbe -> guard-class-not-found `final readonly` is ordinary PHP 8.2. Only the DECLARATION FORMS widen; the line anchor that keeps docblock prose out is unchanged and asserted in both directions. A LAZY CLOSURE READ AS AN EAGER REFERENCE — gate 64 ---------------------------------------------------- This module's header has always said lazy service closures that merely MENTION an AppHost class are deliberately NOT flagged, because their bodies run at resolution time. Both rules ran over the whole file, so they did not. Measured on launchpad — deliberately a DIFFERENT repo shape from larpingapp. launchpad's whole composition root resolves OpenRegister lazily inside closures (it already does this for AppHost\Observability\ManifestLoader); that is the documented leaf pattern and the reason launchpad is green. A closure body naming Bootstrap reported byte-identically to an eager `Bootstrap::register($context, …)`. The gate would have failed the one repo doing it correctly, for doing it correctly, and the only remedy is to stop writing the lazy form. Anonymous and arrow-function bodies are now blanked before both rules. Named methods are untouched (`public function register(` has an identifier between `function` and `(`), and an eager reference AFTER or BETWEEN closures is still caught — both asserted. GATE 63 ON A CONTROLLER-ONLY DIFF — the reason was an overclaim --------------------------------------------------------------- Every BLOCKING rule in this gate reads src/manifest.json, src/manifest.d/ or src/menu-layout.json. The two rules that touch lib/Settings are WARNs and never fail it. So a PR that changes a settings CONTROLLER, or adds or deletes a lib/Settings/*Admin.php section, lands in the empty-scope branch — and the line read "this PR introduces no settings placement (ADR-079) to judge", which on such a diff is false. The author changed the settings surface; this gate does not adjudicate that half of it. NOT WIDENED, on purpose. Reading the controller was tried and reverted, and check_store_and_settings_surface.py records why: a gate that only RUNS when a manifest changed and then judges code the PR never touched "blocked EVERY manifest-touching PR in that repo, permanently". The verdict stays `na` — the correct category, since no change the author could make puts a manifest into a diff that does not touch one. What changes is that the line now names which half it looked at, and when the diff contains lib/Settings or a settings controller it says so explicitly, so `na` cannot be read as a clearance for the change the author actually made. VERIFIED, BOTH ARMS OF THE #270 CONTRACT ----------------------------------------- * empty ADR-020 scope -> NOT APPLICABLE, exit 0 under --require-full-coverage (measured on the controller-only fixture above) * genuine structural gap -> SKIPPED (structural), exit 98 (test_gate_empty_scope_never_passes.sh ARM 4, gate-33 with --axe-enabled and no report) And gate-60's three states, all asserted: real finding · SKIPPED (wiring) naming the missing dependency · clean pass. FLEET SWEEPS — 21 apps-extra repos, old helper vs new ------------------------------------------------------ gate-56 zero verdict changes gate-64 zero verdict changes; the three pre-existing FAILs (openbuild, procest, scholiq) survive, and the three NOTEs (larpingapp, hermiq, nldesign) are unchanged TESTS ----- test_gate_crashed_checker_is_not_a_finding.sh +2 gates, 5 assertions. Two arms: the plants must FAIL with a working interpreter, and the SAME tree must report SKIPPED (wiring) with a dead one — otherwise "always skip" would pass. Against 48c88ba it reports the two PASSes. test_check_register_handler_resolution.py +8 cases incl. the mutant that restores the pre-fix pattern and requires all four forms to go back to not-found. test_check_apphost_autoload_prelude.py +9 cases, closure and anti-widening arms. REPOS PLANTED IN, by gate -------------------------- 56 shillinq (register-owning, 153 register.d files) + a synthetic DI-bound/non-conventional-path fixture 57 shillinq (316 services) + fixture 58 shillinq (60 e2e files); fleet-wide check that no live networkidle call in any repo lives outside tests/e2e/ — 193 inside, 0 outside 59 docudesk (the repo the gate was written against) + fixture 60 shillinq (233 manifests, fake MDI package) + fixture 61 shillinq (15 post-event registrations) 62 shillinq 63 shillinq + a controller-only fixture 64 larpingapp (eager class_exists composition root, 3 live probes) AND launchpad (lazy-closure composition root, 0 eager references) NOT MINE, REPORTED NOT FIXED ----------------------------- test_gate_45_to_55_acceptance.sh fails 4 gate-53 assertions identically on pristine main (48c88ba) and on this branch — the suite expects blocking behaviour for pre-existing orphans that #250/#260/#280 deliberately made advisory. Out of this band; flagged rather than touched.
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.
gate-57 reported
pipelinq LeadService::createLead— a curated, spec'd MCP write tool — as an orphaned write capability. Acting on that verdict would have deleted live code, which is the failure mode this module's own docstring already records from hydra#106 and the reason it says false positives are the primary risk of this gate.The seam
createLeadhas no syntactic->createLead(call site and never will. It is reached by attribute reflection (ADR-063):lib/Service/LeadService.php:117carries#[McpTool(name: 'createLead', …)]lib/Mcp/PipelinqScannableServices.phplistsLeadService::classlib/AppInfo/Application.php:246binds it under'OCA\OpenRegister\Mcp\IMcpScannableServices::pipelinq'AttributeToolScannerreflects the class and invokes the methodThe caller index cannot see any of that.
All three pieces are required, mirroring what the register.d-handler and event-listener seams already demand, and the exemption is applied per method:
#[McpTool]on a class nobody registersLeadService::createInvoicein the fixture is exactly that control: same class, same scannable registration, no attribute, still reported.The gate-64 shape — checked as asked, and present
gate-64's
has_prelude()grepped a quoted string literal, so it missed every constant and matched inside comments — failing in both directions at once. The attribute seam would have inherited the comment half for free. pipelinq's realLeadService.phpopens withon line 7, seven lines above anything executable. A raw-text search matches that sentence.
So the seam reads a comment-blanked copy.
_blank_php_comments()preserves length and line numbers, is string-aware, and knows that#opens a line comment in PHP while#[opens an attribute — blanking those would delete the very thing being looked for.The two pre-existing seams had the same latent shape. A commented-out
registerEventListener(...), or a<!-- <job>…</job> -->, exempts a whole class — a false GREEN. Both now strip comments first.The count-as-exit-status bug (#209) is not present here
This helper prints one line per finding and always returns
0; the bash gate counts the printed lines. There is no count anywhere near the exit status. It is now asserted explicitly so it cannot drift into one.Measured, old vs new
lib/Service/**in the eight repos atorigin/development:Exactly one finding removed —
lib/Service/LeadService.php:125 method=createLead— and every other line byte-identical. The gate did not go quiet.Tests
22 new tests alongside the suite's existing 21 — 43 total, all green. Ten mutations of the changed predicates, each killed:
#[treated as a commentThe word-based boundary mutation is worth naming:
#[McpTool(handler: Foo::class)]contains the wordclass, so a\bclass\bboundary stops the walk-up before reading the attribute it exists to find. That was my first attempt and the test caught it.The issue's second point
The module docstring now records it: a zero-caller write capability has two remedies, and the finding text only suggests one. softwarecatalog's
publishEntryForFederation()had zero callers whilePublicationService::publish()served the same capability through a live route — wiring the orphan would have duplicated a live endpoint and widened the auth surface (it had no per-object guard). Deletion was correct (softwarecatalog#447).The reader-facing copy of this guidance lives in
hydra/.claude/skills/hydra-gate-orphaned-write-capability/SKILL.md, a different repository, and is not touched here.Not done
No waiver, no
@orphaned-write-capability exclude, no threshold change. Nothing in any consumer repo was edited — the 19 genuine findings stand.Closes #200