[#870] add-tags-persona-to-phase-even#879
Closed
nrslib wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughスパン属性 Changestags/persona をフェーズ使用イベントと CSV に追加
推定コードレビュー工数🎯 3 (Moderate) | ⏱️ ~20 minutes 関連する可能性のある PR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/__tests__/phaseUsageEvent.test.ts`:
- Around line 283-362: The test coverage for takt.step.tags validation is
incomplete and missing boundary cases for null and object shapes. Add two
additional test cases following the same pattern as the existing tests: one test
where takt.step.tags is set to null and another where it is set to an object
shape (instead of an array), and verify in both cases that the
mapSpanEndToPhaseUsageEvent function ignores these malformed values by asserting
the record does not have a tags property. This ensures the parser properly
handles unexpected value types at the config/parse boundary.
In `@src/__tests__/tokenUsageCsv.test.ts`:
- Line 1: The test file tokenUsageCsv.test.ts uses camelCase naming instead of
the required kebab-case convention specified in the coding guidelines. Rename
the file from tokenUsageCsv.test.ts to token-usage-csv.test.ts to follow the
kebab-case naming standard for TypeScript files.
In `@src/__tests__/workflowSpans.test.ts`:
- Around line 1023-1047: The test for the judge_stage span path only validates
the positive case where step.tags are provided. Add a negative test case
following the same pattern to verify that when step.tags are not set on the step
passed to recordJudgeStageSpan, the takt.step.tags attribute is not attached to
the judge span. This ensures consistency with the contract testing approach used
in runWithPhaseSpan tests and prevents contract misalignment between branches.
In `@tools/token-usage.sh`:
- Around line 160-163: The CSV output is missing proper cell escaping which
causes column misalignment when values contain commas, quotes, or newlines. The
code directly joins values with commas without escaping special characters that
may appear in the user-provided persona and tags fields. Before joining the
array in the console.log statement within the nested loop iterating through
run.steps, apply RFC 4180 CSV escaping to each value by wrapping values
containing commas, quotes, or newlines in double quotes, and escaping any
internal quotes by doubling them.
🪄 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: f0c42266-a2ee-4c2f-93b1-8d4a291a2de5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (7)
src/__tests__/phaseUsageEvent.test.tssrc/__tests__/tokenUsageCsv.test.tssrc/__tests__/usageEventsSpanProcessor.test.tssrc/__tests__/workflowSpans.test.tssrc/core/logging/phaseUsageEvent.tssrc/core/workflow/observability/workflowSpans.tstools/token-usage.sh
Comment on lines
+283
to
+362
| it('ignores a non-array tags attribute', () => { | ||
| // Given a phase span whose tags attribute is malformed (not an array) | ||
| const span: SpanSnapshot = { | ||
| name: 'phase.implement.execute', | ||
| endTime: [1_778_777_205, 0], | ||
| attributes: { | ||
| 'takt.provider.name': 'codex', | ||
| 'takt.model.name': 'gpt-5', | ||
| 'takt.step.name': 'implement', | ||
| 'takt.step.type': 'agent', | ||
| 'takt.step.tags': 'coding', | ||
| 'takt.phase.number': 1, | ||
| 'takt.phase.name': 'execute', | ||
| 'takt.phase.status': 'done', | ||
| 'gen_ai.usage.input_tokens': 11, | ||
| 'gen_ai.usage.output_tokens': 7, | ||
| 'gen_ai.usage.total_tokens': 18, | ||
| }, | ||
| }; | ||
|
|
||
| // When the span is mapped to a phase usage event record | ||
| const record = mapSpanEndToPhaseUsageEvent(span, context); | ||
|
|
||
| // Then the malformed value is rejected rather than passed through | ||
| expect(record).not.toHaveProperty('tags'); | ||
| }); | ||
|
|
||
| it('ignores a tags array containing a non-string element', () => { | ||
| // Given a phase span whose tags array carries a non-string element | ||
| const span: SpanSnapshot = { | ||
| name: 'phase.implement.execute', | ||
| endTime: [1_778_777_205, 0], | ||
| attributes: { | ||
| 'takt.provider.name': 'codex', | ||
| 'takt.model.name': 'gpt-5', | ||
| 'takt.step.name': 'implement', | ||
| 'takt.step.type': 'agent', | ||
| 'takt.step.tags': ['coding', 42], | ||
| 'takt.phase.number': 1, | ||
| 'takt.phase.name': 'execute', | ||
| 'takt.phase.status': 'done', | ||
| 'gen_ai.usage.input_tokens': 11, | ||
| 'gen_ai.usage.output_tokens': 7, | ||
| 'gen_ai.usage.total_tokens': 18, | ||
| }, | ||
| }; | ||
|
|
||
| // When the span is mapped to a phase usage event record | ||
| const record = mapSpanEndToPhaseUsageEvent(span, context); | ||
|
|
||
| // Then the whole array is rejected rather than partially emitted | ||
| expect(record).not.toHaveProperty('tags'); | ||
| }); | ||
|
|
||
| it('ignores a tags array containing an empty string element', () => { | ||
| // Given a phase span whose tags array carries an empty string element | ||
| const span: SpanSnapshot = { | ||
| name: 'phase.implement.execute', | ||
| endTime: [1_778_777_205, 0], | ||
| attributes: { | ||
| 'takt.provider.name': 'codex', | ||
| 'takt.model.name': 'gpt-5', | ||
| 'takt.step.name': 'implement', | ||
| 'takt.step.type': 'agent', | ||
| 'takt.step.tags': ['coding', ''], | ||
| 'takt.phase.number': 1, | ||
| 'takt.phase.name': 'execute', | ||
| 'takt.phase.status': 'done', | ||
| 'gen_ai.usage.input_tokens': 11, | ||
| 'gen_ai.usage.output_tokens': 7, | ||
| 'gen_ai.usage.total_tokens': 18, | ||
| }, | ||
| }; | ||
|
|
||
| // When the span is mapped to a phase usage event record | ||
| const record = mapSpanEndToPhaseUsageEvent(span, context); | ||
|
|
||
| // Then the whole array is rejected rather than partially emitted | ||
| expect(record).not.toHaveProperty('tags'); | ||
| }); |
There was a problem hiding this comment.
takt.step.tags の境界値として null / object 形状も追加してください。
今回の負例は十分に近いですが、ガイドライン指定の null / object 形状が未カバーです。パーサ境界の契約回帰を防ぐため、この2ケースも tags 非出力を明示的に固定するのが安全です。
✅ 追加イメージ（例）
+ it('ignores a null tags attribute', () => {
+ const span: SpanSnapshot = {
+ name: 'phase.implement.execute',
+ endTime: [1_778_777_205, 0],
+ attributes: {
+ 'takt.provider.name': 'codex',
+ 'takt.step.name': 'implement',
+ 'takt.step.type': 'agent',
+ 'takt.step.tags': null as unknown as string[],
+ 'takt.phase.number': 1,
+ 'takt.phase.name': 'execute',
+ 'takt.phase.status': 'done',
+ 'gen_ai.usage.input_tokens': 1,
+ 'gen_ai.usage.output_tokens': 1,
+ 'gen_ai.usage.total_tokens': 2,
+ },
+ };
+ const record = mapSpanEndToPhaseUsageEvent(span, context);
+ expect(record).not.toHaveProperty('tags');
+ });
+
+ it('ignores an object-shaped tags attribute', () => {
+ const span: SpanSnapshot = {
+ name: 'phase.implement.execute',
+ endTime: [1_778_777_205, 0],
+ attributes: {
+ 'takt.provider.name': 'codex',
+ 'takt.step.name': 'implement',
+ 'takt.step.type': 'agent',
+ 'takt.step.tags': ({ tag: 'coding' } as unknown as string[]),
+ 'takt.phase.number': 1,
+ 'takt.phase.name': 'execute',
+ 'takt.phase.status': 'done',
+ 'gen_ai.usage.input_tokens': 1,
+ 'gen_ai.usage.output_tokens': 1,
+ 'gen_ai.usage.total_tokens': 2,
+ },
+ };
+ const record = mapSpanEndToPhaseUsageEvent(span, context);
+ expect(record).not.toHaveProperty('tags');
+ });As per coding guidelines, "At config/parse boundaries, cover unexpected shapes (missing values, null/object instead of array) to confirm normalization/ignore behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/phaseUsageEvent.test.ts` around lines 283 - 362, The test
coverage for takt.step.tags validation is incomplete and missing boundary cases
for null and object shapes. Add two additional test cases following the same
pattern as the existing tests: one test where takt.step.tags is set to null and
another where it is set to an object shape (instead of an array), and verify in
both cases that the mapSpanEndToPhaseUsageEvent function ignores these malformed
values by asserting the record does not have a tags property. This ensures the
parser properly handles unexpected value types at the config/parse boundary.
Source: Coding guidelines
| @@ -0,0 +1,96 @@ | |||
| import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; | |||
There was a problem hiding this comment.
ファイル名は kebab-case に従う必要があります。
**/*.ts ファイルのコーディングガイドラインでは "Use kebab-case for filenames with focused module names" と明記されています。現在の tokenUsageCsv.test.ts は camelCase であり、token-usage-csv.test.ts に変更してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/tokenUsageCsv.test.ts` at line 1, The test file
tokenUsageCsv.test.ts uses camelCase naming instead of the required kebab-case
convention specified in the coding guidelines. Rename the file from
tokenUsageCsv.test.ts to token-usage-csv.test.ts to follow the kebab-case naming
standard for TypeScript files.
Source: Coding guidelines
Comment on lines
+1023
to
+1047
| it('sets step tags on judge stage spans for phase usage event filtering', async () => { | ||
| const { module, spans } = await loadWorkflowSpansWithMockedApi(); | ||
| const step = makeStep({ personaDisplayName: 'conductor', tags: ['review'] }); | ||
|
|
||
| module.recordJudgeStageSpan({ | ||
| enabled: true, | ||
| workflowName: 'test-workflow', | ||
| step, | ||
| iteration: 1, | ||
| phaseExecutionId: 'implement:3:1:1', | ||
| entry: { | ||
| stage: 1, | ||
| method: 'structured_output', | ||
| status: 'done', | ||
| instruction: 'judge instruction', | ||
| response: 'judge response', | ||
| }, | ||
| }); | ||
|
|
||
| const judgeSpan = findSpan(spans, 'judge_stage.implement.1.structured_output'); | ||
| expect(judgeSpan.attributes).toMatchObject({ | ||
| 'takt.step.persona': 'conductor', | ||
| 'takt.step.tags': ['review'], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
judge_stage 経路でも tags 未設定時の非出力契約を追加してください。
現状は judge_stage.* の正例のみで、step.tags が無い場合に takt.step.tags が付与されないことが未検証です。runWithPhaseSpan と同様の負例を1件追加して、分岐間の契約ずれを防いでください。
✅ 追加イメージ（例）
+ it('omits step tags on judge stage spans when the step has no tags', async () => {
+ const { module, spans } = await loadWorkflowSpansWithMockedApi();
+ const step = makeStep({ personaDisplayName: 'conductor' });
+
+ module.recordJudgeStageSpan({
+ enabled: true,
+ workflowName: 'test-workflow',
+ step,
+ iteration: 1,
+ phaseExecutionId: 'implement:3:1:1',
+ entry: {
+ stage: 1,
+ method: 'structured_output',
+ status: 'done',
+ instruction: 'judge instruction',
+ response: 'judge response',
+ },
+ });
+
+ const judgeSpan = findSpan(spans, 'judge_stage.implement.1.structured_output');
+ expect(judgeSpan.attributes).not.toHaveProperty('takt.step.tags');
+ });As per coding guidelines, "When contract is consolidated/standardized ... verify existing equivalent branches still satisfy the new contract—not only the newly added path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/workflowSpans.test.ts` around lines 1023 - 1047, The test for
the judge_stage span path only validates the positive case where step.tags are
provided. Add a negative test case following the same pattern to verify that
when step.tags are not set on the step passed to recordJudgeStageSpan, the
takt.step.tags attribute is not attached to the judge span. This ensures
consistency with the contract testing approach used in runWithPhaseSpan tests
and prevents contract misalignment between branches.
Source: Coding guidelines
Comment on lines
+160
to
+163
| console.log("task,run_id,provider,model,step,persona,tags,input_tokens,output_tokens,total_tokens,cached_tokens,calls"); | ||
| for (const run of runs.slice(0, top)) { | ||
| for (const [step, s] of [...run.steps.entries()].sort((a, b) => b[1].total - a[1].total)) { | ||
| console.log([run.task || "-", run.run_id, run.provider, run.model, step, s.input, s.output, s.total, s.cached, s.count].join(",")); | ||
| console.log([run.task || "-", run.run_id, run.provider, run.model, step, s.persona || "", s.tags.join("|"), s.input, s.output, s.total, s.cached, s.count].join(",")); |
There was a problem hiding this comment.
CSVセルのエスケープがなく、persona/tags で列崩れします
join(",") のみだと、値に , / " / 改行が含まれた時点でCSV契約が壊れます。今回追加した persona と tags は自由入力由来なので、RFC 4180 相当のエスケープを入れてから連結してください。
差分案
+function csvCell(v) {
+ const s = String(v ?? "");
+ return /[",\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s;
+}
+
if (csv) {
console.log("task,run_id,provider,model,step,persona,tags,input_tokens,output_tokens,total_tokens,cached_tokens,calls");
for (const run of runs.slice(0, top)) {
for (const [step, s] of [...run.steps.entries()].sort((a, b) => b[1].total - a[1].total)) {
- console.log([run.task || "-", run.run_id, run.provider, run.model, step, s.persona || "", s.tags.join("|"), s.input, s.output, s.total, s.cached, s.count].join(","));
+ console.log([
+ run.task || "-",
+ run.run_id,
+ run.provider,
+ run.model,
+ step,
+ s.persona || "",
+ s.tags.join("|"),
+ s.input,
+ s.output,
+ s.total,
+ s.cached,
+ s.count,
+ ].map(csvCell).join(","));
}
}
process.exit(0);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/token-usage.sh` around lines 160 - 163, The CSV output is missing
proper cell escaping which causes column misalignment when values contain
commas, quotes, or newlines. The code directly joins values with commas without
escaping special characters that may appear in the user-provided persona and
tags fields. Before joining the array in the console.log statement within the
nested loop iterating through run.steps, apply RFC 4180 CSV escaping to each
value by wrapping values containing commas, quotes, or newlines in double
quotes, and escaping any internal quotes by doubling them.
Owner
Author
|
Superseded by #1033, which incorporates the preferred implementation and additional CSV grouping, escaping, validation, and documentation improvements. |
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
概要
現在の
.phase.jsonl（phase usage event）にはstepとstep_typeしかなく、ステップのtagsやpersonaが含まれていない。集計・分析時に「review 系ステップだけ」「coder ペルソナのトークン消費」といったフィルタができるよう、これらのフィールドを追加する。
やること
UsageEventsSpanProcessorでスパン属性からtagsとpersonaを取得し、phase usage event に書き出すtakt.step.tagsとtakt.step.persona属性を設定するtools/token-usage.shの CSV 出力にもtagsとpersonaカラムを追加する追加フィールド
{ ...既存フィールド, "tags": ["coding", "review"], "persona": "coder" }関連
Execution Report
Workflow
takt-defaultcompleted successfully.Closes #870
Summary by CodeRabbit
リリースノート
新機能
テスト