Skip to content

fix: address PR review comments for opensmith tooling - #8

Open
ludoplex with Copilot wants to merge 3 commits into
feat/opensmith-parity-pr2-parser-frontendfrom
copilot/sub-pr-5-again
Open

fix: address PR review comments for opensmith tooling#8
ludoplex with Copilot wants to merge 3 commits into
feat/opensmith-parity-pr2-parser-frontendfrom
copilot/sub-pr-5-again

Conversation

Copilot AI commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Addresses review feedback from PR #5 on the OpenSmith parser front-end scaffold.

opensmithgen.c

  • Strip trailing -- from comment bodies (was keeping comment -- instead of comment)
  • Add raw_len field to ast_node_t for byte-accurate reconstruction—handles embedded NUL bytes that strlen() would truncate
  • Skip empty final literal segments to avoid polluting AST with zero-length nodes
  • Defensive null check in reconstruct() loop
// Before: body contains delimiter artifacts
"body": "This is a comment --"

// After: clean comment text
"body": "This is a comment"

Shell security/correctness

  • opensmith_parity.sh: Fix shell injection—pass fixture as separate argv instead of sh -c "$ENGINE $fixture"
  • opensmith_frontend_check.sh: Check -x (executable) not -f (exists)
  • opensmith_corpus.sh: Fix nested_zip_prefix grep pattern to handle JSON whitespace ("key": "value" vs "key":"value")

Makefile

  • opensmithgen-ape target: Add cosmocc detection with fallback to ~/.cosmocc/bin/cosmocc and actionable error message (matches existing ape: target)

No Python removed—scripts were already POSIX sh.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits March 5, 2026 08:55
- Strip trailing `--` from comment bodies in opensmithgen.c
- Add raw_len field to ast_node_t for byte-accurate reconstruction
- Make reconstruct() and final literal segment handling branchless
- Fix shell injection vulnerability in opensmith_parity.sh
- Fix executable check (-x vs -f) in opensmith_frontend_check.sh
- Fix nested_zip_prefix grep pattern for whitespace handling
- Add cosmocc detection logic to opensmithgen-ape Makefile target

Co-authored-by: ludoplex <5705162+ludoplex@users.noreply.github.com>
- Skip empty final literal segments to avoid polluting AST
- Add null pointer check in reconstruct() for defensive programming

Co-authored-by: ludoplex <5705162+ludoplex@users.noreply.github.com>
Copilot AI changed the title [WIP] Add parser front-end AST roundtrip scaffold fix: address PR review comments for opensmith tooling Mar 5, 2026
@ludoplex
ludoplex marked this pull request as ready for review March 14, 2026 02:52
Copilot AI review requested due to automatic review settings March 14, 2026 02:52

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a67ab5ecc5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

RC=0
sh -c "$CMD" > "$OUT_FILE" 2> "$ERR_FILE" || RC=$?
# Execute engine with fixture as separate argument (avoids shell injection)
"$ENGINE" "$fixture" > "$OUT_FILE" 2> "$ERR_FILE" || RC=$?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve command-template semantics for --engine

Invoking the engine as "$ENGINE" "$fixture" now treats the entire --engine string as a single executable path, so command templates with arguments (including the documented ENGINE="cat {input}" flow in docs/OPENSMITH_PARITY.md) fail with 127 for every fixture instead of actually running the tool. This is a regression from the previous {input} substitution behavior and breaks parity runs that need an interpreter + script or any extra flags.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

refactor with previous {input} substitution behavior

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Addresses follow-up review feedback for the OpenSmith front-end tooling by improving parser roundtrip fidelity and tightening shell harness correctness/security.

Changes:

  • Update opensmithgen AST handling to strip comment delimiter artifacts and reconstruct using stored byte lengths (avoids strlen() truncation issues).
  • Harden/correct OpenSmith shell harnesses (engine execution, executable checks, JSON whitespace handling in lock parsing).
  • Improve opensmithgen-ape Makefile target with cosmocc detection + actionable error output.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/opensmithgen/opensmithgen.c Adds raw_len for byte-accurate reconstruction and cleans up comment/literal parsing.
scripts/opensmith_parity.sh Changes engine execution to avoid shell injection by passing fixture as a separate argv.
scripts/opensmith_frontend_check.sh Validates tool is executable (-x) rather than merely present (-f).
scripts/opensmith_corpus.sh Makes nested_zip_prefix extraction tolerant to JSON whitespace.
Makefile Adds cosmocc detection/fallback for opensmithgen-ape builds.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +69 to +70
# Execute engine with fixture as separate argument (avoids shell injection)
"$ENGINE" "$fixture" > "$OUT_FILE" 2> "$ERR_FILE" || RC=$?
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.

3 participants