feat(forge): Add Forge agent cli session support#1
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
![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: 68dbb36708
ℹ️ 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".
| ptc.SkillName = tc.Get("arguments.skill").Str | ||
| } | ||
| case "task": | ||
| ptc.SubagentSessionID = tc.Get("arguments.session_id").Str |
There was a problem hiding this comment.
The task tool call stores arguments.session_id verbatim, but Forge session rows are keyed as forge:<conversation_id> (see buildForgeSession). When Forge emits bare IDs (e.g. conv-123), tool_calls.subagent_session_id will not match any sessions.id, so parent/child linking via LinkSubagentSessions cannot work and subagent relationships are silently lost for those sessions.
Useful? React with 👍 / 👎.
| if err := e.writeSessionFull( | ||
| pendingWrite{sess: *sess, msgs: msgs}, | ||
| ); err != nil && !errors.Is(err, db.ErrSessionExcluded) { | ||
| return fmt.Errorf("write session %s: %w", sess.ID, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
syncSingleForge writes the session and returns immediately, but unlike the file-based single-session path it never calls e.db.LinkSubagentSessions(). As a result, after SyncSingleSession("forge:..."), any newly written Forge task tool-call edges are not reflected in sessions.parent_session_id until a later sync run triggers linking elsewhere.
Useful? React with 👍 / 👎.
|
Hello @mjacobs, I have reviewed the changes for adding Forge agent CLI session support. The implementation looks solid, covering the necessary parsing logic, database integration, and sync engine updates. The added tests in |
SubagentSessionID on task tool calls was stored as a bare conversation ID (e.g. "conv-123"), but sessions are keyed with the "forge:" prefix. Without the prefix, LinkSubagentSessions cannot resolve parent/child edges for Forge sessions. Also call LinkSubagentSessions after syncSingleForge so that edges written during a targeted resync are reflected immediately rather than waiting for the next full sync. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@mjacobs are you planning to upstream this? all agents are welcome |
Will do, I'll send a PR upstream :) |
## What
Adds a `--write-timeout` duration flag to `serve` and `pg serve`, making the API
write deadline configurable. The default is unchanged at 30s. A non-positive value
disables the deadline.
## Why
The write deadline was hardcoded at 30s. On large shared datasets the full-history
analytics aggregates (heatmap, activity, usage summary) can exceed it and return
`503` with `{"error":"request timed out"}`, which surfaces as blank "request timed
out" dashboard panels. Operators had no way to raise it. Relates to kenn-io#1147.
## Where to look
- `internal/config/config.go` — the flag is registered on both the `flag` and
`pflag` serve flag sets (so `serve` and `pg serve` both accept it) and mapped to
`Config.WriteTimeout` in `applyFlagValue`, mirroring `events-coalesce-interval`.
- `internal/server/middleware.go` — the standard-handler timeout wrapper now
bypasses `http.TimeoutHandler` when the timeout is non-positive, so `0` disables
the deadline instead of firing immediately. The typed (Huma) API path already
had this guard; this brings the two paths into agreement.
- `docs/remote-access.md` — documents raising the timeout for slow aggregates and
the flag reference row.
## Tradeoffs and limitations
- Raising the timeout treats the symptom. When aggregates are slow enough to need a
large value, the underlying database-side cost is usually the real fix — for a
multi-tenant read role, a set-based row-level-security predicate rather than a
per-row function call. The docs note points there; #1 has the detail.
- Flag only; no new config-file or environment key was added, consistent with the
neighboring serve duration flags.
Co-authored-by: TechnoPhobe01 <Technophobe01@users.noreply.github.com>
On pg serve, the embeddings status endpoint cannot distinguish remote semantic wiring failures and returns a generic manager-unavailable 501. That response must not replace the search endpoint's actionable pg push remediation with local vector setup instructions. VALID (fixed): #1
The generic semantic-unavailable sentinel embeds a local SQLite workflow, so wrapping it still told DuckDB and PostgreSQL users to build local embeddings even when that could not enable search. Keep the 501 classification separate from rendered backend reasons, and ensure unsupported DuckDB and vector-disabled PostgreSQL searches explain their actual capability and push requirements. VALID (fixed): #1
Backend-specific semantic 501 messages no longer contain the full local setup sentinel. The HTTP fallback therefore treated them as unknown and prepended local embeddings-build remediation, producing contradictory errors for remote CLI and MCP callers. Reconstruct reasoned transport errors without rendering the sentinel while preserving its errors.Is classification. VALID (fixed): #1
The command palette overlay treated every bubbled Enter as result selection, canceling native keyboard activation for focused semantic setup controls. Interactive descendants now retain their keyboard events while Escape remains global and the search input keeps palette navigation behavior.\n\nVALID (fixed): #1
Palette-owned Escape handling must not reach the document shortcut after clearing the modal, because that reinterprets one keypress as a session deselection. Initial build-status probes also need to distinguish failure and successful completion from a never-built ready state so the panel does not hide errors or offer redundant work.\n\nVALID (fixed): #1, kenn-io#2
On pg serve, the embeddings status endpoint cannot distinguish remote semantic wiring failures and returns a generic manager-unavailable 501. That response must not replace the search endpoint's actionable pg push remediation with local vector setup instructions. VALID (fixed): #1
The generic semantic-unavailable sentinel embeds a local SQLite workflow, so wrapping it still told DuckDB and PostgreSQL users to build local embeddings even when that could not enable search. Keep the 501 classification separate from rendered backend reasons, and ensure unsupported DuckDB and vector-disabled PostgreSQL searches explain their actual capability and push requirements. VALID (fixed): #1
Backend-specific semantic 501 messages no longer contain the full local setup sentinel. The HTTP fallback therefore treated them as unknown and prepended local embeddings-build remediation, producing contradictory errors for remote CLI and MCP callers. Reconstruct reasoned transport errors without rendering the sentinel while preserving its errors.Is classification. VALID (fixed): #1
The command palette overlay treated every bubbled Enter as result selection, canceling native keyboard activation for focused semantic setup controls. Interactive descendants now retain their keyboard events while Escape remains global and the search input keeps palette navigation behavior.\n\nVALID (fixed): #1
Palette-owned Escape handling must not reach the document shortcut after clearing the modal, because that reinterprets one keypress as a session deselection. Initial build-status probes also need to distinguish failure and successful completion from a never-built ready state so the panel does not hide errors or offer redundant work.\n\nVALID (fixed): #1, kenn-io#2
2 participants
No description provided.