feat: Policy RFC Engine (proposal-only)#39
Conversation
|
Thanks for the substantial proposal. I am not treating this as landable in the current queue. It adds a new product surface and about a thousand lines of new policy-generation code, while the latest visible checks only include the notify workflow and the PR body says full local testing was not run under this repo's required Node >=24 environment. Before this could move forward, it needs an explicit maintainer/product decision that ClawSweeper should own a Policy RFC Engine at all, plus a current rebase and full |
ds4psb-ai
left a comment
There was a problem hiding this comment.
Independent code review — Policy RFC Engine
Posted on behalf of @ds4psb-ai (read-only contributor). Maintainer judgment owns merge. The product-surface question @steipete already raised is upstream of everything below; these are quality findings if you do choose to land it.
Safety audit summary
- zero GitHub mutation paths —
gh pr diff 39 | grep -E '(ghJson|gh api|ghMutate|gh pr|gh issue|gh label|github\.com|fetch\(|http:|https://api|axios|node-fetch)'returns 0 matches across all 5 source files. The engine is genuinely IO-local. - output scoped to
results/policy-rfc/— onlymkdirSync(outputDir, ...)plus twowriteFileSynccalls insrc/policy-rfc/index.ts:466-473, both underjoin(options.outputRoot, profile.slug). - [~] imports — PR description implies
../repository-profiles.jsis the only cross-module import. Actually three are used:repository-profiles.js,clawsweeper-args.js,stable-json.js. All three are pure local helpers with no GitHub side effects, so the safety claim still holds, but the PR body is technically incomplete. Worth a one-line fix to the description. - no scheduler/apply/automerge integration — no edits to
clawsweeper.ts,repair/,sweep.yml, or any apply path. New code is reachable only via the newpnpm run policy-rfcscript. -
package.jsonchange is minimal — single+ \"policy-rfc\": \"node dist/policy-rfc/index.js\",line; no other script touched.
Net: the proposal-only safety boundary is upheld in code. Good.
Determinism check
The PR claims: "RFC and JSON outputs are deterministic for the same input records to ensure stable diffs and testability." This is only partially true, and the test suite hides the gap.
Two non-deterministic sources reach the CLI's actual output bytes:
src/policy-rfc/synthesizer.ts:14—const createdAt = options.createdAt ?? new Date().toISOString();src/policy-rfc/scorer.ts:44—now: options.now ?? new Date()
The CLI in src/policy-rfc/index.ts:486-491 only forwards createdAt when --created-at is passed (and never forwards a now to the scorer at all). Default pnpm run policy-rfc invocations therefore produce:
- A fresh
created_atin every JSON proposal, and a different "Generated by ClawSweeper Policy RFC Engine at ..." footer in every markdown. - A
confidenceScorethat drifts asrecencyScoredecays — same input records, same records, differentconfidence_scorefield in JSON, and differentConfidence score:line in markdown.
The tests pin both now and createdAt, so they pass. There is no test that asserts "two consecutive CLI runs produce identical RFC bytes," which is exactly what the PR claims to guarantee.
Suggested fix: have runPolicyRfc resolve a single createdAt once and pass it down as now to the scorer (or freeze now to the latest observedAt in the input — that would also remove time-based drift for replayability). Then add a test that runs the engine twice against a fixture and asserts byte-equal .md and .json.
Top findings
P1 / should fix before merge
-
src/policy-rfc/collector.ts:206-208—itemFromPathmishandlesclosed/records. The regex\/items\/([^/.]+)\./only matches theitems/subtree. AGENTS.md explicitly documentsrecords/<repo-slug>/closed/<number>.mdas the archive layout and the engine is designed to mine durable evidence — i.e. exactly the closed records. For closed records,itemfalls back torelativePath, which (a) inflatesdistinctItems(every closed record looks like its own item) and silently helps a pattern clear theminDistinctItemsgate for the wrong reason, and (b) makes evidence lines harder to read. Suggested: extend to\/(?:items|closed)\/([^/.]+)\./or strip.md/.jsonfrom any leaf segment. -
Determinism claim vs. CLI defaults (see above). I'd treat this as P1 because the PR body explicitly markets determinism as a property.
P2 / nice to have
-
src/policy-rfc/scorer.ts:130-145—proposedActionswitch has nodefaultarm. Today the switch is exhaustive, so TS infersstring. The day someone adds a seventhPolicyPatternType, this silently returnsundefinedand propagates intoproposedAction: stringin the JSON. Adefault: throw new Error(...)or asatisfies neverexhaustiveness check would surface the regression at compile/run time. -
src/policy-rfc/collector.ts:210-223—safeRead/safeReadDir/safeIsDirectoryswallow every error class. TreatingEACCESandEMFILEthe same asENOENTmeans a permissions-broken records dir produces an empty proposal set with zero diagnostic. At minimum, logerror.codetoconsole.warnfor non-ENOENTfailures so an operator knows the scan was incomplete. -
src/policy-rfc/collector.ts:277-285—jsonStringValuesregex."${key}"\s*:\s*"([^"]+)"truncates at the first internal\"and silently misses values that span lines. For current ClawSweeper records it's probably fine, but if anyone ever writes JSON evidence with escaped quotes, the extracted value will be wrong, not absent. Consider parsing withJSON.parsewhen the file ends in.json, regex only as a markdown fallback. -
src/policy-rfc/collector.ts:302-304—hasSuccessfulOutcomeis a permissive substring match. It will mark a record containing the phrase "validation pass failed" assuccessfulOutcome: true. Thesuccessterm in the confidence formula then over-rewards noisy records. Consider anchoring (e.g. require the marker to live in a structuredresult:/outcome:line, or in JSON"status": "..."). -
src/policy-rfc/collector.ts:329-336—frontMatterStringArrayJSON failure path returns[]instead of falling back to comma-split. A bracketed but malformed labels line drops all labels for that record rather than degrading. Atry { ... } catch { /* fall through */ }instead ofreturn []would be safer.
Recommendation
Proposal-only safety boundary is upheld and the architecture is clean. The maintainer call about whether ClawSweeper should own this surface is the dominant gate (per @steipete's earlier comment); from a pure code-quality lens this PR is in good shape.
If you do choose to merge: I'd ask for the determinism fix and the closed/ regex fix as P1 blockers, since both are claims-vs-code mismatches that the test suite happens not to catch. The P2 items are quality-of-life and can land later.
Not approving (read-only contributor); flagging as a comment for maintainer judgment.
The Policy RFC engine is proposal-only, but its generated artifacts must be reproducible enough for maintainers to review and diff safely. Derive default timestamps from observed evidence instead of wall-clock time, and preserve archived closed-record item numbers alongside active items. Constraint: PR openclaw#39 was reviewed with two P1 launch blockers around nondeterminism and closed-record parsing. Rejected: Require every caller to pass timestamps | exported helpers are still public test surfaces and should be safe by default. Confidence: high Scope-risk: narrow Directive: Do not reintroduce wall-clock defaults in policy-rfc generation without deterministic tests. Tested: pnpm run build:all; node --test test/policy-rfc.test.ts
Summary
Adds a conservative Policy RFC Engine that scans existing ClawSweeper durable records and generates reviewable RFC-style policy proposals from repeated operational patterns.
This is proposal-only infrastructure. It does not mutate GitHub, dispatch repairs, close issues, alter apply behavior, or change scheduler/automerge paths.
What changed
src/policy-rfc/with collector, scorer, synthesizer, types, and CLI runner.pnpm run policy-rfc.docs/policy-rfc-engine.md.How to try
Generates RFC proposals from local durable records without mutating GitHub.
Validation
Passed:
pnpm installpnpm run buildpnpm run build:allnode --test test/policy-rfc.test.tsFull
pnpm testreaches the build step successfully and then fails in this local Windows / Node 22.19.0 environment. The package requires Node >=24. Visible failures include path separator / CRLF assertions, Codex spawnEPERM,pnpm ENOENTin validation subprocesses, and one repair hydration assertion. The new Policy RFC tests pass.RFC and JSON outputs are deterministic for the same input records to ensure stable diffs and testability.
Safety
The first version is intentionally local-record only and proposal-only. It does not hook into scheduler lanes or GitHub mutation paths.
Non-Goals