fix: license-text detection, expression splitting, and no-placeholder compliance by knitli-proxy-coder[bot] · Pull Request #3 · knitli/licet

knitli-proxy-coder · 2026-06-29T00:43:40Z

🤖 Opened by Proxy Coder on behalf of Adam Poulemanos (@bashandbone)

Summary

Fixes a cluster of license.toml / LICENSES/ handling bugs that could leave a repo silently non-compliant, plus the prior init/detect TOML-escaping fix (6d1e750).

License-text detection & materialization (`5ee7a20`)

.md / extension-less texts ignored. LICENSES/ discovery only matched .txt, so LICENSES/<id>.md and suffix-less files were invisible and re-materialized as .txt. Now recognizes .txt / .md / no-suffix, matching reuse (a versioned LicenseRef-Acme-1.0 keeps its .0).
Compound expressions treated as one id. A detected MIT OR Apache-2.0 was inserted whole, producing LICENSES/MIT OR Apache-2.0.txt. The detected license now flows through collect_ids, splitting into constituents.
Placeholder scaffolding removed. Stubbing a missing text satisfied REUSE's text-existence check while leaving the repo non-compliant — "looks compliant when it isn't". materialize never scaffolds; apply / add-license exit 1 on an unsuppliable text, and the report's PASS/FAIL now folds in missing_license_text so it matches the exit code. apply also materializes against post-apply references, so a license being replaced isn't spuriously demanded.
Actionable guidance instead of --allow-network. Standard ids get the exact SPDX raw-text URL; LicenseRef- ids get the LICENSES/<id>.txt path to create.
Opt-in fetch. --allow-network → --allow-curl, which shells out to the user's own curl (no network crate — offline-guard stays green). Flag grants it outright; an interactive TTY without the flag is asked y/N; non-interactive without the flag errors with the URL. Default invocation stays fully hermetic.

Specs/contracts (FR-017, FR-029), data-model, cli.md, research, tasks, and README updated to match.

Repo self-licensing (`af2de3b`)

Generated license.toml + materialized LICENSES/ texts so licet's own tree is REUSE-compliant; bump mise cargo:licet pin 0.2.1 → 0.2.2.

Also included (`6d1e750`, prior)

init/detect: escape generated TOML and reject non-SPDX detected values.

Testing

cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, cargo test --all-targets — all green.
New integration coverage: compound-expression split, .md recognition, never-scaffold/error-with-guidance for LicenseRef.
Smoke-tested error and success paths against throwaway repos.

🤖 Generated with Claude Code

Co-Authored-By: bashandbone 89049923+bashandbone@users.noreply.github.com
On-Behalf-Of: Adam Poulemanos (@bashandbone) (via knitli-proxy-coder[bot])

…ns, never stub placeholders Five related license-text bugs, all in the materialize/inventory path: 1. LICENSES/ discovery only matched `.txt`, so `LICENSES/<id>.md` and extension-less files were invisible and re-materialized. `present_texts` now recognizes `.txt`/`.md`/no-suffix, matching `reuse` (a versioned `LicenseRef-Acme-1.0` keeps its `.0`, never split as an extension). 2. A detected compound expression (`MIT OR Apache-2.0`) was inserted whole into the referenced-id set, producing `LICENSES/MIT OR Apache-2.0.txt`. The detected license now flows through `collect_ids`, splitting it into its constituents like the config path already did. 3. Missing texts were stubbed with a `TODO` placeholder, which satisfies REUSE's text-existence check while leaving the repo non-compliant — "looks compliant when it isn't". `materialize` never scaffolds now; `apply`/`add-license` exit 1 on an unsuppliable text, and the report's PASS/FAIL folds in `missing_license_text` so it matches the exit code. `apply` also materializes against the *post-apply* references, so a license being replaced is no longer spuriously demanded. 4. Error/hint text pointed at `--allow-network`, unusable in the hermetic build. New `missing_text_guidance` gives the exact SPDX raw-text URL for a standard id, or the `LICENSES/<id>.txt` path for a `LicenseRef-`. 5. Opt-in fetch: `--allow-network` -> `--allow-curl`, which shells out to the user's own `curl` (no network crate, offline-guard stays green). Flag grants it outright; an interactive TTY without the flag is asked y/N; non-interactive without the flag errors with the URL. Default invocation stays fully hermetic. Specs/contracts (FR-017, FR-029), data-model, cli.md, research, tasks, and README updated to match. fmt + clippy -D warnings + all tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DhtjtENVBBE27jRqJuxAw

…se licet pin Generated `license.toml` declaring the repo's own MIT OR Apache-2.0 intent (CC-BY-SA-4.0 for the vendored REUSE spec) plus the materialized LICENSES/ texts, so licet's own tree is REUSE-compliant. Bump the mise `cargo:licet` tool pin 0.2.1 -> 0.2.2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DhtjtENVBBE27jRqJuxAw

codacy-production · 2026-07-06T00:22:20Z

Not up to standards ⛔

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🔴 Metrics 198 complexity · 22 duplication

Metric Results

Complexity ⚠️ 198 (≤ 20 complexity)

Duplication ⚠️ 22 (≤ 2 duplication)

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

codacy-production

Pull Request Overview

The review identified several critical issues that must be addressed before merging. Codacy reports that the PR is not up to standards, primarily due to excessive cyclomatic complexity (37) in src/cli/apply.rs and significant code duplication in integration tests.

A high-severity security risk was identified in src/reuse/inventory.rs, where unsanitized license identifiers could lead to path traversal during file discovery or fetching. Furthermore, the tool's --format json contract is broken by status messages sent to stdout, and IO errors are being swallowed in the apply command, which contradicts requirement FR-017 regarding mandatory failure on missing texts.

Note that while the PR description mentions 'init/detect' logic for TOML escaping and SPDX rejection, this logic appears missing from the provided implementation. Additionally, new safety checks for LicenseRef identifiers currently show zero test coverage, suggesting they are not being executed by the CI suite.

About this PR

Systemic pattern: Interactive prompts and status messages are being directed to stdout. This breaks compatibility with --format json and makes prompts invisible when stdout is redirected to a file or pipe. All non-report output should be directed to stderr.
The PR title and description mention 'init/detect: escape generated TOML and reject non-SPDX detected values' (commit 6d1e750), but the logic for these specific fixes was not found in the current diff. Please verify if these changes were omitted accidentally.

2 comments outside of the diff

specs/001-declarative-license-headers/data-model.md

_{line 55 ⚪ LOW RISK}
Nitpick: Lists in Markdown should be preceded and followed by blank lines to ensure consistent rendering across different engines.

src/cli/apply.rs

_{line 19 🔴 HIGH RISK}
The run function has reached a cyclomatic complexity of 37, far exceeding the standard limit of 8. This makes the logic for dry-runs and post-apply reconciliation extremely brittle and difficult to audit. Extract the post-apply scan (lines 223-232) and materialization loop (lines 234-251) into dedicated private helper functions.

Test suggestions

Verify LICENSES/ discovery recognizes .md and extension-less files.
Verify compound expressions are split into separate IDs for text matching.
Verify LicenseRef-* identifiers are not stubbed and return exit 1 with path guidance.
Verify standard identifiers absent from bundle can be fetched via --allow-curl (shelling out).
Verify 'apply' materializes missing texts against the state resulting after headers are written.
Address zero coverage for safety checks in src/reuse/inventory.rs to ensure logic is actually exercised.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify 'apply' materializes missing texts against the state resulting after headers are written.
2. Address zero coverage for safety checks in src/reuse/inventory.rs to ensure logic is actually exercised.

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-06T00:23:30Z


    #[test]
-    fn scaffolds_license_ref_placeholder() {
+    fn license_ref_is_reported_missing_never_scaffolded() {


_{🔴 HIGH RISK}

Coverage Gap: The new integration tests verifying that LicenseRef identifiers are never stubbed (lines 197-210) have zero coverage. This indicates the logic is not being executed in the current test suite; ensure the #[test] attributes are correct and the file is included in the test runner.

See Coverage in Codacy

codacy-production · 2026-07-06T00:23:30Z

+    }
+    let dir = root.join("LICENSES");
+    std::fs::create_dir_all(&dir).map_err(|e| e.to_string())?;
+    let dest = dir.join(format!("{id}.txt"));


_{🔴 HIGH RISK}

Security: The id parameter is used to construct file paths and URLs without sanitization. A malformed SPDX-License-Identifier (e.g., containing ../) could result in path traversal when joined with the LICENSES directory. Ensure id is validated to contain only alphanumeric characters, dots, and hyphens before use.

codacy-production · 2026-07-06T00:23:30Z

+            continue;
+        }
+        match crate::reuse::inventory::fetch_via_curl(root, id) {
+            Ok(path) => println!("  ↓ fetched {id} → {}", path.display()),


_{🔴 HIGH RISK}

Printing fetch status messages directly to stdout breaks the contract for --format json output. These messages will corrupt the structured report, causing parsing failures for downstream consumers. Status updates should be sent to stderr or integrated into the report's internal warning structure.

codacy-production · 2026-07-06T00:23:30Z

+    let mut missing_texts: Vec<String> = Vec::new();
    if !args.dry_run
-        && let Ok(res) = inventory::materialize(&root, &scan.referenced_ids)
+        && let Ok(res) = inventory::materialize(&root, &post_referenced)


_{🟡 MEDIUM RISK}

IO errors from inventory::materialize are currently swallowed. If materialization fails (e.g., due to disk permissions), the process may exit with code 0 despite failing to satisfy requirement FR-017, which mandates the run MUST fail when a required text cannot be supplied.

codacy-production · 2026-07-06T00:23:30Z

+/// Ask a y/N question on the terminal; any non-affirmative reply (including EOF) is No.
+fn prompt_yes_no(question: &str) -> bool {
+    use std::io::Write;
+    print!("{question} [y/N] ");


_{🟡 MEDIUM RISK}

Suggestion: Direct this prompt to stderr. If stdout is redirected (e.g., licet add-license > report.json), the user will not see the prompt and the process will appear to hang indefinitely.

codacy-production

Pull Request Overview

This PR is currently not up to standards due to high-priority security concerns and logic gaps. While it successfully implements SPDX expression splitting and improved license detection, it introduces a path traversal vulnerability in how license identifiers are handled during filesystem operations and network fetching. Furthermore, the use of stdout for interactive prompts and progress updates will corrupt the output for users relying on the --format json flag.

Critically, the TOML escaping and non-SPDX rejection logic mentioned in the PR metadata (commit 6d1e750) is missing from the provided code changes. Several high-complexity files, including src/cli/apply.rs (complexity 37), are identified as uncovered by tests, which is concerning given that the current implementation silently ignores IO errors during the materialization process. These issues must be addressed before the PR can be safely merged.

About this PR

Missing Definition: The collect_ids helper function is used in src/engine.rs to support compound expressions, but its implementation was not included in the PR files.
Incomplete Implementation: The PR description references changes for TOML escaping and non-SPDX value rejection (linked to commit 6d1e750), but these changes are absent from src/cli/init.rs and src/detect/mod.rs in this diff.

2 comments outside of the diff

src/cli/add_license.rs

_{line 42 🟡 MEDIUM RISK}
Suggestion: Configuration loading errors are silently suppressed using unwrap_or_default(). If a user-provided --config path is invalid or unreadable, the tool should fail with an error instead of silently falling back to an empty configuration.

src/cli/apply.rs

_{line 19 🔴 HIGH RISK}
Suggestion: The run function's cyclomatic complexity (37) significantly exceeds maintainability standards. It interleaves file selection, atomic writes, and network fetching. Consider extracting the post-application re-scan and license text materialization logic (lines 223-251) into a private helper function named finalize_application.

Test suggestions

Materialize individual license texts from a compound SPDX expression.
Recognize .md and extension-less files in LICENSES/ as existing identifiers.
Verify LicenseRef-* identifiers are not scaffolded and result in exit code 1 with path guidance.
Verify that 'apply' materialization uses references from the post-apply state (not pre-apply).
Verify TOML escaping and rejection of non-SPDX identifiers in the 'init' command.
Automate unit tests for path traversal prevention in src/reuse/inventory.rs.
Verify JSON output integrity when interactive prompts are triggered in src/cli/mod.rs.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify that 'apply' materialization uses references from the post-apply state (not pre-apply).
2. Verify TOML escaping and rejection of non-SPDX identifiers in the 'init' command.
3. Automate unit tests for path traversal prevention in `src/reuse/inventory.rs`.
4. Verify JSON output integrity when interactive prompts are triggered in `src/cli/mod.rs`.

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-06T00:24:16Z

+            continue;
+        }
+        match crate::reuse::inventory::fetch_via_curl(root, id) {
+            Ok(path) => println!("  ↓ fetched {id} → {}", path.display()),


_{🔴 HIGH RISK}

Output Corruption: Progress messages and interactive prompts are written to stdout. This will interleave text with JSON output when using --format json, resulting in invalid JSON streams. Use eprintln! and eprint! to direct these messages to stderr instead.

codacy-production · 2026-07-06T00:24:16Z

+pub fn fetch_via_curl(root: &Path, id: &str) -> Result<PathBuf, String> {
+    if spdx::is_license_ref(id) {
+        return Err(format!("{id} is a custom LicenseRef and cannot be fetched"));
+    }
+    let dir = root.join("LICENSES");
+    std::fs::create_dir_all(&dir).map_err(|e| e.to_string())?;
+    let dest = dir.join(format!("{id}.txt"));


_{🔴 HIGH RISK}

High-Risk Path Traversal: The id identifier extracted from source files or user input is used to construct file paths and remote URLs without validation. An attacker could use an identifier like ../../evil to overwrite files outside the LICENSES/ directory. Validate that the identifier contains only alphanumeric characters, dots, or dashes.

Suggested fix:

if spdx::is_license_ref(id) || id.contains('/') || id.contains('\\') || id.contains("..") { return Err(format!("{id} is not a valid standard SPDX identifier for fetching")); }

codacy-production · 2026-07-06T00:24:16Z

+    let mut missing_texts: Vec<String> = Vec::new();
    if !args.dry_run
-        && let Ok(res) = inventory::materialize(&root, &scan.referenced_ids)
+        && let Ok(res) = inventory::materialize(&root, &post_referenced)


_{🟡 MEDIUM RISK}

Critical Error Suppression: Errors from inventory::materialize (e.g., permission denied when creating directories) are silently ignored via if let Ok. This could result in a successful exit code despite failing to materialize required license texts, violating requirement FR-017.

Adam Poulemanos (bashandbone) and others added 2 commits June 28, 2026 20:42

codacy-production Bot reviewed Jul 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: license-text detection, expression splitting, and no-placeholder compliance#3

fix: license-text detection, expression splitting, and no-placeholder compliance#3
knitli-proxy-coder[bot] wants to merge 2 commits into
mainfrom
fix/init-toml-escaping-and-spdx-validation

knitli-proxy-coder Bot commented Jun 29, 2026

Uh oh!

codacy-production Bot commented Jul 6, 2026

Uh oh!

codacy-production Bot left a comment

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot left a comment

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

codacy-production Bot Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

knitli-proxy-coder Bot commented Jun 29, 2026

Summary

License-text detection & materialization (5ee7a20)

Repo self-licensing (af2de3b)

Also included (6d1e750, prior)

Testing

Uh oh!

codacy-production Bot commented Jul 6, 2026

Not up to standards ⛔

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

License-text detection & materialization (`5ee7a20`)

Repo self-licensing (`af2de3b`)

Also included (`6d1e750`, prior)