Repository navigation
feat(lifecycle): add the one-shot ownership bootstrap workflow - #1249
Conversation
Before the observe soak starts, the store has to hold a row for every entity the old mechanism already knows about. Otherwise every comparison reads store-permits-old-forbids -- the one DANGEROUS class -- and the measurement is swamped by its own migration. Operator-submitted, never scheduled. There is no CronWorkflow for this and there should not be: it is a migration step, and a migration that runs on a timer is a reconciler nobody designed. dry_run defaults to "true", and the shell compares against the literal "false" rather than matching "true": a typo must not write to the ownership store, and matching the positive would make a misspelled "flase" apply. The parameter reaches the script as an env var, never interpolated into the body. The clone scaffolding is the shepherd CWFT's, including the pinned GitHub host keys, minus everything SDK- and budget-related: no OAuth token, no per-tick cost cap, no implementer subprocess and so no TMPDIR clone directory. The checkout is read-only -- .status.yaml is an INPUT here -- so there is no push step and no write mutex, and activeDeadlineSeconds is 15 minutes rather than the shepherd's two hours. MCTL_TOKEN is NOT optional here, unlike on the shepherd: the batch read and the acquire route are both admin-gated, and a bootstrap that cannot authenticate reads UNKNOWN for every entity and aborts -- safe, but a wasted run with a confusing report. The report is JSON on stdout and the shell does not parse it. A grep here would become a second, drifting copy of the module's own decision vocabulary. Refs: mctlhq/.github#57 Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
Claude finished @mashkovd's task in 3m 15s —— View job Review complete
Verdict: Changes requested (1 P1, 1 P2). The template's own PR description makes a strong case for the parts that matter most here — the
Both are inline on line 54 of
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| secondsAfterFailure: 86400 | ||
| secondsAfterCompletion: 3600 | ||
| entrypoint: bootstrap | ||
| serviceAccountName: argo-workflow-sa |
There was a problem hiding this comment.
P1: missing imagePullSecrets: [{name: ghcr-credentials}] at the spec level. The main container pulls ghcr.io/mctlhq/mctl-agents (line 68), which is a private GHCR image — every other consumer of this image in this repo (cwft-mctl-agents-shepherd.yaml, -implement, -investigate, -reconcile, -run, -approve) sets this same imagePullSecrets entry at spec: for exactly that reason. Without it, the main container pod will hit ImagePullBackOff and this migration step will never actually execute — a silent no-op for what's meant to be a carefully-reviewed, one-shot ownership import.
| secondsAfterFailure: 86400 | ||
| secondsAfterCompletion: 3600 | ||
| entrypoint: bootstrap | ||
| serviceAccountName: argo-workflow-sa |
There was a problem hiding this comment.
P2: missing podMetadata.labels (app.kubernetes.io/name: mctl-agents, mctl.ai/team: admins) that every sibling agent CWFT sets at this same spot (see cwft-mctl-agents-shepherd.yaml). Per that template's own comment, these labels are what let mctl_get_service_logs find agent pods in Loki during an incident and let NetworkPolicy select agent pods apart from other workflow pods sharing the argo-workflows namespace. Since this template is explicitly a copy of the shepherd's scaffolding, this looks like an unintentional drop rather than a deliberate choice — worth adding for log discoverability on a template whose whole point is to be reviewed row-by-row after the fact.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 1 P2. The new ClusterWorkflowTemplate is missing spec.imagePullSecrets: [{name: ghcr-credentials}], which every other template pulling ghcr.io/mctlhq/mctl-agents in this repo sets — without it the main container will ImagePullBackOff and the workflow never runs (P1). It also drops the podMetadata.labels block the shepherd template it's copied from uses for Loki log discoverability / pod selection (P2).
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved runtime, image-pull, output-format, and template-resolution issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a manually triggered, one-shot Argo workflow to bootstrap lifecycle ownership records before the observe soak.
Changes:
- Adds safe-by-default dry-run and explicit apply modes.
- Clones GitOps state and invokes the lifecycle bootstrap module.
- Configures credentials, SSH verification, resources, and cleanup TTLs.
File summaries
| File | Description | Final findings |
|---|---|---|
platform-gitops/argo-workflows/cluster-templates/wft-lifecycle-bootstrap.yaml |
Defines the lifecycle ownership bootstrap workflow. | Critical (3 votes): Default image lacks the bootstrap module. Critical (1 vote): Missing ghcr-credentials may cause ImagePullBackOff.Moderate (1 vote): Status messages invalidate JSON-only stdout. Critical (1 vote): Literal template braces may fail Argo template resolution. |
Review details
Suppressed comments (1)
platform-gitops/argo-workflows/cluster-templates/wft-lifecycle-bootstrap.yaml:146
- These status messages go to stdout immediately before the module's JSON output, so the workflow log's stdout is not a valid JSON report despite the contract documented above and the comment at lines 148-150. Send the human-readable notices to stderr so stdout remains exclusively the JSON report.
echo "▶ APPLY mode: planned rows WILL be written"
else
echo "▶ dry run: nothing will be written"
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| entrypoint: bootstrap | ||
| serviceAccountName: argo-workflow-sa |
| # --apply to write anything, so an operator who submits this template | ||
| # with no parameters at all gets a report and no writes. | ||
| - name: agent_image | ||
| value: ghcr.io/mctlhq/mctl-agents:1.45.0 |
| - | | ||
| set -eu | ||
| # The parameter reaches the shell as an ENV VAR, never interpolated | ||
| # into the script body: {{workflow.parameters.*}} inside a script is |
…Loki ghcr.io/mctlhq/mctl-agents is private. Without imagePullSecrets the main container ImagePullBackOffs and the workflow never runs -- every other template pulling that image carries the same block, and this one dropped it along with the SDK scaffolding it does not need. podMetadata comes back for the same reason it exists on the shepherd: Promtail maps those labels onto Loki's `app` and `team`, and an Argo pod without them cannot be found through mctl_get_service_logs. This workflow's entire output is a JSON report on stdout, so a run whose logs nobody can read produced nothing. Refs: mctlhq/.github#57 Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a56233b519
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| secondsAfterFailure: 86400 | ||
| secondsAfterCompletion: 3600 | ||
| entrypoint: bootstrap | ||
| serviceAccountName: argo-workflow-sa |
There was a problem hiding this comment.
When this workflow is submitted, the bootstrap pod cannot pull the private ghcr.io/mctlhq/mctl-agents image because the template does not set imagePullSecrets, and argo-workflow-sa has no pull secret attached. Every existing mctl-agents ClusterWorkflowTemplate explicitly references ghcr-credentials; add the same reference here so both dry-run and apply executions can start.
Useful? React with 👍 / 👎.
| - name: agent_image | ||
| value: ghcr.io/mctlhq/mctl-agents:1.45.0 |
There was a problem hiding this comment.
The default 1.45.0 image does not contain orchestrator.lifecycle.bootstrap (as the commit description itself notes), so the documented submission command, which only passes dry_run, exits with a module-not-found error even after the image-pull credentials are fixed. Pin the first release containing the module before publishing this workflow, especially because the existing mctl-agents release bump glob only covers cwft-mctl-agents-*.yaml and therefore will not update this wft-* file automatically.
Useful? React with 👍 / 👎.
|
Claude finished @mashkovd's task in 4m 48s —— View job Review complete
Verdict: Changes requested (1 P1, 1 P2, 1 P3). The fix commit ( Reviewing the rest of the (unchanged) file turned up two new issues:
|
| - | | ||
| set -eu | ||
| # The parameter reaches the shell as an ENV VAR, never interpolated | ||
| # into the script body: {{workflow.parameters.*}} inside a script is |
There was a problem hiding this comment.
P1: this comment contains the literal tag {{workflow.parameters.*}}, inside the same container.args script block where {{workflow.parameters.dry_run}} (line 168) and {{workflow.parameters.agent_image}} (line 137) are genuinely substituted. Argo's parameter substitution pass runs over the whole args string and looks up every {{workflow.parameters.X}} tag it finds in the defined-parameters map; * is not a parameter name, so this tag can't resolve, and Argo's normal (non-lint) substitution mode errors on unresolved tags rather than leaving them in place — this is the same class of error commonly seen as failed to resolve {{workflow.parameters.X}}.
The sibling cwft-mctl-agents-shepherd.yaml makes exactly this same point in an equivalent comment but writes it as plain backticked text (workflow.parameters.*, no braces) — deliberately avoiding the literal {{ }} syntax for this reason.
argo lint --offline passing here isn't strong evidence against this: lint's variable-resolution check runs in a permissive/allow-unresolved mode to tolerate runtime-only variables, which differs from what the controller does at actual submit/execution time — and per the PR description this template has only been linted, never submitted. If this tag errors at submission, the whole bootstrap container script fails to resolve and the workflow can't run at all.
| APPLY="" | ||
| if [ "${WORKFLOW_DRY_RUN}" = "false" ]; then | ||
| APPLY="--apply" | ||
| echo "▶ APPLY mode: planned rows WILL be written" |
There was a problem hiding this comment.
P2: the echo "▶ APPLY mode..." / echo "▶ dry run..." lines write to stdout before exec python -m orchestrator.lifecycle.bootstrap runs, ahead of its JSON output. That contradicts the contract this same script documents two lines below (155-163 area): "It is JSON on stdout for an operator and for the workflow log." An operator or tooling reading the workflow log as JSON (e.g. argo logs <wf> | jq .) gets a leading non-JSON line and has to know to strip it first — easy to miss on the one workflow whose entire point is a row-by-row report review before a write-mode run. Redirect these two status lines to stderr (echo "..." >&2) so stdout stays exclusively the JSON report, matching the documented contract.
| # --apply to write anything, so an operator who submits this template | ||
| # with no parameters at all gets a report and no writes. | ||
| - name: agent_image | ||
| value: ghcr.io/mctlhq/mctl-agents:1.45.0 |
There was a problem hiding this comment.
P3: agent_image defaults to 1.45.0, which the PR description itself says does not contain orchestrator/lifecycle/bootstrap.py — the template is only runnable today via an explicit -p agent_image=... override. That's a reasonable stopgap for a template that's "inert until submitted," but nothing in the template enforces it, so an operator who forgets the override gets a ModuleNotFoundError deep in a workflow log rather than a clear signal. Worth a follow-up to bump the default once the release that bundles E1 exists, so the safe path (submit with no overrides) is also the one that actually runs.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 1 P2, 1 P3. The two prior findings (missing imagePullSecrets, missing podMetadata.labels) are correctly fixed in this commit — both now match the shepherd CWFT exactly. New findings on the unchanged parts of the file: (P1) the comment at line 146 contains the literal tag {{workflow.parameters.*}} inside the same container.args script block where real {{workflow.parameters.X}} substitutions happen; * isn't a defined parameter, and unlike argo lint's permissive resolution mode, the controller's substitution pass at actual submit time is expected to error on unresolved tags — the sibling shepherd template makes the identical point in a comment but deliberately avoids the literal braces. This template has only been linted, never submitted, so the risk is unverified either way. (P2) the two echo status lines at 157/159 write to stdout ahead of the script's own documented JSON-only report contract. (P3, non-blocking) the default agent_image is acknowledged in the PR description as unable to run this module.
The comment warning about parameter tags contained a literal one. The controller's substitution pass runs over the whole args block, comments included, and a tag it cannot resolve fails the submission -- so the comment written to warn about tags would have been the thing that broke the template. `argo lint` resolves permissively and says nothing about it, which is why this survived a clean lint. The shepherd CWFT makes the same point without the braces. The two progress lines move to stderr. stdout carries the JSON report, and the fewer things share it the better -- _discover_refs already prints a skip notice there for every service the shepherd does not own. Refs: mctlhq/.github#57 Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 1m 12s —— View job Review complete
Verdict: Approved (0 P1/P2, 1 P3 pre-existing/non-blocking). Commit
No further changes requested. |
… run it Three reviewers made the same point and they are right: agent_image defaulted to today's pin, which does not contain orchestrator/lifecycle/bootstrap.py. The documented submission -- the one with no parameters -- would have failed inside the container on a missing module, which is a confusing way to learn the image is wrong. There is no value that could go here and work, because the module is not in any released image yet. So the parameter carries no default and Argo refuses the submission by name instead: "parameter agent_image was not supplied". A template that cannot run with its own defaults should say so at submit time, not at runtime. Give it a default once a release carries the module. Refs: mctlhq/.github#57 Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
The There is no value that could go there and work: The |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
No P1/P2 findings (0 P3). This commit only removes the agent_image default (so a submission with no override now fails fast by name instead of hitting ModuleNotFoundError inside the container) and adds explanatory comments — a config-only change equivalent to an image-tag adjustment. All prior P1/P2 findings (imagePullSecrets, podMetadata.labels, unresolvable {{workflow.parameters.*}} tag, stdout JSON contract) remain fixed and unaffected. Good to merge.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P2)
Finding 2 (P3)
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit FindingsFinding 1
|
Part of mctlhq/.github#57 — step E2. Pairs with the bootstrap module in mctl-agents
(E1, separate PR).
Why
Before the observe soak starts, the ownership store has to hold a row for every entity
the old mechanism already knows about. Otherwise every comparison reads
store-permits-old-forbids— the one dangerous class — and the measurement isswamped by its own migration.
Not a CronWorkflow
Operator-submitted, and there is deliberately no schedule. This is a migration step; a
migration that runs on a timer is a reconciler nobody designed.
Two runs, in order:
dry_run=trueprints a JSON report and writes nothing; review everyrow against the store and live Temporal, then
dry_run=false.The safe default is the default
dry_rundefaults to"true", and the shell compares against the literal"false"rather than matching
"true". A typo must not write to the ownership store — matchingthe positive would make a misspelled
flaseapply. The parameter reaches the script asan env var and is never interpolated into the body.
What was dropped from the shepherd's scaffolding
The clone initContainer is the shepherd CWFT's, pinned GitHub host keys included. Removed:
the OAuth token, the per-tick cost cap, the implementer subprocess and therefore the
TMPDIRclone directory. The checkout is read-only here —.status.yamlis an input —so there is no push step and no write mutex, and
activeDeadlineSecondsis 15 minutesrather than two hours.
MCTL_TOKENis notoptionalhere, unlike on the shepherd: the batch read and theacquire route are both admin-gated, and a bootstrap that cannot authenticate reads UNKNOWN
for every entity and aborts — safe, but a wasted run with a confusing report.
MCTL_API_BASE_URLis deliberately not set, matching the shepherd: the default livesin
orchestrator/temporal/mctl_client.py. Pinning it in one template of several is howthe two drift, and the resulting failure is quiet — a bootstrap pointed at another
environment's API imports owners derived from that cluster's liveness.
Before submitting it
The
agent_imagedefault is1.45.0, which does not containorchestrator/lifecycle/bootstrap.py. Bump it to the release that does before the firstrun — or pass
-p agent_image=.... Merging this template changes nothing on its own: itis inert until somebody submits it.
Verification
Then, once the image carries the module: a
dry_run=truesubmission whose report anoperator reads row by row against
mctl_get_lifecycle_ownershipand live Temporal.🤖 Generated with Claude Code
https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC