Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .agents/skills/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,17 @@ Use patterns that make it easy for chart consumers to override defaults. Environ
- Dependencies should be conditional where possible
- Version constraints should be explicit

### Integration Credential Enforcement (KOTS layer, not Helm `required`)

Required credentials/identities for integrations (Jira DC, Bitbucket DC, Azure DevOps, etc.) — service-account emails/PATs, bot tokens, bot usernames — are enforced in `replicated/config.yaml` with `required: true` + `when: "<integration>_enabled"`. That is the install path every customer uses. By established convention the chart itself does **not** use Helm's `required` to fail-fast these: secrets are wired as `secretKeyRef … optional: true` and non-secret values default to empty. This is consistent across all integrations (e.g. Jira DC's `service-account-email` / `service-account-pat`). A chart that enables a new integration this way — empty/optional in `_env.yaml`, `required` in `replicated/config.yaml` — is following the pattern, not introducing a gap. Tightening the bare-`helm install` path to fail-fast is a worthwhile but deliberate, chart-wide change tracked in its own PR; it is out of scope for an integration-wiring PR.

## What NOT to Comment On

- Minor style preferences that don't affect functionality
- Praise for code that follows best practices (just approve)
- Obvious or self-explanatory configuration
- Third-party env var naming (these follow external conventions)
- Integration credentials rendered empty/`optional` in `_env.yaml` when `replicated/config.yaml` already marks them `required` (KOTS-layer enforcement is the convention — see Integration Credential Enforcement)

## Communication Style

Expand Down
2 changes: 1 addition & 1 deletion charts/openhands/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v2
description: OpenHands is an AI-driven autonomous software engineer
name: openhands
appVersion: cloud-1.38.0
version: 0.7.61
version: 0.7.62
maintainers:
- name: rbren
- name: xingyao
Expand Down
2 changes: 2 additions & 0 deletions charts/openhands/templates/_env.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@
{{- if .Values.bitbucketDataCenter.enabled }}
- name: BITBUCKET_DATA_CENTER_HOST
value: {{ .Values.bitbucketDataCenter.host }}
- name: BITBUCKET_DATA_CENTER_BOT_USERNAME
value: {{ .Values.bitbucketDataCenter.botUsername | quote }}

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.

🟠 Important: This adds the env var, but direct Helm installs render it as an empty string when bitbucketDataCenter.enabled=true and bitbucketDataCenter.botUsername is unset. In the same chart path, BITBUCKET_DATA_CENTER_BOT_TOKEN is still sourced from an optional secret key, so the chart still permits exactly the bot-less BBDC configuration this PR is trying to forbid. Please fail fast here (for example, required "bitbucketDataCenter.botUsername is required when bitbucketDataCenter.enabled=true" .Values.bitbucketDataCenter.botUsername and make the bot-token secret key non-optional or add an explicit required value/schema path), and update charts/openhands/README.md's BBDC secret/values example to include both bot credentials.

- name: BITBUCKET_DATA_CENTER_CLIENT_ID
valueFrom:
secretKeyRef:
Expand Down
1 change: 1 addition & 0 deletions charts/openhands/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ secretsChecksum: ""
bitbucketDataCenter:
enabled: false
host: ""
botUsername: ""

azureDevOps:
enabled: false
Expand Down
20 changes: 15 additions & 5 deletions replicated/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -553,14 +553,24 @@ spec:
when: 'repl{{ ConfigOptionEquals "bitbucket_data_center_auth_enabled" "1" }}'
required: true
- name: bitbucket_data_center_bot_token
title: Bitbucket Data Center Bot Token (strongly recommended)
title: Bitbucket Data Center Bot Token (required)
help_text: >-
HTTP access token for a dedicated bot user with repo access. When
set, OpenHands posts comments and reactions as the bot instead of
as the mentioning user or installer; jobs still run on the invoking
user's workspace.
HTTP access token (Repository write) for a dedicated bot user with
access to the target repos. OpenHands posts comments and reactions
as this bot -- not as the mentioning user -- which is required so the
bot's own replies don't re-trigger jobs. The agent still runs on the
invoking user's workspace. Do not sign in to OpenHands as this user.
type: password
when: 'repl{{ ConfigOptionEquals "bitbucket_data_center_auth_enabled" "1" }}'
required: true
- name: bitbucket_data_center_bot_username
title: Bitbucket Data Center Bot Username
help_text: >-
Username (slug) of the bot account above, e.g. openhands. Used to
ignore the bot's own comments so its replies don't re-trigger jobs.
type: text
when: 'repl{{ ConfigOptionEquals "bitbucket_data_center_auth_enabled" "1" }}'
required: true

- name: azure_devops_authentication
title: Azure DevOps Authentication
Expand Down
1 change: 1 addition & 0 deletions replicated/openhands.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ spec:
bitbucketDataCenter:
enabled: repl{{ ConfigOptionEquals "bitbucket_data_center_auth_enabled" "1" }}
host: 'repl{{ ConfigOption "bitbucket_data_center_domain" }}'
botUsername: 'repl{{ ConfigOption "bitbucket_data_center_bot_username" }}'
azureDevOps:
enabled: repl{{ ConfigOptionEquals "azure_devops_auth_enabled" "1" }}
tenantId: 'repl{{ ConfigOption "azure_devops_tenant_id" }}'
Expand Down
Loading