feat: optional ingestion policy for transcript writeback - #16
Tirth21896 wants to merge 1 commit into
Conversation
Adds an opt-in policy file that lets a deployment declare which transcript turns are worth storing durably and how large a single turn may be. Motivation: on a busy harness the rollout contains a large amount of content with no durable memory value — injected plugin catalogues, re-ingested AGENTS.md/CLAUDE.md (already authoritative in git), and system reminders. capture() enqueued all of it verbatim, which is paid ingestion on every turn. On one workspace this class of content accounted for ~15% of all tokens ingested, on top of per-turn dumps that carried a further ~48%. Resolution order: 1. $HONCHO_MEMORY_POLICY 2. ~/.honcho/memory-policy.json 3. none — ingestion is unfiltered, matching current behaviour With no policy present nothing changes, so this is safe for existing users. Patterns are compiled with new RegExp(pattern, flags); flags are declared in a sibling field rather than inline so the same file can be shared with non-JS consumers. A malformed policy file, or an individual unparseable rule, degrades to the unfiltered default rather than failing the hook — a bad policy must never cost a user their session memory.
WalkthroughThe PR adds configurable ingestion policies. ChangesIngestion policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant capture
participant policy
participant turnQueue
participant cursor
capture->>policy: loadPolicy()
capture->>policy: shouldDrop() and capTurn()
capture->>turnQueue: enqueue retained turns
capture->>cursor: advance across all fresh turns
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/policy.ts`:
- Around line 50-52: Update the compiled drop-pattern handling in policy
compilation and the corresponding shouldDrop path so stateful g/y regular
expressions reset lastIndex before every test, preserving consistent matching
across repeated calls. Add a regression test that invokes shouldDrop repeatedly
with a policy containing a g-flagged rule and verifies each matching call is
dropped.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c1e0adb-19ec-4310-82b4-00b0822ba655
📒 Files selected for processing (3)
src/hooks/writeback.tssrc/policy.tstest/policy.test.ts
| try { | ||
| compiled.push(new RegExp(rule.pattern, rule.flags ?? "")); | ||
| } catch { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does JavaScript RegExp.prototype.test mutate lastIndex for patterns that use the global (g) or sticky (y) flags?
💡 Result:
Yes, JavaScript's RegExp.prototype.test method mutates the lastIndex property of a regular expression if it has the global (g) or sticky (y) flags enabled [1][2]. When these flags are present, the RegExp instance is considered stateful, and lastIndex tracks the position for the next match [1][3]. The behavior regarding lastIndex mutation is as follows: 1. Global (g) flag: If the test method finds a match, it advances lastIndex to the position immediately following the end of the matched string [1]. If it fails to find a match, it resets lastIndex to 0 [1][4]. 2. Sticky (y) flag: This flag forces the search to begin exactly at the position specified by lastIndex [5][2]. If a match is successful, lastIndex is advanced to the position immediately following the match; if the match fails, lastIndex is reset to 0 [5][2]. Because test calls the internal exec method, it inherits this stateful behavior [1][3]. This can lead to unexpected results if the same global or sticky RegExp object is reused across multiple calls without resetting lastIndex [1][4]. If you do not intend to track state across multiple matches, it is recommended to avoid using the g or y flags [6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/sticky
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec
- 4: https://stackoverflow.com/questions/28910934/why-does-regex-test-changes-the-result-in-subsequent-calls
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
- 6: https://stackoverflow.com/questions/34646966/regexp-protoype-test-returns-confusing-results
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching policy.ts:"
fd -a 'policy\.ts$' . || true
file="$(fd 'policy\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Tracked status:"
git ls-files -- "$file" || true
echo
echo "Line count:"
wc -l "$file"
echo
echo "Summary:"
ast-grep outline "$file" || true
echo
echo "Relevant sections:"
sed -n '1,120p' "$file" | nl -ba
fi
echo
echo "Search shouldDrop/dropPatterns usages:"
rg -n "shouldDrop|dropPatterns|drop_patterns|CompiledPatterns|IngestionPolicy" .Repository: plastic-labs/codex-honcho
Length of output: 1052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/policy.ts"
echo "== src/policy.ts =="
awk '{printf "%4d %s\n", NR, $0}' "$file"
echo
echo "== policy identifiers usages =="
rg -n "shouldDrop|dropPatterns|drop_patterns|compile|IngestionPolicy" .
echo
echo "== JS RegExp.test state behavior probe =="
node - <<'JS'
const tests = [
[/g/a],
[/y/a],
[/da/a],
[/da/],
];
for (const re of tests) {
re.test("ba");
let last = re.lastIndex;
const first = re.test("aaa");
const second = re.test("aaa");
console.log(re.toString(), { lastIndexAfterFirst, first, second });
}
JSRepository: plastic-labs/codex-honcho
Length of output: 7270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== writeback context =="
awk '{printf "%4d %s\n", NR, $0}' src/hooks/writeback.ts
echo
echo "== policy tests content =="
awk '{printf "%4d %s\n", NR, $0}' test/policy.test.ts
echo
echo "== JS RegExp.test state behavior probe =="
node - <<'JS'
const patterns = ["a", "a", "a", "a"];
const flags = ["", "g", "y", "m"];
for (let i = 0; i < patterns.length; i++) {
const re = new RegExp(patterns[i], flags[i]);
re.test("ba");
const first = re.test("aaa");
const second = re.test("aaa");
console.log(JSON.stringify({ pattern: patterns[i], flags: flags[i], lastIndexAfterFirstMatch: re.lastIndex, first, second }));
}
JSRepository: plastic-labs/codex-honcho
Length of output: 7086
Reset drop-pattern state before each stateful regex test.
RegExp.prototype.test() advances lastIndex for patterns with g or y, so src/hooks/writeback.ts can let the second matching turn pass through shouldDrop even though the same drop_pattern matches it. Reset lastIndex before each test, or reject g/y flags at policy compilation. Add a regression test with repeated calls to shouldDrop(..., policy) using a g-flagged rule.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 50-50: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(rule.pattern, rule.flags ?? "")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 50-50: Do not use variable for regular expressions
Context: new RegExp(rule.pattern, rule.flags ?? "")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🤖 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 `@src/policy.ts` around lines 50 - 52, Update the compiled drop-pattern
handling in policy compilation and the corresponding shouldDrop path so stateful
g/y regular expressions reset lastIndex before every test, preserving consistent
matching across repeated calls. Add a regression test that invokes shouldDrop
repeatedly with a policy containing a g-flagged rule and verifies each matching
call is dropped.
1 participant
Problem
capture()enqueues every fresh rollout turn verbatim. On a busy agent harness a large share of those turns have no durable memory value — injected plugin catalogues, re-ingestedAGENTS.md/CLAUDE.md(already authoritative in git), and system reminders — but they are still paid ingestion on every turn.Measured on one real workspace over 13 days: 12,977 messages / 1,395,373 ingested tokens, of which ~15% was boilerplate of that kind. Separately, 581 turns over 500 tokens accounted for ~48% of all ingested tokens, so a per-turn cap is worth as much as the drop list.
There is currently no way to express "don't store this" short of turning
saveMessagesoff entirely, which loses the useful memory too.Change
An opt-in ingestion policy. Resolution order:
$HONCHO_MEMORY_POLICY~/.honcho/memory-policy.json{ "ingestion": { "max_turn_chars": 2000, "truncation_marker": "\n…[truncated for memory ingestion]", "drop_patterns": [ { "id": "plugin-catalogue", "pattern": "<recommended_plugins>" }, { "id": "agents-md", "pattern": "^#\\s*AGENTS\\.md instructions for", "flags": "m" } ] } }Notes
loadPolicy()returns an unfiltered policy when nothing is configured.(?i)), so the same policy file can be shared with non-JS consumers. In our case a Python-side memory gateway reads the same file, which is why the constraint exists.Testing
bun test— 84 pass, 0 fail (6 new tests intest/policy.test.tscovering: no policy, malformed policy, partially-invalid rules, declared flags, capping, and marker output).tsc --noEmitclean.Happy to adjust naming, the resolution order, or move the default path if you'd prefer something else.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes