Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
--port/--timeoutreceives a non-numeric value,Number(...)becomesNaNand later… · NewArray.shift()is O(n) and will copy elements each time the buffer overflows. With frequent events… · Newcallsarray and closure per listener on every event. For… · Newserver.close()is async; not awaiting its completion can leave an open handle briefly and can… · New
What changed in this PR
Adds an “any event” listener API to TcfChannel and introduces a repo-local TCF CLI + skill documentation to help run/inspect TCF against a live agent (e.g., Simics).
Changes:
- Add
onAnyEvent()support inTcfChannel(with ordering after per-event listeners) and unit tests for it. - Add a TCF CLI script under
.github/skills/tcf/scripts/to run commands / REPL against a live TCF agent. - Wire the CLI into
package.jsonscripts and add a dedicated tsconfig + skill documentation.
| File | Description |
|---|---|
| src/tcf/channel.ts | Adds onAnyEvent() listener set and dispatch logic for all incoming events. |
| src/test/channel.test.ts | Adds unit test verifying onAnyEvent ordering and removal behavior. |
| package.json | Extends typecheck and adds npm run tcf CLI entrypoint. |
| .github/skills/tcf/scripts/tsconfig.json | Adds TS project config for the skill CLI scripts. |
| .github/skills/tcf/scripts/tcf-cli.ts | Implements a TCF CLI/REPL built on TcfChannel + service wrappers. |
| .github/skills/tcf/SKILL.md | Adds skill documentation for TCF protocol/service reference and the CLI usage. |
Review effort: Lite
aragners
left a comment
aragners
left a comment
There was a problem hiding this comment.
Lots and lots of Simics repo stuff. That is not for external github users to know about.
Then I am far from an expert at skills. My AI agent manages this very well without this skill file. Can it be shortened? Seems very long. I assume references to TCF commands etc is good.
aragners
left a comment
aragners
left a comment
There was a problem hiding this comment.
The SKILLS file now looks ok to me. Haven't checked the tcf-cli part as Erasmus seems to have reviewed that.
The Simics internal repo issues are now found. No further comments on SKILLS file from me.
A client that records or shows agent traffic needs all events, also those of services and events it does not know by name. Simics services send events that are not in any spec, so listening to a fixed list of names is not enough. The trace callback sees every message, but only as text with the fields joined by spaces. The JSON arguments cannot be parsed back from that text, for example when a string contains a space. The new listeners run after the listeners of the event itself, so those have updated their state first. The tests with a local TCP server now wait until the server has closed. server.close() only starts closing, so a test could end with an open handle, which can make the test runner flaky. This is a preparation for a TCF skill tool added in the next commit. Signed-off-by: Claes Lillieskold <claes.lillieskold@intel.com>
| const production = process.argv.includes("--production"); | ||
| const watch = process.argv.includes("--watch"); | ||
| const tests = process.argv.includes("--tests"); | ||
| const tcfCli = process.argv.includes("--tcf-cli"); |
There was a problem hiding this comment.
I don't see a point in keeping this behind an optional flag? Also, then building is not covered by our CI jobs, unless we explicitly add a build command with that flag.
If you remove the option we get less conditions and add coverage in CI. So I think that is better. The diff here should become smaller as well.
There was a problem hiding this comment.
In that case we also remove the compile:tcf-cli npm-command (scripts entry point).
| */ | ||
| import * as fs from "node:fs"; | ||
| import * as repl from "node:repl"; | ||
| import { TcfChannel, stringifyJson } from "../src/tcf/channel"; |
There was a problem hiding this comment.
Did you get absolute imports working? I.e. either
import { TcfChannel, stringifyJson } from "/tcf/channel";or
import { TcfChannel, stringifyJson } from "@/tcf/channel";
| logLevel: "warning", | ||
| }), | ||
| ]; | ||
| await esbuild.context({ |
There was a problem hiding this comment.
Something is off with the whitespace. Otherwise it looks better
Coding agents working on the TCF client often guess what a TCF command takes and returns. The skill points them to the spec page of each service, and lists the details that are easy to get wrong: replies with the error report in a later field, 64-bit addresses, and how the Simics agent reports suspend and resume. The skill also adds a CLI (npm run tcf) that runs TCF commands on a live agent with the extension's own TcfChannel and service wrappers. An agent can then check what Simics really replies before it writes code, instead of asking the user to start a debug session with traceTcf. Cost: about 120 tokens in every chat request for the skill description, and about 1800 more only when a TCF task makes the agent read SKILL.md. Signed-off-by: Claes Lillieskold <claes.lillieskold@intel.com>
No description provided.