fix(skills,stack,scripts): shipped decrypt guidance, and a sound package-path linter (#772 review findings 14, 15)#789
Conversation
… the package-path linter sound
Finding 14 — decryptModel/bulkDecryptModels return an
AuditableDecryptModelOperation: thenable, with .withLockContext() and .audit().
Three sites in skills/stash-encryption and four in packages/stack/README.md
said the opposite ("a plain Promise<Result<...>>", "no .withLockContext() to
chain"), steering agents away from the very .audit() chain the audit-on-decrypt
changeset advertises. The skill contradicted its own reference table, which was
already right — 8b74430 fixed that table and nothing else, despite a commit
message claiming otherwise.
Both files ship: the skill inside the stash tarball and thence into customer
repos via installSkills(), the README inside the @cipherstash/stack tarball.
The equivalent statement about the WASM entry is CORRECT and deliberately
untouched — that client really does return a bare promise.
Also `protectOps.eq` in the setup prompt stash init writes for coding agents.
One occurrence repo-wide, and no such export exists; the real API is
createEncryptionOperators(client), conventionally `ops`.
Finding 15 — the package-path linter had a false positive and a false negative,
both fixed with the fixture-driven self-tests the suite already uses:
- The name capture had no right anchor, so a sentence-final `packages/stack.`
swallowed the period and reported a LIVE package as dead. Uppercase was also
excluded from the class, so a capitalised name was never checked at all.
- livePackages came from readdirSync, i.e. the working tree. Deleting a package
leaves dist/ and node_modules/ behind, so every reference to it kept passing —
the exact case the linter was commissioned to catch, silently unenforced on
any checkout that had built the package. Now derived from `git ls-files`.
Deliberately not "has a package.json": packages/utils has none and is live.
- scripts/ was not scanned, so the linters never checked themselves. Adding it
immediately surfaced a dead allowlist entry in lint-no-hardcoded-runners for
a path that never existed in git history. Self-test fixtures stay exempt —
they must name dead packages.
Severity note for the record: CI was never affected (fresh checkout, caching
disabled), and main carries no required status checks — this was a local
developer papercut plus a real soundness hole, not a broken build.
Finally, resolves two changesets that contradicted each other in the same
release: dynamodb-eql-v3 claimed "EQL v2 tables continue to work unchanged" and
"no existing caller needs to change" while stack-dynamodb-v2-write-removal
announced the v2 encrypt overloads as removed. The code sides with removal;
the claims are now scoped to the decrypt path.
🦋 Changeset detectedLatest commit: df047ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approve.
All three parts check out. Confirmed the typed client genuinely returns a chainable AuditableDecryptModelOperation with .audit()/.withLockContext(), so the corrected decrypt-chaining docs now match the code (the old "plain Promise" wording was the bug), and the WASM-entry statement was correctly left intact. The package-path linter's readdirSync → git ls-files change is the right soundness fix, and protectOps no longer appears in any shipped surface. Changeset targeting is correct (stash patch + @cipherstash/stack patch), and the PR also repairs a real contradiction between two unreleased changesets.
Non-blocking:
- The highest-value fix —
livePackagesfromgit ls-files— has no dedicated regression test. It's awkward to test (needs a tracked-vs-untracked dir fixture) but achievable; without it a future revert toreaddirSyncwould leave every test green. setup-prompt.ts:283now points atcreateEncryptionOperators, but this prompt sets up the v3 flow, where the Drizzle factory iscreateEncryptionOperatorsV3. Not wrong the wayprotectOpswas (the v2 name is a real export), just off-version — considercreateEncryptionOperatorsV3or keeping it generic.
2 participants
Review remediation for #772 — findings 14 and 15, plus the contradictory-changeset item from the cut-for-length list.
Finding 14 — the shipped docs contradict the code
decryptModel/bulkDecryptModelsreturn anAuditableDecryptModelOperation: thenable, carrying.withLockContext()and.audit(). Verified both ways — seven chaining forms compile, and a credential-free runtime test (protect-ffi mocked) passes 7/7.Three sites in
skills/stash-encryption/SKILL.mdand four inpackages/stack/README.mdsaid the opposite ("a plainPromise<Result<...>>", "no.withLockContext()to chain"), steering agents away from the exact.audit()chain the audit-on-decrypt changeset advertises. The skill contradicted its own reference table, which was already correct:8b744309fixed that table and nothing else, despite a commit message claiming it had updated the guidance.Both ship — the skill into customer repos via
installSkills(), the README in the@cipherstash/stacktarball.SKILL.md:483says the same thing about the WASM entry and is correct — that client really does return a bare promise with no lock-context argument. Deliberately untouched.Also:
protectOps.eqin the setup promptstash initwrites for coding agents. One occurrence repo-wide, and no such export exists anywhere; the real API iscreateEncryptionOperators(client), conventionallyops.Finding 15 — the linter had a false positive and a false negative
packages/stack.swallowed the period and reported a live package as dead. Uppercase was excluded from the class too, sopackages/Foowas never checked at all.livePackagescame fromreaddirSync— the working tree. Deleting a package leavesdist/andnode_modules/behind, so every reference to it kept passing. That is precisely what this linter was commissioned to catch, silently unenforced on any checkout that had previously built the package. Now derived fromgit ls-files. Deliberately not "has apackage.json" —packages/utilshas none and is live.scripts/was never scanned, so the linters never checked themselves. Adding it immediately surfaced a deadpackages/drizzleallowlist entry inlint-no-hardcoded-runners.mjsfor a path that never existed in git history. Self-test fixtures stay exempt; they must name dead packages.Severity, for the record: CI was never affected —
actions/checkoutgives a fresh workspace with caching disabled — andmaincarries no required status checks. This was a local-developer papercut plus a real soundness hole, not the broken build the review implies.Verified the false negative is closed by re-creating a stale untracked
packages/protect/distshell: the reference is now flagged where it previously passed.Cut-list — contradictory changesets in one release
.changeset/dynamodb-eql-v3.mdclaimed "EQL v2 tables continue to work unchanged" and "no existing caller needs to change", whilestack-dynamodb-v2-write-removal.mdannounced the v2 encrypt overloads as removed — both unreleased, so both render into the same CHANGELOG. The code sides with removal (dynamodb/types.tsdeclaresTable extends AnyV3Tablewith no v2 overload). The claims are now scoped to the decrypt path, which genuinely is unchanged.Verification
pnpm run test:scripts→ 93 passed (90 + 3 new fixture-driven cases).lint:package-pathsandlint:runnersboth clean. Biome: 0 errors.