feat(config): add safe harness-scoped policy profiles - #112
Conversation
There was a problem hiding this comment.
A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
vraspar
left a comment
There was a problem hiding this comment.
Review: sound foundation; one major (schema-level enforcement of global-only keys) before merge
Reviewed against the PR body's guarantees and the spend-policy surface, verified at 5267088. Findings were independently re-verified against the checked-out head; line-cited claims below were reproduced, not just read.
What's solid:
- No-profile behavior is provably unchanged: with
profile === undefined,policyis undefined and everyprofileOrFilefalls through to the global value; the layering is byte-identical to main's behavior. - Precedence is deterministic and right: explicit
--profilebeatsTENJIN_HARNESS(input.profile ?? resolvePolicyProfileName(env)), and unknown--profilenames fail closed with a USAGE error naming the valid set. - Wallet, spend ledger, and config paths derive solely from
dataDir; the rolling session budget goes through the shared atomic spend ledger, not a per-process counter. - Profile writes reuse the same locked, atomic
persisthelper as every otherconfig set, and unknown keys/sibling profiles survive round-trips.
Major
- [integrity] Enforce the global-only invariant in the schema and single-source the profile key set; today the invariant lives only at the
config setedge and its tests hold vacuously:PolicyProfileConfigSchemais.passthrough(), so{"policyProfiles":{"hermes":{"sendMaxAmount":"none","baseUrl":"https://evil.example"}}}loads without error (reproduced); the only enforcement isisProfileScalarKeyin the config command, which the exported installer seampersistPolicyProfile(used by #113) bypasses. The resolver ignores these keys today (verified:resolveSendMaxAmount/resolveBaseUrltake onlyconfig), but the tests asserting that are vacuous — the fixture profile contains no global-only keys, so switchingresolveSettingstoprofileOrFile('sendMaxAmount', ...)leaves every test green. Contributing instances of the same root cause: the profile key set is hand-enumerated in three unlinked places (zod fields at src/lib/config.ts#L49-L57,ProfileScalarKeyat src/lib/config.ts#L432,PROFILE_SCALAR_KEYSat src/commands/config.ts#L50-L55) —satisfiescatches an invalid entry but not a missing one. Fix: add asuperRefinedeny-list toPolicyProfileConfigSchemarejecting the global-only keys (sendMaxAmount,baseUrl,rpcUrl,evalCohort,install,policyProfiles), export onePROFILE_SCALAR_KEYSfromlib/config.tsand derive the other two enumerations from it, and add a resolver test whose fixture putssendMaxAmount/baseUrlinside a profile and asserts the load fails.
Minor
-
[usability/security] Surface the active (and stored) profile on the reporting surfaces; the profile layer is currently invisible everywhere it matters:
EffectiveSettings.policyProfileis computed and read by nothing. Instances: baretenjin configgives no hint a stored $0.25/$5/auto-publish profile exists; underTENJIN_HARNESS=hermesno human line and no--jsonkey names the profile;config setwithout--profilereports the written global value withsource: 'file'while the profile still wins (reproduced: set0.05, nextgetreturns0.25 (profile)— loudest in the tightening direction, where an operator lowering auto-spend gets a green result and keeps spending at the old ceiling); a typo'dTENJIN_HARNESSsilently resolves global;doctorsays nothing. Fix: addpolicyProfileto theconfig/getpayloads plus a human line, re-resolve aftersetand report the effective entry, warn on an unrecognizedTENJIN_HARNESS, and add a doctor line. src/commands/config.ts#L141-L150 -
[integrity] Add
config unset <key> --profile; a profile key can be written but never removed, and the natural clearing gesture widens policy:profileOrFiletreats onlyundefinedas absent, so a written key shadows the global forever without hand-editing config.json. Worse,config set allowlistCreators "" --profile hermesstores[], andpolicy.allowlistCreators.length > 0reads[]as no creator restriction, silently switching a global hard gate off for the autonomous harness. src/lib/policy.ts#L81 -
[docs/security] Add
TENJIN_HARNESSto the permission-caveat surface: it is an env-only lever that moves hard-deny gates (sessionBudget,allowlistCreators,publish.mode) on already-allowlisted verbs, the exact class of capabilitytenjin config setis NEVER_ALLOWLISTED for, yet it appears in neitherFLAG_CAVEAT/MCP_CAVEAT(src/lib/permissions.ts#L282-L307) nordocs/agent-permissions.mdnor the skills text. Doc-level fix is sufficient for this PR since nothing here seeds a profile; see the consent decision raised on #113, where the installer does.
Verified, not issues
- Runtime endpoint/credential isolation holds even with a crafted config: profile-scoped
sendMaxAmount/baseUrl/rpcUrlare ignored by the resolver (the Major is about the missing schema gate and test, not a live exploit). - Prototype pollution via
__proto__profile names does not reach resolution. - Session budget rides the shared atomic ledger under the file lock.
config set --profilerefuses global-only keys with an it.each coveringsendMaxAmount,baseUrl,rpcUrl,evalCohort.- 154 tests across the four touched test files pass at head.
Decisions (owner): when config set runs with no --profile while a profile is active, should it (a) refuse with a USAGE error naming the shadowing profile, or (b) write global and report the still-effective profile value? (a) never leaves a stale ceiling in force; (b) preserves the documented "no --profile means global" contract. Minor 1's fix assumes (b); either is fine, pick one.
Verdict: comments-only. Fix the Major before merge; the minors can ride this PR or a fast follow-up. Ran: focused vitest on the four touched test files (154 passing), live probes of the set/get shadowing and the passthrough schema at head.
There was a problem hiding this comment.
A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed all review feedback in
Validation: typecheck, lint, repository-wide Prettier, production build, and the full suite (1,689 passed / 10 skipped). The eval-harness tests were rerun with localhost socket access and passed. |
|
Owner decision: please close this PR. It will not merge, and not because of code quality. Why:
The machinery itself (fail-closed unknown profiles, global-only |
What
hermesandopenclawpolicy profiles selected byTENJIN_HARNESSor--profilesendMaxAmount, endpoints, telemetry, credentials, and installer state global-only, with schema-level rejection for misplaced keystenjin config unset <key> --profile <name>so an override can safely fall through to the global layerWhy
Hermes and OpenClaw need autonomous defaults without weakening the conservative settings used by Codex and Claude Code. The policy boundary must also remain inspectable and reversible: operators should be able to see which layer wins, remove an override without hand-editing JSON, and avoid accidentally widening an empty creator allowlist.
Behavior and safety
--profilewins overTENJIN_HARNESS; unknown selectors fail closed and are reported.config setstill writes the global layer, then reports the effective profile value if that layer remains shadowed.config unsetto inherit instead.Checks
pnpm typecheckpnpm lintpnpm prettier --check .pnpm build