StarSync 2.4.0 - #10
Merged
Merged
Conversation
The tier-2 pattern file is per-machine, not per-repo, so it names every private sibling — including whichever repository is being committed to. A repository that carried the guard was blocked from writing its own name in ordinary prose, which is why the guard had been confined to the public repos: measured against the local pattern file, 8 of the 11 Spernakit-derived apps hit this. Tier 2 now drops the patterns that match the repository's own directory name and keeps the rest. A repository cannot leak its identity to itself — the name is already its directory, its remote URL, and its package name — while its siblings' names stay guarded, which is the whole reason tier 2 exists. A pattern grep cannot compile is kept rather than dropped, so the filter fails closed. check-leak-guard.sh gains two cases for that boundary and a second scratch repo whose name is what makes the distinction observable. Both files stay byte-identical with the aidd and spernakit copies. Co-Authored-By: Claude Opus 5 <[email protected]>
The lockfile lists every platform's variant of a native package, but an install materializes only the ones the machine matches. A consumer reading each resolved package off disk could not tell "not installed because this platform does not use it" from "not installed because the tree is stale", and only the lockfile knows. LockedPackage now carries an optional platformGated flag, set when the entry names any os, cpu, or libc constraint. Presence rather than evaluation: bun records os and cpu but not libc, so a musl build and a glibc build are indistinguishable here, and evaluating the constraint would call one of them installable on a Linux host that will never install it. Synced from Spernakit by scripts/sync-license-core.ts. Co-Authored-By: Claude Opus 5 <[email protected]>
…position The header named .githooks/leak-guard.sh, but package.json invokes .githooks/leak-guard-setup.sh; it also named prepublishOnly alone, where the guard runs from bun install and from check:leak-guard inside smoke:qc, which prepublishOnly gates on. Separately, the header explained that the shell scripts cannot carry the Windows bash-resolution fix but never said what governs this file itself, which two planning documents then read as a claim of byte-identity and recorded as drift. aidd's leak-guard contract already lists it under SEEDED_SCRIPTS; the header now says so. Co-Authored-By: Claude Opus 5 <[email protected]>
Synced from spernakit v3.37.0 via `bun run licenses:sync-core`. A package that declares no os, cpu, or libc constraint of its own can still be unreachable on every ordinary install when the only paths to it run through gated parents. Gating now travels along each edge, and an unrestricted arrival overwrites a gated one so the result does not depend on traversal order. Co-Authored-By: Claude Opus 5 <[email protected]>
starsync already received the license core and the leak-guard suite by sync, but carried nothing that could tell it when one of those copies had gone stale. It now carries sync-shared-core.ts and its manifest, wired as check:shared-core and run by smoke:qc, so a drifted copy fails here rather than only in the repository that owns it. This repository owns no group, so --write refuses everything from here by design; --check is exempt and verifies all six groups from anywhere. Rosters name private siblings and this repository is public, so the three roster paths are gitignored even though nothing here can produce one. check-license-core.ts gains the Enforces: line the gate-conventions rule requires. It is a synced file and spernakit's copy has moved further; the next resync replaces this one. Co-Authored-By: Claude Opus 5 <[email protected]>
Both files are spernakit-owned and were migrated to the gate conventions there. This is the delivery, run from spernakit with --write; the copies are byte-identical to their source and were not edited here. Co-Authored-By: Claude Opus 5 <[email protected]>
…targets The gate-conventions group now carries scripts/check-script-targets.ts. starsync is not on that group's roster, so this repository takes the manifest without the file. Co-Authored-By: Claude Opus 5 <[email protected]>
Synced from spernakit, which owns scripts/shared-core-manifest.json. starsync is a carrier of the manifest, not a target of the env-spread group: that group's roster names only the repositories where a subprocess-environment policy exists to enforce, and starsync is not one of them today. Co-Authored-By: Claude Opus 5 <[email protected]>
Synced from spernakit, which owns the manifest. starsync holds no roster seat for this group; the entry arrives so the manifest stays byte-identical fleetwide. Co-Authored-By: Claude Opus 5 <[email protected]>
Manifest-only. starsync has no backend/src/db, so it is not on the portable-gates roster and receives no gate file. Co-Authored-By: Claude Opus 5 <[email protected]>
The aidd-owned `env-spread` group now carries `check-audit-artifact-hygiene.ts` alongside `check-env-spread.ts`, so it is renamed `portable-gates-aidd`. This repository holds no seat on that group's roster and receives no gate from it; only the manifest itself is delivered here. Co-Authored-By: Claude Opus 5 <[email protected]>
Received by `sync-shared-core.ts --write` from spernakit, which owns this group. The gate reported `[OK] Shared core: no drift` having compared nothing. Its inputs are sibling checkouts, so two zero cases exist and only one is a defect: no peer checked out is legitimate and is now `[SKIP]` with the reason, while an `--only` that matched no target is now `[FAIL]`. The clean line states what it examined (388 files across 135 targets). Co-Authored-By: Claude Opus 5 <[email protected]>
The portable-gates group now carries check-docs.ts and its waiver library. starsync holds no seat on that group's roster, so this is the manifest copy only; the gate itself is not delivered here. Co-Authored-By: Claude Opus 5 <[email protected]>
…fest closure The portable-gates group gains the gate and its two library files. starsync holds no seat on that roster and has no frontend, so it carries the manifest rather than the gate; the entry keeps the closure identical across all three carriers, which is what check:shared-core compares. Co-Authored-By: Claude Opus 5 <[email protected]>
Resynced from spernakit, which owns this group. The `gate-conventions` group gained `lib/gate/fixtures.ts` and `lib/gate/vacuity.ts` when GC5 was given a static form; without these two entries the sync reports the group as covered while leaving both files unmanaged here. Co-Authored-By: Claude Opus 5 <[email protected]>
Delivered by `sync-shared-core.ts --write`. The `gate-conventions` group gained `docs/reference/gate-conventions.md`; starsync is not on that group's roster and carries the manifest only as a member of `shared-core-sync`, so nothing else changes here. See spernakit 25c5db0. Co-Authored-By: Claude Opus 5 <[email protected]>
Synced from spernakit, which owns the shared-core-sync group. Discovery now resolves a discovered group's targets by the union of its marker and already carrying every one of the group's synced files, so a repository seeded before the marker described it keeps receiving updates to files it already holds. Closes punchlist C1. Co-Authored-By: Claude Opus 5 <[email protected]>
…s it screenshot-guard.sh decided whether a repository captures by testing for a screenshots/ directory, which is gitignored wherever the guard runs, so the predicate answered from untracked local state. The opt-in is now a tracked .screenshot-capture file. This repository does not capture release screenshots and so does not carry one; the guard reports and passes. Delivered by sync-shared-core.ts --write --group push-guards from aidd, which owns it. Closes punchlist C7. Co-Authored-By: Claude Opus 5 <[email protected]>
Picks up the stated decline (`.no-fleet-sync`, honored only where the repository has no push remote) and the eligibility.ts extraction that carries it. Co-Authored-By: Claude Opus 5 <[email protected]>
…nifest Synced from spernakit (bb801d6). `lib/destructive/comments.ts` joins the portable-gates group's file list so the gate does not sync ahead of a module it imports. This repository is not a portable-gates target, so the manifest entry is the whole change here. Co-Authored-By: Claude Opus 5 <[email protected]>
The header is the one secret format whose giveaway string is also a legitimate constant: a config validator compares an incoming key against it, and documentation shows the shape a key takes. The guard scans staged additions only, so content committed before it existed never fires again, which is why this only surfaced when a scaffolded project staged every template file at once and could not make its first commit. The rule now requires the key material as well: base64 following the header on the same line, which is how a one-line JSON or .env value carries a whole key, or an added line of nothing but base64, which is how a pasted body looks. A placeholder remainder matches neither. Written in grep and sed rather than awk because CI runs on Ubuntu, whose mawk has no interval support. Synced from aidd, which owns this file. Co-Authored-By: Claude Opus 5 <[email protected]>
The PEM rule walked staged additions, so it could only see a header and a body when the same commit added both. A body pasted under a header that was already committed puts no header in the additions set at all, and that is the likeliest way the leak actually happens: the placeholder header is left behind by an earlier commit and the key goes in underneath it later. The walk now runs over a diff carrying one line of context. Context lines make the adjacency visible while '+' still marks what the commit is adding, so inline material is only reported on an added header. Reporting it on a committed one would block every later commit to that file with no way to clear it short of the bypass. Synced from aidd, which owns these files. Co-Authored-By: Claude Opus 5 <[email protected]>
spernakit now owns third-party-licenses/resolve.ts as a sync group rather than leaving it to template drift. This repository's copy already matched, so only the manifest changes. Co-Authored-By: Claude Opus 5 <[email protected]>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughThe change adds manifest-driven shared-core validation and synchronization, improves leak and screenshot guard detection, tracks platform-gated license dependencies, updates package metadata, and documents the 2.4.0 release. ChangesShared Core
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as sync-shared-core
participant Manifest as loadManifest
participant Checker as checkGroup
participant Writer as applyFindings
participant Repository as Target repository
CLI->>Manifest: Load synchronization groups
Manifest-->>CLI: Return validated groups
CLI->>Checker: Check selected groups and targets
Checker-->>CLI: Return findings
CLI->>Writer: Apply writable findings
Writer->>Repository: Write files and executable hooks
Repository-->>Writer: Return write outcome
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary>
### ❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
| :----------------: | :--------- | :------------------------------------------------------------------------------------ | :--------------------------------------------------------------------------------- |
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
<details>
<summary>✅ Passed checks (4 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :------------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly identifies the StarSync 2.4.0 release, which matches the primary objective of the changeset. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches 💡 1</summary>
<!-- finishing_touch_suggestion:docstrings -->
<details>
<summary>📝 Generate docstrings 💡</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- This is an auto-generated comment: all tool run failures by coderabbit.ai -->
> [!WARNING]
> There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
>
> <details>
> <summary>🔧 ESLint</summary>
>
> > If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
>
> ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.
>
>
>
> </details>
<!-- end of auto-generated comment: all tool run failures by coderabbit.ai -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
scripts/lib/shared-core/dispatch.ts (2)
101-109: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
.gitis not always a directory.For a linked worktree or a submodule,
.gitis a file that points at the real git directory. Line 106 then builds a path that does not exist, line 108 returnsnull, andcheckGroupclassifies every chained guard in that target asnot-applicable. Such a target reads as intentionally uncovered rather than as unknown. Resolving the hooks directory withgit -C <repoPath> rev-parse --git-path hooksreports the correct location in all layouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/shared-core/dispatch.ts` around lines 101 - 109, Update dispatcherBody to resolve the default hooks directory through Git using repoPath (for example via git -C ... rev-parse --git-path hooks) instead of assuming repoPath/.git/hooks, while preserving configured non-empty paths. Ensure the resolved directory is used to read hookName so linked worktrees and submodules are handled correctly.
23-30: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
--localignores a global or systemcore.hooksPath.
git config --localreads only.git/config. If a target repository inheritscore.hooksPathfrom the global or system scope, this returns''.checkGroupinscripts/lib/shared-core/check.tsthen setsdispatchesto false and reportsunmanaged-dispatch, which no write can clear. Dropping--localmakes the query report the value git actually uses.♻️ Proposed change
- const result = Bun.spawnSync(['git', '-C', repoPath, 'config', '--local', 'core.hooksPath'], { + const result = Bun.spawnSync(['git', '-C', repoPath, 'config', 'core.hooksPath'], {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/shared-core/dispatch.ts` around lines 23 - 30, Update hooksPath to query Git’s effective core.hooksPath configuration instead of restricting the command to the local scope; remove the --local argument from the git invocation while preserving the existing success check and trimmed empty-string fallback.scripts/lib/shared-core/manifest.ts (2)
262-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the
JSON.parsecall so a malformed manifest reports the path.Every other failure in this loader routes through
failand is prefixed withshared-core-manifest.json:. A syntax error at line 267 escapes as a bareSyntaxErrorfromJSON.parse, which names neither the manifest norpath.♻️ Proposed change
- const parsed = JSON.parse(readFileSync(path, 'utf8')) as unknown; + let parsed: unknown; + try { + parsed = JSON.parse(readFileSync(path, 'utf8')) as unknown; + } catch (error) { + fail(`${path} is not valid JSON: ${(error as Error).message}`); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/shared-core/manifest.ts` around lines 262 - 272, Wrap the JSON.parse call in loadManifest with error handling so malformed JSON is reported through fail with the manifest path included, matching the loader’s existing error prefixing. Preserve successful parsing and the current validation flow for valid manifests.
230-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
requiresPackageJsonexplicitly instead of accepting any value.Line 235 only tests
!== true, and line 258 only copies the field when it is exactlytrue. A manifest that writes"requiresPackageJson": "true"or1therefore loads with the flag dropped.skipReasoninscripts/lib/shared-core/eligibility.tsthen compares targets that have nopackage.json, andresolveSourcefalls back for every target. The file header states the loader rejects rather than repairs, so a type error here should fail the load.♻️ Proposed validation
const rawFiles = group['files']; if (!Array.isArray(rawFiles) || rawFiles.length === 0) { fail(`${where} needs at least one file.`); } + const requiresPackageJson = group['requiresPackageJson']; + if (requiresPackageJson !== undefined && typeof requiresPackageJson !== 'boolean') { + fail(`${where} requiresPackageJson must be a boolean when present.`); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/shared-core/manifest.ts` around lines 230 - 243, Validate requiresPackageJson explicitly as a boolean whenever the field is present, rejecting values other than true or false during manifest loading. Update the validation near the wiring checks and ensure the existing assignment logic preserves the validated boolean instead of silently dropping non-boolean values; keep the loader’s reject-on-invalid-input behavior.scripts/run-bash.ts (1)
13-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale authority reference.
scripts/shared-core-manifest.jsonnow definesscripts/run-bash.tsas seeded andscripts/check-leak-guard.shplus.githooks/leak-guard-setup.shas synced. Replace theaidd/scripts/lib/leak-guard/contract.tsreference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-bash.ts` around lines 13 - 20, The comments in scripts/run-bash.ts reference the obsolete authority file. Update that reference to scripts/shared-core-manifest.json, preserving the existing explanation that run-bash.ts is seeded while the leak-guard shell scripts are synced.scripts/lib/shared-core/write.ts (1)
100-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winContain spawn failures so the documented partial-result contract holds.
Lines 97-98 state that failures are reported rather than thrown, because the content is already written.
Bun.spawnSyncat Line 110 throws if thegitbinary cannot be spawned. That exception escapesapplyFindings, aborts the remaining findings, and reports nothing about the file already written at Line 171.Wrap the spawn so every failure path returns the note.
♻️ Proposed fix
- const result = Bun.spawnSync( - ['git', '-C', targetRoot, 'update-index', '--add', '--chmod=+x', relativePath], - { stderr: 'pipe', stdout: 'pipe', windowsHide: true }, - ); - if (result.exitCode === 0) return null; - const detail = result.stderr.toString().trim(); - return `wrote ${relativePath} but could not record its executable bit${detail ? `: ${detail}` : ''}`; + let detail: string; + try { + const result = Bun.spawnSync( + ['git', '-C', targetRoot, 'update-index', '--add', '--chmod=+x', relativePath], + { stderr: 'pipe', stdout: 'pipe', windowsHide: true }, + ); + if (result.exitCode === 0) return null; + detail = result.stderr.toString().trim(); + } catch (err) { + detail = err instanceof Error ? err.message : String(err); + } + return `wrote ${relativePath} but could not record its executable bit${detail ? `: ${detail}` : ''}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/shared-core/write.ts` around lines 100 - 117, Update makeExecutable to catch exceptions from Bun.spawnSync, including failures to launch git, and return the same partial-result note format used for nonzero exit codes. Preserve the existing chmod handling and stderr detail when a process result is available, ensuring no spawn failure escapes to applyFindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.githooks/leak-guard.sh:
- Line 38: Update pem_body_pattern in .githooks/leak-guard.sh:38-38 to allow
horizontal whitespace between the diff-plus marker and the base64 body while
preserving the existing body validation. Add an executable regression case in
scripts/check-leak-guard.sh:66-75 covering an indented PEM header and indented
base64 body, and verify the leak guard detects it.
- Line 90: Update the leak-guard logic around local_hits so grep status 2 from
an invalid Tier 2 ERE causes the guard to fail, while status 1 remains the valid
no-match case; do not mask grep errors with unconditional || true. Add a
regression test covering an invalid active pattern and confirming staged
literals are rejected.
In @.githooks/screenshot-guard.sh:
- Around line 62-68: Update the CONTRACT_FILE handling in the screenshot guard
to verify that .screenshot-capture is tracked by Git before enabling
enforcement, rather than relying only on [ -f ]. Resolve the contract from each
release tag’s tree when enforcement is tag-specific, and preserve the existing
no-contract note behavior when the tracked declaration is absent.
In @.gitignore:
- Around line 36-41: Add the three missing root-anchored roster
patterns—portable-gates-aidd-targets.json, portable-gates-targets.json, and
license-core-adapter-targets.json—to .gitignore, matching the directory used by
resolveTargets. In scripts/shared-core-manifest.json, keep the manifest roster
filenames synchronized with the ignore list when groups are added; no direct
manifest change is required for the currently declared rosters.
In `@scripts/check-license-core.ts`:
- Around line 55-75: Update the closure validation around collectLockfileClosure
and the installed-package inspection so every required locked.unresolved entry
causes a failure, all installed packages are resolved and checked rather than
only installed[0], and the returned values from readLicenseText and
readNoticeText are validated for missing license or notice data. Preserve the
existing failure reporting conventions, or narrow the enforcement claim if this
command is intended only as a smoke test.
In `@scripts/lib/shared-core/dispatch.ts`:
- Around line 53-60: Update chainedByHook to resolve the hook entry in
group.files by its target-or-source name, read each variant through its source
path, and union the invoked names across all available hook variants. Preserve
excluding the hook itself from the returned set so checkGroup and
assertHookChainIsCarried evaluate the complete chained guard set.
In `@scripts/lib/shared-core/targets.ts`:
- Around line 126-132: Update the pushability check around the remotes probe so
any failed git remote -v execution is treated as pushable rather than as
evidence of no push remote. Only allow the opt-out when remotes.success is true
and stdout contains “(push)”; otherwise keep the existing return behavior that
refuses the decline.
- Around line 191-202: Update readScripts to propagate the JSON parse/read
failure from its catch block instead of returning null. Preserve the null result
only for a missing package.json, while allowing skipReason and resolveSource to
distinguish an unreadable file from an absent one.
- Around line 51-78: Update the target normalization map in targets.ts so
packageName is included only when raw declares an explicit packageName; remove
the fallback to directory while preserving directory validation, path
construction, and support for plain-string entries without a packageName.
- Around line 177-188: Update resolveTargets to validate every entry in only
against the resolved targets before filtering, and report or throw for any
unknown directory even when other requested names are valid. Preserve the
existing filtering behavior for valid names and the unrestricted behavior when
only is undefined.
In `@scripts/lib/shared-core/vacuity.ts`:
- Around line 21-40: Update reportClean to distinguish no targets because no
groups were compared from no targets because --only matched nothing. Use the
reports input to detect whether any group comparison occurred, and return the
existing [SKIP] result when all groups were omitted as unverifiable; retain the
--only error only when groups were compared but matched no targets.
In `@scripts/sync-shared-core.ts`:
- Around line 263-276: The owner baseline validation in the groups loop must
verify checkout identity, not only directory existence. Before calling
checkGroup, read and parse ownerRoot/package.json and require its name to equal
group.owner; if the directory is missing or the package identity is invalid or
mismatched, fail the check rather than adding the group as unverifiable and
allowing reportCheck to return success.
---
Nitpick comments:
In `@scripts/lib/shared-core/dispatch.ts`:
- Around line 101-109: Update dispatcherBody to resolve the default hooks
directory through Git using repoPath (for example via git -C ... rev-parse
--git-path hooks) instead of assuming repoPath/.git/hooks, while preserving
configured non-empty paths. Ensure the resolved directory is used to read
hookName so linked worktrees and submodules are handled correctly.
- Around line 23-30: Update hooksPath to query Git’s effective core.hooksPath
configuration instead of restricting the command to the local scope; remove the
--local argument from the git invocation while preserving the existing success
check and trimmed empty-string fallback.
In `@scripts/lib/shared-core/manifest.ts`:
- Around line 262-272: Wrap the JSON.parse call in loadManifest with error
handling so malformed JSON is reported through fail with the manifest path
included, matching the loader’s existing error prefixing. Preserve successful
parsing and the current validation flow for valid manifests.
- Around line 230-243: Validate requiresPackageJson explicitly as a boolean
whenever the field is present, rejecting values other than true or false during
manifest loading. Update the validation near the wiring checks and ensure the
existing assignment logic preserves the validated boolean instead of silently
dropping non-boolean values; keep the loader’s reject-on-invalid-input behavior.
In `@scripts/lib/shared-core/write.ts`:
- Around line 100-117: Update makeExecutable to catch exceptions from
Bun.spawnSync, including failures to launch git, and return the same
partial-result note format used for nonzero exit codes. Preserve the existing
chmod handling and stderr detail when a process result is available, ensuring no
spawn failure escapes to applyFindings.
In `@scripts/run-bash.ts`:
- Around line 13-20: The comments in scripts/run-bash.ts reference the obsolete
authority file. Update that reference to scripts/shared-core-manifest.json,
preserving the existing explanation that run-bash.ts is seeded while the
leak-guard shell scripts are synced.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f2a215a-14c4-4501-8722-5348bd4ac4a5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.githooks/leak-guard.sh.githooks/screenshot-guard.sh.gitignoreCHANGELOG.mdREADME.mdpackage.jsonscripts/check-leak-guard.shscripts/check-license-core.tsscripts/lib/license-core/lockfile.tsscripts/lib/shared-core/check.tsscripts/lib/shared-core/dispatch.tsscripts/lib/shared-core/eligibility.tsscripts/lib/shared-core/manifest.tsscripts/lib/shared-core/owner.tsscripts/lib/shared-core/targets.tsscripts/lib/shared-core/vacuity.tsscripts/lib/shared-core/write.tsscripts/run-bash.tsscripts/shared-core-manifest.jsonscripts/sync-shared-core.tssrc/lib/help-text.ts
| # does not have to belong to this commit; the body is what leaks. | ||
| pem_header_pattern='-----BEGIN [A-Z ]*PRIVATE KEY-----' | ||
| pem_inline_pattern="${pem_header_pattern}.*[A-Za-z0-9+/]{20,}" | ||
| pem_body_pattern='^\+[A-Za-z0-9+/]{20,}={0,2}[[:space:]]*$' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Detect indented PEM body lines. A PEM value in an indented block, such as YAML, has body lines that begin with + in the diff. pem_body_pattern requires base64 immediately after +, so the guard permits the private key.
.githooks/leak-guard.sh#L38-L38: allow leading horizontal whitespace between+and the base64 body.scripts/check-leak-guard.sh#L66-L75: add a case with an indented header and indented base64 body.
📍 Affects 2 files
.githooks/leak-guard.sh#L38-L38(this comment)scripts/check-leak-guard.sh#L66-L75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/leak-guard.sh at line 38, Update pem_body_pattern in
.githooks/leak-guard.sh:38-38 to allow horizontal whitespace between the
diff-plus marker and the base64 body while preserving the existing body
validation. Add an executable regression case in
scripts/check-leak-guard.sh:66-75 covering an indented PEM header and indented
base64 body, and verify the leak guard detects it.
| printf '%s\n' "$self_name" | grep -qEi -e "$pattern" 2>/dev/null || printf '%s\n' "$pattern" | ||
| done)" | ||
| if [ -n "$active" ]; then | ||
| local_hits="$(printf '%s\n' "$added" | grep -nEi -f <(printf '%s\n' "$active") || true)" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
patterns="$(mktemp)"
trap 'rm -f "$patterns"' EXIT
printf '[\n' >"$patterns"
set +e
printf 'private-value\n' | grep -nEi -f "$patterns" >/dev/null 2>&1
status=$?
set -e
test "$status" -eq 2Repository: NomadicDaddy/starsync
Length of output: 159
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target file ---'
sed -n '1,180p' .githooks/leak-guard.sh
printf '%s\n' '--- related tests and references ---'
rg -n -S 'leak-guard|Tier 2|active|local_hits|grep -nEi' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 240Repository: NomadicDaddy/starsync
Length of output: 16715
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
run_case() {
name="$1"
shift
printf '%s\n' "$@" >"$tmp/patterns"
set +e
output="$(printf '%s\n' 'safe-literal' 'PRIVATE_VALUE' | grep -nEi -f "$tmp/patterns" 2>"$tmp/err")"
status=$?
set -e
printf '%s: status=%s output=%q stderr=%q\n' "$name" "$status" "$output" "$(cat "$tmp/err")"
}
run_case invalid_only '['
run_case valid_then_invalid 'PRIVATE_VALUE' '['
run_case invalid_then_valid '[' 'PRIVATE_VALUE'
printf '%s\n' '--- exact command-substitution behavior ---'
printf '[\n' >"$tmp/patterns"
set +e
local_hits="$(printf '%s\n' 'PRIVATE_VALUE' | grep -nEi -f <(printf '%s\n' "$(cat "$tmp/patterns")") || true)"
status=$?
set -e
printf 'invalid-only local_hits=%q wrapper_status=%s\n' "$local_hits" "$status"
printf '%s\n' '--- self-name filtering behavior ---'
active="$(grep -vE '^[[:space:]]*(#|$)' "$tmp/patterns" || true)"
active="$(printf '%s\n' "$active" | while IFS= read -r pattern; do
[ -z "$pattern" ] && continue
printf '%s\n' 'starsync' | grep -qEi -e "$pattern" 2>/dev/null || printf '%s\n' "$pattern"
done)"
printf 'retained_active=%q\n' "$active"Repository: NomadicDaddy/starsync
Length of output: 686
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'bash=%s\n' "$BASH_VERSION"
run() {
name="$1"
active="$2"
printf '%s\n' "--- $name ---"
set +e
direct="$(printf '%s\n' 'PRIVATE_VALUE' | grep -nEi -f <(printf '%s\n' "$active") 2>"$TMP/err")"
direct_status=$?
set -e
printf 'command_substitution: status=%s output=%q stderr=%q\n' \
"$direct_status" "$direct" "$(cat "$TMP/err")"
set +e
printf '%s\n' 'PRIVATE_VALUE' | grep -nEi -f <(printf '%s\n' "$active") >"$TMP/out" 2>"$TMP/err"
plain_status=$?
set -e
printf 'plain_pipeline: status=%s output=%q stderr=%q\n' \
"$plain_status" "$(cat "$TMP/out")" "$(cat "$TMP/err")"
}
TMP="$(mktemp -d)"
trap 'rm -rf "$TMP"' EXIT
run 'valid' 'PRIVATE_VALUE'
run 'invalid-only' '['
run 'valid-and-invalid' $'PRIVATE_VALUE\n['
printf '%s\n' '--- repeat valid command substitution ---'
for i in $(seq 1 20); do
set +e
out="$(printf '%s\n' 'PRIVATE_VALUE' | grep -nEi -f <(printf '%s\n' 'PRIVATE_VALUE') 2>"$TMP/err")"
status=$?
set -e
printf '%02d status=%s output=%q stderr=%q\n' "$i" "$status" "$out" "$(cat "$TMP/err")"
doneRepository: NomadicDaddy/starsync
Length of output: 893
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
printf 'bash=%s\n' "$BASH_VERSION"
printf '%s\n' 'PRIVATE_VALUE' >"$tmp/input"
run_case() {
name="$1"
shift
set +e
output="$("$@" 2>"$tmp/err")"
status=$?
set -e
printf '%s: status=%s output=%q stderr=%q\n' \
"$name" "$status" "$output" "$(cat "$tmp/err")"
}
run_case 'cat process substitution' bash -c 'cat <(printf "%s\n" "PRIVATE_VALUE")'
run_case 'grep file process substitution, redirect input' bash -c \
'grep -nEi -f <(printf "%s\n" "PRIVATE_VALUE") < "$1"' bash "$tmp/input"
run_case 'grep file process substitution, pipe input' bash -c \
'cat "$1" | grep -nEi -f <(printf "%s\n" "PRIVATE_VALUE")' bash "$tmp/input"
run_case 'grep file process substitution, here-string' bash -c \
'grep -nEi -f <(printf "%s\n" "PRIVATE_VALUE") <<< "PRIVATE_VALUE"'
run_case 'process substitution path opened by shell cat' bash -c \
'pattern=<(printf "%s\n" "PRIVATE_VALUE"); cat "$pattern"'Repository: NomadicDaddy/starsync
Length of output: 770
Fail closed when a Tier 2 pattern is invalid.
If active contains an invalid ERE, grep -f returns status 2. The || true masks this status and can leave local_hits empty. The guard can then allow staged literals.
Treat status 2 as a guard failure. Keep status 1 as the no-match case. Add a regression test with an invalid pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/leak-guard.sh at line 90, Update the leak-guard logic around
local_hits so grep status 2 from an invalid Tier 2 ERE causes the guard to fail,
while status 1 remains the valid no-match case; do not mask grep errors with
unconditional || true. Add a regression test covering an invalid active pattern
and confirming staged literals are rejected.
Comment on lines
+62
to
+68
| if [ ! -f "$CONTRACT_FILE" ]; then | ||
| if [ -d "$ROOT_DIR" ]; then | ||
| note "tag $version: $ROOT_DIR/ exists but $CONTRACT_FILE does not, so nothing is enforced" | ||
| note " add $CONTRACT_FILE if this repository's releases must carry a capture" | ||
| else | ||
| note "tag $version: no $CONTRACT_FILE in this repository, nothing to check" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require .screenshot-capture to be tracked.
[ -f "$CONTRACT_FILE" ] accepts an untracked local file. The pre-push result can then differ between worktrees for the same commit. This contradicts the tracked declaration contract.
Check Git tracking status before enabling enforcement. If the contract must apply to each release tag, resolve it from that tag's tree instead of the current worktree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/screenshot-guard.sh around lines 62 - 68, Update the CONTRACT_FILE
handling in the screenshot guard to verify that .screenshot-capture is tracked
by Git before enabling enforcement, rather than relying only on [ -f ]. Resolve
the contract from each release tag’s tree when enforcement is tag-specific, and
preserve the existing no-contract note behavior when the tracked declaration is
absent.
Comment on lines
+36
to
+41
| # Shared-core rosters. This repository owns no group and so writes none today, but a roster names | ||
| # every private sibling it syncs to, and this repository is public. Ignoring them here costs three | ||
| # lines and removes the only way one could ever be committed by accident. | ||
| /shared-core-targets.json | ||
| /shared-core-sync-targets.json | ||
| /gate-conventions-targets.json |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The roster ignore list and the manifest roster set disagree. scripts/shared-core-manifest.json declares six roster files, and .gitignore ignores three of them. The comment in .gitignore states that a roster names every private sibling and that this repository is public, so the three unignored rosters can be committed by accident.
.gitignore#L36-L41: addportable-gates-aidd-targets.json,portable-gates-targets.json, andlicense-core-adapter-targets.json, and confirm the leading/anchor matches the directoryresolveTargetsreads rosters from.scripts/shared-core-manifest.json#L63-L142: keep the roster filenames in step with the ignore list whenever a group is added, or move rosters into one ignored directory so a single pattern covers every group.
📍 Affects 2 files
.gitignore#L36-L41(this comment)scripts/shared-core-manifest.json#L63-L142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 36 - 41, Add the three missing root-anchored roster
patterns—portable-gates-aidd-targets.json, portable-gates-targets.json, and
license-core-adapter-targets.json—to .gitignore, matching the directory used by
resolveTargets. In scripts/shared-core-manifest.json, keep the manifest roster
filenames synchronized with the ignore list when groups are added; no direct
manifest change is required for the currently declared rosters.
Comment on lines
+55
to
+75
| const installed = await collectInstalledPackages(root, []); | ||
| if (installed.length === 0) failures.push({ expected: true, license: '<installed tree>' }); | ||
|
|
||
| const internal = await workspaceNames(root, []); | ||
| const rootFields: RootDependencyField[] = ['dependencies', 'devDependencies']; | ||
| const locked = await collectLockfileClosure(root, { | ||
| internal, | ||
| rootFields, | ||
| workspaces: [''], | ||
| }); | ||
| if (locked.packages.length === 0) failures.push({ expected: true, license: '<lockfile roots>' }); | ||
| const internal = await workspaceNames(root, []); | ||
| const rootFields: RootDependencyField[] = ['dependencies', 'devDependencies']; | ||
| const locked = await collectLockfileClosure(root, { | ||
| internal, | ||
| rootFields, | ||
| workspaces: [''], | ||
| }); | ||
| if (locked.packages.length === 0) | ||
| failures.push({ expected: true, license: '<lockfile roots>' }); | ||
|
|
||
| const sample = installed[0]; | ||
| if (sample !== undefined) { | ||
| const resolved = await resolveInstalledPackage(root, [], sample.name, sample.version); | ||
| if (resolved === null) failures.push({ expected: true, license: '<installed resolution>' }); | ||
| else { | ||
| await readLicenseText(resolved); | ||
| await readNoticeText(resolved); | ||
| const sample = installed[0]; | ||
| if (sample !== undefined) { | ||
| const resolved = await resolveInstalledPackage(root, [], sample.name, sample.version); | ||
| if (resolved === null) failures.push({ expected: true, license: '<installed resolution>' }); | ||
| else { | ||
| await readLicenseText(resolved); | ||
| await readNoticeText(resolved); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Enforce every closure result before returning success.
Line 65 only checks that locked.packages is non-empty. A required unresolved dependency in locked.unresolved does not fail this command.
Lines 68-75 inspect only one installed package. The code discards the nullable results from readLicenseText and readNoticeText. It does not review the remaining installed packages.
A repository can therefore return 0 while a required dependency is unresolved or a package has no license or notice. Validate the full resolved closure, fail on required unresolved entries, and check the returned license and notice values. If this command is only a smoke-test fixture, remove or narrow the enforcement claim in lines 4-6.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-license-core.ts` around lines 55 - 75, Update the closure
validation around collectLockfileClosure and the installed-package inspection so
every required locked.unresolved entry causes a failure, all installed packages
are resolved and checked rather than only installed[0], and the returned values
from readLicenseText and readNoticeText are validated for missing license or
notice data. Preserve the existing failure reporting conventions, or narrow the
enforcement claim if this command is intended only as a smoke test.
Comment on lines
+126
to
+132
| const remotes = Bun.spawnSync(['git', '-C', repoPath, 'remote', '-v'], { | ||
| stderr: 'pipe', | ||
| stdout: 'pipe', | ||
| windowsHide: true, | ||
| }); | ||
| const pushable = remotes.success && remotes.stdout.toString().includes('(push)'); | ||
| if (pushable) return null; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A failed git remote -v probe grants the opt-out.
Line 131 sets pushable to false whenever remotes.success is false. Line 132 then honors the decline. A spawn failure, a missing git binary, or a directory that is not a repository is not evidence that the repository has no push remote. It is no evidence at all, and this code reads it as the permissive answer.
The doc at lines 110-116 states that the declaration is honored only while the repository has no push remote, because an unconditional opt-out rebuilds the hazard this subsystem exists to remove. Treat an unreadable remote list as pushable so the decline is refused.
🛡️ Proposed fix
- const pushable = remotes.success && remotes.stdout.toString().includes('(push)');
+ // A probe that did not run is not evidence of "no remote"; refuse the decline.
+ const pushable = !remotes.success || remotes.stdout.toString().includes('(push)');
if (pushable) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const remotes = Bun.spawnSync(['git', '-C', repoPath, 'remote', '-v'], { | |
| stderr: 'pipe', | |
| stdout: 'pipe', | |
| windowsHide: true, | |
| }); | |
| const pushable = remotes.success && remotes.stdout.toString().includes('(push)'); | |
| if (pushable) return null; | |
| const remotes = Bun.spawnSync(['git', '-C', repoPath, 'remote', '-v'], { | |
| stderr: 'pipe', | |
| stdout: 'pipe', | |
| windowsHide: true, | |
| }); | |
| // A probe that did not run is not evidence of "no remote"; refuse the decline. | |
| const pushable = !remotes.success || remotes.stdout.toString().includes('(push)'); | |
| if (pushable) return null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/shared-core/targets.ts` around lines 126 - 132, Update the
pushability check around the remotes probe so any failed git remote -v execution
is treated as pushable rather than as evidence of no push remote. Only allow the
opt-out when remotes.success is true and stdout contains “(push)”; otherwise
keep the existing return behavior that refuses the decline.
Comment on lines
+177
to
+188
| export function resolveTargets( | ||
| group: SharedCoreGroup, | ||
| fleetRoot: string, | ||
| ownerRoot: string, | ||
| only?: Set<string>, | ||
| ): Target[] { | ||
| const targets = | ||
| group.targets.model === 'roster' | ||
| ? rosterTargets(group, fleetRoot, ownerRoot) | ||
| : discoveredTargets(group, fleetRoot, group.owner); | ||
| return only === undefined ? targets : targets.filter((t) => only.has(t.directory)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how the sync CLI builds and validates the `only` set.
set -euo pipefail
fd -t f 'sync-shared-core.ts' --exec rg -n -C6 'only|--only|resolveTargets' {}Repository: NomadicDaddy/starsync
Length of output: 6973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
targets_file=$(fd -t f 'targets.ts' | head -n1)
vacuity_file=$(fd -t f 'vacuity.ts' | head -n1)
cli_file=$(fd -t f 'sync-shared-core.ts' | head -n1)
printf '%s\n' "== $targets_file =="
sed -n '150,205p' "$targets_file"
printf '%s\n' "== $vacuity_file =="
sed -n '1,220p' "$vacuity_file"
printf '%s\n' "== $cli_file call flow =="
sed -n '120,310p' "$cli_file"
printf '%s\n' "== references to reportClean and resolveTargets =="
rg -n -C4 'reportClean|resolveTargets|only\.has|options\.only' --glob '*.ts' .Repository: NomadicDaddy/starsync
Length of output: 17065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== shared-core tests and documentation mentioning --only =='
rg -n -C5 --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \
-- '--only|reportClean|parseSharedCoreArgs|runSharedCoreSync' .
printf '%s\n' '== checkGroup report shape =='
check_file=$(fd -t f 'check.ts' | head -n1)
sed -n '70,135p' "$check_file"
printf '%s\n' '== write-path handling of empty findings =='
write_file=$(fd -t f 'write.ts' | head -n1)
sed -n '1,220p' "$write_file"Repository: NomadicDaddy/starsync
Length of output: 18684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def report_clean(reports, only=None):
targets = sum(report["targets"] for report in reports)
files = sum(report["matched"] for report in reports)
if targets == 0 and only is not None:
return 1, f"[FAIL] --only {','.join(only)} matched no target in any group"
return 0, f"[OK] Shared core: no drift ({files} file(s) across {targets} target(s) examined)."
def report_write(reports):
wrote = 0
return 0, f"[OK] Wrote {wrote} file(s)."
cases = [
("--check, unknown only", [{"targets": 0, "matched": 0}], {"typo"}),
("--check, valid plus unknown only", [{"targets": 1, "matched": 1}], {"valid", "typo"}),
("--write, unknown only", [{"targets": 0, "matched": 0}], {"typo"}),
]
for name, reports, only in cases:
result = report_clean(reports, only) if name.startswith("--check") else report_write(reports)
print(f"{name}: exit={result[0]} output={result[1]}")
PYRepository: NomadicDaddy/starsync
Length of output: 421
Reject unknown --only directories in every mode
--check --only typo fails only when no requested name matches any target. A mixed --only valid,typo run returns [OK], and --write --only typo returns [OK] Wrote 0 file(s). Validate every requested directory and report unknown names before filtering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/shared-core/targets.ts` around lines 177 - 188, Update
resolveTargets to validate every entry in only against the resolved targets
before filtering, and report or throw for any unknown directory even when other
requested names are valid. Preserve the existing filtering behavior for valid
names and the unrestricted behavior when only is undefined.
Comment on lines
+191
to
+202
| export function readScripts(repoPath: string): null | Record<string, string> { | ||
| const path = join(repoPath, 'package.json'); | ||
| if (!existsSync(path)) return null; | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(path, 'utf8')) as { | ||
| scripts?: Record<string, string>; | ||
| }; | ||
| return parsed.scripts ?? {}; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
readScripts reports an unparsable package.json as an absent one.
Line 200 returns null from the catch block, and line 193 returns the same value when the file does not exist. Two consumers act on that value and both act wrongly for a corrupt file:
skipReasoninscripts/lib/shared-core/eligibility.tsline 38 reports the reasonno package.json, which names a file that is present.resolveSourceinscripts/lib/shared-core/check.tstreatsnullas an unmet script contract and selectsfallbackSource. With--write, the target receives the lesser hook variant while itspackage.jsonmay satisfy the contract.
Throw instead, so the run names the file it cannot read.
🐛 Proposed fix
if (!existsSync(path)) return null;
- try {
- const parsed = JSON.parse(readFileSync(path, 'utf8')) as {
- scripts?: Record<string, string>;
- };
- return parsed.scripts ?? {};
- } catch {
- return null;
- }
+ try {
+ const parsed = JSON.parse(readFileSync(path, 'utf8')) as {
+ scripts?: Record<string, string>;
+ };
+ return parsed.scripts ?? {};
+ } catch (error) {
+ throw new Error(`${path}: package.json is present but unreadable: ${(error as Error).message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function readScripts(repoPath: string): null | Record<string, string> { | |
| const path = join(repoPath, 'package.json'); | |
| if (!existsSync(path)) return null; | |
| try { | |
| const parsed = JSON.parse(readFileSync(path, 'utf8')) as { | |
| scripts?: Record<string, string>; | |
| }; | |
| return parsed.scripts ?? {}; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| export function readScripts(repoPath: string): null | Record<string, string> { | |
| const path = join(repoPath, 'package.json'); | |
| if (!existsSync(path)) return null; | |
| try { | |
| const parsed = JSON.parse(readFileSync(path, 'utf8')) as { | |
| scripts?: Record<string, string>; | |
| }; | |
| return parsed.scripts ?? {}; | |
| } catch (error) { | |
| throw new Error(`${path}: package.json is present but unreadable: ${(error as Error).message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/shared-core/targets.ts` around lines 191 - 202, Update
readScripts to propagate the JSON parse/read failure from its catch block
instead of returning null. Preserve the null result only for a missing
package.json, while allowing skipReason and resolveSource to distinguish an
unreadable file from an absent one.
Comment on lines
+21
to
+40
| export function reportClean(reports: GroupReport[], only?: Set<string>): number { | ||
| const targets = reports.reduce((total, report) => total + report.targets, 0); | ||
| const files = reports.reduce((total, report) => total + report.matched, 0); | ||
|
|
||
| if (targets === 0 && only !== undefined) { | ||
| console.error( | ||
| `[FAIL] --only ${[...only].join(',')} matched no target in any group, so nothing was ` + | ||
| 'compared. --only narrows the directories a group already resolved; it takes ' + | ||
| 'repository directory names, not group names (use --group for those).', | ||
| ); | ||
| return 1; | ||
| } | ||
|
|
||
| if (targets === 0) { | ||
| console.log( | ||
| '[SKIP] Shared core: no peer repository is checked out beside this one, so there is ' + | ||
| 'nothing to compare against.', | ||
| ); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--only reports a typo when the real cause is an absent owner checkout.
scripts/sync-shared-core.ts lines 269-274 push a group to unverifiable and skip it when its owner is not checked out, so that group never reaches reports. If every group is unverifiable and the caller passed --only, targets is 0 and Line 26 prints "matched no target in any group" and returns 1.
The cause is an absent owner, not a bad --only value, and the run should be [SKIP]. Distinguish the two by whether any group was compared at all.
🐛 Proposed fix
- if (targets === 0 && only !== undefined) {
+ // An empty `reports` means no group was comparable at all, which --only cannot cause.
+ if (targets === 0 && only !== undefined && reports.length > 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function reportClean(reports: GroupReport[], only?: Set<string>): number { | |
| const targets = reports.reduce((total, report) => total + report.targets, 0); | |
| const files = reports.reduce((total, report) => total + report.matched, 0); | |
| if (targets === 0 && only !== undefined) { | |
| console.error( | |
| `[FAIL] --only ${[...only].join(',')} matched no target in any group, so nothing was ` + | |
| 'compared. --only narrows the directories a group already resolved; it takes ' + | |
| 'repository directory names, not group names (use --group for those).', | |
| ); | |
| return 1; | |
| } | |
| if (targets === 0) { | |
| console.log( | |
| '[SKIP] Shared core: no peer repository is checked out beside this one, so there is ' + | |
| 'nothing to compare against.', | |
| ); | |
| return 0; | |
| } | |
| export function reportClean(reports: GroupReport[], only?: Set<string>): number { | |
| const targets = reports.reduce((total, report) => total + report.targets, 0); | |
| const files = reports.reduce((total, report) => total + report.matched, 0); | |
| // An empty `reports` means no group was comparable at all, which --only cannot cause. | |
| if (targets === 0 && only !== undefined && reports.length > 0) { | |
| console.error( | |
| `[FAIL] --only ${[...only].join(',')} matched no target in any group, so nothing was ` + | |
| 'compared. --only narrows the directories a group already resolved; it takes ' + | |
| 'repository directory names, not group names (use --group for those).', | |
| ); | |
| return 1; | |
| } | |
| if (targets === 0) { | |
| console.log( | |
| '[SKIP] Shared core: no peer repository is checked out beside this one, so there is ' + | |
| 'nothing to compare against.', | |
| ); | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/shared-core/vacuity.ts` around lines 21 - 40, Update reportClean
to distinguish no targets because no groups were compared from no targets
because --only matched nothing. Use the reports input to detect whether any
group comparison occurred, and return the existing [SKIP] result when all groups
were omitted as unverifiable; retain the --only error only when groups were
compared but matched no targets.
Comment on lines
+263
to
+276
| for (const group of groups) { | ||
| const ownerRoot = join(fleetRoot, group.owner); | ||
|
|
||
| // An owner that is not checked out cannot be a baseline, and comparing against nothing | ||
| // would report every target as drifted. Warn and skip, the same as an absent sibling: CI | ||
| // checks out one repository, and this gate has to stay green there. | ||
| if (!existsSync(ownerRoot)) { | ||
| unverifiable.push( | ||
| `${group.name} (owner ${group.owner} not checked out at ${ownerRoot})`, | ||
| ); | ||
| continue; | ||
| } | ||
| reports.push(checkGroup(group, fleetRoot, ownerRoot, options.only)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every place the owner root is derived, and any name validation on it.
rg -n -C3 'ownerRoot|group\.owner|repoIdentity' scriptsRepository: NomadicDaddy/starsync
Length of output: 13974
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repoIdentity and ownershipRefusal ---'
sed -n '1,90p' scripts/lib/shared-core/write.ts
printf '%s\n' '--- sync entrypoint and reportCheck ---'
sed -n '240,330p' scripts/sync-shared-core.ts
rg -n -C4 'reportCheck|unverifiable|process\.exit|parseSharedCoreArgs' scripts/sync-shared-core.ts
printf '%s\n' '--- group owner declaration and manifest loading ---'
rg -n -C4 'owner:' scripts scripts .github package.json 2>/dev/null | head -200
rg -n -C4 'SharedCoreGroup|groups|owner' scripts/lib/shared-core scripts/sync-shared-core.ts | head -250
printf '%s\n' '--- package names and sibling directories ---'
python3 - <<'PY'
import json
from pathlib import Path
for p in Path('.').glob('*/package.json'):
try:
data = json.loads(p.read_text())
print(f'{p.parent}: package name={data.get("name")!r}')
except Exception:
pass
PYRepository: NomadicDaddy/starsync
Length of output: 35997
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- shared-core files ---'
git ls-files 'scripts/*' 'scripts/**/*' | grep -E 'shared-core|sync-shared-core|gatesync|package\.json' | head -200
printf '%s\n' '--- manifest implementation and data ---'
sed -n '1,240p' scripts/lib/shared-core/manifest.ts
rg -n -C8 'loadManifest|owner[[:space:]]*[:=]|aidd|spernakit|fleet-root|fleetRoot' scripts README.md docs .github 2>/dev/null | head -400
printf '%s\n' '--- repository package names ---'
for p in package.json */package.json */*/package.json; do
if [ -f "$p" ]; then
printf '%s: ' "$p"
sed -n '/"name"[[:space:]]*:/p' "$p" | head -1
fi
done
printf '%s\n' '--- fleet layout documentation ---'
rg -n -C6 'sibling|checkout|checked out|fleet|owner.*directory|directory.*owner|package name|package.json' . \
-g '*.md' -g '*.ts' -g '*.yml' -g '*.yaml' 2>/dev/null | head -500Repository: NomadicDaddy/starsync
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- reportCheck return path ---'
sed -n '152,188p' scripts/sync-shared-core.ts
sed -n '1,120p' scripts/lib/shared-core/vacuity.ts
printf '%s\n' '--- manifest owners and loader owner set ---'
python3 - <<'PY'
import json
from pathlib import Path
manifest = json.loads(Path('scripts/shared-core-manifest.json').read_text())
owners = sorted({group['owner'] for group in manifest['groups']})
print('manifest owners:', owners)
print('group count:', len(manifest['groups']))
PY
sed -n '90,115p' scripts/lib/shared-core/manifest.ts
printf '%s\n' '--- package-name validation call sites ---'
rg -n -C5 'repoIdentity|ownershipRefusal|package\.json' scripts/lib/shared-core scripts/sync-shared-core.tsRepository: NomadicDaddy/starsync
Length of output: 17360
Validate the owner checkout identity before using it as a baseline.
existsSync(ownerRoot) checks only the path. A checkout renamed from aidd to aidd-copy is skipped as unverifiable, and reportCheck still returns 0. Validate ownerRoot/package.json and require its name to equal group.owner, or fail the check instead of treating the group as unverified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync-shared-core.ts` around lines 263 - 276, The owner baseline
validation in the groups loop must verify checkout identity, not only directory
existence. Before calling checkGroup, read and parse ownerRoot/package.json and
require its name to equal group.owner; if the directory is missing or the
package identity is invalid or mismatched, fail the check rather than adding the
group as unverifiable and allowing reportCheck to return success.
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.
Release StarSync 2.4.0 using the notes in CHANGELOG.md.
Summary by CodeRabbit
New Features
Documentation
npx starsyncorbunx starsynccommands.Chores