Repository navigation
Revert "Extend comment scanner with piped-stdin, runner-string, MDX, and worktree TypeScript support (#2821)" - #2910
joeharris76 wants to merge 3 commits into
Conversation
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: pending for ee8839f8e64dc717fa56e0842e2c9426cbc0aaa5
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (972 code lines exceed 150; touches 2 areas).
Reviewers not available
- sol: absent (error)
- luna: absent (error)
- muse: absent (invalid)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
…and worktree TypeScript support (#2821)" This reverts commit 3b4d7e5. The revert also refreshes the remediation-contract audit replay stamps (the restored replay_sha is unreachable; replay re-ran at the revert tree: 112 passed) and applies behavior-preserving executable-payload adapter restructures to seven workflow/shell files. The adapter changes are required because transition coverage-errors are never grandfathered: without them the revert cannot land. They mirror the proven remediation on the #2907 branch.
Takes the proven publication-guard versions from the #2907 branch for the two publication workflows restructured for scanner adapters. The adapter restructuring alone breaks three publication structural tests; these versions satisfy both the scanner and the structural tests (3/3 pass locally).
There was a problem hiding this comment.
oracle-verdict: failure for 60f2bb2bb746c8a14d1c5a3a0bedf07aa924117f
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (1041 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The change converts many comment-policy fail-closed adapters into documented blind spots: piped stdin, SQL-client stdin, MDX/JSX, runner one-string commands, and runner-option validation are removed while the JS-flow test becomes skippable and the CI parser install is dropped, and the evidence file deletes the prior D-series verification. Each lets a comment pass unreported or makes a published claim unverifiable, so the merge gate is weaker and must be restored before merge.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
| SHELL_PIPED_MODELED_INTERPRETER = re.compile( | ||
| r"\|[^\n]*(?<![\w.-])(?:python[0-9.]*|node|bash|sh|zsh)(?=[\s;|&)\"']|$)" + "|" + SHELL_DYNAMIC_PIPED_COMMAND | ||
| ) | ||
| SHELL_INLINE_INTERPRETER = re.compile( |
There was a problem hiding this comment.
Critical: Piped echo/printf into stdin interpreters is no longer scanned
The deleted piped-stdin adapter (piped_sides, piped_stdin_entries, piped_producer_text, scan_source_payload) meant static producer output such as echo '# hidden' | python3, printf variants, absolute-path interpreters, cat pass-through stages and redirect-preserving cases was scanned as a child payload. SHELL_INLINE_INTERPRETER no longer matches the pipe pattern and shell_command_unit no longer visits pipelines, so that payload is never produced and the comment passes silently. Restore piped producer-to-stdin scanning or explicitly fail closed.
| def chunk_can_hold_comment(chunk: str) -> bool: | ||
| has_heredoc = bool(re.search(r"(?<!<)<<-?\s*", chunk)) | ||
| if SHELL_MODELED_INLINE.search(chunk) or (not has_heredoc and SHELL_PIPED_MODELED_INTERPRETER.search(chunk)): | ||
| if SHELL_MODELED_INLINE.search(chunk): | ||
| return True | ||
| for match in SHELL_UNMODELED_NAME.finditer(chunk): | ||
| name = match.group(1) | ||
| markers = re.compile(r"--|/\*") if name in SQL_CLIENTS else UNMODELED_MARKERS.get(name, re.compile(r"#")) | ||
| if markers.search(chunk, match.end()): | ||
| return True | ||
| if not has_heredoc: | ||
| pipe = chunk.rfind("|", 0, match.start()) | ||
| if pipe >= 0 and markers.search(chunk, 0, pipe): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
High: Dynamic and unmodeled piped consumers no longer fail closed
Previously chunk_can_hold_comment treated SHELL_PIPED_MODELED_INTERPRETER matches and comment markers before a pipe to an unmodeled interpreter as comment-capable, and piped_sides raised coverage-error for dynamic consumers (echo 'pass' | $interpreter), unspaced pipes, unsupported intermediates (sed | python3), dynamic later stages, and echo x | perl style cases. The narrowed check only looks for inline interpreters plus markers after unmodeled names, so those inputs now return no findings instead of coverage-error and bypass the blocking gate. Restore the pipe-aware fail-closed checks.
| @@ -761,7 +613,6 @@ def sql_client_source_index(name: str, words: Sequence[str | None]) -> int | Non | |||
|
|
|||
|
|
|||
| SQL_CLIENTS = {"psql", "duckdb", "sqlite3"} | |||
There was a problem hiding this comment.
High: SQL-client stdin comment scanning removed
The removed psql_reads_stdin, sqlite3_reads_stdin, duckdb_reads_stdin and sql_client_reads_stdin routing decided when a psql/sqlite3/duckdb consumer actually reads piped stdin versus file, positional SQL, -no-stdin, or help/version modes. With only sql_client_source_index left, piped SQL payloads such as echo 'SELECT 1 -- hidden' | psql are never routed to the SQL scanner while the negative controls are also gone, so SQL comments on stdin pass unreported. Restore stdin-mode routing.
| @@ -647,23 +645,15 @@ def process_payload(self, args: list[ast.expr]) -> tuple[ast.AST, str, str | Non | |||
| def runner_payload( | |||
There was a problem hiding this comment.
High: Runner-wrapped one-string commands are no longer followed
runner_payload no longer consults RUNNER_COMMANDS or runner_string_args and nested_runner_entries/runner_command_entries are gone, so one-string runner commands such as ssh host "python3 -c '# explanation'", watch/xargs/sudo/nohup/env quoted forms are never decomposed into bash for scanning. The policy doc now states these are not split. Comments reachable only through that indirection pass unreported. Restore runner string decomposition or fail closed.
| @@ -1310,14 +1064,6 @@ def check_unmodeled_inline(words: list, command: str) -> None: | |||
|
|
|||
|
|
|||
| def runner_command_start(words: list[str | None], command: str) -> int: | |||
There was a problem hiding this comment.
High: Runner option validation no longer fails closed
runner_command_start now only handles find and always returns 1 otherwise; the sudo/nohup option validation, RUNNER_VALUE_OPTIONS for ssh/watch/xargs/env/sudo/nohup, runner_command_index, runner_trailing_index, piped_consumer_words, piped_producer_words, unwrap_runner_command and env/sudo/nohup wrapper unwrapping are deleted. Unsupported or dynamic wrapper options (sudo --unknown, nohup -x, env -S) no longer raise coverage-error, so an unrecognized wrapper can hide an interpreter payload. Restore option validation with fail-closed behavior.
| if name.startswith("Dockerfile"): | ||
| return "docker" | ||
| if path.endswith((".md", ".rst")) and not path.startswith(("_project/", "_blog/")): | ||
| if path.endswith((".md", ".mdx", ".rst")) and not path.startswith(("_project/", "_blog/")): |
There was a problem hiding this comment.
High: MDX imports/exports and JSX comments are no longer inspected
.mdx previously mapped to a dedicated mdx language with mdx_js_sources, mdx_jsx_comments, mdx_masked_lines, statement-boundary tracking, inline-code masking, and TypeScript-scanner routing for imports/exports and JSX {/* */} comments. Mapping .mdx to examples means only fenced code blocks are scanned and MDX ESM/JSX comments outside fences are never inspected. Restore MDX extraction or keep .mdx out of the examples mapping with a coverage error.
| assert shutil.which("node") is not None, "Node.js is required to verify reviewed JavaScript flows" | ||
| assert typescript is not None and typescript.exists(), "the TypeScript parser is required for reviewed flows" | ||
| typescript = os.environ.get("COMMENT_POLICY_TYPESCRIPT", str(ROOT / "results-explorer/node_modules/typescript")) | ||
| if shutil.which("node") is None or not Path(typescript).exists(): |
There was a problem hiding this comment.
High: Reviewed JavaScript-flow gate is now skippable and CI parser install removed
The test previously asserted Node.js and the TypeScript parser must exist and the medium-test job installed the locked parser via quality/comment-policy-package.json. The head test skips when node or COMMENT_POLICY_TYPESCRIPT is missing, and .github/workflows/ci.yml no longer installs the locked parser after Install dependencies, so reviewed-JavaScript-flow coverage can silently pass without exercising scripts/comment_syntax_js.cjs. The simplified comment_syntax_js.cjs resolution also drops the git-common-dir worktree fallback. Make the parser mandatory again in CI and in the test.
| replay_sha: ee8839f8e64dc717fa56e0842e2c9426cbc0aaa5 | ||
| replay_scope: PR 2888 mirror, parity, throughput contract tests and full fast suite at replay tree ee8839f8e64dc717fa56e0842e2c9426cbc0aaa5 |
There was a problem hiding this comment.
Medium: Remediation evidence for prior scanner findings deleted
The head record changes replay_sha/scope to a narrower PR 2888-only replay and deletes the current-replay section plus the D10-D26 scanner evidence (piped producers, MDX shapes, runner/SSH/watch/xargs handling, SQL stdin, timing and gate runs). Consumers of this file can no longer re-derive the accepted scanner claims from cited nodes. Restore the deleted evidence or record where each removed claim is now re-verified.
|
Closing unmerged: Trunk is green on develop at d39fa16 (run 38193877662 — all four medium shards pass) with #2907's forward fix in place and #2821's scanner feature intact. The revert is no longer needed; the feature stays. Recovery context and re-land plan in the PR body above are superseded — no re-land required. |
Pull request was closed
This branch was successfully deployed
1 participant
This reverts commit 3b4d7e5.
Recovery context
Reverts #2821 to address the failed Trunk run: https://github.com/BenchBox-dev/benchbox/actions/runs/38177709896 (all four medium-test shards fail in
tests/unit/scripts/test_comment_policy.py— the TypeScript parser is absent from Trunk's medium-test environment). This is a temporary recovery step, not a decision to abandon the scanner feature.Re-land plan
quality/comment-policy-package-lock.json) into Trunk's medium-test environment viaCOMMENT_POLICY_TYPESCRIPT. Acceptance: all four medium shards green on Trunk post-merge.Why this revert is not pure
Two follow-up commits ride along because the revert cannot land without them:
fix(audit): refresh remediation replay stamps— the restored audit record carried an unreachablereplay_sha; the prescribed replay was re-run at the revert tree (112 passed) and stamps updated.coverage-errorfindings are never grandfathered, so these files block every commit until adapted. All restructures are behavior-preserving (verified by inspection; De Morgan inversions and split alternations only).