Skip to content

feat(compiler): add Workshop backend slices - #82

Merged
Teakowa merged 1 commit into
mainfrom
codex/issue-37-workshop-backend
Aug 28, 2026
Merged

feat(compiler): add Workshop backend slices#82
Teakowa merged 1 commit into
mainfrom
codex/issue-37-workshop-backend

Conversation

@Teakowa

@Teakowa Teakowa commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Scope

This is a bounded partial delivery for #37. It deliberately does not claim optimizer/replacement transformation parity, so it references rather than closes #37.

Verification

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo test --workspace --all-targets --all-features
  • python3 -m unittest discover -s compatibility/tests

Refs #37

@Teakowa Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One-pass audit against #37 and the current repository state. Verdict: changes required; do not merge as-is.

Blockers

  1. The branch is not review/merge-clean against current main. PR #78/#42 was squash-merged as 1a434e4, but this branch still carries the pre-squash #78 commits (d18fc2b, c2877ae, 7eb81ed) and then adds c5d24ce. GitHub reports the branch as diverged and the PR as non-mergeable, and the diff repeats already-merged #42 work. Rebase/cherry-pick the #37-specific work onto current main so the PR contains only its intended delta and does not risk dropping intervening mainline changes.

  2. Fixes #37 is not satisfied by the implementation shown here. #37 is the authoritative backend issue for semantics/settings-emission, preprocessing/directive-effects, hooks/post-compile-workshop, translations/locale-emission, and optimization/backend-effects. Its scope explicitly requires evidence-backed observable optimization/replacement/directive effects, rule-prefix/presentation handling at the backend boundary, and minimized integration fixtures for settings, locale, backend directives, and hooks. This compiler path does not consume hir.preprocessing.optimization or replacements, and the docs still leave observable optimization/replacement effects as Coming soon. Either complete the approved #37 scope or have the owning planning decision revise the issue/closure contract; this PR must not close #37 as-is.

    The dependency gate is also unresolved: #37 requires #36 complete. #36 is still open and still records #42 as its active edge even though #42 has since closed. Revalidate/close that dependency (or explicitly revise #37) before merging a PR that closes #37.

Major

  1. Post-compile-hook diagnostics are flattened and lose the structured script provenance the runtime already provides. MacroError::Script carries source_name, line, column, and adjusted stack information, but compile_source converts every hook failure through error.to_string() and retains only the OPY directive span. That contradicts this PR's “preserve hook diagnostics” claim and #37's structured/actionable provenance AC. Preserve the hook runtime's structured provenance instead of discarding it; the directive span can remain the OPY-side anchor/related location.

  2. The unsupported-locale diagnostic fabricates OPY source provenance. compile_hir_with_locale receives locale as an external compiler argument, but on an undeclared locale it attaches the span of hir.preprocessing.translations. The matrix itself describes this feature as the --language output selection contract; a #!translations declaration is not the source of an invalid external locale argument. Unless locale selection is actually derived from that directive, this error should not point at it. Keep the span absent at this layer (or attach option provenance at the caller that owns the option) rather than blaming unrelated OPY source.

  3. The support-state upgrades outrun the executable evidence currently recorded. end-to-end-supported is defined as explicit OPY → HIR → WIR → validated Workshop emission evidence for the declared scope, but translations/locale-emission is advanced using only upstream data plus the existing en-US oracle invocation; this PR adds a negative undeclared-locale unit test, not a positive non-en-US localized emission probe. The settings test only checks a few substrings even though a pinned settings oracle already exists, and the hook row's evidence list does not reference the new compiler-level final-Workshop integration. In addition, the matrix snapshot still says #!postCompileHook execution is lowering-dependent while the row now says end-to-end-supported. Keep claims at the lower state until the affected executable evidence exists, or add the focused evidence required by #37 and update the snapshot/provenance consistently.

Other audit areas

The ownership direction is otherwise correct: OPY/backend behavior stays in opy-rs, while settings/catalog/localization/WIR/emission remain delegated to released workshop-rs; I found no reverse dependency or copied canonical Workshop data in the #37-specific work. Hook execution is correctly placed after successful Workshop emission and its completion string is separated from the pre-hook emitted artifact.

No CI/workflow runs are currently attached to head c5d24ce, so the verification commands in the PR body are author-declared evidence only at this point. Re-run the repository gates after the branch is repaired.

@e54-bot
e54-bot force-pushed the codex/issue-37-workshop-backend branch from c5d24ce to 007d877 Compare August 28, 2026 12:12
@e54-bot e54-bot changed the title feat(compiler): integrate Workshop backend behavior feat(compiler): add Workshop backend slices Aug 28, 2026
@e54-bot

e54-bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Addressed in 007d877.

Local verification rerun: fmt, clippy with -D warnings, full workspace tests, and compatibility Python tests. Hosted CI is running on 007d877.

@Teakowa Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up one-pass audit on 007d877. The previous branch-history/#36 dependency/hook-diagnostic/locale-provenance findings are resolved, the PR is mergeable again, and the current CI run is green. Verdict: changes still required; do not merge this head as-is.

Blockers

  1. Pure optimizer controls are now turned into hard compilation failures without the evidence #37 requires. reject_unlowered_directives() rejects every non-default OptimizationState, so a valid OPY project using e.g. #!disableOptimizations no longer reaches canonical Workshop emission. That conflicts directly with #37's scope: unsupported optimizer/output-form differences are to remain documented non-blocking presentation differences unless observable semantics require them. The current patch has not established such an observable-semantic requirement; it only establishes that there is no canonical optimizer lowering yet. Do not make unsupported optimizer shape/output-form controls correctness blockers by default. Keep those controls non-blocking unless corpus/oracle evidence demonstrates a semantic requirement, while separately failing explicit semantic replacement directives where silently ignoring them would change behavior.

  2. The fail-explicit guard only checks the final aggregate preprocessing state, so scoped/include-local backend effects can still be silently ignored. Opy HIR intentionally retains DirectiveRecord entries with scope_depth and a PreprocessingSnapshot because block-scoped state transitions must remain inspectable. reject_unlowered_directives() checks only preprocessing.replacements.first() and the final preprocessing.optimization. A replacement/optimizer state introduced inside an include or lexical scope and restored before the end can therefore disappear from those final fields while remaining in preprocessing.directives; compilation then proceeds without applying or rejecting that backend effect. That contradicts this PR's stated contract (“reject ... instead of silently ignoring”) and #37's no-silent-fallback AC. For effects that are semantically relevant enough to reject, evaluate the ordered/scoped directive records rather than only the final snapshot. For presentation-only optimizer controls, apply finding 1 instead of rejecting them.

Major

  1. The settings Oracle assertion can hide semantic string changes. normalize_workshop_whitespace() removes all whitespace characters before comparing the emitted settings section with the pinned Oracle. That also removes whitespace inside quoted user values, so "a b" and "ab" compare equal. Since this test is being used as the executable basis for advancing semantics/settings-emission to end-to-end-supported, normalization must preserve string contents. Normalize only structural formatting outside literals, or compare a settings-aware semantic representation that preserves values.

Minor

  1. The support-matrix snapshot provenance is internally stale. The snapshot note now claims the #37 integration slice, while snapshot.date remains 2026-08-24 and asOfCommit remains c5714a4..., which predates this PR. Either advance the snapshot provenance to the state actually described by the note or keep the frozen snapshot note limited to what that commit/date represents.

Resolved from the previous review

  • Branch is rebased/clean enough for GitHub to report mergeable: true.
  • #37 is now correctly treated as an open parent/partial delivery (Refs #37), not closed by this PR.
  • #36 was revalidated and closed.
  • Post-compile hook errors retain structured script provenance in addition to the OPY directive anchor.
  • Unsupported external locales no longer fabricate a #!translations source span.
  • Positive non-English locale and compiler-level hook/settings coverage were added.
  • Ownership remains correct: OPY/backend-specific behavior stays in opy-rs; canonical settings/catalog/localization/WIR/emission stay delegated to workshop-rs.

After the two directive-handling blockers are corrected, re-run the existing repository gates; no additional broad test expansion is needed beyond fixing the settings assertion and adding a focused scoped-directive regression if that failure mode is not already covered.

@e54-bot
e54-bot force-pushed the codex/issue-37-workshop-backend branch from 007d877 to 9f7c62b Compare August 28, 2026 13:41
@e54-bot

e54-bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Addressed review 5051072214 in 9f7c62b.

  • Optimizer controls no longer block compilation; they remain documented non-blocking presentation controls.
  • Semantic replacement directives are detected from ordered DirectiveRecords, rather than only the final aggregate state; a focused regression clears the final replacement snapshot and still verifies source-anchored rejection.
  • Settings formatting normalization now strips structural whitespace only and preserves quoted values, with a direct "a b" != "ab" regression test.
  • The frozen matrix snapshot note has been restored to its recorded 2026-08-24 / c5714a4 provenance; current Integrate Workshop-dependent settings, locale, directive, and hook behavior #37 state remains in individual feature rows.

Reran fmt, Clippy with -D warnings, all workspace tests, and compatibility tests. Hosted CI is running on 9f7c62b.

@Teakowa Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up audit on 9f7c62b.

Verdict: changes still required. The previous optimizer/scoped-state/settings-comparison findings are substantially fixed, and the branch/CI/ownership picture is now clean, but one compiler-boundary correctness hole remains and the support artifact is internally contradictory.

Blocker

B1 — compile_hir can still silently ignore an active replacement state when directive history is absent

reject_unlowered_directives now only scans hir.preprocessing.directives for names beginning with replace. This correctly catches scoped replacement directives whose final state was later restored, but the previous active-state guard was removed entirely.

PreprocessingState exposes replacements and directives as separate serde-defaulted fields. directives is provenance/history; it is not a structural prerequisite for a non-empty replacements state, and HIR validation does not establish an invariant that the two must agree. Therefore a deserialized/constructed hir::Program accepted by the public compile_hir boundary can contain a non-empty active preprocessing.replacements with an empty/missing directives list. This path now reaches lowering and emits Workshop while silently dropping a semantic replacement that this PR explicitly says must fail rather than be approximated.

Please reject the union of (a) active replacement state and (b) historical replacement directive records. The history check is needed for restored scoped state; it should supplement, not replace, the active-state check. A boundary regression test with replacements != [] and directives == [] would protect the actual failure mode.

Major

M1 — compatibility/support-matrix.json still describes the pre-#37 state at the snapshot level

The feature rows in this PR promote settings, locale emission, and post-compile-hook execution to end-to-end-supported, but the same file's snapshot is still dated 2026-08-24, points at c5714a4028464f19f236f54d390e7b7237e434bd, and its note still states that #!postCompileHook execution against final Workshop text is lowering-dependent (#8).

That is now a direct contradiction inside the authoritative support artifact, not just stale cosmetic metadata. Update the snapshot provenance/note to the state this PR is actually claiming (or regenerate it through the repository's intended path) so consumers do not get two incompatible answers from one matrix.

Previous findings / audit notes

  • Resolved: optimizer-only controls are non-blocking again, matching #37's explicit compatibility boundary.
  • Resolved: replacement directive history is now checked, so scoped/restored replacement effects are not silently lost on the canonical source path.
  • Resolved: the settings Oracle comparison no longer erases whitespace inside string literals; the regression assertion distinguishes "a b" from "ab".
  • Resolved: #36 is closed; #82 correctly remains a bounded partial delivery and uses Refs #37 rather than closing #37.
  • #!rulePrefix / #!rulePrefixTemplate are applied in OPY lowering before Workshop compilation; no duplicate Workshop semantics were introduced here.
  • Settings/catalog/locale/WIR/emission ownership remains delegated to workshop-rs; no reverse dependency or copied canonical Workshop table was found.
  • Post-compile hooks still execute only after successful canonical Workshop emission and retain both directive and script-runtime provenance.
  • Current head is mergeable and CI run #162 is green.

I did not find additional objective findings in the changed docs, dependency changes, support-matrix mirror, public hook record change, or CI surface.

@e54-bot
e54-bot force-pushed the codex/issue-37-workshop-backend branch from 9f7c62b to 98e14af Compare August 28, 2026 16:38
@e54-bot

e54-bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Addressed review 5051776859 in the updated head.

  • compile_hir now rejects the union of active preprocessing.replacements and historical replacement directive records. When both exist, it uses the directive record span; constructed/deserialized active state without history still fails explicitly.
  • Added the exact boundary regression: non-empty replacements with directives = [] and no source span is rejected rather than reaching Workshop emission.
  • Advanced both support-matrix snapshots to 2026-08-29 / 9f7c62b and aligned the note with the bounded Integrate Workshop-dependent settings, locale, directive, and hook behavior #37 feature rows.

Reran fmt, Clippy with -D warnings, all workspace tests, and compatibility tests. Hosted CI is running on the new head.

@Teakowa Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Final follow-up audit on 98e14af against #37 and the current repository state.

Verdict: no blocking code findings on this head. The previously reported compiler-boundary and support-artifact issues are resolved.

Verified

  • Active replacement state and historical/scoped replacement records are both rejected, so compile_hir no longer has the replacements != [] && directives == [] silent-fallback hole. The focused boundary regression covers that inverse case.
  • Optimizer-only controls remain non-blocking presentation differences, matching #37's explicit compatibility boundary.
  • The support-matrix snapshot no longer contradicts the promoted settings/locale/hook rows; the bounded #37 slice and remaining optimizer/replacement gaps are described consistently.
  • Settings still delegate validation/emission to workshop-rs, and the Oracle comparison preserves quoted-string contents.
  • Locale selection remains catalog-driven with explicit unsupported-locale behavior.
  • #!postCompileHook still runs only after successful canonical Workshop emission, receives the emitted text, and preserves directive plus script-runtime provenance on failure.
  • #!rulePrefix / #!rulePrefixTemplate stay OPY-owned and are applied before Workshop lowering; no Workshop semantic tables were duplicated in opy-rs.
  • #82 remains a bounded partial delivery and correctly uses Refs #37; #37 is still open and should remain open for its remaining approved scope.
  • The PR is currently mergeable and the diff remains scoped to the compiler/support/docs slice.

CI

CI run #163 is still in progress at review time. Rust 1.85 quality, Rust stable quality, and macOS JS-runtime jobs are green; only the Windows JS-runtime job is still running. Merge should wait for that final hosted gate to complete successfully.

I found no additional Blocker/Major/Minor findings in the changed dependencies, public hook record, compiler API, support-matrix mirror, or documentation surface.

@Teakowa
Teakowa merged commit dad623f into main Aug 28, 2026
4 checks passed
@Teakowa
Teakowa deleted the codex/issue-37-workshop-backend branch August 28, 2026 17:29
This was referenced Aug 28, 2026
@e54-bot e54-bot mentioned this pull request Aug 29, 2026
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.

Integrate Workshop-dependent settings, locale, directive, and hook behavior

2 participants