feat(opensmith): add parser front-end AST roundtrip scaffold - #5
Conversation
Reviewer's GuideIntroduces a new Ring 0 tool Class diagram for opensmithgen parser/AST front-end structuresclassDiagram
class node_kind_t {
<<enum>>
NODE_LITERAL
NODE_DIRECTIVE
NODE_EXPRESSION
NODE_STATEMENT
NODE_COMMENT
}
class mode_t {
<<enum>>
MODE_AST
MODE_ROUNDTRIP
MODE_CHECK_ROUNDTRIP
MODE_STATS
}
class attr_t {
char* key
char* value
}
class ast_node_t {
node_kind_t kind
size_t start
size_t end
int line
int col
char* raw
char* inner
char* body
char* directive_name
attr_t* attrs
size_t attr_count
size_t attr_cap
}
class ast_doc_t {
ast_node_t* nodes
size_t count
size_t cap
}
class parse_error_t {
int line
int col
char msg[160]
}
class opensmithgen_main {
+int main(int argc, char** argv)
+void usage(const char* argv0)
+int read_file(const char* path, char** out, size_t* out_len)
+int parse_template(const char* buf, size_t len, ast_doc_t* doc, parse_error_t* err)
+void emit_ast_json(FILE* out, const char* path, const ast_doc_t* doc)
+int reconstruct(const ast_doc_t* doc, char** out, size_t* out_len)
+int check_roundtrip(const char* orig, size_t orig_len, const ast_doc_t* doc)
+void emit_stats(FILE* out, const ast_doc_t* doc)
+void free_doc(ast_doc_t* doc)
+void free_node(ast_node_t* n)
}
class helpers {
+void* xmalloc(size_t n)
+void* xrealloc(void* ptr, size_t n)
+char* xstrndup0(const char* s, size_t n)
+char* dup_trimmed(const char* s)
+int is_ident_char(int c)
+const char* node_kind_str(node_kind_t kind)
+const char* directive_role(const char* name)
+int add_attr(ast_node_t* node, char* key, char* value)
+int add_node(ast_doc_t* doc, ast_node_t* node)
+void advance_pos(const char* buf, size_t start, size_t end, int* line, int* col)
+size_t find_token(const char* buf, size_t start, size_t len, const char* tok)
+void parse_directive_attrs(ast_node_t* node)
+void classify_node(ast_node_t* node)
+void json_escape(FILE* out, const char* s)
}
ast_doc_t "1" o-- "*" ast_node_t : owns
ast_node_t "1" o-- "*" attr_t : has
opensmithgen_main ..> ast_doc_t : uses
opensmithgen_main ..> ast_node_t : uses
opensmithgen_main ..> parse_error_t : uses
opensmithgen_main ..> node_kind_t : uses
opensmithgen_main ..> mode_t : uses
helpers ..> ast_doc_t : manipulates
helpers ..> ast_node_t : manipulates
helpers ..> attr_t : manipulates
helpers ..> node_kind_t : classifies
Flow diagram for opensmith_frontend_check fixture processingflowchart TD
Start([Start])
ParseArgs["parse_args"]
ResolvePaths["resolve tool, inventory, corpus_dir"]
ReadLock["read_lock"]
CollectFixtures["collect_fixtures"]
CheckMissing["check for missing fixture files"]
LoopFixtures{{"for each fixture"}}
RunRoundtrip["run --check-roundtrip"]
RoundtripFail{{"roundtrip returncode != 0"}}
LogRTFail["log roundtrip FAIL (<=20) and stderr"]
RunAst["run --ast"]
AstExitFail{{"ast returncode != 0"}}
LogAstFail["log ast FAIL (<=20) and stderr"]
ValidateJson["json.loads(ast stdout); ensure dict with nodes"]
JsonFail{{"JSON invalid"}}
LogJsonFail["log ast json FAIL (<=20)"]
NextFixture["next fixture"]
Summary["print fixtures checked, roundtrip failures, ast failures"]
AnyFail{{"roundtrip_fail or ast_fail"}}
Exit3[["exit 3"]]
Exit0[["exit 0"]]
Exit2[["exit 2"]]
Exit1[["exit 1"]]
Start --> ParseArgs --> ResolvePaths
ResolvePaths --> ReadLock
ReadLock --> CollectFixtures
CollectFixtures -->|no fixtures| Exit1
CollectFixtures -->|fixtures found| CheckMissing
CheckMissing -->|missing files| Exit2
CheckMissing -->|all present| LoopFixtures
LoopFixtures -->|fixtures remaining| RunRoundtrip
LoopFixtures -->|none| Summary
RunRoundtrip --> RoundtripFail
RoundtripFail -->|yes| LogRTFail --> NextFixture --> LoopFixtures
RoundtripFail -->|no| RunAst
RunAst --> AstExitFail
AstExitFail -->|yes| LogAstFail --> NextFixture --> LoopFixtures
AstExitFail -->|no| ValidateJson
ValidateJson --> JsonFail
JsonFail -->|yes| LogJsonFail --> NextFixture --> LoopFixtures
JsonFail -->|no| NextFixture --> LoopFixtures
Summary --> AnyFail
AnyFail -->|yes| Exit3
AnyFail -->|no| Exit0
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
opensmithgen.usagethe usage string and examples still mention only<input.cst>, but the tool is used for.cst/.csp/.csmapin the frontend check; consider updating the help text to advertise all supported extensions so it matches how the tool is invoked elsewhere. - In
opensmith_frontend_check.py, returning exit code 1 when no fixtures are selected (print("no fixtures selected")) may be surprising in CI when using a restrictive--patternor--extfilter; consider returning 0 in that case or adding a flag to treat an empty selection as success.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `opensmithgen.usage` the usage string and examples still mention only `<input.cst>`, but the tool is used for `.cst/.csp/.csmap` in the frontend check; consider updating the help text to advertise all supported extensions so it matches how the tool is invoked elsewhere.
- In `opensmith_frontend_check.py`, returning exit code 1 when no fixtures are selected (`print("no fixtures selected")`) may be surprising in CI when using a restrictive `--pattern` or `--ext` filter; consider returning 0 in that case or adding a flag to treat an empty selection as success.
## Individual Comments
### Comment 1
<location path="tools/opensmithgen/opensmithgen.c" line_range="356-357" />
<code_context>
+ int col = 1;
+
+ while (i < len) {
+ size_t tag_start = find_token(buf, i, len, "<%");
+ if (tag_start == (size_t)-1) {
+ if (tag_start != i) {
+ ast_node_t lit;
</code_context>
<issue_to_address>
**nitpick:** The `if (tag_start != i)` check in the EOF case is misleading and always true.
In the `tag_start == (size_t)-1` branch, `tag_start` is always `(size_t)-1` while `i` is a valid offset, so `if (tag_start != i)` is effectively always true. Since the behavior (emitting the trailing literal) is correct but the condition is misleading, consider replacing it with something that reflects the real intent (e.g. `if (i < len)`) or removing the `if` and adding a brief comment explaining the control flow.
</issue_to_address>
### Comment 2
<location path="scripts/opensmith_frontend_check.py" line_range="69" />
<code_context>
return subprocess.run(cmd, text=True, capture_output=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| size_t tag_start = find_token(buf, i, len, "<%"); | ||
| if (tag_start == (size_t)-1) { |
There was a problem hiding this comment.
nitpick: The if (tag_start != i) check in the EOF case is misleading and always true.
In the tag_start == (size_t)-1 branch, tag_start is always (size_t)-1 while i is a valid offset, so if (tag_start != i) is effectively always true. Since the behavior (emitting the trailing literal) is correct but the condition is misleading, consider replacing it with something that reflects the real intent (e.g. if (i < len)) or removing the if and adding a brief comment explaining the control flow.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8028d2400
ℹ️ 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".
| if (tlen >= 2 && trimmed[0] == '-' && trimmed[1] == '-') { | ||
| node->kind = NODE_COMMENT; |
There was a problem hiding this comment.
<%-- ... --%> tags
classify_node() currently treats any trimmed block beginning with -- as a comment, so valid statement blocks like <% --i; %> are misclassified as NODE_COMMENT instead of NODE_STATEMENT. This produces incorrect AST node kinds for legal templates and can corrupt downstream parity metrics or codegen behavior that depends on node type.
Useful? React with 👍 / 👎.
|
|
||
| if (tlen >= 2 && trimmed[0] == '-' && trimmed[1] == '-') { | ||
| node->kind = NODE_COMMENT; | ||
| node->body = dup_trimmed(trimmed + 2); |
There was a problem hiding this comment.
The comment-body extraction only removes the leading -- (dup_trimmed(trimmed + 2)), so canonical comments like <%-- hello --%> produce a body of hello -- with the trailing delimiter fragment still attached. This makes comment payloads consistently inaccurate for standard OpenSmith comment syntax.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a Ring 0 OpenSmith parser/AST “front-end” scaffold (opensmithgen) plus a corpus-wide checker, integrating it into the existing OpenSmith corpus/parity workflow.
Changes:
- Introduce
opensmithgenC tool to parse OpenSmith-style templates into a simple AST, emit JSON, and verify exact roundtrip stability. - Add
opensmith-frontend-checkMakefile target + Python runner to execute roundtrip/AST checks across the extracted corpus fixtures. - Update documentation and spec-type inventory to reflect the new front-end scaffold.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/opensmithgen/opensmithgen.c | New Ring 0 CLI tool: parse tags/literals, emit AST JSON, reconstruct original text, and roundtrip-check. |
| scripts/opensmith_frontend_check.py | New corpus runner that invokes opensmithgen over lock-selected fixtures and validates JSON output. |
| Makefile | Builds opensmithgen, adds opensmith-frontend-check target, and surfaces .cst/.csp/.csmap in make formats. |
| specs/testing/opensmith/README.md | Documents the new front-end check step in the OpenSmith workflow. |
| docs/OPENSMITH_PARITY.md | Expands scope/commands to include front-end roundtrip checks. |
| SPEC_TYPES.md | Reassigns .cst/.csp/.csmap to opensmithgen and updates their status text. |
| README.md | Lists the new make opensmith-frontend-check command. |
| node->kind = NODE_COMMENT; | ||
| node->body = dup_trimmed(trimmed + 2); |
There was a problem hiding this comment.
Comment nodes parsed from <%-- ... --%> will keep the trailing -- in body (e.g., -- comment -- becomes comment --). If body is intended to represent the comment contents, strip the closing -- (after trimming) when classifying comments so the AST JSON doesn’t include delimiter artifacts.
| node->kind = NODE_COMMENT; | |
| node->body = dup_trimmed(trimmed + 2); | |
| char *comment = dup_trimmed(trimmed + 2); | |
| size_t clen = strlen(comment); | |
| if (clen >= 2 && comment[clen - 1] == '-' && comment[clen - 2] == '-') { | |
| size_t newlen = clen - 2; | |
| while (newlen > 0 && isspace((unsigned char)comment[newlen - 1])) { | |
| newlen--; | |
| } | |
| comment[newlen] = '\0'; | |
| } | |
| node->kind = NODE_COMMENT; | |
| node->body = comment; |
| def safe_relpath(parts: Iterable[str]) -> PurePosixPath: | ||
| p = PurePosixPath(*parts) | ||
| if p.is_absolute() or ".." in p.parts: | ||
| raise ValueError(f"unsafe relative path: {p}") | ||
| return p |
There was a problem hiding this comment.
safe_relpath() uses PurePosixPath and only rejects absolute paths / .. segments. On Windows, backslashes in a lock entry (e.g. ..\\secret) won’t be split into path parts by PurePosixPath but will still be interpreted as separators by the OS when you later call Path.joinpath(*rel.parts), allowing traversal outside --corpus-dir. Consider explicitly rejecting \\ (and drive-letter patterns like C:) in inputs, or validating with PureWindowsPath in addition to PurePosixPath before joining.
| if (tag_start != i) { | ||
| ast_node_t lit; | ||
| memset(&lit, 0, sizeof(lit)); | ||
| lit.kind = NODE_LITERAL; | ||
| lit.start = i; | ||
| lit.end = len; | ||
| lit.line = line; | ||
| lit.col = col; | ||
| lit.raw = xstrndup0(buf + i, len - i); | ||
| lit.body = xstrndup0(lit.raw, strlen(lit.raw)); | ||
| add_node(doc, &lit); | ||
| advance_pos(buf, i, len, &line, &col); | ||
| } |
There was a problem hiding this comment.
This condition is always true when tag_start == (size_t)-1 (since i can never be (size_t)-1), so it doesn’t guard anything and makes the control flow harder to follow. Consider removing the if (tag_start != i) check and unconditionally emitting the final literal segment when no more tags are found.
| if (tag_start != i) { | |
| ast_node_t lit; | |
| memset(&lit, 0, sizeof(lit)); | |
| lit.kind = NODE_LITERAL; | |
| lit.start = i; | |
| lit.end = len; | |
| lit.line = line; | |
| lit.col = col; | |
| lit.raw = xstrndup0(buf + i, len - i); | |
| lit.body = xstrndup0(lit.raw, strlen(lit.raw)); | |
| add_node(doc, &lit); | |
| advance_pos(buf, i, len, &line, &col); | |
| } | |
| ast_node_t lit; | |
| memset(&lit, 0, sizeof(lit)); | |
| lit.kind = NODE_LITERAL; | |
| lit.start = i; | |
| lit.end = len; | |
| lit.line = line; | |
| lit.col = col; | |
| lit.raw = xstrndup0(buf + i, len - i); | |
| lit.body = xstrndup0(lit.raw, strlen(lit.raw)); | |
| add_node(doc, &lit); | |
| advance_pos(buf, i, len, &line, &col); |
There was a problem hiding this comment.
why not branchless as well?
| for (i = 0; i < doc->count; i++) { | ||
| const ast_node_t *n = &doc->nodes[i]; | ||
| size_t raw_len = n->raw ? strlen(n->raw) : 0; | ||
| if (raw_len > ((size_t)-1) - total - 1) { | ||
| fprintf(stderr, "error: output too large\n"); | ||
| return -1; | ||
| } | ||
| total += raw_len; | ||
| } |
There was a problem hiding this comment.
reconstruct() recomputes each node’s raw length using strlen(n->raw), which (a) makes reconstruction O(total_bytes) just for length scans, and (b) truncates if a fixture contains embedded NUL bytes even though the parser tracks byte offsets via start/end. Prefer using the already-known length (n->end - n->start) or storing an explicit raw length alongside raw so roundtrips remain byte-accurate and avoid extra scans.
There was a problem hiding this comment.
why not branchless as well?
|
totally remove python, its no value to this project @copilot |
* Initial plan * refactor: remove all Python scripts, replace with POSIX sh equivalents Co-authored-by: ludoplex <5705162+ludoplex@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ludoplex <5705162+ludoplex@users.noreply.github.com>
like what? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
docs/OPENSMITH_PARITY.md:61
- This section claims the lock file guarantees fixture selection stability and corpus integrity, but the current
opensmith_corpus.sh extract/opensmith_parity.shpaths do not use the lock's entry list or hashes for validation/selection (they rescan the filesystem). Either adjust the scripts to honor the lock file or soften/clarify this documentation claim.
`specs/testing/opensmith/corpus.lock.json` captures:
- source archive hash
- nested sample archive scope
- file extension filters
- deterministic entry list with size + sha256
The lock file is used to guarantee fixture selection stability across machines
and future parity PRs.
| # collect .cst/.csp/.csmap fixtures from extracted corpus | ||
| TMPDIR_WORK=$(mktemp -d) | ||
| trap 'rm -rf "$TMPDIR_WORK"' EXIT | ||
|
|
||
| find "$CORPUS_DIR" -type f \( -name '*.cst' -o -name '*.csp' -o -name '*.csmap' \) | sort \ | ||
| > "$TMPDIR_WORK/fixtures" | ||
|
|
There was a problem hiding this comment.
--inventory is required but the script never reads it to select fixtures or validate corpus integrity; it just checks the file exists and then does find "$CORPUS_DIR" .... Either remove the --inventory argument entirely or use the lock file to drive fixture discovery so runs remain deterministic relative to the inventory.
| # collect .cst/.csp/.csmap fixtures from extracted corpus | |
| TMPDIR_WORK=$(mktemp -d) | |
| trap 'rm -rf "$TMPDIR_WORK"' EXIT | |
| find "$CORPUS_DIR" -type f \( -name '*.cst' -o -name '*.csp' -o -name '*.csmap' \) | sort \ | |
| > "$TMPDIR_WORK/fixtures" | |
| # collect .cst/.csp/.csmap fixtures from inventory, validating against corpus | |
| TMPDIR_WORK=$(mktemp -d) | |
| trap 'rm -rf "$TMPDIR_WORK"' EXIT | |
| python3 - "$INVENTORY" "$CORPUS_DIR" > "$TMPDIR_WORK/fixtures" << 'PYEOF' | |
| import json | |
| import os | |
| import sys | |
| if len(sys.argv) != 3: | |
| sys.stderr.write("usage: python3 - <inventory.json> <corpus_dir>\n") | |
| sys.exit(1) | |
| inventory_path = sys.argv[1] | |
| corpus_dir = sys.argv[2] | |
| try: | |
| with open(inventory_path, "r", encoding="utf-8") as f: | |
| data = json.load(f) | |
| except Exception as exc: | |
| sys.stderr.write(f"failed to read inventory '{inventory_path}': {exc}\n") | |
| sys.exit(1) | |
| # Support either: | |
| # - a top-level list of relative fixture paths, or | |
| # - an object with a 'fixtures' key containing such a list. | |
| if isinstance(data, dict) and "fixtures" in data: | |
| fixtures = data["fixtures"] | |
| elif isinstance(data, list): | |
| fixtures = data | |
| else: | |
| sys.stderr.write( | |
| "unsupported inventory format: expected list or object with 'fixtures' key\n" | |
| ) | |
| sys.exit(1) | |
| normalized = [] | |
| for item in fixtures: | |
| if not isinstance(item, str): | |
| sys.stderr.write("inventory entries must be strings (relative fixture paths)\n") | |
| sys.exit(1) | |
| # Only include known fixture extensions. | |
| if not item.endswith((".cst", ".csp", ".csmap")): | |
| continue | |
| rel_path = item.lstrip("/") | |
| full_path = os.path.normpath(os.path.join(corpus_dir, rel_path)) | |
| if not full_path.startswith(os.path.abspath(corpus_dir) + os.sep): | |
| sys.stderr.write(f"inventory path escapes corpus dir: {item}\n") | |
| sys.exit(1) | |
| if not os.path.isfile(full_path): | |
| sys.stderr.write(f"fixture not found in corpus: {item}\n") | |
| sys.exit(1) | |
| normalized.append(full_path) | |
| for path in sorted(set(normalized)): | |
| print(path) | |
| PYEOF |
| # extract: extract corpus using the lock file | ||
| # ────────────────────────────────────────────────────────────────────────────── | ||
| cmd_extract() { | ||
| ZIP="" | ||
| LOCK="" | ||
| OUT_DIR="" | ||
|
|
||
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| --zip) ZIP="$2"; shift 2 ;; | ||
| --lock) LOCK="$2"; shift 2 ;; | ||
| --out-dir) OUT_DIR="$2"; shift 2 ;; | ||
| *) die "unknown option: $1" ;; | ||
| esac | ||
| done | ||
|
|
||
| [ -n "$ZIP" ] || die "--zip required" | ||
| [ -n "$LOCK" ] || die "--lock required" | ||
| [ -n "$OUT_DIR" ] || die "--out-dir required" | ||
| [ -f "$ZIP" ] || die "zip not found: $ZIP" | ||
| [ -f "$LOCK" ] || die "lock not found: $LOCK" | ||
|
|
||
| TMPDIR_WORK=$(mktemp -d) | ||
| trap 'rm -rf "$TMPDIR_WORK"' EXIT | ||
|
|
||
| unzip -q "$ZIP" -d "$TMPDIR_WORK/outer" | ||
|
|
||
| # extract outer entries | ||
| find "$TMPDIR_WORK/outer" -type f | sort | while IFS= read -r f; do | ||
| REL="${f#$TMPDIR_WORK/outer/}" | ||
| EXT=$(printf '%s' "$REL" | sed 's/.*\.//' | tr '[:upper:]' '[:lower:]') | ||
| case ".$EXT" in | ||
| .cst|.csp|.csmap|.xsd|.xml|.json) ;; | ||
| *) continue ;; | ||
| esac | ||
| DST="$OUT_DIR/outer/$REL" | ||
| mkdir -p "$(dirname "$DST")" | ||
| cp "$f" "$DST" | ||
| done |
There was a problem hiding this comment.
cmd_extract() claims to "extract corpus using the lock file" but it ignores the lock's entry list and hashes; it re-scans the unzipped contents and copies everything with allowed extensions. This defeats reproducibility/integrity guarantees (zip changes won't be detected, and extra/missing files aren't validated). Consider extracting only the lock-defined entries and verifying per-entry sha256 (and source zip sha256) like the prior implementation.
|
|
||
| [ -n "$TOOL" ] || die "--tool required" | ||
| [ -n "$CORPUS_DIR" ] || die "--corpus-dir required" | ||
| [ -f "$TOOL" ] || die "tool not found: $TOOL" |
There was a problem hiding this comment.
The script verifies the tool exists with -f, but then executes it. If the file isn't executable (missing +x), the failure will be a confusing runtime error. Prefer checking -x "$TOOL" (or -f plus a clear chmod/message) before running.
| [ -f "$TOOL" ] || die "tool not found: $TOOL" | |
| [ -x "$TOOL" ] || die "tool not found or not executable: $TOOL" |
| NESTED_PREFIX=$(grep -o '"nested_zip_prefix":"[^"]*"' "$LOCK" 2>/dev/null \ | ||
| | sed 's/"nested_zip_prefix":"//; s/"//' || printf 'Samples') |
There was a problem hiding this comment.
The nested_zip_prefix extraction uses grep -o '"nested_zip_prefix":"[^"]*"', but the lock JSON written by cmd_inventory() includes spaces ("nested_zip_prefix": "..."). As written, this grep will fail and always fall back to Samples, ignoring custom prefixes. Update the pattern to tolerate whitespace or parse the JSON more robustly.
| NESTED_PREFIX=$(grep -o '"nested_zip_prefix":"[^"]*"' "$LOCK" 2>/dev/null \ | |
| | sed 's/"nested_zip_prefix":"//; s/"//' || printf 'Samples') | |
| NESTED_PREFIX=$(grep -o '"nested_zip_prefix"[[:space:]]*:[[:space:]]*"[^"]*"' "$LOCK" 2>/dev/null \ | |
| | sed 's/.*"nested_zip_prefix"[[:space:]]*:[[:space:]]*"//; s/".*$//' || printf 'Samples') |
|
|
||
| opensmithgen-ape: | ||
| @mkdir -p "$(BUILD_DIR)" | ||
| @cosmocc $(CFLAGS) \ |
There was a problem hiding this comment.
opensmithgen-ape invokes cosmocc directly without the discovery/error messaging used elsewhere (see the ape: target which falls back to ~/.cosmocc/bin/cosmocc and prints install steps). Consider reusing that detection logic here so make opensmithgen-ape fails with a clear, actionable message when cosmocc isn't installed.
| @cosmocc $(CFLAGS) \ | |
| @COSMOCC=$$( \ | |
| if command -v cosmocc >/dev/null 2>&1; then \ | |
| printf '%s' "cosmocc"; \ | |
| elif [ -x "$$HOME/.cosmocc/bin/cosmocc" ]; then \ | |
| printf '%s' "$$HOME/.cosmocc/bin/cosmocc"; \ | |
| else \ | |
| printf 'cosmocc not found.\n' >&2; \ | |
| printf 'To build APE tools you need cosmocc installed.\n' >&2; \ | |
| printf 'Either:\n' >&2; \ | |
| printf ' - Install cosmocc and ensure it is on your PATH, or\n' >&2; \ | |
| printf ' - Install it under $$HOME/.cosmocc/bin/cosmocc.\n' >&2; \ | |
| exit 1; \ | |
| fi \ | |
| ); \ | |
| "$$COSMOCC" $(CFLAGS) \ |
| opensmith-frontend-check: opensmithgen-ape opensmith-corpus | ||
| @sh ./scripts/opensmith_frontend_check.sh \ | ||
| --tool "$(OPENSMITH_FRONTEND_TOOL)" \ | ||
| --corpus-dir "$(OPENSMITH_CORPUS_DIR)" |
There was a problem hiding this comment.
PR description mentions a Python-based opensmith_frontend_check harness, but this PR removes the Python scripts and wires in a POSIX sh implementation instead (scripts/opensmith_frontend_check.sh, opensmith_parity.sh, opensmith_corpus.sh). Update the PR description to match the actual implementation/language so reviewers/users aren't misled.
| for (i = 0; i < doc->count; i++) { | ||
| const ast_node_t *n = &doc->nodes[i]; | ||
| size_t raw_len = n->raw ? strlen(n->raw) : 0; | ||
| if (raw_len > ((size_t)-1) - total - 1) { | ||
| fprintf(stderr, "error: output too large\n"); | ||
| return -1; | ||
| } | ||
| total += raw_len; | ||
| } | ||
|
|
||
| buf = (char *)xmalloc(total + 1); | ||
| for (i = 0; i < doc->count; i++) { | ||
| const ast_node_t *n = &doc->nodes[i]; | ||
| size_t raw_len = n->raw ? strlen(n->raw) : 0; | ||
| if (raw_len > 0) { | ||
| memcpy(buf + off, n->raw, raw_len); | ||
| off += raw_len; |
There was a problem hiding this comment.
reconstruct() computes each node's contribution using strlen(n->raw), but raw is built from byte slices and may legally contain \0 (files are read in binary mode). This can truncate output and make roundtrip checks incorrect. Prefer tracking raw byte lengths (e.g., n->end - n->start) or storing explicit raw_len alongside raw and using that instead of strlen.
| case "$ENGINE" in | ||
| *'{input}'*) CMD=$(printf '%s' "$ENGINE" | sed "s|{input}|$fixture|g") ;; | ||
| *) CMD="$ENGINE $fixture" ;; | ||
| esac | ||
|
|
||
| OUT_FILE="$ARTIFACTS_DIR/$REL.stdout" | ||
| ERR_FILE="$ARTIFACTS_DIR/$REL.stderr" | ||
| CODE_FILE="$ARTIFACTS_DIR/$REL.exitcode" | ||
|
|
||
| mkdir -p "$(dirname "$OUT_FILE")" | ||
|
|
||
| RC=0 | ||
| sh -c "$CMD" > "$OUT_FILE" 2> "$ERR_FILE" || RC=$? |
There was a problem hiding this comment.
Building the CMD string by concatenating ENGINE with the unescaped fixture path and then executing it via sh -c allows shell metacharacters in corpus file names to break out of the intended engine command. An attacker who can control entries in the corpus ZIP can craft fixture names like foo.cst; rm -rf / so that when sh -c "$CMD" runs, arbitrary shell commands execute with the privileges of this harness. To harden this, avoid sh -c and pass the engine and input path as separate argv elements (or robustly quote the fixture path) so that file names cannot alter the shell command structure.
|
@copilot open a new pull request to apply changes based on the comments in this thread except remove any and all python |
| TMPDIR_WORK=$(mktemp -d) | ||
| trap 'rm -rf "$TMPDIR_WORK"' EXIT | ||
|
|
||
| unzip -q "$ZIP" -d "$TMPDIR_WORK/outer" | ||
|
|
There was a problem hiding this comment.
These unzip extractions trust the archive’s paths and extract directly into a temp directory. A crafted zip with absolute paths or ../ components can perform a zip-slip write outside the temp directory. If OPENSMITH_ZIP is ever untrusted (or simply corrupted), this is a filesystem write vulnerability. Consider pre-validating the zip member list (e.g., via unzip -Z1) to reject unsafe paths before extraction, or use an extraction mode/tool that enforces path safety.
📋 Review Triage — What PR #8 Covers vs Still Open✅ Already addressed by PR #8
🔴 Still open (not in PR #8)
Items 1 and 2 are the highest priority (correctness + security). Will prepare patches. |
PR2 parser front-end scaffold.
Summary by Sourcery
Introduce an OpenSmith parser/AST front-end tool and hook it into the build and parity workflows for corpus-based roundtrip validation.
New Features:
Enhancements:
Documentation: