fix: never let a detected credential leave the process

Addresses the secret-handling half of the review on #22.

A secret finding is the one type where the alert hands us the thing we
must not repeat, and where the pull request is not the remediation: the
credential was committed, so it is in git history and live whether or
not the PR merges. Only rotation fixes it.

The PR is still opened — that is what makes the exposure visible and
gets it actioned — but it is no longer allowed to imply it is the fix.
The body now opens with a warning saying so in terms, and rotation is
injected as the first manual step deterministically rather than left to
the impact model to think of, so it reaches the webhook too.

The credential itself is scrubbed at every point where it would leave
the repository's access boundary (redact.py, one chokepoint per path):
both model prompts, the impact model's prose on the way back, the PR
title, body and commit message, and NotificationPayload — which covers
the run log and the Slack webhook, and means a backend added later
inherits the scrubbing instead of having to remember it.

The commit diff is the deliberate exception: a commit that removes a
line renders that line, and no wording changes what `git show` prints.
That residue is exactly what the PR warning exists to declare.

Candidates come from the alert's code_snippet, longest first — the
assignment's quoted value, other quoted literals, and the whole line as
a diff contains it. Keys are never candidates: `API_KEY = "sk-live-…"`
yields the value, never `API_KEY`, because the variable name is what
the PR has to tell an operator to set. Only secret findings build a
live redactor; blanking a SAST or CVE snippet would blind gate 2 to the
lines it has to judge.

Also from the same comment: the sanity gate's secret-pattern check ran
only for `secret` findings, so a CVE bump that pasted a token into a
lockfile was never checked — the module docstring's "no new secrets"
was true for one type in four. It now runs for every type.

And secret.md promised "never commit the actual secret value, even to
remove it", which is not achievable by construction. It now says what
actually happens and what the agent must never author.

32 new tests; 403 total.

Author: IgorEulalio

plugins/security-engineer/HARNESS.md

@@ -118,7 +118,7 @@ the commit will contain.
 
 | # | Gate | Checks | On failure |
 |---|---|---|---|
-| 1 | **sanity** (`validator.sanity_check`) | Diff is non-empty; within the type's line budget (cve 200, sast 100, iac/secret 50); for `secret` findings, no added line matches a secret pattern; the agent's own `diff_summary` does not name a version its diff never added | `FAILED`, no PR |
+| 1 | **sanity** (`validator.sanity_check`) | Diff is non-empty; within the type's line budget (cve 200, sast 100, iac/secret 50); no added line matches a secret pattern, **whatever the finding type** — a CVE bump that pastes a token into a lockfile fails here too; the agent's own `diff_summary` does not name a version its diff never added | `FAILED`, no PR |
 | 2 | **llm** (`validator.llm_validate`) | A single-shot model call: does this diff address this alert? `pass` / `fail` / `uncertain` | `fail` → `FAILED`. `uncertain` → passes, PR labelled `needs-review` |
 | 3 | **verify** (`pipeline.verify`) | **CVE:** the manifest still declares the package; it is no longer on the version the alert was raised against; if the agent chose a different version, that version carries no known advisory; a lockfile beside the manifest agrees; then `go build ./...` (Go) or `cargo metadata --locked` (Cargo). **Other types:** language build check (below) | `FAILED`, no PR |
 | 4 | **orca** (`validator.orca_check_gate`) | Polls the Orca GitHub App check on the PR head SHA. On failure, pulls the check **annotations** — file, line, message | Reverts the worktree to the branch head, re-invokes the fix agent *with the annotations as feedback*, re-runs gates 1 and 3, and pushes a follow-up commit to the same PR branch. After `max_retries`, follows `on_failure` |
@@ -205,6 +205,46 @@ fix agents.
 
 ---
 
+## Secret findings
+
+A secret finding is the one type where the alert hands the harness the thing it
+must not repeat, and where the pull request is **not** the remediation. The
+credential was committed: it is in git history and live regardless of whether the
+PR merges. Only rotation at the provider fixes it.
+
+The PR is still opened, because it is what makes the exposure visible and gets it
+actioned — but it is not allowed to imply it is the fix:
+
+- The body opens with a warning saying in terms that this does not remediate,
+  that the credential remains in history and live, and that rotation comes first.
+- Rotation is injected as the **first** manual step (`_with_rotation_step`),
+  deterministically rather than left to the impact model to think of — so it
+  reaches the webhook payload as well as the PR body.
+
+**What is redacted, and what cannot be.** The credential is already in the repo,
+so the PR diff adds no audience the repository did not already have. What a run
+adds is the paths that leave the repository's access boundary, and those are
+closed at the point of egress (`redact.py`):
+
+| Path | Redacted |
+|---|---|
+| `llm_validate` prompt → model | Yes — whole prompt, including `alert_json` |
+| `analyze_impact` prompt → model | Yes — whole prompt |
+| Impact `description` / `concerns` / `manual_steps` → PR body | Yes, on parse |
+| PR title and body → GitHub, watch notifications | Yes, immediately before `open-pr` |
+| `NotificationPayload` → console, run log, webhook | Yes, in `_notify_payload` — one chokepoint, so a backend added later inherits it |
+| The commit diff itself | **No — not possible.** A commit that removes a line renders that line. This is what the PR warning exists for |
+
+Candidates come from the alert's `code_snippet`, longest first: the quoted value
+on the right of an assignment, any other quoted literal, and the whole stripped
+line (which is the form a unified diff contains). Keys are never candidates —
+`API_KEY = "sk-live-…"` yields the value, never `API_KEY`, because the variable
+name is exactly what the PR has to tell an operator to set.
+
+Only secret findings build a live redactor. For a SAST or CVE alert the
+`code_snippet` is ordinary vulnerable code, and scrubbing it from the diff would
+blind gate 2 to the lines it has to judge.
+
 ## Degradation policy
 
 A harness that fails closed on every unknown never finishes; one that fails open

plugins/security-engineer/Makefile

@@ -15,6 +15,7 @@ test:
 	python3 skills/run/tests/test_version_data.py
 	python3 skills/run/tests/test_package_identity.py
 	python3 skills/run/tests/test_pipelines.py
+	python3 skills/run/tests/test_redact.py
 	python3 tools/tests/test_check_manifests.py
 
 # Static checks. CI (.github/workflows/ci.yml at the repository root) runs this

plugins/security-engineer/skills/run/fix-agents/secret.md

@@ -87,7 +87,8 @@ ENV API_KEY=""
 
 ## Important Notes
 
-- **Never** commit the actual secret value, even to remove it — the PR body should only describe the type (e.g. "API key", "database password"), not the value itself.
+- **Removing the secret is not remediation.** The credential was committed, so it stays in git history and stays live until somebody rotates it at the provider. The pull request is cleanup and a visible prompt to rotate — never describe it as the fix.
+- **Never write the value into anything you author.** Not the PR body, not the commit message, not the branch name — describe the type only (e.g. "API key", "database password"). The removal diff is the one place the value unavoidably still appears: a deleted line is rendered by `git`, and no wording changes that. The orchestrator redacts known values from the PR body, the run log, the webhook payload and both model prompts, but treat that as a backstop, not a licence.
 - If the secret appears in multiple files, fix all occurrences in the same branch/PR.
 - The PR body must list every env var name that needs to be set by operators after merge.
 

plugins/security-engineer/skills/run/impact_agent.py

@@ -10,6 +10,7 @@
 from dataclasses import dataclass, field
 
 from _json_util import find_last_json_with_key
