[aivong-openhands]

Summary

• Add Helm orgDefaults.condenser.maxTokens, applyToExisting, and overwriteExisting values.
• Render the corresponding enterprise backend env vars only when maxTokens is set, guarding the nested lookup so an explicitly nulled orgDefaults/condenser block renders cleanly.
• Coerce maxTokens through int64 before quoting, so a values file using YAML float or exponent notation (1e6) renders "1000000" rather than "1e+06". JSON Schema's integer type accepts a zero-fraction float, and the backend parses the value with isdecimal() ahead of its apply_to_existing early return, so the unconverted form raised out of the app-server startup lifespan and crashlooped the app tier regardless of the rollout flags.
• Add schema validation for types and a positive-integer minimum, plus render-time guards for contradictory rollout flag combinations.
• Add Helm unit coverage for env rendering, null-block renders, null rollout flags, exponent-notation thresholds, schema validation, and rollout guard behavior.
• Document that reconciliation is triggered from the app-server startup lifespan — so it runs once per saas_server worker process and never from the maintenance CronJobs that share the env block — along with rollback caveats, the SDK-side effective-limit behavior, and the tracking PR for the app version that reads these vars.
• Defer the Replicated/KOTS config surface until the chart app version includes backend support for these env vars.

Validation

• helm unittest charts/openhands (154 passed)
• helm lint charts/openhands
• helm dependency build charts/openhands
• helm template with --set orgDefaults=null and --set orgDefaults.condenser=null
• helm template with a values file setting maxTokens: 1e6 and maxTokens: 200000.0 (render "1000000" and "200000")
• Mutation-checked the new tests by dropping | default false and | int64 from _env.yaml (each fails as expected)
• Confirmed the CronJobs cannot trigger reconciliation from the call graph rather than the import graph: _reconcile_org_condenser_defaults has exactly one non-test caller (SaasAppLifespanService.__aenter__), which only becomes a lifespan via get_app_lifespan_service() in openhands/app_server/app.py, and the three CronJob entrypoints run python -m paths that never construct the FastAPI app. An earlier revision of this description claimed an AST walk showed the entrypoints do not import saas_server/app_lifespan; that measured direct imports only and was wrong about the transitive closure — see the round-4 review. Importing does not execute, so the conclusion stands for the call-graph reason.
• uv run --with pytest --with requests --with PyGithub --with fastapi --with uvicorn --with httpx --with ruamel.yaml --with PyYAML python -m pytest scripts -q

This PR was updated by an AI agent (OpenHands) on behalf of the user.

@aivong-openhands can click here to continue refining the PR

[aivong-openhands]

Code review — feat: PLTF-3553 Add Helm values for org condenser defaults

The chart plumbing itself is clean: it follows the dailyConversationLimit / duplicateEmailCheck precedent, it goes through openhands.env.defaults so .Values.env de-duplication still lets consumers override, the schema is tightened alongside the values, and both the Helm unit tests and the Replicated contract test exercise the real files rather than mocks. Defaults are null/false, which is the right posture for a self-hosted user. CI is green.

I'm commenting rather than approving because of three things worth resolving before this ships.

1. The env contract lands ahead of the app that reads it (important)

OPENHANDS_ORG_DEFAULTS_CONDENSER_* exists only in the still-open OpenHands/enterprise#337. This chart pins appVersion: 1.59.0, which has no reader for those variables. Merged as-is, an operator can set orgDefaults.condenser.maxTokens — or, more likely, fill in the new Replicated field whose help text describes concrete behavior — and get silence.

The Replicated exposure is the risky half. KOTS config is the self-hosted operator's contract surface, and a toggle that does nothing is worse than an absent one: there's no feedback that tells them it isn't wired yet. Suggestion: land enterprise#337 and the image-tag bump first, or split this so the chart values merge now and the replicated/config.yaml surface follows the appVersion that supports it.

2. applyToExisting + overwriteExisting is one knob split across two booleans (important)

Two booleans give four combinations, and one of them lies: applyToExisting: false with overwriteExisting: true renders ..._OVERWRITE_EXISTING=true into the pod and does nothing, because the backend returns early when apply_to_existing is false. That's the "booleans must stay truthful in every value combination" test in this repo's own review guidelines.

Replicated is protected here — the when: conditions plus the and (...) in openhands.yaml make the invalid combination unreachable — but a plain Helm consumer has no such guard, and the chart values interface is the one self-hosted users read.

These are really three states of one rollout decision. Either collapse them:

orgDefaults:
  condenser:
    maxTokens: null
    # none | fillUnset | overwrite
    reconcileExisting: none

…or, if you'd rather keep the shape aligned 1:1 with the backend env vars, add a guard in templates/validations.yaml, which exists for exactly this class of problem (ingress.host, minio.buckets[].purge).

3. "Rollout reconciliation, not continuous enforcement" isn't true of a Helm value (important)

The README frames this as a one-time rollout, but reconciliation runs from SaasAppLifespanService.__aenter__ — i.e. on every app start — while the Helm value that arms it is declarative and persistent. Leaving overwriteExisting: true in a site values file means every pod restart, rollout, HPA scale-up, and node eviction silently re-clobbers any condenser.max_tokens an org admin changed in the UI since the last restart. That's continuous enforcement with an unpredictable trigger, and "take a database backup before enabling it" doesn't help against the second and later occurrences.

Two ways out, either is fine:

• Say plainly in the README (and the KOTS help text) that the flag stays armed until it's removed from values, and should be reverted once the rollout completes.
• Model the reconciliation as something inherently one-shot — a helm.sh/hook: post-upgrade Job, the pattern the chart already uses for the MinIO bucket job — rather than a startup side effect of a long-lived Deployment.

Minor

Three inline notes; none of them block.

---

This review was generated by an AI agent (OpenHands) on behalf of the user, following .agents/skills/code-review.md.

[aivong-openhands]

Addressed the review feedback in 848f9a9:

• Deferred the Replicated/KOTS config surface until the chart app version includes backend support for the org condenser env contract.
• Folded all org-condenser env rendering under maxTokens so default installs do not get inert rollout env vars.
• Added Helm render-time guards for rollout flags without a threshold and overwriteExisting: true without applyToExisting: true.
• Added a maxTokens ceiling and schema coverage.
• Updated README language to make repeated app-start reconciliation and the need to remove rollout flags after completion explicit.

Local validation and GitHub checks are passing.

This comment was generated by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

Code review — feat: PLTF-3553 Add Helm values for org condenser defaults (round 2)

Good progress on the three items from the last round:

• The Replicated/KOTS surface is gone, so the risky half of "the env contract lands ahead of the app" — an operator-facing toggle with concrete help text and no reader — is no longer shipping. Deferring it until the appVersion carries the backend is the right call.
• templates/validations.yaml now rejects overwriteExisting without applyToExisting, and all three env vars render only under maxTokens, so the two-boolean shape no longer has a combination that lies and default installs get no inert env. That was one of the two options I offered, and the guard follows the ingress.host / minio.buckets[].purge precedent.
• The README now says plainly that reconciliation runs on every app start and stays armed until the values are removed, and calls out the rollback caveat.

I re-ran the suites locally against 6d0a05c (helm unittest -f tests/org_defaults_condenser_test.yaml -f tests/validations_test.yaml -f tests/values_schema_test.yaml charts/openhands → 39 passed) and CI is green.

Commenting rather than approving because of one render bug and one limit I think is wrong for self-hosted users.

1. Unguarded nested dereference breaks renders that null the block (important)

_env.yaml:37 and both new guards in validations.yaml walk .Values.orgDefaults.condenser.maxTokens with no nil guard. A site values file that nulls either level — a normal way to say "I don't use this" — takes the whole render down with an opaque error instead of a chart message. Verified on this branch:

--set orgDefaults=null
  _env.yaml:37: nil pointer evaluating interface {}.condenser
--set orgDefaults.condenser=null
  _env.yaml:37: nil pointer evaluating interface {}.maxTokens

Empty maps (orgDefaults: {}, orgDefaults.condenser: {}) render fine, so it's specifically the explicit-null path. _env.yaml already handles this everywhere else with | default dict (lines 2, 427, 799), and the dailyConversationLimit precedent this change follows is a top-level key that can't hit the problem. Either adopt the default dict chain, or tighten the schema so the failure is a schema message rather than a template panic. Worth a test case either way — the new suite covers the happy paths but not this one.

2. maximum: 1000000 is a ceiling the chart invented (important)

The schema hard-caps maxTokens at 1,000,000. The backend contract in OpenHands/enterprise#337 has no such limit — _parse_positive_int_env only requires a positive decimal integer — so the ceiling exists solely in the chart, and the only escape for a consumer who needs more is a raw .Values.env override or a fork.

That's a defaults-serve-the-self-hosted-user problem: models with context windows above 1M are already shipping, and an operator running one gets a render failure with no chart-level way to say what they mean. minimum: 1 earns its keep — it blocks a nonsense value. The maximum blocks a legitimate one. Suggest dropping it, or raising it well clear of real model limits if you want a sanity bound.

Related: README:23 says higher values are "rejected at render time instead of relying on backend clamping to the model input limit," and that "the effective threshold cannot exceed the model input limit." I can't find that clamp in enterprise#337 — the value is parsed and written through as-is. If it lives elsewhere in the stack, a pointer would settle it; if it doesn't exist yet, the README describes behavior an operator will rely on and not get, which is the same class of problem as an unwired toggle.

3. Still worth a decision: does the chart value merge before the backend?

enterprise#337 is still open and this chart pins appVersion: 1.59.1, which has no reader for OPENHANDS_ORG_DEFAULTS_CONDENSER_*. Dropping the KOTS surface removes most of the blast radius, and a Helm value documented in the chart README is a much softer contract than a KOTS field — so I won't argue this blocks. But an operator who sets maxTokens on 1.59.1 today still gets silence, and nothing in the chart tells them. If you'd rather not gate the merge on enterprise#337, consider a one-line README note that these values take effect from the appVersion that ships the reader.

Minor

Stray blank lines: values.yaml:44 and README.md:38-39.

---

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands, following .agents/skills/code-review.md.

[aivong-openhands]

Addressed the round-2 feedback in d7de21c.

1. Unguarded nested dereference — both _env.yaml and validations.yaml now derive the block via (.Values.orgDefaults | default dict).condenser | default dict, the idiom already used elsewhere in _env.yaml. I kept direct field access rather than get, because get returns "" for a missing key, which would have satisfied ne ... nil and rendered an empty MAX_TOKENS. Both null paths (--set orgDefaults=null, --set orgDefaults.condenser=null) now render clean, and the schema accepts an explicit null at both levels so the value is legal rather than merely harmless. Test cases added for both.

2. maximum: 1000000 — dropped; you are right that it is chart-invented and blocks a legitimate value. minimum: 1 stays. The ceiling test is inverted into an acceptance test at maxTokens: 2000000.

3. README clamping claim — the clamp is real but lives in the SDK, not enterprise#337: LLMSummarizingCondenser._effective_max_tokens takes min(self.max_tokens, agent_llm.effective_max_input_tokens). The README now describes that instead of the render-time rejection, and states the operator-visible consequence (a value above the LLM input limit has no effect).

Also from item 3 of the review — added a one-line README note that these values take effect only from the app version that reads OPENHANDS_ORG_DEFAULTS_CONDENSER_*, so an operator on the current appVersion is told rather than left with silence. enterprise#337 is still open; the chart values are deliberately merging ahead of it with that caveat documented.

Minor — stray blank lines in values.yaml:44 and README.md:38-39 removed.

Validation: helm unittest charts/openhands → 149 passed, helm lint charts/openhands clean, and the null-block render cases verified by hand with helm template.

This comment was generated by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

Code review — feat: PLTF-3553 Add Helm values for org condenser defaults

Reviewed against the repo's .agents/skills/code-review.md guidelines. I cloned the branch, built dependencies, and ran the suite locally: 152 tests pass, helm lint clean, CI green on 9907d95.

The chart mechanics here are good. The env vars flow through openhands.env.defaults, so they inherit the de-duplication wrapper for free and consumers can override via .Values.env — the established pattern is followed, not reinvented. The nil-guarding on (.Values.orgDefaults | default dict).condenser | default dict is correct, and I verified the explicitly-nulled-block renders. The tests are genuinely load-bearing rather than decorative: I mutation-tested the notExists assertions by forcing the _env.yaml conditional to true, and 3 tests correctly failed.

Holding at COMMENT rather than approving because of one finding I think is important and two I'd like answered before this leaves draft.

1. The reconciliation env vars reach 6 workloads, including a CronJob that runs every 15 minutes

This is my main concern. Because the vars are added to the shared openhands.env.defaults helper, they render into far more than the app Deployment. With default values only:

Workload	Cadence
Deployment/openhands	rollout / restart
Deployment/openhands-integrations	rollout / restart
Deployment/openhands-mcp	rollout / restart
CronJob/…-app-conversation-start-task-clean	0 3 * * *
CronJob/…-maintenance-tasks	0 6 * * *
CronJob/…-budget-maintenance	*/15 * * * *

All six run the same enterprise-server:1.59.1 image. If the reconciliation is triggered on process startup — which is what the README describes — then overwriteExisting: true is not a rollout-scoped event at all; budget-maintenance alone would re-run it 96 times a day, and the three Deployments each carry it per-replica and per-restart.

That undercuts the README's mitigation advice, which frames removal as something you do "after the rollout completes." There may be no window in which the rollout is complete and reconciliation has stopped.

I can't verify the trigger point from this repo (see #3), so this is a genuine question rather than an assertion: does the reconciliation run on interpreter startup, or only under the uvicorn/saas_server app path? If it's startup-scoped, a destructive overwrite firing on a 15-minute timer is a serious problem, and the env vars should be scoped to the specific workload that owns the migration instead of the shared helper.

2. A one-shot destructive migration expressed as a persistent declarative value

The PR is admirably candid that overwriteExisting: true is destructive and one-way — backup first, no rollback, corrective SQL if you need to undo. But Helm values are persistent and declarative, while this operation is imperative and one-time. The intent "overwrite once during this rollout" has to be encoded as a value that then sits in a site values file indefinitely, which is exactly how the README's pod-restart/HPA/eviction footgun arises.

This bumps against the skill's "booleans must stay truthful in every value combination" test and the OHE-3033 lesson about mismatched lifetimes. As written, the README mitigates a structural problem with documentation and operator discipline.

There's already precedent in this chart for one-shot work: laminar-clickhouse-diagnostic-retention-job.yaml uses helm.sh/hook: post-install,post-upgrade with a hook-delete-policy. A hook Job would make the migration fire exactly once per upgrade, keep it out of the long-running Deployments and CronJobs entirely, and resolve #1 as a side effect. Worth considering — I recognise it's a bigger change and depends on where the backend puts the logic.

3. No backend reads these variables yet, and the README's version reference is unbounded

An org-wide code search for ORG_DEFAULTS_CONDENSER returns 0 results, and chart appVersion is 1.59.1. So today these env vars render into six pods and do nothing — which the PR acknowledges by deferring the Replicated/KOTS surface.

The problem is that the README says these "only take effect from the app version that reads the OPENHANDS_ORG_DEFAULTS_CONDENSER_* environment variables" without ever naming that version. A self-hosted operator can't act on that. Meanwhile the guard messages in validations.yaml assert behaviour ("overwrite mode only runs while reconciling existing organizations") that no shipped code implements, so the chart is now the de-facto spec for an unwritten backend and the two can drift.

Are the variable names and the apply/overwrite semantics locked with the backend work? If so, naming the target appVersion — even approximately — would make the docs actionable.

Nits (non-blocking)

• maxTokens reads like an LLM output-token cap; it's actually a condensation trigger threshold. Something like condensationTriggerTokens would be self-describing per the skill's "name the effect, not the scenario" test. The README explains it well, but that explanation doesn't travel into consumer values files alongside the key.
• values.schema.json uses "type": "boolean" for the two flags while maxTokens allows null. Not a bug — I verified a null in user values drops the key and | default false covers it — just noting the asymmetry looks deliberate and is fine.

Risk assessment

Medium. The chart-side implementation is careful, well-tested, and inert by default (maxTokens: null ships disabled), so a default install is unaffected — that's the right call per "defaults serve the self-hosted user." Risk concentrates entirely on the opt-in path: an operator who sets overwriteExisting: true is performing an irreversible database mutation whose actual execution frequency is, per #1, likely much higher than the documentation implies. Findings #1 and #3 are worth resolving before this leaves draft; #2 is a design discussion.

---

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands.

[aivong-openhands]

Thanks — the CronJob question in #1 turned out to be the important one, and the answer changes the finding. Addressed in cb9f6b0.

1. Reconciliation is app-path scoped, not interpreter-startup scoped

You asked directly: does the reconciliation run on interpreter startup, or only under the uvicorn/saas_server app path? It's the app path only, so the 15-minute CronJob does not trigger it.

The chain: _reconcile_org_condenser_defaults() is called from SaasAppLifespanService.__aenter__, and that service only ever becomes a FastAPI lifespan — openhands/app_server/app.py:50-52 appends app_lifespan_.lifespan to the FastAPI(lifespan=...) list. Nothing else calls it. The three CronJob entrypoints never reach that module:

Entrypoint	imports saas_server / app_lifespan?
python -m run_budget_maintenance	no
python -m run_maintenance_tasks	no
python -m sync.clean_app_conversation_start_tasks	no

I checked by AST-walking the imports of all three, plus a plain grep for saas_server, app_lifespan, and get_app_lifespan_service — zero hits. They import storage.database and their processors directly. So budget-maintenance carries the env vars but never reads them; nothing runs 96 times a day.

Your instinct that the count is higher than "once per rollout" was right, though, just in a different place. All three Deployments run saas_server:app under --workers 3, 2, and 2, and the lifespan runs per worker process — so a fresh install executes reconciliation 7 times, not once. The backend serializes them with pg_advisory_xact_lock and they converge on the same result, so this isn't a correctness problem, but "every app start" was doing too much work in the README. It now says explicitly that the trigger is the app-server startup lifespan, that it runs once per saas_server worker process, that the maintenance CronJobs never trigger it, and that the lock serializes overlapping runs.

On the remedy — scoping the vars out of openhands.env.defaults — I'd rather not, and I think the evidence above is why: the copies in the CronJobs are inert rather than dangerous. Splitting them out means giving up the .Values.env de-duplication and override behavior you called out as the right thing at the top of the review, and it would be a bespoke mechanism for three env vars. It's also not new: DUPLICATE_EMAIL_CHECK and OH_RESOLVER_LABEL already land in budget-maintenance unread today. I've left a comment at the render site recording why the CronJob copies are harmless, so the next reader doesn't have to redo this trace.

2. One-shot migration as a persistent value — agreed, but not fixable in this chart

You're right about the lifetime mismatch, and a helm.sh/hook Job is the correct shape. The blocker is that the chart can't relocate the logic: reconciliation lives inside the app's startup lifespan in enterprise#337, and there's no standalone entrypoint for it. A hook Job would need something like python -m reconcile_org_condenser_defaults on the backend first — at which point the chart change is easy and I'd support it. Better raised against the backend PR than worked around here, since a chart-side approximation would be a second way to do the same thing.

Meanwhile the README no longer frames removal as a rollout-completion step; it states plainly that this is not a rollout-scoped operation and that the flag re-applies on every start until removed.

3. Version reference is now actionable

Fair — "the app version that reads these" with no version named isn't something an operator can act on. I can't name a released version because enterprise#337 is still open, so the README now links it directly and says the current appVersion has no reader. That gives operators something concrete to track.

On the guard messages asserting unimplemented behavior: the validations.yaml message mirrors the contract in enterprise#337 — _reconcile_org_condenser_defaults returns early on if config.max_tokens is None or not config.apply_to_existing, so "overwrite mode only runs while reconciling existing organizations" describes that early return rather than being chart invention. Agreed the two can drift while the backend is unmerged; the README link is the mitigation.

Nits

• maxTokens naming — declining, and it's a real tradeoff rather than laziness. The key maps 1:1 onto the OPENHANDS_ORG_DEFAULTS_CONDENSER_MAX_TOKENS env var and the condenser.max_tokens JSON field in the DB, so an operator debugging can grep one name across values file, pod spec, and database row; round 1 asked for the shape to stay aligned with the backend env vars for the same reason. Renaming to condensationTriggerTokens breaks that chain at the chart boundary only. Your underlying point — that the explanation doesn't travel into consumer values files alongside the key — was the right criticism, so values.yaml now leads with "Conversation-history size that triggers condensation, NOT an LLM output-token cap" and notes the effective-input-limit interaction, instead of deferring to the README.
• Boolean/null asymmetry — agreed it's fine, and I pinned it: a new test asserts null flags render "false" rather than "". It's load-bearing; dropping | default false from _env.yaml fails it.

helm unittest charts/openhands → 153 passed, helm lint clean, and both --set orgDefaults=null and --set orgDefaults.condenser=null still render.

This reply was generated by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

Code review — feat: PLTF-3553 Add Helm values for org condenser defaults (round 4)

Reviewed against .agents/skills/code-review.md. I cloned cb9f6b0, added the missing chart repos, ran helm dependency build, helm lint (clean), and helm unittest charts/openhands → 153 passed. CI is green on this commit.

The round-3 fixes hold up under independent checking, and I want to be specific about what I actually verified rather than restating the PR description:

• The nil guards work. (.Values.orgDefaults | default dict).condenser | default dict renders cleanly for both --set orgDefaults=null and --set orgDefaults.condenser=null. I also checked the case the tests don't cover — explicit applyToExisting: null / overwriteExisting: null in a real values file rather than via --set — and it correctly renders "false".
• The tests are load-bearing, not decorative. I mutation-tested three ways: dropping | default false from _env.yaml fails 1 test, forcing the ne $orgCondenser.maxTokens nil conditional to true fails 3, and deleting the overwriteExisting-without-applyToExisting guard fails 18. Nothing passes vacuously.
• The env vars go through openhands.env.defaults, so they inherit the de-duplication wrapper and .Values.env override behaviour for free. Established pattern followed, not reinvented.
• Defaults ship inert (maxTokens: null renders no env at all), which is the right posture for a self-hosted user.

Holding at COMMENT for one new finding I think is important, plus a correction to the review record from the last round.

1. A schema-valid maxTokens can crashloop all three Deployments (important)

_env.yaml:45 renders the value with | quote and no integer coercion. YAML parses 1e6 and 200000.0 as floats, Go renders those in exponent form, and values.schema.json's "type": ["integer", "null"] does not reject them — a float with a zero fractional part satisfies JSON Schema's integer type.

Verified against this branch:

$ cat /tmp/v1.yaml
orgDefaults:
  condenser:
    maxTokens: 1e6

$ helm template t charts/openhands -f /tmp/v1.yaml | grep -A1 MAX_TOKENS
- name: OPENHANDS_ORG_DEFAULTS_CONDENSER_MAX_TOKENS
  value: "1e+06"

helm lint passes, schema validation passes, all 153 tests pass. Note this only reproduces through a values file — --set orgDefaults.condenser.maxTokens=1e6 is correctly rejected by the schema, which is precisely why the existing coverage misses it. 200000.0 renders "200000" and is harmless; 1e6 is not.

The backend failure mode is worse than a wrong number. In enterprise#337, _parse_positive_int_env gates on raw.isdecimal(), and '1e+06'.isdecimal() is False, so it raises ValueError. That parse happens in get_org_defaults_condenser_config(), called from _reconcile_org_condenser_defaults at line 89 — outside the try that wraps _reconcile_org_condenser_defaults_once. The exception propagates out of SaasAppLifespanService.__aenter__, the FastAPI lifespan fails, and the uvicorn worker dies at startup.

Two consequences worth calling out:

• It fires regardless of applyToExisting, because max_tokens is parsed before the if config.max_tokens is None or not config.apply_to_existing early return. An operator who sets only the threshold, with both rollout flags off, still gets it.
• It hits openhands, openhands-integrations, and openhands-mcp at once, so the symptom is a full crashloop of the app tier traced back to a values file that every validation layer in this repo accepted.

One-token fix, verified to render "1000000" for both 1e6 and 200000.0:

value: {{ $orgCondenser.maxTokens | int64 | quote }}

For the record, dailyConversationLimit has identical behaviour (1e6 → "1e+06"), so the pattern is pre-existing rather than introduced here — but the blast radius differs sharply: a bad daily limit is a wrong number, this is a failed process start. The guard belongs in the chart rather than the backend, since the chart is where the operator's 1e6 becomes a string.

2. Correction: the CronJobs do import these modules — but your conclusion still holds

Your round-3 reply reported an AST import walk of all three CronJob entrypoints with "zero hits" for saas_server / app_lifespan. That isn't right, and I'd rather flag it than leave it in the record as a verified trace. Walking the transitive import closure of each entrypoint in enterprise#337:

Entrypoint	files in closure	reaches saas_app_lifespan_service / org_defaults_config?
run_budget_maintenance	201	yes — both, plus storage.org_default_settings, org_store, user_store
run_maintenance_tasks	196	yes — same set
sync.clean_app_conversation_start_tasks	195	yes — same set

A direct-import grep returns zero, which is presumably what got measured. The closure does not.

Your conclusion survives, for a better reason than "they don't import it": importing doesn't execute. _reconcile_org_condenser_defaults has exactly one non-test caller (__aenter__), and SaasAppLifespanService only ever becomes a lifespan via get_app_lifespan_service() → app.py:52. The CronJobs run python -m entrypoints that never construct the FastAPI app, so reconciliation cannot fire there regardless of what they import. I'm satisfied budget-maintenance does not run this 96 times a day, and I withdraw my round-3 finding #1.

Worth getting on the record correctly, though, because the two arguments have different durability: the call-graph argument holds as long as the only caller is the lifespan, while the import-graph argument would break the moment someone adds an import. The inline comment at _env.yaml:38-42 already gives the right reason ("never start the app") — good comment to have left.

3. Minor notes (non-blocking)

• .Values.env bypasses both guards. --set env.OPENHANDS_ORG_DEFAULTS_CONDENSER_OVERWRITE_EXISTING=true renders the destructive flag with no applyToExisting and no maxTokens, because the dedup wrapper appends overrides after validations.yaml has already run. Inherent to the override mechanism and true of every guarded value in this chart, so I don't think it needs fixing here — noting it so nobody mistakes the guards for a hard invariant.
• Guards are scoped to .Values.enabled. enabled=false with overwriteExisting: true and no maxTokens renders silently. That matches the convention documented at the top of validations.yaml and is harmless since no workload renders. Correct as-is.
• maxTokens naming — I accept your reasoning and withdraw the nit. One greppable name across values file, pod spec, and the condenser.max_tokens DB field is worth more than a self-describing chart key, and values.yaml now leads with what the value actually does, which was the real substance of the objection.

On the still-open items

Round-3 findings #2 (one-shot migration expressed as a persistent declarative value) and #3 (no merged backend reader) both remain, and I think your responses are the right calls. A helm.sh/hook Job genuinely can't be built until the backend exposes a standalone entrypoint, and that belongs on enterprise#337 rather than being approximated chart-side. The README now links the backend PR and states plainly that the current appVersion has no reader. For a draft that intentionally lands ahead of its backend, that's an acceptable place to be.

Risk assessment

Medium. A default install is unaffected — maxTokens: null ships disabled and renders no env, verified. Risk sits entirely on the opt-in path, in two layers. The documented layer: overwriteExisting: true is an irreversible DB mutation that re-applies on every worker start — 7 times on a fresh default install given --workers 3/2/2 — which the README now states accurately and the advisory lock serializes. The undocumented layer is #1, where a schema-valid values file takes down the app tier with an error that points at nothing. The first is well handled; the second is a one-token fix and the only thing I'd want changed before this leaves draft.

---

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands.

[aivong-openhands]

Thanks — the round-4 findings are addressed in f12c06c.

1. Schema-valid maxTokens crashlooping the Deployments — fixed

Reproduced exactly as described, and applied your one-token fix:

value: {{ $orgCondenser.maxTokens | int64 | quote }}

Before, on cb9f6b0, with maxTokens: 1e6 in a values file: value: "1e+06". After: value: "1000000". 200000.0 renders "200000" both ways.

Your analysis of the failure mode is what made this worth fixing rather than documenting. The two consequences you called out are the sharp end of it: the parse sits ahead of the apply_to_existing early return, so an operator who sets only the threshold with both rollout flags off still gets it, and it lands on all three Deployments at once. A values file that helm lint, the schema, and 153 tests all accept, producing a full app-tier crashloop with an error pointing at nothing — that is a bad trade for one missing pipe.

I added one test for it (renders an exponent-notation threshold as a plain integer) rather than two. I first wrote a 200000.0 case as well, then dropped it: with | int64 removed it still passes, because Go already renders that one as 200000. It would have been a test that cannot fail for the reason it appears to test. Mutation-checked the one I kept — dropping | int64 fails it. Suite is now 154 passed, lint clean.

Worth noting for anyone reading the test file later: this only reproduces through a values file, not --set, which is why every set:-based test in the suite was blind to it. Your reproduction identified that distinction, and it is the reason the new case uses nested set: mapping syntax with a YAML float literal instead of the dotted-path style used by its neighbours.

I left dailyConversationLimit alone, per your framing — it has identical behaviour, but a wrong number is not a failed process start, and changing it here would widen a focused PR. Happy to file it separately.

2. Correction on the import walk — you are right, and thank you

My round-3 reply reported an AST walk showing "zero hits" for saas_server / app_lifespan across the three CronJob entrypoints. That measured direct imports only. Your transitive closure (201/196/195 files, all three reaching saas_app_lifespan_service and org_defaults_config) is the correct measurement, and I should not have described a direct-import grep as an import walk. I have corrected the PR description so the record does not carry a claim that does not hold.

The conclusion does survive, and I agree it survives for the better reason: importing does not execute. _reconcile_org_condenser_defaults has one non-test caller, __aenter__, reachable only through get_app_lifespan_service() → app.py:52, and the CronJobs never construct the FastAPI app. Your point about durability is the one I am taking forward — the call-graph argument holds as long as the only caller is the lifespan, while the import-graph argument would have broken on the next stray import, so it was never load-bearing even when I thought it was true. The _env.yaml comment already states the call-graph reason; I have left it as-is.

3. Minor notes

All three accepted as-is, no changes:

• .Values.env bypasses the guards — agreed, and agreed it should not be "fixed" here. It is inherent to a dedup wrapper that appends overrides after validations.yaml has run, and it is true of every guarded value in the chart. Good to have it stated so the guards are not mistaken for a hard invariant.
• Guards scoped to .Values.enabled — matches the convention documented at the top of validations.yaml; nothing renders, so nothing to guard.
• maxTokens naming — thanks for reconsidering.

Still-open items

