Skip to content

fix: make code defaults match the values deployments run - #1305

Merged
alfredolopez80 merged 84 commits into
devfrom
fix/env-audit-code-defaults
Nov 11, 2026
Merged

alfredolopez80 merged 84 commits into
devfrom
fix/env-audit-code-defaults

Conversation

@alfredolopez80

@alfredolopez80 alfredolopez80 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Seven code defaults disagreed with the values deploy/base or the overlays set,
and one shell check was fail-open. This PR makes each code default equal the
value deploy/base sets, or removes a value that is valid on one provider only, apart from P3-4, whose
default is the deploy/base list plus one registry (see below the table). For P3-8 the deploy/base value
is new in this PR: the in-repo renders ran the old code default, so they change (see
"Effect per consumer"). Most items have a main commit and one or more follow-ups from review; no new environment variable.

Item Old default New default Root cause (lines at 562d2e480, the PR's first base)
P3-1 Host and WFC image pull Secret (HCC) clerum empty (no imagePullSecrets) host-context-controller/src/config.ts:708,755 (5d663a496); second copy at deploy/scripts/host-layout-merge.sh:457 (a4c1b1c3b)
P3-1 follow-up Q-LM (host-layout-merge.sh) unresolvable HCC image settings were reported and then ignored the run stops before the orphan check Pod deploy/scripts/host-layout-merge.sh:503 here-string discards the die status (a4c1b1c3b)
P3-2 runtime and minimal-infra namespaces (HCC) mcp-server,mcp-host,sandbox-recipes,rpc-proxy / '' mcp-server,sandbox-recipes,rpc-proxy,sandbox-ui / sandbox-ui host-context-controller/src/config.ts:656-659,672 (5d663a496)
P3-3 Host workspace StorageClass (HCC) do-block-storage-retain unset omits storageClassName (an explicit "" is kept) host-context-controller/src/config.ts:712-715, hostReconciler.ts:3072 (5d663a496)
P3-4 plugin image allowlist (@clerum/image-policy) us-central1-docker.pkg.dev/your-gcp-project/clerum/,registry.evenfire.ai/,mongodb/,mcr.microsoft.com/,clerum/ ghcr.io/evenfire-ai/,mongodb/,mcr.microsoft.com/,clerum/,registry.evenfire.ai/ packages/image-policy/index.cjs:53-59 (cc86e5b6f, 106efe7c4)
P3-6 WORKFLOW_APPROVAL_MAX_TTL_SEC (control-api) 604800 172800 control-api/src/config.ts:887 (5d663a496)
P3-8 external-egress overlap (HCC, WRC) and WRC max entries 300 / 128 3600 / 256 workflow-recipes/src/config.ts:129,138 (1ac021146); host-context-controller/src/config.ts:867 (17eb4d5b9)
P3-10 WORKFLOW_APPROVAL_READER_ENABLED_MEDIA (WAR) telegram,slack telegram,slack,teams workflow-approval-request-reader/src/config.ts:83 (5d663a496); 404960c73 added teams to the parser and to deploy/base but not to the code default
P3-13 CONTROL_API_PROXY_TIMEOUT_MS (control-ui) 30000 300000 control-ui/app/control-api/[...path]/route.ts:7 (a2c9bf79e)

P3-4's default is the deploy/base list plus the shared Evenfire registry. Compared
with the old default it adds ghcr.io/evenfire-ai/, so with enforcement on and the
variable unset those images go from refused to accepted. It drops the your-gcp-project
placeholder.

Effect per consumer

  • deploy/base without an overlay: P3-1 changes (one restart, see below), P3-3
    changes for new PVCs, and P3-8 changes the egress window. P3-2, P3-4, P3-6 and
    P3-10 keep the values deploy/base already sets.
  • Minikube overlay: HCC sets both pull Secret variables to "", which at this head means no pull Secret
    (deploy/scripts/host-layout-merge.sh:458, covered by host-layout-merge.test.mjs), so
    host-layout-merge.sh proceeds. The P3-1 follow-up (Q-LM) makes the script stop at the HCC image lookup
    with the reason when a value cannot be resolved, such as a valueFrom it cannot read; before, it went on
    to create a check Pod with an empty image and ended with exit 2. P3-8 moves the overlay's HCC overlap from
    300 to 3600 s and the WRC entry cap from 128 to 256 through the new deploy/base entries. The
    overlay pins WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS at 300
    (deploy/overlays/minikube/patches/wrc-external-egress-overlap.yaml), because the WRC egress degradation
    E2E refuses a DNS window above its 505 s bound (DNS_WINDOW_EXCEEDS_GATE_RECOVERY_BUDGET);
    scripts/tests/test-wrc-egress-gate-overlap-render.sh checks this in CI. No other item changes the overlay.
  • Public kit template: P3-8 only (300 → 3600 s, 128 → 256).
  • Operated development and production deployments that set every one of these
    variables: no change for any item.
  • Runs with the variables unset (tests, npm run dev): the new defaults.

One-time effects

  • P3-1: on an install that sets neither pull Secret variable, the first
    reconcile after the upgrade removes imagePullSecrets: [{name: clerum}] from
    every Host and workspace-files-controller Deployment. Both use the Recreate
    strategy, so each running Host pod and each workspace-files-controller pod
    restarts once; suspended Hosts start nothing, and a Host whose suspension is blocked keeps its
    template until the hold ends.
  • P3-3: existing PVCs keep their StorageClass. HCC does not update a bound PVC,
    and when it replaces an unbound one with the variable unset it carries over
    the class already set (Kubernetes allows only nil → class on update). A Host
    workspace PVC that stayed Pending because the cluster has no
    do-block-storage-retain class therefore keeps that class; delete it and HCC
    recreates it with the default class on the next reconcile.
  • P3-1 and the layout tool: if the Host image needs the clerum pull Secret and
    CONTEXT_MAPPER_HOST_IMAGE_PULL_SECRET is unset on HCC, set it to clerum on
    HCC before running deploy/scripts/host-layout-merge.sh inventory. The tool
    now reads an unset value as no pull Secret, so without it the orphan check
    Pods cannot pull and their PVCs report UNREADABLE (see
    docs/deploy/host-pvc-layout-upgrade.md).
  • P3-8: where the key is unset, a resolved /32 stays in the egress NetworkPolicies after its
    DNS TTL for up to one hour instead of up to five minutes, and WRC keeps up to 256 entries per policy. An IP rotated
    out of a stable set stays for between about half an hour and an hour (measured 29 to 60 minutes through
    reconcileEgressState and its write gate), because the window is written again only when under half of it is left. While DNS lookups for a
    declared hostname fail with a transient error such as a timeout or SERVFAIL, HCC and WRC keep the IPs they
    already have beyond that hour (fail-static) until a lookup succeeds or the hostname is removed from the
    declaration. When the resolver answers that the hostname has no records (ENOTFOUND, ENODATA), HCC and WRC remove
    its IPs on the next pass that reaches that policy, without waiting for that hour. Values supplied through envFrom no
    longer apply, because the deploy/base env entries take precedence.

Release note

Changed code defaults
adds one subsection to the upgrade notes: what changes for each item, the
maintenance window P3-1 needs, the PVC note for P3-3, the egress window for
P3-8, the control-ui proxy timeout for P3-13, and how to keep an old value.

Tests

Fail-before / pass-after per commit (test changes applied with the production
code at the previous commit):

Commit Before After
1 P3-1 HCC 3 files, EXIT=1, 179/180 (pull-Secret config); layout EXIT=1, 24/26 HCC 180/180, layout 26/26, EXIT=0
2 Q-LM layout EXIT=1, 26/27 (the Q-LM case) 27/27, EXIT=0
3 P3-2 EXIT=1, 2/4 (runtime default, minimal-infra default) 4/4, EXIT=0
4 P3-3 EXIT=1, 121/123 (PVC builder, storage-class config) 123/123, EXIT=0
5 P3-4 control-api EXIT=1, 7/10 (three cases); HCC imagePolicy 2/2 10/10, 2/2, EXIT=0
6 P3-6 EXIT=1, 81/82 ("above 2-day cap") 82/82, EXIT=0
7 P3-8 + WRC max entries HCC EXIT=1, 13/14; WRC EXIT=1, 60/61 14/14, 61/61, EXIT=0
8 P3-10 WAR EXIT=1, 1/2 2/2, EXIT=0
9 Docs — —

15 cases are red before their item and green after (HCC 6, control-api 4, WRC 1,
WAR 1, layout 3). Two cases pass before and after on purpose: the Host builder
case pins the consumer contract the P3-1 fix relies on ("empty means none"), and
the "2 days exactly" case witnesses the "above 2-day cap" case.

Every negative assertion has a witness: a regcred read-back and the built
Deployment's image for P3-1; list equality plus not.toContain('mcp-host') and
an explicit "" for P3-2; not.toHaveProperty('storageClassName') beside the
access modes and size for P3-3; the deny loop beside a listed host accepted by
the same list for P3-4; the at-the-cap case for P3-6; explicit override values
(HCC overlap 120, WRC 512) for P3-8; an explicit slack list read back and
an unsupported medium that throws for P3-10; the check-Pod image plus status 2
and stderr for Q-LM.

Mutations, each applied to the final tree and each turning its named case red
(17/17): p31-default 0/1; p31-builder-vacuity 121/122; p31-wfc-vacuity
57/58; p31-script 26/27; lm-herestring 26/27; p32-default 3/4;
p32-minimal 3/4; p33-empty 121/122; p33-default 0/1; q7-placeholder 8/10;
q7-registry 8/10; q7-ghcr 7/10 (the deny case's witness goes red);
p36-default 81/82; p38-hcc 13/14; p38-wrc 60/61; q9-wrc 60/61;
p310-default 1/2.

Desktop flaky tests (0b458b27f)

Two T2 runs on 35ec08f2a stopped in pre-gate-sync on Desktop renderer tests
this PR does not otherwise touch. All three defects are in the tests; no
production code changes.

  • App.deepLinks.test.tsx › "renders a fresh B workspace after successful A
    logout and null session" (Desktop test App.deepLinks 'renders a fresh B workspace after successful A logout' is flaky #1278). The suite teardown only unmounted the real
    App, so the previous test's pending post-paint bootstrap ran during the next
    test and read the new bridge's team directory. The teardown now ends the
    controller's session through its external-logout listeners, so the
    controller's own identity check drops that task, and the test checks the call
    count once the directory is hydrated.
  • FilePreviewPage.test.tsx › "does not let a rename digest started in the old
    generation…". Switching to generation 2 legitimately re-reads the resource and
    source-folder authority; the test counted those reads whenever they landed
    before its sample. It now holds generation 2's read while the stale rename
    continues, asserts no rename, then releases it and witnesses the +2 reads.
  • FilePreviewPage.test.tsx › "keeps the preview delete confirmation busy…"
    (test(desktop): FilePreviewPage delete-confirmation focus assertion is flaky #1317). The dialog opens after an awaited permission check, so its initial
    focus could land after the spy was installed. The spy is now installed after
    that focus.

Each file passed 5 of 5 runs (107/107 and 23/23). One mutation per test turns it
red: no session retirement with 40 ms animation frames (#1278), the rename's
post-digest check removed, and the focus effect re-run on pending.

Fixes #1278
Fixes #1317

Verification

Pre-review run, on HEAD 90892308d, per package in the order .github/workflows/ci-public.yml
runs it:

  • packages/workflow-runtime-core (needed by workflow-recipes and
    control-api): npm ci EXIT=0; npm run build EXIT=0.
  • host-context-controller: npm ci EXIT=0; the Playwright MCP cluster probe
    node suites 37/37, EXIT=0; npm run build EXIT=0; npm test 120 files,
    2789 passed, 0 failed, EXIT=0; npx tsc --noEmit EXIT=0.
  • workflow-recipes: npm ci EXIT=0; npm run build EXIT=0; npm test
    154 files (150 passed, 4 skipped), 3275 passed, 29 skipped, 0 failed,
    EXIT=0; npx tsc --noEmit EXIT=0.
  • control-api: npm ci EXIT=0; external-rest-api npm ci and
    npm run build EXIT=0 (CI builds it before the control-api build);
    npm run build EXIT=0; npm run typecheck:jwt-contract EXIT=0;
    npm test with the default and JSON reporters 658 files (567 passed,
    91 skipped), 8312 passed, 671 skipped, 0 failed, EXIT=0; pinned suites
    EXIT=0; pinned cases EXIT=0; npx tsc --noEmit EXIT=0.
  • workflow-approval-request-reader: npm ci EXIT=0; npm run build
    EXIT=0; npm test 11 files, 88 passed, 0 failed, EXIT=0;
    npx tsc --noEmit EXIT=0.
  • packages/image-policy: no test or build script, as in CI. npm ci
    EXIT=0; node --check index.cjs EXIT=0; the exported default equals the
    five prefixes in order. Its consumers hold: control-api
    test/imagePolicy.test.ts 10/10 and HCC test/config.imagePolicy.test.ts
    2/2, both EXIT=0.
  • Shell: bash -n deploy/scripts/host-layout-merge.sh EXIT=0;
    shellcheck --severity=error and shellcheck at all severities EXIT=0
    (0.11.0); deploy/scripts/host-layout-merge.test.mjs 27/27 EXIT=0;
    bash -n over deploy/scripts and scripts 391 files, EXIT=0; the
    shell-syntax job's four node test files 52/52, EXIT=0.
  • node scripts/release/validate-release-version-bumps.mjs --previous origin/dev:
    "Release package version bumps are valid.", EXIT=0.
  • git diff --check origin/dev...HEAD: EXIT=0.

No run reported zero tests; the package without a test script is covered by its
two consumer suites above.

Local T0 (ELECTRON_RUN_AS_NODE unset for every command), from the worktree
root:

  • bash -n on scripts/minikube/t2-common.sh, t2-preflight.sh, t2.sh,
    t2-setup-handoff.sh and scripts/tests/test-minikube-t2-setup-handoff.sh:
    EXIT=0.
  • bash scripts/minikube/t0.sh: affected packages control-api host-context-controller packages/image-policy workflow-approval-request-reader workflow-recipes; T0_SHELLCHECK=PASS; every package test and build
    EXIT=0 (control-api 658 files / 8312 passed / 671 skipped; HCC 120 / 2789;
    WAR 11 / 88; WRC 154 / 3275 passed / 29 skipped; image-policy reported "no
    test script" and "no build script"); T0_AFFECTED_CHECKS=PASS; T0_EXIT=0.
  • bash scripts/tests/test-minikube-t2-contract.sh: EXIT=0.
  • bash scripts/tests/test-minikube-t2-setup-handoff.sh: EXIT=0.

desktop-app is not in T0's affected set for this diff, so no Desktop install
was needed.

After the first review fixes, on f65037cde: HCC 2791/2791, control-api 8314 passed, WRC 3275 passed, WAR 88/88, host-layout-merge.test.mjs and the egress proof oracle (43/43) green; scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS; make minikube-t2 on the branch-owned profile (full reconcile): MINIKUBE_T2_PASS, T2_EXIT=0 (T1 790 tests, 97/97 files; NP-08 passed; port-forwards passed).

Before the review fixes, on 90892308d: make minikube-t2 on a branch-owned profile (full bootstrap): MINIKUBE_T2_PASS, T2_EXIT=0 (T1 790 tests, 97/97 files; NP-08 passed; port-forwards passed). The first run on that head stopped in pre-gate-sync on one Desktop test, App.deepLinks.test.tsx › "renders a fresh B workspace after successful A logout and null session" (5411/5412), a known flaky test tracked in #1278; that file is not in this diff, it passed 5 of 5 isolated runs, and the re-run on the unchanged head passed.

After the final review fixes, on 35ec08f2a: HCC 2793/2793 (120 files), WRC 3275 passed / 29 skipped, host-layout-merge.test.mjs 27/27, WRC egress unit contract 143/143, test-np-preflight-inventory.sh 7/7, test-wrc-egress-gate-overlap-render.sh 4/4 overlays plus the negative control; the minikube renders differ from f65037cde only in the WRC overlap (3600 → 300); scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS. On 0b458b27f (that tree plus the Desktop test fixes): make minikube-t2 on the branch-owned profile (full bootstrap): MINIKUBE_T2_PASS, T2_EXIT=0 (T0 including Desktop 447/447 files; Desktop 5412/5412 in pre-gate-sync; T1 790 tests, 97/97 files; NP-08 passed; port-forwards passed).

381202d47 merges dev twice on top of that: #1086 (Desktop tab overflow; its FilePreviewPage rename-test fix conflicted with this PR's and was resolved to this PR's version, which keeps the digest-delivery witness; #1086's generation-key witness was lost in that resolution and is restored in e0854be98) and #1295 (rpc-proxy; the release-notes table now has rows for the rpc-proxy and snippet runner notes, and 1fffea7bc adds the row for "Changed code defaults"). scripts/minikube/t0.sh on 381202d47: T0_AFFECTED_CHECKS=PASS (control-api 568 files, Desktop 455/455 files including the browser tests #1086 added, HCC 120/120, WAR 11/11, WRC 150 passed / 4 skipped); test-wrc-egress-gate-overlap-render.sh 4/4 overlays plus the negative control; test-np-preflight-inventory.sh passed.

c99e9b028 merges dev again, up to #1299 (snippet runner). The only conflict was the workflow-recipes version line, resolved to dev's 0.1.490; that left dev and this branch shipping different workflow-recipes/src/config.ts under one version, and validate-release-version-bumps.mjs (which checks only external-rest-api and rpc-proxy) did not flag it. c310e8b8c bumps it to 0.1.491.

The second review round adds e0854be98 (the rename test's generation-key witness, and a finally in the App.deepLinks teardown), c310e8b8c (the version bump), 99107a44b (tests for the egress cap parity, the allowlist override parsing and the configured PVC class; the 422 mock now allows nil -> class), 2dc2dfa28 (the NP preflight inventory refuses an explicitly empty runtime list and takes the last duplicated env entry) and 1fffea7bc (docs). Each new test was shown to fail under the mutation it targets: generation-1 affordance key, oracle cap 128, toLowerCase() and || in the allowlist parser, dropping the no-class condition, and the three inventory mutations (9/10 each). Touched suites: Desktop FilePreviewPage + App.deepLinks 130/130, HCC hostReconciler.test.ts 127/127, control-api image policy 4/4, egress proof 44/44, NP inventory 10/10. scripts/minikube/t0.sh on 1fffea7bc: T0_AFFECTED_CHECKS=PASS (control-api 568 files, Desktop 455/455, HCC 120/120, WAR 11/11, WRC 151 passed / 4 skipped). make minikube-t2 on 1fffea7bc (branch-owned profile, full bootstrap): MINIKUBE_T2_PASS, T2_EXIT=0; Desktop 5494/5494 in pre-gate-sync; T1 790 tests, 97/97 files; NP-08 passed; port-forwards passed.

0915f2815 merges dev up to #1356 with no conflict. Both sides had moved control-api from 0.1.866 to 0.1.870, so dev and this branch shipped different control-api/src/config.ts under one version: a clean merge runs no pre-commit hook, and validate-release-version-bumps.mjs did not cover control-api. 6923fe97c bumps it.

8af728fd1 merges dev up to #1271. The only conflict was the HCC version line; the hook then moved it to 0.1.421. #1271's scripts/e2e/e2e-egress-cdn-rotation.sh read the HCC_EXTERNAL_EGRESS_OVERLAP_SEC entry this PR adds to deploy/base as a leftover override and refused to start on every profile; f1cc6a1f1 fixes that by comparing the value the run sets.

The third review round adds:

  • 2d26e0448: one apiserver-like mock for the unbound PVC replace tests. The class-change test now expects the failed pass the apiserver really causes (fix(hcc): unbound Host PVC replace fails on every reconcile after CONTEXT_MAPPER_HOST_WORKSPACE_STORAGE_CLASS changes #1310), and new rows cover a live claim with no class.
  • 57d32113d: tests that read each changed code default and its deploy/base value, in control-api, HCC, WRC and WAR. Overlays differ on purpose and are not compared. Variables deploy/base leaves unset are asserted absent.
  • 584646e25: the App.deepLinks teardown resets suite state when session retirement fails.
  • 8994bca0a: hcc_env in host-layout-merge.sh takes the last duplicated env entry, like the NP preflight inventory.
  • 25860ffde: corrects who sets the infra pull Secret in registry-pull-secret-self-provisioning.md.
  • 29bdd394d: this PR's version collision happened twice, so validate-release-version-bumps.mjs gains --all-services, which CI runs on pull requests into dev against the merge result. It stays off the push leg, where a failure has no branch left to fix, and off promotions into main. That commit's message says an out-of-date branch can still collide; dev requires up-to-date branches, so it cannot, unless the ruleset is bypassed. docs/deploy/releases-and-upgrades.md lists what the check covers and what it does not, which includes workflow-approval-request-reader and packages/** other than packages/jwt-key-policy/.

Each new test was shown to fail before its fix or under the mutation it targets. Touched suites: test-egress-cdn-rotation-cleanup.sh 121/121 on bash 5 (on /bin/bash 3.2 it failed check 120 until the fifth round), HCC hostReconciler.test.ts 129/129, the config.deployBaseDefaults tests 10/10, App.deepLinks 108/108, host-layout-merge.test.mjs 28/28, test-validate-release-version-bumps.sh 6/6. The validator fails on the real collision (e946cdecd against 0915f2815) and passes on this head. e2e-egress-cdn-rotation.sh phase 0 itself needs a cluster and was not run; the hermetic test covers its predicate and call site. scripts/minikube/t0.sh on 29bdd394d: T0_AFFECTED_CHECKS=PASS, T0_EXIT=0 (control-api 8323 passed / 671 skipped, Desktop 5495/5495, HCC 2898/2898, WAR 89/89, WRC 3289 passed / 29 skipped).

3b050e441 merges dev up to #1210.

The fourth round adds:

  • 91171e6cf: renames a test variable the public boundary read as a credential, which had turned Shell script syntax red. scripts/minikube/t0.sh does not run the boundary; the contract test does.
  • 52360f70d: moves the --all-services step to the required Repo hygiene job, fails a version below the base, stops counting test files, and pins the service list against the hook's roots. The pin itself stayed in the non-required Shell script syntax job until the fifth round.
  • 8fd096b35: tests the WFC pull Secret through the reconciler's default config, and a live "" PVC class.
  • 77da97a54: corrects the egress and approval upgrade notes.
  • 9defe5cf8: shows the kustomize error in the NP preflight, and tests its helper call and the layout tool's image stops.

Local checks on 9defe5cf8: make minikube-t2-public-boundary PUBLIC_BOUNDARY_PASS; test-validate-release-version-bumps.sh 76 checks, 0 failed; validate-release-version-bumps.mjs --previous origin/dev --all-services passes; test-np-preflight-inventory.sh NP_PREFLIGHT_INVENTORY_PASS. scripts/minikube/t0.sh: T0_AFFECTED_CHECKS=PASS (control-api 8323 passed / 671 skipped, Desktop 5510/5510, HCC 2901/2901, WAR 89/89, WRC 3289 passed / 29 skipped). make minikube-t2 on 9defe5cf8 (branch-owned profile, full bootstrap): MINIKUBE_T2_PASS, T2_EXIT=0; Desktop 5510/5510 in pre-gate-sync; T1 790 tests, 97/97 files; NP-08 passed; port-forwards passed; Health=NOT_RUN. e2e-egress-cdn-rotation.sh was not run: it is a manual lane, and its fix is covered by the hermetic test.

03af981d1 merges dev up to #1285. The conflicts were the control-api version line, moved above dev's 0.1.891 to 0.1.892, and the release note table, which keeps both rows. On 03af981d1: make minikube-t2-public-boundary PUBLIC_BOUNDARY_PASS; test-validate-release-version-bumps.sh passes; validate-release-version-bumps.mjs --previous origin/dev --all-services passes. scripts/minikube/t0.sh: T0_AFFECTED_CHECKS=PASS (control-api 8504 passed / 765 skipped, Desktop 5510/5510, HCC 2901/2901, WAR 89/89, WRC 3289 passed / 29 skipped). The make minikube-t2 result above is on 9defe5cf8; the final checks below replace it.

The fifth round adds:

  • 4ed2694c2: builds the leftover overlap JSON outside the command substitution in test-egress-cdn-rotation-cleanup.sh, which /bin/bash 3.2 brace-expanded.
  • d40a20630: moves the version check's test to the required Repo hygiene job, makes it pin the exact commands and refuse anything that softens them, and has the check look at every file under a service as the hook does. It leaves out what a running service does not load (test code, Markdown, Makefile, makefile, vitest.config.* and .env.example at the service's root, and scripts/ in control-api, mcp-host and workflow-recipes) and the release manifest, counts both sides of a rename, and adds packages/jwt-key-policy/ for control-api. A service the base does not have now passes, and a missing package.json or an unorderable version gives an annotation.
  • d8e2f195c: documents that coverage, what a revert does, and what to do when the check is wrong. 0718f8052 corrects the egress window and the first-reconcile note, and adds the NP preflight's version skew. 514c5a47b states that the hook also skips the release manifest.
  • d68be4537, 63c56fd16, 5aebf44a2: test that the NP preflight walks the derived namespaces, that the WRC recovery bound is the one the gate refuses, and the layout tool's empty and unset image stops. 63c56fd16 moves the gate's budget check into a function the test and the flow both call.
  • 91861f29d: models the PVC update rules the apiserver applies to an unbound claim, and uses that model in the annotations test.

Replayed over the 349 first-parent merges of dev, the previous check fails 59 and this one 64. The 5 new ones are lockfile, package.json, tsconfig.json or next-env.d.ts changes at an unchanged version. In that replay, excluding scripts/ keeps one merge green and excluding the root files changes no verdict. Each new test was shown to fail under the mutation it targets, and every changed shell test passes on /bin/bash 3.2 and bash 5.

The sixth round adds:

  • 3b205f048: corrects the release note: when the resolver answers that a hostname has no records, HCC and WRC remove its IPs on the next pass that reaches that policy.
  • f290fe864: corrects what the version check's doc and comment say it leaves out. channel-reader and external-rest-api compile the test files under src/ into dist/, so the reason is that nothing loads them, not that no image holds them. The test-name pattern covers JavaScript and TypeScript names only. A file moved out of a service gets no hook bump, so the author sets that version by hand; none of the 6055 non-merge commits on dev touched a service only as a rename source, so the hook stays as it is. The check's behaviour does not change.
  • 2514932ba, 1d6964175: the version check's test also refuses a checkout ref, a second checkout and workflow-level defaults or env, gives a FAIL line for fetch-depth: 0, and probes a stylesheet, images, Dockerfile variants and build ignore files.
  • e63ee5ec2, 5c7805aa9: the NP preflight case reads the calls check 4 sends instead of its summary line, and the WRC recovery pin matches the whole statement instead of a substring. That pin is still a source read: the flow needs a cluster.
  • 68d8bfd0f: limits the no-records sentence to passes that reach the policy, because WRC stops a recipe at its first blocking failure. b08437105: states that the version check's test does not pin the job's other steps, and adds a negative for check 4 in the NP preflight case.

The seventh round adds:

  • 6322a0aac: one rule per service in both modes of the version check. d40a20630 gave --all-services its own entries for external-rest-api and rpc-proxy and left the default mode on the old list, so a test file there failed only the default mode, and a Dockerfile, lockfile or package.json edit failed only --all-services. The old list is gone: the default mode checks the two services releaseManifest.ts publishes, with the same rule. The test now runs both modes on the same changes and requires the same verdict: 49 at the head, 47 under those services and 2 under packages/jwt-key-policy/ (8d288c9d2). Replayed over the 350 first-parent merges of dev and 89 commits of main, no verdict changes.
  • a2c69fc84: the check reads changed paths with -z, so a path with a non-ASCII character or a double quote still counts for its service. No tracked path has either.
  • 197df8583: the check escapes %, CR and LF in its annotations, so a version cannot print a workflow command of its own.
  • e0823bb52: the version check's pin accepts permissions, timeout-minutes and persist-credentials on the Repo hygiene job.
  • 91dc684ad: CONTRIBUTING.md and the release doc say how to clear a stayed at failure.
  • 5653def26: the WRC overlap bound test reads the window from reconcileEgressState instead of restating its formula.
  • 8d288c9d2: a test file under packages/jwt-key-policy/ now counts in neither mode (the default mode used to count it for external-rest-api and rpc-proxy); the test probes it in both modes. Deleting external-rest-api/package.json or rpc-proxy/package.json alone now also fails the default mode, as --all-services already did.

The eighth round (zach88's round 6, no must-resolve items) adds:

  • 63c736788, bf799634c, a925bbd76, corrected by b0a64e495: the release note says a fresh install that does not use deploy/base and leaves CONTEXT_MAPPER_RUNTIME_NAMESPACES unset gets none of the four namespace-wide baseline policies HCC used to write in mcp-host, and warns that deploy/base/mcp-host/networkpolicies.yaml, applied as is, opens 443 and 80 to 0.0.0.0/0 (R6-L6, R7-L1, R7-L2). The version check's doc lists the paths git quotes: a double quote, a backslash, a control character, and with git's default core.quotePath a non-ASCII character (R6-L7).
  • 63c736788: the WRC overlap bound test drops protocol, which the accumulator never emits (R6-L5). ci-public.yml gets back the blank line between two steps that the dev merge lost.

f54b69d76 merges dev up to d02fcb9f1. The conflicts were ci-public.yml (both new steps kept), the control-api version (0.1.941, above dev's 0.1.940), App.deepLinks.test.tsx (this branch's reviewed cleanup kept; dev carried an earlier copy) and the release notes table (both rows kept). The merge commit's hook also bumped services this PR does not change; b3a83af17 puts control-ui, mcp-host and the E2E fixtures back on dev's versions.

The ninth round (zach88's round 7) adds:

  • b0a64e495: restores the release note's warning about applying deploy/base/mcp-host/networkpolicies.yaml on its own, says how to apply it and that allow-k8s-api-egress-mcp-host ships with an example address, and names the four baseline policies (R7-L1, R7-L2).

The tenth round adds:

  • 3602cb330: merges dev at 18052d9ce (the Next.js upgrade in control-ui and profile-ui, and new dependency gates). No conflicts; the two files both sides touch (.github/workflows/ci-public.yml, CONTRIBUTING.md) keep both sides' hunks unchanged, and no service version goes below either parent.
  • acf5d4994: P3-13. control-ui's CONTROL_API_PROXY_TIMEOUT_MS defaults to 300000 (was 30000), the value deploy/base sets. A new test reads the base value and checks that the route uses it when the variable is unset; the override case now uses 45000. Reverting the constant fails both default cases.
  • 6e54c106d: review fixes for acf5d4994. The control-ui README and a control-api comment still gave the 30 s default; the release note and docs/surfaces/control-ui.md now say which requests the timeout covers; the GET test asserts the request was proxied.
  • 3f1ff4819: merges dev at b6db9c297 (the sharp 0.35.5 override in control-ui and profile-ui). The only conflict was the control-ui version line: this branch had 0.1.764 and dev 0.1.763; the merge takes dev's package.json and lock and sets 0.1.765. Everything else in 6e54c106d..3f1ff4819 is dev's own change.

Final checks on 3f1ff4819: scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS; the version check's test 320 checks, 0 failed. A blind review of the merge found nothing and judged a new T2 unnecessary: the delta does not touch T1, NP-08 or any backend, and sharp loads only to optimize images, which T2 does not exercise. The T2 below stays pinned to 6e54c106d.

Final checks on 6e54c106d: make minikube-t2 on the branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0, with its own T0 (T0_AFFECTED_CHECKS=PASS) and T1 on this head (T1 1064 tests, 132 files; NP-08 passed; port-forwards passed; Health and Playwright not run). Standalone scripts/minikube/t0.sh on acf5d4994: T0_AFFECTED_CHECKS=PASS (control-api 8836 passed / 944 skipped, control-ui 3105/3105, Desktop 5728/5728, HCC 2904/2904, WAR 89/89, WRC 3289 passed / 29 skipped); the version check's test 320 checks, 0 failed. A blind review of the dev merge found nothing; one of acf5d4994 found four Lows, fixed in 6e54c106d. These replace the final checks on b0a64e495 below.

Final checks on b0a64e495: scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS (control-api 8836 passed / 944 skipped, Desktop 5728/5728, HCC 2904/2904, WAR 89/89, WRC 3289 passed / 29 skipped). a925bbd76..b0a64e495 changes only docs/deploy/releases-and-upgrades.md, so the make minikube-t2 pass on a925bbd76 below was not repeated.

Final checks on a925bbd76: scripts/minikube/t0.sh on 63c736788 T0_AFFECTED_CHECKS=PASS (control-api 8836 passed / 944 skipped, Desktop 5728/5728, HCC 2904/2904, WAR 89/89, WRC 3289 passed / 29 skipped); 63c736788..a925bbd76 changes only docs/deploy/releases-and-upgrades.md. On 63c736788: the version check's test 320 checks, 0 failed, and the CDN cleanup test 121/121, each on /bin/bash 3.2 and bash 5; wrc-egress-gate 39/39; make minikube-t2-public-boundary PUBLIC_BOUNDARY_PASS. make minikube-t2 on the branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0, with its own T0 and T1 on a925bbd76 (T1 1064/1064 tests, 132/132 files; NP-08 passed; port-forwards passed; Health and Playwright not run). CI 60/60 on a925bbd76 after one re-run of a Desktop test this PR does not touch (#1416). These replace the final checks on 8d288c9d2 below.

Final checks on 8d288c9d2: scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS (control-api 8836 passed / 944 skipped, Desktop 5728/5728, HCC 2904/2904, WAR 89/89, WRC 3289 passed / 29 skipped); the version check's test 320 checks, 0 failed, and the CDN cleanup test 121/121, each on /bin/bash 3.2 and bash 5; make minikube-t2-public-boundary PUBLIC_BOUNDARY_PASS. make minikube-t2 on a fresh branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0 (T1 1064/1064 tests, 132/132 files; NP-08 passed; port-forwards passed; Health and Playwright not run). These replace the final checks on b08437105 below.

Final checks on b08437105: scripts/minikube/t0.sh T0_AFFECTED_CHECKS=PASS (control-api 8504 passed / 765 skipped, Desktop 5588/5588, HCC 2904/2904, WAR 89/89, WRC 3289 passed / 29 skipped); make minikube-t2-public-boundary PUBLIC_BOUNDARY_PASS; the version check's test 265 checks, 0 failed, and the CDN cleanup test 121/121, each on /bin/bash 3.2 and bash 5. make minikube-t2 on a fresh branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0 (T1 884/884 tests, 106/106 files; NP-08 passed; port-forwards passed; Health and Playwright not run). This T2 supersedes the earlier T2 results in this body.

CI on 9defe5cf8: Real Postgres integration failed once in passwordPersistence.realPostgres.integration.test.ts (does not repeat an ambiguous successful terminal commit), the race tracked in #1296. This PR does not touch that test or the code it covers, and the re-run of that job on the same head passed.

CI: the first run of "Shell script syntax (bash -n)" failed in scripts/tests/test-minikube-t2-lock-race.sh (two stale-lock reclaimers entered the critical section), the intermittent race tracked in #1254. This PR adds and changes files under scripts/ (scripts/tests/test-np-preflight-inventory.sh, test-wrc-egress-gate-overlap-render.sh, scripts/e2e/_lib/wrc-egress-{gate,proof}.cjs) but not that test or the lock code it covers, and the re-run of that job on the same head passed.

Not in this PR

Follow-ups after the release

  • The infra repository needs one CI change before this PR merges. Its assembled-overlay contract renders this repository's minikube overlay from dev and allows HCC_EXTERNAL_EGRESS_OVERLAP_SEC, WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS and WRC_EXTERNAL_EGRESS_MAX_ENTRIES only in its gcp overlays, so with this PR's deploy/base it reports them as leaked in minikube. Order: merge the infra change that accepts both minikube states (absent, or 3600 / 300 / 256), then this PR, then tighten that contract to the new values. Deploys do not run that contract, and the gcp renders are byte-identical before and after this PR, so nothing changes at runtime.
  • Once the release runs everywhere, the infra overlays and the kit template may delete hcc-pull-ref-overrides.yaml, because both pull Secret keys now default to none.
  • Several comments in an external deployment repository still describe the old external-egress defaults (300 and 128) in the present tense; refreshing them belongs to that repository and is not part of this PR.
  • To revert the whole PR, run git revert -m 1 on its merge commit, or git revert on the squash commit if it was squashed. That also removes the version check, and a conflict-free revert runs no pre-commit hook and lowers every version it touches, so set each service's version above dev's before you push.
  • To undo one item, do not revert commits: items share hostReconciler.test.ts, host-layout-merge.test.mjs and the release note, so a revert also takes other items' tests and the note's corrections. To keep an old value on a deployment, set the variable as the release note says; that needs no code change. To change a default back in the code, edit that default by hand, in a commit of its own. The item's case in config.deployBaseDefaults.test.* then goes red: update it in the same change, because deploy/base and the code default then differ on purpose or both need changing. Edit the item's lines in the release note by hand. The hook bumps the version for a default under a hook root; P3-4's packages/image-policy and P3-10's workflow-approval-request-reader are not under one. For P3-4, bump control-api, host-context-controller and workflow-recipes by hand: all three ship packages/image-policy (file:../packages/image-policy, copied by their Dockerfiles), and neither the hook nor the version check sees a change there.

🤖 Generated with Claude Code

@zach88

zach88 commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Took a deep pass through this one. Overall it's really solid work: one item per commit, the fail-before/pass-after table holds up, and P3-1, P3-2, P3-4, P3-6, P3-8, P3-10 and the Q-LM here-string fix all check out. The one real problem is in P3-3 (Host workspace StorageClass): the omitted storageClassName collides with the PVC replace path. Most of the rest is body/docs sync and a few small test gaps.

🔴 Not ready to approve

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

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

Must resolve before approval

  1. [R1-H1] Unbound Host PVC replace gets a 422 on every reconcile when the StorageClass is unset

    ensurePvc's converge (host-context-controller/src/hostReconciler.ts:4126-4135) PUTs the desired body over every PVC that has no spec.volumeName. With CONTEXT_MAPPER_HOST_WORKSPACE_STORAGE_CLASS unset, that body (buildPvc, hostReconciler.ts:3072-3076) no longer carries storageClassName. The live PVC still has one, though:

    • on new PVCs, the DefaultStorageClass admission plugin stamps it at create time;
    • on old PVCs, do-block-storage-retain is still there.

    The apiserver lets an unbound PVC go from nil to a class (retroactive default), but it rejects going from a class back to nil ("spec is immutable after creation"). So the replace fails, Failed to update Host PVC gets logged every pass, and ensurePvc returns false. In the hold path that leaves the Host ready=false with "degraded, Host PVC did not converge".

    Where it bites:

    • deploy/base with no overlay, on a cluster whose default class is WaitForFirstConsumer (GKE standard-rwo, EKS gp3, kind). A suspended Host never binds its PVC, so the error never goes away. An active Host hits it in the window between PVC creation and pod scheduling.
    • The case the body calls out: a Pending PVC with do-block-storage-retain. Before this PR the replace was a no-op; now it 422s every time.
    • A cluster with no default class when the PVC is created, which gets one later.

    Before this PR, the body always carried the same explicit class as the live object, so the replace was idempotent. Minikube T2 doesn't catch it because the overlay pins standard, which binds immediately.

    I confirmed the replace body comes out without the class against a PVC carrying standard-rwo. I did not reproduce the 422 against a real cluster; that part comes from apiserver PVC update validation.

    Suggested fix: in converge, when config.hostWorkspaceStorageClassName === undefined, copy existing.spec?.storageClassName onto the body before the replace. The test should read back a PVC shaped like what admission leaves (class already set), assert the replace body, and fail on 90892308d.

    This also makes the body line "existing PVCs are not touched" inaccurate (see 4 below).

Optional (non-blocking)

You can merge without these. They're follow-ups or your call.

  1. [R1-M1] host-layout-merge.sh: the new "no pull Secret" default doesn't match when inventory runs

    deploy/scripts/host-layout-merge.sh:457. Per docs/deploy/host-pvc-layout-upgrade.md:34,48, inventory runs before the upgrade, so it runs against the old HCC, whose default is still clerum. With the variable unset, the orphan check Pod now goes out with imagePullSecrets: [] while the live Hosts carry [{name: clerum}]. On a private registry that needs clerum, the check Pod sits in ImagePullBackOff until CHECK_TIMEOUT, the row comes back UNREADABLE, and ORPHAN_UNDECIDED=1 blocks the verdict. The updated expectation in host-layout-merge.test.mjs:444 ([{name:'clerum'}] to []) is exactly that behaviour change.

    Two options:

    • when the variable is unset, inherit imagePullSecrets from any Host Deployment in the namespace (the same HCC rendered it), and fall back to none only if there isn't one;
    • at minimum, document in host-pvc-layout-upgrade.md that the variable must be set before inventory. The release note says "before the upgrade" but doesn't pin the order relative to inventory.
  2. [R1-L1] Body: the minikube overlay does pick up P3-8

    "No other item changes the overlay's effective values" isn't accurate. kubectl kustomize deploy/overlays/minikube sets neither HCC_EXTERNAL_EGRESS_OVERLAP_SEC nor the WRC_EXTERNAL_EGRESS_* keys, so minikube moves from 300 s / 128 entries to 3600 s / 256 entries too.

  3. [R1-L2] Body and release notes: "existing PVCs are not touched"

    HCC does touch them: it tries the replace and fails (item 1). "Keeps that class" only holds because the apiserver rejects the change. Worth syncing the body and docs/deploy/releases-and-upgrades.md once item 1 is fixed, and pinning the invariant with a test.

  4. [R1-L3] No builder test for an explicit "" StorageClass

    host-context-controller/test/hostReconciler.test.ts:2787-2808 only covers undefined and 'standard-rwo'. Changing the !== undefined check in hostReconciler.ts:3074 to a truthy check keeps the HCC suites green (176/176), so "an explicit \"\" is kept" is only pinned at the config layer. Adding a case with hostWorkspaceStorageClassName = '' that asserts spec.storageClassName === '' closes it.

  5. [R1-L4] P3-8 is the one value deploy/base doesn't set

    host-context-controller/src/config.ts:873, workflow-recipes/src/config.ts:129,138. Unlike the other items, nothing in deploy/ sets the overlap or WRC max-entries keys. So this changes the effective value on any deployment that doesn't set them in a private overlay: a rotated-out IP stays allowed for up to an hour instead of five minutes. The risk is well bounded:

    • removing a declared host revokes its IPs immediately;
    • RFC1918 and 169.254/16 are excluded;
    • it's documented.

    Still, I'd set it explicitly in deploy/base (HCC and WRC) so the production value is declared and reviewable, or at least confirm what the private prod overlay sets.

  6. [R1-L5] Release note is missing the old values for three keys

    docs/deploy/releases-and-upgrades.md:245 gives the previous value only for WORKFLOW_APPROVAL_MAX_TTL_SEC. Adding "(was ...)" for these would help the operator who deleted one of them:

    • CONTEXT_MAPPER_RUNTIME_NAMESPACES (mcp-host drops out);
    • CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES (was '');
    • WORKFLOW_APPROVAL_READER_ENABLED_MEDIA (was telegram,slack).
  7. [R1-L6] docs/how-to/connect-teams.md:363-365 still cites the old default

    It says the issue "only bites a reader run by hand with the code default of telegram,slack", which is no longer true after P3-10.

  8. [R1-L7] deploy/scripts/np-enforce-preflight.sh:97-103 still treats unset minimal-infra as empty

    The code default is now sandbox-ui. An overlay that sets CONTEXT_MAPPER_RUNTIME_NAMESPACES but drops CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES would make the preflight expect K8s API egress from sandbox-ui, which HCC no longer allows: a false negative.

  9. [R1-L8] scripts/e2e/_lib/wrc-egress-proof.cjs:12 still has MAX_ENTRIES = 128

    It mirrors the old WRC default, so a policy with 129 to 256 entries (valid by default now) would fail with INVALID_PROVENANCE_LIST. Hard to hit in practice, but it's the sibling of the WRC change.

  10. [R1-L9] Body: Q-LM exit 2 wording

    "Ended the run with exit 2 before a check Pod was created" is slightly off. On the base, the die status was lost in the here-string, run_check went on to kubectl create with an empty image, and the apiserver's rejection of that is where the exit 2 came from. Same end result, different path.

  11. [R1-L10] deploy/scripts/host-layout-merge.test.mjs:453 fixture shape

    { name, value: '' } is hand-written. A real apiserver read omits value: "" and returns { name }. hcc_env yields "" for both forms (.[0].value // ""), so the outcome doesn't change, but adding the { name } case would make the fixture match what the cluster actually returns.

  12. [R1-L11] control-api has no test on the parsed allowlist default

    The image-policy test covers the package constant, but nothing checks that config.allowedPluginImagePrefixes with no env set equals DEFAULT_ALLOWED_PLUGIN_IMAGE_PREFIXES after the split/trim/filter in control-api/src/config.ts:1407. HCC already has the equivalent (host-context-controller/test/config.imagePolicy.test.ts:14); copying it over would close the gap.

What checked out clean

  • P3-1: the Host (hostReconciler.ts:3818) and WFC (sharedFileSystemReconciler.ts:88, sharedFileSystemFactory.ts:317) consumers treat empty as "no pull Secret". deploymentMatchesDesired picks up the removal once and then stays stable, so there's no rewrite loop, and the hold path preserves the template. Both use Recreate, so "one restart" is right. No other clerum pull-Secret default is left in deploy, charts or scripts.
  • Q-LM: host_image="$(hcc_host_image)" propagates exit 2 under set -euo pipefail, and there are no other here-strings with the same pattern in the script. Env values reach the check Pod only through jq --arg, and the Pod stays non-root, with no capabilities and no SA token.
  • P3-2: the only consumers are networkPolicyReconciler.ts:638,672. Dropping mcp-host opens nothing, because its deny-all is static in deploy/base. Adding sandbox-ui tightens things. RBAC and watchers are unaffected.
  • P3-4: every prefix ends in /, so there's no prefix collision. ghcr.io/evenfire-ai-evil/, registry.evenfire.ai.evil.com/, clerumx/ and ../ paths are all rejected. control-api and HCC share the same constant and classifier, WRC doesn't use the default, and dropping the placeholder registry path reduces surface.
  • P3-6: there's a single validation point (user-approval-requests.routes.ts:584) and nothing clamps on read, so already-issued approvals keep their expires_at. No assertion was weakened in the route test.
  • P3-8: interval <= overlap/2 still holds. The annotation byte cap is independent of maxEntries. On a rollback to 128, the tolerant parseState trims without breaking anything, and HCC at 128 alongside WRC at 256 isn't a compatibility problem.
  • P3-10: the parser already accepted teams. With no config, the reader starts and Teams webhooks fail closed: the target is resolved per request and the Bot Framework JWT is verified.
  • Fail-before / pass-after: every new or changed test goes red on an assertion against the merge-base (HCC 6, control-api 4, WRC 1, WAR 1, layout 3), and the declared witnesses pass both before and after.
  • Suites on 90892308d:
    • HCC 2789/2789, plus tsc --noEmit clean;
    • WRC 3275 passed, 0 failed;
    • WAR 88/88, plus tsc clean;
    • control-api targeted suites (imagePolicy, user-approval-requests, registryInstall) 301/301;
    • host-layout-merge.test.mjs 27/27, plus bash -n clean;
    • validate-release-version-bumps.mjs green.

alfredolopez80 and others added 2 commits November 3, 2026 22:05
With CONTEXT_MAPPER_HOST_WORKSPACE_STORAGE_CLASS unset, buildPvc omits
storageClassName. The live claim still has one: the DefaultStorageClass
admission plugin sets it on create, older claims keep
do-block-storage-retain, and the PV controller can assign a default later.
The apiserver allows only nil -> class on a PVC update, so the replace that
ensurePvc sends to every unbound claim was rejected as an immutable spec
change on each reconcile, and the Host stayed not ready.

When no class is configured, the replace now carries the class already set
on the live claim. A configured class is unchanged.

Tests: an unbound claim shaped like the apiserver returns it after admission
(class set, Pending, no volumeName) must converge and the replace body must
keep its class; an explicit "" class must stay on the built claim. The
release note now says how existing claims keep their class.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
host-layout-merge.sh now treats an unset CONTEXT_MAPPER_HOST_IMAGE_PULL_SECRET
as no pull Secret, but inventory runs before the upgrade, against an HCC
whose default is still clerum. A check Pod for an orphan claim or a Host
without a Deployment takes its pull Secret from that variable, so on a
registry that needs clerum it cannot pull the image and the PVC reports
UNREADABLE.

The layout runbook and the release note now say to set the variable to
clerum on HCC before inventory when the Host image needs that Secret.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alfredolopez80 and others added 7 commits November 3, 2026 22:28
Set HCC_EXTERNAL_EGRESS_OVERLAP_SEC=3600 and the WRC accumulator knobs
(WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS=3600, WRC_EXTERNAL_EGRESS_MAX_ENTRIES=256)
explicitly in deploy/base so the production values are reviewable instead of
arriving from the code defaults changed in this release. The minikube overlay
adds env entries through strategic-merge patches and does not override these
keys, so it renders the same values.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L5: list what CONTEXT_MAPPER_RUNTIME_NAMESPACES,
CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES, and
WORKFLOW_APPROVAL_READER_ENABLED_MEDIA defaulted to before this release, so an
operator who deleted one of them knows what changed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L6: the reader's code default and the shipped manifests both include teams,
so the troubleshooting note no longer points at a telegram,slack default that no
longer exists.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L7: when CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES is absent from the
rendered HCC Deployment, derive sandbox-ui like the code default instead of
treating it as an empty list. An explicit value, including "", still overrides
the default, so the preflight no longer expects K8s API egress from a namespace
HCC does not grant it to.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L8: WRC's DEFAULT_EXTERNAL_EGRESS_MAX_ENTRIES is now 256, so the independent
oracle accepts policies with up to 256 accumulated entries instead of rejecting
129-256 as INVALID_PROVENANCE_LIST.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L10: add the { name } fixture, which is how an apiserver read returns an env
var whose value is empty, alongside the hand-written { name, value: '' } form.
Both resolve to no pull Secret.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
R1-L11: import the env-less config and assert that
config.allowedPluginImagePrefixes equals DEFAULT_ALLOWED_PLUGIN_IMAGE_PREFIXES
after the split/trim/filter, as the HCC test already does. A second case covers
the override parsing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@zach88 thanks for the deep pass. The blocker and every optional item are addressed on f65037cde; summary per item:

Blocker

R1-H1 — unbound PVC replace drops storageClassName — confirmed and fixed in ce76d878f. On 90892308d the unbound-claim replace sent a body without storageClassName whenever the variable was unset. The apiserver allows only nil → class on update (ValidatePersistentVolumeClaimUpdate, validateStorageClassUpgradeFromNil), and the DefaultStorageClass admission plugin runs on create only, so the replace was rejected every pass. With no class configured, converge now copies the live claim's class into the replace body, including "". Create still omits the field so admission can apply the default, and a configured class is unchanged. The new test reads back an unbound Pending claim with standard-rwo, shaped as admission leaves it; its replace mock rejects a class change the way the apiserver does. It asserts that ensurePvc converges and that the body keeps standard-rwo, and fails on 90892308d with expected false to be true. Not reproduced against a real cluster; T2 (below) runs the reconcile with the minikube class. The related pre-existing case (a configured class that differs from an unbound claim's class still fails the replace) is tracked in #1310.

Optional

R1-M1 — host-layout-merge.sh pull Secret vs the inventory order — confirmed; fixed by documenting the order (b94ac20cb). The mismatch reaches only orphan claims and Hosts without a Deployment (Hosts with a Deployment use that Deployment's own imagePullSecrets), and it is not silent: those rows report UNREADABLE … ErrImagePull|ImagePullBackOff. I did not inherit imagePullSecrets from a live Host Deployment, because that infers configuration from other objects (Deployments can disagree mid-rollout, and a namespace may have none). docs/deploy/host-pvc-layout-upgrade.md and the release note now say: if the Host image needs the clerum Secret and CONTEXT_MAPPER_HOST_IMAGE_PULL_SECRET is unset on HCC, set it to clerum on HCC before running inventory.

R1-L1 — body: the minikube overlay does pick up P3-8 — fixed in the body: P3-8 changes the overlay's effective values too (300 s → 3600 s, 128 → 256).

R1-L2 — "existing PVCs are not touched" — agreed, HCC does try the replace. The release note and body now say bound claims are skipped and an unbound claim keeps the class already set on it; the R1-H1 test pins it.

R1-L3 — builder test for explicit "" — added (ce76d878f); it fails when the !== undefined check becomes a truthy check.

R1-L4 — P3-8 not declared in deploy/base — fixed (ab23cf833): deploy/base now sets HCC_EXTERNAL_EGRESS_OVERLAP_SEC=3600, WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS=3600 and WRC_EXTERNAL_EGRESS_MAX_ENTRIES=256, equal to the code defaults; no overlay declares those keys, so minikube renders the same values.

R1-L5 — old values in the release note — added (5f1e410ea): CONTEXT_MAPPER_RUNTIME_NAMESPACES was mcp-server,mcp-host,sandbox-recipes,rpc-proxy, CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES was empty, WORKFLOW_APPROVAL_READER_ENABLED_MEDIA was telegram,slack.

R1-L6 — connect-teams.md stale default — fixed (fad4e9a28): it now says both the code default and the shipped manifests include teams.

R1-L7 — np-enforce-preflight.sh unset minimal-infra — fixed (25c5f15b0): an absent CONTEXT_MAPPER_MINIMAL_INFRA_NAMESPACES now means sandbox-ui, as in HCC (?? 'sandbox-ui'); an explicit value, including "", still overrides it.

R1-L8 — wrc-egress-proof.cjs MAX_ENTRIES — fixed (06d0babdd): 256, with a comment tying the hand-kept copy to DEFAULT_EXTERNAL_EGRESS_MAX_ENTRIES; the oracle test passes 43/43.

R1-L9 — Q-LM exit 2 wording — fixed in the body: on the base the die status was lost in the here-string, the run went on to create a check Pod with an empty image, and the exit 2 came from the apiserver rejecting it.

R1-L10 — layout test fixture shape — added (cc61ea565): the { name } case with no value key.

R1-L11 — control-api parsed allowlist default — added (f65037cde): config.allowedPluginImagePrefixes with no env set equals DEFAULT_ALLOWED_PLUGIN_IMAGE_PREFIXES, plus an override trim/filter case.

Verification on f65037cde

HCC 2791/2791, control-api 8314 passed, WRC 3275 passed, WAR 88/88, host-layout-merge.test.mjs 27/27, egress proof oracle 43/43, tsc --noEmit clean, release version validator valid, t0.sh T0_AFFECTED_CHECKS=PASS, and make minikube-t2 MINIKUBE_T2_PASS / T2_EXIT=0 (full reconcile; T1 790 tests, NP-08 and port-forwards passed).

New follow-up from this review: #1310 (configured StorageClass vs an unbound claim's class).

alfredolopez80 and others added 3 commits November 4, 2026 15:05
The unbound-claim converge replaced the live PVC with the body buildPvc
returns, which carries only HCC's name, namespace and labels. The PUT
therefore dropped what other controllers had written on the Pending
claim: the scheduler's volume.kubernetes.io/selected-node, the
provisioner annotations and the kubernetes.io/pvc-protection finalizer.
With no class configured the cluster default (WaitForFirstConsumer on
GKE) reaches this path.

Copy the live annotations and finalizers into the replace body. Labels
stay HCC-authoritative.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
np-enforce-preflight.sh read CONTEXT_MAPPER_RUNTIME_NAMESPACES from the
rendered HCC Deployment and turned an absent entry, a valueFrom entry
and a missing Deployment into an empty list. HCC uses its code default
in the absent case, and the empty list replaced the script's own
namespace list, so the K8s API and RBAC-label checks for the runtime
namespaces were skipped and still reported as passed.

Move the derivation into deploy/scripts/np-preflight-inventory.rb.
It selects the host-context-controller container by name, applies
HCC's code defaults for absent entries, and refuses a valueFrom entry,
an absent entry on a container with envFrom, and a render without the
HCC Deployment. A refusal reaches the script's existing "failed to
derive" exit 2. Renders that set both variables literally produce the
same inventory as before.

scripts/tests/test-np-preflight-inventory.sh covers seven cases,
including a drift check of the helper's defaults against
host-context-controller/src/config.ts, and runs in the shell job.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… bound

The WRC egress degradation E2E bounds its autonomous-recovery wait by
the stored DNS window and refuses a window above 10 minutes with
DNS_WINDOW_EXCEEDS_GATE_RECOVERY_BUDGET. The minikube profile used to
run WRC on its 300 s code default; with the new deploy/base entry it
rendered 3600 s, so the lane refused on every run.

Pin WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS=300 in the minikube overlay,
which the ghcr and no-uis overlays inherit. deploy/base and production
keep 3600.

Name the gate's recovery cap, slack and DNS fixture TTL, and export
them with maxSupportedExternalEgressOverlapSeconds() (505 s), without
changing the gate's behaviour. The new render-contract test
scripts/tests/test-wrc-egress-gate-overlap-render.sh checks every
minikube overlay against that bound, with a 3600 s negative control,
and runs in the deploy-manifest job.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alfredolopez80 and others added 6 commits November 9, 2026 12:00
git diff --name-only quotes a path with a non-ASCII character or a double quote,
and the quote hid the path's service, so a commit adding control-api/src/café.ts
passed with no bump. The check now reads the list with -z. The hook still skips
such a path, so the doc tells the author to set the version by hand, as for a
file moved out of a service. No tracked path, now or in dev's history, has
either character.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failure quotes versions from package.json, and the runner reads each line
starting with :: as a workflow command, so a version holding a newline printed
a second command. The check now escapes %, CR and LF in the message, as the
runner decodes them. The test sets such a version and counts one command.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pin refused any key on the Repo hygiene job beyond name, runs-on and steps,
and any checkout input beyond fetch-depth, so adding permissions,
timeout-minutes or persist-credentials: false turned the required check red.
None of the three can skip a step or turn the job green, so the pin now allows
them. if, needs, continue-on-error, strategy and a checkout ref stay refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CONTRIBUTING.md said the CI validators would catch what a bypassed hook missed,
without saying that the required check then blocks the pull request or how to
clear it. Replayed over dev's 350 first-parent merges, the check fails 65, and
every one of its 94 stayed-at pairs is a pull request that never bumped. Both
documents now name that failure and its two fixes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bound test rebuilt the window WRC stores as now + (overlap + TTL) * 1000,
a copy of packages/network-policy-core's formula. A TTL floor or a new clamp in
the core would have left the 505 s bound green while the gate refused it. The
test now asks reconcileEgressState for the entry's expiresAt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on check

A test file under packages/jwt-key-policy/ now counts in neither mode,
where the default mode used to count it. The probe pins that agreement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@zach88 round 5 handled, on 8d288c9d2. dev is merged up to d02fcb9f1, so the conflicts you saw are resolved.

Must resolve

  • R5-L1 (6322a0aac, 8d288c9d2). Confirmed, and it was wider than one direction: a test file under external-rest-api failed only the default mode, and a Dockerfile, lockfile or package.json edit there failed only --all-services. The old list is gone. The default mode now checks the services releaseManifest.ts publishes, with the same rule as the required check. The test runs both modes on 48 changes under those services and requires the same verdict. Replayed over the 350 first-parent merges of dev and 89 commits of main, no verdict changes. One difference from your note: rpc-proxy's tests do not reach its image, because its tsconfig.json excludes them and no Dockerfile copies test/.

Optional, all closed

  • R5-L2 (a2c69fc84). The check reads changed paths with -z. The hook is left alone: no such path exists in the tree or in dev's history.
  • R5-L3 (91dc684ad). CONTRIBUTING.md and the release doc say what a stayed at failure means and how to clear it.
  • R5-L4 (e0823bb52). The pin accepts permissions, timeout-minutes and persist-credentials. I did not change the PR-into-main half: the only PRs into main that touched the check were dev promotions.
  • R5-L5 (197df8583). Annotations escape %, CR and LF.
  • R5-L6 (5653def26). The bound test reads expiresAt from reconcileEgressState.
  • R5-L7. The body now says P3-4 needs manual bumps in control-api, host-context-controller and workflow-recipes.

I left the immutablePvcSpec comment nit: editing it would move HCC's version and image.

Verification on 8d288c9d2

make minikube-t2 on a fresh branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0; T1 1064/1064 tests, 132/132 files; NP-08 and port-forwards passed; Health and Playwright not run. scripts/minikube/t0.sh: T0_AFFECTED_CHECKS=PASS. The version check's test passes 320/320 on /bin/bash 3.2 and bash 5.

CI: 60/60 green on this head. One job, codex-llm-proxy, failed once on a test this PR does not touch and passed on re-run; I opened #1396 for the likely port race.

Ready for another look, please.

@zach88

zach88 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Took another pass on 8d288c9d2, round 6, scoped to the 8 own commits since b08437105 plus the dev merge f54b69d76. All 7 items from round 5 are closed and the merge didn't lose anything from either side. What's left is all Low and none of it changes runtime behavior: mostly test gaps and small doc or body wording, a lot of it in the version check.

🟡 Approve with nits

  • Must resolve before approval: 0
  • Optional / non-blocking: 9

This is my read from the review, not a formal GH approval. The version check and its test now have three fix: commits each in this PR, so I wouldn't patch them again here. R6-L1 to R6-L4 are fine as a follow-up issue, or closed with a line saying why.

Must resolve before approval

Nothing open here.

Optional (non-blocking)

You can merge without these. Follow-up or your call.

  1. [R6-L1] The annotation escape test only covers \n

    scripts/tests/test-validate-release-version-bumps.sh:170-179, against scripts/release/validate-release-version-bumps.mjs:166. 197df8583 escapes %, CR and LF, but only LF is pinned.

    Repro: removing .replaceAll('%', '%25') or .replaceAll('\r', '%0D') leaves the suite at 320/0.

    Fix idea: one version with a literal %0A and one with \r, asserting %250A and %0D.

  2. [R6-L2] The jwt-key-policy probe doesn't pin what its comment says

    scripts/tests/test-validate-release-version-bumps.sh:496-497. The comment says a test file there "counts in neither mode; index.cjs counts in both", but the probe only compares the two modes with each other, so a mistake shared by both is invisible. The sentence is also only true for *.test.*: packages/jwt-key-policy/test/fix.cjs is red in both modes, and 6 real files have that shape.

    Repro: making NOT_LOADED skip packages/jwt-key-policy/ survives the suite.

    Fix idea: an expect_run with the expected verdict per file, and tighten the comment.

  3. [R6-L3] An invalid package.json prints raw :: lines and no ::error::

    scripts/release/validate-release-version-bumps.mjs:76-79. JSON.parse throws uncaught, so Node dumps the fragment outside annotationMessage. This is the same class as R5-L5. It already happened for the 2 services in the base, and this PR extends it to the 13 in a required check. The impact is small: the job stays red, and a PR author already controls what the script prints.

    Repro: with {\n::warning::pwn in control-api/package.json, the output has ::warning::pwn at the start of a line and no ::error::.

    Fix (tried on a throwaway copy): wrap the parse in try/catch, emit ::error::<path> at <ref> is not valid JSON and exit 1. That gives exit 1, one ::error:: and no ::warning.

  4. [R6-L4] releaseManifest.ts isn't really the source of the default mode's services

    scripts/release/validate-release-version-bumps.mjs:37,44 and scripts/tests/test-validate-release-version-bumps.sh:449-451,473-477. inReleaseManifest is a hand-written flag and the test compares against the literal 'external-rest-api rpc-proxy ', so the comment "a service added to it is probed here with no edit" isn't true.

    Repro: adding a service to MANIFEST_FIELD_BY_PACKAGE in the hook plus a field in the manifest leaves the suite at 320/0 with the check not covering it.

    Fix idea: derive the expected list from the hook's map or the manifest's *Version fields, and fix the comment.

  5. [R6-L5] The WRC bound test still hand-writes its inputs

    scripts/tests/wrc-egress-gate.test.cjs:293-308. 5653def26 reads expiresAt from the core, which is a real improvement, but the observation and overlapMs are still built by hand, and in production they come from WRC. The fixture also has protocol: 'TCP', which externalEgressAccumulator.ts:143-149 never emits (it doesn't change expiresAt).

    Repro: ttlSeconds * 2000 in the accumulator and externalEgressOverlapSeconds * 2000 in the reconciler both pass this test. It would only show up later in the E2E as DNS_WINDOW_EXCEEDS_GATE_RECOVERY_BUDGET.

    Fix idea: drop protocol. The rest is fine to close with a line, since the test is .cjs and the producer is TS.

  6. [R6-L6] With CONTEXT_MAPPER_RUNTIME_NAMESPACES unset, HCC stops creating deny-all-mcp-host

    host-context-controller/src/config.ts:663, networkPolicyReconciler.ts:638. An operator doing a fresh install with their own manifests, without that variable and without deploy/base/mcp-host/networkpolicies.yaml, ends up with that namespace having no default deny. Anyone on deploy/base is fine, and upgrades keep the policies they already have. Only read in the code, not run.

    Listing mcp-host again would break startup on the ownership conflict, so I wouldn't change the default. One line in the release note about fresh installs without deploy/base is enough, or close it as accepted.

  7. [R6-L7] The release doc only names non-ASCII paths and "

    docs/deploy/releases-and-upgrades.md:58. With -z the check also sees paths with \, tab or newline, and those go from pass to fail under --all-services. "Any path git would quote" covers it.

  8. [R6-L8] The body still says 48 probes

    Body line 270 ("48 changes under those services"). The head runs 49: 47 under external-rest-api/ and rpc-proxy/, and 2 under packages/jwt-key-policy/.

  9. [R6-L9] The body points to a section that no longer lists commits

    Body line 8 ("see 'To undo one item' below"). That section doesn't enumerate commits anymore, and the original ones (1fddc5aa2, 3084b7c99, ...) aren't anywhere in the body.

Tiny one, not numbered: .github/workflows/ci-public.yml:988-989 lost the blank line between two steps in the merge.

What checked out clean

  • R5-L1: both modes agree on all 49 probes, and mutants that split the rules go red (13, 6 and 1 failures). Your replay holds: 363 first-parent commits on dev and 89 on main, zero verdict changes. You were right about rpc-proxy: tsconfig.json excludes its tests and the Dockerfile only copies src.
  • R5-L2, L3, L5, L6, L7: closed. café.ts and a"b.ts now fail as they should, the stayed at path works end to end, the annotation is one command, and the bound test goes red with a TTL floor that the old form missed.
  • R5-L4: the hardening keys pass, and if, needs, continue-on-error and ref are still rejected. On the PR-into-main half, agreed: only dev promotions and the initial commit ever touched the check on main.
  • Merge f54b69d76: no test names lost in App.deepLinks.test.tsx and both sides kept in ci-public.yml. The version bumps are right: control-api 0.1.941, HCC 0.1.426, WRC 0.1.492, each above dev with a matching lockfile. The check exits 0 in both modes, and the branch merges cleanly with dev.
  • Contract: kubectl kustomize of base and the 4 minikube overlays lines up. Undoing each default by hand turns its parity test red, 11 out of 11. Old images accept the new values.
  • Body: the 14 citations at 562d2e480 are correct, all 64 SHAs are ancestors of the head, and reverting by merge or squash comes back clean to the base.
  • Security: nothing new. No pull_request_target, no ${{ }} over PR input, execFileSync without a shell, and the allowlist prefix boundary holds.
  • Suites on this head: version check 320/0, wrc-egress-gate 39/39, App.deepLinks 108/108, NP preflight 15/15, host-layout-merge 33/33, CDN cleanup 121/121. CI is green.

alfredolopez80 and others added 3 commits November 10, 2026 09:58
Round 6 review of #1305:
- R6-L6: the release note says a fresh install that does not use
  deploy/base and leaves CONTEXT_MAPPER_RUNTIME_NAMESPACES unset gets no
  deny-all-mcp-host from HCC, and how to add one.
- R6-L7: the version-check doc names every path git would quote, not only
  non-ASCII characters and double quotes.
- R6-L5: the WRC bound test's observation drops `protocol`, which the
  accumulator never emits.
- Restore the blank line between two ci-public.yml steps lost in the dev merge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/base

The previous commit told operators to apply
deploy/base/mcp-host/networkpolicies.yaml. Outside deploy/base that file's
mcp-host policy has no egress exceptions and opens 443 and 80 to 0.0.0.0/0.
Say to create only the deny-all policy, and that listing mcp-host in
CONTEXT_MAPPER_RUNTIME_NAMESPACES is not a way to get it. Name control
characters in general among the paths git quotes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bf79963 told such an install to create only deny-all-mcp-host. That
policy selects every pod in both directions, so without the allow policies
it also cuts DNS for mcp-host. Under the old runtime default HCC wrote
deny-all, DNS, HCC API and Kubernetes API egress there. State that the
install gets none of them and point to the set deploy/base applies, with
the patch its mcp-host policy needs.

Also: git quotes non-ASCII characters only with its default core.quotePath,
and the paragraph no longer breaks mid-sentence.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@zach88 thanks for round 6. Handled on a925bbd76; nothing changes runtime behaviour.

Fixed here

  • R6-L5 (63c736788). The fixture drops protocol, so the observation has the accumulator's exact ok shape. The inputs are still built by hand, as you suggested closing, because the test is .cjs and the producer is TS. In mutation runs, doubling the overlap or the TTL in the core still turns it red, and doubling ttlMs in the producer is caught by externalEgressAccumulator.test.ts. Doubling the overlap where the reconciler converts seconds to milliseconds is caught by no unit test (the full workflow-recipes suite stays green); from reading the code, only the live gate check would see it.
  • R6-L6 (63c736788, bf799634c, a925bbd76). The release note says a fresh install without deploy/base and without CONTEXT_MAPPER_RUNTIME_NAMESPACES gets none of the policies HCC used to write in mcp-host (deny-all, DNS, HCC API and Kubernetes API egress), and points to the set deploy/base/mcp-host/networkpolicies.yaml holds, whose mcp-host policy needs the deploy/base exceptions patch. The default is unchanged.
  • R6-L7 (63c736788, bf799634c, a925bbd76). The doc lists the paths git quotes: a double quote, a backslash, a control character, and with git's default core.quotePath a non-ASCII character.
  • R6-L8, R6-L9. The body gives 49 probes (47 under the two services, 2 under packages/jwt-key-policy/; the test's own counter agrees) and no longer points at "To undo one item" for commits.
  • The blank line in ci-public.yml is back.

Follow-up issue

Verification on a925bbd76

make minikube-t2 on the branch-owned profile: MINIKUBE_T2_PASS, T2_EXIT=0, with its own T0 and T1 on this head (T1 1064/1064 tests, 132/132 files; NP-08 and port-forwards passed; Health and Playwright not run). Standalone scripts/minikube/t0.sh on 63c736788: T0_AFFECTED_CHECKS=PASS; 63c736788..a925bbd76 is doc-only. On 63c736788: version check 320/0 on /bin/bash 3.2 and bash 5, wrc-egress-gate 39/39, CDN cleanup 121/121.

CI: 60/60 green on this head. Build / test / typecheck (desktop-app) failed once on FilesPage.seedLocation.test.tsx, which this PR does not touch, and passed on re-run; it is tracked in #1416.

Nothing is open from round 6 on my side. If this matches your read, could you approve?

@jozer-rami

Copy link
Copy Markdown
Contributor

Review pass on a925bbd76, one major finding: merging this breaks the infra repo's CI, so "No infra edit is required" doesn't hold.

What happens. The infra CI job assembled-overlay-contract checks out this repo at dev, unpinned, and renders deploy/overlays/minikube. Then scripts/tests/test-hcc-netpol-resync-contract.py checks that HCC_EXTERNAL_EGRESS_OVERLAP_SEC, WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS and WRC_EXTERNAL_EGRESS_MAX_ENTRIES are set only in gcp-dev and gcp-prod. A hit in any other overlay is reported as leaked. This PR adds all three to deploy/base, plus the minikube WRC patch, so the minikube render now carries them.

Repro. I replayed that job's assemble, render and contract steps against infra main:

  • this repo at d02fcb9f1 (dev): exit 0
  • at a925bbd76, this head (dev + the PR, i.e. the tree after the merge): exit 1
::error::minikube: HCC_EXTERNAL_EGRESS_OVERLAP_SEC leaked ([('host-context-controller', '3600', True)]); enabled only in gcp-dev, gcp-prod
::error::minikube: WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS leaked ([('workflow-recipes', '300', True)]); enabled only in gcp-dev, gcp-prod
::error::minikube: WRC_EXTERNAL_EGRESS_MAX_ENTRIES leaked ([('workflow-recipes', '256', True)]); enabled only in gcp-dev, gcp-prod

Impact. Once this merges, that job goes red on every infra PR, every push to infra main and the daily drift run, until the contract changes. Deploys don't run this contract. The gcp-dev and gcp-prod renders are byte-identical before and after this PR, so nothing changes at runtime.

Ordering. The infra job renders dev. A contract that expects the new minikube values fails until this PR merges, and the current contract fails after. Suggested sequence:

  1. Infra PR: accept both states for minikube, either absent or exactly 3600 / 300 / 256. Update the minikube leak case in test-hcc-netpol-resync-contract-cases.py in the same PR.
  2. Merge this PR.
  3. Tighten the infra contract to the new values.

Dropping the entries from deploy/base isn't the fix, since they were added for R1-L4. On this PR, only the body needs to change: "No infra edit is required" and "Follow-ups after the release".

@zach88

zach88 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Took another pass on a925bbd76, round 7, scoped to the 3 doc commits since 8d288c9d2 (no dev merge; still merges cleanly with d02fcb9f1). All 9 items from round 6 are settled: R6-L5 to R6-L9 are done, and R6-L1 to R6-L4 being in #1417 works for me. No runtime code changed this round. What's left is a merge-order problem with the infra repo, the body line that would lead to the wrong order, and one sentence in the release note.

🔴 Not ready to approve

  • Must resolve before approval: 2 (one High, which needs no code change, only merge order, and one Low that's a regression from a925bbd76)
  • Optional / non-blocking: 3

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

Must resolve before approval

  1. [R7-H1] Merging this before the infra contract changes turns the infra repo's CI red

    I reproduced what was reported earlier today. The infra job assembled-overlay-contract checks out this repo at dev with no pin, renders deploy/overlays/minikube, and its test-hcc-netpol-resync-contract.py only allows HCC_EXTERNAL_EGRESS_OVERLAP_SEC, WRC_EXTERNAL_EGRESS_OVERLAP_SECONDS and WRC_EXTERNAL_EGRESS_MAX_ENTRIES in gcp-dev and gcp-prod. This PR puts all three in deploy/base/control-plane/host-context-controller.yaml and deploy/base/control-plane/workflow-recipes.yaml, plus deploy/overlays/minikube/patches/wrc-external-egress-overlap.yaml:28-29, so the minikube render now carries them.

    Repro (render + that contract script against infra main): this repo at d02fcb9f1 exits 0. At a925bbd76 it exits 1 with three ::error::minikube: ... leaked lines. It's the same with kubectl 1.31.3 and 1.34.1. Every other contract in that job passes on both shas, and the gcp-dev and gcp-prod renders are byte-identical before and after.

    Impact: no runtime or deploy effect. The deploy workflows pin base_sha and don't run this contract, and the check isn't required to merge there. The real cost is that the job stays red on every infra PR, push and daily run, so it stops catching real dev drift, and the steps after the contract in that job don't run.

    Fix: no code change here. Dropping the entries from deploy/base would undo R1-L4. The infra-side PR that accepts both minikube states (absent, or exactly 3600 / 300 / 256) is already open and green: against this head it exits 0 with 11/11 cases. Order: that one first, then this PR, then tighten the infra contract to the new values.

  2. [R7-L1] The mcp-host note points at a file that, applied on its own, opens 443/80 to 0.0.0.0/0 (regression from a925bbd76)

    docs/deploy/releases-and-upgrades.md:352, and the same sentence in the body. bf799634c had the warning "do not apply the whole file outside deploy/base, it opens 443 and 80 to 0.0.0.0/0". a925bbd76 swapped it for "its mcp-host policy gets its egress exceptions from a deploy/base patch". The critique of bf799634c is right (deny-all alone cuts DNS), but the warning went with it, and that matters more:

    • Applied as is, deploy/base/mcp-host/networkpolicies.yaml:107-129 gives the mcp-host policy 443/80 to 0.0.0.0/0 with except: []. Through kubectl kustomize deploy/base the same policy comes out with 18 except ranges (RFC1918, 169.254.0.0/16, CGNAT, ...). So an operator who follows the note outside deploy/base gets Host pods that can reach the cloud metadata endpoint and in-cluster 80/443. Before this PR, HCC gave that install shape deny-all, DNS, HCC API and K8s API, with no internet.
    • It isn't a patch: it's the replacements: block in deploy/base/kustomization.yaml:22,45-50, fed from deploy/base/public-egress-exceptions.yaml. There's no patches: in deploy/base. "Exceptions" also reads like extra allows, when they're exclusions.
    • allow-k8s-api-egress-mcp-host ships with the example CIDR 10.109.0.1/32 (line 150), which has to be patched to the real API server IP.

    Read, not exploited: the YAML and the render are real. The app-level SSRF guard still stands, but the NP was the second layer.

    Fix idea: one or two sentences. Apply the file through a kustomization that carries public-egress-exceptions.yaml and the replacements (or fill except with those ranges), patch allow-k8s-api-egress-mcp-host with your API server IP, and keep the "applied as is, it opens 443/80 to 0.0.0.0/0" warning. Same change in the body line.

Optional (non-blocking)

These can merge as they are. Follow-up or your call.

  1. [R7-M1] The body says "No infra edit is required for this PR"

    "Follow-ups after the release", first bullet. Per R7-H1, an infra edit is required and it has to land before the merge, so this line is the one that would lead someone to merge in the wrong order. Something like "the external deployment repository must first accept both minikube states; tighten it after this merges" would do. Keep it generic, since this repo is public. If you fix it along with R7-H1, both close together.

  2. [R7-L2] "gets none of the policies HCC used to write in mcp-host" is a bit too broad

    Same line, docs/deploy/releases-and-upgrades.md:352. HCC still writes the context-allow <name>-egress policies (host-context-controller/src/networkPolicyReconciler.ts:941-946) and the per-Host ingress ones there, regardless of the runtime list. The list in parentheses is right and complete against d02fcb9f1. Fix idea: "none of the four namespace-wide baseline policies (deny-all-mcp-host, allow-dns-egress-mcp-host, allow-hcc-api-egress-mcp-host, allow-k8s-api-egress-mcp-host)".

  3. [R7-L3] The body's infra follow-up says "one comment" still has the old max-entries default

    "Follow-ups after the release", second bullet. There are at least six comments in that repo that describe the old defaults in the present tense: the WRC max-entries and overlap patches in both gcp overlays, plus the contract script itself. "Several comments" would keep the cleanup from missing most of them.

Small nit, no need to track: the a2c69fc84 entry in the body still says "non-ASCII or double quote". It's historical and not wrong, since no tracked path has any of the quoted shapes, but it now reads narrower than the doc.

What checked out clean

  • Round 6 items:
    • R6-L5: the fixture now has the exact shape the accumulator emits (workflow-recipes/src/reconciler/externalEgressAccumulator.ts:141-147). Your mutation claims hold: core overlapMs * 2, ttlMs * 2 and ttlMs = 0 go red, and the accumulator ttlSeconds * 2000 goes red in its own test. I'm fine with the inputs staying hand-built (.cjs test vs TS producer).
    • R6-L7: the quoted-path sentence is exact. I checked it with real git under core.quotePath true and false: ", \, control chars and DEL are always quoted, space and ' never, non-ASCII only with the default. The hook does miss those (scripts/precommit/bump-staged-package-versions.mjs:58,153).
    • R6-L8 and R6-L9: done in the body.
    • The superseded "create only deny-all" advice doesn't survive anywhere.
  • The policy list in the mcp-host note matches what HCC wrote at d02fcb9f1 (ensureDefaultDeny and ensureInfrastructurePolicies).
  • ci-public.yml: the YAML parses identically on both sides, and the pin on Repo hygiene still passes. CI is still pull_request, with no secrets in the PR's jobs and no ${{ }} over PR text inside run:.
  • wrc-egress-gate.test.cjs 39/39, test-np-preflight-inventory.sh 15/15.
  • Infra side: every other contract step passes on both shas, np-preflight-inventory.rb prints the same as the old inline Ruby for gcp-dev and gcp-prod, and the gcp renders don't change.

@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@jozer-rami thanks, confirmed. Replaying that job's assemble, render and contract steps against infra main, the contract exits 0 with this repo at d02fcb9f1 and 1 at a925bbd76, with the same three leaked errors for minikube.

  • The infra change that accepts both minikube states (absent, or exactly 3600 / 300 / 256), with the minikube leak case updated in the cases file, is open in the infra repository and green.
  • deploy/base keeps the three entries. The body's "Follow-ups after the release" now gives the order instead of "No infra edit is required": merge the infra change, then this PR, then tighten the infra contract to the new values.
  • No code change here, so the T2 on a925bbd76 still stands.

@alfredolopez80

alfredolopez80 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor Author

@zach88 on R7-H1: this is the merge order, and the infra change is ready for you to test before this PR merges.

  1. Merge the open PR in the deployment repository first. It is at 461eebb and makes the infra contract accept both minikube states: absent, or exactly 3600 / 300 / 256.
  2. Then merge this PR.
  3. Then a new PR in that repository, not opened yet, makes the three values required in minikube and removes the transition.

Since the head you tested, that PR added a test that pins the accepted minikube values and a comment that states what the contract's env walker checks and what it does not see. The contract logic did not change.

Its cases run 12/12 with EXIT=0, and its CI is green. Against this PR's head a925bbd76 the contract exits 0. The current main contract exits 1 there, with the three leaked errors.

a925bbd replaced the warning about this file with a note about its
egress exceptions. Applied as is, the mcp-host policy in
deploy/base/mcp-host/networkpolicies.yaml opens 443 and 80 to 0.0.0.0/0
with an empty except list; deploy/base fills that list through a
replacements entry, not a patch. Restore the warning, say how to apply the
file, and say that allow-k8s-api-egress-mcp-host ships with an example IP.

Also name the four namespace-wide baseline policies: HCC still writes the
context-allow and per-Host policies in mcp-host.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@zach88 thanks for round 7. R7-L1 and R7-L2 are fixed in b0a64e495, which changes only docs/deploy/releases-and-upgrades.md.

  • R7-H1: no code change here; the merge order is in my comment above. The PR in the deployment repository is waiting for its review.
  • R7-L1: the note again says not to apply deploy/base/mcp-host/networkpolicies.yaml as is, because its mcp-host policy opens 443 and 80 to 0.0.0.0/0 with an empty except list. It now names the replacements entry fed from deploy/base/public-egress-exceptions.yaml, says to apply the file through a kustomization that carries both or to fill except with those ranges, and says to patch the example address in allow-k8s-api-egress-mcp-host.
  • R7-L2: the note names the four namespace-wide baseline policies.
  • R7-M1: the body already gives the order.
  • R7-L3: the body now says several comments.
  • The a2c69fc84 entry stays as the historical record.

scripts/minikube/t0.sh on b0a64e495: T0_AFFECTED_CHECKS=PASS. I did not repeat T2: a925bbd76..b0a64e495 changes only that doc, and the T2 pass on a925bbd76 stands.

Could you take a closing look and approve if nothing else is open?

@jozer-rami jozer-rami left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving b0a64e495, on one condition: merge the infra repo's contract PR (461eebb) before this one. If this one merges first, the infra repo's assembled-overlay-contract job goes red.

The major from my earlier comment is fixed.

  • "Follow-ups after the release" now gives the merge order instead of "No infra edit is required".

  • I re-ran the infra contract against this repo before and after the PR:

    this repo infra main infra PR 461eebb
    dev d02fcb9f1 exit 0 exit 0
    this head b0a64e495 exit 1 (the three leaked errors) exit 0
  • The infra PR's test cases pass 12/12. The new check is tight: it rejects a wrong value, an unquoted value, a duplicate, and the value on another container.

  • dev hasn't moved, so this head is still the tree after the merge.

Before merging: Build / test / typecheck (desktop-app) failed on b0a64e495 in FilesPage.seedLocation.test.tsx, the flake tracked in #1416. a925bbd76..b0a64e495 changes only a doc, so a re-run should clear it.

Non-blocking. These can go in a small commit or a follow-up:

  1. "Effect per consumer" leaves out managed tenants. The tenant stack builds its own WRC ConfigMap and sets neither WRC egress variable, while tenant HCC already runs HCC_EXTERNAL_EGRESS_OVERLAP_SEC=3600. Once a release with this PR reaches tenants, their WRC moves from 300 s / 128 entries to 3600 s / 256. That is probably what you want, since it matches WRC to HCC. Still, it changes tenant egress, and neither the body nor the release note mentions it.
  2. The release note understates the egress window. docs/deploy/releases-and-upgrades.md:351 says a rotated-out IP "stays allowed for at most that hour". The code (packages/network-policy-core/index.cjs:289) computes expiresAt = now + min(ttl, 24h) + overlap, so the upper bound is the record's TTL, capped at 24 h, plus the hour. The body's "after its DNS TTL for up to one hour" is right.
  3. The stayed at remedy blames skipped hooks only. docs/deploy/releases-and-upgrades.md:79-82 and CONTRIBUTING.md:90-96 give a commit made without the hook as the usual cause. The hook also skips the bump when the staged version differs from HEAD (scripts/precommit/bump-staged-package-versions.mjs:203), and resolving a dev merge can leave a branch in that state (#1166). Both docs should name that cause and link #1166, because the check now covers every service.
  4. No replace-path test for a configured "" StorageClass. The only configured-"" case is the builder test. In a mutation run, changing the guard at host-context-controller/src/hostReconciler.ts:4141 from === undefined to !config.hostWorkspaceStorageClassName kept the HCC suite green. With that change, HCC copies the live class over an operator's explicit "". A test beside the #1310 case, with "" configured and standard-rwo live, should expect storageClassName === ''.
  5. The control-api and WAR parity tests read only the ConfigMap. control-api/test/config.deployBaseDefaults.test.ts:11-18 and workflow-approval-request-reader/test/config.deployBaseDefaults.test.ts:19-22 stay green if the deploy/base Deployment gains an env entry, which overrides envFrom. The HCC test's extra assertions would close that gap: envFrom references the ConfigMap, and no env entry names the variable.

alfredolopez80 and others added 4 commits November 11, 2026 13:12
…aults

* origin/dev:
  ci: gate React bundled inside next, lockfile advisories and Next.js images
  fix(deps): upgrade Next.js 16.3.5 -> 16.4.6 in control-ui and profile-ui
deploy/base sets CONTROL_API_PROXY_TIMEOUT_MS to 300000 and every
deployment inherits it, while the route handler defaulted to 30000. A
control-ui that runs without the variable, or with a value the handler
rejects, cut body-bearing proxy calls at 30 s.

The default is now 300000. The fallback for an absent, non-integer,
non-positive or out-of-range value uses the same constant, so it moves
too. The browser's own 30 s request limit is unchanged.

Tests: a new case reads the deploy/base value and checks the route arms
the timeout with it when the variable is unset; the override case now
uses 45000 so it still proves the variable is read. Reverting the
constant to 30000 fails both default cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of acf5d49. The control-ui README and the control-api gfsc
timeout comment still gave the old 30 s route default. The release note
and docs/surfaces/control-ui.md now say the timeout covers every method
except GET and HEAD, that GFS upload-v2 parts use their own timeout, and
that values above 2147483647 also fall back to the default.

The GET case of the proxy route test now asserts the request was
proxied, so its "no timeout" assertion cannot pass on a skipped path,
and it sets a value different from the default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aults

dev b6db9c2 raised the sharp override to 0.35.5 in control-ui and
profile-ui. The only conflict was the control-ui version line: this
branch had 0.1.764 and dev 0.1.763. The merged control-ui takes dev's
package.json and lock and sets 0.1.765, above both parents.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alfredolopez80

Copy link
Copy Markdown
Contributor Author

@jozer-rami @zach88 every precondition for this PR is met. Could you take a last look and approve?

  • The contract change in the deployment repository is merged, so the order agreed in R7-H1 holds: that change first, then this PR.
  • dev moved three commits, merged here as 3602cb330 (up to 18052d9ce) and 3f1ff4819 (b6db9c297, the sharp 0.35.5 override). The only conflict was the control-ui version line, now 0.1.765. Blind reviews of both merges found nothing.
  • P3-13 (acf5d4994, review fixes in 6e54c106d): control-ui's CONTROL_API_PROXY_TIMEOUT_MS code default now matches the 300000 that deploy/base sets. It needs no change in the deployment repository.
  • make minikube-t2 passed on 6e54c106d (MINIKUBE_T2_PASS, T1 1064 tests, NP-08 and port-forwards passed). 3f1ff4819 adds only dev's b6db9c297 and the version line; scripts/minikube/t0.sh passes on it.
  • CI on 3f1ff4819 is pending. Since 14:10 UTC today the Node image runtime (host-desktop) jobs fail on every PR because GitHub runners cannot resolve the Ubuntu and Launchpad package hosts (Could not resolve 'archive.ubuntu.com', 'ports.ubuntu.com'); I'll re-run them once those hosts answer.
  • Codex and Grok enabled by default (P3-12) goes to its own PR, because it needs a matching change in the deployment repository's contracts.

@alfredolopez80
alfredolopez80 merged commit 72fe1c0 into dev Nov 11, 2026
63 checks passed
@alfredolopez80
alfredolopez80 deleted the fix/env-audit-code-defaults branch November 11, 2026 17:57
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.

3 participants