fix(cli): close follow-up gaps - #7137
Merged
Merged
Conversation
…al-looking ids One of the eight script tests was reachable from no entrypoint, so it had never run in CI — it passes, it was simply never invoked. This is the second time that hand-maintained list has drifted from the files beside it; the audit runner's own header records the first. The guard against a third is a `check:*` script rather than a test, because the runner derives its list from that namespace and so picks the guard up by name — a test would have had to be hand-added to the very list it guards. A published spec sat outside the generator's manifest and so outside its drift check, and carried six example ids with the texture of real generated ones rather than the pandigital placeholders the rest of the repo uses. The new check globs the directory instead of reading the manifest, since the manifest is what omitted the file. The one pre-existing borderline id is allowlisted with a reason: loosening the threshold to admit it would have hidden one of the six.
`sim profiles <anything>` still exited 0, so a probe reading the exit code to ask whether a command exists was told yes — the one group the earlier guard missed. The exemption was written for commands that are both a group and a leaf, but only `files restore` takes an operand; `profiles` takes none. Registering its listing as a default subcommand puts it back among the pure dispatchers the existing guard already covers, so the guard itself did not need widening. Three commands refuse a workspace API key and said nothing, while their menu siblings said so — reading as though they accept one. They are hand-written, so they never reached the code that appends the note. That note now comes from a helper taking the operation, so a command names the operation it invokes and the two cannot disagree, and a test fails if a hand-written command ever calls a restricted operation without it. A blank numeric value in a request body still became a real zero, the same coercion already fixed for query strings: the guard keyed off the slot when the distinction is the field's declared type. Twenty-one fields across fifteen operations were affected. An empty body string still clears a description. Blank values for the root endpoint, workspace and profile flags fell back to what was configured instead of being refused, and a whitespace workspace was accepted verbatim. A hand-written profile name carrying padding listed as reachable but resolved to defaults rather than erroring. Two schema descriptions named request fields that no flag spells, and a rejected value was echoed unredacted by four messages while their siblings redacted it. A write now re-emits a section header it was not asked to touch byte for byte. The blank-line normalisation around it is left alone: making the writer position-faithful is a change to its model, not a fix.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
waleedlatif1
changed the title
Contributor
Greptile SummaryThe PR closes several CLI correctness and documentation gaps, and the follow-up change completes the prior uppercase-UUID audit fix.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/check-spec-example-ids.ts | Matches UUIDs case-insensitively and normalizes every spelling before checking the approved and sentinel sets, fully resolving the prior thread. |
| packages/sim-cli/src/runtime/request.ts | Corrects blank body-field coercion while preserving intentional empty-string behavior for string fields. |
| packages/sim-cli/src/commands/auth.ts | Tightens profile command dispatch, override-error handling, and terminal-safe presentation. |
| packages/sim-cli/src/config/profile.ts | Strengthens validation for blank endpoint, workspace, and profile overrides. |
| packages/sim-cli/src/runtime/build.ts | Centralizes restricted-operation help descriptions for generated and hand-written commands. |
| scripts/check-script-test-coverage.ts | Adds an audit ensuring repository script tests remain reachable from the root test entrypoint. |
Reviews (4): Last reviewed commit: "fix(scripts): match the uuid sentinels e..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The pattern only recognised lowercase hex, so an uppercase id in a published spec was never examined and the audit reported success without having looked at it. Matching case-insensitively is not enough on its own: hex is case-insensitive, so a mixed-case id counts `A` and `a` as two digits and reports twenty distinct ones rather than sixteen. That inflated count clears the threshold the texture test uses to recognise a hand-authored placeholder, so a real id could have passed for one. The allowlist is an exact-string lookup and would likewise have missed an uppercase spelling of an entry. Both checks and the lookup now take a normalised id, while the finding still reports the spelling as it appears in the file.
Collaborator
Author
Collaborator
Author
|
/cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 29 files
Confidence score: 3/5
packages/sim-cli/src/commands/auth.tsrejectssim profiles helpandsim profiles help --helpas unknown commands, causing an expected help flow to exit with an error; includehelpin the known-command handling.packages/sim-cli/src/config/profile.tsallows blank--profile,--endpoint, or--workspacevalues to be reported as success becauseprofileListingContextswallows the guard error; preserve and rethrow override-validation errors so invalid input fails visibly.
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="packages/sim-cli/src/commands/auth.ts">
<violation number="1" location="packages/sim-cli/src/commands/auth.ts:774">
P2: When users invoke `sim profiles help` or `sim profiles help --help`, this hook treats Commander’s implicit `help` command as unknown and exits with an error. Include `help` in `known`, as `refuseHelpAfterUnknownCommand` does, so nested help remains reachable.</violation>
</file>
<file name="packages/sim-cli/src/config/profile.ts">
<violation number="1" location="packages/sim-cli/src/config/profile.ts:457">
P2: When `profiles` receives a blank `--profile`, `--endpoint`, or `--workspace`, this guard throws, but `profileListingContext` swallows the error and exits successfully after listing profiles. Preserve and rethrow override-validation errors before the broken-profile fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A blank root flag was refused everywhere except `profiles`, where the catch that lets a broken profile still list absorbed it and the command exited 0 after printing the table. The refusal now carries its own error class, which is what the listing rethrows on — the two are distinguished by type rather than by matching message text, and a genuinely broken profile still lists. The unknown-profile message redacted the name the caller typed but not the suggestion or the list of configured names beside it, which come from the same file and are equally attacker-influenced once it has been hand-edited. Those are redacted now, as is every other message in these two files that quotes a name read out of the config, and the profile listing flattens the names it renders the way it already flattened the error column. The example-id audit judged a uuid by its digit texture, on the premise that a real one essentially never looks hand-authored. Measured against ten million generated ids, 0.81% of them do — one in 124, where this change alone replaced six. Requiring each digit exactly twice takes that to zero but rejects all fourteen placeholders now in the specs, so it is no cheaper than the alternative. The audit now holds the eighteen ids the specs actually use, which is one file rather than the twenty-seven a reserved format would touch, and a new id fails until someone lists it — which is the review the check exists to force.
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 30 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Accepting any id built from at most two distinct hex digits let something through that was never on the approved list. A generated id essentially never has that shape, so the practical risk was small — but this check had just stopped being a shape test and become a list, and a structural exception is the one thing that undoes that. The two ids it exists for are the nil and max sentinels, and both are matched by value now.
Collaborator
Author
Collaborator
Author
|
/cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
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
Follow-ups found by black-box testing the shipped CLI after #7126, plus two pre-existing repo issues found alongside them. Every item was reproduced against the real binary before being changed; two reported issues turned out to be false positives and were left alone.
Refusals that didn't fire
sim profiles <anything>exited 0, so a probe reading the exit code to ask whether a command exists was told yes. It was the one group the earlier guard missed — exempted alongsidefiles restore, but only that one takes an operand. Registering its listing as a default subcommand putsprofilesback among the pure dispatchers the existing guard already covers, so the guard itself didn't need widening.0— the same coercion already fixed for query strings, where the guard keyed off the slot when the real distinction is the field's declared type. 21 fields across 15 operations were affected, including required ones. An empty body string still clears a description.--endpoint,--workspaceand--profilefell back to what was configured instead of being refused, and a whitespace-only workspace was accepted verbatim.Help that contradicted itself
sim secrets --help,listanddeletecarried the warning andsetdidn't, reading as thoughsetaccepts one. They're hand-written, so they never reached the code that appends it. The note now comes from a helper taking the operation, so a command names the operation it invokes and the two can't disagree, and a test fails if a hand-written command ever calls a restricted operation without it. 56 → 60 rendered commands.folderPaths,sortBy=startedAt). Rewritten to name the concept, since these strings serve both the REST reference and CLI help. Mentions that are a plain kebab transform (includeOutput→--include-output) were deliberately left.Smaller correctness
Pre-existing, found in passing
check:*script rather than a test, because the runner derives its list from that namespace and picks the guard up by name; a test would have had to be hand-added to the very list it guards.Type of Change
Testing
Every fix reproduced against the real binary first, then proven red before green by hand-reverting the source and watching the test fail.
packages/sim-cli: 758 passingapps/simcontract and v2 route suites: 1,892 passingbun run lint,bun run check:audits(now 35 — both new guards register themselves),bun run test(full chain), both type-checks, all three generator--checks: passNot changed, deliberately
1.5reaches the wire verbatim and the server rejects it with its own message. The field spec carries no bounds, so any would have to be invented.Checklist