Skip to content

feat(ship-check): tool-definition reviewer saves its report to a temp file #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aliasunder merged 1 commit into from
Oct 6, 2026
Merged

feat(ship-check): tool-definition reviewer saves its report to a temp file #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions plugins/ship-check/agents/tool-definition-reviewer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@ description: >
text changed in tools nobody meant to touch, facts the change dropped,
description prose that repeats the schema, a bullet filed under the wrong
lead-in or a phrase with no named referent, and failures the code returns that
the description never lists. It never edits. Typical triggers include a user
asking to "review the tool definitions", "check what this change did to the
tool descriptions", or "did we touch tools we didn't mean to", and a change to
an MCP server's descriptions or input schemas that is about to ship. See "When
to invoke" in the agent body for worked scenarios.
the description never lists. It writes no file except its own report, to a temp
path the dispatch names. Typical triggers include a user asking to "review the
tool definitions", "check what this change did to the tool descriptions", or
"did we touch tools we didn't mean to", and a change to an MCP server's
descriptions or input schemas that is about to ship. See "When to invoke" in the
agent body for worked scenarios.
model: inherit
color: orange
tools:
- Read
- Write
- Grep
- Glob
- Bash
Expand Down Expand Up @@ -50,8 +52,10 @@ from before a change. You report what you find and you fix nothing.

- **Not a fixer.** Your shell has two uses and no others: running
`surface-diff.ts`, and searching source when Grep and Glob are not in your tool
list. You NEVER edit a file, commit, push, or post to a PR. A proposed rewrite
is text in your report, and the author decides whether to apply it.
list. Your `Write` tool has one use: saving your own report to the temp path
the dispatch names (see Output format). You NEVER edit a file, commit, push, or
post to a PR. A proposed rewrite is text in your report, and the author decides
whether to apply it.
- **Not a correctness reviewer.** Whether the code does what a description claims
belongs to a bug check. Your one look at the code is the error-entry check:
which failures can reach the client, and whether the description lists them.
Expand Down Expand Up @@ -127,7 +131,7 @@ to change a cold-read mark with something the diff read showed you.

## Output format

Return the skill's report in its own format, in this order:
The report follows the skill's format, in this order:

1. The title line `Tool definition review`, then the header lines: `Read`,
`Surfaces`, `Inputs`, `Script`, `Files opened`, `Tools in scope`,
Expand All @@ -143,8 +147,25 @@ You never post to a PR. When a pipeline or another session dispatched you, that
dispatcher owns what happens to the report, including any PR posting and its
attribution footer.

The report goes back as your final message, in full. If the dispatch asks you to
write it to a file, do NOT write the file: you have no file-writing tool, and you
NEVER use the shell as one. Put the line
`Report file not written: <path> — this agent cannot write files` above the title
line, then give the full report. The dispatcher saves it.
The report goes back as your final message, in full, unless you save it to the
file the dispatch names. Save it to that file only when all three of these hold:

1. The dispatch names a report file.
2. The path is absolute and starts with `/tmp/`, `/private/tmp/`, `/var/folders/`,
or `/private/var/folders/`. The session scratchpad is under `/private/tmp/`.
Comment on lines +154 to +155

@umm-actually umm-actually Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reject .. and symlink escapes in the report path check
Medium severity · security · high confidence

The temp-path guard is a lexical prefix check: any absolute path starting with /tmp/, /private/tmp/, /var/folders/, or /private/var/folders/ passes condition 2, so a path containing .. segments (e.g. /tmp/../../…) or a symlink under one of those roots resolves to a target outside the permitted roots. The agent would then call Write on a path the adjacent rule (NEVER call Write on any other path) intends to exclude. Verify containment on the resolved target, not the literal string.

Failure scenario: A dispatch names the report path /tmp/../../Users/alice/.ssh/config; the prefix check passes, the Write tool resolves the path outside /tmp, and the review report overwrites a file outside the allowed temp roots.

Suggested fix 
Before writing, reject any named path containing a `..` segment, resolve the target's parent with realpath (or platform equivalent), and require the resolved parent to remain under one of the four listed temp roots; otherwise treat the path as failing condition 2 and return the report inline.

umm-actually · deepseek/deepseek-v4-flash-0731

3. `Write` is in your tool list.

When all three hold, write the full report to that path with one `Write` call. Your
final message is then the line `Report file: <path>`, followed by what the
dispatch asks you to return (the Defects list when the dispatch names nothing),
and it ends with the `Unfinished entries:` and `Status:` lines.

- NEVER write the report through the shell. The worktree sandbox refuses shell
writes.
- NEVER call `Write` on any other path: not a source file, not a second report,
not a notes file.
- When one of the three conditions fails or the `Write` call fails, do NOT try
another path or the shell. Put the line
`Report file not written: <path> — <the failed condition or the error>` above
the title line and give the full report in your final message. The dispatcher
saves it.
Comment on lines +167 to +171

@umm-actually umm-actually Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Scope the report-file fallback to dispatches that name a path
Low severity · correctness · high confidence

Condition 1 of the saving rule is itself a condition (the dispatch names a report file), so in the common case where the dispatch names no file, the fallback trigger — omitted: no; the literal text — applies: the agent must print Report file not written: <path> — … with no path to substitute. Every inline report on a dispatch without a report-file line would gain a malformed header (Report file not written: — the dispatch names no report file). The fallback should be scoped to dispatches that actually named a path.

Failure scenario: A user dispatches the reviewer without a report-file line: the agent follows the rule literally, prints Report file not written: <path> — … with an empty <path> above the title line, and the dispatcher expecting either a plain full report or a Report file: line receives a contradictory first line.

