Skip to content

fix(sdk, collector): apply role-keyed top-level constraints, widen revocation staleness - #199

Open
guillaume-hexamind wants to merge 5 commits into
mainfrom
gp/feat/collector_cache
Open

fix(sdk, collector): apply role-keyed top-level constraints, widen revocation staleness#199
guillaume-hexamind wants to merge 5 commits into
mainfrom
gp/feat/collector_cache

Conversation

@guillaume-hexamind

@guillaume-hexamind guillaume-hexamind commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Two independent changes on one branch

1. fix(sdk) — a role-keyed policy's top-level constraints: was silently dropped

load_policy_set_from_dict validated only the values under roles:, so every other top-level key parsed and was discarded. An author wrote a role-wide fence, got no fence, and no error. The flat shape was never affected — there the whole payload validates as one AgentPolicy — so the same key meant two different things depending on whether the document declared roles.

  • A file-level constraints: block now unions into every role. Union, not replace: a role dropping the file's fence would be fail-open, the same reason constraints union across inherits.
  • Any other unrecognised sibling of roles: is now a PolicySetError naming the key. That closes the defect class rather than this one instance, and matches composed module policies, which are already extra="forbid" at every scope.
  • The block's own shape is checked before the hoist — a bare string would otherwise splat into single characters, each a valid str.

Verified past the unit tests: the fence compiles into the Rego bundle per role, so this holds on the WASM engine and not only the pydantic path; and the resolve → build round-trip still loads, since hexgate policy resolve emits {"roles": …, "_resolved": true} for a multi-role result.

Upgrade note — anyone who wrote such a fence speculatively starts denying on upgrade, and the policy's compiled source_hash moves with it. Documented in docs/policy/constraints.mdx, alongside the note #155 wrote for the flat shape.

Found by a code review of #155 (already merged), then reproduced before being fixed.

2. feat(collector) — revocation max_staleness 2m → 1h, both knobs env-tunable

2m sat under the length of routine control-plane maintenance (minor-version upgrade, volume resize, slow restart), so a Postgres blip meant total ingest rejection — silently, because the deploy healthcheck is a TCP-connect probe that stays green while every request 401s, and refused spans never reach the Redpanda buffer that exists to absorb this.

  • poll_interval stays 20s, so steady-state revocation latency is unchanged. The widened window applies only while refreshes are failing.
  • Both knobs override via HEXGATE_COLLECTOR_REVOCATION_POLL_INTERVAL / _MAX_STALENESS, per stage, no image rebuild.
  • The env default applies only when a variable is unset, so the deploy stack passes them with non-empty fallbacks: a blank value arrives as 0s and the collector refuses to boot (verified against the built binary).

collector-check gained an accepted and a refused override case. The refusal is the load-bearing one — it can only fail if the env values actually reached the config struct, so a typo in either variable name fails the build instead of shipping a dead override.

Testing

make check-all green: SDK 2570 passed, platform-api 638, dashboard 218, collector ok plus the three validate cases. The platform-api run matters here specifically — it compiles signed bundles through the same policy loader, so the new strict rejection could have broken bundle compilation. It didn't.

Not in scope

Three further findings from the same review of #155, none addressed here:

  • a named-export import (caps.yaml#export) drops the imported file's own top-level boundary: while a whole-file import of that same file raises — same fail-open class, reproduced;
  • agent.run admission is audited outside run_scope on the LangChain / LangGraph / OpenAI paths but inside it on Google, so the row admitting a run has no run_id to join on;
  • the CLI's constraint localization doesn't reach the file-level block, so a malformed expression there reports as a schema error rather than default → <policy>.

…nobs env-tunable

The 2m default turned routine control-plane maintenance (minor-version
upgrade, volume resize, slow restart) into total ingest rejection, silently:
the deploy healthcheck is a TCP-connect probe, so it stays green while every
request 401s past max_staleness, and refused spans never reach the Redpanda
buffer that exists to absorb downstream wobble.

poll_interval is unchanged at 20s, so steady-state revocation latency is
unaffected — the widened window applies only while refreshes are failing.

Both knobs are now per-stage overridable via
HEXGATE_COLLECTOR_REVOCATION_POLL_INTERVAL / _MAX_STALENESS, following the
HEXGATE_COLLECTOR_* convention already used for the DSN, endpoints and key
path. The env default applies only when a variable is UNSET, so the deploy
stack passes them with non-empty fallbacks: a blank value would arrive as 0s
and refuse to boot.

collector-check asserts both the accepted and the refused override case. The
refusal is the load-bearing one — it can only fail if the env values reached
the config struct, so a typo in either variable name fails the build instead
of shipping a dead override. A new unit test pins the two literal durations,
which the existing default-config test could not (it asserts the constants
against themselves).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
hexgate/security/policy_set.py 84.61% 2 Missing and 2 partials ⚠️
platform/api/hexgate_api/features/agents/router.py 92.30% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

guillaume-hexamind and others added 4 commits September 9, 2026 14:38
…f dropping them

load_policy_set_from_dict validated only the values under `roles:`, so every
other top-level key parsed and was discarded. A `constraints:` sibling of
`roles:` therefore never reached any policy: the author wrote a role-wide fence,
got no fence, and no error. The flat shape was never affected — there the whole
payload validates as one AgentPolicy, which has the field — so the same key
meant two different things depending on whether the document declared roles.

A file-level `constraints:` block now unions into every role. Union, not
replace: constraints are the one policy field that unions across `inherits`,
because a child dropping an inherited fence would be fail-open, and a file-level
fence is the same kind of promise. The block's own shape is checked before the
hoist — a bare string would otherwise splat into single characters, each a valid
str, and pydantic would accept the wreckage.

Any other unrecognised sibling of `roles:` is now a PolicySetError naming the
key, rather than a silent drop; `roles`, `constraints`, `version` and the
resolved marker are the file-level keys. That closes the defect class, not just
this instance, and brings the inline shape in line with composed module
policies, which are already extra="forbid" at every scope.

The fence reaches both engines (it compiles into the Rego bundle per role, not
only the pydantic path), and the resolve -> build round-trip still loads, since
`hexgate policy resolve` emits {"roles": ..., "_resolved": true} for a
multi-role result. Both are pinned by tests.

Side effect worth knowing: a malformed expression in that block is no longer
invisible to `hexgate policy validate` — it now surfaces as a schema error,
though without the localized "default -> <policy>" form the flat shape gets.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…branch

Eleven blocks across the two commits explained what the code already said, or
argued a decision over three sentences where one carried it. Kept only the why a
reader cannot deduce -- the fail-open reason union beats replace, why an hour
rather than two minutes, why the refused validate case is the load-bearing one
-- and dropped the narration around it.

Prose only: no logic, no test assertions, no config values changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The new file-level strictness only guarded siblings of `roles:`, and only in
the SDK loader, which left three ways for a document to pass validation and
then fail to load.

- platform-api: `/validate` kept its own shape dispatch, so it never
  grammar-checked a file-level `constraints:` block nor rejected an unknown
  sibling of `roles:`, and `_default_role_warnings` swallowed `PolicySetError`
  so `ok` stayed True. Route the document through `load_policy_set_from_dict`
  — the loader the compiler and the SDK both go through — and report the
  failure instead of dropping it. Loads once now and hands the set to the
  lints.

- sdk: `AgentPolicy` had no `extra="forbid"`, so a mistyped field inside a
  role (or in a flat document) was dropped in silence — a dropped fence is
  fail-open, and inside a role is the likelier place to write one.

- sdk: the unknown-key error advertised `_resolved` as a legal file-level
  key. Setting it by hand loads the document as already-resolved, which
  switches off the reserved-tool-name guard; drop it from the advertised list.

Two existing tests pinned the old behaviour: the unresolvable-inheritance
case asserted `ok` for a document the compiler rejects, and
`_trivial_policy_yaml` used `name:`/`rules:` — neither an `AgentPolicy` field
— so it validated as an empty policy and signed an empty bundle while
claiming to compile cleanly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@guillaume-hexamind
guillaume-hexamind marked this pull request as ready for review September 9, 2026 13:40
@guillaume-hexamind guillaume-hexamind self-assigned this Sep 9, 2026
@guillaume-hexamind guillaume-hexamind changed the title feat(collector): raise revocation max_staleness to 1h and make both knobs env-tunable fix(sdk, collector): apply role-keyed top-level constraints, widen revocation staleness Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant