Repository navigation
schematics: phase 0 stack (01/15) - #185
Conversation
| version_ids = [tup[0] for tup in result.all()] # result is a list of 1-tuples | ||
| matches = result.all() | ||
| found = {row.quantified_name for row in matches} | ||
| missing = sorted(requested - found) |
There was a problem hiding this comment.
I feel like there is a way to do this in SQL directly, though idk whether it would be better.
| PRIVATE_API_PATH_PREFIXES = ( | ||
| "/v1/auth/csrf", | ||
| "/v1/minecraft/auth", | ||
| "/v1/submissions/drafts", | ||
| "/v1/users/me", | ||
| "/v1/vote-sessions", | ||
| ) |
There was a problem hiding this comment.
not a good idea hardcoding paths outside of their definiton
| _BOUNDED_MEDIA_UPLOAD = re.compile( | ||
| r"^/v1/submissions/drafts/[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-" | ||
| r"[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}/media/(?:image|video)$" | ||
| ) |
There was a problem hiding this comment.
same here hardcoded paths
| @model_validator(mode="after") | ||
| def _validate_idempotency_encryption(self) -> Self: | ||
| IdempotencyEncryptionConfig( | ||
| active_key_id=self.idempotency_active_key_id, | ||
| keys=self.idempotency_keys, | ||
| ) | ||
| return self |
There was a problem hiding this comment.
hmm, not sure how I feel about constructor as validation
| if build.submission_status != Status.PENDING: | ||
| msg = "The build must be pending to post it." | ||
| raise ValueError(msg) | ||
| unique_channels = tuple({channel.id: channel for channel in channels}.values()) |
| ) | ||
| return self.assemble(records, subject) | ||
|
|
||
| def assemble(self, records: SubjectRecords, subject: Subject) -> tuple[Rule, ...]: |
There was a problem hiding this comment.
why do we have the Subject and its records? why doesnt the Subject carry the records or the records carry the subject?
| class PermissionRole(Base, kw_only=True): | ||
| """A named bundle of permission patterns. | ||
|
|
||
| Built-in roles keep their pattern lists in code, so this row exists only to |
There was a problem hiding this comment.
is this really a good idea
| CheckConstraint(_ONE_SUBJECT, name="permission_role_assignments_one_subject"), | ||
| CheckConstraint(_ROLE_SUBJECT_HAS_GUILD, name="permission_role_assignments_role_subject_has_guild"), | ||
| CheckConstraint(_ROLE_SUBJECT_STAYS_HOME, name="permission_role_assignments_role_subject_stays_home"), |
There was a problem hiding this comment.
so many conditionally nullable stuff
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class AssignmentRecord: |
There was a problem hiding this comment.
if this doesn't get renamed back to RoleRecord in later commits...
| def _role_specs(roles: Iterable[RoleRecord]) -> dict[str, RoleSpec]: | ||
| """Key stored roles by id, folding code-defined built-in patterns in. | ||
|
|
||
| A built-in's pattern list lives in `squid.permissions.domain.catalogue`, and | ||
| any rows stored against it are additive overrides on top. That is what stops | ||
| the seeded list from freezing the catalogue as it looked on migration day. | ||
|
|
||
| Stored includes cannot punch through a built-in's code-level excludes, since | ||
| exclusions are applied after the union. That is deliberate: those excludes are | ||
| the boundary keeping `global-admin` clear of `bot.**` and `@destructive`, so | ||
| write access to `permission_role_patterns` must not be a route to the | ||
| owner-only surface. Widening past the boundary needs a deploy or a new role. | ||
| """ | ||
| specs: dict[str, RoleSpec] = {} | ||
| for role in roles: | ||
| includes = list(role.includes) | ||
| excludes = list(role.excludes) | ||
| builtin = BUILTIN_ROLES_BY_KEY.get(role.builtin_key or "") | ||
| if builtin is not None: | ||
| includes = [*builtin.includes, *includes] | ||
| excludes = [*builtin.excludes, *excludes] | ||
| specs[str(role.id)] = RoleSpec( | ||
| key=str(role.id), | ||
| includes=tuple(dict.fromkeys(includes)), | ||
| excludes=tuple(dict.fromkeys(excludes)), | ||
| includes_roles=tuple(str(edge) for edge in role.includes_roles), | ||
| ) | ||
| return specs | ||
|
|
||
|
|
||
| def _grant_rule(record: GrantRecord) -> Rule: | ||
| by_account = record.subject_account_id is not None | ||
| return Rule( | ||
| pattern=Pattern.parse(record.pattern), | ||
| effect=Effect(record.effect), | ||
| subject_kind=SubjectKind.ACCOUNT if by_account else SubjectKind.DISCORD_ROLE, | ||
| origin=Origin.ACCOUNT_GRANT if by_account else Origin.DISCORD_ROLE_GRANT, | ||
| scope_guild_id=record.scope_guild_id, | ||
| expires_at=record.expires_at, | ||
| source=f"account:{record.subject_account_id}" if by_account else f"discord-role:{record.subject_role_id}", | ||
| ) |
There was a problem hiding this comment.
not sure whats the point of these two reverse mapping functions
| action: Mapped[str] = mapped_column(Text, nullable=False) | ||
| """What happened: `grant`, `revoke`, `role_create`, `role_pattern_add`, and so on.""" |
There was a problem hiding this comment.
text is probably correct but we need to use a enum on the app level
| nullable=True, | ||
| default=None, | ||
| ) | ||
| subject_kind: Mapped[str | None] = mapped_column(Text, nullable=True, default=None) |
| INCLUDE_MODE = 1 | ||
| EXCLUDE_MODE = -1 |
There was a problem hiding this comment.
pretty sure this is defined more than once
| rows = (await session.scalars(select(PermissionRole))).all() | ||
| patterns = (await session.scalars(select(PermissionRolePattern))).all() | ||
| includes = (await session.scalars(select(PermissionRoleInclude))).all() | ||
|
|
||
| by_role_includes: dict[int, list[str]] = {} | ||
| by_role_excludes: dict[int, list[str]] = {} | ||
| for pattern in patterns: | ||
| target = by_role_includes if pattern.mode == INCLUDE_MODE else by_role_excludes | ||
| target.setdefault(pattern.role_id, []).append(pattern.pattern) | ||
|
|
||
| by_role_edges: dict[int, list[int]] = {} | ||
| for edge in includes: | ||
| by_role_edges.setdefault(edge.role_id, []).append(edge.included_role_id) |
There was a problem hiding this comment.
what is this garbage. Do it in SQL.
| @@ -0,0 +1,78 @@ | |||
| """Optional PostgreSQL LISTEN wake hints for durable pollers. | |||
|
|
|||
| A notification is a latency hint and nothing more. Every consumer of this class | |||
| while len(self._entries) > self._max_entries: | ||
| self._entries.popitem(last=False) | ||
|
|
||
| def observe_epoch(self, version: int) -> bool: |
There was a problem hiding this comment.
why version when it is epoch elsewhere
| interval=PERMISSION_POLL_INTERVAL_SECONDS, | ||
| ) | ||
| if watcher.listener is not None: | ||
| supervisor.start(watcher.listen(), name=f"{PERMISSION_EPOCH_JOB}-listener") |
There was a problem hiding this comment.
hmm maybe the names should be a strenum
There was a problem hiding this comment.
Redstone-Squid/squid/bot/utils/permissions.py
Line 102 in 4dde885
this is ignoring member.guild?
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class BuildDraft(StagedMedia, StagedTaxonomy): |
There was a problem hiding this comment.
This is still merging all categories into one and kinda ugly
| def load_worker_log_config() -> LogConfig: | ||
| """Load only inherited logging settings in a schematic worker child.""" | ||
|
|
||
| class WorkerLogSettings(BaseSettings): | ||
| model_config = _ProcessSettings.model_config | ||
| log: LogConfig = LogConfig() | ||
| strict_unknown_keys: bool = False | ||
|
|
||
| try: | ||
| settings = WorkerLogSettings() # type: ignore[call-arg] | ||
| except (ValidationError, SettingsError) as exc: | ||
| raise _configuration_error(exc) from None | ||
| _audit_unknown_environment_keys(strict=settings.strict_unknown_keys) | ||
| return settings.log |
There was a problem hiding this comment.
why a class in a function...
Encrypt draft caches, pending operation records, and short-lived sessions with the draft-cache key held by the credential layer. Bind each versioned envelope to its exact normalized origin and fixed state purpose so copied or swapped ciphertext cannot be accepted.\n\nUse fresh XChaCha20-Poly1305 nonces, atomic owner-only files, bounded reads, process locks, zeroized plaintext buffers, and fail-closed authentication errors.
Serve the double-submit token through the existing credentialed CORS boundary so a separate frontend origin can perform authenticated writes without weakening CSRF validation. Mark the response non-cacheable and publish the endpoint through the generated web client.
Add renderer-neutral browser draft editing, retry-safe media and finalization transport, and bilingual Minecraft device approval. Fetch session-bound CSRF tokens for cross-origin mutations while preserving stable idempotency identities.\n\nNarrow the root lib ignore rule so every handwritten web library module is present on clean checkouts.\n\nTests: 83 Vitest tests with coverage; Astro check; ESLint; production build; bundle budget.
Construct API clients only from validated trusted profiles and bind requests to normalized /api/v1 paths on that exact origin. Send stable protocol, renderer, locale, and instance headers while keeping bearer values and JSON bodies out of diagnostics.\n\nDisable redirects and ambient proxies, require TLS 1.2 or newer for HTTPS, bound CA and JSON inputs, reject non-JSON responses, preserve request IDs and server problems, and map status codes to the published CLI exit classes.
Expose recognized Minecraft versions as a revisioned form option source and revalidate the exact canonical value before build creation. Existing source-draft builds are still returned before catalogue lookup so retries remain stable after retirement.\n\nReject unresolved version relationships in persistence instead of silently creating versionless builds. Cover canonical persistence and unknown-option rejection with unit and PostgreSQL integration tests.
Cap mutation bodies before validation or idempotency can buffer them while preserving the separately bounded media stream. Mark owner-private API responses non-cacheable and enforce per-value, per-item, operation-count, depth, and cumulative retained-answer budgets in the draft domain.
Write mutating requests to origin-bound encrypted state before network execution and reconstruct retries with the original idempotency key. Serialize concurrent queue mutations, validate persisted routes and bodies, and expose only non-sensitive status metadata.\n\nClassify retries conservatively by method safety, idempotency, server retryability, attempt budget, exponential delay, and bounded Retry-After guidance. Redact and zeroize private JSON values after use.
Enforce retention at the application boundary so an elapsed or explicitly expired draft cannot be read, replayed, edited, finalized, or deleted as though it were active. Check expiry before honoring change replays and use an injectable clock for deterministic policy tests.
Secret-bearing installation and device responses were durably replayable but stored as plaintext. Seal every body with metadata-bound AES-256-GCM, support key rotation, and purge the legacy rows during migration. Run retention explicitly from the worker while preserving bounded replay semantics and no-store headers.
Authenticate bounded release manifests with a pinned Ed25519 public key before parsing their contents. Validate channels, SemVer, protocol ranges, native targets, artifact sizes, lowercase SHA-256 values, and credential-free HTTPS URLs.\n\nReport availability or protocol incompatibility without downloading or replacing the executable, and provide streaming artifact verification for explicit user-driven downloads.
Route provider-neutral build.submitted events through the existing Discord voting presentation. Serialize initial session creation per build and use stable Discord nonces so redelivery and send/track failures converge on one session and card per configured channel. Preserve the legacy submit path by moving it onto the same idempotent publisher, and direct pending staff notifications to the authenticated Discord review command instead of a public URL that cannot resolve.
Treat profile removal as a local security-state operation rather than a configuration-only edit. Resolve the profile before confirmation, delete encrypted state before its key, purge the selected credential backend, and retain the profile if any step fails.\n\nAvoid probing the system keyring for profiles that never created credentials and report the purged backend in stable JSON output.
Run bounded expiry batches from the maintenance worker, serialize them with draft artifact and finalization transitions, cancel unfinished finalization work, and tombstone associated media jobs for durable cleanup. Recheck due state under the lifecycle lock so concurrent completion wins safely.
Parse editor commands without shell expansion, bound the temporary text exchange, and reject unsafe replacement files. Child execution now terminates and reaps timed-out processes while preserving stable success, failure, and timeout outcomes.
Add backend-schema-neutral form constraints and a sanitized line-prompt renderer with strict non-interactive behavior. Transport now advertises only the capabilities configured for the invocation, and unknown required controls explicitly require web continuation.
Register normalized outputs before publication and protect in-flight puts with per-claim durable leases. Clean only unreferenced objects through token-fenced claims, with bounded retry state and expired-lease pruning.\n\nRun raw and normalized cleanup on an always-on worker cadence even when FFmpeg normalization is disabled.
Implement the profile-default full-screen editor as a bounded, testable state machine for every initial control kind. Sanitize server text, localize renderer chrome, support cancellation, and restore raw and alternate-screen terminal state on every exit path.
Keep long-running normalization work fenced with durable heartbeats and renew publication leases around every object-store write. Defer cleanup contention without charging an attempt, separate cleanup polling from normalization batches, and document the rolling-deploy grace period.
Bind Paper drafts to their authenticated installation and resolve sponsor consent before finalization. Persist an immutable public projection through schema-two jobs and builds, preserve payload integrity across account merges, and expose the attribution consistently in Discord, OpenAPI, and the web catalogue. Gate production behind a staged rollout flag so older workers never consume the new payload.
List bounded, answer-free draft summaries before submit so players can resume compatible work by category or UUID without guessing. Reconcile ambiguous create failures against authoritative state and replay the exact request only while its grant and idempotency window remain valid.
Make the explicit UUID selection fixture return the same revision advertised by discovery. Stale summary rejection remains covered independently.
Persist only purpose-separated digests for enrollment codes, proof nonces, and bearer sessions. Verify Ed25519 possession before issuing short-lived account-bound credentials, and revoke all sessions when a device is removed.\n\nThe migration registers device references for account merges and refuses to downgrade over retained authorization data. Focused unit coverage and the PostgreSQL migration drift check exercise the new boundary.
Expose browser-previewed CLI enrollment, Ed25519 proof exchange, session renewal, and account/device revocation through the v1 API. Authenticate resulting bearer sessions as a distinct CLI principal and preserve that origin on synchronized drafts.\n\nAdd opt-in HTTPS configuration, a guarded submission-origin migration, generated OpenAPI/TypeScript artifacts, isolated HTTP and security tests, and PostgreSQL fencing coverage.
Add localized approval pages that consume CLI user codes from URL fragments, keep them out of OAuth redirects, and require users to verify the device label and public-key fingerprint before granting access. Exercise preview and idempotent approval behavior in component and transport tests.
Implement Ed25519 device enrollment, short-lived session renewal, revocation, encrypted session state, and localized auth commands. Decode current RFC problem details while retaining legacy response compatibility, and recover one-time responses with scoped idempotent retries without pinning approval polling to a cached pending response.
`requires()` was annotated `Callable[[Any], Any]`, which erased the `.predicate` attribute discord.py attaches -- the very thing the taxonomy test reads to learn a command's node contract. Annotating it as `Check[Context[...]]` removes six type-checker suppressions from the tests that exercise the predicate directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Granting or assigning to a Discord role from a DM produced a database CHECK violation rather than an explanation: the storage rule that a role snowflake carries its guild is right, but it is a poor first thing for a user to see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
discord.py parses only the first keyword-only parameter of a prefix command and stops, so `!perm grant @user build.** global` put "build.** global" into the pattern and silently used the default scope. Only `reason` stays keyword-only, which is the one that legitimately wants the rest of the line; `pattern` moves to the front, where a required argument belongs in the slash-command UI anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The insert path wired tag assignments onto a build row that was not yet in the session. TagDefinition.assignments back-populates immediately, so the next lookup SELECT autoflushed a graph containing a transient assignment and SQLAlchemy warned that the cascade would not proceed. No test had ever seeded official tags, so the real taxonomy write path never ran under the suite's warnings-as-errors filter. Add the row first and guard the lookups with no_autoflush, mirroring the rationale already documented on the update path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The door/extender field lists exist in four hand-maintained copies across the mapper and repository with no test tying them together. This suite is the safety net for the aggregate redesign: hypothesis-driven and per-category save->load round trips against a migrated PostgreSQL database, plus characterization tests that pin the known asymmetries by name instead of hiding them: - unresolvable or ambiguous taxonomy names divert into extra_info and never return to the typed fields (save->load is lossy), - save() mutates its input (tags reassigned, door_type defaulted, edited_time restamped beyond PostgreSQL's precision), - missing door dimensions silently default to 1x2, - one URL under two media types is representable in the domain model but rejected by the build_links primary key. The shared migrated-database fixture moves to a conftest so both integration modules use one implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BuildRepository.save resolved restriction and pattern strings against official tag rows as a side effect of persistence: unknown or ambiguous names were silently diverted into extra_info and never returned to the typed fields, save() reassigned build.tags and injected a default pattern on its input, and save->load was not the identity. Move resolution to the application layer. A BuildTaxonomyResolver port (implemented by OfficialTagResolver, carrying the query that lived in the repository) feeds apply_build_taxonomy, which BuildService calls at the last application-owned step before persistence. It canonicalizes the typed fields to exactly what a reload derives, records unresolvable names in extra_info visibly, and leaves build.tags as the single source the repository now persists verbatim. The repository no longer interprets the restriction string fields at all, and the round-trip integration suite flips from characterizing the lossy asymmetries to asserting save->load identity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Build was a flat 40+-field dataclass shaped like the old Supabase row, carrying every category's fields as nullable options. Nothing stopped a Utility from holding door dimensions, category was a mutable field that could contradict the persisted joined row, and the door/extender field lists were maintained in four hand-written copies across the mapper and the repository. Model the closed hierarchy instead. Build holds the shared facts and one subclass per category holds its own, mirroring the joined-table persistence model; category becomes a ClassVar derived from the type and can no longer be reassigned. The subclasses name their fields exactly as the ORM models do, so both mapping directions now read one CATEGORY_FIELD_NAMES tuple and the four copies collapse to one. Flows that accumulate facts before the category is known -- the guided Discord form and message inference -- use the new BuildDraft, which is flat and fully optional and produces an entity through finalize(). The door dimension defaults that save() used to coerce silently are declared on DoorBuild, and the category defaulting that lived at the submit call moves into the inference path where it can be read. Links become a single typed collection: the database keys build_links by (build_id, url), so one URL cannot carry two media types, and the five parallel url lists could represent a state the database rejected only at write time. The five url names survive as read-only projections, with replace_links for the per-type edit semantics. The original message's five Final columns fold into an OriginalMessage value object owning the jump-link construction. diff() now walks __dataclass_fields__ rather than dir(), so it no longer reports properties as changed fields, and get_attr_type resolves against the concrete category class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BuildMapper issues its own SELECTs for the values other contexts own, because cross-context ORM relationships were deliberately removed. Those queries ran per row while the mapper was called in a loop for every page, so listing a page of builds cost four queries per build on top of the row query -- get_many's docstring claimed one query and meant only the rows. Add to_domain_many, which loads creators, submitter identities, versions and original messages for the whole batch with one query each, and make to_domain its single-row delegate. Every page-shaped read now maps through it. Give the read paths one eager-loading contract as well. They previously disagreed -- two declared selectinload explicitly, two relied on the model-level default -- and the model eagerly loaded creators and versions that the mapper never traverses. Fencing those two off with raiseload drops two more queries per read and turns a future traversal into a loud failure instead of a silently empty collection. Reading a page is now a fixed six queries at any page size, pinned by a statement-counting test. get_builds_by_id also stops mispositioning duplicate ids, which its index() lookup could not express. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BuildDetail carried every category's facts as flat nullable fields, so a utility answered with a door_dimensions object of three nulls and a client had no way to know which fields its category actually populates. BuildPatch had the same shape on the way in, and nothing stopped a door field being sent for an extender. Move the category-specific facts under a details union discriminated by category, which OpenAPI renders as a clean oneOf with a discriminator mapping, and nest the door-only edit fields under a door object that flattens onto the application patch's names. The headline timings stay on the summary rather than being repeated in details: the card projection and the search grammar already address them there. This is a breaking change to /v1. contracts/openapi.json and the generated TypeScript client are regenerated, and the catalogue renders its specification list off the union instead of a fixed field list, so extenders now show their length and type where they previously showed a row of empty door fields. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generic edit modal built its inputs from a hardcoded list, so every build was offered door dimensions, door orientation and door timings -- including utilities, which have no such fields. Nothing checked that the names in that list were ones BuildEditPatch could carry either, so a typo would surface only when a user pressed submit. Declare the fields instead, each naming a patch field and the categories that own it, and filter them by the build being edited. A test pins both properties: every entry names a real patch field, and the door-only ones never reach another category. Also stop diff() from emitting BuildLink objects. The vote session persists its change summary into a JSONB column, so an edit that touched links would have failed to serialize; diff now reports the same per-media url lists it did before the links redesign. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three assertions had drifted from the code they cover. The schematic
content test still called `/v1/schematics/{sha256}/content`, an endpoint
replaced by the build-scoped locator -- `test_public_api.py` even asserts
the old path is gone -- so it could only ever fail with a 404. The bot
logging test expected a `squid.discord.guild_id` attribute, but
`log.py`, `errors.py` and `docs/plans/observability.md` all agree the
name is `squid.guild.id`.
Delete the dead schematic test and move its attachment assertions onto
the build-scoped download, which had no coverage for its media type or
Content-Disposition, so the contract is still proven somewhere.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`f0a1b2c3d4e5` dropped `global_administrators` once the RBAC tables took over, but `_merge_references` still moved rows through it, so every account merge failed on a missing relation. The API fuzz harness seeded the same table and hashed it into its baseline checksum, so its stack could not start either. Move the merge onto the permission tables. The grantor columns have to go first: `permission_grants.granted_by_account_id` and its role- assignment twin are ON DELETE RESTRICT, so the absorbed account cannot be deleted while it is named as a grantor. Subjects then transfer, with the unique indexes forcing a decision on collisions. Role assignments keep the survivor's row, matching how the rest of the function resolves duplicates. Grants instead collapse onto the more restrictive effect, because keeping the survivor's row would let a merge silently drop an absorbed account's emergency forbid and hand back the access it revoked. The audit log is repointed rather than left behind: its account id has no foreign key, so an untouched entry would reference an id that no longer resolves. Covered by a new integration test that exercises the collision rules and the provenance columns against a migrated database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The API test app left `media_jobs`, `minecraft_installations` and `minecraft_player_authorization` as None and configured no Minecraft verification URI, so all sixteen draft-media and device-flow routes answered 503 before touching a handler. Schemathesis counts any 5xx as a server error, which is what the fifteen fuzz failures were: the suite was reporting its own wiring, and those routes went completely unexercised. Wire fakes that fail closed with client-safe errors, following `MockCliAuthorization`, and register a serializer for the wildcard image and video media types the upload route accepts -- schemathesis ships none, and an unserializable body is a failure rather than a skip. The rate-limit test read 503 as its "not limited" signal; with the routes reachable it now sees 400 and 401. Only the contrast with 429 ever carried meaning there, so assert the codes the routes really return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stderr pipe exists to carry the diagnostics a crashed worker leaves behind — faulthandler tracebacks, Rust panic dumps — and three separate defects conspired to drop them exactly when they mattered. The pump suppressed ValueError, which is what StreamReader.readline raises when a line overruns the 64 KiB default limit. A single large traceback therefore killed the pump silently, and that worker's stderr was never forwarded again for the rest of its life. Raise the stream limit to 1 MiB, recover from an overrun by dropping just that line, and log loudly if the pump ever dies for any other reason. _terminate cancelled the pump before awaiting the process, so a traceback still buffered in the pipe was discarded at the moment of the crash. Kill and reap first, then drain the pump with a bounded timeout. A clean aclose() closes stdin, the child exits 0, and the exit was still logged at WARNING — N spurious warnings on every orderly shutdown. Log expected exits at INFO and keep WARNING for the rest. Also moves the unstructured-stderr fallback's pid out of the message text into a worker_pid field, matching the structured path so log queries can filter both the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
configure_worker_logging pinned both the stderr handler and the root logger to DEBUG, making it the only logging setup in the project that ignored LoggingConfig. Every child DEBUG record was therefore serialised to JSON, piped, parsed and rebuilt into a LogRecord by the supervisor purely so the supervisor's INFO handler could drop it. Read SQUID_LOG_LEVEL/SQUID_ROOT_LOG_LEVEL from the inherited environment in the child, the same way load_worker_observability_config already reaches it, and drive the handler and root levels from them. The unset root level resolves to INFO to match WorkerProcessConfig.logging, so the child's floor is the supervising worker's floor. tests/unit/test_logging.py never exercised configure_worker_logging at all; it does now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Child records arrived carrying the child's module name but were dispatched through a single fixed logger, and Logger.handle skips isEnabledFor. The result was that configuring a level for a child module in the parent had no effect whatsoever: filtering happened only at handler level, and the handler chain belonged to a logger the record did not claim to come from. Dispatch each rebuilt record through logging.getLogger(record.name) behind an explicit level check, so a child record is filtered exactly as a record the parent emitted itself would be. Sinks are unchanged — child loggers are all under squid.* and reach the same handlers. worker_logger now serves only the supervisor's own commentary about a worker's stderr, which is what the fallback path always used it for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A schematic worker child stamps its records with service_name=redstone-squid-schematic-worker, and the supervisor rebuilds those records and formats them with its own JSON formatter on the way to the log file. Because service_name was a required format field, and python-json-logger applies static_fields after required fields but skips required fields when merging record extras, the supervisor's own identity always clobbered the child's. Every child log reached the collector labelled redstone-squid-worker, so schematic-engine failures were indistinguishable from the supervisor's own. Supply service_name as a formatter default instead, and drop it from the format string: defaults are applied first and a record attribute wins, so forwarded child records keep their identity while locally emitted records still get the process's. The collector needs no change — its resource/logs processor inserts service.name only when absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The schematic worker child configured observability as service_name "worker", which is also what the supervising database worker uses. Both resolve to service.name=redstone-squid-worker, so a child's spans and metrics were indistinguishable from its parent's — while its logs, which already said redstone-squid-schematic-worker, were not. Name the child schematic-worker so its telemetry identity matches the identity its logs have been claiming all along. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A log line from a schematic worker child said which operation failed but not which request or job it belonged to. The frame's request id was never logged on either side, and the durable layer's job id stopped at the process boundary, so correlating a child's traceback with the job that caused it depended entirely on OTel trace context being configured. Carry the durable job id in the request frame, sourced from a ContextVar the job runner sets, and stamp request id, operation and job id onto every record the child emits via a filter on its stderr handler. The filter sits on the handler rather than one logger so records from the native engine's own loggers are correlated too. The frame id counts per worker rather than per pool, so the correlation key in the logs is the (worker_pid, request_id) pair; both are already present on every forwarded record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Worker respawns, crashes, rlimit kills and circuit-breaker trips were reported only as OpenTelemetry counters, and add_counter is a no-op when the optional observability extra is not installed. A deployment without an exporter therefore had no signal whatsoever that its schematic workers were crash-looping: the first anyone heard of it was users being told the engine was out of service. Emit a log line next to every counter, and track the breaker's open/closed state so each transition is announced exactly once instead of on every rejected call. The trip also gets its own counter, which it never had. Breaker recovery is noticed lazily, when the pool is next called or sampled, because the failure window slides on inspection rather than on a timer. That is enough for an operator watching the logs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Queue depth was reported for the database-backed job queues but not for the worker pool those jobs run on, so nothing showed whether the pool was saturated, how many callers were queued behind a worker, or whether the circuit breaker was currently open. Publish idle worker count, in-flight count, waiting callers and breaker state, sampled periodically by the database worker's task supervisor in the same way it already samples queue health. Readiness requires the sampler only when a native pool actually exists, so a deployment without the schematic engine is unaffected. Render-slot contention is deliberately not reported: renders are serialised by design, and the operation duration histogram already exposes that queueing as latency. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The signed-cursor pagination is forward-only, carries no total, silently returns an empty page when its anchor document leaves the index, and hides a facet-join bug that corrupts OFFSET math on the sorted search path. The replacement design and the decisions behind it lived only in a session transcript, which is not a durable place for a five-commit plan. Write it down: the locked decisions (delete HMAC signing, transparent offset/after_id/before_id params, bidirectional paging and totals everywhere, catalogue sort allowlists), the global design, the commit breakdown, and the risks worth re-reading before each step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every catalogue listing is forward-only descending-ID keyset with no way to ask how many rows exist, so the API cannot offer a Previous link, a page number, or any ordering other than newest-first. The pagination redesign needs those capabilities at the query layer before the REST contract can expose them. Add them additively, ahead of the contract flip. list_page on builds, records, and notifications now accepts offset, after_id, and before_id alongside a sort, and each gains a matching count. A before_id page walks away from the anchor in reversed display order and is flipped back in memory, so its overfetched row lands at the front for the caller to trim -- the asymmetry is documented on every signature that has it. Builds may sort by submission_time (indexed, NULLS LAST, id tie-break); ID anchors reject a non-ID sort, since they address positions in the display order. The status/submitter predicate and the inbox visibility rules are extracted into helpers so page and count queries cannot drift apart. The builds count counts distinct ids, because the submitter-identity join can multiply rows. Old endpoints are untouched and the tree stays green: tests/unit/builds, tests/unit/notifications, and tests/unit/records all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Master removed schematic content filtering and advanced submission schemas and dependency versions. Keep this layer compatible with those changes while retaining its existing behavior and the sequential stack ancestry. Validated the final stack with focused unit and PostgreSQL tests, Ruff, Pyrefly, and a single Alembic head.
Master reserves schema two for submissions retaining original uploads. Apply the same discriminator in the ORM constraint and sponsor migration so the first stack layer starts without schema drift.
64aeb56 to
c299bc6
Compare
Stacked review part 1 of 15 for schematics phase 0.
Authored commit slice: 1-100 of 1,482.
Final stack tip:
83c1c32b4f0ba8f2c2c570296e0b0ce86564a1a5Base:
masterPrevious:
masterNext: #186
Review and merge these pull requests in order. GitHub will narrow each later diff as the preceding layer lands.
The repository's autofix workflow may add a formatting commit to an individual PR head; those generated commits are excluded from the authored slice count.