Skip to content

Add C workflow docs and streamlined run builtin - #105

Merged
orpheus497 merged 6 commits into
develop/compfrom
phase1
Aug 1, 2026
Merged

Add C workflow docs and streamlined run builtin#105
orpheus497 merged 6 commits into
develop/compfrom
phase1

Conversation

@orpheus497

@orpheus497 orpheus497 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Why

mcsh needs a first-class C workflow that is easier than juggling direct compiler and build-tool invocations. This branch documents the compiler/workflow design and lands the first executable slice: a run command that drives compile, build, and execute in one path.

What changed

  • Added comprehensive workflow/compiler documentation and planning artifacts under docs/compiler/, plus related updates to top-level docs (README.md, PLAN.md, ISSUES.md, SYNTAX_AND_SCRIPTING.md) and C workflow analysis/spec docs.
  • Added builtin wiring for run in shell dispatch/build integration (sh.init.c, sh.decls.h, Makefile.in).
  • Implemented sh.cworkflow.c with a staged, data-oriented pipeline (parse -> validate -> prepare -> compile -> build -> execute).
  • run now supports both single C file targets and directory/project targets, with object and binary cache paths derived from source/toolchain hashes.
  • Enforced clang-family toolchain selection (including $mcsh_cc validation) to keep runtime/tooling alignment intact.
  • Added run validation and behavior tests (tests/t100_run_validation.sh, tests/t101_run_basic.sh) covering misuse handling, arg passthrough, exit code propagation, cache reuse, and directory builds.

Notes for review

  • The new run path intentionally focuses on streamlined compile/build/run behavior (--clean, -v, optional -- separator) rather than broader profile/flag matrices; those are tracked in the planning docs.
  • This branch also includes earlier documentation and shell cleanup commits already on phase1; reviewing by commit will make the progression clearer.

Summary by CodeRabbit

  • New Features

    • Added the run command for compiling and executing C files or directory projects.
    • Added caching, cache cleanup, verbose output, argument forwarding, and directory-target support.
    • Added validation for targets, .mcsh files, and supported compiler toolchains.
    • Execution failures and program exit statuses are now reported correctly.
  • Bug Fixes

    • Corrected directory-stack navigation for numeric cd targets.
  • Documentation

    • Updated compiler documentation and plans to reflect shipped and planned workflows.
  • Tests

    • Added coverage for execution, caching, arguments, projects, validation, and navigation.

orpheus497 and others added 2 commits July 28, 2026 17:11
Wire the run builtin into the shell, add a staged data-oriented compile/build/run pipeline for C file and directory targets, and enforce clang-family toolchain selection for BSD/Apache-2.0 alignment.

Update run tests for validation, streamlined argv passthrough, and directory project builds, and align CLI spec docs with the implemented run syntax/options.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 16234030-f13f-4694-815d-b78a2cc79429

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change introduces a cached run builtin for C files and directories, wires it into the build, adds extensive C-workflow planning documentation, expands shell documentation and regression coverage, and applies several shell compatibility and truncation fixes.

Changes

C workflow and shell modernization

Layer / File(s) Summary
Workflow contracts and planning
ISSUES.md, PLAN.md, README.md, SYNTAX_AND_SCRIPTING.md, docs/c-commands-spec.md, docs/c-workflow-analysis.md, docs/compiler/*
Defines the planned compile, build, and run commands, their .mcsh boundaries, cache/data models, milestones, pipeline behavior, risks, and testing strategy.
Cached run builtin implementation
sh.cworkflow.c, sh.decls.h, sh.init.c, sh.sem.c, Makefile.in
Adds dorun, registers run, builds and caches C targets, forwards arguments, executes binaries, and links the new object into mcsh.
Workflow and shell regression coverage
tests/t005_cd_stack.sh, tests/t017_cd_minus_n.sh, tests/t018_syntax_highlight.sh, tests/t019_git_prompt.sh, tests/t020_predict.sh, tests/t021_onintr.sh, tests/t022_function_args.sh, tests/t023_filetest_ops.sh, tests/t024_variable_modifiers.sh, tests/t025_switch.sh, tests/t100_run_validation.sh, tests/t101_run_basic.sh
Adds shell feature smoke tests and validates run input errors, compiler execution, cache reuse, exit statuses, argument forwarding, and project builds.
Shell compatibility and maintenance fixes
sh.c, sh.dir.c, sh.func.c, sh.sem.c, tc.prompt.c, tcsh.man.in, fix_truncation.patch
Updates catalog fallback, directory-stack indexing, platform declarations, prompt truncation handling, signal preprocessing, and user-facing shell documentation.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Poem

A bunny found a cache beneath the tree,
Built little C programs swiftly.
“Run!” cried the hare, as binaries flew,
Shell scripts stayed safely in their queue.
Documentation blossomed bright—
Hop, compile, link, delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: new C workflow documentation and the added streamlined run builtin.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch phase1

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@orpheus497
orpheus497 changed the base branch from master to develop/comp July 28, 2026 07:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
sh.init.c (1)

43-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Restore the unrelated platform builtin registrations.

This change removes guarded builtins such as att, bye, getspath, getxvers, migrate, and universe. Adding run should not silently remove supported commands on their respective platforms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sh.init.c` around lines 43 - 153, Restore the platform-guarded builtin
registrations removed from the builtin table, including att, bye, getspath,
getxvers, migrate, and universe, using their original conditional compilation
guards and handlers. Keep the new run entry and existing alphabetical ordering
intact, ensuring each platform retains its supported commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/c-commands-spec.md`:
- Around line 1-3: Update the documentation to reflect delivered P1 support: in
docs/c-commands-spec.md (lines 1-3) describe run as implemented while keeping
compile/build planned; in docs/c-workflow-analysis.md (lines 1-3) distinguish
completed P1 behavior from future phases; in docs/compiler/ARCHITECTURE.md
(lines 1-4) update status and remaining integration work; in
docs/compiler/CACHE-DESIGN.md (lines 1-4) distinguish the implemented cache
layout from future index/metadata features; in docs/compiler/CLI-SPEC.md (lines
1-17) mark compile/build syntax as planned and document only supported run
options; in docs/compiler/DATA-MODEL.md (lines 1-4) identify the model as a
future target beyond the initial implementation; in
docs/compiler/DEPENDENCY-DISCOVERY.md (lines 1-4) describe dependency scanning
as future work; and in docs/compiler/INDEX.md (lines 1-5) remove the claim that
no runtime is implemented.

In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 55-77: Update the cache-key design in the documented key
construction and normalization section to preserve order and multiplicity:
replace XOR of object keys with a length-delimited ordered sequence hash, and
hash the effective linker/compiler argv in its original order. Remove sorting of
order-sensitive linker flags and canonicalized -I paths, while retaining only
normalization that does not change argument order or meaning.

In `@docs/compiler/DECISIONS.md`:
- Around line 223-227: Update the ADR-006 execution-model documentation around
the `run` behavior to match the implementation: `cw_run_execute_stage()` runs
the stage in a child, returns its status to `dorun()`, and `dorun()` updates
shell status and performs cleanup. Remove or correct the recommendation to use
direct `execvp` so the documented model preserves fork-and-wait behavior and
returns control to the pipeline.

In `@docs/compiler/DEPENDENCY-DISCOVERY.md`:
- Around line 145-158: Revise the “mtime-gated hashing” section to remove the
claim that matching mtime and size safely validates content_hash. Require
hashing of cache-key inputs for correctness, or explicitly document the
optimization as best-effort and add a verification path before accepting the
cached hash.

In `@docs/compiler/MASTER-PLAN.md`:
- Around line 3-4: Synchronize the compiler planning documents with the
implemented PR: in docs/compiler/MASTER-PLAN.md at lines 3-4, replace the
no-runtime status and update the corresponding P1 checklist; in
docs/compiler/MILESTONES.md at lines 3-4, replace the planning-only status and
accurately mark the shipped M1 work.

In `@docs/compiler/PIPELINES.md`:
- Around line 291-303: Update the Stage 4 “Execute” section to document the
shipped fork+exec+waitpid behavior in cw_execute_binary/cw_run_execute_stage
rather than direct execvp replacement. Describe the forked child executing the
binary, the parent waiting and propagating its status, and retain the
exec-failure exit code 127; remove claims that run replaces the mcsh process
directly or requires avoiding an intermediate shell process.

In `@ISSUES.md`:
- Around line 9-21: Refresh the compiler planning documentation to match the
shipped dorun() implementation: in ISSUES.md lines 9-21, move CW-001 from Open
to Resolved and note its directory-target support; in PLAN.md lines 48-57, mark
M1/run as implemented and note directory/project support moved forward from M3;
in docs/compiler/PIPELINES.md lines 1-6, remove or revise the
planning-only/no-implementation status for the run pipeline; and in
docs/compiler/TODO.md lines 28-83, check off the completed P1 items for
sh.cworkflow.c, dorun(), sh.decls.h/sh.init.c wiring, and the run tests.

In `@sh.cworkflow.c`:
- Around line 959-978: Update cw_execute_binary and the cw_run_execute_stage
handling to distinguish an execv failure from a user program that exits 127,
mirroring the existing compile/build-stage status == 127 heuristic. Emit the
required “run: exec failed: <strerror>” diagnostic using the captured execution
error, preserve exit code 127, and keep the existing fork-failure handling
unchanged.
- Around line 807-826: Document the known limitation that project_hash, used by
cw_compute_object_path, is incorporated into every object cache key for
directory targets, causing all objects to rebuild when any discovered source or
header changes. Add this limitation to the appropriate RISKS.md or PLAN.md
Phase-3/known-limitations section, noting that precise incremental caching
awaits include-dependency discovery; do not change the hashing behavior.
- Around line 828-840: Update cw_hash_toolchain and its callers so repeated run
invocations do not SHA-256-hash the entire compiler binary; cache the toolchain
identity keyed by resolved compiler path, modification time, and size, and reuse
it when those attributes are unchanged. Recompute and refresh the cached hash
only when the key changes, while preserving the existing fallback behavior for
hashing the path when file hashing fails.

In `@tcsh.man.in`:
- Around line 8909-8913: Correct the category-count statement in the
compile-time default-colors documentation near the listed token categories:
either update the enumeration to include all 12 categories or remove the
unsupported numeric count, while keeping the existing category descriptions
accurate.
- Around line 8483-8496: Update the %G operation-state documentation to match
the detection logic in git_get_info: remove REBASE_HEAD and
rebase-merge/head-name, and document rebase-apply/rebasing, rebase-apply (AM),
REVERT_HEAD, and BISECT_LOG with their corresponding states.

In `@tests/t023_filetest_ops.sh`:
- Around line 4-12: The test fixture around the C-shell -z check currently uses
an empty tmpfile while expecting ! -z to print z_ok. Update the fixture or
expectation in the test command so it matches correct -z semantics, preserving
the existing checks for -d, -f, and -e.

In `@tests/t100_run_validation.sh`:
- Around line 37-48: Update the non-clang compiler enforcement test around the
gcc availability check to run unconditionally using a guaranteed non-clang
executable such as /bin/sh. Remove the environment-dependent skip, while
preserving the non-zero status assertion and “not a clang-family compiler”
message validation; if no suitable executable can be selected, fail the test
explicitly.

In `@tests/t101_run_basic.sh`:
- Around line 66-71: Strengthen the second invocation in the cache reuse test
around the mcsh command so it runs with verbose output and asserts that no
compile or link actions are emitted, while preserving the existing status and
“hello” output checks. Verify cached artifacts are reused rather than merely
confirming repeated execution correctness.
- Around line 80-85: Extend the run argument-passthrough test around the
existing status/output assertion to also invoke run with the `--` separator
before `one two`. Assert that this dedicated parsing path returns success and
produces exactly `one two`, using the same failure reporting and exit behavior
as the existing direct-forwarding check.

---

Outside diff comments:
In `@sh.init.c`:
- Around line 43-153: Restore the platform-guarded builtin registrations removed
from the builtin table, including att, bye, getspath, getxvers, migrate, and
universe, using their original conditional compilation guards and handlers. Keep
the new run entry and existing alphabetical ordering intact, ensuring each
platform retains its supported commands.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 64a83388-2554-4b51-91d0-ef216852d958

📥 Commits

Reviewing files that changed from the base of the PR and between ab1fcea and 30cb77a.

📒 Files selected for processing (47)
  • ISSUES.md
  • Makefile.in
  • PLAN.md
  • README.md
  • SYNTAX_AND_SCRIPTING.md
  • docs/c-commands-spec.md
  • docs/c-workflow-analysis.md
  • docs/compiler/ARCHITECTURE.md
  • docs/compiler/CACHE-DESIGN.md
  • docs/compiler/CLI-SPEC.md
  • docs/compiler/DATA-MODEL.md
  • docs/compiler/DECISIONS.md
  • docs/compiler/DEPENDENCY-DISCOVERY.md
  • docs/compiler/INDEX.md
  • docs/compiler/MASTER-PLAN.md
  • docs/compiler/MILESTONES.md
  • docs/compiler/PIPELINES.md
  • docs/compiler/RISKS.md
  • docs/compiler/TEST-PLAN.md
  • docs/compiler/TODO.md
  • fix_truncation.patch
  • sh.c
  • sh.cworkflow.c
  • sh.decls.h
  • sh.dir.c
  • sh.func.c
  • sh.init.c
  • sh.sem.c
  • strncpy_analysis.md
  • tc.prompt.c
  • tc.prompt.c.orig
  • tcsh.man.in
  • test2
  • test3
  • tests/t005_cd_stack.sh
  • tests/t015_dotmcshrc_ls_colors.sh
  • tests/t017_cd_minus_n.sh
  • tests/t018_syntax_highlight.sh
  • tests/t019_git_prompt.sh
  • tests/t020_predict.sh
  • tests/t021_onintr.sh
  • tests/t022_function_args.sh
  • tests/t023_filetest_ops.sh
  • tests/t024_variable_modifiers.sh
  • tests/t025_switch.sh
  • tests/t100_run_validation.sh
  • tests/t101_run_basic.sh
💤 Files with no reviewable changes (3)
  • strncpy_analysis.md
  • tc.prompt.c.orig
  • fix_truncation.patch

Comment thread sh.cworkflow.c
Comment on lines +807 to +826
static int
cw_hash_file_set(const cw_str_list_t *files, uint8_t out[32])
{
cw_sha256_ctx_t ctx;
uint8_t file_hash[32];
size_t i;
char nul = '\0';

cw_sha256_init(&ctx);
for (i = 0; i < files->len; i++) {
if (cw_sha256_file(files->items[i], file_hash) != 0)
return -1;
cw_sha256_update(&ctx, (const uint8_t *)files->items[i],
strlen(files->items[i]));
cw_sha256_update(&ctx, (const uint8_t *)&nul, 1);
cw_sha256_update(&ctx, file_hash, sizeof(file_hash));
}
cw_sha256_final(&ctx, out);
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Whole-project hash baked into every object's cache key causes full rebuilds on any single-file change for directory targets.

project_hash (an aggregate SHA-256 over every discovered .c/.h file — line 1135) is mixed into every individual object's cache key in cw_compute_object_path (lines 858-859). Since any file edit anywhere in the tree changes project_hash, it invalidates every object's cache key, not just the unit(s) that actually depend on the changed file — forcing a full recompile of the whole project on any single-file edit for run <directory> targets. This is safe (no wrong results) but defeats the incremental-caching value the project's own docs describe (e.g. TEST-PLAN.md I111 "Only the changed file recompiles").

This is presumably an accepted stopgap in the absence of real #include dependency discovery (CW-002/M2), but it's worth calling out explicitly as a known limitation (e.g., in RISKS.md or PLAN.md's Phase-3/known-limitations list) so it isn't mistaken for working incremental caching once directory run support is exercised on non-trivial projects.

Also applies to: 842-865, 1135-1135

🧰 Tools
🪛 Clang (14.0.6)

[warning] 812-812: variable 'i' is not initialized

(cppcoreguidelines-init-variables)


[warning] 812-812: variable name 'i' is too short, expected at least 3 characters

(readability-identifier-length)


[warning] 817-817: statement should be inside braces

(readability-braces-around-statements)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sh.cworkflow.c` around lines 807 - 826, Document the known limitation that
project_hash, used by cw_compute_object_path, is incorporated into every object
cache key for directory targets, causing all objects to rebuild when any
discovered source or header changes. Add this limitation to the appropriate
RISKS.md or PLAN.md Phase-3/known-limitations section, noting that precise
incremental caching awaits include-dependency discovery; do not change the
hashing behavior.

Comment thread sh.cworkflow.c
Comment on lines +828 to +840
static int
cw_hash_toolchain(const char *cc_path, uint8_t out[32])
{
cw_sha256_ctx_t ctx;

if (cw_sha256_file(cc_path, out) == 0)
return 0;

cw_sha256_init(&ctx);
cw_sha256_update(&ctx, (const uint8_t *)cc_path, strlen(cc_path));
cw_sha256_final(&ctx, out);
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Hashing the entire compiler binary on every run invocation conflicts with the documented <50ms warm-cache-hit target.

cw_hash_toolchain() SHA-256-hashes the whole resolved compiler binary (clang/cc, often 50-150+ MB) unconditionally in cw_run_prepare_state, on every single run call — including a fully cached, nothing-to-compile invocation. TEST-PLAN.md §6.1 and PIPELINES.md Stage 2 both call out a <50ms warm-cache-hit budget; hashing a large binary end-to-end will typically blow well past that on every invocation, defeating the "fast cache check" design goal.

Consider caching the toolchain identity hash itself (e.g., keyed by resolved path + mtime + size in a small sidecar/index, recomputed only when those change) instead of re-hashing the full binary content every time.

Also applies to: 1110-1114

🧰 Tools
🪛 Clang (14.0.6)

[warning] 833-833: statement should be inside braces

(readability-braces-around-statements)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sh.cworkflow.c` around lines 828 - 840, Update cw_hash_toolchain and its
callers so repeated run invocations do not SHA-256-hash the entire compiler
binary; cache the toolchain identity keyed by resolved compiler path,
modification time, and size, and reuse it when those attributes are unchanged.
Recompute and refresh the cached hash only when the key changes, while
preserving the existing fallback behavior for hashing the path when file hashing
fails.

Comment thread sh.cworkflow.c
Comment on lines +37 to +48
if command -v gcc >/dev/null 2>&1; then
out=$("$MCSH" -f -c "set mcsh_cc = gcc; run $tmpdir/hello.c" 2>&1)
status=$?
if [ $status -eq 0 ]; then
printf 'expected non-zero status for non-clang mcsh_cc, got 0\n'
exit 1
fi
printf '%s\n' "$out" | grep -F "not a clang-family compiler" >/dev/null 2>&1 || {
printf "expected clang-family enforcement message, got: %s\n" "$out"
exit 1
}
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Make the non-clang override test unconditional.

Clang-only environments skip the sole $mcsh_cc enforcement test. Use a guaranteed non-clang executable such as /bin/sh, or explicitly fail if no suitable candidate exists.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/t100_run_validation.sh` around lines 37 - 48, Update the non-clang
compiler enforcement test around the gcc availability check to run
unconditionally using a guaranteed non-clang executable such as /bin/sh. Remove
the environment-dependent skip, while preserving the non-zero status assertion
and “not a clang-family compiler” message validation; if no suitable executable
can be selected, fail the test explicitly.

Comment thread tests/t101_run_basic.sh
Comment on lines +66 to +71
out=$("$MCSH" -f -c "set mcsh_cache_dir = $tmpdir/cache; run $tmpdir/hello.c" 2>&1)
status=$?
if [ $status -ne 0 ] || [ "$out" != "hello" ]; then
printf "expected cached hello/0, got status=%d output=%s\n" "$status" "$out"
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert cache reuse rather than repeated correctness.

The second invocation only verifies that the program still prints hello; recompiling and relinking every time would also pass. Assert that the second -v run emits no compile/link action, or otherwise verify cached artifacts are reused.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/t101_run_basic.sh` around lines 66 - 71, Strengthen the second
invocation in the cache reuse test around the mcsh command so it runs with
verbose output and asserts that no compile or link actions are emitted, while
preserving the existing status and “hello” output checks. Verify cached
artifacts are reused rather than merely confirming repeated execution
correctness.

Comment thread tests/t101_run_basic.sh
Comment on lines +80 to +85
out=$("$MCSH" -f -c "set mcsh_cache_dir = $tmpdir/cache; run $tmpdir/args.c one two" 2>&1)
status=$?
if [ $status -ne 0 ] || [ "$out" != "one two" ]; then
printf "expected streamlined arg passthrough, got status=%d output=%s\n" "$status" "$out"
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cover the -- separator path.

This checks direct argument forwarding, but not the dedicated -- parsing branch promised by the run interface. Add an equivalent run … -- one two assertion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/t101_run_basic.sh` around lines 80 - 85, Extend the run
argument-passthrough test around the existing status/output assertion to also
invoke run with the `--` separator before `one two`. Assert that this dedicated
parsing path returns success and produces exactly `one two`, using the same
failure reporting and exit behavior as the existing direct-forwarding check.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
sh.init.c (1)

43-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Restore the unrelated platform builtin registrations.

This change removes guarded builtins such as att, bye, getspath, getxvers, migrate, and universe. Adding run should not silently remove supported commands on their respective platforms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sh.init.c` around lines 43 - 153, Restore the platform-guarded builtin
registrations removed from the builtin table, including att, bye, getspath,
getxvers, migrate, and universe, using their original conditional compilation
guards and handlers. Keep the new run entry and existing alphabetical ordering
intact, ensuring each platform retains its supported commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/c-commands-spec.md`:
- Around line 1-3: Update the documentation to reflect delivered P1 support: in
docs/c-commands-spec.md (lines 1-3) describe run as implemented while keeping
compile/build planned; in docs/c-workflow-analysis.md (lines 1-3) distinguish
completed P1 behavior from future phases; in docs/compiler/ARCHITECTURE.md
(lines 1-4) update status and remaining integration work; in
docs/compiler/CACHE-DESIGN.md (lines 1-4) distinguish the implemented cache
layout from future index/metadata features; in docs/compiler/CLI-SPEC.md (lines
1-17) mark compile/build syntax as planned and document only supported run
options; in docs/compiler/DATA-MODEL.md (lines 1-4) identify the model as a
future target beyond the initial implementation; in
docs/compiler/DEPENDENCY-DISCOVERY.md (lines 1-4) describe dependency scanning
as future work; and in docs/compiler/INDEX.md (lines 1-5) remove the claim that
no runtime is implemented.

In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 55-77: Update the cache-key design in the documented key
construction and normalization section to preserve order and multiplicity:
replace XOR of object keys with a length-delimited ordered sequence hash, and
hash the effective linker/compiler argv in its original order. Remove sorting of
order-sensitive linker flags and canonicalized -I paths, while retaining only
normalization that does not change argument order or meaning.

In `@docs/compiler/DECISIONS.md`:
- Around line 223-227: Update the ADR-006 execution-model documentation around
the `run` behavior to match the implementation: `cw_run_execute_stage()` runs
the stage in a child, returns its status to `dorun()`, and `dorun()` updates
shell status and performs cleanup. Remove or correct the recommendation to use
direct `execvp` so the documented model preserves fork-and-wait behavior and
returns control to the pipeline.

In `@docs/compiler/DEPENDENCY-DISCOVERY.md`:
- Around line 145-158: Revise the “mtime-gated hashing” section to remove the
claim that matching mtime and size safely validates content_hash. Require
hashing of cache-key inputs for correctness, or explicitly document the
optimization as best-effort and add a verification path before accepting the
cached hash.

In `@docs/compiler/MASTER-PLAN.md`:
- Around line 3-4: Synchronize the compiler planning documents with the
implemented PR: in docs/compiler/MASTER-PLAN.md at lines 3-4, replace the
no-runtime status and update the corresponding P1 checklist; in
docs/compiler/MILESTONES.md at lines 3-4, replace the planning-only status and
accurately mark the shipped M1 work.

In `@docs/compiler/PIPELINES.md`:
- Around line 291-303: Update the Stage 4 “Execute” section to document the
shipped fork+exec+waitpid behavior in cw_execute_binary/cw_run_execute_stage
rather than direct execvp replacement. Describe the forked child executing the
binary, the parent waiting and propagating its status, and retain the
exec-failure exit code 127; remove claims that run replaces the mcsh process
directly or requires avoiding an intermediate shell process.

In `@ISSUES.md`:
- Around line 9-21: Refresh the compiler planning documentation to match the
shipped dorun() implementation: in ISSUES.md lines 9-21, move CW-001 from Open
to Resolved and note its directory-target support; in PLAN.md lines 48-57, mark
M1/run as implemented and note directory/project support moved forward from M3;
in docs/compiler/PIPELINES.md lines 1-6, remove or revise the
planning-only/no-implementation status for the run pipeline; and in
docs/compiler/TODO.md lines 28-83, check off the completed P1 items for
sh.cworkflow.c, dorun(), sh.decls.h/sh.init.c wiring, and the run tests.

In `@sh.cworkflow.c`:
- Around line 959-978: Update cw_execute_binary and the cw_run_execute_stage
handling to distinguish an execv failure from a user program that exits 127,
mirroring the existing compile/build-stage status == 127 heuristic. Emit the
required “run: exec failed: <strerror>” diagnostic using the captured execution
error, preserve exit code 127, and keep the existing fork-failure handling
unchanged.
- Around line 807-826: Document the known limitation that project_hash, used by
cw_compute_object_path, is incorporated into every object cache key for
directory targets, causing all objects to rebuild when any discovered source or
header changes. Add this limitation to the appropriate RISKS.md or PLAN.md
Phase-3/known-limitations section, noting that precise incremental caching
awaits include-dependency discovery; do not change the hashing behavior.
- Around line 828-840: Update cw_hash_toolchain and its callers so repeated run
invocations do not SHA-256-hash the entire compiler binary; cache the toolchain
identity keyed by resolved compiler path, modification time, and size, and reuse
it when those attributes are unchanged. Recompute and refresh the cached hash
only when the key changes, while preserving the existing fallback behavior for
hashing the path when file hashing fails.

In `@tcsh.man.in`:
- Around line 8909-8913: Correct the category-count statement in the
compile-time default-colors documentation near the listed token categories:
either update the enumeration to include all 12 categories or remove the
unsupported numeric count, while keeping the existing category descriptions
accurate.
- Around line 8483-8496: Update the %G operation-state documentation to match
the detection logic in git_get_info: remove REBASE_HEAD and
rebase-merge/head-name, and document rebase-apply/rebasing, rebase-apply (AM),
REVERT_HEAD, and BISECT_LOG with their corresponding states.

In `@tests/t023_filetest_ops.sh`:
- Around line 4-12: The test fixture around the C-shell -z check currently uses
an empty tmpfile while expecting ! -z to print z_ok. Update the fixture or
expectation in the test command so it matches correct -z semantics, preserving
the existing checks for -d, -f, and -e.

In `@tests/t100_run_validation.sh`:
- Around line 37-48: Update the non-clang compiler enforcement test around the
gcc availability check to run unconditionally using a guaranteed non-clang
executable such as /bin/sh. Remove the environment-dependent skip, while
preserving the non-zero status assertion and “not a clang-family compiler”
message validation; if no suitable executable can be selected, fail the test
explicitly.

In `@tests/t101_run_basic.sh`:
- Around line 66-71: Strengthen the second invocation in the cache reuse test
around the mcsh command so it runs with verbose output and asserts that no
compile or link actions are emitted, while preserving the existing status and
“hello” output checks. Verify cached artifacts are reused rather than merely
confirming repeated execution correctness.
- Around line 80-85: Extend the run argument-passthrough test around the
existing status/output assertion to also invoke run with the `--` separator
before `one two`. Assert that this dedicated parsing path returns success and
produces exactly `one two`, using the same failure reporting and exit behavior
as the existing direct-forwarding check.

---

Outside diff comments:
In `@sh.init.c`:
- Around line 43-153: Restore the platform-guarded builtin registrations removed
from the builtin table, including att, bye, getspath, getxvers, migrate, and
universe, using their original conditional compilation guards and handlers. Keep
the new run entry and existing alphabetical ordering intact, ensuring each
platform retains its supported commands.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 64a83388-2554-4b51-91d0-ef216852d958

📥 Commits

Reviewing files that changed from the base of the PR and between ab1fcea and 30cb77a.

📒 Files selected for processing (47)
  • ISSUES.md
  • Makefile.in
  • PLAN.md
  • README.md
  • SYNTAX_AND_SCRIPTING.md
  • docs/c-commands-spec.md
  • docs/c-workflow-analysis.md
  • docs/compiler/ARCHITECTURE.md
  • docs/compiler/CACHE-DESIGN.md
  • docs/compiler/CLI-SPEC.md
  • docs/compiler/DATA-MODEL.md
  • docs/compiler/DECISIONS.md
  • docs/compiler/DEPENDENCY-DISCOVERY.md
  • docs/compiler/INDEX.md
  • docs/compiler/MASTER-PLAN.md
  • docs/compiler/MILESTONES.md
  • docs/compiler/PIPELINES.md
  • docs/compiler/RISKS.md
  • docs/compiler/TEST-PLAN.md
  • docs/compiler/TODO.md
  • fix_truncation.patch
  • sh.c
  • sh.cworkflow.c
  • sh.decls.h
  • sh.dir.c
  • sh.func.c
  • sh.init.c
  • sh.sem.c
  • strncpy_analysis.md
  • tc.prompt.c
  • tc.prompt.c.orig
  • tcsh.man.in
  • test2
  • test3
  • tests/t005_cd_stack.sh
  • tests/t015_dotmcshrc_ls_colors.sh
  • tests/t017_cd_minus_n.sh
  • tests/t018_syntax_highlight.sh
  • tests/t019_git_prompt.sh
  • tests/t020_predict.sh
  • tests/t021_onintr.sh
  • tests/t022_function_args.sh
  • tests/t023_filetest_ops.sh
  • tests/t024_variable_modifiers.sh
  • tests/t025_switch.sh
  • tests/t100_run_validation.sh
  • tests/t101_run_basic.sh
💤 Files with no reviewable changes (3)
  • strncpy_analysis.md
  • tc.prompt.c.orig
  • fix_truncation.patch
🛑 Comments failed to post (10)
docs/c-commands-spec.md (1)

1-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Update the documentation from “design-only” to the delivered P1 scope.

The PR registers and implements run; these documents currently tell users and contributors that no runtime exists, while CLI documentation also implies that unimplemented compile and build are available.

  • docs/c-commands-spec.md#L1-L3: describe run as implemented and retain compile/build as planned.
  • docs/c-workflow-analysis.md#L1-L3: distinguish completed P1 behavior from future phases.
  • docs/compiler/ARCHITECTURE.md#L1-L4: update the status and remaining “future/not implemented” integration language.
  • docs/compiler/CACHE-DESIGN.md#L1-L4: distinguish the implemented cache layout from future index/metadata features.
  • docs/compiler/CLI-SPEC.md#L1-L17: mark compile/build syntax as planned; document only supported run options.
  • docs/compiler/DATA-MODEL.md#L1-L4: state that this is the target model beyond the initial implementation.
  • docs/compiler/DEPENDENCY-DISCOVERY.md#L1-L4: identify dependency scanning as future work rather than claiming no workflow exists.
  • docs/compiler/INDEX.md#L1-L5: remove the claim that no runtime is implemented.
📍 Affects 8 files
  • docs/c-commands-spec.md#L1-L3 (this comment)
  • docs/c-workflow-analysis.md#L1-L3
  • docs/compiler/ARCHITECTURE.md#L1-L4
  • docs/compiler/CACHE-DESIGN.md#L1-L4
  • docs/compiler/CLI-SPEC.md#L1-L17
  • docs/compiler/DATA-MODEL.md#L1-L4
  • docs/compiler/DEPENDENCY-DISCOVERY.md#L1-L4
  • docs/compiler/INDEX.md#L1-L5
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/c-commands-spec.md` around lines 1 - 3, Update the documentation to
reflect delivered P1 support: in docs/c-commands-spec.md (lines 1-3) describe
run as implemented while keeping compile/build planned; in
docs/c-workflow-analysis.md (lines 1-3) distinguish completed P1 behavior from
future phases; in docs/compiler/ARCHITECTURE.md (lines 1-4) update status and
remaining integration work; in docs/compiler/CACHE-DESIGN.md (lines 1-4)
distinguish the implemented cache layout from future index/metadata features; in
docs/compiler/CLI-SPEC.md (lines 1-17) mark compile/build syntax as planned and
document only supported run options; in docs/compiler/DATA-MODEL.md (lines 1-4)
identify the model as a future target beyond the initial implementation; in
docs/compiler/DEPENDENCY-DISCOVERY.md (lines 1-4) describe dependency scanning
as future work; and in docs/compiler/INDEX.md (lines 1-5) remove the claim that
no runtime is implemented.
docs/compiler/CACHE-DESIGN.md (1)

55-77: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve ordered inputs in cache keys.

Linker flag order and -I search order affect behavior; XOR also loses multiplicity. Hash a length-delimited, ordered sequence of object keys and effective linker/compiler argv instead of XORing objects or sorting order-sensitive flags. Otherwise distinct builds can reuse the wrong artifact.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/CACHE-DESIGN.md` around lines 55 - 77, Update the cache-key
design in the documented key construction and normalization section to preserve
order and multiplicity: replace XOR of object keys with a length-delimited
ordered sequence hash, and hash the effective linker/compiler argv in its
original order. Remove sorting of order-sensitive linker flags and canonicalized
-I paths, while retaining only normalization that does not change argument order
or meaning.
docs/compiler/DECISIONS.md (1)

223-227: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align ADR-006 with the implemented execution model.

cw_run_execute_stage() returns a child status to dorun(), which then updates shell status and cleans up. Document the fork-and-wait behavior; direct execvp here would not return to that pipeline.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/DECISIONS.md` around lines 223 - 227, Update the ADR-006
execution-model documentation around the `run` behavior to match the
implementation: `cw_run_execute_stage()` runs the stage in a child, returns its
status to `dorun()`, and `dorun()` updates shell status and performs cleanup.
Remove or correct the recommendation to use direct `execvp` so the documented
model preserves fork-and-wait behavior and returns control to the pipeline.
docs/compiler/DEPENDENCY-DISCOVERY.md (1)

145-158: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Do not treat mtime/size gating as cache-safe.

A header can change while retaining its size and timestamp, causing a stale cache hit. Always hash cache-key inputs for correctness, or explicitly define this as a best-effort optimization with a verification path.

🧰 Tools
🪛 markdownlint-cli2 (0.23.0)

[warning] 149-149: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/DEPENDENCY-DISCOVERY.md` around lines 145 - 158, Revise the
“mtime-gated hashing” section to remove the claim that matching mtime and size
safely validates content_hash. Require hashing of cache-key inputs for
correctness, or explicitly document the optimization as best-effort and add a
verification path before accepting the cached hash.
docs/compiler/MASTER-PLAN.md (1)

3-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Synchronize compiler planning documents with the implemented PR.

  • docs/compiler/MASTER-PLAN.md#L3-L4: update the “no runtime implemented” status and corresponding P1 checklist.
  • docs/compiler/MILESTONES.md#L3-L4: update the planning-only status and mark the shipped M1 work accurately.
📍 Affects 2 files
  • docs/compiler/MASTER-PLAN.md#L3-L4 (this comment)
  • docs/compiler/MILESTONES.md#L3-L4
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/MASTER-PLAN.md` around lines 3 - 4, Synchronize the compiler
planning documents with the implemented PR: in docs/compiler/MASTER-PLAN.md at
lines 3-4, replace the no-runtime status and update the corresponding P1
checklist; in docs/compiler/MILESTONES.md at lines 3-4, replace the
planning-only status and accurately mark the shipped M1 work.
docs/compiler/PIPELINES.md (1)

291-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Documented "preferred" execute path doesn't match the shipped implementation.

This doc states the "Preferred path" is execvp(binary_path, program_args) replacing the mcsh process directly. However, sh.cworkflow.c's cw_execute_binary/cw_run_execute_stage always fork()s and then waitpid()s in the parent — matching RISKS.md R-T02's Contingency ("Always fork before exec in dorun"), not this section's stated preference. Worth updating this doc to describe the actually-shipped fork+exec+waitpid design as the implemented approach, since a future reader could otherwise "fix" the code to match this doc and reintroduce the exact hazard R-T02 warns about (replacing the shell process itself in a non-forked builtin context).

🧰 Tools
🪛 LanguageTool

[style] ~296-~296: Consider an alternative for the overused word “exactly”.
Context: ... the full environment. - Exit code is exactly the binary's exit code. - This is the...

(EXACTLY_PRECISELY)


[grammar] ~300-~300: Ensure spelling is correct
Context: ...mple foreground run invocation, the builtin must arrange exec without an extra in...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/PIPELINES.md` around lines 291 - 303, Update the Stage 4
“Execute” section to document the shipped fork+exec+waitpid behavior in
cw_execute_binary/cw_run_execute_stage rather than direct execvp replacement.
Describe the forked child executing the binary, the parent waiting and
propagating its status, and retain the exec-failure exit code 127; remove claims
that run replaces the mcsh process directly or requires avoiding an intermediate
shell process.
ISSUES.md (1)

9-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Planning docs weren't updated to reflect the run implementation shipped in this same PR. Four documents still describe the P1 run file.c milestone as not-yet-implemented / open, but sh.cworkflow.c + sh.decls.h in this PR already ship a working dorun() builtin (file and directory targets, --clean, -v, -- passthrough), wired into the builtin table. Shared root cause: the compiler-planning doc set wasn't refreshed alongside the implementation commit.

  • ISSUES.md#L9-L21: move CW-001 from "Open" to a "Resolved" entry (it already exceeds the stated MVP scope by also supporting directory targets).
  • PLAN.md#L48-L57: update "Immediate next milestone: M1" and the "No runtime implementation yet" status line to reflect that run is implemented (and note directory/project support was pulled forward from M3).
  • docs/compiler/PIPELINES.md#L1-L6: update the "Status: planning / documentation only. No implementation exists yet." banner — at minimum for the run pipeline section, since it's now implemented.
  • docs/compiler/TODO.md#L28-L83: check off the P1 boxes that are demonstrably done (sh.cworkflow.c created, dorun() implemented, sh.decls.h/sh.init.c wiring done, tests/t100_run_validation.sh/tests/t101_run_basic.sh written).
📍 Affects 4 files
  • ISSUES.md#L9-L21 (this comment)
  • PLAN.md#L48-L57
  • docs/compiler/PIPELINES.md#L1-L6
  • docs/compiler/TODO.md#L28-L83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ISSUES.md` around lines 9 - 21, Refresh the compiler planning documentation
to match the shipped dorun() implementation: in ISSUES.md lines 9-21, move
CW-001 from Open to Resolved and note its directory-target support; in PLAN.md
lines 48-57, mark M1/run as implemented and note directory/project support moved
forward from M3; in docs/compiler/PIPELINES.md lines 1-6, remove or revise the
planning-only/no-implementation status for the run pipeline; and in
docs/compiler/TODO.md lines 28-83, check off the completed P1 items for
sh.cworkflow.c, dorun(), sh.decls.h/sh.init.c wiring, and the run tests.
tcsh.man.in (2)

8483-8496: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align %G operation-state documentation with git_get_info.

REBASE_HEAD is not used to produce REBASING; tc.prompt.c instead detects rebase-apply/rebasing, rebase-apply (AM), REVERT_HEAD, and BISECT_LOG. Document the actual paths and states so prompt users are not misled.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tcsh.man.in` around lines 8483 - 8496, Update the %G operation-state
documentation to match the detection logic in git_get_info: remove REBASE_HEAD
and rebase-merge/head-name, and document rebase-apply/rebasing, rebase-apply
(AM), REVERT_HEAD, and BISECT_LOG with their corresponding states.

8909-8913: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the syntax-category count.

The text says 12 categories but lists only 10. Either enumerate the missing categories or remove the count.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tcsh.man.in` around lines 8909 - 8913, Correct the category-count statement
in the compile-time default-colors documentation near the listed token
categories: either update the enumeration to include all 12 categories or remove
the unsupported numeric count, while keeping the existing category descriptions
accurate.
tests/t023_filetest_ops.sh (1)

4-12: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate file =="
if [ -f tests/t023_filetest_ops.sh ]; then
  wc -l tests/t023_filetest_ops.sh
  cat -n tests/t023_filetest_ops.sh
else
  echo "missing tests/t023_filetest_ops.sh"
  fd -a 't023_filetest_ops\.sh' . || true
fi

echo
echo "== mktemp files in repo =="
rg -n '\$\(mktemp\)|! -z|z_ok|-z' tests || true

echo
echo "== shell availability and semantics probe =="
for shell in csh tcsh sh bash zsh; do
  if command -v "$shell" >/dev/null 2>&1; then
    echo "--- $shell ---"
  else
    echo "--- $shell: missing ---"
    continue
  fi

  shell -c 'tmpfile="$(mktemp)"; trap "rm -f \"$tmpfile\"" EXIT; printf "created %s\\n" "$tmpfile"; if (-z "$tmpfile") echo csh_zero_pred; if (! -z "$tmpfile") echo csh_neg_not_zero_pred' || (false; sed -n '1,80p' /tmp/test_log 2>/dev/null || true)
done

echo
echo "== csh zero predicate direct probe =="
tmpfile="$(mktemp)" || exit 77
trap 'rm -f "$tmpfile"' EXIT
csh -f -c 'if (-z "$tmpfile") echo zero; if (! -z "$tmpfile") echo not_zero' "$tmpfile" || true
printf 'x' > "$tmpfile" || exit 77
csh -f -c 'if (-z "$tmpfile") echo zero; if (! -z "$tmpfile") echo not_zero' "$tmpfile" || true

Repository: orpheus497/mcsh

Length of output: 1787


🌐 Web query:

tcsh -z file inquiry operator zero size documentation

💡 Result:

In tcsh, the file inquiry operator for checking if a file has zero size is -z [1][2]. When used in an expression, such as an if statement, -z returns true if the specified file exists and has a size of zero bytes [1][3][2]. Conversely, the -s operator is used to test if a file has a non-zero size (i.e., it exists and is larger than zero bytes) [1][2]. Example usage: if ( -z filename) then echo "The file is empty." endif Additionally, if you need to retrieve the actual size of a file in bytes, you can use the -Z operator [2][4]. Unlike the boolean -z operator, -Z returns the size of the file as an integer [2][4]. These file inquiry operators can also be evaluated using the built-in filetest command [2][5][6].

Citations:


Fix the -z test fixture or expectation.

tmpfile is empty, so C-shell -z is true; ! -z "$tmpfile" will not print z_ok, causing this test to fail against correct -z semantics.

Suggested fixture fix
 tmpfile=$(mktemp) || exit 77
 trap 'rm -rf "$tmpdir" "$tmpfile"' EXIT
+printf 'x' > "$tmpfile" || exit 77
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

tmpdir=$(mktemp -d) || exit 77
tmpfile=$(mktemp) || exit 77
trap 'rm -rf "$tmpdir" "$tmpfile"' EXIT
printf 'x' > "$tmpfile" || exit 77

out=$("$MCSH" -f -c "
if (-d \"$tmpdir\") echo d_ok
if (-f \"$tmpfile\") echo f_ok
if (-e \"$tmpfile\") echo e_ok
if (! -z \"$tmpfile\") echo z_ok
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/t023_filetest_ops.sh` around lines 4 - 12, The test fixture around the
C-shell -z check currently uses an empty tmpfile while expecting ! -z to print
z_ok. Update the fixture or expectation in the test command so it matches
correct -z semantics, preserving the existing checks for -d, -f, and -e.

@orpheus497

Copy link
Copy Markdown
Owner Author

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @docs/c-commands-spec.md:

  • Around line 1-3: Update the documentation to reflect delivered P1 support: in
    docs/c-commands-spec.md (lines 1-3) describe run as implemented while keeping
    compile/build planned; in docs/c-workflow-analysis.md (lines 1-3) distinguish
    completed P1 behavior from future phases; in docs/compiler/ARCHITECTURE.md
    (lines 1-4) update status and remaining integration work; in
    docs/compiler/CACHE-DESIGN.md (lines 1-4) distinguish the implemented cache
    layout from future index/metadata features; in docs/compiler/CLI-SPEC.md (lines
    1-17) mark compile/build syntax as planned and document only supported run
    options; in docs/compiler/DATA-MODEL.md (lines 1-4) identify the model as a
    future target beyond the initial implementation; in
    docs/compiler/DEPENDENCY-DISCOVERY.md (lines 1-4) describe dependency scanning
    as future work; and in docs/compiler/INDEX.md (lines 1-5) remove the claim that
    no runtime is implemented.

In @docs/compiler/CACHE-DESIGN.md:

  • Around line 55-77: Update the cache-key design in the documented key
    construction and normalization section to preserve order and multiplicity:
    replace XOR of object keys with a length-delimited ordered sequence hash, and
    hash the effective linker/compiler argv in its original order. Remove sorting of
    order-sensitive linker flags and canonicalized -I paths, while retaining only
    normalization that does not change argument order or meaning.

In @docs/compiler/DECISIONS.md:

  • Around line 223-227: Update the ADR-006 execution-model documentation around
    the run behavior to match the implementation: cw_run_execute_stage() runs
    the stage in a child, returns its status to dorun(), and dorun() updates
    shell status and performs cleanup. Remove or correct the recommendation to use
    direct execvp so the documented model preserves fork-and-wait behavior and
    returns control to the pipeline.

In @docs/compiler/DEPENDENCY-DISCOVERY.md:

  • Around line 145-158: Revise the “mtime-gated hashing” section to remove the
    claim that matching mtime and size safely validates content_hash. Require
    hashing of cache-key inputs for correctness, or explicitly document the
    optimization as best-effort and add a verification path before accepting the
    cached hash.

In @docs/compiler/MASTER-PLAN.md:

  • Around line 3-4: Synchronize the compiler planning documents with the
    implemented PR: in docs/compiler/MASTER-PLAN.md at lines 3-4, replace the
    no-runtime status and update the corresponding P1 checklist; in
    docs/compiler/MILESTONES.md at lines 3-4, replace the planning-only status and
    accurately mark the shipped M1 work.

In @docs/compiler/PIPELINES.md:

  • Around line 291-303: Update the Stage 4 “Execute” section to document the
    shipped fork+exec+waitpid behavior in cw_execute_binary/cw_run_execute_stage
    rather than direct execvp replacement. Describe the forked child executing the
    binary, the parent waiting and propagating its status, and retain the
    exec-failure exit code 127; remove claims that run replaces the mcsh process
    directly or requires avoiding an intermediate shell process.

In @ISSUES.md:

  • Around line 9-21: Refresh the compiler planning documentation to match the
    shipped dorun() implementation: in ISSUES.md lines 9-21, move CW-001 from Open
    to Resolved and note its directory-target support; in PLAN.md lines 48-57, mark
    M1/run as implemented and note directory/project support moved forward from M3;
    in docs/compiler/PIPELINES.md lines 1-6, remove or revise the
    planning-only/no-implementation status for the run pipeline; and in
    docs/compiler/TODO.md lines 28-83, check off the completed P1 items for
    sh.cworkflow.c, dorun(), sh.decls.h/sh.init.c wiring, and the run tests.

In @sh.cworkflow.c:

  • Around line 959-978: Update cw_execute_binary and the cw_run_execute_stage
    handling to distinguish an execv failure from a user program that exits 127,
    mirroring the existing compile/build-stage status == 127 heuristic. Emit the
    required “run: exec failed: ” diagnostic using the captured execution
    error, preserve exit code 127, and keep the existing fork-failure handling
    unchanged.
  • Around line 807-826: Document the known limitation that project_hash, used by
    cw_compute_object_path, is incorporated into every object cache key for
    directory targets, causing all objects to rebuild when any discovered source or
    header changes. Add this limitation to the appropriate RISKS.md or PLAN.md
    Phase-3/known-limitations section, noting that precise incremental caching
    awaits include-dependency discovery; do not change the hashing behavior.
  • Around line 828-840: Update cw_hash_toolchain and its callers so repeated run
    invocations do not SHA-256-hash the entire compiler binary; cache the toolchain
    identity keyed by resolved compiler path, modification time, and size, and reuse
    it when those attributes are unchanged. Recompute and refresh the cached hash
    only when the key changes, while preserving the existing fallback behavior for
    hashing the path when file hashing fails.

In @tcsh.man.in:

  • Around line 8909-8913: Correct the category-count statement in the
    compile-time default-colors documentation near the listed token categories:
    either update the enumeration to include all 12 categories or remove the
    unsupported numeric count, while keeping the existing category descriptions
    accurate.
  • Around line 8483-8496: Update the %G operation-state documentation to match
    the detection logic in git_get_info: remove REBASE_HEAD and
    rebase-merge/head-name, and document rebase-apply/rebasing, rebase-apply (AM),
    REVERT_HEAD, and BISECT_LOG with their corresponding states.

In @tests/t023_filetest_ops.sh:

  • Around line 4-12: The test fixture around the C-shell -z check currently uses
    an empty tmpfile while expecting ! -z to print z_ok. Update the fixture or
    expectation in the test command so it matches correct -z semantics, preserving
    the existing checks for -d, -f, and -e.

In @tests/t100_run_validation.sh:

  • Around line 37-48: Update the non-clang compiler enforcement test around the
    gcc availability check to run unconditionally using a guaranteed non-clang
    executable such as /bin/sh. Remove the environment-dependent skip, while
    preserving the non-zero status assertion and “not a clang-family compiler”
    message validation; if no suitable executable can be selected, fail the test
    explicitly.

In @tests/t101_run_basic.sh:

  • Around line 66-71: Strengthen the second invocation in the cache reuse test
    around the mcsh command so it runs with verbose output and asserts that no
    compile or link actions are emitted, while preserving the existing status and
    “hello” output checks. Verify cached artifacts are reused rather than merely
    confirming repeated execution correctness.
  • Around line 80-85: Extend the run argument-passthrough test around the
    existing status/output assertion to also invoke run with the -- separator
    before one two. Assert that this dedicated parsing path returns success and
    produces exactly one two, using the same failure reporting and exit behavior
    as the existing direct-forwarding check.

Outside diff comments:
In @sh.init.c:

  • Around line 43-153: Restore the platform-guarded builtin registrations removed
    from the builtin table, including att, bye, getspath, getxvers, migrate, and
    universe, using their original conditional compilation guards and handlers. Keep
    the new run entry and existing alphabetical ordering intact, ensuring each
    platform retains its supported commands.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @docs/c-commands-spec.md:

  • Around line 1-3: Update the documentation to reflect delivered P1 support: in
    docs/c-commands-spec.md (lines 1-3) describe run as implemented while keeping
    compile/build planned; in docs/c-workflow-analysis.md (lines 1-3) distinguish
    completed P1 behavior from future phases; in docs/compiler/ARCHITECTURE.md
    (lines 1-4) update status and remaining integration work; in
    docs/compiler/CACHE-DESIGN.md (lines 1-4) distinguish the implemented cache
    layout from future index/metadata features; in docs/compiler/CLI-SPEC.md (lines
    1-17) mark compile/build syntax as planned and document only supported run
    options; in docs/compiler/DATA-MODEL.md (lines 1-4) identify the model as a
    future target beyond the initial implementation; in
    docs/compiler/DEPENDENCY-DISCOVERY.md (lines 1-4) describe dependency scanning
    as future work; and in docs/compiler/INDEX.md (lines 1-5) remove the claim that
    no runtime is implemented.

In @docs/compiler/CACHE-DESIGN.md:

  • Around line 55-77: Update the cache-key design in the documented key
    construction and normalization section to preserve order and multiplicity:
    replace XOR of object keys with a length-delimited ordered sequence hash, and
    hash the effective linker/compiler argv in its original order. Remove sorting of
    order-sensitive linker flags and canonicalized -I paths, while retaining only
    normalization that does not change argument order or meaning.

In @docs/compiler/DECISIONS.md:

  • Around line 223-227: Update the ADR-006 execution-model documentation around
    the run behavior to match the implementation: cw_run_execute_stage() runs
    the stage in a child, returns its status to dorun(), and dorun() updates
    shell status and performs cleanup. Remove or correct the recommendation to use
    direct execvp so the documented model preserves fork-and-wait behavior and
    returns control to the pipeline.

In @docs/compiler/DEPENDENCY-DISCOVERY.md:

  • Around line 145-158: Revise the “mtime-gated hashing” section to remove the
    claim that matching mtime and size safely validates content_hash. Require
    hashing of cache-key inputs for correctness, or explicitly document the
    optimization as best-effort and add a verification path before accepting the
    cached hash.

In @docs/compiler/MASTER-PLAN.md:

  • Around line 3-4: Synchronize the compiler planning documents with the
    implemented PR: in docs/compiler/MASTER-PLAN.md at lines 3-4, replace the
    no-runtime status and update the corresponding P1 checklist; in
    docs/compiler/MILESTONES.md at lines 3-4, replace the planning-only status and
    accurately mark the shipped M1 work.

In @docs/compiler/PIPELINES.md:

  • Around line 291-303: Update the Stage 4 “Execute” section to document the
    shipped fork+exec+waitpid behavior in cw_execute_binary/cw_run_execute_stage
    rather than direct execvp replacement. Describe the forked child executing the
    binary, the parent waiting and propagating its status, and retain the
    exec-failure exit code 127; remove claims that run replaces the mcsh process
    directly or requires avoiding an intermediate shell process.

In @ISSUES.md:

  • Around line 9-21: Refresh the compiler planning documentation to match the
    shipped dorun() implementation: in ISSUES.md lines 9-21, move CW-001 from Open
    to Resolved and note its directory-target support; in PLAN.md lines 48-57, mark
    M1/run as implemented and note directory/project support moved forward from M3;
    in docs/compiler/PIPELINES.md lines 1-6, remove or revise the
    planning-only/no-implementation status for the run pipeline; and in
    docs/compiler/TODO.md lines 28-83, check off the completed P1 items for
    sh.cworkflow.c, dorun(), sh.decls.h/sh.init.c wiring, and the run tests.

In @sh.cworkflow.c:

  • Around line 959-978: Update cw_execute_binary and the cw_run_execute_stage
    handling to distinguish an execv failure from a user program that exits 127,
    mirroring the existing compile/build-stage status == 127 heuristic. Emit the
    required “run: exec failed: ” diagnostic using the captured execution
    error, preserve exit code 127, and keep the existing fork-failure handling
    unchanged.
  • Around line 807-826: Document the known limitation that project_hash, used by
    cw_compute_object_path, is incorporated into every object cache key for
    directory targets, causing all objects to rebuild when any discovered source or
    header changes. Add this limitation to the appropriate RISKS.md or PLAN.md
    Phase-3/known-limitations section, noting that precise incremental caching
    awaits include-dependency discovery; do not change the hashing behavior.
  • Around line 828-840: Update cw_hash_toolchain and its callers so repeated run
    invocations do not SHA-256-hash the entire compiler binary; cache the toolchain
    identity keyed by resolved compiler path, modification time, and size, and reuse
    it when those attributes are unchanged. Recompute and refresh the cached hash
    only when the key changes, while preserving the existing fallback behavior for
    hashing the path when file hashing fails.

In @tcsh.man.in:

  • Around line 8909-8913: Correct the category-count statement in the
    compile-time default-colors documentation near the listed token categories:
    either update the enumeration to include all 12 categories or remove the
    unsupported numeric count, while keeping the existing category descriptions
    accurate.
  • Around line 8483-8496: Update the %G operation-state documentation to match
    the detection logic in git_get_info: remove REBASE_HEAD and
    rebase-merge/head-name, and document rebase-apply/rebasing, rebase-apply (AM),
    REVERT_HEAD, and BISECT_LOG with their corresponding states.

In @tests/t023_filetest_ops.sh:

  • Around line 4-12: The test fixture around the C-shell -z check currently uses
    an empty tmpfile while expecting ! -z to print z_ok. Update the fixture or
    expectation in the test command so it matches correct -z semantics, preserving
    the existing checks for -d, -f, and -e.

In @tests/t100_run_validation.sh:

  • Around line 37-48: Update the non-clang compiler enforcement test around the
    gcc availability check to run unconditionally using a guaranteed non-clang
    executable such as /bin/sh. Remove the environment-dependent skip, while
    preserving the non-zero status assertion and “not a clang-family compiler”
    message validation; if no suitable executable can be selected, fail the test
    explicitly.

In @tests/t101_run_basic.sh:

  • Around line 66-71: Strengthen the second invocation in the cache reuse test
    around the mcsh command so it runs with verbose output and asserts that no
    compile or link actions are emitted, while preserving the existing status and
    “hello” output checks. Verify cached artifacts are reused rather than merely
    confirming repeated execution correctness.
  • Around line 80-85: Extend the run argument-passthrough test around the
    existing status/output assertion to also invoke run with the -- separator
    before one two. Assert that this dedicated parsing path returns success and
    produces exactly one two, using the same failure reporting and exit behavior
    as the existing direct-forwarding check.

Outside diff comments:
In @sh.init.c:

  • Around line 43-153: Restore the platform-guarded builtin registrations removed
    from the builtin table, including att, bye, getspath, getxvers, migrate, and
    universe, using their original conditional compilation guards and handlers. Keep
    the new run entry and existing alphabetical ordering intact, ensuring each
    platform retains its supported commands.

Copilot AI commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

...

All still-valid findings addressed in commit docs/code: sync P1 status, fix exec-failure detection, correct cache-key design.

Code change (sh.cworkflow.c)
cw_execute_binary now uses a FD_CLOEXEC pipe to properly distinguish execv failure from a user program that legitimately exits 127. If exec fails the child writes its errno through the pipe before _exit; the parent reads it, restores errno, and returns -2. cw_run_execute_stage handles -1 (fork/pipe failure) with "run: fork failed: <strerror>" and -2 with "run: exec failed: <strerror>", both returning exit code 127. The child's real exit status passes through untouched in all other cases.

Documentation updates

  • Status headers in docs/c-commands-spec.md, docs/c-workflow-analysis.md, docs/compiler/ARCHITECTURE.md, CACHE-DESIGN.md, CLI-SPEC.md, DATA-MODEL.md, DEPENDENCY-DISCOVERY.md, INDEX.md, MASTER-PLAN.md, MILESTONES.md, PIPELINES.md: updated to reflect P1 shipped; compile/build marked as planned P2/P3.
  • CACHE-DESIGN.md §2.2: replaced XOR of object keys with a length-delimited ordered sequence hash; linker argv hashed in original order; removed sorting of -I paths and linker flags.
  • DECISIONS.md ADR-006: replaced the execvp-replacement model with the implemented fork+exec+waitpid design including the FD_CLOEXEC pipe.
  • DEPENDENCY-DISCOVERY.md §4.3: mtime-gated hashing documented as best-effort with an explicit caveat and --clean escape hatch noted.
  • PIPELINES.md Stage 4: describes the shipped fork+exec+waitpid behavior and exec-failure detection.
  • MILESTONES.md M1 tasks: all checked off; directory/project support noted.
  • TODO.md P1 items: all checked off.
  • ISSUES.md: CW-001 moved to Resolved; CW-002 is now the first open C-workflow item.
  • PLAN.md: M1 marked shipped; M2 listed as next milestone.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/compiler/PIPELINES.md (1)

267-272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the run validation docs with the actual guards.

cw_run_validate_request only rejects .mcsh files and then rejects non-.c files; it does not read the first line or check a shebang. Limit this sentence to the .mcsh guard, or add the file-open/shebang check in the same function.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/PIPELINES.md` around lines 267 - 272, Update the run validation
documentation to state that cw_run_validate_request rejects arguments ending in
.mcsh, then rejects non-.c files; remove the unsupported shell-script/shebang
detection claim unless the corresponding check is added to
cw_run_validate_request.
docs/compiler/MASTER-PLAN.md (1)

91-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Record the complete toolchain selection contract.

The completed task mentions only clang lookup through PATH. The shipped run contract also validates clang-family compilers and supports $mcsh_cc. Include both paths in this milestone entry.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/MASTER-PLAN.md` at line 91, Update the completed P1 toolchain
entry in MASTER-PLAN.md to document the full run contract: clang-family compiler
validation and compiler selection through both PATH lookup and the $mcsh_cc
environment variable. Preserve the existing BSD-licensing and completion status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/compiler/ARCHITECTURE.md`:
- Around line 3-4: Update the future integration example in Section 3.1 of
ARCHITECTURE.md to remove the stale dorun entry, leaving only the compile and
build placeholders while preserving the shipped run status.

In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 3-4: Update the P1 cache design documentation to describe the
identity generated by cw_run_prepare_state(): sorted .c/.h paths and contents
form project_hash, which combines with cc_path for object and binary cache
paths. Define changes to compiler flags, include search paths, link flags,
source order, or object hash order as invalidating directory-scoped cached
artifacts, and revise the object/binary key contracts to remove unsupported
dep_hash, profile_hash, and artifact-metadata-order components.

In `@docs/compiler/CLI-SPEC.md`:
- Line 11: Add a language tag such as text to the changed Markdown fence at
docs/compiler/CLI-SPEC.md lines 11-11 and docs/compiler/DEPENDENCY-DISCOVERY.md
lines 151-151, preserving the existing fenced content.
- Line 14: Update the run command syntax overview to represent both supported
argument forms: direct optional program arguments and arguments following an
optional `--` separator. Keep the existing run command structure and target
notation unchanged.

In `@docs/compiler/INDEX.md`:
- Around line 3-4: Update the quick-reference TODO entry in the compiler
documentation so it no longer presents the P1 queue as the immediate next task
now that run is shipped; either point it to post-P1 work or clearly label the P1
checklist as completed, while preserving the existing P2 and P3 planning
references.

In `@docs/compiler/MILESTONES.md`:
- Around line 34-54: Update the M1 title and objective to explicitly include
directory targets as shipped P1 behavior, and revise the dorun() validation task
so it permits supported directory targets while still rejecting invalid inputs
such as .mcsh files. Keep the existing execution task and directory-target
support wording consistent with this contract.

In `@PLAN.md`:
- Around line 50-55: Complete the P1 documentation status migration: in PLAN.md
lines 50-55, update the Phase 2 status to show run as implemented and only
compile and build as planned; in docs/c-commands-spec.md line 3, mark run syntax
and behavior as delivered, including directory targets and supported options; in
docs/c-workflow-analysis.md lines 3-4, replace remaining P1 implementation
instructions with delivered behavior or historical design context.

In `@sh.cworkflow.c`:
- Around line 1001-1006: Update the exec error-pipe handling around the child
write and parent read to retry operations interrupted by EINTR and ensure the
full exec_errno object is transferred, including handling partial writes and
reads. When either pipe operation fails or cannot complete, propagate a distinct
internal-error result rather than allowing the failure to be interpreted as
normal exit status 127; preserve the existing execv failure diagnostic path for
successfully transferred errno data.

---

Outside diff comments:
In `@docs/compiler/MASTER-PLAN.md`:
- Line 91: Update the completed P1 toolchain entry in MASTER-PLAN.md to document
the full run contract: clang-family compiler validation and compiler selection
through both PATH lookup and the $mcsh_cc environment variable. Preserve the
existing BSD-licensing and completion status.

In `@docs/compiler/PIPELINES.md`:
- Around line 267-272: Update the run validation documentation to state that
cw_run_validate_request rejects arguments ending in .mcsh, then rejects non-.c
files; remove the unsupported shell-script/shebang detection claim unless the
corresponding check is added to cw_run_validate_request.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d6309327-02af-4c02-84d3-06e1bb984997

📥 Commits

Reviewing files that changed from the base of the PR and between 30cb77a and a0896e8.

📒 Files selected for processing (16)
  • ISSUES.md
  • PLAN.md
  • docs/c-commands-spec.md
  • docs/c-workflow-analysis.md
  • docs/compiler/ARCHITECTURE.md
  • docs/compiler/CACHE-DESIGN.md
  • docs/compiler/CLI-SPEC.md
  • docs/compiler/DATA-MODEL.md
  • docs/compiler/DECISIONS.md
  • docs/compiler/DEPENDENCY-DISCOVERY.md
  • docs/compiler/INDEX.md
  • docs/compiler/MASTER-PLAN.md
  • docs/compiler/MILESTONES.md
  • docs/compiler/PIPELINES.md
  • docs/compiler/TODO.md
  • sh.cworkflow.c

Comment thread docs/compiler/ARCHITECTURE.md
Comment thread docs/compiler/CACHE-DESIGN.md
Comment thread docs/compiler/CLI-SPEC.md Outdated
Comment thread docs/compiler/CLI-SPEC.md Outdated
Comment thread docs/compiler/INDEX.md
Comment thread docs/compiler/MILESTONES.md Outdated
Comment thread PLAN.md
Comment thread sh.cworkflow.c Outdated
@orpheus497

Copy link
Copy Markdown
Owner Author

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @docs/compiler/ARCHITECTURE.md:

  • Around line 3-4: Update the future integration example in Section 3.1 of
    ARCHITECTURE.md to remove the stale dorun entry, leaving only the compile and
    build placeholders while preserving the shipped run status.

In @docs/compiler/CACHE-DESIGN.md:

  • Around line 3-4: Update the P1 cache design documentation to describe the
    identity generated by cw_run_prepare_state(): sorted .c/.h paths and contents
    form project_hash, which combines with cc_path for object and binary cache
    paths. Define changes to compiler flags, include search paths, link flags,
    source order, or object hash order as invalidating directory-scoped cached
    artifacts, and revise the object/binary key contracts to remove unsupported
    dep_hash, profile_hash, and artifact-metadata-order components.

In @docs/compiler/CLI-SPEC.md:

  • Line 11: Add a language tag such as text to the changed Markdown fence at
    docs/compiler/CLI-SPEC.md lines 11-11 and docs/compiler/DEPENDENCY-DISCOVERY.md
    lines 151-151, preserving the existing fenced content.
  • Line 14: Update the run command syntax overview to represent both supported
    argument forms: direct optional program arguments and arguments following an
    optional -- separator. Keep the existing run command structure and target
    notation unchanged.

In @docs/compiler/INDEX.md:

  • Around line 3-4: Update the quick-reference TODO entry in the compiler
    documentation so it no longer presents the P1 queue as the immediate next task
    now that run is shipped; either point it to post-P1 work or clearly label the P1
    checklist as completed, while preserving the existing P2 and P3 planning
    references.

In @docs/compiler/MILESTONES.md:

  • Around line 34-54: Update the M1 title and objective to explicitly include
    directory targets as shipped P1 behavior, and revise the dorun() validation task
    so it permits supported directory targets while still rejecting invalid inputs
    such as .mcsh files. Keep the existing execution task and directory-target
    support wording consistent with this contract.

In @PLAN.md:

  • Around line 50-55: Complete the P1 documentation status migration: in PLAN.md
    lines 50-55, update the Phase 2 status to show run as implemented and only
    compile and build as planned; in docs/c-commands-spec.md line 3, mark run syntax
    and behavior as delivered, including directory targets and supported options; in
    docs/c-workflow-analysis.md lines 3-4, replace remaining P1 implementation
    instructions with delivered behavior or historical design context.

In @sh.cworkflow.c:

  • Around line 1001-1006: Update the exec error-pipe handling around the child
    write and parent read to retry operations interrupted by EINTR and ensure the
    full exec_errno object is transferred, including handling partial writes and
    reads. When either pipe operation fails or cannot complete, propagate a distinct
    internal-error result rather than allowing the failure to be interpreted as
    normal exit status 127; preserve the existing execv failure diagnostic path for
    successfully transferred errno data.

Outside diff comments:
In @docs/compiler/MASTER-PLAN.md:

  • Line 91: Update the completed P1 toolchain entry in MASTER-PLAN.md to document
    the full run contract: clang-family compiler validation and compiler selection
    through both PATH lookup and the $mcsh_cc environment variable. Preserve the
    existing BSD-licensing and completion status.

In @docs/compiler/PIPELINES.md:

  • Around line 267-272: Update the run validation documentation to state that
    cw_run_validate_request rejects arguments ending in .mcsh, then rejects non-.c
    files; remove the unsupported shell-script/shebang detection claim unless the
    corresponding check is added to cw_run_validate_request.

sh.cworkflow.c:
- cw_execute_binary: child write now retries on EINTR and accumulates
  partial writes to transfer the full exec_errno value
- cw_execute_binary: parent read now retries on EINTR, accumulates
  partial reads, and returns -3 on read error or partial transfer
  (distinct from -2 exec failure and -1 fork/pipe-setup failure)
- cw_run_execute_stage: handle -3 with "run: internal error: exec pipe
  failed" diagnostic

Documentation:
- ARCHITECTURE.md §3.1: remove stale dorun from planned-only example
- CACHE-DESIGN.md §2: replace planned P2 key design with actual P1
  project_hash/object/binary key contracts; move P2/P3 planned keys
  to new §2.4/2.5
- CLI-SPEC.md: add `text` language tag to syntax fence; show both
  run arg forms (with and without -- separator)
- DEPENDENCY-DISCOVERY.md §4.3: add `text` language tag to fence
- INDEX.md: update "Immediate next TODO" to M2 (P1 is done)
- MILESTONES.md: M1 title/objective include directory targets; fix
  validation task to permit directories while rejecting .mcsh
- PLAN.md: update Phase 2 "Current status" to reflect M1 shipped
- MASTER-PLAN.md: expand P0 toolchain entry to include $mcsh_cc and
  clang-family validation; check off all P1 items
- PIPELINES.md Stage 1: remove unsupported shebang-detection claim
- docs/c-commands-spec.md: add directory targets to status line
- docs/c-workflow-analysis.md: Phase 1 section describes delivered
  behavior; stale "next milestone" replaced with historical note;
  integration-points section reflects shipped state

Copilot AI commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

...

All still-valid findings addressed in commit fix/docs: exec-pipe EINTR retry, -3 error path, and P1 doc sync.

sh.cworkflow.c (code)

  • Child write now loops on EINTR and accumulates partial writes to ensure the full exec_errno is transferred.
  • Parent read likewise loops on EINTR, accumulates partial reads, and returns -3 when the read fails or delivers a partial result. cw_run_execute_stage handles -3 with "run: internal error: exec pipe failed" — distinct from the -1 fork/pipe-setup path and the -2 exec-failed path.

Documentation (all findings valid, fixed)

  • ARCHITECTURE.md §3.1: removed dorun from the "planned" example block; only build and compile remain as future additions.
  • CACHE-DESIGN.md §2: replaced the old planned P2 key design with the actual P1 run key contracts (§2.1 project_hash, §2.2 object key, §2.3 binary key). Old planned P2/P3 designs moved to §2.4/2.5 for reference. Invalidation contract now explicitly covers compiler flags, include paths, link flags, source order, and object hash order.
  • CLI-SPEC.md line 11: added text language tag; line 14: syntax overview now shows both run <target> [args...] and run <target> -- [args...] forms.
  • DEPENDENCY-DISCOVERY.md line 151: added text language tag.
  • INDEX.md: "Immediate next TODO" updated to M2 (P1 done).
  • MILESTONES.md M1: title/objective updated to include directory targets; validation task now reads "reject .mcsh files; accept .c files and directories".
  • PLAN.md: Phase 2 "Current status" updated from "No runtime implementation yet" to "M1 ✓ shipped".
  • MASTER-PLAN.md line 91: expanded to document $mcsh_cc selection and clang-family validation; all P1 checklist items checked off.
  • PIPELINES.md Stage 1: removed "detected by shebang" claim — cw_run_validate_request only checks .mcsh suffix.
  • docs/c-commands-spec.md: status line now includes directory targets and supported options.
  • docs/c-workflow-analysis.md: Phase 1 section rewritten as delivered behavior; "Immediate next coding milestone" replaced with a historical-context note; integration-points section updated to reflect shipped state.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
docs/compiler/DEPENDENCY-DISCOVERY.md (1)

3-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Separate P1 invalidation from planned dependency tracking.

The status says P1 uses source-only content-hash keys and full header tracking is future P2 work. The invalidation rules still state that run detects header changes, deleted headers, and new dependency edges. Mark those rules as planned P2/P3 behavior, or add separate P1 and future-phase sections.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/DEPENDENCY-DISCOVERY.md` around lines 3 - 5, Update the
invalidation rules in the dependency-discovery documentation to distinguish
current P1 `run` behavior from planned dependency tracking: remove or relabel
claims that `run` detects header changes, deleted headers, or new dependency
edges as future P2/P3 behavior, while preserving the source-content-hash-only P1
cache-key description.
docs/compiler/CACHE-DESIGN.md (1)

3-4: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Scope the artifact metadata sections to planned phases.

The status identifies index.db and sidecar metadata as planned work. Later artifact and lookup sections describe .meta files and metadata validation as current cache behavior. Mark those sections as planned P2/P3 design, or document the actual P1 path-only lookup.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/CACHE-DESIGN.md` around lines 3 - 4, The documentation
currently presents artifact `.meta` files and metadata validation as implemented
despite the status limiting P1 to object and binary cache paths. Update the
later artifact and lookup sections to label `index.db`, sidecar metadata, and
related validation as planned P2/P3 work, or revise them to accurately describe
the implemented P1 path-only lookup.
docs/compiler/CLI-SPEC.md (1)

150-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Synchronize the option matrix with the implemented run syntax.

This section removes -p, -I, -D, and -g from run, but the option matrix still marks all four options as supported. Change those matrix entries to unsupported, or restore matching implementation and tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/CLI-SPEC.md` around lines 150 - 162, Update the option matrix
in the run command documentation to mark -p, -I, -D, and -g as unsupported,
matching the implemented run syntax; do not leave these options listed as
supported unless their implementation and tests are restored.
docs/compiler/PIPELINES.md (1)

301-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Document every exec-pipe result and include run status 255.

The documented run outcome only lists 127 for all execv failures and shows program exit statuses only through 0..254. Add the internal-error path for partial/failing pipe transfer, including its diagnostic run: internal error: exec pipe failed and exit code 127, and extend the executed-binary status range to include 255.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/PIPELINES.md` around lines 301 - 307, The PIPELINES.md “run”
outcome documentation should cover every exec-pipe result: document the partial
or failed pipe transfer as `run: internal error: exec pipe failed` with exit
code 127, and extend the executed binary’s propagated exit-status range from
0..254 to include 255.
docs/compiler/MILESTONES.md (1)

50-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Document the same argument-forwarding contract in both files.

The shipped interface accepts direct arguments and an optional -- separator. It preserves the tokens after the target as program arguments.

  • docs/compiler/MILESTONES.md#L50-L53: replace “with -- args” with direct-or-separator argument forwarding.
  • docs/compiler/PIPELINES.md#L294-L300: state that the child receives the original argument order and multiplicity, whether or not -- is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/MILESTONES.md` around lines 50 - 53, Update the dorun
argument-forwarding documentation in docs/compiler/MILESTONES.md lines 50-53 to
describe direct arguments or an optional -- separator instead of only “with --
args”; update docs/compiler/PIPELINES.md lines 294-300 to state that the child
receives tokens after the target in their original order and multiplicity,
regardless of whether -- is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/compiler/ARCHITECTURE.md`:
- Around line 126-131: Update docs/compiler/ARCHITECTURE.md lines 126-131 to
remove dorun() from future declaration/implementation examples or clearly mark
those sections as current state; update docs/compiler/MILESTONES.md line 41 to
replace “dorun() stub” with wording that reflects the completed implementation.

In `@docs/compiler/CACHE-DESIGN.md`:
- Line 41: Add a suitable language tag, such as text, to each new pseudocode
fenced block in CACHE-DESIGN.md, including the blocks at the referenced
locations, while leaving their pseudocode content unchanged.
- Around line 73-89: Update the P1 binary cache documentation around the “run —
binary cache key” section so invalidation claims match the displayed key: only
source/header changes represented by project_hash and compiler identity
represented by cc_hash should be described as P1 invalidators. Describe compiler
flags, include paths, linker flags, source order, and object order as outside
the P1 cache identity and directory-cache limitations, unless the key,
implementation, and tests are expanded accordingly.
- Around line 127-134: Update the “Normalization before hashing” section to
preserve repeated compiler flags and remove the instruction to eliminate
duplicates such as repeated -O options. Document that normalization must retain
argument order and multiplicity, while continuing to normalize path separators
and leave include paths and linker flags unsorted.
- Around line 35-52: Update the project_hash formula in the “2.1 P1 run —
project hash” section to explicitly include each fingerprint’s path, content,
and delimiter, and list source position if it is an actual input; otherwise
remove unsupported position claims. Ensure the surrounding prose and formula
consistently describe the inputs needed to distinguish different project
layouts.

In `@docs/compiler/MASTER-PLAN.md`:
- Around line 98-101: Update the command contract in the P1 section of
MASTER-PLAN.md to accept both individual C files and directory/project targets,
changing the documented run syntax from file-only to a form such as run
<file.c|target> [...]. Keep the existing compilation and execution behavior
unchanged.
- Around line 102-111: Unify compiler validation so every selected compiler must
pass clang-family validation. In docs/compiler/MASTER-PLAN.md lines 102-111,
clarify that $mcsh_cc or the compiler selected from PATH is mandatory
clang-family validated; make the corresponding discovery step in
docs/compiler/MILESTONES.md lines 45-46 explicitly require validation rather
than merely preferring clang or falling back to cc.

---

Outside diff comments:
In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 3-4: The documentation currently presents artifact `.meta` files
and metadata validation as implemented despite the status limiting P1 to object
and binary cache paths. Update the later artifact and lookup sections to label
`index.db`, sidecar metadata, and related validation as planned P2/P3 work, or
revise them to accurately describe the implemented P1 path-only lookup.

In `@docs/compiler/CLI-SPEC.md`:
- Around line 150-162: Update the option matrix in the run command documentation
to mark -p, -I, -D, and -g as unsupported, matching the implemented run syntax;
do not leave these options listed as supported unless their implementation and
tests are restored.

In `@docs/compiler/DEPENDENCY-DISCOVERY.md`:
- Around line 3-5: Update the invalidation rules in the dependency-discovery
documentation to distinguish current P1 `run` behavior from planned dependency
tracking: remove or relabel claims that `run` detects header changes, deleted
headers, or new dependency edges as future P2/P3 behavior, while preserving the
source-content-hash-only P1 cache-key description.

In `@docs/compiler/MILESTONES.md`:
- Around line 50-53: Update the dorun argument-forwarding documentation in
docs/compiler/MILESTONES.md lines 50-53 to describe direct arguments or an
optional -- separator instead of only “with -- args”; update
docs/compiler/PIPELINES.md lines 294-300 to state that the child receives tokens
after the target in their original order and multiplicity, regardless of whether
-- is present.

In `@docs/compiler/PIPELINES.md`:
- Around line 301-307: The PIPELINES.md “run” outcome documentation should cover
every exec-pipe result: document the partial or failed pipe transfer as `run:
internal error: exec pipe failed` with exit code 127, and extend the executed
binary’s propagated exit-status range from 0..254 to include 255.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b93b01fe-95ce-4540-87da-be2bba37e5f2

📥 Commits

Reviewing files that changed from the base of the PR and between a0896e8 and b74a6bc.

📒 Files selected for processing (12)
  • PLAN.md
  • docs/c-commands-spec.md
  • docs/c-workflow-analysis.md
  • docs/compiler/ARCHITECTURE.md
  • docs/compiler/CACHE-DESIGN.md
  • docs/compiler/CLI-SPEC.md
  • docs/compiler/DEPENDENCY-DISCOVERY.md
  • docs/compiler/INDEX.md
  • docs/compiler/MASTER-PLAN.md
  • docs/compiler/MILESTONES.md
  • docs/compiler/PIPELINES.md
  • sh.cworkflow.c

Comment thread docs/compiler/ARCHITECTURE.md
Comment thread docs/compiler/CACHE-DESIGN.md
Comment thread docs/compiler/CACHE-DESIGN.md Outdated
Comment thread docs/compiler/CACHE-DESIGN.md Outdated
Comment thread docs/compiler/CACHE-DESIGN.md Outdated
Comment on lines +98 to 101
### P1 — `run file.c` MVP ✓ shipped

**Goal:** `run hello.c` compiles hello.c (if needed), executes it, returns exit status.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Update the immutable contract for directory targets.

The completed P1 section documents directory/project targets, but the command contract still lists run file.c [...] only. Change that contract to include a directory target, such as run <file.c|target> [...].

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/MASTER-PLAN.md` around lines 98 - 101, Update the command
contract in the P1 section of MASTER-PLAN.md to accept both individual C files
and directory/project targets, changing the documented run syntax from file-only
to a form such as run <file.c|target> [...]. Keep the existing compilation and
execution behavior unchanged.

Comment on lines +102 to +111
- [x] Implement `dorun()` in new `sh.cworkflow.c`
- [x] Register `"run"` in `bfunc[]` (`sh.init.c`) in sorted position
- [x] Add `extern void dorun(Char **, struct command *)` in `sh.decls.h`
- [x] Input validation: accept `.c` files and directories; reject `.mcsh` with guidance error
- [x] Cache key: SHA-256 of source content + compiler identity (project_hash + cc_hash)
- [x] Cache directory: `~/.mcsh_cache/cworkflow/`
- [x] Compile with discovered system cc (clang-family preferred; `$mcsh_cc` or PATH)
- [x] Execute compiled binary; propagate child exit status
- [x] Basic diagnostics: compile errors printed to stderr with file:line format
- [x] Directory/project target support

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use one compiler-validation contract.

The implementation requires a clang-family compiler, but the documents use “preferred” and “clang → cc” wording that can imply non-clang compilers are accepted.

  • docs/compiler/MASTER-PLAN.md#L102-L111: state that $mcsh_cc or the PATH-selected compiler must pass clang-family validation.
  • docs/compiler/MILESTONES.md#L45-L46: document discovery followed by mandatory clang-family validation.
📍 Affects 2 files
  • docs/compiler/MASTER-PLAN.md#L102-L111 (this comment)
  • docs/compiler/MILESTONES.md#L45-L46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/MASTER-PLAN.md` around lines 102 - 111, Unify compiler
validation so every selected compiler must pass clang-family validation. In
docs/compiler/MASTER-PLAN.md lines 102-111, clarify that $mcsh_cc or the
compiler selected from PATH is mandatory clang-family validated; make the
corresponding discovery step in docs/compiler/MILESTONES.md lines 45-46
explicitly require validation rather than merely preferring clang or falling
back to cc.

@orpheus497

Copy link
Copy Markdown
Owner Author

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @docs/compiler/ARCHITECTURE.md:

  • Around line 126-131: Update docs/compiler/ARCHITECTURE.md lines 126-131 to
    remove dorun() from future declaration/implementation examples or clearly mark
    those sections as current state; update docs/compiler/MILESTONES.md line 41 to
    replace “dorun() stub” with wording that reflects the completed implementation.

In @docs/compiler/CACHE-DESIGN.md:

  • Line 41: Add a suitable language tag, such as text, to each new pseudocode
    fenced block in CACHE-DESIGN.md, including the blocks at the referenced
    locations, while leaving their pseudocode content unchanged.
  • Around line 73-89: Update the P1 binary cache documentation around the “run —
    binary cache key” section so invalidation claims match the displayed key: only
    source/header changes represented by project_hash and compiler identity
    represented by cc_hash should be described as P1 invalidators. Describe compiler
    flags, include paths, linker flags, source order, and object order as outside
    the P1 cache identity and directory-cache limitations, unless the key,
    implementation, and tests are expanded accordingly.
  • Around line 127-134: Update the “Normalization before hashing” section to
    preserve repeated compiler flags and remove the instruction to eliminate
    duplicates such as repeated -O options. Document that normalization must retain
    argument order and multiplicity, while continuing to normalize path separators
    and leave include paths and linker flags unsorted.
  • Around line 35-52: Update the project_hash formula in the “2.1 P1 run —
    project hash” section to explicitly include each fingerprint’s path, content,
    and delimiter, and list source position if it is an actual input; otherwise
    remove unsupported position claims. Ensure the surrounding prose and formula
    consistently describe the inputs needed to distinguish different project
    layouts.

In @docs/compiler/MASTER-PLAN.md:

  • Around line 98-101: Update the command contract in the P1 section of
    MASTER-PLAN.md to accept both individual C files and directory/project targets,
    changing the documented run syntax from file-only to a form such as run
    <file.c|target> [...]. Keep the existing compilation and execution behavior
    unchanged.
  • Around line 102-111: Unify compiler validation so every selected compiler must
    pass clang-family validation. In docs/compiler/MASTER-PLAN.md lines 102-111,
    clarify that $mcsh_cc or the compiler selected from PATH is mandatory
    clang-family validated; make the corresponding discovery step in
    docs/compiler/MILESTONES.md lines 45-46 explicitly require validation rather
    than merely preferring clang or falling back to cc.

Outside diff comments:
In @docs/compiler/CACHE-DESIGN.md:

  • Around line 3-4: The documentation currently presents artifact .meta files
    and metadata validation as implemented despite the status limiting P1 to object
    and binary cache paths. Update the later artifact and lookup sections to label
    index.db, sidecar metadata, and related validation as planned P2/P3 work, or
    revise them to accurately describe the implemented P1 path-only lookup.

In @docs/compiler/CLI-SPEC.md:

  • Around line 150-162: Update the option matrix in the run command documentation
    to mark -p, -I, -D, and -g as unsupported, matching the implemented run syntax;
    do not leave these options listed as supported unless their implementation and
    tests are restored.

In @docs/compiler/DEPENDENCY-DISCOVERY.md:

  • Around line 3-5: Update the invalidation rules in the dependency-discovery
    documentation to distinguish current P1 run behavior from planned dependency
    tracking: remove or relabel claims that run detects header changes, deleted
    headers, or new dependency edges as future P2/P3 behavior, while preserving the
    source-content-hash-only P1 cache-key description.

In @docs/compiler/MILESTONES.md:

  • Around line 50-53: Update the dorun argument-forwarding documentation in
    docs/compiler/MILESTONES.md lines 50-53 to describe direct arguments or an
    optional -- separator instead of only “with -- args”; update
    docs/compiler/PIPELINES.md lines 294-300 to state that the child receives tokens
    after the target in their original order and multiplicity, regardless of whether
    -- is present.

In @docs/compiler/PIPELINES.md:

  • Around line 301-307: The PIPELINES.md “run” outcome documentation should cover
    every exec-pipe result: document the partial or failed pipe transfer as run: internal error: exec pipe failed with exit code 127, and extend the executed
    binary’s propagated exit-status range from 0..254 to include 255.

Copilot AI commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

...

All still-valid findings fixed in commit docs: fix ARCHITECTURE, MILESTONES, CACHE-DESIGN P1 accuracy.

ARCHITECTURE.md §3.2/§3.3dorun removed from the future-declarations block; §3.3 now states sh.cworkflow.c exists and dorun is fully implemented (P1 shipped), with only docompile/dobuild shown as future planned stubs.

MILESTONES.md line 41 — "dorun() stub" → "dorun() implementation".

CACHE-DESIGN.md §2.1 (project_hash formula) — formula updated to explicitly show path(fingerprints[i]) (NUL-terminated) followed by SHA-256(content(fingerprints[i])) per entry, matching cw_hash_file_set(). Unsupported "position" language removed.

CACHE-DESIGN.md §2.2 (object cache key prose) — removed "its position within the project" claim; prose now says "source path and content, the project-wide input set, and the compiler identity".

CACHE-DESIGN.md §2.3 (binary cache key invalidation) — removed the inaccurate claim that compiler flags/include paths/linker flags invalidate the cache "because they alter project_hash or cc_hash" (they don't in P1). Replaced with an explicit note that those inputs are outside the P1 cache identity.

CACHE-DESIGN.md §2.1/§2.2/§2.3 fenced blocks — all pseudocode fences now carry the text language tag.

CACHE-DESIGN.md §2.6 (normalization) — removed "Remove redundant flags (duplicate -O, etc.)"; section now states argument order and multiplicity are preserved exactly as passed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/compiler/CACHE-DESIGN.md (1)

60-75: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Define cc_hash as a single fallback contract.

The implementation hashes the compiler file first, then hashes the compiler path when file hashing fails. Document this as the exact cc_hash input and invalidation rule in the object key formula, binary key formula, and cache invalidators. A separate cached compiler binary means toolchain changes invalidate objects and binaries; a plain path hash would not.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/CACHE-DESIGN.md` around lines 60 - 75, Update CACHE-DESIGN.md
to define cc_hash consistently as the compiler binary-content hash when file
hashing succeeds, with the compiler path hash as the sole fallback when it
fails. Apply this exact contract to both object and binary key formulas and
explicitly state that compiler changes invalidate cached objects and binaries,
while path hashing is only the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 37-50: Update cw_run_prepare_state() to canonicalize each source
and header path before adding it to the fingerprint list used by
cw_hash_file_set(). Ensure equivalent inputs such as file.c, ./file.c, and
/absolute/file.c use the same normalized path while preserving the existing
sorted fingerprint and content-hash behavior.

---

Outside diff comments:
In `@docs/compiler/CACHE-DESIGN.md`:
- Around line 60-75: Update CACHE-DESIGN.md to define cc_hash consistently as
the compiler binary-content hash when file hashing succeeds, with the compiler
path hash as the sole fallback when it fails. Apply this exact contract to both
object and binary key formulas and explicitly state that compiler changes
invalidate cached objects and binaries, while path hashing is only the fallback.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 13e9d258-7c8e-42fe-af05-895ede89ae55

📥 Commits

Reviewing files that changed from the base of the PR and between b74a6bc and 0b86271.

📒 Files selected for processing (3)
  • docs/compiler/ARCHITECTURE.md
  • docs/compiler/CACHE-DESIGN.md
  • docs/compiler/MILESTONES.md

Comment on lines +37 to +50
`cw_run_prepare_state()` computes a **project_hash** that covers all source and
header inputs. Sorted `.c` and `.h` file paths are collected into the
fingerprint list; each entry contributes its path, a NUL delimiter, and its
content hash to the rolling digest:

```text
project_hash = SHA-256(
path(fingerprints[0]) ← absolute path (NUL-terminated)
SHA-256(content(fingerprints[0])) ← content hash of that file
path(fingerprints[1])
SHA-256(content(fingerprints[1]))
...
path(fingerprints[N-1])
SHA-256(content(fingerprints[N-1]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/sh
set -eu

rg -n -C 12 \
  'cw_parse|target_path|realpath|cw_collect_project_files|cw_hash_file_set' \
  sh.cworkflow.c

Repository: orpheus497/mcsh

Length of output: 6613


🏁 Script executed:

#!/bin/sh
set -eu

sed -n '1139,1275p' sh.cworkflow.c
printf '\n--- find references to realpath/canonicalize/norm paths ---\n'
rg -n 'realpath|canon|normalize|target_path|fingerprints|cw_run_prepare_state|cw_hash_toolchain' sh.cworkflow.c docs/compiler/CACHE-DESIGN.md

Repository: orpheus497/mcsh

Length of output: 7094


Canonicalize paths before hashing.

run file.c saves req->target_path as file.c; stat accepts it, but cw_hash_file_set() hashes that literal fingerprint path. This can give different project_hash values for file.c, ./file.c, and /absolute/file.c. Normalize these paths before pushing to the fingerprint list.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/compiler/CACHE-DESIGN.md` around lines 37 - 50, Update
cw_run_prepare_state() to canonicalize each source and header path before adding
it to the fingerprint list used by cw_hash_file_set(). Ensure equivalent inputs
such as file.c, ./file.c, and /absolute/file.c use the same normalized path
while preserving the existing sorted fingerprint and content-hash behavior.

@orpheus497

Copy link
Copy Markdown
Owner Author

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @docs/compiler/CACHE-DESIGN.md:

  • Around line 37-50: Update cw_run_prepare_state() to canonicalize each source
    and header path before adding it to the fingerprint list used by
    cw_hash_file_set(). Ensure equivalent inputs such as file.c, ./file.c, and
    /absolute/file.c use the same normalized path while preserving the existing
    sorted fingerprint and content-hash behavior.

Outside diff comments:
In @docs/compiler/CACHE-DESIGN.md:

  • Around line 60-75: Update CACHE-DESIGN.md to define cc_hash consistently as
    the compiler binary-content hash when file hashing succeeds, with the compiler
    path hash as the sole fallback when it fails. Apply this exact contract to both
    object and binary key formulas and explicitly state that compiler changes
    invalidate cached objects and binaries, while path hashing is only the fallback.

Copilot AI commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@copilot Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

...

Both findings fixed in commit fix/docs: canonicalize fingerprint paths in cw_run_prepare_state, clarify cc_hash in CACHE-DESIGN.

sh.cworkflow.ccw_run_prepare_state() now calls realpath() on the target path before adding it to the fingerprint/sources lists. If realpath() fails (file removed between validate and prepare) it falls back to the original path. This ensures file.c, ./file.c, and /absolute/file.c produce identical project_hash values.

CACHE-DESIGN.md §2.1 — prose updated to document the realpath() canonicalization; formula annotation changed from absolute path to realpath-resolved path.

CACHE-DESIGN.md §2.2 and §2.3cc_hash annotation in both key formulas now reads SHA-256(compiler binary content); SHA-256(compiler path) if binary unreadable, with a prose note after each formula explaining the primary/fallback contract and that any compiler change invalidates cached artifacts.

@orpheus497
orpheus497 merged commit ff31a0d into develop/comp Aug 1, 2026
1 check passed
@orpheus497
orpheus497 deleted the phase1 branch August 1, 2026 15:55
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.

2 participants