fix(state-line-guard): Edit path used String.replace, not a function
current.replace(oldStr, newStr) with newStr as a STRING lets JS treat $-sequences inside it ($&, $`, $', $$, $n) as special replacement patterns, even though oldStr (the search side) is a plain string. A new_string documenting old backtick-substitution style ($`cmd`) - the kind of prose a STATE.md shell-conventions section writes routinely - triggers it. Measured against the real bug (.claude/STATE.md, 2026-08-15): a 5-line addition on a 112-line file projected to 219 lines and was wrongly denied. Fix: current.replace(oldStr, () => newStr) - a function replacement is never pattern-substituted, covering every $-sequence at once. The replace_all branch (split/join) was never affected. Direction was always fail-closed (over-blocks, never under-blocks a real oversize), but it made exactly the STATE.md files that document shell conventions hard to edit via Edit. state-line-guard-selftest.sh: 23/23 (+2, section 9: $` as the repro, $& as a second sequence proving the fix is general). Also updates CLAUDE.md's pinned selftest counts (197/178/69/21 were already stale before this session's own additions; now 206/183/69/23).
This commit is contained in:
parent
bd24b8f0e7
commit
b5c860eb03
3 changed files with 104 additions and 6 deletions
41
CLAUDE.md
41
CLAUDE.md
|
|
@ -16,7 +16,19 @@ marketplace plugin. Three components, one boundary:
|
|||
`coord-done.sh` archives, `coord-count.sh` counts without delivering,
|
||||
`coord-sweep.sh` closes the aged FYI backlog machine-wide.
|
||||
Everything is pinned by `coord-selftest.sh`
|
||||
(197 checks, throwaway mailbox via `CLAUDE_COORD_DIR`).
|
||||
(206 checks, throwaway mailbox via `CLAUDE_COORD_DIR`).
|
||||
|
||||
**`ktg-plugin-marketplace` is a RETIRED `--to` address (operator decision
|
||||
2026-08-15), rejected rather than redirected.** It is a polyrepo directory,
|
||||
not a git repo, so `basename(git toplevel)` can never resolve to it and no
|
||||
session was ever able to hold that identity naturally - mail for it belongs
|
||||
to `catalog` instead. A silent redirect was considered and declined: it
|
||||
delivers mail somewhere the sender does not believe it landed, which is the
|
||||
same misdelivery defect this closes a second time (2 messages sat
|
||||
undelivered 2 days on this exact misaddressing before `catalog`'s H4 count
|
||||
caught 6 more). Rejection fails loud at the sender, at the moment the
|
||||
mistake is made. Only `--to` is retired, not `--from` - the defect was mail
|
||||
*arriving* there, never mail claiming to *originate* there.
|
||||
|
||||
**`coord-sweep.sh` is the only path that closes a message with no human in
|
||||
the loop, and every constraint on it follows from that.** It may close exactly
|
||||
|
|
@ -120,10 +132,29 @@ marketplace plugin. Three components, one boundary:
|
|||
not the current threshold, and are left as-is rather than rewritten.
|
||||
`session-start.mjs`'s 160-line injection window still covers the new
|
||||
120-line limit with room to spare, so no change was needed there.
|
||||
|
||||
**The Edit path's `current.replace(oldStr, newStr)` was a dollar-pattern
|
||||
injection bug, found and fixed 2026-08-15.** Passing `newStr` as a STRING
|
||||
makes JavaScript interpret `$`-sequences inside it ($&, `` $` ``, `$'`,
|
||||
`$$`, `$n`) as special replacement patterns, even though `oldStr` (the
|
||||
search side) is a plain string, not a RegExp. A `new_string` documenting
|
||||
old backtick-substitution style (`` $`cmd` ``) - exactly the prose a
|
||||
STATE.md's shell-conventions section writes routinely - triggers it.
|
||||
Measured against the real bug (`.claude/STATE.md`): a 5-line addition on a
|
||||
112-line file projected to 219 lines and was wrongly denied. Direction is
|
||||
always fail-CLOSED (never fail-open: it can only over-block, never
|
||||
under-block a real oversize), but it made exactly the kind of STATE.md
|
||||
that documents shell conventions hard to edit. Fix: replace with a
|
||||
function, `current.replace(oldStr, () => newStr)` - a function result is
|
||||
never pattern-substituted, so this covers every `$`-sequence at once, not
|
||||
a `` $` ``-specific escape. The `replace_all` branch (`split`/`join`) was
|
||||
never affected - `join` does not interpret its argument as a pattern.
|
||||
Pinned by state-line-guard-selftest.sh section 9 (`$\`` as the real repro,
|
||||
`$&` as a second sequence proving the fix is general).
|
||||
- **Board (`scripts/board.sh`):** cross-repo attention board. Reads STATE.md
|
||||
next-step blocks + board lines, `git status`, and mailbox pending counts, and
|
||||
prints one line per repo. Read-only by construction: it writes to no repo, no
|
||||
STATE.md and no mailbox. Pinned by `board-selftest.sh` (178 checks).
|
||||
STATE.md and no mailbox. Pinned by `board-selftest.sh` (183 checks).
|
||||
|
||||
**It lives here because the mailbox is one of its three inputs, and it carries
|
||||
the same axis distinction the mailbox does.** A pending count means *others
|
||||
|
|
@ -429,10 +460,10 @@ obligations in another repo.
|
|||
- Zero dependencies everywhere: bash + coreutils in the engine, `node:`
|
||||
builtins only in hook and tests.
|
||||
- TDD: no behavior change without a failing selftest check first.
|
||||
`bash scripts/coord-selftest.sh` must exit 0 (197/197),
|
||||
`bash scripts/board-selftest.sh` must exit 0 (178/178),
|
||||
`bash scripts/coord-selftest.sh` must exit 0 (206/206),
|
||||
`bash scripts/board-selftest.sh` must exit 0 (183/183),
|
||||
`bash scripts/route-selftest.sh` must exit 0 (69/69) and
|
||||
`bash scripts/state-line-guard-selftest.sh` must exit 0 (21/21).
|
||||
`bash scripts/state-line-guard-selftest.sh` must exit 0 (23/23).
|
||||
- English for all code, docs, and commit messages (public repo). Norwegian
|
||||
trigger aliases in the skill description are deliberate.
|
||||
- Conventional Commits: `type(scope): description`.
|
||||
|
|
|
|||
|
|
@ -114,7 +114,10 @@ if (toolName === 'Write') {
|
|||
}
|
||||
projected = toolInput.replace_all
|
||||
? current.split(oldStr).join(newStr)
|
||||
: current.replace(oldStr, newStr);
|
||||
// A string replacement here would let JS interpret $-sequences inside
|
||||
// newStr ($&, $`, $', $$, $n) as special patterns instead of literal
|
||||
// text - a function replacement is never pattern-substituted.
|
||||
: current.replace(oldStr, () => newStr);
|
||||
currentLines = countLines(current);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -290,6 +290,70 @@ process.stdout.write(JSON.stringify({
|
|||
run_hook "$P"
|
||||
[ "$HOOK_EXIT" -eq 0 ]; check "Edit: shrinking an oversized file allows, even if still over the limit" $?
|
||||
|
||||
# --- 9. Edit: new_string is treated LITERALLY, never as a String.replace ----
|
||||
# special-pattern ($&, $`, $', $$, $n). Line 117 used to call
|
||||
# current.replace(oldStr, newStr) with newStr as a STRING: JavaScript then
|
||||
# interprets $-sequences inside the REPLACEMENT as special patterns even
|
||||
# though the SEARCH side (oldStr) is a plain string, not a RegExp. A
|
||||
# new_string documenting old backtick-substitution style ($`cmd`) is exactly
|
||||
# the kind of prose a STATE.md's shell-conventions section writes routinely.
|
||||
# Measured against the real bug (.claude/STATE.md, 2026-08-15): a 5-line
|
||||
# addition on a 112-line file projected to 219 lines and was wrongly denied.
|
||||
# Fix: current.replace(oldStr, () => newStr) - a function replacement is
|
||||
# never pattern-substituted, so this covers every $-sequence, not just $`.
|
||||
|
||||
FIXTURE5="$TMPDIR/e"
|
||||
mkdir -p "$FIXTURE5"
|
||||
node -e '
|
||||
const fs = require("fs");
|
||||
const content = "p\n".repeat(100) + "TARGET\n" + "q\n".repeat(11);
|
||||
fs.writeFileSync(process.argv[1], content);
|
||||
' "$FIXTURE5/STATE.md"
|
||||
# 112 lines total (100 + 1 + 11), matching the real repro's file size.
|
||||
|
||||
export STATE_GUARD_FIXTURE5="$FIXTURE5/STATE.md"
|
||||
P="$(payload '
|
||||
const path = process.env.STATE_GUARD_FIXTURE5;
|
||||
const oldStr = "TARGET\n";
|
||||
const newStr = "TARGET\n" +
|
||||
"avoid old backtick-substitution style: $`cmd` (use $(cmd) instead)\n" +
|
||||
"line2\n" + "line3\n" + "line4\n" + "line5\n";
|
||||
process.stdout.write(JSON.stringify({
|
||||
tool_name: "Edit",
|
||||
tool_input: { file_path: path, old_string: oldStr, new_string: newStr }
|
||||
}));
|
||||
')"
|
||||
run_hook "$P"
|
||||
# Real net change is +5 lines (112 -> 117): under MAX_LINES, must allow. A
|
||||
# dollar-pattern-vulnerable replace() balloons this past 120 and wrongly denies.
|
||||
[ "$HOOK_EXIT" -eq 0 ]; check "Edit: new_string containing \$\` is treated literally, not pattern-substituted (allows a real +5-line edit)" $?
|
||||
unset STATE_GUARD_FIXTURE5
|
||||
|
||||
FIXTURE6="$TMPDIR/f"
|
||||
mkdir -p "$FIXTURE6"
|
||||
node -e '
|
||||
const fs = require("fs");
|
||||
const content = "p\n".repeat(100) + "TARGET\n" + "q\n".repeat(11);
|
||||
fs.writeFileSync(process.argv[1], content);
|
||||
' "$FIXTURE6/STATE.md"
|
||||
|
||||
export STATE_GUARD_FIXTURE6="$FIXTURE6/STATE.md"
|
||||
P="$(payload '
|
||||
const path = process.env.STATE_GUARD_FIXTURE6;
|
||||
const oldStr = "TARGET\n";
|
||||
const newStr = "TARGET line, matched text follows: $& -- end\n" +
|
||||
"line2\n" + "line3\n" + "line4\n" + "line5\n";
|
||||
process.stdout.write(JSON.stringify({
|
||||
tool_name: "Edit",
|
||||
tool_input: { file_path: path, old_string: oldStr, new_string: newStr }
|
||||
}));
|
||||
')"
|
||||
run_hook "$P"
|
||||
# Same class, different special sequence ($& = the whole matched substring):
|
||||
# proves the fix is general (a function replacement), not a $`-specific patch.
|
||||
[ "$HOOK_EXIT" -eq 0 ]; check "Edit: new_string containing \$& is also treated literally (fix is general, not backtick-specific)" $?
|
||||
unset STATE_GUARD_FIXTURE6
|
||||
|
||||
echo ""
|
||||
echo "state-line-guard-selftest: $PASS passed, $FAIL failed"
|
||||
[ "$FAIL" -eq 0 ] || exit 1
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue