docs(examples): correct four claims the code does not support - #151
Conversation
All four were imported verbatim from the archived postguard-examples and surfaced by dobby's review of #145. Each is the class the import exists to close: an example teaching something untrue. pg-node's README promised a guardrail that was deliberately removed. It said "The SDK validates the shape and throws a clear TypeError if you pass { notify: true } or forget to nest." No such validator exists, and packages/pg-js/tests/postguard.test.ts pins its removal -- it hand-maintained an allowlist of upload keys and rejected valid options when the types moved ahead of it. Worse, the footgun it claimed to guard is silent and strictly worse than omitting the option, which I traced through the production path rather than assuming: delivery?.recipients on `true` -> undefined (crypto/encrypt.ts:104) undefined ?? false -> false on wire (api/cryptify.ts:112) notify is defined -> notice suppressed (sealed.ts:98) So { notify: true } sends no mail and also silences the warning that would have said so. The README now says that, and points at { notify: { recipients: false } } as the way to mean silence. pg-dotnet's README claimed ".NET 8.0 or 10.0 SDK". The project targets net8.0;net10.0, and an SDK cannot build a target framework newer than itself, so the .NET 8 SDK fails on net10.0 -- examples/README.md already said "10.0+" and the two contradicted each other. examples.yml installing both is about runtimes, not about what can drive the build. pg-dotnet's README snippet could not compile. `sealed` is a C# keyword, so `var sealed = pg.Encrypt(...)` is CS1585/CS1525, and `var result` was then declared twice in one scope (CS0128). Program.cs uses sealed1/sealed2 and result1/result2 for exactly this reason; the snippet now uses named variables and says why. pg-sveltekit exported two constants nothing reads. UPLOAD_CHUNK_SIZE = 1MB read as "this example uploads in 1 MB chunks", but the PostGuard config never passes uploadChunkSize, so the SDK's 5,000,000 default is what runs. Deleted rather than wired: the example demonstrates nothing else about chunk tuning, and configuration that is not applied is the class this workspace has been removing. Wiring it in would be a deliberate change about what the example teaches. Also recorded why integration.yml's `node` lane is the deliberate exception to the --filter reasoning the bun, deno and api-surface lanes carry: it is the one lane whose job is whole-workspace coverage, which is what its env: block exists to supply, so narrowing it would remove coverage rather than relocate it. Refs #145
|
Thanks for the detailed writeup — I'm on it. I'll dig into the |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Three of the four corrections hold up. The fourth replaces a false claim with a different false claim, in the file the PR exists to make accurate, so this is about one sentence away from ready.
What I verified rather than assumed
- pg-node: no validator exists anywhere in
packages/pg-js/src, and all three mechanism claims in the new paragraph check out against source (encrypt.tsreadsdelivery?.recipients,api/cryptify.ts:112sendsnotifyRecipients: options.notifyRecipients ?? false,sealed.ts:98gates the silent-upload notice onopts?.notify === undefined). - pg-dotnet snippet:
sealedis a keyword, andProgram.cs:57,84does usesealed1/sealed2withresult1/result2. The renames are correct. - pg-sveltekit deletion: a repo-wide grep for both constants returns only the two declarations plus the unrelated SDK-internal
DEFAULT_UPLOAD_CHUNK_SIZEinencrypt.ts:14. Zero readers, nothing in a README or changeset describes them, andexamples/*publishes nothing, so deleting rather than wiring them in leaves no stale docs behind. - integration.yml: every claim in the new comment checks out. Details in the inline note.
- The prerequisite's conclusion is right. SDK 10 alone does build both target frameworks; I built them.
Blocking item (inline on examples/pg-dotnet/README.md:14): the .NET 10 SDK does not ship the net8.0 reference assemblies. Restore downloads them from NuGet.
One more, outside the diff so I could not attach it inline: dotnet run at lines 23 and 30 of that same README does not work on this project. TargetFrameworks is net8.0;net10.0, and dotnet run on a multi-target project exits with Unable to run your project / Your project targets multiple frameworks. Specify which framework to run using '--framework'. I reproduced it both with and without --no-build; dotnet run -f net10.0 works. It came in with the #145 import, it is the same class as the four claims this PR fixes, and it is one flag per occurrence while the file is already open.
Rule check. The title matches Conventional Commits and pr-title.yml is active. Refs #145 is the correct form rather than a missing closing keyword, because #145 is a pull request, not an issue. No changeset is needed: nothing under packages/pg-js changed and examples/* is unpublished. Em dash and bold density in the new prose runs above the org writing rules' cap, but it matches what these files already do, so I am not raising it as a finding.
|
Reviewed #151 and requested changes on one item — three of the four corrections hold up, but the pg-dotnet one swapped in a different wrong claim. Blocking ( Also flagged (non-blocking, left as comments):
Everything else — the pg-sveltekit removal, the pg-dotnet snippet renames, the pg-node validator removal, title/Refs — checked out clean. Review: #151 (review) |
Four review findings. The blocking one is that the previous commit replaced a false claim with a different false claim, in the file the PR exists to make accurate. The .NET 10 SDK does not ship the net8.0 reference assemblies. `dotnet restore` acquires that targeting pack from NuGet. Verified locally rather than reasoned about: the SDK's packs directory holds only 10.0.10, while ~/.nuget/packages/microsoft.netcore.app.ref holds the 8.0.x copies. The conclusion -- SDK 10 builds both target frameworks, SDK 8 cannot -- was right; the reason was not. Also records that `--locked-mode` does not block the download, because a targeting pack is not a PackageReference and so never appears in packages.lock.json. `dotnet run` cannot work on this project at all. TargetFrameworks is `net8.0;net10.0`, so it exits with "Your project targets multiple frameworks. Specify which framework to run using '--framework'". Reproduced, then fixed in all three places it appears -- twice in pg-dotnet's README and once in examples/README.md, which the earlier commit missed. Same class as the four claims this PR set out to fix, and it came in with the import. The pg-node note named an SDK-internal identifier. `delivery` exists only inside the SDK, so a reader of that README has nothing to match it against -- they only ever type `notify`. Restated in the caller's vocabulary: reading `.recipients` off the boolean `true` yields `undefined`. Same mechanism, no name the public surface does not have. Moved the node-lane rationale to where its reader is. It explains why the `node` lane must not be narrowed, but sat inside the `bun` job's steps, so whoever edits the node lane never reached it. It now sits above `node:`. And corrected "api-surface below" to "above" while that hunk was open -- api-surface is the second job, the bun lane the third. Refs #145
|
/dobby fixed in The .NET 10 SDK does not ship the net8.0 reference assemblies. Confirmed on this machine rather than taking it:
The Moved the node-lane rationale above On the em-dash and bold density: noted, and I agree it is not worth raising when the surrounding files read the same way. |
|
Thanks for confirming and for catching that |
There was a problem hiding this comment.
Three findings, one blocking.
The .NET rewrite in ec2ecfb holds up. I reproduced the multi-target error verbatim (Your project targets multiple frameworks. Specify which framework to run using '--framework'), and the targeting-pack mechanism checks out: packs/Microsoft.NETCore.App.Ref/ on this machine holds only 10.0.0-rc.2, and packages.lock.json has zero ref-pack entries, so --locked-mode has nothing to block. All three dotnet run sites are fixed and a repo-wide grep finds no fourth. The api-surface above correction and the move above node: are both right: job order is node(24), api-surface(60), bun(108), deno(152), and the comment sits in the bun lane.
The blocker is the one claim ec2ecfb did not touch. apps/website is still named as something the env: block supplies, and it is not. It has two homes: the workflow comment and the PR body's ## Also section, which repeats it. Fixing only the comment leaves the untrue version as the description.
Checked rather than argued, on the other two:
@sealedcompiles and prints 42 under SDK 10.0.100-rc.2, socannot be an identifieris still an overclaim.{ notify: true }isTS2559against the realUploadOptions, sonothing checks that for youis wrong for TypeScript callers. The mechanism in the two bullets under it is correct end to end.
The delivery restatement, the dotnet run -f net10.0 sweep, and the sveltekit constant removal (zero remaining references repo-wide) all verify clean. Not raising the em-dash and bold density, per your note.
|
Reviewed The .NET rewrite holds up end to end: the SDK really doesn't ship net8.0 ref assemblies (only One blocker remains: The two nits carry over as confirmed rather than stylistic: |
|
/dobby fix the issues in the dobby comments |
|
On it! I'll go through the review comments on this PR and push fixes for the issues raised. |
`nothing checks that for you` is false for TypeScript callers: `notify` is
object-only in `UploadOptions`, so `{ notify: true }` is a TS2559. It is the
runtime that has no check, and this example is plain JS, so `at runtime` is
the scope that makes the sentence true without weakening it.
`cannot be an identifier` overstates the C# rule: verbatim identifiers make
`@sealed` legal (`var @Sealed = 42` builds and prints 42 on the 10.0.100-rc.2
SDK). Bare `sealed` is the part the compiler rejects, and the reason
Program.cs picks sealed1/sealed2 is readability, not legality.
Both came out of checking this PR's claims against the toolchain, and both cost a build to rediscover: the example is multi-target so `dotnet run` needs `-f net10.0`, and its first build restores the net8.0 targeting pack from NuGet. `UploadOptions.notify` is checked by the compiler and not at runtime, which is why the plain-JS examples need to say so.
|
The Patch for whoever applies it: diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml
index 6d6eb16..8b114e1 100644
--- a/.github/workflows/integration.yml
+++ b/.github/workflows/integration.yml
@@ -12,20 +12,20 @@ on:
env:
PKG_URL: ${{ vars.PKG_URL || 'https://pkg.postguard.eu' }}
CRYPTIFY_URL: ${{ vars.CRYPTIFY_URL || 'https://storage.postguard.eu' }}
POSTGUARD_WEBSITE_URL: ${{ vars.POSTGUARD_WEBSITE_URL || 'https://postguard.eu' }}
jobs:
# The broad lane, deliberately. It runs the recursive typecheck, build and test
# rather than filtering to the SDK the way `api-surface`, `bun` and `deno` do,
- # because whole-workspace coverage is its job: apps/website and both addons
- # included, which is what the `env:` block above exists to supply. Narrowing it
- # would remove that coverage rather than relocate it. examples/* is cheap here,
- # and examples.yml covering them again is duplication rather than a gap.
+ # because whole-workspace coverage is its job: both addons included, which is
+ # what the `env:` block above exists to supply. Narrowing it would remove that
+ # coverage rather than relocate it. examples/* is cheap here, and examples.yml
+ # covering them again is duplication rather than a gap.
node:
name: Node ${{ matrix.node }}
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
node: ['22', '24']
steps:
I re-checked the claim behind it rather than taking it from the review: The same sentence in the PR body is corrected (a body PATCH needs no push, so no CI rerun). On the base sync: merging current |
There was a problem hiding this comment.
One blocker, unchanged from cycle 2, and it is now the only thing left. It cannot be fixed by another bot cycle, so this is a hand-off rather than another lap.
.github/workflows/integration.yml:20 still ships the false apps/website claim. Confirmed against the tree at 85f06bc rather than re-read from the last round: PKG_URL, CRYPTIFY_URL and POSTGUARD_WEBSITE_URL are consumed only by apps/tb-addon/build.mjs:11, apps/tb-addon/src/lib/pkg-client.ts, and apps/outlook-addon/webpack.config.js:11. Under apps/website the only hit is src/lib/env.ts:10, which hardcodes export const PKG_URL = '/pkg'; every requireEnv call in that file reads a VITE_* name, and those come from the committed apps/website/.env. So the lane covers apps/website, but the env: block supplies it nothing — which is the shape of claim this PR exists to delete.
Why this ends the loop instead of going back for a fix. The App has no workflows permission; the push was already attempted and rejected (refusing to allow a GitHub App to create or update workflow). A second fix cycle would reproduce exactly that. This needs a maintainer — nothing will pick it up on its own. The one-line patch is in the comment above; I re-checked it rather than trusting it:
git apply --checkagainst the branch tip85f06bc— clean.git apply -3 --checkagainst85f06bcmerged with currentmain— clean. Worth stating becausemainhas movedintegration.ymlsince this branch started (322c44f, a different region of the file); the three-way still handles it.
No separate docs commit from me, deliberately: a push touching the same tree would risk the git apply, and there is nothing outstanding that needs one.
Everything else checks out at 85f06bc.
- Both cycle-2 nits are addressed in the shipped text.
pg-dotnet/README.mdnow says@sealedcompiles and gives the readability reason, andpg-node/README.mdscopes the claim to "nothing checks that at runtime", which is the correct scope given TypeScript callers getTS2559. - The
dotnet runfix is complete — a repo-wide grep over*.mdfinds no baredotnet runleft, including theexamples/README.md:54site. - The
pg-sveltekitdeletion leaves nothing stale:UPLOAD_CHUNK_SIZEandFILEREAD_CHUNK_SIZEappear nowhere else in the tree, docs included. The only remaining hits are the SDK's own unrelatedDEFAULT_UPLOAD_CHUNK_SIZEinpackages/pg-js/src/crypto/encrypt.ts. - The PR body's
## Alsosection already carries the corrected wording, so the fix is not left half-applied across the two surfaces — only the workflow file is behind. - New
CLAUDE.mdsection claims no CI gate that no workflow defines, and both bullets are repo-specific, so the rootCLAUDE.mdis the right home for them. - PR title is conventional and
pr-title.ymlwill pass.Refs #145is correct here — this is a partial fix and should not auto-close the tracker.
Loop limit reached. This is the third gatekeeping cycle on #151 (prior CHANGES_REQUESTED on 29daf58-era HEAD and on ec2ecfb), so I am not sending it back again regardless of the blocker. The PR is one git apply away from ready: apply the patch from the comment above, and the last false claim is gone.
| jobs: | ||
| # The broad lane, deliberately. It runs the recursive typecheck, build and test | ||
| # rather than filtering to the SDK the way `api-surface`, `bun` and `deno` do, | ||
| # because whole-workspace coverage is its job: apps/website and both addons |
There was a problem hiding this comment.
Blocking, carried from cycle 2 and re-verified at this HEAD rather than re-read: apps/website takes nothing from the env: block, so "apps/website and both addons included, which is what the env: block above exists to supply" is false about half of what it names.
Evidence: PKG_URL, CRYPTIFY_URL and POSTGUARD_WEBSITE_URL are read only by apps/tb-addon/build.mjs:11, apps/tb-addon/src/lib/pkg-client.ts and apps/outlook-addon/webpack.config.js:11. The single hit under apps/website is src/lib/env.ts:10, which hardcodes export const PKG_URL = '/pkg'; the requireEnv calls there read VITE_* names supplied by the committed apps/website/.env. The lane does cover apps/website — the env: block just is not what covers it.
This is the one file the bot cannot push (no workflows permission — the push was attempted and rejected), so it needs a maintainer. The corrected wording is in the patch comment on this PR; it drops apps/website and from this line and reflows the block. git apply --check passes at 85f06bc and git apply -3 --check passes after merging current main.
|
Done with cycle 3 — I posted a consolidated review requesting changes, and everything from cycle 2 checks out clean (the One blocker is left, and it needs you rather than another bot pass: I've hit the 3-cycle loop limit, so this won't get picked up automatically again — the PR stays in "changes requested" until that one patch lands. |
1 participant
Four claims in
examples/that the code does not support, from dobby's review of #145. All four were imported verbatim from the archivedpostguard-examples, and each is the class the import exists to close: an example teaching something untrue.pg-node's README promised a guardrail that was deliberately removedNo such validator exists.
packages/pg-js/tests/postguard.test.tspins its removal — it hand-maintained an allowlist of upload keys and rejected valid options when the types moved ahead of it.Worse, the footgun it claimed to guard is silent, and strictly worse than omitting the option. Traced through the production path rather than assumed:
delivery?.recipientswheredeliveryistrue(crypto/encrypt.ts:104)undefinedoptions.notifyRecipients ?? false(api/cryptify.ts:112)falseon the wire — no mailopts?.notify === undefined(sealed.ts:98)false, so the silent-upload notice does not fireSo
{ notify: true }sends no mail and suppresses the warning that would have said so. The README now says exactly that, and points at{ notify: { recipients: false } }as the way to mean silence.pg-dotnet's README claimed an SDK that cannot build it.NET 8.0 or 10.0 SDKis not achievable: the project targetsnet8.0;net10.0, and an SDK cannot build a target framework newer than itself, so the .NET 8 SDK fails onnet10.0.examples/README.mdalready said "10.0+", so the two READMEs this repo ships contradicted each other.examples.ymlinstalling both is about runtimes, not about what can drive the build.pg-dotnet's README snippet could not compilesealedis a C# keyword, sovar sealed = pg.Encrypt(…)isCS1585/CS1525— andvar resultwas then declared twice in one scope (CS0128).Program.csusessealed1/sealed2andresult1/result2for precisely this reason. The snippet now uses named variables and says why, since this is the file the .NET surface is taught from.pg-sveltekitexported two constants nothing readsUPLOAD_CHUNK_SIZE = 1MBread as "this example uploads in 1 MB chunks", butnew PostGuard({ pkgUrl, cryptifyUrl })never passesuploadChunkSize, so the SDK'sDEFAULT_UPLOAD_CHUNK_SIZEof 5,000,000 is what actually runs.FILEREAD_CHUNK_SIZEis unread too.Deleted rather than wired in: the example demonstrates nothing else about chunk tuning, and configuration that is not applied is the class this workspace has been removing (
.npmrc, bothoverridesblocks,.prettierignore). Wiring it would be a deliberate change about what the example teaches, and should be argued as one.Also
Recorded why
integration.yml'snodelane is the deliberate exception to the--filterreasoning thebun,denoandapi-surfacelanes carry. It is the one lane whose job is whole-workspace coverage — both addons included, which is what itsenv:block exists to supply (apps/websiteis covered by the lane too, but reads its own committed.env, so it needs nothing from that block) — so narrowing it would remove coverage rather than relocate it. The examples are cheap there, andexamples.ymlcovering them again is duplication rather than a gap.Verified:
pg-sveltekittypecheck, build and lint clean;pg-nodetypecheck clean;pg-dotnetbuildsnet8.0+net10.0;actionlintclean.The wording fix on that
integration.ymlcomment cannot ship in this branch — the bot has noworkflowspermission, so the push is rejected. It is a one-line patch in a comment below for a maintainer to apply.Refs #145.