fix: 2026-07-02 audit remediation — date-safe redaction, DR incomplete flags, symlink-safe installs - #21
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
📝 WalkthroughWalkthroughThis PR fixes date-swallowing phone redaction, adds incomplete-report markers to Deep Research tools on abnormal termination/timeout, resolves symlinks before atomic writes, backs up and atomically rewrites MCP config, and removes the invalid session-cookie fallback in browser auth, plus adds regression tests and minor TOML/None-guard tests. ChangesAudit fixes: redaction, DR completeness, config writes, and auth
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DeepResearchTool
participant SSEStream
Client->>DeepResearchTool: call deep_research/deep_research_heavy
DeepResearchTool->>SSEStream: consume SSE events
SSEStream-->>DeepResearchTool: done event (terminated_abnormally/timeout)
DeepResearchTool->>DeepResearchTool: set truncated, timed_out
DeepResearchTool->>DeepResearchTool: append citations/sources
alt truncated
DeepResearchTool->>DeepResearchTool: append _dr_incomplete_note(timed_out)
end
DeepResearchTool-->>Client: final response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e611c29d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def _phone_repl(m: re.Match) -> str: | ||
| text = m.group(0) | ||
| return text if _DATE_PREFIX_RE.match(text) else "<PHONE>" |
There was a problem hiding this comment.
When a date and phone number are adjacent in one field, e.g. 2026-05-26 617-555-0123, _PHONE_RE greedily matches the entire digit/space/hyphen run. This branch then returns the whole match because it starts with a date, leaving the phone number unmasked in memories/tasks/conversation titles. Preserve only the date portion or continue redacting the remainder instead of returning the full match.
Useful? React with 👍 / 👎.
cx (GPT-5.5) review P1: _phone_repl returned the entire _PHONE_RE match unchanged when it started with a date, so 'appt 2026-05-26 617-555-0123' leaked the phone. Now only the date prefix is preserved and the remainder of the match is re-scanned. Also cx P2: the manual _from_browser fallback no longer suggests pasting the __Secure-next-auth.session-token cookie and rejects non-3-segment-JWT pastes instead of saving a value that can only 401. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsBLZ8EE6o9x4KtJ5WhHbX
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gpt2agent/setup.py (1)
162-168: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueBackup location can diverge from real target if
MCP_CONFIG_PATHis ever a symlink.
_backupwrites.bak-gpt2agentnext toMCP_CONFIG_PATHitself, while_atomic_writeresolves symlinks and writes to the real target's directory. SinceMCP_CONFIG_PATHis a fixed, non-symlinked path in practice, this is unlikely to matter today.🤖 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 `@gpt2agent/setup.py` around lines 162 - 168, The backup path handling in _backup can diverge from the actual config target if MCP_CONFIG_PATH is a symlink, since _atomic_write resolves the real destination but _backup currently writes next to the path object itself. Update the backup logic used in setup.py alongside _atomic_write so both operations resolve the same final target directory before creating .bak-gpt2agent, and keep the existing MCP_CONFIG_PATH existence/read_text flow unchanged.
🤖 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 `@gpt2agent/server.py`:
- Around line 167-177: The incomplete-note gate in deep_research and
deep_research_heavy only checks terminated_abnormally via truncated, so
timeout-only completions never append the warning. Update the done-event
handling in both paths to treat timed_out as part of the same incomplete
condition, and make the final append decision use the timeout flag as well as
abnormal termination. Verify the logic around the deep_research event loop, the
deep_research_heavy flow, and the final note construction uses the existing
timed_out variable consistently.
In `@gpt2agent/tools/_redact.py`:
- Around line 14-20: _recursive_repl in _phone_repl currently recurses through
date-shaped prefixes, which can overflow the stack on long token chains. Replace
the recursive re-entry with an iterative loop that keeps peeling off the
_DATE_PREFIX_RE prefix and applying _PHONE_RE until no prefix remains, while
preserving the same redaction behavior for redact().
---
Nitpick comments:
In `@gpt2agent/setup.py`:
- Around line 162-168: The backup path handling in _backup can diverge from the
actual config target if MCP_CONFIG_PATH is a symlink, since _atomic_write
resolves the real destination but _backup currently writes next to the path
object itself. Update the backup logic used in setup.py alongside _atomic_write
so both operations resolve the same final target directory before creating
.bak-gpt2agent, and keep the existing MCP_CONFIG_PATH existence/read_text flow
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 245d9624-263f-4f34-9eff-635131d32fa9
📒 Files selected for processing (9)
CHANGELOG.mdgpt2agent/auth.pygpt2agent/install.pygpt2agent/server.pygpt2agent/setup.pygpt2agent/tools/_redact.pytests/test_audit_2026_07_02.pytests/test_install.pytests/test_none_guards.py
| truncated = False | ||
| timed_out = False | ||
|
|
||
| async for event in conv.deep_research(q): | ||
| if event["type"] == "tool": | ||
| tool_calls.append(event["call"]) | ||
| elif event["type"] == "done": | ||
| final_text = event["text"] | ||
| refs = event.get("content_references", []) | ||
| truncated = bool(event.get("terminated_abnormally")) | ||
| timed_out = bool(event.get("timeout")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Incomplete note never fires for timeout-only case.
The PR objective is to warn when a report "may be incomplete due to timeout or abnormal termination," but the gate on line 192 (if truncated:) and line 255 only checks terminated_abnormally. If a done event sets timeout: true without terminated_abnormally, timed_out is computed but never consulted for the append decision — the note is silently skipped. Same bug in both deep_research (line 192) and deep_research_heavy (line 255).
This matches the regression tests too: only terminated_abnormally=True is exercised; there's no test asserting the note appears when only timeout is set.
🐛 Proposed fix (apply to both tools)
- truncated = bool(event.get("terminated_abnormally"))
- timed_out = bool(event.get("timeout"))
+ timed_out = bool(event.get("timeout"))
+ truncated = bool(event.get("terminated_abnormally")) or timed_outAlso applies to: 192-194, 214-225, 255-257
🤖 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 `@gpt2agent/server.py` around lines 167 - 177, The incomplete-note gate in
deep_research and deep_research_heavy only checks terminated_abnormally via
truncated, so timeout-only completions never append the warning. Update the
done-event handling in both paths to treat timed_out as part of the same
incomplete condition, and make the final append decision use the timeout flag as
well as abnormal termination. Verify the logic around the deep_research event
loop, the deep_research_heavy flow, and the final note construction uses the
existing timed_out variable consistently.
| def _phone_repl(m: re.Match) -> str: | ||
| text = m.group(0) | ||
| dm = _DATE_PREFIX_RE.match(text) | ||
| if not dm: | ||
| return "<PHONE>" | ||
| prefix = dm.group(1) | ||
| return prefix + _PHONE_RE.sub(_phone_repl, text[len(prefix):]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Where is redact() invoked, and on what kind of content?
rg -nP '\bredact\s*\(' -g '!tests/**' -C2Repository: robotlearning123/gpt2agent
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- gpt2agent/tools/_redact.py ---'
cat -n gpt2agent/tools/_redact.py
echo
echo '--- usages of redact / _phone_repl / _DATE_PREFIX_RE / _PHONE_RE ---'
rg -n "_phone_repl|_DATE_PREFIX_RE|_PHONE_RE|\bredact\b" gpt2agent -g '!tests/**' -C 2Repository: robotlearning123/gpt2agent
Length of output: 12486
🏁 Script executed:
python3 - <<'PY'
import re, sys
_PHONE_RE = re.compile(r"\+?\d[\d ()\-]{8,}\d")
_DATE_PREFIX_RE = re.compile(r"^(\d{4}-\d{2}-\d{2}|\d{1,2}-\d{1,2}-\d{4})(?=$|[^\d])")
calls = 0
max_depth = 0
depth = 0
def _phone_repl(m: re.Match) -> str:
global calls, max_depth, depth
calls += 1
depth += 1
max_depth = max(max_depth, depth)
try:
text = m.group(0)
dm = _DATE_PREFIX_RE.match(text)
if not dm:
return "<PHONE>"
prefix = dm.group(1)
return prefix + _PHONE_RE.sub(_phone_repl, text[len(prefix):])
finally:
depth -= 1
def redact(s: str) -> str:
return _PHONE_RE.sub(_phone_repl, s)
print("recursionlimit=", sys.getrecursionlimit())
for n in [1, 2, 3, 10, 50, 100, 200, 500, 900, 1000, 1100]:
calls = max_depth = depth = 0
s = " ".join([f"2026-05-{i:02d}" for i in range(1, n+1)])
try:
out = redact(s)
print(n, "ok", "calls=", calls, "max_depth=", max_depth, "len(out)=", len(out))
except RecursionError as e:
print(n, "RecursionError", "calls=", calls, "max_depth=", max_depth, type(e).__name__)
break
PYRepository: robotlearning123/gpt2agent
Length of output: 683
🏁 Script executed:
python3 - <<'PY'
import re, sys
_PHONE_RE = re.compile(r"\+?\d[\d ()\-]{8,}\d")
_DATE_PREFIX_RE = re.compile(r"^(\d{4}-\d{2}-\d{2}|\d{1,2}-\d{1,2}-\d{4})(?=$|[^\d])")
calls = 0
max_depth = 0
depth = 0
def _phone_repl(m: re.Match) -> str:
global calls, max_depth, depth
calls += 1
depth += 1
max_depth = max(max_depth, depth)
try:
text = m.group(0)
dm = _DATE_PREFIX_RE.match(text)
if not dm:
return "<PHONE>"
prefix = dm.group(1)
return prefix + _PHONE_RE.sub(_phone_repl, text[len(prefix):])
finally:
depth -= 1
def redact(s: str) -> str:
return _PHONE_RE.sub(_phone_repl, s)
def valid_dates(n):
# Cycle within valid month/day ranges to keep every token date-shaped.
months = [f"{m:02d}" for m in range(1, 13)]
days = [f"{d:02d}" for d in range(1, 29)]
out = []
i = 0
for _ in range(n):
out.append(f"2026-{months[i % 12]}-{days[i % 28]}")
i += 1
return " ".join(out)
print("recursionlimit=", sys.getrecursionlimit())
for n in [1, 2, 3, 10, 50, 100, 200, 500, 800, 900, 950, 1000, 1050, 1100]:
calls = max_depth = depth = 0
s = valid_dates(n)
try:
out = redact(s)
print(n, "ok", "calls=", calls, "max_depth=", max_depth, "len(out)=", len(out))
except RecursionError as e:
print(n, "RecursionError", "calls=", calls, "max_depth=", max_depth, type(e).__name__)
break
PYRepository: robotlearning123/gpt2agent
Length of output: 549
Avoid recursive re-entry in _phone_repl
A long chain of date-shaped tokens can recurse once per token and eventually raise RecursionError, which breaks redact() on large conversation/memory blobs. An iterative loop would keep the same behavior without the stack risk.
🤖 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 `@gpt2agent/tools/_redact.py` around lines 14 - 20, _recursive_repl in
_phone_repl currently recurses through date-shaped prefixes, which can overflow
the stack on long token chains. Replace the recursive re-entry with an iterative
loop that keeps peeling off the _DATE_PREFIX_RE prefix and applying _PHONE_RE
until no prefix remains, while preserving the same redaction behavior for
redact().
1 participant
Summary
Five audit fixes from the 2026-07-01 known-remaining list, plus the two P2 test gaps from the 2026-07-02 cx review of PRs #18/#20.
_PHONE_REmatched calendar dates —"2026-05-26"came back as<PHONE>in memories, tasks, and conversation titles. Phone matches that start with a date shape are now left alone; real phone shapes still mask.deep_research/deep_research_heavydropped theterminated_abnormally/timeoutdone-flags, so a truncated report was indistinguishable from a complete one. Both now append an explicit "⚠ Report may be incomplete" note._atomic_writerename()d over symlinked configs, replacing the link with a plain file and stranding the dotfile-repo target. Now resolves and writes through.write_mcp_configblindlywrite_text'd~/.gpt2agent/config.toml. Now backs up (.bak-gpt2agent), writes atomically, and no-ops on identical content.access_token— that token 401s on every API call; extraction now fails visibly.Tests
tests/test_audit_2026_07_02.py(14) + cx-review P2 gap coverage intest_install.py(combined commented header + commented child subtable) andtest_none_guards.py(conversation/file/task None-contract).163 passed, 9 skipped;ruff check gpt2agent testsclean.🤖 Generated with Claude Code
https://claude.ai/code/session_018FAemSFrVynaESb72jVGwP
Summary by CodeRabbit
Bug Fixes
Tests