Nothing further from me on round-3 #2 and #3. The one-shot-migration-as-a-persistent-value tension is real and I do not think the chart can resolve it before the backend exposes a standalone entrypoint; approximating a helm.sh/hook Job chart-side would mean inventing a trigger the backend does not have. Both remain reasons this stays a draft rather than things I can close here.

---

This reply was generated by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

🟢 Taste Rating: Good taste — the chart change follows the established openhands.env.defaults / de-duplication pattern, keeps the default install inert, and covers the real edge cases that came up in prior review rounds.

I re-ran the repo-specific .agents/skills/code-review.md guidance against the current head (037b7d5). I could not run Helm locally because this sandbox does not have helm installed, but GitHub checks are green on the current head, including helm-unittest, lint, test-scripts, lint-and-test (openhands, charts/openhands), and chart publish checks.

The round-4 crashloop finding is addressed: maxTokens is coerced with | int64 | quote, and the new unit coverage pins exponent notation rendering to "1000000". The env names match the referenced backend parser in enterprise#337, null orgDefaults / condenser blocks remain guarded, consumers can still override through .Values.env, and the README now accurately documents the app-server startup trigger plus the destructive/repeated nature of overwriteExisting.

No blocking issues found.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

Default installs are unaffected because orgDefaults.condenser.maxTokens: null renders no org-condenser env vars. The remaining risk is entirely opt-in: enabling applyToExisting / overwriteExisting mutates persisted org settings and can re-apply on app-worker startup. The chart now makes that explicit, defaults the destructive flags off, validates contradictory rollout combinations, and covers the important render-time failure modes.

VERDICT:
✅ Worth merging: Core chart logic is sound, the previous important issue is fixed, and CI is green.

KEY INSIGHT:
The implementation keeps the hazardous database mutation behind explicit opt-in values while preserving the chart’s override-friendly env pattern.

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands. Submitted as a COMMENT rather than APPROVE because the available GitHub token authenticates as the PR author.

[aivong-openhands]

Code review — feat: PLTF-3553 Add Helm values for org condenser defaults (round 5)

Reviewed against .agents/skills/code-review.md. I cloned f12c06c, added the five missing chart repos, ran helm dependency build, helm lint (clean), and helm unittest charts/openhands → 154 passed. All 23 CI checks are green on this commit (1 skipped).

Verdict: approve. The round-4 finding is fixed, I re-verified that fix and the earlier rounds' fixes independently, and nothing left is above nitpick level. Posted as a comment rather than a formal approval only because GitHub rejects self-approval from the PR author's token; per the repo guidelines this is an APPROVE, and it needs a second pair of eyes to register as one.

Verified, not restated

I re-derived each claim rather than trusting the description:

• The int64 fix works, and the test covering it is load-bearing. maxTokens: 1e6 in a values file renders "1000000" on this commit (it rendered "1e+06" on cb9f6b0). 1.5e6 → "1500000", 200000.0 → "200000". Mutation-checked by dropping | int64 from _env.yaml:50 — the new exponent-notation test fails, the other six in the suite still pass. Your call to drop the second 200000.0 case was right: it passes with or without the pipe, so it would have been a test that cannot fail for the reason it appears to test.
• Both render guards are load-bearing. Forcing validations.yaml:19 to if false fails 1 test; forcing :22 to if false fails 1. Neither passes vacuously.
• The nil guards hold on paths the tests do not cover. --set orgDefaults=null and --set orgDefaults.condenser=null both render clean, and so does a real values file with explicit applyToExisting: null / overwriteExisting: null — renders "false", which --set-based tests cannot reach.
• Schema rejects what it should. 0 and -1 rejected on minimum; 200000.5 and 0.5 rejected as non-integer; "200000" rejected as a string. 2000000 accepted, so the round-2 removal of the invented ceiling stands.
• The CronJob question from rounds 3/4 is settled. With maxTokens set, the env lands on 6 workloads — Deployment/openhands (--workers 3), openhands-integrations (2), openhands-mcp (2), and CronJobs t-app-conversation-start-task-clean (0 3 * * *), t-budget-maintenance (*/15 * * * *), t-maintenance-tasks (0 6 * * *). I dumped each workload's command: the three Deployments run uvicorn saas_server:app, the three CronJobs run python -m entrypoints that never construct the FastAPI app. Since _reconcile_org_condenser_defaults has exactly one non-test caller (SaasAppLifespanService.__aenter__), the CronJob copies are inert — 7 executions on a fresh install, none of them every 15 minutes. The inline comment at _env.yaml:38-42 records the call-graph reason rather than the import-graph one, which is the durable argument.
• Default install is unaffected. helm template with stock values renders zero CONDENSER env vars.
• Established pattern followed. The vars go through openhands.env.defaults, so .Values.env override works — I confirmed env.OPENHANDS_ORG_DEFAULTS_CONDENSER_MAX_TOKENS: "999" wins over a configured maxTokens: 200000.

Minor notes (non-blocking, no action needed here)

1. int64 has the same failure mode at the top of its range. It overflows silently rather than erroring — with maxTokens: 1e30 in a values file:

$ helm template t charts/openhands -f /tmp/v.yaml | grep -A1 MAX_TOKENS
- name: OPENHANDS_ORG_DEFAULTS_CONDENSER_MAX_TOKENS
  value: "-9223372036854775808"

