An agent skill for gx, and the round-trip fix found while writing it - #48
Closed
jpablo wants to merge 4 commits into
Closed
An agent skill for gx, and the round-trip fix found while writing it#48jpablo wants to merge 4 commits into
jpablo wants to merge 4 commits into
Conversation
`DiagramText.render` called the DOT printer directly instead of going through
`ViewerGraph.viewerGraphToText`. The printer is the last of three steps, and
calling it directly skipped the other two:
- `combineStyleAttributes` folds the synthetic style sub-attributes back into
a real `style="filled"`. Without it `gx` wrote `fillstyle="true"` into the
user's file, and since that is not DOT, the reader rejected gx's own
output. Two commands were enough to break a diagram:
gx run g.dot set-attribute --params \
'{"targets":["node:a"],"name":"fillcolor","value":"red"}'
gx run g.dot list-nodes
gx: could not parse the diagram: assertion failed
- `graph.id` and `graph.tpe` carry the graph's name and whether it is
directed. Defaulting them rewrote `graph MyNet { a -- b }` into
`digraph "G" { "a" -> "b" }`, which is a different diagram, and said
nothing about it.
The reader's `assert` that no sub-attribute ever reaches it was detecting a
real defect, but it named neither the attribute nor the element, and files
written by a released gx are already on disk. It now drops stray
sub-attributes, which is what `dot` does with an attribute it does not know;
honouring `fillstyle` would make the viewer paint a fill graphviz would not.
Such a file loses its fill on load, which is the truthful outcome: it renders
the way `dot` renders it.
DiagramTextRoundTripSpec asserts the property — parse -> render -> parse —
rather than the printed text. Five of its six tests fail without this change.
`.claude/skills/gx/SKILL.md` teaches a coding agent to drive gx: the three tiers and which of them need a desktop, how a ref resolves, that element refs must be read from `list-*` rather than constructed (arrow ids carry an index, and a group id is not the cluster name), the exit codes, and the fact that a document mutation reprints the whole file rather than patching it. Written against the running binary — every command in it was run and its real output pasted, which is how the exit code for a malformed ref turned out to be 1 and not 4. `gx skill [<version>] [--latest] [--json]` prints where that skill lives and the sentence to hand an agent. It deliberately does not install it: a skill is a prompt someone's agent will load and act on, so writing it into their agent directory should be a decision rather than a side effect of asking where it is — and every harness keeps skills somewhere different anyway. A location plus the instruction works for all of them and leaves the human in the loop. The location is pinned to the running binary, because the skill names commands, param keys and exit codes, all of which are API that moves between releases: an agent reading the branch tip while driving an older gx would be reading about commands it does not have. A dev build has no tag to point at, so it falls back to the branch and says out loud that it is not pinned, suggesting the release it was cut from.
`.claude/skills/gx/` is where a skill is INSTALLED for one harness. It is the wrong place to publish one from: the Agent Skills format is read by Claude Code, Codex CLI, Cursor and others, and each keeps installed skills somewhere different. The skill now lives at `skills/gx/`, and where it goes on the receiving end is named in the instruction `gx skill` prints rather than baked into the path it is served from. Split for progressive disclosure, which is the point of the format: a runtime loads the frontmatter always, the body when the skill is relevant, and the supporting files only when a step needs them. SKILL.md keeps what an agent must know before it touches a diagram — the tiers, ref resolution, the reformat hazard, conflict safety — and 178 lines of lookup move out: commands.md every command, its params, and the exit codes library.md import/bind/sync modes, watching, the filesystem sandbox Frontmatter stays inside the spec's six permitted fields and gains `license` and `compatibility`; anything else (an `argument-hint`, say) is a hard error when the skill is packaged or uploaded, not an ignored key. Two tests guard what nothing else would notice, since a skill is an asset no Scala code imports: that the location `gx skill` advertises actually exists with the supporting files SKILL.md links, and that the frontmatter carries no non-portable key. Both fail when broken — the second was checked by adding `argument-hint` and watching it name that key.
Found by running the skill against fresh agents. Two of them were given the same task — hide a node — with and without the skill. The one without it ran `gx ls`, saw a diagram named `infra`, hid the node on it, verified against it, and reported success. That record was bound to a different file in a different directory; its own file was never imported and never touched. Nothing in the tool misbehaved. Ref resolution reports *ambiguity*, and one exact name match is not ambiguous — it is just possibly the wrong file. The library is global while a task is local, so a bare name is not a safe address for a record you did not import yourself. Both files now say so: check the `origin` before acting on a library ref, or address the path and sidestep the question.
✅ Deploy Preview for graph-explorer-net ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jpablo
added a commit
that referenced
this pull request
Aug 22, 2026
…anch #48 landed on viewer while this branch was open and touched the same two files this branch rewrote: Cli.scala and CliSpec.scala. Cli.scala auto-merged and is semantically clean: #48 added a `skill` command — a new dispatch arm plus a self-contained handler that goes nowhere near ref resolution, sync or watch, which is all this branch restructured. Verified the merged dispatch carries all thirteen arms and that resolveRef / selectedRefs / SyncOutcome / reportRef all survive. CliSpec.scala had one conflict, in the import block, where both sides added imports: `Paths` and `scala.jdk.CollectionConverters.*` from #48, and `PosixFilePermissions` from this branch's unwritable-store test. Resolved as the union — no test on either side changed. Full suite on the merged tree: 0 failures. gx-cli is 81 tests (71 here + 10 from #48) and gx-core 181 (175 + 6), so both sides' tests are present and passing rather than one side having quietly displaced the other.
Owner
Author
|
Already landed on Verified before closing:
So |
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.
Two related changes: an Agent Skill that teaches a coding agent to drive
gx, and agxbug the writing of it uncovered.The skill
skills/gx/— written against the running binary rather than the source. Every command in it was run and its real output pasted, which is how the exit code for a malformed element ref turned out to be 1 rather than 4.Laid out for the portable Agent Skills format, not
.claude/skills/: that path is where a skill is installed for one harness, and the format is read by several. Split for progressive disclosure — the runtime loads frontmatter always, the body when relevant, supporting files only when a step needs them:SKILL.mdcommands.mdlibrary.mdFrontmatter stays inside the spec's six permitted fields. Anything else is a hard error when the skill is packaged or uploaded, not an ignored key — so a test asserts it, and another asserts the location
gx skilladvertises actually exists with the files SKILL.md links. Both fail when broken.gx skillPrints where the skill lives and the sentence to hand an agent. It deliberately does not install it: a skill is a prompt someone's agent will load and act on, so writing it into their agent directory should be a decision rather than a side effect of asking where it is.
Pinned to the running binary, because command names, param keys and exit codes are API that moves between releases. A dev build has no tag to point at, so it falls back to the branch and says out loud that it is not pinned.
The bug
DiagramText.rendercalled the DOT printer directly instead ofViewerGraph.viewerGraphToText, skipping the two steps in front of it. Two commands were enough to break a diagram:combineStyleAttributesfolds the synthetic sub-attributes back intostyle="filled"; without itgxwrotefillstyle="true"— not DOT — and then rejected its own output. The same bypass defaultedgraph.idandgraph.tpe, silently rewritinggraph MyNet { a -- b }intodigraph "G" { "a" -> "b" }.The reader's
assertwas detecting a real defect but named neither the attribute nor the element, and files written by a releasedgxare already on disk. It now drops stray sub-attributes, asdotdoes with an attribute it does not know.Testing
DiagramTextRoundTripSpecasserts the property (parse → render → parse) rather than the printed text. Five of its six tests fail without the fix.Note
library.mddeliberately does not mentionGX_HOME; that lands on a separate branch. Worth a line here once both are merged.