fix: address unresolved review comments from #1058#1067
Merged
Conversation
Follow-up to #1058 (and #1059/#1063) addressing the review threads that remained open after the earlier follow-ups. Release commands — eliminate the silent `args.join(" ")` defect: - All version-only release commands (archive, restore, create, delete, deploys, finalize, view, set-commits) now declare a single `kind: "tuple"` positional so Stricli enforces arity. Previously `kind: "array"` + `args.join(" ").trim()` silently swallowed extra positionals (`restore 1.0.0 extra` → `"1.0.0 extra"`). - `release deploy` converts to a 3-positional tuple (`<version> <environment> [name]`); multi-word names must be quoted. - New `resolveReleaseTarget()` helper in release/parse.ts centralizes the version parse + org resolution + ContextError boilerplate. - New GritQL lint rule `no-args-join-in-release.grit` bans `args.join()` in release commands so the anti-pattern cannot be reintroduced. sourcemap resolve — single pass over files for the resolved/withDebugId counters instead of two `.filter()` calls. sourcemap inject — use an anchored `^https?://` regex for remote sourcemap detection; replace locale-dependent `localeCompare` path sort with a byte-wise comparison (matching upload.ts). proguard — precompute the namespace bytes once instead of reparsing the hyphenated UUID string on every `uuidV5()` call. Behavior-preserving (golden vectors unchanged). Regenerated skill/docs reference files reflect the single-positional signatures.
Contributor
|
Contributor
Codecov Results 📊✅ Patch coverage is 81.16%. Project has 4427 uncovered lines. Files with missing lines (9)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 81.86% 81.81% -0.05%
==========================================
Files 341 341 —
Lines 24359 24336 -23
Branches 15915 15911 -4
==========================================
+ Hits 19939 19909 -30
- Misses 4420 4427 +7
- Partials 1663 1661 -2Generated by Codecov Action |
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-up to #1058 that addresses the review threads still open after the
earlier follow-ups (#1059, #1063). Each thread is mapped below.
Release commands — eliminate the silent
args.join(" ")defect (thread 3349270396)The sentry bot flagged that
release restore 1.0.0 extrasilently joins extrapositionals into
"1.0.0 extra". Thisargs.join(" ").trim()pattern was sharedby all release commands, so rather than a one-off guard this fixes the whole
class:
archive,restore,create,delete,deploys,finalize,view,set-commits) now declare a singlekind: "tuple"positional, so Stricli enforces arity — extra args arerejected instead of silently swallowed.
release deploybecomes a 3-positional tuple (<version> <environment> [name]);multi-word deploy names must be quoted (e.g.
"Deploy #42"), matching standardCLI conventions and the existing docs example.
resolveReleaseTarget()helper insrc/commands/release/parse.tscentralizes the version-parse + org-resolution +
ContextErrorboilerplate.lint-rules/no-args-join-in-release.gritbansargs.join()insrc/commands/release/so the anti-pattern cannot return(verified it fires on a reintroduced
args.join).sourcemap resolve— single counting loop (thread 3356439786)Replaced two
files.filter(...)passes with onefor...ofloop tallying bothresolvedandwithDebugId.sourcemap inject— remote detection + sort (threads 3356454439, 3356458605)^https?://regex instead oftwo
startsWithchecks.localeCompare(matching the plain
.sort()already used inupload.ts).proguard— precompute namespace bytes (threads 3356471815, 3356474050)uuidV5()reparsed the hyphenated namespace UUID into bytes on every call. Thenamespace is fixed, so the bytes are now precomputed once at module load.
Behavior-preserving — golden vectors unchanged (
void\n→5db7294d-87fc-5726-a5c0-4a90679657a5, namespace4f44f30f-...).Already handled (resolving with notes)
hasSourcemap().The inject-hint sub-point is a non-issue —
sourcemap injectadds debug IDs tothe JS file itself regardless of where its sourcemap lives, so the hint is correct.
Testing
bun run typecheck,bun run lint(incl. new GritQL rule),check:fragments,check:errorsall green.resolveReleaseTargetunit tests; updated therelease deployandsourcemap resolvetests for the new signatures.