Repository navigation
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
❌ PR Specification Reviewer failed to deliver outputs. Please review the logs for details.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head 607e099 against the Unified Agent Session Specification (2.7.0 at review start), especially T-UAS-005, T-UAS-007, T-UAS-040, and T-UAS-056. The implementation and tests align with the intentional Pi timestamp and tool-correlation behavior, preserving source clocks, messageId/model, archival envelopes, child scope, and accounting semantics.
One specification synchronization gap was identified: the PR added normative behavior without updating document version/publication metadata or the existing change log. A focused update to unified-agent-session-specification.md is queued on the same PR branch: version 2.8.0, publication date 2026-11-09, and a concise Section 13 entry. No implementation or test changes were made.
No other actionable contract violations were found. Coverage was limited to static review of the captured diff and repository specifications; tests were not run by this review.
Generated by PR Specification Reviewer for #76103 · copilot · gpt50 · ~5 AIC · ⌖ ~2 AIC · ⊞ 1.7K
Comment /review-specs to run again
There was a problem hiding this comment.
🟡 Changes recommended
Two timestamp-handling issues can leave tool completions untimed or override existing source clocks, affecting unified publication, provenance, and cross-source ordering.
2 open issues
What changed in this PR
This PR improves session fidelity in gh-aw by preserving Pi message timestamps and retaining tool-to-message correlation in unified session artifacts, while keeping native envelopes archival.
Changes:
- Promote observed Pi message clocks and recover timestamps from older canonical metadata.
- Preserve supplied
messageIdandmodelacross unified tool lifecycle events. - Update types, schema, documentation, and regression coverage.
| File | Description |
|---|---|
docs/src/content/docs/specs/unified-agent-session-specification.md |
Specifies clock precedence and tool correlation retention. |
docs/src/content/docs/reference/engines/pi.md |
Documents Pi session artifacts and retained metadata. |
docs/public/schemas/unified-session.schema.json |
Allows message IDs and models on tool events. |
actions/setup/js/unified_session_payload.test.cjs |
Tests correlation retention and envelope omission. |
actions/setup/js/unified_session_payload.cjs |
Projects tool correlation fields and recovers message clocks. |
actions/setup/js/unified_session_order.test.cjs |
Tests recovered clocks, ordering, and provenance. |
actions/setup/js/unified_session_order.cjs |
Recognizes timestamps in saved message metadata. |
actions/setup/js/types/unified_session.d.ts |
Extends unified tool event types. |
actions/setup/js/types/agent_session.type-test.ts |
Checks tool correlation types and envelope exclusion. |
actions/setup/js/pi_session.test.cjs |
Adds timestamp reconciliation and child-scope regressions. |
actions/setup/js/pi_session.cjs |
Retains message timestamps during canonical conversion. |
actions/setup/js/pi_session_pipeline.test.cjs |
Tests publication from current and older canonical artifacts. |
Review effort: Balanced (auto)
|
|
||
| /** @param {SessionEvent} event @param {any} message */ | ||
| function retainMessageTimestamp(event, message) { | ||
| if (event.timestamp === undefined && (typeof message?.timestamp === "string" || typeof message?.timestamp === "number")) event.timestamp = message.timestamp; |
| retainMessageTimestamp( | ||
| emit(raw, "tool.execution_complete", { |
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
I found two correctness issues that should be fixed before merge:
- Legacy mapping can discard recovered message clocks because timestamp retention mutates the pre-mapping event.
- Archived metadata timestamps are promoted without validation, allowing invalid canonical clocks and unstable ordering.
Inline comments include the affected paths and suggested regressions.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~7 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| /** @type {any} */ | ||
| const nativeMessage = event.message; | ||
| if (!Object.hasOwn(metadata, "timestamp") && nativeMessage?.timestamp !== undefined) metadata.timestamp = structuredClone(nativeMessage.timestamp); | ||
| const nativeTimestamp = (nativeMessage && typeof nativeMessage === "object" && "timestamp" in nativeMessage ? nativeMessage.timestamp : undefined) ?? source.messageMetadata?.timestamp; |
There was a problem hiding this comment.
This promotes any string/number from archived messageMetadata.timestamp directly to canonical timestamp, including values such as "yesterday" or timezone-less ISO strings. sessionTimestampNs() then rejects the value, so the event publishes an invalid clock and loses its ordering key, contrary to the spec's timezone-qualified timestamp requirement. Validate with the same parser/range rules before promotion and add invalid-clock coverage.
| output: message.content, | ||
| success: message.isError === true || isPiResultError(message) ? false : message.isError === false ? true : undefined, | ||
| }); | ||
| retainMessageTimestamp( |
There was a problem hiding this comment.
emit() returns the pre-mapping event, while it pushes mapLegacyEvent(event) when processing legacy observations. This mutation therefore updates an object that may not be present in events; a cloning mapper (the normal session CLI path) drops the recovered clock for tool completions, snapshots, and errors. Return the mapped event (or apply the timestamp before mapping), and add a cloning-mapper regression covering these event types.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design. Requesting changes because the promoted messageMetadata clock is validated more loosely than the ordering key.
📋 Key Themes & Highlights
Key Themes
- Validation mismatch:
normalizeUnifiedSessionEventpromotesmessageMetadata.timestampwith only a type check, whilesessionTimestampNsrejects no-TZ ISO strings, impossible dates, and out-of-range numbers. Those values get projected but cannot be ordered. - Test gap: nothing asserts the behavior for invalid promoted clocks.
- Spec drift: the spec says ISO timestamps MUST carry a timezone, which the projection path does not enforce.
Positive Highlights
- ✅ The
emitreturn path correctly carries the clock throughretainMessageTimestamp. - ✅ Source-event clocks take precedence over
messageMetadata, matching the spec. - ✅ Child scope paths are unaffected by the legacy mapper.
Limits
- Schema acceptance of invalid clocks was not verified;
ajvis not installed in this environment. - The test suite was not run; the PR reports 442 focused tests passing.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~7 AIC · ⌖ ~2 AIC · ⊞ 6.3K
Comment /matt to run again
| const nativeMessage = event.message; | ||
| if (!Object.hasOwn(metadata, "timestamp") && nativeMessage?.timestamp !== undefined) metadata.timestamp = structuredClone(nativeMessage.timestamp); | ||
| const nativeTimestamp = (nativeMessage && typeof nativeMessage === "object" && "timestamp" in nativeMessage ? nativeMessage.timestamp : undefined) ?? source.messageMetadata?.timestamp; | ||
| if (!Object.hasOwn(metadata, "timestamp") && (typeof nativeTimestamp === "string" || typeof nativeTimestamp === "number")) metadata.timestamp = nativeTimestamp; |
There was a problem hiding this comment.
[/diagnosing-bugs] Promoted messageMetadata.timestamp is only type-checked here, but sessionTimestampNs (unified_session_order.cjs:13) validates the same value strictly. Malformed clocks such as a no-TZ ISO string, an impossible date like 2026-02-30, or a huge number get projected onto timestamp yet are unorderable, so they silently fall back to a different order.
💡 Suggested fix
Extract the validation from sessionTimestampNs into a shared helper and call it from both places, or promote the clock only when that helper accepts it. The projection and the ordering key then agree on what a valid clock is.
@copilot please address this.
| import { createSessionLookups } from "./grader_session.cjs"; | ||
|
|
||
| describe("essential unified session payloads", () => { | ||
| it.each(["tool.execution_start", "tool.execution_update", "tool.execution_complete"])("retains tool/message correlation without publishing native envelopes (%s)", type => { |
There was a problem hiding this comment.
[/tdd] This test covers correlation and redaction but not the promotion path. Add cases for invalid promoted clocks: a no-TZ ISO string, an impossible date, a huge number, and an object value. Assert each one is either rejected or consistent with sessionTimestamp.
💡 Suggested tests
Write these as red tests first, then choose the behavior (reject or accept consistently) and make the code match. Edge clocks like these are the usual source of ordering regressions.
@copilot please address this.
| @@ -369,7 +369,12 @@ timestamp values and MUST derive its ordering key using the source schema's | |||
| units, not an epoch-magnitude heuristic. ISO timestamps MUST carry a timezone. | |||
There was a problem hiding this comment.
[/codebase-design] The spec says ISO timestamps MUST carry a timezone, but the projection path promotes messageMetadata clocks without enforcing that rule. Either tighten the projection to match this rule (see the comment on unified_session_payload.cjs) or reword the spec so it says the ordering key enforces it. Right now the spec and the code disagree.
@copilot please address this.
Summary
Fix two fidelity bugs confirmed in existing Pi Actions artifacts:
data.messageMetadata.timestampwhen publishing older canonical artifacts, preserving the clock in unifiedtimestampand provenance before omitting archival metadata.messageId/message_idandmodelon generic unified tool start/update/complete records. Update strict types, generated schema, Pi reference and unified-session specification.Full native
messageMetadata, provider envelopes and signatures remain intentionally archival; this PR does not publish those envelopes or invent timestamps/accounting observations. Message payloads, inputs/results/errors, usage and child invocation scope are preserved.Existing live evidence and local replay
4cec482315dd38838403ac3fa83462cc5c669544547a7345a84c66fcf915bd0a5f999f189f68cba9Both runs use repository-local setup with
GH_AW_VERSION: dev; a numeric compiler version is unavailable, so the exact revisions above identify the run compiler. The local collection CLI version is separate from those runtime/compiler identities.Native, agent and unified JSONL from both existing runs were collected privately and compared against original uploaded artifacts. Encoding, escaping, nested payloads, schema validation, provenance, source order, tool outcomes, parent usage and observed child scope were checked. The recovery counts are local replay of captured live artifacts, not newly executed Actions runs. Raw captured content is not committed; regressions use sanitized live-shaped records.
Live coverage is unavailable for durable/resume/compaction, interrupted streams, child failures/tool/reasoning streaming and a separate routed workflow. Existing child final-envelope forwarding is intentional and unchanged. No workflow dispatch, new inference, MCP startup or credential changes were performed.
Cross-PR dependency
Cursor's separate adapter PR will map native
model_call_idto toolmessageIdand assistantapiCallId. This PR's generic tool projection is required for Cursor's toolmessageIdto survive unified publication after both PRs integrate. Neither PR depends on the other's code to pass its own regressions; this PR contains no Cursor-specific adapter changes and uses engine-independent tool correlation tests.Validation
make fmt-cjs && make lint-cjspassed.make agent-report-progresspassed, including changed-file lint/type checking, 295 impacted JavaScript tests and schema freshness. The impacted Go gate correctly skipped because no Go files changed; workflow recompilation correctly skipped because no workflow/compiler source changed.git diff --checkpassed; validated commit is607e0993a7d.