fix: address all 23 confirmed review findings (security, bugs, hardening)

Security
- CRITICAL: gate the /codeboarding (issue_comment) path on author_association
  (OWNER/MEMBER/COLLABORATOR) before any checkout/analysis — closes the
  pwn-request/secret-exfiltration hole; README example + security note updated.
- Pass base_ref/header/cta_base via env (not interpolated into bash); extract
  engine python -c into scripts/cb_engine.py (no ${{ }}->python-source interp);
  preflight prints only the error message, not the whole auth body.

Bugs (diff_to_mermaid.py)
- Escape Mermaid shape metacharacters (] [ ( ) { } | < > &) — a bare ] no longer
  breaks GitHub rendering.
- n_changed is now recursive + a `changed` flag covers relation/nested-only
  changes; the action shows the diagram on `rendered` instead of suppressing it.
- Size guard counts drawn edges and RETRIES changed-only before giving up.
- _filter_changed ignores empty id/name strings.
- Deleted parent keeps its subtree (renders as a deleted subgraph, symmetric
  with added).

action.yml hardening
- Guard: set -uo pipefail + graceful gh-api failure + assert base/head SHAs.
- Generate-base: worktree remove/prune (not just rm -rf); cache the generated
  base keyed by base SHA (pay the cold full-analysis once).
- Seed static_analysis.sha with the pkl (enables the committed-baseline
  warm-start that was previously unreachable).
- Health step writes 0 first (no stale count on exception).
- Post a failure comment (if: failure()) so a failed run isn't silent.

Workflows
- release-major-tag: anchor the semver regex ($) so prereleases don't move vN.
- test-self + README example: concurrency cancel-in-progress.

Tests: +cb_engine smoke tests, escaping/changed-flag cases (31 total, all green).

Author: brovatten

.github/workflows/release-major-tag.yml

@@ -29,8 +29,8 @@ jobs:
         run: |
           set -euo pipefail
           ver="${TAG#v}"
-          if ! printf '%s' "$ver" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+'; then
-            echo "::notice::Tag '$TAG' is not vMAJOR.MINOR.PATCH semver; skipping major-tag move."
+          if ! printf '%s' "$ver" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$'; then
+            echo "::notice::Tag '$TAG' is not a clean vMAJOR.MINOR.PATCH release (prerelease/suffix); skipping major-tag move."
             exit 0
           fi
           major="v${ver%%.*}"

.github/workflows/test-self.yml

@@ -10,6 +10,10 @@ on:
 permissions:
   pull-requests: write
 
+concurrency:
+  group: self-test-${{ github.event.pull_request.number }}
+  cancel-in-progress: true
+
 jobs:
   diagram:
     runs-on: ubuntu-latest

README.md

@@ -31,22 +31,32 @@ on:
 permissions:
   pull-requests: write         # the only permission needed — nothing is pushed
 
+# Cancel a superseded run when new commits land on the same PR (avoid stacking
+# multi-minute LLM jobs).
+concurrency:
+  group: codeboarding-${{ github.event.pull_request.number || github.event.issue.number }}
+  cancel-in-progress: true
+
 jobs:
   diagram:
     runs-on: ubuntu-latest
