Repository navigation
Add SVG viz, test helpers, and makegen scaffold - #1
Merged
Merged
Conversation
![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: e5680ee02d
ℹ️ 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".
Comment on lines
+64
to
+72
| // Compute input hash from Cargo.toml mtime + config | ||
| let cargo_toml = Path::new(&workspace_path).join("Cargo.toml"); | ||
| let mtime = std::fs::metadata(&cargo_toml) | ||
| .and_then(|m| m.modified()) | ||
| .map(|t| format!("{:?}", t)) | ||
| .unwrap_or_else(|_| "unknown".into()); | ||
|
|
||
| let config_str = format!("per_crate={},lint={}", per_crate, lint); | ||
| let input_hash = compute_hash(&format!("{}{}", mtime, config_str)); |
There was a problem hiding this comment.
In MakegenOp::Check, the up-to-date decision is based only on the workspace root Cargo.toml mtime plus the per_crate/lint flags, so changes to a member crate’s Cargo.toml (e.g., renaming package.name) won’t change the hash and needs_generate stays false, leaving stale per-crate targets in the generated Makefile. This shows up when a member’s manifest changes without touching the root Cargo.toml (common during renames), because the hash never reflects the updated crate list; consider hashing the parsed member list or including member manifest mtimes in the hash input.
Useful? React with 👍 / 👎.
Comment on lines
+162
to
+166
| if let Some(members) = workspace.get("members").and_then(|m| m.as_array()) { | ||
| for member in members { | ||
| if let Some(member_path) = member.as_str() { | ||
| if let Some(crate_info) = parse_member_crate(&workspace_path, member_path) { | ||
| crates.push(crate_info); |
There was a problem hiding this comment.
The workspace parser treats each workspace.members entry as a literal path and passes it directly to parse_member_crate, which means common Cargo patterns like members = ["crates/*"] are not expanded. In that case, parse_member_crate tries to read crates/*/Cargo.toml, fails, and the crate list becomes empty, so the generated Makefile silently drops per-crate targets for all globbed members. To avoid missing targets in globbed workspaces, expand globs or switch to cargo metadata.
Useful? React with 👍 / 👎.
…olidated commands for building, testing, and linting, while removing deprecated targets. Updated descriptions for clarity and added new target for listing dependencies.
briansrls
pushed a commit
that referenced
this pull request
Feb 13, 2026
Addresses feedback from 2026-02-12 PR review across 6 key areas: 1. Fix probe→observer recursion (#1): Intermediate observers are now promoted to probes unconditionally (not gated on Exact matchers). Tests are seeded from baseline DryRun, so concrete values aren't needed. This enables compositional segment testing A→B + B→C. 2. Fix extract_observers() seen/merge bug (#2): NodeExamples with only input-dependent matchers (Exact/Contains) no longer suppress valid chain-safe matchers from live_expected_outputs for the same node. Switched to merge-by-node approach using BTreeMap union. 3. Make gaps fail CI (#3): Coverage gaps now generate a failing test_observability_invariant_no_gaps test instead of just a header comment. Aligns behavior with the stated invariant. 4. Make lowering failures loud (#4): DAG lowering errors now generate a failing test_probe_observer_lowering_failed test instead of silently returning None and skipping all chain tests. 5. Seed policy fail-closed (#5/#8): Inverted seed_policy_for_type to whitelist known-safe primitive types (String, Bool, Int, etc.) and default unknown types to ExplicitSeedRequired. New types and aliases no longer silently fall into placeholder generation. 6. Additional hardening: - Add input_mocks as a probe source (#6) for DAGs seeded via entry input ports - Track weak observers (Any/IsRequest/IsResponse) in coverage reports (#5) so teams can identify low-value assertions - Promoted probes now appear in analysis results - ParamType::from(&str) panics on unknown types instead of silently defaulting to Str (#9) - Int parsing returns ParseError::InvalidInt instead of unwrap_or(0) https://claude.ai/code/session_014cTfu4arnDzFZCELaR26P4
briansrls
added a commit
that referenced
this pull request
Mar 1, 2026
Hack #1 - PipeMethod stringly-typed spread: Add PipeMethod::as_str() and Display impl as single source of truth. Delete three duplicate pipe_method_name() functions from lib.rs, expr.rs, fn_codegen.rs. All callsites now use method.as_str(). Hack #2 - Silent fallbacks in resource_defs.rs: Replace silent return-fallback with .expect() on compile_from_context, data_values.get, and serde_json::from_value. Delete ResourceDslData::fallback() and three FALLBACK_* constants. DSL syntax errors now fail the build immediately. Same treatment applied to gitignore.rs: silent fallback → expect(). Hack #3 - #[path] crate boundary (documented): core/resolve uses #[path] to compile resolve_service.rs from gunbc-dag. Physical move requires extracting gunbc-dag dependencies from the module first (use super::* reverse dependency). Documented as Lane A task. Hack #4 - compile-on-demand spread (documented): 7 files use compile_from_context at runtime. Now all fail-closed (no silent fallbacks). The include_str!/StdLibHost migration is a Lane A Phase A3 task. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
briansrls
added a commit
that referenced
this pull request
Mar 1, 2026
…lized #1 (parser auth_input): Verified already correct — returns Err on non-identifier. The general `_ => { self.advance(); }` for unknown config keys is a separate issue (config key validation) tracked as new red-team task. #2 (derive_file_spec panic): Replaced eprintln! + return None with LowerError::InvalidFileOp — fail-closed on invalid file operations. New variant added to LowerError enum with Display impl. #3b (type_id.ends_with('?')): Added Port::is_optional() method on core IR Port struct. Centralizes stringly-typed optionality check to single source of truth. All callsites updated. Long-term: structural Type::Optional. Correctness (dedup): All 6 dedup() calls verified to have preceding sort(). No unsorted dedup found. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
briansrls
added a commit
that referenced
this pull request
Mar 1, 2026
PR comment #1 (param field access wiring): Already fixed in previous commit — ExprComputeOp.execute() destructures Value::Map inputs into field-level env entries (line__is_blank etc). Param source nodes feed the full record; the compute node unpacks it. PR comment #2 (passthrough Skipped fallback): Restore partial fail-closed enforcement. Required outputs now return ExecError when other passthroughs arrived but this one is missing (indicating a genuine lowerer wiring gap). When NO passthroughs arrived (entire callable was skipped/guarded), fall back to Skipped. PR comment #3 (derive_file_spec panic): Change return type from Option to Result<FileOperationSpec, LowerError>. Invalid file operations now return a typed LowerError instead of panicking. Caller maps Err to None (skip operation with diagnostic). Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
briansrls
added a commit
that referenced
this pull request
Mar 13, 2026
Addresses the #1 finding from the invariant audit: eval_stack.rs had its own match_pattern that diverged from eval.rs's authoritative version. The old evaluator's match_pattern has: - Transparent Option matching (Some { value: x } matches bare values) - Structural None matching (Map with _variant: 'None') - unwrap_or(Value::Unit) for missing variant fields - values_equal with Enum/Str cross-matching The eval_stack copy had none of these. This was the root cause of the 9 remaining v2 test failures ('no matching arm for: Enum'). Fix: make match_pattern, eval_literal public in eval.rs; delete the duplicates from eval_stack.rs; import the shared versions. Remaining invariant violations documented for follow-up: - DUP: Env (eval.rs vs eval_stack.rs) — different shapes by design - DUP: bind_let_result / wrap_value_as_output — extract to shared module - DUP: contains_call (anf.rs vs eval_stack.rs) — extract to shared module - DUP: ANF verifier (anf.rs vs eval_stack.rs) — consolidate - PAR: eval_block_stmts / eval_pure_block_stmts / eval_block_as_body - PAR: eval_expr (eval_stack) vs eval_expr_tc (eval.rs) - FAB: extract_projection returns Unit for empty output (vs Error in old) - FAB: Capitalized identifier fallback masks typos Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
briansrls
added a commit
that referenced
this pull request
Mar 13, 2026
…nt Map Addresses review invariant #3 (No fallbacks that fabricate). extract_projection previously fabricated a Value::Map from the output when the expected field wasn't found. Now returns Result<Value, String> so callers handle the error explicitly. PopResult gains an Error variant; run_machine propagates it. Also confirms all 5 review findings against the code: 1. Three parallel stmt evaluators (eval_body, eval_block_as_body, eval_pure_block_stmts) — No parallel implementations violation 2. ANF verifier permits Calls in Blocks — Boundary contract loophole 3. Projection fabrication — Fixed in this commit 4. Duplicate PC (heap vs native stack) — Architecture issue 5. O(N) Rc clone — Performance issue The correct fix for #1, #2, #4 is slice-based continuations (stack bubbling), documented in DESIGN-eval-redesign.md as the next step. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
briansrls
added a commit
that referenced
this pull request
Mar 27, 2026
Review fixes: - Fix variable shadowing: let_value/let_body renamed to val_expr/body_expr in 04_infer.dag ExprLet handling (reviewer comment #1) - Delete 4 unused accessor functions: call_arg_nodes, record_field_nodes, list_elements, string_interp_parts (reviewer comment #2) - Comments #3 (collection kind match) and #4 (variant connective) were already fixed in earlier commits Documentation: - Updated Phase 5 Exit Criteria with full breakdown of remaining 147: - 8 type-name comparisons: 5 access (blocked on inline test std loading), 1 is_kernel_type, 1 string element, 1 Tuple - 139 type constructors: factory functions (bridge debt) - Documented the specific blocker: inline tests don't load std modules, so types resolve as bare kernel seed leaves, not algebra compositions. The algebra infrastructure exists (FreeMonoid.index, OrderedRing, structural method lookup B4) but can't be verified end-to-end in inline tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 28, 2026
Merged
4 tasks done
briansrls
added a commit
that referenced
this pull request
Apr 2, 2026
Reviewer correctly flagged three items as overclaimed: - #1 child_inferred_or_empty: DONE→PARTIAL (None-path still leaks) - #2 authored_name_at: DONE→PARTIAL (wrapper migration, not structural) - #4 transport kind: DONE→PARTIAL (config keys centralized, kind still name-based) Updated bootstrap health dashboard to reflect current reality: - Bootstrap B: 41 Rc::new(HashMap) scaffolding (not true fixes) - Bootstrap C: 548 typed errors from Callable regression, 3 perf fixes shipped (tokenizer O(n^2), dag_syntax cache, shape binop) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 3, 2026
briansrls
added a commit
that referenced
this pull request
Apr 7, 2026
Review #1 (branching_proof_safe first-dimension check): Added comment explaining why checking only the primary dimension is sound — TreeSize/ ListLength children are disjoint subtrees, so branching is O(n). The proof constructor validates non-descending subgraph acyclicity, ensuring all paths eventually descend on the primary dimension. Review #2 (Map<String, ...> in graph types): Added M4 note acknowledging string keys are textual identity proxies. Pre-existing pattern moved from complexity.dag, not introduced here. Deferred to M4 track. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
added a commit
that referenced
this pull request
Apr 8, 2026
…List" Addresses review comment #3: classify_field_recursion was comparing field_type_name == "List" directly, violating structural dispatch. Now uses is_container_type() and container_expected_arity() from std/types.dag to dispatch on container kind structurally: - Arity-1 containers (List, Set, NonEmptyList, NonEmptySet): element type at position 0 → ListRecursion (or SetRecursion for sets, but currently all arity-1 map to ListRecursion since Set<self> recursion is the same structural pattern) - Arity-2 containers (Map): value type at position 1 → MapValueRecursion Adding a new container type to std/types.dag now automatically extends recursive field detection with zero changes to the classify function. Review comments #1 (no downstream consumer) and #2 (string proxies) are acknowledged as known constraints — #1 resolves when CX-L2 lands, #2 resolves with M4 (Node.name deletion). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 of 2 tasks
2 of 3 tasks
This was referenced Jul 23, 2026
This was referenced Jul 25, 2026
This was referenced Aug 15, 2026
This was referenced Aug 27, 2026
This was referenced Sep 4, 2026
This was referenced Sep 12, 2026
This was referenced Sep 24, 2026
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
1 participant
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.
No description provided.