refactor(space): relocate coding artifact writers/readers out of core (#796 PR C) - #2345
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR introduces the
Confidence Score: 5/5Safe to merge — the refactor is mechanically sound, previous reviewer issues were addressed and confirmed fixed, and no correctness regressions were found. Each removed duplicate maps cleanly to a single profile call preserving the same scan order. The legacy shim removal is deliberate and covered by updated prompts and tests. The messageData fix for onGateDataCommitted is correctly implemented and regression-tested. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/daemon/src/lib/space/runtime/artifact-profile.ts | New file: defines the WorkflowArtifactProfile interface — the domain seam that lets daemon core call domain-specific methods without naming coding-specific kinds. |
| packages/daemon/src/lib/space/workflows/coding-artifact-profile.ts | New file: consolidates five previously-duplicated resolvePrUrlForRun copies, the review-posted-gate auto-save, and three kindless-decision outcome readers into CodingArtifactProfile. |
| packages/daemon/src/lib/space/tools/node-agent-tools.ts | Removes fifth resolvePrUrlForRun copy and legacy type/append compat shim from save_artifact; delegates gate-committed hook to artifactProfile. |
| packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts | SaveArtifactSchema simplified: shape now required; type and append fields removed entirely. |
| packages/daemon/src/lib/space/workflows/post-approval-merge-template.ts | All prompts migrated to explicit shape/kind; final merge audit uses shape:'link', kind:'merge'. |
| packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts | SQL tone-classification extended to match modern kind values alongside legacy _legacyType values. |
| packages/daemon/src/lib/rpc-handlers/index.ts | Wires CodingArtifactProfile at composition root, injects into EvolutionEpisodeService, SpaceRuntimeService, and TaskAgentManager. |
Reviews (3): Last reviewed commit: "fix(space): migrate end-node REC + list_..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…link Address review feedback on #2345: - CodingArtifactProfile.onGateDataCommitted now reads review_url from the current send_message payload (messageData) instead of the merged gate state. The prior form would spuriously record an extra review round when a later send updated only comment_urls while a previous round's review_url lingered in the gate state. Adds a regression test for the follow-up-send case. - Fullstack QA success now records the PR as a first-class `link kind:'pr'` (consistent with the other reviewer nodes and robust to legacy-field removal) alongside the terminal `decision` outcome, instead of burying pr_url inside the decision data. - Rebase onto dev: drop the list_artifacts legacy kind-filter and its tests added in #2319 (they re-introduced `pr`/`review` kind names into core, which PR C removes); list_artifacts now filters by shape only.
f92a64f to
6ee28f4
Compare
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (NeoKai)
Model: glm-5.1[1m] | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES (1× P1, 1× P2).
The core refactor is correct and well-executed: the WorkflowArtifactProfile seam, the consolidation of the 5 duplicated resolvePrUrlForRun copies + 3 outcome-summary readers + the review-posted-gate auto-save behind CodingArtifactProfile, and the prompt migrations. The acceptance criterion (#796: "daemon core contains NO coding-specific kind names") holds — the only remaining kind:'pr'/kind:'review' reads in core are in live-query-handlers.ts, which are display-only (timeline tone/title/category). All affected daemon tests pass (602 + 240, 0 fail). The blocker is that the legacy type/append shim was dropped while two live prompt sites still emit that API, so they now break.
P1 — End-node completion instructions still emit the removed type/append API
packages/daemon/src/lib/space/runtime/task-agent-manager.ts:3065 and :3069 — the isEndNode branch of the Runtime Execution Contract builder, injected into every end-node task-agent session (reviewer, QA, etc.):
'When your work is complete: (1) call save_artifact({ type: "result", append: true, summary: "..." }) to record the outcome, ...'
This PR made shape required and removed the type/append alias from SaveArtifactSchema. Zod now strips type/append, leaving shape undefined, so the handler returns { success: false, error: "...shape..." } (confirmed by the kept test "rejects when shape is missing — the legacy type alias is no longer accepted"). An end-node agent following these injected instructions fails to record its terminal outcome, so CodingArtifactProfile.summarizeRunOutcome returns null → the task-completion summary is missing and Forge gap-detection treats the run as result-less. This contradicts the PR's own gate ("drop the shim once prompts are migrated"). Fix at both lines:
save_artifact({ shape: "decision", summary: "..." })
(For completeness: built-in-workflows.ts:1553 also contains type: "result", but that is RETIRED_CODING_WORKFLOW_VALIDATION_STEP_PROMPT — a restamp lookup key used at line 1671 to detect/replace old seeded steps, never shown to agents as instructions. Not a bug.)
P2 — list_artifacts schema still advertises legacy type values that now return empty
packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts:424 and :427:
/** Filter by artifact type (generic string, e.g. 'progress', 'result', 'review'). */
type: z.string().describe('Filter by artifact type (e.g. "progress", "result", "review")').optional(),
This PR removed the legacy multi-shape fan-out from the list_artifacts handler and passes args.type straight through as artifactType. Artifacts are stored only as shapes (link/commit_set/check/metric/decision/note), so list_artifacts({ type: 'result' | 'progress' | 'review' }) now silently returns empty. The .describe() misleads the model into querying broken values. Update both strings to the shape vocabulary.
Notes (no action required)
- Behavior parity (benign, intentional convergence): Consolidating the four resolver strategies into one converges them on the
SpaceRuntimeorder (gate data → all hook state → artifacts). Two callers shift:TaskAgentManagerpreviously scannedpr_readyhook state before gate data (now gate data wins), andnode-agent-toolspreviously didn't scan hook state at all (now does). Both only change the resolved URL in the rare case of competingpr_urlvalues across stores — not normal coding-workflow state. Worth a one-line note in the PR description so it reads as deliberate convergence. live-query-handlers.tsmerge_conflict_loopis absent from the_legacyTypewarning-tone list, but that gap is identical to before this PR (pre-existing) and out of scope; the PR improves new-row classification by addingmerge_conflictto thekindlist.- Wiring verified complete: the single shared
CodingArtifactProfilereachesEvolutionEpisodeService,TaskAgentManager,SpaceRuntime/SpaceRuntimeService(including both temporaryChannelRouterPR-url callbacks), and the node-agent tools. No production construction path omits the profile.
Address review REQUEST_CHANGES on #2345 — two prompt sites the legacy type/append shim was still propping up: - task-agent-manager end-node Runtime Execution Contract (injected into every end-node session) still emitted save_artifact({ type: "result", append: true, summary }) — now rejected with the shim gone, so the terminal outcome was never recorded. Use shape: "decision". - ListArtifactsSchema.type .describe() advertised legacy 'progress'/'result'/ 'review', which now silently return empty. Update to the shape vocabulary.
|
Both findings fixed in 551b711:
Confirmed no other agent-facing |
|
Both findings fixed in 551b711: P1 (end-node REC now uses save_artifact({ shape: "decision", summary })); P2 (ListArtifactsSchema.type describe updated to shape vocabulary). No other agent-facing legacy save_artifact sites remain. |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (NeoKai)
Model: glm-5.1[1m] | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE (0 blocking findings).
Round 2 — commit 551b711 correctly resolves both prior findings:
- P1 (fixed) —
task-agent-manager.ts:3065&:3069now emitsave_artifact({ shape: "decision", summary: "..." }). A kindlessdecisionis exactly whatCodingArtifactProfile.summarizeRunOutcomereads as the terminal outcome, so end-node agents' outcome recording works again. Confirmed no other agent-facing legacysave_artifact({type/append})sites remain — the onlytype:"result"left isRETIRED_…VALIDATION_STEP_PROMPT(a restamp lookup key, never shown to agents). - P2 (fixed) —
ListArtifactsSchema.typeJSDoc +.describe()now advertise the shape vocabulary (link/decision/note+ closed-set reference).
Verification:
- All affected daemon unit tests pass (node-agent-tools, built-in-workflows, end-node-handoff, custom-agent: 517+ pass, 0 fail).
- CI on the PR head (551b711) is green: "All Tests Pass" = success, Coverage gate = success, all daemon unit/online shards success.
- #796 acceptance criterion holds (re-verified, unchanged by this fix): no coding-specific kind names in daemon core runtime — only display-only reads in
live-query-handlers.ts(PR B scope). Profile wiring is complete with no production path omitting it. - All review threads resolved; PR is open and mergeable.
Non-blocking observations (optional follow-ups, not gating merge):
task-agent-tool-schemas.ts:22— the file-level style-conventions comment still says "usesave_artifact... withappend: true";appendwas removed by this PR. It's a non-functional dev comment in an unchanged file — worth a one-line update when convenient.resolveLegacyShape(shared/src/artifact-shapes.ts) now has no production consumer (only its own unit test) after this PR dropped the legacy shim — candidate for removal in a separate cleanup. Knip won't flag it (the test imports it), so it doesn't block CI.
The core refactor — the WorkflowArtifactProfile/CodingArtifactProfile seam consolidating the 5 resolvePrUrlForRun copies, the review auto-save, and the 3 outcome readers out of daemon core, plus the prompt migrations and shim removal — is correct, clean, and well-tested. Approving.
|
@codex review |
![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: 551b711fff
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (NeoKai)
Model: glm-5.1[1m] | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES — reversing my round-2 APPROVE.
The author invoked @codex review, which surfaced P1 regressions I missed. I independently verified them against the code (not taking Codex's word). Do not merge/approve until these land.
P1 — shape:"decision" outcome sites are rejected by the validator (4 live instructions)
validateArtifactShape('decision', data) requires data.recommendation (shared/src/artifact-shapes.ts:323); on failure the handler returns {success:false} and does not persist (node-agent-tools.ts:1475-1478). This PR migrated outcome-recording prompts to shape:"decision" but none include recommendation, so each terminal write is silently dropped right before approve_task()/submit_for_approval():
| Site | File:line | Status |
|---|---|---|
| Dispatcher (stack) | built-in-workflows.ts:334-336 |
rejected — comment |
| Dispatcher (2nd mention) | built-in-workflows.ts:899 |
rejected (same root cause) |
| Fullstack QA (all-green) | built-in-workflows.ts:1078-1080 |
rejected — comment |
| End-node REC (approve + submit) | task-agent-manager.ts:3065 / :3069 |
rejected — comment |
The end-node REC site is the round-2 "fix" I approved — it has summary only and no data at all, so it fails validation too. That approval was my miss: I checked the two fix-sites in isolation but did not verify they satisfy the per-shape validator, nor sweep the other migrated decision sites.
The review auto-save (coding-artifact-profile.ts:185, recommendation:'reviewed') is correct and unaffected.
Fix: add data.recommendation to each outcome decision (e.g. 'dispatched', 'pass', 'completed'). Adding recommendation does not set a kind, so summarizeRunOutcome still treats them as the terminal outcome. (Worth confirming with the human whether decision is even the right shape for a generic outcome summary vs. a verdict — but the minimal unblocking fix is the recommendation field.)
P2 — merge-conflict notes overwrite each other (auditable loop broken)
deriveArtifactKey('note', …) always returns 'current' (shared/src/artifact-shapes.ts:230; ignores kind/attempt), so each merge-conflict retry upserts the same row and erases the prior attempt's approved_head_oid/conflicting_files. Regression from prior append semantics — comment.
P2 (for assessment) — resolvePrimaryLinkUrl prefers stale gate data over a newer link kind:'pr'
coding-artifact-profile.ts:71 returns a gate-data pr_url immediately (step 1) without comparing updatedAt against a newer canonical link kind:'pr' artifact (step 3). dispatchPostApproval resolves {{pr_url}} through this, so a stale handoff URL could in principle target the wrong PR. Please compare against the pre-consolidation resolvePrUrlForRun ordering — if the originals preferred artifacts, this is a behavior change; either way, selecting the freshest source by updatedAt across gate/hook/artifact would be safer.
Disposition of the other two Codex comments
- list_artifacts legacy filter (node-agent-tools.ts:1528) — not a bug. The describe no longer advertises legacy
pr/resultmapping (the round-2 fix removed it), and dropping the legacy filter is intended per scope item 3. - gate hook uses raw send payload (node-agent-tools.ts:1046) — low/theoretical. For
review-posted-gate,review_urlis a declared authorized field, somessageData.review_urlreflects a legitimately committed write. The round-1 fix stands; tightening to the committed subset is optional defense-in-depth.
Why CI is green
No test exercises the prompt-string → save_artifact → validateArtifactShape path, so the P1s aren't caught. A regression test that runs one of these prompt payloads through the real handler (asserting success:true) would prevent recurrence.
The core refactor (profile seam, consolidation, prompt migration) remains sound; these are integration defects in the migrated prompt payloads + one key-derivation gap.
|
To use Codex here, create an environment for this repo. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 551b711fff
ℹ️ 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".
…act persists Review P1 on #2345: validateArtifactShape('decision') requires data.recommendation, so every migrated outcome `decision` that omitted it was silently rejected by save_artifact — the terminal outcome was never recorded before approve_task / submit_for_approval. Add recommendation to the four live sites: - built-in-workflows dispatcher (stack + 2nd mention): recommendation 'dispatched' - built-in-workflows Fullstack QA all-green: recommendation 'pass' - task-agent-manager end-node Runtime Execution Contract (both autonomy branches): recommendation 'completed' Adding recommendation does not set a kind, so summarizeRunOutcome still treats these as the terminal outcome.
…overwrite Review P2 on #2345: deriveArtifactKey('note', …) always keyed 'current' and ignored `key`, so each merge-conflict retry from the post-approval node overwrote the prior attempt's approved_head_oid / conflicting_files — a regression from the dropped append semantics, and the loop was no longer auditable. Extend deriveArtifactKey so `note` honors an explicit key (namespaced by kind, like `decision` does), while a note WITHOUT an explicit key stays the single rolling 'current' row. The merge-conflict prompt now passes key: "attempt-<N>", so each attempt persists as a distinct row. SaveArtifactSchema docs updated to describe the multi-instance note case.
…tted fields Two related review/Codex findings on #2345: - resolvePrimaryLinkUrl returned a gate-data pr_url immediately (sequential priority) instead of comparing freshness across sources. dispatchPostApproval resolves {{pr_url}} through it, so a stale handoff URL could launch the merge session against the wrong PR when a newer link kind:'pr' artifact existed. Rewrite to gather candidates from gate data, hook state, and artifacts, and return the one with the greatest updatedAt. - onGateDataCommitted read review_url from the raw send payload (messageData), so a field the agent sent but was not authorized to write could still trigger a review decision. Pass the committed gate-field subset (committedData) and read review_url from it; comment_urls (non-gate metadata) still travels via messageData. Equivalent for review-posted-gate (review_url is an authorized field), strictly more correct for the general case.
…ifact payloads Review test-gap on #2345: CI stayed green while the outcome `decision` sites omitted data.recommendation because nothing exercised the path from prompt text to the real save_artifact validator. Add a test that runs the exact payloads emitted by the migrated coding-workflow prompts (dispatcher decision, QA PR link + decision, end-node REC decision, per-attempt merge-conflict notes, post-merge audit link) through the real handler, asserting success:true and that two conflict attempts persist as distinct rows.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d679b5469
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (NeoKai)
Model: glm-5.1[1m] | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES (1 CI-blocking P1, 1 P2).
Round-3 fixes — all verified ✓
Every finding from round 3 is correctly resolved on 7d679b546:
- P1 (4 sites): dispatcher ×2, QA all-green, and end-node REC now carry
data.recommendation(dispatched/pass/completed). - P2 #4:
deriveArtifactKeynow honors explicit keys for notes (namespaced by kind); merge-conflict notes usekey:"attempt-<N>". - P2 #3:
resolvePrimaryLinkUrlpicks the freshest URL across gate/hook/artifact byupdatedAt. - #6: gate hook passes
committedData(authorized subset);onGateDataCommittedreadscommittedData.review_url. - Test gap: the new prompt→validator parity test runs the exact migrated payloads through the real handler — exactly the guard I asked for. 169 daemon + 26 shared tests pass locally.
P1 — CI typecheck is RED on this PR (blocks merge)
Lint, Knip, Format & Type Check failed on 7d679b546 (All Tests Pass is just the aggregate of that). The actual errors:
rpc-handlers/index.ts(750,5): TS2739 — bun:sqlite.Database not assignable to sqlite-node.Database (missing inTransaction, filename, handle, serialize, fileControl)
coding-artifact-profile.ts(80,78): TS2345 — bun:sqlite.Database not assignable to sqlite-node.Database
coding-artifact-profile.ts(93,61): TS2345 — (same)
What I verified:
- dev is green on CI for this same check (most recent dev run:
✓ Lint, Knip, Format & Type Check; dev's failure there is an unrelated "CLI Tests" exit-127). So this is not a pre-existing infra issue — it surfaces only on this branch. - Local typecheck passes (
tsc --build --noEmit --force, tsbuildinfo deleted; rootbun run typecheckalso clean). Thesqlite-nodepath in the CI message is how CI's Bun runtime types resolvebun:sqlite(no such module exists in the repo) — i.e. a Bun-version type-surface drift between local and CI. - The profile uses the same
import type { Database as BunDatabase } from 'bun:sqlite'+new GateDataRepository(this.db)pattern as the ~8 other passing callers; the new element is storingdeps.db.getDatabase()in a typeddb: BunDatabasefield and wiring it atrpc-handlers/index.ts:750.
Runtime behavior is fine (the repos are built for bun:sqlite), but CI's tsc gate is strict and is the contract — the PR can't merge red. This needs investigation with CI iteration (I can't reproduce locally). Likely a type-annotation adjustment in CodingArtifactProfile/its config so the assignment is compatible with CI's Bun types.
P2 — QA failure note still overwrites across cycles
built-in-workflows.ts:1070 records QA failures as note kind:'qa' with no explicit key → keys to 'current' → each failure cycle erases the prior repro evidence. Same class as the merge-conflict note (which was fixed), missed here. Add key:"cycle-<N>" and cover it in the parity test — comment. (Codex-flagged; verified.)
Net
The core refactor and the round-3 fixes are sound and well-tested. Two items remain: unblock CI's typecheck (P1) and key the QA-failure note (P2). Requesting changes.
Review P2 on #2345: the Fullstack QA failure note used `shape: "note", kind: "qa"` with no key, so it keyed 'current' and each failure cycle erased the prior cycle's repro evidence — the same overwrite class as the merge-conflict note fixed last round. Add `key: "cycle-<N>"` (<N> = QA round, 1-based). Extend the prompt→validator parity test to cover two QA failure cycles persisting as distinct rows alongside the merge-conflict attempts.
|
Round-4 REQUEST_CHANGES resolved (head d2d3a50):
All 13 open review threads (round-3 + round-4, Codex + reviewer) are replied to and resolved. Re-requesting review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2d3a50edb
ℹ️ 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".
…ifact payloads Review test-gap on #2345: CI stayed green while the outcome `decision` sites omitted data.recommendation because nothing exercised the path from prompt text to the real save_artifact validator. Add a test that runs the exact payloads emitted by the migrated coding-workflow prompts (dispatcher decision, QA PR link + decision, end-node REC decision, per-attempt merge-conflict notes, post-merge audit link) through the real handler, asserting success:true and that two conflict attempts persist as distinct rows.
Review P2 on #2345: the Fullstack QA failure note used `shape: "note", kind: "qa"` with no key, so it keyed 'current' and each failure cycle erased the prior cycle's repro evidence — the same overwrite class as the merge-conflict note fixed last round. Add `key: "cycle-<N>"` (<N> = QA round, 1-based). Extend the prompt→validator parity test to cover two QA failure cycles persisting as distinct rows alongside the merge-conflict attempts.
…n upgrade Review P1 on #2345: dev shipped seeded built-in prompts on the legacy save_artifact({ type: "result", append: true, ... }) API. This PR rewrote them to shape:, but the restamp registry gained no type->shape entry, and the cutover rewrote whole call sites (even expanding some one-call steps into two) that are fragile as substring pairs. So on upgrade, existing seeded spaces kept directing agents to call save_artifact with type:, which the new schema rejects for missing shape — terminal artifacts never persisted and post-approval merge could not resolve the PR. Swap any persisted built-in prompt still using the legacy freeform-type API to the current template during restamp (such a prompt is broken regardless, so restamping is the correct fix). Add a regression test that seeds a stale type:"result" prompt and asserts it swaps to shape: on merge.
…he slot terminal Review P2 on #2345: the end-node Runtime Execution Contract (injected into every end-node session) wrote an unkeyed decision (recommendation "completed") -> key 'current'. The Fullstack QA end node's slot prompt also writes an unkeyed terminal decision (recommendation "pass" + test_output/ui_changed/browser_validation) -> key 'current'. An agent following both overwrites the QA evidence with the generic "completed" right before approval; the Plan & Decompose dispatcher collides the same way. Give the REC decision a distinct key ("outcome") so the two coexist as separate rows. Updated the prompt-parity test to match.
Review P2 on #2345: the post-approval merge template records multiple cleanup_warning notes (branch-delete failure, space-checkout not on $BASE, space $BASE ahead, pull failure) as unkeyed notes -> all key 'current', so each overwrites the prior and loses part of the cleanup audit. Give each a distinct explicit key (branch-delete / space-checkout-base / space-checkout-ahead / space-checkout-pull). Same class as the merge_conflict:attempt-N and qa:cycle-N key fixes already in this PR.
Codex P1 on #2345: the round-6 unconditional swap (any prompt containing save_artifact({ type: ... })) wholesale-replaced persisted prompts with the template, silently discarding operator customizations. Revert that broad check and instead add type→shape patch-variant pairs to BUILT_IN_PROMPT_PATCH_VARIANTS for each rewritten call site (PR link, review-only link, dispatcher stack/short, QA failed note, and the expanded QA all-green two-call step). Only an EXACT retired variant swaps to the current template, so customizations are preserved. Add a regression test for the expanded QA all-green region (the most fragile pair), including a toContain sanity check that the current-text transcription matches the live template.
… PR-link call Codex P1 on #2345: the type→shape cutover rewrote BOTH the "Use save_artifact every cycle…" sentence (Nest pr_url → record the PR as a link) AND the PR-link call in the Coding and Research reviewer prompts. Reversing the call alone leaves the new sentence, so the generated retired variant did not match the real dev-era prompt and the upgrade left these two workflows on the rejected legacy type:"result" API. Add a patch-variant set that reverses the sentence and the call together, and a regression test that swaps both and asserts the restamp.
|
Rebased onto current Why: CI For the reviewer: all commit SHAs changed, but the round 1–6 changes are preserved 1:1 (same 16 commits, same content) — no need to redo rounds 1–6; please re-base on the new diff and verify the round-6 findings still hold. Round-6 fixes (data.recommendation ×4, note keying, freshest-PR resolver, committed gate fields, REC key 'outcome', exact type→shape restamp pairs) all survived the rebase. Also resolved the latest Codex P1 (Coding/Research reviewer preceding-sentence + PR-link call now reverse together). Local: full daemon suite green (12854/12855; the 1 miss is a flaky |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (z.ai)
Model: glm-5.1 | Client: HyperNeo | Provider: z.ai (Zhipu)
Recommendation: APPROVE — posted as COMMENT because the reviewer shares the PR author's GitHub identity, so GitHub rejects a literal APPROVE from the author.
Re-based the review on the new head 1a0326492 (rebase onto current dev, incl. #2365 Reviewer/Merger split). All prior findings resolved; round-6 fixes preserved 1:1; the one outstanding P1 is fixed.
Verified at 1a0326492
Codex P1 — Coding/Research preceding-sentence restamp: FIXED. The PR rewrote both the sentence preceding the PR-link call and the call itself in the Coding and Research reviewer prompts (dev:532/708 Use save_artifact every cycle. Nest pr_url inside artifact data… + type:"result" → head Use save_artifact every cycle to record the PR as a 'link'… + shape:"link"). Because isExactRetiredBuiltInPrompt requires whole-prompt exact equality, reverse-applying only the call pair (SHAPE_PR_LINK) would yield record the PR as a link… + type:result, which can't match a dev-seeded prompt — so spaces seeded on dev would keep emitting type:"result" and be silently rejected after the compat-shim drop. The new SHAPE_PR_EVERY_CYCLE/RETIRED_TYPE_RESULT_EVERY_CYCLE pair (built-in-workflows.ts:1757-1760), wired into BUILT_IN_PROMPT_PATCH_VARIANTS alongside the call pair, reconstructs the full dev-era region. Confirmed the dev text matches the retired constant verbatim against origin/dev.
Architecture criterion (#796/#809): met. Coding-specific logic (resolvePrimaryLinkUrl, summarizeRunOutcome, onGateDataCommitted) lives only in coding-artifact-profile.ts (the seam). Generic node-agent-tools.ts has zero coding kinds; findLinkUrl is a generic *_url/*Url match (no hardcoded pr). The case 'pr'/'review' in resolveLegacyShape is pre-existing (#2313, already on dev) and untouched by this PR.
Round-6 fixes preserved 1:1. All 8 restamp constants present; end-node REC key:"outcome" (×2) so it no longer clobbers the slot terminal decision; 4 distinct cleanup_warning keys; post-approval-router type:"blocked" → shape:"note", kind:"blocked".
No live legacy type:"result" calls. Every remaining save_artifact({ type:"result" …}) reference in built-in-workflows.ts (incl. the validation-step text at :1665) is a RETIRED_* restamp matcher, not an active prompt instruction. The compat shim can be dropped safely.
Tests (run independently, not just trusting the green claim): shared artifact-shapes 26/0; migration shard 104/0 across 20 files (the shard the rebase was meant to fix); restamp regression in built-in-workflows.test.ts 305/0.
PR hygiene: head 1a0326492 == origin branch tip == worktree HEAD (no unreviewed local-only commits); state OPEN, MERGEABLE; 0 unresolved review threads.
Notes (non-blocking)
- The rebase pulled in #2365's Reviewer/Merger split (Post-Approval node +
mergertargetAgent). The type→shape migration is genuinely orthogonal —resolvePrimaryLinkUrlresolves the PR link across gate/hook/artifact regardless of whether reviewer or merger runs post-approval. - The 1 known daemon-suite miss (space-worktree-manager timeout) is outside this PR's surface and reported 23/23 on rerun; I did not re-run the full suite, only the directly-affected shards above.
No P0–P3 findings. Approving.
…on core Introduce a WorkflowArtifactProfile domain seam and a CodingArtifactProfile implementation in the coding-workflow layer, then move every coding-specific writer/reader out of daemon core behind it. Daemon core (node-agent-tools, space-runtime, space-runtime-service, task-agent-manager, evolution-episode-service) now references no domain kinds (`pr`, `review`) or coding identifiers (`review-posted-gate`, `pr_url`) — only the profile does. Relocated: - The 5 duplicated `resolvePrUrlForRun` copies → profile.resolvePrimaryLinkUrl (gate data → hook state → artifacts, preferring link kind:'pr'). - The review-posted-gate auto-save (decision kind:'review' round-N) → profile.onGateDataCommitted, fired as a generic gate-write hook. - The 3 kindless-decision result-summary readers → profile.summarizeRunOutcome. Migrate every coding-workflow agent prompt (built-in-workflows, post-approval-merge-template) off the legacy `type`/`append` API onto explicit shapes: PR → link kind:'pr', planner/QA outcome → decision, audit notes (merge_conflict/merge_blocked/cleanup_warning) → note with kind, merge audit → link kind:'merge'. Drop the legacy type/append compat shim from SaveArtifactSchema and the list_artifacts legacy-type filter mapping now that no caller emits them. Update live-query timeline tone classification to read `kind` (new) as well as `_legacyType` (backfilled) for blocker/warning notes. Task #809 (#796 PR C).
Update tests for the relocated artifact logic: - node-agent-tools: drop the legacy type-shim tests (shim removed); add a "shape required" guard test; wire CodingArtifactProfile into the test ctx so review-posted-gate history and PR-URL resolution assert against the profile. - evolution-episode-service: wire the profile so result-artifact gap detection reads outcomes through it. - space-runtime / -service / -rehydration / event-driven-gate-eval / external-event-delivery-e2e / post-approval-routing: inject the profile where the tests assert primary-link (PR URL) resolution or outcome summaries. - built-in-workflows + end-node-handoff: update prompt-content assertions for the migrated shape API (link kind:'pr', note kind:'merge_*'). Task #809 (#796 PR C).
…link Address review feedback on #2345: - CodingArtifactProfile.onGateDataCommitted now reads review_url from the current send_message payload (messageData) instead of the merged gate state. The prior form would spuriously record an extra review round when a later send updated only comment_urls while a previous round's review_url lingered in the gate state. Adds a regression test for the follow-up-send case. - Fullstack QA success now records the PR as a first-class `link kind:'pr'` (consistent with the other reviewer nodes and robust to legacy-field removal) alongside the terminal `decision` outcome, instead of burying pr_url inside the decision data. - Rebase onto dev: drop the list_artifacts legacy kind-filter and its tests added in #2319 (they re-introduced `pr`/`review` kind names into core, which PR C removes); list_artifacts now filters by shape only.
The live-query warning-tone filter added `merge_conflict` to the new-kind path but the legacy `_legacyType` backcompat list was missing `merge_conflict_loop`, so old backfilled conflict rows stayed 'progress' while new shape rows became 'warning'. Add the legacy type name so both render the same tone.
Address review REQUEST_CHANGES on #2345 — two prompt sites the legacy type/append shim was still propping up: - task-agent-manager end-node Runtime Execution Contract (injected into every end-node session) still emitted save_artifact({ type: "result", append: true, summary }) — now rejected with the shim gone, so the terminal outcome was never recorded. Use shape: "decision". - ListArtifactsSchema.type .describe() advertised legacy 'progress'/'result'/ 'review', which now silently return empty. Update to the shape vocabulary.
…act persists Review P1 on #2345: validateArtifactShape('decision') requires data.recommendation, so every migrated outcome `decision` that omitted it was silently rejected by save_artifact — the terminal outcome was never recorded before approve_task / submit_for_approval. Add recommendation to the four live sites: - built-in-workflows dispatcher (stack + 2nd mention): recommendation 'dispatched' - built-in-workflows Fullstack QA all-green: recommendation 'pass' - task-agent-manager end-node Runtime Execution Contract (both autonomy branches): recommendation 'completed' Adding recommendation does not set a kind, so summarizeRunOutcome still treats these as the terminal outcome.
…overwrite Review P2 on #2345: deriveArtifactKey('note', …) always keyed 'current' and ignored `key`, so each merge-conflict retry from the post-approval node overwrote the prior attempt's approved_head_oid / conflicting_files — a regression from the dropped append semantics, and the loop was no longer auditable. Extend deriveArtifactKey so `note` honors an explicit key (namespaced by kind, like `decision` does), while a note WITHOUT an explicit key stays the single rolling 'current' row. The merge-conflict prompt now passes key: "attempt-<N>", so each attempt persists as a distinct row. SaveArtifactSchema docs updated to describe the multi-instance note case.
…tted fields Two related review/Codex findings on #2345: - resolvePrimaryLinkUrl returned a gate-data pr_url immediately (sequential priority) instead of comparing freshness across sources. dispatchPostApproval resolves {{pr_url}} through it, so a stale handoff URL could launch the merge session against the wrong PR when a newer link kind:'pr' artifact existed. Rewrite to gather candidates from gate data, hook state, and artifacts, and return the one with the greatest updatedAt. - onGateDataCommitted read review_url from the raw send payload (messageData), so a field the agent sent but was not authorized to write could still trigger a review decision. Pass the committed gate-field subset (committedData) and read review_url from it; comment_urls (non-gate metadata) still travels via messageData. Equivalent for review-posted-gate (review_url is an authorized field), strictly more correct for the general case.
…ifact payloads Review test-gap on #2345: CI stayed green while the outcome `decision` sites omitted data.recommendation because nothing exercised the path from prompt text to the real save_artifact validator. Add a test that runs the exact payloads emitted by the migrated coding-workflow prompts (dispatcher decision, QA PR link + decision, end-node REC decision, per-attempt merge-conflict notes, post-merge audit link) through the real handler, asserting success:true and that two conflict attempts persist as distinct rows.
CI typecheck (round 4) flagged CodingArtifactProfile's `db: BunDatabase` field as bun:sqlite.Database not assignable to the sqlite-node-flavored Database the repositories expect — a Bun type-resolution drift that only surfaces in CI, not locally. Rather than import `Database` from `bun:sqlite` directly, derive the field type from the repository constructor (`ConstructorParameters<typeof GateDataRepository>[0]`) so it is structurally identical to what every other repo caller passes and what `getDatabase()` returns under any Bun resolution — the same type the passing `new GateDataRepository(deps.db.getDatabase())` wiring already uses.
Review P2 on #2345: the Fullstack QA failure note used `shape: "note", kind: "qa"` with no key, so it keyed 'current' and each failure cycle erased the prior cycle's repro evidence — the same overwrite class as the merge-conflict note fixed last round. Add `key: "cycle-<N>"` (<N> = QA round, 1-based). Extend the prompt→validator parity test to cover two QA failure cycles persisting as distinct rows alongside the merge-conflict attempts.
…n upgrade Review P1 on #2345: dev shipped seeded built-in prompts on the legacy save_artifact({ type: "result", append: true, ... }) API. This PR rewrote them to shape:, but the restamp registry gained no type->shape entry, and the cutover rewrote whole call sites (even expanding some one-call steps into two) that are fragile as substring pairs. So on upgrade, existing seeded spaces kept directing agents to call save_artifact with type:, which the new schema rejects for missing shape — terminal artifacts never persisted and post-approval merge could not resolve the PR. Swap any persisted built-in prompt still using the legacy freeform-type API to the current template during restamp (such a prompt is broken regardless, so restamping is the correct fix). Add a regression test that seeds a stale type:"result" prompt and asserts it swaps to shape: on merge.
…he slot terminal Review P2 on #2345: the end-node Runtime Execution Contract (injected into every end-node session) wrote an unkeyed decision (recommendation "completed") -> key 'current'. The Fullstack QA end node's slot prompt also writes an unkeyed terminal decision (recommendation "pass" + test_output/ui_changed/browser_validation) -> key 'current'. An agent following both overwrites the QA evidence with the generic "completed" right before approval; the Plan & Decompose dispatcher collides the same way. Give the REC decision a distinct key ("outcome") so the two coexist as separate rows. Updated the prompt-parity test to match.
Codex P1 on #2345: the round-6 unconditional swap (any prompt containing save_artifact({ type: ... })) wholesale-replaced persisted prompts with the template, silently discarding operator customizations. Revert that broad check and instead add type→shape patch-variant pairs to BUILT_IN_PROMPT_PATCH_VARIANTS for each rewritten call site (PR link, review-only link, dispatcher stack/short, QA failed note, and the expanded QA all-green two-call step). Only an EXACT retired variant swaps to the current template, so customizations are preserved. Add a regression test for the expanded QA all-green region (the most fragile pair), including a toContain sanity check that the current-text transcription matches the live template.
… PR-link call Codex P1 on #2345: the type→shape cutover rewrote BOTH the "Use save_artifact every cycle…" sentence (Nest pr_url → record the PR as a link) AND the PR-link call in the Coding and Research reviewer prompts. Reversing the call alone leaves the new sentence, so the generated retired variant did not match the real dev-era prompt and the upgrade left these two workflows on the rejected legacy type:"result" API. Add a patch-variant set that reverses the sentence and the call together, and a regression test that swaps both and asserts the restamp.
…rebase After rebasing onto dev (#2362 added a terminal-validator-gate test that calls evaluateTerminalGateFeatures with ctx.artifactRepo as the 7th arg), the call no longer matched my refactored signature (7th param is artifactProfile, not the repo). Pass ctx.artifactProfile like the sibling calls in this file.
1a03264 to
ea269be
Compare
There was a problem hiding this comment.
💡 Codex Review
HyperNeo/packages/daemon/src/lib/space/tools/node-agent-tools.ts
Lines 2142 to 2144 in ea269be
Remove the legacy-filter compatibility claim
Fresh evidence after the thread's stated fix is that the reviewed tree still advertises legacy progress/result/review/pr filter mapping here, while list_artifacts now passes args.type directly to artifact_type. An agent following this tool description—for example, calling list_artifacts({ type: "pr" })—therefore receives an empty result despite existing link artifacts, so either remove the compatibility claim or restore the mapping.
ℹ️ 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".
…#868] dev advanced through several PRs (#2345–#2408) while this branch was in review, shipping migrations 174–178 and the complementary post-approval source-node work (#851). Conflicts were all additive (dev's postApprovalSourceNodeId / artifactProfile alongside this branch's postApproval completion fields), resolved by keeping both sides. Migration renumber: dev took 174/175/176, so this branch's post-approval migrations move to 179/180/181 to avoid marker-key collisions (a dev DB that ran dev's migration_174 would otherwise skip these columns): m174 → m179 (post-approval completion columns) m175 → m180 (post-approval route target) m176 → m181 (approved-task sweep covering index) plus their tests and the migrations.ts registration (after dev's 178).
1 participant
Follow-up to #796 PR A (#2313) and PR B (#2314). Introduces a
WorkflowArtifactProfiledomain seam with aCodingArtifactProfileimplementation, and moves every coding-specific artifact writer/reader out of daemon core behind it so core references no domain kinds (pr,review,review-posted-gate,pr_url).resolvePrUrlForRuncopies intoprofile.resolvePrimaryLinkUrl, the review-posted-gate auto-save intoprofile.onGateDataCommitted, and the 3 kindless-decision result-summary readers intoprofile.summarizeRunOutcome.type/appendto explicit shapes (PR →link kind:'pr', outcome →decision, audit notes →notewith kind, merge audit →link kind:'merge'), then drops the legacy type/append compat shim fromSaveArtifactSchemaand thelist_artifactslegacy filter.kind(new) in addition to_legacyType(backfilled rows).Notes for review: the result-summary readers were already kind-name-free (kindless-decision convention); I relocated them into the profile anyway to remove the 3× duplication and keep the outcome convention in the coding layer.
live-query-handlers.tsstill referenceskind:'pr'/kind:'review'for display classification (tone/title); that's daemon-side rendering, in PR B's scope.Known CI flake (not this PR):
space-worktree-manager > cleanupOrphanedtimes out under parallel load but passes 23/23 on rerun — last touched by #2364, unrelated to artifact shapes. Otherwise the full daemon suite is green (12854/12855).