+from redact import build_redactor
 from validator import (
     _SINGLE_SHOT_CONTRACT,
     _SINGLE_SHOT_MAX_TURNS,
@@ -133,12 +134,16 @@ def analyze_impact(
     fix_context: a specialist's FixPlan.metadata, when the type produced one.
                  Facts the model would otherwise have to infer from the diff.
     """
+    redactor = build_redactor(alert_json)
     prompt = _PROMPT.format(
         alert_json=json.dumps(alert_json, indent=2),
-        diff_text=diff_text[:6000],
+        diff_text=redactor(diff_text)[:6000],
         fix_context=_render_fix_context(fix_context),
         contract=_SINGLE_SHOT_CONTRACT,
     )
+    # alert_json and fix_context are in here too, and for a secret finding the
+    # alert's code_snippet is the credential. Scrub the assembled prompt.
+    prompt = redactor(prompt)
     # No tools at all — see _SINGLE_SHOT_TOOL_FLAGS for why denying them was
     # not the same thing, and cost 6x more.
     cmd = [
@@ -163,7 +168,7 @@ def analyze_impact(
     if result.returncode != 0:
         # claude reports its own failures on stdout, not stderr — see
         # _subprocess_error_detail. Logging stderr alone printed an empty string.
-        detail = _subprocess_error_detail(result)
+        detail = redactor(_subprocess_error_detail(result))
         print(f"[WARN] impact analysis failed (exit={result.returncode}): {detail}")
         return ImpactResult(
             level="medium",
@@ -172,10 +177,18 @@ def analyze_impact(
             error=f"exit_code={result.returncode}: {detail}",
         )
 
-    return _parse(result.stdout)
+    return _parse(result.stdout, redactor)
 
 
-def _parse(raw: str) -> ImpactResult:
+def _parse(raw: str, redactor=None) -> ImpactResult:
+    """Parse the impact verdict.
+
+    redactor: description, concerns and manual_steps are pasted verbatim into the
+              PR body and the webhook payload, so the model's prose is an egress
+              path like any other. A model explaining why removing a hardcoded
+              credential is risky can name the credential.
+    """
+    redactor = redactor or build_redactor(None)
     try:
         envelope = json.loads(raw)
         text = envelope.get("result", "") or raw
@@ -184,7 +197,7 @@ def _parse(raw: str) -> ImpactResult:
 
     data = find_last_json_with_key(text, "level")
     if not data:
-        snippet = text[:200] if text else "(empty)"
+        snippet = redactor(text[:200]) if text else "(empty)"
         print(f"[WARN] could not parse impact analysis output: {snippet}")
         return ImpactResult(
             level="medium",
@@ -195,9 +208,9 @@ def _parse(raw: str) -> ImpactResult:
 
     return ImpactResult(
         level=data.get("level", "medium"),
-        description=data.get("description", ""),
+        description=redactor(data.get("description", "")),
         downtime_risk=bool(data.get("downtime_risk", False)),
         requires_deploy=bool(data.get("requires_deploy", True)),
-        manual_steps=data.get("manual_steps") or [],
-        concerns=data.get("concerns") or [],
+        manual_steps=redactor.scrub(data.get("manual_steps") or []),
+        concerns=redactor.scrub(data.get("concerns") or []),
     )

plugins/security-engineer/skills/run/orchestrator.py

@@ -39,6 +39,7 @@
     repos_with_cve,
 )
 from pipelines import FixPlan, get_pipeline
+from redact import build_redactor
 from validator import ci_gate, llm_validate, orca_check_gate, sanity_check, worktree_diff
 
 _RUN_AGENT = str(_THIS_DIR / "run_agent.py")
@@ -503,13 +504,49 @@ def _what_changed(task: AlertTask) -> str:
     return "\n".join(lines)
 
 
+_SECRET_WARNING = """\
+> [!WARNING]
+> **This pull request does not remediate the exposure.**
+>
+> The credential was committed, so it is still in this repository's git history
+> and still live whether or not this merges. Removing it from the working tree is
+> cleanup, not remediation — and the diff below necessarily shows the value being
+> removed.
+>
+> **Rotate the credential first.** Revoke the old value at its provider, confirm
+> the replacement works, and only then merge this. Until rotation happens, treat
+> the secret as compromised.
+
+"""
+
+# The one step that actually fixes a secret finding. Generated deterministically
+# rather than left to the impact model, which may or may not think of it — and it
+# rides along to the webhook, so the Slack message carries it too.
+_ROTATE_STEP = "Rotate the exposed credential at its provider, then verify the old value is revoked"
+
+
+def _with_rotation_step(impact: ImpactResult | None, feature_type: str) -> ImpactResult | None:
+    """Put rotation at the top of the manual steps for a secret finding."""
+    if impact is None or feature_type != "secret":
+        return impact
+    steps = list(impact.manual_steps or [])
+    if not any("rotat" in step.lower() for step in steps):
+        impact.manual_steps = [_ROTATE_STEP, *steps]
+    return impact
+
+
 def _commit_and_pr(task: AlertTask, impact: ImpactResult | None, dry_run: bool) -> str | None:
     """Stage, commit, and open PR. Returns PR URL or None (dry-run)."""
     if dry_run:
         print(f"  [dry-run] would commit and open PR for {task.alert_id}")
         return None
 
-    commit_msg = f"fix(security): {task.title} ({task.alert_id})"
+    # Built once here and applied to everything this function authors. The diff
+    # itself cannot be scrubbed — a commit that removes a line renders that line —
+    # which is what _SECRET_WARNING exists to say out loud.
+    redactor = build_redactor(task.alert_json)
+
+    commit_msg = redactor(f"fix(security): {task.title} ({task.alert_id})")
 
     impact_level = impact.level if impact else "unknown"
     impact_desc = impact.description if impact else ""
@@ -525,7 +562,10 @@ def _commit_and_pr(task: AlertTask, impact: ImpactResult | None, dry_run: bool)
             f"- {c}" for c in impact.concerns
         )
 
+    warning = _SECRET_WARNING if task.feature_type == "secret" else ""
+
     pr_body = (
+        f"{warning}"
         f"## Security Fix: {task.title}\n\n"
         f"**Alert:** `{task.alert_id}`  |  "
         f"**Risk:** {task.risk_level}  |  "
@@ -539,6 +579,11 @@ def _commit_and_pr(task: AlertTask, impact: ImpactResult | None, dry_run: bool)
     )
     pr_title = f"fix(security): {task.title[:60]} [{task.alert_id}]"
 
+    # Covers the impact model's prose and the fix agent's own diff_summary in one
+    # pass, before the body reaches GitHub and the watch notifications it sends.
+    pr_body = redactor(pr_body)
+    pr_title = redactor(pr_title)
+
     _run(["python3", _RUN_AGENT, "git-commit", task.alert_id, commit_msg],
          cwd=task.worktree_path)
 
@@ -581,18 +626,26 @@ def _revert(worktree_path: Path) -> None:
 
 
 def _notify_payload(task: AlertTask) -> NotificationPayload:
+    """Build the payload for one alert, scrubbed.
+
+    Every notification in the run is built here, which makes this the single
+    place a credential could reach LogFileNotifier's file on disk or
+    WebhookNotifier's POST to Slack. Scrubbing at the source means a backend
+    added later inherits it instead of having to remember.
+    """
+    redactor = build_redactor(task.alert_json)
     return NotificationPayload(
         event="",
         alert_id=task.alert_id,
         feature_type=task.feature_type,
         risk_level=task.risk_level,
         repo="",
         pr_url=task.pr_url,
-        reason=task.failure_reason,
+        reason=redactor(task.failure_reason),
         impact_level=task.impact.level if task.impact else None,
-        manual_steps=task.impact.manual_steps if task.impact else [],
-        concerns=task.impact.concerns if task.impact else [],
-        error_detail=task.impact.error if task.impact else None,
+        manual_steps=redactor.scrub(task.impact.manual_steps if task.impact else []),
+        concerns=redactor.scrub(task.impact.concerns if task.impact else []),
+        error_detail=redactor(task.impact.error if task.impact else None),
     )
 
 
@@ -720,7 +773,9 @@ def _run_pipeline(task: AlertTask, dry_run: bool, notifier, repo: Repository) ->
         task.state = "DONE"
         p = _notify_payload(task)
         p.repo = repo.name
-        p.detail = fix_result.diff_summary or ""
+        # The fix agent's own account of what it would do. For a secret finding
+        # that prose can quote the credential, and dry-run still notifies.
+        p.detail = build_redactor(task.alert_json)(fix_result.diff_summary or "")
         notifier.notify("fix_planned", p)
         return task
 
@@ -771,8 +826,10 @@ def _run_pipeline(task: AlertTask, dry_run: bool, notifier, repo: Repository) ->
     task.state = "IMPACT_ANALYSIS"
     print(f"[PHASE] {task.alert_id} production impact analysis", flush=True)
     diff_text = _get_diff(task.worktree_path)
-    task.impact = analyze_impact(task.alert_json, diff_text,
-                                 fix_context=plan.metadata or None)
+    task.impact = _with_rotation_step(
+        analyze_impact(task.alert_json, diff_text,
+                       fix_context=plan.metadata or None),
+        task.feature_type)
     if task.impact.error:
         print(f"[WARN] {task.alert_id} impact analysis error: {task.impact.error}")