Skip to content

fix(dev): record the canonical browser origin for port 443 - #2850

Merged
freeqaz merged 3 commits into
mainfrom
lows-1018j-a
Oct 18, 2026
Merged

freeqaz merged 3 commits into
mainfrom
lows-1018j-a

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • With OCC_DEVELOPMENT_BROWSER_PORT=443, Local Setup recorded the browser origin as https://console.<cluster>.oce.localhost:443. WHATWG URL drops the default port, so scripts/first-agent.mjs refused the recorded state ("native admin endpoint is invalid"), and the controller's development origin rule (isDevelopmentBrowserOrigin) refuses the same value as OCC_AUTH_BASE_URL. The k3d auth.baseUrl and the Keycloak redirect URI and sign-in origin were built the same way.
  • docs/guides/deploy/oidc-keycloak.md described a "proposed OCE compatibility policy" for Keycloak 26.x "pending team acceptance": an open decision in operator docs.

Change

  • internal/occdev: new developmentBrowserOrigin(consoleHost, port) returns the canonical origin (no :443). Used for the Compose state's BrowserOrigin, the k3d auth.baseUrl, the startup console line, and the Keycloak redirect URI, sign-in origin and sign-in instructions. The sign-in console client accepts a port-less origin and dials host:443 for it (Keycloak sign-in still reserves port 443, so this is for consistency).
  • scripts/first-agent.mjs: admits a port-less HTTPS origin, and also the explicit :443 form that older Local Setup state recorded. Other non-canonical forms (trailing slash, :0443) are still refused.
  • Keycloak guide: states only the facts. CI pins 26.7.5 by digest in tests/fixtures/keycloak/image.json; other versions are not verified.

Verification

  • go vet ./internal/occdev/ and go test ./internal/occdev/: pass. The new TestDevelopmentBrowserOriginOmitsTheDefaultHTTPSPort and the extended TestDevelopmentConsoleTrustsOnlyTheBrowserCA (port-less origin) pass 20/20 on 2 CPUs.

  • node --test tests/integration/first-agent-startup.test.mjs: 9/9, 20/20 on 2 CPUs. The new case fails against the old first-agent.mjs.

  • Prettier check on the touched JS and Markdown files: clean. The docs word count goes down.

  • No cluster run. Port 443 on host loopback needs a privileged or rootless-engine setup; that path was not exercised end to end.

  • Internal review: one medium and two lows, all fixed in the last commit.

Risks

  • Low. Output only changes for port 443; every other port renders the same string as before.

Local Setup recorded https://console...:443 when OCC_DEVELOPMENT_BROWSER_PORT
was 443. WHATWG URL drops the default port, so first-agent refused the state
and the controller's development origin rule refused OCC_AUTH_BASE_URL.
Record the origin without the default port everywhere it is built (Compose
state, k3d auth.baseUrl, Keycloak redirect URI and sign-in origin), and let
first-agent also admit the explicit :443 form older state recorded.
The guide described a Keycloak 26.x compatibility policy as pending team
acceptance. Keep only the facts: CI pins 26.7.5 by digest; other versions
are not verified.
@freeqaz
freeqaz requested a review from a team as a code owner October 18, 2026 13:23
@clawsweeper

clawsweeper Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

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

View the workflow run.

Review follow-up: newDevelopmentConsole required an explicit port and
compared the dial target with the origin's host. Accept a port-less HTTPS
origin and expect host:443 for it, and print the Keycloak sign-in URL with
the same canonical origin. Keycloak sign-in still reserves port 443, so this
is consistency rather than a reachable failure.
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 18, 2026 13:26 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 18, 2026 13:26 — with GitHub Actions Inactive
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 18, 2026
@freeqaz
freeqaz merged commit eec3b44 into main Oct 18, 2026
47 of 51 checks passed
@freeqaz
freeqaz deleted the lows-1018j-a branch October 18, 2026 13:40
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 18, 2026
@clawsweeper

clawsweeper Bot commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge.

What this changes

This PR canonicalizes development HTTPS origins on port 443, adjusts first-Agent admission, and clarifies the tested Keycloak version.

Example: Local Setup uses OCC_DEVELOPMENT_BROWSER_PORT=443 for cluster occ-dev-example.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused repair has useful harness coverage, with one concrete policy violation and an unresolved persisted-state compatibility gate.
Proof confidence 🦐 gold shrimp (3/6) Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Selecting development browser port 443 produces an origin rejected by the controller and first-Agent workflow.
Reason: This is a narrow correction to a supported development setting, and the collaborator-authored PR supplies the direction decision.

