PLAN_07: spec-sheet methodology (08.1/08.4/08.5) and batch-1 sheets 1-5 of 10 - #31
Merged
Conversation
Author the canonical spec-sheet template (Documents/specs/_TEMPLATE.md) and the indexed layout reference (Documents/specs/README.md) per PLAN_07 §3 and §4. These are the deliverables of subtask 08.1 and the gate for the rest of PLAN_07 (08.2 sheets, 08.4 check-specs, 08.5 refuse!). The template reproduces the seven mandatory sections plus the conditional §7 POSIX divergence. Bare angle-bracket placeholders that trip MD033 are backticked so the file lints clean while preserving the fill-in-the-blank intent.
Implement the spec-sheet cross-reference checker per PLAN_07 §8.1. It walks Documents/specs/ and enforces five invariants: 1. Every support row names a corpus case that loads and declares status pass or deferred:PLAN_XX. 2. Every corpus case under tests/spec/ is referenced by exactly one sheet row (zero = orphan, >1 = conflict; both fail). 3. No §3 row carries the ??? placeholder classification. 4. The seven mandatory sections are present and in order, with the conditional §7 POSIX divergence tolerated between §6 and §8. 5. Every defer:N row is backed by a §6 workaround paragraph. The sheet parser is line-oriented, matching the sibling spec module's house style (no Markdown-parser dependency). _TEMPLATE.md and README.md are excluded from the sheet walk. Not yet wired into xtask pc/check: check 2 cannot pass until the sheet inventory is complete (08.2/08.3/08.6), so the pc/check wiring lands with 08.6. Until then the command is run manually on the branch. 31 unit tests cover classification parsing, row/section parsing, section-order validation, and all five checks; the corpus-root path is injected so tests stay hermetic.
Add a §14 implementation log to PLAN_07 (matching the PLAN_05 §14 format) with rows for 08.1 (template + Documents/specs README) and 08.4 (cargo xtask check-specs). Bump the PLAN_07 header status from `stub` to `in progress` and update plan.md row 07 `drafted` → `in progress` with a note on what landed and what remains. Commit column is TBD until task-07/spec-drafting merges to main.
Implement the PLAN_07 §8.2 refusal path: builtins refuse wontfix /
defer:N behaviours by constructing a typed value, validated against
the spec sheet at compile time.
- New fredshell-core::spec module: Refusal value + RefusalKind
(Wontfix | Defer { milestone, milestone_name, workaround }) and
REFUSAL_EXIT_STATUS = 2. Display renders the exact §5.2 / §5.3
wording. Per ADR 0006 the core returns this value; the binary REPL
renders it as a diagnostic — core never writes to a file descriptor.
- New fredshell-spec-macros proc-macro crate exposing refuse!. The
macro reads the referenced sheet at compile time, parses the §3
table, and fails compilation if the sheet/row is missing or the
classification does not match the form. Grammar:
refuse!(wontfix, "id", "row") and
refuse!(defer, "id", "row", milestone_name = "…", workaround = "…").
The defer milestone N comes from the §3 defer:N cell; the milestone
name and workaround are §6 prose, passed as named args (documented
extension of §8.2's single wontfix example). Sheet resolution
ascends to Documents/specs/, overridable via FREDSHELL_SPECS_ROOT.
- fredshell-core gains a downward dependency on the macro crate and
re-exports refuse!. New workspace deps proc-macro2 / quote / syn
(production) and trybuild (dev), pinned and sorted. Workspace
members, the AGENTS.md crate table, and the dependency-direction
note are updated in this commit per the crate-creation rule.
Tests: 5 Refusal rendering tests, 5 sheet-parser unit tests, 5
refuse! expansion integration tests, and 5 trybuild compile-fail
cases proving the compile-time guarantee. PLAN_07 §14 log + plan.md
status updated. cargo test/clippy/machete/fmt all clean.
`xtask spec record` invoked the pinned bash as `bash -c <script>` with no $0 name argument, so bash's $0 was its own absolute path (a `/nix/store/<hash>-bash-…/bin/bash` path). bash prefixes $0 onto every diagnostic it prints, so the recorder wrote that store path verbatim into committed `.stderr` fixtures. The spec-runner compares stderr byte-exact, so those fixtures were non-portable across machines and unstable across nixpkgs bumps. Pass a fixed $0 (`bash`) via `bash -c <script> bash` so diagnostics read `bash: line 1: …`, matching what a real script sees for $0. Re-recorded the two committed fixtures that carried the store path (`exit_nonnumeric_arg.stderr`, `background_wait.stderr`). Added a regression test asserting recorded diagnostics start with `bash:` and contain no `/nix/store` path (skipped outside the devshell). Surfaced while drafting the cd spec sheet (PLAN_07 08.2); fixed separately as it is corpus-infrastructure, not sheet content.
The cd builtin called std::env::set_current_dir, mutating the process-global working directory instead of the shell's ExecEnv.cwd field. This broke sandbox isolation: when the spec harness ran a cd case, the builtin chdir'd the harness process into a sandbox tempdir that was then deleted, so subsequent relative-path Case::load calls crashed. It also called eprintln!, a direct fd write from fredshell-core that violates the ADR 0006 embedding contract. Fix the cd builtin to resolve the target against env.cwd (not the process cwd), update env.cwd on success, read $HOME from env.env, and write the failure diagnostic to env.stderr. try_run now takes &mut ExecEnv; spawn_via_sh sets the child current_dir to env.cwd. Also anchor the .gitignore target/ rule to /target/ so spec-corpus .fs/ skeleton dirs named target are not dropped from git. Replaces the two process-cwd cd tests with four covering env.cwd update, relative resolution, the stderr diagnostic, and HOME-not-set. Surfaced while drafting the cd spec sheet (PLAN_07 08.2).
First sheet of PLAN_07 batch 1: cd. Authored
Documents/specs/builtins/cd.md with a 12-row §3 support matrix:
- 7 support rows (cd dir, cd to $HOME, cd -, cd .., nonexistent-dir
exit 1, too-many-args exit 2, CDPATH resolution), each backed by a
hermetic corpus case;
- 1 wontfix row (-@ extended attributes, the §5.2 canonical example);
- 4 defer rows (-L/-P symlink resolution and -e to milestone 3,
because the spec-runner's copy_dir_recursive skips symlinks in v0;
cdable_vars to milestone 4 pending shopt).
Added 7 corpus cases under tests/spec/builtins_tier1/cd_*.case.toml
(status deferred:PLAN_12), recorded against bash 5.3p9. Each uses
only shell builtins plus a .fs/ skeleton (no external coreutils, per
the recorder's env_clear constraint) and ${PWD##*/} instead of
basename for sandbox-independent output.
check-specs reports the cd sheet clean; spec lint + compat green
(the 7 cd cases honored as deferred:PLAN_12); COMPAT.md regenerated.
PLAN_07 §14 (08.2a) and plan.md updated.
The two pre-existing bugs surfaced while drafting this sheet (recorder
argv0 store-path, cd builtin global-CWD/eprintln!) were fixed in their
own preceding commits.
An unbackticked `PLAN_07` in the 08.2d implementation-log row opened a CommonMark emphasis run on its underscore. Prettier resolved that run by re-pairing the surrounding backtick delimiters, which normalised the underscore to `PLAN*07`, rewrote the backticked glob `colon_*.case.toml` to `colon*\*.case.toml`, and ate the spaces around several later code spans. Backtick `PLAN_07` (and `refuse!`) so no stray underscore can open an emphasis run, and restore the mangled glob and the eaten spacing. Verified prettier is now idempotent over the row.
Batch-1 spec drafting is half done (5 of 10 sheets) and the branch is about to be PR'd, but the documentation recorded what happened without recording how to continue. No sheets or corpus cases change here. - Add §15 Cleanup registry with entry 08.2e-CU1: the mixed-line-ending pre-commit hook silently rewrites a lone CR in a recorded fixture and then rejects the commit. It surfaced during 08.2e and previously existed only as prose in a log cell, which the AGENTS.md surfaced-bug rule forbids. printf is the next sheet and the most exposed. - Add a Status column to §10 and a §10.1 batch-1 sheet checklist with per-sheet log IDs, notes and batch exit criteria, so progress does not have to be reconstructed from five very long §14 cells. - Record the symbol-named-builtin filename exception in §3 as a table (`:` -> colon.md, `.` -> dot.md, `[` -> bracket.md). The invocation-name rule cannot apply to punctuation, and `.` and `[` are still unsheeted, so the 08.2d decision would be re-litigated. - Add a provisional defer:N milestone table to §5.3 (3 filesystem-touch, 4 shopt, 5 UTF-8/locale) with an instruction to reuse rather than invent numbers, since PLAN_19_milestones.md does not exist yet. - Expand Documents/specs/README.md with the corpus-authoring runbook: probe bash rather than recall it, the recorder constraints that actually bite (no PATH so coreutils exit 127, effectively C locale, symlinks do not materialise, .keep in .fs/ leaf dirs, [env] renamed fields and $SANDBOX, TOML """ processes escapes), the fixtures-are-golden-data warning, and the prettier/CommonMark trap. Also correct four stale PLAN_16 milestone references to PLAN_19. PLAN_16 is the config plan; PLAN_19 is milestones. These were cascade-renumber leftovers -- §13 already pointed at PLAN_19 -- and would have sent a reader to the wrong document.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded spec-sheet documentation and validation tooling, typed refusal handling through a procedural macro, environment-scoped ChangesSpec contracts and workspace setup
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR advances PLAN_07 by introducing spec-sheet authoring infrastructure, a sheet/corpus cross-reference checker, and compile-time enforcement of refusal classifications, plus the first batch of Tier-1 builtin spec sheets (with corresponding spec-corpus cases) to start turning the bash surface area into explicit, test-first acceptance criteria.
Changes:
- Add
cargo xtask check-specsto validate spec-sheet ↔ corpus invariants, and document the spec-sheet authoring workflow/template underDocuments/specs/. - Add
fredshell-spec-macros+refuse!proc-macro and a typedfredshell_core::spec::Refusalso refusal behavior is validated at compile time (no fd writes from core). - Draft batch-1 sheets (
cd,true,false,:,echo) and add/refresh the corresponding spec-corpus cases/fixtures; stabilize pinned-bash diagnostics by setting$0during recording; updatecdto operate onExecEnvCWD (not process-global CWD).
Reviewed changes
Copilot reviewed 107 out of 114 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/src/spec/record.rs | Stabilize pinned-bash diagnostics by setting $0 when recording; add regression test for nix-store path leakage in .stderr. |
| xtask/src/main.rs | Add check-specs xtask subcommand wiring. |
| xtask/src/check_specs.rs | New spec-sheet ↔ corpus cross-reference checker (PLAN_07 08.4) with unit tests. |
| tests/spec/job_control/background_wait.stderr | Refresh recorded stderr to use stable bash: prefix (no store path). |
| tests/spec/builtins_tier1/true_no_output.stdout | New corpus fixture for true sheet. |
| tests/spec/builtins_tier1/true_no_output.case.toml | New corpus case for true sheet. |
| tests/spec/builtins_tier1/true_ignores_help.stdout | New corpus fixture for true sheet. |
| tests/spec/builtins_tier1/true_ignores_help.case.toml | New corpus case for true sheet. |
| tests/spec/builtins_tier1/true_ignores_args.stdout | New corpus fixture for true sheet. |
| tests/spec/builtins_tier1/true_ignores_args.case.toml | New corpus case for true sheet. |
| tests/spec/builtins_tier1/true_exit_zero.stdout | New corpus fixture for true sheet. |
| tests/spec/builtins_tier1/true_exit_zero.case.toml | New corpus case for true sheet. |
| tests/spec/builtins_tier1/false_no_output.stdout | New corpus fixture for false sheet. |
| tests/spec/builtins_tier1/false_no_output.case.toml | New corpus case for false sheet. |
| tests/spec/builtins_tier1/false_ignores_help.stdout | New corpus fixture for false sheet. |
| tests/spec/builtins_tier1/false_ignores_help.case.toml | New corpus case for false sheet. |
| tests/spec/builtins_tier1/false_ignores_args.stdout | New corpus fixture for false sheet. |
| tests/spec/builtins_tier1/false_ignores_args.case.toml | New corpus case for false sheet. |
| tests/spec/builtins_tier1/false_exit_one.stdout | New corpus fixture for false sheet. |
| tests/spec/builtins_tier1/false_exit_one.case.toml | New corpus case for false sheet. |
| tests/spec/builtins_tier1/exit_nonnumeric_arg.stderr | Refresh recorded stderr to use stable bash: prefix (no store path). |
| tests/spec/builtins_tier1/echo_n_flag.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_n_flag.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_literal_dashes.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_literal_dashes.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_invalid_bundle.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_invalid_bundle.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_empty.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_empty.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_e_suppress.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_e_suppress.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_e_octal.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_e_octal.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_e_hex.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_e_hex.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_e_flag.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_e_flag.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_e_escapes.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_e_escapes.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_default_literal.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_default_literal.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_bundled_flags.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_bundled_flags.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/echo_basic.stdout | New corpus fixture for echo sheet. |
| tests/spec/builtins_tier1/echo_basic.case.toml | New corpus case for echo sheet. |
| tests/spec/builtins_tier1/colon_truncate.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_truncate.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_ignores_args.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_ignores_args.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_exit_zero.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_exit_zero.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_cmd_subst.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_cmd_subst.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_assign_scope.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_assign_scope.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_arg_assign.stdout | New corpus fixture for : (colon) sheet. |
| tests/spec/builtins_tier1/colon_arg_assign.case.toml | New corpus case for : (colon) sheet. |
| tests/spec/builtins_tier1/cd_too_many_args.stderr | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_too_many_args.exit | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_too_many_args.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_to_dir.stdout | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_to_dir.fs/sub/.keep | New .fs/ skeleton content for cd corpus case. |
| tests/spec/builtins_tier1/cd_to_dir.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_parent.stdout | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_parent.fs/x/y/.keep | New .fs/ skeleton content for cd corpus case. |
| tests/spec/builtins_tier1/cd_parent.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_nonexistent.stderr | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_nonexistent.exit | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_nonexistent.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_no_args_home.stdout | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_no_args_home.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_dash_oldpwd.stdout | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_dash_oldpwd.fs/b/.keep | New .fs/ skeleton content for cd corpus case. |
| tests/spec/builtins_tier1/cd_dash_oldpwd.fs/a/.keep | New .fs/ skeleton content for cd corpus case. |
| tests/spec/builtins_tier1/cd_dash_oldpwd.case.toml | New corpus case for cd sheet. |
| tests/spec/builtins_tier1/cd_cdpath.stdout | New corpus fixture for cd sheet. |
| tests/spec/builtins_tier1/cd_cdpath.fs/base/target/.keep | New .fs/ skeleton content for cd corpus case. |
| tests/spec/builtins_tier1/cd_cdpath.case.toml | New corpus case for cd sheet. |
| plan.md | Update master plan status/log to reflect PLAN_07 progress and handoff notes. |
| Documents/specs/README.md | New operator README describing sheet workflow and corpus authoring constraints. |
| Documents/specs/builtins/true.md | New true spec sheet (batch-1). |
| Documents/specs/builtins/false.md | New false spec sheet (batch-1). |
| Documents/specs/builtins/echo.md | New echo spec sheet (batch-1). |
| Documents/specs/builtins/colon.md | New : spec sheet (as colon.md) including optional §7. |
| Documents/specs/builtins/cd.md | New cd spec sheet (batch-1). |
| Documents/specs/_TEMPLATE.md | Canonical spec-sheet template. |
| Documents/PLAN_07_spec_drafting.md | Update PLAN_07 methodology/status/log, add checklist/cleanup registry and milestone reference corrections. |
| crates/fredshell-spec-macros/tests/ui/wrong_classification.stderr | New trybuild expected stderr fixture. |
| crates/fredshell-spec-macros/tests/ui/wrong_classification.rs | New trybuild compile-fail test case. |
| crates/fredshell-spec-macros/tests/ui/wontfix_on_defer_row.stderr | New trybuild expected stderr fixture. |
| crates/fredshell-spec-macros/tests/ui/wontfix_on_defer_row.rs | New trybuild compile-fail test case. |
| crates/fredshell-spec-macros/tests/ui/missing_sheet.stderr | New trybuild expected stderr fixture. |
| crates/fredshell-spec-macros/tests/ui/missing_sheet.rs | New trybuild compile-fail test case. |
| crates/fredshell-spec-macros/tests/ui/missing_row.stderr | New trybuild expected stderr fixture. |
| crates/fredshell-spec-macros/tests/ui/missing_row.rs | New trybuild compile-fail test case. |
| crates/fredshell-spec-macros/tests/ui/bad_form.stderr | New trybuild expected stderr fixture. |
| crates/fredshell-spec-macros/tests/ui/bad_form.rs | New trybuild compile-fail test case. |
| crates/fredshell-spec-macros/tests/ui.rs | New trybuild test harness. |
| crates/fredshell-spec-macros/tests/specs/builtins/cd.md | Fixture sheet used by proc-macro tests. |
| crates/fredshell-spec-macros/tests/expansion.rs | Integration tests for successful refuse! expansions. |
| crates/fredshell-spec-macros/src/sheet.rs | Line-oriented §3 support-matrix parser for proc-macro. |
| crates/fredshell-spec-macros/src/lib.rs | New refuse! proc-macro implementation. |
| crates/fredshell-spec-macros/Cargo.toml | New proc-macro crate manifest. |
| crates/fredshell-spec-macros/build.rs | Build script to expose fixture spec root to tests. |
| crates/fredshell-core/src/spec.rs | New typed Refusal / RefusalKind and formatting per PLAN_07. |
| crates/fredshell-core/src/lib.rs | Re-export spec types and refuse! macro. |
| crates/fredshell-core/src/exec/mod.rs | Pass ExecEnv into builtins; ensure spawned external commands inherit env.cwd; update tests. |
| crates/fredshell-core/src/builtins.rs | Refactor builtins to operate on ExecEnv (no process-global cwd/env mutation); implement cd against env.cwd/env.stderr. |
| crates/fredshell-core/Cargo.toml | Add dependency on fredshell-spec-macros. |
| COMPAT.md | Regenerated compat status to include new corpus cases. |
| Cargo.toml | Add fredshell-spec-macros workspace member and proc-macro dependencies. |
| Cargo.lock | Lockfile updates for new crate and dependencies. |
| AGENTS.md | Update crate table + dependency-direction notes for fredshell-spec-macros. |
| .gitignore | Anchor /target/ to repo root to avoid ignoring fixture directories named target/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+297
to
+309
| for row in &sheet.rows { | ||
| // Check 3: no unclassified rows. | ||
| if row.classification == Classification::Unclassified { | ||
| errors.push(SheetError::UnclassifiedRow { | ||
| row: row.number.clone(), | ||
| }); | ||
| } | ||
| // Check 5: every defer row needs a §6 workaround paragraph. | ||
| if matches!(row.classification, Classification::Defer(_)) && !sheet.has_deferred_body { | ||
| errors.push(SheetError::MissingWorkaround { | ||
| row: row.number.clone(), | ||
| }); | ||
| } |
Comment on lines
+48
to
+53
| /// The behaviour will be implemented after a milestone | ||
| /// (`PLAN_07` §5.3). | ||
| Defer { | ||
| /// The `PLAN_16` milestone number after which the behaviour | ||
| /// lands (the `N` in `defer:N`). | ||
| milestone: String, |
Picks up the dependency housekeeping that landed on main while this branch was in progress (lock file maintenance, action digests, nixpkgs reference digest). Re-resolve Cargo.lock after the merge. main locks proc-macro2 1.0.107, quote 1.0.47 and syn 2.0.119, but this branch makes all three *direct* dependencies of the new fredshell-spec-macros crate, so cargo's MSRV-aware resolver clamps them to the newest versions compatible with the workspace's rust-version of 1.95.0: 1.0.106, 1.0.46 and 2.0.118. Without this the merge commit CI tests is inconsistent -- cargo re-clamps during the clippy pre-commit hook, the hook reports "files were modified by this hook", and the Nix devShell checks job fails even though nothing is wrong with the code. Committing the clamped lock makes the resolution stable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (10)
xtask/src/check_specs.rs (1)
370-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the corpus root like
compat::rundoes.
runchecksspecs_rootbut notCORPUS_ROOT. If a user runs the command outside the workspace root,enumerate_corpus_casesfails with a bareread_dir tests/spec: ...message.xtask/src/compat.rs(Lines 148-153) bails with an actionable message for the same condition. Match that behavior.♻️ Proposed guard
let specs_root = Path::new(SPECS_ROOT); if !specs_root.is_dir() { bail!( "check-specs: spec root {} is not a directory", specs_root.display() ); } + + let corpus_root = Path::new(CORPUS_ROOT); + if !corpus_root.is_dir() { + bail!( + "check-specs: corpus root `{}` not found. Run from the workspace root.", + corpus_root.display() + ); + }🤖 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 `@xtask/src/check_specs.rs` around lines 370 - 411, Validate the corpus root before processing corpus cases, matching the actionable directory check used by compat::run. In the flow around enumerate_corpus_cases, ensure Path::new(CORPUS_ROOT) is a directory and bail with a contextual check-specs error if it is not, before calling enumerate_corpus_cases.tests/spec/builtins_tier1/cd_no_args_home.case.toml (1)
1-10: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEnsure the HOME test requires a directory change.
The fixture sets
HOMEto$SANDBOX. If the runner starts in$SANDBOX,PWDalready equalsHOME; the assertion can pass even whencddoes nothing or fails. Use a distinct existing home directory, or verify that the runner starts outside$SANDBOX.As per coding guidelines, tests must be hermetic, order-independent, focused on observable behavior, and cover both success and failure cases where applicable.
🤖 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/spec/builtins_tier1/cd_no_args_home.case.toml` around lines 1 - 10, Update the cd HOME fixture so the test starts with PWD distinct from HOME, while keeping HOME set to an existing directory under the sandbox. Strengthen the assertion in the script to verify that cd actually changes into HOME rather than passing when the runner already starts there.Source: Coding guidelines
tests/spec/builtins_tier1/cd_cdpath.case.toml (1)
1-10: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the CDPATH assertion distinguish the selected directory.
The script prints only
${PWD##*/}. A localtargetand a CDPATH-resolvedtargetboth producetarget. Add supported fixture setup that makes the local target absent and the CDPATH target present, or assert a distinguishing path or marker.As per coding guidelines, tests must be hermetic, order-independent, focused on observable behavior, and cover both success and failure cases where applicable.
🤖 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/spec/builtins_tier1/cd_cdpath.case.toml` around lines 1 - 10, Update the test fixture and script for the CDPATH case so the working directory lacks a local target while the CDPATH directory contains the target, then assert a distinguishing path or marker rather than only the basename. Keep the test hermetic and order-independent, and add coverage for the unresolved-target failure behavior if the surrounding test conventions support it.Source: Coding guidelines
tests/spec/builtins_tier1/cd_to_dir.case.toml (1)
3-4: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
PWDin the native executor.
run_cdupdates onlyExecEnv.cwd. The strict runner refuses this deferred case, and the fallback shell does not useExecEnv.env, so the case does not test native${PWD##*/}expansion. WhenPLAN_12enables it, updatePWDafter successful nativecdand assert the expansion.🤖 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/spec/builtins_tier1/cd_to_dir.case.toml` around lines 3 - 4, Update the native executor’s run_cd path so a successful directory change synchronizes the PWD environment value with ExecEnv.cwd before expansion. Ensure the PLAN_12-enabled test case explicitly asserts the native ${PWD##*/} result, while preserving existing behavior for failed cd operations and fallback execution.crates/fredshell-core/src/builtins.rs (2)
78-86: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDiagnostic write errors are discarded.
Both
writeln!calls uselet _ =, so a failingenv.stderrwrite is silent.spawn_via_shincrates/fredshell-core/src/exec/mod.rslines 241-243 maps the same failure toExecError::HostIo. The two output paths therefore behave differently when the host's stderr sink fails. Consider returning the error fromrun_cdsotry_runcan propagate it, which also gives the reserved error slot documented at lines 43-48 a first user.Also applies to: 104-107
🤖 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 `@crates/fredshell-core/src/builtins.rs` around lines 78 - 86, Update run_cd’s diagnostic output paths, including the HOME-not-set branch and the other writeln! call, to propagate env.stderr write failures instead of discarding them with let _. Return the appropriate error through run_cd so try_run can propagate it consistently with spawn_via_sh’s HostIo handling and use the existing reserved error slot.
99-109: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftPreserve logical paths when implementing
cd -L.Rows 3.8 and 3.9 are currently
defer:3, notsupport, so this is not a current corpus mismatch. When PLAN_12 enables these rows, retain the logical path by default and use canonicalization only forcd -P.🤖 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 `@crates/fredshell-core/src/builtins.rs` around lines 99 - 109, Update the cd implementation around canonicalize_existing_dir and the option handling so the default cd -L behavior preserves the user’s logical path, while canonicalizing the directory only when cd -P is explicitly requested. Keep directory validation and error handling intact, and assign env.cwd from the appropriate logical or canonical resolved path.crates/fredshell-core/src/exec/mod.rs (1)
432-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed temp directory names weaken hermeticity.
Both tests build a fixed path under the shared system temp directory:
fredshell-cd-env-cwd-testandfredshell-cd-relative-test.lock()serialises only within one process. Two concurrent test processes, or a leftover directory from an aborted run, share the same path.cd_resolves_relative_target_against_env_cwdalso callsfs::remove_dir_all, which would delete another run's tree. Add a per-process unique component, for examplestd::process::id(), to each name.As per coding guidelines: "Tests must be hermetic, order-independent, focused on observable behavior".
Also applies to: 465-467
🤖 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 `@crates/fredshell-core/src/exec/mod.rs` around lines 432 - 433, Update the temporary-directory setup in the tests around cd_resolves_relative_target_against_env_cwd and the related test to include a per-process unique component, such as std::process::id(), in both fixed names. Keep the existing cleanup and test behavior unchanged while ensuring concurrent processes and leftover directories cannot share or remove one another’s paths.Source: Coding guidelines
crates/fredshell-core/src/spec.rs (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the placeholder
docs.rslink.Line 20 links
refuse!tohttps://docs.rs, which is the site root and not the macro page. The macro is re-exported from this crate atcrates/fredshell-core/src/lib.rsline 38, so an intra-doc link resolves and stays correct after renames.♻️ Proposed doc link fix
//! [`Refusal`] values are normally constructed by the -//! [`refuse!`](https://docs.rs) macro from the `fredshell-spec-macros` -//! crate, which validates the sheet id, row number, and +//! [`refuse!`](crate::refuse) macro from the `fredshell-spec-macros` +//! crate, which validates the sheet id, row number, and //! classification against the on-disk spec sheet at compile time. The🤖 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 `@crates/fredshell-core/src/spec.rs` around lines 19 - 24, Replace the placeholder external URL in the `refuse!` intra-doc reference within the module documentation with a link to the macro re-exported by this crate, using the existing re-export in `lib.rs` so documentation resolution remains stable.crates/fredshell-spec-macros/src/sheet.rs (1)
78-96: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty cells shift the column indices.
filter(|s| !s.is_empty())removes blank cells before indexing. If a §3 row has an empty Behaviour cell,cells[1]holds the Classification andcells[2]holds the Corpus.Classification::parsethen reads the Corpus cell and returnsOther, so a realwontfixrow is reported as "classifiedbuiltins_tier1/….case.toml". The failure surfaces as a confusing compile error rather than a wrong value, so this is a robustness gap and not a live defect. Keep positional cells and trim the leading and trailing pipes instead.♻️ Proposed positional parsing
- let cells: Vec<&str> = line - .split('|') - .map(str::trim) - .filter(|s| !s.is_empty()) - .collect(); + // Trim the surrounding pipes, then split so blank cells keep + // their position instead of collapsing the column indices. + let cells: Vec<&str> = line + .trim_start_matches('|') + .trim_end_matches('|') + .split('|') + .map(str::trim) + .collect();Note:
xtask/src/check_specs.rsuses the same filtering pattern, so consider aligning both.🤖 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 `@crates/fredshell-spec-macros/src/sheet.rs` around lines 78 - 96, Update the row parsing logic in the visible sheet parser to preserve empty positional cells: trim only the leading and trailing pipe delimiters, then split and trim each field without filtering empty strings before indexing. Keep the existing column mapping for number, summary, and classification, and align the corresponding parsing logic in check_specs.rs if it uses the same filtering pattern.crates/fredshell-spec-macros/src/lib.rs (1)
71-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject trailing tokens in both
refuse!variants.
parse_macro_input!does not require aParseimplementation to consume all tokens. Addinput.is_empty()checks after parsing the fixedwontfixarguments and after thedeferargument loop.🤖 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 `@crates/fredshell-spec-macros/src/lib.rs` around lines 71 - 81, Update RefuseInput::parse to reject unconsumed tokens by checking input.is_empty() after parsing the fixed wontfix arguments and again after completing the defer argument loop; return a parse error when either variant has trailing tokens while preserving the existing parsing behavior for valid input.
🤖 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 `@AGENTS.md`:
- Around line 122-128: Update the fredshell-core crate root in lib.rs to declare
the self-crate alias fredshell_core before any in-core use of the refuse! macro,
so its generated ::fredshell_core::Refusal path resolves correctly.
In `@crates/fredshell-core/src/exec/mod.rs`:
- Around line 432-458: Move the fs::remove_dir(&tmp) cleanup in the cd/pwd test
until after the assertions that canonicalize pwd_path and compare it with
canonical_tmp. Keep cleanup guaranteed after validation, such as by placing it
after the assertions or using an appropriate scope guard.
In `@crates/fredshell-core/src/spec.rs`:
- Around line 50-53: Update the doc comment for the Defer variant’s milestone
field to reference PLAN_19 instead of PLAN_16, keeping the existing explanation
of the defer:N milestone unchanged.
In `@crates/fredshell-spec-macros/src/lib.rs`:
- Around line 291-317: Update find_specs_root and the production expansion path
to register every resolved sheet under the selected specs root as a compile-time
dependency, including files beneath builtins and features. Emit equivalent
tracking for each sheet (for example, include_bytes!-based dependencies) while
preserving FREDSHELL_SPECS_ROOT overrides and the existing directory resolution
behavior.
In `@crates/fredshell-spec-macros/tests/ui.rs`:
- Around line 23-32: Remove the fixture_root declaration and the unsafe
std::env::set_var call from the trybuild test setup in ui.rs; rely on the
FREDSHELL_SPECS_ROOT value emitted by build.rs and inherited by trybuild.
In `@crates/fredshell-spec-macros/tests/ui/bad_form.rs`:
- Around line 1-6: Add the repository-mandated four-line header and blank line
to crates/fredshell-spec-macros/tests/ui/bad_form.rs and
crates/fredshell-spec-macros/tests/ui/missing_row.rs, then regenerate
bad_form.stderr and the corresponding missing_row trybuild stderr snapshot so
diagnostics reflect the updated fixtures.
In `@crates/fredshell-spec-macros/tests/ui/missing_sheet.rs`:
- Line 1: Prepend the exact four-line 2026 Fredshell MIT copyright header before
the first use statement in
crates/fredshell-spec-macros/tests/ui/missing_sheet.rs,
crates/fredshell-spec-macros/tests/ui/wontfix_on_defer_row.rs, and
crates/fredshell-spec-macros/tests/ui/wrong_classification.rs; make no other
changes.
In `@Documents/PLAN_07_spec_drafting.md`:
- Around line 87-88: Update the Phase B status line in
Documents/PLAN_07_spec_drafting.md to identify only the remaining batch-1 sheets
as pending, while preserving the completed status for cd, true, false, :, and
echo documented in §10.1 and §14.
In `@Documents/specs/builtins/cd.md`:
- Around line 19-21: Update the cd specification’s CDPATH behavior in the lines
19–21 description, row 3.7, and the corpus case: for non-absolute dir values,
search the colon-separated CDPATH entries before the unqualified relative path,
with empty entries representing the current directory. Add validation targets in
both relevant locations so the corpus case fails when precedence is incorrect.
In `@Documents/specs/builtins/echo.md`:
- Around line 114-116: Update the POSIX behavior description in the echo
documentation to state that a first operand of -n has an implementation-defined
result, including that XSI implementations treat it as a string. Remove the
assertion that every strict POSIX implementation prints -n, while retaining the
separate Bash option behavior.
In `@Documents/specs/README.md`:
- Around line 35-36: Update the deferred-milestone reference from PLAN_16 to
PLAN_19 in Documents/specs/README.md lines 35-36 and
Documents/specs/_TEMPLATE.md lines 96-97, keeping the surrounding authoring
guidance unchanged.
- Around line 66-70: Update the filename rules in the README to document the
punctuation-named builtin exception: map the invocation names “:”, “.”, and “[”
to colon.md, dot.md, and bracket.md respectively, while retaining the existing
invocation-name rule for other builtins.
In `@tests/spec/builtins_tier1/echo_empty.case.toml`:
- Around line 3-4: Update the echo_empty test command to report the file’s byte
count using wc -c rather than command substitution, which strips the trailing
newline; ensure the expected output in echo_empty.stdout is bytes=[1].
In `@xtask/src/check_specs.rs`:
- Around line 236-252: Update the row parsing logic around cells collection to
remove only the empty fields created by the surrounding pipes while preserving
empty interior columns. Keep the fixed column positions used by is_row_number,
Classification::parse, and strip_code_span so blank Behaviour or Corpus cells
are not shifted, and add coverage verifying a blank Corpus cell parses into a
support row with an empty corpus field.
In `@xtask/src/spec/record.rs`:
- Around line 397-434: The regression test
recorded_diagnostics_use_stable_argv0_not_store_path must reuse the shared Bash
invocation logic used by run, rather than constructing Command and passing
REFERENCE_ARGV0 independently. Extract or expose the common invocation path and
use it in both places so the test fails if run loses the stable argv0 argument;
replace the silent FREDSHELL_REFERENCE_BASH-unset return with an explicit
requirement that the pinned-Bash environment is present.
---
Nitpick comments:
In `@crates/fredshell-core/src/builtins.rs`:
- Around line 78-86: Update run_cd’s diagnostic output paths, including the
HOME-not-set branch and the other writeln! call, to propagate env.stderr write
failures instead of discarding them with let _. Return the appropriate error
through run_cd so try_run can propagate it consistently with spawn_via_sh’s
HostIo handling and use the existing reserved error slot.
- Around line 99-109: Update the cd implementation around
canonicalize_existing_dir and the option handling so the default cd -L behavior
preserves the user’s logical path, while canonicalizing the directory only when
cd -P is explicitly requested. Keep directory validation and error handling
intact, and assign env.cwd from the appropriate logical or canonical resolved
path.
In `@crates/fredshell-core/src/exec/mod.rs`:
- Around line 432-433: Update the temporary-directory setup in the tests around
cd_resolves_relative_target_against_env_cwd and the related test to include a
per-process unique component, such as std::process::id(), in both fixed names.
Keep the existing cleanup and test behavior unchanged while ensuring concurrent
processes and leftover directories cannot share or remove one another’s paths.
In `@crates/fredshell-core/src/spec.rs`:
- Around line 19-24: Replace the placeholder external URL in the `refuse!`
intra-doc reference within the module documentation with a link to the macro
re-exported by this crate, using the existing re-export in `lib.rs` so
documentation resolution remains stable.
In `@crates/fredshell-spec-macros/src/lib.rs`:
- Around line 71-81: Update RefuseInput::parse to reject unconsumed tokens by
checking input.is_empty() after parsing the fixed wontfix arguments and again
after completing the defer argument loop; return a parse error when either
variant has trailing tokens while preserving the existing parsing behavior for
valid input.
In `@crates/fredshell-spec-macros/src/sheet.rs`:
- Around line 78-96: Update the row parsing logic in the visible sheet parser to
preserve empty positional cells: trim only the leading and trailing pipe
delimiters, then split and trim each field without filtering empty strings
before indexing. Keep the existing column mapping for number, summary, and
classification, and align the corresponding parsing logic in check_specs.rs if
it uses the same filtering pattern.
In `@tests/spec/builtins_tier1/cd_cdpath.case.toml`:
- Around line 1-10: Update the test fixture and script for the CDPATH case so
the working directory lacks a local target while the CDPATH directory contains
the target, then assert a distinguishing path or marker rather than only the
basename. Keep the test hermetic and order-independent, and add coverage for the
unresolved-target failure behavior if the surrounding test conventions support
it.
In `@tests/spec/builtins_tier1/cd_no_args_home.case.toml`:
- Around line 1-10: Update the cd HOME fixture so the test starts with PWD
distinct from HOME, while keeping HOME set to an existing directory under the
sandbox. Strengthen the assertion in the script to verify that cd actually
changes into HOME rather than passing when the runner already starts there.
In `@tests/spec/builtins_tier1/cd_to_dir.case.toml`:
- Around line 3-4: Update the native executor’s run_cd path so a successful
directory change synchronizes the PWD environment value with ExecEnv.cwd before
expansion. Ensure the PLAN_12-enabled test case explicitly asserts the native
${PWD##*/} result, while preserving existing behavior for failed cd operations
and fallback execution.
In `@xtask/src/check_specs.rs`:
- Around line 370-411: Validate the corpus root before processing corpus cases,
matching the actionable directory check used by compat::run. In the flow around
enumerate_corpus_cases, ensure Path::new(CORPUS_ROOT) is a directory and bail
with a contextual check-specs error if it is not, before calling
enumerate_corpus_cases.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 9194a3e5-3d17-4b6c-8a07-323b3c45960e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (113)
.gitignoreAGENTS.mdCOMPAT.mdCargo.tomlDocuments/PLAN_07_spec_drafting.mdDocuments/specs/README.mdDocuments/specs/_TEMPLATE.mdDocuments/specs/builtins/cd.mdDocuments/specs/builtins/colon.mdDocuments/specs/builtins/echo.mdDocuments/specs/builtins/false.mdDocuments/specs/builtins/true.mdcrates/fredshell-core/Cargo.tomlcrates/fredshell-core/src/builtins.rscrates/fredshell-core/src/exec/mod.rscrates/fredshell-core/src/lib.rscrates/fredshell-core/src/spec.rscrates/fredshell-spec-macros/Cargo.tomlcrates/fredshell-spec-macros/build.rscrates/fredshell-spec-macros/src/lib.rscrates/fredshell-spec-macros/src/sheet.rscrates/fredshell-spec-macros/tests/expansion.rscrates/fredshell-spec-macros/tests/specs/builtins/cd.mdcrates/fredshell-spec-macros/tests/ui.rscrates/fredshell-spec-macros/tests/ui/bad_form.rscrates/fredshell-spec-macros/tests/ui/bad_form.stderrcrates/fredshell-spec-macros/tests/ui/missing_row.rscrates/fredshell-spec-macros/tests/ui/missing_row.stderrcrates/fredshell-spec-macros/tests/ui/missing_sheet.rscrates/fredshell-spec-macros/tests/ui/missing_sheet.stderrcrates/fredshell-spec-macros/tests/ui/wontfix_on_defer_row.rscrates/fredshell-spec-macros/tests/ui/wontfix_on_defer_row.stderrcrates/fredshell-spec-macros/tests/ui/wrong_classification.rscrates/fredshell-spec-macros/tests/ui/wrong_classification.stderrplan.mdtests/spec/builtins_tier1/cd_cdpath.case.tomltests/spec/builtins_tier1/cd_cdpath.fs/base/target/.keeptests/spec/builtins_tier1/cd_cdpath.stdouttests/spec/builtins_tier1/cd_dash_oldpwd.case.tomltests/spec/builtins_tier1/cd_dash_oldpwd.fs/a/.keeptests/spec/builtins_tier1/cd_dash_oldpwd.fs/b/.keeptests/spec/builtins_tier1/cd_dash_oldpwd.stdouttests/spec/builtins_tier1/cd_no_args_home.case.tomltests/spec/builtins_tier1/cd_no_args_home.stdouttests/spec/builtins_tier1/cd_nonexistent.case.tomltests/spec/builtins_tier1/cd_nonexistent.exittests/spec/builtins_tier1/cd_nonexistent.stderrtests/spec/builtins_tier1/cd_parent.case.tomltests/spec/builtins_tier1/cd_parent.fs/x/y/.keeptests/spec/builtins_tier1/cd_parent.stdouttests/spec/builtins_tier1/cd_to_dir.case.tomltests/spec/builtins_tier1/cd_to_dir.fs/sub/.keeptests/spec/builtins_tier1/cd_to_dir.stdouttests/spec/builtins_tier1/cd_too_many_args.case.tomltests/spec/builtins_tier1/cd_too_many_args.exittests/spec/builtins_tier1/cd_too_many_args.stderrtests/spec/builtins_tier1/colon_arg_assign.case.tomltests/spec/builtins_tier1/colon_arg_assign.stdouttests/spec/builtins_tier1/colon_assign_scope.case.tomltests/spec/builtins_tier1/colon_assign_scope.stdouttests/spec/builtins_tier1/colon_cmd_subst.case.tomltests/spec/builtins_tier1/colon_cmd_subst.stdouttests/spec/builtins_tier1/colon_exit_zero.case.tomltests/spec/builtins_tier1/colon_exit_zero.stdouttests/spec/builtins_tier1/colon_ignores_args.case.tomltests/spec/builtins_tier1/colon_ignores_args.stdouttests/spec/builtins_tier1/colon_truncate.case.tomltests/spec/builtins_tier1/colon_truncate.stdouttests/spec/builtins_tier1/echo_basic.case.tomltests/spec/builtins_tier1/echo_basic.stdouttests/spec/builtins_tier1/echo_bundled_flags.case.tomltests/spec/builtins_tier1/echo_bundled_flags.stdouttests/spec/builtins_tier1/echo_default_literal.case.tomltests/spec/builtins_tier1/echo_default_literal.stdouttests/spec/builtins_tier1/echo_e_escapes.case.tomltests/spec/builtins_tier1/echo_e_escapes.stdouttests/spec/builtins_tier1/echo_e_flag.case.tomltests/spec/builtins_tier1/echo_e_flag.stdouttests/spec/builtins_tier1/echo_e_hex.case.tomltests/spec/builtins_tier1/echo_e_hex.stdouttests/spec/builtins_tier1/echo_e_octal.case.tomltests/spec/builtins_tier1/echo_e_octal.stdouttests/spec/builtins_tier1/echo_e_suppress.case.tomltests/spec/builtins_tier1/echo_e_suppress.stdouttests/spec/builtins_tier1/echo_empty.case.tomltests/spec/builtins_tier1/echo_empty.stdouttests/spec/builtins_tier1/echo_invalid_bundle.case.tomltests/spec/builtins_tier1/echo_invalid_bundle.stdouttests/spec/builtins_tier1/echo_literal_dashes.case.tomltests/spec/builtins_tier1/echo_literal_dashes.stdouttests/spec/builtins_tier1/echo_n_flag.case.tomltests/spec/builtins_tier1/echo_n_flag.stdouttests/spec/builtins_tier1/exit_nonnumeric_arg.stderrtests/spec/builtins_tier1/false_exit_one.case.tomltests/spec/builtins_tier1/false_exit_one.stdouttests/spec/builtins_tier1/false_ignores_args.case.tomltests/spec/builtins_tier1/false_ignores_args.stdouttests/spec/builtins_tier1/false_ignores_help.case.tomltests/spec/builtins_tier1/false_ignores_help.stdouttests/spec/builtins_tier1/false_no_output.case.tomltests/spec/builtins_tier1/false_no_output.stdouttests/spec/builtins_tier1/true_exit_zero.case.tomltests/spec/builtins_tier1/true_exit_zero.stdouttests/spec/builtins_tier1/true_ignores_args.case.tomltests/spec/builtins_tier1/true_ignores_args.stdouttests/spec/builtins_tier1/true_ignores_help.case.tomltests/spec/builtins_tier1/true_ignores_help.stdouttests/spec/builtins_tier1/true_no_output.case.tomltests/spec/builtins_tier1/true_no_output.stdouttests/spec/job_control/background_wait.stderrxtask/src/check_specs.rsxtask/src/main.rsxtask/src/spec/record.rs
Comment on lines
+3
to
+4
| echo > out | ||
| echo "bytes=[$(<out)]" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- case ---'
cat -n tests/spec/builtins_tier1/echo_empty.case.toml
printf '%s\n' '--- expected output ---'
cat -n tests/spec/builtins_tier1/echo_empty.stdout
printf '%s\n' '--- related cases and harness references ---'
rg -n -C 3 'echo_empty|bytes=|wc -c|command substitution|case\.toml' tests/spec tests 2>/dev/null | head -240Repository: fredsystems/fredshell
Length of output: 5130
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cd "$tmpdir"
bash -c '
: > zero
printf "zero_subst=[%s]\n" "$(<zero)"
printf "zero_count=[%d]\n" "$(wc -c < zero)"
echo > one
printf "one_subst=[%s]\n" "$(<one)"
printf "one_count=[%d]\n" "$(wc -c < one)"
od -An -t u1 one
'
printf '%s\n' '--- spec execution rules ---'
sed -n '1,120p' "$OLDPWD/tests/spec/README.md"Repository: fredsystems/fredshell
Length of output: 255
Assert the newline byte, not stripped command output.
$(<out) removes trailing newlines, so bytes=[] also passes when out is empty. Use printf 'bytes=[%d]\n' "$(wc -c < out)" and update tests/spec/builtins_tier1/echo_empty.stdout to bytes=[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 `@tests/spec/builtins_tier1/echo_empty.case.toml` around lines 3 - 4, Update
the echo_empty test command to report the file’s byte count using wc -c rather
than command substitution, which strips the trailing newline; ensure the
expected output in echo_empty.stdout is bytes=[1].
Sources: Coding guidelines, MCP tools
Seventeen review comments, all addressed. Combined into one commit
because the corpus, COMPAT.md and sheet changes are interdependent:
splitting them leaves intermediate commits where
`xtask compat --check-readme` fails, which the pre-commit hook rejects.
Sheet correctness
- cd: §2 and row 3.7 had $CDPATH precedence backwards. Verified against
bash 5.3p9: $CDPATH is searched *before* the plain relative
interpretation, so a $CDPATH entry shadows ./dir. An empty entry, and
an explicit `.`, denote the current directory; a $CDPATH miss falls
back to the relative path; an absolute dir bypasses $CDPATH. The
cd_cdpath case now stages competing ./target and $CDPATH/target
directories, so it discriminates -- it previously passed under either
precedence. New row 3.13 plus cd_cdpath_empty_entry cover the
empty-entry rule, and §4 gains a quirk on the shadowing surprise.
- echo: §7 claimed a strict POSIX implementation prints `-n`. POSIX
makes the result implementation-defined when the first operand is -n.
- echo_empty asserted bytes=[], which $(<file) made vacuous: it passed
whether echo wrote a newline or nothing. It now probes `[ -s out ]`
and counts lines with a builtin read loop. wc -c would read better
but the recorder clears the environment, so no coreutils are on PATH.
check-specs (08.4)
- The row parser dropped all empty cells, not just pipe padding, so a
blank Corpus cell was skipped silently and a blank Behaviour cell
shifted later columns. Strip only the padding.
- Check 5 accepted any non-empty §6 body, letting one paragraph satisfy
every defer row. Require each row number to appear, digit-boundary
aware so 3.13 does not satisfy 3.1. Four new tests.
refuse! (08.5)
- fredshell-core lacked `extern crate self as fredshell_core`, so the
first in-core refuse! would have failed with E0433. Verified, fixed,
and pinned by a test that expands refuse! inside core.
- The expansion now emits `const _: &[u8] = include_bytes!(<sheet>)`,
which makes PLAN_07 §8.2's documented rebuild coupling real. Touching
cd.md now recompiles fredshell-core; previously the sheet was not a
tracked input, so stale validation could ship.
- Removed `unsafe { std::env::set_var }` from tests/ui.rs, which
violated the no-unsafe rule. Probed to confirm cargo already places
the build.rs rustc-env in the test binary's environment and trybuild's
subprocess inherits it, so fixture isolation is preserved.
- Added the mandatory copyright header to all five tests/ui/*.rs
fixtures and regenerated their .stderr snapshots.
Tests
- The cd/env.cwd test removed its temp directory before the
canonicalize that needed it, so the unwrap_or fallback always kept the
raw pwd output and compared an unresolved path against a canonical
one. Derive everything before cleanup and expect the resolution.
- The argv0 regression test built its own Command, so it would have kept
passing after the recorder stopped setting $0. Both paths now share
reference_bash_command; removing the argument from production was
confirmed to fail the test. It no longer skips silently when
FREDSHELL_REFERENCE_BASH is unset.
Docs
- Stale PLAN_16 milestone references corrected to PLAN_19 in
specs/README.md, specs/_TEMPLATE.md and fredshell_core::spec.
- The punctuation-named-builtin filename exception added to the
README layout section.
- The PLAN_07 header status line now names the five landed sheets.
One CodeRabbit rationale was inaccurate and is recorded in PLAN_07 §14
for the next reader: cargo:rustc-env does not reach the rustc that
expands a macro in a consumer crate, so it is the test binary's
environment, not the build script, that carries FREDSHELL_SPECS_ROOT
into trybuild. The conclusion (drop the unsafe) was still correct.
The previous commit made the argv0 regression test panic when FREDSHELL_REFERENCE_BASH is unset, following a review suggestion to stop skipping silently. That broke all four `Check <os> <toolchain>` CI jobs: they run `cargo xtask check` -> `cargo test --workspace` on bare runners that have no nix devshell, so the pinned bash genuinely does not exist there. Only the `Nix devShell checks` job exports the variable. Keep the intent without the false failure. Fail when IN_NIX_SHELL is set but the variable is not, since that means a misconfigured devshell, and otherwise skip with an explicit printed reason rather than a bare return. The devshell CI job still exports the variable, so the regression remains guarded rather than skipped everywhere.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements
PLAN_07(spec-sheet drafting methodology) subtasks 08.1, 08.4, 08.5, and the first half of 08.2 — batch-1 sheets 1–5 of 10.PLAN_07§14 carries a detailed log row per subtask; this is the summary.Tooling (08.1, 08.4, 08.5)
Documents/specs/_TEMPLATE.md(the canonical seven-section sheet template) andDocuments/specs/README.md(layout, classifications, authoring workflow).cargo xtask check-specs, enforcing the five §8.1 invariants: everysupportrow resolves to a loadable corpus case with an acceptedstatus; every corpus case is referenced by exactly one sheet row; no???rows; the mandatory sections present and in order (optional §7 tolerated between §6 and §8); everydefer:Nrow backed by a §6 workaround. 31 unit tests.fredshell-spec-macrosproc-macro crate and therefuse!macro, plus the typedfredshell_core::spec::Refusalvalue.refuse!reads the sheet at compile time and fails the build on a missing row or a classification mismatch. Per ADR 0006 the core returns a typed value rather than writing to a file descriptor. 5trybuildcompile-fail cases prove the guarantee.Batch-1 sheets, 5 of 10 (08.2a–08.2e)
cd-@), 4 defertruefalse:(colon.md)echo33 new corpus cases under
tests/spec/builtins_tier1/, allstatus = "deferred:PLAN_12"and all recorded against the pinned reference bash (5.3p9). Every row was probed against real bash, not recalled — AGENTS.md forbids guessing shell semantics.Remaining batch-1 sheets —
printf,set,shopt,unset,trap— are tracked in the newPLAN_07§10.1 checklist.Two pre-existing bugs found and fixed in their own commits
c1988fc—spec recordbaked bash's/nix/store/…path into.stderrfixtures via$0, making them non-portable across machines and unstable across nixpkgs bumps. Fixed by passing a fixedargv[0].49c94f8— thecdbuiltin mutated the global process CWD and wrote diagnostics witheprintln!fromfredshell-core, violating the ADR 0006 no-fd-writes-from-core invariant. Fixed to useenv.cwd/env.stderr.Handoff documentation (
d3a33c7)Batch 1 is half done, so the last commit makes the docs a runbook rather than a changelog: a §15 cleanup registry, a §10 status column plus §10.1 sheet checklist, the symbol-named-builtin filename rule (
:/./[→colon.md/dot.md/bracket.md), a provisionaldefer:Nmilestone table, and the corpus-authoring constraints inspecs/README.md. It also corrects four stalePLAN_16milestone references toPLAN_19(PLAN_16is config;PLAN_19is milestones — cascade-renumber leftovers).Reviewer notes
cargo xtask check-specsintentionally exits 1 on this branch. Invariant 2 (every corpus case referenced by a sheet) cannot pass until the ~80-sheet inventory exists in 08.6, so it correctly reports the 21 pre-existing unsheeted cases as orphans. It is deliberately not wired intoxtask pcor CI; that wiring lands with 08.6. Documented inPLAN_07§10.1 and the 08.4 log row.08.2e-CU1(PLAN_07§15). Themixed-line-endingpre-commit hook silently rewrites a lone CR inside a recorded fixture and then rejects the commit, so a careless re-stage can commit a wrong golden value. Suggested fix is aflake.nixextraExcludesentry fortests/spec/**/*.{stdout,stderr,exit}. Should land before theprintfsheet.unsafe, no new productionunwrap/expect, no upward crate dependencies.AGENTS.md's crate table was updated in the same commit that addedfredshell-spec-macros.Verification
cargo test --workspace(18 suites, 0 failures) ·cargo clippy --all-targets -- -D warnings·cargo-machete·cargo fmt --all --check·cargo xtask spec lint(54 cases) ·cargo xtask compat(result: ok, no regressions;COMPAT.mdin sync) ·markdownlint-cli2·prettier --check. All pre-commit hooks green on every commit; no--no-verify.Summary by CodeRabbit
New Features
cd, including$HOME, relative paths,CDPATH, and clearer failure handling.check-specscommand to validate specification sheets and test coverage.Documentation
cd,:,echo,false, andtrue, including compatibility details and known limitations.Tests