-    # Run on (non-draft) PR events, OR when someone comments "/codeboarding" on a PR.
-    # The if-gate is important: without it a runner spins up for every comment.
+    # Run on (non-draft) PR events, OR when a TRUSTED collaborator comments exactly
+    # "/codeboarding" on a PR. The if-gate matters: (1) without it a runner spins up
+    # for every comment; (2) the author_association check is a SECURITY gate — see below.
     if: >
       (github.event_name == 'pull_request' && github.event.pull_request.draft == false) ||
       (github.event_name == 'issue_comment' && github.event.issue.pull_request != null &&
-       startsWith(github.event.comment.body, '/codeboarding'))
+       (github.event.comment.body == '/codeboarding' || startsWith(github.event.comment.body, '/codeboarding ')) &&
+       contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association))
     timeout-minutes: 60
     steps:
       - uses: codeboarding/codeboarding-action@v1
         with:
           llm_api_key: ${{ secrets.OPENROUTER_API_KEY }}
 ```
 
+> ⚠️ **Security — the `author_association` gate is required.** `issue_comment` workflows run from your default branch **with full repository secrets, for any commenter**. Without the `OWNER`/`MEMBER`/`COLLABORATOR` check, anyone could comment `/codeboarding` on a fork PR and have the action check out and run the engine over their PR-head code with your `OPENROUTER_API_KEY` present (a "pwn request"). The action's guard enforces this too, but gate it at the workflow level so a runner never even starts for an untrusted commenter.
+
 You need **one secret**: an LLM API key. OpenRouter is the default; pass your own model via the `agent_model` / `parsing_model` inputs if you prefer.
 
 ### On-demand: the `/codeboarding` command

action.yml

@@ -47,26 +47,34 @@ def detect_editors(repo_path: Path) -> list[str]:
 
 
 def build_cta(cta_base: str, owner: str, repo: str, pr: str, repo_path: Path, issues: int = 0) -> str:
-    """Return the markdown CTA footer, or '' when ``cta_base`` is unset."""
-    if not cta_base:
-        return ""
-    base = cta_base.rstrip("/")
-
-    def link(path: str, **extra: str) -> str:
-        return f"{base}/{path}?" + urlencode({"owner": owner, "repo": repo, "pr": pr, **extra})
-
-    editor_links = " · ".join(
-        f"[**Open in {_EDITOR_LABEL[e]} →**]({link('open-in-editor', editor=e)})" for e in detect_editors(repo_path)
-    )
+    """Return the markdown CTA footer (the warning banner shows even without a proxy URL).
 
-    lines = ["", "---"]
+    The ⚠️ health banner is informational and needs no proxy, so it renders
+    whenever ``issues > 0``; the editor/marketplace links require ``cta_base``.
+    Returns '' only when there's nothing to show.
+    """
+    parts: list[str] = []
     if issues > 0:
         noun = "issue" if issues == 1 else "issues"
-        lines += [f"⚠️ **{issues} architecture {noun} found** — open CodeBoarding to explore them.", ""]
+        parts.append(f"⚠️ **{issues} architecture {noun} found** — open CodeBoarding to explore them.")
 
-    lines += [f"🧭 See this architecture in your editor: {editor_links}", ""]
+    if cta_base:
+        base = cta_base.rstrip("/")
 
-    lines += [f"💡 New to CodeBoarding? [**Get the extension →**]({link('use-marketplace')})"]
+        def link(path: str, **extra: str) -> str:
+            return f"{base}/{path}?" + urlencode({"owner": owner, "repo": repo, "pr": pr, **extra})
+
+        editor_links = " · ".join(
+            f"[**Open in {_EDITOR_LABEL[e]} →**]({link('open-in-editor', editor=e)})" for e in detect_editors(repo_path)
+        )
+        parts.append(f"🧭 See this architecture in your editor: {editor_links}")
+        parts.append(f"💡 New to CodeBoarding? [**Get the extension →**]({link('use-marketplace')})")
+
+    if not parts:
+        return ""
+    lines = ["", "---"]
+    for p in parts:
+        lines += ["", p]
     return "\n".join(lines)
 
 

scripts/__pycache__/build_cta.cpython-310.pyc

@@ -0,0 +1,129 @@
+"""Engine orchestration for the action — extracted from inline ``python -c`` blocks
+in action.yml so it is checked in, reviewable, and unit-testable.
+
+Subcommands (all paths/refs come in as argv, never interpolated into source):
+
+  base    --repo P --out D --name N --run-id ID --depth K --source-sha SHA
+  head    --repo P --out D --name N --run-id ID --depth K --base-ref B --target-ref T --source-sha SHA
+  health  --artifact-dir D --repo P --name N --issues-out FILE
+
+``base`` runs a full analysis; ``head`` runs incremental, falling back to full on
+``IncrementalCacheMissingError``/``BaselineUnavailableError``; ``health`` writes the
+WARNING/CRITICAL finding count to ``--issues-out`` (and never fails the run).
+
+The engine (``codeboarding_workflows`` etc.) is imported lazily inside each
+function so this module imports without the engine venv present — the tests stub
+those modules and assert we call the engine with the right arguments.
+"""
+
+from __future__ import annotations
+
+import argparse
+from pathlib import Path
+
+_BASE_LOG = "/tmp/cb-base.log"
+_HEAD_LOG = "/tmp/cb-head.log"
+
+
+def run_base(repo: str, out: str, name: str, run_id: str, depth: int, source_sha: str) -> None:
+    from codeboarding_workflows.analysis import run_full
+
+    res = run_full(
+        repo_name=name,
+        repo_path=Path(repo),
+        output_dir=Path(out),
+        run_id=run_id,
+        log_path=_BASE_LOG,
+        depth_level=int(depth),
+        source_sha=source_sha,
+    )
+    print(f"Base analysis written: {res}")
+
+
+def run_head(repo: str, out: str, name: str, run_id: str, depth: int, base_ref: str, target_ref: str, source_sha: str) -> None:
+    from codeboarding_workflows.analysis import BaselineUnavailableError, run_full, run_incremental
+    from diagram_analysis.exceptions import IncrementalCacheMissingError
+
+    try:
+        res = run_incremental(
+            repo_path=Path(repo),
+            output_dir=Path(out),
+            project_name=name,
+            run_id=run_id,
+            log_path=_HEAD_LOG,
+            base_ref=base_ref,
+            target_ref=target_ref,
+            source_sha=source_sha,
+        )
+    except (IncrementalCacheMissingError, BaselineUnavailableError) as exc:
+        print(f"Incremental unavailable ({exc}); running full analysis on head.")
+        for p in Path(out).glob("*"):
+            if p.is_file():
+                p.unlink()
+        res = run_full(
+            repo_name=name,
+            repo_path=Path(repo),
+            output_dir=Path(out),
+            run_id=run_id,
+            log_path=_HEAD_LOG,
+            depth_level=int(depth),
+            source_sha=source_sha,
+        )
+    print(f"Head analysis written: {res}")
+
+
+def run_health(artifact_dir: str, repo: str, name: str) -> int:
+    """Return the WARNING/CRITICAL finding count; 0 on any failure (best-effort)."""
+    try:
+        from health.models import Severity
+        from health.runner import run_health_checks
+        from static_analyzer.analysis_cache import StaticAnalysisCache
+    except Exception as exc:  # engine without the health module
+        print(f"Health check skipped ({exc}).")
+        return 0
+    try:
+        cache = StaticAnalysisCache(artifact_dir=Path(artifact_dir), repo_root=Path(repo))
+        sa = cache.get()
+        issues = 0
+        if sa is not None:
+            report = run_health_checks(sa, repo_name=name, repo_path=Path(repo))
+            if report is not None:
+                for cs in report.check_summaries:
+                    for fg in getattr(cs, "finding_groups", []):
+                        if getattr(fg, "severity", None) in (Severity.WARNING, Severity.CRITICAL):
+                            issues += len(fg.entities)
+        print(f"Architecture issues found: {issues}")
+        return issues
+    except Exception as exc:
+        print(f"Health check skipped ({exc}).")
+        return 0
+
+
+def main(argv=None) -> int:
+    p = argparse.ArgumentParser(description=__doc__)
+    sub = p.add_subparsers(dest="cmd", required=True)
+
+    b = sub.add_parser("base")
+    for a in ("--repo", "--out", "--name", "--run-id", "--depth", "--source-sha"):
+        b.add_argument(a, required=True)
+
+    h = sub.add_parser("head")
+    for a in ("--repo", "--out", "--name", "--run-id", "--depth", "--base-ref", "--target-ref", "--source-sha"):
+        h.add_argument(a, required=True)
+
+    hc = sub.add_parser("health")
+    for a in ("--artifact-dir", "--repo", "--name", "--issues-out"):
+        hc.add_argument(a, required=True)
+
+    args = p.parse_args(argv)
+    if args.cmd == "base":
+        run_base(args.repo, args.out, args.name, args.run_id, args.depth, args.source_sha)
+    elif args.cmd == "head":
+        run_head(args.repo, args.out, args.name, args.run_id, args.depth, args.base_ref, args.target_ref, args.source_sha)
+    elif args.cmd == "health":
+        Path(args.issues_out).write_text(str(run_health(args.artifact_dir, args.repo, args.name)))
+    return 0
+
+
+if __name__ == "__main__":
+    raise SystemExit(main())