[freeqaz]

Problem

profile-renderer and profile-public-github-read each carry a byte-identical helmTemplate(output, extraValueFiles, releaseName, extraArgs) (22 lines). The shared waitFor fails at once on any error, so loops that treat errors as "not yet" keep their own deadline and last-error code (test DRY survey clusters 10 and 8).

Change

• tests/helpers/production-chart.mjs exports helmTemplate (same arguments and execFileSync options, plus the module's GOGC=off tool environment). Both profile files import it; their call sites are unchanged. profile-public-github-read also imports chartTooling statically instead of through a dynamic import.
• waitFor(description, read, { retryErrors: true }) treats every error from read as "not yet" and appends the most recent one to the timeout message (and keeps it as cause). Without the option nothing changes.
• tests/helpers/runtime-image-fixture.test.mjs: the process-group settle loop now uses it (3 s, 25 ms, as before). A timeout now reads Timed out waiting for the detached process group to settle. Last error: ... instead of the raw last error.
• Not migrated: profile-preflight-chart-parity's helmTemplate is a different function (async, returns { ok, output } for refused renders). The remaining retry-on-error copies need a model key, a runtime image or a cluster, or are held for another PR: docker-compute-real, docker-compute-token-retry, kubernetes-client, logging-otel-observation.

Verification

All runs compared against a main worktree at 2bc826a.

• Mutants: 3 planted (public GitHub reader automountServiceAccountToken: true, a constant installation checksum annotation, and assertProcessGroupSettled never seeing ESRCH). The same test cases fail on main and on the branch for each.
• Pins: profile-renderer, profile-public-github-read and runtime-image-fixture.test 20/20 each at 2 CPUs, on the branch and on main. Median walls 5.40/0.95/0.65 s (branch) and 5.45/0.95/0.65 s (main).
• Prettier and ESLint clean on the touched files.
• Open PRs touching these files (fix(profiles): refuse composed Backend ID collisions #2731, Support custom Installation profiles and readable Preset starters #2253, Draft implementation: managed outbound HTTPS for dedicated Gateways #2518, Credential Proxy Implementation #2487): no new merge conflicts.
• Internal review: approve with nits. Applied: the timeout keeps the retried error as cause, and the doc says the reported error may come from an earlier read. MB3 re-run on the new head: same kill, and runtime-image-fixture.test 20/20 again.

Risks

Test-only. The shared helmTemplate now runs Helm with GOGC=off, as every other render in production-chart.mjs already does; the output does not change.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge.

What this changes

This PR shares the profile tests’ Helm renderer and adds optional error retries to the common polling helper.

Example: A detached process group remains active after the fixture’s 3-second settling deadline.

• Before: The test throws the last process-group error.
• After: The test reports the settling timeout, includes the last error, and preserves that error as its cause.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	A focused, apparently correct test refactor supported by reported parity checks and successful relevant CI; evidence remains at the test-harness level.
Proof confidence	🦐 gold shrimp (3/6)	This collaborator-authored test-only PR is outside the ordinary contributor runtime-proof gate; reported suite and mutation comparisons support the changed helpers, while the inspected Console screenshots concern unrelated profile work and provide no evidence for this diff.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Contributors maintain duplicate Helm invocation helpers and a separate error-retrying polling loop.
Reason: The narrow consolidation reduces duplication without weakening assertions; collaborator authorship establishes owner direction.

Merge readiness

✅ Ready for maintainer review

Keep this PR open for maintainer triage: it provides focused test-helper consolidation, and no actionable defect was found in the introduced changes.

Priority: P3
Reviewed head: bf653817987fb83ec8fcec135e4e86b283c6a23a

Before merge

None.

Findings

None.

Agent review details

How this fits together

These test helpers turn rendered profile values into Helm manifests and poll fixture observations until success or a deadline; they do not change deployed platform behavior.

flowchart TD
  A[Profile tests] --> B[Shared Helm helper]
  B --> C[Helm chart rendering]
  C --> D[Manifest assertions]
  E[Process group fixture test] --> F[Shared waiter]
  F --> G[Process group observation]

Loading

Technical review

Best possible solution:

Use the existing chart-helper and waiter owners while retaining profile assertions, default error propagation, and bounded process-group observation.

Do we have a high-confidence way to reproduce the issue?

This is a test-maintenance refactor rather than a product bug report; source confirms the duplicated renderers and retry loop, and no target code was run during review.

Is this the best way to solve the issue?

The extraction uses existing helper owners, preserves Helm argument precedence and refusal behavior, and keeps error retries explicit.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against dd8706ff9ed3.

Provenance checked

• Shared waiter keeps the original intent (0904aa0: test(helpers): share the free-port and waitFor helpers #1079 centralized equivalent polling helpers; test(helpers): share one bounded waiter and Postgres lock-wait probe across tests #3165 explicitly deferred retry-on-error callers pending this option.)
• Profile Helm rendering keeps the original intent (ee48017: Installation profiles introduced real Helm rendering checks; subsequent history added release-name and upgrade arguments, which this extraction preserves.)
• Go tool environment keeps the original intent (59eb244: perf(test): share equal chart renders and run chart Go tools without GC #3108 established GOGC=off for short-lived chart tools using measured startup savings; this PR explicitly extends that existing choice to the extracted renderer.)
• Process-group settling keeps the original intent (08e9b00: The fixture verifies surviving descendants before accepting settlement; the migrated test preserves that observation, deadline, and interval.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

• Introduced scope: The pinned introduction diff changes only five test files, removes both duplicate synchronous Helm helpers, and preserves their callers and assertions. (tests/helpers/production-chart.mjs:105, bf653817987f)
• Retry behavior: Retries require explicit opt-in; ordinary read errors still propagate immediately, and the migrated process-group test retains its 3000 ms timeout and 25 ms interval. (tests/helpers/wait-for.mjs:17, bf653817987f)
• Current main: The five reviewed files match the merge-base versions on fetched main, so main does not already contain this consolidation. (dd8706ff9ed3)
• Verification and discussion: The author reports repeated base/head suite runs and matching mutation kills. Discussion contains only review-status bot comments. GitHub reports CI Required, Static Checks, Checks and Conformance 1, and Runtime Image Fixture successful at the reviewed head; First Agent Smoke remains in progress. (bf653817987f)
• Policy and clean checkout: The complete root AGENTS.md was read; no nested tests instructions, maintainer-notes directory, or pr-simplify skill were found. Applicable test-integrity rules were applied, with the user’s sole-reviewer and read-only instructions taking precedence over helper execution. The checkout remained clean. (AGENTS.md, bf653817987f)

Likely related people:

• freeqaz: Raw commit 0904aa0 adds tests/helpers/wait-for.mjs:1 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 0904aa0c3267; files: tests/helpers/wait-for.mjs)

Review metrics

Metric	Value	Why it matters
Change scope	Production +0/-0; tests and helpers +69/-65	The small net increase centralizes two duplicate renderers and one retry loop without adding production machinery.

Labels

Label changes:

• add P3: This is test-maintenance work with no demonstrated user-facing runtime failure.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

• P3: This is test-maintenance work with no demonstrated user-facing runtime failure.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 22, 2026, 7:09 AM ET / 11:09 UTC.