guard: dynamically-ish typed userspace proof of concept#5668
Conversation
This comment has been minimized.
This comment has been minimized.
arthyn
left a comment
arthyn
left a comment
There was a problem hiding this comment.
lgtm this is the beginning of ruby on rails for gall
Adds a +groups!rail generator, which generates a file, intended to live at /lib/rail.hoon, which imports all the desk's marks and incorporates them into the $rail type and its helpers.
Many marks didn't compile into proper mark cores. Here, we make a bunch of changes to correct this.
The discipline mark set lets us convert vases into well-typed rails. (But does not take away crash risk due to poorly typed vases.) We make chat use it right away to deduplicate its mark list. The generator contains a list of marks marked as non-strict for the purpose of mark-type-changed checking. We also add a conversion from cage to rail, paving the way for further guardian work.
This makes the argument contain rails instead of cages, for better compile-time type safety. Using this you cannot unpack a vase from a cage into a type different from that of its mark. Applies both to /app/chat, athough +on-agent doesn't do much there: chat doesn't do subscriptions, so we don't handle fact cages anyway.
Stubs out some of the hooks stuff, whose types have been drifting.
REVIEW: this didn't used to have discipline on it, but now its marks are implicitly strictly disciplined.
Fang-
left a comment
Fang-
left a comment
There was a problem hiding this comment.
Ready enough for review.
Some stuff highlighted for extra-care review. There was more here, but #5764 resolved most of it!
Note about the discipline stuff: I'm not a fan. Needing to explicate every mark that should be non-strict-ed (in the rail generator) is probably not the modality we want.
With some rejiggering we could probably move back to agents specifying the marks which they do want strict-checked, whilst making /lib/discipline pull in the mark types from the /lib/rail instead of depending on agents to pass them.
afaic I'll consider that an outstanding TODO, but I'm open to hearing feedback/ideas.
Note about other libraries: lib-negotiate and lib-subscriber are quite low impact wrt this. lib-logs is a bit rougher, but I'm also hesitant to make that always produce type-safe cards. afaic the current situation is fine, especially if we clean the local patches up just a tad. But open to feedback.
| @@ -0,0 +1,14 @@ | |||
| /- gv=groups-ver | |||
| /+ j=groups-json | |||
| |_ =foreigns:v10:gv | |||
There was a problem hiding this comment.
Review: this was missing even though groups was using it for its latest foreigns scry endpoint. Correct to add this, yes?
There was a problem hiding this comment.
Yes, it should have been included! We should also remember to update the discipline while we are at it; looks like discipline for the singular foreign-2 mark is also missing.
| %groups | ||
| =/ =cage group-action-4+!>(`a-groups:v7:gv`a-groups.effect) | ||
| (emit [%pass /hooks/effect %agent [our.bowl %groups] %poke cage]) | ||
| =/ =rail !! ::group-action-4+`a-groups:v7:gv`a-groups.effect ::REVIEW |
There was a problem hiding this comment.
This and the other nearby cases suggest hook implementation has drifted wrt the types. @arthyn confirmed. We'll probably want to clean these up somehow. Could be trivial, I haven't given it a serious try yet.
There was a problem hiding this comment.
The two main problems with hooks is that (1) they are not in real use and (2) we have never sat down to describe what should happen when we update hook type dependencies. I think they should probably use current types always and attempt to recompile when types change. The proper solution here is out of scope, but we should somehow make the types consistent so it is still possible to use hooks going forward.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
![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: b46ff7916c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| %groups | ||
| =/ =cage group-action-4+!>(`a-groups:v7:gv`a-groups.effect) | ||
| (emit [%pass /hooks/effect %agent [our.bowl %groups] %poke cage]) | ||
| =/ =rail !! ::group-action-4+`a-groups:v7:gv`a-groups.effect ::REVIEW |
There was a problem hiding this comment.
In run-hook-effects, the %groups, %dm, %club, and %contacts branches bind rail to !!, which always crashes before emitting the poke. Any hook effect of those kinds will therefore terminate this path and fail to dispatch the downstream action, breaking hook-driven automations for those targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed all library files. Answered the questions.
The only substantial question I have thus far is regarding not having a dedicated mark exclusion list in the code generator.
We should also accompany, or follow this PR with a short documentation about this pattern (a few terse paragraphs, with a few code samples) , given that it substantially alters the usual gall agent api. This is not a blocker for the release obviously, but would be good to have to aid models in comprehending our codebase.
| :: specifies a $rail type, a "typed $cage" containing all the desk's marks | ||
| :: and a +de-rail helper for turning that back into a $cage. |
There was a problem hiding this comment.
In what sense the $rail type is a "typed $cage"? It would rather seem be a typed $page. The %unsafe variant carries a vase, but all others just carry simple values, not vases.
There was a problem hiding this comment.
Perhaps it's more nuanced to say "statically typed $cage"?
The thinking is that the $rail gets put in all the places we would otherwise put $cages (pokes, facts), and that they're now typed in such a way (dynamically) that the compiler can see the types. Vases always have a single (static) type. They contain the information about the type of the data, but not in a way that's legible to the compiler.
| ::TODO investigate adding a "meta-build" for rune that evaluates the file | ||
| :: to obtain a hoon ast, which it then compiles into the result |
There was a problem hiding this comment.
| ::TODO investigate adding a "meta-build" for rune that evaluates the file | |
| :: to obtain a hoon ast, which it then compiles into the result | |
| ::TODO investigate adding a "meta-build" ford rune that evaluates the file | |
| :: to obtain a hoon ast, which it then compiles into the result |
| %activity-summary-1 | ||
| %activity-update | ||
| %activity-update-1 | ||
| %aqua-effect |
There was a problem hiding this comment.
It is not ideal that we have to care about the marks that are not going to be used in any of our agents, but I understand this is because we scan all the marks in the desk, and thus need to specify all exceptions to allow for future changes.
This makes me think whether we should come up with a similar wrapper for threads, but this would be out of scope and rather low-priority, since historically we have not such problems with threads.
There was a problem hiding this comment.
One other alternative with perhaps better semantics would be to have a mark exclusion list for marks that are not of interest, which there are some.
There was a problem hiding this comment.
we have to care about the marks that are not going to be used in any of our agents
We could consider removing them from our desk. %aqua-effect for example is only included here (and in the generated output) because we have a /mar/aqua/effect.hoon in the groups desk distribution.
Having a mark exclusion list would let us clean those out of the library before even cleaning up desks. (We certainly don't want to do desk cleanup while the library still depends on those marks!)
mikolajpp
left a comment
mikolajpp
left a comment
There was a problem hiding this comment.
I have reviewed agent integrations. Looked more closely at channels-server.hoon, and skimmed over the other agents. Any mistakes or omissions would cause certain compile errors, so while changes are extensive they only amount to replacing cages with rails and consequences.
It seems like these changes should not cause any failures in any of the tests, but based on the logs the desk fails to upgrade. We should investigate it locally before merging.
| =? old ?=(%0 -.old) (state-0-to-1 old) | ||
| =? old ?=(%1 -.old) (state-1-to-2 old) | ||
| =? cor ?=(%2 -.old) (emit %pass /trim %agent [our.bowl %chat] %poke %chat-trim !>(~)) | ||
| =? cor ?=(%2 -.old) (emit %pass /trim %agent [our.bowl %chat] %poke %unsafe %chat-trim !>(~)) |
There was a problem hiding this comment.
We use unsafe here because we don't have a corresponding mark?
Probably fine, given that this is historical. If gall will start to enforce marks on local pokes we will be able to easily find these cases with guardian.
More clearly indicates what's going on.
Fang-
changed the title
3 participants
/lib/guardattempts to detect mark-mismatching data at compile-time. We do this by loading in our marks and replacing the$cages in%pokeand%factcards with a$railtype, which is a%$over all the imported marks.This approach has some advantages:
+on-peekresults.%unsafe.Draft because this is incomplete (only chat agent uses it rn)and wants some additional questions answered.Do we just want to chuck every/marfile from our desk into/lib/guardian? You can also imagine per-agent lib files for this, and you may need to do that if you also specify API details, but rn we could get away with a single (huge) file.Doing so would worsen our "changing one file recompiles everything" problem though. But maybe that's a price we can live with for what we gain.Notably this already found a bad type in
+refs:migrate. (;