Merge readiness

⛔ Blocked before merge - 2 items remain

Keep this PR open: current main still has the port-443 origin mismatch, but the legacy-state exception conflicts with repository policy.

Priority: P2
Reviewed head: efeb821ee1395d3cfc4b7f42d557443d47d4178e

Before merge

  • Retain canonical-only admission for saved browser origins (P2) - For saved state containing https://console.occ-dev-example.oce.localhost:443, this new branch silently admits and normalizes the older spelling instead of rejecting unsupported state. AGENTS.md's Production and compatibility boundary explicitly prohibits retaining older persisted formats or silent fallbacks solely for backward compatibility, which is the stated purpose of this exception. The updated launcher already emits the canonical value, so retain the removal of the mandatory-port check, restore exact equality with browserOrigin.origin, and change the added test to reject explicit :443 state.
  • Add data-model compatibility proof - The review found that existing stored data may not work after upgrade. Show that existing data still loads and works with this change.

Findings

  • [P2] Retain canonical-only admission for saved browser origins — scripts/first-agent.mjs:198-200

Tests

  • Missing end-to-end proof: The supplied checks do not demonstrate fresh port-443 launcher output and existing saved-state behavior through a real supported installation.
Agent review details

How this fits together

The development launcher converts selected browser settings into saved Local Setup state, Compose configuration and Helm values; the controller and first-Agent CLI consume those values for browser admission and native UI access.

flowchart TD
  A[Development browser settings] --> B[Development launcher]
  B --> C[Saved Local Setup state]
  B --> D[Compose and Helm configuration]
  C --> E[First Agent CLI]
  D --> F[Controller origin admission]
  E --> G[Console and Agent URLs]
  F --> G
Loading

Technical review

Best possible solution:

Generate canonical origins through the existing launcher, retain exact canonical admission in first-Agent, and verify fresh and existing development state.

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

The current-main producer writes an explicit :443 while both first-Agent admission and the controller reject that spelling; this source path is clear, but no runtime reproduction was performed during this read-only review.

Is this the best way to solve the issue?

The shared origin formatter fixes the producer consistently without changing identity or transport boundaries; the additional legacy-format exception is unnecessary under the repository's canonical-state policy.

Full review comments:

  • [P2] Retain canonical-only admission for saved browser origins — scripts/first-agent.mjs:198-200
    For saved state containing https://console.occ-dev-example.oce.localhost:443, this new branch silently admits and normalizes the older spelling instead of rejecting unsupported state. AGENTS.md's Production and compatibility boundary explicitly prohibits retaining older persisted formats or silent fallbacks solely for backward compatibility, which is the stated purpose of this exception. The updated launcher already emits the canonical value, so retain the removal of the mandatory-port check, restore exact equality with browserOrigin.origin, and change the added test to reject explicit :443 state.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Compose browser-origin production and first-Agent admission keeps the original intent (21b8084: Expose the native Agent UI through the installation's exact HTTPS browser origin, as described in feat(dev): expose first-agent Control UI with OpenShell #1750.)
  • Legacy persisted browser-origin admission changes intended behavior with a stated reason (21b8084: The existing consumer requires the saved value to equal its parsed canonical origin; this PR explicitly relaxes that requirement to accept older :443 state.)
  • Kubernetes auth.baseUrl keeps the original intent (e82fa9b: Supply the Console's published loopback authentication origin while enabling the dedicated native runtime.)
  • Keycloak redirect and Console client keeps the original intent (feat(occdev): enable local Keycloak sign-in and manual launcher qualification #1138: Use the verified Console HTTPS endpoint, exact Origin, browser CA and cookie session for local Keycloak setup.)
  • Keycloak version documentation keeps the original intent (24bc585: Distinguish a proposed compatibility policy from verified fixture coverage; the revised wording retains the coverage boundary without presenting an unresolved proposal.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

  • node --test tests/integration/first-agent-startup.test.mjs
  • go test ./internal/occdev/
  • pnpm lint
  • pnpm format:check
  • pnpm check:workspace

What I checked:

Labels

Label changes:

  • add P2: The bug affects an explicitly selected development browser port; the default 8443 workflow is unchanged.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: The bug affects an explicitly selected development browser port; the default 8443 workflow is unchanged.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

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

Workflow

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

Reviewed October 18, 2026, 9:40 AM ET / 13:40 UTC.

This branch was previously deployed

1 inactive deployment
integration-qa-pr — efeb821e Deployed Oct 18, 2026 by freeqaz via QA advisory (kubernetes) #2213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant