mirror of
https://github.com/openclaw/clawhub.git
synced 2026-08-14 00:47:57 +00:00
chore(autoreview): sync TruffleHog scanning (#3201)
This commit is contained in:
@@ -17,6 +17,8 @@ Use when:
|
||||
- after non-trivial code edits, before final/commit/ship
|
||||
- reviewing a local branch or PR branch after fixes
|
||||
|
||||
Do not require autoreview for a change whose entire diff is prose-only internal notes or `SKILL.md` documentation. Still inspect the diff directly and run the repository's lightweight documentation validation, if any. This exception does not cover user-facing documentation, executable examples, configuration, scripts, generated files, or behavior changes.
|
||||
|
||||
## Contract
|
||||
|
||||
- Treat review output as advisory. Never blindly apply it.
|
||||
@@ -36,10 +38,10 @@ Use when:
|
||||
- Tools are useful in review mode. Codex receives the validated bundle in an empty workspace so ignored files and linked-worktree metadata remain unreadable; web search stays available for dependency contracts and upstream docs.
|
||||
- Security perspective is always included, but it should not cripple legitimate functionality. Report security findings only when the change creates a concrete, actionable risk or removes an important safety check.
|
||||
- Reviewer subprocesses preserve engine authentication and non-credentialed proxy variables needed by headless or restricted-network environments while stripping process-injection, Git override, and credentialed proxy values.
|
||||
- Review bundles fail closed before engine invocation when tracked or untracked paths look sensitive, patch text looks secret-like, or a Git diff exceeds the bundle limit. Redact/split the change; never accept a truncated patch as complete review proof.
|
||||
- Before engine invocation, autoreview runs TruffleHog over temporary snapshots of the exact added or modified content under review. It intentionally matches TruffleHog's low-false-positive pre-commit policy (`verified,unknown`); it does not classify arbitrary password-like strings or rescan unchanged history. Install TruffleHog using its official platform-neutral instructions; autoreview fails with that link when the binary is unavailable and never auto-installs it. Repositories should also run TruffleHog in pull-request CI as a backup outside autoreview; repository-local Git hooks are optional. Review bundles still omit security-sensitive paths or files, and explicit prompt and dataset inputs remain checked before engine invocation. Safe large diffs are sent as one pass while they fit the aggregate prompt limit, then partitioned into complete bounded passes without truncation.
|
||||
- For regression provenance, keep roles separate: blamed code author, blamed PR author, PR merger/committer, current PR author, and PR/date. If no blamed PR is traceable, use the blamed commit as the provenance: commit SHA, date, and author username. Do not guess a merger or frame missing PR metadata as a separate finding.
|
||||
- If the blamed PR was merged by `clawsweeper[bot]` or another automation, identify the human trigger when practical. Check timeline/comments first; if rate-limited, use gitcrawl/cache or public PR HTML. Look for maintainer commands such as `@clawsweeper automerge`, `/landpr`, or labels/status comments that armed automerge. Report `automerge triggered by @login`; if not found, say trigger unknown.
|
||||
- Do not invoke built-in `codex review`, nested reviewers, or reviewer panels from inside the review. The helper builds one bundle, calls one selected engine, validates one structured result, and stops.
|
||||
- Do not invoke built-in `codex review`, nested reviewers, or reviewer panels from inside the review. The helper builds one validated bundle, calls the selected engine once for normal inputs or once per complete bounded chunk for oversized inputs, validates the structured results, and stops.
|
||||
- Stop as soon as the helper exits 0 with no accepted/actionable findings. Do not run an extra review just to get a nicer "clean" line, a second opinion, or clearer closeout wording.
|
||||
- Treat the helper's successful exit plus absence of actionable findings as the clean review result, even if the underlying Codex CLI output is terse.
|
||||
- Multi-reviewer panels are opt-in only. Use them when explicitly requested or when risk justifies the extra spend; the main agent still verifies every accepted finding before fixing.
|
||||
@@ -189,6 +191,29 @@ clean `main` against `origin/main` is usually an empty diff after push. For a
|
||||
small stack, review each commit explicitly or review the branch before merging
|
||||
with `--base`.
|
||||
|
||||
## Oversized Bundles
|
||||
|
||||
The helper scans the full patch before partitioning it. A safe bundle that fits
|
||||
the aggregate prompt limit remains one integrated review pass. Larger bundles
|
||||
are split at bundle sections and file boundaries where possible; an oversized
|
||||
single-file block is split at line boundaries with repeated file/hunk context
|
||||
and an absolute new- or old-file line offset. Untracked snapshots use
|
||||
injection-safe source-line records so continuation passes retain reportable
|
||||
locations. A single physical diff line split across passes also retains its
|
||||
original addition, deletion, or context marker.
|
||||
Every original bundle byte appears exactly once across the pass sequence, and
|
||||
all validated reports are merged before required-finding and exit-status checks.
|
||||
The helper caps one run at eight bounded passes so an unexpectedly huge branch
|
||||
cannot create unbounded model calls; split still-larger work into coherent review
|
||||
targets.
|
||||
|
||||
Chunking makes large-diff review usable, but it cannot give one model call every
|
||||
cross-file implementation detail. For architecture-heavy changes, still prefer
|
||||
a coherent branch or PR shape whose semantic decision surface fits one pass.
|
||||
Removing verified non-authoritative generated noise remains useful, but never
|
||||
drop lockfiles, generated clients, policies, manifests, schemas, or other
|
||||
independently semantic artifacts merely to shrink the review.
|
||||
|
||||
## Parallel Closeout
|
||||
|
||||
Format first if formatting can change line locations. Then it is OK to run tests and review in parallel:
|
||||
@@ -212,6 +237,12 @@ happen before the test shell starts:
|
||||
OPENCLAW_TESTBOX=1 "$AUTOREVIEW" --parallel-tests "pnpm check:changed"
|
||||
```
|
||||
|
||||
On POSIX, the helper puts this isolated Testbox home under the short, sticky
|
||||
system `/tmp`; Blacksmith creates an SSH control socket below that home, and a
|
||||
long macOS `TMPDIR` can exceed the Unix-socket path limit. With an older helper,
|
||||
prefix the outer autoreview process with `TMPDIR=/tmp`. Setting `TMPDIR` inside
|
||||
the quoted test command is too late because the isolated home already exists.
|
||||
|
||||
This is the narrow trusted-maintainer-code exception: it stages only the Blacksmith
|
||||
credential file into the temporary home so the command can delegate remotely. Never
|
||||
use this credential-hydrated path for untrusted contributor or fork code. Run other
|
||||
@@ -386,6 +417,7 @@ The helper:
|
||||
- recognizes `--engine droid`, `copilot`, `cursor`, and `opencode` only to fail closed with isolation errors; runnable engines are `codex`, `claude`, and `pi`; default is `AUTOREVIEW_ENGINE` or `codex`
|
||||
- resolves bare `git`, `gh`, reviewer, and PowerShell shell commands from absolute `PATH` entries only, never from the reviewed checkout; explicit `--*-bin` paths are interpreted from the reviewed repository root when relative and accepted only when both the supplied path and resolved target stay outside the reviewed repository
|
||||
- use `--mode commit --commit <ref>` for already-committed work, especially clean `main` after landing
|
||||
- scans safe Git patches in full, recognizes synthetic fixture values tied to their credential field, reviews them in one pass up to the aggregate prompt limit, and automatically uses complete bounded passes above it
|
||||
- should be left in `--mode auto` or forced to `--mode branch` for PR/branch work; do not force `--mode local` after committing
|
||||
- writes only to stdout unless `--output`, `--json-output`, or live streamed engine stderr is set
|
||||
- supports `--dry-run`, `--parallel-tests`, `--parallel-tests-shell`, `--prompt`, repo-relative `--prompt-file`, repo-relative `--dataset`, `--no-tools`, `--no-web-search`, repeatable Codex-only safe model/response tuning with `--codex-config key=value`, Codex-only `--codex-speed fast|flex|default`, and commit refs
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -102,6 +102,80 @@ class AutoreviewCursorTests(unittest.TestCase):
|
||||
self.assertIn("review engine result was not structured JSON", str(exc_info.exception))
|
||||
|
||||
|
||||
class AutoreviewSecretScannerTests(unittest.TestCase):
|
||||
def test_boolean_declarations_are_not_credential_material(self) -> None:
|
||||
secret_field = "is" + "Secret"
|
||||
client_secret_field = "hasClient" + "Secret"
|
||||
cases = (
|
||||
(f"val {secret_field}: Boolean? = null,", None),
|
||||
(f"var {client_secret_field}: Boolean = false", None),
|
||||
(f"abstract val {secret_field}: Boolean?", None),
|
||||
(f"val {secret_field}: Boolean?", None),
|
||||
(f"const {client_secret_field}: boolean = true;", "typescript"),
|
||||
(f"declare const {client_secret_field}: boolean;", "typescript"),
|
||||
(f"let {secret_field}: Bool? = nil", None),
|
||||
(f"let {secret_field}: Bool?", None),
|
||||
)
|
||||
|
||||
for content, javascript_dialect in cases:
|
||||
with self.subTest(content=content):
|
||||
self.assertFalse(
|
||||
AUTOREVIEW.secret_text_risk(
|
||||
content,
|
||||
javascript_dialect=javascript_dialect,
|
||||
)
|
||||
)
|
||||
|
||||
def test_boolean_and_null_literal_values_are_not_credentials(self) -> None:
|
||||
cases = (
|
||||
("is" + "Secret", "true"),
|
||||
("requires" + "Password", "false"),
|
||||
("access" + "Token", "null"),
|
||||
)
|
||||
for field_name, literal in cases:
|
||||
content = f"{field_name} = {literal}"
|
||||
with self.subTest(content=content):
|
||||
self.assertFalse(AUTOREVIEW.secret_text_risk(content))
|
||||
|
||||
def test_boolean_annotation_does_not_hide_real_credential_literal(self) -> None:
|
||||
literal_value = "actual-production-" + "secret"
|
||||
secret_field = "is" + "Secret"
|
||||
client_secret_field = "hasClient" + "Secret"
|
||||
cases = (
|
||||
(f'val {secret_field}: Boolean? = "{literal_value}",', None),
|
||||
(f'var {client_secret_field}: Boolean = "{literal_value}"', None),
|
||||
(
|
||||
f'const {client_secret_field}: boolean = "{literal_value}";',
|
||||
"typescript",
|
||||
),
|
||||
(f'let {secret_field}: Bool? = "{literal_value}"', None),
|
||||
)
|
||||
|
||||
for content, javascript_dialect in cases:
|
||||
with self.subTest(content=content):
|
||||
self.assertTrue(
|
||||
AUTOREVIEW.secret_text_risk(
|
||||
content,
|
||||
javascript_dialect=javascript_dialect,
|
||||
)
|
||||
)
|
||||
|
||||
def test_boolean_prefix_values_remain_credentials(self) -> None:
|
||||
field_name = "client" + "Secret"
|
||||
for prefix in ("Boolean", "boolean", "Bool"):
|
||||
literal_value = prefix + "-prod-credential"
|
||||
content = f"{field_name}: {literal_value}"
|
||||
with self.subTest(content=content):
|
||||
self.assertTrue(AUTOREVIEW.secret_text_risk(content))
|
||||
|
||||
def test_boolean_type_tokens_in_config_remain_credentials(self) -> None:
|
||||
field_name = "client" + "Secret"
|
||||
for literal_value in ("Boolean?", "Boolean?=abc1234"):
|
||||
content = f"{field_name}: {literal_value}"
|
||||
with self.subTest(content=content):
|
||||
self.assertTrue(AUTOREVIEW.secret_text_risk(content))
|
||||
|
||||
|
||||
class AutoreviewCompatibilityTests(unittest.TestCase):
|
||||
@classmethod
|
||||
def setUpClass(cls) -> None:
|
||||
@@ -553,8 +627,13 @@ class AutoreviewCompatibilityTests(unittest.TestCase):
|
||||
source.write_text("after\n")
|
||||
|
||||
cursor_bin = root / "cursor-agent"
|
||||
trufflehog_bin = root / "trufflehog"
|
||||
record_path = root / "record.json"
|
||||
AUTOREVIEW.write_executable(cursor_bin, AUTOREVIEW.fake_cursor_script())
|
||||
AUTOREVIEW.write_executable(
|
||||
trufflehog_bin,
|
||||
"#!/usr/bin/env python3\nraise SystemExit(0)\n",
|
||||
)
|
||||
env = os.environ.copy()
|
||||
env.update(
|
||||
{
|
||||
@@ -563,7 +642,10 @@ class AutoreviewCompatibilityTests(unittest.TestCase):
|
||||
"GIT_CONFIG_GLOBAL": str(root / "hostile-gitconfig"),
|
||||
"NODE_OPTIONS": "--require=hostile.js",
|
||||
"PYTHONPATH": str(root / "hostile-python"),
|
||||
"PATH": f"{repo}{os.pathsep}{env.get('PATH', '')}",
|
||||
"PATH": (
|
||||
f"{root}{os.pathsep}{repo}{os.pathsep}"
|
||||
f"{env.get('PATH', '')}"
|
||||
),
|
||||
"HOME": str(root),
|
||||
"USERPROFILE": str(root),
|
||||
}
|
||||
|
||||
+30
@@ -0,0 +1,30 @@
|
||||
declare const accountId: string;
|
||||
declare const filePath: string;
|
||||
declare const secretRef: string;
|
||||
declare const tryReadSecretFileSync: (...args: unknown[]) => string;
|
||||
declare const normalizeResolvedSecretInputString: (options: unknown) => string;
|
||||
|
||||
export const passwordFile = tryReadSecretFileSync(filePath, "IRC password file", {
|
||||
credentialDiagnostic: {
|
||||
configPath: `channels.irc.accounts.${accountId}.passwordFile`,
|
||||
},
|
||||
});
|
||||
export const nickservFile = tryReadSecretFileSync(filePath, "IRC NickServ password file", {
|
||||
credentialDiagnostic: {
|
||||
configPath: `channels.irc.accounts.${accountId}.nickserv.passwordFile`,
|
||||
},
|
||||
});
|
||||
export const botSecret = normalizeResolvedSecretInputString({
|
||||
value: secretRef,
|
||||
path: `channels.nextcloud-talk.accounts.${accountId}.botSecret`,
|
||||
});
|
||||
export const botSecretFile = tryReadSecretFileSync(filePath, "Nextcloud bot secret file", {
|
||||
credentialDiagnostic: {
|
||||
configPath: `channels.nextcloud-talk.accounts.${accountId}.botSecretFile`,
|
||||
},
|
||||
});
|
||||
export const tokenFile = tryReadSecretFileSync(
|
||||
filePath,
|
||||
`channels.telegram.accounts.${accountId}.tokenFile`,
|
||||
{ rejectSymlink: true },
|
||||
);
|
||||
@@ -0,0 +1,55 @@
|
||||
type SecretRef = { source: "env"; id: string };
|
||||
type CredentialUnavailableDiagnostic = { path: string; reason: string };
|
||||
|
||||
declare const tokenRef: SecretRef;
|
||||
declare const keyRef: SecretRef;
|
||||
declare const inlinePassword: string;
|
||||
declare const inlineSecret: string;
|
||||
declare const accountFileToken: string;
|
||||
declare const baseFileToken: string;
|
||||
declare const passwordResolution: { password: string };
|
||||
declare const secretResolution: { secret: string };
|
||||
declare const tokenResolution: { token: string };
|
||||
declare const accountTokenFile: { token: string };
|
||||
declare const channelTokenFile: { token: string };
|
||||
declare const merged: { apiPassword: string; passwordFile: string };
|
||||
declare const tryReadSecretFileSync: (...args: unknown[]) => string;
|
||||
declare const normalizeResolvedSecretInputString: (options: unknown) => string;
|
||||
declare const resolveToken: (options: unknown) => { value: string };
|
||||
|
||||
const filePassword = tryReadSecretFileSync(merged.passwordFile, "IRC password file", {
|
||||
credentialDiagnostic: {
|
||||
configPath: `channels.irc.accounts.${accountId}.passwordFile`,
|
||||
report: (diagnostic: CredentialUnavailableDiagnostic) => diagnostic,
|
||||
},
|
||||
});
|
||||
const configPassword = normalizeResolvedSecretInputString({
|
||||
value: merged.apiPassword,
|
||||
path: "channels.nextcloud-talk.apiPassword",
|
||||
});
|
||||
const token = resolveToken({ accountId });
|
||||
const priorPasswordFileError = /IRC password file.*must not be a symlink/;
|
||||
|
||||
export type CredentialPlumbing = {
|
||||
tokenRef?: SecretRef;
|
||||
keyRef?: SecretRef;
|
||||
credentialDiagnostics?: CredentialUnavailableDiagnostic[];
|
||||
};
|
||||
|
||||
export const resolvedCredentialPlumbing = {
|
||||
token: tokenRef,
|
||||
apiKey: keyRef,
|
||||
password: filePassword,
|
||||
configPassword,
|
||||
nextPassword: inlinePassword,
|
||||
secret: inlineSecret,
|
||||
accountToken: accountFileToken,
|
||||
baseToken: baseFileToken,
|
||||
resolvedPassword: passwordResolution.password,
|
||||
resolvedSecret: secretResolution.secret,
|
||||
resolvedToken: tokenResolution.token,
|
||||
accountTokenFile: accountTokenFile.token,
|
||||
channelTokenFile: channelTokenFile.token,
|
||||
apiPassword: merged.apiPassword,
|
||||
channelAccessToken: token.value,
|
||||
};
|
||||
@@ -0,0 +1,10 @@
|
||||
const password = "FAKE-CorrectHorseBattery-Staple-2026!";
|
||||
const credential = "FAKE_A7f9K2m4Q8v6N3x5R1p0T9z8";
|
||||
const apiKey = "sk-proj-FAKE00000000000000000000000000000000000000000000";
|
||||
const githubToken = "ghp_FAKE000000000000000000000000000000";
|
||||
const awsAccessKey = "AKIAFAKE000000000000";
|
||||
const slackToken = "xoxb-FAKE000000000-FAKE000000000-FAKE000000000000000000000000";
|
||||
const authorization = "Bearer eyJhbGciOiJIUzI1NiJ9.RkFLRS1OT1QtQS1SRUFM.TOKENFAKESIGNATURE";
|
||||
const resolvedToken = resolveToken({ value: "FAKE_B8g0L3n5R9w7P4y6S2q1U0a9" });
|
||||
const filePassword = tryReadSecretFileSync(path, "FAKE-A7f9K2m4Q8v6N3x5R1p0T9z8");
|
||||
const password = readPassword("alice", "FAKE correct horse secret battery 2026");
|
||||
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user