Skip to content

fix(deploy): preserve user-managed Context connectors - #1483

Open
alfredolopez80 wants to merge 19 commits into
devfrom
fix/issue-628-context-connectors
Open

alfredolopez80 wants to merge 19 commits into
devfrom
fix/issue-628-context-connectors

Conversation

@alfredolopez80

@alfredolopez80 alfredolopez80 commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Provisioning reapplies the bootstrap Context manifest with an empty mcpServers list, removing connectors configured in Control UI and causing Context network policies to be removed and later recreated.

Change

Treat Context manifests as bootstrap configuration: create a missing Context once and preserve an existing Context without writing it. Continue applying other instance resources. If other declarative Context fields differ from the previous applied manifest, stop and require an explicit migration. When there is no last-applied annotation, preserve controller-owned metadata not declared by the bootstrap manifest.

For isolated local validation, allow the E2E environment to opt out of loading the canonical developer .env. The prerequisite check then requires an explicitly supplied local test credential. The default credential-resolution behavior is unchanged.

The Context protection covers the GFS provisioning path used by the dev deployment. Other direct instance-apply paths and recovery after cluster deletion are outside this change. The dev deployment must consume a base SHA containing this fix before it protects clerum-dev; replaying an earlier base SHA would use the old helper.

Validation

  • Provisioning shell tests cover repeated application, concurrent connector changes, first creation, unrelated metadata, declarative drift, failure paths, and both manifest output forms.
  • The connector regression test failed against the origin/dev provisioning script, and the metadata test failed before its fix.
  • Isolated credential-resolution and prerequisite tests cover the opt-in, a missing process credential, and the unchanged default. Bash syntax, ShellCheck, and the public-boundary check passed locally.
  • The two E2E cases pass static audit and TypeScript checks and are collected by Playwright. CI repeats the static audit and collection without running the journey.
  • Full T0/T1/T2 and the Control UI journey are running against the current PR commit on a dedicated local Minikube profile. Earlier attempts did not complete T0; no runtime verdict is claimed yet. CI for this commit is in progress.
  • A read-only check before the no-cluster instruction found Eventasks in the clerum-dev Context and a last-applied configuration matching the dev bootstrap manifest outside mcpServers.

Pending end-to-end acceptance

The E2E cases have not yet completed. They exercise visible connector configuration, two provisioning passes, HCC and Host deployment restarts, policy identity and specification, an MCP tool result from a Host pod, fresh-session visibility, and fixture cleanup. The negative case checks the unauthenticated agent route.

These checks do not by themselves prove uninterrupted availability during a transition, agent tool discovery or dispatch, a full cluster restart, or restoration after cluster deletion. The same journey still needs a demonstrated failing run against the original regression. The local Minikube profile has been created; no remote cluster resources were changed or deployed.

Related to #628. Deployed validation is still needed before closing the issue.

…connectors

* origin/dev:
  fix(e2e): keep every kill attempt's node log and report a watcher that ended early
  fix(hcc): reach the overrun and call-failure counters only through typed helpers
  docs(hcc): correct claims about the request timeouts and the hard limit
  docs(hcc): say what the pass hard limit does not do
  fix(e2e): continuity lane retries after NO_PASS_START and arms its watcher without a gap
  fix(mcp-host): accept plain digits only for HCC_AUTHORITY_MAX_STALENESS_MS
  fix(hcc): expose every pass-overrun and call-failure series at zero
  fix(hcc): refuse a request timeout that is not below the pass hard limit
  fix(hcc): accept plain digits or an empty value for the request timeouts and the pass hard limit
  test(hcc): pin the write-attempt signal of the Service and ConfigMap sync
  docs: describe the HCC request deadline, the pass hard limit and the mcp-host staleness floor
  test(e2e): add an HCC restart lane that proves the fleet is not disturbed
  fix(mcp-host): require an authority staleness bound that survives two missed polls
  fix(hcc): bound the NetworkPolicy pass and keep ready verdicts over transient failures
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

The regression this PR fixes reproduced on clerum-dev today during the regular dev deploy of 193f5f4 (the merge of #1339), evenfire-infra run 43601313209, step "Provision GFS runtime and dev CRD instances". Read-only observations from the deploy log, the HCC log and the live objects:

Time (UTC) Event
14:00:28 HCC reconciles context1 with 4 allowed servers: evenfire-vulnerability-plugin-mcp, recipe-evenfire-dev-brain-plugin-v0-1-5-185c2682-brain-mcp, recipe-sqlite-mcp-stack-v1-0-0-f0ea309a-sqlite-mcp, eventasks-et-mcp-cbbcdf5c
14:03:19.122 provision-gfs-runtime applies deploy/overlays/gcp-dev/instances: context.clerum.io/context1 configured (field manager kubectl-client-side-apply)
14:03:19.126 HCC reconciles context1 with allowedServers: [] and deletes its ctx-context1-* policies for all four servers, including the L2 egress policy
14:03:21 Field manager node-fetch writes spec.mcpServers again with two servers; HCC reconciles [brain-mcp, evenfire-vulnerability-plugin-mcp]

After the deploy, context1 lists only recipe-evenfire-dev-brain-plugin-v0-1-5-185c2682-brain-mcp and evenfire-vulnerability-plugin-mcp. recipe-sqlite-mcp-stack-v1-0-0-f0ea309a-sqlite-mcp and eventasks-et-mcp-cbbcdf5c still exist as McpServers but are no longer in the Context, and nothing has re-added them. Nothing was changed in the cluster.

On the HCC side the rewrite was absorbed: the NetworkPolicy pass running at that moment ended aborted-authority and the next pass certified 47 s later. So the visible damage is the lost connectors, not an outage.

This confirms the failure on the deployed cluster. It does not validate the fix: that needs a dev deploy whose base SHA contains this PR, followed by the same check that context1 keeps every connector across the provisioning step.

…e-628-context-connectors

* commit '1b1b1131ef784929f7f5551c509f0aa2f4b19de6':
  fix(workflow-recipes): never report a pod that is being deleted as ready
  test(workflow-recipes): return the PodReadiness kind from readiness mocks
  fix(workflow-recipes): fail the pass when patchStatus cannot read the live recipe
  fix(workflow-recipes): give no readiness verdict while the mcp-host pod is absent
  fix(workflow-recipes): start the mcp-host readiness window at the later of run or pod start
  docs(crds): state that backoffSeconds retry waits are capped at 300 s
  test(workflow-recipes): probe healthCheck against the real runner server, drop the abort pin
  test(workflow-recipes): cover resume with a snippet step persisted as running
  docs(crds): state the maxRetries backoff formula
  fix(workflow-recipes): release the snippet-runner health response after reading its status
  fix(workflow-recipes): wait for the snippet runner only when a remaining step needs it
  chore(workflow-recipes): forward workflow step tuning env to recipe mcp-host pods
  fix(workflow-recipes): wait for the snippet runner before dispatching snippet steps
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

Blind review (round 1): 4 lanes on Opus 5.5

Reviewed head a19ecac8a against merge-base 1b1b1131e. The four lanes were contract, provisioning/deploy scripts, security and tests. No lane read prior reviews, issue history or project notes. Every lane ran the hermetic shell tests under /bin/bash 3.2 and bash 5: context 31, gfs-runtime 32, restart 12, prereqs 3, sdk-auth 34 PASS, all EXIT=0. CI on the head: 63/63 green.

Verdict: not ready to merge. One High finding, introduced by this PR, blocks every deploy after the first one.

High

B2-01: the drift check reads a List, so every existing Context fails the check (deploy/scripts/lib/bootstrap-instances.sh:81, compare at :88-108, error at :110)

  • kubectl get -f - --ignore-not-found -o json reads its input from stdin, so kubectl does not treat it as a single item. For an object that exists it prints a v1 List wrapper, not the object. The check then compares List.apiVersion (v1) with the Context's clerum.io/v1alpha1 and List.metadata.annotations (absent). It fails on every existing Context and exits with "use an explicit Context migration".
  • Effect: the first run passes because the Context does not exist yet. Every later run stops before applying anything. That includes provision-gfs-runtime.sh on the dev deploy (set -euo pipefail, so finalize_credentials_after_overlay at :239 never runs), minikube-deploy-all, minikube-deploy-instances and full-setup re-runs. On clerum-dev, where context1 exists, the first dev deploy after the merge would fail.
  • Evidence:
    • From the kubectl source: builder.go visitByPaths sets singleItemImplied=false for stdin, and printGeneric wraps the result.
    • A stub probe: the List shape gives EXIT=1 and the bare object gives EXIT=0.
    • A read-only run against a local minikube cluster (kubectl client 1.36): piping an existing object's manifest to kubectl get -f - --ignore-not-found -o json printed kind: List with 1 item; a missing object printed nothing.
  • Why the tests miss it: the stub kubectl in scripts/tests/test-provision-context-preservation.sh returns the bare object for get -f -.
  • Direction: read by explicit name (kubectl get context <name> -n <ns> --ignore-not-found -o json), or unwrap .items[0] when the output is a List. Make the stub return what kubectl really returns, so the test fails today.

Medium

B1-01: the drift stop has no recovery path (bootstrap-instances.sh:110, provision-gfs-runtime.sh:234-239). This is separate from B2-01.

  • When a Context's declared fields really do differ from the overlay, the error names neither the Context nor the field.
  • The procedure it asks for ("an explicit Context migration") does not exist in the repo.
  • Under set -e it also skips GFS credential finalize and every other instance.
  • Prod runs the same path (evenfire-infra deploy-clerum-prod.yml). The first edit to an overlay context.yaml would block every deploy until someone runs a manual kubectl apply, and that apply is the overwrite Deploy-time Context patch makes HCC delete context1 NetworkPolicies; sqlite-mcp stays without them for 50-57 min #628 is about.
  • Corroborated by RP-A-01, RP-B-03, ZS-4 and OS.
  • Direction: name the Context and the differing fields, document the migration (or ship a small script), and decide whether one Context's drift should block unrelated instances and credential finalize.

B4-01: five of the seven new call sites are untested.

  • Reverting any of these to plain kubectl apply leaves every test green: both minikube-deploy-all-body branches in Makefile, both full-setup.sh sites and scripts/minikube/setup.sh (mutations M13-M17).
  • Only the provision-gfs-runtime.sh path is pinned.

Low

  • B1-02 / B3-02 (also OS-2): in isolated mode scripts/check-prereqs.sh:238-248 accepts only ADMIN_PASSWORD, while the loader and the runner also take E2E_ADMIN_PASSWORD. With only E2E_ADMIN_PASSWORD set, prereqs fail (EXIT=1) and the loader passes. With both set to different values, setup seeds one and prereqs check the other. The comment "Match the isolated test loader" is false.
  • B3-01: E2E_DISABLE_CANONICAL_DOTENV is honoured only in dotenv_canonical_root (scripts/e2e/load-dotenv.sh:74-78). scripts/minikube/apply-llm-secret.sh:43-62 still finds and sources the primary checkout's .env on its own. It runs from Makefile:680 through pre-gate-sync.sh:790-794, so an "isolated" run can still write the developer's real LLM keys into the cluster, with the keys on kubectl argv. A fixture reproduced it; reachability on the T2 path was established by reading the code.
  • B4-02 (also ZS-5a): dropping create --save-config (bootstrap-instances.sh:119) survives the suite. A probe shows that a field later removed from the bootstrap manifest is then accepted with no migration.
  • B1-03: on an existing profile, make minikube-setup-e2e no longer adds the demo connectors. The edited comment in deploy/overlays/minikube/instances/context.yaml:13 still says it does.
  • B1-04: minikube-deploy-instances now needs the branch lease, so the default clerum-test profile is refused. Three docs and scripts still tell you to run it without one.
  • B2-02: jq became a hard dependency of the dev deploy. It is checked only after the auth-sync mutation.
  • B4-03 to B4-05: three restart guard clauses and two runner preconditions have no rejecting case. The contexts-only, skip-instances and six restart negative cases have no liveness witness; instrumented runs pass with the feature disabled.
  • B4-06 / B4-07: the Playwright journey has never run (CI only collects it with --list). The unauthenticated case checks the rendered UI, not that the admin API refuses the request.
  • B1-05 (body text): CI is no longer "in progress" (63/63). "ShellCheck passed" is partly true: SC1091 and SC2034 are new.

Pre-existing (not introduced here)

  • B2-04: scripts/minikube/setup.sh points at deploy/minikube/instances/, which nothing generates.

Verified clean

  • The connector-regression test fails against the merge-base provisioning script, and the metadata test fails before its fix, as the body says.
  • Mutations that disable preservation, break either drift-check path or drop the metadata preservation all turn the suite red.
  • CI changes: pull_request trigger, no secrets, no interpolated input, no continue-on-error.
  • The lockfile change is only the package's own version.
  • No eval or command injection in bootstrap-instances.sh.
  • The Playwright spec: static audit passes, tsc reports 0 errors, no force:, no fixed sleeps, no storage mutation, and the child-process environment allowlist leaves out the password.

Limitations: no cluster or Playwright run. No lane read the evenfire-infra overlay pipeline beyond the deploy workflows.

@alfredolopez80

Copy link
Copy Markdown
Contributor Author

/review-pr: 2 lanes on Opus 5.5 (regression and architecture; runtime/E2E and CI)

Head a19ecac8a, base 1b1b1131e. Both lanes compared base and head on the same inputs, and every attribution below comes from that comparison. The High finding B2-01 from the blind review (the drift check reads a List and stops every deploy after the first) applies here too, and these lanes did not catch it independently. It is the merge blocker.

Architecture

  • "A Context manifest is bootstrap-only" is the right invariant. spec.mcpServers has two writers, the control-api PUT and the workflow-recipes union. The CRD requires the field and declares it as an atomic list. Leaving it out of the manifest or switching to server-side apply would therefore not be simpler.
  • The dev deploy path is the only instance writer that needed the fix. evenfire-infra's direct targets are refused, and its kustomize render contains no Context.

Medium

RP-B-01: the E2E journey cannot pass even with a correct fix (static, not run)

  • The journey runs make minikube-deploy-all (tests/e2e/playwright/issue-628/connector-persistence.spec.ts:481). That target rolls control-api, external-rest-api and rpc-proxy through apply-inter-service-tokens.sh:342-363, and it restarts HCC and chatllm.
  • The spec then calls branch-profile-health (:545) without holding the port-forwards again. The svc/ forwards stay bound to the old pods (runbook :83-86), so the health probe fails.
  • Restart mode has the same problem: scripts/e2e/issue-628-profile-restart.sh:33 runs the health probe before the forwards are restored at :65.
  • The original regression would still trip the journey at spec :460-461, but that has not been shown with a failing run.
  • Direction: run branch-profile-pf after every rollout and before every health probe, then run the journey once against the base and once against the head.

RP-A-01: the drift stop has no migration tool or doc. Same as B1-01.

RP-A-03 (pre-existing, scope gap): two more resources are overwritten on every dev and prod deploy

Low

  • RP-A-02: the verdict depends on whether the last-applied annotation exists. The same UI edit is preserved when the annotation is absent and stopped when it is present. The PR's test pins this behaviour. It is not reachable on clerum-dev today.
  • RP-A-04: seed re-runs no longer add demo connectors. docs/get-started/quickstart.md:125-133 and context.yaml:13 are stale.
  • RP-A-06: replaying an older base_sha (any SHA before this PR) repeats the incident. Whether prod has adopted this path was not verified.
  • RP-A-07: jq is a new, undocumented prerequisite.
  • RP-B-02: the spec runs the dev deploy entrypoint on a local profile without --skip-auth-sync. Its children take the remote path and skip their own identity checks. The outer lease is still held.
  • RP-B-04: behaviour changed without doc updates: the e2e reseed, and minikube-deploy-instances is now lease-gated.
  • RP-B-05: a failing E2E run is opaque. Child stderr is dropped and the polls swallow the failure reason.
  • RP-B-06: the negative guard tests have no reason-token witness.
  • RP-B-07 (info): the CI --list step does not assert the expected count (Total: 2).

Verified

  • Base vs head: the head's context test, run against the base provisioning script, fails (EXIT=1, "existing Context changed after bootstrap"), and the outer CI step propagates the failure.
  • All five new or changed hermetic tests pass on the head under bash 3.2 and bash 5.
  • CI: 63/63 green, safe for fork PRs.
  • evenfire-infra copies deploy/scripts/lib/, and its kubectl shim passes stdin through, so the dev deploy runs the new helper.

Limitations:

  • No cluster or Playwright run.
  • The context guard blocked 4 unchanged tests from running locally.
  • Lease acceptance in restart mode and CNI NetworkPolicy enforcement were not traced.

@alfredolopez80

Copy link
Copy Markdown
Contributor Author

opencode swarm lane: DeepSeek V4.1 Flash (max), verified by Opus 5.5

One opencode 2.2.2 worker (opencode-go/deepseek-v4.1-flash, variant max) reviewed head a19ecac8a against 1b1b1131e.

  • It ran for 25 min in a detached worktree. Its tools were stubbed so that kubectl, minikube, gcloud, docker, helm and gh exit 99, and it ended EXIT=0.
  • An Opus 5.5 driver then checked each finding against the head and the base.
  • The 5 touched shell tests were re-run under bash 3.2 and bash 5, with the same counts (EXIT=0).

Result: none of this lane's findings is above Low, and the worker did not catch the High blocker B2-01 from the blind review. One remark points near it: the create path assumes kubectl get --ignore-not-found -o json behaves like the test stub. That assumption is exactly where B2-01 lives, for the existing-object case.

Confirmed, Low

  • OS-2: in isolated mode the prereq check (scripts/check-prereqs.sh:238-247) accepts only ADMIN_PASSWORD. The credential resolver used by setup and the runner (scripts/e2e/admin-credentials.sh:62-64) also takes E2E_ADMIN_PASSWORD and other aliases. Reproduced: only E2E_ADMIN_PASSWORD or TEST_ADMIN_PASSWORD set gives EXIT=1; only ADMIN_PASSWORD gives EXIT=0. Same as B1-02 / B3-02.
  • OS-3: on a reused profile, SEED_PROFILE=e2e no longer adds the demo connectors to an existing Context. The behaviour change is deliberate; the text drifted. The success log at scripts/minikube/full-setup.sh:1973 and four help/doc entries still promise the connectors: Makefile:231, docs/deploy/minikube.md:80, docs/get-started/quickstart.md:132-133 and docs/testing/e2e-guide.md:74.

Over-defensive, no change needed

  • OS-1: minikube-deploy-instances now requires the T2 mutation lease. That matches the already-wrapped minikube-deploy-all (Makefile:421-426). It is worth one line in the PR body.

Informational, pre-existing

  • scripts/minikube/setup.sh targets deploy/minikube/, which exists at neither the head nor the base.

@alfredolopez80

Copy link
Copy Markdown
Contributor Author

zai swarm lane: GLM-5.3 (max), verified by Opus 5.5

One GLM-5.3 worker reviewed head a19ecac8a against 1b1b1131e.

  • It ran on the Z.ai endpoint for 21 min, 90 turns, and ended EXIT=0.
  • It could only read, search, run shell commands and write its report. kubectl, minikube, gcloud, docker, gh and helm were replaced by stubs that exit 99.
  • An Opus 5.5 driver then checked each finding against the head and the base.

Result:

  • 2 confirmed Low, 2 pre-existing and 2 over-defensive.
  • Nothing is above Low.
  • This lane did not find the High blocker B2-01 from the blind review.

Confirmed, Low

  • ZS-4: the new gate fails with "use an explicit Context migration" (deploy/scripts/lib/bootstrap-instances.sh:110), but no such procedure is documented anywhere. The gate applies on every instance path, so the first change to a declared field of a Context manifest blocks all of them on existing profiles and clusters. Same as B1-01 / RP-A-01.
  • ZS-5a: no test pins create --save-config (bootstrap-instances.sh:119). With the flag removed on a copy of the head, test-provision-gfs-runtime.sh still passed (EXIT=0, 32 PASS). Same as B4-02.

Pre-existing

  • ZS-1: scripts/bootstrap-cluster.sh:222 still runs kubectl apply on a different context1 manifest. If that script creates context1 first, the new gate stops the next provisioning run. That failure is loud, as designed. It is worth naming in the PR's out-of-scope list.
  • ZS-2: scripts/minikube/setup.sh:213 was rewired, but the script exits at line 90 because deploy/minikube does not exist.

Over-defensive, no change needed

  • ZS-3: when a last-applied annotation exists, the live object's drift is not compared. That matches the rule stated in the PR body.
  • ZS-5b: a failure in the middle of the non-Context apply list is not tested. The code returns 1 on any failure of that apply.

@zach88

zach88 commented Nov 14, 2026

Copy link
Copy Markdown
Contributor

Took a deep pass through this one at 125006993. The redesign in 655b926ee holds up well where it matters most: on a cluster that already has context1, the connector list survives, the last-applied migration is tight, and the earlier List unwrap issue is properly fixed. I checked that path against a real kube-apiserver (v1.33) with kubectl 1.27, 1.31 and 1.34, not just the stub. The one real problem is on the other branch: a fresh install where context1 doesn't exist yet. Most of the rest is test coverage on the E2E journey and a PR body that still describes the old design.

🔴 Not ready to approve

  • Must resolve before approval: 1
  • Optional / non-blocking: 17

This is my read from the review, not a formal GH approval.

Must resolve before approval

  1. [R1-H2] A fresh install (no context1 yet) always fails: kubectl create rewrites the last-applied annotation, so the next apply tries to delete mcpServers
    • deploy/scripts/lib/bootstrap-instances.sh:118-125 builds context1 with a hand-made last-applied-configuration that leaves out mcpServers, then runs kubectl create -f -.
    • Without --save-config, kubectl create overwrites a last-applied annotation that is already on the object with the full object, so the stored value now contains mcpServers: [].
    • The apply at :168 then computes a deletion and sends "spec":{"mcpServers":null}. The apiserver rejects it with The Context "context1" is invalid: spec.mcpServers: Required value and rc=1, so set -euo pipefail stops the run.
    • Who hits it: every fresh minikube setup (scripts/minikube/full-setup.sh:1971, :1977), a new GKE cluster, or a deleted context1. Existing dev and prod clusters are not affected. A second run recovers on its own, because the migration cleans the annotation.
    • Repro: disposable kube-apiserver v1.33.5 + etcd, the CRDs from this branch, and the real apply_bootstrap_instances against the gcp-dev instances with no context1 present. The output is context.clerum.io/context1 created, then spec.mcpServers: Required value, rc=1. Same result on kubectl 1.27.16, 1.31.3 and 1.34.1. To isolate the cause, I created an object carrying the annotation {"marker":true}; the stored annotation came back as the full object. -v=9 shows the mcpServers: null in the PATCH body.
    • Why the suite is green: the kubectl stub in scripts/tests/test-provision-context-preservation.sh:288-332 stores the annotation as given and doesn't mimic the rewrite on create. That's the same stub-vs-real-kubectl gap as the earlier List issue. Relatedly, removing the annotation injection at :118-121 also leaves the suite green.
    • Possible directions (please check the side effects before picking one): (a) create context1 with no last-applied annotation, so the following apply records a baseline without mcpServers and computes no deletion; or (b) create it, then set the annotation with a guarded patch --type=json (with test ops) the way the migration already does. Either way, a regression test against a real apiserver (envtest or kind) would close this class for good; the stub can't catch it.

Optional (non-blocking)

These can land as follow-ups or at your discretion; none of them blocks the merge.

  1. [R1-M1] PR body describes the previous design

    • The body still says "preserve an existing Context without writing it", "treat Context manifests as bootstrap configuration" and "if other declarative Context fields differ… stop and require an explicit migration".
    • Since 655b926ee the helper applies every field of context1 except mcpServers and patches its last-applied (bootstrap-instances.sh:75-82, :131-171). There is no drift stop anymore.
    • Only mcp-server/context1 is protected; any other Context is still overwritten.
    • The "declarative drift" line under Validation describes a test that no longer exists. "ShellCheck passed" also isn't quite true: provision-gfs-runtime.sh:5 adds a new SC1091.
    • The body doesn't mention that minikube-deploy-instances now takes the T2 lease.
  2. [R1-M2] The two call sites in minikube-deploy-all-body aren't tested (Makefile:479-491)

    • This is the most common local redeploy path and the one the E2E journey uses.
    • Repro: reverting both calls back to $(KC) apply -f … leaves test-provision-context-preservation.sh and test-minikube-makefile.sh green.
    • A case for MINIKUBE_GFS_MUTATION=true and one for false, or pulling the instance block into a sub-target you can test, would pin it.
  3. [R1-M3] The 60 min test timeout is smaller than the per-step budgets (connector-persistence.spec.ts:431, issue-628.config.ts:6 vs :348 x3, :531, :545 x2, :570)

    • The step budgets add up to more than 7000 s in restart mode.
    • If the test times out, Playwright closes the page and deleteFixture(page) at :684 can't clean up. execFile also leaves run-with-deadline running, so provisioning can keep changing the profile after the test has been reported as failed.
  4. [R1-M4] Provisioning failures in the spec are still opaque (connector-persistence.spec.ts:57-74, :313-316, :492, :631, :710)

    • run() drops stderr except for the ISSUE628_RESTART_* tokens, so a [bootstrap-instances] error, a kubectl error or an MCP result mismatch all come out as provision-instances failed: 1.
    • The polls swallow the cause with catch { return false }.
    • Allowing a few more prefixes through, or writing the full stderr to a report file, would help a lot. Given R1-H2, the first real run will probably need this.
  5. [R1-L2] mcp-server/context1 is hardcoded and fails silently (bootstrap-instances.sh:78-80). An overlay with a different name or namespace loses the protection and nothing is logged. A log line when a non-target Context gets a full apply would make this visible.

  6. [R1-L3] The overlay can no longer revoke a connector from context1 (bootstrap-instances.sh:75-82). That's intended, and it doesn't widen anyone's privileges. Still, it's worth one runbook line saying that cutting off a compromised MCP server now goes through Control UI or kubectl patch, not the overlay.

  7. [R1-L4] Stub shapes real kubectl never emits (test-provision-context-preservation.sh). GET_RESPONSE=object for get -f - and RENDER=list for create --dry-run=client are in the success loop. Real kubectl always wraps stdin get in a List and prints create output as separate objects. That leaves the bare-object branch at bootstrap-instances.sh:29 unreachable and makes the "previously supported single-object form" comment at :17-19 inaccurate.

  8. [R1-L5] Any helper failure aborts before finalize_credentials_after_overlay (provision-gfs-runtime.sh:237). That covers the create race, the JSON patch test failing and an invalid last-applied. It fails closed and a rerun converges, but it deserves a line in the runbook.

  9. [R1-L6] The new lease on minikube-deploy-instances isn't documented everywhere (Makefile:409-414). It now refuses clerum-test and profiles that haven't been bootstrapped. docs/crds/workflowrecipepolicy.md:105, scripts/e2e/seed-stateless-host.sh:85 and the target's own help text still say to run it without a lease.

  10. [R1-L7] jq is now required but missing from Prerequisites (docs/get-started/quickstart.md:10-20, docs/deploy/minikube.md:45-53). full-setup.sh:796-801 exits without it. make prereqs does catch it.

  11. [R1-L8] Two restart guard conditions have no rejecting case (scripts/e2e/issue-628-profile-restart.sh:10,12: T2_PROFILE != PROFILE and T2_SKIP_LOCK != true). Repro: a mutant with both removed still passes test-issue-628-profile-restart.sh.

  12. [R1-L9] Sibling of the argv fix: full-setup.sh still passes the LLM keys with --from-literal (scripts/minikube/full-setup.sh:966-973, :1012, :1023). apply-llm-secret.sh moved them to stdin, but this other writer of chatllm-api-keys didn't. In isolated mode the values are fixtures, so a written "won't fix" is fine too.

  13. [R1-L10] Readiness right after minikube start may be stale (issue-628-profile-restart.sh:60). The apiserver can still report Ready=True from before the stop. Worst case it fails loudly and never passes when it shouldn't. Checking startTime against the restart timestamp would tighten it.

  14. [R1-L11] The spec's provisioning runs without --skip-auth-sync (connector-persistence.spec.ts:338-346). The child scripts take the remote path and skip their own identity checks. The outer lease still holds.

  15. [R1-L12] Exact equality on the allowlist can go red for the wrong reason (connector-persistence.spec.ts:480). workflow-recipes restarts during minikube-deploy-all and its union could re-add a recipe server. Asserting "connector present and nothing from original lost" is enough.

  16. [R1-L13] The control-api, external-rest-api and rpc-proxy rollouts aren't awaited before the re-forward (connector-persistence.spec.ts:553, :592). It's a small race; a rollout status before refreshProfileForwards() closes it.

  17. [R1-L14] The >= 3 NetworkPolicy threshold isn't tied to what HCC actually generates (connector-persistence.spec.ts:489, :500). If HCC generates fewer, the poll times out and the test fails for the wrong reason.

What checked out clean

  • On an existing context1, verified against a real apiserver: kubectl sends single documents with fieldValidation=Strict, so the server validates the merged object and the missing mcpServers is fine. The comment at :164-166 about v1/List forcing client-side validation is correct. After the migration, the three-way patch never touches mcpServers. A live ["notion","sentry"] survives, the second run is unchanged, and an emptied list from the UI is respected. Prod takes the same path and behaves the same.
  • The List unwrap from the earlier round is correct. A list with 0 or N items, or a mismatched identity, fails before anything is mutated. Duplicate context1 declarations are rejected up front.
  • The three JSON patch test ops are all load-bearing: dropping any of them turns the suite red. A UI edit that lands between the get and the apply is preserved.
  • The regression test fails against the merge-base provision-gfs-runtime.sh on the assertion, not the setup. The isolation and prereq tests fail against the base the same way.
  • The isolation work is solid. With the flag unset, the default behaviour is byte-for-byte unchanged. With it set, apply-llm-secret.sh never sources a .env, the keys are off argv, set +x comes first, and clusterEnv keeps the admin password out of child processes. Prereqs now resolve the password the same way the loader does.
  • The fixture process helper only signals a single PID whose start time and command both match, and it treats a reused PID as already gone.
  • The restart script's order is now readiness, then port-forward, then health, pinned by its test. The unauthenticated case asserts the API 401. The CI --list asserts Total: 2 tests in 1 file.
  • CI: pull_request trigger, no secrets, no PR-controlled ${{ }} interpolation, no new actions. The lockfile change is just the package's own version.
  • Suites on bash 3.2: context preservation 63 PASS, gfs-runtime 65 PASS. minikube-makefile, gcp-scripts, minikube-full-setup, apply-llm-secret-isolation, fixture-process, check-prereqs-isolated-dotenv, profile-restart and sdk-auth-bootstrap all pass. branch-profile-lifecycle passes with 1059 assertions. The static audit of the spec passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants