fix(eval): emulate Actions ::add-mask:: on functional-test artifacts

[ascerra]

Summary

• Before uploading eval-results-* artifacts, scrub eval output trees by emulating GitHub Actions ::add-mask:: handling: register values from mask lines, replace them with *** everywhere, rewrite mask lines to ::add-mask::***
• Keeps defense-in-depth redaction for common GitHub token shapes / x-access-token URLs and deletes .eval-env
• Fails the scrub step if live secrets remain

Why

post-review / post-code / post-fix echo ::add-mask::<token> for the Actions job UI. Eval archives raw stdout without the runner’s masking, which leaked EVAL_GH_TOKEN into artifacts. Production review logs are masked by Actions; eval needs the same treatment on the artifact path. Triage never prints tokens, so it was already clean.

Test plan

• bash eval/scripts/scrub-eval-results.sh <dir> turns ::add-mask::ghp_… + later uses of that value into ***
• Functional-tests (review) artifact has no raw ::add-mask::ghp_ / ghp_ payloads

Standalone from #177 / #183 — merge this first; rebase those eval PRs onto main afterward to pick it up.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:08 PM UTC · Completed 5:18 PM UTC
Commit: 06b6dbc · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Scrub eval artifacts by emulating GitHub Actions ::add-mask:: redaction

🐞 Bug fix ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Scrub eval result trees before artifact upload using Actions-style ::add-mask:: masking.
• Delete .eval-env files and regex-redact common GitHub token patterns as defense-in-depth.
• Fail the CI scrub step if any secret-shaped payloads remain in artifacts.

Diagram

graph TD
  A["Functional tests job"] --> B["Scrub secrets step"] --> C["scrub-eval-results.sh"] --> D[("Eval output trees")] --> E["Upload artifact"]
  C --> F["Fail if leaks"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Stop archiving raw stdout (redact at source)

• ➕ Eliminates an entire class of leakage by not persisting sensitive log streams
• ➕ Less reliance on post-processing correctness across many file formats
• ➖ Reduces debuggability of eval runs (stdout is often essential)
• ➖ Requires broader changes to eval harness/logging and may be disruptive

2. Pure-regex scrub only (no ::add-mask:: registry emulation)

• ➕ Simpler implementation and fewer edge cases around mask-value collection
• ➕ Lower runtime cost for large artifact trees
• ➖ Misses non-token secrets that are masked via ::add-mask:: but don’t match token regexes
• ➖ Diverges from GitHub Actions masking contract; higher leak risk

3. Centralize artifact upload behind a wrapper action/script

• ➕ Ensures all artifact uploads get consistent scrubbing policies
• ➕ Easier to evolve scrubbing logic without touching many workflows
• ➖ Requires workflow refactors and adoption across repos/jobs
• ➖ More plumbing than needed for this targeted eval-artifact issue

Recommendation: Keep the PR’s approach: emulating Actions’ ::add-mask:: registry semantics is the most reliable way to prevent leaks of arbitrary masked values, and the fail-closed verification meaningfully reduces residual risk. The added defense-in-depth regexes and .eval-env deletion are appropriate as secondary protections; broader refactors (changing what is archived or introducing a wrapper action) can be considered later if more workflows need the same guarantees.

Files changed (2) +144 / -1

Bug fix (1) +143 / -0

scrub-eval-results.shNew Actions-style ::add-mask:: scrubber with leak detection +143/-0

New Actions-style ::add-mask:: scrubber with leak detection

• Adds a scrubber that collects values from ::add-mask::<value> lines and replaces those values with *** across text artifacts, rewriting mask lines to ::add-mask::***. Also deletes .eval-env files, applies token/URL redaction patterns, and fails the step if any secret-shaped payloads remain after rewriting.

eval/scripts/scrub-eval-results.sh

Other (1) +1 / -1

functional-tests.ymlInvoke eval artifact scrubber before uploading results +1/-1

Invoke eval artifact scrubber before uploading results

• Replaces the previous one-liner that only deleted .eval-env files with a call to the new eval results scrub script. Ensures scrubbing runs (when secrets are available) prior to the artifact upload step.

.github/workflows/functional-tests.yml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. Upload runs after scrub fail ✓ Resolved 🐞 Bug ⛨ Security

Description

In functional-tests.yml, the artifact upload step uses if: always(), so it will still upload
eval/runs/ even if scrub-eval-results.sh exits 1 after detecting remaining secrets. This can
publish artifacts that the scrubber explicitly determined still contain live secret material.

Code

.github/workflows/functional-tests.yml[R336-341]

      - name: Scrub secrets from eval results
        if: always() && steps.secrets-check.outputs.available == 'true'
-        run: find eval/runs/ -name '.eval-env' -delete 2>/dev/null || true; find /tmp/agent-eval/ -name '.eval-env' -delete 2>/dev/null || true
+        run: bash eval/scripts/scrub-eval-results.sh eval/runs /tmp/agent-eval

      - name: Upload eval results
        if: always() && steps.secrets-check.outputs.available == 'true'

Relevance

⭐⭐ Medium

No direct precedent on always()+upload gating; team likes fail-closed CI hardening (PRs #89,#94
accepted similar).

PR-#89
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The workflow shows both scrub and upload are unconditional under always(), while the scrubber
explicitly exits 1 when leaks remain, meaning the upload can proceed after a failed scrub.

.github/workflows/functional-tests.yml[336-348]
eval/scripts/scrub-eval-results.sh[132-136]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Upload eval results` is guarded with `always()`, so it runs even when the scrub step fails due to detected leaks. That defeats the “fail closed” behavior and can publish secret-bearing artifacts.

### Issue Context
The scrub script exits non-zero when `leaks` are found, but the next step still runs due to `always()`.

### Fix Focus Areas
- .github/workflows/functional-tests.yml[336-348]

### Suggested fix
1. Give the scrub step an `id` (e.g., `id: scrub_eval_results`).
2. Update the upload step condition to require scrub success, e.g.:
  - `if: always() && steps.secrets-check.outputs.available == 'true' && steps.scrub_eval_results.outcome == 'success'`
3. (Optional) If you still want artifacts when tests fail, keep `always()` but *only* allow upload when scrub succeeded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Leak snippet can expose tokens ✓ Resolved 🐞 Bug ⛨ Security

Description

On scrub failure, the script prints a surrounding text snippet for each match, but it only masks
ghp_ and ::add-mask::... in that snippet. If the leak is a gho_ or github_pat_ match (both
are in leak_re), the raw token can be emitted to CI logs.

Code

eval/scripts/scrub-eval-results.sh[R112-136]

+# Fail closed: no live add-mask payloads or classic PAT shapes should remain.
+leak_re = re.compile(
+    r"::add-mask::(?!\*\*\*)(\S+)"
+    r"|\bghp_[A-Za-z0-9_]{20,}"
+    r"|\bgho_[A-Za-z0-9_]{20,}"
+    r"|\bgithub_pat_[A-Za-z0-9_]{20,}"
+)
+leaks: list[str] = []
+for root in ROOTS:
+    for path in iter_text_files(root):
+        text = read_text(path)
+        if text is None:
+            continue
+        for m in leak_re.finditer(text):
+            snippet = text[max(0, m.start() - 20) : m.end() + 20].replace("\n", "\\n")
+            # Never print the secret itself in CI logs.
+            snippet = re.sub(r"ghp_[A-Za-z0-9_]+", "ghp_***", snippet)
+            snippet = re.sub(r"::add-mask::\S+", "::add-mask::***", snippet)
+            leaks.append(f"{path}: ...{snippet}...")
+
+if leaks:
+    print("::error::Secrets remain in eval results after Actions-style scrub:", file=sys.stderr)
+    for line in leaks[:50]:
+        print(line, file=sys.stderr)
+    sys.exit(1)

Relevance

⭐⭐ Medium

No prior findings on CI log snippet leaking tokens; security hardening often accepted (PR #89, #94).

PR-#89
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

leak_re searches for gho_ and github_pat_ tokens, but the diagnostic masking only replaces
ghp_... and ::add-mask::..., so other matched tokens can be printed verbatim.

eval/scripts/scrub-eval-results.sh[113-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The leak-reporting path prints nearby content for matches, but the masking applied to that snippet is incomplete. Tokens detected as `gho_...` or `github_pat_...` can be printed unredacted.

### Issue Context
`leak_re` matches multiple token types, but the snippet sanitization only rewrites `ghp_...` and `::add-mask::...`.

### Fix Focus Areas
- eval/scripts/scrub-eval-results.sh[112-136]

### Suggested fix
Pick one of the following safe approaches:
1. **Safest:** Do not print any matched content at all—only print the file path (and maybe the match kind), e.g. `leaks.append(str(path))`.
2. **If you keep snippets:** apply sanitization for *all* token families that `leak_re` can match (gho_, github_pat_, etc.), and consider just running the existing `redact()` function over the snippet before printing.

Also ensure the `::error::...` line itself contains no secret-derived data.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. functional-tests.yml protected-path change ✓ Resolved 📜 Skill insight § Compliance

Description

This PR modifies .github/workflows/functional-tests.yml, which is a protected
governance/infrastructure path and must be explicitly flagged for human review (must not be
auto-approved). Without an explicit in-file justification/link, reviewers cannot verify governance
intent for the workflow change.

Code

.github/workflows/functional-tests.yml[338]

+        run: bash eval/scripts/scrub-eval-results.sh eval/runs /tmp/agent-eval

Relevance

⭐⭐ Medium

Protected-path justification finding exists but outcome undetermined (PR #29); no consistent
enforcement evidence.

PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance checklist requires raising a finding for any modifications under protected
governance/infrastructure paths such as .github/. The diff shows a change to
.github/workflows/functional-tests.yml, triggering this rule.

.github/workflows/functional-tests.yml[336-341]
Skill: pr-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A protected-path workflow file under `.github/` was modified, which requires explicit justification and human review per compliance policy.

## Issue Context
The change is in a GitHub Actions workflow, which is considered governance/infrastructure and must be surfaced for manual approval. Add an explicit justification (ideally with a linked issue/ADR) close to the change so reviewers can quickly validate intent.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[336-341]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Verification omits redacted patterns ✓ Resolved 🐞 Bug ☼ Reliability

Description

The script’s redaction pass targets several token shapes (ghu_/ghs_/ghr_ and x-access-token URLs),
but the final “fail closed” leak_re does not check for those patterns. This weakens the guarantee
that the scrub step will fail if any of the handled secret shapes remain.

Code

eval/scripts/scrub-eval-results.sh[R34-118]

+TEXT_SUFFIXES = {".log", ".txt", ".json", ".jsonl", ".yaml", ".yml", ".md"}
+ADD_MASK_RE = re.compile(r"::add-mask::(\S+)")
+# Defense in depth — tokens that never went through ::add-mask::.
+TOKEN_RES = [
+    re.compile(r"\bghp_[A-Za-z0-9_]{20,}"),
+    re.compile(r"\bgho_[A-Za-z0-9_]{20,}"),
+    re.compile(r"\bghu_[A-Za-z0-9_]{20,}"),
+    re.compile(r"\bghs_[A-Za-z0-9_]{20,}"),
+    re.compile(r"\bghr_[A-Za-z0-9_]{20,}"),
+    re.compile(r"\bgithub_pat_[A-Za-z0-9_]{20,}"),
+    re.compile(r"x-access-token:[^@/\s]+@"),
+]
+
+def iter_text_files(root: Path):
+    if not root.is_dir():
+        return
+    for path in root.rglob("*"):
+        if not path.is_file():
+            continue
+        if path.name == ".eval-env":
+            try:
+                path.unlink()
+            except OSError:
+                pass
+            continue
+        if path.suffix.lower() in TEXT_SUFFIXES:
+            yield path
+
+def read_text(path: Path) -> str | None:
+    try:
+        return path.read_text(encoding="utf-8", errors="surrogateescape")
+    except OSError as e:
+        print(f"WARNING: could not read {path}: {e}", file=sys.stderr)
+        return None
+
+# Pass 1: collect secrets registered via ::add-mask:: (Actions-compatible).
+secrets: set[str] = set()
+for root in ROOTS:
+    for path in iter_text_files(root):
+        text = read_text(path)
+        if text is None:
+            continue
+        for match in ADD_MASK_RE.finditer(text):
+            value = match.group(1)
+            if value in ("***", "[REDACTED]", ""):
+                continue
+            secrets.add(value)
+
+# Longest first so overlapping prefixes redact correctly.
+ordered = sorted(secrets, key=len, reverse=True)
+
+def redact(text: str) -> str:
+    # Emulate Actions: mask line becomes ::add-mask::*** and the value is ***
+    # wherever it appears (including earlier lines in the same file).
+    for secret in ordered:
+        if secret and secret in text:
+            text = text.replace(secret, "***")
+    text = ADD_MASK_RE.sub("::add-mask::***", text)
+    for pat in TOKEN_RES:
+        if pat.pattern.startswith("x-access-token:"):
+            text = pat.sub("x-access-token:***@", text)
+        else:
+            # Keep a stable prefix hint without the secret material.
+            text = pat.sub(lambda m: m.group(0).split("_", 1)[0] + "_***", text)
+    return text
+
+# Pass 2: rewrite files.
+changed = 0
+for root in ROOTS:
+    for path in iter_text_files(root):
+        text = read_text(path)
+        if text is None:
+            continue
+        new = redact(text)
+        if new != text:
+            path.write_text(new, encoding="utf-8", errors="surrogateescape")
+            changed += 1
+
+# Fail closed: no live add-mask payloads or classic PAT shapes should remain.
+leak_re = re.compile(
+    r"::add-mask::(?!\*\*\*)(\S+)"
+    r"|\bghp_[A-Za-z0-9_]{20,}"
+    r"|\bgho_[A-Za-z0-9_]{20,}"
+    r"|\bgithub_pat_[A-Za-z0-9_]{20,}"
+)

Relevance

⭐⭐ Medium

No repo history on scrubber regex parity; only general fail-closed hardening accepted elsewhere (PRs
#89,#94).

PR-#89
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code shows additional token regexes in TOKEN_RES that are not included in the post-scrub
leak_re, so the end-of-script validation is narrower than the redaction scope.

eval/scripts/scrub-eval-results.sh[34-45]
eval/scripts/scrub-eval-results.sh[112-118]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The script redacts a broader set of secret patterns than it verifies at the end. This creates a maintenance/robustness gap: if a redaction rule regresses or misses a variant, the final validation may still pass.

### Issue Context
`TOKEN_RES` includes more patterns than `leak_re` checks.

### Fix Focus Areas
- eval/scripts/scrub-eval-results.sh[34-45]
- eval/scripts/scrub-eval-results.sh[112-118]

### Suggested fix
- Expand `leak_re` to include the same token families covered by `TOKEN_RES` (and any other sensitive URL forms you want to fail on).
- Alternatively, generate the verification regex directly from the same pattern list used for redaction (single source of truth) so they cannot diverge.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

All six prior findings have been addressed: ADD_MASK_RE uses (.+)$ with re.MULTILINE, leak_re joins TOKEN_PATTERNS for full redaction/verification parity, error-path output sanitizes :: sequences, directory passing uses newline delimitation, and token_hint preserves the github_pat_ discriminator.

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under the .github/ protected path with no linked issue. Protected-path modifications require traceable authorization. Human approval is always required for protected-path changes.
  Remediation: Link the PR to a tracking issue documenting the EVAL_GH_TOKEN leak incident.

Low

• [naming-convention] .github/workflows/functional-tests.yml:339 — Step ID scrub_eval_results uses underscores; the existing multi-word step ID secrets-check uses kebab-case.
  Remediation: Rename to scrub-eval-results and update the upload step condition reference.

• [missing-workflow-documentation] README.md:57 — Workflows section does not document functional-tests.yml (pre-existing gap).

• [missing-directory-documentation] README.md:18 — Repository structure section does not document eval/ (pre-existing gap).

Previous run

Review of PR #184 — fix(eval): emulate Actions ::add-mask:: on functional-test artifacts

Verdict: comment | Findings: 3 medium, 3 low

Summary

This PR replaces a simple find -delete step (which only removed .eval-env files) with a comprehensive scrub script that emulates GitHub Actions' ::add-mask:: behavior on eval artifact files before upload. The approach is sound and addresses a real secret-leak vector — eval archives raw stdout without the Actions runner's masking, leaking EVAL_GH_TOKEN into downloadable artifacts.

The implementation uses a two-pass Python strategy (collect mask values → redact everywhere) with defense-in-depth regex patterns for known GitHub token shapes, plus a verification pass that exits non-zero if secrets remain. This is a significant security improvement.

Three medium-severity findings identify gaps in the defense-in-depth layers. None undermine the primary redaction logic, but they weaken the "fail closed" guarantee the script explicitly aims for.

---

Findings

1. ⚠ Verification regex missing token prefixes — leak_re incomplete (medium)

File: eval/scripts/scrub-eval-results.sh ~line 108 | Category: incomplete-verification

The verification regex leak_re checks for ghp_, gho_, and github_pat_ tokens after scrubbing, but the redaction list TOKEN_RES also covers ghu_ (user-to-server), ghs_ (installation access), ghr_ (refresh), and x-access-token: URLs. If redaction fails for any of these token types (e.g., encoding edge case), the verification pass would exit 0 and the artifact would be uploaded with a live secret.

Remediation: Add the missing prefixes to leak_re:

leak_re = re.compile(
    r"::add-mask::(?!\*\*\*)(\S+)"
    r"|\bghp_[A-Za-z0-9_]{20,}"
    r"|\bgho_[A-Za-z0-9_]{20,}"
    r"|\bghu_[A-Za-z0-9_]{20,}"
    r"|\bghs_[A-Za-z0-9_]{20,}"
    r"|\bghr_[A-Za-z0-9_]{20,}"
    r"|\bgithub_pat_[A-Za-z0-9_]{20,}"
    r"|x-access-token:[^@/\s]+@"
)

2. ⚠ GHA workflow command injection in error-path snippets (medium)

File: eval/scripts/scrub-eval-results.sh ~line 107 | Category: gha-workflow-command-injection

When the leak detector fires, snippets from eval output files are printed via ::error:: to stderr. The snippet sanitization replaces ghp_* tokens and ::add-mask:: patterns but does NOT sanitize other GHA workflow commands (::set-env::, ::warning::, ::notice::). Since eval output content can be influenced by attacker-crafted fixtures (e.g., a PR that modifies test fixtures), an attacker could embed workflow commands in eval output that survive snippet sanitization.

Remediation: Sanitize all :: sequences in snippets before printing:

snippet = snippet.replace("::", ": :")

3. ⚠ ADD_MASK_RE uses \S+ — diverges from Actions' actual ::add-mask:: semantics (medium)

File: eval/scripts/scrub-eval-results.sh ~line 35 | Category: semantic-divergence

The regex ::add-mask::(\S+) captures only non-whitespace characters after the mask prefix. GitHub Actions' actual ::add-mask:: command masks everything to end-of-line. Verified: scripts/prepare-sandbox-credentials.sh line 29 emits ::add-mask::$OIDC_AUTH where $OIDC_AUTH is a GCP Authorization header value (typically Bearer <token> — contains a space). While this particular mask runs as a GHA step (not in eval output), the behavioral divergence means future ::add-mask:: lines with space-containing values in eval output would only have the pre-space portion scrubbed.

Remediation: Use re.compile(r"::add-mask::(.+)$", re.MULTILINE) and strip captured values before adding to the secrets set.

4. 🔵 Space-delimited directory passing fragile (low)

File: eval/scripts/scrub-eval-results.sh ~line 25 | Category: edge-case

EVAL_SCRUB_ROOTS="${ROOTS[*]}" joins directories with spaces, then Python splits on whitespace. Paths with spaces would break silently (non-existent fragments skipped by is_dir() check). Current callers use space-free paths.

5. 🔵 Missing linked issue (low)

Category: missing-authorization

Non-trivial PR (144 added lines) with no linked issue. The security-fix nature (fix(eval) prefix, addressing a token leak) provides reasonable implicit authorization, but issue tracking aids post-incident review.

6. 🔵 github_pat_ prefix hint truncated in redaction output (low)

File: eval/scripts/scrub-eval-results.sh ~line 88 | Category: edge-case

The redaction lambda m.group(0).split("_", 1)[0] + "_***" produces github_*** for github_pat_ tokens (losing the pat_ discriminator), while ghp_ tokens correctly produce ghp_***. Minor inconsistency in log forensics.

---

Labels: PR fixes a secret-leak bug in eval artifact handling.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:24 PM UTC · Ended 5:24 PM UTC
Commit: 5cd495a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:25 PM UTC · Completed 5:41 PM UTC
Commit: 1192ff7 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:03 PM UTC
Commit: 579fdeb · View workflow run →

[ben-alkov]

LGTM 🌮

[fullsend-ai-review]

Review skipped — this PR is already merged.

The /fs-review command only reviews open pull requests.

_{Posted by fullsend post-review check}