chore(opencode-go-fixes): improve reviewer resilience, auth mapping, and shell reporting - #12
Merged
Conversation
Schema-validated .coderabbit.yaml with assertive review profile, Python-specific tool tuning (ruff on, flake8/pylint off), security scanners (semgrep, trufflehog, gitleaks, trivy, zizmor), path instructions for telemetry PII, type safety, CI workflow security, and the zero-new-runtime-deps policy gate.
Wire CONTRIBUTING.md and AGENTS.md into knowledge_base.code_guidelines so CodeRabbit enforces project standards (zero-deps policy, agent conventions) automatically during reviews. Add labeling_instructions to map the four existing repo labels to conventional commit prefixes for accurate suggestions.
…cy wording Address CodeRabbit review comments: - type(scope)?: description — scope is optional in conventional commits - CONTRIBUTING.md: prefer stdlib/internal modules; request exception rather than hand-rolling, to guide contributors toward the approval path
The merge from main into this branch reverted the labeling_instructions and knowledge_base.code_guidelines additions. Restore them.
pyproject.toml touched to satisfy branch protection required status checks (check, test, release-validate) that only run when tracked paths change.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughMulti-area PR: surfaces retry validation errors and truncates finding titles; adds fenced JSON report parsing and Rich rendering; makes scratchpad session-start idempotent with label collapsing; enriches OpenCode Go models via models.dev; grants SetTodoList to subagents; UI, feedback, terminal, Homebrew, and tests updated. ChangesReview System and Report Rendering
Session Management and Authentication
Model Discovery and Provider Enrichment
Agent Tooling and UI Viewport
Feedback Command and Terminal Rendering
Distribution and Documentation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.coderabbit.yaml:
- Line 93: Update the conventional commit types in the requirements string to
include the missing types so linting matches project policy: modify the value of
the requirements key (the line containing "Follow conventional commits:
type(scope)?: description. Valid types: feat, fix, chore, docs, refactor, test,
ci, perf.") to add build, style, and revert (e.g., "Valid types: feat, fix,
test, refactor, chore, style, docs, perf, build, ci, revert") so commit
validation accepts those types.
In `@pyproject.toml`:
- Line 179: Replace the empty line that is intended to silence the
false-positive typo detector by adding the assignment ba = "ba" in
tasks/agent-behavior-findings.md at the location flagged (around line 92,
session ID 06ba6c38); ensure the token name is exactly ba and the value is "ba"
so the pipeline no longer flags the "ba" token.
In `@src/pythinker_code/ui/shell/components/report.py`:
- Around line 171-174: In parse_report_block, the current except block silently
returns None on json.loads failure; instead catch the exception as e and emit a
debug-level log (using the module/class logger) that records non-sensitive
diagnostics such as payload type and length (e.g., type(payload), len(payload)
if payload is a string/bytes) and include the exception via exc_info=True, then
return None; ensure you do not log the raw payload content itself to avoid
leaking sensitive data.
In `@tasks/agent-behavior-findings.md`:
- Line 92: The typo checker is flagging "ba" in hex session IDs; update the
typos config by adding an allowlist entry under the
[tool.typos.default.extend-words] table: add the key/value pair ba = "ba" (i.e.,
include the symbol "ba" as an allowed word) so the pipeline no longer flags
session IDs like `06ba6c38`; modify the section labeled
[tool.typos.default.extend-words] in pyproject.toml to include this entry.
🪄 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: c992c8d7-6173-4a6d-953d-490b2bbacae5
📒 Files selected for processing (45)
.coderabbit.yamlAGENTS.mdCONTRIBUTING.mdpackages/pythinker-review/src/pythinker_review/reviewers/common.pypackages/pythinker-review/src/pythinker_review/reviewers/prompts/code_review.system.mdpackages/pythinker-review/src/pythinker_review/reviewers/prompts/debug_review.system.mdpackages/pythinker-review/src/pythinker_review/reviewers/prompts/deslopify_review.system.mdpackages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.mdpackages/pythinker-review/src/pythinker_review/reviewers/schema.pypackages/pythinker-review/tests/unit/test_reviewers.pypackages/pythinker-review/tests/unit/test_schema.pypyproject.tomlsrc/pythinker_code/agents/default/code_reviewer.yamlsrc/pythinker_code/agents/default/coder.yamlsrc/pythinker_code/agents/default/debugger.yamlsrc/pythinker_code/agents/default/explore.yamlsrc/pythinker_code/agents/default/implementer.yamlsrc/pythinker_code/agents/default/plan.yamlsrc/pythinker_code/agents/default/review.yamlsrc/pythinker_code/agents/default/security_reviewer.yamlsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/agents/default/verifier.yamlsrc/pythinker_code/auth/opencode_go.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/scratchpad.pysrc/pythinker_code/ui/print/visualize.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/messages.pysrc/pythinker_code/ui/shell/components/render_utils.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/components/settings_list.pysrc/pythinker_code/ui/shell/selector.pysrc/pythinker_code/ui/shell/visualize/_blocks.pytasks/agent-behavior-findings.mdtests/auth/test_opencode_go_auth.pytests/core/test_agent_spec.pytests/core/test_auth_error_handling.pytests/core/test_default_agent.pytests/core/test_scratchpad.pytests/core/test_subagent_builder.pytests/ui_and_conv/test_print_final_only.pytests/ui_and_conv/test_report.pytests/ui_and_conv/test_selectors_simple.pytests/ui_and_conv/test_settings_selector.pytests/ui_and_conv/test_tui_components.py
elkaix
changed the title
- Add `ba = "ba"` to typos allowlist so hex session IDs don't trip the spell-checker (fixes Typos CI failure) - Apply ruff format to opencode_go.py and test_opencode_go_auth.py (fixes check CI failure) - Expand coderabbit.yaml valid commit types to match pr-title-checker regexp (build, style, revert, obs were missing) - Add debug-level logging to parse_report_block on JSON decode failure (non-sensitive: logs type and length only, no raw payload)
…tance errors Cast narrowed dict variables to dict[str, Any] before calling .get() to avoid pyright treating isinstance-narrowed Any as dict[Unknown, Unknown]. Remove redundant isinstance(model_id, str) check (key is already str from cast) and drop dead str|bytes guard in parse_report_block (payload: str).
Replace the terminal-prompt + OAuth/HTTP-POST flow with a single webbrowser.open() call to the GitHub new-issue chooser URL. Users no longer need to type feedback in the terminal; /feedback opens the issue form in the browser immediately. Remove dead helpers (_feedback_github_config, _feedback_issue_title, _feedback_issue_body) and update tests to match the new behavior.
Onefile builds re-extract ~70 MB to a temp dir on every launch, causing 10+ second cold starts on macOS (Gatekeeper re-validates each extracted file). Onedir builds extract nothing at runtime and avoid this entirely. Update asset name templates to use -onedir suffix, install the full directory tree (launcher + _internal) into libexec via Dir["*"], and update tests to match the new names and install step.
Contributor
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 `@tests/ui_and_conv/test_shell_feedback_slash.py`:
- Around line 25-27: The _run helper method defined as def _run(self, fn,
*args): ... is unused and should be removed to clean up the test file; delete
the entire _run method (including its definition and body) from
tests/ui_and_conv/test_shell_feedback_slash.py so tests call
shell_slash.feedback directly as they already do, ensuring no other references
to _run remain.
🪄 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: e14ef9d0-f0f7-489c-abbc-484bcc0bc1b7
📒 Files selected for processing (7)
packages/homebrew-tap/generate-formula.pypackages/homebrew-tap/pythinker-code.rb.tmplsrc/pythinker_code/auth/opencode_go.pysrc/pythinker_code/ui/shell/components/report.pysrc/pythinker_code/ui/shell/slash.pytests/test_homebrew_formula.pytests/ui_and_conv/test_shell_feedback_slash.py
This was referenced May 29, 2026
elkaix
added a commit
that referenced
this pull request
Jun 14, 2026
…eme parity, agent preview, MCP type filter) (#143) * docs(tasks): add verified reference-adoption catalog 25-agent gap-analysis scout of the blackbox agent-harness reference vs current pythinker. Of ~75 candidate patterns only 13 are actionable (1 adopt-now + 12 adapt); ~44 are already-have (pythinker is frequently more robust), the rest stub-only/rewrite-defer/anti-pattern. Items are framed generically (technique, not the reference's proprietary text) and fit the existing soul/ loop (no swap). Planned in 4 waves: low-risk hardening, permission safety, loop/hook quality, heavier/cross-package. * feat(cli): add read-only `system-prompt` command to inspect the assembled prompt Renders an agent's fully-assembled system prompt (work dir, OS/shell, merged AGENTS.md, discovered skills) without creating a session, authenticating, or loading MCP. Adds `render_agent_system_prompt` + `build_builtin_system_prompt_args` in soul/agent.py (filesystem/environment only; mirrors the arg assembly in Runtime.create for the read-only inspection path) and a lazy `system-prompt` subcommand. Wave 1 of the reference-adoption arc (adopt-now: dump-system-prompt-entrypoint). * fix(shell): interpolate timeout caps into the tool description from the enforced constants bash.md/powershell.md hardcoded the foreground (300s) and background (86400s) timeout literals, which could silently drift from the MAX_FOREGROUND_TIMEOUT / MAX_BACKGROUND_TIMEOUT constants that Params actually enforces. Replace the literals with ${MAX_FOREGROUND_TIMEOUT}/${MAX_BACKGROUND_TIMEOUT} placeholders rendered from the same constants, and add drift-guard tests (sentinel-render the markdown; assert the live description states the enforced caps, never a raw placeholder). Rendered output is identical for the current values. Wave 1 of the reference-adoption arc (adapt: tool limits interpolated from enforced constants). * feat(memory): warn that recalled notes are a point-in-time snapshot The recall block already framed memory as past context that is not an instruction, but did not warn that a recalled fact may be stale — a file, flag, path, or decision it names may have changed or been removed. Add a freshness caveat instructing the model to verify against current code before relying on any recalled fact (failure-truthfulness C13: stale data must not read as authoritative). Single injection site, so the caveat is inlined (no helper). Wave 1 of the reference-adoption arc (adapt: per-memory freshness disclaimer). * feat(toolset): bound parallel-safe tool fan-out with a concurrency cap Parallel-safe tools previously overlapped without bound: a turn emitting many parallel-safe calls (e.g. dozens of FetchURL) opened that many sockets/file handles at once. Add an asyncio.Semaphore(10) to _ReadWriteGate acquired BEFORE the writer-lock/counter bump so the cap throttles readers without affecting writer draining (a reader queued on the cap has not incremented _active_readers, so it never holds _readers_drained open, and writers never touch the semaphore). Tests cover the cap and the no-deadlock writer-draining invariant. Wave 1 of the reference-adoption arc (adapt: bounded parallel fan-out cap). * feat(loop): enforce an optional per-session USD spend ceiling Cost was accumulated and displayed but never enforced. Add loop_control.max_session_cost_usd (off by default): when the session's accrued estimated cost reaches the ceiling, the turn stops with a new budget_exhausted outcome and a handoff message before starting another paid step, instead of running to max_steps_per_turn. Goal auto-continuations already halt on any non-no_tool_calls outcome; agent flows now halt on budget_exhausted too. Cost is 0 for unpriced models, so the ceiling fails open (never blocks un-estimable spend). Decision/message logic are pure, boundary-tested helpers. Wave 1 of the reference-adoption arc (adapt: per-session USD spend ceiling). * feat(approval): always re-confirm edits to sensitive host files Add an always-re-confirm deny-set for dangerous host paths so an auto-approve tier can never silently approve them. New is_dangerous_host_path() flags shell startup files, .git internals/hooks, .ssh, .vscode, and git credentials; a new FileActions.EDIT_DANGEROUS is classified BEFORE the workspace branch (so a .git/hooks write inside the workspace is no longer a plain EDIT). The approval gate treats EDIT_DANGEROUS exactly like EDIT_CONFIG via a shared _is_always_confirm_edit predicate: re-confirms even under yolo/auto, never session-approved, denied with feedback in unattended runs. Security-reviewed (SAFE TO MERGE): full parity with the config-edit gate, no fail-open bypass, strict superset of prior protection. Known parity limitation (pre-existing, shared with EDIT_CONFIG, deferred): a .git/.ssh/.vscode directory that is itself a symlink evades name-matching after realpath; Shell-tool writes bypass classify_edit_action entirely. Wave 2 of the reference-adoption arc (adapt: dangerous-dotfile deny-set). * feat(approval): add accept-edits auto-approve tier for reversible in-workspace edits New /accept-edits toggle auto-approves only plain FileActions.EDIT (reversible, in-workspace, non-destructive ordinary file edits) while shell, destructive, outside-workspace, config-surface, and dangerous host edits still prompt. Adds a session-local accept_edits flag (not persisted), _accept_edits_covers() gated by safe_mode (like auto, unlike yolo), a request() auto-approve branch placed after deliberation/unattended-deny, and the /accept-edits slash command. The enable message is safe_mode-truthful (reports edits will still prompt under safe mode). Pairs with the Wave 2 deny-set: dangerous paths classify as EDIT_DANGEROUS (checked first), so they are never covered by EDIT auto-approval. Security-reviewed (SAFE TO MERGE): over-approval bounded to EDIT only; EDIT is produced by exactly one path (classify_edit_action); no persistence leak. Wave 2 of the reference-adoption arc (adapt: accept-edits mode tier). * feat(loop): add terminal-quality predicate (observable degenerate-stop signal) Print mode exits 0 on any non-exception completion, including a stuck/empty terminal that delivered no usable answer. Add a TurnOutcome.produced_answer derived property: True only when the turn ended with a substantive assistant answer (no_tool_calls stop with non-empty text); a forced handoff (stuck / budget_exhausted), a rejected tool call, or an empty final message is False. Surfaced on the existing turn span (turn.produced_answer) and turn.end event so the degenerate-completion rate is measurable. Observational-first: exit-code mapping is deliberately deferred until the rate is measured, to avoid false-positives on tool-only-then-stop turns. Wave 3 of the reference-adoption arc (adapt: terminal-quality success predicate). * feat(subagents): gate spawn on declarative required MCP servers A markdown agent's frontmatter can declare required_mcp_servers; spawning that type via the Agent or RunAgents tools is rejected with a clear message when the servers are configured-and-absent (after MCP loading settles), instead of wasting a turn on an agent that cannot reach its tools. While MCP is still loading the spawn is allowed (servers may yet connect). Adds required_mcp_servers to AgentTypeDefinition + markdown frontmatter; a root-only Runtime.mcp_status seam wired from the toolset snapshot; a pure _missing_required_mcp_servers decision function; and AgentTool.check_required_mcp_servers reused by both spawn tools. Also fixes a TurnStopReason annotation in the item-8 test that make check surfaced. Wave 3 of the reference-adoption arc (adapt: required-MCP spawn gate). * feat(hooks): inject UserPromptSubmit additionalContext into the user turn A UserPromptSubmit hook could block a prompt but its additionalContext was ignored. Now a non-blocking hook's additional_context is appended to the user turn as a system reminder so the model sees it as context for this prompt, via a pure _user_message_with_hook_context builder. Slash-command parsing keeps reading only the user's original text, never the appended hook context. Ships the clean UserPromptSubmit half per the catalog's staging; the PostToolUse half (await the currently fire-and-forget trigger, gated on has_hooks_for, to inject its additionalContext) is a documented follow-up. Wave 3 of the reference-adoption arc (adapt: hook additionalContext feedback). * docs(tasks): record adoption-arc status + Wave 4 execution notes Waves 1-3 done (10 items); capture the read-before-write technique, the project-context refactor's test-pin risk, and the max-output-token cross-package blocker for Wave 4 so the remaining work is executable cleanly. * feat(file): stale-overwrite guard via a per-agent file read cache If the agent read a file and it then changed on disk (user edit or another tool), a WriteFile overwrite is now rejected so the external change is not clobbered. ReadFile records the mtime at read; WriteFile rejects an overwrite whose on-disk mtime is newer than the recorded read, and refreshes the read-state after its own write (so consecutive edits are never falsely flagged); StrReplaceFile likewise refreshes after editing. First-contact writes (a file never read) are unaffected. Adapts the reference's read-before-write/file-state-cache to the stale-detection half only — a full read-before-write requirement is incompatible with pythinker's 'write without read' contract (would break ordinary regenerate-file flows). The cache (utils/file_read_cache.py, placed there to avoid a tools<->soul import cycle) keys on the canonical real path, fails open on stat errors, and is per-agent. Unit + e2e tests (stale-blocked, allowed-after-read, consecutive). Wave 4 of the reference-adoption arc (adapt: stale-read detection). * docs(tasks): record executable plan for the remaining Wave 4 items (12, 13) Item 13: reverse-engineered the precise finish_reason seam in PythinkerStreamedMessage -> GenerateResult.truncated -> soul escalation ladder. Item 12: documented the marginal-value/high-brittleness assessment. * feat(core): surface a truncation signal on GenerateResult pythinker-core captured no finish_reason, so a response cut off by the output-token limit (visible text then cap) was returned as a clean completion — the agent loop could not tell truncation from a normal stop. Capture the provider's finish_reason in PythinkerStreamedMessage (both stream and non-stream paths) and set GenerateResult.truncated when it is 'length'. _generate reads it optionally (getattr) so other stream implementations are unaffected. MockChatProvider gains a finish_reason arg for testing. Stage 1 of the max-output-token recovery ladder (Wave 4, item 13); the soul-side escalate-then-nudge recovery follows in stage 2. * feat(loop): recover from output-token truncation with bounded continuation nudges Stage 2 of item 13. StepResult now carries truncated (propagated from GenerateResult). When a step is cut off by the output-token limit and makes no tool call, the soul nudges the model to continue from where it stopped instead of ending the turn with a half-finished answer reported as complete (C06/C13 truthfulness gap). Bounded by loop_control.max_truncation_recoveries (default 3; 0 disables) per turn so a persistently-truncating model cannot loop forever. Decision is a pure, parametrized-tested function; integration tests cover the nudge-and-continue path and the disabled-at-zero path. Wave 4 of the reference-adoption arc (adapt: max-output-token recovery). * polish(subagents): harden required-MCP gate diagnostics Warn (and ignore) a non-list required_mcp_servers frontmatter value, and point a gated spawn at `pythinker mcp auth <server>` for a configured-but-unauthorized server in addition to `pythinker mcp add`. Reformat path.py. Follow-up polish to Wave 3 item 9. * feat(file): extend the stale-overwrite guard to StrReplaceFile Wave 4 follow-up hardening for the reference-adoption arc: - StrReplaceFile now applies the same stale-read guard as WriteFile via a shared overwrite_is_stale() helper. Exact old-string matching alone cannot catch an external edit that leaves the matched string intact while changing the rest of the file, so gate on the recorded read mtime as WriteFile does. First-contact edits and a tool's own consecutive edits stay unblocked. - Pin the custom .githooks (core.hooksPath) case in the dangerous-host-path deny-set test and call it out in the changelog. - Pin the system-prompt CLI in the PyInstaller hiddenimports expectation so the one-file build keeps shipping it. - Assert the UserPromptSubmit hook-context path wraps stdout in the untrusted-data envelope and strips invisible smuggling characters. * docs(tasks): record Wave 4 outcomes (#11/#13 done, #12 architectural no-go) - #11 and #13 are committed; mark them DONE in the execution status with their commit refs and the scope adaptations made. - #12 (move AGENTS.md to a separate session-start <system-reminder>) is recorded as an architectural no-go: AGENTS.md must survive compaction and not truncate, and only the system prompt (stored separately, never summarized, 32 KiB budget) satisfies both. A seed message is lossily summarized at the first compaction (prepare() preserves only the last 2 messages verbatim); a dynamic injection is hard-capped at 2048 tokens. Both alternatives would need new compaction-pinning or unbudgeted-injection machinery the project's MVC rules forbid, for marginal non-reference cache value. Keep AGENTS.md in the system prompt. * feat(prompt): deliver merged AGENTS.md as a session-start system-reminder preamble Move the merged AGENTS.md out of the immutable system prompt (system.md §11) into a session-start, user-role <system-reminder> message that is assembled fresh from runtime state on every model request and is never written to context.history. Robustness — this placement is the whole point. AGENTS.md carries the project's non-negotiable rules, so it must (a) survive compaction and (b) never be truncated. Two tempting homes fail: - a persisted seed message is lossily summarized at the first compaction (the compactor preserves only the last few messages verbatim); - a dynamic injection is hard-capped by the 2048-token injection budget. Assembling it at the step boundary, outside history, sidesteps both: compaction cannot summarize what it never sees, and the injection budget cannot truncate a non-injection. Net per-request tokens are unchanged (it simply moves from the system role to a leading user-role reminder), and the system prompt is now free of per-project content. - render_agents_md_reminder(builtin_args): authoritative <system-reminder> body, or None. - _with_agents_md_preamble(history, builtin_args): prepend it, unmutated, ahead of history. - _step assembles normalize_history(_with_agents_md_preamble(...)). - system.md §11 explains the separate delivery instead of interpolating the block. - pythinker system-prompt appends the reminder below a labeled divider so the dump stays faithful. Resolves the agent.py:66 TODO. Closes Wave 4 #12 of the reference-adoption arc. * fix(core): make output-token truncation a required StreamedMessage signal GenerateResult.truncated drives the agent loop's truncation recovery, but finish_reason was only an optional getattr property implemented by the Pythinker and Mock providers. Every other provider returned None, so a reply cut off by the output-token limit was silently treated as a clean completion — truncation recovery never fired for Anthropic, OpenAI-legacy, OpenAI-responses, or Google (4 of 5 real providers). Silent degradation in an execution-critical path. Make finish_reason a required member of the StreamedMessage Protocol so no provider can omit it, and map each provider's native truncation signal onto the loop's 'length' value: - Anthropic: stop_reason 'max_tokens' -> 'length' (non-stream + message_delta). - OpenAI-legacy: native choices[0].finish_reason ('length' already). - OpenAI-responses: incomplete_details.reason 'max_output_tokens' -> 'length' (handles the response.incomplete terminal event). - Google GenAI: candidate.finish_reason MAX_TOKENS -> 'length'. - Chaos: delegates to the wrapped stream; Echo/ScriptedEcho never truncate. _generate now reads stream.finish_reason directly (contract-guaranteed). Adds a two-sided test pinning finish_reason='stop' -> not truncated so the negative side can't pass only because the default is falsy. * fix(file,mcp): harden stale-overwrite detection and the required-MCP startup gate Two review-driven robustness fixes: - Stale-overwrite guard now tracks (mtime, size), not mtime alone. An external edit within the same filesystem-mtime tick, or one that preserves/restores the mtime (touch -r, archive extraction), left mtime unchanged and slipped past the guard; comparing size as well catches those size-changing edits. ReadFile records both from one stat; Write/StrReplace refresh both after their own write. - mcp_status_snapshot() is now loading-honest: it returns a loading=True snapshot (instead of None) while a deferred MCP startup is queued but has not populated the server map yet. None now means only "no MCP configured", so the required-MCP spawn gate can no longer mistake the startup window for a genuinely-absent server and reject a first-turn subagent. Closes the gap between _missing_required_mcp_servers' documented "[] while still loading" contract and its None-snapshot handling. * feat(auth,providers): add Kimi provider and GLM-5.2 defaults, fix MiniMax usage - Add Kimi K2.7 Code model and new Kimi Coding Plan provider with Moonshot Anthropic-compatible endpoint - Set GLM-5.2 as default Z.AI model with pinned catalog entry for 1M context - Fix MiniMax token-plan adapter to read percentage-metered usage and convert reset times from milliseconds to seconds - Enhance UI shell modules to surface new providers and integrate new platform catalog entries - Add comprehensive tests for Moonshot and Z.AI auth, and MiniMax usage adapter Reference adoption Wave 2 delivery. * fix(core,file): satisfy the finish_reason contract in test doubles; suppress post-write stat Adversarial-review follow-ups. - Making finish_reason a required StreamedMessage member meant the six soul/auth test doubles that drive a real generate() (StaticStreamedMessage, SequenceStreamedMessage, PartialThenErrorStreamedMessage, and the notifications / cumulative-usage doubles) raised AttributeError at _generate's direct stream.finish_reason read. Give each the no-op property so the now-required contract is satisfied and the suites stay green. - WriteFile's post-write stat() was the only one of the three file tools not wrapped in contextlib.suppress(OSError): a stat hiccup after a successful write fell through to the except handler and reported a false write failure (and skipped the cache refresh, which could then trip a spurious stale flag on retry). Suppress it, keep the success result, and omit the size note when the stat is unavailable — matching read.py and replace.py. * feat(file): reject a full overwrite of a partially-read file The stale-overwrite guard caught external changes but not the read-completeness case the deep-scan flagged: the agent reads only part of a large file (a capped or offset ReadFile), then WriteFile-overwrites it — silently discarding the lines it never saw, with no external change for the mtime/size guard to catch. Track read completeness alongside (mtime, size): a ReadFile that started at line 1 and returned every line is complete; a capped/offset read is not. A WriteFile overwrite of a file whose recorded read was incomplete is now rejected with an actionable message ("read it in full first"). Scope is deliberately narrow so the write-without-read contract is preserved: first-contact writes, full-read-then- overwrite, append, and all StrReplaceFile edits (which preserve unseen bytes) are unaffected. Completeness is sticky — a full read is not undone by a later partial re-read of the unchanged bytes — and an overwrite/StrReplace refreshes it to complete (the agent then knows the whole content), while an append leaves it incomplete (the prior content is still unseen). * Revert "feat(file): reject a full overwrite of a partially-read file" This reverts commit af531ea. * feat(tui): review findings table, lighter borders, and RunAgents polish Three visual improvements to the TUI tool-card renderer: 1. **Border reharmonisation** — `border` token updated from the old slate blue (#3A506D) to a neutral light grey (#e8ebed); `border_muted` follows suit (#2B3A52 → #b8bcc0). Panels and markdown table borders now recede instead of competing with accent colours. 2. **RunAgents review findings table** — after a completed RunAgents run that includes `code-reviewer`, `security-reviewer`, or `review` subagents, a compact findings panel is appended below the agent tree. It parses structured severity markers from each reviewer's prose (five forms: `[SEVERITY]` bullets, colon-bullets, bold-start, named-severity subsection headers, and markdown table rows) and aggregates Critical / High / Medium / Low counts with a "Reported by" column listing the contributing agents. Unparsed reports are tracked separately so the footer ("Parsed N/M · K kept as unparsed prose") communicates parsing confidence explicitly. 3. **RunAgents name-extra label** — agent entries whose `name_extra` field is set now render the extra label in bold tool-title style instead of plain text. Parser guards: - `_RE_SEVERITY_IN_HEADER` uses a `(?!-)` lookahead so "## High-level overview" is not classified as a high-severity section. - Mid-sentence severity words ("not a high-risk change") are never counted. 17 direct unit tests verify exact severity counts, reporters mapping, and aggregate behaviour. 7 renderer-level integration tests cover end-to-end rendering. Related border-colour assertions updated in two existing test files. * feat(tui): standardize welcome banner colors and diff sign spacing - Directory label: periwinkle accent (#B3B9F4) to match skill/highlight color - Branch label: teal from statusline palette (#64d2c8) to match bottom bar branch - Diff signs: remove trailing space after +/- so format is {ln} -{content} not {ln} - {content} - Update tests: accent token assertion for Directory, spacing assertions for diff output * fix(core): make Gemini finish_reason sticky once 'length' is captured A second candidate with a non-MAX_TOKENS finish_reason (e.g. STOP) would overwrite a previously captured 'length', silently dropping truncation signal. Guard the assignment so 'length' is never overwritten. * test(core): add streaming + reverse-order sticky-truncation test coverage * fix(soul): wire budget-exhausted and stuck-loop messages to shell Both handoff messages were appended to context but never sent to the wire, so the interactive shell showed no feedback on budget ceiling or stuck-loop exits. Add wire_send(TextPart) after each append_message. * fix(ui): sync dark-theme ptk hex values to TUI token constants _PROMPT_STYLE_DARK carried six stale slate hexes that diverged from the current _TUI_TOKENS_DARK border/border_muted values. Update them and add a parity test that binds the ptk layer to the token constants. * fix(ui): restore preview line for non-review successful agents The agent tree renderer only showed preview text on error/failed agents. Non-review successful agents with a brief or summary_preview now also show a preview line so completed work is visible in the tree. * fix(subagents): drop non-string values from required_mcp_servers YAML non-string values (integers, booleans, null) were coerced to strings via str(s), creating permanently unsatisfiable MCP server names. Filter to isinstance(s, str) instead so only real string entries are retained. * docs(changelog): add Unreleased entries for deep-scan fixes Five correctness and UX fixes: Gemini sticky truncation, shell wire handoff for budget/stuck messages, dark-theme ptk hex parity, non-review agent summary preview, and required_mcp_servers type filter. * style(memory,tests): satisfy CodeQL implicit-concat and mixed-import nits - recall.py: make the two wrapped recall-block list entries explicit `+` concatenations instead of relying on implicit adjacent-literal concat (CodeQL py/implicit-string-concatenation-in-list). Output is byte-identical. - test_shell_timeout_drift.py: drop the redundant `from pythinker_code.tools.shell` import and qualify Shell/MAX_*_TIMEOUT through the already-imported `shell_mod` so the module is imported in a single style (CodeQL mixed import). * fix(config): resolve scoped config for read-only system-prompt dump pythinker system-prompt fell back to an empty Config() whenever ~/.pythinker/config.toml was absent, silently dropping project/local scoped settings (e.g. extra_skill_dirs) and printing an inaccurate prompt. Add a persist=False mode to load_config/_load_scoped that runs the full user→project→ local merge with no disk side effects — no share-dir/lock creation, default seeding, JSON→TOML migration, or auto-gitignore, and skipping the project-trust read (trust only gates hooks, which no read-only consumer renders). The default persist=True path is byte-for-byte unchanged. Regression test covers no-user-config + project-config-present and asserts no config is seeded. * fix(soul): re-check spend ceiling after billable compaction The per-session spend ceiling was checked only at the top of the step loop, but proactive compact_context() makes a billable LLM call that raises _session_cost_usd. A session just under max_session_cost_usd could therefore pay for compaction and still run a full model step before the guard re-fired. Add a second ceiling check immediately after the proactive compaction (before _step()), reusing the existing budget_exhausted handoff. Regression test asserts no model step runs once compaction pushes the session over the ceiling. * fix(file): re-check staleness after approval to close TOCTOU window StrReplaceFile and WriteFile validated staleness before the (unbounded) approval prompt, then wrote the in-memory content wholesale — so an external edit landing during approval was silently clobbered (exact-string matching ran pre-approval and gives no write-time protection). Add a second overwrite_is_stale / _reject_if_stale check immediately after approval, returning the same stale-read error. Regression tests mutate the file during the approval call and assert the external change survives. * style(soul,tests): address CodeRabbit type/hygiene nits - slash.py: annotate the new accept_edits handler with -> None (public src rule). - test_pythinkersoul_steer.py: annotate the finish_reason test-double property. - test_agent_tool.py: replace the hardcoded /tmp agent_file with the in-file runtime.subagent_store.root pattern, and split the combined assertion for clearer failure messages. * fix(typo): toggleable, edge_diagnostic Fix the two repo-wide spellcheck failures flagged by the typos CI gate: 'togglable' -> 'toggleable' in a test comment, and 'ede_diagnostic' -> 'edge_diagnostic' in a planning note.
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
/feedbacknow opens the GitHub “new issue” page in the browser.Improvements
Bug Fixes