Conversation
Replaces '\[yyyy\]' with '[[]yyyy[]]' to ensure compatibility with BSD/macOS sed. Adds regression test.
Refactor scan_dependencies to use find/xargs/jq in a batched manner, significantly reducing process spawning. Corrected subshell variable propagation issue to ensure violations are accurately reported.
Includes environment override in install.sh to support testing without git checkout
…n, portable regex)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds lib/utils.sh with get_source_files(), adopts it across libs; changes deps scanning to traverse full node_modules; enhances license detection/regeneration and init hook backup/idempotency; introduces SOURCE_ROOT in install.sh and copies install.sh into Docker image; many new tests and FIXES.md; bumps VERSION to 0.1.1. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing pedantic scrutiny:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_helper.bash (1)
25-36: Document the newno_git_addflag and nested deps helper behavior.Functionally this is fine, but the contract is now non‑obvious:
create_source_filesilently gained a third argument that changes whether files are staged.create_nested_npm_depsencodes “every 3rd dep is GPL‑3.0” as test data, which is relied on by deps tests.Add brief comments describing both behaviors so future tests don’t have to reverse‑engineer them, e.g.:
-# Create a sample source file -create_source_file() { +# Create a sample source file. +# Usage: create_source_file <path> [content] [no_git_add=true|false] +create_source_file() { @@ - local no_git_add="${3:-false}" # New optional argument + local no_git_add="${3:-false}" # If "true", do not git add the fileand a one‑liner above
create_nested_npm_depsnoting the “every 3rd package is GPL‑3.0” convention.Also applies to: 78-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
Dockerfile(1 hunks)FIXES.md(1 hunks)docs/SPEC.md(1 hunks)install.sh(4 hunks)lib/check.sh(2 hunks)lib/deps.sh(1 hunks)lib/fix.sh(3 hunks)lib/init.sh(3 hunks)lib/utils.sh(1 hunks)test/deps.bats(1 hunks)test/fix.bats(1 hunks)test/init.bats(1 hunks)test/init_safety.bats(1 hunks)test/install.bats(1 hunks)test/test_helper.bash(3 hunks)test/utils.bats(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
test/utils.bats (2)
test/test_helper.bash (4)
setup_test_repo(9-23)create_source_file(26-36)assert_output_contains(54-61)assert_output_not_contains(64-71)lib/utils.sh (1)
get_source_files(10-37)
test/deps.bats (1)
test/test_helper.bash (4)
create_config(39-51)assert_output_contains(54-61)create_nested_npm_deps(79-108)assert_output_not_contains(64-71)
test/init_safety.bats (1)
test/test_helper.bash (1)
setup_test_repo(9-23)
test/install.bats (1)
test/test_helper.bash (1)
assert_output_contains(54-61)
lib/fix.sh (1)
lib/utils.sh (1)
get_source_files(10-37)
lib/check.sh (1)
lib/utils.sh (1)
get_source_files(10-37)
lib/deps.sh (1)
lib/config.sh (1)
is_license_allowed(97-116)
test/fix.bats (1)
test/test_helper.bash (2)
create_config(39-51)run_tlb(74-76)
🪛 LanguageTool
docs/SPEC.md
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...missing, create an empty boilerplate. - If README.md is missing the license sectio...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Shellcheck (0.11.0)
lib/utils.sh
[warning] 32-32: This flag is used as a command name. Bad line break or missing [ .. ]?
(SC2215)
lib/fix.sh
[info] 11-11: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
lib/check.sh
[info] 10-10: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (8)
install.sh (1)
10-10: LGTM: Clean abstraction for source path flexibility.The introduction of
SOURCE_ROOTwith override capability is well-executed. The variable provides a clean mechanism for alternate source locations while maintaining backward compatibility through the default fallback toSCRIPT_DIR.Also applies to: 29-29, 40-40
lib/init.sh (1)
32-32: LGTM: Proper backslash handling in user input.The addition of
-rflags to allreadcommands correctly prevents backslash interpretation, eliminating potential issues with user input containing escape sequences or path separators.Also applies to: 35-35, 38-38, 41-41, 52-52
Dockerfile (1)
29-29: LGTM: Logical addition for containerized workflows.Shipping
install.shin the image enables installation testing and workflow validation within the container environment.docs/SPEC.md (1)
56-56: LGTM: Documentation reflects implementation improvements.The additions accurately document the deep scanning behavior and portable regex handling implemented in this PR.
Also applies to: 64-64
lib/fix.sh (2)
11-11: LGTM but blocked by critical bug in utils.sh.The sourcing of
utils.shand delegation toget_source_files()is clean refactoring. However,get_source_files()currently has a critical operator precedence bug (see my comment onlib/utils.shlines 24-33) that will cause incorrect file discovery in the fallbackfindpath.This file's changes are correct, but the system won't work properly until that bug is fixed.
Also applies to: 213-213
103-104: LGTM: Portable literal bracket matching.The use of character classes
[[]and[]]to match literal brackets is correct and portable across both GNU and BSDsed. This properly targets the placeholder patterns[yyyy]and[name of copyright owner]in license templates.test/fix.bats (1)
186-207: LGTM: Comprehensive placeholder replacement validation.The test correctly validates both the negative case (placeholders removed) and positive case (actual values present). The use of
grepexit codes to assert absence (status -eq 1) is appropriate.lib/check.sh (1)
8-11: Centralizing source file discovery onget_source_filesis the right move.Sourcing
utils.shand delegating header checks toget_source_filesde‑duplicates the previous ad‑hoc discovery logic and correctly extends coverage to non‑git repos via the find fallback. Thewhile IFS= read -r file; … done <<< "${files}"pattern is also safe for paths with spaces.Just make sure any entry point that sources
lib/check.shalso setsLIB_DIRsoutils.shis always found at runtime (same requirement you already had fordeps.sh).Also applies to: 81-87
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Ross <james@flyingrobots.dev>
Replaced batched xargs approach with a while loop using process substitution and single-shot jq per file. Eliminates large variable buffering and extra jq round-trips while maintaining subshell safety.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
test/install.bats (2)
51-58: PATH assertions are way too brittle; assert behavior, not one exact export line.Both PATH tests require the literal string
export PATH="…:\$PATH"to appear in the rc files. Any harmless formatting change ininstall.sh(different quoting, usingPATH=…:$PATH; export PATH, reordering PATH elements, extra whitespace, etc.) will break these tests while behavior remains identical.The real contract is “the install dir is on PATH for bash/zsh”. Check for the directory, not the exact export syntax, e.g. by grepping for
${MOCK_HOME}/.totally-legal-broand asserting the grep succeeds.@test "install.sh: adds to PATH in .bashrc" { /app/install.sh - # Assert PATH is added to .bashrc - # Check if the expected line exists in the file - run cat "${HOME}/.bashrc" - assert_output_contains "export PATH=\"${MOCK_HOME}/.totally-legal-bro:\$PATH\"" + # Assert PATH is added to .bashrc (format-agnostic) + run grep -F "${MOCK_HOME}/.totally-legal-bro" "${HOME}/.bashrc" + [ "$status" -eq 0 ] } @test "install.sh: adds to PATH in .zshrc" { /app/install.sh - # Assert PATH is added to .zshrc - run cat "${HOME}/.zshrc" - assert_output_contains "export PATH=\"${MOCK_HOME}/.totally-legal-bro:\$PATH\"" + # Assert PATH is added to .zshrc (format-agnostic) + run grep -F "${MOCK_HOME}/.totally-legal-bro" "${HOME}/.zshrc" + [ "$status" -eq 0 ] }Also applies to: 60-66
68-79: Idempotence test ignores exit status and reuses the same brittle PATH literal.Two problems here:
You pipe
'y'intoinstall.shviarunbut never assert$status. The test will still pass if the second install fails, as long as.bashrchappens to keep a single PATH line. That undermines the “happy path re‑install” guarantee.As with the earlier PATH tests, you hardcode the full
export PATH="…:\$PATH"line, so benign formatting tweaks ininstall.shwill break the test.At minimum, assert that the second install succeeds before grepping
.bashrc. Ideally, also relax the PATH check in the same way as above, and consider mirroring this idempotence check for.zshrcif bash/zsh share the same PATH logic.@test "install.sh: does not duplicate PATH entries" { @@ # Run again, it should detect and not add again # We must pipe 'y' because the directory exists run bash -c "echo 'y' | /app/install.sh" - + [ "$status" -eq 0 ] # Count occurrences of the export line run grep -c "export PATH=\"${MOCK_HOME}/.totally-legal-bro:\$PATH\"" "${HOME}/.bashrc" - [ "$output" = "1" ] + [ "$status" -eq 0 ] + [ "$output" = "1" ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
lib/init.sh(3 hunks)lib/utils.sh(1 hunks)test/init.bats(1 hunks)test/install.bats(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/install.bats (1)
test/test_helper.bash (1)
assert_output_contains(54-61)
🔇 Additional comments (1)
lib/init.sh (1)
111-123: Git hook backup flow now does the right thingThe reordering here (first grep for
totally-legal-bro checkand early‑return, then back up and append only when needed) finally kills the pointless.bakchurn on already‑configured hooks while still protecting existing custom hooks before you modify them. The control flow is clean and matches the new backup/merge test expectations.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
test/deps.bats (1)
89-97: Good: negative assertion now matches the actual violation format.Using:
assert_output_not_contains "pkg-1-level-4@1.0.1 (MIT)"finally lines up with the real output format (
name@version (license)) and will actually fail if an MIT package is incorrectly reported as a violation. This addresses the earlier concern about the too-loose “pkg-1-level-4 (MIT)” check.test/init_safety.bats (1)
16-35: Simulated stdin still doesn’t match the documented prompts and over‑escapes the backslash.Right now:
"Acme\\\ Corp"feeds two literal backslashes before the space (Acme\\ Corp), not the singleAcme\ Corpyou describe in the comments.- The line sequence is: license, owner, empty, empty,
n, empty — which does not match the documented order of “deps empty, hook=n, CI=n”.So this test is not actually validating the precise
read -rbehavior you’re describing.You want:
MITAcme\ Corp""(deps: empty)n(hook)n(CI)and only a single backslash before the space in line 2. Something like this is much closer to the real interaction:
- # 3. Dependency Policy: (empty line to finish) - # 4. Git hook setup: n - # 5. CI workflow setup: n - printf "MIT\nAcme\\\ Corp\n\n\nn\n" | totally-legal-bro init + # 3. Dependency Policy: (empty line to finish) + # 4. Git hook setup: n + # 5. CI workflow setup: n + # MIT, owner, deps (empty), hook=n, CI=n + printf "MIT\nAcme\\ Corp\n\nn\nn\n" | totally-legal-bro initThat feeds exactly one backslash before the space and aligns the line order with the prompts, so the assertion on
ownerNameactually matches the scenario you’re documenting.test/install.bats (3)
6-18: Stop assuming the repo (andinstall.sh) live at/app; derive the path from the test instead.These tests still bake in
/app:
SOURCE_ROOT_OVERRIDE="/app"- direct
/app/install.shinvocations in every testwhich makes the suite unusable outside a very specific Docker layout. You can keep the semantics and make this portable by deriving the repo root from
BATS_TEST_DIRNAMEand centralizing the script path:setup() { - # Create a mock HOME directory for testing install.sh + # Create a mock HOME directory for testing install.sh MOCK_HOME="${BATS_TEST_TMPDIR}/mock_home" @@ - # Set SOURCE_ROOT_OVERRIDE for install.sh to Docker's /app - export SOURCE_ROOT_OVERRIDE="/app" + # Point SOURCE_ROOT_OVERRIDE and INSTALL_SH at the actual repo root + local repo_root="${BATS_TEST_DIRNAME%/*}" + export SOURCE_ROOT_OVERRIDE="${repo_root}" + INSTALL_SH="${repo_root}/install.sh" } @@ - # Run install.sh from within the test repo context - /app/install.sh + # Run install.sh from within the test repo context + "${INSTALL_SH}" @@ - /app/install.sh + "${INSTALL_SH}" @@ - /app/install.sh + "${INSTALL_SH}" @@ - /app/install.sh + "${INSTALL_SH}" @@ - /app/install.sh > /dev/null + "${INSTALL_SH}" > /dev/null @@ - run bash -c "echo 'y' | /app/install.sh" + run bash -c "echo 'y' | \"${INSTALL_SH}\"" @@ - run bash -c "echo 'n' | /app/install.sh" + run bash -c "echo 'n' | \"${INSTALL_SH}\"" @@ - run bash -c "echo 'y' | /app/install.sh" + run bash -c "echo 'y' | \"${INSTALL_SH}\""This makes the tests runnable from a normal checkout while preserving the intended behavior.
Also applies to: 27-42, 51-61, 67-74, 84-103
20-25: Hardenteardown()against partial setup failures and env leakage.
teardown()still assumesMOCK_HOMEis set and only unsetsHOME, so:
- If
setup()dies before assigningMOCK_HOME,rm -rf "${MOCK_HOME}"will complain about an empty path.MOCK_HOMEandSOURCE_ROOT_OVERRIDEremain in the environment and can bleed into later tests.Guard the cleanup and fully reset the env:
teardown() { cd / - [[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" - rm -rf "${MOCK_HOME}" - unset HOME + [[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" + [[ -n "${MOCK_HOME:-}" ]] && rm -rf "${MOCK_HOME}" + unset HOME MOCK_HOME SOURCE_ROOT_OVERRIDE INSTALL_SH }This matches the defensive pattern you’re already using for
REPO_DIR.
84-93: Overwrite-flow tests still don’t assert the CLI exit contract.For both overwrite branches you only look at output strings:
'n'path: asserts"Aborting install."but not that the command failed.'y'path: asserts"Installing totally-legal-bro"/"Done."but not that it actually exited 0.If
install.shprinted those messages but bailed with a non-zero status (or vice versa), these tests would still pass. Make the expected exit codes explicit:@test "install.sh: handles overwrite confirmation" { @@ - run bash -c "echo 'n' | /app/install.sh" + run bash -c "echo 'n' | ${INSTALL_SH:-/app/install.sh}" @@ - assert_output_contains "Aborting install." + assert_output_contains "Aborting install." + [ "$status" -ne 0 ] } @@ @test "install.sh: allows overwriting with 'y'" { @@ - run bash -c "echo 'y' | /app/install.sh" + run bash -c "echo 'y' | ${INSTALL_SH:-/app/install.sh}" @@ - assert_output_contains "Installing totally-legal-bro" - assert_output_contains "Done." + assert_output_contains "Installing totally-legal-bro" + assert_output_contains "Done." + [ "$status" -eq 0 ](Adjust the
${INSTALL_SH:-...}part if you adopt the location-agnostic setup above.)Also applies to: 95-108
test/utils.bats (1)
6-14:/apphardcoding makes these tests brittle; mirror the repo‑root derivation used elsewhere.These tests correctly exercise
get_source_filesin both git and non‑git scenarios, but they assume the library always lives at/app/lib/utils.sh, which is the same fragility noted in the install tests.You can make them portable without changing behavior by resolving the repo root from the test location and sourcing via that:
setup() { - REPO_DIR=$(setup_test_repo) - cd "${REPO_DIR}" + REPO_DIR=$(setup_test_repo) + cd "${REPO_DIR}" + + # Locate the project repo root (parent of test directory) + REPO_ROOT="${BATS_TEST_DIRNAME%/*}" } teardown() { cd / - rm -rf "${REPO_DIR}" + [[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" + unset REPO_ROOT } @@ @test "get_source_files: correctly identifies source files in a git repo" { - source /app/lib/utils.sh # Source from absolute path in Docker container + source "${REPO_ROOT}/lib/utils.sh" @@ @test "get_source_files: falls back to find if not a git repo" { @@ - source /app/lib/utils.sh # Source from absolute path in Docker container + source "${REPO_ROOT}/lib/utils.sh"This keeps the tests’ logic the same but removes the hidden “must be running in a Docker image at /app” requirement.
Also applies to: 16-45, 47-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
lib/deps.sh(1 hunks)test/deps.bats(1 hunks)test/init_safety.bats(1 hunks)test/install.bats(1 hunks)test/test_helper.bash(2 hunks)test/utils.bats(1 hunks)totally-legal-bro(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/install.bats (1)
test/test_helper.bash (1)
assert_output_contains(55-62)
lib/deps.sh (1)
lib/config.sh (1)
is_license_allowed(97-116)
test/utils.bats (2)
test/test_helper.bash (4)
setup_test_repo(9-23)create_source_file(27-37)assert_output_contains(55-62)assert_output_not_contains(65-72)lib/utils.sh (1)
get_source_files(10-35)
test/init_safety.bats (1)
test/test_helper.bash (1)
setup_test_repo(9-23)
test/deps.bats (1)
test/test_helper.bash (4)
create_config(40-52)assert_output_contains(55-62)create_nested_npm_deps(80-109)assert_output_not_contains(65-72)
🔇 Additional comments (1)
totally-legal-bro (1)
11-11: Version bump looks fine; verify single source of truth across scripts/docs.Updating
VERSIONto0.1.1here is straightforward, but make sure any other places that surface or embed the version (e.g.,install.shbanners, docs, Docker labels) are kept in sync so--versionand installation messaging don’t diverge.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
test/install.bats (4)
20-25: Unguardedrm -rfon potentially-empty variable—still not fixed.Line 22 correctly guards
REPO_DIR, but line 23 still runsrm -rf "${MOCK_HOME}"without checking if it's set. Ifsetup()dies before assigningMOCK_HOME, this becomesrm -rf ""which, depending on shell behavior and working directory, could be catastrophic.teardown() { cd / [[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" - rm -rf "${MOCK_HOME}" - unset HOME + [[ -n "${MOCK_HOME:-}" ]] && rm -rf "${MOCK_HOME}" + unset HOME MOCK_HOME SOURCE_ROOT_OVERRIDE }
16-17: Hardcoded/apppath makes tests non-portable.
SOURCE_ROOT_OVERRIDE="/app"assumes the repo is mounted at/app(Docker-specific). Running this test suite from a normal checkout will fail. This was flagged previously.Derive the path dynamically:
- # Set SOURCE_ROOT_OVERRIDE for install.sh to Docker's /app - export SOURCE_ROOT_OVERRIDE="/app" + # Derive repo root from test file location + export SOURCE_ROOT_OVERRIDE="${BATS_TEST_DIRNAME%/*}"
82-91: Abort test doesn't verify exit status—still unaddressed.You assert the message but not whether
install.shactually signals failure. If aborting should be non-zero (sensible), enforce it:run bash -c "echo 'n' | /app/install.sh" # Note: read -p might not print prompt when not TTY, so we check for the abort message assert_output_contains "Aborting install." + [ "$status" -ne 0 ]Define the contract. Test the contract.
93-116: Missing exit status assertion after 'y' confirmation—still unaddressed.You verify the output messages but never confirm
$status -eq 0. The script could print "Done." and then crash, and this test would pass.assert_output_contains "Installing totally-legal-bro" assert_output_contains "Done." + [ "$status" -eq 0 ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
lib/fix.sh(4 hunks)test/fix.bats(1 hunks)test/install.bats(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/fix.bats (1)
test/test_helper.bash (2)
create_config(40-52)run_tlb(75-77)
lib/fix.sh (1)
lib/utils.sh (1)
get_source_files(10-35)
test/install.bats (1)
test/test_helper.bash (1)
assert_output_contains(55-62)
🪛 Shellcheck (0.11.0)
lib/fix.sh
[info] 11-11: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (3)
lib/fix.sh (1)
239-245: Good refactor to centralized file discovery.The switch from inline
git ls-files | greptoget_source_filesimproves maintainability and ensures consistent behavior across modules. The empty-check on line 242-245 handles the no-files case gracefully.test/fix.bats (2)
186-207: Solid test for Apache-2.0 placeholder replacement.Correctly verifies that placeholder tokens are removed and actual values are substituted. The use of grep exit status 1 to confirm absence is appropriate.
209-243: Comprehensive regeneration test.Tests the full lifecycle: existing LICENSE with placeholders → detection → regeneration → verification of actual content. The use of
<<'EOF'(quoted heredoc) ensures the placeholder text is literal. Good coverage.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
lib/fix.sh (3)
11-11: UnguardedLIB_DIRmakes this script brittle and fails with opaque errorsRight now you blindly do:
source "${LIB_DIR}/utils.sh"If
LIB_DIRis unset/empty, this becomessource "/utils.sh"and blows up with a useless “No such file or directory” instead of a clear, actionable failure.Add a hard guard and a file-existence check before sourcing, e.g.:
+: "${LIB_DIR:?LIB_DIR must be set to the directory containing utils.sh}" +if [[ ! -f "${LIB_DIR}/utils.sh" ]]; then + echo "ERROR: ${LIB_DIR}/utils.sh not found" >&2 + exit 1 +fi source "${LIB_DIR}/utils.sh"This keeps failures deterministic and debuggable if someone invokes
fix.shoutside the usual wiring.Please run the existing Bats suite invoking
totally-legal-bro fixdirectly (without the usual wrapper) to confirm you don’t hit a confusingsource: : No such file or directoryifLIB_DIRis accidentally missing from the environment.
83-108: Placeholder detection is still overly broad;TODO/PLACEHOLDERwill cause noisy false positivesThe placeholder scan:
local placeholder_patterns=( '\[Full.*text.*here\]' '\[.*license.*text.*\]' 'TODO' 'PLACEHOLDER' '\[yyyy\]' '\[name of copyright owner\]' '\[fullname\]' ) ... if grep -qiE "${pattern}" LICENSE; then has_placeholder=true break fiwill trigger regeneration on any incidental
TODOorplaceholdermention in a legitimate LICENSE (you’re using-i+ no anchoring). For example, a NOTICE-like paragraph describing “placeholder text” or a comment block embedded in the license file would be enough to nuke an otherwise valid LICENSE.You’ve added tests that explicitly use:
TODO: Add license text herePLACEHOLDER - Replace with full license textSo you can safely tighten the regexes without losing coverage. For example:
- local placeholder_patterns=( - '\[Full.*text.*here\]' - '\[.*license.*text.*\]' - 'TODO' - 'PLACEHOLDER' - '\[yyyy\]' - '\[name of copyright owner\]' - '\[fullname\]' - ) + local placeholder_patterns=( + '^\s*TODO[: ].*license' + '^\s*PLACEHOLDER\b.*license' + '\[Full.*text.*here\]' + '\[.*license.*text.*\]' + '\[yyyy\]' + '\[name of copyright owner\]' + '\[fullname\]' + )Optionally, you can require at least two distinct placeholder hits before regenerating (e.g.,
[yyyy]ANDTODO) to further reduce accidental matches.Please sanity-check this against a few real-world LICENSE files in your repos (Apache-2.0, MIT, BSD-3-Clause) to ensure you don’t accidentally trigger on benign text while still catching your synthetic placeholder examples.
142-143: Inconsistent bracket escaping between grep and sed is a long-term footgunHere you use the character-class trick:
-e "s/[[]yyyy[]]/${escaped_year}/g" \ -e "s/[[]name of copyright owner[]]/${escaped_owner}/g" \while the detection side uses
'\[yyyy\]'/'\[name of copyright owner\]'. Both are technically valid, but having two different escaping conventions for the same tokens is exactly how one side gets updated and the other forgotten.For maintainability, prefer a single style everywhere, e.g. backslash-escaped literals:
- -e "s/[[]yyyy[]]/${escaped_year}/g" \ - -e "s/[[]name of copyright owner[]]/${escaped_owner}/g" \ + -e "s/\[yyyy\]/${escaped_year}/g" \ + -e "s/\[name of copyright owner\]/${escaped_owner}/g" \Then keep the grep patterns aligned with that style.
Double-check all license templates that contain
[yyyy]/[name of copyright owner]to ensure the replacement still fires correctly after unifying the escaping.test/fix.bats (1)
245-266: Still missing an assertion that the configured owner appears in the regenerated LICENSEYou’ve improved coverage around placeholder handling, but this test still only checks:
PLACEHOLDERis gone.- Apache license text (“TERMS AND CONDITIONS”) is present.
It does not assert that the configured owner (“Acme Inc”) actually appears in the regenerated LICENSE. If
create_license_templateregressed and stopped substituting the owner, this test would still pass.Recommend adding an assertion like:
# Should contain real Apache license run grep "TERMS AND CONDITIONS" LICENSE [ "$status" -eq 0 ] + + # Should contain the configured owner + run grep "Acme Inc" LICENSE + [ "$status" -eq 0 ](And, per earlier comments, also assert
run_tlb fixsucceeded.)After adding the owner assertion, temporarily break owner substitution in
create_license_template(e.g., hard‑code a dummy name) to ensure this test correctly fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
lib/fix.sh(4 hunks)test/fix.bats(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/fix.sh (1)
lib/utils.sh (1)
get_source_files(10-35)
test/fix.bats (1)
test/test_helper.bash (2)
create_config(40-52)run_tlb(75-77)
🪛 Shellcheck (0.11.0)
lib/fix.sh
[info] 11-11: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (1)
lib/fix.sh (1)
251-283: The current implementation is correct;get_source_filesoutputs newline-delimited paths, not space-separated
get_source_filesinlib/utils.shusesgit ls-filesandfind, both of which output one file per line (newline-delimited), not space-separated on a single line. Thewhile IFS= read -r file; do ... done <<< "${files}"pattern is the correct and idiomatic way to process this output in bash—the here-string preserves newlines, so each line becomes a separate loop iteration.The test at
test/fix.bats("fix: reports number of fixes made") verifies this behavior: it creates three source files and expects the fix command to process all three alongside the LICENSE, NOTICE, and README files, for a total of 6 fixes. This test would fail if only a single iteration occurred as described in the review.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/init.sh (1)
50-57: Trim dependency license entries before persisting them.You now correctly treat whitespace‑only input as “blank”, but you still store whatever the user typed verbatim:
read -r -p "License (or blank to finish): " license if [[ -z "${license//[[:space:]]/}" ]]; then break fi dep_licenses+=("${license}")If someone types
"MIT "or" Apache-2.0", the JSON will contain"MIT "/" Apache-2.0", andis_license_allowed(which does[[ "${license}" == "${allowed}" ]]) will never match real"MIT"values coming from package.json. That silently turns “allowed” into “denied”.Recommend normalizing before appending, e.g.:
- dep_licenses+=("${license}") + # Normalize whitespace so policy entries match SPDX IDs from manifests + local trimmed="${license#"${license%%[![:space:]]*}"}" + trimmed="${trimmed%"${trimmed##*[![:space:]]}"}" + dep_licenses+=("${trimmed}")(or factor a small
trim()helper), so policies behave predictably.
♻️ Duplicate comments (2)
test/install.bats (1)
118-141: Still not asserting the exit status on the positive overwrite path.This test verifies only that the “Installing totally-legal-bro” and “Done.” strings were printed. If
install.shprints those but exits non‑zero (or bails before copying anything), the test passes.You already enforce a non‑zero status on the
'n'abort path above; mirror that here to lock in the CLI contract for the'y'path:@@ - # Check that it proceeded (installed files) - assert_output_contains "Installing totally-legal-bro" - assert_output_contains "Done." + # Check that it proceeded (installed files) and exited successfully + assert_output_contains "Installing totally-legal-bro" + assert_output_contains "Done." + [ "$status" -eq 0 ]This ensures the test fails if overwrite is “pretend succeeded” via output only.
test/fix.bats (1)
365-405: Still missing an assertion onrun_tlb fixexit status in this testIn
@test "fix: fixes source headers missing copyright symbol"you never assert that:run_tlb fixactually succeeded. If
fixexits non‑zero after partially editing files, this test could still pass as long as the subsequentcheckrun succeeds.Add an explicit status check right after
run_tlb fix:# Fix should detect and fix it run_tlb fix + + [ "$status" -eq 0 ]This makes the test fail fast on a broken
fiximplementation instead of only noticing via downstream effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
lib/check.sh(4 hunks)lib/deps.sh(1 hunks)lib/fix.sh(7 hunks)lib/init.sh(3 hunks)lib/utils.sh(1 hunks)test/check.bats(1 hunks)test/deps.bats(1 hunks)test/fix.bats(1 hunks)test/init.bats(1 hunks)test/install.bats(1 hunks)test/test_helper.bash(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
test/install.bats (1)
test/test_helper.bash (1)
assert_output_contains(55-62)
lib/check.sh (1)
lib/utils.sh (1)
get_source_files(10-34)
test/deps.bats (1)
test/test_helper.bash (4)
create_config(40-52)assert_output_contains(55-62)create_nested_npm_deps(82-111)assert_output_not_contains(65-72)
lib/deps.sh (1)
lib/config.sh (1)
is_license_allowed(97-116)
test/check.bats (1)
test/test_helper.bash (3)
create_config(40-52)run_tlb(75-77)assert_output_contains(55-62)
test/fix.bats (1)
test/test_helper.bash (3)
create_config(40-52)run_tlb(75-77)assert_output_contains(55-62)
lib/fix.sh (1)
lib/utils.sh (1)
get_source_files(10-34)
🪛 Shellcheck (0.11.0)
lib/check.sh
[info] 10-10: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
lib/fix.sh
[info] 18-18: Not following: ./utils.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 82-82: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🔇 Additional comments (10)
test/deps.bats (1)
71-97: Large nested tree coverage looks correct and precise.This test cleanly exercises the
create_nested_npm_depsshape, asserts several distinct GPL-3.0 violations by fullname@version (license), and negatively asserts an allowed MIT chain is not reported. The expectations line up with the helper’si % 3 == 0licensing pattern and withscan_dependencies’ output format.No changes needed here.
lib/init.sh (1)
111-123: Hook backup order now matches the intended idempotent semantics.The new flow—first short‑circuiting when the marker is already present, then backing up only when you’re about to append—is exactly what you want:
- No spurious
.bakfiles when the hook is already configured.- A single backup per original hook when you actually modify it.
This pairs cleanly with the new tests in
test/init.batsand avoids littering.bakfiles on no‑op runs.Looks good.
lib/deps.sh (1)
65-77: Streaming npm scan is correct and much more scalable.The new
find … -print0 | while IFS= read -r -d '' pkgfileloop plus singlejqcall with tab‑separated fields is a big improvement over buffering everything into one variable and re‑parsing:
- Null‑delimited paths are handled safely.
IFS=$'\t' read -r pkg license <<< "${result}"is clear and avoids the opaque parameter‑expansion splitting from the prior revision.- The fallback
printf "unknown\tUNKNOWN"path keepspkg/licensewell‑defined even whenjqchokes.This aligns nicely with the deep tree tests in
test/deps.bats. No changes needed.test/test_helper.bash (2)
17-37: Repo setup andcreate_source_fileflagging are sane and backwards-compatible.
- Forcing
setup_test_repotocdinto the new repo beforegit initis exactly what you want; it hardens every helper against accidental “current directory” surprises.create_source_filekeeps the old default (auto‑staging) while letting callers opt out withno_git_add="true", which matches the usage pattern in the tests.This is a clean, low-friction API evolution for the helpers.
79-111: Nested npm fixture generator matches the deps tests’ expectations.
create_nested_npm_depsbuilds exactly the structure your tests assert against:
pkg-$i-level-$jnames, versions1.0.$i, and consistent license per top-leveli.- Every 3rd top-level chain (
i % 3 == 0) is GPL-3.0, others MIT, aligning with the “every 3rd dep is GPL” checks.- Each level has its own
node_modulesdir, so full-tree traversal behaves like a real npm graph.Nice targeted helper; no changes needed.
lib/check.sh (2)
81-87: Switch toget_source_filesand “no sources == pass” is consistent and sane.Using
get_source_fileshere is a clear win:
- Single point of truth for which extensions count as “source”.
- Works both in and out of git repos (unlike the previous hard-wired
git ls-files).Treating an empty set as:
{ "status": "pass", "detail": "No source files found", "total": 0, "missing": 0, "files": [] }also lines up with the existing “ignores non-source files” test semantics—you’re not forcing people with pure-doc or data repos to fail header checks.
Implementation (newline-separated list +
while IFS= read -r file) is fine; no issues spotted.
177-191: Warning semantics are finally consistent between JSON and human modes.The updated failure counting and rendering logic looks correct:
- In JSON mode you now only bump
CHECK_FAILURESwhen.status == "fail", and explicitly document that warnings are not failures.- In human mode,
render_statusandrender_deps_statuscolour WARN appropriately without incrementingCHECK_FAILURES, while FAIL still does.render_deps_status’secho "${json}" | jq -r '.notes[]? | " • " + .'is a nice, null-safe way to show notes without exploding on empty arrays.This matches the new tests in
test/check.batsand gives a coherent story: warnings inform, failures gate the exit code.Looks good.
Also applies to: 255-272
test/fix.bats (1)
186-315: New LICENSE regeneration tests are well targeted and now cover owner/year correctlyThese tests do a good job of pinning down behavior:
- Assert
fixsucceeds ([ "$status" -eq 0 ]) before inspecting LICENSE.- Verify placeholders (
[yyyy], owner placeholders,TODO,PLACEHOLDER) are gone.- Check that the regenerated LICENSE matches the configured year/owner and expected MIT/Apache phrases.
- For wrong-license-type scenarios, also assert
checkpasses afterfix.This is the right level of specificity; nicely tightens the contract around
fix_license_file.lib/fix.sh (2)
11-18: LIB_DIR guard + utils sourcing is finally robustThis guard:
: "${LIB_DIR:?LIB_DIR must be set to the directory containing utils.sh}" if [[ ! -f "${LIB_DIR}/utils.sh" ]]; then echo "ERROR: ${LIB_DIR}/utils.sh not found" >&2 exit 1 fi source "${LIB_DIR}/utils.sh"is exactly what was missing before—fail-fast with a clear error instead of a cryptic
source: : No such file or directory. No further nits here.
79-90: The current regex pattern is correct and works as intended; the proposed changes are invalidYour analysis of the
\bpattern is incorrect. Testing confirms:
\bworks correctly in this environment: GNU grep 3.8 supports\bas a word boundary in ERE mode. While not strictly POSIX, it's a widely-supported GNU extension that functions properly.Current code correctly identifies licenses:
grep -qiE "\bMIT\b"correctly matches MIT in MIT.txtgrep -qiE "\bApache\b"correctly matches Apache in Apache-2.0.txt- Cross-checks correctly return no match (MIT not in Apache file, Apache not in MIT file)
- The repo's actual Apache-2.0 LICENSE is correctly identified
The proposed fix is invalid:
[[:<:]]and[[:>:]]are not valid in POSIX ERE or GNU grep. These produce "Invalid character class name" errors in grep -E. They're BSD/gawk-specific extensions that don't apply here.The code requires no changes. The license-type detection is working correctly without false negatives or false positives.
Likely an incorrect or invalid review comment.
| # Create a cleaned version of the file without existing SPDX/Copyright header lines | ||
| local cleaned_file | ||
| cleaned_file=$(mktemp) | ||
|
|
||
| if [[ "${has_shebang}" == true ]]; then | ||
| # Keep shebang, skip old header lines, keep rest | ||
| head -n 1 "${file}" > "${cleaned_file}" | ||
| tail -n +2 "${file}" | sed -E '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; /^[[:space:]]*\*\/[[:space:]]*$/d; /^[[:space:]]*\/\*[[:space:]]*$/d' >> "${cleaned_file}" | ||
| else | ||
| # No shebang, just remove header lines | ||
| sed -E '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; /^[[:space:]]*\*\/[[:space:]]*$/d; /^[[:space:]]*\/\*[[:space:]]*$/d' "${file}" > "${cleaned_file}" | ||
| fi |
There was a problem hiding this comment.
Header “cleanup” is deleting all /* / */ lines in the file, corrupting non-header block comments
This section is dangerous:
# Create a cleaned version of the file without existing SPDX/Copyright header lines
local cleaned_file
cleaned_file=$(mktemp)
if [[ "${has_shebang}" == true ]]; then
head -n 1 "${file}" > "${cleaned_file}"
tail -n +2 "${file}" | sed -E \
'/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; \
/^[[:space:]]*\*\/[[:space:]]*$/d; \
/^[[:space:]]*\/\*[[:space:]]*$/d' >> "${cleaned_file}"
else
sed -E \
'/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; \
/^[[:space:]]*\*\/[[:space:]]*$/d; \
/^[[:space:]]*\/\*[[:space:]]*$/d' "${file}" > "${cleaned_file}"
fi
...
rm -f "${cleaned_file}"
mv "${tmp_file}" "${file}"Those second and third sed patterns (/^\s*\*\/\s*$/ and /^\s*\/\*\s*$/) match every standalone /* or */ line anywhere in the file, not just the top-of-file license header. For C/JS/TS/etc. this means:
- Any normal block comment later in the file loses its delimiters.
- Remaining
* commentlines become invalid code in C-family languages (very easy to break builds). - You’re silently corrupting source files just by running
fix.
You only need to strip the actual SPDX/Copyright lines before reinjecting the header. Drop the blanket /* / */ deletions:
- if [[ "${has_shebang}" == true ]]; then
- # Keep shebang, skip old header lines, keep rest
- head -n 1 "${file}" > "${cleaned_file}"
- tail -n +2 "${file}" | sed -E '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; /^[[:space:]]*\*\/[[:space:]]*$/d; /^[[:space:]]*\/\*[[:space:]]*$/d' >> "${cleaned_file}"
- else
- # No shebang, just remove header lines
- sed -E '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d; /^[[:space:]]*\*\/[[:space:]]*$/d; /^[[:space:]]*\/\*[[:space:]]*$/d' "${file}" > "${cleaned_file}"
- fi
+ if [[ "${has_shebang}" == true ]]; then
+ # Keep shebang, skip old SPDX/Copyright lines, keep rest
+ head -n 1 "${file}" > "${cleaned_file}"
+ tail -n +2 "${file}" | sed -E \
+ '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d' \
+ >> "${cleaned_file}"
+ else
+ # No shebang, just remove SPDX/Copyright lines
+ sed -E \
+ '/^[#%\/\*[:space:]]*(SPDX-License-Identifier|Copyright)/d' \
+ "${file}" > "${cleaned_file}"
+ fi
@@
- rm -f "${cleaned_file}"
+ rm -f "${cleaned_file}"This keeps the behavior you actually need (drop old SPDX/Copyright lines before reinjecting) while leaving unrelated block comments elsewhere in the file untouched.
Right now this is a silent source-corruption bug; I would not ship without fixing it.
Also applies to: 368-375
🤖 Prompt for AI Agents
In lib/fix.sh around lines 330-341 (and similarly 368-375), the sed commands are
removing any standalone "/*" or "*/" lines throughout the file which corrupts
non-header block comments; remove the two sed patterns that match
/^[[:space:]]*\*\/[[:space:]]*$/ and /^[[:space:]]*\/\*[[:space:]]*$/ so the
script only deletes SPDX-License-Identifier and Copyright lines, or
alternatively restrict the header-strip to the top-of-file region (e.g., operate
only on the initial comment block or first N lines) so that block comment
delimiters elsewhere in the file are not touched.
| function get_source_files() { | ||
| local files | ||
|
|
||
| # Define common source file extensions | ||
| # Exclude typical config/data files that might be caught by ls-files | ||
| local SOURCE_FILE_EXTENSIONS='\.(sh|bash|py|js|ts|tsx|jsx|go|rs|c|cpp|h|hpp|java|rb|php|tex)$' | ||
| local EXCLUDED_FILE_TYPES='\.(json|toml|yaml|yml|xml|md|txt)$' | ||
|
|
||
| # Check if in a git repository | ||
| if command -v git &> /dev/null && git rev-parse --is-inside-work-tree &> /dev/null; then | ||
| files=$(git ls-files | grep -E "${SOURCE_FILE_EXTENSIONS}" | grep -v -E "${EXCLUDED_FILE_TYPES}" || true) | ||
| else | ||
| # Fallback to find if not a git repo or git command fails | ||
| files=$(find . -type f \ | ||
| \( -name "*.sh" -o -name "*.bash" -o -name "*.py" -o \ | ||
| -name "*.js" -o -name "*.ts" -o -name "*.tsx" -o \ | ||
| -name "*.jsx" -o -name "*.go" -o -name "*.rs" -o \ | ||
| -name "*.c" -o -name "*.cpp" -o -name "*.h" -o \ | ||
| -name "*.hpp" -o -name "*.java" -o -name "*.rb" -o \ | ||
| -name "*.php" -o -name "*.tex" \) \ | ||
| || true) | ||
| fi | ||
|
|
||
| echo "${files}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Core file-discovery logic looks good; tighten fallback to avoid vendor/third‑party trees
The git‑based branch + explicit extension allowlist are solid, and the find() expression is now correctly grouped.
The fallback branch, however:
files=$(find . -type f \( -name "*.sh" -o ... -name "*.tex" \) || true)will happily traverse ./node_modules, ./vendor, build artifacts, etc., for non‑git projects. When fix_source_headers uses this list, you risk:
- Mutating third‑party code (e.g., vendored JS/TS in node_modules).
- Doing a ton of unnecessary I/O on large dependency trees.
Consider excluding common non‑source roots in the fallback, e.g.:
- files=$(find . -type f \
+ files=$(find . \
+ -path "./.git" -prune -o \
+ -path "./node_modules" -prune -o \
+ -path "./vendor" -prune -o \
+ -type f \
\( -name "*.sh" -o -name "*.bash" -o -name "*.py" -o \
-name "*.js" -o -name "*.ts" -o -name "*.tsx" -o \
-name "*.jsx" -o -name "*.go" -o -name "*.rs" -o \
-name "*.c" -o -name "*.cpp" -o -name "*.h" -o \
-name "*.hpp" -o -name "*.java" -o -name "*.rb" -o \
-name "*.php" -o -name "*.tex" \) \
|| true)so you don’t accidentally rewrite dependency trees when git isn’t available.
| @test "check: warnings do not cause check to fail" { | ||
| create_config "MIT" "Test Owner" | ||
|
|
||
| # Create valid LICENSE and README (will pass) | ||
| cat > LICENSE <<EOF | ||
| MIT License | ||
|
|
||
| Copyright (c) 2025 Test Owner | ||
|
|
||
| Permission is hereby granted... | ||
| EOF | ||
| git add LICENSE | ||
|
|
||
| cat > README.md <<EOF | ||
| # Project | ||
| ## License | ||
| MIT | ||
| EOF | ||
| git add README.md | ||
|
|
||
| # No NOTICE file - will warn | ||
| # Create package.json but no dependencyPolicy - will warn | ||
| cat > package.json <<'EOF' | ||
| { | ||
| "name": "test", | ||
| "version": "1.0.0" | ||
| } | ||
| EOF | ||
| git add package.json | ||
|
|
||
| # Check should pass despite warnings | ||
| run_tlb check | ||
| [ "$status" -eq 0 ] | ||
| assert_output_contains "All checks passed" | ||
| assert_output_contains "WARN" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests correctly pin warning vs failure semantics; one misleading comment.
The behaviour you’re asserting is spot‑on:
- With LICENSE + README + NOTICE missing + npm manifest but no node_modules, you get WARNs but exit 0.
- With a missing LICENSE, you still get a hard failure even when README is fine.
Two small nits on the first test:
create_configalways writes adependencyPolicy, so the comment# Create package.json but no dependencyPolicy - will warnis inaccurate—the warning is actually from missingNOTICEandnode_modules. I’d update the comment to match reality to avoid confusing future readers.- If you truly want coverage for the “no dependencyPolicy defined; all licenses allowed” branch in
scan_dependencies, you’ll need a separate test that writes a config withoutdependencyPolicyat all.
Semantics are good; this is just about making the intent explicit.
Also applies to: 207-222
🤖 Prompt for AI Agents
In test/check.bats around lines 170 to 205, the inline comment claiming "Create
package.json but no dependencyPolicy - will warn" is incorrect because
create_config always writes a dependencyPolicy; update that comment to state the
actual warning sources (missing NOTICE and missing node_modules) to avoid
confusion, and add a note suggesting a separate test that creates a config
without dependencyPolicy to exercise the "no dependencyPolicy defined; all
licenses allowed" branch in scan_dependencies.
| @test "deps: npm scan catches deep transitive license violations" { | ||
| create_config "MIT" "Tester" | ||
| mkdir -p node_modules/parent/node_modules/child/node_modules/deep | ||
| cat > node_modules/parent/node_modules/child/node_modules/deep/package.json <<'EOF' | ||
| { | ||
| "name": "deep-bad", | ||
| "version": "2.0.0", | ||
| "license": "GPL-3.0" | ||
| } | ||
| EOF | ||
|
|
||
| run totally-legal-bro check --manifests package.json | ||
| [ "$status" -ne 0 ] | ||
| assert_output_contains "deep-bad@2.0.0 (GPL-3.0)" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deep-transitive test is solid, but the fixture is a bit implicit.
This does exercise the new full-tree traversal correctly, but two nits:
- You never create a top-level
package.json, yet pass--manifests package.json. Todaydetect_pmonly checks the filename, but if it ever starts validating existence you’ll get a silent behavioural change. A tiny stubpackage.jsonhere would make the contract explicit. create_config "MIT" "Tester"also enables Apache-2.0 independencyPolicy. That’s fine for catching GPL-3.0, but the test name reads like “only MIT” — either tighten the config or adjust the comment so future readers aren’t misled.
Functionally fine, just worth de-obfuscating the fixture.
🤖 Prompt for AI Agents
In test/deps.bats around lines 48 to 62, the test relies on an implicit
top-level package.json and a config that enables Apache-2.0 in addition to MIT,
which can hide intent; add an explicit minimal top-level package.json stub
(e.g., {"name":"test","version":"1.0.0"}) before running the check so
--manifests package.json refers to an existing file, and tighten the fixture by
changing the created config to only allow MIT (or update the test name/comment
to reflect that Apache-2.0 is also allowed) so the test intent is explicit and
robust against future detect_pm behavior changes.
| @test "init: backs up existing pre-commit hook (P2)" { | ||
| # Create a git repo | ||
| git init >/dev/null 2>&1 | ||
|
|
||
| # Create a dummy pre-commit hook | ||
| mkdir -p .git/hooks | ||
| echo "#!/bin/bash" > .git/hooks/pre-commit | ||
| echo "echo 'Original hook content'" >> .git/hooks/pre-commit | ||
| chmod +x .git/hooks/pre-commit | ||
|
|
||
| # Run init, say yes to hook setup | ||
| # License, Owner, Dep Policy blank, Git hook yes, CI no | ||
| echo -e "MIT\nTest User\n\ny\nn" | totally-legal-bro init >/dev/null 2>&1 | ||
|
|
||
| # Assert backup file exists | ||
| [ -f .git/hooks/pre-commit.bak ] | ||
|
|
||
| # Assert backup file contains original content | ||
| run grep "Original hook content" .git/hooks/pre-commit.bak | ||
| [ "$status" -eq 0 ] | ||
|
|
||
| # Assert new pre-commit hook contains both original and new content | ||
| run grep "Original hook content" .git/hooks/pre-commit | ||
| [ "$status" -eq 0 ] | ||
| run grep "totally-legal-bro check" .git/hooks/pre-commit | ||
| [ "$status" -eq 0 ] | ||
|
|
||
| # Ensure the merged hook remains executable | ||
| [ -x .git/hooks/pre-commit ] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Excellent coverage of hook backup and idempotency; git init is slightly redundant.
These two tests nail the tricky edges:
- You prove that an existing pre-commit gets backed up, merged with the new snippet, and stays executable.
- You also prove that when the marker is already present,
initbecomes a pure no‑op: no backups, exactly one marker line, and original content intact.
Only nit: both tests call git init even though setup_test_repo has already initialized a repo. It doesn’t break anything, but dropping the extra git init would make the setup a bit less noisy.
Otherwise, this is exactly the kind of safety net the new hook logic needs.
Also applies to: 122-156
🤖 Prompt for AI Agents
In test/init.bats around lines 91 to 120 (and similarly for lines 122 to 156),
remove the redundant git init calls inside the tests since setup_test_repo
already initializes a repository; simply rely on the existing repo setup (or add
an explicit call to setup_test_repo at the start of the test if missing) so the
tests are less noisy but unchanged in behavior.
| teardown() { | ||
| cd / | ||
| [[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" | ||
| [[ -n "${MOCK_HOME:-}" ]] && rm -rf "${MOCK_HOME}" | ||
|
|
||
| # Restore original HOME | ||
| if [[ -n "${ORIGINAL_HOME:-}" ]]; then | ||
| export HOME="${ORIGINAL_HOME}" | ||
| fi | ||
|
|
||
| unset MOCK_HOME REPO_DIR ORIGINAL_HOME SOURCE_ROOT_OVERRIDE INSTALL_SH | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop unused REPO_DIR handling from teardown() for clarity.
In this file REPO_DIR is never set, so:
[[ -n "${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}"is effectively dead code. It doesn’t hurt thanks to the guard, but it does suggest there’s some hidden dependency on a cloned repo when there isn’t. Removing it would make the fixture’s responsibilities crisper.
🤖 Prompt for AI Agents
In test/install.bats around lines 24 to 35, remove the dead REPO_DIR cleanup
from teardown(): the test never sets REPO_DIR so the guarded rm -rf is unused;
edit teardown() to delete only MOCK_HOME and ORIGINAL_HOME handling (and unset
MOCK_HOME ORIGINAL_HOME SOURCE_ROOT_OVERRIDE INSTALL_SH) and drop the [[ -n
"${REPO_DIR:-}" ]] && rm -rf "${REPO_DIR}" line to make the fixture
responsibilities clear.
1 participant
No description provided.