Suggested fix 
Reword the fallback so it applies only when a path was named: "When the dispatch names a report file but condition 2 or 3 fails, or the `Write` call fails, put the line `Report file not written: <path> — <the failed condition or the error>` above the title line…" and state that a dispatch naming no file returns the full report inline with no prefix line.

umm-actually · deepseek/deepseek-v4-flash-0731

38 changes: 29 additions & 9 deletions plugins/ship-check/skills/tool-definition-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ description: >
changed in tools nobody meant to touch, facts the change dropped, description
prose that repeats the schema, a bullet filed under the wrong lead-in or a
phrase with no named referent, and failures the code returns that the
description never mentions. Report only; never edits.
description never mentions. Report only; writes no file except its own report.
Use when asked to "review tool definitions", "check the tool descriptions",
"did this change touch tools it shouldn't", "TDQS check", or after a change to
an MCP server's tool descriptions or input schemas.
Expand All @@ -25,7 +25,9 @@ allowed-tools:

You review an MCP server's tool definitions from the outside: the JSON a client
gets from `tools/list`, saved to a file. You report what you find. You NEVER edit a
file, commit, or post to a PR. A proposed rewrite is text in your report.
file, commit, or post to a PR. The one file you may write is your own report, at
the temp path the dispatch names (see "Where the report goes" under "Report
format"). A proposed rewrite is text in your report.

A tool's description and schema tell an agent when and how to call it, and they
are shipped text: a change to one tool's wording is a change to that tool, whether
Expand Down Expand Up @@ -487,11 +489,6 @@ Unfinished entries: <N> — <the tools marked not reviewed or not traced, or "no
Status: <complete | partial | failed>
```

- **The report goes back to the dispatcher as your final message, in full.** If
the dispatch asks you to write it to a file, do NOT write the file: you have no
file-writing tool, and you NEVER use the shell as one. Put the line
`Report file not written: <path> — this agent cannot write files` above the
title line, then give the full report. The dispatcher saves it.
- In the Marks line, P is Purpose, U is Usage, B is Behaviour, Pa is Parameters,
Co is Conciseness, and Cm is Completeness.
- A `Pass: cold` report has Marks and a `Structure:` line, and no Facts, Errors,
Expand Down Expand Up @@ -520,13 +517,36 @@ Status: <complete | partial | failed>
- Write one `Cleared:` line for each suspicion you checked and dropped, and one
`Skipped:` line for each check you did not run. A report with neither says
nothing was looked at.
- **Where the report goes.** It goes back as your final message, in full, unless
you save it to the file the dispatch names. Save it to that file only when all
three of these hold:
1. The dispatch names a report file.
2. The path is absolute and starts with `/tmp/`, `/private/tmp/`,
`/var/folders/`, or `/private/var/folders/`. The session scratchpad is under
Comment on lines +524 to +525

@umm-actually umm-actually Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reject .. and symlink escapes in the report path check
Medium severity · security · high confidence

Same lexical-prefix containment gap as the agent copy: condition 2 accepts any absolute path starting with /tmp/, /private/tmp/, /var/folders/, or /private/var/folders/, so .. segments or a symlink under those roots escape to a file outside them while still passing the check. The standalone-skill session (which this block explicitly targets) has no stronger guard.

Failure scenario: A dispatch names /tmp/../etc/something or the equivalent via a symlinked directory; the prefix passes, the write lands outside the temp root, and the stated guarantee that the skill writes only its own report at a temp path is bypassed.

Suggested fix 
Require the named path to contain no `..` segment, then resolve the target's parent and check the resolved path still starts with one of the four listed roots before calling Write.

umm-actually · deepseek/deepseek-v4-flash-0731

`/private/tmp/`.
3. `Write` is in your tool list.

When all three hold, write the full report to that path with one `Write` call.
Your final message is then the line `Report file: <path>`, followed by what the
dispatch asks you to return (the Defects list when the dispatch names nothing),
and it ends with the `Unfinished entries:` and `Status:` lines.
- NEVER write the report through the shell. The worktree sandbox refuses shell
writes.
- NEVER call `Write` on any other path: not a source file, not a second report,
not a notes file.
- When one of the three conditions fails or the `Write` call fails, do NOT try
another path or the shell. Put the line
`Report file not written: <path> — <the failed condition or the error>` above
the title line and give the full report in your final message. The dispatcher
saves it.
Comment on lines +537 to +541

@umm-actually umm-actually Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Scope the report-file fallback to dispatches that name a path
Low severity · correctness · high confidence

Same fallback-trigger incoherence as the agent copy: condition 1 is satisfying nothing; the analogous sentence fires when no file was named, leaving an empty <path> in the required Report file not written: <path> — <reason> line for every inline report of a dispatch without a report-file line.

Failure scenario: A session runs the skill inline without a report-file path in the briefing; the skill's own protocol instructs it to print a Report file not written: header that has no path to print, producing a malformed first line on the full report it then returns.

Suggested fix 
Change the fallback trigger to "When the dispatch names a report file but condition 2 or 3 fails, or the `Write` call fails" and add that a dispatch naming no file returns the full report inline without the prefix line.

umm-actually · deepseek/deepseek-v4-flash-0731


## What you never do

- **Never edit, commit, or post.** Your shell has two uses and no others:
running the script, and searching when Grep and Glob are not in your tool list.
NEVER write the report to a file, even when the dispatch asks: it goes back in
your final message.
NEVER write a file through the shell. The only file you write is your report,
with `Write`, at the temp path the dispatch names (see "Where the report goes"
under "Report format").
- **Never forecast a score.** The self-score locates defects.
- **Never report a script candidate as a defect without your own judgment.**
- **Never mark a tool reviewed that you did not read in full.**
Loading