Copilot commented on code in PR #12950:
URL: https://github.com/apache/trafficserver/pull/12950#discussion_r2916267356
##########
tools/hrw4u/src/visitor_base.py:
##########
@@ -434,6 +438,9 @@ def _parse_op_tails(self, node, ctx=None) ->
tuple[list[str], object, object]:
raise Exception(f"Unknown modifier:
{flag_text}")
else:
raise Exception(f"Unknown modifier: {flag_text}")
+ sandbox = self._sandbox
+ if sandbox is not None:
+ sandbox.check_modifier(flag_text)
Review Comment:
Sandbox modifier checks in `_parse_op_tails()` ignore the warning string
returned by `SandboxConfig.check_modifier()`. This means
`sandbox.warn.modifiers` will never surface as warnings for bracket modifiers
(e.g. `[L]`, `[QSA]`), even though the policy supports warn/deny for modifiers.
Capture the return value and route it into the warning reporting path (e.g.
via the visitor's error collector / diagnostic formatter), similar to how other
sandbox checks add warnings.
```suggestion
warning = sandbox.check_modifier(flag_text)
if warning:
Dbg("sandbox", f"Sandbox warning for modifier
{flag_text}: {warning}")
```
##########
doc/admin-guide/configuration/hrw4u.en.rst:
##########
@@ -494,6 +542,156 @@ Run with `--debug all` to trace:
- Condition evaluations
- State and output emission
+Sandbox Policy Enforcement
+==========================
+
+Organizations deploying HRW4U across teams can restrict which language features
+are permitted using a sandbox configuration file. Features can be **denied**
+(compilation fails with an error) or **warned** (compilation succeeds but a
+warning is emitted). Both modes support the same feature categories.
+
+Pass the sandbox file with ``--sandbox``:
+
+.. code-block:: none
+
+ hrw4u --sandbox /etc/trafficserver/hrw4u-sandbox.yaml rules.hrw4u
+
+The sandbox file is YAML with a single top-level ``sandbox`` key. A JSON
+Schema for editor validation and autocomplete is provided at
+``tools/hrw4u/schema/sandbox.schema.json``.
+
+.. code-block:: yaml
+
+ sandbox:
+ message: | # optional: shown once after all errors/warnings
+ ...
+ deny:
+ sections: [ ... ] # section names, e.g. TXN_START
+ functions: [ ... ] # function names, e.g. run-plugin
+ conditions: [ ... ] # condition keys, e.g. geo.
+ operators: [ ... ] # operator keys, e.g. inbound.conn.dscp
+ language: [ ... ] # break, variables, in, else, elif
+ warn:
+ functions: [ ... ] # same categories as deny
+ conditions: [ ... ]
+
+All lists are optional. An empty or missing sandbox file permits everything.
Review Comment:
The docs say “An empty or missing sandbox file permits everything,” but
`SandboxConfig.load()` requires the YAML to contain a top-level `sandbox`
mapping and will raise if the file is empty or lacks that key. Consider
rewording to clarify that omitting `--sandbox` permits everything, and that an
empty policy still must be expressed as `sandbox: {}` (or equivalent) when a
file is provided.
```suggestion
All lists are optional. If ``--sandbox`` is omitted, all features are
permitted. When a sandbox file
is provided but you want an empty policy, express it as an empty mapping
(for example, ``sandbox: {}``).
```
##########
tools/hrw4u/scripts/hrw4u-lsp:
##########
@@ -147,7 +151,7 @@ class DocumentManager:
parser_errors = list(parser._syntax_errors)
if tree is not None:
- visitor = HRW4UVisitor(filename=filename,
error_collector=error_collector)
+ visitor = HRW4UVisitor(filename=filename,
error_collector=error_collector, sandbox=self.sandbox)
try:
Review Comment:
Now that the LSP runs the visitor with a sandbox, sandbox *warnings* can be
emitted into `ErrorCollector.warnings`, but `_analyze_document()` only converts
`error_collector.errors` into LSP diagnostics (`has_errors()` gate). As a
result, warned features won’t appear in the editor.
Consider also mapping collected warnings into diagnostics with LSP
severity=2 (Warning) so policy `warn:` is visible in the IDE.
##########
tools/hrw4u/scripts/hrw4u-lsp:
##########
@@ -603,7 +615,16 @@ class HRW4ULanguageServer:
def main() -> None:
"""Main entry point for the LSP server."""
+ parser = argparse.ArgumentParser(description="HRW4U Language Server")
+ parser.add_argument("--sandbox", metavar="FILE", type=Path, help="Path to
sandbox YAML configuration file")
+ args = parser.parse_args()
Review Comment:
`hrw4u-lsp` now uses `argparse.parse_args()`, which will exit with an error
on any unknown CLI flags. Previously the server ignored argv entirely, so
wrappers/editors that pass extra arguments would have continued to work.
To avoid breaking existing editor integrations, consider using
`parse_known_args()` (ignoring unknown flags) or explicitly supporting common
LSP server flags (e.g. `--stdio`) in addition to `--sandbox`.
```suggestion
parser.add_argument("--stdio", action="store_true",
help="Use stdio for LSP communication (default;
accepted for compatibility)")
args, _unknown = parser.parse_known_args()
```
##########
tools/hrw4u/src/visitor.py:
##########
@@ -318,6 +354,8 @@ def visitVarSection(self, ctx) -> None:
else:
raise error
with self.debug_context("visitVarSection"):
Review Comment:
`SandboxConfig.check_section()` is not applied to the `VARS { ... }` block
because `visitSection()` short-circuits to `visitVarSection()` before
`_prepare_section()` runs. This means a policy like `sandbox.deny.sections:
[VARS]` (which is documented and allowed by the schema) currently has no effect.
Either enforce `sections: VARS` in `visitVarSection()` (e.g. call
`check_section("VARS")` there) or remove/clarify VARS support in the
docs/schema and require using `sandbox.*.language: [variables]` instead.
```suggestion
with self.debug_context("visitVarSection"):
warning = self._sandbox.check_section("VARS")
if warning:
self._add_sandbox_warning(ctx, warning)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
