PLTF-2890: Update openhands charts script to cover image-loader chart and replicated config sandbox tag

[aivong-openhands]

Description

Releasing 1.37.1 (#679) required two manual edits the update script didn't cover. This PR teaches scripts/update_openhands_charts/update_openhands_charts.py to make them:

• charts/image-loader: update the agent-server image tag in values.yaml (the DaemonSet pre-pulls this image onto nodes) and bump the chart version when it changes.
• replicated/config.yaml: update the agent-server tag in the custom_sandbox_image_tag option — both the help_text example and the default value shown to admins.

Verified end-to-end against the same release: a real run now reproduces #679's manual edits exactly (the resulting charts/image-loader/Chart.yaml diffs to the identical blob as the PR, d97cd2db). New dry-run sections:

============================================================
Updating image-loader chart...
============================================================
Updating image-loader values.yaml...
Updated image-loader image tag: 1.22.1-python -> 1.23.1-python

Updating image-loader Chart.yaml...
Updated image-loader chart version: 0.1.6 -> 0.1.7

Updating replicated/config.yaml...
Updated replicated config sandbox image tag help text: 1.21.1-python -> 1.23.1-python
Updated replicated config sandbox image tag default: 1.21.1-python -> 1.23.1-python

Fixes picked up along the way:

• YAML re-wrapping: bumping the image-loader chart re-wrapped its 103-char description onto two lines (with trailing whitespace) because the writer wraps at ~80 chars; yaml.width = 4096 keeps untouched lines untouched.
• find_repo_root(): path constants previously assumed scripts/<name>/ two levels below the repo root, which broke the repo's mutmut config (setup.cfg) — mutants run from a mutants/ subdirectory, so every real-file canary failed at baseline. The constants now walk up to the directory containing charts/ and replicated/.
• Canary safety: the real-file canary tests now run updaters on tmp copies of the real files, so nothing (including dry-run-bypassing mutants during mutation testing) can write into the working tree.

The config patterns anchor on - name: custom_sandbox_image_tag and refuse to cross into the next list item, so a sibling option's default/help_text can never be rewritten; real-file canary tests fail loudly if the option is renamed or restructured (the failure mode that made #679 manual).

Helm Chart Checklist

No Helm charts are modified by this PR (script + tests only).

checklist N/A — scripts-only change

Additional Notes

Test evidence: 252/252 tests passing (37 new).

TDD evidence:

• RED: fb6933c (failing tests for both gaps)
• GREEN: 8e88355 (updaters + workflow wiring)
• MUTATE + KILL: a3ede57 (sibling-option guard, chart-path canary), 74eede0 (mutmut sweep survivors)
• FIX (test-first): 8fef8a7 (YAML width), 22b46d4 (canaries on tmp copies)

Mutation testing (mutmut, 918 mutants): all genuine new-code survivors killed — including an and -> or marker check in find_repo_root and the image-loader workflow narration (release PRs paste this output verbatim, so the lines naming the chart/files are asserted). Remaining new-code survivors are equivalents only: mutated dry_run defaults are unreachable through mutmut's wrapper (verified by running them directly), and "charts" -> "CHARTS" path mutants only die on case-sensitive filesystems. Note: mutmut's fork-based runner reports every mutant as segfault on macOS; the sweep was driven by running mutmut's generated mutants via MUTANT_UNDER_TEST in subprocesses.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Code Review

This is a solid automation improvement that closes two classes of manual edits from the release process. The implementation is correct and the test suite is exemplary — TDD with documented RED/GREEN/MUTATE commits, real-file canary tests, and 918-mutant coverage. A few minor observations below.

Strengths

• Motivation is airtight: the PR description includes a complete dry-run transcript that reproduces PR #679's manual edits exactly.
• find_repo_root elegantly solves the mutmut compatibility problem by walking up to marker directories rather than assuming a fixed depth. The parametric test test_directory_with_single_marker_is_skipped catches the tricky edge case where an intermediate directory has one but not both markers.
• Sibling-option guard ((?!\s*- name:)) in the Replicated config patterns is the right design. The test test_never_crosses_into_sibling_option_when_fields_missing validates the hardest scenario (option present but fields absent) — exactly the failure mode that forced #679's manual edit.
• yaml.width = 4096: subtle but important — without it, every chart-version bump would reformat the 103-char description into a folded block scalar, producing noisy unrelated diffs in release PRs.
• Canary tests now use copy_of_real_file: eliminates the risk of a dry-run-bypassing mutant writing into the working tree.
• All three new file types (image-loader values, image-loader chart, replicated/config.yaml) have real-file canary tests.

Minor observations

See inline comments for two small nits. Neither is a blocker.

No blockers — implementation is correct, test coverage is thorough, and the changes are safe to ship.

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

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Follow-up Review

Both inline nits from the previous review have been addressed:

• cab194d: Added an inline comment explaining that group 2 captures only the tag value while the closing " lives outside the capture group and is restored via replacement_suffix='"'.
• 4a229a4: Expanded the patched_inner_calls fixture docstring in TestUpdateOpenhandsWorkflowReplicated to make explicit that only (mock_values, mock_chart) are returned — the replicated-config mock is patch-only in that fixture.

No new issues found. The implementation is correct, find_repo_root handles the mutmut-relocation edge case cleanly, the sibling-option guard in the Replicated config patterns is well-tested, and the canary tests now safely operate on tmp_path copies of the real files.

One minor observation (no action required): The Helm Chart Checklist in the description has unchecked boxes, but since this PR touches only scripts and tests (no charts), those checklist items don't apply. Worth clearing them or adding a note before marking ready to avoid reviewer confusion.

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

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review — Head 4a229a4

Both nits from the first review have been resolved, confirmed by the follow-up review. I reviewed the current HEAD (4a229a4) end-to-end and found no new issues.

What's in this PR (confirmed correct)

Area	Change	Assessment
find_repo_root()	Walks up to the dir containing both charts/ and replicated/; falls back to grandparent	Correct; edge-case tests cover single-marker decoys and no-marker fallback
yaml.width = 4096	Prevents YAML serialiser from re-wrapping long chart descriptions on every bump	Correct; without it every bump silently dirtied unrelated lines
update_replicated_config	Updates help_text example and default value in custom_sandbox_image_tag; uses sibling-option guard to avoid bleeding into adjacent items	Correct; the guard is well-tested, including the hardest case (option present, fields missing)
update_image_loader_values / update_image_loader_workflow	Updates agent-server image tag in image-loader values.yaml and bumps chart version when it changes	Correct; follows existing chart-update pattern
REPLICATED_CONFIG_SANDBOX_DEFAULT_PATTERN inline comment	Explains that group 2 captures only the tag and the closing " is restored via replacement_suffix	Clear and complete
patched_inner_calls docstring	Now states explicitly that only (mock_values, mock_replicated, mock_chart) are returned; the new config mock is patch-only	Accurate

One open observation (no action required before merge)

The PR description's Helm Chart Checklist contains unchecked items. Since this PR touches only scripts and tests — not any chart source — those checklist items don't apply. Clearing the checkboxes or adding a brief note (e.g. "checklist N/A — scripts-only change") before marking ready would prevent reviewers from pausing on them.

Summary

The implementation is correct, the test suite is thorough (real-file canary tests, dry-run assertions, sibling-option guard validation, find_repo_root relocation coverage), and all previous review comments are addressed. No blockers.

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

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Review — Head 4a229a4

This PR has been reviewed across three previous passes (all at the current HEAD). All prior nits have been addressed. I reviewed the full diff end-to-end and found no new issues.

Summary of changes (verified correct)

File	Change	Assessment
.gitignore	Add mutants/ and mutmut-stats.json	Correct — these are mutmut artifacts that should not be committed
conftest.py	Add sample_image_loader_chart, sample_image_loader_values, sample_replicated_config fixtures	Well-structured; mirrors real file content
test_update_openhands_charts.py	+553 lines: TestFindRepoRoot, TestUpdateReplicatedConfig, TestUpdateImageLoaderValues, workflow integration tests, real-file canary updates	Edge cases fully covered (single-marker decoy, no-marker fallback, sibling-option overflow, missing-option loud failure)
update_openhands_charts.py	find_repo_root(), yaml.width=4096, update_replicated_config(), update_image_loader_values(), update_image_loader_workflow(), wired into process_updates() and update_openhands_workflow()	Correct; follows existing patterns; YAML-width fix prevents spurious diffs

Key design decisions (verified correct)

• find_repo_root() walks up from __file__ to find the directory containing both charts/ and replicated/, making path resolution robust when mutmut executes mutants from a mutants/ subdirectory. The two-marker requirement prevents anchoring to an intermediate directory that coincidentally contains one marker.
• Sibling-option guard ((?!\s*- name:)) in REPLICATED_CONFIG_SANDBOX_HELP_TEXT_PATTERN and REPLICATED_CONFIG_SANDBOX_DEFAULT_PATTERN prevents the regex from crossing YAML list boundaries even when the target option is missing its help_text/default fields.
• yaml.width = 4096 prevents ruamel.yaml from re-wrapping the 103-char image-loader chart description at its default ~80-char limit, eliminating noisy unrelated diffs on every chart-version bump.
• Real-file canary tests operate on tmp_path copies: no dry-run-bypassing mutant can write into the working tree.

No blockers. Implementation is correct, test coverage is comprehensive, and all previous review feedback has been addressed.

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