feat(dsh-plugin): integrate Obelisk through a plugin-owned skill - #115
Conversation
…Harness ADR-0009 (Mode 0): one read-only obelisk_query tool plus a guidance section, mounting the cross-tool archive and memory layer beside the built-in session history search. The tool runs bounded JavaScript through the obelisk CLI sandbox, caps output, and stays a thin translation layer; the obelisk skill keeps owning query semantics. - packages/dsh-plugin: cordis plugin (tool + system prompt section), schemastery config, sample overlay patch, package README - tests/dsh-plugin.test.mjs + fake CLI fixture: registration, JSON round-trip, failure/stderr surfacing, non-JSON passthrough, output cap, spawn errors
Presentation-only frontend contribution per ADR-0009: the package now declares dsh.client (platform web) and ships a lib/client.js bundle that registers a keyed toolview for the obelisk_query wire name — monolith glyph, Obelisk label, bounded query summary, expandable QUERY/RESULT sections on DSW theme tokens. Invariants: the model-facing surface and the durable session record are unchanged; only the human-facing card differs, and only the obelisk_query key is claimed. Skill-driven bash invocations keep the standard row (documented; conditional takeover needs a future generic capability in the harness). - src/client: row-model (pure derivation), obelisk-row card, slot registration, CSS module - tsdown.config.ts: self-contained client bundle build (closure-factory contract + inline CSS modules, adapted from deepseek-harness MIT preset) - tests/dsh-plugin-client-model.test.mjs: 6 model tests - README: frontend-presentation section and rc.6 alignment fix
Supplementary decision: the plugin carries its own browser half via the dsh.client dual-face mechanism and styles first-party obelisk_query calls. Three invariants are recorded — model-facing surface unchanged, durable session record unchanged, only the obelisk_query key claimed — and the bash-invocation recognition is deferred behind a future generic keyed-select capability in the harness.
tommy0103
left a comment
There was a problem hiding this comment.
Review: request changes
The overall direction is sound — the runtime-skill registration approach, the precedence design (project-local > plugin > user-global), and the decision to stay on the standard Bash path are all well-reasoned, and ADR-0012 documents the contract clearly. A few things need fixing before merge:
Blocking
-
The new test never runs in CI.
.github/workflows/cli.ymlenumerates test files explicitly, andtests/dsh-plugin.test.mjswas not added. Localnpm testpicks it up via thetests/*.test.mjsglob, but CI won't — this violates the verification contract in CONTRIBUTING.md, and #113 set the precedent of adding new test files to the workflow. Please add it. -
The packaged
dist/skillpath is untested, despite ADR-0012's Verification section promising build/pack verification.tests/dsh-plugin.test.mjsimportssrc/index.tsand assertsresourceBaseagainst the sourcepackages/dsh-plugin/skilltree, so thePACKAGED_SKILL_ROOTbranch (dist/index.js→dist/skill) can never be exercised. Compounding this,skillRoot()/bodyUrl/resourceBaseare computed at module load (src/index.ts:78-80): in a published tarball (files: [dist, obelisk.cordis.yml]), a missing or staledist/skillsilently falls back to a nonexistent../skill, and the failure surfaces later as an opaque ENOENT insideapply(). Please compute these lazily inapply()(or fail fast with a clear error) and add a test that builds the package and loadsdist/index.js.
Non-blocking suggestions
- Precedence test uses assumed ranks. The duplicate-name test mocks competitors with hardcoded
rank: 500/rank: 100; the real guarantee depends on DSH's shipped filesystem provider ranking project roots below 250 and user roots above. The test proves registry mechanics, not precedence against the real discovery path the ADR's Context section motivates. Consider asserting against the actual rank constants fromdsh-skill, or at least adding a comment pinning the assumption. - Two dangling reference files ship in the bundle.
skill/references/recap-patterns.md(93 lines) andskill/references/recap-writing.mdare compatibility-pointer stubs referenced by nothing in the tree (SKILL.md →recap/overview.md→ per-card files only). They're verbatim copies inherited from theskill-doc/seed, but since the plugin now owns this copy, either link them or drop them. - Housekeeping:
- Root
tsconfig.jsonincludeenumerates each package'ssrcexplicitly;packages/dsh-plugin/srcwasn't added (currently only covered transitively via the test import). packages/dsh-plugin/tsconfig.jsonsets"erasableSyntaxOnly": false, overriding the root'strue, but the code uses no erasable-syntax constructs — suggest dropping the override.'@obelisk/dsh-obelisk-plugin'appears as two separate literals insrc/index.ts(nameandPROVIDER_NAME).- Metadata nits:
version: "0.1.0"vs the0.0.0convention for private packages;"AGPL-3.0-only"vs the root/cli"AGPL-3.0". - Three commit messages still reference "ADR-0009", which on main is a different document; not worth rewriting history over, but worth knowing.
- Root
Minor note: the hand-rolled frontmatter parser in parseSkill and the test's bodyOf regex are two independent implementations whose agreement is asserted, not guaranteed — acceptable for now, but they diverge on edge cases like --- inside a YAML block scalar.
Verified locally against current main (4d5a620): diff touches only packages/dsh-plugin/**, tests/dsh-plugin.test.mjs, docs/adr/0012-*, and package-lock.json; no DSH code is modified, and the earlier obelisk_query browser card from 6819f04 was fully removed by 0ea4d2a. Once the two blocking items are addressed this looks good to merge.
|
Thanks for the careful review. I addressed the two blocking gaps in two focused commits:
Final-head verification includes I kept The PR description now reflects the packaged-runtime checks and the intentionally owned plugin skill tree. Ready for re-review. |
|
Follow-up correction: This supersedes the removal noted in my previous reply. |
tommy0103
left a comment
There was a problem hiding this comment.
Review: approve
Re-reviewed at a889eb8. Both blocking items from my previous review are resolved, and I verified locally in a clean worktree: build succeeds, tests/dsh-plugin.test.mjs passes 8/8 (including the two new tests covering the packaged dist/skill path and the clear-failure case), the full suite passes 554/554, and both root and workspace typechecks are clean.
Specifically:
- CI now builds the plugin and runs its test explicitly, and the root
pretestkeeps localnpm testworking. ✓ skillRoot()is now computed lazily inapply()and fails fast with a clear message naming both expected paths instead of silently falling back. ✓- The rank assumptions are now pinned as named constants with a comment citing
@deepseek-ai/dsh-skill-filesystem 0.0.1-rc.1, and all four ranks are exercised. ✓ - Root
tsconfig.jsonincludespackages/dsh-plugin/src, the redundanterasableSyntaxOnlyoverride is gone, and the duplicated package-name literal now reusesname. ✓ - Moving the build to
scripts/build.mjsis a nice portability improvement over therm -rf/cp -Rshell one-liner.
Two minor leftovers, not worth another round — fine to clean up here or in a follow-up:
skill/references/recap-patterns.mdandrecap-writing.mdare still unreferenced by anything in the tree but ship in the bundle.- Metadata nits remain:
version: "0.1.0"vs the0.0.0convention for private packages, and"AGPL-3.0-only"vs the root/cli"AGPL-3.0".
LGTM.
This PR incorporates the work proposed in #61 and revises it into an upstream merge candidate based on the current
mainbranch. It preserves the original product goal—making Obelisk available inside DeepSeek Harness—while keeping Obelisk's retrieval contract consistent across agent harnesses and giving the DSH integration a deliberate lifecycle of its own.Summary
@obelisk/dsh-obelisk-pluginpackage with a plugin-owned DSH-facingobeliskskillskilltoolDiscussion
The implementation in #61 was written before the Obelisk session adapter for DSH existed. It came from a constrained prompt-driven exercise intended to demonstrate three declared DSH plugin aspects without first inspecting DSH source: register a tool, append model guidance, and add a branded browser card. The generated implementation was mechanically coherent with those prompts, but the visual goal ended up deciding the model protocol. The shared skill taught Bash while the plugin introduced a second invocation path with its own temporary-file handling, synchronous subprocess, timeout, truncation, and error semantics.
During review we separated the cross-agent contract from DSH-specific presentation. The dedicated tool had mainly become necessary because a tool name could own a distinctive frontend renderer. DSH currently cannot let an external plugin decorate only selected Bash calls, and identifying Obelisk inside arbitrary shell pipelines would not provide a reliable attribution boundary anyway. We therefore chose consistency over branding: this revision accepts the ordinary Bash presentation instead of changing DSH, copying its Bash renderer, or keeping a second tool identity.
The review also exposed a less visible integration boundary. A user may already have installed a global
obeliskskill for another agent. DSH can discover that same name from its user skill roots, so merely bundling another candidate would leave the selected instructions dependent on filesystem precedence. The plugin now registers its own DSH-facing definition at runtime: it intentionally shadows a user-global copy while still allowing a project to make an explicit local override.Those choices require product and architecture judgment beyond satisfying an implementation checklist. Obelisk exists to help people recover the decisions behind prior work; this integration should follow the same discipline by separating presentation, model protocol, ownership, and precedence instead of allowing whichever adapter mechanism is easiest to become the architecture.
The resulting decisions are:
skilltool handle discovery and loadingImplementation
@obelisk/dsh-obelisk-plugindepends only on DSH's standardskillsservice. It reads the packagedskill/SKILL.md, registers the parsed definition throughctx.skills.register(...), and exposes the complete plugin-owned skill directory as its resource base. The checked-in copy is byte-for-byte identical to the canonical Obelisk skill at introduction; it is checked in under the plugin so future DSH-specific guidance is reviewed as part of the integration rather than selected implicitly from a user's global installation.The build copies that tree to
dist/skill/. Runtime path resolution is lazy and checks the packaged tree first, so a malformed publication fails with a plugin-specific missing-bundle error rather than an opaque filesystem error. Tests import the actualdist/index.js, prove that it loadsdist/skill, exercise registration and disposal against the real DSH registry, prove the intended duplicate-name precedence, verify every referenced resource, and cover the missing-package-tree failure.The package declares
obelisk.cordis.ymlthroughdsh.bundle. Installing the local checkout links the dependency and adds its plugin row; removing the package withdraws both. The README provides one-line local install and uninstall commands, including pnpm's explicit workspace-root flag.The plugin build is implemented as a cross-platform Node script. Root tests build the package before running, and the CLI workflow now has a dedicated plugin build and test step on Linux, macOS, and Windows so the distributable package cannot silently fall outside the enumerated test files.
ADR-0012 follows the repository's established
Context/Decision/Verification/Consequencesstructure. It describes the future architecture positively; implementation-review history remains in this PR discussion.Verification
npm test— 554 tests passednpm run typechecknpx eslint packages/dsh-plugin/src/index.ts packages/dsh-plugin/scripts/build.mjs tests/dsh-plugin.test.mjsnpm run build --workspace @obelisk/dsh-obelisk-pluginnode --experimental-test-module-mocks --test tests/dsh-plugin.test.mjs— 8 tests passeddiff -qr packages/dsh-plugin/skill packages/dsh-plugin/dist/skilldiff -qr skill-doc packages/dsh-plugin/skill— verifies the complete canonical seed before any future DSH-specific evolutionnpm pack --workspace @obelisk/dsh-obelisk-plugin --dry-run --json— the package contains the bundle manifest, built entry point,SKILL.md, and every referenced resource, including the two canonical compatibility pointerswebprofile, and confirmed the application served successfully with the plugin mounted and no bundle or skill resolution error