fix(cli,api): close defects found by live-testing the published CLI - #7149
Closed
waleedlatif1 wants to merge 8 commits into
Closed
fix(cli,api): close defects found by live-testing the published CLI#7149waleedlatif1 wants to merge 8 commits into
waleedlatif1 wants to merge 8 commits into
Conversation
Live testing of the published package found a set of flags the CLI documents as constrained and then transmits anyway. `--limit` on the two filtered row mutations says "0 is not accepted" and sent 0, -1 and 1.5; `--max-bytes` leaked its validator's own wording for a fraction or an oversized value; `--recipe` named no accepted values though the server takes four; a blank message or a malformed conversation id reached the wire, and an empty `-c` was dropped from the body so the turn silently started a new conversation. Selecting rows to delete by neither filter nor id reported the opposite mistake, because the only message came from the refinement that also catches passing both. `workflows activate create` chooses which existing version serves live traffic, exactly as `rollback` does, and `rollback` is gated where it was not. `deploy` is left alone: it publishes the draft the caller just typed, it is additive, and rollback undoes it. `--limit 0` with run state walked the whole table in pages the server accepts one at a time, which is what its own rule exists to prevent. `--workspace` is a root flag, so it is parsed for the thirty-nine operations that have nowhere to put it and then dropped. That silence is what made a correctly-scoped route look like it was ignoring scope, so it warns now rather than refusing — too many commands take it harmlessly for a refusal to be safe. Messages that named a request field the caller cannot type were only ever half rewritten, because a single-word field is indistinguishable from an English word. A message is now translated whole or left as the server sent it, so no sentence mixes the two vocabularies.
… slots it cannot show A bulk document update dropped ids that matched nothing and said so only in a server log, then answered a selection where nothing matched with a 404 — refusing a request that had nothing to refuse. It was never atomic, so there was no guarantee to preserve. It now reports unmatched ids the way the chunk sweep beside it always has, and a selection that matches nothing is the same answer as one that matches some of what was asked for. Its count is renamed to match that sibling, which also corrects it: the update returns every row it matched, so the old name promised a count of what changed. Uploading a document could write a value into a tag slot the knowledge base has not defined. Nothing could then read it back — every filter refuses an undefined tag, and the app renders a slot only when a definition covers it. Refused now rather than defined automatically: a slot write carries no display name, so inventing one would put a name the caller never chose into the vocabulary every later reader sees. Clearing such a slot still works, so anything already written can be removed. Reading an archived knowledge base answers not-found, which is deliberate — the same answer conceals a workspace the caller cannot reach, so the message cannot say more without telling the two apart. The parameter now says which lifecycle it addresses, which reaches both the reference and the terminal.
…are scoped to Nothing asserted that a secret list is narrowed to its caller's own rows. The query is mocked, so its canned result proved nothing about what was asked for, and removing the narrowing left the suite green. The same was true of the refusal that keeps a workspace key — which acts for a workspace, not a person — out of a personal secret list: with the policy flipped the use case resolved and returned secrets. Both are pinned by the arguments the query is built from. Personal secrets remain invisible from a workspace the caller reaches through inherited organization access. Their metadata has no canonical row, only per-workspace mirrors written for a narrower set of workspaces than a caller can authorize into, so a list scoped to one workspace cannot find them while set and delete, which key on the person, work. Dropping the scope would return one row per mirror and break pagination, and a mirror's timestamps describe when it was written rather than when the secret was made, so the sort has no meaning. Closing it is a storage change; the divergence is recorded where the read happens and stated in the contract rather than left implied. Account-wide billing is requested by omitting the workspace, which is indistinguishable from an ordinary request, so a workspace key asking for it is answered with its own workspace rather than refused. The refusal belongs where the intent exists, which is the client; the contract now says what omission means for each kind of key, and that a workspace-scoped page reports its members in aggregate and names none of them. A run is addressed by an id that already names its workspace, so that read takes no workspace and refuses one that is sent. Unlike its siblings, nothing said so.
Three list surfaces describe the same filter, and one still named the request field rather than the concept — the spelling a caller reading the reference wants and the one a caller typing a command cannot use. The workflows sibling was reworded already; tables and knowledge now match, and a test pins the three together so they cannot drift apart again. The two bulk knowledge surfaces are pinned against each other rather than against a literal, since what matters is that they answer the same question under the same names.
Carries the contract wording and the new bulk-document response shape into the published reference, the generated command surface, and the command pages.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile SummaryThe PR fixes live-tested CLI and API defects involving validation, output semantics, authorization coverage, and transactional tag handling.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/knowledge/application/documents.ts | Passes knowledge-base context into document updates so tag-slot validation occurs under the write transaction’s lock. |
| apps/sim/lib/knowledge/documents/service.ts | Revalidates final tag-slot values after acquiring the knowledge-base lock, closing the previously reported create and update races. |
| apps/sim/lib/knowledge/tags/service.ts | Uses the shared knowledge-base row lock throughout tag clearing and definition deletion. |
| packages/sim-cli/src/runtime/options.ts | Strengthens CLI option validation for constrained numeric and enumerated inputs. |
| packages/sim-cli/src/runtime/execute.ts | Updates execution behavior for workspace handling and command outcomes without leaving a blocking issue related to the prior threads. |
Reviews (4): Last reviewed commit: "fix(knowledge): check the slot values ac..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The guard added here read the definitions and then let the write happen in a later transaction that locks only the document row. Tag deletion locks the knowledge-base row and, inside it, clears the slot and drops the definition — so the two shared no lock and a deletion committing in that gap left a value in a slot nothing defines. The check now runs inside the writing transaction, after the same knowledge-base row lock every other writer here takes first. The guard already accepted a transaction and was written to be called this way. This also covers a case the earlier placement missed: an update given tag definition ids resolves them to slots before writing, and a deletion after that resolution stranded a value the pre-write check never looked at. It removes a lock-order inversion rather than adding one. Clearing a slot took document then embedding while an update took embedding then document, which is a deadlock window between two transactions that shared no lock. Taking the knowledge-base lock first orders them. The lock is taken only when an update actually writes a slot, so a rename, an enable, or the processing-status writes on the ingest path are unaffected. Note this narrows rather than introduces: before the guard, a single ordinary request could write an undefined slot with no concurrency at all.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Collaborator
Author
Collaborator
Author
|
/cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 64 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
The bulk document sweep reports which of the ids it was given matched nothing, but only the v2 contract declared the field. The internal contract is a plain object, so its response validator dropped it — the surface that already logged the misses server-side still told its callers nothing. It is declared now, and required, because both selections always return one. Its count keeps its internal name. The v2 wire renamed it, and the internal contract passes the use case through verbatim, so the two never agreed and the internal name still matches its source; six call sites read it. The name guard added here refused control characters but not the two Unicode line separators, which the storage key strips the same way and a terminal renders as a break — the same divergence between stored and shown that the guard exists to stop. It now refuses the set the config writer already refuses for its own reasons, and the message no longer calls a separator a control character. A sweep selected by --select-all sends no ids, so measuring the request by its id count read a whole-knowledge-base no-op as nothing having been asked. Every sibling either requires ids or measures the miss from the response, so this was the only one. The archived half of four scope descriptions dropped its relative pronoun and read as nonsense once a second clause was added. Rewritten as one passive clause across all four, with a restore named only where one exists, and pinned so a reword of any single one cannot bring the others back.
Collaborator
Author
Collaborator
Author
|
/cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 71 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… they arrived Four ways past the same guard, all one mistake: it was attached per call site on a belief about how the values were produced, rather than to the values about to be written. A tags payload that is truthy but not a JSON array, or not JSON at all, left the caller's raw slot values in place while the guard was skipped, because the flag keyed on the payload being present rather than on the name-keyed path having run. It is unconditional now, inside the transaction, against the values being inserted. That also closes a read taken before the lock on the successful path, which a deletion could invalidate before the insert — the same check-then-act the previous commit closed for the other branch. The predicate deciding whether an update writes a slot trimmed before testing, while every writer clears only on the exact empty value. So a whitespace-only value skipped both the lock and the check and was then stored: verbatim for a text tag, as zero for a number, as false for a boolean. It now tests exactly what the writers test, so the two cannot disagree again. A value with meaningful surrounding space was a write before and still is. The bulk upload path accepted the same slots and had no guard at all. Its transaction already takes the row lock first, so the check is one read for the batch rather than one per document. The shape underneath is still worth changing: three write sites each hand-wire both the lock and the check, and nothing makes a fourth do either. A seam owning "write the tag columns" would remove the chance to forget, and is left as follow-up rather than carried here.
Collaborator
Author
|
Note for reviewers on the tag-slot guard, since five defects have now been found in it across this PR. They are one mistake instantiated five times, not five cases. The guard was attached per call site, conditioned on a belief about how the values were produced, rather than unconditionally at the point the columns are written against the values actually being written:
Each is now fixed against the values being written, inside the transaction, under the lock every tag mutation takes first. The shape underneath is still worth changing, and I have deliberately not changed it here. There are three independent document-tag write sites, and each hand-wires both the row lock and the guard. Nothing in the types or the call graph forces a fourth site — or a future edit to an existing one — to do either. The clean shape is one in-transaction seam owning "write the tag columns" that takes the lock, runs the check and performs the write, with all three sites going through it, so the guard stops being something a call site opts into. That is a larger refactor than a review round should carry, so it is filed as follow-up. To be explicit about the residual risk rather than imply it is closed: the current shape does not prevent a sixth bypass, it just does not currently have one. |
Collaborator
Author
Collaborator
Author
|
/cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 71 files
Confidence score: 2/5
apps/sim/lib/knowledge/tags/service.tscan race with concurrent cleanup, allowing a definition to be deleted between the read and subsequent insert and leaving a value in an undefined slot—serialize the read and insert with cleanup under the knowledge-base lock.apps/sim/lib/knowledge/documents/service.tscan retaintag1: ''as the representative for a bulk batch, causing a later nonempty value to bypass definition validation—replace an empty representative when a later nonempty value is found.apps/sim/lib/knowledge/documents/service.tscan let an empty first record suppress validation for a later write to the same slot—skip empty values when buildingbatchTagSlotValues.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/knowledge/tags/service.ts">
<violation number="1" location="apps/sim/lib/knowledge/tags/service.ts:282">
P1: When cleanup runs concurrently with a document write, this read can pass before cleanup deletes the definition without the knowledge-base lock; the subsequent insert then strands a value in an undefined slot. Run cleanup under `lockKnowledgeBaseForTagMutation` and perform its counts and deletes in that transaction.
(Based on your team's feedback about validating tag slots under the knowledge-base row lock.)</violation>
</file>
<file name="apps/sim/lib/knowledge/documents/service.ts">
<violation number="1" location="apps/sim/lib/knowledge/documents/service.ts:2252">
P1: When the first bulk document supplies `tag1: ''` and a later document supplies a nonempty `tag1`, this keeps `''` as the representative and bypasses the definition check. Replace an empty representative when a later nonempty value appears, so the later document cannot insert an unreadable undefined-slot value.</violation>
<violation number="2" location="apps/sim/lib/knowledge/documents/service.ts:2252">
P2: When the first batch record clears a slot and a later record writes it, the empty representative suppresses validation for the later value. Skip empty values when building `batchTagSlotValues`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const writtenSlots = collectWrittenTagSlots(slotValues) | ||
| if (writtenSlots.length === 0) return | ||
|
|
||
| const definitions = await getDocumentTagDefinitions(knowledgeBaseId, txDb) |
There was a problem hiding this comment.
P1: When cleanup runs concurrently with a document write, this read can pass before cleanup deletes the definition without the knowledge-base lock; the subsequent insert then strands a value in an undefined slot. Run cleanup under lockKnowledgeBaseForTagMutation and perform its counts and deletes in that transaction.
(Based on your team's feedback about validating tag slots under the knowledge-base row lock.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/knowledge/tags/service.ts, line 282:
<comment>When cleanup runs concurrently with a document write, this read can pass before cleanup deletes the definition without the knowledge-base lock; the subsequent insert then strands a value in an undefined slot. Run cleanup under `lockKnowledgeBaseForTagMutation` and perform its counts and deletes in that transaction.
(Based on your team's feedback about validating tag slots under the knowledge-base row lock.) </comment>
<file context>
@@ -203,6 +204,89 @@ export async function getNextAvailableSlot(
+ const writtenSlots = collectWrittenTagSlots(slotValues)
+ if (writtenSlots.length === 0) return
+
+ const definitions = await getDocumentTagDefinitions(knowledgeBaseId, txDb)
+ const definedSlots = new Set(definitions.map((definition) => definition.tagSlot))
+ const undefinedSlots = writtenSlots.filter((slot) => !definedSlots.has(slot))
</file context>
| boolean3: processedTags.boolean3 ?? null, | ||
| } | ||
| for (const [key, value] of Object.entries(baseDocument)) { | ||
| if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) { |
There was a problem hiding this comment.
P1: When the first bulk document supplies tag1: '' and a later document supplies a nonempty tag1, this keeps '' as the representative and bypasses the definition check. Replace an empty representative when a later nonempty value appears, so the later document cannot insert an unreadable undefined-slot value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/knowledge/documents/service.ts, line 2252:
<comment>When the first bulk document supplies `tag1: ''` and a later document supplies a nonempty `tag1`, this keeps `''` as the representative and bypasses the definition check. Replace an empty representative when a later nonempty value appears, so the later document cannot insert an unreadable undefined-slot value.</comment>
<file context>
@@ -2239,6 +2248,11 @@ export async function createDocumentRecords(
boolean3: processedTags.boolean3 ?? null,
}
+ for (const [key, value] of Object.entries(baseDocument)) {
+ if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) {
+ batchTagSlotValues[key] = value
+ }
</file context>
Suggested change
| if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) { | |
| if ( | |
| value !== null && | |
| value !== undefined && | |
| (batchTagSlotValues[key] === undefined || batchTagSlotValues[key] === '') | |
| ) { |
| boolean3: processedTags.boolean3 ?? null, | ||
| } | ||
| for (const [key, value] of Object.entries(baseDocument)) { | ||
| if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) { |
There was a problem hiding this comment.
P2: When the first batch record clears a slot and a later record writes it, the empty representative suppresses validation for the later value. Skip empty values when building batchTagSlotValues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/knowledge/documents/service.ts, line 2252:
<comment>When the first batch record clears a slot and a later record writes it, the empty representative suppresses validation for the later value. Skip empty values when building `batchTagSlotValues`.</comment>
<file context>
@@ -2239,6 +2248,11 @@ export async function createDocumentRecords(
boolean3: processedTags.boolean3 ?? null,
}
+ for (const [key, value] of Object.entries(baseDocument)) {
+ if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) {
+ batchTagSlotValues[key] = value
+ }
</file context>
Suggested change
| if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) { | |
| + if (value !== null && value !== undefined && String(value) !== '' && batchTagSlotValues[key] === undefined) { |
Collaborator
Author
|
Closing to re-validate from scratch. Five review rounds produced ten defects in one guard, and two of the most recent were introduced by the previous round's own fix. That pattern is evidence the changes want auditing as a whole rather than another patch on top — so I am enumerating the entire diff and validating each change independently for correctness, minimality, and unintended consequences before reopening. The branch is unchanged and nothing is lost; this will return as a fresh PR once that audit is done. |
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.
Summary
Defects found by driving the published
simpackage against a live server — all ~220 leaf commands across 26 groups. Everything here reproduced against the real binary before it was touched, and eight further reports were investigated and rejected as false positives rather than "fixed".Silent wrong answers
404— refusing a request that had nothing to refuse. It was never atomic, so there was no guarantee to preserve. It now reports unmatched ids the way the chunk sweep beside it always has. Its count is renamed to match, which also corrects it: the update returns every row it matched, so the old name promised a count of what changed.Flags the CLI documented as constrained and then transmitted anyway
--limiton the two filtered row mutations says "0 is not accepted" and sent0,-1,1.5.--max-bytesleaked its validator's own wording for a fraction or an oversized value.--recipenamed none of its four accepted values.-cwas falsy, so it was dropped from the body and silently started a new conversation.Gaps and inconsistencies
workflows activate createchooses which existing version serves live traffic, exactly asrollbackdoes, and onlyrollbackwas gated.deployis deliberately left alone — it publishes the draft the caller just typed, it is additive, and rollback undoes it.--limit 0with run state walked the whole table in pages the server accepts one at a time, which is what its own rule exists to prevent.--workspaceis a root flag, so it is parsed for the 39 operations that have nowhere to put it and then dropped. That silence made a correctly-scoped route look like it was ignoring scope. It warns now rather than refusing — too many commands take it harmlessly for a refusal to be safe.Two untested authorization invariants, now pinned
Nothing asserted that a secret list is narrowed to its caller's own rows — the query is mocked, so its canned result proved nothing, and removing the narrowing left the suite green. The same was true of the refusal keeping a workspace key, which acts for a workspace rather than a person, out of a personal secret list: with the policy flipped the use case resolved and returned secrets.
Type of Change
Testing
Live, against a real server, using the published package rather than source. Every fix reproduced first; every test proven red by hand-reverting the source before it was accepted.
packages/sim-cli: 780 passingapps/simaffected suites: 8,922 passingbun run lint,check:audits(36),bun run test, both type-checks, all three generator--checks: passA cross-lane regression was caught by a guard added in this PR: making the document sweep answer
200would have silently turned a zero-match sweep from exit 1 into exit 0. It now has a bulk-outcome entry mirroring its sibling.Known limitations, deliberately not fixed here
--description nullcannot be expressed. JSONnulldoes clear these fields, but the generated field spec carries no nullability, so the CLI cannot tell which string flags accept it. Needsnullableon the spec first.Checklist