Skip to content

feat(core)!: retire PluginSecurityScanner — plugin security scanning is not a platform capability - #15930

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-14919-retire-plugin-security-scanner
Sep 5, 2026
Merged

feat(core)!: retire PluginSecurityScanner — plugin security scanning is not a platform capability#15930
os-zhuang merged 3 commits into
mainfrom
claude/issue-14919-retire-plugin-security-scanner

Conversation

@zhuangjianguo

@zhuangjianguo zhuangjianguo commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #14919

Retires PluginSecurityScanner from @objectstack/core under ADR-0049
enforce-or-remove, across all four surfaces the ruling names, plus the ADR-0087
ledger entry the disposition turned out to owe.

Why removal beat repair

The class was a shell that reported success. scan() composed five private
scanners:

  • scanCode, scanMalware, scanLicenses, scanConfiguration each allocated
    an empty issue array, logged, and returned it with no code in between. None
    could report a finding for any input.
  • scanDependencies ran a real loop, but matched only against vulnerabilityDb,
    an in-memory Map whose sole writer was the public addVulnerability — which
    had zero callers in this repo, in objectui at the pinned sha, and in the
    example itself. updateVulnerabilityDatabase() logged twice and fetched
    nothing.

So the database was empty on every code path that has ever executed, no issue
was ever produced, the score stayed 100, and the result was status: 'passed'
for every plugin the scanner was ever handed — a malicious one included. A
security control that cannot fail is worse than no control, because callers rely
on it. Repair was refused by name: a real vulnerability scanner is a feature with
a design surface, not a defect fix. There is no replacement export.

What changed

Surface Change
packages/core/examples/phase2-integration.ts deleted — the only constructor in the tree
packages/core/src/security/security-scanner.ts deleted
packages/core/src/security/index.ts export block dropped; tombstone comment left in its place
packages/core/PHASE2_IMPLEMENTATION.md section 6 rewritten to state that plugin security scanning is not a platform capability
docs/qa/platform-checklist/FOLLOW-UPS.md row deleted; dependent prose repaired
packages/core/src/security/security-scanner-retirement.pin.test.ts new — export-list pin
packages/spec/src/migrations/entries/semantic/18.plugin-security-scanner-retired.ts new — the ADR-0087 D3 ledger entry
packages/spec/src/migrations/registry.ts regenerated by gen:migration-registry (157 semantic entries)
.changeset/plugin-security-scanner-retired.md new — @objectstack/core minor, BREAKING banner
.changeset/plugin-security-scanner-ledger-entry.md new — @objectstack/spec patch, the ledger half

Three exports leave the public surface, not one: ScanTarget and SecurityIssue
existed only to feed the class and were exported beside it. The changeset names
all three.

The pin is an export-list assertion, not a grep

Object.keys() over both barrel namespaces, per the ruling. A grep cannot answer
this: the name legitimately survives in the tombstone comment, in the pin's own
header and in the retired doc section, so a grep pin would go red on the
tombstones that exist to explain the retirement, and would stay green if the
class were re-exported under a different local name.

The pin carries a control assertion — PluginSandboxRuntime, the export
block immediately above the retired one, reaching the root barrel by the same
export * line the scanner used. Without it, a barrel that failed to load would
answer "absent" for every name and pass forever.

Reverse verification. Re-adding a PluginSecurityScanner declaration to the
security barrel turned both retirement assertions red while the control
stayed green — expected [ 156 names ] to not include 'PluginSecurityScanner'
on the root barrel and [ 82 names ] on the security barrel. Mutation confirmed
on disk before the run; restore proven byte-exact afterwards (worktree
git hash-object equal to the HEAD blob, git diff HEAD empty). Both barrels
are reached by relative specifiers into this package's own src/, so no dist
leg is involved and no rebuild gates the result.

The ADR-0087 disposition, and what the gate actually wanted

The first push carried not-required (no-migration-prescription) and
check:adr-0087-registration refused it. The gate was right, and the reading is
worth recording because the fix was not the obvious one.

What it wanted. Not a missing marker — an untruthful one. The changeset
carries a real consumer prescription (delete the import and every call), so
claiming no consumer has to rewrite anything is a self-contradiction the gate
checks statement-against-statement. Every other not-required category is false
here too: @objectstack/core publishes, so unpublished is out;
already-registered had no entry to name; type-surface-only needs an
any/unknown-to-concrete narrowing this is not; and runtime-interface-only
explicitly inherits the same prescription refusal rather than escaping it.
So the only truthful disposition was registered, which required actually
writing the ledger entry.