9223372036854775807 and 99999999999999999999 do the same; 9007199254740993 renders 9007199254740992 (float64 precision). '-9223372036854775808'.isdecimal() is False, so _parse_positive_int_env raises and you get exactly the round-4 crashloop.

I am not asking for a change. The round-4 input was 1e6, a plausible way to write a million tokens; nothing in this range is a plausible token count, and the obvious guard — a schema maximum — is the chart-invented ceiling you correctly removed in round 2 on my own advice. Noting it so the int64 fix is not mistaken for a total one.

2. Misspelled keys are silently ignored. orgDefaults.condensor.maxTokens and orgDefaults.condenser.maxToken both render nothing and exit 0 — values.schema.json does not set additionalProperties: false on the new objects. That is chart-wide convention rather than something this PR introduced, so tightening it here would be inconsistent.

3. README rollback wording is slightly off from the guard. Line 39 offers "Remove applyToExisting/overwriteExisting ... or remove orgDefaults.condenser.maxTokens." Taking only the second branch fails the render:

Error: ... orgDefaults.condenser.maxTokens must be set before applyToExisting or overwriteExisting can be enabled.

The guard message says exactly what to do next, so an operator is never stuck — but the two branches are not independent, and "remove the flags, then the threshold" would be more accurate.

4. Prior rounds' accepted items still stand as accepted. .Values.env bypasses both guards (inherent to the dedup wrapper, true of every guarded value in this chart); guards scoped to .Values.enabled (matches the convention documented atop validations.yaml); enterprise#337 still open with appVersion: 1.59.1 carrying no reader (the README states this plainly and links the PR, and this is a draft landing deliberately ahead of its backend).

Against the repo guidelines

• Consumer-friendly configuration — the three keys name their effect, not our environment. No saasMode-style scenario key.
• Booleans truthful in every combination — the overwriteExisting-without-applyToExisting combination is rejected at render time rather than allowed to lie.
• One knob, one effect — maxTokens sets the threshold, the two flags scope reconciliation. No knob smuggles a second effect.
• Defaults serve the self-hosted user — null/false, inert, and the PR does not justify anything by pointing at our dev/staging/prod.
• Override-friendly — every value overridable via .Values.env without forking.
• Data outlives the pod — N/A, no storage backend selection here.

Risk assessment

Low-to-medium. Default installs render nothing, verified. Risk is confined to the opt-in path: overwriteExisting: true is an irreversible DB mutation re-applied on every worker start (7 times on a fresh default install), which the README now states accurately and the backend serializes with pg_advisory_xact_lock. The undocumented crashloop path that held this at COMMENT last round is closed for every realistic input. What remains is documented, guarded, or implausible.

---

This review was generated by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

Addressed the round-5 README rollback wording nit in 1c87257.

The docs no longer present removing orgDefaults.condenser.maxTokens as an independent alternative while rollout flags remain enabled. They now say to remove applyToExisting/overwriteExisting first, then remove maxTokens as well if the operator wants to stop future defaulting and reconciliation.

Docs-only change; no chart behavior changed.

This comment was created by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

🟢 Taste Rating: Good taste — the latest head keeps the chart behavior simple, override-friendly, and guarded against the real rollout hazards identified in prior rounds.

Reviewed PR #1214 against the repository-specific .agents/skills/code-review.md guidance at head 1c8725761bece9879cfc7c03ed3fa187fad17eee.

The only change since the prior approval-equivalent review is docs-only: charts/openhands/README.md now correctly tells operators to remove applyToExisting / overwriteExisting after the rollout before optionally removing orgDefaults.condenser.maxTokens. That fixes the misleading alternative wording without changing rendered chart behavior.

I re-checked the important chart mechanics on the current head:

• OPENHANDS_ORG_DEFAULTS_CONDENSER_* env vars are emitted only when orgDefaults.condenser.maxTokens is set.
• The new defaults flow through the existing openhands.env de-duplication wrapper, so consumers can override them via .Values.env without duplicate env vars.
• Explicit orgDefaults: null and orgDefaults.condenser: null remain guarded by the default dict pattern.
• The rollout guard rejects overwriteExisting: true unless applyToExisting: true, and rejects rollout flags without a threshold.
• maxTokens is coerced through int64 | quote, so schema-valid exponent/float integer forms render as decimal strings the backend parser can accept.
• Current GitHub checks are green on this head, including helm-unittest, lint, test-scripts, chart lint/test, publish-chart checks, and image scans. I could not run Helm locally in this sandbox because helm is not installed.
• There are no unresolved review threads.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

This is a small Helm values/env/docs/test change with inert defaults, explicit opt-in for persisted-data reconciliation, schema and render-time guards for unsafe combinations, and passing CI. The destructive overwriteExisting path is documented and gated behind explicit operator configuration.

VERDICT:
✅ Worth merging: Core logic is sound, the docs now describe the operational lifecycle accurately, and no important issues remain.

KEY INSIGHT:
The implementation preserves the existing chart contract: default installs do nothing, while opt-in operators get explicit values, override compatibility, and guardrails around repeated reconciliation.

This review was created by an AI agent (OpenHands) on behalf of the user.

[aivong-openhands]

live validation OpenHands/enterprise#337 (comment)

[openhands-release-bot]

🚀 Released in openhands/0.68.0.