Skip to content

Enable postprocess in Crisp#131

Open
Crocodoctopus wants to merge 1 commit into
mainfrom
bg/enable-postprocess
Open

Enable postprocess in Crisp#131
Crocodoctopus wants to merge 1 commit into
mainfrom
bg/enable-postprocess

Conversation

@Crocodoctopus

@Crocodoctopus Crocodoctopus commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Introduces c2rust transpile postprocess to the Crisp loop. Postprocess occurs after transpile with a separate sandbox invocation. If postprocess fails, or the subsequent cargo check, we discard the node and fall back to just the transpile output. Postprocess runs with --on-error warn, to produce a best-guess output, so we get something on an imperfect run rather than nothing.

The key/base/model used by postprocess are inherited from CRISP_API_{KEY|BASE|MODEL} respectively.

Comment thread crisp/sandbox/docker.py Outdated

@thedataking thedataking left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please clean up commit history so you don't make changes in one commit that is later undone in a later commit before we review the non-draft version.

For example, you add and then remove a postprocess parameter to the transpile function. You probably planned on doing this anyway, sorry to belabor the point.

@Crocodoctopus

Copy link
Copy Markdown
Collaborator Author

Divided up the commit into two: one that bumps deps/c2rust (and its associated fixes), and the other that integrates postprocess. The postprocess commit requires immunant/c2rust#1838 before landing.

On the topic of forwarding CRISP_API_KEY as OPENAI_API_KEY to postprocess: I can do this, but it would mean if CRISP_API_KEY is set, we can't have postprocess use Gemini, unless OPENAI is forawrded explicitly as empty. We should probably introduce the equivalent API_KEY/API_MODEL to postprocess so its not as mess? I can get a PR going for that if you'd like. I can also add forwarding of --codex-login for support that as well, I have a prototype already thrown together for testing.

Thoughts?

Comment thread crisp/workflow.py
Comment thread tools/merge_rust/src/main.rs
@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch from c02264f to f6d731d Compare June 5, 2026 19:37
@Crocodoctopus

Copy link
Copy Markdown
Collaborator Author

This should be about good to go. Last thing I need to do is fix the commit message, and bump deps/c2rust, which is blocked until immunant/c2rust#1839, immunant/c2rust#1840, and immunant/c2rust#1841 land. I shouldn't change shape too much, unless any of those other PRs drastically change.

@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch from f6d731d to f34db9d Compare June 12, 2026 06:23
@Crocodoctopus

Copy link
Copy Markdown
Collaborator Author

Current this is blocked by immunant/c2rust#1851 and immunant/c2rust#1852. Once those two PRs are merged, I want to do one final test before this can be merged.

@thedataking thedataking left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Branden, please revisit this comment:

Couldn't we pass the CRISP_API_KEY to the postprocessor? If it is an OpenAI key (likely), the postprocessor should be able to use it provided that we tell the postprocessor to use the model selected in CRISP_API_MODEL.

I opened immunant/c2rust#1853 so c2rust-postprocess picks up the CRISP_API_MODEL/KEY/BASE env variables. With this change you should not have to copy env keys around here, everything should just work.

Please simplify and then test this PR with C2Rust PRs 1851 and 1853.

@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch 2 times, most recently from f6b5a3a to aa4b437 Compare June 13, 2026 01:13
@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch from aa4b437 to 4cda5b5 Compare June 13, 2026 02:02
@Crocodoctopus

Crocodoctopus commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for review. Tests are passing, I'm just mucking with the commit message.

Edit edit: nevermind, tests passing again after rebase.

Comment thread Dockerfile Outdated
Comment thread crisp/workflow.py
Comment thread crisp/workflow.py
Comment thread crisp/workflow.py Outdated
@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch 2 times, most recently from b252139 to 53fc942 Compare June 16, 2026 22:34
@Crocodoctopus

Copy link
Copy Markdown
Collaborator Author

If everything is acceptable, we can merge this.

Comment thread crisp/workflow.py
"--output-dir",
sb.join(output_path),
"--emit-build-files",
"--emit-c-decl-map",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is on the not hayroll arm of the conditional. How do you get this flag passed to the transpiler on the other arm?

@Crocodoctopus Crocodoctopus Jun 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Does Hayroll have support for this? I couldn't find anything.

Edit: My understanding is, hayroll=True transpiles invoke Hayroll, which invokes c2rust. I have two possible concerns:

  1. Hayroll does not appear to expose any way to forward --emit-c-decl-map.
  2. Can Hayroll transform the c2rust output in a way that breaks the decl maps? Codex suggests it can.

Comment thread crisp/__main__.py Outdated
Comment thread crisp/__main__.py
Comment thread crisp/workflow.py
n_op = EditOpNode.new(
mvir,
old_code=n_tree.node_id(),
new_code=n_new_tree.node_id(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I haven't looked closely into how this works but I wonder: we don't need the *.c_decls.json files after postprocessing. Do you do anything to stop carrying them around in the MVIR nodes after postprocessing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

They were just left before, I added a cleaning pass. Should keeping or cleaning them be decided by a flag?

@thedataking thedataking left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need postprocess to work independently of whether Hayroll is on or off; I'm not sure this is currently true.

@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch from 53fc942 to f4e5dc4 Compare June 18, 2026 21:28
Add support for a postprocess pass in Crisp. Postprocess parameters are
controlled via CRISP_* env variables.
@Crocodoctopus Crocodoctopus force-pushed the bg/enable-postprocess branch from f4e5dc4 to 0a6fc10 Compare June 18, 2026 21:35
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.

Enable c2rust-postprocess in Crisp

3 participants