That is also the repo's settled convention for this exact shape — a published TS
symbol with no spec schema, no stored source and no tombstone, where the ledger
is the only channel that reaches an upgrader. contracts.IDataDriver.findStream
and actor-user-roles-to-positions are both registered on those grounds, and the
gate's own header names the predicament verbatim.

D3 semantic, not a D2 conversion — so this is not the metadata migration the
ruling excludes. The class has no spec schema, so there is no authorable key to
tombstone and no stored sys_metadata row to rewrite: a scanner was constructed
per call and every result lived in a per-instance Map discarded with the object,
leaving applyConversionsToStoredItem no seam that would ever see one.

Two more things the gate taught, both measured rather than assumed:

  1. It reads committed state. Fixing the marker in the working tree changed
    nothing; the gate re-read the old text from HEAD until the edit was
    committed.
  2. registered takes ids only — no trailing why. The three not-required
    forms accept prose after the category; registered parses everything after it
    as a comma/space-separated id list, so a rationale sitting there was read as
    131 nonexistent migration ids. The rationale now lives in the changeset body,
    where a reader gets it anyway. That asymmetry is real and AGENTS.md spells it.

The regeneration lap did not materialise, and that is measured. The entries
README warns that spec-changes.json and docs/protocol-upgrade-guide.md are
projections that must be regenerated when an entry lands. Here check:generated
reports all 15 artifacts up to date, and running gen:spec-changes and
gen:upgrade-guide explicitly moved neither file — a major-18 semantic entry
is not yet projected into either. registry.ts is the whole generated diff.

Beyond the four named sites, and why

Two edits go past the ruling's literal enumeration. Both are the same defect
class the ruling closes — a shipped document teaching a capability that does not
exist — and leaving either would have produced exactly the dangling reference the
ruling forbids.

  1. PHASE2_IMPLEMENTATION.md carried two more capability claims that name no
    class and so were invisible to a grep for the symbol: "Security scanner
    integrates with CVE databases" (Security) and "Security scanning can be run
    asynchronously" (Performance). The first is replaced by a statement that the
    platform performs no plugin security scanning; the second is dropped.
  2. FOLLOW-UPS.md's row had two dependents. The paragraph beginning
    "Compounding the first row" existed only to elaborate it, and cites two files
    this PR deletes; after the deletion "the first row" would also point at a
    different surface. It is rewritten to record the closure and to preserve the
    half that survives. The neighbouring row's evidence column said its "only
    consumer is the dead scanner" — this PR makes that false, so it now reads
    zero consumers.

Verification

All at head 67e559697. Exit codes captured by redirect before any pipe.

Command Exit Verdict line
pnpm --filter @objectstack/core exec vitest run --maxWorkers=2 0 Test Files 50 passed (50) · Tests 1204 passed (1204)
pnpm --filter @objectstack/core run typecheck 0 tsc --noEmit + tsc -p tsconfig.examples.json + check:test-typecheck: OK
pnpm --filter @objectstack/spec run check:generated 0 All 15 generated artifacts are up to date
node scripts/check-adr-0087-registration.mjs --base origin/main 0 registered plugin-security-scanner-retired (new here: plugin-security-scanner-retired)
node scripts/check-published-readme-exports.mjs 0 60 published document(s) across 79 workspace package(s)
the 75 families scripts/pm/dispatch-gates.mjs derives for this diff all 0 re-derived after the diff widened into packages/spec (54 before)

The examples program still has an input after the deletion
(kernel-features-example.ts), and --listFiles confirms the new pin is inside
tsconfig.test.json's program while the deleted source is inside neither — so
the typecheck green is a measurement over the new file, not a green over a file
nothing read.

Two derivation notes, since the derived list is a lead rather than a
specification. check:migration-registry — the gate that proves registry.ts
still matches the entries directory, and the one most obviously implicated by
this diff — is not in the derived 75; it was run anyway, via spec's
check:generated. And check:api-surface entered the derived set only once the
diff reached packages/spec; on the core-only diff it was named by the ruling
and by nothing else.

Parked — do not land

Clause-2 card (a public export is removed), reviewable only at
CONTRACT_REVIEW_TIER, which is quota-exhausted. The dispatch exemption covers
dispatch only, never contract review. needs:contract-review is on this PR and
on the card. Green CI on this PR is not landability.


Generated by Claude Code

…14919)

ADR-0049 enforce-or-remove; maintainer ruling 2026-09-05 (director summon #14,
decision batch #42). The class was a shell that reported success: four of its
five private scanners returned an empty issue list unconditionally, and the
fifth matched against an in-memory vulnerability database whose only writer had
zero callers -- so every scan() ever performed answered status: 'passed' with a
perfect score, for a malicious plugin as readily as a benign one. A security
control that cannot fail is worse than none, because callers rely on it.

- delete packages/core/examples/phase2-integration.ts (the only constructor)
- delete src/security/security-scanner.ts; drop its export block from
  src/security/index.ts, leaving a tombstone naming the retirement
- rewrite PHASE2_IMPLEMENTATION.md section 6 to state plainly that plugin
  security scanning is NOT a platform capability, and drop the two capability
  claims elsewhere in the same document that outlived their subject
- delete the FOLLOW-UPS.md row, repair the paragraph that existed only to
  compound it, and correct the neighbouring row whose evidence the deletion
  falsified
- pin the retirement as an export-list assertion on both barrels

Repair was refused by name: a real vulnerability scanner is a feature with a
design surface, not a defect fix. There is no replacement export.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/core, @objectstack/spec, touching 34 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/core/PHASE2_IMPLEMENTATION.md), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

27 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json f1e91595f706b7eefb73457754f4eb05fa13e362.

1 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/core/PHASE2_IMPLEMENTATION.md) — pages documenting those are invisible to this run
  • 23 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 135 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f1e91595f706b7eefb73457754f4eb05fa13e362packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3bd95c5c5e6b2d4f98cf5da03fa76e27737191a3 — the merge of head 67e559697ada8e10715b6dfa73cadf71446164de into base f1e91595f706b7eefb73457754f4eb05fa13e362, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 3bd95c5c5e6b2d4f98cf5da03fa76e27737191a3 && git checkout 3bd95c5c5e6b2d4f98cf5da03fa76e27737191a3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f1e91595f706b7eefb73457754f4eb05fa13e362 67e559697ada8e10715b6dfa73cadf71446164de && git checkout -B drift-repro f1e91595f706b7eefb73457754f4eb05fa13e362 && git merge --no-ff 67e559697ada8e10715b6dfa73cadf71446164de

node scripts/docs-audit/affected-docs.mjs --json f1e91595f706b7eefb73457754f4eb05fa13e362

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f1e91595f706b7eefb73457754f4eb05fa13e362 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…0087 ledger (#14919)

check-adr-0087-registration refused the previous disposition, correctly: the
changeset carries a real consumer prescription (delete the import and every
call), so `not-required (no-migration-prescription)` was a self-contradiction.
Every other not-required category is false too -- @objectstack/core publishes,
so `unpublished` is out; `already-registered` has no entry to name;
`type-surface-only` needs an any/unknown-to-concrete narrowing this is not; and
`runtime-interface-only` explicitly inherits the same prescription refusal
(#8299). The only truthful disposition left is `registered`.

That is also the repo's settled convention for this exact shape -- a published TS
symbol with no spec schema, no stored source and no tombstone, where the ledger
is the only channel that reaches an upgrader. contracts.IDataDriver.findStream
and actor-user-roles-to-positions are both registered on those grounds.

D3 semantic, not a D2 conversion: the class has no spec schema, so there is no
authorable key to tombstone and no stored sys_metadata row to rewrite -- a
scanner was constructed per call and every result lived in a per-instance Map
discarded with the object, so applyConversionsToStoredItem has no seam that would
ever see one. This is what the ruling's "no metadata migration" excludes, and it
is excluded.

- add entries/semantic/18.plugin-security-scanner-retired.ts (one file, per the
  entries README kit -- no hand edit inside registry.ts's generated markers)
- regenerate registry.ts via gen:migration-registry (157 semantic entries)
- flip the core changeset's marker to `registered
  plugin-security-scanner-retired`, keeping the BREAKING banner, the
  no-replacement statement and the NOT MEASURED paragraph untouched
- add the @objectstack/spec patch changeset, mirroring the #6138 backfill

Measured and recorded in that changeset: the regeneration lap the entries README
warns about did not materialise. check:generated reports all 15 artifacts up to
date, and running gen:spec-changes and gen:upgrade-guide explicitly moved
neither file -- a major-18 semantic entry is not yet projected into either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
…ionale into the body

check-adr-0087-registration parses everything after `registered` as a
comma/space-separated id list, so the trailing `why` prose that the
`not-required (...)` forms accept was read as 131 nonexistent migration ids. The
asymmetry is real and AGENTS.md spells it: `registered SOME-MIGRATION-ID` carries
no `why`, the three `not-required` forms do. The rationale is unchanged, only
relocated into the changeset body where a reader gets it anyway.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ

Copy link
Copy Markdown
Contributor

Contract review (clause ②) PASS at head 67e55969 — director seat, session_01TezFG8ZMrNH6n5VTNpPpdH, 2026-09-05T16:1xZ. Verdict with the item-by-item derived judgments, semver reading and boundary-flag answers: objectstack#14919 comment 5553061275. needs:contract-review removed from this PR and from the card in the same stroke (a legitimate clear: two removals seconds apart, both citing this comment). The dev's open question — the packages/spec D3 ledger entry — is ruled inside the ruling, not a scope breach.

Landing follows landing-operations.md: carrier pair check, all checks green on 67e55969, governed test 0 of 10 paths ⇒ ready + auto-merge (SQUASH). Closes #14919 is correct; the card's pm:dispatched comes off at MERGED confirmation.

Implemented-by: session_01ARYe3yQTQCUFm5qPYNgKaJ
Reviewed-by: session_01TezFG8ZMrNH6n5VTNpPpdH


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review September 5, 2026 16:11
@os-zhuang
os-zhuang enabled auto-merge September 5, 2026 16:12
@os-zhuang
os-zhuang added this pull request to the merge queue Sep 5, 2026
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

PM note on this PR's clause-② status — recorded so a cleared label is not later read as a review that happened.

Measured just now, not inferred:

The one thing that should not be misread. This card was declared Clause-②: yes, and no contract review at CONTRACT_REVIEW_TIER was ever performed on it — not by this seat, not by anyone. The implementing round was dispatched at opus under the quota exemption, and that exemption covers dispatch only; it has never covered contract review. Fable has been rate-limited since roughly 15:10; a real probe task at 15:55 returned HTTP 429 ("You've reached your Fable limit"), so the tier was unavailable rather than skipped by choice.

So the carrier now reads clear, but it reads clear because it was cleared — not because a review passed. If this PR lands from here, it lands on maintainer authority. That is the maintainer's call to make; this comment exists only so it is made knowingly, and so nobody reading the labels in a month concludes a review is on file.

This seat has not armed this PR and will not arm it, consistent with the park recorded on the card. No action is requested.

Two things still on offer, if wanted:

  1. When the fable tier returns, this seat can still run the clause-② review and post the result, whether or not the PR has landed by then.
  2. One scope question from the implementing round remains open: the packages/spec ledger entry (commits 5b3d1a9ed + 67e559697, +63/−0). If the intent was no ledger entry at all, backing out those two commits is the whole remedy — they are cleanly separable from the rest of the change.

Generated by Claude Code

Copy link
Copy Markdown
Contributor

Correction to the PM note above (5553093398) — a clause-② review at CONTRACT_REVIEW_TIER IS on file for this head. The note's "Reviews on record: none" read GitHub's PR-review objects; this repo's contract verdicts live as card comments, and the one for this PR is objectstack#14919 comment 5553061275 (director seat, session_01TezFG8ZMrNH6n5VTNpPpdH, tier read by get_session as claude-fable-5-1 on both session_context.model and last_served_model; posted 16:0xZ, before the labels came off). The provenance comment on this PR is 5553069685 (16:12Z). So the carrier reads clear because a review PASSED, not on maintainer authority — and the label removals (card, then PR, seconds apart, both citing the PASS) are the legitimate-clear signature. The note's "not queued" reading was also a few seconds early: the timeline shows auto_merge_enabled 16:12:03Z and added_to_merge_queue 16:12:09Z.

The open scope question the note repeats was ruled in that PASS (row 4): the packages/spec D3 ledger entry (5b3d1a9e + 67e55969) is inside the ruling — a semantic entry is not the metadata migration the ruling excludes, and check:adr-0087-registration left registered as the only truthful disposition. No back-out. Thank you for recording the note rather than assuming; it is exactly the ambiguity a legitimate clear needs to be distinguishable from a strip, and the answer is now beside it.


Generated by Claude Code

Merged via the queue into main with commit cc00df2 Sep 5, 2026
42 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-14919-retire-plugin-security-scanner branch September 5, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

3 participants