Harden policy validation checks - #683
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While the PR is technically up to Codacy standards, several functional risks were identified during review. The hardening of policy validation introduces a logic flaw in the live() function; by discarding entire lines containing quoted text, the script may ignore legitimate policy violations appearing on the same line.
Additionally, the workflow validation script now mandates a YAML parser and exits with a failure status even if no workflows are present in the repository, which may lead to breaking CI in environments that do not require these checks. There is also a lack of automated test coverage for the expanded regex patterns and filtering logic, making the hardening difficult to verify.
About this PR
- The
check-workflows-parse.shscript exits with code 1 if no parser is found before checking if workflows actually exist. This could cause unnecessary CI failures in repositories that contain no workflows but lack the required tooling in their runner environment. - The PR lacks automated tests (e.g., BATS or shell unit tests) to verify the expanded regex patterns and the new line-filtering logic for quotes. Without these, it is difficult to ensure the hardening works as intended across different edge cases.
Test suggestions
- Verify 'Supports TypeScript' triggers a failure in standard policy text.
- Verify 'Supports TypeScript' is ignored when inside a markdown blockquote ('>').
- Verify 'Supports TypeScript' is ignored when wrapped in double quotes (") or curly quotes (“”).
- Verify a policy table with an empty first cell ('blanking scar') causes the script to fail.
- Verify
check-workflows-parse.shreturns exit code 1 when no YAML parser is available in the environment.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify 'Supports TypeScript' triggers a failure in standard policy text.
2. Verify 'Supports TypeScript' is ignored when inside a markdown blockquote ('>').
3. Verify 'Supports TypeScript' is ignored when wrapped in double quotes (") or curly quotes (“”).
4. Verify a policy table with an empty first cell ('blanking scar') causes the script to fail.
5. Verify `check-workflows-parse.sh` returns exit code 1 when no YAML parser is available in the environment.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| # 2. The rule that told repos not to declare dependencies at all. hyperpolymath/ubicity | ||
| # a phrase inside a blockquote or quotation marks is HISTORY, not policy | ||
| live(){ grep -vE '^[[:space:]]*>' "$1" | grep -vE '"[^"]*'"$2"'[^"]*"|“[^”]*'"$2"'[^”]*”'; } | ||
| live(){ grep -vE '^[[:space:]]*>' "$1" | grep -vE '"[^"]*('"$2"')[^"]*"|“[^”]*('"$2"')[^”]*”'; } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The current implementation of live using grep -vE to exclude quoted patterns can lead to false negatives. If a line contains both a legitimate violation and a quoted historical reference, the entire line is discarded. Instead of excluding the whole line, modify the function to strip the quoted substrings before performing the violation check.
| # "no typescript ... that should not exist at all." | ||
| if grep -nE 'Executes .\.ts. directly|JS/TS runtime' "$f" >/dev/null; then | ||
| fail "$f:$(grep -nE 'Executes .\.ts. directly|JS/TS runtime' "$f" | head -1 | cut -d: -f1)" \ | ||
| typescript_runtime='Executes .\.ts. directly|JS/TS runtime|[Ss]upports? TypeScript|[Rr]uns? [^[:alnum:][:space:]]*\.ts[^[:alnum:][:space:]]* files?' |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The regex pattern .\.ts. uses unescaped dots as wildcards, which may be too broad for a policy gate and lead to false positives. Refine this to use literal characters (like backticks or quotes) common in markdown documentation to ensure more precise matching.
JoshuaJewell
left a comment
JoshuaJewell
left a comment
There was a problem hiding this comment.
This fixes some of the previous issues. However, live() removes the entire line when it finds a quoted historical match - please strip or mask the quoted substring instead. The workflow checker should also discover workflow files before requiring a parser, so repositories with no workflows can still take the documented “nothing to check” path.
Please add reproducible fixtures for quoted history, quoted history plus a live violation on the same line, the blank-cell case, no workflows, and workflows without a parser. The current Actions runs all ended in startup failure and Sonar’s quality gate is failing, so the changes are not otherwise adequately verified.
Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com> Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
…icy-checks/dfd712e9
|
## Outcome Consolidates the sound intent of #661, #682, and #683 onto current `main` without importing their stacked/conflicting history. - adds a NUL-safe language-policy invariant gate - masks quoted historical substrings instead of discarding whole lines - discovers tracked workflows before requiring a YAML parser - fails closed when workflows exist but no parser is available - uses a portable byte-level forbidden-control check - wires both gates into the reusable governance workflow - adds executable positive and negative fixtures ## Planted controls Verified locally: - quoted history: pass - quoted history plus a live violation on the same line: fail - blockquoted history: pass - blank policy cell: fail - no workflows and no parser: pass - workflow present and no parser: fail - valid workflow YAML: pass - invalid workflow YAML: fail - all 157 currently tracked Standards workflows: parse Also passed `just validate`, `just test`, `bash -n`, and `git diff --check`. Supersedes #661, #682, and #683 after merge.
2 participants
Expand language-policy detection to catch more TypeScript runtime claims while excluding quoted historical text, fix pipeline and awk exit handling, and make missing YAML parsers fail workflow validation. Validation: Bash syntax checks and both policy scripts exited successfully; the workflow script reported no tracked workflows.
View coding task