fix: eight adversarially-confirmed bugs from an engine hunt - #111
Conversation
An adversarial bug hunt (fan-out finders, refute-by-default verify) over the shipped engine surfaced ten defects; these are the eight with clear, low-risk fixes and a regression test each. Two are deferred to focused follow-ups (a distill embedding-reconcile that needs atomicity/keyless care, and an API error-message-leak that needs boundary date validation). - scrub.ts (sacred rule 1, high): Stripe underscore keys (sk_live_/sk_test_/ rk_live_) bypassed the sk- rule and reached immutable evidence in plaintext. Added a sibling detector. - sync.ts (data loss): an idle sync run fell back to the wall clock instead of keeping the prior watermark, silently dropping late-visible items. Retain the cursor; only the very first run falls back. - transcripts.ts (data loss): a VTT cue whose payload began NOTE/STYLE/WEBVTT was dropped as a comment. Track cue state; skip comments only between cues. - store.ts (data integrity): entity merge left goal.entity_id dangling, so deleting a goal-referenced entity tripped the FK and rolled back the distill. Re-point (or detach) the goal alongside edges and verifications. - link.ts (correctness): codeMatchesTerm and decisionsConcerningEntity matched substrings (sync in async, auth in author), raising false drift and forging bogus concerns edges. Match whole identifier words, splitting camelCase/snake. - cli.ts: --hops/--depth were not value flags, so a flag before the node id made positional() read the number as the id. - api.ts: a non-positive/fractional limit reached SQL as a negative LIMIT and 500d; fall back to the default. Six new core tests + one server test; typecheck, lint, root test (fail 0), smoke:packed, and benchmark-drift all green. Patch changeset (core+cli+web). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f038a41cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * never a substring: "sync" no longer matches "async", "test" not "latest", | ||
| * "auth" not "author". */ | ||
| function codeTokens(code: string): Set<string> { | ||
| return new Set(words(code.replace(/([a-z0-9])([A-Z])/g, "$1 $2"))); |
There was a problem hiding this comment.
When a relevant code identifier contains an initialism, this tokenizer leaves the initialism and following PascalCase word fused: for example, parseHTTPResponse becomes parse, httpresponse, so a decision such as “no HTTP responses” produces the responses/response variants but neither is present in codeWords. The prior substring fallback detected these common identifiers; add acronym-to-word boundary handling (such as HTTPResponse → HTTP Response) so the whole-word fix does not silently miss drift signals.
Useful? React with 👍 / 👎.
| const decWords = new Set(words(`${decision.title} ${decision.rationale}`)); | ||
| return terms.some((term) => decWords.has(term)); |
There was a problem hiding this comment.
This exact-token check drops ordinary singular/plural references: an entity named password no longer matches a decision saying “no passwords,” even though the previous matcher did. In that case entityHasDecision returns false, which prevents the intended concerns edge and raises a spurious gap question. Check the existing whole-word variants(term) (or equivalent inflection variants) against decWords to retain this relation without reintroducing substring matches such as auth in author.
Useful? React with 👍 / 👎.
1 participant
An adversarial bug hunt over the shipped engine (a finder per high-risk module, each candidate then refuted-by-default by an independent verifier) surfaced ten real defects. This PR fixes the eight with clear, low-risk fixes and a regression test each. Two are deferred to focused follow-ups (see bottom).
Fixes
scrub.tssk_live_/sk_test_/rk_live_) bypassed thesk-rule and reached immutable evidence in plaintext (sacred rule 1).sync.tstranscripts.tsNOTE/STYLE/WEBVTTwas dropped as a comment block.store.tsgoal.entity_iddangling, so deleting a goal-referenced entity tripped the FK and rolled back the whole distill.link.tscodeMatchesTerm/decisionsConcerningEntitymatched substrings —syncinasync,authinauthor— raising false drift questions and forging bogusconcernsedges.cli.ts--hops/--depthweren't value flags, so a flag before the node id made the parser read the number as the id.api.tslimitreached SQL as a negativeLIMITand 500'd.Verification
Six new core tests + one server test, each reproducing its bug.
pnpm typecheck,pnpm lint, rootpnpm test(ℹ fail 0; core 366, web 117),pnpm smoke:packed, andcheck-benchmark-driftall green.(Note: the scrub test fixtures are built by string concatenation so the file carries no contiguous key literal for push-protection scanners.)
Deferred to focused follow-ups (both confirmed, both mitigated)
embedNodefailure after the row commits leaves a node without an embedding, and the idempotent re-distill skip makes it permanent. Needs an atomicity/backfill design with keyless-mode care; mitigated today by the keyword fallback inrunSearchand edge-only graph neighbors inprepareTask./invalid /, so a bad timestamp param can reflect a Postgres error. Needs boundary date-validation across endpoints.🤖 Generated with Claude Code