Repository navigation
feat(plugins): Codex demarkus-memory - #213
Closed
ontehfritz wants to merge 1 commit into
Closed
ontehfritz wants to merge 1 commit into
ontehfritz wants to merge 1 commit into
Conversation
WalkthroughAdds Codex support to the shared demarkus-plugin CLI, rewires pin bumping to read Go consts from ChangesCodex plugin ecosystem
Sequence Diagram(s)sequenceDiagram
participant Codex
participant session_start as plugins/codex-memory/hooks/session-start.sh
participant bootstrap as plugins/codex-memory/scripts/bootstrap.sh
participant plugin as demarkus-plugin
Codex->>session_start: SessionStart hook
session_start->>bootstrap: run bootstrap.sh
session_start->>plugin: provision
session_start->>plugin: guidance using session-guidance.md
plugin-->>Codex: guidance JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/bump-plugin-pins.sh:
- Around line 49-84: The rewrite helpers in bump-plugin-pins.sh currently
succeed even when no target is updated, which can leave stale pins undetected.
Update set_goconst, set_bootstrap_tools, bump_json_version, and
set_marketplace_version to verify they actually matched and rewrote the expected
entry (for example by checking match counts or comparing before/after content)
and return a non-zero status when a target key, TOOLS_VERSION, .version field,
or marketplace source block is missing. Keep the existing function names and
preserve their current edit behavior, but make the workflow fail fast instead of
reporting success on partial or no-op rewrites.
In `@plugins/codex-knowledge/install.sh`:
- Around line 45-49: The config write in install.sh is doing a direct redirect
into CONFIG, which can leave config.toml corrupted if the process is
interrupted. Update the CONFIG generation flow to write the final contents to a
second temporary file first, then atomically move it into place with mv. Keep
the existing BEGIN, rendered, and END assembly logic, but change the write path
around CONFIG/tmp so the replacement happens only after the temp file is fully
written.
In `@plugins/codex-knowledge/skills/knowledge-doctor/SKILL.md`:
- Line 7: The skill doc in SKILL.md has markdownlint issues: add a top-level
heading at the start to satisfy MD041, and update the fenced code blocks to
include a language identifier and blank lines before and after the fences to
satisfy MD040 and MD031. Make the fixes consistently across the affected
sections so the document renders cleanly and passes linting.
In `@plugins/codex-knowledge/skills/knowledge/SKILL.md`:
- Around line 17-23: The knowledge-systems check currently reads a private state
file directly, which bypasses the supported registry source of truth. Update the
skill text in SKILL.md to point users to the existing `registry knowledge-list`
command from `registry_cmd.go` instead of `cat ~/.demarkus/knowledge-systems`,
and adjust the “no joined system” fallback to reference the registry command’s
output rather than private storage. Keep the guidance aligned with the
`knowledge-list` behavior so the skill and registry implementation stay
consistent.
In `@plugins/codex-memory/context/session-guidance.md`:
- Line 5: The session-guidance Markdown uses overly hyphen-dense, chained prose
that should be rewritten into shorter, simpler natural sentences. Update the
affected guidance text in session-guidance.md and the other referenced sections
so the same meaning is preserved while reducing in-sentence em dashes/hyphens,
keeping the wording natural and readable, and using bullet lists only where
needed.
In `@plugins/codex-memory/hooks/session-journal-nudge.sh`:
- Around line 86-89: The session-journal nudge logic is treating every shell
tool invocation as a file mutation, which can incorrectly set changed_files=true
for read-only commands. Update the grep/decision in session-journal-nudge.sh so
only true write actions are counted, using the existing changed_files flag and
the surrounding shell parsing logic to detect actual edit/write commands rather
than any "name":"shell" entry.
In `@plugins/codex-memory/install.sh`:
- Around line 40-52: The managed-block update in install.sh rewrites ${CONFIG}
in place, which can truncate the user’s Codex config if the rebuild fails.
Update the install flow around the tmp/awk merge logic so the merged content is
written to a separate temporary file first, then atomically renamed into
${CONFIG} only after the full render succeeds. Keep the existing block
replacement behavior in the same install.sh path, but avoid using the final
config file as the direct output target.
In `@plugins/codex-memory/README.md`:
- Line 3: The README prose uses hyphen-heavy, stacked sentence construction that
makes the soul doc harder to read. Rewrite the affected sentences in the
introductory description and the other referenced prose sections into shorter,
natural-language sentences, avoiding chained hyphen/em-dash joins while keeping
the meaning the same. Use the existing README text and the descriptive
paragraphs near the top and in the later sections as the targets for this
cleanup.
In `@plugins/codex-memory/skills/soul-context/SKILL.md`:
- Around line 10-16: The project resolution and existence check in the
soul-context workflow are too brittle: `basename "$PWD"` should not be the
source of truth, and a missing `index.md` must not stop valid projects. Update
the project lookup logic in the soul-context skill to use the bound repo/config
for the project slug instead of deriving it from `PWD`, then only use
`mark_fetch /<project>/index.md` as an optional check and continue to
`mark_list`/`mark_fetch` journal entries even when the index is not found.
In `@plugins/codex-memory/skills/soul-doctor/SKILL.md`:
- Around line 20-30: The orphan detection in the Gather/Core checks flow is
using a depth-limited `mark_graph` result as if it were exhaustive, which can
falsely mark documents as orphans. Update the logic around `mark_graph` and the
orphan check so that orphan reporting only runs when the crawl is complete, or
explicitly mark the crawl as partial and skip orphan detection when `depth: 5`
is used; keep `mark_list` as the source of truth for existence and only use the
full inbound edge set when it is guaranteed complete.
In `@plugins/codex-memory/skills/soul-status/SKILL.md`:
- Around line 25-29: The server-process check in the soul-status skill uses
SOUL_DIR directly in the pgrep pattern, which can mis-match paths containing
regex metacharacters or shared prefixes. Update the pgrep-based lookup in the
server check to safely escape or quote SOUL_DIR before building the match
expression, and keep the existing fallback message in sync so the health report
reflects the correct server status.
In `@plugins/README.md`:
- Around line 17-20: Add a language identifier to the fenced code block in the
README so markdownlint MD040 is satisfied. Update the bare fence around the
harness flow diagram in the markdown file to use a text fence, keeping the
content unchanged and preserving the existing diagram layout.
In `@tools/demarkus-plugin/emit_test.go`:
- Around line 41-42: The ask-case test in emitDecision should verify that the
original gate reason is preserved when confirmation text is added. Update the
codex-pre ask→deny+confirm case in emit_test.go so it asserts both the injected
“Confirm with the user” text and the original Reason value from gate.Decision,
using emitDecision-related symbols to ensure the function appends rather than
replaces the reason.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89dc8950-d80c-4432-ae5b-28631646b8f2
📒 Files selected for processing (41)
.github/scripts/bump-plugin-pins.sh.github/workflows/plugin-pin-bump.ymlREADME.mdplugins/README.mdplugins/codex-knowledge/README.mdplugins/codex-knowledge/config.example.tomlplugins/codex-knowledge/context/session-guidance.mdplugins/codex-knowledge/hooks/gate-post.shplugins/codex-knowledge/hooks/gate-pre.shplugins/codex-knowledge/hooks/recall-nudge.shplugins/codex-knowledge/hooks/session-start.shplugins/codex-knowledge/install.shplugins/codex-knowledge/scripts/bootstrap.shplugins/codex-knowledge/skills/knowledge-doctor/SKILL.mdplugins/codex-knowledge/skills/knowledge-join/SKILL.mdplugins/codex-knowledge/skills/knowledge/SKILL.mdplugins/codex-memory/README.mdplugins/codex-memory/config.example.tomlplugins/codex-memory/context/session-guidance.mdplugins/codex-memory/hooks/gate-post.shplugins/codex-memory/hooks/gate-pre.shplugins/codex-memory/hooks/promote-nudge.shplugins/codex-memory/hooks/recall-nudge.shplugins/codex-memory/hooks/session-journal-nudge.shplugins/codex-memory/hooks/session-start.shplugins/codex-memory/install.shplugins/codex-memory/scripts/bootstrap.shplugins/codex-memory/skills/promote-scan/SKILL.mdplugins/codex-memory/skills/promote/SKILL.mdplugins/codex-memory/skills/soul-context/SKILL.mdplugins/codex-memory/skills/soul-default/SKILL.mdplugins/codex-memory/skills/soul-doctor/SKILL.mdplugins/codex-memory/skills/soul-init/SKILL.mdplugins/codex-memory/skills/soul-join/SKILL.mdplugins/codex-memory/skills/soul-journal/SKILL.mdplugins/codex-memory/skills/soul-refresh/SKILL.mdplugins/codex-memory/skills/soul-status/SKILL.mdplugins/codex-memory/skills/soul/SKILL.mdtools/demarkus-plugin/emit_test.gotools/demarkus-plugin/internal/provision/provision.gotools/demarkus-plugin/main.go
Comment on lines
+49
to
+84
| # set_goconst <NAME> <value> — portable in-place edit (temp file; no sed -i). | ||
| set_goconst() { | ||
| local tmp; tmp=$(mktemp) | ||
| sed -E "s/^([[:space:]]*$1[[:space:]]*=[[:space:]]*)\"[^\"]+\"/\1\"$2\"/" "$provision" >"$tmp" && mv "$tmp" "$provision" | ||
| } | ||
|
|
||
| # set_bootstrap_tools <value> — update TOOLS_VERSION in every plugin bootstrap. | ||
| set_bootstrap_tools() { | ||
| local f tmp | ||
| for f in plugins/*/scripts/bootstrap.sh; do | ||
| [[ -f "$f" ]] || continue | ||
| tmp=$(mktemp) | ||
| sed -E "s/^(TOOLS_VERSION=)\"[^\"]+\"/\1\"$1\"/" "$f" >"$tmp" && mv "$tmp" "$f" | ||
| done | ||
| } | ||
|
|
||
| # bump_json_version <file> — patch-bump the .version field in a JSON file. Echoes | ||
| # the new version. Uses jq (these files round-trip cleanly through it). | ||
| bump_json_version() { | ||
| local f cur new tmp; f="$1" | ||
| cur=$(jq -r '.version' "$f") | ||
| new=$(awk -F. '{printf "%d.%d.%d", $1, $2, $3 + 1}' <<<"$cur") | ||
| tmp=$(mktemp) | ||
| jq --arg v "$new" '.version = $v' "$f" >"$tmp" && mv "$tmp" "$f" | ||
| echo "$new" | ||
| } | ||
|
|
||
| # set_marketplace_version <source-path> <value> — set the version of the | ||
| # marketplace entry whose "source" is <source-path>, preserving hand formatting. | ||
| set_marketplace_version() { | ||
| local tmp; tmp=$(mktemp) | ||
| awk -v v="$2" -v src="$1" ' | ||
| index($0, "\"source\": \"" src "\"") { inblock = 1 } | ||
| inblock && /"version":/ { sub(/"version": "[^"]*"/, "\"version\": \"" v "\""); inblock = 0 } | ||
| { print } | ||
| ' "$marketplace" >"$tmp" && mv "$tmp" "$marketplace" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the rewrite helpers fail on zero matches.
Lines 49-84 silently succeed when the target key is missing. A renamed Go const, missing TOOLS_VERSION, or drifted marketplace source leaves one pin stale, but the script still reports success and the workflow will open a partial bump PR. Count matches and return non-zero unless each rewrite hits the expected target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/bump-plugin-pins.sh around lines 49 - 84, The rewrite
helpers in bump-plugin-pins.sh currently succeed even when no target is updated,
which can leave stale pins undetected. Update set_goconst, set_bootstrap_tools,
bump_json_version, and set_marketplace_version to verify they actually matched
and rewrote the expected entry (for example by checking match counts or
comparing before/after content) and return a non-zero status when a target key,
TOOLS_VERSION, .version field, or marketplace source block is missing. Keep the
existing function names and preserve their current edit behavior, but make the
workflow fail fast instead of reporting success on partial or no-op rewrites.
Comment on lines
+45
to
+49
| { | ||
| cat "${tmp}" | ||
| printf '\n%s\n%s\n%s\n' "${BEGIN}" "${rendered}" "${END}" | ||
| } > "${CONFIG}" | ||
| rm -f "${tmp}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use atomic config replacement instead of direct truncating write.
If the process is interrupted while writing config.toml, the file can be left corrupted. Write to a second temp file and mv it into place.
Proposed fix
tmp="$(mktemp)"
+new_cfg="$(mktemp)"
awk -v b="${BEGIN}" -v e="${END}" '
$0 == b { skip = 1; next }
$0 == e { skip = 0; next }
!skip { print }
' "${CONFIG}" > "${tmp}"
{
cat "${tmp}"
printf '\n%s\n%s\n%s\n' "${BEGIN}" "${rendered}" "${END}"
-} > "${CONFIG}"
+} > "${new_cfg}"
+mv "${new_cfg}" "${CONFIG}"
rm -f "${tmp}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| cat "${tmp}" | |
| printf '\n%s\n%s\n%s\n' "${BEGIN}" "${rendered}" "${END}" | |
| } > "${CONFIG}" | |
| rm -f "${tmp}" | |
| new_cfg="$(mktemp)" | |
| { | |
| cat "${tmp}" | |
| printf '\n%s\n%s\n%s\n' "${BEGIN}" "${rendered}" "${END}" | |
| } > "${new_cfg}" | |
| mv "${new_cfg}" "${CONFIG}" | |
| rm -f "${tmp}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-knowledge/install.sh` around lines 45 - 49, The config write in
install.sh is doing a direct redirect into CONFIG, which can leave config.toml
corrupted if the process is interrupted. Update the CONFIG generation flow to
write the final contents to a second temporary file first, then atomically move
it into place with mv. Keep the existing BEGIN, rendered, and END assembly
logic, but change the write path around CONFIG/tmp so the replacement happens
only after the temp file is fully written.
| argument-hint: "[slug | mark://<world>/ | blank = every joined system, all worlds]" | ||
| --- | ||
|
|
||
| Run a read-only health check over an organizational demarkus knowledge system and report what's rotting. This is the knowledge-system counterpart to `/soul-doctor` (which audits the local soul). It **never writes** — it surfaces findings and suggests fixes; the user decides what to act on. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve markdownlint violations in this skill doc.
Current content triggers MD041 (missing top-level heading), MD031 (blank lines around fenced code), and MD040 (missing fenced-code language).
Also applies to: 20-22, 76-76
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🪛 SkillSpector (2.2.3)
[warning] 110: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-knowledge/skills/knowledge-doctor/SKILL.md` at line 7, The
skill doc in SKILL.md has markdownlint issues: add a top-level heading at the
start to satisfy MD041, and update the fenced code blocks to include a language
identifier and blank lines before and after the fences to satisfy MD040 and
MD031. Make the fixes consistently across the affected sections so the document
renders cleanly and passes linting.
Source: Linters/SAST tools
Comment on lines
+17
to
+23
| 1. **Find the joined systems.** Read the registry: | ||
|
|
||
| ```bash | ||
| cat ~/.demarkus/knowledge-systems 2>/dev/null | ||
| ``` | ||
|
|
||
| Each line is a joined system's MCP server slug. If the file is missing or empty, tell the user no knowledge system is joined yet and suggest `/knowledge-join <broker-url>`. Stop here. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the registry command as the source of truth.
cat ~/.demarkus/knowledge-systems bakes in an internal state file, and the note that codex mcp list includes souls is misleading. tools/demarkus-plugin/registry_cmd.go already exposes registry knowledge-list, so this skill should point users there instead of coupling them to private storage.
Based on tools/demarkus-plugin/registry_cmd.go:110-340.
Also applies to: 33-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-knowledge/skills/knowledge/SKILL.md` around lines 17 - 23, The
knowledge-systems check currently reads a private state file directly, which
bypasses the supported registry source of truth. Update the skill text in
SKILL.md to point users to the existing `registry knowledge-list` command from
`registry_cmd.go` instead of `cat ~/.demarkus/knowledge-systems`, and adjust the
“no joined system” fallback to reference the registry command’s output rather
than private storage. Keep the guidance aligned with the `knowledge-list`
behavior so the skill and registry implementation stay consistent.
|
|
||
| You have a local, versioned memory store (the "soul") wired through the `demarkus-memory` MCP tools. Treat it like project memory you'd get from an AGENTS.md: consult it before you start, and record what matters as you go — without waiting to be asked. | ||
|
|
||
| **This is your durable memory store — prefer it over any other.** Persistent knowledge (decisions, patterns, gotchas, session progress) belongs here, in one versioned and queryable place, not scattered into ad-hoc scratch files or a harness built-in memory tool. Keeping it unfragmented is the whole point. AGENTS.md still owns always-in-context project instructions; the soul owns everything you'd otherwise need to recall on demand. If your harness has a built-in memory tool and the user would rather demarkus be the single source of truth, you may offer to turn it off — but only with their say-so (the one-time setup offer surfaced at session start covers this); never disable it unasked. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rewrite hyphen-dense prose into simpler natural sentences.
This guidance uses frequent in-sentence em-dash/hyphen chaining. Please split into shorter direct sentences to match the Markdown style rule.
As per coding guidelines, "*.md: Use natural language in soul docs and avoid overusing hyphens within sentences (bullet lists with '- ' are acceptable)".
Also applies to: 14-18, 39-46, 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-memory/context/session-guidance.md` at line 5, The
session-guidance Markdown uses overly hyphen-dense, chained prose that should be
rewritten into shorter, simpler natural sentences. Update the affected guidance
text in session-guidance.md and the other referenced sections so the same
meaning is preserved while reducing in-sentence em dashes/hyphens, keeping the
wording natural and readable, and using bullet lists only where needed.
Source: Coding guidelines
Comment on lines
+10
to
+16
| 1. **Resolve the project slug.** Read `PWD` via the Bash tool (`basename "$PWD"`). Lowercase it and replace spaces with hyphens. If `PWD` is unset, ask the user which project. | ||
|
|
||
| 2. **Confirm the project exists in the soul.** Call `mark_fetch /<project>/index.md` (or `mark_list /<project>/`). If the result is `not-found`, tell the user the project has no entries in the soul yet, suggest `/soul-journal "<entry>"` to start one, and stop. | ||
|
|
||
| 3. **Pull recent journal entries.** List `/<project>/journal/` via `mark_list`. Take today's entry plus the two most recent prior days (UTC `YYYY-MM-DD.md`). For each, `mark_fetch` and read in full. If today's file doesn't exist, just use the most recent two prior days. | ||
|
|
||
| 4. **Pull active work.** `mark_fetch /<project>/roadmap.md` if it exists (what's next / in flight). Skip silently if `not-found`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the project binding as the source of truth.
Deriving the slug from basename "$PWD" is brittle outside the repo root, and stopping on a missing index.md hides projects that already have journal entries. Resolve the project from the bound repo/config instead, and continue to journal lookup when the index is absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-memory/skills/soul-context/SKILL.md` around lines 10 - 16, The
project resolution and existence check in the soul-context workflow are too
brittle: `basename "$PWD"` should not be the source of truth, and a missing
`index.md` must not stop valid projects. Update the project lookup logic in the
soul-context skill to use the bound repo/config for the project slug instead of
deriving it from `PWD`, then only use `mark_fetch /<project>/index.md` as an
optional check and continue to `mark_list`/`mark_fetch` journal entries even
when the index is not found.
Comment on lines
+20
to
+30
| ## Gather (cheap — two calls) | ||
|
|
||
| 1. **Crawl the link graph.** `mark_graph` on the scope hub with `depth: 5`. Parse: | ||
| - **Nodes:** `[status] <url> "title" N links` — note any status that isn't `ok` (e.g. `archived`), and `(no title)`. | ||
| - **Edges:** `<from> -> <to>` — `mark://` targets are internal; `http(s)` targets are external. | ||
| 2. **Inventory.** `mark_list` the scope (recurse into subdirectories) for the full set of documents that actually exist. | ||
|
|
||
| ## Core checks (from the graph + inventory — no per-doc fetching) | ||
|
|
||
| - **Broken links** — an edge whose `mark://` target does not exist. The inventory (`mark_list`) is the source of truth for existence — **not** the crawl: a target can be absent from the graph's Nodes simply because it sat beyond the crawl depth, which is *not* a broken link. So flag a target only when it's missing from the inventory, and confirm each suspect with a single `mark_fetch` (`not-found`) before reporting `source → missing-target`. | ||
| - **Orphans** — an inventory document that is never the target of any `mark://` edge (no inbound links) and isn't the scope's root hub. It's unreachable except by knowing its path. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The orphan check is wrong with a depth-5 crawl.
Line 22 caps mark_graph at depth 5, but line 30 treats the resulting edge set as complete. Any document that exists in mark_list but is only reachable beyond that depth will be reported as an orphan even though it has inbound links. Make orphan detection conditional on an exhaustive crawl, or mark the result as partial and skip that check when the crawl is bounded.
Proposed fix
-1. **Crawl the link graph.** `mark_graph` on the scope hub with `depth: 5`. Parse:
+1. **Crawl the link graph.** `mark_graph` on the scope hub. If you cap depth for cost, treat the graph as partial and skip checks that require a complete inbound-edge set (especially orphan detection). Parse:
@@
-- **Orphans** — an inventory document that is never the target of any `mark://` edge (no inbound links) and isn't the scope's root hub. It's unreachable except by knowing its path.
+- **Orphans** — **only when the graph crawl was exhaustive**: an inventory document that is never the target of any `mark://` edge (no inbound links) and isn't the scope's root hub. If the crawl was depth-limited, say orphan detection was skipped because the edge set is incomplete.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Gather (cheap — two calls) | |
| 1. **Crawl the link graph.** `mark_graph` on the scope hub with `depth: 5`. Parse: | |
| - **Nodes:** `[status] <url> "title" N links` — note any status that isn't `ok` (e.g. `archived`), and `(no title)`. | |
| - **Edges:** `<from> -> <to>` — `mark://` targets are internal; `http(s)` targets are external. | |
| 2. **Inventory.** `mark_list` the scope (recurse into subdirectories) for the full set of documents that actually exist. | |
| ## Core checks (from the graph + inventory — no per-doc fetching) | |
| - **Broken links** — an edge whose `mark://` target does not exist. The inventory (`mark_list`) is the source of truth for existence — **not** the crawl: a target can be absent from the graph's Nodes simply because it sat beyond the crawl depth, which is *not* a broken link. So flag a target only when it's missing from the inventory, and confirm each suspect with a single `mark_fetch` (`not-found`) before reporting `source → missing-target`. | |
| - **Orphans** — an inventory document that is never the target of any `mark://` edge (no inbound links) and isn't the scope's root hub. It's unreachable except by knowing its path. | |
| ## Gather (cheap — two calls) | |
| 1. **Crawl the link graph.** `mark_graph` on the scope hub. If you cap depth for cost, treat the graph as partial and skip checks that require a complete inbound-edge set (especially orphan detection). Parse: | |
| - **Nodes:** `[status] <url> "title" N links` — note any status that isn't `ok` (e.g. `archived`), and `(no title)`. | |
| - **Edges:** `<from> -> <to>` — `mark://` targets are internal; `http(s)` targets are external. | |
| 2. **Inventory.** `mark_list` the scope (recurse into subdirectories) for the full set of documents that actually exist. | |
| ## Core checks (from the graph + inventory — no per-doc fetching) | |
| - **Broken links** — an edge whose `mark://` target does not exist. The inventory (`mark_list`) is the source of truth for existence — **not** the crawl: a target can be absent from the graph's Nodes simply because it sat beyond the crawl depth, which is *not* a broken link. So flag a target only when it's missing from the inventory, and confirm each suspect with a single `mark_fetch` (`not-found`) before reporting `source → missing-target`. | |
| - **Orphans** — **only when the graph crawl was exhaustive**: an inventory document that is never the target of any `mark://` edge (no inbound links) and isn't the scope's root hub. If the crawl was depth-limited, say orphan detection was skipped because the edge set is incomplete. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-memory/skills/soul-doctor/SKILL.md` around lines 20 - 30, The
orphan detection in the Gather/Core checks flow is using a depth-limited
`mark_graph` result as if it were exhaustive, which can falsely mark documents
as orphans. Update the logic around `mark_graph` and the orphan check so that
orphan reporting only runs when the crawl is complete, or explicitly mark the
crawl as partial and skip orphan detection when `depth: 5` is used; keep
`mark_list` as the source of truth for existence and only use the full inbound
edge set when it is guaranteed complete.
Comment on lines
+25
to
+29
| 3. **Check the server process.** Read `SOUL_DIR` and `PORT` from the config. Run: | ||
|
|
||
| ``` | ||
| pgrep -af "demarkus-server.*-root ${SOUL_DIR}" || echo "(no server running at ${SOUL_DIR})" | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape SOUL_DIR before matching it in pgrep.
The current regex can mis-detect the managed server for paths with regex metacharacters or shared prefixes, so the health report can say "no server" when one is actually running.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.2.3)
[warning] 15: [E3] File System Enumeration: Code scans file system directories looking for sensitive files. This could be reconnaissance for credential theft.
Remediation: Remove unnecessary filesystem scanning. If file access is needed, use explicit, scoped paths. Avoid reading ~/.ssh, ~/.aws, or credential directories.
(Data Exfiltration (E3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/codex-memory/skills/soul-status/SKILL.md` around lines 25 - 29, The
server-process check in the soul-status skill uses SOUL_DIR directly in the
pgrep pattern, which can mis-match paths containing regex metacharacters or
shared prefixes. Update the pgrep-based lookup in the server check to safely
escape or quote SOUL_DIR before building the match expression, and keep the
existing fallback message in sync so the health report reflects the correct
server status.
Comment on lines
+17
to
+20
| ``` | ||
| harness hook/event ──▶ thin adapter ──▶ demarkus-plugin <cmd> --format <harness> ──▶ decision ──▶ adapter translates back | ||
| (all logic here, shared) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to this fenced block.
Lines 17-20 use a bare fence, which trips markdownlint MD040 on this file. text is enough here.
Patch sketch
-```
+```text
harness hook/event ──▶ thin adapter ──▶ demarkus-plugin <cmd> --format <harness> ──▶ decision ──▶ adapter translates back
(all logic here, shared)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/README.md` around lines 17 - 20, Add a language identifier to the
fenced code block in the README so markdownlint MD040 is satisfied. Update the
bare fence around the harness flow diagram in the markdown file to use a text
fence, keeping the content unchanged and preserving the existing diagram layout.
Source: Linters/SAST tools
Comment on lines
+41
to
+42
| {name: "codex-pre block→deny", format: "codex-pre", dec: gate.Decision{Decision: "block", Reason: "no tags"}, event: "PreToolUse", permission: "deny", reasonHas: "no tags"}, | ||
| {name: "codex-pre ask→deny+confirm", format: "codex-pre", dec: gate.Decision{Decision: "ask", Reason: "stricter."}, event: "PreToolUse", permission: "deny", reasonHas: "Confirm with the user"}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Lock in the original gate reason in the ask case.
Lines 41-42 only assert the injected confirmation text. If emitDecision() later replaces d.Reason instead of appending to it, this test still passes even though the user loses the actionable reason. Assert that "stricter." survives too.
Patch sketch
- {name: "codex-pre ask→deny+confirm", format: "codex-pre", dec: gate.Decision{Decision: "ask", Reason: "stricter."}, event: "PreToolUse", permission: "deny", reasonHas: "Confirm with the user"},
+ {name: "codex-pre ask→deny+confirm", format: "codex-pre", dec: gate.Decision{Decision: "ask", Reason: "stricter."}, event: "PreToolUse", permission: "deny", reasonHas: "stricter. Confirm with the user"},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {name: "codex-pre block→deny", format: "codex-pre", dec: gate.Decision{Decision: "block", Reason: "no tags"}, event: "PreToolUse", permission: "deny", reasonHas: "no tags"}, | |
| {name: "codex-pre ask→deny+confirm", format: "codex-pre", dec: gate.Decision{Decision: "ask", Reason: "stricter."}, event: "PreToolUse", permission: "deny", reasonHas: "Confirm with the user"}, | |
| {name: "codex-pre block→deny", format: "codex-pre", dec: gate.Decision{Decision: "block", Reason: "no tags"}, event: "PreToolUse", permission: "deny", reasonHas: "no tags"}, | |
| {name: "codex-pre ask→deny+confirm", format: "codex-pre", dec: gate.Decision{Decision: "ask", Reason: "stricter."}, event: "PreToolUse", permission: "deny", reasonHas: "stricter. Confirm with the user"}, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/demarkus-plugin/emit_test.go` around lines 41 - 42, The ask-case test
in emitDecision should verify that the original gate reason is preserved when
confirmation text is added. Update the codex-pre ask→deny+confirm case in
emit_test.go so it asserts both the injected “Confirm with the user” text and
the original Reason value from gate.Decision, using emitDecision-related symbols
to ensure the function appends rather than replaces the reason.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests