Feat: add operator scheduling analysis skills - #888
Conversation
📝 WalkthroughWalkthroughAdded three operator skills and CLI report generators for critical-path analysis, producer-side early dispatch, and sibling suppression through one dummy dependency. The tools validate level-4 artifacts, classify scheduling evidence, compare captures, and emit Markdown or JSON results. ChangesScheduling analysis and control
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Level4Artifacts
participant CriticalPathCLI
participant EarlyDispatchCLI
participant AddDummyCLI
Operator->>Level4Artifacts: capture level-4 artifacts
CriticalPathCLI->>Level4Artifacts: validate and analyze paths
CriticalPathCLI-->>Operator: report blockers and early dispatch
EarlyDispatchCLI->>Level4Artifacts: compare producer annotations
EarlyDispatchCLI-->>Operator: report dispatch changes
AddDummyCLI->>Level4Artifacts: compare sibling topology and timing
AddDummyCLI-->>Operator: report suppression decision
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 8
🧹 Nitpick comments (2)
.claude/skills/critical-path/scripts/report.py (1)
273-295: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the per-task finish aggregation instead of recomputing it per call.
Line 277 calls
_aggregate_max(analysis.rows_by_task, "finish_time_us")on every invocation. That aggregation scans every task and every physical row of the whole capture._observed_earlyruns once per Observed-path segment in_path_tableand again per 🐌 row in_dispatch_diagnostics, so the cost isO(path_length × total_rows)._dispatch_diagnosticsalready computes the same map at line 489 and discards it.Compute the map once per
RunAnalysisand pass it in.♻️ Proposed change
-def _observed_early(task: str, analysis: RunAnalysis, tol_us: float) -> tuple[str, int, int]: +def _observed_early( + task: str, + analysis: RunAnalysis, + tol_us: float, + finish: dict[str, float] | None = None, +) -> tuple[str, int, int]: """Return proof status and early/total physical-row counts.""" rows = analysis.rows_by_task.get(task, []) total = len(rows) - finish = _aggregate_max(analysis.rows_by_task, "finish_time_us") + if finish is None: + finish = _aggregate_max(analysis.rows_by_task, "finish_time_us")Then pass the precomputed map from
_path_tableand from_dispatch_diagnostics(which already holds it asfinishes).🤖 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 @.claude/skills/critical-path/scripts/report.py around lines 273 - 295, Update _observed_early to accept a precomputed finish-time aggregation instead of calling _aggregate_max internally. Compute that map once per RunAnalysis, pass it through each _path_table call, and reuse the existing finishes map in _dispatch_diagnostics; preserve the current predecessor and early-count behavior..claude/skills/add-dummy/scripts/report.py (1)
127-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the duplicated artifact helpers into one shared module.
This file,
.claude/skills/early-dispatch/scripts/report.py, and.claude/skills/critical-path/scripts/report.pyeach carry near-identical copies of_read_json,_task_id,_finite,_records_file,_record_directories,_program,_operator_matches,_rank_label,_rank_sort_key,_dispatch_sort_key,_name_map,_md,_resource_saturation, the artifact-completeness loop, and the rank-selection block. That is roughly 250 duplicated lines per file.The copies have already diverged.
_record_directoriesnarrows todfx_outputshere (line 153) and in early-dispatch (line 128), but not in critical-path (line 83)._name_mapraises on a missing file here (line 198) and returns{}in critical-path (line 125).Extracting the shared helpers keeps future validation-rule changes consistent across the three skills. This is not required for this PR.
Also applies to: 749-833
🤖 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 @.claude/skills/add-dummy/scripts/report.py around lines 127 - 204, Extract the duplicated artifact utilities and reporting logic from the three report.py scripts into one shared module, including _read_json, _task_id, _finite, _records_file, _record_directories, _program, _operator_matches, rank helpers, _name_map, _md, _resource_saturation, artifact validation, and rank selection. Update each report entry point to reuse the shared implementations, preserving the intended dfx_outputs discovery and missing name-map behavior consistently across all skills.
🤖 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 @.claude/skills/add-dummy/scripts/report.py:
- Line 306: Resolve the Ruff findings in the report formatting and zip call
sites: replace the ambiguous multiplication character in the message near the
joined/expected row counts with ASCII “x”, and update both zip usages around the
relevant processing logic to use itertools.pairwise. Do not change the
local-file writing code associated with the use-jsonify hint.
- Around line 411-441: Update _identity_count and _task_signature to count only
tasks whose IDs are present in run.timings, matching the timed-task filtering
already used by _select_identity. Ensure occurrence numbering and expected_total
validation consistently use this same task set across _identity_count,
_select_identity, _task_signature, and PairSnapshot.
- Around line 587-599: Update `_discover_pair` to enforce baseline precondition
5 before any source modification: inspect the baseline data for
`suppressed_task`’s existing dummy predecessors, using the same
`suppressed_dummy_predecessors` information captured by `_snapshot` and surfaced
through `_baseline_payload`. Reject the pair with a clear `ValueError` when any
direct dummy predecessor already exists, while preserving the existing checks
for preconditions 1–4.
In @.claude/skills/critical-path/scripts/report.py:
- Line 223: Resolve all Ruff findings in report.py: replace the ambiguous
multiplication sign in the joined/expected diagnostic near the row-count
reporting logic, use itertools.pairwise in the sequence comparison at the zip
call near line 453, and replace ambiguous en dashes in the messages near lines
466 and 472 with unambiguous characters or wording. Preserve the existing
validation behavior and output meaning.
- Around line 233-234: Make the critical_path API available before report.py
invokes critical_path.build_graph and critical_path.analyze_rank: import the
repository’s canonical implementation or define a compatible module exposing
those functions and the graph/result/segment fields consumed by the report.
Ensure the skill can resolve this module at runtime without changing the
existing analysis flow.
In @.claude/skills/early-dispatch/scripts/report.py:
- Line 289: Resolve the Ruff findings in report.py: replace the ambiguous
Unicode operators and dashes at the referenced expressions with appropriate
ASCII equivalents, and update the iteration at line 466 to use
itertools.pairwise(ordered), preserving its current behavior. Leave the ast-grep
use-jsonify hint unchanged because this code writes a local file.
- Around line 1040-1044: Validate that the baseline loaded by the comparison
CLIs is a dictionary before calling .get: add this check in
.claude/skills/early-dispatch/scripts/report.py lines 1040-1044 and
.claude/skills/add-dummy/scripts/report.py lines 1232-1239, raising ValueError
for any other JSON type. In the early-dispatch comparison branch at line 1073,
test baseline is not None rather than truthiness so an empty object is still
processed.
- Around line 399-405: Update the peer selection in `_task_signature` to include
only tasks present in `run.rows_by_task`, matching the candidate filtering used
by `_matching_tasks`. Keep the existing name comparison, sorting, occurrence
lookup, and signature formatting unchanged.
---
Nitpick comments:
In @.claude/skills/add-dummy/scripts/report.py:
- Around line 127-204: Extract the duplicated artifact utilities and reporting
logic from the three report.py scripts into one shared module, including
_read_json, _task_id, _finite, _records_file, _record_directories, _program,
_operator_matches, rank helpers, _name_map, _md, _resource_saturation, artifact
validation, and rank selection. Update each report entry point to reuse the
shared implementations, preserving the intended dfx_outputs discovery and
missing name-map behavior consistently across all skills.
In @.claude/skills/critical-path/scripts/report.py:
- Around line 273-295: Update _observed_early to accept a precomputed
finish-time aggregation instead of calling _aggregate_max internally. Compute
that map once per RunAnalysis, pass it through each _path_table call, and reuse
the existing finishes map in _dispatch_diagnostics; preserve the current
predecessor and early-count behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed30e057-aeb7-4735-b9ca-816f2c6efad5
📒 Files selected for processing (9)
.claude/skills/add-dummy/SKILL.md.claude/skills/add-dummy/agents/openai.yaml.claude/skills/add-dummy/scripts/report.py.claude/skills/critical-path/SKILL.md.claude/skills/critical-path/agents/openai.yaml.claude/skills/critical-path/scripts/report.py.claude/skills/early-dispatch/SKILL.md.claude/skills/early-dispatch/agents/openai.yaml.claude/skills/early-dispatch/scripts/report.py
| if actual_rows != expected_rows: | ||
| raise ValueError( | ||
| f"{records}: incomplete task {task} rows " | ||
| f"(joined={actual_rows}, expected={logical_blocks}×{active_slots}={expected_rows})" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff findings.
Ruff 0.16.0 reports RUF001 ambiguous × at line 306, and B905 plus RUF007 at lines 559 and 566. Replace × with x and use itertools.pairwise at both zip sites.
The ast-grep use-jsonify hint at line 1266 does not apply. This code writes a local file.
🔧 Proposed change
+import itertools
@@
-def _observed_edges(run: Run) -> list[tuple[str, str]]:
- path = [segment.task for segment in run.result.segments]
- return list(zip(path, path[1:]))
+def _observed_edges(run: Run) -> list[tuple[str, str]]:
+ path = [segment.task for segment in run.result.segments]
+ return list(itertools.pairwise(path))
@@
- return [
- (previous.task, current.task)
- for previous, current in zip(segments, segments[1:])
- if current.kind == "data-wait"
- ]
+ return [
+ (previous.task, current.task)
+ for previous, current in itertools.pairwise(segments)
+ if current.kind == "data-wait"
+ ]Also applies to: 559-566
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 306-306: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 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 @.claude/skills/add-dummy/scripts/report.py at line 306, Resolve the Ruff
findings in the report formatting and zip call sites: replace the ambiguous
multiplication character in the message near the joined/expected row counts with
ASCII “x”, and update both zip usages around the relevant processing logic to
use itertools.pairwise. Do not change the local-file writing code associated
with the use-jsonify hint.
Source: Linters/SAST tools
| def _identity_count(run: Run, names: list[str]) -> int: | ||
| return sum(candidate_names == names for candidate_names in run.names.values()) | ||
|
|
||
|
|
||
| def _select_identity( | ||
| run: Run, | ||
| names: list[str], | ||
| occurrence: int, | ||
| expected_total: int, | ||
| role: str, | ||
| ) -> tuple[int, str]: | ||
| actual_total = _identity_count(run, names) | ||
| if actual_total != expected_total: | ||
| raise ValueError( | ||
| f"{run.directory}: {role} identity {'/'.join(names)!r} occurrence count changed " | ||
| f"from {expected_total} to {actual_total}" | ||
| ) | ||
| matches = sorted( | ||
| ( | ||
| task | ||
| for task, candidate_names in run.names.items() | ||
| if task in run.timings and candidate_names == names | ||
| ), | ||
| key=int, | ||
| ) | ||
| if occurrence < 0 or occurrence >= len(matches): | ||
| raise ValueError( | ||
| f"{run.directory}: {role} identity {'/'.join(names)!r} occurrence {occurrence} " | ||
| f"is outside 0..{len(matches) - 1}" | ||
| ) | ||
| return occurrence, matches[occurrence] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make _identity_count and _select_identity count the same task set.
_identity_count at line 412 counts every entry in run.names. _select_identity at line 428 builds matches only from tasks present in run.timings. A task can be in run.names and absent from run.timings: _build_run accepts actual_rows == expected_rows == 0, which holds for any task whose kernel_ids are all negative.
Two effects follow. First, expected_total compared at line 423 uses the untimed-inclusive count while the occurrence index at line 436 is bounded by the timed-only count, so a valid occurrence can be rejected as "outside 0..N". Second, _task_signature at line 486 uses the same untimed-inclusive numbering, which then disagrees with the occurrence value stored in PairSnapshot.
Filter _identity_count and _task_signature by run.timings so all four functions share one definition of occurrence.
🐛 Proposed fix
def _identity_count(run: Run, names: list[str]) -> int:
- return sum(candidate_names == names for candidate_names in run.names.values())
+ return sum(
+ candidate in run.timings and candidate_names == names
+ for candidate, candidate_names in run.names.items()
+ ) names = run.names.get(task, ["unknown"])
peers = sorted(
- (candidate for candidate, candidate_names in run.names.items() if candidate_names == names),
+ (
+ candidate
+ for candidate, candidate_names in run.names.items()
+ if candidate in run.timings and candidate_names == names
+ ),
key=int,
)🤖 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 @.claude/skills/add-dummy/scripts/report.py around lines 411 - 441, Update
_identity_count and _task_signature to count only tasks whose IDs are present in
run.timings, matching the timed-task filtering already used by _select_identity.
Ensure occurrence numbering and expected_total validation consistently use this
same task set across _identity_count, _select_identity, _task_signature, and
PairSnapshot.
| suppressed_status = _early_status(run, suppressed_task) | ||
| if suppressed_status.observed not in {"full", "partial"}: | ||
| raise ValueError( | ||
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) " | ||
| f"is not actually early-dispatched ({suppressed_status.observed}); do not add a dummy" | ||
| ) | ||
|
|
||
| path_tasks = {segment.task for segment in run.result.segments} | ||
| if suppressed_task in path_tasks: | ||
| raise ValueError( | ||
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) is on the " | ||
| "Observed critical path; adding a dummy to it is unsafe" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce baseline precondition 5 in _discover_pair.
.claude/skills/add-dummy/SKILL.md line 128 states that the helper must prove all five baseline preconditions before source modification, and line 138 lists "b does not already have a dummy predecessor". _discover_pair checks preconditions 1 through 4 but not this one. _snapshot records suppressed_dummy_predecessors at line 718, and _baseline_payload never inspects it.
The current failure path is late and expensive. The operator edits source, rebuilds, submits a new NPU run, and only then does _validate_comparison line 1107 raise "expected exactly one direct dummy predecessor after the edit". Reject the baseline instead.
🐛 Proposed fix
path_tasks = {segment.task for segment in run.result.segments}
if suppressed_task in path_tasks:
raise ValueError(
f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) is on the "
"Observed critical path; adding a dummy to it is unsafe"
)
+
+ existing_dummies = sorted(
+ (pred for pred in run.predecessors.get(suppressed_task, set()) if _is_dummy(pred, run)),
+ key=int,
+ )
+ if existing_dummies:
+ raise ValueError(
+ f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) already has "
+ f"dummy predecessor(s) {', '.join(existing_dummies)}; adding another is out of scope"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suppressed_status = _early_status(run, suppressed_task) | |
| if suppressed_status.observed not in {"full", "partial"}: | |
| raise ValueError( | |
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) " | |
| f"is not actually early-dispatched ({suppressed_status.observed}); do not add a dummy" | |
| ) | |
| path_tasks = {segment.task for segment in run.result.segments} | |
| if suppressed_task in path_tasks: | |
| raise ValueError( | |
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) is on the " | |
| "Observed critical path; adding a dummy to it is unsafe" | |
| ) | |
| suppressed_status = _early_status(run, suppressed_task) | |
| if suppressed_status.observed not in {"full", "partial"}: | |
| raise ValueError( | |
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) " | |
| f"is not actually early-dispatched ({suppressed_status.observed}); do not add a dummy" | |
| ) | |
| path_tasks = {segment.task for segment in run.result.segments} | |
| if suppressed_task in path_tasks: | |
| raise ValueError( | |
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) is on the " | |
| "Observed critical path; adding a dummy to it is unsafe" | |
| ) | |
| existing_dummies = sorted( | |
| (pred for pred in run.predecessors.get(suppressed_task, set()) if _is_dummy(pred, run)), | |
| key=int, | |
| ) | |
| if existing_dummies: | |
| raise ValueError( | |
| f"{run.directory}: suppressed task {suppressed_task} ({_name(suppressed_task, run)}) already has " | |
| f"dummy predecessor(s) {', '.join(existing_dummies)}; adding another is out of scope" | |
| ) |
🤖 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 @.claude/skills/add-dummy/scripts/report.py around lines 587 - 599, Update
`_discover_pair` to enforce baseline precondition 5 before any source
modification: inspect the baseline data for `suppressed_task`’s existing dummy
predecessors, using the same `suppressed_dummy_predecessors` information
captured by `_snapshot` and surfaced through `_baseline_payload`. Reject the
pair with a clear `ValueError` when any direct dummy predecessor already exists,
while preserving the existing checks for preconditions 1–4.
| if actual_rows != expected_rows: | ||
| raise ValueError( | ||
| f"{records}: incomplete task {task} rows " | ||
| f"(joined={actual_rows}, expected={logical_blocks}×{active_slots}={expected_rows})" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff findings.
The PR objectives state that validation includes Ruff checks. Ruff 0.16.0 reports the following in this file:
- Line 223: RUF001 ambiguous
×(MULTIPLICATION SIGN). - Line 453: B905
zip()withoutstrict=, and RUF007 preferitertools.pairwise(). - Lines 466 and 472: RUF001 ambiguous
–(EN DASH).
itertools.pairwise resolves both line-453 findings at once.
🔧 Proposed change
+import itertools
@@
- f"(joined={actual_rows}, expected={logical_blocks}×{active_slots}={expected_rows})"
+ f"(joined={actual_rows}, expected={logical_blocks}x{active_slots}={expected_rows})"
@@
- for left, right in zip(ordered, ordered[1:]):
+ for left, right in itertools.pairwise(ordered):
@@
- f"full-engine saturation not proven: {target_type} core {core} had a free "
- f"descriptor slot during {left:.3f}–{right:.3f} µs"
+ f"full-engine saturation not proven: {target_type} core {core} had a free "
+ f"descriptor slot during {left:.3f}-{right:.3f} µs"
@@
- f"descriptor slots occupied throughout {window_start_us:.3f}–{window_end_us:.3f} µs"
+ f"descriptor slots occupied throughout {window_start_us:.3f}-{window_end_us:.3f} µs"Also applies to: 453-453, 466-472
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 223-223: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 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 @.claude/skills/critical-path/scripts/report.py at line 223, Resolve all Ruff
findings in report.py: replace the ambiguous multiplication sign in the
joined/expected diagnostic near the row-count reporting logic, use
itertools.pairwise in the sequence comparison at the zip call near line 453, and
replace ambiguous en dashes in the messages near lines 466 and 472 with
unambiguous characters or wording. Preserve the existing validation behavior and
output meaning.
Source: Linters/SAST tools
| graph = critical_path.build_graph(directory, root, tol) | ||
| result = critical_path.analyze_rank(graph, tol) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the canonical critical_path module and confirm the API used by the skill scripts.
set -euo pipefail
fd -t f 'critical_path.py' | while IFS= read -r f; do
echo "=== $f ==="
ast-grep outline "$f" --items all
done
# Confirm the segment/result/graph attribute names.
rg -nP -C3 '\b(build_graph|analyze_rank|cpm_len|makespan|segments)\b' --type=py -g '!.claude/**'Repository: hw-native-sys/pypto-lib
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== matching files ==="
git ls-files | rg '(^|/)(critical_path\.py|report\.py)$|critical-path' || true
echo "=== critical_path.py candidates ==="
fd -t f 'critical_path\.py$' . || true
echo "=== report.py candidates ==="
fd -t f 'report\.py$' . || true
echo "=== API symbol searches ==="
rg -nP -C2 '\b(build_graph|analyze_rank|cpm_len|makespan|segments|critical_path|analysis\.graph\.freq|result\.freq)\b' --type=py --glob '!/.git/**' || trueRepository: hw-native-sys/pypto-lib
Length of output: 501
Define or import the missing critical_path API.
critical-path/scripts/report.py calls critical_path.build_graph, critical_path.analyze_rank, and reads graph.freq/.name plus result/segment fields, but no critical_path.py or compatible module exists in the repository. Either import the canonical implementation into the skill or define these symbols before this script runs, otherwise operator use of the skill fails at runtime.
🤖 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 @.claude/skills/critical-path/scripts/report.py around lines 233 - 234, Make
the critical_path API available before report.py invokes
critical_path.build_graph and critical_path.analyze_rank: import the
repository’s canonical implementation or define a compatible module exposing
those functions and the graph/result/segment fields consumed by the report.
Ensure the skill can resolve this module at runtime without changing the
existing analysis flow.
| if actual_rows != expected_rows: | ||
| raise ValueError( | ||
| f"{records}: incomplete task {task} rows " | ||
| f"(joined={actual_rows}, expected={logical_blocks}×{active_slots}={expected_rows})" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff findings.
Ruff 0.16.0 reports RUF001 ambiguous characters at lines 289 (×), 773 (−), 827 (–), 834 (–), and 931 (−), plus B905 and RUF007 at line 466. Replace the ambiguous characters with ASCII equivalents and use itertools.pairwise(ordered) at line 466.
The ast-grep use-jsonify hint at line 1080 does not apply. This code writes a local file; it does not emit an HTTP response.
Also applies to: 466-466, 773-773
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 289-289: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 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 @.claude/skills/early-dispatch/scripts/report.py at line 289, Resolve the
Ruff findings in report.py: replace the ambiguous Unicode operators and dashes
at the referenced expressions with appropriate ASCII equivalents, and update the
iteration at line 466 to use itertools.pairwise(ordered), preserving its current
behavior. Leave the ast-grep use-jsonify hint unchanged because this code writes
a local file.
Source: Linters/SAST tools
| peers = sorted( | ||
| (candidate for candidate, candidate_names in run.names.items() if candidate_names == names), | ||
| key=int, | ||
| ) | ||
| occurrence = peers.index(task) | ||
| rows = len(run.rows_by_task.get(task, [])) | ||
| return f"{'/'.join(names)}#{occurrence}|logical={logical_blocks}|slots={active_slots}|rows={rows}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the occurrence index in _task_signature with the one used for --occurrence.
Line 400 builds peers from every entry in run.names. _matching_tasks at line 330 builds its candidate list only from tasks present in run.rows_by_task. A task can be in run.names but not in run.rows_by_task: _build_run accepts actual_rows == expected_rows == 0, which is the case for a task_dummy with all-negative kernel_ids.
The two indices then disagree. TargetSnapshot.occurrence (line 593) carries the timed index that the user passes back as --occurrence N, while task_signature embeds the untimed-inclusive index. When the after build changes the untimed/timed split, _render_comparison line 914 rejects the run with "changed logical block/kernel-slot signature" even though the target identity did not change.
Filter peers the same way as _matching_tasks.
🐛 Proposed fix
peers = sorted(
- (candidate for candidate, candidate_names in run.names.items() if candidate_names == names),
+ (
+ candidate
+ for candidate, candidate_names in run.names.items()
+ if candidate in run.rows_by_task and candidate_names == names
+ ),
key=int,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| peers = sorted( | |
| (candidate for candidate, candidate_names in run.names.items() if candidate_names == names), | |
| key=int, | |
| ) | |
| occurrence = peers.index(task) | |
| rows = len(run.rows_by_task.get(task, [])) | |
| return f"{'/'.join(names)}#{occurrence}|logical={logical_blocks}|slots={active_slots}|rows={rows}" | |
| peers = sorted( | |
| ( | |
| candidate | |
| for candidate, candidate_names in run.names.items() | |
| if candidate in run.rows_by_task and candidate_names == names | |
| ), | |
| key=int, | |
| ) | |
| occurrence = peers.index(task) | |
| rows = len(run.rows_by_task.get(task, [])) | |
| return f"{'/'.join(names)}#{occurrence}|logical={logical_blocks}|slots={active_slots}|rows={rows}" |
🤖 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 @.claude/skills/early-dispatch/scripts/report.py around lines 399 - 405,
Update the peer selection in `_task_signature` to include only tasks present in
`run.rows_by_task`, matching the candidate filtering used by `_matching_tasks`.
Keep the existing name comparison, sorting, occurrence lookup, and signature
formatting unchanged.
| if args.baseline_json: | ||
| baseline_path = args.baseline_json.expanduser().resolve() | ||
| baseline = _read_json(baseline_path) | ||
| if baseline.get("version") != BASELINE_VERSION: | ||
| raise ValueError(f"{baseline_path}: unsupported baseline version") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Both comparison CLIs read the baseline snapshot without checking its type. Each script calls _read_json on the --baseline-json path and then calls .get on the result. If the file holds a list, a string, or null, .get raises AttributeError, which neither except clause catches. The CLI prints a traceback instead of error: ... with exit code 2. An empty JSON object is also falsy, so the comparison branch is skipped silently.
.claude/skills/early-dispatch/scripts/report.py#L1040-L1044: after line 1042, raiseValueErrorwhenbaselineis not adict; at line 1073, testbaseline is not Noneinstead of truthiness..claude/skills/add-dummy/scripts/report.py#L1232-L1239: after line 1236, raiseValueErrorwhenbaselineis not adict.
📍 Affects 2 files
.claude/skills/early-dispatch/scripts/report.py#L1040-L1044(this comment).claude/skills/add-dummy/scripts/report.py#L1232-L1239
🤖 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 @.claude/skills/early-dispatch/scripts/report.py around lines 1040 - 1044,
Validate that the baseline loaded by the comparison CLIs is a dictionary before
calling .get: add this check in .claude/skills/early-dispatch/scripts/report.py
lines 1040-1044 and .claude/skills/add-dummy/scripts/report.py lines 1232-1239,
raising ValueError for any other JSON type. In the early-dispatch comparison
branch at line 1073, test baseline is not None rather than truthiness so an
empty object is still processed.
1 participant
Summary
/critical-pathto extract the runtimecritical_path.pyObserved Path from level-4 swimlanes, mark gaps over 1 us and early-dispatched tasks, and report proven blockers/early-dispatchto annotate every direct producer safely, compare fixed-rank before/after captures, prove scheduler early dispatch, and show predecessor-end and target-start timestamps/add-dummyto prevent unwanted sibling early dispatch with a dummy dependency and measure the resulting critical-edge timingValidation
ruff checkon all three report helpersquick_validate.pyon all three skill directoriestask-submiton NPU