Skip to content

feat(config): add safe harness-scoped policy profiles - #112

Closed
A1igator wants to merge 2 commits into
mainfrom
agent/harness-policy-profiles
Closed

feat(config): add safe harness-scoped policy profiles#112
A1igator wants to merge 2 commits into
mainfrom
agent/harness-policy-profiles

Conversation

@A1igator

@A1igator A1igator commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What

  • add harness-scoped hermes and openclaw policy profiles selected by TENJIN_HARNESS or --profile
  • let profiles tune read auto-spend, rolling session budget, confirmation, creator allowlisting, and publish defaults
  • keep the wallet, spend ledger, sendMaxAmount, endpoints, telemetry, credentials, and installer state global-only, with schema-level rejection for misplaced keys
  • add tenjin config unset <key> --profile <name> so an override can safely fall through to the global layer
  • identify active profiles and invalid selectors in config/doctor output, and document the environment-policy caveat for CLI and MCP permissions

Why

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

  • Existing installations are unchanged when no profile is selected.
  • Explicit --profile wins over TENJIN_HARNESS; unknown selectors fail closed and are reported.
  • A global config set still writes the global layer, then reports the effective profile value if that layer remains shadowed.
  • Known global-only keys are rejected inside profile data while unknown future profile keys remain forward-compatible.
  • Empty profile creator allowlists are refused; use config unset to inherit instead.

Checks

  • pnpm typecheck
  • pnpm lint
  • pnpm prettier --check .
  • pnpm build
  • 1,689 tests passed; 10 skipped
  • eval-harness localhost tests passed with loopback socket access

@A1igator A1igator added the priority: high High priority label Aug 9, 2026
@A1igator
A1igator marked this pull request as ready for review August 9, 2026 19:43

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@vraspar vraspar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, policy is undefined and every profileOrFile falls through to the global value; the layering is byte-identical to main's behavior.
  • Precedence is deterministic and right: explicit --profile beats TENJIN_HARNESS (input.profile ?? resolvePolicyProfileName(env)), and unknown --profile names 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 persist helper as every other config set, and unknown keys/sibling profiles survive round-trips.

Major

  1. [integrity] Enforce the global-only invariant in the schema and single-source the profile key set; today the invariant lives only at the config set edge and its tests hold vacuously: PolicyProfileConfigSchema is .passthrough(), so {"policyProfiles":{"hermes":{"sendMaxAmount":"none","baseUrl":"https://evil.example"}}} loads without error (reproduced); the only enforcement is isProfileScalarKey in the config command, which the exported installer seam persistPolicyProfile (used by #113) bypasses. The resolver ignores these keys today (verified: resolveSendMaxAmount/resolveBaseUrl take only config), but the tests asserting that are vacuous — the fixture profile contains no global-only keys, so switching resolveSettings to profileOrFile('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, ProfileScalarKey at src/lib/config.ts#L432, PROFILE_SCALAR_KEYS at src/commands/config.ts#L50-L55) — satisfies catches an invalid entry but not a missing one. Fix: add a superRefine deny-list to PolicyProfileConfigSchema rejecting the global-only keys (sendMaxAmount, baseUrl, rpcUrl, evalCohort, install, policyProfiles), export one PROFILE_SCALAR_KEYS from lib/config.ts and derive the other two enumerations from it, and add a resolver test whose fixture puts sendMaxAmount/baseUrl inside a profile and asserts the load fails.

Minor

  1. [usability/security] Surface the active (and stored) profile on the reporting surfaces; the profile layer is currently invisible everywhere it matters: EffectiveSettings.policyProfile is computed and read by nothing. Instances: bare tenjin config gives no hint a stored $0.25/$5/auto-publish profile exists; under TENJIN_HARNESS=hermes no human line and no --json key names the profile; config set without --profile reports the written global value with source: 'file' while the profile still wins (reproduced: set 0.05, next get returns 0.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'd TENJIN_HARNESS silently resolves global; doctor says nothing. Fix: add policyProfile to the config/get payloads plus a human line, re-resolve after set and report the effective entry, warn on an unrecognized TENJIN_HARNESS, and add a doctor line. src/commands/config.ts#L141-L150

  2. [integrity] Add config unset <key> --profile; a profile key can be written but never removed, and the natural clearing gesture widens policy: profileOrFile treats only undefined as absent, so a written key shadows the global forever without hand-editing config.json. Worse, config set allowlistCreators "" --profile hermes stores [], and policy.allowlistCreators.length > 0 reads [] as no creator restriction, silently switching a global hard gate off for the autonomous harness. src/lib/policy.ts#L81

  3. [docs/security] Add TENJIN_HARNESS to 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 capability tenjin config set is NEVER_ALLOWLISTED for, yet it appears in neither FLAG_CAVEAT/MCP_CAVEAT (src/lib/permissions.ts#L282-L307) nor docs/agent-permissions.md nor 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/rpcUrl are 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 --profile refuses global-only keys with an it.each covering sendMaxAmount, 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.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@A1igator A1igator changed the title feat(config): add harness-scoped policy profiles feat(config): add safe harness-scoped policy profiles Aug 10, 2026
@A1igator

Copy link
Copy Markdown
Contributor Author

Addressed all review feedback in 3dbae83 and merged current main to clear the conflict.

  • Added schema-level rejection for all known global-only profile keys and exported one PROFILE_SCALAR_KEYS source shared by validation, typing, and config commands. Tests cover every denied key plus forward-compatible unknown keys.
  • Chose decision (b): a global config set writes global state, then re-resolves and reports the still-effective profile value when TENJIN_HARNESS shadows it.
  • Config output now names an active profile in both human and machine output, warns on an invalid env selector, and doctor reports the same state without failing on a typo.
  • Added config unset <key> --profile for scalar and publish fields. Empty profile creator allowlists are refused with an unset fix so clearing cannot silently widen policy.
  • Added TENJIN_HARNESS to the CLI/MCP permission caveats, permissions guide, search skill, README, and changeset.

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.

@vraspar

vraspar commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Owner decision: please close this PR. It will not merge, and not because of code quality.

Why:

  1. It has no consumers anymore. The feat(install): add native Hermes retrieval and publish-back hooks #113 rework dropped its dependency on the profile machinery (/bin/zsh.25/$5 autonomous defaults, TENJIN_HARNESS selection), which was the only thing consuming it. See the direction comment on feat(install): add native Hermes retrieval and publish-back hooks #113: feat(install): add native Hermes retrieval and publish-back hooks #113 (comment)

  2. Its premise only ever applied to a shrinking group. Per-harness spend policy is only meaningful for users of the Tenjin-managed wallet. Users who bring their own wallet get their spend policy from their own wallet middleware, and that is where it belongs; our side stays provider-neutral. For Tenjin-wallet users, the decision is one conservative global policy for every harness until real usage demonstrates a need for more, and loosening autonomous-spend defaults per harness is an owner-level product decision that is being declined for now, not deferred.

  3. Keeping it open invites building on it. Closed now, the branch and its history remain available; if a per-harness policy need ever materializes from real users, we revisit with a decided spec and can resurrect from here.

The machinery itself (fail-closed unknown profiles, global-only sendMaxAmount/endpoints, existing installs unchanged) was carefully built, and none of that care is wasted: the same discipline is exactly what the #113 fix round needs. Thanks for the fast turnaround on the rework there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants