env_artifact attest verdict + implement skill spec

attest + qa.test_attestation:
  When master_run AND fix_run BOTH fail with overlapping compile-class
  anchors (undefined reference / cannot find -l / fatal error / linker
  errors / build-script failure), raise env_artifact instead of
  test_fails_on_fix. attest_cycle's exception handler routes env_artifact
  to sink (state=OPEN_ENV_ARTIFACT) — terminal on our side, alive
  upstream. Substrate-env-incompat is real (Catch2 in slang-server's
  build, docker-root chmod skew in conserve#300 per memory
  feedback_docker_root_invalidates_chmod_tests) and shouldn't false-fail
  PRs whose upstream CI is green.

  _looks_like_env_artifact: structural overlap of compile-class
  anchors between master and fix slices, with hex/line/path
  normalization to ignore instance-level noise.

skills/implement.md (new):
  Engagement-lane implementation skill spec. Takes a prior /investigate
  artifact that named the fix (file:line + change shape) + maintainer
  feedback, IMPLEMENTS the named change, verifies, pushes amend.
  Contract is "finish the work" — distinct from /investigate (which
  produces the plan but may stop at "pending implementation").
  Halt conditions are sparing: named location no longer exists,
  contradictory feedback, test_attestation actually fails after the
  patch (real implementation failure), push rejected.

  Actor wiring (sweep/activities/implement.py + lifecycle registration)
  not yet built — skill spec captures the structural shape. Today's
  EnzymeAD/Enzyme#2819 H1 patch was applied manually by operator
  using exactly this shape (read prior artifact + maintainer's inline
  comment → moved aggregate fallback from TypeAnalysis to
  AdjointGenerator.h:1894 → committed amend → pushed); the next
  card of this shape should run through the implement-actor.

Author: kimjune01

repo-hypotheses/VictoriaMetrics__helm-charts__2882.md

@@ -1,95 +1,76 @@
-# VictoriaMetrics/helm-charts#2882 — Operator admissionWebhooks Aren't Working As Expected
+# VictoriaMetrics/helm-charts#2882 — admissionWebhooks accept malformed manifests
 
-**Report:** user installs `victoria-metrics-operator` chart v0.62.1. A `VMAgent` manifest containing a non-existent field (`test: true`) is accepted by the cluster. With the upstream `install-with-webhook.yaml` from the operator repo, the same manifest is rejected.
+## H₀ (observation)
+Helm chart's admission webhooks accept malformed VMAgent manifests (e.g. `spec.test: true`); upstream `install-with-webhook.yaml` from the operator release rejects the same input. Both ship the same operator binary (v0.70.0).
 
-## Diagnosis
-
-The bad manifest is silently pruned/accepted because the helm chart, by default, installs **specless CRDs** — every CRD's `spec.spec` schema is just `{ type: object, x-kubernetes-preserve-unknown-fields: true }`. The kube-apiserver's strict field validation (which is what actually rejects `test: true` in the upstream manifest) is bypassed: with `preserveUnknownFields: true` every key under `spec.*` is allowed.
-
-The webhook **is** running and reachable; the webhook config that helm renders is wired correctly (path, port, CA, service all match what the operator expects). The webhook just doesn't catch this kind of error: the VMAgent validator (`internal/webhook/operator/v1beta1/vmagent_webhook.go`) only checks `obj.Status.ParsingSpecError` (empty at CREATE time) and `obj.Validate()` (typed struct validation — unknown fields were already dropped by the JSON decode). It has no way to see `test: true`.
-
-The upstream `install-with-webhook.yaml` works because it ships the **full** openAPIV3Schema (~3000 lines per CRD). The apiserver's structural schema check rejects unknown fields under `spec` before the webhook ever runs.
-
-## Why the chart is shaped this way
+## H₁ — webhook routing broken in chart (KILLED, deduction)
+**Perturbation**: `helm template charts/victoria-metrics-operator/`; diff rendered Service/Deployment/ValidatingWebhookConfiguration against upstream `install-with-webhook.yaml`.
 
-- Commit `0f6a51d19f` / PR [#2419](https://github.com/VictoriaMetrics/helm-charts/pull/2419) made the templated CRDs specless on purpose, to fix #2420: the full schema pushed the helm release Secret over 1 MiB (`Too long: may not be more than 1048576 bytes`) and broke upgrades.
-- The full strict CRDs are available via the `crds` subchart, gated on `crds.plain: true`. That path uses a kubectl-apply Job (init container `bzcat` of `crd.yaml.bz2`) to install CRDs server-side instead of through helm-managed manifests, dodging the 1 MiB limit.
+- Webhook path `/validate-operator-victoriametrics-com-v1beta1-vmagent` matches upstream.
+- Service port 9443 → targetPort `webhook` (containerPort 9443). Plumbing is fine.
+- TLS cert SANs cover `<svc>.<ns>.svc` and `.svc.<cluster.dnsDomain>`.
+- `failurePolicy: Fail`, so an unreachable webhook would *reject*, not accept. The manifest being accepted proves the webhook is reachable and returns "allow".
 
-## Workaround for users
+Trajectory: divergent against. Webhook plumbing is correct.
 
-Set:
+## H₂ — operator's validator doesn't see unknown fields (CONFIRMED, deduction)
+**Perturbation**: read `internal/webhook/operator/v1beta1/vmagent_webhook.go@v0.70.0`.
 
-```yaml
-crds:
-  plain: true
-admissionWebhooks:
-  enabled: true
+```go
+func (*VMAgentCustomValidator) ValidateCreate(_ context.Context, obj *vmv1beta1.VMAgent) {
+    if obj.Status.ParsingSpecError != "" { return ..., errors.New(...) }
+    if err := obj.Validate(); err != nil { return ..., err }
+    return nil, nil
+}
 ```
 
-With `crds.plain: true`, the operator chart short-circuits its specless `templates/crd.yaml` (see `{{- if and (not .Values.crds.plain) .Values.crds.enabled }}` guard) and lets the `crds` subchart deliver the full schema via an upgrade Job. Apiserver-side strict validation then rejects unknown fields like `test: true` without any webhook involvement.
+Validator runs on a parsed Go struct. Unknown JSON fields (`test: true`) are dropped by `json.Unmarshal` before the validator sees them. The webhook **cannot** catch unknown-field errors on the protocol level — it operates after apiserver→struct decode.
 
-Note: the subchart's `upgrade.enabled` is also `false` by default; users likely need `crds.upgrade.enabled: true` as well for the Job to actually run.
+## H₃ — chart ships specless CRDs; upstream ships full schema (CONFIRMED, induction)
+**Perturbation**: compare CRDs.
 
-## Hypothesis graph
+| Source | VMAgent CRD `spec` schema |
+|---|---|
+| `charts/victoria-metrics-operator/crd.yaml` (default, `crds.plain: false`) | `{"required":["remoteWrite"],"type":"object","x-kubernetes-preserve-unknown-fields":true}` — **specless** |
+| `charts/victoria-metrics-operator/charts/crds/crds/crd.yaml` (when `crds.plain: true`) | Full `properties:` schema, no preserve-unknown-fields. 45,807-line file. |
+| Upstream `install-with-webhook.yaml` | Full `properties:` schema. |
 
-### H₀ — webhook config malformed (path/port/CA wrong)
+With `x-kubernetes-preserve-unknown-fields: true` and no `properties`, the **apiserver itself** accepts any field under `spec`. There is no schema-level rejection.
 
-- **Perturbation:** render the chart, compare each webhook entry to upstream `install-with-webhook.yaml`.
-- **Result:** path scheme matches (`/validate-operator-victoriametrics-com-<version>-<singular>`). Port is `9443` (explicit) vs upstream omits (defaults 443) — but the helm Service exposes port 9443 → targetPort `webhook` (=9443 in deployment). CA is self-generated and inlined; cert dir matches operator default `/tmp/k8s-webhook-server/serving-certs`. ObjectSelector excludes `app.kubernetes.io/name = victoria-metrics-operator`; a user-applied VMAgent without that label still matches `NotIn` (label-selector semantics: missing key satisfies NotIn).
-- **Trajectory:** convergent — the webhook IS reachable.
-- **Status:** killed. Wiring isn't the issue.
-- **Mode:** deduction (read the rendered manifest, compared to upstream).
+Upstream rejects `test: true` at the **apiserver layer**, before the webhook runs, via CRD-pruning. The webhook is incidental for this failure mode.
 
-### H₁ — operator webhook validator only checks known fields
+**Provenance**: helm-charts PR #2419 ("operator: make templated crds specless"), merged 2025-09-27. Closed #2420 and #2334 (rendering perf issues). Deliberate tradeoff: apiserver-side schema validation for chart-render speed.
 
-- **Perturbation:** read `internal/webhook/operator/v1beta1/vmagent_webhook.go`.
-- **Result:** `ValidateCreate` returns based on `obj.Status.ParsingSpecError` (not set at create) and `obj.Validate()` (typed struct validation). At decode time, unknown JSON fields not in the Go struct are silently dropped — webhook can't see `test: true`.
-- **Trajectory:** divergent (against "webhook can catch this"). Webhook is structurally incapable of catching unknown fields.
-- **Status:** confirmed.
-- **Mode:** deduction.
-
-### H₂ — specless CRD allows unknown fields through
-
-- **Perturbation:** extract VMAgent CRD from helm `crd.yaml` vs `install-with-webhook.yaml`. Compare schema for `.spec.versions[].schema.openAPIV3Schema.properties.spec`.
-- **Result:**
-  - Helm chart (templated path): 107 lines for VMAgent CRD; `spec: { type: object, x-kubernetes-preserve-unknown-fields: true }`.
-  - Upstream install-with-webhook: 3096 lines for VMAgent CRD; full property list, no preserveUnknownFields.
-- **Trajectory:** divergent (for the diagnosis). With `preserveUnknownFields: true`, apiserver structural-schema rejection doesn't fire on unknown keys under `spec`.
-- **Status:** confirmed root cause.
-- **Mode:** induction (compared the actual rendered CRDs).
-
-### H₃ — chart provides an opt-in to the strict CRDs
-
-- **Perturbation:** inspect `templates/crd.yaml` and `Chart.yaml`. Trace the `crds.plain` flag.
-- **Result:** `crds.plain: true` disables the specless templated CRDs AND enables the `crds` subchart (gated by `condition: crds.plain`), which ships `crds/crd.yaml` containing the full 3000-line schema per CRD and applies via a kubectl Job.
-- **Trajectory:** convergent — the chart already has the right escape hatch, it's just not the default.
-- **Status:** confirmed.
-- **Mode:** deduction.
+## Diagnosis
+The chart's default-mode (`crds.plain: false`) ships specless CRDs that disable apiserver-side schema validation. The admission webhook validates a Go struct, which silently drops unknown JSON keys. So malformed manifests slip through.
 
-### H₄ — default flip would regress #2420 (>1 MiB helm release Secret)
+`install-with-webhook.yaml` includes full-schema CRDs; apiserver rejects unknown fields before the webhook runs.
 
-- **Perturbation:** read PR #2419 and issue #2420.
-- **Result:** specless was a deliberate fix for the helm release Secret hitting 1 MiB. Flipping the default back to plain CRDs in `templates/crd.yaml` would resurrect that bug.
-- **Trajectory:** divergent against "just change the default". The subchart path (kubectl Job applying CRDs out of helm's Secret) is the only safe way to ship the full schema, and it has its own UX cost (extra Job, extra ServiceAccount).
-- **Status:** confirmed — there's no clean default flip.
-- **Mode:** deduction (read the prior PR + issue).
+Setting `crds.plain: true` (or pre-applying upstream CRDs) restores rejection. Workaround already available; no chart bug in the routing sense.
 
-## Graph state
+## Candidate fixes
 
-| Node | Status | Shape | Mode |
-|------|--------|-------|------|
-| H₀ webhook wiring wrong | killed | convergent | deduction |
-| H₁ webhook can't see unknown fields | confirmed | divergent | deduction |
-| H₂ specless CRD preserves unknown fields | confirmed (root cause) | divergent | induction |
-| H₃ `crds.plain: true` opt-in exists | confirmed | convergent | deduction |
-| H₄ default flip would regress #2420 | confirmed | divergent | deduction |
+1. **Doc fix (low risk)**: clarify in `values.yaml` and README that `crds.plain: false` ships specless CRDs and that admission webhooks alone do not reject unknown fields; recommend `crds.plain: true` (or applying the upstream CRD bundle) for strict validation.
+2. **Default flip to `crds.plain: true`**: re-introduces the rendering perf hit PR #2419 explicitly chose to avoid. Needs maintainer signoff.
+3. **Specless-but-strict**: drop `x-kubernetes-preserve-unknown-fields: true` from the specless CRD. Apiserver would then reject **all** spec fields (no properties defined). Breaks valid manifests. Not viable.
 
-## Reframe
+## Recommendation
+Halt at diagnosis. The fix is a tradeoff only the maintainer can pick.
 
-Original framing: "admissionWebhooks aren't working". Actual finding: webhooks **are** working as designed; the cluster's structural schema validation isn't, because the templated CRDs are deliberately specless to fit under helm's 1 MiB Secret cap. The webhook layer was never the right place to catch unknown-field errors — that's the CRD schema's job.
+- Option (1) is a one-paragraph doc clarification.
+- Option (2) reverses the explicit decision in PR #2419 and needs maintainer judgment.
+- Option (3) is structurally impossible.
 
-## Verdict
+Substrate shouldn't ship "a fix" that papers over a deliberate design decision. Route to operator for a yes/no on PRing option (1).
 
-No fix to ship. The behavior is a documented trade-off: chart maintainers picked "small helm releases" over "strict schema by default". Users who want strict validation should set `crds.plain: true` and `crds.upgrade.enabled: true`. A README/values.yaml note clarifying that field-validation strictness depends on `crds.plain` would help future users hit this less, but that's a docs-only nit and not a behavioral bug.
+## Reasoning mode table
+| Claim | Mode | Confidence |
+|---|---|---|
+| Webhook routing is correct | deduction (rendered manifests) | 98% |
+| Webhook can't see unknown JSON fields | deduction (Go source) | 99% |
+| Chart's specless CRDs accept unknown fields; upstream's full CRDs don't | deduction (schema comparison) | 99% |
+| PR #2419 was a deliberate perf tradeoff | deduction (PR description) | 95% |
 
-→ Route to tissue (report findings, no PR).
+## Frontier (open)
+- Live verify in `kind` that `crds.plain: true` restores rejection (untested; pure inference).
+- Confirm v0.62.1 (user's version) ships the same specless CRDs as v0.63.0 — almost certainly yes; PR #2419 merged well before v0.62.1.

skills/implement.md

@@ -0,0 +1,100 @@
+---
+name: implement
+description: Take a prior /investigate artifact that named the fix (file:line + change shape) plus maintainer feedback, IMPLEMENT the named change in the existing PR's branch, verify, and push an amend. Distinct from /investigate (which produces the plan but stops at "pending implementation"). Implement's job is to FINISH.
+argument-hint: <repo#pr>
+allowed-tools: Read, Write, Edit, Grep, Glob, Bash, Agent
+---
+
+# Implement: take the plan to a pushed amend
+
+This skill is called by `implement-actor` in the engagement lane,
+AFTER /investigate has already produced an artifact that names the
+fix (file:line + change shape) and AFTER a maintainer has either
+raised a concern naming a different location, requested changes, or
+left an inline comment with explicit direction.
+
+The contract is **finish the work**: read the maintainer feedback +
+the prior /investigate artifact, IMPLEMENT the named fix, verify
+locally, push an amend commit. Do NOT re-run a hypothesis graph —
+/investigate already did that. If the artifact doesn't have a clear
+named fix, halt and let the actor escalate back to /investigate.
+
+## Phase order
+
+1. **Read the prior artifact** — `repo-hypotheses/<owner>__<repo>__<issue>.md`.
+   It almost certainly names the file, line range, and shape of the
+   fix the maintainer wants. If the artifact is missing or doesn't
+   contain a clear directive, escalate to /investigate (you can't
+   "finish" what hasn't been planned).
+
+2. **Read the maintainer feedback** — inline comments + top-level
+   comments + review states. The freshest non-author message is
+   authoritative direction. Quote it verbatim in your reasoning.
+
+3. **Re-confirm location** — open the named file in the worktree,
+   re-read the relevant lines. The artifact's file:line should still
+   match the current code; if drift has occurred (rebase against
+   master moved the line), re-locate via `grep`/`Glob`.
+
+4. **Implement** — Edit the file. Apply the named change. Keep diff
+   minimal: only what the maintainer asked for. Don't refactor
+   surrounding code, don't add tests beyond what's needed to verify
+   the fix.
+
+5. **Verify locally** — run the repo's test command. If a test was
+   added to the original PR, it should still pass. If new behavior
+   needs a test, add one focused on the maintainer-named scenario.
+   The fix MUST fail-on-master / pass-on-fix (test_attestation gate).
+
+6. **Amend + force-with-lease push** — amend the existing top commit
+   (don't add a new "Address feedback" commit on top — maintainers
+   prefer rebased history for substantive reworks). Push to the fork
+   branch. Use `--force-with-lease` not `--force`; protects against
+   parallel pushes.
+
+7. **Emit decision** — write the per-issue attestation at
+   `~/.sweep/attestations/implement/<owner>__<repo>__<issue>.md`
+   with `decision: pushed-amend` and the new HEAD SHA.
+
+## Halt conditions (use sparingly)
+
+You may halt and route to operator only when:
+
+- The artifact's named location no longer exists in the current code
+  AND grep cannot find a plausible replacement
+- The maintainer feedback contradicts itself or is genuinely ambiguous
+  (rare — most "I don't know what they want" is laziness; re-read)
+- The test_attestation gate fails after the patch — meaning your
+  implementation doesn't actually fix the named bug (real failure;
+  surface for operator review)
+- The push is rejected for reasons beyond `--force-with-lease`
+  (auth, branch protection)
+
+Do NOT halt for:
+
+- "I'm not sure if this is what the maintainer meant" — they wrote
+  the comment; take it at face value and implement
+- "There might be a better approach" — there might be; the maintainer
+  chose theirs. Ship what they asked for.
+- "The test infrastructure feels brittle" — fix what's named, leave
+  the rest
+
+## Output contract
+
+After all narration, the **last printed line** must be a single JSON
+object matching this schema:
+
+```json
+{
+  "decision":      "pushed-amend | halt | escalate-to-investigate",
+  "head_sha":      "<10-char SHA if pushed-amend, else empty>",
+  "reason":        "short string, ≤200 chars",
+  "rejected":      false,
+  "reject_reason": null
+}
+```
+
+`decision: pushed-amend` is the success path. The downstream wrapper
+kicks reqa to verify the new SHA against the maintainer's named
+behavior; on green, respond posts a brief "addressed feedback at
+SHA <head_sha>" to the PR.

sweep/activities/attest.py

@@ -535,6 +535,29 @@ async def attest_cycle(msg: Message) -> dict:
             return {"verdict": "skip", "target": "sink",
                     "reason": "test_passes_on_master",
                     "msg_id": msg.msg_id}
+        # `env_artifact` is "substrate's local env failed both branches
+        # with the same error class" — the PR is fine upstream (CI green
+        # is the ground truth), our container is the problem. Sink as
+        # OPEN_ENV_ARTIFACT: terminal on our side, alive upstream. The
+        # week-stale-CI sweep (future) evicts the repo if maintainer
+        # never moves. Distinct from test_fails_on_fix where master
+        # actually failed the test (the real bug) and fix should've
+        # passed but didn't.
+        if "env_artifact" in reason:
+            from sweep.activities.pr_state import _sink_pr
+            if msg.pr:
+                _sink_pr(msg.repo, int(msg.pr),
+                         reason=f"env_artifact: {reason[:200]}",
+                         state="OPEN_ENV_ARTIFACT")
+            observe.event(
+                "attest_routed", repo=msg.repo, branch=msg.branch,
+                verdict="skip", target="sink",
+                reason="env_artifact",
+                msg_id=msg.msg_id,
+            )
+            return {"verdict": "skip", "target": "sink",
+                    "reason": "env_artifact",
+                    "msg_id": msg.msg_id}
         # "test_fails_on_fix" is a genuine, routable verdict, not
         # broken substrate. Everything else (toolchain missing, master
         # passing, git checkout failure) propagates to halt the actor.