Remediate R1/R2/R3 per review-v1.1

R1: remove gate.artifact multi-version guard (schema superRefine),
delete artifact-guard test, docs to recommendation (README+SPEC).
R2: collectSecretValues returns only live.keys, MIN 4->8,
scrub JSON-escaped form; update evidence-scrub tests.
R3: action.yml resolved-dir guard (pwd -P vs GITHUB_WORKSPACE),
result=fail on reject, allow absolute-inside; update
action-working-dir tests + README/SPEC.

Triad: programmer->QA PASS (316 pass)->Drift Guard VERDICT:PASS
This commit is contained in:
Agency Coordinator committed 2026-10-18 01:14:21 -04:00
1 parent 8422645da7
commit eaf6e21063
8 files changed
+183 -134

No files matched your search

+4 -4
View File
@@ -299,9 +299,9 @@ is true, and `--gate` was passed or `GITHUB_ACTIONS == "true"`. Without
**Matrix builds:** artifact names must be unique within a workflow run, so
give each matrix leg a distinct `artifact-name` Action input. The Action
passes it to the CLI as `--gate-artifact`, so each leg reads its own
history; no config change is needed. This is enforced: validation fails
when `gate.artifact` is the default `opencode-plugin-ci-results` while
`opencode.versions` has more than one entry. Example:
history; no config change is needed. Matrix legs SHOULD use a distinct
artifact-name (passed as `--gate-artifact`) so each leg reads its own
history; no config change is needed, and validation does NOT enforce this. Example:
```yaml
strategy:
@@ -318,7 +318,7 @@ steps:
`Heretek-AI/opencode-v2-plugin-teser@v1` (see [action.yml](action.yml)).
Inputs `tiers`, `opencode-versions` (overrides config), `working-directory`
(must stay inside `GITHUB_WORKSPACE`; `..` and absolute paths fail closed),
(must stay inside `GITHUB_WORKSPACE`; absolute paths inside the workspace are fine, paths outside exit 2),
`upload-artifact` (default `true`), `artifact-name` (default
`opencode-plugin-ci-results`; also the gate's history name). There is no
`config` input: the tool reads `.opencode-ci/config.*` in
+6 -3
View File
@@ -9,7 +9,7 @@ inputs:
description: "Overrides config, comma-separated"
default: ""
working-directory:
description: "Plugin repo directory; must be inside GITHUB_WORKSPACE (.. and absolute paths are unsupported)"
description: "Plugin repo directory; must stay inside GITHUB_WORKSPACE (absolute paths inside the workspace are fine)"
default: "."
upload-artifact:
description: "Upload evidence + results"
@@ -47,9 +47,12 @@ runs:
ACTION_PATH: ${{ github.action_path }}
run: |
# OCI-6.3-working-dir: working-directory must stay inside the
# workspace; `..` or absolute paths would escape GITHUB_WORKSPACE.
# workspace. The step has already changed into working-directory,
# so $PWD is the resolved path; compare it against the resolved
# workspace root. Absolute paths inside the workspace are fine.
# Checked via env passthrough to keep the zero-interpolation invariant.
case "$IN_WD" in /*|*..*) echo "working-directory must be inside the workspace (got: $IN_WD)" >&2; exit 2;; esac
ws="$(cd "$GITHUB_WORKSPACE" && pwd -P)"
case "$(pwd -P)/" in "$ws"/*) ;; *) echo "working-directory must be inside the workspace (got: $IN_WD)" >&2; echo "result=fail" >> "$GITHUB_OUTPUT"; exit 2;; esac
args=()
IFS=',' read -ra T <<< "$IN_TIERS"; for t in "${T[@]}"; do args+=(--tier "$t"); done
if [ -n "$IN_VERSIONS" ]; then IFS=',' read -ra V <<< "$IN_VERSIONS"; for v in "${V[@]}"; do args+=(--opencode "$v"); done; fi
+3 -3
View File
@@ -187,7 +187,7 @@ gate:
enabled: true # historical flake gate, live tier only (7.2)
bootstrap: fail # fail | allow ("no history yet" passes)
workflow: plugin-ci.yml # optional; else from GITHUB_WORKFLOW_REF
artifact: opencode-plugin-ci-results # history artifact; `run --gate-artifact` overrides it (must be distinct per leg when versions > 1)
artifact: opencode-plugin-ci-results # history artifact; `run --gate-artifact` overrides it
logLevel: info # info | all
```
@@ -312,7 +312,7 @@ This is ported from `IUMBTEMS/scripts/e2e-gate.ts` and applies only to live-tier
- **Acknowledgments**: `.opencode-ci/acknowledgments.json` in the plugin repo has the format `{ "version": 1, "acknowledgments": [{ "case", "reason", "issue", "date" }] }`. It is human-written; the tool never writes it. It is fail-closed: a malformed file fails the gate.
- **No history**: a red live tier with no history to judge it against exits 2. The Action reports this as a failure unless `gate.bootstrap: allow` is set, for the first runs in a new repo.
- **Local runs**: the gate is skipped locally unless `--gate` is passed. It then needs `gh`, a token (`GH_TOKEN` or `GITHUB_TOKEN`), `GITHUB_REPOSITORY` and a branch (`GITHUB_BASE_REF` or `GITHUB_REF_NAME`); without them it falls back to "fail on any live failure".
- **Matrix builds**: artifact names must be unique within a workflow run, so each leg sets its own `artifact-name`, and that same name is the leg's gate history. No per-leg config is needed. Validation enforces this: the default `gate.artifact` with more than one `opencode.versions` entry is a configuration error.
- **Matrix builds**: artifact names must be unique within a workflow run, so each leg sets its own `artifact-name`, and that same name is the leg's gate history. No per-leg config is needed. Matrix legs SHOULD use a distinct `artifact-name` so each leg reads its own history; validation does NOT enforce this.
## 8. Packaging
@@ -349,7 +349,7 @@ jobs:
OPENCODE_API_KEY: ${{ secrets.OPENCODE_API_KEY }}
```
Inputs are `tiers`, `opencode-versions` (overrides config), `working-directory` (must stay inside `GITHUB_WORKSPACE`; `..` and absolute paths exit 2), `upload-artifact` (default true) and `artifact-name` (default `opencode-plugin-ci-results`; passed to the CLI as `--gate-artifact`). There is no `config` input: the CLI reads `.opencode-ci/config.*` in `working-directory`. Outputs are `result` (`pass` when the CLI exits 0, else `fail`; a fully skipped run is `pass`) and `report-path`. Inputs reach the script only through `env:`, never through `${{ }}` inside `run:`.
Inputs are `tiers`, `opencode-versions` (overrides config), `working-directory` (must stay inside `GITHUB_WORKSPACE`; absolute paths inside the workspace are fine, paths outside exit 2), `upload-artifact` (default true) and `artifact-name` (default `opencode-plugin-ci-results`; passed to the CLI as `--gate-artifact`). There is no `config` input: the CLI reads `.opencode-ci/config.*` in `working-directory`. Outputs are `result` (`pass` when the CLI exits 0, else `fail`; a fully skipped run is `pass`) and `report-path`. Inputs reach the script only through `env:`, never through `${{ }}` inside `run:`.
The docs must state plainly: never run this on `pull_request_target` with the PR's code checked out and secrets exposed. The live tier runs the plugin under test with the real key.
-18
View File
@@ -89,24 +89,6 @@ export const ConfigSchema = z
message: `live.pass (${val.live.pass}) must be <= live.repeat (${val.live.repeat})`,
});
}
// OCI-6.2-artifact-poisoning: matrix legs uploading the same default
// artifact name collide (upload-artifact merges by name), so allowed
// `latest` results can poison required-leg gate history. Fail closed
// when the default name is shared across versions; per-leg distinct
// names (README matrix example) pass.
if (
val.opencode.versions.length > 1 &&
val.gate.artifact === DEFAULT_RESULTS_ARTIFACT
) {
ctx.addIssue({
code: "custom",
path: ["gate", "artifact"],
message:
`gate.artifact must not be the default ${JSON.stringify(DEFAULT_RESULTS_ARTIFACT)} ` +
"when opencode.versions has more than one entry: give each matrix leg " +
"a distinct artifact name (e.g. opencode-plugin-ci-results-2.0.26).",
});
}
});
export type Config = z.infer<typeof ConfigSchema>;
+47 -33
View File
@@ -24,61 +24,75 @@ export function excerpt(value: unknown, max: number = TRUNCATE_MAX): string {
}
/**
* Env names that mark a value as secret-bearing. Mirrors the probe's
* SECRET_KEY regex in `probe/core.js`: the probe redacts by NAME at the
* source, while `redactSecretValues` below scrubs by VALUE as defense in
* depth on persisted evidence.
* Env names that used to mark a value as secret-bearing. Kept for API
* compatibility; no longer used for scrubbing.
*
* @deprecated Host-env scanning was removed: it scrubbed innocuous values
* (e.g. `TOKENIZERS_PARALLELISM=false`) out of persisted evidence and broke
* JSONL files. Scrubbing now covers only explicitly passed live-key values.
*/
export const SECRET_NAME_RE = /KEY|TOKEN|SECRET|PASSWORD|AUTH|COOKIE/i;
/**
* Minimum secret length worth scrubbing. Shorter values (a one-char flag, a
* port number) would nuke unrelated evidence text.
* port number, or common words like "false") would nuke unrelated evidence
* text.
*/
const MIN_SECRET_LENGTH = 4;
const MIN_SECRET_LENGTH = 8;
/**
* Replace every occurrence of each known secret value with `[redacted]`.
* Longer secrets go first so overlapping values scrub completely. Empty,
* whitespace-only and short values are skipped. Never throws: returns the
* text unchanged on malformed input. Split/join (not regex) so values with
* regex metacharacters are matched literally.
* Longer secrets go first so overlapping values scrub completely, and each
* value's JSON-escaped form (`JSON.stringify(value).slice(1, -1)`) is
* scrubbed too, so secrets containing `"` or `\` are still caught inside
* JSONL lines. Empty, whitespace-only and short values are skipped. Never
* throws: returns the text unchanged on malformed input. Split/join (not
* regex) so values with regex metacharacters are matched literally.
*/
export function redactSecretValues(text: string, secrets: string[]): string {
let values: string[];
try {
values = [...new Set(secrets)].filter(
(secret) =>
typeof secret === "string" && secret.trim().length >= MIN_SECRET_LENGTH,
);
if (typeof text !== "string") {
return text;
}
if (!Array.isArray(secrets)) {
return text;
}
const variants = new Set<string>();
for (const secret of new Set(secrets)) {
if (typeof secret !== "string") {
continue;
}
if (secret.trim().length < MIN_SECRET_LENGTH) {
continue;
}
variants.add(secret);
const escaped = JSON.stringify(secret).slice(1, -1);
if (escaped !== secret) {
variants.add(escaped);
}
}
const values = [...variants].sort((a, b) => b.length - a.length);
let out = text;
for (const value of values) {
out = out.split(value).join("[redacted]");
}
return out;
} catch {
return text;
}
values.sort((a, b) => b.length - a.length);
let out = text;
for (const value of values) {
out = out.split(value).join("[redacted]");
}
return out;
}
/**
* Collect live secret values: every non-empty env value whose NAME looks
* secret-bearing, plus the values of explicitly configured key names
* Collect live secret values: the values of explicitly configured key names
* (`extra`, e.g. `collectLiveKeys(config)` — a key var may have an innocuous
* name like `MY_API`). Values travel only into scrubbing, never into logs.
* name like `MY_API`). Host environment variables are deliberately NOT
* scanned: the sandbox env is an allowlist, so host vars never reach tier 1
* or scripted runs, and scanning them scrubbed innocuous values (e.g.
* `TOKENIZERS_PARALLELISM=false`) out of persisted evidence. Values travel
* only into scrubbing, never into logs.
*/
export function collectSecretValues(extra?: Record<string, string>): string[] {
const values: string[] = [];
for (const [name, value] of Object.entries(process.env)) {
if (
typeof value === "string" &&
value.length > 0 &&
SECRET_NAME_RE.test(name)
) {
values.push(value);
}
}
for (const value of Object.values(extra ?? {})) {
if (typeof value === "string" && value.length > 0) {
values.push(value);
+32 -20
View File
@@ -1,8 +1,10 @@
// OCI-6.3-working-dir: `working-directory` is runner-evaluated and would
// otherwise honor `../` or absolute paths escaping GITHUB_WORKSPACE. The
// OCI-6.3-working-dir: `working-directory` is runner-evaluated, so the
// action guards it via env passthrough (`IN_WD`, keeping the
// zero-interpolation invariant): `..` and absolute values fail before any
// tier runs. These tests pin the guard script and its wiring.
// zero-interpolation invariant). The guard compares the resolved `$PWD`
// (the step has already changed into working-directory) against the
// resolved workspace root: absolute paths inside the workspace are fine,
// paths outside fail before any tier runs. These tests pin the guard
// script and its wiring.
import { describe, expect, test } from "bun:test";
import { readFileSync } from "node:fs";
import { join } from "node:path";
@@ -21,17 +23,25 @@ function action(): ActionDoc {
) as ActionDoc;
}
/** The guard each `run:` script must carry (kept in sync with action.yml). */
export function workingDirGuardFails(input: string): boolean {
return input.startsWith("/") || input.includes("..");
/**
* Mirrors the guard in action.yml: the resolved working directory must be
* the workspace root or beneath it. Accepts absolute paths inside the
* workspace (including `sub/../sub`, already resolved by the runner);
* rejects anything outside, including `ws/..` (which resolves outside).
*/
export function isOutsideWorkspace(pwd: string, workspace: string): boolean {
const ws = workspace.endsWith("/") ? workspace : `${workspace}/`;
const here = pwd.endsWith("/") ? pwd : `${pwd}/`;
return !here.startsWith(ws);
}
describe("working-directory guard", () => {
test("working-directory is documented as workspace-bound", () => {
test("working-directory is documented as workspace-bound, absolute-inside allowed", () => {
const description =
action().inputs?.["working-directory"]?.description ?? "";
expect(description).toMatch(/GITHUB_WORKSPACE/);
expect(description).toMatch(/\.\./);
expect(description).toMatch(/absolute/);
expect(description).not.toMatch(/unsupported/);
});
test("the input travels via IN_WD env, never interpolated into run:", () => {
@@ -45,22 +55,24 @@ describe("working-directory guard", () => {
expect(runStep?.run ?? "").not.toContain(expr);
});
test("the run: script rejects absolute and dot-dot paths up front", () => {
test("the run: script pins the resolved-directory guard", () => {
const run = (action().runs?.steps ?? []).find(
(step) => typeof step.run === "string" && step.run.includes("IN_WD"),
)?.run as string;
expect(run).toContain('case "$IN_WD"');
expect(run).toContain("/*|*..*");
expect(run).toContain('ws="$(cd "$GITHUB_WORKSPACE" && pwd -P)"');
expect(run).toContain('case "$(pwd -P)/" in "$ws"/*)');
expect(run).toContain('echo "result=fail" >> "$GITHUB_OUTPUT"');
expect(run).toContain("exit 2");
expect(run).not.toContain("/*|*..*");
});
test("guard predicate rejects escapes and accepts normal dirs", () => {
expect(workingDirGuardFails("/tmp/evil")).toBe(true);
expect(workingDirGuardFails("../escape")).toBe(true);
expect(workingDirGuardFails("a/../../b")).toBe(true);
expect(workingDirGuardFails(".")).toBe(false);
expect(workingDirGuardFails("test/fixtures/plugins/tool-basic")).toBe(
false,
);
test("guard helper accepts inside paths and rejects outside ones", () => {
const ws = "/home/runner/work/repo/repo";
expect(isOutsideWorkspace(ws, ws)).toBe(false);
expect(isOutsideWorkspace(`${ws}/plugin`, ws)).toBe(false);
expect(isOutsideWorkspace(`${ws}/sub/../sub`, ws)).toBe(false);
expect(isOutsideWorkspace("/tmp/evil", ws)).toBe(true);
expect(isOutsideWorkspace("/home/runner/work/other/other", ws)).toBe(true);
expect(isOutsideWorkspace("/home/runner/work", ws)).toBe(true);
});
});
-39
View File
@@ -1,39 +0,0 @@
// OCI-6.2-artifact-poisoning: matrix legs uploading the same default
// artifact name collide (upload-artifact merges by name), so allowed-leg
// `latest` results can poison required-leg gate history. Validation fails
// closed when `gate.artifact` is the default AND more than one opencode
// version is configured, naming the file and field. Per-leg distinct names
// (README matrix example) pass.
import { describe, expect, test } from "bun:test";
import { formatIssues } from "../../src/config/load.ts";
import { ConfigSchema } from "../../src/config/schema.ts";
describe("gate artifact poisoning guard", () => {
test("default artifact with two versions fails naming file and field", () => {
const parsed = ConfigSchema.safeParse({
opencode: { versions: ["2.0.26", "latest"] },
});
expect(parsed.success).toBe(false);
if (parsed.success) {
return;
}
const message = formatIssues("config.yaml", parsed.error);
expect(message).toContain("config.yaml");
expect(message).toContain("gate.artifact");
});
test("default artifact with one version passes", () => {
expect(
ConfigSchema.safeParse({ opencode: { versions: ["2.0.26"] } }).success,
).toBe(true);
});
test("distinct artifacts with two versions pass", () => {
expect(
ConfigSchema.safeParse({
opencode: { versions: ["2.0.26", "latest"] },
gate: { artifact: "matrix-leg-results" },
}).success,
).toBe(true);
});
});
+91 -14
View File
@@ -27,9 +27,9 @@ describe("redactSecretValues", () => {
});
test("longer overlapping values scrub completely", () => {
expect(redactSecretValues("xxabcxx", ["abc", "xxabcxx"])).toBe(
"[redacted]",
);
expect(
redactSecretValues("xxsecret12xx", ["secret12", "xxsecret12xx"]),
).toBe("[redacted]");
});
test("values with regex metacharacters match literally", () => {
@@ -39,8 +39,41 @@ describe("redactSecretValues", () => {
test("empty, whitespace-only and short values are skipped", () => {
expect(redactSecretValues("abc 123", ["", " ", "123"])).toBe("abc 123");
// Anything under 8 chars — including common env noise like "false"
// (e.g. TOKENIZERS_PARALLELISM=false) — is never scrubbed.
expect(
redactSecretValues("Defaults to false; code 1234567", [
"false",
"1234567",
]),
).toBe("Defaults to false; code 1234567");
});
test("scrubs each value's JSON-escaped form as well", () => {
const value = `sec"ret\\key-123`;
expect(value.length).toBeGreaterThanOrEqual(8);
const line = JSON.stringify({ content: `print ${value} now` });
expect(line).toContain(value.slice(0, 3));
// The raw value does not appear literally in the JSON line (quotes
// and backslashes are escaped), yet scrubbing must still catch it.
expect(line).not.toContain(value);
const scrubbed = redactSecretValues(line, [value]);
expect(scrubbed).not.toContain(JSON.stringify(value).slice(1, -1));
expect(scrubbed).toContain("[redacted]");
// And the raw form is still scrubbed where it appears literally.
expect(redactSecretValues(`saw ${value} here`, [value])).toBe(
"saw [redacted] here",
);
});
test("never throws and returns text unchanged on malformed input", () => {
expect(
redactSecretValues("keep false intact", ["false", 42 as never]),
).toBe("keep false intact");
expect(
redactSecretValues("untouched", undefined as unknown as string[]),
).toBe("untouched");
});
test("clean text and an empty secret list pass through", () => {
expect(redactSecretValues("nothing here", [CANARY])).toBe("nothing here");
expect(redactSecretValues(`has ${CANARY}`, [])).toBe(`has ${CANARY}`);
@@ -61,21 +94,32 @@ describe("collectSecretValues", () => {
saved.clear();
});
test("picks up secret-named env vars plus explicit key values", () => {
for (const name of ["OCI_SCRUB_TEST_KEY", "OCI_SCRUB_TEST_PLAIN"]) {
if (!saved.has(name)) {
saved.set(
name,
Object.hasOwn(process.env, name) ? process.env[name] : undefined,
);
}
function setEnv(name: string, value: string): void {
if (!saved.has(name)) {
saved.set(
name,
Object.hasOwn(process.env, name) ? process.env[name] : undefined,
);
}
process.env.OCI_SCRUB_TEST_KEY = CANARY;
process.env.OCI_SCRUB_TEST_PLAIN = "not-a-secret-name";
process.env[name] = value;
}
test("picks up explicit key values and ignores host env", () => {
setEnv("OCI_SCRUB_TEST_KEY", CANARY);
setEnv("OCI_SCRUB_TEST_PLAIN", "not-a-secret-name");
setEnv("TOKENIZERS_PARALLELISM", "false");
// Host env is never scanned, even for secret-looking names.
expect(collectSecretValues()).toEqual([]);
const values = collectSecretValues({ MY_API: "explicit-key-value" });
expect(values).toContain(CANARY);
expect(values).toContain("explicit-key-value");
expect(values).not.toContain(CANARY);
expect(values).not.toContain("not-a-secret-name");
expect(values).not.toContain("false");
});
test("an explicitly passed value is picked up even if secret-named", () => {
setEnv("OCI_SCRUB_TEST_KEY", CANARY);
expect(collectSecretValues({ MY_API: CANARY })).toContain(CANARY);
});
});
@@ -117,6 +161,39 @@ describe("evidence scrubbing", () => {
expect(JSON.stringify(stub.requests[0]?.body)).toContain(CANARY);
});
test("host env noise (TOKENIZERS_PARALLELISM=false) leaves evidence JSONL valid", async () => {
const had = Object.hasOwn(process.env, "TOKENIZERS_PARALLELISM")
? process.env.TOKENIZERS_PARALLELISM
: undefined;
process.env.TOKENIZERS_PARALLELISM = "false";
try {
const dir = tempDir("evidence-scrub-r2-");
const recordFile = join(dir, "stub-requests.jsonl");
const stub = await startStub({
script: [{ text: "Defaults to false." }],
recordFile,
redactValues: collectSecretValues(),
});
stubs.push(stub);
await post(stub.url, {
messages: [{ role: "user", content: "Defaults to false." }],
tools: [{ type: "function", function: { name: "read" } }],
});
const persisted = readFileSync(recordFile, "utf8");
expect(persisted).toContain("false");
expect(persisted).not.toContain("[redacted]");
for (const line of persisted.trim().split("\n")) {
JSON.parse(line);
}
} finally {
if (had === undefined) {
delete process.env.TOKENIZERS_PARALLELISM;
} else {
process.env.TOKENIZERS_PARALLELISM = had;
}
}
});
test("report markdown scrubs secret values from failures", () => {
const results: ResultsFile = {
version: 1,