chore: validate package-lock.json sync in release workflow#1
Closed
jerome-benoit wants to merge 1 commit into
Closed
chore: validate package-lock.json sync in release workflow#1jerome-benoit wants to merge 1 commit into
jerome-benoit wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds package-lock.json synchronization validation to the npm publish workflow to prevent releases with mismatched dependencies. The validation ensures the lockfile is up-to-date before building and publishing.
Key Changes:
- Adds a validation step that regenerates the lockfile and checks for modifications
- Fails the build job early if lockfile is out of sync with package.json
Comment on lines
+18
to
+21
| - name: Validate lockfile | ||
| run: | | ||
| npm install --package-lock-only | ||
| git diff --exit-code package-lock.json || (echo "package-lock.json is out of sync. Run 'npm install' and commit the changes." && exit 1) |
There was a problem hiding this comment.
The lockfile validation step is placed only in the build job but not in the publish-npm job. Since publish-npm runs independently (after checkout), it should also validate the lockfile before running npm ci to ensure consistency at the point of actual publishing.
Consider either:
- Adding the same validation step to the
publish-npmjob before line 43, or - Moving this validation to run in both jobs, or
- If the intent is to fail fast in the build job only, add a comment explaining why the publish-npm job doesn't need this validation.
| - name: Validate lockfile | ||
| run: | | ||
| npm install --package-lock-only | ||
| git diff --exit-code package-lock.json || (echo "package-lock.json is out of sync. Run 'npm install' and commit the changes." && exit 1) |
There was a problem hiding this comment.
The error message references npm install, but the recommended command for fixing lockfile sync issues should be npm install --package-lock-only to update only the lockfile without modifying node_modules.
Suggested message:
"package-lock.json is out of sync. Run 'npm install --package-lock-only' and commit the changes."
This is more precise and matches what the validation step itself does to check for sync issues.
Suggested change
| git diff --exit-code package-lock.json || (echo "package-lock.json is out of sync. Run 'npm install' and commit the changes." && exit 1) | |
| git diff --exit-code package-lock.json || (echo "package-lock.json is out of sync. Run 'npm install --package-lock-only' and commit the changes." && exit 1) |
Owner
Author
|
Closing this PR as it was created on the wrong repository. The correct PR is on upstream: BITASIA/sap-ai-provider#23 |
jerome-benoit
added a commit
that referenced
this pull request
May 5, 2026
- types.ts: parseFindingsSafe with partial recovery (finding #18) - concurrency-pool.ts: guard NaN/float/Infinity in constructor (#1,#4), JSDoc re-entrant warning (#2) - task-source.ts: NFKC normalize + extended blocklist in sanitizeForPrompt (#17,#20), Map lookup in validatePlan (#7) - refinement-loop.ts: readFileSync replaces execSync/sed (shell injection #5), radius used (#12), fallback hash hardened (#13), line clamping (#14), realpathSync symlinks (#15), file cache Map (#11), parseFindingsSafe (#18), nonce validation (#16), trivial match skip (#19), implementer try/catch (#7), ratchet guard round<=2 (#6), no duplicate nonLowFindings (#9) - finalizer.ts: pushBranch returns boolean (#22), crypto rescue suffix (#21), rollback error level (#8) - main.ts: TASK_TIMEOUT_MS + Promise.race timeout (#3), flat budget JSDoc (#10)
jerome-benoit
added a commit
that referenced
this pull request
May 5, 2026
* feat: implement sandcastle refinement loop with critic-based convergence Replace single-pass implement→review→merge with iterative implement↔critic loop per task. Key changes: - Orchestrator fetches and sanitizes issues (prevents prompt injection) - Implement↔Critic loop with deterministic dedup convergence - Critic produces structured findings (nonce-tagged JSON, zod-validated) - Decreasing iteration budget per round [100, 50, 25, 10, 10] - Host-side validation and rebase (no agent needed) - One PR per task (no merger agent) - Draft PR on non-convergence with outstanding findings listed Implements #110 * fix: address review findings (shell injection, false convergence, nesting, zod validation) - Replace execSync with execFileSync for gh pr create (prevents shell injection) - Guard parseFindings against empty matches (prevents false convergence) - Add try/catch on gh issue list startup call - Guard git push in rebase catch block - Extract finalizeIssue function (reduces nesting from 6+ to 3 levels) - Add zod schema for rawIssues (replaces unsafe 'as' cast) - Implement validation retry round per spec (one more implement→critic if budget remains) * fix: guard retry calls, split rebase logic, remove dead critic retry * fix: handle nullable issue body and guard JSON parse * fix: distinguish stalled from converged (re-reported findings → draft PR) * fix: LOW-only findings should not prevent convergence * fix: log validation errors, conditional checklist, derive PR title from labels * refactor: extract sandcastle into modular architecture Split main.ts (525 lines) into 6 self-contained modules: - types.ts: shared domain types (TaskSpec, Finding, LoopResult, FinalizeResult) - refinement-loop.ts: reusable implement↔critic loop engine - finalizer.ts: validation, rebase, PR creation - concurrency-pool.ts: semaphore utility - task-source.ts: TaskSource interface + GithubIssueSource - main.ts: 74-line thin orchestrator wiring all modules The refinement loop is now reusable by any task source (GitHub issues, CI failures, manual triggers) without coupling to the planner. * fix: centralize constants, fix PR type-of-change, sanitize titles * fix: full validation post-rebase, execFileSync for gh issue list * perf: skip critic when implementer produces 0 commits on round 2+ * fix: remove unsound type guard, filter unknown plan IDs, guard ConcurrencyPool, warn on planner exhaustion * feat: state-of-the-art algorithmic improvements - Budget: flat constant 50/round (ARCS/SWE-Agent pattern, replaces suboptimal decreasing schedule) - Dedup: context-hash fingerprint of ±3 lines (CodeQL/Qodana pattern, drift-safe) - Quality ratchet: rollback round on regression (findings increase → git reset --hard) - Log: post-rebase validation failure now logs stderr - Safety: rescue branch on push failure (preserves commits before sandbox disposal) * fix(sandcastle): resolve all algorithmic audit findings - Replace execSync+sed shell injection with readFileSync+join in hashContextLines (RCE, P0) - Fix quality ratchet to compare nonLowFindings.length not findings.length (logic bug, P1) - Replace title truncation with SHA-256 hash in findingKey (collision fix, P2) - Hash single target line + file salt instead of positional context window (line-drift fix, P2) - Replace overlapping regex quantifiers in sanitizeForPrompt (ReDoS O(n²), P2) - Replace Array FIFO queue with O(1) singly-linked list in ConcurrencyPool (P3) - Move totalCommits accumulation after ratchet check to fix phantom counting (P3) - Replace O(T×I) .find() with O(1) Map lookup in validatePlan (P3) * fix(sandcastle): harden subprocess calls and reduce cyclomatic complexity - Replace all execSync template literals with execFileSync (array args) to eliminate shell injection vectors from LLM-controlled data - Strengthen path traversal guard: resolve() + sep instead of join() - Validate SHA format (/^[0-9a-f]{40}$/) before git reset --hard - Decompose finalizeTask (CC 15→5) into runValidation, attemptRebase, pushBranch, buildPrArgs, extractStderr - Decompose runRefinementLoop (CC 18→8) into executeRound, deduplicateFindings, checkQualityRatchet - Replace process.exit(0) with natural script termination - Add title validation (length ≤200, no control chars) in validatePlan - Add console.warn in previously silent catch blocks * fix(sandcastle): address review findings — convergence, planner failure, rebase note - Treat newFindings=0 as 'converged' regardless of repeated non-LOW findings (previously marked 'exhausted', forcing draft PRs) - Set process.exitCode=1 when planner exhausts retries (prevents masking a broken planner as 'no work to do') - Pass rebaseSucceeded to buildPrArgs, append rebase failure note to PR body when branch is not rebased onto main - Reword sanitizeForPrompt JSDoc to accurately describe scope * fix(.sandcastle): address all algorithmic audit findings - types.ts: parseFindingsSafe with partial recovery (finding #18) - concurrency-pool.ts: guard NaN/float/Infinity in constructor (#1,#4), JSDoc re-entrant warning (#2) - task-source.ts: NFKC normalize + extended blocklist in sanitizeForPrompt (#17,#20), Map lookup in validatePlan (#7) - refinement-loop.ts: readFileSync replaces execSync/sed (shell injection #5), radius used (#12), fallback hash hardened (#13), line clamping (#14), realpathSync symlinks (#15), file cache Map (#11), parseFindingsSafe (#18), nonce validation (#16), trivial match skip (#19), implementer try/catch (#7), ratchet guard round<=2 (#6), no duplicate nonLowFindings (#9) - finalizer.ts: pushBranch returns boolean (#22), crypto rescue suffix (#21), rollback error level (#8) - main.ts: TASK_TIMEOUT_MS + Promise.race timeout (#3), flat budget JSDoc (#10) * fix: unref timeout timer to prevent process hang, catch critic throws * fix: count commits before break on critic failure (prevents work loss) * fix: suppress unhandled rejection from timeout promise on task success * fix: force process exit after completion (prevents hang from timed-out sandboxes) * fix: check findings null before commits zero (correct status on implementer crash) * fix: report known findings in PR body even when converged (prevents silent false convergence) * feat: state-of-the-art convergence improvements (ARCS, SWE-Agent, OpenHands) - Validation in-loop (ARCS): deterministic convergence when tests pass mid-loop - Best-state checkpoint (SWE-Agent): reset to best SHA on non-convergence - Severity-weighted convergence (OpenHands): refuse convergence if CRITICAL/HIGH persist * fix: move bestSha after ratchet, add validation timeout, fix severity/bestSha mismatch * fix: recount totalCommits from git after best-state reset (semantic correctness) * refactor(.sandcastle): address all 45 quality audit findings - New constants.ts: shared constants (VALIDATION_COMMAND, timeouts, model names) + utilities (getHeadSha, toErrorMessage) - refinement-loop.ts: decompose runRefinementLoop (CC 17→≤10), RoundContext/HashInput param objects, computeFindingKey rename - finalizer.ts: add timeouts to all execFileSync, use runValidation helper consistently - task-source.ts: add timeout, replace char loop with regex, fix terse names - main.ts: extract withTimeout helper, use model constants - types.ts: unexport FindingsSchema (internal only) * fix: use constants from constants.ts + add planner timeout (multi-agent audit findings) * refactor(.sandcastle): harden prompts — cap findings, add known decisions, scope preference * perf(.sandcastle): convert execFileSync to async execFileAsync (unblock event loop) Replace all blocking execFileSync calls with util.promisify(execFile) to enable true parallelism between tasks during subprocess execution. - constants.ts: add execFileAsync export, convert getHeadSha to async - refinement-loop.ts: captureHeadSha, checkQualityRatchet, checkConvergence, runMidLoopValidation, resetToBestState all async - finalizer.ts: runValidation, attemptRebase, pushBranch all async - task-source.ts: fetchAndSanitizeIssues async readFileSync/realpathSync stay sync (<1ms local I/O, no benefit from async). maxBuffer: 8MB added to validation and gh issue list calls. * fix: catch planner timeout rejection (retry instead of crash) + add catch type annotations
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
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.
Adds lockfile synchronization validation to the npm publish workflow to ensure reproducible installs.
Validates that
package-lock.jsonis synchronized withpackage.jsonbefore building and publishing the package. This prevents releases with mismatched dependencies that could lead to non-reproducible installations.Changes:
buildjob beforenpm cinpm install --package-lock-onlyand checks for lockfile modifications