Harden content and script maintenance safeguards#62
Conversation
Validate host and port inputs before use, verify lsof/bundler/Jekyll prerequisites, and run Jekyll through a command array to avoid shell injection via dynamic flags.
Run markdownlint and prettier checks for _teaching Markdown files on pull requests and pushes to main so style regressions are blocked before review.
Load utils.js before main.js on teaching, history, and research layouts, and remove duplicate footer script includes from research and team layouts to prevent double initialization.
Replace custom tags element usage with .tags containers in research content, JS filtering logic, CSS selectors, and tag-page generation so style and static-analysis tooling treat selectors as standard HTML class selectors.
Add a content validation script and CI workflow to check descending year/reverse-month ordering in history, enforce .tags markup in research content, and keep add-news/docs guidance aligned with those rules.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds two GitHub Actions workflows for content and teaching checks; introduces a content-validation script enforcing history/tag/docs rules; migrates Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "PR / Push"
participant GH as "GitHub Actions"
participant Runner as "Runner (ubuntu-latest)"
participant Repo as "Repository"
participant Script as "scripts/validate-content-rules.sh"
PR->>GH: trigger content-rules workflow
GH->>Runner: start job
Runner->>Repo: actions/checkout
Runner->>Script: run ./scripts/validate-content-rules.sh
Script->>Repo: read history.md, _research/index.md, docs files
Script->>Script: validate history order, tag markup, required docs
Script-->>Runner: exit status (0 / non-zero)
Runner-->>GH: report job result
GH-->>PR: mark CI pass/fail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
_layouts/teaching.html (1)
133-133:⚠️ Potential issue | 🟡 MinorDuplicate
command-palette.jsscript include.This script is already loaded at line 88. Loading it twice is wasteful and may cause double-initialization issues.
🔧 Proposed fix
- <script defer src="/assets/js/command-palette.js"></script> - <script> // Check if we're on localhostscripts/deploy.sh (1)
213-235:⚠️ Potential issue | 🟠 MajorScript exits when no livereload port is available due to
set -euo pipefail.With
set -euo pipefailenabled (line 6), the command substitutionLIVERELOAD_PORT=$(find_available_livereload_port)causes immediate exit whenfind_available_livereload_portreturns 1. The fallback logic checking[ -z "$LIVERELOAD_PORT" ]is never reached. Guard the assignment within anifstatement so the error branch executes:✅ Safer livereload port handling under strict mode
if [ "$ENABLE_LIVERELOAD" = true ]; then - LIVERELOAD_PORT=$(find_available_livereload_port) - - if [ -z "$LIVERELOAD_PORT" ]; then - echo -e "${RED}Failed to find an available livereload port. Disabling livereload.${NC}" - else - JEKYLL_CMD+=(--livereload --livereload-port "$LIVERELOAD_PORT") - fi + if LIVERELOAD_PORT=$(find_available_livereload_port); then + JEKYLL_CMD+=(--livereload --livereload-port "$LIVERELOAD_PORT") + else + echo -e "${RED}Failed to find an available livereload port. Disabling livereload.${NC}" + ENABLE_LIVERELOAD=false + fi fi
🤖 Fix all issues with AI agents
In `@scripts/deploy.sh`:
- Around line 112-116: The host validation allows colons for IPv6 but URL
construction later will produce ambiguous addresses; update scripts/deploy.sh to
detect when the input "$2" contains a ':' and is not already bracketed and wrap
it in square brackets before building URLs. Keep the existing validation check
([[ ! "$2" =~ ^[A-Za-z0-9._:-]+$ ]]) but when you form endpoints (places that
currently use "http://$host:$port" or similar), replace them with conditional
logic that uses "http://[$host]:$port" only when $host contains a colon and does
not start with '[' and end with ']'. Ensure this logic uses the same variable
(e.g., $host or "$2") so all URL constructions consistently bracket IPv6
addresses.
🧹 Nitpick comments (2)
scripts/validate-content-rules.sh (1)
1-11: Consider adding file existence checks before validation.The script assumes all referenced files exist. If any file is missing (e.g., during partial checkouts or file renames), the script will fail with an unclear error from the
while readorhas_matchcalls.🛡️ Proposed defensive check
FAILURES=0 + +for f in "$HISTORY_FILE" "$RESEARCH_FILE" "$ADD_NEWS_RULES_FILE" "$CLAUDE_FILE" "$README_FILE"; do + if [[ ! -f "$f" ]]; then + echo "✗ Required file not found: $f" + FAILURES=$((FAILURES + 1)) + fi +done + +if (( FAILURES > 0 )); then + echo "Aborting due to missing files." + exit 1 +fiassets/css/research.css (1)
728-739: Consider using CSS variables for consistency.The active state uses hardcoded hex colors (
#9333ea,#4d8dff) instead of CSS variables. While this works, using variables would improve maintainability and theme consistency.♻️ Suggested refactor
Consider defining these as CSS variables in
:rootand[data-theme="dark"]::root { --tag-active-bg: `#9333ea`; --tag-active-shadow: rgba(147, 51, 234, 0.2); } [data-theme="dark"] { --tag-active-bg: `#4d8dff`; --tag-active-shadow: rgba(77, 141, 255, 0.4); }Then reference them in the active states.
There was a problem hiding this comment.
Pull request overview
This PR hardens maintenance guardrails for the Jekyll site by adding CI checks for content rules/markdown quality, tightening local deploy script safety, and standardizing research tag markup/selectors to prevent regressions in filtering/styling.
Changes:
- Added CI workflows to lint/format teaching markdown and validate cross-file content rules (history ordering + research tag markup/docs consistency).
- Hardened
scripts/deploy.shwith stricter shell settings, argument validation, dependency checks, and safer command execution via arrays. - Migrated research tag markup from the deprecated
<tags>element to<div class="tags">and updated CSS/JS/Ruby selectors accordingly; removed duplicate script includes in layouts and ensuredutils.jsloads beforemain.js.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/validate-content-rules.sh |
New validation script enforcing history.md ordering and .tags markup/docs rules. |
scripts/generate_filtered_research.rb |
Updates Nokogiri selector to collect tags from .tags span. |
scripts/deploy.sh |
Adds strict mode, validates args, checks dependencies, and executes Jekyll via a command array. |
assets/js/main.js |
Updates DOM selectors from tags to .tags for featured-paper extraction and click handling. |
assets/js/command-data.js |
Updates research tag collection/click simulation selectors to .tags span. |
assets/css/styles.css |
Updates featured-paper tag styling selectors to .featured-paper .tags. |
assets/css/research.css |
Updates research tag styling selectors from tags to .tags (including dark theme rules). |
_research/index.md |
Replaces <tags> blocks with <div class="tags"> throughout the research page. |
_layouts/team.html |
Removes duplicate footer script includes to avoid double-loading. |
_layouts/teaching.html |
Ensures utils.js loads before main.js. |
_layouts/research.html |
Ensures utils.js loads before main.js, removes duplicate footer scripts, updates tag filtering selectors to .tags. |
_layouts/history.html |
Ensures utils.js loads before main.js. |
README.md |
Updates documented tag markup examples to <div class="tags">. |
CLAUDE.md |
Updates documented tag markup guidance/examples to <div class="tags">. |
.opencode/commands/add-news.md |
Corrects history month ordering documentation to reverse-chronological. |
.github/workflows/teaching-content-checks.yml |
Adds markdownlint + prettier checks for _teaching/**/*.md. |
.github/workflows/content-rules-checks.yml |
Adds CI job to run scripts/validate-content-rules.sh on relevant changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/validate-content-rules.sh`:
- Around line 80-106: The validations call has_match on files that may not
exist, producing misleading errors; add an explicit file-existence guard (e.g.,
require_file_exists or ensure_file_present) and call it at the start of
validate_research_tags and validate_docs_rules for RESEARCH_FILE,
ADD_NEWS_RULES_FILE, CLAUDE_FILE, README_FILE, and ADD_PAPER_RULES_FILE so
missing files log a clear "file not found" error and cause an immediate
failure/return; only run has_match checks when the guard confirms the file
exists.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/validate-content-rules.sh`:
- Around line 78-88: The parser currently skips unknown month names and months
that appear before a year, allowing malformed history.md to pass; update the
block that calls month_to_number in the loop (the logic using month_to_number,
month_name, month_number, previous_month, current_year) to treat month_number <=
0 as an error and call log_error with a clear message including the offending
month_name and line context, and also add a check to ensure a year context
(current_year is set) before accepting a month heading—if current_year is empty
or unset, call log_error indicating a month appeared before any year heading;
keep using the same symbols (month_to_number, month_name, month_number,
previous_month, current_year, log_error) so the validation fails loudly instead
of silently skipping.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| paths: | ||
| - "history.md" | ||
| - "_research/index.md" | ||
| - ".opencode/commands/add-news.md" | ||
| - "CLAUDE.md" | ||
| - "README.md" | ||
| - "scripts/validate-content-rules.sh" | ||
| push: | ||
| branches: ["main"] | ||
| paths: | ||
| - "history.md" | ||
| - "_research/index.md" | ||
| - ".opencode/commands/add-news.md" | ||
| - "CLAUDE.md" | ||
| - "README.md" | ||
| - "scripts/validate-content-rules.sh" |
There was a problem hiding this comment.
scripts/validate-content-rules.sh validates .opencode/commands/add-paper.md, but this workflow’s paths filter doesn’t include that file. As a result, edits to add-paper.md won’t trigger the content-rules job even though they can violate the enforced rules. Add .opencode/commands/add-paper.md to both the pull_request.paths and push.paths lists (or remove the paths filter if you want all content-rule checks to always run).
| <!-- Command palette (implementation in assets/js/command-palette.js) --> | ||
| <script defer src="/assets/js/command-palette.js"></script> | ||
|
|
||
| <!-- JavaScript dependencies --> | ||
| <script defer src="/assets/js/utils.js"></script> | ||
| <script defer src="https://cdn.jsdelivr.net/npm/marked/marked.min.js"></script> | ||
| <script defer src="https://cdn.jsdelivr.net/npm/fuse.js@6.6.2"></script> | ||
| <script defer src="/assets/js/main.js"></script> |
There was a problem hiding this comment.
In this layout, command-palette.js is loaded before utils.js, but command-palette.js calls Utils.updatePlatformSpecificElements() on DOMContentLoaded. With defer scripts executing in order, this can throw a ReferenceError: Utils is not defined and break the command palette. Load utils.js before command-palette.js (and keep that ordering consistent with _layouts/default.html).
1 participant
Summary
Changes Made
scripts/deploy.shwith strict argument validation, dependency checks (lsof,bundle,jekyll), and safe command-array execution..github/workflows/teaching-content-checks.ymlto run markdownlint and prettier checks for_teaching/**/*.mdon PRs and main pushes.utils.jsloads beforemain.jsand removing duplicate footer script includes in research/team layouts.<tags>to.tagsacross_research/index.md, CSS, JS, andscripts/generate_filtered_research.rb.scripts/validate-content-rules.shplus.github/workflows/content-rules-checks.ymlto enforcehistory.mdordering rules and documentation/markup consistency..opencode/commands/add-news.md,CLAUDE.md, andREADME.mdto match current reverse-month ordering and.tagsconventions.Testing
bash -n scripts/deploy.sh./scripts/validate-content-rules.shnode --check assets/js/main.jsnode --check assets/js/command-data.jsruby -c scripts/generate_filtered_research.rbruby -e 'require "yaml"; YAML.load_file(".github/workflows/teaching-content-checks.yml")'ruby -e 'require "yaml"; YAML.load_file(".github/workflows/content-rules-checks.yml")'bundle exec jekyll build(fails locally: bundler 2.5.23 not installed in this environment)