Config: centralize known plugin ID resolution for validation#56705
Config: centralize known plugin ID resolution for validation#56705yejiming wants to merge 8 commits intoopenclaw:mainfrom
Conversation
…or-contribution Tighten TTS directive parsing, disable Bolt ignoreSelf for socket mode, and recognize bundled provider IDs in validation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a145356ebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const blockRegex = /\[\[tts:text\]\]([\s\S]*?)\[\[\/tts:text\]\]/gi; | ||
| const blockRegex = | ||
| /(?:^|\n)[ \t]*\[\[tts:text\]\][ \t]*(?:\n|$)([\s\S]*?)(?:\n|^)[ \t]*\[\[\/tts:text\]\][ \t]*(?=\n|$)/gi; |
There was a problem hiding this comment.
Preserve inline [[tts:text]] blocks in directive parser
The new blockRegex only matches [[tts:text]] when the opening and closing tags are on their own lines, so compact tagged payloads like [[tts:text]]Hello world[[/tts:text]] are no longer recognized as directives. That regresses tagged-mode auto-TTS behavior (these payloads are treated as plain text, hasDirective stays false, and synthesis is skipped), even though this inline form is already used in existing tests and runtime message flows.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR centralizes known plugin-ID resolution in Key observations:
Confidence Score: 4/5Safe to merge pending clarification of the intentional TTS inline-directive behavior change, which silently drops previously-functional overrides. The config-validation core and the Slack fix are correct and well-tested. Score is 4 rather than 5 because the TTS regex change represents a present, undocumented behavioral regression: directives that previously activated TTS overrides when placed inline in text are now silently ignored. If this is intentional, documenting the change and adding a user-visible warning would resolve the concern and bring the score to 5. src/tts/directives.ts — the regex tightening changes parsing behavior in a way that is silent and undocumented.
|
| Filename | Overview |
|---|---|
| src/tts/directives.ts | Both TTS directive regexes changed to require on-own-line placement; inline directives are now silently ignored — a behavioral change out of the stated PR scope. |
| src/config/validation.ts | Extracts resolveKnownPluginIds helper that merges manifest registry IDs with bundled alias keys+values and auto-enable IDs, making cold-start validation resilient. Logic is correct. |
| src/config/config.plugin-validation.test.ts | Adds parameterized severity-consistency tests and a missing-bundled-dir test; coverage is solid. |
| extensions/slack/src/monitor/provider.ts | Adds ignoreSelf: false to socket-mode Bolt App config so reaction events on bot-authored messages are not suppressed. Correct fix, well-commented. |
| extensions/slack/src/monitor.test-helpers.ts | Adds lastAppOptions capture to the mock Bolt App constructor and exposes getLastSlackAppOptions() for test assertions. |
| extensions/slack/src/monitor.tool-result.test.ts | Adds test asserting socket mode is started with ignoreSelf: false. Well-targeted guard for the provider.ts fix. |
| src/plugins/contracts/tts.contract.test.ts | Test inputs updated to place directives on their own lines matching the new regex requirement; one new inline-literal test added to assert the changed behavior. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/tts/directives.ts
Line: 55-65
Comment:
**Silent breaking change: inline directives no longer parsed**
Both `blockRegex` and `directiveRegex` now require `[[tts:...]]` markers to appear on their own dedicated lines (preceded by `^` or `\n`, followed by `\n` or `$`). Previously any inline occurrence — e.g. `Please use [[tts:provider=edge]] for this` — was parsed and applied.
After this change, that same directive is silently left in the text and **ignored**; no warning is emitted, no error is raised. Existing users or integrations that embed TTS directives inline in a sentence will have their overrides quietly dropped without any feedback.
This is also out of the stated scope in the PR description:
> *"What did NOT change (scope boundary): … this PR only adjusts validation known-ID resolution and related tests."*
If the intent is to deliberately require directives to be on their own lines (to avoid false positives in prose), that design decision should be explicitly documented, a migration note / deprecation warning should be added, and the PR description should be updated to reflect this behavioral change.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/tts/directives.ts
Line: 55-56
Comment:
**`^` in closing-tag alternation is dead code**
In the `blockRegex`, the closing-tag prefix is `(?:\n|^)`. Without the `m` (multiline) flag, `^` only matches the very start of the entire input string. A `[[/tts:text]]` closing tag can never appear at position 0 (the opening tag and its content always precede it), so the `^` branch can never fire in practice. The regex works correctly, but the `^` is misleading. Replacing it with just `\n` would be clearer:
```suggestion
const blockRegex =
/(?:^|\n)[ \t]*\[\[tts:text\]\][ \t]*(?:\n|$)([\s\S]*?)\n[ \t]*\[\[\/tts:text\]\][ \t]*(?=\n|$)/gi;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Config: centralize known plugin id resol..." | Re-trigger Greptile
| @@ -61,7 +62,7 @@ export function parseTtsDirectives( | |||
| return ""; | |||
| }); | |||
|
|
|||
| const directiveRegex = /\[\[tts:([^\]]+)\]\]/gi; | |||
| const directiveRegex = /(?:^|\n)[ \t]*\[\[tts:([^\]]+)\]\][ \t]*(?=\n|$)/gi; | |||
There was a problem hiding this comment.
Both blockRegex and directiveRegex now require [[tts:...]] markers to appear on their own dedicated lines (preceded by ^ or \n, followed by \n or $). Previously any inline occurrence — e.g. Please use [[tts:provider=edge]] for this — was parsed and applied.
After this change, that same directive is silently left in the text and ignored; no warning is emitted, no error is raised. Existing users or integrations that embed TTS directives inline in a sentence will have their overrides quietly dropped without any feedback.
This is also out of the stated scope in the PR description:
"What did NOT change (scope boundary): … this PR only adjusts validation known-ID resolution and related tests."
If the intent is to deliberately require directives to be on their own lines (to avoid false positives in prose), that design decision should be explicitly documented, a migration note / deprecation warning should be added, and the PR description should be updated to reflect this behavioral change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tts/directives.ts
Line: 55-65
Comment:
**Silent breaking change: inline directives no longer parsed**
Both `blockRegex` and `directiveRegex` now require `[[tts:...]]` markers to appear on their own dedicated lines (preceded by `^` or `\n`, followed by `\n` or `$`). Previously any inline occurrence — e.g. `Please use [[tts:provider=edge]] for this` — was parsed and applied.
After this change, that same directive is silently left in the text and **ignored**; no warning is emitted, no error is raised. Existing users or integrations that embed TTS directives inline in a sentence will have their overrides quietly dropped without any feedback.
This is also out of the stated scope in the PR description:
> *"What did NOT change (scope boundary): … this PR only adjusts validation known-ID resolution and related tests."*
If the intent is to deliberately require directives to be on their own lines (to avoid false positives in prose), that design decision should be explicitly documented, a migration note / deprecation warning should be added, and the PR description should be updated to reflect this behavioral change.
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3de196e13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const blockRegex = | ||
| /(?:^|\n)[ \t]*\[\[tts:text\]\][ \t]*(?:\n|$)([\s\S]*?)\n[ \t]*\[\[\/tts:text\]\][ \t]*(?=\n|$)/gi; |
There was a problem hiding this comment.
Accept compact [[tts:text]] blocks in directive parser
The updated blockRegex now requires [[tts:text]] and [[/tts:text]] to be on separate lines, so compact payloads like [[tts:text]]Hello world[[/tts:text]] are no longer recognized as directives. In tagged auto-TTS mode this leaves hasDirective false and skips synthesis, which regresses existing tagged payload behavior (the contract suite still includes a tagged case using this compact form).
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. A source-level reproduction is Next step before merge Security Review findings
Review detailsBest possible solution: Ship a focused config-validation patch on current main that centralizes known plugin-id resolution through current manifest/metadata contracts, preserves inline TTS parsing, and leaves the already-shipped Slack behavior alone. Do we have a high-confidence way to reproduce the issue? Yes. A source-level reproduction is Is this the best way to solve the issue? No. Centralizing known plugin-id validation is a reasonable narrow fix, but this branch is not the best vehicle because it mixes unrelated behavior changes and is stale against the current bundled metadata boundary. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 0fbf4636f52c. |
Summary
Describe the problem and fix in 2–5 bullets:
src/config/validation.ts, which made future extension/error-prone drift likely.resolveKnownPluginIds(...)to centralize known-ID resolution from manifest registry IDs + bundled alias metadata + auto-enable provider IDs; updatedensureKnownIdsto use this helper; added parameterized tests covering warning/issue severity acrossplugins.allow,plugins.entries.*, andplugins.slots.memory.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
git blame, prior PR, issue, or refactor if known): Validation had evolved incrementally with separate known-ID additions in-place.Regression Test Plan (if applicable)
src/config/config.plugin-validation.test.tsplugins.allow,plugins.entries.*, andplugins.slots.memorymust classify known bundled IDs as valid and unknown IDs with stable warning/issue severity per surface.User-visible / Behavior Changes
Diagram (if applicable)