Skip to content

fix(config): name the refused field for repository delivery settings - #3270

Merged
freeqaz merged 4 commits into
mainfrom
fix-transport-field-errors
Oct 23, 2026
Merged

freeqaz merged 4 commits into
mainfrom
fix-transport-field-errors

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Two repository-delivery startup refusals were plain Errors. API and worker startup therefore logged only STARTUP_FAILED, and the chart's upgrade check printed no field:

Change

  • validateOpenShellRepositoryTransport throws InstallationFieldError with one detail per unmet requirement. Each detail has a JSON Pointer and a schema code, and its message never echoes a value. Details are bounded as in fix(egress): name the refused field in an authority block #3260. The pointers are:

    • /repository/preparedGitHub
    • /backend/<i>/configuration/endpoint (must be an https:// URL)
    • /backend/<i>/configuration/auth or .../auth/mode (must be bearerTokenFile or mtls; mtls when observedHookPreviewWorkspace is set)
    • /drivers/sandbox or /drivers/sandbox/package
    • /drivers/compute
    • /backend, when the github or openshell entry is missing

    loadInstallationConfiguration passes the OpenShell Backend's index so the pointer names the right backend entry.

  • The native peer refusal is an InstallationFieldError: REQUIRED at /drivers/compute/configuration/network/repositoryCredentials when the peer is absent, and INVALID_VALUE at .../repositoryCredentials/port otherwise.

  • Startup and the upgrade check report both refusals as DRIVER_CONFIGURATION_INVALID through the existing startupConfigurationFailure path.

Verification

  • tests/integration/sandbox-driver-startup.test.mjs:
    • The protected OpenShell Git load test gains a case with no preparedGitHub and a plaintext, unauthenticated gateway. It asserts three details: /repository/preparedGitHub REQUIRED, and /backend/0/configuration/endpoint and /auth/mode INVALID_VALUE. It also asserts that the endpoint value is not echoed.
    • A table test covers the requirements that earlier startup checks normally catch first.
  • tests/integration/configuration-startup.test.mjs: a new test checks the missing peer and the wrong-port peer through the log filter. It also boots API and worker and asserts DRIVER_CONFIGURATION_INVALID with the field. It replaces two regex cases in the repository-startup table.
  • The new tests fail on main and pass 10/10 at 2 CPUs.
  • Full runs pass:
    • sandbox-driver-startup: 114 pass, 1 skip, the same as on main
    • configuration-startup: 41/41
    • installation-change-preflight: 7/7
  • tsc --build, eslint and prettier are clean on the touched files.

Risks

Low. The same configurations are refused as before, now with field-level diagnostics. Anything that matched the old message text needs updating. Only the two table cases did, and this PR replaces them.

…port

validateOpenShellRepositoryTransport threw one message for eight unmet
requirements, so startup and the chart's upgrade check said only
STARTUP_FAILED. It now throws InstallationFieldError with one bounded
detail per unmet requirement: repository.preparedGitHub, the OpenShell
Backend's configuration.endpoint and configuration.auth, drivers.sandbox,
drivers.compute and the backend entries. Startup logs them as
DRIVER_CONFIGURATION_INVALID, never echoing values. This is the remaining
part of finding 2216.
@freeqaz
freeqaz requested a review from a team as a code owner October 23, 2026 03:53
@clawsweeper

clawsweeper Bot commented Oct 23, 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.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 23, 2026
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR identifies refused repository delivery settings in API, worker, and installation-check diagnostics.

Example: Native repository delivery is selected without drivers.compute.configuration.network.repositoryCredentials.

  • Before: API startup reports STARTUP_FAILED without naming the missing setting.
  • After: API startup reports DRIVER_CONFIGURATION_INVALID with REQUIRED at /drivers/compute/configuration/network/repositoryCredentials.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported repair with meaningful regression coverage and no identified defect; execution evidence remains test-based.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored diagnostic repair is outside the ordinary contributor-proof gate; supplied verification consists of reported test results and inspected tests exercising the production loader, formatter, and native-peer API/worker startup.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Operators cannot identify which repository delivery setting caused API or worker startup to refuse an Installation.
Reason: The collaborator-authorized diagnostic repair makes existing refusals actionable while retaining the supported configuration and security boundaries.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it fixes two diagnostic gaps still present on main, preserves existing validation, and introduces no identified correctness or security defect.

Priority: P2
Reviewed head: 58162450c99e42de058e5f96f85b89648083d722

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No separate runtime artifact demonstrates the OpenShell multi-field refusal through API, worker, or Helm, and the reported failing-base/passing-head comparison was not independently executed.
Agent review details

How this fits together

Installation composition validates operator configuration before startup; the shared failure mapper and log filter turn refusals into API, worker, and Helm installation-check diagnostics.

flowchart TB
  A[Installation configuration] --> B[Repository delivery validation]
  B --> C[Installation field error]
  C --> D[Startup failure mapping]
  D --> E[Filtered diagnostics]
  E --> F[API and worker logs]
  E --> G[Helm installation check]
Loading

Technical review

Best possible solution:

Keep the existing validation owners and emit bounded, value-free field diagnostics through the shared startup failure pipeline.

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

Current-main source shows both plain errors bypassing typed configuration diagnostics, while the added scenarios provide concrete invalid configurations; no target code was executed during this review.

Is this the best way to solve the issue?

The patch repairs the existing validation owners, preserves their acceptance rules, and reuses the established diagnostic pipeline without adding another configuration mechanism.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 38ecfca18762.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Review metrics

Metric Value Why it matters
Change footprint Production +85/-20 lines; tests +177/-8 lines The growth replaces combined refusals with individual diagnostics and adds loader, filtering, and startup-process coverage.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This focused operator-diagnostic repair identifies already-refused configuration without changing transport admission or authorization.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-23T04:06:32.275Z sha 54711dc :: needs maintainer review before merge. :: none

Reviewed October 23, 2026, 12:30 AM ET / 04:30 UTC (Revision 2).

With repository.nativeSender, a Compute network.repositoryCredentials peer
that is missing or not on port 8443 was a plain Error, so API and worker
startup logged only STARTUP_FAILED (finding 2227). It is now an
InstallationFieldError: REQUIRED at the peer when absent, INVALID_VALUE at
its port otherwise. The new configuration-startup test covers both and the
two regex cases it replaces.

Review: move the OpenShell transport table test above the comment that
heads the next test, label its two /backend details, and say serviceName
and port do not stand in for an https endpoint.
@freeqaz freeqaz changed the title fix(config): name each refused field for a repository OpenShell transport fix(config): name the refused field for repository delivery settings Oct 23, 2026
Internal review nits: the replaced table case used a port below 8443, so
keep that setup; compare the booted processes' full detail as #3268's
test does; pin the endpoint REQUIRED wording.
@freeqaz

freeqaz commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 23, 2026
@freeqaz
freeqaz merged commit 4936991 into main Oct 23, 2026
66 checks passed
@freeqaz
freeqaz deleted the fix-transport-field-errors branch October 23, 2026 04:48
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: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant