Spec the plugin system, the pds migration, and the analytics plugin - #18
Merged
Conversation
The analytics feature must not ship with the CLI by default. Rather than build a bespoke install, config and lifecycle path for one feature, these specs introduce an internal plugin system and land analytics on top of it: install, config ownership, writing a config block into an existing JSONC file, and resource lifecycle become one mechanism instead of four per-feature decisions. blogwright-pds migrates onto the SPI in the same effort, staying a default dependency so users see no change. It exists to validate the contract against a second consumer of genuinely different shape before anything is frozen — designing a plugin API against one example is how the wrong abstraction happens. Also adds the implementation plan: 57 tasks across eight milestones, with the analytics AWS clients and transform Lambda deliberately independent of the plugin-system track so both can be worked from day one. Two research findings shaped the design and are recorded in the specs. Firehose matches JSON keys to Iceberg column names exactly and drops the rest silently, so the analytics transform Lambda is mandatory rather than an optimisation; it is also where the viewer IP is hashed and never stored. And discovery cannot read <package>/package.json — Node's exports encapsulation makes that throw for every package in this workspace — so it resolves the entry point and walks up, proven by an integration test against the real loader rather than a fake. An adversarial review of the plan found six blocking defects, four of which were gaps in these specs rather than in the plan. All six are repaired here; the plan carries the record in its Verification history section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewing the change specs against the code (spec-reviewer template R3) turned
up two defects introduced by the earlier repair pass, both invisible to a
reading of the task files alone.
Task 08's discovery called ports.loader.packageJsonPathFor(), an operation task
05 never defined — a dependency on a port method no task created. Task 05 now
declares it, implements it as resolve-then-walk-up, and carries a test
contrasting it with require.resolve('<name>/package.json'), which throws
ERR_PACKAGE_PATH_NOT_EXPORTED for every package in this workspace.
Tasks 01 and 52 were repaired for the site-state/plugin-state split but their
done certificates were not, so they still encoded the single-state-field
contract. A validator would have discharged them against the wrong shape and
passed. Both certificates now name state, siteState and record() separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten findings from the adversarial review, plus two decisions the plan had resolved silently rather than asking. The two decisions, both settled here. The visitor_key salt is now a random secret in Secrets Manager rather than derived from the date: IPv4 is a 2^32 space, so a salt an attacker can compute makes the digest brute-forceable in seconds and the pseudonymisation decorative. That adds an eleventh node, a secretsmanager:GetSecretValue grant scoped to that secret, a saltSecretName config field and a cold-start read. And blogwright-analytics joins the fixed changeset group, because plugin add installs it at the CLI's own version and that version would otherwise never exist on the registry. The structural fixes: plugin config validation moves out of createContext into the dispatch path, which is the only place that can see the dispatched plugin without making discovery non-lazy; core gains pluginBlock() so a plugin-owned config key can be read without a cast, placed in task 03 rather than 27 because 27 depends on 19 and putting it there would make the graph cyclic; the transform moves under src/ so the existing tsconfig, vitest glob and lint script cover it, matching what build-agent actually does; and the plugin namespace dispatches before createContext, so plugin add works on a repo that has no config yet. The rest close correctness gaps: the static pds USAGE block now goes at task 26 rather than 29, so --help never lists pds twice mid-migration; task 57 triages five open questions rather than four, the missing one being the destroy interlock; and task 20 no longer claims to execute a merge-plan step it deliberately defers. The dependency graph is verified acyclic: 57 tasks, no dangling edges, no forward edges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The spec said the visitor_key salt "rotates daily" and lived in Secrets Manager. Taken literally that means managed rotation — a rotation Lambda, a schedule and a second execution role — which is more moving parts than the thing they protect, and the plan had no node for any of it. One long-lived secret with the per-day value derived as HMAC-SHA256(secret, day) gives the same daily turnover with nothing to operate. The stored secret is created once and never rewritten, because replacing it after rows exist breaks unique-visitor comparison across the boundary. Brute-force resistance is unchanged and is still the reason the salt is secret at all: IPv4 is a 2^32 space, so a salt an attacker can compute reverses every row to its source address in seconds. Cost is a flat $0.40/month per environment. The transform caches the secret at cold start — reading it per invocation would be ~43,000 GetSecretValue calls a month at a 60-second Firehose buffer, more than half the price of the secret for no benefit. SSM Parameter Store would be free but would cost a hand-rolled ssm client, a bad trade against $4.80/year. Tasks 41, 42 and 50 and their certificates now state the derived design; both 41's functions stay pure and take the secret and day as arguments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two places broke the rule that a plugin's resources belong to the plugin. The site's OIDC deploy role branched on config.pds and derived that plugin's secret ARN (nodes.ts:906) — plugin topography in the site graph. pds now contributes its own resource node attaching a blogwright-pds-named inline policy to the site's role (task 23), and the site drops its branch (task 58). IamClient already has putRolePolicy/listRolePolicies/deleteRolePolicy, so no client work. The two are sequenced additive-first: named inline policies are separate, so both grants coexist for one step and no commit leaves a CI deploy without access to the secret. Four AWS clients existed in blogwright-core solely for the analytics plugin. They move into the plugin, built over the SigningClient already on its context. knip corroborates the placement: core would have exported four clients nothing in core or the CLI consumes. None of that was reachable before. ServiceKey is keyof typeof SIGNING_NAMES and SendOptions.service is typed to it, so a plugin could not sign against a service core did not enumerate — every new service meant editing core and moving its published surface. Task 31 changes from "add four signing names" to "accept a plugin-supplied service descriptor", which is what makes the rule hold for this plugin and any future one. Moving the clients into the analytics package made tasks 32-35 depend on the package skeleton, which was a forward edge — the plan promises its numbers are a valid execution order. Tasks 32-38 are rotated so the skeleton lands first. Also resolves the ten minor review findings: plugin-commands.ts gets a named author instead of three tasks racing to create it; task 09's collision handling is collect-as-failure in both its Steps and its DoD rather than one of each; the analytics package seeds a real test, because vitest exits non-zero with no test files and would have held the repo-wide gate red for eight tasks; rolldown is scheduled as a dependency; the plugin gets a declared composition root for its own bundled artifact; and task 28's "if eager" branch collapses to the answer already settled. 58 tasks, graph verified acyclic with no forward edges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng, typed config An independent semi-formal review of the three change specs found six blocking defects. Four are applied here, plus the generic config shape that fixes two more. Discovery could not find blogwright-pds at all. The spec scanned the consuming repo for blogwright-* dependencies, but a consumer depends on blogwright, which does not match the prefix — this repo's own package.json has zero blogwright-* entries. Every bundled plugin was invisible, so deleting the hardcoded pds branch would have broken blogwright pds entirely. The candidate set is now the union of the consumer's deps and the CLI package's own. The analytics spec contradicted itself: its Proposed-changes blocks put the four AWS clients in the plugin while its Implementation notes still said to add them to core's SIGNING_NAMES and AwsClients. The notes were stale from before the topography change; they now describe the seams core actually gains. Nothing could build a us-east-1 client. A SigningClient's region is fixed at construction and the us-east-1 one is a local const reachable only through the pre-built acm/cloudfront/route53 clients — so a plugin could not construct one, and every analytics service is us-east-1. AwsClients gains signingUsEast1. Analytics resource names carried no environment while every other derived name in the repo is <env>-<siteName>. Staging and production would have shared one Iceberg table and 'analytics destroy' in staging would have deleted production's data. AWS gives a second reason: Firehose does not recommend two streams writing one Iceberg table. The shared delivery source could be torn out from under the plugin: the site's delete() removes the source unconditionally (Conflict, throwing part-way through destroy) and its retry deletes every delivery on the source — silently, while the plugin's state still reports healthy. New task 52 adds both guards. Plugin config moves to Plugin<TConfig> with validateConfig returning the resolved block onto ctx.pluginConfig. ctx.config.analytics cannot compile — OpsConfig has no index signature — and a validator returning void left pds' secretName default nowhere to live, which would have widened a dozen call sites to string | undefined. Also corrects a claim I had asserted as researched: Firehose does not silently discard mismatched fields. AWS documents that a column name or type mismatch throws and delivers to the S3 error bucket; only an additional field is skipped silently. The transform Lambda is still mandatory — only the failure mode was wrong, and it puts the error bucket on the normal path rather than a rare one. 59 tasks, graph verified acyclic with no forward edges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rified table path Research resolved all three, and one of them differently than expected. The Firehose error bucket becomes a twelfth node in us-east-1 rather than reusing the site environment bucket. S3DestinationConfiguration.BucketARN matches an ARN pattern that carries no region, so the API can neither express nor reject a cross-region bucket, and Firehose cross-region documentation covers only HTTP endpoint destinations. That leaves the behaviour undocumented rather than forbidden, which is not something to rest the pipeline on. It matters more than it first appeared: since a schema mismatch errors every affected record to that bucket rather than dropping it, the error path is a normal path. The stream is created with AppendOnly: true. page_views is insert-only by design and Firehose scales throughput automatically for append-only Iceberg streams. The flag is settable only at CreateDeliveryStream, so a stream created without it has to be deleted and recreated. The table-creation question resolved rather than blocking. Firehose documents that it only delivers to tables created through the Iceberg GlueCatalog API, which read as though it forbade the S3 Tables control-plane path the plan uses. AWS own S3 Tables and Firehose walkthrough creates the namespace and table with aws s3tables create-namespace and create-table and then points the stream at them, so the limitation applies to plain Iceberg-on-S3 tables registered in Glue, not to S3 Tables reached through the s3tablescatalog federation. Recorded in task 48 so the next reader does not re-litigate it; resource links, required until 2025-07-31, are also no longer needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four alternating rounds of clean remediation and clean review, each agent independent with no shared context. The blocking/important/minor trajectory was 2/3/4, then 0/3/4, then 0/4/1, then 1/2/5. It did not converge, and the shape of that matters more than the counts. Round 1's blocking findings were surface contradictions — the plan still carrying the superseded "clients in core" design in seven places, a PluginContext whose enumerated fields omitted names and accountId that both consumers need. What round 4 found is a layer deeper and was only reachable once those were cleared: the additive-first argument for moving the pds IAM grant reasons about commit ordering, but the operative event is a deployed bootstrap, and there the two grants never coexist. applyOidcRole rewrites the same <env>-deploy inline policy name on every reconcile, so the first bootstrap after upgrade drops the secretsmanager statement whether or not the plugin's own policy exists yet. Closed across the four rounds: discovery could not resolve the blogwright package to read its own dependencies, because the CLI's exports map has no "." entry — so the union-with-bundled-plugins fix from the previous round was unimplementable as written, and self-location has to go through import.meta.url. PdsContext requiring pluginConfig broke the post-deploy sync, which reaches requirePdsConfig from an OpsContext with no dispatch boundary to populate it. pluginBlock could not be written at all, since indexing OpsConfig by a string is TS7053. Plugin state was typed as a bare outputs map, which does not compile against destroyGraph's ctx.state.resources. Plugin<TConfig> had no default, so a plugin owning no config key had no valid type argument. Two claims were checked against primary sources and deliberately not "fixed": the multiple-streams-per-Iceberg-table sentence is verbatim from AWS's Firehose considerations page, and AppendOnly is contested between that page's prose and the IcebergDestinationUpdate API reference, so the stream node reconciles it and falls back to replacement rather than asserting either reading. Eight findings remain open and are listed in the pull request: one blocking, two important, five minor. Nothing here is implemented, so they are review items rather than defects in shipped behaviour. 59 tasks, twelve analytics nodes. Graph verified acyclic with no dangling or forward edges, every task paired with a certificate, task dependencies matching their plan rows, all links resolving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fifth review, on a different model, found a blocking defect three earlier passes had missed, and independently rediscovered the deployment-ordering gap a fourth had found. Both are fixed here, along with the remaining twelve open findings and one the remediation turned up on its own. The blocking defect was a release window writing a broken grant into a live role. Task 27 removes core's secretName default while nodes.ts:925 still interpolates that field into the OIDC policy's Resource ARN, and TypeScript accepts string | undefined inside a template literal, so nothing fails to compile. applyOidcRole rewrites the whole <env>-deploy document on every bootstrap, so the next one writes secret:undefined-* — and the plan explicitly blessed shipping in that state with the removal task outstanding. Task 27 now applies the default locally, one commented expression with a named owner, and task 59 deletes it along with the block it lives in. The ARN is correct at every commit rather than at every release boundary. That defect was residue from an earlier rewrite of task 23, and the residue had spread: task 27 pointed at "task 23's rewired ARN" when nothing rewires it any more, and task 59 removed a blogwright-pds import from nodes.ts that has never existed. The spec sentence that seeded the phantom step — the CLI's graph "stops importing" from the plugin — is corrected to say it continues to import nothing. The deployment gap is now argued in the terms that matter. Additive-first was true of commits and false of deployed stacks: the plugin's named policy reaches a real role only when an operator runs blogwright pds bootstrap, which nothing else invokes. The pds spec gains an Upgrading a deployed stack section naming that step as required, and an open question pricing the three self-healing shapes, including why a config.pds-keyed guard is rejected — it would put back the topography leak the migration exists to remove. Also closed: save() was classified as pass-through host surface when it closes over the site's store, so a plugin graph would have persisted into the site's state key and never its own — the one context member that typechecks either way. The pds migration's "nothing changes for users" no longer holds now that pds contributes a node, and the affected commands are enumerated. Four anchors, a region-pinning absolute that two global clients legitimately violate, and a stale claim that four AWS clients "existed in core" when they only ever existed in a superseded draft. The remediation also caught what neither review did: task 30 flipped the pds spec to Merged while one of its Proposed-changes blocks was still outstanding at task 59. Merge-plan steps 4-5 now execute where the work actually lands. 59 tasks, 107 edges. Graph acyclic with no dangling or forward edges, every task paired with a certificate, dependencies matching their plan rows, 451 links resolving, node counts consistent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A seventh review found one blocking defect and ten others, all of the same shape: a fix applied in one place while an adjacent file kept asserting the belief the fix disproved. The blocking one was the clearest instance. Task 02 declared ResourceNode<Ctx extends PluginContext> and justified it by parameter contravariance, but an earlier round had added pluginConfig, siteState and record() to PluginContext — which OpsContext does not have. So ResourceNode <OpsContext> violated its own constraint, and the spec rests the analogous assignability on method bivariance, which task 02 explicitly disclaimed. The constraint is dropped and the engine is generic over the structural minimum both contexts satisfy. The genericization had also fallen between tasks 02 and 16 with each deferring to the other; task 02 owns it now. Also closed: the Firehose stream declared no edge to its own delivery role while task 54's edge test asserted one, so the two tasks pinned contradictory sets and the ordering survived only by topoSort's alphabetical accident. A certificate demanded an init command the spec forbids and discovery would reject. The pds spec claimed validation outcomes are identical when the settled dispatch-only decision makes built-in commands stop rejecting a malformed block — a fifth operator-visible change in a migration that listed four. Plus a task whose pointer and definition of done contradicted each other inside one item, stale either/or scaffolding around a settled decision, drifted README anchors, and a task filename left over from a superseded design. The rest of this change is a gate, because that failure mode is mechanically checkable and was being caught by 400k-token reviews two rounds late instead. .specs/plans/.../type-claims/ carries a transcription of the proposed SPI types, each citing the spec section it came from, and compiles 29 assertions against the repo's real OpsContext, OpsConfig, OpsState and PdsContext. Claims the documents say must fail are pinned with @ts-expect-error, so a claim that silently stops being true fails the gate too — verified by removing Readonly from SiteState and watching C13 break on the unused directive. Reintroducing the constraint this round removed makes it name the broken claim and exit non-zero. The harness lives outside packages/ and is not wired into CI or the root scripts; graduating it is proposed, not done. knip, build, test and oxfmt are all clean with it present. 60 tasks, 108 edges. Graph acyclic with no dangling or forward edges, every task paired with a certificate, dependencies matching their plan rows, links resolving, node counts consistent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bootstrap warning is the one that changes behaviour rather than prose. Moving the pds grant to the plugin left an upgrade path that only worked if the operator read the release notes; bootstrap now warns, once per scoped state key it finds under state/, naming the plugin verb to run. It reads key names only — not config, not the plugin registry — which is what lets it stay honest about the two rules it must not break: a config-keyed check would put plugin topography back in the site graph, and a discovery-based one would cost bootstrap the discovery-free path the SPI pins with a test. It warns rather than refuses, and says plainly what it cannot see: a plugin configured but never bootstrapped leaves no key to find. The record-expiration question was factually wrong and is now correct. AWS documents that record expiration is not available for tables you create, and PutTableMaintenanceConfiguration governs snapshot expiry and compaction — space, not rows. Aging data out means partition-level deletes the plugin issues itself, which the append-only day partition makes cheap to shape. Whether to do it at all stays open; only the false premise is gone. analytics backfill becomes a declared optional action, reading the CloudWatch log group the site already writes through core's existing filterEvents. The second-ingestion-path objection is weaker than it reads: DuckDB is already a dependency for the dashboard adapter, so this reuses what is there. It is idempotent by construction rather than by bookkeeping — it only writes whole days strictly before the day the delivery node recorded at creation, so it can never race the Firehose path for a row. Daily salt rotation stands, with the consequence written down rather than implied: visitor_key never correlates across a day boundary, so a monthly unique figure is the sum of daily uniques. The dashboard's named queries now say so, so they cannot imply a distinct count the data does not support. plugin remove now asks before removing a plugin that owns resources, and refuses in non-interactive mode rather than defaulting. Removal forecloses the destroy verb, so answering the default would strand resources with no command left to reach them. Two new tasks, 60 and 61, each landing its spec's last outstanding block and so carrying that spec's status flip. Eleven open questions down to seven, with both that blocked implementation resolved. 62 tasks, 115 edges. Graph acyclic with no dangling or forward edges, every task paired with a certificate, dependencies matching their plan rows, links resolving. Type-claim gate passing at 29 claims; knip, build, test and oxfmt clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Three linked change specs and the implementation plan for them. Nothing here is implemented — this is design work for review, not code.
What it proposes
blogwright-core, discovery via apackage.jsonmanifest field, genericblogwright <plugin> <action>dispatch, andblogwright plugin add|list|remove. Internal and unversioned until it has carried two features through a release cycle.blogwright plugin add analytics.Plus the plan: 62 tasks across eight milestones, each with a done certificate. Two milestones depend on nothing in the plugin-system or pds streams and can be worked from day one.
Review history
Seven independent passes, each by an agent with no shared context — five on Opus, two on Fable. All findings raised are closed.
The last two passes are the interesting ones. A four-round remediate/review loop went
2/3/4 → 0/3/4 → 0/4/1 → 1/2/5(blocking/important/minor) and did not converge — each round reached a layer the previous had made visible. A final pass on a different model then found a blocking defect the three Opus passes had all missed:Both models independently identified a second defect: the "additive-first" argument for moving the pds IAM grant was true of commits and false of deployed stacks. The plugin's named policy reaches a real role only when an operator runs
blogwright pds bootstrap, which nothing else invokes.Other defects found and closed across the passes: discovery that could not resolve the
blogwrightpackage at all (itsexportsmap has no.entry); aPdsContextchange that broke the post-deploy sync; apluginBlock()helper that could not be written (TS7053); plugin state typed so it would not compile againstdestroyGraph;save()classified as pass-through when it closes over the site's store — the one context member that typechecks either way; and an unreachable us-east-1 signer that would have blocked the entire analytics pipeline.Two claims deliberately left as-is
Both checked against primary sources, not defects — please don't "fix" them:
AppendOnlyis genuinely contested: that page's prose says it is settable only atCreateDeliveryStream, while theIcebergDestinationUpdateAPI reference accepts it. The spec treats it defensively rather than picking a side AWS has not.A type-claim gate
These documents make type-level claims — that
OpsContextfails to satisfyPluginContextwithTS2739, thatctx.config.analyticsisTS2339, that a constraint holds. Nothing checked them against each other, so a change to one proposed type left adjacent tasks asserting the old truth, and reviews caught it two rounds later.type-claims/closes that. It carries a transcription of the proposed SPI types — each citing the spec section it came from — and compiles 29 assertions against the repo's realOpsContext,OpsConfig,OpsStateandPdsContext. Claims the documents say must fail are pinned with@ts-expect-error, so a claim that silently stops being true breaks the gate too.Verified in both directions: reintroducing the constraint this PR removes makes it name the broken claim and exit 1, and removing
ReadonlyfromSiteStatebreaks the readonly claim on the unused directive.When a spec changes a proposed type, update the transcription and re-run — whatever breaks names a task that needs updating.
It lives outside
packages/, andknip,build,testandoxfmtare all clean with it present. It is not wired into CI; graduating it is proposed as a plan-close decision, not done here.Verification
Graph acyclic, 115 edges, no dangling or forward edges · 62 tasks each paired with a certificate · every task's dependencies matching its plan row · all links resolving · node counts consistent at twelve.
The last review also verified ~70
file:lineanchors as accurate, recompiled every load-bearing type claim against this repo'stsc, and confirmed each load-bearing AWS claim verbatim against vendor documentation.Open questions
Seven remain, down from eleven — both that blocked implementation are resolved.
Settled in the latest commit:
blogwright bootstrapnow warns while a plugin's scoped state exists (reading key names only, so it costs neither the discovery-free path nor a topography leak); daily salt rotation stands, with the sum-of-daily-uniques consequence written into the dashboard's named queries;analytics backfillis a declared optional action, idempotent by construction against the Firehose path; andblogwright plugin removeasks before removing a plugin that owns resources, refusing rather than defaulting when non-interactive.Also corrected: the record-expiration question asserted that the S3 Tables API supports a record-expiration configuration. AWS documents that it is not available for tables you create —
PutTableMaintenanceConfigurationgoverns snapshot expiry and compaction, not row retention. Aging data out means partition-level deletes the plugin issues itself. Whether to do that at all stays open; the false premise is gone.The remaining seven are deferred by design — SPI version declaration (moot while both consumers version in lockstep), an
afterDeployhook (waiting on a second consumer), whetherpreviewbecomes a plugin,pdsaction aliases, the Glue catalog integration's teardown contract, row retention, and whether core should hold plugin config blocks as an opaque map.Known residual risk
No review pass has run since the last remediation, and this corpus's history shows remediation introducing defects at roughly the rate it fixes them — the seventh pass found a blocking defect created by the fifth. The type-claim gate now catches that specific class automatically; the rest is still human-reviewable prose.
One acknowledged limitation:
Readonly<Record<string, ResourceOutputs>>prevents replacing an entry insiteState.resourcesbut not mutating inside one. The gate pins what the spec claims; a deeper guard would need a recursive mapped type.🤖 Generated with Claude Code