From eac028cca69771b025962c8ac36499736581f3e1 Mon Sep 17 00:00:00 2001 From: Willem Jiang Date: Wed, 2 Sep 2026 16:54:23 +0800 Subject: [PATCH] ci: preauthorize skill review waiver hashes (#5143) --- .github/skill-review-waivers.v1.json | 6 ++ AGENTS.md | 10 ++- README.md | 2 + backend/tests/test_skill_review_waivers.py | 82 ++++++++++++++++++- .../waiver_manifest.v1.schema.json | 6 ++ scripts/AGENTS.md | 10 ++- scripts/skill_review_waivers.py | 37 +++++++-- 7 files changed, 138 insertions(+), 15 deletions(-) diff --git a/.github/skill-review-waivers.v1.json b/.github/skill-review-waivers.v1.json index 8e1b4da41..ba47f6cbf 100644 --- a/.github/skill-review-waivers.v1.json +++ b/.github/skill-review-waivers.v1.json @@ -9,6 +9,9 @@ "line": 35, "evidence": "subprocess.run", "file_sha256": "sha256:87d864570220b699fac52da309d2d6efdb060647bfebc74f768128e646accf80", + "preapproved_file_sha256s": [ + "sha256:2877bde08bf3f437b9dae3d57585a0840b9b1024736d2e5c6c657b71899269d0" + ], "reason": "Required Claude CLI invocation uses a fixed executable, an argv list, prompt input over stdin, and shell=False.", "expires_on": "2027-02-28" }, @@ -20,6 +23,9 @@ "line": 85, "evidence": "subprocess.Popen", "file_sha256": "sha256:43e3b8f80dbf69c343967ba77e268fae991d9fa3ed68b32a0ff02532cd48657f", + "preapproved_file_sha256s": [ + "sha256:ea2521ba41c8fd16b2900758c890bb6c6d2b4b01da10b4a860facb8587ed0bde" + ], "reason": "Required Claude CLI invocation uses a fixed executable, an argv list, captured output streams, and shell=False.", "expires_on": "2027-02-28" } diff --git a/AGENTS.md b/AGENTS.md index 448805af3..423981bba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -105,9 +105,13 @@ Skill quality review note: edits from their head revision, but only the manifest from the trusted base revision can suppress that run. Entries match one error finding exactly, include the reviewed file's SHA-256 and an expiry date, remain visible in CI - output, and can never waive blocker findings. Adding a waiver and relying on - it therefore requires two steps: merge the reviewed waiver first, then update - the affected public skill in a later pull request. + output, and can never waive blocker findings. An entry may also preapprove a + bounded list of future full-file SHA-256 values. Those hashes become effective + only after the manifest change lands in the trusted base. Adding or changing a + waiver and relying on it therefore requires two steps: merge the reviewed + manifest change first, then update the affected public skill in a later pull + request. After that skill change lands, promote its hash to `file_sha256` and + remove the consumed preapproval in a follow-up manifest cleanup. Scheduled-task note: - The scheduled-task MVP adds a workspace page at `/workspace/scheduled-tasks` plus a background scheduler service gated by `config.yaml -> scheduler.enabled`. diff --git a/README.md b/README.md index 51487fadc..c6dc57ca4 100644 --- a/README.md +++ b/README.md @@ -976,6 +976,8 @@ cd backend uv run python -m deerflow.skills.review.cli ../skills/public/data-analysis --format text --fail-on error --fail-on-incomplete ``` +Public-skill CI waivers are exact, expiring exceptions in `.github/skill-review-waivers.v1.json`. Because only the trusted base manifest can suppress a finding, a file-changing pull request can be preauthorized safely by first merging a manifest-only change that lists the reviewed future full-file SHA-256 in `preapproved_file_sha256s`; the file change can then land in a later pull request. + Tools follow the same philosophy. DeerFlow comes with a core toolset — web search, web fetch, rendered web capture, file operations, bash execution — and supports custom tools via MCP servers and Python functions. The bundled DDG, Brave, Tavily, and SearXNG search providers accept an optional `time_range` of `day`, `week`, `month`, or `year`; omitting it preserves existing search behavior. For DDG recency searches, DeerFlow excludes DDGS backends that ignore time limits. Swap anything. Add anything. Advanced deployments can enable pluggable authorization with `authorization.enabled` in `config.yaml`. A configured `AuthorizationProvider` filters denied tools before they reach the model or deferred-tool catalog, then the same provider is checked again before every business-tool execution through the existing guardrail middleware. Gateway `threads:*` and `runs:*` route permissions are derived from the same provider, while existing owner checks and admin-only management gates remain in force. Every HTTP route that starts or enables a future Agent run requires `runs:create`: this includes the stateless `POST /api/runs/stream` and `POST /api/runs/wait` endpoints plus scheduled-task create, update, resume, and manual-trigger mutations. Scheduled-task mutations retain their existing `threads:write` requirement, and the stateless routes separately enforce ownership when the optional thread ID is supplied in the request body. A generated `tool_search` may bypass the second tool check only when it fronts the current build's already-filtered deferred catalog. Model access follows the same provider: the Gateway `models` list is filtered per principal, `model:use` is enforced on model detail requests and again when the runtime resolves the agent's model, and a denied default model falls back to the first remaining candidate that also passes `model:use`. The built-in RBAC provider supports per-role `tools`, `routes`, `models`, `skills`, and `sandbox` allow/deny policies and validates that `default_role` names a configured role; authorization is disabled by default. See `config.example.yaml` and the [authorization RFC](docs/plans/2026-07-10-pluggable-authorization-rfc.md). diff --git a/backend/tests/test_skill_review_waivers.py b/backend/tests/test_skill_review_waivers.py index abb7fc2bc..97870bdea 100644 --- a/backend/tests/test_skill_review_waivers.py +++ b/backend/tests/test_skill_review_waivers.py @@ -34,7 +34,12 @@ def _write_target(repo_root: Path, content: bytes = b"safe subprocess invocation return target, f"sha256:{hashlib.sha256(content).hexdigest()}" -def _waiver(*, digest: str, expires_on: date = date(2027, 2, 28)) -> SkillReviewWaiver: +def _waiver( + *, + digest: str, + expires_on: date = date(2027, 2, 28), + preapproved_file_sha256s: tuple[str, ...] = (), +) -> SkillReviewWaiver: return SkillReviewWaiver( package="skills/public/demo", source="skillscan", @@ -45,6 +50,7 @@ def _waiver(*, digest: str, expires_on: date = date(2027, 2, 28)) -> SkillReview file_sha256=digest, reason="Fixed executable and argv invocation with shell disabled.", expires_on=expires_on, + preapproved_file_sha256s=preapproved_file_sha256s, ) @@ -60,7 +66,13 @@ def _finding(*, severity: str = "error", line: int = 12) -> dict[str, object]: } -def _payload(*, digest: str, path: str = "scripts/run.py", duplicate: bool = False) -> bytes: +def _payload( + *, + digest: str, + path: str = "scripts/run.py", + duplicate: bool = False, + preapproved_file_sha256s: object | None = None, +) -> bytes: entry = { "package": "skills/public/demo", "source": "skillscan", @@ -72,6 +84,8 @@ def _payload(*, digest: str, path: str = "scripts/run.py", duplicate: bool = Fal "reason": "Fixed executable and argv invocation with shell disabled.", "expires_on": "2027-02-28", } + if preapproved_file_sha256s is not None: + entry["preapproved_file_sha256s"] = preapproved_file_sha256s return json.dumps({"schema_version": SCHEMA_VERSION, "waivers": [entry, entry] if duplicate else [entry]}).encode() @@ -84,6 +98,8 @@ def test_committed_manifest_matches_schema_and_strict_parser() -> None: parsed = parse_manifest(manifest_path.read_bytes(), source=str(manifest_path)) assert len(parsed.waivers) == 2 + assert parsed.waivers[0].preapproved_file_sha256s == ("sha256:2877bde08bf3f437b9dae3d57585a0840b9b1024736d2e5c6c657b71899269d0",) + assert parsed.waivers[1].preapproved_file_sha256s == ("sha256:ea2521ba41c8fd16b2900758c890bb6c6d2b4b01da10b4a860facb8587ed0bde",) def test_skill_creator_waivers_match_current_error_findings() -> None: @@ -115,6 +131,27 @@ def test_parser_rejects_duplicate_exact_waivers(tmp_path: Path) -> None: parse_manifest(_payload(digest=digest, duplicate=True), source="test manifest") +@pytest.mark.parametrize( + ("preapproved", "error"), + [ + ("sha256:" + "1" * 64, "must be an array"), + (["sha256:" + "1" * 63], "64 lowercase hex characters"), + (["sha256:" + "1" * 64] * 2, "entries must be unique"), + (["PRIMARY"], "must not repeat file_sha256"), + ([f"sha256:{index:064x}" for index in range(9)], "at most 8 entries"), + ], +) +def test_parser_rejects_invalid_preapproved_hashes(tmp_path: Path, preapproved: object, error: str) -> None: + _, digest = _write_target(tmp_path) + value = [digest] if preapproved == ["PRIMARY"] else preapproved + + with pytest.raises(WaiverManifestError, match=error): + parse_manifest( + _payload(digest=digest, preapproved_file_sha256s=value), + source="test manifest", + ) + + def test_missing_manifest_at_ref_means_no_waivers(tmp_path: Path, monkeypatch) -> None: monkeypatch.setattr( waiver_support.subprocess, @@ -138,6 +175,47 @@ def test_matching_waiver_requires_exact_finding_and_current_file_hash(tmp_path: assert matching_waiver(_finding(), package="skills/public/demo", manifest=manifest, repo_root=tmp_path, today=date(2026, 8, 31)) is None +def test_preapproved_file_hash_authorizes_a_later_file_revision(tmp_path: Path) -> None: + target, current_digest = _write_target(tmp_path) + future_content = b"safe subprocess invocation with explicit UTF-8\n" + future_digest = f"sha256:{hashlib.sha256(future_content).hexdigest()}" + waiver = _waiver(digest=current_digest, preapproved_file_sha256s=(future_digest,)) + manifest = WaiverManifest((waiver,)) + facts = {"findings": [_finding()]} + + assert ( + validate_manifest_against_facts( + manifest, + facts_by_package={waiver.package: facts}, + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + == [] + ) + + target.write_bytes(future_content) + + assert ( + matching_waiver( + _finding(), + package=waiver.package, + manifest=manifest, + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + is waiver + ) + assert ( + validate_manifest_against_facts( + manifest, + facts_by_package={waiver.package: facts}, + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + == [] + ) + + def test_matching_waiver_rejects_symlinked_package_outside_repository(tmp_path: Path) -> None: external_root = tmp_path.parent / f"{tmp_path.name}-external" external_target, digest = _write_target(external_root) diff --git a/contracts/skill_review/waiver_manifest.v1.schema.json b/contracts/skill_review/waiver_manifest.v1.schema.json index bd633bf54..d0b3d575d 100644 --- a/contracts/skill_review/waiver_manifest.v1.schema.json +++ b/contracts/skill_review/waiver_manifest.v1.schema.json @@ -20,6 +20,12 @@ "line": { "type": "integer", "minimum": 1 }, "evidence": { "type": "string", "minLength": 1 }, "file_sha256": { "type": "string", "pattern": "^sha256:[0-9a-f]{64}$" }, + "preapproved_file_sha256s": { + "type": "array", + "maxItems": 8, + "uniqueItems": true, + "items": { "type": "string", "pattern": "^sha256:[0-9a-f]{64}$" } + }, "reason": { "type": "string", "minLength": 20 }, "expires_on": { "type": "string", "format": "date" }, "approved_in": { "type": "string", "minLength": 1 } diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 3ef4969e9..386dacc66 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -22,15 +22,19 @@ CI-only exceptions from `.github/skill-review-waivers.v1.json`. The manifest is versioned by `contracts/skill_review/waiver_manifest.v1.schema.json`; each entry must identify one current error by package, source, rule, path, line, and evidence, and pin the complete source file with SHA-256 plus an expiry date. -Blockers are never waivable, and waived errors are still printed with their -original severity and justification. +An optional, bounded `preapproved_file_sha256s` list authorizes reviewed future +full-file digests without relaxing the exact finding match. Blockers are never +waivable, and waived errors are still printed with their original severity and +justification. For pull requests, only the base revision's manifest is effective. The head manifest is parsed and checked against the current analyzer output, but cannot self-authorize a finding in the same pull request. Push comparisons use the same before/after trust boundary. A waiver-only change can therefore land without weakening its own check, then become effective for later changes after -it is part of the trusted base. +it is part of the trusted base. Preapproved digests must be code-reviewed in +that first change; after the corresponding file revision lands, promote the +consumed digest to `file_sha256` and remove it from the preapproval list. ## Backend Static Analysis Commands diff --git a/scripts/skill_review_waivers.py b/scripts/skill_review_waivers.py index 4fc8d2916..bf644403d 100644 --- a/scripts/skill_review_waivers.py +++ b/scripts/skill_review_waivers.py @@ -26,7 +26,8 @@ _REQUIRED_ENTRY_FIELDS = { "reason", "expires_on", } -_OPTIONAL_ENTRY_FIELDS = {"approved_in"} +_OPTIONAL_ENTRY_FIELDS = {"approved_in", "preapproved_file_sha256s"} +_MAX_PREAPPROVED_FILE_SHA256S = 8 class WaiverManifestError(ValueError): @@ -45,11 +46,16 @@ class SkillReviewWaiver: reason: str expires_on: date approved_in: str | None = None + preapproved_file_sha256s: tuple[str, ...] = () @property def finding_key(self) -> tuple[str, str, str, int, str]: return (self.source, self.rule_id, self.path, self.line, self.evidence) + @property + def approved_file_sha256s(self) -> tuple[str, ...]: + return (self.file_sha256, *self.preapproved_file_sha256s) + @dataclass(frozen=True) class WaiverManifest: @@ -100,9 +106,17 @@ def parse_manifest(payload: bytes | str, *, source: str) -> WaiverManifest: source_name = _nonempty_string(raw["source"], field=f"{entry_source}.source") rule_id = _nonempty_string(raw["rule_id"], field=f"{entry_source}.rule_id") evidence = _nonempty_string(raw["evidence"], field=f"{entry_source}.evidence") - file_sha256 = _nonempty_string(raw["file_sha256"], field=f"{entry_source}.file_sha256") - if not _SHA256_RE.fullmatch(file_sha256): - raise WaiverManifestError(f"{entry_source}.file_sha256: must be sha256 followed by 64 lowercase hex characters") + file_sha256 = _parse_sha256(raw["file_sha256"], field=f"{entry_source}.file_sha256") + raw_preapproved_hashes = raw.get("preapproved_file_sha256s", []) + if not isinstance(raw_preapproved_hashes, list): + raise WaiverManifestError(f"{entry_source}.preapproved_file_sha256s: must be an array") + if len(raw_preapproved_hashes) > _MAX_PREAPPROVED_FILE_SHA256S: + raise WaiverManifestError(f"{entry_source}.preapproved_file_sha256s: must contain at most {_MAX_PREAPPROVED_FILE_SHA256S} entries") + preapproved_file_sha256s = tuple(_parse_sha256(value, field=f"{entry_source}.preapproved_file_sha256s[{hash_index}]") for hash_index, value in enumerate(raw_preapproved_hashes)) + if len(set(preapproved_file_sha256s)) != len(preapproved_file_sha256s): + raise WaiverManifestError(f"{entry_source}.preapproved_file_sha256s: entries must be unique") + if file_sha256 in preapproved_file_sha256s: + raise WaiverManifestError(f"{entry_source}.preapproved_file_sha256s: must not repeat file_sha256") reason = _nonempty_string(raw["reason"], field=f"{entry_source}.reason") if len(reason) < 20: raise WaiverManifestError(f"{entry_source}.reason: must contain at least 20 characters") @@ -125,6 +139,7 @@ def parse_manifest(payload: bytes | str, *, source: str) -> WaiverManifest: reason=reason, expires_on=expires_on, approved_in=approved_in, + preapproved_file_sha256s=preapproved_file_sha256s, ) identity = (*waiver.finding_key, waiver.package) if identity in identities: @@ -204,7 +219,7 @@ def matching_waiver( continue if waiver.expires_on < current_date: continue - if file_sha256(repo_root, waiver) != waiver.file_sha256: + if file_sha256(repo_root, waiver) not in waiver.approved_file_sha256s: continue return waiver return None @@ -240,8 +255,9 @@ def validate_manifest_against_facts( errors.append(f"{description}: waivers may target error findings only; blockers can never be waived") continue actual_hash = file_sha256(repo_root, waiver) - if actual_hash != waiver.file_sha256: - errors.append(f"{description}: file digest changed (expected {waiver.file_sha256}, found {actual_hash or 'unavailable'})") + if actual_hash not in waiver.approved_file_sha256s: + expected_hashes = ", ".join(waiver.approved_file_sha256s) + errors.append(f"{description}: file digest changed (expected one of {expected_hashes}, found {actual_hash or 'unavailable'})") return errors @@ -261,6 +277,13 @@ def _nonempty_string(value: object, *, field: str) -> str: return value +def _parse_sha256(value: object, *, field: str) -> str: + digest = _nonempty_string(value, field=field) + if not _SHA256_RE.fullmatch(digest): + raise WaiverManifestError(f"{field}: must be sha256 followed by 64 lowercase hex characters") + return digest + + def _parse_date(value: object, *, field: str) -> date: text = _nonempty_string(value, field=field) try: