Mirror of 376 by illicitonion · Pull Request #2 · CodeYourFuture/Module-Tools-Mirror-For-AI-Code-Reviewer-Testing

illicitonion · 2026-03-16T10:22:45Z

No description provided.

cyf-ai-code-reviewer · 2026-03-16T10:22:49Z

Thanks for opening a new PR! AI started to review it

github-actions · 2026-03-16T10:22:58Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

github-actions · 2026-03-16T10:22:59Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/cat/cat.js

+            if(options.numberNonEmpty) {
+                //-b option: number non-empty lines
+                if(line.trim()) {
+                    process.stdout.write(
+                        `${String(globalLineCounter).padStart(6)}\t${line}\n`
+                    );
+                    globalLineCounter++;
+                } else {
+                    process.stdout.write('\n');
+                }
+            } else if(options.numberAll) {
+                //-n option: number all lines
+                process.stdout.write(
+                    `${String(globalLineCounter).padStart(6)}\t${line}\n`
+                );
+                globalLineCounter++;
+            } else {
+                //default: just print the line
+                process.stdout.write(line + '\n');
+            }


What differences and similarities do you notice in how each option prints lines? How might you avoid repeating the printing and formatting logic?

I left a similar comment, but with a little more detail

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/ls/ls.js

+    const showAll = args.includes('-a');
+    const onePerLine = args.includes('-1');


If someone else reads this code, would it be immediately obvious what 'showAll' or 'onePerLine' do? Is there a name you could choose that would make their purpose even clearer?

@Droid-An These variable names look pretty reasonable to me as-is - WDYT?

I also think the naming is fine - this feedback seems to be just informational. In the Coolify logs for the deployed app, the AI review output is logged, and this point is classified as 1 on a scale from 1 to 5, while the other points are classified as 2.

Yeah, let's maybe filter out informational things.

When we have quality tracking in place, we could try letting in some % of informational comments to see if people find them useful (maybe leave 10% of informational comments, but gather feedback on which are useful). But this would be a future enhancement, not part of the initial project :)

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/ls/ls.js

+    if(directories.length === 0) {
+        listDirectory(process.cwd(), showAll, onePerLine);
+        return;


Can you think of a way to remove this duplication so that the function for listing directories is always called in the same way, regardless of how many directories or files are provided?

I left a similar comment to this 👍

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/ls/ls.js

+    directories.forEach((arg, index) => {
+        try {
+            const stats = fs.statSync(arg);
+            if(stats.isDirectory()) {
+                //Print header if multiple directories are listed
+                if(directories.length > 1) console.log(`${arg}:`);
+
+                listDirectory(arg, showAll, onePerLine);
+                //add a blank line between directory listings if there are multiple directories
+                if(directories.length > 1 && index < directories.length - 1) console.log('');


Can you think of a way to remove this duplication so that the function for listing directories is always called in the same way, regardless of how many directories or files are provided?

@Droid-An This code looks pretty reasonable to me - do you know what it's trying to nudge towards?

From the logs, it looks like this comment refers to the same point as the one above. I assume it simply specifies the lines where the problematic code appears. Since listDirectories is called twice, the ai posted two comments.

Yeah, consolidating these down into just one comment (either at the first location, or in a line range that covers both) feels more useful?

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/wc/wc.js

+    let totalCounts = { lines: 0, words: 0, bytes: 0 };
+    const multipleFiles = files.length > 1;
+
+    files.forEach(file => {
+        try {
+            const content = fs.readFileSync(file, 'utf-8');
+            const counts = countFileContent(content);
+
+            // Sum counts for total if multiple files
+            totalCounts.lines += counts.lines;
+            totalCounts.words += counts.words;
+            totalCounts.bytes += counts.bytes;


What might happen to the 'totalCounts' object if you only have one file? Does it ever get used? How could you make updating totals conditional on having more than one file?

@Droid-An This comment feels contradictory - before the comments were saying "try not to special case the single-file case" and now it's saying "try to special case the single-file case"?

yes, I agree. I think I need to promt ai to be more consistent

cyf-ai-code-reviewer · 2026-03-16T10:23:06Z

implement-shell-tools/wc/wc.js

+    const parts = [];
+    if(options.line) parts.push(counts.lines);
+    if(options.word) parts.push(counts.words);
+    if(options.byte) parts.push(counts.bytes);
+//if no specific count options are provided, print all counts
+    if(!options.line && !options.word && !options.byte) {
+    //default is to print all counts
+        parts.push(counts.lines, counts.words, counts.bytes);
+    }
+    console.log(parts.join('\t'),filePath);


How would you design the code so that adding future options for 'wc' outputs would not require modifying existing code for every new output?

@Droid-An I'm not sure this comment is specific enough? I guess this is trying to get at the same thing as CodeYourFuture/Module-Tools#376 (comment)?

I think it's telling about lots of if else statements. Here is more detailed description of this issue from ai logs:

description: "Within the 'printCounts' function, you use several if-statements to determine what should be in the output, but the approach means you need to keep updating 'parts' for each new possible option. Have you thought about a way to build up the list of what to print based on the enabled options, or set the default in a way that avoids having to manually check and set each option for every output?",

illicitonion · 2026-03-16T10:45:15Z

implement-shell-tools/cat/cat.js

+                );
+                globalLineCounter++;
+            } else {
+                //default: just print the line


Maybe the tool could have noticed this comment isn't very useful

illicitonion · 2026-03-16T10:45:41Z

implement-shell-tools/cat/cat.js

+            filePatterns.push(arg);
+        }
+    });
+    // -b takes precedence over -n


Maybe the tool could have asked if there's a way to rewrite the code to not need this comment

Interesting, I haven't thought about this, thanks for pointing it out

illicitonion · 2026-03-16T10:45:51Z

implement-shell-tools/ls/ls.js

+    // Check for options
+    const showAll = args.includes('-a');
+    const onePerLine = args.includes('-1');
+    //remove options from args 


This comment feels unnecessary

illicitonion · 2026-03-16T10:46:18Z

implement-shell-tools/wc/wc.js

+        }
+    });
+
+    // If multiple files, print total counts


This comment feels unnecessary

LonMcGregor · 2026-03-16T11:25:25Z

implement-shell-tools/cat/cat.js

+    };
+    const filePatterns = [];
+
+    args.forEach((arg) => {


While it is possible to build an argument parser like this, my approach would be to prefer an argument parser specific library. Is the ai bot set up to recognise situations like this? I guess it is tricky because we don't want it to just suggest replacing the entire file with (in this case) a call to an existing cat tool, but using something like commander's program API might make this more readable.

I think it depend on the goal of the assignment whether trainee supposed to build something from scratch or learn to use existing tools. Ai bot doesn't set up to give feedback on such situations. In this particular task I believe trainees should use commander, since it's covered in the prep for this backlog.

Yeah, the bot is (currently) doing context-free reviews without knowing the assignments - if we want to include "are you solving the correct problem" style reviews, I'd punt that to a future feature request :)

implements-shell-tool

708e4ce

illicitonion added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 16, 2026

github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 16, 2026

cyf-ai-code-reviewer bot reviewed Mar 16, 2026

View reviewed changes

illicitonion commented Mar 16, 2026

View reviewed changes

implement-shell-tools/wc/wc.js

}

});

// If multiple files, print total counts

Copy link

Member Author

illicitonion Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment feels unnecessary

LonMcGregor reviewed Mar 16, 2026

View reviewed changes

		const showAll = args.includes('-a');
		const onePerLine = args.includes('-1');

Conversation

illicitonion commented Mar 16, 2026

Uh oh!

cyf-ai-code-reviewer bot commented Mar 16, 2026

Uh oh!

github-actions bot commented Mar 16, 2026

Uh oh!

github-actions bot commented Mar 16, 2026

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyf-ai-code-reviewer bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants