Skip to content

feat(groq): CLI tool that selects the optimal subset of survival gear under a weight limit using the knapsack algorithm.#3645

Open
polsala wants to merge 1 commit intomainfrom
ai/groq-20260307-0226
Open

feat(groq): CLI tool that selects the optimal subset of survival gear under a weight limit using the knapsack algorithm.#3645
polsala wants to merge 1 commit intomainfrom
ai/groq-20260307-0226

Conversation

@polsala
Copy link
Owner

@polsala polsala commented Mar 7, 2026

Implementation Summary

  • Utility: nightly-survival-gear-optimizer
  • Provider: groq
  • Location: node-utils/nightly-nightly-survival-gear-optimi
  • Files Created: 3
  • Description: CLI tool that selects the optimal subset of survival gear under a weight limit using the knapsack algorithm.

Rationale

  • Automated proposal from the Groq generator delivering a fresh community utility.
  • This utility was generated using the groq AI provider.

Why safe to merge

  • Utility is isolated to node-utils/nightly-nightly-survival-gear-optimi.
  • README + tests ship together (see folder contents).
  • No secrets or credentials touched.
  • All changes are additive and self-contained.

Test Plan

  • Follow the instructions in the generated README at node-utils/nightly-nightly-survival-gear-optimi/README.md
  • Run tests located in node-utils/nightly-nightly-survival-gear-optimi/tests/

Links

  • Generated docs and examples committed alongside this change.

Mock Justification

  • Not applicable; generator did not introduce new mocks.

… under a weight limit using the knapsack algorithm.
@polsala
Copy link
Owner Author

polsala commented Mar 7, 2026

🤖 Review by GROQ Agent

✅ What's solid

  • Self‑contained CLI – the entry point (src/index.js) has a proper shebang, parses arguments, and prints JSON output, making it easy to run directly or after a global install.
  • Correct DP implementation – the 2‑dimensional DP table correctly computes the optimal utility and the back‑track logic returns the selected item names in the right order.
  • Minimal dependencies – only core Node modules (fs, child_process, assert) are used, keeping the utility lightweight.
  • Readable code – functions are small and well‑named (loadItems, knapsack, main), which aids future maintenance.

🧪 Tests

Observation Recommendation
Only a single happy‑path test is present. Add tests for edge cases:
• Empty item list.
maxWeight = 0.
• Items with equal utility/weight (tie‑breaking).
• Non‑integer or negative weight values.
Tests invoke the CLI via a constructed command string (execSync). Use spawnSync (or execFileSync) with an argument array to avoid shell interpolation and make the test platform‑agnostic:
js\nconst { spawnSync } = require('child_process');\nconst result = spawnSync('node', [cliPath, tmpFile, String(maxWeight)], { encoding: 'utf8' });\nassert.strictEqual(result.status, 0);\n
Temporary file is written to a fixed location (tmp_items.json). Use a unique temporary directory (fs.mkdtempSync) or the OS temp folder (os.tmpdir()) to prevent race conditions when tests run in parallel.
Test harness is a single script that prints “All tests passed”. Adopt a test runner (e.g., Mocha, Jest, or Node’s built‑in test runner) so failures are reported with stack traces and CI can detect failures automatically.
No assertions on error handling. Add tests that pass malformed JSON or an invalid weight and verify that the process exits with a non‑zero code and prints the expected error message.

🔒 Security

  • JSON parsingloadItems calls JSON.parse without a try/catch. A malformed file will cause an uncaught exception and a stack trace to be printed. Wrap parsing and surface a user‑friendly error:
    function loadItems(path) {
      try {
        const data = fs.readFileSync(path, 'utf8');
        return JSON.parse(data);
      } catch (err) {
        console.error(`Failed to load items from ${path}: ${err.message}`);
        process.exit(1);
      }
    }
  • Shell injection risk – the test builds a command string (node ${cliPath} ${tmpFile} ${maxWeight}) and executes it via execSync. While the production CLI itself does not use a shell, any future code that does could be vulnerable. Prefer the argument‑array APIs (spawnSync, execFileSync) as shown above.
  • File‑system exposure – the CLI reads any path supplied by the user. Consider adding a sanity check that the file exists and is a regular file before reading:
    if (!fs.existsSync(filePath) || !fs.statSync(filePath).isFile()) {
      console.error('Provided path does not point to a readable file');
      process.exit(1);
    }
  • Weight validation – you already check that maxWeight is a non‑negative integer, which is good. Ensure that each item’s weight and utility are also numbers and non‑negative; otherwise the DP table could behave unexpectedly.

🧩 Docs / Developer Experience

  • README enhancements

    • Clarify the installation steps: mention that a package.json with a bin field is required for global installs, or show how to run via npx/node src/index.js.
    • Document error messages and exit codes (e.g., 1 for usage errors, 2 for file‑read errors).
    • Provide a full example including the command output and a brief explanation of the DP result (utility vs. weight).
    • Add a section on testing that explains the npm test script (once a test runner is added).
  • CLI ergonomics

    • Accept an optional flag (--json or -o) to output the full item objects instead of just names, which can be useful for downstream tooling.
    • Consider supporting stdin (-) for piping JSON directly:
      cat items.json | node src/index.js - 10
  • Package metadata – ensure a package.json exists with at least:

    {
      "name": "nightly-survival-gear-optimizer",
      "version": "0.1.0",
      "bin": { "survival-optimizer": "./src/index.js" },
      "type": "commonjs"
    }

    This enables npm install -g . to expose a clean command name.


🧱 Mocks / Fakes

  • The current test suite creates a real temporary file on disk, which is fine for an integration‑style test. If you want faster unit tests for the pure knapsack function, you can export it from src/index.js and test it directly without spawning a child process:
    const { knapsack } = require('../src/index');
    const result = knapsack(items, 7);
    assert.deepStrictEqual(result, ['Water', 'Food']);
  • For the file‑loading logic, you could mock fs.readFileSync using a library like sinon or Node’s built‑in jest.mock (if using Jest) to avoid filesystem I/O in unit tests. This isolates failures to the parsing logic and speeds up the test suite.

Overall, the utility is a solid addition and the core algorithm works as intended. Strengthening the test coverage, tightening error handling, and polishing the documentation will make the tool more robust and developer‑friendly.

@polsala
Copy link
Owner Author

polsala commented Mar 7, 2026

🤖 Review by GEMINI Agent

✅ What's solid

  • Robust Knapsack Implementation: The 0/1 knapsack algorithm is correctly implemented using dynamic programming, including the backtracking logic to identify the selected items. This forms a solid core for the utility.
  • Self-contained CLI: The src/index.js file is a well-structured CLI tool with a proper shebang, argument parsing, and clear JSON output, making it easy to execute directly or integrate into scripts.
  • Clear Output Format: The utility outputs a JSON array of selected item names, which is machine-readable and consistent.

🧪 Tests

  • Expand Test Scenarios: The current test suite covers one basic optimal selection. Consider adding tests for:
    • Edge cases: empty items list, maxWeight of 0, maxWeight less than any item's weight, all items too heavy, all items fit.
    • Cases where multiple optimal solutions exist (if the problem definition allows for different sets with the same max utility).
    • Items with zero weight or utility (if valid inputs).
  • Validate Error Handling: Add tests to ensure the CLI gracefully handles invalid inputs, such as:
    • Missing arguments (node index.js items.json).
    • Non-numeric maxWeight (node index.js items.json abc).
    • Non-existent items.json file.
    • Malformed items.json (e.g., invalid JSON syntax).
  • Assert Total Utility: Beyond just the selected item names, it would be beneficial to assert the total utility of the selected items to fully verify the knapsack algorithm's optimality. This might require a slight modification to the test helper or the main utility to also output the total utility.
  • Consider a Testing Framework: For improved test structure, reporting, and assertion capabilities, consider adopting a dedicated testing framework like Mocha or Jest if the test suite is expected to grow.

🔒 Security

  • Path Traversal Vulnerability: The loadItems function uses fs.readFileSync(path, 'utf8') directly with a user-provided filePath. This could expose the system to path traversal attacks if an attacker provides a path like ../../../../etc/passwd, allowing them to read arbitrary files.
    • Actionable Feedback: Implement robust path sanitization. For example, resolve the path and ensure it remains within an expected directory, or explicitly disallow .. segments.
    const path = require('path');
    const fs = require('fs');
    
    function loadItems(filePath) {
      const resolvedPath = path.resolve(filePath);
      // Example: Ensure the file is within a designated 'data' directory
      // if (!resolvedPath.startsWith(path.resolve('./data'))) {
      //   throw new Error('Access denied: File must be in data directory.');
      // }
      // Or, more generally, disallow directory traversal
      if (resolvedPath.includes('..')) {
        throw new Error('Invalid path: Directory traversal not allowed.');
      }
      const data = fs.readFileSync(resolvedPath, 'utf8');
      return JSON.parse(data);
    }
  • JSON Parsing Error Handling: The JSON.parse call in loadItems is not wrapped in a try-catch block. Malformed JSON input will cause the utility to crash.
    • Actionable Feedback: Add error handling around JSON.parse to provide a more graceful exit and informative error message to the user.
    function loadItems(path) {
      try {
        const data = fs.readFileSync(path, 'utf8');
        return JSON.parse(data);
      } catch (error) {
        if (error instanceof SyntaxError) {
          console.error(`Error: Malformed JSON in ${path}.`, error.message);
        } else if (error.code === 'ENOENT') {
          console.error(`Error: File not found at ${path}.`);
        } else {
          console.error(`An unexpected error occurred while reading ${path}:`, error.message);
        }
        process.exit(1);
      }
    }
  • Large Input File Handling: fs.readFileSync reads the entire items.json file into memory. For extremely large input files, this could lead to memory exhaustion.
    • Actionable Feedback: Document the expected limits for the items.json file size. For future scalability, consider implementing a streaming approach for parsing large JSON files if this becomes a concern.

🧩 Docs/DX

  • Clarify CLI Arguments in README: The usage example node src/index.js items.json 10 is clear, but explicitly stating that 10 represents maxWeight in the usage section would enhance clarity for new users.
  • Document Error Messages: Add a section to the README.md detailing common error messages and their causes (e.g., file not found, malformed JSON, invalid maxWeight). This improves debuggability for users.
  • Input Schema/Type Definition: For better developer experience, consider providing a JSON schema or a TypeScript interface definition for the expected items.json structure. This helps users understand the required format and can be used for input validation.
  • Alternative Output Formats: While JSON is excellent for programmatic use, consider adding an optional flag (e.g., --pretty or --human-readable) to output the results in a more human-friendly format for quick checks, in addition to the default JSON.

🧱 Mocks/Fakes

  • Acknowledge Test Mocks: The PR body states "Not applicable; generator did not introduce new mocks." However, the test_main.js file does create a temporary tmp_items.json file to simulate input for the CLI. This is a form of faking/mocking the file system interaction.
    • Actionable Feedback: Update the "Mock Justification" in the PR body to acknowledge and justify the use of temporary files for testing CLI input/output.
  • Improve Unit Test Isolation: The current test approach relies on child_process.execSync to run the entire CLI. While good for integration testing, it couples the test to file system operations.
    • Actionable Feedback: For more granular unit testing of the knapsack algorithm itself, consider exporting the knapsack function from src/index.js. This would allow tests to directly call the function with in-memory item arrays, isolating the algorithm from file I/O and argument parsing, leading to faster and more focused unit tests.
    // In src/index.js, at the end:
    // module.exports = { knapsack, loadItems, main }; // or similar exports
    
    // In tests/test_main.js (or a new unit test file):
    // const { knapsack } = require('../src/index');
    // const itemsForUnit = [
    //   { name: 'A', weight: 1, utility: 1 },
    //   { name: 'B', weight: 2, utility: 3 }
    // ];
    // assert.deepStrictEqual(knapsack(itemsForUnit, 2), ['B']);

@polsala
Copy link
Owner Author

polsala commented Mar 8, 2026

🤖 Review by OPENROUTER Agent

✅ What's solid

  • Knapsack Algorithm Correctness: The dynamic programming implementation of the 0/1 knapsack problem is correct. It builds a 2D table to compute maximum utility and includes functional backtracking to determine the selected items.
  • CLI Structure: The command-line interface is well-structured with appropriate argument parsing, input validation, and JSON output formatting. The shebang line allows direct execution.
  • Test Coverage: Basic functionality is covered by a test case that verifies the selection of optimal items given a fixed dataset and weight constraint.

🧪 Tests

  • Expand Test Cases: Add more edge cases such as:
    • Empty item list
    • Items with zero weights or utilities
    • Max weight of zero
    • Duplicate items
    • Large datasets to check performance
  • Improve Test Robustness: Consider using a testing framework like Jest or Mocha for better assertion handling and clearer error messages. Example improvement:
    // Instead of relying on deepStrictEqual alone
    try {
      assert.deepStrictEqual(result, expected);
    } catch (error) {
      console.error(`Expected ${expected}, but got ${result}`);
      throw error;
    }

🔒 Security

  • Input Validation: While basic checks exist, enhance validation for file paths to prevent directory traversal attacks. For example:
    const path = require('path');
    if (path.relative(process.cwd(), filePath).startsWith('..')) {
      console.error("Invalid file path");
      process.exit(1);
    }
  • Safe File Reading: Ensure that only valid JSON files are processed and handle potential errors during file read operations gracefully.

🧩 Docs/DX

  • Clarify Installation Instructions: Specify whether users should run npm install -g . from within the project directory. Also consider adding instructions for local development setup.
  • Add Examples Section: Include additional usage examples in the README showing different scenarios (e.g., empty inputs, various outputs).
  • Document Exit Codes: Explain what exit codes mean when running the CLI tool (e.g., success vs failure due to invalid arguments).

🧱 Mocks/Fakes

  • Mock External Dependencies in Tests: Although there aren't many external dependencies here, mocking fs and child_process modules would make tests faster and more reliable. Use libraries like sinon or built-in Jest mocks:
    jest.mock('fs');
    jest.mock('child_process', () => ({
      execSync: jest.fn()
    }));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant