Repository navigation
Fix review findings: 204 void contract, redaction gaps, glob-anchored name matching - #1114
Merged
Merged
Conversation
TheGreatAxios
marked this pull request as draft
October 9, 2026 02:48
TheGreatAxios
added a commit
that referenced
this pull request
Oct 9, 2026
Fixes an eslint unused-variable error that turned CI red on PR #1114; the removed constant was a named fixture that the tests never referenced.
TheGreatAxios
marked this pull request as ready for review
October 9, 2026 05:27
Provider keys, PATs, free-text keyword+space secrets, key-named JSON fields, and stringified-object arguments all slip into error-sink and approval headlines unredacted. These tests pin the redaction behavior so the follow-up implementation lands against a failing test.
Expand redactText's provider-key detection to the three distinct shapes (underscore PATs, segmented OpenAI-style keys, and bare sk-/pk-/rk- keys) without eating ordinary hyphenated words or paths. Add a keyword+space pattern so a bare secret-shaped run after password/secret/token/api_key in free text is masked. Export redactText and redactExtra from the package index so consumers can redact structural and free-text values.
The approval title is free-form caller input and the write_file content preview / generic argument bag can carry credentials; route them through error-sink's redactText and redactExtra so key-named and string-embedded secrets are masked from any headline a human sees. Adds the @corbits/error-sink workspace dependency approvals now imports.
The single-step, mail-triggered worker shape is deployment contract: a second step trades away the warm agent's durable memory and an awaitSignal gate parks the run forever. Pin buildWorkerWorkflow's output so the deploy path cannot reintroduce the broken shape.
DELETE/role-assign/inference routes return nothing, so reading and schema-validating a body was unsound . Add a dedicated requestVoid helper (in api-request, tenancy-api, and inference api) that fails loudly if a void route answers with any 2xx other than 204, keeping the 204 contract in force instead of silently accepting a drifted body. Also report shadow-offering rollback failures through error-sink instead of swallowing them.
DB-gated suites must only ever run against the derived _e2e sibling database, never a developer's own DATABASE_URL. dbGate now redirects the resolved URL onto the _e2e database (propagating it back onto process.env.DATABASE_URL so bootHub and direct reads hit the same DB) and refuses to proceed if that redirect ever lands on a non-_e2e name. baseUrlToE2eUrl becomes idempotent so a prior redirect is not stacked.
Suites that create tenants/principals/roles over stock HTTP routes now register an afterAll that deletes exactly the tenants they made, so reruns stop accumulating rows in the shared _e2e database. Suites that open the database directly resolve it through e2eDatabaseUrl instead of the raw DATABASE_URL, and the browser suites assert the second form field exists before typing into it instead of silently skipping.
…e state dirs Hold the concurrency resolver in one module and drop the unused script argument from its signature. Cap at every available core on CI and leave a couple free locally for the editor, rather than always subtracting two. resetLocalState now clears the schema before removing hub state dirs, so a stale DB never survives the reset.
The custom base URL error now shows for an empty value once the custom URL toggle is on, instead of only for a non-empty invalid value, so a blank base URL no longer presents as valid.
The previous implementation spread a .* wildcard between every character and dropped the * entirely, so exa.* matched names with no literal dot and memory_* matched prefixed names. Escape every regex metacharacter except *, then let * be a glob for any run of characters, anchoring the pattern at both ends. Also strip two embedded NUL bytes the old wildcard trick left in the source.
…stVoid, tenant teardown Warden regression tests for real-world provider key formats (Anthropic, OpenAI service-account, data-control, Stripe, Gemini) plus pure-alpha uppercase free-text secrets and provider-key-shaped object keys. The credential-shaped fixtures are stamped at runtime from benign parts so no full provider key string exists contiguously at rest; each value must be absent post-redaction. RequestVoid variant tests for the tenancy-api and inference/api seams, pinning literal-204-in/2xx-drift-out plus the 401/403 and fetchImpl paths, mirroring the api-request helper tests. Mailbox persist and send-terminal suites now tear down the tenants they insert, via e2e/lib/tenant-cleanup like the grants/roles suites.
… the e2e sibling Redaction: extend the error-sink provider-key matcher to Anthropic, OpenAI service-account, data-control, Stripe underscore, and Gemini key formats without regressing the over-redaction guards (pk-manifest.yaml, sk-parallel-copy, pk-abcdefgh, ghp-garbage-collector still pass). Drop the digit-only requirement in the free-text keyword+space pattern so a long pure-uppercase run redacts too, and redact any field whose key name itself is provider-key shaped. In approvals/headline, run the workflow_deploy line through redactText and route the free-form title through the same structural redaction as generic arguments, so JSON-in-title passwords are redacted rather than passing the flat assignment pass. The write_file content preview already redacts before truncating. DB gate: the fail-closed gate redirects DB-gated suites to the derived _<database>_e2e sibling, but nothing provisioned it. Add scripts/docker-e2e-init.sh mounted as an initdb.d script in both CI and compose postgres services to create the sibling on first boot, and have scripts/db-setup.ts derive and ensure it alongside the base database. Add a DB-gated reachability probe asserting the sibling answers SELECT 1 and failing with a provisioning hint instead of a raw connect error.
… reachable The gate reachability probe now also detects schema presence: it checks the platform 'tenant' table exists via to_regclass, alongside SELECT 1 connectivity, and fails with a provisioning hint naming db-setup and docker-e2e-init instead of a raw connect/schema error. This pins that a fresh _e2e sibling is migrated, so direct-DB mailbox suites are not order-dependent on a hub-booting suite migrating first. Add unit tests for db-setup's dbTargetFromUrl and e2eSiblingTarget pure derivations.
Provisioning created the _e2e sibling but left its schema empty, so the direct-DB mailbox suites (insert tenant/principal with no hub boot) failed 42P01 until some hub-booting suite migrated it first — order-dependent and flaky in a fresh CI. Extract a migrateTarget helper (create-if-missing then run migrateHub) shared by setupDatabase and the sibling path, and have the db-setup CLI migrate the sibling alongside the base database. Add a db-setup step before test:e2e in CI so DATABASE_URL's db and its _e2e sibling are both created and migrated before any suite runs.
Fixes an eslint unused-variable error that turned CI red on PR #1114; the removed constant was a named fixture that the tests never referenced.
TheGreatAxios
force-pushed
the
cl-10203-review-fix-settings-redaction-e2e
branch
from
October 9, 2026 05:29
1c88d82 to
c71eae3
Compare
…res service GitHub Actions provisions service containers BEFORE the checkout step, so the volumes source path does not exist yet and Docker auto-creates an empty DIRECTORY at the mount destination; pg initdb then fails with 'Is a directory' on every shard, deterministically red (not a flake). Remove the volumes block from the e2e-browser job's postgres service — browser/mcp suites never reference the _e2e sibling, and the _e2e provisioning is covered by the hub job's 'bun scripts/db-setup.ts' step. Delete the now-unused scripts/docker-e2e-init.sh and its references in compose.test.yml and the gate-reachability probe's wording.
Contributor
|
@TheGreatAxios, please address the following:
|
…tup hanging The browser/mcp suites set DATABASE_URL and go through dbGate, which redirects onto the derived _e2e sibling, so every browser shard needs workbench_e2e provisioned — but it only existed when the (now-removed) initdb mount created it. Add 'bun scripts/db-setup.ts' (idempotent: CREATE IF NOT EXISTS + migrateHub) to the e2e-browser job, before the sharded test loop, mirroring the hub job. Also fix db-setup hanging in CI after migrations: migrateTarget opened a drizzle pool via createDB that held the event loop open and never closed, so the script printed migration NOTICEs then stalled for the full job timeout. Close the pool in a finally and exit explicitly at the end of the CLI, so it completes and terminates within seconds (verified locally against a live Postgres: provisions and migrates both workbench and workbench_e2e, exits 0).
…CT teardown The install package seeder suite tears every tenant it creates down after run, but deleting a tenant used to trip on workflow_run_launch_spec's source_authority_principal_id (RESTRICT on principal): the launch spec has no tenant_id of its own, so cleanupTenants could not reach it and the tenant delete failed with 'violates RESTRICT setting of foreign key constraint workflow_run_launch_spec_source_authority_principal_id'. This suite seeds exactly that shape (tenant -> principal -> definition -> run -> launch spec) and asserts cleanupTenants removes both the launch spec and the tenant row, so the regression cannot return silently.
cleanupTenants removed a tenant's restrict- and no-action-on-tenant rows before the tenant itself, but workflow_run_launch_spec references the tenant's principal with onDelete restrict while carrying no tenant_id of its own: it reaches the tenant only through its anchor run. Deleting the tenant tried to cascade the principal, which the launch spec's RESTRICT constraint blocked, so install-package and any seed suite that deploys failed its afterAll teardown with 'violates RESTRICT setting of foreign key constraint workflow_run_launch_spec_source_authority_principal_id'. Resolve the tenant's run ids, delete the launch specs they anchor, then the runs, before the tenant row's cascade reaches the principal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the CL-10203 review findings. Secret redaction now covers provider keys (PATs, segmented OpenAI-style keys, bare sk-/pk-/rk- keys) and free-text keyword+space secrets, and is applied structurally to approval headlines and their sibling render paths. Void settings routes (DELETE / role-assign / inference shadows) now require a literal
204 No Contentvia a dedicatedrequestVoidhelper and fail loudly on any drifted 2xx instead of accepting an unsound empty body. The e2e database gate fails closed: suites only ever run against the derived_e2esibling, never a developer's ownDATABASE_URL, and the seeding suites tear down the tenants they create. Name-pattern matching in deferred-tools is glob-anchored (the CL-9712exa.*/ NUL-sentinel regression), the worker definition shape is pinned against CL-9712, the base-URL empty display is fixed, concurrency resolution moves toscripts/concurrency, and reset clears the schema before state dirs.Verification
bun run typecheck— exit 0bun run lint— exit 0 (0 errors; 3 pre-existing warnings inapps/sidecar, untouched by this change)bun run fmt:check— exit 0bun test— green (error-sink 24/0, approvals 9/0, deferred-tools 11/0, worker definition-json 0 fail; combined 47 pass / 0 fail)bun run test(full e2e) has pre-existing infra failures unrelated to this change:Cannot find module '@intx/inference-testing'and a stale local DB schema (column "initialization_lease_id" does not exist) on the DB-gated suites. Noted separately; not caused by these files.Fixes CL-10203