Parse obstruction strand continuations - #128
Conversation
Summary by CodeRabbit
WalkthroughAdds parser-first support for ChangesObstruction-strand parsing support
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review please Self-audit findings before repair:
I will repair these narrowly, rerun the relevant docs/parser gate, commit once, and update the PR. |
Activity Summary
Verification after repairs: cargo xtask verifyResult: green locally. Review state: GraphQL reviewThreads returned zero unresolved threads. CodeRabbit is rate-limited on this PR, so alternate review has been requested with |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/topics/obstruction-strands/test-plan.md (1)
8-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope section reads as self-contradictory.
"future Core and Target IR disposition for obstruction preservation" is listed under In scope, immediately followed by "Core semantics for the first-class source syntax" / "Target IR lowering for the first-class source syntax" under Out of scope. Without a qualifier (e.g. "tracked as planned rows only"), this looks like exactly the kind of overclaim ambiguity the PR's own self-audit (CL-1) already flagged and fixed elsewhere.
As per coding guidelines, "Topic README.md files must not describe intended behavior before it lands" and shelves should avoid ceremonial ambiguity about contract status.
✏️ Suggested wording fix
-- first-class source syntax for preserved repairable obstruction attempts; -- future Core and Target IR disposition for obstruction preservation; +- first-class source syntax for preserved repairable obstruction attempts; +- ledger tracking (planned rows only) of future Core and Target IR + disposition for obstruction preservation;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/topics/obstruction-strands/test-plan.md` around lines 8 - 20, The Scope section in the test-plan README is internally inconsistent because it lists future Core/Target IR disposition as in scope while also marking the corresponding Core semantics and Target IR lowering as out of scope. Update the wording in this document so the in-scope items only describe what is actually covered today, and move any future disposition language into a clearly labeled tracking/planned note or remove it entirely. Use the existing section headings and phrases around obstruction preservation, Core IR, and Target IR to make the scope status unambiguous.Source: Coding guidelines
🤖 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 `@crates/edict-syntax/src/parser.rs`:
- Around line 1096-1126: The duplicate `reason` diagnostic in
`require_obstructed_reason` is too broad and should point at the second
offending entry instead of the whole payload. Update the `DuplicateField` path
to use the span from the current `RecordEntry`/`Expr` candidate when the second
`reason` is seen, while keeping the existing `MissingRequiredField` behavior on
`payload_span`.
In `@docs/design/obstruction-strands-v0.md`:
- Around line 153-168: The “Future Verification” checklist in this design doc is
stale because several items are already covered by executable tests in this PR.
Update the checklist in obstruction-strands-v0.md to separate the
already-verified behaviors from the genuinely future work: mark the source AST
node, helper-shaped negative guard, missing/duplicate reason rejection, and
contextual-only rejection as implemented/verified, and leave only the
Core/Target IR, digest/mutation evidence, and receipt fixture items as future
verification. Use the existing test names like
continue_obstructed_source_arm_parses,
continue_obstructed_requires_reason_field,
continue_obstructed_rejects_duplicate_reason_field,
continue_obstructed_is_contextual_to_require_else, and
helper_shaped_continue_in_obstructed_strand_is_terminal to anchor the “verified
in PR” section.
---
Outside diff comments:
In `@docs/topics/obstruction-strands/test-plan.md`:
- Around line 8-20: The Scope section in the test-plan README is internally
inconsistent because it lists future Core/Target IR disposition as in scope
while also marking the corresponding Core semantics and Target IR lowering as
out of scope. Update the wording in this document so the in-scope items only
describe what is actually covered today, and move any future disposition
language into a clearly labeled tracking/planned note or remove it entirely. Use
the existing section headings and phrases around obstruction preservation, Core
IR, and Target IR to make the scope status unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e60df0ee-5969-4cbc-bf82-7b6ed4c9dc14
📒 Files selected for processing (11)
CHANGELOG.mdcrates/edict-syntax/src/ast.rscrates/edict-syntax/src/parser.rscrates/edict-syntax/src/semantic.rscrates/edict-syntax/tests/parse_review_regressions.rsdocs/design/obstruction-strands-v0.mddocs/topics/obstruction-strands/README.mddocs/topics/obstruction-strands/test-plan.mddocs/topics/syntax/test-plan.mdfixtures/obstruction-strands/v0/stale-basis/README.mdfixtures/obstruction-strands/v0/stale-basis/source.edict
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: supply-chain (cargo-deny)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # AGENTSGit Rules
NEVER amend git commits. Make a new commit instead.
NEVER use
git rebaseunless the user explicitly approves a rare exception.
Use regular merge commits.NEVER force any git operation. If a force operation appears necessary, stop and
explain what happened and what options remain.NEVER create draft pull requests.
NEVER use a
codexprefix in branch names, PR titles, or commit messages.Pull request bodies for issue work MUST include GitHub auto-close text such as
Closes#123`` for every issue the PR is intended to close.Think
Think is durable memory for cross-session coordination.
- Use
codex-think --remember --jsonwhen starting a new session, changing into
this repository, or regaining context after a context shift.- Use
codex-think "..." --jsonwhen a cycle closes or a significant event
should survive across turns.- Treat Think as memory, not repo truth. Anchor strong claims back to files,
commits, commands, issues, or pull requests.- Claude memories are read-only. Use
claude-think --remember --jsononly for
additional context.Topic Shelves
docs/topics/contains the living contract graph for landed behavior. Topic
shelves are not proposals, retrospectives, or design archaeology.Each shelf may contain:
README.md: what is true in HEAD.test-plan.md: how those truths are verified, including requirements, cases,
fixtures, oracles, implemented evidence, planned cases, and known gaps.architecture.md: optional structure or dataflow notes when the machinery
earns a separate page.rationale.md: optional still-relevant tradeoffs and rejected approaches.When To Update Topic Shelves
For every nontrivial behavior, contract, workflow, release, schema, validation,
or public-surface change:
- Identify the owning topic shelf before editing code.
- If no shelf owns durable behavior, create one.
- Update
test-plan.mdbefore or alongside tests with requirement IDs, case
IDs,...
Files:
fixtures/obstruction-strands/v0/stale-basis/source.edictfixtures/obstruction-strands/v0/stale-basis/README.mdCHANGELOG.mddocs/topics/obstruction-strands/README.mddocs/topics/syntax/test-plan.mdcrates/edict-syntax/tests/parse_review_regressions.rsdocs/topics/obstruction-strands/test-plan.mdcrates/edict-syntax/src/ast.rsdocs/design/obstruction-strands-v0.mdcrates/edict-syntax/src/semantic.rscrates/edict-syntax/src/parser.rs
docs/topics/**/README.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/topics/**/README.md: Topic shelfREADME.mdfiles must describe only what is true in HEAD.
Update the topic shelfREADME.mdonly after the behavior exists in the branch; do not describe intended future behavior there.
Do not churn topic-shelf README files for purely mechanical edits that do not change a contract.
Files:
docs/topics/obstruction-strands/README.md
docs/topics/**/test-plan.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/topics/**/test-plan.md: Update a topic shelftest-plan.mdbefore or alongside tests with requirement IDs, case IDs, fixtures, oracles, implemented evidence, planned cases, and known gaps.
Mark planned cases as implemented only when executable evidence exists.
Do not usepolicyrows in topic-shelf test plans to avoid writing behavior tests for software behavior.
Negative tests in topic-shelf test plans should assert stable error kinds or structured artifacts, not merelyis_err()or diagnostic text.
For nontrivial behavior, contract, workflow, release, schema, validation, or public-surface changes, update the owning topictest-plan.mdwith planned requirement and case rows before or alongside the first test.
Mark planned rows as implemented only after executable evidence exists.
Files:
docs/topics/syntax/test-plan.mddocs/topics/obstruction-strands/test-plan.md
🔇 Additional comments (18)
crates/edict-syntax/src/parser.rs (1)
7-10: LGTM!Also applies to: 31-32, 63-64, 225-230, 906-910, 1597-1598
crates/edict-syntax/src/semantic.rs (1)
8-8: LGTM!Also applies to: 365-368, 416-428, 459-473, 503-503
docs/topics/obstruction-strands/README.md (1)
3-16: LGTM!Also applies to: 30-60
docs/topics/obstruction-strands/test-plan.md (2)
30-34: LGTM!Also applies to: 68-70
44-56: 📐 Maintainability & Code QualityDrop this concern.
crates/edict-syntax/tests/parse_review_regressions.rsalready containscontinue_obstructed_is_contextual_to_require_else,helper_shaped_continue_in_obstructed_strand_is_terminal, andstale_basis_obstruction_strand_fixture_parses, andfixtures/obstruction-strands/v0/stale-basis/{README.md,source.edict}are present.> Likely an incorrect or invalid review comment.docs/topics/syntax/test-plan.md (1)
42-42: LGTM!Also applies to: 79-79, 105-107
crates/edict-syntax/src/ast.rs (1)
208-211: 🎯 Functional CorrectnessNo downstream exhaustiveness issue
Stmt::Require { .. }matches are unaffected by theobstruction→armrename, andRequireElseArmis already handled explicitly for both variants.> Likely an incorrect or invalid review comment.crates/edict-syntax/tests/parse_review_regressions.rs (4)
8-14: LGTM!
249-262: LGTM!
264-355: LGTM!
309-317: 🎯 Functional CorrectnessNo change needed:
continueis a bare identifier here, soParseErrorKind::ExpectedTokenis the right contract.> Likely an incorrect or invalid review comment.fixtures/obstruction-strands/v0/stale-basis/README.md (1)
1-34: LGTM!fixtures/obstruction-strands/v0/stale-basis/source.edict (1)
1-17: LGTM!CHANGELOG.md (1)
13-20: LGTM!docs/design/obstruction-strands-v0.md (4)
3-9: LGTM!
27-81: LGTM!
101-131: LGTM!
141-147: LGTM!
Review Fix SummaryAddressed CodeRabbit
Verification: cargo test -p edict-syntax --test parse_review_regressions continue_obstructed
cargo xtask contract-check
cargo xtask verifyAll green locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/edict-syntax/src/parser.rs`:
- Around line 1581-1599: The expr_span helper currently duplicates the Expr
variant list and must stay exhaustive; keep its match arms aligned with Expr in
parser.rs so any new variant is handled here too. If you add or rename an Expr
variant, update expr_span alongside the enum to preserve the compile-time
exhaustiveness check and avoid a stale second source of truth for spans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c38b0175-b30f-4e70-afba-d45522accc27
📒 Files selected for processing (4)
crates/edict-syntax/src/parser.rscrates/edict-syntax/tests/parse_review_regressions.rsdocs/design/obstruction-strands-v0.mddocs/topics/obstruction-strands/test-plan.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: supply-chain (cargo-deny)
🧰 Additional context used
📓 Path-based instructions (2)
docs/topics/**/test-plan.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/topics/**/test-plan.md: Update a topic shelftest-plan.mdbefore or alongside tests with requirement IDs, case IDs, fixtures, oracles, implemented evidence, planned cases, and known gaps.
Mark planned cases as implemented only when executable evidence exists.
Do not usepolicyrows in topic-shelf test plans to avoid writing behavior tests for software behavior.
Negative tests in topic-shelf test plans should assert stable error kinds or structured artifacts, not merelyis_err()or diagnostic text.
For nontrivial behavior, contract, workflow, release, schema, validation, or public-surface changes, update the owning topictest-plan.mdwith planned requirement and case rows before or alongside the first test.
Mark planned rows as implemented only after executable evidence exists.
Files:
docs/topics/obstruction-strands/test-plan.md
**
⚙️ CodeRabbit configuration file
**: # AGENTSGit Rules
NEVER amend git commits. Make a new commit instead.
NEVER use
git rebaseunless the user explicitly approves a rare exception.
Use regular merge commits.NEVER force any git operation. If a force operation appears necessary, stop and
explain what happened and what options remain.NEVER create draft pull requests.
NEVER use a
codexprefix in branch names, PR titles, or commit messages.Pull request bodies for issue work MUST include GitHub auto-close text such as
Closes#123`` for every issue the PR is intended to close.Think
Think is durable memory for cross-session coordination.
- Use
codex-think --remember --jsonwhen starting a new session, changing into
this repository, or regaining context after a context shift.- Use
codex-think "..." --jsonwhen a cycle closes or a significant event
should survive across turns.- Treat Think as memory, not repo truth. Anchor strong claims back to files,
commits, commands, issues, or pull requests.- Claude memories are read-only. Use
claude-think --remember --jsononly for
additional context.Topic Shelves
docs/topics/contains the living contract graph for landed behavior. Topic
shelves are not proposals, retrospectives, or design archaeology.Each shelf may contain:
README.md: what is true in HEAD.test-plan.md: how those truths are verified, including requirements, cases,
fixtures, oracles, implemented evidence, planned cases, and known gaps.architecture.md: optional structure or dataflow notes when the machinery
earns a separate page.rationale.md: optional still-relevant tradeoffs and rejected approaches.When To Update Topic Shelves
For every nontrivial behavior, contract, workflow, release, schema, validation,
or public-surface change:
- Identify the owning topic shelf before editing code.
- If no shelf owns durable behavior, create one.
- Update
test-plan.mdbefore or alongside tests with requirement IDs, case
IDs,...
Files:
docs/topics/obstruction-strands/test-plan.mdcrates/edict-syntax/tests/parse_review_regressions.rsdocs/design/obstruction-strands-v0.mdcrates/edict-syntax/src/parser.rs
🔇 Additional comments (11)
crates/edict-syntax/src/parser.rs (3)
1096-1131: 🎯 Functional CorrectnessDuplicate-reason span fix looks correct — resolves prior CodeRabbit feedback.
The previous review asked for the
DuplicateFielddiagnostic to point at the second offendingreasonentry instead of the whole payload span. The current implementation computesexpr_span(value)forFieldentries and uses the shorthand's own span forShorthandentries, then returnsSelf::err_at(ParseErrorKind::DuplicateField, ..., span)using the second candidate's span whileMissingRequiredFieldstill correctly usespayload_span. This matches the updated regression test (continue_obstructed_rejects_duplicate_reason_field) assertingerr.span.startequals the position of the secondreasonvalue.
225-230: LGTM!Also applies to: 1072-1094
7-10: LGTM!Also applies to: 31-32, 63-64, 906-912, 1133-1148, 1605-1625
docs/design/obstruction-strands-v0.md (2)
149-176: "Verification" split now correctly separates PR-1 evidence from future work — prior feedback addressed.The earlier review flagged that the "Future Verification" checklist understated already-landed evidence. This revision now cleanly splits "Already verified in PR
#128" (anchored to concrete test names:continue_obstructed_source_arm_parses,helper_shaped_continue_in_obstructed_strand_is_terminal,continue_obstructed_requires_reason_field,continue_obstructed_rejects_duplicate_reason_field,continue_obstructed_is_contextual_to_require_else) from genuinely future Core/Target IR/receipt work. This matches the coding guideline that test-plan/evidence claims should only be made once executable evidence exists.Source: Coding guidelines
3-5: LGTM!Also applies to: 27-81, 101-131, 141-148
crates/edict-syntax/tests/parse_review_regressions.rs (4)
243-262: LGTM!
265-297: LGTM!
300-311: 🎯 Functional CorrectnessUpdated duplicate-field test correctly asserts the tightened span — matches parser fix.
Switching from
reject_kindto directparse_module+expect_errlets the test asserterr.span.startagainstsrc.find("jim.EditObstruction.Other"), which is exactly the evidence the guidelines require ("Negative tests ... should assert stable error kinds or structured artifacts, not merelyis_err()or diagnostic text"). This is a solid regression addition.Source: Coding guidelines
313-359: LGTM!docs/topics/obstruction-strands/test-plan.md (2)
12-16: 📐 Maintainability & Code Quality | 💤 Low value"planned-row ledger entries" is an unusual scope item — double-check wording intent.
Line 13 lists "planned-row ledger entries for future Core and Target IR disposition work" as in scope. That's fine if it means "this shelf tracks the planned rows," but the guidelines specifically warn against
policyrows being used "to avoid writing behavior tests for software behavior." Worth a quick sanity check that this bullet isn't read as license to defer real test coverage indefinitely rather than as a documentation-tracking statement.Source: Coding guidelines
27-56: LGTM!Also applies to: 68-68
| fn expr_span(expr: &Expr) -> Span { | ||
| match expr { | ||
| Expr::Ident { span, .. } | ||
| | Expr::Int { span, .. } | ||
| | Expr::Str { span, .. } | ||
| | Expr::Bool { span, .. } | ||
| | Expr::Digest { span, .. } | ||
| | Expr::Field { span, .. } | ||
| | Expr::Call { span, .. } | ||
| | Expr::Unary { span, .. } | ||
| | Expr::Binary { span, .. } | ||
| | Expr::Record { span, .. } | ||
| | Expr::If { span, .. } | ||
| | Expr::IfYield { span, .. } | ||
| | Expr::VariantLit { span, .. } | ||
| | Expr::Match { span, .. } => *span, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Exhaustive expr_span match — verify it stays in sync with Expr.
The match arms cover all 14 current Expr variants with no wildcard, so this will fail to compile (good) if Expr grows a new variant without updating this helper — that's a reasonable non-silent-failure design. Just flagging for awareness since it's a second source of truth for spans alongside whatever span access already exists on Expr variants individually.
🤖 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/edict-syntax/src/parser.rs` around lines 1581 - 1599, The expr_span
helper currently duplicates the Expr variant list and must stay exhaustive; keep
its match arms aligned with Expr in parser.rs so any new variant is handled here
too. If you add or rename an Expr variant, update expr_span alongside the enum
to preserve the compile-time exhaustiveness check and avoid a stale second
source of truth for spans.
1 participant
Summary
This PR lands PR 1 of the obstruction-strands plan: source parser support only.
It adds first-class parsing for:
The new form is represented as
RequireElseArm::ContinueObstructed, requires exactly onereasonfield, rejects duplicatereasonfields, and is contextual to arequire ... elsearm. Existing terminalrequire ... else SomeObstruction;behavior remains terminal, and helper-shaped constructors such ascontinueInObstructedStrand(...)do not acquire hidden control-flow semantics.Closes #118
Refs #116
Claim Delta
New in this PR:
else continue obstructed { reason: ... }.reasonfields reject with stable parser error kinds.Still not claimed:
Claims Checklist
RED/GREEN Evidence
RED:
cargo test -p edict-syntax --test parse_review_regressions continue_obstructedObserved failure before implementation: missing
RequireElseArm, missingStmt::Require { arm }, and missingParseErrorKind::{MissingRequiredField, DuplicateField}.GREEN:
Notes